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

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

Closed
1 of 5 tasks
rhboyd opened this issue Aug 6, 2019 · 16 comments
Closed
1 of 5 tasks
Assignees
Labels
@aws-cdk/aws-certificatemanager Related to Amazon Certificate Manager needs-triage This issue or PR still needs to be triaged.

Comments

@rhboyd
Copy link
Contributor

rhboyd commented Aug 6, 2019

  • I'm submitting a ...

    • πŸͺ² bug report
    • πŸš€ feature request
    • πŸ“š construct library gap
    • ☎️ security issue or vulnerability => Please see policy
    • ❓ support request => Please see note at the top of this template.
  • What is the current behavior?
    If the current behavior is a πŸͺ²bugπŸͺ²: Please provide the steps to reproduce
    I have a hosted zone that is created when I purchase a route53 domain. that domain has a Public Hosted Zone that AWS creates for me. I want to use this hosted zone to validate ACM Certificates through DNS. In order to do this in Python CDK, I am trying to leverage the aws_route53.HostedZone.from_hosted_zone_id method.

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.

  • What is the expected behavior (or behavior of feature suggested)?
    from_hosted_zone_id should create a IHostedZone that can be used by resources that need the Hosted Zone name as well

  • What is the motivation / use case for changing the behavior or adding this feature?
    better UX? Who doesn't love that?

  • Please tell us about your environment:

    • CDK CLI Version: 1.3.0 (build bba9914)
    • Module Version: 1.3.0
    • OS: OSX Mojave 10.14.6 (18G84)
    • Language: Python
  • Other information
    My "full" Python code.

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)
@rhboyd rhboyd added the needs-triage This issue or PR still needs to be triaged. label Aug 6, 2019
@revolutionisme
Copy link

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

@rhboyd
Copy link
Contributor Author

rhboyd commented Aug 15, 2019

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

rix0rrr commented Aug 28, 2019

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

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

@rix0rrr rix0rrr closed this as completed Aug 28, 2019
@rhboyd
Copy link
Contributor Author

rhboyd commented Aug 28, 2019

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

garyd203 commented Dec 12, 2019

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

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

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

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

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

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

ophilli commented Sep 14, 2020

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

@ricardosllm
Copy link

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

Dunklas added a commit to Dunklas/gbg-farligt-avfall that referenced this issue Nov 12, 2020
Dunklas added a commit to Dunklas/gbg-farligt-avfall that referenced this issue Nov 13, 2020
@mimozell
Copy link

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

douglasnaphas added a commit to douglasnaphas/madliberation that referenced this issue Mar 9, 2021
gh-293

As discussed in these issues:

aws/aws-cdk#3663
aws/aws-cdk#3558

you can't get an acutal Hosted Zone just with the ID. What you wind up
with when you query by ID, as I was doing prior to this commit, is some
kind of mock object that sounds like it helps with local synth,
but not with actually deploying resources.

AWS does not seem like it will fix this unfortunately.

This change tries to fetch by both name and ID.

I was getting errors like:

Error: HostedZone.fromHostedZoneId doesn't support "zoneName"

https://github.com/douglasnaphas/madliberation/runs/2062714182?check_suite_focus=true

I believe because the hosted zone I fetched was not a real hosted zone.

The docs for fromHostedZoneId
(https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-route53.HostedZone.html#static-fromwbrhostedwbrzonewbridscope-id-hostedzoneid),
which I was using, do say:

> Hosted zone name becomes unavailable through this query.

A CDK maintainer adds the shocking statement:

> The first two methods create use different Classes of IHostedZone that
> throw an error if you use the wrong parameter (e.g. calling getName()
> when you used fromHostedZoneId())

aws/aws-cdk#3663 (comment)

It is very disappointing that the CDK maintainers decided to provide an
implementation of an interface that throws an error when you try to get
its name.
@CarlosDomingues
Copy link

CarlosDomingues commented Apr 7, 2021

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

Tristan971 commented Jan 26, 2022

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

speterson-zoll commented May 25, 2022

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-certificatemanager Related to Amazon Certificate Manager needs-triage This issue or PR still needs to be triaged.
Projects
None yet
Development

No branches or pull requests