Created the Configuration class for Ec2Metadata #3265

srsaikumarreddy · 2022-06-22T22:28:04Z

Created a Client Class . Implemented the Builder Interface and the entire build functionality to create client Objects.

Motivation and Context

Implemented the Builder Interface and the entire build functionality to create client Objects.

Modifications

Implemented the Builder Interface and the entire build functionality to create client Objects.

Testing

Wrote Unit cases to test the multiple methods

Screenshots (if appropriate)

Types of changes

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

Checklist

[ X] I have read the CONTRIBUTING document
[ X] Local run of mvn install succeeds
[ X] 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
[ X] I have added tests to cover my changes
[ X] All new and existing tests passed
I have added a changelog entry. Adding a new entry must be accomplished by running the scripts/new-change script and following the instructions. Commit the new file created by the script in .changes/next-release with your changes.
My change is to implement 1.11 parity feature and I have updated LaunchChangelog

License

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

dagnir · 2022-06-23T16:25:18Z

core/imds/pom.xml

+        <dependency>
+            <groupId>software.amazon.awssdk</groupId>
+            <artifactId>url-connection-client</artifactId>
+            <version>2.17.209-SNAPSHOT</version>


Replace with ${awsjavasdk.version}

dagnir · 2022-06-23T16:28:49Z

core/imds/src/main/java/software/amazon/awssdk/imds/Ec2Metadata.java

+    /**
+     * @return The number of retry attempts for any failed request.
+     */
+    Integer retries();
+
+    /**
+     * @return The endpoint of IMDS.
+     */
+    String endpoint();
+
+    /**
+     * @return The port for the endpoint.
+     */
+    Integer port();
+
+    /**
+     * @return The Time to live (TTL) of the token.
+     */
+    Integer tokenTtl();
+
+    /**
+     * @return The endpoint mode of IMDS.Supported values include IPv4 and IPv6.
+     */
+    String endpointMode();
+
+    /**
+     * @return The number of seconds to wait for the connection to open.
+     */
+    Integer httpOpenTimeout();
+
+    /**
+     * @return The number of seconds for one chunk of data to be read.
+     */
+    Integer httpReadTimeout();
+
+    /**
+     * @return The output stream for debugging.
+     */
+    String httpDebugOutput();
+
+    /**
+     * @return The number of seconds to sleep in-between retries.
+     */
+    Integer backoff();
+
+    /**
+     * @return The SdkHttpClient instance to make the http requests.
+     */
+    SdkHttpClient httpClient();


Do we actually need getters for all of this? I would recommend leaving most if not all of them out for now. In general, this is probably information that the user will already know (because they configured it), or not really care about if they're getting the client from something else that created it for them.

dagnir · 2022-06-23T16:31:01Z

core/imds/src/main/java/software/amazon/awssdk/imds/Ec2Metadata.java

+         * @param retries The number of retry attempts for any failed request.
+         * @return Returns a reference to this builder
+         */
+        Builder retries(Integer retries);


We should be using something like (but maybe a simplified version) of RetryPolicy:

aws-sdk-java-v2/core/sdk-core/src/main/java/software/amazon/awssdk/core/retry/RetryPolicy.java

Line 47 in b3ff6d5

