SingleHost + TLS on OpenShift #201

sleshchenko · 2020-11-30T07:55:21Z

What does this PR do?

This PR enables Single host + TLS on OpenShift for basic routing.
It has self-describing commits.

⚠️ It worths noting that changes are done on creator access only.
Now creator access only access is provided only for workspaces annotated with controller.devfile.io/restricted-access: true
Now CloudShell workspace (not web-terminal) can be opened only with OpenShift OAuth routing.
CloudShell workspace also does not support single-host feature because it uses absolute references at this point.

TODOs:

Theia /che/cluster-ip is failing because it uses absolute reference Adapt telemetry functionality in Che Theia to single-host eclipse-che/che#18503
Theia Webview
Theia Webview does not work on Chrome if you don't import CA into the browser. Artem will create an issue to expose this info and the most probable solution is just documenting it, or improve Theia to render the proper error page;
Theia Webview is not stable

2020-12-01 14:53:12.187 root ERROR Security problem: Unable to configure separate webviews domain:  Params: Error: Unable to get workspace containers. Cause: [object Object]
    at CheServerWorkspaceServiceImpl.<anonymous> (/home/theia/node_modules/@eclipse-che/theia-remote-api/lib/node/che-server-workspace-service-impl.js:193:31)
    at step (/home/theia/node_modules/@eclipse-che/theia-remote-api/lib/node/che-server-workspace-service-impl.js:62:23)
    at Object.throw (/home/theia/node_modules/@eclipse-che/theia-remote-api/lib/node/che-server-workspace-service-impl.js:43:53)
    at rejected (/home/theia/node_modules/@eclipse-che/theia-remote-api/lib/node/che-server-workspace-service-impl.js:35:65)
    at processTicksAndRejections (internal/process/task_queues.js:97:5)

It works depending on the container startup sequence.
Theia needs to try again when Che is not available for such fundamental stuff like Webview or che-api-sidecar may be moved to a dedicated pod which we start first, but no one of them worths attention because Che API Sidecar is just a temporary solution and it should be reworked in a proper way where Theia access like K8s cluster to get needed info;

OpenShift OAuth expects to be the only application on the host and sends an absolute redirect
Endpoint name is not unique. Should we include the component name to path rule as well?

What issues does this PR fix or reference?

#200

Is it tested? How?

Minikube

Verified that on minikube everything works as previously + now terminal works fine for:

make install

kubectl apply -f ./samples/theia-next.yaml

kubectl apply -f ./samples/all-in-one-theia-nodejs.devworkspace.yaml

CRC

Verified that on crc now:

basic routing uses single-host approach;
Theia works as expected (apart from Webview which is not stable and it's known issue described above);
now terminal works fine;
for:

kubectl apply -f ./samples/theia-next.yaml

kc apply -f ./samples/all-in-one-theia-nodejs.devworkspace.yaml

CloudShell works as it used to work with host per endpoint approach.

openshift-ci-robot · 2020-11-30T07:55:22Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

sleshchenko · 2020-11-30T15:47:37Z

/test v5-devworkspaces-operator-e2e

amisevsk

A few minor comments below, but LGTM at its core. I'll test it later today, once I get crc up and running

controllers/controller/workspacerouting/solvers/common.go

controllers/controller/workspacerouting/solvers/solver.go

controllers/controller/workspacerouting/solvers/openshift_oauth_solver.go

controllers/controller/workspacerouting/solvers/resolve_endpoints.go

pkg/common/naming.go

samples/theia-next.yaml

amisevsk · 2020-11-30T16:59:39Z

samples/cloud-shell.yaml

@@ -2,8 +2,13 @@ kind: DevWorkspace
 apiVersion: workspace.devfile.io/v1alpha1
 metadata:
  name: cloud-shell
+  annotations:
+    # it's important to make workspace immutable to make sure nobody with right access


Suggested change

# it's important to make workspace immutable to make sure nobody with right access

# it's important to restrict access to the workspace to make sure other users with sufficient privileges

Since we add this annotation in four places, I think it makes sense to remove these comments from our samples. Instead, it's better to start describing annotation which is supposed to be used by users somewhere, we can start with README.md. WDYT?

Yeah, I like the idea of putting these annotations in the README

I've added the corresponding section into README.md. I've used the format as https://kubernetes.github.io/ingress-nginx/user-guide/nginx-configuration/annotations/ has. Feedback or remarks are welcome.

openshift-ci-robot · 2020-11-30T17:00:54Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: amisevsk, 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~~ [amisevsk,sleshchenko]

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 · 2020-11-30T17:26:10Z

New changes are detected. LGTM label has been removed.

JPinkney · 2020-11-30T19:13:36Z

samples/cloud-shell.yaml

@@ -2,8 +2,13 @@ kind: DevWorkspace
 apiVersion: workspace.devfile.io/v1alpha1
 metadata:
  name: cloud-shell
+  annotations:
+    # it's important to make workspace immutable to make sure nobody with right access


Yeah, I like the idea of putting these annotations in the README

controllers/controller/workspacerouting/workspacerouting_controller.go

amisevsk

I ran into a few issues on crc. I'm not sure if this is expected at this point, but during theia startup I see the error message

2020-11-30 22:03:44.880 root ERROR Security problem: Unable to configure separate webviews domain: Params: Error: Unable to get workspace containers. Cause: [object Object]

logged. This may also be related to the issue below.

amisevsk · 2020-11-30T22:50:54Z

pkg/common/naming.go

+}
+
+func EndpointPath(endpointName string) string {
+	return "/" + endpointName + "/"


In testing on crc I found a few issues with our approach to paths:

When we suffix the path with /, that exact string must be used to access the endpoint. This is an issue because the ideURL ends up without the trailing / so clicking the ideURL takes you to the OpenShift "application not available" page.

The workspace never enters a Running state -- it's stuck in Starting. I believe (but haven't verified) that this is because we use ideURL in checkServerStatus():
ideUrl := workspace.Status.IdeUrl healthz, err := url.Parse(ideUrl) healthz.Path = healthz.Path + "healthz"
since I'm seeing IdeURL as e.g. https://workspace99cfef48974c4fef.apps-crc.testing/theia it would appear that we're trying to ping https://workspace99cfef48974c4fef.apps-crc.testing/theiahealthz, which returns 503 instead of a 4xx that we're expecting. This causes the readiness check to fail at the end.

ideUrl was with trailing /... probably it went after Path.join stuff
Interesting that theia is not loaded when you access it without a trailing slash, seems Theia needs the right redirect:
without trailing slash

with trailing slash

So, I'll make sure that endpoints have trailing slash if it's path rule, and it should be possible to specify endpoint path without a slash.

Should be fixed with db048ad

sleshchenko · 2020-12-01T13:40:11Z

/test v5-devworkspaces-operator-e2e

sleshchenko · 2020-12-01T15:56:59Z

Unreal, even e2e tests passed :-D

amisevsk

Testing on crc + firefox and chrome, I can start up theia-next and see the webview (no images are loaded). However, once Theia is loaded, I see a tight loop on requests to https://workspace918799a646044762.apps-crc.testing/che/client-ip which all return 503 -- this route does not exist on the cluster (there's probably >50 req/sec here)

I don't know enough about theia internals to debug further, and the theia logs become enormous pretty quickly with an (probably related) error

Promise rejection not handled in one second: Error: Request failed with status code 503 , reason: Error: Request failed with status code 503
With stack trace: UJUX/e.exports@https://workspace918799a646044762.apps-crc.testing/theia/vendors.c8bdf28598a14f9b5776.js:212:2276
MkyZ/e.exports@https://workspace918799a646044762.apps-crc.testing/theia/vendors.c8bdf28598a14f9b5776.js:116:5450
hR+n/e.exports/</p.onreadystatechange@https://workspace918799a646044762.apps-crc.testing/theia/vendors.c8bdf28598a14f9b5776.js:293:2425 
2020-12-01 18:56:12.186 root ERROR [hosted-plugin: 47] Promise rejection not handled in one second: Error: Request failed with status code 503 , reason: Error: Request failed with status code 503
With stack trace: UJUX/e.exports@https://workspace918799a646044762.apps-crc.testing/theia/vendors.c8bdf28598a14f9b5776.js:212:2276
MkyZ/e.exports@https://workspace918799a646044762.apps-crc.testing/theia/vendors.c8bdf28598a14f9b5776.js:116:5450
hR+n/e.exports/</p.onreadystatechange@https://workspace918799a646044762.apps-crc.testing/theia/vendors.c8bdf28598a14f9b5776.js:293:2425

amisevsk · 2020-12-01T19:11:36Z

For

It works depending on the container startup sequence.
Theia needs to try again when Che is not available for such fundamental stuff like Webview or che-api-sidecar may be moved to a dedicated pod which we start first, but no one of them worths attention because Che API Sidecar is just a temporary solution and it should be reworked in a proper way where Theia access like K8s cluster to get needed info;

it may just be a bit of weird trivia, but I think we can get around this by making sure che-rest-apis is the first container in the pod spec. Kube starts containers in the order they're listed, iirc. If che-rest-apis is temporary this may be a worthwhile workaround.

sleshchenko · 2020-12-01T19:13:16Z

I don't know enough about theia internals to debug further, and the theia logs become enormous pretty quickly with an (probably related) error

Known bug, Artem will take a look at that. See PR description )

amisevsk · 2020-12-01T19:16:25Z

Known bug, Artem will take a look at that. See PR description )

Ah missed that line, apologies.

sleshchenko · 2020-12-01T19:19:46Z

New happy path check helps me to figure out that creator-access only is broken. I'll take a look tomorrow on that

sleshchenko · 2020-12-02T10:43:29Z

Creator access only is recovered in bd01489
I just missed restrictedAnnotation propagation to deployment and pod.

Also, I think it may be redundant to deny workspace update for workspace with not-restricted-access, so it's done in 1250c5c

Note that all workspace-related objects, route, service, deployment are still immutable for both workspace types (with and without restricted access). We may keep it only for restricted access related workspace if we decide, but it may be done in a separate PR as well.

…ccess

…abled

sleshchenko · 2020-12-02T12:27:50Z

/test v5-devworkspaces-operator-e2e

sleshchenko · 2020-12-02T12:30:48Z

sleshchenko · 2020-12-02T15:00:14Z

/test v5-devworkspaces-operator-e2e

JPinkney · 2020-12-02T20:27:52Z

Just tested on cluster bot cluster and ran into a few things:

For samples/theia-next.yaml

Cloning when using samples/theia-next.yaml failed with Couldn't clone https://github.com/che-samples/web-nodejs-sample.git: fatal: could not create work tree dir '/projects/project': Permission denied. Other than the project not being available I was able to get into the theia terminal and the workspace view was working

For samples/all-in-one-theia-nodejs.devworkspace.yaml

Seems like i'm getting errors when its trying to find other containers so the workspace view isn't working :

2020-12-02 20:17:02.405 root ERROR Error: Unable to get workspace containers. Cause: [object Object]
    at CheServerWorkspaceServiceImpl.<anonymous> (/home/theia/node_modules/@eclipse-che/theia-remote-api/lib/node/che-server-workspace-service-impl.js:184:31)
    at step (/home/theia/node_modules/@eclipse-che/theia-remote-api/lib/node/che-server-workspace-service-impl.js:53:23)
    at Object.throw (/home/theia/node_modules/@eclipse-che/theia-remote-api/lib/node/che-server-workspace-service-impl.js:34:53)
    at rejected (/home/theia/node_modules/@eclipse-che/theia-remote-api/lib/node/che-server-workspace-service-impl.js:26:65)
    at processTicksAndRejections (internal/process/task_queues.js:97:5)
2020-12-02 20:17:02.491 root ERROR Unable to create Che API REST Client: "CHE_WORKSPACE_TELEMETRY_BACKEND_PORT" is not set.
2020-12-02 20:17:02.526 root ERROR Request getCurrentWorkspacesContainers failed with error: Unable to get workspace containers. Cause: [object Object] Params: Error: Unable to get workspace containers. Cause: [object Object]
    at CheServerWorkspaceServiceImpl.<anonymous> (/home/theia/node_modules/@eclipse-che/theia-remote-api/lib/node/che-server-workspace-service-impl.js:184:31)
    at step (/home/theia/node_modules/@eclipse-che/theia-remote-api/lib/node/che-server-workspace-service-impl.js:53:23)
    at Object.throw (/home/theia/node_modules/@eclipse-che/theia-remote-api/lib/node/che-server-workspace-service-impl.js:34:53)
    at rejected (/home/theia/node_modules/@eclipse-che/theia-remote-api/lib/node/che-server-workspace-service-impl.js:26:65)
    at processTicksAndRejections (internal/process/task_queues.js:97:5)
2020-12-02 20:17:02.529 root ERROR Request findUniqueEndpointByAttribute failed with error: Unable to get workspace containers. Cause: [object Object] Params: Error: Unable to get workspace containers. Cause: [object Object]
    at CheServerWorkspaceServiceImpl.<anonymous> (/home/theia/node_modules/@eclipse-che/theia-remote-api/lib/node/che-server-workspace-service-impl.js:184:31)
    at step (/home/theia/node_modules/@eclipse-che/theia-remote-api/lib/node/che-server-workspace-service-impl.js:53:23)
    at Object.throw (/home/theia/node_modules/@eclipse-che/theia-remote-api/lib/node/che-server-workspace-service-impl.js:34:53)
    at rejected (/home/theia/node_modules/@eclipse-che/theia-remote-api/lib/node/che-server-workspace-service-impl.js:26:65)
    at processTicksAndRejections (internal/process/task_queues.js:97:5)
2020-12-02 20:17:03.206 root ERROR Request currentWorkspace failed with error: connect ECONNREFUSED 127.0.0.1:9999 Params:

The workspace view isn't populated, project didn't clone etc. Not exactly sure why though

sleshchenko · 2020-12-03T09:31:18Z

After I found and fixed bug with empty annotations (I had introduced before), AllInOne Theia and Theia Next work just fine on OpenShift 4.7 powered by cluster bot.

and cloud shell as well.

sleshchenko · 2020-12-03T09:33:34Z

I assume permissions issues could be related to mkdir which currently implemented in a wrong way, and it's actual for master branch as well.
Theia is not able to access to Che API Sidecar, that's strange and should be investigated more if we can reproduce it, but I don't see how it may be related to that PR.

sleshchenko · 2020-12-03T09:35:29Z

There are no critical objections against this PR, so, I'm merging it not to inflate this PR even more. I will be happy to address any feedback with separate PRs if any.

sleshchenko · 2020-12-03T10:13:31Z

🤦‍♂️ I forgot to push made fixes, so they are merged in #213

sleshchenko · 2020-12-08T09:28:56Z

it may just be a bit of weird trivia, but I think we can get around this by making sure che-rest-apis is the first container in the pod spec. Kube starts containers in the order they're listed, iirc. If che-rest-apis is temporary this may be a worthwhile workaround.

Quickly tried and it does not help

sleshchenko · 2020-12-08T10:20:48Z

I was quick in my conclusion. The error above is caused by Theia 7.20 which has issues with single host. Seems it helps to workaround an issue and I'll create PR

openshift-ci-robot added the do-not-merge/work-in-progress label Nov 30, 2020

openshift-ci-robot requested review from amisevsk and JPinkney November 30, 2020 07:55

openshift-ci-robot added the approved label Nov 30, 2020

sleshchenko force-pushed the tls branch 2 times, most recently from 7302dc0 to 66d885d Compare November 30, 2020 13:41

sleshchenko marked this pull request as ready for review November 30, 2020 14:27

openshift-ci-robot removed the do-not-merge/work-in-progress label Nov 30, 2020

sleshchenko force-pushed the tls branch from 66d885d to 4e622d9 Compare November 30, 2020 15:34

sleshchenko self-assigned this Nov 30, 2020

amisevsk approved these changes Nov 30, 2020

View reviewed changes

openshift-ci-robot assigned amisevsk Nov 30, 2020

openshift-ci-robot added lgtm and removed lgtm labels Nov 30, 2020

JPinkney reviewed Nov 30, 2020

View reviewed changes

amisevsk reviewed Nov 30, 2020

View reviewed changes

sleshchenko force-pushed the tls branch from 27503c8 to a98619d Compare December 1, 2020 10:46

sleshchenko requested a review from amisevsk December 1, 2020 13:52

amisevsk reviewed Dec 1, 2020

View reviewed changes

sleshchenko changed the title ~~SingleHost + TLS on OpenShift~~ WIP SingleHost + TLS on OpenShift Dec 1, 2020

sleshchenko changed the title ~~WIP SingleHost + TLS on OpenShift~~ SingleHost + TLS on OpenShift Dec 2, 2020

openshift-ci-robot removed the do-not-merge/work-in-progress label Dec 2, 2020

sleshchenko added 9 commits December 2, 2020 12:48

Tiny code clean up

602fe76

Clean up openshift oauth routing to make things with terminal clearer

3d3ae25

Enable TLS for basic routing on OpenShift

9f7071b

Add Theia Next sample

06f5b0d

Expose restricted access annotation with README.md

6a6ca31

Fix evaluation of the endpoint URL

f977fb7

Disable single host for openshift OAuth routing

8375872

Provide creator access only for workspace annotated with restricted-a…

bd01489

…ccess

Allow workspace modification if it does not have restricted-access en…

1250c5c

…abled

sleshchenko force-pushed the tls branch from 5dc1a9f to 1250c5c Compare December 2, 2020 10:48

Remove operator sdk from PROW CI since it's unused

b5f73f3

sleshchenko merged commit 84a728e into devfile:master Dec 3, 2020

sleshchenko deleted the tls branch December 3, 2020 09:35

This was referenced Dec 3, 2020

Support basic TLS routing #200

Closed

Single Host + TLS on OpenShift fixup #213

Merged

sleshchenko mentioned this pull request Dec 8, 2020

Place Che API Sidecar as the first container #223

Merged

sleshchenko mentioned this pull request Feb 18, 2021

Use bearertoken when trying to idle terminals redhat-developer/web-terminal-exec#17

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SingleHost + TLS on OpenShift #201

SingleHost + TLS on OpenShift #201

sleshchenko commented Nov 30, 2020 •

edited

Loading

openshift-ci-robot commented Nov 30, 2020

sleshchenko commented Nov 30, 2020

amisevsk left a comment

amisevsk Nov 30, 2020

sleshchenko Nov 30, 2020 •

edited

Loading

JPinkney Nov 30, 2020

sleshchenko Dec 1, 2020

openshift-ci-robot commented Nov 30, 2020

openshift-ci-robot commented Nov 30, 2020

JPinkney Nov 30, 2020

amisevsk left a comment

amisevsk Nov 30, 2020

sleshchenko Dec 1, 2020

sleshchenko Dec 1, 2020

sleshchenko commented Dec 1, 2020

sleshchenko commented Dec 1, 2020

amisevsk left a comment •

edited

Loading

amisevsk commented Dec 1, 2020 •

edited

Loading

sleshchenko commented Dec 1, 2020

amisevsk commented Dec 1, 2020

sleshchenko commented Dec 1, 2020

sleshchenko commented Dec 2, 2020 •

edited

Loading

sleshchenko commented Dec 2, 2020

sleshchenko commented Dec 2, 2020

sleshchenko commented Dec 2, 2020

JPinkney commented Dec 2, 2020 •

edited

Loading

sleshchenko commented Dec 3, 2020

sleshchenko commented Dec 3, 2020

sleshchenko commented Dec 3, 2020

sleshchenko commented Dec 3, 2020

sleshchenko commented Dec 8, 2020

sleshchenko commented Dec 8, 2020

	# it's important to make workspace immutable to make sure nobody with right access
	# it's important to restrict access to the workspace to make sure other users with sufficient privileges

SingleHost + TLS on OpenShift #201

SingleHost + TLS on OpenShift #201

Conversation

sleshchenko commented Nov 30, 2020 • edited Loading

What does this PR do?

TODOs:

What issues does this PR fix or reference?

Is it tested? How?

Minikube

CRC

openshift-ci-robot commented Nov 30, 2020

sleshchenko commented Nov 30, 2020

amisevsk left a comment

Choose a reason for hiding this comment

amisevsk Nov 30, 2020

Choose a reason for hiding this comment

sleshchenko Nov 30, 2020 • edited Loading

Choose a reason for hiding this comment

JPinkney Nov 30, 2020

Choose a reason for hiding this comment

sleshchenko Dec 1, 2020

Choose a reason for hiding this comment

openshift-ci-robot commented Nov 30, 2020

openshift-ci-robot commented Nov 30, 2020

JPinkney Nov 30, 2020

Choose a reason for hiding this comment

amisevsk left a comment

Choose a reason for hiding this comment

amisevsk Nov 30, 2020

Choose a reason for hiding this comment

sleshchenko Dec 1, 2020

Choose a reason for hiding this comment

sleshchenko Dec 1, 2020

Choose a reason for hiding this comment

sleshchenko commented Dec 1, 2020

sleshchenko commented Dec 1, 2020

amisevsk left a comment • edited Loading

Choose a reason for hiding this comment

amisevsk commented Dec 1, 2020 • edited Loading

sleshchenko commented Dec 1, 2020

amisevsk commented Dec 1, 2020

sleshchenko commented Dec 1, 2020

sleshchenko commented Dec 2, 2020 • edited Loading

sleshchenko commented Dec 2, 2020

sleshchenko commented Dec 2, 2020

sleshchenko commented Dec 2, 2020

JPinkney commented Dec 2, 2020 • edited Loading

For samples/theia-next.yaml

For samples/all-in-one-theia-nodejs.devworkspace.yaml

sleshchenko commented Dec 3, 2020

sleshchenko commented Dec 3, 2020

sleshchenko commented Dec 3, 2020

sleshchenko commented Dec 3, 2020

sleshchenko commented Dec 8, 2020

sleshchenko commented Dec 8, 2020

sleshchenko commented Nov 30, 2020 •

edited

Loading

sleshchenko Nov 30, 2020 •

edited

Loading

amisevsk left a comment •

edited

Loading

amisevsk commented Dec 1, 2020 •

edited

Loading

sleshchenko commented Dec 2, 2020 •

edited

Loading

JPinkney commented Dec 2, 2020 •

edited

Loading