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.TagManager: (is_taggable function stopped working in while migrating from 2.70.0 to 2.88.0) #26495

Closed
Ranjith072 opened this issue Jul 25, 2023 · 20 comments · Fixed by #31600 or softwaremill/tapir#4137 · May be fixed by NOUIY/aws-solutions-constructs#135 or NOUIY/aws-solutions-constructs#136
Assignees
Labels
@aws-cdk/core Related to core CDK functionality bug This issue is a bug. p1

Comments

@Ranjith072
Copy link

Describe the bug

aws-cdk-lib.TagManager.is_taggable has stopped working wth cdk python version 2.88.0 with the below error message
Screenshot 2023-07-25 at 16 40 33

Expected Behavior

it should just return the boolean value during the runtime.

Current Behavior

just fails with the error message as in the screenshot.

Reproduction Steps

Try to run synth after migrating to the cdk version2.88.0, it will through the same error message as in the screenshot attached.

Possible Solution

It was working since cdk 2.0 not sure what has changed behind the scen..i see new method called is_taggable_v2 being added to the TagManager class using this will not break the code but it will remove the existing tags.

Additional Information/Context

No response

CDK CLI Version

2.88.0

Framework Version

No response

Node.js Version

14.17.6

OS

macos: ventura 13.5

Language

Python

Language Version

3.8.8

Other information

No response

@Ranjith072 Ranjith072 added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Jul 25, 2023
@github-actions github-actions bot added the aws-cdk-lib Related to the aws-cdk-lib package label Jul 25, 2023
@peterwoodworth
Copy link
Contributor

Could you provide reproduction code please

@peterwoodworth peterwoodworth added response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. and removed needs-triage This issue or PR still needs to be triaged. labels Jul 25, 2023
@Ranjith072
Copy link
Author

Ranjith072 commented Jul 26, 2023

Hi ,
Create a new python venv and do fresh installation of cdk 2.88.0 and try to run synth , it will through the same error message as in the screenshot attached.
Here is the code i am using to add the tags .

@jsii.implements(IAspect)
class AddTags:
    def visit(self, node):
        if isinstance(node, Stack) and not isinstance(node, WbStack):
            # We want to extend WbStack to ensure we add the ApplicationName
            # tag, which is currently the only obvious way we can verify we have it
            # added to all taggable resources.
            raise Exception("Found a stack that is not extending WbStack")
        if TagManager.is_taggable(node):
            # Verify the element is taggable.
            # For some reason, the is_taggable returns more nodes than
            # those which can be tagged, giving us the error:
            # Error: 'CfnResource' object has no attribute 'tags'
            # noinspection PyBroadException
            try:
                # noinspection PyStatementEffect
                node.tags
            except Exception:
                return

            # noinspection PyBroadException
            try:
                # The lookup will fail if the node we are looking at is above a
                # stack in the tree.
                stack = Stack.of(node)
            except Exception:
                return

            assert isinstance(stack, WbStack)

            node.tags.set_tag("StackName", stack.stack_name, 100, True)
            node.tags.set_tag("ProjectName", "WesternAva", 100, True)
            node.tags.set_tag("ApplicationName", stack.application_name, 100, True)
            
Aspects.of(app).add(AddTags())

this was all working till 2.70.0 by adding the tags to all the resources that support tagging.

@github-actions github-actions bot removed the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Jul 26, 2023
@peterwoodworth
Copy link
Contributor

It would be really helpful if you could please provide a full reproduction which I can copy+paste. I can't be sure exactly how you're using this class you've defined or if there are specific elements in your stack that trigger this error

@peterwoodworth peterwoodworth added the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Jul 26, 2023
@Ranjith072
Copy link
Author

Ranjith072 commented Jul 27, 2023

Hi,
sorry for the confusion, please find the below code snippet which will produce the error.
copy and paste the below code snippet into app.py and run cdk synth it will produce the error .
if we replace TagManager.is_taggable with TagManager.is_taggable_v2 it will complete the synth but without any tags.

#!/usr/bin/env python

import jsii
from aws_cdk import (
    aws_s3 as s3,
    App,
    IAspect,
    TagManager,
    Stack,
    Tags,
    Aspects,
)
from constructs import Construct

app = App()

