From 7cdfd6553d8119fca2b2a2d170e49ee425b9ed58 Mon Sep 17 00:00:00 2001
From: Ole Markus With <o.with@sportradar.com>
Date: Sat, 11 Sep 2021 10:39:05 +0200
Subject: [PATCH] Do not precreate dns record for api lbs

Precreating DNS records that kops cli will overwrite shortly after doesn't provide much benefit. It is also hard to say to external-dns it doesn't own those records.
---
 upup/pkg/fi/cloudup/dns.go      | 13 +++--
 upup/pkg/fi/cloudup/dns_test.go | 85 +++++++++++++++++++++++----------
 2 files changed, 67 insertions(+), 31 deletions(-)

diff --git a/upup/pkg/fi/cloudup/dns.go b/upup/pkg/fi/cloudup/dns.go
index c8e4fdb18c..d31b855e15 100644
--- a/upup/pkg/fi/cloudup/dns.go
+++ b/upup/pkg/fi/cloudup/dns.go
@@ -245,18 +245,17 @@ func precreateDNS(ctx context.Context, cluster *kops.Cluster, cloud fi.Cloud) er
 
 // buildPrecreateDNSHostnames returns the hostnames we should precreate
 func buildPrecreateDNSHostnames(cluster *kops.Cluster) []string {
-	var dnsHostnames []string
+	dnsHostnames := []string{}
 
-	if cluster.Spec.MasterPublicName != "" {
+	hasAPILoadbalancer := cluster.Spec.API != nil && cluster.Spec.API.LoadBalancer != nil
+	useLBForInternalAPI := hasAPILoadbalancer && cluster.Spec.API.LoadBalancer.UseForInternalApi
+
+	if cluster.Spec.MasterPublicName != "" && !hasAPILoadbalancer {
 		dnsHostnames = append(dnsHostnames, cluster.Spec.MasterPublicName)
-	} else {
-		klog.Warningf("cannot pre-create MasterPublicName - not set")
 	}
 
-	if cluster.Spec.MasterInternalName != "" {
+	if cluster.Spec.MasterInternalName != "" && !useLBForInternalAPI {
 		dnsHostnames = append(dnsHostnames, cluster.Spec.MasterInternalName)
-	} else {
-		klog.Warningf("cannot pre-create MasterInternalName - not set")
 	}
 
 	if apimodel.UseKopsControllerForNodeBootstrap(cluster) {
diff --git a/upup/pkg/fi/cloudup/dns_test.go b/upup/pkg/fi/cloudup/dns_test.go
index 19861f624a..51fb64d8d0 100644
--- a/upup/pkg/fi/cloudup/dns_test.go
+++ b/upup/pkg/fi/cloudup/dns_test.go
@@ -25,40 +25,77 @@ import (
 )
 
 func TestPrecreateDNSNames(t *testing.T) {
-	cluster := &kops.Cluster{}
-	cluster.ObjectMeta.Name = "cluster1.example.com"
-	cluster.Spec.MasterPublicName = "api." + cluster.ObjectMeta.Name
-	cluster.Spec.MasterInternalName = "api.internal." + cluster.ObjectMeta.Name
-	cluster.Spec.EtcdClusters = []kops.EtcdClusterSpec{
+
+	grid := []struct {
+		cluster  *kops.Cluster
+		expected []string
+	}{
 		{
-			Name: "main",
-			Members: []kops.EtcdMemberSpec{
-				{Name: "zone1"},
-				{Name: "zone2"},
-				{Name: "zone3"},
+			cluster: &kops.Cluster{},
+			expected: []string{
+				"api.cluster1.example.com",
+				"api.internal.cluster1.example.com",
 			},
 		},
 		{
-			Name: "events",
-			Members: []kops.EtcdMemberSpec{
-				{Name: "zonea"},
-				{Name: "zoneb"},
-				{Name: "zonec"},
+			cluster: &kops.Cluster{
+				Spec: kops.ClusterSpec{
+					API: &kops.AccessSpec{
+						LoadBalancer: &kops.LoadBalancerAccessSpec{},
+					},
+				},
 			},
+			expected: []string{
+				"api.internal.cluster1.example.com",
+			},
+		},
+		{
+			cluster: &kops.Cluster{
+				Spec: kops.ClusterSpec{
+					API: &kops.AccessSpec{
+						LoadBalancer: &kops.LoadBalancerAccessSpec{
+							UseForInternalApi: true,
+						},
+					},
+				},
+			},
+			expected: []string{},
 		},
 	}
 
-	actual := buildPrecreateDNSHostnames(cluster)
+	for _, g := range grid {
+		cluster := g.cluster
 
-	expected := []string{
-		"api.cluster1.example.com",
-		"api.internal.cluster1.example.com",
-	}
+		cluster.ObjectMeta.Name = "cluster1.example.com"
+		cluster.Spec.MasterPublicName = "api." + cluster.ObjectMeta.Name
+		cluster.Spec.MasterInternalName = "api.internal." + cluster.ObjectMeta.Name
+		cluster.Spec.EtcdClusters = []kops.EtcdClusterSpec{
+			{
+				Name: "main",
+				Members: []kops.EtcdMemberSpec{
+					{Name: "zone1"},
+					{Name: "zone2"},
+					{Name: "zone3"},
+				},
+			},
+			{
+				Name: "events",
+				Members: []kops.EtcdMemberSpec{
+					{Name: "zonea"},
+					{Name: "zoneb"},
+					{Name: "zonec"},
+				},
+			},
+		}
 
-	sort.Strings(actual)
-	sort.Strings(expected)
+		actual := buildPrecreateDNSHostnames(cluster)
 
-	if !reflect.DeepEqual(expected, actual) {
-		t.Fatalf("unexpected records.  expected=%v actual=%v", expected, actual)
+		expected := g.expected
+		sort.Strings(actual)
+		sort.Strings(expected)
+
+		if !reflect.DeepEqual(expected, actual) {
+			t.Errorf("unexpected records.  expected=%v actual=%v", expected, actual)
+		}
 	}
 }