Fix editing local rke1 and eks clusters (rancher#12583)

* rework isImported and provisionerDisplay cluster properties * refactor cluster unti tests to account for isImport property refactor * update CLI e2e test to expect successful download
eva-vashkevich · Nov 12, 2024 · 6f52e59 · 6f52e59
1 parent a3026d3
commit 6f52e59
Show file tree

Hide file tree

Showing 7 changed files with 81 additions and 114 deletions.
diff --git a/cypress/e2e/tests/pages/generic/about.spec.ts b/cypress/e2e/tests/pages/generic/about.spec.ts
@@ -112,8 +112,7 @@ describe('About Page', { testIsolation: 'off', tags: ['@generic', '@adminUser',
 
   describe('CLI Downloads', () => {
     // Shouldn't be needed with https://github.com/rancher/dashboard/issues/11393
-    // Change back to 200 when binaries are available - see https://github.com/rancher/dashboard/issues/12576
-    const expectedLinkStatusCode = 404;
+    const expectedLinkStatusCode = 200;
 
     // workaround to make the following CLI tests work https://github.com/cypress-io/cypress/issues/8089#issuecomment-1585159023
     beforeEach(() => {

diff --git a/shell/assets/translations/en-us.yaml b/shell/assets/translations/en-us.yaml
@@ -1967,7 +1967,7 @@ cluster:
     pinganyunecs: Pinganyun ECS
     pnap: phoenixNAP
     rackspace: RackSpace
-    rancherkubernetesengine: RKE
+    rancherkubernetesengine: RKE1
     rke2: RKE2
     rke: RKE1
     rkeWindows: Windows (RKE1 only)

diff --git a/shell/components/formatter/ClusterProvider.vue b/shell/components/formatter/ClusterProvider.vue
@@ -25,12 +25,12 @@ export default {
         {{ row.machineProviderDisplay }}
       </span>
     </template>
-    <template v-else-if="row.isCustom">
-      {{ t('cluster.provider.custom') }}
-    </template>
     <template v-else-if="row.isImported">
       {{ t('cluster.provider.imported') }}
     </template>
+    <template v-else-if="row.isCustom">
+      {{ t('cluster.provider.custom') }}
+    </template>
     <div class="text-muted">
       {{ row.provisionerDisplay }}
     </div>

diff --git a/shell/models/__tests__/management.cattle.io.cluster.test.ts b/shell/models/__tests__/management.cattle.io.cluster.test.ts
@@ -4,12 +4,34 @@ jest.mock('@shell/utils/clipboard', () => {
   return { copyTextToClipboard: jest.fn(() => Promise.resolve({})) };
 });
 
+const importedRKE2ClusterInfo = { status: { driver: 'rke2', provider: 'rke2' } };
+
+const provisionedRKE2ClusterInfo = { status: { driver: 'rke2', provider: 'imported' } };
+
+const importedK3sClusterInfo = { status: { driver: 'k3s', provider: 'k3s' } };
+
+const provisionedK3sClusterInfo = { status: { driver: 'k3s', provider: 'imported' } };
+
+const importedAksClusterInfo = { spec: { aksConfig: { imported: true } }, status: { provider: 'aks', driver: 'AKS' } };
+
+const provisionedAksClusterInfo = { spec: { aksConfig: { imported: false } }, status: { provider: 'aks', driver: 'AKS' } };
+
+const importedRKE1ClusterInfo = { status: { provider: 'rke', driver: 'imported' } };
+
+const provisionedRKE1ClusterInfo = { status: { provider: 'rke', driver: 'rancherKubernetesEngine' } };
+
+const localRKE1ClusterInfo = { status: { provider: 'rke', driver: 'imported' } };
+
+const localRKE2ClusterInfo = { status: { provider: 'rke2', driver: 'rke2' } };
+
+const localEKSClusterInfo = { status: { provider: 'eks', driver: 'imported' } };
+
 describe('class MgmtCluster', () => {
   describe('provisioner', () => {
     const testCases = [
-      [{ provider: 'rke', driver: 'imported' }, 'rke'],
-      [{ provider: 'k3s', driver: 'K3S' }, 'k3s'],
-      [{ provider: 'aks', driver: 'AKS' }, 'aks'],
+      [{ provider: 'rke', driver: 'imported' }, 'imported'],
+      [{ provider: 'k3s', driver: 'K3S' }, 'K3S'],
+      [{ provider: 'aks', driver: 'AKS' }, 'AKS'],
       [{}, 'imported'],
     ];
 
@@ -20,4 +42,24 @@ describe('class MgmtCluster', () => {
     }
     );
   });
+
+  describe('isImported', () => {
+    it.each([
+      [importedRKE2ClusterInfo, true],
+      [provisionedRKE2ClusterInfo, false],
+      [importedK3sClusterInfo, true],
+      [provisionedK3sClusterInfo, false],
+      [importedAksClusterInfo, true],
+      [provisionedAksClusterInfo, false],
+      [importedRKE1ClusterInfo, true],
+      [provisionedRKE1ClusterInfo, false],
+      [localRKE1ClusterInfo, true],
+      [localRKE2ClusterInfo, true],
+      [localEKSClusterInfo, true]
+    ])('should return isImported based on props data', (clusterData, expected) => {
+      const cluster = new MgmtCluster(clusterData);
+
+      expect(cluster.isImported).toBe(expected);
+    });
+  });
 });
diff --git a/shell/models/__tests__/provisioning.cattle.io.cluster.test.ts b/shell/models/__tests__/provisioning.cattle.io.cluster.test.ts
@@ -1,31 +1,6 @@
 import ProvCluster from '@shell/models/provisioning.cattle.io.cluster';
 
 describe('class ProvCluster', () => {
-  const importedClusterInfo = {
-    clusterName: 'test', provisioner: 'imported', mgmt: { spec: { gkeConfig: {} } }, spec: {}
-  };
-  const importedGkeClusterInfo = {
-    clusterName: 'test', provisioner: 'rke2', mgmt: { spec: { gkeConfig: { imported: true } } }
-  };
-  const importedAksClusterInfo = {
-    clusterName: 'test', provisioner: 'rke2', mgmt: { spec: { aksConfig: { imported: true } } }
-  };
-  const importedEksClusterInfo = {
-    clusterName: 'test', provisioner: 'rke2', mgmt: { spec: { eksConfig: { imported: true } } }
-  };
-  const notImportedGkeClusterInfo = {
-    clusterName: 'test', provisioner: 'rke2', mgmt: { spec: { gkeConfig: { imported: false } }, rkeConfig: {} }
-  };
-  const importedClusterInfoWithProviderForEmberParam = {
-    clusterName: 'test', provisioner: 'rke2', mgmt: { providerForEmberParam: 'import' }
-  };
-  const localClusterInfo = {
-    clusterName: 'test', provisioner: 'imported', mgmt: { isLocal: true, spec: { gkeConfig: {} } }, spec: {}
-  };
-  const doRke2Info = {
-    clusterName: 'test', provisioner: 'rke2', mgmt: { isLocal: false, providerForEmberParam: 'import' }, spec: { rkeConfig: {} }
-  };
-
   const gkeClusterWithPrivateEndpoint = {
     clusterName: 'test',
     provisioner: 'GKE',
@@ -76,67 +51,6 @@ describe('class ProvCluster', () => {
     });
   });
 
-  describe('isImported', () => {
-    const testCases = [
-      [importedClusterInfo, true],
-      [importedGkeClusterInfo, true],
-      [importedAksClusterInfo, true],
-      [importedEksClusterInfo, true],
-      [notImportedGkeClusterInfo, false],
-      [importedClusterInfoWithProviderForEmberParam, true],
-      [localClusterInfo, false],
-      [doRke2Info, false],
-      [{}, false],
-    ];
-    const resetMocks = () => {
-      // Clear all mock function calls:
-      jest.clearAllMocks();
-    };
-
-    it.each(testCases)('should return the isImported value properly based on the props data', (clusterData: Object, expected: Boolean) => {
-      const cluster = new ProvCluster({ spec: clusterData.spec });
-
-      jest.spyOn(cluster, 'mgmt', 'get').mockReturnValue(
-        clusterData.mgmt
-      );
-      jest.spyOn(cluster, 'provisioner', 'get').mockReturnValue(
-        clusterData.provisioner
-      );
-
-      expect(cluster.isImported).toBe(expected);
-      resetMocks();
-    }
-    );
-  });
-
-  describe('mgmt', () => {
-    const testCases = [
-      [importedClusterInfo, importedClusterInfo.mgmt],
-      [importedGkeClusterInfo, importedGkeClusterInfo.mgmt],
-      [importedAksClusterInfo, importedAksClusterInfo.mgmt],
-      [importedEksClusterInfo, importedEksClusterInfo.mgmt],
-      [notImportedGkeClusterInfo, notImportedGkeClusterInfo.mgmt],
-      [importedClusterInfoWithProviderForEmberParam, importedClusterInfoWithProviderForEmberParam.mgmt],
-      [localClusterInfo, localClusterInfo.mgmt],
-      [doRke2Info, doRke2Info.mgmt],
-      [{}, null],
-    ];
-
-    const resetMocks = () => {
-      // Clear all mock function calls:
-      jest.clearAllMocks();
-    };
-
-    it.each(testCases)('should return the isImported value properly based on the props data', (clusterData: Object, expected: Object) => {
-      const clusterMock = jest.fn(() => clusterData.mgmt);
-      const ctx = { rootGetters: { 'management/byId': clusterMock } };
-      const cluster = new ProvCluster({ status: { clusterName: clusterData.clusterName } }, ctx);
-
-      expect(cluster.mgmt).toBe(expected);
-      resetMocks();
-    });
-  });
-
   describe('hasError', () => {
     const conditionsWithoutError = [
       {

diff --git a/shell/models/management.cattle.io.cluster.js b/shell/models/management.cattle.io.cluster.js
@@ -89,11 +89,31 @@ export default class MgmtCluster extends SteveModel {
     return pools.filter((x) => x.spec?.clusterName === this.id);
   }
 
-  get provisioner() {
-    if (this.status?.provider ) {
-      return this.status.provider;
+  get isImported() {
+    if (this.isLocal) {
+      return false;
+    }
+    // imported rke2 and k3s have status.driver === rke2 and k3s respectively
+    // Provisioned rke2 and k3s have status.driver === imported
+    if (this.status?.provider === 'k3s' || this.status?.provider === 'rke2') {
+      return this.status?.driver === this.status?.provider;
+    }
+
+    // imported KEv2
+    const kontainerConfigs = ['aksConfig', 'eksConfig', 'gkeConfig'];
+
+    const isImportedKontainer = kontainerConfigs.filter((key) => {
+      return this.spec?.[key]?.imported === true;
+    }).length;
+
+    if (isImportedKontainer) {
+      return true;
     }
 
+    return this.provisioner === 'imported';
+  }
+
+  get provisioner() {
     // For imported K3s clusters, this.status.driver is 'k3s.'
     return this.status?.driver ? this.status.driver : 'imported';
   }
@@ -117,10 +137,11 @@ export default class MgmtCluster extends SteveModel {
   get providerForEmberParam() {
     // Ember wants one word called provider to tell what component to show, but has much indirect mapping to figure out what it is.
     let provider;
-    // Provisioner is the "<something>Config" in the model
+
+    //  provisioner is status.driver
     const provisioner = KONTAINER_TO_DRIVER[(this.provisioner || '').toLowerCase()] || this.provisioner;
 
-    if ( provisioner === 'rancherKubernetesEngine' || provisioner === 'rke') {
+    if ( provisioner === 'rancherKubernetesEngine') {
       // Look for a cloud provider in one of the node templates
       if ( this.machinePools?.[0] ) {
         provider = this.machinePools[0]?.nodeTemplate?.spec?.driver || null;

diff --git a/shell/models/provisioning.cattle.io.cluster.js b/shell/models/provisioning.cattle.io.cluster.js
@@ -282,19 +282,7 @@ export default class ProvCluster extends SteveModel {
   }
 
   get isImported() {
-    // As of Rancher v2.6.7, this returns false for imported K3s clusters,
-    // in which this.provisioner is `k3s`.
-
-    const isImportedProvisioner = this.provisioner === 'imported';
-    const isImportedSpecialCases = this.mgmt?.providerForEmberParam === 'import' ||
-      // when imported cluster is GKE
-      !!this.mgmt?.spec?.gkeConfig?.imported ||
-      // or AKS
-      !!this.mgmt?.spec?.aksConfig?.imported ||
-      // or EKS
-      !!this.mgmt?.spec?.eksConfig?.imported;
-
-    return !this.isLocal && (isImportedProvisioner || (!this.isRke2 && !this.mgmt?.machineProvider && isImportedSpecialCases));
+    return this.mgmt?.isImported;
   }
 
   get isCustom() {
@@ -330,7 +318,8 @@ export default class ProvCluster extends SteveModel {
   }
 
   get isRke1() {
-    return !!this.mgmt?.spec?.rancherKubernetesEngineConfig || this.labels['provider.cattle.io'] === 'rke';
+    // rancherKubernetesEngineConfig is not defined on imported RKE1 clusters
+    return !!this.mgmt?.spec?.rancherKubernetesEngineConfig || this.mgmt?.labels['provider.cattle.io'] === 'rke';
   }
 
   get isHarvester() {
@@ -407,6 +396,8 @@ export default class ProvCluster extends SteveModel {
       provisioner = 'k3s';
     } else if ( this.isImportedRke2 ) {
       provisioner = 'rke2';
+    } else if ((this.isImported || this.isLocal) && this.isRke1) {
+      provisioner = 'rke';
     }
 
     return this.$rootGetters['i18n/withFallback'](`cluster.provider."${ provisioner }"`, null, ucFirst(provisioner));