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

Not able to set the default ingressClassName when user creates issuer using class tag. #6897

Open
anirudhAgniRedhat opened this issue Apr 12, 2024 · 5 comments
Labels
kind/bug Categorizes issue or PR as related to a bug.

Comments

@anirudhAgniRedhat
Copy link

anirudhAgniRedhat commented Apr 12, 2024

The alert IngressWithoutClassName was introduced to Cluster. whether Ingress objects would exist without an ingressClassName specified. Given that without an ingressClassName specified, it's not clear what the ingress object is supposed to do, it's vital that alerting is taking place and actions are taken to set the ingressClassName properly for each ingress object.

And as the Ingress resources without a spec.ingressClassName are generated by the Cert-Manager.

I request that Cert-Manager should be fixed to create its resolver Ingress resources with a spec.ingressClassName defined."

Describe the bug:
We can create an Ingress without having IngressClassNAme specified. I am getting alerts messages that ingress Exists without IngressClassName Specified.

I propose that we can add default ingressClassName to issuer if ingressClassName is not specified.

Expected behaviour:
It should Set default ingressClassName if ClassName is not specified.

/kind bug

@jetstack-bot jetstack-bot added the kind/bug Categorizes issue or PR as related to a bug. label Apr 12, 2024
@anirudhAgniRedhat
Copy link
Author

Screenshot from 2024-04-12 13-55-19

The sample-ingress-1 is created with IngressClassName specified.
The sample-ingress-class is created with class specified.

@maelvls
Copy link
Member

maelvls commented Apr 22, 2024

Hey, thanks for sharing this!

I assume that you are referring to the alert introduced in OpenShift 4.12. I wasn't aware of this warning. I found an explanation as to why this warning exists in transition-ingress-from-beta-to-stable.md:

We are considering adding alerts in case any Routes existed in this state, so that the administrator would know that the Routes needed to be deleted, or the Ingress modified to specify an appropriate IngressClass so that OpenShift would once again reconcile the Routes.

Right now, cert-manager has three different "modes":

  1. The "Old" way: when class is configured on an ACME Issuer, the generated Ingress is set with the annotation kubernetes.io/ingress.class.

    apiVersion: cert-manager.io/v1
    kind: Issuer
    spec:
      acme:
        solvers:
        - http01:
            ingress:
              class: nginx
    ---
    # Resulting ACME resolver ingress:
    apiVersion: networking.k8s.io/v1
    kind: Ingress
    metadata:
      annotations:
        kubernetes.io/ingress.class: nginx
  2. The "New" way: when ingressClassName is configured on an ACME Issuer, the generated Ingress's spec. ingressClassName is that to that value.

    apiVersion: cert-manager.io/v1
    kind: Issuer
    spec:
      acme:
        solvers:
        - http01:
            ingress:
              ingressClassName: nginx
    ---
    # Resulting ACME resolver ingress:
    apiVersion: networking.k8s.io/v1
    kind: Ingress
    spec:
      ingressClassName: nginx
  3. The "default" way: when class and ingressClassName aren't configured, the spec.ingressClassName field is left empty and no annotation is added.

I imagine that you are referring to (3). (3) is necessary for backwards compatibility reasons: a while back, some ingress controllers were picking up Ingresses by default. This is still how ingress-gce operates.

I propose that we can add default ingressClassName to issuer if ingressClassName is not specified.

If we were to add a "default" ingressClassName for issuers that don't have ingressClassName set, it would have to be in a non-breaking way, for example by adding a flag --default-issuer-ingress-class-name.

Before continuing, can you explain what prevents users from setting ingressClassName on their issuers? I imagine that it would require changes in lots of places, and the platform admin in charge of "fixing" this warning would like to do that without changing everyone's issuers.

@anirudhAgniRedhat
Copy link
Author

anirudhAgniRedhat commented Apr 22, 2024

@maelvls Firstly user has to add any one of class or ingressClassName, I Agree this would need changes.
The small hack we can provide for users is to patch ingress kubectl patch ingress/<ingress-name> --type=merge --patch '{"spec":{"ingressClassName":default"}}' -n <namespace>

The main issue arises is that in cases of these alerts there are some cascading effects like re-issuing the certificate and restarting the ingress.

I think can we add class as ingressClassName itself if ingressClassName is not provided this would help people to avoid these errors in first place.

Please correct me if I am wrong.

@maelvls
Copy link
Member

maelvls commented May 2, 2024

Can we add class as ingressClassName itself if ingressClassName is not provided

Defaulting the field ingressClassName with the value in the field class would break anyone using Azure AGIC with class set and ingressClassName unset.

The problem is that class accepts anything that is a valid annotation value, but ingressClassName must be a DNS label. We broken Azure AGIC users in the past (#4547) due to this subtle difference.

Does it make sense?

@maelvls
Copy link
Member

maelvls commented May 3, 2024

I met with Anirudh this morning. Here are the notes I took from our meeting:

  1. An OpenShift customer, specifically the platform team, says that they are spammed by a lot of alerts since they upgraded to OpenShift 4.12.
  2. The official documentation for using cert-manager in OpenShift with ACME says that you should use class. This recommendation is made for all current OpenShift versions: 4.12, 4.13, 4.14, and 4.15. The page shows the following: Screenshot of OpenShift 4.12 documentation on 3 May 2024
  3. The OpenShift Ingress Controller team decided to add this warning in OpenShift 4.12, but many ingress controllers still offer the "default controller" feature (see this comparison page), meaning that it is possible to use an Ingress resource without setting the annotation or ingressClassName. Does it mean that the OpenShift Ingress Controller team says that ingress-gce's default controller isn't supported by OpenShift anymore?
  4. It is possible to default the ingressClassName to some dummy value on Issuers or Ingress resources using Kyverno (or a custom controller), but it feels like a work around.

Actions:

  • Anirudh to ask question (3) to the OpenShift team responsible for the Ingress API, and ask if it's possible to set up a meeting with them and invite me.
  • Anirudh to investigate why the OpenShift docs still says "use the class field" when it's not recommended by OpenShift.
  • Anirudh to investigate Kyverno and other policy engines that can mutate objects. An example of Kyverno ClusterPolicy is available in https://cert-manager.io/docs/tutorials/certificate-defaults/.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug.
Projects
None yet
Development

No branches or pull requests

3 participants