add: container securityContext not available in podSecurityContext by ChrisFraun · Pull Request #24588 · apache/airflow

ChrisFraun · 2022-06-21T20:43:14Z

This PR is only a small change in the helm chart of Airflow.

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.

In case of existing issue, reference it using one of the following:

closes: #27612

Could not find any related issue at first sight.

^ Add meaningful description above

Read the Pull Request Guidelines for more information.

Test was a simple helm lint . on chart level as well as a successful deployment.
WARNING: Kubernetes configuration file is group-readable. This is insecure. Location: /Users/christophfraundorfer/.kube/config
WARNING: Kubernetes configuration file is world-readable. This is insecure. Location: /Users/christophfraundorfer/.kube/config
==> Linting .

1 chart(s) linted, 0 chart(s) failed

boring-cyborg · 2022-06-21T20:43:17Z

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 · 2022-06-29T05:29:16Z

I'd say that better action would really be to have two separate contexts and maybe simply - rather than adding "containerSecurityContext", introduce values/securityContexts/pod and values/securityContexts/container as a bit more detailed structure in values, allowing more fine-grained security context configuration (while keeping deprecated default)

Something like:

I think that would be much more versatile and rather easy to use configure (more detailed values could be merged into the defaults).

ChrisFraun · 2022-06-29T08:09:14Z

Updated the change, can you check it?

ChrisFraun · 2022-07-11T09:24:12Z

is the change how you imagined it @potiuk ?

potiuk · 2022-07-11T11:05:11Z

Yep it looks good :). I am not sure if there is an easy way to "warn" if you use the deprecated configuration (I think this is the only change that I would like to see here) - and of course some tests would be useful. @jedcunningham @dstandish - WDYT?

jedcunningham · 2022-07-11T18:30:07Z

Deprecation warnings happen in NOTES, e.g:

airflow/chart/templates/NOTES.txt

Lines 160 to 166 in 178af9d

    
           {{- if .Values.flower.extraNetworkPolicies }} 
        
           DEPRECATION WARNING: 
        
              `flower.extraNetworkPolicies` has been renamed to `flower.networkPolicy.peers`. 
        
              Please change your values as support for the old name will be dropped in a future release. 
        
           {{- end }}

This should be expanded to cover all of the components as well.

I'd also like to see the component specific override come from the components config section, e.g. scheduler.securityContexts.pod instead of securityContexts.pod.scheduler.

ChrisFraun · 2022-07-19T09:18:10Z

Hi, sorry for the late reply!
what was added:

added deprecation notes
Modified values.yaml (webserver, scheduler, statsd)
Modified deployments (webserver, scheduler, statsd)
Modified values.schema (webserver, scheduler, statsd)

Tested with helm lint . and helm template . --debug - both compiled.
Is this the way you imagine it? @potiuk @jedcunningham

potiuk · 2022-07-21T08:09:05Z

still some static checks are failing,

ChrisFraun · 2022-07-26T09:21:04Z

Hi @potiuk can you maybe point me into a direction on why the checks are not running through? - I don't see the problem so far.

jedcunningham

Probably worth adding some test coverage to make sure the old deprecated options still work.

jedcunningham · 2022-08-09T18:24:48Z

+    allowPrivilegeEscalation: false
+    capabilities:
+      drop:
+        - ALL


This is the default, yeah? Not sure we need to expand it here.

I removed it from here and put it in the helper:
{{/*
Set the default value for container securityContext
If no value is passed for securityContext.container or .securityContext.container, defaults to deny privileges escallation and dropping all POSIX capabilities.

+------------------------+ +-----------------+ +-------------------------+ | <node>.securityContext.container | -> | securityContext.container | -> | allowPrivilegesEscalation: false, capabilities.drop: [ALL]| +------------------------+ +-----------------+ +-------------------------+

The template can be called like so:
include "airflowSecurityContextContainer" (list . .Values.webserver)

Where . is the global variables scope and .Values.webserver the local variables scope for the webserver template.
*/}}
{{- define "airflowSecurityContextContainer" -}}
{{- $ := index . 0 -}}
{{- with index . 1 }}
{{- if .securityContext.container -}}
{{ toYaml .securityContext.container | print }}
{{- else if $.Values.securityContext.container -}}
{{ toYaml $.Values.securityContext.container | print }}
{{- else -}}
allowPrivilegeEscalation: false
capabilities:
drop:
- ALL
{{- end -}}
{{- end -}}
{{- end -}}

