correct artifact preview behavior in UI (kubeflow#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 <ip>:<port> for object store endpoints,
as is the case in the default kfp deployment manifests.

Signed-off-by: Humair Khan <HumairAK@users.noreply.github.com>
Signed-off-by: sefgsefg <sefgsefg@gmail.com>

Author: HumairAK

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,
     }),
   );
 

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;

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<ArtifactsQueryStrings>;
     if (!source) {
       res.status(500).send('Storage source is missing from artifact request');
       return;

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 => {

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: <ip>:<port> 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;

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: