From 480e7d1ede34e5f9b492b6c324c5700274907556 Mon Sep 17 00:00:00 2001 From: Eric Promislow Date: Mon, 26 Jun 2023 13:34:19 -0700 Subject: [PATCH 1/3] Handle case were set/start proposed changed field kubernetes.version is locked. Signed-off-by: Eric Promislow --- background.ts | 31 +++++++++++++++---- e2e/lockedFields.e2e.spec.ts | 14 ++++++++- .../config/commandLineOptions.ts | 22 ++++++++----- 3 files changed, 53 insertions(+), 14 deletions(-) diff --git a/background.ts b/background.ts index ee2a8e88090..79e02170c0a 100644 --- a/background.ts +++ b/background.ts @@ -935,7 +935,7 @@ function validateEarlySettings(cfg: settings.Settings, newSettings: RecursivePar // We'd have to add more code to report that. // It isn't worth adding that code yet. It might never be needed. const newSettingsForValidation = _.omit(newSettings, 'kubernetes.version'); - const [_needToUpdate, errors] = new SettingsValidator().validateSettings(cfg, newSettingsForValidation, lockedFields); + const errors = (new SettingsValidator().validateSettings(cfg, newSettingsForValidation, lockedFields))[1]; if (errors.length > 0) { throw new LockedFieldError(`Error in deployment profiles:\n${ errors.join('\n') }`); @@ -952,7 +952,6 @@ function validateEarlySettings(cfg: settings.Settings, newSettings: RecursivePar * The `requestShutdown` method is a special case that never returns. */ class BackgroundCommandWorker implements CommandWorkerInterface { - protected k8sVersions: string[] = []; protected settingsValidator = new SettingsValidator(); /** @@ -960,12 +959,32 @@ class BackgroundCommandWorker implements CommandWorkerInterface { * initialization. */ protected async validateSettings(existingSettings: settings.Settings, newSettings: RecursivePartial) { - if (this.k8sVersions.length === 0) { - this.k8sVersions = (await k8smanager.kubeBackend.availableVersions).map(entry => entry.version.version); - this.settingsValidator.k8sVersions = this.k8sVersions; + let clearVersionsAfterTesting = false; + + if (newSettings.kubernetes?.version && this.settingsValidator.k8sVersions.length === 0) { + // If we're starting up (by running `rdctl start...`) we probably haven't loaded all the k8s versions yet. + // We don't want to verify the proposed version makes sense (if it doesn't, we'll assign the default version later). + // Here we just want to make sure that if we're changing the version to a different value from the current one, + // the field isn't locked. + let currentK8sVersions = (await k8smanager.kubeBackend.availableVersions).map(entry => entry.version.version); + + if (currentK8sVersions.length === 0 || (currentK8sVersions.length === 1 && currentK8sVersions[0] === '0.0.0')) { + clearVersionsAfterTesting = true; + currentK8sVersions = [newSettings.kubernetes.version]; + if (existingSettings.kubernetes.version) { + currentK8sVersions.push(existingSettings.kubernetes.version); + } + } + this.settingsValidator.k8sVersions = currentK8sVersions; + } + + const result = this.settingsValidator.validateSettings(existingSettings, newSettings, settings.getLockedSettings()); + + if (clearVersionsAfterTesting) { + this.settingsValidator.k8sVersions = []; } - return this.settingsValidator.validateSettings(existingSettings, newSettings, settings.getLockedSettings()); + return result; } getSettings() { diff --git a/e2e/lockedFields.e2e.spec.ts b/e2e/lockedFields.e2e.spec.ts index 89676e5d508..2b9815184fc 100644 --- a/e2e/lockedFields.e2e.spec.ts +++ b/e2e/lockedFields.e2e.spec.ts @@ -70,6 +70,8 @@ test.describe('Locked fields', () => { } test.describe.configure({ mode: 'serial' }); + const lockedK8sVersion = '1.26.3'; + const proposedK8sVersion = '1.26.1'; test.beforeAll(async() => { await tool('rdctl', 'factory-reset', '--verbose'); @@ -85,7 +87,7 @@ test.describe('Locked fields', () => { await saveUserProfile(); await createUserProfile( { containerEngine: { allowedImages: { enabled: true } } }, - { containerEngine: { allowedImages: { enabled: true, patterns: ['c', 'd', 'f'] } } }, + { containerEngine: { allowedImages: { enabled: true, patterns: ['c', 'd', 'f'] } }, kubernetes: { version: lockedK8sVersion } }, ); electronApp = await startRancherDesktop(__filename); context = electronApp.context(); @@ -123,5 +125,15 @@ test.describe('Locked fields', () => { stdout: '', stderr: expect.stringContaining("field 'containerEngine.allowedImages.enabled' is locked"), }); + await expect(rdctl(['set', `--kubernetes.version=${ proposedK8sVersion }`])) + .resolves.toMatchObject({ + stdout: '', + stderr: expect.stringContaining("field 'kubernetes.version' is locked"), + }); + await expect(rdctl(['set', `--kubernetes.version=${ lockedK8sVersion }`])) + .resolves.toMatchObject({ + stdout: expect.stringContaining('Status: no changes necessary.'), + stderr: '', + }); }); }); diff --git a/pkg/rancher-desktop/config/commandLineOptions.ts b/pkg/rancher-desktop/config/commandLineOptions.ts index 27d97c28796..68d164848d9 100644 --- a/pkg/rancher-desktop/config/commandLineOptions.ts +++ b/pkg/rancher-desktop/config/commandLineOptions.ts @@ -115,13 +115,21 @@ export function updateFromCommandLine(cfg: Settings, lockedFields: LockedSetting } newSettings = _.merge(newSettings, getObjectRepresentation(fqFieldName as RecursiveKeys, finalValue)); } + const settingsValidator = new SettingsValidator(); - // RD hasn't loaded the supported k8s versions yet, so have it defer actually checking the specified version. - // If it can't find this version, it will silently move to the closest version. - // We'd have to add more code to report that. - // It isn't worth adding that code yet. It might never be needed. - const newSettingsForValidation = _.omit(newSettings, 'kubernetes.version'); - const [needToUpdate, errors] = (new SettingsValidator()).validateSettings(cfg, newSettingsForValidation, lockedFields); + if (_.has(newSettings, 'kubernetes.version')) { + // RD hasn't loaded the supported k8s versions yet, so fake the list. + // If the field is locked, we don't need to know what it's locked to, + // just that the proposed version is different from the current version. + /// The current version doesn't have to be the locked version, but will be after processing ends. + const limitedK8sVersionList: Array = [newSettings.kubernetes?.version ?? '']; + + if (cfg.kubernetes.version) { + limitedK8sVersionList.push(cfg.kubernetes.version); + } + settingsValidator.k8sVersions = limitedK8sVersionList; + } + const [needToUpdate, errors] = settingsValidator.validateSettings(cfg, newSettings, lockedFields); if (errors.length > 0) { const errorString = `Error in command-line options:\n${ errors.join('\n') }`; @@ -131,7 +139,7 @@ export function updateFromCommandLine(cfg: Settings, lockedFields: LockedSetting } throw new Error(errorString); } - if (needToUpdate || _.has(newSettings, 'kubernetes.version')) { + if (needToUpdate) { cfg = _.merge(cfg, newSettings); save(cfg); } else { From 0015031f08216aada800402d81de1d057c5c023d Mon Sep 17 00:00:00 2001 From: Eric Promislow Date: Wed, 28 Jun 2023 14:51:10 -0700 Subject: [PATCH 2/3] Apply reviewer's comments. Signed-off-by: Eric Promislow --- background.ts | 6 ++++-- pkg/rancher-desktop/config/commandLineOptions.ts | 7 ++++--- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/background.ts b/background.ts index 79e02170c0a..ebff13e9fb2 100644 --- a/background.ts +++ b/background.ts @@ -935,7 +935,7 @@ function validateEarlySettings(cfg: settings.Settings, newSettings: RecursivePar // We'd have to add more code to report that. // It isn't worth adding that code yet. It might never be needed. const newSettingsForValidation = _.omit(newSettings, 'kubernetes.version'); - const errors = (new SettingsValidator().validateSettings(cfg, newSettingsForValidation, lockedFields))[1]; + const [, errors] = new SettingsValidator().validateSettings(cfg, newSettingsForValidation, lockedFields); if (errors.length > 0) { throw new LockedFieldError(`Error in deployment profiles:\n${ errors.join('\n') }`); @@ -966,9 +966,11 @@ class BackgroundCommandWorker implements CommandWorkerInterface { // We don't want to verify the proposed version makes sense (if it doesn't, we'll assign the default version later). // Here we just want to make sure that if we're changing the version to a different value from the current one, // the field isn't locked. + // + let currentK8sVersions = (await k8smanager.kubeBackend.availableVersions).map(entry => entry.version.version); - if (currentK8sVersions.length === 0 || (currentK8sVersions.length === 1 && currentK8sVersions[0] === '0.0.0')) { + if (currentK8sVersions.length === 0) { clearVersionsAfterTesting = true; currentK8sVersions = [newSettings.kubernetes.version]; if (existingSettings.kubernetes.version) { diff --git a/pkg/rancher-desktop/config/commandLineOptions.ts b/pkg/rancher-desktop/config/commandLineOptions.ts index 68d164848d9..e25af3109bf 100644 --- a/pkg/rancher-desktop/config/commandLineOptions.ts +++ b/pkg/rancher-desktop/config/commandLineOptions.ts @@ -116,13 +116,14 @@ export function updateFromCommandLine(cfg: Settings, lockedFields: LockedSetting newSettings = _.merge(newSettings, getObjectRepresentation(fqFieldName as RecursiveKeys, finalValue)); } const settingsValidator = new SettingsValidator(); + const newKubernetesVersion = newSettings.kubernetes?.version; - if (_.has(newSettings, 'kubernetes.version')) { + if (newKubernetesVersion) { // RD hasn't loaded the supported k8s versions yet, so fake the list. // If the field is locked, we don't need to know what it's locked to, // just that the proposed version is different from the current version. - /// The current version doesn't have to be the locked version, but will be after processing ends. - const limitedK8sVersionList: Array = [newSettings.kubernetes?.version ?? '']; + // The current version doesn't have to be the locked version, but will be after processing ends. + const limitedK8sVersionList: Array = [newKubernetesVersion]; if (cfg.kubernetes.version) { limitedK8sVersionList.push(cfg.kubernetes.version); From f76c2075b1016a9cb3e6a3730400270a28c18676 Mon Sep 17 00:00:00 2001 From: Eric Promislow Date: Mon, 3 Jul 2023 11:28:46 -0700 Subject: [PATCH 3/3] No need for a special-purpose k8s version. Signed-off-by: Eric Promislow --- background.ts | 4 +--- pkg/rancher-desktop/backend/mock.ts | 2 +- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/background.ts b/background.ts index ebff13e9fb2..fbd3139ab6a 100644 --- a/background.ts +++ b/background.ts @@ -963,11 +963,9 @@ class BackgroundCommandWorker implements CommandWorkerInterface { if (newSettings.kubernetes?.version && this.settingsValidator.k8sVersions.length === 0) { // If we're starting up (by running `rdctl start...`) we probably haven't loaded all the k8s versions yet. - // We don't want to verify the proposed version makes sense (if it doesn't, we'll assign the default version later). + // We don't want to verify if the proposed version makes sense (if it doesn't, we'll assign the default version later). // Here we just want to make sure that if we're changing the version to a different value from the current one, // the field isn't locked. - // - let currentK8sVersions = (await k8smanager.kubeBackend.availableVersions).map(entry => entry.version.version); if (currentK8sVersions.length === 0) { diff --git a/pkg/rancher-desktop/backend/mock.ts b/pkg/rancher-desktop/backend/mock.ts index ccab40972b7..2fa339d335a 100644 --- a/pkg/rancher-desktop/backend/mock.ts +++ b/pkg/rancher-desktop/backend/mock.ts @@ -178,7 +178,7 @@ export default class MockBackend extends events.EventEmitter implements VMExecut } class MockKubernetesBackend extends events.EventEmitter implements KubernetesBackend { - readonly availableVersions = Promise.resolve([{ version: new semver.SemVer('0.0.0'), channels: ['latest'] }]); + readonly availableVersions = Promise.resolve([]); version = ''; desiredPort = 9443;