@jsii.implements(IAspect)
class AddTags:
    def visit(self, node):
        if TagManager.is_taggable(node):
            # Verify the element is taggable.
            # For some reason, the is_taggable returns more nodes than
            # those which can be tagged, giving us the error:
            # Error: 'CfnResource' object has no attribute 'tags'
            # noinspection PyBroadException
            try:
                # noinspection PyStatementEffect
                node.tags
            except Exception:
                return

            # noinspection PyBroadException
            try:
                # The lookup will fail if the node we are looking at is above a
                # stack in the tree.
                stack = Stack.of(node)
            except Exception:
                return

            node.tags.set_tag("StackName", stack.stack_name, 100, True)
            node.tags.set_tag("ProjectName", "WesternAva", 100, True)
            node.tags.set_tag("ApplicationName", stack.application_name, 100, True)



@jsii.implements(IAspect)
class RemoveEipTags:
    @staticmethod
    def remove(node: Stack, name: str):
        Tags.of(node).remove(name, include_resource_types=["AWS::EC2::EIP"])

    def visit(self, node):
        if isinstance(node, Stack):
            self.remove(node, "Name")
            self.remove(node, "StackName")
            self.remove(node, "ProjectName")


Aspects.of(app).add(AddTags())
Aspects.of(app).add(RemoveEipTags())

class MyCustomStack(Stack):
    def __init__(self, scope: Construct, id: str) -> None:
        super().__init__(scope, id)

        my_bucket = s3.Bucket(self, "MyBucket")


MyCustomStack(app, "MyCustomStack")
app.synth()

@github-actions github-actions bot removed the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Jul 27, 2023
@rittneje
Copy link

rittneje commented Jul 30, 2023

@Ranjith072 While this does not address the bug itself, I think you can simplify your code to not rely on TagManager at all.

@jsii.implements(aws_cdk.IAspect)
class AddTags:
    def visit(self, node):
        if isinstance(node, aws_cdk.Stack):
            if not isinstance(node, WbStack):
                raise Exception("Found a stack that is not extending WbStack")
        else:
            stack = aws_cdk.Stack.of(node)
            aws_cdk.Tag("StackName", stack.stack_name).visit(node)
            aws_cdk.Tag("ProjectName", "WesternAva").visit(node)
            aws_cdk.Tag("ApplicationName", stack.application_name).visit(node)
            
aws_cdk.Aspects.of(app).add(AddTags())

@Ranjith072
Copy link
Author

Ranjith072 commented Jul 31, 2023

Hi,
I was using the Aspects same way before but if you see the comments i have on the visit method (from the previous code i shared, i started to get issues with some previous versions so i had to use TagManager to get it working).

I tried the below code as suggested but it didn't add any tags to the resource when i did cdk synth.

#!/usr/bin/env python

import aws_cdk
import jsii
from aws_cdk import (
    aws_s3 as s3,
    App,
    IAspect,
    Stack,
    Tags,
    Aspects,
)
from constructs import Construct

app = App()
@jsii.implements(aws_cdk.IAspect)
class AddTags:
    def visit(self, node):
        if isinstance(node, aws_cdk.Stack):
            if not isinstance(node, Stack):
                raise Exception("Found a stack that is not extending WbStack")
            else:
                stack = aws_cdk.Stack.of(node)
                aws_cdk.Tag("StackName", stack.stack_name).visit(node)
                aws_cdk.Tag("ProjectName", "WesternAva").visit(node)
                aws_cdk.Tag("ApplicationName", "Test").visit(node)



@jsii.implements(IAspect)
class RemoveEipTags:
    @staticmethod
    def remove(node: Stack, name: str):
        Tags.of(node).remove(name, include_resource_types=["AWS::EC2::EIP"])

    def visit(self, node):
        if isinstance(node, Stack):
            self.remove(node, "Name")
            self.remove(node, "StackName")
            self.remove(node, "ProjectName")


Aspects.of(app).add(AddTags())
Aspects.of(app).add(RemoveEipTags())

class MyCustomStack(Stack):
    def __init__(self, scope: Construct, id: str) -> None:
        super().__init__(scope, id)

        my_bucket = s3.Bucket(self, "MyBucket")


MyCustomStack(app, "MyCustomStack")
app.synth()

@rittneje
Copy link

rittneje commented Jul 31, 2023

@Ranjith072 The code you shared in your latest comment has a bug. The else needs to be un-indented by one level as it goes with the isinstance(node, aws_cdk.Stack) check not the not isinstance(node, Stack) check. (Also I think you meant not isinstance(node, WbStack).)

@Ranjith072
Copy link
Author

@rittneje ,
Yes that's correct i meant not isinstance(node, WbStack) but i replaced it with just Stack if you guys wanted to reproduce the issue by copy pasting the code .
coming to the second part of the code .
The else needs to be un-indented by one level
i actually tried that first but ended up getting an runtime error with the below error message.
**RuntimeError: Error: App at '' should be created in the scope of a Stack, but no Stack found
**
below code will reproduce the error when you try to run synth .

#!/usr/bin/env python

import aws_cdk
import jsii
from aws_cdk import (
    aws_s3 as s3,
    App,
    IAspect,
    Stack,
    Tags,
    Aspects,
)
from constructs import Construct

app = App()
@jsii.implements(aws_cdk.IAspect)
class AddTags:
    def visit(self, node):
        if isinstance(node, aws_cdk.Stack):
            if not isinstance(node, WbStack):
                raise Exception("Found a stack that is not extending WbStack")
        else:
            stack = aws_cdk.Stack.of(node)
            aws_cdk.Tag("StackName", stack.stack_name).visit(node)
            aws_cdk.Tag("ProjectName", "WesternAva").visit(node)
            aws_cdk.Tag("ApplicationName", "Test").visit(node)



@jsii.implements(IAspect)
class RemoveEipTags:
    @staticmethod
    def remove(node: Stack, name: str):
        Tags.of(node).remove(name, include_resource_types=["AWS::EC2::EIP"])

    def visit(self, node):
        if isinstance(node, Stack):
            self.remove(node, "Name")
            self.remove(node, "StackName")
            self.remove(node, "ProjectName")


Aspects.of(app).add(AddTags())
Aspects.of(app).add(RemoveEipTags())


class WbStack(Stack):
    """
    A base class we can extend that helps us adding the ApplicationName
    tag to all stacks.
    """

    def __init__(
        self, scope: Construct, id: str, *, application_name: str,
    ) -> None:
        super().__init__(
            scope=scope, id=id,
        )

        self.application_name = application_name

class MyCustomStack(WbStack):
    def __init__(self, scope: Construct, id: str, application_name: str) -> None:
        super().__init__(scope, id, application_name=application_name)

        my_bucket = s3.Bucket(self, "MyBucket")


MyCustomStack(app, "MyCustomStack", "test")
app.synth()

@Ranjith072
Copy link
Author

also tried it like this and ended up getting the same error.

@jsii.implements(aws_cdk.IAspect)
class AddTags:
    def visit(self, node):
        if isinstance(node, Stack) and not isinstance(node, WbStack):
            raise Exception("Found a stack that is not extending WbStack")

        stack = aws_cdk.Stack.of(node)
        aws_cdk.Tag("StackName", stack.stack_name).visit(node)
        aws_cdk.Tag("ProjectName", "WesternAva").visit(node)
        aws_cdk.Tag("ApplicationName", "Test").visit(node)

@Ranjith072
Copy link
Author

Ranjith072 commented Aug 2, 2023

