Use AWSKMSClientBuilder #6

austinmoore- · 2016-07-21T15:33:22Z

I decided to make a new KmsMasterKeyProviderBuilder class instead of just using the builder directly inside KmsMasterKeyProvider. There are two reasons for this:

The motivation behind the client builders in the AWS Java SDK was to remove the need to have/use multiple constructors to build an object, and I felt it would be valuable to carry that reasoning over to this library. Using the builder removes the need to search for the correct constructor, and is a bit more expressive.
A separate builder class ensures that code previously written using the old constructors will not break with the introduction of the client builders. I noticed that clients created with the builder are immutable, so any calls to the setters in KmsMasterKeyProvider that change the state of the KMS client (e.g. KmsMasterKeyProvider#setRegion), will throw an exception, and won't change as expected. This way felt safer.

Also, feel free to share thoughts on how the new builder can be tested. I originally added an equals method to KmsMasterKeyProvider, and was going to ensure that both the builder and constructor created objects with the same properties, but because AWSKMSClient does not have equals overriden, this proved to be difficult.

SalusaSecondus · 2016-07-21T16:59:32Z

src/main/java/com/amazonaws/encryptionsdk/kms/KmsMasterKeyProviderBuilder.java

+ *       .build();
+ * </pre>
+ */
+public class KmsMasterKeyProviderBuilder {


This is already missing withMetricsCollector() and withRequestHandlers(). Does it make sense to simply inherit from AwsClientBuilder so that you always get the appropriate (and new) configuration as available? That said, I'm worried that you might need to use reflection (possibly Proxies, which would be ugly) to properly plumb all current and future calls to your inner client builder.

At the very least, please add withMetricsCollector() and withRequestHandlers() and consider more future proof solutions.

pravsingh · 2017-05-14T10:15:09Z

pom.xml

@@ -37,7 +37,7 @@
    <dependency>
      <groupId>com.amazonaws</groupId>
      <artifactId>aws-java-sdk</artifactId>
-      <version>1.10.61</version>
+      <version>1.11.18</version>


Upgrade & Builder change both of the part of the PR. Shouldn't the title say so?

pravsingh · 2017-05-14T10:16:34Z

src/examples/java/com/amazonaws/crypto/examples/BuilderExample.java

@@ -0,0 +1,71 @@
+package com.amazonaws.crypto.examples;


Examples are great however we should prefer unit test cases as they assert the functionality as part of the build and also show the usage of it.

Yes, and we should also just update existing examples to use the new API instead of adding a new bespoke example.

@bdonlan,
Since you are the contributor and know more about this project than I do.
I have commented it in other PR and asking the same question here again.
Do you think we should put examples as part of the main sdk library,
or we should either create separate maven modules or keep them under src/test?
Usually the tendency is that being it an example, developers can put less secured code and insensitive logging or other kinds of code which could be prone to security threats. e.g. example providing public methods or utils which could reveal secrets. This is the one example I can think of right now however there could be more.
just a thought.

Generally I think if we're checking in example code we should make sure it's production quality example code that we want people to imitate. We should also avoid pulling in new dependencies for examples in the main repository. For the more complex example you refer to, since it involves a lot of moving parts (a client component, a server in lambda, a cloudformation template...) it might be preferable to split it off to avoid confusion - or maybe have it as a separate maven subproject in a subdirectory? I'm not enough of a maven expert to know if that is a good approach here though.

agree with you.
I have maven expertise and will create PR so that you can review if it makes sense.

actually aws-encryption-sdk-java dir will have a pom.xml which links to poms inside 2 subdirectories. It looks something like:

aws-encryption-sdk-java\ -> pom.xml //artifact name for this can be aws-encryption-sdk-java-parent -> aws-encryption-sdk-java-core -> pom.xml, src //this generates aws-encryption-sdk-java artifact -> aws-encryption-sdk-java-examples -> pom.xml, src //this generates aws-encryption-sdk-java-examples artifact

this way, the renaming of repo (aws-encryption-sdk-java) will not be required. And the current and future users will not be affected.

created #32

pravsingh · 2017-05-14T10:16:59Z

src/main/java/com/amazonaws/encryptionsdk/kms/KmsMasterKeyProvider.java

+        try {
+            kms_.setRegion(region);
+        } catch (UnsupportedOperationException e) {
+            // Ignored. Not a problem, region should already be set by the builder.


pravsingh · 2017-05-14T10:20:40Z

src/main/java/com/amazonaws/encryptionsdk/kms/KmsMasterKeyProviderBuilder.java

+            return region;
+        } else {
+            final String regionName = regionProvider.getRegion();
+            if (regionName != null) {


instead of using US_WEST_2 as default, could we use Regions.getCurrentRegion()?

Where do you see us using us-west-2 as default?

This looks like a fallback in the case where the SDK's default region selection logic fails, which is reasonable enough.

@SalusaSecondus,
in Regions.java inside aws-java-sdk-core:

/** * The default region that new customers in the US are encouraged to use * when using AWS services for the first time. */ public static final Regions DEFAULT_REGION = US_WEST_2;

@bdonlan agree with you. looks good to me.

pravsingh · 2017-05-14T10:21:58Z

src/test/java/com/amazonaws/services/kms/MockKMSClient.java

@@ -83,7 +93,7 @@
    private Region region_ = Region.getRegion(Regions.DEFAULT_REGION);

    @Override
-    public void createAlias(CreateAliasRequest arg0) throws AmazonServiceException, AmazonClientException {
+    public CreateAliasResult createAlias(CreateAliasRequest arg0) throws AmazonServiceException, AmazonClientException {


I see changes in the contract here.

doesn't it break the existing test cases written by developers in their own applications?

This is not part of the public API as it's in the test directory. It's also responding to breaking SDK API changes (i.e. it's a build fix after the dependency update).

pravsingh

minor improvements needed.

bdonlan

A couple of small changes are needed to leave open the possibility of future improvements to the KMS MKP. Also, this'll need to be rebased before it can be merged.

bdonlan · 2017-09-15T22:20:50Z

src/main/java/com/amazonaws/encryptionsdk/kms/KmsMasterKeyProviderBuilder.java

+ *       .build();
+ * </pre>
+ */
+public class KmsMasterKeyProviderBuilder extends AwsClientBuilder<KmsMasterKeyProviderBuilder, KmsMasterKeyProvider> {


So, one thing I note is that the Python SDK supports using key IDs from multiple regions in a single KMS MKP. I'd prefer to align the Java behavior more closely with the Python behavior in this regard, and we need to make sure the builder API we're creating here is amenable to this.

I believe this is supported with withKeyIds.

withKeyIds does not set a region, so you could use withDefaultRegion, add multiple key IDs, and it should create a KMS MKP for the region you set in withDefaultRegion.

bdonlan · 2017-09-15T22:22:37Z

src/main/java/com/amazonaws/encryptionsdk/kms/KmsMasterKeyProviderBuilder.java

+    /**
+     * Sets the region to be used by the client. Overrides any previously set region.
+     */
+    public KmsMasterKeyProviderBuilder withRegion(Region region) {


I'd prefer to call this withDefaultRegion to leave open the possibility of supporting keys from multiple regions in the same client in future versions.

bdonlan · 2017-09-15T22:23:29Z

src/main/java/com/amazonaws/encryptionsdk/kms/KmsMasterKeyProviderBuilder.java

+    }
+
+    /**
+     * Sets the region using a keyId. This will override any previously set regions.


We should only infer the region from a key ID if the user hasn't manually configured a region, to avoid confusion. The rule is that we'd use withDefaultRegion first, followed by the implied region in withKeyId, followed by the SDK default region.

This sounds good to me. Should we throw an exception if the user tries to use withKeyId after they have already set a default region? Or just continue with the region set with withDefaultRegion?

I'm leaning towards being explicit, and throwing an exception so that there is no unexpected behavior on the user's end. But I could be swayed.

bdonlan · 2017-09-15T22:24:47Z

src/main/java/com/amazonaws/encryptionsdk/kms/KmsMasterKeyProvider.java

@@ -125,7 +125,12 @@ protected KmsMasterKeyProvider(final AWSKMSClient kms, final Region region, fina
        kms_ = kms;


We should @Deprecated the old constructors and encourage folks to move to the builder API.

I'd also suggest a builder() static method on KmsMasterKeyProvider for consistency with other builders in the SDK.

bdonlan · 2017-09-15T22:25:11Z

src/examples/java/com/amazonaws/crypto/examples/BuilderExample.java

@@ -0,0 +1,71 @@
+package com.amazonaws.crypto.examples;


Yes, and we should also just update existing examples to use the new API instead of adding a new bespoke example.

bdonlan · 2017-09-15T22:25:21Z

pom.xml

@@ -37,7 +37,7 @@
    <dependency>
      <groupId>com.amazonaws</groupId>
      <artifactId>aws-java-sdk</artifactId>


This will probably need a rebase.

bdonlan · 2017-09-15T22:32:11Z

I should note that it would be useful to be able to reuse KMS clients, but I don't want to tie us to an API for that until we know what multi-region KMS MKPs would look like.

bdonlan · 2017-09-15T22:34:53Z

Regarding testing, we'll need to figure out how we want to run automated integration tests for this. We have some cross-language compatibility tests that operate on KMS, but I'm not sure if we've put those in a public repository just yet, and we certainly haven't set up a publicly visible test fleet using actual AWS credentials to run KMS calls. When we do that's probably the best place to put tests for client construction.

pravsingh

clear from my side.

…the builder

austinmoore- · 2017-09-20T22:31:44Z

Since this isn't apparent by the commit messages, I've rebased this PR onto master. I've also added 2 new commits: bebe70d, and 40f8624. They're pretty simple, but should still be looked over as I haven't looked at this SDK in a while.

bdonlan · 2017-11-08T01:11:26Z

Thanks for the submission, and sorry for the radio silence - it wasn't obvious that progress was happening on this for a while and it dropped off my raday. In the meantime, I've had some work-in-progress code that did something like this kicking around for a while in my local git repo and finally put it up as PR #40. The main notable differences are that the one I submitted supports contacting multiple regions from the same MKP, and marks a bunch of older methods as now being deprecated. It also includes tests ;) I'd encourage you to check it out and see if it is along the lines of what you wanted to see from the API.

bdonlan · 2017-12-05T18:49:54Z

Closing this as moot at this point as we merged a different version of the builder concept.

SalusaSecondus reviewed Jul 21, 2016
View reviewed changes

pravsingh reviewed May 14, 2017

View reviewed changes

pravsingh suggested changes May 14, 2017

View reviewed changes

mattsb42-aws requested a review from bdonlan September 15, 2017 21:23

bdonlan suggested changes Sep 15, 2017

View reviewed changes

pravsingh mentioned this pull request Sep 19, 2017

refactor examples into its own module #32

Open

pravsingh approved these changes Sep 19, 2017

View reviewed changes

austinmoore- added 6 commits September 20, 2017 17:17

Add KmsMasterKeyProviderBuilder

0b9164a

Add example using KmsMasterKeyProviderBuilder

4a3ae70

Add a warning to some setters that they may not work if created with …

801cab5

…the builder

Extend KmsMasterKeyProviderBuilder from AwsClientBuilder

d58e32c

Add a static builder method on KmsMasterKeyProvider

bebe70d

Change withRegion to withDefaultRegion

40f8624

austinmoore- force-pushed the client-builders branch 2 times, most recently from 8d3813c to 40f8624 Compare September 20, 2017 22:18

bdonlan closed this Dec 5, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use AWSKMSClientBuilder #6

Use AWSKMSClientBuilder #6

austinmoore- commented Jul 21, 2016

SalusaSecondus Jul 21, 2016

pravsingh May 14, 2017

pravsingh May 14, 2017

bdonlan Sep 15, 2017

pravsingh Sep 16, 2017

bdonlan Sep 18, 2017

pravsingh Sep 19, 2017 •

edited

Loading

pravsingh Sep 19, 2017

pravsingh May 14, 2017

pravsingh May 14, 2017

SalusaSecondus May 14, 2017

bdonlan Sep 15, 2017

pravsingh Sep 16, 2017

pravsingh May 14, 2017

bdonlan Sep 15, 2017

pravsingh left a comment

bdonlan left a comment

bdonlan Sep 15, 2017

austinmoore- Sep 20, 2017

bdonlan Sep 15, 2017

bdonlan Sep 15, 2017

austinmoore- Sep 20, 2017

bdonlan Sep 15, 2017

bdonlan Sep 15, 2017

bdonlan Sep 15, 2017

bdonlan commented Sep 15, 2017

bdonlan commented Sep 15, 2017

pravsingh left a comment

austinmoore- commented Sep 20, 2017

bdonlan commented Nov 8, 2017

bdonlan commented Dec 5, 2017

		@@ -125,7 +125,12 @@ protected KmsMasterKeyProvider(final AWSKMSClient kms, final Region region, fina
		kms_ = kms;

Use AWSKMSClientBuilder #6

Use AWSKMSClientBuilder #6

Conversation

austinmoore- commented Jul 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pravsingh Sep 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pravsingh left a comment

Choose a reason for hiding this comment

bdonlan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bdonlan commented Sep 15, 2017

bdonlan commented Sep 15, 2017

pravsingh left a comment

Choose a reason for hiding this comment

austinmoore- commented Sep 20, 2017

bdonlan commented Nov 8, 2017

bdonlan commented Dec 5, 2017

pravsingh Sep 19, 2017 •

edited

Loading