Infoblox multiple A records support (#1479)

* Infoblox multiple A records support

* improve test coverage

provider/infoblox/infoblox.go

@@ -21,6 +21,7 @@ import (
 	"fmt"
 	"net/http"
 	"os"
+	"sort"
 	"strconv"
 	"strings"
 
@@ -157,7 +158,24 @@ func (p *InfobloxProvider) Records(ctx context.Context) (endpoints []*endpoint.E
 			return nil, fmt.Errorf("could not fetch A records from zone '%s': %s", zone.Fqdn, err)
 		}
 		for _, res := range resA {
-			endpoints = append(endpoints, endpoint.NewEndpoint(res.Name, endpoint.RecordTypeA, res.Ipv4Addr))
+			newEndpoint := endpoint.NewEndpoint(res.Name, endpoint.RecordTypeA, res.Ipv4Addr)
+			// Check if endpoint already exists and add to existing endpoint if it does
+			foundExisting := false
+			for _, ep := range endpoints {
+				if ep.DNSName == newEndpoint.DNSName && ep.RecordType == newEndpoint.RecordType {
+					logrus.Debugf("Adding target '%s' to existing A record '%s'", newEndpoint.Targets[0], ep.DNSName)
+					ep.Targets = append(ep.Targets, newEndpoint.Targets[0])
+					foundExisting = true
+					break
+				}
+			}
+			if !foundExisting {
+				endpoints = append(endpoints, newEndpoint)
+			}
+		}
+		// sort targets so that they are always in same order, as infoblox might return them in different order
+		for _, ep := range endpoints {
+			sort.Sort(ep.Targets)
 		}
 
 		// Include Host records since they should be treated synonymously with A records
@@ -309,14 +327,14 @@ func (p *InfobloxProvider) findZone(zones []ibclient.ZoneAuth, name string) *ibc
 	return result
 }
 
-func (p *InfobloxProvider) recordSet(ep *endpoint.Endpoint, getObject bool) (recordSet infobloxRecordSet, err error) {
+func (p *InfobloxProvider) recordSet(ep *endpoint.Endpoint, getObject bool, targetIndex int) (recordSet infobloxRecordSet, err error) {
 	switch ep.RecordType {
 	case endpoint.RecordTypeA:
 		var res []ibclient.RecordA
 		obj := ibclient.NewRecordA(
 			ibclient.RecordA{
 				Name:     ep.DNSName,
-				Ipv4Addr: ep.Targets[0],
+				Ipv4Addr: ep.Targets[targetIndex],
 				View:     p.view,
 			},
 		)
@@ -399,72 +417,26 @@ func (p *InfobloxProvider) createRecords(created infobloxChangeMap) {
 				zone,
 			)
 
-			recordSet, err := p.recordSet(ep, false)
-			if err != nil {
-				logrus.Errorf(
-					"Failed to retrieve %s record named '%s' to '%s' for DNS zone '%s': %v",
-					ep.RecordType,
-					ep.DNSName,
-					ep.Targets,
-					zone,
-					err,
-				)
-				continue
-			}
-			_, err = p.client.CreateObject(recordSet.obj)
-			if err != nil {
-				logrus.Errorf(
-					"Failed to create %s record named '%s' to '%s' for DNS zone '%s': %v",
-					ep.RecordType,
-					ep.DNSName,
-					ep.Targets,
-					zone,
-					err,
-				)
-			}
-		}
-	}
-}
-
-func (p *InfobloxProvider) deleteRecords(deleted infobloxChangeMap) {
-	// Delete records first
-	for zone, endpoints := range deleted {
-		for _, ep := range endpoints {
-			if p.dryRun {
-				logrus.Infof("Would delete %s record named '%s' for Infoblox DNS zone '%s'.", ep.RecordType, ep.DNSName, zone)
-			} else {
-				logrus.Infof("Deleting %s record named '%s' for Infoblox DNS zone '%s'.", ep.RecordType, ep.DNSName, zone)
-				recordSet, err := p.recordSet(ep, true)
+			for targetIndex := range ep.Targets {
+				recordSet, err := p.recordSet(ep, false, targetIndex)
 				if err != nil {
 					logrus.Errorf(
 						"Failed to retrieve %s record named '%s' to '%s' for DNS zone '%s': %v",
 						ep.RecordType,
 						ep.DNSName,
-						ep.Targets,
+						ep.Targets[targetIndex],
 						zone,
 						err,
 					)
 					continue
 				}
-				switch ep.RecordType {
-				case endpoint.RecordTypeA:
-					for _, record := range *recordSet.res.(*[]ibclient.RecordA) {
-						_, err = p.client.DeleteObject(record.Ref)
-					}
-				case endpoint.RecordTypeCNAME:
-					for _, record := range *recordSet.res.(*[]ibclient.RecordCNAME) {
-						_, err = p.client.DeleteObject(record.Ref)
-					}
-				case endpoint.RecordTypeTXT:
-					for _, record := range *recordSet.res.(*[]ibclient.RecordTXT) {
-						_, err = p.client.DeleteObject(record.Ref)
-					}
-				}
+				_, err = p.client.CreateObject(recordSet.obj)
 				if err != nil {
 					logrus.Errorf(
-						"Failed to delete %s record named '%s' for Infoblox DNS zone '%s': %v",
+						"Failed to create %s record named '%s' to '%s' for DNS zone '%s': %v",
 						ep.RecordType,
 						ep.DNSName,
+						ep.Targets[targetIndex],
 						zone,
 						err,
 					)
@@ -474,6 +446,56 @@ func (p *InfobloxProvider) deleteRecords(deleted infobloxChangeMap) {
 	}
 }
 
+func (p *InfobloxProvider) deleteRecords(deleted infobloxChangeMap) {
+	// Delete records first
+	for zone, endpoints := range deleted {
+		for _, ep := range endpoints {
+			if p.dryRun {
+				logrus.Infof("Would delete %s record named '%s' for Infoblox DNS zone '%s'.", ep.RecordType, ep.DNSName, zone)
+			} else {
+				logrus.Infof("Deleting %s record named '%s' for Infoblox DNS zone '%s'.", ep.RecordType, ep.DNSName, zone)
+				for targetIndex := range ep.Targets {
+					recordSet, err := p.recordSet(ep, true, targetIndex)
+					if err != nil {
+						logrus.Errorf(
+							"Failed to retrieve %s record named '%s' to '%s' for DNS zone '%s': %v",
+							ep.RecordType,
+							ep.DNSName,
+							ep.Targets[targetIndex],
+							zone,
+							err,
+						)
+						continue
+					}
+					switch ep.RecordType {
+					case endpoint.RecordTypeA:
+						for _, record := range *recordSet.res.(*[]ibclient.RecordA) {
+							_, err = p.client.DeleteObject(record.Ref)
+						}
+					case endpoint.RecordTypeCNAME:
+						for _, record := range *recordSet.res.(*[]ibclient.RecordCNAME) {
+							_, err = p.client.DeleteObject(record.Ref)
+						}
+					case endpoint.RecordTypeTXT:
+						for _, record := range *recordSet.res.(*[]ibclient.RecordTXT) {
+							_, err = p.client.DeleteObject(record.Ref)
+						}
+					}
+					if err != nil {
+						logrus.Errorf(
+							"Failed to delete %s record named '%s' for Infoblox DNS zone '%s': %v",
+							ep.RecordType,
+							ep.DNSName,
+							zone,
+							err,
+						)
+					}
+				}
+			}
+		}
+	}
+}
+
 func lookupEnvAtoi(key string, fallback int) (i int) {
 	val, ok := os.LookupEnv(key)
 	if !ok {

provider/infoblox/infoblox_test.go

@@ -327,6 +327,18 @@ func createMockInfobloxObject(name, recordType, value string) ibclient.IBObject
 				Text: value,
 			},
 		)
+	case "HOST":
+		return ibclient.NewHostRecord(
+			ibclient.HostRecord{
+				Ref:  ref,
+				Name: name,
+				Ipv4Addrs: []ibclient.HostRecordIpv4Addr{
+					{
+						Ipv4Addr: value,
+					},
+				},
+			},
+		)
 	}
 	return nil
 }
@@ -354,6 +366,13 @@ func TestInfobloxRecords(t *testing.T) {
 			createMockInfobloxObject("whitespace.example.com", endpoint.RecordTypeA, "123.123.123.124"),
 			createMockInfobloxObject("whitespace.example.com", endpoint.RecordTypeTXT, "heritage=external-dns,external-dns/owner=white space"),
 			createMockInfobloxObject("hack.example.com", endpoint.RecordTypeCNAME, "cerberus.infoblox.com"),
+			createMockInfobloxObject("multiple.example.com", endpoint.RecordTypeA, "123.123.123.122"),
+			createMockInfobloxObject("multiple.example.com", endpoint.RecordTypeA, "123.123.123.121"),
+			createMockInfobloxObject("multiple.example.com", endpoint.RecordTypeTXT, "heritage=external-dns,external-dns/owner=default"),
+			createMockInfobloxObject("existing.example.com", endpoint.RecordTypeA, "124.1.1.1"),
+			createMockInfobloxObject("existing.example.com", endpoint.RecordTypeA, "124.1.1.2"),
+			createMockInfobloxObject("existing.example.com", endpoint.RecordTypeTXT, "heritage=external-dns,external-dns/owner=existing"),
+			createMockInfobloxObject("host.example.com", "HOST", "125.1.1.1"),
 		},
 	}
 
@@ -371,6 +390,11 @@ func TestInfobloxRecords(t *testing.T) {
 		endpoint.NewEndpoint("whitespace.example.com", endpoint.RecordTypeA, "123.123.123.124"),
 		endpoint.NewEndpoint("whitespace.example.com", endpoint.RecordTypeTXT, "\"heritage=external-dns,external-dns/owner=white space\""),
 		endpoint.NewEndpoint("hack.example.com", endpoint.RecordTypeCNAME, "cerberus.infoblox.com"),
+		endpoint.NewEndpoint("multiple.example.com", endpoint.RecordTypeA, "123.123.123.122", "123.123.123.121"),
+		endpoint.NewEndpoint("multiple.example.com", endpoint.RecordTypeTXT, "\"heritage=external-dns,external-dns/owner=default\""),
+		endpoint.NewEndpoint("existing.example.com", endpoint.RecordTypeA, "124.1.1.1", "124.1.1.2"),
+		endpoint.NewEndpoint("existing.example.com", endpoint.RecordTypeTXT, "\"heritage=external-dns,external-dns/owner=existing\""),
+		endpoint.NewEndpoint("host.example.com", endpoint.RecordTypeA, "125.1.1.1"),
 	}
 	validateEndpoints(t, actual, expected)
 }
@@ -391,12 +415,15 @@ func TestInfobloxApplyChanges(t *testing.T) {
 		endpoint.NewEndpoint("other.com", endpoint.RecordTypeTXT, "tag"),
 		endpoint.NewEndpoint("new.example.com", endpoint.RecordTypeA, "111.222.111.222"),
 		endpoint.NewEndpoint("newcname.example.com", endpoint.RecordTypeCNAME, "other.com"),
+		endpoint.NewEndpoint("multiple.example.com", endpoint.RecordTypeA, "1.2.3.4,3.4.5.6,8.9.10.11"),
+		endpoint.NewEndpoint("multiple.example.com", endpoint.RecordTypeTXT, "tag-multiple-A-records"),
 	})
 
 	validateEndpoints(t, client.deletedEndpoints, []*endpoint.Endpoint{
 		endpoint.NewEndpoint("old.example.com", endpoint.RecordTypeA, ""),
 		endpoint.NewEndpoint("oldcname.example.com", endpoint.RecordTypeCNAME, ""),
 		endpoint.NewEndpoint("deleted.example.com", endpoint.RecordTypeA, ""),
+		endpoint.NewEndpoint("deleted.example.com", endpoint.RecordTypeTXT, ""),
 		endpoint.NewEndpoint("deletedcname.example.com", endpoint.RecordTypeCNAME, ""),
 	})
 
@@ -424,6 +451,7 @@ func testInfobloxApplyChangesInternal(t *testing.T, dryRun bool, client ibclient
 	}
 	client.(*mockIBConnector).mockInfobloxObjects = &[]ibclient.IBObject{
 		createMockInfobloxObject("deleted.example.com", endpoint.RecordTypeA, "121.212.121.212"),
+		createMockInfobloxObject("deleted.example.com", endpoint.RecordTypeTXT, "test-deleting-txt"),
 		createMockInfobloxObject("deletedcname.example.com", endpoint.RecordTypeCNAME, "other.com"),
 		createMockInfobloxObject("old.example.com", endpoint.RecordTypeA, "121.212.121.212"),
 		createMockInfobloxObject("oldcname.example.com", endpoint.RecordTypeCNAME, "other.com"),
@@ -447,6 +475,8 @@ func testInfobloxApplyChangesInternal(t *testing.T, dryRun bool, client ibclient
 		endpoint.NewEndpoint("other.com", endpoint.RecordTypeTXT, "tag"),
 		endpoint.NewEndpoint("nope.com", endpoint.RecordTypeA, "4.4.4.4"),
 		endpoint.NewEndpoint("nope.com", endpoint.RecordTypeTXT, "tag"),
+		endpoint.NewEndpoint("multiple.example.com", endpoint.RecordTypeA, "1.2.3.4,3.4.5.6,8.9.10.11"),
+		endpoint.NewEndpoint("multiple.example.com", endpoint.RecordTypeTXT, "tag-multiple-A-records"),
 	}
 
 	updateOldRecords := []*endpoint.Endpoint{
@@ -463,6 +493,7 @@ func testInfobloxApplyChangesInternal(t *testing.T, dryRun bool, client ibclient
 
 	deleteRecords := []*endpoint.Endpoint{
 		endpoint.NewEndpoint("deleted.example.com", endpoint.RecordTypeA, "121.212.121.212"),
+		endpoint.NewEndpoint("deleted.example.com", endpoint.RecordTypeTXT, "test-deleting-txt"),
 		endpoint.NewEndpoint("deletedcname.example.com", endpoint.RecordTypeCNAME, "other.com"),
 		endpoint.NewEndpoint("deleted.nope.com", endpoint.RecordTypeA, "222.111.222.111"),
 	}