You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "BasPH (via GitHub)" <gi...@apache.org> on 2023/12/18 21:01:48 UTC
[PR] Implement pluggable connection policy [airflow]
BasPH opened a new pull request, #36291:
URL: https://github.com/apache/airflow/pull/36291
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of an existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
This PR implements a pluggable connection policy. This can apply validation rules e.g. only access to a pre-defined list of connection ids in a secrets backend, or apply some business logic such as prefixing the given connection id based on some property such as DAG owner.
Example:
```python
def connection_policy(conn_id: str) -> str:
"""
This connection policy prefixes the connection id with "bi/" in case the owner is set to "bi", and raises
an AirflowClusterPolicyViolation in case an unknown owner is found.
"""
dagowner_connid_prefixes = {"bi": "bi/"}
from airflow.operators.python import get_current_context
task_context = get_current_context()
dag_owner = task_context["dag"].owner
try:
return dagowner_connid_prefixes[dag_owner] + conn_id
except KeyError:
raise AirflowClusterPolicyViolation(
f"DAG owner '{dag_owner}' not found in list of owners to route a connection. "
"Consult your cluster admin."
)
```
<!-- Please keep an empty line above the dashes. -->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
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](https://github.com/apache/airflow/tree/main/newsfragments).
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Implement pluggable connection policy [airflow]
Posted by "dstandish (via GitHub)" <gi...@apache.org>.
dstandish commented on code in PR #36291:
URL: https://github.com/apache/airflow/pull/36291#discussion_r1430690414
##########
airflow/policies.py:
##########
@@ -117,6 +117,19 @@ def get_dagbag_import_timeout(dag_file_path: str) -> int | float: # type: ignor
"""
+@local_settings_hookspec
+def connection_policy(conn_id: str) -> str: # type: ignore[empty-body]
+ """
+ Alter and/or apply validation rules on the provided connection id.
+
+ When used in combination with the task context (``airflow.operators.python.get_current_context``), you can
+ apply logic, e.g. to check if the requested connection id was requested by a DAG with a certain owner.
+
+ To support connection id alterations at runtime (typically prefixes are added), a connection id must be
+ returned, even when not altering the connection id.
+ """
Review Comment:
I am not sure, but I also wonder whether we should add a disclaimer that this could provide a false sense of security? To me it makes sense to have this because people want _something_, and this is about as good as we can do right now. But, it's pretty easy to circumvent e.g. with `secrets_backend_list[0].get_connection`.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Implement pluggable connection policy [airflow]
Posted by "dstandish (via GitHub)" <gi...@apache.org>.
dstandish commented on code in PR #36291:
URL: https://github.com/apache/airflow/pull/36291#discussion_r1430691088
##########
airflow/hooks/base.py:
##########
@@ -77,6 +78,8 @@ def get_connection(cls, conn_id: str) -> Connection:
:param conn_id: connection id
:return: connection
"""
+ conn_id = settings.connection_policy(conn_id=conn_id)
Review Comment:
I wonder if `Connection.get_connection_from_secrets` would be a better location
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Implement pluggable connection policy [airflow]
Posted by "dstandish (via GitHub)" <gi...@apache.org>.
dstandish commented on code in PR #36291:
URL: https://github.com/apache/airflow/pull/36291#discussion_r1430676913
##########
airflow/policies.py:
##########
@@ -117,6 +117,19 @@ def get_dagbag_import_timeout(dag_file_path: str) -> int | float: # type: ignor
"""
+@local_settings_hookspec
+def connection_policy(conn_id: str) -> str: # type: ignore[empty-body]
Review Comment:
I thought for a second that maybe we should call this `connection_id_policy` or something because... e.g. with task policy or dag policy, you get and can mutate the actual object -- but here we only touch the conn_id. So the naming is perhaps slightly inconsistent but that's nit picking.
I suppose we could call it `connection_retrieval_policy`? Or leave it 🤷
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Implement pluggable connection policy [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #36291:
URL: https://github.com/apache/airflow/pull/36291#issuecomment-1861767802
This is an interesting idea, but I think we should be very clear in the documentation and description that it's not a security feature. Any DAG Author can simply instantiate the connection via creating the model and retrieving it. There is a risk that this will give the users a false sense of security if we are not explicit about it.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Implement pluggable connection policy [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #36291:
URL: https://github.com/apache/airflow/pull/36291#issuecomment-1861770193
Just to add - I can very easily imagine - that if we don't do it, that someone will raise a security report that you can bypass it. And if somoene does it and it's not clearly explained in our security model, then we will have to well, fix it.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Implement pluggable connection policy [airflow]
Posted by "BasPH (via GitHub)" <gi...@apache.org>.
BasPH commented on PR #36291:
URL: https://github.com/apache/airflow/pull/36291#issuecomment-1863427331
@dstandish @potiuk I didn't intend this to be a security feature. I gave your comments some thought and decided to close the PR. Security isn't something I want to be fuzzy about and stating we put a secure-looking policy in Airflow with a disclaimer that it isn't a security feature somewhere in the docs just doesn't work for me.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Implement pluggable connection policy [airflow]
Posted by "BasPH (via GitHub)" <gi...@apache.org>.
BasPH commented on code in PR #36291:
URL: https://github.com/apache/airflow/pull/36291#discussion_r1431407292
##########
airflow/hooks/base.py:
##########
@@ -77,6 +78,8 @@ def get_connection(cls, conn_id: str) -> Connection:
:param conn_id: connection id
:return: connection
"""
+ conn_id = settings.connection_policy(conn_id=conn_id)
Review Comment:
Possibly, but I don't think it matters that much?
The policy would have to be applied before the first request to any secrets system. We could apply it inside `Connection.get_connection_from_secrets`, but it would have to be the first statement in that method, because after that we start requesting secrets. That applies the policy one level deeper in the stack but logically it wouldn't make a difference.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Implement pluggable connection policy [airflow]
Posted by "BasPH (via GitHub)" <gi...@apache.org>.
BasPH closed pull request #36291: Implement pluggable connection policy
URL: https://github.com/apache/airflow/pull/36291
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Implement pluggable connection policy [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #36291:
URL: https://github.com/apache/airflow/pull/36291#discussion_r1430699207
##########
airflow/policies.py:
##########
@@ -117,6 +117,19 @@ def get_dagbag_import_timeout(dag_file_path: str) -> int | float: # type: ignor
"""
+@local_settings_hookspec
+def connection_policy(conn_id: str) -> str: # type: ignore[empty-body]
+ """
+ Alter and/or apply validation rules on the provided connection id.
+
+ When used in combination with the task context (``airflow.operators.python.get_current_context``), you can
+ apply logic, e.g. to check if the requested connection id was requested by a DAG with a certain owner.
+
+ To support connection id alterations at runtime (typically prefixes are added), a connection id must be
+ returned, even when not altering the connection id.
+ """
Review Comment:
Ah.. I **just** see that very same comment.
##########
airflow/policies.py:
##########
@@ -117,6 +117,19 @@ def get_dagbag_import_timeout(dag_file_path: str) -> int | float: # type: ignor
"""
+@local_settings_hookspec
+def connection_policy(conn_id: str) -> str: # type: ignore[empty-body]
+ """
+ Alter and/or apply validation rules on the provided connection id.
+
+ When used in combination with the task context (``airflow.operators.python.get_current_context``), you can
+ apply logic, e.g. to check if the requested connection id was requested by a DAG with a certain owner.
+
+ To support connection id alterations at runtime (typically prefixes are added), a connection id must be
+ returned, even when not altering the connection id.
+ """
Review Comment:
Ah.. I **just** see that we have very same comment.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org