diff --git a/frontend/server/app.ts b/frontend/server/app.ts index f6ae1988bf4a..50d8565772c8 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 7108db23df9c..4188338498f5 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 d5bea2cbf6ae..2f04e232b66d 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 d104e1a8e635..bbe37f849be2 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 5f244800f2a9..9295f6cbcf63 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 8e8923a3659c..adfcfc9f9287 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: