Feature/refresh #273

matthewpeterkort · 2024-07-11T15:32:28Z

Link to JIRA ticket if there is one:

New Features

Adds the ability to refresh guppy via a POST request to the path _refresh. Guppy must be configured to accept this request via the allowRefresh variable in the guppy config. If this variable is not set, the feature is not available.
Authenticated users who present a valid token that has admin_access or * method permissions for service guppy on resource path /guppy_admin are able to make the _refresh request.
This request restarts the server, allowing for the ability to update the guppy configurations to reflect any new column additions in the elastic indices as a data submitter.

Breaking Changes

None

Bug Fixes

None

Improvements

None

Dependency updates

None

Deployment changes

To enable this feature in helm charts add "allowRefresh": {{ .Values.allowRefresh }} to gen3-helm/helm/guppy/templates/guppy_config.yaml and then in your values.yaml file, specify allowRefresh as the boolean true value.

mfshao · 2024-07-23T19:28:49Z

src/server/auth/arboristClient.js

+        Object.keys(result).forEach((key) => {
+        // logic: you have access to a project if you have the following access:
+        // method 'create' (or '*' - all methods) to service 'guppy' (or '*' - all services)
+        // on the project resource.


This is too broad, and it sort of binds the two roles of data submitter and Guppy admin togather. The restarting of Guppy should be an "admin-level" feature. I think we'd better to have a dedicated "guppy_admin" policy for this feature, which could be service == guppy, method == *, or service == guppy, method == admin_access

see 713fc1f

mfshao · 2024-07-23T19:35:30Z

src/server/auth/arboristClient.js

@@ -65,6 +65,64 @@ class ArboristClient {
      },
    );
  }
+
+  listAuthorizedCreateResources(jwt) {


Most of this function is duplicating listAuthorizedResources(jwt), except for the part that check's the user for certain service + methods.
We don't need to duplicate those codes, we should be extracting the part of making request to arborist and checking for user's access in the returned auth mapping into two separated functions, and just reuse them. In this way, we can avoid sending duplicated requests to arborist
See how the fetch for user's auth mapping and check for user's access can be separated in another service
https://github.com/uc-cdis/data-portal/blob/master/src/actions.js#L513
https://github.com/uc-cdis/data-portal/blob/master/src/authMappingUtils.js#L53

Ok getAuthMapping function has been created and used twice, once for the reader permissions checking, and again for the guppy admin permissions checking. See 713fc1f

mfshao · 2024-07-23T19:41:37Z

src/server/auth/utils.js

+    throw new CodedError(data.error.code, data.error.message);
+  }
+  const resources = data.resources ? _.uniq(data.resources) : [];
+  return resources;


We don't need to execute all those expensive array manipulations for this feature, if your goal is to determine whether am user is Guppy admin or not. This function can simply returns a boolean based on the return value from listAuthorizedCreateResources()
Of couse variable and function names should be updated to relflect this change

Ok boolean is returned now when checking for refresh permissions

mfshao · 2024-07-23T19:48:53Z

src/server/server.js

+      }
+      await server.stop()
+      await initializeAndStartServer();
+    } else if (config.allowRefresh === false) {


I think this check should happens first, before checking for user's token

mfshao · 2024-07-23T19:52:48Z

src/server/server.js

+        throw noPermsUser;
+      }
+      await server.stop()
+      await initializeAndStartServer();


This solution is generally ok, but it only works if there is only 1 guppy pod/container/server in the system.
In the cases of how Gen3 is usually deployed, which is on a k8s cluster with a load balancer / reverse proxy in front of pods, and also if there are replicas of guppy pods. This solution will only triggers 1 guppy pod to restart each time, which will causes a discrepancy in the states of pods

@mfshao thanks for all the feedback. In the case of replica guppy pods, is there a link you could provide for how requests from the clients are distributed to the guppy replicas? In other words, is there a nginx or helm configuration that fans out the requests?

@mfshao Alternatively, as a brainstorm: guppy could checking mappings on a configurable interval and initiate a restart if the version changed. For example, for index gen3.aced.io_file_0

$ curl localhost:9200/gen3.aced.io_file_0 | jq '.["gen3.aced.io_file_0"].settings.index.version.created ' >>> "7170399"

On startup, guppy could cache this value for each index, and then monitor the index for changes

mfshao

This branch need a rebase with the current master

