feat: Support Ingress Class Annotation in Argo CD CRD

[sagikazarmark]

What type of PR is this?
/kind enhancement

What does this PR do / why we need it:
This PR adds IngressClass support to Ingress resources.

Have you updated the necessary documentation?

• Documentation update is required by this PR.
• Documentation has been updated.

Which issue(s) this PR fixes:

Fixes #626

How to test changes / Special notes to the reviewer:

Relevant tests can be run by the following command:

go test -v -run 'TestReconcileArgoCD_reconcile_.*Ingress_ingressClassName' ./controllers/argocd/

Change can be tested by setting up a dev environment and applying the following manifest:

apiVersion: argoproj.io/v1alpha1
kind: ArgoCD
metadata:
  name: example-argocd
  labels:
    example: ingress
spec:
  server:
    grpc:
      ingress:
        enabled: true
    ingress:
      enabled: true
      ingressClassName: nginx
    insecure: true

Note: As discussed in #626, this PR contains a breaking change: kubernetes.io/ingress.class annotation is no longer added and nginx is no longer the default ingress controller (Kubernetes will fall back to the default ingress class)

Default nginx annotations (SSL redirect, backend) are still added though.

[CLAassistant]

All committers have signed the CLA.

[iam-veeramalla]

@sagikazarmark , I was trying to test this changes on my minikube cluster which has the following IngressClass resource

apiVersion: networking.k8s.io/v1
kind: IngressClass
metadata:
  annotations:
    ingressclass.kubernetes.io/is-default-class: "true"

What I noticed is the IngressClass is always set to nginx. In a nutshell, .spec.server.ingress.ingressClassName is not respected.

[sagikazarmark]

@iam-veeramalla Can you clarify what is always set to nginx means? Is it actually set to nginx? Or does it fallback to the above default (which is nginx)?

Do you have any other ingress classes defined in the cluster?

I tested it in a kind cluster with grpc and server ingress with grpc left empty (falling back to the default) and the server explicitly set to nginx. Both showed up with the correct ingress class (grpc: null, server: nginx).

[iam-veeramalla]

e any other ingress classes defined in the cluster?

Hi @sagikazarmark , I just have one IngressClass defined in the cluster and that is set to default -> "nginx".

I created an Argo CD instance with .spec.server.ingress.ingressClass: fake. Up on the reconciliation I see that the Ingress resource created for Argo CD server has IngressClass set to nginx, which I believe should be fake.

However, I can try it one more time on my Kind cluster.

[sagikazarmark]

Just to clarify: the new field is called ingressClassName, not ingressClass.

I don't know how Kubernetes behaves if you specify a non-existent ingress class in the ingress resource, but falling back to the default one doesn't sound necessarily bad. Try to create an ingress class resource with that name.

I'll do some additional testing as well.

[iam-veeramalla]

Just to clarify: the new field is called ingressClassName, not ingressClass.

I don't know how Kubernetes behaves if you specify a non-existent ingress class in the ingress resource, but falling back to the default one doesn't sound necessarily bad. Try to create an ingress class resource with that name.

I'll do some additional testing as well.

Yeah ingressClassName.

Thank you !! I will do some additional testing as well !!

[sagikazarmark]

@iam-veeramalla any updates?

[iam-veeramalla]

HI @sagikazarmark , Sorry I reviewed this and also tested before I went to vacation, Just forgot to merge it. I can confirm that this works :)

[iam-veeramalla]

LGTM, Thanks :)

[sagikazarmark]

Awesome, thanks!

[wtam2018]

@sagikazarmark @iam-veeramalla
I see the annotation kubernetes.io/ingress.class has been removed but the constant is still there in the code.
Any there any impact to upgrade by this change? That is, folks who may depend on this annotation. The default ingress class is empty. So, will this change break use cases which uses the old default annotation?

[iam-veeramalla]

ss is em

Hi @wtam2018 the old annotation cannot be used in anyways as the annotation is deprecated. This will be a breaking change as mentioned by @sagikazarmark in the comment
#626 (comment)

[wtam2018]

Well, introducing a break change for an enhancement without any mitigation does not seem to make too much sense. Let's hope no users are depending the old behavior. BTW, deprecation means can still be used.

[sagikazarmark]

@sagikazarmark @iam-veeramalla I see the annotation kubernetes.io/ingress.class has been removed but the constant is still there in the code.

Correct, the annotation could be removed if nothing else uses it.

So, will this change break use cases which uses the old default annotation?

It depends: it might break on unsupported Kubernetes version or with old ingress controllers.

This PR changes a default that didn't really make sense in the first place (defaulting to an ingress class called "nginx" that may or may not exist in the first place).

The new default uses the new ingress class types and defaults to the default ingress controller.

To sum up: it may break in some edge cases, but the default makes more sense now and supports more setups, so I'd say it's an improvement even if it breaks in some cases.

[wtam2018]

Please push a PR to remove the unused constant. Please add doc about the removal of the annotation and the migration instructions for those who are upgrading with those use/edge cases. @iam-veeramalla