public final class RetryPolicy implements ToCopyableBuilder<RetryPolicy.Builder, RetryPolicy> {

.

dagnir · 2022-06-23T16:31:28Z

core/imds/src/main/java/software/amazon/awssdk/imds/Ec2Metadata.java

+         * @param endpoint The endpoint of IMDS.
+         * @return Returns a reference to this builder
+         */
+        Builder endpoint(String endpoint);


endpoint should probably be a URI

dagnir · 2022-06-23T16:32:10Z

core/imds/src/main/java/software/amazon/awssdk/imds/Ec2Metadata.java

+         * @param tokenTtl The Time to live (TTL) of the token.
+         * @return Returns a reference to this builder
+         */
+        Builder tokenTtl(Integer tokenTtl);


Should be Duration

dagnir · 2022-06-23T16:32:26Z

core/imds/src/main/java/software/amazon/awssdk/imds/Ec2Metadata.java

+         * @param port The port for the endpoint.
+         * @return Returns a reference to this builder
+         */
+        Builder port(Integer port);


Why boxed time here? Can it just be int?

dagnir · 2022-06-23T16:33:05Z

core/imds/src/main/java/software/amazon/awssdk/imds/Ec2Metadata.java

+        Builder httpOpenTimeout(Integer httpOpenTimeout);
+
+        /**
+         * Define the number of seconds for one chunk of data to be read.
+         *
+         * @param httpReadTimeout The number of seconds for one chunk of data to be read.
+         * @return Returns a reference to this builder
+         */
+        Builder httpReadTimeout(Integer httpReadTimeout);


These are configs that are already handled at the SdkHttpClient level

dagnir · 2022-06-23T16:33:21Z

core/imds/src/main/java/software/amazon/awssdk/imds/Ec2Metadata.java

+         * @param httpDebugOutput TThe output stream for debugging.
+         * @return Returns a reference to this builder
+         */
+        Builder httpDebugOutput(String httpDebugOutput);


I'm not sure what this

dagnir · 2022-06-23T16:34:12Z

core/imds/src/test/java/software/amazon/awssdk/imds/Ec2MetadataTest.java

+    @Test
+    public void verify_not_equal_objects(){
+
+        Ec2Metadata client = Ec2Metadata.builder().retries(2).build();
+        Ec2Metadata anotherClient = Ec2Metadata.builder().build();
+        assertThat(client.retries()).isNotEqualTo(anotherClient.retries());
+    }
+
+    @Test
+    public void verify_hashcode_equal(){
+
+        Ec2Metadata client = Ec2Metadata.builder().build();
+        Ec2Metadata anotherClient = Ec2Metadata.builder().build();
+        assertThat(client.hashCode()).isEqualTo(anotherClient.hashCode());
+    }
+


Use equalsVerifier

aws-sdk-java-v2/services-custom/s3-transfer-manager/src/test/java/software/amazon/awssdk/transfer/s3/internal/DefaultCopyTest.java

Line 25 in 1032916

EqualsVerifier.forClass(DefaultCopy.class)

Did not know about it. Will implement

zoewangg · 2022-06-24T18:07:55Z

core/imds/src/main/java/software/amazon/awssdk/imds/Ec2Metadata.java

+        /**
+         * Define the Time to live (TTL) of the token.
+         *
+         * @param tokenTtl The Time to live (TTL) of the token.
+         * @return Returns a reference to this builder
+         */
+        Builder tokenTtl(Duration tokenTtl);


I believe token ttl is not something configurable and is provided by IMDS

The SEP mentions to have the tokenTtl functionality support

zoewangg · 2022-06-24T18:09:29Z

core/imds/src/main/java/software/amazon/awssdk/imds/Ec2Metadata.java

+         * @param backoff The number of seconds to sleep in-between retries.
+         * @return Returns a reference to this builder
+         */
+        Builder backoff(Integer backoff);


Isn't this included in the retryPolicy?

+1 we should remove this

The SEP mentions that the if the user mentions the backoff , it should be an override. Would it be better to have a field for it as it would be convenient for user ?

zoewangg · 2022-06-24T18:10:23Z

core/imds/src/main/java/software/amazon/awssdk/imds/Ec2Metadata.java

+
+    /**
+     * @return The retry policy which includes the number of retry attempts for any failed request.
+     */
+    RetryPolicy retryPolicy();
+
+    /**
+     * @return The endpoint of IMDS.
+     */
+    URI endpoint();
+
+    /**
+     * @return The port for the endpoint.
+     */
+    Integer port();
+
+    /**
+     * @return The Time to live (TTL) of the token.
+     */
+    Duration tokenTtl();
+
+    /**
+     * @return The endpoint mode of IMDS.Supported values include IPv4 and IPv6.
+     */
+    String endpointMode();
+
+    /**
+     * @return The output stream for debugging.
+     */
+    String httpDebugOutput();
+
+    /**
+     * @return The number of seconds to sleep in-between retries.
+     */
+    Integer backoff();
+
+    /**
+     * @return The SdkHttpClient instance to make the http requests.
+     */
+    SdkHttpClient httpClient();


Do we need to expose getters for the settings?

I followed the client configuration.md file , it had getters there.

zoewangg · 2022-06-24T18:11:06Z

core/imds/src/main/java/software/amazon/awssdk/imds/Ec2Metadata.java

+         * @param endpointMode The endpoint mode of IMDS.Supported values include IPv4 and IPv6.
+         * @return Returns a reference to this builder
+         */
+        Builder endpointMode(String endpointMode);


Should we use enum for the endpoint mode so users can easily choose from?

Makes sense. Will change it

zoewangg · 2022-06-24T18:12:54Z

core/imds/src/main/java/software/amazon/awssdk/imds/Ec2Metadata.java

+         * @param port The port for the endpoint.
+         * @return Returns a reference to this builder
+         */
+        Builder port(Integer port);


This is probably not needed because it should be supplied as part of URI in endpoint

zoewangg · 2022-06-28T16:55:08Z

core/imds/src/main/java/software/amazon/awssdk/imds/internal/EndpointMode.java

+    IPv4,
+    IPv6


Enum should use all caps. IPV4, IPV6

sonarqubecloud · 2022-06-28T18:38:55Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
2 Code Smells

26.9% Coverage
0.0% Duplication

srsaikumarreddy requested a review from a team as a code owner June 22, 2022 22:28

dagnir reviewed Jun 23, 2022

View reviewed changes

srsaikumarreddy force-pushed the saikred/client_configuration branch from 5c7a498 to 7720343 Compare June 24, 2022 03:10

zoewangg reviewed Jun 24, 2022

View reviewed changes

Created the Configuration class for Ec2Metadata

e873bc2

srsaikumarreddy force-pushed the saikred/client_configuration branch from 7720343 to e873bc2 Compare June 25, 2022 01:23

zoewangg reviewed Jun 28, 2022

View reviewed changes

dagnir approved these changes Jun 28, 2022

View reviewed changes

Added CR comments in EndpointMode.java

b366811

zoewangg approved these changes Jun 28, 2022

View reviewed changes

zoewangg enabled auto-merge (squash) June 28, 2022 17:48

zoewangg merged commit 88902b9 into aws:feature/master/imds Jun 28, 2022

Created the Configuration class for Ec2Metadata #3265

Created the Configuration class for Ec2Metadata #3265

Uh oh!

Conversation

srsaikumarreddy commented Jun 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation and Context

Modifications

Testing

Screenshots (if appropriate)

Types of changes

Checklist

License

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Jun 28, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

srsaikumarreddy commented Jun 22, 2022 •

edited

Loading