search

zalando-stups / mint-worker

The secret rotator and distributor for the STUPS ecosystem
http://stups.readthedocs.org/en/latest/components/mint.html
Other
9 stars 3 forks source link

#33 Create general protocol for bucket storage interfaces #39

Closed mikkeloscar closed 7 years ago

mikkeloscar commented 7 years ago

This is the first step to address #33

It solves the issue that S3 related code was used directly everywhere making it hard to use another bucket storage service in place of S3 e.i. Google Cloud Storage.

To solve this I have defined a protocol BucketStorage and used this protocol wherever s3 functions was previously called directly in the code. I have also changed the S3Exception to the more general name StorageException.

This PR only implements the s3 version of BucketStorage so it will work like before. I would like to get this reviewed before providing the Google Cloud Storage implementation in another PR.

It does not deal with how we determine if a bucket is in s3 or in another service. I would also like to discuss this (can move it to a separate issue if you prefer).

The easiest solution would be to just derive the bucket type from the bucket name, but a more robust solution would probably be to add a flag to the mint storage defining what service the bucket belongs to.

What do you think about this?

Lastly regarding the Google Cloud Storage integration. As I understand from working on this, it will cause problems (racing) if you were to run mint-worker in AWS and GCP at the same time. Do you have any idea how we could solve that issue? One solution could be to just run the worker in AWS and then setup a proxy in GCP that can deal with the bucket permissions for google cloud storage, but it's possible not the best solution and I would like to hear your opinion on this matter.

mikkeloscar commented 7 years ago

@harti2006 & @dryewo thanks for the comments about exception handling. Since that code was already there (I only renamed some of it) and it doesn't really relate to this PR, I think it would make sense to address those issues in a separate PR. Are you ok with that?

mikkeloscar commented 7 years ago

@harti2006 mind reviewing the latest changes which uses defmulti instead of defprotocol? :)

harti2006 commented 7 years ago

:+1:

mikkeloscar commented 7 years ago

:+1: