Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Get Airflow Variables from Hashicorp Vault #7944

Merged
merged 1 commit into from
Mar 28, 2020

Conversation

kaxil
Copy link
Member

@kaxil kaxil commented Mar 28, 2020

Depends on #7948

Get Airflow Variables from Hashicorp Vault

Please only check the last commit

Make sure to mark the boxes below before creating PR: [x]


In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.
Read the Pull Request Guidelines for more information.

@codecov-io
Copy link

codecov-io commented Mar 28, 2020

Codecov Report

Merging #7944 into master will decrease coverage by 25.28%.
The diff coverage is 64.70%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #7944       +/-   ##
===========================================
- Coverage   86.36%   61.07%   -25.29%     
===========================================
  Files         929      931        +2     
  Lines       45036    45172      +136     
===========================================
- Hits        38895    27590    -11305     
- Misses       6141    17582    +11441     
Impacted Files Coverage Δ
airflow/executors/kubernetes_executor.py 56.87% <0.00%> (ø)
airflow/models/dag.py 91.08% <ø> (+0.08%) ⬆️
...ow/providers/amazon/aws/secrets/systems_manager.py 56.52% <0.00%> (-43.48%) ⬇️
.../providers/google/cloud/secrets/secrets_manager.py 43.18% <0.00%> (-56.82%) ⬇️
airflow/utils/log/logging_mixin.py 95.38% <ø> (ø)
...oviders/amazon/aws/operators/sagemaker_endpoint.py 19.64% <12.50%> (-68.12%) ⬇️
airflow/providers/hashicorp/secrets/vault.py 22.72% <20.00%> (-53.55%) ⬇️
airflow/providers/google/suite/hooks/sheets.py 26.08% <21.42%> (-73.92%) ⬇️
airflow/providers/google/suite/operators/sheets.py 55.22% <55.22%> (ø)
airflow/secrets/base_secrets.py 89.47% <80.00%> (-4.65%) ⬇️
... and 327 more

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 eb4af4f...277e7fc. Read the comment docs.

@kaxil
Copy link
Member Author

kaxil commented Mar 28, 2020

Wrong PR for this change.

Separated it to a different PR #7948

@kaxil kaxil changed the title [Depends on #7923] Get Airflow Variables from Hashicorp Vault [Depends on #7948] Get Airflow Variables from Hashicorp Vault Mar 28, 2020
@kaxil kaxil changed the title [Depends on #7948] Get Airflow Variables from Hashicorp Vault Get Airflow Variables from Hashicorp Vault Mar 28, 2020
@kaxil kaxil merged commit c1c88ab into apache:master Mar 28, 2020
@kaxil kaxil deleted the add-variables-vault branch March 28, 2020 13:35
@kaxil
Copy link
Member Author

kaxil commented Mar 28, 2020

Ci failure is unrelated

@kaxil kaxil added this to the Airflow 1.10.10 milestone Mar 28, 2020
kaxil added a commit that referenced this pull request Mar 28, 2020
kaxil added a commit to astronomer/airflow that referenced this pull request Mar 30, 2020
kaxil added a commit that referenced this pull request Mar 30, 2020
kaxil added a commit that referenced this pull request Apr 1, 2020
@kaxil kaxil added the kind:feature Feature Requests label Apr 1, 2020
kaxil added a commit to astronomer/airflow that referenced this pull request Apr 28, 2020
(cherry picked from commit c1c88ab)
(cherry picked from commit 9f481d3)
kaxil added a commit to astronomer/airflow that referenced this pull request May 4, 2020
(cherry picked from commit c1c88ab)
(cherry picked from commit 9f481d3)
:return: Variable Value
"""
response = self._get_secret(self.variables_path, key)
return response.get("value") if response else None
Copy link
Contributor

@jacobhjkim jacobhjkim Jul 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kaxil Why get response.get("value") instead of response[key]?
Specifically, why

variables
|- foo
|    |- value = some_value
└ bar
     └ value = another_value

Not

variables
|- foo = some_value
└ bar = another_value

This makes the structure simple.

Copy link
Member Author

@kaxil kaxil Jul 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @jacobhjkim it was mainly because we wanted to store each Variable as a separate secret (to have proper access control / avoid limits on a secret) and the fact that we use KV store, so we needed a key

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, thanks for the answer 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:secrets kind:feature Feature Requests provider:amazon-aws AWS/Amazon - related issues provider:google Google (including GCP) related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants