aws / aws-cdk

The AWS Cloud Development Kit is a framework for defining cloud infrastructure in code
https://aws.amazon.com/cdk
Apache License 2.0
11.59k stars 3.89k forks source link

from_hosted_zone_id fails when the object is referenced to retrieve a HZ name. #3558

Closed rhboyd closed 5 years ago

rhboyd commented 5 years ago

My first attempt looks like this:

    hosted_zone: route53.HostedZone = route53.HostedZone.from_hosted_zone_id(
            self,
            id="MyResolvedHZ",
            hosted_zone_id=props.hosted_zone_id
    )

    acm_cert: acm.Certificate = acm.DnsValidatedCertificate(
            self,
            "MyWebsiteCert",
            hosted_zone=hosted_zone,
            domain_name=props.domain_name
    )

which results in an error stating: jsii.errors.JSIIError: HostedZone.fromHostedZoneId doesn't support "zoneName" and points to the cause at the line domain_name=props.domain_name

If I change the method to this it works:

hosted_zone: route53.HostedZone = route53.HostedZone.from_hosted_zone_attributes(
            self,
            id="MyResolvedHZ",
            hosted_zone_id=props.hosted_zone_id,
            zone_name="rboyd.dev"
)

but this requires me to pass in the hosted zone name, hosted zone id, and the desired domain name (subdomain of hosted zone), when I should be able to do what I need with just the Hosted Zone Id and desired domain name since I assume HZ Ids are region unique within an account.

from aws_cdk import (
    aws_certificatemanager as acm,
    aws_route53 as route53,
    aws_cloudfront as cloudfront,
    aws_s3 as s3,
    aws_route53_targets as targets,
    aws_dynamodb as ddb,
    core
)

class BlogProps(object):
    def __init__(self, domain_name: str, hosted_zone_id: str):
        self._domain_name = domain_name
        self._hosted_zone_id = hosted_zone_id

    @property
    def domain_name(self) -> str:
        return self._domain_name

    @property
    def hosted_zone_id(self) -> str:
        return self._hosted_zone_id

class InfrastructureStack(core.Stack):

    def __init__(self, scope: core.Construct, id: str, props: BlogProps, **kwargs) -> None:
        super().__init__(scope, id, **kwargs)

        hosted_zone: route53.HostedZone = route53.HostedZone.from_hosted_zone_id(
            self,
            id="MyResolvedHZ",
            hosted_zone_id=props.hosted_zone_id
        )

        acm_cert: acm.Certificate = acm.DnsValidatedCertificate(
            self,
            "MyWebsiteCert",
            hosted_zone=hosted_zone,
            domain_name=props.domain_name
        )

        # We can keep the Read capacity low because we are going to use API Gateway Caching to reduce the load on our DB
        blog_post_table: ddb.Table = ddb.Table(
            self,
            "BlogPostTable",
            partition_key=ddb.Attribute(name="PartitionKey", type=ddb.AttributeType.STRING),
            sort_key=ddb.Attribute(name="SortKey", type=ddb.AttributeType.STRING),
            read_capacity=5,
            write_capacity=5

        )

        site_bucket = s3.Bucket(
            self,
            'SiteBucket',
            bucket_name=props.domain_name,
            website_index_document='index.html',
            website_error_document='error.html',
            public_read_access=True
        )

        alias_configuration = cloudfront.AliasConfiguration(
            acm_cert_ref=acm_cert.certificate_arn,
            names=[props.domain_name],
            ssl_method=cloudfront.SSLMethod.SNI,
            security_policy=cloudfront.SecurityPolicyProtocol.TLS_V1_1_2016
        )

        source_configuration = cloudfront.SourceConfiguration(
            s3_origin_source=cloudfront.S3OriginConfig(
                s3_bucket_source=site_bucket
            ),
            behaviors=[cloudfront.Behavior(is_default_behavior=True)]
        )

        distribution = cloudfront.CloudFrontWebDistribution(
            self,
            'SiteDistribution',
            alias_configuration=alias_configuration,
            origin_configs=[source_configuration]
        )

        route53.ARecord(
            self,
            'SiteAliasRecord',
            record_name=props.domain_name,
            target=route53.AddressRecordTarget.from_alias(targets.CloudFrontTarget(distribution)),
            zone=hosted_zone
        )

        core.CfnOutput(self, 'Bucket', value=site_bucket.bucket_name)
        core.CfnOutput(self, 'ACMCert', value=acm_cert.certificate_arn)
        core.CfnOutput(self, 'TableName', value=blog_post_table.table_name)
revolutionisme commented 5 years ago

Yup, hit the same issue as well, using the "from_hosted_zone_attributes" for now.

rhboyd commented 5 years ago

This should probably be reclassified as a UX Issue. It's not clear to new developers when to use from_id() from_name() or from_attributes() for most resources.

I don't think this is an actual bug, it just leads to people using the wrong methods.

rix0rrr commented 5 years ago

I think you might want to use HostedZone.fromLookup().

Closing this issue, please reopen if you feel this was closed in error.

rhboyd commented 5 years ago

I think this should be made into a docs issue. This exact issue (from_id() vs from_name() vs lookup()) has been brought up at least 3 times in the past two weeks. Enough people are misled by it that the docs could probably be made a bit clearer.

garyd203 commented 4 years ago

I don't think this outcome is satisfactory (closing the issue as "wont-fix"). The use case is that users want to be able to get the domain of a HostedZone from it's zone ID (either directly, or for passing to some other CDK function), and there is no solution for this.

The root issue here is not documentation (although that's certainly contributing to confusion). Rather, the problem is that some users expect the fromXxx functions in the CDK API to produce a real object that represents contextual information determined at synthesize time (like VPC.fromLookup does) - not a mock object that only reflects the values that were put into it.

Further, it is a reasonable expectation that if an object implements an interface (such as IHostedZone), then it will implement all the functionality in that interface.

At the very least, we should make a clearer distinction between methods that perform a contextual lookup, and methods that create a partial mock (eg. have a dedicated mock class and do something like FakeHostedZone.withHostedZoneId()).

jkodroff commented 4 years ago

I would agree with @garyd203 here in that throwing an exception on an interface member is a violation of the Liskov Substitution Principle. (See the part about throwing a NotImplementedException.)

I think the solution here is to adjust the model, something like adding a deprecation warning where appropriate and then changing from_hosted_zone_id to return something other than IHostedZone (sorry for mixing languages here) since the returned value does not correctly implement the interface.

Since I would consider the LSP to be fairly critical to design of a tool like CDK, I would ask that the maintainers reopen this issue.

kjellski commented 4 years ago

I ran into this just now and the error one is getting is very misleading as well as the implementation of the IHostedZone interface as the return type not quite accurate to be hones. I'd classify this as a bug/defect as well. The method does not return an IHostedZone, right? Could you point me to the documentation I should read about this?

john-tipper commented 4 years ago

I, too, think this is definitely a bug. This will cause the same error:

IHostedZone hostedZone = HostedZone.fromHostedZoneId(this, "my-hosted-zone", "someZoneId");

DnsValidatedCertificate authCertificate = DnsValidatedCertificate.Builder.create(this, "my-certificate")
                .domainName(domainName)
                .region("us-east-1")
                .hostedZone(hostedZone)
                .build();

I have obtained an IHostedZone and passed into a method that wants one, yet there is an error thrown.

bahrmichael commented 4 years ago

I just stumbled upon this, too. Based on the CDK docs I expect fromHostedZoneId to give me an IHostedZone that I can use in further CDK operations.

radu-malliu commented 4 years ago

Just ran into this issue, fromHostedZoneId allegedly returns an IPublicHostedZone, but addDelegation can't use it:

// subZone looks like this:
// {hostedZoneId: 'XXXX'}

props.subZones.forEach(subZone => {
  var subZoneConstruct = PublicHostedZone.fromPublicHostedZoneId(
    this, 'subZone', subZone.hostedZoneId
  );
  this.rootZone.addDelegation(
    subZoneConstruct,
    {ttl: core.Duration.seconds(60)}
  );
});
ophilli commented 4 years ago

I also just ran into this issue while trying to use fromPublicHostedZoneId to create an ARecord.

ricardosllm commented 4 years ago

Also just ran into this when wanting to define a ApplicationLoadBalancedFargateService with a domain_zone

I should be able to retrieve an existing r53 hosted zone to use here

mimozell commented 3 years ago

Same experience when trying to create a record in a hostedzone when I only have the ID and use fromHostedZoneId.

CarlosDomingues commented 3 years ago

I was bitten by that in cdk 1.94.1 when trying to create a record in a hosted zone retrieved using from_public_hosted_zone_id

The HostedZone docs do have this line: Use when hosted zone ID is known. Hosted zone name becomes unavailable through this query.

Unfortunately that is not mentioned in PublicHostedZone's docs.

Tristan971 commented 2 years ago

Why would anyone half-sane prefer essentially relying on a search instead of a get-by-id for something as sensitive as DNS zones?

Especially when HostedZoneProviderProps' privateZone is nullable and not setting it assumes you want a public one at the time of writing, thus risking leaking sensible records particularly if having a split-horizon DNS going on?

Wat

speterson-zoll commented 2 years ago

I was hit with this bug today as well. All I want to do is to call HostedZone.fromHostedZoneId by passing in an ID that I have in an ssm parameter, and receive a full (real) IHostedZone object, from which I can retrieve the zoneName. I do NOT want to have to look up our HostedZones using a query that involves passing in the zone name. We have environment specific accounts, so we have one account for dev, one for test, etc. I need my CDK stacks to be environment agnostic, but our zone names (created elsewhere by a different department) take the environment into account, i.e., dev.{accountAbbreviation}.company.com or test.{accountAbbreviation}.company.com. I do not want to have to map the account numbers to the environment names, I just need to look up by hosted zone ID. Can this issue please be reopened and actually fixed?