Mask /sys/devices/virtual/powercap #20501
Conversation
openshift-ci
bot
added
release-note-none
approved
mheon
force-pushed
the
powercap
branch
2 times, most recently
from
0d50cee to
55bfd71
Compare
I don't really like this solution because it can't be undone by `--security-opt unmask=all` but I don't see another way to make this retroactive. We can potentially change things up to do this the right way with 5.0 (actually have it in the list of masked paths, as opposed to adding at spec finalization as now). Signed-off-by: Matthew Heon <[email protected]>
|
Had to disable the test, as our CI VMs don't have the file in question anyways. |
|
This should be ready. |
|
Ephemeral COPR build failed. @containers/packit-build please check. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mheon, vrothberg The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
There was a problem hiding this comment.
I do not like the fact that it does not work correctly for new containers created with unmask=/sys/devices/virtual/powercap. Should we add a new option to the container config to indicate the container was created after this patch and then do not apply this logic because for new containers we should definitely add it to BlockAccessToKernelFilesystems() in the spec generation.
| @@ -805,3 +805,9 @@ func (c *Container) makePlatformMtabLink(etcInTheContainerFd, rootUID, rootGID i | |||
| func (c *Container) getPlatformRunPath() (string, error) { | |||
| return "/run", nil | |||
| } | |||
|
|
|||
| func (c *Container) addMaskedPaths(g *generate.Generator) { | |||
| if !c.config.Privileged { | |||
There was a problem hiding this comment.
should we also add && len(g.Config.Linux.MaskedPaths) != 0 as check here? if umask=all is set it would mean we do not add the path which seems more desirable.
There was a problem hiding this comment.
I agree with @Luap99, it'd be better to have a way to not mask it.
Otherwise we have no way to have an unmasked /sys in the container and I think that breaks mounting /sys in a nested container
/cherry-pick v4.7 |
|
@lsm5: new pull request created: #20509 In response to this:
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. |
|
@giuseppe @Luap99 I share your concerns, but introducing some sort of versioning to the container config to indicate whether it was created before or after the change is sufficiently drastic I'd like to push it out to 5. We will probably want a way to migrate existing containers to the new, changed config ( |
|
This has the potential to cause regressions, if there is no way to undo that then we have problems and that cannot wait for a 5.0. I personally see no problem to add a simple int field to the container config and set this to 1 for all new containers, the old ones will have 0 as default. Then you can check if it is a new one and do not apply this logic because new ones should be set correctly in the specgen were we handle masked paths right now. |
|
I think the suggestion from @Luap99 to add At some point, we can drop this check completely and just use |
|
Partially addresses https://issues.redhat.com/browse/RHEL-12098 and https://issues.redhat.com/browse/RHEL-12097. A vendor of Buildah with the fix is needed to complete the fix. |
github-actions
bot
added
the
locked - please file new issue/PR
I don't really like this solution because it can't be undone by
--security-opt unmask=allbut I don't see another way to make this retroactive. We can potentially change things up to do this the right way with 5.0 (actually have it in the list of masked paths, as opposed to adding at spec finalization as now).