Abstract Secret Management Support #703
Conversation
xiaoyongzhu
added
the
safe to test
xiaoyongzhu
changed the title
xiaoyongzhu
requested review from
jaymo001,
Yuqing-cat,
windoze,
blee1234,
blrchen,
edwincheung,
hangfei and
jainr
| import botocore.session | ||
| from aws_secretsmanager_caching import SecretCache, SecretCacheConfig | ||
|
|
||
| client = botocore.session.get_session().create_client( |
There was a problem hiding this comment.
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'), |
There was a problem hiding this comment.
Should this introduce an else and error out in the case if the secrets manager client is not supported by feathr?
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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) |
| from typing import Any, Dict, List, Optional, Tuple | ||
|
|
||
|
|
||
| class FeathrSecretsManagementClient(ABC): |
There was a problem hiding this comment.
the file should not be named as abc.py?
| from aws_secretsmanager_caching.secret_cache import SecretCache | ||
|
|
||
|
|
||
| class AWSSecretManagerClient(FeathrSecretsManagementClient): |
There was a problem hiding this comment.
the file name: aws_secret_manager
jaymo001
force-pushed
the
secret_manager
branch
from
0b8d6ef
to
7054d07
Compare
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_managerandtest_feathr_get_secrets_from_azure_key_vault.Note that
test_feathr_get_secrets_from_aws_secret_managerwill fail, claiming that the credentials cannot be found. This is actually expected as the CI workflow forAWS_ACCESS_KEY_IDandAWS_SECRET_ACCESS_KEYhas 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?
This PR also adds a parameter in FeathrClient so users can pass the secret manager client that they want to use to retrieve credentials.