Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

hariohmprasath
Copy link

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
@github-actions github-actions bot added the type: dependency-upgrade Dependency version bump label Sep 5, 2022
@maciejwalkowiak
Copy link
Contributor

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.

Copy link
Contributor

@maciejwalkowiak maciejwalkowiak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pushed a fix.

spring-cloud-aws-cloudmap/pom.xml Outdated Show resolved Hide resolved
<name>Spring Cloud Cloud Map</name>
<description>Spring Cloud AWS Cloud Map</description>

<dependencies>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Author

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

@github-actions github-actions bot added the type: documentation Documentation or Samples related issue label Sep 7, 2022
@hariohmprasath
Copy link
Author

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
Copy link
Author

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
Copy link
Contributor

@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
Copy link
Author

@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
Copy link
Contributor

@hariohmprasath sounds like the way to go!

@github-actions 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
@hariohmprasath
Copy link
Author

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
Copy link
Contributor

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

@maciejwalkowiak maciejwalkowiak modified the milestones: 3.0.0 M3, 3.0.0 M4 Oct 22, 2022
@maciejwalkowiak maciejwalkowiak modified the milestones: 3.0.0 M4, 3.1 Jan 4, 2023
@hariohmprasath
Copy link
Author

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

@maciejwalkowiak
Copy link
Contributor

@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
Copy link
Author

Sounds good, thanks for letting me know

@maciejwalkowiak maciejwalkowiak modified the milestones: 3.1, 3.1.0 Nov 6, 2023
@maciejwalkowiak maciejwalkowiak modified the milestones: 3.1.0, 3.1 Dec 9, 2023
@zampettim
Copy link

Any updates to when this will be included?

@hariohmprasath
Copy link
Author

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

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

@codefromthecrypt codefromthecrypt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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";

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since this PR, the key is now LOCALSTACK_AUTH_TOKEN

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably wanna read from env and skip if not present

// 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"))

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link

codefromthecrypt commented Mar 15, 2024

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
Copy link

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
Copy link
Author

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
Copy link

Vostan commented Aug 12, 2024

+1

@maciejwalkowiak
Copy link
Contributor

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

@maciejwalkowiak maciejwalkowiak modified the milestones: 3.x, 3.4.x Sep 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: cloudmap component: parameter-store Parameter Store integration related issue component: s3 S3 integration related issue component: secrets-manager Secrets Manager integration related issue type: dependency-upgrade Dependency version bump type: documentation Documentation or Samples related issue type: feature Integration with a new AWS service or bigger change in existing integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants