Add permanent-redirect and permanent-redirect-code annotations

[Xinayder]

This PR adds 2 new annotations:

• permanent-redirect
• permanent-redirect-code

permanent-redirect will build a static_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 like permanent or temporary. If this annotation is not set, it defaults to 301.

Test results

Without permanent-redirect-code

apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  name: example
  annotations:
    kubernetes.io/ingress.class: caddy
    caddy.ingress.kubernetes.io/permanent-redirect: https://google.com
    caddy.ingress.kubernetes.io/backend-protocol: http
    caddy.ingress.kubernetes.io/disable-ssl-redirect: "true"
spec:
  rules:
  - host: example1.kubernetes.localhost
    http:
      paths:
      - path: /hello1
        pathType: Prefix
        backend:
          service:
            name: example1
            port:
              number: 8080

$ curl -H 'Host: example1.kubernetes.localhost' http://127.0.0.1:8080/hello1 -v
*   Trying 127.0.0.1:8080...
* Connected to 127.0.0.1 (127.0.0.1) port 8080
> GET /hello1 HTTP/1.1
> Host: example1.kubernetes.localhost
> User-Agent: curl/8.4.0
> Accept: */*
> 
< HTTP/1.1 301 Moved Permanently
< Alt-Svc: h3=":443"; ma=2592000
< Location: https://google.com
< Server: Caddy
< Date: Sat, 21 Oct 2023 14:58:19 GMT
< Content-Length: 0
< 
* Connection #0 to host 127.0.0.1 left intact

With permanent-redirect-code set to 308

apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  name: example
  annotations:
    kubernetes.io/ingress.class: caddy
    caddy.ingress.kubernetes.io/permanent-redirect: https://google.com
    caddy.ingress.kubernetes.io/permanent-redirect-code: "308"
    caddy.ingress.kubernetes.io/backend-protocol: http
    caddy.ingress.kubernetes.io/disable-ssl-redirect: "true"
spec:
  rules:
  - host: example1.kubernetes.localhost
    http:
      paths:
      - path: /hello1
        pathType: Prefix
        backend:
          service:
            name: example1
            port:
              number: 8080

$ curl -H 'Host: example1.kubernetes.localhost' http://127.0.0.1:8080/hello1 -v
*   Trying 127.0.0.1:8080...
* Connected to 127.0.0.1 (127.0.0.1) port 8080
> GET /hello1 HTTP/1.1
> Host: example1.kubernetes.localhost
> User-Agent: curl/8.4.0
> Accept: */*
> 
< HTTP/1.1 308 Permanent Redirect
< Alt-Svc: h3=":443"; ma=2592000
< Location: https://google.com
< Server: Caddy
< Date: Sat, 21 Oct 2023 14:57:03 GMT
< Content-Length: 0
< 
* Connection #0 to host 127.0.0.1 left intact

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (cafb53c) 42.19% compared to head (f215e19) 43.68%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #165      +/-   ##
==========================================
+ Coverage   42.19%   43.68%   +1.49%     
==========================================
  Files          12       17       +5     
  Lines         365      515     +150     
==========================================
+ Hits          154      225      +71     
- Misses        210      289      +79     
  Partials        1        1              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[mavimo]

Hey @Xinayder, thanks for your contribution!

I added some comments, let me know if you agree and eventually if I can help 😄

[mavimo]

@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?

• https://kubernetes.github.io/ingress-nginx/user-guide/nginx-configuration/annotations/#permanent-redirect
• https://kubernetes.github.io/ingress-nginx/user-guide/nginx-configuration/annotations/#permanent-redirect-code
• https://kubernetes.github.io/ingress-nginx/user-guide/nginx-configuration/annotations/#temporal-redirect
  I don't think is too much different from what you're doing here but make thing more understandable for a newcomer...

[mavimo]

PS: test are failing, I think tests do not reflect the code changes made in 660815b

[Xinayder]

@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.

[Xinayder]

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:

• Should I create custom error types for the plugin, so whenever their wording changes we don't need to change the tests?
• Should there be input validation for the annotation values, e.g., to check if they are valid URLs? Or is it already done by Caddy?

[mavimo]

@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?

[Xinayder]

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.

[mavimo]

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.

[mavimo]

@Xinayder thanks for your contribution! 🙌