Fix hsts support when not using SSL

[bilby91]

Closes #627

Proposed changes

This change will fix the HSTS support when not handling SSL at the ingress layer. Previously, the templating code would only render the HSTS related directives if the SSL was true which is never the case when a load balancer that sits in front of the ingress handle the TLS.

Fixes the following issue: #627

Checklist

Before creating a PR, run through this checklist and mark each as complete.

• I have read the CONTRIBUTING doc
• I have added tests that prove my fix is effective or that my feature works
• I have checked that all unit tests pass after adding my changes
• I have updated necessary documentation
• I have rebased my branch onto master
• I will ensure my PR is targeting the master branch and pulling from my branch from my own fork

[pleshakov]

@bilby91 thanks for the PR! we will review it shortly.

[Dean-Coakley]

This PR fixes the issue at first glance but there appears to be some issues.

If hsts, hsts-behind-proxy and redirect-to-https are set in the annotations. The following warning appears:

*84 using uninitialized "hsts_header_val" variable

This occurs because it hits the redirect in server.SSLRedirect before the headers are added. Some some additional handling needs to be done here.

[bilby91]

@Dean-Coakley Good catch, I totally missed that. Do you think that moving the block of code on top of {{if $server.SSL}} would make sense ?

BTW, do you have a cheap way of testing this ? The way I tested this took me some time to get stuff setup.

[Dean-Coakley]

@bilby91 I think this issue is more complex than we initially expected. I'd recommend waiting until we can provide you with some guidance on how to meet all edge cases. But feel free to share commits/comments in the meantime if you feel so inclined.

There is no way to test without a cluster - if that's what you're hoping for. However you don't need to test with a cloud loadbalancer. I'm testing against a minikube cluster with the complete-example deployed. And then use curl curl -k http://<MINIKUBE>/tea -I -L -H "X-Forwarded-Proto: https" Perhaps that's easier for you.

You can also run the binary locally against a running cluster. You'd set this up by

1. Compiling the binary go build -installsuffix cgo -ldflags "-w -X main.version=my-version" -o nginx-ingress github.com/nginxinc/kubernetes-ingress/cmd/nginx-ingress
2. Run kubectl proxy is a separate terminal window
3. Make sure the default server secret is deployed in your cluster
4. Run the binary ./nginx-ingress -proxy 127.0.0.1:8001 -default-server-tls-secret=nginx-ingress/default-server-secret -v=3 -main-template-path=internal/nginx/templates/nginx.tmpl -ingress-template-path=internal/nginx/templates/nginx.ingress.tmpl

This will help you debug template compilation. However please note this will not substitute testing in a cluster. There is no nginx running here. You can use this for dev purposes before testing in a cluster.

[pleshakov]

@bilby91
we reviewed the PR more thoroughly. Please see the suggestions. Thanks!

[pleshakov]

with this suggestion HSTS will only be enabled when TLS termination is enabled and hsts-behind-proxy annotation/configmapkey is true.

Suggested change

	{{- if $server.HSTS}}
	{{- if and $server.HSTS (or $server.SSL $server.HSTSBehindProxy)}}

[pleshakov]

Additionally, please move lines 66-78 (the {{- if and $server.HSTS (or $server.SSL $server.HSTSBehindProxy)}} ... {{end}} block) above line 56 ({{if $server.SSL}}). This will fix the warning message like this *84 using uninitialized "hsts_header_val" variable.

[bilby91]

@pleshakov Thanks for taking the time to review the change! The suggested changes would apply for nginx-plus and regular nginx right ? Just making sure!

[pleshakov]

yep. for both templates! thanks for double checking.

[bilby91]

Pushed suggested changes!

[pleshakov]

👍

[Dean-Coakley]

Thanks a lot @bilby91 !