Skip to content

Commit

Permalink
Refactor SidecarEgressMap to support cluster-level mapping and remove…
Browse files Browse the repository at this point in the history 
… unused cname logic
  • Loading branch information
Punakshi committed Oct 6, 2024
1 parent 182ccab commit 1a95eb0
Show file tree
Hide file tree
Showing 5 changed files with 49 additions and 46 deletions.
31 changes: 15 additions & 16 deletions admiral/pkg/clusters/serviceentry.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,6 @@ func modifyServiceEntryForNewServiceOrPod(
clientConnectionSettings = make(map[string][]*v1.ClientConnectionConfig)
gtps = make(map[string][]*v1.GlobalTrafficPolicy)
weightedServices = make(map[string]*WeightedService)
cnames = make(map[string]string)
sourceServices = make(map[string]map[string]*k8sV1.Service)
sourceWeightedServices = make(map[string]map[string]*WeightedService)
sourceDeployments = make(map[string]*k8sAppsV1.Deployment)
Expand Down Expand Up @@ -307,7 +306,6 @@ func modifyServiceEntryForNewServiceOrPod(
deploymentOrRolloutNS, rc.ClusterID, "updating identity<->cluster mapping")
clusterDeployRolloutPresent[rc.ClusterID][common.Rollout] = true
cname = common.GetCnameForRollout(rollout, common.GetWorkloadIdentifier(), common.GetHostnameSuffix())
cnames[cname] = "1"
sourceRollouts[rc.ClusterID] = rollout
sourceClusters = append(sourceClusters, clusterId)
namespace = rollout.Namespace
Expand Down Expand Up @@ -603,7 +601,7 @@ func modifyServiceEntryForNewServiceOrPod(
blueGreenService := updateEndpointsForBlueGreen(
sourceRollouts[sourceCluster],
sourceWeightedServices[sourceCluster],
cnames, ep, sourceCluster, key)
ep, sourceCluster, key)
if common.IsAdmiralStateSyncerMode() {
registryConfig.Clusters[sourceCluster].Environment[env].Services = map[string]*registry.RegistryServiceConfig{
blueGreenService.Service.Name: &registry.RegistryServiceConfig{
Expand Down Expand Up @@ -756,7 +754,9 @@ func modifyServiceEntryForNewServiceOrPod(
}

for _, val := range dependents {
remoteRegistry.AdmiralCache.DependencyNamespaceCache.Put(val, serviceInstance[appType[sourceCluster]].Namespace, localFqdn, cnames)
if remoteRegistry.AdmiralCache.IdentityClusterCache.Get(val) != nil && remoteRegistry.AdmiralCache.IdentityClusterCache.Get(val).CheckIfPresent(sourceCluster) {
remoteRegistry.AdmiralCache.DependencyNamespaceCache.Put(val, sourceCluster, serviceInstance[appType[sourceCluster]].Namespace, localFqdn, map[string]string{cname: "1"})
}
}

if common.DoVSRoutingForCluster(sourceCluster) {
Expand Down Expand Up @@ -1072,22 +1072,20 @@ func sortClientConnectionConfigByCreationTime(ods []*v1.ClientConnectionConfig,
})
}

func updateEndpointsForBlueGreen(rollout *argo.Rollout, weightedServices map[string]*WeightedService, cnames map[string]string,
func updateEndpointsForBlueGreen(rollout *argo.Rollout, weightedServices map[string]*WeightedService,
ep *networking.WorkloadEntry, sourceCluster string, meshHost string) *WeightedService {
activeServiceName := rollout.Spec.Strategy.BlueGreen.ActiveService
previewServiceName := rollout.Spec.Strategy.BlueGreen.PreviewService

if previewService, ok := weightedServices[previewServiceName]; strings.HasPrefix(meshHost, common.BlueGreenRolloutPreviewPrefix+common.Sep) && ok {
previewServiceInstance := previewService.Service
localFqdn := previewServiceInstance.Name + common.Sep + previewServiceInstance.Namespace + common.GetLocalDomainSuffix()
cnames[localFqdn] = "1"
ep.Address = localFqdn
ep.Ports = GetMeshPortsForRollout(sourceCluster, previewServiceInstance, rollout)
return previewService
} else if activeService, ok := weightedServices[activeServiceName]; ok {
activeServiceInstance := activeService.Service
localFqdn := activeServiceInstance.Name + common.Sep + activeServiceInstance.Namespace + common.GetLocalDomainSuffix()
cnames[localFqdn] = "1"
ep.Address = localFqdn
ep.Ports = GetMeshPortsForRollout(sourceCluster, activeServiceInstance, rollout)
return activeService
Expand Down Expand Up @@ -1140,10 +1138,11 @@ func modifySidecarForLocalClusterCommunication(
defer util.LogElapsedTime("modifySidecarForLocalClusterCommunication", sourceIdentity, sidecarNamespace, rc.ClusterID)
//get existing sidecar from the cluster
sidecarConfig := rc.SidecarController
sidecarEgressMap.Range(func(k string, v map[string]common.SidecarEgress) {
sidecarEgressMap.Range(func(k string, v map[string]map[string]common.SidecarEgress) {
if k == sourceIdentity {
sidecarEgress := v
if sidecarConfig == nil || sidecarEgress == nil {
serverClusterNsSidecarEgressMapping := v
serverNsSidecarEgressMapping := serverClusterNsSidecarEgressMapping[rc.ClusterID]
if sidecarConfig == nil || serverNsSidecarEgressMapping == nil {
return
}

Expand All @@ -1159,13 +1158,13 @@ func modifySidecarForLocalClusterCommunication(
//copy and add our new local FQDN
newSidecar := copySidecar(sidecar)
egressHosts := make(map[string]string)
for _, sidecarEgress := range sidecarEgress {
egressHost := sidecarEgress.Namespace + "/" + sidecarEgress.FQDN
egressHosts[egressHost] = egressHost
sidecarEgress.CNAMEs.Range(func(k, v string) {
scopedCname := sidecarEgress.Namespace + "/" + k
for _, serverSidecarEgress := range serverNsSidecarEgressMapping {
egressHost := serverSidecarEgress.Namespace + "/" + serverSidecarEgress.FQDN
egressHosts[egressHost] = egressHost //.local entry
/*serverSidecarEgress.CNAMEs.Range(func(k, v string) {
scopedCname := serverSidecarEgress.Namespace + "/" + k
egressHosts[scopedCname] = scopedCname
})
})*/
}

for egressHost := range egressHosts {
Expand Down
27 changes: 13 additions & 14 deletions admiral/pkg/clusters/serviceentry_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2856,12 +2856,15 @@ func TestModifyNonExistingSidecarForLocalClusterCommunication(t *testing.T) {
sidecarController.IstioClient = istiofake.NewSimpleClientset()
sidecarController.IstioClient.NetworkingV1alpha3().Sidecars(identityNamespace).Create(context.TODO(), sidecar, metav1.CreateOptions{})

remoteController := &RemoteController{}
remoteController := &RemoteController{
ClusterID: "c1",
}
remoteController.SidecarController = sidecarController

sidecarCacheEgressMap := common.NewSidecarEgressMap()
sidecarCacheEgressMap.Put(
assetIdentity,
"c1",
identityNamespace,
assetFQDN,
nil,
Expand All @@ -2879,6 +2882,7 @@ func TestModifyNonExistingSidecarForLocalClusterCommunication(t *testing.T) {
default:
sidecarCacheEgressMap.Put(
assetIdentity,
"c1",
identityNamespace,
assetFQDN,
map[string]string{
Expand Down Expand Up @@ -2938,17 +2942,15 @@ func TestModifyExistingSidecarForLocalClusterCommunication(t *testing.T) {
}

sidecarController = &istio.SidecarController{}
remoteController = &RemoteController{}
remoteController = &RemoteController{ClusterID: "c1"}
sidecarCacheEgressMap = common.NewSidecarEgressMap()
)
sidecarCacheEgressMap.Put(
assetIdentity,
"c1",
"test-dependency-namespace",
"test-local-fqdn",
map[string]string{
"test.myservice.global": "1",
},
)
map[string]string{})
ctx, cancel := context.WithDeadline(context.Background(), time.Now().Add(5*time.Second))
defer cancel()

Expand All @@ -2966,7 +2968,6 @@ func TestModifyExistingSidecarForLocalClusterCommunication(t *testing.T) {
if createdSidecar != nil {
sidecarEgressMap := make(map[string]common.SidecarEgress)
cnameMap := common.NewMap()
cnameMap.Put("test.myservice.global", "1")
sidecarEgressMap["test-dependency-namespace"] = common.SidecarEgress{Namespace: "test-dependency-namespace", FQDN: "test-local-fqdn", CNAMEs: cnameMap}
modifySidecarForLocalClusterCommunication(ctxLogger, ctx, identityNamespace, assetIdentity, sidecarCacheEgressMap, remoteController)

Expand All @@ -2976,7 +2977,7 @@ func TestModifyExistingSidecarForLocalClusterCommunication(t *testing.T) {
t.Fail()
}

hostList := append(createdSidecar.Spec.Egress[0].Hosts, "test-dependency-namespace/test-local-fqdn", "test-dependency-namespace/test.myservice.global")
hostList := append(createdSidecar.Spec.Egress[0].Hosts, "test-dependency-namespace/test-local-fqdn")
createdSidecar.Spec.Egress[0].Hosts = hostList

// Egress host order doesn't matter but will cause tests to fail. Move these values to their own lists for comparision
Expand Down Expand Up @@ -3039,25 +3040,23 @@ func TestRetryUpdatingSidecar(t *testing.T) {
},
}
sidecarController = &istio.SidecarController{}
remoteController = &RemoteController{}
remoteController = &RemoteController{ClusterID: "c1"}
sidecarCacheEgressMap = common.NewSidecarEgressMap()
)
ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
defer cancel()
sidecarCacheEgressMap.Put(
assetIdentity,
"c1",
"test-dependency-namespace",
"test-local-fqdn",
map[string]string{
"test.myservice.global": "1",
},
map[string]string{},
)
remoteController.SidecarController = sidecarController
sidecarController.IstioClient = istiofake.NewSimpleClientset()
createdSidecar, _ := sidecarController.IstioClient.NetworkingV1alpha3().Sidecars(identityNamespace).Create(context.TODO(), sidecar, metav1.CreateOptions{})
sidecarEgressMap := make(map[string]common.SidecarEgress)
cnameMap := common.NewMap()
cnameMap.Put("test.myservice.global", "1")
sidecarEgressMap["test-dependency-namespace"] = common.SidecarEgress{Namespace: "test-dependency-namespace", FQDN: "test-local-fqdn", CNAMEs: cnameMap}
newSidecar := copySidecar(createdSidecar)
egressHosts := make(map[string]string)
Expand Down Expand Up @@ -3962,7 +3961,7 @@ func TestUpdateEndpointsForBlueGreen(t *testing.T) {

for _, c := range testCases {
t.Run(c.name, func(t *testing.T) {
updateEndpointsForBlueGreen(c.rollout, c.weightedServices, map[string]string{}, c.inputEndpoint, "test", c.meshHost)
updateEndpointsForBlueGreen(c.rollout, c.weightedServices, c.inputEndpoint, "test", c.meshHost)
if c.inputEndpoint.Address != c.wantedEndpoints.Address {
t.Errorf("Wanted %s endpoint, got: %s", c.wantedEndpoints.Address, c.inputEndpoint.Address)
}
Expand Down
2 changes: 1 addition & 1 deletion admiral/pkg/clusters/shard_handler.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ func ProduceIdentityConfigsFromShard(ctxLogger *log.Entry, shard admiralapiv1.Sh
cname := common.GetCnameVal([]string{identityConfigEnv.Name, strings.ToLower(identityConfig.IdentityName), common.GetHostnameSuffix()})
cnames[cname] = "1"
localFqdn := identityConfigEnv.ServiceName + common.Sep + identityConfigEnv.Namespace + common.GetLocalDomainSuffix()
rr.AdmiralCache.DependencyNamespaceCache.Put(clientAsset, identityConfigEnv.Namespace, localFqdn, cnames)
rr.AdmiralCache.DependencyNamespaceCache.Put(clientAsset, identityConfigCluster.Name, identityConfigEnv.Namespace, localFqdn, cnames)
}
}
// Fill the ClusterLocalityCache
Expand Down
23 changes: 14 additions & 9 deletions admiral/pkg/controller/common/types.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ type SidecarEgress struct {

// maintains a map from workload identity -> map[namespace]SidecarEgress
type SidecarEgressMap struct {
cache map[string]map[string]SidecarEgress
cache map[string]map[string]map[string]SidecarEgress
mutex *sync.Mutex
}

Expand Down Expand Up @@ -182,7 +182,7 @@ type Context struct {

func NewSidecarEgressMap() *SidecarEgressMap {
n := new(SidecarEgressMap)
n.cache = make(map[string]map[string]SidecarEgress)
n.cache = make(map[string]map[string]map[string]SidecarEgress)
n.mutex = &sync.Mutex{}
return n
}
Expand Down Expand Up @@ -380,22 +380,27 @@ func (s *Map) GetKeys() []string {
return keys
}

func (s *SidecarEgressMap) Put(identity string, namespace string, fqdn string, cnames map[string]string) {
func (s *SidecarEgressMap) Put(sidentity string, cluster string, namespace string, fqdn string, cnames map[string]string) {
defer s.mutex.Unlock()
s.mutex.Lock()
var mapVal = s.cache[identity]
var mapVal = s.cache[sidentity]
if mapVal == nil {
mapVal = make(map[string]SidecarEgress)
mapVal = make(map[string]map[string]SidecarEgress)
}
nsMap, ok := mapVal[cluster]
if !ok {
nsMap = make(map[string]SidecarEgress)
}
cnameMap := NewMap()
for k, v := range cnames {
cnameMap.Put(k, v)
}
mapVal[namespace] = SidecarEgress{Namespace: namespace, FQDN: fqdn, CNAMEs: cnameMap}
s.cache[identity] = mapVal
nsMap[namespace] = SidecarEgress{Namespace: namespace, FQDN: fqdn, CNAMEs: cnameMap}
mapVal[cluster] = nsMap
s.cache[sidentity] = mapVal
}

func (s *SidecarEgressMap) Get(key string) map[string]SidecarEgress {
func (s *SidecarEgressMap) Get(key string) map[string]map[string]SidecarEgress {
defer s.mutex.Unlock()
s.mutex.Lock()
return s.cache[key]
Expand All @@ -408,7 +413,7 @@ func (s *SidecarEgressMap) Delete(key string) {
}

// Range is a thread safe iterator to iterate through the SidecarEgress map
func (s *SidecarEgressMap) Range(fn func(k string, v map[string]SidecarEgress)) {
func (s *SidecarEgressMap) Range(fn func(k string, v map[string]map[string]SidecarEgress)) {
defer s.mutex.Unlock()
s.mutex.Lock()
for k, v := range s.cache {
Expand Down
12 changes: 6 additions & 6 deletions admiral/pkg/controller/common/types_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,7 @@ func TestMapRange(t *testing.T) {
func TestSidecarEgressGet(t *testing.T) {

egressMap := NewSidecarEgressMap()
egressMap.Put("pkey1", "pkey2", "fqdn", map[string]string{"pkey2": "pkey2"})
egressMap.Put("pkey1", "dkey1", "pkey2", "fqdn", map[string]string{"pkey2": "pkey2"})

ctx, cancel := context.WithDeadline(context.Background(), time.Now().Add(3*time.Second))
defer cancel()
Expand All @@ -242,7 +242,7 @@ func TestSidecarEgressGet(t *testing.T) {
case <-ctx.Done():
return
default:
egressMap.Put("pkey1", string(uuid.NewUUID()), "fqdn", map[string]string{"pkey2": "pkey2"})
egressMap.Put("pkey1", "dkey1", string(uuid.NewUUID()), "fqdn", map[string]string{"pkey2": "pkey2"})
}
}
}(ctx)
Expand All @@ -266,12 +266,12 @@ func TestSidecarEgressGet(t *testing.T) {
func TestSidecarEgressRange(t *testing.T) {

egressMap := NewSidecarEgressMap()
egressMap.Put("pkey1", "pkey2", "fqdn", map[string]string{"pkey2": "pkey2"})
egressMap.Put("pkey2", "pkey2", "fqdn", map[string]string{"pkey2": "pkey2"})
egressMap.Put("pkey3", "pkey2", "fqdn", map[string]string{"pkey2": "pkey2"})
egressMap.Put("pkey1", "dkey1", "pkey2", "fqdn", map[string]string{"pkey2": "pkey2"})
egressMap.Put("pkey2", "dkey2", "pkey2", "fqdn", map[string]string{"pkey2": "pkey2"})
egressMap.Put("pkey3", "dkey3", "pkey2", "fqdn", map[string]string{"pkey2": "pkey2"})

numOfIter := 0
egressMap.Range(func(k string, v map[string]SidecarEgress) {
egressMap.Range(func(k string, v map[string]map[string]SidecarEgress) {
assert.NotNil(t, v)
numOfIter++
})
Expand Down

0 comments on commit 1a95eb0

Please sign in to comment.