From 55d433fd2d90a526f7b8a129475dc01d346e03a3 Mon Sep 17 00:00:00 2001 From: Justin Kulikauskas Date: Mon, 17 Apr 2023 16:11:26 -0400 Subject: [PATCH 1/7] Refactor SendEvent This should make it slightly more re-usable in other controllers. In particular this allows the `instance` to be `nil`, which might be the case if the template was not created. Signed-off-by: Justin Kulikauskas (cherry picked from commit c6dadad4adb66ae52c4826b68026abf7a0ca5927) --- .../gatekeeper_constraint_sync.go | 4 +- controllers/utils/events.go | 47 ++++++++++--------- test/e2e/case12_ordering_test.go | 3 ++ 3 files changed, 32 insertions(+), 22 deletions(-) diff --git a/controllers/gatekeepersync/gatekeeper_constraint_sync.go b/controllers/gatekeepersync/gatekeeper_constraint_sync.go index e94751a2..6d6ee2f6 100644 --- a/controllers/gatekeepersync/gatekeeper_constraint_sync.go +++ b/controllers/gatekeepersync/gatekeeper_constraint_sync.go @@ -396,7 +396,9 @@ func (r *GatekeeperConstraintReconciler) sendComplianceEvent( return nil } - err := r.SendEvent(ctx, constraint, owner, msg, compliance) + reason := utils.EventReason(constraint.GetNamespace(), constraint.GetName()) + + err := r.SendEvent(ctx, constraint, owner, reason, msg, compliance) if err != nil { return err } diff --git a/controllers/utils/events.go b/controllers/utils/events.go index a55c421e..d9a31c32 100644 --- a/controllers/utils/events.go +++ b/controllers/utils/events.go @@ -28,6 +28,7 @@ func (c *ComplianceEventSender) SendEvent( ctx context.Context, instance client.Object, owner metav1.OwnerReference, + reason string, msg string, compliance policyv1.ComplianceState, ) error { @@ -37,15 +38,6 @@ func (c *ComplianceEventSender) SendEvent( } now := time.Now() - var reason string - - if instance.GetNamespace() == "" { - reason = "policy: " + instance.GetName() - } else { - reason = fmt.Sprintf("policy: %s/%s", instance.GetNamespace(), instance.GetName()) - } - - gvk := instance.GetObjectKind().GroupVersionKind() event := &corev1.Event{ ObjectMeta: metav1.ObjectMeta{ @@ -60,31 +52,36 @@ func (c *ComplianceEventSender) SendEvent( UID: owner.UID, APIVersion: owner.APIVersion, }, - Reason: reason, Message: msg, Source: corev1.EventSource{ Component: c.ControllerName, Host: c.InstanceName, }, - FirstTimestamp: metav1.NewTime(now), - LastTimestamp: metav1.NewTime(now), - Count: 1, - Type: "Normal", - EventTime: metav1.NewMicroTime(now), - Action: "ComplianceStateUpdate", - Related: &corev1.ObjectReference{ + FirstTimestamp: metav1.NewTime(now), + LastTimestamp: metav1.NewTime(now), + Count: 1, + EventTime: metav1.NewMicroTime(now), + Action: "ComplianceStateUpdate", + ReportingController: c.ControllerName, + ReportingInstance: c.InstanceName, + } + + if instance != nil { + gvk := instance.GetObjectKind().GroupVersionKind() + + event.Related = &corev1.ObjectReference{ Kind: gvk.Kind, Name: instance.GetName(), Namespace: instance.GetNamespace(), UID: instance.GetUID(), APIVersion: gvk.GroupVersion().String(), - }, - ReportingController: c.ControllerName, - ReportingInstance: c.InstanceName, + } } - if compliance == policyv1.NonCompliant { + if compliance == policyv1.Compliant { + event.Type = "Normal" + } else { event.Type = "Warning" } @@ -93,3 +90,11 @@ func (c *ComplianceEventSender) SendEvent( return err } + +func EventReason(ns, name string) string { + if ns == "" { + return fmt.Sprintf(PolicyClusterScopedFmtStr, name) + } + + return fmt.Sprintf(PolicyFmtStr, ns, name) +} diff --git a/test/e2e/case12_ordering_test.go b/test/e2e/case12_ordering_test.go index 8a4177a2..981c80d1 100644 --- a/test/e2e/case12_ordering_test.go +++ b/test/e2e/case12_ordering_test.go @@ -13,6 +13,8 @@ import ( "k8s.io/apimachinery/pkg/util/uuid" policiesv1 "open-cluster-management.io/governance-policy-propagator/api/v1" "open-cluster-management.io/governance-policy-propagator/test/utils" + + fwutils "open-cluster-management.io/governance-policy-framework-addon/controllers/utils" ) // Helper function to create events @@ -47,6 +49,7 @@ func generateEventOnPolicy(plcName string, cfgPlcName string, msg string, compli Name: managedPlc.GetName(), UID: managedPlc.GetUID(), }, + fwutils.EventReason(clusterNamespace, cfgPlcName), msg, policiesv1.ComplianceState(complianceState), ) From d0e46513b156989172aa0aa281878f902a0db9f4 Mon Sep 17 00:00:00 2001 From: Justin Kulikauskas Date: Mon, 17 Apr 2023 16:14:52 -0400 Subject: [PATCH 2/7] Add clientset to template-sync reconciler This might be slightly more performant, and other things can use this clientset. Signed-off-by: Justin Kulikauskas (cherry picked from commit c01cf0a46a09295fe3bb07d9440ba5d73ac5fc78) --- controllers/templatesync/template_sync.go | 4 ++-- main.go | 1 + 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/controllers/templatesync/template_sync.go b/controllers/templatesync/template_sync.go index 88850a0a..b2102f46 100644 --- a/controllers/templatesync/template_sync.go +++ b/controllers/templatesync/template_sync.go @@ -80,6 +80,7 @@ type PolicyReconciler struct { Config *rest.Config Recorder record.EventRecorder ClusterNamespace string + Clientset *kubernetes.Clientset DisableGkSync bool createdGkConstraint *bool } @@ -135,8 +136,7 @@ func (r *PolicyReconciler) Reconcile(ctx context.Context, request reconcile.Requ var dClient dynamic.Interface if len(instance.Spec.PolicyTemplates) > 0 { - clientset := kubernetes.NewForConfigOrDie(r.Config) - discoveryClient = clientset.Discovery() + discoveryClient = r.Clientset.Discovery() // initialize dynamic client dClient, err = dynamic.NewForConfig(r.Config) diff --git a/main.go b/main.go index 27142d77..1f9e8aed 100644 --- a/main.go +++ b/main.go @@ -458,6 +458,7 @@ func getManager( Config: mgr.GetConfig(), Recorder: mgr.GetEventRecorderFor(templatesync.ControllerName), ClusterNamespace: tool.Options.ClusterNamespace, + Clientset: kubernetes.NewForConfigOrDie(mgr.GetConfig()), DisableGkSync: tool.Options.DisableGkSync, } From 8f815dda54a4ee4cc3f00f6e464abe2359e8a830 Mon Sep 17 00:00:00 2001 From: Justin Kulikauskas Date: Mon, 17 Apr 2023 16:16:44 -0400 Subject: [PATCH 3/7] Synchronously emit template events Previously, the controller-runtime event recorder was used for these events. Other policy controllers have moved away from that, for various reasons. In this case, if a policy went from pending to noncompliant and back to pending, the "old" pending event would be re-used by the event recorder, and only the `lastTimestamp` would be updated. In this case, if a policy controller emitted a compliance event within the same second as the Pending event, the status-sync would see it as a tie, and use the hex-encoded nanoseconds in the event name. But the event name was not updated from the original instance when the policy was pending, so the events would be ordered incorrectly. Most error cases from this synchronous sending can be ignored because they are already error cases that would be requeued. Refs: - https://issues.redhat.com/browse/ACM-4699 Signed-off-by: Justin Kulikauskas (cherry picked from commit f0e2c60e57c02ef18a5e1d4803e134039a35689d) --- controllers/templatesync/template_sync.go | 141 +++++++++++++++++----- main.go | 3 + 2 files changed, 111 insertions(+), 33 deletions(-) diff --git a/controllers/templatesync/template_sync.go b/controllers/templatesync/template_sync.go index b2102f46..1c1f5b0c 100644 --- a/controllers/templatesync/template_sync.go +++ b/controllers/templatesync/template_sync.go @@ -81,6 +81,7 @@ type PolicyReconciler struct { Recorder record.EventRecorder ClusterNamespace string Clientset *kubernetes.Clientset + InstanceName string DisableGkSync bool createdGkConstraint *bool } @@ -262,7 +263,8 @@ func (r *PolicyReconciler) Reconcile(ctx context.Context, request reconcile.Requ resultError = err errMsg := fmt.Sprintf("Failed to decode policy template with err: %s", err) - r.emitTemplateError(instance, tIndex, fmt.Sprintf("[template %v]", tIndex), false, errMsg) + //nolint:errcheck // it will already be requeued for the resultError. + r.emitTemplateError(ctx, instance, tIndex, fmt.Sprintf("[template %v]", tIndex), false, errMsg) reqLogger.Error(resultError, "Failed to decode the policy template", "templateIndex", tIndex) policyUserErrorsCounter.WithLabelValues(instance.Name, "", "format-error").Inc() @@ -310,7 +312,9 @@ func (r *PolicyReconciler) Reconcile(ctx context.Context, request reconcile.Requ resultError = fmt.Errorf("dependency on %s has conflicting compliance states", dep.Name) errMsg := fmt.Sprintf("Failed to decode policy template with err: %s", resultError) - r.emitTemplateError(instance, tIndex, fmt.Sprintf("[template %v]", tIndex), isClusterScoped, errMsg) + //nolint:errcheck // it will already be requeued for the resultError. + r.emitTemplateError(ctx, instance, tIndex, + fmt.Sprintf("[template %v]", tIndex), isClusterScoped, errMsg) reqLogger.Error(resultError, "Failed to decode the policy template", "templateIndex", tIndex) depConflictErr = true @@ -338,7 +342,8 @@ func (r *PolicyReconciler) Reconcile(ctx context.Context, request reconcile.Requ errMsg := fmt.Sprintf("Failed to get name from policy template at index %v", tIndex) resultError = k8serrors.NewBadRequest(errMsg) - r.emitTemplateError(instance, tIndex, fmt.Sprintf("[template %v]", tIndex), isClusterScoped, errMsg) + //nolint:errcheck // it will already be requeued for the resultError. + r.emitTemplateError(ctx, instance, tIndex, fmt.Sprintf("[template %v]", tIndex), isClusterScoped, errMsg) reqLogger.Error(resultError, "Failed to process the policy template", "templateIndex", tIndex) policyUserErrorsCounter.WithLabelValues(instance.Name, "", "format-error").Inc() @@ -367,7 +372,8 @@ func (r *PolicyReconciler) Reconcile(ctx context.Context, request reconcile.Requ errMsg += fmt.Sprintf(": %s", err) - r.emitTemplateError(instance, tIndex, tName, isClusterScoped, errMsg) + //nolint:errcheck // it will already be requeued for the resultError. + r.emitTemplateError(ctx, instance, tIndex, tName, isClusterScoped, errMsg) tLogger.Error(err, "Could not find an API mapping for the object definition", "group", gvk.Group, "version", gvk.Version, @@ -415,7 +421,8 @@ func (r *PolicyReconciler) Reconcile(ctx context.Context, request reconcile.Requ resultError = err - r.emitTemplateError(instance, tIndex, tName, isClusterScoped, errMsg) + //nolint:errcheck // it will already be requeued for the resultError. + r.emitTemplateError(ctx, instance, tIndex, tName, isClusterScoped, errMsg) tLogger.Error(err, "Unsupported policy-template kind found in object definition", "group", gvk.Group, "version", gvk.Version, @@ -435,7 +442,8 @@ func (r *PolicyReconciler) Reconcile(ctx context.Context, request reconcile.Requ errMsg := fmt.Sprintf("Templates are not supported for kind : %s", gvk.Kind) resultError = k8serrors.NewBadRequest(errMsg) - r.emitTemplateError(instance, tIndex, tName, isClusterScoped, errMsg) + //nolint:errcheck // it will already be requeued for the resultError. + r.emitTemplateError(ctx, instance, tIndex, tName, isClusterScoped, errMsg) tLogger.Error(resultError, "Failed to process the policy template") policyUserErrorsCounter.WithLabelValues(instance.Name, tName, "format-error").Inc() @@ -464,7 +472,8 @@ func (r *PolicyReconciler) Reconcile(ctx context.Context, request reconcile.Requ resultError = err errMsg := fmt.Sprintf("Failed to unmarshal the policy template: %s", err) - r.emitTemplateError(instance, tIndex, tName, isClusterScoped, errMsg) + //nolint:errcheck // it will already be requeued for the resultError. + r.emitTemplateError(ctx, instance, tIndex, tName, isClusterScoped, errMsg) tLogger.Error(resultError, "Failed to unmarshal the policy template") policySystemErrorsCounter.WithLabelValues(instance.Name, tName, "unmarshal-error").Inc() @@ -486,7 +495,14 @@ func (r *PolicyReconciler) Reconcile(ctx context.Context, request reconcile.Requ if err != nil { if len(dependencyFailures) > 0 { // template must be pending, do not create it - r.emitTemplatePending(instance, tIndex, tName, isClusterScoped, generatePendingMsg(dependencyFailures)) + emitErr := r.emitTemplatePending(ctx, instance, tIndex, tName, isClusterScoped, + generatePendingMsg(dependencyFailures)) + if emitErr != nil { + resultError = emitErr + + continue + } + tLogger.Info("Dependencies were not satisfied for the policy template", "namespace", instance.GetNamespace(), "kind", gvk.Kind, @@ -530,7 +546,8 @@ func (r *PolicyReconciler) Reconcile(ctx context.Context, request reconcile.Requ resultError = err errMsg := fmt.Sprintf("Failed to create policy template: %s", err) - r.emitTemplateError(instance, tIndex, tName, isClusterScoped, errMsg) + //nolint:errcheck // it will already be requeued for the resultError. + r.emitTemplateError(ctx, instance, tIndex, tName, isClusterScoped, errMsg) tLogger.Error(resultError, "Failed to create policy template") policySystemErrorsCounter.WithLabelValues(instance.Name, tName, "create-error").Inc() @@ -555,7 +572,11 @@ func (r *PolicyReconciler) Reconcile(ctx context.Context, request reconcile.Requ if isGkConstraintTemplate { tLogger.Info("Emitting status event for " + gvk.Kind) msg := fmt.Sprintf("%s %s was created successfully", gvk.Kind, tName) - r.emitTemplateSuccess(instance, tIndex, tName, isClusterScoped, msg) + + emitErr := r.emitTemplateSuccess(ctx, instance, tIndex, tName, isClusterScoped, msg) + if emitErr != nil { + resultError = emitErr + } } } @@ -574,7 +595,8 @@ func (r *PolicyReconciler) Reconcile(ctx context.Context, request reconcile.Requ resultError = err errMsg := fmt.Sprintf("Failed to get the object in the policy template: %s", err) - r.emitTemplateError(instance, tIndex, tName, isClusterScoped, errMsg) + //nolint:errcheck // it will already be requeued for the resultError. + r.emitTemplateError(ctx, instance, tIndex, tName, isClusterScoped, errMsg) tLogger.Error(err, "Failed to get the object in the policy template", "namespace", instance.GetNamespace(), "kind", gvk.Kind, @@ -588,12 +610,17 @@ func (r *PolicyReconciler) Reconcile(ctx context.Context, request reconcile.Requ if len(dependencyFailures) > 0 { // template must be pending, need to delete it and error - r.emitTemplatePending(instance, tIndex, tName, isClusterScoped, generatePendingMsg(dependencyFailures)) tLogger.Info("Dependencies were not satisfied for the policy template", "namespace", instance.GetNamespace(), "kind", gvk.Kind, ) + emitErr := r.emitTemplatePending(ctx, instance, tIndex, tName, + isClusterScoped, generatePendingMsg(dependencyFailures)) + if emitErr != nil { + resultError = err + } + err = res.Delete(ctx, tName, metav1.DeleteOptions{}) if err != nil { tLogger.Error(err, "Failed to delete a template that entered pending state", @@ -665,7 +692,8 @@ func (r *PolicyReconciler) Reconcile(ctx context.Context, request reconcile.Requ resultError = k8serrors.NewBadRequest(errMsg) - r.emitTemplateError(instance, tIndex, tName, isClusterScoped, errMsg) + //nolint:errcheck // it will already be requeued for the resultError. + r.emitTemplateError(ctx, instance, tIndex, tName, isClusterScoped, errMsg) tLogger.Error(resultError, "Failed to create the policy template") policyUserErrorsCounter.WithLabelValues(instance.Name, tName, "format-error").Inc() @@ -711,7 +739,8 @@ func (r *PolicyReconciler) Reconcile(ctx context.Context, request reconcile.Requ resultError = err errMsg := fmt.Sprintf("Failed to update policy template %s: %s", tName, err) - r.emitTemplateError(instance, tIndex, tName, isClusterScoped, errMsg) + //nolint:errcheck // it will already be requeued for the resultError. + r.emitTemplateError(ctx, instance, tIndex, tName, isClusterScoped, errMsg) tLogger.Error(err, "Failed to update the policy template") policySystemErrorsCounter.WithLabelValues(instance.Name, tName, "patch-error").Inc() @@ -734,7 +763,11 @@ func (r *PolicyReconciler) Reconcile(ctx context.Context, request reconcile.Requ if isGkConstraintTemplate { tLogger.Info("Emitting status event for " + gvk.Kind) msg := fmt.Sprintf("%s %s was updated successfully", gvk.Kind, tName) - r.emitTemplateSuccess(instance, tIndex, tName, isClusterScoped, msg) + + emitErr := r.emitTemplateSuccess(ctx, instance, tIndex, tName, isClusterScoped, msg) + if emitErr != nil { + resultError = emitErr + } } } @@ -1186,26 +1219,39 @@ func overrideRemediationAction(instance *policiesv1.Policy, tObjectUnstructured // policy framework. If the policy's status already reflects the current message, then no actions // are taken. func (r *PolicyReconciler) emitTemplateSuccess( - pol *policiesv1.Policy, tIndex int, tName string, clusterScoped bool, msg string, -) { - r.emitTemplateEvent(pol, tIndex, tName, clusterScoped, "Normal", "Compliant; ", msg) + ctx context.Context, pol *policiesv1.Policy, tIndex int, tName string, clusterScoped bool, msg string, +) error { + err := r.emitTemplateEvent(ctx, pol, tIndex, tName, clusterScoped, "Normal", "Compliant; ", msg) + if err != nil { + tlog := log.WithValues("Policy.Namespace", pol.Namespace, "Policy.Name", pol.Name, "template", tName) + tlog.Error(err, "Failed to emit template success event") + } + + return err } // emitTemplateError performs actions that ensure correct reporting of template errors in the // policy framework. If the policy's status already reflects the current error, then no actions // are taken. func (r *PolicyReconciler) emitTemplateError( - pol *policiesv1.Policy, tIndex int, tName string, clusterScoped bool, errMsg string, -) { - r.emitTemplateEvent(pol, tIndex, tName, clusterScoped, "Warning", "NonCompliant; template-error; ", errMsg) + ctx context.Context, pol *policiesv1.Policy, tIndex int, tName string, clusterScoped bool, errMsg string, +) error { + err := r.emitTemplateEvent(ctx, pol, tIndex, tName, clusterScoped, + "Warning", "NonCompliant; template-error; ", errMsg) + if err != nil { + tlog := log.WithValues("Policy.Namespace", pol.Namespace, "Policy.Name", pol.Name, "template", tName) + tlog.Error(err, "Failed to emit template error event") + } + + return err } // emitTemplatePending performs actions that ensure correct reporting of pending dependencies in the // policy framework. If the policy's status already reflects the current status, then no actions // are taken. func (r *PolicyReconciler) emitTemplatePending( - pol *policiesv1.Policy, tIndex int, tName string, clusterScoped bool, msg string, -) { + ctx context.Context, pol *policiesv1.Policy, tIndex int, tName string, clusterScoped bool, msg string, +) error { msgMeta := "Pending; " eventType := "Warning" @@ -1215,33 +1261,62 @@ func (r *PolicyReconciler) emitTemplatePending( eventType = "Normal" } - r.emitTemplateEvent(pol, tIndex, tName, clusterScoped, eventType, msgMeta, msg) + err := r.emitTemplateEvent(ctx, pol, tIndex, tName, clusterScoped, eventType, msgMeta, msg) + if err != nil { + tlog := log.WithValues("Policy.Namespace", pol.Namespace, "Policy.Name", pol.Name, "template", tName) + tlog.Error(err, "Failed to emit template pending event") + } + + return err } // emitTemplateEvent performs actions that ensure correct reporting of template sync events. If the // policy's status already reflects the current status, then no actions are taken. The msgMeta and // msg are concatenated without spaces, so any spacing should be included inside the msgMeta string. func (r *PolicyReconciler) emitTemplateEvent( - pol *policiesv1.Policy, tIndex int, tName string, clusterScoped bool, + ctx context.Context, pol *policiesv1.Policy, tIndex int, tName string, clusterScoped bool, eventType string, msgMeta string, msg string, -) { +) error { // check if the error is already present in the policy status - if so, return early if strings.Contains(getLatestStatusMessage(pol, tIndex), msgMeta+msg) { - return + return nil } - // emit the non-compliance event + // emit an informational event + r.Recorder.Event(pol, eventType, "PolicyTemplateSync", msg) + + // emit the compliance event var policyComplianceReason string if clusterScoped { - policyComplianceReason = fmt.Sprintf(utils.PolicyClusterScopedFmtStr, tName) + policyComplianceReason = utils.EventReason("", tName) } else { - policyComplianceReason = fmt.Sprintf(utils.PolicyFmtStr, pol.GetNamespace(), tName) + policyComplianceReason = utils.EventReason(pol.GetNamespace(), tName) } - r.Recorder.Event(pol, eventType, policyComplianceReason, msgMeta+msg) + sender := utils.ComplianceEventSender{ + ClusterNamespace: pol.Namespace, + InstanceName: r.InstanceName, + ClientSet: r.Clientset, + ControllerName: ControllerName, + } - // emit an informational event - r.Recorder.Event(pol, eventType, "PolicyTemplateSync", msg) + ownerref := metav1.OwnerReference{ + APIVersion: pol.APIVersion, + Kind: pol.Kind, + Name: pol.Name, + UID: pol.UID, + } + + var compState policiesv1.ComplianceState + if strings.HasPrefix(msgMeta, "Compliant") { + compState = policiesv1.Compliant + } else if strings.HasPrefix(msgMeta, "NonCompliant") { + compState = policiesv1.NonCompliant + } else if strings.HasPrefix(msgMeta, "Pending") { + compState = policiesv1.Pending + } + + return sender.SendEvent(ctx, nil, ownerref, policyComplianceReason, msgMeta+msg, compState) } // handleSyncSuccess performs common actions that should be run whenever a template is in sync, diff --git a/main.go b/main.go index 1f9e8aed..cfedc4e7 100644 --- a/main.go +++ b/main.go @@ -451,6 +451,8 @@ func getManager( os.Exit(1) } + instanceName, _ := os.Hostname() // on an error, instanceName will be empty, which is ok + templateReconciler := &templatesync.PolicyReconciler{ Client: mgr.GetClient(), DynamicWatcher: watcher, @@ -459,6 +461,7 @@ func getManager( Recorder: mgr.GetEventRecorderFor(templatesync.ControllerName), ClusterNamespace: tool.Options.ClusterNamespace, Clientset: kubernetes.NewForConfigOrDie(mgr.GetConfig()), + InstanceName: instanceName, DisableGkSync: tool.Options.DisableGkSync, } From 2666d6d97d3d21e3a05389dc4023eadf28d7c416 Mon Sep 17 00:00:00 2001 From: Justin Kulikauskas Date: Tue, 18 Apr 2023 14:14:48 -0400 Subject: [PATCH 4/7] Fix gosec:G104 Signed-off-by: Justin Kulikauskas (cherry picked from commit d6cb733c0cfb66ea1b1d8b03b073c78096090151) --- controllers/templatesync/template_sync.go | 45 ++++++++++++----------- 1 file changed, 23 insertions(+), 22 deletions(-) diff --git a/controllers/templatesync/template_sync.go b/controllers/templatesync/template_sync.go index 1c1f5b0c..bdb1c25b 100644 --- a/controllers/templatesync/template_sync.go +++ b/controllers/templatesync/template_sync.go @@ -263,8 +263,8 @@ func (r *PolicyReconciler) Reconcile(ctx context.Context, request reconcile.Requ resultError = err errMsg := fmt.Sprintf("Failed to decode policy template with err: %s", err) - //nolint:errcheck // it will already be requeued for the resultError. - r.emitTemplateError(ctx, instance, tIndex, fmt.Sprintf("[template %v]", tIndex), false, errMsg) + _ = r.emitTemplateError(ctx, instance, tIndex, fmt.Sprintf("[template %v]", tIndex), false, errMsg) + reqLogger.Error(resultError, "Failed to decode the policy template", "templateIndex", tIndex) policyUserErrorsCounter.WithLabelValues(instance.Name, "", "format-error").Inc() @@ -312,9 +312,9 @@ func (r *PolicyReconciler) Reconcile(ctx context.Context, request reconcile.Requ resultError = fmt.Errorf("dependency on %s has conflicting compliance states", dep.Name) errMsg := fmt.Sprintf("Failed to decode policy template with err: %s", resultError) - //nolint:errcheck // it will already be requeued for the resultError. - r.emitTemplateError(ctx, instance, tIndex, + _ = r.emitTemplateError(ctx, instance, tIndex, fmt.Sprintf("[template %v]", tIndex), isClusterScoped, errMsg) + reqLogger.Error(resultError, "Failed to decode the policy template", "templateIndex", tIndex) depConflictErr = true @@ -342,8 +342,9 @@ func (r *PolicyReconciler) Reconcile(ctx context.Context, request reconcile.Requ errMsg := fmt.Sprintf("Failed to get name from policy template at index %v", tIndex) resultError = k8serrors.NewBadRequest(errMsg) - //nolint:errcheck // it will already be requeued for the resultError. - r.emitTemplateError(ctx, instance, tIndex, fmt.Sprintf("[template %v]", tIndex), isClusterScoped, errMsg) + _ = r.emitTemplateError(ctx, instance, tIndex, + fmt.Sprintf("[template %v]", tIndex), isClusterScoped, errMsg) + reqLogger.Error(resultError, "Failed to process the policy template", "templateIndex", tIndex) policyUserErrorsCounter.WithLabelValues(instance.Name, "", "format-error").Inc() @@ -372,8 +373,8 @@ func (r *PolicyReconciler) Reconcile(ctx context.Context, request reconcile.Requ errMsg += fmt.Sprintf(": %s", err) - //nolint:errcheck // it will already be requeued for the resultError. - r.emitTemplateError(ctx, instance, tIndex, tName, isClusterScoped, errMsg) + _ = r.emitTemplateError(ctx, instance, tIndex, tName, isClusterScoped, errMsg) + tLogger.Error(err, "Could not find an API mapping for the object definition", "group", gvk.Group, "version", gvk.Version, @@ -421,8 +422,8 @@ func (r *PolicyReconciler) Reconcile(ctx context.Context, request reconcile.Requ resultError = err - //nolint:errcheck // it will already be requeued for the resultError. - r.emitTemplateError(ctx, instance, tIndex, tName, isClusterScoped, errMsg) + _ = r.emitTemplateError(ctx, instance, tIndex, tName, isClusterScoped, errMsg) + tLogger.Error(err, "Unsupported policy-template kind found in object definition", "group", gvk.Group, "version", gvk.Version, @@ -442,8 +443,8 @@ func (r *PolicyReconciler) Reconcile(ctx context.Context, request reconcile.Requ errMsg := fmt.Sprintf("Templates are not supported for kind : %s", gvk.Kind) resultError = k8serrors.NewBadRequest(errMsg) - //nolint:errcheck // it will already be requeued for the resultError. - r.emitTemplateError(ctx, instance, tIndex, tName, isClusterScoped, errMsg) + _ = r.emitTemplateError(ctx, instance, tIndex, tName, isClusterScoped, errMsg) + tLogger.Error(resultError, "Failed to process the policy template") policyUserErrorsCounter.WithLabelValues(instance.Name, tName, "format-error").Inc() @@ -472,8 +473,8 @@ func (r *PolicyReconciler) Reconcile(ctx context.Context, request reconcile.Requ resultError = err errMsg := fmt.Sprintf("Failed to unmarshal the policy template: %s", err) - //nolint:errcheck // it will already be requeued for the resultError. - r.emitTemplateError(ctx, instance, tIndex, tName, isClusterScoped, errMsg) + _ = r.emitTemplateError(ctx, instance, tIndex, tName, isClusterScoped, errMsg) + tLogger.Error(resultError, "Failed to unmarshal the policy template") policySystemErrorsCounter.WithLabelValues(instance.Name, tName, "unmarshal-error").Inc() @@ -546,8 +547,8 @@ func (r *PolicyReconciler) Reconcile(ctx context.Context, request reconcile.Requ resultError = err errMsg := fmt.Sprintf("Failed to create policy template: %s", err) - //nolint:errcheck // it will already be requeued for the resultError. - r.emitTemplateError(ctx, instance, tIndex, tName, isClusterScoped, errMsg) + _ = r.emitTemplateError(ctx, instance, tIndex, tName, isClusterScoped, errMsg) + tLogger.Error(resultError, "Failed to create policy template") policySystemErrorsCounter.WithLabelValues(instance.Name, tName, "create-error").Inc() @@ -595,8 +596,8 @@ func (r *PolicyReconciler) Reconcile(ctx context.Context, request reconcile.Requ resultError = err errMsg := fmt.Sprintf("Failed to get the object in the policy template: %s", err) - //nolint:errcheck // it will already be requeued for the resultError. - r.emitTemplateError(ctx, instance, tIndex, tName, isClusterScoped, errMsg) + _ = r.emitTemplateError(ctx, instance, tIndex, tName, isClusterScoped, errMsg) + tLogger.Error(err, "Failed to get the object in the policy template", "namespace", instance.GetNamespace(), "kind", gvk.Kind, @@ -692,8 +693,8 @@ func (r *PolicyReconciler) Reconcile(ctx context.Context, request reconcile.Requ resultError = k8serrors.NewBadRequest(errMsg) - //nolint:errcheck // it will already be requeued for the resultError. - r.emitTemplateError(ctx, instance, tIndex, tName, isClusterScoped, errMsg) + _ = r.emitTemplateError(ctx, instance, tIndex, tName, isClusterScoped, errMsg) + tLogger.Error(resultError, "Failed to create the policy template") policyUserErrorsCounter.WithLabelValues(instance.Name, tName, "format-error").Inc() @@ -739,8 +740,8 @@ func (r *PolicyReconciler) Reconcile(ctx context.Context, request reconcile.Requ resultError = err errMsg := fmt.Sprintf("Failed to update policy template %s: %s", tName, err) - //nolint:errcheck // it will already be requeued for the resultError. - r.emitTemplateError(ctx, instance, tIndex, tName, isClusterScoped, errMsg) + _ = r.emitTemplateError(ctx, instance, tIndex, tName, isClusterScoped, errMsg) + tLogger.Error(err, "Failed to update the policy template") policySystemErrorsCounter.WithLabelValues(instance.Name, tName, "patch-error").Inc() From b31170f8f90731565ee632a623bfc2d4c0c13165 Mon Sep 17 00:00:00 2001 From: Justin Kulikauskas Date: Thu, 20 Apr 2023 13:27:01 -0400 Subject: [PATCH 5/7] Check gosec results in CI The KinD tests action will now run the gosec-scan, and that target will fail if any vulnerabilities are found. The target was also configured to ignore the test code. Signed-off-by: Justin Kulikauskas (cherry picked from commit 8c251da2352fd5ece0589273d641905a35705214) --- .github/workflows/kind.yml | 3 ++- Makefile | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/.github/workflows/kind.yml b/.github/workflows/kind.yml index ee0e33f5..441dfb5d 100644 --- a/.github/workflows/kind.yml +++ b/.github/workflows/kind.yml @@ -47,11 +47,12 @@ jobs: run: | go mod verify - - name: Verify format + - name: Some quality checks run: | make fmt git diff --exit-code make lint + make gosec-scan - name: Verify deploy/operator.yaml run: | diff --git a/Makefile b/Makefile index d9a02fac..4ff79a8c 100644 --- a/Makefile +++ b/Makefile @@ -177,7 +177,7 @@ gosec: .PHONY: gosec-scan gosec-scan: gosec - $(GOSEC) -fmt sonarqube -out gosec.json -no-fail -exclude-dir=.go ./... + $(GOSEC) -fmt sonarqube -out gosec.json -stdout -exclude-dir=.go -exclude-dir=test ./... ############################################################ # build section From e88d76fb5cae2c00e74a2fdd09987388a84ea14a Mon Sep 17 00:00:00 2001 From: Justin Kulikauskas Date: Thu, 20 Apr 2023 13:31:46 -0400 Subject: [PATCH 6/7] Add more information to the e2e-debug Information about the gatekeeper pods might help if those tests fail. Signed-off-by: Justin Kulikauskas (cherry picked from commit 0034b03fe8a36868903f07484221b1cea1b5dd6c) --- Makefile | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/Makefile b/Makefile index 4ff79a8c..433c8264 100644 --- a/Makefile +++ b/Makefile @@ -337,6 +337,15 @@ e2e-stop-instrumented: e2e-debug: @echo local controller log: -cat build/_output/controller.log + @echo pods on hub cluster + -kubectl get pods -A --kubeconfig=$(HUB_CONFIG) + -kubectl get pods -A -o yaml --kubeconfig=$(HUB_CONFIG) + @echo pods on managed cluster + -kubectl get pods -A --kubeconfig=$(MANAGED_CONFIG) + -kubectl get pods -A -o yaml --kubeconfig=$(MANAGED_CONFIG) + @echo gatekeeper logs on managed cluster + -kubectl logs -n gatekeeper-system -l control-plane=audit-controller --prefix=true --since=5m --kubeconfig=$(MANAGED_CONFIG) + -kubectl logs -n gatekeeper-system -l control-plane=controller-manager --prefix=true --since=5m --kubeconfig=$(MANAGED_CONFIG) @echo remote controller log: -kubectl logs $$(kubectl get pods -n $(KIND_NAMESPACE) -o name --kubeconfig=$(MANAGED_CONFIG) | grep $(IMG)) -n $(KIND_NAMESPACE) --kubeconfig=$(MANAGED_CONFIG) From 8a87b21221c53101a6cebff05279d5333257521b Mon Sep 17 00:00:00 2001 From: Justin Kulikauskas Date: Thu, 20 Apr 2023 16:15:35 -0400 Subject: [PATCH 7/7] Fix test assertion for duplicate events The test is meant to ensure that the gatekeeper-sync is not emitting the same event multiple times in a row. But the assertion was failing sometimes because of duplicate events from template-errors. Those will sometimes occur during normal (correct) operation of the template-sync. Signed-off-by: Justin Kulikauskas (cherry picked from commit 906dcffd4704c872e1ef57a07b34e0403f875191) --- test/e2e/case17_gatekeeper_sync_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/e2e/case17_gatekeeper_sync_test.go b/test/e2e/case17_gatekeeper_sync_test.go index fdb52d0a..cefe9328 100644 --- a/test/e2e/case17_gatekeeper_sync_test.go +++ b/test/e2e/case17_gatekeeper_sync_test.go @@ -297,9 +297,9 @@ var _ = Describe("Test Gatekeeper ConstraintTemplate and constraint sync", Order fmt.Sprintf("Got %s but expected one of %v", history[0].Message, validMsgs), ) - // Verify that there are no duplicate status messages. + // Verify that there are no duplicate gatekeeper status messages. for i, historyEvent := range managedPolicy.Status.Details[1].History { - if i == 0 || strings.HasPrefix(historyEvent.Message, "NonCompliant; template-error;") { + if i == 0 || strings.Contains(historyEvent.Message, "NonCompliant; template-error;") { continue }