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

Issue:
v0.15.1 AWS ListTagsForResource Member must have length less than or equal to 32
<!-- 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**:
After upgrading external-dns from 0.15.0 to 0.15.1 it starts failing to refresh zones list cache

error:
```
time="2025-01-17T11:20:58Z" level=debug msg="Refreshing zones list cache"
time="2025-01-17T11:20:58Z" level=error msg="Failed to do run once: soft error\nrecords retrieval failed: soft error\nfailed to list zones tags: soft error\nfailed to list tags for zone /hostedzone/Z12345678901234567890: operation error Route 53: ListTagsForResource, https response error StatusCode: 400, RequestID: a4d4c7e0-488e-4c30-8a10-c1be7107e757, InvalidInput: 1 validation error detected: Value '/hostedzone/Z12345678901234567890' at 'resourceId' failed to satisfy constraint: Member must have length less than or equal to 32"
```

**What you expected to happen**:
external-dns to keep working

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

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

**Environment**:
- External-DNS version (use `external-dns --version`): 0.15.1
- DNS provider: AWS R53
- Others: 
```
        args:
        - --source=ingress
        - --domain-filter=xxx
        - --provider=aws
        - --registry=txt
        - --aws-zone-type=private
        - --aws-zone-tags=xxx
        - --txt-owner-id=xxx
        - --log-level=debug
        - --annotation-filter=kubernetes.io/external-dns-class in (xxx, yyy)
```


Discussion / hints:
/assign

Ok. Most likely found an isssue. Something I encounter with terraform while back as well.

The change introduced in this pull request cause an issue https://github.com/kubernetes-sigs/external-dns/pull/4640 


With AWS SDK for Go v1, Route53 API URLs were sanitized, removing the leading /hostedzone/ or /change/: https://github.com/aws/aws-sdk-go/blob/ae77b91ecc806411e9c3ebbeebbea036ed908a0d/service/route53/customizations.go#L27.

