Ensure s3 bucket issue 495 - Bucket Tags

[GarisonLotus]

I ended up using the exisiting functions to perform the tagging by pulling in the tags from context. Let me know if this is the right path, or if you wanted me to change something.

[GarisonLotus]

This sets the tags on every run, just like with cloudformation.

[GarisonLotus]

The CI failure looks like an IAM role issue of some sort, or AWS issue. Let me know if I'm wrong with that assumption and need to revisit this PR.

[GarisonLotus]

@phobologic - I went over this with @troyready this morning and came up with a concern with the path I went down to solve this. I want to take another stab at this by adding a TLK for bucket tags.

My Thought process on this is that a stacker end-user could have seperate configuration files within the same stacker environment/namespace that contain different cost tags. (DB tier, App Tier, Web tier w/ different tags, as an example.) In reality, these seperate configurations might want to have the same cost tag for the bucket used vs the deployed resources. Originally I thought this would only be needed for the aws_lambda module, but now I'm convinced that we should have it overall.

New Idea: Have a TLK "bucket_tags" that if specified, will tag the S3 bucket with the tags supplied. If not, it will fail back to context.tags like it does in this PR. Thoughts?

[phobologic]

Thanks @GarisonLotus - quick question, what is TLK? :)

[GarisonLotus]

@phobologic - TLK was shorthand for "Top Level Keyword". I have amended my PR to include a new TLK for "bucket_tags" that falls back to the tags behavior if not set. I also updated tags while I was there to add the stacker_namespace tag if not included, while allowing the a blank tag set (via tags: {}) to still be set. I updated the existing tests to include the new behavior.

This PR now resolves #495 and #496.

I'm not too sure we want to actually solve #496, now that I have played with it. The unintended side effect of this is if you did specify "tags" previously without also including the stacker_namespace tag, you would now have a new stack update that would be required. In other words, it would look like everything is being updated after updating stacker... and to a user who doesn't exact that, they might freak out and cancel the run thinking stacker screwed something up.

I don't really know how to get around that, without just reverting back to the old behavior. The lines in question are:

https://github.com/GarisonLotus/stacker/blob/2780cc0362636d6e5716250e94fe518c64e3d5ca/stacker/context.py#L99-L105

Thoughts? or is the new behavior alright to force on users? (I know it's actually safe to do, but it's the experience I'm worried about)

Maybe I add a logger note?

[troyready]

My 2c: I'd be in favor of putting off #496 until Stacker v2, to minimize the changes to currently deployed stacks. It's not really A Big Deal, but since the current behavior isn't really a bug I think there's value in postponing it.

[GarisonLotus]

That's how I was leaning... I think it's a good idea to hold off on #496 until later, but perhaps put in a deprecation warning instead. This way, people can know it's going to happen and have time to add the tag into their deployments done with 1.1.1. @phobologic, opinion?

[phobologic]

Yeah, lets hold off on #496 till 2.0. I'm starting to think more about that, and this sounds like a good candidate for it.

[troyready]

Looks like an error in rebasing here -- this class variable shouldn't go away.

[phobologic]

update to fix this... a little scary that no tests caught this though. Makes me think this could use some more tests w/ stubber. I'll take a poke at that in a bit.

[phobologic]

Hey @GarisonLotus - I merged from master upstream, and everything seems to pass now :)

[phobologic]

Ok, I've done a little bit of cleaning up - but I think this is close. @GarisonLotus can you add some tests for setting tags with the new config setting?

[ejholmes]

I was thinking a bit about this recently. What if we just allowed people to create their stacker bucket, with stacker? I was thinking of supporting something like this in the config:

# Normally, this accepts a string, but it could accept a `stacker.config.Stack`
# dict like this:
stacker_bucket:
  name: stacker-bucket
  class_path: stacker.blueprints.StackerBucket
  tags:
    whatever: "you want"

So, if stacker_bucket is a dict, it gets parsed as a stacker.config.Stack, which gets added to the dependency graph and creates a ${namespace}-stacker-bucket stack. This stack would be expected to output BucketId, which stacker would lookup and use as the stacker bucket.

By default, the StackerBucket blueprint would just create a bucket similar to how stacker does right now, but people could override it and add anything extra to it that they want, as long as it continues to include the BucketId output.

[ejholmes]

So, we (internally) actually switched over to a method similar to what I outlined above, without any changes to stacker. Our stacker config now has a stack that creates a stacker bucket (and also does some stuff like setup a BucketPolicy for supporting cross account access for CloudFormation). Once the stack is created, we just updated stacker_bucket to reference the stack. Not completely automated, but it only happens once and gives us unlimited flexibility in managing the bucket.

What do you guys think about just documenting that? We could also provide a base "StackerBucket" blueprint to make it a little easier (although, it's pretty trivial since it just creates an s3 bucket).

[troyready]

@ejholmes I really like your previous PoC (putting the stacker bucket stack in the dag), but I'm way fine with this simple recommendation as well.