From e6f428ca1f2622d507539fce33588f2f074ab50c Mon Sep 17 00:00:00 2001 From: Maskym Vavilov Date: Tue, 14 Nov 2023 15:37:46 +0000 Subject: [PATCH] GH-496 addressing comments p1 --- pkg/controllers/dnspolicy/dns_helper.go | 1 - .../dnspolicy/dnspolicy_dnsrecords.go | 72 +---------- pkg/utils/gateway_wrapper.go | 115 +++++++++++++----- ...dnspolicy_controller_health_checks_test.go | 6 +- ...dnspolicy_controller_multi_cluster_test.go | 6 +- .../dnspolicy_controller_test.go | 2 +- 6 files changed, 95 insertions(+), 107 deletions(-) diff --git a/pkg/controllers/dnspolicy/dns_helper.go b/pkg/controllers/dnspolicy/dns_helper.go index a08a7156..3df3efa3 100644 --- a/pkg/controllers/dnspolicy/dns_helper.go +++ b/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{} diff --git a/pkg/controllers/dnspolicy/dnspolicy_dnsrecords.go b/pkg/controllers/dnspolicy/dnspolicy_dnsrecords.go index a5908edf..b6185e5a 100644 --- a/pkg/controllers/dnspolicy/dnspolicy_dnsrecords.go +++ b/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 -} diff --git a/pkg/utils/gateway_wrapper.go b/pkg/utils/gateway_wrapper.go index 9d5416a8..05739369 100644 --- a/pkg/utils/gateway_wrapper.go +++ b/pkg/utils/gateway_wrapper.go @@ -2,59 +2,109 @@ 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" ) type GatewayWrapper struct { *gatewayv1beta1.Gateway - 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 +118,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 } diff --git a/test/policy_integration/dnspolicy_controller_health_checks_test.go b/test/policy_integration/dnspolicy_controller_health_checks_test.go index d88b84fa..3f86fe5a 100644 --- a/test/policy_integration/dnspolicy_controller_health_checks_test.go +++ b/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, }, } diff --git a/test/policy_integration/dnspolicy_controller_multi_cluster_test.go b/test/policy_integration/dnspolicy_controller_multi_cluster_test.go index dde6661d..407c0cef 100644 --- a/test/policy_integration/dnspolicy_controller_multi_cluster_test.go +++ b/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, }, } diff --git a/test/policy_integration/dnspolicy_controller_test.go b/test/policy_integration/dnspolicy_controller_test.go index e39ded3e..4f2a6d55 100644 --- a/test/policy_integration/dnspolicy_controller_test.go +++ b/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" )