fix(gcp): check for secret version exists in PushSecret by bpalko · Pull Request #5593 · external-secrets/external-secrets

bpalko · 2025-11-16T02:12:08Z

Related Issue

Problem Statement

The GCP PushSecret controller has a race condition. When creating secrets, the controller calls CreateSecret() followed by AddSecretVersion(). If something interrupts between these calls (timeout, rate limit, etc.), you end up with a secret in GCP that has no versions. From here, the secret is considered created, yet isn't functional with no data. The goal of this PR is to address this by expanding the reconcilation check.

Proposed Changes

I've modified SecretExists() to check both:

That the secret resource exists
At least one version exists. In this case, I'm just using latest.

If the secret exists but has no versions, SecretExists() now returns false. This allows for a retry and the ability to complete the push.

How I tested this

I started by reproducing the race condition locally. I used the GCP API directly to create a secret without a version, then verified SecretExists() returns false. Then, I wanted to test the new logic, so I went and added a version to confirm it returns true.

I also ran the full PushSecret flow on a local Kind cluster with GCP Secret Manager integration. I created multiple secrets, updated them, verified sync status matches actual state in GCP. I had a few debug logs within the SecretExists() function for tracing to ensure each step and if checks were hit.

I added unit tests covering:

Secret with version exists
Secret exists but no version
Secret doesn't exist
Unexpected error handling for both API calls

Checklist

[x ] I have read the contribution guidelines
[x ] All commits are signed with git commit --signoff
[x ] My changes have reasonable test coverage
[x ] All tests pass with make test
[x ] I ensured my PR is ready for review with make reviewable

Signed-off-by: Brendan Palkowski <b.palko@outlook.com>

sonarqubecloud · 2025-11-16T02:18:34Z

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

Skarlso · 2025-11-16T07:51:17Z

My problem with this pull request is that it's obviously Claude Code, or some other LLM, generated. This tells me that you didn't investigate the root cause or didn't even bother to verify / test your fixes just committed whatever the LLM spewed out.

I'm not against using LLMs, but I am against trusting it blindly.

Before I even look at this, please update your PR description to be something that you wrote that shows me that you investigated the problem, understood the root cause and the fix makes sense in-light of that.

Thanks.

gusfcarvalho · 2025-11-16T13:50:52Z

I agree this PR description tells very little about what has been committed.

though, I think the implementation here looks okay.

Skarlso · 2025-11-16T14:22:39Z

Sure. Sorry if this came over too cross, but I would like to make sure we/you understand what you're trying to fix/do. And the description doesn't tell that story.

bpalko · 2025-11-16T17:01:09Z

Thanks for the feedback. I'm happy to rewrite the PR description. I did use Claude to help structure the description with the template given; I'll be fully transparent here. I can understand your position as a maintainer trying to review PRs, and I imagine it's difficult to assess the merit of the committer. I recognize that AI-styled wording can sometimes raise red flags for maintainers.

I spent several hours yesterday, first trying to understand the codebase, and tracing the issue described by the user. Second, I wrote local, additional tests that weren't committed here to confirm my understanding and implementation. I’m still getting familiar with the internals of the project, but I feel confident in the implementation I've submitted. My new PR description will reflect this.

My intention in being here is to help your team and project out as I've been using it for years, and feel I can make a meaningful impact here. Tangentially, contributing is accelerating my skills as an engineer. I won't learn vibing a PR through Claude.

AI is an accelerator. It's something that can help with drafting, not a replacement for understanding the root cause or validating a change. I don’t think its use should be dismissed outright or treated exclusively as a sign of poor-quality work. I appreciate your callout, but I do want to emphasize that responsible use of AI tools should be viewed as normal and acceptable as long as the contributor is owning the work behind it.

gusfcarvalho · 2025-11-16T18:05:30Z

Thanks a lot for clarifying your point of view @bpalko! It really means a lot (and it is way more than most AI slop contributions we’ve had)

I’ll write something up related to some contribution guidelines with AI and suggest as a policy to the other maintainers 🙂 I think it is fair as a user to try to use whatever the tools you have available. We just need to define what “good” looks like for both sides here

Skarlso · 2025-11-16T18:25:19Z

Thank you very much from me too! Really appreciate it! :)

bpalko · 2025-11-16T18:53:45Z

Thanks again! Please let me know if you need anything else from me to get this over the line.

Skarlso · 2025-11-16T19:38:26Z

I'll take a closer look later today. :)

Skarlso · 2025-11-17T06:53:31Z

Thank you for taking the effort of the description update again. I really appreciate it. 🙇

tosih · 2025-11-17T17:56:10Z

Oops, I had already made the PR for this. I meant to post it today as I didn't expect someone to pick this up so fast.

My main difference was that the GetSecret was an extraneous call. We only really need to grab the latest enabled secret version for SecretExists. This code has an additional call to GCP, which I would say is less efficient.

tosih · 2025-11-17T17:58:48Z

@Skarlso would you mind if I posted a PR to remove that extra call and grab the latest enabled secret version?

Also, thanks for your work @bpalko

Skarlso · 2025-11-17T18:38:02Z

Sure. Go for it!

This PR contains the following updates: | Package | Update | Change | |---|---|---| | [external-secrets](https://github.com/external-secrets/external-secrets) | minor | `1.0.0` -> `1.1.0` | --- ### Release Notes <details> <summary>external-secrets/external-secrets (external-secrets)</summary> ### [`v1.1.0`](https://github.com/external-secrets/external-secrets/releases/tag/v1.1.0) [Compare Source](external-secrets/external-secrets@v1.0.0...v1.1.0) Image: `ghcr.io/external-secrets/external-secrets:v1.1.0` Image: `ghcr.io/external-secrets/external-secrets:v1.1.0-ubi` Image: `ghcr.io/external-secrets/external-secrets:v1.1.0-ubi-boringssl`  #### What's Changed !*NOTE*!: During last community meeting we discussed that we are retiring our scarf account. With that, we will be changing back to [ghcr.io/external-secrets/external-secrets](http://ghcr.io/external-secrets/external-secrets) instead of [oci.external-secrets.io/external-secrets/external-secrets](http://oci.external-secrets.io/external-secrets/external-secrets). For now, the old domain will live for a couple months to give people to change back. With this release , the helm chart switched back to ghcr. ##### General - chore(chart): release helm chart 1.0.0 by [@Skarlso](https://github.com/Skarlso) in [#5552](external-secrets/external-secrets#5552) - feat(security): add support for ECDSA ssh keys by [@bigjazzsound](https://github.com/bigjazzsound) in [#5559](external-secrets/external-secrets#5559) - fix: minor typo in comment of KeeperSecurity example by [@mdjong1](https://github.com/mdjong1) in [#5573](external-secrets/external-secrets#5573) - docs(gcp): update documentation for using WorkloadIdentityFederation in non-GKE cluster by [@jennweir](https://github.com/jennweir) in [#5556](external-secrets/external-secrets#5556) - chore(release): add darwin\_arm64 releases by [@lbordowitz](https://github.com/lbordowitz) in [#5583](external-secrets/external-secrets#5583) - feat: support override IAM endpoint in IBM provider for APIkey auth by [@fidel-ruiz](https://github.com/fidel-ruiz) in [#5550](external-secrets/external-secrets#5550) - feat(security): build tags for all the providers to disable them on d… by [@ShimonDarshan](https://github.com/ShimonDarshan) in [#5578](external-secrets/external-secrets#5578) - fix: do not include the last element of the path in the iteration by [@Skarlso](https://github.com/Skarlso) in [#5588](external-secrets/external-secrets#5588) - fix(k8s): support deleting whole secret by [@tiagolobocastro](https://github.com/tiagolobocastro) in [#5538](external-secrets/external-secrets#5538) - fix(provider): configure TLS for secret server provider by [@Lumexralph](https://github.com/Lumexralph) in [#5558](external-secrets/external-secrets#5558) - chore(aws): remove any usage of aws-sdk-v1 by [@Skarlso](https://github.com/Skarlso) in [#5590](external-secrets/external-secrets#5590) - fix(gcp): check for secret version exists in PushSecret by [@bpalko](https://github.com/bpalko) in [#5593](external-secrets/external-secrets#5593) - feat(vault): add GCP Workload Identity authentication support by [@SamuelMolling](https://github.com/SamuelMolling) in [#5356](external-secrets/external-secrets#5356) - chore: fix sonar cloud issues by [@Skarlso](https://github.com/Skarlso) in [#5405](external-secrets/external-secrets#5405) - chore(aws-sdk-v2): update dependencies to accept new aws regions by [@damienpuig](https://github.com/damienpuig) in [#5577](external-secrets/external-secrets#5577) - feat(chart): use ghcr.io instead of our own domain by [@evrardjp](https://github.com/evrardjp) in [#5617](external-secrets/external-secrets#5617) ##### Dependencies - chore(deps): bump golang from 1.25.3 to 1.25.4 by [@dependabot](https://github.com/dependabot)\[bot] in [#5560](external-secrets/external-secrets#5560) - chore(deps): bump golang from 1.25.3-bookworm to 1.25.4-bookworm in /e2e by [@dependabot](https://github.com/dependabot)\[bot] in [#5568](external-secrets/external-secrets#5568) - chore(deps): bump step-security/harden-runner from 2.13.1 to 2.13.2 by [@dependabot](https://github.com/dependabot)\[bot] in [#5561](external-secrets/external-secrets#5561) - chore(deps): bump softprops/action-gh-release from 2.4.1 to 2.4.2 by [@dependabot](https://github.com/dependabot)\[bot] in [#5564](external-secrets/external-secrets#5564) - chore(deps): bump helm/chart-testing-action from 2.7.0 to 2.8.0 by [@dependabot](https://github.com/dependabot)\[bot] in [#5565](external-secrets/external-secrets#5565) - chore(deps): bump aws-actions/configure-aws-credentials from [`0d00a56`](external-secrets/external-secrets@0d00a56) to [`2475ef7`](external-secrets/external-secrets@2475ef7) by [@dependabot](https://github.com/dependabot)\[bot] in [#5562](external-secrets/external-secrets#5562) - chore(deps): bump helm/kind-action from 1.12.0 to 1.13.0 by [@dependabot](https://github.com/dependabot)\[bot] in [#5563](external-secrets/external-secrets#5563) - chore(deps): bump docker/setup-qemu-action from 3.6.0 to 3.7.0 by [@dependabot](https://github.com/dependabot)\[bot] in [#5567](external-secrets/external-secrets#5567) - chore(deps): bump regex from 2025.10.23 to 2025.11.3 in /hack/api-docs by [@dependabot](https://github.com/dependabot)\[bot] in [#5570](external-secrets/external-secrets#5570) - chore(deps): bump markdown from 3.9 to 3.10 in /hack/api-docs by [@dependabot](https://github.com/dependabot)\[bot] in [#5569](external-secrets/external-secrets#5569) - chore(deps): bump hashicorp/setup-terraform from [`982f6f0`](external-secrets/external-secrets@982f6f0) to [`4c5fdab`](external-secrets/external-secrets@4c5fdab) by [@dependabot](https://github.com/dependabot)\[bot] in [#5566](external-secrets/external-secrets#5566) - chore(deps): bump golang from `d3f0cf7` to `d3f0cf7` by [@dependabot](https://github.com/dependabot)\[bot] in [#5595](external-secrets/external-secrets#5595) - chore(deps): bump github/codeql-action from 4.31.2 to 4.31.3 by [@dependabot](https://github.com/dependabot)\[bot] in [#5596](external-secrets/external-secrets#5596) - chore(deps): bump click from 8.3.0 to 8.3.1 in /hack/api-docs by [@dependabot](https://github.com/dependabot)\[bot] in [#5602](external-secrets/external-secrets#5602) - chore(deps): bump ubi9/ubi from `dec374e` to `dcd8128` by [@dependabot](https://github.com/dependabot)\[bot] in [#5594](external-secrets/external-secrets#5594) - chore(deps): bump aws-actions/configure-aws-credentials from [`2475ef7`](external-secrets/external-secrets@2475ef7) to [`f2964c7`](external-secrets/external-secrets@f2964c7) by [@dependabot](https://github.com/dependabot)\[bot] in [#5597](external-secrets/external-secrets#5597) - chore(deps): bump actions/dependency-review-action from 4.8.1 to 4.8.2 by [@dependabot](https://github.com/dependabot)\[bot] in [#5598](external-secrets/external-secrets#5598) - chore(deps): bump pymdown-extensions from 10.16.1 to 10.17.1 in /hack/api-docs by [@dependabot](https://github.com/dependabot)\[bot] in [#5599](external-secrets/external-secrets#5599) - chore(deps): bump certifi from 2025.10.5 to 2025.11.12 in /hack/api-docs by [@dependabot](https://github.com/dependabot)\[bot] in [#5600](external-secrets/external-secrets#5600) - chore(deps): bump mkdocs-material from 9.6.23 to 9.7.0 in /hack/api-docs by [@dependabot](https://github.com/dependabot)\[bot] in [#5601](external-secrets/external-secrets#5601) - chore(deps): bump mkdocs-macros-plugin from 1.4.1 to 1.5.0 in /hack/api-docs by [@dependabot](https://github.com/dependabot)\[bot] in [#5603](external-secrets/external-secrets#5603) #### New Contributors - [@bigjazzsound](https://github.com/bigjazzsound) made their first contribution in [#5559](external-secrets/external-secrets#5559) - [@mdjong1](https://github.com/mdjong1) made their first contribution in [#5573](external-secrets/external-secrets#5573) - [@jennweir](https://github.com/jennweir) made their first contribution in [#5556](external-secrets/external-secrets#5556) - [@lbordowitz](https://github.com/lbordowitz) made their first contribution in [#5583](external-secrets/external-secrets#5583) - [@fidel-ruiz](https://github.com/fidel-ruiz) made their first contribution in [#5550](external-secrets/external-secrets#5550) - [@ShimonDarshan](https://github.com/ShimonDarshan) made their first contribution in [#5578](external-secrets/external-secrets#5578) - [@bpalko](https://github.com/bpalko) made their first contribution in [#5593](external-secrets/external-secrets#5593) - [@SamuelMolling](https://github.com/SamuelMolling) made their first contribution in [#5356](external-secrets/external-secrets#5356) - [@damienpuig](https://github.com/damienpuig) made their first contribution in [#5577](external-secrets/external-secrets#5577) **Full Changelog**: <external-secrets/external-secrets@v1.0.0...v1.1.0> </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR is behind base branch, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).  Reviewed-on: https://gitea.alexlebens.dev/alexlebens/infrastructure/pulls/2081 Co-authored-by: Renovate Bot <renovate-bot@alexlebens.net> Co-committed-by: Renovate Bot <renovate-bot@alexlebens.net>

…rets#5593) Signed-off-by: Brendan Palkowski <b.palko@outlook.com> Signed-off-by: Alvin Wong <alvin.wong@forgerock.com>

github-project-automation bot added this to External Secrets Nov 16, 2025

github-actions bot added area/gcp Issues / Pull Requests related to gcp provider kind/bug Categorizes issue or PR as related to a bug. size/s labels Nov 16, 2025

bpalko force-pushed the fix-5584-gcp-secret-version branch from cec6883 to 3c2275e Compare November 16, 2025 02:17

fix(gcp): check for secret version exists in PushSecret

3d72cfb

Signed-off-by: Brendan Palkowski <b.palko@outlook.com>

bpalko force-pushed the fix-5584-gcp-secret-version branch from 3c2275e to 3d72cfb Compare November 16, 2025 02:17

bpalko marked this pull request as ready for review November 16, 2025 02:18

bpalko mentioned this pull request Nov 16, 2025

PushSecret: GCPSM Provider reconcile checks for secret existence but not also a secret version existence #5584

Closed

Skarlso approved these changes Nov 17, 2025

View reviewed changes

Skarlso merged commit a70586a into external-secrets:main Nov 17, 2025
37 checks passed

github-project-automation bot moved this to Done in External Secrets Nov 17, 2025

This was referenced Nov 18, 2025

GCP PushSecret - Look for latest enabled secret version and reduce extraneous GetSecret call #5609

Open

fix(gcpsm): Improve SecretExists method in GCP secret manager provider #5610

Merged

Uh oh!

Conversation

bpalko commented Nov 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Related Issue

Problem Statement

Proposed Changes

How I tested this

Checklist

Uh oh!

sonarqubecloud bot commented Nov 16, 2025

Quality Gate passed

Uh oh!

Skarlso commented Nov 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gusfcarvalho commented Nov 16, 2025

Uh oh!

Skarlso commented Nov 16, 2025

Uh oh!

bpalko commented Nov 16, 2025

Uh oh!

gusfcarvalho commented Nov 16, 2025

Uh oh!

Skarlso commented Nov 16, 2025

Uh oh!

bpalko commented Nov 16, 2025

Uh oh!

Skarlso commented Nov 16, 2025

Uh oh!

Uh oh!

Skarlso commented Nov 17, 2025

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

Skarlso commented Nov 17, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

bpalko commented Nov 16, 2025 •

edited

Loading

Skarlso commented Nov 16, 2025 •

edited

Loading

tosih commented Nov 17, 2025 •

edited

Loading

tosih commented Nov 17, 2025 •

edited

Loading