Split core module to core, aws-auth and aws-core #475

zoewangg · 2018-05-02T17:54:36Z

Description

Splitting core module to auth, core, aws-core and profiles modules.

regions: regions classes
profiles: contains profile classes.
auth: contains credentials, aws signer classes
aws-core: core runtime classes used by aws java clients.
core: core runtime classes.

Dependencies:
profiles -> core
regions -> profiles
auth -> regions
aws-core -> auth
services -> aws-core
Apigatway generated clients -> core
Apigatway generated clients using IAM role -> core, auth

Motivation and Context

Related to #27
Note: This is the first task to move out AWS related classes.
The rest of the tasks left are:

ResponseHandler and Json error response handler classes are still dealing with aws headers and need to be moved to aws-core.
CRC32_FROM_COMPRESSED_DATA_ENABLED needs to be moved to aws-core module once we replace the legacyHttpResponse with SdkHttpFullResponse

Testing

mvn clean install and all integration tests were passed

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)

Checklist

I have read the CONTRIBUTING document
Local run of mvn install succeeds
My code follows the code style of this project
My change requires a change to the Javadoc documentation
I have updated the Javadoc documentation accordingly
I have read the README document
I have added tests to cover my changes
All new and existing tests passed
A short description of the change has been added to the CHANGELOG

License

I confirm that this pull request can be released under the Apache 2 license

kiiadi · 2018-05-03T17:18:06Z

Out of interest why aws-auth / aws-core rather than just auth and core ? we're already bounded by the AWS groupId and it means we have repetition in the package name as well: software.amazon.awssdk.awsauth.credentials for exampl has "aws" in it twice.

zoewangg · 2018-05-03T17:58:07Z

This motivation of splitting core module is to break out AWS related concepts(AWS credentials, AWS signers, etc) to a separate module so that the clients generated from API gateway API just needs to depend on core module rather than pulling the whole AWS-specific logic.

As to the naming, using aws-auth makes is clear that it contains AWS related authentication classes and applies to all AWS services. The clients generated from API gateway don't need to depend on it unless they use IAM roles for authentication.

I thought about updating core to sdk-core to differentiate from aws-core, but don't have strong opinions and open to discussion.

kiiadi · 2018-05-03T18:06:21Z

I suppose that makes the thought process a little clearer - but from a customer perspective do you think they're going to notice the subtly between software.amazon.awssdk:auth and software.amazon.awssdk:aws-auth? As I say, we're already in the awssdk groupId.

shorea

Still working my way through this. I'll revisit after lunch.

shorea · 2018-05-03T18:00:38Z

aws-auth/src/main/java/software/amazon/awssdk/awsauth/AwsExecutionAttributes.java

@@ -32,39 +34,14 @@
                     + "4. The remaining public AWS stuff should be moved to the AWS module. "
                     + "We should also consider making some of the SDK/AWS-owned set of attributes part of the immutable context "
                     + "if we don't want the interceptors to modify them.")
