Optimize dynamic loading of IaaS Credentials using credential file timestamps #670
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
|
@renormalize Thank you for your contribution. |
gardener-robot
added
needs/review
|
Thank you @renormalize for your contribution. Before I can start building your PR, a member of the organization must set the required label(s) {'reviewed/ok-to-test'}. Once started, you can check the build status in the PR checks section below. |
gardener-robot
added
size/l
renormalize
force-pushed
the
optimize-dynamic-loading
branch
from
9822ec3 to
7a5e52b
Compare
gardener-robot
added
size/xl
|
/assign @ishan16696 |
seshachalam-yv
requested changes
Nov 13, 2023
There was a problem hiding this comment.
Thanks @renormalize for PR
Overall looks good to me.
Please address my NIT comments
seshachalam-yv
added
the
reviewed/ok-to-test
gardener-robot-ci-1
added
needs/ok-to-test
seshachalam-yv
added
the
reviewed/ok-to-test
gardener-robot-ci-3
removed
the
reviewed/ok-to-test
seshachalam-yv
added
the
reviewed/ok-to-test
gardener-robot-ci-1
removed
the
reviewed/ok-to-test
There was a problem hiding this comment.
Thanks @renormalize for addressing my comments.
Overall looks good to me.
renormalize
force-pushed
the
optimize-dynamic-loading
branch
from
7d02cd4 to
fd46a0c
Compare
gardener-robot-ci-2
removed
the
reviewed/ok-to-test
ishan16696
reviewed
Dec 6, 2023
pkg/snapstore/snapstore_test.go
Outdated
Comment on lines
325
to
328
| for providerIndex, provider := range providers { | ||
| // value is used in the closure, thus is to be redefined | ||
| providerIndex := providerIndex | ||
| provider := provider |
There was a problem hiding this comment.
I didn't get the reasoning, why they require to be redefined ?
There was a problem hiding this comment.
The reason the loop variables, and many variables in the tests had to be redefined as so was that the closure functions which are used to define the test cases using the Ginkgo framework capture the address of the loop variables when Describe, Context, and It functions run. This is essentially the infamous for loop "bug" when using the address of the loop variable inside the for loop in Go. Redefining ensures the right values are passed to the closures in new variables instead of the actual loop variable which is rewritten every time during the spec tree construction, before the tests are actually run.
…).TempDir()` and structs for cleaner tests. * Only Ginkgo provided temporary directories are used for the unit tests. * Structs defined where each instance holds all relevant information for the test. * Same tests cover both directory flow and JSON flow.
gardener-robot-ci-1
added
the
reviewed/ok-to-test
gardener-robot-ci-2
removed
the
reviewed/ok-to-test
seshachalam-yv
requested changes
Dec 13, 2023
* Changed a few function definition comments * Changed the verb to `%q` in Ginkgo nodes' strings for better string formatting
gardener-robot-ci-3
added
reviewed/ok-to-test
seshachalam-yv
approved these changes
Dec 13, 2023
There was a problem hiding this comment.
Overall looks good to me
Thanks @renormalize for addressing my review comments
ishan16696
added
reviewed/do-not-merge
gardener-robot
added
the
status/closed
|
@ishan16696 this needs to be cherry-picked to |
yes .... but we need to revisit all PRs which are merged after |
|
Ok. Then @renormalize can you please take care of raising a cherry-pick PR on |
renormalize
added a commit
to renormalize/etcd-backup-restore
that referenced
this pull request
Dec 20, 2023
…mestamps (gardener#670) * Dynamic access credential rotation unit tests written for all providers * Unit tests for GetSnapstoreSecretModifiedTime() use idiomatic `time` functions * Unit tests reworked to fix concourse build * Changes to test GinkgoT and os temp directories for unit tests for dynamic loading * Added a direct os.Stat to ensure the correct time is being fetched * Workaround to change the modification time of a credential file in the unit test * Writing to the file does not change the modification time on the concourse machine. This is worked around by recreating the credential file. * Better logging during the tests for easier debugging. * Changed the directory used by the dynamic credential loading unit tests * The directory is set to /test/credential to store the files. * Cleaner logging. * Added test to verify concourse's time modification os call * Added a time.Sleep() before modifying the file. * Replaced random file deletion test with file deletion of each possible credential file * Unit tests now use a sleep to fix issues on concourse, .ci/unit_test restored so all tests run. * Addressing review comments for units tests of dynamic credential loading 1 * Dynamic credential loading unit tests: removed file creation and updation logs * `GetSnapstoreSecretModifiedTime` unit tests rewritten using `GinkgoT().TempDir()` and structs for cleaner tests. * Only Ginkgo provided temporary directories are used for the unit tests. * Structs defined where each instance holds all relevant information for the test. * Same tests cover both directory flow and JSON flow. * Addressing review comments * Changed a few function definition comments * Changed the verb to `%q` in Ginkgo nodes' strings for better string formatting
ishan16696
pushed a commit
that referenced
this pull request
Dec 20, 2023
…mestamps (#670) (#695) * Dynamic access credential rotation unit tests written for all providers * Unit tests for GetSnapstoreSecretModifiedTime() use idiomatic `time` functions * Unit tests reworked to fix concourse build * Changes to test GinkgoT and os temp directories for unit tests for dynamic loading * Added a direct os.Stat to ensure the correct time is being fetched * Workaround to change the modification time of a credential file in the unit test * Writing to the file does not change the modification time on the concourse machine. This is worked around by recreating the credential file. * Better logging during the tests for easier debugging. * Changed the directory used by the dynamic credential loading unit tests * The directory is set to /test/credential to store the files. * Cleaner logging. * Added test to verify concourse's time modification os call * Added a time.Sleep() before modifying the file. * Replaced random file deletion test with file deletion of each possible credential file * Unit tests now use a sleep to fix issues on concourse, .ci/unit_test restored so all tests run. * Addressing review comments for units tests of dynamic credential loading 1 * Dynamic credential loading unit tests: removed file creation and updation logs * `GetSnapstoreSecretModifiedTime` unit tests rewritten using `GinkgoT().TempDir()` and structs for cleaner tests. * Only Ginkgo provided temporary directories are used for the unit tests. * Structs defined where each instance holds all relevant information for the test. * Same tests cover both directory flow and JSON flow. * Addressing review comments * Changed a few function definition comments * Changed the verb to `%q` in Ginkgo nodes' strings for better string formatting
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:
Dynamic loading of IaaS credentials currently involves computing the hash of the secret file and comparing the in-memory hash with the newly computed hash to check for changes in the access credentials.
When there is a change in the hash, the
SnapStoreobject is updated. The computation of this hash happens whenever a delta snapshot is triggered.This is now optimized by checking the timestamps of the credential files which were used to calculate the hash.
Currently the following providers are supported:
Which issue(s) this PR fixes:
Fixes #449 #683
Special notes for your reviewer:
Release note: