Merge pull request #10018 from backstage/freben/ldap-org-sched

Add a `schedule` parameter to `LdapOrgEntityProvider`

.changeset/many-tools-buy.md

@@ -0,0 +1,5 @@
+---
+'@backstage/backend-tasks': patch
+---
+
+Relaxed the task ID requirement to now support any non-empty string

.changeset/new-books-protect.md

@@ -0,0 +1,30 @@
+---
+'@backstage/plugin-catalog-backend-module-ldap': minor
+---
+
+**BREAKING**: Added a `schedule` field to `LdapOrgEntityProvider.fromConfig`, which is required. If you want to retain the old behavior of scheduling the provider manually, you can set it to the string value `'manual'`. But you may want to leverage the ability to instead pass in the recurring task schedule information directly. This will allow you to simplify your backend setup code to not need an intermediate variable and separate scheduling code at the bottom.
+
+All things said, a typical setup might now look as follows:
+
+```diff
+ // packages/backend/src/plugins/catalog.ts
++import { Duration } from 'luxon';
++import { LdapOrgEntityProvider } from '@backstage/plugin-catalog-backend-module-ldap';
+ export default async function createPlugin(
+   env: PluginEnvironment,
+ ): Promise<Router> {
+   const builder = await CatalogBuilder.create(env);
++  // The target parameter below needs to match the ldap.providers.target
++  // value specified in your app-config.
++  builder.addEntityProvider(
++    LdapOrgEntityProvider.fromConfig(env.config, {
++      id: 'our-ldap-master',
++      target: 'ldaps://ds.example.net',
++      logger: env.logger,
++      schedule: env.scheduler.createScheduledTaskRunner({
++        frequency: Duration.fromObject({ minutes: 60 }),
++        timeout: Duration.fromObject({ minutes: 15 }),
++      }),
++    }),
++  );
+```

.changeset/silent-cats-kneel.md

@@ -2,4 +2,4 @@
 '@backstage/plugin-catalog-backend-module-gitlab': minor
 ---
 
-Added package, moving out gitlab specific functionality from the catalog-backend
+Added package, moving out GitLab specific functionality from the catalog-backend

.changeset/ten-queens-dance.md

@@ -0,0 +1,7 @@
+---
+'@backstage/backend-tasks': minor
+---
+
+**BREAKING**: The `TaskDefinition` type has been removed, and replaced by the equal pair `TaskScheduleDefinition` and `TaskInvocationDefinition`. The interface for `PluginTaskScheduler.scheduleTask` stays effectively unchanged, so this only affects you if you use the actual types directly.
+
+Added the method `PluginTaskScheduler.createTaskSchedule`, which returns a `TaskSchedule` wrapper that is convenient to pass down into classes that want to control their task invocations while the caller wants to retain control of the actual schedule chosen.

docs/integrations/ldap/org.md

@@ -32,55 +32,29 @@ yarn add @backstage/plugin-catalog-backend-module-ldap
 Update the catalog plugin initialization in your backend to add the provider and
 schedule it:
 
-```ts
-// packages/backend/src/plugins/catalog.ts
-import { CatalogBuilder } from '@backstage/plugin-catalog-backend';
-import { Router } from 'express';
-import { PluginEnvironment } from '../types';
-import { Duration } from 'luxon';
-import { LdapOrgEntityProvider } from '@backstage/plugin-catalog-backend-module-ldap';
+```diff
+ // packages/backend/src/plugins/catalog.ts
++import { Duration } from 'luxon';
++import { LdapOrgEntityProvider } from '@backstage/plugin-catalog-backend-module-ldap';
 
-export default async function createPlugin(
-  env: PluginEnvironment,
-): Promise<Router> {
-  // The target parameter below needs to match the ldap.providers.target
-  // value specified in your app-config
-  const ldapEntityProvider = LdapOrgEntityProvider.fromConfig(env.config, {
-    id: 'our-ldap-master',
-    target: 'ldaps://ds.example.net',
-    logger: env.logger,
-  });
+ export default async function createPlugin(
+   env: PluginEnvironment,
+ ): Promise<Router> {
+   const builder = await CatalogBuilder.create(env);
 
-  const builder = await CatalogBuilder.create(env);
-  builder.addEntityProvider(ldapEntityProvider);
-
-  // You can change the refresh interval for the other catalog entries
-  // independently, or just leave the line below out to use the default
-  // refresh interval. Note that this interval does NOT at all affect
-  // the LDAP refresh when using the provider method, which is good!
-  builder.setRefreshIntervalSeconds(100);
-
-  const { processingEngine, router } = await builder.build();
-  await processingEngine.start();
-
-  // Only perform this scheduling after starting the processing engine
-  await env.scheduler.scheduleTask({
-    id: 'refresh_ldap',
-    // frequency sets how often you want to ingest users and groups from
-    // LDAP, in this case every 60 minutes
-    frequency: Duration.fromObject({ minutes: 60 }),
-    timeout: Duration.fromObject({ minutes: 15 }),
-    fn: async () => {
-      try {
-        await ldapEntityProvider.read();
-      } catch (error) {
-        env.logger.error(error);
-      }
-    },
-  });
-
-  return router;
-}
++  // The target parameter below needs to match the ldap.providers.target
++  // value specified in your app-config.
++  builder.addEntityProvider(
++    LdapOrgEntityProvider.fromConfig(env.config, {
++      id: 'our-ldap-master',
++      target: 'ldaps://ds.example.net',
++      logger: env.logger,
++      schedule: env.scheduler.createScheduledTaskRunner({
++        frequency: Duration.fromObject({ minutes: 60 }),
++        timeout: Duration.fromObject({ minutes: 15 }),
++      }),
++    }),
++  );
 ```
 
 After this, you also have to add some configuration in your app-config that

packages/backend-tasks/api-report.md

@@ -11,17 +11,10 @@ import { Logger } from 'winston';
 
 // @public
 export interface PluginTaskScheduler {
-  scheduleTask(task: TaskDefinition): Promise<void>;
-}
-
-// @public
-export interface TaskDefinition {
-  fn: TaskFunction;
-  frequency: Duration;
-  id: string;
-  initialDelay?: Duration;
-  signal?: AbortSignal_2;
-  timeout: Duration;
+  createScheduledTaskRunner(schedule: TaskScheduleDefinition): TaskRunner;
+  scheduleTask(
+    task: TaskScheduleDefinition & TaskInvocationDefinition,
+  ): Promise<void>;
 }
 
 // @public
@@ -29,6 +22,25 @@ export type TaskFunction =
   | ((abortSignal: AbortSignal_2) => void | Promise<void>)
   | (() => void | Promise<void>);
 
+// @public
+export interface TaskInvocationDefinition {
+  fn: TaskFunction;
+  id: string;
+  signal?: AbortSignal_2;
+}
+
+// @public
+export interface TaskRunner {
+  run(task: TaskInvocationDefinition): Promise<void>;
+}
+
+// @public
+export interface TaskScheduleDefinition {
+  frequency: Duration;
+  initialDelay?: Duration;
+  timeout: Duration;
+}
+
 // @public
 export class TaskScheduler {
   constructor(databaseManager: DatabaseManager, logger: Logger);

packages/backend-tasks/src/tasks/PluginTaskSchedulerImpl.test.ts

@@ -59,4 +59,31 @@ describe('PluginTaskManagerImpl', () => {
       60_000,
     );
   });
+
+  // This is just to test the wrapper code; most of the actual tests are in
+  // TaskWorker.test.ts
+  describe('createScheduledTaskRunner', () => {
+    it.each(databases.eachSupportedId())(
+      'can run the happy path, %p',
+      async databaseId => {
+        const { manager } = await init(databaseId);
+
+        const fn = jest.fn();
+        await manager
+          .createScheduledTaskRunner({
+            timeout: Duration.fromMillis(5000),
+            frequency: Duration.fromMillis(5000),
+          })
+          .run({
+            id: 'task1',
+            fn,
+          });
+
+        await waitForExpect(() => {
+          expect(fn).toBeCalled();
+        });
+      },
+      60_000,
+    );
+  });
 });

packages/backend-tasks/src/tasks/PluginTaskSchedulerImpl.ts

@@ -17,7 +17,12 @@
 import { Knex } from 'knex';
 import { Logger } from 'winston';
 import { TaskWorker } from './TaskWorker';
-import { PluginTaskScheduler, TaskDefinition } from './types';
+import {
+  PluginTaskScheduler,
+  TaskInvocationDefinition,
+  TaskRunner,
+  TaskScheduleDefinition,
+} from './types';
 import { validateId } from './util';
 
 /**
@@ -29,7 +34,9 @@ export class PluginTaskSchedulerImpl implements PluginTaskScheduler {
     private readonly logger: Logger,
   ) {}
 
-  async scheduleTask(task: TaskDefinition): Promise<void> {
+  async scheduleTask(
+    task: TaskScheduleDefinition & TaskInvocationDefinition,
+  ): Promise<void> {
     validateId(task.id);
 
     const knex = await this.databaseFactory();
@@ -47,4 +54,12 @@ export class PluginTaskSchedulerImpl implements PluginTaskScheduler {
       },
     );
   }
+
+  createScheduledTaskRunner(schedule: TaskScheduleDefinition): TaskRunner {
+    return {
+      run: async task => {
+        await this.scheduleTask({ ...task, ...schedule });
+      },
+    };
+  }
 }

packages/backend-tasks/src/tasks/index.ts

@@ -17,6 +17,8 @@
 export { TaskScheduler } from './TaskScheduler';
 export type {
   PluginTaskScheduler,
-  TaskDefinition,
   TaskFunction,
+  TaskInvocationDefinition,
+  TaskRunner,
+  TaskScheduleDefinition,
 } from './types';

packages/backend-tasks/src/tasks/types.ts

@@ -31,27 +31,11 @@ export type TaskFunction =
   | (() => void | Promise<void>);
 
 /**
- * Options that apply to the invocation of a given task.
+ * Options that control the scheduling of a task.
  *
  * @public
  */
-export interface TaskDefinition {
-  /**
-   * A unique ID (within the scope of the plugin) for the task.
-   */
-  id: string;
-
-  /**
-   * The actual task function to be invoked regularly.
-   */
-  fn: TaskFunction;
-
-  /**
-   * An abort signal that, when triggered, will stop the recurring execution of
-   * the task.
-   */
-  signal?: AbortSignal;
-
+export interface TaskScheduleDefinition {
   /**
    * The maximum amount of time that a single task invocation can take, before
    * it's considered timed out and gets "released" such that a new invocation
@@ -91,6 +75,43 @@ export interface TaskDefinition {
   initialDelay?: Duration;
 }
 
+/**
+ * Options that apply to the invocation of a given task.
+ *
+ * @public
+ */
+export interface TaskInvocationDefinition {
+  /**
+   * A unique ID (within the scope of the plugin) for the task.
+   */
+  id: string;
+
+  /**
+   * The actual task function to be invoked regularly.
+   */
+  fn: TaskFunction;
+
+  /**
+   * An abort signal that, when triggered, will stop the recurring execution of
+   * the task.
+   */
+  signal?: AbortSignal;
+}
+
+/**
+ * A previously prepared task schedule, ready to be invoked.
+ *
+ * @public
+ */
+export interface TaskRunner {
+  /**
+   * Takes the schedule and executes an actual task using it.
+   *
+   * @param task - The actual runtime properties of the task
+   */
+  run(task: TaskInvocationDefinition): Promise<void>;
+}
+
 /**
  * Deals with the scheduling of distributed tasks, for a given plugin.
  *
@@ -99,15 +120,34 @@ export interface TaskDefinition {
 export interface PluginTaskScheduler {
   /**
    * Schedules a task function for coordinated exclusive invocation across
-   * workers.
+   * workers. This convenience method performs both the scheduling and
+   * invocation in one go.
+   *
+   * @remarks
    *
    * If the task was already scheduled since before by us or by another party,
    * its options are just overwritten with the given options, and things
    * continue from there.
    *
-   * @param definition - The task definition
+   * @param task - The task definition
    */
-  scheduleTask(task: TaskDefinition): Promise<void>;
+  scheduleTask(
+    task: TaskScheduleDefinition & TaskInvocationDefinition,
+  ): Promise<void>;
+
+  /**
+   * Creates a scheduled but dormant recurring task, ready to be launched at a
+   * later time.
+   *
+   * @remarks
+   *
+   * This method is useful for pre-creating a schedule in outer code to be
+   * passed into an inner implementation, such that the outer code controls
+   * scheduling while inner code controls implementation.
+   *
+   * @param schedule - The task schedule
+   */
+  createScheduledTaskRunner(schedule: TaskScheduleDefinition): TaskRunner;
 }
 
 function isValidOptionalDurationString(d: string | undefined): boolean {

packages/backend-tasks/src/tasks/util.test.ts

@@ -20,14 +20,14 @@ import { delegateAbortController, sleep, validateId } from './util';
 
 describe('util', () => {
   describe('validateId', () => {
-    it.each(['a', 'a_b', 'ab123c_2'])(
+    it.each(['a', 'a_b', 'ab123c_2', 'a!', 'A', 'a-b', 'a.b', '_a', 'a_'])(
       'accepts valid inputs, %p',
       async input => {
         expect(validateId(input)).toBeUndefined();
       },
     );
 
-    it.each(['', 'a!', 'A', 'a-b', 'a.b', '_a', 'a_', null, Symbol('a')])(
+    it.each(['', null, Symbol('a')])(
       'rejects invalid inputs, %p',
       async input => {
         expect(() => validateId(input as any)).toThrow();

packages/backend-tasks/src/tasks/util.ts

@@ -19,12 +19,10 @@ import { Knex } from 'knex';
 import { DateTime, Duration } from 'luxon';
 import { AbortController, AbortSignal } from 'node-abort-controller';
 
-// Keep the IDs compatible with e.g. Prometheus
+// Keep the IDs compatible with e.g. Prometheus labels
 export function validateId(id: string) {
-  if (typeof id !== 'string' || !/^[a-z0-9]+(?:_[a-z0-9]+)*$/.test(id)) {
-    throw new InputError(
-      `${id} is not a valid ID, expected string of lowercase characters and digits separated by underscores`,
-    );
+  if (typeof id !== 'string' || !id.trim()) {
+    throw new InputError(`${id} is not a valid ID, expected non-empty string`);
   }
 }
 

plugins/catalog-backend-module-ldap/api-report.md

@@ -15,6 +15,7 @@ import { LocationSpec } from '@backstage/plugin-catalog-backend';
 import { Logger } from 'winston';
 import { SearchEntry } from 'ldapjs';
 import { SearchOptions } from 'ldapjs';
+import { TaskRunner } from '@backstage/backend-tasks';
 import { UserEntity } from '@backstage/catalog-model';
 
 // @public
@@ -106,17 +107,21 @@ export class LdapOrgEntityProvider implements EntityProvider {
   // (undocumented)
   static fromConfig(
     configRoot: Config,
-    options: {
-      id: string;
-      target: string;
-      userTransformer?: UserTransformer;
-      groupTransformer?: GroupTransformer;
-      logger: Logger;
-    },
+    options: LdapOrgEntityProviderOptions,
   ): LdapOrgEntityProvider;
   // (undocumented)
   getProviderName(): string;
-  read(): Promise<void>;
+  read(options?: { logger?: Logger }): Promise<void>;
+}
+
+// @public
+export interface LdapOrgEntityProviderOptions {
+  groupTransformer?: GroupTransformer;
+  id: string;
+  logger: Logger;
+  schedule: 'manual' | TaskRunner;
+  target: string;
+  userTransformer?: UserTransformer;
 }
 
 // @public

plugins/catalog-backend-module-ldap/package.json

@@ -33,6 +33,7 @@
     "start": "backstage-cli package start"
   },
   "dependencies": {
+    "@backstage/backend-tasks": "^0.1.10",
     "@backstage/catalog-model": "^0.12.0",
     "@backstage/config": "^0.1.15",
     "@backstage/errors": "^0.2.2",
@@ -41,6 +42,7 @@
     "@types/ldapjs": "^2.2.0",
     "ldapjs": "^2.2.0",
     "lodash": "^4.17.21",
+    "uuid": "^8.0.0",
     "winston": "^3.2.1"
   },
   "devDependencies": {

plugins/catalog-backend-module-ldap/src/processors/LdapOrgEntityProvider.ts

@@ -14,6 +14,7 @@
  * limitations under the License.
  */
 
+import { TaskRunner } from '@backstage/backend-tasks';
 import {
   ANNOTATION_LOCATION,
   ANNOTATION_ORIGIN_LOCATION,
@@ -25,6 +26,7 @@ import {
   EntityProviderConnection,
 } from '@backstage/plugin-catalog-backend';
 import { merge } from 'lodash';
+import * as uuid from 'uuid';
 import { Logger } from 'winston';
 import {
   GroupTransformer,
@@ -36,6 +38,59 @@ import {
   UserTransformer,
 } from '../ldap';
 
+/**
+ * Options for {@link LdapOrgEntityProvider}.
+ *
+ * @public
+ */
+export interface LdapOrgEntityProviderOptions {
+  /**
+   * A unique, stable identifier for this provider.
+   *
+   * @example "production"
+   */
+  id: string;
+
+  /**
+   * The target that this provider should consume.
+   *
+   * Should exactly match the "target" field of one of the "ldap.providers"
+   * configuration entries.
+   *
+   * @example "ldaps://ds-read.example.net"
+   */
+  target: string;
+
+  /**
+   * The logger to use.
+   */
+  logger: Logger;
+
+  /**
+   * The refresh schedule to use.
+   *
+   * @remarks
+   *
+   * If you pass in 'manual', you are responsible for calling the `read` method
+   * manually at some interval.
+   *
+   * But more commonly you will pass in the result of
+   * {@link @backstage/backend-tasks#PluginTaskScheduler.createScheduledTaskRunner}
+   * to enable automatic scheduling of tasks.
+   */
+  schedule: 'manual' | TaskRunner;
+
+  /**
+   * The function that transforms a user entry in LDAP to an entity.
+   */
+  userTransformer?: UserTransformer;
+
+  /**
+   * The function that transforms a group entry in LDAP to an entity.
+   */
+  groupTransformer?: GroupTransformer;
+}
+
 /**
  * Reads user and group entries out of an LDAP service, and provides them as
  * User and Group entities for the catalog.
@@ -49,35 +104,11 @@ import {
  */
 export class LdapOrgEntityProvider implements EntityProvider {
   private connection?: EntityProviderConnection;
+  private scheduleFn?: () => Promise<void>;
 
   static fromConfig(
     configRoot: Config,
-    options: {
-      /**
-       * A unique, stable identifier for this provider.
-       *
-       * @example "production"
-       */
-      id: string;
-      /**
-       * The target that this provider should consume.
-       *
-       * Should exactly match the "target" field of one of the "ldap.providers"
-       * configuration entries.
-       *
-       * @example "ldaps://ds-read.example.net"
-       */
-      target: string;
-      /**
-       * The function that transforms a user entry in LDAP to an entity.
-       */
-      userTransformer?: UserTransformer;
-      /**
-       * The function that transforms a group entry in LDAP to an entity.
-       */
-      groupTransformer?: GroupTransformer;
-      logger: Logger;
-    },
+    options: LdapOrgEntityProviderOptions,
   ): LdapOrgEntityProvider {
     // TODO(freben): Deprecate the old catalog.processors.ldapOrg config
     const config =
@@ -101,13 +132,17 @@ export class LdapOrgEntityProvider implements EntityProvider {
       target: options.target,
     });
 
-    return new LdapOrgEntityProvider({
+    const result = new LdapOrgEntityProvider({
       id: options.id,
       provider,
       userTransformer: options.userTransformer,
       groupTransformer: options.groupTransformer,
       logger,
     });
+
+    result.schedule(options.schedule);
+
+    return result;
   }
 
   constructor(
@@ -128,18 +163,20 @@ export class LdapOrgEntityProvider implements EntityProvider {
   /** {@inheritdoc @backstage/plugin-catalog-backend#EntityProvider.connect} */
   async connect(connection: EntityProviderConnection) {
     this.connection = connection;
+    await this.scheduleFn?.();
   }
 
   /**
-   * Runs one complete ingestion loop. Call this method regularly at some
-   * appropriate cadence.
+   * Runs one single complete ingestion. This is only necessary if you use
+   * manual scheduling.
    */
-  async read() {
+  async read(options?: { logger?: Logger }) {
     if (!this.connection) {
       throw new Error('Not initialized');
     }
 
-    const { markReadComplete } = trackProgress(this.options.logger);
+    const logger = options?.logger ?? this.options.logger;
+    const { markReadComplete } = trackProgress(logger);
 
     // Be lazy and create the client each time; even though it's pretty
     // inefficient, we usually only do this once per entire refresh loop and
@@ -157,7 +194,7 @@ export class LdapOrgEntityProvider implements EntityProvider {
       {
         groupTransformer: this.options.groupTransformer,
         userTransformer: this.options.userTransformer,
-        logger: this.options.logger,
+        logger,
       },
     );
 
@@ -173,6 +210,32 @@ export class LdapOrgEntityProvider implements EntityProvider {
 
     markCommitComplete();
   }
+
+  private schedule(schedule: LdapOrgEntityProviderOptions['schedule']) {
+    if (schedule === 'manual') {
+      return;
+    }
+
+    this.scheduleFn = async () => {
+      const id = `${this.getProviderName()}:refresh`;
+      await schedule.run({
+        id,
+        fn: async () => {
+          const logger = this.options.logger.child({
+            class: LdapOrgEntityProvider.prototype.constructor.name,
+            taskId: id,
+            taskInstanceId: uuid.v4(),
+          });
+
+          try {
+            await this.read({ logger });
+          } catch (error) {
+            logger.error(error);
+          }
+        },
+      });
+    };
+  }
 }
 
 // Helps wrap the timing and logging behaviors

plugins/catalog-backend-module-ldap/src/processors/index.ts

@@ -15,4 +15,5 @@
  */
 
 export { LdapOrgEntityProvider } from './LdapOrgEntityProvider';
+export type { LdapOrgEntityProviderOptions } from './LdapOrgEntityProvider';
 export { LdapOrgReaderProcessor } from './LdapOrgReaderProcessor';