From dd7d1b1c2e2bc5fc84a74df9cd360636b84b658d Mon Sep 17 00:00:00 2001 From: William Chargin Date: Sat, 5 Oct 2019 19:00:43 -0700 Subject: [PATCH 1/9] application: factor out middleware installation Summary: Currently, we apply the `_handle_errors` middleware as a decorator directly on `__call__`. We plan to add middleware that is parameterized on instance variables, so this pattern will no longer suffice. This commit simply refactors the existing code to apply middleware at initialization time. Test Plan: It suffices to run `bazel test //tensorboard/backend:application_test`. wchargin-branch: app-middleware wchargin-source: 77c4f793de8b64d88bd10212db5a0191995b29ca --- tensorboard/backend/application.py | 35 +++++++++++++++++++++--------- 1 file changed, 25 insertions(+), 10 deletions(-) diff --git a/tensorboard/backend/application.py b/tensorboard/backend/application.py index 1f6ad959da..fc98f90b00 100644 --- a/tensorboard/backend/application.py +++ b/tensorboard/backend/application.py @@ -323,6 +323,13 @@ def __init__(self, plugins, path_prefix=''): key=lambda x: len(x[0]), reverse=True)) + self._app = self._create_wsgi_app() + + def _create_wsgi_app(self): + """Apply middleware to create the final WSGI app.""" + app = self._route_request + app = _handling_errors(app) + return app @wrappers.Request.application def _serve_plugins_listing(self, request): @@ -392,23 +399,31 @@ def _serve_plugins_listing(self, request): response[plugin.plugin_name] = output_metadata return http_util.Respond(request, response, 'application/json') - @_handling_errors - def __call__(self, environ, start_response): # pylint: disable=invalid-name + def __call__(self, environ, start_response): """Central entry point for the TensorBoard application. - This method handles routing to sub-applications. It does simple routing - using strict string matching. Regular expressions are not supported. - Wildcard routes such as `/foo/*` are supported as a special case. - This __call__ method conforms to the WSGI spec, so that instances of this class are WSGI applications. Args: - environ: See WSGI spec. - start_response: See WSGI spec. + environ: See WSGI spec (PEP 3333). + start_response: See WSGI spec (PEP 3333). + """ + return self._app(environ, start_response) - Returns: - A werkzeug Response. + def _route_request(self, environ, start_response): + """Delegate an incoming request to sub-applications. + + This method supports strict string matching and wildcard routes of a + single path component, such as `/foo/*`. Other routing patterns, + like regular expressions, are not supported. + + This is the main TensorBoard entry point before middleware is + applied. (See `_create_wsgi_app`.) + + Args: + environ: See WSGI spec (PEP 3333). + start_response: See WSGI spec (PEP 3333). """ request = wrappers.Request(environ) parsed_url = urlparse.urlparse(request.path) From 2f60e5cdbb9fe41bb3f9cd591b1ec0e304ced773 Mon Sep 17 00:00:00 2001 From: William Chargin Date: Sat, 5 Oct 2019 19:01:00 -0700 Subject: [PATCH 2/9] application: extract path prefix logic to middleware MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Summary: This way, we can reason about and test it in isolation as we add more middleware to the application. Note that when using `--path_prefix`, loading the main TensorBoard page without a trailing slash on the URL has long been broken (#1117). This commit does not fix that, but changes the exact failure mode: rather than 404ing, we now load a broken TensorBoard (relative URLs don’t resolve). A follow-up commit will fix this properly. Test Plan: Verify that TensorBoard still works fully, with `--path_prefix` and without it. wchargin-branch: path-prefix-middleware wchargin-source: 3e07d2b681f12c4bab310bef2afc066590ae5cb7 --- tensorboard/backend/BUILD | 24 +++++ tensorboard/backend/application.py | 30 +++---- tensorboard/backend/path_prefix.py | 65 ++++++++++++++ tensorboard/backend/path_prefix_test.py | 112 ++++++++++++++++++++++++ 4 files changed, 214 insertions(+), 17 deletions(-) create mode 100644 tensorboard/backend/path_prefix.py create mode 100644 tensorboard/backend/path_prefix_test.py diff --git a/tensorboard/backend/BUILD b/tensorboard/backend/BUILD index 901fe86c45..90cccd19dd 100644 --- a/tensorboard/backend/BUILD +++ b/tensorboard/backend/BUILD @@ -63,6 +63,7 @@ py_library( visibility = ["//visibility:public"], deps = [ ":http_util", + ":path_prefix", "//tensorboard:errors", "//tensorboard:expect_sqlite3_installed", "//tensorboard/backend/event_processing:data_provider", @@ -97,6 +98,29 @@ py_test( ], ) +py_library( + name = "path_prefix", + srcs = ["path_prefix.py"], + srcs_version = "PY2AND3", + deps = [ + "//tensorboard:errors", + ], +) + +py_test( + name = "path_prefix_test", + size = "small", + srcs = ["path_prefix_test.py"], + srcs_version = "PY2AND3", + tags = ["support_notf"], + deps = [ + ":path_prefix", + "//tensorboard:errors", + "//tensorboard:test", + "@org_pocoo_werkzeug", + ], +) + py_library( name = "process_graph", srcs = ["process_graph.py"], diff --git a/tensorboard/backend/application.py b/tensorboard/backend/application.py index fc98f90b00..0ceb68eebb 100644 --- a/tensorboard/backend/application.py +++ b/tensorboard/backend/application.py @@ -40,6 +40,7 @@ from tensorboard import errors from tensorboard.backend import http_util +from tensorboard.backend import path_prefix from tensorboard.backend.event_processing import db_import_multiplexer from tensorboard.backend.event_processing import data_provider as event_data_provider # pylint: disable=line-too-long from tensorboard.backend.event_processing import plugin_event_accumulator as event_accumulator # pylint: disable=line-too-long @@ -258,8 +259,7 @@ def __init__(self, plugins, path_prefix=''): # TODO(@chihuahua): Delete this RPC once we have skylark rules that # obviate the need for the frontend to determine which plugins are # active. - self._path_prefix + DATA_PREFIX + PLUGINS_LISTING_ROUTE: - self._serve_plugins_listing, + DATA_PREFIX + PLUGINS_LISTING_ROUTE: self._serve_plugins_listing, } unordered_prefix_routes = {} @@ -291,10 +291,11 @@ def __init__(self, plugins, path_prefix=''): 'route does not start with a slash' % (plugin.plugin_name, route)) if type(plugin) is core_plugin.CorePlugin: # pylint: disable=unidiomatic-typecheck - path = self._path_prefix + route + path = route else: - path = (self._path_prefix + DATA_PREFIX + PLUGIN_PREFIX + '/' + - plugin.plugin_name + route) + path = ( + DATA_PREFIX + PLUGIN_PREFIX + '/' + plugin.plugin_name + route + ) if path.endswith('/*'): # Note we remove the '*' but leave the slash in place. @@ -328,6 +329,7 @@ def __init__(self, plugins, path_prefix=''): def _create_wsgi_app(self): """Apply middleware to create the final WSGI app.""" app = self._route_request + app = path_prefix.PathPrefixMiddleware(app, self._path_prefix) app = _handling_errors(app) return app @@ -384,7 +386,7 @@ def _serve_plugins_listing(self, request): loading_mechanism = { 'type': 'IFRAME', 'module_path': ''.join([ - self._path_prefix, DATA_PREFIX, PLUGIN_PREFIX, '/', + request.script_root, DATA_PREFIX, PLUGIN_PREFIX, '/', plugin.plugin_name, es_module_handler, ]), } @@ -427,7 +429,7 @@ def _route_request(self, environ, start_response): """ request = wrappers.Request(environ) parsed_url = urlparse.urlparse(request.path) - clean_path = _clean_path(parsed_url.path, self._path_prefix) + clean_path = _clean_path(parsed_url.path) # pylint: disable=too-many-function-args if clean_path in self.exact_routes: @@ -579,22 +581,16 @@ def _get_connect_params(query): return {k: json.loads(v[0]) for k, v in params.items()} -def _clean_path(path, path_prefix=""): - """Cleans the path of the request. - - Removes the ending '/' if the request begins with the path prefix and pings a - non-empty route. +def _clean_path(path): + """Removes a trailing slash from a non-root path. Arguments: path: The path of a request. - path_prefix: The prefix string that every route of this TensorBoard instance - starts with. Returns: - The route to use to serve the request (with the path prefix stripped if - applicable). + The route to use to serve the request. """ - if path != path_prefix + '/' and path.endswith('/'): + if path != '/' and path.endswith('/'): return path[:-1] return path diff --git a/tensorboard/backend/path_prefix.py b/tensorboard/backend/path_prefix.py new file mode 100644 index 0000000000..2192deb9f7 --- /dev/null +++ b/tensorboard/backend/path_prefix.py @@ -0,0 +1,65 @@ +# Copyright 2019 The TensorFlow Authors. All Rights Reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# ============================================================================== +"""Internal path prefix support for TensorBoard. + +Using a path prefix of `/foo/bar` enables TensorBoard to serve from +`http://localhost:6006/foo/bar/` rather than `http://localhost:6006/`. +See the `--path_prefix` flag docs for more details. +""" + +from __future__ import absolute_import +from __future__ import division +from __future__ import print_function + +from tensorboard import errors + + +class PathPrefixMiddleware(object): + """WSGI middleware for path prefixes. + + All requests to this middleware must begin with the specified path + prefix (otherwise, a 404 will be returned immediately). Requests will + be forwarded to the underlying application with the path prefix + stripped and appended to `SCRIPT_NAME` (see the WSGI spec, PEP 3333, + for details). + """ + + def __init__(self, application, path_prefix): + """Initializes this middleware. + + Args: + application: The WSGI application to wrap (see PEP 3333). + path_prefix: A string path prefix to be stripped from incoming + requests. If empty, this middleware is a no-op. If non-empty, + the path prefix must start with a slash and not end with one + (e.g., "/tensorboard"). + """ + if path_prefix.endswith("/"): + raise ValueError("Path prefix must not end with slash: %r" % path_prefix) + if path_prefix and not path_prefix.startswith("/"): + raise ValueError( + "Non-empty path prefix must start with slash: %r" % path_prefix + ) + self._application = application + self._path_prefix = path_prefix + self._strict_prefix = self._path_prefix + "/" + + def __call__(self, environ, start_response): + path = environ.get("PATH_INFO", "") + if path != self._path_prefix and not path.startswith(self._strict_prefix): + raise errors.NotFoundError() + environ["PATH_INFO"] = path[len(self._path_prefix):] + environ["SCRIPT_NAME"] = environ.get("SCRIPT_NAME", "") + self._path_prefix + return self._application(environ, start_response) diff --git a/tensorboard/backend/path_prefix_test.py b/tensorboard/backend/path_prefix_test.py new file mode 100644 index 0000000000..b4a9c5421b --- /dev/null +++ b/tensorboard/backend/path_prefix_test.py @@ -0,0 +1,112 @@ +# Copyright 2019 The TensorFlow Authors. All Rights Reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# ============================================================================== +"""Tests for `tensorboard.backend.path_prefix`.""" + +from __future__ import absolute_import +from __future__ import division +from __future__ import print_function + +import json + +import werkzeug + +from tensorboard import errors +from tensorboard import test as tb_test +from tensorboard.backend import path_prefix + + +class PathPrefixMiddlewareTest(tb_test.TestCase): + """Tests for `PathPrefixMiddleware`.""" + + def _echo_app(self, environ, start_response): + # https://www.python.org/dev/peps/pep-0333/#environ-variables + data = { + "path": environ.get("PATH_INFO", ""), + "script": environ.get("SCRIPT_NAME", ""), + } + body = json.dumps(data, sort_keys=True) + start_response("200 OK", [("Content-Type", "application/json")]) + return [body] + + def _assert_ok(self, response, path, script): + self.assertEqual(response.status_code, 200) + actual = json.loads(response.get_data()) + expected = dict(path=path, script=script) + self.assertEqual(actual, expected) + + def test_bad_path_prefix_without_leading_slash(self): + with self.assertRaises(ValueError) as cm: + path_prefix.PathPrefixMiddleware(self._echo_app, "hmm") + msg = str(cm.exception) + self.assertIn("must start with slash", msg) + self.assertIn(repr("hmm"), msg) + + def test_bad_path_prefix_with_trailing_slash(self): + with self.assertRaises(ValueError) as cm: + path_prefix.PathPrefixMiddleware(self._echo_app, "/hmm/") + msg = str(cm.exception) + self.assertIn("must not end with slash", msg) + self.assertIn(repr("/hmm/"), msg) + + def test_empty_path_prefix(self): + app = path_prefix.PathPrefixMiddleware(self._echo_app, "") + server = werkzeug.test.Client(app, werkzeug.BaseResponse) + + with self.subTest("at empty"): + self._assert_ok(server.get(""), path="", script="") + + with self.subTest("at root"): + self._assert_ok(server.get("/"), path="/", script="") + + with self.subTest("at subpath"): + response = server.get("/foo/bar") + self._assert_ok(server.get("/foo/bar"), path="/foo/bar", script="") + + def test_nonempty_path_prefix(self): + app = path_prefix.PathPrefixMiddleware(self._echo_app, "/pfx") + server = werkzeug.test.Client(app, werkzeug.BaseResponse) + + with self.subTest("at root"): + response = server.get("/pfx") + self._assert_ok(response, path="", script="/pfx") + + with self.subTest("at root with slash"): + response = server.get("/pfx/") + self._assert_ok(response, path="/", script="/pfx") + + with self.subTest("at subpath"): + response = server.get("/pfx/foo/bar") + self._assert_ok(response, path="/foo/bar", script="/pfx") + + with self.subTest("at non-path-component extension"): + with self.assertRaises(errors.NotFoundError): + server.get("/pfxz") + + with self.subTest("above path prefix"): + with self.assertRaises(errors.NotFoundError): + server.get("/hmm") + + def test_composition(self): + app = self._echo_app + app = path_prefix.PathPrefixMiddleware(app, "/bar") + app = path_prefix.PathPrefixMiddleware(app, "/foo") + server = werkzeug.test.Client(app, werkzeug.BaseResponse) + + response = server.get("/foo/bar/baz/quux") + self._assert_ok(response, path="/baz/quux", script="/foo/bar") + + +if __name__ == "__main__": + tb_test.main() From 6f68f68dea870e9b597e201858ffde9abf0eb5f8 Mon Sep 17 00:00:00 2001 From: William Chargin Date: Sat, 5 Oct 2019 19:01:20 -0700 Subject: [PATCH 3/9] application: add redirect to ensure trailing slash MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Summary: When using a `--path_prefix`, TensorBoard has historically required a trailing slash after that prefix: e.g., one visits `localhost:6006/foo/` rather than `localhost:6006/foo`. (See, for instance, #1176.) Different versions of TensorBoard have different failure modes when the non-slash path is loaded; currently, the TensorBoard shell loads, but the frontend computes relative URLs incorrectly. This commit adds a redirect from the empty path to `/` to avoid the problem. This logic could be inlined into the `PathPrefixMiddleware`, but we’ll soon have other middlewares with similar needs, so providing this as its own middleware avoids duplicating the functionality. Test Plan: Launch TensorBoard with `--path_prefix /foo` (or `--path_prefix /foo/`; the two are equivalent) and navigate to `/foo/` and `/foo` in a browser. Note that they now both work and resolve to `/foo/`, while prior to this commit navigating to `/foo` yielded a broken TensorBoard. wchargin-branch: empty-path-redirect wchargin-source: 300682590a9a44f1b37d16168b457b4f5898463c --- tensorboard/backend/BUILD | 20 ++++++ tensorboard/backend/application.py | 2 + tensorboard/backend/application_test.py | 6 +- tensorboard/backend/empty_path_redirect.py | 46 ++++++++++++ .../backend/empty_path_redirect_test.py | 72 +++++++++++++++++++ 5 files changed, 143 insertions(+), 3 deletions(-) create mode 100644 tensorboard/backend/empty_path_redirect.py create mode 100644 tensorboard/backend/empty_path_redirect_test.py diff --git a/tensorboard/backend/BUILD b/tensorboard/backend/BUILD index 90cccd19dd..0bcc8bc12f 100644 --- a/tensorboard/backend/BUILD +++ b/tensorboard/backend/BUILD @@ -62,6 +62,7 @@ py_library( srcs_version = "PY2AND3", visibility = ["//visibility:public"], deps = [ + ":empty_path_redirect", ":http_util", ":path_prefix", "//tensorboard:errors", @@ -98,6 +99,25 @@ py_test( ], ) +py_library( + name = "empty_path_redirect", + srcs = ["empty_path_redirect.py"], + srcs_version = "PY2AND3", +) + +py_test( + name = "empty_path_redirect_test", + size = "small", + srcs = ["empty_path_redirect_test.py"], + srcs_version = "PY2AND3", + tags = ["support_notf"], + deps = [ + ":empty_path_redirect", + "//tensorboard:test", + "@org_pocoo_werkzeug", + ], +) + py_library( name = "path_prefix", srcs = ["path_prefix.py"], diff --git a/tensorboard/backend/application.py b/tensorboard/backend/application.py index 0ceb68eebb..0c35a780d8 100644 --- a/tensorboard/backend/application.py +++ b/tensorboard/backend/application.py @@ -39,6 +39,7 @@ from werkzeug import wrappers from tensorboard import errors +from tensorboard.backend import empty_path_redirect from tensorboard.backend import http_util from tensorboard.backend import path_prefix from tensorboard.backend.event_processing import db_import_multiplexer @@ -329,6 +330,7 @@ def __init__(self, plugins, path_prefix=''): def _create_wsgi_app(self): """Apply middleware to create the final WSGI app.""" app = self._route_request + app = empty_path_redirect.EmptyPathRedirectMiddleware(app) app = path_prefix.PathPrefixMiddleware(app, self._path_prefix) app = _handling_errors(app) return app diff --git a/tensorboard/backend/application_test.py b/tensorboard/backend/application_test.py index 8af63963e3..d1a2b740cf 100644 --- a/tensorboard/backend/application_test.py +++ b/tensorboard/backend/application_test.py @@ -302,9 +302,9 @@ def _get_json(self, path): return json.loads(response.get_data().decode('utf-8')) def testBaseUrlRequest(self): - """Request a page that doesn't exist; it should 404.""" + """Base URL should redirect to "/" for proper relative URLs.""" response = self.server.get(self.path_prefix) - self.assertEqual(404, response.status_code) + self.assertEqual(301, response.status_code) def testBaseUrlRequestNonexistentPage(self): """Request a page that doesn't exist; it should 404.""" @@ -680,7 +680,7 @@ def testMissingRoute(self): self._test_route('/data/plugin/foo/bogus', 404) def testEmptyRoute(self): - self._test_route('', 404) + self._test_route('', 301) def testSlashlessRoute(self): self._test_route('runaway', 404) diff --git a/tensorboard/backend/empty_path_redirect.py b/tensorboard/backend/empty_path_redirect.py new file mode 100644 index 0000000000..64f5d649ca --- /dev/null +++ b/tensorboard/backend/empty_path_redirect.py @@ -0,0 +1,46 @@ +# Copyright 2019 The TensorFlow Authors. All Rights Reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# ============================================================================== +"""Redirect from an empty path to "/". + +Sometimes, middleware transformations will make the path empty: for +example, navigating to "/foo" (no trailing slash) when the path prefix +is exactly "/foo". In such cases, relative links on the frontend would +break. Instead of handling this special case in each relevant +middleware, we install a top-level redirect handler from "" to "/". +""" + +from __future__ import absolute_import +from __future__ import division +from __future__ import print_function + + +class EmptyPathRedirectMiddleware(object): + """WSGI middleware to redirect from "" to "/".""" + + def __init__(self, application): + """Initializes this middleware. + + Args: + application: The WSGI application to wrap (see PEP 3333). + """ + self._application = application + + def __call__(self, environ, start_response): + path = environ.get("PATH_INFO", "") + if path: + return self._application(environ, start_response) + location = environ.get("SCRIPT_NAME", "") + "/" + start_response("301 Moved Permanently", [("Location", location)]) + return [] diff --git a/tensorboard/backend/empty_path_redirect_test.py b/tensorboard/backend/empty_path_redirect_test.py new file mode 100644 index 0000000000..5494ce80b4 --- /dev/null +++ b/tensorboard/backend/empty_path_redirect_test.py @@ -0,0 +1,72 @@ +# Copyright 2019 The TensorFlow Authors. All Rights Reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# ============================================================================== +"""Tests for `tensorboard.backend.empty_path_redirect`.""" + +from __future__ import absolute_import +from __future__ import division +from __future__ import print_function + +import json + +import werkzeug + +from tensorboard import test as tb_test +from tensorboard.backend import empty_path_redirect + + +class EmptyPathRedirectMiddlewareTest(tb_test.TestCase): + """Tests for `EmptyPathRedirectMiddleware`.""" + + def setUp(self): + super(EmptyPathRedirectMiddlewareTest, self).setUp() + app = werkzeug.Request.application(lambda req: werkzeug.Response(req.path)) + app = empty_path_redirect.EmptyPathRedirectMiddleware(app) + app = self._lax_strip_foo_middleware(app) + self.app = app + self.server = werkzeug.test.Client(self.app, werkzeug.BaseResponse) + + def _lax_strip_foo_middleware(self, app): + """Strips a `/foo` prefix if it exists; no-op otherwise.""" + + def wrapper(environ, start_response): + path = environ.get("PATH_INFO", "") + if path.startswith("/foo"): + environ["PATH_INFO"] = path[len("/foo") :] + environ["SCRIPT_NAME"] = "/foo" + return app(environ, start_response) + + return wrapper + + def test_normal_route_not_redirected(self): + response = self.server.get("/foo/bar") + self.assertEqual(response.status_code, 200) + + def test_slash_not_redirected(self): + response = self.server.get("/foo/") + self.assertEqual(response.status_code, 200) + + def test_empty_redirected_with_script_name(self): + response = self.server.get("/foo") + self.assertEqual(response.status_code, 301) + self.assertEqual(response.headers["Location"], "/foo/") + + def test_empty_redirected_with_blank_script_name(self): + response = self.server.get("") + self.assertEqual(response.status_code, 301) + self.assertEqual(response.headers["Location"], "/") + + +if __name__ == "__main__": + tb_test.main() From 60ec28240e1639bb03fb9a5f76728578cada0072 Mon Sep 17 00:00:00 2001 From: William Chargin Date: Sat, 5 Oct 2019 19:01:35 -0700 Subject: [PATCH 4/9] application: add middleware for experiment IDs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Summary: This commit teaches TensorBoard to recognize URLs like `/experiment/123` as specifying experiment IDs. If these two path components are omitted, the experiment ID is the empty string, for backward compatibility. The experiment ID is intercepted by middleware and stripped from the URL before the application handles it, so no existing logic needs to change. The ID is added to the WSGI environment and may be extracted via a new public function, `plugin_util.experiment_id`. This commit does not yet replace the existing query parameter wiring, so this is not a user-facing change (except that more URLs are now valid). This functionality is compatible with `--path_prefix`. Note that the `/data/plugins_listing` implementation does not need to change: it correctly specifies experiment IDs in iframe loading mechanisms because we already use the general-purpose `SCRIPT_NAME` key specified by WSGI for this purpose. Test Plan: Unit tests and a small integration test in `:application_test` included. For manual testing, modify `application.py`’s `_route_request` method to print out the application-level path and experiment ID: ```python # at top of `_route_request`: from tensorboard import plugin_util eid = plugin_util.experiment_id(environ) logger.warn("%r [%r]" % (environ.get("PATH_INFO"), eid)) ``` Then, check that the scalars plugin and the dynamically loaded projector plugin both work, and that the paths and experiment IDs are as expected, in the following configurations: - with `--path_prefix=/foo`: - at `/foo` (should redirect to add slash) - at `/foo/` - at `/foo/experiment/123` (should redirect to add slash) - at `/foo/experiment/123/` - with no path prefix: - at `/` - at `/experiment/123` (should redirect to add slash) - at `/experiment/123/` wchargin-branch: eid-middleware wchargin-source: 21f505c418f05a90ed434a9ca86b41959a39040f --- tensorboard/BUILD | 2 + tensorboard/backend/BUILD | 19 ++++++ tensorboard/backend/application.py | 3 + tensorboard/backend/application_test.py | 29 +++++++- tensorboard/backend/experiment_id.py | 70 ++++++++++++++++++++ tensorboard/backend/experiment_id_test.py | 80 +++++++++++++++++++++++ tensorboard/plugin_util.py | 20 ++++++ tensorboard/plugin_util_test.py | 15 +++++ 8 files changed, 236 insertions(+), 2 deletions(-) create mode 100644 tensorboard/backend/experiment_id.py create mode 100644 tensorboard/backend/experiment_id_test.py diff --git a/tensorboard/BUILD b/tensorboard/BUILD index 54d4f35884..8d5d280cc0 100644 --- a/tensorboard/BUILD +++ b/tensorboard/BUILD @@ -409,6 +409,7 @@ py_library( srcs_version = "PY2AND3", visibility = ["//visibility:public"], deps = [ + "//tensorboard/backend:experiment_id", "@org_mozilla_bleach", "@org_pythonhosted_markdown", "@org_pythonhosted_six", @@ -424,6 +425,7 @@ py_test( deps = [ ":plugin_util", ":test", + "//tensorboard/backend:experiment_id", "@org_pythonhosted_six", ], ) diff --git a/tensorboard/backend/BUILD b/tensorboard/backend/BUILD index 0bcc8bc12f..fb5eaa83fb 100644 --- a/tensorboard/backend/BUILD +++ b/tensorboard/backend/BUILD @@ -63,6 +63,7 @@ py_library( visibility = ["//visibility:public"], deps = [ ":empty_path_redirect", + ":experiment_id", ":http_util", ":path_prefix", "//tensorboard:errors", @@ -91,6 +92,7 @@ py_test( deps = [ ":application", "//tensorboard:errors", + "//tensorboard:plugin_util", "//tensorboard:test", "//tensorboard/backend/event_processing:event_multiplexer", "//tensorboard/plugins:base_plugin", @@ -118,6 +120,23 @@ py_test( ], ) +py_library( + name = "experiment_id", + srcs = ["experiment_id.py"], + srcs_version = "PY2AND3", +) + +py_test( + name = "experiment_id_test", + srcs = ["experiment_id_test.py"], + srcs_version = "PY2AND3", + deps = [ + ":experiment_id", + "//tensorboard:test", + "@org_pocoo_werkzeug", + ], +) + py_library( name = "path_prefix", srcs = ["path_prefix.py"], diff --git a/tensorboard/backend/application.py b/tensorboard/backend/application.py index 0c35a780d8..c9c17d9b41 100644 --- a/tensorboard/backend/application.py +++ b/tensorboard/backend/application.py @@ -40,6 +40,7 @@ from tensorboard import errors from tensorboard.backend import empty_path_redirect +from tensorboard.backend import experiment_id from tensorboard.backend import http_util from tensorboard.backend import path_prefix from tensorboard.backend.event_processing import db_import_multiplexer @@ -331,6 +332,7 @@ def _create_wsgi_app(self): """Apply middleware to create the final WSGI app.""" app = self._route_request app = empty_path_redirect.EmptyPathRedirectMiddleware(app) + app = experiment_id.ExperimentIdMiddleware(app) app = path_prefix.PathPrefixMiddleware(app, self._path_prefix) app = _handling_errors(app) return app @@ -429,6 +431,7 @@ def _route_request(self, environ, start_response): environ: See WSGI spec (PEP 3333). start_response: See WSGI spec (PEP 3333). """ + request = wrappers.Request(environ) parsed_url = urlparse.urlparse(request.path) clean_path = _clean_path(parsed_url.path) diff --git a/tensorboard/backend/application_test.py b/tensorboard/backend/application_test.py index d1a2b740cf..e34d96603a 100644 --- a/tensorboard/backend/application_test.py +++ b/tensorboard/backend/application_test.py @@ -41,6 +41,7 @@ from werkzeug import wrappers from tensorboard import errors +from tensorboard import plugin_util from tensorboard import test as tb_test from tensorboard.backend import application from tensorboard.backend.event_processing import plugin_event_multiplexer as event_multiplexer # pylint: disable=line-too-long @@ -621,6 +622,11 @@ def setUp(self): '/wildcard/special/exact': self._foo_handler, }, construction_callback=self._construction_callback), + FakePluginLoader( + plugin_name='whoami', + routes_mapping={ + '/eid': self._eid_handler, + }), ], dummy_assets_zip_provider) @@ -641,6 +647,12 @@ def _foo_handler(self, request): def _bar_handler(self): pass + @wrappers.Request.application + def _eid_handler(self, request): + eid = plugin_util.experiment_id(request.environ) + body = json.dumps({'experiment_id': eid}) + return wrappers.Response(body, 200, content_type='application/json') + @wrappers.Request.application def _wildcard_handler(self, request): if request.path == '/data/plugin/bar/wildcard/ok': @@ -664,11 +676,12 @@ def testPluginsAdded(self): self.assertLessEqual(expected_routes, frozenset(self.app.exact_routes)) def testNameToPluginMapping(self): - # The mapping from plugin name to instance should include both plugins. + # The mapping from plugin name to instance should include all plugins. mapping = self.context.plugin_name_to_instance - self.assertItemsEqual(['foo', 'bar'], list(mapping.keys())) + self.assertItemsEqual(['foo', 'bar', 'whoami'], list(mapping.keys())) self.assertEqual('foo', mapping['foo'].plugin_name) self.assertEqual('bar', mapping['bar'].plugin_name) + self.assertEqual('whoami', mapping['whoami'].plugin_name) def testNormalRoute(self): self._test_route('/data/plugin/foo/foo_route', 200) @@ -679,6 +692,18 @@ def testNormalRouteIsNotWildcard(self): def testMissingRoute(self): self._test_route('/data/plugin/foo/bogus', 404) + def testExperimentIdIntegration_withNoExperimentId(self): + response = self.server.get('/data/plugin/whoami/eid') + self.assertEqual(response.status_code, 200) + data = json.loads(response.get_data().decode('utf-8')) + self.assertEqual(data, {'experiment_id': ''}) + + def testExperimentIdIntegration_withExperimentId(self): + response = self.server.get('/experiment/123/data/plugin/whoami/eid') + self.assertEqual(response.status_code, 200) + data = json.loads(response.get_data().decode('utf-8')) + self.assertEqual(data, {'experiment_id': '123'}) + def testEmptyRoute(self): self._test_route('', 301) diff --git a/tensorboard/backend/experiment_id.py b/tensorboard/backend/experiment_id.py new file mode 100644 index 0000000000..c01f33b89a --- /dev/null +++ b/tensorboard/backend/experiment_id.py @@ -0,0 +1,70 @@ +# Copyright 2019 The TensorFlow Authors. All Rights Reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# ============================================================================== +"""Application-level experiment ID support.""" + +from __future__ import absolute_import +from __future__ import division +from __future__ import print_function + +import re + + +# Value of the first path component that signals that the second path +# component represents an experiment ID. +_EXPERIMENT_PATH_COMPONENT = "experiment" + +# Key into the WSGI environment used for the experiment ID. +WSGI_ENVIRON_KEY = "HTTP_TENSORBOARD_EXPERIMENT_ID" + + +class ExperimentIdMiddleware(object): + """WSGI middleware extracting experiment IDs from URL to environment. + + Any request whose path matches `/experiment/SOME_EID[/...]` will have + its first two path components stripped, and its experiment ID stored + onto the WSGI environment with key `WSGI_ENVIRON_KEY`. All other requests + will have paths unchanged and the experiment ID set to the empty string. + + Instances of this class are WSGI applications (see PEP 3333). + """ + + def __init__(self, application): + """Initializes an `ExperimentIdMiddleware`. + + Args: + application: The WSGI application to wrap (see PEP 3333). + """ + self._application = application + # Regular expression that matches the whole `/experiment/EID` prefix + # (without any trailing slash) and captures the experiment ID. + self._pat = re.compile( + r"/%s/([^/]*)" % re.escape(_EXPERIMENT_PATH_COMPONENT) + ) + + def __call__(self, environ, start_response): + path = environ.get("PATH_INFO", "") + m = self._pat.match(path) + if m: + eid = m.group(1) + new_path = path[m.end(0):] + root = m.group(0) + else: + eid = "" + new_path = path + root = "" + environ[WSGI_ENVIRON_KEY] = eid + environ["PATH_INFO"] = new_path + environ["SCRIPT_NAME"] = environ.get("SCRIPT_NAME", "") + root + return self._application(environ, start_response) diff --git a/tensorboard/backend/experiment_id_test.py b/tensorboard/backend/experiment_id_test.py new file mode 100644 index 0000000000..04ad8c50cd --- /dev/null +++ b/tensorboard/backend/experiment_id_test.py @@ -0,0 +1,80 @@ +# Copyright 2019 The TensorFlow Authors. All Rights Reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# ============================================================================== +"""Tests for `tensorboard.experiment_id`.""" + +from __future__ import absolute_import +from __future__ import division +from __future__ import print_function + +import json + +import werkzeug + +from tensorboard import test as tb_test +from tensorboard.backend import experiment_id + + +class ExperimentIdMiddlewareTest(tb_test.TestCase): + """Tests for `ExperimentIdMiddleware`.""" + + def setUp(self): + super(ExperimentIdMiddlewareTest, self).setUp() + self.app = experiment_id.ExperimentIdMiddleware(self._echo_app) + self.server = werkzeug.test.Client(self.app, werkzeug.BaseResponse) + + def _echo_app(self, environ, start_response): + # https://www.python.org/dev/peps/pep-0333/#environ-variables + data = { + "eid": environ[experiment_id.WSGI_ENVIRON_KEY], + "path": environ.get("PATH_INFO", ""), + "script": environ.get("SCRIPT_NAME", ""), + } + body = json.dumps(data, sort_keys=True) + start_response("200 OK", [("Content-Type", "application/json")]) + return [body] + + def _assert_ok(self, response, eid, path, script): + self.assertEqual(response.status_code, 200) + actual = json.loads(response.get_data()) + expected = dict(eid=eid, path=path, script=script) + self.assertEqual(actual, expected) + + def test_no_experiment_empty_path(self): + response = self.server.get("") + self._assert_ok(response, eid="", path="", script="") + + def test_no_experiment_root_path(self): + response = self.server.get("/") + self._assert_ok(response, eid="", path="/", script="") + + def test_no_experiment_sub_path(self): + response = self.server.get("/x/y") + self._assert_ok(response, eid="", path="/x/y", script="") + + def test_with_experiment_empty_path(self): + response = self.server.get("/experiment/123") + self._assert_ok(response, eid="123", path="", script="/experiment/123") + + def test_with_experiment_root_path(self): + response = self.server.get("/experiment/123/") + self._assert_ok(response, eid="123", path="/", script="/experiment/123") + + def test_with_experiment_sub_path(self): + response = self.server.get("/experiment/123/x/y") + self._assert_ok(response, eid="123", path="/x/y", script="/experiment/123") + + +if __name__ == "__main__": + tb_test.main() diff --git a/tensorboard/plugin_util.py b/tensorboard/plugin_util.py index d1dfef5ccf..c259f001d2 100644 --- a/tensorboard/plugin_util.py +++ b/tensorboard/plugin_util.py @@ -24,6 +24,9 @@ import markdown import six +from tensorboard.backend import experiment_id as _experiment_id + + _ALLOWED_ATTRIBUTES = { 'a': ['href', 'title'], 'img': ['src', 'title', 'alt'], @@ -85,3 +88,20 @@ def markdown_to_safe_html(markdown_string): string_sanitized = bleach.clean( string_html, tags=_ALLOWED_TAGS, attributes=_ALLOWED_ATTRIBUTES) return warning + string_sanitized + + +def experiment_id(environ): + """Determine the experiment ID associated with a WSGI request. + + Each request to TensorBoard has an associated experiment ID, which is + always a string and may be empty. This experiment ID should be passed + to data providers. + + Args: + environ: A WSGI environment `dict`. For a Werkzeug request, this is + `request.environ`. + + Returns: + A experiment ID, as a possibly-empty `str`. + """ + return environ.get(_experiment_id.WSGI_ENVIRON_KEY, "") diff --git a/tensorboard/plugin_util_test.py b/tensorboard/plugin_util_test.py index 320c78348f..ae029ccb67 100644 --- a/tensorboard/plugin_util_test.py +++ b/tensorboard/plugin_util_test.py @@ -22,6 +22,7 @@ from tensorboard import plugin_util from tensorboard import test as tb_test +from tensorboard.backend import experiment_id class MarkdownToSafeHTMLTest(tb_test.TestCase): @@ -123,5 +124,19 @@ def test_null_bytes_stripped_before_markdown_processing(self): '

un_der_score

') +class ExperimentIdTest(tb_test.TestCase): + """Tests for `plugin_util.experiment_id`.""" + + def test_default(self): + # This shouldn't happen; the `ExperimentIdMiddleware` always set an + # experiment ID. In case something goes wrong, degrade gracefully. + environ = {} + self.assertEqual(plugin_util.experiment_id(environ), "") + + def test_present(self): + environ = {experiment_id.WSGI_ENVIRON_KEY: "123"} + self.assertEqual(plugin_util.experiment_id(environ), "123") + + if __name__ == '__main__': tb_test.main() From b0fc7ba7508570a5aee52a94f90b18897f04592e Mon Sep 17 00:00:00 2001 From: William Chargin Date: Sat, 5 Oct 2019 21:24:15 -0700 Subject: [PATCH 5/9] [update patch] wchargin-branch: eid-middleware wchargin-source: e956be9e14a712e424594550d58ef337710a0a2b --- tensorboard/backend/experiment_id_test.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tensorboard/backend/experiment_id_test.py b/tensorboard/backend/experiment_id_test.py index 04ad8c50cd..4e8a85a129 100644 --- a/tensorboard/backend/experiment_id_test.py +++ b/tensorboard/backend/experiment_id_test.py @@ -12,7 +12,7 @@ # See the License for the specific language governing permissions and # limitations under the License. # ============================================================================== -"""Tests for `tensorboard.experiment_id`.""" +"""Tests for `tensorboard.backend.experiment_id`.""" from __future__ import absolute_import from __future__ import division From b5002d6c801f538b749c6d975d7b7424a351fa48 Mon Sep 17 00:00:00 2001 From: William Chargin Date: Mon, 7 Oct 2019 13:25:35 -0700 Subject: [PATCH 6/9] [update patch] wchargin-branch: empty-path-redirect wchargin-source: 5cb309fa62fd9efac68ac52b8a57fcef67a2fcb5 --- tensorboard/backend/BUILD | 3 --- tensorboard/backend/application.py | 3 --- 2 files changed, 6 deletions(-) diff --git a/tensorboard/backend/BUILD b/tensorboard/backend/BUILD index 1dddc2cce1..0bcc8bc12f 100644 --- a/tensorboard/backend/BUILD +++ b/tensorboard/backend/BUILD @@ -100,7 +100,6 @@ py_test( ) py_library( -<<<<<<< HEAD name = "empty_path_redirect", srcs = ["empty_path_redirect.py"], srcs_version = "PY2AND3", @@ -120,8 +119,6 @@ py_test( ) py_library( -======= ->>>>>>> 0981d8a7b0137d0dd67b935d77ee139e687d212f name = "path_prefix", srcs = ["path_prefix.py"], srcs_version = "PY2AND3", diff --git a/tensorboard/backend/application.py b/tensorboard/backend/application.py index 7ad9e34d5c..0c35a780d8 100644 --- a/tensorboard/backend/application.py +++ b/tensorboard/backend/application.py @@ -330,10 +330,7 @@ def __init__(self, plugins, path_prefix=''): def _create_wsgi_app(self): """Apply middleware to create the final WSGI app.""" app = self._route_request -<<<<<<< HEAD app = empty_path_redirect.EmptyPathRedirectMiddleware(app) -======= ->>>>>>> 0981d8a7b0137d0dd67b935d77ee139e687d212f app = path_prefix.PathPrefixMiddleware(app, self._path_prefix) app = _handling_errors(app) return app From 31680153d1be8f99b77f07160ba9c15893c2eb67 Mon Sep 17 00:00:00 2001 From: William Chargin Date: Mon, 7 Oct 2019 13:56:39 -0700 Subject: [PATCH 7/9] [update patch] wchargin-branch: empty-path-redirect wchargin-source: fa2c0afa5ecc3882d03ea695c6370edaa77399a2 --- tensorboard/backend/empty_path_redirect.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/tensorboard/backend/empty_path_redirect.py b/tensorboard/backend/empty_path_redirect.py index 64f5d649ca..d2d5be4fe0 100644 --- a/tensorboard/backend/empty_path_redirect.py +++ b/tensorboard/backend/empty_path_redirect.py @@ -12,13 +12,17 @@ # See the License for the specific language governing permissions and # limitations under the License. # ============================================================================== -"""Redirect from an empty path to "/". +"""Redirect from an empty path to the virtual application root. Sometimes, middleware transformations will make the path empty: for example, navigating to "/foo" (no trailing slash) when the path prefix is exactly "/foo". In such cases, relative links on the frontend would break. Instead of handling this special case in each relevant middleware, we install a top-level redirect handler from "" to "/". + +This middleware respects `SCRIPT_NAME` as described by the WSGI spec. If +`SCRIPT_NAME` is set to "/foo", then an empty `PATH_INFO` corresponds to +the actual path "/foo", and so will be redirected to "/foo/". """ from __future__ import absolute_import From 927083338a4c5f4874e8edc3fee730ce4f698154 Mon Sep 17 00:00:00 2001 From: William Chargin Date: Mon, 7 Oct 2019 14:21:19 -0700 Subject: [PATCH 8/9] [update patch] wchargin-branch: eid-middleware wchargin-source: f581a5c84d81b331ffcae4c0ef7dfbc32da9986e --- tensorboard/backend/experiment_id.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tensorboard/backend/experiment_id.py b/tensorboard/backend/experiment_id.py index c01f33b89a..7c427abe32 100644 --- a/tensorboard/backend/experiment_id.py +++ b/tensorboard/backend/experiment_id.py @@ -34,8 +34,9 @@ class ExperimentIdMiddleware(object): Any request whose path matches `/experiment/SOME_EID[/...]` will have its first two path components stripped, and its experiment ID stored - onto the WSGI environment with key `WSGI_ENVIRON_KEY`. All other requests - will have paths unchanged and the experiment ID set to the empty string. + onto the WSGI environment with key taken from the `WSGI_ENVIRON_KEY` + constant. All other requests will have paths unchanged and the + experiment ID set to the empty string. Instances of this class are WSGI applications (see PEP 3333). """ From b4e8824bfb82183e3f5ac76c5c1b12e38307f661 Mon Sep 17 00:00:00 2001 From: William Chargin Date: Mon, 7 Oct 2019 15:12:46 -0700 Subject: [PATCH 9/9] [update patch] wchargin-branch: eid-middleware wchargin-source: 1883d4c6fa7cf726cd9fc159b325232080e856db --- tensorboard/backend/BUILD | 6 ------ tensorboard/backend/application.py | 6 ------ 2 files changed, 12 deletions(-) diff --git a/tensorboard/backend/BUILD b/tensorboard/backend/BUILD index 4ec8cf0e65..fb5eaa83fb 100644 --- a/tensorboard/backend/BUILD +++ b/tensorboard/backend/BUILD @@ -63,10 +63,7 @@ py_library( visibility = ["//visibility:public"], deps = [ ":empty_path_redirect", -<<<<<<< HEAD ":experiment_id", -======= ->>>>>>> 45b62065642b56da62fb5a4a0c930ca04b8b55f6 ":http_util", ":path_prefix", "//tensorboard:errors", @@ -124,7 +121,6 @@ py_test( ) py_library( -<<<<<<< HEAD name = "experiment_id", srcs = ["experiment_id.py"], srcs_version = "PY2AND3", @@ -142,8 +138,6 @@ py_test( ) py_library( -======= ->>>>>>> 45b62065642b56da62fb5a4a0c930ca04b8b55f6 name = "path_prefix", srcs = ["path_prefix.py"], srcs_version = "PY2AND3", diff --git a/tensorboard/backend/application.py b/tensorboard/backend/application.py index 04273300d2..c9c17d9b41 100644 --- a/tensorboard/backend/application.py +++ b/tensorboard/backend/application.py @@ -40,10 +40,7 @@ from tensorboard import errors from tensorboard.backend import empty_path_redirect -<<<<<<< HEAD from tensorboard.backend import experiment_id -======= ->>>>>>> 45b62065642b56da62fb5a4a0c930ca04b8b55f6 from tensorboard.backend import http_util from tensorboard.backend import path_prefix from tensorboard.backend.event_processing import db_import_multiplexer @@ -335,10 +332,7 @@ def _create_wsgi_app(self): """Apply middleware to create the final WSGI app.""" app = self._route_request app = empty_path_redirect.EmptyPathRedirectMiddleware(app) -<<<<<<< HEAD app = experiment_id.ExperimentIdMiddleware(app) -======= ->>>>>>> 45b62065642b56da62fb5a4a0c930ca04b8b55f6 app = path_prefix.PathPrefixMiddleware(app, self._path_prefix) app = _handling_errors(app) return app