Merge branch 'main' into Add-a-metric-to-measure-how-long-resolving-p…

…olicy-templates-take

Makefile

@@ -177,8 +177,8 @@ deploy: generate-operator-yaml
 
 .PHONY: create-ns
 create-ns:
-	@kubectl create namespace $(CONTROLLER_NAMESPACE) || true
-	@kubectl create namespace $(WATCH_NAMESPACE) || true
+	-@kubectl create namespace $(CONTROLLER_NAMESPACE)
+	-@kubectl create namespace $(WATCH_NAMESPACE)
 
 # Run against the current locally configured Kubernetes cluster
 .PHONY: run
@@ -239,7 +239,7 @@ kind-bootstrap-cluster-dev: kind-create-cluster install-crds install-resources
 .PHONY: kind-deploy-controller
 kind-deploy-controller: generate-operator-yaml
 	@echo Installing $(IMG)
-	kubectl create ns $(KIND_NAMESPACE) || true
+	-kubectl create ns $(KIND_NAMESPACE)
 	kubectl apply -f deploy/operator.yaml -n $(KIND_NAMESPACE)
 	kubectl patch deployment $(IMG) -n $(KIND_NAMESPACE) -p "{\"spec\":{\"template\":{\"spec\":{\"containers\":[{\"name\":\"$(IMG)\",\"env\":[{\"name\":\"WATCH_NAMESPACE\",\"value\":\"$(WATCH_NAMESPACE)\"}]}]}}}}"
 

api/v1/configurationpolicy_types.go

@@ -210,7 +210,7 @@ type ConfigurationPolicy struct {
 	metav1.TypeMeta   `json:",inline"`
 	metav1.ObjectMeta `json:"metadata,omitempty"`
 
-	Spec   ConfigurationPolicySpec   `json:"spec,omitempty"`
+	Spec   *ConfigurationPolicySpec  `json:"spec,omitempty"`
 	Status ConfigurationPolicyStatus `json:"status,omitempty"`
 }
 

controllers/configurationpolicy_controller.go

@@ -38,7 +38,6 @@ import (
 	"k8s.io/kubectl/pkg/validation"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
-	"sigs.k8s.io/controller-runtime/pkg/metrics"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 
 	policyv1 "open-cluster-management.io/config-policy-controller/api/v1"
@@ -180,6 +179,8 @@ type ConfigurationPolicyReconciler struct {
 	// where the controller is running.
 	TargetK8sClient kubernetes.Interface
 	TargetK8sConfig *rest.Config
+	// Whether custom metrics collection is enabled
+	EnableMetrics bool
 	discoveryInfo
 	// This is used to fetch and parse OpenAPI documents to perform client-side validation of object definitions.
 	openAPIParser *openapi.CachedOpenAPIParser
@@ -203,6 +204,10 @@ func (r *ConfigurationPolicyReconciler) Reconcile(ctx context.Context, request c
 		_ = plcTempsProcessCounter.DeleteLabelValues(request.Name)
 		_ = compareObjEvalCounter.DeletePartialMatch(prometheus.Labels{"config_policy_name": request.Name})
 		_ = compareObjSecondsCounter.DeletePartialMatch(prometheus.Labels{"config_policy_name": request.Name})
+		_ = policyRelatedObjectGauge.DeletePartialMatch(
+			prometheus.Labels{"policy": fmt.Sprintf("%s/%s", request.Namespace, request.Name)})
+		_ = policyUserErrorsCounter.DeletePartialMatch(prometheus.Labels{"template": request.Name})
+		_ = policySystemErrorsCounter.DeletePartialMatch(prometheus.Labels{"template": request.Name})
 	}
 
 	return reconcile.Result{}, nil
@@ -256,6 +261,9 @@ func (r *ConfigurationPolicyReconciler) PeriodicallyExecConfigPolicies(freq uint
 		if !skipLoop {
 			log.Info("Processing the policies", "count", len(policiesList.Items))
 
+			// Initialize the related object map
+			policyRelatedObjectMap = sync.Map{}
+
 			for i := 0; i < int(r.EvaluationConcurrency); i++ {
 				wg.Add(1)
 
@@ -276,6 +284,11 @@ func (r *ConfigurationPolicyReconciler) PeriodicallyExecConfigPolicies(freq uint
 		close(policyQueue)
 		wg.Wait()
 
+		// Update the related object metric after policy processing
+		if r.EnableMetrics {
+			updateRelatedObjectMetric()
+		}
+		// Update the evaluation histogram with the elapsed time
 		elapsed := time.Since(start).Seconds()
 		evalLoopHistogram.Observe(elapsed)
 		// making sure that if processing is > freq we don't sleep
@@ -377,9 +390,9 @@ func shouldEvaluatePolicy(policy *policyv1.ConfigurationPolicy) bool {
 
 	var interval time.Duration
 
-	if policy.Status.ComplianceState == policyv1.Compliant {
+	if policy.Status.ComplianceState == policyv1.Compliant && policy.Spec != nil {
 		interval, err = policy.Spec.EvaluationInterval.GetCompliantInterval()
-	} else if policy.Status.ComplianceState == policyv1.NonCompliant {
+	} else if policy.Status.ComplianceState == policyv1.NonCompliant && policy.Spec != nil {
 		interval, err = policy.Spec.EvaluationInterval.GetNonCompliantInterval()
 	} else {
 		log.V(2).Info("The policy has an unknown compliance. Will evaluate it now.")
@@ -627,11 +640,18 @@ func (r *ConfigurationPolicyReconciler) handleObjectTemplates(plc policyv1.Confi
 	relatedObjects := []policyv1.RelatedObject{}
 	parentStatusUpdateNeeded := false
 
-	// error if no remediationAction is specified
-	if plc.Spec.RemediationAction == "" {
-		message := "Policy does not have a RemediationAction specified"
+	validationErr := ""
+	if plc.Spec == nil {
+		validationErr = "Policy does not have a Spec specified"
+	} else if plc.Spec.RemediationAction == "" {
+		validationErr = "Policy does not have a RemediationAction specified"
+	}
+
+	// error if no spec or remediationAction is specified
+	if validationErr != "" {
+		message := validationErr
 		log.Info(message)
-		statusChanged := addConditionToStatus(&plc, 0, false, "No RemediationAction", message)
+		statusChanged := addConditionToStatus(&plc, 0, false, "Invalid spec", message)
 
 		if statusChanged {
 			r.Recorder.Event(&plc, eventWarning,
@@ -640,6 +660,13 @@ func (r *ConfigurationPolicyReconciler) handleObjectTemplates(plc policyv1.Confi
 
 		r.checkRelatedAndUpdate(plc, relatedObjects, oldRelated, statusChanged)
 
+		parent := ""
+		if len(plc.OwnerReferences) > 0 {
+			parent = plc.OwnerReferences[0].Name
+		}
+
+		policyUserErrorsCounter.WithLabelValues(parent, plc.GetName(), "invalid-template").Add(1)
+
 		return
 	}
 
@@ -1002,14 +1029,14 @@ func (r *ConfigurationPolicyReconciler) handleObjectTemplates(plc policyv1.Confi
 func (r *ConfigurationPolicyReconciler) checkRelatedAndUpdate(
 	plc policyv1.ConfigurationPolicy, related, oldRelated []policyv1.RelatedObject, sendEvent bool,
 ) {
-	sortRelatedObjectsAndUpdate(&plc, related, oldRelated)
+	sortRelatedObjectsAndUpdate(&plc, related, oldRelated, r.EnableMetrics)
 	// An update always occurs to account for the lastEvaluated status field
 	r.addForUpdate(&plc, sendEvent)
 }
 
 // helper function to check whether related objects has changed
 func sortRelatedObjectsAndUpdate(
-	plc *policyv1.ConfigurationPolicy, related, oldRelated []policyv1.RelatedObject,
+	plc *policyv1.ConfigurationPolicy, related, oldRelated []policyv1.RelatedObject, collectMetrics bool,
 ) {
 	sort.SliceStable(related, func(i, j int) bool {
 		if related[i].Object.Kind != related[j].Object.Kind {
@@ -1024,7 +1051,33 @@ func sortRelatedObjectsAndUpdate(
 
 	update := false
 
+	// Instantiate found objects for the related object metric
+	found := map[string]bool{}
+
+	if collectMetrics {
+		for _, obj := range oldRelated {
+			found[getObjectString(obj)] = false
+		}
+	}
+
+	// Format policy for related object metric
+	policyVal := fmt.Sprintf("%s/%s", plc.Namespace, plc.Name)
+
 	for i, newEntry := range related {
+		var objKey string
+		// Collect the policy and related object for related object metric
+		if collectMetrics {
+			objKey = getObjectString(newEntry)
+			policiesArray := []string{}
+
+			if objValue, ok := policyRelatedObjectMap.Load(objKey); ok {
+				policiesArray = append(policiesArray, objValue.([]string)...)
+			}
+
+			policiesArray = append(policiesArray, policyVal)
+			policyRelatedObjectMap.Store(objKey, policiesArray)
+		}
+
 		for _, oldEntry := range oldRelated {
 			// Get matching objects
 			if gocmp.Equal(newEntry.Object, oldEntry.Object) {
@@ -1034,11 +1087,27 @@ func sortRelatedObjectsAndUpdate(
 					!(*newEntry.Properties.CreatedByPolicy) {
 					// Use the old properties if they existed and this is not a newly created resource
 					related[i].Properties = oldEntry.Properties
+
+					if collectMetrics {
+						found[objKey] = true
+					}
+
+					break
 				}
 			}
 		}
 	}
 
+	// Clean up old related object metrics if the related object list changed
+	if collectMetrics {
+		for _, obj := range oldRelated {
+			objString := getObjectString(obj)
+			if !found[objString] {
+				_ = policyRelatedObjectGauge.DeleteLabelValues(objString, policyVal)
+			}
+		}
+	}
+
 	if len(oldRelated) == len(related) {
 		for i, entry := range oldRelated {
 			if !gocmp.Equal(entry, related[i]) {
@@ -1080,11 +1149,11 @@ func addConditionToStatus(
 
 	log := log.WithValues("policy", plc.GetName(), "complianceState", complianceState)
 
-	if compliant && plc.Spec.EvaluationInterval.Compliant == "never" {
+	if compliant && plc.Spec != nil && plc.Spec.EvaluationInterval.Compliant == "never" {
 		msg := `This policy will not be evaluated again due to spec.evaluationInterval.compliant being set to "never"`
 		log.Info(msg)
 		cond.Message += fmt.Sprintf(". %s.", msg)
-	} else if !compliant && plc.Spec.EvaluationInterval.NonCompliant == "never" {
+	} else if !compliant && plc.Spec != nil && plc.Spec.EvaluationInterval.NonCompliant == "never" {
 		msg := "This policy will not be evaluated again due to spec.evaluationInterval.noncompliant " +
 			`being set to "never"`
 		log.Info(msg)
@@ -1645,7 +1714,15 @@ func (r *ConfigurationPolicyReconciler) getMapping(
 			kind := afterPrefix[0:(strings.Index(afterPrefix, "\" "))]
 			mappingErrMsg = "couldn't find mapping resource with kind " + kind +
 				", please check if you have CRD deployed"
+
 			log.Error(err, "Could not map resource, do you have the CRD deployed?", "kind", kind)
+
+			parent := ""
+			if len(policy.OwnerReferences) > 0 {
+				parent = policy.OwnerReferences[0].Name
+			}
+
+			policyUserErrorsCounter.WithLabelValues(parent, policy.GetName(), "no-object-CRD").Add(1)
 		}
 
 		errMsg := err.Error()
@@ -2421,12 +2498,16 @@ func IsSimilarToLastCondition(oldCond policyv1.Condition, newCond policyv1.Condi
 func (r *ConfigurationPolicyReconciler) addForUpdate(policy *policyv1.ConfigurationPolicy, sendEvent bool) {
 	compliant := true
 
-	for index := range policy.Spec.ObjectTemplates {
-		if index < len(policy.Status.CompliancyDetails) {
-			if policy.Status.CompliancyDetails[index].ComplianceState == policyv1.NonCompliant {
-				compliant = false
+	if policy.Spec == nil {
+		compliant = false
+	} else {
+		for index := range policy.Spec.ObjectTemplates {
+			if index < len(policy.Status.CompliancyDetails) {
+				if policy.Status.CompliancyDetails[index].ComplianceState == policyv1.NonCompliant {
+					compliant = false
 
-				break
+					break
+				}
 			}
 		}
 	}
@@ -2436,7 +2517,7 @@ func (r *ConfigurationPolicyReconciler) addForUpdate(policy *policyv1.Configurat
 	if policy.ObjectMeta.DeletionTimestamp != nil {
 		policy.Status.ComplianceState = policyv1.Terminating
 	} else if len(policy.Status.CompliancyDetails) == 0 {
-		policy.Status.ComplianceState = "Undetermined"
+		policy.Status.ComplianceState = policyv1.UnknownCompliancy
 	} else if compliant {
 		policy.Status.ComplianceState = policyv1.Compliant
 	} else {
@@ -2463,6 +2544,13 @@ func (r *ConfigurationPolicyReconciler) addForUpdate(policy *policyv1.Configurat
 		policyLog.Error(err, "Tried to re-update status before previous update could be applied, retrying next loop")
 	} else if err != nil {
 		policyLog.Error(err, "Could not update status, retrying next loop")
+
+		parent := ""
+		if len(policy.OwnerReferences) > 0 {
+			parent = policy.OwnerReferences[0].Name
+		}
+
+		policySystemErrorsCounter.WithLabelValues(parent, policy.GetName(), "status-update-failed").Add(1)
 	}
 }
 
@@ -2472,7 +2560,7 @@ func (r *ConfigurationPolicyReconciler) updatePolicyStatus(
 	policy *policyv1.ConfigurationPolicy,
 	sendEvent bool,
 ) (*policyv1.ConfigurationPolicy, error) {
-	if policy.Status.ComplianceState != "Undetermined" && sendEvent {
+	if sendEvent {
 		log.V(1).Info("Sending parent policy compliance event")
 
 		// If the compliance event can't be created, then don't update the ConfigurationPolicy
@@ -2545,20 +2633,17 @@ func (r *ConfigurationPolicyReconciler) sendComplianceEvent(instance *policyv1.C
 		ReportingInstance:   r.InstanceName,
 	}
 
-	if instance.Status.ComplianceState == policyv1.NonCompliant {
+	if instance.Status.ComplianceState != policyv1.Compliant {
 		event.Type = "Warning"
 	}
 
-	eventClient := r.TargetK8sClient.CoreV1().Events(instance.Namespace)
-	_, err := eventClient.Create(context.TODO(), event, metav1.CreateOptions{})
-
-	return err
+	return r.Create(context.TODO(), event)
 }
 
 // convertPolicyStatusToString to be able to pass the status as event
 func convertPolicyStatusToString(plc *policyv1.ConfigurationPolicy) (results string) {
-	if plc.Status.ComplianceState == "" {
-		return "ComplianceState is still undetermined"
+	if plc.Status.ComplianceState == "" || plc.Status.ComplianceState == policyv1.UnknownCompliancy {
+		return "ComplianceState is still unknown"
 	}
 
 	result := string(plc.Status.ComplianceState)

controllers/configurationpolicy_controller_suite_test.go

@@ -23,7 +23,7 @@ func getSamplePolicy() policyv1.ConfigurationPolicy {
 			Name:      "foo",
 			Namespace: "default",
 		},
-		Spec: policyv1.ConfigurationPolicySpec{
+		Spec: &policyv1.ConfigurationPolicySpec{
 			Severity: "low",
 			NamespaceSelector: policyv1.Target{
 				Include: []policyv1.NonEmptyString{"default", "kube-*"},