Skip to content

Commit 2425ebb

Browse files
JustinKuliJustinKuli
committed
Correct compliance on no-op dryrun updates
Previously, the policy status would incorrectly have a noncompliant event followed immediately by a compliant event when a dryrun update would not make any changes on that object. This would cause the policy status to repeatedly update if the policy was reconciling often - for example if any of its watched objects were updating. Now only the correct "compliant" event should be emitted in that case. NOTE: This is a backport, but not just a cherry-pick. This incorporates some of the changes from these upstream PRs, with updates for tests: - open-cluster-management-io/config-policy-controller#404 - open-cluster-management-io/config-policy-controller#377 Refs: - https://issues.redhat.com/browse/ACM-25742 Signed-off-by: Justin Kulikauskas <[email protected]>
1 parent e0d973d commit 2425ebb

File tree

5 files changed

+212
-16
lines changed

5 files changed

+212
-16
lines changed

‎controllers/configurationpolicy_controller.go‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3313,9 +3313,10 @@ func (r *ConfigurationPolicyReconciler) checkAndUpdateResource(
33133313
diff = handleDiff(log, recordDiff, existingObjectCopy, mergedObjCopy, r.FullDiffs)
33143314
}
33153315

3316-
r.setEvaluatedObject(obj.policy, obj.existingObj, !throwSpecViolation, "")
3316+
// treat the object as compliant, with no updates needed
3317+
r.setEvaluatedObject(obj.policy, obj.existingObj, true, "")
33173318

3318-
return throwSpecViolation, "", diff, updateNeeded, updatedObj
3319+
return false, "", diff, false, updatedObj
33193320
}
33203321

33213322
diff = handleDiff(log, recordDiff, existingObjectCopy, dryRunUpdatedObj, r.FullDiffs)
@@ -3561,6 +3562,10 @@ func removeFieldsForComparison(obj *unstructured.Unstructured) {
35613562
)
35623563
// The generation might actually bump but the API output might be the same.
35633564
unstructured.RemoveNestedField(obj.Object, "metadata", "generation")
3565+
3566+
if len(obj.GetAnnotations()) == 0 {
3567+
unstructured.RemoveNestedField(obj.Object, "metadata", "annotations")
3568+
}
35643569
}
35653570

35663571
// setEvaluatedObject updates the cache to indicate that the ConfigurationPolicy has evaluated this

‎test/e2e/case8_status_check_test.go‎

Lines changed: 136 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -6,26 +6,27 @@ package e2e
66
import (
77
. "github.com/onsi/ginkgo/v2"
88
. "github.com/onsi/gomega"
9+
v1 "k8s.io/api/core/v1"
910
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
1011

1112
"open-cluster-management.io/config-policy-controller/test/utils"
1213
)
1314

