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/09/17 20:31:45 UTC
[GitHub] [airflow] oliverisaac opened a new pull request #18336: Configuration lookup in environment wil ignore empty env var
oliverisaac opened a new pull request #18336:
URL: https://github.com/apache/airflow/pull/18336
<!--
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 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/
-->
If environment variables are empty strings, but still set, then airflow will not try other environment variables (such as the `_CMD` env vars). This patch fixes that.
---
**^ 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 change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+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 [UPDATING.md](https://github.com/apache/airflow/blob/main/UPDATING.md).
--
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] mik-laj commented on pull request #18336: Configuration lookup in environment wil ignore empty env var
Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #18336:
URL: https://github.com/apache/airflow/pull/18336#issuecomment-922119250
https://github.com/apache/airflow/issues/16684 is it related?
--
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] mik-laj commented on pull request #18336: Configuration lookup in environment wil ignore empty env var
Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #18336:
URL: https://github.com/apache/airflow/pull/18336#issuecomment-922121681
@oliverisaac Fantastic. This means you have experience with Hashicorp Vault. Would you like to improve support for this tool in Helm Chart? I am happy to help with the review.
--
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 #18336: Configuration lookup in environment wil ignore empty env var
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #18336:
URL: https://github.com/apache/airflow/pull/18336#issuecomment-922129873
I think relying on empty value is a hack.
Better solution would be to disable setting the values in helm chart .
Something like that should do (writing without checking so might need some tweaking):
values.yaml:
```
data:
# If secret names are provided, use those secrets
useMetadataSecret: True
```
_helpers.yaml:
```
{{- if .Values.data.useMetadataSecret }}
- name: AIRFLOW__CORE__SQL_ALCHEMY_CONN
valueFrom:
secretKeyRef:
name: {{ template "airflow_metadata_secret" . }}
key: connection
- name: AIRFLOW_CONN_AIRFLOW_DB
valueFrom:
secretKeyRef:
name: {{ template "airflow_metadata_secret" . }}
key: connection
{{- end }}
```
--
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 #18336: Configuration lookup in environment wil ignore empty env var
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #18336:
URL: https://github.com/apache/airflow/pull/18336#issuecomment-922130253
cc: @jedcunningham ^^
--
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] oliverisaac commented on pull request #18336: Configuration lookup in environment wil ignore empty env var
Posted by GitBox <gi...@apache.org>.
oliverisaac commented on pull request #18336:
URL: https://github.com/apache/airflow/pull/18336#issuecomment-922120480
Yeah, we are doing [exactly this vault-injector patter](https://github.com/apache/airflow/issues/16684#issuecomment-870981955)
--
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] oliverisaac closed pull request #18336: Configuration lookup in environment wil ignore empty env var
Posted by GitBox <gi...@apache.org>.
oliverisaac closed pull request #18336:
URL: https://github.com/apache/airflow/pull/18336
--
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 #18336: Configuration lookup in environment wil ignore empty env var
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #18336:
URL: https://github.com/apache/airflow/pull/18336#issuecomment-922112766
Agree with @mik-laj - in POSIX world. there is a difference between "missing" and "empty" variable and unless there is a good reason why we should treat "empty" as "missing", I do not see why we should change it.
Why do you think we need that @oliverisaac ?
--
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] oliverisaac commented on pull request #18336: Configuration lookup in environment wil ignore empty env var
Posted by GitBox <gi...@apache.org>.
oliverisaac commented on pull request #18336:
URL: https://github.com/apache/airflow/pull/18336#issuecomment-922990542
> Currently, when you pass an empty string to the AIRFLOW__CORE__FERNET_KEY environment variable, secret encryption is turned off.
Ah, yeah, that's an excellent example of why this is not going to work...
> This means you have experience with Hashicorp Vault. Would you like to improve support for this tool in Helm Chart?
I don't think there is much Vault-specific value that needs to be brought. The specifics of how Vault is implemented is going to vary wildly from org to org. The biggest/only change that needs to be made is not defining the `AIRFLOW__CORE__FERNET_KEY` and `AIRFLOW__CORE__SQL_ALCHEMY_CONN` env vars when they shouldn't be defined (either a flag to turn them off, or logic to assess if they are needed or not.)
For future people who land here from a Google search, I ended up making the airflow chart a subchart of a custom chart and defining a custom `_helpers.tpl` file in my parent chart. This allows the parent chart to override the `standard_airflow_environment` template block and is where I removed the env vars.
```
$ tree .
.
├── Chart.yaml
├── charts
│ └── airflow-1.1.0.tgz
├── templates
│ └── _helpers_overrides.yaml
└── values.yaml
```
In your values file, set all the values to be under the top-level key of `airflow`.
In `Chart.yaml` make sure you add the dependencies:
```
dependencies:
- name: airflow
repository: https://airflow.apache.org
version: 1.1.0
```
--
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] mik-laj commented on pull request #18336: Configuration lookup in environment wil ignore empty env var
Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #18336:
URL: https://github.com/apache/airflow/pull/18336#issuecomment-922333195
There is another problem with that. Currently, when you pass an empty string to the `AIRFLOW__CORE__FERNET_KEY` environment variable, secret encryption is turned off. This is commonly used by docker-compose, which provides strict network isolation and is designed to be run on a single node, so fernet is only a hindrance to deployment and offers no benefit.
https://github.com/apache/airflow/blob/a54f8d6b0ce9fd2a2387ccbc11f543d86212ca35/docs/apache-airflow/start/docker-compose.yaml#L58
https://github.com/apache/airflow/blob/main/UPDATING.md#fernet-is-enabled-by-default
https://github.com/apache/airflow/blob/a54f8d6b0ce9fd2a2387ccbc11f543d86212ca35/airflow/models/crypto.py#L80-L83
When the user does not specify a key, a random key is generated.
https://github.com/apache/airflow/blob/a54f8d6b0ce9fd2a2387ccbc11f543d86212ca35/airflow/configuration.py#L847
--
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] oliverisaac commented on pull request #18336: Configuration lookup in environment wil ignore empty env var
Posted by GitBox <gi...@apache.org>.
oliverisaac commented on pull request #18336:
URL: https://github.com/apache/airflow/pull/18336#issuecomment-922118076
The root of the problem is that the helm chart does not work if you do not use the built in database connection config: the chart always defines the environment var `AIRFLOW__CORE__SQL_ALCHEMY_CONN` and thus one cannot override that environment var with any other config as the configuration precedence dictates that `AIRFLOW__CORE__SQL_ALCHEMY_CONN` is higher precedence tahn `AIRFLOW__CORE__SQL_ALCHEMY_CONN_CMD` or `AIRFLOW__CORE__SQL_ALCHEMY_CONN_SECRET` (let alone the config files.)
As I was looking to solve that problem, the behavior of blindly using an empty string from an env var seemed like the wrong default to set. Especially when empty strings are not a valid configuration for `AIRFLOW__CORE__SQL_ALCHEMY_CONN`.
Another approach could be saving the first env var that is set and, if none of the others are set, then we return the first one we found. Otherwise, we return the first non-empty env var.
--
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] oliverisaac edited a comment on pull request #18336: Configuration lookup in environment wil ignore empty env var
Posted by GitBox <gi...@apache.org>.
oliverisaac edited a comment on pull request #18336:
URL: https://github.com/apache/airflow/pull/18336#issuecomment-922120480
Yeah, we are doing [exactly this vault-injector pattern.](https://github.com/apache/airflow/issues/16684#issuecomment-870981955)
--
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] boring-cyborg[bot] commented on pull request #18336: Configuration lookup in environment wil ignore empty env var
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on pull request #18336:
URL: https://github.com/apache/airflow/pull/18336#issuecomment-922064455
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 Contribution Guide (https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst)
Here are some useful points:
- Pay attention to the quality of your code (flake8, mypy and type annotations). Our [pre-commits]( https://github.com/apache/airflow/blob/main/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/docs/apache-airflow/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/BREEZE.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.rst#coding-style-and-best-practices).
Apache Airflow is a community-driven project and together we are making it better 🚀.
In case of doubts contact the developers at:
Mailing List: dev@airflow.apache.org
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on pull request #18336: Configuration lookup in environment wil ignore empty env var
Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #18336:
URL: https://github.com/apache/airflow/pull/18336#issuecomment-922092233
@oliverisaac The current behavior sounds to me as expected. Why should we ignore user-set variables? Even if they are empty, the user had to set them explicitly.
--
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