You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2021/07/23 07:55:16 UTC
[GitHub] [airflow] potiuk opened a new issue #17186: Smarter template_ext processing
potiuk opened a new issue #17186:
URL: https://github.com/apache/airflow/issues/17186
The "template_ext" mechanism is useful for automatically loading and jinja-processing files which are specified in parameters of Operators. However this might lead to certain problems for example (from slack conversation):
```
The templated_fields in KubernetesPodOperator seems cause the error airflow jinja2.exceptions.TemplateNotFound when some character in the column, e.g / in env_vars.
The code got error.
env_vars=[
k8s.V1EnvVar(
name="GOOGLE_APPLICATION_CREDENTIALS",
value="/var/secrets/google/service-account.json",
),
```
I believe the behaviour changed comparing to not-so-long-past. Some of the changes with processing parameters with jinja recursively caused this template behaviour to be applied also nested parameters like above.
There are two ways we could improve the situation:
1) limit the "template_extension" resolution to only direct string kwargs passed to the operator (I think this is no brainer and we should do it)
2) propose some "escaping" mechanism, where you could either disable template_extension processing entirely or somehow mark the parameters that should not be treated as templates.
Here I have several proposals:
a) we could add "skip_template_ext_processing" or similar parameter in BaseOperator <- I do not like it as many operators rely on this behaviour for a good reason
b) we could add "template_ext" parameter in the operator that could override the original class-level-field <- I like this one a lot
c) we could add "disable_template_ext_pattern" (str) parameter where we could specify list of regexp's where we could filter out only specific values <- this one will allow to disable template_ext much more "selectively" - only for certain parameters.
--
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 issue #17186: Smarter template_ext processing
Posted by GitBox <gi...@apache.org>.
potiuk commented on issue #17186:
URL: https://github.com/apache/airflow/issues/17186#issuecomment-888244788
But we have 'env_vars' in K8SPodOperator: https://github.com/apache/airflow/blob/4dc622d8ca4b14a2bb3b8c5e68bde04cc80c5379/airflow/providers/cncf/kubernetes/operators/kubernetes_pod.py#L169 So I think this behaviour is pretty expected with the current state of "recursive" behaviour.
--
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] kaxil commented on issue #17186: Smarter template_ext processing
Posted by GitBox <gi...@apache.org>.
kaxil commented on issue #17186:
URL: https://github.com/apache/airflow/issues/17186#issuecomment-888264662
I think you are confusing 2 things:
1) **Recursive classes**
https://github.com/apache/airflow/blob/4dc622d8ca4b14a2bb3b8c5e68bde04cc80c5379/tests/models/test_baseoperator.py#L141-L180
2) **Iterable** (list, dict, tuple, etc)
https://github.com/apache/airflow/blob/4dc622d8ca4b14a2bb3b8c5e68bde04cc80c5379/tests/models/test_baseoperator.py#L121-L123
Logic - Flow of code:
https://github.com/apache/airflow/blob/4dc622d8ca4b14a2bb3b8c5e68bde04cc80c5379/airflow/models/baseoperator.py#L1047-L1078
For nested it should do:
https://github.com/apache/airflow/blob/4dc622d8ca4b14a2bb3b8c5e68bde04cc80c5379/airflow/models/baseoperator.py#L1080-L1091
Since env_vars is a list, the items inside that list will be rendered if it is a string but since it is `k8s.V1EnvVar` class that does not have `template_field` attribute it should not.
The only reason it is doing that is we override that method for KPO:
https://github.com/apache/airflow/blob/4dc622d8ca4b14a2bb3b8c5e68bde04cc80c5379/airflow/providers/cncf/kubernetes/operators/kubernetes_pod.py#L284-L302
So the issue / bug is because of a fix in https://github.com/apache/airflow/pull/14123 - and it does not affect all other operators.
--
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] uranusjr commented on issue #17186: Smarter template_ext processing
Posted by GitBox <gi...@apache.org>.
uranusjr commented on issue #17186:
URL: https://github.com/apache/airflow/issues/17186#issuecomment-885533838
Oh, of course we shouldn't use `typing.Literal`. I was just borrowing the name and should've clarified. We should implement something like `from airflow import Literal`.
--
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] uranusjr commented on issue #17186: Smarter template_ext processing
Posted by GitBox <gi...@apache.org>.
uranusjr commented on issue #17186:
URL: https://github.com/apache/airflow/issues/17186#issuecomment-885521935
In my ideal world, passing a path as an argument shouldn't need special escaping, and we should require something like
```python
BashOperator(bash_command=Tempalte('/path/to/my-script.sh'))
```
to trigger templating.
More realistic though, something like
```python
MyOperator(path=Literal('/path/to/my-file.json'))
```
should work?
--
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 issue #17186: Smarter template_ext processing
Posted by GitBox <gi...@apache.org>.
potiuk commented on issue #17186:
URL: https://github.com/apache/airflow/issues/17186#issuecomment-885531254
The problem is that we already have operators that heavily rely on this behaviour and will replace ANY string ending with the designated extensions with the jinja-processed value of the files they are pointing to.
This is convenient, though yeah - super-ambiguous in a number of cases, and I think we need a tactical solution for now to handle confusion it creates. I think the "recursive" behaviour is relatively new and unintended behaviour and we could address it in patchlevel release classifying it as a "bug" rather than breaking feature.
Then if we do above, I think we might even not need the "Escaping" but straight away implementing some "explicit" way of marking the template files - a variant of what you proposed @uranusjr and deprecate the usage of implicit extension handling with warning, targeting its removal for Airflow 3.
BTW - I thought Literal (https://www.python.org/dev/peps/pep-0586/) is a "type-checking" feature only, not actual class you can instantiate? Am I wrong :D ?
--
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] raphaelauv edited a comment on issue #17186: Fix template_ext processing for Kubernetes Pod Operator
Posted by GitBox <gi...@apache.org>.
raphaelauv edited a comment on issue #17186:
URL: https://github.com/apache/airflow/issues/17186#issuecomment-929992305
For people confused by the [release 2.0.2 ](https://airflow.apache.org/docs/apache-airflow-providers-cncf-kubernetes/stable/index.html)
the commit `Fix using XCom with ''KubernetesPodOperator'' (#17760)` mostly fix the error of the templating of the K8S secrets
so don't use 2.0.1 !
--
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] raphaelauv edited a comment on issue #17186: Fix template_ext processing for Kubernetes Pod Operator
Posted by GitBox <gi...@apache.org>.
raphaelauv edited a comment on issue #17186:
URL: https://github.com/apache/airflow/issues/17186#issuecomment-929992305
For people confused by the [release 2.0.2 ](https://airflow.apache.org/docs/apache-airflow-providers-cncf-kubernetes/stable/index.html)
the commit `Fix using XCom with ''KubernetesPodOperator'' (#17760)` mostly fixing the error of the templating of the K8S secrets
so don't use 2.0.1 !
--
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] kaxil commented on issue #17186: Smarter template_ext processing
Posted by GitBox <gi...@apache.org>.
kaxil commented on issue #17186:
URL: https://github.com/apache/airflow/issues/17186#issuecomment-888184665
Airflow shouldn't try to template the following. It's a bug -- for recursive we look for `template_fields` attribute (https://github.com/apache/airflow/pull/4743/files#diff-b5ec97b8301ab688b7e045426f24d485). `k8s.V1EnvVar` should have that attribute so it should not try to render template. Need to look at it in more detail 👀 .
```python.
env_vars=[
k8s.V1EnvVar(
name="GOOGLE_APPLICATION_CREDENTIALS",
value="/var/secrets/google/service-account.json",
),
```
--
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] kaxil closed issue #17186: Fix template_ext processing for Kubernetes Pod Operator
Posted by GitBox <gi...@apache.org>.
kaxil closed issue #17186:
URL: https://github.com/apache/airflow/issues/17186
--
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] kaxil edited a comment on issue #17186: Smarter template_ext processing
Posted by GitBox <gi...@apache.org>.
kaxil edited a comment on issue #17186:
URL: https://github.com/apache/airflow/issues/17186#issuecomment-888264662
I think you are confusing 2 things:
1) **Recursive classes**
https://github.com/apache/airflow/blob/4dc622d8ca4b14a2bb3b8c5e68bde04cc80c5379/tests/models/test_baseoperator.py#L141-L180
2) **Iterable** (list, dict, tuple, etc)
https://github.com/apache/airflow/blob/4dc622d8ca4b14a2bb3b8c5e68bde04cc80c5379/tests/models/test_baseoperator.py#L121-L123
Logic - Flow of code:
https://github.com/apache/airflow/blob/4dc622d8ca4b14a2bb3b8c5e68bde04cc80c5379/airflow/models/baseoperator.py#L1047-L1078
For nested it should do:
https://github.com/apache/airflow/blob/4dc622d8ca4b14a2bb3b8c5e68bde04cc80c5379/airflow/models/baseoperator.py#L1080-L1091
Since env_vars is a list, the items inside that list will be rendered if it is a string but since it is `k8s.V1EnvVar` class that does not have `template_field` attribute it should not.
The only reason it is doing that is we override that method for KPO:
https://github.com/apache/airflow/blob/4dc622d8ca4b14a2bb3b8c5e68bde04cc80c5379/airflow/providers/cncf/kubernetes/operators/kubernetes_pod.py#L284-L302
So the issue / bug is because of a fix in https://github.com/apache/airflow/pull/14123 - and it does not affect any other operators.
--
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] raphaelauv commented on issue #17186: Fix template_ext processing for Kubernetes Pod Operator
Posted by GitBox <gi...@apache.org>.
raphaelauv commented on issue #17186:
URL: https://github.com/apache/airflow/issues/17186#issuecomment-929992305
For people confused by the release 2.0.2
the commit `Fix using XCom with ''KubernetesPodOperator'' (#17760)` mostly fixing the error of the templating of the K8S secrets
so don't use 2.0.1 !
--
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 issue #17186: Fix template_ext processing for Kubernetes Pod Operator
Posted by GitBox <gi...@apache.org>.
potiuk commented on issue #17186:
URL: https://github.com/apache/airflow/issues/17186#issuecomment-891973397
I have changed the name to correctly reflect the discussion results.
--
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 edited a comment on issue #17186: Smarter template_ext processing
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on issue #17186:
URL: https://github.com/apache/airflow/issues/17186#issuecomment-888244788
--
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] raphaelauv commented on issue #17186: Fix template_ext processing for Kubernetes Pod Operator
Posted by GitBox <gi...@apache.org>.
raphaelauv commented on issue #17186:
URL: https://github.com/apache/airflow/issues/17186#issuecomment-929992305
For people confused by the release 2.0.2
the commit `Fix using XCom with ''KubernetesPodOperator'' (#17760)` mostly fixing the error of the templating of the K8S secrets
so don't use 2.0.1 !
--
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 issue #17186: Smarter template_ext processing
Posted by GitBox <gi...@apache.org>.
potiuk commented on issue #17186:
URL: https://github.com/apache/airflow/issues/17186#issuecomment-888289531
Ah ok. I see now :)
--
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] raphaelauv edited a comment on issue #17186: Fix template_ext processing for Kubernetes Pod Operator
Posted by GitBox <gi...@apache.org>.
raphaelauv edited a comment on issue #17186:
URL: https://github.com/apache/airflow/issues/17186#issuecomment-929992305
--
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