Skip to content

Commit

Permalink
Fixed issues from comments
Browse files Browse the repository at this point in the history
  • Loading branch information
@nastassia-dailidava
nastassia-dailidava committed Apr 24, 2024
1 parent 51d3df9 commit c470838
Show file tree
Hide file tree
Showing 6 changed files with 52 additions and 16 deletions.
3 changes: 2 additions & 1 deletion docs/configuration.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,8 @@ Property
**envoy-control.envoy.snapshot.load-balancing.policy** | load balancing policy used for clusters. [Accepted values](https://www.envoyproxy.io/docs/envoy/latest/api-v3/config/cluster/v3/cluster.proto#enum-config-cluster-v3-cluster-lbpolicy) | LEAST_REQUEST
**envoy-control.envoy.snapshot.load-balancing.use-keys-subset-fallback-policy** | KEYS_SUBSET fallback policy is used by default when canary and service-tags are enabled. It is not supported in Envoy <= 1.12.x. Set to false for compatibility with Envoy 1.12.x | true
**envoy-control.envoy.snapshot.load-balancing.traffic-splitting.zoneName** | a zone to which traffic will be routed if traffic splitting is enabled | ""
**envoy-control.envoy.snapshot.load-balancing.traffic-splitting.weights-by-service-properties.** | a map that maps service name to a map [zoneName: weight] | empty map
**envoy-control.envoy.snapshot.load-balancing.traffic-splitting.zonesAllowingTrafficSplitting** | a zone from which traffic should be splitted | empty list
**envoy-control.envoy.snapshot.load-balancing.traffic-splitting.weights-by-service.** | a map that maps service name to a map [zoneName: weight] | empty map

## Routing

Expand Down
10 changes: 8 additions & 2 deletions .../allegro/tech/servicemesh/envoycontrol/snapshot/resource/clusters/EnvoyClustersFactory.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -283,11 +283,17 @@ class EnvoyClustersFactory(
): Boolean {
val trafficSplitting = properties.loadBalancing.trafficSplitting
val trafficSplitEnabled = trafficSplitting.weightsByService.containsKey(serviceName)
val allowed = clusterLoadAssignment != null &&
properties.loadBalancing.trafficSplitting.zonesAllowingTrafficSplitting.contains(currentZone)
val allowed = clusterLoadAssignment != null && hasEndpointsInZone(clusterLoadAssignment)
properties.loadBalancing.trafficSplitting.zonesAllowingTrafficSplitting.contains(currentZone)
return trafficSplitEnabled && allowed
}

private fun hasEndpointsInZone(
clusterLoadAssignment: ClusterLoadAssignment?
) = clusterLoadAssignment?.endpointsList
?.any { e -> properties.loadBalancing.trafficSplitting.zoneName == e.locality.zone && e.lbEndpointsCount > 0 }
?: false

private fun shouldAddDynamicForwardProxyCluster(group: Group) =
group.proxySettings.outgoing.getDomainPatternDependencies().isNotEmpty()

Expand Down
30 changes: 28 additions & 2 deletions ...egro/tech/servicemesh/envoycontrol/snapshot/resource/clusters/EnvoyClustersFactoryTest.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package pl.allegro.tech.servicemesh.envoycontrol.snapshot.resource.clusters
import io.envoyproxy.controlplane.cache.SnapshotResources
import io.envoyproxy.envoy.config.cluster.v3.Cluster
import io.envoyproxy.envoy.config.endpoint.v3.ClusterLoadAssignment
import io.envoyproxy.envoy.config.endpoint.v3.LocalityLbEndpoints
import org.assertj.core.api.Assertions.assertThat
import org.junit.jupiter.api.Test
import pl.allegro.tech.servicemesh.envoycontrol.groups.DependencySettings
Expand All @@ -17,6 +18,7 @@ import pl.allegro.tech.servicemesh.envoycontrol.utils.TRAFFIC_SPLITTING_ZONE
import pl.allegro.tech.servicemesh.envoycontrol.utils.createAllServicesGroup
import pl.allegro.tech.servicemesh.envoycontrol.utils.createCluster
import pl.allegro.tech.servicemesh.envoycontrol.utils.createClusterConfigurations
import pl.allegro.tech.servicemesh.envoycontrol.utils.createEndpoints
import pl.allegro.tech.servicemesh.envoycontrol.utils.createListenersConfig
import pl.allegro.tech.servicemesh.envoycontrol.utils.createLoadAssignments
import pl.allegro.tech.servicemesh.envoycontrol.utils.createServicesGroup
Expand Down Expand Up @@ -114,15 +116,39 @@ internal class EnvoyClustersFactoryTest {
}
}

@Test
fun `should not apply locality weighted config if there are no endpoints in the ts zone`() {
val cluster1 = createCluster(snapshotPropertiesWithWeights, CLUSTER_NAME1)
val factory = EnvoyClustersFactory(snapshotPropertiesWithWeights, CURRENT_ZONE)
val result = factory.getClustersForGroup(
createServicesGroup(
snapshotProperties = snapshotPropertiesWithWeights,
listenersConfig = createListenersConfig(snapshotPropertiesWithWeights, true),
dependencies = arrayOf(CLUSTER_NAME1 to null),
),
createGlobalSnapshot(cluster1, endpoints = null)
)
assertThat(result)
.anySatisfy {
assertThat(it.name).isEqualTo(CLUSTER_NAME1)
assertThat(it.edsClusterConfig).isEqualTo(cluster1.edsClusterConfig)
assertThat(it.commonLbConfig.hasLocalityWeightedLbConfig()).isFalse()
}
}

private fun createGlobalSnapshot(
vararg clusters: Cluster,
securedClusters: List<Cluster> = clusters.asList()
securedClusters: List<Cluster> = clusters.asList(),
endpoints: List<LocalityLbEndpoints>? = createEndpoints()
): GlobalSnapshot {
val clusterLoadAssignment = endpoints
?.let { createLoadAssignments(clusters.toList(), endpoints) }
?: createLoadAssignments(clusters.toList(), listOf())
return GlobalSnapshot(
SnapshotResources.create<Cluster>(clusters.toList(), "pl/allegro/tech/servicemesh/envoycontrol/v3")
.resources(),
clusters.map { it.name }.toSet(),
SnapshotResources.create<ClusterLoadAssignment>(createLoadAssignments(clusters.toList()), "v1").resources(),
SnapshotResources.create<ClusterLoadAssignment>(clusterLoadAssignment, "v1").resources(),
createClusterConfigurations(),
SnapshotResources.create<Cluster>(securedClusters, "v3").resources()
)
Expand Down
7 changes: 5 additions & 2 deletions ...ore/src/test/kotlin/pl/allegro/tech/servicemesh/envoycontrol/utils/EndpointsOperations.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,14 @@ import io.envoyproxy.envoy.config.endpoint.v3.ClusterLoadAssignment
import io.envoyproxy.envoy.config.endpoint.v3.LbEndpoint
import io.envoyproxy.envoy.config.endpoint.v3.LocalityLbEndpoints

fun createLoadAssignments(clusters: List<Cluster>): List<ClusterLoadAssignment> {
fun createLoadAssignments(
clusters: List<Cluster>,
endpoints: List<LocalityLbEndpoints>
): List<ClusterLoadAssignment> {
return clusters.map {
ClusterLoadAssignment.newBuilder()
.setClusterName(it.name)
.addAllEndpoints(createEndpoints())
.addAllEndpoints(endpoints)
.build()
}
}
Expand Down
12 changes: 6 additions & 6 deletions ...legro/tech/servicemesh/envoycontrol/trafficsplitting/LocalityWeightedLoadBalancingTest.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,12 @@ class LocalityWeightedLoadBalancingTest {
private val properties = mapOf(
"envoy-control.envoy.snapshot.stateSampleDuration" to Duration.ZERO,
"envoy-control.sync.enabled" to true,
"envoy-control.envoy.snapshot.load-balancing.trafficSplitting.zoneName" to FORCE_TRAFFIC_ZONE,
"envoy-control.envoy.snapshot.load-balancing.trafficSplitting.zonesAllowingTrafficSplitting" to listOf("dc1"),
"envoy-control.envoy.snapshot.load-balancing.trafficSplitting.weightsByService.$SERVICE_NAME.weightByZone.dc1" to 30,
"envoy-control.envoy.snapshot.load-balancing.trafficSplitting.weightsByService.$SERVICE_NAME.weightByZone.dc2" to 10,
"envoy-control.envoy.snapshot.load-balancing.trafficSplitting.weightsByService.$SERVICE_NAME.weightByZone.dc3" to 1,
"envoy-control.envoy.snapshot.load-balancing.priorities.zonePriorities" to DEFAULT_PRIORITIES
"envoy-control.envoy.snapshot.loadBalancing.trafficSplitting.zoneName" to FORCE_TRAFFIC_ZONE,
"envoy-control.envoy.snapshot.loadBalancing.trafficSplitting.zonesAllowingTrafficSplitting" to listOf("dc1"),
"envoy-control.envoy.snapshot.loadBalancing.trafficSplitting.weightsByService.$SERVICE_NAME.weightByZone.dc1" to 30,
"envoy-control.envoy.snapshot.loadBalancing.trafficSplitting.weightsByService.$SERVICE_NAME.weightByZone.dc2" to 10,
"envoy-control.envoy.snapshot.loadBalancing.trafficSplitting.weightsByService.$SERVICE_NAME.weightByZone.dc3" to 1,
"envoy-control.envoy.snapshot.loadBalancing.priorities.zonePriorities" to DEFAULT_PRIORITIES
)

private val echo2Config = """
Expand Down
6 changes: 3 additions & 3 deletions ...icemesh/envoycontrol/trafficsplitting/LocalityWeightedLoadBalancingUnlistedServiceTest.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,9 @@ class LocalityWeightedLoadBalancingUnlistedServiceTest {
private val properties = mapOf(
"envoy-control.envoy.snapshot.stateSampleDuration" to Duration.ZERO,
"envoy-control.sync.enabled" to true,
"envoy-control.envoy.snapshot.load-balancing.trafficSplitting.zoneName" to FORCE_TRAFFIC_ZONE,
"envoy-control.envoy.snapshot.load-balancing.trafficSplitting.zonesAllowingTrafficSplitting" to listOf("dc1"),
"envoy-control.envoy.snapshot.load-balancing.priorities.zonePriorities" to DEFAULT_PRIORITIES
"envoy-control.envoy.snapshot.loadBalancing.trafficSplitting.zoneName" to FORCE_TRAFFIC_ZONE,
"envoy-control.envoy.snapshot.loadBalancing.trafficSplitting.zonesAllowingTrafficSplitting" to listOf("dc1"),
"envoy-control.envoy.snapshot.loadBalancing.priorities.zonePriorities" to DEFAULT_PRIORITIES
)

private val echo2Config = """
Expand Down

0 comments on commit c470838

Please sign in to comment.