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-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

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

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

mahpatil · 2026-04-27T14:16:16Z

Good catch — removed the duplicate now, clock, and cache setup from concurrentExpiryAndInsertDoNotLoseNewEntry and switched to the T0/T1 constants and instance fields already initialised in @BeforeEach.

toddbaert · 2026-04-30T15:02:54Z

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.

The two PRs duplicate ~80% of code (FlagCache, FlagValueConverter, the provider scaffolding, tests). They're already drifting (Clock injection, STATIC vs CACHED, exception cause, log4j versions).
The GCP client deps are technically different artifacts, but their transitive trees overlap heavily (gax, gRPC, protobuf, auth, etc.); the incremental footprint of having both on the classpath is small.
One module = one CHANGELOG, one release, one set of internals to maintain. Two modules = ongoing duplication tax. A third gcp-common module is overkill for what's effectively one shared FlagCache.

@mahpatil - to keep things reviewable, suggest stacking the branches: rework this PR (#1772) to add a single providers/gcp module with pom.xml, shared internals (FlagCache, FlagValueConverter), and the Secret Manager provider. Then rebase #1771 on top, so it only adds the Parameter Manager provider on top of the shared scaffolding. SM-first makes sense since this PR is closer to approval. That way each PR stays small and focused, and the shared scaffolding only shows up in the diff once.

Suggested module name: providers/gcp (artifact gcp). Short, accurate, and leaves room if other GCP-backed providers ever land later. gcp-secret-and-parameter-manager works too but is mouthful-y. WDYT?

@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.

mahpatil · 2026-05-01T06:07:48Z

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.

The two PRs duplicate ~80% of code (FlagCache, FlagValueConverter, the provider scaffolding, tests). They're already drifting (Clock injection, STATIC vs CACHED, exception cause, log4j versions).

The GCP client deps are technically different artifacts, but their transitive trees overlap heavily (gax, gRPC, protobuf, auth, etc.); the incremental footprint of having both on the classpath is small.

One module = one CHANGELOG, one release, one set of internals to maintain. Two modules = ongoing duplication tax. A third gcp-common module is overkill for what's effectively one shared FlagCache.

@mahpatil - to keep things reviewable, suggest stacking the branches: rework this PR (#1772) to add a single providers/gcp module with pom.xml, shared internals (FlagCache, FlagValueConverter), and the Secret Manager provider. Then rebase #1771 on top, so it only adds the Parameter Manager provider on top of the shared scaffolding. SM-first makes sense since this PR is closer to approval. That way each PR stays small and focused, and the shared scaffolding only shows up in the diff once.

Suggested module name: providers/gcp (artifact gcp). Short, accurate, and leaves room if other GCP-backed providers ever land later. gcp-secret-and-parameter-manager works too but is mouthful-y. WDYT?

@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.

This is a bit frustrating as I have been receiving feedback on the PR and now requested to refactor again. @aepfli please finalize with @chrfwow what you would want, I don't want to be keep coming back and incorporating comments, wasting everyones time. I feel the process very slow even though we are using gemini and im using claude, this PR has been going back and forth for a while.
Please provide concrete steps that I can incorporate quickly so we can merge this PR.

aepfli · 2026-05-01T06:52:20Z

@mahpatil first of all, I want to apologise. Reading back through the thread, I can absolutely see how
my original comment landed the way it did, and the frustration you are voicing is completely fair. You
have been responsive, you have been thorough, and the last thing I want is for a contributor putting in
this kind of effort to feel that the goalposts keep moving. That is on me for not being clearer the
first time, and I am sorry for the churn it has caused.

Let me try to clear up the misunderstanding, because I think @toddbaert read my intent correctly. When I
asked you to split the work, I was talking about splitting the pull request for review hygiene —
keeping each PR small and focused on one feature so that we as maintainers can evaluate it properly. I
was not asking for two separate Maven modules or two separate artifacts. That part was an interpretation
on top of my ask, and I should have spelled it out at the time. The duplication that has surfaced
between #1771 and #1772 is exactly the kind of thing the "one module" approach avoids.

The technique that makes "split the PR" and "share the code" coexist is what is usually called stacked
diffs (or stacked PRs). The idea is simple: instead of two independent branches off main, you stack
them. PR A branches off main and contains the shared scaffolding plus the first provider. PR B then
branches off PR A's branch (not main) and adds only the second provider on top. Each PR stays small and
reviewable on its own, the shared code only shows up in the diff once, and once PR A merges, PR B's diff
naturally collapses to just its own delta. It is the same review benefit I was originally asking for,
without paying the duplication tax.

Concretely, for your case I think Todd's suggestion is exactly right: rework #1772 to add a single
providers/gcp module containing the shared internals (FlagCache, FlagValueConverter, scaffolding) and
the Secret Manager provider, then rebase #1771 on top of that branch so it only adds the Parameter
Manager provider. SM-first makes sense since this PR is closer to approval anyway.

Again, I am sorry for the back-and-forth — your contribution is genuinely appreciated and we want to
land it. If it would help, I am happy to jump on a quick call or pair on the rebase so we can get this
over the line without another round of comments.

Signed-off-by: Ayushman Gaur <ayushmangaur2017@gmail.com> Signed-off-by: Mahesh Patil <maheshfinity@gmail.com>

…pen-feature#1780) Signed-off-by: OpenFeature Bot <109696520+openfeaturebot@users.noreply.github.com> Signed-off-by: Mahesh Patil <maheshfinity@gmail.com>

- 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>

…/java-sdk-contrib into feat/gcp-secret-manager

mahpatil · 2026-05-12T17:55:54Z

@mahpatil first of all, I want to apologise. Reading back through the thread, I can absolutely see how my original comment landed the way it did, and the frustration you are voicing is completely fair. You have been responsive, you have been thorough, and the last thing I want is for a contributor putting in this kind of effort to feel that the goalposts keep moving. That is on me for not being clearer the first time, and I am sorry for the churn it has caused.

Let me try to clear up the misunderstanding, because I think @toddbaert read my intent correctly. When I asked you to split the work, I was talking about splitting the pull request for review hygiene — keeping each PR small and focused on one feature so that we as maintainers can evaluate it properly. I was not asking for two separate Maven modules or two separate artifacts. That part was an interpretation on top of my ask, and I should have spelled it out at the time. The duplication that has surfaced between #1771 and #1772 is exactly the kind of thing the "one module" approach avoids.

The technique that makes "split the PR" and "share the code" coexist is what is usually called stacked diffs (or stacked PRs). The idea is simple: instead of two independent branches off main, you stack them. PR A branches off main and contains the shared scaffolding plus the first provider. PR B then branches off PR A's branch (not main) and adds only the second provider on top. Each PR stays small and reviewable on its own, the shared code only shows up in the diff once, and once PR A merges, PR B's diff naturally collapses to just its own delta. It is the same review benefit I was originally asking for, without paying the duplication tax.

Concretely, for your case I think Todd's suggestion is exactly right: rework #1772 to add a single providers/gcp module containing the shared internals (FlagCache, FlagValueConverter, scaffolding) and the Secret Manager provider, then rebase #1771 on top of that branch so it only adds the Parameter Manager provider. SM-first makes sense since this PR is closer to approval anyway.

Again, I am sorry for the back-and-forth — your contribution is genuinely appreciated and we want to land it. If it would help, I am happy to jump on a quick call or pair on the rebase so we can get this over the line without another round of comments.

okay thanks for clarifying. I have now consolidated this into single provider for GCP. Do we also want to have single factory and tests to instantiate & test both the providers? Happy to incorporate that here or when merging Parameter manager.

mahpatil · 2026-05-27T08:10:22Z

@chrfwow @aepfli just checking next steps on this? I dont see any responses to my comments or updated PR. Are you still keen to include this contribution?

chrfwow · 2026-05-27T12:32:06Z

The CI fails because there are some formatting issue. You can fix them by running spotless:apply

Signed-off-by: Mahesh Patil <maheshfinity@gmail.com>

mahpatil · 2026-05-27T12:59:08Z

Fixed formatting issues.

…onfig Restore root pom.xml and release-please-config.json to their state on main, keeping only the additions required for providers/gcp: - <module>providers/gcp</module> in pom.xml - new providers/gcp block in release-please-config.json Signed-off-by: Todd Baert <todd.baert@dynatrace.com>

toddbaert

Approved. I have some recommendations that I think are mostly around doc, and one about potentially improving the deps with a BOM ref, but I will leave that up to you. If I don't hear anything in the next 24 hrs, I will merge; otherwise feel free to make those minor fixes or tell me to hold off.

Thanks again for your contribution and patience.

PS: I made a small change to revert whitespace noise in the parent POM.

Co-authored-by: Todd Baert <todd.baert@dynatrace.com> Signed-off-by: Mahesh Patil <17205424+mahpatil@users.noreply.github.com>

…gcp/GcpProviderOptions.java Co-authored-by: Todd Baert <todd.baert@dynatrace.com> Signed-off-by: Mahesh Patil <17205424+mahpatil@users.noreply.github.com>

Co-authored-by: Todd Baert <todd.baert@dynatrace.com> Signed-off-by: Mahesh Patil <17205424+mahpatil@users.noreply.github.com>

mahpatil · 2026-05-28T08:33:22Z

Approved. I have some recommendations that I think are mostly around doc, and one about potentially improving the deps with a BOM ref, but I will leave that up to you. If I don't hear anything in the next 24 hrs, I will merge; otherwise feel free to make those minor fixes or tell me to hold off.

Thanks again for your contribution and patience.

PS: I made a small change to revert whitespace noise in the parent POM.

Thank you @toddbaert, only one outstanding option on use of BOM, will incorporate it as part of parameter mgr, which is coming next. If you can now merge I can start on Parameter mgr which is really the more common usecase for us.

Signed-off-by: Todd Baert <todd.baert@dynatrace.com>

toddbaert · 2026-05-28T13:53:41Z

FYI I added you as a "component_owner" for this, which will add you as assignee/reviewer to PRs in this folder.

Signed-off-by: Ayushman Gaur <ayushmangaur2017@gmail.com> Signed-off-by: Mahesh Patil <maheshfinity@gmail.com> Signed-off-by: OpenFeature Bot <109696520+openfeaturebot@users.noreply.github.com> Signed-off-by: Thomas Poignant <thomas.poignant@gofeatureflag.org> Signed-off-by: Mahesh Patil <17205424+mahpatil@users.noreply.github.com> Signed-off-by: Todd Baert <todd.baert@dynatrace.com> Co-authored-by: Ayushman-Gaur <120575155+Ayushman-Gaur@users.noreply.github.com> Co-authored-by: OpenFeature Bot <109696520+openfeaturebot@users.noreply.github.com> Co-authored-by: Thomas Poignant <thomas.poignant@gofeatureflag.org> Co-authored-by: Todd Baert <todd.baert@dynatrace.com>

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

Closed

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 force-pushed the feat/gcp-secret-manager branch from 74e41c0 to 1d65ba5 Compare April 24, 2026 22:18

chrfwow reviewed Apr 27, 2026

View reviewed changes

Comment thread ...manager/src/test/java/dev/openfeature/contrib/providers/gcpsecretmanager/FlagCacheCTest.java Outdated

mahpatil force-pushed the feat/gcp-secret-manager branch from d01ff47 to ea481a2 Compare April 27, 2026 15:41

toddbaert requested a review from aepfli April 30, 2026 15:02

mahpatil force-pushed the feat/gcp-secret-manager branch from 6078c87 to a5b85cd Compare May 12, 2026 15:27

github-actions Bot assigned thomaspoignant May 12, 2026

github-actions Bot requested a review from thomaspoignant May 12, 2026 16:18

Ayushman-Gaur and others added 7 commits May 12, 2026 17:38

fix: fixing null pointer exception error (open-feature#1709)

c3adbc5

Signed-off-by: Ayushman Gaur <ayushmangaur2017@gmail.com> Signed-off-by: Mahesh Patil <maheshfinity@gmail.com>

chore(main): release dev.openfeature.contrib.tools.flagdcore 1.0.1 (o…

065763f

…pen-feature#1780) Signed-off-by: OpenFeature Bot <109696520+openfeaturebot@users.noreply.github.com> Signed-off-by: Mahesh Patil <maheshfinity@gmail.com>

Revert "fix: simplify and strengthen FlagCacheCTest concurrent expiry…

f73f21c

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

Merge branch 'feat/gcp-secret-manager' of https://github.com/mahpatil…

a3e904c

…/java-sdk-contrib into feat/gcp-secret-manager

chrfwow reviewed May 13, 2026

View reviewed changes

Comment thread providers/gcp/src/main/java/dev/openfeature/contrib/providers/gcp/GcpSecretManagerProvider.java

Merge branch 'main' into feat/gcp-secret-manager

0d8a293

chrfwow approved these changes May 27, 2026

View reviewed changes

Fix formatting issues on gcp provider using spotless apply

38b8739

Signed-off-by: Mahesh Patil <maheshfinity@gmail.com>