delete recursive=True for ensure_dir

[xiaoge1001]

Proposed Commit Message

delete recursive=True for ensure_dir

Fixes GH-5807

Additional Context

Test Steps

Merge type

• [x] Squash merge using "Proposed Commit Message"
• [ ] Rebase and merge unique commits. Requires commit messages per-commit each referencing the pull request number (#<PR_NUM>)

[TheRealFalcon]

@ani-sinha , @major , or @xiachen-rh , would any of you be able to review and/or test these changes? I don't have enough background with SELinux to know the full context.

[ani-sinha]

@ani-sinha , @major , or @xiachen-rh , would any of you be able to review and/or test these changes? I don't have enough background with SELinux to know the full context.

I don't have much experience with selinix either so can't comment on this.

[xiaoge1001]

Excuse me, may I know if anyone is concerned about this question at the moment?

[a-dubs]

Hello, @xiaoge1001 could you provide more context on what this change is doing and more importantly, why?

[xiaoge1001]

Hello, @xiaoge1001 could you provide more context on what this change is doing and more importantly, why?

1. I back up /etc/sysconfig/network-scripts/ifcfg-eth0 to the /home directory. cp -a /etc/sysconfig/network-scripts/ifcfg-eth0 /home
2. The configuration file about the cc_mounts module is added to /etc/cloud/cloud.cfg.d/ . Run the cloud-init service.
3. Run cp -a /home/ifcfg-eth0 /etc/sysconfig/network-scripts/
4. Run systemctl restart network
5. The IPv6 address of eth0 is lost.

Start to check the cause:

1. Run the systemctl status network command to check the service status. The following logs are displayed:

grep: /etc/sysconfig/network-scripts/ifcfg-eth0: Permission denied

2. Run ls -Z /etc/sysconfig/network-scripts/ifcfg-eth0. It is found that the SELinux context changes from net_conf_t to default_t.
3. Check the /var/log/cloud-init.log file. The following key logs are found: 2024-10-08 07:31:41,161 - util.py[DEBUG]: Restoring selinux mode for / (recursive=True)
4. Run chcon -t net_conf_t /etc/sysconfig/network-scripts/ifcfg-eth0 and systemctl restart network，the IPv6 address of eth0 was restored. It was confirmed that the SELinux context was changed.

[xiachen-rh]

@ani-sinha , @major , or @xiachen-rh , would any of you be able to review and/or test these changes? I don't have enough background with SELinux to know the full context.

@TheRealFalcon Sorry I don't have much experience with SELinux either. I reviewed the context of this change, I don't understand why do theses steps @xiaoge1001 , so I can't test it.

Run cp -a /home/ifcfg-eth0 to the /etc/sysconfig/network-scripts/ Run systemctl restart network

As far as l know ifcfg-eth0 is created by cloud-init and not expect to change, am I right?

$ cat /etc/sysconfig/network-scripts/ifcfg-eth0 
# Created by cloud-init automatically, do not edit.
#

[xiaoge1001]

As far as l know ifcfg-eth0 is created by cloud-init and not expect to change, am I right?

@xiachen-rh I agree with you.

I only back up the original ifcfg-eth0 in the system. After the cloud-init is running, run the cloud-init clean command to restore the original ifcfg-eth0.

It doesn't matter whether these steps are reasonable, and I can avoid this problem without modifying the cloud-init code.

My question is whether the following operations are correct? 2024-10-08 07:31:41,161 - util.py[DEBUG]: Restoring selinux mode for / (recursive=True)

if we set recursive to True when path is "/",

1. selinux.restorecon() will take a long time. （In my environment, it takes about 2 minutes）
2. there may be many files whose selinux contexts will change, such as /home/ifcfg-eth0 mentioned above. the SELinux context of these files changes, which may cause some potential problems.

[xiaoge1001]

My question is whether the following operations are correct? 2024-10-08 07:31:41,161 - util.py[DEBUG]: Restoring selinux mode for / (recursive=True)

if we set recursive to True when path is "/",

1. selinux.restorecon() will take a long time. （In my environment, it takes about 2 minutes）
2. there may be many files whose selinux contexts will change, such as /home/ifcfg-eth0 mentioned above. the SELinux context of these files changes, which may cause some potential problems.

Hello, is there anyone concerned about this?

[xiaoge1001]

https://github.com/canonical/cloud-init/blob/main/cloudinit/util.py#L1883 path = "/mnt1" https://github.com/canonical/cloud-init/blob/main/cloudinit/util.py#L195 path = "/"

"/mnt1" does not exist. What is the meaning of setting recursive=True? I understand it's the same scenario as here.

            if not os.path.exists(parent_folder):
                # directory does not exist, and permission so far are good:
                # create the directory, and make it accessible by everyone
                # but owned by root, as it might be used by many users.
                with util.SeLinuxGuard(parent_folder):

I don't think it's necessary to set recursive=True at this point.

[TheRealFalcon]

@xiaoge1001 , I haven't forgotten this issue, but it still seems we have yet to find anybody else with any real expertise here. For something that has been around so long and used by a commonly used utility function, I would really like to find at least one other person with the relevant expertise to review this.

Are you on the cloud-init mailing list? It might help asking for help there or in the cloud-init IRC channel on Libera.

[xiaoge1001]

@xiaoge1001 , I haven't forgotten this issue, but it still seems we have yet to find anybody else with any real expertise here. For something that has been around so long and used by a commonly used utility function, I would really like to find at least one other person with the relevant expertise to review this.

Are you on the cloud-init mailing list? It might help asking for help there or in the cloud-init IRC channel on Libera.

Sorry, I'm not on the mailing list.

[xiaoge1001]

from cloudinit import util
# the /mnt1 directory does not exist
util.ensure_dir("/mnt1")      # Restoring selinux mode for / (recursive=True)
util.ensure_dir("/mnt1/")     # Restoring selinux mode for /mnt1 (recursive=True)

Is that a problem?

[holmanb]

@Conan-Kudo Hey Neal, could you please review this?

[xiaoge1001]

I'm pretty sure we can't drop this, because ensure_dir() can be used for a deep hierarchy and not setting the labels from policy for the hierarchy. But this logic also doesn't make sense, because it seems like it assumes there's always only one level up? If that assumption is always true, then yes, we can drop recursive=True, but I am not sure if it's always used that way.

I don't think it's necessary for “/”. Can we add a judgment condition? If os.path.dirname(path) == "/", set recursive to False.

[github-actions[bot]]

Hello! Thank you for this proposed change to cloud-init. This pull request is now marked as stale as it has not seen any activity in 14 days. If no activity occurs within the next 7 days, this pull request will automatically close.

If you are waiting for code review and you are seeing this message, apologies! Please reply, tagging TheRealFalcon, and he will ensure that someone takes a look soon.

(If the pull request is closed and you would like to continue working on it, please do tag TheRealFalcon to reopen it.)

[TheRealFalcon]

@xiaoge1001

I don't think it's necessary for “/”. Can we add a judgment condition? If os.path.dirname(path) == "/", set recursive to False.

This sounds reasonable to me.

[xiaoge1001]

@xiaoge1001

I don't think it's necessary for “/”. Can we add a judgment condition? If os.path.dirname(path) == "/", set recursive to False.

This sounds reasonable to me.

I have made the modification.