Repository navigation
[Python] Resolve AWS credentials through a secret manager (--aws_credentials_secret_manager) - #40547
Conversation
…als from a secret manager In this case, --aws_access_key_id, --aws_secret_access_key and --aws_session_token can be secret specs instead of plain-text values.
db79e12 to
e439d58
Compare
Let callers override non-credential AWS client settings (e.g. region_name, endpoint_url, verify, disable_ssl) on top of the --aws_* options and the service overrides.
e439d58 to
b9d1d30
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #40547 +/- ##
=============================================
+ Coverage 56.16% 79.94% +23.77%
=============================================
Files 1124 488 -636
Lines 178336 82347 -95989
Branches 1489 0 -1489
=============================================
- Hits 100170 65829 -34341
+ Misses 75645 16518 -59127
+ Partials 2521 0 -2521
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Assigning reviewers: R: @claudevdm for label python. Note: If you would like to opt out of this review, comment Available commands:
The PR bot will only process comments in the main thread (not review comments). |
claudevdm
left a comment
There was a problem hiding this comment.
Left some questions but LGTM
| With no secret_manager, Secret.from_json returns the value itself (as a | ||
| RawSecret), so plain credentials keep working. | ||
| """ | ||
| if not value: |
There was a problem hiding this comment.
Maybe also return early if secret manager is not set here?
There was a problem hiding this comment.
We didn't do that here, because Secret.from_json() is going to handle the behavior (RawSecret) when secret manager is not set.
| RawSecret), so plain credentials keep working. | ||
| """ | ||
| if not value: | ||
| return value |
There was a problem hiding this comment.
Is value expected to be a string here? What if it is passed as a dictionary? Maybe check if it is dictionary and convert to a str?
There was a problem hiding this comment.
Yes. We expected the value to be a string. The type checking is already in Secret.from_json(), but we can definitely add type hint in these functions.
There was a problem hiding this comment.
Added type hints.
| self.assertEqual(kwargs['aws_secret_access_key'], 'SECRETEXAMPLE') | ||
|
|
||
| @mock.patch('google.cloud.secretmanager.SecretManagerServiceClient') | ||
| def test_gcp_secret_manager_end_to_end(self, mock_gcp_client): |
There was a problem hiding this comment.
Should this test be skipped if secretmanager is not installed similar to secret_test.py?
There was a problem hiding this comment.
Good call. Added a condition to skip this test.
Motivation
Today, AWS credentials have to be passed as plain text in pipeline options (
--aws_access_key_id,--aws_secret_access_key,--aws_session_token). That means they show up in job graphs, launch commands and logs. This PR lets them be references to secrets stored in a secret manager instead.Changes
--aws_credentials_secret_managerinAwsOptions.--s3_*overrides are secret specs (a JSON string or plain text). They are resolved withSecret.from_json(spec, secret_manager).apache_beam/io/aws/clients/client_options.py, so every boto3 client Beam creates is configured the same wayExample
Part of #40528.