feat: add GCP Secret Manager OpenFeature provider by mahpatil · Pull Request #1772 · open-feature/java-sdk-contrib

mahpatil · 2026-04-09T05:18:07Z

Summary

Adds a new OpenFeature provider for GCP Secret Manager that reads feature flags from GCP Secret Manager secrets
Includes flag caching, value conversion for all OpenFeature types (bool, string, int, double, object), and background polling for updates
Adds a sample application under samples/gcp-secret-manager-sample/ with setup/teardown scripts

Provider Details

Package: providers/gcp-secret-manager
Class: GcpSecretManagerProvider
Supports all OpenFeature flag types via structured or plain-text secret values
Configurable poll interval and GCP project settings

Test plan

Unit tests for FlagCache, FlagValueConverter, and GcpSecretManagerProvider
Integration test (GcpSecretManagerProviderIntegrationTest) requires a real GCP project — set GCP_PROJECT_ID env var to run
Sample app: follow samples/gcp-secret-manager-sample/README.md to run end-to-end

gemini-code-assist

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().

aepfli · 2026-04-13T12:44:25Z

/gemini review

gemini-code-assist

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.

chrfwow · 2026-04-15T10:21:01Z

+        @Test
+        @DisplayName("converts numeric string to Integer")
+        void numericString() {
+            assertThat(FlagValueConverter.convert("42", Integer.class)).isEqualTo(42);


Maybe we should also add a test for negative numbers

@chrfwow there is already negativeNumericString below?

chrfwow · 2026-04-15T10:22:09Z

+        @Test
+        @DisplayName("converts numeric string to Double")
+        void numericString() {
+            assertThat(FlagValueConverter.convert("3.14", Double.class)).isEqualTo(3.14);


Do we also want to support the exponential format, e.g. 3141.5e-3? We should add tests accordingly

added exponentialNotation

chrfwow · 2026-04-20T06:07:50Z

+            @Override public Instant instant() { return now.get(); }
+        };
+
+        FlagCache cache = new FlagCache(Duration.ofSeconds(30), 100, controllableClock);


What do we need this cache for?

i think this has changed now. getAfterTtlExpiry is the only one that uses timedCache to check if cache is empty after TTL.

chrfwow · 2026-04-20T06:14:30Z

+                        // Thread B: get() triggers expiry-removal of the old entry
+                        () -> sharedCache.get("key"));
+
+                // After both threads complete, either the new value is present or the cache is


After a thread completes that puts a new value to the cache, I would not expect the cache to be empty. Am I missing something here?

chrfwow · 2026-04-20T06:14:49Z

+                        // 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"));


I believe this should either return nothing or the new value, and we should assert that

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.

chrfwow · 2026-04-21T06:26:59Z

The build pipeline currently fails for two reasons:
The commits are not signed off. To fix this, you will need to sign off all your changes and force push them on this branch
The type check fails, this can be fixed automatically when you run the spotlessApply command

mahpatil · 2026-04-24T19:44:20Z

The build pipeline currently fails for two reasons: The commits are not signed off. To fix this, you will need to sign off all your changes and force push them on this branch The type check fails, this can be fixed automatically when you run the spotlessApply command

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>

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>

mahpatil · 2026-04-25T07:27:27Z

I hope all comments have been addressed now. Please let me know if there's anything outstanding.

mahpatil requested a review from a team as a code owner April 9, 2026 05:18

gemini-code-assist Bot reviewed Apr 9, 2026

View reviewed changes

mahpatil force-pushed the feat/gcp-secret-manager branch from 1c0fd8d to 0d10a01 Compare April 11, 2026 20:44

gemini-code-assist Bot reviewed Apr 13, 2026

View reviewed changes

chrfwow reviewed Apr 15, 2026

View reviewed changes

chrfwow mentioned this pull request Apr 15, 2026

feat: add GCP Parameter Manager OpenFeature provider #1771

Open

3 tasks

chrfwow reviewed Apr 20, 2026

View reviewed changes

mahpatil force-pushed the feat/gcp-secret-manager branch from bcefaa7 to 6af66e7 Compare April 24, 2026 08:54

mahpatil added 4 commits April 24, 2026 23:15

Revert "fix: simplify and strengthen FlagCacheCTest concurrent expiry…

8f0cc2c

… test" This reverts commit 58cf2a6. Signed-off-by: Mahesh Patil <maheshfinity@gmail.com>

mahpatil force-pushed the feat/gcp-secret-manager branch from 74e41c0 to 1d65ba5 Compare April 24, 2026 22:18

Conversation

mahpatil commented Apr 9, 2026

Summary

Provider Details

Test plan

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aepfli commented Apr 13, 2026

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrfwow commented Apr 21, 2026

Uh oh!

mahpatil commented Apr 24, 2026

Uh oh!

mahpatil commented Apr 25, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants