feat: add GCP Secret Manager OpenFeature provider#1772
feat: add GCP Secret Manager OpenFeature provider#1772mahpatil wants to merge 7 commits intoopen-feature:mainfrom
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new OpenFeature provider for Google Cloud Secret Manager, enabling feature flags to be managed as GCP secrets. The changes include the core provider implementation with caching and value conversion, along with comprehensive unit and integration tests, and a sample application. Feedback suggests adding an initial entry to the new module's CHANGELOG.md, refining the spotbugs-exclusions.xml to only include relevant exclusions, and improving the testability of FlagCache by injecting a Clock instance instead of relying on Instant.now() and Thread.sleep().
mahpatil
force-pushed
the
feat/gcp-secret-manager
branch
from
1c0fd8d to
0d10a01
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new OpenFeature provider for Google Cloud Secret Manager, enabling feature flags to be stored and managed as secrets. The implementation includes a TTL-based in-memory cache to optimize API usage and a converter to handle various OpenFeature data types. Feedback identifies several improvement opportunities, including fixing duplicate entries in the changelog, addressing a race condition and a potential 'thundering herd' issue in the caching logic, and strengthening input validation for the secret version configuration.
| @Override public Instant instant() { return now.get(); } | ||
| }; | ||
|
|
||
| FlagCache cache = new FlagCache(Duration.ofSeconds(30), 100, controllableClock); |
There was a problem hiding this comment.
What do we need this cache for?
There was a problem hiding this comment.
i think this has changed now. getAfterTtlExpiry is the only one that uses timedCache to check if cache is empty after TTL.
| // Thread A: re-insert fresh value for the same key | ||
| () -> sharedCache.put("key", "new-value"), | ||
| // Thread B: get() triggers expiry-removal of the old entry | ||
| () -> sharedCache.get("key")); |
There was a problem hiding this comment.
I believe this should either return nothing or the new value, and we should assert that
There was a problem hiding this comment.
This may be an outdated comment now based on new quote. Thread A's value should not have expired regardless of the order so it should stay as get-check-remove now synchronized.
|
The build pipeline currently fails for two reasons: |
mahpatil
force-pushed
the
feat/gcp-secret-manager
branch
from
bcefaa7 to
6af66e7
Compare
Signed off commits to fix DCO |
- TTL-based in-memory FlagCache with Clock injection for deterministic testing - Thread-safe cache: synchronized get/remove block eliminates check-then-act race - Double-checked locking in fetchWithCache prevents thundering herd on GCP API - FlagValueConverter uses Boolean.parseBoolean for boolean flag values - secretVersion validation added to GcpSecretManagerProviderOptions - VmLens concurrent cache test covering concurrent get/put/expiry scenarios - Tests for negative and exponential number formats in FlagValueConverterTest - Integration test null guard for provider Signed-off-by: Mahesh Patil <maheshfinity@gmail.com>
- Move all setup (clock, cache, expired entry) inside the VmLens loop
so each interleaving starts with a clean, deterministic state
- Replace the conditional assertion with an unconditional check:
assertThat(cache.get("key")).isPresent().hasValue("new-value")
After Thread A's put() completes, the new value must always be present;
the previous if-present guard would silently pass a correctness bug
- Remove unused outer cache/controllableClock/now variables and the
redundant sharedCache/freshClock inside the loop
Signed-off-by: Mahesh Patil <maheshfinity@gmail.com>
… test" This reverts commit 58cf2a6. Signed-off-by: Mahesh Patil <maheshfinity@gmail.com>
The previous implementation had several structural problems:
- clock, cache (and an unused outer cache) were recreated inside the
VmLens while-loop. VmLens needs stable object references across
iterations to correctly track which shared memory locations are
concurrently accessed; recreating objects each iteration means VmLens
sees different heap addresses and cannot build a reliable access model.
- now.set(now.get().plusSeconds(31)) inside the loop kept advancing the
clock monotonically, so state was never cleanly reset between
interleavings and could allow "new-value" entries to expire.
- The assertion was too weak: if (result.isPresent()) { ... } would
silently pass even when the cache is empty, hiding the exact bug the
test is meant to catch.
Fixed by:
- Creating clock and cache once outside the loop (stable references)
- Resetting state inside the loop to fixed instants (t0/t1) via
cache.clear() + now.set(t0) before each interleaving
- Using an unconditional assertion:
assertThat(cache.get("key")).isPresent().hasValue("new-value")
After Runner.runParallel returns, Thread A's put() has completed so
"new-value" must always survive Thread B's expiry-removal.
Signed-off-by: Mahesh Patil <maheshfinity@gmail.com>
mahpatil
force-pushed
the
feat/gcp-secret-manager
branch
from
74e41c0 to
1d65ba5
Compare
Adds getOnTimedOutEntryWhileConcurrentInsertNeverReturnsStaleValue following the FlagdProviderCTest pattern: shared state prepared once before the interleaving loop in @beforeeach, only Runner.runParallel inside the loop, and assertions embedded in the parallel lambdas. Verifies that get() on a timed-out entry concurrent with a put() of the same key never returns the stale value — result must be empty or the freshly inserted value. Signed-off-by: Mahesh Patil <maheshfinity@gmail.com>
|
I hope all comments have been addressed now. Please let me know if there's anything outstanding. |
| } | ||
| }; | ||
|
|
||
| FlagCache cache = new FlagCache(Duration.ofSeconds(30), 100, clock); |
There was a problem hiding this comment.
Why do we create a cache, clock and now when all of that is already done in the setup method?
|
Good catch — removed the duplicate |
…oseNewEntry Signed-off-by: Mahesh Patil <maheshfinity@gmail.com>
mahpatil
force-pushed
the
feat/gcp-secret-manager
branch
from
d01ff47 to
ea481a2
Compare
|
Re: #1771 (cc @aepfli @chrfwow @mahpatil) Looking at #1771 + #1772 side by side, I think there's been a small misunderstanding of @aepfli's earlier feedback. IIUC he asked for the PR to be split for review hygiene ("a pull request is small, and provides just one additional feature"), not necessarily for two separate Maven modules. The split-PRs ask has been honoured; the split-modules part was an additional interpretation. @chrfwow already raised the same question on #1771. Posting here too since this is where active review is happening, and IMO it's much cheaper to settle before #1772 merges than after. Let's keep 2 PRs but land them in one module.
@mahpatil - to keep things reviewable, suggest stacking the branches: rework this PR (#1772) to add a single Suggested module name: @aepfli - was your earlier ask only about the PR split, or did you also intend two artifacts? Happy to discuss if there's a packaging reason I'm missing. |
4 participants
Summary
samples/gcp-secret-manager-sample/with setup/teardown scriptsProvider Details
providers/gcp-secret-managerGcpSecretManagerProviderTest plan
FlagCache,FlagValueConverter, andGcpSecretManagerProviderGcpSecretManagerProviderIntegrationTest) requires a real GCP project — setGCP_PROJECT_IDenv var to runsamples/gcp-secret-manager-sample/README.mdto run end-to-end