refactor: Update dnsrecord creation to use common reconciler methods

api/v1alpha1/dnspolicy_types.go

@@ -315,7 +315,7 @@ func (p *DNSPolicy) WithHealthCheckFor(endpoint string, port *int, protocol kuad
 
 func (p *DNSPolicy) WithLoadBalancingWeighted(lbWeighted LoadBalancingWeighted) *DNSPolicy {
 	if p.Spec.LoadBalancing == nil {
-		p.Spec.LoadBalancing = &LoadBalancingSpec{}
+		p.WithLoadBalancing(LoadBalancingSpec{})
 	}
 	p.Spec.LoadBalancing.Weighted = &lbWeighted
 	return p

controllers/dns_helper.go

@@ -10,12 +10,9 @@ import (
 
 	"golang.org/x/net/publicsuffix"
 
-	"k8s.io/apimachinery/pkg/api/equality"
-	k8serrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/labels"
 	"sigs.k8s.io/controller-runtime/pkg/client"
-	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 	"sigs.k8s.io/controller-runtime/pkg/log"
 	gatewayapiv1 "sigs.k8s.io/gateway-api/apis/v1"
 
@@ -105,36 +102,6 @@ func gatewayDNSRecordLabels(gwKey client.ObjectKey) map[string]string {
 	}
 }
 
-func (dh *dnsHelper) buildDNSRecordForListener(gateway *gatewayapiv1.Gateway, dnsPolicy *v1alpha1.DNSPolicy, targetListener gatewayapiv1.Listener, managedZone *kuadrantdnsv1alpha1.ManagedZone) *kuadrantdnsv1alpha1.DNSRecord {
-	dnsRecord := &kuadrantdnsv1alpha1.DNSRecord{
-		ObjectMeta: metav1.ObjectMeta{
-			Name:      dnsRecordName(gateway.Name, string(targetListener.Name)),
-			Namespace: managedZone.Namespace,
-			Labels:    commonDNSRecordLabels(client.ObjectKeyFromObject(gateway), client.ObjectKeyFromObject(dnsPolicy)),
-		},
-		Spec: kuadrantdnsv1alpha1.DNSRecordSpec{
-			ManagedZoneRef: &kuadrantdnsv1alpha1.ManagedZoneReference{
-				Name: managedZone.Name,
-			},
-		},
-	}
-	dnsRecord.Labels[LabelListenerReference] = string(targetListener.Name)
-	return dnsRecord
-}
-
-// getDNSRecordForListener returns a v1alpha1.DNSRecord, if one exists, for the given listener in the given kuadrantdnsv1alpha1.ManagedZone.
-func (dh *dnsHelper) getDNSRecordForListener(ctx context.Context, listener gatewayapiv1.Listener, owner metav1.Object) (*kuadrantdnsv1alpha1.DNSRecord, error) {
-	recordName := dnsRecordName(owner.GetName(), string(listener.Name))
-	dnsRecord := &kuadrantdnsv1alpha1.DNSRecord{}
-	if err := dh.Get(ctx, client.ObjectKey{Name: recordName, Namespace: owner.GetNamespace()}, dnsRecord); err != nil {
-		if k8serrors.IsNotFound(err) {
-			log.Log.V(1).Info("no dnsrecord found for listener ", "listener", listener)
-		}
-		return nil, err
-	}
-	return dnsRecord, nil
-}
-
 func withGatewayListener[T metav1.Object](gateway common.GatewayWrapper, listener gatewayapiv1.Listener, obj T) T {
 	if obj.GetAnnotations() == nil {
 		obj.SetAnnotations(map[string]string{})
@@ -146,8 +113,7 @@ func withGatewayListener[T metav1.Object](gateway common.GatewayWrapper, listene
 	return obj
 }
 
-func (dh *dnsHelper) setEndpoints(ctx context.Context, mcgTarget *multicluster.GatewayTarget, dnsRecord *kuadrantdnsv1alpha1.DNSRecord, listener gatewayapiv1.Listener, strategy v1alpha1.RoutingStrategy) error {
-	old := dnsRecord.DeepCopy()
+func (dh *dnsHelper) setEndpoints(mcgTarget *multicluster.GatewayTarget, dnsRecord *kuadrantdnsv1alpha1.DNSRecord, listener gatewayapiv1.Listener, strategy v1alpha1.RoutingStrategy) error {
 	gwListenerHost := string(*listener.Hostname)
 	var endpoints []*kuadrantdnsv1alpha1.Endpoint
 
@@ -172,10 +138,6 @@ func (dh *dnsHelper) setEndpoints(ctx context.Context, mcgTarget *multicluster.G
 
 	dnsRecord.Spec.Endpoints = endpoints
 
-	if !equality.Semantic.DeepEqual(old, dnsRecord) {
-		return dh.Update(ctx, dnsRecord)
-	}
-
 	return nil
 }
 
@@ -384,27 +346,6 @@ func dnsRecordName(gatewayName, listenerName string) string {
 	return fmt.Sprintf("%s-%s", gatewayName, listenerName)
 }
 
-func (dh *dnsHelper) createDNSRecordForListener(ctx context.Context, gateway *gatewayapiv1.Gateway, dnsPolicy *v1alpha1.DNSPolicy, mz *kuadrantdnsv1alpha1.ManagedZone, listener gatewayapiv1.Listener) (*kuadrantdnsv1alpha1.DNSRecord, error) {
-	logger := log.FromContext(ctx)
-	logger.Info("creating dns for gateway listener", "listener", listener.Name)
-	dnsRecord := dh.buildDNSRecordForListener(gateway, dnsPolicy, listener, mz)
-	if err := controllerutil.SetControllerReference(dnsPolicy, dnsRecord, dh.Scheme()); err != nil {
-		return dnsRecord, err
-	}
-
-	err := dh.Create(ctx, dnsRecord, &client.CreateOptions{})
-	if err != nil && !k8serrors.IsAlreadyExists(err) {
-		return dnsRecord, err
-	}
-	if err != nil && k8serrors.IsAlreadyExists(err) {
-		err = dh.Get(ctx, client.ObjectKeyFromObject(dnsRecord), dnsRecord)
-		if err != nil {
-			return dnsRecord, err
-		}
-	}
-	return dnsRecord, nil
-}
-
 func (dh *dnsHelper) deleteDNSRecordForListener(ctx context.Context, owner metav1.Object, listener gatewayapiv1.Listener) error {
 	recordName := dnsRecordName(owner.GetName(), string(listener.Name))
 	dnsRecord := kuadrantdnsv1alpha1.DNSRecord{

controllers/dnspolicy_controller_multi_cluster_test.go

@@ -304,8 +304,8 @@ var _ = Describe("DNSPolicy Multi Cluster", func() {
 				dnsPolicy = v1alpha1.NewDNSPolicy("test-dns-policy", testNamespace).
 					WithTargetGateway(TestGatewayName).
 					WithRoutingStrategy(v1alpha1.LoadBalancedRoutingStrategy).
-					WithLoadBalancingWeightedFor(120, nil).
-					WithLoadBalancingGeoFor("IE")
+					WithLoadBalancingGeoFor("IE").
+					WithLoadBalancingWeightedFor(120, nil)
 				Expect(k8sClient.Create(ctx, dnsPolicy)).To(Succeed())
 			})
 

controllers/dnspolicy_controller_test.go

@@ -116,7 +116,7 @@ var _ = Describe("DNSPolicy controller", func() {
 			// this one should get deleted if the gateway is invalid policy ref
 			probe := &kuadrantdnsv1alpha1.DNSHealthCheckProbe{
 				ObjectMeta: metav1.ObjectMeta{
-					Name:      fmt.Sprintf("%s-%s-%s", TestIPAddressTwo, TestGatewayName, TestHostOne),
+					Name:      fmt.Sprintf("%s-%s-%s", TestIPAddressTwo, TestGatewayName, TestHostTwo),
 					Namespace: testNamespace,
 					Labels: map[string]string{
 						common.DNSPolicyBackRefAnnotation:                              "test-dns-policy",
@@ -138,7 +138,7 @@ var _ = Describe("DNSPolicy controller", func() {
 		It("should not process gateway with inconsistent addresses", func() {
 			// build invalid gateway
 			gateway = NewGatewayBuilder("test-gateway", gatewayClass.Name, testNamespace).
-				WithHTTPListener(TestListenerNameOne, TestHostOne).Gateway
+				WithHTTPListener(TestListenerNameOne, TestHostTwo).Gateway
 			Expect(k8sClient.Create(ctx, gateway)).To(Succeed())
 
 			// ensure Gateway exists and invalidate it by setting inconsistent addresses
@@ -201,7 +201,7 @@ var _ = Describe("DNSPolicy controller", func() {
 
 		BeforeEach(func() {
 			gateway = NewGatewayBuilder(testGatewayName, gatewayClass.Name, testNamespace).
-				WithHTTPListener(TestListenerNameOne, TestHostOne).
+				WithHTTPListener(TestListenerNameOne, TestHostTwo).
 				Gateway
 			dnsPolicy = v1alpha1.NewDNSPolicy("test-dns-policy", testNamespace).
 				WithTargetGateway(testGatewayName).
@@ -254,7 +254,7 @@ var _ = Describe("DNSPolicy controller", func() {
 
 		BeforeEach(func() {
 			gateway = NewGatewayBuilder(TestGatewayName, gatewayClass.Name, testNamespace).
-				WithHTTPListener(TestListenerNameOne, TestHostOne).
+				WithHTTPListener(TestListenerNameOne, TestHostTwo).
 				WithHTTPListener(TestListenerNameWildcard, TestHostWildcard).
 				Gateway
 			dnsPolicy = v1alpha1.NewDNSPolicy("test-dns-policy", testNamespace).

controllers/dnspolicy_dnsrecords.go

@@ -3,8 +3,10 @@ package controllers
 import (
 	"context"
 	"fmt"
+	"reflect"
 
-	k8serrors "k8s.io/apimachinery/pkg/api/errors"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/labels"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	crlog "sigs.k8s.io/controller-runtime/pkg/log"
@@ -86,35 +88,59 @@ func (r *DNSPolicyReconciler) reconcileGatewayDNSRecords(ctx context.Context, gw
 			}
 			return nil
 		}
-		dnsRecord, err := r.dnsHelper.createDNSRecordForListener(ctx, gatewayWrapper.Gateway, dnsPolicy, mz, listener)
-		if err := client.IgnoreAlreadyExists(err); err != nil {
-			return fmt.Errorf("failed to create dns record for listener host %s : %w", *listener.Hostname, err)
-		}
-		if k8serrors.IsAlreadyExists(err) {
-			dnsRecord, err = r.dnsHelper.getDNSRecordForListener(ctx, listener, gatewayWrapper)
-			if err != nil {
-				return fmt.Errorf("failed to get dns record for host %s : %w", listener.Name, err)
-			}
-		}
 
-		mcgTarget, err := multicluster.NewGatewayTarget(gatewayWrapper.Gateway, listenerGateways, dnsPolicy.Spec.LoadBalancing)
+		dnsRecord, err := r.desiredDNSRecord(ctx, gatewayWrapper.Gateway, dnsPolicy, listener, listenerGateways, mz)
 		if err != nil {
-			return fmt.Errorf("failed to create multi cluster gateway target for listener %s : %w", listener.Name, err)
+			return err
 		}
 
-		log.Info("setting dns dnsTargets for gateway listener", "listener", dnsRecord.Name, "values", mcgTarget)
-		probes, err := r.dnsHelper.getDNSHealthCheckProbes(ctx, mcgTarget.Gateway, dnsPolicy)
+		err = r.SetOwnerReference(dnsPolicy, dnsRecord)
 		if err != nil {
 			return err
 		}
-		mcgTarget.RemoveUnhealthyGatewayAddresses(probes, listener)
-		if err := r.dnsHelper.setEndpoints(ctx, mcgTarget, dnsRecord, listener, dnsPolicy.Spec.RoutingStrategy); err != nil {
-			return fmt.Errorf("failed to add dns record dnsTargets %w %v", err, mcgTarget)
+
+		err = r.ReconcileResource(ctx, &kuadrantdnsv1alpha1.DNSRecord{}, dnsRecord, dnsRecordBasicMutator)
+		if err != nil && !apierrors.IsAlreadyExists(err) {
+			log.Error(err, "ReconcileResource failed to create/update DNSRecord resource")
+			return err
 		}
 	}
 	return nil
 }
 
+func (r *DNSPolicyReconciler) desiredDNSRecord(ctx context.Context, gateway *gatewayapiv1.Gateway, dnsPolicy *v1alpha1.DNSPolicy, targetListener gatewayapiv1.Listener, clusterGateways []multicluster.ClusterGateway, managedZone *kuadrantdnsv1alpha1.ManagedZone) (*kuadrantdnsv1alpha1.DNSRecord, error) {
+	dnsRecord := &kuadrantdnsv1alpha1.DNSRecord{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      dnsRecordName(gateway.Name, string(targetListener.Name)),
+			Namespace: managedZone.Namespace,
+			Labels:    commonDNSRecordLabels(client.ObjectKeyFromObject(gateway), client.ObjectKeyFromObject(dnsPolicy)),
+		},
+		Spec: kuadrantdnsv1alpha1.DNSRecordSpec{
+			ManagedZoneRef: &kuadrantdnsv1alpha1.ManagedZoneReference{
+				Name: managedZone.Name,
+			},
+		},
+	}
+	dnsRecord.Labels[LabelListenerReference] = string(targetListener.Name)
+
+	mcgTarget, err := multicluster.NewGatewayTarget(gateway, clusterGateways, dnsPolicy.Spec.LoadBalancing)
+	if err != nil {
+		return nil, fmt.Errorf("failed to create multi cluster gateway target for listener %s : %w", targetListener.Name, err)
+	}
+
+	probes, err := r.dnsHelper.getDNSHealthCheckProbes(ctx, mcgTarget.Gateway, dnsPolicy)
+	if err != nil {
+		return nil, err
+	}
+	mcgTarget.RemoveUnhealthyGatewayAddresses(probes, targetListener)
+
+	if err = r.dnsHelper.setEndpoints(mcgTarget, dnsRecord, targetListener, dnsPolicy.Spec.RoutingStrategy); err != nil {
+		return nil, fmt.Errorf("failed to add dns record dnsTargets %w %v", err, mcgTarget)
+	}
+
+	return dnsRecord, nil
+}
+
 func (r *DNSPolicyReconciler) deleteGatewayDNSRecords(ctx context.Context, gateway *gatewayapiv1.Gateway, dnsPolicy *v1alpha1.DNSPolicy) error {
 	return r.deleteDNSRecordsWithLabels(ctx, commonDNSRecordLabels(client.ObjectKeyFromObject(gateway), client.ObjectKeyFromObject(dnsPolicy)), dnsPolicy.Namespace)
 }
@@ -140,3 +166,22 @@ func (r *DNSPolicyReconciler) deleteDNSRecordsWithLabels(ctx context.Context, lb
 	}
 	return nil
 }
+
+func dnsRecordBasicMutator(existingObj, desiredObj client.Object) (bool, error) {
+	existing, ok := existingObj.(*kuadrantdnsv1alpha1.DNSRecord)
+	if !ok {
+		return false, fmt.Errorf("%T is not an *kuadrantdnsv1alpha1.DNSRecord", existingObj)
+	}
+	desired, ok := desiredObj.(*kuadrantdnsv1alpha1.DNSRecord)
+	if !ok {
+		return false, fmt.Errorf("%T is not an *kuadrantdnsv1alpha1.DNSRecord", desiredObj)
+	}
+
+	if reflect.DeepEqual(existing.Spec, desired.Spec) {
+		return false, nil
+	}
+
+	existing.Spec = desired.Spec
+
+	return true, nil
+}

controllers/helper_test.go

@@ -49,7 +49,7 @@ const (
 	TestClusterNameOne       = "test-placed-control"
 	TestClusterNameTwo       = "test-placed-workload-1"
 	TestHostOne              = "test.example.com"
-	TestHostTwo              = "other.example.com"
+	TestHostTwo              = "other.test.example.com"
 	TestHostWildcard         = "*.example.com"
 	TestListenerNameWildcard = "wildcard"
 	TestListenerNameOne      = "test-listener-1"