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.

Sign up for GitHub

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

Jump to bottom

Fail any DevWorkspace when webhooks are not present on cluster #307

Closed
wants to merge 2 commits into from
Closed

Fail any DevWorkspace when webhooks are not present on cluster #307

wants to merge 2 commits into from

Conversation

amisevsk
Copy link
Collaborator

@amisevsk amisevsk commented Mar 8, 2021

Note: I'm not sure this PR should even be merged. The added checks may catch some simple configuration errors (e.g. webhooks accidentally get deleted) but does nothing to address the main reason we do any checking for webhooks (i.e. preventing unauthorized access). I'm opening this PR for discussion in case there's something I'm missing, but it seems kind of pointless IMO.

What does this PR do?

  • Validate that webhooks exist on cluster when reconciling a DevWorkspace, and fail the workspace if they're not present
  • Rework validateCreatorID check to check that both mutating and validating webhooks exist and that they're older than the DevWorkspace.
  • Extract two functions: killWorkspace, which deletes a workspace's deployment and marks it as failed, and failWorkspace which just cuts out the four lines we use to fail workspaces everywhere in the main reconcile loop.

The PR is split into two commits:

  • The first commit is a half-way solution, doing the detailed check when a workspace has the restricted-access annotation set.
  • The second commit extends the check above to all DevWorkspaces and does some light refactoring

What issues does this PR fix or reference?

Fixes #305

Is it tested? How?

  1. Create a DevWorkspace on the cluster
  2. Delete the mutating webhook: kc delete mutatingwebhookconfigurations.admissionregistration.k8s.io controller.devfile.io
  3. Edit the DevWorkspace created in step 1
  4. Workspace should be failed, workspace deployment should be deleted.

amisevsk added 2 commits March 8, 2021 15:34
@amisevsk
Ensure webhooks are active on reconcile of restricted-access workspaces
dd18ba4 
Instead of checking the webhooks creation timestamp once on startup and
caching this result, check that webhooks exist on the cluster every time
we reconcile a DevWorkspace.

Signed-off-by: Angel Misevski <amisevsk@redhat.com>
@amisevsk
Fail workspaces on reconcile whenever webhooks are not present
7c733e7 
Extend the webhooks check to include all workspaces, instead of just
those with the restricted-access annotation set.

Signed-off-by: Angel Misevski <amisevsk@redhat.com>
@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: amisevsk

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:

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

@amisevsk amisevsk marked this pull request as draft March 8, 2021 20:39
@openshift-ci-robot
Copy link
Collaborator

@amisevsk: PR needs rebase.

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.

@openshift-ci
Copy link

openshift-ci bot commented Apr 12, 2021

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

Test name Commit Details Rerun command
ci/prow/v7-images 7c733e7 link /test v7-images
ci/prow/v7-devworkspace-happy-path 7c733e7 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

Closing this PR as it's out of date with the current repo and I don't think there's much value here.

@amisevsk amisevsk closed this Apr 28, 2021
@amisevsk amisevsk deleted the check-webhooks branch February 8, 2023 15:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sleshchenko sleshchenko Awaiting requested review from sleshchenko

@JPinkney JPinkney Awaiting requested review from JPinkney

@tinakurian tinakurian Awaiting requested review from tinakurian

@dkwon17 dkwon17 Awaiting requested review from dkwon17 dkwon17 will be requested when the pull request is marked ready for review dkwon17 is a code owner

@ibuziuk ibuziuk Awaiting requested review from ibuziuk ibuziuk will be requested when the pull request is marked ready for review ibuziuk is a code owner

Assignees
No one assigned
Labels
approved do-not-merge/work-in-progress needs-rebase
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Check that required webhooks exist on the cluster when a DW has restricted-access annotation
2 participants
@amisevsk @openshift-ci-robot