From e3b444c912a6d6dd992d13b7fea98d239b17c14b Mon Sep 17 00:00:00 2001 From: Justin Santa Barbara Date: Fri, 6 Jan 2017 23:57:36 -0500 Subject: [PATCH] Fix double initialization of DNSZone And, while we are it, clean up DNSZone so that it has separate notions of TaskName, DNSName and HostedZoneID. We conflated the three previously, which we don't want to do at the task layer. We don't want to conflate the TaskName and the DNSName so that we can create a private & public hosted zone with the same DNSName. We don't want to "smuggle" the hosted zone ID in the DNSName because it doesn't belong in the task layer. Fix #1374 --- pkg/model/dns.go | 40 +++++++++++------- upup/pkg/fi/cloudup/awstasks/dnsname.go | 4 +- upup/pkg/fi/cloudup/awstasks/dnszone.go | 54 ++++++++++++------------- 3 files changed, 55 insertions(+), 43 deletions(-) diff --git a/pkg/model/dns.go b/pkg/model/dns.go index 5744abab3c..340f56f721 100644 --- a/pkg/model/dns.go +++ b/pkg/model/dns.go @@ -19,6 +19,7 @@ package model import ( "k8s.io/kops/upup/pkg/fi" "k8s.io/kops/upup/pkg/fi/cloudup/awstasks" + "strings" ) // DNSModelBuilder builds DNS related model objects @@ -29,16 +30,29 @@ type DNSModelBuilder struct { var _ fi.ModelBuilder = &DNSModelBuilder{} func (b *DNSModelBuilder) Build(c *fi.ModelBuilderContext) error { - if b.UsePrivateDNS() { - // This is only exposed as a feature flag currently + // Add a HostedZone if we are going to publish a dns record that depends on it + if b.UsePrivateDNS() || b.UsesBastionDns() { + // UsePrivateDNS is only exposed as a feature flag currently // TODO: We may still need a public zone to publish an ELB + // Check to see if we are using a bastion DNS record that points to the hosted zone + // If we are, we need to make sure we include the hosted zone as a task + // Configuration for a DNS zone, attached to our VPC dnsZone := &awstasks.DNSZone{ - Name: s(b.Cluster.Spec.DNSZone), + Name: s("private-" + b.Cluster.Spec.DNSZone), Private: fi.Bool(true), PrivateVPC: b.LinkToVPC(), } + + if !strings.Contains(b.Cluster.Spec.DNSZone, ".") { + // Looks like a hosted zone ID + dnsZone.ZoneID = s(b.Cluster.Spec.DNSZone) + } else { + // Looks like a normal ddns name + dnsZone.DNSName = s(b.Cluster.Spec.DNSZone) + } + c.AddTask(dnsZone) } else if b.UseLoadBalancerForAPI() { // This will point our DNS to the load balancer, and put the pieces @@ -49,6 +63,15 @@ func (b *DNSModelBuilder) Build(c *fi.ModelBuilderContext) error { Name: s(b.Cluster.Spec.DNSZone), Private: fi.Bool(false), } + + if !strings.Contains(b.Cluster.Spec.DNSZone, ".") { + // Looks like a hosted zone ID + dnsZone.ZoneID = s(b.Cluster.Spec.DNSZone) + } else { + // Looks like a normal ddns name + dnsZone.DNSName = s(b.Cluster.Spec.DNSZone) + } + c.AddTask(dnsZone) } @@ -65,16 +88,5 @@ func (b *DNSModelBuilder) Build(c *fi.ModelBuilderContext) error { c.AddTask(dnsName) } - // Check and see if we are using a bastion DNS record that points to the hosted zone - // If we are, we need to make sure we include the hosted zone as a task - if b.UsesBastionDns() { - dnsZone := &awstasks.DNSZone{ - Name: s(b.Cluster.Spec.DNSZone), - Private: fi.Bool(true), - PrivateVPC: b.LinkToVPC(), - } - c.AddTask(dnsZone) - } - return nil } diff --git a/upup/pkg/fi/cloudup/awstasks/dnsname.go b/upup/pkg/fi/cloudup/awstasks/dnsname.go index 0a45a47f40..c0c7e7af5c 100644 --- a/upup/pkg/fi/cloudup/awstasks/dnsname.go +++ b/upup/pkg/fi/cloudup/awstasks/dnsname.go @@ -54,7 +54,7 @@ func (e *DNSName) Find(c *fi.Context) (*DNSName, error) { } request := &route53.ListResourceRecordSetsInput{ - HostedZoneId: e.Zone.ID, + HostedZoneId: e.Zone.ZoneID, // TODO: Start at correct name? } @@ -153,7 +153,7 @@ func (_ *DNSName) RenderAWS(t *awsup.AWSAPITarget, a, e, changes *DNSName) error changeBatch.Changes = []*route53.Change{change} request := &route53.ChangeResourceRecordSetsInput{} - request.HostedZoneId = e.Zone.ID + request.HostedZoneId = e.Zone.ZoneID request.ChangeBatch = changeBatch glog.V(2).Infof("Updating DNS record %q", *e.Name) diff --git a/upup/pkg/fi/cloudup/awstasks/dnszone.go b/upup/pkg/fi/cloudup/awstasks/dnszone.go index b7561f5a06..57129cf09c 100644 --- a/upup/pkg/fi/cloudup/awstasks/dnszone.go +++ b/upup/pkg/fi/cloudup/awstasks/dnszone.go @@ -33,8 +33,9 @@ import ( //go:generate fitask -type=DNSZone type DNSZone struct { - Name *string - ID *string + Name *string + DNSName *string + ZoneID *string Private *bool PrivateVPC *VPC @@ -60,7 +61,10 @@ func (e *DNSZone) Find(c *fi.Context) (*DNSZone, error) { actual := &DNSZone{} actual.Name = e.Name - actual.ID = z.HostedZone.Id + if z.HostedZone.Name != nil { + actual.DNSName = fi.String(strings.TrimSuffix(*z.HostedZone.Name, ".")) + } + actual.ZoneID = z.HostedZone.Id actual.Private = z.HostedZone.Config.PrivateZone // If the zone is private, but we don't want it to be, that will be an error @@ -77,8 +81,11 @@ func (e *DNSZone) Find(c *fi.Context) (*DNSZone, error) { } } - if e.ID == nil { - e.ID = actual.ID + if e.ZoneID == nil { + e.ZoneID = actual.ZoneID + } + if e.DNSName == nil { + e.DNSName = actual.DNSName } return actual, nil @@ -86,24 +93,15 @@ func (e *DNSZone) Find(c *fi.Context) (*DNSZone, error) { func (e *DNSZone) findExisting(cloud awsup.AWSCloud) (*route53.GetHostedZoneOutput, error) { findID := "" - if e.ID != nil { - findID = *e.ID - } else if e.Name != nil && !strings.Contains(*e.Name, ".") { - // Looks like a hosted zone ID - findID = *e.Name - } - if findID != "" { + if e.ZoneID != nil { request := &route53.GetHostedZoneInput{ - Id: aws.String(findID), + Id: e.ZoneID, } response, err := cloud.Route53().GetHostedZone(request) if err != nil { if awsup.AWSErrorCode(err) == "NoSuchHostedZone" { - if e.ID != nil { - return nil, nil - } - // Otherwise continue ... maybe the name was not an id after all... + return nil, nil } else { return nil, fmt.Errorf("error fetching DNS HostedZone %q: %v", findID, err) } @@ -112,7 +110,7 @@ func (e *DNSZone) findExisting(cloud awsup.AWSCloud) (*route53.GetHostedZoneOutp } } - findName := fi.StringValue(e.Name) + findName := fi.StringValue(e.DNSName) if findName == "" { return nil, nil } @@ -165,10 +163,10 @@ func (s *DNSZone) CheckChanges(a, e, changes *DNSZone) error { } func (_ *DNSZone) RenderAWS(t *awsup.AWSAPITarget, a, e, changes *DNSZone) error { - name := aws.StringValue(e.Name) + name := aws.StringValue(e.DNSName) if a == nil { request := &route53.CreateHostedZoneInput{} - request.Name = e.Name + request.Name = e.DNSName nonce := rand.Int63() request.CallerReference = aws.String(strconv.FormatInt(nonce, 10)) @@ -186,11 +184,11 @@ func (_ *DNSZone) RenderAWS(t *awsup.AWSAPITarget, a, e, changes *DNSZone) error return fmt.Errorf("error creating DNS HostedZone %q: %v", name, err) } - e.ID = response.HostedZone.Id + e.ZoneID = response.HostedZone.Id } else { if changes.PrivateVPC != nil { request := &route53.AssociateVPCWithHostedZoneInput{ - HostedZoneId: a.ID, + HostedZoneId: a.ZoneID, VPC: &route53.VPC{ VPCId: e.PrivateVPC.ID, VPCRegion: aws.String(t.Cloud.Region()), @@ -227,10 +225,12 @@ type terraformRoute53Zone struct { func (_ *DNSZone) RenderTerraform(t *terraform.TerraformTarget, a, e, changes *DNSZone) error { cloud := t.Cloud.(awsup.AWSCloud) + dnsName := fi.StringValue(e.DNSName) + // As a special case, we check for an existing zone // It is really painful to have TF create a new one... // (you have to reconfigure the DNS NS records) - glog.Infof("Check for existing route53 zone to re-use with name %q", *e.Name) + glog.Infof("Check for existing route53 zone to re-use with name %q", dnsName) z, err := e.findExisting(cloud) if err != nil { return err @@ -239,7 +239,7 @@ func (_ *DNSZone) RenderTerraform(t *terraform.TerraformTarget, a, e, changes *D if z != nil { glog.Infof("Existing zone %q found; will configure TF to reuse", aws.StringValue(z.HostedZone.Name)) - e.ID = z.HostedZone.Id + e.ZoneID = z.HostedZone.Id } if z == nil { @@ -271,9 +271,9 @@ func (_ *DNSZone) RenderTerraform(t *terraform.TerraformTarget, a, e, changes *D } func (e *DNSZone) TerraformLink() *terraform.Literal { - if e.ID != nil { - glog.V(4).Infof("reusing existing route53 zone with id %q", *e.ID) - return terraform.LiteralFromStringValue(*e.ID) + if e.ZoneID != nil { + glog.V(4).Infof("reusing existing route53 zone with id %q", *e.ZoneID) + return terraform.LiteralFromStringValue(*e.ZoneID) } return terraform.LiteralSelfLink("aws_route53_zone", *e.Name)