diff --git a/CHANGELOG.md b/CHANGELOG.md index 12127b58e..ce4348dd5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/). ### Changed - Add possibility to set dependency to group of services by tag mechanism - flaky test fixed +- remove duplicated routes ## [0.19.26] diff --git a/docs/configuration.md b/docs/configuration.md index 4da54ea06..c29102dfe 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -184,6 +184,7 @@ Where `` 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" diff --git a/envoy-control-core/src/main/kotlin/pl/allegro/tech/servicemesh/envoycontrol/groups/NodeMetadata.kt b/envoy-control-core/src/main/kotlin/pl/allegro/tech/servicemesh/envoycontrol/groups/NodeMetadata.kt index 6437aad7b..159829b66 100644 --- a/envoy-control-core/src/main/kotlin/pl/allegro/tech/servicemesh/envoycontrol/groups/NodeMetadata.kt +++ b/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 @@ -101,30 +103,40 @@ private class RawDependency( 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 { @@ -259,27 +271,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() }, @@ -610,11 +622,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 = emptyList(), + val retryOn: List? = emptyList(), val hostSelectionRetryMaxAttempts: Long? = null, val numberRetries: Int? = null, val retryHostPredicate: List? = null, diff --git a/envoy-control-core/src/main/kotlin/pl/allegro/tech/servicemesh/envoycontrol/snapshot/SnapshotProperties.kt b/envoy-control-core/src/main/kotlin/pl/allegro/tech/servicemesh/envoycontrol/snapshot/SnapshotProperties.kt index 49f7ca974..c61da55b5 100644 --- a/envoy-control-core/src/main/kotlin/pl/allegro/tech/servicemesh/envoycontrol/snapshot/SnapshotProperties.kt +++ b/envoy-control-core/src/main/kotlin/pl/allegro/tech/servicemesh/envoycontrol/snapshot/SnapshotProperties.kt @@ -373,6 +373,7 @@ data class RateLimitProperties( ) data class RetryPolicyProperties( + var enabled: Boolean = true, var retryOn: List = emptyList(), var numberOfRetries: Int = 1, var hostSelectionRetryMaxAttempts: Long = 3, diff --git a/envoy-control-core/src/main/kotlin/pl/allegro/tech/servicemesh/envoycontrol/snapshot/resource/routes/EnvoyEgressRoutesFactory.kt b/envoy-control-core/src/main/kotlin/pl/allegro/tech/servicemesh/envoycontrol/snapshot/resource/routes/EnvoyEgressRoutesFactory.kt index 4aed85859..01502311e 100644 --- a/envoy-control-core/src/main/kotlin/pl/allegro/tech/servicemesh/envoycontrol/snapshot/resource/routes/EnvoyEgressRoutesFactory.kt +++ b/envoy-control-core/src/main/kotlin/pl/allegro/tech/servicemesh/envoycontrol/snapshot/resource/routes/EnvoyEgressRoutesFactory.kt @@ -74,6 +74,11 @@ class EnvoyEgressRoutesFactory( .setValue("%UPSTREAM_REMOTE_ADDRESS%").build() ) + private val defaultRouteMatch = RouteMatch + .newBuilder() + .setPrefix("/") + .build() + /** * @see TestResources.createRoute */ @@ -86,12 +91,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() @@ -122,37 +122,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() @@ -164,23 +182,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 */ @@ -196,11 +197,7 @@ class EnvoyEgressRoutesFactory( .addAllDomains(routeSpecification.routeDomains) .addRoutes( Route.newBuilder() - .setMatch( - RouteMatch.newBuilder() - .setPrefix("/") - .build() - ) + .setMatch(defaultRouteMatch) .setRoute( createRouteAction(routeSpecification) ).build() @@ -254,7 +251,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) } diff --git a/envoy-control-core/src/test/kotlin/pl/allegro/tech/servicemesh/envoycontrol/groups/RoutesAssertions.kt b/envoy-control-core/src/test/kotlin/pl/allegro/tech/servicemesh/envoycontrol/groups/RoutesAssertions.kt index 909a359c0..ec252e7d5 100644 --- a/envoy-control-core/src/test/kotlin/pl/allegro/tech/servicemesh/envoycontrol/groups/RoutesAssertions.kt +++ b/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() diff --git a/envoy-control-core/src/test/kotlin/pl/allegro/tech/servicemesh/envoycontrol/snapshot/resource/routes/EnvoyEgressRoutesFactoryTest.kt b/envoy-control-core/src/test/kotlin/pl/allegro/tech/servicemesh/envoycontrol/snapshot/resource/routes/EnvoyEgressRoutesFactoryTest.kt index 52a34b493..dc3d2b8d8 100644 --- a/envoy-control-core/src/test/kotlin/pl/allegro/tech/servicemesh/envoycontrol/snapshot/resource/routes/EnvoyEgressRoutesFactoryTest.kt +++ b/envoy-control-core/src/test/kotlin/pl/allegro/tech/servicemesh/envoycontrol/snapshot/resource/routes/EnvoyEgressRoutesFactoryTest.kt @@ -1,20 +1,28 @@ package pl.allegro.tech.servicemesh.envoycontrol.snapshot.resource.routes import com.google.protobuf.util.Durations +import org.junit.jupiter.api.Assertions.assertEquals import org.junit.jupiter.api.Test import pl.allegro.tech.servicemesh.envoycontrol.groups.DependencySettings import pl.allegro.tech.servicemesh.envoycontrol.groups.Outgoing +import pl.allegro.tech.servicemesh.envoycontrol.groups.RetryPolicy import pl.allegro.tech.servicemesh.envoycontrol.groups.hasCustomIdleTimeout import pl.allegro.tech.servicemesh.envoycontrol.groups.hasCustomRequestTimeout import pl.allegro.tech.servicemesh.envoycontrol.groups.hasHostRewriteHeader import pl.allegro.tech.servicemesh.envoycontrol.groups.hasNoRequestHeaderToAdd +import pl.allegro.tech.servicemesh.envoycontrol.groups.hasNoRetryPolicy import pl.allegro.tech.servicemesh.envoycontrol.groups.hasRequestHeaderToAdd import pl.allegro.tech.servicemesh.envoycontrol.groups.hasRequestHeadersToRemove import pl.allegro.tech.servicemesh.envoycontrol.groups.hasResponseHeaderToAdd +import pl.allegro.tech.servicemesh.envoycontrol.groups.hasRetryPolicy +import pl.allegro.tech.servicemesh.envoycontrol.groups.hasVirtualHostThat import pl.allegro.tech.servicemesh.envoycontrol.groups.hasVirtualHostsInOrder import pl.allegro.tech.servicemesh.envoycontrol.groups.hostRewriteHeaderIsEmpty import pl.allegro.tech.servicemesh.envoycontrol.snapshot.EgressProperties import pl.allegro.tech.servicemesh.envoycontrol.snapshot.IncomingPermissionsProperties +import pl.allegro.tech.servicemesh.envoycontrol.groups.matchingOnAnyMethod +import pl.allegro.tech.servicemesh.envoycontrol.groups.matchingOnMethod +import pl.allegro.tech.servicemesh.envoycontrol.groups.matchingOnPrefix import pl.allegro.tech.servicemesh.envoycontrol.snapshot.RouteSpecification import pl.allegro.tech.servicemesh.envoycontrol.snapshot.SnapshotProperties @@ -30,7 +38,8 @@ internal class EnvoyEgressRoutesFactoryTest { idleTimeout = Durations.fromSeconds(10L), requestTimeout = Durations.fromSeconds(10L) ), - rewriteHostHeader = true + rewriteHostHeader = true, + retryPolicy = RetryPolicy() ) ) ) @@ -204,4 +213,86 @@ internal class EnvoyEgressRoutesFactoryTest { ) routeConfig.hasRequestHeadersToRemove(listOf("x-special-case-header", "x-custom")) } + + @Test + fun `should create route config and apply retry police only to specified methods`() { + // given + val routesFactory = EnvoyEgressRoutesFactory(SnapshotProperties()) + val retryPolicy = RetryPolicy(methods = setOf("GET", "POST")) + val routesSpecifications = listOf( + RouteSpecification("example", listOf("example.pl:1553"), DependencySettings(retryPolicy = retryPolicy)), + ) + + val routeConfig = routesFactory.createEgressRouteConfig( + "test", + routesSpecifications, + routeName = "retry", + addUpstreamAddressHeader = false + ) + + routeConfig.hasVirtualHostThat("example") { + assertEquals(2, routesCount) + val retryRoute = getRoutes(0) + val defaultRoute = getRoutes(1) + + retryRoute.hasRetryPolicy() + retryRoute.matchingOnMethod("GET|POST", true) + retryRoute.matchingOnPrefix("/") + + defaultRoute.hasNoRetryPolicy() + defaultRoute.matchingOnAnyMethod() + defaultRoute.matchingOnPrefix("/") + } + } + + @Test + fun `should create route config and apply retry policy without specified methods to default prefix and all method`() { + // given + val routesFactory = EnvoyEgressRoutesFactory(SnapshotProperties()) + val retryPolicy = RetryPolicy(numberRetries = 3) + val routesSpecifications = listOf( + RouteSpecification("example", listOf("example.pl:1553"), DependencySettings(retryPolicy = retryPolicy)), + ) + + val routeConfig = routesFactory.createEgressRouteConfig( + "test", + routesSpecifications, + routeName = "retry", + addUpstreamAddressHeader = false + ) + + routeConfig.hasVirtualHostThat("example") { + assertEquals(1, routesCount) + val defaultRoute = getRoutes(0) + + defaultRoute.hasRetryPolicy() + defaultRoute.matchingOnAnyMethod() + defaultRoute.matchingOnPrefix("/") + } + } + + @Test + fun `should create route config and not apply retry policy if it is not specified`() { + // given + val routesFactory = EnvoyEgressRoutesFactory(SnapshotProperties()) + val routesSpecifications = listOf( + RouteSpecification("example", listOf("example.pl:1553"), DependencySettings()), + ) + + val routeConfig = routesFactory.createEgressRouteConfig( + "test", + routesSpecifications, + routeName = "retry", + addUpstreamAddressHeader = false + ) + + routeConfig.hasVirtualHostThat("example") { + assertEquals(1, routesCount) + val defaultRoute = getRoutes(0) + + defaultRoute.hasNoRetryPolicy() + defaultRoute.matchingOnAnyMethod() + defaultRoute.matchingOnPrefix("/") + } + } }