CloudMap support for spring aws applications #95

hariohmprasath · 2021-03-04T01:44:01Z

📢 Type of change

Bugfix
New feature
Enhancement
Refactoring

📜 Description

This pull request adds support for integrating spring boot application with AWS cloudmap. Here is an example of how an simple yaml file integration looks like for both cloudmap registration and discovery

aws:
    cloudmap:
        discovery:
            discoveryList:
            -   healthCheckProtocol: http
                healthCheckResourcePath: /health
                healthCheckThreshold: 5
                service: test-service
                serviceNameSpace: test-namespace
            failFast: true
        enabled: true
        region: us-east-1
        registry:
            description: Namespace for sample cloudmap registry service
            port: 80
            service: service-service
            serviceNameSpace: service-namespace

You can also use annotations to register the spring boot applications to cloudmap

@CloudMapRegistry(
		serviceNameSpace = "dark1-namespace",
		service = "dark1-service",
		port = 80
)
public class TestController {}

Limitation: This pull requests support cloudmap discovery on both EC2 and Fargate based ECS instances, but for service registry currently it only supported for Fargate instance as it uses theECS_CONTAINER_METADATA_URI_V4 URL.

💡 Motivation and Context

Adds support for cloudmap integration with spring boot applications for both service registry and discovery

#5

💚 How did you test it?

I executed the following tests:

Ran unit test cases and made sure nothing failed
Deployed the sample spring boot app checked in part of this pull request in ECS fargate instance. The application could successfully discover and register the application to AWS cloudmap.

📝 Checklist

I reviewed submitted code
I added tests to verify changes
I updated reference documentation to reflect the change
All tests passing
No breaking changes

🔮 Next steps

Complete code review
Add support for cloudmap registration from EC2 instances

eddumelendez

@hariohmprasath thank you so much for the PR! I am really excited about this one TBH and I don't know why. I have submitted some comments to help you organize the modules much better. Please, don't forget to add yourself as an author for the classes that have been touched.

I'm not an expert in cloudmap and left one question to be clear about it. Also, it would be nice to add some javadocs to know more the class responsibility.

...oudmap-config/src/main/java/org/springframework/cloud/aws/cloudmap/AwsCloudMapDiscovery.java

spring-cloud-aws-cloudmap-config/pom.xml

...fig/src/main/java/org/springframework/cloud/aws/cloudmap/AwsCloudMapDiscoveryProperties.java

eddumelendez · 2021-03-04T01:52:20Z

...udmap-config/src/main/java/org/springframework/cloud/aws/cloudmap/AwsCloudMapProperties.java

+
+@ConfigurationProperties(AwsCloudMapProperties.CONFIG_PREFIX)
+
+public class AwsCloudMapProperties {


Keep CloudMapProperties and move this class to autoconfigure module. there should be a package io.awspring.cloud.autoconfigure.cloudmap containing Properties and AutoConfiguration classes

I followed the same model that we have for parameter store, where we have a module for reading config and a starter project for beans and initiation. Is it ok if I keep it the same way? Or do you want me to move it here? Because this might require a lot more refactoring, so thought of checking with you before that.

...udmap-config/src/main/java/org/springframework/cloud/aws/cloudmap/AwsCloudMapProperties.java

eddumelendez · 2021-03-04T01:56:49Z

...p-config/src/main/java/org/springframework/cloud/aws/cloudmap/AwsCloudMapPropertySource.java

+
+import org.springframework.core.env.EnumerablePropertySource;
+
+public class AwsCloudMapPropertySource extends EnumerablePropertySource<AWSServiceDiscovery> {


why a propertysource for cloudmap? sorry, maybe I am missing something but this service is different from parameter store or secrets manager, right? I see this in spring-cloud-consul but it is because consul provides kv store

I second the question

No problem happy to explain. Cloudmap has two parts to it.

Registration --> Where applications can register itself to CloudMap service

Discovery --> Where application can discover other registered instances using namespace & service name. We are using property store to allow discovery of these instances.

Here is an example of a property getting injected with HttpInstanceSummary (in string format) from cloudmap service based on CloudMap namespace and service name using the key "namespace/service"

@Value("${test-namespace/test-service}") private String registryDetails;

You can find this example of this inside [SpringCloudAwsCloudMapSample.java](spring-cloud-aws-samples
/spring-cloud-aws-samples/src/main/java/io/awspring/cloud/cloudmap/sample)

Hope this answers your questions, if you have any further questions, let me know. Happy to explain.

Thanks for the explanation @hariohmprasath. However, I think we should give this more a flavor similar to spring-cloud-consul-discovery module which brings the service registry and discovery.

...map-config/src/main/java/org/springframework/cloud/aws/cloudmap/CloudMapRegistryService.java

...g/src/test/java/org/springframework/cloud/aws/cloudmap/AwsCloudMapAnnotationScannerTest.java

hariohmprasath · 2021-03-04T06:24:50Z

@hariohmprasath thank you so much for the PR! I am really excited about this one TBH and I don't know why. I have submitted some comments to help you organize the modules much better. Please, don't forget to add yourself as an author for the classes that have been touched.
@eddumelendez Thanks a lot for all the support and helping with the review. Really appreciate it. I have responded to your comments and I am happy to answer any further questions you may have.

I'm not an expert in cloudmap and left one question to be clear about it. Also, it would be nice to add some javadocs to know more the class responsibility.
Sure I have added more javadocs. Once the changes are approved, I'm planning to create a blog post with end to end deployment scripts using AWS CDK (which can build a ECS cluster and deploying this application) so it would be easier for anyone to incorporate this in future.

...oud-aws-cloudmap/src/main/java/org/springframework/cloud/aws/cloudmap/CloudMapDiscovery.java

hariohmprasath · 2021-03-08T23:03:38Z

Hi @spencergibb, @eddumelendez, @MatejNedic, @jkuipers,

Hope everyone had a wonderful weekend. I have incorporated everyone's feedback, just want to check whether we are fine in merging this pull request or do we have any feedback that you would like to incorporate. Happy to do so.

Let me know, appreciate all the love and support from this community. Thanks.

vanekjar

Thanks for the integration with AWS Cloud Map. I added a few comments.

...p/src/main/java/org/springframework/cloud/aws/cloudmap/AwsCloudMapPropertySourceLocator.java

...loudmap/src/main/java/org/springframework/cloud/aws/cloudmap/AwsCloudMapPropertySources.java

...s-cloudmap/src/main/java/org/springframework/cloud/aws/cloudmap/CloudMapDiscoverService.java

...oudmap/src/main/java/org/springframework/cloud/aws/cloudmap/CloudMapDiscoveryProperties.java

...s-cloudmap/src/main/java/org/springframework/cloud/aws/cloudmap/CloudMapRegistryService.java

hariohmprasath · 2021-03-09T07:08:06Z

Thanks for the integration with AWS Cloud Map. I added a few comments.

Thanks @vanekjar for all the feedback. I have incorporated the changes. By the way @vanekjar is part of the AWS cloudmap development team, thanks for accepting the request for review and validating the correctness of the integration.

...s-cloudmap/src/main/java/org/springframework/cloud/aws/cloudmap/CloudMapRegistryService.java

hariohmprasath · 2021-03-30T07:25:20Z

Hi @eddumelendez & @vanekjar,
I have taken care of all the requested changes, so following up on this PR to see whether this can get merged to the repository?

Thanks
Hari

eddumelendez · 2021-03-31T02:15:37Z

@hariohmprasath sorry for the delay. I will review on Thursday but still see things to improve. I think we should see at how https://github.com/spring-cloud/spring-cloud-consul/ has been implemented. More details on Thursday.

hariohmprasath · 2021-03-31T05:15:22Z

@hariohmprasath sorry for the delay. I will review on Thursday but still see things to improve. I think we should see at how https://github.com/spring-cloud/spring-cloud-consul/ has been implemented. More details on Thursday.

Thanks, appreciate the help on this

MatejNedic

Hey @hariohmprasath , I left few comments about changes.

.../org/springframework/cloud/aws/autoconfigure/cloudmap/AwsCloudMapBootstrapConfiguration.java

...map/src/test/java/org/springframework/cloud/aws/cloudmap/AwsCloudMapRegisterServiceTest.java

...oud-aws-cloudmap/src/test/java/org/springframework/cloud/aws/cloudmap/CloudMapTestUtils.java

eddumelendez · 2021-04-02T04:33:33Z

@hariohmprasath please make sure this PR only contains your changes. it is hard to review.

hariohmprasath · 2021-04-02T07:11:22Z

@hariohmprasath please make sure this PR only contains your changes. it is hard to review.

@eddumelendez this PR only contains changes related to cloudmap integration. We got lot of good feedback from this community and everything related to this feature has been incorporated part of this PR.

Thanks
Hari

eddumelendez · 2021-04-02T07:15:42Z

check the commits tab, please

hariohmprasath · 2021-04-02T07:26:30Z

check the commits tab, please

There were other changes that were happening in this branch in parallel while this pull request was open. So I have to rebase my changes with the target branch to make it compatible for getting this merged, which has led to other commits getting in the way of this one. If you remember I infact created a discussion to ask a few questions around this.

What do you suggest? I am open to make things easier for you to review so I can take care of the comments. Let me know your thoughts on this. Thanks.

eddumelendez · 2021-04-02T07:32:09Z

after the rebase is done only your commits should be displayed. looks like in the process some commits from other authors keep in your branch. there is no need to update the PR with changes happening in the main branch but this time I will appreciate to do it and only keep your commits, please

hariohmprasath · 2021-04-02T07:46:43Z

after the rebase is done only your commits should be displayed. looks like in the process some commits from other authors keep in your branch. there is no need to update the PR with changes happening in the main branch but this time I will appreciate to do it and only keep your commits, please

Are you fine in reviewing it this time? or do you want me to change the PR. Sorry I am trying to understand your response. Appreciate your quick responses. Thanks

eddumelendez · 2021-04-02T17:00:21Z

@hariohmprasath please, just make sure the branch contains only your commits related to cloudmap integration

hariohmprasath · 2021-04-08T07:21:05Z

please, just make sure the branch contains only your commits related to cloudmap integration

Hi @eddumelendez,
I might need some help on this. Only way I can think of doing this is creating a new fork/branch and merging the cloudmap changes that we have in this pull request on to the new branch. Do you want me to that?

Or do we have a way of doing what you have in this current pull request? I know we are almost there with a lot of reviews done and feedback incorporated frankly I am not an expert with GIT so can you please help with this? Any pointer to how I can do this on the current one will be really helpful.

Thanks in advance

Hari

kdhrubo · 2022-03-08T05:48:14Z

Sounds good @eddumelendez . Do you need testing help? Given that dev is completed - any reviews pending?

hariohmprasath · 2022-04-04T06:41:20Z

Hi @kdhrubo,
Based on @eddumelendez suggestion. I rewrote this integration using spring discovery. You can find a first draft of the PR here:

hariohmprasath@4339b0a

I have reached out @eddumelendez and team for some early peer review before I wrap up the testcases and submit a PR (more polished one :) ) thanks again for your patience.

kdhrubo · 2022-04-04T06:51:28Z

@hariohmprasath thanks. I will try to test the code this week and share my observation. Awesome 👍 work.

hariohmprasath · 2022-04-04T10:20:34Z

@hariohmprasath thanks. I will try to test the code this week and share my observation. Awesome 👍 work.

Thanks @kdhrubo. You can directly reach out to me at this email address [email protected]

MatejNedic

Hey @hariohmprasath, tnx for your contribution!
It is looking good so far.

MatejNedic · 2022-04-04T13:08:35Z

.../org/springframework/cloud/aws/autoconfigure/cloudmap/AwsCloudMapBootstrapConfiguration.java

+	@Bean
+	@ConditionalOnMissingBean
+	AWSServiceDiscovery serviceDiscovery(CloudMapProperties properties) {
+		return createServiceDiscoveryClient(properties);


There is no need for a new method.

Yes, you are right. Removed it in the new version

MatejNedic · 2022-04-04T13:09:04Z

.../org/springframework/cloud/aws/autoconfigure/cloudmap/AwsCloudMapBootstrapConfiguration.java

+public class AwsCloudMapBootstrapConfiguration {
+
+	@Autowired
+	private ApplicationContext context;


Use constructor injection instead

MatejNedic · 2022-04-04T13:09:52Z

.../org/springframework/cloud/aws/autoconfigure/cloudmap/AwsCloudMapBootstrapConfiguration.java

+@ConditionalOnDiscoveryEnabled
+@ConditionalOnBlockingDiscoveryEnabled
+public class AwsCloudMapBootstrapConfiguration {
+


Instead of injecting properties in each method rather inject it through a constructor and reference it.

MatejNedic · 2022-04-04T13:12:52Z

.../org/springframework/cloud/aws/autoconfigure/cloudmap/AwsCloudMapBootstrapConfiguration.java

+ */
+@Configuration(proxyBeanMethods = false)
+@EnableConfigurationProperties(CloudMapProperties.class)
+@ConditionalOnClass({AWSServiceDiscovery.class})


Add ServiceRegistration, CloudMapDiscoveryClient and CloudMapAutoRegistration

those 3 lives in the same library, one of them is just needed in the @ConditionalOnClass. We can add org.springframework.cloud.client.discovery.event.InstanceRegisteredEvent and org.springframework.cloud.client.serviceregistry.AutoServiceRegistration if live in different libraries.

I have added the missing beans as part of the @ConditionalOnClass annotations. But not sure about InstanceRegisteredEvent and AutoServiceRegistration as I am not using it.

...g-cloud-aws-cloudmap/src/main/java/org/springframework/cloud/aws/cloudmap/CloudMapUtils.java

MatejNedic · 2022-04-04T20:43:44Z

spring-cloud-aws-cloudmap/pom.xml

+		<dependency>
+			<groupId>net.oneandone.reflections8</groupId>
+			<artifactId>reflections8</artifactId>
+			<version>0.11.7</version>


Can we extract the version to the project level?

I wonder where it is used or if it is needed for another dependency

I have removed this as it is no longer needed

MatejNedic · 2022-04-04T20:46:58Z

...g-cloud-aws-cloudmap/src/main/java/org/springframework/cloud/aws/cloudmap/CloudMapUtils.java

+	 */
+	public List<String> listServices(final AWSServiceDiscovery serviceDiscovery,
+									 List<CloudMapDiscoveryProperties> discoveryProperties) {
+		final List<String> serviceList = new ArrayList<>();


Instead if mb we can have Assert.notNull(discoveryProperties, "DiscoveryProperties must not be null");`

MatejNedic · 2022-04-04T20:48:44Z

...g-cloud-aws-cloudmap/src/main/java/org/springframework/cloud/aws/cloudmap/CloudMapUtils.java

+						final ServiceFilter serviceFilter = new ServiceFilter().withName(NAMESPACE_ID)
+							.withCondition("EQ").withValues(nameSpaceId);
+						final ListServicesRequest servicesRequest = new ListServicesRequest().withFilters(serviceFilter);
+						if (token != null)


Mb change If with this. It is more readable

Optional.ofNullable(token, String.class)).ifPresent(servicesRequest::withNextToken);

Code updated

MatejNedic · 2022-04-04T20:52:15Z

...g-cloud-aws-cloudmap/src/main/java/org/springframework/cloud/aws/cloudmap/CloudMapUtils.java

+
+			List<NamespaceSummary> namespaceSummaries = namespacesResult.getNamespaces();
+			Optional<String> namespaceId = namespaceSummaries.stream()
+				.filter(n -> n.getName().equals(nameSpace))


Just a question are namespaces case sensitive?

Yes thats correct

MatejNedic · 2022-04-04T20:57:56Z

...oudmap/src/main/java/org/springframework/cloud/aws/cloudmap/http/CloudMapRequestFactory.java

+import java.net.URI;
+
+@Service
+public class CloudMapRequestFactory implements ClientHttpRequestFactory {


Should this be a Bean, if so define it in starters and remove @Service

it should be define in the auto-configure module, right? not in the starter

I have removed this as its not getting used

eddumelendez · 2022-04-04T23:42:08Z

...g-cloud-aws-cloudmap/src/main/java/org/springframework/cloud/aws/cloudmap/CloudMapUtils.java

+			ResponseEntity<String> metaDataResponse = REST_TEMPLATE
+				.getForEntity(System.getenv("ECS_CONTAINER_METADATA_URI_V4") + "/task", String.class);
+			JsonNode root = JSON_MAPPER.readTree(metaDataResponse.getBody());
+			JsonNode jsonNode = root.get("Containers").get(0).get("Networks").get(0);


is this always true? I mean the first item in the container list and the first network? In general I think is ok but just double checking

Good catch. I have created two methods which will be picked up based on the DEPLOYMENT_PLATFORM value ECS

getEcsRegistrationAttributes - Used for ECS

getEksRegistrationAttributes - Used for EKS

eddumelendez · 2022-04-04T23:42:45Z

...g-cloud-aws-cloudmap/src/main/java/org/springframework/cloud/aws/cloudmap/CloudMapUtils.java

+		Map<String, String> attributes = new HashMap<>();
+		try {
+			ResponseEntity<String> metaDataResponse = REST_TEMPLATE
+				.getForEntity(System.getenv("ECS_CONTAINER_METADATA_URI_V4") + "/task", String.class);


the system environment variable is for ECS only right? ehat about services in EC2 or EKS?

Now we have two different methods to handle ECS, EKS. Inside ECS, we have a mechanism to use default metaURL if not available (to handle EC2 use case)

sonarqubecloud · 2022-04-10T08:38:06Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

hariohmprasath · 2022-04-10T08:39:31Z

Hi @eddumelendez, @MatejNedic,

Thanks for the quick response on the code review and for helping with the early feedback. It was super helpful. I have updated the code based on the feedback, additionally tested the integration both in ECS and EKS, with default behavior for EC2. It's working fine. Along with this, I also added unit test cases to validate the code. Please review it and let me know if you have questions, appreciate all the help and support for the team.

eddumelendez

thanks for the update @hariohmprasath ! I have added some suggestions and questions in order to understand a little bit more about the implementation.

Please, take into account the following points

starter should only contain a pom
cloudmap module should not have dependency on spring-boot. So far I have identified it depends on ConfigurationProperties
auto-configuration is only managed at spring-cloud-aws-autoconfigure module

eddumelendez · 2022-04-19T02:47:06Z

spring-cloud-aws-autoconfigure/src/main/resources/META-INF/spring.factories

+org.springframework.cloud.client.discovery.EnableDiscoveryClient=\
+org.springframework.cloud.aws.cloudmap.servicediscovery.CloudMapDiscoveryClient


I was not able to see a similar config on spring-cloud-consul. can you point us to it, please?

You are right, this is not required. So I have removed it.

Hi @eddumelendez,
Please find my responses below:

starter should only contain a pom & auto-configuration is only managed at spring-cloud-aws-autoconfigure module

I followed spring-cloud-starter-aws-parameter-store-config module implementation to begin with and since we have write a discovery and auto-registration client part of spring-cloud-aws-cloudmap module I am unable to move the autoconfiguration to autoconfigure module.

cloudmap module should not have dependency on spring-boot. So far I have identified it depends on ConfigurationProperties

Removed spring-boot dependencies from the module

eddumelendez · 2022-04-19T02:52:37Z

...g-cloud-aws-cloudmap/src/main/java/org/springframework/cloud/aws/cloudmap/CloudMapUtils.java

+	 */
+	INSTANCE;
+
+	public static final String META_DATA_URL = "http://169.254.169.254/latest/meta-data";


I think this is a EC2 metadata url, right? can we rename it to EC2_METADATA_URL

Updated variable name

eddumelendez · 2022-04-19T02:55:30Z

...g-cloud-aws-cloudmap/src/main/java/org/springframework/cloud/aws/cloudmap/CloudMapUtils.java

+ * @author Hari Ohm Prasath
+ * @since 2.3.2
+ */
+public enum CloudMapUtils {


I am also in favor or a class instead of enum

eddumelendez · 2022-04-19T02:56:17Z

...g-cloud-aws-cloudmap/src/main/java/org/springframework/cloud/aws/cloudmap/CloudMapUtils.java

+	public Map<String, String> getRegistrationAttributes() {
+		String deploymentPlatform = System.getenv(DEPLOYMENT_PLATFORM);
+		LOGGER.info("Deployment platform passed in {}", deploymentPlatform);
+		if (StringUtils.hasText(deploymentPlatform) && EKS.equalsIgnoreCase(deploymentPlatform.trim()))


deploymentPlatform can have empty spaces at the end?

Updated CloudMapUtils to a singleton class and fixed space issue

eddumelendez · 2022-04-19T02:58:46Z

...g-cloud-aws-cloudmap/src/main/java/org/springframework/cloud/aws/cloudmap/CloudMapUtils.java

+	 * register instances to cloudmap service.
+	 * @return map containing ip address and vpcid
+	 */
+	public Map<String, String> getRegistrationAttributes() {


in case of ECS is taking eks attributes? I think we can clarify this in the javadoc

I have added additional docs to clarify this, let me know if its still not clear

eddumelendez · 2022-04-19T03:58:12Z

.../src/main/java/org/springframework/cloud/aws/cloudmap/model/discovery/CloudMapDiscovery.java

+	private List<CloudMapDiscoveryProperties> discoveryList;
+
+	public boolean isFailFast() {
+		return failFast;


Suggested change

return failFast;

return this.failFast;

eddumelendez · 2022-04-19T03:58:20Z

.../src/main/java/org/springframework/cloud/aws/cloudmap/model/discovery/CloudMapDiscovery.java

+	}
+
+	public List<CloudMapDiscoveryProperties> getDiscoveryList() {
+		return discoveryList;


Suggested change

return discoveryList;

return this.discoveryList;

eddumelendez · 2022-04-19T03:59:17Z

...map/src/test/java/org/springframework/cloud/aws/cloudmap/AwsCloudMapRegisterServiceTest.java

+ * @author Hari Ohm Prasath
+ * @since 2.3.2
+ */
+public class AwsCloudMapRegisterServiceTest {


Suggested change

public class AwsCloudMapRegisterServiceTest {

public class CloudMapRegisterServiceTest {

eddumelendez · 2022-04-19T04:00:30Z

.../org/springframework/cloud/aws/autoconfigure/cloudmap/AwsCloudMapBootstrapConfiguration.java

+@ConditionalOnProperty(prefix = CloudMapProperties.CONFIG_PREFIX, name = "enabled", matchIfMissing = true)
+@ConditionalOnDiscoveryEnabled
+@ConditionalOnBlockingDiscoveryEnabled
+public class AwsCloudMapBootstrapConfiguration {


Suggested change

public class AwsCloudMapBootstrapConfiguration {

public class CloudMapBootstrapConfiguration {

eddumelendez · 2022-04-19T04:03:20Z

.../org/springframework/cloud/aws/autoconfigure/cloudmap/AwsCloudMapBootstrapConfiguration.java

+
+	public AwsCloudMapBootstrapConfiguration(CloudMapProperties properties, ApplicationContext context) {
+		AWSServiceDiscoveryClientBuilder builder = AWSServiceDiscoveryClientBuilder.standard()
+				.withCredentials(new DefaultAWSCredentialsProviderChain());


can you add .overrideConfiguration(SpringCloudClientConfiguration.clientOverrideConfiguration()) and Optional.ofNullable(this.properties.getEndpoint()).ifPresent(client::endpointOverride); similar to other auto-configurations. Also. Region and Credentials can be injected. See other auto-configurations

Fixed the overrides

Vostan · 2022-06-23T12:59:35Z

Hi, we really seeking this module to reach the main branch, is there something we can do to boost this up?

nstdio · 2022-06-24T08:20:02Z

Thanks for the great effort! We are looking forward to see this PR merged. Is there anything that we can possibly help?

fahime-ghasemi · 2022-07-07T10:59:37Z

Any update on this feature?

sonarqubecloud · 2022-08-30T07:54:15Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

hariohmprasath · 2022-08-30T07:59:27Z

Hi @eddumelendez,
Sorry about my delay in submitting the updates to the PR. Finally I got a chance to take care of all the requested changes, please review it and let me know if you have any questions. Thanks

Vostan · 2022-09-01T10:12:25Z

@maciejwalkowiak If you would have a chance to review, these changes and merge them that would be amazing.

maciejwalkowiak · 2022-09-01T13:37:11Z

I am a bit puzzled right now because since this PR started we got huge progress with migration to AWS SDK v2 in Spring Cloud AWS 3.0.

The options we have are: merge CloudMap to 2.x and release 3.0 without CloudMap with an idea to migrate it to AWS SDK v2 in 3.1, or rebase this PR on the top of main branch migrated to AWS SDK v2 and include it in 3.x at one point - but this would postpone having CloudMap support even further.

@hariohmprasath are you able to estimate the efforts needed to migrate this PR to AWS SDK v2 and if you would be even willing to do it?

Vostan · 2022-09-02T07:36:57Z

I am a bit puzzled right now because since this PR started we got huge progress with migration to AWS SDK v2 in Spring Cloud AWS 3.0.

The options we have are: merge CloudMap to 2.x and release 3.0 without CloudMap with an idea to migrate it to AWS SDK v2 in 3.1, or rebase this PR on the top of main branch migrated to AWS SDK v2 and include it in 3.x at one point - but this would postpone having CloudMap support even further.

@hariohmprasath are you able to estimate the efforts needed to migrate this PR to AWS SDK v2 and if you would be even willing to do it?

Is there some way I can help @hariohmprasath ?

hariohmprasath · 2022-09-04T10:06:58Z

I am a bit puzzled right now because since this PR started we got huge progress with migration to AWS SDK v2 in Spring Cloud AWS 3.0.

The options we have are: merge CloudMap to 2.x and release 3.0 without CloudMap with an idea to migrate it to AWS SDK v2 in 3.1, or rebase this PR on the top of main branch migrated to AWS SDK v2 and include it in 3.x at one point - but this would postpone having CloudMap support even further.

@hariohmprasath are you able to estimate the efforts needed to migrate this PR to AWS SDK v2 and if you would be even willing to do it?

Hi @maciejwalkowiak,
Thanks for going over the options and I can completely understand the timelines involved in releasing 3.x with AWS SDK v2 support. So I took some time over this weekend to rework this feature based on AWS SDK 2.x. I have submitted a new PR (for 3.x branch) with all the required changes, tested it using AWS ECS and everything seems to work fine. Since this PR has already been reviewed by the team, the new PR based on AWS SDK 2.x contains all the requested changes. Hopefully, this will make things easier on your end for planning the release and let me know if you have questions or want me to take care of anything before merging this change. Thanks again for the support from this community.

#506

hariohmprasath · 2022-09-04T10:08:37Z

I am a bit puzzled right now because since this PR started we got huge progress with migration to AWS SDK v2 in Spring Cloud AWS 3.0.
The options we have are: merge CloudMap to 2.x and release 3.0 without CloudMap with an idea to migrate it to AWS SDK v2 in 3.1, or rebase this PR on the top of main branch migrated to AWS SDK v2 and include it in 3.x at one point - but this would postpone having CloudMap support even further.
@hariohmprasath are you able to estimate the efforts needed to migrate this PR to AWS SDK v2 and if you would be even willing to do it?

Is there some way I can help @hariohmprasath ?

Thanks @Vostan for volunteering to help me out on this. Since we have a new PR posted based on AWS SDK 2.x we should be good in releasing this feature on time. I would definitely need your help in trying this feature out and work on improving the integration with AWS CloudMap. Thanks

hariohmprasath · 2022-09-04T10:10:09Z

Hi Maciej, Thanks for going over the options and I can completely understand the timelines involved in releasing 3.x with AWS SDK v2 support. So I took some time over this weekend to rework this feature based on AWS SDK 2.x. I have submitted a new PR (for 3.x branch) <#506> with all the required changes, tested it using AWS ECS and everything seems to work fine. Since this PR has already been reviewed by the team, the new PR based on AWS SDK 2.x contains all the requested changes. Hopefully, this will make things easier on your end for planning the release and let me know if you have questions or want me to take care of anything before merging this change. Thanks again for all the support from this team. #506 <#506>

…

On Thu, Sep 1, 2022 at 6:37 AM Maciej Walkowiak ***@***.***> wrote: I am a bit puzzled right now because since this PR started we got huge progress with migration to AWS SDK v2 in Spring Cloud AWS 3.0. The options we have are: merge CloudMap to 2.x and release 3.0 without CloudMap with an idea to migrate it to AWS SDK v2 in 3.1, or rebase this PR on the top of main branch migrated to AWS SDK v2 and include it in 3.x at one point - but this would postpone having CloudMap support even further. @hariohmprasath <https://github.com/hariohmprasath> are you able to estimate the efforts needed to migrate this PR to AWS SDK v2 and if you would be even willing to do it? — Reply to this email directly, view it on GitHub <#95 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAT6QHJJSULQHQ2N2HSPO6LV4CWRHANCNFSM4YSKINKA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

-- With Regards, Hari Ohm Prasath

maciejwalkowiak · 2022-09-04T10:42:48Z

@hariohmprasath fantastic! Thanks a lot for putting effort into this! I'm scheduling CloudMap support for M3 and do our best to review and try it out on time.

maciejwalkowiak · 2022-09-04T10:43:10Z

Closing in favour of #506

hariohmprasath requested review from eddumelendez and maciejwalkowiak as code owners March 4, 2021 01:44

github-actions bot added the type: dependency-upgrade Dependency version bump label Mar 4, 2021

eddumelendez suggested changes Mar 4, 2021

View reviewed changes

hariohmprasath requested a review from eddumelendez March 4, 2021 06:33

MatejNedic reviewed Mar 5, 2021

View reviewed changes

...oud-aws-cloudmap/src/main/java/org/springframework/cloud/aws/cloudmap/CloudMapDiscovery.java Outdated Show resolved Hide resolved

hariohmprasath requested a review from MatejNedic March 5, 2021 17:30

vanekjar suggested changes Mar 9, 2021

View reviewed changes

vanekjar reviewed Mar 17, 2021

View reviewed changes

...s-cloudmap/src/main/java/org/springframework/cloud/aws/cloudmap/CloudMapRegistryService.java Outdated Show resolved Hide resolved

MatejNedic requested changes Apr 1, 2021

View reviewed changes

hariohmprasath force-pushed the 2.3.x branch from 540afad to ff4a44c Compare April 29, 2021 17:15

First draft of Service discovery

4339b0a

MatejNedic requested changes Apr 4, 2022

View reviewed changes

eddumelendez reviewed Apr 4, 2022

View reviewed changes

hariohmprasath added 3 commits April 10, 2022 01:04

Address code review comments and unit tests

b34b9fc

Updates to logger

a492d72

Checkstyle updates

f1fab5b

eddumelendez reviewed Apr 19, 2022

View reviewed changes

hariohmprasath added 2 commits August 30, 2022 00:45

Incorporating code review comments

4e20c71

Updates to AWS client and overrides

8309f9e

Merge branch 'awspring:2.3.x' into 2.3.x

a130ec4

maciejwalkowiak closed this Sep 4, 2022


		@ConfigurationProperties(AwsCloudMapProperties.CONFIG_PREFIX)

		public class AwsCloudMapProperties {


		import org.springframework.core.env.EnumerablePropertySource;

		public class AwsCloudMapPropertySource extends EnumerablePropertySource<AWSServiceDiscovery> {

		org.springframework.cloud.client.discovery.EnableDiscoveryClient=\
		org.springframework.cloud.aws.cloudmap.servicediscovery.CloudMapDiscoveryClient

	public class AwsCloudMapRegisterServiceTest {
	public class CloudMapRegisterServiceTest {

	public class AwsCloudMapBootstrapConfiguration {
	public class CloudMapBootstrapConfiguration {

CloudMap support for spring aws applications #95

CloudMap support for spring aws applications #95

Conversation

hariohmprasath commented Mar 4, 2021 • edited Loading

📢 Type of change

📜 Description

💡 Motivation and Context

💚 How did you test it?

📝 Checklist

🔮 Next steps

eddumelendez 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

hariohmprasath commented Mar 4, 2021

hariohmprasath commented Mar 8, 2021

vanekjar left a comment

Choose a reason for hiding this comment

hariohmprasath commented Mar 9, 2021 • edited Loading

hariohmprasath commented Mar 30, 2021

eddumelendez commented Mar 31, 2021

hariohmprasath commented Mar 31, 2021

MatejNedic left a comment

Choose a reason for hiding this comment

eddumelendez commented Apr 2, 2021

hariohmprasath commented Apr 2, 2021

eddumelendez commented Apr 2, 2021

hariohmprasath commented Apr 2, 2021

eddumelendez commented Apr 2, 2021

hariohmprasath commented Apr 2, 2021 • edited Loading

eddumelendez commented Apr 2, 2021

hariohmprasath commented Apr 8, 2021

kdhrubo commented Mar 8, 2022

hariohmprasath commented Apr 4, 2022 • edited Loading

kdhrubo commented Apr 4, 2022 • edited Loading

hariohmprasath commented Apr 4, 2022

MatejNedic 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

sonarqubecloud bot commented Apr 10, 2022

hariohmprasath commented Apr 10, 2022

eddumelendez 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

hariohmprasath commented Mar 4, 2021 •

edited

Loading

hariohmprasath commented Mar 9, 2021 •

edited

Loading

hariohmprasath commented Apr 2, 2021 •

edited

Loading

hariohmprasath commented Apr 4, 2022 •

edited

Loading

kdhrubo commented Apr 4, 2022 •

edited

Loading