From 89466b812ecba0713d71bb2d948521e3175a8ce3 Mon Sep 17 00:00:00 2001 From: Syed Hussain Date: Wed, 29 May 2024 00:15:07 +0000 Subject: [PATCH 1/3] Refactor EventManager to prevent cyclic dependencies - Modify EventManager.addTargetToEvent to accept a scope parameter - Get or create event rules within the provided scope - This ensures that event rules are created in the caller's scope instead of the eventManager's scope - Add tests to verify the behavior of EventManager with different configurations --- src/control-plane/billing/billing-provider.ts | 2 +- src/control-plane/control-plane.ts | 2 + src/core-app-plane/core-app-plane.ts | 6 +- src/utils/event-manager.ts | 25 ++- test/core-app-plane.test.ts | 1 - test/event-manager.test.ts | 195 ++++++++++++++++++ 6 files changed, 218 insertions(+), 13 deletions(-) create mode 100644 test/event-manager.test.ts diff --git a/src/control-plane/billing/billing-provider.ts b/src/control-plane/billing/billing-provider.ts index e10fb219..6d8d894b 100644 --- a/src/control-plane/billing/billing-provider.ts +++ b/src/control-plane/billing/billing-provider.ts @@ -153,6 +153,6 @@ export class BillingProvider extends Construct { } const { handler, trigger } = this.getFunctionProps(fn, defaultEvent); - eventManager.addTargetToEvent(trigger, new event_targets.LambdaFunction(handler)); + eventManager.addTargetToEvent(this, trigger, new event_targets.LambdaFunction(handler)); } } diff --git a/src/control-plane/control-plane.ts b/src/control-plane/control-plane.ts index eb19cc03..0942e755 100644 --- a/src/control-plane/control-plane.ts +++ b/src/control-plane/control-plane.ts @@ -88,11 +88,13 @@ export class ControlPlane extends Construct { this.controlPlaneAPIGatewayUrl = controlPlaneAPI.apiUrl; this.eventManager.addTargetToEvent( + this, DetailType.PROVISION_SUCCESS, controlPlaneAPI.tenantUpdateServiceTarget ); this.eventManager.addTargetToEvent( + this, DetailType.DEPROVISION_SUCCESS, controlPlaneAPI.tenantUpdateServiceTarget ); diff --git a/src/core-app-plane/core-app-plane.ts b/src/core-app-plane/core-app-plane.ts index 3be60285..c26c4b91 100644 --- a/src/core-app-plane/core-app-plane.ts +++ b/src/core-app-plane/core-app-plane.ts @@ -133,7 +133,11 @@ export class CoreApplicationPlane extends Construct { bashJobRunner: job, }); - this.eventManager.addTargetToEvent(jobRunnerProps.incomingEvent, jobOrchestrator.eventTarget); + this.eventManager.addTargetToEvent( + this, + jobRunnerProps.incomingEvent, + jobOrchestrator.eventTarget + ); }); } } diff --git a/src/utils/event-manager.ts b/src/utils/event-manager.ts index dafdabdb..6c36c95d 100644 --- a/src/utils/event-manager.ts +++ b/src/utils/event-manager.ts @@ -171,7 +171,7 @@ export interface IEventManager { * @param eventType The detail type of the event to add a target to. * @param target The target that will be added to the event. */ - addTargetToEvent(eventType: DetailType, target: IRuleTarget): void; + addTargetToEvent(scope: Construct, eventType: DetailType, target: IRuleTarget): void; /** * Provides grantee the permissions to place events @@ -218,8 +218,6 @@ export class EventManager extends Construct implements IEventManager { */ public readonly busArn: string; - private readonly connectedRules: Map = new Map(); - constructor(scope: Construct, id: string, props?: EventManagerProps) { super(scope, id); addTemplateTag(this, 'EventManager'); @@ -275,25 +273,32 @@ export class EventManager extends Construct implements IEventManager { /** * Adds an IRuleTarget to an event. * + * @param scope The scope in which to find (or create) the Rule. * @param eventType The detail type of the event to add a target to. * @param target The target that will be added to the event. */ - public addTargetToEvent(eventType: DetailType, target: IRuleTarget): void { - this.getOrCreateRule(eventType).addTarget(target); + public addTargetToEvent(scope: Construct, eventType: DetailType, target: IRuleTarget): void { + this.getOrCreateRule(scope, eventType).addTarget(target); } - private getOrCreateRule(eventType: DetailType): Rule { - let rule = this.connectedRules.get(eventType); - + /** + * Returns a Rule for the given eventType in the context of a scope. + * A new one is created not found in a scope. + * + * @param scope The scope in which to find (or create) the Rule. + * @param eventType The detail type of the event to add a target to. + * @returns A Rule for the given eventType in the provided scope. + */ + private getOrCreateRule(scope: Construct, eventType: DetailType): Rule { + let rule = scope.node.tryFindChild(`${eventType}Rule`) as Rule; if (!rule) { - rule = new Rule(this, `${eventType}Rule`, { + rule = new Rule(scope, `${eventType}Rule`, { eventBus: this.eventBus, eventPattern: { source: [this.supportedEvents[eventType]], detailType: [eventType], }, }); - this.connectedRules.set(eventType, rule); } return rule; diff --git a/test/core-app-plane.test.ts b/test/core-app-plane.test.ts index bbeb570d..210f11d5 100644 --- a/test/core-app-plane.test.ts +++ b/test/core-app-plane.test.ts @@ -173,7 +173,6 @@ describe('CoreApplicationPlane', () => { const template = Template.fromStack(coreApplicationPlaneStack); template.hasResourceProperties('AWS::CodeBuild::Project', { - // check that codebuild has the MY_TEST_ENV_VAR environment variable defined Environment: Match.objectLike({ EnvironmentVariables: Match.anyValue(), }), diff --git a/test/event-manager.test.ts b/test/event-manager.test.ts new file mode 100644 index 00000000..a79d848f --- /dev/null +++ b/test/event-manager.test.ts @@ -0,0 +1,195 @@ +// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +import * as cdk from 'aws-cdk-lib'; +import { Capture, Template } from 'aws-cdk-lib/assertions'; +import { Effect, PolicyDocument, PolicyStatement } from 'aws-cdk-lib/aws-iam'; +import { AwsSolutionsChecks } from 'cdk-nag'; +import { CognitoAuth, ControlPlane } from '../src/control-plane'; +import { CoreApplicationPlane } from '../src/core-app-plane'; +import { DetailType, EventManager } from '../src/utils'; + +function testForDuplicateRulesInCoreAppPlaneStack(stack: cdk.Stack) { + const template = Template.fromStack(stack); + const eventRuleCapture = new Capture(); + + template.allResourcesProperties('AWS::Events::Rule', eventRuleCapture); + const eventRules: { [key: string]: any } = []; + do { + eventRules.push(eventRuleCapture.asObject()); + } while (eventRuleCapture.next()); + + const onboardingRequestEventRules = eventRules.filter( + (eventRule: { EventPattern: { 'detail-type': string[] } }) => { + return eventRule.EventPattern['detail-type'][0] === DetailType.ONBOARDING_REQUEST; + } + ); + + it('should not have duplicate rules for the same event detail type', () => { + expect(onboardingRequestEventRules).toHaveLength(1); + }); + + it('should reuse the same rules for targets triggered by the same event detail type', () => { + expect(onboardingRequestEventRules[0].Targets).toHaveLength(2); + }); +} + +function testForRulesInOtherStack(stack: cdk.Stack) { + const template = Template.fromStack(stack); + const eventRuleCapture = new Capture(); + + const rules = template.findResources('AWS::Events::Rule', eventRuleCapture); + console.log(rules); + if (Object.keys(rules).length === 0) { + it('should not have rules for triggering runners in stacks that do not define them', () => { + expect(true).toBe(true); + }); + // return as there are no rules in this stack + return; + } + + // template.allResourcesProperties('AWS::Events::Rule', eventRuleCapture); + const eventRules: { [key: string]: any } = []; + do { + eventRules.push(eventRuleCapture.asObject()); + } while (eventRuleCapture.next()); + + console.log(eventRules); + const onboardingRequestEventRules = eventRules.filter( + (eventRule: { EventPattern?: { 'detail-type': string[] } }) => { + return ( + eventRule.EventPattern && + eventRule.EventPattern['detail-type'][0] === DetailType.ONBOARDING_REQUEST + ); + } + ); + + it('should not have rules for triggering runners in stacks that do not define them', () => { + expect(onboardingRequestEventRules).toHaveLength(0); + }); +} + +const samplePolicyDocument = new PolicyDocument({ + statements: [ + new PolicyStatement({ + actions: ['cloudformation:CreateStack'], + resources: ['arn:aws:cloudformation:*:*:stack/MyStack/ExampleStack'], + effect: Effect.ALLOW, + }), + ], +}); + +const jobsUsingTheSameIncomingEvent = [ + { + name: 'provisioning-1', + outgoingEvent: DetailType.PROVISION_SUCCESS, + incomingEvent: DetailType.ONBOARDING_REQUEST, + permissions: samplePolicyDocument, + script: '', + }, + { + name: 'provisioning-2', + outgoingEvent: DetailType.PROVISION_SUCCESS, + incomingEvent: DetailType.ONBOARDING_REQUEST, + permissions: samplePolicyDocument, + script: '', + }, +]; + +describe('EventManager', () => { + describe('using default event-manager created by control plane', () => { + const app = new cdk.App(); + const controlPlaneStack = new cdk.Stack(app, 'ControlPlaneStack'); + + const cognitoAuth = new CognitoAuth(controlPlaneStack, 'CognitoAuth', { + systemAdminEmail: 'test@example.com', + }); + + const controlPlane = new ControlPlane(controlPlaneStack, 'ControlPlane', { + auth: cognitoAuth, + }); + + const coreAppPlaneStack = new cdk.Stack(app, 'CoreApplicationPlaneStack'); + new CoreApplicationPlane(coreAppPlaneStack, 'CoreApplicationPlane', { + eventManager: controlPlane.eventManager, + jobRunnerPropsList: jobsUsingTheSameIncomingEvent, + }); + + cdk.Aspects.of(controlPlaneStack).add(new AwsSolutionsChecks({ verbose: true })); + cdk.Aspects.of(coreAppPlaneStack).add(new AwsSolutionsChecks({ verbose: true })); + + it('should synth without errors', () => { + const assembly = app.synth(); + expect(assembly).toBeTruthy(); + }); + + testForDuplicateRulesInCoreAppPlaneStack(coreAppPlaneStack); + testForRulesInOtherStack(controlPlaneStack); + }); + + describe('using event-manager created outside of control plane', () => { + const app = new cdk.App(); + const controlPlaneStack = new cdk.Stack(app, 'ControlPlaneStack'); + + const eventManager = new EventManager(controlPlaneStack, 'EventManager'); + + const cognitoAuth = new CognitoAuth(controlPlaneStack, 'CognitoAuth', { + systemAdminEmail: 'test@example.com', + }); + + new ControlPlane(controlPlaneStack, 'ControlPlane', { + auth: cognitoAuth, + eventManager: eventManager, + }); + + const coreAppPlaneStack = new cdk.Stack(app, 'CoreApplicationPlaneStack'); + new CoreApplicationPlane(coreAppPlaneStack, 'CoreApplicationPlane', { + eventManager: eventManager, + jobRunnerPropsList: jobsUsingTheSameIncomingEvent, + }); + + cdk.Aspects.of(controlPlaneStack).add(new AwsSolutionsChecks({ verbose: true })); + cdk.Aspects.of(coreAppPlaneStack).add(new AwsSolutionsChecks({ verbose: true })); + + it('should synth without errors', () => { + const assembly = app.synth(); + expect(assembly).toBeTruthy(); + }); + + testForDuplicateRulesInCoreAppPlaneStack(coreAppPlaneStack); + testForRulesInOtherStack(controlPlaneStack); + }); + + describe('using an event-manager created in a separate stack', () => { + const app = new cdk.App(); + const eventManagerStack = new cdk.Stack(app, 'EventManagerStack'); + const eventManager = new EventManager(eventManagerStack, 'EventManager'); + + const controlPlaneStack = new cdk.Stack(app, 'ControlPlaneStack'); + const cognitoAuth = new CognitoAuth(controlPlaneStack, 'CognitoAuth', { + systemAdminEmail: 'test@example.com', + }); + + new ControlPlane(controlPlaneStack, 'ControlPlane', { + auth: cognitoAuth, + eventManager: eventManager, + }); + + const coreAppPlaneStack = new cdk.Stack(app, 'CoreApplicationPlaneStack'); + new CoreApplicationPlane(coreAppPlaneStack, 'CoreApplicationPlane', { + eventManager: eventManager, + jobRunnerPropsList: jobsUsingTheSameIncomingEvent, + }); + cdk.Aspects.of(controlPlaneStack).add(new AwsSolutionsChecks({ verbose: true })); + cdk.Aspects.of(coreAppPlaneStack).add(new AwsSolutionsChecks({ verbose: true })); + + it('should synth without errors', () => { + const assembly = app.synth(); + expect(assembly).toBeTruthy(); + }); + + testForDuplicateRulesInCoreAppPlaneStack(coreAppPlaneStack); + testForRulesInOtherStack(controlPlaneStack); + testForRulesInOtherStack(eventManagerStack); + }); +}); From 8cdf07261011ae5669e500dcbf6079279c893502 Mon Sep 17 00:00:00 2001 From: Syed Hussain Date: Wed, 29 May 2024 00:19:47 +0000 Subject: [PATCH 2/3] update API.md --- API.md | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/API.md b/API.md index f477cc22..6489d87a 100644 --- a/API.md +++ b/API.md @@ -1102,11 +1102,19 @@ Returns a string representation of this construct. ##### `addTargetToEvent` ```typescript -public addTargetToEvent(eventType: DetailType, target: IRuleTarget): void +public addTargetToEvent(scope: Construct, eventType: DetailType, target: IRuleTarget): void ``` Adds an IRuleTarget to an event. +###### `scope`Required + +- *Type:* constructs.Construct + +The scope in which to find (or create) the Rule. + +--- + ###### `eventType`Required - *Type:* DetailType @@ -3510,11 +3518,17 @@ The table containing the aggregated data. ##### `addTargetToEvent` ```typescript -public addTargetToEvent(eventType: DetailType, target: IRuleTarget): void +public addTargetToEvent(scope: Construct, eventType: DetailType, target: IRuleTarget): void ``` Adds an IRuleTarget to an event. +###### `scope`Required + +- *Type:* constructs.Construct + +--- + ###### `eventType`Required - *Type:* DetailType From 3067541e163bf663e38c5bc75943006c675903c1 Mon Sep 17 00:00:00 2001 From: Syed Hussain Date: Wed, 29 May 2024 00:22:43 +0000 Subject: [PATCH 3/3] update docs --- src/utils/event-manager.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/utils/event-manager.ts b/src/utils/event-manager.ts index 6c36c95d..2e70ee0a 100644 --- a/src/utils/event-manager.ts +++ b/src/utils/event-manager.ts @@ -283,7 +283,7 @@ export class EventManager extends Construct implements IEventManager { /** * Returns a Rule for the given eventType in the context of a scope. - * A new one is created not found in a scope. + * A new one is created if the rule is not found in the scope. * * @param scope The scope in which to find (or create) the Rule. * @param eventType The detail type of the event to add a target to.