Sorry, should have been more clear. Isn't that the default kubernetes sets if you provide nothing? If so, I'd rather we not set it at all.

I think you have to set them yourself, they are not automatically set.

Sorry for the delay, coming back to this now.

I had this backwards - this removes rights. While I agree this is a good sane default, we can't make this change in a minor release without breaking backward compatibility. Given that, I think for now we should keep the default as nothing.

ChrisFraun · 2022-08-10T13:11:23Z

Hi @jedcunningham thanks a lot for the input! I tried to stick to your comments, but feel free to add some more :)

jedcunningham · 2022-08-11T16:54:35Z

-Set the default value for securityContext
-If no value is passed for securityContext or <node>.securityContext, defaults to global uid and gid.
+Set the default value for pod securityContext
+If no value is passed for securityContext.pod or <node>.securityContext.pod, defaults to global uid and gid.


I think we want this order of precedence:

<node>.securityContexts.pod <node>.securityContext securityContexts.pod securityContext (backcompat for deprecated config) uid/gid

So I think this template still needs a little work, right? Also feel free to rename this airflowPodSecurityContext to make it more clear.

You are right, I tried to adapt to your suggestion and renamed also the template.
Btw airflowSecurityContextIds was not used anywhere so the renamed version airflowPodSecurityContextsIds can probably be deleted.

Hope I got it right this time.

Hi, I think airflowSecurityContextIds is used on workers-deployment to set the uid and gid during a chown. Not sure if has changed.

ChrisFraun · 2022-09-07T07:02:07Z

Hi, I am still getting this error during the images build: ERROR: denied: permission_denied: write_package
It does not seem to me that this has something to do with my changes.
Any advice?

potiuk · 2022-09-07T16:39:25Z

Hi, I am still getting this error during the images build: ERROR: denied: permission_denied: write_package It does not seem to me that this has something to do with my changes. Any advice?

Rebase please. GitHub seemed to have a rough day.

jedcunningham · 2022-09-16T18:26:06Z

+
+Where `.` is the global variables scope and `.Values.webserver` the local variables scope for the webserver template.
+*/}}
+{{- define "airflowContainerSecurityContext" -}}


I wonder if we should call this containerSecurityContext instead, since it's used for more than just Airflow containers?

Wait, this isn't even used? Let's drop it?

I added it to the charts

potiuk · 2022-09-19T20:27:21Z

