Skip to content
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.

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

Add HTTP Rewrite and Redirect Filter guide #1162

Merged
merged 10 commits into from
Jun 8, 2022

Conversation

rainest
Copy link
Contributor

@rainest rainest commented May 14, 2022

What type of PR is this?
/kind documentation

What this PR does / why we need it:
Adds a guide for GEP-726 features, including the changes currently under review in #1124

It is a WIP for two reasons:

  • For HTTP->HTTPS redirects, are implementations expected to not redirect requests that are already HTTPS, or does the spec allow misconfigurations that result in redirect loops? If so, how should users properly set up their HTTPRoutes and Gateway Listener configuration to only match these redirect rules on HTTP requests? I expect this generalizes to path/hostname/etc. also, but this was the most prominent case I could think of.
  • Do we care about path types for path modifiers? It seems that you can use prefix modifiers for exact paths, though it's probably pointless (if harmless), and I'm not sure whether RegularExpression paths would be considered prefixes here (i.e. ReplacePrefixMatch changes only the path segment matching the regex).
  • I added the text suggested in Document HTTPRoute Filter Compatibility #928 mostly as-is, but it raises the question "what status must we set?". It seems like we need a new RouteConditionType for filter-related statuses if we want this, or a new RouteConditionReason (it doesn't seem like it'd fit under any of the existing types unless we reject routes with invalid filters entirely, and I wouldn't expect to since they're per-rule and since the text indicates this should just be a warning, not an error).

As a side note, I have bad timing and will be out from 5/18-5/28, so it may make sense to use this as a starting point for someone else if review takes time or it needs significant changes. Edit: then I forgot this is the week that a bunch of people are at Kubecon 🙂 I should actually be able to check things on the 26th even though I'm still technically out.

Which issue(s) this PR fixes:
Fixes #1038
Fixes #928

Does this PR introduce a user-facing change?:

NONE

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/documentation Categorizes issue or PR as related to documentation. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 14, 2022
@k8s-ci-robot
Copy link
Contributor

Hi @rainest. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot requested review from thockin and youngnick May 14, 2022 00:41
@robscott
Copy link
Member

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 17, 2022
@rainest rainest force-pushed the doc/redirect-rewrite branch from d854f3d to d364cc5 Compare May 17, 2022 18:14
@robscott robscott linked an issue May 18, 2022 that may be closed by this pull request
@youngnick youngnick requested review from robscott and removed request for thockin May 19, 2022 09:45
Copy link
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 @rainest, this is really great!

site-src/v1alpha2/guides/http-redirect-rewrite.md Outdated Show resolved Hide resolved
site-src/v1alpha2/guides/http-redirect-rewrite.md Outdated Show resolved Hide resolved
site-src/v1alpha2/guides/http-redirect-rewrite.md Outdated Show resolved Hide resolved
site-src/v1alpha2/guides/http-redirect-rewrite.md Outdated Show resolved Hide resolved
Comment on lines 111 to 117
<!---
Do these behave at all differently based on matches.path.type? I expect no, and
that implementations should just always replace only the matching path prefix
or the entire path. Using ReplacePrefixMatch on Exact is the same as
ReplaceFullPath. RegularExpression looks a bit ambiguous, since the
PathMatchType looks unclear on whether they're handled as prefixes or not.
-->
Copy link
Member

Choose a reason for hiding this comment

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

Good point, I think #1166 will resolve this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Original in case the diff does something silly now that it's gone:

Do these behave at all differently based on matches.path.type? I expect no, and that implementations should just always replace only the matching path prefix or the entire path. Using ReplacePrefixMatch on Exact is the same as ReplaceFullPath. RegularExpression looks a bit ambiguous, since the PathMatchType looks unclear on whether they're handled as prefixes or not.

I simply removed this without adding additional explanation since it looks like 1166's answer is just "you can't configure them on those other types". Do we think we'd need explanation of that here or is that too far off into the weeds?

site-src/v1alpha2/guides/http-redirect-rewrite.md Outdated Show resolved Hide resolved
site-src/v1alpha2/guides/http-redirect-rewrite.md Outdated Show resolved Hide resolved
site-src/v1alpha2/guides/http-redirect-rewrite.md Outdated Show resolved Hide resolved
@rainest
Copy link
Contributor Author

rainest commented Jun 1, 2022

examples/experimental/http-redirect-rewrite/httproute-redirect-https.yaml is technically already valid under v1alpha2, but I expect it's easier to not split hairs and just call the whole thing experimental, assuming it'll imminently become not experimental and we'll move the whole directory back.

@rainest rainest requested review from robscott and youngnick June 1, 2022 23:03
@youngnick
Copy link
Contributor

/lgtm

Nice work @rainest, this is very clear.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 6, 2022
Copy link
Member

@shaneutt shaneutt left a comment

Choose a reason for hiding this comment

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

Looks like we're at a point with this one where we can remove [WIP] from the name. Thanks Travis for the effort on this one, looks great.

/lgtm

@rainest rainest changed the title [WIP] Add HTTP Rewrite and Redirect Filter guide Add HTTP Rewrite and Redirect Filter guide Jun 6, 2022
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 6, 2022
Copy link
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 @rainest! Mostly LGTM, just a few small nits. On an unrelated note, if anyone wants to remove https://github.com/kubernetes-sigs/gateway-api/blob/master/site-src/css/extra.css, I would welcome that. The alignment that results in is particularly bad with this new page.

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 7, 2022
@rainest rainest force-pushed the doc/redirect-rewrite branch from 0dda0bc to 7b02e13 Compare June 7, 2022 18:44
@rainest
Copy link
Contributor Author

rainest commented Jun 7, 2022

@robscott do we indeed want the backref comments in examples? Those do actually render in the guide:

backref

The other examples don't include the same; their usage is just loosely indicated by the directory name, though I'm not sure others have the same quality where the guide is under v1alpha2 and the examples are under experimental. I removed the original commit from the PR, backrefs.diff.txt if we want to re-add it.

@rainest rainest requested a review from robscott June 7, 2022 18:48
@robscott
Copy link
Member

robscott commented Jun 7, 2022

@rainest any chance you checked that after #1186 merged? I had to change the prefix characters to #? to avoid hiding all subheadings. If that doesn't work, I think it's fine to just leave that part out of this PR. This is a great improvement and would love to get it in.

rainest added 9 commits June 7, 2022 14:16
Move rewrite and redirect YAML into examples and load them via includes.

Replace all tabs with spaces and remove trailing spaces from examples.

Place example explanations before examples.
Remove the paragraph describing redirect loop handling. The spec
behavior is still under discussion and remains undefined for now.

See kubernetes-sigs#1185
@rainest rainest force-pushed the doc/redirect-rewrite branch 3 times, most recently from 2f586c0 to 8393061 Compare June 7, 2022 21:32
@rainest rainest force-pushed the doc/redirect-rewrite branch from 8393061 to 5c02d19 Compare June 7, 2022 21:46
@rainest
Copy link
Contributor Author

rainest commented Jun 7, 2022

Alright, preview properly stripping the new header (albeit only after the second try for some reason), so this should be all good to go.

@youngnick
Copy link
Contributor

LGTM, I'll just approve though.

/approve

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 8, 2022
Copy link
Member

@shaneutt shaneutt left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 8, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rainest, shaneutt, youngnick

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 merged commit aee4c22 into kubernetes-sigs:master Jun 8, 2022
@rainest rainest deleted the doc/redirect-rewrite branch June 8, 2022 16:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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/documentation Categorizes issue or PR as related to documentation. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
5 participants