Allow airflow standard images to run in openshift utilising the official helm chart #18136 #18147

nwalens · 2021-09-10T15:32:06Z

This pull request adds the parameter rbac.createSCCRoleBinding.
When enabled, a new RoleBinding object will be created targeting the various serviceAccounts utilised by deployments and pods.
The roleBinding will target the system:openshift:scc:anyuid cluster role which allows for pods to start with any arbitrary uid.
The ideal solution would also add the possibility of removing the security contexts altogether, however this would not be possible due to a number of pod templates setting different uids.
I will investigate the possibility to add the option of removing the security contexts so that openshift can then set its own uid as intended.
As it is, the pods will start and work as expected, utilising the predefined uids set in values file or the default image uid as set during build.

The change is not intrusive and should work in the current workflow as is. The option is also set to false by default in order to not impact any existing setups.

^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, 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 UPDATING.md.

boring-cyborg · 2021-09-10T15:32:09Z

Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst)
Here are some useful points:

Pay attention to the quality of your code (flake8, mypy and type annotations). Our pre-commits will help you with that.
In case of a new feature add useful documentation (in docstrings or in docs/ directory). Adding a new operator? Check this short guide Consider adding an example DAG that shows how users should use it.
Consider using Breeze environment for testing locally, it’s a heavy docker but it ships with a working Airflow and a lot of integrations.
Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
Please follow ASF Code of Conduct for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
Be sure to read the Airflow Coding style.
Apache Airflow is a community-driven project and together we are making it better 🚀.
In case of doubts contact the developers at:
Mailing List: dev@airflow.apache.org
Slack: https://s.apache.org/airflow-slack

potiuk

Not sure of all the details but some initial comments

chart/values.yaml

chart/values.schema.json

chart/templates/rbac/security-context-constraint-rolebinding.yaml

jedcunningham

Shouldn't we check that the serviceaccounts are being created?

chart/CHANGELOG.txt

chart/templates/rbac/security-context-constraint-rolebinding.yaml

chart/values.yaml

chart/templates/rbac/security-context-constraint-rolebinding.yaml

nwalens · 2021-09-10T20:50:14Z

@jedcunningham I hope I managed to fix all issues you pointed out.

Sorry for the trouble and thanks for the help!

jedcunningham

@nwalens, I apologize, but looking again I think I was wrong with checking <component>.serviceAccount.create. I think we want them to be set regardless and the <component>.serviceAccountName templates handles the details. Sorry about that!

chart/templates/rbac/security-context-constraint-rolebinding.yaml

docs/helm-chart/production-guide.rst

- Create a rolebinding for all required service accounts so they can use the anyuid SCC in openshift. Signed-off-by: Ney walens De Mesquita <walens@gmail.com>

- Support openshift securityContextConstraints in order to allow pods to run with anyuid scc. - The option is disabled by default since it is specific to openshift 4 setups Signed-off-by: Ney Walens De Mesquita <walens@gmail.com>

- SCCs will always be cluster roles Signed-off-by: Ney Walens De Mesquita <walens@gmail.com>

- simplify the parameter name - add checks for service accounts so we do not bind the role to unexisting accounts Signed-off-by: Ney Walens De Mesquita <walens@gmail.com>

Signed-off-by: Ney Walens De Mesquita <walens@gmail.com>

- Signed-off-by: Ney Walens De Mesquita <walens@gmail.com>

Signed-off-by: Ney Walens De Mesquita <walens@gmail.com>

- The feature will be released in a future release Signed-off-by: Ney Walens De Mesquita <walens@gmail.com>

- This will ensure that the correct values for each service account are properly checked Signed-off-by: Ney Walens De Mesquita <walens@gmail.com>

Signed-off-by: Ney Walens De Mesquita <walens@gmail.com>

nwalens · 2021-09-20T14:08:57Z

@nwalens, I apologize, but looking again I think I was wrong with checking <component>.serviceAccount.create. I think we want them to be set regardless and the <component>.serviceAccountName templates handles the details. Sorry about that!

No worries at all. I have removed the checks as requested.

jedcunningham · 2021-09-20T17:43:15Z

@nwalens, looks good. Can you add at least a basic test to make sure it is created when enabled and has the right kind. Ideally we cover the conditional stuff too, like "flower".

kaxil · 2021-09-20T21:33:13Z

@nwalens Flower tests are at https://github.com/apache/airflow/blob/main/chart/tests/test_flower.py which you can use as an example

potiuk · 2021-09-24T18:27:47Z

Approved - but the test mentioned by @jedcunningham should be added

github-actions · 2021-09-24T18:27:57Z

The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest main or amend the last commit of the PR, and push it with --force-with-lease.

Signed-of-by: Ney Walens De Mesquita <walens@gmail.com>

nwalens · 2021-09-26T17:49:23Z

@jedcunningham and @potiuk, sorry for the late reply.

I have just rebased and added the unit test.

Thanks a lot.

potiuk · 2021-09-27T18:16:18Z

Some static/helm unit test failed

boring-cyborg · 2021-09-28T16:38:24Z

Awesome work, congrats on your first merged pull request!

potiuk · 2021-09-28T16:38:35Z

🎉 🎉 🎉 🎉 🎉 🎉

nwalens · 2021-09-28T17:13:17Z

🎉 🎉 🎉 🎉 🎉 🎉

Thank you very much!

nwalens requested review from ashb, dimberman, jedcunningham and kaxil as code owners September 10, 2021 15:32

boring-cyborg bot added the area:helm-chart Airflow Helm Chart label Sep 10, 2021

nwalens mentioned this pull request Sep 10, 2021

Allow airflow standard images to run in openshift utilising the official helm chart #18136

Closed

2 tasks

potiuk reviewed Sep 10, 2021

View reviewed changes

jedcunningham reviewed Sep 10, 2021

View reviewed changes

nwalens requested a review from jedcunningham September 11, 2021 23:20

mik-laj linked an issue Sep 12, 2021 that may be closed by this pull request

Allow airflow standard images to run in openshift utilising the official helm chart #18136

Closed

2 tasks

nwalens mentioned this pull request Sep 14, 2021

Add support for securityContext per deployment #18249

Merged

jedcunningham reviewed Sep 15, 2021

View reviewed changes

nwalens added 11 commits September 20, 2021 11:24

Add support for openshift security context constraints.

cc87310

- Create a rolebinding for all required service accounts so they can use the anyuid SCC in openshift. Signed-off-by: Ney walens De Mesquita <walens@gmail.com>

add extra option for security context constraints

684ef9a

- Support openshift securityContextConstraints in order to allow pods to run with anyuid scc. - The option is disabled by default since it is specific to openshift 4 setups Signed-off-by: Ney Walens De Mesquita <walens@gmail.com>

Cleanup on scc rolebing

6999884

- SCCs will always be cluster roles Signed-off-by: Ney Walens De Mesquita <walens@gmail.com>

rename helm option and add further checks

e5521ec

- simplify the parameter name - add checks for service accounts so we do not bind the role to unexisting accounts Signed-off-by: Ney Walens De Mesquita <walens@gmail.com>

include documentation for the added parameter

913d4ef

Signed-off-by: Ney Walens De Mesquita <walens@gmail.com>

update changelog

f06e5dd

- Signed-off-by: Ney Walens De Mesquita <walens@gmail.com>

fix typo (extra space)

d06335b

Signed-off-by: Ney Walens De Mesquita <walens@gmail.com>

remove changelog

62d1e28

- The feature will be released in a future release Signed-off-by: Ney Walens De Mesquita <walens@gmail.com>

Check for service accounts for each of the components

fcb16b1

- This will ensure that the correct values for each service account are properly checked Signed-off-by: Ney Walens De Mesquita <walens@gmail.com>

Add documentation and improve reference to the new feature.

52acf61

Signed-off-by: Ney Walens De Mesquita <walens@gmail.com>

remove service accunt checks and fix documentation typos.

92f01af

Signed-off-by: Ney Walens De Mesquita <walens@gmail.com>

nwalens force-pushed the main branch from 5dfb9d0 to 92f01af Compare September 20, 2021 09:26

nwalens requested a review from potiuk September 20, 2021 11:12

potiuk approved these changes Sep 24, 2021

View reviewed changes

github-actions bot added the okay to merge It's ok to merge this PR as it does not require more tests label Sep 24, 2021

nwalens and others added 2 commits September 26, 2021 18:20

Merge branch 'apache:main' into main

453e03e

add unit test for SCC creation

ea1c4fe

Signed-of-by: Ney Walens De Mesquita <walens@gmail.com>

fix unit test code formating for static checks

cbc881b

nwalens requested a review from jedcunningham September 27, 2021 07:32

jedcunningham approved these changes Sep 27, 2021

View reviewed changes

jedcunningham added this to the Airflow Helm Chart 1.3.0 milestone Sep 27, 2021

potiuk approved these changes Sep 27, 2021

View reviewed changes

Fix test code formating

ab58e49

potiuk merged commit 45e8191 into apache:main Sep 28, 2021

kaxil mentioned this pull request Nov 5, 2021

Add Changelog for Airflow Chart 1.3.0 #19417

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow airflow standard images to run in openshift utilising the official helm chart #18136 #18147

Allow airflow standard images to run in openshift utilising the official helm chart #18136 #18147

nwalens commented Sep 10, 2021 •

edited

Loading

boring-cyborg bot commented Sep 10, 2021

potiuk left a comment

jedcunningham left a comment

nwalens commented Sep 10, 2021

jedcunningham left a comment

nwalens commented Sep 20, 2021

jedcunningham commented Sep 20, 2021

kaxil commented Sep 20, 2021

potiuk commented Sep 24, 2021

github-actions bot commented Sep 24, 2021

nwalens commented Sep 26, 2021

potiuk commented Sep 27, 2021

boring-cyborg bot commented Sep 28, 2021

potiuk commented Sep 28, 2021

nwalens commented Sep 28, 2021

Allow airflow standard images to run in openshift utilising the official helm chart #18136 #18147

Allow airflow standard images to run in openshift utilising the official helm chart #18136 #18147

Conversation

nwalens commented Sep 10, 2021 • edited Loading

boring-cyborg bot commented Sep 10, 2021

potiuk left a comment

Choose a reason for hiding this comment

jedcunningham left a comment

Choose a reason for hiding this comment

nwalens commented Sep 10, 2021

jedcunningham left a comment

Choose a reason for hiding this comment

nwalens commented Sep 20, 2021

jedcunningham commented Sep 20, 2021

kaxil commented Sep 20, 2021

potiuk commented Sep 24, 2021

github-actions bot commented Sep 24, 2021

nwalens commented Sep 26, 2021

potiuk commented Sep 27, 2021

boring-cyborg bot commented Sep 28, 2021

potiuk commented Sep 28, 2021

nwalens commented Sep 28, 2021

nwalens commented Sep 10, 2021 •

edited

Loading