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

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

Open
GarisonLotus opened this issue Oct 18, 2017 · 5 comments
Open

Comments

@GarisonLotus
Copy link
Contributor

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...

  • We modify the function to tag S3 buckets based on the tags top level keyword
  • We create a new argument in the aws_lambda.upload_lambda_functions function to allow an override, in case an end-user wants to specify a different set of tags for the S3 bucket created by aws_lambda.py:493

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

@phobologic
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

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

@GarisonLotus
Copy link
Contributor Author

@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
Copy link
Member

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.

@ejholmes ejholmes added this to the 1.1.3 milestone Mar 1, 2018
@ejholmes ejholmes removed this from the 1.3 milestone Apr 11, 2018
@GarisonLotus GarisonLotus removed their assignment May 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants