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

cert-manager does not use ingress.class from Ingress annotated with cert-manager.io/cluster-issuer #2538

Open
whut opened this issue Jan 22, 2020 · 47 comments
Labels
area/api Indicates a PR directly modifies the 'pkg/apis' directory help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/feature Categorizes issue or PR as related to a new feature. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. priority/backlog Higher priority than priority/awaiting-more-evidence.

Comments

@whut
Copy link

whut commented Jan 22, 2020

Describe the bug:

When using the cert-manager.io/cluster-issuer annotation, for example like this:

apiVersion: extensions/v1beta1
kind: Ingress
metadata:
  name: myexample
  annotations:
    kubernetes.io/ingress.class: mycustomingressclass
    cert-manager.io/cluster-issuer: letsencrypt-prod
spec:
  tls:
    - hosts:
        - myexample.example.com
      secretName: myexample
  rules:
    - host: myexample.example.com
      http:
        paths:
          - backend:
              serviceName: myexampleservice
              servicePort: http

And with ClusterIssuer like this:

apiVersion: cert-manager.io/v1alpha2
kind: ClusterIssuer
metadata:
  name: letsencrypt-prod
spec:
  acme:
    server: https://acme-v02.api.letsencrypt.org/directory
    email: myemail@example.com
    privateKeySecretRef:
      name: letsencrypt-prod
    solvers:
      - http01:
          ingress: {}

The Ingress generated to handle the http01 challenge have no kubernetes.io/ingress.class annotation set.

Alternatively, if the ClusterIssuer would have ingress class set, for example to someotherclass, the generated Ingress would have the same class set, in this case to someotherclass.

Expected behaviour:

The Ingress generated to handle the http01 challenge should be annotated with the same ingress class as the Ingress annotated with cert-manager.io/cluster-issuer. In above case this should be mycustomingressclass.

As a workaround, the acme.cert-manager.io/http01-ingress-class attribute can be set on Ingress, but this just duplicates value of kubernetes.io/ingress.class:

apiVersion: extensions/v1beta1
kind: Ingress
metadata:
  name: myexample
  annotations:
    kubernetes.io/ingress.class: mycustomingressclass
    cert-manager.io/cluster-issuer: letsencrypt-prod
    # Note that below line sets the same ingress class like in above kubernetes.io/ingress.class
    acme.cert-manager.io/http01-ingress-class: mycustomingressclass
spec:
  tls:
    - hosts:
        - myexample.example.com
      secretName: myexample
  rules:
    - host: myexample.example.com
      http:
        paths:
          - backend:
              serviceName: myexampleservice
              servicePort: http

Anything else we need to know?:

I understand why ClusterIssuer have ability to set ingress class. This is required when manually creating Certificate resources. But in case of using the cert-manager.io/cluster-issuer annotation it works in surprising way as just adding cert-manager.io/cluster-issuer annotation does not work if using multiple ingress classes. One must also add acme.cert-manager.io/http01-ingress-class with just use the same value as kubernetes.io/ingress.class. I believe there should be option on ClusterIssuer/Issuer to allow for using ingress class from annotated ingress instead of the one set up on ClusterIssuer/Issuer. I would even consider enabling it by default, to avoid the surprise I had:)

/kind bug

@jetstack-bot jetstack-bot added the kind/bug Categorizes issue or PR as related to a bug. label Jan 22, 2020
@sazzle2611
Copy link

Is this likely to be fixed anytime soon because I really want to upgrade (still on version 0.9) but we have 4 or 5 different ingress classes per cluster and it seems messy to have to add a duplicate annotation for the ingress class to every ingress. The other alternative, which is to create a separate cluster issuer for each ingress class, also doesn't feel right.

Please can you give me an idea of whether this is likely to be fixed anytime soon, if it's not then I'll probably go ahead with the upgrade as soon as possible but if there's any chance it will be fixed in the next couple of months then I will wait as it would save a lot of extra work.

Thanks

@sagikazarmark
Copy link

Might be a duplicate of #2314

@munnerz
Copy link
Member

munnerz commented Apr 24, 2020

This isn't something we're planning on changing imminently, but it does make sense to make it easier to have this sort of behaviour configured.

I think to do so, we'll need to adjust the way the HTTP01 solver on the issuer resource is configured to allow for a user to choose to opt-in to this behaviour or not. Perhaps using some alternative to class and name.

If you've got any thoughts on how this can look, it'd be good to hear 😄

In the meantime, I'd advise a) upgrading and b) adding the second annotation (or otherwise configuring your ClusterIssuer with something like a dnsZones selector for each ingress class/domain combination you need).

/priority backlog
/kind feature
/remove-kind bug
/area api

@jetstack-bot jetstack-bot added priority/backlog Higher priority than priority/awaiting-more-evidence. kind/feature Categorizes issue or PR as related to a new feature. area/api Indicates a PR directly modifies the 'pkg/apis' directory and removed kind/bug Categorizes issue or PR as related to a bug. labels Apr 24, 2020
@sagikazarmark
Copy link

As I mentioned in #2314 (comment) I either consider this a bug in cert-manager itself or in the documentation.

From https://cert-manager.io/docs/usage/ingress/#supported-annotations

If not specified (edit: acme.cert-manager.io/http01-ingress-class annotation) and the acme-http01-edit-in-place annotation is not set, this defaults to the ingress class of the ingress resource.

The defaults to the ingress class of the ingress resource is clearly not true, and in my opinion it would be the logical thing to do, so for me, this is a bug, not a feature.

It's worth keeping in mind that ingress class will become a part of the ingress spec soon.

@discostur
Copy link

discostur commented Jun 19, 2020

@sagikazarmark is completely right!

The documentation says if no default ingress class is set it should stick to the ingress class (annotation) of the resource.

I have configured a cluster issuer:

apiVersion: cert-manager.io/v1alpha2
kind: ClusterIssuer
...
    - selector: {}
      http01:
        ingress: {}

And i have two ingress classes:

  • nginx (for internal use / private IPs only)
  • nginx-public (for external use / public IPs)

If i create a new ingress ressource with the annotation

kind: Ingress
apiVersion: extensions/v1beta1
...
  annotations:
    kubernetes.io/ingress.class: nginx-public

the acme-solver ingress still uses the default "nginx" ingress and the challenge could not be resolved!
In older releases this definitely worked for me!

Cert-Manager: v0.15.1
k8s: 1.15.10

@discostur
Copy link

@munnerz any news on this?
Documentation is still wrong - even in the latest 0.16 release:

acme.cert-manager.io/http01-ingress-class: this annotation allows you to configure the ingress class that will be used to solve challenges for this ingress. Customizing this is useful when you are trying to secure internal services, and need to solve challenges using a different ingress class to that of the ingress. If not specified and the acme-http01-edit-in-place annotation is not set, this defaults to the ingress class of the ingress resource.

@sagikazarmark
Copy link

I feel this is going to become invalid anyway, since the ingress class will become a spec field in newer ingress versions.

@discostur
Copy link

discostur commented Jul 29, 2020

@sagikazarmark sure with k8s v1.22 the extensions/v1beta1 gets deprecated and we must use networking.k8s.io/v1beta1. But will it become invalid then? I thought you just replace the kubernetes.io/ingress.class annotation with ingressClassName spec filed - but the problem still remains?

@sagikazarmark
Copy link

I guess you are right, in a way the problem will still be there, but in a different form.

@michelefa1988
Copy link

Guys, whats the conclusion of this? Is it a bug which needs to be fixed or are threr any workarounds?

My goal is simple. I have 2x ingresses, NGINX and INTERNAL. When I am using internal ingress, I want to be able to resolve the ACME challage using the NGINX ingress controller.

Please advise,

@sagikazarmark
Copy link

@michelefa1988 you can manually set the ingress class to be used by Cert Manager:

    # See https://github.com/jetstack/cert-manager/issues/2314
    acme.cert-manager.io/http01-ingress-class: "nginx"

You are actually not affected by this (in my opinion) bug, because you actually want to use a different ingress class anyway. This issue is about cert manager not falling back to the ingress class of the ingress when the issuer does not define an ingress class itself (or there is no annotation).

@michelefa1988
Copy link

@sagikazarmark , Do you know if this is a known issue I have bumped into or are there any work arounds? I have been adding the below, to my internal application ingress but the challenge is never created on internal services (works fine on public nginxingress)

acme.cert-manager.io/http01-ingress-class: nginx
acme.cert-manager.io/http01-edit-in-place: "true"

@sagikazarmark
Copy link

I think that's a different issue. This one is about the right fallback which is documented, but does not actually work.

@michelefa1988
Copy link

@SaaldjorMike ,If you think it doesn't work then ill lay it to rest for now 👍

Thank you!!

@SaaldjorMike
Copy link
Contributor

@SaaldjorMike ,If you think it doesn't work then ill lay it to rest for now 👍

@michelefa1988 I think you highlighted the wrong person 😄

@whereisaaron
Copy link
Contributor

whereisaaron commented Sep 30, 2020

@munnerz I believe this behavior is a regression, We have used cert-manager with custom classes forever and the ingress class for challenges use to behave as the documentation said (as @sagikazarmark highlighted). If you set in ingress.class on the Ingress then the challenge Ingress would use the same class. Then, after upgrading a few versions to 0.16, this suddenly stopped working and challenge Ingress's suddenly had no ingress.class even if one was set on the original Ingress.

We were able to work around the regression bug by adding an explicit acme.cert-manager.io/http01-ingress-class, setting it to be the same value ingress.class. But we never had to do that before upgrading to 0.16.

I am not sure where the regression occurred, but I have clusters running v0.8.x that still behave as the documentation describes, but everything else that v0.16 now behaves as these various issues report, failing to default to the Ingress ingress.class. Update: Looking at #2314 it looks like the regression occurred between 0.8 and 0.11.

It really makes no sense that a challenge for an Ingress with an explicit ingress.class set would not use the same class. Why would HTTP01 challenges - by default - attempt to use a different ingress from the Ingress it is for?

@whereisaaron
Copy link
Contributor

Further to this, I have found that adding the annotation acme.cert-manager.io/http01-ingress-class to the Ingress does not work if there is an existing order or cert, it will keep creating challenge ingresses with no kubernetes.io/ingress.class annotation at all. Sometimes deleting the pending order is enough, but sometimes we have found we have to delete the certificate resource so that it gets recreated, in order to flush the bad behavior and pick up the duplicate class annotation.

The documentation pages still documents this the way it is supposed to work, and the way it used to work, before the regression. That is, challenge Ingresses should default to the same class as the Ingress they are for.

@munnerz you've marked this a feature, not a bug, and said "This isn't something we're planning on changing imminently", but it already has been changed, by some bug along the way 😄 You can verify this by comparing the current behavior to older versions or the documentation.

image

@fliphess
Copy link

fliphess commented Dec 8, 2020

Any progress on this issue?

@goddamit-io
Copy link

goddamit-io commented Dec 29, 2020

Actually I'd like to come back to what @discostur mentioned. There are a lot of issues out there with engineers using networking.k8s.io/v1beta1 on >NGINX-controller 1.9.0, however the described behaviour from above seems to have changed.

I have a public and a private ingress controller and tried to challenge ACME obviously over the public one. But it always keeps choosing the private one by the kubernetes.io/ingress.class annotation although this annotation is deprecated and not in use by my system, as it shouldn't anymore by best-practice. It still seems to register to the private ingress controller even after deleting it and also it keeps setting the same annotation to ACME ingresses. This is crazy... I will consider a fallback.

Possible solution: https://web.archive.org/web/20200911002539/http://ukhomeoffice.github.io/application-container-platform/how-to-docs/cert-manager-upgrade-from-v0.8.html

Cheers guys

@lboix
Copy link

lboix commented Feb 3, 2021

My team and I are facing this exact same situation : we are migrating our cluster from Kubernetes 1.14 to 1.18 and wanted to upgrade to the last cert-manager version too (from 0.9.1 to 1.1.0)

