Added includeClassAnno as a workaround for cert-manager v1.14.5

[pertsevds]

Closes #2267 and #1570

[ikreymer]

I wonder if this should be called something else, like useOldClassField instead of includeClassAnno since its not an annotation and its specifically about the old field?

[pertsevds]

So. Should I change it to useOldClassField or what should be a better name for it?

[ikreymer]

Yes, let's change it to that if you don't mind and then can merge

[ikreymer]

Suggested change

	includeClassAnno: false
	useOldClassField: false

[ikreymer]

Suggested change

	{{- if .Values.ingress.includeClassAnno }}
	{{- if .Values.ingress.useOldClassField }}

[ikreymer]

Hm, seems like this should be an if / else, using ingressClassName only if useOldClassField is not set?

[pertsevds]

Why?

flowchart TD
    A[Start] --> B
    B{useOldClassField is set}
    B -->|No| C['class:' will not be rendered]
    B -->|Yes| D
    D{useOldClassField is true}
    D -->|No| C['class:' will not be rendered]
    D -->|Yes| F
    F{ingress_class is set}
    F -->|No| G[ingress_class: nginx,<br>class: nginx]
    F -->|Yes, some string| H[ingress_class: some string,<br>class: some string]

Loading

[pertsevds]

Oh, I see now.
Right now everyone have a config without useOldClassField and after this update they will have their class: option removed.
Right?

So. Should we name this option useNewClassField instead?

[ikreymer]

Yes, I think the old class value is for the annotation, which is now obsolete, so can default it to off.
I'm surprised that this worked before, as this check seems to be here for a while:
https://github.com/cert-manager/cert-manager/blame/b96e0af16d197f021904b11066d6307504524f6f/pkg/issuer/acme/http/ingress.go#L159

Elsewhere, we already use ingressClassName so I think that should be the default, and only use the old class if the useOldClassField is set.

Maybe useOldClassAnnotation is more clear even. It was deprecated a while ago now (1.18): https://kubernetes.io/docs/concepts/services-networking/ingress/#deprecated-annotation

Perhaps the only reason to keep it is for GKE, which still requires it: https://cloud.google.com/kubernetes-engine/docs/concepts/ingress#controller_summary

[ikreymer]

Probably to be consistent, we should update the other two ingressClassName to also only be set if useOldClassAnnotation is false, which will be default, otherwise set kubernetes.io/ingress.class
here: https://github.com/webrecorder/browsertrix/blob/main/chart/templates/ingress.yaml#L24
and https://github.com/webrecorder/browsertrix/blob/main/chart/templates/ingress.yaml#L65

[ikreymer]

If you can update soon, we can add this to next 1.13.x release, otherwise will need to wait till 1.14.0

[pertsevds]

Updated.

[pertsevds]

If you can update soon, we can add this to next 1.13.x release, otherwise will need to wait till 1.14.0

Maybe it's better to merge it to 1.14.0 as it's a new parameter in the configuration?