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

RDX: Work with deployment profiles #4655

Merged
merged 6 commits into from
May 18, 2023
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
6 changes: 5 additions & 1 deletion background.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1097,7 +1097,9 @@ class BackgroundCommandWorker implements CommandWorkerInterface {
if (state === 'install') {
console.debug(`Installing extension ${ image }...`);
try {
if (await extension.install()) {
const { enabled, list } = cfg.application.extensions.allowed;

if (await extension.install(enabled ? list : undefined)) {
return { status: 201 };
} else {
return { status: 204 };
Expand All @@ -1109,6 +1111,8 @@ class BackgroundCommandWorker implements CommandWorkerInterface {
return { status: 422, data: `The image ${ image } has invalid extension metadata` };
case ExtensionErrorCode.FILE_NOT_FOUND:
return { status: 422, data: `The image ${ image } failed to install: ${ ex.message }` };
case ExtensionErrorCode.INSTALL_DENIED:
return { status: 403, data: `The image ${ image } is not an allowed extension` };
}
}
throw ex;
Expand Down
107 changes: 107 additions & 0 deletions bats/tests/extensions/allow-list.bats
Original file line number Diff line number Diff line change
@@ -0,0 +1,107 @@
load '../helpers/load'

setup() {
TESTDATA_DIR="${PATH_BATS_ROOT}/tests/extensions/testdata/"

if using_windows_exe; then
TESTDATA_DIR_CLI="$(wslpath -m "${TESTDATA_DIR}")"
else
TESTDATA_DIR_CLI="${TESTDATA_DIR}"
fi

if using_containerd; then
namespace_arg=('--namespace=rancher-desktop-extensions')
else
namespace_arg=()
fi
}

write_allow_list() { # list
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe a test that passes more than one arg to this function, because currently it only handles 0 or 1 values in the list?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It takes a single argument that is the JSON encoded list.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realized that, but none of the args contain a comma

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I thought I had one. Will fix.

local list=${1-}
local allowed=true

if [ -z "$list" ]; then
allowed=false
fi

# Note that the list preference is not writable using `rdctl set`, and we
# need to do a direct API call instead.

rdctl api /v1/settings --input - <<<'{
"version": '"$(get_setting .version)"',
"application": {
"extensions": {
"allowed": {
"enabled": '"${allowed}"',
"list": '"${list:-[]}"'
}
}
}
}'
}

@test 'factory reset' {
factory_reset
}

@test 'start container engine' {
RD_ENV_EXTENSIONS=1 start_container_engine
wait_for_container_engine
}

@test 'build extension testing image' {
ctrctl "${namespace_arg[@]}" build \
--tag "rd/extension/basic" \
--build-arg "variant=basic" \
"$TESTDATA_DIR_CLI"

run ctrctl "${namespace_arg[@]}" image list --format '{{ .Repository }}'
assert_success
assert_line "rd/extension/basic"
}

@test 'when no extension allow list is set up, all extensions can install' {
write_allow_list ''
rdctl extension install rd/extension/basic
rdctl extension uninstall rd/extension/basic
}

@test 'when an extension is explicitly allowed, it can be installed' {
write_allow_list '["rd/extension/basic:latest"]'
rdctl extension install rd/extension/basic:latest
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would also run rdctl extension ls and check for success and basic:latest when the extension whitelist is in effect

rdctl extension uninstall rd/extension/basic
}

@test 'when an extension is not in the allowe list, it cannot be installed' {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: "allowed"

write_allow_list '["rd/extension/other"]'
run rdctl extension install rd/extension/basic
assert_failure
}

@test 'when no tags given, any tag is allowed' {
write_allow_list '["rd/extension/basic"]'
ctrctl "${namespace_arg[@]}" tag rd/extension/basic rd/extension/basic:0.0.3
rdctl extension install rd/extension/basic:0.0.3
rdctl extension uninstall rd/extension/basic
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, I would verify that the extension is actually installed, not just that ext install didn't fail

}

@test 'when tags are given, only the specified tag is allowed' {
sleep 20
write_allow_list '["rd/extension/basic:0.0.2"]'
ctrctl "${namespace_arg[@]}" tag rd/extension/basic rd/extension/basic:0.0.3
run rdctl extension install rd/extension/basic:0.0.3
assert_failure
}

@test 'extensions can be allowed by organization' {
write_allow_list '["rd/extension/"]'
rdctl extension install rd/extension/basic
rdctl extension uninstall rd/extension/basic
}

@test 'extensions can be allowed by repository host' {
write_allow_list '["registry.test/"]'
ctrctl "${namespace_arg[@]}" tag rd/extension/basic registry.test/basic:0.0.3
rdctl extension install registry.test/basic:0.0.3
rdctl extension uninstall registry.test/basic
}
14 changes: 14 additions & 0 deletions pkg/rancher-desktop/assets/specs/command-api.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -345,6 +345,20 @@ components:
debug:
type: boolean
x-rd-usage: generate more verbose logging
extensions:
type: object
properties:
allowed:
type: object
properties:
enabled:
type: boolean
x-rd-hidden: true
list:
type: array
items:
type: string
x-rd-hidden: true
pathManagementStrategy:
type: string
enum: [manual, rcfiles]
Expand Down
10 changes: 8 additions & 2 deletions pkg/rancher-desktop/config/settings.ts
Original file line number Diff line number Diff line change
Expand Up @@ -69,8 +69,14 @@ export enum CacheMode {
export const defaultSettings = {
version: CURRENT_SETTINGS_VERSION,
application: {
adminAccess: true,
debug: false,
adminAccess: true,
debug: false,
extensions: {
allowed: {
enabled: false,
list: [] as Array<string>,
},
},
pathManagementStrategy: PathManagementStrategy.NotSet,
telemetry: { enabled: true },
/** Whether we should check for updates and apply them. */
Expand Down
39 changes: 36 additions & 3 deletions pkg/rancher-desktop/main/commandServer/settingsValidator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import {
} from '@pkg/config/settings';
import { NavItemName, navItemNames, TransientSettings } from '@pkg/config/transientSettings';
import { PathManagementStrategy } from '@pkg/integrations/pathManager';
import { validateImageName, validateImageTag } from '@pkg/utils/dockerUtils';
import { parseImageReference, validateImageName, validateImageTag } from '@pkg/utils/dockerUtils';
import { RecursivePartial } from '@pkg/utils/typeUtils';
import { preferencesNavItems } from '@pkg/window/preferences';

Expand Down Expand Up @@ -73,8 +73,14 @@ export default class SettingsValidator {
this.allowedSettings ||= {
version: this.checkUnchanged,
application: {
adminAccess: this.checkLima(this.checkBoolean),
debug: this.checkBoolean,
adminAccess: this.checkLima(this.checkBoolean),
debug: this.checkBoolean,
extensions: {
allowed: {
enabled: this.checkBoolean,
list: this.checkExtensionAllowList,
},
},
pathManagementStrategy: this.checkLima(this.checkPathManagementStrategy),
telemetry: { enabled: this.checkBoolean },
/** Whether we should check for updates and apply them. */
Expand Down Expand Up @@ -548,6 +554,33 @@ export default class SettingsValidator {
return !_.isEqual(desiredValue, currentValue);
}

protected checkExtensionAllowList(
mergedSettings: Settings,
currentValue: string[],
desiredValue: any,
errors: string[],
fqname: string,
): boolean {
if (_.isEqual(desiredValue, currentValue)) {
// Accept no-op changes
return false;
}

const changed = this.checkUniqueStringArray(mergedSettings, currentValue, desiredValue, errors, fqname);

if (errors.length) {
return changed;
}

for (const pattern of desiredValue as string[]) {
if (!parseImageReference(pattern, true)) {
errors.push(`${ fqname }: "${ pattern }" does not describe an image reference`);
}
}

return errors.length === 0 && changed;
}

protected checkPreferencesNavItemCurrent(
mergedSettings: TransientSettings,
currentValue: NavItemName,
Expand Down
49 changes: 49 additions & 0 deletions pkg/rancher-desktop/main/extensions/__tests__/extensions.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
import { ExtensionImpl } from '@pkg/main/extensions/extensions';

describe('ExtensionImpl', () => {
describe('checkInstallAllowed', () => {
const subject = ExtensionImpl['checkInstallAllowed'];

it('should reject invalid image references', () => {
expect(() => subject(undefined, '/')).toThrow();
});

it('should allow images if the allow list is not enabled', () => {
expect(() => subject(undefined, 'image')).not.toThrow();
});

it('should disallow any images given an empty list', () => {
expect(() => subject([], 'image')).toThrow();
});

it('should allow specified image', () => {
expect(() => subject(['other', 'image'], 'image')).not.toThrow();
});

it('should reject unknown image', () => {
expect(() => subject(['allowed'], 'image')).toThrow();
});

it('should support missing tags', () => {
expect(() => subject(['image'], 'image:1.0.0')).not.toThrow();
});

it('should reject images with the wrong tag', () => {
expect(() => subject(['image:0.1'], 'image:0.2')).toThrow();
});

it('should support image references with registries', () => {
const ref = 'r.example.test:1234/org/name:tag';

expect(() => subject([ref], ref)).not.toThrow();
});

it('should support org-level references', () => {
expect(() => subject(['test.invalid/org/'], 'test.invalid/org/image:tag')).not.toThrow();
});

it('should support registry-level references', () => {
expect(() => subject(['registry.test/'], 'registry.test/image:tag')).not.toThrow();
});
});
});
53 changes: 52 additions & 1 deletion pkg/rancher-desktop/main/extensions/extensions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import {

import type { ContainerEngineClient } from '@pkg/backend/containerClient';
import mainEvents from '@pkg/main/mainEvents';
import { parseImageReference } from '@pkg/utils/dockerUtils';
import Logging from '@pkg/utils/logging';
import paths from '@pkg/utils/paths';
import { defined } from '@pkg/utils/typeUtils';
Expand Down Expand Up @@ -142,9 +143,59 @@ export class ExtensionImpl implements Extension {
return this._iconName as Promise<string>;
}

async install(): Promise<boolean> {
/**
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uncaught typo from an earlier PR: does't at line 28 of pkg/rancher-desktop/main/extensions/extensions.ts

* Check if the given image is allowed to be installed according to the
* extension allow list.
* @throws If the image is not allowed to be installed.
*/
protected static checkInstallAllowed(allowedImages: readonly string[] | undefined, image: string) {
const desired = parseImageReference(image);
const code = ExtensionErrorCode.INSTALL_DENIED;
const prefix = `Disallowing install of ${ image }:`;

if (!desired) {
throw new ExtensionErrorImpl(code, `${ prefix } Invalid image reference`);
}
if (!allowedImages) {
return;
}
for (const pattern of allowedImages) {
const allowed = parseImageReference(pattern, true);

if (allowed?.tag && allowed.tag !== desired.tag) {
// This pattern doesn't match the tag, look for something else.
continue;
}

if (allowed?.registry.href !== desired.registry.href) {
// This pattern has a different registry
continue;
}

if (!allowed.name) {
// If there's no name given, the whole registry is allowed.
return '';
}

if (allowed.name.endsWith('/')) {
if (desired.name.startsWith(allowed.name)) {
// The allowed pattern ends with a slash, anything in the org is fine.
return '';
}
} else if (allowed.name === desired.name) {
return '';
}
}

throw new ExtensionErrorImpl(code, `${ prefix } Image is not allowed`);
}

async install(allowedImages: readonly string[] | undefined): Promise<boolean> {
const metadata = await this.metadata;

ExtensionImpl.checkInstallAllowed(allowedImages, this.image);
console.debug(`Image ${ this.image } is allowed to install: ${ allowedImages }`);

await fs.promises.mkdir(this.dir, { recursive: true });
try {
await this.installMetadata(this.dir, metadata);
Expand Down
4 changes: 3 additions & 1 deletion pkg/rancher-desktop/main/extensions/manager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,8 @@ class ExtensionManagerImpl implements ExtensionManager {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another question about an older commit: why is the block through which urgency is set at line 158 marked as const when it's a temporary object?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as const just means "don't try to be smart, and derive the types as I wrote it"; in this case, it means that it becomes {success: string, warning: string, error: string}, whereas without the as const it's a Record<string, string>. The latter doesn't work because <thing>[level] could end up being undefined (because it lost the information necessary to detect that <thing>[level], where level is "success" | "warning" | "error", would always have a string value).

// Install / uninstall extensions as needed.
const tasks: Promise<any>[] = [];
const { enabled: allowEnabled, list: allowListRaw } = config.application.extensions.allowed;
const allowList = allowEnabled ? allowListRaw : undefined;

for (const [repo, tag] of Object.entries(config.extensions)) {
if (!tag) {
Expand All @@ -221,7 +223,7 @@ class ExtensionManagerImpl implements ExtensionManager {

tasks.push((async(id: string) => {
try {
return (await this.getExtension(id)).install();
return (await this.getExtension(id)).install(allowList);
} catch (ex) {
console.error(`Failed to install extension ${ id }`, ex);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another earlier-commit comment:

FWICT, tag ??= undefined; is there just to get typescript to stop complaining that tag is a let-var and not a const? If so, there should be a comment specifying that. But I would prefer putting this line before the let line:

// eslint-disable-next-line prefer-const

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yeah, that works. (Ideally we could use const but that breaks repo.)

}
Copy link
Contributor

@ericpromislow ericpromislow May 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is await Promise.all(tasks) done just before the end of the surrounding for-block and not after it? I would have expected the code to have this structure:

const tasks: Promise<any>[] = [];
for (...) {
  tasks.push((async(id: string) => { ... })(ARGUMENT);
}
await Promise.all(tasks);

Instead the code is like this:

const tasks: Promise<any>[] = [];
for (...) {
  tasks.push((async(id: string) => { ... })(ARGUMENT);
  await Promise.all(tasks);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch; that is a bug.

Expand Down
6 changes: 5 additions & 1 deletion pkg/rancher-desktop/main/extensions/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -71,10 +71,13 @@ export interface Extension {

/**
* Install this extension.
* @param allowedImages The list of extension images that are allowed to be
* used; if all images are allowed, pass in undefined.
* @note If the extension is already installed, this is a no-op.
* @throws If the settings specify an allow list and this is not in it.
* @return Whether the extension was installed.
*/
install(): Promise<boolean>;
install(allowedImages: readonly string[] | undefined): Promise<boolean>;
/**
* Uninstall this extension.
* @note If the extension was not installed, this is a no-op.
Expand Down Expand Up @@ -169,6 +172,7 @@ export const ExtensionErrorMarker = Symbol('extension-error');
export enum ExtensionErrorCode {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From a recent commit, @param id at line 112 should be @param image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

INVALID_METADATA,
FILE_NOT_FOUND,
INSTALL_DENIED,
}

export interface ExtensionError extends Error {
Expand Down
Loading