Don't fail workspaces that use Kubernetes/OpenShift components with URIs #1104

amisevsk · 2023-05-19T17:06:03Z

What does this PR do?

Update how Kubernetes/OpenShift components with URIs are handled:

If a component has deployByDefault: false, it is ignored and workspace starts normally (the component is expected to be applied later, e.g. via an apply command
If a component has deployByDefault: true, it is not applied to the cluster if it uses a URI, and instead the workspace gets a warning condition. This effectively downgrades the webhook rejection to a warning condition

What issues does this PR fix or reference?

Is it tested? How?

Apply the DevWorkspace

apiVersion: workspace.devfile.io/v1alpha2
kind: DevWorkspace
metadata:
  name: plain
spec:
  routingClass: basic
  started: true
  template:
    attributes:
      controller.devfile.io/storage-type: ephemeral
    components:
    - name: web-terminal
      container:
        command: ["tail", "-f", "/dev/null"]
        image: quay.io/amisevsk/web-terminal-tooling:dev
        memoryLimit: 512Mi
        mountSources: true
    - name: test-k8s
      kubernetes: 
        deployByDefault: false
        uri: https://raw.githubusercontent.com/devfile/devworkspace-operator/main/deploy/deployment/kubernetes/objects/devworkspace-controller-metrics.Service.yaml

Update the workspace to set deployByDefault:

If false, workspace should start normally, without any issues or warnings
If true, the workspace should still start, but have a warning condition:

Ignored components that use unsupported features: component test-k8s
defines a Kubernetes/OpenShift component via URI

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

AObuchow

Code looks good to me and works as expected on OpenShift 4.12.
Minor comment regarding documentation, but LGTM

AObuchow · 2023-05-19T18:46:41Z

pkg/library/kubernetes/provision.go

@@ -34,11 +35,16 @@ import (
 // Only Kubernetes/OpenShift components that are inlined are supported; components that define
 // a URI will cause a FailError to be returned


Documentation should be changed to ...a URI will cause a WarningError to be returned

AObuchow · 2023-05-19T18:48:03Z

webhook/workspace/handler/kubernetes.go

@@ -42,7 +42,8 @@ func (h *WebhookHandler) validateKubernetesObjectPermissionsOnCreate(ctx context
 			continue
 		}
 		if component.Uri != "" {
-			return fmt.Errorf("kubenetes components specified via URI are unsupported")
+			// We're going to ignore URI components for now


A future improvement could be to return a webhook warning (so that the dashboard could display it) but for now, I don't think this is necessary.

codecov · 2023-05-19T18:56:44Z

Codecov Report

Patch coverage: 62.50% and project coverage change: -0.02 ⚠️

Comparison is base (89370ce) 51.64% compared to head (0319e70) 51.62%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1104      +/-   ##
==========================================
- Coverage   51.64%   51.62%   -0.02%     
==========================================
  Files          79       79              
  Lines        7285     7299      +14     
==========================================
+ Hits         3762     3768       +6     
- Misses       3237     3243       +6     
- Partials      286      288       +2

Impacted Files	Coverage Δ
webhook/workspace/handler/kubernetes.go	`0.00% <0.00%> (ø)`
pkg/library/kubernetes/provision.go	`72.50% <54.54%> (-7.79%)`	⬇️
pkg/library/kubernetes/util.go	`95.55% <100.00%> (+0.55%)`	⬆️

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

Ignore Kubernetes/OpenShift-type components that define a URI, allowing such workspaces to start successfully. Instead of failing startup, add a warning to the workspace to notify the user. This helps us give at least partial compatibility with some standard devfile samples, which use currently use URIs. With this change, the workspace will at least be started (or, more accurately, partially started). Signed-off-by: Angel Misevski <amisevsk@redhat.com>

Update the webhook server to ignore Kubernetes and OpenShift components that use URIs with deployByDefault: true. Instead these components will be ignored by DWO and a warning should be added to the workspace. Signed-off-by: Angel Misevski <amisevsk@redhat.com>

amisevsk · 2023-05-19T21:09:56Z

/retest

l0rd · 2023-05-22T09:23:49Z

@amisevsk do you have an already built image to test this PR?

openshift-ci · 2023-05-23T10:53:06Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: amisevsk, AObuchow, ibuziuk

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

l0rd · 2023-05-23T14:00:02Z

I have tested with https://github.com/devfile-samples/devfile-sample-go-basic.git and I was able to start the workspace successfully.

The weird thing is that my workspace pod has 3 containers (but it may be related to a dashboard issue that ignores the parent and adds a default component):

NOTE: I have tested using Dev Spaces 3.6

amisevsk · 2023-05-23T16:54:31Z

do you have an already built image to test this PR?

I didn't build a specific image for this PR, so it was only pushed to quay.io/amisevsk/devworkspace-controller:dev (which is the image I use for whatever I'm working on).

The weird thing is that my workspace pod has 3 containers (but it may be related to a dashboard issue that ignores the parent and adds a default component):

This should be reported as an issue on the Eclipse Che side -- it looks like something is going wrong with merging components. The runtime component is coming from the parent, but since #993 the che-code-runtime-description component should have been merged into it automatically (i.e. even without the explicit merge-components attribute). I tried briefly to reproduce the issue but would need to roll changes from this PR out to test properly.

l0rd · 2023-05-23T17:07:47Z

I opened the parent issue a few weeks ago and it has been fixed Che-side but not back ported to 3.6. So yes, probably related to that.

amisevsk requested review from l0rd and AObuchow May 19, 2023 17:06

amisevsk requested a review from ibuziuk as a code owner May 19, 2023 17:06

openshift-ci bot added the approved label May 19, 2023

AObuchow approved these changes May 19, 2023

View reviewed changes

openshift-ci bot assigned AObuchow May 19, 2023

openshift-ci bot added the lgtm label May 19, 2023

amisevsk added 2 commits May 19, 2023 15:02

amisevsk force-pushed the kubernetes-components-uri branch from 6930146 to 0319e70 Compare May 19, 2023 19:02

openshift-ci bot removed the lgtm label May 19, 2023

AObuchow approved these changes May 19, 2023

View reviewed changes

openshift-ci bot added the lgtm label May 19, 2023

ibuziuk approved these changes May 23, 2023

View reviewed changes

openshift-ci bot assigned ibuziuk May 23, 2023

amisevsk merged commit 636b1a5 into devfile:main May 23, 2023

amisevsk deleted the kubernetes-components-uri branch May 23, 2023 16:55

amisevsk mentioned this pull request May 23, 2023

[backport-0.21.x] Don't fail workspaces if they have kube components that use URI #1107

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't fail workspaces that use Kubernetes/OpenShift components with URIs #1104

Don't fail workspaces that use Kubernetes/OpenShift components with URIs #1104

amisevsk commented May 19, 2023 •

edited

Loading

AObuchow left a comment

AObuchow May 19, 2023 •

edited

Loading

AObuchow May 19, 2023

codecov bot commented May 19, 2023 •

edited

Loading

amisevsk commented May 19, 2023

l0rd commented May 22, 2023

openshift-ci bot commented May 23, 2023

l0rd commented May 23, 2023 •

edited

Loading

amisevsk commented May 23, 2023

l0rd commented May 23, 2023

		@@ -34,11 +35,16 @@ import (
		// Only Kubernetes/OpenShift components that are inlined are supported; components that define
		// a URI will cause a FailError to be returned

Don't fail workspaces that use Kubernetes/OpenShift components with URIs #1104

Don't fail workspaces that use Kubernetes/OpenShift components with URIs #1104

Conversation

amisevsk commented May 19, 2023 • edited Loading

What does this PR do?

What issues does this PR fix or reference?

Is it tested? How?

PR Checklist

AObuchow left a comment

Choose a reason for hiding this comment

AObuchow May 19, 2023 • edited Loading

Choose a reason for hiding this comment

AObuchow May 19, 2023

Choose a reason for hiding this comment

codecov bot commented May 19, 2023 • edited Loading

Codecov Report

amisevsk commented May 19, 2023

l0rd commented May 22, 2023

openshift-ci bot commented May 23, 2023

l0rd commented May 23, 2023 • edited Loading

amisevsk commented May 23, 2023

l0rd commented May 23, 2023

amisevsk commented May 19, 2023 •

edited

Loading

AObuchow May 19, 2023 •

edited

Loading

codecov bot commented May 19, 2023 •

edited

Loading

l0rd commented May 23, 2023 •

edited

Loading