remove duplicated routes due to route with retries (#357)

* remove duplicated routes due to route with retries

* Test creating routes for retries

* refactor defaultDependencySettings

Author: Ferdudas97

CHANGELOG.md

@@ -7,6 +7,7 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/).
 
 ### Changed
 - flaky test fixed
+- remove duplicated routes
 
 ## [0.19.26]
 

docs/configuration.md

@@ -184,6 +184,7 @@ Where `<selector>` is one of the following:
 ### Outgoing traffic
 Property                                                                                                | Description                                                                                                                               | Default value
 --------------------------------------------------------------------------------------------------------| -----------------------------------------------------------------------------------------------------------------------------             | ---------
+**envoy-control.envoy.snapshot.retryPolicy.enabled**                                                    | Flag which enables default retries                                                                                                        | true
 **envoy-control.envoy.snapshot.retryPolicy.numberOfRetries**                                            | Number of retries                                                                                                                         | 1
 **envoy-control.envoy.snapshot.retryPolicy.hostSelectionRetryMaxAttempts**                              | The maximum number of times host selection will be reattempted before request being routed to last selected host                          | 3
 **envoy-control.envoy.snapshot.retryPolicy.retryHostPredicate**                                         | Specifies a collection of RetryHostPredicates that will be consulted when selecting a host for retries                                    | a list with one entry "envoy.retry_host_predicates.previous_hosts"

envoy-control-core/src/main/kotlin/pl/allegro/tech/servicemesh/envoycontrol/groups/NodeMetadata.kt

@@ -8,6 +8,8 @@ import io.envoyproxy.controlplane.server.exception.RequestException
 import io.grpc.Status
 import pl.allegro.tech.servicemesh.envoycontrol.groups.ClientWithSelector.Companion.decomposeClient
 import pl.allegro.tech.servicemesh.envoycontrol.snapshot.AccessLogFiltersProperties
+import pl.allegro.tech.servicemesh.envoycontrol.snapshot.CommonHttpProperties
+import pl.allegro.tech.servicemesh.envoycontrol.snapshot.RetryPolicyProperties
 import pl.allegro.tech.servicemesh.envoycontrol.snapshot.SnapshotProperties
 import pl.allegro.tech.servicemesh.envoycontrol.utils.AccessLogFilterParser
 import pl.allegro.tech.servicemesh.envoycontrol.utils.ComparisonFilterSettings
@@ -95,30 +97,40 @@ fun Value?.toHeaderFilter(default: String? = null): HeaderFilterSettings? {
 
 private class RawDependency(val service: String?, val domain: String?, val domainPattern: String?, val value: Value)
 
+private fun defaultRetryPolicy(retryPolicy: RetryPolicyProperties) = if (retryPolicy.enabled) {
+    RetryPolicy(
+        retryOn = retryPolicy.retryOn,
+        numberRetries = retryPolicy.numberOfRetries,
+        retryHostPredicate = retryPolicy.retryHostPredicate,
+        hostSelectionRetryMaxAttempts = retryPolicy.hostSelectionRetryMaxAttempts,
+        rateLimitedRetryBackOff = RateLimitedRetryBackOff(
+            retryPolicy.rateLimitedRetryBackOff.resetHeaders.map { ResetHeader(it.name, it.format) }
+        ),
+        retryBackOff = RetryBackOff(
+            Durations.fromMillis(retryPolicy.retryBackOff.baseInterval.toMillis())
+        ),
+    )
+} else {
+    null
+}
+
+private fun defaultTimeoutPolicy(commonHttpProperties: CommonHttpProperties) = Outgoing.TimeoutPolicy(
+    idleTimeout = Durations.fromMillis(commonHttpProperties.idleTimeout.toMillis()),
+    connectionIdleTimeout = Durations.fromMillis(commonHttpProperties.connectionIdleTimeout.toMillis()),
+    requestTimeout = Durations.fromMillis(commonHttpProperties.requestTimeout.toMillis())
+)
+
+private fun defaultDependencySettings(properties: SnapshotProperties) = DependencySettings(
+    handleInternalRedirect = properties.egress.handleInternalRedirect,
+    timeoutPolicy = defaultTimeoutPolicy(properties.egress.commonHttp),
+    retryPolicy = defaultRetryPolicy(properties.retryPolicy)
+)
+
 fun Value?.toOutgoing(properties: SnapshotProperties): Outgoing {
     val allServiceDependenciesIdentifier = properties.outgoingPermissions.allServicesDependencies.identifier
     val rawDependencies = this?.field("dependencies")?.list().orEmpty().map(::toRawDependency)
     val allServicesDependencies = toAllServiceDependencies(rawDependencies, allServiceDependenciesIdentifier)
-    val defaultSettingsFromProperties = DependencySettings(
-        handleInternalRedirect = properties.egress.handleInternalRedirect,
-        timeoutPolicy = Outgoing.TimeoutPolicy(
-            idleTimeout = Durations.fromMillis(properties.egress.commonHttp.idleTimeout.toMillis()),
-            connectionIdleTimeout = Durations.fromMillis(properties.egress.commonHttp.connectionIdleTimeout.toMillis()),
-            requestTimeout = Durations.fromMillis(properties.egress.commonHttp.requestTimeout.toMillis())
-        ),
-        retryPolicy = RetryPolicy(
-            retryOn = properties.retryPolicy.retryOn,
-            numberRetries = properties.retryPolicy.numberOfRetries,
-            retryHostPredicate = properties.retryPolicy.retryHostPredicate,
-            hostSelectionRetryMaxAttempts = properties.retryPolicy.hostSelectionRetryMaxAttempts,
-            rateLimitedRetryBackOff = RateLimitedRetryBackOff(
-                properties.retryPolicy.rateLimitedRetryBackOff.resetHeaders.map { ResetHeader(it.name, it.format) }
-            ),
-            retryBackOff = RetryBackOff(
-                Durations.fromMillis(properties.retryPolicy.retryBackOff.baseInterval.toMillis())
-            ),
-        )
-    )
+    val defaultSettingsFromProperties = defaultDependencySettings(properties)
     val allServicesDefaultSettings = allServicesDependencies?.value.toSettings(defaultSettingsFromProperties)
     val services = rawDependencies.filter { it.service != null && it.service != allServiceDependenciesIdentifier }
         .map {
@@ -239,27 +251,27 @@ private fun Value?.toSettings(defaultSettings: DependencySettings): DependencySe
     }
 }
 
-private fun mapProtoToRetryPolicy(value: Value, defaultRetryPolicy: RetryPolicy): RetryPolicy {
+private fun mapProtoToRetryPolicy(value: Value, defaultRetryPolicy: RetryPolicy?): RetryPolicy {
     return RetryPolicy(
-        retryOn = value.field("retryOn")?.listValue?.valuesList?.map { it.stringValue } ?: defaultRetryPolicy.retryOn,
+        retryOn = value.field("retryOn")?.listValue?.valuesList?.map { it.stringValue } ?: defaultRetryPolicy?.retryOn,
         hostSelectionRetryMaxAttempts = value.field("hostSelectionRetryMaxAttempts")?.numberValue?.toLong()
-            ?: defaultRetryPolicy.hostSelectionRetryMaxAttempts,
-        numberRetries = value.field("numberRetries")?.numberValue?.toInt() ?: defaultRetryPolicy.numberRetries,
+            ?: defaultRetryPolicy?.hostSelectionRetryMaxAttempts,
+        numberRetries = value.field("numberRetries")?.numberValue?.toInt() ?: defaultRetryPolicy?.numberRetries,
         retryHostPredicate = value.field("retryHostPredicate")?.listValue?.valuesList?.map {
             RetryHostPredicate(it.field("name")!!.stringValue)
-        }?.toList() ?: defaultRetryPolicy.retryHostPredicate,
+        }?.toList() ?: defaultRetryPolicy?.retryHostPredicate,
         perTryTimeoutMs = value.field("perTryTimeoutMs")?.numberValue?.toLong(),
         retryBackOff = value.field("retryBackOff")?.structValue?.let {
             RetryBackOff(
                 baseInterval = it.fieldsMap["baseInterval"]?.toDuration(),
                 maxInterval = it.fieldsMap["maxInterval"]?.toDuration()
             )
-        } ?: defaultRetryPolicy.retryBackOff,
+        } ?: defaultRetryPolicy?.retryBackOff,
         rateLimitedRetryBackOff = value.field("rateLimitedRetryBackOff")?.structValue?.let {
             RateLimitedRetryBackOff(
                 it.fieldsMap["resetHeaders"]?.listValue?.valuesList?.mapNotNull(::mapProtoToResetHeader)
             )
-        } ?: defaultRetryPolicy.rateLimitedRetryBackOff,
+        } ?: defaultRetryPolicy?.rateLimitedRetryBackOff,
         retryableStatusCodes = value.field("retryableStatusCodes")?.listValue?.valuesList?.map {
             it.numberValue.toInt()
         },
@@ -573,11 +585,11 @@ data class DependencySettings(
     val handleInternalRedirect: Boolean = false,
     val timeoutPolicy: Outgoing.TimeoutPolicy = Outgoing.TimeoutPolicy(),
     val rewriteHostHeader: Boolean = false,
-    val retryPolicy: RetryPolicy = RetryPolicy()
+    val retryPolicy: RetryPolicy? = RetryPolicy()
 )
 
 data class RetryPolicy(
-    val retryOn: List<String> = emptyList(),
+    val retryOn: List<String>? = emptyList(),
     val hostSelectionRetryMaxAttempts: Long? = null,
     val numberRetries: Int? = null,
     val retryHostPredicate: List<RetryHostPredicate>? = null,

envoy-control-core/src/main/kotlin/pl/allegro/tech/servicemesh/envoycontrol/snapshot/SnapshotProperties.kt

@@ -372,6 +372,7 @@ data class RateLimitProperties(
 )
 
 data class RetryPolicyProperties(
+    var enabled: Boolean = true,
     var retryOn: List<String> = emptyList(),
     var numberOfRetries: Int = 1,
     var hostSelectionRetryMaxAttempts: Long = 3,

envoy-control-core/src/main/kotlin/pl/allegro/tech/servicemesh/envoycontrol/snapshot/resource/routes/EnvoyEgressRoutesFactory.kt

@@ -72,6 +72,11 @@ class EnvoyEgressRoutesFactory(
             .setValue("%UPSTREAM_REMOTE_ADDRESS%").build()
     )
 
+    private val defaultRouteMatch = RouteMatch
+        .newBuilder()
+        .setPrefix("/")
+        .build()
+
     /**
      * @see TestResources.createRoute
      */
@@ -84,12 +89,7 @@ class EnvoyEgressRoutesFactory(
         val virtualHosts = routes
             .filter { it.routeDomains.isNotEmpty() }
             .map { routeSpecification ->
-                addMultipleRoutes(
-                    VirtualHost.newBuilder()
-                        .setName(routeSpecification.clusterName)
-                        .addAllDomains(routeSpecification.routeDomains),
-                    routeSpecification
-                ).build()
+                buildEgressRoute(routeSpecification)
             }
 
         var routeConfiguration = RouteConfiguration.newBuilder()
@@ -120,37 +120,55 @@ class EnvoyEgressRoutesFactory(
         return routeConfiguration.build()
     }
 
-    private fun addMultipleRoutes(
-        addAllDomains: VirtualHost.Builder,
-        routeSpecification: RouteSpecification
-    ): VirtualHost.Builder {
-        routeSpecification.settings.retryPolicy.let {
-            buildRouteForRetryPolicy(addAllDomains, routeSpecification)
+    private fun buildEgressRoute(routeSpecification: RouteSpecification): VirtualHost {
+        val virtualHost = VirtualHost.newBuilder()
+            .setName(routeSpecification.clusterName)
+            .addAllDomains(routeSpecification.routeDomains)
+        val retryPolicy = routeSpecification.settings.retryPolicy
+        if (retryPolicy != null) {
+            buildEgressRouteWithRetryPolicy(virtualHost, retryPolicy, routeSpecification)
+        } else {
+            virtualHost.addRoutes(
+                Route.newBuilder()
+                    .setMatch(defaultRouteMatch)
+                    .setRoute(createRouteAction(routeSpecification, shouldAddRetryPolicy = false))
+                    .build()
+            )
         }
-        buildDefaultRoute(addAllDomains, routeSpecification)
-        return addAllDomains
+        return virtualHost.build()
     }
 
-    private fun buildRouteForRetryPolicy(
-        addAllDomains: VirtualHost.Builder,
+    private fun buildEgressRouteWithRetryPolicy(
+        virtualHost: VirtualHost.Builder,
+        retryPolicy: EnvoyControlRetryPolicy,
         routeSpecification: RouteSpecification
-    ): VirtualHost.Builder? {
-        val regexAsAString = routeSpecification.settings.retryPolicy.methods?.joinToString(separator = "|")
-        val routeMatchBuilder = RouteMatch
-            .newBuilder()
-            .setPrefix("/")
-            .also { routeMatcher ->
-                regexAsAString?.let {
-                    routeMatcher.addHeaders(buildMethodHeaderMatcher(it))
-                }
-            }
-
-        return addAllDomains.addRoutes(
-            Route.newBuilder()
-                .setMatch(routeMatchBuilder.build())
-                .setRoute(createRouteAction(routeSpecification, shouldAddRetryPolicy = true))
+    ): VirtualHost.Builder {
+        return if (retryPolicy.methods != null) {
+            val regexAsAString = retryPolicy.methods.joinToString(separator = "|")
+            val retryRouteMatch = RouteMatch
+                .newBuilder()
+                .setPrefix("/")
+                .addHeaders(buildMethodHeaderMatcher(regexAsAString))
                 .build()
-        )
+            virtualHost.addRoutes(
+                Route.newBuilder()
+                    .setMatch(retryRouteMatch)
+                    .setRoute(createRouteAction(routeSpecification, shouldAddRetryPolicy = true))
+                    .build()
+            ).addRoutes(
+                Route.newBuilder()
+                    .setMatch(defaultRouteMatch)
+                    .setRoute(createRouteAction(routeSpecification, shouldAddRetryPolicy = false))
+                    .build()
+            )
+        } else {
+            virtualHost.addRoutes(
+                Route.newBuilder()
+                    .setMatch(defaultRouteMatch)
+                    .setRoute(createRouteAction(routeSpecification, shouldAddRetryPolicy = true))
+                    .build()
+            )
+        }
     }
 
     private fun buildMethodHeaderMatcher(regexAsAString: String) = HeaderMatcher.newBuilder()
@@ -162,23 +180,6 @@ class EnvoyEgressRoutesFactory(
                 .build()
         )
 
-    private fun buildDefaultRoute(
-        addAllDomains: VirtualHost.Builder,
-        routeSpecification: RouteSpecification
-    ) {
-        addAllDomains.addRoutes(
-            Route.newBuilder()
-                .setMatch(
-                    RouteMatch.newBuilder()
-                        .setPrefix("/")
-                        .build()
-                )
-                .setRoute(
-                    createRouteAction(routeSpecification)
-                ).build()
-        )
-    }
-
     /**
      * @see TestResources.createRoute
      */
@@ -194,11 +195,7 @@ class EnvoyEgressRoutesFactory(
                     .addAllDomains(routeSpecification.routeDomains)
                     .addRoutes(
                         Route.newBuilder()
-                            .setMatch(
-                                RouteMatch.newBuilder()
-                                    .setPrefix("/")
-                                    .build()
-                            )
+                            .setMatch(defaultRouteMatch)
                             .setRoute(
                                 createRouteAction(routeSpecification)
                             ).build()
@@ -252,7 +249,7 @@ class RequestPolicyMapper private constructor() {
             return retryPolicy?.let { policy ->
                 val retryPolicyBuilder = RetryPolicy.newBuilder()
 
-                policy.retryOn.let { retryPolicyBuilder.setRetryOn(it.joinToString { joined -> joined }) }
+                policy.retryOn?.let { retryPolicyBuilder.setRetryOn(it.joinToString { joined -> joined }) }
                 policy.hostSelectionRetryMaxAttempts?.let {
                     retryPolicyBuilder.setHostSelectionRetryMaxAttempts(it)
                 }

envoy-control-core/src/test/kotlin/pl/allegro/tech/servicemesh/envoycontrol/groups/RoutesAssertions.kt

@@ -22,6 +22,11 @@ fun RouteConfiguration.hasSingleVirtualHostThat(condition: VirtualHost.() -> Uni
     return this
 }
 
+fun RouteConfiguration.hasVirtualHostThat(name: String, condition: VirtualHost.() -> Unit): RouteConfiguration {
+    condition(this.virtualHostsList.find { it.name == name }!!)
+    return this
+}
+
 fun RouteConfiguration.hasRequestHeaderToAdd(key: String, value: String): RouteConfiguration {
     assertThat(this.requestHeadersToAddList).anySatisfy {
         assertThat(it.header.key).isEqualTo(key)
@@ -136,15 +141,22 @@ fun Route.matchingOnPath(path: String): Route {
     return this
 }
 
-fun Route.matchingOnHeader(name: String, value: String): Route {
+fun Route.matchingOnHeader(name: String, value: String, isRegex: Boolean = false): Route {
+    val matcher = RegexMatcher.newBuilder().setRegex(value)
+        .setGoogleRe2(RegexMatcher.GoogleRE2.getDefaultInstance())
+        .build()
     assertThat(this.match.headersList).anyMatch {
-        it.name == name && it.exactMatch == value
+        if (isRegex) {
+            it.name == name && it.safeRegexMatch == matcher
+        } else {
+            it.name == name && it.exactMatch == value
+        }
     }
     return this
 }
 
-fun Route.matchingOnMethod(method: String): Route {
-    return this.matchingOnHeader(":method", method)
+fun Route.matchingOnMethod(method: String, isRegex: Boolean = false): Route {
+    return this.matchingOnHeader(":method", method, isRegex)
 }
 
 fun Route.matchingOnAnyMethod(): Route {
@@ -207,6 +219,10 @@ fun Route.hasNoRetryPolicy() {
     assertThat(this.route.retryPolicy).isEqualTo(RetryPolicy.newBuilder().build())
 }
 
+fun Route.hasRetryPolicy() {
+    assertThat(this.route.hasRetryPolicy()).isTrue()
+}
+
 fun Route.ingressRoute() {
     this.matchingOnPrefix("/")
         .publicAccess()