GH-496 addressing comments p1

pkg/controllers/dnspolicy/dns_helper.go

@@ -124,7 +124,6 @@ func (dh *dnsHelper) buildDNSRecordForListener(gateway *gatewayv1beta1.Gateway,
 }
 
 // getDNSRecordForListener returns a v1alpha1.DNSRecord, if one exists, for the given listener in the given v1alpha1.ManagedZone.
-// It needs a reference string to enforce DNS record serving a single utils.Interface owner
 func (dh *dnsHelper) getDNSRecordForListener(ctx context.Context, listener gatewayv1beta1.Listener, owner metav1.Object) (*v1alpha1.DNSRecord, error) {
 	recordName := dnsRecordName(owner.GetName(), string(listener.Name))
 	dnsRecord := &v1alpha1.DNSRecord{}

pkg/controllers/dnspolicy/dnspolicy_dnsrecords.go

@@ -3,7 +3,6 @@ package dnspolicy
 import (
 	"context"
 	"fmt"
-	"strings"
 
 	clusterv1 "open-cluster-management.io/api/cluster/v1"
 
@@ -20,10 +19,6 @@ import (
 	"github.com/Kuadrant/multicluster-gateway-controller/pkg/utils"
 )
 
-const (
-	singleCluster = "kudarant.io/single"
-)
-
 func (r *DNSPolicyReconciler) reconcileDNSRecords(ctx context.Context, dnsPolicy *v1alpha1.DNSPolicy, gwDiffObj *reconcilers.GatewayDiff) error {
 	log := crlog.FromContext(ctx)
 
@@ -48,7 +43,8 @@ func (r *DNSPolicyReconciler) reconcileDNSRecords(ctx context.Context, dnsPolicy
 func (r *DNSPolicyReconciler) reconcileGatewayDNSRecords(ctx context.Context, gw *gatewayv1beta1.Gateway, dnsPolicy *v1alpha1.DNSPolicy) error {
 	log := crlog.FromContext(ctx)
 
-	gatewayWrapper, err := utils.NewGatewayWrapper(gw)
+	gatewayWrapper := utils.NewGatewayWrapper(gw)
+	err := gatewayWrapper.Validate()
 	if err != nil {
 		return err
 	}
@@ -58,7 +54,7 @@ func (r *DNSPolicyReconciler) reconcileGatewayDNSRecords(ctx context.Context, gw
 		return err
 	}
 
-	clusterGatewayAddresses := getClusterGatewayAddresses(gatewayWrapper)
+	clusterGatewayAddresses := gatewayWrapper.GetClusterGatewayAddresses()
 
 	log.V(3).Info("checking gateway for attached routes ", "gateway", gatewayWrapper.Name, "clusters", clusterGatewayAddresses)
 
@@ -77,7 +73,7 @@ func (r *DNSPolicyReconciler) reconcileGatewayDNSRecords(ctx context.Context, gw
 			// Only consider host for dns if there's at least 1 attached route to the listener for this host in *any* gateway
 
 			log.V(3).Info("checking downstream", "listener ", listener.Name)
-			attached := listenerTotalAttachedRoutes(gatewayWrapper, clusterName, listener)
+			attached := gatewayWrapper.ListenerTotalAttachedRoutes(clusterName, listener)
 
 			if attached == 0 {
 				log.V(1).Info("no attached routes for ", "listener", listener, "cluster ", clusterName)
@@ -161,7 +157,7 @@ func (r *DNSPolicyReconciler) buildClusterGateway(ctx context.Context, clusterNa
 	singleClusterAddresses := make([]gatewayv1beta1.GatewayAddress, len(gatewayAddresses))
 
 	var metaObj client.Object
-	if clusterName != singleCluster {
+	if clusterName != utils.SingleClusterAddressValue {
 		mc := &clusterv1.ManagedCluster{}
 		if err := r.Client().Get(ctx, client.ObjectKey{Name: clusterName}, mc, &client.GetOptions{}); err != nil {
 			return target, err
@@ -172,11 +168,7 @@ func (r *DNSPolicyReconciler) buildClusterGateway(ctx context.Context, clusterNa
 	}
 
 	for i, addr := range gatewayAddresses {
-		addrType, multicluster := utils.AddressTypeToSingleCluster(addr)
-
-		if !multicluster {
-			addrType = *addr.Type
-		}
+		addrType := utils.AddressTypeToSingleCluster(addr)
 
 		singleClusterAddresses[i] = gatewayv1beta1.GatewayAddress{
 			Type:  &addrType,
@@ -187,55 +179,3 @@ func (r *DNSPolicyReconciler) buildClusterGateway(ctx context.Context, clusterNa
 
 	return target, nil
 }
-
-func getClusterGatewayAddresses(gw *utils.GatewayWrapper) map[string][]gatewayv1beta1.GatewayAddress {
-	clusterAddrs := make(map[string][]gatewayv1beta1.GatewayAddress, len(gw.Status.Addresses))
-
-	for _, address := range gw.Status.Addresses {
-		//Default to Single Cluster (Normal Gateway Status)
-		cluster := singleCluster
-		addressValue := address.Value
-
-		//Check for Multi Cluster (MGC Gateway Status)
-		if gw.IsMultiCluster() {
-			tmpCluster, tmpAddress, found := strings.Cut(address.Value, "/")
-			//If this fails something is wrong and the value hasn't been set correctly
-			if found {
-				cluster = tmpCluster
-				addressValue = tmpAddress
-			}
-		}
-
-		if _, ok := clusterAddrs[cluster]; !ok {
-			clusterAddrs[cluster] = []gatewayv1beta1.GatewayAddress{}
-		}
-
-		clusterAddrs[cluster] = append(clusterAddrs[cluster], gatewayv1beta1.GatewayAddress{
-			Type:  address.Type,
-			Value: addressValue,
-		})
-	}
-
-	return clusterAddrs
-}
-
-func listenerTotalAttachedRoutes(upstreamGateway *utils.GatewayWrapper, downstreamCluster string, specListener gatewayv1beta1.Listener) int {
-	for _, statusListener := range upstreamGateway.Status.Listeners {
-		// for Multi Cluster (MGC Gateway Status)
-		if upstreamGateway.IsMultiCluster() {
-			clusterName, listenerName, found := strings.Cut(string(statusListener.Name), ".")
-			if !found {
-				return 0
-			}
-			if clusterName == downstreamCluster && listenerName == string(specListener.Name) {
-				return int(statusListener.AttachedRoutes)
-			}
-		}
-		// Single Cluster (Normal Gateway Status)
-		if string(statusListener.Name) == string(specListener.Name) {
-			return int(statusListener.AttachedRoutes)
-		}
-	}
-
-	return 0
-}

pkg/utils/gateway_wrapper.go

@@ -2,12 +2,14 @@ package utils
 
 import (
 	"fmt"
+	"strings"
 
-	"k8s.io/apimachinery/pkg/types"
 	gatewayv1beta1 "sigs.k8s.io/gateway-api/apis/v1beta1"
 )
 
 const (
+	SingleClusterAddressValue = "kudarant.io/single"
+
 	MultiClusterIPAddressType       gatewayv1beta1.AddressType = "kuadrant.io/MultiClusterIPAddress"
 	MultiClusterHostnameAddressType gatewayv1beta1.AddressType = "kuadrant.io/MultiClusterHostnameAddress"
 )
@@ -17,44 +19,93 @@ type GatewayWrapper struct {
 	isMultiCluster bool
 }
 
-func NewGatewayWrapper(g *gatewayv1beta1.Gateway) (*GatewayWrapper, error) {
-	gw := &GatewayWrapper{Gateway: g, isMultiCluster: false}
-
-	for i, address := range gw.Status.Addresses {
-		if i == 0 {
-			gw.isMultiCluster = isMultiClusterAddressType(*address.Type)
-			continue
-		}
-		if gw.isMultiCluster == isMultiClusterAddressType(*address.Type) {
-			continue
-		}
-		return nil, fmt.Errorf("gateway is invalid: inconsistent status addresses")
-
-	}
-	return gw, nil
+func NewGatewayWrapper(g *gatewayv1beta1.Gateway) *GatewayWrapper {
+	return &GatewayWrapper{Gateway: g}
 }
 
 func isMultiClusterAddressType(addressType gatewayv1beta1.AddressType) bool {
 	return addressType == MultiClusterIPAddressType || addressType == MultiClusterHostnameAddressType
 }
 
-func (a *GatewayWrapper) GetKind() string {
-	return "GatewayWrapper"
+// IsMultiCluster reports a type of the first address in the Status block
+// returns false if no addresses present
+func (g *GatewayWrapper) IsMultiCluster() bool {
+	if len(g.Status.Addresses) > 0 {
+		return isMultiClusterAddressType(*g.Status.Addresses[0].Type)
+	}
+	return false
 }
 
-func (a *GatewayWrapper) IsMultiCluster() bool {
-	return a.isMultiCluster
+// Validate ensures correctly configured underlying Gateway object
+// Returns nil if validation passed
+func (g *GatewayWrapper) Validate() error {
+
+	// Status.Addresses validation
+	// Compares all addresses against the first address to ensure the same type
+	for _, address := range g.Status.Addresses {
+		if g.IsMultiCluster() == isMultiClusterAddressType(*address.Type) {
+			continue
+		}
+		return fmt.Errorf("gateway is invalid: inconsistent status addresses")
+	}
+	return nil
 }
 
-func (a *GatewayWrapper) GetNamespaceName() types.NamespacedName {
-	return types.NamespacedName{
-		Namespace: a.Namespace,
-		Name:      a.Name,
+// GetClusterGatewayAddresses constructs a map from Status.Addresses of underlying Gateway
+// with key being a cluster and value being an address in the cluster.
+// In case of a single-cluster Gateway the key is SingleClusterAddressValue
+func (g *GatewayWrapper) GetClusterGatewayAddresses() map[string][]gatewayv1beta1.GatewayAddress {
+	clusterAddrs := make(map[string][]gatewayv1beta1.GatewayAddress, len(g.Status.Addresses))
+
+	for _, address := range g.Status.Addresses {
+		//Default to Single Cluster (Normal Gateway Status)
+		cluster := SingleClusterAddressValue
+		addressValue := address.Value
+
+		//Check for Multi Cluster (MGC Gateway Status)
+		if g.IsMultiCluster() {
+			tmpCluster, tmpAddress, found := strings.Cut(address.Value, "/")
+			//If this fails something is wrong and the value hasn't been set correctly
+			if found {
+				cluster = tmpCluster
+				addressValue = tmpAddress
+			}
+		}
+
+		if _, ok := clusterAddrs[cluster]; !ok {
+			clusterAddrs[cluster] = []gatewayv1beta1.GatewayAddress{}
+		}
+
+		clusterAddrs[cluster] = append(clusterAddrs[cluster], gatewayv1beta1.GatewayAddress{
+			Type:  address.Type,
+			Value: addressValue,
+		})
 	}
+
+	return clusterAddrs
 }
 
-func (a *GatewayWrapper) String() string {
-	return fmt.Sprintf("kind: %v, namespace/name: %v", a.GetKind(), a.GetNamespaceName())
+// ListenerTotalAttachedRoutes returns a count of attached routes from the Status.Listeners for a specified
+// combination of downstreamClusterName and specListener.Name
+func (g *GatewayWrapper) ListenerTotalAttachedRoutes(downstreamClusterName string, specListener gatewayv1beta1.Listener) int {
+	for _, statusListener := range g.Status.Listeners {
+		// for Multi Cluster (MGC Gateway Status)
+		if g.IsMultiCluster() {
+			clusterName, listenerName, found := strings.Cut(string(statusListener.Name), ".")
+			if !found {
+				return 0
+			}
+			if clusterName == downstreamClusterName && listenerName == string(specListener.Name) {
+				return int(statusListener.AttachedRoutes)
+			}
+		}
+		// Single Cluster (Normal Gateway Status)
+		if string(statusListener.Name) == string(specListener.Name) {
+			return int(statusListener.AttachedRoutes)
+		}
+	}
+
+	return 0
 }
 
 // AddressTypeToMultiCluster returns a multi cluster version of the address type
@@ -68,13 +119,12 @@ func AddressTypeToMultiCluster(address gatewayv1beta1.GatewayAddress) (gatewayv1
 	return "", false
 }
 
-// AddressTypeToSingleCluster returns a single cluster version of the address type
-// and a bool to indicate that provided address was of the multi cluster type
-func AddressTypeToSingleCluster(address gatewayv1beta1.GatewayAddress) (gatewayv1beta1.AddressType, bool) {
+// AddressTypeToSingleCluster converts provided multicluster address to single cluster version if applicable
+func AddressTypeToSingleCluster(address gatewayv1beta1.GatewayAddress) gatewayv1beta1.AddressType {
 	if *address.Type == MultiClusterIPAddressType {
-		return gatewayv1beta1.IPAddressType, true
+		return gatewayv1beta1.IPAddressType
 	} else if *address.Type == MultiClusterHostnameAddressType {
-		return gatewayv1beta1.HostnameAddressType, true
+		return gatewayv1beta1.HostnameAddressType
 	}
-	return "", false
+	return *address.Type
 }

test/policy_integration/dnspolicy_controller_health_checks_test.go

@@ -18,8 +18,8 @@ import (
 	gatewayv1beta1 "sigs.k8s.io/gateway-api/apis/v1beta1"
 
 	"github.com/Kuadrant/multicluster-gateway-controller/pkg/apis/v1alpha1"
-	mgcgateway "github.com/Kuadrant/multicluster-gateway-controller/pkg/controllers/gateway"
 	"github.com/Kuadrant/multicluster-gateway-controller/pkg/dns"
+	"github.com/Kuadrant/multicluster-gateway-controller/pkg/utils"
 	testutil "github.com/Kuadrant/multicluster-gateway-controller/test/util"
 )
 
@@ -66,11 +66,11 @@ var _ = Describe("DNSPolicy Health Checks", func() {
 			}
 			gateway.Status.Addresses = []gatewayv1beta1.GatewayAddress{
 				{
-					Type:  testutil.Pointer(mgcgateway.MultiClusterIPAddressType),
+					Type:  testutil.Pointer(utils.MultiClusterIPAddressType),
 					Value: TestClusterNameOne + "/" + TestIPAddressOne,
 				},
 				{
-					Type:  testutil.Pointer(mgcgateway.MultiClusterIPAddressType),
+					Type:  testutil.Pointer(utils.MultiClusterIPAddressType),
 					Value: TestClusterNameTwo + "/" + TestIPAddressTwo,
 				},
 			}

test/policy_integration/dnspolicy_controller_multi_cluster_test.go

@@ -17,8 +17,8 @@ import (
 	gatewayv1beta1 "sigs.k8s.io/gateway-api/apis/v1beta1"
 
 	"github.com/Kuadrant/multicluster-gateway-controller/pkg/apis/v1alpha1"
-	mgcgateway "github.com/Kuadrant/multicluster-gateway-controller/pkg/controllers/gateway"
 	"github.com/Kuadrant/multicluster-gateway-controller/pkg/dns"
+	"github.com/Kuadrant/multicluster-gateway-controller/pkg/utils"
 	testutil "github.com/Kuadrant/multicluster-gateway-controller/test/util"
 )
 
@@ -65,11 +65,11 @@ var _ = Describe("DNSPolicy Multi Cluster", func() {
 			}
 			gateway.Status.Addresses = []gatewayv1beta1.GatewayAddress{
 				{
-					Type:  testutil.Pointer(mgcgateway.MultiClusterIPAddressType),
+					Type:  testutil.Pointer(utils.MultiClusterIPAddressType),
 					Value: TestClusterNameOne + "/" + TestIPAddressOne,
 				},
 				{
-					Type:  testutil.Pointer(mgcgateway.MultiClusterIPAddressType),
+					Type:  testutil.Pointer(utils.MultiClusterIPAddressType),
 					Value: TestClusterNameTwo + "/" + TestIPAddressTwo,
 				},
 			}

test/policy_integration/dnspolicy_controller_test.go

@@ -20,7 +20,7 @@ import (
 	"github.com/Kuadrant/multicluster-gateway-controller/pkg/_internal/conditions"
 	"github.com/Kuadrant/multicluster-gateway-controller/pkg/apis/v1alpha1"
 	. "github.com/Kuadrant/multicluster-gateway-controller/pkg/controllers/dnspolicy"
-	mgcgateway "github.com/Kuadrant/multicluster-gateway-controller/pkg/controllers/gateway"
+	"github.com/Kuadrant/multicluster-gateway-controller/pkg/utils"
 	testutil "github.com/Kuadrant/multicluster-gateway-controller/test/util"
 )