From 5312089fa606f320d8c64def37ac872a62c715e3 Mon Sep 17 00:00:00 2001 From: Mark Yen Date: Fri, 3 May 2024 12:45:11 -0700 Subject: [PATCH 1/3] Extensions: split getting and initializing At some point we had a call site that needed to both get an extension manager as well initialize it. That no longer exists, so simplify the code by splitting it into two functions. We also are now documented to return HTTP 503 if the extension API endpoints are called before the extension subsystem has been set up. Signed-off-by: Mark Yen --- background.ts | 21 +++++++------ .../assets/specs/command-api.yaml | 4 +++ .../main/commandServer/httpCommandServer.ts | 13 ++++++-- pkg/rancher-desktop/main/extensions/index.ts | 30 ------------------- .../main/extensions/manager.ts | 21 +++++-------- 5 files changed, 33 insertions(+), 56 deletions(-) diff --git a/background.ts b/background.ts index cc2e5db520c..5840658e4bf 100644 --- a/background.ts +++ b/background.ts @@ -552,9 +552,9 @@ async function startK8sManager() { } await k8smanager.start(cfg); - const getEM = (await import('@pkg/main/extensions/manager')).default; + const { initializeExtensionManager } = await import('@pkg/main/extensions/manager'); - await getEM(k8smanager.containerEngineClient, cfg); + await initializeExtensionManager(k8smanager.containerEngineClient, cfg); window.send('extensions/changed'); } @@ -1493,25 +1493,28 @@ class BackgroundCommandWorker implements CommandWorkerInterface { async listExtensions() { const extensionManager = await getExtensionManager(); - const extensions = await extensionManager?.getInstalledExtensions() ?? []; + + if (!extensionManager) { + return undefined; + } + const extensions = await extensionManager.getInstalledExtensions(); const entries = await Promise.all(extensions.map(async x => [x.id, { version: x.version, metadata: await x.metadata, labels: await x.labels, }] as const)); - return Promise.resolve(Object.fromEntries(entries)); + return Object.fromEntries(entries); } async installExtension(image: string, state: 'install' | 'uninstall'): Promise<{status: number, data?: any}> { const em = await getExtensionManager(); - const extension = await em?.getExtension(image, { preferInstalled: state === 'uninstall' }); - if (!extension) { - console.debug(`Failed to install extension ${ image }: could not get extension.`); - - return { status: 503 }; + if (!em) { + return { status: 503, data: 'Extension manager is not ready yet.' }; } + const extension = await em.getExtension(image, { preferInstalled: state === 'uninstall' }); + if (state === 'install') { console.debug(`Installing extension ${ image }...`); try { diff --git a/pkg/rancher-desktop/assets/specs/command-api.yaml b/pkg/rancher-desktop/assets/specs/command-api.yaml index d2cf5ebef6c..452bb746262 100644 --- a/pkg/rancher-desktop/assets/specs/command-api.yaml +++ b/pkg/rancher-desktop/assets/specs/command-api.yaml @@ -141,6 +141,10 @@ paths: type: object additionalProperties: type: string + '503': + description: >- + The extension manager has not been loaded yet. The client should + retry the request at some future point in time. /v1/extensions/install: post: diff --git a/pkg/rancher-desktop/main/commandServer/httpCommandServer.ts b/pkg/rancher-desktop/main/commandServer/httpCommandServer.ts index 53aaf9ef529..aa1f49f8efc 100644 --- a/pkg/rancher-desktop/main/commandServer/httpCommandServer.ts +++ b/pkg/rancher-desktop/main/commandServer/httpCommandServer.ts @@ -698,7 +698,11 @@ export class HttpCommandServer { protected async listExtensions(request: express.Request, response: express.Response, context: commandContext): Promise { const extensions = await this.commandWorker.listExtensions(); - response.status(200).type('json').send(extensions); + if (!extensions) { + response.status(503).type('txt').send('Extension manager is not ready yet.'); + } else { + response.status(200).type('json').send(extensions); + } } protected async installExtension(request: express.Request, response: express.Response, context: commandContext): Promise { @@ -898,8 +902,11 @@ export interface CommandWorkerInterface { setBackendState: (state: BackendState) => Promise; // #region extensions - /** List the installed extensions with their versions */ - listExtensions(): Promise}>>; + /** + * List the installed extensions with their versions. + * If the extension manager is not ready, returns undefined. + */ + listExtensions(): Promise}> | undefined>; /** * Install or uninstall the given extension, returning an appropriate HTTP status code. * @param state Whether to install or uninstall the extension. diff --git a/pkg/rancher-desktop/main/extensions/index.ts b/pkg/rancher-desktop/main/extensions/index.ts index 5d3db50cb39..fcb073fefcd 100644 --- a/pkg/rancher-desktop/main/extensions/index.ts +++ b/pkg/rancher-desktop/main/extensions/index.ts @@ -1,31 +1 @@ -import { ExtensionManager } from './types'; - -import { ContainerEngineClient } from '@pkg/backend/containerClient'; -import type { Settings } from '@pkg/config/settings'; -import { RecursiveReadonly } from '@pkg/utils/typeUtils'; - export * from './types'; - -/** - * Get the extension manager for the given client using the given settings. - * If the client is not given, return the previously fetched extension manager. - * It is an error to call this without a client if not previously called with - * one. - */ -export async function getExtensionManager(): Promise; -export async function getExtensionManager(client: ContainerEngineClient, cfg: RecursiveReadonly): Promise; -export async function getExtensionManager(client?: ContainerEngineClient, cfg?: RecursiveReadonly): Promise { - // We do a local import here to ensure we don't pull in everything when this - // is just imported for the types. - const getEMImpl = (await import('./manager')).default; - - if (client) { - if (!cfg) { - throw new Error(`getExtensionManager called without configuration`); - } - - return getEMImpl(client, cfg); - } else { - return getEMImpl(); - } -} diff --git a/pkg/rancher-desktop/main/extensions/manager.ts b/pkg/rancher-desktop/main/extensions/manager.ts index 8cd93ea4a99..6248d4f6a1a 100644 --- a/pkg/rancher-desktop/main/extensions/manager.ts +++ b/pkg/rancher-desktop/main/extensions/manager.ts @@ -576,19 +576,14 @@ export class ExtensionManagerImpl implements ExtensionManager { } } -async function getExtensionManager(): Promise; -async function getExtensionManager(client: ContainerEngineClient, cfg: RecursiveReadonly): Promise; -async function getExtensionManager(client?: ContainerEngineClient, cfg?: RecursiveReadonly): Promise { - if (!client || manager?.client === client) { - if (!client && !manager) { - console.debug(`Warning: cached client missing, returning nothing`); - } - - return manager; - } +function getExtensionManager(): Promise { + return Promise.resolve(manager); +} - if (!cfg) { - throw new Error(`getExtensionManager called without configuration`); +export async function initializeExtensionManager(client: ContainerEngineClient, cfg: RecursiveReadonly): Promise { + if (manager?.client === client) { + // The manager is already the correct one; do nothing. + return; } await manager?.shutdown(); @@ -597,8 +592,6 @@ async function getExtensionManager(client?: ContainerEngineClient, cfg?: Recursi manager = new ExtensionManagerImpl(client, cfg.containerEngine.name === ContainerEngine.CONTAINERD); await manager.init(cfg); - - return manager; } export default getExtensionManager; From bbed332a17684838eed1d5d3a9653b1b4e9df26c Mon Sep 17 00:00:00 2001 From: Mark Yen Date: Fri, 3 May 2024 17:27:17 -0700 Subject: [PATCH 2/3] BATS: Wait for extensions in extensions tests Now that we have a reliable status code for when the extension manager is not ready, we can look for it and wait for the extension manager to be ready before running the extension related tests. Signed-off-by: Mark Yen --- bats/tests/extensions/allow-list.bats | 1 + bats/tests/extensions/containers.bats | 1 + bats/tests/extensions/install.bats | 1 + bats/tests/helpers/vm.bash | 21 +++++++++++++++++++++ 4 files changed, 24 insertions(+) diff --git a/bats/tests/extensions/allow-list.bats b/bats/tests/extensions/allow-list.bats index 4f5267af4a0..1a75f4cc9ed 100644 --- a/bats/tests/extensions/allow-list.bats +++ b/bats/tests/extensions/allow-list.bats @@ -55,6 +55,7 @@ check_extension_installed() { # refute, name } @test 'when no extension allow list is set up, all extensions can install' { + wait_for_extension_manager write_allow_list '' rdctl extension install rd/extension/basic check_extension_installed diff --git a/bats/tests/extensions/containers.bats b/bats/tests/extensions/containers.bats index 8ebd07ac171..47f02b7fe06 100644 --- a/bats/tests/extensions/containers.bats +++ b/bats/tests/extensions/containers.bats @@ -41,6 +41,7 @@ encoded_id() { # variant } @test 'no extensions installed' { + wait_for_extension_manager run rdctl api /v1/extensions assert_success assert_output $'\x7b'$'\x7d' # empty JSON dict, {} diff --git a/bats/tests/extensions/install.bats b/bats/tests/extensions/install.bats index 401b837566d..0a77d863b3c 100644 --- a/bats/tests/extensions/install.bats +++ b/bats/tests/extensions/install.bats @@ -38,6 +38,7 @@ encoded_id() { # variant @test 'start container engine' { RD_ENV_EXTENSIONS=1 start_container_engine wait_for_container_engine + wait_for_extension_manager } @test 'no extensions installed' { diff --git a/bats/tests/helpers/vm.bash b/bats/tests/helpers/vm.bash index d0a3a6f65a4..7449c24c7fb 100644 --- a/bats/tests/helpers/vm.bash +++ b/bats/tests/helpers/vm.bash @@ -1,3 +1,7 @@ +local_setup_file() { + bats_require_minimum_version 1.5.0 +} + wait_for_shell() { if is_windows; then try --max 48 --delay 5 rdctl shell true @@ -378,6 +382,23 @@ wait_for_container_engine() { try --max 12 --delay 10 get_container_engine_info } +# Wait fot the extension manager to be initialized. +wait_for_extension_manager() { + trace "waiting for extension manager to be ready" + # We want to match specific error strings, so we can't use try() directly. + local count=0 max=30 message + while true; do + run --separate-stderr rdctl api /extensions + if ((status == 0 || ++count >= max)); then + break + fi + message=$(jq_output .message) + output="$message" assert_output "503 Service Unavailable" + sleep 10 + done + trace "$count/$max tries: wait_for_extension_manager" +} + # See definition of `State` in # pkg/rancher-desktop/backend/backend.ts for an explanation of each state. assert_backend_available() { From 5973212fc84fa271a36baceeb1a8c7c24f20202e Mon Sep 17 00:00:00 2001 From: Mark Yen Date: Mon, 6 May 2024 12:41:46 -0700 Subject: [PATCH 3/3] BATS: Fix minimum BATS version Rather than having specific requests for minimum BATS version, just have one globally in `load.bash` and let everything else assume we have a good version. Signed-off-by: Mark Yen --- bats/tests/helpers/load.bash | 4 ++++ bats/tests/helpers/vm.bash | 4 ---- bats/tests/k8s/traefik.bats | 1 - bats/tests/k8s/wasm.bats | 1 - 4 files changed, 4 insertions(+), 6 deletions(-) diff --git a/bats/tests/helpers/load.bash b/bats/tests/helpers/load.bash index 53b3d7ef35d..74749f83c1e 100644 --- a/bats/tests/helpers/load.bash +++ b/bats/tests/helpers/load.bash @@ -89,6 +89,10 @@ setup_file() { if semver_gt 5.0.0 "$(semver "$BASH_VERSION")"; then fail "Bash 5.0.0 is required; you have $BASH_VERSION" fi + # We currently use a submodule that provides BATS 1.10; we do not test + # against any other copy of BATS (and therefore only support the version in + # that submodule). + bats_require_minimum_version 1.10.0 # Ideally this should be printed only when using the tap formatter, # but I don't see a way to check for this. echo "# ===== $RD_TEST_FILENAME =====" >&3 diff --git a/bats/tests/helpers/vm.bash b/bats/tests/helpers/vm.bash index 7449c24c7fb..6b50b4c1367 100644 --- a/bats/tests/helpers/vm.bash +++ b/bats/tests/helpers/vm.bash @@ -1,7 +1,3 @@ -local_setup_file() { - bats_require_minimum_version 1.5.0 -} - wait_for_shell() { if is_windows; then try --max 48 --delay 5 rdctl shell true diff --git a/bats/tests/k8s/traefik.bats b/bats/tests/k8s/traefik.bats index efcc948a973..313ffc54a62 100644 --- a/bats/tests/k8s/traefik.bats +++ b/bats/tests/k8s/traefik.bats @@ -6,7 +6,6 @@ local_setup() { skip "Test does not yet work from inside a WSL distro when using networking tunnel, since it requires WSL integration" fi needs_port 80 - bats_require_minimum_version 1.5.0 } assert_traefik_pods_are_down() { diff --git a/bats/tests/k8s/wasm.bats b/bats/tests/k8s/wasm.bats index 66a5732b86b..40e65a4fe7e 100644 --- a/bats/tests/k8s/wasm.bats +++ b/bats/tests/k8s/wasm.bats @@ -16,7 +16,6 @@ assert_traefik_crd_established() { # Get Kubernetes RuntimeClasses; sets $output to the JSON list. get_runtime_classes() { # kubectl may emit warnings here; ensure that we don't fall over. - bats_require_minimum_version 1.5.0 run --separate-stderr kubectl get RuntimeClasses --output json assert_success || return