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

Abstract Secret Management Support #703

Open
wants to merge 24 commits into
base: main
Choose a base branch
from

Conversation

xiaoyongzhu
Copy link
Member

@xiaoyongzhu xiaoyongzhu commented Sep 25, 2022

Description

This PR builds an abstraction for the secret manager layer so Feathr can support more secret management services including Azure Key Vault. This PR also adds support for AWS Secret Manager so folks can get credentials from there.

How was this PR tested?

Added two tests:
test_feathr_get_secrets_from_aws_secret_manager and test_feathr_get_secrets_from_azure_key_vault.

Note that test_feathr_get_secrets_from_aws_secret_manager will fail, claiming that the credentials cannot be found. This is actually expected as the CI workflow for AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY has not been checked in. After merged to main branch, this test will pass (I also tested it locally to make sure it's working).

Does this PR introduce any user-facing changes?

  • No. You can skip the rest of this section.
  • Yes. Make sure to clarify your proposed changes.

This PR also adds a parameter in FeathrClient so users can pass the secret manager client that they want to use to retrieve credentials.

@xiaoyongzhu xiaoyongzhu added the safe to test Tag to execute build pipeline for a PR from forked repo label Sep 26, 2022
@xiaoyongzhu xiaoyongzhu changed the title Add more secret manager support Abstract Secret Management Support Sep 29, 2022
@xiaoyongzhu xiaoyongzhu marked this pull request as ready for review September 29, 2022 15:19
import botocore.session
from aws_secretsmanager_caching import SecretCache, SecretCacheConfig

client = botocore.session.get_session().create_client(

Choose a reason for hiding this comment

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

As an alternative, we can include in the documentation, that IRSA based authentication is possible as well if the users are running feathr in k8s. More documentation here: https://docs.aws.amazon.com/eks/latest/userguide/iam-roles-for-service-accounts.html
I don't believe any code changes are required for this auth to be supported.

secret_client=secret_manager_client)
elif secret_manager_client and self.get_environment_variable_with_default('secrets', 'aws_secrets_manager', 'secret_id'):
self.secret_manager_client = AWSSecretManagerClient(
secret_namespace=self.get_environment_variable_with_default('secrets', 'aws_secrets_manager', 'secret_id'),

Choose a reason for hiding this comment

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

Should this introduce an else and error out in the case if the secrets manager client is not supported by feathr?

Copy link
Member Author

Choose a reason for hiding this comment

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

good catch. I'll update

try:
return self.akv_client.get_feathr_akv_secret(env_keyword)
return self.secret_manager_client.get_feathr_secret(env_keyword)
Copy link
Collaborator

Choose a reason for hiding this comment

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

should this check be case insensitive (upper case and lower case)?

try:
return self.akv_client.get_feathr_akv_secret(variable_key)
return self.secret_manager_client.get_feathr_secret(variable_key)
Copy link
Collaborator

Choose a reason for hiding this comment

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

same as above

from typing import Any, Dict, List, Optional, Tuple


class FeathrSecretsManagementClient(ABC):
Copy link
Collaborator

Choose a reason for hiding this comment

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

the file should not be named as abc.py?

from aws_secretsmanager_caching.secret_cache import SecretCache


class AWSSecretManagerClient(FeathrSecretsManagementClient):
Copy link
Collaborator

Choose a reason for hiding this comment

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

the file name: aws_secret_manager

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
safe to test Tag to execute build pipeline for a PR from forked repo
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants