Skip to content

Commit 60d79e3

Browse files
committed
fix(object-controller): retry referenced Secret failures
Read each referenced Secret once per reconciliation. Return transient read errors so the controller retries, and requeue while referenced Secrets remain mutable. Signed-off-by: Fabricio Aguiar <fabricio.aguiar@gmail.com> rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
1 parent aa82c0d commit 60d79e3

4 files changed

Lines changed: 233 additions & 18 deletions

File tree

‎internal/object-controller/controllers/clusterobjectset_controller.go‎

Lines changed: 18 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,6 @@ import (
1515
"time"
1616

1717
"github.com/go-logr/logr"
18-
corev1 "k8s.io/api/core/v1"
1918
"k8s.io/apimachinery/pkg/api/equality"
2019
apierrors "k8s.io/apimachinery/pkg/api/errors"
2120
"k8s.io/apimachinery/pkg/api/meta"
@@ -132,14 +131,20 @@ func (c *ClusterObjectSetReconciler) reconcile(ctx context.Context, cos *ocv1.Cl
132131
remaining, hasDeadline := durationUntilDeadline(c.Clock, cos)
133132
isDeadlineExceeded := hasDeadline && remaining <= 0
134133

134+
secretReader := newReferencedSecretReader(c.Client)
135135
// Blocked takes precedence over ProgressDeadlineExceeded: it is more actionable for the user.
136-
if err := c.verifyReferencedSecretsImmutable(ctx, cos); err != nil {
136+
if err := c.verifyReferencedSecretsImmutable(ctx, cos, secretReader); err != nil {
137+
var mutableSecrets *mutableSecretsError
138+
if !errors.As(err, &mutableSecrets) {
139+
setRetryingConditions(l, cos, err.Error(), isDeadlineExceeded)
140+
return ctrl.Result{}, err
141+
}
137142
l.Error(err, "referenced Secret verification failed, blocking reconciliation")
138143
markAsNotProgressing(cos, ocv1.ClusterObjectSetReasonBlocked, err.Error())
139-
return ctrl.Result{}, nil
144+
return ctrl.Result{RequeueAfter: 10 * time.Second}, nil
140145
}
141146

142-
phases, currentPhases, opts, err := c.buildBoxcutterPhases(ctx, cos)
147+
phases, currentPhases, opts, err := c.buildBoxcutterPhases(ctx, cos, secretReader)
143148
if err != nil {
144149
setRetryingConditions(l, cos, err.Error(), isDeadlineExceeded)
145150
return ctrl.Result{}, fmt.Errorf("converting to boxcutter revision: %v", err)
@@ -467,7 +472,7 @@ func (c *ClusterObjectSetReconciler) listOtherActiveRevisions(
467472
return result, nil
468473
}
469474

470-
func (c *ClusterObjectSetReconciler) buildBoxcutterPhases(ctx context.Context, cos *ocv1.ClusterObjectSet) ([]boxcutter.Phase, []ocv1.ObservedPhase, []boxcutter.RevisionReconcileOption, error) {
475+
func (c *ClusterObjectSetReconciler) buildBoxcutterPhases(ctx context.Context, cos *ocv1.ClusterObjectSet, secretReader *referencedSecretReader) ([]boxcutter.Phase, []ocv1.ObservedPhase, []boxcutter.RevisionReconcileOption, error) {
471476
siblings, err := c.listSiblingRevisions(ctx, cos)
472477
if err != nil {
473478
return nil, nil, nil, fmt.Errorf("listing sibling revisions: %w", err)
@@ -499,7 +504,7 @@ func (c *ClusterObjectSetReconciler) buildBoxcutterPhases(ctx context.Context, c
499504
case specObj.Object.Object != nil:
500505
obj = specObj.Object.DeepCopy()
501506
case specObj.Ref.Name != "":
502-
resolved, err := c.resolveObjectRef(ctx, specObj.Ref)
507+
resolved, err := secretReader.resolveObjectRef(ctx, specObj.Ref)
503508
if err != nil {
504509
return nil, nil, nil, fmt.Errorf("resolving ref in phase %q: %w", specPhase.Name, err)
505510
}
@@ -541,10 +546,10 @@ func (c *ClusterObjectSetReconciler) buildBoxcutterPhases(ctx context.Context, c
541546

542547
// resolveObjectRef fetches the referenced Secret, reads the value at the specified key,
543548
// auto-detects gzip compression, and deserializes into an unstructured.Unstructured.
544-
func (c *ClusterObjectSetReconciler) resolveObjectRef(ctx context.Context, ref ocv1.ObjectSourceRef) (*unstructured.Unstructured, error) {
545-
secret := &corev1.Secret{}
549+
func (r *referencedSecretReader) resolveObjectRef(ctx context.Context, ref ocv1.ObjectSourceRef) (*unstructured.Unstructured, error) {
546550
key := client.ObjectKey{Name: ref.Name, Namespace: ref.Namespace}
547-
if err := c.Client.Get(ctx, key, secret); err != nil {
551+
secret, err := r.get(ctx, key)
552+
if err != nil {
548553
return nil, fmt.Errorf("getting Secret %s/%s: %w", ref.Namespace, ref.Name, err)
549554
}
550555

@@ -777,7 +782,7 @@ func verifyObservedPhases(stored, current []ocv1.ObservedPhase) error {
777782
// verifyReferencedSecretsImmutable checks that all referenced Secrets
778783
// have Immutable set to true. It collects all violations and returns
779784
// a single error listing every misconfigured Secret.
780-
func (c *ClusterObjectSetReconciler) verifyReferencedSecretsImmutable(ctx context.Context, cos *ocv1.ClusterObjectSet) error {
785+
func (c *ClusterObjectSetReconciler) verifyReferencedSecretsImmutable(ctx context.Context, cos *ocv1.ClusterObjectSet, secretReader *referencedSecretReader) error {
781786
type secretRef struct {
782787
name string
783788
namespace string
@@ -800,9 +805,9 @@ func (c *ClusterObjectSetReconciler) verifyReferencedSecretsImmutable(ctx contex
800805

801806
var mutableSecrets []string
802807
for _, ref := range refs {
803-
secret := &corev1.Secret{}
804808
key := client.ObjectKey{Name: ref.name, Namespace: ref.namespace}
805-
if err := c.Client.Get(ctx, key, secret); err != nil {
809+
secret, err := secretReader.get(ctx, key)
810+
if err != nil {
806811
if apierrors.IsNotFound(err) {
807812
// Secret not yet available — skip verification.
808813
// resolveObjectRef will handle the not-found with a retryable error.
@@ -817,8 +822,7 @@ func (c *ClusterObjectSetReconciler) verifyReferencedSecretsImmutable(ctx contex
817822
}
818823

819824
if len(mutableSecrets) > 0 {
820-
return fmt.Errorf("the following secrets are not immutable (referenced secrets must have immutable set to true): %s",
821-
strings.Join(mutableSecrets, ", "))
825+
return &mutableSecretsError{names: mutableSecrets}
822826
}
823827

824828
return nil

‎internal/object-controller/controllers/clusterobjectset_controller_internal_test.go‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -436,7 +436,7 @@ func TestVerifyReferencedSecretsImmutable(t *testing.T) {
436436
},
437437
}
438438

439-
err := reconciler.verifyReferencedSecretsImmutable(t.Context(), cos)
439+
err := reconciler.verifyReferencedSecretsImmutable(t.Context(), cos, newReferencedSecretReader(testClient))
440440
require.NoError(t, err)
441441
})
442442

@@ -466,7 +466,7 @@ func TestVerifyReferencedSecretsImmutable(t *testing.T) {
466466
},
467467
}
468468

469-
err := reconciler.verifyReferencedSecretsImmutable(t.Context(), cos)
469+
err := reconciler.verifyReferencedSecretsImmutable(t.Context(), cos, newReferencedSecretReader(testClient))
470470
require.Error(t, err)
471471
assert.Contains(t, err.Error(), "not immutable")
472472
})
@@ -489,7 +489,7 @@ func TestVerifyReferencedSecretsImmutable(t *testing.T) {
489489
},
490490
}
491491

492-
err := reconciler.verifyReferencedSecretsImmutable(t.Context(), cos)
492+
err := reconciler.verifyReferencedSecretsImmutable(t.Context(), cos, newReferencedSecretReader(testClient))
493493
require.NoError(t, err)
494494
})
495495

@@ -523,7 +523,7 @@ func TestVerifyReferencedSecretsImmutable(t *testing.T) {
523523
},
524524
}
525525

526-
err := reconciler.verifyReferencedSecretsImmutable(t.Context(), cos)
526+
err := reconciler.verifyReferencedSecretsImmutable(t.Context(), cos, newReferencedSecretReader(testClient))
527527
require.NoError(t, err)
528528
assert.Equal(t, int32(1), secretGetCount.Load(), "secret should be fetched only once despite multiple references")
529529
})
Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
1+
package controllers
2+
3+
import (
4+
"context"
5+
"fmt"
6+
"strings"
7+
8+
corev1 "k8s.io/api/core/v1"
9+
"sigs.k8s.io/controller-runtime/pkg/client"
10+
)
11+
12+
// referencedSecretReader shares a Secret snapshot between verification and object
13+
// decoding within one reconciliation. A new reader must be used on the next
14+
// reconciliation so that deleted and recreated Secrets are read again.
15+
type referencedSecretReader struct {
16+
reader client.Reader
17+
secrets map[client.ObjectKey]*corev1.Secret
18+
}
19+
20+
func newReferencedSecretReader(reader client.Reader) *referencedSecretReader {
21+
return &referencedSecretReader{reader: reader, secrets: make(map[client.ObjectKey]*corev1.Secret)}
22+
}
23+
24+
func (r *referencedSecretReader) get(ctx context.Context, key client.ObjectKey) (*corev1.Secret, error) {
25+
if secret, ok := r.secrets[key]; ok {
26+
return secret, nil
27+
}
28+
secret := &corev1.Secret{}
29+
if err := r.reader.Get(ctx, key, secret); err != nil {
30+
return nil, err
31+
}
32+
r.secrets[key] = secret
33+
return secret, nil
34+
}
35+
36+
type mutableSecretsError struct {
37+
names []string
38+
}
39+
40+
func (e *mutableSecretsError) Error() string {
41+
return fmt.Sprintf("the following secrets are not immutable (referenced secrets must have immutable set to true): %s", strings.Join(e.names, ", "))
42+
}
Lines changed: 169 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,169 @@
1+
package controllers_test
2+
3+
import (
4+
"context"
5+
"fmt"
6+
"testing"
7+
"time"
8+
9+
"github.com/stretchr/testify/require"
10+
"go.uber.org/mock/gomock"
11+
corev1 "k8s.io/api/core/v1"
12+
apierrors "k8s.io/apimachinery/pkg/api/errors"
13+
"k8s.io/apimachinery/pkg/api/meta"
14+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
15+
clocktesting "k8s.io/utils/clock/testing"
16+
"k8s.io/utils/ptr"
17+
"pkg.package-operator.run/boxcutter/machinery"
18+
machinerytypes "pkg.package-operator.run/boxcutter/machinery/types"
19+
ctrl "sigs.k8s.io/controller-runtime"
20+
"sigs.k8s.io/controller-runtime/pkg/client"
21+
"sigs.k8s.io/controller-runtime/pkg/client/fake"
22+
"sigs.k8s.io/controller-runtime/pkg/client/interceptor"
23+
24+
ocv1 "github.com/operator-framework/operator-controller/api/v1"
25+
"github.com/operator-framework/operator-controller/internal/object-controller/controllers"
26+
)
27+
28+
func TestReferencedSecretsReadOncePerReconcile(t *testing.T) {
29+
ctx := t.Context()
30+
mockCtrl := gomock.NewController(t)
31+
secret := &corev1.Secret{
32+
ObjectMeta: metav1.ObjectMeta{Name: "content", Namespace: "source-a"},
33+
Immutable: ptr.To(true), Data: map[string][]byte{},
34+
}
35+
cos := newRefTestCOS("packed", ocv1.ObjectSourceRef{})
36+
cos.Spec.Phases = nil
37+
for phaseIndex := range 2 {
38+
phase := ocv1.ClusterObjectSetPhase{Name: fmt.Sprintf("phase-%d", phaseIndex)}
39+
for objectIndex := range 25 {
40+
name := fmt.Sprintf("cm-%d-%d", phaseIndex, objectIndex)
41+
secret.Data[name] = []byte(fmt.Sprintf(`{"apiVersion":"v1","kind":"ConfigMap","metadata":{"name":%q,"namespace":"target"}}`, name))
42+
phase.Objects = append(phase.Objects, ocv1.ClusterObjectSetObject{Ref: ocv1.ObjectSourceRef{Name: secret.Name, Namespace: secret.Namespace, Key: name}})
43+
}
44+
cos.Spec.Phases = append(cos.Spec.Phases, phase)
45+
}
46+
otherSecret := &corev1.Secret{
47+
ObjectMeta: metav1.ObjectMeta{Name: secret.Name, Namespace: "source-b"},
48+
Immutable: ptr.To(true),
49+
Data: map[string][]byte{"other": []byte(`{"apiVersion":"v1","kind":"ConfigMap","metadata":{"name":"other","namespace":"target"}}`)},
50+
}
51+
cos.Spec.Phases[1].Objects = append(cos.Spec.Phases[1].Objects, ocv1.ClusterObjectSetObject{Ref: ocv1.ObjectSourceRef{Name: otherSecret.Name, Namespace: otherSecret.Namespace, Key: "other"}})
52+
reads := map[client.ObjectKey]int{}
53+
cl := fake.NewClientBuilder().WithScheme(newSchemeWithCoreV1(t)).WithObjects(secret, otherSecret, cos).
54+
WithStatusSubresource(&ocv1.ClusterObjectSet{}).
55+
WithInterceptorFuncs(interceptor.Funcs{
56+
Get: func(ctx context.Context, cl client.WithWatch, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error {
57+
if _, ok := obj.(*corev1.Secret); ok {
58+
reads[key]++
59+
}
60+
return cl.Get(ctx, key, obj, opts...)
61+
},
62+
}).Build()
63+
engine := newMockRevisionEngineWithReconcile(mockCtrl,
64+
func(context.Context, machinerytypes.Revision, ...machinerytypes.RevisionReconcileOption) (machinery.RevisionResult, error) {
65+
return newMockRevisionResult(mockCtrl, revisionResultConfig{inTransition: true}), nil
66+
}, nil)
67+
reconciler := &controllers.ClusterObjectSetReconciler{
68+
Client: cl, TrackingCache: newMockTrackingCache(mockCtrl, cl, nil),
69+
RevisionEngineFactory: newMockRevisionEngineFactoryWithEngine(mockCtrl, engine, nil),
70+
Clock: clocktesting.NewFakeClock(metav1.Now().Time),
71+
}
72+
reconcile := func(wantReads int, wantReason string) {
73+
t.Helper()
74+
_, err := reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: client.ObjectKeyFromObject(cos)})
75+
require.NoError(t, err)
76+
require.Equal(t, wantReads, reads[client.ObjectKeyFromObject(secret)])
77+
require.Equal(t, wantReads, reads[client.ObjectKeyFromObject(otherSecret)])
78+
require.NoError(t, cl.Get(ctx, client.ObjectKeyFromObject(cos), cos))
79+
condition := meta.FindStatusCondition(cos.Status.Conditions, ocv1.ClusterObjectSetTypeProgressing)
80+
require.NotNil(t, condition)
81+
require.Equal(t, wantReason, condition.Reason)
82+
}
83+
reconcile(1, ocv1.ReasonRollingOut)
84+
reconcile(2, ocv1.ReasonRollingOut)
85+
86+
// Reusing a fetched Secret must not hide a replacement on the next reconcile.
87+
require.NoError(t, cl.Delete(ctx, secret))
88+
changed := secret.DeepCopy()
89+
changed.ResourceVersion = ""
90+
changed.Data["cm-0-0"] = []byte(`{"apiVersion":"v1","kind":"ConfigMap","metadata":{"name":"cm-0-0","namespace":"target"},"data":{"value":"changed"}}`)
91+
require.NoError(t, cl.Create(ctx, changed))
92+
reconcile(3, ocv1.ClusterObjectSetReasonBlocked)
93+
require.Contains(t, meta.FindStatusCondition(cos.Status.Conditions, ocv1.ClusterObjectSetTypeProgressing).Message, "resolved content of 1 phase(s) has changed")
94+
95+
require.NoError(t, cl.Delete(ctx, changed))
96+
secret.ResourceVersion = ""
97+
require.NoError(t, cl.Create(ctx, secret))
98+
reconcile(4, ocv1.ReasonRollingOut)
99+
}
100+
101+
func TestReferencedSecretReadFailureRetries(t *testing.T) {
102+
ctx := t.Context()
103+
mockCtrl := gomock.NewController(t)
104+
secret := &corev1.Secret{
105+
ObjectMeta: metav1.ObjectMeta{Name: "content", Namespace: "source"},
106+
Immutable: ptr.To(true),
107+
Data: map[string][]byte{"object": []byte(`{"apiVersion":"v1","kind":"ConfigMap","metadata":{"name":"cm","namespace":"target"}}`)},
108+
}
109+
cos := newRefTestCOS("retry", ocv1.ObjectSourceRef{Name: secret.Name, Namespace: secret.Namespace, Key: "object"})
110+
failReads := true
111+
cl := fake.NewClientBuilder().WithScheme(newSchemeWithCoreV1(t)).WithObjects(secret, cos).
112+
WithStatusSubresource(&ocv1.ClusterObjectSet{}).
113+
WithInterceptorFuncs(interceptor.Funcs{
114+
Get: func(ctx context.Context, cl client.WithWatch, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error {
115+
if _, ok := obj.(*corev1.Secret); ok && failReads {
116+
return apierrors.NewServiceUnavailable("temporary API outage")
117+
}
118+
return cl.Get(ctx, key, obj, opts...)
119+
},
120+
}).Build()
121+
engine := newMockRevisionEngineWithReconcile(mockCtrl,
122+
func(context.Context, machinerytypes.Revision, ...machinerytypes.RevisionReconcileOption) (machinery.RevisionResult, error) {
123+
return newMockRevisionResult(mockCtrl, revisionResultConfig{inTransition: true}), nil
124+
}, nil)
125+
reconciler := &controllers.ClusterObjectSetReconciler{
126+
Client: cl, TrackingCache: newMockTrackingCache(mockCtrl, cl, nil),
127+
RevisionEngineFactory: newMockRevisionEngineFactoryWithEngine(mockCtrl, engine, nil),
128+
Clock: clocktesting.NewFakeClock(metav1.Now().Time),
129+
}
130+
req := ctrl.Request{NamespacedName: client.ObjectKeyFromObject(cos)}
131+
for range 2 {
132+
_, err := reconciler.Reconcile(ctx, req)
133+
require.ErrorContains(t, err, "temporary API outage")
134+
require.NoError(t, cl.Get(ctx, req.NamespacedName, cos))
135+
condition := meta.FindStatusCondition(cos.Status.Conditions, ocv1.ClusterObjectSetTypeProgressing)
136+
require.NotNil(t, condition)
137+
require.Equal(t, metav1.ConditionTrue, condition.Status)
138+
require.Equal(t, ocv1.ReasonRetrying, condition.Reason)
139+
}
140+
failReads = false
141+
_, err := reconciler.Reconcile(ctx, req)
142+
require.NoError(t, err)
143+
require.NoError(t, cl.Get(ctx, req.NamespacedName, cos))
144+
require.Equal(t, ocv1.ReasonRollingOut, meta.FindStatusCondition(cos.Status.Conditions, ocv1.ClusterObjectSetTypeProgressing).Reason)
145+
}
146+
147+
func TestMutableReferencedSecretRequeues(t *testing.T) {
148+
secret := &corev1.Secret{
149+
ObjectMeta: metav1.ObjectMeta{Name: "content", Namespace: "source"},
150+
Immutable: ptr.To(false),
151+
}
152+
cos := newRefTestCOS("wait-for-immutable-secret", ocv1.ObjectSourceRef{
153+
Name: secret.Name, Namespace: secret.Namespace, Key: "object",
154+
})
155+
cl := fake.NewClientBuilder().WithScheme(newSchemeWithCoreV1(t)).
156+
WithStatusSubresource(&ocv1.ClusterObjectSet{}).
157+
WithObjects(secret, cos).Build()
158+
reconciler := &controllers.ClusterObjectSetReconciler{Client: cl}
159+
160+
result, err := reconciler.Reconcile(t.Context(), ctrl.Request{NamespacedName: client.ObjectKeyFromObject(cos)})
161+
require.NoError(t, err)
162+
require.Equal(t, ctrl.Result{RequeueAfter: 10 * time.Second}, result)
163+
require.NoError(t, cl.Get(t.Context(), client.ObjectKeyFromObject(cos), cos))
164+
condition := meta.FindStatusCondition(cos.Status.Conditions, ocv1.ClusterObjectSetTypeProgressing)
165+
require.NotNil(t, condition)
166+
require.Equal(t, metav1.ConditionFalse, condition.Status)
167+
require.Equal(t, ocv1.ClusterObjectSetReasonBlocked, condition.Reason)
168+
require.Contains(t, condition.Message, "source/content")
169+
}

0 commit comments

Comments
 (0)