Conflict to solve :(

ChrisFraun · 2022-09-20T10:14:15Z

Tried to add the changes demanded in #25985

jedcunningham

I suspect there are more of this scenario than just the 2 below:

anja-istenic · 2022-09-26T06:43:52Z

Thanks @ChrisFraun for reconciling our 2 PRs into one (#25985 and this one). These changes look good to me 👍

potiuk · 2022-10-25T19:15:43Z

Running the tests now (sorry @ChrisFraun for this all taking so long - just returned from Holidays and trying to catch-up - let's see how this one looks like for tests and I will give it another pass).

potiuk · 2022-10-31T04:46:15Z

errors ?

malthe

I have tested this out and it works beautifully!

…apache#29913) * Add a failing test to make it pass * use partial_kwargs when they are provide and override only None values by dag default values * update the test and check if the values are filled in the right order * fix overriding retry_delay with default value when it is equal to 0 * add missing default value for inlets and outlets * set partial_kwargs dict type to dict[str, Any] and remove type ignore comments * create a dict for default values and use NotSet instead of None to support None as accepted value * update partial typing by removing None type from some args and set NotSet for all args * Tweak kwarg merging slightly This should improve iteration a bit, I think. * Fix unit tests --------- Co-authored-by: Tzu-ping Chung <uranusjr@gmail.com>

This PR consistent mostly of code that was created in OpenLineage project. It consists of - Provider wiring - OpenLineageListener that uses Listener API to get notification about changes to TaskInstance and Dag states - Extractor framework, which is used to extract lineage information from particular operators. It's ment to be replaced by direct implementation of lineage features in later phase and extracting them using DefaultExtractor. This PR does not include actual extractors, but code around using and registering them. - OpenLineageAdapter that translates extracted information to OpenLineage events. - Utils around specific Airflow OL facets and features This is a base implementation that's not ment to be released yet, but to add code modified to be consistent with Airflow standards, get early feedback and provide canvas to add later features, docs, tests on. Signed-off-by: Jakub Dardzinski <kuba0221@gmail.com>

…he#30640)

…pache#29809) * Adding configuration to control retry parameters for k8s api client * Handling review comments * Fixing code bug * Fixing failing tests * Temporary commit with UT wip * Fixing unit test * Fixing the strict checks * Handling review comments from Hussein * Revert "Handling review comments from Hussein" This reverts commit fa3bc26. * Fixing failing ut * Reverting bad hack * Updating logic in kube_client.py Co-authored-by: Hussein Awala <hussein@awala.fr> * Fixing unit tests * Fixing unit tests * Handling review comments from Ash * Fix loading mock call args for python3.7 * Apply suggestions from code review * fix static check * add in 2.6.0 --------- Co-authored-by: Amogh <adesai@cloudera.com> Co-authored-by: Hussein Awala <houssein.awala.96@gmail.com>

* fix(chart): webserver probes timeout and period * Update default values in JSON schema to reflect values.yaml * remove defautl templated values

…ype` (apache#30595) * Add while initilizing * Add while initilizing * Add logic to pick either client_type or resource_type * Add test case * Assert expected path

* update run clear+mark, update task clear * add mark as tasks and include list of affected tasks * Add support for mapped tasks, add shared modal component * Clean up styling, restore warning for past/future tg clear

--------- Co-authored-by: Hussein Awala <hussein@awala.fr> Co-authored-by: Hussein Awala <houssein.awala.96@gmail.com>

…apache#29065) * When clearing task instances try to get associated DAGs from database. This fixes problems when recursively clearing task instances across multiple DAGs: * Task instances in downstream DAGs weren't having their `max_tries` property incremented, which could cause downstream external task sensors in reschedule mode to instantly time out (issue apache#29049). * Task instances in downstream DAGs could have some of their properties overridden by an unrelated task in the upstream DAG if they had the same task ID. * Use session fixture for new `test_clear_task_instances_without_dag_param` test. * Use session fixture for new `test_clear_task_instances_in_multiple_dags` test. --------- Co-authored-by: eladkal <45845474+eladkal@users.noreply.github.com>

--------- Co-authored-by: Tzu-ping Chung <uranusjr@gmail.com> Co-authored-by: Ephraim Anierobi <splendidzigy24@gmail.com>

…30646)

--------- Co-authored-by: Josh Fell <48934154+josh-fell@users.noreply.github.com>

Co-authored-by: Mikaël Ducharme <mikaelducharme@gmail.com>

…hrisFraun/airflow into feat/adding-container-security-context

ChrisFraun · 2023-04-20T08:32:39Z

I messed up :(

I was trying to resolve the conflicts locally but did not realise that vscode is adding all the changes in the remote branch...
I am not sure what to do exactly from here, maybe it is even smarter to create a new PR and add the changes to the most current version?

mikaeld · 2023-04-20T15:05:39Z

You likely can revert and/or rewrite history to fix this since this is on your fork. If you can't fix the conflicts, we would need to link this PR in the new one to keep the context. @potiuk thoughts on this?

potiuk · 2023-04-22T10:30:14Z

Create a new PR and make your changes there again. It happens, and has the opportunity to re-review your own code while applying.

Closing it for now.

mikaeld · 2023-04-25T14:10:22Z

@ChrisFraun did you plan on re-creating a PR? I don't mind doing it if you did not plan on re-creating it.

ChrisFraun · 2023-05-02T08:03:16Z

Feel free to go ahead @mikaeld! 👍

ChrisFraun requested review from dstandish and jedcunningham as code owners June 21, 2022 20:43

boring-cyborg Bot added the area:helm-chart Airflow Helm Chart label Jun 21, 2022

jedcunningham reviewed Aug 9, 2022

View reviewed changes

jedcunningham reviewed Aug 11, 2022

View reviewed changes

jedcunningham mentioned this pull request Sep 12, 2022

feat(chart): add security context on container levels #25985

Closed

jedcunningham reviewed Sep 16, 2022

View reviewed changes

jedcunningham reviewed Sep 20, 2022

View reviewed changes

Comment thread chart/templates/scheduler/scheduler-deployment.yaml Outdated

Comment thread chart/templates/workers/worker-deployment.yaml Outdated

jedcunningham reviewed Oct 3, 2022

View reviewed changes

Comment thread chart/values.schema.json

eladkal added this to the Airflow Helm Chart 1.8.0 milestone Oct 24, 2022

ecerulm mentioned this pull request Oct 28, 2022

Make helm chart compliant with the Pod Security Standard "restricted" #27318

Closed

2 tasks

malthe self-requested a review November 3, 2022 14:46

malthe approved these changes Nov 3, 2022

View reviewed changes

hussein-awala and others added 23 commits April 20, 2023 09:46

Add v2-6-test and v2-6-stable to codecov and protected branches (apac…

c62559b

…he#30640)

fix(chart): webserver probes timeout and period. (apache#30609)

ca6b7ed

* fix(chart): webserver probes timeout and period * Update default values in JSON schema to reflect values.yaml * remove defautl templated values

Clarify release announcements on social media (apache#30639)

25ea84c

DynamoDBHook - waiter_path() to consider resource_type or `client_t…

b6aab0e

…ype` (apache#30595) * Add while initilizing * Add while initilizing * Add logic to pick either client_type or resource_type * Add test case * Assert expected path

Improve task & run actions ux in grid view (apache#30373)

8b2ed02

* update run clear+mark, update task clear * add mark as tasks and include list of affected tasks * Add support for mapped tasks, add shared modal component * Clean up styling, restore warning for past/future tg clear

Add command to get DAG Details via CLI (apache#30432)

c5ebab1

--------- Co-authored-by: Hussein Awala <hussein@awala.fr> Co-authored-by: Hussein Awala <houssein.awala.96@gmail.com>

Organize Amazon providers docs index (apache#30541)

e2ce991

preload airflow imports before dag parsing to save time (apache#30495)

9de8ee5

--------- Co-authored-by: Tzu-ping Chung <uranusjr@gmail.com> Co-authored-by: Ephraim Anierobi <splendidzigy24@gmail.com>

Add delete inactive run functionality to databricks provider (apache#…

4d4350c

…30646)

Create audit_logs.rst (apache#30405)

dfaed19

--------- Co-authored-by: Josh Fell <48934154+josh-fell@users.noreply.github.com>

Present affected task instances as table (apache#30633)

ef1b53a

Helm chart 1.9.0 has been released (apache#30649)

a0a754c

Add 2.6.0b1 to issue template (apache#30652)

081f087

add missing project_id in BigQueryGetDataOperator (apache#30651)

d486425

Update chart/templates/_helpers.yaml

a60558e

Co-authored-by: Mikaël Ducharme <mikaelducharme@gmail.com>

Update chart/templates/_helpers.yaml

54b40c1

Co-authored-by: Mikaël Ducharme <mikaelducharme@gmail.com>

Update chart/templates/_helpers.yaml

62648a3

Co-authored-by: Mikaël Ducharme <mikaelducharme@gmail.com>

debugging: add securityContexts for dag gitSync

827e2cf

Merge branch 'feat/adding-container-security-context' of github.com:C…

1a04143

…hrisFraun/airflow into feat/adding-container-security-context

potiuk closed this Apr 22, 2023

mikaeld mentioned this pull request May 3, 2023

feat(chart): add container security context #31043

Merged

Uh oh!

Conversation

ChrisFraun commented Jun 21, 2022 • edited by jedcunningham Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

boring-cyborg Bot commented Jun 21, 2022

Uh oh!

potiuk commented Jun 29, 2022

Uh oh!

ChrisFraun commented Jun 29, 2022

Uh oh!

ChrisFraun commented Jul 11, 2022

Uh oh!

potiuk commented Jul 11, 2022

Uh oh!

jedcunningham commented Jul 11, 2022

Uh oh!

ChrisFraun commented Jul 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

potiuk commented Jul 21, 2022

Uh oh!

ChrisFraun commented Jul 26, 2022

Uh oh!

jedcunningham left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChrisFraun Aug 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ChrisFraun commented Aug 10, 2022

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChrisFraun Aug 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ChrisFraun commented Sep 7, 2022

Uh oh!

potiuk commented Sep 7, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

potiuk commented Sep 19, 2022

Uh oh!

ChrisFraun commented Sep 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jedcunningham left a comment

Choose a reason for hiding this comment

ChrisFraun commented Jun 21, 2022 •

edited by jedcunningham

Loading

ChrisFraun commented Jul 19, 2022 •

edited

Loading

ChrisFraun Aug 12, 2022 •

edited

Loading

ChrisFraun Aug 12, 2022 •

edited

Loading

ChrisFraun commented Sep 20, 2022 •

edited

Loading

ChrisFraun commented Apr 20, 2023 •

edited

Loading