Add support for ignoring reconciliation of children (#321)

* Add support for ignoring reconciliation of children * check if should reconcile before reconciling for all resources * add tests for ignore label * Fix logic bug * use more compact if check for proceeding reconcile * add labelchangepredicate to encompassing eventfilter for application controller * use shouldReconcile during delete steps for resources which may be more than one --------- Co-authored-by: anderssonw <[email protected]> Co-authored-by: Even Holthe <[email protected]>
kartverket · Sep 25, 2023 · 7db0608 · 7db0608
1 parent 60443b1
commit 7db0608
Show file tree

Hide file tree

Showing 27 changed files with 243 additions and 13 deletions.
diff --git a/controllers/application/authorization_policy.go b/controllers/application/authorization_policy.go
@@ -22,6 +22,12 @@ func (r *ApplicationReconciler) reconcileAuthorizationPolicy(ctx context.Context
 	}
 	defaultDenyAuthPolicy := getDefaultDenyPolicy(application, defaultDenyPaths)
 
+	shouldReconcile, err := r.ShouldReconcile(ctx, &defaultDenyAuthPolicy)
+	if err != nil || !shouldReconcile {
+		r.SetControllerFinishedOutcome(ctx, application, controllerName, err)
+		return reconcile.Result{}, err
+	}
+
 	if application.Spec.AuthorizationSettings != nil {
 		if application.Spec.AuthorizationSettings.AllowAll == true {
 			err := r.GetClient().Delete(ctx, &defaultDenyAuthPolicy)
@@ -36,7 +42,7 @@ func (r *ApplicationReconciler) reconcileAuthorizationPolicy(ctx context.Context
 		}
 	}
 
-	_, err := ctrlutil.CreateOrPatch(ctx, r.GetClient(), &defaultDenyAuthPolicy, func() error {
+	_, err = ctrlutil.CreateOrPatch(ctx, r.GetClient(), &defaultDenyAuthPolicy, func() error {
 		err := ctrlutil.SetControllerReference(application, &defaultDenyAuthPolicy, r.GetScheme())
 		if err != nil {
 			r.SetControllerError(ctx, application, controllerName, err)

diff --git a/controllers/application/certificate.go b/controllers/application/certificate.go
@@ -41,6 +41,7 @@ func (r *ApplicationReconciler) SkiperatorOwnedCertRequests(_ context.Context, o
 }
 
 func (r *ApplicationReconciler) reconcileCertificate(ctx context.Context, application *skiperatorv1alpha1.Application) (reconcile.Result, error) {
+
 	controllerName := "Certificate"
 	r.SetControllerProgressing(ctx, application, controllerName)
 
@@ -49,7 +50,18 @@ func (r *ApplicationReconciler) reconcileCertificate(ctx context.Context, applic
 		certificateName := fmt.Sprintf("%s-%s-ingress-%x", application.Namespace, application.Name, util.GenerateHashFromName(hostname))
 
 		certificate := certmanagerv1.Certificate{ObjectMeta: metav1.ObjectMeta{Namespace: "istio-gateways", Name: certificateName}}
-		_, err := ctrlutil.CreateOrPatch(ctx, r.GetClient(), &certificate, func() error {
+
+		shouldReconcile, err := r.ShouldReconcile(ctx, &certificate)
+		if err != nil {
+			r.SetControllerFinishedOutcome(ctx, application, controllerName, err)
+			return reconcile.Result{}, err
+		}
+
+		if !shouldReconcile {
+			continue
+		}
+
+		_, err = ctrlutil.CreateOrPatch(ctx, r.GetClient(), &certificate, func() error {
 			r.SetLabelsFromApplication(&certificate, *application)
 
 			certificate.Spec = certmanagerv1.CertificateSpec{
@@ -79,8 +91,19 @@ func (r *ApplicationReconciler) reconcileCertificate(ctx context.Context, applic
 		return reconcile.Result{}, err
 	}
 
+	// Could we get in trouble with shouldReconcile here? I'm not entirely sure
 	for _, certificate := range certificates.Items {
 
+		shouldReconcile, err := r.ShouldReconcile(ctx, &certificate)
+		if err != nil {
+			r.SetControllerFinishedOutcome(ctx, application, controllerName, err)
+			return reconcile.Result{}, err
+		}
+
+		if !shouldReconcile {
+			continue
+		}
+
 		certificateInApplicationSpecIndex := slices.IndexFunc(application.Spec.Ingresses, func(hostname string) bool {
 			certificateName := fmt.Sprintf("%s-%s-ingress-%x", application.Namespace, application.Name, util.GenerateHashFromName(hostname))
 			return certificate.Name == certificateName

diff --git a/controllers/application/configmap.go b/controllers/application/configmap.go
@@ -84,6 +84,12 @@ func (r *ApplicationReconciler) setupGCPAuthConfigMap(ctx context.Context, gcpId
 		return err
 	}
 
+	shouldReconcile, err := r.ShouldReconcile(ctx, &gcpAuthConfigMap)
+	if err != nil || !shouldReconcile {
+		r.SetControllerFinishedOutcome(ctx, application, controllerName, err)
+		return err
+	}
+
 	_, err = ctrlutil.CreateOrPatch(ctx, r.GetClient(), &gcpAuthConfigMap, func() error {
 		// Set application as owner of the configmap
 		err := ctrlutil.SetControllerReference(application, &gcpAuthConfigMap, r.GetScheme())

diff --git a/controllers/application/controller.go b/controllers/application/controller.go
@@ -3,10 +3,11 @@ package applicationcontroller
 import (
 	"context"
 	"fmt"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"regexp"
 	"strings"
 
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+
 	policyv1 "k8s.io/api/policy/v1"
 
 	certmanagerv1 "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1"
@@ -70,7 +71,7 @@ func (r *ApplicationReconciler) SetupWithManager(mgr ctrl.Manager) error {
 		Owns(&securityv1beta1.AuthorizationPolicy{}).
 		Owns(&pov1.ServiceMonitor{}).
 		Watches(&certmanagerv1.Certificate{}, handler.EnqueueRequestsFromMapFunc(r.SkiperatorOwnedCertRequests)).
-		WithEventFilter(predicate.GenerationChangedPredicate{}).
+		WithEventFilter(predicate.Or(predicate.GenerationChangedPredicate{}, predicate.LabelChangedPredicate{})).
 		Complete(r)
 }
 

diff --git a/controllers/application/deployment.go b/controllers/application/deployment.go
@@ -188,10 +188,21 @@ func (r *ApplicationReconciler) reconcileDeployment(ctx context.Context, applica
 	controllerName := "Deployment"
 	r.SetControllerProgressing(ctx, application, controllerName)
 
-	deployment := appsv1.Deployment{}
+	deployment := appsv1.Deployment{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      application.Name,
+			Namespace: application.Namespace,
+		},
+	}
 	deploymentDefinition, err := r.defineDeployment(ctx, application)
 
-	err = r.GetClient().Get(ctx, types.NamespacedName{Name: application.Name, Namespace: application.Namespace}, &deployment)
+	shouldReconcile, err := r.ShouldReconcile(ctx, &deployment)
+	if err != nil || !shouldReconcile {
+		r.SetControllerFinishedOutcome(ctx, application, controllerName, err)
+		return reconcile.Result{}, err
+	}
+
+	err = r.GetClient().Get(ctx, client.ObjectKeyFromObject(&deployment), &deployment)
 	if err != nil {
 		if errors.IsNotFound(err) {
 			r.EmitNormalEvent(application, "NotFound", fmt.Sprintf("deployment resource for application %s not found, creating deployment", application.Name))

diff --git a/controllers/application/egress_service_entry.go b/controllers/application/egress_service_entry.go
@@ -28,7 +28,18 @@ func (r *ApplicationReconciler) reconcileEgressServiceEntry(ctx context.Context,
 		// CreateOrPatch gets the object (from cache) before the mutating function is run, masquerading actual changes
 		// Restoring the Spec from a copy within the mutating func fixes this
 		desiredServiceEntry := serviceEntry.DeepCopy()
-		_, err := ctrlutil.CreateOrPatch(ctx, r.GetClient(), &serviceEntry, func() error {
+
+		shouldReconcile, err := r.ShouldReconcile(ctx, &serviceEntry)
+		if err != nil {
+			r.SetControllerFinishedOutcome(ctx, application, controllerName, err)
+			return reconcile.Result{}, err
+		}
+
+		if !shouldReconcile {
+			continue
+		}
+
+		_, err = ctrlutil.CreateOrPatch(ctx, r.GetClient(), &serviceEntry, func() error {
 			serviceEntry.Spec = desiredServiceEntry.Spec
 			// Set application as owner of the service entry
 			err := ctrlutil.SetControllerReference(application, &serviceEntry, r.GetScheme())
@@ -57,6 +68,16 @@ func (r *ApplicationReconciler) reconcileEgressServiceEntry(ctx context.Context,
 
 	serviceEntriesToDelete := istio.GetServiceEntriesToDelete(serviceEntriesInNamespace.Items, application.Name, serviceEntries)
 	for _, serviceEntry := range serviceEntriesToDelete {
+		shouldReconcile, err := r.ShouldReconcile(ctx, &serviceEntry)
+		if err != nil {
+			r.SetControllerFinishedOutcome(ctx, application, controllerName, err)
+			return reconcile.Result{}, err
+		}
+
+		if !shouldReconcile {
+			continue
+		}
+
 		err = r.GetClient().Delete(ctx, &serviceEntry)
 		err = client.IgnoreNotFound(err)
 		if err != nil {

diff --git a/controllers/application/horizontal_pod_autoscaler.go b/controllers/application/horizontal_pod_autoscaler.go
@@ -16,6 +16,12 @@ func (r *ApplicationReconciler) reconcileHorizontalPodAutoscaler(ctx context.Con
 	r.SetControllerProgressing(ctx, application, controllerName)
 
 	horizontalPodAutoscaler := autoscalingv2.HorizontalPodAutoscaler{ObjectMeta: metav1.ObjectMeta{Namespace: application.Namespace, Name: application.Name}}
+	shouldReconcile, err := r.ShouldReconcile(ctx, &horizontalPodAutoscaler)
+	if err != nil || !shouldReconcile {
+		r.SetControllerFinishedOutcome(ctx, application, controllerName, err)
+		return reconcile.Result{}, err
+	}
+
 	if shouldScaleToZero(application.Spec.Replicas) || !skiperatorv1alpha1.IsHPAEnabled(application.Spec.Replicas) {
 		err := r.GetClient().Delete(ctx, &horizontalPodAutoscaler)
 		err = client.IgnoreNotFound(err)
@@ -27,7 +33,7 @@ func (r *ApplicationReconciler) reconcileHorizontalPodAutoscaler(ctx context.Con
 		return reconcile.Result{}, nil
 	}
 
-	_, err := ctrlutil.CreateOrPatch(ctx, r.GetClient(), &horizontalPodAutoscaler, func() error {
+	_, err = ctrlutil.CreateOrPatch(ctx, r.GetClient(), &horizontalPodAutoscaler, func() error {
 		// Set application as owner of the horizontal pod autoscaler
 		err := ctrlutil.SetControllerReference(application, &horizontalPodAutoscaler, r.GetScheme())
 		if err != nil {

diff --git a/controllers/application/ingress_gateway.go b/controllers/application/ingress_gateway.go
@@ -26,7 +26,17 @@ func (r *ApplicationReconciler) reconcileIngressGateway(ctx context.Context, app
 		name := fmt.Sprintf("%s-ingress-%x", application.Name, util.GenerateHashFromName(hostname))
 
 		gateway := networkingv1beta1.Gateway{ObjectMeta: metav1.ObjectMeta{Namespace: application.Namespace, Name: name}}
-		_, err := ctrlutil.CreateOrPatch(ctx, r.GetClient(), &gateway, func() error {
+		shouldReconcile, err := r.ShouldReconcile(ctx, &gateway)
+		if err != nil {
+			r.SetControllerFinishedOutcome(ctx, application, controllerName, err)
+			return reconcile.Result{}, err
+		}
+
+		if !shouldReconcile {
+			continue
+		}
+
+		_, err = ctrlutil.CreateOrPatch(ctx, r.GetClient(), &gateway, func() error {
 			// Set application as owner of the gateway
 			err := ctrlutil.SetControllerReference(application, &gateway, r.GetScheme())
 			if err != nil {
@@ -92,6 +102,16 @@ func (r *ApplicationReconciler) reconcileIngressGateway(ctx context.Context, app
 	}
 
 	for _, gateway := range gateways.Items {
+		shouldReconcile, err := r.ShouldReconcile(ctx, gateway)
+		if err != nil {
+			r.SetControllerFinishedOutcome(ctx, application, controllerName, err)
+			return reconcile.Result{}, err
+		}
+
+		if !shouldReconcile {
+			continue
+		}
+
 		// Skip unrelated gateways
 		if !isIngressGateway(gateway) {
 			continue

diff --git a/controllers/application/ingress_virtual_service.go b/controllers/application/ingress_virtual_service.go
@@ -28,6 +28,12 @@ func (r *ApplicationReconciler) reconcileIngressVirtualService(ctx context.Conte
 	var err error
 
 	if len(application.Spec.Ingresses) > 0 {
+		shouldReconcile, err := r.ShouldReconcile(ctx, &virtualService)
+		if err != nil || !shouldReconcile {
+			r.SetControllerFinishedOutcome(ctx, application, controllerName, err)
+			return reconcile.Result{}, err
+		}
+
 		_, err = ctrlutil.CreateOrPatch(ctx, r.GetClient(), &virtualService, func() error {
 
 			err := ctrlutil.SetControllerReference(application, &virtualService, r.GetScheme())

diff --git a/controllers/application/network_policy.go b/controllers/application/network_policy.go
@@ -30,6 +30,12 @@ func (r *ApplicationReconciler) reconcileNetworkPolicy(ctx context.Context, appl
 		},
 	}
 
+	shouldReconcile, err := r.ShouldReconcile(ctx, &networkPolicy)
+	if err != nil || !shouldReconcile {
+		r.SetControllerFinishedOutcome(ctx, application, controllerName, err)
+		return reconcile.Result{}, err
+	}
+
 	netpolSpec := networking.CreateNetPolSpec(
 		networking.NetPolOpts{
 			AccessPolicy:     application.Spec.AccessPolicy,

diff --git a/controllers/application/peer_authentication.go b/controllers/application/peer_authentication.go
@@ -17,7 +17,13 @@ func (r *ApplicationReconciler) reconcilePeerAuthentication(ctx context.Context,
 	r.SetControllerProgressing(ctx, application, controllerName)
 
 	peerAuthentication := securityv1beta1.PeerAuthentication{ObjectMeta: metav1.ObjectMeta{Namespace: application.Namespace, Name: application.Name}}
-	_, err := ctrlutil.CreateOrPatch(ctx, r.GetClient(), &peerAuthentication, func() error {
+	shouldReconcile, err := r.ShouldReconcile(ctx, &peerAuthentication)
+	if err != nil || !shouldReconcile {
+		r.SetControllerFinishedOutcome(ctx, application, controllerName, err)
+		return reconcile.Result{}, err
+	}
+
+	_, err = ctrlutil.CreateOrPatch(ctx, r.GetClient(), &peerAuthentication, func() error {
 		// Set application as owner of the peer authentication
 		err := ctrlutil.SetControllerReference(application, &peerAuthentication, r.GetScheme())
 		if err != nil {

diff --git a/controllers/application/pod_disruption_budget.go b/controllers/application/pod_disruption_budget.go
@@ -17,6 +17,11 @@ func (r *ApplicationReconciler) reconcilePodDisruptionBudget(ctx context.Context
 	_, _ = r.SetControllerProgressing(ctx, application, controllerName)
 
 	pdb := policyv1.PodDisruptionBudget{ObjectMeta: metav1.ObjectMeta{Namespace: application.Namespace, Name: application.Name}}
+	shouldReconcile, err := r.ShouldReconcile(ctx, &pdb)
+	if err != nil || !shouldReconcile {
+		r.SetControllerFinishedOutcome(ctx, application, controllerName, err)
+		return reconcile.Result{}, err
+	}
 
 	if *application.Spec.EnablePDB {
 		_, err := ctrlutil.CreateOrPatch(ctx, r.GetClient(), &pdb, func() error {

diff --git a/controllers/application/service.go b/controllers/application/service.go
@@ -24,7 +24,13 @@ func (r *ApplicationReconciler) reconcileService(ctx context.Context, applicatio
 	r.SetControllerProgressing(ctx, application, controllerName)
 
 	service := corev1.Service{ObjectMeta: metav1.ObjectMeta{Namespace: application.Namespace, Name: application.Name}}
-	_, err := ctrlutil.CreateOrPatch(ctx, r.GetClient(), &service, func() error {
+	shouldReconcile, err := r.ShouldReconcile(ctx, &service)
+	if err != nil || !shouldReconcile {
+		r.SetControllerFinishedOutcome(ctx, application, controllerName, err)
+		return reconcile.Result{}, err
+	}
+
+	_, err = ctrlutil.CreateOrPatch(ctx, r.GetClient(), &service, func() error {
 		// Set application as owner of the service
 		err := ctrlutil.SetControllerReference(application, &service, r.GetScheme())
 		if err != nil {

diff --git a/controllers/application/service_account.go b/controllers/application/service_account.go
@@ -16,7 +16,14 @@ func (r *ApplicationReconciler) reconcileServiceAccount(ctx context.Context, app
 	r.SetControllerProgressing(ctx, application, controllerName)
 
 	serviceAccount := corev1.ServiceAccount{ObjectMeta: metav1.ObjectMeta{Namespace: application.Namespace, Name: application.Name}}
-	_, err := ctrlutil.CreateOrPatch(ctx, r.GetClient(), &serviceAccount, func() error {
+
+	shouldReconcile, err := r.ShouldReconcile(ctx, &serviceAccount)
+	if err != nil || !shouldReconcile {
+		r.SetControllerFinishedOutcome(ctx, application, controllerName, err)
+		return reconcile.Result{}, err
+	}
+
+	_, err = ctrlutil.CreateOrPatch(ctx, r.GetClient(), &serviceAccount, func() error {
 		// Set application as owner of the sidecar
 		err := ctrlutil.SetControllerReference(application, &serviceAccount, r.GetScheme())
 		if err != nil {

diff --git a/controllers/application/service_monitor.go b/controllers/application/service_monitor.go
@@ -26,6 +26,12 @@ func (r *ApplicationReconciler) reconcileServiceMonitor(ctx context.Context, app
 		Labels:    map[string]string{"instance": "primary"},
 	}}
 
+	shouldReconcile, err := r.ShouldReconcile(ctx, &serviceMonitor)
+	if err != nil || !shouldReconcile {
+		r.SetControllerFinishedOutcome(ctx, application, controllerName, err)
+		return reconcile.Result{}, err
+	}
+
 	if application.Spec.Prometheus == nil {
 		err := client.IgnoreNotFound(r.GetClient().Delete(ctx, &serviceMonitor))
 		if err != nil {
@@ -37,7 +43,7 @@ func (r *ApplicationReconciler) reconcileServiceMonitor(ctx context.Context, app
 		return reconcile.Result{}, nil
 	}
 
-	_, err := ctrlutil.CreateOrPatch(ctx, r.GetClient(), &serviceMonitor, func() error {
+	_, err = ctrlutil.CreateOrPatch(ctx, r.GetClient(), &serviceMonitor, func() error {
 		// Set application as owner of the service
 		err := ctrlutil.SetControllerReference(application, &serviceMonitor, r.GetScheme())
 		if err != nil {

diff --git a/pkg/resourcegenerator/istio/service_entry.go b/pkg/resourcegenerator/istio/service_entry.go
@@ -64,6 +64,7 @@ func GetServiceEntriesToDelete(serviceEntriesInNamespace []*networkingv1beta1.Se
 	var serviceEntriesToDelete []networkingv1beta1.ServiceEntry
 
 	for _, serviceEntry := range serviceEntriesInNamespace {
+
 		ownerIndex := slices.IndexFunc(serviceEntry.GetOwnerReferences(), func(ownerReference metav1.OwnerReference) bool {
 			return ownerReference.Name == ownerName
 		})

diff --git a/pkg/util/reconciler.go b/pkg/util/reconciler.go
@@ -125,6 +125,24 @@ func (r *ReconcilerBase) IsIstioEnabledForNamespace(ctx context.Context, namespa
 	return exists
 }
 
+func hasIgnoreLabel(obj client.Object) bool {
+	labels := obj.GetLabels()
+	return labels["skiperator.kartverket.no/ignore"] == "true"
+}
+
+func (r *ReconcilerBase) ShouldReconcile(ctx context.Context, obj client.Object) (bool, error) {
+	err := r.GetClient().Get(ctx, client.ObjectKeyFromObject(obj), obj)
+	err = client.IgnoreNotFound(err)
+
+	if err != nil {
+		return false, err
+	}
+
+	shouldReconcile := !hasIgnoreLabel(obj)
+
+	return shouldReconcile, nil
+}
+
 func (r *ReconcilerBase) EmitWarningEvent(object runtime.Object, reason string, message string) {
 	r.GetRecorder().Event(
 		object,

diff --git a/tests/application/ignore-reconcile/00-application.yaml b/tests/application/ignore-reconcile/00-application.yaml
@@ -0,0 +1,9 @@
+apiVersion: skiperator.kartverket.no/v1alpha1
+kind: Application
+metadata:
+  name: ignore-reconcile
+spec:
+  image: image
+  port: 8080
+  ingresses:
+    - example.com
diff --git a/tests/application/ignore-reconcile/00-assert.yaml b/tests/application/ignore-reconcile/00-assert.yaml
@@ -0,0 +1,7 @@
+apiVersion: networking.istio.io/v1beta1
+kind: VirtualService
+metadata:
+   name: ignore-reconcile-ingress
+spec:
+  hosts:
+    - example.com
diff --git a/tests/application/ignore-reconcile/01-assert.yaml b/tests/application/ignore-reconcile/01-assert.yaml
@@ -0,0 +1,6 @@
+apiVersion: networking.istio.io/v1beta1
+kind: VirtualService
+metadata:
+  name: ignore-reconcile-ingress
+  labels:
+    skiperator.kartverket.no/ignore: "true"