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

ingress: adds configurable SSL redirect nginx controller #850

Merged
merged 2 commits into from Jun 8, 2016

Conversation

Projects
None yet
6 participants
@simonswine
Member

simonswine commented Apr 26, 2016

Hi everyone,

I want my services behind the ingress to decide if HTTPS is strictly required or not. So I added the feature to disable the 301 redirect to the https:/ URL.

To make this actually work #849 needs to be merged before. (Otherwise you cannot override and default value true with a false)

Cheers,

Christian

@googlebot googlebot added the cla: yes label Apr 26, 2016

@aledbf

This comment has been minimized.

Show comment
Hide comment
@aledbf

aledbf Apr 26, 2016

Member

@simonswine I fixed that issue in #766 (not merged yet) using use-hts=false in the configmap

Member

aledbf commented Apr 26, 2016

@simonswine I fixed that issue in #766 (not merged yet) using use-hts=false in the configmap

@simonswine

This comment has been minimized.

Show comment
Hide comment
@simonswine

simonswine Apr 26, 2016

Member

Hi @aledbf, I missed your work on that. I am not too sure why it's not merged. I guess your PR is rather large and doing a lot of different things.

How and where exactly did you solve the default value overriding problem with bool (default=true) and you want to have it set false via ConfigMap. See fix + tests in #849

Member

simonswine commented Apr 26, 2016

Hi @aledbf, I missed your work on that. I am not too sure why it's not merged. I guess your PR is rather large and doing a lot of different things.

How and where exactly did you solve the default value overriding problem with bool (default=true) and you want to have it set false via ConfigMap. See fix + tests in #849

@bprashanth

This comment has been minimized.

Show comment
Hide comment
@bprashanth

bprashanth May 2, 2016

Member

I'm fine with a smaller change, doens't matter either way how it gets in i guess, as long as we alll agree on how it's expressed. You and @aledbf can choose which one of the 2 prs goes in :)

Member

bprashanth commented May 2, 2016

I'm fine with a smaller change, doens't matter either way how it gets in i guess, as long as we alll agree on how it's expressed. You and @aledbf can choose which one of the 2 prs goes in :)

@aledbf

This comment has been minimized.

Show comment
Hide comment
@aledbf

aledbf May 4, 2016

Member

@simonswine please check the latest version gcr.io/google_containers/nginx-ingress-controller:0.61. Contains #766 and #849. Use hts=false in the nginx configmap.

Member

aledbf commented May 4, 2016

@simonswine please check the latest version gcr.io/google_containers/nginx-ingress-controller:0.61. Contains #766 and #849. Use hts=false in the nginx configmap.

@simonswine

This comment has been minimized.

Show comment
Hide comment
@simonswine

simonswine May 5, 2016

Member

@aledbf thanks for that update. Thinking a bit about it, I came to the conclusion that we probably want to be able to configure HSTS, and server-side SSL redirect enforcement separately.

