From 2c91fb797ed5e95bb51ae80c4daa2c6b9334b51b Mon Sep 17 00:00:00 2001 From: Humair Khan Date: Tue, 3 Sep 2024 19:24:14 -0400 Subject: [PATCH] correct artifact preview behavior in UI (#11059) This change allows KFP UI to fallback to UI host namespace when no namespaces are provided when referencing the artifact object store provider secret, in default kubeflow deployments this namespace is simply "kubeflow", however the user can customize this behavior by providing the environment variable "SERVER_NAMESPACE" to the KFP UI deployment. Further more, this change addresses a bug that caused URL parse to fail when parsing endpoints without a protocol, this will support such endpoint types as : for object store endpoints, as is the case in the default kfp deployment manifests. Signed-off-by: Humair Khan --- frontend/server/app.ts | 2 + frontend/server/configs.ts | 4 + frontend/server/handlers/artifacts.ts | 12 ++- .../integration-tests/artifact-get.test.ts | 95 +++++++++++++++++-- frontend/server/minio-helper.ts | 8 +- .../pipeline/ml-pipeline-ui-deployment.yaml | 4 + 6 files changed, 109 insertions(+), 16 deletions(-) diff --git a/frontend/server/app.ts b/frontend/server/app.ts index f6ae1988bf4..50d8565772c 100644 --- a/frontend/server/app.ts +++ b/frontend/server/app.ts @@ -143,6 +143,7 @@ function createUIServer(options: UIConfigs) { artifactsConfigs: options.artifacts, useParameter: false, tryExtract: true, + options: options, }), ); // /artifacts/ endpoint downloads the artifact as is, it does not try to unzip or untar. @@ -158,6 +159,7 @@ function createUIServer(options: UIConfigs) { artifactsConfigs: options.artifacts, useParameter: true, tryExtract: false, + options: options, }), ); diff --git a/frontend/server/configs.ts b/frontend/server/configs.ts index 7108db23df9..4188338498f 100644 --- a/frontend/server/configs.ts +++ b/frontend/server/configs.ts @@ -123,6 +123,7 @@ export function loadConfigs(argv: string[], env: ProcessEnv): UIConfigs { * e.g. a valid header value for default values can be like `accounts.google.com:user@gmail.com`. */ KUBEFLOW_USERID_PREFIX = 'accounts.google.com:', + FRONTEND_SERVER_NAMESPACE = 'kubeflow', } = env; return { @@ -190,6 +191,7 @@ export function loadConfigs(argv: string[], env: ProcessEnv): UIConfigs { : asBool(HIDE_SIDENAV), port, staticDir, + serverNamespace: FRONTEND_SERVER_NAMESPACE, }, viewer: { tensorboard: { @@ -266,6 +268,8 @@ export interface ServerConfigs { apiVersion2Prefix: string; deployment: Deployments; hideSideNav: boolean; + // Namespace where the server is deployed + serverNamespace: string; } export interface GkeMetadataConfigs { disabled: boolean; diff --git a/frontend/server/handlers/artifacts.ts b/frontend/server/handlers/artifacts.ts index d5bea2cbf6a..2f04e232b66 100644 --- a/frontend/server/handlers/artifacts.ts +++ b/frontend/server/handlers/artifacts.ts @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. import fetch from 'node-fetch'; -import { AWSConfigs, HttpConfigs, MinioConfigs, ProcessEnv } from '../configs'; +import { AWSConfigs, HttpConfigs, MinioConfigs, ProcessEnv, UIConfigs } from '../configs'; import { Client as MinioClient } from 'minio'; import { PreviewStream, findFileOnPodVolume, parseJSONString } from '../utils'; import { createMinioClient, getObjectStream } from '../minio-helper'; @@ -80,6 +80,7 @@ export function getArtifactsHandler({ artifactsConfigs, useParameter, tryExtract, + options, }: { artifactsConfigs: { aws: AWSConfigs; @@ -89,15 +90,18 @@ export function getArtifactsHandler({ }; tryExtract: boolean; useParameter: boolean; + options: UIConfigs; }): Handler { const { aws, http, minio, allowedDomain } = artifactsConfigs; return async (req, res) => { const source = useParameter ? req.params.source : req.query.source; const bucket = useParameter ? req.params.bucket : req.query.bucket; const key = useParameter ? req.params[0] : req.query.key; - const { peek = 0, providerInfo = '', namespace = '' } = req.query as Partial< - ArtifactsQueryStrings - >; + const { + peek = 0, + providerInfo = '', + namespace = options.server.serverNamespace, + } = req.query as Partial; if (!source) { res.status(500).send('Storage source is missing from artifact request'); return; diff --git a/frontend/server/integration-tests/artifact-get.test.ts b/frontend/server/integration-tests/artifact-get.test.ts index d104e1a8e63..bbe37f849be 100644 --- a/frontend/server/integration-tests/artifact-get.test.ts +++ b/frontend/server/integration-tests/artifact-get.test.ts @@ -184,8 +184,11 @@ describe('/artifacts', () => { }); }); - it('responds error when source is s3, and creds are sourced from Provider Configs, but no namespace is provided', done => { + it('responds with artifact if source is AWS S3, and creds are sourced from Provider Configs, and uses default kubeflow namespace when no namespace is provided', done => { const mockedGetK8sSecret: jest.Mock = getK8sSecret as any; + mockedGetK8sSecret.mockResolvedValue('somevalue'); + const mockedMinioClient: jest.Mock = minio.Client as any; + const namespace = 'kubeflow'; const configs = loadConfigs(argv, {}); app = new UIServer(configs); const request = requests(app.start()); @@ -203,18 +206,90 @@ describe('/artifacts', () => { }; request .get( - `/artifacts/get?source=s3&bucket=ml-pipeline&key=hello%2Fworld.txt$&providerInfo=${JSON.stringify( + `/artifacts/get?source=s3&bucket=ml-pipeline&key=hello%2Fworld.txt&providerInfo=${JSON.stringify( + providerInfo, + )}`, + ) + .expect(200, artifactContent, err => { + expect(mockedMinioClient).toBeCalledWith({ + accessKey: 'somevalue', + endPoint: 's3.amazonaws.com', + port: undefined, + region: 'us-east-2', + secretKey: 'somevalue', + useSSL: undefined, + }); + expect(mockedMinioClient).toBeCalledTimes(1); + expect(mockedGetK8sSecret).toBeCalledTimes(2); + expect(mockedGetK8sSecret).toHaveBeenNthCalledWith( + 1, + 'aws-s3-creds', + 'AWS_ACCESS_KEY_ID', + `${namespace}`, + ); + expect(mockedGetK8sSecret).toHaveBeenNthCalledWith( + 2, + 'aws-s3-creds', + 'AWS_SECRET_ACCESS_KEY', + `${namespace}`, + ); + expect(mockedGetK8sSecret).toBeCalledTimes(2); + done(err); + }); + }); + + it('responds with artifact if source is AWS S3, and creds are sourced from Provider Configs, and uses default namespace when no namespace is provided, as specified in ENV', done => { + const mockedGetK8sSecret: jest.Mock = getK8sSecret as any; + mockedGetK8sSecret.mockResolvedValue('somevalue'); + const mockedMinioClient: jest.Mock = minio.Client as any; + const namespace = 'notkubeflow'; + const configs = loadConfigs(argv, { FRONTEND_SERVER_NAMESPACE: namespace }); + app = new UIServer(configs); + const request = requests(app.start()); + const providerInfo = { + Params: { + accessKeyKey: 'AWS_ACCESS_KEY_ID', + disableSSL: 'false', + endpoint: 's3.amazonaws.com', + fromEnv: 'false', + region: 'us-east-2', + secretKeyKey: 'AWS_SECRET_ACCESS_KEY', + secretName: 'aws-s3-creds', + }, + Provider: 's3', + }; + request + .get( + `/artifacts/get?source=s3&bucket=ml-pipeline&key=hello%2Fworld.txt&providerInfo=${JSON.stringify( providerInfo, )}`, ) - .expect( - 500, - 'Failed to initialize Minio Client for S3 Provider: Error: Artifact Store provider given, but no namespace provided.', - err => { - expect(mockedGetK8sSecret).toBeCalledTimes(0); - done(err); - }, - ); + .expect(200, artifactContent, err => { + expect(mockedMinioClient).toBeCalledWith({ + accessKey: 'somevalue', + endPoint: 's3.amazonaws.com', + port: undefined, + region: 'us-east-2', + secretKey: 'somevalue', + useSSL: undefined, + }); + expect(mockedMinioClient).toBeCalledTimes(1); + expect(mockedGetK8sSecret).toBeCalledTimes(2); + expect(mockedGetK8sSecret).toHaveBeenNthCalledWith( + 1, + 'aws-s3-creds', + 'AWS_ACCESS_KEY_ID', + `${namespace}`, + ); + expect(mockedGetK8sSecret).toHaveBeenNthCalledWith( + 2, + 'aws-s3-creds', + 'AWS_SECRET_ACCESS_KEY', + `${namespace}`, + ); + expect(mockedGetK8sSecret).toBeCalledTimes(2); + done(err); + }); }); it('responds with artifact if source is s3-compatible, and creds are sourced from Provider Configs', done => { diff --git a/frontend/server/minio-helper.ts b/frontend/server/minio-helper.ts index 5f244800f2a..9295f6cbcf6 100644 --- a/frontend/server/minio-helper.ts +++ b/frontend/server/minio-helper.ts @@ -78,7 +78,7 @@ export async function createMinioClient( } // If using s3 and sourcing credentials from environment (currently only aws is supported) - if (providerType === 's3' && (!config.accessKey || !config.secretKey)) { + if (providerType === 's3' && !(config.accessKey && config.secretKey)) { // AWS S3 with credentials from provider chain if (isAWSS3Endpoint(config.endPoint)) { try { @@ -168,7 +168,11 @@ async function parseS3ProviderInfo( config.useSSL = undefined; } else { if (providerInfo.Params.endpoint) { - const parseEndpoint = new URL(providerInfo.Params.endpoint); + const url = providerInfo.Params.endpoint; + // this is a bit of a hack to add support for endpoints without a protocol (required by WHATWG URL standard) + // example: : format. In general should expect most endpoints to provide a protocol as serviced + // by the backend + const parseEndpoint = new URL(url.startsWith('http') ? url : `https://${url}`); const host = parseEndpoint.hostname; const port = parseEndpoint.port; config.endPoint = host; diff --git a/manifests/kustomize/base/pipeline/ml-pipeline-ui-deployment.yaml b/manifests/kustomize/base/pipeline/ml-pipeline-ui-deployment.yaml index 8e8923a3659..adfcfc9f928 100644 --- a/manifests/kustomize/base/pipeline/ml-pipeline-ui-deployment.yaml +++ b/manifests/kustomize/base/pipeline/ml-pipeline-ui-deployment.yaml @@ -48,6 +48,10 @@ spec: key: secretkey - name: ALLOW_CUSTOM_VISUALIZATIONS value: "true" + - name: FRONTEND_SERVER_NAMESPACE + valueFrom: + fieldRef: + fieldPath: metadata.namespace readinessProbe: exec: command: