Repository: kubernetes-sigs/external-dns (language: go)

Issue:
Pi-Hole v6 AAAA dual record causes crashback loop
<!-- Please use this template while reporting a bug and provide as much info as possible. Not doing so may result in your bug not being addressed in a timely manner. Thanks!

-->

**What happened**:

When using AAAA dual records (e.g. `::ffff:192.168.20.3`), the Pi-Hole v6 client goes into a crash loop.

https://github.com/kubernetes-sigs/external-dns/pull/5226#issuecomment-2781088132

This may also effect the v5 client? I never tried to use these dual records with it, but it works fine with other external dns providers (e.g. UniFi webhook)

If I use `--exclude-record-types=AAAA` and manually create the records in Pi-Hole everything works fine as well, so I believe Pi-Hole is fine with these types of records.

**What you expected to happen**:

AAAA dual records should work. For instance, it works fine with the UniFi webhook.

**How to reproduce it (as minimally and precisely as possible)**:

Using the staging image `v20250403-v0.16.1-70-gc5af75e3`

I get the following error when I have the sources set to `service` (I get the same fatal error for both both `upsert-only` and `sync`, I'm using `sync` here when writing it up):

(No records at all in the `Pi-Hole` UI when I startup external-dns)

```sh
time="2025-04-03T21:41:04-05:00" level=debug msg="Endpoints generated from service: kube-system/cilium-gateway-internal: [internal.domain.name 0 IN A  192.168.20.3 [] internal.domain.name 0 IN AAAA  ::ffff:192.168.20.3 []]"
...
time="2025-04-03T21:41:04-05:00" level=info msg="PUT internal.domain.name IN AAAA -> ::ffff:192.168.20.3"
time="2025-04-03T21:41:04-05:00" level=info msg="PUT internal.domain.name IN A -> 192.168.20.3"
...
time="2025-04-03T21:41:09-05:00" level=debug msg="Endpoints generated from service: kube-system/cilium-gateway-internal: [internal.domain.name 0 IN A  192.168.20.3 [] internal.domain.name 0 IN AAAA  ::ffff:192.168.20.3 []]"
...
time="2025-04-03T21:41:09-05:00" level=info msg="PUT internal.domain.name IN AAAA -> ::ffff:192.168.20.3"
time="2025-04-03T21:41:09-05:00" level=debug msg="Error on request %!s(<nil>)"
time="2025-04-03T21:41:09-05:00" level=fatal msg="Failed to do run once: received 400 status code from request: [bad_request] Item already present (Uniqueness of items is enforced) - 0.000316s"
```

(These are the only logs with debug enabled that include `192.168.20.3`)

This causes the pod to enter `CrashLoopBackOff`.

When the pod retries, it shows instead:

```sh
time="2025-04-03T21:48:40-05:00" level=debug msg="Endpoints generated from service: kube-system/cilium-gateway-external: [internal.domain.name 0 IN A  192.168.20.3 [] internal.domain.name 0 IN AAAA  ::ffff:192.1
68.20.3 []]"
...
time="2025-04-03T21:48:40-05:00" level=info msg="PUT internal.domain.name IN AAAA -> ::ffff:192.168.20.3"
time="2025-04-03T21:48:40-05:00" level=debug msg="Error on request %!s(<nil>)"
time="2025-04-03T21:48:40-05:00" level=fatal msg="Failed to do run once: received 400 status code from request: [bad_request] Item already present (Uniqueness of items is enforced) - 0.000280s"
```

(So only the one, instead of showing it twice)

If I switch from `service` as the source to `gateway-httproute`, the `A` record get deleted and my `CNAME` records get created as expected, but the `AAAA` records do not get deleted, I still see them in the Pi-Hole UI. (And I see the `All records are already up to date`, the pod does not crash)

If I then switch back again from `gateway-httproute` to `service`, all the `CNAME` records get deleted, but I get the fatal error before the `A` record even get created, since it fails to re-create the `AAAA` record first.

Adding `--exclude-record-types=AAAA` fixes the crash, but obviously the `AAAA` records do not get created.

**Anything else we need to know?**:

**Environment**:

- External-DNS version (use `external-dns --version`): v20250403-v0.16.1-70-gc5af75e3
- DNS provider: Pi-Hole v6
- Others:
 `pihole-FTL --version`: v6.1


Discussion / hints:

Fix patch:
diff --git a/provider/pihole/clientV6.go b/provider/pihole/clientV6.go
index ae6ae399b3..ebaaabd6a2 100644
--- a/provider/pihole/clientV6.go
+++ b/provider/pihole/clientV6.go
@@ -25,6 +25,7 @@ import (
 	"fmt"
 	"io"
 	"net/http"
+	"net/netip"
 	"net/url"
 	"strconv"
 	"strings"
@@ -63,6 +64,7 @@ func newPiholeClientV6(cfg PiholeConfig) (piholeAPI, error) {
 			},
 		},
 	}
+
 	cl := instrumented_http.NewClient(httpClient, &instrumented_http.Callbacks{})
 
 	p := &piholeClientV6{
@@ -114,6 +116,32 @@ func (p *piholeClientV6) getConfigValue(ctx context.Context, rtype string) ([]st
 	return results, nil
 }
 
+/**
+ * isValidIPv4 checks if the given IP address is a valid IPv4 address.
+ * It returns true if the IP address is valid, false otherwise.
+ * If the IP address is in IPv6 format, it will return false.
+ */
+func isValidIPv4(ip string) bool {
+	addr, err := netip.ParseAddr(ip)
+	if err != nil {
+		return false
+	}
+	return addr.Is4()
+}
+
+/**
+ * isValidIPv6 checks if the given IP address is a valid IPv6 address.
+ * It returns true if the IP address is valid, false otherwise.
+ * If the IP address is in IPv6 with dual format y:y:y:y:y:y:x.x.x.x. , it will return true.
+ */
+func isValidIPv6(ip string) bool {
+	addr, err := netip.ParseAddr(ip)
+	if err != nil {
+		return false
+	}
+	return addr.Is6()
+}
+
 func (p *piholeClientV6) listRecords(ctx context.Context, rtype string) ([]*endpoint.Endpoint, error) {
 	out := make([]*endpoint.Endpoint, 0)
 	results, err := p.getConfigValue(ctx, rtype)
@@ -126,42 +154,39 @@ func (p *piholeClientV6) listRecords(ctx context.Context, rtype string) ([]*endp
 			return r == ' ' || r == ','
 		})
 		if len(recs) < 2 {
-			log.Warnf("skipping record %s: invalid format", rec)
+			log.Warnf("skipping record %s: invalid format received from PiHole", rec)
 			continue
 		}
 		var DNSName, Target string
-		var Ttl endpoint.TTL = 0
+		var Ttl = endpoint.TTL(0)
 		// A/AAAA record format is target(IP) DNSName
 		DNSName, Target = recs[1], recs[0]
-
 		switch rtype {
 		case endpoint.RecordTypeA:
-			if strings.Contains(Target, ":") {
+			//PiHole return A and AAAA records. Filter to only keep the A records
+			if !isValidIPv4(Target) {
 				continue
 			}
 		case endpoint.RecordTypeAAAA:
-			if strings.Contains(Target, ".") {
+			//PiHole return A and AAAA records. Filter to only keep the AAAA records
+			if !isValidIPv6(Target) {
 				continue
 			}
 		case endpoint.RecordTypeCNAME:
-			// CNAME format is DNSName,target
+			//PiHole return only CNAME records.
+			// CNAME format is DNSName,target, ttl?
 			DNSName, Target = recs[0], recs[1]
 			if len(recs) == 3 { // TTL is present
 				// Parse string to int64 first
 				if ttlInt, err := strconv.ParseInt(recs[2], 10, 64); err == nil {
 					Ttl = endpoint.TTL(ttlInt)
 				} else {
-					log.Warnf("failed to parse TTL value '%s': %v; using a TTL of %d", recs[2], err, Ttl)
+					log.Warnf("failed to parse TTL value received from PiHole '%s': %v; using a TTL of %d", recs[2], err, Ttl)
 				}
 			}
 		}
 
-		out = append(out, &endpoint.Endpoint{
-			DNSName:    DNSName,
-			Targets:    []string{Target},
-			RecordTTL:  Ttl,
-			RecordType: rtype,
-		})
+		out = append(out, endpoint.NewEndpointWithTTL(DNSName, rtype, Ttl, Target))
 	}
 	return out, nil
 }
@@ -375,7 +400,13 @@ func (p *piholeClientV6) do(req *http.Request) ([]byte, error) {
 		if err := json.Unmarshal(jRes, &apiError); err != nil {
 			return nil, fmt.Errorf("failed to unmarshal error response: %w", err)
 		}
-		log.Debugf("Error on request %s", req.Body)
+		if log.IsLevelEnabled(log.DebugLevel) {
+			log.Debugf("Error on request %s", req.URL)
+			if req.Body != nil {
+				log.Debugf("Body of the request %s", req.Body)
+			}
+		}
+
 		if res.StatusCode == http.StatusUnauthorized && p.token != "" {
 			tryCount := 1
 			maxRetries := 3


Test patch:
diff --git a/provider/pihole/clientV6_test.go b/provider/pihole/clientV6_test.go
index 664d88aa15..6975717928 100644
--- a/provider/pihole/clientV6_test.go
+++ b/provider/pihole/clientV6_test.go
@@ -29,6 +29,62 @@ import (
 	"sigs.k8s.io/external-dns/endpoint"
 )
 
+func TestIsValidIPv4(t *testing.T) {
+	tests := []struct {
+		ip       string
+		expected bool
+	}{
+		{"192.168.1.1", true},
+		{"255.255.255.255", true},
+		{"0.0.0.0", true},
+		{"", false},
+		{"256.256.256.256", false},
+		{"192.168.0.1/22", false},
+		{"192.168.1", false},
+		{"abc.def.ghi.jkl", false},
+		{"::ffff:192.168.20.3", false},
+	}
+
+	for _, test := range tests {
+		t.Run(test.ip, func(t *testing.T) {
+			if got := isValidIPv4(test.ip); got != test.expected {
+				t.Errorf("isValidIPv4(%s) = %v; want %v", test.ip, got, test.expected)
+			}
+		})
+	}
+}
+
+func TestIsValidIPv6(t *testing.T) {
+	tests := []struct {
+		ip       string
+		expected bool
+	}{
+		{"2001:0db8:85a3:0000:0000:8a2e:0370:7334", true},
+		{"2001:db8:85a3::8a2e:370:7334", true},
+		//IPV6 dual, the format is y:y:y:y:y:y:x.x.x.x.
+		{"::ffff:192.168.20.3", true},
+		{"::1", true},
+		{"::", true},
+		{"2001:db8::", true},
+		{"", false},
+		{":", false},
+		{"::ffff:", false},
+		{"192.168.20.3", false},
+		{"2001:db8:85a3:0:0:8a2e:370:7334:1234", false},
+		{"2001:db8:85a3::8a2e:370g:7334", false},
+		{"2001:db8:85a3::8a2e:370:7334::", false},
+		{"2001:db8:85a3::8a2e:370:7334::1", false},
+	}
+
+	for _, test := range tests {
+		t.Run(test.ip, func(t *testing.T) {
+			if got := isValidIPv6(test.ip); got != test.expected {
+				t.Errorf("isValidIPv6(%s) = %v; want %v", test.ip, got, test.expected)
+			}
+		})
+	}
+}
+
 func newTestServerV6(t *testing.T, hdlr http.HandlerFunc) *httptest.Server {
 	t.Helper()
 	svr := httptest.NewServer(hdlr)
@@ -137,7 +193,9 @@ func TestListRecordsV6(t *testing.T) {
 							"192.168.178.34 service3.example.com",
 							"fc00::1:192:168:1:1 service4.example.com",
 							"fc00::1:192:168:1:2 service5.example.com",
-							"fc00::1:192:168:1:3 service6.example.com"
+							"fc00::1:192:168:1:3 service6.example.com",
+							"::ffff:192.168.20.3 service7.example.com",
+							"192.168.20.3 service7.example.com"
 						]
 					}
 				},
@@ -177,20 +235,22 @@ func TestListRecordsV6(t *testing.T) {
 		t.Fatal(err)
 	}
 
+	// Ensure A records were parsed correctly
+	expected := [][]string{
+		{"service1.example.com", "192.168.178.33"},
+		{"service2.example.com", "192.168.178.34"},
+		{"service3.example.com", "192.168.178.34"},
+		{"service7.example.com", "192.168.20.3"},
+	}
 	// Test retrieve A records unfiltered
 	arecs, err := cl.listRecords(context.Background(), endpoint.RecordTypeA)
 	if err != nil {
 		t.Fatal(err)
 	}
-	if len(arecs) != 3 {
-		t.Fatal("Expected 3 A records returned, got:", len(arecs))
-	}
-	// Ensure records were parsed correctly
-	expected := [][]string{
-		{"service1.example.com", "192.168.178.33"},
-		{"service2.example.com", "192.168.178.34"},
-		{"service3.example.com", "192.168.178.34"},
+	if len(arecs) != len(expected) {
+		t.Fatalf("Expected %d A records returned, got: %d", len(expected), len(arecs))
 	}
+
 	for idx, rec := range arecs {
 		if rec.DNSName != expected[idx][0] {
 			t.Error("Got invalid DNS Name:", rec.DNSName, "expected:", expected[idx][0])
@@ -200,20 +260,23 @@ func TestListRecordsV6(t *testing.T) {
 		}
 	}
 
+	// Ensure AAAA records were parsed correctly
+	expected = [][]string{
+		{"service4.example.com", "fc00::1:192:168:1:1"},
+		{"service5.example.com", "fc00::1:192:168:1:2"},
+		{"service6.example.com", "fc00::1:192:168:1:3"},
+		{"service7.example.com", "::ffff:192.168.20.3"},
+	}
 	// Test retrieve AAAA records unfiltered
 	arecs, err = cl.listRecords(context.Background(), endpoint.RecordTypeAAAA)
 	if err != nil {
 		t.Fatal(err)
 	}
-	if len(arecs) != 3 {
-		t.Fatal("Expected 3 AAAA records returned, got:", len(arecs))
-	}
-	// Ensure records were parsed correctly
-	expected = [][]string{
-		{"service4.example.com", "fc00::1:192:168:1:1"},
-		{"service5.example.com", "fc00::1:192:168:1:2"},
-		{"service6.example.com", "fc00::1:192:168:1:3"},
+
+	if len(arecs) != len(expected) {
+		t.Fatalf("Expected %d AAAA records returned, got: %d", len(expected), len(arecs))
 	}
+
 	for idx, rec := range arecs {
 		if rec.DNSName != expected[idx][0] {
 			t.Error("Got invalid DNS Name:", rec.DNSName, "expected:", expected[idx][0])
@@ -223,20 +286,22 @@ func TestListRecordsV6(t *testing.T) {
 		}
 	}
 
+	// Ensure CNAME records were parsed correctly
+	expected = [][]string{
+		{"source1.example.com", "target1.domain.com", "1000"},
+		{"source2.example.com", "target2.domain.com", "50"},
+		{"source3.example.com", "target3.domain.com"},
+	}
+
 	// Test retrieve CNAME records unfiltered
 	cnamerecs, err := cl.listRecords(context.Background(), endpoint.RecordTypeCNAME)
 	if err != nil {
 		t.Fatal(err)
 	}
-	if len(cnamerecs) != 3 {
-		t.Fatal("Expected 3 CAME records returned, got:", len(cnamerecs))
-	}
-	// Ensure records were parsed correctly
-	expected = [][]string{
-		{"source1.example.com", "target1.domain.com", "1000"},
-		{"source2.example.com", "target2.domain.com", "50"},
-		{"source3.example.com", "target3.domain.com"},
+	if len(cnamerecs) != len(expected) {
+		t.Fatalf("Expected %d CAME records returned, got: %d", len(expected), len(cnamerecs))
 	}
+
 	for idx, rec := range cnamerecs {
 		if rec.DNSName != expected[idx][0] {
 			t.Error("Got invalid DNS Name:", rec.DNSName, "expected:", expected[idx][0])
@@ -261,6 +326,7 @@ func TestListRecordsV6(t *testing.T) {
 		t.Fatal("Expected error for using unsupported record type")
 	}
 }
+
 func TestErrorsV6(t *testing.T) {
 	//Error test cases