Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Bug: Objects are pruned on templating errors #139

Merged
merged 1 commit into from
Jun 1, 2023
Merged

Bug: Objects are pruned on templating errors #139

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 20 additions & 9 deletions controllers/configurationpolicy_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -698,7 +698,7 @@ func (r *ConfigurationPolicyReconciler) handleObjectTemplates(plc policyv1.Confi
fmt.Sprintf(plcFmtStr, plc.GetName()), convertPolicyStatusToString(&plc))
}

r.checkRelatedAndUpdate(plc, relatedObjects, oldRelated, statusChanged)
r.checkRelatedAndUpdate(plc, relatedObjects, oldRelated, statusChanged, true)

parent := ""
if len(plc.OwnerReferences) > 0 {
Expand Down Expand Up @@ -793,7 +793,7 @@ func (r *ConfigurationPolicyReconciler) handleObjectTemplates(plc policyv1.Confi
}

// don't change related objects while deletion is in progress
r.checkRelatedAndUpdate(plc, oldRelated, oldRelated, parentStatusUpdateNeeded)
r.checkRelatedAndUpdate(plc, oldRelated, oldRelated, parentStatusUpdateNeeded, true)
}

return
Expand All @@ -810,6 +810,8 @@ func (r *ConfigurationPolicyReconciler) handleObjectTemplates(plc policyv1.Confi
}
}

// When it is hub or managed template parse error, deleteDetachedObjs should be false
// Then it doesn't remove resources
addTemplateErrorViolation := func(reason, msg string) {
log.Info("Setting the policy to noncompliant due to a templating error", "error", msg)

Expand All @@ -829,7 +831,8 @@ func (r *ConfigurationPolicyReconciler) handleObjectTemplates(plc policyv1.Confi
)
}

r.checkRelatedAndUpdate(plc, relatedObjects, oldRelated, parentStatusUpdateNeeded)
// deleteDetachedObjs should be false
r.checkRelatedAndUpdate(plc, relatedObjects, oldRelated, parentStatusUpdateNeeded, false)
}

// initialize apiresources for template processing before starting objectTemplate processing
Expand Down Expand Up @@ -1054,7 +1057,7 @@ func (r *ConfigurationPolicyReconciler) handleObjectTemplates(plc policyv1.Confi

if err != nil {
if parentStatusUpdateNeeded {
r.checkRelatedAndUpdate(plc, relatedObjects, oldRelated, parentStatusUpdateNeeded)
r.checkRelatedAndUpdate(plc, relatedObjects, oldRelated, parentStatusUpdateNeeded, false)
}

return
Expand All @@ -1074,7 +1077,7 @@ func (r *ConfigurationPolicyReconciler) handleObjectTemplates(plc policyv1.Confi
convertPolicyStatusToString(&plc))
}

r.checkRelatedAndUpdate(plc, relatedObjects, oldRelated, statusUpdateNeeded)
r.checkRelatedAndUpdate(plc, relatedObjects, oldRelated, statusUpdateNeeded, true)

return
}
Expand Down Expand Up @@ -1187,14 +1190,17 @@ func (r *ConfigurationPolicyReconciler) handleObjectTemplates(plc policyv1.Confi
}
}

r.checkRelatedAndUpdate(plc, relatedObjects, oldRelated, parentStatusUpdateNeeded)
r.checkRelatedAndUpdate(plc, relatedObjects, oldRelated, parentStatusUpdateNeeded, true)
}