For my particular use case I want to be always able to access something via HTTP (if the clients wants to do so). It's needed for doing the http-01 challenge for Let's Encrypt. At the same time I want to be able to enforce HTTPS via HSTS for client that support that. Ultimately I want to be able to do that on location level (see #904).

Member

simonswine commented May 5, 2016

@aledbf thanks for that update. Thinking a bit about it, I came to the conclusion that we probably want to be able to configure HSTS, and server-side SSL redirect enforcement separately.

For my particular use case I want to be always able to access something via HTTP (if the clients wants to do so). It's needed for doing the http-01 challenge for Let's Encrypt. At the same time I want to be able to enforce HTTPS via HSTS for client that support that. Ultimately I want to be able to do that on location level (see #904).

@aledbf

This comment has been minimized.

Show comment
Hide comment
@aledbf

aledbf May 8, 2016

Member

I came to the conclusion that we probably want to be able to configure HSTS, and server-side SSL redirect enforcement separately.

Yes, you are right about this. How about defining the redirect and hsts in the configuration to define a default value and adding an annotation in the services? This way is possible to just enable things like http-01 challenge and force the behavior with tls rules with existing certificates

Member

aledbf commented May 8, 2016

I came to the conclusion that we probably want to be able to configure HSTS, and server-side SSL redirect enforcement separately.

Yes, you are right about this. How about defining the redirect and hsts in the configuration to define a default value and adding an annotation in the services? This way is possible to just enable things like http-01 challenge and force the behavior with tls rules with existing certificates

@rh-kube-bot

This comment has been minimized.

Show comment
Hide comment
@rh-kube-bot

rh-kube-bot May 16, 2016

Can one of the admins verify this patch?

rh-kube-bot commented May 16, 2016

Can one of the admins verify this patch?

1 similar comment
@rh-kube-bot

This comment has been minimized.

Show comment
Hide comment
@rh-kube-bot

rh-kube-bot May 16, 2016

Can one of the admins verify this patch?

rh-kube-bot commented May 16, 2016

Can one of the admins verify this patch?

@simonswine simonswine changed the title from Adds configurable SSL redirect to Nginx Ingress Controller to ingress: adds configurable SSL redirect nginx controller May 25, 2016

@simonswine

This comment has been minimized.

Show comment
Hide comment
@simonswine

simonswine May 25, 2016

Member
  • add global value to config map
  • add per service value as annotation to service object (ingress-nginx.kubernetes.io/ssl-redirect: "false")

@aledbf could please take a look at my changes. They are still a WiP. It felt like it is a bit messy implemented.

Definitely missing are docs

Member

simonswine commented May 25, 2016

  • add global value to config map
  • add per service value as annotation to service object (ingress-nginx.kubernetes.io/ssl-redirect: "false")

@aledbf could please take a look at my changes. They are still a WiP. It felt like it is a bit messy implemented.

Definitely missing are docs

@simonswine simonswine changed the title from ingress: adds configurable SSL redirect nginx controller to [WiP] ingress: adds configurable SSL redirect nginx controller May 25, 2016

@aledbf

This comment has been minimized.

Show comment
Hide comment
@aledbf

aledbf May 25, 2016

Member

@simonswine looks good 👍
I like the getServiceAnnotations addition. Can you make this public? (some of the pending PR like the auth are located in a different package)

Just the server.SetCfg(cfgCamelCase) before the template is what I don't "like". If the user specifies --v=3 it will log the content of the map before the rendering of the template with the cfg repeated N times (one per server)

Member

aledbf commented May 25, 2016

@simonswine looks good 👍
I like the getServiceAnnotations addition. Can you make this public? (some of the pending PR like the auth are located in a different package)

Just the server.SetCfg(cfgCamelCase) before the template is what I don't "like". If the user specifies --v=3 it will log the content of the map before the rendering of the template with the cfg repeated N times (one per server)

@aledbf

This comment has been minimized.

Show comment
Hide comment
@aledbf

aledbf May 25, 2016

Member

@simonswine why not the annotation in the ingress instead of the service?

Member

aledbf commented May 25, 2016

@simonswine why not the annotation in the ingress instead of the service?

@simonswine

This comment has been minimized.

Show comment
Hide comment
@simonswine

simonswine May 26, 2016

Member

@aledbf

why not the annotation in the ingress instead of the service?

As I want it to be able to config stuff on Location-level. If this has to be done with ingress controller annotations, their key has to contain the path as well. This felt a bit ugly.

SetCfg/cfg in Server struct

It's modified now:

  • the struct field is public
  • we use a pointer instead of a map

GetServiceAnnotations

Is now public

Docs

added

Member

simonswine commented May 26, 2016

@aledbf

why not the annotation in the ingress instead of the service?

As I want it to be able to config stuff on Location-level. If this has to be done with ingress controller annotations, their key has to contain the path as well. This felt a bit ugly.

SetCfg/cfg in Server struct

It's modified now:

  • the struct field is public
  • we use a pointer instead of a map

GetServiceAnnotations

Is now public

Docs

added

@eparis eparis added the area/ingress label May 26, 2016

@simonswine simonswine changed the title from [WiP] ingress: adds configurable SSL redirect nginx controller to ingress: adds configurable SSL redirect nginx controller May 27, 2016

@simonswine

This comment has been minimized.

Show comment
Hide comment
@simonswine

simonswine May 27, 2016

Member

@eparis IMHO now ready to merge

Member

simonswine commented May 27, 2016

@eparis IMHO now ready to merge

@aledbf

This comment has been minimized.

Show comment
Hide comment
@aledbf

aledbf May 27, 2016

Member

their key has to contain the path as well....

Why, just create a restriction: using this annotation in an ingress rules applies to all the paths
(if I want multiple paths with the same host I just need to create multiple rules with 1 path)

As I want it to be able to config stuff on Location-level....

I know, but if the annotation is in the service and not the ingress rule then:

  • There's two places to indicate a "routing" behavior, ingress rule and service.
  • If the annotation is in the service I cannot create two rules with different behavior using the same service (I need to duplicate the service)
Member

aledbf commented May 27, 2016

their key has to contain the path as well....

Why, just create a restriction: using this annotation in an ingress rules applies to all the paths
(if I want multiple paths with the same host I just need to create multiple rules with 1 path)

As I want it to be able to config stuff on Location-level....

I know, but if the annotation is in the service and not the ingress rule then:

  • There's two places to indicate a "routing" behavior, ingress rule and service.
  • If the annotation is in the service I cannot create two rules with different behavior using the same service (I need to duplicate the service)
@aledbf

This comment has been minimized.

Show comment
Hide comment
@aledbf

aledbf May 31, 2016

Member

@simonswine can you rebase?
I'm working on #1104 and I would really love to get this merged

Member

aledbf commented May 31, 2016

@simonswine can you rebase?
I'm working on #1104 and I would really love to get this merged

@simonswine

This comment has been minimized.

Show comment
Hide comment
@simonswine

simonswine Jun 3, 2016

Member

@aledbf sorry I haven't managed to get this done before 0.7.

I can see your point in the ingress vs. service resource discussion. It feels cleaner to me now to do it in the ingress. I will modify this accordingly..

Member

simonswine commented Jun 3, 2016

@aledbf sorry I haven't managed to get this done before 0.7.

I can see your point in the ingress vs. service resource discussion. It feels cleaner to me now to do it in the ingress. I will modify this accordingly..

simonswine added some commits Jun 5, 2016

ingress: adds configurable SSL redirect nginx controller
* add global value to config map
* add per ingress value as annotation to ingress resources
@simonswine

This comment has been minimized.

Show comment
Hide comment
@simonswine

simonswine Jun 5, 2016

Member

@aledbf @bprashanth

Refactored the code to use ingress resource annotations. It is now part of rewrite package.

Moved config into own package to prevent dependency cycles in the imports

Member

simonswine commented Jun 5, 2016

@aledbf @bprashanth

Refactored the code to use ingress resource annotations. It is now part of rewrite package.

Moved config into own package to prevent dependency cycles in the imports

// SSL enabled protocols to use
// http://nginx.org/en/docs/http/ngx_http_ssl_module.html#ssl_protocols
sslProtocols = "TLSv1 TLSv1.1 TLSv1.2"

This comment has been minimized.

@bprashanth

bprashanth Jun 6, 2016

Member

I know this is just cut/paste, but why include tls1 as a default?

@bprashanth

bprashanth Jun 6, 2016

Member

I know this is just cut/paste, but why include tls1 as a default?

This comment has been minimized.

@aledbf

aledbf Jun 6, 2016

Member

This comment has been minimized.

@bprashanth

bprashanth Jun 7, 2016

Member

I was suggesting an opt in, not default. I'm ok if we really need it, but tls 1 is compromised and people can just force a downgrade and BEAST. Please add a comment.

@bprashanth

bprashanth Jun 7, 2016

Member

I was suggesting an opt in, not default. I'm ok if we really need it, but tls 1 is compromised and people can just force a downgrade and BEAST. Please add a comment.

This comment has been minimized.

@aledbf

aledbf Jun 7, 2016

Member

@bprashanth just in case the defaults in sslCiphers and sslProtocols mitigate BEAST

captura de pantalla 2016-06-06 a las 9 20 28 p m

(screenshot from ssllabs.com/ssltest/analyze.html of a site with defaults)

@aledbf

aledbf Jun 7, 2016

Member

@bprashanth just in case the defaults in sslCiphers and sslProtocols mitigate BEAST

captura de pantalla 2016-06-06 a las 9 20 28 p m

(screenshot from ssllabs.com/ssltest/analyze.html of a site with defaults)

This comment has been minimized.

@bprashanth

bprashanth Jun 8, 2016

Member

alright, SG, lets add it to the readme somewhere?

@bprashanth

bprashanth Jun 8, 2016

Member

alright, SG, lets add it to the readme somewhere?

This comment has been minimized.

@aledbf

aledbf Jun 8, 2016

Member
@bprashanth

This comment has been minimized.

Show comment
Hide comment
@bprashanth

bprashanth Jun 6, 2016

Member

LGTM but for the nit, thanks for the package cleanup also

Member

bprashanth commented Jun 6, 2016

LGTM but for the nit, thanks for the package cleanup also

@aledbf

This comment has been minimized.

Show comment
Hide comment
@aledbf

aledbf Jun 7, 2016

Member

LGTM

Member

aledbf commented Jun 7, 2016

LGTM

@bprashanth

This comment has been minimized.

Show comment
Hide comment
@bprashanth

bprashanth Jun 8, 2016

Member

Spinoff pr for docs sounds good, merging

Member

bprashanth commented Jun 8, 2016

Spinoff pr for docs sounds good, merging

@bprashanth bprashanth merged commit 1ec21d8 into kubernetes:master Jun 8, 2016

2 checks passed

cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details

cdemers pushed a commit to cdemers/contrib that referenced this pull request Feb 14, 2017

Prashanth B
Merge pull request kubernetes#850 from simonswine/feature-ssl-redirect
ingress: adds configurable SSL redirect nginx controller
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment