support pod securitycontext and container security context #496
Conversation
| @@ -50,6 +50,7 @@ spec: | |||
| ports: | |||
| - name: server | |||
| containerPort: {{ .Values.dagDeploy.ports.dagServerHttp }} | |||
| securityContext: {{ toYaml .Values.dagDeploy.securityContext | nindent 12 }} | |||
There was a problem hiding this comment.
Should this be containerSecurityContext ?
There was a problem hiding this comment.
It seems like securityContext should apply to both pods and containers, podSecurityContext should apply to pods, and containerSecurityContext should apply to containers.
There was a problem hiding this comment.
Yes we can set containerSecurityContext for better naming and we will also release a houston api changes as well for adopting new naming. should we target this release or next patch
There was a problem hiding this comment.
It seems like we should do it on this patch because otherwise customers could put a securityContext config in place for containers, and later that becomes a global setting for pods and containers, which is probably not what they intended.
There was a problem hiding this comment.
@danielhoherd can we follow oss approach here
securityContexts:
pods: {}
container: {}
There was a problem hiding this comment.
i like that, but what do they use for the lower-priority hierarchy?
There was a problem hiding this comment.
they have top level securityContext and then at service levels - but our use case it to allow passing securityContext when a user what to set more extensive properties at container level
There was a problem hiding this comment.
I think we should probably widen this securityContext effort out into a ticket that standardizes on a way that we will do securityContexts, describe the way we think is best to do them, and then make an effort to do that everywhere. We don't want to come up with a cool way to do this in this one feature if it is inconsistent with how we do it elsewhere, so we should be consistent with some other implementation here (maybe the airflow one) and also strive to be consistent with a default implementation everywhere. I'll open a ticket about that.
There was a problem hiding this comment.
Airflow has a pretty good securityContext design diagram in their sourcecode https://github.com/apache/airflow/blob/9dd77520be3d8492156958d57b63b5779a3f55eb/chart/templates/_helpers.yaml#L848-L880
There was a problem hiding this comment.
those are strairght forward but we should mend our code in houston thats why we are going in selectives
values.yaml
Outdated
| securityContext: {} | ||
| podSecurityContext: {} | ||
| # runAsUser: 999 | ||
| # runAsGroup: 0 | ||
| containerSecurityContext: {} |
There was a problem hiding this comment.
securityContexts:
pod: {}
container: {}
There was a problem hiding this comment.
This looks good for now. It's not perfect, but it will unblock us. We can circle back to this when working on https://github.com/astronomer/issues/issues/6368
This is going to break older dagDeploy deployments though because it changes the securityContexts helm values structure.
Description
This PR segregates podSecurityContext and containerSecurityContext in order to add changes independently
Related Issues
Yet to update
Testing
QA should able to pass both container securityContext and podSecurity context independly
Merging
cherry-pick to valid dag-server releases