[ODC-4388] Allow cluster admins to create terminals #7145

JPinkney · 2020-11-06T13:25:40Z

This PR is part of https://issues.redhat.com/browse/ODC-4388 which is for giving privileged users access to create terminal.

Currently, the flow is:
If the user is a cluster-admin then deny them from opening a terminal.

Now, the flow is:
If the user is a cluster-admin then automatically create their terminal in a secure openshift-terminal namespace. If the openshift-terminal namespace does not exist then create it.

The backend code then checks if the current user is a cluster-admin and if they are then check that the workspace they are trying to access is in the openshift-terminal namespace.

Before: https://www.youtube.com/watch?v=TUri-TE52UA
After: https://www.youtube.com/watch?v=v6Xy7LKbYrU

You can test with: quay.io/jpinkney/console:515

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

openshift-ci-robot · 2020-11-06T13:26:03Z

Hi @JPinkney. Thanks for your PR.

I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

pkg/terminal/proxy.go

sleshchenko

Everything apart from the shared corner-case works just fine

frontend/packages/console-app/src/components/cloud-shell/CloudShellTerminal.tsx

pkg/terminal/proxy.go

serenamarie125 · 2020-11-18T13:39:21Z

This is aligned with one of our committed epics - confirmed that this is not dependent on updated to the WTO.

@christianvogt can you help get reviews on this?

serenamarie125

Thanks for doing this! LGTM

serenamarie125 · 2020-11-18T13:40:36Z

FYI @parvathyvr @alimobrem

sleshchenko

Tested on crc (1.18.0+bb304aa with OpenShift 4.6) with my built console and everything works as expected:

Cluster admin is not able to exec into developer workspace.
Cluster admin gets terminal in openshift-terminal namespace.
Console ignores all terminals which are not in openshift-terminal namespace for cluster admin.
Backend denies to /exec/init for cluster admin if terminal is not in openshift-terminal.

Good job!

frontend/packages/console-app/src/components/cloud-shell/useCloudShellWorkspace.ts

frontend/packages/console-app/src/components/cloud-shell/setup/CloudShellAdminSetup.tsx

christianvogt · 2020-11-20T22:22:32Z

Tried this and works as expected on the surface.
However, when I first installed the operator and opened the terminal as an admin, I noticed that 3 DevWorkspace resources were created inside the openshift-terminal namespace.
I cannot reproduce this again on the same cluster anymore though....

JPinkney · 2020-11-23T19:57:15Z

@christianvogt Regarding:

However, when I first installed the operator and opened the terminal as an admin, I noticed that 3 DevWorkspace resources were created inside the openshift-terminal namespace.
I cannot reproduce this again on the same cluster anymore though....

Can you try with the latest changes? I just got a fresh cluster from cluster bot and deployed the changes and looks like I can't reproduce anymore

christianvogt · 2020-11-23T21:07:29Z

/ok-to-test

JPinkney · 2020-11-24T13:07:11Z

/retest

christianvogt · 2020-11-24T22:39:09Z

@sleshchenko @JPinkney is everything on the backend good now? Sorry I haven't been following what's needed on the backend changes.

JPinkney · 2020-11-24T22:52:14Z

Yeah, everything should be good on the backend and frontend now

christianvogt · 2020-11-24T23:43:52Z

/lgtm
/approve

christianvogt · 2020-11-24T23:44:11Z

I tried again on cluster bot and everything worked for the admin and normal user.

christianvogt · 2020-11-25T14:38:26Z

/retest

christianvogt · 2020-11-30T13:15:32Z

@spadgett @rhamilto this PR needs your approval

@JPinkney please rebase

frontend/packages/console-app/src/components/cloud-shell/CloudShellTerminal.tsx

frontend/packages/console-app/src/components/cloud-shell/setup/CloudShellAdminSetup.tsx

pkg/terminal/auth.go

frontend/packages/console-app/src/components/cloud-shell/CloudShellTerminal.tsx

christianvogt · 2020-12-02T19:45:34Z

front end changes lgtm. Waiting for cluster bot to test again.

spadgett

/approve

Backend changes LGTM. I will let @christianvogt give final lgtm. Thanks!

pkg/terminal/auth.go

…mespace Modify backend proxy check to allow cluster admins if and only if their terminal is in the openshift-terminal namespace Only search for terminals in openshift-terminal namespace when using cluster admin Rework terminal proxy to use isClusterAdmin for checking if the user can access the terminal Use error status to see if namespace exists in CloudShellAdminSetup Return false if namespace is not found in CloudShellAdminSetup instead of displaying error. The reason we need to do this is if you set the error the terminal will flash quickly with the error before switching to the newly created terminal. Make subject access review request against openshift-terminal namespace Display loading screen until admin check has finished Signed-off-by: Josh Pinkney <joshpinkney@gmail.com>

christianvogt · 2020-12-03T18:49:05Z

/lgtm

openshift-ci-robot · 2020-12-03T18:49:25Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: christianvogt, JPinkney, serenamarie125, sleshchenko, spadgett

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~~ [spadgett]

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 added the component/backend Related to backend label Nov 6, 2020

openshift-ci-robot requested review from dtaylor113 and spadgett November 6, 2020 13:25

openshift-ci-robot added component/core Related to console core functionality needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 6, 2020

sleshchenko reviewed Nov 9, 2020

View reviewed changes

pkg/terminal/proxy.go Show resolved Hide resolved

JPinkney changed the title ~~Allow cluster admins to create terminals~~ [ODC-4388] Allow cluster admins to create terminals Nov 12, 2020

sleshchenko reviewed Nov 13, 2020

View reviewed changes

frontend/packages/console-app/src/components/cloud-shell/CloudShellTerminal.tsx Show resolved Hide resolved

sleshchenko reviewed Nov 17, 2020

View reviewed changes

pkg/terminal/proxy.go Outdated Show resolved Hide resolved

serenamarie125 approved these changes Nov 18, 2020

View reviewed changes

sleshchenko approved these changes Nov 18, 2020

View reviewed changes

sleshchenko mentioned this pull request Nov 19, 2020

Che Controller Sprint #193 eclipse-che/che#18412

Closed

14 tasks

christianvogt reviewed Nov 20, 2020

View reviewed changes

openshift-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 23, 2020

openshift-ci-robot assigned christianvogt Nov 24, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 24, 2020

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Nov 30, 2020

spadgett requested changes Dec 1, 2020

View reviewed changes

christianvogt reviewed Dec 2, 2020

View reviewed changes

frontend/packages/console-app/src/components/cloud-shell/CloudShellTerminal.tsx Outdated Show resolved Hide resolved

spadgett approved these changes Dec 2, 2020

View reviewed changes

pkg/terminal/auth.go Outdated Show resolved Hide resolved

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 3, 2020

openshift-merge-robot merged commit f9df538 into openshift:master Dec 4, 2020

spadgett added this to the v4.7 milestone Dec 9, 2020

JPinkney mentioned this pull request Dec 11, 2020

Update web-Terminal-adminuser.feature with actual admin flow #7532

Merged

sleshchenko mentioned this pull request Jan 11, 2021

Web terminal docs openshift/openshift-docs#28480

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ODC-4388] Allow cluster admins to create terminals #7145

[ODC-4388] Allow cluster admins to create terminals #7145

JPinkney commented Nov 6, 2020 •

edited

openshift-ci-robot commented Nov 6, 2020

sleshchenko left a comment

serenamarie125 commented Nov 18, 2020

serenamarie125 left a comment

serenamarie125 commented Nov 18, 2020

sleshchenko left a comment

christianvogt commented Nov 20, 2020

JPinkney commented Nov 23, 2020

christianvogt commented Nov 23, 2020

JPinkney commented Nov 24, 2020

christianvogt commented Nov 24, 2020

JPinkney commented Nov 24, 2020

christianvogt commented Nov 24, 2020

christianvogt commented Nov 24, 2020

christianvogt commented Nov 25, 2020

christianvogt commented Nov 30, 2020

christianvogt commented Dec 2, 2020

spadgett left a comment

christianvogt commented Dec 3, 2020

openshift-ci-robot commented Dec 3, 2020

[ODC-4388] Allow cluster admins to create terminals #7145

[ODC-4388] Allow cluster admins to create terminals #7145

Conversation

JPinkney commented Nov 6, 2020 • edited

openshift-ci-robot commented Nov 6, 2020

sleshchenko left a comment

Choose a reason for hiding this comment

serenamarie125 commented Nov 18, 2020

serenamarie125 left a comment

Choose a reason for hiding this comment

serenamarie125 commented Nov 18, 2020

sleshchenko left a comment

Choose a reason for hiding this comment

christianvogt commented Nov 20, 2020

JPinkney commented Nov 23, 2020

christianvogt commented Nov 23, 2020

JPinkney commented Nov 24, 2020

christianvogt commented Nov 24, 2020

JPinkney commented Nov 24, 2020

christianvogt commented Nov 24, 2020

christianvogt commented Nov 24, 2020

christianvogt commented Nov 25, 2020

christianvogt commented Nov 30, 2020

christianvogt commented Dec 2, 2020

spadgett left a comment

Choose a reason for hiding this comment

christianvogt commented Dec 3, 2020

openshift-ci-robot commented Dec 3, 2020

JPinkney commented Nov 6, 2020 •

edited