fix: add missing metrics and fundamentally fix the caching logic#5894
fix: add missing metrics and fundamentally fix the caching logic#5894Skarlso merged 12 commits intoexternal-secrets:mainfrom
Conversation
Signed-off-by: Gergely Brautigam <182850+Skarlso@users.noreply.github.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughRenamed exported client type to Changes
Possibly related PRs
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
I can test this in my cluster see if it helps. |
|
I do see the new metrics, so that part is good. I am not sure the fixes to caching, and the cache itself, is helping quite enough. I think one problem is if you have a large number of secrets and you're starting from scratch, you're going to have a thundering herd where the controller will try to reconcile everything and start failing very quickly. Hopefully if the cache TTL is large enough, when the quota resets, then another batch of secrets will sync and those will have an "offset" cache freshness, and so on until a staggered update schedule naturally establishes itself. I haven't quite landed on a large enough cache TTL yet, but I'm already at 36 hours. |
|
@jfroy Yeah gotcha, I understand the problem of the initial large amount of secrets, but... like, realistically, I don't have a solution for that, right? I mean, we can't throttle the controller really, and 1Password is clearly going for the not too large limit for the API requests. |
|
Looking at the metrics, it is doing a lot of |
A per-secret-store reconciliation rate limit field could be added to the CRDs and implemented in the controller, but hopefully a judicious configuration of the cache TTL and size should hopefully settle into a working pattern. |
Hm, there IS a client cache in the controller. 🤔 But I wonder if that needs to be configured or works correctly. |
|
BTW, I wonder if this is also influencing that problem #5921 While I doubt that, I'll incorporate this in this Pr as well. |
Signed-off-by: Gergely Brautigam <182850+Skarlso@users.noreply.github.com>
|
Yeah, the client construction on each reconcile might have been because of the pointer override. 🤔 I wonder if this is the actual underlying cause of the API limit problem. I fixed the client I would appreciate if you could retest it @jfroy. And thanks so much for the effort! |
I've deployed the latest patches and will be testing when my quota resets. |
|
Thanks! |
|
I still ran out of quota pretty quickly. Below is the entire log since my quota reset. You can see there are still a lot of clients being created (330 times), with client reuse only happening 78 times. Store validation, which happens every 5 minutes by default, seems to always create a new client. |
|
Seems like the This makes the onepasswordsdk cache, as implemented, pretty useless? |
|
Hm, the client should be per store. Not per ExternalSecret. 🤔 |
|
Yah, I see what you're saying, let me dig into this a bit. |
|
My understanding was that the cache is persisted somewhere somehow. :D Maybe I'm missing something but I don't see how that would work. :/ That said, making it persist raises so many concerns like thread safety of the client. Now every provider needs to be thread safe. Stale creds, mutliple ES sharing the same client might be a security risk, yada yada. Maybe if we change this it needs to be an experimental flag. 🤔 I'll have to talk about this with the team. |
|
We talked about this, and yes that's how it works. We have a plan to rewrite things so persists, but that's going to take some time I'm afraid. :/ |
Thanks for the update. I'll revert back to the 1p-connect backend for now, but keep an eye on this. |
|
Thanks! Sorry for the trouble. |
|
@jfroy ACTUALLY! I might be completely wrong. I just found that we have a global builder tag that caches the provider. There might still be hope here!! |
|
@jfroy Dude, I'm an idiot. I misunderstood how the cache is working and I was flushing the f*cking cache on every |
|
@gusfcarvalho Yeah I think I'm going to do that. |
|
Okay, I shifted the cache into the provider and the key is the same as doppler which is the resource that would include the vault id. Hopefully this will actually significantly reduce the API calls @jfroy ❤️ thank you so much for testing! |
Signed-off-by: Gergely Brautigam <182850+Skarlso@users.noreply.github.com> On-behalf-of: Gergely Brautigam <gergely.brautigam@sap.com>
339f8d2 to
aeb7463
Compare
|
Deploying the latest patch and testing today. |
|
Please note my update in #5894 (comment). I am sorry for the confusion. I made sure to clean and rebuild the Go programs this time. |
|
Oh dang. :D Well, even so. Let's see what this does. :D |
|
Alright! That looks a LOT better 🎉 Let's see if it can handle multiple stores? :D |
|
@jfroy Okay, so can you give this other one a spin then please? :D mostly, because I changed alot around how the caching works again after seeing your initial report. :D |
“This” other one? I’ve been running what I think is the latest (HEAD aeb7463, rebased on v2.0.1) and it’s working as hoped, no issue with rate limits. |
|
I changed it in this one aeb7463 |
|
Oh great! That looks to be okay then :) Yaaay! Thank you so much! :) |
moolen
left a comment
There was a problem hiding this comment.
found a small edge case, i think this should be addressed.
Signed-off-by: Gergely Brautigam <182850+Skarlso@users.noreply.github.com>
|
|
@moolen Should be addressed now. :) |
…2.1.0 (#4491) This PR contains the following updates: | Package | Update | Change | |---|---|---| | [external-secrets/external-secrets](https://github.com/external-secrets/external-secrets) | minor | `v2.0.1` → `v2.1.0` | --- ### Release Notes <details> <summary>external-secrets/external-secrets (external-secrets/external-secrets)</summary> ### [`v2.1.0`](https://github.com/external-secrets/external-secrets/releases/tag/v2.1.0) [Compare Source](external-secrets/external-secrets@v2.0.1...v2.1.0) Image: `ghcr.io/external-secrets/external-secrets:v2.1.0` Image: `ghcr.io/external-secrets/external-secrets:v2.1.0-ubi` Image: `ghcr.io/external-secrets/external-secrets:v2.1.0-ubi-boringssl` <!-- Release notes generated using configuration in .github/release.yml at main --> #### What's Changed ##### General - chore(release): Update helm chart by [@​evrardj-roche](https://github.com/evrardj-roche) in [#​5981](external-secrets/external-secrets#5981) - fix: cosign verify does not use signing config by [@​gusfcarvalho](https://github.com/gusfcarvalho) in [#​5982](external-secrets/external-secrets#5982) - docs: Update release process by [@​evrardj-roche](https://github.com/evrardj-roche) in [#​5980](external-secrets/external-secrets#5980) - fix: allow cross-namespace push with ClusterSecretStore objects by [@​Skarlso](https://github.com/Skarlso) in [#​5998](external-secrets/external-secrets#5998) - feat(charts): add new flag enable leader for cert-manager by [@​nutmos](https://github.com/nutmos) in [#​5863](external-secrets/external-secrets#5863) - feat(kubernetes): fall back to system CA roots when no CA is configured by [@​rajsinghtech](https://github.com/rajsinghtech) in [#​5961](external-secrets/external-secrets#5961) - feat: dedup sbom but keep it monolithic by [@​moolen](https://github.com/moolen) in [#​6004](external-secrets/external-secrets#6004) - fix: add missing metrics and fundamentally fix the caching logic by [@​Skarlso](https://github.com/Skarlso) in [#​5894](external-secrets/external-secrets#5894) - docs: designate Oracle Vault provider as 'stable' by [@​anders-swanson](https://github.com/anders-swanson) in [#​6020](external-secrets/external-secrets#6020) - docs: Oracle Vault provider capabilities by [@​anders-swanson](https://github.com/anders-swanson) in [#​6023](external-secrets/external-secrets#6023) - docs(azurekv): cert-manager pushsecret example and cleanups by [@​illrill](https://github.com/illrill) in [#​5972](external-secrets/external-secrets#5972) - feat(kubernetes): implement SecretExists by [@​Saku2](https://github.com/Saku2) in [#​5973](external-secrets/external-secrets#5973) - fix(charts): Fix wrongly set annotations for cert-controller metrics service by [@​josemaia](https://github.com/josemaia) in [#​6029](external-secrets/external-secrets#6029) - feat(providers): Nebius MysteryBox integration by [@​greenmapc](https://github.com/greenmapc) in [#​5868](external-secrets/external-secrets#5868) ##### Dependencies - chore(deps): bump aquasecurity/trivy-action from 0.34.0 to 0.34.1 by [@​dependabot](https://github.com/dependabot)\[bot] in [#​5986](external-secrets/external-secrets#5986) - chore(deps): bump mkdocs-material from 9.7.1 to 9.7.2 in /hack/api-docs by [@​dependabot](https://github.com/dependabot)\[bot] in [#​5992](external-secrets/external-secrets#5992) - chore(deps): bump ubi9/ubi from `b8923f5` to `cecb1cd` by [@​dependabot](https://github.com/dependabot)\[bot] in [#​5984](external-secrets/external-secrets#5984) - chore(deps): bump helm/kind-action from 1.13.0 to 1.14.0 by [@​dependabot](https://github.com/dependabot)\[bot] in [#​5985](external-secrets/external-secrets#5985) - chore(deps): bump actions/dependency-review-action from 4.8.2 to 4.8.3 by [@​dependabot](https://github.com/dependabot)\[bot] in [#​5990](external-secrets/external-secrets#5990) - chore(deps): bump github/codeql-action from 4.32.3 to 4.32.4 by [@​dependabot](https://github.com/dependabot)\[bot] in [#​5989](external-secrets/external-secrets#5989) - chore(deps): bump goreleaser/goreleaser-action from 6.4.0 to 7.0.0 by [@​dependabot](https://github.com/dependabot)\[bot] in [#​5987](external-secrets/external-secrets#5987) - chore(deps): bump regex from 2026.1.15 to 2026.2.19 in /hack/api-docs by [@​dependabot](https://github.com/dependabot)\[bot] in [#​5991](external-secrets/external-secrets#5991) - chore(deps): bump actions/stale from 10.1.1 to 10.2.0 by [@​dependabot](https://github.com/dependabot)\[bot] in [#​5988](external-secrets/external-secrets#5988) - chore(deps): bump regex from 2026.2.19 to 2026.2.28 in /hack/api-docs by [@​dependabot](https://github.com/dependabot)\[bot] in [#​6012](external-secrets/external-secrets#6012) - chore(deps): bump mkdocs-material from 9.7.2 to 9.7.3 in /hack/api-docs by [@​dependabot](https://github.com/dependabot)\[bot] in [#​6014](external-secrets/external-secrets#6014) - chore(deps): bump step-security/harden-runner from 2.14.2 to 2.15.0 by [@​dependabot](https://github.com/dependabot)\[bot] in [#​6015](external-secrets/external-secrets#6015) - chore(deps): bump anchore/sbom-action from 0.22.2 to 0.23.0 by [@​dependabot](https://github.com/dependabot)\[bot] in [#​6016](external-secrets/external-secrets#6016) - chore(deps): bump certifi from 2026.1.4 to 2026.2.25 in /hack/api-docs by [@​dependabot](https://github.com/dependabot)\[bot] in [#​6013](external-secrets/external-secrets#6013) - chore(deps): bump actions/setup-go from 6.2.0 to 6.3.0 by [@​dependabot](https://github.com/dependabot)\[bot] in [#​6010](external-secrets/external-secrets#6010) - chore(deps): bump hashicorp/setup-terraform from [`ce70bcf`](external-secrets/external-secrets@ce70bcf) to [`5e8dbf3`](external-secrets/external-secrets@5e8dbf3) by [@​dependabot](https://github.com/dependabot)\[bot] in [#​6011](external-secrets/external-secrets#6011) - chore(deps): bump actions/attest-build-provenance from 3.2.0 to 4.1.0 by [@​dependabot](https://github.com/dependabot)\[bot] in [#​6009](external-secrets/external-secrets#6009) - chore(deps): bump distroless/static from `972618c` to `28efbe9` by [@​dependabot](https://github.com/dependabot)\[bot] in [#​6008](external-secrets/external-secrets#6008) #### New Contributors - [@​nutmos](https://github.com/nutmos) made their first contribution in [#​5863](external-secrets/external-secrets#5863) - [@​rajsinghtech](https://github.com/rajsinghtech) made their first contribution in [#​5961](external-secrets/external-secrets#5961) - [@​illrill](https://github.com/illrill) made their first contribution in [#​5972](external-secrets/external-secrets#5972) - [@​Saku2](https://github.com/Saku2) made their first contribution in [#​5973](external-secrets/external-secrets#5973) - [@​greenmapc](https://github.com/greenmapc) made their first contribution in [#​5868](external-secrets/external-secrets#5868) **Full Changelog**: <external-secrets/external-secrets@v2.0.1...v2.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. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiI0My41MS4wIiwidXBkYXRlZEluVmVyIjoiNDMuNTEuMCIsInRhcmdldEJyYW5jaCI6Im1haW4iLCJsYWJlbHMiOlsiaW1hZ2UiXX0=--> Reviewed-on: https://gitea.alexlebens.dev/alexlebens/infrastructure/pulls/4491 Co-authored-by: Renovate Bot <renovate-bot@alexlebens.net> Co-committed-by: Renovate Bot <renovate-bot@alexlebens.net>
This PR contains the following updates: | Package | Update | Change | |---|---|---| | [external-secrets](https://github.com/external-secrets/external-secrets) | minor | `2.0.1` → `2.1.0` | --- ### Release Notes <details> <summary>external-secrets/external-secrets (external-secrets)</summary> ### [`v2.1.0`](https://github.com/external-secrets/external-secrets/releases/tag/v2.1.0) [Compare Source](external-secrets/external-secrets@v2.0.1...v2.1.0) Image: `ghcr.io/external-secrets/external-secrets:v2.1.0` Image: `ghcr.io/external-secrets/external-secrets:v2.1.0-ubi` Image: `ghcr.io/external-secrets/external-secrets:v2.1.0-ubi-boringssl` <!-- Release notes generated using configuration in .github/release.yml at main --> #### What's Changed ##### General - chore(release): Update helm chart by [@​evrardj-roche](https://github.com/evrardj-roche) in [#​5981](external-secrets/external-secrets#5981) - fix: cosign verify does not use signing config by [@​gusfcarvalho](https://github.com/gusfcarvalho) in [#​5982](external-secrets/external-secrets#5982) - docs: Update release process by [@​evrardj-roche](https://github.com/evrardj-roche) in [#​5980](external-secrets/external-secrets#5980) - fix: allow cross-namespace push with ClusterSecretStore objects by [@​Skarlso](https://github.com/Skarlso) in [#​5998](external-secrets/external-secrets#5998) - feat(charts): add new flag enable leader for cert-manager by [@​nutmos](https://github.com/nutmos) in [#​5863](external-secrets/external-secrets#5863) - feat(kubernetes): fall back to system CA roots when no CA is configured by [@​rajsinghtech](https://github.com/rajsinghtech) in [#​5961](external-secrets/external-secrets#5961) - feat: dedup sbom but keep it monolithic by [@​moolen](https://github.com/moolen) in [#​6004](external-secrets/external-secrets#6004) - fix: add missing metrics and fundamentally fix the caching logic by [@​Skarlso](https://github.com/Skarlso) in [#​5894](external-secrets/external-secrets#5894) - docs: designate Oracle Vault provider as 'stable' by [@​anders-swanson](https://github.com/anders-swanson) in [#​6020](external-secrets/external-secrets#6020) - docs: Oracle Vault provider capabilities by [@​anders-swanson](https://github.com/anders-swanson) in [#​6023](external-secrets/external-secrets#6023) - docs(azurekv): cert-manager pushsecret example and cleanups by [@​illrill](https://github.com/illrill) in [#​5972](external-secrets/external-secrets#5972) - feat(kubernetes): implement SecretExists by [@​Saku2](https://github.com/Saku2) in [#​5973](external-secrets/external-secrets#5973) - fix(charts): Fix wrongly set annotations for cert-controller metrics service by [@​josemaia](https://github.com/josemaia) in [#​6029](external-secrets/external-secrets#6029) - feat(providers): Nebius MysteryBox integration by [@​greenmapc](https://github.com/greenmapc) in [#​5868](external-secrets/external-secrets#5868) ##### Dependencies - chore(deps): bump aquasecurity/trivy-action from 0.34.0 to 0.34.1 by [@​dependabot](https://github.com/dependabot)\[bot] in [#​5986](external-secrets/external-secrets#5986) - chore(deps): bump mkdocs-material from 9.7.1 to 9.7.2 in /hack/api-docs by [@​dependabot](https://github.com/dependabot)\[bot] in [#​5992](external-secrets/external-secrets#5992) - chore(deps): bump ubi9/ubi from `b8923f5` to `cecb1cd` by [@​dependabot](https://github.com/dependabot)\[bot] in [#​5984](external-secrets/external-secrets#5984) - chore(deps): bump helm/kind-action from 1.13.0 to 1.14.0 by [@​dependabot](https://github.com/dependabot)\[bot] in [#​5985](external-secrets/external-secrets#5985) - chore(deps): bump actions/dependency-review-action from 4.8.2 to 4.8.3 by [@​dependabot](https://github.com/dependabot)\[bot] in [#​5990](external-secrets/external-secrets#5990) - chore(deps): bump github/codeql-action from 4.32.3 to 4.32.4 by [@​dependabot](https://github.com/dependabot)\[bot] in [#​5989](external-secrets/external-secrets#5989) - chore(deps): bump goreleaser/goreleaser-action from 6.4.0 to 7.0.0 by [@​dependabot](https://github.com/dependabot)\[bot] in [#​5987](external-secrets/external-secrets#5987) - chore(deps): bump regex from 2026.1.15 to 2026.2.19 in /hack/api-docs by [@​dependabot](https://github.com/dependabot)\[bot] in [#​5991](external-secrets/external-secrets#5991) - chore(deps): bump actions/stale from 10.1.1 to 10.2.0 by [@​dependabot](https://github.com/dependabot)\[bot] in [#​5988](external-secrets/external-secrets#5988) - chore(deps): bump regex from 2026.2.19 to 2026.2.28 in /hack/api-docs by [@​dependabot](https://github.com/dependabot)\[bot] in [#​6012](external-secrets/external-secrets#6012) - chore(deps): bump mkdocs-material from 9.7.2 to 9.7.3 in /hack/api-docs by [@​dependabot](https://github.com/dependabot)\[bot] in [#​6014](external-secrets/external-secrets#6014) - chore(deps): bump step-security/harden-runner from 2.14.2 to 2.15.0 by [@​dependabot](https://github.com/dependabot)\[bot] in [#​6015](external-secrets/external-secrets#6015) - chore(deps): bump anchore/sbom-action from 0.22.2 to 0.23.0 by [@​dependabot](https://github.com/dependabot)\[bot] in [#​6016](external-secrets/external-secrets#6016) - chore(deps): bump certifi from 2026.1.4 to 2026.2.25 in /hack/api-docs by [@​dependabot](https://github.com/dependabot)\[bot] in [#​6013](external-secrets/external-secrets#6013) - chore(deps): bump actions/setup-go from 6.2.0 to 6.3.0 by [@​dependabot](https://github.com/dependabot)\[bot] in [#​6010](external-secrets/external-secrets#6010) - chore(deps): bump hashicorp/setup-terraform from [`ce70bcf`](external-secrets/external-secrets@ce70bcf) to [`5e8dbf3`](external-secrets/external-secrets@5e8dbf3) by [@​dependabot](https://github.com/dependabot)\[bot] in [#​6011](external-secrets/external-secrets#6011) - chore(deps): bump actions/attest-build-provenance from 3.2.0 to 4.1.0 by [@​dependabot](https://github.com/dependabot)\[bot] in [#​6009](external-secrets/external-secrets#6009) - chore(deps): bump distroless/static from `972618c` to `28efbe9` by [@​dependabot](https://github.com/dependabot)\[bot] in [#​6008](external-secrets/external-secrets#6008) #### New Contributors - [@​nutmos](https://github.com/nutmos) made their first contribution in [#​5863](external-secrets/external-secrets#5863) - [@​rajsinghtech](https://github.com/rajsinghtech) made their first contribution in [#​5961](external-secrets/external-secrets#5961) - [@​illrill](https://github.com/illrill) made their first contribution in [#​5972](external-secrets/external-secrets#5972) - [@​Saku2](https://github.com/Saku2) made their first contribution in [#​5973](external-secrets/external-secrets#5973) - [@​greenmapc](https://github.com/greenmapc) made their first contribution in [#​5868](external-secrets/external-secrets#5868) **Full Changelog**: <external-secrets/external-secrets@v2.0.1...v2.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. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiI0My41OS4yIiwidXBkYXRlZEluVmVyIjoiNDMuNTkuMiIsInRhcmdldEJyYW5jaCI6Im1haW4iLCJsYWJlbHMiOlsiY2hhcnQiXX0=--> Reviewed-on: https://gitea.alexlebens.dev/alexlebens/infrastructure/pulls/4516 Co-authored-by: Renovate Bot <renovate-bot@alexlebens.net> Co-committed-by: Renovate Bot <renovate-bot@alexlebens.net>
…ernal-secrets#5894) Signed-off-by: AlexOQ <30403857+AlexOQ@users.noreply.github.com>






Problem Statement
What is the problem you're trying to solve?
Related Issue
Fixes #5891
Proposed Changes
How do you like to solve the issue and why?
Format
Please ensure that your PR follows the following format for the title:
Where
scopeis optionally one of:Checklist
git commit --signoffmake testmake reviewableThis PR fixes the 1Password SDK provider caching lifecycle, adds missing API-call metrics, and refactors the client surface to SecretsClient.
Key changes
Notes