SCC can't be patched via JSONPatch because users is nil #17185
Conversation
When users or groups are nil, standard JSONPatch can't be used to add a new item to the list because the array is nil instead of empty. Alter the serialization of SCC so that there is always a user or group array returned. This allows us to do declarative patching against SCC until we move to PSP in a future release.
openshift-ci-robot
added
kind/bug
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: smarterclayton The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
openshift-merge-robot
added
approved
|
Targeting 3.7 |
openshift-ci-robot
added
size/L
People patching SCCs in a declarative fashion can cause unbounded struct growth. Strike a balance between simple code and efficient (since some SCCs can have hundreds of users).
smarterclayton
force-pushed
the
scc_cant_be_patched
branch
from
64a84dc
to
8f9995b
Compare
| @@ -50,7 +50,27 @@ func (strategy) PrepareForCreate(_ genericapirequest.Context, obj runtime.Object | |||
| func (strategy) PrepareForUpdate(_ genericapirequest.Context, obj, old runtime.Object) { | |||
| } | |||
|
|
|||
| // Canonicalize removes duplicate user and group values, preserving order. | |||
There was a problem hiding this comment.
Do you need to preserve order, or do you just need consistent order? If you only need the latter, you could just use StringKeySet.List which would sort and dedupe.
There was a problem hiding this comment.
order is better in case you have a patch that removes a specific index
| @@ -518,8 +518,16 @@ func autoConvert_security_SecurityContextConstraints_To_v1_SecurityContextConstr | |||
| } | |||
| out.ReadOnlyRootFilesystem = in.ReadOnlyRootFilesystem | |||
| out.SeccompProfiles = *(*[]string)(unsafe.Pointer(&in.SeccompProfiles)) | |||
| out.Users = *(*[]string)(unsafe.Pointer(&in.Users)) | |||
| out.Groups = *(*[]string)(unsafe.Pointer(&in.Groups)) | |||
| if in.Users == nil { | |||
There was a problem hiding this comment.
Generated files in their own commit?
|
Looks backwards compatible as far as I can tell. |
|
Afaict we had nothing that depended on nil vs empty lists. It does highlight how frustrating cfg mgmt is here |
|
LGTM. |
| @@ -79,9 +79,11 @@ type SecurityContextConstraints struct { | |||
| ReadOnlyRootFilesystem bool `json:"readOnlyRootFilesystem" protobuf:"varint,17,opt,name=readOnlyRootFilesystem"` | |||
|
|
|||
| // The users who have permissions to use this security context constraints | |||
| Users []string `json:"users,omitempty" protobuf:"bytes,18,rep,name=users"` | |||
| // +optional | |||
There was a problem hiding this comment.
surprised this didn't regenerate the proto files
There was a problem hiding this comment.
oh, it did. surprised that didn't regenerate the fileDescriptorGenerated blob
There was a problem hiding this comment.
Generated strips comments (last I checked)
|
this lgtm, but is just a band-aid on one particular nullable list field (we have lots and lots) |
|
Yes, important but not sufficient. I’m going to open an upstream. |
smarterclayton
added
lgtm
|
/test all [submit-queue is verifying that this PR is safe to merge] |
|
Automatic merge from submit-queue (batch tested with PRs 17160, 17185). |
|
/retest
|
|
@smarterclayton: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. I understand the commands that are listed here. |
When users or groups are nil, standard JSONPatch can't be used to add a
new item to the list because the array is nil instead of empty. Alter
the serialization of SCC so that there is always a user or group
array returned.
This allows us to do declarative patching against SCC until we move to
PSP in a future release.
@liggitt realized this while trying to switch router to a declarative model - patch is our best option for update, but you can't actually do a safe addition without JSONPatch and without this change.
/kind bug