-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
Add support for redirect https to https (from-to-www-redirect) #3637
Conversation
2ef4ad4
to
7b1a9b0
Compare
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick but this reads better as singular: redirect
without s
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
r := &redirect{ | ||
From: n, | ||
To: srv.Hostname, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the log message need to be corrected, this construct itself is fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm I don't see any change, it still says redirecting from hostname to n
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you removing this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be consistent with the other e2e tests. I removed the helper, not the tests itself
@ElvinEfendi please review |
/lgtm |
/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