Add support for fetching k8s Ingress TLS data from secrets #2439

gopenguin · 2017-11-20T19:56:52Z

What does this PR do?

Enable Traefik to read the k8s secret defined in a k8s ingress resource via the tls property.

Motivation

This allows a k8s user to easily setup a tls terminated endpoint

More

Added/updated tests
Added/updated documentation

Additional Notes

This is my first try on this. I would be pleased to get your opinion on this. This is the implementation to #2438

ldez · 2017-11-20T20:14:43Z

@gopenguin Could you fix?

---> Making bundle: validate-autogen (in .)
checking autogen is up-to-date with templates...
The result of 'go generate' differs

 M autogen/gentemplates/gen.go

Please do "go generate" to update the `autogen` package.

gopenguin · 2017-11-20T20:22:56Z

@ldez When I regenerate the file all file modes change from os.FileMode(436) to os.FileMode(420). Is this supposed to happen?

ldez · 2017-11-20T21:43:54Z

It's not the expected behavior.
Don't change nothing for now, I fix this problem in #2440.

gopenguin · 2017-11-21T07:43:25Z

@ldez I've just pulled your changes and I think there is sill an issue with the generated file. It isn't go fmt'ed but the version in the repo is.

gopenguin · 2017-11-21T07:45:16Z

If you just run go generate, the file changes but after running go fmt the changes are gone.

errm

I am happy with the code, looks clean, seems to have relevant unit tests. Thanks for working on this @gopenguin I think its a feature a lot of K8s folks have been waiting on.

I want to do a little manual testing before we merge, but I will wait for the generate stuff to be settled first.

We will need some documentation, but I am happy to help out/write it/whatever if you need @gopenguin?

gopenguin · 2017-11-21T10:42:19Z

I had some time this morning and wrote some docs but I realy appreciate any help.

timoreimann · 2017-11-21T12:41:31Z

provider/kubernetes/kubernetes.go

