fix: replace deprecated Ingress annotation

[gbonnefille]

What does this PR do?

Before the IngressClass resource and ingressClassName field were added in Kubernetes 1.18, Ingress classes were specified with a kubernetes.io/ingress.class annotation on the Ingress. This annotation was never formally defined, but was widely supported by Ingress controllers.

The newer ingressClassName field on Ingresses is a replacement for that annotation, but is not a direct equivalent. While the annotation was generally used to reference the name of the Ingress controller that should implement the Ingress, the field is a reference to an IngressClass resource that contains additional Ingress configuration, including the name of the Ingress controller.

Cf. https://kubernetes.io/docs/concepts/services-networking/ingress/#deprecated-annotation

PR Checklist

• E2E tests pass (when PR is ready, comment /test v8-devworkspace-operator-e2e, v8-che-happy-path to trigger)
  • v8-devworkspace-operator-e2e: DevWorkspace e2e test
  • v8-che-happy-path: Happy path for verification integration with Che

[openshift-ci]

Hi @gbonnefille. Thanks for your PR.

I'm waiting for a devfile 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.

[codecov]

Codecov Report

Patch coverage has no change and project coverage change: +0.05 🎉

Comparison is base (1392456) 52.37% compared to head (5cded0f) 52.43%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1143      +/-   ##
==========================================
+ Coverage   52.37%   52.43%   +0.05%     
==========================================
  Files          81       81              
  Lines        7381     7381              
==========================================
+ Hits         3866     3870       +4     
+ Misses       3234     3230       -4     
  Partials      281      281              

Impacted Files	Coverage Δ
...roller/devworkspacerouting/solvers/basic_solver.go	0.00% <ø> (ø)
...s/controller/devworkspacerouting/solvers/common.go	0.00% <0.00%> (ø)

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[gbonnefille]

Since the addition of Kind=IngressClass, there is no reason to hardcode a reference to nginx in the generated Kind=Ingress. Perhaps DWO can look at Kind=IngressClass, identify the default one, and then use this as ingressClassName:.

[amisevsk]

/ok-to-test

[amisevsk]

LGTM

Thanks @gbonnefille for continuing to follow up here; it's much appreciated!

I'd still like to figure something out for #1068 -- I'll try to fit it in when I have time, I've been more focused on the OpenShift use case.

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: amisevsk, gbonnefille

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

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [amisevsk]

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

[gbonnefille]

Yes, finding a solution for #1068 could be really interesting.

Having in mind that the availability of Kind=ingressClass could be a solution to discover the default IngressClassName instead of having the user to configure it, I started reviewing the code. And it is hard to find an elegant solution. Because I will need to search for the Kind=IngressClass from solver implementations, but in this context we do not have simple access to the K8S itself (for example, for requesting the list of Kind=IngressClass).

I also realized that a good implementation would need to request the K8S in order to evaluate if we are using networking.k8s.io/v1 (and so ingressClassName:) or a previous version (and thus kubernetes.io/ingress.class annotation). But the same here: no simple way to request the K8S from solver implementations.

And finally, as Ingress is a mess with many implementation specific annotations, having a basic_router without any hardcoded values, but a good way to configure any aspect could help a lot.

[amisevsk]

Yeah, making manual configuration possible would be ideal, though as you say the variety of ingress implementations is kind of a mess, and anything we do around this would also require work in whatever operators depend on us (and implement a routing reconcile -- e.g. Che Operator).

It'd be great if there was a way to automatically detect e.g. traefik vs nginx and apply the appropriate annotations, though I don't know if it's possible on Kubernetes (on OpenShift, we use e.g. a test route to discover the cluster's base hostname).

[gbonnefille]

Yeah, making manual configuration possible would be ideal, though as you say the variety of ingress implementations is kind of a mess, and anything we do around this would also require work in whatever operators depend on us (and implement a routing reconcile -- e.g. Che Operator).

Yes, sure.

It'd be great if there was a way to automatically detect e.g. traefik vs nginx and apply the appropriate annotations, though I don't know if it's possible on Kubernetes (on OpenShift, we use e.g. a test route to discover the cluster's base hostname).

First, the IngressClass' name. As spotted by https://kubernetes.io/docs/concepts/services-networking/ingress/, the spec.ingressClassName: attribute is mostly mandatory. When present, it should be respected. When absent, a default IngressClass will be selected. Recently, I noticed that ingress-nginx really needs it. If absent, traefik is able to update the resource and set it.

With the current fix proposed by this PR, a K8S cluster admin is able to have basic routing solver works, as he only have to declare a Kind=IngressClass with name nginx and routing to the traefik.

Other ideas:

1. exclude the spec.ingressClassName attribute from the reconciling part, in order to let ingress controler fix it.
2. avoid an hardcoded value and pick one from the installed Kind=IngressClass resources, if there is a default.

But it could be tricky to auto-discover and if the automatic job is not the right one, there is no chance to manually adapt it.

One possible other solution is to use a dedicated IngressClass resource, with name dwo-ingress for example. The generated Ingress then have hardcoded spec.ingressClassName: dwo-ingress. The initial installation/start of the operator create the IngressClass with name dwo-ingress if absent, referencing nginx or trying to discover an ingress controler.
And then, if the K8S admin has a different opinion, he is able to update the IngressClass with name dwo-ingress to reflect his better choice.

Concerning the other annotations (nginx.ingress.kubernetes.io/rewrite-target and nginx.ingress.kubernetes.io/ssl-redirect) I have no idea. Traefik has also a lot of annotations: https://doc.traefik.io/traefik/routing/providers/kubernetes-ingress/
If it is not possible to manually configure them, perhaps should we add (hardcoded) all known annotations for any known ingress controller (currently nginx and traefik). For example traefik.ingress.kubernetes.io/router.tls: false. But I fear it could be rapidly related to the effective configuration of the ingress controler.

[gbonnefille]

Please, note that cert-manager, an other controller having to create Ingress in order to solve the ACME challenge, has a setting for ingressClassName: https://cert-manager.io/docs/configuration/acme/http01/#ingressclassname

[amisevsk]

Merging this PR as it fixes a problem (thanks again!). I'll try to schedule some work on configuring ingress annotations/ingress class when I have some time.