Add the ability to specify a security context to the deployment #289

drustan · 2023-07-21T13:28:49Z

This aims to allow the secret operator deployment in a restricted policy applied namespace. It adds the securityContext and podSecurityContext to avoid the following error :

58s Warning FailedCreate replicaset/vault-secrets-operator-controller-manager-ccbf4f6db Error creating: pods "vault-secrets-operator-controller-manager-ccbf4f6db-kmd5p" is forbidden: violates PodSecurity "restricted:latest": unrestricted capabilities (containers "kube-rbac-proxy", "manager" must set securityContext.capabilities.drop=["ALL"]), seccompProfile (pod or containers "kube-rbac-proxy", "manager" must set securityContext.seccompProfile.type to "RuntimeDefault" or "Localhost")

…od and containers

hashicorp-cla · 2023-07-21T13:28:52Z

All committers have signed the CLA.

drustan · 2023-08-23T08:03:41Z

Is there any reason this is not accepted? Just asking so that I can make any necessary corrections.

tomhjp

Sorry for the delay reviewing, I think this looks like a good addition, thank you! Would you be able to add some tests in https://github.com/hashicorp/vault-secrets-operator/blob/main/test/unit/deployment.bats please? With that and the other comment in this review addressed, I think this will look in a good place to get merged.

chart/values.yaml

…ller

…ontext parameters

drustan · 2023-08-23T13:39:50Z

I'm really not sure about the unit tests…

drustan · 2023-09-07T09:09:19Z

This addresses issue #311

tomhjp

Thanks for adding the tests! They look good to me, just 2 quick fixes needed and then I think this is ready to land

test/unit/deployment.bats

Co-authored-by: Tom Proctor <[email protected]>

drustan · 2023-09-14T13:54:59Z

Thanks for the suggestions <3

tomhjp

Thanks for the updates! I took another pass and found a few more minor fixes to make, alongside one more trivial test fix.

chart/templates/deployment.yaml

test/unit/deployment.bats

tomhjp · 2023-09-18T14:30:15Z

chart/templates/deployment.yaml

        volumeMounts:
        - mountPath: /var/run/podinfo
          name: podinfo
      securityContext:
-        runAsNonRoot: true
+        {{- toYaml .Values.controller.podSecurityContext | nindent 8 }}


Since this PR was opened, we've added a pre-delete hook that runs the same image as the manager container further down in this file. I think we should probably apply the .Values.controller.podSecurityContext and .Values.controller.manager.securityContext settings to the pod and container respectively in that job. WDYT?

I pushed a commit that includes some test updates to better illustrate this suggestion: f6a5aa8. If your environment has a requirement for setting securityContext on all pods, I think it will be required.

kschoche · 2023-09-19T20:22:46Z

Hi @drustan -
Thanks for posting this PR!
I had a chat with @tomhjp today about this and I think that we are close but maybe we can clean up the UX a little.
Is there a reason to keep the securityContext settings separate in the containers? I can't envision needing the kubeRbacProxy to be more permissive than the controller (manager) and afaik they don't have any different requirements to be able to run?
Does it make the most sense to just create:
.Values.controller.securityContext and apply it to both manager and kubeRbacProxy ?

Co-authored-by: Tom Proctor <[email protected]>

…inct container security contexts for kubeRbacProxy and manager containers. Let's set only one for both.

drustan · 2023-09-20T07:39:31Z

Hello @kschoche! I guess you guys are right. I committed the changes, let me know if this is what you guys expected further to your chat. I'm still not very comfortable with the unit tests, but I'm not sure they need any amend following these changes.

tomhjp

Thanks, I think the updated values.yaml looks good other than some tidy-up 👍

tomhjp · 2023-09-20T10:21:38Z

chart/values.yaml

+    # Configures the Container Security Context
+    # https://kubernetes.io/docs/tasks/configure-pod-container/security-context
+    securityContext:
+      allowPrivilegeEscalation: false


I think this one can be deleted now.

I overlooked that one sorry.

tomhjp · 2023-09-20T10:42:27Z

