-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
[enterprise-logs] Run GEL as non-root user enterprise-logs
#691
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good to me, will have another quick look after the rebase just to be sure
@@ -42,8 +42,10 @@ spec: | |||
priorityClassName: {{ .Values.adminApi.priorityClassName }} | |||
{{- end }} | |||
securityContext: | |||
{{- toYaml .Values.adminApi.securityContext | nindent 8 }} | |||
fsGroup: 10001 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand this correctly, this affects all containers in the Pod but I guess minio is fine with this as its user can still read the configuration that is mounted in. We need the Pod level SecurityContext to support the volumeMounts in the AdminAPI container because the Container SecurityContext doesn't affect the Pod Volumes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. fsGroup
is only supported on the Pod spec, but not on container level. Since minio runs as root, this is ok.
This is needed when running GEL as AWS Marketplace container product on EKS where the users needs access to the EKS service account token. Fixes #687 Signed-off-by: Christian Haudum <christian.haudum@gmail.com>
Signed-off-by: Christian Haudum <christian.haudum@gmail.com>
589bd35
to
a736998
Compare
@jdbaldry rebased with |
This PR contains PR #684 and therefore needs to be rebased after that is merged.
Fixes #687