Skip to content

Commit

Permalink
Emit template-error if template already exists
Browse files Browse the repository at this point in the history
Previously, if a Policy defined a template that already exists, it
assumed that another Policy had defined it, and tried to get its owner
reference in order to give a helpful error message. But if the template
was created outside of a Policy, then this process would panic.

Refs:
 - https://issues.redhat.com/browse/ACM-3416

Signed-off-by: Justin Kulikauskas <[email protected]>
  • Loading branch information
JustinKuli authored and openshift-merge-robot committed Feb 14, 2023
1 parent 8654c6f commit c07ac53
Show file tree
Hide file tree
Showing 3 changed files with 70 additions and 6 deletions.
31 changes: 25 additions & 6 deletions controllers/templatesync/template_sync.go
Original file line number Diff line number Diff line change
Expand Up @@ -424,14 +424,33 @@ func (r *PolicyReconciler) Reconcile(ctx context.Context, request reconcile.Requ
continue
}

refName := eObject.GetOwnerReferences()[0].Name
refName := ""

for _, ownerref := range eObject.GetOwnerReferences() {
refName = ownerref.Name

break // just get the first ownerReference, if there are any at all
}

// violation if object reference and policy don't match
if instance.GetName() != refName {
errMsg := fmt.Sprintf(
"Template name must be unique. Policy template with kind: %s name: %s already exists in policy %s",
tObjectUnstructured.Object["kind"],
tName,
refName)
var errMsg string

if refName == "" {
errMsg = fmt.Sprintf(
"Template name must be unique. Policy template with "+
"kind: %s name: %s already exists outside of a Policy",
tObjectUnstructured.Object["kind"],
tName)
} else {
errMsg = fmt.Sprintf(
"Template name must be unique. Policy template with "+
"kind: %s name: %s already exists in policy %s",
tObjectUnstructured.Object["kind"],
tName,
refName)
}

resultError = errors.NewBadRequest(errMsg)

r.emitTemplateError(instance, tIndex, tName, errMsg)
Expand Down
28 changes: 28 additions & 0 deletions test/e2e/case10_error_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,34 @@ var _ = Describe("Test error handling", func() {
1,
).Should(BeFalse())
})
It("should throw a noncompliance event if the template already exists outside of a policy", func() {
By("Creating the ConfigurationPolicy on the managed cluster directly")
_, err := kubectlManaged(
"apply",
"--filename=../resources/case10_template_sync_error_test/working-policy-configpol.yaml",
"--namespace="+clusterNamespace,
)
Expect(err).Should(BeNil())

managedPlc := utils.GetWithTimeout(
clientManagedDynamic,
gvrConfigurationPolicy,
"case10-config-policy",
clusterNamespace,
true,
defaultTimeoutSeconds)
ExpectWithOffset(1, managedPlc).NotTo(BeNil())

hubApplyPolicy("case10-test-policy",
"../resources/case10_template_sync_error_test/working-policy.yaml")

By("Checking for the error event")
Eventually(
checkForEvent("case10-test-policy", "already exists outside of a Policy"),
defaultTimeoutSeconds,
1,
).Should(BeTrue())
})
})

// Checks for an event on the managed cluster
Expand Down
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: case10-config-policy
spec:
remediationAction: inform
object-templates:
- complianceType: musthave
objectDefinition:
apiVersion: v1
kind: Pod
metadata:
name: nginx-pod-e2e
namespace: default
spec:
containers:
- name: nginx

0 comments on commit c07ac53

Please sign in to comment.