Skip to content

Feature/path rewrite#288

Merged
k8s-ci-robot merged 6 commits intokubernetes-sigs:mainfrom
Stevenjin8:feature/path-rewrite
Jan 22, 2026
Merged

Feature/path rewrite#288
k8s-ci-robot merged 6 commits intokubernetes-sigs:mainfrom
Stevenjin8:feature/path-rewrite

Conversation

@Stevenjin8
Copy link
Copy Markdown
Contributor

@Stevenjin8 Stevenjin8 commented Jan 5, 2026 •
edited
Loading

depends on #283

What type of PR is this?

/kind feature
What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #

Does this PR introduce a user-facing change?:

Add support for `nginx.ingress.kubernetes.io/x-forwarded-prefix` and `nginx.ingress.kubernetes.io/rewrite-target` (non-regex case)

@k8s-ci-robot k8s-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Jan 5, 2026
@k8s-ci-robot k8s-ci-robot requested a review from rikatz January 5, 2026 21:52
@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jan 5, 2026
This was referenced Jan 5, 2026
Merged
Merged
Draft
robscott
robscott approved these changes Jan 13, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@robscott robscott left a comment

Choose a reason for hiding this comment

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

Thanks @Stevenjin8!

Comment thread pkg/i2gw/providers/ingressnginx/annotations.go Outdated
ConnectionProxyHeaderAnnotation = "nginx.ingress.kubernetes.io/connection-proxy-header"
CustomHeadersAnnotation = "nginx.ingress.kubernetes.io/custom-headers"

// R2gex
Copy link
Copy Markdown
Member

@robscott robscott Jan 13, 2026

Choose a reason for hiding this comment

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

Suggested change
// R2gex
// Regex

Comment thread pkg/i2gw/providers/ingressnginx/headers_test.go
@k8s-ci-robot
Copy link
Copy Markdown
Contributor

k8s-ci-robot commented Jan 13, 2026

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: robscott, Stevenjin8

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

The pull request process is described here

Details Needs approval from an approver in each of these files:
  • OWNERS [Stevenjin8,robscott]

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 the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 13, 2026
@Stevenjin8 Stevenjin8 self-assigned this Jan 13, 2026
@Stevenjin8 Stevenjin8 force-pushed the feature/path-rewrite branch from e6048d3 to d0a4795 Compare January 13, 2026 19:05
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 13, 2026
@mikemorris
Copy link
Copy Markdown
Contributor

mikemorris commented Jan 15, 2026 •
edited
Loading

Refs #271, #272

mikemorris
mikemorris reviewed Jan 15, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@mikemorris mikemorris left a comment

Choose a reason for hiding this comment

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

Logic looks good, just a minor nit on the IR structure/naming.

Comment thread pkg/i2gw/emitter_intermediate/intermediate_representation.go
Comment on lines +66 to +67
// Headers to add on path rewrite.
Headers map[string]string
Copy link
Copy Markdown
Contributor

@mikemorris mikemorris Jan 15, 2026

Choose a reason for hiding this comment

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

Does this belong inside PathRewrite? The logic below looks correct, just feels like either an odd structure or struct name (versus something like RequestRewrite if paths and headers should be siblings).

Copy link
Copy Markdown
Contributor Author

@Stevenjin8 Stevenjin8 Jan 16, 2026

Choose a reason for hiding this comment

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

You mean Headers? Yeah its not clear to me either. Generally, its nice for one function to worry about one IR field, that way you don't have to worry about things already existing.

Copy link
Copy Markdown
Contributor

@mikemorris mikemorris Jan 16, 2026

Choose a reason for hiding this comment

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

Yea having Headers as a field inside the PathRewrite struct feels weird...

Copy link
Copy Markdown
Contributor Author

@Stevenjin8 Stevenjin8 Jan 21, 2026

Choose a reason for hiding this comment

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

oh I remember now why I did it this way. Since headers are pretty straightforward, they aren't really IR, they are just added as a header modifier and it felt weird to add that header into the HTTPRoute, but leave the PathRewrite stuff for later.

@Stevenjin8 Stevenjin8 mentioned this pull request Jan 21, 2026
Merged
@LiorLieberman
Copy link
Copy Markdown
Member

LiorLieberman commented Jan 22, 2026

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 22, 2026
@k8s-ci-robot k8s-ci-robot merged commit 1557d5b into kubernetes-sigs:main Jan 22, 2026
5 checks passed
@asklokesh asklokesh mentioned this pull request Jan 27, 2026
Closed
2 tasks
@danehans danehans mentioned this pull request Feb 6, 2026
Closed
This was referenced Feb 10, 2026
Closed
Closed
Closed
rajashish pushed a commit to rajashish/ingress2gateway1 that referenced this pull request Feb 21, 2026
@Stevenjin8
Feature/path rewrite (kubernetes-sigs#288)
4e9c9c4 
* non-regex rewrites

* handle regex paths

* non-nil map

* fix tests

* lint

* lint and test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@robscott robscott robscott approved these changes

@LiorLieberman LiorLieberman Awaiting requested review from LiorLieberman LiorLieberman is a code owner

@rikatz rikatz Awaiting requested review from rikatz

+1 more reviewer

@mikemorris mikemorris mikemorris left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@Stevenjin8 Stevenjin8

@LiorLieberman LiorLieberman

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. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@Stevenjin8 @k8s-ci-robot @mikemorris @LiorLieberman @robscott @jaellio