Giter Club home page Giter Club logo

Comments (5)

maelvls avatar maelvls commented on May 26, 2024 1

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?

from cert-manager.

anirudhAgniRedhat avatar anirudhAgniRedhat commented on May 26, 2024

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.

from cert-manager.

maelvls avatar maelvls commented on May 26, 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.

from cert-manager.

anirudhAgniRedhat avatar anirudhAgniRedhat commented on May 26, 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.

from cert-manager.

maelvls avatar maelvls commented on May 26, 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/.

from cert-manager.

Related Issues (20)

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. 📊📈🎉

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❤️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.