From 969d2ecae8da78b5c6007c2e00ae282a9755bdd7 Mon Sep 17 00:00:00 2001 From: David Butenhof Date: Fri, 23 Jun 2023 20:29:13 -0400 Subject: [PATCH 1/3] Add URI to TOC API PBENCH-624 We're not quite ready to rebuild the `/datasets/{dataset}/contents` API on the cache manager. (I actually tried; it's not all there, and currently would need to be built via a full tarball unpack each time we build a `CacheManager` instance, while the changes to persist it via SQL or Redis, which I'd like to do soon, seemed too big a lift right now.) However, we have planned for some time to supplement the TOC information with a uri element for each file and subdirectory; this much smaller change makes the code match the API documentation. --- .../components/TableOfContent/index.jsx | 6 +- lib/pbench/server/api/resources/__init__.py | 64 +++++++++++++++ .../api/resources/endpoint_configure.py | 81 ++++++------------- .../api/resources/query_apis/__init__.py | 2 + .../query_apis/datasets/datasets_contents.py | 28 +++++-- .../query_apis/test_datasets_contents.py | 19 ++++- 6 files changed, 132 insertions(+), 68 deletions(-) diff --git a/dashboard/src/modules/components/TableOfContent/index.jsx b/dashboard/src/modules/components/TableOfContent/index.jsx index 0adb087500..ed61ae05e2 100644 --- a/dashboard/src/modules/components/TableOfContent/index.jsx +++ b/dashboard/src/modules/components/TableOfContent/index.jsx @@ -241,7 +241,7 @@ const TableOfContent = () => { key={index} direction="down" onClick={() => { - attachBreadCrumbs(data, true); + attachBreadCrumbs(data.name, true); }} drilldownMenu={ @@ -255,11 +255,11 @@ const TableOfContent = () => { key={index} direction="down" onClick={() => { - attachBreadCrumbs(data, false); + attachBreadCrumbs(data.name, false); }} > - {data} + {data.name} ); } else { diff --git a/lib/pbench/server/api/resources/__init__.py b/lib/pbench/server/api/resources/__init__.py index 4b6cf4f73d..7712340a1a 100644 --- a/lib/pbench/server/api/resources/__init__.py +++ b/lib/pbench/server/api/resources/__init__.py @@ -1210,6 +1210,19 @@ def authorize( return self.schemas[method].authorize(args) +class HostSource(Enum): + REQUEST = auto() + FORWARDED = auto() + X_FORWARDED = auto() + + +@dataclass +class UriBase: + host: str + host_source: HostSource + host_value: str + + class ApiBase(Resource): """A base class for Pbench queries that provides common parameter handling behavior for specialized subclasses. @@ -1628,6 +1641,57 @@ def _set_dataset_metadata( fail[k] = str(e) return fail + HEADER_FORWARD = re.compile(r";\s*host\s*=\s*(?P[^;\s]+)") + X_HEADER_FORWARD = re.compile(r"\s*(?P[^;\s,]+)") + PARAM_TEMPLATE = re.compile(r"/<(?P[^:]+):(?P\w+)>") + + def _get_uri_base(self, request: Request) -> UriBase: + """Determine the original request URI + + When a request is directed through reverse proxies, the origin we see + may not be the URI used by the client to arrive here. When we're + providing a follow-up URI, we want to decode the forwarding headers to + avoid dropping a proxy (which may provide important infrastructure + controls and visibility). + + Args: + request: the original HTTP Request + + Return: + An object describing the origin and how we got there + """ + current_app.logger.debug( + "Received headers: {!r}, access_route {!r}, base_url {!r}, host {!r}, host_url {!r}", + request.headers, + request.access_route, + request.base_url, + request.host, + request.host_url, + ) + origin = None + host_source = HostSource.REQUEST + host_value = request.host + header = request.headers.get("Forwarded") + if header: + m = self.HEADER_FORWARD.search(header) + if m: + origin = m.group("host") + host_source = HostSource.FORWARDED + host_value = header + if not origin: + header = request.headers.get("X-Forwarded-Host") + if header: + m = self.X_HEADER_FORWARD.match(header) + if m: + origin = m.group("host") + host_source = HostSource.X_FORWARDED + host_value = header + if not origin: + origin = host_value + proto = request.headers.get("X-Forwarded-Proto", "https") + host = f"{proto}://{origin}" + return UriBase(host, host_source, host_value) + def _dispatch( self, method: ApiMethod, diff --git a/lib/pbench/server/api/resources/endpoint_configure.py b/lib/pbench/server/api/resources/endpoint_configure.py index 884ca4313c..51d24be441 100644 --- a/lib/pbench/server/api/resources/endpoint_configure.py +++ b/lib/pbench/server/api/resources/endpoint_configure.py @@ -1,24 +1,25 @@ -from http import HTTPStatus -import re -from typing import Any, Dict +from typing import Any from urllib.parse import urljoin -from flask import current_app, jsonify, request -from flask_restful import abort, Resource +from flask import current_app, jsonify, Request, Response -from pbench.server import PbenchServerConfig +from pbench.server import OperationCode, PbenchServerConfig +from pbench.server.api.resources import ( + ApiBase, + ApiContext, + APIInternalError, + ApiMethod, + ApiParams, + ApiSchema, +) -class EndpointConfig(Resource): +class EndpointConfig(ApiBase): """ This supports dynamic dashboard configuration from the Pbench server rather than constructing a static dashboard config file. """ - forward_pattern = re.compile(r";\s*host\s*=\s*(?P[^;\s]+)") - x_forward_pattern = re.compile(r"\s*(?P[^;\s,]+)") - param_template = re.compile(r"/<(?P[^:]+):(?P\w+)>") - def __init__(self, config: PbenchServerConfig): """ __init__ Construct the API resource @@ -42,9 +43,10 @@ def __init__(self, config: PbenchServerConfig): proxying was set up for the original endpoints query: e.g., the Javascript `window.origin` from which the Pbench dashboard was loaded. """ + super().__init__(config, ApiSchema(ApiMethod.GET, OperationCode.READ)) self.server_config = config - def get(self): + def _get(self, args: ApiParams, request: Request, context: ApiContext) -> Response: """ Return server configuration information required by web clients including the Pbench dashboard UI. This includes: @@ -76,43 +78,16 @@ def get(self): template.replace('{target_username}', 'value') """ - current_app.logger.debug( - "Received headers: {!r}, access_route {!r}, base_url {!r}, host {!r}, host_url {!r}", - request.headers, - request.access_route, - request.base_url, - request.host, - request.host_url, - ) - origin = None - host_source = "request" - host_value = request.host - header = request.headers.get("Forwarded") - if header: - m = self.forward_pattern.search(header) - if m: - origin = m.group("host") - host_source = "Forwarded" - host_value = header - if not origin: - header = request.headers.get("X-Forwarded-Host") - if header: - m = self.x_forward_pattern.match(header) - if m: - origin = m.group("host") - host_source = "X-Forwarded-Host" - host_value = header - if not origin: - origin = host_value - proto = request.headers.get("X-Forwarded-Proto", "https") - host = f"{proto}://{origin}" + origin = self._get_uri_base(request) current_app.logger.info( "Advertising endpoints at {} relative to {} ({})", - host, - host_source, - host_value, + origin.host, + origin.host_source, + origin.host_value, ) + host = origin.host + templates = {} # Iterate through the Flask endpoints to add a description for each. @@ -122,9 +97,9 @@ def get(self): # Ignore anything that doesn't use our API prefix, because it's # not in our API. if url.startswith(self.server_config.rest_uri): - simplified = self.param_template.sub(r"/{\g}", url) - matches = self.param_template.finditer(url) - template: Dict[str, Any] = { + simplified = self.PARAM_TEMPLATE.sub(r"/{\g}", url) + matches = self.PARAM_TEMPLATE.finditer(url) + template: dict[str, Any] = { "template": urljoin(host, simplified), "params": { match.group("name"): {"type": match.group("type")} @@ -160,12 +135,6 @@ def get(self): } try: - response = jsonify(endpoints) + return jsonify(endpoints) except Exception: - current_app.logger.exception( - "Something went wrong constructing the endpoint info" - ) - abort(HTTPStatus.INTERNAL_SERVER_ERROR, message="INTERNAL ERROR") - else: - response.status_code = HTTPStatus.OK - return response + APIInternalError("Something went wrong constructing the endpoint info") diff --git a/lib/pbench/server/api/resources/query_apis/__init__.py b/lib/pbench/server/api/resources/query_apis/__init__.py index 391a9fdfc2..09d480d421 100644 --- a/lib/pbench/server/api/resources/query_apis/__init__.py +++ b/lib/pbench/server/api/resources/query_apis/__init__.py @@ -468,6 +468,7 @@ def _post( uri_params: URI encoded keyword-arg supplied by the Flask framework """ + context["request"] = request return self._call(requests.post, params, context) def _get( @@ -484,6 +485,7 @@ def _get( uri_params: URI encoded keyword-arg supplied by the Flask framework """ + context["request"] = request return self._call(requests.get, params, context) diff --git a/lib/pbench/server/api/resources/query_apis/datasets/datasets_contents.py b/lib/pbench/server/api/resources/query_apis/datasets/datasets_contents.py index 87d21c9a27..4ce0a12a96 100644 --- a/lib/pbench/server/api/resources/query_apis/datasets/datasets_contents.py +++ b/lib/pbench/server/api/resources/query_apis/datasets/datasets_contents.py @@ -174,7 +174,10 @@ def postprocess(self, es_json: JSON, context: ApiContext) -> JSON: { "directories": [ - "sample1" + { + "name": "sample1", + "uri": "https://host/api/v1/datasets/id/contents/1-default/sample1" + } ], "files": [ { @@ -183,24 +186,35 @@ def postprocess(self, es_json: JSON, context: ApiContext) -> JSON: "size": 0, "mode": "0o777", "type": "sym", - "linkpath": "sample1" + "linkpath": "sample1", + "uri": "https://host/api/v1/datasets/id/inventory/1-default/reference-result" } ] } """ + request = context["request"] + resource_id = context["dataset"].resource_id + target = context["target"] if len(es_json["hits"]["hits"]) == 0: raise PostprocessError( HTTPStatus.NOT_FOUND, - f"No directory '{context['target']}' in '{context['dataset']}' contents.", + f"No directory {target!r} in {resource_id!r} contents.", ) + origin = f"{self._get_uri_base(request).host}/datasets/{resource_id}" + dir_list = [] file_list = [] for val in es_json["hits"]["hits"]: - if val["_source"]["directory"] == context["target"]: + if val["_source"]["directory"] == target: # Retrieve files list if present else add an empty list. - file_list = val["_source"].get("files", []) - elif val["_source"]["parent"] == context["target"]: - dir_list.append(val["_source"]["name"]) + for f in val["_source"].get("files", []): + f["uri"] = f"{origin}/inventory{target}/{f['name']}" + file_list.append(f) + elif val["_source"]["parent"] == target: + name = val["_source"]["name"] + dir_list.append( + {"name": name, "uri": f"{origin}/contents{target}/{name}"} + ) return {"directories": dir_list, "files": file_list} diff --git a/lib/pbench/test/unit/server/query_apis/test_datasets_contents.py b/lib/pbench/test/unit/server/query_apis/test_datasets_contents.py index e1beecede6..bad41138fd 100644 --- a/lib/pbench/test/unit/server/query_apis/test_datasets_contents.py +++ b/lib/pbench/test/unit/server/query_apis/test_datasets_contents.py @@ -159,7 +159,12 @@ def test_query( if expected_status == HTTPStatus.OK: res_json = response.json expected_result = { - "directories": ["sample1"], + "directories": [ + { + "name": "sample1", + "uri": "https://localhost/datasets/random_md5_string1/contents/1-default/sample1", + } + ], "files": [ { "name": "reference-result", @@ -168,6 +173,7 @@ def test_query( "mode": "0o777", "type": "sym", "linkpath": "sample1", + "uri": "https://localhost/datasets/random_md5_string1/inventory/1-default/reference-result", } ], } @@ -270,7 +276,15 @@ def test_subdirectory_query( ) if expected_status == HTTPStatus.OK: res_json = response.json - expected_result = {"directories": ["sample1"], "files": []} + expected_result = { + "directories": [ + { + "name": "sample1", + "uri": "https://localhost/datasets/random_md5_string1/contents/1-default/sample1", + } + ], + "files": [], + } assert expected_result == res_json def test_files_query( @@ -353,6 +367,7 @@ def test_files_query( "size": 122, "mode": "0o644", "type": "reg", + "uri": "https://localhost/datasets/random_md5_string1/inventory/1-default/default.csv", } ], } From 24f6ff894a56392e33d8d99e5b447981b12fe7c7 Mon Sep 17 00:00:00 2001 From: David Butenhof Date: Sat, 24 Jun 2023 07:55:47 -0400 Subject: [PATCH 2/3] Rough changes to the Javascript With these changes, it's possible to navigate down the directory hierarchy. Navigating backwards on the "breadcrumb" line (still) doesn't quite work, but I now believe that my URI change in the API hasn't made anything worse than it was before. --- dashboard/src/actions/tableOfContentActions.js | 5 ++++- dashboard/src/modules/components/TableOfContent/index.jsx | 4 ++-- dashboard/src/reducers/tableOfContentReducer.js | 2 +- 3 files changed, 7 insertions(+), 4 deletions(-) diff --git a/dashboard/src/actions/tableOfContentActions.js b/dashboard/src/actions/tableOfContentActions.js index 6136de32e6..f3e4c86362 100644 --- a/dashboard/src/actions/tableOfContentActions.js +++ b/dashboard/src/actions/tableOfContentActions.js @@ -1,6 +1,8 @@ import * as TYPES from "./types"; import API from "../utils/axiosInstance"; import { uriTemplate } from "../utils/helper"; +import { showToast } from "./toastActions"; +import { DANGER } from "assets/constants/toastConstants"; export const fetchTOC = (param, parent, callForSubData) => async (dispatch, getState) => { @@ -18,7 +20,8 @@ export const fetchTOC = }); } } catch (error) { - return error; + const msg = error.response?.data?.message; + dispatch(showToast(DANGER, msg ? msg : `Error response: ERROR_MSG`)); } }; diff --git a/dashboard/src/modules/components/TableOfContent/index.jsx b/dashboard/src/modules/components/TableOfContent/index.jsx index ed61ae05e2..0d5ee7345c 100644 --- a/dashboard/src/modules/components/TableOfContent/index.jsx +++ b/dashboard/src/modules/components/TableOfContent/index.jsx @@ -195,7 +195,7 @@ const TableOfContent = () => { ? initialBreadcrumb(breadCrumbLabels) : appGroupingBreadcrumb(false, breadCrumbLabels) ); - const dirPath = param.concat(`${firstHierarchyLevel ? "" : "/"}`, data); + const dirPath = param.concat(firstHierarchyLevel ? "" : "/", data); setParam(dirPath); setIsLoading(true); getSubFolderData(dirPath); @@ -288,7 +288,7 @@ const TableOfContent = () => { } > - {data} + {data.name} ); })} diff --git a/dashboard/src/reducers/tableOfContentReducer.js b/dashboard/src/reducers/tableOfContentReducer.js index 75b4482c7f..0346568470 100644 --- a/dashboard/src/reducers/tableOfContentReducer.js +++ b/dashboard/src/reducers/tableOfContentReducer.js @@ -36,7 +36,7 @@ const TableOfContentReducer = (state = initialState, action = {}) => { stack: [...state.stack, payload], searchSpace: payload.files, tableData: payload.files, - currData: payload, + contentData: payload, isLoading: false, }; From 79f0863e3708c317d306c4ddc9b44bebd4730557 Mon Sep 17 00:00:00 2001 From: David Butenhof Date: Mon, 26 Jun 2023 19:37:27 -0400 Subject: [PATCH 3/3] Review --- dashboard/src/actions/tableOfContentActions.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dashboard/src/actions/tableOfContentActions.js b/dashboard/src/actions/tableOfContentActions.js index f3e4c86362..6f941ebdf6 100644 --- a/dashboard/src/actions/tableOfContentActions.js +++ b/dashboard/src/actions/tableOfContentActions.js @@ -21,7 +21,7 @@ export const fetchTOC = } } catch (error) { const msg = error.response?.data?.message; - dispatch(showToast(DANGER, msg ? msg : `Error response: ERROR_MSG`)); + dispatch(showToast(DANGER, msg ?? `Error response: ${error}`)); } };