-
Notifications
You must be signed in to change notification settings - Fork 665
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
[dy] Support reading authentication settings from AWS secrets manager #4674
[dy] Support reading authentication settings from AWS secrets manager #4674
Conversation
@@ -300,6 +304,13 @@ def set_project_uuid_from_metadata() -> None: | |||
project_uuid = config.get('project_uuid') | |||
|
|||
|
|||
def update_settings_on_metadata_change() -> None: | |||
global project_uuid |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where is the project_uuid used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops i'll remove that
mage_ai/settings/backends.py
Outdated
except ClientError as error: | ||
if error.response['Error']['Code'] == 'ResourceNotFoundException': | ||
return None | ||
raise |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what are other possible errors? do we not fall back to env variables for those errors?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point. i'll log an error and return None
self.client = boto3.client('secretsmanager') | ||
self.prefix = kwargs.get('prefix', '') | ||
|
||
def _get(self, key: str, **kwargs) -> Optional[str]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we also allow user to configure whether to use AWS secret cache?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good idea, i'll add that
e4050e9
to
438fdf5
Compare
@wangxiaoyou1993 PTAL |
438fdf5
to
ececb04
Compare
Have you tested with all those different authentication methods? |
can you also update the doc? |
b170fd7
to
eb338ad
Compare
…mage-ai#4674) * [dy] Update settings * [dy] Update * [dy] Update again * [dy] Update once more * [dy] Fix test * [dy] Fix session test * [dy] Fix session test again * [dy] Fix test * [dy] Fix test 2 * [dy] Address comments * [dy] Update * [dy] Rebase * [dy] Update comments * [dy] Update docs * [dy] Move all git settings
Description
This PR adds support for reading settings from AWS secrets manager. Only some of the existing settings have been moved to using the new
Settings
class. Most server settings that are not secrets are still being directly read from environment variables.In order to set the settings backend, the user will need to add a section to the project metadata. Here is an example to set the setting backend to AWS secrets manager:
This PR also adds an observer to the project metadata file so that if there are any updates to that file, the settings backend can be updated.
How Has This Been Tested?
Checklist
docs/mint.json
cc: