feat: add caching to the 1Password SDK provider#5811
feat: add caching to the 1Password SDK provider#5811Skarlso merged 4 commits intoexternal-secrets:mainfrom
Conversation
WalkthroughAdds a public CacheConfig type and a Changes
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
94370d5 to
e5fc5bc
Compare
Signed-off-by: Gergely Brautigam <182850+Skarlso@users.noreply.github.com>
e5fc5bc to
8017f2b
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
providers/v1/onepasswordsdk/provider.go (1)
58-110: Resetp.cacheinNewClientand validate cache config values.Since Provider instances are cached by type and reused across multiple stores, the cache field persists across NewClient calls. If a store with caching enabled is followed by a store with caching disabled, the old cache remains ("sticky cache"). Additionally, ValidateStore should reject negative TTL or MaxSize values to fail fast; currently they're silently ignored with defaults.
Proposed fix (defensive reset + config validation)
func (p *Provider) NewClient(ctx context.Context, store esv1.GenericStore, kube client.Client, namespace string) (esv1.SecretsClient, error) { config := store.GetSpec().Provider.OnePasswordSDK + // Defensive reset in case this Provider instance is reused. + p.cache = nil + serviceAccountToken, err := resolvers.SecretKeyRef( ctx, kube, store.GetKind(), namespace, &config.Auth.ServiceAccountSecretRef, ) @@ if err != nil { return nil, err } @@ p.vaultID = vaultID if config.Cache != nil { + if config.Cache.TTL.Duration < 0 { + return nil, fmt.Errorf("invalid cache.ttl: must be >= 0") + } + if config.Cache.MaxSize < 0 { + return nil, fmt.Errorf("invalid cache.maxSize: must be >= 0") + } + ttl := 5 * time.Minute if config.Cache.TTL.Duration > 0 { ttl = config.Cache.TTL.Duration } @@ p.cache = expirable.NewLRU[string, []byte](maxSize, nil, ttl) } return p, nil }Alternatively, centralize cache validation in
ValidateStore.
🤖 Fix all issues with AI agents
In @config/crds/bases/external-secrets.io_clustersecretstores.yaml:
- Around line 3868-3887: Add stricter validation and clarify wording for the
cache schema used by onepasswordSDK: set properties.maxSize to require integers
>= 1 (add "minimum: 1") and keep default 100; make ttl non-empty by adding a
pattern or minLength (e.g., "minLength: 1" or a duration regex) and keep default
"5m"; mark fields as non-nullable (nullable: false) if supported; and update the
cache description to replace “If nil, caching is disabled” with “If omitted,
caching is disabled” to avoid confusion.
In @providers/v1/onepasswordsdk/client.go:
- Around line 446-461: The invalidateCacheByPrefix function currently uses
strings.HasPrefix and can over-match (e.g., "op://vault/secret1" matching
"op://vault/secret10"); update the loop in Provider.invalidateCacheByPrefix to
only remove keys that are exactly equal to prefix or start with the prefix
followed by the delimiter (e.g., "/") to avoid accidental evictions—specifically
replace the strings.HasPrefix(key, prefix) check with something like key ==
prefix || strings.HasPrefix(key, prefix + "/") and keep using p.cache.Keys() and
p.cache.Remove(key) as before.
🧹 Nitpick comments (7)
apis/externalsecrets/v1/secretstore_onepassword_sdk_types.go (1)
41-54: Add basic validation forCacheConfigto avoid surprising “silent fallback to defaults”.Right now, negative/zero values are accepted and then ignored later (provider-side). Consider enforcing constraints either via kubebuilder validation markers and/or provider
ValidateStore(at least reject negatives).Proposed API-side validation (minimal)
type CacheConfig struct { @@ // +kubebuilder:default=100 + // +kubebuilder:validation:Minimum=1 // +optional MaxSize int `json:"maxSize,omitempty"` }apis/externalsecrets/v1/zz_generated.deepcopy.go (1)
2913-2931: CurrentCachedeepcopy is OK, but consider future-proofing by callingDeepCopyInto.
**out = **inis fine today (value fields only). IfCacheConfiglater gains reference types, this will become subtly wrong.Proposed tweak
if in.Cache != nil { in, out := &in.Cache, &out.Cache *out = new(CacheConfig) - **out = **in + (*in).DeepCopyInto(*out) }config/crds/bases/external-secrets.io_secretstores.yaml (1)
3868-3887: Nice addition; consider basic guardrails forcache.maxSizeand wording cleanup
- This being added only to
v1(and notv1beta1) is the right move. Based on learnings.- Consider adding a minimum for
cache.maxSize(e.g.,minimum: 1) to avoid surprising behavior for0/negative values.- Minor: the description says “If nil”; in CRDs it’s more user-facing to say “If unset/omitted”.
providers/v1/onepasswordsdk/client.go (2)
52-71: Consider caching negative results to address rate limit exhaustion from non-existent secrets.The linked issue #4925 mentions that reconciling non-existent secrets quickly exhausts rate limits. Currently, only successful lookups are cached. If
Resolvereturns an error for a missing secret, repeated reconciliation attempts will still hit the API.Consider caching not-found results with a shorter TTL to reduce API calls for non-existent secrets.
Please confirm whether negative caching (caching of failed lookups) is planned for a follow-up or if it should be addressed in this PR to fully satisfy issue #4925.
145-176: Cache fallback logic is reasonable; consider adding debug logging.The silent fallback on JSON unmarshal/marshal failures (lines 148-150, 171-173) is appropriate for cache resilience. However, adding debug-level logging when these fallbacks occur would help with troubleshooting cache-related issues in production.
providers/v1/onepasswordsdk/client_test.go (2)
974-1000: Consider removing redundant method delegations.The methods
Put,Delete,Archive,List,Shares,Files, andCreatesimply delegate to the embeddedfakeListerwithout adding any behavior. Go's type embedding automatically promotes methods from the embedded type, so these explicit delegations are unnecessary. You only need to overrideGetto add the counter behavior.♻️ Simplified implementation
type fakeListerWithCounter struct { *fakeLister getCallCount int } func (f *fakeListerWithCounter) Get(ctx context.Context, vaultID, itemID string) (onepassword.Item, error) { f.getCallCount++ return f.fakeLister.Get(ctx, vaultID, itemID) } - -func (f *fakeListerWithCounter) Put(ctx context.Context, item onepassword.Item) (onepassword.Item, error) { - return f.fakeLister.Put(ctx, item) -} - -func (f *fakeListerWithCounter) Delete(ctx context.Context, vaultID, itemID string) error { - return f.fakeLister.Delete(ctx, vaultID, itemID) -} - -func (f *fakeListerWithCounter) Archive(ctx context.Context, vaultID, itemID string) error { - return f.fakeLister.Archive(ctx, vaultID, itemID) -} - -func (f *fakeListerWithCounter) List(ctx context.Context, vaultID string, opts ...onepassword.ItemListFilter) ([]onepassword.ItemOverview, error) { - return f.fakeLister.List(ctx, vaultID, opts...) -} - -func (f *fakeListerWithCounter) Shares() onepassword.ItemsSharesAPI { - return f.fakeLister.Shares() -} - -func (f *fakeListerWithCounter) Files() onepassword.ItemsFilesAPI { - return f.fakeLister.Files() -} - -func (f *fakeListerWithCounter) Create(ctx context.Context, item onepassword.ItemCreateParams) (onepassword.Item, error) { - return f.fakeLister.Create(ctx, item) -}
676-950: Consider adding tests for TTL expiration and cache size limits.The current test suite effectively validates cache hit/miss behavior and invalidation logic. However, since the PR introduces
CacheConfigwithTTLandMaxSizefields (per the AI summary), consider adding tests to verify:
- TTL expiration: Entries should expire and be refetched after the configured TTL period
- MaxSize enforcement: Cache should evict old entries when the size limit is reached
These tests would ensure the advertised cache configuration features work correctly and help prevent regressions.
Example TTL expiration test outline
func TestCacheTTLExpiration(t *testing.T) { fcWithCounter := &fakeClientWithCounter{ fakeClient: &fakeClient{resolveResult: "secret-value"}, } p := &Provider{ client: &onepassword.Client{ SecretsAPI: fcWithCounter, VaultsAPI: fcWithCounter.fakeClient, }, vaultPrefix: "op://vault/", cache: expirable.NewLRU[string, []byte](100, nil, 100*time.Millisecond), } ref := v1.ExternalSecretDataRemoteRef{Key: "item/field"} // First call _, err := p.GetSecret(context.Background(), ref) require.NoError(t, err) assert.Equal(t, 1, fcWithCounter.resolveCallCount) // Wait for TTL expiration time.Sleep(150 * time.Millisecond) // Should fetch again after expiration _, err = p.GetSecret(context.Background(), ref) require.NoError(t, err) assert.Equal(t, 2, fcWithCounter.resolveCallCount) }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
providers/v1/onepasswordsdk/go.sumis excluded by!**/*.sum
📒 Files selected for processing (13)
apis/externalsecrets/v1/secretstore_onepassword_sdk_types.goapis/externalsecrets/v1/zz_generated.deepcopy.goconfig/crds/bases/external-secrets.io_clustersecretstores.yamlconfig/crds/bases/external-secrets.io_secretstores.yamldeploy/crds/bundle.yamldocs/api/spec.mddocs/provider/1password-sdk.mdproviders/v1/onepasswordsdk/client.goproviders/v1/onepasswordsdk/client_test.goproviders/v1/onepasswordsdk/go.modproviders/v1/onepasswordsdk/provider.gotests/__snapshot__/clustersecretstore-v1.yamltests/__snapshot__/secretstore-v1.yaml
👮 Files not reviewed due to content moderation or server errors (1)
- docs/api/spec.md
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2026-01-09T19:14:42.737Z
Learnt from: rbstp
Repo: external-secrets/external-secrets PR: 5712
File: config/crds/bases/external-secrets.io_secretstores.yaml:1958-2044
Timestamp: 2026-01-09T19:14:42.737Z
Learning: In external-secrets CRDs, new providers are added to the GA v1 schema only; v1beta1 is maintained for backward compatibility and does not receive new features (e.g., DVLS should be v1-only).
Applied to files:
apis/externalsecrets/v1/secretstore_onepassword_sdk_types.godeploy/crds/bundle.yaml
📚 Learning: 2026-01-09T19:14:42.737Z
Learnt from: rbstp
Repo: external-secrets/external-secrets PR: 5712
File: config/crds/bases/external-secrets.io_secretstores.yaml:1958-2044
Timestamp: 2026-01-09T19:14:42.737Z
Learning: In external-secrets CRDs, add new providers only to the GA v1 schema. Do not introduce new features to v1beta1, which exists only for backward compatibility. Ensure that any provider-related changes (e.g., DVLS) are implemented in v1-only definitions to avoid breaking older installations. This guideline applies to YAML files under config/crds/bases that define CRD schemas for external-secrets providers.
Applied to files:
config/crds/bases/external-secrets.io_clustersecretstores.yamlconfig/crds/bases/external-secrets.io_secretstores.yaml
📚 Learning: 2026-01-09T19:17:37.361Z
Learnt from: rbstp
Repo: external-secrets/external-secrets PR: 5712
File: config/crds/bases/external-secrets.io_secretstores.yaml:1958-2044
Timestamp: 2026-01-09T19:17:37.361Z
Learning: In CRD YAML files under config/crds, avoid provider-specific schema validations (e.g., format: uri or CEL rules) when other providers don’t use them. Aim for consistent, provider-agnostic validations across all CRDs to ensure uniform behavior and compatibility across providers. If a field requires provider-specific constraints, document and implement them in a separate, clearly scoped area or add conditional validation only where it’s universally applicable.
Applied to files:
config/crds/bases/external-secrets.io_clustersecretstores.yamlconfig/crds/bases/external-secrets.io_secretstores.yaml
🧬 Code graph analysis (2)
apis/externalsecrets/v1/zz_generated.deepcopy.go (1)
apis/externalsecrets/v1/secretstore_onepassword_sdk_types.go (1)
CacheConfig(42-54)
providers/v1/onepasswordsdk/client_test.go (2)
providers/v1/onepasswordsdk/provider.go (1)
Provider(50-55)apis/externalsecrets/v1alpha1/pushsecret_types.go (2)
PushSecretMatch(166-172)PushSecret(261-267)
🪛 LanguageTool
docs/provider/1password-sdk.md
[style] ~44-~44: Using many exclamation marks might seem excessive (in this case: 3 exclamation marks for a text that’s 1940 characters long)
Context: ...lly invalidate relevant cache entries. !!! warning "Experimental" This is an e...
(EN_EXCESSIVE_EXCLAMATION)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: publish-artifacts (Dockerfile.ubi, CGO_ENABLED=0 GOEXPERIMENT=boringcrypto, amd64 ppc64le, linux/... / Build and Publish
- GitHub Check: publish-artifacts (Dockerfile, CGO_ENABLED=0, amd64 arm64 s390x ppc64le, linux/amd64,linux/arm64,... / Build and Publish
- GitHub Check: publish-artifacts (Dockerfile.ubi, CGO_ENABLED=0, amd64 arm64 ppc64le, linux/amd64,linux/arm64,li... / Build and Publish
- GitHub Check: unit-tests
- GitHub Check: check-diff
- GitHub Check: Analyze project (go, autobuild)
🔇 Additional comments (19)
tests/__snapshot__/secretstore-v1.yaml (1)
585-587: Cache configuration defaults look reasonable.The default values (TTL of 5 minutes and max size of 100) provide a good balance between reducing 1Password API calls and maintaining reasonable data freshness. This aligns with the documentation and should help address the rate limiting issues described in issue #4925.
docs/provider/1password-sdk.md (1)
21-46: Clarify that cache only stores successful responses, not error cases.The documentation is clear about caching behavior, but it should explicitly state that only successful secret retrievals are cached. Errors—including invalid tokens, invalid service accounts, and non-existent secrets—are not cached and will trigger API calls on every retry. This is an important distinction because it means caching does not help mitigate rate limits in scenarios involving authentication failures or missing secrets (relevant to issue #4925).
Additionally, consider adding:
Security considerations: Mention that secrets are cached in-memory and persist for the configured TTL even after deletion from 1Password. Recommend shorter TTLs (e.g., 1m) for highly sensitive environments where secrets change frequently.
Error handling: Clarify that errors (invalid tokens, non-existent secrets, API failures) bypass the cache and are retried with each request, potentially impacting rate limits when dealing with transient authentication issues.
providers/v1/onepasswordsdk/go.mod (1)
9-9: The HashiCorp golang-lru library is an appropriate choice for LRU caching, and v2.0.7 is the current stable version with no known security vulnerabilities.tests/__snapshot__/clustersecretstore-v1.yaml (1)
579-587: Snapshot update looks consistent with the newonepasswordSDK.cacheschema.
maxSize: 100andttl: "5m"match the intended defaults and provide a clear example in the snapshot.apis/externalsecrets/v1/secretstore_onepassword_sdk_types.go (2)
19-23: Importingmetav1formetav1.Durationis appropriate.
64-71:Cache *CacheConfigbeing nil-by-default is the right safety default.One small doc nuance: caching secrets in-memory increases exposure surface (memory dumps, etc.)—worth explicitly stating in the provider docs if not already.
providers/v1/onepasswordsdk/provider.go (2)
49-55: Storingcacheon the Provider is fine, but confirm Provider instance lifecycle (avoid cross-store/cache leakage).If the same
*Providerinstance can be reused across different SecretStores, the cache (and client/vault state) can leak across stores. If that’s possible in this codebase, reset state at the start ofNewClient.
21-35: No action required—theexpirable.LRUusage is safe and correct.The
github.com/hashicorp/golang-lru/v2/expirablepackage is thread-safe and well-documented. The code properly handles TTL and size edge cases: TTL defaults to 5 minutes (configurable, validated > 0) and maxSize defaults to 100 (configurable, validated > 0), ensuringNewLRU()is never called with invalid edge case values. The background purge goroutine is expected behavior and poses no lifecycle issues for long-lived Provider instances.apis/externalsecrets/v1/zz_generated.deepcopy.go (1)
816-830: DeepCopy forCacheConfiglooks correct for current fields.providers/v1/onepasswordsdk/client.go (2)
429-444: LGTM!The cache helper methods properly guard against nil cache and provide clean abstractions.
100-110: Cache invalidation placement is correct.The cache invalidation after successful delete and update operations ensures stale data is not served. The prefix matching issue is addressed in a separate comment.
providers/v1/onepasswordsdk/client_test.go (7)
23-23: LGTM!The new imports are correctly added to support cache initialization and testing with TTL-based LRU caching.
Also applies to: 26-26
676-737: LGTM!The test comprehensively covers cache hit/miss behavior and validates that caching is properly disabled when cache is nil. The use of call counters effectively verifies that the cache prevents redundant API calls.
739-793: LGTM!The test validates cache behavior for
GetSecretMap, ensuring that map-based secret retrieval also benefits from caching and prevents redundant API calls.
795-856: LGTM!This test validates a critical behavior: cache invalidation on push operations. Without proper invalidation, the cache would serve stale data after updates, which directly addresses the rate-limiting issue mentioned in the linked issue #4925 by ensuring cache consistency.
858-912: LGTM!The test ensures cache entries are properly invalidated when secrets are deleted, preventing the cache from serving deleted data.
914-950: LGTM!The test thoroughly validates the prefix-based cache invalidation helper, including proper nil-safety to prevent panics when caching is disabled.
1005-1006: LGTM!The compile-time interface assertions ensure the test doubles correctly implement the expected interfaces, providing early detection of interface mismatches.
deploy/crds/bundle.yaml (1)
5683-5706: Cache block placement is correctly v1-only.The OnePasswordSDK provider and its cache feature are exclusive to the GA v1 schema, not duplicated in v1beta1. This adheres to the project's CRD maintenance policy where new features are added only to v1 and v1beta1 is reserved for backward compatibility.
| cache: | ||
| description: |- | ||
| Cache configures client-side caching for read operations (GetSecret, GetSecretMap). | ||
| When enabled, secrets are cached with the specified TTL. | ||
| Write operations (PushSecret, DeleteSecret) automatically invalidate relevant cache entries. | ||
| If nil, caching is disabled (default). | ||
| properties: | ||
| maxSize: | ||
| default: 100 | ||
| description: |- | ||
| MaxSize is the maximum number of secrets to cache. | ||
| When the cache is full, least-recently-used entries are evicted. | ||
| type: integer | ||
| ttl: | ||
| default: 5m | ||
| description: |- | ||
| TTL is the time-to-live for cached secrets. | ||
| Format: duration string (e.g., "5m", "1h", "30s") | ||
| type: string | ||
| type: object |
There was a problem hiding this comment.
Harden onepasswordSDK.cache schema: avoid invalid maxSize/ttl + clarify “nil” wording.
Right now maxSize can be 0/negative and ttl can be empty, which can cause runtime issues; also “nil” is confusing in YAML/CRD usage (users omit the field rather than set null).
Proposed diff
cache:
description: |-
Cache configures client-side caching for read operations (GetSecret, GetSecretMap).
When enabled, secrets are cached with the specified TTL.
Write operations (PushSecret, DeleteSecret) automatically invalidate relevant cache entries.
- If nil, caching is disabled (default).
+ If omitted, caching is disabled (default).
properties:
maxSize:
default: 100
description: |-
MaxSize is the maximum number of secrets to cache.
When the cache is full, least-recently-used entries are evicted.
+ minimum: 1
type: integer
ttl:
default: 5m
description: |-
TTL is the time-to-live for cached secrets.
Format: duration string (e.g., "5m", "1h", "30s")
+ minLength: 1
type: string
type: object📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| cache: | |
| description: |- | |
| Cache configures client-side caching for read operations (GetSecret, GetSecretMap). | |
| When enabled, secrets are cached with the specified TTL. | |
| Write operations (PushSecret, DeleteSecret) automatically invalidate relevant cache entries. | |
| If nil, caching is disabled (default). | |
| properties: | |
| maxSize: | |
| default: 100 | |
| description: |- | |
| MaxSize is the maximum number of secrets to cache. | |
| When the cache is full, least-recently-used entries are evicted. | |
| type: integer | |
| ttl: | |
| default: 5m | |
| description: |- | |
| TTL is the time-to-live for cached secrets. | |
| Format: duration string (e.g., "5m", "1h", "30s") | |
| type: string | |
| type: object | |
| cache: | |
| description: |- | |
| Cache configures client-side caching for read operations (GetSecret, GetSecretMap). | |
| When enabled, secrets are cached with the specified TTL. | |
| Write operations (PushSecret, DeleteSecret) automatically invalidate relevant cache entries. | |
| If omitted, caching is disabled (default). | |
| properties: | |
| maxSize: | |
| default: 100 | |
| description: |- | |
| MaxSize is the maximum number of secrets to cache. | |
| When the cache is full, least-recently-used entries are evicted. | |
| minimum: 1 | |
| type: integer | |
| ttl: | |
| default: 5m | |
| description: |- | |
| TTL is the time-to-live for cached secrets. | |
| Format: duration string (e.g., "5m", "1h", "30s") | |
| minLength: 1 | |
| type: string | |
| type: object |
🤖 Prompt for AI Agents
In @config/crds/bases/external-secrets.io_clustersecretstores.yaml around lines
3868 - 3887, Add stricter validation and clarify wording for the cache schema
used by onepasswordSDK: set properties.maxSize to require integers >= 1 (add
"minimum: 1") and keep default 100; make ttl non-empty by adding a pattern or
minLength (e.g., "minLength: 1" or a duration regex) and keep default "5m"; mark
fields as non-nullable (nullable: false) if supported; and update the cache
description to replace “If nil, caching is disabled” with “If omitted, caching
is disabled” to avoid confusion.
There was a problem hiding this comment.
@coderabbitai I have this in the code:
ttl := 5 * time.Minute
if config.Cache.TTL.Duration > 0 {
ttl = config.Cache.TTL.Duration
}
maxSize := 100
if config.Cache.MaxSize > 0 {
maxSize = config.Cache.MaxSize
}I think that should be enough to ward against these types of things.
There was a problem hiding this comment.
@Skarlso Thanks — the runtime guards you showed do protect behavior at run time, so no crash or bad cache sizing will slip through.
The reason I still suggested tightening the CRD is to fail fast and improve UX: invalid values would be rejected by the API server at apply time, and the OpenAPI docs would accurately reflect constraints. It’s also purely additive — it won’t change your current defaults.
Two light-weight options (pick either):
- Source-of-truth in Go (preferred; will propagate to all CRDs/bundles/tests on generate)
- apis/externalsecrets/v1/secretstore_onepassword_sdk_types.go
// CacheConfig configures client-side caching for read operations.
type CacheConfig struct {
// TTL is the time-to-live for cached secrets.
// +kubebuilder:default="5m"
// +kubebuilder:validation:MinLength=1
TTL metav1.Duration `json:"ttl,omitempty"`
// MaxSize is the maximum number of secrets to cache.
// When the cache is full, least-recently-used entries are evicted.
// +kubebuilder:default=100
// +kubebuilder:validation:Minimum=1
MaxSize int `json:"maxSize,omitempty"`
}
// In OnePasswordSDKProvider, also tweak the field comment:
// If omitted, caching is disabled (default).
Cache *CacheConfig `json:"cache,omitempty"`Then run your usual generate to refresh:
- config/crds/bases/external-secrets.io_{secretstores,clustersecretstores}.yaml
- deploy/crds/bundle.yaml
- docs/api/spec.md (if generated), and update docs/provider/1password-sdk.md wording from “If nil” to “If omitted”.
- Minimal YAML-only tweak in this PR (if you don’t want to touch types now)
- In both SecretStore and ClusterSecretStore CRDs under provider.onepasswordSDK.cache:
- Change description tail to: “If omitted, caching is disabled (default).”
- Add “minimum: 1” under maxSize.
- Add “minLength: 1” under ttl.
If you prefer keeping the runtime-only guard for now, I’m fine to drop the suggestion — I mainly wanted to shift validation left to make bad inputs obvious at apply time. Also, nice job keeping the addition v1-only; that matches the project’s v1-new-features policy.
🧠 Learnings used
Learnt from: rbstp
Repo: external-secrets/external-secrets PR: 5712
File: config/crds/bases/external-secrets.io_secretstores.yaml:1958-2044
Timestamp: 2026-01-09T19:14:42.737Z
Learning: In external-secrets CRDs, new providers are added to the GA v1 schema only; v1beta1 is maintained for backward compatibility and does not receive new features (e.g., DVLS should be v1-only).
There was a problem hiding this comment.
If I add // +kubebuilder:validation:MinLength=1 I can still use optional, right? I want people to be able to define cache: {} without any of the fields.
There was a problem hiding this comment.
@Skarlso Yes. Adding +kubebuilder:validation:MinLength=1 on TTL keeps it optional. Validation applies only if the field is present; it does not make the field required. With your current shapes:
- Cache is a pointer (Cache *CacheConfig
json:"cache,omitempty"), so the whole block remains optional. - cache: {} is valid. With the default markers, the API server will default ttl to "5m" and maxSize to 100.
- To disable caching, users should omit the cache field entirely. Using null isn’t allowed by the schema; “omitted” is the correct wording.
Recommended markers in apis/externalsecrets/v1/secretstore_onepassword_sdk_types.go:
// CacheConfig configures client-side caching for read operations.
type CacheConfig struct {
// TTL is the time-to-live for cached secrets.
// +kubebuilder:default="5m"
// +kubebuilder:validation:MinLength=1
TTL metav1.Duration `json:"ttl,omitempty"`
// MaxSize is the maximum number of secrets to cache.
// When the cache is full, least-recently-used entries are evicted.
// +kubebuilder:default=100
// +kubebuilder:validation:Minimum=1
MaxSize int `json:"maxSize,omitempty"`
}
// If omitted, caching is disabled (default).
Cache *CacheConfig `json:"cache,omitempty"`After updating markers, regenerate CRDs so both SecretStore and ClusterSecretStore pick up:
- minimum: 1 for maxSize
- minLength: 1 for ttl
- description change: “If omitted, caching is disabled (default).”
✏️ Learnings added
Learnt from: Skarlso
Repo: external-secrets/external-secrets PR: 5811
File: config/crds/bases/external-secrets.io_clustersecretstores.yaml:3868-3887
Timestamp: 2026-01-10T10:41:30.240Z
Learning: For the 1Password SDK provider’s CacheConfig, keep the cache block optional and allow users to set `cache: {}` to enable caching with defaults. Do not make `ttl` required; use `+kubebuilder:validation:MinLength=1` (validates only when provided) and `+kubebuilder:default="5m"`. Use `+kubebuilder:validation:Minimum=1` and default 100 for `maxSize`. Update docs/CRD wording to “If omitted, caching is disabled (default).”
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: rbstp
Repo: external-secrets/external-secrets PR: 5712
File: config/crds/bases/external-secrets.io_secretstores.yaml:1958-2044
Timestamp: 2026-01-09T19:17:37.361Z
Learning: In this repo’s CRDs, avoid adding provider-specific schema validations (e.g., `format: uri` or CEL rules) when other providers don’t use them; prioritize consistency across providers.
Learnt from: rbstp
Repo: external-secrets/external-secrets PR: 5712
File: config/crds/bases/external-secrets.io_secretstores.yaml:1958-2044
Timestamp: 2026-01-09T19:14:42.737Z
Learning: In external-secrets CRDs, new providers are added to the GA v1 schema only; v1beta1 is maintained for backward compatibility and does not receive new features (e.g., DVLS should be v1-only).
There was a problem hiding this comment.
// +kubebuilder:validation:MinLength=1 doesn't work with duration types.
There was a problem hiding this comment.
@Skarlso You're right — controller-gen doesn’t apply MinLength to metav1.Duration reliably. Let’s drop that part.
Recommended minimal hardening while keeping TTL optional:
- Keep defaulting on TTL and rely on your runtime guard for invalid/zero values.
- Add a lower bound for maxSize only.
- Clarify wording from “If nil” to “If omitted”.
Go markers (source of truth), then regenerate CRDs/bundle/docs:
// CacheConfig configures client-side caching for read operations.
type CacheConfig struct {
// TTL is the time-to-live for cached secrets.
// +kubebuilder:default="5m"
TTL metav1.Duration `json:"ttl,omitempty"`
// MaxSize is the maximum number of secrets to cache.
// When the cache is full, least-recently-used entries are evicted.
// +kubebuilder:default=100
// +kubebuilder:validation:Minimum=1
MaxSize int `json:"maxSize,omitempty"`
}
// In OnePasswordSDKProvider:
// If omitted, caching is disabled (default).
Cache *CacheConfig `json:"cache,omitempty"`If you prefer a YAML-only tweak for this PR, we can:
- Change description to “If omitted, caching is disabled (default).”
- Add minimum: 1 under maxSize.
We’ll avoid regex/CEL on ttl to stay consistent with the project’s CRD style.
Would you like me to proceed with a small follow-up to update the types + regenerate, or just patch the CRD YAML text and maxSize minimum here? Also, ok to update our stored learning to remove the MinLength-on-duration suggestion and note the approach above?
🧠 Learnings used
Learnt from: Skarlso
Repo: external-secrets/external-secrets PR: 5811
File: config/crds/bases/external-secrets.io_clustersecretstores.yaml:3868-3887
Timestamp: 2026-01-10T10:41:30.240Z
Learning: For the 1Password SDK provider’s CacheConfig, keep the cache block optional and allow users to set `cache: {}` to enable caching with defaults. Do not make `ttl` required; use `+kubebuilder:validation:MinLength=1` (validates only when provided) and `+kubebuilder:default="5m"`. Use `+kubebuilder:validation:Minimum=1` and default 100 for `maxSize`. Update docs/CRD wording to “If omitted, caching is disabled (default).”
Learnt from: rbstp
Repo: external-secrets/external-secrets PR: 5712
File: config/crds/bases/external-secrets.io_secretstores.yaml:1958-2044
Timestamp: 2026-01-09T19:17:37.361Z
Learning: In this repo’s CRDs, avoid adding provider-specific schema validations (e.g., `format: uri` or CEL rules) when other providers don’t use them; prioritize consistency across providers.
Learnt from: rbstp
Repo: external-secrets/external-secrets PR: 5712
File: config/crds/bases/external-secrets.io_secretstores.yaml:1958-2044
Timestamp: 2026-01-09T19:14:42.737Z
Learning: In external-secrets CRDs, new providers are added to the GA v1 schema only; v1beta1 is maintained for backward compatibility and does not receive new features (e.g., DVLS should be v1-only).
|
|
||
| Caching applies to read operations (`GetSecret`, `GetSecretMap`). Write operations (`PushSecret`, `DeleteSecret`) automatically invalidate relevant cache entries. | ||
|
|
||
| !!! warning "Experimental" |
There was a problem hiding this comment.
Do you want to add that behavior under a flag?
IMO, I think we shouldn't really need to even mark it as experimental. If there are bugs, we fix it. Optional, off by default, etc.
There was a problem hiding this comment.
(but if we want to say its experimental, we need to be consistent and add a --experimental-enable-onepassword-cache or something like that)
There was a problem hiding this comment.
I updated the store API with an optional that if set, will enable caching.
gusfcarvalho
left a comment
There was a problem hiding this comment.
Aside from the RabbitMQ bit - LGTM
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
providers/v1/onepasswordsdk/client_test.go (2)
739-793: Consider adding a "cache disabled" subtest for GetSecretMap.
TestCachingGetSecrethas both "cache hit" and "cache disabled" subtests, butTestCachingGetSecretMaponly tests the cache hit scenario. For symmetry and completeness, consider adding a subtest that verifiesGetSecretMapworks correctly whencacheisnil.
698-698: Usingt.Context()is consistent with the project's Go 1.25.5 minimum version.The new caching tests use
t.Context(), which is supported by the project (targets Go 1.25.5 per go.mod). However, existing tests in this file usecontext.Background()(lines 107, 277, 420, 546, 582). For consistency with the existing test style, consider usingcontext.Background()in the new test functions (lines 698, 704, 729, 733, 779, 788, 829, 847, 851, 895, 904, 908).
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
providers/v1/onepasswordsdk/client.goproviders/v1/onepasswordsdk/client_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- providers/v1/onepasswordsdk/client.go
🧰 Additional context used
🧬 Code graph analysis (1)
providers/v1/onepasswordsdk/client_test.go (3)
providers/v1/onepasswordsdk/provider.go (1)
Provider(50-55)apis/externalsecrets/v1alpha1/pushsecret_types.go (2)
PushSecretMatch(166-172)PushSecret(261-267)providers/v1/fake/fake.go (1)
Data(50-54)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: publish-artifacts (Dockerfile.ubi, CGO_ENABLED=0 GOEXPERIMENT=boringcrypto, amd64 ppc64le, linux/... / Build and Publish
- GitHub Check: publish-artifacts (Dockerfile.ubi, CGO_ENABLED=0, amd64 arm64 ppc64le, linux/amd64,linux/arm64,li... / Build and Publish
- GitHub Check: publish-artifacts (Dockerfile, CGO_ENABLED=0, amd64 arm64 s390x ppc64le, linux/amd64,linux/arm64,... / Build and Publish
- GitHub Check: check-diff
- GitHub Check: unit-tests
- GitHub Check: Analyze project (actions, none)
- GitHub Check: Analyze project (go, autobuild)
🔇 Additional comments (3)
providers/v1/onepasswordsdk/client_test.go (3)
914-990: LGTM! Comprehensive prefix invalidation tests.The test cases cover important edge cases for prefix-based cache invalidation, including the distinction between valid item path separators (
/,|) and characters that form different item names (like-). The nil cache safety check is also a good defensive test.
1003-1040: LGTM! Clean test double implementation.The counter wrapper follows a clean delegation pattern and correctly tracks only the
Getmethod calls needed for verifying cache behavior.
1045-1046: LGTM! Interface conformance checks.Good practice to include compile-time interface checks for the new test doubles.
Signed-off-by: Gergely Brautigam <182850+Skarlso@users.noreply.github.com>
0d8d648 to
6d3ad3e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
providers/v1/onepasswordsdk/client.go (1)
145-152: Consider logging cache unmarshal failures for observability.The silent fallback to fresh fetch is safe, but logging a warning when unmarshal fails would help diagnose potential cache corruption issues during debugging.
🔧 Suggested improvement
cacheKey := p.constructRefKey(ref.Key) + "|" + ref.Property if cached, ok := p.cacheGet(cacheKey); ok { var result map[string][]byte if err := json.Unmarshal(cached, &result); err == nil { return result, nil } - // continue with fresh instead + // Log and continue with fresh fetch instead + // Consider adding: log.V(1).Info("cache unmarshal failed, fetching fresh", "key", cacheKey, "error", err) }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
providers/v1/onepasswordsdk/client.goproviders/v1/onepasswordsdk/client_test.go
🧰 Additional context used
🧬 Code graph analysis (1)
providers/v1/onepasswordsdk/client.go (1)
providers/v1/onepasswordsdk/provider.go (1)
Provider(50-55)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: publish-artifacts (Dockerfile.ubi, CGO_ENABLED=0, amd64 arm64 ppc64le, linux/amd64,linux/arm64,li... / Build and Publish
- GitHub Check: publish-artifacts (Dockerfile.ubi, CGO_ENABLED=0 GOEXPERIMENT=boringcrypto, amd64 ppc64le, linux/... / Build and Publish
- GitHub Check: publish-artifacts (Dockerfile, CGO_ENABLED=0, amd64 arm64 s390x ppc64le, linux/amd64,linux/arm64,... / Build and Publish
- GitHub Check: check-diff
- GitHub Check: unit-tests
- GitHub Check: Analyze project (go, autobuild)
- GitHub Check: Analyze project (actions, none)
🔇 Additional comments (12)
providers/v1/onepasswordsdk/client.go (6)
52-71: LGTM!The cache-aside pattern is correctly implemented: check cache first, return on hit, fetch and cache on miss. The helper methods handle the nil cache case safely.
100-109: LGTM!Cache invalidation is correctly placed after successful mutations (both delete and update paths), ensuring stale cache entries are cleared.
271-273: LGTM!Cache invalidation correctly placed after successful item creation.
363-365: LGTM!Cache invalidation correctly placed after successful secret push.
430-444: LGTM!Clean helper methods with proper nil-safety for when caching is disabled.
446-466: Well-implemented prefix invalidation with proper boundary checking.The delimiter check (
/or|) at line 461 correctly prevents false positives when item names share common prefixes (e.g., invalidating "item" won't affect "item-backup"). The documentation clearly explains the design rationale.providers/v1/onepasswordsdk/client_test.go (6)
676-737: LGTM!Comprehensive test coverage for GetSecret caching: validates cache hit behavior (single API call for multiple reads) and correct fallback when caching is disabled.
739-793: LGTM!Good test coverage for GetSecretMap caching, verifying both data integrity and reduced API calls on cache hit.
795-856: LGTM!Well-structured test verifying that PushSecret correctly invalidates cached entries, forcing a fresh API call on subsequent reads.
858-912: LGTM!Solid test verifying that DeleteSecret correctly invalidates cached entries.
914-990: Excellent edge case coverage!The "does not invalidate entries with similar prefixes" subtest (lines 951-989) is particularly valuable - it validates that invalidating "item" won't affect "item-backup" and "prod-db" won't affect "prod-db-replica". This guards against subtle bugs in the prefix matching logic.
992-1046: LGTM!Clean test double implementation with proper composition and delegation. The interface conformance checks ensure type safety.
Signed-off-by: Gergely Brautigam <182850+Skarlso@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @deploy/crds/bundle.yaml:
- Around line 5686-5707: Add a minLength: 1 validation to the cache.ttl schema
so empty strings are rejected: edit the TTL property under the cache object in
the two CRD source files external-secrets.io_clustersecretstores.yaml and
external-secrets.io_secretstores.yaml (the ttl property currently has type:
string and default: 5m) and add minLength: 1 without making cache required; then
regenerate the consolidated bundle.yaml (deploy/crds/bundle.yaml) so the change
is propagated.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apis/externalsecrets/v1/secretstore_onepassword_sdk_types.goconfig/crds/bases/external-secrets.io_clustersecretstores.yamlconfig/crds/bases/external-secrets.io_secretstores.yamldeploy/crds/bundle.yamldocs/api/spec.md
🚧 Files skipped from review as they are similar to previous changes (3)
- apis/externalsecrets/v1/secretstore_onepassword_sdk_types.go
- config/crds/bases/external-secrets.io_clustersecretstores.yaml
- config/crds/bases/external-secrets.io_secretstores.yaml
👮 Files not reviewed due to content moderation or server errors (1)
- docs/api/spec.md
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: Skarlso
Repo: external-secrets/external-secrets PR: 5811
File: config/crds/bases/external-secrets.io_clustersecretstores.yaml:3868-3887
Timestamp: 2026-01-10T10:41:30.240Z
Learning: For the 1Password SDK provider’s CacheConfig, keep the cache block optional and allow users to set `cache: {}` to enable caching with defaults. Do not make `ttl` required; use `+kubebuilder:validation:MinLength=1` (validates only when provided) and `+kubebuilder:default="5m"`. Use `+kubebuilder:validation:Minimum=1` and default 100 for `maxSize`. Update docs/CRD wording to “If omitted, caching is disabled (default).”
📚 Learning: 2026-01-10T10:41:30.240Z
Learnt from: Skarlso
Repo: external-secrets/external-secrets PR: 5811
File: config/crds/bases/external-secrets.io_clustersecretstores.yaml:3868-3887
Timestamp: 2026-01-10T10:41:30.240Z
Learning: For the 1Password SDK provider’s CacheConfig, keep the cache block optional and allow users to set `cache: {}` to enable caching with defaults. Do not make `ttl` required; use `+kubebuilder:validation:MinLength=1` (validates only when provided) and `+kubebuilder:default="5m"`. Use `+kubebuilder:validation:Minimum=1` and default 100 for `maxSize`. Update docs/CRD wording to “If omitted, caching is disabled (default).”
Applied to files:
deploy/crds/bundle.yamldocs/api/spec.md
📚 Learning: 2026-01-09T19:14:42.737Z
Learnt from: rbstp
Repo: external-secrets/external-secrets PR: 5712
File: config/crds/bases/external-secrets.io_secretstores.yaml:1958-2044
Timestamp: 2026-01-09T19:14:42.737Z
Learning: In external-secrets CRDs, new providers are added to the GA v1 schema only; v1beta1 is maintained for backward compatibility and does not receive new features (e.g., DVLS should be v1-only).
Applied to files:
deploy/crds/bundle.yaml
📚 Learning: 2026-01-09T19:17:37.361Z
Learnt from: rbstp
Repo: external-secrets/external-secrets PR: 5712
File: config/crds/bases/external-secrets.io_secretstores.yaml:1958-2044
Timestamp: 2026-01-09T19:17:37.361Z
Learning: In this repo’s CRDs, avoid adding provider-specific schema validations (e.g., `format: uri` or CEL rules) when other providers don’t use them; prioritize consistency across providers.
Applied to files:
deploy/crds/bundle.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: publish-artifacts (Dockerfile.ubi, CGO_ENABLED=0, amd64 arm64 ppc64le, linux/amd64,linux/arm64,li... / Build and Publish
- GitHub Check: publish-artifacts (Dockerfile, CGO_ENABLED=0, amd64 arm64 s390x ppc64le, linux/amd64,linux/arm64,... / Build and Publish
- GitHub Check: publish-artifacts (Dockerfile.ubi, CGO_ENABLED=0 GOEXPERIMENT=boringcrypto, amd64 ppc64le, linux/... / Build and Publish
- GitHub Check: check-diff
- GitHub Check: unit-tests
- GitHub Check: Analyze project (go, autobuild)
|
/ok-to-test sha=c85c2ad9fcafd3cfa5e257481d2454d605501128 |
|
|
I laughed #5811 (comment) |
Problem Statement
What is the problem you're trying to solve?
Related Issue
Fixes #4925
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 adds optional client-side caching to the 1Password SDK provider to reduce repeated API calls that can quickly exhaust 1Password rate limits (fixes #4925).
Key changes
Notes