Skip to content

fix(zonefinder): transform zone name to unicode as well #5980

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
rbeuque74 wants to merge 1 commit into
base: master
Choose a base branch
from
+69 −8
Open

fix(zonefinder): transform zone name to unicode as well #5980

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 30 additions & 7 deletions provider/ovh/ovh.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import (
"golang.org/x/sync/errgroup"

"sigs.k8s.io/external-dns/endpoint"
"sigs.k8s.io/external-dns/internal/idna"
"sigs.k8s.io/external-dns/pkg/apis/externaldns"
"sigs.k8s.io/external-dns/plan"
"sigs.k8s.io/external-dns/provider"
Expand Down Expand Up @@ -156,43 +157,55 @@ func (p *OVHProvider) Records(ctx context.Context) ([]*endpoint.Endpoint, error)
return endpoints, nil
}

func planChangesByZoneName(zones []string, changes *plan.Changes) map[string]*plan.Changes {
func planChangesByZoneName(zones []string, changes *plan.Changes) (map[string]*plan.Changes, error) {
zoneNameIDMapper := provider.ZoneIDName{}
for _, zone := range zones {
zoneNameIDMapper.Add(zone, zone)
}

output := map[string]*plan.Changes{}
for _, endpt := range changes.Delete {
_, zoneName := zoneNameIDMapper.FindZone(endpt.DNSName)
zoneName, _ := zoneNameIDMapper.FindZone(endpt.DNSName)
if zoneName == "" {
return nil, provider.NewSoftError(fmt.Errorf("record %q have not found matching DNS zone in OVH provider", endpt.DNSName))
Copy link
Member

@ivankatliarchuk ivankatliarchuk Dec 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return nil, provider.NewSoftError(fmt.Errorf("record %q have not found matching DNS zone in OVH provider", endpt.DNSName))
return nil, provider.NewSoftErrorf("record %q have not found matching DNS zone in OVH provider", endpt.DNSName)

}
if _, ok := output[zoneName]; !ok {
output[zoneName] = &plan.Changes{}
}
output[zoneName].Delete = append(output[zoneName].Delete, endpt)
}
for _, endpt := range changes.Create {
_, zoneName := zoneNameIDMapper.FindZone(endpt.DNSName)
zoneName, _ := zoneNameIDMapper.FindZone(endpt.DNSName)
if zoneName == "" {
return nil, provider.NewSoftError(fmt.Errorf("record %q have not found matching DNS zone in OVH provider", endpt.DNSName))
Copy link
Member

@ivankatliarchuk ivankatliarchuk Dec 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same NewSoftErrorf

}
if _, ok := output[zoneName]; !ok {
output[zoneName] = &plan.Changes{}
}
output[zoneName].Create = append(output[zoneName].Create, endpt)
}
for _, endpt := range changes.UpdateOld {
_, zoneName := zoneNameIDMapper.FindZone(endpt.DNSName)
zoneName, _ := zoneNameIDMapper.FindZone(endpt.DNSName)
if zoneName == "" {
return nil, provider.NewSoftError(fmt.Errorf("record %q have not found matching DNS zone in OVH provider", endpt.DNSName))
Copy link
Contributor

@szuecs szuecs Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same NewSoftErrorf

}
if _, ok := output[zoneName]; !ok {
output[zoneName] = &plan.Changes{}
}
output[zoneName].UpdateOld = append(output[zoneName].UpdateOld, endpt)
}
for _, endpt := range changes.UpdateNew {
_, zoneName := zoneNameIDMapper.FindZone(endpt.DNSName)
zoneName, _ := zoneNameIDMapper.FindZone(endpt.DNSName)
if zoneName == "" {
return nil, provider.NewSoftError(fmt.Errorf("record %q have not found matching DNS zone in OVH provider", endpt.DNSName))
Copy link
Contributor

@szuecs szuecs Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same NewSoftErrorf

}
if _, ok := output[zoneName]; !ok {
output[zoneName] = &plan.Changes{}
}
output[zoneName].UpdateNew = append(output[zoneName].UpdateNew, endpt)
}

return output
return output, nil
}

func (p *OVHProvider) computeSingleZoneChanges(_ context.Context, zoneName string, existingRecords []ovhRecord, changes *plan.Changes) ([]ovhChange, error) {
Expand Down Expand Up @@ -264,7 +277,10 @@ func (p *OVHProvider) ApplyChanges(ctx context.Context, changes *plan.Changes) e
}
}

changesByZoneName := planChangesByZoneName(zones, changes)
changesByZoneName, err := planChangesByZoneName(zones, changes)
if err != nil {
return err
}
eg, ctx := errgroup.WithContext(ctx)

for zoneName, changes := range changesByZoneName {
Expand Down Expand Up @@ -553,6 +569,13 @@ func (p *OVHProvider) newOvhChangeCreateDelete(action int, endpoints []*endpoint
}

func convertDNSNameIntoSubDomain(DNSName string, zoneName string) string { // nolint: gocritic // captLocal
if name, err := idna.Profile.ToUnicode(DNSName); err == nil {
Copy link
Member

@ivankatliarchuk ivankatliarchuk Dec 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not to first

if DNSName == zoneName {
		return ""
	}

and only then execute idna normalisation?

I have tried

func convertDNSNameIntoSubDomain(DNSName string, zoneName string) string { // nolint: gocritic // captLocal
	if DNSName == zoneName {
		return ""
	}

	if name, err := idna.Profile.ToUnicode(DNSName); err == nil {
		DNSName = name
	}
	if name, err := idna.Profile.ToUnicode(zoneName); err == nil {
		zoneName = name
	}

	return strings.TrimSuffix(DNSName, "."+zoneName)
}

All tests still green, either required a test that will fail or we could do the fastest check first.

DNSName = name
}
if name, err := idna.Profile.ToUnicode(zoneName); err == nil {
zoneName = name
}

if DNSName == zoneName {
return ""
}
Expand Down
21 changes: 21 additions & 0 deletions provider/ovh/ovh_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -585,6 +585,27 @@ func TestOvhApplyChanges(t *testing.T) {
client.AssertExpectations(t)
}

func TestOvhApplyChangesPunyCode(t *testing.T) {
client := new(mockOvhClient)
provider := &OVHProvider{client: client, apiRateLimiter: ratelimit.New(10), cacheInstance: cache.New(cache.NoExpiration, cache.NoExpiration)}
changes := plan.Changes{
Create: []*endpoint.Endpoint{
{DNSName: "example.testécassé.fr", RecordType: "A", RecordTTL: 10, Targets: []string{"203.0.113.42"}},
},
}

client.On("GetWithContext", "/domain/zone").Return([]string{"xn--testcass-e1ae.fr"}, nil).Once()
client.On("GetWithContext", "/domain/zone/xn--testcass-e1ae.fr/record").Return([]uint64{}, nil).Once()
client.On("PostWithContext", "/domain/zone/xn--testcass-e1ae.fr/record", ovhRecordFields{FieldType: "A", ovhRecordFieldUpdate: ovhRecordFieldUpdate{SubDomain: "example", TTL: 10, Target: "203.0.113.42"}}).Return(nil, nil).Once()
client.On("PostWithContext", "/domain/zone/xn--testcass-e1ae.fr/refresh", nil).Return(nil, nil).Once()

_, err := provider.Records(t.Context())
td.CmpNoError(t, err)
// Basic changes
td.CmpNoError(t, provider.ApplyChanges(t.Context(), &changes))
client.AssertExpectations(t)
}

func TestOvhChange(t *testing.T) {
assert := assert.New(t)
client := new(mockOvhClient)
Expand Down
7 changes: 6 additions & 1 deletion provider/zonefinder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,12 @@ import (
type ZoneIDName map[string]string

func (z ZoneIDName) Add(zoneID, zoneName string) {
z[zoneID] = zoneName
var err error
z[zoneID], err = idna.Profile.ToUnicode(zoneName)
if err != nil {
log.Warnf("failed to convert zonename %q to its Unicode form: %v", zoneName, err)
z[zoneID] = zoneName
}
}

// FindZone identifies the most suitable DNS zone for a given hostname.
Expand Down
12 changes: 12 additions & 0 deletions provider/zonefinder_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ func TestZoneIDName(t *testing.T) {
z.Add("1231231", "_foo._metadata.example.com")
z.Add("456456", "_metadata.エイミー.みんな")
z.Add("123412", "*.example.com")
// adding a zone as punycode, see that it is injected as unicode/international format
z.Add("234567", "xn--testcass-e1ae.fr")

assert.Equal(t, ZoneIDName{
"123456": "qux.baz",
Expand All @@ -44,6 +46,7 @@ func TestZoneIDName(t *testing.T) {
"1231231": "_foo._metadata.example.com",
"456456": "_metadata.エイミー.みんな",
"123412": "*.example.com",
"234567": "testécassé.fr",
}, z)

// simple entry in a domain
Expand Down Expand Up @@ -89,6 +92,15 @@ func TestZoneIDName(t *testing.T) {
assert.Equal(t, "*.example.com", zoneName)
assert.Equal(t, "123412", zoneID)

// looking for a zone that has been inserted as punycode
zoneID, zoneName = z.FindZone("example.testécassé.fr")
assert.Equal(t, "testécassé.fr", zoneName)
assert.Equal(t, "234567", zoneID)

zoneID, zoneName = z.FindZone("example.xn--testcass-e1ae.fr")
assert.Equal(t, "testécassé.fr", zoneName)
assert.Equal(t, "234567", zoneID)

hook := testutils.LogsUnderTestWithLogLevel(log.WarnLevel, t)
_, _ = z.FindZone("xn--not-a-valid-punycode")

Expand Down
Loading