feat(gcp): get latest enabled secret by itaispiegel · Pull Request #5131 · external-secrets/external-secrets

itaispiegel · 2025-08-12T11:10:07Z

Problem Statement

When using External Secrets Operator (ESO) with Google Secret Manager (GSM), fetching a secret fails if the secret has multiple versions where the latest version is destroyed but an older version is still enabled.
Example: a secret has versions 1 (enabled) and 2 (destroyed). ESO appears to check both versions/latest and the latest active version; the call against versions/latest fails because that points to the destroyed v2, causing the whole fetch to fail even though v1 is valid.

Related Issue

Fixes #5129

Proposed Changes

Implement GetSecret to fetch latest enabled secret, if the latest is destroyed or disabled.

Checklist

I have read the contribution guidelines
All commits are signed with git commit --signoff
My changes have reasonable test coverage
All tests pass with make test
I ensured my PR is ready for review with make reviewable (wasn't able to run this on my setup 😢)

gusfcarvalho · 2025-08-12T11:32:46Z

This PR introduces an implicit behavior where ESO starts to get random versions based on GCP SM latest version. It was done like that by design to have an explicit control of versions via the version property on data.remoteRef / dataFrom.extract

I would like to discuss it on a community meeting before merging, as this will introduce major breaking changes to users on GCP SM (which is a provider graded as generally available).

gusfcarvalho · 2025-08-12T11:34:17Z

Plus, it also increases the permission scope needed for the provider (which is yet another breaking change).

Skarlso · 2025-08-12T20:45:35Z

It is marked as stable. We cannot break it. Releasing this would require an entirely new version for GCM.

gusfcarvalho · 2025-08-13T11:55:56Z

AS discussed in the community meeting - This is fine to be implemented as long as we have a configuration flag on the SecretStore. This behavior then should only be triggered if the appropriate configuration is set on that SecretStore.

Default behavior should be the current behavior so no breaking changes are introduced.

itaispiegel · 2025-08-13T12:10:42Z

AS discussed in the community meeting - This is fine to be implemented as long as we have a configuration flag on the SecretStore. This behavior then should only be triggered if the appropriate configuration is set on that SecretStore.

Default behavior should be the current behavior so no breaking changes are introduced.

Just added this configuration flag 💪🏻
I'd appreciate it if you could review again.
Let me know if I'm missing anything, and thanks again! 😄

pkg/provider/gcp/secretmanager/client.go

jakobmoellerdev · 2025-08-16T21:04:49Z

apis/externalsecrets/v1/secretstore_gcpsm_types.go

+	// GetLatestEnabledSecret if true, the latest enabled secret version will be fetched
+	// +optional
+	// +default=false
+	GetLatestEnabledSecret bool `json:"getLatestEnabledSecret,omitempty"`


I mean this is very specific behavior, but imo (and also kubernetes API conventions), bools are an antipattern in specs. https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#primitive-types

Think twice about bool fields. Many ideas start as boolean but eventually trend towards a small set of mutually exclusive options. Plan for future expansions by describing the policy options explicitly as a string type alias (e.g. TerminationMessagePolicy).

What else would you suggest to use here to signify that we want to have a toggle to enable new behaviour vs old while the new behaviour actually has no effect API-wise?

I guess my entire point is that the behavior is not a toggle but a behavior "policy", so we could change this to:

// SecretVersionSelectionPolicy specifies how the provider selects a secret version // when "latest" is disabled or destroyed. // // +kubebuilder:validation:Enum=LatestOrFail;LatestOrFetch // +default=LatestOrFail type SecretVersionSelectionPolicy string const ( // SecretVersionSelectionPolicyLatestOrFail means the provider always uses "latest", or fails if that version is disabled/destroyed. SecretVersionSelectionPolicyLatestOrFail SecretVersionSelectionPolicy = "LatestOrFail" // SecretVersionSelectionPolicyLatestOrFetch behaves like SecretVersionSelectionPolicyLatestOrFail but falls back to fetching the latest version if the version is DESTROYED or DISABLED SecretVersionSelectionPolicyLatestOrFetch SecretVersionSelectionPolicy = "LatestOrFetch" )

that way we can add other behaviors later if desired or we notice other behaviors we want to add.

But again, its very specific so I dont think this is a must have.

Interesting. Hm, I like the simplicity of the bool, but I also like the policy. :D I leave it up to you @itaispiegel.

I believe a Policy is indeed better here. Who knows when an user will want to always fetch the second-to-last-counting-backwards-valid secret? 😆

:D Alright. Let's move with that then. :)

I think it makes sense, so just updated the PR accordingly with the new field you suggested 😄

jakobmoellerdev · 2025-08-30T19:23:53Z

@itaispiegel do you mind incorporating the latest changes discussed in the maintainer review? Thanks so much!

itaispiegel · 2025-08-30T19:36:34Z

@itaispiegel do you mind incorporating the latest changes discussed in the maintainer review? Thanks so much!

Sure thing! I haven’t had the chance to get to it yet, but I’ll aim to take care of it this week 🙏🏻

itaispiegel · 2025-09-14T10:09:34Z

Hi, I finally found some time to address the review 🙏🏻
I'd appreciate if you can give another look.
A current gap I was unable to overcome is with the unit tests, which I explained more about here.
What's your opinion on this?
If necessary I can hop on the next Bi-weekly meeting where we can discuss this in more detail.

Skarlso · 2025-09-14T13:18:08Z

Just run make check-diff and commit the diff. :)

itaispiegel · 2025-09-14T15:57:15Z

Just run make check-diff and commit the diff. :)

Oops, I didn't notice 😅
Just fixed 🙏🏻

Skarlso · 2025-09-14T18:01:06Z

Damn, yeah. It's super hard to mock the iterator. What a nightmare.

itaispiegel · 2025-09-25T08:05:52Z

@Skarlso @gusfcarvalho @jakobmoellerdev hey guys, what's required so we can merge this PR? 😄

Skarlso · 2025-09-25T08:17:52Z

@itaispiegel Ah sorry, did you do the thing then in the comment? All good, can we check again? :)

itaispiegel · 2025-09-25T08:29:53Z

@itaispiegel Ah sorry, did you do the thing then in the comment? All good, can we check again? :)

Yes, I addressed the comment regarding the policy configuration field. The only thing I hadn't addressed is implementing unit tests. As I've explained in an earlier comment, this is a bit too complex at this stage, as Google doesn't provide a convenient method to mock the secret iterator.

Skarlso · 2025-09-25T08:31:09Z

Thanks! Will take a look as soon as I can. :)

pkg/provider/gcp/secretmanager/client.go

Skarlso · 2025-09-25T16:15:23Z

/ok-to-test sha=b1e2c7268360a747529810e62006ff9201f379eb

eso-service-account-app · 2025-09-25T16:34:31Z

[Bot] - ✅ e2e for b1e2c7268360a747529810e62006ff9201f379eb passed

Signed-off-by: Itai Spiegel <itai.spiegel@gmail.com>

Skarlso · 2025-09-26T11:21:31Z

@itaispiegel Ah, could you sign the commit please? Then I can approve and this is good to go. :)

Signed-off-by: Itai Spiegel <itai.spiegel@gmail.com>

itaispiegel · 2025-09-26T11:22:57Z

@itaispiegel Ah, could you sign the commit please? Then I can approve and this is good to go. :)

You're right, I forgot to sign the commit!
Just amended it and pushed again 😄

sonarqubecloud · 2025-09-26T11:23:25Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

* Get latest enabled secret Signed-off-by: Itai Spiegel <itai.spiegel@gmail.com> * Add a feature flag to control the new behavior Signed-off-by: Itai Spiegel <itai.spiegel@gmail.com> * Simplify conditions Signed-off-by: Itai Spiegel <itai.spiegel@gmail.com> * Replace boolean field with string field Signed-off-by: Itai Spiegel <itai.spiegel@gmail.com> * Add test for LatestOrFail policy Signed-off-by: Itai Spiegel <itai.spiegel@gmail.com> * Fix golangci Signed-off-by: Itai Spiegel <itai.spiegel@gmail.com> * Fix CI Signed-off-by: Itai Spiegel <itai.spiegel@gmail.com> * Fix metrics.ObserveAPICall Signed-off-by: Itai Spiegel <itai.spiegel@gmail.com> --------- Signed-off-by: Itai Spiegel <itai.spiegel@gmail.com> Signed-off-by: Samuel Molling <samuelmolling@gmail.com>

itaispiegel requested a review from a team as a code owner August 12, 2025 11:10

itaispiegel requested a review from Skarlso August 12, 2025 11:10

gusfcarvalho added halted This Issue / Pull Request is halted from review / action until the blocking points are solved discuss-community-meeting This was labeled by a maintainer asking you to join the next community meeting for clarification labels Aug 12, 2025

gusfcarvalho removed discuss-community-meeting This was labeled by a maintainer asking you to join the next community meeting for clarification halted This Issue / Pull Request is halted from review / action until the blocking points are solved labels Aug 13, 2025

Skarlso reviewed Aug 14, 2025

View reviewed changes

pkg/provider/gcp/secretmanager/client.go Outdated Show resolved Hide resolved

itaispiegel force-pushed the feat/gsm-latest-active-version branch 2 times, most recently from 240f381 to 1bf9714 Compare August 16, 2025 06:12

itaispiegel requested a review from Skarlso August 16, 2025 06:12

Skarlso reviewed Aug 16, 2025

View reviewed changes

pkg/provider/gcp/secretmanager/client.go Outdated Show resolved Hide resolved

jakobmoellerdev reviewed Aug 16, 2025

View reviewed changes

Skarlso mentioned this pull request Aug 20, 2025

Interim maintainer admission: Jakob Moeller #5164

