-
Notifications
You must be signed in to change notification settings - Fork 336
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
feat: update an redirect annotation for ingress resource #975
Conversation
Codecov Report
@@ Coverage Diff @@
## master #975 +/- ##
==========================================
- Coverage 31.91% 31.90% -0.01%
==========================================
Files 73 73
Lines 7953 7955 +2
==========================================
Hits 2538 2538
- Misses 5139 5141 +2
Partials 276 276
Continue to review full report at Codecov.
|
@AlinsRan Hi, some test error, pls have a check |
This error is |
_httpToHttps = AnnotationsPrefix + "http-to-https" | ||
_httpToHttps = AnnotationsPrefix + "http-to-https" | ||
_permanentRedirect = AnnotationsPrefix + "permanent-redirect" | ||
_permanentRedirectCode = AnnotationsPrefix + "permanent-redirect-code" |
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.
IMHO since we want to make a permanent redirect, only codes 301
and 308
are allowed, in such a case, let users specify a custom HTTP status code that doesn't comply with the HTTP semantic.
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.
Do you mean to add test cases for other status codes?
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 mean, we should only allow users configuring 301
or 308
here. If I configure a 302
code here, it doesn't comply the HTTP semantic, because 302
is a "temporarily" redirect, but what we want is "permanent".
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.
Got it.
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.
@tokers @AlinsRan According to RFC 7231 Status codes between 300 and 308 can indicate redirection.
Considering that we will also support temporary redirection, but adding a specific implementation for it alone is not of much value, here I suggest to merge it directly and modify this annotation to http-redirect
and http-redirect-code
, the corresponding verification logic is, if not configured, the default is http.StatusMovedPermanently
, the allowed value is between http.StatusMultipleChoices
and http.StatusPermanentRedirect
.
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.
LGTM
Type of change:
What this PR does / why we need it:
Pre-submission checklist: