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

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ea21fd8
Use credentials file passed through config
zubron Dec 12, 2020
16a5b9b
Add function to create session options
zubron Mar 1, 2021
0221619
Add documentation for per-BSL credentials
zubron Mar 2, 2021
fb9727b
Address code review and add changelog
zubron Mar 4, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
16 changes: 16 additions & 0 deletions velero-plugin-for-aws/object_store.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"crypto/tls"
"io"
"net/http"
"os"
"sort"
"strconv"
"strings"
Expand All @@ -46,6 +47,7 @@ const (
s3ForcePathStyleKey = "s3ForcePathStyle"
bucketKey = "bucket"
signatureVersionKey = "signatureVersion"
credentialsFileKey = "credentialsFile"
credentialProfileKey = "profile"
serverSideEncryptionKey = "serverSideEncryption"
insecureSkipTLSVerifyKey = "insecureSkipTLSVerify"
Expand Down Expand Up @@ -90,6 +92,7 @@ func (o *ObjectStore) Init(config map[string]string) error {
kmsKeyIDKey,
s3ForcePathStyleKey,
signatureVersionKey,
credentialsFileKey,
credentialProfileKey,
serverSideEncryptionKey,
insecureSkipTLSVerifyKey,
Expand All @@ -105,6 +108,7 @@ func (o *ObjectStore) Init(config map[string]string) error {
s3ForcePathStyleVal = config[s3ForcePathStyleKey]
signatureVersion = config[signatureVersionKey]
credentialProfile = config[credentialProfileKey]
credentialsFile = config[credentialsFileKey]
serverSideEncryption = config[serverSideEncryptionKey]
insecureSkipTLSVerifyVal = config[insecureSkipTLSVerifyKey]

Expand Down Expand Up @@ -170,6 +174,15 @@ func (o *ObjectStore) Init(config map[string]string) error {
if len(caCert) > 0 {
sessionOptions.CustomCABundle = strings.NewReader(caCert)
}
if credentialsFile != "" {
if _, err := os.Stat(credentialsFile); err != nil {
if os.IsNotExist(err) {
return errors.Wrapf(err, "provided credentialsFile does not exist")
}
return errors.Wrapf(err, "could not get credentialsFile info")
}
sessionOptions.SharedConfigFiles = []string{credentialsFile}
}
serverSession, err := getSession(sessionOptions)
if err != nil {
return err
Expand All @@ -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

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.

sessionOptions.SharedConfigFiles = []string{credentialsFile}
}
publicSession, err := getSession(sessionOptions)
if err != nil {
return err
Expand Down