Fix dashboard + proxy incompatibility #441

wbuchwalter · 2018-03-07T14:55:02Z

Fixes #323 .
The frontend is now using hashRouter instead of browserRouter, meaning the location of the UI is determined only by the portion of the url after the # (similar to k8s dashboard).

Request to the backend are also properly routed now.
The backend request are sent to the portion of the url until the first occurence of tfjobs + /api
i.e:
http://127.0.0.1:8001/api/v1/namespaces/kubeflow/services/ambassador:80/proxy/tfjobs/ui/#/new
will translate into: http://127.0.0.1:8001/api/v1/namespaces/kubeflow/services/ambassador:80/proxy/tfjobs/api

This means this would break if we decide to route the dashboard with ambassador on /dashboard instead of /tfjob, so the route should be part of a dashboard config at some point.

See kubeflow/kubeflow#381 for the related route change in ambassador.

cc @jlewi @jimexist

This change is

coveralls · 2018-03-07T15:07:10Z

Coverage remained the same at 45.408% when pulling ccb6f34 on wbuchwalter:dashboard-proxy into ebb0053 on kubeflow:master.

jlewi · 2018-03-08T04:10:24Z

This fantastic thanks for doing this.

jlewi · 2018-03-08T04:11:01Z

dashboard/backend/handler/api_handler.go

@@ -61,7 +61,7 @@ func CreateHTTPAPIHandler(client client.ClientManager) (http.Handler, error) {

 	apiV1Ws := new(restful.WebService)

-	apiV1Ws.Path("/api").
+	apiV1Ws.Path("/tfjobs/api").


Can you capture the relevant details from the PR description in a comment here or wherever it makes sense?

Sure.
So the biggest issue was figuring out which part of the url corresponded to the application itself, and which parts were related to the proxying.
If we have this url with kubectl proxy + ambassador:
127.0.0.1:8001/api/v1/namespaces/kubeflow/services/ambassador:80/proxy/tfjobs/ui/
Proxy parts are:
127.0.0.1:8001/api/v1/namespaces/kubeflow/services/ambassador:80/proxy
Application parts are (depending on how the rewrite is configured in ambassador):
/tfjobs/ui/

So the way the application handles it, is finding the first occurence of tfjobs in the url and appending /api/ to reach the backend.

Finally, by rewriting in ambassador to /tfjobs/and not / and by having the backend listening on /tfjobs/ui and /tfjobs/api we ensure that the logic won't break when not using ambassador, i.e:

When using a dev server: 127.0.0.1:3000/tfjobs/ui

When proxying directly on the dashboard service:
http://127.0.0.1:8001/api/v1/namespaces/kubeflow/services/tf-job-dashboard:80/proxy/tfjobs/ui/#/

Please make this a comment in the code.

wbuchwalter · 2018-03-08T14:18:22Z

dashboard/frontend/package.json

@@ -2,6 +2,7 @@
  "name": "tensorflowk8s-dashboard",
  "version": "0.1.0",
  "description": "Dashboard for kubeflow/tf-operator.",
+  "homepage": ".",


Homepage defines where the frontend will query for the static assets. If not specified the static assets will be queried at the root, i.e: 127.0.0.1:8001/static which breaks.
"." means that we query at the path where the application is served. i.e: http://127.0.0.1:8001/api/v1/namespaces/kubeflow/services/ambassador:80/proxy/tfjobs/ui/static

Please make this a comment in the code.

wbuchwalter · 2018-03-08T14:28:14Z

dashboard/frontend/src/components/App.js

@@ -2,7 +2,7 @@ import React from "react";
 import MuiThemeProvider from "material-ui/styles/MuiThemeProvider";
 import getMuiTheme from "material-ui/styles/getMuiTheme";
 import FlatButton from "material-ui/FlatButton";
-import { BrowserRouter as Router, Link } from "react-router-dom";
+import { HashRouter as Router, Link } from "react-router-dom";


BrowserRouter is a traditional router, If I currently am at the root / and I navigate to /new, the resulting route will be: ${baseroute}/new. However this breaks with ambassador:
If I am at /api/v1/namespaces/kubeflow/services/ambassador:80/proxy/tfjobs/ui/ and I navigate to /new I still end up on /new.
And we also can't just figure out the proxy part of the url and preprend it to each link, such as: /api/v1/namespaces/kubeflow/services/ambassador:80/proxy/tfjobs/ui/new since the go backend is configured to only handle /tfjobs/ui not /api/v1/.....

So instead we can use the HashRouter, which will only use the part of the url located after a # to infer it's current state.
So /api/v1/namespaces/kubeflow/services/ambassador:80/proxy/tfjobs/ui/# will become /api/v1/namespaces/kubeflow/services/ambassador:80/proxy/tfjobs/ui/#/new. This works regardless of how the dashboard is accessed. This is also how the k8s dashboard works.

Please make this a comment in the code.

wbuchwalter · 2018-03-12T13:25:03Z

@jimexist Any chance you can review this?

jlewi · 2018-03-17T22:10:45Z

Looks good except please make the explanation comments in the code.

It might be useful to capture guidance in our developer guide on how to write react APPs so that they work correctly with Ambassador.

wbuchwalter · 2018-03-19T15:14:39Z

@jlewi Added the comments in the code.

jimexist · 2018-03-20T00:01:52Z

looks good to me but feeling sad that ambassador couldn't have fixed all this and an application has to adapt and use a different frontent router (I am thinking why not using a subdomain, e.g.)

jlewi · 2018-03-20T13:43:45Z

/retest

jlewi · 2018-03-20T13:44:47Z

Thanks. Looks good but looks like there was a problem running the tests. I retriggered them.

/assign jlewi

wbuchwalter · 2018-03-20T14:19:49Z

@jlewi Tests are fixed, had to pull master.

jlewi · 2018-03-20T17:05:04Z

/lgtm
/approve

k8s-ci-robot · 2018-03-20T17:05:10Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jlewi

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:

~~dashboard/OWNERS~~ [jlewi]

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

* dashboard: dev guide * fix dashboard + proxy issue * dashboard: move to hash router * [dashboard] Add comments about routes and proxying

wbuchwalter added 3 commits February 27, 2018 16:09

dashboard: dev guide

2ccfafa

fix dashboard + proxy issue

91f1e00

dashboard: move to hash router

77fbc52

k8s-ci-robot added the size/M label Mar 7, 2018

k8s-ci-robot requested review from jimexist and ScorpioCPH March 7, 2018 14:55

Merge branch 'master' into dashboard-proxy

bd249f9

k8s-ci-robot added size/S and removed size/M labels Mar 7, 2018

This was referenced Mar 7, 2018

Fix tfjob dashboard ambassador route kubeflow/kubeflow#381

Merged

Add proxying to front-end development server. #442

Merged

jlewi reviewed Mar 8, 2018

View reviewed changes

wbuchwalter commented Mar 8, 2018

View reviewed changes

jlewi mentioned this pull request Mar 17, 2018

Guide for writing react apps that work with ambassador kubeflow/kubeflow#448

Closed

[dashboard] Add comments about routes and proxying

a081064

k8s-ci-robot added size/M and removed size/S labels Mar 19, 2018

k8s-ci-robot assigned jlewi Mar 20, 2018

Merge branch 'master' into dashboard-proxy

ccb6f34

k8s-ci-robot added the lgtm label Mar 20, 2018

k8s-ci-robot added the approved label Mar 20, 2018

k8s-ci-robot merged commit 181cb05 into kubeflow:master Mar 20, 2018

gaocegege mentioned this pull request Mar 22, 2018

[v1alpha2] Sync commits with v1alpha1 #490

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix dashboard + proxy incompatibility #441

Fix dashboard + proxy incompatibility #441

wbuchwalter commented Mar 7, 2018 •

edited

Loading

coveralls commented Mar 7, 2018 •

edited

Loading

jlewi commented Mar 8, 2018

jlewi Mar 8, 2018

wbuchwalter Mar 8, 2018

jlewi Mar 17, 2018

wbuchwalter Mar 8, 2018

jlewi Mar 17, 2018

wbuchwalter Mar 8, 2018

jlewi Mar 17, 2018

wbuchwalter commented Mar 12, 2018

jlewi commented Mar 17, 2018

wbuchwalter commented Mar 19, 2018

jimexist commented Mar 20, 2018

jlewi commented Mar 20, 2018

jlewi commented Mar 20, 2018

wbuchwalter commented Mar 20, 2018

jlewi commented Mar 20, 2018

k8s-ci-robot commented Mar 20, 2018

Fix dashboard + proxy incompatibility #441

Fix dashboard + proxy incompatibility #441

Conversation

wbuchwalter commented Mar 7, 2018 • edited Loading

coveralls commented Mar 7, 2018 • edited Loading

jlewi commented Mar 8, 2018

jlewi Mar 8, 2018

Choose a reason for hiding this comment

wbuchwalter Mar 8, 2018

Choose a reason for hiding this comment

jlewi Mar 17, 2018

Choose a reason for hiding this comment

wbuchwalter Mar 8, 2018

Choose a reason for hiding this comment

jlewi Mar 17, 2018

Choose a reason for hiding this comment

wbuchwalter Mar 8, 2018

Choose a reason for hiding this comment

jlewi Mar 17, 2018

Choose a reason for hiding this comment

wbuchwalter commented Mar 12, 2018

jlewi commented Mar 17, 2018

wbuchwalter commented Mar 19, 2018

jimexist commented Mar 20, 2018

jlewi commented Mar 20, 2018

jlewi commented Mar 20, 2018

wbuchwalter commented Mar 20, 2018

jlewi commented Mar 20, 2018

k8s-ci-robot commented Mar 20, 2018

wbuchwalter commented Mar 7, 2018 •

edited

Loading

coveralls commented Mar 7, 2018 •

edited

Loading