AWS Spring cloud map support (with AWS SDK v2)

[hariohmprasath]

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

[maciejwalkowiak]

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]

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.

[hariohmprasath]

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

[maciejwalkowiak]

I pushed a fix.

[maciejwalkowiak]

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.

[hariohmprasath]

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

[maciejwalkowiak]

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.

[hariohmprasath]

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]

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

[hariohmprasath]

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]

@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]

@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]

@hariohmprasath sounds like the way to go!

[hariohmprasath]

Hi @maciejwalkowiak,
I have added integration tests to test service discovery and registration for both the platforms (ECS and EKS).

• EcsCloudMapIntegrationTest.java - Integration tests for ECS platform
• EksCloudMapIntegrationTest.java - Integration tests for EKS platform

Note: Since CloudMap service is only enabled for pro version of LocalStack make sure to create an API Key (either using trial mode or buying a subscription), update the key in IntegrationTestUtil.LOCAL_STACK_API_KEY variable before running the integration tests.

Hopefully this should make things easier for testing, let me know if you want me to update anything. Thanks and appreciate your support for this PR.

[hariohmprasath]

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]

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

[hariohmprasath]

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

[maciejwalkowiak]

@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]

Sounds good, thanks for letting me know

[zampettim]

Any updates to when this will be included?

[hariohmprasath]

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]

since this PR, the key is now LOCALSTACK_AUTH_TOKEN

[codefromthecrypt]

probably wanna read from env and skip if not present

[codefromthecrypt]

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]

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]

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)