Migrate to devfile/api v1alpha2 and add conversion webhooks. #197

amisevsk · 2020-11-17T21:47:12Z

What does this PR do?

Switches the devworkspace-operator to use v1alpha2 devworkspaces, and implements conversion webhooks. Currently this is done by updating go.mod to point at devfile/api#213, as that PR is not yet merged.

To avoid too many temporary changes in the repo (changing where devworkspace CRDs are downloaded from), you'll have to manually copy in the CRDs generated in devfile/api#213 to config/crd/bases/:

curl "https://raw.githubusercontent.com/devfile/api/fc53138e/crds/workspace.devfile.io_devworkspaces.yaml" \
    -o config/crd/bases/workspace.devfile.io_devworkspaces.yaml

curl "https://raw.githubusercontent.com/devfile/api/fc53138e/crds/workspace.devfile.io_devworkspacetemplates.yaml" \
    -o config/crd/bases/workspace.devfile.io_devworkspacetemplates.yaml

What issues does this PR fix or reference?

Resolves #189

Is it tested? How?

To test this PR (on minikube, for now):
0. Set env vars as usual for development

Deploy cert-manager on your cluster: make install_cert_manager
Deploy controller: make docker install
Verify that conversion webhooks are created: log message conversion webhook enabled appears in kubectl logs -f deploy/devworkspace-webhook-server
Create a DevWorkspace: kubectl apply -f samples/theia.yaml

Check that you can get both v1alpha1 and v1alpha2 versions of the devworkspace:

kubectl get devworkspaces.v1alpha1.workspace.devfile.io theia -o yaml
kubectl get devworkspaces.v1alpha2.workspace.devfile.io theia -o yaml

Test usability as normal

Remaining TODOs

