(core): crossRegionReference resources cirmcumvent Aspects application

[rv2673]

Describe the bug

I am very happy that we now can add cross region references without jumping through hoop thanks to: #22008
I however have ran into issue with it.
The feature makes it so that is adds a custom resource for the cross region reference when resolving references. This however circumvents defined Aspects. We have defined Aspects for Tags and a PermissionBoundary for our resources which are are enforced in the target account. However the aspects are not run for the added resources.

Expected Behavior

Aspects are applied to the cross region reference resources.

Current Behavior

Aspects are not applied to the cross region references resources.

Reproduction Steps

Example with permissionboundary and cross region waf for cloudfront, which can be placed in cdk init project.
Permissionboundary is not applied to all created Iam roles.

#!/usr/bin/env node
import {
  App,
  Stack,
  aws_cloudfront as cloudfront,
  aws_cloudfront_origins as origins,
  aws_iam as iam,
  aws_wafv2 as waf,
} from "aws-cdk-lib";

const app = new App();
const stack1 = new Stack(app, "Stack1", {
  env: {
    region: "us-east-1",
  },
  crossRegionReferences: true,
});
const boundary = iam.ManagedPolicy.fromAwsManagedPolicyName("AdministratorAccess");

const wacl = new waf.CfnWebACL(stack1, "waf", {
  defaultAction: { allow: {} },
  scope: "Cloudfront",
  visibilityConfig: {
    sampledRequestsEnabled: false,
    cloudWatchMetricsEnabled: false,
    metricName: "none",
  },
});
const stack2 = new Stack(app, "Stack2", {
  env: {
    region: "eu-central-1",
  },
  crossRegionReferences: true,
});
new cloudfront.Distribution(stack2, "Distribution", {
  defaultBehavior: {
    origin: new origins.HttpOrigin("example.com"),
  },
  webAclId: wacl.attrId,
});
iam.PermissionsBoundary.of(app).apply(boundary);

Possible Solution

Probable cause:
This is probably because aspects are applied here:

aws-cdk/packages/@aws-cdk/core/lib/private/synthesis.ts

Line 30 in b20a6b4

invokeAspects(root);

And the crossRegionReferences path under prepareApp on line 35 add the resources after which the aspects are not (re)applied.

aws-cdk/packages/@aws-cdk/core/lib/private/synthesis.ts

Line 35 in b20a6b4

prepareApp(root);

Possible solution
ResolveReferences before invokeAspects in addition to after.
- Move function for applying aspects to separate file(to prevent circular references)

aws-cdk/packages/@aws-cdk/core/lib/private/synthesis.ts

Line 102 in 738922b

function invokeAspects(root: IConstruct) {

- Invoke aspect application on crossRegion construct creation with the aspects inherited from the stack here and here or in the getOrCreate methods on those same classes(since it only needs to be called when creating the resources not on each export registration).

~~ or ~~
- reinvoke aspect application after resolving references, though not sure if it can be asummed that aspect application can be repeated.

Additional Information/Context

No response

CDK CLI Version

2.50.0

Framework Version

2.50.0

Node.js Version

16

OS

Ubuntu

Language

Typescript

Language Version

No response

Other information

No response

[rix0rrr]

Thanks for bringing this up! As you already found out, there are some issues around aspects and last-minute modification of the construct tree. I'm honestly not quite sure if this is solveable or not... we'll have to think on this a little.

[rv2673]

The issue is caused because Apects are expected to be the last modifiers before synthesis. However during resolving references there are currently modifications done(besides the cross region part), exports and cfnparameters for example.

I am not sure if Aspects are idempotent outside the invokeAspects call which keep track of aspects that are run to prevent duplicates. Resolve references is already called multiple times, so maybe the following might work.

We resolve references(and its modifications) before aspects are applied. After that we apply the aspects after which we do another pass of resolving references, in case they add references. If this second pass results in modifications aspects are not applied, but that is similar to when aspects apply other aspects, which results in warning since they are not applied.

[rv2673]

It look like adding an extra invocation to resolveReferences(root); before

aws-cdk/packages/@aws-cdk/core/lib/private/synthesis.ts

Line 30 in b20a6b4

invokeAspects(root);

produces the desired behaviour.
Though some more testing is required. Will try to setup proper dev environment later this week.

[rv2673]

I did some testing(which was tricky since not all tests are stable/succesful on main and all packages depend on the core package).

I tried adding a call to resolveReferences(root); before invokeAspect, this does seem to result in expected behaviour, but also results in errors for aws-batch, aws-eks, aws-kinesis-destinations, pipelines. Placing the call after invokeAspects but before prepareApp does not result in errors, so maybe these packages do depend on some kind of interaction between applying aspects(or a certain aspect) and references.

[rv2673]

Placing resolveReferences before aspect invocation results in issues with DependsOn and with Lazy references using Aspect results. So while it does solve aspect application on constructs added during reference resolving it breaks references depending on aspect application. So reference resolving cannot be done before and after aspects like I originally thought.

So to get the desired behaviour we need to solve the aspect invocation on constructs added after/during aspect invocation.

I see 2 possible solutions:

1. Try to apply aspects multiple times

We return invokedByPath mapping from invokeAspects + count of constructs we used aspect.visit on. If we add optional parameter to invokeAspects we can prefill this mapping to prevent multiple invocations of an aspect on a construct.

That way we can call invokeAspects multiple times until zero nodes are visited, that way aspects are applied to constructs that are added during aspect invocation.
We can also call it again after reference resolving during synth process to apply it to constructs added during that phase.

Potential problems:

• While it does not invoke aspects multiple times, it does walk the tree multiple times while skipping over nodes.
• Possibly unbounded number of walks, if aspect keeps adding construct below another construct on each walk, though can be mitigated.
• Aspects added during other Aspects do get applied when calling it multiple times. Not sure if that behavior is desirable, since currently an explicit warning is added if it detects that.

2. Add callback to apply aspects when node is added to tree after aspect invocation

This would require change to constructs.
Add method to Construct class to register callback(s) that is/are called by child construct after it is initialized(since aspects application requires node.path to exist.

This would allow for aspect application to register a callback on the nodes it has visited that immediately applies the already applied aspect(s) if a construct is added to the tree after(during reference resolving) and/or during aspect invocation.

I think option 2 would be the best, though I might be missing something. @rix0rrr what do you think?

[rv2673]

@rix0rrr Do you think above PR might be an acceptable solution?

[tomeh100]

is there a workaround to include the permission boundary to the auto generated role used by the customer resource lambda?

[crushton-reapit]

@rv2673 did you ever find/release a fix for this?

[rv2673]

@crushton-reapit No, I got no response on PR's and it got autoclosed. Eventually we replicated the behaviour of the feature with wrapper around attributes we wanted to use cross-region, but very verbose an finicky.

[rv2673]

is there a workaround to include the permission boundary to the auto generated role used by the customer resource lambda?

@tomeh100 As far as I can tell no. The role is added to construct tree during reference resolving which runs after any user code and aspects have been run.

So the only way I can think of modifying the created cloudassembly, template, assets etc on the file system after the synthesis.

[btan-godaddy]

@rv2673 Hey Richard, may I know how do you replicate the behavior of the feature with wrapper around attributes? Thanks!

[rv2673]

@rv2673 Hey Richard, may I know how do you replicate the behavior of the feature with wrapper around attributes? Thanks!

@btan-godaddy this is it.

import { Stack, aws_ssm as ssm, Token, PhysicalName } from 'aws-cdk-lib';
import { IStringParameter } from 'aws-cdk-lib/aws-ssm';
import {
  AwsCustomResource,
  AwsCustomResourcePolicy,
  AwsSdkCall,
} from 'aws-cdk-lib/custom-resources';
import { Construct, IConstruct } from 'constructs';

export interface ICustomReferenceWrapper {
  getValue(scope: IConstruct): string;
}

const getTopLevelStack = (construct: IConstruct) => {
  let stack = Stack.of(construct);
  while (stack && stack.nested && stack.node.scope) {
    stack = Stack.of(stack.node.scope);
  }
  return stack;
};

export class CustomReferenceWrapper
  extends Construct
  implements ICustomReferenceWrapper
{
  private ssmParam?: ssm.IStringParameter;

  private id: string;

  private value: string;

  public readonly region: string;

  public readonly account: string;

  public readonly stack: Stack;

  constructor(scope: IConstruct, id: string, props: { value: string }) {
    super(scope, id);
    this.id = `/${this.node.path}`;
    this.stack = getTopLevelStack(this);
    this.region = this.stack.region;
    this.account = this.stack.account;
    this.value = props.value;
  }

  private getOrCreateSSMParam(): IStringParameter {
    if (!this.ssmParam) {
      this.ssmParam = new ssm.StringParameter(this.stack, this.id, {
        parameterName: PhysicalName.GENERATE_IF_NEEDED,
        simpleName: true,
        stringValue: this.value,
      });
    }
    return this.ssmParam;
  }

  public getValue(scope: IConstruct): string {
    const importStack = Stack.of(scope);
    const importRegion = importStack.region;
    if (!Token.isUnresolved(this.value)) {
      return this.value;
    }
    const node = importStack.node.tryFindChild(this.id);
    if (
      node instanceof CustomReferenceWrapper ||
      (importRegion === this.region && importStack.account === this.account)
    ) {
      // No need for cross region/account reference so let cdk handle reference
      return this.value;
    }
    if (node instanceof CustomReferenceWrapper.CrossRegionReferenceReader) {
      // All ready used within the stack return same reference
      return node.getParameterValue();
    }
    const ssmParam = this.getOrCreateSSMParam();
    const reader = new CustomReferenceWrapper.CrossRegionReferenceReader(
      importStack,
      this.id,
      {
        parameterName: ssmParam.parameterName,
        region: this.region,
      }
    );
    importStack.addDependency(this.stack);
    return reader.getParameterValue();
  }

  private static CrossRegionReferenceReader = class extends AwsCustomResource {
    constructor(
      scope: IConstruct,
      name: string,
      props: { parameterName: string; region: string }
    ) {
      const ssmAwsSdkCall: AwsSdkCall = {
        service: 'SSM',
        action: 'getParameter',
        parameters: {
          Name: props.parameterName,
        },
        region: props.region,
        physicalResourceId: {
          id: Date.now().toString(), // Update physical id to always fetch the latest version
        },
      };

      super(scope, name, {
        onUpdate: ssmAwsSdkCall,
        policy: AwsCustomResourcePolicy.fromSdkCalls({
          resources: AwsCustomResourcePolicy.ANY_RESOURCE,
        }),
        installLatestAwsSdk: false,
      });
    }

    public getParameterValue(): string {
      return this.getResponseField('Parameter.Value').toString();
    }
  };
}

Usage

this.certificateArn = new CustomReferenceWrapper(
  this.certificate,
  'certificateArn',
  {
	value: this.certificate.certificateArn,
  }
).getValue(this);

The value from this.certificateArn can now be used almost anywhere(unless it create dependency cycles of course). Downside is that every attribute needs to be wrapped explicitly. It also doesn't create hard depedencies like crossRegion feature has. It are weak dependencies, so in theorie you could delete resource being depended on without removing usage, though in some cases this will break or fail to deploy.

[rix0rrr]

Unfortunately the above is a result of the design around Aspects, which makes the following guarantees:

• Aspects are invoked on a construct only after the Aspect has been invoked on all of its children
• Because Aspects can add resources that lead to cross-stack references, Aspects run before cross-stack references are calculated.

The upshot of that is two downsides:

• Aspects cannot (always) reflect on constructs created by other Aspects, because there is a deterministic run order.
• Aspects cannot reflect on constructs added later in the synthesis life cycle (like the custom resources created by cross-region references).

---

We cannot fix the ordering problems without breaking the guarantee that Aspect execution is ordered. That's a breaking proposition: it might be that nobody is depending on the execution order, it might be that many users are depending on the execution order, and it might be that users are depending on the execution order without knowing it.

There are two solutions I can think of, not perfect but workable:

Iterating to a fixpoint: a new type of Aspect returns true or false depending on whether it did a mutation. We repeat applying all Aspects until all of them return false, indicating that no more work needs to be done. Has performance implications, can potentially either get into an infinite loop or miss updates if the Aspect is not carefully coded.

Reactive Aspects: either a new type of Aspect, or an Aspect explicitly marked as "order-agnostic", will get invoked on any new nodes added to the construct tree, in whatever order they happen to be added in. Invoking can happen instantly as the construct is added in a sort of onAdd event, or in passes (invoke all aspects on all nodes, then invoke all aspects on new nodes, repeat until no new nodes). Since this doesn't require modifying the visit() code itself, this gives us the ability to switch this mode on for all aspects automatically, or all aspects in a certain scope. This method is also more efficient. The downside is that it will catch "construct adds", but not "construct mutations".

Anyone any other ideas I might have missed?

[danw-mpl]

Just ran into this one. I'm using an Aspect to apply a Permissions Boundary which doesn't get applied to the IAM Role created by crossRegionReference.

[rix0rrr]

Duplicate of #27780

[github-actions]

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.