-
Notifications
You must be signed in to change notification settings - Fork 158
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
feat: Add options to specify AWS credential source for RDS functions #3289
base: master
Are you sure you want to change the base?
Conversation
Hi @hairyhum, |
There is a principal issue with using profile as a source of the credentials. Because the credentials here are the data source credentials, while profile is by design the data destination credential.
And it is still the case even with this change (unless I messed up and introduced a bug). There is no intention to switch away from profiles as destination (e.g. object store) configuration. |
Hi @hairyhum , |
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.
LGTM overall. left some small comments.
docs_new/functions.md
Outdated
|
||
::: tip NOTE | ||
|
||
`credentialsSource: serviceaccount` uses kanister operator service account. IAM role for service |
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.
would it not be a good idea to have another field (credentialsSA
) like credentialsSecret
, that can can be used to configured the service account?
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.
Maybe. The idea here was that an AWS IAM admin would allow kanister service account access to RDS by creating a role for it.
Associating a different service account would be better because it's not trivial to assign multiple roles to a single SA, but implementation-wise it's harder because we currently run RDS operation from within kanister controller (in the function code) meaning that we can't change SA on the flight there.
We can also try using AssumeRole and configure which role it will assume. This is going to be more flexible with less code changes.
I'd suggest we keep it for the follow-up PRs.
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.
ok 👍 .
8038c52
to
8b6c20d
Compare
Did manual testing on RDS. All seems to be working fine. Will need to update the example readme with a |
03cca2c
to
ab5398f
Compare
Currently AWS credentials are always taken from the profile, which limits the ability to export into non-s3 storage and s3 storage in other regions. Added options to specify a separate secret or use IAM role for service account. Signed-off-by: Daniil Fedotov <[email protected]>
Signed-off-by: Daniil Fedotov <[email protected]>
ab5398f
to
6f153f9
Compare
Change Overview
Currently AWS credentials are always taken from the profile, which limits
the ability to export into non-s3 storage and s3 storage in other regions.
Added options to specify a separate secret or use IAM role for service account.
Pull request type
Please check the type of change your PR introduces:
Issues
Test Plan
Run RDS example with new arguments added in the blueprint
💪 Manual
⚡ Unit test
💚 E2E