Skip to content

[feat gw-api]modify ReplacePrefixMatch support #4457

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 2 commits into
base: AGAController
Choose a base branch
from
Open

[feat gw-api]modify ReplacePrefixMatch support #4457

wants to merge 2 commits into from
+51 −37

Conversation

@shuqz
Copy link
Collaborator

@shuqz shuqz commented Nov 18, 2025 •
edited
Loading

Description

  • due to ALB limitation (cannot preserve suffix), we can support ReplacePrefixMatch in Filter type: RequestRedirect with limitation. we can still support ReplaceFullPath
  • added limitations in documentation
  • modified e2e test too
  • see manual test result

Checklist

  • Added tests that cover your change (if possible)
  • Added/modified documentation as required (such as the README.md, or the docs directory)
  • Manually tested
    manifest:
---
apiVersion: gateway.networking.k8s.io/v1
kind: HTTPRoute
metadata:
  name: 
  namespace: 
spec:
  parentRefs:
    - name: 
      port: 80
  rules:
    - matches:
        - path:
            type: PathPrefix
            value: /original-prefix
      filters:
        - type: RequestRedirect
          requestRedirect:
            path:
              type: ReplacePrefixMatch
              replacePrefixMatch: /replacement-prefix
            statusCode: 301
            port: 443

Test 1: Request to exact prefix (no suffix)

% curl -v "http://k8s-shuqztes-shuqztes--.us-west-2.elb.amazonaws.com/original-prefix"

* Request completely sent off
< HTTP/1.1 301 Moved Permanently
< Server: awselb/2.0
< Date: Thu, 20 Nov 2025 02:07:01 GMT
< Content-Type: text/html
< Content-Length: 134
< Connection: keep-alive
< Location: http://k8s-shuqztes-shuqztes--.us-west-2.elb.amazonaws.com:443/replacement-prefix
<
<html>
<head><title>301 Moved Permanently</title></head>
<body>
<center><h1>301 Moved Permanently</h1></center>
</body>
</html>
* Connection #0 to host k8s-shuqztes-shuqztes--.us-west-2.elb.amazonaws.com left intact

Test 2: Request with path suffix

% curl -v "http://k8s-shuqztes-shuqztes--.us-west-2.elb.amazonaws.com/original-prefix/original-prefix/lemon"

* Request completely sent off
< HTTP/1.1 301 Moved Permanently
< Server: awselb/2.0
< Date: Thu, 20 Nov 2025 02:07:44 GMT
< Content-Type: text/html
< Content-Length: 134
< Connection: keep-alive
< Location: http://k8s-shuqztes-shuqztes--.us-west-2.elb.amazonaws.com:443/replacement-prefix/original-prefix/lemon
<
<html>
<head><title>301 Moved Permanently</title></head>
<body>
<center><h1>301 Moved Permanently</h1></center>
</body>
</html>

Test 3: Request with deeper path

% curl -v "http://k8s-shuqztes-shuqztes--.us-west-2.elb.amazonaws.com/original-prefix/path/to/resource"

* Request completely sent off
< HTTP/1.1 301 Moved Permanently
< Server: awselb/2.0
< Date: Thu, 20 Nov 2025 02:09:23 GMT
< Content-Type: text/html
< Content-Length: 134
< Connection: keep-alive
< Location: http://k8s-shuqztes-shuqztes--us-west-2.elb.amazonaws.com:443/replacement-prefix/path/to/resource
<
<html>
<head><title>301 Moved Permanently</title></head>
<body>
<center><h1>301 Moved Permanently</h1></center>
</body>
</html>

Conformance test

    --- FAIL: TestConformance/HTTPRouteRedirectPath (0.26s)
        --- PASS: TestConformance/HTTPRouteRedirectPath/4_request_to_'/full-path-and-host'_should_receive_a_302 (12.32s)
        --- PASS: TestConformance/HTTPRouteRedirectPath/5_request_to_'/full-path-and-status'_should_receive_a_301 (12.32s)
        --- FAIL: TestConformance/HTTPRouteRedirectPath/0_request_to_'/original-prefix/lemon'_should_receive_a_302 (30.00s)
        --- FAIL: TestConformance/HTTPRouteRedirectPath/1_request_to_'/full/path/original'_should_receive_a_302 (30.00s)
        --- FAIL: TestConformance/HTTPRouteRedirectPath/2_request_to_'/path-and-host'_should_receive_a_302 (30.00s)
        --- FAIL: TestConformance/HTTPRouteRedirectPath/3_request_to_'/path-and-status'_should_receive_a_301 (30.00s)

test failed because rules are sorted by precedence order first, and the 3nd rule causes redirect loop and we error out from api, so following rules does not got created at all.

e2e test

ginkgo -v -r test/e2e/gateway --  --kubeconfig=$KUBECONFIG --cluster-name=awslbc-loadtest --aws-region=us-west-2 --aws-vpc-id=vpc---enable-gateway-tests -ginkgo.focus="with ALB ip target configuration with HTTPRoute specified filter"

Ran 1 of 28 Specs in 296.336 seconds
SUCCESS! -- 1 Passed | 0 Failed | 0 Pending | 27 Skipped
PASS

Ginkgo ran 1 suite in 5m15.427688292s
Test Suite Passed
  • Made sure the title of the PR is a good description that can go into the release notes

BONUS POINTS checklist: complete for good vibes and maybe prizes?! 🤯

  • Backfilled missing tests for code in same general area 🎉
  • Refactored something and made the world a better place 🌟

@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Nov 18, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: shuqz

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 18, 2025
@shuqz shuqz force-pushed the conformance-no-matching-parent branch from 5abaea3 to a9e59cc Compare November 20, 2025 18:55
@shuqz shuqz changed the title [feat gw-api]remove ReplacePrefixMatch support [feat gw-api]modify ReplacePrefixMatch support Nov 20, 2025
zac-nixon
zac-nixon reviewed Nov 20, 2025
View reviewed changes
pkg/gateway/routeutils/route_rule_action.go Outdated
pathVariable := "/#{path}"
path = &pathVariable
} else {
path = filter.Path.ReplacePrefixMatch
Copy link
Collaborator

@zac-nixon zac-nixon Nov 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how is this different than a full path replacement?

Copy link
Collaborator Author

@shuqz shuqz Nov 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch, i meant path = filter.Path.ReplacePrefixMatch + /* i will update it

Copy link
Collaborator Author

@shuqz shuqz Nov 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually, though path = filter.Path.ReplacePrefixMatch + /* makes more sense logically, but it will fail cases like below

  - matches:
    - path:
        type: PathPrefix
        value: /path-and-status
    filters:
    - type: RequestRedirect
      requestRedirect:
        path:
          type: ReplacePrefixMatch
          replacePrefixMatch: /replacement-prefix
        statusCode: 301

does it brings more value to have path = filter.Path.ReplacePrefixMatch + /*? if not, we can leave it same as full path replacement?

Copy link
Collaborator Author

@shuqz shuqz Nov 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

synced offline, updated

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zac-nixon zac-nixon zac-nixon left review comments

@oliviassss oliviassss Awaiting requested review from oliviassss

Assignees

No one assigned

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@shuqz @k8s-ci-robot @zac-nixon