Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Use credentials file passed through config #69

Merged
merged 4 commits into from
Mar 6, 2021
Merged

Use credentials file passed through config #69

merged 4 commits into from
Mar 6, 2021

Conversation

zubron
Copy link
Contributor

@zubron zubron commented Dec 12, 2020 •
edited
Loading

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>.

Fixes vmware-tanzu/velero#3428.

Signed-off-by: Bridget McErlean bmcerlean@vmware.com

@zubron zubron mentioned this pull request Dec 12, 2020
Merged
@zubron zubron mentioned this pull request Feb 8, 2021
Merged
@zubron zubron force-pushed the use-credentials-from-config branch from 43d41dc to 33a2c0b Compare February 8, 2021 22:17
@zubron zubron mentioned this pull request Feb 8, 2021
Closed
@zubron
Use credentials file passed through config
ea21fd8 
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 zubron marked this pull request as ready for review February 20, 2021 22:16
carlisia
carlisia reviewed Feb 24, 2021
View reviewed changes
velero-plugin-for-aws/object_store.go Outdated
@@ -196,6 +209,9 @@ func (o *ObjectStore) Init(config map[string]string) error {
if len(caCert) > 0 {
sessionOptions.CustomCABundle = strings.NewReader(caCert)
}
if credentialsFile != "" {
Copy link
Contributor

@carlisia carlisia Feb 24, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

carlisia
carlisia reviewed Feb 24, 2021
View reviewed changes
Copy link
Contributor

@carlisia carlisia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks solid, just wanted to do a sanity check.

@carlisia
Copy link
Contributor

Almost forgot: the file in this PR needs a year change.

@zubron
Add function to create session options
16a5b9b 
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 zubron force-pushed the use-credentials-from-config branch 3 times, most recently from 53a3c8e to ca65f0f Compare March 2, 2021 18:57
@zubron
Add documentation for per-BSL credentials
0221619 
Signed-off-by: Bridget McErlean <bmcerlean@vmware.com>
@zubron zubron force-pushed the use-credentials-from-config branch from ca65f0f to 0221619 Compare March 2, 2021 18:58
jenting
jenting reviewed Mar 3, 2021
View reviewed changes
Copy link
Contributor

@jenting jenting left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rest LGTM

velero-plugin-for-aws/object_store.go Outdated Show resolved Hide resolved
@jenting
Copy link
Contributor

jenting commented Mar 3, 2021

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? 🤔

@jenting
Copy link
Contributor

jenting commented Mar 3, 2021

Should we add the changelog? https://github.com/vmware-tanzu/velero-plugin-for-aws/blob/main/CONTRIBUTING.md#changelog

@zubron
Address code review and add changelog
fb9727b 
Signed-off-by: Bridget McErlean <bmcerlean@vmware.com>
jenting
jenting approved these changes Mar 4, 2021
View reviewed changes
Copy link
Contributor

@jenting jenting left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank u 🎉

@carlisia carlisia self-requested a review March 6, 2021 01:10
carlisia
carlisia approved these changes Mar 6, 2021
View reviewed changes
Copy link
Contributor

@carlisia carlisia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@carlisia carlisia merged commit e6ffa2a into vmware-tanzu:main Mar 6, 2021
mirage2012 pushed a commit to mirage2012/velero-plugin-for-aws that referenced this pull request Jun 20, 2023
@zubron @mirage2012
Use credentials file passed through config (vmware-tanzu#69)
865a2d5 
* 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jenting jenting jenting approved these changes

@carlisia carlisia carlisia approved these changes

@nrb nrb Awaiting requested review from nrb

@ashish-amarnath ashish-amarnath Awaiting requested review from ashish-amarnath

@dsu-igeek dsu-igeek Awaiting requested review from dsu-igeek

Assignees
No one assigned
Labels
release-blocker
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[velero-plugin-for-aws] Add support for multiple credentials
4 participants
@zubron @carlisia @jenting @nrb