New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adding the ability to define a secuirty context and SELinux options #257
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you going to plumb these new fields through to anything? See how the SecurityContext for the upgrade pod is generated at
system-upgrade-controller/pkg/upgrade/job/job.go
Lines 326 to 333 in 04a0b9e
upgradectr.WithSecurityContext(&corev1.SecurityContext{ | |
Privileged: &Privileged, | |
Capabilities: &corev1.Capabilities{ | |
Add: []corev1.Capability{ | |
corev1.Capability("CAP_SYS_BOOT"), | |
}, | |
}, | |
}), |
You need to modify that bit to actually use the SecurityContext from the Plan, if it is non-nil. You also need to similarly wire up the SELinuxOptions.
Also, please fill out the PR body with some background and an example of how and when this might be used. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple nits on how this is plumbed through to the upgrade
container.
Note that the prepare
container also uses all the fields from ContainerSpec
; we should probably wire up all the SecurityContext stuff there as well. I'm not sure why it currently doesn't do this, as it's not documented as only supported for the upgrade pod.
pkg/upgrade/container/container.go
Outdated
@@ -37,6 +37,12 @@ func WithSecurityContext(securityContext *corev1.SecurityContext) Option { | |||
} | |||
} | |||
|
|||
func WithSELinuxOptions(seLinuxoptions *corev1.SELinuxOptions) Option { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this is necessary - you're setting container.SecurityContext.SELinuxOptions
, when container.SecurityContext
is already set by the WithSecurityContext
function. Just set this directly on the SecurityContext.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, the option to enable privilege for the upgrade pod is set at the controller level:
system-upgrade-controller/pkg/upgrade/job/job.go
Lines 73 to 82 in f4a9a0f
Privileged = func(defaultValue bool) bool { | |
if str, ok := os.LookupEnv("SYSTEM_UPGRADE_JOB_PRIVILEGED"); ok { | |
if b, err := strconv.ParseBool(str); err != nil { | |
logrus.Errorf("failed to parse $%s: %v", "SYSTEM_UPGRADE_JOB_PRIVILEGED", err) | |
} else { | |
return b | |
} | |
} | |
return defaultValue | |
}(defaultPrivileged) |
Since this is a security control enforced by the administrator, I feel like we probably also need to wire up a new option to allow administrators to opt in to allowing users to specify a SecurityContext for their pods. It can default to true, but if the option is false the SecurityContext from the Plan should be ignored - perhaps along with a logged warning?
@Auston-Ivison-Suse are you still working on this? |
You generally want to rebase your branch on top of master, instead of pulling master into your branch. makes for a cleaner commit log. |
Also made recommended changes.
…tch) (#3717) This PR contains the following updates: | Package | Update | Change | |---|---|---| | [docker.io/rancher/system-upgrade-controller](https://togithub.com/rancher/system-upgrade-controller) | patch | `v0.13.1` -> `v0.13.2` | | [rancher/system-upgrade-controller](https://togithub.com/rancher/system-upgrade-controller) | patch | `v0.13.1` -> `v0.13.2` | --- ### Release Notes <details> <summary>rancher/system-upgrade-controller (docker.io/rancher/system-upgrade-controller)</summary> ### [`v0.13.2`](https://togithub.com/rancher/system-upgrade-controller/releases/tag/v0.13.2) [Compare Source](https://togithub.com/rancher/system-upgrade-controller/compare/v0.13.1...v0.13.2) ##### What's Changed - feat: allow plan to ignore secret updates by [@​buroa](https://togithub.com/buroa) in [rancher/system-upgrade-controller#263 - 225: Add support for exclusive plans by [@​jrodonnell](https://togithub.com/jrodonnell) in [rancher/system-upgrade-controller#260 - Fix: upgrade go in go.mod and bci image by [@​matttrach](https://togithub.com/matttrach) in [rancher/system-upgrade-controller#268 - Use node name for job name instead of host name by [@​brandond](https://togithub.com/brandond) in [rancher/system-upgrade-controller#274 - Adding the ability to define a secuirty context and SELinux options by [@​Auston-Ivison-Suse](https://togithub.com/Auston-Ivison-Suse) in [rancher/system-upgrade-controller#257 - Adding image pull secrets by [@​Dr-N00B](https://togithub.com/Dr-N00B) in [rancher/system-upgrade-controller#272 ##### New Contributors - [@​buroa](https://togithub.com/buroa) made their first contribution in [rancher/system-upgrade-controller#263 - [@​jrodonnell](https://togithub.com/jrodonnell) made their first contribution in [rancher/system-upgrade-controller#260 - [@​matttrach](https://togithub.com/matttrach) made their first contribution in [rancher/system-upgrade-controller#268 - [@​Auston-Ivison-Suse](https://togithub.com/Auston-Ivison-Suse) made their first contribution in [rancher/system-upgrade-controller#257 - [@​Dr-N00B](https://togithub.com/Dr-N00B) made their first contribution in [rancher/system-upgrade-controller#272 **Full Changelog**: rancher/system-upgrade-controller@v0.13.1...v0.13.2 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about these updates again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://togithub.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy43NC4zIiwidXBkYXRlZEluVmVyIjoiMzcuNzQuMyIsInRhcmdldEJyYW5jaCI6Im1haW4ifQ==--> Co-authored-by: lumiere-bot[bot] <98047013+lumiere-bot[bot]@users.noreply.github.com>
We were running into the problems with the upgrade job failing on a host with SELinux enabled. So when one would need to upgrade a host's OS, allowing to set a security context and SELinux Options therein will allow you to upgrade the OS, without disabling SELinux beforehand.
Within the yaml file of a plan, you will be able to set
plan.Spec.Upgrade.SecurityContext
and it will take effect in the upgrade job. The same concept applies to the SELinux Options within a plan.