Chart: Do not propagate global security context to statsd and redis

[Aakcht]

Global pod security context (.Values.securityContexts.pod parameter) is not propagated to statsd deployment when statsd security context is not specified. However at the same time container security context is propagated(.Values.securityContexts.containers parameter). According to documentation looks like it should be propagated, see https://github.com/apache/airflow/blob/main/chart/values.schema.json#L4806 and https://github.com/apache/airflow/blob/main/chart/values.yaml#L1588 . I think it would be convinient to be able to propagate global security context to statsd deployment, so instead of changing documentation I added the propagation of global pod security context to statsd deployment.

---

^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

[jedcunningham]

Actually statsd and redis were intentionally excluded from using globals, note how those 2 are the ones using localPodSecurityContext. They also aren't "airflow" pods ultimately.

I'm not necessarily tied to that decision though.

[Aakcht]

@jedcunningham actually statsd is only half excluded from using globals - since Values.securityContexts.containers is getting propagated without issues to statsd container (note how containerSecurityContext is used in statsd deployment instead of localContainerSecurityContext). So it looks a bit inconsistent to me.

As for redis, I don't use it, so I didn't notice the problem :( Looks like it's the same issue with the redis: according to documentation, global security context should be used when local is not specified, see https://github.com/apache/airflow/blob/main/chart/values.schema.json#L5520 .

I added the same change for redis.

However if you think it'll be better to use only local security context for statsd/redis, I can modify the PR - to change the documentation and add local redis/statsd container security context logic. In this case I suspect localContainerSecurityContext should be used for redis/statsd container security context .

[jedcunningham]

Yeah, I think redis and statsd should be excluded from using the global Airflow stuff, just like it is with env, for example. You're right that it its inconsistent now, but I think we should resolve it by not applying the global container security context to them - that wasn't intentional.

[Aakcht]

@jedcunningham , please take a look. I completely excluded redis/statsd from global security context. Looks like I was wrong in my previous comment and using localContainerSecurityContext logic for redis/statsd container security context is not the best idea, cause it might lead to some of pod security context parameters ending up in container level context. At the same time looks like simple .Values.redis.securityContexts.container/.Values.redis.securityContexts.container works well in this case - at least I can't see anything wrong with it except for the fact that it looks too easy :)

[jedcunningham]

I completely excluded redis/statsd from global security context.

Sounds good!

At the same time looks like simple .Values.redis.securityContexts.container/.Values.redis.securityContexts.container works well in this case - at least I can't see anything wrong with it except for the fact that it looks too easy :)

Close! Just some backcompat to maintain and I think we are there. I'd love to get this fixed before I cut 1.10.0, so thanks for being responsive here 🍺

[Aakcht]

@jedcunningham , I updated the PR, please take a look.

I'm not sure about backward compatibility, since container security context for redis/statsd wasn't even there in the latest helm chart release. So in the latest release statsd.securityContext wasn't going to end up in statsd container security context. In fact, it is even true for the current dev version, see: https://github.com/apache/airflow/blob/main/chart/templates/_helpers.yaml#L842 . So by current logic in main branch statsd.securityContext does not affect statsd container security context in any way.

However I added default container security context as allowPrivilegesEscalation: false, capabilities.drop: [ALL] for redis/statsd, so it should cover people who took the dev version and deployed it with default parameters to kubernetes with restricted PSS - in this case it still should be working for them

P.S. Pretty sure check fails are not related to this PR - I'll rebase the PR if I see that it's fixed in main.

[jedcunningham]

@Aakcht , absolutely right. I see it now with fresh eyes 🍺

[Aakcht]

@jedcunningham , I made the changes and rebased, all checks related to helm chartare now successful and the only failed check is not related to the changes in this PR. Please take a look.

[jedcunningham]

Thanks @Aakcht! Appreciate the fix here.