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.

Sign up for GitHub

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

Jump to bottom

Add support for endpoint_url for local dynamodb table #300

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Add support for endpoint_url for local dynamodb table #300

wants to merge 2 commits into from

Conversation

vaibhavkhurana2018
Copy link

We will like to use credstash with the local dynamodb table created using https://github.com/localstack/localstack.

This PR adds support for adding endpoint_url while making the session to connect to the local dynamodb table rather than the remote AWS service.

It accepts the endpoint_url as an argument or via an environment variable DYNAMODB_ENDPOINT_URL, defaulting to None.

Have tested all the functions both with and without the endpoint_url. This will be a non-breaking change.

@vaibhavkhurana2018
Copy link
Author

vaibhavkhurana2018 commented Feb 11, 2021 •
edited
Loading

Create Table:

./credstash.py -t testing -r us-east-1 --endpoint_url http://localhost:4566 setup
Creating table...
Waiting for table to be created...
Adding tags...
Table has been created. Go read the README about how to create your KMS key

Put:

./credstash.py -t testing -r us-east-1 --endpoint_url http://localhost:4566 put test 'test'
test has been stored

List:

./credstash.py -t testing -r us-east-1 --endpoint_url http://localhost:4566 list
test -- version 0000000000000000001 -- comment

Get:

./credstash.py -t testing -r us-east-1 --endpoint_url http://localhost:4566 get test
test

GetAll:

./credstash.py -t testing -r us-east-1 --endpoint_url http://localhost:4566 getall
{
    "test": "test"
}

Localstack Running on Local:

CONTAINER ID        IMAGE                   COMMAND                  CREATED             STATUS              PORTS                                                                              NAMES
449fb6534869        localstack/localstack   "docker-entrypoint.sh"   About an hour ago   Up About an hour    0.0.0.0:4566->4566/tcp, 0.0.0.0:4571->4571/tcp, 0.0.0.0:8080-8081->8080-8081/tcp   localstack_main

@SamCullin
Copy link

Would love for this to go through

SamCullin
SamCullin approved these changes Apr 29, 2021
View reviewed changes
Copy link

@SamCullin SamCullin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Been using this on my local for the past day and works well

@vaibhavkhurana2018
Copy link
Author

Thanks @SamCullin !!

jason-fugue
jason-fugue requested changes May 7, 2021
View reviewed changes
Copy link
Contributor

@jason-fugue jason-fugue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! This is an awesome idea. I tried this out with localstack and it works great. I'm going to look into doing the same for KMS for offline testing.

I left one very minor suggestion about the new arg. Could you please update when you have time?

credstash.py Outdated
@@ -907,6 +911,14 @@ def get_parser():
"CREDSTASH_DEFAULT_TABLE env variable, "
"or if that is not set, the value "
"`credential-store` will be used")
parsers['super'].add_argument("--endpoint_url", default=os.environ.get("DYNAMODB_ENDPOINT_URL", None),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Super-small issue - could you please change this to --endpoint-url to be consistent with the other args?

Suggested change
parsers['super'].add_argument("--endpoint_url", default=os.environ.get("DYNAMODB_ENDPOINT_URL", None),
parsers['super'].add_argument("--endpoint-url", default=os.environ.get("DYNAMODB_ENDPOINT_URL", None),

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done 👍

@SamCullin
Copy link

One thing that may be an issue is that people may expect the --endpoint-url to also update the KMS endpoint. Maybe something like --dynamo-endpoint-url might be more appropriate. So then if someone wants to add --kms-endpoint-url in the future they can.

@jason-fugue
Copy link
Contributor

One thing that may be an issue is that people may expect the --endpoint-url to also update the KMS endpoint. Maybe something like --dynamo-endpoint-url might be more appropriate. So then if someone wants to add --kms-endpoint-url in the future they can.

This is a good point - and matching the env variable name would also be intuitive --dynamodb-endpoint-url. @vaibhavkhurana2018 I apologize that your PR has sat for so long already, but if this is something that you could do it would be appreciated. Otherwise, I can take care of it after the merge.

I'll create a followup issue to add similar support for the KMS endpoint.

@jason-fugue jason-fugue mentioned this pull request May 10, 2021
Open
@vertig0ne
Copy link

This PR was needed for a use case I had, I had to make further changes to get it working, one typographic error and also add in KMS to the endpoint_url.

diff --git a/credstash.py b/credstash.py
index aa8735b..c78a555 100755
--- a/credstash.py
+++ b/credstash.py
@@ -329,7 +329,7 @@ def putSecret(name, secret, version="", kms_key="alias/credstash",
         if dynamodb is None:
             dynamodb = session.resource('dynamodb', region_name=region, endpoint_url=endpoint_url)
         if kms is None:
-            kms = session.client('kms', region_name=kms_region or region)
+            kms = session.client('kms', region_name=kms_region or region, endpoint_url=endpoint_url)
 
     key_service = KeyService(kms, kms_key, context)
     sealed = seal_aes_ctr_legacy(
@@ -565,7 +565,7 @@ def getSecret(name, version="", region=None, endpoint_url=None, table="credentia
         if dynamodb is None:
             dynamodb = session.resource('dynamodb', region_name=region, endpoint_url=endpoint_url)
         if kms is None:
-            kms = session.client('kms', region_name=kms_region or region)
+            kms = session.client('kms', region_name=kms_region or region, endpoint_url=endpoint_url)
 
     secrets = dynamodb.Table(table)
 
@@ -1112,7 +1112,7 @@ def main():
     # test for region
     try:
         region = args.region
-        endpoint_url = args.endpoint-url
+        endpoint_url = args.endpoint_url
         session = get_session(**session_params)
         session.resource('dynamodb', region_name=region, endpoint_url=endpoint_url)
     except botocore.exceptions.NoRegionError:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jason-fugue jason-fugue jason-fugue requested changes

@SamCullin SamCullin SamCullin approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@vaibhavkhurana2018 @SamCullin @jason-fugue @vertig0ne