support added for ingress.kubernetes.io/force-ssl-redirect #146
Conversation
poonai
force-pushed
the
force_ssl
branch
2 times, most recently
from
764dd8b
to
95012e0
Compare
|
Thank you for this PR. This will need some tests before I can review it. Thank you. |
There was a problem hiding this comment.
Thank you for this PR.
What I would like to see is the following.
- Please talk about your approach before you start coding. This should be on the issue Support ingress.kubernetes.io/force-ssl-redirect #63. You will find more details in the CONTRIBUTING document
- Add a fixes Support ingress.kubernetes.io/force-ssl-redirect #63 to the PR description so github will close the issue when this PR is merged.
- All changes must have tests.
internal/contour/listener.go
Outdated
| @@ -194,6 +194,19 @@ func (lc *ListenerCache) httpsAddress() string { | |||
| return DEFAULT_HTTPS_LISTENER_ADDRESS | |||
| } | |||
|
|
|||
| func (lc *listenerCache) HostHasTls(host string) bool { | |||
| for _, listener := range lc.Values() { | |||
There was a problem hiding this comment.
Please don't use lc.Values for this. Any information that you get out of lc.Values will be present in the other caches maintained by the listenerCache.
internal/contour/virtualhost.go
Outdated
| @@ -145,3 +150,10 @@ func clusteraction(cluster string) *v2.Route_Route { | |||
| }, | |||
| } | |||
| } | |||
|
|
|||
| func sslRedirect(i *v1beta1.Ingress) bool { | |||
| if i.Annotations["kubenetes.io/ingress.ssl-redirect"] == "false" { | |||
There was a problem hiding this comment.
this can be simplified to
return i.Annotations["kubenetes.io/ingress.ssl-redirect"] == "true"
Signed-off-by: பாலாஜி <rbalajis25@gmail.com>
Signed-off-by: பாலாஜி <rbalajis25@gmail.com>
Signed-off-by: பாலாஜி <rbalajis25@gmail.com>
|
@davecheney I have made changes. please review 👍 |
| @@ -194,6 +194,19 @@ func (lc *ListenerCache) httpsAddress() string { | |||
| return DEFAULT_HTTPS_LISTENER_ADDRESS | |||
| } | |||
|
|
|||
| func (lc *listenerCache) HostHasTls(host string) bool { | |||
There was a problem hiding this comment.
You cannot use values here without taking the lock.
Please discuss your design before sending code, this is the second time I have asked.
There was a problem hiding this comment.
Thank you, the tests look fine but I am not happy with the implementation. Please discuss your design on issue #63 before sending further changes to this PR.
There was a problem hiding this comment.
Thank you for your PR. It needs some changes which I will take care of before merging.
I'm grateful for your contribution and will welcome more from you but please discuss your changes before sending any more code as we must agree on a design before you start to code.
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
| ) | ||
|
|
||
| func TestSslRedirect(t *testing.T) { |
There was a problem hiding this comment.
This is a good test, but does not test the right thing. Then name of the annotation is
ingress.kubernetes.io/force-ssl-redirect
Fixes projectcontour#63 Closes projectcontour#146 Add support for the ingress.kubernetes.io/force-ssl-redirect: "true" annotation Signed-off-by: Dave Cheney <dave@cheney.net>
close #63
ingress.kubernetes.io/force-ssl-redirect annotation added for redirecting http to https.
Signed-off-by: பாலாஜி rbalajis25@gmail.com