I haven't tested/verified that local run/debug still works as expected, but will do that soon.
These changes make webhooks mandatory; we likely need to update how WEBHOOKS_ENABLED works, perhaps disabling the mutating and validating webhooks when set to false but still deploying the webhooks server
The service name for the webhooks server is hard-coded, so it has to be hard-coded into templates as well, we should probably make this configurable
Only k8s with cert-manager is supported at this point. The next step is to add support for OpenShift via the Service CA operator, which supports a similar CRD annotation. We'll need to verify that the certificates secret is correctly propagated to the webhooks server
This PR includes a workaround for deploying CRDs when they're already present on the cluster (we first remove them). I've found that on minikube, without those changes, the second make install fails. You can test this by
```
kustomize build config/crd | kubectl apply -f -
kustomize build config/crd | kubectl apply -f -
```
or by commenting out the delete command in make install.
This PR updates existing CRDs for Components and WorkspaceRoutings. In terms of yaml spec they are identical, except the new version has defaults for some endpoint fields. We need to verify that
- Applying a v1 version of the CRD is not an error when a `v1beta1 CRD with the same GVK is present
- That upgrading from a previous version will not cause issues for existing devworkspaces.

Re-add DevWorkspaceTemplates to CRDs and adjust scripts/gitignore to accomodate them. Signed-off-by: Angel Misevski <[email protected]>

JPinkney

Just some initial comments, ~~my CRC was being a pain so I'll have to look further in depth on monday~~. I should have read the description better :)

config/base/manager_image_patch.yaml

config/cert-manager/manager_certmanager_patch.yaml

config/components/certificates/cert-manager/kustomizeconfig.yaml

webhook/workspace/handler/immutable.go

JPinkney · 2020-11-20T21:30:14Z

webhook/main.go

 	if err != nil {
 		log.Error(err, "Failed to create webhooks")
 		os.Exit(1)
 	}

+	if err := ctrl.NewWebhookManagedBy(mgr).For(&workspacev1alpha1.DevWorkspace{}).Complete(); err != nil {
+		log.Error(err, "failed creating conversion webhook")


Just curious, if the conversion webhooks fail to create should we exit or just continue without them?

I believe if the webhooks fail to set up you won't be able to create DevWorkspaces -- when you create a DevWorkspace it calls the conversion webhooks (at least for the v1alpha1 case) and if it can't get a response it blocks creation.

Also surprising info about how conversion works: If you specify conversion: none in the CRD, you can "get" a v1alpha2-spec workspace that is just the v1alpha1 and a wrong GVK 🤷

JPinkney · 2020-11-23T20:59:15Z

I just tried the testing instructions and everything was working as expected. For some reason my make _init_devworkspace_crds wasn't working but I think that was more of a me issue.

I tried the local run/debug and I get:

2020-11-23T15:49:09.458-0500    ERROR   setup   failed to setup webhooks        {"error": "failed to create webhooks deployment: environment variable WEBHOOK_SECRET_NAME is unset"}

Update dependency on devfile/api to commit 1103496. This is required as this commit contains both v1alpha1 and v1alpha2 APIs as well as conversion code. Signed-off-by: Angel Misevski <[email protected]>

* Configure `make manifests` to generate v1 CRDs (instead of v1beta1) to resolve a validation issue (v1beta CRDs cannot define default fields) * Fix patch_crds.sh to patch v1 CRDs instead of v1beta1 * Add rule for restarting the webhooks server * Add rule for installing cert-manager to a cluster Signed-off-by: Angel Misevski <[email protected]>

Update all code to use v1alpha2 instead of v1alpha1. In general this is a straightforward switching of imports, but some changes are required: * Change in devfile structure (move keys up a level) means that some vistior-based code no longer works in the same way (visitor functions cannot get key), so they're reworked into a switch statement * Some methods can now return an error. * Devfile API v1alpha2 removes a duplicated memoryLimit feild from containers, and other minor changes for compatibility. Signed-off-by: Angel Misevski <[email protected]>

Signed-off-by: Angel Misevski <[email protected]>

Clean up unused/autogenerated code from config/ directory. A lot of it won't work for our use case, and some of it is out of date (e.g. v1beta1 cert-manager certificates when v1 is available) Signed-off-by: Angel Misevski <[email protected]>

Signed-off-by: Angel Misevski <[email protected]>

Update separate webhooks server to create conversion webhooks. This means we can no longer generate a certificate at runtime, and must instead use a certificate that is included in the DevWorkspace CRD specs. Signed-off-by: Angel Misevski <[email protected]>

Attempting to apply CRDs when they already exist on the cluster hangs indefinitely for unclear reasons. Signed-off-by: Angel Misevski <[email protected]>

Signed-off-by: Angel Misevski <[email protected]>

v1alpha2 introduces default values for Endpoint.Exposure and Endpoint.Protocol. When left empty, they are populated by the API server which causes spec vs cluster comparisons to always fail. Signed-off-by: Angel Misevski <[email protected]>

Signed-off-by: Angel Misevski <[email protected]>

Fix issue when running locally, where the operator expects environment variable WEBHOOK_SECRET_NAME to be set according to the secret generated by cert-manager. For now, we export a hard-coded secret name in the Makefile when trying to run/debug locally. Signed-off-by: Angel Misevski <[email protected]>

Reflecting devfile/api PR devfile/api#214 Cherry-picked from commit cdef6dc and adapted slightly Signed-off-by: David Festal <[email protected]>

Signed-off-by: David Festal <[email protected]>

Signed-off-by: Angel Misevski <[email protected]>

Signed-off-by: David Festal <[email protected]>

openshift-ci-robot · 2020-11-26T16:03:03Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: amisevsk, JPinkney, 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~~ [JPinkney,amisevsk,sleshchenko]

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

openshift-ci-robot · 2020-11-26T16:03:10Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: amisevsk, JPinkney, 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~~ [JPinkney,amisevsk,sleshchenko]

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

* Move config/components/certificates/cert-manager to /config/components/cert-manager since there's no other files required for deploying certs * Move common labels to last-step kustomizations * Remove namespace from base as it cannot be changed * Add plantuml diagram documenting structure of kustomize directories Signed-off-by: Angel Misevski <[email protected]>

openshift-ci-robot · 2020-11-26T19:11:39Z

New changes are detected. LGTM label has been removed.

Signed-off-by: Angel Misevski <[email protected]>

amisevsk · 2020-11-26T19:21:35Z

Pushed a couple of last-minute fixups:

Removed bundle-related makefile rules, since they don't work with our current structure; we'll have to address this later. Operator SDK-generated manifests expect the default folder structure, which we don't/cannot use and don't include devworkspace CRDs
Reorganized the config/ directory a bit:
- Moved config/components/certificates/cert-manager up to config/components/cert-manager, since I initially thought we might need other types of deployments for certificates (e.g. something for Service CA). Since there's only one subfolder it makes sense to simplify the structure for now
- Removed namespace from config/base as it was hard-coded to devworkspace-operator
- Removed common labels from config/base as they should be applied at the end (otherwise anything not used as a base in config/base will not get the labels
Added a plantuml diagram of the current structure of the config/ directory with some explanation of what each kustomize.yaml does:

amisevsk · 2020-11-26T19:21:56Z

/test v5-devworkspaces-operator-e2e

Signed-off-by: Angel Misevski <[email protected]>

amisevsk · 2020-11-27T01:55:23Z

/test v5-devworkspaces-operator-e2e

amisevsk · 2020-11-27T02:44:42Z

/retest

sleshchenko · 2020-11-27T07:27:30Z

test/e2e/pkg/deploy/controller.go

@@ -64,13 +64,3 @@ func (w *Deployment) DeployWorkspacesController() error {

 	return nil
 }
-
-func (w *Deployment) CustomResourceDefinitions() error {


Is it really removed from Makefile https://github.com/amisevsk/devworkspace-operator/blob/add-conversion-v1alpha2/Makefile ?
Or it just does not make sense to execute it separately since e2e tests do make install, which includes crds installing as well?

sleshchenko · 2020-11-27T07:29:55Z

test is failing because it can't initialize CRDs:

Downloading devfile/api CRDs to /tmp/tmp.9VboUUlNdl
Initialized empty Git repository in /tmp/tmp.9VboUUlNdl/.git/
aeda60d4361911da85103f224644bfa792498499
DevWorkspace API is specified from branch
fatal: reference is not a tree: aeda60d4361911da85103f224644bfa792498499
make[1]: *** [_init_devworkspace_crds] Error 128

sleshchenko · 2020-11-27T10:00:01Z

/test v5-devworkspaces-operator-e2e

sleshchenko · 2020-11-27T12:18:59Z

/test v5-devworkspaces-operator-e2e

sleshchenko · 2020-11-27T12:49:59Z

Something strange happens with tests. I think the PR can be merged without this PR check if make test_e2e works locally

sleshchenko · 2020-11-27T14:00:47Z

/test v5-devworkspaces-operator-e2e

sleshchenko · 2020-11-27T14:52:54Z

Found a bug which may cause tests failure on CI, at least it cause my local failure, controller tries to update components forever because of difference in sourceMapping

See diff from debug logs

{
  "wanted": {
    "spec": {
      "workspaceId": "workspace267da45101b44759",
      "components": [
        {
          "name": "dev",
          "container": {
            "image": "quay.io/wto/web-terminal-tooling:latest",
            "env": [
              {
                "name": "PS1",
                "value": "\\[\\e[34m\\]>\\[\\e[m\\]\\[\\e[33m\\]>\\[\\e[m\\]"
              }
            ],
            "memoryLimit": "256Mi",
            "args": [
              "tail",
              "-f",
              "/dev/null"
            ]
          }
        }
      ]
    }
  },
  "clusterComponent": {
    "spec": {
      "workspaceId": "workspace267da45101b44759",
      "components": [
        {
          "name": "dev",
          "container": {
            "image": "quay.io/wto/web-terminal-tooling:latest",
            "env": [
              {
                "name": "PS1",
                "value": "\\[\\e[34m\\]>\\[\\e[m\\]\\[\\e[33m\\]>\\[\\e[m\\]"
              }
            ],
            "memoryLimit": "256Mi",
            "args": [
              "tail",
              "-f",
              "/dev/null"
            ],
            "sourceMapping": "/projects"
          }
        }
      ]
    }
  }
}

I assume component CRD needs to be updated.

sleshchenko · 2020-11-27T15:16:10Z

/test v5-devworkspaces-operator-e2e

Running an older container (centos:7) causes the update_devworkspace_crds.sh script to fail with error: fatal: reference is not a tree: aeda60d4361911da851 This appears to be due to different behavior for git-fetch in older versions (centos 7 uses 1.8.3.1). Signed-off-by: Angel Misevski <[email protected]>

amisevsk · 2020-11-27T15:54:38Z

/test v5-devworkspaces-operator-e2e

amisevsk · 2020-11-27T17:21:48Z

@sleshchenko I tested again on crc and was able to start up a cloud-shell workspace -- no idea why it's failing in CI now. I'll look into the souremapping issue, it may be causing a problem but it's not clear why.

The devfile/api ContainerComponent spec sets default values, e.g. for sourceMapping. Once a resource is created on the cluster, these defaults will be applied, and so defaulted fields must have a defined value in the static spec we use to create resources -- otherwise the controller should detect a difference and attempt to reconcile it away. Signed-off-by: Angel Misevski <[email protected]>

amisevsk · 2020-11-27T17:26:03Z

/test v5-devworkspaces-operator-e2e

amisevsk · 2020-11-27T18:50:26Z

Merging to unblock all progress on the operator.

Update scripts to grab devfile/api DevWorkspaceTemplates

b62c112

Re-add DevWorkspaceTemplates to CRDs and adjust scripts/gitignore to accomodate them. Signed-off-by: Angel Misevski <[email protected]>

amisevsk requested review from davidfestal, sleshchenko and JPinkney November 17, 2020 21:47

openshift-ci-robot added the approved label Nov 17, 2020

JPinkney reviewed Nov 20, 2020

View reviewed changes

amisevsk and others added 23 commits November 24, 2020 00:04

Update project latest devfile/api

34e8837

Update dependency on devfile/api to commit 1103496. This is required as this commit contains both v1alpha1 and v1alpha2 APIs as well as conversion code. Signed-off-by: Angel Misevski <[email protected]>

Regenerate deepcopy, crds after updating to v1alpha2

db8edf4

Signed-off-by: Angel Misevski <[email protected]>

Add yaml templates for generating certs using cert-manager

b4bcc9e

Signed-off-by: Angel Misevski <[email protected]>

Add templates for deploying controller with cert-manager on k8s

c2e6c41

Signed-off-by: Angel Misevski <[email protected]>

Rename config/devel to config/base

7837690

Signed-off-by: Angel Misevski <[email protected]>

Workaround: uninstall CRDs when running make install

3cd22e7

Attempting to apply CRDs when they already exist on the cluster hangs indefinitely for unclear reasons. Signed-off-by: Angel Misevski <[email protected]>

Make webhooks handle both v1alpha1 and v1alpha2 DevWorkspaces

145e464

Signed-off-by: Angel Misevski <[email protected]>

Fix deployment templates TODOs around namespace and service name

a41126b

Signed-off-by: Angel Misevski <[email protected]>

fixup namespace for certificates

c2ccf22

Add support for Service CA operator on OpenShift

453e5fe

Signed-off-by: Angel Misevski <[email protected]>

fixup go.mod

54f05a4

Adapt code to use new format for Attributes

28a8107

Reflecting devfile/api PR devfile/api#214 Cherry-picked from commit cdef6dc and adapted slightly Signed-off-by: David Festal <[email protected]>

Update the samples to use v1alpha2

4367041

Signed-off-by: David Festal <[email protected]>

Add the all-in-one sample to validate plugins

ee92eb6

Signed-off-by: David Festal <[email protected]>

Use the initContainer library

10f5a7d

Signed-off-by: David Festal <[email protected]>

Update subresource CRDs

51e99fb

Signed-off-by: Angel Misevski <[email protected]>

Adapt to attributes + fix endpoints

8b2dbb8

Signed-off-by: David Festal <[email protected]>

openshift-ci-robot added the lgtm label Nov 26, 2020

openshift-ci-robot removed the lgtm label Nov 26, 2020

Remove rules for bundling from Makefile as they are out of date

d1c3aab

Signed-off-by: Angel Misevski <[email protected]>

Remove installing CRDs from e2e test as that rule has been removed

9485a2b

Signed-off-by: Angel Misevski <[email protected]>

sleshchenko reviewed Nov 27, 2020

View reviewed changes

Fix detecting the type of devfile/api version

e7e788d

sleshchenko force-pushed the add-conversion-v1alpha2 branch from 941d553 to e7e788d Compare November 27, 2020 12:49

amisevsk merged commit b2743e3 into devfile:master Nov 27, 2020

amisevsk deleted the add-conversion-v1alpha2 branch February 8, 2023 15:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate to devfile/api v1alpha2 and add conversion webhooks. #197

Migrate to devfile/api v1alpha2 and add conversion webhooks. #197

amisevsk commented Nov 17, 2020

JPinkney left a comment •

edited

Loading

JPinkney Nov 20, 2020

amisevsk Nov 24, 2020

JPinkney commented Nov 23, 2020

openshift-ci-robot commented Nov 26, 2020

openshift-ci-robot commented Nov 26, 2020

openshift-ci-robot commented Nov 26, 2020

amisevsk commented Nov 26, 2020

amisevsk commented Nov 26, 2020

amisevsk commented Nov 27, 2020

amisevsk commented Nov 27, 2020

sleshchenko Nov 27, 2020

sleshchenko commented Nov 27, 2020

sleshchenko commented Nov 27, 2020

sleshchenko commented Nov 27, 2020

sleshchenko commented Nov 27, 2020 •

edited

Loading

sleshchenko commented Nov 27, 2020

sleshchenko commented Nov 27, 2020

sleshchenko commented Nov 27, 2020

amisevsk commented Nov 27, 2020

amisevsk commented Nov 27, 2020

amisevsk commented Nov 27, 2020

amisevsk commented Nov 27, 2020

Migrate to devfile/api v1alpha2 and add conversion webhooks. #197

Migrate to devfile/api v1alpha2 and add conversion webhooks. #197

Conversation

amisevsk commented Nov 17, 2020

What does this PR do?

What issues does this PR fix or reference?

Is it tested? How?

Remaining TODOs

JPinkney left a comment • edited Loading

Choose a reason for hiding this comment

JPinkney Nov 20, 2020

Choose a reason for hiding this comment

amisevsk Nov 24, 2020

Choose a reason for hiding this comment

JPinkney commented Nov 23, 2020

openshift-ci-robot commented Nov 26, 2020

openshift-ci-robot commented Nov 26, 2020

openshift-ci-robot commented Nov 26, 2020

amisevsk commented Nov 26, 2020

amisevsk commented Nov 26, 2020

amisevsk commented Nov 27, 2020

amisevsk commented Nov 27, 2020

sleshchenko Nov 27, 2020

Choose a reason for hiding this comment

sleshchenko commented Nov 27, 2020

sleshchenko commented Nov 27, 2020

sleshchenko commented Nov 27, 2020

sleshchenko commented Nov 27, 2020 • edited Loading

sleshchenko commented Nov 27, 2020

sleshchenko commented Nov 27, 2020

sleshchenko commented Nov 27, 2020

amisevsk commented Nov 27, 2020

amisevsk commented Nov 27, 2020

amisevsk commented Nov 27, 2020

amisevsk commented Nov 27, 2020

JPinkney left a comment •

edited

Loading

sleshchenko commented Nov 27, 2020 •

edited

Loading