cloudtools / stacker

An AWS CloudFormation Stack orchestrator/manager.
http://stacker.readthedocs.io/en/stable/
BSD 2-Clause "Simplified" License
710 stars 167 forks source link

`stacker.util.ensure_s3_bucket` function does not tag S3 buckets created #495

Open GarisonLotus opened 7 years ago

GarisonLotus commented 7 years ago

Currently S3 buckets created by stacker do not have any tags, even if that bucket is created on a stacker build run with a stacker config yaml defining tags. This is because the ensure_s3_bucket function imported by base and aws_lambda do not submit tags to the function.

We should probably tag the S3 buckets. My concern is not the buckets storing the json templates, but the buckets created for uploads of serverless lambda functions as they could get big and actually have somewhat of an effect on a budget.

So I propose the following...

I think this should cover all bases as to cost tags that may be required by businesses using stacker.

phobologic commented 7 years ago

I think automatically providing some tags could be a good idea - probably at least stacker_namespace?

It's always bothered me that https://github.com/remind101/stacker/blob/ecd8933d715da8ede5b0d6e6d1e98bd827ff6b43/stacker/context.py#L97 removed the stacker_namespace tag automatically if you provided any of your own, seems like there's very little to remove that tag in the first place, and is kind of an unexpected behavior.

👍 to this though.

GarisonLotus commented 7 years ago

Yeah - I have been manually adding in the stacker_namespace tag in my configs to get it back in there... I so I guess I agree with you since I had workarounds in place to deal with the same thing.

I think the the tags function in stacker.context should be a seperate issue though. It's a valid one.

phobologic commented 7 years ago

Definitely a different issue - just wanted to make sure we don't make the same "mistake" here :)

GarisonLotus commented 7 years ago

@phobologic - Fourth potential feature needing to be included, updating tags on subsequent runs... as in, I have hundreds of S3 buckets already deployed by this function (the aws_lambda hook gets some love), and I want to be able to rerun the stacker yaml and have the ensure_s3_bucket function drop the tags into place. Thoughts? This would not be backwards compatible if I added it in there.

If you want to assign this to me, you can. I got permission to fix this on company time.

phobologic commented 7 years ago

That sounds fine to me - I don't think anything would be broken by adding tags. The bigger question is what it's going to do for performance - I imagine you'd either update the tags every time, or you'd do a get_bucket_tagging call to see if anything should change first. My guess is that updating the tags is a single call that is probably just about as fast as a call to get_bucket_tagging, so calling each time makes sense.

Thanks for offering to handle this @GarisonLotus, all for you knocking it out.