-
Notifications
You must be signed in to change notification settings - Fork 99
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
Use credentials file passed through config #90
Use credentials file passed through config #90
Conversation
Add a new supported key to the config (`credentialsFile`) 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 to load the Azure environment variables from. Signed-off-by: Bridget McErlean <[email protected]>
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.
@zubron this looks solid. I had to bring up the chance to reduce some code :)
} | ||
|
||
return nil | ||
} | ||
|
||
func loadEnv() error { |
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.
Since this function is reduced to just this one line, maybe it's better to get rid of it and call loadEnvFromCredentialsFile(map[string]string{})
directly both in the object_store.go and in the volume_snapshotter.go files. What do you think? I don't think having it adds any benefit to the code.
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.
Yeah, good thinking 👍 I think I initially put this in to avoid changes to the volume snapshotter file, but you're right that it doesn't really offer any benefit.
func loadEnv() error { | ||
envFile := os.Getenv("AZURE_CREDENTIALS_FILE") | ||
if envFile == "" { | ||
func loadEnvFromCredentialsFile(config map[string]string) error { |
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.
I don't remember if we added a general Velero documentation for this new decision making tree (if file, else env var), or if we should be adding this to each plugin doc. Just wanted to ensure we document this, but does not need to be in this PR if it should go in this repo.
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.
I was going to add documentation to the main Velero docs, but thinking about it now, the individual plugins will need doc updates to highlight that they are compatible with this new feature. Thanks for pointing that out!
@zubron, please update the year and add a changelog. |
5d95dd7
to
b9b7115
Compare
Signed-off-by: Bridget McErlean <[email protected]>
b9b7115
to
2f71549
Compare
|
||
// loadCredentialsIntoEnv loads the variables in the given credentials | ||
// file into the current environment. | ||
func loadCredentialsIntoEnv(credentialsFile string) error { |
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.
@carlisia I opted to break this down into a few smaller functions that I can compose together in the object store and volume snapshotter code to hopefully make it a bit clearer which credentials are being used. For example, in the Init
function for the volume snapshotter, we now explicitly state that we are using the credentials from the environment. It's more verbose but to me is more readable. Let me know if you disagree though :)
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.
Looks good
Signed-off-by: Bridget McErlean <[email protected]>
24f9a92
to
d169112
Compare
return nil | ||
} | ||
|
||
if err := godotenv.Overload(envFile); err != nil { | ||
return errors.Wrapf(err, "error loading environment from AZURE_CREDENTIALS_FILE (%s)", envFile) | ||
if err := godotenv.Overload(credentialsFile); err != nil { |
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.
Cool, this function not only reads the env file but also reads the file path 💯
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.
Same, the only concern is should we update the support matrix in this PR.
The rest LGTM, thank u.
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.
👍
* Use credentials file passed through config Add a new supported key to the config (`credentialsFile`) 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 to load the Azure environment variables from. Signed-off-by: Bridget McErlean <[email protected]> * Address code review comments and add changelog Signed-off-by: Bridget McErlean <[email protected]> * Add documentation for per-BSL credentials Signed-off-by: Bridget McErlean <[email protected]>
Add a new supported key to the config (
credentialsFile
) which allowsthe 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 to load the Azure environment variables from.
Fixes vmware-tanzu/velero#3429.
Signed-off-by: Bridget McErlean [email protected]