Closed

Skarlso changed the title ~~Get latest enabled secret~~ feat(gcp): get latest enabled secret Sep 4, 2025

github-project-automation bot added this to External Secrets Sep 4, 2025

github-actions bot added area/gcp Issues / Pull Requests related to gcp provider kind/feature Categorizes issue or PR as related to a new feature. kind/documentation Categorizes issue or PR as related to documentation. labels Sep 4, 2025

Skarlso moved this to Waiting for External in External Secrets Sep 4, 2025

github-actions bot added the size/s label Sep 4, 2025

github-actions bot added the size/m label Sep 14, 2025

itaispiegel force-pushed the feat/gsm-latest-active-version branch from 1e7346c to 650348f Compare September 14, 2025 10:02

itaispiegel force-pushed the feat/gsm-latest-active-version branch from 64765ae to 18f0d91 Compare September 14, 2025 15:48

itaispiegel force-pushed the feat/gsm-latest-active-version branch from c1144d5 to b1e2c72 Compare September 25, 2025 08:05

Skarlso reviewed Sep 25, 2025

View reviewed changes

pkg/provider/gcp/secretmanager/client.go Outdated Show resolved Hide resolved

itaispiegel added 7 commits September 26, 2025 14:12

Get latest enabled secret

75c058d

Signed-off-by: Itai Spiegel <itai.spiegel@gmail.com>

Add a feature flag to control the new behavior

5389295

Signed-off-by: Itai Spiegel <itai.spiegel@gmail.com>

Simplify conditions

9fe5053

Signed-off-by: Itai Spiegel <itai.spiegel@gmail.com>

Replace boolean field with string field

1ae1c57

Signed-off-by: Itai Spiegel <itai.spiegel@gmail.com>

Add test for LatestOrFail policy

5d353e3

Signed-off-by: Itai Spiegel <itai.spiegel@gmail.com>

Fix golangci

03dbdb8

Signed-off-by: Itai Spiegel <itai.spiegel@gmail.com>

Fix CI

7620565

Signed-off-by: Itai Spiegel <itai.spiegel@gmail.com>

itaispiegel force-pushed the feat/gsm-latest-active-version branch from b1e2c72 to 7620565 Compare September 26, 2025 11:12

Fix metrics.ObserveAPICall

e9c0218

Signed-off-by: Itai Spiegel <itai.spiegel@gmail.com>

itaispiegel force-pushed the feat/gsm-latest-active-version branch from 0e53411 to e9c0218 Compare September 26, 2025 11:22

Skarlso approved these changes Sep 26, 2025

View reviewed changes

Skarlso merged commit bd62d5b into external-secrets:main Sep 26, 2025
30 checks passed

github-project-automation bot moved this from Waiting for External to Done in External Secrets Sep 26, 2025

Uh oh!

Conversation

itaispiegel commented Aug 12, 2025

Problem Statement

Related Issue

Proposed Changes

Checklist

Uh oh!

gusfcarvalho commented Aug 12, 2025

Uh oh!

gusfcarvalho commented Aug 12, 2025

Uh oh!

Skarlso commented Aug 12, 2025

Uh oh!

gusfcarvalho commented Aug 13, 2025

Uh oh!

itaispiegel commented Aug 13, 2025

Uh oh!

Uh oh!

Uh oh!

jakobmoellerdev Aug 16, 2025

Choose a reason for hiding this comment

Uh oh!

Skarlso Aug 17, 2025

Choose a reason for hiding this comment

Uh oh!

jakobmoellerdev Aug 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Skarlso Aug 17, 2025

Choose a reason for hiding this comment

Uh oh!

gusfcarvalho Aug 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Skarlso Aug 18, 2025

Choose a reason for hiding this comment

Uh oh!

itaispiegel Sep 14, 2025

Choose a reason for hiding this comment

Uh oh!

jakobmoellerdev commented Aug 30, 2025

Uh oh!

itaispiegel commented Aug 30, 2025

Uh oh!

itaispiegel commented Sep 14, 2025

Uh oh!

Skarlso commented Sep 14, 2025

Uh oh!

itaispiegel commented Sep 14, 2025

Uh oh!

Skarlso commented Sep 14, 2025

Uh oh!

itaispiegel commented Sep 25, 2025

Uh oh!

Skarlso commented Sep 25, 2025

Uh oh!

itaispiegel commented Sep 25, 2025

Uh oh!

Skarlso commented Sep 25, 2025

Uh oh!

Uh oh!

Skarlso commented Sep 25, 2025

Uh oh!

eso-service-account-app bot commented Sep 25, 2025

Uh oh!

Skarlso commented Sep 26, 2025

Uh oh!

itaispiegel commented Sep 26, 2025

Uh oh!

sonarqubecloud bot commented Sep 26, 2025

Quality Gate passed

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

jakobmoellerdev Aug 17, 2025 •

edited

Loading

gusfcarvalho Aug 18, 2025 •

edited

Loading