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)

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.

@Mergifyio update

update

✅ Branch has been successfully updated

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

@Mergifyio update

update

✅ Branch has been successfully updated

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

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

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

@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. 😕

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.

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.

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. 🤔

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

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 CodeBuild CI Report

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

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

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).

@jmortlock @mrgrain does having DNS_AND_API variable make any sense for this statement?

because its used in opposite to API use cases throughout the code. but the name implies that its both options which it is not since if its DNS its not API.

@jmortlock

the default discoveryType in new Service is DNS_AND_API

const namespace = new PrivateDnsNamespace(stack, `ServiceDiscoveryNamespace`, {
  name: 'example.internal', vpc
})
const service = new Service(stack, `apiServiceDiscovery`, {
  namespace, dnsTtl: Duration.seconds(300), name: 'api', loadBalancer: false
})
service.registerNonIpInstance('apiCname', {
  instanceId: 'apigw',
  customAttributes: { url: api.url } // api is of type SpecRestApi
})

gives:

Error: This type of instance can only be registered for HTTP namespaces.

This is what the discoveryType option is for You can find it in the docs (https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_servicediscovery-readme.html#private-dns-namespace-example)

Which translates to

const service = new Service(stack, `apiServiceDiscovery`, {
  namespace, dnsTtl: Duration.seconds(300), name: 'api', loadBalancer: false, 
  discoveryType: DiscoveryType.API
})

yes I understand that. I'm just saying that DiscoveryType.DNS_AND_API cannot be used with nonIp instances like API which is counter intuitive, when _AND_API is in the name. This is specific to Dns namespaces of course.

My understanding is that NamespaceType.HTTP translates to a AWS::ServiceDiscovery::HttpNamespace and that this can only be deployed by setting DiscoveryType.DNS_AND_API which translates to a not-set Type property in AWS::ServiceDiscovery::Service.

Based on this, I can understand how this is still confusing. If I recall correctly, with this PR we tried to address some of the DX concerns the CloudFormation API has. However it seems that wasn't fully resolved.

@adminy I believe there are really only two possible courses of action here:

• Improve the CDK docs to make this behavior more clear.
• Rename the enum values to something that is more descriptive. Do you have any suggestion?

Edit: Although maybe the confusion comes more from the fact that DiscoveryType also influences what kind of instances can be registered. Which is potentially difficult to reconcile.

I think there is still some misunderstanding here; the discovery type is telling CloudMap how you would like to find the service that you are registering into the cloudmap.

DiscoveryType::API means you can find the entry in the service map using only the AWS SDKs
DiscoveryType::DNS_AND_API means you can find the entry in the service map using a standard DNS resolver or via the AWS SDKs.

Obviously DNS has more restrictions about what you can put into it, but is much easier for the most part to work with.

HTTPNamspaces services are only every discoverable via API
`DNS Namespace(s) are discoverable either via DNS or API (which is the default), or if you are registering a non-ip instance into the service map than you have the choice to exclude it from the DNS lookup (as its not possible to do so anyway). Currently you have to explicitly define the service discovery type to do this.

So one change I could potentially see is that if you call the registerNonIpInstance then the service discovery type is automatically set to API only as a DNS lookup is not possible anyway; currently it fails with the exception we saw above.

Prehaps another change could be the Namspace Service object could offer some factory methods to take other CDK constructs and work out the best way of registering this within a namespace. Kind of like how the "grant" system works for IAM.

Note the original PR was all about allowing the DNS namespace(s) to support both types of discovery so you could use a DNS Namespace to register both your IP related things (like ec2 instance) and your other AWS services like SQS, S3 ARNs, etc within the same Namespace. Before the change you would have to have two separate namespaces a HTTP one for the SQS, S3, etc and a DNS one for your IP ones. After the change you can do it all within a single DNS namespace.