From 530d9515b3034d09bea95d3d340c3c190cd03858 Mon Sep 17 00:00:00 2001 From: Dmitriy Rudko Date: Mon, 13 Dec 2021 16:14:04 -0800 Subject: [PATCH 1/6] fix: [BUG 59] - Increase timeout for MSI call. Added retry logic --- src/cloud/providers/azure/credentials.ts | 52 ++++++++++++++++++++++-- 1 file changed, 49 insertions(+), 3 deletions(-) diff --git a/src/cloud/providers/azure/credentials.ts b/src/cloud/providers/azure/credentials.ts index 140c9c7b..bf3d77af 100644 --- a/src/cloud/providers/azure/credentials.ts +++ b/src/cloud/providers/azure/credentials.ts @@ -23,8 +23,9 @@ import { SASProtocol, UserDelegationKey } from '@azure/storage-blob'; -import { AzureConfig } from './config'; -import { DefaultAzureCredential, TokenCredential } from '@azure/identity'; +import { DefaultAzureCredential, TokenCredential, DefaultAzureCredentialOptions} from '@azure/identity'; +import {ExponentialRetryPolicyOptions} from '@azure/core-rest-pipeline' +import { AccessToken, GetTokenOptions } from "@azure/core-auth"; import request from 'request-promise'; import { AzureDataEcosystemServices } from './dataecosystem'; @@ -69,7 +70,7 @@ export class AzureCredentials extends AbstractCredentials { // - AZURE_CLIENT_SECRET: The client secret for the registered application // https://docs.microsoft.com/en-us/azure/active-directory/managed-identities-azure-resources/overview - return new DefaultAzureCredential(); + return new RetriableAzureCredential();; } @@ -194,3 +195,48 @@ export class AzureCredentials extends AbstractCredentials { } } + + +export class RetriableAzureCredential extends DefaultAzureCredential { + private static DefaultRetryCount = 10; + private static DefaultRetryInterval = 1000; + private static DefaultMaxRetryInterval = 64 * 1000; + private static DefaultRequestTimeout = 5 * 1000; + + private options:ExponentialRetryPolicyOptions = { + maxRetries: RetriableAzureCredential.DefaultRetryCount, + retryDelayInMs: RetriableAzureCredential.DefaultRetryInterval, // Not supported yet + maxRetryDelayInMs: RetriableAzureCredential.DefaultMaxRetryInterval // Not supported yet + }; + + private credentials: DefaultAzureCredential; + + private defaultRequestOptions = { + requestOptions: { + timeout: RetriableAzureCredential.DefaultRequestTimeout + } + }; + + public constructor(tokenCredentialOptions?: DefaultAzureCredentialOptions) { + super(tokenCredentialOptions); + const retryOptions = tokenCredentialOptions?.retryOptions; + this.options = {...this.options, ...retryOptions} + this.credentials = new DefaultAzureCredential(); + } + + public getToken(scopes: string | string[], options?: GetTokenOptions): Promise { + const requestOptions = {...options, ...this.defaultRequestOptions}; + return this.retry(() => this.credentials.getToken(scopes, requestOptions)); + } + + private async retry (fn: () => Promise, retries: number = this.options.maxRetries): Promise { + if(retries <= 0) { + return Promise.reject("Failed after several attememts"); + } + + const prom = fn(); + return prom + .then(res => prom) + .catch(err => this.retry(fn, retries - 1)) + } +} \ No newline at end of file -- GitLab From be763f91a398e2a3f0919acfcba83d997b60e73b Mon Sep 17 00:00:00 2001 From: Dmitriy Rudko Date: Mon, 13 Dec 2021 17:47:01 -0800 Subject: [PATCH 2/6] fix: [BUG 65] - Added credentials to the readiness checks for the SDMS pod --- .../templates/azure-istio-auth-policy.yaml | 14 ++++++++---- devops/azure/chart/templates/deployment.yaml | 2 +- src/server/server.ts | 5 ++++- src/services/general/handler.ts | 22 +++++++++++++++---- src/services/general/optype.ts | 2 +- src/services/general/service.ts | 5 +++++ src/shared/error.ts | 3 ++- 7 files changed, 41 insertions(+), 12 deletions(-) diff --git a/devops/azure/chart/templates/azure-istio-auth-policy.yaml b/devops/azure/chart/templates/azure-istio-auth-policy.yaml index 306e8b2e..bb38871d 100644 --- a/devops/azure/chart/templates/azure-istio-auth-policy.yaml +++ b/devops/azure/chart/templates/azure-istio-auth-policy.yaml @@ -14,8 +14,14 @@ spec: notRequestPrincipals: ["*"] to: - operation: - notPaths: ["/","*/index.html", + notPaths: ["/", + "*/index.html", "*/v2/api-docs", - "*/actuator/health", "*/health", - "*/configuration/ui","*/configuration/security", - "/seistore-svc/api/v3/swagger-ui.html*"] + "*/actuator/health", + "*/health", + "*/configuration/ui", + "*/configuration/security", + "/seistore-svc/api/v3/swagger-ui.html*", + "*/svcstatus", + "*/svcstatus/readiness" + ] diff --git a/devops/azure/chart/templates/deployment.yaml b/devops/azure/chart/templates/deployment.yaml index 9a5ac488..a7c1444d 100644 --- a/devops/azure/chart/templates/deployment.yaml +++ b/devops/azure/chart/templates/deployment.yaml @@ -35,7 +35,7 @@ spec: periodSeconds: 60 readinessProbe: httpGet: - path: /seistore-svc/api/v3/svcstatus + path: /seistore-svc/api/v3/svcstatus/readiness port: 80 httpHeaders: - name: X-Api-Key diff --git a/src/server/server.ts b/src/server/server.ts index acf1ae58..35c067fe 100644 --- a/src/server/server.ts +++ b/src/server/server.ts @@ -115,7 +115,10 @@ export class Server { // the imptoken endpoints have been marked as obsoleted and will be deprecated with the // next service upgrade (v3>v4) if (!req.headers.authorization) { - if(!((req.method === 'PUT' && req.url.endsWith('imptoken')) || req.url.endsWith('svcstatus'))) { + const imptokenCall = (req.method === 'PUT' && req.url.endsWith('imptoken')); + const statusCall = req.url.endsWith('svcstatus'); + const readinessCall = req.url.endsWith('readiness'); + if(!(imptokenCall || statusCall || readinessCall)) { Response.writeError(res, Error.make( Error.Status.UNAUTHENTICATED, 'Unauthenticated Access. Authorizations not found in the request.')); diff --git a/src/services/general/handler.ts b/src/services/general/handler.ts index daf80059..170f05e8 100644 --- a/src/services/general/handler.ts +++ b/src/services/general/handler.ts @@ -17,22 +17,36 @@ import { Request as expRequest, Response as expResponse } from 'express'; import { Error, Response } from '../../shared'; import { GeneralOP } from './optype'; +import { Config } from '../../cloud'; +import { AzureCredentials } from '../../cloud/providers/azure/credentials'; export class GeneralHandler { // handler for the [ /svcstatus ] endpoints public static async handler(req: expRequest, res: expResponse, op: GeneralOP) { - try { - if (op === GeneralOP.Status) { Response.writeOK(res, 'service OK'); } else if (op === GeneralOP.Access) { Response.writeOK(res, { status: 'running' }); - } else { throw (Error.make(Error.Status.UNKNOWN, 'Internal Server Error')); } - + } else if (op === GeneralOP.Readiness) { + this.handleReadinessCheck(req, res); + } else { + throw (Error.make(Error.Status.UNKNOWN, 'Internal Server Error')); + } } catch (error) { Response.writeError(res, error); } + } + private static async handleReadinessCheck(req: expRequest, res: expResponse){ + if(Config.CLOUDPROVIDER === 'azure') { + const credentials = AzureCredentials.getCredential() + credentials.getToken('.default') + .then(token => Response.writeOK(res, {ready: true})) + .catch(err => Response.writeError(res, this.serviceUnavailableError({ready: false}))) + } } + private static serviceUnavailableError(msg: any): Error { + return Error.make(Error.Status.NOT_AVAILABLE, String(msg)) + } } diff --git a/src/services/general/optype.ts b/src/services/general/optype.ts index 596519fe..359cb1f5 100644 --- a/src/services/general/optype.ts +++ b/src/services/general/optype.ts @@ -14,4 +14,4 @@ // limitations under the License. // ============================================================================ -export enum GeneralOP { Status, Access } +export enum GeneralOP { Status, Access, Readiness } diff --git a/src/services/general/service.ts b/src/services/general/service.ts index 93644aaa..25353210 100644 --- a/src/services/general/service.ts +++ b/src/services/general/service.ts @@ -30,4 +30,9 @@ router.get('/access', async (req: expRequest, res: expResponse) => { await GeneralHandler.handler(req, res, GeneralOP.Access); }); +// get the service readiness status response [jwt not required by esp] +router.get('/readiness', async (req: expRequest, res: expResponse) => { + await GeneralHandler.handler(req, res, GeneralOP.Readiness); +}); + export { router as GeneralRouter }; diff --git a/src/shared/error.ts b/src/shared/error.ts index 03ced658..a62b312f 100644 --- a/src/shared/error.ts +++ b/src/shared/error.ts @@ -34,7 +34,8 @@ export class Error { PERMISSION_DENIED: 403, UNAUTHENTICATED: 401, UNKNOWN: 500, - NOT_IMPLEMENTED: 501 + NOT_IMPLEMENTED: 501, + NOT_AVAILABLE: 503 }; -- GitLab From 6190aa450735410d9d4534234de0c0ebbaa852b6 Mon Sep 17 00:00:00 2001 From: Dmitriy Rudko Date: Wed, 15 Dec 2021 09:18:17 -0800 Subject: [PATCH 3/6] fix: [BUG 65] - fix MR comments --- src/cloud/providers/azure/credentials.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/cloud/providers/azure/credentials.ts b/src/cloud/providers/azure/credentials.ts index bf3d77af..d422a7e0 100644 --- a/src/cloud/providers/azure/credentials.ts +++ b/src/cloud/providers/azure/credentials.ts @@ -70,7 +70,7 @@ export class AzureCredentials extends AbstractCredentials { // - AZURE_CLIENT_SECRET: The client secret for the registered application // https://docs.microsoft.com/en-us/azure/active-directory/managed-identities-azure-resources/overview - return new RetriableAzureCredential();; + return new RetriableAzureCredential(); } @@ -231,7 +231,7 @@ export class RetriableAzureCredential extends DefaultAzureCredential { private async retry (fn: () => Promise, retries: number = this.options.maxRetries): Promise { if(retries <= 0) { - return Promise.reject("Failed after several attememts"); + return Promise.reject("Failed after several attempts"); } const prom = fn(); -- GitLab From 9c13ee4a67075137bde7f957b765d5e1bb92b9db Mon Sep 17 00:00:00 2001 From: Dmitriy Rudko Date: Wed, 15 Dec 2021 14:21:21 -0800 Subject: [PATCH 4/6] fix: [BUG 65] - fix linter errors --- src/cloud/providers/azure/credentials.ts | 10 +++++----- src/services/general/handler.ts | 6 +++--- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/cloud/providers/azure/credentials.ts b/src/cloud/providers/azure/credentials.ts index d422a7e0..de6c1807 100644 --- a/src/cloud/providers/azure/credentials.ts +++ b/src/cloud/providers/azure/credentials.ts @@ -25,7 +25,7 @@ import { } from '@azure/storage-blob'; import { DefaultAzureCredential, TokenCredential, DefaultAzureCredentialOptions} from '@azure/identity'; import {ExponentialRetryPolicyOptions} from '@azure/core-rest-pipeline' -import { AccessToken, GetTokenOptions } from "@azure/core-auth"; +import { AccessToken, GetTokenOptions } from '@azure/core-auth'; import request from 'request-promise'; import { AzureDataEcosystemServices } from './dataecosystem'; @@ -201,7 +201,7 @@ export class RetriableAzureCredential extends DefaultAzureCredential { private static DefaultRetryCount = 10; private static DefaultRetryInterval = 1000; private static DefaultMaxRetryInterval = 64 * 1000; - private static DefaultRequestTimeout = 5 * 1000; + private static DefaultRequestTimeout = 5 * 1000; private options:ExponentialRetryPolicyOptions = { maxRetries: RetriableAzureCredential.DefaultRetryCount, @@ -210,7 +210,7 @@ export class RetriableAzureCredential extends DefaultAzureCredential { }; private credentials: DefaultAzureCredential; - + private defaultRequestOptions = { requestOptions: { timeout: RetriableAzureCredential.DefaultRequestTimeout @@ -231,9 +231,9 @@ export class RetriableAzureCredential extends DefaultAzureCredential { private async retry (fn: () => Promise, retries: number = this.options.maxRetries): Promise { if(retries <= 0) { - return Promise.reject("Failed after several attempts"); + return Promise.reject('Failed after several attempts'); } - + const prom = fn(); return prom .then(res => prom) diff --git a/src/services/general/handler.ts b/src/services/general/handler.ts index 170f05e8..8075ee41 100644 --- a/src/services/general/handler.ts +++ b/src/services/general/handler.ts @@ -30,9 +30,9 @@ export class GeneralHandler { } else if (op === GeneralOP.Access) { Response.writeOK(res, { status: 'running' }); } else if (op === GeneralOP.Readiness) { - this.handleReadinessCheck(req, res); - } else { - throw (Error.make(Error.Status.UNKNOWN, 'Internal Server Error')); + await this.handleReadinessCheck(req, res); + } else { + throw (Error.make(Error.Status.UNKNOWN, 'Internal Server Error')); } } catch (error) { Response.writeError(res, error); } } -- GitLab From 93a946cb8f3f2ea4fe276e38cf38dfc6368ee3ef Mon Sep 17 00:00:00 2001 From: Dmitriy Rudko Date: Sun, 19 Dec 2021 16:01:10 -0800 Subject: [PATCH 5/6] fix: [BUG 65] - Fix scope of token for MSI readiness check --- devops/azure/chart/templates/azure-istio-auth-policy.yaml | 8 +------- src/cloud/providers/azure/keyvault.ts | 2 +- src/services/general/handler.ts | 6 ++++-- 3 files changed, 6 insertions(+), 10 deletions(-) diff --git a/devops/azure/chart/templates/azure-istio-auth-policy.yaml b/devops/azure/chart/templates/azure-istio-auth-policy.yaml index bb38871d..8acbd7be 100644 --- a/devops/azure/chart/templates/azure-istio-auth-policy.yaml +++ b/devops/azure/chart/templates/azure-istio-auth-policy.yaml @@ -15,13 +15,7 @@ spec: to: - operation: notPaths: ["/", - "*/index.html", - "*/v2/api-docs", - "*/actuator/health", - "*/health", - "*/configuration/ui", - "*/configuration/security", - "/seistore-svc/api/v3/swagger-ui.html*", + "*/swagger-ui.html*", "*/svcstatus", "*/svcstatus/readiness" ] diff --git a/src/cloud/providers/azure/keyvault.ts b/src/cloud/providers/azure/keyvault.ts index d9519b5d..d844fdb9 100644 --- a/src/cloud/providers/azure/keyvault.ts +++ b/src/cloud/providers/azure/keyvault.ts @@ -74,7 +74,7 @@ export class Keyvault { AzureConfig.SP_CLIENT_SECRET = AzureConfig.SP_CLIENT_SECRET = (await client.getSecret(this.SP_CLIENT_SECRET)).value; AzureConfig.SP_APP_RESOURCE_ID = - AzureConfig.SP_APP_RESOURCE_ID = (await client.getSecret(this.SP_APP_RESOURCE_ID)).value;; + AzureConfig.SP_APP_RESOURCE_ID = (await client.getSecret(this.SP_APP_RESOURCE_ID)).value; } } \ No newline at end of file diff --git a/src/services/general/handler.ts b/src/services/general/handler.ts index 8075ee41..b0943ddb 100644 --- a/src/services/general/handler.ts +++ b/src/services/general/handler.ts @@ -19,6 +19,7 @@ import { Error, Response } from '../../shared'; import { GeneralOP } from './optype'; import { Config } from '../../cloud'; import { AzureCredentials } from '../../cloud/providers/azure/credentials'; +import { AzureConfig } from '../../cloud/providers/azure'; export class GeneralHandler { @@ -39,8 +40,9 @@ export class GeneralHandler { private static async handleReadinessCheck(req: expRequest, res: expResponse){ if(Config.CLOUDPROVIDER === 'azure') { - const credentials = AzureCredentials.getCredential() - credentials.getToken('.default') + const credentials = AzureCredentials.getCredential(); + const scope = AzureConfig.SP_APP_RESOURCE_ID; + credentials.getToken(`${scope}/.default`) .then(token => Response.writeOK(res, {ready: true})) .catch(err => Response.writeError(res, this.serviceUnavailableError({ready: false}))) } -- GitLab From a90aed0eaaf7834942aa2237ad9a69e83106ff62 Mon Sep 17 00:00:00 2001 From: Dmitriy Rudko Date: Sun, 19 Dec 2021 16:06:41 -0800 Subject: [PATCH 6/6] fix: [BUG 65] - Fix NOTICE file check --- NOTICE | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/NOTICE b/NOTICE index 923eb62c..f5367687 100644 --- a/NOTICE +++ b/NOTICE @@ -7,7 +7,7 @@ AFL-3.0 ======================================================================== The following software have components provided under the terms of this license: -- json-schema (from https://www.npmjs.com/package/json-schema) +- json-schema (from ) ======================================================================== Apache-2.0 @@ -137,7 +137,7 @@ The following software have components provided under the terms of this license: - rc (from https://www.npmjs.com/package/rc) - shimmer (from https://www.npmjs.com/package/shimmer) - source-map (from https://www.npmjs.com/package/source-map) -- sprintf-js (from https://www.npmjs.com/package/sprintf-js) +- sprintf-js (from ) - tough-cookie (from https://www.npmjs.com/package/tough-cookie) - tough-cookie (from https://www.npmjs.com/package/tough-cookie) - tslib (from https://www.npmjs.com/package/tslib) @@ -174,7 +174,7 @@ The following software have components provided under the terms of this license: - google-gax (from https://www.npmjs.com/package/google-gax) - idna (from https://github.com/kjd/idna) - ieee754 (from https://www.npmjs.com/package/ieee754) -- json-schema (from https://www.npmjs.com/package/json-schema) +- json-schema (from ) - node-forge (from https://www.npmjs.com/package/node-forge) - node-forge (from https://www.npmjs.com/package/node-forge) - node-pre-gyp (from https://www.npmjs.com/package/node-pre-gyp) @@ -187,7 +187,7 @@ The following software have components provided under the terms of this license: - qs (from https://www.npmjs.com/package/qs) - rc (from https://www.npmjs.com/package/rc) - source-map (from https://www.npmjs.com/package/source-map) -- sprintf-js (from https://www.npmjs.com/package/sprintf-js) +- sprintf-js (from ) - stack-chain (from https://www.npmjs.com/package/stack-chain) - tough-cookie (from https://www.npmjs.com/package/tough-cookie) - tough-cookie (from https://www.npmjs.com/package/tough-cookie) @@ -306,7 +306,7 @@ The following software have components provided under the terms of this license: - which (from https://www.npmjs.com/package/which) - which-module (from https://www.npmjs.com/package/which-module) - wide-align (from https://www.npmjs.com/package/wide-align) -- wrappy (from https://github.com/npm/wrappy) +- wrappy (from https://www.npmjs.com/package/wrappy) - write-file-atomic (from https://www.npmjs.com/package/write-file-atomic) - y18n (from https://www.npmjs.com/package/y18n) - y18n (from https://www.npmjs.com/package/y18n) @@ -322,7 +322,7 @@ LGPL-2.1-only The following software have components provided under the terms of this license: - chardet (from https://github.com/chardet/chardet) -- xmlrunner (from https://github.com/pycontribs/xmlrunner) +- xmlrunner (from ) ======================================================================== LGPL-2.1-or-later @@ -433,7 +433,7 @@ The following software have components provided under the terms of this license: - call-bind (from https://www.npmjs.com/package/call-bind) - camelcase (from https://www.npmjs.com/package/camelcase) - camelize (from https://www.npmjs.com/package/camelize) -- cffi (from http://cffi.readthedocs.org) +- cffi (from ) - chalk (from https://www.npmjs.com/package/chalk) - chownr (from https://www.npmjs.com/package/chownr) - cliui (from https://www.npmjs.com/package/cliui) @@ -479,7 +479,7 @@ The following software have components provided under the terms of this license: - debuglog (from https://www.npmjs.com/package/debuglog) - decamelize (from https://www.npmjs.com/package/decamelize) - decompress-response (from https://www.npmjs.com/package/decompress-response) -- deep-extend (from https://www.npmjs.com/package/deep-extend) +- deep-extend (from ) - define-properties (from https://www.npmjs.com/package/define-properties) - delayed-stream (from https://www.npmjs.com/package/delayed-stream) - delegates (from https://www.npmjs.com/package/delegates) @@ -522,7 +522,7 @@ The following software have components provided under the terms of this license: - fn.name (from https://www.npmjs.com/package/fn.name) - follow-redirects (from https://www.npmjs.com/package/follow-redirects) - follow-redirects (from https://www.npmjs.com/package/follow-redirects) -- for-each (from https://www.npmjs.com/package/for-each) +- for-each (from ) - form-data (from https://www.npmjs.com/package/form-data) - form-data (from https://www.npmjs.com/package/form-data) - form-data (from https://www.npmjs.com/package/form-data) @@ -600,12 +600,12 @@ The following software have components provided under the terms of this license: - isarray (from https://www.npmjs.com/package/isarray) - isexe (from https://www.npmjs.com/package/isexe) - isstream (from https://github.com/rvagg/isstream) -- jmespath (from https://www.npmjs.com/package/jmespath) - jmespath (from https://github.com/jmespath/jmespath.py) +- jmespath (from https://www.npmjs.com/package/jmespath) - jsbn (from https://www.npmjs.com/package/jsbn) - json-bigint (from https://www.npmjs.com/package/json-bigint) - json-bigint (from https://www.npmjs.com/package/json-bigint) -- json-schema (from https://www.npmjs.com/package/json-schema) +- json-schema (from ) - json-schema-traverse (from https://www.npmjs.com/package/json-schema-traverse) - json-stringify-safe (from https://www.npmjs.com/package/json-stringify-safe) - jsonfile (from https://www.npmjs.com/package/jsonfile) @@ -855,7 +855,7 @@ The following software have components provided under the terms of this license: - winston-transport (from https://www.npmjs.com/package/winston-transport) - wrap-ansi (from https://www.npmjs.com/package/wrap-ansi) - wrap-ansi (from https://www.npmjs.com/package/wrap-ansi) -- wrappy (from https://github.com/npm/wrappy) +- wrappy (from https://www.npmjs.com/package/wrappy) - write-file-atomic (from https://www.npmjs.com/package/write-file-atomic) - xdg-basedir (from https://www.npmjs.com/package/xdg-basedir) - xml2js (from https://www.npmjs.com/package/xml2js) -- GitLab