Skip to content

Add noexec nodev and nosuid to sandbox /etc/resolv.conf mount bind.#8309

Merged
samuelkarp merged 3 commits intocontainerd:mainfrom
vinayakankugoyal:fixresolv
Mar 31, 2023
Merged

Add noexec nodev and nosuid to sandbox /etc/resolv.conf mount bind.#8309
samuelkarp merged 3 commits intocontainerd:mainfrom
vinayakankugoyal:fixresolv

Conversation

@vinayakankugoyal
Copy link
Copy Markdown
Contributor

@vinayakankugoyal vinayakankugoyal commented Mar 24, 2023 •
edited
Loading

If you are running a pod with user namespace enabled you might see EPERM errors while mounting resolv.conf without these options.

This was discovered while we were debugging: opencontainers/runc#3770

@k8s-ci-robot
Copy link
Copy Markdown

k8s-ci-robot commented Mar 24, 2023

Hi @vinayakankugoyal. Thanks for your PR.

I'm waiting for a containerd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@vinayakankugoyal
Copy link
Copy Markdown
Contributor Author

vinayakankugoyal commented Mar 24, 2023

/cc @rata

@k8s-ci-robot
Copy link
Copy Markdown

k8s-ci-robot commented Mar 24, 2023

@vinayakankugoyal: GitHub didn't allow me to request PR reviews from the following users: rata.

Note that only containerd members and repo collaborators can review this PR, and authors cannot review their own PRs.

Details

In response to this:

/cc @rata

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@vinayakankugoyal
Copy link
Copy Markdown
Contributor Author

vinayakankugoyal commented Mar 24, 2023

@rata - please take a look.

@AkihiroSuda AkihiroSuda added cherry-pick/sbserver area/cri Container Runtime Interface (CRI) labels Mar 27, 2023
@AkihiroSuda
Copy link
Copy Markdown
Member

AkihiroSuda commented Mar 27, 2023

/ok-to-test

rata
rata suggested changes Mar 27, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@rata rata left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vinayakankugoyal I'm not sure this is the right fix.

When I mentioned to add these flags in the runc issue, it was as a debugging step, to gain insight; to make sure that worked on your setup too. Now that we know that works, though, we need to see why that helps in your setup. I guess it might be because the partition we are bind-mounting the files from might be mounted with those flags. But let's continue that conversation on the runc issue.

Also, IIUC this is not enough to make userns work in COS (but you share so little that is really me guessing here, I don't really know what is happening on your setup). I think the proper course of action is to fix that first, then see what we needed to do to make that work and then see what the proper fixes are and on which projects.

For example, if my guess about why these flags are needed in your setup, then we might either want to do the changes here or in runc. There is an open PR to improve the situation in runc just in those cases (see: opencontainers/runc#3710).

So, IMHO, it is a big no to merge this with the information we currently have. As I said, we don't know it will fix your issue (still pods with userns are failing in your setup), we don't know if we want to fix this in runc or containerd, etc. It might be that we decide to merge this code. But we still lack needed information to make this decision, so it seems too soon to open a PR. And maybe we want to mention these reasons in the commit msg, maybe in the code as a comment too. But until we really understand why it fails and your setup, we don't really know what to write either. My gut feeling, also, is that even if we decide to use this workaround, it might not be enough and we might need to change the mounts in the container too, not just the sandbox.

@vinayakankugoyal
Copy link
Copy Markdown
Contributor Author

vinayakankugoyal commented Mar 27, 2023

Based on what you suggested I was looking at the containerd code to understand why this was not set for /etc/resolv.conf but set for /dev/shm. I noticed that these options were explicitly being set for /dev/shm explicitly and not setting these options for /etc/resolv.conf which seemed odd to me. I don't think there is any harm in setting these options since we already set it for /dev/shm.

@rata
Copy link
Copy Markdown
Contributor

rata commented Mar 28, 2023

Probably it doesn't hurt to add them (explaining that in the commit msg would have been good). But we don't know so far if those are even needed in your setup, with what we debugged so far we know they are needed to mount a file when we manually modify the config.json to point to a file mounted on a filesystem with those flags. Is that why the original (not manually modified) config.json fails? We don't know yet, we need to check those mount flags in the original source path.

Also, even if this does indeed help, I doubt these would be the only changes needed and not sure we want to do them in runc too (besides the link I pasted in my previous comment, it seems crun is also handling that transparently).

IMHO it is better to understand your setup, why it fails, and later open PRs making fixes, when we are certain we want those fixes and why.

@vinayakankugoyal vinayakankugoyal mentioned this pull request Mar 28, 2023
Open
@vinayakankugoyal
Copy link
Copy Markdown
Contributor Author

vinayakankugoyal commented Mar 28, 2023

Please see opencontainers/runc#3770 (comment)

Running containerd built from this PR fixed the issue that we were debugging.

@vinayakankugoyal
Copy link
Copy Markdown
Contributor Author

vinayakankugoyal commented Mar 28, 2023

/retest

rata
rata approved these changes Mar 29, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@rata rata left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, LGTM now that we know this fixes the issue! :)

Once this is merged, we will need to backport it to v1.7 too.

@rata
Copy link
Copy Markdown
Contributor

rata commented Mar 29, 2023 •
edited
Loading

@vinayakankugoyal Do you want to see if this is something simple to test? Maybe an integration test is the simplest, I don't know. So we make sure we don't regress on this.

@vinayakankugoyal
Copy link
Copy Markdown
Contributor Author

vinayakankugoyal commented Mar 29, 2023 •
edited
Loading

Added a check that the /etc/resolv.conf mount in in the desired configuration.

rata
rata approved these changes Mar 30, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@rata rata left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test also LGTM, thanks!

@samuelkarp
Copy link
Copy Markdown
Member

samuelkarp commented Mar 30, 2023

@vinayakankugoyal Do you mind porting this over to sbserver as well? We'll likely be removing the old server package relatively soon as we're planning to move to sbserver exclusively in 2.0. We can then backport this to both server and sbserver in 1.7.

@samuelkarp samuelkarp added the cherry-pick/1.7.x Change to be cherry picked to release/1.7 branch label Mar 30, 2023
@vinayakankugoyal
Copy link
Copy Markdown
Contributor Author

vinayakankugoyal commented Mar 30, 2023

@samuelkarp - Done! Please take another look.

@samuelkarp
Copy link
Copy Markdown
Member

samuelkarp commented Mar 30, 2023

/test pull-containerd-sandboxed-node-e2e

samuelkarp
samuelkarp approved these changes Mar 31, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@samuelkarp samuelkarp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@samuelkarp samuelkarp merged commit 8f756bc into containerd:main Mar 31, 2023
@fuweid fuweid mentioned this pull request Mar 31, 2023
Closed
@fuweid fuweid added cherry-picked/1.7.x PR commits are cherry-picked into release/1.7 branch and removed cherry-pick/1.7.x Change to be cherry picked to release/1.7 branch labels Mar 31, 2023
@fuweid fuweid mentioned this pull request Mar 31, 2023
Merged
@thaJeztah thaJeztah mentioned this pull request Mar 31, 2023
Open
@cyphar cyphar mentioned this pull request Aug 9, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@samuelkarp samuelkarp samuelkarp approved these changes

@AkihiroSuda AkihiroSuda AkihiroSuda approved these changes

+1 more reviewer

@rata rata rata approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area/cri Container Runtime Interface (CRI) cherry-picked/1.7.x PR commits are cherry-picked into release/1.7 branch ok-to-test

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@vinayakankugoyal @k8s-ci-robot @AkihiroSuda @rata @samuelkarp @fuweid