Merge pull request #1529 from DaoDaoNoCode/upstream-issue-1433

Update modals and generate meaningful k8s name for images

backend/.eslintignore

@@ -1 +1,2 @@
 package.json
+jest.config.js

backend/jest.config.js

@@ -0,0 +1,7 @@
+module.exports = {
+  transform: {
+    'node_modules': 'ts-jest',
+  },
+  preset: 'ts-jest',
+  testEnvironment: 'node',
+};

backend/package.json

@@ -23,14 +23,16 @@
     "start:dev": "npm run clean && export NODE_TLS_REJECT_UNAUTHORIZED=0 && export NODE_ENV=development && nodemon src/server.ts --log=1 --registry=localhost:50051",
     "debug": "npm run tsc && export NODE_TLS_REJECT_UNAUTHORIZED=0 && export NODE_ENV=development && node --inspect ./dist/server.js --log=1 --registry=localhost:50051",
     "build-only": "tsc -p . && node ./dist/server.js --log=1 --registry=localhost:50051 --buildonly",
-    "build": "npm run build:clean; npm run tsc",
+    "build": "npm run build:clean; npm run tsc:prod",
     "build:clean": "rimraf ./dist",
-    "test": "npm run test:lint; npm run test:type-check",
+    "test": "npm run test:lint; npm run test:type-check; npm run test:jest",
     "test:lint": "eslint --max-warnings 0 --ext .json,.js,.ts src/plugins src/routes src/utils",
     "test:fix": "eslint --ext .json,.js,.ts src/plugins src/routes src/utils --fix",
     "test:type-check": "tsc --noEmit",
+    "test:jest": "jest",
     "server": "NODE_ENV=production node ./dist/server.js",
     "tsc": "tsc -p .",
+    "tsc:prod": "tsc -p tsconfig.prod.json",
     "lint": "eslint ./src/",
     "watch": "tsc -p . -w"
   },
@@ -66,13 +68,16 @@
     "typescript": "^4.0.3"
   },
   "optionalDependencies": {
+    "@types/jest": "^29.5.3",
     "eslint": "^6.8.0",
     "eslint-config-esnext": "^4.1.0",
     "eslint-config-node": "^4.1.0",
     "eslint-config-prettier": "^6.15.0",
     "eslint-plugin-babel": "^5.3.1",
     "eslint-plugin-import": "^2.22.1",
     "eslint-plugin-node": "^11.1.0",
-    "eslint-plugin-prettier": "^3.4.0"
+    "eslint-plugin-prettier": "^3.4.0",
+    "jest": "^29.6.1",
+    "ts-jest": "^29.1.1"
   }
 }

backend/src/__tests__/dockerRepositoryURL.spec.ts

@@ -0,0 +1,77 @@
+// https://cloud.google.com/artifact-registry/docs/docker/names
+// The full name for a container image is one of the following formats:
+// LOCATION-docker.pkg.dev/PROJECT-ID/REPOSITORY/IMAGE
+// LOCATION-docker.pkg.dev/PROJECT-ID/REPOSITORY/IMAGE:TAG
+// LOCATION-docker.pkg.dev/PROJECT-ID/REPOSITORY/IMAGE@IMAGE-DIGEST
+
+import { parseImageURL } from '../routes/api/images/imageUtils';
+
+test('Invalid URL: space string', () => {
+  const url = '     ';
+  const { fullURL, host } = parseImageURL(url);
+  expect(fullURL).toBe('');
+  expect(host).toBeUndefined();
+});
+
+test('Invalid URL: no match', () => {
+  const url = '/';
+  const { host, tag } = parseImageURL(url);
+  expect(host).toBeUndefined();
+  expect(tag).toBeUndefined();
+});
+
+test('Invalid URL: host only', () => {
+  const url = 'docker.io';
+  const { host } = parseImageURL(url);
+  expect(host).toBe('');
+});
+
+test('Invalid URL: host and repo, no image', () => {
+  const url = 'docker.io/opendatahub';
+  const { host } = parseImageURL(url);
+  expect(host).toBe('');
+});
+
+test('Valid URL with spaces on both sides', () => {
+  const url = '  docker.io/library/mysql:test  ';
+  const { fullURL, host, tag } = parseImageURL(url);
+  expect(fullURL).toBe('docker.io/library/mysql:test');
+  expect(host).toBe('docker.io');
+  expect(tag).toBe('test');
+});
+
+test('Docker container URL without tag', () => {
+  const url = 'docker.io/library/mysql';
+  const { host, tag } = parseImageURL(url);
+  expect(host).toBe('docker.io');
+  expect(tag).toBeUndefined();
+});
+
+test('Docker container URL with tag', () => {
+  const url = 'docker.io/library/mysql:test-tag';
+  const { host, tag } = parseImageURL(url);
+  expect(host).toBe('docker.io');
+  expect(tag).toBe('test-tag');
+});
+
+test('OpenShift internal registry URL without tag', () => {
+  const url = 'image-registry.openshift-image-registry.svc:5000/opendatahub/s2i-minimal-notebook';
+  const { host, tag } = parseImageURL(url);
+  expect(host).toBe('image-registry.openshift-image-registry.svc:5000');
+  expect(tag).toBeUndefined();
+});
+
+test('OpenShift internal registry URL with tag', () => {
+  const url =
+    'image-registry.openshift-image-registry.svc:5000/opendatahub/s2i-minimal-notebook:v0.3.0-py36';
+  const { host, tag } = parseImageURL(url);
+  expect(host).toBe('image-registry.openshift-image-registry.svc:5000');
+  expect(tag).toBe('v0.3.0-py36');
+});
+
+test('Quay URL with port and tag', () => {
+  const url = 'quay.io:443/opendatahub/odh-dashboard:main-55e19fa';
+  const { host, tag } = parseImageURL(url);
+  expect(host).toBe('quay.io:443');
+  expect(tag).toBe('main-55e19fa');
+});

