Add cert-manager annotations when using Traefik IngressController

alexellis commented 5 years ago

When using Traefik and the FunctionIngress object, we will get an Ingress record created, however I am not sure we will get functioning TLS with cert-manager. We do get that with Nginx presently, so this issue should be partly figuring out which additional annotations are needed and applying them.

Expected Behaviour

Add cert-manager should be supported with Traefik

Current Behaviour

Untested, it may be but I suspect it is not due to the additional annotations I found in a (French) blog post.

Currently only the annotations for cert-manager are added: https://github.com/openfaas-incubator/ingress-operator/blob/master/pkg/controller/controller.go#L538

Possible Solution

Edit the following code: https://github.com/openfaas-incubator/ingress-operator/blob/master/pkg/controller/controller.go#L514

Add these annotations:

    traefik.ingress.kubernetes.io/redirect-entry-point: https
    traefik.ingress.kubernetes.io/redirect-permanent: "true"
    ingress.kubernetes.io/ssl-redirect: "true"
    ingress.kubernetes.io/ssl-temporary-redirect: "false"

As see here: https://www.cerenit.fr/blog/kubernetes-ovh-traefik-cert-manager-secrets/

You can rebuild the controller with make and specify a TAG env-var, then tag this as your own username before pushing it. You can also run the cluster as a local Go binary.

How to test

K3s ships with Traefik running in "host mode", so this would be a very fast way of testing the fix. A VM on DO or a similar cloud will come with a public IP for the DNS certificate challenge.

You'll also need your own domain name.

Get a cloud VM with Ubuntu 18.x - i.e. Civo or DigitalOcean
Install k3s via https://k3s.io
Install OpenFaaS & the IngressOperator -> https://docs.openfaas.com/reference/ssl/kubernetes-with-cert-manager/#deploy-the-ingressoperator
Create a FunctionIngress with the Ingress type of traefik
Create a DNS A record for your domain and the IP of the node
See if you get a valid TLS cert

Context

This is an important piece of the puzzle for Traefik users.

ajaegle commented 5 years ago

I'll have a look at this. Still need to get familiar with OpenFaaS and operator development in general.

alexellis commented 5 years ago

Let's consider the issue still as "fair game" until you have your environment set up and ready? When do you think you'll be able to start setting up?

ajaegle commented 5 years ago

All right. I have openfaas running in a cluster with traefik and cert-manager and already have the gateway exposed using traefik with a cert-manager generated tls secret based on a manually deployed Certificate object. So http-01 challenge works as expected. Next up installing the ingress operator.

alexellis commented 5 years ago

Perfect. Let's consider this issue as yours :+1:

ajaegle commented 5 years ago

Thanks! Can you point me to some resources regarding namespacing? The ingress-operator has an env var function_namespace. I assumed this must match the openfaas-fn namespace where the functions are deployed. Is this only used to search for FunctionIngress objects which are assumed to be put to the main namespace (defaultopenfaas)?

Still struggling with some permissions as with my current setup the ingress-operator is not allowed to create Certificates (certificates.certmanager.k8s.io) which makes sense according to the RBAC rules in https://github.com/openfaas-incubator/ingress-operator/blob/master/artifacts/operator-rbac.yaml.

ajaegle commented 5 years ago

Extending the RBAC rules with (maybe too wide permissions) made the ingress-operator creating Certificate resources that are used by cert-manager.

- apiGroups: ["certmanager.k8s.io"]
  resources: ["certificates"]
  verbs: ["get", "list", "watch", "create", "update", "patch", "delete"]

I am a bit confused as I expected the ingress-operator to just create ingress resources that are labelled correctly which are then picked up by cert-manager watching those ingresses as described in https://docs.cert-manager.io/en/latest/tasks/issuing-certificates/ingress-shim.html.

I'm also observing two Certificate resources created. Presumably one by ingress-operator directly and the other one bycert-manager watching the ingress objects.

alexellis commented 5 years ago

Hi @ajaegle thanks for this testing and feedback 👑

Did you run through the instructions as provided in the docs? I haven't been issued two certificates in my testing with Nginx and someone else has been testing with Skipper.

Can you say what you're doing differently?

The namespaces I believe are documented already, to recap:

