feat: Extracts forwarder stack

[pistazie]

In order to configure multiple regions, one has to configure the Forwarder stack in each region. This is currently not supported as:

• You can't opt out of installing the Forwarder stack
• You can't separately use the stack in order to deploy it in additional regions.

This PR is a POC of separating concerns and adding an opt-out option for the forwarder.

If this approach is acceptable, I'm happy to add the rest of the stack parameters, test cases, and documentation.

wdyt?

[blimmer]

Hey @pistazie - thanks for your PR. Sure enough, there's not currently a good way to do this, as the forwarder stack is inside the Construct. I think that your approach of separating out the stack will work.

If I understand the requirements from Datadog's side, we need to install all of the DD stacks into one region. Let's call this the "home region". That means that the home region will get:

• Datadog policy macro (if using)
• Integration role stack
• Forwarding stack

Then, each additional region needs just the forwarder stack.

Current Approach

If that's correct, with your recommended approach, I think a user would do something like this:

#!/usr/bin/env node
import { App, Stack, StackProps } from 'aws-cdk-lib';
import { Secret } from 'aws-cdk-lib/aws-secretsmanager';
import { DatadogIntegration, DatadogForwarder } from 'cdk-datadog-integration';

const app = new App();

class MonitoringStack extends Stack {
  private homeRegion = 'us-east-1';

  constructor(scope: App, props: Required<Pick<StackProps, 'env'>>) {
    super(scope, 'MonitoringStack', props);
    const { region } = props.env;
    const externalId = 'foobar';

    if (region === this.homeRegion) {
      new DatadogIntegration(this, 'Datadog', {
        apiKey: new Secret(this, 'DatadogSecret', {}),
        externalId,
      })
    } else {
      // It might make sense be nice if this extended a `Construct` instead of `cdk.CfnStack`, like DatadogIntegration.
      new DatadogForwarder(this, 'DatadogForwarder', {
        apiKey: new Secret(this, 'DatadogSecret', {}),
        externalId,
      })
    }
  }
}

new MonitoringStack(app, { env: { account: '1', region: 'us-east-1' }}); // Creates 3 stacks
new MonitoringStack(app, { env: { account: '1', region: 'us-west-2' }}); // Creates forwarder
new MonitoringStack(app, { env: { account: '1', region: 'us-east-2' }}); // Creates forwarder

I think this would work, but it exposes the implementation details to the user. Additionally, would we need to add the installDatadogForwarder boolean if we go this route? I don't think it's strictly necessary if I understand the setup docs properly.

Alternative Idea

Instead of making the user implement this if statement based on their home region, what if we encapsulated everything into the existing API?

We could add a new optional parameter to the DatadogIntegrationConfig interface:

interface DatadogIntegrationConfig {
  // all existing properties...

  readonly multiRegionConfig?: {
    /** 
     * The region the stack is currently being deployed to.
     * Since DatadogIntegration is a region-agnostic construct, we'd have to have the user tell us
     */
    currentStackRegion: string;
  
    /** The region that gets all three stacks. */
    homeRegion: string;
  };
}

Then the DatadogIntegration construct would know what to do differently when in the home region or not.

constructor(
  scope: Construct,
  id: string,
  props: DatadogIntegrationConfig,
) {
  super(scope, id);

  if (props.multiRegionConfig) {
    const { currentStackRegion, homeRegion } = props.multiRegionConfig;

    if (homeRegion === currentStackRegion) {
      // set up everything
    } else {
      // set up forwarder only
    }
  }
}

Then the user's MonitoringStack would be a bit cleaner:

#!/usr/bin/env node
import { App, Stack, StackProps } from 'aws-cdk-lib';
import { Secret } from 'aws-cdk-lib/aws-secretsmanager';
import { DatadogIntegration } from 'cdk-datadog-integration';

const app = new App();

class MonitoringStack extends Stack {
  constructor(scope: App, props: Required<Pick<StackProps, 'env'>>) {
    super(scope, 'MonitoringStack', props);
    const externalId = 'foobar';

    new DatadogIntegration(this, 'Datadog', {
      apiKey: new Secret(this, 'DatadogSecret', {}),
      externalId,
      multiRegionConfig: {
        homeRegion: 'us-east-1',
        currentRegion: props.env.region,
      },
    })
  }
}

new MonitoringStack(app, { env: { account: '1', region: 'us-east-1' }}); // Creates 3 stacks
new MonitoringStack(app, { env: { account: '1', region: 'us-west-2' }}); // Creates forwarder
new MonitoringStack(app, { env: { account: '1', region: 'us-east-2' }}); // Creates forwarder

As a future improvement, we could even do something fancy like replicate the secret from the home region for the user.

Thoughts?

What do you think? Thanks again for taking the time to contribute back to this project.

[pistazie]

Hi,

Interesting stuff. I like the multi-home region configuration.

I have two reservations:

• I find the facts, that MonitoringStack is very different according to the region rather confusing because I expect a stack to have a defined use-case and not change most of it's components according to such a condition.
• As much as I understand this means that each region will have a MonitoringStack which only has a nested stack and no other resources/stacks.

How about the following Idea:

// bin/cdk.ts

const app = new cdk.App()

datadogMonitoringStacks(app, {
     homeRegion : 'us-east-1', 
     additionalRegions: ['us-west-1', 'eu-central-1']
})  
// adds one stack of two nested stacks in us-east-1 (macro, integration)
// adds one forwarder stack in us-east-1 
// adds one forwarder stack per `additionalRegion` 

This way, there aren't superfluous nested stacks, the library is usable for forwarder only, and each region has explicitly a forwarder stack.

wdyt?

[blimmer]

I find the facts, that MonitoringStack is very different according to the region rather confusing because I expect a stack to have a defined use-case and not change most of it's components according to such a condition.

Sure, I can definitely understand your concern there. My counter-argument is that, as a user, I don't really care about what goes into the Datadog setup working. Sure, under the hood it's deploying some different components, but I just know "Datadog works in the regions I care about".

That said, in either case, I think we should allow importing and creating each individual CfnStack if you want to customize how any of this works. As you saw, having those CfnStack definitions in private class methods required you to bring up this issue.

As much as I understand this means that each region will have a MonitoringStack which only has a nested stack and no other resources/stacks.

In the example of the monitoring stack, we actually need a wrapping stack to do the Secrets Manager secret import/creation. I've also seen a lot of folks that have other monitoring setup they need to do in each region (like CloudWatch billing alarms).

In general, I'm not tied to requiring folks to create a top-level monitoring stack. I was mostly using it as an example to think through how people would interact with the library with this change.

How about the following idea:

// bin/cdk.ts

const app = new cdk.App()

datadogMonitoringStacks(app, {
     homeRegion : 'us-east-1', 
     additionalRegions: ['us-west-1', 'eu-central-1']
})  
// adds one stack of two nested stacks in us-east-1 (macro, integration)
// adds one forwarder stack in us-east-1 
// adds one forwarder stack per `additionalRegion` 

Again, the main issue here is regarding the API key secret. You'd need to have the definition (or import) of the key associated with a different stack and passed in as a cross-stack reference.

Also, I'm not a huge fan of the "function that creates Stacks approach". In general, that doesn't feel like it follows common patterns I see in CDK core or other CDK libraries.

How to proceed

Again, thank you for your time and willingness to discuss this with me 😄

I think we should proceed with extracting the CfnStack as you originally proposed. This will allow people to keep working with the library while we consider the larger changes we're discussing.

As mentioned before, I don't think we need to provide the installDatadogForwarder boolean parameter, which will limit the scope of changes.

Then, we can circle back on the larger homeRegion ideas we've been talking about.

What do you think?

[Hawxy]

Just to throw in my two cents, I would like the ability to skip the forwarder stack, we've configured everything to go through Kinesis and don't need it.