Hi @rittneje ,@peterwoodworth ,
After the new release of cdk V2.89.0 my current version(2.70.0) of cdk that was working for me has started to remove the tags for many resources..i have even tried different versions like 2.69.0 and 2.71.0, all of them are removing tags . it looks like the binaries for old versions have been refreshed with the new release.
it would be great to get some help here as this is breaking our builds (As part of our CICD pipeline we always do cdk synth to compare with previous successful commit to check for this kind of unexpected changes) because of this changing behavior cicd pipeline is failing and also we use Tags for billing allocation and cannot afford to lose these tags.
its changing all the stacks we have and i am using the same logic as i have shared above (#26495 (comment)).
cdk synth is showing these many changes and all of them are removing the tags.
Screenshot 2023-08-02 at 14 44 00

@Ranjith072
Copy link
Author

cdk managed singleton lambda functions and aws managed iam policies are some of the resource for which the tags are getting removed after the new release.

@Ranjith072
Copy link
Author

Hi,
it looks the old binaries are back now and the tags are intact with the old versions of cdk . Also found a fix for the TagManager bug.
change from if TagManager.is_taggable(node): --> .if hasattr(node, "tags"):

please find the code snippet below for the full code

@jsii.implements(IAspect)
class AddTags:
    def visit(self, node):
        if isinstance(node, Stack) and not isinstance(node, WbStack):
            # We want to extend WbStack to ensure we add the ApplicationName
            # tag, which is currently the only obvious way we can verify we have it
            # added to all taggable resources.
            raise Exception("Found a stack that is not extending WbStack")
        if hasattr(node, "tags"):
            # Verify the element is taggable.
            # For some reason, the is_taggable returns more nodes than
            # those which can be tagged, giving us the error:
            # Error: 'CfnResource' object has no attribute 'tags'
            # noinspection PyBroadException
            try:
                # noinspection PyStatementEffect
                node.tags
            except Exception:
                return

            # noinspection PyBroadException
            try:
                # The lookup will fail if the node we are looking at is above a
                # stack in the tree.
                stack = Stack.of(node)
            except Exception:
                return

            assert isinstance(stack, WbStack)

            node.tags.set_tag("StackName", stack.stack_name, 100, True)
            node.tags.set_tag("ProjectName", "WesternAva", 100, True)
            node.tags.set_tag("ApplicationName", stack.application_name, 100, True)

@peterwoodworth
Copy link
Contributor

it looks the old binaries are back now and the tags are intact with the old versions of cdk .

So the issue is fixed @Ranjith072 ?

@peterwoodworth peterwoodworth added the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Aug 11, 2023
@github-actions
Copy link

This issue has not received a response in a while. If you want to keep this issue open, please leave a comment below and auto-close will be canceled.

@github-actions github-actions bot added the closing-soon This issue will automatically close in 4 days unless further comments are made. label Aug 14, 2023
@Ranjith072
Copy link
Author

Hi,
i'm afraid the actual issue with TagManager.is_taggable(node): still exist. i only found a workaround.

@github-actions github-actions bot removed closing-soon This issue will automatically close in 4 days unless further comments are made. response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. labels Aug 14, 2023
@peterwoodworth
Copy link
Contributor

@Ranjith072, I start seeing the issue on 2.81.0. Check out this commit - it changes the code to the following:

  public static isTaggable(construct: any): construct is ITaggable {
    const tags = (construct as any).tags;
    return tags && typeof tags === 'object' && tags.constructor.name === 'TagManager';
  }

This may be causing issues only in Python, which means this is either a JSII issue, or this can be rewritten to be understood by Python. I'm not familiar enough with JSII to know much more however

@frankpengau
Copy link
Contributor

frankpengau commented Dec 3, 2023

I'm starting to see this problem when migrating from aws-cdk 2.99.1 to 2.100.0+ versions of the npm package.

I have some speculations/suspicions around the aws-service-spec being the cause for the issue but haven't found any solid evidence-backed root cause for the issue.

I am using jest and assertions to check for Tag properties in the cdk stack and facing issues trying to resolve them due to changes in the spec/type.

Anyone aware of anything around TagManager or Tags having breaking changes?

Edit: This is for aws-cdk for typescript btw!

@whussain7
Copy link

I'm currently facing the exact same issue as @frankpengau when migrating from 2.50.0 to 2.100.0, the tags are no longer being enforced with the way our custom CDK library is setup to handle tags which is more of a static approach.

The issue stems from the initialTagPriority property in the TagManager class and from looking at the updates it's received by AWS it seems like the tags are now being handled dynamically instead.

Currently I am attempting a workaround for our library but what it means is that we cannot use any version above 2.70.0 since our stacks extend interfaces for tags that are mandatory in any CDK Deployment we do.

@moelasmar moelasmar added @aws-cdk/core Related to core CDK functionality and removed aws-cdk-lib Related to the aws-cdk-lib package labels Aug 28, 2024
@sumupitchayan sumupitchayan self-assigned this Sep 24, 2024
@mergify mergify bot closed this as completed in #31600 Oct 2, 2024
@mergify mergify bot closed this as completed in be70c82 Oct 2, 2024
Copy link

github-actions bot commented Oct 2, 2024

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.

1 similar comment
Copy link

github-actions bot commented Oct 2, 2024

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.