Support devfile endpoint annotations

[AObuchow]

Is your enhancement related to a problem? Please describe

The devfile spec defines endpoint annotations, which are a map<string, string> that define annotations to add to created routes/ingresses corresponding to an endpoint.

Currently, these endpoint annotations have no effect in Che.

Describe the solution you'd like

The Che Routing Solver should take the map of annotations defined per-endpoint in the DevWorkspaceRouting object and add them to the spec of routes & ingresses created for a workspace.

Currently, the DevWorkspaceRouting does not yet have endpoint annotations for the Che-Operator to use. Consequently, devfile/devworkspace-operator#1292 must be resolved first, and then Che-Operator can fetch the new DWR API that supports endpoint annotations.

Describe alternatives you've considered

DevWorkspaceOperator receives the list of routes/ingresses created by RoutingSolvers such as the CheRoutingSolver.

We could potentially have DWO add the endpoint annotations to the routes and ingresses -- we're already doing something similar for adding the restricted annotation.

However, there are some considerations to this approach:

1. We'd have to define a standardized way to determine which endpoint corresponds to which route/ingress. Currently, DWO's basic solver will add the controller.devfile.io/endpoint_name to routes/annotations to associate endpoints to routes/ingresses. Che however, uses the che.routing.controller.devfile.io/endpoint-name for the same purpose.
2. There may be endpoint annotations defined in the devfile/devworkspace that Che does not want to have applied to routes/ingresses. Che should have control over deciding which annotations get applied to routes/ingresses.
3. The RoutingSolver implementation is responsible for fully creating routing objects that DWO will sync to the cluster. DWO should ideally not modify these objects.

Additional context

Related to devfile/devworkspace-operator#1023

[AObuchow]

@tolusha @ibuziuk @dkwon17 let me know if you have any opinions on the suggested approach (or if the alternate approach seems better). I will handle the changes required to Che-Operator.

[tolusha]

Hello @AObuchow
I believe (correct me if I am wrong) che operator must take care of some specific things related to Che and not to be the operator which completes the devfile specification. So, despite the fact that routes/ingresses are created by che-operator, DWO should add all needed annotations to them.

[dkwon17]

I see the value of both approaches, I did mention that the solution makes sense and gives lots of freedom to the consumers, but the alternative might be simpler for consumers and simpler to implement on our side (since I don't think we would need to make a new field in the DW routing CR)

I suggest going with the alternative idea and using the DW annotations for endpoints like you mentioned:

diff --git a/controllers/devworkspace/solver/endpoint_exposer.go b/controllers/devworkspace/solver/endpoint_exposer.go
index 13843149..e6078195 100644
--- a/controllers/devworkspace/solver/endpoint_exposer.go
+++ b/controllers/devworkspace/solver/endpoint_exposer.go
@@ -241,8 +241,8 @@ func (e *IngressExposer) getIngressForService(endpoint *EndpointInfo, endpointSt
 
 func routeAnnotations(machineName string, endpointName string) map[string]string {
        return map[string]string{
-               defaults.ConfigAnnotationEndpointName:  endpointName,
-               defaults.ConfigAnnotationComponentName: machineName,
+               dwconstants.DevWorkspaceEndpointNameAnnotation: endpointName,
+               defaults.ConfigAnnotationComponentName:         machineName,
        }
 }
}

[AObuchow]

Hello @AObuchow I believe (correct me if I am wrong) che operator must take care of some specific things related to Che and not to be the operator which completes the devfile specification. So, despite the fact that routes/ingresses are created by che-operator, DWO should add all needed annotations to them.

It would make sense that DWO is in charge of completing the devfile specification given a devworkspace, since DWO can technically be used in isolation. However, the lines are a bit blurred by allowing other actors (e.g. Che-Operator) to create the routing objects required for a devworkspace.

I'm honestly not sure here which component (DWO or Che Router) should be responsible for adding these endpoint annotations.

[AObuchow]

but the alternative might be simpler for consumers and simpler to implement on our side (since I don't think we would need to make a new field in the DW routing CR)

@dkwon17 Correct, it seems like the alternate solution is simpler for consumers. Though a new field in the DWR CRD is needed for the endpoint annotations to be passed from the DevWorkspace object to the DevWorkspaceRouting object (as described in devfile/devworkspace-operator#1292).

Furthermore, since the Che router modifies the DWR instances, Che Operator needs to be updated to consume the latest DWR API that includes endpoint annotations (otherwise, Che router will unset these annotations, causing an endless reconcile loop between DWO and Che router).

After some testing, it seems like there's some details I missed that are required to implement the alternate solution. The changes required are:
On the Che-Operator side:

• Update to the newest DWR API (after Add Annotations to DevWorkspaceRouting Endpoints CRD devfile/devworkspace-operator#1292 is resolved)
• Use a standardized annotation for every workspaces route & ingress to specify the associated endpoint name
• Use a standardized annotation for every workspaces route & ingress to specify the associated endpoint machine name (i.e. component name)
  • The DWR has a map of <machine/component names, array of endpoints>. The Che router is already adding the machine/component name correctly to routes/ingresses using its own Che-specific annotation (which is great)

I don't think it's worth replacing the annotations Che adds to the routes/ingresses it creates, but rather just add the new standardized annotations. This would prevent having to check for the current Che-specific annotations (like we do here) as well as the new standardized annotations. This new standardized annotation would only be something that the Che router adds, and DWO uses.

So something more like:

 func routeAnnotations(machineName string, endpointName string) map[string]string {
        return map[string]string{
               defaults.ConfigAnnotationEndpointName:  endpointName,
               defaults.ConfigAnnotationComponentName: machineName,
+             dwconstants.DevWorkspaceEndpointNameAnnotation: endpointName,
+             dwconstants.DevWorkspaceComponentNameAnnotation:  machineName,
        }
 }

Though, this is an implementation detail that could be discussed further in a PR.

On the DWO-side:

• Pass the devworkspace endpoint annotations to the DWR (this is required regardless of the solution we take)
• Check the ingresses & routes created by the RoutingSolver (Che-Operator, basic-solver, etc) and add their associated endpoint annotations. The endpoint annotations to add are retrieved from the DWR using the endpoint name and machine/component name annotations.

[dkwon17]

I don't think it's worth replacing the annotations Che adds to the routes/ingresses it creates, but rather just add the new standardized annotations. This would prevent having to check for the current Che-specific annotations (like we do here) as well as the new standardized annotations. This new standardized annotation would only be something that the Che router adds, and DWO uses.

@tolusha @AObuchow Is it safe to replace defaults.ConfigAnnotationEndpointName to dwconstants.DevWorkspaceEndpointNameAnnotation everywhere in the che operator? Since existing routes will have their annotations updated by the solver on workspace start, it can help us stick with one kind of annotation

[AObuchow]

@tolusha @AObuchow Is it safe to replace defaults.ConfigAnnotationEndpointName to dwconstants.DevWorkspaceEndpointNameAnnotation everywhere in the che operator? Since existing routes will have their annotations updated by the solver on workspace start, it can help us stick with one kind of annotation

Looking at the Che router code, we actually should be able to safely replace defaults.ConfigAnnotationEndpointName to dwconstants.DevWorkspaceEndpointNameAnnotation everywhere in the che operator. This will have to be verified in testing however, but good call 👍

Edit: I'm not sure yet if the che.routing.controller.devfile.io/endpoint-name annotation is being used elsewhere in Che however.

Edit: I tested, and the annotations can safely be replaced -- existing routes & ingresses owned by the Che Router are updated correctly.

[AObuchow]

A quick search seems to show that only Che Operator and the Che Devfile registry (which is now deprecated IIRC?) use the che.routing.controller.devfile.io/endpoint-name annotation.

A similar search for the che.routing.controller.devfile.io/component-name annotation shows that it's only in use by Che Operator (the Dashboard search results are unrelated)

[AObuchow]

@tolusha After further consideration, I think having the Che Router add the devfile endpoint annotations would make sense and is the cleanest approach.

One could argue the Che Router is already taking care of implementing certain aspects of the devfile API that are not explicitly mentioned in DWO's RoutingSolver interface. For example, the Che Router will check the devfile endpoint's exposure when exposing the endpoints. However.. you'd need to make use of the devfile endpoints exposure when implementing the RoutingSolver interface's GetExposedEndpoints() function, even if it is not explicitly stated in the interface documentation. So this might not be the strongest argument.

A better argument might be that having the Che Router add the devfile endpoint annotations is a lot cleaner and less hacky. If we want DWO to add the endpoint annotations, we have to request RoutingSolver implementors to specifically add 2 annotations to the routes & ingresses it creates (as mentioned here) to fulfil this part of the devfile spec. To me, it feels like a hack to rely on implementors to use a specific string for an annotation, rather than programmatically passing the annotation to use.

I'm still open to being swayed to either approach, but I will be submitting a draft PR for Che-Operator soon which implements my initial suggested solution.