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

feat: 💥 support http redirections and http challenges with cert-manager #934

Merged
merged 6 commits into from
Oct 19, 2023

Conversation

renebarbosafl
Copy link
Contributor

@renebarbosafl renebarbosafl commented Oct 12, 2023

What does this PR do?

We found that when using the HTTP redirections feature (to force usage of HTTPS) all cert-manager challenges to issue new certificates are failing, this PR adds a new priority parameter that actually enables Traefik to work with cert-manager even if using HTTP redirections.

This helm chart does not have any tests on enabling this feature either so I added it.

Motivation

The solution was found here:
https://community.traefik.io/t/solved-http-to-https-redirect-ends-in-an-https-url-with-8443-appended/10136/5

(Thanks to matthiasbaldi)

More

  • Yes, I updated the tests accordingly
  • Yes, I ran make test and all the tests passed

Copy link
Contributor

@darkweaver87 darkweaver87 left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution to Traefik helm chart.

It's more common with Traefik to use TLS-ALPN-01 with Traefik ACME provider or DNS challenge of cert-manager.

Having that said, this implementation is too specific for many users.

What do you think about changing

ports:
  web:
      redirectTo: websecure

to:

ports:
  web:
    redirectTo:
      port: websecure
      priority: 10

?

@renebarbosafl
Copy link
Contributor Author

@darkweaver87
It makes sense! I pushed a new commit with this change.
Probably it's a breaking change for users of this feature (they'll need to update their values.yaml files).

@mloiseleur mloiseleur added the kind/enhancement New feature or request label Oct 14, 2023
@mloiseleur mloiseleur changed the title fix: http redirections breaking cert-manager challenges feat: support http redirections and http challenges with cert-manager Oct 14, 2023
@mloiseleur mloiseleur changed the title feat: support http redirections and http challenges with cert-manager feat: 💥 support http redirections and http challenges with cert-manager Oct 16, 2023
Copy link
Contributor

@mloiseleur mloiseleur left a comment

Choose a reason for hiding this comment

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

@renebarbosafl For users using dns challenges and this redirect, it will fail unless they set a priority.
=> priority should be an optional settings

@renebarbosafl
Copy link
Contributor Author

Ok @mloiseleur!
I pushed a new commit changing this feature to Optional and updating tests accordingly.

Copy link
Contributor

@darkweaver87 darkweaver87 left a comment

Choose a reason for hiding this comment

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

LGTM

@traefiker traefiker merged commit 8cf14c8 into traefik:master Oct 19, 2023
1 check passed
mxmxchere pushed a commit to mxmxchere/terraform-hcloud-kube-hetzner that referenced this pull request Oct 23, 2023
Traefik Chart v25.0.0 was released with a breaking change
(traefik/traefik-helm-chart#934).
This is a quick fix to pin to the old behaviour.

Signed-off-by: Malte Muench <[email protected]>
mxmxchere added a commit to mxmxchere/terraform-hcloud-kube-hetzner that referenced this pull request Oct 23, 2023
Traefik Chart v25.0.0 was released with a breaking change
(traefik/traefik-helm-chart#934).
This is a quick fix to pin to the old behaviour.

Signed-off-by: Malte Muench <[email protected]>
mxmxchere added a commit to mxmxchere/terraform-hcloud-kube-hetzner that referenced this pull request Oct 23, 2023
Traefik Chart v25.0.0 was released with a breaking change
(traefik/traefik-helm-chart#934).
This is a quick fix to pin to the old behaviour.

Signed-off-by: Malte Münch <[email protected]>
robkooper added a commit to ncsa/radiant-cluster that referenced this pull request Oct 23, 2023
h3mmy added a commit to h3mmy/bloopySphere that referenced this pull request Oct 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants