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 cert-manager annotations when using Traefik IngressController #6

Open
alexellis opened this issue Jun 30, 2019 · 24 comments
Open
Labels
good first issue Good for newcomers help wanted Extra attention is needed size/m

Comments

@alexellis
Copy link
Member

alexellis commented Jun 30, 2019

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.

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

Context

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

@alexellis alexellis changed the title Add cert-manager support for Traefik Add cert-manager annotations when using Traefik IngressController Jun 30, 2019
@alexellis alexellis added help wanted Extra attention is needed good first issue Good for newcomers size/m labels Jun 30, 2019
@ajaegle
Copy link

ajaegle commented Jun 30, 2019

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

@alexellis
Copy link
Member Author

alexellis commented Jun 30, 2019

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

ajaegle commented Jun 30, 2019

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
Copy link
Member Author

Perfect. Let's consider this issue as yours 👍

@ajaegle
Copy link

ajaegle commented Jun 30, 2019

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

ajaegle commented Jun 30, 2019

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
Copy link
Member Author

alexellis commented Jul 1, 2019

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
Copy link
Member Author

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?

alexellis added a commit that referenced this issue Jul 1, 2019
Thanks to @ajaegle for noticing this. The Operator needs to be
tested in-cluster, since local testing with Go gives admin RBAC
by default.

Ref: #6

Signed-off-by: Alex Ellis <[email protected]>
@ajaegle
Copy link

ajaegle commented Jul 1, 2019

(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
Copy link
Member Author

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
Copy link
Member Author

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.

alexellis added a commit that referenced this issue Jul 1, 2019
Added docs for "openfaas_gateway_namespace" - thanks for
feedback @ajaegle. Re: #6

Signed-off-by: Alex Ellis <[email protected]>
@ajaegle
Copy link

ajaegle commented Jul 1, 2019

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
Copy link
Member Author

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 - Remove Certificate step from SSL page? docs#160

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

Alex

@alexellis
Copy link
Member Author

@ajaegle WDYT?

@ajaegle
Copy link

ajaegle commented Jul 1, 2019

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
Copy link
Member Author

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

ajaegle commented Jul 2, 2019

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
Copy link
Member Author

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

@ajaegle
Copy link

ajaegle commented Jul 5, 2019

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
Copy link
Member Author

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

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?

067ca8a

@alexellis
Copy link
Member Author

Thank you for looking at this

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

@alexellis
Copy link
Member Author

@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
Copy link
Member Author

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

0acd59e

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers help wanted Extra attention is needed size/m
Projects
None yet
Development

No branches or pull requests

3 participants