Unfortunately without adding an extra annotation to all our existing Ingress objects, or hardcoding the class name of one of our Ingress Controllers in the ClusterIssuer specs, it's impossible to generate a certificate : the issued Ingress cm-acme-http-solver is never attached to one of our existing Ingress Controllers and the challenge is never achieved.

We reverted in order to use old cert-manager 0.9.1 version and hopefully the certificate generation is still working in our new Kubernetes 1.18 cluster.

I strongly agree with all the remarks made in this ticket : it will be a good thing that a next cert-manager version brings this behaviour back (especially because the current documentation does send a big mixed signal about it, letting us thinking that's it's still here : https://cert-manager.io/docs/configuration/acme/http01/#class)

To sum up please considering offering in a next cert-manager version with this ClusterIssuer setup:

spec:
  acme:
      - http01:
          ingress: {}

The same behaviour as in the old spec (aka the issued Ingress cm-acme-http-solver have the kubernetes.io/ingress.class annotation with the same value of the source Ingress we have trying to generate a certificate for):

spec:
  acme:
    http01: {}

Thank you for your time, and all the hard work done with this project!

@goddamit-io
Copy link

@lboix quick: I had to move to Cloudflare due to this issue... http01 challenge seems to be broken or I did not achieve implementing it on networking.k8s.io/v1beta1 for NGINX ingress controller v1.9.1...

If that somehow helps you out in any way I am glad to share you my experience.

Cheers mate

@maelvls
Copy link
Member

maelvls commented Sep 15, 2021

The documentation says if no default ingress class is set it should stick to the ingress class (annotation) of the resource.

The HTTP01 page on the cert-manager website is unclear, I'm not sure what "ingress class" means in the following context:

In the Ingress object, if the field class is not specified, cert-manager will default to create new Ingress resources but will not set the ingress class on these resources.

The words "set the ingress class" could mean two different things:

  • the spec.ingressClassName field on the solver Ingress,
  • or the annotation kubernetes.io/ingress.class on the solver Ingress.

I'll assume that it means the annotation kubernetes.io/ingress.class, not the ingressClassName.

Digging into the past, I noticed in the 0.8 documentation that in certmanager.k8s.io/v1alpha1, the Certificate resource had a field for the ingressName:

apiVersion: certmanager.k8s.io/v1alpha1
kind: Certificate
spec:
  acme:
    config:
    - http01:
        ingressClass: nginx

But in cert-manager.io/v1alpha2, the ingressClass disappears.

With certmanager.k8s.io/v1alpha1, the annotation kubernetes.io/ingress.class would flow from the Ingress to the Certificate (in setIssuerSpecificConfig) and from the Certificate to the temporary Ingress (in buildIngressResource):

kind: Ingress
metadata:
  annotations:
    kubernetes.io/ingress.class: nginx

      |
      v

kind: Certificate
apiVersion: certmanager.k8s.io/v1alpha1
spec:
  acme:
    config:
    - http01:
        ingressClass: nginx

      |
      v

kind: Ingress
metadata:
  annotations:
    kubernetes.io/ingress.class: nginx

With cert-manager.io/v1alpha2, the ingress class does not get passed to the Certificate, and the only way to get around that is to set an annotation introduced in v0.11.0:

kind: Ingress
metadata:
  annotations:
    acme.cert-manager.io/http01-ingress-class: nginx

      |
      v

kind: Certificate
apiVersion: certmanager.k8s.io/v1alpha1
metadata:
  annotations:
    acme.cert-manager.io/http01-override-ingress-class: nginx

      |
      v

kind: Ingress
metadata:
  annotations:
    kubernetes.io/ingress.class: nginx

I don't think this is a regression; rather, a feature that was present in certmanager.k8s.io/v1alpha1 was not carried over to cert-manager.io/v1alpha2. I agree with @munnerz that this is a feature request.

Jancis added a commit to wunderio/drupal-project-k8s that referenced this issue Sep 20, 2021
@whut
Copy link
Author

whut commented Aug 7, 2022

/remove-lifecycle rotten

@jetstack-bot jetstack-bot removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Aug 7, 2022
@Xplouder
Copy link

Xplouder commented Aug 19, 2022

How about create a flag in cert manager controller to enable the use of ingressClassName instead the annotation (the behavior we had in 1.6.1) so we can have both?
As already said since the annotation is deprecated by kubernetnes so we should eventually migrate to ingressClassName as default but having a way to have backward compatibility too would be great.

@khumps
Copy link

khumps commented Sep 27, 2022

Is there any progress on this? I am stumbling upon this 2 year old abandoned issue on what appears to be a completely necessary fix. This completely breaks the ability to use HTTP01 on top of nginx if it only surfaces specific ingress classes. My only solution is to rely on DNS01 which thankfully wasn't a huge issue since I use Cloudflare

@jetstack-bot
Copy link
Collaborator

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
If this issue is safe to close now please do so with /close.
Send feedback to jetstack.
/lifecycle stale

@jetstack-bot jetstack-bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 26, 2022
@jetstack-bot
Copy link
Collaborator

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.
If this issue is safe to close now please do so with /close.
Send feedback to jetstack.
/lifecycle rotten
/remove-lifecycle stale

@jetstack-bot jetstack-bot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Jan 25, 2023
@Xplouder
Copy link

/remove-lifecycle rotten

@jetstack-bot jetstack-bot removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Jan 25, 2023
@jetstack-bot
Copy link
Collaborator

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
If this issue is safe to close now please do so with /close.
Send feedback to jetstack.
/lifecycle stale

@jetstack-bot jetstack-bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 25, 2023
@Nuru
Copy link

Nuru commented Apr 26, 2023

/remove-lifecycle stale

@jetstack-bot jetstack-bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 26, 2023
@jetstack-bot
Copy link
Collaborator

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
If this issue is safe to close now please do so with /close.
Send feedback to jetstack.
/lifecycle stale

@jetstack-bot jetstack-bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 25, 2023
@Xplouder
Copy link

/remove-lifecycle stale

@jetstack-bot jetstack-bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 25, 2023
@jetstack-bot
Copy link
Collaborator

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
If this issue is safe to close now please do so with /close.
Send feedback to jetstack.
/lifecycle stale

@jetstack-bot jetstack-bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Oct 24, 2023
@maelvls maelvls added the help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. label Nov 6, 2023
@maelvls
Copy link
Member

maelvls commented Nov 6, 2023

As part of today's triage party, we have looked at this issue again. It seems like this is still a feature request even with the Ingress v1 API (i.e., with ingressClassName). We need someone willing to try to work on this for this issue to progress.

@maelvls maelvls added lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Nov 6, 2023
@MalteMagnussen
Copy link

Found this issue after seeing this spammed in the log:

annotation "kubernetes.io/ingress.class" is deprecated, please use 'spec.ingressClassName' instead

@mehmetaydogduu
Copy link

Found this issue after seeing this spammed in the log:

annotation "kubernetes.io/ingress.class" is deprecated, please use 'spec.ingressClassName' instead

If you use microK8s, I created the issue

@mattttv
Copy link

mattttv commented Jan 19, 2024

Hey guys. I'd like to add that this problem results in alerts on Openshift 4.13 and probably 4.12 like IngressWithoutClassName -> https://access.redhat.com/solutions/7017955. Openshift would like the classname to be "openshift-default". It is not broken yet but I suppose it will break in the future.

@ZandercraftGames
Copy link

Still having this issue here as well.

@MalteMagnussen
Copy link

MalteMagnussen commented May 10, 2024

If you use microK8s, I created the issue

We use the https://charts.jetstack.io/ "cert-manager" chart: https://artifacthub.io/packages/helm/cert-manager/cert-manager at 1.13.2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/api Indicates a PR directly modifies the 'pkg/apis' directory help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/feature Categorizes issue or PR as related to a new feature. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. priority/backlog Higher priority than priority/awaiting-more-evidence.
Projects
None yet
Development

No branches or pull requests