Re: [PR] fix: add SCC to dag processor [airflow]
eladkal merged PR #51080: URL: https://github.com/apache/airflow/pull/51080 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] fix: add SCC to dag processor [airflow]
eladkal commented on PR #51080: URL: https://github.com/apache/airflow/pull/51080#issuecomment-2966116259 Needs rebase -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] fix: add SCC to dag processor [airflow]
kesem0811 commented on code in PR #51080:
URL: https://github.com/apache/airflow/pull/51080#discussion_r2140088435
##
helm-tests/tests/helm_tests/security/test_scc_rolebinding.py:
##
@@ -61,6 +61,29 @@ def test_create_scc(self, rbac_enabled, scc_enabled,
created):
assert jmespath.search("subjects[7].name", docs[0]) ==
"release-name-airflow-create-user-job"
assert jmespath.search("subjects[8].name", docs[0]) ==
"release-name-airflow-cleanup"
[email protected](
+"dag_processor_enabled",
+[
+(True),
+(False),
+],
+)
+def test_scc_subjects_include_dag_processor(self, dag_processor_enabled):
Review Comment:
Thanks for your review!
I’ve removed the new test and validated that the existing tests still cover
the behavior.
I also considered changing the default to False, but doing so caused many
other tests to fail. Since the DAG processor will always be enabled in Airflow
3, I believe changing the default value isn't necessary at this point.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] fix: add SCC to dag processor [airflow]
romsharon98 commented on code in PR #51080:
URL: https://github.com/apache/airflow/pull/51080#discussion_r2136940830
##
helm-tests/tests/helm_tests/security/test_scc_rolebinding.py:
##
@@ -61,6 +61,29 @@ def test_create_scc(self, rbac_enabled, scc_enabled,
created):
assert jmespath.search("subjects[7].name", docs[0]) ==
"release-name-airflow-create-user-job"
assert jmespath.search("subjects[8].name", docs[0]) ==
"release-name-airflow-cleanup"
[email protected](
+"dag_processor_enabled",
+[
+(True),
+(False),
+],
+)
+def test_scc_subjects_include_dag_processor(self, dag_processor_enabled):
Review Comment:
Thanks for the explanation. I have two questions:
- Why do we need a separate test focusing on DagProcessor? What makes this
component more special than the others? In the end, it uses the same setup and
assertion as the tests above, so it seems like they could be combined.
- Why not just change the default value to False?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] fix: add SCC to dag processor [airflow]
kesem0811 commented on code in PR #51080:
URL: https://github.com/apache/airflow/pull/51080#discussion_r2135403077
##
helm-tests/tests/helm_tests/security/test_scc_rolebinding.py:
##
@@ -61,6 +61,29 @@ def test_create_scc(self, rbac_enabled, scc_enabled,
created):
assert jmespath.search("subjects[7].name", docs[0]) ==
"release-name-airflow-create-user-job"
assert jmespath.search("subjects[8].name", docs[0]) ==
"release-name-airflow-cleanup"
[email protected](
+"dag_processor_enabled",
+[
+(True),
+(False),
+],
+)
+def test_scc_subjects_include_dag_processor(self, dag_processor_enabled):
Review Comment:
You're right that technically this could be merged into the existing test,
but I separated it for two main reasons:
Different focus of validation:
The first test checks whether the SCC RoleBinding object is created at all,
based on the rbac.create and rbac.createSCCRoleBinding values.
The second test focuses specifically on how the dagProcessor.enabled value
affects the list of subjects in the generated RoleBinding. It's verifying
whether the dag processor component is included or excluded depending on
whether it's enabled.
Special handling of the default value:
The dagProcessor.enabled field has a default value of ~ (i.e., null), unlike
the other components. That’s why I simulated both True, False, and unset (null)
scenarios in a controlled way.
Keeping this separate helped ensure the logic around the enabled value is
fully tested without overloading the first test. But I’m open to combining them
if you still think it better...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] fix: add SCC to dag processor [airflow]
romsharon98 commented on code in PR #51080:
URL: https://github.com/apache/airflow/pull/51080#discussion_r2134754761
##
helm-tests/tests/helm_tests/security/test_scc_rolebinding.py:
##
@@ -61,6 +61,29 @@ def test_create_scc(self, rbac_enabled, scc_enabled,
created):
assert jmespath.search("subjects[7].name", docs[0]) ==
"release-name-airflow-create-user-job"
assert jmespath.search("subjects[8].name", docs[0]) ==
"release-name-airflow-cleanup"
[email protected](
+"dag_processor_enabled",
+[
+(True),
+(False),
+],
+)
+def test_scc_subjects_include_dag_processor(self, dag_processor_enabled):
Review Comment:
You can combine this test with the upper test
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] fix: add SCC to dag processor [airflow]
jedcunningham commented on PR #51080: URL: https://github.com/apache/airflow/pull/51080#issuecomment-2916703303 Thanks for the PR @kesem0811. Can you add test coverage [here](https://github.com/apache/airflow/blob/main/helm-tests/tests/helm_tests/security/test_scc_rolebinding.py#L36)? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] fix: add SCC to dag processor [airflow]
romsharon98 commented on code in PR #51080:
URL: https://github.com/apache/airflow/pull/51080#discussion_r2109141024
##
chart/templates/rbac/security-context-constraint-rolebinding.yaml:
##
@@ -90,4 +90,9 @@ subjects:
name: {{ include "cleanup.serviceAccountName" . }}
namespace: "{{ .Release.Namespace }}"
{{- end }}
+ {{- if and .Values.dagProcessor.enabled }}
Review Comment:
```suggestion
{{- if .Values.dagProcessor.enabled }}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] fix: add SCC to dag processor [airflow]
boring-cyborg[bot] commented on PR #51080: URL: https://github.com/apache/airflow/pull/51080#issuecomment-2910083186 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 Contributors' Guide (https://github.com/apache/airflow/blob/main/contributing-docs/README.rst) Here are some useful points: - Pay attention to the quality of your code (ruff, mypy and type annotations). Our [pre-commits]( https://github.com/apache/airflow/blob/main/contributing-docs/08_static_code_checks.rst#prerequisites-for-pre-commit-hooks) 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](https://github.com/apache/airflow/blob/main/airflow-core/docs/howto/custom-operator.rst) Consider adding an example DAG that shows how users should use it. - Consider using [Breeze environment](https://github.com/apache/airflow/blob/main/dev/breeze/doc/README.rst) 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](https://www.apache.org/foundation/policies/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]( https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#coding-style-and-best-practices). - Always keep your Pull Requests rebased, otherwise your build might fail due to changes not related to your commits. Apache Airflow is a community-driven project and together we are making it better 🚀. In case of doubts contact the developers at: Mailing List: [email protected] Slack: https://s.apache.org/airflow-slack -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
