AWS Spring cloud map support (with AWS SDK v2) #506

hariohmprasath · 2022-09-04T10:00:41Z

Support for integrating with AWS Cloudmap

📢 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 properties file integration looks like for both cloudmap registration and discovery

spring.cloud.aws.cloudmap.region=us-east-1
spring.cloud.aws.cloudmap.enabled=true
spring.application.name=cloudmap-namespace-here

# Discover existing cloudmap instances
spring.cloud.aws.cloudmap.discovery.failFast=false
spring.cloud.aws.cloudmap.discovery.discoveryList[0].service=TestService
spring.cloud.aws.cloudmap.discovery.discoveryList[0].nameSpace=ECS-CloudMap

# Register new instance
spring.cloud.aws.cloudmap.registry.description=Namespace for sample cloudmap registry service
spring.cloud.aws.cloudmap.registry.port=80
spring.cloud.aws.cloudmap.registry.service=a-service
spring.cloud.aws.cloudmap.registry.nameSpace=a-namespace

The property file is optional, any application can easily register themselves to cloudmap by adding spring-cloud-starter-aws-cloudmap dependency to pom or gradle files, here is a example:

<dependency>
  <groupId>io.awspring.cloud</groupId>
  <artifactId>spring-cloud-starter-aws-cloudmap</artifactId>
</dependency>

💡 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, EKS fargate instances. 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

Code review and merging code to 3.x branch

Support for integrating with AWS Cloudmap

maciejwalkowiak · 2022-09-05T22:21:53Z

Thanks @hariohmprasath. I'll do some polishing and very likely come back to you with some questions. In meantime, there are 3 things missing:

reference documentation
infrastructure for running a sample - or an instruction how to run the sample (I guess it's not possible to run it with Localstack)
nullability annotations & package-info.java.

maciejwalkowiak

There are quite a few unused fields that I am not sure if just should be deleted or some parts of implementation is still missing.

Missing integration tests stopped me from doing further refactoring. If we can't test against Localstack, we should mock AWS API and perhaps use Wiremock to stub EC2/ECS endpoints.

I feel like the CloudMapUtils class is the center of this implementation and ideally we would refactor it to avoid having so much logic in the util class - but this can or even has to wait until we have integration tests so that we can refactor safely.

@hariohmprasath would you be willing to address these issues? In case I am missing something, your opinion is appreciated.

maciejwalkowiak · 2022-09-06T06:39:46Z

...e/src/main/java/io/awspring/cloud/autoconfigure/cloudmap/CloudMapBootstrapConfiguration.java

+	@Bean
+	@ConditionalOnMissingBean
+	CloudMapAutoRegistration createAutoRegistration(ServiceDiscoveryClient serviceDiscovery) {
+		return new CloudMapAutoRegistration(context, serviceDiscovery, properties.getRegistry());


Instead of ApplicationContext, CloudMapRegistration should use ApplicationEventPublisher for publishing events.

Since it's the only bean that uses is, ApplicationEventPublisher should be provided as a bean factory method parameter instead of a field in the class.

I might need some help on this, do you have a sample for this?

I pushed a fix.

...e/src/main/java/io/awspring/cloud/autoconfigure/cloudmap/CloudMapBootstrapConfiguration.java

...re/src/main/java/io/awspring/cloud/autoconfigure/cloudmap/properties/CloudMapProperties.java

...in/java/io/awspring/cloud/autoconfigure/cloudmap/properties/discovery/CloudMapDiscovery.java

spring-cloud-aws-cloudmap/pom.xml

maciejwalkowiak · 2022-09-06T06:45:48Z

spring-cloud-aws-cloudmap/pom.xml

+	<name>Spring Cloud Cloud Map</name>
+	<description>Spring Cloud AWS Cloud Map</description>
+
+	<dependencies>


These dependencies are not used in spring-cloud-aws-cloudmap module. Since they are required by the autoconfiguration that is triggered only if these dependencies are on the classpath, they should go to starter module and autoconfiguration should trigger only if each of the required dependency is on the classpath.

Sorry can you please elaborate the change, I am not able to understand the recommendation here

Module should have only dependencies used in this very particular module.

Starter should list all the dependencies that are used additionally in autoconfiguration.

Autoconfiguration should be have @ConditionalOnClass listing classes from each dependency that is required for the integration to work.

...-cloud-aws-samples/spring-cloud-aws-cloud-map-sample/src/main/resources/bootstrap.properties

hariohmprasath · 2022-09-07T04:17:48Z

There are quite a few unused fields that I am not sure if just should be deleted or some parts of implementation is still missing.

Missing integration tests stopped me from doing further refactoring. If we can't test against Localstack, we should mock AWS API and perhaps use Wiremock to stub EC2/ECS endpoints.

I feel like the CloudMapUtils class is the center of this implementation and ideally we would refactor it to avoid having so much logic in the util class - but this can or even has to wait until we have integration tests so that we can refactor safely.

@hariohmprasath would you be willing to address these issues? In case I am missing something, your opinion is appreciated.

Thanks for reviewing the code @maciejwalkowiak, let me start addressing some of the comments and we can sync up based on the changes

hariohmprasath · 2022-09-07T21:33:06Z

Reference documentation added, package-info with nullability annotation added, no infrastructure required if cloudmap namespace or service doesnt exist the cloudmap connector module will automatically create them

new test cases, readme, instructions

hariohmprasath · 2022-09-07T22:43:29Z

There are quite a few unused fields that I am not sure if just should be deleted or some parts of implementation is still missing.
Missing integration tests stopped me from doing further refactoring. If we can't test against Localstack, we should mock AWS API and perhaps use Wiremock to stub EC2/ECS endpoints.
I feel like the CloudMapUtils class is the center of this implementation and ideally we would refactor it to avoid having so much logic in the util class - but this can or even has to wait until we have integration tests so that we can refactor safely.
@hariohmprasath would you be willing to address these issues? In case I am missing something, your opinion is appreciated.

Thanks for reviewing the code @maciejwalkowiak, let me start addressing some of the comments and we can sync up based on the changes

@maciejwalkowiak, The last commit that I pushed in has code changes to mock the required HTTP responses. So we can use these tests as a reference for further refactoring. Since CloudMap is not supported in CloudMap you may need to connect with AWS using credentials to run this sample.

It just requires the following environment variables to be set before running the sample application:

AWS_ACCESS_KEY_ID=<key>
AWS_REGION=us-east1
AWS_SECRET_ACCESS_KEY=<secret>
DEPLOYMENT_PLATFORM=ECS

Update CloudMapUtils.getEcsRegistrationAttributes(), comment out line between 536 to 547 and return hard-coded ipAddress and vpcId, this is all you need to pretty much to run this sample.

maciejwalkowiak · 2022-09-09T08:17:27Z

@hariohmprasath thanks for an update. We still need to have an integration test that checks everything all together. Something like ParameterStoreConfigDataLoaderIntegrationTests - does not need to use SpringApplication, can just use Spring testing framework capabilities, but it has to give us confidence that when details change, the integration still works.

Considering it's not possible to use Localstack for that, either we need to mock responses from AWS or run the test against real AWS and spin up resources with Cloudformation. I can imagine this is quite a bit of work but we just cannot merge the integration lacking solid test coverage.

hariohmprasath · 2022-09-13T07:55:06Z

@hariohmprasath thanks for an update. We still need to have an integration test that checks everything all together. Something like ParameterStoreConfigDataLoaderIntegrationTests - does not need to use SpringApplication, can just use Spring testing framework capabilities, but it has to give us confidence that when details change, the integration still works.

Considering it's not possible to use Localstack for that, either we need to mock responses from AWS or run the test against real AWS and spin up resources with Cloudformation. I can imagine this is quite a bit of work but we just cannot merge the integration lacking solid test coverage.

Hi @maciejwalkowiak,
Thanks for reaching out to localstack team regarding the support. I had a chance to catch up on their response and based on that I have submitted an enhancement (testcontainers/testcontainers-java#5838) + PR (testcontainers/testcontainers-java#5839) to support AWS CloudMap for localstack module in testcontainers project.

Once the enhancement and PR gets approved I can use the same to add the required integration test cases. Hope this works for you, let me know your thoughts. Thanks

maciejwalkowiak · 2022-09-13T11:47:00Z

@hariohmprasath sounds like the way to go!

hariohmprasath · 2022-09-22T15:36:52Z

Hi @maciejwalkowiak, Just checking to see whether you had a chance to review the changes, let me know if you have any questions/suggestions. Thanks

maciejwalkowiak · 2022-09-23T04:10:56Z

@hariohmprasath not yet unfortunately but it's still high in my priority list so don't worry - it won't be ignored :-)

hariohmprasath · 2023-03-26T05:14:34Z

Hi @maciejwalkowiak,
Hope you are doing great. Any tentative timeline when this support will be released? Thanks

maciejwalkowiak · 2023-03-26T12:06:55Z

@hariohmprasath apologies again, but we didn't find enough time to include it in 3.0.

3.0 will be released in April, CloudMap is planned for 3.1, so Q2/Q3 2023.

hariohmprasath · 2023-03-27T02:14:37Z

Sounds good, thanks for letting me know

zampettim · 2023-12-20T18:01:14Z

Any updates to when this will be included?

hariohmprasath · 2023-12-20T19:43:28Z

Hi @maciejwalkowiak, Would love to get this merged by early 2024 :)

codefromthecrypt

I saw this and think it is kindof cool, but it will be hard for any community to support unless localstack has a free OSS account. Do you have a relationship with them to ask? I think even if there was such an account it may imply test after merge to prevent key hijacking on pull requests.

I say this not knowing how other amazon resources are managed, so take this advice knowing it is a random drive by.

codefromthecrypt · 2024-03-14T07:12:30Z

.../src/test/java/io/awspring/cloud/autoconfigure/cloudmap/integration/IntegrationTestUtil.java

+	public static final String REGION = "us-east-1";
+	public static final String CIDR_BLOCK = "10.0.0.0/16";
+	public static final String META_DATA_MOCK_RESPONSE = "https://5qjz8e33hj.api.quickmocker.com";
+	public static final String LOCAL_STACK_API = "LOCALSTACK_API_KEY";


since this PR, the key is now LOCALSTACK_AUTH_TOKEN

codefromthecrypt · 2024-03-14T07:12:47Z

.../src/test/java/io/awspring/cloud/autoconfigure/cloudmap/integration/IntegrationTestUtil.java

+	public static final String CIDR_BLOCK = "10.0.0.0/16";
+	public static final String META_DATA_MOCK_RESPONSE = "https://5qjz8e33hj.api.quickmocker.com";
+	public static final String LOCAL_STACK_API = "LOCALSTACK_API_KEY";
+	public static final String LOCAL_STACK_API_KEY = ""; // TODO: Add your localstack api key here


probably wanna read from env and skip if not present

codefromthecrypt · 2024-03-14T07:14:25Z

...st/java/io/awspring/cloud/autoconfigure/cloudmap/integration/EcsCloudMapIntegrationTest.java

+	// Local stack container with Cloud Map and Ec2 services enabled
+	@Container
+	private static final LocalStackContainer localStackContainer = new LocalStackContainer(
+		DockerImageName.parse("localstack/localstack:1.1.0"))


doesn't this need to be localstack-pro? I tried to use normal and it didn't work until I switched the image name. Maybe that changed since this PR was raised (current version is 3.2.0)

codefromthecrypt · 2024-03-14T07:38:06Z

...st/java/io/awspring/cloud/autoconfigure/cloudmap/integration/EksCloudMapIntegrationTest.java

+	 * Register a service with no specification provided in application.properties.
+	 */
+	@Test
+	void registerEksContainerWithCloudMapWithNoSpecification() {


curios on this.. ECS makes immediate sense as at the pure container layer it is basically just DNS. On the other hand EKS is in k8s, so there's services, endpoints etc.

what's your opinion on registration here clashing or otherwise being redundant with k8s (endpoint, service, etc) or layers over it (istio)? Is this to support multi-clusters? or is the idea to duplicate register for those using raw cloud map?

curious your thinking as I'm a cloudmap noob and missing the insight on this.

codefromthecrypt · 2024-03-15T00:37:55Z

my first comment about license with localstack was wrong.. It seems by README and anecdotally in some issues, that there is a pro license for this project.

Just it doesn't seem used in CI not even on protected branches. For example, I would expect an env variable sourced from a repo or org secret. So, the impact is integration tests even when merged, won't run.. unless I'm missing somewhere else where they are run (should be in README somewhere about how the localstack creds are used)

zampettim · 2024-07-25T15:05:23Z

We are interested in using this integration as well. What needs to be done to get this reviewed and merged, and how could we help with that process?

hariohmprasath · 2024-07-25T15:08:57Z

Hi @maciejwalkowiak, The PR was created on Sep 4, 2022, probably 2 years back :) Any plans to merge it part of a mainline or release?

Vostan · 2024-08-12T12:07:22Z

+1

maciejwalkowiak · 2024-09-19T14:23:19Z

Again apologies for postponing it for so long. I'll get some support and we will get it through.

hariohmprasath requested review from maciejwalkowiak, MatejNedic and tomazfernandes as code owners September 4, 2022 10:00

hariohmprasath mentioned this pull request Sep 4, 2022

CloudMap support for spring aws applications #95

Closed

9 tasks

maciejwalkowiak added this to the 3.0.0 M3 milestone Sep 4, 2022

maciejwalkowiak added type: feature Integration with a new AWS service or bigger change in existing integration component: cloudmap labels Sep 4, 2022

hariohmprasath force-pushed the 3.0.x branch from 690b3be to 4465e63 Compare September 4, 2022 23:48

AWS Spring cloud map support

9986358

Support for integrating with AWS Cloudmap

hariohmprasath force-pushed the 3.0.x branch from 4465e63 to 9986358 Compare September 5, 2022 00:56

Fix CloudMap modules in pom.xml, reformat code.

31f4c42

github-actions bot added the type: dependency-upgrade Dependency version bump label Sep 5, 2022

Do not start CloudMap integration for SSM and Secrets Manager tests.

9fac576

maciejwalkowiak requested changes Sep 6, 2022

View reviewed changes

Polish poms

eed8a3e

github-actions bot added the type: documentation Documentation or Samples related issue label Sep 7, 2022

Addressing code review comments

1e5f541

new test cases, readme, instructions

hariohmprasath force-pushed the 3.0.x branch from ab03056 to 1e5f541 Compare September 7, 2022 22:43

hariohmprasath and others added 2 commits September 7, 2022 15:44

Merge branch 'main' into 3.0.x

84d3a9e

Refactor ApplicationContext -> ApplicationEventPublisher.

da461c3

maciejwalkowiak mentioned this pull request Sep 9, 2022

feature request: CloudMap support localstack/localstack#6844

Closed

1 task

hariohmprasath mentioned this pull request Sep 13, 2022

[Enhancement]: Enable CloudMap support in localstack module testcontainers/testcontainers-java#5838

Closed

Code refactoring and integration tests using local containers

f6e8667

Updates to README.md and deployment platform

6efdbee

github-actions bot added component: parameter-store Parameter Store integration related issue component: s3 S3 integration related issue component: secrets-manager Secrets Manager integration related issue labels Sep 22, 2022

maciejwalkowiak modified the milestones: 3.0.0 M3, 3.0.0 M4 Oct 22, 2022

maciejwalkowiak modified the milestones: 3.0.0 M4, 3.1 Jan 4, 2023

maciejwalkowiak modified the milestones: 3.1, 3.1.0 Nov 6, 2023

maciejwalkowiak modified the milestones: 3.1.0, 3.1 Dec 9, 2023

maciejwalkowiak modified the milestones: 3.1.x, 3.x Mar 10, 2024

codefromthecrypt reviewed Mar 14, 2024

View reviewed changes

maciejwalkowiak modified the milestones: 3.x, 3.4.x Sep 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AWS Spring cloud map support (with AWS SDK v2) #506

AWS Spring cloud map support (with AWS SDK v2) #506

hariohmprasath commented Sep 4, 2022

maciejwalkowiak commented Sep 5, 2022

maciejwalkowiak left a comment

maciejwalkowiak Sep 6, 2022

hariohmprasath Sep 7, 2022

maciejwalkowiak Sep 9, 2022

maciejwalkowiak Sep 6, 2022

hariohmprasath Sep 7, 2022

maciejwalkowiak Sep 9, 2022

hariohmprasath commented Sep 7, 2022

hariohmprasath commented Sep 7, 2022

hariohmprasath commented Sep 7, 2022

maciejwalkowiak commented Sep 9, 2022

hariohmprasath commented Sep 13, 2022

maciejwalkowiak commented Sep 13, 2022

hariohmprasath commented Sep 22, 2022

maciejwalkowiak commented Sep 23, 2022

hariohmprasath commented Mar 26, 2023

maciejwalkowiak commented Mar 26, 2023

hariohmprasath commented Mar 27, 2023

zampettim commented Dec 20, 2023

hariohmprasath commented Dec 20, 2023

codefromthecrypt left a comment

codefromthecrypt Mar 14, 2024

codefromthecrypt Mar 14, 2024

codefromthecrypt Mar 14, 2024

codefromthecrypt Mar 14, 2024

codefromthecrypt commented Mar 15, 2024 •

edited

Loading

zampettim commented Jul 25, 2024

hariohmprasath commented Jul 25, 2024

Vostan commented Aug 12, 2024

maciejwalkowiak commented Sep 19, 2024

AWS Spring cloud map support (with AWS SDK v2) #506

Are you sure you want to change the base?

AWS Spring cloud map support (with AWS SDK v2) #506

Conversation

hariohmprasath commented Sep 4, 2022

📢 Type of change

📜 Description

💡 Motivation and Context

💚 How did you test it?

📝 Checklist

🔮 Next steps

maciejwalkowiak commented Sep 5, 2022

maciejwalkowiak 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 Sep 7, 2022

hariohmprasath commented Sep 7, 2022

hariohmprasath commented Sep 7, 2022

maciejwalkowiak commented Sep 9, 2022

hariohmprasath commented Sep 13, 2022

maciejwalkowiak commented Sep 13, 2022

hariohmprasath commented Sep 22, 2022

maciejwalkowiak commented Sep 23, 2022

hariohmprasath commented Mar 26, 2023

maciejwalkowiak commented Mar 26, 2023

hariohmprasath commented Mar 27, 2023

zampettim commented Dec 20, 2023

hariohmprasath commented Dec 20, 2023

codefromthecrypt 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

codefromthecrypt commented Mar 15, 2024 • edited Loading

zampettim commented Jul 25, 2024

hariohmprasath commented Jul 25, 2024

Vostan commented Aug 12, 2024

maciejwalkowiak commented Sep 19, 2024

codefromthecrypt commented Mar 15, 2024 •

edited

Loading