Unable to set child tags when parent tags are inherited

[cbeattie-bsm]

It seems when you apply tags at a parent scope, any additional tags set on a child will not be set.

class MyParentConstruct extends cdk.Construct {
  constructor(scope: cdk.Construct, id: string) {
    super(scope, id);

    this.apply(new cdk.Tag('Foo', 'Bar'));

    new MyChildConstruct(this, 'Child');
  }
}

class MyChildConstruct extends cdk.Construct {
  constructor(scope: cdk.Construct, id: string) {
    super(scope, id);

    new cdkEc2.CfnVPC(this, 'Vpc', {
      cidrBlock: '10.0.0.0/16',
      tags: [
        {key: 'Name', value: 'MyVPC'},
      ]
    });
  }
}

Results:

Resources:
  testChildVpc5FFB6B5B:
    Type: AWS::EC2::VPC
    Properties:
      CidrBlock: 10.0.0.0/16
      Tags:
        - Key: Foo
          Value: Bar
    Metadata:
      aws:cdk:path: PartitionStack/test/Child/Vpc
  CDKMetadata:
    Type: AWS::CDK::Metadata
    Properties:
      Modules: aws-cdk=0.24.1,@aws-cdk/aws-cloudwatch=0.24.1,@aws-cdk/aws-ec2=0.24.1,@aws-cdk/aws-iam=0.24.1,@aws-cdk/aws-logs=0.24.1,@aws-cdk/cdk=0.24.1,@aws-cdk/cx-api=0.24.1,jsii-runtime=node.js/v8.11.3

I would expect that the child object would have both a 'Foo' and 'Name' tag.

[cbeattie-bsm]

A workaround is to use the apply() method instead of the tags property.

new cdkEc2.CfnVPC(...).apply(new cdk.Tag('Name', 'Value'));

Perhaps this is "works as expected"? Its unclear from the documentation IMO.

[eladb]

This is not the expected behavior. I think we have an issue with how tags are merged in L1s between the tags supplied in props and TagManager.

[eladb]

@moofish32 could you take a look?

I wonder if this is related to #1717

[moofish32]

@eladb - this is the mixing of property tags and aspect tags. I was trying to look at #1717 and my first place to check is this same issue. In the design I tried to draw our attention to this: #1451 (comment).

@cbeattie-bsm can you try?

class MyParentConstruct extends cdk.Construct {
  constructor(scope: cdk.Construct, id: string) {
    super(scope, id);

    this.apply(new cdk.Tag('Foo', 'Bar'));

    const child = new MyChildConstruct(this, 'Child');
    child.apply(new cdk.Tag('Name', 'MyVpc'));
  }
}

@eladb I can go back and add the ability to merge tags. The code is a little ugly as we have multiple formats and we have a broad cdk.Token that can represent the entire list of tags, as well as the easier handle tokens as strings for key/value pairs. I'm going to make a few comments back in #1717 about where I think this type mismatch is happening. I suspect they are related.

[cbeattie-bsm]

@moofish32 Your code snippet above works. If you use obj.apply(new cdk.Tag(...)) it will set the tag. Just to be clear, the issue is if you use the tags props property AND apply() on a parent, the tags attribute is not applied.

[moofish32]

@cbeattie-bsm - yes right now the two do not merge. If there is an apply() in your tree then it assumes you are not using property tags on Cfn* resources.

1. When we do merge these would you expect the parent apply or the property tag attribute to take precedence on collision?

2. Does the location of the apply (on a parent, or on the node directly) change your answer to the above?

[cbeattie-bsm]

1. When we do merge these would you expect the parent apply or the property tag attribute to take precedence on collision?

I think I would expect order of operations to apply. I would assume that internally we treat tags like a set so if there was a 'collision' my expectation would be that the tag value (and properties) would be overwritten.

2. Does the location of the apply (on a parent, or on the node directly) change your answer to the above?

I think my answer in question 1 (above) applies again here. My expectation would be that a subsequent tag operation would (or could) overwrite previous values.

Taking a bit of a step back, let's look at a very practical use-case more mixing and matching these approaches to tags.

In my case, I have a bunch of common tags that I want to set on all children of a construct (note: the parent/child tag inheritance is a GREAT feature). However, I want to set the "Name" tag individually on child items (for obvious reasons setting the "Name" tag at the parent scope doesn't make sense). I could simply use the item.apply() syntax and everything would work fine, but since I'm setting props on all of these child items, it seems cleaner to use the tag attribute of the props to set the "Name" tag: its one less statement.