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

[jaeger-operator] Enforce default container security context #263

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

majidazimi
Copy link
Contributor

Since upstream docker image is already using non root user, I thought it might be good to enforce security context at container level in the chart to prevent accidental problems.

Does it make sense to make this configurable? jaeger-operator will always run under non root and it will not modify local file system anyways. So I hardcoded all the parameters.

@majidazimi majidazimi changed the title [jaeger-operator] Enforce default container security policy [jaeger-operator] Enforce default container security context Jun 30, 2021
@majidazimi
Copy link
Contributor Author

lint-test stage has failed without a clear error message:

 jaeger-operator => (version: "2.22.1", path: "charts/jaeger-operator") > Error waiting for process: exit status 1

@mehta-ankit
Copy link
Member

lint-test stage has failed without a clear error message:

 jaeger-operator => (version: "2.22.1", path: "charts/jaeger-operator") > Error waiting for process: exit status 1

I saw similar failures on the job for jaeger helm chart PR's. Pod is failing to be scheduled because node has a taint.

64s         Warning   FailedScheduling    pod/jaeger-operator-7n3zv2gdkq-7c7c5bcc8-22fpg                default-scheduler       0/1 nodes are available: 1 node(s) had taint {node.kubernetes.io/not-ready: }, that the pod didn't tolerate.   10m          8       jaeger-operator-7n3zv2gdkq-7c7c5bcc8-22fpg.168d5b94a8712b0c

@majidazimi
Copy link
Contributor Author

I fixed the version conflict by bumping chart version. Would you please take a look into this?

@@ -36,6 +36,11 @@ spec:
- name: {{ include "jaeger-operator.fullname" . }}
image: "{{ .Values.image.repository }}:{{ .Values.image.tag }}"
imagePullPolicy: {{ .Values.image.pullPolicy }}
securityContext:
Copy link
Contributor

Choose a reason for hiding this comment

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

If you don't mind a small comment: I personally prefer the flexibility of setting the entire securityContext via values.yaml, e.g.:

# values.yaml:
securityContext:
  allowPrivilegeEscalation: false
  capabilities:
    drop:
      - ALL
  readOnlyRootFilesystem: true
  runAsNonRoot: true
  privileged: false
  runAsUser: 65532

with deployment.yaml being:

          securityContext:
            {{- toYaml .securityContext | nindent 12 }}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. However, since there is already securityContext at pod level, it would be better to add containerSecurityContext in values.yml

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.

3 participants