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

Add ClusterPolicyViolation support to airflow local settings #10282

Merged
merged 22 commits into from
Aug 12, 2020
Merged

Add ClusterPolicyViolation support to airflow local settings #10282

merged 22 commits into from
Aug 12, 2020

Conversation

jaketf
Copy link
Contributor

@jaketf jaketf commented Aug 10, 2020

This change will allow users to throw other exceptions (namely AirflowClusterPolicyViolation) than DagCycleException as part of Cluster Policies.

This can be helpful for running checks on tasks / DAGs (e.g. asserting task has a non-airflow owner) and failing to run tasks aren't compliant with these checks.

This is meant as a tool for airflow admins to prevent user mistakes (especially in shared airlfow infrastructure with newbies) than as a strong technical control for security / compliance posture.

CC: @potiuk @mik-laj who are familiar with this change.

@mik-laj
Copy link
Member

mik-laj commented Aug 10, 2020

@jaketf Can you add some docs?

@mik-laj
Copy link
Member

mik-laj commented Aug 10, 2020

I don't think we need a separate option in airflow_local_settings. I think we can use the cluster policy. The example configuration file, you can find in example_config/airflow_local_settings.py in our project.

docs/concepts.rst Outdated Show resolved Hide resolved
docs/concepts.rst Outdated Show resolved Hide resolved
docs/concepts.rst Outdated Show resolved Hide resolved
docs/concepts.rst Outdated Show resolved Hide resolved
docs/concepts.rst Outdated Show resolved Hide resolved
docs/concepts.rst Outdated Show resolved Hide resolved
docs/concepts.rst Outdated Show resolved Hide resolved
docs/concepts.rst Outdated Show resolved Hide resolved
docs/concepts.rst Outdated Show resolved Hide resolved
docs/concepts.rst Outdated Show resolved Hide resolved
@mik-laj
Copy link
Member

mik-laj commented Aug 11, 2020

@kaxil Thanks for the review. I was just about to ask you for this as I am the author of some of the code for this change and an independent review would be helpful.

docs/concepts.rst Outdated Show resolved Hide resolved
tests/models/test_dagbag.py Outdated Show resolved Hide resolved
tests/models/test_dagbag.py Outdated Show resolved Hide resolved
tests/test_local_settings.py Outdated Show resolved Hide resolved
Jacob Ferriero and others added 4 commits August 11, 2020 17:25
Co-authored-by: Kaxil Naik <[email protected]>
Co-authored-by: Kaxil Naik <[email protected]>
Co-authored-by: Kaxil Naik <[email protected]>
Co-authored-by: Kaxil Naik <[email protected]>
@jaketf
Copy link
Contributor Author

jaketf commented Aug 12, 2020

@kaxil @mik-laj (please ignore my rage committing to get CI checks to pass) This is ready for review again.

@jaketf jaketf requested review from kaxil and mik-laj August 12, 2020 21:11
@kaxil kaxil merged commit 7f76b8b into apache:master Aug 12, 2020
@kaxil
Copy link
Member

kaxil commented Aug 12, 2020

👏 Nice work @jaketf

@potiuk
Copy link
Member

potiuk commented Aug 13, 2020

Indeed :).

@kaxil kaxil added this to the Airflow 1.10.12 milestone Aug 14, 2020
kaxil pushed a commit that referenced this pull request Aug 14, 2020
This change will allow users to throw other exceptions (namely `AirflowClusterPolicyViolation`) than `DagCycleException` as part of Cluster Policies.

This can be helpful for running checks on tasks / DAGs (e.g. asserting task has a non-airflow owner) and failing to run tasks aren't compliant with these checks.

This is meant as a tool for airflow admins to prevent user mistakes (especially in shared Airflow infrastructure with newbies) than as a strong technical control for security/compliance posture.

(cherry picked from commit 7f76b8b)
kaxil pushed a commit that referenced this pull request Aug 15, 2020
This change will allow users to throw other exceptions (namely `AirflowClusterPolicyViolation`) than `DagCycleException` as part of Cluster Policies.

This can be helpful for running checks on tasks / DAGs (e.g. asserting task has a non-airflow owner) and failing to run tasks aren't compliant with these checks.

This is meant as a tool for airflow admins to prevent user mistakes (especially in shared Airflow infrastructure with newbies) than as a strong technical control for security/compliance posture.

(cherry picked from commit 7f76b8b)
kaxil pushed a commit that referenced this pull request Aug 15, 2020
This change will allow users to throw other exceptions (namely `AirflowClusterPolicyViolation`) than `DagCycleException` as part of Cluster Policies.

This can be helpful for running checks on tasks / DAGs (e.g. asserting task has a non-airflow owner) and failing to run tasks aren't compliant with these checks.

This is meant as a tool for airflow admins to prevent user mistakes (especially in shared Airflow infrastructure with newbies) than as a strong technical control for security/compliance posture.

(cherry picked from commit 7f76b8b)
potiuk pushed a commit to PolideaInternal/airflow that referenced this pull request Oct 9, 2020
The custom ClusterPolicyViolation has been added in apache#10282
This one adds more comprehensive test to it.
potiuk added a commit that referenced this pull request Oct 9, 2020
The custom ClusterPolicyViolation has been added in #10282
This one adds more comprehensive test to it.

Co-authored-by: Jacob Ferriero <[email protected]>
cfei18 pushed a commit to cfei18/incubator-airflow that referenced this pull request Mar 5, 2021
…10282)

This change will allow users to throw other exceptions (namely `AirflowClusterPolicyViolation`) than `DagCycleException` as part of Cluster Policies.

This can be helpful for running checks on tasks / DAGs (e.g. asserting task has a non-airflow owner) and failing to run tasks aren't compliant with these checks.

This is meant as a tool for airflow admins to prevent user mistakes (especially in shared Airflow infrastructure with newbies) than as a strong technical control for security/compliance posture.

(cherry picked from commit 7f76b8b)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants