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_servicediscovery: PublicDnsNamespace NS records #20510

Open
bhsp opened this issue May 26, 2022 · 12 comments
Open

aws_servicediscovery: PublicDnsNamespace NS records #20510

bhsp opened this issue May 26, 2022 · 12 comments
Labels
@aws-cdk/aws-servicediscovery Related to Amazon ECS Service Discovery feature-request A feature should be added or improved. needs-cfn This issue is waiting on changes to CloudFormation before it can be addressed. p2

Comments

@bhsp
Copy link

bhsp commented May 26, 2022

Describe the bug

When creating a public Service Discovery namespace, the nameserver records are not added to the hosted zone. There doesn't appear to be a way to get the hosted zone ID created from the public Service Discovery namespace. For the example below, NS records are not added to the hosted zone for "sub2.sub1.mydomain.com". A hosted zone is added for "sub3.sub2.sub1.mydomain.com" but the NS records from this HZ are not linked to the HZ for "sub2.sub1.mydomain.com".

Expected Behavior

NS record(s) for sub3.sub2.sub1.mydomain.com is added to HZ for sub2.sub1.mydomain.com

Current Behavior

NS records not added, public Service Discovery namespace fails DNS look-ups

Reproduction Steps

#!/usr/bin/env python3
from constructs import Construct
from aws_cdk import App, Environment, Stack, CfnOutput
from aws_cdk import (aws_servicediscovery as sd)


class SampleStack(Stack):
    def __init__(self, scope: Construct, id: str, **kwargs) -> None:
        super().__init__(scope, id, **kwargs)

        # This FQDN is a hosted zone in the target account
        fqdn = "sub2.sub1.mydomain.com"
        service_discovery_subdomain = "sub3"

        # Does not add NS records to hosted zone $fqdn
        service_discovery = sd.PublicDnsNamespace(
            self,
            "service-discovery",
            name=f"{service_discovery_subdomain}.{fqdn}")

        CfnOutput(
            self,
            "service-discovery-namespace-name",
            value=service_discovery.namespace_name,
            export_name="SDNSName")

env = Environment(account="111111111111", region='us-east-1')
app = App()

lambda_stack = SampleStack(app, "sample-stack", env=env)

app.synth()

Possible Solution

No response

Additional Information/Context

No response

CDK CLI Version

2.25.0 (build ae1cb4b)

Framework Version

No response

Node.js Version

v14.19.1

OS

Windows

Language

Python

Language Version

No response

Other information

No response

@bhsp bhsp added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels May 26, 2022
@github-actions github-actions bot added the @aws-cdk/aws-servicediscovery Related to Amazon ECS Service Discovery label May 26, 2022
@peterwoodworth
Copy link
Contributor

I've created a PR which allows you to access the created hosted zone id from the namespace construct.

Otherwise - any functionality about adding record sub3.sub2.sub1.mydomain.com record to HZ for sub2.sub1.mydomain.com I'm unaware of. Our PublicDnsNamespace construct pretty much just creates the same named CloudFormation resource - are you expecting this to happen just when you create a Cloudformation PublicDnsNamespace?

@peterwoodworth peterwoodworth added p2 feature-request A feature should be added or improved. and removed bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Jun 2, 2022
@peterwoodworth peterwoodworth added needs-cfn This issue is waiting on changes to CloudFormation before it can be addressed. response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. labels Jun 2, 2022
@github-actions
Copy link

github-actions bot commented Jun 4, 2022

This issue has not received a response in a while. If you want to keep this issue open, please leave a comment below and auto-close will be canceled.

@github-actions github-actions bot added the closing-soon This issue will automatically close in 4 days unless further comments are made. label Jun 4, 2022
@bhsp
Copy link
Author

bhsp commented Jun 6, 2022

@peterwoodworth This is great! Thank you. Given if/when the PR is merged, can I use the created hosted zone id (namespace_hosted_zone_id) from the namespace construct in the same stack to retrieve the Name Server records and then create the NS record in the parent hosted zone? Something like, POC addition to my Python previously submitted.

dns.NsRecord(
            self,
            "ns",
            zone=dns.PublicHostedZone.from_lookup(
                 self,
                 "parent-hosted-zone",
                 domain_name=fqdn
            ),
            record_name=service_discovery.namespace_name,
            values=dns.PublicHostedZone.from_public_hosted_zone_id(
                self,
                id="sd-hosted-zone",
                public_hosted_zone_id=service_discovery.namespace_hosted_zone_id).hosted_zone_name_servers,
            ttl=Duration.seconds(60)
        )

@github-actions github-actions bot removed closing-soon This issue will automatically close in 4 days unless further comments are made. response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. labels Jun 6, 2022
@peterwoodworth
Copy link
Contributor

Not quite, but close - the hosted_zone_name_servers prop will be undefined if your hosted zone is imported. You can instead directly pass in the new namespace_hosted_zone_id prop to the NsRecord.values prop

@bhsp
Copy link
Author

bhsp commented Jun 7, 2022

@peterwoodworth I'm not seeing where/how that's an option from the documentation, if I'm understanding your correction.
I see that values accepts "(Sequence[str]) – The NS values." to my understanding values=['n1.example.com', 'n2.example.com']

Is my example below correct from your last message?

dns.NsRecord(
            self,
            "ns",
            zone=dns.PublicHostedZone.from_lookup(
                 self,
                 "parent-hosted-zone",
                 domain_name=fqdn
            ),
            record_name=service_discovery.namespace_name,
            values=service_discovery.namespace_hosted_zone_id,
            ttl=Duration.seconds(60)
        )

@peterwoodworth
Copy link
Contributor

namespaceHostedZoneId will return a string! So I would expect you to be able to put it in an array of strings

values=[service_discovery.namespace_hosted_zone_id, ...]

@bhsp
Copy link
Author

bhsp commented Jun 7, 2022

@peterwoodworth Ah yes, I meant to use as a list above, I didn't realize CFN would see a hosted_zone_id in place of name server records and look-up said records auto-magically given the hosted_zone_id in-place of the name servers. Neat.

@peterwoodworth
Copy link
Contributor

Ack, I spaced pretty bad here, I thought you were describing something slightly different. No, CloudFormation will not magically do that unfortunately. You want to use the the name

I'm not sure what the path forward is for you here is. The way we do this for our regular HostedZone construct is that we leverage CloudFormation's attribute on their HostedZone (which doesn't apply here of course). We currently don't provide any other way to look this up to my knowledge

@peterwoodworth
Copy link
Contributor

You could use a custom resource to get the resource records associated with the hosted zone using a custom resource and the LIstResourceRecordSets API call

@bhsp
Copy link
Author

bhsp commented Jun 7, 2022

No worries, kind of didn't seem to be the case, but I went with it :-). Truly appreciate the help + insight. I have a side channel Python Boto3 script that looks everything up after the formation event and adds the record to the parent hosted zone. Having it all in the CDK would be brilliant, oh well.

Just noticed another response from you while typing this. I do have a footnote in my script to move it to a custom resource Boto3 Lambda, I simply haven't experimented w/ custom resources yet.

Still - providing the namespace_hosted_zone_id would provide a tighter lookup, rather than matching on FQDN to find the HostedZone and then finding it's name records.

mergify bot pushed a commit that referenced this issue Jun 20, 2022
…20583)

related to #20510 

----

### All Submissions:

* [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md)

### Adding new Unconventional Dependencies:

* [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md/#adding-new-unconventional-dependencies)

### New Features

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/master/INTEGRATION_TESTS.md)?
	* [ ] 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*
@peterwoodworth
Copy link
Contributor

We should be supporting the attribute by next release 🙂

@bhsp
Copy link
Author

bhsp commented Jun 23, 2022

Awesome, very excited to incorporate. Thank you

daschaa pushed a commit to daschaa/aws-cdk that referenced this issue Jul 9, 2022
…ws#20583)

related to aws#20510 

----

### All Submissions:

* [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md)

### Adding new Unconventional Dependencies:

* [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md/#adding-new-unconventional-dependencies)

### New Features

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/master/INTEGRATION_TESTS.md)?
	* [ ] 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*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-servicediscovery Related to Amazon ECS Service Discovery feature-request A feature should be added or improved. needs-cfn This issue is waiting on changes to CloudFormation before it can be addressed. p2
Projects
None yet
Development

No branches or pull requests

3 participants