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
Allow preservation of trailing slashes on TLS redirects via annotation. #7144
Conversation
Welcome @mmiller1! |
Hi @mmiller1. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the 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. |
Signed-off-by: mamiller <mamiller@rosettastone.com>
@@ -85,6 +87,7 @@ func (a rewrite) Parse(ing *networking.Ingress) (interface{}, error) { | |||
|
|||
config.Target, _ = parser.GetStringAnnotation("rewrite-target", ing) | |||
config.SSLRedirect, err = parser.GetBoolAnnotation("ssl-redirect", ing) | |||
config.PreserveTrailingSlash, err = parser.GetBoolAnnotation("preserve-trailing-slash", ing) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should check the err
from the ssl-redirect
annotation parsing firstly, considering the following case:
ssl-redirect
annotation is invalidpreserve-trailing-slash
is valid
so err
will be reset to nil
and the error process was dismissed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed, thanks.
rootfs/etc/nginx/lua/lua_ingress.lua
Outdated
request_uri = string.sub(request_uri, 1, -2) | ||
-- do not append a trailing slash on redirects unless enabled by annotations | ||
if location_config.preserve_trailing_slash == false then | ||
if string.sub(request_uri, -1) == "/" then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if string.sub(request_uri, -1) == "/" then | |
if string.byte(request_uri, -1, -1) == string.byte('/') then |
Use string.byte
is more efficient, as string.sub
will generate a new string, which allocates some small memory chunk and makes the LuaJIT GC collector busy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated, thaanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missed this comment! :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/ok-to-test
@@ -0,0 +1,53 @@ | |||
/* | |||
Copyright 2018 The Kubernetes Authors. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2018 -> 2021
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
/lgtm
/assign @rikatz |
request_uri = string.sub(request_uri, 1, -2) | ||
-- do not append a trailing slash on redirects unless enabled by annotations | ||
if location_config.preserve_trailing_slash == false then | ||
if string.byte(request_uri, -1, -1) == string.byte('/') then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a Lua expert, but why are we replacing string.sub here for string.byte? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, did some experiments here, looks good, just getting strictly the last character.
/label tide/merge-method-squash Thank you! |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mmiller1, rikatz, tao12345666333, tokers 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 |
…n. (kubernetes#7144) * allow retaining a trailing slash in a TLS redirect via annotation. Signed-off-by: mamiller <mamiller@rosettastone.com> * requested changes * gofmt
…n. (kubernetes#7144) * allow retaining a trailing slash in a TLS redirect via annotation. Signed-off-by: mamiller <mamiller@rosettastone.com> * requested changes * gofmt
…n. (kubernetes#7144) * allow retaining a trailing slash in a TLS redirect via annotation. Signed-off-by: mamiller <mamiller@rosettastone.com> * requested changes * gofmt
What this PR does / why we need it:
this allows setting an optional "reserve-trailing-slash" annotation to "true" in order to keep trailing slashes in the Location header returned by a TLS redirect.
The default behavior is retained, removing the slashes.
Types of changes
Which issue/s this PR fixes
fixes #6931
How Has This Been Tested?
additional e2e tests for the annotation.
Checklist: