Standalone caching client factory #1189
Conversation
- Add isStandalone boolean to cachingClientFactory and CachingClientFactoryConfig - Modify computeClientCacheKey to accept isStandalone parameter - When isStandalone=true: use content-based hashes of VaultAuth/VaultConnection specs - When isStandalone=false: use K8s resource UIDs with strict 36-char validation - Update all callers to pass isStandalone (defaults to false for VSO) - GetStandalone uses m.isStandalone flag for content-based caching
benashz
left a comment
benashz
left a comment
There was a problem hiding this comment.
In general, I like the extension of the client factory support the non-api approach. Although I wonder if we should implement this feature as a standAloneClientFactory rather than extending the ClientFactory interface? For some reason that feels a bit clearer to me.
| authSpecHash := helpers.HashString(fmt.Sprintf("%+v", authObj.Spec)) | ||
| connSpecHash := helpers.HashString(fmt.Sprintf("%+v", connObj.Spec)) | ||
|
|
There was a problem hiding this comment.
Would it make sense to validate the various Spec? Could the "repr" be empty or missing required values?
| // Format: "authHash-1.connHash-1.providerUID" | ||
| // (generation is always 1 for standalone since we didn't fetch the auth and conn objects from a K8s resource) | ||
| input = fmt.Sprintf("%s-%d.%s-%d.%s", | ||
| authSpecHash, 1, |
There was a problem hiding this comment.
nit: is there a particular reason to interpolate the generation since it is always 1?
There was a problem hiding this comment.
I was just trying to match the format of the original client cache key, but I suppose it's not necessary!
| if len(uid) != requireUIDLen { | ||
| errs = errors.Join(errs, fmt.Errorf("%w %d, must be %d", errorInvalidUIDLength, len(uid), requireUIDLen)) | ||
| var input string | ||
| if isStandalone { |
There was a problem hiding this comment.
I wonder if you may have considered setting the UIDs from the Spec, before passing them into this function. That might obviate the need for the special casing here?
There was a problem hiding this comment.
I thought about it, but got a bit scared off by the "read-only" description of the UID field (since traditionally the field is set by the API server).
I like your idea about making this whole thing a standaloneClientFactory instead though; that may clean up all of this and also allow us to more definitively avoid breaking existing code.
| Get(context.Context, ctrlclient.Client, ctrlclient.Object) (Client, error) | ||
| // GetStandalone obtains a client without requiring Kubernetes API access. | ||
| // Suitable for standalone usage where K8s resources are unavailable. | ||
| GetStandalone(context.Context, *secretsv1beta1.VaultAuth, *secretsv1beta1.VaultConnection) (Client, error) |
There was a problem hiding this comment.
I wonder if you had maybe considered implementing a StandAloneFactory rather than extending the interface?
| encClientSetupTimeout time.Duration | ||
| // isStandalone indicates this factory will be used to create clients that cannot be validated against real K8s API resources. | ||
| // Useful for creating clients using VSO's caching client factory pattern in other use cases besides VSO. | ||
| isStandalone bool |
There was a problem hiding this comment.
When isStandlone is true, does that mean the usual Get() method should not be used?
| return nil, errs | ||
| } | ||
|
|
||
| cacheKey, err = computeClientCacheKey(authObj, connObj, provider.GetUID(), m.isStandalone) |
There was a problem hiding this comment.
I am a bit confused by the meaning of isStandAlone member - is it legal to call GetStandAlone() when it is false?
|
Closing in favor of #1199 |
2 participants
There are use cases where our other non-Operator consumers of the VSO client library want to use the VaultAuth and VaultConnection structs but do not have access to actual VaultAuth and VaultConnection resources. Currently, many of the existing caching client factory-related methods have dependencies on the actual resources existing in cluster.
This PR changes that, by allowing developers to designate the caching client factory (and thus, the client) as "standalone", meaning that it does not require VaultAuth or VaultConnection custom resources in the cluster.
PCI review checklist
I have documented a clear reason for, and description of, the change I am making.
If applicable, I've documented a plan to revert these changes if they require more than reverting the pull request.
If applicable, I've documented the impact of any changes to security controls.
Examples of changes to security controls include using new access control methods, adding or removing logging pipelines, etc.