Ingress points at the gateway, the gateway lives in ns:openfaas, therefore ingress must be created in ns:openfaas. You should not need to be aware of namespaces or need to change anything other than to know the above.

Still struggling with some permissions as with my current setup the ingress-operator is not allowed to create Certificates (certificates.certmanager.k8s.io) which makes sense according to the RBAC rules

You are right. I think this is because I was testing certificates with go run as a local Go binary (which uses the admin role). Can you send a PR to add the certmanager RBAC rule?

alexellis commented 5 years ago

The approach we have taken in [1] the OpenFaaS docs has the user create each Certificate, Issuer and Ingress resource. This was written in collaboration with @LucasRoesler @stefanprodan and myself.

[1] https://docs.openfaas.com/reference/ssl/kubernetes-with-cert-manager/#10-ssl-for-the-gateway

I don't think we've observed the Certificate object being created automatically in the past, perhaps there are two ways to use cert-manager?

ajaegle commented 5 years ago

(not fully verified): The ingress-operator logged issues creating Certificate objects when I deployed it as written in the docs. When I added the additional permission, the error was gone. According to the OwnerReference one is caused by an Ingress and the other one by a FunctionIngress.

Cert-Manager has two ways of integration, explicitly by Certificate resources and also by scanning Ingresses for specific annotations. Should we first clarify which way should be supported or if there should be a switch for that? If the desired way was to create a Certificate directly, maybe the second one is triggered accidentally caused by the ingress.

From a dependency-perspective just using ingress-annotations instead of including the cert-manager library directly. WDYT?

alexellis commented 5 years ago

Did you change any code to get the two certificates?

The cert-manager version that is in the instructions is 0.7.0, are you using the same version?

kubectl logs deploy/ingress-operator -n openfaas
I0701 07:45:31.765321       1 main.go:47] Starting FunctionIngress controller version: latest-dev commit: 03b070497c84dbd8bd0fd0f45f440e470a037c6f
W0701 07:45:32.034364       1 client_config.go:548] Neither --kubeconfig nor --master was specified.  Using the inClusterConfig.  This might not work.
I0701 07:45:32.038330       1 controller.go:129] Setting up event handlers
I0701 07:45:32.038418       1 controller.go:186] Waiting for informer caches to sync
I0701 07:45:32.138603       1 controller.go:191] Starting workers
I0701 07:45:32.138630       1 controller.go:197] Started workers
alext:ingress-operator alex$ kubectl logs deploy/ingress-operator -n openfaas -f
I0701 07:45:31.765321       1 main.go:47] Starting FunctionIngress controller version: latest-dev commit: 03b070497c84dbd8bd0fd0f45f440e470a037c6f
W0701 07:45:32.034364       1 client_config.go:548] Neither --kubeconfig nor --master was specified.  Using the inClusterConfig.  This might not work.
I0701 07:45:32.038330       1 controller.go:129] Setting up event handlers
I0701 07:45:32.038418       1 controller.go:186] Waiting for informer caches to sync
I0701 07:45:32.138603       1 controller.go:191] Starting workers
I0701 07:45:32.138630       1 controller.go:197] Started workers
I0701 07:45:39.837276       1 controller.go:268] FunctionIngress name: nodeinfo-tls
I0701 07:45:39.837543       1 controller.go:279] function.Spec.UseTLS() true
I0701 07:45:39.843202       1 controller.go:291] createCert true
I0701 07:45:39.843231       1 controller.go:294] Need to create certifcate for FunctionIngress: nodeinfo-tls
I0701 07:45:39.843251       1 controller.go:300] Asked for TLS cert for nodeinfo-tls.myfaas.club via {letsencrypt-staging Issuer}
I0701 07:45:39.898192       1 controller.go:311] createIngress true

kubectl get ingress -n openfaas
NAME            HOSTS                      ADDRESS        PORTS     AGE
nodeinfo-tls    nodeinfo-tls.myfaas.club   167.71.8.149   80, 443   19m

I can confirm that I'm seeing Ingress create an additional certificate on its own

kubectl get certificate -n openfaas
NAME                       READY   SECRET                            AGE
nodeinfo-tls-certificate   True    nodeinfo-tls-certificate-secret   19m
nodeinfo-tls-secret        True    nodeinfo-tls-secret               19m

alexellis commented 5 years ago

It seems like whatever is placed in secretName is being used as the key / object name for the Certificate that Ingress/cert-manager generates:

    secretName: nodeinfo-tls-secret

nodeinfo-tls-certificate is the one that the operator created.

ajaegle commented 5 years ago

Exactly. Having the annotations on the ingress allows for immediate mode without creating a Certificate manually. So cert-manager picks up the ingress, creates a Certificate and links that to the secret name given in the ingress. Without the annotations, we still need the tls secret name to match the secret name provided in a manually (or automatically by openfaas ingress-controller) created Certificate. By using the way via a regular Ingress object, we could reduce the requirement of ingress-operator permissions for accessing cert-manager resources.

This could also be applied for exposing the openfaas gateway.

alexellis commented 5 years ago

Thank you 👍

Can we proceed as follows:

Set "automatic TLS" ingress annotations
Set any additional TLS annotations required for Traefik
Remove cert-manager code and vendor dependencies
Remove certificate from OpenFaaS docs for the gateway - this must tested e2e before I merge - https://github.com/openfaas/docs/issues/160

Anything else you would suggest? I'll look out for the PRs if you'd like to take this?

Alex

alexellis commented 5 years ago

@ajaegle WDYT?

ajaegle commented 5 years ago

Without having looked deeper into the current implementation, your proposal sounds like a valid way to go.

One more thing we should think about (even not directly affected by that change): The current solution of having one FunctionIngress == one Domain == one Ingress == one Certificate works if people only deploy a few (not hundreds) of such FunctionIngresses due to Letsencrypt limitations. From my experience working with cert-manager and several ingresses, it is often easier to deploy one wildcard Certificate resource manually specifying a wildcard secretName and then referencing that same secret from many ingresses. Maybe this should also be an option for the FunctionIngress to provide a tlsSecretNameRef as alternative to specifying the issuerRef for automated certificate creation?

alexellis commented 5 years ago

I'm not sure that wildcard certificate should be in scope for the initial release. They rely on DNS01 challenges which are much more complex for DNS beginners and rely on API keys and secrets. We use this with OpenFaaS Cloud and people struggle.

Unless you mean something else?

ajaegle commented 5 years ago

Setting up letsencrypt with DNS challenges is a bit more complex, but not that hard. But It's not only about cert-manager integration. If you have some hand-crafted wildcard cert (many enterprises do so), you should also be able to use that. And the way of doing so is by referencing a secret in the namespace, no matter how this got there.

I'm with you that it may be better to not expose all possible ways in the ingress-operator docs to not overwhelm newcomers but still I think feature is relevant and could be added as a side note like "managing your certs without letsencrypt? We got you covered, too". This can be a reference to the kubernetes docs.

alexellis commented 5 years ago

@ajaegle do you want to run with this, or would it be better if I took the work?

ajaegle commented 5 years ago

Sorry for my late reply. I'd like to take this, if I can take some time to get into it. If you want to get it done quickly, please do so.

alexellis commented 5 years ago

Can you give me more of an idea on timing? It'll be almost a week ago that we opened it. I don't mind if you do several smaller PRs starting with removing the cert-manager code.

MarcusNoble commented 5 years ago

I think this is what's needed to cover this issue but not 100% sure as I don't use cert-manager. Either of you able to take a look before I open a PR for it?

https://github.com/openfaas-incubator/ingress-operator/commit/067ca8ac9bdfe85e36ec76f968b67a65a8ed6fd7

alexellis commented 5 years ago

Thank you for looking at this

Please could you just go ahead and open the pr anyway.

alexellis commented 5 years ago

@MarcusNoble @ajaegle I've gone ahead and done the patch to remove cert-manager, please can you test with the latest YAML files and tag 0.3.0?

alexellis commented 5 years ago

@MarcusNoble your patch was close, we just needed to also re-vendor by removing cert-manager from Gopkg.toml and running dep ensure.

https://github.com/openfaas-incubator/ingress-operator/commit/0acd59ed916d5825b32f33d698280f7175ad1007

openfaas / ingress-operator