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.

Sign up for GitHub

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

Jump to bottom

Standardize SecretBackend class names #7846

Merged
merged 3 commits into from
Mar 25, 2020
Merged

Standardize SecretBackend class names #7846

merged 3 commits into from
Mar 25, 2020

Conversation

kaxil
Copy link
Member

@kaxil kaxil commented Mar 24, 2020

  • AwsSsmSecretsBackend -> AwsSsmBackend
  • CloudSecretsManagerSecretsBackend -> CloudSecretsManagerBackend
  • VaultSecrets -> VaultBackend
  • EnvironmentVariablesSecretsBackend -> EnvironmentVariablesBackend
  • MetastoreSecretsBackend -> MetastoreBackend

Issue link: WILL BE INSERTED BY boring-cyborg

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.

cc @xinbinhuang

@kaxil
Standardize SecretBackend class names
bee8d92 
- AwsSsmSecretsBackend -> AwsSsmBackend
- CloudSecretsManagerSecretsBackend -> CloudSecretsManagerBackend
- VaultSecrets -> VaultBackend
- EnvironmentVariablesSecretsBackend -> EnvironmentVariablesBackend
- MetastoreSecretsBackend -> MetastoreBackend
@kaxil kaxil requested a review from potiuk March 24, 2020 00:51
@boring-cyborg boring-cyborg bot added area:docs area:secrets provider:amazon-aws AWS/Amazon - related issues provider:google Google (including GCP) related issues labels Mar 24, 2020
@xinbinhuang
Copy link
Contributor

Nice! Like the name standardization. I am also thinking if we should standardize the connection_prefix (used in AWS SSM & GCP Secrets Manager) and connection_path ( in Hashicorp Vault)

@kaxil
Copy link
Member Author

kaxil commented Mar 24, 2020

Nice! Like the name standardization. I am also thinking if we should standardize the connection_prefix (used in AWS SSM & GCP Secrets Manager) and connection_path ( in Hashicorp Vault)

What name would you suggest ?

@kaxil
Copy link
Member Author

kaxil commented Mar 24, 2020

Options:

  1. connections_path_prefix
  2. connections_prefix
  3. connections_path

What do others think @potiuk @ashb

@ashb
Copy link
Member

ashb commented Mar 24, 2020

Aws SSM is a suite of tools/products so calling it AwsSSMBackend is too generic. AwsSSMParameterStoreBackend (though it's too long a name)

@kaxil
Copy link
Member Author

kaxil commented Mar 24, 2020 •
edited
Loading

Aws SSM

How about AwsSecretsManager? (Based on https://docs.aws.amazon.com/secretsmanager/latest/userguide/intro.html)

@ashb
Copy link
Member

ashb commented Mar 24, 2020

In fact it seems that "SSM" is the "Systems Manager Agent" https://docs.aws.amazon.com/systems-manager/latest/userguide/ssm-agent.html.

The full product name of the feature we are using is the AWS Systems Manager Parameter Store.

@ashb
Copy link
Member

ashb commented Mar 24, 2020

I think SecretsManager may be something else. Go AWS!

@kaxil
Copy link
Member Author

kaxil commented Mar 24, 2020

I think SecretsManager may be something else. Go AWS!

🤦‍♂ I am confused. WTH!

@kaxil
Copy link
Member Author

kaxil commented Mar 24, 2020 •
edited
Loading

AwsSystemsManagerParameterStoreBackend wow that name would be huge

SystemsManagerParameterStoreBackend ? We can remove aws as it is in module_path and providers path too.

@ashb
Copy link
Member

ashb commented Mar 24, 2020

https://docs.aws.amazon.com/systems-manager/latest/userguide/what-is-systems-manager.html

Systems Manager Service Name History

AWS Systems Manager (Systems Manager) was formerly known as " Amazon Simple Systems Manager (SSM) " and " Amazon EC2 Systems Manager (SSM) ". The original abbreviated name of the service, " SSM ", is still reflected in various AWS resources, including a few other service consoles. Some examples:

   Systems Manager Agent: SSM Agent

   Systems Manager parameters: SSM parameters

@ashb
Copy link
Member

ashb commented Mar 24, 2020

I think the "Store" part is not needed, so AwsSMParametersBackend, AwsSystemManagerParametersBackend or soemthing like that, or perhaps AwsSSMParametersBackend.

@kaxil
Copy link
Member Author

kaxil commented Mar 24, 2020 •
edited
Loading

SystemsManagerParametersBackend stripping AWS as we don't use GCP or Hashicorp in their respective backends too.

@xinbinhuang
Copy link
Contributor

For some context, this is a snapshot of the parameter store service. So I feel like better to still have the ParameterStore part

image

@kaxil
Copy link
Member Author

kaxil commented Mar 24, 2020

For some context, this is a snapshot of the parameter store service. So I feel like better to still have the ParameterStore part

image

Done

@kaxil kaxil requested a review from ashb March 24, 2020 23:29
@xinbinhuang
Copy link
Contributor

xinbinhuang commented Mar 25, 2020 •
edited
Loading

Options:

  1. connections_path_prefix
  2. connections_prefix
  3. connections_path

What do others think @potiuk @ashb

Add a comment here to keep a record on what we discussed on Slack:

  • @kaxil and I agree on using connections_prefix because it is more generic than path and may apply to different systems better (GCP Secrets Manager does not support /, so airflow/connections is invalid.)

@kaxil
Copy link
Member Author

kaxil commented Mar 25, 2020

Options:

  1. connections_path_prefix
  2. connections_prefix
  3. connections_path

What do others think @potiuk @ashb

Add a comment here to keep a record on what we discussed on Slack:

  • @kaxil and I agree on using connections_prefix because it is more generic than path and may apply to different systems better (GCP Secrets Manager does not support /, so airflow/connections is invalid.)

Hey, Let's keep the args as it is. After reading some more docs on Hashicorp Vault and talking to Ash, I am going to not change the args for now.

turbaszek
turbaszek approved these changes Mar 25, 2020
View reviewed changes
Copy link
Member

@turbaszek turbaszek left a comment

Choose a reason for hiding this comment

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

LGTM

@kaxil kaxil merged commit 686d7d5 into apache:master Mar 25, 2020
@kaxil kaxil deleted the standardize-backend-names branch March 25, 2020 13:35
kaxil added a commit that referenced this pull request Mar 25, 2020
@kaxil
Standardize SecretBackend class names (#7846)
3b72866 
- AwsSsmSecretsBackend -> SystemsManagerParameterStoreBackend
- CloudSecretsManagerSecretsBackend -> CloudSecretsManagerBackend
- VaultSecrets -> VaultBackend
- EnvironmentVariablesSecretsBackend -> EnvironmentVariablesBackend
- MetastoreSecretsBackend -> MetastoreBackend

(cherry picked from commit 686d7d5)
@kaxil kaxil added this to the Airflow 1.10.10 milestone Mar 26, 2020
kaxil added a commit to astronomer/airflow that referenced this pull request Mar 30, 2020
@kaxil
Standardize SecretBackend class names (apache#7846)
c29e605 
- AwsSsmSecretsBackend -> SystemsManagerParameterStoreBackend
- CloudSecretsManagerSecretsBackend -> CloudSecretsManagerBackend
- VaultSecrets -> VaultBackend
- EnvironmentVariablesSecretsBackend -> EnvironmentVariablesBackend
- MetastoreSecretsBackend -> MetastoreBackend

(cherry picked from commit 686d7d5)
kaxil added a commit that referenced this pull request Mar 30, 2020
@kaxil
Standardize SecretBackend class names (#7846)
7a4ec16 
- AwsSsmSecretsBackend -> SystemsManagerParameterStoreBackend
- CloudSecretsManagerSecretsBackend -> CloudSecretsManagerBackend
- VaultSecrets -> VaultBackend
- EnvironmentVariablesSecretsBackend -> EnvironmentVariablesBackend
- MetastoreSecretsBackend -> MetastoreBackend

(cherry picked from commit 686d7d5)
kaxil added a commit to astronomer/airflow that referenced this pull request Apr 28, 2020
@kaxil
Standardize SecretBackend class names (apache#7846)
008cb6e 
- AwsSsmSecretsBackend -> SystemsManagerParameterStoreBackend
- CloudSecretsManagerSecretsBackend -> CloudSecretsManagerBackend
- VaultSecrets -> VaultBackend
- EnvironmentVariablesSecretsBackend -> EnvironmentVariablesBackend
- MetastoreSecretsBackend -> MetastoreBackend

(cherry picked from commit 686d7d5)
(cherry picked from commit 7a4ec16)
@kaxil kaxil mentioned this pull request Apr 29, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@turbaszek turbaszek turbaszek approved these changes

@potiuk potiuk Awaiting requested review from potiuk

@ashb ashb Awaiting requested review from ashb

Assignees
No one assigned
Labels
area:secrets provider:amazon-aws AWS/Amazon - related issues provider:google Google (including GCP) related issues
Projects
None yet
Milestone
Airflow 1.10.10
Development

Successfully merging this pull request may close these issues.
4 participants
@kaxil @xinbinhuang @ashb @turbaszek