Skip to content
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

Use new devworkspace-controller templates and add cert-manager v1 apiVersion #1100

Merged
merged 6 commits into from
Feb 22, 2021

Conversation

flacatus
Copy link
Collaborator

@flacatus flacatus commented Feb 16, 2021

Signed-off-by: Flavius Lacatusu flacatus@redhat.com

What does this PR do?

This PR replace old templates of devworkspace-controller with new one after migration to operatorsdk1. Also in this PR change cert-manager api version from v1alpha2 to v1(stable)

Reff issue: eclipse-che/che#18962
Reff issue: eclipse-che/che#18842

Screenshot/screencast of this PR

What issues does this PR fix or reference?

How to test this PR?

PR Checklist

As the author of this Pull Request I made sure that:

Reviewers

Reviewers, please comment how you tested the PR when approving it.

@flacatus
Copy link
Collaborator Author

Helm tests will fail. Need to change api version of cert-manager here: https://github.com/eclipse/che/blob/master/deploy/cert-manager/che-cluster-issuer.yml

@flacatus
Copy link
Collaborator Author

I was able to deploy devworkspace-controller in Ocp 4.X and also in minikube. I've create workspaces in both platform using samples: https://github.com/devfile/devworkspace-operator/tree/master/samples

@metlos
Copy link
Contributor

metlos commented Feb 16, 2021

Does this mean that we will need to change chectl whenever devworkspace-operator decides to add or remove a file from the deployment? Wouldn't it be better to just generically apply all the files in the deploy/deployment/${PLATFORM}/objects ?

@flacatus
Copy link
Collaborator Author

flacatus commented Feb 16, 2021

Does this mean that we will need to change chectl whenever devworkspace-operator decides to add or remove a file from the deployment? Wouldn't it be better to just generically apply all the files in the deploy/deployment/${PLATFORM}/objects ?

@metlos Honestly I preffer to add one by one the objects because for example if one of the objects exist in cluster whole installation will fail.

Anyway if you remove/add objects in devworkspace you need to change chect server:delete.

Signed-off-by: Flavius Lacatusu <flacatus@redhat.com>
Signed-off-by: Flavius Lacatusu <flacatus@redhat.com>
Signed-off-by: Flavius Lacatusu <flacatus@redhat.com>
@metlos
Copy link
Contributor

metlos commented Feb 17, 2021

I was trying to override the namespace by running:

bin/run server:deploy -p minikube --workspace-engine=dev-workspace --dev-workspace-controller-namespace=dw-ctrl

and got this error:

Create ServiceAccount devworkspace-controller-serviceaccount in namespace dw-ctrl
      → the namespace of the provided object does not match the namespace sent on the request

src/api/kube.ts Show resolved Hide resolved
src/api/kube.ts Show resolved Hide resolved
src/api/kube.ts Outdated Show resolved Hide resolved
src/api/kube.ts Outdated Show resolved Hide resolved
src/tasks/component-installers/cert-manager.ts Outdated Show resolved Hide resolved
src/api/kube.ts Outdated Show resolved Hide resolved
src/api/kube.ts Outdated Show resolved Hide resolved
// Deployment names
protected deploymentName = 'devworkspace-controller-manager'

protected devworkspaceConfigMap = 'devworkspace-controller-configmap'
Copy link
Contributor

Choose a reason for hiding this comment

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

devworkspaceConfigMap -> devWorkspaceConfigMap

}
}

async isMutatingWebhookConfigurationExists(name: string): Promise<boolean> {
Copy link
Contributor

Choose a reason for hiding this comment

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

at milestone 1 we must guarantee that DevWorkspace Engine it not enabled on the cluster where Web Terminal is enabled.

So, apart from checking if mutating webhooks cfg exist, it would be cool to check the configured namespace inside. It's the example of Mutating Webhooks created by Web Terminal

apiVersion: admissionregistration.k8s.io/v1
kind: MutatingWebhookConfiguration
metadata:
  labels:
    app.kubernetes.io/name: devworkspace-webhook-server
    app.kubernetes.io/part-of: devworkspace-operator
  name: controller.devfile.io
webhooks:
- admissionReviewVersions:
  - v1beta1
  clientConfig:
    ...
    service:
      name: devworkspace-webhookserver
      namespace: openshift-operators
      path: /mutate
      port: 443
  ...
  name: mutate.devworkspace-controller.svc
  namespaceSelector: {}
  objectSelector: {}
  reinvocationPolicy: Never
  rules:
  ...
- admissionReviewVersions:
  - v1beta1
  clientConfig:
    service:
      name: devworkspace-webhookserver
      namespace: openshift-operators
      path: /mutate
      port: 443
  ...

Pay attention on namespace: openshift-operators.

Also, about Web Terminal presence can tell Subscription:

apiVersion: operators.coreos.com/v1alpha1
kind: Subscription
metadata:
  labels:
    operators.coreos.com/web-terminal.openshift-operators: ""
  name: web-terminal
  namespace: openshift-operators
spec:
  channel: alpha
  installPlanApproval: Automatic
  name: web-terminal
  source: redhat-operators
  sourceNamespace: openshift-marketplace
  startingCSV: web-terminal.v1.1.0

But in general it's going to break DevWorkspaces, if there is another devworkspace controller deployed.

So, the smart logic on Chectl side could be (at least in my opinion ;) ):

  1. There is no need to provide an ability to override devworkspace operator namespace, so let's just remove it.
  2. Then chectl checks and fails if Web Terminal installed (Subscription exists).
  3. Then chectl checks if mutatingwebhooks exists, and their namespace is different than devworkspace-operator - checlt fails and tells that WebTerminal is not uninstalled properly (if namespace if openshift-operators) or another devworkspace controller is already deployed in namespace X (it can be evaluated on chectl with webhooks object).

Signed-off-by: Flavius Lacatusu <flacatus@redhat.com>
@@ -1400,7 +1498,7 @@ export class KubeHelper {
return crdv ? crdv.name : 'v1'
}

async deleteCrd(name: string): Promise<void> {
async deleteCrdV1Beta1(name: string): Promise<void> {
Copy link
Contributor

Choose a reason for hiding this comment

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

What if a resource with v1alpha1 or v1alpha2 appear? What if it then turned into v1beta2?
According to the current strategy, we'll have to add several methods per version.
To me it is better to have version as second parameter (maybe default to v1)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agree. But let's keep as is for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: flacatus, mmorhun, tolusha
To complete the pull request process, please assign after the PR has been reviewed.
You can assign the PR to them by writing /assign in a comment when ready.

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

Needs approval from an approver in each of these files:

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

Signed-off-by: Flavius Lacatusu <flacatus@redhat.com>
Signed-off-by: Flavius Lacatusu <flacatus@redhat.com>
@tolusha
Copy link
Collaborator

tolusha commented Feb 22, 2021

/test v7-chectl-e2e-operator-installer

@tolusha
Copy link
Collaborator

tolusha commented Feb 22, 2021

/test v6-chectl-e2e-olm-installer

@flacatus flacatus merged commit e0392c8 into master Feb 22, 2021
@flacatus flacatus deleted the templates_devw branch February 22, 2021 16:23
@che-bot che-bot added this to the 7.27 milestone Feb 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants