Support patch directives in pod/container overrides #967
Conversation
Add support for Kubernetes strategic merge directives, allowing more
control over pod/container overrides. For example, the attribute
pod-overrides:
spec:
securityContext:
runAsUser: 1001
$patch: replace
can be used to overwrite the default securityContext for a pod -- if the
default pod SecurityContext is configured to be
podSecurityContext:
fsGroup: 2000
runAsGroup: 3000
runAsUser: 1000
using $patch: replace will result in the pod SecurityContext
podSecurityContext:
runAsUser: 1001
instead of
podSecurityContext:
fsGroup: 2000
runAsGroup: 3000
runAsUser: 1001
To achieve this, we use the raw patch as defined in the overrides
attribute rather than deserializing and reserializing it.
Signed-off-by: Angel Misevski <[email protected]>
Signed-off-by: Angel Misevski <[email protected]>
Codecov ReportBase: 50.18% // Head: 50.20% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #967 +/- ##
==========================================
+ Coverage 50.18% 50.20% +0.01%
==========================================
Files 39 39
Lines 2467 2484 +17
==========================================
+ Hits 1238 1247 +9
- Misses 1103 1107 +4
- Partials 126 130 +4
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
| @@ -0,0 +1,26 @@ | |||
| name: "container overrides can override securityContext" | |||
There was a problem hiding this comment.
This test name as well as /container-overrides/overrides-can-use-delete-directive.yaml and /container-overrides/overrides-can-use-replace-directive.yaml starts with a lower-case letter rather than uppercase like the others.
Very unimportant and doesn't need to be fixed, but it's the only remark I could find about this PR :P
|
I still need to do some manual testing, but the code changes look good to me. It might be worth linking https://github.com/kubernetes/community/blob/master/contributors/devel/sig-api-machinery/strategic-merge-patch.md?rgh-link-date=2022-11-03T17%3A21%3A38Z#basic-patch-format in the additional documentation for pod/container overrides. Maybe something as simple as:
|
There was a problem hiding this comment.
Tested on OpenShift 4.11, everything works as expected 👍
Here was the DWOC I used:
config:
routing:
defaultRoutingClass: basic
workspace:
imagePullPolicy: Always
podSecurityContext:
fsGroup: 1000670000
runAsUser: 1000670000Setting the devworkspace's pod-overrides to the following:
attributes:
pod-overrides:
spec:
securityContext:
runAsUser: 1000670001Resulted in the following security context:
securityContext:
runAsUser: 1000670001
fsGroup: 1000670000Here I tested the replace patch directive:
attributes:
pod-overrides:
spec:
securityContext:
runAsUser: 1000670001
$patch: replaceWhich resulted in the following security context (as expected):
securityContext:
runAsUser: 1000670001And lastly, I tested the delete patch directive:
attributes:
pod-overrides:
spec:
securityContext:
runAsUser: 1000670001
$patch: deleteWhich removed the security context entirely, as expected:
securityContext: {}
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: amisevsk, AObuchow, dkwon17, ibuziuk 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 |
What does this PR do?
Adds support for Kubernetes strategic merge patch directives. This allows deleting/overwriting fields that are not settable otherwise.
For example, if the default pod securityContext is configured to be
setting the pod-overrides attribute to be
will result in the pod using the securityContext
whereas setting it to be
will result in the securityContext
To do this, we have to apply the raw yaml specified in the overrides field as the patch rather than deserializing it, since Go will drop unrecognized fields in the struct when deserializing.
What issues does this PR fix or reference?
Closes #966
Is it tested? How?
Test by using directives in pod/container overrides.
PR Checklist
/test v8-devworkspace-operator-e2e, v8-che-happy-pathto trigger)v8-devworkspace-operator-e2e: DevWorkspace e2e testv8-che-happy-path: Happy path for verification integration with Che