From 30c60334a36e71b2889e4337d7bfc11ef32049e4 Mon Sep 17 00:00:00 2001 From: vinay-g Date: Tue, 13 Aug 2024 08:47:41 -0700 Subject: [PATCH] [MESH-5364] - Adding source cluster to VS copy list --- .gitignore | 1 + admiral/cmd/admiral/cmd/root.go | 1 - .../pkg/clusters/virtualservice_handler.go | 13 +- .../clusters/virtualservice_handler_test.go | 63 ++++++++- admiral/pkg/controller/common/config.go | 6 - admiral/pkg/controller/common/types.go | 121 +++++++++--------- 6 files changed, 122 insertions(+), 83 deletions(-) diff --git a/.gitignore b/.gitignore index 4e443ccb..aa4df09a 100644 --- a/.gitignore +++ b/.gitignore @@ -9,3 +9,4 @@ out.yaml istio-* .DS_Store cobertura-coverage.xml +.vscode diff --git a/admiral/cmd/admiral/cmd/root.go b/admiral/cmd/admiral/cmd/root.go index 8509e0d5..f977d43e 100644 --- a/admiral/cmd/admiral/cmd/root.go +++ b/admiral/cmd/admiral/cmd/root.go @@ -237,7 +237,6 @@ func GetRootCmd(args []string) *cobra.Command { rootCmd.PersistentFlags().StringArrayVar(¶ms.GatewayAssetAliases, "gateway_asset_aliases", []string{"Intuit.platform.servicesgateway.servicesgateway"}, "The asset aliases used for API Gateway") rootCmd.PersistentFlags().BoolVar(¶ms.EnableActivePassive, "enable_active_passive", false, "Enable/Disable Active-Passive behavior") rootCmd.PersistentFlags().BoolVar(¶ms.EnableSWAwareNSCaches, "enable_sw_aware_ns_caches", false, "Enable/Disable SW Aware NS Caches") - rootCmd.PersistentFlags().BoolVar(¶ms.EnableSyncIstioResourcesToSourceClusters, "enable_sync_istio_resources_to_source_clusters", true, "Enable/Disable Sync of Istio Resources to Source Clusters") rootCmd.PersistentFlags().BoolVar(¶ms.AdmiralStateSyncerMode, "admiral_state_syncer_mode", false, "Enable/Disable admiral to run as state syncer only") rootCmd.PersistentFlags().Int64Var(¶ms.DefaultWarmupDurationSecs, "default_warmup_duration_in_seconds", 45, "The default value for the warmupDurationSecs to be used on Destination Rules created by admiral") diff --git a/admiral/pkg/clusters/virtualservice_handler.go b/admiral/pkg/clusters/virtualservice_handler.go index 0691ecd3..e96c5ed3 100644 --- a/admiral/pkg/clusters/virtualservice_handler.go +++ b/admiral/pkg/clusters/virtualservice_handler.go @@ -161,9 +161,12 @@ func (vh *VirtualServiceHandler) handleVirtualServiceEvent(ctx context.Context, dependentClusters := vh.remoteRegistry.AdmiralCache.CnameDependentClusterCache.Get(spec.Hosts[0]).CopyJustValues() if len(dependentClusters) > 0 { + // Add source clusters to the list of clusters to copy the virtual service + sourceClusters := vh.remoteRegistry.AdmiralCache.CnameClusterCache.Get(spec.Hosts[0]).CopyJustValues() + clusters := append(dependentClusters, sourceClusters...) err := vh.syncVirtualServiceForDependentClusters( ctx, - dependentClusters, + clusters, virtualService, event, vh.remoteRegistry, @@ -261,10 +264,6 @@ func syncVirtualServicesToAllDependentClusters( var wg sync.WaitGroup wg.Add(len(clusters)) for _, cluster := range clusters { - if cluster == sourceCluster && !common.DoSyncIstioResourcesToSourceClusters() { - wg.Done() - continue - } go func(ctx context.Context, cluster string, remoteRegistry *RemoteRegistry, virtualServiceCopy *v1alpha3.VirtualService, event common.Event, syncNamespace string) { defer wg.Done() err := syncVirtualServiceToDependentCluster( @@ -373,10 +372,6 @@ func syncVirtualServicesToAllRemoteClusters( var wg sync.WaitGroup wg.Add(len(clusters)) for _, cluster := range clusters { - if cluster == sourceCluster && !common.DoSyncIstioResourcesToSourceClusters() { - wg.Done() - continue - } go func(ctx context.Context, cluster string, remoteRegistry *RemoteRegistry, virtualServiceCopy *v1alpha3.VirtualService, event common.Event, syncNamespace string) { defer wg.Done() err := syncVirtualServiceToRemoteCluster( diff --git a/admiral/pkg/clusters/virtualservice_handler_test.go b/admiral/pkg/clusters/virtualservice_handler_test.go index dd6b248c..298fde91 100644 --- a/admiral/pkg/clusters/virtualservice_handler_test.go +++ b/admiral/pkg/clusters/virtualservice_handler_test.go @@ -38,6 +38,7 @@ func TestHandleVirtualServiceEvent(t *testing.T) { ctx = context.TODO() remoteRegistry = NewRemoteRegistry(ctx, common.AdmiralParams{}) remoteRegistryWithDependents = newRemoteRegistryWithDependents(ctx, cname1, dependentCluster1) + remoteRegistryWithDependentsAndSource = newRemoteRegistryWithSourceClusters(ctx, cname1, clusterID, dependentCluster1) ) cases := []struct { name string @@ -332,6 +333,30 @@ func TestHandleVirtualServiceEvent(t *testing.T) { expectToCallSyncResourceForAllClusters: true, expectedErr: nil, }, + { + name: "Given VirtualService is valid and argo is enabled, " + + "When, handleVirtualServiceEvent is invoked, " + + "When there are dependents clusters and source clusters," + + "And, source clusters should be included in the list" + + "And, syncVirtualServiceForDependentClusters should be called", + params: common.AdmiralParams{ + ArgoRolloutsEnabled: true, + SyncNamespace: syncNamespace, + }, + remoteRegistry: remoteRegistryWithDependentsAndSource, + virtualService: &apiNetworkingV1Alpha3.VirtualService{ + Spec: networkingV1Alpha3.VirtualService{ + Hosts: []string{cname1}, + }, + }, + updateResource: newFakeUpdateResource(false, nil), + syncResourceForDependentClusters: checkSourceClusterInDependentList(nil), + syncResourceForAllClusters: newFakeSyncResource(nil), + expectToCallUpdateResource: true, + expectToCallSyncResourceForDependentClusters: false, + expectToCallSyncResourceForAllClusters: false, + expectedErr: nil, + }, } for _, c := range cases { t.Run(c.name, func(t *testing.T) { @@ -898,9 +923,7 @@ func TestSyncVirtualServicesToAllDependentClusters(t *testing.T) { for _, c := range cases { t.Run(c.name, func(t *testing.T) { common.ResetSync() - admiralParams := common.AdmiralParams{ - EnableSyncIstioResourcesToSourceClusters: c.doSyncVSToSourceCluster, - } + admiralParams := common.AdmiralParams{} common.InitializeConfig(admiralParams) err := syncVirtualServicesToAllDependentClusters( ctx, @@ -1264,9 +1287,7 @@ func TestSyncVirtualServicesToAllRemoteClusters(t *testing.T) { for _, c := range cases { t.Run(c.name, func(t *testing.T) { common.ResetSync() - admiralParams := common.AdmiralParams{ - EnableSyncIstioResourcesToSourceClusters: c.doSyncVSToSourceCluster, - } + admiralParams := common.AdmiralParams{} common.InitializeConfig(admiralParams) err := syncVirtualServicesToAllRemoteClusters( ctx, @@ -1505,6 +1526,29 @@ func newFakeSyncResource(err error) *fakeSyncResource { return f } +func checkSourceClusterInDependentList(err error) *fakeSyncResource { + f := &fakeSyncResource{} + f.syncResourceFunc = func() SyncVirtualServiceResource { + return func( + _ context.Context, + dependentClusters []string, + _ *apiNetworkingV1Alpha3.VirtualService, + _ common.Event, + _ *RemoteRegistry, + clusterId string, + _ string) error { + for _, cluster := range dependentClusters { + if cluster == clusterId { + return nil + } + } + f.called = true + return fmt.Errorf("source clusters are not present in cluster list") + } + } + return f +} + type fakeUpdateResource struct { updateResourceFunc func() UpdateResourcesForVirtualService called bool @@ -1532,6 +1576,13 @@ func newRemoteRegistryWithDependents(ctx context.Context, cname, clusterID strin return remoteRegistry } +func newRemoteRegistryWithSourceClusters(ctx context.Context, cname, clusterID string, depCluster string) *RemoteRegistry { + remoteRegistry := NewRemoteRegistry(ctx, common.AdmiralParams{}) + remoteRegistry.AdmiralCache.CnameClusterCache.Put(cname, clusterID, clusterID) + remoteRegistry.AdmiralCache.CnameDependentClusterCache.Put(cname, depCluster, depCluster) + return remoteRegistry +} + func newRemoteRegistry(ctx context.Context, clusters map[string]*RemoteController) *RemoteRegistry { remoteRegistry := NewRemoteRegistry(ctx, common.AdmiralParams{}) for cluster, controller := range clusters { diff --git a/admiral/pkg/controller/common/config.go b/admiral/pkg/controller/common/config.go index 49972bff..c4f29a99 100644 --- a/admiral/pkg/controller/common/config.go +++ b/admiral/pkg/controller/common/config.go @@ -432,12 +432,6 @@ func DoGenerationCheck() bool { return wrapper.params.EnableGenerationCheck } -func DoSyncIstioResourcesToSourceClusters() bool { - wrapper.RLock() - defer wrapper.RUnlock() - return wrapper.params.EnableSyncIstioResourcesToSourceClusters -} - func GetResyncIntervals() util.ResyncIntervals { wrapper.RLock() defer wrapper.RUnlock() diff --git a/admiral/pkg/controller/common/types.go b/admiral/pkg/controller/common/types.go index fa06f3aa..af7c3bf4 100644 --- a/admiral/pkg/controller/common/types.go +++ b/admiral/pkg/controller/common/types.go @@ -39,67 +39,66 @@ type SidecarEgressMap struct { } type AdmiralParams struct { - ArgoRolloutsEnabled bool - KubeconfigPath string - SecretFilterTags string - CacheReconcileDuration time.Duration - SeAndDrCacheReconcileDuration time.Duration - ClusterRegistriesNamespace string - DependenciesNamespace string - DnsConfigFile string - DNSTimeoutMs int - DNSRetries int - TrafficConfigNamespace string - SyncNamespace string - EnableSAN bool - SANPrefix string - AdmiralConfig string - Profile string - LabelSet *LabelSet - LogLevel int - HostnameSuffix string - PreviewHostnamePrefix string - MetricsEnabled bool - ChannelCapacity int - WorkloadSidecarUpdate string - WorkloadSidecarName string - AdmiralStateCheckerName string - DRStateStoreConfigPath string - ServiceEntryIPPrefix string - EnvoyFilterVersion string - DeprecatedEnvoyFilterVersion string - EnvoyFilterAdditionalConfig string - EnableRoutingPolicy bool - ExcludedIdentityList []string - AdditionalEndpointSuffixes []string - AdditionalEndpointLabelFilters []string - HAMode string - EnableWorkloadDataStorage bool - EnableDiffCheck bool - EnableProxyEnvoyFilter bool - EnableDependencyProcessing bool - DeploymentOrRolloutWorkerConcurrency int - DependentClusterWorkerConcurrency int - SeAddressConfigmap string - DependencyWarmupMultiplier int - EnableOutlierDetection bool - EnableClientConnectionConfigProcessing bool - MaxRequestsPerConnection int32 - EnableAbsoluteFQDN bool - EnableAbsoluteFQDNForLocalEndpoints bool - DisableDefaultAutomaticFailover bool - EnableServiceEntryCache bool - AlphaIdentityList []string - EnableDestinationRuleCache bool - DisableIPGeneration bool - EnableActivePassive bool - EnableSWAwareNSCaches bool - ExportToIdentityList []string - ExportToMaxNamespaces int - EnableSyncIstioResourcesToSourceClusters bool - AdmiralStateSyncerMode bool - DefaultWarmupDurationSecs int64 - EnableGenerationCheck bool + ArgoRolloutsEnabled bool + KubeconfigPath string + SecretFilterTags string + CacheReconcileDuration time.Duration + SeAndDrCacheReconcileDuration time.Duration + ClusterRegistriesNamespace string + DependenciesNamespace string + DnsConfigFile string + DNSTimeoutMs int + DNSRetries int + TrafficConfigNamespace string + SyncNamespace string + EnableSAN bool + SANPrefix string + AdmiralConfig string + Profile string + LabelSet *LabelSet + LogLevel int + HostnameSuffix string + PreviewHostnamePrefix string + MetricsEnabled bool + ChannelCapacity int + WorkloadSidecarUpdate string + WorkloadSidecarName string + AdmiralStateCheckerName string + DRStateStoreConfigPath string + ServiceEntryIPPrefix string + EnvoyFilterVersion string + DeprecatedEnvoyFilterVersion string + EnvoyFilterAdditionalConfig string + EnableRoutingPolicy bool + ExcludedIdentityList []string + AdditionalEndpointSuffixes []string + AdditionalEndpointLabelFilters []string + HAMode string + EnableWorkloadDataStorage bool + EnableDiffCheck bool + EnableProxyEnvoyFilter bool + EnableDependencyProcessing bool + DeploymentOrRolloutWorkerConcurrency int + DependentClusterWorkerConcurrency int + SeAddressConfigmap string + DependencyWarmupMultiplier int + EnableOutlierDetection bool + EnableClientConnectionConfigProcessing bool + MaxRequestsPerConnection int32 + EnableAbsoluteFQDN bool + EnableAbsoluteFQDNForLocalEndpoints bool + DisableDefaultAutomaticFailover bool + EnableServiceEntryCache bool + AlphaIdentityList []string + EnableDestinationRuleCache bool + DisableIPGeneration bool + EnableActivePassive bool + EnableSWAwareNSCaches bool + ExportToIdentityList []string + ExportToMaxNamespaces int + AdmiralStateSyncerMode bool + DefaultWarmupDurationSecs int64 + EnableGenerationCheck bool // Cartographer specific params TrafficConfigPersona bool