Add support to read JSON credential files present in a directory passed through the <PROVIDER>_APPLICATION_CREDENTIALS environment variable
#730
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ed through the `${PROVIDER}_APPLICATION_CREDENTIALS` environment variable.
* Add support to read JSON credential files present in a directory which
is passed through the `${PROVIDER}_APPLICATION_CREDENTIALS` environment
variable for all (applicable) providers.
* Remove examples which suggest that cloud provider credentials can be
passed through a JSON file since this will be deprecated in future releases.
* Improve error handling in various `SnapStore` related files for each
cloud provider.
* Updated docs to discourage usage of JSON files to pass credentials.
* Housekeeping in `pkg/server/backuprestoreserver.go`.
Changed an unused named function parameter to `_`, and removed a
tautological if condition.
renormalize
added
kind/bug
gardener-robot
added
needs/review
gardener-robot-ci-3
added
reviewed/ok-to-test
anveshreddy18
requested changes
Apr 29, 2024
There was a problem hiding this comment.
Thanks for the PR @renormalize. I left a couple of comments. PTAL
|
Would 3 releases be necessary for deprecation @gardener/etcd-druid-maintainers? We're still in a 0 major version and given the release cadence of etcd-backup-restore, this will take a long time as @anveshreddy18 has mentioned in a review comment. Being in "alpha" gives us the freedom to deprecate features without any repercussions. However, I do understand that etcd-backup-restore has been used by a lot of members who have come to rely on their particular workflows with regards to passing credentials, and 1 release is not enough time. Would 2 be good enough? |
ishan16696
reviewed
May 2, 2024
* Remove JSON method from the various ways to pass credentials section. * Remove a confusing line about passing credentials as individual files.
gardener-robot-ci-3
added
reviewed/ok-to-test
ishan16696
requested changes
May 13, 2024
* Fix comments documenting functions * Enhance `docs/deployment/getting_started.md` * Unnecessary argument for error string creation in `pkg/snapstore/utils.go` removed
gardener-robot-ci-3
added
reviewed/ok-to-test
|
I've addressed your comments @ishan16696, feel free to let me know if you have any more suggestions! |
renormalize
changed the title
${PROVIDER}_APPLICATION_CREDENTIALS environment variable<PROVIDER>_APPLICATION_CREDENTIALS environment variable
ishan16696
reviewed
May 16, 2024
anveshreddy18
approved these changes
May 16, 2024
|
Thanks for the approval @ishan16696 @anveshreddy18! Thanks for the feedback that made this PR better. Will merge soon. |
gardener-robot
added
the
status/closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it:
The dual way of passing credentials to etcd-backup-restore, but the file-type agnostic way of passing credentials in gardener/etcd-druid caused confusion in the community consuming both etcd-druid and etcd-backup-restore.
Passing credentials through a JSON file in etcd-druid does not cause it to error, but is not supported by etcd-druid - and thereby causes etcd-backup-restore to error as a result, even though etcd-backup-restore supports credentials in JSON files.
If consumers familiar with etcd-backup-restore who pass credentials as JSON typically, use the same method to pass credentials in etcd-druid, etcd-backup-restore errors.
To temporarily solve this, if credentials are passed through a JSON file in etcd-druid (i.e. as a file in a directory whose path is exported in
<PROVIDER>_APPLICATION_CREDENTIALS), etcd-backup-restore handles this and fetches the credentials from the JSON file. If a JSON file is present in the directory, all other files are ignored.However, as mentioned in #729, credentials will only be passed to etcd-backup-restore as individual files in a directory in 3 releases from the next (as of writing, v0.31.0), and credentials in the form of JSON files will be deprecated.
This PR makes the following changes:
Add support to read JSON credential files present in a directory which is passed through the
<PROVIDER>_APPLICATION_CREDENTIALSenvironment variable for all (applicable) providers.Remove examples which suggest that cloud provider credentials can be passed through a JSON file since this will be deprecated in future releases.
Improve error handling in various
SnapStorerelated files for each cloud provider.Updated docs to discourage usage of JSON files to pass credentials.
Housekeeping in
pkg/server/backuprestoreserver.go. Changed an unused named function parameter to_, and removed a tautological if condition.Tested with the following cloud providers to test the feature and check for regressions:
service_account.jsonis accepted so this issue will not be faced.)Which issue(s) this PR fixes:
Fixes #729
Special notes for your reviewer:
Please update the block header and the release note to something that you feel is more appropriate.
Release note: