Skip to content

Commit b67b8fe

Browse files
erdiierdii
committed
fix: use ownership instead of revision to guard teardown deletion
When an Owner is provided, check whether we are the controller via detectOwner rather than comparing revision annotations. Owner refs are the authoritative signal for ownership and correctly handle orphaning deletions where the revision annotation may have been updated by a different reconciliation pass. Callers without an Owner fall back to the existing revision comparison.
1 parent 80bb241 commit b67b8fe

3 files changed

Lines changed: 202 additions & 43 deletions

File tree

‎machinery/objects.go‎

Lines changed: 14 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -151,19 +151,10 @@ func (e *ObjectEngine) Teardown(
151151
return false, fmt.Errorf("getting object before deletion: %w", err)
152152
}
153153

154-
// Check revision matches.
155-
actualRevision, err := e.getObjectRevision(actualObject)
156-
if err != nil {
157-
return false, fmt.Errorf("getting object revision: %w", err)
158-
}
159-
160-
// Object is not owned by this revision
161-
if actualRevision != revision {
162-
if options.Owner == nil {
163-
// No Owner to remove from the object, return.
164-
return true, nil
165-
}
166-
154+
if options.Owner != nil {
155+
// Check ownership instead of revision to determine if we should delete.
156+
// If we're not the controller, only remove our owner ref and leave the object in place.
157+
// A possible reason for this could be an orphaning deletion.
167158
ctrlSit, _ := e.detectOwner(options.Owner, options.OwnerStrategy, actualObject, nil)
168159
if ctrlSit != ctrlSituationIsController {
169160
// Remove us from owners list:
@@ -173,6 +164,16 @@ func (e *ObjectEngine) Teardown(
173164
// TODO should we check if the patch differs from actualObject before firing the request?
174165
return true, e.writer.Patch(ctx, patch, client.MergeFrom(actualObject))
175166
}
167+
} else {
168+
// No Owner to check against. Fall back to revision comparison.
169+
actualRevision, err := e.getObjectRevision(actualObject)
170+
if err != nil {
171+
return false, fmt.Errorf("getting object revision: %w", err)
172+
}
173+
174+
if actualRevision != revision {
175+
return true, nil
176+
}
176177
}
177178

178179
// Actually delete the object.

‎machinery/objects_test.go‎

Lines changed: 79 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -1177,6 +1177,7 @@ func TestObjectEngine_Teardown(t *testing.T) {
11771177
cache *cacheMock,
11781178
writer *testutil.CtrlClient,
11791179
actualObject *unstructured.Unstructured,
1180+
mode ownerMode,
11801181
)
11811182
modes []ownerMode // nil = both default modes
11821183
expectedGone bool
@@ -1191,19 +1192,10 @@ func TestObjectEngine_Teardown(t *testing.T) {
11911192
return []types.ObjectTeardownOption{types.WithOrphan()}
11921193
},
11931194
mockSetup: func(
1194-
cache *cacheMock, writer *testutil.CtrlClient,
1195-
actualObject *unstructured.Unstructured,
1195+
_ *cacheMock, writer *testutil.CtrlClient,
1196+
_ *unstructured.Unstructured, _ ownerMode,
11961197
) {
11971198
writer.On("Patch", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil)
1198-
cache.
1199-
On("Get", mock.Anything,
1200-
client.ObjectKey{Name: "testi", Namespace: "test"},
1201-
mock.Anything, mock.Anything).
1202-
Run(func(args mock.Arguments) {
1203-
obj := args.Get(2).(*unstructured.Unstructured)
1204-
*obj = *actualObject
1205-
}).
1206-
Return(nil)
12071199
},
12081200
expectedGone: true,
12091201
},
@@ -1213,7 +1205,7 @@ func TestObjectEngine_Teardown(t *testing.T) {
12131205
desiredObject: buildObj("testi", "test"),
12141206
mockSetup: func(
12151207
cache *cacheMock, _ *testutil.CtrlClient,
1216-
_ *unstructured.Unstructured,
1208+
_ *unstructured.Unstructured, _ ownerMode,
12171209
) {
12181210
cache.
12191211
On("Get", mock.Anything,
@@ -1229,7 +1221,7 @@ func TestObjectEngine_Teardown(t *testing.T) {
12291221
desiredObject: buildObj("testi", "test"),
12301222
mockSetup: func(
12311223
cache *cacheMock, _ *testutil.CtrlClient,
1232-
_ *unstructured.Unstructured,
1224+
_ *unstructured.Unstructured, _ ownerMode,
12331225
) {
12341226
cache.
12351227
On("Get", mock.Anything,
@@ -1249,7 +1241,7 @@ func TestObjectEngine_Teardown(t *testing.T) {
12491241
desiredObject: buildObj("testi", "test"),
12501242
mockSetup: func(
12511243
cache *cacheMock, _ *testutil.CtrlClient,
1252-
_ *unstructured.Unstructured,
1244+
_ *unstructured.Unstructured, _ ownerMode,
12531245
) {
12541246
cache.
12551247
On("Get", mock.Anything,
@@ -1266,7 +1258,7 @@ func TestObjectEngine_Teardown(t *testing.T) {
12661258
actualObject: buildObj("testi", "test", withRevision("1"), withManaged),
12671259
mockSetup: func(
12681260
cache *cacheMock, writer *testutil.CtrlClient,
1269-
actualObject *unstructured.Unstructured,
1261+
actualObject *unstructured.Unstructured, _ ownerMode,
12701262
) {
12711263
cache.
12721264
On("Get", mock.Anything,
@@ -1290,7 +1282,7 @@ func TestObjectEngine_Teardown(t *testing.T) {
12901282
actualObject: buildObj("testi", "test", withRevision("1"), withManaged),
12911283
mockSetup: func(
12921284
cache *cacheMock, writer *testutil.CtrlClient,
1293-
actualObject *unstructured.Unstructured,
1285+
actualObject *unstructured.Unstructured, _ ownerMode,
12941286
) {
12951287
cache.
12961288
On("Get", mock.Anything,
@@ -1314,7 +1306,7 @@ func TestObjectEngine_Teardown(t *testing.T) {
13141306
actualObject: buildObj("testi", "test", withRevision("1"), withManaged),
13151307
mockSetup: func(
13161308
cache *cacheMock, writer *testutil.CtrlClient,
1317-
actualObject *unstructured.Unstructured,
1309+
actualObject *unstructured.Unstructured, _ ownerMode,
13181310
) {
13191311
cache.
13201312
On("Get", mock.Anything,
@@ -1346,7 +1338,7 @@ func TestObjectEngine_Teardown(t *testing.T) {
13461338
},
13471339
mockSetup: func(
13481340
cache *cacheMock, writer *testutil.CtrlClient,
1349-
actualObject *unstructured.Unstructured,
1341+
actualObject *unstructured.Unstructured, _ ownerMode,
13501342
) {
13511343
cache.
13521344
On("Get", mock.Anything,
@@ -1357,8 +1349,6 @@ func TestObjectEngine_Teardown(t *testing.T) {
13571349
*obj = *actualObject
13581350
}).
13591351
Return(nil)
1360-
writer.On("Delete", mock.Anything, mock.Anything, mock.Anything).
1361-
Panic("Delete should not be called on the engine writer")
13621352
},
13631353
expectedGone: false,
13641354
},
@@ -1377,7 +1367,7 @@ func TestObjectEngine_Teardown(t *testing.T) {
13771367
},
13781368
mockSetup: func(
13791369
cache *cacheMock, writer *testutil.CtrlClient,
1380-
actualObject *unstructured.Unstructured,
1370+
actualObject *unstructured.Unstructured, _ ownerMode,
13811371
) {
13821372
cache.
13831373
On("Get", mock.Anything,
@@ -1388,8 +1378,6 @@ func TestObjectEngine_Teardown(t *testing.T) {
13881378
*obj = *actualObject
13891379
}).
13901380
Return(nil)
1391-
writer.On("Delete", mock.Anything, mock.Anything, mock.Anything).
1392-
Panic("Delete should not be called on the engine writer")
13931381
},
13941382
expectedError: "deleting object: teardown delete failed",
13951383
},
@@ -1400,7 +1388,7 @@ func TestObjectEngine_Teardown(t *testing.T) {
14001388
actualObject: buildObj("testi", "test", withRevision("4")),
14011389
mockSetup: func(
14021390
cache *cacheMock, writer *testutil.CtrlClient,
1403-
actualObject *unstructured.Unstructured,
1391+
actualObject *unstructured.Unstructured, mode ownerMode,
14041392
) {
14051393
cache.
14061394
On("Get", mock.Anything,
@@ -1411,8 +1399,11 @@ func TestObjectEngine_Teardown(t *testing.T) {
14111399
*obj = *actualObject
14121400
}).
14131401
Return(nil)
1402+
14141403
// Patch may be called in with-owner mode to remove owner ref.
1415-
writer.On("Patch", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil)
1404+
if mode.teardownOpts() != nil {
1405+
writer.On("Patch", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil)
1406+
}
14161407
},
14171408
expectedGone: true,
14181409
},
@@ -1425,7 +1416,7 @@ func TestObjectEngine_Teardown(t *testing.T) {
14251416
modes: []ownerMode{withNativeOwnerMode},
14261417
mockSetup: func(
14271418
cache *cacheMock, writer *testutil.CtrlClient,
1428-
actualObject *unstructured.Unstructured,
1419+
actualObject *unstructured.Unstructured, _ ownerMode,
14291420
) {
14301421
cache.
14311422
On("Get", mock.Anything,
@@ -1447,7 +1438,7 @@ func TestObjectEngine_Teardown(t *testing.T) {
14471438
actualObject: buildObj("testi", "test", withRevision("2")),
14481439
mockSetup: func(
14491440
cache *cacheMock, writer *testutil.CtrlClient,
1450-
actualObject *unstructured.Unstructured,
1441+
actualObject *unstructured.Unstructured, mode ownerMode,
14511442
) {
14521443
cache.
14531444
On("Get", mock.Anything,
@@ -1458,7 +1449,11 @@ func TestObjectEngine_Teardown(t *testing.T) {
14581449
*obj = *actualObject
14591450
}).
14601451
Return(nil)
1461-
writer.On("Patch", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil)
1452+
1453+
// Patch may be called in with-owner mode to remove owner ref.
1454+
if mode.teardownOpts() != nil {
1455+
writer.On("Patch", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil)
1456+
}
14621457
},
14631458
expectedGone: true,
14641459
},
@@ -1475,12 +1470,62 @@ func TestObjectEngine_Teardown(t *testing.T) {
14751470
})},
14761471
mockSetup: func(
14771472
_ *cacheMock, writer *testutil.CtrlClient,
1478-
_ *unstructured.Unstructured,
1473+
_ *unstructured.Unstructured, _ ownerMode,
14791474
) {
14801475
writer.On("Patch", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil)
14811476
},
14821477
expectedGone: true,
14831478
},
1479+
{
1480+
name: "Not controller, revision matches, removes owner ref",
1481+
revision: 1,
1482+
desiredObject: buildObj("testi", "test"),
1483+
actualObject: buildObj("testi", "test", withRevision("1"),
1484+
withOwnerRef("v1", "ConfigMap", "owner", "12345-678", false),
1485+
withOwnerRef("v1", "Node", "node1", "xxxx", true)),
1486+
modes: []ownerMode{withNativeOwnerMode},
1487+
mockSetup: func(
1488+
cache *cacheMock, writer *testutil.CtrlClient,
1489+
actualObject *unstructured.Unstructured, _ ownerMode,
1490+
) {
1491+
cache.
1492+
On("Get", mock.Anything,
1493+
client.ObjectKeyFromObject(actualObject),
1494+
mock.Anything, mock.Anything).
1495+
Run(func(args mock.Arguments) {
1496+
obj := args.Get(2).(*unstructured.Unstructured)
1497+
*obj = *actualObject
1498+
}).
1499+
Return(nil)
1500+
writer.On("Patch", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil)
1501+
},
1502+
expectedGone: true,
1503+
},
1504+
{
1505+
name: "Is controller, revision mismatch, still deletes",
1506+
revision: 1,
1507+
desiredObject: buildObj("testi", "test"),
1508+
actualObject: buildObj("testi", "test", withRevision("4"), withManaged),
1509+
modes: []ownerMode{withNativeOwnerMode},
1510+
mockSetup: func(
1511+
cache *cacheMock, writer *testutil.CtrlClient,
1512+
actualObject *unstructured.Unstructured, _ ownerMode,
1513+
) {
1514+
cache.
1515+
On("Get", mock.Anything,
1516+
client.ObjectKeyFromObject(actualObject),
1517+
mock.Anything, mock.Anything).
1518+
Run(func(args mock.Arguments) {
1519+
obj := args.Get(2).(*unstructured.Unstructured)
1520+
*obj = *actualObject
1521+
}).
1522+
Return(nil)
1523+
writer.
1524+
On("Delete", mock.Anything, mock.Anything, mock.Anything, mock.Anything).
1525+
Return(nil)
1526+
},
1527+
expectedGone: false,
1528+
},
14841529
}
14851530

14861531
for _, tc := range sharedTests {
@@ -1514,7 +1559,7 @@ func TestObjectEngine_Teardown(t *testing.T) {
15141559
actualObject = tc.actualObject(&mode)
15151560
}
15161561

1517-
tc.mockSetup(cache, writer, actualObject)
1562+
tc.mockSetup(cache, writer, actualObject, mode)
15181563

15191564
opts := mode.teardownOpts()
15201565
if tc.opts != nil {
@@ -1533,6 +1578,10 @@ func TestObjectEngine_Teardown(t *testing.T) {
15331578
require.Error(t, err)
15341579
assert.Contains(t, err.Error(), tc.expectedError)
15351580
}
1581+
1582+
cache.AssertExpectations(t)
1583+
writer.AssertExpectations(t)
1584+
divergeDetector.AssertExpectations(t)
15361585
})
15371586
}
15381587
}

0 commit comments

Comments
 (0)