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 Tags Support #91

Closed
eladb opened this issue Jun 13, 2018 · 16 comments
Closed

AWS Tags Support #91

eladb opened this issue Jun 13, 2018 · 16 comments

Comments

@eladb
Copy link
Contributor

eladb commented Jun 13, 2018

It should be possible to apply tags to AWS resources which support them and we would like to have a uniform API for tags across the AWS construct library

@moofish32
Copy link
Contributor

moofish32 commented Aug 2, 2018

Coming from #458 I agree that a generic tag implementation seems like the right idea.

Before I spend too much time proposing, does the group have strong opinions on the two high level concepts:

  1. Implement tagg-able functionality on cdk.Resource
    • All resources will get a set of functions addTag, removeTag, getTags, hasTag, getTag, isTaggable
    • The functions would respond correctly if the props type has a field called tags, if the props does not have field called tags all of the methods would raise and isTaggable returns false
    • This means it's not hooked on constructs which implies our custom integrations later have to design and deal with their own tag concepts, however we could still define an interface to help them
    • The interface to the consumer would be cdk.Tag or {key: string, value: string} while the array would be backed with cdk.Token[] for the lazy eval
  2. Use a mixin design pattern
    • Concept is well designed in Scala with Traits and Ruby, Typescript has some patterns; so first concern is how portable this is across the core languages?
    • Likely could hook cdk.Resource still, but I wonder if Construct is better?
    • Same methods as above except we would use a constructor style mixin

I'm much less certain about the pitfalls in option 2, but these are the first front runners for a cross cutting implementation in my mind. I am still new to typescript and if there is a better pattern for this cross-cutting implementation throw some links my way and I'll read up. If we think one of these is a good starting point, I can dig a little deeper and create some code to prototype.

Also for the sake of discussion the inheritance tree is the following AFAIK for cdk.Resource <- Refererencable <- StackElement <- Constructor I did consider StackElement but only really Metadata and Resources from a CFN context made sense to me.

@moofish32
Copy link
Contributor

moofish32 commented Aug 2, 2018

Ok - big issue with option 1

  1. Implement tagg-able functionality on cdk.Resource
    All resources will get a set of functions addTag, removeTag, getTags, hasTag, getTag, isTaggable
    The functions would respond correctly if the props type has a field called tags, if the props does not have field called tags all of the methods would raise and isTaggable returns false
    This means it's not hooked on constructs which implies our custom integrations later have to design and deal with their own tag concepts, however we could still define an interface to help them
    The interface to the consumer would be cdk.Tag or {key: string, value: string} while the array would be backed with cdk.Token[] for the lazy eval

Things like VpcSubnet <- VpcSubnetRef <- Construct ... so Resource does not have a relationship to VpcSubnet meaning this can't be attached to Resource.

This raises a key question should Construct know about or have a member of type Resource or Resource[]? If Constructs must be unaware of resources because things beyond Cloudformation resources are coming, then I understand. However, this separation is an interesting design choice that might limit options for a cross cutting (AOP - like) implementation of tags.

@eladb
Copy link
Contributor Author

eladb commented Aug 3, 2018

In general, the CDK prefers composition over inheritance. So the rule of thumb is that constructs almost always directly extend the Construct class. To represent common behavior we use interfaces and sometimes utility classes.

So for example, you might envision something like:

interface ITaggable {
  tags: Tags
}

class Tags {
  addTag()
  removeTag()
  ...
}

And then,

class VpcSubnet extends Construct implements ITaggable {
  constructor(...) {
    const resource = new cloudformation.VpcSubnetResource(this, 'Resource', ...);
    this.tags = new Tags(resource); // or some other way to let the Tags class manage tags for the resource
  }
}

Resource, specifically, is used as the base class for the generated CloudFormation resources (the classes under the cloudformation namespace in each library), and should not be "exposed" in the AWS Construct Library. It is very low-level and mostly deals with serializing CloudFormation. We should probably rename it to CloudFormationResource.

@rix0rrr
Copy link
Contributor

rix0rrr commented Aug 3, 2018

Can we first list some use cases? I'm not entirely sure what users will expect from this functionality...

@eladb
Copy link
Contributor Author

eladb commented Aug 3, 2018

@rix0rrr, you are right.

Here's an attempt:

  • It should be possible to specify tags when taggable constructs are defined (in "props").
  • It should be possible to add/modify/remove tags for taggable constructs.
  • Question: should tags should be recursive or maybe optionally recursive? If I add a tag to a construct which transitively includes multiple taggable resources in it, then would users expect that all the resources within this tree are tagged with the same set tag?

When aspects will be available, it should be possible to define aspects (#282) which will tag resources throughout a stack or a part of the stack. There is nothing special to do here in order to support this, but just calling it out.

@PaulMaddox
Copy link
Contributor

I see two tagging related gaps

  • As a user, I want to be able to specify tags for a stack that flow down to all sub resources (note: this functionality is provided by CloudFormation but I don’t think it’s exposed in CDK).

  • As a user, I want things created to have meaningful names in the AWS console. Often unless you manually specify a Name tag this won’t happen. With CDK, users specify a name for each resource, so CDK could automatically create a Name tag based on StackName+ResourceName (unless overridden)

@PaulMaddox
Copy link
Contributor

Question: should tags should be recursive or maybe optionally recursive? If I add a tag to a construct which transitively includes multiple taggable resources in it, then would users expect that all the resources within this tree are tagged with the same set tag?

I think tag propagation should be an option for each tag with a default=true. This matches other areas of the platform (such as auto scaling group tag propagation).

@rix0rrr
Copy link
Contributor

rix0rrr commented Aug 3, 2018

Okay, but there was also some Kubernetes-related use case right? That's how this started.

My question is not "how does the CDK enable tags", my question is "what do people use tags for".

  • EKS uses it for some purpose?
  • Cost queries
  • Presenting pretty names in the console (is that universally supported and distinct from the PhysicalName of a resource?)
  • ...

@eladb
Copy link
Contributor Author

eladb commented Aug 3, 2018

👍 for propagate=true by default. This will allow implementing the stack-wide tagging by simply adding a tag at the stack level.

A note about a default "name" tag: I think this is something we will be able to implement using aspects once we have a common tagging interface. The aspect will simply traverse the construct tree and will automatically assign a "Name" tag to all taggable constructs. I would argue that the default "Name" should be the construct's path (which is unique within the stack) and not the construct's id (which is only locally unique within it's parent construct).

@moofish32
Copy link
Contributor

moofish32 commented Aug 3, 2018

@rix0rrr

EKS uses it for some purpose?

EKS has special tags that denote a VPC subnet as an EKS VPC subnet I believe this is to enable the communication back to the Master nodes not in the VPC and managed by AWS. I think there a few layers of security on the subnet besides tags, but without tags you can't use EKS.

Presenting pretty names in the console (is that universally supported and distinct from the PhysicalName of a resource?)

I actually don't know the absolute answer here. For most things in EC2 console I know the Name tag is the visual display, but there are so many other services. I use Kinesis a lot and I'm not even sure there, and the Code Deploy UI is difficult to work with ... so I just don't know, but it seems like a reasonable default.

@moofish32
Copy link
Contributor

Question: should tags should be recursive or maybe optionally recursive? If I add a tag to a construct which transitively includes multiple taggable resources in it, then would users expect that all the resources within this tree are tagged with the same set tag?

This is where my question was really going. In order to do this I need to make Construct aware of Resource. Today this is implied by the author of the construct, but there is actually no member or class level relationship. If Construct had private resources: Resource[] then Resource has properties and we could check if tags were supported. However, I didn't know if Resource and Construct were intentionally separated so that non-AWS resources could be added later?

I think we are in the right discussion, but if I sound way off the mark let me know. There is plenty of context I could be missing.

@eladb
Copy link
Contributor Author

eladb commented Aug 3, 2018

I don’t think Construct needs to be aware of Resource in order to implement tag propagation. As mentioned above, Resource is only used as a base class for generated CloudFormation resources, and this is about tagging support at the AWS Construct Library layer, which a rich, object-oriented framework.

AWS Constructs that support tagging should implement an interface ITaggable and then one can iterate on children via and identify that a child is taggble via duck-typing (e.g ”tags” in child).

I do realize there might be missing some design/architectural context, and happy to elaborate as needed.

@moofish32
Copy link
Contributor

I don’t think Construct needs to be aware of Resource in order to implement tag propagation. As mentioned above, Resource is only used as a base class for generated CloudFormation resources, and this is about tagging support at the AWS Construct Library layer, which a rich, object-oriented framework.

I think this might be my point of confusion. Construct tagging does not alone get me to Tags on AWS Resources does it? The Construct has to properly pass those tags to the Resource it creates? That's the part I added for #458? Without putting the tags into the ResourceProps.properties that support tags, how do I actually get the tags on my resources (e.g. for cost allocation or EKS)? What are the benefits and use cases of only tagging at the AWS Construct Library layer instead of the resulting AWS Resource?

@eladb
Copy link
Contributor Author

eladb commented Aug 3, 2018

@moofish32 of course, the AWS Constructs will have to assign the tags to the resources they define. Something like #458 is definitely needed, but eventually we will want a uniform interface for tagging so we can later on do some cool "aspect-oriented" things with it.

I am totally fine with #458 as-is until we have the tagging pattern established, or if you want to take a whack at a more general purpose programming model for tagging, you are more than welcome.

Here's a sketch of my mental model around tags:

interface ITaggable {
  tags: Tags
};

// tag manager
class Tags {
  private readonly tags: { [key: string]: string };

  constructor(readonly parent: Construct, initialTags?: { [key: string]: string }) {
    this.tags = initialTags || { }
  }

  toJson() {
    return Object.keys(this.tags).map(key => ({ key, value: this.tags[key] }));
  }

  setTag(key: string, value: string, propagate = true) {
    this.tags[key] = value;

    if (propagate) {
      applyTagToChildren(this.parent);
    }
    
    function applyTagToChildren(construct: Construct) {
      for (const child of construct.children) {
        if ('tags' in child) {
          // child implements ITaggable (duck-typing)
          child.tags.setTag(key, value);
        }

        applyTagToChildren(child); // recurse
      }
    }

  }
}

// s3 bucket
interface BucketProps {
  // ...
  tags?: { [key: string]: string }
}

class Bucket extends Construct implements ITaggable {
  public readonly tags: Tags;
  constructor(...) {
    this.tags = new Tags(this, props.tags);
    new cloudformation.BucketResource(this, 'Resource', {
      // ..
      tags: new Token(() => this.tags.toJson()) // render tags for this resource during synthesis
    });
  }
}

Now, I can do:

bucket.setTag('foo', 'bar');

@eladb
Copy link
Contributor Author

eladb commented Aug 3, 2018

@moofish32 edited my comment above after reading your notes again...

@moofish32
Copy link
Contributor

💡 the moment where academically I can describe composition over inheritance, turns out to have occurred much sooner than I could apply it. This was my confusion. I was trying to figure out how to put the TagManager trait on either Resource or Construct so the developer didn't have to manage any of it. I see this composition pattern fits much more cleanly in as-built CDK lib. Thanks for taking the time to connect the theory and practice.

moofish32 added a commit to moofish32/aws-cdk that referenced this issue Aug 23, 2018
Fixes aws#91, Closes aws#458 (as obsolete)

The TagManager is a class Construct authors can use to implement tagging
consistently. The manager provides the ability to propagate tags from
parents to child, override parent tags in the child, set default tags on
the child that can be overwritten by parents, and block tags from
parents.

Adding tagging support for the Vpc and Subnet constructs.
moofish32 added a commit to moofish32/aws-cdk that referenced this issue Aug 24, 2018
Fixes aws#91, Closes aws#458 (as obsolete)

The TagManager is a class Construct authors can use to implement tagging
consistently. The manager provides the ability to propagate tags from
parents to child, override parent tags in the child, set default tags on
the child that can be overwritten by parents, and block tags from
parents.

Adding tagging support for the Vpc and Subnet constructs.
@eladb eladb closed this as completed in 45d0aa2 Sep 3, 2018
homakk pushed a commit to homakk/aws-cdk that referenced this issue Dec 1, 2022
* chore: add data structures for service connect config

* chore(spec): remove erroneous LogConfiguration property

* chore: fix signature of aliases
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants