Use credentials file passed through config #69
Conversation
zubron
force-pushed
the
use-credentials-from-config
branch
from
43d41dc
to
33a2c0b
Compare
Add a new supported key to the config which allows the path to a credentials file to be passed through to the ObjectStore plugin. If that key is set in the config, use that file as the credentials file for the AWS client. This can be tested by adding a new key to the `cloud-credentials` secret and adding the key value pair `credentialsFile=/credentials/<key_name>`. Signed-off-by: Bridget McErlean <bmcerlean@vmware.com>
zubron
force-pushed
the
use-credentials-from-config
branch
from
33a2c0b
to
ea21fd8
Compare
carlisia
requested review from
nrb,
jenting,
carlisia,
ashish-amarnath and
dsu-igeek
| @@ -196,6 +209,9 @@ func (o *ObjectStore) Init(config map[string]string) error { | |||
| if len(caCert) > 0 { | |||
| sessionOptions.CustomCABundle = strings.NewReader(caCert) | |||
| } | |||
| if credentialsFile != "" { | |||
There was a problem hiding this comment.
If this is != "", does it not need the processing as in this line: https://github.com/vmware-tanzu/velero-plugin-for-aws/pull/69/files#diff-fdece575520884a3b21814a0f7d1d42953950bed4aff84fe9f9ca635b958d717R178
?
There was a problem hiding this comment.
I skipped it here because if it was not empty, it would already have been checked above but it would probably be better to have the check in both places. I could move the above validation into a separate function and call it in both locations. Would that work for you?
There was a problem hiding this comment.
Yes, yes, I see. Something threw me off and I think it's this:
LN 184 already sets sessionOptions.SharedConfigFiles = []string{credentialsFile} so this is not needed? If there was an error that didn't allow for the sessionOptions.SharedConfigFiles to be set, the code would've returned. I don't this LNs 212-214 are needed.
There was a problem hiding this comment.
Ah, it's because the sessionOptions variable is shadowed in this block on line 208 so this is a new object that we're changing here. I wonder if we can use the same sessionOptions object for both so that we don't need to performa all these checks again. I'll take a look and try to improve it. I haven't reviewed it thoroughly yet, but this may already be addressed in #71.
There was a problem hiding this comment.
Oh right, I missed that.
If you change LN 208 from:
sessionOptions := session.Options{Config: *publicConfig, Profile: credentialProfile}
to:
sessionOptions = session.Options{Config: *publicConfig, Profile: credentialProfile}
would that not just overwrite that struct and allow you to continue using the same instance of sessionOptions?
There was a problem hiding this comment.
@carlisia Yes, it would override it and use the same instance, but I would still need to add the CACert and credentials to it. I added a new function newSessionOptions to handle creation of the session so that we use the same logic for both the main session and the public session, which I think it is a bit clearer.
|
Almost forgot: the file in this PR needs a year change. |
We have some repeated logic for creating session option. This introduces a new function to handle the creation of those session options. Signed-off-by: Bridget McErlean <bmcerlean@vmware.com>
zubron
force-pushed
the
use-credentials-from-config
branch
3 times, most recently
from
53a3c8e
to
ca65f0f
Compare
Signed-off-by: Bridget McErlean <bmcerlean@vmware.com>
zubron
force-pushed
the
use-credentials-from-config
branch
from
ca65f0f
to
0221619
Compare
|
BTW, since the PR describes Velero 1.6.x version, not sure should we change the support matrix https://github.com/vmware-tanzu/velero-plugin-for-aws/#compatibility in this PR also? 🤔 |
|
Should we add the changelog? https://github.com/vmware-tanzu/velero-plugin-for-aws/blob/main/CONTRIBUTING.md#changelog |
Signed-off-by: Bridget McErlean <bmcerlean@vmware.com>
* Use credentials file passed through config Add a new supported key to the config which allows the path to a credentials file to be passed through to the ObjectStore plugin. If that key is set in the config, use that file as the credentials file for the AWS client. This can be tested by adding a new key to the `cloud-credentials` secret and adding the key value pair `credentialsFile=/credentials/<key_name>`. Signed-off-by: Bridget McErlean <bmcerlean@vmware.com> * Add function to create session options We have some repeated logic for creating session option. This introduces a new function to handle the creation of those session options. Signed-off-by: Bridget McErlean <bmcerlean@vmware.com> * Add documentation for per-BSL credentials Signed-off-by: Bridget McErlean <bmcerlean@vmware.com> * Address code review and add changelog Signed-off-by: Bridget McErlean <bmcerlean@vmware.com>
Add a new supported key to the config which allows the path to a
credentials file to be passed through to the ObjectStore plugin.
If that key is set in the config, use that file as the credentials
file for the AWS client.
This can be tested by adding a new key to the
cloud-credentialssecretand adding the key value pair
credentialsFile=/credentials/<key_name>.Fixes vmware-tanzu/velero#3428.
Signed-off-by: Bridget McErlean bmcerlean@vmware.com