[AIRFLOW-4448] Don't bake ENV and _cmd into tmp config for non-sudo #4050

ashb · 2018-10-14T13:40:33Z

Make sure you have checked all steps below.

Jira

https://issues.apache.org/jira/browse/AIRFLOW-4448

Description

Further to [AIRFLOW-3178] Handle percents signs in configs for airflow run #4029,

If we are running tasks via sudo then AIRFLOW__ config env vars won't be
visible anymore (without them showing up in ps) and we likely might
not have permission to run the _cmd's specified to find the passwords.

But if we are running as the same user then there is no need to "bake"
those options in to the temporary config file -- if the operator decided
they didn't want those values appearing in a config file on disk, then
lets do our best to respect that.

Tests

My PR adds the following unit tests: added some tests to tests/configuration.py.

Commits

My commits all reference Jira issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "How to write a good git commit message":
1. Subject is separated from body by a blank line
2. Subject is limited to 50 characters (not including Jira issue reference)
3. Subject does not end with a period
4. Subject uses the imperative mood ("add", not "adding")
5. Body wraps at 72 characters
6. Body explains "what" and "why", not "how"

Documentation

In case of new functionality, my PR adds documentation that describes how to use it.
- When adding new operators/hooks/sensors, the autoclass documentation generation needs to be added.

Code Quality

Passes git diff upstream/master -u -- "*.py" | flake8 --diff

ashb · 2018-10-14T13:42:02Z

I haven't thought enough if this will mess up with the Kubernetes Executor, so someone familair with that (@Fokko? @tedmiston?) should take a look at this.

Fokko · 2018-10-15T05:23:50Z

@ashb I just checked the Kubeternetes tests, and they seem to pass. Why do you think it messes up the Kubeternetes part? cc @dimberman

ashb · 2018-10-16T08:46:48Z

It was more I wasn't sure how the Kubernetes Executor etc worked, and if something was going to mess up as a result of that change it would be that.

XD-DENG

Two minor typos in your comments.

airflow/task/task_runner/base_task_runner.py

ms32035 · 2018-10-24T15:51:50Z

@ashb, has this branch been intentionally pushed to the apache repo, or just a mistake?

ashb · 2018-10-24T15:56:41Z

@ms32035 Yes it was intentional. Do you think there's a reason not to have it here?

ms32035 · 2018-10-24T16:07:23Z

Not really. It was just so different from all other, that it made me wonder

ashb · 2018-10-26T15:33:46Z

@bolkedebruin PTAL

tests/configuration.py

stale · 2018-12-13T14:58:42Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

ashb · 2018-12-15T15:41:34Z

@Fokko updated (and rebased). PTAL.

dimberman · 2019-04-30T17:25:15Z

@ashb just pinging on this since it seems like a pretty good addition :).

ashb · 2019-04-30T19:52:11Z

@dimberman Turns out we forget our own PRs sometimes too 😁. Rebased, hopefully it's good now.

Fixed typos.

codecov-io · 2019-04-30T20:37:49Z

Codecov Report

Merging #4050 into master will increase coverage by 0.04%.
The diff coverage is 80.76%.

@@            Coverage Diff             @@
##           master    #4050      +/-   ##
==========================================
+ Coverage   78.54%   78.59%   +0.04%     
==========================================
  Files         469      469              
  Lines       29981    30077      +96     
==========================================
+ Hits        23550    23639      +89     
- Misses       6431     6438       +7

Impacted Files	Coverage Δ
airflow/utils/configuration.py	`100% <100%> (ø)`	⬆️
airflow/task/task_runner/base_task_runner.py	`73.33% <50%> (-1.25%)`	⬇️
airflow/configuration.py	`91.87% <82.6%> (+0.05%)`	⬆️
airflow/models/taskinstance.py	`92.42% <0%> (-0.18%)`	⬇️
airflow/contrib/operators/bigquery_operator.py	`94.21% <0%> (+0.25%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f926249...9918541. Read the comment docs.

If we are running tasks via sudo then AIRFLOW__ config env vars won't be visible anymore (without them showing up in `ps`) and we likely might not have permission to run the _cmd's specified to find the passwords. But if we are running as the same user then there is no need to "bake" those options in to the temporary config file -- if the operator decided they didn't want those values appearing in a config file on disk, then lets do our best to respect that.

ashb · 2019-04-30T21:40:01Z

@dimberman Update to new jira too since the previous one was part of 1.10.1. PTAL

dimberman · 2019-04-30T22:18:22Z

@ashb LGTM!

…pache#4050) If we are running tasks via sudo then AIRFLOW__ config env vars won't be visible anymore (without them showing up in `ps`) and we likely might not have permission to run the _cmd's specified to find the passwords. But if we are running as the same user then there is no need to "bake" those options in to the temporary config file -- if the operator decided they didn't want those values appearing in a config file on disk, then lets do our best to respect that.

…4050) If we are running tasks via sudo then AIRFLOW__ config env vars won't be visible anymore (without them showing up in `ps`) and we likely might not have permission to run the _cmd's specified to find the passwords. But if we are running as the same user then there is no need to "bake" those options in to the temporary config file -- if the operator decided they didn't want those values appearing in a config file on disk, then lets do our best to respect that. (cherry picked from commit 670f2ed)

…pache#4050) If we are running tasks via sudo then AIRFLOW__ config env vars won't be visible anymore (without them showing up in `ps`) and we likely might not have permission to run the _cmd's specified to find the passwords. But if we are running as the same user then there is no need to "bake" those options in to the temporary config file -- if the operator decided they didn't want those values appearing in a config file on disk, then lets do our best to respect that.

…pache#4050) If we are running tasks via sudo then AIRFLOW__ config env vars won't be visible anymore (without them showing up in `ps`) and we likely might not have permission to run the _cmd's specified to find the passwords. But if we are running as the same user then there is no need to "bake" those options in to the temporary config file -- if the operator decided they didn't want those values appearing in a config file on disk, then lets do our best to respect that. (cherry picked from commit 670f2ed)

When sending configs to Airflow workers we materialize a temp config file. In apache#18772 a feature was added so that `_cmd` generated secrets are not written to the files in some cases instead favoring maintaining the raw `_cmd` settings. Unfortunately during materializing of the configs via `as_dict()` Airflow defaults are generated and materialized as well including defaults for the non `_cmd` versions of some settings. And due to Airflow setting precedence stating bare versions of settings winning over `_cmd` versions it results in `_cmd` settings being discarded: https://airflow.apache.org/docs/apache-airflow/stable/howto/set-config.html This change checks `_cmd`, env, and secrets when materializing configs via `as_dict()` so that if the bare versions of the values is exactly the same as Airflow defaults and we have "hidden" / special versions of these configs that are trying to be set we remove the bare versions so that the correct version can be used. Fixes: apache#20092 Related to: apache#18772 apache#4050

* Filter out default configs when overrides exist. When sending configs to Airflow workers we materialize a temp config file. In #18772 a feature was added so that `_cmd` generated secrets are not written to the files in some cases instead favoring maintaining the raw `_cmd` settings. Unfortunately during materializing of the configs via `as_dict()` Airflow defaults are generated and materialized as well including defaults for the non `_cmd` versions of some settings. And due to Airflow setting precedence stating bare versions of settings winning over `_cmd` versions it results in `_cmd` settings being discarded: https://airflow.apache.org/docs/apache-airflow/stable/howto/set-config.html This change checks `_cmd`, env, and secrets when materializing configs via `as_dict()` so that if the bare versions of the values is exactly the same as Airflow defaults and we have "hidden" / special versions of these configs that are trying to be set we remove the bare versions so that the correct version can be used. Fixes: #20092 Related to: #18772 #4050

* Filter out default configs when overrides exist. When sending configs to Airflow workers we materialize a temp config file. In #18772 a feature was added so that `_cmd` generated secrets are not written to the files in some cases instead favoring maintaining the raw `_cmd` settings. Unfortunately during materializing of the configs via `as_dict()` Airflow defaults are generated and materialized as well including defaults for the non `_cmd` versions of some settings. And due to Airflow setting precedence stating bare versions of settings winning over `_cmd` versions it results in `_cmd` settings being discarded: https://airflow.apache.org/docs/apache-airflow/stable/howto/set-config.html This change checks `_cmd`, env, and secrets when materializing configs via `as_dict()` so that if the bare versions of the values is exactly the same as Airflow defaults and we have "hidden" / special versions of these configs that are trying to be set we remove the bare versions so that the correct version can be used. Fixes: #20092 Related to: #18772 #4050 (cherry picked from commit e07bc63)

XD-DENG previously requested changes Oct 23, 2018

View reviewed changes

airflow/task/task_runner/base_task_runner.py Outdated Show resolved Hide resolved

airflow/task/task_runner/base_task_runner.py Outdated Show resolved Hide resolved

ashb force-pushed the dont-bake-env-into-tmp-config branch from cd59ec5 to 0df13ac Compare October 24, 2018 12:28

ashb force-pushed the dont-bake-env-into-tmp-config branch from 0df13ac to b005425 Compare October 26, 2018 15:33

Fokko reviewed Oct 29, 2018

View reviewed changes

tests/configuration.py Outdated Show resolved Hide resolved

stale bot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Dec 13, 2018

ashb removed the stale Stale PRs per the .github/workflows/stale.yml policy file label Dec 13, 2018

ashb force-pushed the dont-bake-env-into-tmp-config branch from b005425 to be38843 Compare December 15, 2018 15:41

ashb force-pushed the dont-bake-env-into-tmp-config branch from be38843 to c3dc3b4 Compare January 14, 2019 12:43

ashb force-pushed the dont-bake-env-into-tmp-config branch from c3dc3b4 to 17bf3d1 Compare April 30, 2019 19:51

ashb changed the title ~~[AIRFLOW-3178] Don't bake ENV and _cmd into tmp config for non-sudo~~ [AIRFLOW-4448] Don't bake ENV and _cmd into tmp config for non-sudo Apr 30, 2019

ashb force-pushed the dont-bake-env-into-tmp-config branch from 17bf3d1 to 9918541 Compare April 30, 2019 21:39

dimberman merged commit 670f2ed into master May 7, 2019

potiuk deleted the dont-bake-env-into-tmp-config branch May 19, 2019 12:10

ashb mentioned this pull request Oct 6, 2021

Don't bake ENV and _cmd into tmp config for non-sudo #18772

Merged

xyu mentioned this pull request Feb 12, 2022

Filter out default configs when overrides exist. #21539

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[AIRFLOW-4448] Don't bake ENV and _cmd into tmp config for non-sudo #4050

[AIRFLOW-4448] Don't bake ENV and _cmd into tmp config for non-sudo #4050

ashb commented Oct 14, 2018 •

edited

Loading

ashb commented Oct 14, 2018

Fokko commented Oct 15, 2018

ashb commented Oct 16, 2018

XD-DENG left a comment

ms32035 commented Oct 24, 2018

ashb commented Oct 24, 2018

ms32035 commented Oct 24, 2018

ashb commented Oct 26, 2018

stale bot commented Dec 13, 2018

ashb commented Dec 15, 2018

dimberman commented Apr 30, 2019

ashb commented Apr 30, 2019

codecov-io commented Apr 30, 2019 •

edited

Loading

ashb commented Apr 30, 2019

dimberman commented Apr 30, 2019

[AIRFLOW-4448] Don't bake ENV and _cmd into tmp config for non-sudo #4050

[AIRFLOW-4448] Don't bake ENV and _cmd into tmp config for non-sudo #4050

Conversation

ashb commented Oct 14, 2018 • edited Loading

Jira

Description

Tests

Commits

Documentation

Code Quality

ashb commented Oct 14, 2018

Fokko commented Oct 15, 2018

ashb commented Oct 16, 2018

XD-DENG left a comment

Choose a reason for hiding this comment

ms32035 commented Oct 24, 2018

ashb commented Oct 24, 2018

ms32035 commented Oct 24, 2018

ashb commented Oct 26, 2018

stale bot commented Dec 13, 2018

ashb commented Dec 15, 2018

dimberman commented Apr 30, 2019

ashb commented Apr 30, 2019

codecov-io commented Apr 30, 2019 • edited Loading

Codecov Report

ashb commented Apr 30, 2019

dimberman commented Apr 30, 2019

ashb commented Oct 14, 2018 •

edited

Loading

codecov-io commented Apr 30, 2019 •

edited

Loading