Skip to content

Commit de9700b

Browse files
camilamacedo86camilamacedo86
authored
fix(boxcutter): detect collision when duplicate package is installed after upgrade (#2578)
1 parent 09d32fb commit de9700b

File tree

4 files changed

+348
-1
lines changed

4 files changed

+348
-1
lines changed

‎internal/operator-controller/controllers/clusterextensionrevision_controller.go‎

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -145,6 +145,12 @@ func (c *ClusterExtensionRevisionReconciler) reconcile(ctx context.Context, cer
145145
return ctrl.Result{}, fmt.Errorf("converting to boxcutter revision: %v", err)
146146
}
147147

148+
siblings, err := c.siblingRevisionNames(ctx, cer)
149+
if err != nil {
150+
setRetryingConditions(cer, err.Error())
151+
return ctrl.Result{}, fmt.Errorf("listing sibling revisions: %v", err)
152+
}
153+
148154
revisionEngine, err := c.RevisionEngineFactory.CreateRevisionEngine(ctx, cer)
149155
if err != nil {
150156
setRetryingConditions(cer, err.Error())
@@ -207,6 +213,11 @@ func (c *ClusterExtensionRevisionReconciler) reconcile(ctx context.Context, cer
207213
if ores.Action() == machinery.ActionCollision {
208214
collidingObjs = append(collidingObjs, ores.String())
209215
}
216+
if ores.Action() == machinery.ActionProgressed && siblings != nil {
217+
if ref := foreignRevisionController(ores.Object(), siblings); ref != nil {
218+
collidingObjs = append(collidingObjs, ores.String()+fmt.Sprintf("\nConflicting Owner: %s", ref.String()))
219+
}
220+
}
210221
}
211222

212223
if len(collidingObjs) > 0 {
@@ -517,6 +528,42 @@ func EffectiveCollisionProtection(cp ...ocv1.CollisionProtection) ocv1.Collision
517528
return ecp
518529
}
519530

531+
// siblingRevisionNames returns the names of all ClusterExtensionRevisions that belong to
532+
// the same ClusterExtension as cer. Returns nil when cer has no owner label.
533+
func (c *ClusterExtensionRevisionReconciler) siblingRevisionNames(ctx context.Context, cer *ocv1.ClusterExtensionRevision) (sets.Set[string], error) {
534+
ownerLabel, ok := cer.Labels[labels.OwnerNameKey]
535+
if !ok {
536+
return nil, nil
537+
}
538+
revList := &ocv1.ClusterExtensionRevisionList{}
539+
if err := c.TrackingCache.List(ctx, revList, client.MatchingLabels{
540+
labels.OwnerNameKey: ownerLabel,
541+
}); err != nil {
542+
return nil, fmt.Errorf("listing sibling revisions: %w", err)
543+
}
544+
names := sets.New[string]()
545+
for i := range revList.Items {
546+
names.Insert(revList.Items[i].Name)
547+
}
548+
return names, nil
549+
}
550+
551+
// foreignRevisionController returns the controller OwnerReference when obj is owned by a
552+
// ClusterExtensionRevision that is not in siblings (i.e. belongs to a different ClusterExtension).
553+
// Returns nil when the controller is a sibling or is not a ClusterExtensionRevision.
554+
func foreignRevisionController(obj metav1.Object, siblings sets.Set[string]) *metav1.OwnerReference {
555+
refs := obj.GetOwnerReferences()
556+
for i := range refs {
557+
if refs[i].Controller != nil && *refs[i].Controller &&
558+
refs[i].Kind == ocv1.ClusterExtensionRevisionKind &&
559+
refs[i].APIVersion == ocv1.GroupVersion.String() &&
560+
!siblings.Has(refs[i].Name) {
561+
return &refs[i]
562+
}
563+
}
564+
return nil
565+
}
566+
520567
// buildProgressionProbes creates a set of boxcutter probes from the fields provided in the CER's spec.progressionProbes.
521568
// Returns nil and an error if encountered while attempting to build the probes.
522569
func buildProgressionProbes(progressionProbes []ocv1.ProgressionProbe) (probing.And, error) {

‎internal/operator-controller/controllers/clusterextensionrevision_controller_test.go‎

Lines changed: 236 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1111,7 +1111,7 @@ func newTestClusterExtensionRevision(t *testing.T, revisionName string, ext *ocv
11111111
labels.ServiceAccountNamespaceKey: ext.Spec.Namespace,
11121112
},
11131113
Labels: map[string]string{
1114-
labels.OwnerNameKey: "test-ext",
1114+
labels.OwnerNameKey: ext.Name,
11151115
},
11161116
},
11171117
Spec: ocv1.ClusterExtensionRevisionSpec{
@@ -1344,6 +1344,241 @@ func (m *mockTrackingCache) Free(ctx context.Context, user client.Object) error
13441344
return nil
13451345
}
13461346

1347+
func Test_ClusterExtensionRevisionReconciler_Reconcile_ForeignRevisionCollision(t *testing.T) {
1348+
testScheme := newScheme(t)
1349+
1350+
for _, tc := range []struct {
1351+
name string
1352+
reconcilingRevisionName string
1353+
existingObjs func() []client.Object
1354+
revisionResult machinery.RevisionResult
1355+
expectCollision bool
1356+
}{
1357+
{
1358+
name: "progressed object owned by a foreign CER is treated as a collision",
1359+
reconcilingRevisionName: "ext-B-1",
1360+
existingObjs: func() []client.Object {
1361+
extA := &ocv1.ClusterExtension{
1362+
ObjectMeta: metav1.ObjectMeta{Name: "ext-A", UID: "ext-A-uid"},
1363+
Spec: ocv1.ClusterExtensionSpec{
1364+
Namespace: "ns-a",
1365+
ServiceAccount: ocv1.ServiceAccountReference{Name: "sa-a"},
1366+
Source: ocv1.SourceConfig{
1367+
SourceType: ocv1.SourceTypeCatalog,
1368+
Catalog: &ocv1.CatalogFilter{PackageName: "pkg"},
1369+
},
1370+
},
1371+
}
1372+
extB := &ocv1.ClusterExtension{
1373+
ObjectMeta: metav1.ObjectMeta{Name: "ext-B", UID: "ext-B-uid"},
1374+
Spec: ocv1.ClusterExtensionSpec{
1375+
Namespace: "ns-b",
1376+
ServiceAccount: ocv1.ServiceAccountReference{Name: "sa-b"},
1377+
Source: ocv1.SourceConfig{
1378+
SourceType: ocv1.SourceTypeCatalog,
1379+
Catalog: &ocv1.CatalogFilter{PackageName: "pkg"},
1380+
},
1381+
},
1382+
}
1383+
cerA2 := newTestClusterExtensionRevision(t, "ext-A-2", extA, testScheme)
1384+
cerB1 := newTestClusterExtensionRevision(t, "ext-B-1", extB, testScheme)
1385+
return []client.Object{extA, extB, cerA2, cerB1}
1386+
},
1387+
revisionResult: mockRevisionResult{
1388+
phases: []machinery.PhaseResult{
1389+
mockPhaseResult{
1390+
name: "everything",
1391+
objects: []machinery.ObjectResult{
1392+
mockObjectResult{
1393+
action: machinery.ActionProgressed,
1394+
object: &unstructured.Unstructured{
1395+
Object: map[string]interface{}{
1396+
"apiVersion": "apiextensions.k8s.io/v1",
1397+
"kind": "CustomResourceDefinition",
1398+
"metadata": map[string]interface{}{
1399+
"name": "widgets.example.com",
1400+
"ownerReferences": []interface{}{
1401+
map[string]interface{}{
1402+
"apiVersion": ocv1.GroupVersion.String(),
1403+
"kind": ocv1.ClusterExtensionRevisionKind,
1404+
"name": "ext-A-2",
1405+
"uid": "ext-A-2",
1406+
"controller": true,
1407+
"blockOwnerDeletion": true,
1408+
},
1409+
},
1410+
},
1411+
},
1412+
},
1413+
probes: machinerytypes.ProbeResultContainer{
1414+
boxcutter.ProgressProbeType: {
1415+
Status: machinerytypes.ProbeStatusTrue,
1416+
},
1417+
},
1418+
},
1419+
},
1420+
},
1421+
},
1422+
},
1423+
expectCollision: true,
1424+
},
1425+
{
1426+
name: "progressed object owned by a sibling CER is not a collision",
1427+
reconcilingRevisionName: "ext-A-1",
1428+
existingObjs: func() []client.Object {
1429+
extA := &ocv1.ClusterExtension{
1430+
ObjectMeta: metav1.ObjectMeta{Name: "ext-A", UID: "ext-A-uid"},
1431+
Spec: ocv1.ClusterExtensionSpec{
1432+
Namespace: "ns-a",
1433+
ServiceAccount: ocv1.ServiceAccountReference{Name: "sa-a"},
1434+
Source: ocv1.SourceConfig{
1435+
SourceType: ocv1.SourceTypeCatalog,
1436+
Catalog: &ocv1.CatalogFilter{PackageName: "pkg"},
1437+
},
1438+
},
1439+
}
1440+
cerA1 := newTestClusterExtensionRevision(t, "ext-A-1", extA, testScheme)
1441+
cerA2 := newTestClusterExtensionRevision(t, "ext-A-2", extA, testScheme)
1442+
return []client.Object{extA, cerA1, cerA2}
1443+
},
1444+
revisionResult: mockRevisionResult{
1445+
phases: []machinery.PhaseResult{
1446+
mockPhaseResult{
1447+
name: "everything",
1448+
objects: []machinery.ObjectResult{
1449+
mockObjectResult{
1450+
action: machinery.ActionProgressed,
1451+
object: &unstructured.Unstructured{
1452+
Object: map[string]interface{}{
1453+
"apiVersion": "apiextensions.k8s.io/v1",
1454+
"kind": "CustomResourceDefinition",
1455+
"metadata": map[string]interface{}{
1456+
"name": "widgets.example.com",
1457+
"ownerReferences": []interface{}{
1458+
map[string]interface{}{
1459+
"apiVersion": ocv1.GroupVersion.String(),
1460+
"kind": ocv1.ClusterExtensionRevisionKind,
1461+
"name": "ext-A-2",
1462+
"uid": "ext-A-2",
1463+
"controller": true,
1464+
"blockOwnerDeletion": true,
1465+
},
1466+
},
1467+
},
1468+
},
1469+
},
1470+
probes: machinerytypes.ProbeResultContainer{
1471+
boxcutter.ProgressProbeType: {
1472+
Status: machinerytypes.ProbeStatusTrue,
1473+
},
1474+
},
1475+
},
1476+
},
1477+
},
1478+
},
1479+
},
1480+
expectCollision: false,
1481+
},
1482+
{
1483+
name: "progressed object owned by a non-CER controller is not a collision",
1484+
reconcilingRevisionName: "ext-B-1",
1485+
existingObjs: func() []client.Object {
1486+
extB := &ocv1.ClusterExtension{
1487+
ObjectMeta: metav1.ObjectMeta{Name: "ext-B", UID: "ext-B-uid"},
1488+
Spec: ocv1.ClusterExtensionSpec{
1489+
Namespace: "ns-b",
1490+
ServiceAccount: ocv1.ServiceAccountReference{Name: "sa-b"},
1491+
Source: ocv1.SourceConfig{
1492+
SourceType: ocv1.SourceTypeCatalog,
1493+
Catalog: &ocv1.CatalogFilter{PackageName: "pkg"},
1494+
},
1495+
},
1496+
}
1497+
cerB1 := newTestClusterExtensionRevision(t, "ext-B-1", extB, testScheme)
1498+
return []client.Object{extB, cerB1}
1499+
},
1500+
revisionResult: mockRevisionResult{
1501+
phases: []machinery.PhaseResult{
1502+
mockPhaseResult{
1503+
name: "everything",
1504+
objects: []machinery.ObjectResult{
1505+
mockObjectResult{
1506+
action: machinery.ActionProgressed,
1507+
object: &unstructured.Unstructured{
1508+
Object: map[string]interface{}{
1509+
"apiVersion": "v1",
1510+
"kind": "ConfigMap",
1511+
"metadata": map[string]interface{}{
1512+
"name": "some-cm",
1513+
"namespace": "default",
1514+
"ownerReferences": []interface{}{
1515+
map[string]interface{}{
1516+
"apiVersion": "apps/v1",
1517+
"kind": "Deployment",
1518+
"name": "some-deployment",
1519+
"uid": "deploy-uid",
1520+
"controller": true,
1521+
"blockOwnerDeletion": true,
1522+
},
1523+
},
1524+
},
1525+
},
1526+
},
1527+
probes: machinerytypes.ProbeResultContainer{
1528+
boxcutter.ProgressProbeType: {
1529+
Status: machinerytypes.ProbeStatusTrue,
1530+
},
1531+
},
1532+
},
1533+
},
1534+
},
1535+
},
1536+
},
1537+
expectCollision: false,
1538+
},
1539+
} {
1540+
t.Run(tc.name, func(t *testing.T) {
1541+
testClient := fake.NewClientBuilder().
1542+
WithScheme(testScheme).
1543+
WithStatusSubresource(&ocv1.ClusterExtensionRevision{}).
1544+
WithObjects(tc.existingObjs()...).
1545+
Build()
1546+
1547+
mockEngine := &mockRevisionEngine{
1548+
reconcile: func(ctx context.Context, rev machinerytypes.Revision, opts ...machinerytypes.RevisionReconcileOption) (machinery.RevisionResult, error) {
1549+
return tc.revisionResult, nil
1550+
},
1551+
}
1552+
result, err := (&controllers.ClusterExtensionRevisionReconciler{
1553+
Client: testClient,
1554+
RevisionEngineFactory: &mockRevisionEngineFactory{engine: mockEngine},
1555+
TrackingCache: &mockTrackingCache{client: testClient},
1556+
}).Reconcile(t.Context(), ctrl.Request{
1557+
NamespacedName: types.NamespacedName{
1558+
Name: tc.reconcilingRevisionName,
1559+
},
1560+
})
1561+
1562+
if tc.expectCollision {
1563+
require.Equal(t, ctrl.Result{RequeueAfter: 10 * time.Second}, result)
1564+
require.NoError(t, err)
1565+
1566+
rev := &ocv1.ClusterExtensionRevision{}
1567+
require.NoError(t, testClient.Get(t.Context(), client.ObjectKey{Name: tc.reconcilingRevisionName}, rev))
1568+
cond := meta.FindStatusCondition(rev.Status.Conditions, ocv1.ClusterExtensionRevisionTypeProgressing)
1569+
require.NotNil(t, cond)
1570+
require.Equal(t, metav1.ConditionTrue, cond.Status)
1571+
require.Equal(t, ocv1.ClusterExtensionRevisionReasonRetrying, cond.Reason)
1572+
require.Contains(t, cond.Message, "revision object collisions")
1573+
require.Contains(t, cond.Message, "Conflicting Owner")
1574+
} else {
1575+
require.Equal(t, ctrl.Result{}, result)
1576+
require.NoError(t, err)
1577+
}
1578+
})
1579+
}
1580+
}
1581+
13471582
func Test_effectiveCollisionProtection(t *testing.T) {
13481583
for _, tc := range []struct {
13491584
name string

‎test/e2e/features/update.feature‎

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -210,6 +210,58 @@ Feature: Update ClusterExtension
210210
And ClusterExtension is available
211211
And bundle "test-operator.1.0.4" is installed in version "1.0.4"
212212

213+
@BoxcutterRuntime
214+
Scenario: Detect collision when a second ClusterExtension installs the same package after an upgrade
215+
Given ClusterExtension is applied
216+
"""
217+
apiVersion: olm.operatorframework.io/v1
218+
kind: ClusterExtension
219+
metadata:
220+
name: ${NAME}
221+
spec:
222+
namespace: ${TEST_NAMESPACE}
223+
serviceAccount:
224+
name: olm-sa
225+
source:
226+
sourceType: Catalog
227+
catalog:
228+
packageName: test
229+
selector:
230+
matchLabels:
231+
"olm.operatorframework.io/metadata.name": test-catalog
232+
version: 1.0.0
233+
"""
234+
And ClusterExtension is rolled out
235+
And ClusterExtension is available
236+
When ClusterExtension is updated to version "1.0.1"
237+
Then ClusterExtension is rolled out
238+
And ClusterExtension is available
239+
And bundle "test-operator.1.0.1" is installed in version "1.0.1"
240+
And the current ClusterExtension is tracked for cleanup
241+
When ClusterExtension is applied
242+
"""
243+
apiVersion: olm.operatorframework.io/v1
244+
kind: ClusterExtension
245+
metadata:
246+
name: ${NAME}-dup
247+
spec:
248+
namespace: ${TEST_NAMESPACE}
249+
serviceAccount:
250+
name: olm-sa
251+
source:
252+
sourceType: Catalog
253+
catalog:
254+
packageName: test
255+
selector:
256+
matchLabels:
257+
"olm.operatorframework.io/metadata.name": test-catalog
258+
version: 1.0.1
259+
"""
260+
Then ClusterExtension reports Progressing as True with Reason Retrying and Message includes:
261+
"""
262+
revision object collisions
263+
"""
264+
213265
@BoxcutterRuntime
214266
Scenario: Each update creates a new revision and resources not present in the new revision are removed from the cluster
215267
Given ClusterExtension is applied

0 commit comments

Comments
 (0)