Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add ability to configure SecurityContext for web container #176

Merged
merged 2 commits into from
Dec 7, 2020

Conversation

chenbh
Copy link
Contributor

@chenbh chenbh commented Oct 30, 2020

Applies only to the web container on the web-deployment pod, useful for stuff like attaching a live debugger to the process.

While this provides a escape hatch for configuring stuff that the
Concourse team doesn't want to support (such as a separate option for
securityContext), it does open up the possiblity to terribly
misconfigure your cluster.

Signed-off-by: Bohan Chen <bochen@pivotal.io>
@@ -51,6 +51,9 @@ spec:
initContainers:
{{- toYaml .Values.web.extraInitContainers | nindent 8 }}
{{- end }}
{{- if .Values.web.dangerousAdditionalPodSpecFields }}
{{- toYaml .Values.web.dangerousAdditionalPodSpecFields | nindent 6 }}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chenbh

nindent 6

Wouldn't this mean it's going to live at the same level as spec on line 34?

I tried using the example listed in this way:

dangerousAdditionalPodSpecFields:     
    securityContext:
       capabilities:
         add:
           - SYS_PTRACE

On a dry-run I get:
unknown field "capabilities" in io.k8s.api.core.v1.PodSecurityContext

Seems capabilities are meant to be set at the individual container level for a pod.

So I tried putting it inside the first container:

containers:
      {{- if .Values.web.sidecarContainers }}
      {{- toYaml .Values.web.sidecarContainers | nindent 8 }}
      {{- end }}
        - name: {{ template "concourse.web.fullname" . }}
          { { - if .Values.web.dangerousAdditionalPodSpecFields } }
          { { - toYaml .Values.web.dangerousAdditionalPodSpecFields | nindent 10 } }
          { { - end } }

But I end up with a parsing error:

Error: YAML parse error on concourse/templates/web-deployment.yaml: error converting YAML to JSON: yaml: line 29: could not find expected ':'

I'm using Helm3. Have you been able to successfully run the example?

Copy link
Contributor Author

@chenbh chenbh Nov 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't this mean it's going to live at the same level as spec on line 34?

Good point, I blindly assumed the pod level securityContext is the same as the container level one.

Originally this was meant as an escape hatch for all of the fancy things you can configure at the pod level that we don't expose in our chart. If it's going to be exposed at the container level and since we explicitly exposed almost all of the fields at that level, I'm going to rework this PR so that it just exposes the web.securityContext field instead.

Since securityContext is only set at the container level, and we pretty
much explicitly configure most of the container level fields

Signed-off-by: Bohan Chen <bochen@pivotal.io>
Copy link
Contributor

@muntac muntac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chenbh Looks good. I would rename the PR and change PR description to reflect that this specifically allows setting the securityContext for the container in the web deployment.

@chenbh chenbh changed the title add ability to configure additional pod spec fields add ability to configure SecurityContext for web container Dec 7, 2020
@chenbh chenbh merged commit d9fc598 into master Dec 7, 2020
@chenbh chenbh deleted the arbitrary-pod-spec-fields branch December 7, 2020 20:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants