Skip to content

Conversion webhooks in main #474

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 13 commits into from
Jul 8, 2021

Conversation

amisevsk
Copy link
Collaborator

What does this PR do?

Moves the conversion webhooks used for DevWorkspaces and DevWorkspaceTemplates into the main controller deployment rather than the webhooks server. This allows OLM to generate appropriate CRD fields when an operator is installed, and so enables installing the operator to any namespace on the cluster with the appropriate OperatorGroup configuration.

As part of this process, the Operator SDK dependency is updated to v1.8.0, as conversion webhooks support was added after v1.7.2. This change is done in the first commit of this PR and results in a seemingly large line diff, but semantically the files are identical and all line changes are due to line wrapping long strings. This can be tested using e.g.

diff <(yq -Y -S 'walk(if type == "array" then sort else . end)' ${oldfile}) <(yq -Y -S 'walk(if type == "array" then sort else . end)' ${newfile})

Known issues

What issues does this PR fix or reference?

Closes #470

Is it tested? How?

For each of

  1. Kubernetes install
  2. OpenShift install (direct install via yaml -- i.e. make install)
  3. OLM install
    • in the openshift-operators namespace
    • in a different namespace (e.g. devworkspace-operator)

test that

  1. Operator installs and starts correctly
  2. CRD conversion webhooks point at the controller service (get crd devworkspaces.workspace.devfile.io -o yaml | yq '.spec.conversion')
  3. Conversion works:
    • Create a DevWorkspace from sample
    • Read DevWorkspace as v1alpha1: kc get devworkspaces.v1alpha1.workspace.devfile.io -o yaml
    • Read DevWorkspace as v1alpha2: kc get devworkspaces.v1alpha2.workspace.devfile.io -o yaml
    • Easiest way to check that conversion is functioning correctly is to check e.g. plugins -- on v1alpha1, it's .components.plugins[].plugin.name and on v1alpha2 it's .components.plugins[].name.

PR Checklist

  • E2E tests pass (when PR is ready, comment /test v7-devworkspaces-operator-e2e, v7-devworkspace-happy-path to trigger)
    • v7-devworkspaces-operator-e2e: DevWorkspace e2e test
    • v7-devworkspace-happy-path: DevWorkspace e2e test

Copy link
Member

@sleshchenko sleshchenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job. Minor comments are left, some of them are even not really related to the current PR)

@openshift-ci openshift-ci bot removed the lgtm label Jul 2, 2021
amisevsk added 11 commits July 5, 2021 16:43
Update operator-sdk version in makefile to 1.8.0 to support generating
webhooks in OPM bundle.

Note: this commit has a large number of line changes, but all of them
are formatting due to operator-sdk 1.8.0 hard-wrapping long strings.

Signed-off-by: Angel Misevski <[email protected]>
Use yq to make long strings in bundle manifests stay on a single line.
As of operator-sdk v1.8.0, long strings are hard-wrapped, and this
appears to cause issues when installing the operator.

See operator-framework/operator-sdk#5033 for
more details

Signed-off-by: Angel Misevski <[email protected]>
Move conversion webhooks from the standalone webhooks server into the
main controller deployment. This helps in using automatic configuration
from OLM in creating webhooks, as this is unavailable for deployments
not in the CSV.

Using OLM to create the webhook configs and inject service
name/namespace into the CRDs enables deploying the DevWorkspace
controller to any namespace instead of restricting it to
openshift-operators.

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

* Create a service for webhooks pointing at the controller dpeloyment
* Update the certs issued by cert-manager to match the controller's
service name
* Expose a conversion port in the main controller deployment

Signed-off-by: Angel Misevski <[email protected]>
Rename the service associated with the controller deployment to
'devworkspace-controller-manager-service' to match what appears to be
expected by OLM. Otherwise, a duplicate service named such is created
when the operator is installed via OLM.

Signed-off-by: Angel Misevski <[email protected]>
Add workaround for issue [1] when generating webhookdefinitions in the
ClusterServiceVersion by using yq to directly sort the array. Also add
documentation explaining current workarounds, since they're strange at
first glance.

