Merge pull request #1378 from justinsb/fix_1374

Fix double initialization of DNSZone

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
 }

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)

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)