Filter: path support for RequestRedirectFilter

[sjberman]

As a user of NGF
I want to have a RequestRedirect Filter for my paths for my API
So that I can redirect using paths.

Acceptance

• The path option of the RequestRedirect filter is implemented as defined for HTTP Routes here.
• The compatibility doc is updated with path support for the requestRedirect filter.
• Existing how-to guide for redirects is updated to describe how to use the path option of a redirect filter.
• Conformance test for path redirects is enabled

[Imesh7]

@sjberman @mpstefan
Hi,
I am new to the open source contribution. I would like to working/look on this issue.
Is there anyone working on this issue?

Thanks

[sjberman]

@Imesh7 Nobody is currently working on this issue, feel free to take a look at it! Thanks for your interest!

[Imesh7]

Hi @sjberman ,
I am working on this issue.
As in the doc HTTPPathModifier has replacePrefixMatch , In here how to get the prefix match to the createRewritesValForRewriteFilter fuction

[sjberman]

@Imesh7 The HTTPRequestRedirectFilter struct will need to be updated to include the path. Based on whether that value is a prefix or full path will determine how you have to configure nginx.

[Imesh7]

Proposal to solve the Issue

1. add Path field to HTTPRequestRedirectFilter struct , type is pointer to HTTPPathModifier

type HTTPRequestRedirectFilter struct {
	Scheme *string
	Hostname *string
	Port *int32
	StatusCode *int
	Path *HTTPPathModifier
}

2. configure nginx

Existing function
https://github.com/nginxinc/nginx-gateway-fabric/blob/bf87eec5cb510659971b7836348e4fbf256f260f/internal/mode/static/nginx/config/servers.go#L269C1-L312C2

Proposed function (Please read the comments on the below function)

//added 'path' as paramter , because it is the Request Path
func createReturnValForRedirectFilter(filter *dataplane.HTTPRequestRedirectFilter, listenerPort int32, path string) *http.Return {
	if filter == nil {
		return nil
	}

	hostname := "$host"
	if filter.Hostname != nil {
		hostname = *filter.Hostname
	}

	code := http.StatusFound
	if filter.StatusCode != nil {
		code = http.StatusCode(*filter.StatusCode)
	}

	port := listenerPort
	if filter.Port != nil {
		port = *filter.Port
	}

	hostnamePort := fmt.Sprintf("%s:%d", hostname, port)

	scheme := "$scheme"
	if filter.Scheme != nil {
		scheme = *filter.Scheme
		// Don't specify the port in the return url if the scheme is
		// well known and the port is already set to the correct well known port
		if (port == 80 && scheme == "http") || (port == 443 && scheme == "https") {
			hostnamePort = hostname
		}
		if filter.Port == nil {
			// Don't specify the port in the return url if the scheme is
			// well known and the port is not specified by the user
			if scheme == "http" || scheme == "https" {
				hostnamePort = hostname
			}
		}
	}
	
         // Starts from here
	newPath := "$replace_path"
	redirectFilter := filter.Path.Replacement

	if filter.Path != nil {
		switch filter.Path.Type {
		case dataplane.ReplaceFullPath:
			{
				newPath = filter.Path.Replacement
			}
		case dataplane.ReplacePrefixMatch:
			{
				if filter.Path.Replacement == "" {
					newPath = path
				}

				// eg:= if path is /foo/bar
				// redirectFilter is /xyz/
				regex := fmt.Sprintf("^%s(.*)$", path)

				re, err := regexp.Compile(regex)
				if err != nil {
					fmt.Println("Error compiling regex:", err)
				}
				replacement := fmt.Sprintf("%s$1", redirectFilter)
				newPath = re.ReplaceAllString(path, replacement)
				//newPath is /xyz/bar
			}
		}
	}

	return &http.Return{
		Code: code,
		Body: fmt.Sprintf("%s://%s %s", scheme, hostnamePort, newPath),
	}
}

@sjberman
Can I know I am on the correct move & Happy to have some requests.
Thanks

[sjberman]

I think the approach in general is on the right track. We'll know for sure once we start testing all the variants in the example table to see if they redirect properly.

A few things to note:

• newPath can just be initialized to an empty string; "$replace_path" doesn't add any value
• I don't think you need to use the regexp library at all. Nginx will use the regex to match the incoming request. The control plane doesn't have to do anything.
• You can likely follow a similar pattern that is established in createRewritesValForRewriteFilter to ensure that / is handled properly, and probably don't need any string replacements like you have.

[Imesh7]

@sjberman I have a question,

• You can likely follow a similar pattern that is established in createRewritesValForRewriteFilter to ensure that / is handled properly, and probably don't need any string replacements like you have.

createReturnValForRedirectFilter function should return http.Return

&http.Return{
	Code: code,
	Body: fmt.Sprintf("%s://%s %s", scheme, hostnamePort, redirectPath),
}

for the ReplacePrefixMatch implemtation , without string replace how to return the value?
Can you guide me for the implementation.

[sjberman]

What I mean is that you don't need to use the regexp library and call ReplaceAllString. The regex variable used in the rewrite function is a literal string that is put into the nginx config, it's not used by the control plane to determine how to replace the path. However, this is only used in the rewrite case because the rewrite nginx directive uses the regex string as one of its inputs. The return directive used for a redirect does not use that input.

The redirectPath for the prefix case should be whatever was defined in the filter, plus appended to it should be whatever else was a part of the request path. So if the location block is /foo, our filter is /xyz, then a request to /foo/bar would be redirected to /xyz/bar. We have to be able to preserve /bar at the end (or whatever a request path may contain), which we don't know about until a request actually comes into nginx. This means that we need some way in nginx to capture whatever extra path segments are added on to /foo so we can append those to /xyz. This issue is turning out a little more complicated than I originally thought!

@pleshakov Do you know if there's a way in nginx to capture the path segments without using a regex location path, so that we can append them to the redirect path? I'm not finding anything, so we may have to update this specific location block to use a regex path, just so we can capture the request path segments if they exist.

[pleshakov]

@pleshakov Do you know if there's a way in nginx to capture the path segments without using a regex location path, so that we can append them to the redirect path? I'm not finding anything, so we may have to update this specific location block to use a regex path, just so we can capture the request path segments if they exist.

@sjberman I woudn't use regex location, because they change how request matching is processed https://nginx.org/en/docs/http/ngx_http_core_module.html#location

what if we just use rewrite directive to rewrite uri of the request, similar to what we do in the rewrite filter?

[sjberman]

@pleshakov The expectation with a redirect is a 3xx response and the client performs the redirection. A rewrite happens server-side and the client is unaware.

[pleshakov]

@pleshakov The expectation with a redirect is a 3xx response and the client performs the redirection. A rewrite happens server-side and the client is unaware.

yep, so we do rewrite the same way, but then perform a redirect.

So something like below. Would that work?

server {
    listen 8093;

    location /foo {
        rewrite ^/foo(.*)$ /xyz$1;
        return 302  $uri;
    }
}

curl localhost:8093/foo/abc -I
HTTP/1.1 302 Moved Temporarily
Server: nginx/1.25.3
Date: Thu, 11 Jan 2024 17:28:16 GMT
Content-Type: text/html
Content-Length: 145
Location: http://localhost:8093/xyz/abc
Connection: keep-alive

[sjberman]

@pleshakov Oh I see, yeah that looks like it could work. I'm curious if there is much of a performance difference doing it this way versus a regex location with just a return.

Regarding the regex location, what specifically should we be worried about in regards to the matching? Is it the case of a user specifying /foo and /foo/bar in their HTTPRoute, with the former using a regex and therefore unintentionally matching the latter?

[pleshakov]

@sjberman

regex location can mess up prefix matching, because if it matches, all prefix locations will be ignored. Please see https://nginx.org/en/docs/http/ngx_http_core_module.html#location

server {
    listen 8094;

    location /hello/abc {
        return 200 "hello/abc\n";
    }

    location ~^/hello {
        return 200 "hello\n";
    }
}

curl localhost:8094/hello/abc
hello

[sjberman]

@pleshakov Understood. This will make regex path support very interesting in the future...

[github-actions]

This issue is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 14 days.

[mpstefan]

Moving to backlog as there has been no PR activity.