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 errors middleware to redirect on idled workspace after 4 seconds #1392

Merged
merged 2 commits into from
Jun 16, 2022

Conversation

dkwon17
Copy link
Contributor

@dkwon17 dkwon17 commented May 24, 2022

Signed-off-by: David Kwon [email protected]

What does this PR do?

Alternative PR to #1386. The main difference is that this PR uses the Errors Traefik middleware.

Screenshot/screencast of this PR

In this gif we see that:

  1. Workspace is stopped
  2. IDE page is refreshed
  3. Instead of a 504 error, the user is redirected to the dashboard after 4 seconds

errorsmiddleware

What issues does this PR fix or reference?

Part of eclipse-che/che#20599. In another PR, maybe we can redirect the user to some error page provided by the dashboard.

How to test this PR?

Tested on CRC with the following:

  1. Deploy
chectl server:deploy \
     --platform crc\
     --che-operator-image quay.io/dkwon17/che-operator:useerrorsmiddleware
  1. Open the dashboard with the URL provided by the output from chectl.
  2. Start a workspace, and wait until the editor opens.
  3. Stop the workspace.
  4. Refresh the editor. You should be redirected to the dashboard after 4 seconds where you can start the workspace again.

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.

@openshift-ci
Copy link

openshift-ci bot commented May 24, 2022

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

@dkwon17 dkwon17 force-pushed the useerrorsmiddleware branch from e83a46c to 89698d4 Compare June 1, 2022 18:44
@codecov
Copy link

codecov bot commented Jun 1, 2022

Codecov Report

Merging #1392 (c130e99) into main (705d653) will increase coverage by 0.05%.
The diff coverage is 100.00%.

❗ Current head c130e99 differs from pull request most recent head 67182b0. Consider uploading reports for the commit 67182b0 to get more accurate results

@@            Coverage Diff             @@
##             main    #1392      +/-   ##
==========================================
+ Coverage   63.12%   63.18%   +0.05%     
==========================================
  Files          70       70              
  Lines        5804     5834      +30     
==========================================
+ Hits         3664     3686      +22     
- Misses       1777     1785       +8     
  Partials      363      363              
Impacted Files Coverage Δ
controllers/devworkspace/solver/che_routing.go 79.52% <100.00%> (+0.55%) ⬆️
pkg/deploy/gateway/traefik_config_util.go 91.80% <100.00%> (+2.00%) ⬆️
pkg/common/operator-defaults/defaults.go 15.97% <0.00%> (-0.80%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 705d653...67182b0. Read the comment docs.

@dkwon17 dkwon17 force-pushed the useerrorsmiddleware branch from 89698d4 to 37955dd Compare June 1, 2022 19:45
@benoitf
Copy link
Contributor

benoitf commented Jun 1, 2022

I was wondering if we should redirect to the workspace page in dashboard instead of the home page.

@dkwon17
Copy link
Contributor Author

dkwon17 commented Jun 2, 2022

The workspace page is <CHE_HOST>/dashboard/#/workspaces is that right?

On the Traeffik side with the Traeffik errors middleware, the content after the # is considered a URL fragment and is ignored because it is a browser-concept: traefik/traefik#9022 (comment)

To support redirection to the workspaces page, I think we would need to make some changes where the dashboard server receives the HTTP request.

@dkwon17
Copy link
Contributor Author

dkwon17 commented Jun 7, 2022

There is a bug somewhere in this implementation. When restarting an idled workspace, instead of opening the IDE, I'm redirected to the dashboard. I am investigating

EDIT: the latest commit (3f85fd3) fixes this issue

@dkwon17
Copy link
Contributor Author

dkwon17 commented Jun 9, 2022

This PR is ready to review. This PR adds two middlewares (errors and headers) to the workspace Traefik config:

Here is an example config diff reflecting what this PR adds:

		  middlewares:
		    workspace27366e668ca645c9-auth:
		      forwardAuth:
		        address: http://127.0.0.1:8089?namespace=admin-che
		        trustForwardHeader: false
+		    workspace27366e668ca645c9-errors:
+		      errors:
+		        query: /
+		        service: che-dashboard
+		        status: 500-599
+		    workspace27366e668ca645c9-headers:
+		      headers:
+		        customResponseHeaders:
+		          cache-control: no-store, max-age=0
		    workspace27366e668ca645c9-strip-prefix:
		      stripPrefix:
		        prefixes:
		        - /workspace27366e668ca645c9
		  routers:
		    workspace27366e668ca645c9:
		      middlewares:
		      - workspace27366e668ca645c9-strip-prefix
		      - workspace27366e668ca645c9-auth
+		      - workspace27366e668ca645c9-headers
+		      - workspace27366e668ca645c9-errors
		      priority: 100
		      rule: PathPrefix(`/workspace27366e668ca645c9`)
		      service: workspace27366e668ca645c9
		    workspace27366e668ca645c9-3100-healthz:
		      middlewares:
		      - workspace27366e668ca645c9-strip-prefix
		      priority: 101
		      rule: Path(`/workspace27366e668ca645c9/theia-ide/3100/healthz`)
		      service: workspace27366e668ca645c9
+		  serversTransports:
+		    workspace27366e668ca645c9:
+		      forwardingTimeouts:
+		        dialTimeout: 4s
		  services:
		    workspace27366e668ca645c9:
		      loadBalancer:
		        servers:
		        - url: http://workspace27366e668ca645c9-service.admin-che.svc:3030
		        serversTransport: workspace27366e668ca645c9

The headers middleware was added to deal with this issue with browser cache: #1392 (comment).

The serverTransport config was added to add a 4 second timeout for when trying to access the workspace url. If the workspace server cannot be reached after 4 seconds, then, the errors middleware will kick in and redirect requests to the workspace url to the dashboard.

@dkwon17 dkwon17 marked this pull request as ready for review June 9, 2022 21:33
@openshift-ci
Copy link

openshift-ci bot commented Jun 10, 2022

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: dkwon17, ibuziuk, tolusha

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

@dkwon17 dkwon17 force-pushed the useerrorsmiddleware branch from c130e99 to 67182b0 Compare June 15, 2022 16:10
@openshift-ci openshift-ci bot removed the lgtm label Jun 15, 2022
@openshift-ci
Copy link

openshift-ci bot commented Jun 15, 2022

New changes are detected. LGTM label has been removed.

@dkwon17
Copy link
Contributor Author

dkwon17 commented Jun 15, 2022

/retest

@dkwon17 dkwon17 force-pushed the useerrorsmiddleware branch from 67182b0 to ef96f57 Compare June 15, 2022 20:44
@openshift-ci
Copy link

openshift-ci bot commented Jun 15, 2022

@dkwon17: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/v9-devworkspace-happy-path ef96f57 link true /test v9-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.

@dkwon17
Copy link
Contributor Author

dkwon17 commented Jun 16, 2022

/retest

@tolusha
Copy link
Contributor

tolusha commented Jun 16, 2022

@dkwon17
v9-devworkspace-happy-path is unstable.
I checked and results look good.
Pls merge.

@dkwon17
Copy link
Contributor Author

dkwon17 commented Jun 16, 2022

Thank you @tolusha , will merge

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.

5 participants