Directly added gateway filters bypass setting of correct order

[NadChel]

I came across this problem a few months ago when writing my Dynamic Gateway project. Since I was dynamically building Routes in runtime, I chose to forgo RouteLocatorBuilder and instead assembled each Route by calling methods on Route.AsyncBuilder

Then I noticed something unusual: while the filters were definitely added to my route builders, my routes still didn't work as expected. Everything was fine once I wrapped the filters in OrderedGatewayFilter instances with the order of zero

Today, as I begin my journey as an open-source contributor, I took time to investigate the problem. I found it and wrote a small MRE. Here it is:

<?xml version="1.0" encoding="UTF-8"?>
<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
         xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 https://maven.apache.org/xsd/maven-4.0.0.xsd">
    <modelVersion>4.0.0</modelVersion>
    <parent>
        <groupId>org.springframework.boot</groupId>
        <artifactId>spring-boot-starter-parent</artifactId>
        <version>3.1.5</version>
        <relativePath/> <!-- lookup parent from repository -->
    </parent>
    <groupId>com.example</groupId>
    <artifactId>gatewaydemo</artifactId>
    <version>0.0.1-SNAPSHOT</version>
    <name>gatewaydemo</name>
    <description>gatewaydemo</description>
    <properties>
        <java.version>17</java.version>
        <spring-cloud.version>2022.0.4</spring-cloud.version>
    </properties>
    <dependencies>
        <dependency>
            <groupId>org.springframework.cloud</groupId>
            <artifactId>spring-cloud-starter</artifactId>
        </dependency>
        <dependency>
            <groupId>org.springframework.cloud</groupId>
            <artifactId>spring-cloud-starter-gateway</artifactId>
        </dependency>
        <dependency>
            <groupId>org.projectlombok</groupId>
            <artifactId>lombok</artifactId>
            <optional>true</optional>
        </dependency>
        <dependency>
            <groupId>org.springframework.boot</groupId>
            <artifactId>spring-boot-starter-test</artifactId>
            <scope>test</scope>
        </dependency>
    </dependencies>
    <dependencyManagement>
        <dependencies>
            <dependency>
                <groupId>org.springframework.cloud</groupId>
                <artifactId>spring-cloud-dependencies</artifactId>
                <version>${spring-cloud.version}</version>
                <type>pom</type>
                <scope>import</scope>
            </dependency>
        </dependencies>
    </dependencyManagement>

    <build>
        <plugins>
            <plugin>
                <groupId>org.springframework.boot</groupId>
                <artifactId>spring-boot-maven-plugin</artifactId>
                <configuration>
                    <excludes>
                        <exclude>
                            <groupId>org.projectlombok</groupId>
                            <artifactId>lombok</artifactId>
                        </exclude>
                    </excludes>
                </configuration>
            </plugin>
        </plugins>
    </build>

</project>

package com.example.gatewaydemo;

import org.springframework.boot.SpringApplication;
import org.springframework.boot.autoconfigure.SpringBootApplication;

@SpringBootApplication
public class GatewaydemoApplication {

    public static void main(String[] args) {
        SpringApplication.run(GatewaydemoApplication.class, args);
    }

}

package com.example.gatewaydemo.routing;

import org.springframework.cloud.gateway.filter.GatewayFilter;
import org.springframework.cloud.gateway.filter.OrderedGatewayFilter;
import org.springframework.cloud.gateway.filter.factory.RewritePathGatewayFilterFactory;
import org.springframework.cloud.gateway.handler.predicate.PathRoutePredicateFactory;
import org.springframework.cloud.gateway.route.Route;
import org.springframework.cloud.gateway.route.RouteLocator;
import org.springframework.context.annotation.Bean;
import org.springframework.context.annotation.Configuration;
import reactor.core.publisher.Flux;

import java.util.List;
import java.util.UUID;

@Configuration
public class MyRoutingConfig {
    @Bean
    public RouteLocator routeLocator() {
        Route dogRoute = Route.async()
                .id(UUID.randomUUID().toString())
                .predicate(new PathRoutePredicateFactory()
                        .apply(config -> config.setPatterns(List.of("/dog-fact"))))
                .filter(new RewritePathGatewayFilterFactory()
                                .apply(config -> config.setRegexp("/dog-fact").setReplacement("/api/facts")))
                .uri("https://dog-api.kinduff.com")
                .build();
        return () -> Flux.just(dogRoute);
    }
}

Here's what I get when I hit http://localhost:8080/dog-fact. It's the API's 404:

¯\_(ツ)_/¯

The reason I get a 404 is because the API server received GET https://dog-api.kinduff.com:443/dog-fact, and there's no such endpoint

It seems like my filter never got applied, but that's not exactly the case. Here's a picture from the IntelliJ debugger:

My path rewriting filter is on the list of GatewayFilters, but since all of them, except mine, are ordered, the rewritig filter comes last – importantly, after NettyRoutingFilter that already built the response Flux

// NettyRoutingFilter

    public Mono<Void> filter(ServerWebExchange exchange, GatewayFilterChain chain) {
        URI requestUrl = exchange.getRequiredAttribute(GATEWAY_REQUEST_URL_ATTR); // not yet rewritten by my filter!
                // ...
        final String url = requestUrl.toASCIIString();
                // ...
        Flux<HttpClientResponse> responseFlux = getHttpClient(route, exchange).headers(headers -> {
                    // ...
        }).request(method).uri(url /* includes not yet rewritten path! */).send((req, nettyOutbound) -> {

So when my it's my filter's turn, the fact that it sets the right value for GATEWAY_REQUEST_URL_ATTR attribute doesn't matter anymore, the attribute has already been read

// RewritePathGatewayFilterFactory

@Override
public Mono<Void> filter(ServerWebExchange exchange, GatewayFilterChain chain) {
    ServerHttpRequest req = exchange.getRequest();
    addOriginalRequestUrl(exchange, req.getURI());
    String path = req.getURI().getRawPath();
    String newPath = pattern.matcher(path).replaceAll(replacement);

    ServerHttpRequest request = req.mutate().path(newPath).build();

    exchange.getAttributes().put(GATEWAY_REQUEST_URL_ATTR, request.getURI()); // doesn't matter anymore!

    return chain.filter(exchange.mutate().request(request).build());
}

It's also important to put it ahead of RouteToRequestUrlFilter. It sets the same GATEWAY_REQUEST_URL_ATTR attribute with a request URI in which Gateway's host and port, localhost:8080 in my case, are replaced with the Route's getUri(). We don't want the rewrite path filter to set it back to localhost:8080 (which it will do if it has a lower precedence)

So to get it working, you have to wrap the filter in an OrderedGatewayFilter instance with the right order which should be

1. Below the order of NettyRoutingFilter (Integer.MAX_VALUE)
2. Below the order of RouteToRequestUrlFilter (10 000)

   
   // it works

@Configuration public class MyRoutingConfig { @Bean public RouteLocator routeLocator() { Route dogRoute = Route.async() .id(UUID.randomUUID().toString()) .predicate(new PathRoutePredicateFactory() .apply(config -> config.setPatterns(List.of("/dog-fact")))) .filter(wrapInOrderedGatewayFilter( new RewritePathGatewayFilterFactory() .apply(config -> config.setRegexp("/dog-fact").setReplacement("/api/facts")))) .uri("https://dog-api.kinduff.com") .build(); return () -> Flux.just(dogRoute); }

private static GatewayFilter wrapInOrderedGatewayFilter(GatewayFilter delegate) {
    return new OrderedGatewayFilter(delegate, 10_000 - 1);
}

}

How does `RouteLocatorBuilder` handle it?
```java
// it works too
    @Bean
    public RouteLocator routeLocator(RouteLocatorBuilder builder) {
        return builder.routes()
                .route(p -> p.method(HttpMethod.GET)
                        .and()
                        .path("/dog-fact")
                        .filters(f /* GatewayFilterSpec*/ -> f.rewritePath("/dog-fact", "/api/facts"))
                        .uri("https://dog-api.kinduff.com"))
                .build();
    }

Very straighforwardly:

// GatewayFilterSpec
    public GatewayFilterSpec filter(GatewayFilter gatewayFilter) {
        if (gatewayFilter instanceof Ordered) {
            this.routeBuilder.filter(gatewayFilter);
            return this;
        }
        return this.filter(gatewayFilter, 0 /* it implicitly sets the order of zero! */);
    }

    public GatewayFilterSpec filter(GatewayFilter gatewayFilter, int order) {
        if (gatewayFilter instanceof Ordered) {
            this.routeBuilder.filter(gatewayFilter);
            log.warn("GatewayFilter already implements ordered " + gatewayFilter.getClass()
                    + "ignoring order parameter: " + order);
            return this;
        }
        this.routeBuilder.filter(new OrderedGatewayFilter(gatewayFilter, order));
        return this;
    }
// ...
    public GatewayFilterSpec rewritePath(String regex, String replacement) {
        return filter(getBean(RewritePathGatewayFilterFactory.class)
                .apply(c -> c.setRegexp(regex).setReplacement(replacement)));
    }

To recap:

1. Spring Cloud Gateway exposes its Route class and allows a direct creation of Routes
2. However, you can't build a correct Route with a mutating custom filter without manually setting a correct order
3. As of now, to know what order is correct, you have to look up other filters' order in the source code. It's not documented
4. If the order of those other filters changes at any point (after all, it's an implementation detail), your custom filter may no longer work. The correct behavior of your filter gets effectively coupled with other classes that are outside of your control

It's not right

In case you recognize this problem, I want to solve it myself. For example, we could move this logic ("if it's not already wrapped, wrap in a reasonably ordered OrderedGatewayFilter"):

    public GatewayFilterSpec filter(GatewayFilter gatewayFilter) {
        if (gatewayFilter instanceof Ordered) {
            this.routeBuilder.filter(gatewayFilter);
            return this;
        }
        return this.filter(gatewayFilter, 0);
    }

from GatewayFilterSpec to Route itself

I also believe we should move away from those magic ints. For instance, we can take a leaf out of Spring Security's book with its SecurityWebFiltersOrder

I'm ready to discuss it further

[spencergibb]

It's not right

That is subjective. The Route class is low level. It needs to be public for use across packages. It is not documented anywhere to do things as you have described.

I'm not sure how SecurityWebFiltersOrder would help.

[NadChel]

Well, it's now part of public API, whether intentionally or not. Generally speaking, it means we have to properly maintain it (forever)

SecurityWebFiltersOrder is used in Spring Security as an alternative for hard-coded order ints. It ties filters to certain "phases" rather than specific ints that are liable to change. We too have a few implicit phases (though it's not as complex), but we don't yet recognize it on a programmatic level. I don't exactly insist on a similar abstraction, but it's something that deserves consideration, imo

P.S.: It's off-topic, but my PR has been sitting for a week, do you believe someone on the team could give it a look?