With AWS SDK for Go v2 it looks like [ListTagsForResource](https://docs.aws.amazon.com/Route53/latest/APIReference/API_ListTagsForResource.html) and [ChangeTagsForResource](https://docs.aws.amazon.com/Route53/latest/APIReference/API_ChangeTagsForResource.html) don't clean the API URL (see https://github.com/aws/aws-sdk-go-v2/issues/1838).
/remove-help

Fix patch:
diff --git a/docs/tutorials/aws.md b/docs/tutorials/aws.md
index fe0b7ce974..a98ddf8b30 100644
--- a/docs/tutorials/aws.md
+++ b/docs/tutorials/aws.md
@@ -990,4 +990,96 @@ Because those limits are in place, `aws-batch-change-size` can be set to any val
 
 ## Using CRD source to manage DNS records in AWS
 
-Please refer to the [CRD source documentation](../sources/crd.md#example) for more information.
\ No newline at end of file
+Please refer to the [CRD source documentation](../sources/crd.md#example) for more information.
+
+## Strategies for Scoping Zones
+
+> Without specifying these flags, management applies to all zones.
+
+In order to manage specific zones,  you may need to combine multiple options
+
+| Argument                    | Description                                                                 | Flow Control |
+|:----------------------------|:----------------------------------------------------------------------------|:------------:|
+| `--zone-id-filter`          | Specify multiple times if needed                                            |      OR      |
+| `--domain-filter`           | By domain suffix - specify multiple times if needed                         |      OR      |
+| `--regex-domain-filter`     | By domain suffix but as a regex - overrides domain-filter                   |     AND      |
+| `--exclude-domains`         | To exclude a domain or subdomain                                            |      OR      |
+| `--regex-domain-exclusion`  | Subtracts its matches from `regex-domain-filter`'s matches                  |     AND      |
+| `--aws-zone-type`           | Only sync zones of this type `[public\|private]`                            |      OR      |
+| `--aws-zone-tags`           | Only sync zones with this tag                                               |     AND      |
+
+Minimum required configuration
+
+```sh
+args:
+    --provider=aws
+    --registry=txt
+    --source=service
+```
+
+### Filter by Zone Type
+
+> If this flag is not specified, management applies to both public and private zones.
+
+```sh
+args:
+    --aws-zone-type=private|public # choose between public or private
+    ...
+```
+
+### Filter by Domain
+
+> Specify multiple times if needed.
+
+```sh
+args:
+    --domain-filter=example.com
+    --domain-filter=.paradox.example.com
+    ...
+```
+
+Example `--domain-filter=example.com` will allow for zone `example.com` and any zones that end in `.example.com`, including `an.example.com`, i.e., the subdomains of example.com.
+
+When there are multiple domains, filter `--domain-filter=example.com` will match domains `example.com`, `ex.par.example.com`, `par.example.com`, `x.par.eu-west-1.example.com`.
+
+And if the filter is prepended with `.` e.g., `--domain-filter=.example.com` it will allow *only* zones that end in `.example.com`, i.e., the subdomains of example.com but not the `example.com` zone itself. Example result: `ex.par.eu-west-1.example.com`, `ex.par.example.com`, `par.example.com`.
+
+> Note: if you prepend the filter with ".", it will not attempt to match parent zones.
+
+### Filter by Zone ID
+
+> Specify multiple times if needed, the flow logic is OR
+
+```sh
+args:
+    --zone-id-filter=ABCDEF12345678
+    --zone-id-filter=XYZDEF12345888
+    ...
+```
+
+### Filter by Tag
+
+> Specify multiple times if needed, the flow logic is AND
+
+Keys only
+
+```sh
+args:
+    --aws-zone-tags=owner
+    --aws-zone-tags=vertical
+```
+
+Or specify keys with values
+
+```sh
+args:
+    --aws-zone-tags=owner=k8s
+    --aws-zone-tags=vertical=k8s
+```
+
+Can't specify multiple or separate values with commas: `key1=val1,key2=val2` at the moment.
+
+```sh
+args:
+    --aws-zone-tags=team=k8s,vertical=platform
+```
diff --git a/provider/aws/aws.go b/provider/aws/aws.go
index db1ce06c16..5593861d81 100644
--- a/provider/aws/aws.go
+++ b/provider/aws/aws.go
@@ -199,7 +199,7 @@ var canonicalHostedZones = map[string]string{
 }
 
 // Route53API is the subset of the AWS Route53 API that we actually use.  Add methods as required. Signatures must match exactly.
-// mostly taken from: https://github.com/kubernetes/kubernetes/blob/853167624edb6bc0cfdcdfb88e746e178f5db36c/federation/pkg/dnsprovider/providers/aws/route53/stubs/route53api.go
+// https://github.com/aws/aws-sdk-go-v2/tree/main/service/route53
 type Route53API interface {
 	ListResourceRecordSets(ctx context.Context, input *route53.ListResourceRecordSetsInput, optFns ...func(options *route53.Options)) (*route53.ListResourceRecordSetsOutput, error)
 	ChangeResourceRecordSets(ctx context.Context, input *route53.ChangeResourceRecordSetsInput, optFns ...func(options *route53.Options)) (*route53.ChangeResourceRecordSetsOutput, error)
@@ -208,7 +208,7 @@ type Route53API interface {
 	ListTagsForResource(ctx context.Context, input *route53.ListTagsForResourceInput, optFns ...func(options *route53.Options)) (*route53.ListTagsForResourceOutput, error)
 }
 
-// wrapper to handle ownership relation throughout the provider implementation
+// Route53Change wrapper to handle ownership relation throughout the provider implementation
 type Route53Change struct {
 	route53types.Change
 	OwnedRecord string
@@ -379,7 +379,7 @@ func (p *AWSProvider) zones(ctx context.Context) (map[string]*profiledZone, erro
 			}
 		}
 		if tagErr != nil {
-			return nil, provider.NewSoftError(fmt.Errorf("failed to list zones tags: %w", tagErr))
+			return nil, provider.NewSoftErrorf("failed to list zones tags: %w", tagErr)
 		}
 	}
 
@@ -427,7 +427,7 @@ func containsOctalSequence(domain string) bool {
 func (p *AWSProvider) Records(ctx context.Context) (endpoints []*endpoint.Endpoint, _ error) {
 	zones, err := p.zones(ctx)
 	if err != nil {
-		return nil, provider.NewSoftError(fmt.Errorf("records retrieval failed: %w", err))
+		return nil, provider.NewSoftErrorf("records retrieval failed: %w", err)
 	}
 
 	return p.records(ctx, zones)
@@ -447,7 +447,7 @@ func (p *AWSProvider) records(ctx context.Context, zones map[string]*profiledZon
 		for paginator.HasMorePages() {
 			resp, err := paginator.NextPage(ctx)
 			if err != nil {
-				return nil, provider.NewSoftError(fmt.Errorf("failed to list resource records sets for zone %s using aws profile %q: %w", *z.zone.Id, z.profile, err))
+				return nil, provider.NewSoftErrorf("failed to list resource records sets for zone %s using aws profile %q: %w", *z.zone.Id, z.profile, err)
 			}
 
 			for _, r := range resp.ResourceRecordSets {
@@ -608,7 +608,7 @@ func (p *AWSProvider) GetDomainFilter() endpoint.DomainFilterInterface {
 func (p *AWSProvider) ApplyChanges(ctx context.Context, changes *plan.Changes) error {
 	zones, err := p.zones(ctx)
 	if err != nil {
-		return provider.NewSoftError(fmt.Errorf("failed to list zones, not applying changes: %w", err))
+		return provider.NewSoftErrorf("failed to list zones, not applying changes: %w", err)
 	}
 
 	updateChanges := p.createUpdateChanges(changes.UpdateNew, changes.UpdateOld)
@@ -718,7 +718,7 @@ func (p *AWSProvider) submitChanges(ctx context.Context, changes Route53Changes,
 	}
 
 	if len(failedZones) > 0 {
-		return provider.NewSoftError(fmt.Errorf("failed to submit all changes for the following zones: %v", failedZones))
+		return provider.NewSoftErrorf("failed to submit all changes for the following zones: %v", failedZones)
 	}
 
 	return nil
@@ -943,12 +943,14 @@ func groupChangesByNameAndOwnershipRelation(cs Route53Changes) map[string]Route5
 func (p *AWSProvider) tagsForZone(ctx context.Context, zoneID string, profile string) (map[string]string, error) {
 	client := p.clients[profile]
 
+	// TODO: this will make single API request for each zone, which consumes API requests
+	// more effective way is to batch requests https://github.com/aws/aws-sdk-go-v2/blob/ed8a3caa0df9ce36a5b60aebeee201187098d205/service/route53/api_op_ListTagsForResources.go#L37
 	response, err := client.ListTagsForResource(ctx, &route53.ListTagsForResourceInput{
 		ResourceType: route53types.TagResourceTypeHostedzone,
-		ResourceId:   aws.String(zoneID),
+		ResourceId:   aws.String(cleanZoneID(zoneID)),
 	})
 	if err != nil {
-		return nil, provider.NewSoftError(fmt.Errorf("failed to list tags for zone %s: %w", zoneID, err))
+		return nil, provider.NewSoftErrorf("failed to list tags for zone %s: %w", zoneID, err)
 	}
 	tagMap := map[string]string{}
 	for _, tag := range response.ResourceTagSet.Tags {
diff --git a/provider/provider.go b/provider/provider.go
index 2bbfac1619..317a643bad 100644
--- a/provider/provider.go
+++ b/provider/provider.go
@@ -19,6 +19,7 @@ package provider
 import (
 	"context"
 	"errors"
+	"fmt"
 	"net"
 	"strings"
 
@@ -31,6 +32,11 @@ import (
 // that this is a transient error.
 var SoftError error = errors.New("soft error")
 
+// NewSoftErrorf creates a SoftError with formats according to a format specifier and returns the string as a
+func NewSoftErrorf(format string, a ...any) error {
+	return NewSoftError(fmt.Errorf(format, a...))
+}
+
 // NewSoftError creates a SoftError from the given error
 func NewSoftError(err error) error {
 	return errors.Join(SoftError, err)


Test patch:
diff --git a/provider/aws/aws_test.go b/provider/aws/aws_test.go
index 12d9f6940d..ef45bf9a4b 100644
--- a/provider/aws/aws_test.go
+++ b/provider/aws/aws_test.go
@@ -161,7 +161,11 @@ func specialCharactersEscape(s string) string {
 
 func (r *Route53APIStub) ListTagsForResource(ctx context.Context, input *route53.ListTagsForResourceInput, optFns ...func(options *route53.Options)) (*route53.ListTagsForResourceOutput, error) {
 	if input.ResourceType == route53types.TagResourceTypeHostedzone {
-		tags := r.zoneTags[*input.ResourceId]
+		zoneId := fmt.Sprintf("/%s/%s", input.ResourceType, *input.ResourceId)
+		if strings.Contains(zoneId, "ext-dns-test-error-on-list-tags") {
+			return nil, fmt.Errorf("operation error Route53APIStub: ListTagsForResource")
+		}
+		tags := r.zoneTags[zoneId]
 		return &route53.ListTagsForResourceOutput{
 			ResourceTagSet: &route53types.ResourceTagSet{
 				ResourceId:   input.ResourceId,
@@ -180,7 +184,7 @@ func (r *Route53APIStub) ChangeResourceRecordSets(ctx context.Context, input *ro
 
 	_, ok := r.zones[*input.HostedZoneId]
 	if !ok {
-		return nil, fmt.Errorf("Hosted zone doesn't exist: %s", *input.HostedZoneId)
+		return nil, fmt.Errorf("hosted zone doesn't exist: %s", *input.HostedZoneId)
 	}
 
 	if len(input.ChangeBatch.Changes) == 0 {
@@ -216,12 +220,12 @@ func (r *Route53APIStub) ChangeResourceRecordSets(ctx context.Context, input *ro
 		switch change.Action {
 		case route53types.ChangeActionCreate:
 			if _, found := recordSets[key]; found {
-				return nil, fmt.Errorf("Attempt to create duplicate rrset %s", key) // TODO: Return AWS errors with codes etc
+				return nil, fmt.Errorf("attempt to create duplicate rrset %s", key) // TODO: Return AWS errors with codes etc
 			}
 			recordSets[key] = append(recordSets[key], *change.ResourceRecordSet)
 		case route53types.ChangeActionDelete:
 			if _, found := recordSets[key]; !found {
-				return nil, fmt.Errorf("Attempt to delete non-existent rrset %s", key) // TODO: Check other fields too
+				return nil, fmt.Errorf("attempt to delete non-existent rrset %s", key) // TODO: Check other fields too
 			}
 			delete(recordSets, key)
 		case route53types.ChangeActionUpsert:
@@ -331,15 +335,36 @@ func TestAWSZones(t *testing.T) {
 	} {
 		t.Run(ti.msg, func(t *testing.T) {
 			provider, _ := newAWSProviderWithTagFilter(t, endpoint.NewDomainFilter([]string{"ext-dns-test-2.teapot.zalan.do."}), ti.zoneIDFilter, ti.zoneTypeFilter, ti.zoneTagFilter, defaultEvaluateTargetHealth, false, nil)
-
 			zones, err := provider.Zones(context.Background())
 			require.NoError(t, err)
-
 			validateAWSZones(t, zones, ti.expectedZones)
 		})
 	}
 }
 
+func TestAWSZonesWithTagFilterError(t *testing.T) {
+	client := NewRoute53APIStub(t)
+	provider := &AWSProvider{
+		clients:       map[string]Route53API{defaultAWSProfile: client},
+		zoneTagFilter: provider.NewZoneTagFilter([]string{"zone=2"}),
+		dryRun:        false,
+		zonesCache:    &zonesListCache{duration: 1 * time.Minute},
+	}
+	createAWSZone(t, provider, &route53types.HostedZone{
+		Id:     aws.String("/hostedzone/zone-1.ext-dns-test-ok.example.com."),
+		Name:   aws.String("zone-1.ext-dns-test-ok.example.com."),
+		Config: &route53types.HostedZoneConfig{PrivateZone: false},
+	})
+	createAWSZone(t, provider, &route53types.HostedZone{
+		Id:     aws.String("/hostedzone/zone-2.ext-dns-test-error-on-list-tags.example.com."),
+		Name:   aws.String("zone-2.ext-dns-test-error-on-list-tags.example.com."),
+		Config: &route53types.HostedZoneConfig{PrivateZone: false},
+	})
+	_, err := provider.Zones(context.Background())
+	require.Error(t, err)
+	require.ErrorContains(t, err, "failed to list tags for zone /hostedzone/zone-2.ext-dns-test-error-on-list-tags")
+}
+
 func TestAWSRecordsFilter(t *testing.T) {
 	provider, _ := newAWSProvider(t, endpoint.DomainFilter{}, provider.ZoneIDFilter{}, provider.ZoneTypeFilter{}, false, false, nil)
 	domainFilter := provider.GetDomainFilter()