helm chart: add container security context #3322
Conversation
|
Welcome @viktoriaas! |
volcano-sh-bot
added
the
size/M
|
/assign @william-wang |
|
Hi, please also update README |
|
@Monokaix hi, I updated README and signed off the commits. |
Please also rebase master and submit one clean pr with git squash : ) |
volcano-sh-bot
added
retest-not-required-docs-only
approved
|
/reopen |
|
@Monokaix: Reopened this PR. 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 kubernetes/test-infra repository. |
volcano-sh-bot
added
size/M
|
@Monokaix Hi, I tried to create one clean commit but ended up messing up my forked repo and somehow closing this issue 😅 I thought I would end up with one clean commit but there one merge I can't get rid of. I'm sorry for the mess, please let me know if it's okay this way or I should fix the merge. Thanks! |
You can checkout a new local branch based on master and then cherry-pick your commit to it and force push your repo to remove the merge commit. |
volcano-sh-bot
added
retest-not-required-docs-only
approved
|
/reopen |
|
@Monokaix: Failed to re-open PR: state cannot be changed. There are no new commits on the viktoriaas:master branch. 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 kubernetes/test-infra 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 |
volcano-sh-bot
removed
the
approved
|
[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 |
volcano-sh-bot
added
size/M
|
@Monokaix finally one clean commit! Thanks for the patience! |
| @@ -150,4 +151,8 @@ spec: | |||
| - -v=4 | |||
| - 2>&1 | |||
| imagePullPolicy: {{ .Values.basic.image_pull_policy }} | |||
| {{- if $controller_container_sc }} | |||
| securityContext: | |||
| {{- toYaml $controller_container_sc | nindent 14 }} | |||
There was a problem hiding this comment.
Why nindent is 14 here but other places are 12?
There was a problem hiding this comment.
I followed the setting from resources field of the container, to be consistent. In other deployments, resources has nindent 12 as well, just in this one 14
There was a problem hiding this comment.
But nindent of all deployments should be 12 or 14, we should check that by helm template to get a correct value, right?
There was a problem hiding this comment.
I tested with helm template and resulting YAMLs are produced correctly and can be applied. For functionality, it doesn't matter if indents are the same across the deployments nor it matters if the indents are the same inside one YAML file as long as indents are used consistently at the same level inside one logical section - e.g. all items of map/list are are indented the same.
The controller deployment already featured 14 so I left it that way but of course, can be changed to 12 to align with the rest of the deployments. I can change that, if desired (but it will be one more commit I guess)
There was a problem hiding this comment.
You can change it to 12 in this commit: )
this change adds container security context support which might be required in some restricted environments Signed-off-by: viktoriaas <viktoria.spisakovaa@gmail.com>
|
/lgtm |
|
Hi, please use |
|
New changes are detected. LGTM label has been removed. |
Signed-off-by: viktoriaas <viktoria.spisakovaa@gmail.com>
volcano-sh-bot
added
size/XXL
|
Hi @Monokaix , I synced my branch with upstream and then used |
| @@ -0,0 +1,220 @@ | |||
| apiVersion: apiextensions.k8s.io/v1 | |||
There was a problem hiding this comment.
I only performed make update-development-yaml but I haven't noticed go version set to 1.20 in go.mod. I built today again but with go 1.20 and the result looks much better. Would you mind having a look on a separate branch in my forked volcano repo with the changes? If that's better, I can rather open a new PR from that branch (and this one can be closed) or merge that branch to master.
There was a problem hiding this comment.
so should I merge that new branch to the master? 😄
There was a problem hiding this comment.
If you are not familiar with git, you can open a new pr with a new commit and close this one.
| @@ -1,6 +1,8 @@ | |||
| module volcano.sh/volcano | |||
|
|
|||
| go 1.20 | |||
There was a problem hiding this comment.
the same comment as above. I did something wrong in the process, sorry
|
closing as per @Monokaix comment |
This change adds container security context to
admission, controller, scheduler Deploymentsandscheduler Job. Container security context is necessary in some restricted environments, e.g. in environment that employsRestrictedPod Security Standard as default.