Skip to content

Commit 79321ee

Browse files
niv1612niv1612
committed
Select public hosted zone for Amazon-issued ACM DNS validation
The create-acm-cert (EnableCertificateManagement) feature writes the ACM DNS validation CNAME into the most-specific matching Route 53 hosted zone. In split-horizon DNS (a private zone that is a subdomain of a public zone) that most-specific zone is the private one. ACM validates Amazon-issued (public) certificates over public DNS, so the record never resolves and the certificate is stuck in PENDING_VALIDATION forever; the HTTPS listener never finalizes. Select the validation record's hosted zone from public zones only (nearest public ancestor) for Amazon-issued certificates. If no public zone matches, fail fast with an actionable error pointing at the certificate-arn annotation instead of creating a certificate that hangs in PENDING_VALIDATION. The delete path attempts cleanup in both the most-specific zone (records written by earlier controller versions) and the public zone (records written after this change), so validation records are not orphaned across the upgrade. Fixes #4840 Signed-off-by: niv1612 <35202955+niv1612@users.noreply.github.com>
1 parent 00935c0 commit 79321ee

7 files changed

Lines changed: 253 additions & 38 deletions

File tree

‎docs/guide/ingress/certificate_management.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,8 @@ Amazon Issued certificates are currently validated using DNS Method and Route53
3838
E-Mail validation is not supported due to significant higher delays between requesting a certificate and it's issuance.
3939
When using a PCA, certificates don't have to be validated.
4040