// checkRelatedAndUpdate checks the related objects field and triggers an update on the ConfigurationPolicy
func (r *ConfigurationPolicyReconciler) checkRelatedAndUpdate(
plc policyv1.ConfigurationPolicy, related, oldRelated []policyv1.RelatedObject, sendEvent bool,
plc policyv1.ConfigurationPolicy,
related, oldRelated []policyv1.RelatedObject,
sendEvent bool,
deleteDetachedObjs bool,
) {
r.sortRelatedObjectsAndUpdate(&plc, related, oldRelated, r.EnableMetrics)
r.sortRelatedObjectsAndUpdate(&plc, related, oldRelated, r.EnableMetrics, deleteDetachedObjs)
// An update always occurs to account for the lastEvaluated status field
r.addForUpdate(&plc, sendEvent)
}
Expand All @@ -1203,6 +1209,7 @@ func (r *ConfigurationPolicyReconciler) checkRelatedAndUpdate(
func (r *ConfigurationPolicyReconciler) sortRelatedObjectsAndUpdate(
plc *policyv1.ConfigurationPolicy, related, oldRelated []policyv1.RelatedObject,
collectMetrics bool,
deleteDetachedObjs bool,
) {
sort.SliceStable(related, func(i, j int) bool {
if related[i].Object.Kind != related[j].Object.Kind {
Expand Down Expand Up @@ -1273,7 +1280,11 @@ func (r *ConfigurationPolicyReconciler) sortRelatedObjectsAndUpdate(
}

if !gocmp.Equal(related, oldRelated) {
r.deleteDetachedObj(*plc, related, oldRelated)
// When it is hub or managed template parse error, it should not remove previous objects
if deleteDetachedObjs {
r.deleteDetachedObj(*plc, related, oldRelated)
}

plc.Status.RelatedObjects = related
}
}
Expand Down
6 changes: 3 additions & 3 deletions controllers/configurationpolicy_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -414,14 +414,14 @@ func TestSortRelatedObjectsAndUpdate(t *testing.T) {

empty := []policyv1.RelatedObject{}

r.sortRelatedObjectsAndUpdate(policy, relatedList, empty, false)
r.sortRelatedObjectsAndUpdate(policy, relatedList, empty, false, true)
assert.True(t, relatedList[0].Object.Metadata.Name == "bar")

// append another object named bar but also with namespace bar
relatedList = append(relatedList, addRelatedObjects(true, rsrc,
"ConfigurationPolicy", "bar", true, []string{name}, "reason", nil)...)

r.sortRelatedObjectsAndUpdate(policy, relatedList, empty, false)
r.sortRelatedObjectsAndUpdate(policy, relatedList, empty, false, true)
assert.True(t, relatedList[0].Object.Metadata.Namespace == "bar")

// clear related objects and test sorting with no namespace
Expand All @@ -432,7 +432,7 @@ func TestSortRelatedObjectsAndUpdate(t *testing.T) {
relatedList = append(relatedList, addRelatedObjects(true, rsrc, "ConfigurationPolicy", "",
false, []string{name}, "reason", nil)...)

r.sortRelatedObjectsAndUpdate(policy, relatedList, empty, false)
r.sortRelatedObjectsAndUpdate(policy, relatedList, empty, false, true)
assert.True(t, relatedList[0].Object.Metadata.Name == "bar")
}

Expand Down
105 changes: 105 additions & 0 deletions test/e2e/case13_templatization_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,11 @@ const (
case13CopyRefObjectYaml = "../resources/case13_templatization/case13_copy_referenced_configmap.yaml"
)

const (
case13PruneTmpErr string = "case13-prune-template-error"
case13PruneTmpErrYaml string = "../resources/case13_templatization/case13_prune_template_error.yaml"
)

var _ = Describe("Test templatization", func() {
Describe("Create a secret and pull data from it into a configurationPolicy", func() {
It("should be created properly on the managed cluster", func() {
Expand Down Expand Up @@ -416,4 +421,104 @@ var _ = Describe("Test templatization", func() {
utils.Kubectl("delete", "configmap", configMapReplName, "-n", "default", "--ignore-not-found")
})
})
Describe("Create a secret and create template error", func() {
It("Should the object created by configpolicy remain", func() {
By("Creating " + case13CfgPolCreateSecret + " and " + case13CfgPolCheckSecret + " on managed")
// create secret
utils.Kubectl("apply", "-f", case13SecretYaml, "-n", "default")
secret := utils.GetWithTimeout(clientManagedDynamic, gvrSecret,
case13Secret, "default", true, defaultTimeoutSeconds)
Expect(secret).NotTo(BeNil())
// create copy with password from original secret using a templatized policy
utils.Kubectl("apply", "-f", case13PruneTmpErrYaml, "-n", testNamespace)
plc := utils.GetWithTimeout(clientManagedDynamic, gvrConfigPolicy,
case13PruneTmpErr, testNamespace, true, defaultTimeoutSeconds)
Expect(plc).NotTo(BeNil())

By("By verifying that the configurationpolicy is working well")
Eventually(func() interface{} {
managedPlc := utils.GetWithTimeout(clientManagedDynamic, gvrConfigPolicy,
case13PruneTmpErr, testNamespace, true, defaultTimeoutSeconds)

return utils.GetComplianceState(managedPlc)
}, defaultTimeoutSeconds, 1).Should(Equal("Compliant"))

By("By verifying that the configmap exist ")
Eventually(func() interface{} {
configmap := utils.GetWithTimeout(clientManagedDynamic, gvrConfigMap,
case13PruneTmpErr+"-configmap", "default", true, defaultTimeoutSeconds)

return configmap
}, defaultTimeoutSeconds, 1).ShouldNot(BeNil())

By("Patch with invalid hub template")
utils.Kubectl("patch", "configurationpolicy", case13PruneTmpErr, "--type=json", "-p",
`[{ "op": "replace",
"path": "/spec/object-templates/0/objectDefinition/data/test",
"value": '{{hub "default" "e2esecret" dddddd vvvvv d hub}}' }]`,
"-n", testNamespace)

By("By verifying that the configurationpolicy is NonCompliant")
Eventually(func() interface{} {
managedPlc := utils.GetWithTimeout(clientManagedDynamic, gvrConfigPolicy,
case13PruneTmpErr, testNamespace, true, defaultTimeoutSeconds)

return utils.GetComplianceState(managedPlc)
}, defaultTimeoutSeconds, 1).Should(Equal("NonCompliant"))

By("By verifying that the configmap still exist ")
Consistently(func() interface{} {
configmap := utils.GetWithTimeout(clientManagedDynamic, gvrConfigMap,
case13PruneTmpErr+"-configmap", "default", true, defaultTimeoutSeconds)

return configmap
}, defaultTimeoutSeconds, 1).ShouldNot(BeNil())

By("Change to valid configmap")
utils.Kubectl("patch", "configurationpolicy", case13PruneTmpErr, "--type=json", "-p",
`[{ "op": "replace",
"path": "/spec/object-templates/0/objectDefinition/data/test",
"value": "working" }]`,
"-n", testNamespace)

By("By verifying that the configmap has no error")
Eventually(func() interface{} {
managedPlc := utils.GetWithTimeout(clientManagedDynamic, gvrConfigPolicy,
case13PruneTmpErr, testNamespace, true, defaultTimeoutSeconds)

return utils.GetComplianceState(managedPlc)
}, defaultTimeoutSeconds, 1).Should(Equal("Compliant"))

By("Patch with invalid managed template")
utils.Kubectl("patch", "configurationpolicy", case13PruneTmpErr, "--type=json", "-p",
`[{ "op": "replace",
"path": "/spec/object-templates/0/objectDefinition/data/test",
"value": '{{ "default" "e2esecret" dddddd vvvvv d }}' }]`,
"-n", testNamespace)

By("By verifying that the configurationpolicy is NonCompliant")
Eventually(func() interface{} {
managedPlc := utils.GetWithTimeout(clientManagedDynamic, gvrConfigPolicy,
case13PruneTmpErr, testNamespace, true, defaultTimeoutSeconds)

return utils.GetComplianceState(managedPlc)
}, defaultTimeoutSeconds, 1).Should(Equal("NonCompliant"))

By("By verifying that the configmap still exist ")
Consistently(func() interface{} {
configmap := utils.GetWithTimeout(clientManagedDynamic, gvrConfigMap,
case13PruneTmpErr+"-configmap", "default", true, defaultTimeoutSeconds)

return configmap
}, defaultTimeoutSeconds, 1).ShouldNot(BeNil())
})
It("Cleans up", func() {
utils.Kubectl("delete", "configurationpolicy", case13PruneTmpErr,
"-n", testNamespace, "--ignore-not-found")
utils.Kubectl("delete", "configmap", case13PruneTmpErr+"-configmap",
"-n", "default", "--ignore-not-found")
utils.Kubectl("delete", "secret", case13Secret,
"-n", "default", "--ignore-not-found")
})
})
})
17 changes: 17 additions & 0 deletions test/resources/case13_templatization/case13_prune_template_error.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
apiVersion: policy.open-cluster-management.io/v1
kind: ConfigurationPolicy
metadata:
name: case13-prune-template-error
spec:
remediationAction: enforce
object-templates:
- complianceType: musthave
objectDefinition:
apiVersion: v1
kind: ConfigMap
metadata:
name: case13-prune-template-error-configmap
namespace: default
data:
test: '{{ print "doraemong" }}'