feat : Add support to add Service annotations from DevWorkspaceRouting configuration (#1293)

[rohanKanojia]

What does this PR do?

Add support for adding Service annotations from DevWorkspace component configuration to actual Service created by DevWorkspace operator

• Add new field service that contains annotation field for storing annotations in DevWorkspaceRouting CustomResourceDefinition
• Update code to read DevWorkspace component service annotation to DevWorkspaceRouting service configuration and apply these annotations while creating routing objects in DevWorkspaceRouting controller
• Removes references to service annotations from the unsupported Devfile API documentation
• Remove the webhook warning when applying service annotations

Signed-off-by: Rohan Kumar [email protected]

What issues does this PR fix or reference?

Fix #1293

Is it tested? How?

• Make sure CRC cluster is running
• Install DWO on cluster based on changes added in this PR. You'd need to create new container image and install operator

export DWO_IMG=quay.io/rokumar/devworkspace-controller:dev
make docker
make install

• Apply the following DevWorkspace object, that adds one endpoint exposes by a Service. There is also configuration for adding annotations in Service:

kind: DevWorkspace
apiVersion: workspace.devfile.io/v1alpha2
metadata:
  name: plain-dw-endpoint-annotations
spec:
  started: true
  routingClass: 'basic'
  template:
    components:
      - name: web-terminal
        container:
          image: quay.io/wto/web-terminal-tooling:next
          memoryRequest: 256Mi
          memoryLimit: 512Mi
          mountSources: true
          command:
           - "tail"
           - "-f"
           - "/dev/null"
          annotation:
            service:
              first-service-annotation: test
              second-service-annotation: test2
          endpoints:
          - name: endpoint-1
            targetPort: 8080
            protocol: http
            annotation:
              first-endpoint-annotation: test1
              second-endpoint-annotation: test2  

• Once the workspace has started, check the created Service, annotations specified in should be applied there

oc get svc workspaceaf104799e6fb4633-service -ojsonpath='{.metadata.annotations}'
{"first-service-annotation":"test","second-service-annotation":"test2"}

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]

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

[rohanKanojia]

/ok-to-test

[rohanKanojia]

/ok-to-test

[dkwon17]

If possible, it would be best if this function signature (and also GetServiceForEndpoints) are not changed so that function consumers are not affected. For example for che-operator, we use solver.GetServiceForEndpoints in che_routing.go

[dkwon17]

Or, if we want to change the function signature, maybe we can refactor it to use "Functional Options Pattern" so that new fields can be easily added without breaking consumer code: https://golang.cafe/blog/golang-functional-options-pattern.html ?

For example something similar to:

solver.GetDiscoverableServicesForEndpoints(
    solver.WithEndpoints(endpoints),
    solver.WithMetadata(metadata),
    solver.WithAnnotations(annotations),
)

[rohanKanojia]

hmm, good point. I wasn't aware about this. Let me think if it's possible to do it without changing method signature.

[rohanKanojia]

Looking closely at the changes, Currently Service generation is closely tied to Endpoints, We're going over component to endpoint map and creating Service accordingly. At this point we know for which component we're creating Service, so we can add appropriate annotations for appropriate component.

However, if we want to move this logic outside and set annotations after creating the Service; it would be uncertain which Service belongs to which component. If we decide to mutate Service with a marker label containing component, it should become possible:

I mean replacing this

devworkspace-operator/controllers/controller/devworkspacerouting/solvers/common.go

Lines 60 to 63 in 492c050

	Namespace: meta.Namespace,
	Labels: map[string]string{
	constants.DevWorkspaceIDLabel: meta.DevWorkspaceId,
	},

                                                Labels: map[string]string{
                                                        constants.DevWorkspaceIDLabel: meta.DevWorkspaceId,
+                                                       "workspace.dev/component":     componentName,
                                                },

We can then iterate over generated Services and apply annotations by reading component label (that can be deleted after processing). If you think this approach is okay then I can make changes to keep original method signature.

[dkwon17]

IMHO it should be the DWO that provides the API that conforms with the Devfile spec, therefore, it makes more sense to apply annotations within the functions provided by DWO

[rohanKanojia]

Umm, Not 100% sure I understand but do you mean it's okay to keep these changes?

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dkwon17, rohanKanojia

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 [dkwon17]

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

[dkwon17]

Since infrastructure.IsOpenShift() is commonly used in the codebase, I don't think this fcuntion is necessary

[rohanKanojia]

Actually, I had exposed this for testing. I wanted to mock the behavior of infrastructure.IsOpenShift() method.

Let me think on how I can do it without adding this function.

[rohanKanojia]

Thanks, I see there is already a method created for mocking behavior of this method for tests

devworkspace-operator/pkg/infrastructure/cluster.go

Lines 57 to 60 in cf4c38d

	// InitializeForTesting is used to mock running on a specific type of cluster (Kubernetes, OpenShift) in testing code.
	func InitializeForTesting(currentInfrastructure Type) {
	current = currentInfrastructure
	initialized = true

I'll use already provided method instead of what I added.

[dkwon17]

Suggested change

	func serviceAnnotations(sourceAnnotations map[string]string, isDiscoverable bool, serviceRoutingConfig controllerv1alpha1.Service) map[string]string {
	func mergeServiceAnnotations(sourceAnnotations map[string]string, serviceRoutingConfig controllerv1alpha1.Service) map[string]string {

I suggest "merge" for more clarity.

Also, for readability and simplicity I think it would be best to avoid boolean flags and instead call the function like:

serviceAnnotations := map[string]string{
  constants.DevWorkspaceDiscoverableServiceAnnotation: "true",
}

mergeServiceAnnotations(serviceAnnotations, routingSpec.Service[componentName]),

WDYT?

[rohanKanojia]

Okay, I'll update it as per your comments.

[openshift-ci]

New changes are detected. LGTM label has been removed.

[openshift-ci]

@rohanKanojia: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/v14-che-happy-path	e8fbf4b	link	true	/test v14-che-happy-path

Full PR test history. Your PR dashboard.

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-sigs/prow repository. I understand the commands that are listed here.