Skip to content

Commit

Permalink
Bug: Objects are pruned on templating errors
Browse files Browse the repository at this point in the history 
Signed-off-by: Yi Rae Kim <yikim@redhat.com>
  • Loading branch information
@yiraeChristineKim
yiraeChristineKim committed May 31, 2023
1 parent 0499944 commit b01efd1
Show file tree
Hide file tree
Showing 4 changed files with 147 additions and 12 deletions.
31 changes: 22 additions & 9 deletions controllers/configurationpolicy_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -810,7 +810,9 @@ func (r *ConfigurationPolicyReconciler) handleObjectTemplates(plc policyv1.Confi
}
}

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

if reason == "" {
Expand All @@ -829,7 +831,7 @@ func (r *ConfigurationPolicyReconciler) handleObjectTemplates(plc policyv1.Confi
)
}

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

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

addTemplateErrorViolation("Error processing hub templates", hubTemplatesErrMsg)
addTemplateErrorViolation("Error processing hub templates", hubTemplatesErrMsg, true)

return
}
Expand Down Expand Up @@ -967,7 +969,7 @@ func (r *ConfigurationPolicyReconciler) handleObjectTemplates(plc policyv1.Confi
encryptionConfig, usedKeyCache, err = r.getEncryptionConfig(plc, true)

if err != nil {
addTemplateErrorViolation("", err.Error())
addTemplateErrorViolation("", err.Error(), true)

return
}
Expand All @@ -982,7 +984,7 @@ func (r *ConfigurationPolicyReconciler) handleObjectTemplates(plc policyv1.Confi
}

msg := getTemplateConfigErrorMsg(err)
addTemplateErrorViolation("", msg)
addTemplateErrorViolation("", msg, true)

return
}
Expand All @@ -991,7 +993,7 @@ func (r *ConfigurationPolicyReconciler) handleObjectTemplates(plc policyv1.Confi
}

if tplErr != nil {
addTemplateErrorViolation("", tplErr.Error())
addTemplateErrorViolation("", tplErr.Error(), true)

return
}
Expand All @@ -1000,7 +1002,7 @@ func (r *ConfigurationPolicyReconciler) handleObjectTemplates(plc policyv1.Confi
if isRawObjTemplate {
err := json.Unmarshal(resolvedTemplate.ResolvedJSON, &objTemps)
if err != nil {
addTemplateErrorViolation("Error unmarshalling raw template", err.Error())
addTemplateErrorViolation("Error unmarshalling raw template", err.Error(), true)

return
}
Expand Down Expand Up @@ -1193,8 +1195,14 @@ func (r *ConfigurationPolicyReconciler) handleObjectTemplates(plc policyv1.Confi
// 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,
isTemplateParseErr ...bool,
) {
r.sortRelatedObjectsAndUpdate(&plc, related, oldRelated, r.EnableMetrics)
tempParseErr := false
if len(isTemplateParseErr) > 0 {
tempParseErr = isTemplateParseErr[0]
}

r.sortRelatedObjectsAndUpdate(&plc, related, oldRelated, r.EnableMetrics, tempParseErr)
// An update always occurs to account for the lastEvaluated status field
r.addForUpdate(&plc, sendEvent)
}
Expand All @@ -1203,6 +1211,7 @@ func (r *ConfigurationPolicyReconciler) checkRelatedAndUpdate(
func (r *ConfigurationPolicyReconciler) sortRelatedObjectsAndUpdate(
plc *policyv1.ConfigurationPolicy, related, oldRelated []policyv1.RelatedObject,
collectMetrics bool,
isTemplateParseErr bool,
) {
sort.SliceStable(related, func(i, j int) bool {
if related[i].Object.Kind != related[j].Object.Kind {
Expand Down Expand Up @@ -1273,7 +1282,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 !isTemplateParseErr {
r.deleteDetachedObj(*plc, related, oldRelated)
}

plc.Status.RelatedObjects = related
}
}
Expand Down
6 changes: 3 additions & 3 deletions controllers/configurationpolicy_controller_test.go
View file
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, false)
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, false)
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, false)
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
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
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" }}'

0 comments on commit b01efd1

Please sign in to comment.