Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BYON Improvements Phase 1 to main #1997

Merged
merged 20 commits into from
Oct 20, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
61558a1
keep the enabled/disabled status after editing the custom notebook im…
uidoyen Jun 28, 2023
264504c
Refactor BYON images table
DaoDaoNoCode Jul 12, 2023
2c849f3
Merge pull request #1506 from DaoDaoNoCode/upstream-issue-1432
openshift-ci[bot] Jul 17, 2023
6f6801c
Merge remote-tracking branch 'opendatahub-io/main' into f/byon
DaoDaoNoCode Jul 28, 2023
3494a78
Update modals and generate meaningful k8s name for images
DaoDaoNoCode Jul 17, 2023
3d68d79
Merge pull request #1529 from DaoDaoNoCode/upstream-issue-1433
openshift-merge-robot Jul 31, 2023
9fef3fb
Merge remote-tracking branch 'opendatahub-io/main' into f/byon
DaoDaoNoCode Aug 1, 2023
15d3b81
Make custom image location regex match return correct full URL
DaoDaoNoCode Aug 7, 2023
937ff2a
Merge branch 'main' into f/byon
lucferbux Aug 10, 2023
1ade815
Fix byon branch type issues
DaoDaoNoCode Aug 10, 2023
74092d3
Merge pull request #1665 from DaoDaoNoCode/fix-type-issue-byon
openshift-merge-robot Aug 10, 2023
5c1811b
Merge remote-tracking branch 'opendatahub-io/main' into f/byon
DaoDaoNoCode Sep 12, 2023
e0138b4
Merge remote-tracking branch 'opendatahub-io/main' into f/byon
DaoDaoNoCode Sep 13, 2023
cfc492e
Minor UX fixes for custom notebook images
DaoDaoNoCode Sep 13, 2023
b204b3c
Merge pull request #1782 from DaoDaoNoCode/upstream-issue-1769
openshift-merge-robot Sep 15, 2023
42659df
Merge remote-tracking branch 'opendatahub-io/main' into f/byon
DaoDaoNoCode Sep 15, 2023
37a58f9
Merge branch 'f/byon' of github.com:opendatahub-io/odh-dashboard into…
DaoDaoNoCode Sep 15, 2023
d28a9d4
added util tests and integration tests for BYON
Gkrumbach07 Oct 18, 2023
1e45941
Merge pull request #1992 from Gkrumbach07/byon-testing
openshift-ci[bot] Oct 19, 2023
5ef9bee
Merge branch 'main' into f/byon
andrewballantyne Oct 20, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions backend/.eslintignore
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
package.json
jest.config.js
7 changes: 7 additions & 0 deletions backend/jest.config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
module.exports = {
transform: {
'node_modules': 'ts-jest',
},
preset: 'ts-jest',
testEnvironment: 'node',
};
9,316 changes: 6,792 additions & 2,524 deletions backend/package-lock.json

Large diffs are not rendered by default.

11 changes: 8 additions & 3 deletions backend/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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"
},
Expand Down Expand Up @@ -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"
}
}
77 changes: 77 additions & 0 deletions backend/src/__tests__/dockerRepositoryURL.spec.ts
Original file line number Diff line number Diff line change
@@ -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');
});
102 changes: 72 additions & 30 deletions backend/src/routes/api/images/imageUtils.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { IMAGE_ANNOTATIONS, imageUrlRegex } from '../../../utils/constants';
import { IMAGE_ANNOTATIONS } from '../../../utils/constants';
import { convertLabelsToString } from '../../../utils/componentUtils';
import {
ImageStreamTag,
Expand All @@ -7,14 +7,48 @@ import {
ImageStream,
TagContent,
KubeFastifyInstance,
BYONImageCreateRequest,
BYONImageUpdateRequest,
BYONImagePackage,
BYONImage,
} from '../../../types';
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: trimmedString,
host: result[1],
image: result[2],
tag: result[3],
};
};

