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

Allow ingressClassName to be set for HTTP01 solver ingresses. #4821

Closed
Robfz opened this issue Feb 1, 2022 · 24 comments · Fixed by #5849
Closed

Allow ingressClassName to be set for HTTP01 solver ingresses. #4821

Robfz opened this issue Feb 1, 2022 · 24 comments · Fixed by #5849
Labels
area/ingress-shim Indicates a PR or issue relates to the ingress-shim 'auto-certificate' component kind/feature Categorizes issue or PR as related to a new feature.

Comments

@Robfz
Copy link

Robfz commented Feb 1, 2022

Is your feature request related to a problem? Please describe.

I have multiple IngressControllers running in my cluster. Each of them is configured to only control ingresses with a given IngressClass (as described in Ingress Nginx documentation). With the recent revert of changes derived from this discussion (#4537 (comment)), if I add the class property to the solver's Ingress spec, the now deprecated kubernetes.io/ingress.class annotation will be used:

  solvers:
    - http01:
       ingress:
         class: external-nginx

This will result in the Ingress created by the solver to never being picked up by any IngressController, resulting in the inability of the solver to complete the challenge. Currently, we are solving this by manually editing the Ingress deployment to include the desired ingressClassName, but this implies manual work every time new certificates need to be updated/issued.

Describe the solution you'd like
I understand (and agree) that maintaining compatibility is important. Given my limited context of this project, it would be nice to have the option to set the ingressClassName like this:

  solvers:
    - http01:
       ingress:
         ingressClassName: external-nginx

This way, the class attribute could still be implemented with the anotation, and newer clients could opt-in to use IngressClasses.

Describe alternatives you've considered
We could probably use another IngressController without any IngressClass set in its configuration, allowing it to pickup the ingresses created by cert-manager, but that seems more like a workaround that a solution.

Additional context
None, but may provide more if asked.

Environment details (remove if not applicable):

  • Kubernetes version: 1.21
  • Cloud-provider/provisioner: AWS
  • cert-manager version: v1.7.0
  • Install method: Helm, including resource definitions.

/kind feature

@jetstack-bot jetstack-bot added the kind/feature Categorizes issue or PR as related to a new feature. label Feb 1, 2022
@irbekrm
Copy link
Collaborator

irbekrm commented Feb 2, 2022

Thanks for opening the issue and the good problem description.

Describe alternatives you've considered
We could probably use another IngressController without any IngressClass set in its configuration, allowing it to pickup the ingresses created by cert-manager, but that seems more like a workaround that a solution.

Would using the existing Ingress resources (via the acme.cert-manager.io/http01-edit-in-place: "true" annotation see supported annotations) instead of allowing the solver to create new Ingresses to solve challenges not work for you?

@irbekrm irbekrm added the area/ingress-shim Indicates a PR or issue relates to the ingress-shim 'auto-certificate' component label Feb 2, 2022
@Robfz
Copy link
Author

Robfz commented Feb 4, 2022

Hey @irbekrm, thanks for the reply!

I think using the annotation should work! I will give it a shot early next week and report back. But still, I think the ability of adding the ingressClassName to the Ingresses should be supported.

@xmath279
Copy link

xmath279 commented Feb 8, 2022

Thanks for opening the issue and the good problem description.

Describe alternatives you've considered
We could probably use another IngressController without any IngressClass set in its configuration, allowing it to pickup the ingresses created by cert-manager, but that seems more like a workaround that a solution.

Would using the existing Ingress resources (via the acme.cert-manager.io/http01-edit-in-place: "true" annotation see supported annotations) instead of allowing the solver to create new Ingresses to solve challenges not work for you?

It's a good solution, but it wouldn't work in cases where something like ArgoCD or Flux is used to make sure what is deployed into the cluster matches what is specified in a Git repository. Those tools would just instantly overwrite the in-place edit cert-manager would do.

@sagikazarmark
Copy link

sagikazarmark commented Feb 16, 2022

IMHO this should be reclassified as a bug: many ingress controllers don't support the ingress annotation anymore that gets set when a class is defined rendering that config option useless.

For networking.k8s.io/v1 setting the ingressClassName should always be preferred.

Update: and to make things worse, ingressTemplate doesn't support setting ingressClassName either, so this can't even be worked around.

Update 2: I found the discussions about reverting to the annotation and I'm a bit surprised that there is no forward compatibility with ingressClassName.

@auwaerter
Copy link

auwaerter commented Mar 2, 2022

IMHO this should be reclassified as a bug: many ingress controllers don't support the ingress annotation anymore that gets set when a class is defined rendering that config option useless.

For networking.k8s.io/v1 setting the ingressClassName should always be preferred.

Update: and to make things worse, ingressTemplate doesn't support setting ingressClassName either, so this can't even be worked around.

Update 2: I found the discussions about reverting to the annotation and I'm a bit surprised that there is no forward compatibility with ingressClassName.

Agree regarding the missing forward compatibility, as i do have to deal with the same issue.

My workaround currently (using ingress-nginx 1.1.1 & cert-manager 1.7.1) is to delete the failed order & certificaterequest, retrigger the renewal with:

kubectl cert-manager renew <nameofcert>

get the created ingress object as fast as possible after creation:

kubectl get ingress

and patch the ingress object with the ingressClassName the ingress-nginx controller is using:

kubectl patch ingress cm-acme-http-solver-<ID> --type=json -p '[{"op":"add","path":"/spec/ingressClassName","value":INGRESS-CLASS-NAME}]'

Would be great if the service is getting created with ingressClassName, as in my case i already completley moved away from any annotation related to ingress in my cluster. Also it should be seen as deprecated since k8s 1.18.

Update: Now with ingress-nginx 1.1.2, they provide a backwards compatibility to the annotation, which makes my workaround obsolete. Anyway, a support for ingressClassName would be great :)

@wenslauw
Copy link

Thanks for that patch line, @auwaerter. I hope this gets fixed soon, because it's a bit of a pain.

@adroste
Copy link

adroste commented Apr 21, 2022

Same here. It's definitely a regression bug to revert to the annotation. Stupid question, why not set both? (Annotation + ingressClassName)

@irbekrm
Copy link
Collaborator

irbekrm commented Apr 22, 2022

Same here. It's definitely a regression bug to revert to the annotation. Stupid question, why not set both? (Annotation + ingressClassName)

There are some (rather lengtly) discussions that shows how/why that decision was made here #4537 and on Slack here with some more links, but TLDR is backwards compatibility
An ingress resource with both set is invalid unfortunately else it would be the optimal solution

@tculp
Copy link

tculp commented Apr 22, 2022

Could a solution be for cert manager to set the ingressclassname if it can see an ingressclassresource with the same class name, and set the annotation otherwise?

Either way, being able to choose to adopt the behavior through a different annotation or field would be sufficient for our uses.

@jakexks
Copy link
Member

jakexks commented Apr 28, 2022

We're stuck between a rock and a hard place here.

Unless a conformance test for Ingress is written and adhered to by all Ingress controllers we have to choose the most compatible option, which is still currently the annotation. I still don't believe we can use ingressClassName on ingress-gce for example. (kubernetes/ingress-gce#1301)

Perhaps we could add a feature gate command line flag to cert-manager that chooses between the annotation and spec versions of ingress class? I don't want to change our API (crds) for this.

@TheHunter
Copy link

TheHunter commented May 11, 2022

Thanks for opening the issue and the good problem description.

Describe alternatives you've considered
We could probably use another IngressController without any IngressClass set in its configuration, allowing it to pickup the ingresses created by cert-manager, but that seems more like a workaround that a solution.

Would using the existing Ingress resources (via the acme.cert-manager.io/http01-edit-in-place: "true" annotation see supported annotations) instead of allowing the solver to create new Ingresses to solve challenges not work for you?

It's a good solution, but it wouldn't work in cases where something like ArgoCD or Flux is used to make sure what is deployed into the cluster matches what is specified in a Git repository. Those tools would just instantly overwrite the in-place edit cert-manager would do.

Hi all,
not only, because if you use another annotation like "nginx.ingress.kubernetes.io/rewrite-target", the current ingress resouce doesn't work .. so the problem remains.

This is a big issue, and now even using the "http01-edit-in-place: "true" annotation could be resolved the problem completely.

The solution proposed by @Robfz is great, but with some changes maybe, example

solvers:
- http01:
ingress:
name: external-nginx ## the class name to use (IngressClass object)

@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 Aug 9, 2022
@merusso
Copy link

merusso commented Aug 17, 2022

/remove-lifecycle stale

We're using cert-manager with Kong Ingress Controller and rely upon the ingress class being set correctly. Once PR #4439 was reverted, this broke our setup. It appears that the Kong Ingress Controller does not honor the annotation kubernetes.io/ingress.class when using v1 Ingress, only works with ingressClassName.