backend/src/routes/api/images/imageUtils.ts

@@ -1,4 +1,4 @@
-import { IMAGE_ANNOTATIONS, imageUrlRegex } from '../../../utils/constants';
+import { IMAGE_ANNOTATIONS } from '../../../utils/constants';
 import { convertLabelsToString } from '../../../utils/componentUtils';
 import {
   ImageStreamTag,
@@ -13,6 +13,42 @@ import {
 import { FastifyRequest } from 'fastify';
 import createError from 'http-errors';
 
+const translateDisplayNameForK8s = (name: string): string =>
+  name
+    .trim()
+    .toLowerCase()
+    .replace(/\s/g, '-')
+    .replace(/[^A-Za-z0-9-]/g, '');
+
+/**
+ * This function uses a regex to match the image location string
+ * The match result will return an array of 4 elements:
+ * Full URL, host, repo/image and tag(if any)
+ * @param imageString
+ */
+export const parseImageURL = (
+  imageString: string,
+): { fullURL: string; host: string; image: string; tag: string } => {
+  const imageUrlRegex =
+    /^([\w.\-_]+(?::\d+|)(?=\/[a-z0-9._-]+\/[a-z0-9._-]+)|)(?:\/|)([a-z0-9.\-_]+(?:\/[a-z0-9.\-_]+|))(?::([\w.\-_]{1,127})|)/;
+  const trimmedString = imageString.trim();
+  const result = trimmedString.match(imageUrlRegex);
+  if (!result) {
+    return {
+      fullURL: trimmedString,
+      host: undefined,
+      image: undefined,
+      tag: undefined,
+    };
+  }
+  return {
+    fullURL: result[0],
+    host: result[1],
+    image: result[2],
+    tag: result[3],
+  };
+};
+
 export const getImageList = async (
   fastify: KubeFastifyInstance,
   labels: { [key: string]: string },
@@ -189,8 +225,14 @@ const packagesToString = (packages: BYONImagePackage[]): string => {
 const mapImageStreamToBYONImage = (is: ImageStream): BYONImage => ({
   id: is.metadata.uid,
   name: is.metadata.name,
-  display_name: is.metadata.annotations['opendatahub.io/notebook-image-name'] || is.metadata.name,
-  description: is.metadata.annotations['opendatahub.io/notebook-image-desc'] || '',
+  display_name:
+    is.metadata.annotations['opendatahub.io/notebook-image-name'] ||
+    is.metadata.annotations['openshift.io/display-name'] ||
+    is.metadata.name,
+  description:
+    is.metadata.annotations['opendatahub.io/notebook-image-desc'] ||
+    is.metadata.annotations['openshift.io/description'] ||
+    '',
   visible: is.metadata.labels['opendatahub.io/notebook-image'] === 'true',
   error: getBYONImageErrorMessage(is),
   packages: JSON.parse(
@@ -209,14 +251,12 @@ export const postImage = async (
   const customObjectsApi = fastify.kube.customObjectsApi;
   const namespace = fastify.kube.namespace;
   const body = request.body as BYONImage;
-  const fullUrl = body.url;
-  const matchArray = fullUrl.match(imageUrlRegex);
-  // check if the host is valid
-  if (!matchArray[1]) {
+  const inputURL = body.url;
+  const { fullURL, host, tag } = parseImageURL(inputURL);
+  if (!host) {
     fastify.log.error('Invalid repository URL unable to add notebook image');
-    return { success: false, error: 'Invalid repository URL: ' + fullUrl };
+    return { success: false, error: 'Invalid repository URL: ' + fullURL };
   }
-  const imageTag = matchArray[4];
   const labels = {
     'app.kubernetes.io/created-by': 'byon',
     'opendatahub.io/notebook-image': 'true',
@@ -227,7 +267,10 @@ export const postImage = async (
 
   if (validName.length > 0) {
     fastify.log.error('Duplicate name unable to add notebook image');
-    return { success: false, error: 'Unable to add notebook image: ' + body.display_name };
+    return {
+      success: false,
+      error: 'Duplicated name. Unable to add notebook image: ' + body.display_name,
+    };
   }
 
   const payload: ImageStream = {
@@ -237,10 +280,10 @@ export const postImage = async (
       annotations: {
         'opendatahub.io/notebook-image-desc': body.description || '',
         'opendatahub.io/notebook-image-name': body.display_name,
-        'opendatahub.io/notebook-image-url': fullUrl,
+        'opendatahub.io/notebook-image-url': fullURL,
         'opendatahub.io/notebook-image-creator': body.provider,
       },
-      name: `byon-${Date.now()}`,
+      name: `custom-${translateDisplayNameForK8s(body.display_name)}`,
       namespace: namespace,
       labels: labels,
     },
@@ -253,13 +296,13 @@ export const postImage = async (
           annotations: {
             'opendatahub.io/notebook-software': packagesToString(body.software),
             'opendatahub.io/notebook-python-dependencies': packagesToString(body.packages),
-            'openshift.io/imported-from': fullUrl,
+            'openshift.io/imported-from': fullURL,
           },
           from: {
             kind: 'DockerImage',
-            name: fullUrl,
+            name: fullURL,
           },
-          name: imageTag || 'latest',
+          name: tag || 'latest',
         },
       ],
     },

backend/src/types.ts

@@ -473,6 +473,7 @@ export type ODHSegmentKey = {
 
 export type BYONImage = {
   id: string;
+  // FIXME: This shouldn't be a user defined value consumed from the request payload but should be a controlled value from an authentication middleware.
   provider: string;
   imported_time: string;
   error: string;

backend/src/utils/constants.ts

@@ -131,6 +131,3 @@ export const DEFAULT_NOTEBOOK_SIZES: NotebookSize[] = [
     },
   },
 ];
-
-export const imageUrlRegex =
-  /^([\w.\-_]+((?::\d+|)(?=\/[a-z0-9._-]+\/[a-z0-9._-]+))|)(?:\/|)([a-z0-9.\-_]+(?:\/[a-z0-9.\-_]+|))(?::([\w.\-_]{1,127})|)/;

backend/tsconfig.prod.json

@@ -0,0 +1,6 @@
+{
+  "extends": "./tsconfig",
+  "exclude": [
+    "src/__tests__/"
+  ]
+}

frontend/src/concepts/dashboard/DashboardModalFooter.tsx

@@ -1,3 +1,4 @@
+import * as React from 'react';
 import {
   ActionList,
   ActionListItem,
@@ -6,13 +7,13 @@ import {
   Stack,
   StackItem,
 } from '@patternfly/react-core';
-import * as React from 'react';
 
 type DashboardModalFooterProps = {
   submitLabel: string;
   onSubmit: () => void;
   onCancel: () => void;
   isSubmitDisabled: boolean;
+  isCancelDisabled?: boolean;
   alertTitle: string;
   error?: Error;
 };
@@ -22,6 +23,7 @@ const DashboardModalFooter: React.FC<DashboardModalFooterProps> = ({
   onSubmit,
   onCancel,
   isSubmitDisabled,
+  isCancelDisabled,
   error,
   alertTitle,
 }) => (
@@ -42,7 +44,7 @@ const DashboardModalFooter: React.FC<DashboardModalFooterProps> = ({
           </Button>
         </ActionListItem>
         <ActionListItem>
-          <Button key="cancel" variant="link" onClick={onCancel}>
+          <Button key="cancel" variant="link" isDisabled={isCancelDisabled} onClick={onCancel}>
             Cancel
           </Button>
         </ActionListItem>