Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(aws-cdk-lib): Property overrides in CDK Aspects and CfnParameter #19447

Closed
avanderm opened this issue Mar 17, 2022 · 1 comment
Closed

(aws-cdk-lib): Property overrides in CDK Aspects and CfnParameter #19447

avanderm opened this issue Mar 17, 2022 · 1 comment
Assignees
Labels
aws-cdk-lib Related to the aws-cdk-lib package bug This issue is a bug. p1

Comments

@avanderm
Copy link

avanderm commented Mar 17, 2022

General Issue

CDK Aspects used to modify properties will lead to odd behaviour when the original value was a CfnReference

The Question

Consider the following CDK Aspect, a simplified version we use to modify names of resources according to a naming convention:

import {
  CfnResource,
  IAspect,
  Stack,
  Token,
} from 'aws-cdk-lib';
import { CfnRole } from 'aws-cdk-lib/aws-iam';
import { IConstruct } from 'constructs';

function hasOwnProperty<X extends {}, Y extends PropertyKey>
  (obj X, prop: Y): obj is X & Record<Y, unknown> {
    return obj.hasOwnProperty(prop)
  }

function isRef(name: string | object) {
  return typeof name === 'object' && this.hasOwnProperty(name, 'Ref');
}

function isJoin(name: string | object) {
  return typeof name === 'object' && this.hasOwnProperty(name, 'Fn::Join');
}

export class MyAspect implements IAspect {
  public visit(node: IConstruct): void {
    if (CfnResource.isCfnResource(node) && node.cfnResourceType == CfnRole.CFN_RESOURCE_TYPE_NAME) {
      const role = node as CfnRole;
      const name = Stack.of(node).resolve(role.roleName) || node.logicalId;

      const newName = [ 
        'my',
        this.isRef(name) || this.isJoin(name) ? Token.asString(name) : name, 
      ].join('-');

      node.addPropertyOverride('RoleName', newName);
    }
  }
}

And the following stack we apply the aspect on:

import { CfnParameter, Fn, Stack, StackProps } from 'aws-cdk-lib';
import { Role, ServicePrincipal } from 'aws-cdk-lib/aws-iam';
import { Construct } from 'constructs';

export class AspectsReferencesStack extends Stack {
  constructor(scope: Construct, id: string, props?: StackProps) {
    super(scope, id, props);

    const parameter = new CfnParameter(this, 'MyParameter', {
      type: 'String',
    }).valueAsString;

    new Role(this, 'MyRole1', {
      assumedBy: new ServicePrincipal('codebuild.amazonaws.com'),
    });

    new Role(this, 'MyRole2', {
      assumedBy: new ServicePrincipal('codebuild.amazonaws.com'),
      roleName: 'a-role',
    });

    new Role(this, 'MyRole3', {
      assumedBy: new ServicePrincipal('codebuild.amazonaws.com'),
      roleName: parameter,
    });

    new Role(this, 'MyRole4', {
      assumedBy: new ServicePrincipal('codebuild.amazonaws.com'),
      roleName: `a-${parameter}`,
    });
  }
}

What you will see is that most results are as expected and produce valid CloudFormation. However, the third role will contain invalid code, and the RoleName property will be:

RoleName:
  Ref: MyParameter
  Fn::Join:
    - ""
    - - my-
      - Ref: MyParameter

The Ref key will always be present, which is odd considering the override of the property. I've tried various tricks, such as deletion overrides on RoleName.Ref and more, but this additional field always seems to persist and just sit alongside the Fn::Join resulting from the interpolation in the aspect. I realize this is a fringe case, and the reason why I'm even considering a CfnParameter is because we use this to in combination with the ProductStack of the service catalog module to provide input fields when provisioning.

What happens under the hood in addPropertyOverride? Does it simply do some sort of object merge?

CDK CLI Version

2.10.0 (build e5b301f)

Framework Version

No response

Node.js Version

v16.13.0

OS

Fedora release 35 (Thirty Five)

Language

Typescript

Language Version

TypeScript (3.9.7)

Other information

No response

@avanderm avanderm added guidance Question that needs advice or information. needs-triage This issue or PR still needs to be triaged. labels Mar 17, 2022
@avanderm avanderm changed the title (aws-cdk-lib): Property overrides in CDK Aspects and references (aws-cdk-lib): Property overrides in CDK Aspects and CfnParameter Mar 17, 2022
@peterwoodworth peterwoodworth added bug This issue is a bug. p1 and removed guidance Question that needs advice or information. labels Mar 17, 2022
@peterwoodworth peterwoodworth added aws-cdk-lib Related to the aws-cdk-lib package and removed needs-triage This issue or PR still needs to be triaged. labels Mar 17, 2022
@corymhall corymhall self-assigned this Jun 1, 2022
mergify bot pushed a commit that referenced this issue Jun 3, 2022
…20608)

When you add a property override, we merge it with any properties set
when the construct is initialized. The merge happens _after_ tokens are
resolved. For example, if we created a resource:

```ts
const resource = new CfnResource(this, 'Resource', {
  type: 'MyResourceType',
  properties: {
    prop1: Fn.ref('Param'),
  },
});
```

And the added an override for `prop1`:

```ts
resource.addPropertyOverride('prop1', Fn.ref('OtherParam'));
```

We would then perform a deep merge on the properties with a priority on
the overrides. The above example would work fine, eventually the merge
would get to the point where it was comparing two objects:

```
target = { prop1: { Ref: 'Param' } }
source = { prop1: { Ref: 'OtherParam' } }
result = { prop1: { Ref: 'OtherParam' } }
```

If we instead added an override using a different intrinsic:

```ts
resource.addPropertyOverride('prop1', Fn.join('-', ['hello',
Fn.ref('Param')]));
```

Then we would get to the point in the merge where we were comparing two
different objects:

```
target = { prop1: { Ref: 'Param' } }
source = { prop1: { 'Fn::Join': ['-', ['hello', { Ref: 'Param' } ]] } }
result = { prop1: {
  Ref: 'Param',
  'Fn::Join': ['-', ['hello', { Ref: 'Param' } ]] },
}}
```

This PR solves this issue by filtering out any CloudFormation
intrinsics. If the merge finds an intrinsic key in the target it "drops" the
intrinsic and takes whatever value is in the source (override).

fix #19971, #19447


----

### All Submissions:

* [ ] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md)

### Adding new Unconventional Dependencies:

* [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies)

### New Features

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)?

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@github-actions
Copy link

github-actions bot commented Jun 3, 2022

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

daschaa pushed a commit to daschaa/aws-cdk that referenced this issue Jul 9, 2022
…ws#20608)

When you add a property override, we merge it with any properties set
when the construct is initialized. The merge happens _after_ tokens are
resolved. For example, if we created a resource:

```ts
const resource = new CfnResource(this, 'Resource', {
  type: 'MyResourceType',
  properties: {
    prop1: Fn.ref('Param'),
  },
});
```

And the added an override for `prop1`:

```ts
resource.addPropertyOverride('prop1', Fn.ref('OtherParam'));
```

We would then perform a deep merge on the properties with a priority on
the overrides. The above example would work fine, eventually the merge
would get to the point where it was comparing two objects:

```
target = { prop1: { Ref: 'Param' } }
source = { prop1: { Ref: 'OtherParam' } }
result = { prop1: { Ref: 'OtherParam' } }
```

If we instead added an override using a different intrinsic:

```ts
resource.addPropertyOverride('prop1', Fn.join('-', ['hello',
Fn.ref('Param')]));
```

Then we would get to the point in the merge where we were comparing two
different objects:

```
target = { prop1: { Ref: 'Param' } }
source = { prop1: { 'Fn::Join': ['-', ['hello', { Ref: 'Param' } ]] } }
result = { prop1: {
  Ref: 'Param',
  'Fn::Join': ['-', ['hello', { Ref: 'Param' } ]] },
}}
```

This PR solves this issue by filtering out any CloudFormation
intrinsics. If the merge finds an intrinsic key in the target it "drops" the
intrinsic and takes whatever value is in the source (override).

fix aws#19971, aws#19447


----

### All Submissions:

* [ ] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md)

### Adding new Unconventional Dependencies:

* [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies)

### New Features

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)?

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
aws-cdk-lib Related to the aws-cdk-lib package bug This issue is a bug. p1
Projects
None yet
Development

No branches or pull requests

5 participants