Closed Xinayder closed 9 months ago
All modified and coverable lines are covered by tests :white_check_mark:
Comparison is base (
cafb53c
) 42.19% compared to head (f215e19
) 43.68%.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
@Xinayder what do you think to change a bit approach here? I made a few research and having a temporary
as redirect code for a permanent-redirect
annotation is a bit confusing to me. Can I propose to switch a bit the approach here and follow a more commont approach to have two different annotations for permanent-redirect
and temporary-redirect
as Nginx ingress controller does?
PS: test are failing, I think tests do not reflect the code changes made in https://github.com/caddyserver/ingress/pull/165/commits/660815b7d5b30f2f3ce4720ac499c031c6162e29
@mavimo I was actually thinking of submitting a new PR including the temporal redirect but now I think it should be included in this one.
Regarding the behavior, how do we want to proceed? Keep Caddy compatibility or try to make it compatible with Nginx?
In Nginx they check if the custom code falls between the 300-308 range, if it doesn't, the code is set to the default, which is the 301 : https://github.com/kubernetes/ingress-nginx/blob/7f723c59855e82614582ff7b2efd1783b1afc2ee/internal/ingress/annotations/redirect/redirect.go#L129
EDIT: I think our approach is fine. According to MDN, status codes 300-399 are redirection messages.
I've added the temporal-redirect
annotation to be compatible with Nginx. I don't mind changing it to temporary-redirect
, but I guess it's nice to be compatible with nginx.
Other things I wanted to ask:
@Xinayder you're doing an amazing job!
IMHO we can avoid to create a custom error and to do domain validation (people may decide to use internal domain as redirect and is very hard to predict what is going to be allowed or not).
Eventually can be iterated in the future, WDYT?
I think it's idiomatic to have custom error types in this case (correct me if I'm wrong). wrt validating URLs, you're right, validation would be a pain to setup to consider every possible case.
I think it's idiomatic to have custom error types in this case (correct me if I'm wrong).
Agree, but as part of internals we can add them later as they are not BC.
BTW, when you think can be reviewed plz mark the PR as "Ready for review" and I'll try to take a look ASAP.
@Xinayder thanks for your contribution! 🙌
This PR adds 2 new annotations:
permanent-redirect
permanent-redirect-code
permanent-redirect
will build astatic_response
handler for Caddy, redirecting requests to the value of the annotation field.permanent-redirect-code
can be used to set custom HTTP codes for the redirect. It supports Caddy's values likepermanent
ortemporary
. If this annotation is not set, it defaults to 301.Test results
Without
permanent-redirect-code
With
permanent-redirect-code
set to 308