Skip to content

Commit a06836b

Browse files
Handle other changes to PlacementBinding
To reduce the number of unnecessary reconciles on replicated policies, when a PlacementBinding is updated, only the added or removed policies are reconciled. However, this meant that when other fields are updated in a PlacementBinding, the affected policies were not being updated accordingly. Now, when there are other changes, all possibly affected replicated policies will be reconciled. Refs: - https://issues.redhat.com/browse/ACM-8515 Signed-off-by: Justin Kulikauskas <[email protected]>
1 parent db0c8dd commit a06836b

File tree

2 files changed

+72
-2
lines changed

2 files changed

+72
-2
lines changed

controllers/propagator/replicatepolicy_pb_eventHandler.go

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,22 @@ func (e *handlerForBinding) Update(ctx context.Context,
4949
oldRepPolcies := common.GetRepPoliciesInPlacementBinding(ctx, e.c, oldObj)
5050
newRepPolcies := common.GetRepPoliciesInPlacementBinding(ctx, e.c, newObj)
5151

52-
// Send only affected replicated policies
52+
// If the BindingOverrides or subFilter has changed, all new and old policies need to be checked
53+
if newObj.BindingOverrides.RemediationAction != oldObj.BindingOverrides.RemediationAction ||
54+
newObj.SubFilter != oldObj.SubFilter {
55+
for _, obj := range newRepPolcies {
56+
q.Add(obj)
57+
}
58+
59+
// The workqueue will handle any de-duplication.
60+
for _, obj := range oldRepPolcies {
61+
q.Add(obj)
62+
}
63+
64+
return
65+
}
66+
67+
// Otherwise send only affected replicated policies
5368
affectedRepPolicies := common.GetAffectedObjs(oldRepPolcies, newRepPolcies)
5469
for _, obj := range affectedRepPolicies {
5570
q.Add(obj)

test/e2e/case16_selective_policy_enforcement_test.go

Lines changed: 56 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,7 @@ var _ = Describe("Test selective policy enforcement", Ordered, func() {
7676
)
7777
Expect(err).ToNot(HaveOccurred())
7878

79-
By("Verifying no replicated policy created on cluster ns managed3")
79+
By("Verifying no replicated policy created on cluster ns managed3 because of the subfilter")
8080
Eventually(func() interface{} {
8181
replicatedPlc := utils.GetWithTimeout(
8282
clientHubDynamic, gvrPolicy, testNamespace+"."+case16PolicyName,
@@ -192,5 +192,60 @@ var _ = Describe("Test selective policy enforcement", Ordered, func() {
192192
return rootPlc.Object["status"].(map[string]interface{})["placement"]
193193
}, defaultTimeoutSeconds, 1).Should(utils.SemanticEqual(expectedPlacementStatus))
194194
})
195+
196+
It("should propagate to all clusters when the subfilter is removed", func() {
197+
By("Putting all clusters in the case16-test-policy-plr-enforce decisions")
198+
plr := utils.GetWithTimeout(
199+
clientHubDynamic, gvrPlacementRule, case16PolicyName+"-plr-enforce",
200+
testNamespace, true, defaultTimeoutSeconds,
201+
)
202+
plr.Object["status"] = utils.GeneratePlrStatus("managed1", "managed2", "managed3")
203+
_, err := clientHubDynamic.Resource(gvrPlacementRule).Namespace(testNamespace).UpdateStatus(
204+
context.TODO(), plr, metav1.UpdateOptions{},
205+
)
206+
Expect(err).ToNot(HaveOccurred())
207+
208+
By("Verifying that the policy on managed3 is still not created")
209+
Consistently(func() interface{} {
210+
return utils.GetWithTimeout(
211+
clientHubDynamic, gvrPolicy, testNamespace+"."+case16PolicyName,
212+
"managed3", false, defaultTimeoutSeconds,
213+
)
214+
}, "5s", 1).Should(BeNil())
215+
216+
By("Removing the subfilter from the binding")
217+
utils.Kubectl("patch", "placementbinding", "case16-test-policy-pb-enforce", "-n", testNamespace,
218+
"--type=json", `-p=[{"op":"remove","path":"/subFilter"}]`)
219+
220+
By("Verifying the policies exist and are enforcing on all 3 clusters")
221+
for _, clustername := range []string{"managed1", "managed2", "managed3"} {
222+
Eventually(func() interface{} {
223+
replicatedPlc := utils.GetWithTimeout(
224+
clientHubDynamic, gvrPolicy, testNamespace+"."+case16PolicyName,
225+
clustername, true, defaultTimeoutSeconds,
226+
)
227+
228+
return replicatedPlc.Object["spec"].(map[string]interface{})["remediationAction"]
229+
}, defaultTimeoutSeconds, 1).Should(Equal("enforce"))
230+
}
231+
})
232+
233+
It("should change remediationAction when the bindingOverrides are removed", func() {
234+
By("Removing the overrides from the binding")
235+
utils.Kubectl("patch", "placementbinding", "case16-test-policy-pb-enforce", "-n", testNamespace,
236+
"--type=json", `-p=[{"op":"remove","path":"/bindingOverrides"}]`)
237+
238+
By("Verifying the policies are informing on all 3 clusters")
239+
for _, clustername := range []string{"managed1", "managed2", "managed3"} {
240+
Eventually(func() interface{} {
241+
replicatedPlc := utils.GetWithTimeout(
242+
clientHubDynamic, gvrPolicy, testNamespace+"."+case16PolicyName,
243+
clustername, true, defaultTimeoutSeconds,
244+
)
245+
246+
return replicatedPlc.Object["spec"].(map[string]interface{})["remediationAction"]
247+
}, defaultTimeoutSeconds, 1).Should(Equal("inform"))
248+
}
249+
})
195250
})
196251
})

0 commit comments

Comments
 (0)