Skip to content

Commit ff9da55

Browse files
authored
Merge pull request #704 from mikenairn/refactor/provider-zone-matching-errors
refactor(provider): Improve zone matching with sentinel errors
2 parents f718560 + a6863f9 commit ff9da55

File tree

5 files changed

+216
-154
lines changed

5 files changed

+216
-154
lines changed

internal/controller/dnsrecord_controller_test.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -951,7 +951,7 @@ var _ = Describe("DNSRecordReconciler", func() {
951951
"Type": Equal(string(v1alpha1.ConditionTypeReady)),
952952
"Status": Equal(metav1.ConditionFalse),
953953
"Reason": Equal("DNSProviderError"),
954-
"Message": Equal("Unable to find suitable zone in provider: no valid zone found for host: foo.noexist.com"),
954+
"Message": And(ContainSubstring("Unable to find suitable zone in provider"), ContainSubstring("foo.noexist.com")),
955955
"ObservedGeneration": Equal(dnsRecord.Generation),
956956
})),
957957
)
@@ -988,7 +988,7 @@ var _ = Describe("DNSRecordReconciler", func() {
988988
"Type": Equal(string(v1alpha1.ConditionTypeReady)),
989989
"Status": Equal(metav1.ConditionFalse),
990990
"Reason": Equal("DNSProviderError"),
991-
"Message": ContainSubstring("is an apex domain"),
991+
"Message": And(ContainSubstring("Unable to find suitable zone in provider"), ContainSubstring("apex domain not allowed")),
992992
"ObservedGeneration": Equal(dnsRecord.Generation),
993993
})),
994994
)
@@ -1029,7 +1029,7 @@ var _ = Describe("DNSRecordReconciler", func() {
10291029
"Type": Equal(string(v1alpha1.ConditionTypeReady)),
10301030
"Status": Equal(metav1.ConditionFalse),
10311031
"Reason": Equal("DNSProviderError"),
1032-
"Message": Equal(fmt.Sprintf("Unable to find suitable zone in provider: no valid zone found for host: %s", testHostname2)),
1032+
"Message": And(ContainSubstring("Unable to find suitable zone in provider"), ContainSubstring(testHostname2)),
10331033
"ObservedGeneration": Equal(dnsRecord.Generation),
10341034
})),
10351035
)

internal/provider/coredns/coredns.go

