From d52dc05ff33f4259880f6b657601bab0a53e386f Mon Sep 17 00:00:00 2001 From: Shriram Sharma Date: Wed, 9 Jul 2025 22:38:30 -0700 Subject: [PATCH 1/3] fixed dr pinning issue --- .../pkg/clusters/destinationrule_handler.go | 7 ++ admiral/pkg/clusters/serviceentry.go | 17 ++++- admiral/pkg/clusters/serviceentry_test.go | 34 +++++++++- admiral/pkg/clusters/util.go | 2 +- .../pkg/clusters/virtualservice_routing.go | 16 ++++- .../clusters/virtualservice_routing_test.go | 67 +++++++++++++++++++ 6 files changed, 136 insertions(+), 7 deletions(-) diff --git a/admiral/pkg/clusters/destinationrule_handler.go b/admiral/pkg/clusters/destinationrule_handler.go index 6098dc00..79bcb8c6 100644 --- a/admiral/pkg/clusters/destinationrule_handler.go +++ b/admiral/pkg/clusters/destinationrule_handler.go @@ -107,12 +107,19 @@ func getDestinationRule(se *networkingV1Alpha3.ServiceEntry, if isSEMultiRegion(se) { dr.TrafficPolicy.LoadBalancer.LocalityLbSetting, err = getLocalityLBSettings(locality) if err == nil { + ctxLogger.Infof(common.CtxLogFormat, + "doDRUpdateForInClusterRouting", "", "", "", + fmt.Sprintf("dr pinning successful for host %s", se.Hosts[0])) return dr } else { ctxLogger.Errorf(common.CtxLogFormat, "doDRUpdateForInClusterRouting", "", "", "", fmt.Sprintf("error getting locality LB settings: %v", err)) } + } else { + ctxLogger.Infof(common.CtxLogFormat, + "doDRUpdateForInClusterRouting", "", "", "", + fmt.Sprintf("Skipping DR pinning as ServiceEntry is not multi-region for host: %s", se.Hosts[0])) } } diff --git a/admiral/pkg/clusters/serviceentry.go b/admiral/pkg/clusters/serviceentry.go index cccb3fc7..d0b99b6d 100644 --- a/admiral/pkg/clusters/serviceentry.go +++ b/admiral/pkg/clusters/serviceentry.go @@ -1951,7 +1951,10 @@ func IsCartographerVSDisabled( // It returns true if the in-cluster virtual service exists // and its exportTo does not contain the sync namespace, otherwise false. // The VSName is constructed using the service entry host name -func hasInClusterVSWithValidExportToNS(se *networking.ServiceEntry, rc *RemoteController) (bool, error) { +func hasInClusterVSWithValidExportToNS( + ctxLogger *logrus.Entry, + se *networking.ServiceEntry, + rc *RemoteController) (bool, error) { if rc == nil { return false, fmt.Errorf("remoteController is nil") } @@ -1967,7 +1970,14 @@ func hasInClusterVSWithValidExportToNS(se *networking.ServiceEntry, rc *RemoteCo if rc.VirtualServiceController.VirtualServiceCache == nil { return false, fmt.Errorf("VirtualServiceCache is nil in VirtualServiceController") } - vsName := fmt.Sprintf("%s-%s", se.Hosts[0], common.InclusterVSNameSuffix) + seHost := se.Hosts[0] + if strings.HasPrefix(seHost, canaryPrefix) { + seHost = strings.ReplaceAll(seHost, canaryPrefix+".", "") + } + if strings.HasPrefix(seHost, previewPrefix) { + seHost = strings.ReplaceAll(seHost, previewPrefix+".", "") + } + vsName := fmt.Sprintf("%s-%s", seHost, common.InclusterVSNameSuffix) cachedVS := rc.VirtualServiceController.VirtualServiceCache.Get(vsName) if cachedVS == nil { return false, fmt.Errorf("virtualService %s not found in cache", vsName) @@ -1977,6 +1987,9 @@ func hasInClusterVSWithValidExportToNS(se *networking.ServiceEntry, rc *RemoteCo return false, nil } } + ctxLogger.Infof(common.CtxLogFormat, "hasInClusterVSWithValidExportToNS", + "", "", rc.ClusterID, + fmt.Sprintf("in-cluster VS %s has valid exportTo namespaces", vsName)) return true, nil } diff --git a/admiral/pkg/clusters/serviceentry_test.go b/admiral/pkg/clusters/serviceentry_test.go index 6f786cf6..6ac8b002 100644 --- a/admiral/pkg/clusters/serviceentry_test.go +++ b/admiral/pkg/clusters/serviceentry_test.go @@ -10334,6 +10334,36 @@ func TestHasInClusterVSWithValidExportToNS(t *testing.T) { }, expected: true, }, + { + name: "Given an SE with canary host" + + "When hasInClusterVSWithValidExportToNS is called" + + "And the in-cluster VS has valid NS" + + "Then it should return true", + remoteCtrl: &RemoteController{ + VirtualServiceController: &istio.VirtualServiceController{ + VirtualServiceCache: virtualServiceCache, + }, + }, + serviceEntry: &istioNetworkingV1Alpha3.ServiceEntry{ + Hosts: []string{"canary.foo.testns.global"}, + }, + expected: true, + }, + { + name: "Given an SE with preview host" + + "When hasInClusterVSWithValidExportToNS is called" + + "And the in-cluster VS has valid NS" + + "Then it should return true", + remoteCtrl: &RemoteController{ + VirtualServiceController: &istio.VirtualServiceController{ + VirtualServiceCache: virtualServiceCache, + }, + }, + serviceEntry: &istioNetworkingV1Alpha3.ServiceEntry{ + Hosts: []string{"preview.foo.testns.global"}, + }, + expected: true, + }, { name: "Given an SE with valid hosts" + "When hasInClusterVSWithValidExportToNS is called" + @@ -10351,9 +10381,11 @@ func TestHasInClusterVSWithValidExportToNS(t *testing.T) { }, } + var ctxLogger = logrus.WithFields(logrus.Fields{}) + for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - result, err := hasInClusterVSWithValidExportToNS(tc.serviceEntry, tc.remoteCtrl) + result, err := hasInClusterVSWithValidExportToNS(ctxLogger, tc.serviceEntry, tc.remoteCtrl) if tc.expectedError != nil { assert.NotNil(t, err) assert.Equal(t, tc.expectedError.Error(), err.Error()) diff --git a/admiral/pkg/clusters/util.go b/admiral/pkg/clusters/util.go index 88d6b95d..fcc60b6b 100644 --- a/admiral/pkg/clusters/util.go +++ b/admiral/pkg/clusters/util.go @@ -276,9 +276,9 @@ func getSortedDependentNamespaces( namespaceSlice = []string{"*"} ctxLogger.Infof("exceeded max namespaces for cname=%s in cluster=%s", cname, clusterId) } - sort.Strings(namespaceSlice) } } + sort.Strings(namespaceSlice) // this is to avoid duplication in namespaceSlice e.g. dynamicrouting deployment present in istio-system can be a dependent of blackhole on blackhole's source cluster var dedupNamespaceSlice []string for i := 0; i < len(namespaceSlice); i++ { diff --git a/admiral/pkg/clusters/virtualservice_routing.go b/admiral/pkg/clusters/virtualservice_routing.go index 182fa747..fab5f8ae 100644 --- a/admiral/pkg/clusters/virtualservice_routing.go +++ b/admiral/pkg/clusters/virtualservice_routing.go @@ -1122,8 +1122,7 @@ func performDRPinning(ctx context.Context, newDR := cachedDR.DeepCopy() if newDR.Spec.TrafficPolicy == nil || - newDR.Spec.TrafficPolicy.LoadBalancer == nil || - newDR.Spec.TrafficPolicy.LoadBalancer.LocalityLbSetting == nil { + newDR.Spec.TrafficPolicy.LoadBalancer == nil { errs = append(errs, fmt.Errorf( "skipped pinning DR to remote region as TrafficPolicy or LoadBalancer or LocalityLbSetting is nil for DR %s in cluster %s", drName, sourceCluster)) @@ -2528,6 +2527,9 @@ func DoVSRoutingInClusterForClusterAndIdentity( ctxLogger.Infof(common.CtxLogFormat, "DoVSRoutingInClusterForClusterAndIdentity", identity, "", cluster, "identity has a custom VS in its namespace") return false + } else { + ctxLogger.Infof(common.CtxLogFormat, "DoVSRoutingInClusterForClusterAndIdentity", + identity, "", cluster, "identity does not have a custom VS in its namespace") } // This should be set to true if we need to check if .mesh DR should be pinned to remote @@ -2552,6 +2554,9 @@ func DoVSRoutingInClusterForClusterAndIdentity( ctxLogger.Infof(common.CtxLogFormat, "DoVSRoutingInClusterForClusterAndIdentity", identity, "", cluster, fmt.Sprintf("isCartographerVSDisabled=%v", isCartographerVSDisabled)) return false + } else { + ctxLogger.Infof(common.CtxLogFormat, "DoVSRoutingInClusterForClusterAndIdentity", + identity, "", cluster, fmt.Sprintf("isCartographerVSDisabled=%v", isCartographerVSDisabled)) } } @@ -2596,7 +2601,8 @@ func DoDRUpdateForInClusterVSRouting( } if isSourceCluster { // Check if the incluster VS has valid exportTo namespaces (not sync namespace) - hasValidInClusterVS, err := hasInClusterVSWithValidExportToNS(se, remoteRegistry.GetRemoteController(cluster)) + hasValidInClusterVS, err := hasInClusterVSWithValidExportToNS( + ctxLogger, se, remoteRegistry.GetRemoteController(cluster)) if err != nil { ctxLogger.Warnf(common.CtxLogFormat, "DoDRUpdateForInClusterVSRouting", identity, "", cluster, fmt.Sprintf("error checking for valid in-cluster VS %v", err)) @@ -2606,6 +2612,10 @@ func DoDRUpdateForInClusterVSRouting( ctxLogger.Infof(common.CtxLogFormat, "DoDRUpdateForInClusterVSRouting", identity, "", cluster, "skipping DR update as incluter VS does not have valid exportTo namespaces") return false + } else { + ctxLogger.Infof(common.CtxLogFormat, "DoDRUpdateForInClusterVSRouting", + identity, "", cluster, + fmt.Sprintf("in-cluster VS has valid exportTo namespaces for identity %s and env %s", identity, env)) } if DoVSRoutingInClusterForClusterAndIdentity( ctx, ctxLogger, env, cluster, identity, remoteRegistry, performCartographerVSCheck) { diff --git a/admiral/pkg/clusters/virtualservice_routing_test.go b/admiral/pkg/clusters/virtualservice_routing_test.go index 374ce164..769c4b57 100644 --- a/admiral/pkg/clusters/virtualservice_routing_test.go +++ b/admiral/pkg/clusters/virtualservice_routing_test.go @@ -8305,6 +8305,20 @@ func TestPerformDRPinning(t *testing.T) { }, } + cachedDefaultActiveActiveDR := &apiNetworkingV1Alpha3.DestinationRule{ + ObjectMeta: metaV1.ObjectMeta{ + Name: "foo.test-aa.global-default-dr", + Namespace: "sync-ns", + }, + Spec: networkingV1Alpha3.DestinationRule{ + Host: "foo.test-aa.global", + ExportTo: []string{"test-dependent-ns0", "test-dependent-ns1", "test-ns"}, + TrafficPolicy: &networkingV1Alpha3.TrafficPolicy{ + LoadBalancer: &networkingV1Alpha3.LoadBalancerSettings{}, + }, + }, + } + expectedDefaultDR := &apiNetworkingV1Alpha3.DestinationRule{ ObjectMeta: metaV1.ObjectMeta{ Name: "foo.test-ns.global-default-dr", @@ -8328,15 +8342,41 @@ func TestPerformDRPinning(t *testing.T) { }, } + expectedActiveActiveDefaultDR := &apiNetworkingV1Alpha3.DestinationRule{ + ObjectMeta: metaV1.ObjectMeta{ + Name: "foo.test-aa.global-default-dr", + Namespace: "sync-ns", + }, + Spec: networkingV1Alpha3.DestinationRule{ + Host: "foo.test-aa.global", + ExportTo: []string{"test-dependent-ns0", "test-dependent-ns1", "test-ns"}, + TrafficPolicy: &networkingV1Alpha3.TrafficPolicy{ + LoadBalancer: &networkingV1Alpha3.LoadBalancerSettings{ + LocalityLbSetting: &networkingV1Alpha3.LocalityLoadBalancerSetting{ + Distribute: []*networkingV1Alpha3.LocalityLoadBalancerSetting_Distribute{ + { + From: "*", + To: map[string]uint32{"us-east-2": 100}, + }, + }, + }, + }, + }, + }, + } + drCache := istio.NewDestinationRuleCache() drCache.Put(cachedDefaultDR) drCache.Put(cachedAdditionalEndpointDR) + drCache.Put(cachedDefaultActiveActiveDR) istioClient := istioFake.NewSimpleClientset() istioClient.NetworkingV1alpha3().DestinationRules("sync-ns"). Create(context.Background(), cachedDefaultDR, metaV1.CreateOptions{}) istioClient.NetworkingV1alpha3().DestinationRules("sync-ns"). Create(context.Background(), cachedAdditionalEndpointDR, metaV1.CreateOptions{}) + istioClient.NetworkingV1alpha3().DestinationRules("sync-ns"). + Create(context.Background(), cachedDefaultActiveActiveDR, metaV1.CreateOptions{}) ap := common.AdmiralParams{ LabelSet: &common.LabelSet{ @@ -8447,6 +8487,33 @@ func TestPerformDRPinning(t *testing.T) { env: "foo", expectDestinationRule: cachedDefaultDR, }, + { + name: "Given for a VS host there is active/active DR in cache and region needs to be flipped" + + "When performDRPinning func is called" + + "Then the func should not return an error and DR will be updated with new region", + remoteRegistry: &RemoteRegistry{}, + remoteController: &RemoteController{ + NodeController: &admiral.NodeController{ + Locality: &admiral.Locality{ + Region: "us-west-2", + }, + }, + DestinationRuleController: &istio.DestinationRuleController{ + IstioClient: istioClient, + Cache: drCache, + }, + }, + vs: &apiNetworkingV1Alpha3.VirtualService{ + Spec: networkingV1Alpha3.VirtualService{ + Hosts: []string{"foo.test-aa.global"}, + }, + }, + drName: "foo.test-aa.global-default-dr", + sourceCluster: "cluster1", + sourceIdentity: "foo.test-aa.global-default-dr", + env: "foo", + expectDestinationRule: expectedActiveActiveDefaultDR, + }, { name: "Given for a VS host there is DR in cache and region needs to be flipped" + "When performDRPinning func is called" + From 218207e7d023901af4754f14727850fcd8fac6d9 Mon Sep 17 00:00:00 2001 From: Shriram Sharma Date: Thu, 10 Jul 2025 12:17:14 -0700 Subject: [PATCH 2/3] fixed the weights with VS merge --- .../pkg/clusters/virtualservice_routing.go | 7 +- .../clusters/virtualservice_routing_test.go | 739 ++++++++++++++++-- 2 files changed, 689 insertions(+), 57 deletions(-) diff --git a/admiral/pkg/clusters/virtualservice_routing.go b/admiral/pkg/clusters/virtualservice_routing.go index fab5f8ae..e61e4155 100644 --- a/admiral/pkg/clusters/virtualservice_routing.go +++ b/admiral/pkg/clusters/virtualservice_routing.go @@ -2443,12 +2443,7 @@ func adjustWeights( adjustedRDs := make([]*networkingV1Alpha3.HTTPRouteDestination, 0) for _, rd := range routeDestinations { newRD := rd.DeepCopy() - if rd.Weight != 0 { - newRD.Weight = int32((float32(rd.Weight) / 100) * float32(weight)) - adjustedRDs = append(adjustedRDs, newRD) - continue - } - newRD.Weight = weight + newRD.Weight = int32((float32(rd.Weight) / 100) * float32(weight)) adjustedRDs = append(adjustedRDs, newRD) } return adjustedRDs, nil diff --git a/admiral/pkg/clusters/virtualservice_routing_test.go b/admiral/pkg/clusters/virtualservice_routing_test.go index 769c4b57..2f49656f 100644 --- a/admiral/pkg/clusters/virtualservice_routing_test.go +++ b/admiral/pkg/clusters/virtualservice_routing_test.go @@ -6634,9 +6634,50 @@ func TestModifyCustomVSHTTPRoutes(t *testing.T) { }, }, } + vsQALAirFailover := &apiNetworkingV1Alpha3.VirtualService{ + ObjectMeta: metaV1.ObjectMeta{ + Name: "qal-air-failover.stage1.host1.global-incluster-vs", + Labels: map[string]string{common.VSRoutingType: common.VSRoutingTypeInCluster}, + }, + Spec: networkingV1Alpha3.VirtualService{ + ExportTo: []string{"ns1"}, + Hosts: []string{"qal-air-failover.stage1.host1.global"}, + Http: []*networkingV1Alpha3.HTTPRoute{ + { + Name: "qal-air-failover.stage1.host1.global", + Route: []*networkingV1Alpha3.HTTPRouteDestination{ + { + Destination: &networkingV1Alpha3.Destination{ + Host: "qal-air-failover.svc.cluster.local", + Port: &networkingV1Alpha3.PortSelector{ + Number: 8090, + }, + }, + }, + { + Destination: &networkingV1Alpha3.Destination{ + Host: "qal-air-failover.stage1.host1.global", + Port: &networkingV1Alpha3.PortSelector{ + Number: 80, + }, + }, + Weight: 100, + }, + }, + }, + }, + }, + } + + ap := common.AdmiralParams{ + HostnameSuffix: "global", + } + common.ResetSync() + common.InitializeConfig(ap) hostToRouteDestinationCache := istio.NewHostToRouteDestinationCache() hostToRouteDestinationCache.Put(vs) + hostToRouteDestinationCache.Put(vsQALAirFailover) rc := &RemoteController{ ClusterID: "cluster-1", @@ -6880,6 +6921,208 @@ func TestModifyCustomVSHTTPRoutes(t *testing.T) { }, }, }, + { + name: "Given a custom vs with fqdn that exists in the incluster cache which simulates a failover" + + "And modifyCustomVSHTTPRoutes func is called" + + "Then the func should successfully modify the customVS", + customVSRoutes: []*networkingV1Alpha3.HTTPRoute{ + { + Timeout: &duration.Duration{Seconds: 10}, + Route: []*networkingV1Alpha3.HTTPRouteDestination{ + { + Destination: &networkingV1Alpha3.Destination{ + Host: "qal.stage1.host1.global", + Port: &networkingV1Alpha3.PortSelector{ + Number: 80, + }, + }, + Weight: 100, + }, + }, + Match: []*networkingV1Alpha3.HTTPMatchRequest{ + { + Headers: map[string]*networkingV1Alpha3.StringMatch{ + "x-intuit-route-name": { + MatchType: &networkingV1Alpha3.StringMatch_Exact{ + Exact: "Health Check", + }, + }, + }, + }, + }, + }, + }, + inclusterVSRoutes: []*networkingV1Alpha3.HTTPRoute{ + { + Name: "qal.stage1.host1.global", + Route: []*networkingV1Alpha3.HTTPRouteDestination{ + { + Destination: &networkingV1Alpha3.Destination{ + Host: "qal.stage1.svc.cluster.local", + Port: &networkingV1Alpha3.PortSelector{ + Number: 8090, + }, + }, + }, + { + Destination: &networkingV1Alpha3.Destination{ + Host: "qal.stage1.host1.global", + Port: &networkingV1Alpha3.PortSelector{ + Number: 80, + }, + }, + Weight: 100, + }, + }, + Match: []*networkingV1Alpha3.HTTPMatchRequest{ + { + Authority: &networkingV1Alpha3.StringMatch{ + MatchType: &networkingV1Alpha3.StringMatch_Prefix{ + Prefix: "qal.stage1.host1.global", + }, + }, + }, + }, + }, + }, + expectedMergedRoutes: []*networkingV1Alpha3.HTTPRoute{ + { + Timeout: &duration.Duration{Seconds: 10}, + Route: []*networkingV1Alpha3.HTTPRouteDestination{ + { + Destination: &networkingV1Alpha3.Destination{ + Host: "qal.stage1.svc.cluster.local", + Port: &networkingV1Alpha3.PortSelector{ + Number: 8090, + }, + }, + }, + { + Destination: &networkingV1Alpha3.Destination{ + Host: "qal.stage1.host1.global", + Port: &networkingV1Alpha3.PortSelector{ + Number: 80, + }, + }, + Weight: 100, + }, + }, + Match: []*networkingV1Alpha3.HTTPMatchRequest{ + { + Headers: map[string]*networkingV1Alpha3.StringMatch{ + "x-intuit-route-name": { + MatchType: &networkingV1Alpha3.StringMatch_Exact{ + Exact: "Health Check", + }, + }, + }, + }, + }, + }, + }, + }, + { + name: "Given a custom vs with fqdn that exists in the incluster cache which has traffic splits" + + "And modifyCustomVSHTTPRoutes func is called" + + "Then the func should successfully modify the customVS", + customVSRoutes: []*networkingV1Alpha3.HTTPRoute{ + { + Timeout: &duration.Duration{Seconds: 10}, + Route: []*networkingV1Alpha3.HTTPRouteDestination{ + { + Destination: &networkingV1Alpha3.Destination{ + Host: "qal.stage1.host1.global", + Port: &networkingV1Alpha3.PortSelector{ + Number: 80, + }, + }, + Weight: 100, + }, + }, + Match: []*networkingV1Alpha3.HTTPMatchRequest{ + { + Headers: map[string]*networkingV1Alpha3.StringMatch{ + "x-intuit-route-name": { + MatchType: &networkingV1Alpha3.StringMatch_Exact{ + Exact: "Health Check", + }, + }, + }, + }, + }, + }, + }, + inclusterVSRoutes: []*networkingV1Alpha3.HTTPRoute{ + { + Name: "qal.stage1.host1.global", + Route: []*networkingV1Alpha3.HTTPRouteDestination{ + { + Destination: &networkingV1Alpha3.Destination{ + Host: "qal.stage1.svc.cluster.local", + Port: &networkingV1Alpha3.PortSelector{ + Number: 8090, + }, + }, + Weight: 10, + }, + { + Destination: &networkingV1Alpha3.Destination{ + Host: "qal.stage1.host1.global", + Port: &networkingV1Alpha3.PortSelector{ + Number: 80, + }, + }, + Weight: 90, + }, + }, + Match: []*networkingV1Alpha3.HTTPMatchRequest{ + { + Authority: &networkingV1Alpha3.StringMatch{ + MatchType: &networkingV1Alpha3.StringMatch_Prefix{ + Prefix: "qal.stage1.host1.global", + }, + }, + }, + }, + }, + }, + expectedMergedRoutes: []*networkingV1Alpha3.HTTPRoute{ + { + Timeout: &duration.Duration{Seconds: 10}, + Route: []*networkingV1Alpha3.HTTPRouteDestination{ + { + Destination: &networkingV1Alpha3.Destination{ + Host: "qal.stage1.svc.cluster.local", + Port: &networkingV1Alpha3.PortSelector{ + Number: 8090, + }, + }, + Weight: 10, + }, + { + Destination: &networkingV1Alpha3.Destination{ + Host: "qal.stage1.host1.global", + Port: &networkingV1Alpha3.PortSelector{ + Number: 80, + }, + }, + Weight: 90, + }, + }, + Match: []*networkingV1Alpha3.HTTPMatchRequest{ + { + Headers: map[string]*networkingV1Alpha3.StringMatch{ + "x-intuit-route-name": { + MatchType: &networkingV1Alpha3.StringMatch_Exact{ + Exact: "Health Check", + }, + }, + }, + }, + }, + }, + }, + }, { name: "Given a custom vs with fqdn that exists in the incluster cache" + "And also contains a route to a different fqdn that is in the hostToRouteDestinationCache cache" + @@ -7219,7 +7462,149 @@ func TestModifyCustomVSHTTPRoutes(t *testing.T) { Number: 80, }, }, - Weight: 100, + Weight: 100, + }, + }, + Match: []*networkingV1Alpha3.HTTPMatchRequest{ + { + Headers: map[string]*networkingV1Alpha3.StringMatch{ + "x-intuit-route-name": { + MatchType: &networkingV1Alpha3.StringMatch_Exact{ + Exact: "Health Check", + }, + }, + }, + }, + }, + }, + { + Timeout: &duration.Duration{Seconds: 50}, + Route: []*networkingV1Alpha3.HTTPRouteDestination{ + { + Destination: &networkingV1Alpha3.Destination{ + Host: "qal-air.stage1.host1.global", + Port: &networkingV1Alpha3.PortSelector{ + Number: 80, + }, + }, + }, + }, + Match: []*networkingV1Alpha3.HTTPMatchRequest{ + { + Headers: map[string]*networkingV1Alpha3.StringMatch{ + "x-intuit-route-name": { + MatchType: &networkingV1Alpha3.StringMatch_Exact{ + Exact: "qal-air", + }, + }, + }, + }, + }, + }, + { + Timeout: &duration.Duration{Seconds: 30}, + Route: []*networkingV1Alpha3.HTTPRouteDestination{ + { + Destination: &networkingV1Alpha3.Destination{ + Host: "qal.greeting.host1.global", + Port: &networkingV1Alpha3.PortSelector{ + Number: 80, + }, + }, + }, + }, + Match: []*networkingV1Alpha3.HTTPMatchRequest{ + { + Headers: map[string]*networkingV1Alpha3.StringMatch{ + "x-intuit-route-name": { + MatchType: &networkingV1Alpha3.StringMatch_Exact{ + Exact: "qal-greeting", + }, + }, + }, + }, + }, + }, + }, + inclusterVSRoutes: []*networkingV1Alpha3.HTTPRoute{ + { + Name: "qal.stage1.host1.global", + Route: []*networkingV1Alpha3.HTTPRouteDestination{ + { + Destination: &networkingV1Alpha3.Destination{ + Host: "qal.stage1.svc.cluster.local", + Port: &networkingV1Alpha3.PortSelector{ + Number: 8090, + }, + }, + Weight: 90, + }, + { + Destination: &networkingV1Alpha3.Destination{ + Host: "canary.qal.stage1.svc.cluster.local", + Port: &networkingV1Alpha3.PortSelector{ + Number: 8090, + }, + }, + Weight: 10, + }, + }, + Match: []*networkingV1Alpha3.HTTPMatchRequest{ + { + Authority: &networkingV1Alpha3.StringMatch{ + MatchType: &networkingV1Alpha3.StringMatch_Prefix{ + Prefix: "qal.stage1.host1.global", + }, + }, + }, + }, + }, + }, + expectedMergedRoutes: []*networkingV1Alpha3.HTTPRoute{ + { + Timeout: &duration.Duration{Seconds: 10}, + Route: []*networkingV1Alpha3.HTTPRouteDestination{ + { + Destination: &networkingV1Alpha3.Destination{ + Host: "qal.stage1.svc.cluster.local", + Port: &networkingV1Alpha3.PortSelector{ + Number: 8090, + }, + }, + Weight: 90, + }, + { + Destination: &networkingV1Alpha3.Destination{ + Host: "canary.qal.stage1.svc.cluster.local", + Port: &networkingV1Alpha3.PortSelector{ + Number: 8090, + }, + }, + Weight: 10, + }, + }, + Match: []*networkingV1Alpha3.HTTPMatchRequest{ + { + Headers: map[string]*networkingV1Alpha3.StringMatch{ + "x-intuit-route-name": { + MatchType: &networkingV1Alpha3.StringMatch_Exact{ + Exact: "Health Check", + }, + }, + }, + }, + }, + }, + { + Timeout: &duration.Duration{Seconds: 50}, + Route: []*networkingV1Alpha3.HTTPRouteDestination{ + { + Destination: &networkingV1Alpha3.Destination{ + Host: "qal-air.svc.cluster.local", + Port: &networkingV1Alpha3.PortSelector{ + Number: 8090, + }, + }, }, }, Match: []*networkingV1Alpha3.HTTPMatchRequest{ @@ -7227,7 +7612,7 @@ func TestModifyCustomVSHTTPRoutes(t *testing.T) { Headers: map[string]*networkingV1Alpha3.StringMatch{ "x-intuit-route-name": { MatchType: &networkingV1Alpha3.StringMatch_Exact{ - Exact: "Health Check", + Exact: "qal-air", }, }, }, @@ -7235,11 +7620,11 @@ func TestModifyCustomVSHTTPRoutes(t *testing.T) { }, }, { - Timeout: &duration.Duration{Seconds: 50}, + Timeout: &duration.Duration{Seconds: 30}, Route: []*networkingV1Alpha3.HTTPRouteDestination{ { Destination: &networkingV1Alpha3.Destination{ - Host: "qal-air.stage1.host1.global", + Host: "qal.greeting.host1.global", Port: &networkingV1Alpha3.PortSelector{ Number: 80, }, @@ -7251,23 +7636,41 @@ func TestModifyCustomVSHTTPRoutes(t *testing.T) { Headers: map[string]*networkingV1Alpha3.StringMatch{ "x-intuit-route-name": { MatchType: &networkingV1Alpha3.StringMatch_Exact{ - Exact: "qal-air", + Exact: "qal-greeting", }, }, }, }, }, }, + }, + }, + { + name: "Given a custom vs with fqdn that exists in the incluster cache" + + "And also contains a route to a different fqdn that is in the hostToRouteDestinationCache cache" + + "And modifyCustomVSHTTPRoutes func is called" + + "Then the func should successfully modify the customVS", + customVSRoutes: []*networkingV1Alpha3.HTTPRoute{ { - Timeout: &duration.Duration{Seconds: 30}, + Timeout: &duration.Duration{Seconds: 10}, Route: []*networkingV1Alpha3.HTTPRouteDestination{ { Destination: &networkingV1Alpha3.Destination{ - Host: "qal.greeting.host1.global", + Host: "qal.stage1.host1.global", + Port: &networkingV1Alpha3.PortSelector{ + Number: 80, + }, + }, + Weight: 50, + }, + { + Destination: &networkingV1Alpha3.Destination{ + Host: "qal-air-failover.stage1.host1.global", Port: &networkingV1Alpha3.PortSelector{ Number: 80, }, }, + Weight: 50, }, }, Match: []*networkingV1Alpha3.HTTPMatchRequest{ @@ -7275,7 +7678,7 @@ func TestModifyCustomVSHTTPRoutes(t *testing.T) { Headers: map[string]*networkingV1Alpha3.StringMatch{ "x-intuit-route-name": { MatchType: &networkingV1Alpha3.StringMatch_Exact{ - Exact: "qal-greeting", + Exact: "Health Check", }, }, }, @@ -7294,16 +7697,15 @@ func TestModifyCustomVSHTTPRoutes(t *testing.T) { Number: 8090, }, }, - Weight: 90, }, { Destination: &networkingV1Alpha3.Destination{ - Host: "canary.qal.stage1.svc.cluster.local", + Host: "qal.stage1.host1.global", Port: &networkingV1Alpha3.PortSelector{ - Number: 8090, + Number: 80, }, }, - Weight: 10, + Weight: 100, }, }, Match: []*networkingV1Alpha3.HTTPMatchRequest{ @@ -7328,64 +7730,32 @@ func TestModifyCustomVSHTTPRoutes(t *testing.T) { Number: 8090, }, }, - Weight: 90, }, { Destination: &networkingV1Alpha3.Destination{ - Host: "canary.qal.stage1.svc.cluster.local", + Host: "qal.stage1.host1.global", Port: &networkingV1Alpha3.PortSelector{ - Number: 8090, - }, - }, - Weight: 10, - }, - }, - Match: []*networkingV1Alpha3.HTTPMatchRequest{ - { - Headers: map[string]*networkingV1Alpha3.StringMatch{ - "x-intuit-route-name": { - MatchType: &networkingV1Alpha3.StringMatch_Exact{ - Exact: "Health Check", - }, + Number: 80, }, }, + Weight: 50, }, - }, - }, - { - Timeout: &duration.Duration{Seconds: 50}, - Route: []*networkingV1Alpha3.HTTPRouteDestination{ { Destination: &networkingV1Alpha3.Destination{ - Host: "qal-air.svc.cluster.local", + Host: "qal-air-failover.svc.cluster.local", Port: &networkingV1Alpha3.PortSelector{ Number: 8090, }, }, }, - }, - Match: []*networkingV1Alpha3.HTTPMatchRequest{ - { - Headers: map[string]*networkingV1Alpha3.StringMatch{ - "x-intuit-route-name": { - MatchType: &networkingV1Alpha3.StringMatch_Exact{ - Exact: "qal-air", - }, - }, - }, - }, - }, - }, - { - Timeout: &duration.Duration{Seconds: 30}, - Route: []*networkingV1Alpha3.HTTPRouteDestination{ { Destination: &networkingV1Alpha3.Destination{ - Host: "qal.greeting.host1.global", + Host: "qal-air-failover.stage1.host1.global", Port: &networkingV1Alpha3.PortSelector{ Number: 80, }, }, + Weight: 50, }, }, Match: []*networkingV1Alpha3.HTTPMatchRequest{ @@ -7393,7 +7763,7 @@ func TestModifyCustomVSHTTPRoutes(t *testing.T) { Headers: map[string]*networkingV1Alpha3.StringMatch{ "x-intuit-route-name": { MatchType: &networkingV1Alpha3.StringMatch_Exact{ - Exact: "qal-greeting", + Exact: "Health Check", }, }, }, @@ -10068,3 +10438,270 @@ func TestUpdateClientSidecarWithClusterLocalServices(t *testing.T) { } } + +func TestAdjustWeights(t *testing.T) { + + testCases := []struct { + name string + weight int32 + routeDestinations []*networkingV1Alpha3.HTTPRouteDestination + expectedRoutesDestinations []*networkingV1Alpha3.HTTPRouteDestination + }{ + { + name: "Given a weight of 0 and a single routeDestination in the slice" + + "When adjustWeights is called" + + "Then the function should return a slice of HTTPRouteDestination with weight 0", + weight: 0, + routeDestinations: []*networkingV1Alpha3.HTTPRouteDestination{ + { + Destination: &networkingV1Alpha3.Destination{ + Host: "foo.bar.svc.cluster.local", + }, + Weight: 0, + }, + }, + expectedRoutesDestinations: []*networkingV1Alpha3.HTTPRouteDestination{ + { + Destination: &networkingV1Alpha3.Destination{ + Host: "foo.bar.svc.cluster.local", + }, + Weight: 0, + }, + }, + }, + { + name: "Given weight 100 and two .local svc in route destination" + + ", which simulated deployment/rollout migration" + + "When adjustWeights is called" + + "Then the function should return a slice of HTTPRouteDestination with 50/50 weight to each destination", + weight: 100, + routeDestinations: []*networkingV1Alpha3.HTTPRouteDestination{ + { + Destination: &networkingV1Alpha3.Destination{ + Host: "foo.deployment.svc.cluster.local", + }, + Weight: 50, + }, + { + Destination: &networkingV1Alpha3.Destination{ + Host: "foo.rollout.svc.cluster.local", + }, + Weight: 50, + }, + }, + expectedRoutesDestinations: []*networkingV1Alpha3.HTTPRouteDestination{ + { + Destination: &networkingV1Alpha3.Destination{ + Host: "foo.deployment.svc.cluster.local", + }, + Weight: 50, + }, + { + Destination: &networkingV1Alpha3.Destination{ + Host: "foo.rollout.svc.cluster.local", + }, + Weight: 50, + }, + }, + }, + { + name: "Given weight 100 a .local svc and .global with their corresponding weights" + + ", which simulates percentage based traffic routing" + + "When adjustWeights is called" + + "Then the function should return a slice of HTTPRouteDestination with correct traffic split", + weight: 100, + routeDestinations: []*networkingV1Alpha3.HTTPRouteDestination{ + { + Destination: &networkingV1Alpha3.Destination{ + Host: "foo.bar.svc.cluster.local", + }, + Weight: 20, + }, + { + Destination: &networkingV1Alpha3.Destination{ + Host: "foo.bar.global", + }, + Weight: 80, + }, + }, + expectedRoutesDestinations: []*networkingV1Alpha3.HTTPRouteDestination{ + { + Destination: &networkingV1Alpha3.Destination{ + Host: "foo.bar.svc.cluster.local", + }, + Weight: 20, + }, + { + Destination: &networkingV1Alpha3.Destination{ + Host: "foo.bar.global", + }, + Weight: 80, + }, + }, + }, + { + name: "Given weight 50 a .local svc and .global with their corresponding weights" + + ", which simulates percentage based traffic routing" + + "When adjustWeights is called" + + "Then the function should return a slice of HTTPRouteDestination with correct traffic split", + weight: 50, + routeDestinations: []*networkingV1Alpha3.HTTPRouteDestination{ + { + Destination: &networkingV1Alpha3.Destination{ + Host: "foo.bar.svc.cluster.local", + }, + Weight: 20, + }, + { + Destination: &networkingV1Alpha3.Destination{ + Host: "foo.bar.global", + }, + Weight: 80, + }, + }, + expectedRoutesDestinations: []*networkingV1Alpha3.HTTPRouteDestination{ + { + Destination: &networkingV1Alpha3.Destination{ + Host: "foo.bar.svc.cluster.local", + }, + Weight: 10, + }, + { + Destination: &networkingV1Alpha3.Destination{ + Host: "foo.bar.global", + }, + Weight: 40, + }, + }, + }, + { + name: "Given weight 100 a .local svc and .global in routes with 0 and 100 weight" + + ", which simulates 100% failover" + + "When adjustWeights is called" + + "Then the function should return a slice of HTTPRouteDestination with correct traffic split", + weight: 100, + routeDestinations: []*networkingV1Alpha3.HTTPRouteDestination{ + { + Destination: &networkingV1Alpha3.Destination{ + Host: "foo.bar.svc.cluster.local", + }, + Weight: 0, + }, + { + Destination: &networkingV1Alpha3.Destination{ + Host: "foo.bar.global", + }, + Weight: 100, + }, + }, + expectedRoutesDestinations: []*networkingV1Alpha3.HTTPRouteDestination{ + { + Destination: &networkingV1Alpha3.Destination{ + Host: "foo.bar.svc.cluster.local", + }, + Weight: 0, + }, + { + Destination: &networkingV1Alpha3.Destination{ + Host: "foo.bar.global", + }, + Weight: 100, + }, + }, + }, + { + name: "Given weight 100 a .local svc and .global in routes with 0 and 100 weight" + + ", which simulates 100% failover" + + "When adjustWeights is called" + + "Then the function should return a slice of HTTPRouteDestination with correct traffic split", + weight: 100, + routeDestinations: []*networkingV1Alpha3.HTTPRouteDestination{ + { + Destination: &networkingV1Alpha3.Destination{ + Host: "foo.bar.svc.cluster.local", + }, + }, + { + Destination: &networkingV1Alpha3.Destination{ + Host: "foo.bar.global", + }, + Weight: 100, + }, + }, + expectedRoutesDestinations: []*networkingV1Alpha3.HTTPRouteDestination{ + { + Destination: &networkingV1Alpha3.Destination{ + Host: "foo.bar.svc.cluster.local", + }, + }, + { + Destination: &networkingV1Alpha3.Destination{ + Host: "foo.bar.global", + }, + Weight: 100, + }, + }, + }, + { + name: "Given weight 100 and canary and regular .local svc with their respective traffic split" + + "When adjustWeights is called" + + "Then the function should return a slice of HTTPRouteDestination with correct traffic split", + weight: 100, + routeDestinations: []*networkingV1Alpha3.HTTPRouteDestination{ + { + Destination: &networkingV1Alpha3.Destination{ + Host: "canary.foo.bar.svc.cluster.local", + }, + Weight: 25, + }, + { + Destination: &networkingV1Alpha3.Destination{ + Host: "root.foo.bar.svc.cluster.local", + }, + Weight: 25, + }, + { + Destination: &networkingV1Alpha3.Destination{ + Host: "foo.bar.global", + }, + Weight: 50, + }, + }, + expectedRoutesDestinations: []*networkingV1Alpha3.HTTPRouteDestination{ + { + Destination: &networkingV1Alpha3.Destination{ + Host: "canary.foo.bar.svc.cluster.local", + }, + Weight: 25, + }, + { + Destination: &networkingV1Alpha3.Destination{ + Host: "root.foo.bar.svc.cluster.local", + }, + Weight: 25, + }, + { + Destination: &networkingV1Alpha3.Destination{ + Host: "foo.bar.global", + }, + Weight: 50, + }, + }, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + actual, err := adjustWeights(tc.routeDestinations, tc.weight) + if err != nil { + assert.Fail(t, err.Error()) + } + assert.Equal(t, len(tc.expectedRoutesDestinations), len(actual)) + for i, expected := range tc.expectedRoutesDestinations { + assert.Equal(t, expected.Destination.Host, actual[i].Destination.Host) + assert.Equal(t, expected.Weight, actual[i].Weight) + } + }) + } + +} From 8f16127f36c0e04604dffbd3846e4cabdc38b934 Mon Sep 17 00:00:00 2001 From: Shriram Sharma Date: Thu, 10 Jul 2025 14:40:29 -0700 Subject: [PATCH 3/3] fixed the weights with VS merge --- admiral/pkg/clusters/virtualservice_routing.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/admiral/pkg/clusters/virtualservice_routing.go b/admiral/pkg/clusters/virtualservice_routing.go index e61e4155..b20a8927 100644 --- a/admiral/pkg/clusters/virtualservice_routing.go +++ b/admiral/pkg/clusters/virtualservice_routing.go @@ -2441,6 +2441,12 @@ func adjustWeights( return nil, fmt.Errorf("slice of HTTPRouteDestination is nil") } adjustedRDs := make([]*networkingV1Alpha3.HTTPRouteDestination, 0) + if len(routeDestinations) == 1 { + newRD := routeDestinations[0].DeepCopy() + newRD.Weight = weight + adjustedRDs = append(adjustedRDs, newRD) + return adjustedRDs, nil + } for _, rd := range routeDestinations { newRD := rd.DeepCopy() newRD.Weight = int32((float32(rd.Weight) / 100) * float32(weight))