export const getImageList = async (
fastify: KubeFastifyInstance,
labels: { [key: string]: string },
Expand Down Expand Up @@ -189,18 +223,25 @@ const packagesToString = (packages: BYONImagePackage[]): string => {
return '[]';
};
const mapImageStreamToBYONImage = (is: ImageStream): BYONImage => ({
id: is.metadata.name,
name: is.metadata.annotations['opendatahub.io/notebook-image-name'],
description: is.metadata.annotations['opendatahub.io/notebook-image-desc'],
id: is.metadata.uid,
name: is.metadata.name,
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: jsonParsePackage(
is.spec.tags?.[0]?.annotations?.['opendatahub.io/notebook-python-dependencies'],
),
software: jsonParsePackage(is.spec.tags?.[0]?.annotations?.['opendatahub.io/notebook-software']),
uploaded: is.metadata.creationTimestamp,
imported_time: is.metadata.creationTimestamp,
url: is.metadata.annotations['opendatahub.io/notebook-image-url'],
user: is.metadata.annotations['opendatahub.io/notebook-image-creator'],
provider: is.metadata.annotations['opendatahub.io/notebook-image-creator'],
});

export const postImage = async (
Expand All @@ -209,15 +250,13 @@ export const postImage = async (
): Promise<{ success: boolean; error: string }> => {
const customObjectsApi = fastify.kube.customObjectsApi;
const namespace = fastify.kube.namespace;
const body = request.body as BYONImageCreateRequest;
const fullUrl = body.url;
const matchArray = fullUrl.match(imageUrlRegex);
// check if the host is valid
if (!matchArray[1]) {
const body = request.body as BYONImage;
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',
Expand All @@ -228,20 +267,23 @@ 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.name };
return {
success: false,
error: 'Duplicated name. Unable to add notebook image: ' + body.display_name,
};
}

const payload: ImageStream = {
kind: 'ImageStream',
apiVersion: 'image.openshift.io/v1',
metadata: {
annotations: {
'opendatahub.io/notebook-image-desc': body.description ? body.description : '',
'opendatahub.io/notebook-image-name': body.name,
'opendatahub.io/notebook-image-url': fullUrl,
'opendatahub.io/notebook-image-creator': body.user,
'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-creator': body.provider,
},
name: `byon-${Date.now()}`,
name: `custom-${translateDisplayNameForK8s(body.display_name)}`,
namespace: namespace,
labels: labels,
},
Expand All @@ -254,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',
},
],
},
Expand Down Expand Up @@ -319,7 +361,7 @@ export const updateImage = async (
const customObjectsApi = fastify.kube.customObjectsApi;
const namespace = fastify.kube.namespace;
const params = request.params as { image: string };
const body = request.body as BYONImageUpdateRequest;
const body = request.body as BYONImage;
const labels = {
'app.kubernetes.io/created-by': 'byon',
'opendatahub.io/notebook-image': 'true',
Expand All @@ -328,8 +370,8 @@ export const updateImage = async (
const imageStreams = await getImageStreams(fastify, labels);
const validName = imageStreams.filter(
(is) =>
is.metadata.annotations['opendatahub.io/notebook-image-name'] === body.name &&
is.metadata.name !== body.id,
is.metadata.annotations['opendatahub.io/notebook-image-name'] === body.display_name &&
is.metadata.name !== body.name,
);

if (validName.length > 0) {
Expand Down Expand Up @@ -362,15 +404,15 @@ export const updateImage = async (
);
}

if (typeof body.visible !== undefined) {
if (body.visible !== undefined) {
if (body.visible) {
imageStream.metadata.labels['opendatahub.io/notebook-image'] = 'true';
} else {
imageStream.metadata.labels['opendatahub.io/notebook-image'] = 'false';
}
}
if (body.name) {
imageStream.metadata.annotations['opendatahub.io/notebook-image-name'] = body.name;
if (body.display_name) {
imageStream.metadata.annotations['opendatahub.io/notebook-image-name'] = body.display_name;
}

if (body.description !== undefined) {
Expand Down
31 changes: 10 additions & 21 deletions backend/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -473,36 +473,24 @@ export type ODHSegmentKey = {

export type BYONImage = {
id: string;
user?: string;
uploaded?: Date;
error?: string;
} & BYONImageCreateRequest &
BYONImageUpdateRequest;

export type BYONImageCreateRequest = {
// 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;
name: string;
url: string;
description?: 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.
user: string;
software?: BYONImagePackage[];
packages?: BYONImagePackage[];
display_name: string;
description: string;
visible: boolean;
software: BYONImagePackage[];
packages: BYONImagePackage[];
};

export type ImageTag = {
image: ImageInfo | undefined;
tag: ImageTagInfo | undefined;
};

export type BYONImageUpdateRequest = {
id: string;
name?: string;
description?: string;
visible?: boolean;
software?: BYONImagePackage[];
packages?: BYONImagePackage[];
};

export type BYONImagePackage = {
name: string;
version: string;
Expand Down Expand Up @@ -553,6 +541,7 @@ export type ImageStream = {
namespace: string;
labels?: { [key: string]: string };
annotations?: { [key: string]: string };
creationTimestamp?: string;
};
spec: {
lookupPolicy?: {
Expand Down
6 changes: 6 additions & 0 deletions backend/tsconfig.prod.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
{
"extends": "./tsconfig",
"exclude": [
"src/__tests__/"
]
}
Loading