@@ -18,6 +18,7 @@ import (
 	"github.com/containous/traefik/log"
 	"github.com/containous/traefik/provider"
 	"github.com/containous/traefik/safe"
+	traefikTls "github.com/containous/traefik/tls"


I think it's more conventional in Go to not camel case import names. I'd suggest ttls or traefiktls instead.

I thought, I'd reuse the prefix used elswhere in this project like in acme/acme.go, cmd/traefik/traefik.go, server/server.go and types/types.go but I can't remember why I introduced the prefix in the first place so I will remove it.

It's quite possible we used camel case elsewhere. If we do, we should fix those (by a separate PR).

timoreimann · 2017-11-21T12:42:14Z

provider/kubernetes/kubernetes.go

+		for _, t := range i.Spec.TLS {
+			tlsSecret, exists, err := k8sClient.GetSecret(i.Namespace, t.SecretName)
+			if err != nil {
+				log.Errorf("Unable to retrive secret %s/%s: %s", i.Namespace, t.SecretName, err)


Typo: retrieve

timoreimann · 2017-11-21T12:43:25Z

provider/kubernetes/kubernetes.go

+				continue
+			}
+			_, tlsCrtExists := tlsSecret.Data["tls.crt"]
+			_, tlsKeyExists := tlsSecret.Data["tls.key"]


We seem to need both ignored variables later, so let's store them here for reusal below.

timoreimann · 2017-11-21T12:44:10Z

provider/kubernetes/kubernetes.go

+			}
+			_, tlsCrtExists := tlsSecret.Data["tls.crt"]
+			_, tlsKeyExists := tlsSecret.Data["tls.key"]
+			if !tlsCrtExists || !tlsKeyExists {


Can we split this up into two dedicated errors?

Should there also be a specific error for both entries missing to speed up the errorhandling?

timoreimann · 2017-11-21T12:44:42Z

provider/kubernetes/kubernetes.go

+			_, tlsCrtExists := tlsSecret.Data["tls.crt"]
+			_, tlsKeyExists := tlsSecret.Data["tls.key"]
+			if !tlsCrtExists || !tlsKeyExists {
+				log.Warnf("Secret %s/%s is not of type 'kubernetes.io/tls'", i.Namespace, t.SecretName)


Is the error message correct?

Sorry, no, this message isn't correct anymore

timoreimann

One more minor thing.

Looks good overall. Will do another final check tonight.

timoreimann · 2017-11-21T16:28:13Z

provider/kubernetes/kubernetes_test.go

@@ -2262,6 +2274,169 @@ func TestBasicAuthInTemplate(t *testing.T) {
 	}
 }

+func TestTlsSecret(t *testing.T) {


TLS should be all caps.

timoreimann · 2017-11-22T00:13:42Z

provider/kubernetes/kubernetes.go

+			if len(t.Hosts) > 0 {
+				hosts = t.Hosts
+			} else {
+				// If no hosts are provided the hosts property should default ot the wildcard host. To ensure that the


Typo: ot instead of to.

timoreimann · 2017-11-22T00:18:09Z

provider/kubernetes/kubernetes.go

+		for _, t := range i.Spec.TLS {
+			tlsSecret, exists, err := k8sClient.GetSecret(i.Namespace, t.SecretName)
+			if err != nil {
+				log.Errorf("Unable to retrieve secret %s/%s: %s", i.Namespace, t.SecretName, err)


Please use Failed instead of Unable to be consistent with the existing wording.

timoreimann · 2017-11-22T00:19:09Z

provider/kubernetes/kubernetes.go

+				continue
+			}
+			if !exists {
+				log.Warnf("Unable to find secret %s/%s", i.Namespace, t.SecretName)


find here and retrieve above are very similar. Can we say something like Secret %s/%s does not exist?

timoreimann · 2017-11-22T00:24:41Z

provider/kubernetes/kubernetes.go

+			}
+
+			templateObjects.TLSConfiguration = append(templateObjects.TLSConfiguration, tlsConfig)
+		}


Can we put the TLS logic into a dedicated function?

timoreimann · 2017-11-22T00:25:34Z

provider/kubernetes/kubernetes_test.go

@@ -310,6 +311,7 @@ func TestLoadIngresses(t *testing.T) {
 				},
 			},
 		},
+		TLSConfiguration: []*tls.Configuration{},


Is it needed to specify an empty TLSConfiguration for tests unrelated to the TLS functionality?

If no, then I'd prefer to omit those here and further below.

Because I used the empty slice it was necessary (nil slice != empty slice) but I didn't know that you could append to a nil slice without any errors.

timoreimann · 2017-11-22T00:29:56Z

provider/kubernetes/kubernetes.go

+					log.Warnf("Secret %s/%s doesn't have an entry named 'tls.key'", i.Namespace, t.SecretName)
+				}
+				continue
+			}


None of the four error cases above seem to have test coverage. Can we improve here?

timoreimann · 2017-11-22T00:30:56Z

provider/kubernetes/kubernetes.go

+						hosts = append(hosts, r.Host)
+					}
+				}
+			}


My code coverage indicator tells me that the else branch does not have a corresponding test. We should change that.

timoreimann · 2017-11-22T00:36:03Z

provider/kubernetes/kubernetes.go

+				// If no hosts are provided the hosts property should default ot the wildcard host. To ensure that the
+				// tls config only matches rules in this ingress, add all the used hosts to simulate a wildcard like
+				// behavior.
+				hosts = []string{}


If tls.Configuration.EntryPoints is fine with nil slices, you can drop this line as we have already defined the nil slice in line 337, and Go can append to such slices without problems.

If tls.Configuration.EntryPoints requires a non-nil empty slice, then you can replace line 337 with this one (i.e., initialize an empty slice right away).

gopenguin · 2017-11-22T07:59:05Z

Thanks for the review. I'll probably get it fixed by tomorrow evening.

Move the extraction of the tls cert definition into its own method to improve readability and testability. Add tests for error cases and wildcard behavior.

nmengin

Many thanks @gopenguin for this awesome PR ! 👏 👏

LGTM

timoreimann

Left some more nit-picks; I'm generally fine with the PR, just need to make sure that we handle error cases appropriately. (See one of my comments pointing to a previous discussion.)

timoreimann · 2017-12-21T10:14:42Z

docs/user-guide/kubernetes.md

+          serviceName: traefik-web-ui
+          servicePort: 80
+  tls:
+  - hosts:


Hmm, I don't see us parsing the hosts list in our code explicitly. Am I missing something?

timoreimann · 2017-12-21T10:16:34Z

docs/user-guide/kubernetes.md

+
+```shell
+openssl req -x509 -nodes -days 365 -newkey rsa:2048 -keyout /tmp/tls.key -out /tmp/tls.crt -subj "/CN=traefik-ui.minikube"
+kubectl -n kube-system create secret tls traefik-ui-tls-cert --key=/tmp/tls.key --cert=/tmp/tls.crt


I'd drop the /tmp directory part as it may not exist on every system. Saving the key and cert in the current working directory seems good enough for me.

timoreimann · 2017-12-21T10:17:28Z

docs/user-guide/kubernetes.md

+!!! note
+    For this example to work you need a TLS entrypoint. You don't have to provide a TLS certificate at this point. For more details see [here](/configuration/entrypoints/).
+
+To setup a HTTPS protected ingress, you can leverage the TLS feature of the ingress resource.


s/a HTTPS protected/an HTTPS-protected/

timoreimann · 2017-12-21T10:17:45Z

docs/user-guide/kubernetes.md

+    The secret must have two entries named `tls.key`and `tls.crt`. See the [kubernetes documentation](https://kubernetes.io/docs/concepts/services-networking/ingress/#tls) for more details.
+
+!!! note
+    The TLS certificates will be added to all entrypoints defined by the ingress annotation `traefik.frontend.entryPoints`. If no such annotation is provided, the TLS certificates will be added to all TLS enabled `defaultEntryPoints`.


s/to all TLS enabled/to all TLS-enabled/

timoreimann · 2017-12-21T10:19:01Z

provider/kubernetes/builder_ingress_test.go

@@ -92,6 +92,23 @@ func iBackend(name string, port intstr.IntOrString) func(*v1beta1.HTTPIngressPat
 	}
 }

+func iTLSs(opts ...func(*v1beta1.IngressTLS)) func(*v1beta1.Ingress) {


Can we rename this to iTLSes? Makes it slightly more readable (and grammatically correct) IMHO.

timoreimann · 2017-12-21T10:22:08Z

provider/kubernetes/kubernetes.go

+
+		tlsConfigs, err := getTLSConfigurations(i, k8sClient)
+		if err != nil {
+			log.Errorf("Error configuring TLS for ingress %s/%s: %v", i.Namespace, i.Name, err)


Does this express the intended behavior as summarized by @dtomcej here?

I couldn't figure out, if an entry point is TLS-enabled at this point as the default configuration (passing nil) is handled further down the road and override entry points are only available as strings without any context. Therefore, I can't check if an entry point is TLS-enabled or not.

But I can't imagine a way to solve this problem properly as either the internals need to know about a TLS configuration error and abort in the case of an HTTPS-only entry point or the provider needs to know about the entry points (default and override).

I'm sorry for not pointing this out earlier.

Thanks for doing the research.

If we can't figure out whether it's safe to accept an illegal definition or not, then my opinion is that we should err on the side of safety and skip it. Better be safe than sorry.

What do you (and @dtomcej) think?

Staying on the safe side should be our first choice as this is security critical. I'd also add a note to the docs regarding this and also the deviation from the kubernetes spec (host names are ignored and therefore the wildcard behaviour cannot be implemented).

Could you refresh my mind again: what's the reason we cannot comply with the Kubernetes spec? Is there a potential path (possibly in the future) we could improve on this point?

In a kubernetes ingress resource multiple TLSes per ingress are allowed. Each consists of two parts: a list of host names and a reference to a kubernetes secret. There are two cases in which a TLS configuration can be:

A list of host names is provided: Only the defined host names in the list are used for this TLS certificate, not the ones defined in the certificate

The list of host names is empty (wildcard behavior): In this case not a general wildcard is used. Instead, only the host names in the ingress are used as the wildcard. A wildcard TLS may only be used without any other TLS definitions in this ingress.

I think there is a path to implement this feature in the future as this behavior is a special case of the already implemented one in the traefik internals. (https://github.com/containous/traefik/blob/c446c291d9dc49895e69c91e51a054778c71f21a/tls/certificate.go#L150-L154 would be the default behavior, but it can be overwritten by the provider)
I have no clue what so ever how to pass this information from a provider to the internals.

Honestly, after doing a bit of research, there does not seem to be any implementation outside of ingress controller annotations to handle the failures of any of the components of an ingress. It appears that it is all-or-nothing, where the tls section of the spec is parsed in parallel with the rules section of the spec, leaving the annotations to handle the relationship between http/rules and tls sections.

Therefore, I would now be more inclined to just fail/skip loading the entire ingress on an error loading any portion of the tls section of the spec, since there is no way (without going into all the middlewares) to know how important the separate tls sections are.

@gopenguin thanks for the clarification with regards to the wildcard matter. As long as we clearly document the constraints, I'm fine with it.

In the light of @dtomcej's latest comment: could you please change the logic so that we skip illegal Ingress definitions? AFAICS, that is the last point that keeps us from merging this PR.

timoreimann · 2017-12-21T10:22:47Z

provider/kubernetes/kubernetes.go

+		}
+		if len(missingEntries) > 0 {
+			return nil, fmt.Errorf("secret %s/%s is missing the following TLS data entries: %s", ingress.Namespace, t.SecretName, strings.Join(missingEntries, ", "))
+		}


I like this a lot 👍

dtomcej

Incredible work!

LGTM

timoreimann · 2018-01-06T20:33:33Z

provider/kubernetes/kubernetes.go

@@ -174,6 +175,13 @@ func (p *Provider) loadIngresses(k8sClient Client) (*types.Configuration, error)
 			continue
 		}

+		tlsConfigs, err := getTLSConfigurations(i, k8sClient)
+		if err != nil {
+			log.Errorf("Error configuring TLS for ingress %s/%s: %v", i.Namespace, i.Name, err)


Hmm, my IDE is telling me that this line is supposedly missing code coverage. Are we possibly missing a test to validate that we actually skip the Ingress if getTLSConfigurations returns an error?

Oh, you are right. This case isn't covered.

timoreimann

LGTM -- thanks a ton @gopenguin for this super useful PR. 🎉

timoreimann · 2018-01-07T21:58:50Z

@gopenguin our bot cannot proceed due to a merge conflict. Would you mind resolving it manually?

ldez · 2018-01-07T23:35:43Z

@timoreimann Not due to conflicts but to the missing milestone.

gopenguin · 2018-01-08T08:07:54Z

It was a pleasure to contribute to this awesome project. Thanks for your patience and till next time.

Add tls support for k8s ingress ressource

3892fb7

gopenguin requested a review from a team as a code owner November 20, 2017 19:56

traefiker added area/provider/k8s/ingress size/L status/0-needs-triage labels Nov 20, 2017

ldez added kind/enhancement a new or improved feature. status/2-needs-review and removed status/0-needs-triage labels Nov 20, 2017

ldez added the area/tls label Nov 20, 2017

Merge branch 'master' into k8s-ingress-tls

228362c

errm reviewed Nov 21, 2017

View reviewed changes

Add docs for the k8s tls ingress feature

765fba4

timoreimann suggested changes Nov 21, 2017

View reviewed changes

traefiker added the contributor/waiting-for-corrections label Nov 21, 2017

gopenguin added 2 commits November 21, 2017 14:24

Fix import prefix and errorhandling

1f96b88

Fix typo

bf648b8

timoreimann suggested changes Nov 21, 2017

View reviewed changes

Fix naming convention

4c0ba9e

timoreimann suggested changes Nov 22, 2017

View reviewed changes

timoreimann reviewed Nov 22, 2017

View reviewed changes

gopenguin added 2 commits November 22, 2017 10:51

Merge branch 'master' into k8s-ingress-tls

18c5f9e

Refactor k8s ingress tls extraction and add tests

cc0b448

Move the extraction of the tls cert definition into its own method to improve readability and testability. Add tests for error cases and wildcard behavior.

ldez removed the contributor/waiting-for-corrections label Nov 23, 2017

nmengin approved these changes Dec 21, 2017

View reviewed changes

timoreimann suggested changes Dec 21, 2017

View reviewed changes

traefiker added the contributor/waiting-for-corrections label Dec 21, 2017

gopenguin added 3 commits January 6, 2018 14:23

Abort ingress if error in TLS and add docs

4d156ec

Merge branch 'master' into k8s-ingress-tls

d00111a

Fix tests

6ab8f21

ldez removed the contributor/waiting-for-corrections label Jan 6, 2018

dtomcej approved these changes Jan 6, 2018

View reviewed changes

timoreimann suggested changes Jan 6, 2018

View reviewed changes

traefiker added the contributor/waiting-for-corrections label Jan 6, 2018

Add an invalid ingress to TestTLSSecretLoad to cover error case

1d2e121

ldez added bot/no-merge and removed contributor/waiting-for-corrections bot/no-merge labels Jan 6, 2018

timoreimann approved these changes Jan 7, 2018

View reviewed changes

timoreimann added status/3-needs-merge and removed status/2-needs-review labels Jan 7, 2018

traefiker added the bot/need-human-merge label Jan 7, 2018

ldez added this to the 1.6 milestone Jan 7, 2018

ldez removed the bot/need-human-merge label Jan 7, 2018

traefiker merged commit 8327dd0 into traefik:master Jan 7, 2018

traefiker removed the status/3-needs-merge label Jan 7, 2018

gopenguin mentioned this pull request Jan 8, 2018

Add tls support for k8s ingress resource #2438

Closed

gopenguin deleted the k8s-ingress-tls branch January 8, 2018 19:38

This was referenced Feb 21, 2018

kubernetes tls (SSL) support? #378

Closed

stable/traefik: support Traefik HTTPS SNI helm/charts#2637

Closed

Proposal: Native Kubernetes LetsEncrypt Implementation #2542

Open

Add support for fetching k8s Ingress TLS data from secrets #2439

Add support for fetching k8s Ingress TLS data from secrets #2439

Conversation

gopenguin commented Nov 20, 2017 • edited by ldez

What does this PR do?

Motivation

More

Additional Notes

ldez commented Nov 20, 2017

gopenguin commented Nov 20, 2017 • edited

ldez commented Nov 20, 2017

gopenguin commented Nov 21, 2017

gopenguin commented Nov 21, 2017

errm left a comment

Choose a reason for hiding this comment

gopenguin commented Nov 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timoreimann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gopenguin commented Nov 22, 2017

nmengin left a comment

Choose a reason for hiding this comment

timoreimann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dtomcej left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timoreimann left a comment

Choose a reason for hiding this comment

timoreimann commented Jan 7, 2018

ldez commented Jan 7, 2018

gopenguin commented Jan 8, 2018

gopenguin commented Nov 20, 2017 •

edited by ldez

gopenguin commented Nov 20, 2017 •

edited