14-
const (
15-
case8ConfigPolicyNamePod string = "policy-pod-to-check"
16-
case8ConfigPolicyNameCheck string = "policy-status-checker"
17-
case8ConfigPolicyNameCheckFail string = "policy-status-checker-fail"
18-
case8ConfigPolicyNameEnforceFail string = "policy-status-enforce-fail"
19-
case8PolicyYamlPod string = "../resources/case8_status_check/case8_pod.yaml"
20-
case8PolicyYamlCheck string = "../resources/case8_status_check/case8_status_check.yaml"
21-
case8PolicyYamlCheckFail string = "../resources/case8_status_check/case8_status_check_fail.yaml"
22-
case8PolicyYamlEnforceFail string = "../resources/case8_status_check/case8_status_enforce_fail.yaml"
23-
case8ConfigPolicyStatusPod string = "policy-pod-invalid"
24-
case8PolicyYamlBadPod string = "../resources/case8_status_check/case8_pod_fail.yaml"
25-
case8PolicyYamlSpecChange string = "../resources/case8_status_check/case8_pod_change.yaml"
26-
)
27-
2815
var _ = Describe("Test pod obj template handling", func() {
16+
const (
17+
case8ConfigPolicyNamePod string = "policy-pod-to-check"
18+
case8ConfigPolicyNameCheck string = "policy-status-checker"
19+
case8ConfigPolicyNameCheckFail string = "policy-status-checker-fail"
20+
case8ConfigPolicyNameEnforceFail string = "policy-status-enforce-fail"
21+
case8PolicyYamlPod string = "../resources/case8_status_check/case8_pod.yaml"
22+
case8PolicyYamlCheck string = "../resources/case8_status_check/case8_status_check.yaml"
23+
case8PolicyYamlCheckFail string = "../resources/case8_status_check/case8_status_check_fail.yaml"
24+
case8PolicyYamlEnforceFail string = "../resources/case8_status_check/case8_status_enforce_fail.yaml"
25+
case8ConfigPolicyStatusPod string = "policy-pod-invalid"
26+
case8PolicyYamlBadPod string = "../resources/case8_status_check/case8_pod_fail.yaml"
27+
case8PolicyYamlSpecChange string = "../resources/case8_status_check/case8_pod_change.yaml"
28+
)
29+
2930
Describe("Create a policy on managed cluster in ns:"+testNamespace, Ordered, func() {
3031
It("should create a policy properly on the managed cluster", func() {
3132
By("Creating " + case8ConfigPolicyNamePod + " on managed")
@@ -171,3 +172,124 @@ var _ = Describe("Test pod obj template handling", func() {
171172
})
172173
})
173174
})
175+
176+
var _ = FDescribe("Test related object property status", Ordered, func() {
177+
Describe("Create a policy missing a field added by kubernetes", Ordered, func() {
178+
const (
179+
policyName = "policy-service"
180+
serviceName = "grc-policy-propagator-metrics"
181+
policyYAML = "../resources/case8_status_check/case8_service_inform.yaml"
182+
serviceYAML = "../resources/case8_status_check/case8_service.yaml"
183+
parentYAML = "../resources/case8_status_check/case8_parent.yaml"
184+
parentName = "case8-parent"
185+
)
186+
187+
It("Should be compliant when the inform policy omits a field added by the apiserver", func() {
188+
By("Creating a Service with explicit type: ClusterIP")
189+
utils.Kubectl("apply", "-f", serviceYAML)
190+
191+
By("Creating the " + policyName + " policy that omits the type field)")
192+
createObjWithParent(parentYAML, parentName, policyYAML, testNamespace, gvrPolicy, gvrConfigPolicy)
193+
194+
By("Verifying that the " + policyName + " policy is compliant")
195+
Eventually(func(g Gomega) {
196+
managedPlc := utils.GetWithTimeout(
197+
clientManagedDynamic, gvrConfigPolicy, policyName, testNamespace, true, defaultTimeoutSeconds,
198+
)
199+
200+
utils.CheckComplianceStatus(g, managedPlc, "Compliant")
201+
202+
relatedObjects, _, err := unstructured.NestedSlice(managedPlc.Object, "status", "relatedObjects")
203+
g.Expect(err).ToNot(HaveOccurred())
204+
g.Expect(relatedObjects).To(HaveLen(1))
205+
}, defaultTimeoutSeconds, 1).Should(Succeed())
206+
207+
By("Verifying that only one compliance event was emitted")
208+
Consistently(func() []v1.Event {
209+
return utils.GetMatchingEvents(clientManaged, testNamespace, parentName, "policy:", ".*", 5)
210+
}, 10, 1).Should(HaveLen(1))
211+
})
212+
213+
It("Should be compliant when the enforce policy omits a field added by the apiserver", func() {
214+
By("Changing the policy to enforce mode")
215+
utils.EnforceConfigurationPolicy(policyName, testNamespace)
216+
217+
By("Verifying that the " + policyName + " policy is compliant")
218+
Eventually(func(g Gomega) {
219+
managedPlc := utils.GetWithTimeout(
220+
clientManagedDynamic, gvrConfigPolicy, policyName, testNamespace, true, defaultTimeoutSeconds,
221+
)
222+
223+
utils.CheckComplianceStatus(g, managedPlc, "Compliant")
224+
225+
gen, found, err := unstructured.NestedInt64(managedPlc.Object, "status", "lastEvaluatedGeneration")
226+
g.Expect(err).NotTo(HaveOccurred())
227+
g.Expect(found).To(BeTrue())
228+
g.Expect(gen).To(BeEquivalentTo(2))
229+
}, defaultTimeoutSeconds, 1).Should(Succeed())
230+
231+
By("Verifying that only two compliance events have been emitted")
232+
Consistently(func() []v1.Event {
233+
return utils.GetMatchingEvents(clientManaged, testNamespace, parentName, "policy:", ".*", 5)
234+
}, 10, 1).Should(HaveLen(2))
235+
})
236+
237+
It("Should be compliant when the enforce policy includes all fields", func() {
238+
By("Patching the " + policyName + " policy with the Service type field)")
239+
utils.Kubectl("patch", "configurationpolicy", policyName, "-n", testNamespace, "--type=json", "-p",
240+
`[{"op": "add", "path": "/spec/object-templates/0/objectDefinition/spec/type", "value": "ClusterIP"}]`)
241+
242+
By("Verifying that the " + policyName + " policy is compliant")
243+
Eventually(func(g Gomega) {
244+
managedPlc := utils.GetWithTimeout(
245+
clientManagedDynamic, gvrConfigPolicy, policyName, testNamespace, true, defaultTimeoutSeconds,
246+
)
247+
248+
utils.CheckComplianceStatus(g, managedPlc, "Compliant")
249+
250+
gen, found, err := unstructured.NestedInt64(managedPlc.Object, "status", "lastEvaluatedGeneration")
251+
g.Expect(err).NotTo(HaveOccurred())
252+
g.Expect(found).To(BeTrue())
253+
g.Expect(gen).To(BeEquivalentTo(3))
254+
}, defaultTimeoutSeconds, 1).Should(Succeed())
255+
256+
By("Verifying that only three compliance events have been emitted")
257+
Consistently(func() []v1.Event {
258+
return utils.GetMatchingEvents(clientManaged, testNamespace, parentName, "policy:", ".*", 5)
259+
}, 10, 1).Should(HaveLen(3))
260+
})
261+
262+
It("Should be compliant when the inform policy includes all fields", func() {
263+
By("Changing the policy to inform mode")
264+
utils.Kubectl("patch", "configurationpolicy", policyName, `--type=json`,
265+
`-p=[{"op":"replace","path":"/spec/remediationAction","value":"inform"}]`, "-n", testNamespace)
266+
267+
By("Verifying that the " + policyName + " policy is compliant")
268+
Eventually(func(g Gomega) {
269+
managedPlc := utils.GetWithTimeout(
270+
clientManagedDynamic, gvrConfigPolicy, policyName, testNamespace, true, defaultTimeoutSeconds,
271+
)
272+
273+
utils.CheckComplianceStatus(g, managedPlc, "Compliant")
274+
275+
gen, found, err := unstructured.NestedInt64(managedPlc.Object, "status", "lastEvaluatedGeneration")
276+
g.Expect(err).NotTo(HaveOccurred())
277+
g.Expect(found).To(BeTrue())
278+
g.Expect(gen).To(BeEquivalentTo(4))
279+
}, defaultTimeoutSeconds, 1).Should(Succeed())
280+
281+
By("Verifying that only four compliance events have been emitted")
282+
Consistently(func() []v1.Event {
283+
return utils.GetMatchingEvents(clientManaged, testNamespace, parentName, "policy:", ".*", 5)
284+
}, 10, 1).Should(HaveLen(4))
285+
})
286+
287+
AfterAll(func() {
288+
deleteConfigPolicies([]string{policyName, policyName})
289+
290+
utils.KubectlDelete("service", serviceName, "-n", "managed")
291+
utils.KubectlDelete("policy", parentName, "-n", testNamespace)
292+
utils.KubectlDelete("events", "-n", testNamespace, "--field-selector=involvedObject.name="+parentName)
293+
})
294+
})
295+
})

‎test/resources/case8_status_check/case8_parent.yaml‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
apiVersion: policy.open-cluster-management.io/v1
2+
kind: Policy
3+
metadata:
4+
name: case8-parent
5+
spec:
6+
remediationAction: inform
7+
disabled: false
8+
policy-templates: []

‎test/resources/case8_status_check/case8_service.yaml‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
apiVersion: v1
2+
kind: Service
3+
metadata:
4+
name: grc-policy-propagator-metrics
5+
namespace: managed
6+
spec:
7+
internalTrafficPolicy: Cluster
8+
ipFamilies:
9+
- IPv4
10+
ipFamilyPolicy: SingleStack
11+
ports:
12+
- name: metrics-https
13+
port: 8443
14+
protocol: TCP
15+
targetPort: 8443
16+
selector:
17+
app: grc
18+
component: ocm-policy-propagator
19+
release: grc
20+
sessionAffinity: None
21+
type: ClusterIP

‎test/resources/case8_status_check/case8_service_inform.yaml‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
apiVersion: policy.open-cluster-management.io/v1
2+
kind: ConfigurationPolicy
3+
metadata:
4+
name: policy-service
5+
namespace: managed
6+
ownerReferences:
7+
- apiVersion: policy.open-cluster-management.io/v1
8+
kind: Policy
9+
name: case8-parent
10+
uid: 12345678-90ab-cdef-1234-567890abcdef # must be replaced before creation
11+
spec:
12+
remediationAction: inform
13+
severity: low
14+
object-templates:
15+
- complianceType: mustonlyhave
16+
metadataComplianceType: musthave
17+
objectDefinition:
18+
apiVersion: v1
19+
kind: Service
20+
metadata:
21+
name: grc-policy-propagator-metrics
22+
namespace: managed
23+
spec: # Intentionally omitting 'type' field - K8s will default to ClusterIP
24+
clusterIP: '{{ (lookup "v1" "Service" "managed" "grc-policy-propagator-metrics").spec.clusterIP }}'
25+
clusterIPs:
26+
- '{{ (lookup "v1" "Service" "managed" "grc-policy-propagator-metrics").spec.clusterIP }}'
27+
internalTrafficPolicy: Cluster
28+
ipFamilies:
29+
- IPv4
30+
ipFamilyPolicy: SingleStack
31+
ports:
32+
- name: metrics-https
33+
port: 8443
34+
protocol: TCP
35+
targetPort: 8443
36+
selector:
37+
app: grc
38+
component: ocm-policy-propagator
39+
release: grc
40+
sessionAffinity: None

0 commit comments

Comments
 (0)