🌱 tilt: remove securityContext for live_update #7846
Conversation
k8s-ci-robot
added
cncf-cla: yes
k8s-ci-robot
added
the
size/XS
|
Hi @bengentil. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
hack/tools/tilt-prepare/main.go
Outdated
| @@ -775,6 +775,10 @@ func prepareWorkload(name, prefix, binaryName, containerName string, objs []unst | |||
| cmd := []string{"sh", "/start.sh", "/" + binaryName} | |||
| args := append(container.Args, []string(ts.ExtraArgs[name])...) | |||
|
|
|||
| // remove securityContext for tilt live_update, see https://github.com/tilt-dev/tilt/issues/3060 | |||
| d.Spec.Template.Spec.SecurityContext = nil | |||
There was a problem hiding this comment.
nit: let's move this line before the for loop so we do it only once for the deployment (please add the comment in both places)
There was a problem hiding this comment.
Yes I wasn't sure because if I set it out of the loop, it means removing .spec.securityContext for every deployments returned by kustomize without taking in account the containerName arg. (updateDeployment iterates on all objects and execute this function)
This means that all deployments won't have .spec.template.spec.securityContext but may have spec.template.spec.containers[].securityContext.
This shouldn't be an issue per say but may be misleading if a provider adds more deployments.
As is only container matching containerName will have the securityContext, and it will be ensure on its parent template spec.
I can add a comment to explain why it's modified in the loop or make the change globally on all deployments (+every containers maybe so it's not misleading), let me know
There was a problem hiding this comment.
Oh I missed that. Thx for the explanation :)
Maybe a short comment that this is only intentionally done for Deployments that have a container with the name containerName would be good
There was a problem hiding this comment.
Just pushed a new revision, let me know if that comment is clear enough 😉
There was a problem hiding this comment.
Mostly just a safeguard if someone else comes along in the future and wants to "optimize" it :)
Signed-off-by: Benjamin Gentil <[email protected]>
bengentil
force-pushed
the
ben/tilt-securitycontext
branch
from
d5eb23a to
850794a
Compare
|
/ok-to-test /assign @fabriziopandini |
k8s-ci-robot
added
ok-to-test
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. Git tree hash: 3a0c921a2f50ebd60e1f6fe73e100c9f30fafe8c
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fabriziopandini 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 |
k8s-ci-robot
added
the
approved
What this PR does / why we need it:
As of now, tilt live_update feature requires the pod to run as root, see tilt-dev/tilt#3060
This PR removes any securityContext in manager manifests during tilt-prepare.
it allows every providers to add securityContext in manifests (like in #7831)
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #