You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "hussein-awala (via GitHub)" <gi...@apache.org> on 2023/02/28 21:24:57 UTC
[GitHub] [airflow] hussein-awala opened a new pull request, #29821: Add a check for not templateable fields
hussein-awala opened a new pull request, #29821:
URL: https://github.com/apache/airflow/pull/29821
closes: #29819
---
There are some fields processed by the scheduler and they are not templateable, but currently we don't have any check for these fields. In this PR a create an instance from `BaseOperator` and I check if there is a field in the `template_fields` to raise an exception, where all the `BaseOperator` task instance are processed and used by the scheduler during processing time.
```python
>>> from airflow.models import BaseOperator
>>> set(BaseOperator(task_id="base_task").__dict__.keys())
{'_pre_execute_hook', 'doc', 'executor_config', 'email', 'retry_exponential_backoff', 'depends_on_past', 'inlets', 'wait_for_downstream', 'email_on_failure', 'doc_json', 'execution_timeout', 'upstream_task_ids', 'task_id', 'sla', 'on_success_callback', 'trigger_rule', 'ignore_first_depends_on_past', 'doc_md', 'pool', '_BaseOperator__from_mapped', '_post_execute_hook', 'on_failure_callback', 'doc_rst', 'downstream_task_ids', 'email_on_retry', 'weight_rule', 'pool_slots', 'priority_weight', 'run_as_user', 'doc_yaml', 'on_retry_callback', 'do_xcom_push', 'max_retry_delay', '_BaseOperator__instantiated', 'retries', 'owner', '_BaseOperator__init_kwargs', 'params', 'queue', 'resources', 'max_active_tis_per_dag', '_log', 'wait_for_past_depends_before_skipping', 'retry_delay', 'on_execute_callback', 'outlets'}
```
--
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] Add a check for not templateable fields [airflow]
Posted by "hussein-awala (via GitHub)" <gi...@apache.org>.
hussein-awala commented on PR #29821:
URL: https://github.com/apache/airflow/pull/29821#issuecomment-1802749813
@gdavoian I agree with Jarek.
This PR was created to avoid the rendering issues with BaseOperator attributes like `execution_timeout`.
IMHO, you can create a test that loops over the excluded attributes, try to use a templated value, and check the value in runtime after rendering it. To know what attributes could be excluded, you can start by excluding all the params and keeping only the params that pass the 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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add a check for not templateable fields [airflow]
Posted by "gdavoian (via GitHub)" <gi...@apache.org>.
gdavoian commented on PR #29821:
URL: https://github.com/apache/airflow/pull/29821#issuecomment-1803354795
Hm, you were right, looks like only `email` is a field that is 100% safe to template (as I mentioned before, the field is heavily used by our team as `default_args["email"]` for each and every DAG). There is also `run_as_user` that seems to be safe as well, but I've never used it, so can't really comment on its usage. If you're fine with that, then I can open a PR to exclude `email` (and optionally `run_as_user` if you think it makes sense) from that check.
--
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
[GitHub] [airflow] Inetov commented on pull request #29821: Add a check for not templateable fields
Posted by "Inetov (via GitHub)" <gi...@apache.org>.
Inetov commented on PR #29821:
URL: https://github.com/apache/airflow/pull/29821#issuecomment-1612542756
@hussein-awala what if my custom operator needs to use templating for "params" field?
how to bypass the restriction introduced in this commit?
--
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
[GitHub] [airflow] eladkal commented on a diff in pull request #29821: Add a check for not templateable fields
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on code in PR #29821:
URL: https://github.com/apache/airflow/pull/29821#discussion_r1121242914
##########
airflow/serialization/serialized_objects.py:
##########
@@ -770,8 +772,12 @@ def _serialize_node(cls, op: BaseOperator | MappedOperator, include_deps: bool)
# Store all template_fields as they are if there are JSON Serializable
# If not, store them as strings
+ # And raise an exception if the field is not templateable
+ forbidden_fields = set(inspect.signature(BaseOperator.__init__).parameters.keys())
if op.template_fields:
for template_field in op.template_fields:
+ if template_field in forbidden_fields:
+ raise AirflowException(f"Cannot use {template_field} as template field")
Review Comment:
I think the only use case where we can not template fields is when user wants to template BaseOperator parameters which is something that can't be done. So maybe we can be more informative on the exception here?
--
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
[GitHub] [airflow] Inetov commented on pull request #29821: Add a check for not templateable fields
Posted by "Inetov (via GitHub)" <gi...@apache.org>.
Inetov commented on PR #29821:
URL: https://github.com/apache/airflow/pull/29821#issuecomment-1612643030
thanks @eladkal, but #31801 is still in the process of being solved, and I can no longer upgrade to 2.5.2 and higher.
@hussein-awala, maybe there is a temporary workaround?
--
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
[GitHub] [airflow] eladkal commented on pull request #29821: Add a check for not templateable fields
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on PR #29821:
URL: https://github.com/apache/airflow/pull/29821#issuecomment-1612596662
@Inetov @hussein-awala see feature request for this https://github.com/apache/airflow/issues/31801
--
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] Add a check for not templateable fields [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #29821:
URL: https://github.com/apache/airflow/pull/29821#issuecomment-1795509731
I think that's a good idea to make a PR to exclude some of the fields in this way. I think PR for that would be good @gdavoian
--
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] Add a check for not templateable fields [airflow]
Posted by "gdavoian (via GitHub)" <gi...@apache.org>.
gdavoian commented on PR #29821:
URL: https://github.com/apache/airflow/pull/29821#issuecomment-1802865640
> This PR was created to avoid the rendering issues with BaseOperator attributes like execution_timeout.
But how does this PR help solve the rendering issue? I understand that it tries to warn the users against their potentially mistaken intentions, though in fact it simply replaces one type of exception raised with another, reduces flexibility, and introduces a backward-incompatible change as a side effect. So the person who tried to add `execution_timeout` to `template_fields` still won't be able to do that, whereas those who are using Jinja templates for rendering some of the `BaseOperator` fields (e.g., `email`, `pool`, `doc_md`) in their custom operators simply won't be able to upgrade their Airflow environments since their working code will suddenly turn out to be broken. Thus, honestly, I don't see any benefits from this change at all...
> IMHO, you can create a test that loops over the excluded attributes, try to use a templated value, and check the value in runtime after rendering it. To know what attributes could be excluded, you can start by excluding all the params and keeping only the params that pass the test.
This sounds like a workaround for a workaround :) For some reason, you've prohibited the rendering of all the `BaseOperator` fields (I'm still not convinced of the need for this change), and now you're asking me to exclude some of the fields from that rule (the rule I don't even agree and the rule you can't really justify)...
--
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
[GitHub] [airflow] potiuk commented on pull request #29821: Add a check for not templateable fields
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #29821:
URL: https://github.com/apache/airflow/pull/29821#issuecomment-1454829985
Nice one!
--
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
[GitHub] [airflow] hussein-awala commented on pull request #29821: Add a check for not templateable fields
Posted by "hussein-awala (via GitHub)" <gi...@apache.org>.
hussein-awala commented on PR #29821:
URL: https://github.com/apache/airflow/pull/29821#issuecomment-1612563568
> @hussein-awala what if my custom operator needs to use templating for params field?
how to bypass the restriction introduced in this commit?
@Inetov This is not possible for now, I will check if templating `params` (and other parameters) is 100% safe, and if it's the case, maybe we can remove them from the excluded fields list.
--
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] Add a check for not templateable fields [airflow]
Posted by "gdavoian (via GitHub)" <gi...@apache.org>.
gdavoian commented on PR #29821:
URL: https://github.com/apache/airflow/pull/29821#issuecomment-1802186567
@potiuk do you have any ideas on how it would fit into `BaseOperator`? Because I can only think about something like `templateable_fields` (the exact naming isn't important at the moment) to exclude them from the check. But simultaneously having two entities like `template_fields` and `templateable_fields` would be very misleading: the former is a list of fields that **can be** templated and the latter is a list of fields that **are allowed to be** templated, meaning that some fields (e.g., `BaseOperator.email`) have to be added to both lists at the same time... Again, the exact naming doesn't matter here as soon as the whole idea is weird.
And that actually leads to my initial question: why did we ever need that check in the first place? `BaseOperator` doesn't define any `template_fields` by default, leaving that up to developers of third-party/custom operators to do that, which is completely fine. Thus, it's the responsibility of those developers to ensure their operators are valid and serializable, isn't 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
[GitHub] [airflow] hussein-awala commented on a diff in pull request #29821: Add a check for not templateable fields
Posted by "hussein-awala (via GitHub)" <gi...@apache.org>.
hussein-awala commented on code in PR #29821:
URL: https://github.com/apache/airflow/pull/29821#discussion_r1122294609
##########
airflow/serialization/serialized_objects.py:
##########
@@ -770,8 +772,12 @@ def _serialize_node(cls, op: BaseOperator | MappedOperator, include_deps: bool)
# Store all template_fields as they are if there are JSON Serializable
# If not, store them as strings
+ # And raise an exception if the field is not templateable
+ forbidden_fields = set(inspect.signature(BaseOperator.__init__).parameters.keys())
if op.template_fields:
for template_field in op.template_fields:
+ if template_field in forbidden_fields:
+ raise AirflowException(f"Cannot use {template_field} as template field")
Review Comment:
I updated the exception message to `Cannot template BaseOperator fields: {template_field}`, do you have another suggestion?
--
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] Add a check for not templateable fields [airflow]
Posted by "gdavoian (via GitHub)" <gi...@apache.org>.
gdavoian commented on PR #29821:
URL: https://github.com/apache/airflow/pull/29821#issuecomment-1795037963
@hussein-awala Unfortunately, this change is going to break our Airflow environment.
Is there any reason why `BaseOperator.email` can't be templated? E.g., we store different email lists as Airflow variables, and we don't want to use `Variable.get(...)` on the module-level of our scripts because it's actually an anti-pattern (opening a DB connection, making a request, etc.). So that's why we had to implement a function for adding `email` to `template_fields` for all subclasses of `BaseOperator` dynamically at run-time, which allowed us to use Jinja templates in `default_args["email"]` of our DAGs.
Actually, it's working just fine! But you're going to prohibit that in future versions, and I'd like to know the rationale behind that decision.
--
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] Add a check for not templateable fields [airflow]
Posted by "hussein-awala (via GitHub)" <gi...@apache.org>.
hussein-awala commented on PR #29821:
URL: https://github.com/apache/airflow/pull/29821#issuecomment-1802930988
> But how does this PR help solve the rendering issue?
Most of these attributes should not be used as a templated field, so the PR helped quickly fail the dag parsing instead of falling in runtime.
> I'm still not convinced of the need for this change
Most of the BaseOperator fields are used in parsing time and not runtime, for example, execution_date, priority_weight, the different callbacks, the different dependencies params, retry params, and the pool (I don't know why you think it could be templated, it's used to see if we can queue the task or not, so before executing the job), and even doc_md which is a tasks param more than task instance param.
For the email, we used it in the TaskInstance class when the TI fails, so after executing it, it's ok to exclude it. I'm not sure if we can exclude the other params; as I mentioned, most of them are used by the scheduler, executor, or webserver before executing the task.
--
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] Add a check for not templateable fields [airflow]
Posted by "gdavoian (via GitHub)" <gi...@apache.org>.
gdavoian commented on PR #29821:
URL: https://github.com/apache/airflow/pull/29821#issuecomment-1803444413
@potiuk @hussein-awala You're welcome to review https://github.com/apache/airflow/pull/35546 :)
--
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
[GitHub] [airflow] potiuk merged pull request #29821: Add a check for not templateable fields
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk merged PR #29821:
URL: https://github.com/apache/airflow/pull/29821
--
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
[GitHub] [airflow] eladkal commented on a diff in pull request #29821: Add a check for not templateable fields
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on code in PR #29821:
URL: https://github.com/apache/airflow/pull/29821#discussion_r1121242914
##########
airflow/serialization/serialized_objects.py:
##########
@@ -770,8 +772,12 @@ def _serialize_node(cls, op: BaseOperator | MappedOperator, include_deps: bool)
# Store all template_fields as they are if there are JSON Serializable
# If not, store them as strings
+ # And raise an exception if the field is not templateable
+ forbidden_fields = set(inspect.signature(BaseOperator.__init__).parameters.keys())
if op.template_fields:
for template_field in op.template_fields:
+ if template_field in forbidden_fields:
+ raise AirflowException(f"Cannot use {template_field} as template field")
Review Comment:
I think the only case where we can not template fields is when user wants to template BaseOperator parameters which is something that can't be done. So maybe we can be more informative on the exception here?
--
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] Add a check for not templateable fields [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #29821:
URL: https://github.com/apache/airflow/pull/29821#issuecomment-1803386889
> Hm, you were right, looks like only `email` is a field that is 100% safe to template (as I mentioned before, the field is heavily used by our team as `default_args["email"]` for each and every DAG). There is also `run_as_user` that seems to be safe as well, but I've never used it, so can't really comment on its usage. If you're fine with that, then I can open a PR to exclude `email` (and optionally `run_as_user` if you think it makes sense) from that check.
I would say - just `email`. Because security. I have not realized it before, but this PR als had some interesting security implications.
I believe it is better to not allow to modify `run_as_user` because it is a potential security risk. The `run_as_user` is used to do impersonation, so basically run the final task as a specific linux user (via sudo basically). You can easily imagine someone tricking the code of task A to pass `root` there to run task B with `root` priviledges where someone wanted to use `nobody`. And usually impersonation is done because of security concerns actually, so dynamically generating it is quite a bit risky
Of course that would require several layers of other security issues, but generally speaking (similarly to SQL INJECTION kind of vulnerabilities) - if you have a content generated from (potentially) user input, you should always sanitize it and not use it any context that is anywhere near security - bound decisions.
--
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] Add a check for not templateable fields [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #29821:
URL: https://github.com/apache/airflow/pull/29821#issuecomment-1802925162
@gdavoidan - what I proposed and I believe @hussein-awala agrees with is that you define which fields of BaseOperator you exclude from the check. This is what my proposal is about. Find out which of those fields CAN be templated and remove them from the check. Basically allow-list of fields you want to allow templating. Rather than allow all by default. There is no need "specify" them as extra field - just hard-code them.
What @hussein-awala proposes is that you attempt to test it and figure out which fields can be templated by templating then one by one and trying to serialize the DAG. because not all of them CAN be serialized and you need to serialize DAG to DB to execute it. See description ogf the original error https://github.com/apache/airflow/issues/29819 whcih explains the reason for doing it. Try to do what the author of the oriignal issue and see it for yourself when you remoge the protection. You don't need to believe anyone's word just try it and read the issues.
> This sounds like a workaround for a workaround :) For some reason, you've prohibited the rendering of all the BaseOperator fields (I'm still not convinced of the need for this change), and now you're asking me to exclude some of the fields from that rule (the rule I don't even agree and the rule you can't really justify)...
No. You simply didn't dig deep enough. Look at the issue and try to understand what happened there. This is solution to a real problem that overcautiously excluded all the fields. What we are kindly asking you to do is to find out which fields out of those can be templated.
Just looking at your examples neither `pool` nor `doc_md` should be allowed to be tempalted. The`pool` field is evaluated by the scheduler when scheduler scheduler tasks for execution, and then when celery worker picks up the task it only selects the tasks belonging to it's queue. And it happens long before template rendering happens - template rendering happpen in the worker of cellery AFTER the worker already used `queue` field to pick tasks. Similary `doc_md` should not be templated, because it is used by the UI to display task details NOT task instance details - and in order to do do it, cannot render the field, because again - rendering happens at execution time by the task, not when the UI decides to display description of the task. Similarly `execution_timeout` cannot be rendered, because it is serialized to the DB by DAG file processor, WAY BEFORE the task gets executed, and there it is stored as INT not string (which is the original issue that this PR solved). And it mak
es completely no sense to render `execution_timeout` because again, it is used BEFORE the task starts and rendering is happening in the worker - because execution_timout is applied at the monitoring level of the task in teh process that starts the actual "local task process" where rendering happen. So basically (if you look at the code) SIGARM is started and alarm on timeout is set before JINJA rendering happens. You likely would not like to do:
```
signal.setitimer(signal.ITIMER_REAL, '{{ renerd_timer }}')
```
That simply would not work.
So assumption when that PR got merged that most (if not all) of the fields of BaseOperators are like that and all of them should be excluded by default.
You seem to find one that might be excluded. Maybe more can be as well. What we are kindly asking you to review all of those, possibly test (as @hussein-awala proposed) and come with PR where you specifically exclude those fields that CAN be allowlisted and you tested that they work.
Is it too much of an ask?
--
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] Add a check for not templateable fields [airflow]
Posted by "gdavoian (via GitHub)" <gi...@apache.org>.
gdavoian commented on PR #29821:
URL: https://github.com/apache/airflow/pull/29821#issuecomment-1802995760
@potiuk @hussein-awala Thank you for the explanations. I'm starting to understand the problem and what I need to do. I'll check and get back to you soon. My apologies for any earlier misunderstandings.
--
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] Add a check for not templateable fields [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #29821:
URL: https://github.com/apache/airflow/pull/29821#issuecomment-1802998230
> @potiuk @hussein-awala Thank you for the explanations! I'm starting to understand the problem and what I'm supposed to do. I'll check and get back to you soon. My apologies for any earlier misunderstandings!
No worries :). Airflow is pretty ... complicated. ... and even after years of working on it every day I find something new or make wrong assumption. See that one for example https://github.com/apache/airflow/pull/35539#issuecomment-1802996355 just about 2 minutes ago. ...
--
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