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

Rework cert generation so that it uses two deployments #96

Merged
merged 1 commit into from
Jun 17, 2020

Conversation

JPinkney
Copy link
Contributor

@JPinkney JPinkney commented Jun 5, 2020

Signed-off-by: Josh Pinkney joshpinkney@gmail.com

What does this PR do?

This PR reworks the way we do cert generation completely and has a few massive benefits:

  1. We can mount the configmap in the controller deployment itself, meaning that we don't need to restart the controller once the configmaps have been created
  2. We save SO much time because the controller starts right away
  3. We save ourself from things like Do not allow to change webhooks enabled on the fly eclipse-che/che#16980 (comment)
  4. Fixes a deployment issue when installing via OLM. Basically what was happening was that If you modify the deployment to add volumes after (like we previously did with cert generation) then OLM will automatically use the deployment template from the CSV, instead of our changed deployment for creating the new pod.

The way it works is that when webhooks are enabled we deploy an additional deployment called che-workspace-controller-cert-gen that has the sole responsibility of creating the configmap and service needed for the certs. Then, instead of mounting the configmap into the controller deployment later we mount the configmap in the controller deployment yaml right away. That way when the controller deployment is ready and started it already has all the certs available.

Since the che-workspace-controller-cert-gen binary is so small it actually finishes running before the controller deployment even finishes getting setup. In the worst case, the controller deployment will block for 1-2 seconds (this is only theoretical as I've never actually seen it happen). When the controller starts setting up the webhooks (and the certs are available) it will delete the che-workspace-controller-cert-gen deployment.

Ideally, we'd use a Job for creating che-workspace-controller-cert-gen but since OLM doesn't allow you to specify jobs in the bundle, we will have to use deployment.

What issues does this PR fix or reference?

Part of eclipse-che/che#16980

Is it tested? How?

make docker_cert
make docker
make deploy

with both webhooks on and then webhooks off

cert-generation/main.go Outdated Show resolved Hide resolved
cert-generation/main.go Outdated Show resolved Hide resolved
cert-generation/main.go Show resolved Hide resolved
Copy link
Collaborator

@amisevsk amisevsk left a comment

Choose a reason for hiding this comment

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

LGTM. Rebased this PR on master and tested on crc.

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.

I did not test it but code LGTM

Makefile Outdated Show resolved Hide resolved
cert-generation/main.go Show resolved Hide resolved
cert-generation/main.go Outdated Show resolved Hide resolved
@sleshchenko
Copy link
Member

/approve

Signed-off-by: Josh Pinkney <joshpinkney@gmail.com>
@amisevsk
Copy link
Collaborator

/approve

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: amisevsk, JPinkney, sleshchenko
To complete the pull request process, please assign
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

@JPinkney
Copy link
Contributor Author

/ok-to-test

@JPinkney
Copy link
Contributor Author

/auto-cc

@JPinkney
Copy link
Contributor Author

/retest

2 similar comments
@amisevsk
Copy link
Collaborator

/retest

@sleshchenko
Copy link
Member

/retest

@JPinkney JPinkney merged commit b194813 into devfile:master Jun 17, 2020
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.

4 participants