Add flag to allow switching ingressClassName specification

[dsonck92]

Pull Request Motivation

In order to support both the legacy annotation and the new ingressClassName of Ingress v1, a flag has been added to allow switching. This should allow users with clusters supporting only ingressClassName to use the latest version, while being fully backwards compatible with existing users that use clusters that do not support ingressClassName.

Closes: #4821

Kind

feature

Release Note

Add new flag `acme-http01-solver-use-ingress-class-name` to allow switching between the legacy annotation and new ingressClassName field.

[jetstack-bot]

Hi @dsonck92. Thanks for your PR.

I'm waiting for a cert-manager member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[jetstack-bot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: dsonck92 To complete the pull request process, please assign wallrj after the PR has been reviewed. You can assign the PR to them by writing /assign @wallrj in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment Approvers can cancel approval by writing /approve cancel in a comment

[irbekrm]

Hi, thanks for the PR @dsonck92 !

I've not looked at the contents yet, in general the idea of a flag seems not bad if it solves people's problems.

There was a lengthy discussion about this and around how downstream projects like us and ingress controllers should approach the new field vs annotation and I know that at least ingress-nginx folks rolled back a bunch of deprecations in regards to the class name field vs annotation as an outcome (same as us).

To help us understand the user value of this flag, could you please provide one or two scenarios where the latest cert-manager release does not work for you (i.e which ingress controller and which version of it and why, if you know that)? (I'm aware that there are a bunch of comments on the original issue, but most of them don't have enough detail)

[dsonck92]

@irbekrm Well, it turns out I could bump the version of my traefik install enough to the point where I could specify the class name as suggested by cert-manager (the value to tweak this setting was added to the helm chart), and this did fix the issue of traefik not picking up the ingresses with only an annotation.

So as far as my own problem, this is actually solved. However, looking back at the whole crisis going on with terraform-kubernetes-provider for lack of support of v1 ingresses, I do have the opinion that giving the end user choice of going full v1 if possible is definitely a plus. In my eyes, the implementation of the flag was relatively straight forward (not sure why it's classed size/M) and documented so it adds the option to go full v1 without annotations. But I do understand that I won't be the one that maintains this code in the long run.

[irbekrm]

Thanks @dsonck92 appreciate that the current situation is generally suboptimal as it is also not clear if there ever will be a migration path off the annotation or if the ingress controllers and us will have to keep supporting this 'forever' in which case we'd then have two mechanisms to maintain 'forever' instead of one with this PR- that is my main concern.

For now, I'd probably keep this open for a bit to see if any more use cases appear - for now I am not convinced that there is enough value for users to justify this.

[YarekTyshchenko]

So what is the current workaround now? I have 2 ingress controllers, and solvers for one are not being picked up. Setting edit in place gives me cryptic:

Warning  Solver  6m32s  cert-manager  Failed to determine a valid solver configuration for the set of domains on the Order: both ingress name and ingress class overrides specified - only one may be specified at a time

cert-manager has both ingress controllers set in solvers settings. I am using ingressClassName field in spec, as recommended by networking.k8s.io/v1 resource version.

Versions: helm chart nginx-ingress-0.14.0 AppVersion 2.3.0 helm chart cert-manager-v1.7.1 AppVersion v1.7.1 K8s Rev: v1.21.9

[markgould]

I came across this as I'm having similar issues. cert-manager 1.9.1 and nginx 1.3.1. None of my renewals work anymore without me going into the ingress and setting ingressClassName on the spec. The annotation is not working anymore. Would love to know what the intended solution is.

[klausenbusk]

For now, I'd probably keep this open for a bit to see if any more use cases appear - for now I am not convinced that there is enough value for users to justify this.

One use-case is newer ingress controllers that do not support the ingress annotation, like the ingress controller in Cilium. In my case, using the ingress controller is Cilium easier as I'm already running Cilium, so I don't need to install a extra component to handle ingresses.

[sathyanarays]

Why do we need a flag to switch? Is it not possible to have both fields simultaneously?

We can have a feature flag; when enabled, the cert-manager adds ingressClassName in addition to the classic annotations. With this approach, backward compatibility is not compromised. We can plan for deprecating the classic annotations at a later point in time. Does this make sense?

[stvnwrgs]

As the annotation is deprecated, why not go with the initial solution from https://github.com/cert-manager/cert-manager/issues/4821?

Just add

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

It's non breaking, easy to upgrade.

[alijahnas]

Hi, we have the same issue as @markgould. The annotation doesn't work, and the ingress controller tells us that : error="ingress class annotation is not equal to the expected by Ingress Controller"

We need to use IngressClassName in the ingress objects created by cert-manager.

When do you think this will be available ? Thanks.

[maelvls]

Hi! I'll be spending time reviewing this PR today.

@alijahnas A few questions:

1. Which version of ingress-nginx are you using?
2. What is the value of your Issuer's spec.acme.solvers.http01.ingress.class?
3. Do you use the flag --ingress-class on your ingress-nginx deployment?

Check that the value in spec.acme.solvers.http01.ingress.class matches the value in --ingress-class.

Note that the flag --ingress-class doesn't refer to an IngressClass resource. The flag --ingress-class refers to the value of the annotation kubernetes.io/ingress.class.

[alijahnas]

Hi @maelvls We use ingress version 1.5.1, and cert-manager 1.9.1. Thanks a lot !

[maelvls]

Hey @dsonck92. Thank you so much for the PR!

Using a flag to "toggle" between the annotation mode and the ingressClassName mode seemed OK to me at first, but I see two problems:

• Validation Consistency: the annotation value for kubernetes.io/ingress.class that you can set with http01.ingress.class can be any string. On the other side, ingressClassName must only be a DNS label. A notable example is the Azure AGIC ingress controller that uses the annotation value azure/application-gateway. With --acme-http01-solver-use-ingress-class-name=false, no validation issue will be found. With --acme-http01-solver-use-ingress-class-name, the ingress controller will start failing.
• Incompatible Ingress Controllers: if I use an ingress controller that only supports annotations (ingress-gce is the only example) and I also want to use ingressClassName with another ingress controller, then this flag won't be useful.

What do you think about adding a new field on the Issuer and ClusterIssuer CRD instead of using a flag?

For example:

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

The field has the disadvantage of making the API even more confusing, but I think it is the lesser evil.

Let me know if you need help on that.

[jetstack-bot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: dsonck92 To complete the pull request process, please ask for approval from maelvls after the PR has been reviewed.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment Approvers can cancel approval by writing /approve cancel in a comment

[jetstack-bot]

@dsonck92: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-cert-manager-master-make-test	d79c1f19df5f47e85098c58a88ba8bea14639d15	link	true	/test pull-cert-manager-master-make-test

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

[dsonck92]

@maelvls That seems like an acceptable solution.

[jetstack-bot]

@dsonck92: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.