chart/templates/deployment.yaml

        volumeMounts:
        - mountPath: /var/run/podinfo
          name: podinfo
      securityContext:
-        runAsNonRoot: true
+        {{- toYaml .Values.controller.podSecurityContext | nindent 8 }}


I pushed a commit that includes some test updates to better illustrate this suggestion: f6a5aa8. If your environment has a requirement for setting securityContext on all pods, I think it will be required.

test/unit/deployment.bats

…tainer specific configuration values (.cf #58b3a087)

Co-authored-by: Tom Proctor <[email protected]>

drustan · 2023-09-20T12:44:38Z

I don't see your commit f6a5aa8 in the PR. It's not as obvious as accepting suggestions

tomhjp

Sorry about that, I couldn't add suggestions for the changes needed in the pre-delete hook because the PR doesn't touch that area of the code at all yet, so I thought the commit would be easier. I rolled the test changes up into GH suggestion comments, so that just leaves the deployment.yaml changes, which is applying exactly the same changes as you've already got to the pod spec at the bottom of the file.

i.e. after line 153, insert:

      securityContext:
        {{- toYaml .Values.controller.podSecurityContext | nindent 8 }}

And after line (currently) 165:

        securityContext:
          {{- toYaml .Values.controller.securityContext | nindent 10 }}

LMK if any of that doesn't quite make sense.

test/unit/deployment.bats

…lues Co-authored-by: Tom Proctor <[email protected]>

…xt test with default values Co-authored-by: Tom Proctor <[email protected]>

…xt test with example values Co-authored-by: Tom Proctor <[email protected]>

…to the pre-delete job

drustan · 2023-09-20T14:03:28Z

Thanks @tomhjp for the clarifications. Hope these last commits will wrap this up :)

tomhjp

LGTM! Thanks for your patience through the multiple rounds of feedback. I'll leave merging to @kschoche so he gets a chance to review too.

kschoche

Great work on this, thank you for submitting and helping us all converge on a design!

feat(chart): add the ability to specify a security context for both p…

8819f1d

…od and containers

drustan requested a review from a team July 21, 2023 13:28

Merge branch 'main' into securitycontext

fd1b36a

drustan requested a review from a team as a code owner August 22, 2023 15:47

tomhjp reviewed Aug 23, 2023

View reviewed changes

chart/values.yaml Outdated Show resolved Hide resolved

drustan added 2 commits August 23, 2023 15:38

fix(chart): the podSecurityContext parameter is related to the contro…

89ad8b6

…ller

test(chart): add unit tests for both podSecurityContext and securityC…

815a6fa

…ontext parameters

drustan requested a review from tomhjp August 28, 2023 12:26

drustan added 2 commits August 29, 2023 09:37

Merge branch 'main' into securitycontext

98bcb71

Merge branch 'hashicorp:main' into securitycontext

0085fba

Merge branch 'main' into securitycontext

6b511bc

tomhjp reviewed Sep 14, 2023

View reviewed changes

test/unit/deployment.bats Show resolved Hide resolved

test/unit/deployment.bats Outdated Show resolved Hide resolved

test/unit/deployment.bats Outdated Show resolved Hide resolved

drustan and others added 2 commits September 14, 2023 15:53

Apply suggestions from code review

51cd4e1

Co-authored-by: Tom Proctor <[email protected]>

Merge branch 'main' into securitycontext

e9b5c7f

tomhjp reviewed Sep 18, 2023

View reviewed changes

tomhjp linked an issue Sep 19, 2023 that may be closed by this pull request

[helm] Allow override pod securityContext with Helm values #311

Closed

Merge branch 'main' into securitycontext

26b745a

drustan and others added 4 commits September 20, 2023 09:08

fix(chart): indentation in deployment template

454eca9

Co-authored-by: Tom Proctor <[email protected]>

fix(chart): indentation in deployment template

7eabe6e

Co-authored-by: Tom Proctor <[email protected]>

fix(chart): delete unwanted whitespace

4ad2541

Co-authored-by: Tom Proctor <[email protected]>

feat(chart): as suggested, it is not necessary to be able to set dist…

58b3a08

…inct container security contexts for kubeRbacProxy and manager containers. Let's set only one for both.

tomhjp reviewed Sep 20, 2023

View reviewed changes

fix(chart): delete now unwanted security context in kubeRbacProxy con…

590de5c

…tainer specific configuration values (.cf #58b3a087)

fix(chart): values references have changed (cf. 58b3a08)

f5e4525

Co-authored-by: Tom Proctor <[email protected]>

tomhjp reviewed Sep 20, 2023

View reviewed changes

test/unit/deployment.bats Outdated Show resolved Hide resolved

test/unit/deployment.bats Outdated Show resolved Hide resolved

test/unit/deployment.bats Outdated Show resolved Hide resolved

test/unit/deployment.bats Outdated Show resolved Hide resolved

drustan and others added 5 commits September 20, 2023 15:58

test(chart): amend controller.podSecurityContext test with default va…

60a8723

…lues Co-authored-by: Tom Proctor <[email protected]>

test(chart): amend controller.podSecurityContext test with example va…

13e4072

…lues Co-authored-by: Tom Proctor <[email protected]>

test(chart): amend controller.{manager,kube-rbac-proxy}.securityConte…

52a6ee1

…xt test with default values Co-authored-by: Tom Proctor <[email protected]>

test(chart): amend controller.{manager,kube-rbac-proxy}.securityConte…

beb76bd

…xt test with example values Co-authored-by: Tom Proctor <[email protected]>

feat(chart): add pod security context and container security context …

ed5da1d

…to the pre-delete job

tomhjp approved these changes Sep 20, 2023

View reviewed changes

kschoche approved these changes Sep 20, 2023

View reviewed changes

tomhjp merged commit a5eda32 into hashicorp:main Sep 20, 2023

benashz added this to the v0.3.0 milestone Sep 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add the ability to specify a security context to the deployment #289

Add the ability to specify a security context to the deployment #289

drustan commented Jul 21, 2023

hashicorp-cla commented Jul 21, 2023 •

edited

Loading

drustan commented Aug 23, 2023

tomhjp left a comment

drustan commented Aug 23, 2023

drustan commented Sep 7, 2023

tomhjp left a comment

drustan commented Sep 14, 2023

tomhjp left a comment

tomhjp Sep 18, 2023

tomhjp Sep 20, 2023

kschoche commented Sep 19, 2023

drustan commented Sep 20, 2023

tomhjp left a comment

tomhjp Sep 20, 2023

drustan Sep 20, 2023

tomhjp Sep 20, 2023

drustan commented Sep 20, 2023

tomhjp left a comment

drustan commented Sep 20, 2023

tomhjp left a comment

kschoche left a comment

Add the ability to specify a security context to the deployment #289

Add the ability to specify a security context to the deployment #289

Conversation

drustan commented Jul 21, 2023

hashicorp-cla commented Jul 21, 2023 • edited Loading

drustan commented Aug 23, 2023

tomhjp left a comment

Choose a reason for hiding this comment

drustan commented Aug 23, 2023

drustan commented Sep 7, 2023

tomhjp left a comment

Choose a reason for hiding this comment

drustan commented Sep 14, 2023

tomhjp left a comment

Choose a reason for hiding this comment

tomhjp Sep 18, 2023

Choose a reason for hiding this comment

tomhjp Sep 20, 2023

Choose a reason for hiding this comment

kschoche commented Sep 19, 2023

drustan commented Sep 20, 2023

tomhjp left a comment

Choose a reason for hiding this comment

tomhjp Sep 20, 2023

Choose a reason for hiding this comment

drustan Sep 20, 2023

Choose a reason for hiding this comment

tomhjp Sep 20, 2023

Choose a reason for hiding this comment

drustan commented Sep 20, 2023

tomhjp left a comment

Choose a reason for hiding this comment

drustan commented Sep 20, 2023

tomhjp left a comment

Choose a reason for hiding this comment

kschoche left a comment

Choose a reason for hiding this comment

hashicorp-cla commented Jul 21, 2023 •

edited

Loading