normalize path (#407)

* normalize path

* unescape action

* change pathWithEscapedSlashesAction to unchanged

* configurable path normalization

* fix linter

* #292 Traffic splitting - Updated param name (#399)

* #292 Traffic splitting - Updated param name

(cherry picked from commit 1abb570)

* fix linter

* fix validator

* add default value for path normalization in tests

---------

Co-authored-by: Nastassia Dailidava <[email protected]>

Author: Ferdudas97

CHANGELOG.md

@@ -2,7 +2,17 @@
 
 Lists all changes with user impact.
 The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/).
-## [0.20.4]
+## [0.20.9]
+### Changed
+- Configurable path normalization
+
+## [0.20.6 - 0.20.8]
+
+### Changed
+- Merge slashes in http request
+- Feature flag for auditing global snapshot
+
+## [0.20.5]
 
 ### Changed
 - Added possibility to add response header for weighted secondary cluster 
@@ -23,6 +33,12 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/).
 - Updated property names:  secondaryClusterPostfix is changed to secondaryClusterSuffix, 
 - aggregateClusterPostfix is changed to aggregateClusterSuffix
 
+## [0.20.2]
+
+### Changed
+- Updated property names:  secondaryClusterPostfix is changed to secondaryClusterSuffix, 
+- aggregateClusterPostfix is changed to aggregateClusterSuffix
+
 ## [0.20.1]
 
 ### Changed

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

@@ -7,6 +7,7 @@ sealed class Group {
     abstract val serviceName: String
     abstract val discoveryServiceName: String?
     abstract val proxySettings: ProxySettings
+    abstract val pathNormalizationConfig: PathNormalizationConfig
     abstract val listenersConfig: ListenersConfig?
 }
 
@@ -15,17 +16,25 @@ data class ServicesGroup(
     override val serviceName: String = "",
     override val discoveryServiceName: String? = null,
     override val proxySettings: ProxySettings = ProxySettings(),
-    override val listenersConfig: ListenersConfig? = null
+    override val pathNormalizationConfig: PathNormalizationConfig = PathNormalizationConfig(),
+    override val listenersConfig: ListenersConfig? = null,
 ) : Group()
 
 data class AllServicesGroup(
     override val communicationMode: CommunicationMode,
     override val serviceName: String = "",
     override val discoveryServiceName: String? = null,
     override val proxySettings: ProxySettings = ProxySettings(),
+    override val pathNormalizationConfig: PathNormalizationConfig = PathNormalizationConfig(),
     override val listenersConfig: ListenersConfig? = null
 ) : Group()
 
+data class PathNormalizationConfig(
+    val normalizationEnabled: Boolean? = null,
+    val mergeSlashes: Boolean? = null,
+    val pathWithEscapedSlashesAction: String? = null
+)
+
 data class ListenersConfig(
     val ingressHost: String,
     val ingressPort: Int,

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

@@ -172,14 +172,14 @@ class MetadataNodeGroup(
         val discoveryServiceName = nodeMetadata.discoveryServiceName
         val proxySettings = proxySettings(nodeMetadata)
         val listenersConfig = createListenersConfig(node.id, node.metadata, node.userAgentBuildVersion)
-
         return when {
             hasAllServicesDependencies(nodeMetadata) ->
                 AllServicesGroup(
                     nodeMetadata.communicationMode,
                     serviceName,
                     discoveryServiceName,
                     proxySettings,
+                    nodeMetadata.pathNormalizationConfig,
                     listenersConfig
                 )
             else ->
@@ -188,6 +188,7 @@ class MetadataNodeGroup(
                     serviceName,
                     discoveryServiceName,
                     proxySettings,
+                    nodeMetadata.pathNormalizationConfig,
                     listenersConfig
                 )
         }

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

@@ -33,6 +33,7 @@ class NodeMetadata(metadata: Struct, properties: SnapshotProperties) {
 
     val communicationMode = getCommunicationMode(metadata.fieldsMap["ads"])
 
+    val pathNormalizationConfig = getPathNormalization(metadata.fieldsMap["path_normalization"], properties)
     val proxySettings: ProxySettings = ProxySettings(metadata.fieldsMap["proxy_settings"], properties)
 }
 
@@ -68,6 +69,23 @@ data class ProxySettings(
     )
 }
 
+fun getPathNormalization(proto: Value?, snapshotProperties: SnapshotProperties): PathNormalizationConfig {
+    val defaultNormalizationConfig = PathNormalizationConfig(
+        snapshotProperties.pathNormalization.enabled,
+        snapshotProperties.pathNormalization.mergeSlashes,
+        snapshotProperties.pathNormalization.pathWithEscapedSlashesAction
+    )
+    if (proto == null) {
+        return defaultNormalizationConfig
+    }
+    return PathNormalizationConfig(
+        normalizationEnabled = proto.field("enabled")?.boolValue ?: defaultNormalizationConfig.normalizationEnabled,
+        mergeSlashes = proto.field("merge_slashes")?.boolValue ?: defaultNormalizationConfig.mergeSlashes,
+        pathWithEscapedSlashesAction = proto.field("path_with_escaped_slashes_action")?.stringValue
+            ?: defaultNormalizationConfig.pathWithEscapedSlashesAction
+    )
+}
+
 private fun getCommunicationMode(proto: Value?): CommunicationMode {
     val ads = proto
         ?.boolValue
@@ -366,9 +384,11 @@ fun Value.toIncomingEndpoint(properties: SnapshotProperties): IncomingEndpoint {
         pathPrefix != null -> IncomingEndpoint(
             pathPrefix, PathMatchingType.PATH_PREFIX, methods, clients, unlistedClientsPolicy, oauth
         )
+
         pathRegex != null -> IncomingEndpoint(
             pathRegex, PathMatchingType.PATH_REGEX, methods, clients, unlistedClientsPolicy, oauth
         )
+
         else -> throw NodeMetadataValidationException("One of 'path', 'pathPrefix' or 'pathRegex' field is required")
     }
 }
@@ -391,9 +411,11 @@ fun Value.toIncomingRateLimitEndpoint(): IncomingRateLimitEndpoint {
         pathPrefix != null -> IncomingRateLimitEndpoint(
             pathPrefix, PathMatchingType.PATH_PREFIX, methods, clients, rateLimit
         )
+
         pathRegex != null -> IncomingRateLimitEndpoint(
             pathRegex, PathMatchingType.PATH_REGEX, methods, clients, rateLimit
         )
+
         else -> throw NodeMetadataValidationException("One of 'path', 'pathPrefix' or 'pathRegex' field is required")
     }
 }
@@ -473,13 +495,15 @@ fun Value.toDuration(): Duration? {
             "Timeout definition has number format" +
                 " but should be in string format and ends with 's'"
         )
+
         Value.KindCase.STRING_VALUE -> {
             try {
                 this.stringValue?.takeIf { it.isNotBlank() }?.let { Durations.parse(it) }
             } catch (ex: ParseException) {
                 throw NodeMetadataValidationException("Timeout definition has incorrect format: ${ex.message}")
             }
         }
+
         else -> null
     }
 }

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

@@ -1,6 +1,7 @@
 package pl.allegro.tech.servicemesh.envoycontrol.groups
 
 import io.envoyproxy.controlplane.server.DiscoveryServerCallbacks
+import io.envoyproxy.envoy.extensions.filters.network.http_connection_manager.v3.HttpConnectionManager
 import pl.allegro.tech.servicemesh.envoycontrol.groups.CommunicationMode.ADS
 import pl.allegro.tech.servicemesh.envoycontrol.groups.CommunicationMode.XDS
 import pl.allegro.tech.servicemesh.envoycontrol.logger
@@ -40,6 +41,9 @@ class ServiceNameNotProvidedException : NodeMetadataValidationException(
     "Service name has not been provided."
 )
 
+class InvalidPathWithEscapedSlashesAction(action: String) : NodeMetadataValidationException(
+    "$action is invalid value for pathWithEscapedSlashesAction."
+)
 class RateLimitIncorrectValidationException(rateLimit: String?) : NodeMetadataValidationException(
     "Rate limit value: $rateLimit is incorrect."
 )
@@ -82,11 +86,24 @@ class NodeMetadataValidator(
     private fun validateMetadata(metadata: NodeMetadata) {
         validateServiceName(metadata)
         validateDependencies(metadata)
+        validatePathNormalization(metadata)
         validateIncomingEndpoints(metadata)
         validateIncomingRateLimitEndpoints(metadata)
         validateConfigurationMode(metadata)
     }
 
+    private fun validatePathNormalization(metadata: NodeMetadata) {
+        val action = metadata.pathNormalizationConfig.pathWithEscapedSlashesAction
+
+        if (action != null) {
+            val actionIsValidEnumValue = HttpConnectionManager.PathWithEscapedSlashesAction.values()
+                .any { it.name.uppercase() == action.uppercase() }
+            if (!actionIsValidEnumValue) {
+                throw InvalidPathWithEscapedSlashesAction(action)
+            }
+        }
+    }
+
     private fun validateServiceName(metadata: NodeMetadata) {
         if (properties.requireServiceName) {
             if (metadata.serviceName.isNullOrEmpty()) {

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

@@ -13,6 +13,7 @@ import java.time.Duration
 class SnapshotProperties {
     var routes = RoutesProperties()
     var localService = LocalServiceProperties()
+    var pathNormalization = PathNormalizationProperties()
     var egress = EgressProperties()
     var ingress = IngressProperties()
     var incomingPermissions = IncomingPermissionsProperties()
@@ -39,6 +40,11 @@ class SnapshotProperties {
     var shouldAuditGlobalSnapshot: Boolean = true
 }
 
+class PathNormalizationProperties {
+    var enabled = true
+    var mergeSlashes = true
+    var pathWithEscapedSlashesAction = "KEEP_UNCHANGED"
+}
 class MetricsProperties {
     var cacheSetSnapshot = false
 }

envoy-control-core/src/main/kotlin/pl/allegro/tech/servicemesh/envoycontrol/snapshot/resource/listeners/filters/HttpConnectionManagerFactory.kt

@@ -51,6 +51,7 @@ class HttpConnectionManagerFactory(
     ): HttpConnectionManager? {
         val listenersConfig = group.listenersConfig!!
 
+        val normalizationConfig = group.pathNormalizationConfig
         val connectionManagerBuilder = HttpConnectionManager.newBuilder()
             .setStatPrefix(statPrefix)
             .setRds(setupRds(group.communicationMode, initialFetchTimeout, routeConfigName))
@@ -66,15 +67,22 @@ class HttpConnectionManagerFactory(
                     .setUseRemoteAddress(BoolValue.newBuilder().setValue(listenersConfig.useRemoteAddress).build())
                     .setDelayedCloseTimeout(Duration.newBuilder().setSeconds(0).build())
                     .setCommonHttpProtocolOptions(httpProtocolOptions)
-                    .setMergeSlashes(true)
                     .setCodecType(HttpConnectionManager.CodecType.AUTO)
                     .setHttpProtocolOptions(ingressHttp1ProtocolOptions(group.serviceName))
+
+                normalizationConfig.normalizationEnabled
+                    ?.let { connectionManagerBuilder.setNormalizePath(BoolValue.newBuilder().setValue(it).build()) }
+                normalizationConfig.mergeSlashes?.let { connectionManagerBuilder.setMergeSlashes(it) }
+                normalizationConfig.pathWithEscapedSlashesAction?.toPathWithEscapedSlashesActionEnum()?.let {
+                    connectionManagerBuilder.setPathWithEscapedSlashesAction(it)
+                }
                 if (listenersConfig.useRemoteAddress) {
                     connectionManagerBuilder.setXffNumTrustedHops(
                         snapshotProperties.dynamicListeners.httpFilters.ingressXffNumTrustedHops
                     )
                 }
             }
+
             Direction.EGRESS -> {
                 connectionManagerBuilder
                     .setHttpProtocolOptions(
@@ -106,6 +114,12 @@ class HttpConnectionManagerFactory(
         return connectionManagerBuilder.build()
     }
 
+    private fun String.toPathWithEscapedSlashesActionEnum(): HttpConnectionManager.PathWithEscapedSlashesAction {
+        return HttpConnectionManager.PathWithEscapedSlashesAction.values()
+            .find { it.name.uppercase() == this.uppercase() }
+            ?: HttpConnectionManager.PathWithEscapedSlashesAction.UNRECOGNIZED
+    }
+
     private fun setupRds(
         communicationMode: CommunicationMode,
         rdsInitialFetchTimeout: Duration,

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

@@ -17,6 +17,7 @@ import pl.allegro.tech.servicemesh.envoycontrol.groups.Group
 import pl.allegro.tech.servicemesh.envoycontrol.groups.IncomingRateLimitEndpoint
 import pl.allegro.tech.servicemesh.envoycontrol.groups.ListenersConfig
 import pl.allegro.tech.servicemesh.envoycontrol.groups.Outgoing
+import pl.allegro.tech.servicemesh.envoycontrol.groups.PathNormalizationConfig
 import pl.allegro.tech.servicemesh.envoycontrol.groups.ProxySettings
 import pl.allegro.tech.servicemesh.envoycontrol.groups.ServicesGroup
 import pl.allegro.tech.servicemesh.envoycontrol.groups.with
@@ -405,6 +406,7 @@ class EnvoySnapshotFactoryTest {
                 serviceDependencies = serviceDependencies(*dependencies),
                 rateLimitEndpoints = rateLimitEndpoints
             ),
+            PathNormalizationConfig(),
             listenersConfig
         )
     }
@@ -430,6 +432,7 @@ class EnvoySnapshotFactoryTest {
                 serviceDependencies = serviceDependencies(*dependencies),
                 defaultServiceSettings = defaultServiceSettings
             ),
+            PathNormalizationConfig(),
             listenersConfig
         )
     }

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

@@ -25,6 +25,11 @@ import io.envoyproxy.envoy.config.core.v3.Node as NodeV3
 
 class MetadataNodeGroupTest {
 
+    private val defaultNormalizationConfig = PathNormalizationConfig(
+        normalizationEnabled = true,
+        mergeSlashes = true,
+        pathWithEscapedSlashesAction = "KEEP_UNCHANGED"
+    )
     @Test
     fun `should assign to group with all dependencies`() {
         // given
@@ -41,6 +46,7 @@ class MetadataNodeGroupTest {
                 // because service may define different settings for different dependencies (for example endpoints, which
                 // will be implemented in https://github.com/allegro/envoy-control/issues/6
                 communicationMode = XDS,
+                pathNormalizationConfig = defaultNormalizationConfig,
                 proxySettings = ProxySettings().with(
                     serviceDependencies = serviceDependencies("a", "b", "c"),
                     allServicesDependencies = true
@@ -61,6 +67,7 @@ class MetadataNodeGroupTest {
         assertThat(group).isEqualTo(
             ServicesGroup(
                 proxySettings = ProxySettings().with(serviceDependencies = setOf()),
+                pathNormalizationConfig = defaultNormalizationConfig,
                 communicationMode = XDS
             )
         )
@@ -79,6 +86,7 @@ class MetadataNodeGroupTest {
         assertThat(group).isEqualTo(
             ServicesGroup(
                 proxySettings = ProxySettings().with(serviceDependencies = serviceDependencies("a", "b", "c")),
+                pathNormalizationConfig = defaultNormalizationConfig,
                 communicationMode = XDS
             )
         )
@@ -97,6 +105,7 @@ class MetadataNodeGroupTest {
         assertThat(group).isEqualTo(
             AllServicesGroup(
                 communicationMode = ADS,
+                pathNormalizationConfig = defaultNormalizationConfig,
                 proxySettings = ProxySettings().with(allServicesDependencies = true)
             )
         )
@@ -115,6 +124,7 @@ class MetadataNodeGroupTest {
         assertThat(group).isEqualTo(
             ServicesGroup(
                 proxySettings = ProxySettings().with(serviceDependencies = serviceDependencies("a", "b", "c")),
+                pathNormalizationConfig = defaultNormalizationConfig,
                 communicationMode = ADS
             )
         )
@@ -135,6 +145,7 @@ class MetadataNodeGroupTest {
                 // we have to preserve all services even if outgoingPermissions is disabled,
                 // because service may define different settings for different dependencies (for example retry config)
                 communicationMode = ADS,
+                pathNormalizationConfig = defaultNormalizationConfig,
                 proxySettings = ProxySettings().with(serviceDependencies = serviceDependencies("a", "b", "c"))
             )
         )
@@ -157,6 +168,7 @@ class MetadataNodeGroupTest {
         assertThat(group).isEqualTo(
             ServicesGroup(
                 proxySettings = ProxySettings().with(serviceDependencies = serviceDependencies("a", "b", "c")),
+                pathNormalizationConfig = defaultNormalizationConfig,
                 communicationMode = XDS,
                 serviceName = "app1"
             )
@@ -197,6 +209,7 @@ class MetadataNodeGroupTest {
             ServicesGroup(
                 communicationMode = ADS,
                 serviceName = "app1",
+                pathNormalizationConfig = defaultNormalizationConfig,
                 proxySettings = addedProxySettings.with(serviceDependencies = serviceDependencies("a", "b"))
             )
         )
@@ -218,6 +231,7 @@ class MetadataNodeGroupTest {
             AllServicesGroup(
                 communicationMode = XDS,
                 serviceName = "app1",
+                pathNormalizationConfig = defaultNormalizationConfig,
                 proxySettings = addedProxySettings.with(allServicesDependencies = true)
             )
         )