From bf82d571a6c4cfa5793bb7c1e7c9ec5bc2005739 Mon Sep 17 00:00:00 2001 From: Dale Haiducek <19750917+dhaiducek@users.noreply.github.com> Date: Wed, 3 Apr 2024 07:14:16 -0400 Subject: [PATCH] Fix handling of undefined fields for mustonlyhave If a field exists in the object but wasn't defined for a mustonlyhave policy, we need to check the existing object's fields and set them accordingly, particularly to set them to null. Signed-off-by: Dale Haiducek <19750917+dhaiducek@users.noreply.github.com> --- controllers/configurationpolicy_controller.go | 34 ++++++++++ test/e2e/case2_role_handling_test.go | 64 ++++++++++++++++++- test/e2e/e2e_suite_test.go | 6 ++ .../case2_rolebinding_create_enforce.yaml | 23 +++++++ .../case2_rolebinding_create_patch.yaml | 19 ++++++ 5 files changed, 145 insertions(+), 1 deletion(-) create mode 100644 test/resources/case2_role_handling/case2_rolebinding_create_enforce.yaml create mode 100644 test/resources/case2_role_handling/case2_rolebinding_create_patch.yaml diff --git a/controllers/configurationpolicy_controller.go b/controllers/configurationpolicy_controller.go index 1dc54f38..9dd772f1 100644 --- a/controllers/configurationpolicy_controller.go +++ b/controllers/configurationpolicy_controller.go @@ -2750,7 +2750,11 @@ func handleKeys( mdCompType string, zeroValueEqualsNil bool, ) (throwSpecViolation bool, message string, updateNeeded bool, statusMismatch bool) { + handledKeys := map[string]bool{} + + // Iterate over keys of the desired object to compare with the existing object on the cluster for key := range desiredObj.Object { + handledKeys[key] = true isStatus := key == "status" // use metadatacompliancetype to evaluate metadata if it is set @@ -2798,6 +2802,36 @@ func handleKeys( } } + // If the complianceType is "mustonlyhave", then compare the existing object's keys, + // skipping over: previously compared keys, metadata, and status. + if compType == "mustonlyhave" { + for key := range existingObj.Object { + if handledKeys[key] || key == "status" || key == "metadata" { + continue + } + + // check key for mismatch + errorMsg, keyUpdateNeeded, mergedObj, skipped := handleSingleKey( + key, desiredObj, existingObjectCopy, compType, zeroValueEqualsNil, + ) + if errorMsg != "" { + log.Info(errorMsg) + + return true, errorMsg, true, statusMismatch + } + + // If the mergedObj is nil, we'll use the value to erase it from the existing object. + if skipped { + continue + } + + if keyUpdateNeeded { + existingObj.Object[key] = mergedObj + updateNeeded = true + } + } + } + return } diff --git a/test/e2e/case2_role_handling_test.go b/test/e2e/case2_role_handling_test.go index b9320df1..b843875c 100644 --- a/test/e2e/case2_role_handling_test.go +++ b/test/e2e/case2_role_handling_test.go @@ -6,6 +6,7 @@ package e2e import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "open-cluster-management.io/config-policy-controller/test/utils" ) @@ -22,6 +23,11 @@ var _ = Describe("Test role obj template handling", Ordered, func() { policyCheckMNHYaml string = resourcePrefix + "case2_role_check-mnh.yaml" policyCheckMOHYaml string = resourcePrefix + "case2_role_check-moh.yaml" policyCheckCompliant string = resourcePrefix + "case2_role_check-c.yaml" + configPolicyNameBindingEnforce string = "policy-rolebinding-create" + bindingName string = "pod-reader-e2e-binding" + policyYamlBindingEnforce string = resourcePrefix + "case2_rolebinding_create_enforce.yaml" + policyYamlBindingPatch string = resourcePrefix + "case2_rolebinding_create_patch.yaml" + ) AfterAll(func() { By("clean up case2") @@ -31,9 +37,11 @@ var _ = Describe("Test role obj template handling", Ordered, func() { "policy-role-check-comp", "policy-role-check-mnh", "policy-role-check-moh", + configPolicyNameBindingEnforce, } deleteConfigPolicies(policies) utils.Kubectl("delete", "role", roleName, "-n", "default", "--ignore-not-found") + utils.Kubectl("delete", "rolebinding", bindingName, "-n", "default", "--ignore-not-found") }) It("should be created properly on the managed cluster", func() { @@ -44,7 +52,7 @@ var _ = Describe("Test role obj template handling", Ordered, func() { Expect(plc).NotTo(BeNil()) Eventually(func() interface{} { managedPlc := utils.GetWithTimeout(clientManagedDynamic, gvrConfigPolicy, - case2ConfigPolicyNameInform, testNamespace, true, defaultTimeoutSeconds) + configPolicyNameInform, testNamespace, true, defaultTimeoutSeconds) return utils.GetComplianceState(managedPlc) }, defaultTimeoutSeconds, 1).Should(Equal("NonCompliant")) @@ -106,6 +114,60 @@ var _ = Describe("Test role obj template handling", Ordered, func() { }, defaultTimeoutSeconds, 1).Should(Equal("Compliant")) }) + It("should create rolebinding on managed cluster", func() { + By("creating " + policyYamlBindingEnforce + " on hub with spec.remediationAction = enforce") + utils.Kubectl("apply", "-f", policyYamlBindingEnforce, "-n", testNamespace) + plc := utils.GetWithTimeout(clientManagedDynamic, gvrConfigPolicy, + configPolicyNameBindingEnforce, testNamespace, true, defaultTimeoutSeconds) + Expect(plc).NotTo(BeNil()) + Eventually(func() interface{} { + managedPlc := utils.GetWithTimeout(clientManagedDynamic, gvrConfigPolicy, + configPolicyNameBindingEnforce, testNamespace, true, defaultTimeoutSeconds) + + return utils.GetComplianceState(managedPlc) + }, defaultTimeoutSeconds, 1).Should(Equal("Compliant")) + + Eventually(func() interface{} { + informPlc := utils.GetWithTimeout(clientManagedDynamic, gvrConfigPolicy, + configPolicyNameBindingEnforce, testNamespace, true, defaultTimeoutSeconds) + + return utils.GetComplianceState(informPlc) + }, defaultTimeoutSeconds, 1).Should(Equal("Compliant")) + binding := utils.GetWithTimeout(clientManagedDynamic, gvrRoleBinding, bindingName, + "default", true, defaultTimeoutSeconds) + Expect(binding).NotTo(BeNil()) + subjects, _, err := unstructured.NestedSlice(binding.Object, "subjects") + Expect(err).ToNot(HaveOccurred()) + Expect(subjects).To(HaveLen(1)) + }) + + It("should patch the rolebinding on managed cluster to an empty subjects", func() { + By("creating " + policyYamlBindingPatch + " on hub with spec.remediationAction = inform") + utils.Kubectl("apply", "-f", policyYamlBindingPatch, "-n", testNamespace) + plc := utils.GetWithTimeout(clientManagedDynamic, gvrConfigPolicy, + configPolicyNameBindingEnforce, testNamespace, true, defaultTimeoutSeconds) + Expect(plc).NotTo(BeNil()) + Eventually(func() interface{} { + managedPlc := utils.GetWithTimeout(clientManagedDynamic, gvrConfigPolicy, + configPolicyNameBindingEnforce, testNamespace, true, defaultTimeoutSeconds) + + return utils.GetComplianceState(managedPlc) + }, defaultTimeoutSeconds, 1).Should(Equal("NonCompliant")) + By("patching policy spec.remediationAction = enforce") + utils.Kubectl("patch", "configurationpolicy", configPolicyNameBindingEnforce, `--type=json`, + `-p=[{"op":"replace","path":"/spec/remediationAction","value":"enforce"}]`, "-n", testNamespace) + Eventually(func() interface{} { + informPlc := utils.GetWithTimeout(clientManagedDynamic, gvrConfigPolicy, + configPolicyNameBindingEnforce, testNamespace, true, defaultTimeoutSeconds) + + return utils.GetComplianceState(informPlc) + }, defaultTimeoutSeconds, 1).Should(Equal("Compliant")) + binding := utils.GetWithTimeout(clientManagedDynamic, gvrRoleBinding, bindingName, + "default", true, defaultTimeoutSeconds) + Expect(binding).NotTo(BeNil()) + subjects, _, err := unstructured.NestedSlice(binding.Object, "subjects") + Expect(err).ToNot(HaveOccurred()) + Expect(subjects).To(BeNil()) }) }) }) diff --git a/test/e2e/e2e_suite_test.go b/test/e2e/e2e_suite_test.go index a2cf6941..4e80feca 100644 --- a/test/e2e/e2e_suite_test.go +++ b/test/e2e/e2e_suite_test.go @@ -40,6 +40,7 @@ var ( gvrCRD schema.GroupVersionResource gvrPod schema.GroupVersionResource gvrRole schema.GroupVersionResource + gvrRoleBinding schema.GroupVersionResource gvrNS schema.GroupVersionResource gvrSCC schema.GroupVersionResource gvrSecret schema.GroupVersionResource @@ -75,6 +76,11 @@ var _ = BeforeSuite(func() { gvrNS = schema.GroupVersionResource{Group: "", Version: "v1", Resource: "namespaces"} gvrConfigMap = schema.GroupVersionResource{Group: "", Version: "v1", Resource: "configmaps"} gvrRole = schema.GroupVersionResource{Group: "rbac.authorization.k8s.io", Version: "v1", Resource: "roles"} + gvrRoleBinding = schema.GroupVersionResource{ + Group: "rbac.authorization.k8s.io", + Version: "v1", + Resource: "rolebindings", + } gvrConfigPolicy = schema.GroupVersionResource{ Group: "policy.open-cluster-management.io", Version: "v1", diff --git a/test/resources/case2_role_handling/case2_rolebinding_create_enforce.yaml b/test/resources/case2_role_handling/case2_rolebinding_create_enforce.yaml new file mode 100644 index 00000000..ecaff2d0 --- /dev/null +++ b/test/resources/case2_role_handling/case2_rolebinding_create_enforce.yaml @@ -0,0 +1,23 @@ +apiVersion: policy.open-cluster-management.io/v1 +kind: ConfigurationPolicy +metadata: + name: policy-rolebinding-create +spec: + remediationAction: enforce + namespaceSelector: + include: ["default"] + object-templates: + - complianceType: mustonlyhave + objectDefinition: + apiVersion: rbac.authorization.k8s.io/v1 + kind: RoleBinding + metadata: + name: pod-reader-e2e-binding + roleRef: + apiGroup: rbac.authorization.k8s.io + kind: Role + name: pod-reader-e2e + subjects: + - apiGroup: rbac.authorization.k8s.io + kind: Group + name: system:authenticated:oauth diff --git a/test/resources/case2_role_handling/case2_rolebinding_create_patch.yaml b/test/resources/case2_role_handling/case2_rolebinding_create_patch.yaml new file mode 100644 index 00000000..8a6a1dfa --- /dev/null +++ b/test/resources/case2_role_handling/case2_rolebinding_create_patch.yaml @@ -0,0 +1,19 @@ +apiVersion: policy.open-cluster-management.io/v1 +kind: ConfigurationPolicy +metadata: + name: policy-rolebinding-create +spec: + remediationAction: inform + namespaceSelector: + include: ["default"] + object-templates: + - complianceType: mustonlyhave + objectDefinition: + apiVersion: rbac.authorization.k8s.io/v1 + kind: RoleBinding + metadata: + name: pod-reader-e2e-binding + roleRef: + apiGroup: rbac.authorization.k8s.io + kind: Role + name: pod-reader-e2e