allow to configure specific annotations #1070

gbonnefille · 2023-03-21T17:38:24Z

What does this PR do?

It adds the ability to configure the annotations applied to routing object (Route or Ingress).

What issues does this PR fix or reference?

It fixes #1068

Is it tested? How?

Currently, only tested by unit tests

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 · 2023-03-21T17:38:33Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: gbonnefille
Once this PR has been reviewed and has the lgtm label, please assign amisevsk for approval. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

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

openshift-ci · 2023-03-21T17:38:36Z

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.

AObuchow

@gbonnefille Thank you so much for the PR :) 🙏

I left some initial review comments, though you'll have to wait for @amisevsk's final review and approval for this to get merged (assuming there are no blocking reasons why this couldn't be added to DWO). He's currently on PTO right now, but will be back soon enough.

3 other details I wanted to point out:

You'll have to modify sync.go to allow the new config.routing field you've added to be merged and logged.

You'll have to add something like the following here:

if from.Routing.Annotations != nil {
	to.Routing.Annotations = from.Routing.Annotations
}

And something like the following here:

if routing.Annotations != nil && len(routing.Annotations) > 0 {
	config = append(config, fmt.Sprintf("routing.Annotations=%s", routing.Annotations))
}

Make sure all tests complete successfully (make test), with this one in particular for merging the config

The CRDs will have to be re-generated since you added a new field to the operator configuration. Run the following, and add the resulting changes into a separate commit: make generate manifests fmt generate_default_deployment generate_olm_bundle_yaml
Once you've done all of the above, please manually test that your changes resolve your issue. If you need help deploying your custom build of DWO, just ping me :)

apis/controller/v1alpha1/devworkspaceoperatorconfig_types.go

controllers/controller/devworkspacerouting/solvers/basic_solver.go

controllers/controller/devworkspacerouting/solvers/common_test.go

AObuchow · 2023-03-21T19:42:43Z

Also @gbonnefille please sign-off your commits (you can fix this by doing git commit --amend --signoff for your existing commit, and git commit -m "[commit message] --signoff for future commits)

gbonnefille · 2023-03-22T09:23:42Z

@AObuchow thanks a lot for this detailed review and all the suggestions.

Initially, I'm just interested in presenting a minimal change in order to clearly express my ideas and collecting feedback about this ideas. Now I understand it is a way to go, so I will improve my PR following your suggestions.

Thanks again.

Signed-off-by: Guilhem Bonnefille <guilhem.bonnefille@csgroup.eu>

Co-authored-by: Andrew Obuchowicz <aobuchow@redhat.com> Signed-off-by: Guilhem Bonnefille <guilhem.bonnefille@csgroup.eu>

gbonnefille · 2023-03-22T10:19:04Z

With the changes suggested, I was able to have my own version of devworkspace-operator running, with my changes. And it worked: traefik picked the Ingress and processed it.
I'm really happy.

gbonnefille · 2023-03-22T10:36:57Z

I uploaded changes for generated code and CRDs. But I have to admit my workspace is not fully compliant with the requirements for developping on devworkspace-operator. The suggested make failed. I hope a maintainer will be able to terminate this PR if there is missing pieces of code.

AObuchow · 2023-03-22T14:22:17Z

/ok-to-test

AObuchow · 2023-03-22T14:24:19Z

/test all

AObuchow · 2023-03-24T16:36:54Z

@gbonnefille Glad to hear you got DWO running with your changes and things worked correctly, :) I compiled the image based off of your PR successfully without any issues. We'll wait for @amisevsk's feedback before continuing with this PR for now.

amisevsk

Thank you for the PR @gbonnefille! This is quite a large change for the DevWorkspace Operator.

I think there a few improvements we should make before merging, though they're on the complicated side, so let me know if you would like my help/contribution:

If we're adding Annotations to the devworkspaceoperatorconfig CR, setting the default value should also occur the config package instead of in basic_solver.go (similar to how we manage the initial set up of [Kubernetes|OpenShift]PodSecurityContext -- see pkg/config/defaults.go)
Any routing reconciler related packages should avoid references to the global or workspace-specific configuration in general, for a few reasons
- The basic routing class is not really intended for anything other than development purposes; it offers no authentication and should not be used in production, so writing functionality specifically for it isn't the best approach.
- The routing reconciler is generally assumed to not have access to the DevWorkspace, DevWorkspaceOperatorConfig, or related objects. This general mechanism was chosen to allow other operators to implement routing as they require (since there are too many various configurations). For an example of this, see the Eclipse Che Operator's implementation of a routing reconciler that provides single-host access and auth for workspaces. When working on the dwr controller, we have to assume all we can see is the dwr itself and objects it owns.

For the second point above, I think the best we can do for now is to serialize the routing annotations config to a JSON string and put it in DWO-defined annotation on the DevWorkspaceRouting resource, e.g.

apiVersion: controller.devfile.io/v1alpha1
kind: DevWorkspaceRouting
metadata:
  annotations:
    controller.devfile.io/routing-annotations: '{"kubernetes.io/ingress.class":"nginx","nginx.ingress.kubernetes.io/rewrite-target":"/","nginx.ingress.kubernetes.io/ssl-redirect":"false"}'

The solvers in the DevWorkspaceRouting reconciler could then deserialize this annotation and apply it to ingresses/routes created. The benefits of this approach are

It defines a general way for configuring this routing information through the DevWorkspace Operator for any routingClass (should the implementer choose to use this configuration)
It makes applying workspace-scoped configuration simple, as the resolved configuration is available at the moment the DevWorkspaceRouting is created

amisevsk · 2023-03-28T21:00:31Z

controllers/controller/devworkspacerouting/solvers/common.go

@@ -220,7 +220,7 @@ func getIngressForEndpoint(routingSuffix string, endpoint controllerv1alpha1.End
 			Labels: map[string]string{
 				constants.DevWorkspaceIDLabel: meta.DevWorkspaceId,
 			},
-			Annotations: nginxIngressAnnotations(endpoint.Name),
+			Annotations: createAnnotations(endpoint.Name, annotations, nginxIngressAnnotations),


I'm not sure about this approach; there's a pretty distant link between where annotations is defined and where it's used, and how it's defined completely changes the code significantly further down the call stack. The function createAnnotations behaves completely differently based on whether annotations is an empty map here:

if routingAnnotations == nil || len(routingAnnotations) == 0 { appendMap(annotations, defaultAnnotations) } else { appendMap(annotations, routingAnnotations) }

amisevsk · 2023-03-28T21:19:28Z

controllers/controller/devworkspacerouting/solvers/basic_solver.go

-var nginxIngressAnnotations = func(endpointName string) map[string]string {
-	return map[string]string{
-		"kubernetes.io/ingress.class":                "nginx",
-		"nginx.ingress.kubernetes.io/rewrite-target": "/",
-		"nginx.ingress.kubernetes.io/ssl-redirect":   "false",
+var routeAnnotations = map[string]string{
+	"haproxy.router.openshift.io/rewrite-target": "/",
+}
+
+var nginxIngressAnnotations = map[string]string{
+	"kubernetes.io/ingress.class":                "nginx",
+	"nginx.ingress.kubernetes.io/rewrite-target": "/",
+	"nginx.ingress.kubernetes.io/ssl-redirect":   "false",
+}
+
+func createAnnotations(endpointName string, routingAnnotations map[string]string, defaultAnnotations map[string]string) map[string]string {
+	annotations := map[string]string{
 		constants.DevWorkspaceEndpointNameAnnotation: endpointName,
 	}
+
+	if routingAnnotations == nil || len(routingAnnotations) == 0 {
+		appendMap(annotations, defaultAnnotations)
+	} else {
+		appendMap(annotations, routingAnnotations)
+	}
+	return annotations


A better approach for setting a default value if the configuration is unspecified would be similar to what we do in the config package -- this avoids needing functions that take an optional value and a default.

amisevsk · 2023-03-28T21:30:24Z

controllers/controller/devworkspacerouting/solvers/basic_solver.go

+	// TODO: Use workspace-scoped routing annotations to allow overriding
+	routingAnnotations := config.GetGlobalConfig().Routing.Annotations


It will likely not be possible to get workspace-scoped annotations, as this is within the context of the DevWorkspaceRouting reconciler. In fact, we should avoid using config within controllers/controller/devworkspacerouting/... in general, as the routing reconciler may be run outside the DWO context (as it is in Eclipse Che, for example).

In an ideal world, the configuration of DWO is propagated into the DevWorkspaceRouting CR directly in some way. However, some choices made in the past make this very difficult -- likely the only way to do this is to serialize the map to JSON and add it as an annotation on the routing CR.

amisevsk · 2023-03-28T21:52:38Z

apis/controller/v1alpha1/devworkspaceoperatorconfig_types.go

+	//
+	// Use this property to set the annotations expected by the routing framework used
+	// in your cluster (nginx, traefik, ...)
+	Annotations map[string]string `json:"annotations,omitempty"`


We might need to find a more descriptive name here -- perhaps RoutingAnnotations? I feel that annotations is too generic here (though I'm open to being swayed on this one).

gbonnefille · 2023-04-11T08:04:02Z

Thanks @amisevsk for your review. I understand now how naive my proposal was.

One more naive question: is it really mandatory to serialize the annotations in another annotation? What about simply setting annotations on the DevWorkspaceRouting and then having the controller to copy all the annotations of the DevWorkspaceRouting to the effective object (Route/Ingress)? In think this is already how other controllers do, for example Deployment's annotations are copied to the underlying Pod.

amisevsk · 2023-04-11T15:45:01Z

@gbonnefille Your PR isn't naive -- I had completely forgotten these details until I went to review it :). It's complicated since DevWorkspaceRoutings are typically reconciled by other operators so we have to be more careful.

What about simply setting annotations on the DevWorkspaceRouting and then having the controller to copy all the annotations of the DevWorkspaceRouting to the effective object (Route/Ingress)?

I have some concerns with this approach, though it is much cleaner. The main issues I see are

It constrains how DWO can use annotations on DevWorkspaceRouting objects, as it's assumed that all annotations will be copied. This could cause issues down the line if we want to set an annotation that isn't included in routing objects. For example, DWO supports copying special annotations to the DWR from the DevWorkspace (see PR Add support for configuring the workspace routing controllers through annotations on DevWorkspace. #245)
It's less clear how annotations get applied (do they apply to services as well as ingresses/routes?). In the future, there may be a use case for specifying both "annotations for route/ingress" and "annotations for the service", and copying all annotations interferes with this.

Basically, using one annotation to group everything is the safer (and uglier) option. I'm still open to doing it this way, but we would need to consider the implications more than a simple scoped annotation.

openshift-merge-robot · 2023-07-13T17:02:59Z

PR needs rebase.

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.

openshift-ci · 2024-01-15T18:35:55Z

@gbonnefille: The following tests 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-devworkspace-operator-e2e	`e28c740`	link	true	`/test v14-devworkspace-operator-e2e`
ci/prow/v14-images	`e28c740`	link	true	`/test v14-images`

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

gbonnefille requested review from amisevsk and ibuziuk as code owners March 21, 2023 17:38

openshift-ci bot added the needs-ok-to-test label Mar 21, 2023

AObuchow requested changes Mar 21, 2023

View reviewed changes

openshift-ci bot assigned AObuchow Mar 21, 2023

gbonnefille changed the title ~~allow to configure specific annotations~~ WIP: allow to configure specific annotations Mar 22, 2023

openshift-ci bot added the do-not-merge/work-in-progress label Mar 22, 2023

gbonnefille and others added 5 commits March 22, 2023 11:15

feat: allow to configure specific annotations

1000cd3

Signed-off-by: Guilhem Bonnefille <guilhem.bonnefille@csgroup.eu>

fix: sync the routing.annotations property

b5023ec

Signed-off-by: Guilhem Bonnefille <guilhem.bonnefille@csgroup.eu>

fix: wording

4321e9c

Signed-off-by: Guilhem Bonnefille <guilhem.bonnefille@csgroup.eu>

doc: TODO

240e0c9

Co-authored-by: Andrew Obuchowicz <aobuchow@redhat.com> Signed-off-by: Guilhem Bonnefille <guilhem.bonnefille@csgroup.eu>

fix: improve tests

a8992c0

Co-authored-by: Andrew Obuchowicz <aobuchow@redhat.com> Signed-off-by: Guilhem Bonnefille <guilhem.bonnefille@csgroup.eu>

gbonnefille force-pushed the feat/configure-routing-annotations branch from 96d8bbd to a8992c0 Compare March 22, 2023 10:15

fix: regenerate code and CRDs

e28c740

gbonnefille changed the title ~~WIP: allow to configure specific annotations~~ allow to configure specific annotations Mar 22, 2023

openshift-ci bot removed the do-not-merge/work-in-progress label Mar 22, 2023

openshift-ci bot added ok-to-test and removed needs-ok-to-test labels Mar 22, 2023

amisevsk reviewed Mar 28, 2023

View reviewed changes

openshift-merge-robot added the needs-rebase label Jul 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

allow to configure specific annotations #1070

allow to configure specific annotations #1070

gbonnefille commented Mar 21, 2023

openshift-ci bot commented Mar 21, 2023

openshift-ci bot commented Mar 21, 2023

AObuchow left a comment

AObuchow commented Mar 21, 2023

gbonnefille commented Mar 22, 2023

gbonnefille commented Mar 22, 2023

gbonnefille commented Mar 22, 2023

AObuchow commented Mar 22, 2023

AObuchow commented Mar 22, 2023

AObuchow commented Mar 24, 2023

amisevsk left a comment

amisevsk Mar 28, 2023

amisevsk Mar 28, 2023

amisevsk Mar 28, 2023

amisevsk Mar 28, 2023

gbonnefille commented Apr 11, 2023

amisevsk commented Apr 11, 2023

openshift-merge-robot commented Jul 13, 2023

openshift-ci bot commented Jan 15, 2024

		// TODO: Use workspace-scoped routing annotations to allow overriding
		routingAnnotations := config.GetGlobalConfig().Routing.Annotations

allow to configure specific annotations #1070

Are you sure you want to change the base?

allow to configure specific annotations #1070

Conversation

gbonnefille commented Mar 21, 2023

What does this PR do?

What issues does this PR fix or reference?

Is it tested? How?

PR Checklist

openshift-ci bot commented Mar 21, 2023

openshift-ci bot commented Mar 21, 2023

AObuchow left a comment

Choose a reason for hiding this comment

AObuchow commented Mar 21, 2023

gbonnefille commented Mar 22, 2023

gbonnefille commented Mar 22, 2023

gbonnefille commented Mar 22, 2023

AObuchow commented Mar 22, 2023

AObuchow commented Mar 22, 2023

AObuchow commented Mar 24, 2023

amisevsk left a comment

Choose a reason for hiding this comment

amisevsk Mar 28, 2023

Choose a reason for hiding this comment

amisevsk Mar 28, 2023

Choose a reason for hiding this comment

amisevsk Mar 28, 2023

Choose a reason for hiding this comment

amisevsk Mar 28, 2023

Choose a reason for hiding this comment

gbonnefille commented Apr 11, 2023

amisevsk commented Apr 11, 2023

openshift-merge-robot commented Jul 13, 2023

openshift-ci bot commented Jan 15, 2024