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.

Sign up for GitHub

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

Jump to bottom

Tags not applied in aspects in 1.45 #8536

Closed
henrist opened this issue Jun 13, 2020 · 4 comments
Closed

Tags not applied in aspects in 1.45 #8536

henrist opened this issue Jun 13, 2020 · 4 comments
Assignees
@NetaNir
Labels
@aws-cdk/core Related to core CDK functionality bug This issue is a bug.

Comments

@henrist
Copy link

henrist commented Jun 13, 2020

I have an aspect to add tags to all resources in the app.

It worked in 1.44, but in 1.45 tags are not added.

Reproduction Steps

app.node.applyAspect({
  visit(construct: cdk.IConstruct) {
    if (cdk.Construct.isConstruct(construct)) {
      const stack = construct.node.scopes.find((it): it is cdk.Stack =>
        cdk.Stack.isStack(it),
      )
      if (stack != null) {
        console.info("Adding tag")
        cdk.Tag.add(construct, "Test", "Value")
      }
    }
  },
})

No tag added. Full reproduction repo from sample-app: https://github.com/henrist/cdk-aspect-tag-issue - the commits shows it working in 1.44 before not producing tags in 1.45.

Error Log

No error seen. However, the log statement shown above is triggered, so the cdk.Tag.add is called.

Environment

  • CLI Version : 1.45
  • Framework Version: 1.45
  • Node.js Version: v12.13.0
  • OS : Arch Linux
  • Language (Version): TypeScript, not tested other

Other

The same code worked fine in 1.44, but no tags are emitted in 1.45.

In our setup I have a helper to simplify this found at https://github.com/capralifecycle/liflig-cdk/blob/9032da75b67d4d5ffa1b518034cf3150091c398a/src/tags.ts

We commit snapshots of the templates in the Cloud Assembly, which led us to catch this during update to 1.45.

This is 🐛 Bug Report
@henrist henrist added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Jun 13, 2020
@NetaNir
Copy link
Contributor

NetaNir commented Jun 15, 2020 •
edited
Loading

Hi!

Tags are implemented using aspects, so in the code you pasted, which uses aspects to tag constructs you are actually applying an aspect via an aspect, this was never truly supported in the CDK and would only work in very specific cases, depending on the order in which the construct tree was built.
We did introduce a change to the CDK in v1.45.0 that might have changed this behavior.

While we investigate this, it seems like you are trying to tag all constructs which are defined inside a stack? If so, could you instead add the tag to the stacks? Let me know if I missed something in your use case.

@NetaNir NetaNir self-assigned this Jun 15, 2020
@NetaNir NetaNir added the @aws-cdk/core Related to core CDK functionality label Jun 15, 2020
@henrist
Copy link
Author

henrist commented Jun 15, 2020

While we investigate this, it seems like you are trying to tag all constructs which are defined inside a stack? If so, could you instead add the tag to the stacks? Let me know if I missed something in your use case.

What I am trying to achieve is to tag all resources in the application, without having to change the stacks or maintain an extra statement per stack. The tag value also includes the stack name, or otherwise I could have worked around this by cdk.Tag.add(app, "Name", "Value") at the end.

Maybe there should be some error if an aspect is applied while already applying a aspect?

For my use case I can probably solve this by creating a custom Tag implementation.

henrist added a commit to capralifecycle/liflig-cdk that referenced this issue Jun 15, 2020
@henrist
fix(tags): avoid nested aspects
3ecf03f 
See aws/aws-cdk#8536
@henrist
Copy link
Author

henrist commented Jun 15, 2020

I've worked around this by avoiding the nested aspects (by using construct.tags.setTag directly, as seen in the linked commit). For my part I'm fine with closing this as wont-fix (unsupported).

@SomayaB SomayaB removed the needs-triage This issue or PR still needs to be triaged. label Jun 18, 2020
@NetaNir NetaNir mentioned this issue Jun 19, 2020
Merged
@NetaNir
Copy link
Contributor

NetaNir commented Jun 19, 2020

Im glad to hear you found a workaround, Im closing this in favor of #8639

@NetaNir NetaNir closed this as completed Jun 19, 2020
mergify bot pushed a commit that referenced this issue Aug 3, 2020
@NetaNir
feat(core): warn if an aspect was added via another aspect (#8639)
9d7bef7 
### Updated PR

Since we introduced [`stages`](#8423) which had the unintended side effect of the CDK not supporting adding an Aspect via another Aspect, we have not seen any issues reported beside #8536, that was resolved without requiring this capability.
Given that, and the fact this features has many sharp edges we decided to leave it unsupported, and add a warning in the meantime. 

-----------

Following up on #8536

If an aspect is added via another aspect, the inner aspect  will  not be invoked. Take for example this code:

```typescript
const app = new cdk.App();

app.node.applyAspect({
  visit(construct: cdk.IConstruct) {
    construct.node.applyAspect({
      visit(construct: cdk.IConstruct) {
        console.info("Invoking aspect on construct: " + construct.node.id);  // This will not be called
      }
    })
  }
});
```

Since aspects are added only on the top level node,  if an aspect is added while `InvokeAspects` is called on that node,  it will be ignored since it will not be added to list of aspects to invoke (`allAspectsHere` in the bellow code):
```typescript
function invokeAspects(root: IConstruct) {
  recurse(root, []);

  function recurse(construct: IConstruct, inheritedAspects: constructs.IAspect[]) {
    // hackery to be able to access some private members with strong types (yack!)
    const node: NodeWithAspectPrivatesHangingOut = construct.node._actualNode as any;

    const allAspectsHere = [...inheritedAspects ?? [], ...node._aspects];

    for (const aspect of allAspectsHere) {
      if (node.invokedAspects.includes(aspect)) { continue; }

      aspect.visit(construct); <-- an aspect that was added here will not be added to `allAspectsHere` and will be ignored

      node.invokedAspects.push(aspect);
    }

    for (const child of construct.node.children) {
      if (!Stage.isStage(child)) {
        recurse(child, allAspectsHere);
      }
    }
  }
}
 
```

Assuming this is not something we want to support**, we can detect it by comparing the size of `node._aspects` before and after the call to `aspect.visit`, and emit a warning if there has been a change. Note that while the aspect will not be invoked it will be added to every child construct during the recursive visit. Emitting a warning for each child construct will result in a noisy console, to prevent this I have added a flag that will only allow adding **one warning per application**, given this limitation I'm not sure  there is a lot of value in adding the warning, thoughts?
 
If we decide to add it I will add tests.



 (** theoretically we could support it by adding the aspects to `allAspectsHere`  during the loop, but this will require a non trivial implementation in order to avoid infinite recursion) 



----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
eladb pushed a commit that referenced this issue Aug 10, 2020
@NetaNir
feat(core): warn if an aspect was added via another aspect (#8639)
80288f7 
### Updated PR

Since we introduced [`stages`](#8423) which had the unintended side effect of the CDK not supporting adding an Aspect via another Aspect, we have not seen any issues reported beside #8536, that was resolved without requiring this capability.
Given that, and the fact this features has many sharp edges we decided to leave it unsupported, and add a warning in the meantime. 

-----------

Following up on #8536

If an aspect is added via another aspect, the inner aspect  will  not be invoked. Take for example this code:

```typescript
const app = new cdk.App();

app.node.applyAspect({
  visit(construct: cdk.IConstruct) {
    construct.node.applyAspect({
      visit(construct: cdk.IConstruct) {
        console.info("Invoking aspect on construct: " + construct.node.id);  // This will not be called
      }
    })
  }
});
```

Since aspects are added only on the top level node,  if an aspect is added while `InvokeAspects` is called on that node,  it will be ignored since it will not be added to list of aspects to invoke (`allAspectsHere` in the bellow code):
```typescript
function invokeAspects(root: IConstruct) {
  recurse(root, []);

  function recurse(construct: IConstruct, inheritedAspects: constructs.IAspect[]) {
    // hackery to be able to access some private members with strong types (yack!)
    const node: NodeWithAspectPrivatesHangingOut = construct.node._actualNode as any;

    const allAspectsHere = [...inheritedAspects ?? [], ...node._aspects];

    for (const aspect of allAspectsHere) {
      if (node.invokedAspects.includes(aspect)) { continue; }

      aspect.visit(construct); <-- an aspect that was added here will not be added to `allAspectsHere` and will be ignored

      node.invokedAspects.push(aspect);
    }

    for (const child of construct.node.children) {
      if (!Stage.isStage(child)) {
        recurse(child, allAspectsHere);
      }
    }
  }
}
 
```

Assuming this is not something we want to support**, we can detect it by comparing the size of `node._aspects` before and after the call to `aspect.visit`, and emit a warning if there has been a change. Note that while the aspect will not be invoked it will be added to every child construct during the recursive visit. Emitting a warning for each child construct will result in a noisy console, to prevent this I have added a flag that will only allow adding **one warning per application**, given this limitation I'm not sure  there is a lot of value in adding the warning, thoughts?
 
If we decide to add it I will add tests.



 (** theoretically we could support it by adding the aspects to `allAspectsHere`  during the loop, but this will require a non trivial implementation in order to avoid infinite recursion) 



----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
curtiseppel pushed a commit to curtiseppel/aws-cdk that referenced this issue Aug 11, 2020
@NetaNir
feat(core): warn if an aspect was added via another aspect (aws#8639)
a75cba2 
### Updated PR

Since we introduced [`stages`](aws#8423) which had the unintended side effect of the CDK not supporting adding an Aspect via another Aspect, we have not seen any issues reported beside aws#8536, that was resolved without requiring this capability.
Given that, and the fact this features has many sharp edges we decided to leave it unsupported, and add a warning in the meantime. 

-----------

Following up on aws#8536

If an aspect is added via another aspect, the inner aspect  will  not be invoked. Take for example this code:

```typescript
const app = new cdk.App();

app.node.applyAspect({
  visit(construct: cdk.IConstruct) {
    construct.node.applyAspect({
      visit(construct: cdk.IConstruct) {
        console.info("Invoking aspect on construct: " + construct.node.id);  // This will not be called
      }
    })
  }
});
```

Since aspects are added only on the top level node,  if an aspect is added while `InvokeAspects` is called on that node,  it will be ignored since it will not be added to list of aspects to invoke (`allAspectsHere` in the bellow code):
```typescript
function invokeAspects(root: IConstruct) {
  recurse(root, []);

  function recurse(construct: IConstruct, inheritedAspects: constructs.IAspect[]) {
    // hackery to be able to access some private members with strong types (yack!)
    const node: NodeWithAspectPrivatesHangingOut = construct.node._actualNode as any;

    const allAspectsHere = [...inheritedAspects ?? [], ...node._aspects];

    for (const aspect of allAspectsHere) {
      if (node.invokedAspects.includes(aspect)) { continue; }

      aspect.visit(construct); <-- an aspect that was added here will not be added to `allAspectsHere` and will be ignored

      node.invokedAspects.push(aspect);
    }

    for (const child of construct.node.children) {
      if (!Stage.isStage(child)) {
        recurse(child, allAspectsHere);
      }
    }
  }
}
 
```

Assuming this is not something we want to support**, we can detect it by comparing the size of `node._aspects` before and after the call to `aspect.visit`, and emit a warning if there has been a change. Note that while the aspect will not be invoked it will be added to every child construct during the recursive visit. Emitting a warning for each child construct will result in a noisy console, to prevent this I have added a flag that will only allow adding **one warning per application**, given this limitation I'm not sure  there is a lot of value in adding the warning, thoughts?
 
If we decide to add it I will add tests.



 (** theoretically we could support it by adding the aspects to `allAspectsHere`  during the loop, but this will require a non trivial implementation in order to avoid infinite recursion) 



----

*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
Assignees

@NetaNir NetaNir

Labels
@aws-cdk/core Related to core CDK functionality bug This issue is a bug.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@eladb @henrist @NetaNir @SomayaB