-public final class AwsExecutionAttributes {
+public interface AwsExecutionAttributes extends SdkExecutionAttributes {


This seems to be more of an awscore thing rather than auth specific.

This is used by Aws4Signer for now to get the credentials from ExecutionAttributes. I guess this will be moved around once we start working on signer refactor.

Shirley the awsauth module should depend on awscore?

shorea · 2018-05-03T18:01:30Z

aws-auth/src/main/java/software/amazon/awssdk/awsauth/credentials/AwsSystemSetting.java

@@ -43,9 +35,8 @@
 public enum AwsSystemSetting implements SystemSetting {


Belongs in awscore

This is used by some credential providers, eg:SystemSettingsCredentialsProvider is using it to locate the credentials.

shorea · 2018-05-03T18:03:05Z

aws-auth/src/main/java/software/amazon/awssdk/awsauth/credentials/HttpCredentialsProvider.java

@@ -132,7 +132,7 @@ public void close() {
        private String asyncThreadName;

        /**
-         * Created using {@link #builder()}.
+         * Created using {@link #build()}.


Nitpicky but this seemed more correct before. Comment is mostly useless though anyways.

yup, removed

shorea · 2018-05-03T18:05:44Z

aws-auth/src/main/java/software/amazon/awssdk/awsauth/credentials/internal/ConnectionUtils.java

@@ -13,7 +13,7 @@
 * permissions and limitations under the License.
 */

-package software.amazon.awssdk.core.internal.net;
+package software.amazon.awssdk.awsauth.credentials.internal;


For ConnectionUtils we should consider refactoring to use the SDK HTTP client instead of URL connection. Also we should consider putting the ec2 metadata "client" into it's own module.

added ReviewBeforeRelease on it

shorea · 2018-05-03T18:07:08Z

aws-auth/src/main/java/software/amazon/awssdk/awsauth/credentials/profile/Profile.java

@@ -13,7 +13,7 @@
 * permissions and limitations under the License.
 */

-package software.amazon.awssdk.auth.profile;
+package software.amazon.awssdk.awsauth.credentials.profile;


The profile stuff may be better place in it's own module. It isn't just credentials and region and we may expand on it in the future to include other profile configuration.

shorea · 2018-05-03T18:08:19Z

aws-auth/src/main/java/software/amazon/awssdk/awsauth/regions/Region.java

@@ -13,7 +13,7 @@
 * permissions and limitations under the License.
 */

-package software.amazon.awssdk.core.regions;
+package software.amazon.awssdk.awsauth.regions;


Auth and region are related but not necesarrily coupled. Should region related stuff be in a separate module?

Yes, we can further split aws-auth module to region, profile, credentials, signer, I'll try and see how it looks like, not sure if they have circular dependency right now.

profile classes are moved to profiles module via commit 70825f4.

As to regions and credential classes, they are tightly coupled now because they share common functionalities to to retrieve regions/creds from local EC2 instances in HttpCredentialUtils, endpointRetry and other classes, unless we break out those class to a separate module, it's difficult to separate them. If we are going with that route, we might end up having plenty "micro" modules and it could increase our maintenance burden and unlike profiles, regions and creds seem to be always used together.

shorea · 2018-05-03T18:35:13Z

aws-auth/src/main/java/software/amazon/awssdk/awsauth/signer/Aws4Signer.java

        }
        return expirationInSeconds;
    }

+    private boolean isAnonymous(AwsCredentials credentials) {


Why inline this but not in query string signer? Should this be moved up to abstract?

good catch, fixed

shorea · 2018-05-03T18:36:28Z

aws-auth/src/main/java/software/amazon/awssdk/awsauth/signer/SdkClock.java

@@ -13,7 +13,7 @@
 * permissions and limitations under the License.
 */

-package software.amazon.awssdk.core.auth;


There may be other places where we'd want to mock the clock. This can be moved up to the generic core.

shorea · 2018-05-03T18:37:54Z

aws-auth/src/main/java/software/amazon/awssdk/awsauth/signer/SignerConstants.java

@@ -13,7 +13,7 @@
 * permissions and limitations under the License.
 */

-package software.amazon.awssdk.core.auth.internal;
+package software.amazon.awssdk.awsauth.signer;


Can you sync up with @varunnvs92 about the signer refactoring? I'd imagine there will be another module like signer-spi that has just the interfaces for signing and awsauth contains the AWS specific implementations like SigVn

shorea · 2018-05-03T18:40:48Z

aws-auth/src/main/java/software/amazon/awssdk/awsauth/util/EndpointUtils.java


 import java.net.URI;
 import software.amazon.awssdk.annotations.SdkInternalApi;
-import software.amazon.awssdk.core.regions.Region;
-import software.amazon.awssdk.core.runtime.endpoint.DefaultServiceEndpointBuilder;
+import software.amazon.awssdk.awsauth.regions.Region;


EndpointUtils feels more like awscore than auth.

shorea · 2018-05-03T18:42:02Z

aws-auth/src/test/java/software/amazon/awssdk/awsauth/signer/Aws4SignerTest.java

@@ -51,7 +53,7 @@ public void testSigning() throws Exception {
                "Signature=e73e20539446307a5dc71252dbd5b97e861f1d1267456abda3ebd8d57e519951";


-        AwsCredentials credentials = new AwsCredentials("access", "secret");
+        AwsCredentials credentials = AwsCredentials.create("access", "secret");


How was this compiling before? Was the constructor protected? If so should we make it private?

the constructor is protected and this class was in the same package with AwsCredentials. can't make is private since it has child classes.

shorea · 2018-05-03T19:12:56Z

core/src/main/java/software/amazon/awssdk/core/client/SdkAsyncClientHandlerImpl.java

+final class SdkAsyncClientHandlerImpl extends BaseAsyncClientHandler implements AsyncClientHandler {
+
+    @ReviewBeforeRelease("Should this be migrated to use a params object, particularly because it crosses module boundaries?" +
+                         "We might also need to think about how it will work after AWS/SDK are split.")


Is this comment now addressed?

shorea · 2018-05-03T19:17:48Z

core/src/main/java/software/amazon/awssdk/core/client/builder/ClientAsyncHttpConfiguration.java

@@ -68,7 +68,7 @@ private ClientAsyncHttpConfiguration(DefaultHttpConfigurationBuilder builder) {
     * to ease resolution of the client. Returns an empty {@link Optional} if neither is set.
     */
    @SdkInternalApi
-    Optional<Either<SdkAsyncHttpClient, SdkAsyncHttpClientFactory>> toEither() {
+    public Optional<Either<SdkAsyncHttpClient, SdkAsyncHttpClientFactory>> toEither() {


Hmm I don't like this. Is there anything we can do about it? Maybe a ReviewBeforeRelease annotation as I believe we want to flatten this out anyways which could make this cleaner.

Good catch. public was added in the middle of the work before I moved the duplicate codes to parent class and I forgot to remove it. Fixed

shorea · 2018-05-03T19:24:12Z

core/src/main/java/software/amazon/awssdk/core/http/AmazonSyncHttpClient.java

@@ -55,15 +55,15 @@

 @ThreadSafe
 @SdkInternalApi
-public class AmazonHttpClient implements SdkAutoCloseable {


Can you add a ReviewBeforeRelease annotation to come up with a better name for this and the async variant?

shorea · 2018-05-03T19:27:09Z

core/src/main/java/software/amazon/awssdk/core/http/pipeline/stages/SigningStage.java

-    private boolean shouldSign(Signer signer, AwsCredentials credentials) {
-        return signer != null && (credentials != null || signer instanceof CanHandleNullCredentials);
+    private boolean shouldSign(Signer signer) {
+        return signer != null;


This seems like a behavior change. Is this okay?

I removed the credential check because core is no long aware of AwsCredentials. Will add a note here to add back this logic when we start working on signer refactor

shorea · 2018-05-03T19:28:12Z

core/src/main/java/software/amazon/awssdk/core/interceptor/SdkExecutionAttributes.java

+     * The key under which the request config is stored.
+     */
+    @ReviewBeforeRelease("RequestConfig feels pretty internal. Can we just expose parts of it?")
+    ExecutionAttribute<RequestOverrideConfig> REQUEST_CONFIG = new ExecutionAttribute<>("RequestConfig");


This comment doesn't seem to apply anymore since RequestOverrideConfig is a public class.

shorea · 2018-05-04T04:20:03Z

This may be out of scope of this review but I think the other couplings we have to AWS in core is the retry related stuff. How we detect throttling and clock skew is a bit AWS specific (moreso throttling).

I think there is also some coupling in the marshalling/unmarshalling paths. Particularly around the response handlers and error response handlers.

zoewangg · 2018-05-04T04:30:57Z

Yes, this PR is just the first step and there are still some remaining AWS functionality such as parsing aws headers in response handler, stax response marshaller, retry related stuff as you mentioned. To keep PR smaller(it's already huge as it is..), I will create separate PR for each of the remaining task.

…2cb15bf8 Pull request: release <- staging/416f5504-caf9-45ad-a842-345f2cb15bf8

zoewangg force-pushed the CoreModuleBreakDown branch from 0f4bd61 to 5747b25 Compare May 3, 2018 16:42

shorea reviewed May 3, 2018

View reviewed changes

This was referenced May 7, 2018

Move profile classes to profile module #480

Merged

Move AWS speicifc Unmarshaller/Mashallers to aws-core module #482

Closed

zoewangg force-pushed the CoreModuleBreakDown branch 3 times, most recently from e4401a5 to 7231034 Compare May 11, 2018 21:17

Split core module to profiles, regions, auth, aws-core and core modules.

77ded43

zoewangg force-pushed the CoreModuleBreakDown branch from 7231034 to 77ded43 Compare May 11, 2018 21:30

shorea approved these changes May 11, 2018

View reviewed changes

zoewangg merged commit 2bb7e5b into master May 11, 2018

zoewangg deleted the CoreModuleBreakDown branch May 11, 2018 22:44

zoewangg mentioned this pull request Jun 11, 2018

Remove AWS-specific code from the "core" module #27

Closed

dagnir mentioned this pull request Jun 27, 2018

OSGI Bundle #59

Open

aws-sdk-java-automation added a commit that referenced this pull request Apr 16, 2019

Merge pull request #475 from aws/staging/416f5504-caf9-45ad-a842-345f…

5568f99

…2cb15bf8 Pull request: release <- staging/416f5504-caf9-45ad-a842-345f2cb15bf8

		@@ -43,9 +35,8 @@
		public enum AwsSystemSetting implements SystemSetting {

Split core module to core, aws-auth and aws-core #475

Split core module to core, aws-auth and aws-core #475

Conversation

zoewangg commented May 2, 2018 • edited Loading

Description

Motivation and Context

Testing

Types of changes

Checklist

License

kiiadi commented May 3, 2018

zoewangg commented May 3, 2018

kiiadi commented May 3, 2018

shorea 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shorea commented May 4, 2018

zoewangg commented May 4, 2018

zoewangg commented May 2, 2018 •

edited

Loading