Skip to content

Commit 7b3c4e4

Browse files
shriramsharmashriramsharma
authored
made cartographer VS check conditional (#435)
made cartographer VS check conditional The cartographer VS check is only needed to verify if .mesh DR should be pinned. This check is not needed anywhere else.
1 parent dcdd1dc commit 7b3c4e4

File tree

3 files changed

+57
-39
lines changed

3 files changed

+57
-39
lines changed

‎admiral/pkg/clusters/serviceentry.go‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1571,8 +1571,10 @@ func AddServiceEntriesWithDrWorker(
15711571
currentDR := getCurrentDRForLocalityLbSetting(rr, isServiceEntryModifyCalledForSourceCluster, cluster, se, partitionedIdentity)
15721572
ctxLogger.Infof("currentDR set for dr=%v cluster=%v", getIstioResourceName(se.Hosts[0], "-default-dr"), cluster)
15731573

1574+
// performCartographerVSCheck is set to true because we need to check if cartographer VS's exportTo
1575+
// has been modified to dot before pinning the .mesh DR
15741576
doDRUpdateForInClusterVSRouting := DoDRUpdateForInClusterVSRouting(
1575-
ctx, ctxLogger, env, cluster, identityId, isServiceEntryModifyCalledForSourceCluster, rr, se)
1577+
ctx, ctxLogger, env, cluster, identityId, isServiceEntryModifyCalledForSourceCluster, rr, se, true)
15761578

15771579
ctxLogger.Infof(
15781580
common.CtxLogFormat, "AddServiceEntriesWithDrWorker", "", "", cluster,

‎admiral/pkg/clusters/virtualservice_routing.go‎

Lines changed: 50 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -539,7 +539,8 @@ func generateVirtualServiceForIncluster(
539539
virtualService.Spec.ExportTo = []string{common.GetSyncNamespace()}
540540
vsRoutingInclusterEnabledForClusterAndIdentity := false
541541
if common.EnableExportTo(vsName) &&
542-
DoVSRoutingInClusterForClusterAndIdentity(ctx, ctxLogger, env, sourceCluster, sourceIdentity, remoteRegistry) {
542+
DoVSRoutingInClusterForClusterAndIdentity(
543+
ctx, ctxLogger, env, sourceCluster, sourceIdentity, remoteRegistry, false) {
543544
vsRoutingInclusterEnabledForClusterAndIdentity = true
544545
virtualService.Spec.ExportTo = getSortedDependentNamespaces(
545546
remoteRegistry.AdmiralCache, vsName, sourceCluster, ctxLogger, true)
@@ -847,7 +848,10 @@ func shouldPerformDRPinning(
847848
return false
848849
}
849850

850-
if !DoVSRoutingInClusterForClusterAndIdentity(ctx, ctxLogger, env, sourceCluster, sourceIdentity, remoteRegistry) {
851+
// performCartographerVSCheck param is set to true since it is needed to make a decision
852+
// if DR pinning should be performed
853+
if !DoVSRoutingInClusterForClusterAndIdentity(
854+
ctx, ctxLogger, env, sourceCluster, sourceIdentity, remoteRegistry, true) {
851855
ctxLogger.Infof(common.CtxLogFormat, "shouldPerformDRPinning",
852856
vsName, common.NamespaceIstioSystem, sourceCluster,
853857
fmt.Sprintf("DoVSRoutingInClusterForClusterAndIdentity=false for cluster %s and identity %s", sourceCluster, sourceIdentity))
@@ -1742,26 +1746,27 @@ func addUpdateInClusterDestinationRule(
17421746
}
17431747

17441748
for sourceCluster, drHosts := range sourceClusterToDRHosts {
1745-
if !DoVSRoutingInClusterForClusterAndIdentity(ctx, ctxLogger, env, sourceCluster, sourceIdentity, remoteRegistry) {
1746-
ctxLogger.Infof(common.CtxLogFormat, "VSBasedRoutingInCluster",
1747-
"", "", sourceCluster,
1748-
fmt.Sprintf("Writing phase: addUpdateInClusterDestinationRule: VS based routing in-cluster disabled for cluster %s and identity %s", sourceCluster, sourceIdentity))
1749-
continue
1750-
}
1751-
1752-
ctxLogger.Info(common.CtxLogFormat, "VSBasedRoutingInCluster",
1753-
"", "", sourceCluster,
1754-
fmt.Sprintf("Writing phase: addUpdateInClusterDestinationRule: VS based routing in-cluster enabled for cluster %s and identity %s", sourceCluster, sourceIdentity))
1755-
17561749
san := fmt.Sprintf("%s%s/%s", common.SpiffePrefix, common.GetSANPrefix(), sourceIdentity)
17571750

17581751
clientTLSSettings := &networkingV1Alpha3.ClientTLSSettings{
17591752
Mode: networkingV1Alpha3.ClientTLSSettings_ISTIO_MUTUAL,
17601753
SubjectAltNames: []string{san},
17611754
}
17621755

1763-
exportToNamespaces := getSortedDependentNamespaces(
1764-
remoteRegistry.AdmiralCache, cname, sourceCluster, ctxLogger, true)
1756+
exportToNamespaces := []string{common.GetSyncNamespace()}
1757+
if common.EnableExportTo(cname) &&
1758+
DoVSRoutingInClusterForClusterAndIdentity(
1759+
ctx, ctxLogger, env, sourceCluster, sourceIdentity, remoteRegistry, false) {
1760+
exportToNamespaces = getSortedDependentNamespaces(
1761+
remoteRegistry.AdmiralCache, cname, sourceCluster, ctxLogger, true)
1762+
ctxLogger.Info(common.CtxLogFormat, "VSBasedRoutingInCluster",
1763+
"", "", sourceCluster,
1764+
fmt.Sprintf("Writing phase: addUpdateInClusterDestinationRule: VS based routing in-cluster enabled for cluster %s and identity %s", sourceCluster, sourceIdentity))
1765+
} else {
1766+
ctxLogger.Infof(common.CtxLogFormat, "VSBasedRoutingInCluster",
1767+
"", "", sourceCluster,
1768+
fmt.Sprintf("Writing phase: addUpdateInClusterDestinationRule: VS based routing in-cluster disabled for cluster %s and identity %s", sourceCluster, sourceIdentity))
1769+
}
17651770

17661771
err := addUpdateRoutingDestinationRule(
17671772
ctx, ctxLogger, remoteRegistry, drHosts, sourceCluster,
@@ -2331,13 +2336,16 @@ func mergeHosts(hosts1 []string, hosts2 []string) []string {
23312336
// or for a specific cluster and identity.
23322337
// It also checks if there is a custom Virtual Service in the identity's namespace
23332338
// and if the Cartographer Virtual Service is disabled for the given cluster and identity.
2339+
// performCartographerVSCheck is a flag to indicate whether to perform the Cartographer VS check as this check
2340+
// is only needed to verify if .mesh DR pinning should be performed.
23342341
func DoVSRoutingInClusterForClusterAndIdentity(
23352342
ctx context.Context,
23362343
ctxLogger *log.Entry,
23372344
env,
23382345
cluster,
23392346
identity string,
2340-
remoteRegistry *RemoteRegistry) bool {
2347+
remoteRegistry *RemoteRegistry,
2348+
performCartographerVSCheck bool) bool {
23412349

23422350
// Check if the feature is enabled globally
23432351
if !common.GetEnableVSRoutingInCluster() {
@@ -2380,24 +2388,29 @@ func DoVSRoutingInClusterForClusterAndIdentity(
23802388
return false
23812389
}
23822390

2383-
// Check if the Cartographer Virtual Service is disabled
2384-
// We will disable this feature if the Cartographer VS does not have dot in exportTo
2385-
rc := remoteRegistry.GetRemoteController(cluster)
2386-
if rc == nil {
2387-
ctxLogger.Warnf(common.CtxLogFormat, "DoVSRoutingInClusterForClusterAndIdentity",
2388-
identity, "", cluster, "remote controller is nil")
2389-
return false
2390-
}
2391-
isCartographerVSDisabled, err := IsCartographerVSDisabled(ctx, ctxLogger, rc, env, identity, getCustomVirtualService)
2392-
if err != nil {
2393-
ctxLogger.Warnf(common.CtxLogFormat, "DoVSRoutingInClusterForClusterAndIdentity",
2394-
identity, "", cluster, fmt.Sprintf("failed IsCartographerVSDisabled check due to error %v", err))
2395-
return false
2396-
}
2397-
if !isCartographerVSDisabled {
2398-
ctxLogger.Infof(common.CtxLogFormat, "DoVSRoutingInClusterForClusterAndIdentity",
2399-
identity, "", cluster, fmt.Sprintf("isCartographerVSDisabled=%v", isCartographerVSDisabled))
2400-
return false
2391+
// This should be set to true if we need to check if .mesh DR should be pinned to remote
2392+
// region or not. For the DR to be pinned, the cartographer VS should have exportTo set to
2393+
// dot
2394+
if performCartographerVSCheck {
2395+
// Check if the Cartographer Virtual Service is disabled
2396+
// We will disable this feature if the Cartographer VS does not have dot in exportTo
2397+
rc := remoteRegistry.GetRemoteController(cluster)
2398+
if rc == nil {
2399+
ctxLogger.Warnf(common.CtxLogFormat, "DoVSRoutingInClusterForClusterAndIdentity",
2400+
identity, "", cluster, "remote controller is nil")
2401+
return false
2402+
}
2403+
isCartographerVSDisabled, err := IsCartographerVSDisabled(ctx, ctxLogger, rc, env, identity, getCustomVirtualService)
2404+
if err != nil {
2405+
ctxLogger.Warnf(common.CtxLogFormat, "DoVSRoutingInClusterForClusterAndIdentity",
2406+
identity, "", cluster, fmt.Sprintf("failed IsCartographerVSDisabled check due to error %v", err))
2407+
return false
2408+
}
2409+
if !isCartographerVSDisabled {
2410+
ctxLogger.Infof(common.CtxLogFormat, "DoVSRoutingInClusterForClusterAndIdentity",
2411+
identity, "", cluster, fmt.Sprintf("isCartographerVSDisabled=%v", isCartographerVSDisabled))
2412+
return false
2413+
}
24012414
}
24022415

24032416
return true
@@ -2431,7 +2444,8 @@ func DoDRUpdateForInClusterVSRouting(
24312444
identity string,
24322445
isSourceCluster bool,
24332446
remoteRegistry *RemoteRegistry,
2434-
se *networkingV1Alpha3.ServiceEntry) bool {
2447+
se *networkingV1Alpha3.ServiceEntry,
2448+
performCartographerVSCheck bool) bool {
24352449

24362450
if remoteRegistry == nil {
24372451
ctxLogger.Warnf(common.CtxLogFormat, "DoDRUpdateForInClusterVSRouting",
@@ -2451,7 +2465,7 @@ func DoDRUpdateForInClusterVSRouting(
24512465
return false
24522466
}
24532467
if isSourceCluster &&
2454-
DoVSRoutingInClusterForClusterAndIdentity(ctx, ctxLogger, env, cluster, identity, remoteRegistry) {
2468+
DoVSRoutingInClusterForClusterAndIdentity(ctx, ctxLogger, env, cluster, identity, remoteRegistry, performCartographerVSCheck) {
24552469
return true
24562470
}
24572471
return false

‎admiral/pkg/clusters/virtualservice_routing_test.go‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8713,7 +8713,7 @@ func TestDoDRUpdateForInClusterVSRouting(t *testing.T) {
87138713
common.InitializeConfig(p)
87148714

87158715
assert.Equal(t, tc.expected, DoDRUpdateForInClusterVSRouting(context.Background(),
8716-
ctxLogger, tc.env, tc.cluster, tc.identity, tc.isSourceCluster, tc.remoteRegistry, tc.serviceEntry))
8716+
ctxLogger, tc.env, tc.cluster, tc.identity, tc.isSourceCluster, tc.remoteRegistry, tc.serviceEntry, true))
87178717
})
87188718
}
87198719

@@ -8866,6 +8866,7 @@ func TestDoVSRoutingInClusterForClusterAndIdentity(t *testing.T) {
88668866
vsRoutingInClusterDisabledResources map[string]string
88678867
remoteRegistry *RemoteRegistry
88688868
expected bool
8869+
performCartographerVSCheck bool
88698870
}{
88708871
{
88718872
name: "Given enableVSRoutingInCluster is false, enabledVSRoutingInClusterForResources is empty" +
@@ -8913,6 +8914,7 @@ func TestDoVSRoutingInClusterForClusterAndIdentity(t *testing.T) {
89138914
enabledVSRoutingInClusterForResources: map[string]string{"cluster9": "*"},
89148915
remoteRegistry: remoteRegistry,
89158916
expected: false,
8917+
performCartographerVSCheck: true,
89168918
},
89178919
{
89188920
name: "Given enableVSRoutingInCluster is true, and given cluster does exists in the map" +
@@ -9012,7 +9014,7 @@ func TestDoVSRoutingInClusterForClusterAndIdentity(t *testing.T) {
90129014
common.InitializeConfig(p)
90139015

90149016
assert.Equal(t, tc.expected, DoVSRoutingInClusterForClusterAndIdentity(
9015-
context.Background(), ctxLogger, tc.env, tc.cluster, tc.identity, tc.remoteRegistry))
9017+
context.Background(), ctxLogger, tc.env, tc.cluster, tc.identity, tc.remoteRegistry, tc.performCartographerVSCheck))
90169018
})
90179019
}
90189020

0 commit comments

Comments
 (0)