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 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.
Validation options whose resource record has not yet been populated by ACM
are skipped during cleanup.

Signed-off-by: niv1612 <35202955+niv1612@users.noreply.github.com>

Author: niv1612

docs/guide/ingress/certificate_management.md

@@ -38,6 +38,8 @@ Amazon Issued certificates are currently validated using DNS Method and Route53
 E-Mail validation is not supported due to significant higher delays between requesting a certificate and it's issuance. 
 When using a PCA, certificates don't have to be validated.
 
+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.
+
 ## Ingress Group Behavior
 
 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

@@ -22,6 +22,7 @@ const (
 type Route53 interface {
 	ChangeRecordsWithContext(ctx context.Context, input *route53.ChangeResourceRecordSetsInput) (*route53.ChangeResourceRecordSetsOutput, error)
 	GetHostedZoneID(ctx context.Context, domain string) (*string, error)
+	GetPublicHostedZoneID(ctx context.Context, domain string) (*string, error)
 }
 
 func NewRoute53(awsClientsProvider provider.AWSClientsProvider) Route53 {
@@ -57,11 +58,39 @@ func (c *route53Client) GetHostedZoneID(ctx context.Context, domain string) (*st
 		return nil, err
 	}
 
+	if bestID := findHostedZoneID(zones, domain, false); bestID != nil {
+		return bestID, nil
+	}
+
+	return nil, fmt.Errorf("no hosted zone found for validation records")
+}
+
+// GetPublicHostedZoneID skips private zones: Amazon-issued ACM certificates are
+// validated over public DNS, so a validation record in a private zone (e.g. the
+// most-specific match in split-horizon Route 53) leaves the cert in PENDING_VALIDATION.
+func (c *route53Client) GetPublicHostedZoneID(ctx context.Context, domain string) (*string, error) {
+	zones, err := c.listHostedZones(ctx)
+	if err != nil {
+		return nil, err
+	}
+
+	if bestID := findHostedZoneID(zones, domain, true); bestID != nil {
+		return bestID, nil
+	}
+
+	return nil, fmt.Errorf("no public Route 53 hosted zone found for %q", domain)
+}
+
+// findHostedZoneID returns the nearest-ancestor hosted zone (longest matching suffix).
+func findHostedZoneID(zones []types.HostedZone, domain string, publicOnly bool) *string {
 	recParts := strings.Split(domain, ".")
 
 	var bestID *string
 	bestLen := -1
 	for _, zone := range zones {
+		if publicOnly && zone.Config != nil && zone.Config.PrivateZone {
+			continue
+		}
 		zoneParts := strings.Split(strings.TrimRight(*zone.Name, "."), ".")
 		if len(zoneParts) > len(recParts) {
 			continue
@@ -72,11 +101,7 @@ func (c *route53Client) GetHostedZoneID(ctx context.Context, domain string) (*st
 		}
 	}
 
-	if bestID != nil {
-		return bestID, nil
-	}
-
-	return nil, fmt.Errorf("no hosted zone found for validation records")
+	return bestID
 }
 
 func (c *route53Client) listHostedZones(ctx context.Context) ([]types.HostedZone, error) {

pkg/aws/services/route53_mocks.go

@@ -24,6 +24,14 @@ func hostedZone(id, name string) types.HostedZone {
 	return types.HostedZone{Id: awssdk.String(id), Name: awssdk.String(name)}
 }
 
+func privateHostedZone(id, name string) types.HostedZone {
+	return types.HostedZone{
+		Id:     awssdk.String(id),
+		Name:   awssdk.String(name),
+		Config: &types.HostedZoneConfig{PrivateZone: true},
+	}
+}
+
 func TestGetHostedZoneID(t *testing.T) {
 	tests := []struct {
 		name   string
@@ -63,3 +71,65 @@ func TestGetHostedZoneID(t *testing.T) {
 		})
 	}
 }
+
+func TestGetPublicHostedZoneID(t *testing.T) {
+	tests := []struct {
+		name    string
+		domain  string
+		zones   []types.HostedZone
+		want    string
+		wantErr bool
+	}{
+		{
+			name:   "split-horizon: public chosen even though private zone is more specific",
+			domain: "app.sub.example.com",
+			zones: []types.HostedZone{
+				hostedZone("Z_PUBLIC", "example.com."),
+				privateHostedZone("Z_PRIVATE", "sub.example.com."),
+			},
+			want: "Z_PUBLIC",
+		},
+		{
+			name:   "only a private zone matches: fail fast",
+			domain: "app.sub.example.com",
+			zones: []types.HostedZone{
+				privateHostedZone("Z_PRIVATE", "sub.example.com."),
+			},
+			wantErr: true,
+		},
+		{
+			name:   "multiple public zones match: longest suffix wins",
+			domain: "*.app.sub.example.com",
+			zones: []types.HostedZone{
+				hostedZone("Z_PARENT", "example.com."),
+				hostedZone("Z_SUB", "sub.example.com."),
+			},
+			want: "Z_SUB",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			c := newCachedRoute53Client(tt.zones)
+			got, err := c.GetPublicHostedZoneID(context.Background(), tt.domain)
+			if tt.wantErr {
+				assert.Error(t, err)
+				return
+			}
+			assert.NoError(t, err)
+			assert.Equal(t, tt.want, awssdk.ToString(got))
+		})
+	}
+}
+
+// The unfiltered lookup (delete path) must still resolve private zones so legacy
+// records written there can be cleaned up.
+func TestGetHostedZoneID_UnfilteredReturnsPrivate(t *testing.T) {
+	c := newCachedRoute53Client([]types.HostedZone{
+		hostedZone("Z_PUBLIC", "example.com."),
+		privateHostedZone("Z_PRIVATE", "sub.example.com."),
+	})
+	got, err := c.GetHostedZoneID(context.Background(), "app.sub.example.com")
+	assert.NoError(t, err)
+	assert.Equal(t, "Z_PRIVATE", awssdk.ToString(got))
+}

pkg/aws/services/route53_test.go

@@ -83,7 +83,7 @@ func (c *defaultCertificateManager) CreateWithValidationRecords(ctx context.Cont
 		if _, checked := hostedZoneByDomain[host]; checked {
 			continue
 		}
-		zoneID, err := c.route53Client.GetHostedZoneID(ctx, host)
+		zoneID, err := c.route53Client.GetPublicHostedZoneID(ctx, host)
 		if err != nil {
 			return nil, fmt.Errorf("pre-check failed for domain %q: %w", host, err)
 		}
@@ -226,6 +226,12 @@ func (c *defaultCertificateManager) DeleteWithValidationRecords(ctx context.Cont
 
 	for _, opts := range desc.Certificate.DomainValidationOptions {
 		if opts.ValidationMethod == acmtypes.ValidationMethodDns {
+			if opts.ResourceRecord == nil {
+				// ACM populates ResourceRecord asynchronously; a certificate deleted right
+				// after creation may not have one yet, so there is no record to clean up.
+				c.logger.Info("no resource record on validation option, skipping validation record cleanup", "domain", awssdk.ToString(opts.DomainName))
+				continue
+			}
 			c.logger.Info("deleting validation records for certificate", "certificateARN", arn)
 			id, err := c.route53Client.GetHostedZoneID(ctx, awssdk.ToString(opts.DomainName))
 			if err != nil {
@@ -237,37 +243,49 @@ func (c *defaultCertificateManager) DeleteWithValidationRecords(ctx context.Cont
 				}
 				return err
 			}
-			input := &route53sdk.ChangeResourceRecordSetsInput{
-				HostedZoneId: id,
-				ChangeBatch: &route53types.ChangeBatch{
-					Changes: []route53types.Change{
-						{
-							Action: "DELETE",
-							ResourceRecordSet: &route53types.ResourceRecordSet{
-								Name: opts.ResourceRecord.Name,
-								Type: route53types.RRType(opts.ResourceRecord.Type),
-								TTL:  awssdk.Int64(validationRecordTTL),
-								ResourceRecords: []route53types.ResourceRecord{
-									{
-										Value: opts.ResourceRecord.Value,
+			// The validation record lives in the nearest public zone (written by current
+			// controllers) or, for certificates created by controller versions that
+			// predate the public-zone selection fix (issue #4840), in the most-specific
+			// zone regardless of visibility. Attempt cleanup in both when they differ;
+			// a delete against the wrong zone fails with "not found", which is
+			// tolerated below.
+			zoneIDs := []*string{id}
+			if publicID, err := c.route53Client.GetPublicHostedZoneID(ctx, awssdk.ToString(opts.DomainName)); err == nil && awssdk.ToString(publicID) != awssdk.ToString(id) {
+				zoneIDs = append(zoneIDs, publicID)
+			}
+			for _, zoneID := range zoneIDs {
+				input := &route53sdk.ChangeResourceRecordSetsInput{
+					HostedZoneId: zoneID,
+					ChangeBatch: &route53types.ChangeBatch{
+						Changes: []route53types.Change{
+							{
+								Action: "DELETE",
+								ResourceRecordSet: &route53types.ResourceRecordSet{
+									Name: opts.ResourceRecord.Name,
+									Type: route53types.RRType(opts.ResourceRecord.Type),
+									TTL:  awssdk.Int64(validationRecordTTL),
+									ResourceRecords: []route53types.ResourceRecord{
+										{
+											Value: opts.ResourceRecord.Value,
+										},
 									},
 								},
 							},
 						},
 					},
-				},
-			}
-			_, err = c.route53Client.ChangeRecordsWithContext(ctx, input)
-			if err != nil && strings.Contains(err.Error(), "not found") {
-				c.logger.Info("validation records no longer found, ignoring", "name", opts.ResourceRecord.Name, "value", opts.ResourceRecord.Value, "type", opts.ResourceRecord.Type)
-				continue
-			}
-			if err != nil && strings.Contains(err.Error(), "do not match the current values") {
-				c.logger.Info("validation records have been reused for another certificate, ignoring", "name", opts.ResourceRecord.Name, "value", opts.ResourceRecord.Value, "type", opts.ResourceRecord.Type)
-				continue
-			}
-			if err != nil {
-				return err
+				}
+				_, err = c.route53Client.ChangeRecordsWithContext(ctx, input)
+				if err != nil && strings.Contains(err.Error(), "not found") {
+					c.logger.Info("validation records no longer found, ignoring", "name", opts.ResourceRecord.Name, "value", opts.ResourceRecord.Value, "type", opts.ResourceRecord.Type)
+					continue
+				}
+				if err != nil && strings.Contains(err.Error(), "do not match the current values") {
+					c.logger.Info("validation records have been reused for another certificate, ignoring", "name", opts.ResourceRecord.Name, "value", opts.ResourceRecord.Value, "type", opts.ResourceRecord.Type)
+					continue
+				}
+				if err != nil {
+					return err
+				}
 			}
 		}
 	}

pkg/deploy/acm/certificate_manager.go

@@ -0,0 +1,126 @@
+package acm
+
+import (
+	"context"
+	"errors"
+	"testing"
+
+	awssdk "github.com/aws/aws-sdk-go-v2/aws"
+	"github.com/aws/aws-sdk-go-v2/service/acm"
+	acmtypes "github.com/aws/aws-sdk-go-v2/service/acm/types"
+	"github.com/aws/aws-sdk-go-v2/service/route53"
+	route53types "github.com/aws/aws-sdk-go-v2/service/route53/types"
+	"github.com/go-logr/logr"
+	"github.com/golang/mock/gomock"
+	"github.com/stretchr/testify/assert"
+	"sigs.k8s.io/aws-load-balancer-controller/pkg/aws/services"
+	"sigs.k8s.io/controller-runtime/pkg/log"
+)
+
+// Split-horizon cleanup: the unfiltered lookup resolves the private zone (legacy
+// records) while the public lookup resolves the public zone (records written by
+// current controllers). Delete must attempt cleanup in both zones.
+func TestDeleteWithValidationRecords_SplitHorizonCleansBothZones(t *testing.T) {
+	ctrl := gomock.NewController(t)
+	defer ctrl.Finish()
+
+	mockACM := services.NewMockACM(ctrl)
+	mockRoute53 := services.NewMockRoute53(ctrl)
+	m := &defaultCertificateManager{
+		acmClient:     mockACM,
+		route53Client: mockRoute53,
+		logger:        logr.New(&log.NullLogSink{}),
+	}
+
+	arn := "arn:aws:acm:us-east-1:123456789012:certificate/test"
+	mockACM.EXPECT().DescribeCertificateWithContext(gomock.Any(), gomock.Eq(&acm.DescribeCertificateInput{
+		CertificateArn: awssdk.String(arn),
+	})).Return(&acm.DescribeCertificateOutput{
+		Certificate: &acmtypes.CertificateDetail{
+			DomainValidationOptions: []acmtypes.DomainValidation{
+				{
+					ValidationMethod: acmtypes.ValidationMethodDns,
+					DomainName:       awssdk.String("app.sub.example.com"),
+					ResourceRecord: &acmtypes.ResourceRecord{
+						Name:  awssdk.String("cname-name"),
+						Value: awssdk.String("cname-value"),
+						Type:  acmtypes.RecordTypeCname,
+					},
+				},
+			},
+		},
+	}, nil)
+
+	mockRoute53.EXPECT().GetHostedZoneID(gomock.Any(), gomock.Eq("app.sub.example.com")).Return(awssdk.String("Z_PRIVATE"), nil)
+	mockRoute53.EXPECT().GetPublicHostedZoneID(gomock.Any(), gomock.Eq("app.sub.example.com")).Return(awssdk.String("Z_PUBLIC"), nil)
+
+	deleteInput := func(zoneID string) *route53.ChangeResourceRecordSetsInput {
+		return &route53.ChangeResourceRecordSetsInput{
+			HostedZoneId: awssdk.String(zoneID),
+			ChangeBatch: &route53types.ChangeBatch{
+				Changes: []route53types.Change{
+					{
+						Action: "DELETE",
+						ResourceRecordSet: &route53types.ResourceRecordSet{
+							Name: awssdk.String("cname-name"),
+							Type: route53types.RRType(acmtypes.RecordTypeCname),
+							TTL:  awssdk.Int64(validationRecordTTL),
+							ResourceRecords: []route53types.ResourceRecord{
+								{Value: awssdk.String("cname-value")},
+							},
+						},
+					},
+				},
+			},
+		}
+	}
+	// record was written to the public zone: private-zone delete fails "not found" (tolerated)
+	mockRoute53.EXPECT().ChangeRecordsWithContext(gomock.Any(), gomock.Eq(deleteInput("Z_PRIVATE"))).
+		Return(nil, errors.New("InvalidChangeBatch: Tried to delete resource record set but it was not found"))
+	mockRoute53.EXPECT().ChangeRecordsWithContext(gomock.Any(), gomock.Eq(deleteInput("Z_PUBLIC"))).
+		Return(&route53.ChangeResourceRecordSetsOutput{}, nil)
+
+	mockACM.EXPECT().DeleteCertificateWithContext(gomock.Any(), gomock.Eq(&acm.DeleteCertificateInput{
+		CertificateArn: awssdk.String(arn),
+	})).Return(&acm.DeleteCertificateOutput{}, nil)
+
+	err := m.DeleteWithValidationRecords(context.Background(), arn)
+	assert.NoError(t, err)
+}
+
+// A DNS validation option may have no ResourceRecord yet (ACM populates it
+// asynchronously); delete must skip record cleanup instead of panicking.
+func TestDeleteWithValidationRecords_NilResourceRecordSkipsCleanup(t *testing.T) {
+	ctrl := gomock.NewController(t)
+	defer ctrl.Finish()
+
+	mockACM := services.NewMockACM(ctrl)
+	mockRoute53 := services.NewMockRoute53(ctrl)
+	m := &defaultCertificateManager{
+		acmClient:     mockACM,
+		route53Client: mockRoute53,
+		logger:        logr.New(&log.NullLogSink{}),
+	}
+
+	arn := "arn:aws:acm:us-east-1:123456789012:certificate/test"
+	mockACM.EXPECT().DescribeCertificateWithContext(gomock.Any(), gomock.Eq(&acm.DescribeCertificateInput{
+		CertificateArn: awssdk.String(arn),
+	})).Return(&acm.DescribeCertificateOutput{
+		Certificate: &acmtypes.CertificateDetail{
+			DomainValidationOptions: []acmtypes.DomainValidation{
+				{
+					ValidationMethod: acmtypes.ValidationMethodDns,
+					DomainName:       awssdk.String("app.sub.example.com"),
+					ResourceRecord:   nil,
+				},
+			},
+		},
+	}, nil)
+
+	mockACM.EXPECT().DeleteCertificateWithContext(gomock.Any(), gomock.Eq(&acm.DeleteCertificateInput{
+		CertificateArn: awssdk.String(arn),
+	})).Return(&acm.DeleteCertificateOutput{}, nil)
+
+	err := m.DeleteWithValidationRecords(context.Background(), arn)
+	assert.NoError(t, err)
+}