41+
Because ACM validates Amazon-issued certificates over **public** DNS, the controller writes the validation record into the nearest-ancestor **public** Route53 hosted zone. In split-horizon setups (a private zone that is a subdomain of a public zone), the private zone is skipped so the record lands where ACM can resolve it. If no public hosted zone matches the domain (private-only domain, or the public parent lives in an account the controller can't see), the controller fails fast. In that case, pre-create the certificate yourself and reference it with the [`certificate-arn`](annotations.md#certificate-arn) annotation.
42+
4143
## Ingress Group Behavior
4244

4345
When using certificate management with [IngressGroups](ingress_class.md#specgroup), each ingress in the group gets its own certificate based on its own hostnames. All certificates are attached to the shared ALB's HTTPS listener.

‎pkg/aws/services/route53.go‎

Lines changed: 30 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ const (
2222
type Route53 interface {
2323
ChangeRecordsWithContext(ctx context.Context, input *route53.ChangeResourceRecordSetsInput) (*route53.ChangeResourceRecordSetsOutput, error)
2424
GetHostedZoneID(ctx context.Context, domain string) (*string, error)
25+
GetPublicHostedZoneID(ctx context.Context, domain string) (*string, error)
2526
}
2627

2728
func NewRoute53(awsClientsProvider provider.AWSClientsProvider) Route53 {
@@ -57,11 +58,39 @@ func (c *route53Client) GetHostedZoneID(ctx context.Context, domain string) (*st
5758
return nil, err
5859
}
5960

61+
if bestID := findHostedZoneID(zones, domain, false); bestID != nil {
62+
return bestID, nil
63+
}
64+
65+
return nil, fmt.Errorf("no hosted zone found for validation records")
66+
}
67+
68+
// GetPublicHostedZoneID skips private zones: Amazon-issued ACM certificates are
69+
// validated over public DNS, so a validation record in a private zone (e.g. the
70+
// most-specific match in split-horizon Route 53) leaves the cert in PENDING_VALIDATION.
71+
func (c *route53Client) GetPublicHostedZoneID(ctx context.Context, domain string) (*string, error) {
72+
zones, err := c.listHostedZones(ctx)
73+
if err != nil {
74+
return nil, err
75+
}
76+
77+
if bestID := findHostedZoneID(zones, domain, true); bestID != nil {
78+
return bestID, nil
79+
}
80+
81+
return nil, fmt.Errorf("no public Route 53 hosted zone found for %q", domain)
82+
}
83+
84+
// findHostedZoneID returns the nearest-ancestor hosted zone (longest matching suffix).
85+
func findHostedZoneID(zones []types.HostedZone, domain string, publicOnly bool) *string {
6086
recParts := strings.Split(domain, ".")
6187

6288
var bestID *string
6389
bestLen := -1
6490
for _, zone := range zones {
91+
if publicOnly && zone.Config != nil && zone.Config.PrivateZone {
92+
continue
93+
}
6594
zoneParts := strings.Split(strings.TrimRight(*zone.Name, "."), ".")
6695
if len(zoneParts) > len(recParts) {
6796
continue
@@ -72,11 +101,7 @@ func (c *route53Client) GetHostedZoneID(ctx context.Context, domain string) (*st
72101
}
73102
}
74103

75-
if bestID != nil {
76-
return bestID, nil
77-
}
78-
79-
return nil, fmt.Errorf("no hosted zone found for validation records")
104+
return bestID
80105
}
81106

82107
func (c *route53Client) listHostedZones(ctx context.Context) ([]types.HostedZone, error) {

‎pkg/aws/services/route53_mocks.go‎

Lines changed: 15 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎pkg/aws/services/route53_test.go‎

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,14 @@ func hostedZone(id, name string) types.HostedZone {
2424
return types.HostedZone{Id: awssdk.String(id), Name: awssdk.String(name)}
2525
}
2626

27+
func privateHostedZone(id, name string) types.HostedZone {
28+
return types.HostedZone{
29+
Id: awssdk.String(id),
30+
Name: awssdk.String(name),
31+
Config: &types.HostedZoneConfig{PrivateZone: true},
32+
}
33+
}
34+
2735
func TestGetHostedZoneID(t *testing.T) {
2836
tests := []struct {
2937
name string
@@ -63,3 +71,65 @@ func TestGetHostedZoneID(t *testing.T) {
6371
})
6472
}
6573
}
74+
75+
func TestGetPublicHostedZoneID(t *testing.T) {
76+
tests := []struct {
77+
name string
78+
domain string
79+
zones []types.HostedZone
80+
want string
81+
wantErr bool
82+
}{
83+
{
84+
name: "split-horizon: public chosen even though private zone is more specific",
85+
domain: "app.sub.example.com",
86+
zones: []types.HostedZone{
87+
hostedZone("Z_PUBLIC", "example.com."),
88+
privateHostedZone("Z_PRIVATE", "sub.example.com."),
89+
},
90+
want: "Z_PUBLIC",
91+
},
92+
{
93+
name: "only a private zone matches: fail fast",
94+
domain: "app.sub.example.com",
95+
zones: []types.HostedZone{
96+
privateHostedZone("Z_PRIVATE", "sub.example.com."),
97+
},
98+
wantErr: true,
99+
},
100+
{
101+
name: "multiple public zones match: longest suffix wins",
102+
domain: "*.app.sub.example.com",
103+
zones: []types.HostedZone{
104+
hostedZone("Z_PARENT", "example.com."),
105+
hostedZone("Z_SUB", "sub.example.com."),
106+
},
107+
want: "Z_SUB",
108+
},
109+
}
110+
111+
for _, tt := range tests {
112+
t.Run(tt.name, func(t *testing.T) {
113+
c := newCachedRoute53Client(tt.zones)
114+
got, err := c.GetPublicHostedZoneID(context.Background(), tt.domain)
115+
if tt.wantErr {
116+
assert.Error(t, err)
117+
return
118+
}
119+
assert.NoError(t, err)
120+
assert.Equal(t, tt.want, awssdk.ToString(got))
121+
})
122+
}
123+
}
124+
125+
// The unfiltered lookup (delete path) must still resolve private zones so legacy
126+
// records written there can be cleaned up.
127+
func TestGetHostedZoneID_UnfilteredReturnsPrivate(t *testing.T) {
128+
c := newCachedRoute53Client([]types.HostedZone{
129+
hostedZone("Z_PUBLIC", "example.com."),
130+
privateHostedZone("Z_PRIVATE", "sub.example.com."),
131+
})
132+
got, err := c.GetHostedZoneID(context.Background(), "app.sub.example.com")
133+
assert.NoError(t, err)
134+
assert.Equal(t, "Z_PRIVATE", awssdk.ToString(got))
135+
}

‎pkg/deploy/acm/certificate_manager.go‎

Lines changed: 39 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -83,7 +83,7 @@ func (c *defaultCertificateManager) CreateWithValidationRecords(ctx context.Cont
8383
if _, checked := hostedZoneByDomain[host]; checked {
8484
continue
8585
}
86-
zoneID, err := c.route53Client.GetHostedZoneID(ctx, host)
86+
zoneID, err := c.route53Client.GetPublicHostedZoneID(ctx, host)
8787
if err != nil {
8888
return nil, fmt.Errorf("pre-check failed for domain %q: %w", host, err)
8989
}
@@ -237,37 +237,49 @@ func (c *defaultCertificateManager) DeleteWithValidationRecords(ctx context.Cont
237237
}
238238
return err
239239
}
240-
input := &route53sdk.ChangeResourceRecordSetsInput{
241-
HostedZoneId: id,
242-
ChangeBatch: &route53types.ChangeBatch{
243-
Changes: []route53types.Change{
244-
{
245-
Action: "DELETE",
246-
ResourceRecordSet: &route53types.ResourceRecordSet{
247-
Name: opts.ResourceRecord.Name,
248-
Type: route53types.RRType(opts.ResourceRecord.Type),
249-
TTL: awssdk.Int64(validationRecordTTL),
250-
ResourceRecords: []route53types.ResourceRecord{
251-
{
252-
Value: opts.ResourceRecord.Value,
240+
// The validation record lives in the nearest public zone (written by current
241+
// controllers) or, for certificates created by controller versions that
242+
// predate the public-zone selection fix (issue #4840), in the most-specific
243+
// zone regardless of visibility. Attempt cleanup in both when they differ;
244+
// a delete against the wrong zone fails with "not found", which is
245+
// tolerated below.
246+
zoneIDs := []*string{id}
247+
if publicID, err := c.route53Client.GetPublicHostedZoneID(ctx, awssdk.ToString(opts.DomainName)); err == nil && awssdk.ToString(publicID) != awssdk.ToString(id) {
248+
zoneIDs = append(zoneIDs, publicID)
249+
}
250+
for _, zoneID := range zoneIDs {
251+
input := &route53sdk.ChangeResourceRecordSetsInput{
252+
HostedZoneId: zoneID,
253+
ChangeBatch: &route53types.ChangeBatch{
254+
Changes: []route53types.Change{
255+
{
256+
Action: "DELETE",
257+
ResourceRecordSet: &route53types.ResourceRecordSet{
258+
Name: opts.ResourceRecord.Name,
259+
Type: route53types.RRType(opts.ResourceRecord.Type),
260+
TTL: awssdk.Int64(validationRecordTTL),
261+
ResourceRecords: []route53types.ResourceRecord{
262+
{
263+
Value: opts.ResourceRecord.Value,
264+
},
253265
},
254266
},
255267
},
256268
},
257269
},
258-
},
259-
}
260-
_, err = c.route53Client.ChangeRecordsWithContext(ctx, input)
261-
if err != nil && strings.Contains(err.Error(), "not found") {
262-
c.logger.Info("validation records no longer found, ignoring", "name", opts.ResourceRecord.Name, "value", opts.ResourceRecord.Value, "type", opts.ResourceRecord.Type)
263-
continue
264-
}
265-
if err != nil && strings.Contains(err.Error(), "do not match the current values") {
266-
c.logger.Info("validation records have been reused for another certificate, ignoring", "name", opts.ResourceRecord.Name, "value", opts.ResourceRecord.Value, "type", opts.ResourceRecord.Type)
267-
continue
268-
}
269-
if err != nil {
270-
return err
270+
}
271+
_, err = c.route53Client.ChangeRecordsWithContext(ctx, input)
272+
if err != nil && strings.Contains(err.Error(), "not found") {
273+
c.logger.Info("validation records no longer found, ignoring", "name", opts.ResourceRecord.Name, "value", opts.ResourceRecord.Value, "type", opts.ResourceRecord.Type)
274+
continue
275+
}
276+
if err != nil && strings.Contains(err.Error(), "do not match the current values") {
277+
c.logger.Info("validation records have been reused for another certificate, ignoring", "name", opts.ResourceRecord.Name, "value", opts.ResourceRecord.Value, "type", opts.ResourceRecord.Type)
278+
continue
279+
}
280+
if err != nil {
281+
return err
282+
}
271283
}
272284
}
273285
}

‎pkg/deploy/acm/certificate_manager_test.go‎

Lines changed: 89 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,89 @@
1+
package acm
2+
3+
import (
4+
"context"
5+
"errors"
6+
"testing"
7+
8+
awssdk "github.com/aws/aws-sdk-go-v2/aws"
9+
"github.com/aws/aws-sdk-go-v2/service/acm"
10+
acmtypes "github.com/aws/aws-sdk-go-v2/service/acm/types"
11+
"github.com/aws/aws-sdk-go-v2/service/route53"
12+
route53types "github.com/aws/aws-sdk-go-v2/service/route53/types"
13+
"github.com/go-logr/logr"
14+
"github.com/golang/mock/gomock"
15+
"github.com/stretchr/testify/assert"
16+
"sigs.k8s.io/aws-load-balancer-controller/pkg/aws/services"
17+
"sigs.k8s.io/controller-runtime/pkg/log"
18+
)
19+
20+
// Split-horizon cleanup: the unfiltered lookup resolves the private zone (legacy
21+
// records) while the public lookup resolves the public zone (records written by
22+
// current controllers). Delete must attempt cleanup in both zones.
23+
func TestDeleteWithValidationRecords_SplitHorizonCleansBothZones(t *testing.T) {
24+
ctrl := gomock.NewController(t)
25+
defer ctrl.Finish()
26+
27+
mockACM := services.NewMockACM(ctrl)
28+
mockRoute53 := services.NewMockRoute53(ctrl)
29+
m := &defaultCertificateManager{
30+
acmClient: mockACM,
31+
route53Client: mockRoute53,
32+
logger: logr.New(&log.NullLogSink{}),
33+
}
34+
35+
arn := "arn:aws:acm:us-east-1:123456789012:certificate/test"
36+
mockACM.EXPECT().DescribeCertificateWithContext(gomock.Any(), gomock.Eq(&acm.DescribeCertificateInput{
37+
CertificateArn: awssdk.String(arn),
38+
})).Return(&acm.DescribeCertificateOutput{
39+
Certificate: &acmtypes.CertificateDetail{
40+
DomainValidationOptions: []acmtypes.DomainValidation{
41+
{
42+
ValidationMethod: acmtypes.ValidationMethodDns,
43+
DomainName: awssdk.String("app.sub.example.com"),
44+
ResourceRecord: &acmtypes.ResourceRecord{
45+
Name: awssdk.String("cname-name"),
46+
Value: awssdk.String("cname-value"),
47+
Type: acmtypes.RecordTypeCname,
48+
},
49+
},
50+
},
51+
},
52+
}, nil)
53+
54+
mockRoute53.EXPECT().GetHostedZoneID(gomock.Any(), gomock.Eq("app.sub.example.com")).Return(awssdk.String("Z_PRIVATE"), nil)
55+
mockRoute53.EXPECT().GetPublicHostedZoneID(gomock.Any(), gomock.Eq("app.sub.example.com")).Return(awssdk.String("Z_PUBLIC"), nil)
56+
57+
deleteInput := func(zoneID string) *route53.ChangeResourceRecordSetsInput {
58+
return &route53.ChangeResourceRecordSetsInput{
59+
HostedZoneId: awssdk.String(zoneID),
60+
ChangeBatch: &route53types.ChangeBatch{
61+
Changes: []route53types.Change{
62+
{
63+
Action: "DELETE",
64+
ResourceRecordSet: &route53types.ResourceRecordSet{
65+
Name: awssdk.String("cname-name"),
66+
Type: route53types.RRType(acmtypes.RecordTypeCname),
67+
TTL: awssdk.Int64(validationRecordTTL),
68+
ResourceRecords: []route53types.ResourceRecord{
69+
{Value: awssdk.String("cname-value")},
70+
},
71+
},
72+
},
73+
},
74+
},
75+
}
76+
}
77+
// record was written to the public zone: private-zone delete fails "not found" (tolerated)
78+
mockRoute53.EXPECT().ChangeRecordsWithContext(gomock.Any(), gomock.Eq(deleteInput("Z_PRIVATE"))).
79+
Return(nil, errors.New("InvalidChangeBatch: Tried to delete resource record set but it was not found"))
80+
mockRoute53.EXPECT().ChangeRecordsWithContext(gomock.Any(), gomock.Eq(deleteInput("Z_PUBLIC"))).
81+
Return(&route53.ChangeResourceRecordSetsOutput{}, nil)
82+
83+
mockACM.EXPECT().DeleteCertificateWithContext(gomock.Any(), gomock.Eq(&acm.DeleteCertificateInput{
84+
CertificateArn: awssdk.String(arn),
85+
})).Return(&acm.DeleteCertificateOutput{}, nil)
86+
87+
err := m.DeleteWithValidationRecords(context.Background(), arn)
88+
assert.NoError(t, err)
89+
}

‎pkg/deploy/acm/certificate_synthesizer_test.go‎

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -83,7 +83,7 @@ func Test_Synthesizer(t *testing.T) {
8383
},
8484
}, nil)
8585

86-
mockRoute53.EXPECT().GetHostedZoneID(gomock.Any(), gomock.Eq("example.com")).Return(awssdk.String("Z0382403B3S5MSK4SVXX"), nil)
86+
mockRoute53.EXPECT().GetPublicHostedZoneID(gomock.Any(), gomock.Eq("example.com")).Return(awssdk.String("Z0382403B3S5MSK4SVXX"), nil)
8787

8888
mockRoute53.EXPECT().ChangeRecordsWithContext(gomock.Any(), gomock.Eq(&route53.ChangeResourceRecordSetsInput{
8989
HostedZoneId: awssdk.String("Z0382403B3S5MSK4SVXX"),
@@ -242,8 +242,8 @@ func Test_Synthesizer(t *testing.T) {
242242
},
243243
}, nil)
244244

245-
mockRoute53.EXPECT().GetHostedZoneID(gomock.Any(), gomock.Eq("example.com")).Return(awssdk.String("Z0382403B3S5MSK4SVXX"), nil)
246-
mockRoute53.EXPECT().GetHostedZoneID(gomock.Any(), gomock.Eq("otherexample.com")).Return(awssdk.String("Z0922506B3S0MGK4SALX"), nil)
245+
mockRoute53.EXPECT().GetPublicHostedZoneID(gomock.Any(), gomock.Eq("example.com")).Return(awssdk.String("Z0382403B3S5MSK4SVXX"), nil)
246+
mockRoute53.EXPECT().GetPublicHostedZoneID(gomock.Any(), gomock.Eq("otherexample.com")).Return(awssdk.String("Z0922506B3S0MGK4SALX"), nil)
247247
mockRoute53.EXPECT().ChangeRecordsWithContext(gomock.Any(), gomock.Eq(&route53.ChangeResourceRecordSetsInput{
248248
HostedZoneId: awssdk.String("Z0382403B3S5MSK4SVXX"),
249249
ChangeBatch: &route53types.ChangeBatch{
@@ -366,6 +366,8 @@ func Test_Synthesizer(t *testing.T) {
366366
}, nil)
367367

368368
mockRoute53.EXPECT().GetHostedZoneID(gomock.Any(), gomock.Eq("example.com")).Return(awssdk.String("Z0382403B3S5MSK4SVXX"), nil)
369+
// delete path also checks the public zone; same ID → single DELETE
370+
mockRoute53.EXPECT().GetPublicHostedZoneID(gomock.Any(), gomock.Eq("example.com")).Return(awssdk.String("Z0382403B3S5MSK4SVXX"), nil)
369371
mockRoute53.EXPECT().ChangeRecordsWithContext(gomock.Any(), gomock.Eq(&route53.ChangeResourceRecordSetsInput{
370372
HostedZoneId: awssdk.String("Z0382403B3S5MSK4SVXX"),
371373
ChangeBatch: &route53types.ChangeBatch{
@@ -411,7 +413,7 @@ func Test_Synthesizer(t *testing.T) {
411413
},
412414
}, nil)
413415

414-
mockRoute53.EXPECT().GetHostedZoneID(gomock.Any(), gomock.Eq("example.com")).Return(awssdk.String("Z0382403B3S5MSK4SVXX"), nil)
416+
mockRoute53.EXPECT().GetPublicHostedZoneID(gomock.Any(), gomock.Eq("example.com")).Return(awssdk.String("Z0382403B3S5MSK4SVXX"), nil)
415417

416418
mockRoute53.EXPECT().ChangeRecordsWithContext(gomock.Any(), gomock.Eq(&route53.ChangeResourceRecordSetsInput{
417419
HostedZoneId: awssdk.String("Z0382403B3S5MSK4SVXX"),
@@ -510,8 +512,8 @@ func Test_Synthesizer(t *testing.T) {
510512
mockACM.EXPECT().ListCertificatesAsList(gomock.Any(), gomock.Eq(&acm.ListCertificatesInput{})).
511513
Return([]acmtypes.CertificateSummary{}, nil)
512514

513-
// Pre-check: GetHostedZoneID fails — no cert should be requested
514-
mockRoute53.EXPECT().GetHostedZoneID(gomock.Any(), gomock.Eq("wrong.nonexistent-domain.com")).
515+
// Pre-check: GetPublicHostedZoneID fails — no cert should be requested
516+
mockRoute53.EXPECT().GetPublicHostedZoneID(gomock.Any(), gomock.Eq("wrong.nonexistent-domain.com")).
515517
Return(nil, fmt.Errorf("no hosted zone found for validation records"))
516518

517519
// RequestCertificate should NOT be called

0 commit comments

Comments
 (0)