IMO, having a feature flag to enable use of ingressClassName would work best for us.

Unfortunately we're not able to use the annotation acme.cert-manager.io/http01-edit-in-place: "true" with our setup.

Edit: After doing more testing, we found that Kong Ingress Controller was correctly processing the deprecated annotation. We think a separate config issue was the real cause for our issues. This is working fine for us now.

@jetstack-bot jetstack-bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Aug 17, 2022
@ruckc
Copy link

ruckc commented Sep 30, 2022

bumping this... trying to use cilium's ingress controller and it requires ingressClassName. It does not support the annotation.

@ruckc
Copy link

ruckc commented Sep 30, 2022

Also the "blocking" issue with kubernetes/ingress-gce#1301 has been closed now. It would be appreciated if when v1 is detected, that it uses ingressClassName per the kubernetes spec.

@tudor-pop
Copy link

just wasted 8 hours wondering why I get 404... turns out it's because the annotation doesn't work or is not picked up by cert-manager... works fine with edit-in-place but for how long?
if ingressClassName is added to ingressTemplate it should be used

@markgould
Copy link

Any updates on this? How are we supposed to move forward?

@dvjjoling
Copy link

This cluster issuer deployment seems to work for me.
You have to apply it with the --validate=false parameter.

https://gist.github.com/dvjjoling/0a4bb444cc3fca9f65d55e4b0fff784a

@obliadp
Copy link

obliadp commented Dec 21, 2022

We're stuck between a rock and a hard place here.

Unless a conformance test for Ingress is written and adhered to by all Ingress controllers we have to choose the most compatible option, which is still currently the annotation. I still don't believe we can use ingressClassName on ingress-gce for example. (kubernetes/ingress-gce#1301)

ingress-gce now only uses v1 apis, so this is no longer an issue

@stvnwrgs
Copy link

@Robfz I would change

solvers:
    - http01:
       ingress:
         ingressClassName: external-nginx

to

solvers:
   - http01:
      ingress:
        className: external-nginx

@nabokihms
Copy link

@irbekrm Hello from the community! We are struggling without this feature because the only option left for us now is to add issuer per ingress class, which leads to many letsencrypt issuers like letsencrypt-nginx, letsencrtypt-class-1, letsencrtypt-class-2, etc.

What can we do to move this forward? Thank you in advance for all your work.

@irbekrm
Copy link
Collaborator

irbekrm commented Feb 16, 2023

Hi @nabokihms there isn't really many options for us- we cannot start using the field instead of annotation as it would break backwards compatibility and both the field and the annotation aren't supported. I would be open to the idea of adding a flag as per #5225 but we need to understand whether that is a solution that would work

What can we do to move this forward?

  • Any clear description of why a latest version of any ingress controller implementation (which implementation, which version and what you're trying to do) does not work with cert-manager for a particular scenario would be very helpful- the original issue description is great, but I don't know what ingress controller version this is referring to and I assume the problem might no longer exist with the latest nginx ingress- as I understand once the networking community realized that there is no deprecation path for the annotation, controller implementations reverted removal of support for the annotation just like we did- and as I understand they should keep supporting it unless they break backwards compatibility

  • A comment as to whether a flag like suggested in Add flag to allow switching ingressClassName specification #5225 would fix the problem or not

Again linking this Slack chat with sig-network folks for context https://kubernetes.slack.com/archives/C09QYUH5W/p1641818790038500

@markgould
Copy link

I upgraded our helm chart for ingress-nginx from 4.4.0 to 4.5.2 and it looks like it has fixed it.

@maelvls
Copy link
Member

maelvls commented Feb 20, 2023

In the OP's case, ingress-nginx (>v1.0.1) already picks up ingresses that have the nginx annotation, which can be configured with --ingress-class.

I have looked at 13 ingress controllers, and the only one that doesn't support the kubernetes.io/ingress.class annotation is NGINX Inc.'s NGINX Ingress. NGINX Inc.'s NGINX Ingress is the only ingress controller that won't work with cert-manager because cert-manager doesn't support ingressClassName.

Note: the annotation kubernetes.io/ingress-class will never go away, so we will always support it.

The real problem is that cert-manager can't be configured to work with an IngressClass. The ingress controller that is used by cert-manager can't be configured with an IngressClassParams resource.

I suggest that we move on with implementing #5225 in a backwards-compatible way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/ingress-shim Indicates a PR or issue relates to the ingress-shim 'auto-certificate' component kind/feature Categorizes issue or PR as related to a new feature.
Projects
None yet