Skip to content
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 fetching k8s Ingress TLS data from secrets #2439

Merged
merged 22 commits into from
Jan 7, 2018
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
3892fb7
Add tls support for k8s ingress ressource
gopenguin Nov 20, 2017
228362c
Merge branch 'master' into k8s-ingress-tls
gopenguin Nov 21, 2017
765fba4
Add docs for the k8s tls ingress feature
gopenguin Nov 21, 2017
1f96b88
Fix import prefix and errorhandling
gopenguin Nov 21, 2017
bf648b8
Fix typo
gopenguin Nov 21, 2017
4c0ba9e
Fix naming convention
gopenguin Nov 21, 2017
18c5f9e
Merge branch 'master' into k8s-ingress-tls
gopenguin Nov 22, 2017
cc0b448
Refactor k8s ingress tls extraction and add tests
gopenguin Nov 23, 2017
1c60f29
Merge branch 'master' into k8s-ingress-tls
gopenguin Nov 23, 2017
b0a7899
Remove cert validation and fix some other issues
gopenguin Dec 4, 2017
741163e
Merge branch 'master' into k8s-ingress-tls
gopenguin Dec 5, 2017
c13b21a
Implement the builder pattern for TLS types
gopenguin Dec 5, 2017
bce97e9
Improve error messages and doc
gopenguin Dec 6, 2017
383b9e8
Pass annotation 'traefik.frontend.entryPoints' to cert
gopenguin Dec 6, 2017
74616a4
Merge branch 'master' into k8s-ingress-tls
gopenguin Dec 11, 2017
e082818
Add tls config to k8s template and extend test
gopenguin Dec 11, 2017
3ecf770
Fix template file
nmengin Dec 19, 2017
d0a5c2f
Update documentation
gopenguin Dec 20, 2017
4d156ec
Abort ingress if error in TLS and add docs
gopenguin Jan 6, 2018
d00111a
Merge branch 'master' into k8s-ingress-tls
gopenguin Jan 6, 2018
6ab8f21
Fix tests
gopenguin Jan 6, 2018
1d2e121
Add an invalid ingress to TestTLSSecretLoad to cover error case
gopenguin Jan 6, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions autogen/gentemplates/gen.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

39 changes: 39 additions & 0 deletions docs/user-guide/kubernetes.md
Original file line number Diff line number Diff line change
Expand Up @@ -330,6 +330,45 @@ echo "$(minikube ip) traefik-ui.minikube" | sudo tee -a /etc/hosts

We should now be able to visit [traefik-ui.minikube](http://traefik-ui.minikube) in the browser and view the Træfik Web UI.

### Add a TLS Certificate to the Ingress

!!! 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/).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please uppercase tls to TLS here and in every other comment (also the ones in the code).


To setup a https protected ingress, you can leverage the tls feature of the ingress resource.

```yaml
apiVersion: extensions/v1beta1
kind: Ingress
metadata:
name: traefik-web-ui
namespace: kube-system
annotations:
kubernetes.io/ingress.class: traefik
spec:
rules:
- host: traefik-ui.minikube
http:
paths:
- backend:
serviceName: traefik-web-ui
servicePort: 80
tls:
- hosts:
Copy link
Contributor

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 us parsing the hosts list in our code explicitly. Am I missing something?

- traefik-ui.minikube
secretName: traefik-ui-tls-cert
```

In addition to the modified ingress you need to provide the tls certificate via a kubernetes secret in the same namespace as the ingress. The following two commands will generate a new certificate and create a secret containing the key and cert files.

```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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

```

!!! note
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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent extension to the guide 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had no idea how to do something like this but I'm glad it is OK


Copy link
Contributor

@nmengin nmengin Dec 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please add a note to explain that the certificates contained into the K8s secrets will be added to all the TLS entryPoints linked to the services which use them (if the label traefik.frontend.entryPoints is defined) or to all the TLS entryPoints defined into DefaultEntryPoints by default ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would the following ok with you?

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.

Did you mean with 'note' the note formatting or just an addition to the documentation?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gopenguin

Yes the sentence LGTM 👍
I mean note formatting at the end of the block instructions 😉

## Basic Authentication

It's possible to add additional authentication annotations in the Ingress rule.
Expand Down
56 changes: 54 additions & 2 deletions provider/kubernetes/kubernetes.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import (
"github.com/containous/traefik/log"
"github.com/containous/traefik/provider"
"github.com/containous/traefik/safe"
"github.com/containous/traefik/tls"
"github.com/containous/traefik/types"
"k8s.io/client-go/pkg/api/v1"
"k8s.io/client-go/pkg/apis/extensions/v1beta1"
Expand Down Expand Up @@ -140,8 +141,9 @@ func (p *Provider) loadIngresses(k8sClient Client) (*types.Configuration, error)
ingresses := k8sClient.GetIngresses()

templateObjects := types.Configuration{
Backends: map[string]*types.Backend{},
Frontends: map[string]*types.Frontend{},
Backends: map[string]*types.Backend{},
Frontends: map[string]*types.Frontend{},
TLSConfiguration: []*tls.Configuration{},
}
for _, i := range ingresses {
ingressClass := i.Annotations[annotationKubernetesIngressClass]
Expand Down Expand Up @@ -308,6 +310,56 @@ func (p *Provider) loadIngresses(k8sClient Client) (*types.Configuration, error)
}
}
}

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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

continue
}

tlsCrtData, tlsCrtExists := tlsSecret.Data["tls.crt"]
tlsKeyData, tlsKeyExists := tlsSecret.Data["tls.key"]
if !tlsCrtExists || !tlsKeyExists {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we split this up into two dedicated errors?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Never mind

if !tlsCrtExists {
log.Warnf("Secret %s/%s doesn't have an entry named 'tls.crt'", i.Namespace, t.SecretName)
}
if !tlsKeyExists {
log.Warnf("Secret %s/%s doesn't have an entry named 'tls.key'", i.Namespace, t.SecretName)
}
continue
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


var hosts []string
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: ot instead of to.

// tls config only matches rules in this ingress, add all the used hosts to simulate a wildcard like
// behavior.
hosts = []string{}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).


for _, r := range i.Spec.Rules {
if r.Host != "" {
hosts = append(hosts, r.Host)
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


tlsConfig := &tls.Configuration{
EntryPoints: hosts,
Certificate: &tls.Certificate{
CertFile: tls.FileOrContent(tlsCrtData),
KeyFile: tls.FileOrContent(tlsKeyData),
},
}

templateObjects.TLSConfiguration = append(templateObjects.TLSConfiguration, tlsConfig)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we put the TLS logic into a dedicated function?

}
return &templateObjects, nil
}
Expand Down