From 955555b8d4a58c4761e20b7f37c5a285173659df Mon Sep 17 00:00:00 2001 From: Jacky Jiang Date: Wed, 22 Feb 2023 16:10:46 +1100 Subject: [PATCH 01/13] use node-cache to cache queryRegistryRecordApi in ckanRedirectionModule --- magda-gateway/package.json | 1 + .../src/createCkanRedirectionRouter.ts | 27 +++++++++++++++++++ yarn.lock | 12 +++++++++ 3 files changed, 40 insertions(+) diff --git a/magda-gateway/package.json b/magda-gateway/package.json index f389739781..348c4c0ab8 100644 --- a/magda-gateway/package.json +++ b/magda-gateway/package.json @@ -75,6 +75,7 @@ "chai-as-promised": "^7.1.1", "mocha": "^3.4.2", "nock": "https://github.com/magda-io/nock.git#master", + "node-cache": "^5.1.2", "randomstring": "^1.1.5", "sinon": "^2.4.1", "supertest": "^3.0.0", diff --git a/magda-gateway/src/createCkanRedirectionRouter.ts b/magda-gateway/src/createCkanRedirectionRouter.ts index 664c069a3f..b2708b15f5 100644 --- a/magda-gateway/src/createCkanRedirectionRouter.ts +++ b/magda-gateway/src/createCkanRedirectionRouter.ts @@ -4,6 +4,12 @@ import _ from "lodash"; import Registry from "magda-typescript-common/src/registry/RegistryClient"; import unionToThrowable from "magda-typescript-common/src/util/unionToThrowable"; import { escapeRegExp } from "lodash"; +import NodeCache from "node-cache"; + +const registryQueryCache = new NodeCache({ + stdTTL: 15 * 60, + maxKeys: 5000 +}); export type CkanRedirectionRouterOptions = { ckanRedirectionDomain: string; @@ -162,11 +168,31 @@ export default function buildCkanRedirectionRouter({ ); }); + function getQueryRegistryRecordApiCacheKey( + aspectQuery: string[], + aspect: string[], + limit: number = 1 + ) { + return JSON.stringify({ + aspectQuery, + aspect, + limit + }); + } + async function queryRegistryRecordApi( aspectQuery: string[], aspect: string[], limit: number = 1 ) { + const cacheKey = getQueryRegistryRecordApiCacheKey( + aspectQuery, + aspect, + limit + ); + if (registryQueryCache.has(cacheKey)) { + return registryQueryCache.get(cacheKey); + } const queryParameters: any = { limit }; @@ -188,6 +214,7 @@ export default function buildCkanRedirectionRouter({ ) ); + registryQueryCache.set(cacheKey, records); return records; } diff --git a/yarn.lock b/yarn.lock index f187c53ca3..40964523dd 100644 --- a/yarn.lock +++ b/yarn.lock @@ -6979,6 +6979,11 @@ clone-response@^1.0.2: dependencies: mimic-response "^1.0.0" +clone@2.x: + version "2.1.2" + resolved "https://registry.yarnpkg.com/clone/-/clone-2.1.2.tgz#1b7f4b9f591f1e8f83670401600345a02887435f" + integrity sha512-3Pe/CF1Nn94hyhIYpjtiLhdCoEoz0DqQ+988E9gmeEdQZlojxnOb74wctFyuwWQHzqyf9X7C7MG8juUpqBJT8w== + clone@^1.0.2: version "1.0.4" resolved "https://registry.yarnpkg.com/clone/-/clone-1.0.4.tgz#da309cc263df15994c688ca902179ca3c7cd7c7e" @@ -15262,6 +15267,13 @@ node-addon-api@^3.1.0: resolved "https://registry.yarnpkg.com/node-addon-api/-/node-addon-api-3.2.1.tgz#81325e0a2117789c0128dab65e7e38f07ceba161" integrity sha512-mmcei9JghVNDYydghQmeDX8KoAm0FAiYyIcUt/N4nhyAipB17pllZQDOJD2fotxABnt4Mdz+dKTO7eftLg4d0A== +node-cache@^5.1.2: + version "5.1.2" + resolved "https://registry.yarnpkg.com/node-cache/-/node-cache-5.1.2.tgz#f264dc2ccad0a780e76253a694e9fd0ed19c398d" + integrity sha512-t1QzWwnk4sjLWaQAS8CHgOJ+RAfmHpxFWmc36IWTiWHQfs0w5JDMBS1b1ZxQteo0vVVuWJvIUKHDkkeK7vIGCg== + dependencies: + clone "2.x" + node-ensure@^0.0.0: version "0.0.0" resolved "https://registry.yarnpkg.com/node-ensure/-/node-ensure-0.0.0.tgz#ecae764150de99861ec5c810fd5d096b183932a7" From 2890881b9cf9c80bd54200179ca1735d290063c0 Mon Sep 17 00:00:00 2001 From: Jacky Jiang Date: Fri, 24 Feb 2023 11:11:03 +1100 Subject: [PATCH 02/13] - set default cache TTL to 600 seconds - set default max cache item to 500 - make them configurable via helm chart values file --- deploy/helm/internal-charts/gateway/README.md | 2 ++ .../gateway/templates/deployment.yaml | 6 +++++ .../helm/internal-charts/gateway/values.yaml | 8 +++++++ magda-gateway/src/buildApp.ts | 6 ++++- .../src/createCkanRedirectionRouter.ts | 22 +++++++++++++------ magda-gateway/src/index.ts | 12 ++++++++++ 6 files changed, 48 insertions(+), 8 deletions(-) diff --git a/deploy/helm/internal-charts/gateway/README.md b/deploy/helm/internal-charts/gateway/README.md index 90544c9213..4c1959df24 100644 --- a/deploy/helm/internal-charts/gateway/README.md +++ b/deploy/helm/internal-charts/gateway/README.md @@ -51,6 +51,8 @@ Kubernetes: `>= 1.14.0-0` | helmet.frameguard | bool | `false` | | | image.name | string | `"magda-gateway"` | | | proxyTimeout | int | nil (120 seconds default value will be used by upstream lib internally) | How long time (in seconds) before upstream service must complete request in order to avoid request timeout error. If not set, the request will time out after 120 seconds. | +| registryQueryCacheMaxKeys | number | `nil` | Specifies a maximum amount of keys that can be stored in the registryQueryCache. By default, it will be set to 500 seconds if leave blank. | +| registryQueryCacheStdTTL | number | `nil` | The standard ttl as number in seconds for every generated cache element in the registryQueryCache. By default, it will be set to 600 seconds if leave blank. | | resources.limits.cpu | string | `"200m"` | | | resources.requests.cpu | string | `"50m"` | | | resources.requests.memory | string | `"40Mi"` | | diff --git a/deploy/helm/internal-charts/gateway/templates/deployment.yaml b/deploy/helm/internal-charts/gateway/templates/deployment.yaml index 439a470d59..696dfc6fa2 100644 --- a/deploy/helm/internal-charts/gateway/templates/deployment.yaml +++ b/deploy/helm/internal-charts/gateway/templates/deployment.yaml @@ -58,6 +58,12 @@ spec: {{- end }} {{- if .Values.enableCkanRedirection }} "--enableCkanRedirection", {{ .Values.enableCkanRedirection | quote }}, +{{- end }} +{{- if .Values.registryQueryCacheStdTTL }} + "--registryQueryCacheStdTTL", {{ .Values.registryQueryCacheStdTTL | quote }}, +{{- end }} +{{- if .Values.registryQueryCacheMaxKeys }} + "--registryQueryCacheMaxKeys", {{ .Values.registryQueryCacheMaxKeys | quote }}, {{- end }} "--proxyRoutesJson", "/etc/config/routes.json", "--webProxyRoutesJson", "/etc/config/webRoutes.json", diff --git a/deploy/helm/internal-charts/gateway/values.yaml b/deploy/helm/internal-charts/gateway/values.yaml index e039f5dc7d..9b93b2f50c 100644 --- a/deploy/helm/internal-charts/gateway/values.yaml +++ b/deploy/helm/internal-charts/gateway/values.yaml @@ -34,6 +34,14 @@ enableWebAccessControl: false # that is specified by config option `ckanRedirectionDomain` and `ckanRedirectionPath`. enableCkanRedirection: false +# -- (number) The standard ttl as number in seconds for every generated cache element in the registryQueryCache. +# By default, it will be set to 600 seconds if leave blank. +registryQueryCacheStdTTL: + +# -- (number) Specifies a maximum amount of keys that can be stored in the registryQueryCache. +# By default, it will be set to 500 seconds if leave blank. +registryQueryCacheMaxKeys: + # -- CKAN redirection target CKAN instance domain. See `enableCkanRedirection` for more details. ckanRedirectionDomain: diff --git a/magda-gateway/src/buildApp.ts b/magda-gateway/src/buildApp.ts index 5a395801a6..4574f1d281 100644 --- a/magda-gateway/src/buildApp.ts +++ b/magda-gateway/src/buildApp.ts @@ -74,6 +74,8 @@ export type Config = { magdaAdminPortalName?: string; proxyTimeout?: string; skipAuth?: boolean; + registryQueryCacheMaxKeys: number; + registryQueryCacheStdTTL: number; }; export default function buildApp(app: express.Application, config: Config) { @@ -225,7 +227,9 @@ export default function buildApp(app: express.Application, config: Config) { ckanRedirectionDomain: config.ckanRedirectionDomain, ckanRedirectionPath: config.ckanRedirectionPath, registryApiBaseUrlInternal: routes.registry.to, - tenantId: 0 // FIXME: Rather than being hard-coded to the default tenant, the CKAN router needs to figure out the correct tenant. + tenantId: 0, // FIXME: Rather than being hard-coded to the default tenant, the CKAN router needs to figure out the correct tenant., + cacheStdTTL: config.registryQueryCacheStdTTL, + cacheMaxKeys: config.registryQueryCacheMaxKeys }) ); } diff --git a/magda-gateway/src/createCkanRedirectionRouter.ts b/magda-gateway/src/createCkanRedirectionRouter.ts index b2708b15f5..1212df710d 100644 --- a/magda-gateway/src/createCkanRedirectionRouter.ts +++ b/magda-gateway/src/createCkanRedirectionRouter.ts @@ -6,16 +6,13 @@ import unionToThrowable from "magda-typescript-common/src/util/unionToThrowable" import { escapeRegExp } from "lodash"; import NodeCache from "node-cache"; -const registryQueryCache = new NodeCache({ - stdTTL: 15 * 60, - maxKeys: 5000 -}); - export type CkanRedirectionRouterOptions = { ckanRedirectionDomain: string; ckanRedirectionPath: string; registryApiBaseUrlInternal: string; tenantId: number; + cacheStdTTL: number; + cacheMaxKeys: number; }; export type genericUrlRedirectConfig = @@ -82,8 +79,14 @@ export default function buildCkanRedirectionRouter({ ckanRedirectionDomain, ckanRedirectionPath, registryApiBaseUrlInternal, - tenantId + tenantId, + cacheStdTTL, + cacheMaxKeys }: CkanRedirectionRouterOptions): express.Router { + const registryQueryCache = new NodeCache({ + stdTTL: cacheStdTTL, + maxKeys: cacheMaxKeys + }); const router = express.Router(); const registry = new Registry({ baseUrl: registryApiBaseUrlInternal, @@ -214,7 +217,12 @@ export default function buildCkanRedirectionRouter({ ) ); - registryQueryCache.set(cacheKey, records); + try { + registryQueryCache.set(cacheKey, records); + } catch (e) { + console.log("Failed to save registryQueryCache: " + e); + } + return records; } diff --git a/magda-gateway/src/index.ts b/magda-gateway/src/index.ts index e921033d04..7c35682dd4 100755 --- a/magda-gateway/src/index.ts +++ b/magda-gateway/src/index.ts @@ -139,6 +139,18 @@ const argv = addJwtSecretFromEnvVar( type: "string", default: "" }) + .option("registryQueryCacheStdTTL", { + describe: + "the standard ttl as number in seconds for every generated cache element in the registryQueryCache.", + type: "number", + default: 600 + }) + .option("registryQueryCacheMaxKeys", { + describe: + "specifies a maximum amount of keys that can be stored in the registryQueryCache.", + type: "number", + default: 500 + }) .option("enableWebAccessControl", { describe: "Whether users are required to enter a username & password to access the magda web interface", From cb4444d50410146598dd738bcd9e694372e79126 Mon Sep 17 00:00:00 2001 From: Jacky Jiang Date: Fri, 24 Feb 2023 11:24:43 +1100 Subject: [PATCH 03/13] make sure createCkanRedirectionRouter will use readonly registry API when it's available --- magda-gateway/src/buildApp.ts | 8 +++++++- magda-gateway/src/defaultConfig.ts | 11 ++++++++++- 2 files changed, 17 insertions(+), 2 deletions(-) diff --git a/magda-gateway/src/buildApp.ts b/magda-gateway/src/buildApp.ts index 4574f1d281..51e86ab847 100644 --- a/magda-gateway/src/buildApp.ts +++ b/magda-gateway/src/buildApp.ts @@ -29,6 +29,7 @@ import AuthDecisionQueryClient from "magda-typescript-common/src/opa/AuthDecisio type Route = { to: string; auth?: boolean; + methods?: { method: string; target: string }[]; }; type Routes = { @@ -222,11 +223,16 @@ export default function buildApp(app: express.Application, config: Config) { "Cannot locate routes.registry for ckan redirection!" ); } else { + const registryReadOnlyApi = routes.registry?.methods?.find( + (item) => item.method.toLowerCase() === "get" + )?.target; mainRouter.use( createCkanRedirectionRouter({ ckanRedirectionDomain: config.ckanRedirectionDomain, ckanRedirectionPath: config.ckanRedirectionPath, - registryApiBaseUrlInternal: routes.registry.to, + registryApiBaseUrlInternal: registryReadOnlyApi + ? registryReadOnlyApi + : routes.registry?.to, tenantId: 0, // FIXME: Rather than being hard-coded to the default tenant, the CKAN router needs to figure out the correct tenant., cacheStdTTL: config.registryQueryCacheStdTTL, cacheMaxKeys: config.registryQueryCacheMaxKeys diff --git a/magda-gateway/src/defaultConfig.ts b/magda-gateway/src/defaultConfig.ts index fc96451b2f..e83d43a4b1 100644 --- a/magda-gateway/src/defaultConfig.ts +++ b/magda-gateway/src/defaultConfig.ts @@ -10,7 +10,16 @@ export default { }, registry: { to: "http://localhost:6101/v0", - auth: true + auth: true, + methods: [ + { method: "get", target: "http://localhost:6101/v0" }, + { method: "head", target: "http://localhost:6101/v0" }, + { method: "options", target: "http://localhost:6101/v0" }, + { method: "post", target: "http://localhost:6101/v0" }, + { method: "put", target: "http://localhost:6101/v0" }, + { method: "patch", target: "http://localhost:6101/v0" }, + { method: "delete", target: "http://localhost:6101/v0" } + ] }, "registry-read-only": { to: "http://localhost:6101/v0", From dbad5b5a0ee1a0181f535d8c85736dcd1d8e5fac Mon Sep 17 00:00:00 2001 From: Jacky Jiang Date: Fri, 24 Feb 2023 11:35:19 +1100 Subject: [PATCH 04/13] fix test cases --- magda-gateway/src/test/ckanRedirection.spec.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/magda-gateway/src/test/ckanRedirection.spec.ts b/magda-gateway/src/test/ckanRedirection.spec.ts index fe736e8efe..140f4c8210 100644 --- a/magda-gateway/src/test/ckanRedirection.spec.ts +++ b/magda-gateway/src/test/ckanRedirection.spec.ts @@ -39,7 +39,9 @@ describe("ckanRedirectionRouter router", () => { ckanRedirectionDomain, ckanRedirectionPath, registryApiBaseUrlInternal: registryUrl, - tenantId: 1 + tenantId: 1, + cacheMaxKeys: 500, + cacheStdTTL: 600 }); app = express(); app.use(router); From 49e9e010ae9daebb9073502f958788bb42ea92fe Mon Sep 17 00:00:00 2001 From: Jacky Jiang Date: Fri, 24 Feb 2023 13:35:13 +1100 Subject: [PATCH 05/13] fix test cases --- magda-gateway/src/test/createAuthApiKeyMiddleware.spec.ts | 4 +++- .../src/test/createHttpsRedirectionMiddleware.spec.ts | 4 +++- magda-gateway/src/test/integration.spec.ts | 2 ++ magda-gateway/src/test/proxy.spec.ts | 4 +++- 4 files changed, 11 insertions(+), 3 deletions(-) diff --git a/magda-gateway/src/test/createAuthApiKeyMiddleware.spec.ts b/magda-gateway/src/test/createAuthApiKeyMiddleware.spec.ts index d82917fe48..1687a1b0d6 100644 --- a/magda-gateway/src/test/createAuthApiKeyMiddleware.spec.ts +++ b/magda-gateway/src/test/createAuthApiKeyMiddleware.spec.ts @@ -71,7 +71,9 @@ const defaultAppOptions = { web: "https://127.0.0.1", tenantUrl: "http://tenant", defaultCacheControl: "DEFAULT CACHE CONTROL", - authPluginConfigJson: [] as AuthPluginBasicConfig[] + authPluginConfigJson: [] as AuthPluginBasicConfig[], + registryQueryCacheStdTTL: 600, + registryQueryCacheMaxKeys: 500 }; function getUserIdFromNockReq(req: any, jwtSecret: string) { diff --git a/magda-gateway/src/test/createHttpsRedirectionMiddleware.spec.ts b/magda-gateway/src/test/createHttpsRedirectionMiddleware.spec.ts index a64949bf41..62ad72d07f 100644 --- a/magda-gateway/src/test/createHttpsRedirectionMiddleware.spec.ts +++ b/magda-gateway/src/test/createHttpsRedirectionMiddleware.spec.ts @@ -40,7 +40,9 @@ const defaultAppOptions = { web: "https://127.0.0.1", tenantUrl: "http://tenant", defaultCacheControl: "DEFAULT CACHE CONTROL", - authPluginConfigJson: [] as AuthPluginBasicConfig[] + authPluginConfigJson: [] as AuthPluginBasicConfig[], + registryQueryCacheStdTTL: 600, + registryQueryCacheMaxKeys: 500 }; describe("Test createHttpsRedirectionMiddleware", () => { diff --git a/magda-gateway/src/test/integration.spec.ts b/magda-gateway/src/test/integration.spec.ts index a5573bfbf3..522180ad03 100644 --- a/magda-gateway/src/test/integration.spec.ts +++ b/magda-gateway/src/test/integration.spec.ts @@ -120,6 +120,8 @@ describe("Integration Tests", function (this: Mocha.ISuiteCallbackContext) { defaultCacheControl: "public, max-age=60", openfaasGatewayUrl: undefined, authPluginConfigJson: [], + registryQueryCacheStdTTL: 600, + registryQueryCacheMaxKeys: 500, ...config }); diff --git a/magda-gateway/src/test/proxy.spec.ts b/magda-gateway/src/test/proxy.spec.ts index 6a585d02a4..551bd83820 100644 --- a/magda-gateway/src/test/proxy.spec.ts +++ b/magda-gateway/src/test/proxy.spec.ts @@ -49,7 +49,9 @@ const defaultAppOptions = { web: "https://127.0.0.1", tenantUrl: "http://tenant", defaultCacheControl: "DEFAULT CACHE CONTROL", - authPluginConfigJson: [] as AuthPluginBasicConfig[] + authPluginConfigJson: [] as AuthPluginBasicConfig[], + registryQueryCacheStdTTL: 600, + registryQueryCacheMaxKeys: 500 }; describe("proxying", () => { From 52a4403752e94fa867e26a8059ffe4009b63575d Mon Sep 17 00:00:00 2001 From: Jacky Jiang Date: Fri, 24 Feb 2023 14:45:24 +1100 Subject: [PATCH 06/13] Allow cache to be disabled by passing cacheMaxKeys==0 --- deploy/helm/internal-charts/gateway/README.md | 2 +- .../gateway/templates/deployment.yaml | 2 +- deploy/helm/internal-charts/gateway/values.yaml | 1 + magda-gateway/src/createCkanRedirectionRouter.ts | 15 +++++++++------ magda-gateway/src/index.ts | 2 +- 5 files changed, 13 insertions(+), 9 deletions(-) diff --git a/deploy/helm/internal-charts/gateway/README.md b/deploy/helm/internal-charts/gateway/README.md index 4c1959df24..de98c8f543 100644 --- a/deploy/helm/internal-charts/gateway/README.md +++ b/deploy/helm/internal-charts/gateway/README.md @@ -52,7 +52,7 @@ Kubernetes: `>= 1.14.0-0` | image.name | string | `"magda-gateway"` | | | proxyTimeout | int | nil (120 seconds default value will be used by upstream lib internally) | How long time (in seconds) before upstream service must complete request in order to avoid request timeout error. If not set, the request will time out after 120 seconds. | | registryQueryCacheMaxKeys | number | `nil` | Specifies a maximum amount of keys that can be stored in the registryQueryCache. By default, it will be set to 500 seconds if leave blank. | -| registryQueryCacheStdTTL | number | `nil` | The standard ttl as number in seconds for every generated cache element in the registryQueryCache. By default, it will be set to 600 seconds if leave blank. | +| registryQueryCacheStdTTL | number | `nil` | The standard ttl as number in seconds for every generated cache element in the registryQueryCache. To disable the cache, set this value to `0`. By default, it will be set to 600 seconds if leave blank. | | resources.limits.cpu | string | `"200m"` | | | resources.requests.cpu | string | `"50m"` | | | resources.requests.memory | string | `"40Mi"` | | diff --git a/deploy/helm/internal-charts/gateway/templates/deployment.yaml b/deploy/helm/internal-charts/gateway/templates/deployment.yaml index 696dfc6fa2..135bb56d31 100644 --- a/deploy/helm/internal-charts/gateway/templates/deployment.yaml +++ b/deploy/helm/internal-charts/gateway/templates/deployment.yaml @@ -59,7 +59,7 @@ spec: {{- if .Values.enableCkanRedirection }} "--enableCkanRedirection", {{ .Values.enableCkanRedirection | quote }}, {{- end }} -{{- if .Values.registryQueryCacheStdTTL }} +{{- if not (kindIs "invalid" .Values.registryQueryCacheStdTTL) }} "--registryQueryCacheStdTTL", {{ .Values.registryQueryCacheStdTTL | quote }}, {{- end }} {{- if .Values.registryQueryCacheMaxKeys }} diff --git a/deploy/helm/internal-charts/gateway/values.yaml b/deploy/helm/internal-charts/gateway/values.yaml index 9b93b2f50c..de2c499bd1 100644 --- a/deploy/helm/internal-charts/gateway/values.yaml +++ b/deploy/helm/internal-charts/gateway/values.yaml @@ -35,6 +35,7 @@ enableWebAccessControl: false enableCkanRedirection: false # -- (number) The standard ttl as number in seconds for every generated cache element in the registryQueryCache. +# To disable the cache, set this value to `0`. # By default, it will be set to 600 seconds if leave blank. registryQueryCacheStdTTL: diff --git a/magda-gateway/src/createCkanRedirectionRouter.ts b/magda-gateway/src/createCkanRedirectionRouter.ts index 1212df710d..8fdc41fb50 100644 --- a/magda-gateway/src/createCkanRedirectionRouter.ts +++ b/magda-gateway/src/createCkanRedirectionRouter.ts @@ -83,10 +83,13 @@ export default function buildCkanRedirectionRouter({ cacheStdTTL, cacheMaxKeys }: CkanRedirectionRouterOptions): express.Router { - const registryQueryCache = new NodeCache({ - stdTTL: cacheStdTTL, - maxKeys: cacheMaxKeys - }); + const registryQueryCache = + cacheMaxKeys === 0 // turn off the cache when cacheMaxKeys==0 + ? null + : new NodeCache({ + stdTTL: cacheStdTTL, + maxKeys: cacheMaxKeys + }); const router = express.Router(); const registry = new Registry({ baseUrl: registryApiBaseUrlInternal, @@ -193,7 +196,7 @@ export default function buildCkanRedirectionRouter({ aspect, limit ); - if (registryQueryCache.has(cacheKey)) { + if (registryQueryCache?.has(cacheKey)) { return registryQueryCache.get(cacheKey); } const queryParameters: any = { @@ -218,7 +221,7 @@ export default function buildCkanRedirectionRouter({ ); try { - registryQueryCache.set(cacheKey, records); + registryQueryCache?.set(cacheKey, records); } catch (e) { console.log("Failed to save registryQueryCache: " + e); } diff --git a/magda-gateway/src/index.ts b/magda-gateway/src/index.ts index 7c35682dd4..d0bd88e401 100755 --- a/magda-gateway/src/index.ts +++ b/magda-gateway/src/index.ts @@ -147,7 +147,7 @@ const argv = addJwtSecretFromEnvVar( }) .option("registryQueryCacheMaxKeys", { describe: - "specifies a maximum amount of keys that can be stored in the registryQueryCache.", + "specifies a maximum amount of keys that can be stored in the registryQueryCache. To disable the cache, set this value to `0`.", type: "number", default: 500 }) From eb2d706fc2893ebeea6e4193626f6d31aab6d947 Mon Sep 17 00:00:00 2001 From: Jacky Jiang Date: Fri, 24 Feb 2023 15:07:45 +1100 Subject: [PATCH 07/13] add test cases --- .../src/test/ckanRedirection.spec.ts | 205 ++++++++++++++++-- 1 file changed, 189 insertions(+), 16 deletions(-) diff --git a/magda-gateway/src/test/ckanRedirection.spec.ts b/magda-gateway/src/test/ckanRedirection.spec.ts index 140f4c8210..136d34dcfa 100644 --- a/magda-gateway/src/test/ckanRedirection.spec.ts +++ b/magda-gateway/src/test/ckanRedirection.spec.ts @@ -17,6 +17,22 @@ import resCkanDatasetQuery from "./sampleRegistryResponses/ckanDatasetQuery.json import resCkanOrganizationQuery from "./sampleRegistryResponses/ckanOrganizationQuery.json"; import resCkanResource from "./sampleRegistryResponses/ckanResource.json"; +function checkRedirectionDetails(location: string | RegExp) { + return (res: supertest.Response) => { + if (_.isRegExp(location)) { + expect(location.test(res.header["location"])).to.equal(true); + } else { + expect(res.header["location"]).to.equal(location); + } + }; +} + +function checkStatusCode(statusCode: number = 308) { + return (res: supertest.Response) => { + expect(res.status).to.equal(statusCode); + }; +} + describe("ckanRedirectionRouter router", () => { const ckanRedirectionDomain = "ckan.data.gov.au"; const ckanRedirectionPath = ""; @@ -476,22 +492,6 @@ describe("ckanRedirectionRouter router", () => { }); } - function checkRedirectionDetails(location: string | RegExp) { - return (res: supertest.Response) => { - if (_.isRegExp(location)) { - expect(location.test(res.header["location"])).to.equal(true); - } else { - expect(res.header["location"]).to.equal(location); - } - }; - } - - function checkStatusCode(statusCode: number = 308) { - return (res: supertest.Response) => { - expect(res.status).to.equal(statusCode); - }; - } - function testCkanDomainChangeOnly( targetUrlOrUrls: string | string[], statusCode: number = 308, @@ -572,3 +572,176 @@ describe("ckanRedirectionRouter router", () => { }); } }); + +describe("ckanRedirectionRouter router cache", () => { + const ckanRedirectionDomain = "ckan.data.gov.au"; + const ckanRedirectionPath = ""; + + let app: express.Application; + const registryUrl = "http://registry.example.com"; + let registryScope: nock.Scope; + let registryQueryCount: number = 0; + + function setupRegistryApiForCkanDatasetQuery() { + registryQueryCount = 0; + const errorResponse = `{ + "hasMore": false, + "records": [ ] + }`; + + const okCkanDatasetResponse = resCkanDatasetQuery; + + registryScope + .persist() + .get("/records") + .query(true) + .reply(200, function (uri: string) { + registryQueryCount++; + const uriObj = URI(uri); + const query = uriObj.search(true); + if (!query || !query.aspectQuery) return errorResponse; + const [path, value] = (query.aspectQuery as string).split(":"); + if ( + path === "ckan-dataset.name" && + value === "pg_skafsd0_f___00120141210_11a" + ) { + return okCkanDatasetResponse; + } else if ( + path === "ckan-dataset.id" && + value === "8beb4387-ec03-46f9-8048-3ad76c0416c8" + ) { + return okCkanDatasetResponse; + } else { + return errorResponse; + } + }); + } + + before(() => { + registryScope = nock(registryUrl); + setupRegistryApiForCkanDatasetQuery(); + }); + + after(() => { + nock.cleanAll(); + }); + + beforeEach(() => { + app = express(); + registryQueryCount = 0; + }); + + afterEach(() => { + if ((console.error).restore) { + (console.error).restore(); + } + }); + + function setCkanRedirectRouter( + cacheMaxKeys: number = 500, + cacheStdTTL: number = 600 + ) { + const router = createCkanRedirectionRouter({ + ckanRedirectionDomain, + ckanRedirectionPath, + registryApiBaseUrlInternal: registryUrl, + tenantId: 1, + cacheMaxKeys, + cacheStdTTL + }); + app.use(router); + } + + it("should query registry only once for multiple requests to the same URL", async () => { + setCkanRedirectRouter(); + for (let i = 0; i < 3; i++) { + await supertest(app) + .get("/dataset/pg_skafsd0_f___00120141210_11a") + .expect(303) + .expect( + checkRedirectionDetails( + "/dataset/ds-dga-8beb4387-ec03-46f9-8048-3ad76c0416c8/details" + ) + ); + } + expect(registryQueryCount).to.equal(1); + }); + + it("should query registry once for every request to the same URL", async () => { + // set cacheMaxKeys = 0 to disable the cache + setCkanRedirectRouter(0); + for (let i = 0; i < 3; i++) { + await supertest(app) + .get("/dataset/pg_skafsd0_f___00120141210_11a") + .expect(303) + .expect( + checkRedirectionDetails( + "/dataset/ds-dga-8beb4387-ec03-46f9-8048-3ad76c0416c8/details" + ) + ); + } + expect(registryQueryCount).to.equal(3); + }); + + it("should query registry more than once for different requests to different URLs when cacheMaxKeys = 1", async () => { + setCkanRedirectRouter(1); + await supertest(app) + .get("/dataset/pg_skafsd0_f___00120141210_11a") + .expect(303) + .expect( + checkRedirectionDetails( + "/dataset/ds-dga-8beb4387-ec03-46f9-8048-3ad76c0416c8/details" + ) + ); + await supertest(app) // this request will not be cached + .get("/dataset/8beb4387-ec03-46f9-8048-3ad76c0416c8") + .expect(303) + .expect( + checkRedirectionDetails( + "/dataset/ds-dga-8beb4387-ec03-46f9-8048-3ad76c0416c8/details" + ) + ); + for (let i = 0; i < 3; i++) { + await supertest(app) + .get("/dataset/8beb4387-ec03-46f9-8048-3ad76c0416c8") + .expect(303) + .expect( + checkRedirectionDetails( + "/dataset/ds-dga-8beb4387-ec03-46f9-8048-3ad76c0416c8/details" + ) + ); + } + expect(registryQueryCount).to.equal(5); + }); + + it("should query registry 2 times for different requests to different URLs when cacheMaxKeys = 2", async () => { + setCkanRedirectRouter(2); + await supertest(app) + .get("/dataset/pg_skafsd0_f___00120141210_11a") + .expect(303) + .expect( + checkRedirectionDetails( + "/dataset/ds-dga-8beb4387-ec03-46f9-8048-3ad76c0416c8/details" + ) + ); + await supertest(app) // this request will not be cached + .get("/dataset/8beb4387-ec03-46f9-8048-3ad76c0416c8") + .expect(303) + .expect( + checkRedirectionDetails( + "/dataset/ds-dga-8beb4387-ec03-46f9-8048-3ad76c0416c8/details" + ) + ); + for (let i = 0; i < 3; i++) { + await supertest(app) + .get("/dataset/8beb4387-ec03-46f9-8048-3ad76c0416c8") + .expect(303) + .expect( + checkRedirectionDetails( + "/dataset/ds-dga-8beb4387-ec03-46f9-8048-3ad76c0416c8/details" + ) + ); + } + expect(registryQueryCount).to.equal(2); + }); +}); From 9ab6339a76dd5eb84a9e9596804a5b8c3e8e60fa Mon Sep 17 00:00:00 2001 From: Jacky Jiang Date: Fri, 24 Feb 2023 16:08:29 +1100 Subject: [PATCH 08/13] update changes.md --- CHANGES.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGES.md b/CHANGES.md index f26229c36e..720a141e6f 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -3,6 +3,7 @@ ## v2.2.3 - Fixed an issue that zendesk integration feedback form might not always be opened via footer link +- #3449: Gateway ckanRedirection module performance improvement ## v2.2.2 From 93399584654d3635bd86d4be12a36396f0eed04b Mon Sep 17 00:00:00 2001 From: Jacky Jiang Date: Fri, 24 Feb 2023 17:31:25 +1100 Subject: [PATCH 09/13] related to #3448: make web server module to use read only registry node for site map generation --- CHANGES.md | 1 + deploy/helm/internal-charts/web-server/README.md | 2 +- deploy/helm/internal-charts/web-server/values.yaml | 2 +- 3 files changed, 3 insertions(+), 2 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index 720a141e6f..1fa9a54507 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -4,6 +4,7 @@ - Fixed an issue that zendesk integration feedback form might not always be opened via footer link - #3449: Gateway ckanRedirection module performance improvement +- related to #3448: make web server module to use read only registry node for site map generation ## v2.2.2 diff --git a/deploy/helm/internal-charts/web-server/README.md b/deploy/helm/internal-charts/web-server/README.md index 2028844466..bbe166300c 100644 --- a/deploy/helm/internal-charts/web-server/README.md +++ b/deploy/helm/internal-charts/web-server/README.md @@ -99,7 +99,7 @@ Kubernetes: `>= 1.14.0-0` | openInExternalTerriaMapButtonText | string | `nil` | When set, the string here will replace the text of the `Open in National Map` button in Map Preview area. | | openInExternalTerriaMapTargetUrl | string | `nil` | When set, the `Open in National Map` button in Map Preview area will sent map data to the URL provided and open the map preview there. When not set, UI will by default send to the National Map. | | previewMapFormatPerference | list | `[{"format":"WMS","urlRegex":"^(?!.*(SceneServer)).*$"},{"format":"ESRI MAPSERVER","urlRegex":"MapServer"},{"format":"WFS","urlRegex":"^(?!.*(SceneServer)).*$"},{"format":"ESRI FEATURESERVER","urlRegex":"FeatureServer"},{"format":"GeoJSON","singleFile":true},{"format":"csv-geo-au","singleFile":true},{"format":"KML","singleFile":true},{"format":"KMZ","singleFile":true}]` | Preview map module format perference list The list includes one or more `format perference item`. When there are more than one data source available, "Preview Map module" will use this perference to determine which data soruce will be used. It will go through the perference list. The first matched format (i.e. find a data source with the format ) will be chosen. A `format perference item` can have the following fields:
  • format: the format of the preferred data source. compulsory. case insensitive.
  • isDataFile: Optional. Default to `false`. Indicate whether the specified format is a static data file or API. If it's a static file, "Preview Map Module" will attempt to check the target file size and ask user to confirm whether he wants to render the file for large files. The file size threshold is specified by config option `automaticPreviewMaxFileSize`.
  • urlRegex: optional; when exists, it will be used as regex string to double check the data source access url to confirm whether it's indeed the format specified. If not provided or empty, only `format` will be used to determine whether a data source matches the `format perference item`.
| -| registryApiBaseUrlInternal | string | `"http://registry-api/v0"` | | +| registryApiBaseUrlInternal | string | `"http://registry-api-read-only/v0"` | | | replicas | string | `nil` | no. of replicas required for the deployment. If not set, k8s will assume `1` but allows HPA (autoscaler) alters it. @default 1 | | resources.limits.cpu | string | `"100m"` | | | resources.requests.cpu | string | `"10m"` | | diff --git a/deploy/helm/internal-charts/web-server/values.yaml b/deploy/helm/internal-charts/web-server/values.yaml index 4c6cd1a943..80fd6d3c89 100644 --- a/deploy/helm/internal-charts/web-server/values.yaml +++ b/deploy/helm/internal-charts/web-server/values.yaml @@ -46,7 +46,7 @@ useLocalStyleSheet: false contentApiBaseUrlInternal: "http://content-api/v0/" -registryApiBaseUrlInternal: "http://registry-api/v0" +registryApiBaseUrlInternal: "http://registry-api-read-only/v0" adminApiBaseUrl: From f753ab0e014a05594f495828ee3219be5d122280 Mon Sep 17 00:00:00 2001 From: Jacky Jiang Date: Fri, 24 Feb 2023 18:16:21 +1100 Subject: [PATCH 10/13] fix: node-cache has been incorrectly set as `devDependencies` --- magda-gateway/package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/magda-gateway/package.json b/magda-gateway/package.json index 348c4c0ab8..e82ddf20b0 100644 --- a/magda-gateway/package.json +++ b/magda-gateway/package.json @@ -40,6 +40,7 @@ "pg": "^8.7.3", "read-pkg-up": "^3.0.0", "request": "^2.88.0", + "node-cache": "^5.1.2", "tsmonad": "^0.7.2", "urijs": "^1.19.11", "uuid": "^8.2.0", @@ -75,7 +76,6 @@ "chai-as-promised": "^7.1.1", "mocha": "^3.4.2", "nock": "https://github.com/magda-io/nock.git#master", - "node-cache": "^5.1.2", "randomstring": "^1.1.5", "sinon": "^2.4.1", "supertest": "^3.0.0", From 63bc717da3df6d07b022347025dac39fbe4e5d59 Mon Sep 17 00:00:00 2001 From: Jacky Jiang Date: Fri, 24 Feb 2023 19:33:08 +1100 Subject: [PATCH 11/13] update ckanRedirectionDomain related helm chart doc --- deploy/helm/internal-charts/gateway/README.md | 4 ++-- deploy/helm/internal-charts/gateway/values.yaml | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/deploy/helm/internal-charts/gateway/README.md b/deploy/helm/internal-charts/gateway/README.md index de98c8f543..5def85e474 100644 --- a/deploy/helm/internal-charts/gateway/README.md +++ b/deploy/helm/internal-charts/gateway/README.md @@ -18,8 +18,8 @@ Kubernetes: `>= 1.14.0-0` | autoscaler.maxReplicas | int | `3` | | | autoscaler.minReplicas | int | `1` | | | autoscaler.targetCPUUtilizationPercentage | int | `80` | | -| ckanRedirectionDomain | string | `nil` | CKAN redirection target CKAN instance domain. See `enableCkanRedirection` for more details. | -| ckanRedirectionPath | string | `nil` | CKAN redirection target CKAN instance path. See `enableCkanRedirection` for more details. | +| ckanRedirectionDomain | string | `nil` | CKAN redirection target CKAN instance domain (e.g. `data.gov.au`). See `enableCkanRedirection` for more details. | +| ckanRedirectionPath | string | `nil` | CKAN redirection target CKAN instance path (e.g. `/data`). See `enableCkanRedirection` for more details. | | cookie | object | default value see `Description` | Session cookie settings.
More info: https://github.com/expressjs/session#cookie
Supported options are:
  • `expires`: A fix cookie expire date. The expires option should not be set directly; instead only use the maxAge option.
  • `httpOnly`: Default: true.
  • `maxAge`: Default: 7 * 60 * 60 * 1000 (7 hours)
  • `path`: Default: '/'.
  • `sameSite`: Default: lax
  • `secure`: Default: "auto"
| | cors.exposedHeaders[0] | string | `"Content-Range"` | | | cors.exposedHeaders[1] | string | `"X-Content-Range"` | | diff --git a/deploy/helm/internal-charts/gateway/values.yaml b/deploy/helm/internal-charts/gateway/values.yaml index de2c499bd1..608234424d 100644 --- a/deploy/helm/internal-charts/gateway/values.yaml +++ b/deploy/helm/internal-charts/gateway/values.yaml @@ -43,10 +43,10 @@ registryQueryCacheStdTTL: # By default, it will be set to 500 seconds if leave blank. registryQueryCacheMaxKeys: -# -- CKAN redirection target CKAN instance domain. See `enableCkanRedirection` for more details. +# -- CKAN redirection target CKAN instance domain (e.g. `data.gov.au`). See `enableCkanRedirection` for more details. ckanRedirectionDomain: -# -- CKAN redirection target CKAN instance path. See `enableCkanRedirection` for more details. +# -- CKAN redirection target CKAN instance path (e.g. `/data`). See `enableCkanRedirection` for more details. ckanRedirectionPath: From 43eed07f782cd510bafdee20f736abdd5069a372 Mon Sep 17 00:00:00 2001 From: Jacky Jiang Date: Fri, 24 Feb 2023 22:34:28 +1100 Subject: [PATCH 12/13] add more test cases --- magda-gateway/src/test/integration.spec.ts | 88 ++++++++++++++++++++++ 1 file changed, 88 insertions(+) diff --git a/magda-gateway/src/test/integration.spec.ts b/magda-gateway/src/test/integration.spec.ts index 522180ad03..a6c94e94b6 100644 --- a/magda-gateway/src/test/integration.spec.ts +++ b/magda-gateway/src/test/integration.spec.ts @@ -471,4 +471,92 @@ describe("Integration Tests", function (this: Mocha.ISuiteCallbackContext) { await app.put("/xxx").expect(200, resText2); }); }); + + describe("ckanRedirectionRouter setup", () => { + it("should use read only registry API endpoint when available", async () => { + const app = setupTestApp({ + enableCkanRedirection: true, + proxyRoutesJson: { + registry: { + to: "http://registry-full.com", + auth: true, + statusCheck: false, + methods: [ + { + method: "get", + target: "http://registry-readonly.com" + } + ] + } + }, + enableWebAccessControl: false + }); + + const registryReadOnly = nock("http://registry-readonly.com") + .get("/records") + .query(true) + .reply( + 200, + `{ + hasMore: false, + records: [] + }` + ); + + const registryFull = nock("http://registry-full.com") + .get("/records") + .query(true) + .reply( + 200, + `{ + hasMore: false, + records: [] + }` + ); + + await app + .get("/dataset/pg_skafsd0_f___00120141210_11a") + .expect(303); + + expect(registryFull.isDone()).to.be.false; + expect(registryReadOnly.isDone()).to.be.true; + }); + + it("should use full registry API endpoint when read only node is not available", async () => { + const app = setupTestApp({ + enableCkanRedirection: true, + proxyRoutesJson: { + registry: { + to: "http://registry-full.com", + auth: true, + statusCheck: false + } + }, + enableWebAccessControl: false + }); + + const registryReadOnly = nock("http://registry-readonly.com") + .get("/records") + .query(true) + .reply(200, { + hasMore: false, + records: [] + }); + + const registryFull = nock("http://registry-full.com") + .get("/records") + .query(true) + .reply(200, { + hasMore: false, + records: [] + }); + + await app + .get("/dataset/pg_skafsd0_f___00120141210_11a") + .expect(303); + + expect(registryFull.isDone()).to.be.true; + expect(registryReadOnly.isDone()).to.be.false; + }); + }); }); From 8c395cedbe90e6e88528d084a07b5aa6cfc9b241 Mon Sep 17 00:00:00 2001 From: Jacky Jiang Date: Sat, 25 Feb 2023 01:24:50 +1100 Subject: [PATCH 13/13] fixed performance issue: - limit=1 will largely impact the performance negatively - specify the `aspect` parameter with aspectQuery will help speed up the query --- magda-gateway/src/createCkanRedirectionRouter.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/magda-gateway/src/createCkanRedirectionRouter.ts b/magda-gateway/src/createCkanRedirectionRouter.ts index 8fdc41fb50..441ed807af 100644 --- a/magda-gateway/src/createCkanRedirectionRouter.ts +++ b/magda-gateway/src/createCkanRedirectionRouter.ts @@ -177,7 +177,7 @@ export default function buildCkanRedirectionRouter({ function getQueryRegistryRecordApiCacheKey( aspectQuery: string[], aspect: string[], - limit: number = 1 + limit: number ) { return JSON.stringify({ aspectQuery, @@ -189,7 +189,7 @@ export default function buildCkanRedirectionRouter({ async function queryRegistryRecordApi( aspectQuery: string[], aspect: string[], - limit: number = 1 + limit: number = undefined ) { const cacheKey = getQueryRegistryRecordApiCacheKey( aspectQuery, @@ -234,7 +234,7 @@ export default function buildCkanRedirectionRouter({ aspectName: string, retrieveAspectContent: boolean = true, retrieveAspects: string[] = [], - limit: number = 1 + limit: number = undefined ): Promise { const query = `${aspectName}.${ uuidRegEx.test(ckanIdOrName) ? "id" : "name" @@ -270,7 +270,7 @@ export default function buildCkanRedirectionRouter({ ckanIdOrName: string, aspectName: string ) { - const records = await queryCkanAspect(ckanIdOrName, aspectName, false); + const records = await queryCkanAspect(ckanIdOrName, aspectName); if (!records || !records.length || !records[0]["id"]) { return null; } else {