Avoid mutating the client-go informer cache #7743
Conversation
Signed-off-by: Richard Wall <richard.wall@venafi.com>
There was a problem hiding this comment.
Pull Request Overview
This PR aims to prevent accidental mutation of the client-go informer cache by deep copying the Certificate object before applying defaults.
- A deep copy is now performed in ProcessItem to safeguard the informer cache.
- Documentation in the defaulting functions is updated to clearly warn against mutating cached objects.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| pkg/controller/certificates/keymanager/keymanager_controller.go | Deep copy of Certificate objects to avoid cache mutation |
| internal/apis/certmanager/v1/defaults.go | Updated comments to caution against direct mutation of cached objects |
|
I imagine that you can now revert the "update status" logic (https://github.com/cert-manager/cert-manager/pull/7723/files#r2078072530)? -_, err := c.client.CertmanagerV1().Certificates(crt.Namespace).UpdateStatus(ctx, &cmapi.Certificate{
- ObjectMeta: crt.ObjectMeta,
- Status: crt.Status,
-}, metav1.UpdateOptions{})
+_, err := c.client.CertmanagerV1().Certificates(crt.Namespace).UpdateStatus(ctx, crt, metav1.UpdateOptions{}) |
No, because the defaulting function will still set the |
|
Ah, makes sense. /lgtm |
| @@ -157,6 +157,8 @@ func (c *controller) ProcessItem(ctx context.Context, key types.NamespacedName) | |||
|
|
|||
| // Apply runtime defaults to apply default values that are governed by | |||
| // controller feature gates, such as DefaultPrivateKeyRotationPolicyAlways. | |||
| // We deep copy the object to avoid mutating the client-go cache. | |||
| crt = crt.DeepCopy() | |||
| cminternal.SetRuntimeDefaults_Certificate(crt) | |||
There was a problem hiding this comment.
Alternatively, SetRuntimeDefaults_Certificate could return a copy instead of modifying the value that was passed in.
There was a problem hiding this comment.
I considered that, because I think that would be more intuitive and less error prone.
But I also thought it worth keeping the semantics the same as standard defaulting functions and imagined there might be cases where the deep copy had already been made by a parent controller in which case it would be wasteful to do an extra deep copy.
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: inteon The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/unhold |
See #7723 (comment) for context.
/kind bug