Lines changed: 5 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -79,22 +79,12 @@ func (p *CoreDNSProvider) DNSZones(_ context.Context) ([]provider.DNSZone, error
7979

8080
// DNSZoneForHost returns the DNSZone that best matches the given host in the providers list of zones
8181
func (p *CoreDNSProvider) DNSZoneForHost(ctx context.Context, host string) (*provider.DNSZone, error) {
82-
var assignedZone *provider.DNSZone
83-
zones, _ := p.DNSZones(ctx)
84-
for _, z := range zones {
85-
if strings.HasSuffix(host, z.DNSName) {
86-
if assignedZone == nil {
87-
assignedZone = &z
88-
}
89-
if len(assignedZone.DNSName) < len(z.DNSName) {
90-
assignedZone = &z
91-
}
92-
}
93-
}
94-
if assignedZone == nil {
95-
return nil, provider.ErrNoZoneForHost
82+
zones, err := p.DNSZones(ctx)
83+
if err != nil {
84+
return nil, err
9685
}
97-
return assignedZone, nil
86+
// CoreDNS supports apex domains (denyApex=false), similar to EndpointProvider
87+
return provider.FindDNSZoneForHost(ctx, host, zones, false)
9888
}
9989

10090
func (p *CoreDNSProvider) ProviderSpecific() provider.ProviderSpecificLabels {

internal/provider/coredns/coredns_test.go

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ package coredns_test
22

33
import (
44
"context"
5+
"errors"
56
"testing"
67

78
v1 "k8s.io/api/core/v1"
@@ -54,7 +55,22 @@ func TestCoreDNSProvider_DNSZoneForHost(t *testing.T) {
5455
},
5556
ExpectedZoneRoot: "",
5657
ExpectedErr: func(err error) bool {
57-
return err == provider.ErrNoZoneForHost
58+
// FindDNSZoneForHost consistently wraps ErrNoZoneForHost
59+
return errors.Is(err, provider.ErrNoZoneForHost)
60+
},
61+
},
62+
{
63+
Name: "test error when multiple zones with same name found",
64+
Host: "api.k.example.com",
65+
Secret: &v1.Secret{
66+
Data: map[string][]byte{
67+
"ZONES": []byte("k.example.com,k.example.com,example.com"),
68+
},
69+
},
70+
ExpectedZoneRoot: "",
71+
ExpectedErr: func(err error) bool {
72+
// FindDNSZoneForHost should return ErrMultipleZonesFound for duplicate zones
73+
return errors.Is(err, provider.ErrMultipleZonesFound)
5874
},
5975
},
6076
}

internal/provider/provider.go

Lines changed: 46 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -22,8 +22,35 @@ var (
2222
requestIDRegexp = regexp.MustCompile(`request id: [^\s]+`)
2323
saxParseExceptionRegexp = regexp.MustCompile(`Invalid XML ; javax.xml.stream.XMLStreamException: org.xml.sax.SAXParseException; lineNumber: [^\s]+; columnNumber: [^\s]+`)
2424

25+
// ErrNoZoneForHost is returned when no DNS zone can be found that matches the requested host.
26+
// This error occurs when:
27+
// - No zones are available from the provider
28+
// - The host is a top-level domain (TLD)
29+
// - The host doesn't match any available zone domains
30+
// - The host has invalid format (single label)
31+
//
32+
// Callers should use errors.Is(err, ErrNoZoneForHost) to check for this error.
2533
ErrNoZoneForHost = fmt.Errorf("no zone for host")
2634

35+
// ErrApexDomainNotAllowed is returned when the requested host matches an apex domain
36+
// (zone root) and the provider does not support apex domains (denyApex=true).
37+
// Apex domains can only have A/AAAA records, not CNAME records, so some providers
38+
// restrict their usage.
39+
//
40+
// Example: For zone "example.com", the host "example.com" is the apex domain.
41+
//
42+
// Callers should use errors.Is(err, ErrApexDomainNotAllowed) to check for this error.
43+
ErrApexDomainNotAllowed = fmt.Errorf("apex domain not allowed")
44+
45+
// ErrMultipleZonesFound is returned when multiple zones with the same DNS name exist,
46+
// making zone selection ambiguous. This typically indicates a configuration issue
47+
// where zone filters should be used to disambiguate.
48+
//
49+
// Example: Two zones both named "example.com" with different IDs.
50+
//
51+
// Callers should use errors.Is(err, ErrMultipleZonesFound) to check for this error.
52+
ErrMultipleZonesFound = fmt.Errorf("multiple zones found for host")
53+
2754
DNSProviderCoreDNS DNSProviderName = "coredns"
2855
DNSProviderAWS DNSProviderName = "aws"
2956
DNSProviderAzure DNSProviderName = "azure"
@@ -116,30 +143,34 @@ func IsWildCardHost(host string) bool {
116143
// findDNSZoneForHost will take a host and look for a zone that patches the immediate parent of that host and will continue to step through parents until it either finds a zone or fails. Example *.example.com will look for example.com and other.domain.example.com will step through each subdomain until it hits example.com.
117144
func findDNSZoneForHost(originalHost, host string, zones []DNSZone, denyApex bool) (*DNSZone, string, error) {
118145
if len(zones) == 0 {
119-
return nil, "", fmt.Errorf("%w : %s", ErrNoZoneForHost, host)
146+
return nil, "", fmt.Errorf("%w: %s", ErrNoZoneForHost, host)
120147
}
121148
host = strings.ToLower(host)
122149
//get the TLD from this host
123150
tld, _ := publicsuffix.PublicSuffix(host)
124151

125152
//The host is a TLD, so we now know `originalHost` can't possibly have a valid `DNSZone` available.
126153
if host == tld {
127-
return nil, "", fmt.Errorf("no valid zone found for host: %v", originalHost)
154+
return nil, "", fmt.Errorf("%w: %s", ErrNoZoneForHost, originalHost)
128155
}
129156

130157
// We do not currently support creating records for Apex domains, and a DNSZone represents an Apex domain we cannot setup dns for the host
131-
if id, is := IsApexDomain(originalHost, zones); is && denyApex {
132-
return nil, "", fmt.Errorf("host %s is an apex domain with zone id %s. Cannot configure DNS for apex domain as apex domains only support A records", originalHost, id)
158+
if _, is := IsApexDomain(originalHost, zones); is && denyApex {
159+
return nil, "", fmt.Errorf("%w: %s", ErrApexDomainNotAllowed, originalHost)
133160
}
134161

135162
hostParts := strings.SplitN(host, ".", 2)
136163
if len(hostParts) < 2 {
137-
return nil, "", fmt.Errorf("no valid zone found for host: %s", originalHost)
164+
return nil, "", fmt.Errorf("%w: %s", ErrNoZoneForHost, originalHost)
138165
}
139166
parentDomain := hostParts[1]
140-
// We do not currently support creating records for Apex domains, and a DNSZone represents an Apex domain, as such
141-
// we should never be trying to find a zone that matches the `originalHost` exactly. Instead, we just continue
142-
// on to the next possible valid host to try i.e. the parent domain.
167+
168+
// When apex domains are denied and we're on the first iteration (host == originalHost),
169+
// skip checking if the host itself is a zone and immediately recurse to the parent domain.
170+
// This prevents matching the host as an apex domain, which would be denied by the check above
171+
// after matching. Instead, we proactively skip to the parent to find a valid parent zone.
172+
// Example: For "example.com" with denyApex=true, skip directly to "com" instead of
173+
// potentially matching "example.com" as a zone and then failing the apex check.
143174
if host == originalHost && denyApex {
144175
return findDNSZoneForHost(originalHost, parentDomain, zones, denyApex)
145176
}
@@ -149,9 +180,14 @@ func findDNSZoneForHost(originalHost, host string, zones []DNSZone, denyApex boo
149180
})
150181
if len(matches) > 0 {
151182
if len(matches) > 1 {
152-
return nil, "", fmt.Errorf("multiple zones found for host: %s", originalHost)
183+
return nil, "", fmt.Errorf("%w: %s", ErrMultipleZonesFound, originalHost)
184+
}
185+
// Calculate subdomain by removing the zone suffix
186+
// For apex domains (where host == zone), subdomain should be empty
187+
subdomain := ""
188+
if strings.ToLower(originalHost) != strings.ToLower(matches[0].DNSName) {
189+
subdomain = strings.Replace(strings.ToLower(originalHost), "."+strings.ToLower(matches[0].DNSName), "", 1)
153190
}
154-
subdomain := strings.Replace(strings.ToLower(originalHost), "."+strings.ToLower(matches[0].DNSName), "", 1)
155191
return &matches[0], subdomain, nil
156192
}
157193

0 commit comments

Comments
 (0)