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

do ssl-redirect only if tls declares the hostname #465

Merged
merged 1 commit into from
Nov 29, 2019

Conversation

jcmoraisjr
Copy link
Owner

Ingress.spec.tls declares hostnames/domains and an optional secret with crt+key used to ssl-offload https requests on that domains. If a secret name is not declared or is invalid, the default crt+key is used instead.

This commit fixes a misbehaviour when ssl-redirect is true, declared or inherited from configmap, without a TLS being declared. Now ssl-redirect only return 302 redirect if ingress.spec.tls declares the hostname/domain.

Should be merged to v0.8.

Ingress.spec.tls declares hostnames/domains and an optional secret with crt+key used to ssl-offload https requests on that domains. If a secret name is not declared or is invalid, the default crt+key is used instead.

This commit fixes a misbehaviour when ssl-redirect is true, declared or inherited from configmap, without a TLS being declared. Now ssl-redirect only return 302 redirect if ingress.spec.tls declares the hostname/domain.

Should be merged to v0.8.
@prometherion
Copy link
Contributor

I tested this branch and it seems to address the issue: well done!

However, considered the following ingress:

apiVersion: networking.k8s.io/v1beta1
kind: Ingress
metadata:
  name: foo
spec:
  tls:
    - hosts:
      - 42.dontpanic.space
  rules:
    - host: 42.dontpanic.space
      http:
        paths:
          - path: /
            backend:
              serviceName: dontpanic
              servicePort: 80

I can reach the host both on plain HTTP and HTTPS: in other Ingress controllers like NGINX, the default behavior is to force automatically the upgrade to HTTPS but it seems not happening here.

Even considered that this could be the default behavior, do you plan to achieve that goal?
From an end-user perspective, if I'm declaring an Ingress with a TLS host, I'm expecting to get the redirect by default.
Despite of that, if I would like to serve both HTTP and HTTPS I just need to use the annotation ingress.kubernetes.io/ssl-redirect=false

@prometherion
Copy link
Contributor

Nah, my bad: ignore my last comment... it works as expected but I was building this branch with other changes that weren't handling correctly the redirection.

@jcmoraisjr jcmoraisjr merged commit 6aea47c into master Nov 29, 2019
@jcmoraisjr jcmoraisjr deleted the jm-tls-ssl-redirect branch November 29, 2019 23:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants