diff --git a/pkg/catalog/inbound_traffic_policies.go b/pkg/catalog/inbound_traffic_policies.go index 1d8d2f78fc..4c30cdfcf5 100644 --- a/pkg/catalog/inbound_traffic_policies.go +++ b/pkg/catalog/inbound_traffic_policies.go @@ -26,16 +26,18 @@ const ( // 2. for the given service account and upstream services from SMI Traffic Target and Traffic Split // Note: ServiceIdentity must be in the format "name.namespace" [https://github.com/openservicemesh/osm/issues/3188] func (mc *MeshCatalog) ListInboundTrafficPolicies(upstreamIdentity identity.ServiceIdentity, upstreamServices []service.MeshService) []*trafficpolicy.InboundTrafficPolicy { + inboundPoliciesFromSplits := mc.listInboundPoliciesForTrafficSplits(upstreamIdentity, upstreamServices) + if mc.configurator.IsPermissiveTrafficPolicyMode() { var inboundPolicies []*trafficpolicy.InboundTrafficPolicy for _, svc := range upstreamServices { inboundPolicies = trafficpolicy.MergeInboundPolicies(DisallowPartialHostnamesMatch, inboundPolicies, mc.buildInboundPermissiveModePolicies(svc)...) } + inboundPolicies = trafficpolicy.MergeInboundPolicies(DisallowPartialHostnamesMatch, inboundPolicies, inboundPoliciesFromSplits...) return inboundPolicies } inbound := mc.listInboundPoliciesFromTrafficTargets(upstreamIdentity, upstreamServices) - inboundPoliciesFromSplits := mc.listInboundPoliciesForTrafficSplits(upstreamIdentity, upstreamServices) inbound = trafficpolicy.MergeInboundPolicies(AllowPartialHostnamesMatch, inbound, inboundPoliciesFromSplits...) return inbound } @@ -73,6 +75,35 @@ func (mc *MeshCatalog) listInboundPoliciesForTrafficSplits(upstreamIdentity iden upstreamServiceAccount := upstreamIdentity.ToK8sServiceAccount() var inboundPolicies []*trafficpolicy.InboundTrafficPolicy + if mc.configurator.IsPermissiveTrafficPolicyMode() { + for _, upstreamSvc := range upstreamServices { + //check if the upstream service belong to a traffic split + if !mc.isTrafficSplitBackendService(upstreamSvc) { + continue + } + + apexServices := mc.getApexServicesForBackendService(upstreamSvc) + for _, apexService := range apexServices { + // build an inbound policy for every apex service + locality := service.LocalCluster + if apexService.Namespace == upstreamServiceAccount.Namespace { + locality = service.LocalNS + } + hostnames, err := mc.GetServiceHostnames(apexService, locality) + if err != nil { + log.Error().Err(err).Str(errcode.Kind, errcode.GetErrCodeWithMetric(errcode.ErrServiceHostnames)). + Msgf("Error getting service hostnames for apex service %v", apexService) + continue + } + servicePolicy := trafficpolicy.NewInboundTrafficPolicy(apexService.FQDN(), hostnames) + weightedCluster := getDefaultWeightedClusterForService(upstreamSvc) + servicePolicy.AddRule(*trafficpolicy.NewRouteWeightedCluster(trafficpolicy.WildCardRouteMatch, []service.WeightedCluster{weightedCluster}), identity.WildcardServiceIdentity) + inboundPolicies = trafficpolicy.MergeInboundPolicies(AllowPartialHostnamesMatch, inboundPolicies, servicePolicy) + } + } + return inboundPolicies + } + for _, t := range mc.meshSpec.ListTrafficTargets() { // loop through all traffic targets if !isValidTrafficTarget(t) { continue diff --git a/pkg/catalog/inbound_traffic_policies_test.go b/pkg/catalog/inbound_traffic_policies_test.go index 5c570e109c..83860a5f57 100644 --- a/pkg/catalog/inbound_traffic_policies_test.go +++ b/pkg/catalog/inbound_traffic_policies_test.go @@ -472,6 +472,90 @@ func TestListInboundTrafficPolicies(t *testing.T) { }, permissiveMode: true, }, + { + name: "permissive mode with traffic split", + downstreamSA: tests.BookbuyerServiceIdentity, + upstreamSA: tests.BookstoreV2ServiceIdentity, + upstreamServices: []service.MeshService{tests.BookstoreV2Service}, + meshServices: []service.MeshService{tests.BookbuyerService, tests.BookstoreV1Service, tests.BookstoreV2Service, tests.BookstoreApexService}, + meshServiceAccounts: []identity.K8sServiceAccount{tests.BookbuyerServiceAccount, tests.BookstoreV2ServiceAccount}, + trafficSpec: spec.HTTPRouteGroup{}, + trafficSplit: split.TrafficSplit{ + ObjectMeta: v1.ObjectMeta{ + Namespace: "default", + }, + Spec: split.TrafficSplitSpec{ + Service: "bookstore-apex", + Backends: []split.TrafficSplitBackend{ + { + Service: tests.BookstoreV1ServiceName, + Weight: tests.Weight90, + }, + { + Service: tests.BookstoreV2ServiceName, + Weight: tests.Weight10, + }, + }, + }, + }, + expectedInboundPolicies: []*trafficpolicy.InboundTrafficPolicy{ + { + Name: "bookstore-v2.default.svc.cluster.local", + Hostnames: []string{ + "bookstore-v2", + "bookstore-v2.default", + "bookstore-v2.default.svc", + "bookstore-v2.default.svc.cluster", + "bookstore-v2.default.svc.cluster.local", + "bookstore-v2:8888", + "bookstore-v2.default:8888", + "bookstore-v2.default.svc:8888", + "bookstore-v2.default.svc.cluster:8888", + "bookstore-v2.default.svc.cluster.local:8888", + }, + Rules: []*trafficpolicy.Rule{ + { + Route: trafficpolicy.RouteWeightedClusters{ + HTTPRouteMatch: tests.WildCardRouteMatch, + WeightedClusters: mapset.NewSet(service.WeightedCluster{ + ClusterName: "default/bookstore-v2", + Weight: 100, + }), + }, + AllowedServiceIdentities: mapset.NewSet(identity.WildcardServiceIdentity), + }, + }, + }, + { + Name: "bookstore-apex.default.svc.cluster.local", + Hostnames: []string{ + "bookstore-apex", + "bookstore-apex.default", + "bookstore-apex.default.svc", + "bookstore-apex.default.svc.cluster", + "bookstore-apex.default.svc.cluster.local", + "bookstore-apex:8888", + "bookstore-apex.default:8888", + "bookstore-apex.default.svc:8888", + "bookstore-apex.default.svc.cluster:8888", + "bookstore-apex.default.svc.cluster.local:8888", + }, + Rules: []*trafficpolicy.Rule{ + { + Route: trafficpolicy.RouteWeightedClusters{ + HTTPRouteMatch: tests.WildCardRouteMatch, + WeightedClusters: mapset.NewSet(service.WeightedCluster{ + ClusterName: "default/bookstore-v2", + Weight: 100, + }), + }, + AllowedServiceIdentities: mapset.NewSet(identity.WildcardServiceIdentity), + }, + }, + }, + }, + permissiveMode: true, + }, } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { @@ -501,6 +585,8 @@ func TestListInboundTrafficPolicies(t *testing.T) { mockEndpointProvider.EXPECT().GetID().Return("fake").AnyTimes() + mockMeshSpec.EXPECT().ListTrafficSplits().Return([]*split.TrafficSplit{&tc.trafficSplit}).AnyTimes() + if tc.permissiveMode { var serviceAccounts []*corev1.ServiceAccount for _, sa := range tc.meshServiceAccounts { @@ -511,7 +597,6 @@ func TestListInboundTrafficPolicies(t *testing.T) { mockKubeController.EXPECT().ListServiceAccounts().Return(serviceAccounts).AnyTimes() } else { mockMeshSpec.EXPECT().ListHTTPTrafficSpecs().Return([]*spec.HTTPRouteGroup{&tc.trafficSpec}).AnyTimes() - mockMeshSpec.EXPECT().ListTrafficSplits().Return([]*split.TrafficSplit{&tc.trafficSplit}).AnyTimes() trafficTarget := tests.NewSMITrafficTarget(tc.downstreamSA, tc.upstreamSA) mockMeshSpec.EXPECT().ListTrafficTargets().Return([]*access.TrafficTarget{&trafficTarget}).AnyTimes() } @@ -541,6 +626,7 @@ func TestListInboundPoliciesForTrafficSplits(t *testing.T) { testCases := []struct { name string + permissiveMode bool downstreamSA identity.ServiceIdentity upstreamSA identity.ServiceIdentity upstreamServices []service.MeshService @@ -551,7 +637,8 @@ func TestListInboundPoliciesForTrafficSplits(t *testing.T) { }{ // TODO(draychev): use ServiceIdentity in the rest of the tests [https://github.com/openservicemesh/osm/issues/2218] { - name: "inbound policies in same namespaces, without traffic split", + name: "inbound policies in same namespaces, without traffic split", + permissiveMode: false, downstreamSA: identity.K8sServiceAccount{ Name: "bookbuyer", Namespace: "default", @@ -603,7 +690,8 @@ func TestListInboundPoliciesForTrafficSplits(t *testing.T) { expectedInboundPolicies: []*trafficpolicy.InboundTrafficPolicy{}, }, { - name: "inbound policies in same namespaces, with traffic split", + name: "inbound policies in same namespaces, with traffic split", + permissiveMode: false, downstreamSA: identity.K8sServiceAccount{ Name: "bookbuyer", Namespace: "default", @@ -719,7 +807,8 @@ func TestListInboundPoliciesForTrafficSplits(t *testing.T) { }, }, { - name: "inbound policies in same namespaces, with traffic split with namespaced root service", + name: "inbound policies in same namespaces, with traffic split with namespaced root service", + permissiveMode: false, downstreamSA: identity.K8sServiceAccount{ Name: "bookbuyer", Namespace: "default", @@ -835,7 +924,8 @@ func TestListInboundPoliciesForTrafficSplits(t *testing.T) { }, }, { - name: "inbound policies in same namespaces: with traffic split (namespaced root service) and traffic target (having host header)", + name: "inbound policies in same namespaces: with traffic split (namespaced root service) and traffic target (having host header)", + permissiveMode: false, downstreamSA: identity.K8sServiceAccount{ Name: "bookbuyer", Namespace: "default", @@ -957,6 +1047,77 @@ func TestListInboundPoliciesForTrafficSplits(t *testing.T) { }, }, }, + { + name: "inbound policies with traffic split in permissive mode", + permissiveMode: true, + downstreamSA: identity.K8sServiceAccount{ + Name: "bookbuyer", + Namespace: "default", + }.ToServiceIdentity(), + upstreamSA: identity.K8sServiceAccount{ + Name: "bookstore", + Namespace: "default", + }.ToServiceIdentity(), + upstreamServices: []service.MeshService{{ + Name: "bookstore", + Namespace: "default", + }}, + meshServices: []service.MeshService{{ + Name: "bookstore", + Namespace: "default", + }, { + Name: "bookstore-apex", + Namespace: "default", + }}, + trafficSpec: spec.HTTPRouteGroup{}, + trafficSplit: split.TrafficSplit{ + ObjectMeta: v1.ObjectMeta{ + Namespace: "default", + }, + Spec: split.TrafficSplitSpec{ + Service: "bookstore-apex", + Backends: []split.TrafficSplitBackend{ + { + Service: "bookstore", + Weight: tests.Weight90, + }, + { + Service: tests.BookstoreV2ServiceName, + Weight: tests.Weight10, + }, + }, + }, + }, + expectedInboundPolicies: []*trafficpolicy.InboundTrafficPolicy{ + { + Name: "bookstore-apex.default.svc.cluster.local", + Hostnames: []string{ + "bookstore-apex", + "bookstore-apex.default", + "bookstore-apex.default.svc", + "bookstore-apex.default.svc.cluster", + "bookstore-apex.default.svc.cluster.local", + "bookstore-apex:8888", + "bookstore-apex.default:8888", + "bookstore-apex.default.svc:8888", + "bookstore-apex.default.svc.cluster:8888", + "bookstore-apex.default.svc.cluster.local:8888", + }, + Rules: []*trafficpolicy.Rule{ + { + Route: trafficpolicy.RouteWeightedClusters{ + HTTPRouteMatch: tests.WildCardRouteMatch, + WeightedClusters: mapset.NewSet(service.WeightedCluster{ + ClusterName: "default/bookstore", + Weight: 100, + }), + }, + AllowedServiceIdentities: mapset.NewSet(identity.WildcardServiceIdentity), + }, + }, + }, + }, + }, } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { @@ -967,12 +1128,14 @@ func TestListInboundPoliciesForTrafficSplits(t *testing.T) { mockMeshSpec := smi.NewMockMeshSpec(mockCtrl) mockEndpointProvider := endpoint.NewMockProvider(mockCtrl) mockServiceProvider := service.NewMockProvider(mockCtrl) + mockConfigurator := configurator.NewMockConfigurator(mockCtrl) mc := MeshCatalog{ kubeController: mockKubeController, meshSpec: mockMeshSpec, endpointsProviders: []endpoint.Provider{mockEndpointProvider}, serviceProviders: []service.Provider{mockServiceProvider}, + configurator: mockConfigurator, } for _, meshSvc := range tc.meshServices { @@ -984,9 +1147,12 @@ func TestListInboundPoliciesForTrafficSplits(t *testing.T) { mockMeshSpec.EXPECT().ListTrafficSplits().Return([]*split.TrafficSplit{&tc.trafficSplit}).AnyTimes() mockEndpointProvider.EXPECT().GetID().Return("fake").AnyTimes() - trafficTarget := tests.NewSMITrafficTarget(tc.downstreamSA, tc.upstreamSA) - mockMeshSpec.EXPECT().ListTrafficTargets().Return([]*access.TrafficTarget{&trafficTarget}).AnyTimes() - + if tc.permissiveMode { + mockMeshSpec.EXPECT().ListTrafficTargets().Return([]*access.TrafficTarget{}).AnyTimes() + } else { + trafficTarget := tests.NewSMITrafficTarget(tc.downstreamSA, tc.upstreamSA) + mockMeshSpec.EXPECT().ListTrafficTargets().Return([]*access.TrafficTarget{&trafficTarget}).AnyTimes() + } for _, ms := range tc.meshServices { locality := service.LocalCluster if ms.Namespace == tc.downstreamSA.ToK8sServiceAccount().Namespace { @@ -998,6 +1164,7 @@ func TestListInboundPoliciesForTrafficSplits(t *testing.T) { } } } + mockConfigurator.EXPECT().IsPermissiveTrafficPolicyMode().Return(tc.permissiveMode).AnyTimes() actual := mc.listInboundPoliciesForTrafficSplits(tc.upstreamSA, tc.upstreamServices) assert.ElementsMatch(tc.expectedInboundPolicies, actual) diff --git a/pkg/catalog/outbound_traffic_policies.go b/pkg/catalog/outbound_traffic_policies.go index 42b79b6711..66d75d23b2 100644 --- a/pkg/catalog/outbound_traffic_policies.go +++ b/pkg/catalog/outbound_traffic_policies.go @@ -41,18 +41,17 @@ var ( // Note: ServiceIdentity must be in the format "name.namespace" [https://github.com/openservicemesh/osm/issues/3188] func (mc *MeshCatalog) ListOutboundTrafficPolicies(downstreamIdentity identity.ServiceIdentity) []*trafficpolicy.OutboundTrafficPolicy { downstreamServiceAccount := downstreamIdentity.ToK8sServiceAccount() - if mc.configurator.IsPermissiveTrafficPolicyMode() { - var outboundPolicies []*trafficpolicy.OutboundTrafficPolicy - mergedPolicies := trafficpolicy.MergeOutboundPolicies(DisallowPartialHostnamesMatch, outboundPolicies, mc.buildOutboundPermissiveModePolicies(downstreamServiceAccount.Namespace)...) - outboundPolicies = mergedPolicies - return outboundPolicies - } - outbound := mc.listOutboundPoliciesForTrafficTargets(downstreamIdentity) outboundPoliciesFromSplits := mc.listOutboundTrafficPoliciesForTrafficSplits(downstreamServiceAccount.Namespace) - outbound = trafficpolicy.MergeOutboundPolicies(AllowPartialHostnamesMatch, outbound, outboundPoliciesFromSplits...) - return outbound + if mc.configurator.IsPermissiveTrafficPolicyMode() { + permissiveOutboundPolicies := mc.buildOutboundPermissiveModePolicies(downstreamServiceAccount.Namespace) + //TODO: I don't see a reason to disallow partial hostname match. Will follow up on this. + return trafficpolicy.MergeOutboundPolicies(DisallowPartialHostnamesMatch, trafficpolicy.OverwriteWeightedClusters, permissiveOutboundPolicies, outboundPoliciesFromSplits...) + } + + allowedOutboundPolicies := mc.listOutboundPoliciesForTrafficTargets(downstreamIdentity) + return trafficpolicy.MergeOutboundPolicies(AllowPartialHostnamesMatch, trafficpolicy.OverwriteWeightedClusters, allowedOutboundPolicies, outboundPoliciesFromSplits...) } // listOutboundPoliciesForTrafficTargets loops through all SMI Traffic Target resources and returns outbound traffic policies @@ -70,7 +69,7 @@ func (mc *MeshCatalog) listOutboundPoliciesForTrafficTargets(downstreamIdentity for _, source := range t.Spec.Sources { // TODO(draychev): must check for the correct type of ServiceIdentity as well if source.Name == downstreamServiceAccount.Name && source.Namespace == downstreamServiceAccount.Namespace { // found outbound - outboundPolicies = trafficpolicy.MergeOutboundPolicies(AllowPartialHostnamesMatch, outboundPolicies, mc.buildOutboundPolicies(downstreamIdentity, t)...) + outboundPolicies = trafficpolicy.MergeOutboundPolicies(AllowPartialHostnamesMatch, trafficpolicy.UnionWeightedClusters, outboundPolicies, mc.buildOutboundPolicies(downstreamIdentity, t)...) break } } @@ -262,7 +261,7 @@ func (mc *MeshCatalog) buildOutboundPolicies(sourceServiceIdentity identity.Serv Msgf("Error adding Route to outbound policy for source %s/%s and destination %s/%s with host header %s", source.Namespace, source.Name, destService.Namespace, destService.Name, routeMatch.Headers[hostHeaderKey]) continue } - outboundPolicies = trafficpolicy.MergeOutboundPolicies(AllowPartialHostnamesMatch, outboundPolicies, policyWithHostHeader) + outboundPolicies = trafficpolicy.MergeOutboundPolicies(AllowPartialHostnamesMatch, trafficpolicy.UnionWeightedClusters, outboundPolicies, policyWithHostHeader) } else { needWildCardRoute = true } @@ -275,7 +274,7 @@ func (mc *MeshCatalog) buildOutboundPolicies(sourceServiceIdentity identity.Serv } } - outboundPolicies = trafficpolicy.MergeOutboundPolicies(AllowPartialHostnamesMatch, outboundPolicies, policy) + outboundPolicies = trafficpolicy.MergeOutboundPolicies(AllowPartialHostnamesMatch, trafficpolicy.UnionWeightedClusters, outboundPolicies, policy) } return outboundPolicies } diff --git a/pkg/catalog/outbound_traffic_policies_test.go b/pkg/catalog/outbound_traffic_policies_test.go index 248d092f00..a937d692f0 100644 --- a/pkg/catalog/outbound_traffic_policies_test.go +++ b/pkg/catalog/outbound_traffic_policies_test.go @@ -332,6 +332,95 @@ func TestListOutboundTrafficPolicies(t *testing.T) { }, permissiveMode: true, }, + { + name: "permissive mode with traffic splits", + downstreamSA: tests.BookbuyerServiceIdentity, + apexMeshServices: []service.MeshService{tests.BookstoreApexService}, + meshServices: []service.MeshService{tests.BookstoreV1Service, tests.BookstoreV2Service, tests.BookbuyerService}, + meshServiceAccounts: []identity.K8sServiceAccount{tests.BookbuyerServiceAccount, tests.BookstoreServiceAccount}, + trafficsplits: []*split.TrafficSplit{&tests.TrafficSplit}, + traffictargets: []*access.TrafficTarget{}, + trafficspecs: []*spec.HTTPRouteGroup{}, + expectedOutbound: []*trafficpolicy.OutboundTrafficPolicy{ + { + Name: "bookstore-apex.default.svc.cluster.local", + Hostnames: tests.BookstoreApexHostnames, + Routes: []*trafficpolicy.RouteWeightedClusters{ + { + HTTPRouteMatch: tests.WildCardRouteMatch, + WeightedClusters: mapset.NewSetFromSlice([]interface{}{ + service.WeightedCluster{ClusterName: "default/bookstore-v1", Weight: 90}, + service.WeightedCluster{ClusterName: "default/bookstore-v2", Weight: 10}, + }), + }, + }, + }, + { + Name: "bookstore-v1.default.svc.cluster.local", + Hostnames: []string{ + "bookstore-v1", + "bookstore-v1.default", + "bookstore-v1.default.svc", + "bookstore-v1.default.svc.cluster", + "bookstore-v1.default.svc.cluster.local", + "bookstore-v1:8888", + "bookstore-v1.default:8888", + "bookstore-v1.default.svc:8888", + "bookstore-v1.default.svc.cluster:8888", + "bookstore-v1.default.svc.cluster.local:8888", + }, + Routes: []*trafficpolicy.RouteWeightedClusters{ + { + HTTPRouteMatch: tests.WildCardRouteMatch, + WeightedClusters: mapset.NewSet(tests.BookstoreV1DefaultWeightedCluster), + }, + }, + }, + { + Name: "bookstore-v2.default.svc.cluster.local", + Hostnames: []string{ + "bookstore-v2", + "bookstore-v2.default", + "bookstore-v2.default.svc", + "bookstore-v2.default.svc.cluster", + "bookstore-v2.default.svc.cluster.local", + "bookstore-v2:8888", + "bookstore-v2.default:8888", + "bookstore-v2.default.svc:8888", + "bookstore-v2.default.svc.cluster:8888", + "bookstore-v2.default.svc.cluster.local:8888", + }, + Routes: []*trafficpolicy.RouteWeightedClusters{ + { + HTTPRouteMatch: tests.WildCardRouteMatch, + WeightedClusters: mapset.NewSet(tests.BookstoreV2DefaultWeightedCluster), + }, + }, + }, + { + Name: "bookbuyer.default.svc.cluster.local", + Hostnames: []string{ + "bookbuyer", + "bookbuyer.default", + "bookbuyer.default.svc", + "bookbuyer.default.svc.cluster", + "bookbuyer.default.svc.cluster.local", + "bookbuyer:8888", + "bookbuyer.default:8888", + "bookbuyer.default.svc:8888", + "bookbuyer.default.svc.cluster:8888", + "bookbuyer.default.svc.cluster.local:8888", + }, + Routes: []*trafficpolicy.RouteWeightedClusters{ + { + HTTPRouteMatch: tests.WildCardRouteMatch, + WeightedClusters: mapset.NewSet(tests.BookbuyerDefaultWeightedCluster), + }, + }, + }, + }, + permissiveMode: true, + }, } for _, tc := range testCases { @@ -368,6 +457,7 @@ func TestListOutboundTrafficPolicies(t *testing.T) { } mockKubeController.EXPECT().ListServices().Return(services).AnyTimes() mockKubeController.EXPECT().ListServiceAccounts().Return(serviceAccounts).AnyTimes() + mockMeshSpec.EXPECT().ListTrafficSplits().Return(tc.trafficsplits).AnyTimes() } else { mockMeshSpec.EXPECT().ListTrafficSplits().Return(tc.trafficsplits).AnyTimes() mockMeshSpec.EXPECT().ListTrafficTargets().Return(tc.traffictargets).AnyTimes() @@ -1114,6 +1204,7 @@ func TestListOutboundPoliciesForTrafficTargets(t *testing.T) { trafficsplits: []*split.TrafficSplit{}, trafficspecs: []*spec.HTTPRouteGroup{&tests.HTTPRouteGroupWithHost}, enableRetryPolicy: v1alpha1.FeatureFlags{EnableRetryPolicy: true}, + expectedOutbound: []*trafficpolicy.OutboundTrafficPolicy{ { Name: "bookstore-v1.default.svc.cluster.local", diff --git a/pkg/envoy/cds/cluster.go b/pkg/envoy/cds/cluster.go index 8a9ddeb223..86858f1107 100644 --- a/pkg/envoy/cds/cluster.go +++ b/pkg/envoy/cds/cluster.go @@ -81,16 +81,10 @@ func getUpstreamServiceCluster(downstreamIdentity identity.ServiceIdentity, upst }, } - if o.permissive { - // Since no traffic policies exist with permissive mode, rely on cluster provided service discovery. - remoteCluster.ClusterDiscoveryType = &xds_cluster.Cluster_Type{Type: xds_cluster.Cluster_ORIGINAL_DST} - remoteCluster.LbPolicy = xds_cluster.Cluster_CLUSTER_PROVIDED - } else { - // Configure service discovery based on traffic policies - remoteCluster.ClusterDiscoveryType = &xds_cluster.Cluster_Type{Type: xds_cluster.Cluster_EDS} - remoteCluster.EdsClusterConfig = &xds_cluster.Cluster_EdsClusterConfig{EdsConfig: envoy.GetADSConfigSource()} - remoteCluster.LbPolicy = xds_cluster.Cluster_ROUND_ROBIN - } + // Configure service discovery based on traffic policies + remoteCluster.ClusterDiscoveryType = &xds_cluster.Cluster_Type{Type: xds_cluster.Cluster_EDS} + remoteCluster.EdsClusterConfig = &xds_cluster.Cluster_EdsClusterConfig{EdsConfig: envoy.GetADSConfigSource()} + remoteCluster.LbPolicy = xds_cluster.Cluster_ROUND_ROBIN if o.withActiveHealthChecks { enableHealthChecksOnCluster(remoteCluster, upstreamSvc) diff --git a/pkg/envoy/cds/cluster_test.go b/pkg/envoy/cds/cluster_test.go index ddaf225bb3..154a69c741 100644 --- a/pkg/envoy/cds/cluster_test.go +++ b/pkg/envoy/cds/cluster_test.go @@ -27,37 +27,26 @@ func TestGetUpstreamServiceCluster(t *testing.T) { testCases := []struct { name string - permissiveMode bool expectedClusterType xds_cluster.Cluster_DiscoveryType expectedLbPolicy xds_cluster.Cluster_LbPolicy addHealthCheck bool }{ { name: "Returns an EDS based cluster when permissive mode is disabled", - permissiveMode: false, expectedClusterType: xds_cluster.Cluster_EDS, expectedLbPolicy: xds_cluster.Cluster_ROUND_ROBIN, addHealthCheck: false, }, - { - name: "Returns an Original Destination based cluster when permissive mode is enabled", - permissiveMode: true, - expectedClusterType: xds_cluster.Cluster_ORIGINAL_DST, - expectedLbPolicy: xds_cluster.Cluster_CLUSTER_PROVIDED, - addHealthCheck: false, - }, { name: "Adds health checks when configured", - permissiveMode: true, - expectedClusterType: xds_cluster.Cluster_ORIGINAL_DST, - expectedLbPolicy: xds_cluster.Cluster_CLUSTER_PROVIDED, + expectedClusterType: xds_cluster.Cluster_EDS, + expectedLbPolicy: xds_cluster.Cluster_ROUND_ROBIN, addHealthCheck: true, }, { name: "Does not add health checks when not configured", - permissiveMode: true, - expectedClusterType: xds_cluster.Cluster_ORIGINAL_DST, - expectedLbPolicy: xds_cluster.Cluster_CLUSTER_PROVIDED, + expectedClusterType: xds_cluster.Cluster_EDS, + expectedLbPolicy: xds_cluster.Cluster_ROUND_ROBIN, addHealthCheck: false, }, } @@ -67,9 +56,6 @@ func TestGetUpstreamServiceCluster(t *testing.T) { assert := tassert.New(t) opts := []clusterOption{} - if tc.permissiveMode { - opts = append(opts, permissive) - } if tc.addHealthCheck { opts = append(opts, withActiveHealthChecks) } diff --git a/pkg/envoy/rds/response.go b/pkg/envoy/rds/response.go index f254db688c..47635ae89f 100644 --- a/pkg/envoy/rds/response.go +++ b/pkg/envoy/rds/response.go @@ -30,6 +30,7 @@ func NewResponse(cataloger catalog.MeshCataloger, proxy *envoy.Proxy, discoveryR } services, err := proxyRegistry.ListProxyServices(proxy) + if err != nil { log.Error().Err(err).Str(errcode.Kind, errcode.GetErrCodeWithMetric(errcode.ErrFetchingServiceList)). Msgf("Error looking up services for Envoy with serial number=%q", proxy.GetCertificateSerialNumber()) diff --git a/pkg/trafficpolicy/trafficpolicy.go b/pkg/trafficpolicy/trafficpolicy.go index 61077dee95..401c57fad4 100644 --- a/pkg/trafficpolicy/trafficpolicy.go +++ b/pkg/trafficpolicy/trafficpolicy.go @@ -13,6 +13,16 @@ import ( "github.com/openservicemesh/osm/pkg/service" ) +// weightedClusterStrategyType is a type to signify the strategy associated with merging weighted clusters +type weightedClusterStrategyType int + +const ( + // UnionWeightedClusters is the weightedClusterStrategyType indicating union weighted clusters + UnionWeightedClusters weightedClusterStrategyType = 0 + // OverwriteWeightedClusters is the weightedClusterStrategyType indicating union weighted clusters + OverwriteWeightedClusters weightedClusterStrategyType = 1 +) + // WildCardRouteMatch represents a wildcard HTTP route match condition var WildCardRouteMatch HTTPRouteMatch = HTTPRouteMatch{ Path: constants.RegexMatchAll, @@ -142,14 +152,14 @@ func MergeInboundPolicies(allowPartialHostnamesMatch bool, original []*InboundTr // when a policy having its hostnames from a host header needs to be merged with other outbound policies // This is because there will be a single hostname (from the host header) and there is a possibility that this hostname is part of an existing traffic policy // hence the rules need to be merged -func MergeOutboundPolicies(allowPartialHostnamesMatch bool, original []*OutboundTrafficPolicy, latest ...*OutboundTrafficPolicy) []*OutboundTrafficPolicy { +func MergeOutboundPolicies(allowPartialHostnamesMatch bool, weightedClusterStrategy weightedClusterStrategyType, original []*OutboundTrafficPolicy, latest ...*OutboundTrafficPolicy) []*OutboundTrafficPolicy { for _, l := range latest { foundHostnames := false for _, or := range original { if !allowPartialHostnamesMatch { if reflect.DeepEqual(or.Hostnames, l.Hostnames) { foundHostnames = true - mergedRoutes := mergeRoutesWeightedClusters(or.Routes, l.Routes) + mergedRoutes := mergeRoutesWeightedClusters(or.Routes, l.Routes, weightedClusterStrategy) or.Routes = mergedRoutes } } else { @@ -157,7 +167,7 @@ func MergeOutboundPolicies(allowPartialHostnamesMatch bool, original []*Outbound if hostsUnion := slicesUnionIfSubset(or.Hostnames, l.Hostnames); len(hostsUnion) > 0 { or.Hostnames = hostsUnion foundHostnames = true - mergedRoutes := mergeRoutesWeightedClusters(or.Routes, l.Routes) + mergedRoutes := mergeRoutesWeightedClusters(or.Routes, l.Routes, weightedClusterStrategy) or.Routes = mergedRoutes } } @@ -189,16 +199,21 @@ func mergeRules(originalRules, latestRules []*Rule) []*Rule { } // mergeRoutesWeightedClusters merges two slices of RouteWeightedClusters and returns a slice where there is one RouteWeightedCluster -// for any HTTPRouteMatch. Where there is an overlap in HTTPRouteMatch between the originalRoutes and latestRoutes, the WeightedClusters -// will be unioned as there can only be one set of WeightedClusters per HTTPRouteMatch. -func mergeRoutesWeightedClusters(originalRoutes, latestRoutes []*RouteWeightedClusters) []*RouteWeightedClusters { +// for any HTTPRouteMatch. Where there is an overlap in HTTPRouteMatch between the originalRoutes and latestRoutes, the WeightedClusters from +// the latest HTTPRouteMatch will be taken if overwriteWeighted clusters is true. Otherwise, the WeightedClusters from the original +// HTTPRouteMatch and the latest will be unioned as there can only be one set of WeightedClusters per HTTPRouteMatch. +func mergeRoutesWeightedClusters(originalRoutes, latestRoutes []*RouteWeightedClusters, weightedClusterStrategy weightedClusterStrategyType) []*RouteWeightedClusters { for _, latest := range latestRoutes { foundRoute := false for _, original := range originalRoutes { if reflect.DeepEqual(original.HTTPRouteMatch, latest.HTTPRouteMatch) { foundRoute = true if !reflect.DeepEqual(original.WeightedClusters, latest.WeightedClusters) { - original.WeightedClusters = original.WeightedClusters.Union(latest.WeightedClusters) + if weightedClusterStrategy == OverwriteWeightedClusters { + original.WeightedClusters = latest.WeightedClusters + } else { + original.WeightedClusters = original.WeightedClusters.Union(latest.WeightedClusters) + } } continue } diff --git a/pkg/trafficpolicy/trafficpolicy_test.go b/pkg/trafficpolicy/trafficpolicy_test.go index f45bea79d4..9201889b8e 100644 --- a/pkg/trafficpolicy/trafficpolicy_test.go +++ b/pkg/trafficpolicy/trafficpolicy_test.go @@ -518,11 +518,13 @@ func TestMergeRules(t *testing.T) { func TestMergeOutboundPolicies(t *testing.T) { testCases := []struct { name string + weightedClusterStrategy weightedClusterStrategyType originalPolicies, latestPolicies, expectedPolicies []*OutboundTrafficPolicy allowPartialHostnamesMatch bool }{ { - name: "hostnames don't match", + name: "hostnames don't match", + weightedClusterStrategy: UnionWeightedClusters, originalPolicies: []*OutboundTrafficPolicy{ { Hostnames: testHostnames, @@ -548,7 +550,8 @@ func TestMergeOutboundPolicies(t *testing.T) { allowPartialHostnamesMatch: false, }, { - name: "hostnames match", + name: "hostnames match", + weightedClusterStrategy: UnionWeightedClusters, originalPolicies: []*OutboundTrafficPolicy{ { Hostnames: testHostnames, @@ -570,7 +573,8 @@ func TestMergeOutboundPolicies(t *testing.T) { allowPartialHostnamesMatch: false, }, { - name: "hostnames match, routes match", + name: "hostnames match, routes match", + weightedClusterStrategy: UnionWeightedClusters, originalPolicies: []*OutboundTrafficPolicy{ { Hostnames: testHostnames, @@ -592,11 +596,15 @@ func TestMergeOutboundPolicies(t *testing.T) { allowPartialHostnamesMatch: false, }, { - name: "hostnames match, routes have same match conditions but diff weighted clusters", + name: "hostnames match, routes have same match conditions but diff weighted clusters with overwrite strategy", + weightedClusterStrategy: OverwriteWeightedClusters, originalPolicies: []*OutboundTrafficPolicy{ { Hostnames: testHostnames, - Routes: []*RouteWeightedClusters{&testRoute}, + Routes: []*RouteWeightedClusters{{ + HTTPRouteMatch: testHTTPRouteMatch, + WeightedClusters: mapset.NewSet(testWeightedCluster), + }}, }, }, latestPolicies: []*OutboundTrafficPolicy{ @@ -611,13 +619,49 @@ func TestMergeOutboundPolicies(t *testing.T) { expectedPolicies: []*OutboundTrafficPolicy{ { Hostnames: testHostnames, - Routes: []*RouteWeightedClusters{&testRoute}, + Routes: []*RouteWeightedClusters{{ + HTTPRouteMatch: testHTTPRouteMatch, + WeightedClusters: mapset.NewSet(testWeightedCluster2), + }}, }, }, allowPartialHostnamesMatch: false, }, { - name: "hostnames partially match", + name: "hostnames match, routes have same match conditions but diff weighted clusters with union strategy", + weightedClusterStrategy: UnionWeightedClusters, + originalPolicies: []*OutboundTrafficPolicy{ + { + Hostnames: testHostnames, + Routes: []*RouteWeightedClusters{{ + HTTPRouteMatch: testHTTPRouteMatch, + WeightedClusters: mapset.NewSet(testWeightedCluster), + }}, + }, + }, + latestPolicies: []*OutboundTrafficPolicy{ + { + Hostnames: testHostnames, + Routes: []*RouteWeightedClusters{{ + HTTPRouteMatch: testHTTPRouteMatch, + WeightedClusters: mapset.NewSet(testWeightedCluster2), + }}, + }, + }, + expectedPolicies: []*OutboundTrafficPolicy{ + { + Hostnames: testHostnames, + Routes: []*RouteWeightedClusters{{ + HTTPRouteMatch: testHTTPRouteMatch, + WeightedClusters: mapset.NewSet(testWeightedCluster, testWeightedCluster2), + }}, + }, + }, + allowPartialHostnamesMatch: false, + }, + { + name: "hostnames partially match", + weightedClusterStrategy: UnionWeightedClusters, originalPolicies: []*OutboundTrafficPolicy{ { Hostnames: testHostnames, @@ -643,7 +687,7 @@ func TestMergeOutboundPolicies(t *testing.T) { t.Run(tc.name, func(t *testing.T) { assert := tassert.New(t) - actual := MergeOutboundPolicies(tc.allowPartialHostnamesMatch, tc.originalPolicies, tc.latestPolicies...) + actual := MergeOutboundPolicies(tc.allowPartialHostnamesMatch, tc.weightedClusterStrategy, tc.originalPolicies, tc.latestPolicies...) assert.ElementsMatch(actual, tc.expectedPolicies) }) } @@ -652,23 +696,46 @@ func TestMergeOutboundPolicies(t *testing.T) { func TestMergeRouteWeightedClusters(t *testing.T) { testCases := []struct { name string + weightedClusterStrategy weightedClusterStrategyType originalRoutes, latestRoutes, expectedRoutes []*RouteWeightedClusters }{ { - name: "merge routes with different match conditions", - originalRoutes: []*RouteWeightedClusters{&testRoute}, - latestRoutes: []*RouteWeightedClusters{&testRoute2}, - expectedRoutes: []*RouteWeightedClusters{&testRoute, &testRoute2}, + name: "merge routes with different match conditions", + weightedClusterStrategy: UnionWeightedClusters, + originalRoutes: []*RouteWeightedClusters{&testRoute}, + latestRoutes: []*RouteWeightedClusters{&testRoute2}, + expectedRoutes: []*RouteWeightedClusters{&testRoute, &testRoute2}, + }, + { + name: "collapse routes with same match conditions and weighted clusters", + weightedClusterStrategy: UnionWeightedClusters, + originalRoutes: []*RouteWeightedClusters{&testRoute}, + latestRoutes: []*RouteWeightedClusters{&testRoute}, + expectedRoutes: []*RouteWeightedClusters{&testRoute}, }, { - name: "collapse routes with same match conditions and weighted clusters", - originalRoutes: []*RouteWeightedClusters{&testRoute}, - latestRoutes: []*RouteWeightedClusters{&testRoute}, - expectedRoutes: []*RouteWeightedClusters{&testRoute}, + name: "use latest weighted cluster when routes have same match conditions and weightedClusterStrategy is OverwriteWeightedClusters", + weightedClusterStrategy: OverwriteWeightedClusters, + originalRoutes: []*RouteWeightedClusters{{ + HTTPRouteMatch: testHTTPRouteMatch, + WeightedClusters: mapset.NewSet(testWeightedCluster), + }}, + latestRoutes: []*RouteWeightedClusters{{ + HTTPRouteMatch: testHTTPRouteMatch, + WeightedClusters: mapset.NewSet(testWeightedCluster2), + }}, + expectedRoutes: []*RouteWeightedClusters{{ + HTTPRouteMatch: testHTTPRouteMatch, + WeightedClusters: mapset.NewSet(testWeightedCluster2), + }}, }, { - name: "routes have same match conditions but different weighted clusters, union the weighted clusters", - originalRoutes: []*RouteWeightedClusters{&testRoute}, + name: "union weighted clusters when routes have same match conditions and weightedClusterStrategy is UnionWeightedClusters", + weightedClusterStrategy: UnionWeightedClusters, + originalRoutes: []*RouteWeightedClusters{{ + HTTPRouteMatch: testHTTPRouteMatch, + WeightedClusters: mapset.NewSet(testWeightedCluster), + }}, latestRoutes: []*RouteWeightedClusters{{ HTTPRouteMatch: testHTTPRouteMatch, WeightedClusters: mapset.NewSet(testWeightedCluster2), @@ -682,8 +749,7 @@ func TestMergeRouteWeightedClusters(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { assert := tassert.New(t) - - actual := mergeRoutesWeightedClusters(tc.originalRoutes, tc.latestRoutes) + actual := mergeRoutesWeightedClusters(tc.originalRoutes, tc.latestRoutes, tc.weightedClusterStrategy) assert.Equal(tc.expectedRoutes, actual) }) }