Abstract Secret Management Support - #703
Conversation
Sorry, something went wrong.
| 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.
Sorry, something went wrong.
| 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?
Sorry, something went wrong.
There was a problem hiding this comment.
good catch. I'll update
Sorry, something went wrong.
| if self.secret_manager_client: | ||
| 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)?
Sorry, something went wrong.
| if self.secret_manager_client: | ||
| try: | ||
| return self.akv_client.get_feathr_akv_secret(variable_key) | ||
| return self.secret_manager_client.get_feathr_secret(variable_key) |
There was a problem hiding this comment.
same as above
Sorry, something went wrong.
| 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?
Sorry, something went wrong.
| from aws_secretsmanager_caching.secret_cache import SecretCache | ||
|
|
||
|
|
||
| class AWSSecretManagerClient(FeathrSecretsManagementClient): |
There was a problem hiding this comment.
the file name: aws_secret_manager
Sorry, something went wrong.
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?
This PR also adds a parameter in FeathrClient so users can pass the secret manager client that they want to use to retrieve credentials.