diff --git a/admiral/cmd/admiral/cmd/root.go b/admiral/cmd/admiral/cmd/root.go index 4636553e..3bd4cd42 100644 --- a/admiral/cmd/admiral/cmd/root.go +++ b/admiral/cmd/admiral/cmd/root.go @@ -256,6 +256,7 @@ func GetRootCmd(args []string) *cobra.Command { rootCmd.PersistentFlags().BoolVar(¶ms.EnableVSRouting, "enable_vs_routing", false, "Enable/Disable VS Based Routing") rootCmd.PersistentFlags().StringArrayVar(¶ms.VSRoutingGateways, "vs_routing_gateways", []string{}, "The PASSTHROUGH gateways to use for VS based routing") rootCmd.PersistentFlags().StringArrayVar(¶ms.IngressVSExportToNamespaces, "ingress_vs_export_to_namespaces", []string{"istio-system"}, "List of namespaces where the ingress VS should be exported") + rootCmd.PersistentFlags().StringVar(¶ms.IngressLBPolicy, "ingress_lb_policy", "round_robin", "loadbalancer policy for ingress destination rule (round_robin/random/passthrough/least_request)") return rootCmd } diff --git a/admiral/pkg/clusters/serviceentry.go b/admiral/pkg/clusters/serviceentry.go index d7cedbde..059a031a 100644 --- a/admiral/pkg/clusters/serviceentry.go +++ b/admiral/pkg/clusters/serviceentry.go @@ -54,7 +54,6 @@ const ( gtpManagedByMeshAgent = "mesh-agent" gtpManagerMeshAgentFieldValue = "ewok-mesh-agent" errorCluster = "error-cluster" - ingressVSGenerationErrorMessage = "skipped generating ingress virtual service on cluster %s due to error %w" ) func createServiceEntryForDeployment( @@ -163,6 +162,8 @@ func modifyServiceEntryForNewServiceOrPod( // Holds the VS destinations for the TLSRoutes sourceClusterToDestinations = make(map[string]map[string][]*networking.RouteDestination) + // Holds the DR hosts (*.svc.cluster.local) used for VS based routing + sourceClusterToDRHosts = make(map[string]map[string]string) ) clusterName, ok := ctx.Value(common.ClusterName).(string) @@ -772,8 +773,17 @@ func modifyServiceEntryForNewServiceOrPod( ctxLogger.Errorf(common.CtxLogFormat, "getAllVSRouteDestinationsByCluster", deploymentOrRolloutName, deploymentOrRolloutNS, sourceCluster, err) modifySEerr = common.AppendError(modifySEerr, err) + } else if len(destinations) == 0 { + ctxLogger.Warnf(common.CtxLogFormat, "getAllVSRouteDestinationsByCluster", + deploymentOrRolloutName, deploymentOrRolloutNS, sourceCluster, + "No RouteDestinations generated for VS based routing ") } else { sourceClusterToDestinations[sourceCluster] = destinations + // Get the hosts to populate the DR + drHost := fmt.Sprintf("*.%s.%s", deploymentOrRolloutNS, common.DotLocalDomainSuffix) + sourceClusterToDRHosts[sourceCluster] = map[string]string{ + deploymentOrRolloutNS + common.DotLocalDomainSuffix: drHost, + } } } } @@ -799,7 +809,21 @@ func modifyServiceEntryForNewServiceOrPod( // gathered during the discovery phase and write them to the source cluster err := addUpdateVirtualServicesForSourceIngress(ctx, ctxLogger, remoteRegistry, sourceClusterToDestinations) if err != nil { + ctxLogger.Errorf(common.CtxLogFormat, "addUpdateVirtualServicesForSourceIngress", + deploymentOrRolloutName, deploymentOrRolloutNS, "", err) modifySEerr = common.AppendError(modifySEerr, err) + } else { + err := addUpdateDestinationRuleForSourceIngress( + ctx, + ctxLogger, + remoteRegistry, + sourceClusterToDRHosts, + sourceIdentity) + if err != nil { + ctxLogger.Errorf(common.CtxLogFormat, "addUpdateDestinationRuleForSourceIngress", + deploymentOrRolloutName, deploymentOrRolloutNS, "", err) + modifySEerr = common.AppendError(modifySEerr, err) + } } } diff --git a/admiral/pkg/clusters/virtualservice_routing.go b/admiral/pkg/clusters/virtualservice_routing.go index c2fe1d97..44298a8b 100644 --- a/admiral/pkg/clusters/virtualservice_routing.go +++ b/admiral/pkg/clusters/virtualservice_routing.go @@ -8,6 +8,7 @@ import ( argo "github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1" "github.com/istio-ecosystem/admiral/admiral/pkg/controller/common" + "github.com/istio-ecosystem/admiral/admiral/pkg/util" log "github.com/sirupsen/logrus" networkingV1Alpha3 "istio.io/api/networking/v1alpha3" "istio.io/client-go/pkg/apis/networking/v1alpha3" @@ -619,3 +620,101 @@ func getMeshHTTPPortForRollout(ports map[string]map[string]uint32) (uint32, erro func getMeshHTTPPortForDeployment(ports map[string]map[string]uint32) (uint32, error) { return getMeshHTTPPort(common.Deployment, ports) } + +// addUpdateDestinationRuleForSourceIngress adds or updates the DestinationRule for the source ingress +// This is where the DestinationRules are created for the cross-cluster VS based routing +// The DestinationRule is created for the .svc.cluster.local hosts that were discovered during the discovery phase +// on each source cluster +func addUpdateDestinationRuleForSourceIngress( + ctx context.Context, + ctxLogger *log.Entry, + remoteRegistry *RemoteRegistry, + sourceClusterToDRHosts map[string]map[string]string, + sourceIdentity string) error { + + if sourceIdentity == "" { + return fmt.Errorf("sourceIdentity is empty") + } + + san := fmt.Sprintf("%s%s/%s", common.SpiffePrefix, common.GetSANPrefix(), sourceIdentity) + + for sourceCluster, drHosts := range sourceClusterToDRHosts { + + rc := remoteRegistry.GetRemoteController(sourceCluster) + if rc == nil { + ctxLogger.Warnf(common.CtxLogFormat, "addUpdateDestinationRuleForSourceIngress", + "", "", sourceCluster, "remote controller not initialized on this cluster") + continue + } + + for name, drHost := range drHosts { + drObj := networkingV1Alpha3.DestinationRule{ + Host: drHost, + ExportTo: common.GetIngressVSExportToNamespace(), + TrafficPolicy: &networkingV1Alpha3.TrafficPolicy{ + LoadBalancer: &networkingV1Alpha3.LoadBalancerSettings{ + LbPolicy: &networkingV1Alpha3.LoadBalancerSettings_Simple{ + Simple: getIngressDRLoadBalancerPolicy(), + }, + LocalityLbSetting: &networkingV1Alpha3.LocalityLoadBalancerSetting{ + Distribute: []*networkingV1Alpha3.LocalityLoadBalancerSetting_Distribute{ + { + From: "*", + To: map[string]uint32{"*": 100}, + }, + }, + }, + }, + Tls: &networkingV1Alpha3.ClientTLSSettings{ + SubjectAltNames: []string{san}, + }, + }, + } + drName := fmt.Sprintf("%s-routing-dr", name) + + newDR := createDestinationRuleSkeleton(drObj, drName, util.IstioSystemNamespace) + + //Get existing DR + existingDR, err := rc. + DestinationRuleController. + IstioClient. + NetworkingV1alpha3(). + DestinationRules(util.IstioSystemNamespace).Get(ctx, drName, metaV1.GetOptions{}) + if err != nil { + ctxLogger.Warnf(common.CtxLogFormat, + "addUpdateDestinationRuleForSourceIngress", + drName, + util.IstioSystemNamespace, + sourceCluster, fmt.Sprintf("failed getting existing DR, error=%v", err)) + existingDR = nil + } + + err = addUpdateDestinationRule(ctxLogger, ctx, newDR, existingDR, util.IstioSystemNamespace, rc, remoteRegistry) + if err != nil { + return err + } + + } + + } + return nil +} + +// getIngressDRLoadBalancerPolicy return the load balancer policy for the ingress destination rule +// Default is networkingV1Alpha3.LoadBalancerSettings_ROUND_ROBIN +func getIngressDRLoadBalancerPolicy() networkingV1Alpha3.LoadBalancerSettings_SimpleLB { + + switch common.GetIngressLBPolicy() { + case "round_robin": + return networkingV1Alpha3.LoadBalancerSettings_ROUND_ROBIN + case "random": + return networkingV1Alpha3.LoadBalancerSettings_RANDOM + case "least_request": + return networkingV1Alpha3.LoadBalancerSettings_LEAST_REQUEST + case "passthrough": + return networkingV1Alpha3.LoadBalancerSettings_PASSTHROUGH + default: + return networkingV1Alpha3.LoadBalancerSettings_ROUND_ROBIN + } + +} diff --git a/admiral/pkg/clusters/virtualservice_routing_test.go b/admiral/pkg/clusters/virtualservice_routing_test.go index 4269c600..49d50e19 100644 --- a/admiral/pkg/clusters/virtualservice_routing_test.go +++ b/admiral/pkg/clusters/virtualservice_routing_test.go @@ -8,6 +8,7 @@ import ( "github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1" "github.com/istio-ecosystem/admiral/admiral/pkg/controller/common" "github.com/istio-ecosystem/admiral/admiral/pkg/controller/istio" + "github.com/istio-ecosystem/admiral/admiral/pkg/util" log "github.com/sirupsen/logrus" "github.com/stretchr/testify/require" networkingV1Alpha3 "istio.io/api/networking/v1alpha3" @@ -2085,3 +2086,240 @@ func TestGetPreviewSNIHostFromRollout(t *testing.T) { } } + +func TestGetIngressDRLoadBalancerPolicy(t *testing.T) { + + testCases := []struct { + name string + admiralParams common.AdmiralParams + expectedPolicy networkingV1Alpha3.LoadBalancerSettings_SimpleLB + }{ + { + name: "Given a no ingressPolicy " + + "When getIngressDRLoadBalancerPolicy is invoked, " + + "Then it should return the default round robin LoadBalancerPolicy", + admiralParams: common.AdmiralParams{ + IngressLBPolicy: "", + }, + expectedPolicy: networkingV1Alpha3.LoadBalancerSettings_ROUND_ROBIN, + }, + { + name: "Given a random ingressPolicy " + + "When getIngressDRLoadBalancerPolicy is invoked, " + + "Then it should return random LoadBalancerPolicy", + admiralParams: common.AdmiralParams{ + IngressLBPolicy: "random", + }, + expectedPolicy: networkingV1Alpha3.LoadBalancerSettings_RANDOM, + }, + { + name: "Given a least request ingressPolicy " + + "When getIngressDRLoadBalancerPolicy is invoked, " + + "Then it should return least request LoadBalancerPolicy", + admiralParams: common.AdmiralParams{ + IngressLBPolicy: "least_request", + }, + expectedPolicy: networkingV1Alpha3.LoadBalancerSettings_LEAST_REQUEST, + }, + { + name: "Given a round robin ingressPolicy " + + "When getIngressDRLoadBalancerPolicy is invoked, " + + "Then it should return round robin LoadBalancerPolicy", + admiralParams: common.AdmiralParams{ + IngressLBPolicy: "round_robin", + }, + expectedPolicy: networkingV1Alpha3.LoadBalancerSettings_ROUND_ROBIN, + }, + { + name: "Given a passthrough ingressPolicy " + + "When getIngressDRLoadBalancerPolicy is invoked, " + + "Then it should return passthrough LoadBalancerPolicy", + admiralParams: common.AdmiralParams{ + IngressLBPolicy: "passthrough", + }, + expectedPolicy: networkingV1Alpha3.LoadBalancerSettings_PASSTHROUGH, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + common.ResetSync() + common.InitializeConfig(tc.admiralParams) + actual := getIngressDRLoadBalancerPolicy() + require.Equal(t, tc.expectedPolicy, actual) + }) + } + +} + +func TestAaddUpdateDestinationRuleForSourceIngress(t *testing.T) { + + existingDR := &apiNetworkingV1Alpha3.DestinationRule{ + ObjectMeta: metaV1.ObjectMeta{ + Name: "test-ns.svc.cluster.local-routing-dr", + Namespace: util.IstioSystemNamespace, + }, + Spec: networkingV1Alpha3.DestinationRule{ + Host: "*.test-ns.svc.cluster.local", + ExportTo: []string{util.IstioSystemNamespace}, + TrafficPolicy: &networkingV1Alpha3.TrafficPolicy{ + LoadBalancer: &networkingV1Alpha3.LoadBalancerSettings{ + LbPolicy: &networkingV1Alpha3.LoadBalancerSettings_Simple{ + Simple: networkingV1Alpha3.LoadBalancerSettings_LEAST_REQUEST, + }, + }, + Tls: &networkingV1Alpha3.ClientTLSSettings{ + SubjectAltNames: []string{"spiffe://test-san-prefix/test-identity"}, + }, + }, + }, + } + + admiralParams := common.AdmiralParams{ + SANPrefix: "test-san-prefix", + IngressVSExportToNamespaces: []string{"istio-system"}, + } + common.ResetSync() + common.InitializeConfig(admiralParams) + + istioClientWithExistingDR := istioFake.NewSimpleClientset() + istioClientWithExistingDR.NetworkingV1alpha3().DestinationRules(util.IstioSystemNamespace). + Create(context.Background(), existingDR, metaV1.CreateOptions{}) + + istioClientWithNoExistingDR := istioFake.NewSimpleClientset() + + rr := NewRemoteRegistry(context.Background(), admiralParams) + + ctxLogger := log.WithFields(log.Fields{ + "type": "DestinationRule", + }) + + testCases := []struct { + name string + istioClient *istioFake.Clientset + sourceClusterToDRHosts map[string]map[string]string + sourceIdentity string + expectedError error + expectedDestinationRules *apiNetworkingV1Alpha3.DestinationRule + }{ + { + name: "Given a empty sourceIdentity " + + "When addUpdateDestinationRuleForSourceIngress is invoked, " + + "Then it should return an error", + sourceIdentity: "", + expectedError: fmt.Errorf("sourceIdentity is empty"), + }, + { + name: "Given a valid sourceClusterToDRHosts " + + "When addUpdateDestinationRuleForSourceIngress is invoked, " + + "Then it should create the destination rules", + sourceIdentity: "test-identity", + sourceClusterToDRHosts: map[string]map[string]string{ + "cluster-1": { + "test-ns.svc.cluster.local": "*.test-ns.svc.cluster.local", + }, + }, + istioClient: istioClientWithNoExistingDR, + expectedError: nil, + expectedDestinationRules: &apiNetworkingV1Alpha3.DestinationRule{ + ObjectMeta: metaV1.ObjectMeta{ + Name: "test-ns.svc.cluster.local-routing-dr", + Namespace: util.IstioSystemNamespace, + }, + Spec: networkingV1Alpha3.DestinationRule{ + Host: "*.test-ns.svc.cluster.local", + ExportTo: []string{util.IstioSystemNamespace}, + TrafficPolicy: &networkingV1Alpha3.TrafficPolicy{ + LoadBalancer: &networkingV1Alpha3.LoadBalancerSettings{ + LbPolicy: &networkingV1Alpha3.LoadBalancerSettings_Simple{ + Simple: networkingV1Alpha3.LoadBalancerSettings_ROUND_ROBIN, + }, + LocalityLbSetting: &networkingV1Alpha3.LocalityLoadBalancerSetting{ + Distribute: []*networkingV1Alpha3.LocalityLoadBalancerSetting_Distribute{ + { + From: "*", + To: map[string]uint32{"*": 100}, + }, + }, + }, + }, + Tls: &networkingV1Alpha3.ClientTLSSettings{ + SubjectAltNames: []string{"spiffe://test-san-prefix/test-identity"}, + }, + }, + }, + }, + }, + { + name: "Given a valid sourceClusterToDRHosts " + + "When addUpdateDestinationRuleForSourceIngress is invoked, " + + "Then it should create the destination rules", + sourceIdentity: "test-identity", + sourceClusterToDRHosts: map[string]map[string]string{ + "cluster-1": { + "test-ns.svc.cluster.local": "*.test-ns.svc.cluster.local", + }, + }, + istioClient: istioClientWithExistingDR, + expectedError: nil, + expectedDestinationRules: &apiNetworkingV1Alpha3.DestinationRule{ + ObjectMeta: metaV1.ObjectMeta{ + Name: "test-ns.svc.cluster.local-routing-dr", + Namespace: util.IstioSystemNamespace, + }, + Spec: networkingV1Alpha3.DestinationRule{ + Host: "*.test-ns.svc.cluster.local", + ExportTo: []string{util.IstioSystemNamespace}, + TrafficPolicy: &networkingV1Alpha3.TrafficPolicy{ + LoadBalancer: &networkingV1Alpha3.LoadBalancerSettings{ + LbPolicy: &networkingV1Alpha3.LoadBalancerSettings_Simple{ + Simple: networkingV1Alpha3.LoadBalancerSettings_ROUND_ROBIN, + }, + LocalityLbSetting: &networkingV1Alpha3.LocalityLoadBalancerSetting{ + Distribute: []*networkingV1Alpha3.LocalityLoadBalancerSetting_Distribute{ + { + From: "*", + To: map[string]uint32{"*": 100}, + }, + }, + }, + }, + Tls: &networkingV1Alpha3.ClientTLSSettings{ + SubjectAltNames: []string{"spiffe://test-san-prefix/test-identity"}, + }, + }, + }, + }, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + rc := &RemoteController{ + ClusterID: "cluster-1", + DestinationRuleController: &istio.DestinationRuleController{}, + } + rc.DestinationRuleController.IstioClient = tc.istioClient + rr.PutRemoteController("cluster-1", rc) + + err := addUpdateDestinationRuleForSourceIngress( + context.Background(), + ctxLogger, + rr, + tc.sourceClusterToDRHosts, + tc.sourceIdentity) + if tc.expectedError != nil { + require.NotNil(t, err) + require.Equal(t, tc.expectedError.Error(), err.Error()) + } else { + actualDR, err := tc.istioClient.NetworkingV1alpha3().DestinationRules(util.IstioSystemNamespace). + Get(context.Background(), "test-ns.svc.cluster.local-routing-dr", metaV1.GetOptions{}) + require.Nil(t, err) + require.Equal(t, tc.expectedDestinationRules.Spec.Host, actualDR.Spec.Host) + require.Equal(t, tc.expectedDestinationRules.Spec.TrafficPolicy, actualDR.Spec.TrafficPolicy) + require.Equal(t, tc.expectedDestinationRules.Spec.ExportTo, actualDR.Spec.ExportTo) + } + }) + } + +} diff --git a/admiral/pkg/controller/common/config.go b/admiral/pkg/controller/common/config.go index ebe74fd4..39401a45 100644 --- a/admiral/pkg/controller/common/config.go +++ b/admiral/pkg/controller/common/config.go @@ -426,6 +426,12 @@ func EnableSWAwareNSCaches() bool { return wrapper.params.EnableSWAwareNSCaches } +func GetIngressLBPolicy() string { + wrapper.RLock() + defer wrapper.RUnlock() + return wrapper.params.IngressLBPolicy +} + func GetIngressVSExportToNamespace() []string { wrapper.RLock() defer wrapper.RUnlock() diff --git a/admiral/pkg/controller/common/types.go b/admiral/pkg/controller/common/types.go index cb0189b8..27a84ee1 100644 --- a/admiral/pkg/controller/common/types.go +++ b/admiral/pkg/controller/common/types.go @@ -124,6 +124,7 @@ type AdmiralParams struct { EnableVSRouting bool VSRoutingGateways []string IngressVSExportToNamespaces []string + IngressLBPolicy string } func (b AdmiralParams) String() string { diff --git a/admiral/pkg/util/constants.go b/admiral/pkg/util/constants.go index 0387547c..2e56559a 100644 --- a/admiral/pkg/util/constants.go +++ b/admiral/pkg/util/constants.go @@ -13,4 +13,6 @@ const ( GlobalTrafficPolicy = "globalTrafficPolicy" OutlierDetection = "outlierDetection" ClientConnectionConfig = "clientConnectionConfig" + + IstioSystemNamespace = "istio-system" )