Plugin flattening #240

amisevsk · 2021-01-12T19:03:35Z

(Don't worry about line count, 1100 lines are from test cases and 400 are from new devworkspace samples)

What does this PR do?

Readds support for Plugins in DevWorkspaces by implementing basic plugin resolution and merging.

Commits:

Add a basic implementation of plugins based on k8s NamespacedName references, as described in the devfile API spec.
Use this new implementation to resolve plugins in the main reconcile loop (could potentially be moved to a subresource in the future)
Some minor fixups to the reconcile loop (since we now have to be careful to not update the workspace accidentally)
Add samples to support new functionality (samples/plugins contains plugin definitions; samples/with-k8s-ref contains devworkspaces that reference these plugins)
Add support for plugin overriding (components and commands)
Add checking for plugin import cycles (plugin a -> plugin b -> plugin a) to avoid killing the controller due to stack overflows
Add really basic support for plugin and editor compatibility. We add labels to plugins when we create them and check those labels to report when a devworkspace uses incompatible plugins and editors
Add test cases for the library functions (97% coverage)

In a couple of cases, I had to work around devfile/api issues: devfile/api#296 and devfile/api#295; these are referenced in the code with TODOs.

Hopefully commit messages are clear enough to explain the purpose of each change.

What issues does this PR fix or reference?

#249; also a followup to #237 and issue #185

Is it tested? How?

This PR adds a makefile rule install_plugin_templates, which deploys new DevWorkspaceTemplates to the devworkspace-plugins namespace (namespace is hardcoded so we can easily refer to them). After this is done, DevWorkspaces can be deployed with plugins included from the samples/with-k8s-ref/ directory.

TODO:

Reimplement defaulting for the web-terminal container component
Re-enable default Web Terminal DW in e2e tests.
Merge Implement support for Volume components #237 and rebase this PR

openshift-ci-robot · 2021-01-12T19:03:37Z

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

sleshchenko

Amazing work. Seems you did it faster than I need to review )
left some minor comments and will continue to review

samples/with-k8s-ref/theia-next.yaml

samples/with-k8s-ref/theia-nodejs.yaml

Makefile

sleshchenko

Seems to work as expected.

We need to agree on the plan regards devfile's plugin registry, if we are going to support them or not (ones we're able to reference with ID, as we used to do).
if no - there is no a reason to keep it in the model.
Actually URI could work with the registry as well, it's just a question if we need some defaulting for the registry or not.

pkg/library/flatten/flatten.go

pkg/library/flatten/compatibility.go

amisevsk · 2021-01-15T16:24:02Z

We need to agree on the plan regards devfile's plugin registry, if we are going to support them or not (ones we're able to reference with ID, as we used to do).

I agree -- my current understanding is that ID would be used in reference to a devfile/registry which is a devfile/api-side spec. I think it's easy enough to support that we shouldn't worry about getting rid of it, especially for backwards compatibility purposes (WTO uses plugin IDs)

@sleshchenko WDYT about:

ID specified without plugin.registryURL: check "internal" registry (allows us to still support WTO use case)
ID specified with plugin.registryURL: check registry at registryURL and assume it follows devfile/registry spec

amisevsk · 2021-01-15T20:15:37Z

I re-squashed and PR #237 and rebased onto it, which broke any commit refs, sorry for any confusion.

Add a library function ResolveDevWorkspace, which is intended to manage flattening of DevWorkspaces with parents and plugins. An initial, basic implementation supports only flattening plugins defined via kubernetes reference. Contains a multitude of TODOs for next steps. Signed-off-by: Angel Misevski <amisevsk@redhat.com>

Temporarily use the flatten library in the main reconcile loop for DevWorkspaces; this should eventually be moved to a subcontroller. Signed-off-by: Angel Misevski <amisevsk@redhat.com>

Since the reconcile loop has to work on the flattened devworkspace contents while updating annotations on the original object, some care is required. To this end: * Make a copy of the original devworkspace from the cluster, which is the only object to be updated * Rework startup timing functionality to not involve directly modifying the devworkspace resource; instead use a map that we merge into the (cluster) workspace annotations only when needed. Signed-off-by: Angel Misevski <amisevsk@redhat.com>

* Add samples/plugins directory containing DevWorkspaceTemplates defining plugins used in sample devfiles * Add Makefile rule install_plugin_templates which deploys above templates to devworkspace-plugins namespace * Add samples/with-k8s-ref directory containing DevWorkspace samples that reference plugins via DevWorkspaceTemplate Signed-off-by: Angel Misevski <amisevsk@redhat.com>

Devfile API merge function panics if parent content is nil Signed-off-by: Angel Misevski <amisevsk@redhat.com>

Signed-off-by: Angel Misevski <amisevsk@redhat.com>

Work around panic when trying to use plugin overrides to override a devworkspace spec in devfile API. Ref: devfile/api#296 Signed-off-by: Angel Misevski <amisevsk@redhat.com>

* Add functionality for detecting reference cycles in plugins (e.g. plugin A references plugin B which references plugin A), avoiding the potential to lock the operator * Add basic functionality for supporting compatibility between plugins and editors, based on annotations applied to DevWorkspaceTemplates representing plugins; some work would be required for similar functionality for plugins specified by e.g. ID. This is done by building a tree of plugin information as we flatten plugin imports; each node in the tree stores the information about the plugin, a node representing the component that imported it, and a list of nodes representing plugins it imports. We can walk this tree to check e.g. plugin labels for all plugins included in the workspace (to check compatiblity) or to look for import loops (by following parent references). Signed-off-by: Angel Misevski <amisevsk@redhat.com>

Signed-off-by: Angel Misevski <amisevsk@redhat.com>

This functionality is required for e.g. Theia to be able to create DevWorkspaceTemplates and import them into the current workspace. Signed-off-by: Angel Misevski <amisevsk@redhat.com>

If a DevWorkspace specifies a plugin via Kubernetes reference and does not include a namespace, look in the DevWorkspace's namespace by default. Signed-off-by: Angel Misevski <amisevsk@redhat.com>

Configure the DevWorkspace controller as an owner of DevWorkspaceTemplates, in order to trigger reconciles when DevWorkspaceTemplates owned by DevWorkspaces are updated. Signed-off-by: Angel Misevski <amisevsk@redhat.com>

Signed-off-by: Angel Misevski <amisevsk@redhat.com>

amisevsk · 2021-01-21T22:06:06Z

#237 is merged and this PR has been rebased onto the new master branch.

Feature needs more time in the oven -- especially around how annotations/metadata is used, or whether this is something that should be implemented in the controller at all. Signed-off-by: Angel Misevski <amisevsk@redhat.com>

Move remote-runtime-injector and vsx-installer to separate plugins that are imported by theia, rather than having a huge theia plugin definition Signed-off-by: Angel Misevski <amisevsk@redhat.com>

Signed-off-by: Angel Misevski <amisevsk@redhat.com>

sleshchenko · 2021-02-03T10:40:10Z

Makefile

@@ -199,6 +199,11 @@ endif
 	mv config/base/config.properties.bak config/base/config.properties
 	mv config/base/manager_image_patch.yaml.bak config/base/manager_image_patch.yaml

+### install_plugin_templates: Deploy sample plugin templates to namespace devworkspace-plugins:
+install_plugin_templates: _print_vars


I'm OK with it as temporary solution until Theia did not provide DWT for its plugins. But generally I'm not sure we should have a dedicated namespace for devworkspace-plugins, and to avoid security issues we should disable cross-namespace DTW usage at this point.

Yeah, this is a development feature, since we need something to replace plugin registry until something else is implemented. this rule deploys sample plugins that are compatible with devfile samples.

As for cross-namespace dwt use, I'm not sure I see the security risk necessarily. We end up running into an issue of cleanup if we're spreading dwts across the cluster, for statically created resources as we currently use.

sleshchenko

Approving it, since I'm OK with the proposed devworkspace startup flow.
Did not re-review or retest, but everything was fine I tried it.

The TODOs are listed in #249 as separate items and for me it's OK to handle them in further PRs.

openshift-ci-robot · 2021-02-03T13:44:24Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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,sleshchenko]

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

amisevsk · 2021-02-03T16:22:19Z

/test v5-devworkspaces-operator-e2e

amisevsk · 2021-02-03T16:23:47Z

Reminding myself of why those TODOs are there (a couple of weeks later :)) -- I think it makes sense to do that as a separate PR. This PR implements "plugins via kubernetes reference" but WTO depends on "plugins via plugin ID". I don't expect it to be too complicated but it's probably cleaner than adding to this already-large PR.

I'll retest everything on minikube and crc and merge once all tests are passing.

amisevsk · 2021-02-03T19:22:20Z

Tested current changes:

Platform	Workspace	Works?	Notes
Kubernetes	with-k8s-ref/web-terminal-dev.yaml	Starts, no UI	Expected since on k8s web terminal is not supported
	with-k8s-ref/theia-next.yaml	Works	Note this works with Theia importing its own plugins (e.g. vsx-installer)
	with-k8s-ref/theia-nodejs.yaml	Works
OpenShift	with-k8s-ref/web-terminal-custom-tooling.yaml	Starts, unable to start web terminal due to error: `Error Loading OpenShift command line terminal: Could not retrieve $KUBECONFIG` (see below)	Note: the custom tooling variant is required since we don't have defaulting for the dev container
	with-k8s-ref/theia-next.yaml	Works
	with-k8s-ref/theia-nodejs.yaml	Works, sort of	Using tasks fails with `Error launching task 'download-dependencies': Request runTask failed with message: Failed to execute Che command: self signed certificate in certificate chain`

The default use case for web terminal on OpenShift fails because the web-terminal plugin tries to read $KUBECONFIG from the first container in the pod. This functionality depends on the tooling container being first in the list, and fails for the current PR as web-terminal ends up being the first container due to the way merging works in devfile/api. I'm not blocking merge of this PR as-is because this failure mode should be handled in web-terminal, IMO. I've created #258 to track this

amisevsk · 2021-02-03T19:25:56Z

Merging PR as-is to unblock other work -- it seems fairly stable with known issues.

amisevsk requested review from davidfestal, sleshchenko and JPinkney January 12, 2021 19:03

openshift-ci-robot added do-not-merge/work-in-progress approved labels Jan 12, 2021

sleshchenko reviewed Jan 13, 2021

View reviewed changes

samples/with-k8s-ref/theia-next.yaml Outdated Show resolved Hide resolved

samples/with-k8s-ref/theia-nodejs.yaml Show resolved Hide resolved

Makefile Show resolved Hide resolved

sleshchenko reviewed Jan 15, 2021

View reviewed changes

pkg/library/flatten/flatten.go Show resolved Hide resolved

pkg/library/flatten/compatibility.go Outdated Show resolved Hide resolved

amisevsk force-pushed the plugin-flattening branch 2 times, most recently from b7ea6a1 to 39cb7a8 Compare January 15, 2021 20:15

amisevsk force-pushed the plugin-flattening branch from 39cb7a8 to 57a6335 Compare January 15, 2021 20:42

This was referenced Jan 19, 2021

Implement support for Volume components #237

Merged

Implement plugin flattening for DevWorkspaces #249

Closed

amisevsk added 13 commits January 21, 2021 17:01

Use flatten library in main reconcile loop for DevWorkspace

e4e2bb8

Temporarily use the flatten library in the main reconcile loop for DevWorkspaces; this should eventually be moved to a subcontroller. Signed-off-by: Angel Misevski <amisevsk@redhat.com>

Add workaround for devfile/api issue

e735b8a

Devfile API merge function panics if parent content is nil Signed-off-by: Angel Misevski <amisevsk@redhat.com>

Add support for plugin overrides in library

99f13bc

Signed-off-by: Angel Misevski <amisevsk@redhat.com>

Workaround panic error in devfile/api override code

bb2237f

Work around panic when trying to use plugin overrides to override a devworkspace spec in devfile API. Ref: devfile/api#296 Signed-off-by: Angel Misevski <amisevsk@redhat.com>

Add test cases for plugin resolution in library code.

01c479d

Signed-off-by: Angel Misevski <amisevsk@redhat.com>

Add DevWorkspaceTemplates get/create/update to workspace RBAC

83f5133

This functionality is required for e.g. Theia to be able to create DevWorkspaceTemplates and import them into the current workspace. Signed-off-by: Angel Misevski <amisevsk@redhat.com>

Look for plugins in the current namespace if namespace is empty

9e01806

If a DevWorkspace specifies a plugin via Kubernetes reference and does not include a namespace, look in the DevWorkspace's namespace by default. Signed-off-by: Angel Misevski <amisevsk@redhat.com>

Make DevWorkspace controller own DevWorkspaceTemplates

037b982

Configure the DevWorkspace controller as an owner of DevWorkspaceTemplates, in order to trigger reconciles when DevWorkspaceTemplates owned by DevWorkspaces are updated. Signed-off-by: Angel Misevski <amisevsk@redhat.com>

Add DevWorkspaceTemplates to aggregate view and edit roles

c2ae3ed

Signed-off-by: Angel Misevski <amisevsk@redhat.com>

amisevsk force-pushed the plugin-flattening branch from 57a6335 to c2ae3ed Compare January 21, 2021 22:02

amisevsk marked this pull request as ready for review January 21, 2021 22:19

openshift-ci-robot removed the do-not-merge/work-in-progress label Jan 21, 2021

amisevsk added 2 commits January 26, 2021 12:38

Remove proof-of-concept for plugin/editor compatibility

4eb99ea

Feature needs more time in the oven -- especially around how annotations/metadata is used, or whether this is something that should be implemented in the controller at all. Signed-off-by: Angel Misevski <amisevsk@redhat.com>

Rework plugin samples to separate components better

e6cf53a

Move remote-runtime-injector and vsx-installer to separate plugins that are imported by theia, rather than having a huge theia plugin definition Signed-off-by: Angel Misevski <amisevsk@redhat.com>

amisevsk force-pushed the plugin-flattening branch from 2ccc23f to e6cf53a Compare January 26, 2021 17:47

amisevsk mentioned this pull request Jan 27, 2021

Remove plugin registry #252

Merged

l0rd mentioned this pull request Feb 1, 2021

Adopt DevWorkspace - a simpler and more lightweight kubernetes-native architecture, and the Devfile v2 specification for increased interoperability eclipse-che/che#17307

Closed

Move test cases related to compatibility out of test suite

50eac65

Signed-off-by: Angel Misevski <amisevsk@redhat.com>

sleshchenko reviewed Feb 3, 2021

View reviewed changes

sleshchenko approved these changes Feb 3, 2021

View reviewed changes

openshift-ci-robot assigned sleshchenko Feb 3, 2021

openshift-ci-robot added the lgtm label Feb 3, 2021

amisevsk mentioned this pull request Feb 3, 2021

Web Terminal Tooling depends on injecting kubeconfig into the first available container #258

Closed

amisevsk merged commit 7a34f21 into devfile:master Feb 3, 2021

amisevsk mentioned this pull request Mar 19, 2021

Add library package for resolving devfile parents and plugins devfile/library#74

Closed

amisevsk deleted the plugin-flattening branch February 8, 2023 15:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Plugin flattening #240

Plugin flattening #240

amisevsk commented Jan 12, 2021 •

edited

Loading

openshift-ci-robot commented Jan 12, 2021

sleshchenko left a comment •

edited

Loading

sleshchenko left a comment

amisevsk commented Jan 15, 2021

amisevsk commented Jan 15, 2021

amisevsk commented Jan 21, 2021

sleshchenko Feb 3, 2021

amisevsk Feb 3, 2021

amisevsk Feb 3, 2021

sleshchenko left a comment

openshift-ci-robot commented Feb 3, 2021

amisevsk commented Feb 3, 2021

amisevsk commented Feb 3, 2021

amisevsk commented Feb 3, 2021

amisevsk commented Feb 3, 2021

Plugin flattening #240

Plugin flattening #240

Conversation

amisevsk commented Jan 12, 2021 • edited Loading

What does this PR do?

What issues does this PR fix or reference?

Is it tested? How?

TODO:

openshift-ci-robot commented Jan 12, 2021

sleshchenko left a comment • edited Loading

Choose a reason for hiding this comment

sleshchenko left a comment

Choose a reason for hiding this comment

amisevsk commented Jan 15, 2021

amisevsk commented Jan 15, 2021

amisevsk commented Jan 21, 2021

sleshchenko Feb 3, 2021

Choose a reason for hiding this comment

amisevsk Feb 3, 2021

Choose a reason for hiding this comment

amisevsk Feb 3, 2021

Choose a reason for hiding this comment

sleshchenko left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented Feb 3, 2021

amisevsk commented Feb 3, 2021

amisevsk commented Feb 3, 2021

amisevsk commented Feb 3, 2021

amisevsk commented Feb 3, 2021

amisevsk commented Jan 12, 2021 •

edited

Loading

sleshchenko left a comment •

edited

Loading