Add support for redirect https to https (from-to-www-redirect) #3637
Conversation
aledbf
added
the
do-not-merge/hold
k8s-ci-robot
added
the
size/L
k8s-ci-robot
added
approved
aledbf
force-pushed
the
fix-redirect
branch
3 times, most recently
from
2ef4ad4
to
7b1a9b0
Compare
aledbf
removed
the
do-not-merge/hold
internal/ingress/controller/nginx.go
Outdated
| @@ -1022,3 +1004,62 @@ func cleanTempNginxCfg() error { | |||
|
|
|||
| return nil | |||
| } | |||
|
|
|||
| type redirects struct { | |||
There was a problem hiding this comment.
nitpick but this reads better as singular: redirect without s.
| r := &redirect{ | ||
| From: n, | ||
| To: srv.Hostname, | ||
| } |
There was a problem hiding this comment.
this is not consistent with https://github.com/kubernetes/ingress-nginx/pull/3637/files#diff-cde3fffe2425ad7efaa8add1d05ae2c0R1034. In the message it says redirect from srv.Hostname to n, but here you're reversing them.
There was a problem hiding this comment.
I think the log message need to be corrected, this construct itself is fine.
There was a problem hiding this comment.
hmm I don't see any change, it still says redirecting from hostname to n
There was a problem hiding this comment.
There was a problem hiding this comment.
@aledbf now that log is wrong :D If you check my initial comment I'm pointing to line 1034 not 1056.
There was a problem hiding this comment.
ok, I renamed the variables to avoid confusions 😉
| ing.Spec.TLS[0].Hosts, | ||
| ing.Spec.TLS[0].SecretName, | ||
| ing.Namespace) | ||
| } |
There was a problem hiding this comment.
To be consistent with the other e2e tests. I removed the helper, not the tests itself
|
@ElvinEfendi please review |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
k8s-ci-robot
removed
the
lgtm
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: aledbf, ElvinEfendi The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What this PR does / why we need it:
Verifies the generated server contains an SSL Certificate and the list of common names contains both FQDN.
Also, this PR adds support for wildcard names in the CN of the SSL certificate
Which issue this PR fixes:
fixes #2230
fixes #2043