Merge #149980

149980: roachprod: add A records during cluster create r=golgeek a=dt

Release note: none.
Epic: none.

Co-authored-by: David Taylor <[email protected]>
Co-authored-by: Ludovic Leroux <[email protected]>

Author: 3 people

pkg/roachprod/cloud/cluster_cloud.go

@@ -534,9 +534,16 @@ func DestroyCluster(l *logger.Logger, c *Cluster) error {
 	// DNS entries are destroyed first to ensure that the GC job will not try
 	// and clean-up entries prematurely.
 	stopSpinner := ui.NewDefaultSpinner(l, "Destroying DNS entries").Start()
+	publicRecords := make([]string, 0, len(c.VMs))
+	for _, v := range c.VMs {
+		publicRecords = append(publicRecords, v.PublicDNS)
+	}
 	dnsErr := vm.FanOutDNS(c.VMs, func(p vm.DNSProvider, vms vm.List) error {
-		return p.DeleteRecordsBySubdomain(context.Background(), c.Name)
+		publicRecordsErr := p.DeletePublicRecordsByName(context.Background(), publicRecords...)
+		srvRecordsErr := p.DeleteSRVRecordsBySubdomain(context.Background(), c.Name)
+		return errors.CombineErrors(publicRecordsErr, srvRecordsErr)
 	})
+
 	stopSpinner()
 
 	stopSpinner = ui.NewDefaultSpinner(l, "Destroying VMs").Start()

pkg/roachprod/cloud/gc.go

@@ -570,7 +570,7 @@ func GCDNS(l *logger.Logger, cloud *Cloud, dryrun bool) error {
 		sort.Strings(recordNames)
 
 		if err := destroyResource(dryrun, func() error {
-			return p.DeleteRecordsByName(ctx, recordNames...)
+			return p.DeleteSRVRecordsByName(ctx, recordNames...)
 		}); err != nil {
 			return err
 		}

pkg/roachprod/install/services.go

@@ -154,7 +154,7 @@ func (c *SyncedCluster) discoverServices(
 	mu := syncutil.Mutex{}
 	records := make([]vm.DNSRecord, 0)
 	err := vm.FanOutDNS(c.VMs, func(dnsProvider vm.DNSProvider, _ vm.List) error {
-		r, lookupErr := dnsProvider.LookupSRVRecords(ctx, serviceDNSName(dnsProvider, virtualClusterName, serviceType, c.Name))
+		r, lookupErr := dnsProvider.LookupRecords(ctx, vm.SRV, serviceDNSName(dnsProvider, virtualClusterName, serviceType, c.Name))
 		if lookupErr != nil {
 			return lookupErr
 		}

pkg/roachprod/install/services_test.go

@@ -334,7 +334,7 @@ func TestMultipleRegistrations(t *testing.T) {
 	verify := func(c *SyncedCluster, servicesToRegister [][]ServiceDesc) bool {
 		for _, services := range servicesToRegister {
 			if len(services) == 0 {
-				err := testDNS.DeleteRecordsBySubdomain(ctx, c.Name)
+				err := testDNS.DeleteSRVRecordsBySubdomain(ctx, c.Name)
 				require.NoError(t, err)
 				continue
 			}

pkg/roachprod/roachprod.go

@@ -1772,6 +1772,11 @@ func Create(
 		// No need for ssh for local clusters.
 		return LoadClusters()
 	}
+
+	if err := CreatePublicDNS(ctx, l, clusterName); err != nil {
+		l.Printf("Failed to create DNS for cluster %s: %v", clusterName, err)
+	}
+
 	l.Printf("Created cluster %s; setting up SSH...", clusterName)
 	return SetupSSH(ctx, l, clusterName, false /* sync */)
 }
@@ -2597,8 +2602,34 @@ func DestroyDNS(ctx context.Context, l *logger.Logger, clusterName string) error
 	if err != nil {
 		return err
 	}
+	publicRecords := make([]string, 0, len(c.VMs))
+	for _, v := range c.VMs {
+		publicRecords = append(publicRecords, v.PublicDNS)
+	}
+
+	return vm.FanOutDNS(c.VMs, func(p vm.DNSProvider, vms vm.List) error {
+		return errors.CombineErrors(
+			p.DeleteSRVRecordsBySubdomain(ctx, c.Name),
+			p.DeletePublicRecordsByName(ctx, publicRecords...),
+		)
+	})
+}
+
+// CreatePublicDNS creates or updates the public A records for the given cluster.
+func CreatePublicDNS(ctx context.Context, l *logger.Logger, clusterName string) error {
+	c, err := GetClusterFromCache(l, clusterName)
+	if err != nil {
+		return err
+	}
+
 	return vm.FanOutDNS(c.VMs, func(p vm.DNSProvider, vms vm.List) error {
-		return p.DeleteRecordsBySubdomain(ctx, c.Name)
+		recs := make([]vm.DNSRecord, 0, len(c.VMs))
+		for _, v := range c.VMs {
+			rec := vm.CreateDNSRecord(v.PublicDNS, vm.A, v.PublicIP, 60)
+			rec.Public = true
+			recs = append(recs, rec)
+		}
+		return p.CreateRecords(ctx, recs...)
 	})
 }
 

pkg/roachprod/vm/dns.go

@@ -38,6 +38,8 @@ type DNSRecord struct {
 	Data string `json:"data"`
 	// TTL is the time to live of the DNS record.
 	TTL int `json:"TTL"`
+	// Public indicates whether the DNS should be published in the public zone.
+	Public bool
 }
 
 // DNSProvider is an optional capability for a Provider that provides DNS
@@ -49,13 +51,15 @@ type DNSProvider interface {
 	// subdomain. The protocol is usually "tcp" and the subdomain is usually the
 	// cluster name. The service is a combination of the virtual cluster name and
 	// type of service.
-	LookupSRVRecords(ctx context.Context, name string) ([]DNSRecord, error)
+	LookupRecords(ctx context.Context, recordType DNSType, name string) ([]DNSRecord, error)
 	// ListRecords lists all DNS records managed for the zone.
 	ListRecords(ctx context.Context) ([]DNSRecord, error)
-	// DeleteRecordsBySubdomain deletes all DNS records with the given subdomain.
-	DeleteRecordsBySubdomain(ctx context.Context, subdomain string) error
-	// DeleteRecordsByName deletes all DNS records with the given name.
-	DeleteRecordsByName(ctx context.Context, names ...string) error
+	// DeleteSRVRecordsBySubdomain deletes all DNS SRV records with the given subdomain.
+	DeleteSRVRecordsBySubdomain(ctx context.Context, subdomain string) error
+	// DeleteRecordsByName deletes all DNS SRV records with the given name.
+	DeleteSRVRecordsByName(ctx context.Context, names ...string) error
+	// DeletePublicRecordsByName deletes all DNS A records named.
+	DeletePublicRecordsByName(ctx context.Context, names ...string) error
 	// Domain returns the domain name (zone) of the DNS provider.
 	Domain() string
 }

pkg/roachprod/vm/gce/dns.go

@@ -131,8 +131,12 @@ func (n *dnsProvider) CreateRecords(ctx context.Context, records ...vm.DNSRecord
 	}
 
 	for name, recordGroup := range recordsByName {
+		// We assume that all records in a group have the same name, type, and ttl.
+		// TODO(herko): Add error checking to ensure that the above is the case.
+		firstRecord := recordGroup[0]
+
 		err := n.withRecordLock(name, func() error {
-			existingRecords, err := n.lookupSRVRecords(ctx, name)
+			existingRecords, err := n.lookupRecords(ctx, firstRecord.Type, name)
 			if err != nil {
 				return err
 			}
@@ -151,15 +155,16 @@ func (n *dnsProvider) CreateRecords(ctx context.Context, records ...vm.DNSRecord
 				combinedRecords[record.Data] = record
 			}
 
-			// We assume that all records in a group have the same name, type, and ttl.
-			// TODO(herko): Add error checking to ensure that the above is the case.
-			firstRecord := recordGroup[0]
 			data := maps.Keys(combinedRecords)
 			sort.Strings(data)
+			zone := n.managedZone
+			if firstRecord.Public {
+				zone = n.publicZone
+			}
 			args := []string{"--project", n.dnsProject, "dns", "record-sets", command, name,
 				"--type", string(firstRecord.Type),
 				"--ttl", strconv.Itoa(firstRecord.TTL),
-				"--zone", n.managedZone,
+				"--zone", zone,
 				"--rrdatas", strings.Join(data, ","),
 			}
 			cmd := exec.CommandContext(ctx, "gcloud", args...)
@@ -170,10 +175,10 @@ func (n *dnsProvider) CreateRecords(ctx context.Context, records ...vm.DNSRecord
 				n.clearCacheEntry(name)
 				return rperrors.TransientFailure(errors.Wrapf(err, "output: %s", out), dnsProblemLabel)
 			}
-			// If fastDNS is enabled, we need to wait for the records to become available
+			// If fastDNS is enabled, we need to wait for the SRV records to become available
 			// on the Google DNS servers.
-			if config.FastDNS {
-				err = n.waitForRecordsAvailable(ctx, maps.Values(combinedRecords)...)
+			if config.FastDNS && !firstRecord.Public {
+				err = n.waitForSRVRecordsAvailable(ctx, maps.Values(combinedRecords)...)
 				if err != nil {
 					return err
 				}
@@ -190,33 +195,36 @@ func (n *dnsProvider) CreateRecords(ctx context.Context, records ...vm.DNSRecord
 }
 
 // LookupSRVRecords implements the vm.DNSProvider interface.
-func (n *dnsProvider) LookupSRVRecords(ctx context.Context, name string) ([]vm.DNSRecord, error) {
+func (n *dnsProvider) LookupRecords(
+	ctx context.Context, recordType vm.DNSType, name string,
+) ([]vm.DNSRecord, error) {
 	var records []vm.DNSRecord
 	var err error
 	err = n.withRecordLock(name, func() error {
-		if config.FastDNS {
+		if config.FastDNS && recordType == vm.SRV {
 			rIdx := randutil.FastUint32() % uint32(len(n.resolvers))
 			records, err = n.fastLookupSRVRecords(ctx, n.resolvers[rIdx], name, true)
 			return err
 		}
-		records, err = n.lookupSRVRecords(ctx, name)
+		records, err = n.lookupRecords(ctx, recordType, name)
 		return err
 	})
 	return records, err
 }
 
 // ListRecords implements the vm.DNSProvider interface.
 func (n *dnsProvider) ListRecords(ctx context.Context) ([]vm.DNSRecord, error) {
-	return n.listSRVRecords(ctx, "", dnsMaxResults)
+	return n.listRecords(ctx, vm.SRV, "", dnsMaxResults)
 }
 
-// DeleteRecordsByName implements the vm.DNSProvider interface.
-func (n *dnsProvider) DeleteRecordsByName(ctx context.Context, names ...string) error {
+func (n *dnsProvider) deleteRecords(
+	ctx context.Context, zone string, recordType vm.DNSType, names ...string,
+) error {
 	for _, name := range names {
 		err := n.withRecordLock(name, func() error {
 			args := []string{"--project", n.dnsProject, "dns", "record-sets", "delete", name,
-				"--type", string(vm.SRV),
-				"--zone", n.managedZone,
+				"--type", string(recordType),
+				"--zone", zone,
 			}
 			cmd := exec.CommandContext(ctx, "gcloud", args...)
 			out, err := n.execFn(cmd)
@@ -235,10 +243,20 @@ func (n *dnsProvider) DeleteRecordsByName(ctx context.Context, names ...string)
 	return nil
 }
 
+// DeleteSRVRecordsByName implements the vm.DNSProvider interface.
+func (n *dnsProvider) DeleteSRVRecordsByName(ctx context.Context, names ...string) error {
+	return n.deleteRecords(ctx, n.managedZone, vm.SRV, names...)
+}
+
+// DeletePublicRecordsByName implements the vm.DNSProvider interface
+func (n *dnsProvider) DeletePublicRecordsByName(ctx context.Context, names ...string) error {
+	return n.deleteRecords(ctx, n.publicZone, vm.A, names...)
+}
+
 // DeleteRecordsBySubdomain implements the vm.DNSProvider interface.
-func (n *dnsProvider) DeleteRecordsBySubdomain(ctx context.Context, subdomain string) error {
+func (n *dnsProvider) DeleteSRVRecordsBySubdomain(ctx context.Context, subdomain string) error {
 	suffix := fmt.Sprintf("%s.%s.", subdomain, n.Domain())
-	records, err := n.listSRVRecords(ctx, suffix, dnsMaxResults)
+	records, err := n.listRecords(ctx, vm.SRV, suffix, dnsMaxResults)
 	if err != nil {
 		return err
 	}
@@ -256,7 +274,7 @@ func (n *dnsProvider) DeleteRecordsBySubdomain(ctx context.Context, subdomain st
 			delete(names, name)
 		}
 	}
-	return n.DeleteRecordsByName(ctx, maps.Keys(names)...)
+	return n.DeleteSRVRecordsByName(ctx, maps.Keys(names)...)
 }
 
 // Domain implements the vm.DNSProvider interface.
@@ -272,13 +290,15 @@ func (n *dnsProvider) Domain() string {
 // network problems. For lookups, we prefer this to using the gcloud command as
 // it is faster, and preferable when service information is being queried
 // regularly.
-func (n *dnsProvider) lookupSRVRecords(ctx context.Context, name string) ([]vm.DNSRecord, error) {
+func (n *dnsProvider) lookupRecords(
+	ctx context.Context, recordType vm.DNSType, name string,
+) ([]vm.DNSRecord, error) {
 	// Check the cache first.
 	if cachedRecords, ok := n.getCache(name); ok {
 		return cachedRecords, nil
 	}
 	// Lookup the records, if no records are found in the cache.
-	records, err := n.listSRVRecords(ctx, name, dnsMaxResults)
+	records, err := n.listRecords(ctx, recordType, name, dnsMaxResults)
 	if err != nil {
 		return nil, err
 	}
@@ -295,16 +315,21 @@ func (n *dnsProvider) lookupSRVRecords(ctx context.Context, name string) ([]vm.D
 	return filteredRecords, nil
 }
 
-// listSRVRecords returns all SRV records that match the given filter from Google Cloud DNS.
+// listRecords returns all records that match the given filter from Google Cloud DNS.
 // The data field of the records could be a comma-separated list of values if multiple
 // records are returned for the same name.
-func (n *dnsProvider) listSRVRecords(
-	ctx context.Context, filter string, limit int,
+func (n *dnsProvider) listRecords(
+	ctx context.Context, recordType vm.DNSType, filter string, limit int,
 ) ([]vm.DNSRecord, error) {
+	zone := n.managedZone
+	if recordType == vm.A {
+		zone = n.publicZone
+	}
+
 	args := []string{"--project", n.dnsProject, "dns", "record-sets", "list",
 		"--limit", strconv.Itoa(limit),
 		"--page-size", strconv.Itoa(limit),
-		"--zone", n.managedZone,
+		"--zone", zone,
 		"--format", "json",
 	}
 	if filter != "" {
@@ -333,11 +358,11 @@ func (n *dnsProvider) listSRVRecords(
 		if record.Kind != "dns#resourceRecordSet" {
 			continue
 		}
-		if record.RecordType != string(vm.SRV) {
+		if record.RecordType != string(recordType) {
 			continue
 		}
 		for _, data := range record.RRDatas {
-			records = append(records, vm.CreateDNSRecord(record.Name, vm.SRV, data, record.TTL))
+			records = append(records, vm.CreateDNSRecord(record.Name, recordType, data, record.TTL))
 		}
 	}
 	return records, nil

pkg/roachprod/vm/gce/fast_dns.go

@@ -35,7 +35,9 @@ func googleDNSResolvers() []*net.Resolver {
 
 // waitForRecordsAvailable waits for the DNS records to become available on all
 // the DNS servers through a standard net tools lookup.
-func (n *dnsProvider) waitForRecordsAvailable(ctx context.Context, records ...vm.DNSRecord) error {
+func (n *dnsProvider) waitForSRVRecordsAvailable(
+	ctx context.Context, records ...vm.DNSRecord,
+) error {
 	checkResolver := func(resolver *net.Resolver) error {
 		type recordKey struct {
 			name string

pkg/roachprod/vm/local/dns.go

@@ -58,15 +58,17 @@ func (n *dnsProvider) CreateRecords(_ context.Context, records ...vm.DNSRecord)
 	return n.saveRecords(entries)
 }
 
-// LookupSRVRecords is part of the vm.DNSProvider interface.
-func (n *dnsProvider) LookupSRVRecords(_ context.Context, name string) ([]vm.DNSRecord, error) {
+// LookupRecords is part of the vm.DNSProvider interface.
+func (n *dnsProvider) LookupRecords(
+	_ context.Context, recordType vm.DNSType, name string,
+) ([]vm.DNSRecord, error) {
 	records, err := n.loadRecords()
 	if err != nil {
 		return nil, err
 	}
 	var matchingRecords []vm.DNSRecord
 	for _, record := range records {
-		if record.Name == name && record.Type == vm.SRV {
+		if record.Name == name && record.Type == recordType {
 			matchingRecords = append(matchingRecords, record)
 		}
 	}
@@ -82,8 +84,12 @@ func (n *dnsProvider) ListRecords(_ context.Context) ([]vm.DNSRecord, error) {
 	return maps.Values(records), nil
 }
 
+func (n *dnsProvider) DeletePublicRecordsByName(ctx context.Context, names ...string) error {
+	return n.DeleteSRVRecordsByName(ctx, names...)
+}
+
 // DeleteRecordsByName is part of the vm.DNSProvider interface.
-func (n *dnsProvider) DeleteRecordsByName(_ context.Context, names ...string) error {
+func (n *dnsProvider) DeleteSRVRecordsByName(_ context.Context, names ...string) error {
 	unlock, err := lock.AcquireFilesystemLock(n.lockFilePath)
 	if err != nil {
 		return err
@@ -100,8 +106,8 @@ func (n *dnsProvider) DeleteRecordsByName(_ context.Context, names ...string) er
 	return n.saveRecords(entries)
 }
 
-// DeleteRecordsBySubdomain is part of the vm.DNSProvider interface.
-func (n *dnsProvider) DeleteRecordsBySubdomain(_ context.Context, subdomain string) error {
+// DeleteSRVRecordsBySubdomain is part of the vm.DNSProvider interface.
+func (n *dnsProvider) DeleteSRVRecordsBySubdomain(_ context.Context, subdomain string) error {
 	unlock, err := lock.AcquireFilesystemLock(n.lockFilePath)
 	if err != nil {
 		return err

pkg/roachprod/vm/local/dns_test.go

@@ -48,7 +48,7 @@ func TestLookupRecords(t *testing.T) {
 	}...)
 
 	t.Run("lookup system", func(t *testing.T) {
-		records, err := p.LookupSRVRecords(ctx, "_system-sql._tcp.local.local-zone")
+		records, err := p.LookupRecords(ctx, vm.SRV, "_system-sql._tcp.local.local-zone")
 		require.NoError(t, err)
 		require.Equal(t, 3, len(records))
 		for _, r := range records {
@@ -58,7 +58,7 @@ func TestLookupRecords(t *testing.T) {
 	})
 
 	t.Run("parse SRV data", func(t *testing.T) {
-		records, err := p.LookupSRVRecords(ctx, "_tenant-1-sql._tcp.local.local-zone")
+		records, err := p.LookupRecords(ctx, vm.SRV, "_tenant-1-sql._tcp.local.local-zone")
 		require.NoError(t, err)
 		require.Equal(t, 1, len(records))
 		data, err := records[0].ParseSRVRecord()