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

feat(chart): add container security context #31043

Merged

Conversation

mikaeld
Copy link
Contributor

@mikaeld mikaeld commented May 3, 2023

This is a follow up to #24588 which was closed due to a fork sync'ing issue. From the original PR by @ChrisFraun:

What: Deployments can have security settings in their manifest on two levels: pod and container. However, there are some capabilities only configurable in one of the respective levels(https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.23/#securitycontext-v1-core). This PR sets a default configuration for container securityContext, which denies privilege escalation and drops all POSIX capabilities. These are and should be standard settings in the context of Kubernetes. It also adds the possibility of running Airflow in an Kubernetes environment without PSP (to be removed in v1.25 https://kubernetes.io/docs/concepts/security/pod-security-policy/), but with OpenPolicyAgent (a or possibly the PSP substitute) with the same capabilities as a restricted PSP instead.
Why: This missing configuration restricts Airflow from being used with the simple upstream helm chart without modifications/unnecessary maintenance. This especially applies to the restricted policy use in OPA. The specific setting in this PR is not inherited from podSecurityContext(pod level) in securityContext(container level).
Problem: There is already a securityContext in the values.yaml, however, this should also be be called podSecurityContext since it's on pod level, but it isn't. To not break backwards compatibility of Airflow, this PR hardcodes the respective capabilities on container level for statsd, scheduler and webserver.
The other possibility would be to introduce a containerSecurityContext in the values.yaml, which is a made up word since it is commonly called scurityContext.
Benefit in either case would be a more secure deployment.

closes: #27612


^ 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.

@mikaeld
Copy link
Contributor Author

mikaeld commented May 3, 2023

I've fixed most of the unit test issues in tests/charts. However, there are failing tests left in tests/charts/airflow_aux/test_pod_template_file.py which I haven't been able to fix on my own. I tried replicating the errors from running the unit tests in breeze by using helm locally but I do not see any errors. I would appreciate another set of eyes because all the remaining failing tests seem to fail for the same reason.

@potiuk @jedcunningham @ChrisFraun Thanks!

@mikaeld mikaeld changed the title Feat/chart/container security context feat(chart): add container security context May 3, 2023
@potiuk potiuk force-pushed the feat/chart/container-security-context branch from bd3c365 to 23c7961 Compare May 8, 2023 11:17
@potiuk
Copy link
Member

potiuk commented May 8, 2023

I rebased it to latest main to check if it was not a temporary issue but if it does not help, I think the only way is to follow the exact steps in https://github.com/apache/airflow/blob/main/TESTING.rst#helm-unit-tests and see if it can be reproduced (including rebuilding the latest image) - this will run the tests in the same conditions as they are run in CI and you can also enter breeze image and run the tests from there - looks like for some reason, trying to render those helm templates failed, but what was the reason is hard to say until you manually run the exact helm command with all the same parameters that were used in the tests or run the tests.

It can also be (though not very likely) that this is a side effect of runnning the tests one after another and then the reason for the side effects should be investigated.

mikaeld and others added 3 commits May 8, 2023 11:14
Co-authored-by: Christoph Fraundorfer <christoph.fraundorfer@allianzdirect.de>
Co-authored-by: ChrisFraun <85613395+ChrisFraun@users.noreply.github.com>
Co-authored-by: Jed Cunningham <66968678+jedcunningham@users.noreply.github.com>
@mikaeld mikaeld force-pushed the feat/chart/container-security-context branch from 0c21962 to b8aff05 Compare May 8, 2023 15:15
@mikaeld
Copy link
Contributor Author

mikaeld commented May 8, 2023

@potiuk I've fixed the tests!

@potiuk
Copy link
Member

potiuk commented May 8, 2023

great. I think some comments on that from a few people involved in chart more than me would be great, let's wait for them - now that the tests are green it;s good for review.

Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

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

@jedcunningham @dstandish @ephraimbuddy - it seems like a good change - do you see some problems with it?

chart/templates/_helpers.yaml Outdated Show resolved Hide resolved
chart/templates/_helpers.yaml Outdated Show resolved Hide resolved
chart/values.schema.json Show resolved Hide resolved
chart/values.schema.json Outdated Show resolved Hide resolved
chart/values.schema.json Outdated Show resolved Hide resolved
chart/values.schema.json Outdated Show resolved Hide resolved
chart/values.yaml Outdated Show resolved Hide resolved
docs/helm-chart/production-guide.rst Outdated Show resolved Hide resolved
@mikaeld
Copy link
Contributor Author

mikaeld commented May 26, 2023

@jedcunningham Thanks for the feedback, I've addressed the issues. This is ready for re-review!

Copy link
Member

@jedcunningham jedcunningham left a comment

Choose a reason for hiding this comment

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

Last round 🤞

docs/helm-chart/production-guide.rst Outdated Show resolved Hide resolved
docs/helm-chart/production-guide.rst Outdated Show resolved Hide resolved
docs/helm-chart/production-guide.rst Outdated Show resolved Hide resolved
mikaeld and others added 2 commits June 1, 2023 12:15
Co-authored-by: Jed Cunningham <66968678+jedcunningham@users.noreply.github.com>
Co-authored-by: Jed Cunningham <66968678+jedcunningham@users.noreply.github.com>
@mikaeld
Copy link
Contributor Author

mikaeld commented Jun 1, 2023

@jedcunningham ready for review!

@jedcunningham jedcunningham merged commit 465d0b3 into apache:main Jun 2, 2023
48 checks passed
@jedcunningham
Copy link
Member

#protm

Thanks @ChrisFraun and @mikaeld! Really appreciate all the effort and persistence to get this change in 🍺

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Chart] Container security context is not defined
3 participants