diff --git a/.tekton/config-policy-controller-acm-215-pull-request.yaml b/.tekton/config-policy-controller-acm-215-pull-request.yaml index 39cc746a..46b00387 100644 --- a/.tekton/config-policy-controller-acm-215-pull-request.yaml +++ b/.tekton/config-policy-controller-acm-215-pull-request.yaml @@ -8,7 +8,7 @@ metadata: build.appstudio.redhat.com/target_branch: '{{target_branch}}' pipelinesascode.tekton.dev/cancel-in-progress: "true" pipelinesascode.tekton.dev/max-keep-runs: "3" - pipelinesascode.tekton.dev/on-cel-expression: event == "pull_request" && target_branch == "main" + pipelinesascode.tekton.dev/on-cel-expression: event == "pull_request" && target_branch == "release-2.15" creationTimestamp: labels: appstudio.openshift.io/application: release-acm-215 diff --git a/controllers/configurationpolicy_controller.go b/controllers/configurationpolicy_controller.go index e7a3f2de..34bb4530 100644 --- a/controllers/configurationpolicy_controller.go +++ b/controllers/configurationpolicy_controller.go @@ -1357,8 +1357,37 @@ func (r *ConfigurationPolicyReconciler) determineDesiredObjects( return nil, nil, errEvent, nil } + skippedObjMsg := "All objects of kind %s were skipped by the `skipObject` template function" + scopedGVR, err := r.getMapping(objGVK, plc, index) if err != nil { + // Parse templates in a generic way to check whether skipObject was called + if tmplResolver != nil && templates.HasTemplate(objectT.ObjectDefinition.Raw, "", true) { + // Provide a full but empty context to prevent template errors + templateContext := struct { + Object map[string]any + ObjectNamespace string + ObjectName string + }{Object: map[string]any{}, ObjectNamespace: "", ObjectName: ""} + + _, skipObject, _ := resolveGoTemplates( + objectT.ObjectDefinition.Raw, + templateContext, + tmplResolver, + resolveOptions, + ) + + if skipObject { + event := &objectTmplEvalEvent{ + compliant: true, + reason: "", + message: fmt.Sprintf(skippedObjMsg, objGVK.Kind), + } + + return []*unstructured.Unstructured{}, nil, event, nil + } + } + errEvent := &objectTmplEvalEvent{ compliant: false, reason: "K8s error", @@ -1650,63 +1679,16 @@ func (r *ConfigurationPolicyReconciler) determineDesiredObjects( if tmplResolver != nil && hasTemplate && desiredObj == nil { //nolint:gocritic r.processedPolicyCache.Delete(plc.GetUID()) - var templateContext any - // Remove managedFields because it has a key that's just a dot, // which is problematic in the template library unstructured.RemoveNestedField(obj.Object, "metadata", "managedFields") - // Only populate context variables as they are available: - switch { - case name != "" && ns != "": - // - Namespaced object with metadata.name or objectSelector - templateContext = struct { - Object map[string]any - ObjectNamespace string - ObjectName string - }{Object: obj.Object, ObjectNamespace: ns, ObjectName: name} - - case name != "": - // - Cluster-scoped object with metadata.name or objectSelector - templateContext = struct { - Object map[string]any - ObjectName string - }{Object: obj.Object, ObjectName: name} - - case ns != "": - // - Unnamed namespaced object - templateContext = struct { - ObjectNamespace string - }{ObjectNamespace: ns} - } - - skipObject := false - - resolveOptions.CustomFunctions = map[string]any{ - "skipObject": func(skips ...any) (empty string, err error) { - switch len(skips) { - case 0: - skipObject = true - case 1: - if !skipObject { - if skip, ok := skips[0].(bool); ok { - skipObject = skip - } else { - err = fmt.Errorf( - "expected boolean but received '%v'", skips[0]) - } - } - default: - err = fmt.Errorf( - "expected one optional boolean argument but received %d arguments", len(skips)) - } - - return empty, err - }, - } + // Get the template context for resolving Go templates + templateContext := getTemplateContext(obj.Object, name, ns) - resolvedTemplate, err := tmplResolver.ResolveTemplate( - objectT.ObjectDefinition.Raw, templateContext, resolveOptions, + // Resolve the Go templates + resolvedTemplate, skipObject, err := resolveGoTemplates( + objectT.ObjectDefinition.Raw, templateContext, tmplResolver, resolveOptions, ) if skipObject { @@ -1844,7 +1826,7 @@ func (r *ConfigurationPolicyReconciler) determineDesiredObjects( switch { case skipObjectCalled: - msg = "All objects of kind %s were skipped by the `skipObject` template function" + msg = skippedObjMsg case objectSelector != nil: msg = "No objects of kind %s were matched from the policy objectSelector" default: diff --git a/controllers/configurationpolicy_utils.go b/controllers/configurationpolicy_utils.go index bae606e2..3a726593 100644 --- a/controllers/configurationpolicy_utils.go +++ b/controllers/configurationpolicy_utils.go @@ -13,6 +13,7 @@ import ( gocmp "github.com/google/go-cmp/cmp" "github.com/pmezard/go-difflib/difflib" + templates "github.com/stolostron/go-template-utils/v7/pkg/templates" depclient "github.com/stolostron/kubernetes-dependency-watches/client" apiRes "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -847,3 +848,80 @@ func generateDiff(existingObj, updatedObj *unstructured.Unstructured, fullDiffs return diff, nil } + +// getTemplateContext constructs a context object for Go template resolution. It +// selectively includes the object, name, and namespace depending on what +// information is available. +func getTemplateContext( + obj map[string]any, + name string, + ns string, +) (templateContext any) { + // Only populate context variables as they are available: + switch { + case name != "" && ns != "": + // - Namespaced object with metadata.name or objectSelector + return struct { + Object map[string]any + ObjectNamespace string + ObjectName string + }{Object: obj, ObjectNamespace: ns, ObjectName: name} + + case name != "": + // - Cluster-scoped object with metadata.name or objectSelector + return struct { + Object map[string]any + ObjectName string + }{Object: obj, ObjectName: name} + + case ns != "": + // - Unnamed namespaced object + return struct { + ObjectNamespace string + }{ObjectNamespace: ns} + } + + return nil +} + +// resolveGoTemplates resolves Go templates in the given raw object using the +// provided template context and template resolver. It returns the resolved +// template, a boolean indicating whether the object should be skipped, and an +// error if any occurred. +func resolveGoTemplates( + rawObj []byte, + templateContext any, + tmplResolver *templates.TemplateResolver, + resolveOptions *templates.ResolveOptions, +) ( + resolvedTemplate templates.TemplateResult, + skipObject bool, + err error, +) { + resolveOptions.CustomFunctions = map[string]any{ + "skipObject": func(skips ...any) (empty string, err error) { + switch len(skips) { + case 0: + skipObject = true + case 1: + if !skipObject { + if skip, ok := skips[0].(bool); ok { + skipObject = skip + } else { + err = fmt.Errorf( + "expected boolean but received '%v'", skips[0]) + } + } + default: + err = fmt.Errorf( + "expected one optional boolean argument but received %d arguments", len(skips)) + } + + return empty, err + }, + } + + resolvedTemplate, err = tmplResolver.ResolveTemplate(rawObj, templateContext, resolveOptions) + + return resolvedTemplate, skipObject, err +} diff --git a/controllers/configurationpolicy_utils_test.go b/controllers/configurationpolicy_utils_test.go index d92f926e..6afa025a 100644 --- a/controllers/configurationpolicy_utils_test.go +++ b/controllers/configurationpolicy_utils_test.go @@ -5,8 +5,11 @@ import ( "strings" "testing" + "github.com/stolostron/go-template-utils/v7/pkg/templates" "github.com/stretchr/testify/assert" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/client-go/dynamic/fake" + "k8s.io/client-go/kubernetes/scheme" policyv1 "open-cluster-management.io/config-policy-controller/api/v1" ) @@ -301,3 +304,260 @@ func TestGenerateDiff(t *testing.T) { }) } } + +func TestGetTemplateContext(t *testing.T) { + t.Parallel() + + testObj := map[string]any{ + "apiVersion": "v1", + "kind": "ConfigMap", + "metadata": map[string]any{ + "name": "test-configmap", + }, + } + + tests := map[string]struct { + obj map[string]any + name string + ns string + expected any + }{ + "namespaced object with name and namespace": { + obj: testObj, + name: "my-configmap", + ns: "default", + expected: struct { + Object map[string]any + ObjectNamespace string + ObjectName string + }{ + Object: testObj, + ObjectNamespace: "default", + ObjectName: "my-configmap", + }, + }, + "cluster-scoped object with name only": { + obj: testObj, + name: "my-clusterrole", + ns: "", + expected: struct { + Object map[string]any + ObjectName string + }{ + Object: testObj, + ObjectName: "my-clusterrole", + }, + }, + "unnamed namespaced object with namespace only": { + obj: testObj, + name: "", + ns: "kube-system", + expected: struct { + ObjectNamespace string + }{ + ObjectNamespace: "kube-system", + }, + }, + "no name and no namespace": { + obj: testObj, + name: "", + ns: "", + expected: nil, + }, + "empty object with name and namespace": { + obj: map[string]any{}, + name: "empty-obj", + ns: "test-ns", + expected: struct { + Object map[string]any + ObjectNamespace string + ObjectName string + }{ + Object: map[string]any{}, + ObjectNamespace: "test-ns", + ObjectName: "empty-obj", + }, + }, + "nil object with name and namespace": { + obj: nil, + name: "nil-obj", + ns: "test-ns", + expected: struct { + Object map[string]any + ObjectNamespace string + ObjectName string + }{ + Object: nil, + ObjectNamespace: "test-ns", + ObjectName: "nil-obj", + }, + }, + } + + for testName, test := range tests { + t.Run(testName, func(t *testing.T) { + t.Parallel() + + result := getTemplateContext(test.obj, test.name, test.ns) + + assert.Equal(t, test.expected, result) + }) + } +} + +func TestResolveGoTemplates(t *testing.T) { + t.Parallel() + + // Create a fake dynamic client and template resolver for testing + dynamicClient := fake.NewSimpleDynamicClient(scheme.Scheme) + tmplResolver, err := templates.NewResolverWithClients( + dynamicClient, nil, templates.Config{}, + ) + assert.NoError(t, err) + + tests := map[string]struct { + rawObj string + templateContext any + expectSkipObject bool + expectError bool + expectedResolved string + errorContains string + }{ + "simple template without skipObject": { + rawObj: `apiVersion: v1 +kind: ConfigMap +metadata: + name: test + namespace: '{{ .ObjectNamespace }}'`, + templateContext: struct { + ObjectNamespace string + }{ + ObjectNamespace: "test-namespace", + }, + expectSkipObject: false, + expectError: false, + expectedResolved: `{"apiVersion":"v1",` + + `"kind":"ConfigMap",` + + `"metadata":{` + + `"name":"test",` + + `"namespace":"test-namespace"}}`, + }, + "template with skipObject no arguments": { + rawObj: `apiVersion: v1 +kind: ConfigMap +metadata: + name: '{{ if true }}{{ skipObject }}{{ end }}test'`, + templateContext: nil, + expectSkipObject: true, + expectError: false, + expectedResolved: `{"apiVersion":"v1",` + + `"kind":"ConfigMap",` + + `"metadata":{"name":"test"}}`, + }, + "template with skipObject true": { + rawObj: `apiVersion: v1 +kind: ConfigMap +metadata: + name: '{{ skipObject true }}test'`, + templateContext: nil, + expectSkipObject: true, + expectError: false, + expectedResolved: `{"apiVersion":"v1",` + + `"kind":"ConfigMap",` + + `"metadata":{"name":"test"}}`, + }, + "template with skipObject false": { + rawObj: `apiVersion: v1 +kind: ConfigMap +metadata: + name: '{{ skipObject false }}test'`, + templateContext: nil, + expectSkipObject: false, + expectError: false, + expectedResolved: `{"apiVersion":"v1",` + + `"kind":"ConfigMap",` + + `"metadata":{"name":"test"}}`, + }, + "template with skipObject invalid argument": { + rawObj: `apiVersion: v1 +kind: ConfigMap +metadata: + name: '{{ skipObject "invalid" }}test'`, + templateContext: nil, + expectSkipObject: false, + expectError: true, + errorContains: "expected boolean but received", + }, + "template with skipObject multiple arguments": { + rawObj: `apiVersion: v1 +kind: ConfigMap +metadata: + name: '{{ skipObject true false }}test'`, + templateContext: nil, + expectSkipObject: false, + expectError: true, + errorContains: "expected one optional boolean argument but received 2 arguments", + }, + "template without any templates": { + rawObj: `apiVersion: v1 +kind: ConfigMap +metadata: + name: plain-configmap`, + templateContext: nil, + expectSkipObject: false, + expectError: false, + expectedResolved: `{"apiVersion":"v1",` + + `"kind":"ConfigMap",` + + `"metadata":{` + + `"name":"plain-configmap"}}`, + }, + "template with context variables": { + rawObj: `apiVersion: v1 +kind: Pod +metadata: + name: '{{ .ObjectName }}' + namespace: '{{ .ObjectNamespace }}'`, + templateContext: struct { + ObjectName string + ObjectNamespace string + }{ + ObjectName: "test-pod", + ObjectNamespace: "test-namespace", + }, + expectSkipObject: false, + expectError: false, + expectedResolved: `{"apiVersion":"v1",` + + `"kind":"Pod",` + + `"metadata":{` + + `"name":"test-pod",` + + `"namespace":"test-namespace"}}`, + }, + } + + for testName, test := range tests { + t.Run(testName, func(t *testing.T) { + t.Parallel() + + resolveOptions := &templates.ResolveOptions{} + resolvedTemplate, skipObject, err := resolveGoTemplates( + []byte(test.rawObj), + test.templateContext, + tmplResolver, + resolveOptions, + ) + + if test.expectError { + assert.Error(t, err) + + if test.errorContains != "" { + assert.Contains(t, err.Error(), test.errorContains) + } + } else { + assert.NoError(t, err) + assert.JSONEq(t, test.expectedResolved, string(resolvedTemplate.ResolvedJSON)) + } + + assert.Equal(t, test.expectSkipObject, skipObject) + }) + } +} diff --git a/test/e2e/case44_template_vars_test.go b/test/e2e/case44_template_vars_test.go index 1bd49654..21ec68de 100644 --- a/test/e2e/case44_template_vars_test.go +++ b/test/e2e/case44_template_vars_test.go @@ -351,6 +351,37 @@ var _ = Describe("Test template context variables", func() { )) }, defaultTimeoutSeconds, 1).Should(Succeed()) }) + + It("Should be compliant when skipObject is used with a non-existent CRD (ACM-23563)", func() { + const ( + policyName = "case44-skipobject-missing-crd" + ) + + By("Applying the " + policyName + " ConfigurationPolicy with a non-existent kind") + utils.KubectlApplyAndLabel( + testLabel, "-n", testNamespace, "-f", rsrcPath+"case44_skipobject_missing_crd.yaml") + + By("By verifying that the ConfigurationPolicy is compliant without CRD errors") + Eventually(func(g Gomega) { + managedPlc := utils.GetWithTimeout( + clientManagedDynamic, + gvrConfigPolicy, + policyName, + testNamespace, + true, + defaultTimeoutSeconds, + ) + + utils.CheckComplianceStatus(g, managedPlc, "Compliant") + g.Expect(utils.GetStatusMessage(managedPlc)).To(Equal( + "All objects of kind FakeKind were skipped by the `skipObject` template function", + )) + + // Verify there are no related objects since everything was skipped + relatedObjects, _, _ := unstructured.NestedSlice(managedPlc.Object, "status", "relatedObjects") + g.Expect(relatedObjects).To(BeEmpty()) + }, defaultTimeoutSeconds, 1).Should(Succeed()) + }) }) Describe("Policy with the Object variable", Ordered, func() { diff --git a/test/resources/case44_template_vars/case44_skipobject_missing_crd.yaml b/test/resources/case44_template_vars/case44_skipobject_missing_crd.yaml new file mode 100644 index 00000000..5417a9eb --- /dev/null +++ b/test/resources/case44_template_vars/case44_skipobject_missing_crd.yaml @@ -0,0 +1,23 @@ +apiVersion: policy.open-cluster-management.io/v1 +kind: ConfigurationPolicy +metadata: + name: case44-skipobject-missing-crd + labels: + e2e-test: case44-objectname-var +spec: + remediationAction: inform + severity: medium + object-templates: + - complianceType: musthave + objectDefinition: + apiVersion: v1 + kind: FakeKind + metadata: + name: test-skipobject-missing-crd + namespace: default + labels: + # skipObject is called to skip this object since the CRD doesn't exist + removed-in-ocp419: '{{ skipObject }}' + spec: + data: + fake: test