STOR-2370: Add enhancement for fsgroupChangePolicy defaulting #1804
Conversation
gnufied
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@gnufied: This pull request references STOR-2370 which is a valid jira issue. 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 openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| Since this feature requires explicit action by the admin, I am not *yet* proposing a feature-gate for this (although I am not opposed to the idea). | ||
| If required during TP phase, we can name the annotation in such a way that, it is clear to the user that this is a tech-preview feature. For example - `preview.openshift.io/fsgroup-change-policy`. |
There was a problem hiding this comment.
I am afraid we will need a feature gate.
Since this feature requires explicit action by the admin,
If I am reading the current conditions right, this IMO falls under "Accessible-by-default", even though the annotation would have a weird prefix. Such cluster must be fully upgradeable with the feature (i.e. the prefix needs to stay forever)
If you want to put it into Inaccessible-by-default, upgrades of the cluster are not supported and it should be declared so (Upgradeable: false).
Still, adding a feature gate should be cheap and we can flip it to GA withing the same release as when it's introduced, if we prove it's stable enough.
There was a problem hiding this comment.
Still, adding a feature gate should be cheap and we can flip it to GA.
I was unsure about OCP feature gates being accessible in basically what is a patched version of core k8s component. I have pinged @deads2k @bertinatto offline.
|
|
||
| ```go | ||
| func getPodFsGroupChangePolicy(ns *corev1.Namespace) *api.PodFSGroupChangePolicy { | ||
| fsGroupPolicy, ok := ns.Annotations[securityv1.OnRootMismatchFSGroupPolicy] |
There was a problem hiding this comment.
securityv1.OnRootMismatchFSGroupPolicy -> securityv1.FSGroupChangePolicy,
since the constant will have value openshift.io/fsgroup-change-policy.
|
|
||
| ## Proposal | ||
|
|
||
| ### Allow admins to opt-in to `FSGroupChangePolicy` via namespace annotation |
There was a problem hiding this comment.
Have you considered a way to have this as a structured API, rather than an annotation?
Perhaps we could have a namespace LabelSelector somewhere in the API and allow admins to define their own rules for which namespaces should/shouldn't be included?
There was a problem hiding this comment.
Can you explain this bit more? We are discussing this mechanism to be supported via MAP - https://docs.google.com/document/d/1N7jU2ZG2hRz3MvMZlRrdmPK98jB3N9GYedjVEooeMkQ/edit?usp=sharing , which would I guess require having the namespace label selector.
There was a problem hiding this comment.
Are you expecting this MAP to always be present? Or is this something we are documenting that users would create if they desire it?
You mention a specific annotation that would allow users to opt into this feature.
What if instead, there was an exposed namespace selector that users could configure their own rules for namespace selection.
Your operator could then build the MAP and apply it with the user supplied namespace selector, and we wouldn't have to have a specific annotation or anything to opt folks it, it would be entirely up to the end user as to how the namespaces are selected
There was a problem hiding this comment.
Our plan is that the MAP will be created during cluster installation / upgrade. We're discussing if it should be part of the Storage capability or present in all clusters.
In both cases, the MAP would be very static. All pods in a namespace with a dedicated (and fixed) label will get the default from the MAP.
A new field somewhere would mean a code that would translate it into MAP in the runtime, which we're trying to avoid.
| } | ||
| ``` | ||
|
|
||
| ### Set default `FSGroupChangePolicy`in pods during admission: |
There was a problem hiding this comment.
Generally when I've seen guidance changes like this in the past, we've effected that change throughout operators in OpenShift as well. Is there any plan for forcing this value on OpenShift operators/operands?
There was a problem hiding this comment.
It could be useful for operators that use persistent volumes, such as - alertmanager, prometheus and image-registry etc. Having said that, it is relatively easier to modify pod spec of OCP operators to have this field, but the idea is - a typical web developer may not be even aware of existance of this option, so having an OCP admin set this defaulting for developer namespaces will speed up their workloads even if workload author's aren't aware of the option.
|
@gnufied: The following test failed, say
Full PR test history. Your PR dashboard. 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-sigs/prow repository. I understand the commands that are listed here. |
https://issues.redhat.com/browse/STOR-2370