Skip to content

Commit 1e0324c

Browse files
stevekuznetsovstevekuznetsov
authored
olm,catalog: only validate the resources we label (#3165)
* *: don't duplicate owner references Signed-off-by: Steve Kuznetsov <[email protected]> * olm,catalog: only validate the resources we label Each controller can see (and, therefore, can label) a separate set of GVRs. When we start up, detecting if any OLM-related resource needs labelling means that one controller may start, detect a need for labelling a resource it cannot itself label, detect that it's labelled everything it can, and restart. If the other operator is stuck for whatever reason, this leads the first controller to enter CrashLoopBackOff and break OCP upgrade. Signed-off-by: Steve Kuznetsov <[email protected]> * catalog: prune duplicate OwnerReferences on Services Signed-off-by: Steve Kuznetsov <[email protected]> --------- Signed-off-by: Steve Kuznetsov <[email protected]>
1 parent 1bb6009 commit 1e0324c

File tree

6 files changed

+149
-18
lines changed

6 files changed

+149
-18
lines changed

‎pkg/controller/operators/catalog/operator.go

Lines changed: 48 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -186,7 +186,7 @@ func NewOperator(ctx context.Context, kubeconfigPath string, clock utilclock.Clo
186186
return nil, err
187187
}
188188

189-
canFilter, err := labeller.Validate(ctx, logger, metadataClient, crClient)
189+
canFilter, err := labeller.Validate(ctx, logger, metadataClient, crClient, labeller.IdentityCatalogOperator)
190190
if err != nil {
191191
return nil, err
192192
}
@@ -541,6 +541,49 @@ func NewOperator(ctx context.Context, kubeconfigPath string, clock utilclock.Clo
541541
return nil, err
542542
}
543543

544+
{
545+
gvr := servicesgvk
546+
informer := serviceInformer.Informer()
547+
548+
logger := op.logger.WithFields(logrus.Fields{"gvr": gvr.String()})
549+
logger.Info("registering owner reference fixer")
550+
551+
queue := workqueue.NewRateLimitingQueueWithConfig(workqueue.DefaultControllerRateLimiter(), workqueue.RateLimitingQueueConfig{
552+
Name: gvr.String(),
553+
})
554+
queueInformer, err := queueinformer.NewQueueInformer(
555+
ctx,
556+
queueinformer.WithQueue(queue),
557+
queueinformer.WithLogger(op.logger),
558+
queueinformer.WithInformer(informer),
559+
queueinformer.WithSyncer(queueinformer.LegacySyncHandler(func(obj interface{}) error {
560+
service, ok := obj.(*corev1.Service)
561+
if !ok {
562+
err := fmt.Errorf("wrong type %T, expected %T: %#v", obj, new(*corev1.Service), obj)
563+
logger.WithError(err).Error("casting failed")
564+
return fmt.Errorf("casting failed: %w", err)
565+
}
566+
567+
deduped := deduplicateOwnerReferences(service.OwnerReferences)
568+
if len(deduped) != len(service.OwnerReferences) {
569+
localCopy := service.DeepCopy()
570+
localCopy.OwnerReferences = deduped
571+
if _, err := op.opClient.KubernetesInterface().CoreV1().Services(service.Namespace).Update(ctx, localCopy, metav1.UpdateOptions{}); err != nil {
572+
return err
573+
}
574+
}
575+
return nil
576+
}).ToSyncer()),
577+
)
578+
if err != nil {
579+
return nil, err
580+
}
581+
582+
if err := op.RegisterQueueInformer(queueInformer); err != nil {
583+
return nil, err
584+
}
585+
}
586+
544587
// Wire Pods for CatalogSource
545588
catsrcReq, err := labels.NewRequirement(reconciler.CatalogSourceLabelKey, selection.Exists, nil)
546589
if err != nil {
@@ -2860,3 +2903,7 @@ func (o *Operator) apiresourceFromGVK(gvk schema.GroupVersionKind) (metav1.APIRe
28602903
logger.Info("couldn't find GVK in api discovery")
28612904
return metav1.APIResource{}, olmerrors.GroupVersionKindNotFoundError{Group: gvk.Group, Version: gvk.Version, Kind: gvk.Kind}
28622905
}
2906+
2907+
func deduplicateOwnerReferences(refs []metav1.OwnerReference) []metav1.OwnerReference {
2908+
return sets.New(refs...).UnsortedList()
2909+
}

‎pkg/controller/operators/catalog/ownerrefs_test.go

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
1+
package catalog
2+
3+
import (
4+
"testing"
5+
6+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
7+
)
8+
9+
func TestDedupe(t *testing.T) {
10+
yes := true
11+
refs := []metav1.OwnerReference{
12+
{
13+
APIVersion: "apiversion",
14+
Kind: "kind",
15+
Name: "name",
16+
UID: "uid",
17+
Controller: &yes,
18+
BlockOwnerDeletion: &yes,
19+
},
20+
{
21+
APIVersion: "apiversion",
22+
Kind: "kind",
23+
Name: "name",
24+
UID: "uid",
25+
Controller: &yes,
26+
BlockOwnerDeletion: &yes,
27+
},
28+
{
29+
APIVersion: "apiversion",
30+
Kind: "kind",
31+
Name: "name",
32+
UID: "uid",
33+
Controller: &yes,
34+
BlockOwnerDeletion: &yes,
35+
},
36+
}
37+
deduped := deduplicateOwnerReferences(refs)
38+
t.Logf("got %d deduped from %d", len(deduped), len(refs))
39+
if len(deduped) == len(refs) {
40+
t.Errorf("didn't dedupe: %#v", deduped)
41+
}
42+
}

‎pkg/controller/operators/labeller/filters.go

Lines changed: 37 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -80,7 +80,39 @@ var filters = map[schema.GroupVersionResource]func(metav1.Object) bool{
8080
},
8181
}
8282

83-
func Validate(ctx context.Context, logger *logrus.Logger, metadataClient metadata.Interface, operatorClient operators.Interface) (bool, error) {
83+
type Identity string
84+
85+
const (
86+
IdentityCatalogOperator = "catalog-operator"
87+
IdentityOLMOperator = "olm-operator"
88+
)
89+
90+
func gvrRelevant(identity Identity) func(schema.GroupVersionResource) bool {
91+
switch identity {
92+
case IdentityCatalogOperator:
93+
return sets.New(
94+
rbacv1.SchemeGroupVersion.WithResource("roles"),
95+
rbacv1.SchemeGroupVersion.WithResource("rolebindings"),
96+
corev1.SchemeGroupVersion.WithResource("serviceaccounts"),
97+
corev1.SchemeGroupVersion.WithResource("services"),
98+
corev1.SchemeGroupVersion.WithResource("pods"),
99+
corev1.SchemeGroupVersion.WithResource("configmaps"),
100+
batchv1.SchemeGroupVersion.WithResource("jobs"),
101+
apiextensionsv1.SchemeGroupVersion.WithResource("customresourcedefinitions"),
102+
).Has
103+
case IdentityOLMOperator:
104+
return sets.New(
105+
appsv1.SchemeGroupVersion.WithResource("deployments"),
106+
rbacv1.SchemeGroupVersion.WithResource("clusterroles"),
107+
rbacv1.SchemeGroupVersion.WithResource("clusterrolebindings"),
108+
apiextensionsv1.SchemeGroupVersion.WithResource("customresourcedefinitions"),
109+
).Has
110+
default:
111+
panic("programmer error: invalid identity")
112+
}
113+
}
114+
115+
func Validate(ctx context.Context, logger *logrus.Logger, metadataClient metadata.Interface, operatorClient operators.Interface, identity Identity) (bool, error) {
84116
okLock := sync.Mutex{}
85117
ok := true
86118
g, ctx := errgroup.WithContext(ctx)
@@ -125,6 +157,10 @@ func Validate(ctx context.Context, logger *logrus.Logger, metadataClient metadat
125157
})
126158

127159
for gvr, filter := range allFilters {
160+
if !gvrRelevant(identity)(gvr) {
161+
logger.WithField("gvr", gvr.String()).Info("skipping irrelevant gvr")
162+
continue
163+
}
128164
gvr, filter := gvr, filter
129165
g.Go(func() error {
130166
list, err := metadataClient.Resource(gvr).List(ctx, metav1.ListOptions{})

‎pkg/controller/operators/olm/operator.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -183,7 +183,7 @@ func newOperatorWithConfig(ctx context.Context, config *operatorConfig) (*Operat
183183
return nil, err
184184
}
185185

186-
canFilter, err := labeller.Validate(ctx, config.logger, config.metadataClient, config.externalClient)
186+
canFilter, err := labeller.Validate(ctx, config.logger, config.metadataClient, config.externalClient, labeller.IdentityOLMOperator)
187187
if err != nil {
188188
return nil, err
189189
}

‎pkg/lib/ownerutil/util.go

Lines changed: 12 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -167,20 +167,13 @@ func AddNonBlockingOwner(object metav1.Object, owner Owner) {
167167
ownerRefs = []metav1.OwnerReference{}
168168
}
169169

170-
// Infer TypeMeta for the target
171-
if err := InferGroupVersionKind(owner); err != nil {
172-
log.Warn(err.Error())
173-
}
174-
gvk := owner.GetObjectKind().GroupVersionKind()
175-
170+
nonBlockingOwner := NonBlockingOwner(owner)
176171
for _, item := range ownerRefs {
177-
if item.Kind == gvk.Kind {
178-
if item.Name == owner.GetName() && item.UID == owner.GetUID() {
179-
return
180-
}
172+
if item.Kind == nonBlockingOwner.Kind && item.Name == nonBlockingOwner.Name && item.UID == nonBlockingOwner.UID {
173+
return
181174
}
182175
}
183-
ownerRefs = append(ownerRefs, NonBlockingOwner(owner))
176+
ownerRefs = append(ownerRefs, nonBlockingOwner)
184177
object.SetOwnerReferences(ownerRefs)
185178
}
186179

@@ -284,14 +277,20 @@ func AddOwner(object metav1.Object, owner Owner, blockOwnerDeletion, isControlle
284277
}
285278
gvk := owner.GetObjectKind().GroupVersionKind()
286279
apiVersion, kind := gvk.ToAPIVersionAndKind()
287-
ownerRefs = append(ownerRefs, metav1.OwnerReference{
280+
ownerRef := metav1.OwnerReference{
288281
APIVersion: apiVersion,
289282
Kind: kind,
290283
Name: owner.GetName(),
291284
UID: owner.GetUID(),
292285
BlockOwnerDeletion: &blockOwnerDeletion,
293286
Controller: &isController,
294-
})
287+
}
288+
for _, ref := range ownerRefs {
289+
if ref.Kind == ownerRef.Kind && ref.Name == ownerRef.Name && ref.UID == ownerRef.UID {
290+
return
291+
}
292+
}
293+
ownerRefs = append(ownerRefs, ownerRef)
295294
object.SetOwnerReferences(ownerRefs)
296295
}
297296

‎pkg/lib/testobj/runtime.go

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -135,14 +135,21 @@ func WithOwner(owner, obj RuntimeMetaObject) RuntimeMetaObject {
135135
out := obj.DeepCopyObject().(RuntimeMetaObject)
136136
gvk := owner.GetObjectKind().GroupVersionKind()
137137
apiVersion, kind := gvk.ToAPIVersionAndKind()
138-
refs := append(out.GetOwnerReferences(), metav1.OwnerReference{
138+
139+
nonBlockingOwner := metav1.OwnerReference{
139140
APIVersion: apiVersion,
140141
Kind: kind,
141142
Name: owner.GetName(),
142143
UID: owner.GetUID(),
143144
BlockOwnerDeletion: &dontBlockOwnerDeletion,
144145
Controller: &notController,
145-
})
146+
}
147+
for _, item := range out.GetOwnerReferences() {
148+
if item.Kind == nonBlockingOwner.Kind && item.Name == nonBlockingOwner.Name && item.UID == nonBlockingOwner.UID {
149+
return out
150+
}
151+
}
152+
refs := append(out.GetOwnerReferences(), nonBlockingOwner)
146153
out.SetOwnerReferences(refs)
147154

148155
return out

0 commit comments

Comments
 (0)