Use default cert-manager annotations to manage certificates

[jonatasbaldin]

Expected Behaviour

Instead of using the custom spec.tls.* definition to generate certificates we can use the default cert-manager annotations (certmanager.k8s.io/issuer or certmanager.k8s.io/cluster-issuer), since #7 was implemented.

Current Behaviour

We need to use a custom spec.tls.* definition to enable the certificate creation.

Even though we are copying all the annotations to the Ingress, the current implementation of the controller doesn't support the default cert-manager ones.

Possible Solution

Remove the special case for TLS annotations inside makeAnnotations and change the makeTLS functions to look into the annotations instead of function.Spec.UseTLS().

I didn't try these changes, just an idea by taking a quick look into the code. It may be more complicated than that.

Context

Using the same Ingress behaviour to create TLS certificates in the FunctionIngress would require a lower learning curve, less docs and make configurations more standard across the cluster.

[alexellis]

I'm pretty sure that we are already using ingress shim. I know that because I'm not creating any Certificate objects in the controller anymore?

[alexellis]

I prefer the TLS being in the Spec, this is to abstract away the gritty details from users

[jonatasbaldin]

I proposed this because I think that the way it works today has some drawbacks:

• Custom flag to enable TLS. We could strive to have the same behaviour as an Ingress to reduce friction and to rely on annotations already known by the community.

For me, having this YAML:

apiVersion: openfaas.com/v1alpha2
kind: FunctionIngress
metadata:
  name: nodeinfo
  namespace: openfaas
  annotations:
    certmanager.k8s.io/issuer: "letsencrypt-staging"
spec:
  domain: "nodeinfo.deployeveryday.com"
  function: "nodeinfo"
  ingressType: "nginx"

Looks better than this:

apiVersion: openfaas.com/v1alpha2
kind: FunctionIngress
metadata:
  name: nodeinfo
  namespace: openfaas
spec:
  domain: "nodeinfo.deployeveryday.com"
  function: "nodeinfo"
  ingressType: "nginx"
  tls:
    enabled: true
    issuerRef:
      name: "letsencrypt-staging"
      kind: "Issuer"

• We are forcing the http01 challenge, as defined here. Actually, this is a deprecated annotation. If cert-manager implements a new annotation in the future, we would need to implement in this controller.

All of this is my personal taste... What would be nice is get some more users involved in this discussion and see what they think about 🤔 What do you think about this?

Cheers 🚀

[makkus]

Agree with @jonatasbaldin , I use a wildcard certificate for my ingresses, and it seems that is not directly supported by this operator.

I might be wrong though, or not the target audience for this operator? Is it recommended to create my own (non-Function) ingress resources for my use-case?

[alexellis]

@makkus as you noted, support for the HTTP01 challenge is already available, if you need the DNS01 challenge then feel free to open an issue to propose it.

[makkus]

@alexellis thanks. I don't really need the DNS01 challenge, as I'm re-using the same wild-card certificate for every one of my ingresses. It kinda works for me at the moment, because I configured nginx-ingress to use the wild-card cert secret as the default. I can't access my logs at the moment, but if I remember right then it tries to get the secret that this operator should have configured, but couldn't, and then falls back to the default certificate. I guess I'd just prefer if I could configure this Ingress more or less the same way as a 'normal' one, except that I only need to specify the function name instead of the service details as usual.

But at the end of the day it's not a big problem, I can always create the ingress manual, and not use this operator. Haven't tried it yet, but I don't assume this does anything super-special, right?

[alexellis]

@makkus what would it look like if we implemented what you needed?

I need a bit more info from you please, because I don't seem to be seeing the full picture.

Thanks 👍

[makkus]

Sure, I hope I remember the details right, the setup I used 2 weeks ago is gone and I'll re-build it again next week or so...

Basically, (the tls-specific parts of) my 'normal' ingresses look like this:

 metadata:
   name: graphql-ingress
   annotations:
     kubernetes.io/ingress.class: "nginx"    
 spec:
   tls:
   - hosts:
     - graphql.intern.example.com
   rules:
   - host: graphql.intern.example.com
      ...
      ...

This is an internal network that can't be reached from the internet, but because I'm using the dns challenge and the dns server pointing to the internal address I'm still getting a valid LetsEncrypt certificate for this service.
Because I have a wildcard certificate for *.intern.example.com I don't have to configure anything else.
My nginx ingress is configured to use that wildcard cert as default-ssl-certificate:

       containers:
         - name: nginx-ingress-controller
           image: quay.io/kubernetes-ingress-controller/nginx-ingress-controller:0.26.1
           args:
             - /nginx-ingress-controller
             - --configmap=$(POD_NAMESPACE)/nginx-configuration
             - --tcp-services-configmap=$(POD_NAMESPACE)/tcp-services
             - --udp-services-configmap=$(POD_NAMESPACE)/udp-services
             - --publish-service=$(POD_NAMESPACE)/ikh-mgmt
             - --annotations-prefix=nginx.ingress.kubernetes.io
             - --default-ssl-certificate=kube-system/wildcard-cert-tls

As I mentioned before, this kind of works with a 'FunctionIngress' like this one (note that I don't have a 'issuerRef' key at all):

 spec:
   domain: "myfunc.intern.example.com"
   function: "myfunc"
   ingressType: "nginx"
   tls:
     enabled: true

But only because nginx is configured to use my wildcard cert as default. So I had to use the same subdomain (.intern.example.com) for my functions, another one like '.func.intern.example.com) didn't work, because the wildcard certificate wasn't issued for that.
So, for my use-case (and some others I had in mind but can't recall the exact details of at the moment -- I would need some extra functionality provided by normal Ingresses, not just the option to use dns challenges), I'd have preferred if the FunctionIngress would let me specify my tls config the same as a 'normal' Ingress, and just hand it down the line. Personally, I'm not too concerned about the gritty details of the tls config (if I loose flexibility, that is, otherwise I'm all for less config), since I have to do that anyway. But I like the fact that I can directly specify the function without having to map the service details in the FunctionIngress resource type. Does that make sense? Sorry if it's still unclear.

In the end, as I mentioned, I might just not be a good target audience for the FunctionIngress and it's probably not worth changing anything. I'm happy enough to just write my own Ingress resources for my functions if the 'default-wildcard-cert' thing I have at the moment doesn't work for some future use-case. But, of course, I'd still prefer if I didn't have to :-)

[goncalo-oliveira]

I agree that the annotations look simpler and more compliant. I can also understand @alexellis point of view, though as a personal preference I would rather add a one line annotation than the custom tls section. Maybe a compromise would be to allow both? :)

[alexellis]

As far as I'm aware both are permissable. What happened when you tried?

[goncalo-oliveira]

apiVersion: openfaas.com/v1alpha2
kind: FunctionIngress
metadata:
  name: cows
  namespace: openfaas
  annotations:
    cert-manager.io/issuer: letsencrypt-staging
spec:
  domain: "faas.mydomain.com"
  function: "cows"
  ingressType: "nginx"
  path: "/cows"

Without the TLS section, the certificate isn't created.