Don't add DNSZone task twice

pkg/model/dns.go

@@ -17,6 +17,8 @@ limitations under the License.
 package model
 
 import (
+	"fmt"
+	"k8s.io/kops/pkg/apis/kops"
 	"k8s.io/kops/upup/pkg/fi"
 	"k8s.io/kops/upup/pkg/fi/cloudup/awstasks"
 	"strings"
@@ -29,56 +31,60 @@ type DNSModelBuilder struct {
 
 var _ fi.ModelBuilder = &DNSModelBuilder{}
 
+func (b *DNSModelBuilder) ensureDNSZone(c *fi.ModelBuilderContext) error {
+	// Configuration for a DNS zone
+	dnsZone := &awstasks.DNSZone{
+		Name: s(b.NameForDNSZone()),
+	}
+
+	topology := b.Cluster.Spec.Topology
+	if topology != nil && topology.DNS != nil {
+		switch topology.DNS.Type {
+		case kops.DNSTypePublic:
+		// Ignore
+
+		case kops.DNSTypePrivate:
+			dnsZone.Private = fi.Bool(true)
+			dnsZone.PrivateVPC = b.LinkToVPC()
+
+		default:
+			return fmt.Errorf("Unknown DNS type %q", topology.DNS.Type)
+		}
+	}
+
+	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 DNS name
+		dnsZone.DNSName = s(b.Cluster.Spec.DNSZone)
+	}
+
+	return c.EnsureTask(dnsZone)
+}
+
 func (b *DNSModelBuilder) Build(c *fi.ModelBuilderContext) error {
 	// Add a HostedZone if we are going to publish a dns record that depends on it
 	if b.UsePrivateDNS() {
-		// 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("private-" + b.Cluster.Spec.DNSZone),
-			Private:    fi.Bool(true),
-			PrivateVPC: b.LinkToVPC(),
+		if err := b.ensureDNSZone(c); err != nil {
+			return err
 		}
-
-		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)
 	}
 
 	if b.UseLoadBalancerForAPI() {
 		// This will point our DNS to the load balancer, and put the pieces
 		// together for kubectl to be work
 
-		// Configuration for a DNS name for the master
-		dnsZone := &awstasks.DNSZone{
-			Name:    s(b.Cluster.Spec.DNSZone),
-			Private: fi.Bool(false),
+		if err := b.ensureDNSZone(c); err != nil {
+			return err
 		}
 
-		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)
-
 		apiDnsName := &awstasks.DNSName{
 			Name:               s(b.Cluster.Spec.MasterPublicName),
-			Zone:               &awstasks.DNSZone{Name: s(b.Cluster.Spec.DNSZone)},
+			Zone:               b.LinkToDNSZone(),
 			ResourceType:       s("A"),
 			TargetLoadBalancer: b.LinkToELB("api"),
 		}
@@ -89,20 +95,10 @@ func (b *DNSModelBuilder) Build(c *fi.ModelBuilderContext) error {
 		// Pulling this down into it's own if statement. The DNS configuration here
 		// is similar to others, but I would like to keep it on it's own in case we need
 		// to change anything.
-		dnsZone := &awstasks.DNSZone{
-			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)
+		if err := b.ensureDNSZone(c); err != nil {
+			return err
 		}
-
-		c.AddTask(dnsZone)
 	}
 
 	return nil

pkg/model/names.go

@@ -90,10 +90,15 @@ func (b *KopsModelContext) LinkToVPC() *awstasks.VPC {
 }
 
 func (b *KopsModelContext) LinkToDNSZone() *awstasks.DNSZone {
-	name := b.Cluster.Spec.DNSZone
+	name := b.NameForDNSZone()
 	return &awstasks.DNSZone{Name: &name}
 }
 
+func (b *KopsModelContext) NameForDNSZone() string {
+	name := b.Cluster.Spec.DNSZone
+	return name
+}
+
 func (b *KopsModelContext) IAMName(role kops.InstanceGroupRole) string {
 	var name string
 

upup/pkg/fi/task.go

@@ -19,6 +19,7 @@ package fi
 import (
 	"fmt"
 	"github.com/golang/glog"
+	"reflect"
 	"strings"
 )
 
@@ -57,6 +58,30 @@ func (c *ModelBuilderContext) AddTask(task Task) {
 	c.Tasks[key] = task
 }
 
+// EnsureTask ensures that the specified task is configured.
+// It adds the task if it does not already exist.
+// If it does exist, it verifies that the existing task reflect.DeepEqual the new task,
+// if they are different an error is returned.
+func (c *ModelBuilderContext) EnsureTask(task Task) error {
+	key := buildTaskKey(task)
+
+	existing, found := c.Tasks[key]
+	if found {
+		if reflect.DeepEqual(task, existing) {
+			glog.V(8).Infof("EnsureTask ignoring identical ")
+			return nil
+		} else {
+			glog.Warningf("EnsureTask found task mismatch for %q", key)
+			glog.Warningf("\tExisting: %v", existing)
+			glog.Warningf("\tNew: %v", task)
+
+			return fmt.Errorf("cannot add different task with same key %q", key)
+		}
+	}
+	c.Tasks[key] = task
+	return nil
+}
+
 func buildTaskKey(task Task) string {
 	hasName, ok := task.(HasName)
 	if !ok {