Merge pull request #31244 from drodil/scaffolder_actions_registry2

fix: distributed actions visibility in scaffolder

.changeset/loud-windows-give.md

@@ -0,0 +1,8 @@
+---
+'@backstage/plugin-scaffolder-backend': patch
+---
+
+Fixed distributed actions not being visible in the scaffolder template actions.
+
+Depending on the plugin startup order, some of the distributed actions were not being registered correctly,
+causing them to be invisible in the scaffolder template actions list.

app-config.yaml

@@ -65,7 +65,9 @@ backend:
       - host: example.com
       - host: '*.mozilla.org'
   # workingDirectory: /tmp # Use this to configure a working directory for the scaffolder, defaults to the OS temp-dir
-
+  actions:
+    pluginSources:
+      - catalog
 # See README.md in the proxy-backend plugin for information on the configuration format
 proxy:
   endpoints:

plugins/scaffolder-backend/src/scaffolder/actions/TemplateActionRegistry.test.ts

@@ -0,0 +1,268 @@
+/*
+ * Copyright 2021 The Backstage Authors
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import { ConflictError, NotFoundError } from '@backstage/errors';
+import { createTemplateAction } from '@backstage/plugin-scaffolder-node';
+import { DefaultTemplateActionRegistry } from './TemplateActionRegistry';
+import { mockCredentials, mockServices } from '@backstage/backend-test-utils';
+import { actionsRegistryServiceMock } from '@backstage/backend-test-utils/alpha';
+
+describe('DefaultTemplateActionRegistry', () => {
+  let registry: DefaultTemplateActionRegistry;
+  let mockActionsService: ReturnType<typeof actionsRegistryServiceMock>;
+  let mockLogger: ReturnType<typeof mockServices.logger.mock>;
+
+  beforeEach(() => {
+    mockActionsService = actionsRegistryServiceMock();
+    mockLogger = mockServices.logger.mock();
+
+    registry = new DefaultTemplateActionRegistry(
+      mockActionsService,
+      mockLogger,
+    );
+  });
+
+  describe('register', () => {
+    it('should register a template action successfully', () => {
+      const action = createTemplateAction({
+        id: 'test-action',
+        description: 'Test action',
+        handler: jest.fn(),
+      });
+
+      expect(() => registry.register(action)).not.toThrow();
+    });
+
+    it('should throw ConflictError when registering action with duplicate ID', () => {
+      const action1 = createTemplateAction({
+        id: 'duplicate-action',
+        description: 'First action',
+        handler: jest.fn(),
+      });
+
+      const action2 = createTemplateAction({
+        id: 'duplicate-action',
+        description: 'Second action',
+        handler: jest.fn(),
+      });
+
+      registry.register(action1);
+
+      expect(() => registry.register(action2)).toThrow(ConflictError);
+      expect(() => registry.register(action2)).toThrow(
+        "Template action with ID 'duplicate-action' has already been registered",
+      );
+    });
+  });
+
+  describe('get', () => {
+    it('should return a registered action', async () => {
+      const action = createTemplateAction({
+        id: 'test-action',
+        description: 'Test action',
+        handler: jest.fn(),
+      });
+
+      registry.register(action);
+      const result = await registry.get('test-action', {
+        credentials: mockCredentials.user(),
+      });
+
+      expect(result).toBe(action);
+    });
+
+    it('should return action from ActionsService', async () => {
+      mockActionsService.register({
+        name: 'service-action',
+        title: 'Service Action',
+        description: 'Service action',
+        schema: {
+          input: z => z.object({}),
+          output: z => z.object({}),
+        },
+        attributes: {
+          readOnly: true,
+          destructive: false,
+        },
+        action: async () => ({ output: {} }),
+      });
+
+      const result = await registry.get('test:service-action', {
+        credentials: mockCredentials.user(),
+      });
+
+      expect(result.id).toBe('test:service-action');
+      expect(result.description).toBe('Service action');
+      expect(result.supportsDryRun).toBe(true);
+    });
+
+    it('should throw NotFoundError when action is not found', async () => {
+      await expect(
+        registry.get('non-existent-action', {
+          credentials: mockCredentials.user(),
+        }),
+      ).rejects.toThrow(NotFoundError);
+      await expect(
+        registry.get('non-existent-action', {
+          credentials: mockCredentials.user(),
+        }),
+      ).rejects.toThrow(
+        "Template action with ID 'non-existent-action' is not registered",
+      );
+    });
+  });
+
+  describe('list', () => {
+    it('should return empty map when no actions are registered', async () => {
+      const result = await registry.list({
+        credentials: mockCredentials.user(),
+      });
+
+      expect(result).toBeInstanceOf(Map);
+      expect(result.size).toBe(0);
+    });
+
+    it('should return all registered actions', async () => {
+      const action1 = createTemplateAction({
+        id: 'action-1',
+        description: 'First action',
+        handler: jest.fn(),
+      });
+
+      const action2 = createTemplateAction({
+        id: 'action-2',
+        description: 'Second action',
+        handler: jest.fn(),
+      });
+
+      registry.register(action1);
+      registry.register(action2);
+
+      const result = await registry.list({
+        credentials: mockCredentials.user(),
+      });
+
+      expect(result.size).toBe(2);
+      expect(result.get('action-1')).toBe(action1);
+      expect(result.get('action-2')).toBe(action2);
+    });
+
+    it('should include actions from ActionsService', async () => {
+      mockActionsService.register({
+        name: 'service-action',
+        title: 'Service Action',
+        description: 'Service action',
+        schema: {
+          input: z => z.object({}),
+          output: z => z.object({}),
+        },
+        attributes: {
+          readOnly: true,
+          destructive: false,
+        },
+        action: async () => ({ output: {} }),
+      });
+
+      const result = await registry.list({
+        credentials: mockCredentials.user(),
+      });
+
+      expect(result.size).toBe(1);
+      const action = result.get('test:service-action');
+      expect(action).toBeDefined();
+      expect(action!.id).toBe('test:service-action');
+      expect(action!.description).toBe('Service action');
+      expect(action!.supportsDryRun).toBe(true);
+    });
+
+    it('should prioritize locally registered actions over service actions', async () => {
+      const localAction = createTemplateAction({
+        id: 'test:same-id',
+        description: 'Local action',
+        handler: jest.fn(),
+      });
+
+      mockActionsService.register({
+        name: 'same-id',
+        title: 'Same ID',
+        description: 'Service action',
+        schema: {
+          input: z => z.object({}),
+          output: z => z.object({}),
+        },
+        action: async () => ({ output: {} }),
+      });
+
+      registry.register(localAction);
+      const result = await registry.list({
+        credentials: mockCredentials.user(),
+      });
+
+      expect(result.get('test:same-id')).toBe(localAction);
+      expect(mockLogger.warn).toHaveBeenCalledWith(
+        "Template action with ID 'test:same-id' has already been registered, skipping action provided by actions service",
+      );
+    });
+
+    it('should set supportsDryRun to false for destructive actions', async () => {
+      mockActionsService.register({
+        name: 'destructive-action',
+        title: 'Destructive Action',
+        description: 'Destructive action',
+        schema: {
+          input: z => z.object({}),
+          output: z => z.object({}),
+        },
+        attributes: {
+          readOnly: false,
+          destructive: true,
+        },
+        action: async () => ({ output: {} }),
+      });
+
+      const result = await registry.list({
+        credentials: mockCredentials.user(),
+      });
+      const action = result.get('test:destructive-action');
+
+      expect(action!.supportsDryRun).toBe(false);
+    });
+
+    it('should set supportsDryRun to false for non-readonly actions', async () => {
+      mockActionsService.register({
+        name: 'non-readonly-action',
+        title: 'Non-readonly Action',
+        description: 'Non-readonly action',
+        schema: {
+          input: z => z.object({}),
+          output: z => z.object({}),
+        },
+        attributes: {
+          readOnly: false,
+          destructive: false,
+        },
+        action: async () => ({ output: {} }),
+      });
+
+      const result = await registry.list({
+        credentials: mockCredentials.user(),
+      });
+      const action = result.get('test:non-readonly-action');
+
+      expect(action!.supportsDryRun).toBe(false);
+    });
+  });
+});

plugins/scaffolder-backend/src/scaffolder/actions/TemplateActionRegistry.ts

@@ -16,13 +16,40 @@
 
 import { ConflictError, NotFoundError } from '@backstage/errors';
 import { TemplateAction } from '@backstage/plugin-scaffolder-node';
+import { ActionsService } from '@backstage/backend-plugin-api/alpha';
+import {
+  BackstageCredentials,
+  LoggerService,
+} from '@backstage/backend-plugin-api';
+import { isPlainObject } from 'lodash';
+import { Schema } from 'jsonschema';
+import { JsonObject } from '@backstage/types';
+
+/**
+ * @internal
+ */
+export interface TemplateActionRegistry {
+  register(action: TemplateAction<any, any, any>): void;
+  get(
+    actionId: string,
+    options: { credentials: BackstageCredentials },
+  ): Promise<TemplateAction<any, any, any>>;
+  list(options: {
+    credentials: BackstageCredentials;
+  }): Promise<Map<string, TemplateAction<any, any, any>>>;
+}
 
 /**
  * Registry of all registered template actions.
  */
-export class TemplateActionRegistry {
+export class DefaultTemplateActionRegistry implements TemplateActionRegistry {
   private readonly actions = new Map<string, TemplateAction>();
 
+  constructor(
+    private readonly actionsRegistry: ActionsService,
+    private readonly logger: LoggerService,
+  ) {}
+
   register(action: TemplateAction<any, any, any>) {
     if (this.actions.has(action.id)) {
       throw new ConflictError(
@@ -33,8 +60,11 @@ export class TemplateActionRegistry {
     this.actions.set(action.id, action);
   }
 
-  get(actionId: string): TemplateAction<any, any, any> {
-    const action = this.actions.get(actionId);
+  async get(
+    actionId: string,
+    options: { credentials: BackstageCredentials },
+  ): Promise<TemplateAction<any, any, any>> {
+    const action = (await this.list(options)).get(actionId);
     if (!action) {
       throw new NotFoundError(
         `Template action with ID '${actionId}' is not registered. See https://backstage.io/docs/features/software-templates/builtin-actions/ on how to add a new action module.`,
@@ -43,7 +73,49 @@ export class TemplateActionRegistry {
     return action;
   }
 
-  list(): TemplateAction<any, any, any>[] {
-    return [...this.actions.values()];
+  async list(options: {
+    credentials: BackstageCredentials;
+  }): Promise<Map<string, TemplateAction<any, any, any>>> {
+    const ret = new Map(this.actions);
+
+    const { actions } = await this.actionsRegistry.list({
+      credentials: options.credentials,
+    });
+
+    for (const action of actions) {
+      if (ret.has(action.id)) {
+        this.logger.warn(
+          `Template action with ID '${action.id}' has already been registered, skipping action provided by actions service`,
+        );
+        continue;
+      }
+
+      ret.set(action.id, {
+        id: action.id,
+        description: action.description,
+        examples: [],
+        supportsDryRun:
+          action.attributes?.readOnly === true &&
+          action.attributes?.destructive === false,
+        handler: async ctx => {
+          const { output } = await this.actionsRegistry.invoke({
+            id: action.id,
+            input: ctx.input,
+            credentials: await ctx.getInitiatorCredentials(),
+          });
+
+          if (isPlainObject(output)) {
+            for (const [key, value] of Object.entries(output as JsonObject)) {
+              ctx.output(key as keyof typeof output, value);
+            }
+          }
+        },
+        schema: {
+          input: action.schema.input as Schema,
+          output: action.schema.output as Schema,
+        },
+      });
+    }
+    return ret;
   }
 }

plugins/scaffolder-backend/src/scaffolder/actions/index.ts

@@ -16,4 +16,7 @@
 
 export * from './builtin';
 
-export { TemplateActionRegistry } from './TemplateActionRegistry';
+export {
+  type TemplateActionRegistry,
+  DefaultTemplateActionRegistry,
+} from './TemplateActionRegistry';

plugins/scaffolder-backend/src/scaffolder/dryrun/DecoratedActionsRegistry.ts

@@ -16,24 +16,46 @@
 
 import { TemplateAction } from '@backstage/plugin-scaffolder-node';
 import { TemplateActionRegistry } from '../actions';
+import { BackstageCredentials } from '@backstage/backend-plugin-api';
 
 /** @internal */
-export class DecoratedActionsRegistry extends TemplateActionRegistry {
+export class DecoratedActionsRegistry implements TemplateActionRegistry {
+  private readonly innerActions: Map<string, TemplateAction> = new Map();
+
   constructor(
     private readonly innerRegistry: TemplateActionRegistry,
     extraActions: Array<TemplateAction>,
   ) {
-    super();
     for (const action of extraActions) {
-      this.register(action);
+      this.innerActions.set(action.id, action);
     }
   }
 
-  get(actionId: string): TemplateAction {
+  async get(
+    actionId: string,
+    options: { credentials: BackstageCredentials },
+  ): Promise<TemplateAction> {
     try {
-      return super.get(actionId);
-    } catch {
-      return this.innerRegistry.get(actionId);
+      return await this.innerRegistry.get(actionId, options);
+    } catch (e) {
+      if (!this.innerActions.has(actionId)) {
+        throw e;
+      }
+      return this.innerActions.get(actionId)!;
     }
   }
+
+  async list(options: {
+    credentials: BackstageCredentials;
+  }): Promise<Map<string, TemplateAction<any, any, any>>> {
+    const inner = await this.innerRegistry.list(options);
+    return new Map<string, TemplateAction<any, any, any>>([
+      ...inner,
+      ...this.innerActions,
+    ]);
+  }
+
+  register(action: TemplateAction<any, any, any>): void {
+    this.innerRegistry.register(action);
+  }
 }

plugins/scaffolder-backend/src/scaffolder/dryrun/createDryRunner.ts

@@ -41,9 +41,9 @@ import fs from 'fs-extra';
 import path from 'path';
 import { fileURLToPath } from 'url';
 import { v4 as uuid } from 'uuid';
-import { TemplateActionRegistry } from '../actions';
 import { NunjucksWorkflowRunner } from '../tasks/NunjucksWorkflowRunner';
 import { DecoratedActionsRegistry } from './DecoratedActionsRegistry';
+import { TemplateActionRegistry } from '../actions';
 
 interface DryRunInput {
   spec: TaskSpec;

plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.test.ts

@@ -15,15 +15,18 @@
  */
 
 import { NunjucksWorkflowRunner } from './NunjucksWorkflowRunner';
-import { TemplateActionRegistry } from '../actions';
+import {
+  DefaultTemplateActionRegistry,
+  TemplateActionRegistry,
+} from '../actions';
 import { ScmIntegrations } from '@backstage/integration';
 import { JsonObject } from '@backstage/types';
 import { ConfigReader } from '@backstage/config';
 import { TaskSpec } from '@backstage/plugin-scaffolder-common';
 import {
   createTemplateAction,
-  TaskSecrets,
   TaskContext,
+  TaskSecrets,
 } from '@backstage/plugin-scaffolder-node';
 import { UserEntity } from '@backstage/catalog-model';
 import {
@@ -36,6 +39,7 @@ import {
   mockCredentials,
   mockServices,
 } from '@backstage/backend-test-utils';
+import { actionsRegistryServiceMock } from '@backstage/backend-test-utils/alpha';
 
 describe('NunjucksWorkflowRunner', () => {
   let actionRegistry: TemplateActionRegistry;
@@ -104,7 +108,10 @@ describe('NunjucksWorkflowRunner', () => {
     // This one is ESM-only
     stripAnsi = await import('strip-ansi').then(m => m.default);
 
-    actionRegistry = new TemplateActionRegistry();
+    actionRegistry = new DefaultTemplateActionRegistry(
+      actionsRegistryServiceMock(),
+      mockServices.logger.mock(),
+    );
     fakeActionHandler = jest.fn();
     fakeTaskLog = jest.fn();
 

plugins/scaffolder-backend/src/scaffolder/tasks/NunjucksWorkflowRunner.ts

@@ -61,8 +61,8 @@ import { createCounterMetric, createHistogramMetric } from '../../util/metrics';
 import { BackstageLoggerTransport, WinstonLogger } from './logger';
 import { convertFiltersToRecord } from '../../util/templating';
 import {
-  CheckpointState,
   CheckpointContext,
+  CheckpointState,
 } from '@backstage/plugin-scaffolder-node/alpha';
 
 type NunjucksWorkflowRunnerOptions = {
@@ -245,7 +245,9 @@ export class NunjucksWorkflowRunner implements WorkflowRunner {
         return;
       }
       const action: TemplateAction<JsonObject> =
-        this.options.actionRegistry.get(step.action);
+        await this.options.actionRegistry.get(step.action, {
+          credentials: await task.getInitiatorCredentials(),
+        });
       const { taskLogger } = createStepLogger({
         task,
         step,

plugins/scaffolder-backend/src/service/router.ts

@@ -26,7 +26,7 @@ import {
   resolveSafeChildPath,
   SchedulerService,
 } from '@backstage/backend-plugin-api';
-import { Schema } from 'jsonschema';
+import { validate } from 'jsonschema';
 import {
   CompoundEntityRef,
   Entity,
@@ -41,10 +41,10 @@ import { ScmIntegrations } from '@backstage/integration';
 import { EventsService } from '@backstage/plugin-events-node';
 
 import {
-  createConditionAuthorizer,
-  createPermissionIntegrationRouter,
-  createConditionTransformer,
   ConditionTransformer,
+  createConditionAuthorizer,
+  createConditionTransformer,
+  createPermissionIntegrationRouter,
 } from '@backstage/plugin-permission-node';
 import {
   TaskSpec,
@@ -53,11 +53,11 @@ import {
 } from '@backstage/plugin-scaffolder-common';
 import {
   RESOURCE_TYPE_SCAFFOLDER_ACTION,
-  RESOURCE_TYPE_SCAFFOLDER_TEMPLATE,
   RESOURCE_TYPE_SCAFFOLDER_TASK,
+  RESOURCE_TYPE_SCAFFOLDER_TEMPLATE,
   scaffolderActionPermissions,
-  scaffolderTaskPermissions,
   scaffolderPermissions,
+  scaffolderTaskPermissions,
   scaffolderTemplatePermissions,
   taskCancelPermission,
   taskCreatePermission,
@@ -67,6 +67,7 @@ import {
 } from '@backstage/plugin-scaffolder-common/alpha';
 import {
   TaskBroker,
+  TaskFilters,
   TaskStatus,
   TemplateAction,
   TemplateFilter,
@@ -80,15 +81,14 @@ import {
 } from '@backstage/plugin-scaffolder-node/alpha';
 import { HumanDuration, JsonObject } from '@backstage/types';
 import express from 'express';
-import { validate } from 'jsonschema';
 import { Duration } from 'luxon';
 import { pathToFileURL } from 'url';
 import { v4 as uuid } from 'uuid';
 import { z } from 'zod';
 import {
   DatabaseTaskStore,
+  DefaultTemplateActionRegistry,
   TaskWorker,
-  TemplateActionRegistry,
 } from '../scaffolder';
 import { createDryRunner } from '../scaffolder/dryrun';
 import { StorageTaskBroker } from '../scaffolder/tasks/StorageTaskBroker';
@@ -115,25 +115,22 @@ import {
 } from '../util/templating';
 import { createDefaultFilters } from '../lib/templating/filters/createDefaultFilters';
 import {
-  ScaffolderPermissionRuleInput,
-  TaskPermissionRuleInput,
-  isTaskPermissionRuleInput,
   ActionPermissionRuleInput,
   isActionPermissionRuleInput,
+  isTaskPermissionRuleInput,
   isTemplatePermissionRuleInput,
+  ScaffolderPermissionRuleInput,
+  TaskPermissionRuleInput,
   TemplatePermissionRuleInput,
 } from './permissions';
 import { CatalogService } from '@backstage/plugin-catalog-node';
 
 import {
   scaffolderActionRules,
-  scaffolderTemplateRules,
   scaffolderTaskRules,
+  scaffolderTemplateRules,
 } from './rules';
-
-import { TaskFilters } from '@backstage/plugin-scaffolder-node';
 import { ActionsService } from '@backstage/backend-plugin-api/alpha';
-import { isPlainObject } from 'lodash';
 
 /**
  * RouterOptions
@@ -272,7 +269,10 @@ export async function createRouter(
     taskBroker = options.taskBroker;
   }
 
-  const actionRegistry = new TemplateActionRegistry();
+  const actionRegistry = new DefaultTemplateActionRegistry(
+    actionsRegistry,
+    logger,
+  );
 
   const templateExtensions = {
     additionalTemplateFilters: convertFiltersToRecord(
@@ -306,48 +306,10 @@ export async function createRouter(
     workers.push(worker);
   }
 
-  // TODO(blam): it's a little unfortunate that you have to restart the scaffolder
-  // backend in order to pick these up. We should really just make `ActionsRegistry.get()` async
-  // and then we can move this logic into the there instead.
-  // But we can't make those changes until next major.
-  // Alternatively, we could look at setting up a periodic task that refreshes the actions registry, but
-  // not feeling that it's worth the complexity.
-  const { actions: distributedActions } = await actionsRegistry.list({
-    credentials: await auth.getOwnServiceCredentials(),
-  });
-
   for (const action of actions) {
     actionRegistry.register(action);
   }
 
-  for (const action of distributedActions) {
-    actionRegistry.register({
-      id: action.id,
-      description: action.description,
-      examples: [],
-      supportsDryRun:
-        action.attributes?.readOnly === true &&
-        action.attributes?.destructive === false,
-      handler: async ctx => {
-        const { output } = await actionsRegistry.invoke({
-          id: action.id,
-          input: ctx.input,
-          credentials: await ctx.getInitiatorCredentials(),
-        });
-
-        if (isPlainObject(output)) {
-          for (const [key, value] of Object.entries(output as JsonObject)) {
-            ctx.output(key as keyof typeof output, value);
-          }
-        }
-      },
-      schema: {
-        input: action.schema.input as Schema,
-        output: action.schema.output as Schema,
-      },
-    });
-  }
-
   const launchWorkers = () => workers.forEach(worker => worker.start());
 
   const shutdownWorkers = async () => {
@@ -479,16 +441,20 @@ export async function createRouter(
         eventId: 'action-fetch',
         request: req,
       });
+      const credentials = await httpAuth.credentials(req);
 
       try {
-        const actionsList = actionRegistry.list().map(action => {
-          return {
-            id: action.id,
-            description: action.description,
-            examples: action.examples,
-            schema: action.schema,
-          };
-        });
+        const list = await actionRegistry.list({ credentials });
+        const actionsList = Array.from(list.values())
+          .map(action => {
+            return {
+              id: action.id,
+              description: action.description,
+              examples: action.examples,
+              schema: action.schema,
+            };
+          })
+          .sort((a, b) => a.id.localeCompare(b.id));
 
         await auditorEvent?.success();