[1] - operator-framework/operator-sdk#5022

Signed-off-by: Angel Misevski <[email protected]>
@amisevsk amisevsk force-pushed the conversion-webhooks-in-main branch from 4fac554 to 7b9e0a3 Compare July 6, 2021 00:51
amisevsk added 2 commits July 6, 2021 23:25
Define webhooks server secret name to deployment templates when running
in OpenShift and use similar functionality for getting this name in both
Kubernetes and OpenShift.

Signed-off-by: Angel Misevski <[email protected]>
@amisevsk amisevsk force-pushed the conversion-webhooks-in-main branch from 76ac02c to 6248389 Compare July 7, 2021 03:25
@sleshchenko
Copy link
Member

/test v7-devworkspaces-operator-e2e, v7-devworkspace-happy-path

Copy link
Contributor

@JPinkney JPinkney left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@openshift-ci
Copy link

openshift-ci bot commented Jul 7, 2021

[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

@amisevsk
Copy link
Collaborator Author

amisevsk commented Jul 7, 2021

/test v7-devworkspaces-operator-e2e, v7-devworkspace-happy-path

@sleshchenko
Copy link
Member

/test v7-devworkspace-happy-path

@sleshchenko
Copy link
Member

Tests fail with https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/origin-ci-test/pr-logs/pull/devfile_devworkspace-operator/474/pull-ci-devfile-devworkspace-operator-main-v7-devworkspace-happy-path/1412825297429991424/artifacts/devworkspace-happy-path/devworkspace-happy-path/artifacts/devworkspace-controller-info/devworkspace-controller-manager-66db66f88c-mkgnj-devworkspace-controller.log

{"level":"error","ts":1625682041.475059,"logger":"setup","msg":"failed to setup webhooks","error":"could not deploy webhooks server: environment variable WEBHOOK_SECRET_NAME is unset","stacktrace":"main.main\n\t/devworkspace-operator/main.go:140\nruntime.main\n\t/usr/lib/golang/src/runtime/proc.go:204"}

@openshift-ci
Copy link

openshift-ci bot commented Jul 8, 2021

@amisevsk: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
ci/prow/v7-devworkspace-happy-path 6248389 link /test v7-devworkspace-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/test-infra repository. I understand the commands that are listed here.

@amisevsk
Copy link
Collaborator Author

amisevsk commented Jul 8, 2021

Happy path tests are failing because they use chectl to deploy templates, which ignores updates to templates from the PR being tested. I think fixing this is out of scope for the current PR.

Tested latest changes manually on OpenShift and the happy path test failure does not reproduce.

@amisevsk amisevsk merged commit 80702f7 into devfile:main Jul 8, 2021
@amisevsk amisevsk deleted the conversion-webhooks-in-main branch July 8, 2021 19:09
@sleshchenko
Copy link
Member

+1 to merge without waiting for happy path fix, but I remember we already faced the same and Serhii faced it.
We need a separate issue for Maxim to take a look on that.

@sleshchenko
Copy link
Member

OK, it seems that it's not possible to use the latest deployment files since Che Operator image embeds them on container image build, unless we build Che operator image with custom templates... Or we may install devworkspace operator with make install before installing Che with devworkspace support.

@amisevsk
Copy link
Collaborator Author

amisevsk commented Jul 9, 2021

Forgot to mention on closing -- created a dedicated issue: #484

@amisevsk
Copy link
Collaborator Author

amisevsk commented Jul 9, 2021

Or we may install devworkspace operator with make install before installing Che with devworkspace support.

Will Che overwrite existing devworkspace resources on install, or will it detect that DevWorkspace is already present? A relatively simple fix would be to oc apply -f deploy/templates/openshift/combined.yaml before/after installing the Che operator, since that's basically what chectl does.

@amisevsk amisevsk mentioned this pull request Jul 9, 2021
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move conversion webhooks into the controller deployment rather than webhooks server
3 participants