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.

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

Bug: Creating two policy templates with the same name succeeds #69

Merged
merged 1 commit into from
Jun 9, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
38 changes: 38 additions & 0 deletions controllers/templatesync/template_sync.go
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,19 @@ func (r *PolicyReconciler) Reconcile(ctx context.Context, request reconcile.Requ
return reconcile.Result{}, nil
}

// Check duplicate names in configuration-policies
has := hasDupName(instance)
if has {
msg := "There are duplicate names in configurationpolicies, please check the policy"
reqLogger.Info(msg)

for tIndex := range instance.Spec.PolicyTemplates {
_ = r.emitTemplateError(ctx, instance, tIndex, fmt.Sprintf("[template %v]", tIndex), false, msg)
}

return reconcile.Result{}, nil
}

// Whether policy needs a finalizer to handle cleanup
var addFinalizer bool

Expand Down Expand Up @@ -1507,3 +1520,28 @@ func finalizerCleanup(
func (r *PolicyReconciler) setCreatedGkConstraint(b bool) {
r.createdGkConstraint = &b
}

// Check duplicate names in policy-templates(configurationPolicies)
func hasDupName(pol *policiesv1.Policy) bool {
templates := pol.Spec.PolicyTemplates

foundNames := make(map[string]struct{})

for _, v := range templates {
unstructured, err := unmarshalFromJSON(v.ObjectDefinition.Raw)
if err != nil {
// Skip unmarshal error here, template error should appear later
return false
}

name := unstructured.GetName()

if _, has := foundNames[name]; has {
return true
}

foundNames[name] = struct{}{}
}

return false
}
20 changes: 20 additions & 0 deletions controllers/templatesync/template_utils.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
package templatesync

import (
"encoding/json"

"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
)

// unmarshalFromJSON unmarshals raw JSON data into an object
func unmarshalFromJSON(rawData []byte) (unstructured.Unstructured, error) {
var unstruct unstructured.Unstructured

if jsonErr := json.Unmarshal(rawData, &unstruct.Object); jsonErr != nil {
log.Error(jsonErr, "Could not unmarshal data from JSON")

return unstruct, jsonErr
}

return unstruct, nil
}
25 changes: 23 additions & 2 deletions test/e2e/case10_error_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,11 @@ import (

var _ = Describe("Test error handling", func() {
const (
caseNumber = "case10"
yamlBasePath = "../resources/" + caseNumber + "_template_sync_error_test/"
caseNumber = "case10"
yamlBasePath = "../resources/" + caseNumber + "_template_sync_error_test/"
dupNamePolicyYaml = yamlBasePath + "dup-name-policy.yaml"
dupNamePolicyName = "dup-policy"
dupConfigPolicyName = "policy-config-dup"
)

AfterEach(func() {
Expand All @@ -29,6 +32,24 @@ var _ = Describe("Test error handling", func() {
_, err = kubectlManaged("delete", "events", "--all", "-A")
Expect(err).ToNot(HaveOccurred())
})
It("should not reconcile the policy when there are same names in policy-template", func() {
By("Creating policy")
hubApplyPolicy(dupNamePolicyName, dupNamePolicyYaml)

By("Should generate warning events")
Eventually(
checkForEvent(dupNamePolicyName,
"There are duplicate names in configurationpolicies, please check the policy"),
defaultTimeoutSeconds,
1,
).Should(BeTrue())

By("Should not create any configuration policy")
Consistently(func() interface{} {
return utils.GetWithTimeout(clientManagedDynamic, gvrConfigurationPolicy, dupConfigPolicyName,
testNamespace, false, defaultTimeoutSeconds)
}, 10, 1).Should(BeNil())
})
It("should not override remediationAction if doesn't exist on parent policy", func() {
hubApplyPolicy("case10-remediation-action-not-exists",
yamlBasePath+"remediation-action-not-exists.yaml")
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
apiVersion: policy.open-cluster-management.io/v1
kind: Policy
metadata:
name: dup-policy
labels:
policy.open-cluster-management.io/cluster-name: managed
policy.open-cluster-management.io/cluster-namespace: managed
policy.open-cluster-management.io/root-policy: dup-policy
spec:
disabled: false
policy-templates:
- objectDefinition:
apiVersion: policy.open-cluster-management.io/v1
kind: ConfigurationPolicy
metadata:
name: policy-config-dup
spec:
namespaceSelector:
exclude:
- kube-*
include:
- default
object-templates:
- complianceType: musthave
objectDefinition:
apiVersion: v1
kind: Pod
metadata:
name: pod-2
spec:
containers:
- name: nginx
image: nginx:1.18.0
ports:
- containerPort: 80
remediationAction: inform
severity: low
- objectDefinition:
apiVersion: policy.open-cluster-management.io/v1
kind: ConfigurationPolicy
metadata:
name: policy-config-dup
spec:
object-templates:
- complianceType: musthave
objectDefinition:
apiVersion: v1
kind: Namespace
metadata:
name: my-namespace
remediationAction: inform
severity: low