feat(aws-servicediscovery): Add support for API only services within a DNS namespace

[jmortlock]

---

Closes: #21490

DNS namespaces are currently setup in a way which only allow for discoverability via DNS or API, this means it is not currently possible to register non-ip instances within a DNS based namespace.

After this change you can create a DNS based service with an optional discoveryType If this discoveryType is set to DiscoveryType.API then you can register non IP based instances to this service using the registerNonIpInstance function

If no DiscoveryType is passed than the default from the namespace is used, so for an HTTP namespace this is API and for DNS derived namespaces this is DNS_AND_API

This means DNS type namespaces can have services registered with a combination of discovery types.

---

All Submissions:

• [✓] Have you followed the guidelines in our Contributing guide?

Adding new Unconventional Dependencies:

• [✗] This PR adds new unconventional dependencies following the process described here

New Features

• [✓] Have you added the new feature to an integration test?
  • [✓] Did you use yarn integ to deploy the infrastructure and generate the snapshot (i.e. yarn integ without --dry-run)?

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

[gitpod-io[bot]]

[jmortlock]

Thank you for this contribution @jmortlock. I'm not very familiar with the CloudMap service, so please forgive my questions.

* What's the use case of a `DnsNamespace` that's only discoverable via API? Beyond the physical ability of deploying it in Cloudformation, does this make sense?

So the namespace itself only dictates the discoverability types of the services registered within that namespace are. So a DnsNamespace will always have services that can be resolved via dns and api or api only*. The challenge at the moment comes when you want to register something that cannot be resolved via DNS, say an S3 ARN, or a Dynamodb table endpoint. Or when you need the extra flexibility of using a more complex service custom attributes model. At the same time there will still be services within the namespace that you wish to keep simple and resolve via DNS; things such as RDS endpoints, etc. Back to your original question; yes a CDK customer could theoretically register every service within a DNS namespace as API only; the only inconvenience I can think of is they would be paying for an unused route53 zone.

* Currently the service type is exclusively modelled through namespaces. Have you consider a design that holds up that contract?

I believe this is how it is still modeled; unless the customer provides an explicit discoverability type for the service being created it will fall back to the original namespace default, which is unchanged.

Additionally, please update the integration test to the new style (using new integ.IntegTest() etc).

Can do

* api only is the support we are adding to CDK here (CFN, console, api etc already support this)

[mrgrain]

Thanks for the explanation @jmortlock . I'm with you now. Good work on the API design, I like. We will give this another review once the building is passing.

[TheRealAmazonKendra]

@Mergifyio update

[mergify[bot]]

update

✅ Branch has been successfully updated

[TheRealAmazonKendra]

The build failures don't make sense to me. Giving this a retry because ¯\_(ツ)_/¯ .

[TheRealAmazonKendra]

@Mergifyio update

[mergify[bot]]

update

✅ Branch has been successfully updated

[jmortlock]

I can see the error in the logs will fix them up; not sure why it worked locally :shrug:

aws-cdk-lib: aws-servicediscovery/test/integ.service-with-public-dns-namespace.lit.ts:2:24 - error TS2307: Cannot find module '../../integ-tests' or its corresponding type declarations.
aws-cdk-lib: 2 import * as integ from '../../integ-tests';

[jmortlock]

I'm kinda out of ideas on how to resolve the error; I've regenerated the integration snapshots, updated Jest and tried to make the integ-test integration the same as what I can see in other parts of the codebase.

Any ideas @mrgrain @TheRealAmazonKendra

[mrgrain]

@jmortlock Apologies for side tracking this with my request to upgrade the integration test style. I've tracked the build failure down to the fact that these tests are .lit.ts files, which appear to get special treatment. I'm yet to find out how they are supposed to work. Documentation is sparse. 🤷🏻

In the interest of moving this PR forward, let's revert back to the old style.

Again, I'm sorry for missing this and wasting your time. 😕

[jmortlock]

No worries I will revert back; I have done a bit more digging with your clue about the .lit files and it appears that those files are included in the documentation.

For example https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_servicediscovery-readme.html

I think however and perhaps this is a CDKv2 breakage that the rewritten imports are not really what the customer would be using; and the original ones in the integration test are actually better.

[jmortlock]

I think a fix in my case would be to use relative pathing for the integration test require as well; but I will open up a second PR with the change over to the newer style tests.

[mrgrain]

I think however and perhaps this is a CDKv2 breakage that the rewritten imports are not really what the customer would be using; and the original ones in the integration test are actually better. I think a fix in my case would be to use relative pathing for the integration test require as well; but I will open up a second PR with the change over to the newer style tests.

Thanks for looking further into this. Yeah they don't look good for v2, but also in v1 we get some relative paths. Also adding new IntegRunner() to the examples would be confusing as well. I have been told that there's a way to exclude bits from the rendered example, but don't have anymore info yet.

In general, these are somewhat legacy and I wonder if the easiest thing would be to remove the .lit bit, change to new style and replace the example with a more concise inline one. 🤔

[jmortlock]

@mrgrain I believe all your comments are addressed, are there any other changes you would like me to make?

[mergify[bot]]

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

[aws-cdk-automation]

AWS CodeBuild CI Report

• CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
• Commit ID: 565ba72e6eaeaa9260bd9dc637f0c4e4ad5dd324
• Result: SUCCEEDED
• Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

[mergify[bot]]

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).