Merge pull request #22589 from jamieklassen/unique-cluster-names

Checks for unique cluster names

.changeset/great-rats-collect.md

@@ -0,0 +1,7 @@
+---
+'@backstage/plugin-kubernetes-backend': minor
+---
+
+**BREAKING** The backend will fail to start if two clusters in the app-config
+have the same name. The requirement for unique names has been declared in the
+docs for some time, but is now enforced.

.changeset/tiny-donuts-drive.md

@@ -0,0 +1,6 @@
+---
+'@backstage/plugin-kubernetes-backend': patch
+---
+
+Backstage will log a warning whenever duplicate cluster names are detected --
+even if clusters sharing the same name come from separate locators.

plugins/kubernetes-backend/src/cluster-locator/ConfigClusterLocator.test.ts

@@ -405,4 +405,25 @@ describe('ConfigClusterLocator', () => {
       `Invalid cluster 'cluster1': mock error`,
     );
   });
+
+  it('fails on duplicate cluster names', () => {
+    const config: Config = new ConfigReader({
+      clusters: [
+        {
+          name: 'cluster',
+          url: 'url',
+          authProvider: 'authProvider',
+        },
+        {
+          name: 'cluster',
+          url: 'url',
+          authProvider: 'authProvider',
+        },
+      ],
+    });
+
+    expect(() => ConfigClusterLocator.fromConfig(config, authStrategy)).toThrow(
+      `Duplicate cluster name 'cluster'`,
+    );
+  });
 });

plugins/kubernetes-backend/src/cluster-locator/ConfigClusterLocator.ts

@@ -35,12 +35,17 @@ export class ConfigClusterLocator implements KubernetesClustersSupplier {
     config: Config,
     authStrategy: AuthenticationStrategy,
   ): ConfigClusterLocator {
+    const clusterNames = new Set();
     return new ConfigClusterLocator(
       config.getConfigArray('clusters').map(c => {
         const authMetadataBlock = c.getOptional<{
           [ANNOTATION_KUBERNETES_AUTH_PROVIDER]?: string;
         }>('authMetadata');
         const name = c.getString('name');
+        if (clusterNames.has(name)) {
+          throw new Error(`Duplicate cluster name '${name}'`);
+        }
+        clusterNames.add(name);
         const authProvider =
           authMetadataBlock?.[ANNOTATION_KUBERNETES_AUTH_PROVIDER] ??
           c.getOptionalString('authProvider');

plugins/kubernetes-backend/src/cluster-locator/index.test.ts

@@ -14,6 +14,7 @@
  * limitations under the License.
  */
 
+import { getVoidLogger } from '@backstage/backend-common';
 import { Config, ConfigReader } from '@backstage/config';
 import { CatalogApi } from '@backstage/catalog-client';
 import { ANNOTATION_KUBERNETES_AUTH_PROVIDER } from '@backstage/plugin-kubernetes-common';
@@ -60,6 +61,7 @@ describe('getCombinedClusterSupplier', () => {
       config,
       catalogApi,
       mockStrategy,
+      getVoidLogger(),
     );
     const result = await clusterSupplier.getClusters();
 
@@ -99,9 +101,64 @@ describe('getCombinedClusterSupplier', () => {
         config,
         catalogApi,
         new DispatchStrategy({ authStrategyMap: {} }),
+        getVoidLogger(),
       ),
     ).toThrow(
       new Error('Unsupported kubernetes.clusterLocatorMethods: "magic"'),
     );
   });
+
+  it('logs a warning when two clusters have the same name', async () => {
+    const logger = getVoidLogger();
+    const warn = jest.spyOn(logger, 'warn');
+    const config: Config = new ConfigReader(
+      {
+        kubernetes: {
+          clusterLocatorMethods: [
+            {
+              type: 'config',
+              clusters: [
+                { name: 'cluster', url: 'url', authProvider: 'authProvider' },
+              ],
+            },
+            { type: 'catalog' },
+          ],
+        },
+      },
+      'ctx',
+    );
+    const mockStrategy: jest.Mocked<AuthenticationStrategy> = {
+      getCredential: jest.fn(),
+      validateCluster: jest.fn().mockReturnValue([]),
+      presentAuthMetadata: jest.fn(),
+    };
+    catalogApi = {
+      getEntities: jest.fn().mockResolvedValue({
+        items: [{ metadata: { annotations: {}, name: 'cluster' } }],
+      }),
+      getEntitiesByRefs: jest.fn(),
+      queryEntities: jest.fn(),
+      getEntityAncestors: jest.fn(),
+      getEntityByRef: jest.fn(),
+      removeEntityByUid: jest.fn(),
+      refreshEntity: jest.fn(),
+      getEntityFacets: jest.fn(),
+      getLocationById: jest.fn(),
+      getLocationByRef: jest.fn(),
+      addLocation: jest.fn(),
+      removeLocationById: jest.fn(),
+      getLocationByEntity: jest.fn(),
+      validateEntity: jest.fn(),
+    };
+
+    const clusterSupplier = getCombinedClusterSupplier(
+      config,
+      catalogApi,
+      mockStrategy,
+      logger,
+    );
+    await clusterSupplier.getClusters();
+
+    expect(warn).toHaveBeenCalledWith(`Duplicate cluster name 'cluster'`);
+  });
 });

plugins/kubernetes-backend/src/cluster-locator/index.ts

@@ -14,21 +14,25 @@
  * limitations under the License.
  */
 
+import { CatalogApi } from '@backstage/catalog-client';
 import { Config } from '@backstage/config';
 import { Duration } from 'luxon';
+import { Logger } from 'winston';
 import { ClusterDetails, KubernetesClustersSupplier } from '../types/types';
 import { AuthenticationStrategy } from '../auth/types';
 import { ConfigClusterLocator } from './ConfigClusterLocator';
 import { GkeClusterLocator } from './GkeClusterLocator';
 import { CatalogClusterLocator } from './CatalogClusterLocator';
-import { CatalogApi } from '@backstage/catalog-client';
 import { LocalKubectlProxyClusterLocator } from './LocalKubectlProxyLocator';
 
 class CombinedClustersSupplier implements KubernetesClustersSupplier {
-  constructor(readonly clusterSuppliers: KubernetesClustersSupplier[]) {}
+  constructor(
+    readonly clusterSuppliers: KubernetesClustersSupplier[],
+    readonly logger: Logger,
+  ) {}
 
   async getClusters(): Promise<ClusterDetails[]> {
-    return await Promise.all(
+    const clusters = await Promise.all(
       this.clusterSuppliers.map(supplier => supplier.getClusters()),
     )
       .then(res => {
@@ -37,6 +41,23 @@ class CombinedClustersSupplier implements KubernetesClustersSupplier {
       .catch(e => {
         throw e;
       });
+    return this.warnDuplicates(clusters);
+  }
+
+  private warnDuplicates(clusters: ClusterDetails[]): ClusterDetails[] {
+    const clusterNames = new Set<string>();
+    const duplicatedNames = new Set<string>();
+    for (const clusterName of clusters.map(c => c.name)) {
+      if (clusterNames.has(clusterName)) {
+        duplicatedNames.add(clusterName);
+      } else {
+        clusterNames.add(clusterName);
+      }
+    }
+    for (const clusterName of duplicatedNames) {
+      this.logger.warn(`Duplicate cluster name '${clusterName}'`);
+    }
+    return clusters;
   }
 }
 
@@ -44,6 +65,7 @@ export const getCombinedClusterSupplier = (
   rootConfig: Config,
   catalogClient: CatalogApi,
   authStrategy: AuthenticationStrategy,
+  logger: Logger,
   refreshInterval: Duration | undefined = undefined,
 ): KubernetesClustersSupplier => {
   const clusterSuppliers = rootConfig
@@ -72,5 +94,5 @@ export const getCombinedClusterSupplier = (
       }
     });
 
-  return new CombinedClustersSupplier(clusterSuppliers);
+  return new CombinedClustersSupplier(clusterSuppliers, logger);
 };

plugins/kubernetes-backend/src/service/KubernetesBuilder.ts

@@ -243,6 +243,7 @@ export class KubernetesBuilder {
       config,
       this.env.catalogApi,
       new DispatchStrategy({ authStrategyMap: this.getAuthStrategyMap() }),
+      this.env.logger,
       refreshInterval,
     );