mfshao · 2024-10-23T21:19:11Z

src/server/auth/utils.js

+      ],
+    };
+  } else {
+    data = await arboristClient.listAuthMapping(jwt);


Since this canRefresh() will only be called within initialize(), you don't have to make another call to arborist to get user's ath mapping again because you have already done it in getAccessibleResourcesFromArboristasync()
consider to put the auth mapping in a vairable and use it here instead of making another call

This has been implemented in recent commits

mfshao · 2024-10-23T21:20:10Z

src/server/auth/utils.js

  const resources = data.resources ? _.uniq(data.resources) : [];
  return resources;
 };

+export const canRefresh = async (jwt) => {


Suggested change

export const canRefresh = async (jwt) => {

export const checkIfUserCanRefreshServer = async (jwt) => {

canRefresh => checkIfUserCanRefreshServer has been implemented

mfshao · 2024-10-23T21:27:31Z

src/server/server.js

+      const disabledRefresh = new CodedError(404, '[Refresh] guppy _refresh functionality is not enabled');
+      throw disabledRefresh;
+    }
+    else if (config.allowRefresh) {


this can be just an else?

implemented

* chore(deps): bump braces from 3.0.2 to 3.0.3 Bumps [braces](https://github.com/micromatch/braces) from 3.0.2 to 3.0.3. - [Changelog](https://github.com/micromatch/braces/blob/master/CHANGELOG.md) - [Commits](micromatch/braces@3.0.2...3.0.3) --- updated-dependencies: - dependency-name: braces dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> * trigger gh action * trigger travis --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Mingfei Shao <mshao1@uchicago.edu>

Bumps [ws](https://github.com/websockets/ws) from 6.2.2 to 6.2.3. - [Release notes](https://github.com/websockets/ws/releases) - [Commits](websockets/ws@6.2.2...6.2.3) --- updated-dependencies: - dependency-name: ws dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Mingfei Shao <2475897+mfshao@users.noreply.github.com>

* fix apply hide number resolver to as text agg results * update dockerfile

Param order was wrong on changed line, put err first. Refer to: https://expressjs.com/en/guide/error-handling.html

…dis#280) The following vulnerabilities are fixed with an upgrade: - https://snyk.io/vuln/SNYK-JS-SEND-7926862 Co-authored-by: snyk-bot <snyk-bot@snyk.io>

…e to use newer dependencies (uc-cdis#285) * PPS-1564 PPS-1567 PPS-1585: update body-parser and package lock file to use newer dependencies * PPS-1564 PPS-1567 PPS-1585: add file so nvm can set node version

Fix a dead link

mfshao

we are getting close to it, still a few small issues to fix, and we will run our CI over this to make sure there is no regression
I think after fixing these issues and passing CI we could approve

mfshao · 2024-10-25T19:02:35Z

src/server/server.js

-      }
-    },
-  );
+// eslint-disable-next-line no-unused-vars


you don't need this for the next line

mfshao · 2024-10-25T19:03:12Z

src/server/server.js

-  });
-};
+// download endpoint for fetching data directly from es
+app.post('/download', downloadRouter, (err, req, res, next) => {


put // eslint-disable-next-line no-unused-vars above this to satisfy the linter

mfshao · 2024-10-25T19:03:30Z

src/server/server.js

-};
+// download endpoint for fetching data directly from es
+app.post('/download', downloadRouter, (err, req, res, next) => {
+  // eslint-disable-line no-unused-vars


you don't need this

mfshao · 2024-10-25T19:03:38Z

src/server/server.js

+  }
+});
+
+app.post('/_refresh', refreshRouter, (err, req, res, next) => {


put // eslint-disable-next-line no-unused-vars above this to satisfy the linter

mfshao · 2024-10-25T19:04:13Z

src/server/server.js

 const app = express();
 app.use(cors());
 app.use(helmet());
 app.use(bodyParser.json({ limit: '50mb' }));

+const refreshRouter = async (req, res, next) => {


refreshRouter should be placed after initializeAndStartServer

mfshao · 2024-10-25T19:04:49Z

src/server/config.js

 }

 const config = {
+  allowRefresh: inputConfig.allowRefresh || false,


allowRefresh is duplicated in this config

mfshao · 2024-10-25T19:07:21Z

src/server/auth/utils.js

+
+export const checkIfUserCanRefreshServer = async (data) => {
+  if (config.internalLocalTest) {
+    data = {


consider fix the failed es-lint check here

mfshao

CI is passing in this PR #292
code-wsie lgtm

paulineribeyre · 2024-11-04T18:40:03Z

Authenticated users who present a valid token that has create method permissions on at least one resource path are able to make the _refresh request.

This statement in the PR description and release notes is not accurate

mfshao · 2024-11-04T19:07:01Z

Authenticated users who present a valid token that has create method permissions on at least one resource path are able to make the _refresh request.

This statement in the PR description and release notes is not accurate

yeah you are right, I can update the PR description, and release note

matthewpeterkort added 4 commits July 10, 2024 15:03

Adds initial support for guppy refresh

362cf06

Fix Formatting

096ed8f

Adds create method checking, updates tests

248be52

fix: test

cca5950

matthewpeterkort mentioned this pull request Jul 11, 2024

Dataframer/Pivot with Facet Mangement ACED-IDP/submission#23

Open

mfshao reviewed Jul 23, 2024

View reviewed changes

Addresses PR feedback

713fc1f

matthewpeterkort requested a review from mfshao July 26, 2024 21:17

quinnwai mentioned this pull request Jul 29, 2024

Feature/grip ACED-IDP/gen3-helm#62

Merged

mfshao reviewed Oct 23, 2024

View reviewed changes

dependabot bot and others added 13 commits October 24, 2024 10:31

address recent feedback

c77cfdc

fix apply hide number resolver to as text agg results (uc-cdis#274)

91403ea

* fix apply hide number resolver to as text agg results * update dockerfile

Fix param ordering in server.js (uc-cdis#277)

1498b60

Param order was wrong on changed line, put err first. Refer to: https://expressjs.com/en/guide/error-handling.html

fix: package.json & package-lock.json to reduce vulnerabilities (uc-c…

71a4fe3

…dis#280) The following vulnerabilities are fixed with an upgrade: - https://snyk.io/vuln/SNYK-JS-SEND-7926862 Co-authored-by: snyk-bot <snyk-bot@snyk.io>

Update integration_tests.yaml (uc-cdis#284)

5e6193b

(PPS-1564 PPS-1567 PPS-1585): update body-parser and package lock fil…

44a4e97

…e to use newer dependencies (uc-cdis#285) * PPS-1564 PPS-1567 PPS-1585: update body-parser and package lock file to use newer dependencies * PPS-1564 PPS-1567 PPS-1585: add file so nvm can set node version

Update quickstart_helm.md (uc-cdis#289)

19c0de2

Fix a dead link

Adds initial support for guppy refresh

1b5a2df

Merge branch 'master' into feature/refresh

f6b3136

run linter

0e8d792

bug fixes

09b84cd

mfshao reviewed Oct 25, 2024

View reviewed changes

fix es-lint errors

d13f30a

mfshao approved these changes Oct 28, 2024

View reviewed changes

mfshao merged commit c89aff7 into uc-cdis:master Oct 29, 2024
2 of 4 checks passed

github-actions bot mentioned this pull request Dec 11, 2024

Gen3 Monthly Release 2024.12 staging.chordshealth.org 1733883498 uc-cdis/cdis-manifest#8181

Open

	export const canRefresh = async (jwt) => {
	export const checkIfUserCanRefreshServer = async (jwt) => {

Feature/refresh #273

Feature/refresh #273

Conversation

matthewpeterkort commented Jul 11, 2024 • edited by mfshao Loading

New Features

Breaking Changes

Bug Fixes

Improvements

Dependency updates

Deployment changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthewpeterkort Jul 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bwalsh Jul 30, 2024 • edited Loading

Choose a reason for hiding this comment

mfshao left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mfshao left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthewpeterkort Oct 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthewpeterkort Oct 25, 2024 • edited Loading

Choose a reason for hiding this comment

mfshao left a comment

Choose a reason for hiding this comment

paulineribeyre commented Nov 4, 2024

mfshao commented Nov 4, 2024

matthewpeterkort commented Jul 11, 2024 •

edited by mfshao

Loading

matthewpeterkort Jul 26, 2024 •

edited

Loading

bwalsh Jul 30, 2024 •

edited

Loading

matthewpeterkort Oct 25, 2024 •

edited

Loading

matthewpeterkort Oct 25, 2024 •

edited

Loading