From c391f5588ac4deea4f9fac5b0a72c205938fe3a4 Mon Sep 17 00:00:00 2001 From: Megum1n Date: Tue, 19 Dec 2023 10:21:47 +0100 Subject: [PATCH 1/8] Validate AWS record values size during batch set generation --- main.go | 22 +-- pkg/apis/externaldns/types.go | 6 + pkg/apis/externaldns/types_test.go | 8 + provider/aws/aws.go | 105 ++++++++---- provider/aws/aws_test.go | 255 +++++++++++++++++++++++++++-- 5 files changed, 338 insertions(+), 58 deletions(-) diff --git a/main.go b/main.go index 921a1e9591..e68dad8433 100644 --- a/main.go +++ b/main.go @@ -227,16 +227,18 @@ func main() { case "aws": p, err = aws.NewAWSProvider( aws.AWSConfig{ - DomainFilter: domainFilter, - ZoneIDFilter: zoneIDFilter, - ZoneTypeFilter: zoneTypeFilter, - ZoneTagFilter: zoneTagFilter, - BatchChangeSize: cfg.AWSBatchChangeSize, - BatchChangeInterval: cfg.AWSBatchChangeInterval, - EvaluateTargetHealth: cfg.AWSEvaluateTargetHealth, - PreferCNAME: cfg.AWSPreferCNAME, - DryRun: cfg.DryRun, - ZoneCacheDuration: cfg.AWSZoneCacheDuration, + DomainFilter: domainFilter, + ZoneIDFilter: zoneIDFilter, + ZoneTypeFilter: zoneTypeFilter, + ZoneTagFilter: zoneTagFilter, + BatchChangeSize: cfg.AWSBatchChangeSize, + BatchChangeSizeBytes: cfg.AWSBatchChangeSizeBytes, + BatchChangeSizeValues: cfg.AWSBatchChangeSizeValues, + BatchChangeInterval: cfg.AWSBatchChangeInterval, + EvaluateTargetHealth: cfg.AWSEvaluateTargetHealth, + PreferCNAME: cfg.AWSPreferCNAME, + DryRun: cfg.DryRun, + ZoneCacheDuration: cfg.AWSZoneCacheDuration, }, route53.New(awsSession), ) diff --git a/pkg/apis/externaldns/types.go b/pkg/apis/externaldns/types.go index 0ae71197d9..77449bb0b7 100644 --- a/pkg/apis/externaldns/types.go +++ b/pkg/apis/externaldns/types.go @@ -86,6 +86,8 @@ type Config struct { AWSAssumeRole string AWSAssumeRoleExternalID string AWSBatchChangeSize int + AWSBatchChangeSizeBytes int + AWSBatchChangeSizeValues int AWSBatchChangeInterval time.Duration AWSEvaluateTargetHealth bool AWSAPIRetries int @@ -258,6 +260,8 @@ var defaultConfig = &Config{ AWSAssumeRole: "", AWSAssumeRoleExternalID: "", AWSBatchChangeSize: 1000, + AWSBatchChangeSizeBytes: 32000, + AWSBatchChangeSizeValues: 1000, AWSBatchChangeInterval: time.Second, AWSEvaluateTargetHealth: true, AWSAPIRetries: 3, @@ -477,6 +481,8 @@ func (cfg *Config) ParseFlags(args []string) error { app.Flag("aws-assume-role", "When using the AWS API, assume this IAM role. Useful for hosted zones in another AWS account. Specify the full ARN, e.g. `arn:aws:iam::123455567:role/external-dns` (optional)").Default(defaultConfig.AWSAssumeRole).StringVar(&cfg.AWSAssumeRole) app.Flag("aws-assume-role-external-id", "When using the AWS API and assuming a role then specify this external ID` (optional)").Default(defaultConfig.AWSAssumeRoleExternalID).StringVar(&cfg.AWSAssumeRoleExternalID) app.Flag("aws-batch-change-size", "When using the AWS provider, set the maximum number of changes that will be applied in each batch.").Default(strconv.Itoa(defaultConfig.AWSBatchChangeSize)).IntVar(&cfg.AWSBatchChangeSize) + app.Flag("aws-batch-change-size-bytes", "When using the AWS provider, set the maximum byte size that will be applied in each batch.").Default(strconv.Itoa(defaultConfig.AWSBatchChangeSizeBytes)).IntVar(&cfg.AWSBatchChangeSizeBytes) + app.Flag("aws-batch-change-size-values", "When using the AWS provider, set the maximum total record values that will be applied in each batch.").Default(strconv.Itoa(defaultConfig.AWSBatchChangeSizeValues)).IntVar(&cfg.AWSBatchChangeSizeValues) app.Flag("aws-batch-change-interval", "When using the AWS provider, set the interval between batch changes.").Default(defaultConfig.AWSBatchChangeInterval.String()).DurationVar(&cfg.AWSBatchChangeInterval) app.Flag("aws-evaluate-target-health", "When using the AWS provider, set whether to evaluate the health of a DNS target (default: enabled, disable with --no-aws-evaluate-target-health)").Default(strconv.FormatBool(defaultConfig.AWSEvaluateTargetHealth)).BoolVar(&cfg.AWSEvaluateTargetHealth) app.Flag("aws-api-retries", "When using the AWS API, set the maximum number of retries before giving up.").Default(strconv.Itoa(defaultConfig.AWSAPIRetries)).IntVar(&cfg.AWSAPIRetries) diff --git a/pkg/apis/externaldns/types_test.go b/pkg/apis/externaldns/types_test.go index a8093fe1eb..a8e6154219 100644 --- a/pkg/apis/externaldns/types_test.go +++ b/pkg/apis/externaldns/types_test.go @@ -58,6 +58,8 @@ var ( AWSAssumeRole: "", AWSAssumeRoleExternalID: "", AWSBatchChangeSize: 1000, + AWSBatchChangeSizeBytes: 32000, + AWSBatchChangeSizeValues: 1000, AWSBatchChangeInterval: time.Second, AWSEvaluateTargetHealth: true, AWSAPIRetries: 3, @@ -169,6 +171,8 @@ var ( AWSAssumeRole: "some-other-role", AWSAssumeRoleExternalID: "pg2000", AWSBatchChangeSize: 100, + AWSBatchChangeSizeBytes: 16000, + AWSBatchChangeSizeValues: 100, AWSBatchChangeInterval: time.Second * 2, AWSEvaluateTargetHealth: false, AWSAPIRetries: 13, @@ -354,6 +358,8 @@ func TestParseFlags(t *testing.T) { "--aws-assume-role=some-other-role", "--aws-assume-role-external-id=pg2000", "--aws-batch-change-size=100", + "--aws-batch-change-size-bytes=16000", + "--aws-batch-change-size-values=100", "--aws-batch-change-interval=2s", "--aws-api-retries=13", "--aws-prefer-cname", @@ -476,6 +482,8 @@ func TestParseFlags(t *testing.T) { "EXTERNAL_DNS_AWS_ASSUME_ROLE": "some-other-role", "EXTERNAL_DNS_AWS_ASSUME_ROLE_EXTERNAL_ID": "pg2000", "EXTERNAL_DNS_AWS_BATCH_CHANGE_SIZE": "100", + "EXTERNAL_DNS_AWS_BATCH_CHANGE_SIZE_BYTES": "16000", + "EXTERNAL_DNS_AWS_BATCH_CHANGE_SIZE_VALUES": "100", "EXTERNAL_DNS_AWS_BATCH_CHANGE_INTERVAL": "2s", "EXTERNAL_DNS_AWS_EVALUATE_TARGET_HEALTH": "0", "EXTERNAL_DNS_AWS_API_RETRIES": "13", diff --git a/provider/aws/aws.go b/provider/aws/aws.go index fd16a6d1ac..cbd35f319f 100644 --- a/provider/aws/aws.go +++ b/provider/aws/aws.go @@ -206,6 +206,8 @@ type Route53API interface { type Route53Change struct { route53.Change OwnedRecord string + sizeBytes int + sizeValues int } type Route53Changes []*Route53Change @@ -227,11 +229,13 @@ type zonesListCache struct { // AWSProvider is an implementation of Provider for AWS Route53. type AWSProvider struct { provider.BaseProvider - client Route53API - dryRun bool - batchChangeSize int - batchChangeInterval time.Duration - evaluateTargetHealth bool + client Route53API + dryRun bool + batchChangeSize int + batchChangeSizeBytes int + batchChangeSizeValues int + batchChangeInterval time.Duration + evaluateTargetHealth bool // only consider hosted zones managing domains ending in this suffix domainFilter endpoint.DomainFilter // filter hosted zones by id @@ -248,33 +252,37 @@ type AWSProvider struct { // AWSConfig contains configuration to create a new AWS provider. type AWSConfig struct { - DomainFilter endpoint.DomainFilter - ZoneIDFilter provider.ZoneIDFilter - ZoneTypeFilter provider.ZoneTypeFilter - ZoneTagFilter provider.ZoneTagFilter - BatchChangeSize int - BatchChangeInterval time.Duration - EvaluateTargetHealth bool - PreferCNAME bool - DryRun bool - ZoneCacheDuration time.Duration + DomainFilter endpoint.DomainFilter + ZoneIDFilter provider.ZoneIDFilter + ZoneTypeFilter provider.ZoneTypeFilter + ZoneTagFilter provider.ZoneTagFilter + BatchChangeSize int + BatchChangeSizeBytes int + BatchChangeSizeValues int + BatchChangeInterval time.Duration + EvaluateTargetHealth bool + PreferCNAME bool + DryRun bool + ZoneCacheDuration time.Duration } // NewAWSProvider initializes a new AWS Route53 based Provider. func NewAWSProvider(awsConfig AWSConfig, client Route53API) (*AWSProvider, error) { provider := &AWSProvider{ - client: client, - domainFilter: awsConfig.DomainFilter, - zoneIDFilter: awsConfig.ZoneIDFilter, - zoneTypeFilter: awsConfig.ZoneTypeFilter, - zoneTagFilter: awsConfig.ZoneTagFilter, - batchChangeSize: awsConfig.BatchChangeSize, - batchChangeInterval: awsConfig.BatchChangeInterval, - evaluateTargetHealth: awsConfig.EvaluateTargetHealth, - preferCNAME: awsConfig.PreferCNAME, - dryRun: awsConfig.DryRun, - zonesCache: &zonesListCache{duration: awsConfig.ZoneCacheDuration}, - failedChangesQueue: make(map[string]Route53Changes), + client: client, + domainFilter: awsConfig.DomainFilter, + zoneIDFilter: awsConfig.ZoneIDFilter, + zoneTypeFilter: awsConfig.ZoneTypeFilter, + zoneTagFilter: awsConfig.ZoneTagFilter, + batchChangeSize: awsConfig.BatchChangeSize, + batchChangeSizeBytes: awsConfig.BatchChangeSizeBytes, + batchChangeSizeValues: awsConfig.BatchChangeSizeValues, + batchChangeInterval: awsConfig.BatchChangeInterval, + evaluateTargetHealth: awsConfig.EvaluateTargetHealth, + preferCNAME: awsConfig.PreferCNAME, + dryRun: awsConfig.DryRun, + zonesCache: &zonesListCache{duration: awsConfig.ZoneCacheDuration}, + failedChangesQueue: make(map[string]Route53Changes), } return provider, nil @@ -565,7 +573,8 @@ func (p *AWSProvider) submitChanges(ctx context.Context, changes Route53Changes, retriedChanges, newChanges := findChangesInQueue(cs, p.failedChangesQueue[z]) p.failedChangesQueue[z] = nil - batchCs := append(batchChangeSet(newChanges, p.batchChangeSize), batchChangeSet(retriedChanges, p.batchChangeSize)...) + batchCs := append(batchChangeSet(newChanges, p.batchChangeSize, p.batchChangeSizeBytes, p.batchChangeSizeValues), + batchChangeSet(retriedChanges, p.batchChangeSize, p.batchChangeSizeBytes, p.batchChangeSizeValues)...) for i, b := range batchCs { if len(b) == 0 { continue @@ -719,6 +728,8 @@ func (p *AWSProvider) newChange(action string, ep *endpoint.Endpoint) (*Route53C Name: aws.String(ep.DNSName), }, }, + sizeBytes: 0, + sizeValues: 0, } dualstack := false if targetHostedZone := isAWSAlias(ep); targetHostedZone != "" { @@ -736,6 +747,8 @@ func (p *AWSProvider) newChange(action string, ep *endpoint.Endpoint) (*Route53C HostedZoneId: aws.String(cleanZoneID(targetHostedZone)), EvaluateTargetHealth: aws.Bool(evalTargetHealth), } + change.sizeBytes += len([]byte(ep.Targets[0])) + change.sizeValues += 1 } else { change.ResourceRecordSet.Type = aws.String(ep.RecordType) if !ep.RecordTTL.IsConfigured() { @@ -748,9 +761,18 @@ func (p *AWSProvider) newChange(action string, ep *endpoint.Endpoint) (*Route53C change.ResourceRecordSet.ResourceRecords[idx] = &route53.ResourceRecord{ Value: aws.String(val), } + change.sizeBytes += len([]byte(val)) + change.sizeValues += 1 } } + if action == route53.ChangeActionUpsert { + // When the value of the Action element is UPSERT, each ResourceRecord element and each character in a Value + // element is counted twice + change.sizeBytes *= 2 + change.sizeValues *= 2 + } + setIdentifier := ep.SetIdentifier if setIdentifier != "" { change.ResourceRecordSet.SetIdentifier = aws.String(setIdentifier) @@ -856,8 +878,26 @@ func (p *AWSProvider) tagsForZone(ctx context.Context, zoneID string) (map[strin return tagMap, nil } -func batchChangeSet(cs Route53Changes, batchSize int) []Route53Changes { - if len(cs) <= batchSize { +// count bytes for all changes values +func countChangeBytes(cs Route53Changes) int { + count := 0 + for _, c := range cs { + count += c.sizeBytes + } + return count +} + +// count total value count for all changes +func countChangeValues(cs Route53Changes) int { + count := 0 + for _, c := range cs { + count += c.sizeValues + } + return count +} + +func batchChangeSet(cs Route53Changes, batchSize int, batchSizeBytes int, batchSizeValues int) []Route53Changes { + if len(cs) <= batchSize && countChangeBytes(cs) <= batchSizeBytes && countChangeValues(cs) <= batchSizeValues { res := sortChangesByActionNameType(cs) return []Route53Changes{res} } @@ -880,7 +920,10 @@ func batchChangeSet(cs Route53Changes, batchSize int) []Route53Changes { continue } - if len(currentBatch)+len(v) > batchSize { + bytes := countChangeBytes(currentBatch) + countChangeBytes(v) + values := countChangeValues(currentBatch) + countChangeValues(v) + + if len(currentBatch)+len(v) > batchSize || bytes > batchSizeBytes || values > batchSizeValues { // currentBatch would be too large if we add this changeset; // add currentBatch to batchChanges and start a new currentBatch batchChanges = append(batchChanges, sortChangesByActionNameType(currentBatch)) diff --git a/provider/aws/aws_test.go b/provider/aws/aws_test.go index 13355b6787..67e2714a38 100644 --- a/provider/aws/aws_test.go +++ b/provider/aws/aws_test.go @@ -19,6 +19,7 @@ package aws import ( "context" "fmt" + "math" "net" "sort" "strings" @@ -39,9 +40,11 @@ import ( ) const ( - defaultBatchChangeSize = 4000 - defaultBatchChangeInterval = time.Second - defaultEvaluateTargetHealth = true + defaultBatchChangeSize = 4000 + defaultBatchChangeSizeBytes = 32000 + defaultBatchChangeSizeValues = 1000 + defaultBatchChangeInterval = time.Second + defaultEvaluateTargetHealth = true ) // Compile time check for interface conformance @@ -1527,7 +1530,7 @@ func TestAWSBatchChangeSet(t *testing.T) { }) } - batchCs := batchChangeSet(cs, defaultBatchChangeSize) + batchCs := batchChangeSet(cs, defaultBatchChangeSize, defaultBatchChangeSizeBytes, defaultBatchChangeSizeValues) require.Equal(t, 1, len(batchCs)) @@ -1565,7 +1568,7 @@ func TestAWSBatchChangeSetExceeding(t *testing.T) { ) } - batchCs := batchChangeSet(cs, testLimit) + batchCs := batchChangeSet(cs, testLimit, defaultBatchChangeSizeBytes, defaultBatchChangeSizeValues) require.Equal(t, expectedBatchCount, len(batchCs)) @@ -1603,11 +1606,227 @@ func TestAWSBatchChangeSetExceedingNameChange(t *testing.T) { ) } - batchCs := batchChangeSet(cs, testLimit) + batchCs := batchChangeSet(cs, testLimit, defaultBatchChangeSizeBytes, defaultBatchChangeSizeValues) require.Equal(t, 0, len(batchCs)) } +func TestAWSBatchChangeSetExceedingBytesLimit(t *testing.T) { + var cs Route53Changes + const testCount = 50 + const testLimit = 100 + const groupSize = 2 + // Bytes for each name + var testBytes = len([]byte("1.2.3.4")) + len([]byte("test-record")) + // testCount / groupSize / (testLimit // bytes) + var expectedBatchCountFloat = float64(testCount) / float64(groupSize) / float64(testLimit/testBytes) + // Round up + var expectedBatchCount = int(math.Floor(expectedBatchCountFloat + 0.5)) + + for i := 1; i <= testCount; i += groupSize { + cs = append(cs, + &Route53Change{ + Change: route53.Change{ + Action: aws.String(route53.ChangeActionCreate), + ResourceRecordSet: &route53.ResourceRecordSet{ + Name: aws.String(fmt.Sprintf("host-%d", i)), + Type: aws.String("A"), + ResourceRecords: []*route53.ResourceRecord{ + { + Value: aws.String("1.2.3.4"), + }, + }, + }, + }, + sizeBytes: len([]byte("1.2.3.4")), + sizeValues: 1, + }, + &Route53Change{ + Change: route53.Change{ + Action: aws.String(route53.ChangeActionCreate), + ResourceRecordSet: &route53.ResourceRecordSet{ + Name: aws.String(fmt.Sprintf("host-%d", i)), + Type: aws.String("TXT"), + ResourceRecords: []*route53.ResourceRecord{ + { + Value: aws.String("txt-record"), + }, + }, + }, + }, + sizeBytes: len([]byte("txt-record")), + sizeValues: 1, + }, + ) + } + + batchCs := batchChangeSet(cs, defaultBatchChangeSize, testLimit, defaultBatchChangeSizeValues) + + require.Equal(t, expectedBatchCount, len(batchCs)) +} + +func TestAWSBatchChangeSetExceedingBytesLimitUpsert(t *testing.T) { + var cs Route53Changes + const testCount = 50 + const testLimit = 100 + const groupSize = 2 + // Bytes for each name multiplied by 2 for Upsert records + var testBytes = (len([]byte("1.2.3.4")) + len([]byte("test-record"))) * 2 + // testCount / groupSize / (testLimit // bytes) + var expectedBatchCountFloat = float64(testCount) / float64(groupSize) / float64(testLimit/testBytes) + // Round up + var expectedBatchCount = int(math.Floor(expectedBatchCountFloat + 0.5)) + + for i := 1; i <= testCount; i += groupSize { + cs = append(cs, + &Route53Change{ + Change: route53.Change{ + Action: aws.String(route53.ChangeActionUpsert), + ResourceRecordSet: &route53.ResourceRecordSet{ + Name: aws.String(fmt.Sprintf("host-%d", i)), + Type: aws.String("A"), + ResourceRecords: []*route53.ResourceRecord{ + { + Value: aws.String("1.2.3.4"), + }, + }, + }, + }, + sizeBytes: len([]byte("1.2.3.4")) * 2, + sizeValues: 1, + }, + &Route53Change{ + Change: route53.Change{ + Action: aws.String(route53.ChangeActionUpsert), + ResourceRecordSet: &route53.ResourceRecordSet{ + Name: aws.String(fmt.Sprintf("host-%d", i)), + Type: aws.String("TXT"), + ResourceRecords: []*route53.ResourceRecord{ + { + Value: aws.String("txt-record"), + }, + }, + }, + }, + sizeBytes: len([]byte("txt-record")) * 2, + sizeValues: 1, + }, + ) + } + + batchCs := batchChangeSet(cs, defaultBatchChangeSize, testLimit, defaultBatchChangeSizeValues) + + require.Equal(t, expectedBatchCount, len(batchCs)) +} + +func TestAWSBatchChangeSetExceedingValuesLimit(t *testing.T) { + var cs Route53Changes + const testCount = 50 + const testLimit = 100 + const groupSize = 2 + // Values for each group + const testValues = 2 + // testCount / groupSize / (testLimit // bytes) + var expectedBatchCountFloat = float64(testCount) / float64(groupSize) / float64(testLimit/testValues) + // Round up + var expectedBatchCount = int(math.Floor(expectedBatchCountFloat + 0.5)) + + for i := 1; i <= testCount; i += groupSize { + cs = append(cs, + &Route53Change{ + Change: route53.Change{ + Action: aws.String(route53.ChangeActionCreate), + ResourceRecordSet: &route53.ResourceRecordSet{ + Name: aws.String(fmt.Sprintf("host-%d", i)), + Type: aws.String("A"), + ResourceRecords: []*route53.ResourceRecord{ + { + Value: aws.String("1.2.3.4"), + }, + }, + }, + }, + sizeBytes: len([]byte("1.2.3.4")), + sizeValues: 1, + }, + &Route53Change{ + Change: route53.Change{ + Action: aws.String(route53.ChangeActionCreate), + ResourceRecordSet: &route53.ResourceRecordSet{ + Name: aws.String(fmt.Sprintf("host-%d", i)), + Type: aws.String("TXT"), + ResourceRecords: []*route53.ResourceRecord{ + { + Value: aws.String("txt-record"), + }, + }, + }, + }, + sizeBytes: len([]byte("txt-record")), + sizeValues: 1, + }, + ) + } + + batchCs := batchChangeSet(cs, defaultBatchChangeSize, defaultBatchChangeSizeBytes, testLimit) + + require.Equal(t, expectedBatchCount, len(batchCs)) +} + +func TestAWSBatchChangeSetExceedingValuesLimitUpsert(t *testing.T) { + var cs Route53Changes + const testCount = 50 + const testLimit = 100 + const groupSize = 2 + // Values for each group multiplied by 2 for Upsert records + const testValues = 2 * 2 + // testCount / groupSize / (testLimit // bytes) + var expectedBatchCountFloat = float64(testCount) / float64(groupSize) / float64(testLimit/testValues) + // Round up + var expectedBatchCount = int(math.Floor(expectedBatchCountFloat + 0.5)) + + for i := 1; i <= testCount; i += groupSize { + cs = append(cs, + &Route53Change{ + Change: route53.Change{ + Action: aws.String(route53.ChangeActionUpsert), + ResourceRecordSet: &route53.ResourceRecordSet{ + Name: aws.String(fmt.Sprintf("host-%d", i)), + Type: aws.String("A"), + ResourceRecords: []*route53.ResourceRecord{ + { + Value: aws.String("1.2.3.4"), + }, + }, + }, + }, + sizeBytes: len([]byte("1.2.3.4")) * 2, + sizeValues: 1, + }, + &Route53Change{ + Change: route53.Change{ + Action: aws.String(route53.ChangeActionUpsert), + ResourceRecordSet: &route53.ResourceRecordSet{ + Name: aws.String(fmt.Sprintf("host-%d", i)), + Type: aws.String("TXT"), + ResourceRecords: []*route53.ResourceRecord{ + { + Value: aws.String("txt-record"), + }, + }, + }, + }, + sizeBytes: len([]byte("txt-record")) * 2, + sizeValues: 1, + }, + ) + } + + batchCs := batchChangeSet(cs, defaultBatchChangeSize, defaultBatchChangeSizeBytes, testLimit) + + require.Equal(t, expectedBatchCount, len(batchCs)) +} + func validateEndpoints(t *testing.T, provider *AWSProvider, endpoints []*endpoint.Endpoint, expected []*endpoint.Endpoint) { assert.True(t, testutils.SameEndpoints(endpoints, expected), "actual and expected endpoints don't match. %+v:%+v", endpoints, expected) @@ -1933,17 +2152,19 @@ func newAWSProviderWithTagFilter(t *testing.T, domainFilter endpoint.DomainFilte client := NewRoute53APIStub(t) provider := &AWSProvider{ - client: client, - batchChangeSize: defaultBatchChangeSize, - batchChangeInterval: defaultBatchChangeInterval, - evaluateTargetHealth: evaluateTargetHealth, - domainFilter: domainFilter, - zoneIDFilter: zoneIDFilter, - zoneTypeFilter: zoneTypeFilter, - zoneTagFilter: zoneTagFilter, - dryRun: false, - zonesCache: &zonesListCache{duration: 1 * time.Minute}, - failedChangesQueue: make(map[string]Route53Changes), + client: client, + batchChangeSize: defaultBatchChangeSize, + batchChangeSizeBytes: defaultBatchChangeSizeBytes, + batchChangeSizeValues: defaultBatchChangeSizeValues, + batchChangeInterval: defaultBatchChangeInterval, + evaluateTargetHealth: evaluateTargetHealth, + domainFilter: domainFilter, + zoneIDFilter: zoneIDFilter, + zoneTypeFilter: zoneTypeFilter, + zoneTagFilter: zoneTagFilter, + dryRun: false, + zonesCache: &zonesListCache{duration: 1 * time.Minute}, + failedChangesQueue: make(map[string]Route53Changes), } createAWSZone(t, provider, &route53.HostedZone{ From 822c2df06b7b07735a3a4e0c3096292d478d5e19 Mon Sep 17 00:00:00 2001 From: Megum1n Date: Thu, 21 Dec 2023 02:42:27 +0100 Subject: [PATCH 2/8] Skip change if it doesn't fit in any batch --- provider/aws/aws.go | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/provider/aws/aws.go b/provider/aws/aws.go index cbd35f319f..e89fa5bd15 100644 --- a/provider/aws/aws.go +++ b/provider/aws/aws.go @@ -915,13 +915,23 @@ func batchChangeSet(cs Route53Changes, batchSize int, batchSizeBytes int, batchS currentBatch := Route53Changes{} for k, name := range names { v := changesByOwnership[name] + vBytes := countChangeBytes(v) + vValues := countChangeValues(v) if len(v) > batchSize { log.Warnf("Total changes for %v exceeds max batch size of %d, total changes: %d; changes will not be performed", k, batchSize, len(v)) continue } + if vBytes > batchSizeBytes { + log.Warnf("Total changes for %v exceeds max batch size bytes of %d, total changes bytes: %d; changes will not be performed", k, batchSizeBytes, vBytes) + continue + } + if vBytes > batchSizeBytes { + log.Warnf("Total changes for %v exceeds max batch size values of %d, total changes values: %d; changes will not be performed", k, batchSizeValues, vValues) + continue + } - bytes := countChangeBytes(currentBatch) + countChangeBytes(v) - values := countChangeValues(currentBatch) + countChangeValues(v) + bytes := countChangeBytes(currentBatch) + vBytes + values := countChangeValues(currentBatch) + vValues if len(currentBatch)+len(v) > batchSize || bytes > batchSizeBytes || values > batchSizeValues { // currentBatch would be too large if we add this changeset; From 126c5130b1588f52fc86d4d49e5accfcd4e92401 Mon Sep 17 00:00:00 2001 From: Megum1n Date: Thu, 21 Dec 2023 02:52:48 +0100 Subject: [PATCH 3/8] Update AWS provider documentation regarding batch size options --- docs/tutorials/aws.md | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/docs/tutorials/aws.md b/docs/tutorials/aws.md index 33204ba713..eac4b25710 100644 --- a/docs/tutorials/aws.md +++ b/docs/tutorials/aws.md @@ -982,3 +982,19 @@ An effective starting point for EKS with an ingress controller might look like: --domain-filter=example.com --aws-zones-cache-duration=1h ``` + +### Batch size options + +After external-dns generates all changes, it will perform a task to group those changes into batches. Each change will be validated against batch-change-size limits. If at least one of those parameters out of range - the change will be moved to a separate branch. If the change can't fit into any batch - *it will be skipped.*
+There are 3 options to control batch size for AWS provider: +* Maximum amount of changes added to one batch + * `--aws-batch-change-size` (default `1000`) +* Maximum size of changes in bytes added to one batch + * `--aws-batch-change-size-bytes` (default `32000`) +* Maximum value count of changes added to one batch + * `aws-batch-change-size-values` (default `1000`) + +`aws-batch-change-size` can be very useful for throttling purposes and can be set to any value. + +Default values for flags `aws-batch-change-size-bytes` and `aws-batch-change-size-values` are taken from [AWS documentation](https://docs.aws.amazon.com/Route53/latest/DeveloperGuide/DNSLimitations.html#limits-api-requests) for Route53 API. **You should not change those values until you really have to.**
+Because those limits are in place, `aws-batch-change-size` can be set to any value: Even if your batch size is `4000` records, your change will be split to separate batches due to bytes/values size limits and apply request will be finished without issues. From a814586ca268c5782827c734250e5447be536daf Mon Sep 17 00:00:00 2001 From: Megum1n Date: Fri, 5 Jan 2024 22:02:02 +0100 Subject: [PATCH 4/8] Test fixes --- provider/aws/aws_test.go | 100 +++++++++++++++++++++++---------------- 1 file changed, 60 insertions(+), 40 deletions(-) diff --git a/provider/aws/aws_test.go b/provider/aws/aws_test.go index 67e2714a38..df60b446c5 100644 --- a/provider/aws/aws_test.go +++ b/provider/aws/aws_test.go @@ -1612,16 +1612,21 @@ func TestAWSBatchChangeSetExceedingNameChange(t *testing.T) { } func TestAWSBatchChangeSetExceedingBytesLimit(t *testing.T) { - var cs Route53Changes - const testCount = 50 - const testLimit = 100 - const groupSize = 2 - // Bytes for each name - var testBytes = len([]byte("1.2.3.4")) + len([]byte("test-record")) - // testCount / groupSize / (testLimit // bytes) - var expectedBatchCountFloat = float64(testCount) / float64(groupSize) / float64(testLimit/testBytes) - // Round up - var expectedBatchCount = int(math.Floor(expectedBatchCountFloat + 0.5)) + const ( + testCount = 50 + testLimit = 100 + groupSize = 2 + ) + + var ( + cs Route53Changes + // Bytes for each name + testBytes = len([]byte("1.2.3.4")) + len([]byte("test-record")) + // testCount / groupSize / (testLimit // bytes) + expectedBatchCountFloat = float64(testCount) / float64(groupSize) / float64(testLimit/testBytes) + // Round up + expectedBatchCount = int(math.Ceil(expectedBatchCountFloat)) + ) for i := 1; i <= testCount; i += groupSize { cs = append(cs, @@ -1666,16 +1671,21 @@ func TestAWSBatchChangeSetExceedingBytesLimit(t *testing.T) { } func TestAWSBatchChangeSetExceedingBytesLimitUpsert(t *testing.T) { - var cs Route53Changes - const testCount = 50 - const testLimit = 100 - const groupSize = 2 - // Bytes for each name multiplied by 2 for Upsert records - var testBytes = (len([]byte("1.2.3.4")) + len([]byte("test-record"))) * 2 - // testCount / groupSize / (testLimit // bytes) - var expectedBatchCountFloat = float64(testCount) / float64(groupSize) / float64(testLimit/testBytes) - // Round up - var expectedBatchCount = int(math.Floor(expectedBatchCountFloat + 0.5)) + const ( + testCount = 50 + testLimit = 100 + groupSize = 2 + ) + + var ( + cs Route53Changes + // Bytes for each name multiplied by 2 for Upsert records + testBytes = (len([]byte("1.2.3.4")) + len([]byte("test-record"))) * 2 + // testCount / groupSize / (testLimit // bytes) + expectedBatchCountFloat = float64(testCount) / float64(groupSize) / float64(testLimit/testBytes) + // Round up + expectedBatchCount = int(math.Ceil(expectedBatchCountFloat)) + ) for i := 1; i <= testCount; i += groupSize { cs = append(cs, @@ -1720,16 +1730,21 @@ func TestAWSBatchChangeSetExceedingBytesLimitUpsert(t *testing.T) { } func TestAWSBatchChangeSetExceedingValuesLimit(t *testing.T) { - var cs Route53Changes - const testCount = 50 - const testLimit = 100 - const groupSize = 2 - // Values for each group - const testValues = 2 - // testCount / groupSize / (testLimit // bytes) - var expectedBatchCountFloat = float64(testCount) / float64(groupSize) / float64(testLimit/testValues) - // Round up - var expectedBatchCount = int(math.Floor(expectedBatchCountFloat + 0.5)) + const ( + testCount = 50 + testLimit = 100 + groupSize = 2 + // Values for each group + testValues = 2 + ) + + var ( + cs Route53Changes + // testCount / groupSize / (testLimit // bytes) + expectedBatchCountFloat = float64(testCount) / float64(groupSize) / float64(testLimit/testValues) + // Round up + expectedBatchCount = int(math.Ceil(expectedBatchCountFloat)) + ) for i := 1; i <= testCount; i += groupSize { cs = append(cs, @@ -1774,16 +1789,21 @@ func TestAWSBatchChangeSetExceedingValuesLimit(t *testing.T) { } func TestAWSBatchChangeSetExceedingValuesLimitUpsert(t *testing.T) { - var cs Route53Changes - const testCount = 50 - const testLimit = 100 - const groupSize = 2 - // Values for each group multiplied by 2 for Upsert records - const testValues = 2 * 2 - // testCount / groupSize / (testLimit // bytes) - var expectedBatchCountFloat = float64(testCount) / float64(groupSize) / float64(testLimit/testValues) - // Round up - var expectedBatchCount = int(math.Floor(expectedBatchCountFloat + 0.5)) + const ( + testCount = 50 + testLimit = 100 + groupSize = 2 + // Values for each group multiplied by 2 for Upsert records + testValues = 2 * 2 + ) + + var ( + cs Route53Changes + // testCount / groupSize / (testLimit // bytes) + expectedBatchCountFloat = float64(testCount) / float64(groupSize) / float64(testLimit/testValues) + // Round up + expectedBatchCount = int(math.Ceil(expectedBatchCountFloat)) + ) for i := 1; i <= testCount; i += groupSize { cs = append(cs, From c711820c77f89c2e1e40fdbe66e0d665d4dfd052 Mon Sep 17 00:00:00 2001 From: Megum1n Date: Fri, 5 Jan 2024 22:02:10 +0100 Subject: [PATCH 5/8] Bug fixes --- provider/aws/aws.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/provider/aws/aws.go b/provider/aws/aws.go index e89fa5bd15..67efe9fdd9 100644 --- a/provider/aws/aws.go +++ b/provider/aws/aws.go @@ -767,7 +767,7 @@ func (p *AWSProvider) newChange(action string, ep *endpoint.Endpoint) (*Route53C } if action == route53.ChangeActionUpsert { - // When the value of the Action element is UPSERT, each ResourceRecord element and each character in a Value + // If the value of the Action element is UPSERT, each ResourceRecord element and each character in a Value // element is counted twice change.sizeBytes *= 2 change.sizeValues *= 2 @@ -925,7 +925,7 @@ func batchChangeSet(cs Route53Changes, batchSize int, batchSizeBytes int, batchS log.Warnf("Total changes for %v exceeds max batch size bytes of %d, total changes bytes: %d; changes will not be performed", k, batchSizeBytes, vBytes) continue } - if vBytes > batchSizeBytes { + if vValues > batchSizeValues { log.Warnf("Total changes for %v exceeds max batch size values of %d, total changes values: %d; changes will not be performed", k, batchSizeValues, vValues) continue } From 1a8bbd3af3623b8b9c6dcdd39ebadbe94f18945a Mon Sep 17 00:00:00 2001 From: Megum1n Date: Fri, 12 Jan 2024 00:20:54 +0100 Subject: [PATCH 6/8] Fix typo in docs --- docs/tutorials/aws.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/tutorials/aws.md b/docs/tutorials/aws.md index eac4b25710..e6c87a534e 100644 --- a/docs/tutorials/aws.md +++ b/docs/tutorials/aws.md @@ -329,7 +329,7 @@ kubectl patch serviceaccount "external-dns" --namespace ${EXTERNALDNS_NS:-"defau "{\"metadata\": { \"annotations\": { \"eks.amazonaws.com/role-arn\": \"$ROLE_ARN\" }}}" ``` -If any part of this step is misconfigured, such as the role with incorrect namespace configured in the trust relationship, annotation pointing the the wrong role, etc., you will see errors like `WebIdentityErr: failed to retrieve credentials`. Check the configuration and make corrections. +If any part of this step is misconfigured, such as the role with incorrect namespace configured in the trust relationship, annotation pointing the the wrong role, etc., you will see errors like `WebIdentityErr: failed to retrieve credentials`. Check the configuration and make corrections. When the service account annotations are updated, then the current running pods will have to be terminated, so that new pod(s) with proper configuration (environment variables) will be created automatically. @@ -985,7 +985,7 @@ An effective starting point for EKS with an ingress controller might look like: ### Batch size options -After external-dns generates all changes, it will perform a task to group those changes into batches. Each change will be validated against batch-change-size limits. If at least one of those parameters out of range - the change will be moved to a separate branch. If the change can't fit into any batch - *it will be skipped.*
+After external-dns generates all changes, it will perform a task to group those changes into batches. Each change will be validated against batch-change-size limits. If at least one of those parameters out of range - the change will be moved to a separate batch. If the change can't fit into any batch - *it will be skipped.*
There are 3 options to control batch size for AWS provider: * Maximum amount of changes added to one batch * `--aws-batch-change-size` (default `1000`) @@ -997,4 +997,4 @@ There are 3 options to control batch size for AWS provider: `aws-batch-change-size` can be very useful for throttling purposes and can be set to any value. Default values for flags `aws-batch-change-size-bytes` and `aws-batch-change-size-values` are taken from [AWS documentation](https://docs.aws.amazon.com/Route53/latest/DeveloperGuide/DNSLimitations.html#limits-api-requests) for Route53 API. **You should not change those values until you really have to.**
-Because those limits are in place, `aws-batch-change-size` can be set to any value: Even if your batch size is `4000` records, your change will be split to separate batches due to bytes/values size limits and apply request will be finished without issues. +Because those limits are in place, `aws-batch-change-size` can be set to any value: Even if your batch size is `4000` records, your change will be split to separate batches due to bytes/values size limits and apply request will be finished without issues. From 7f726e09f5a6b0def253e8a1429ff1d7eb2d58fb Mon Sep 17 00:00:00 2001 From: Megum1n Date: Fri, 12 Jan 2024 00:22:48 +0100 Subject: [PATCH 7/8] Remove unnecessary assignments --- provider/aws/aws.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/provider/aws/aws.go b/provider/aws/aws.go index 67efe9fdd9..d15ee4ac8b 100644 --- a/provider/aws/aws.go +++ b/provider/aws/aws.go @@ -728,8 +728,6 @@ func (p *AWSProvider) newChange(action string, ep *endpoint.Endpoint) (*Route53C Name: aws.String(ep.DNSName), }, }, - sizeBytes: 0, - sizeValues: 0, } dualstack := false if targetHostedZone := isAWSAlias(ep); targetHostedZone != "" { From 4ed7b2888d55960f09d272bba30802caaaf8f556 Mon Sep 17 00:00:00 2001 From: Megum1n Date: Wed, 7 Feb 2024 16:29:51 +0100 Subject: [PATCH 8/8] Fix struct indentation --- main.go | 22 ++++++++++----------- provider/aws/aws.go | 48 ++++++++++++++++++++++----------------------- 2 files changed, 35 insertions(+), 35 deletions(-) diff --git a/main.go b/main.go index 5230915793..ebcabcba31 100644 --- a/main.go +++ b/main.go @@ -229,19 +229,19 @@ func main() { case "aws": p, err = aws.NewAWSProvider( aws.AWSConfig{ - DomainFilter: domainFilter, - ZoneIDFilter: zoneIDFilter, - ZoneTypeFilter: zoneTypeFilter, - ZoneTagFilter: zoneTagFilter, - ZoneMatchParent: cfg.AWSZoneMatchParent, - BatchChangeSize: cfg.AWSBatchChangeSize, + DomainFilter: domainFilter, + ZoneIDFilter: zoneIDFilter, + ZoneTypeFilter: zoneTypeFilter, + ZoneTagFilter: zoneTagFilter, + ZoneMatchParent: cfg.AWSZoneMatchParent, + BatchChangeSize: cfg.AWSBatchChangeSize, BatchChangeSizeBytes: cfg.AWSBatchChangeSizeBytes, BatchChangeSizeValues: cfg.AWSBatchChangeSizeValues, - BatchChangeInterval: cfg.AWSBatchChangeInterval, - EvaluateTargetHealth: cfg.AWSEvaluateTargetHealth, - PreferCNAME: cfg.AWSPreferCNAME, - DryRun: cfg.DryRun, - ZoneCacheDuration: cfg.AWSZoneCacheDuration, + BatchChangeInterval: cfg.AWSBatchChangeInterval, + EvaluateTargetHealth: cfg.AWSEvaluateTargetHealth, + PreferCNAME: cfg.AWSPreferCNAME, + DryRun: cfg.DryRun, + ZoneCacheDuration: cfg.AWSZoneCacheDuration, }, route53.New(awsSession), ) diff --git a/provider/aws/aws.go b/provider/aws/aws.go index 9045bde5e6..7a077212de 100644 --- a/provider/aws/aws.go +++ b/provider/aws/aws.go @@ -253,39 +253,39 @@ type AWSProvider struct { // AWSConfig contains configuration to create a new AWS provider. type AWSConfig struct { - DomainFilter endpoint.DomainFilter - ZoneIDFilter provider.ZoneIDFilter - ZoneTypeFilter provider.ZoneTypeFilter - ZoneTagFilter provider.ZoneTagFilter - ZoneMatchParent bool - BatchChangeSize int + DomainFilter endpoint.DomainFilter + ZoneIDFilter provider.ZoneIDFilter + ZoneTypeFilter provider.ZoneTypeFilter + ZoneTagFilter provider.ZoneTagFilter + ZoneMatchParent bool + BatchChangeSize int BatchChangeSizeBytes int BatchChangeSizeValues int - BatchChangeInterval time.Duration - EvaluateTargetHealth bool - PreferCNAME bool - DryRun bool - ZoneCacheDuration time.Duration + BatchChangeInterval time.Duration + EvaluateTargetHealth bool + PreferCNAME bool + DryRun bool + ZoneCacheDuration time.Duration } // NewAWSProvider initializes a new AWS Route53 based Provider. func NewAWSProvider(awsConfig AWSConfig, client Route53API) (*AWSProvider, error) { provider := &AWSProvider{ - client: client, - domainFilter: awsConfig.DomainFilter, - zoneIDFilter: awsConfig.ZoneIDFilter, - zoneTypeFilter: awsConfig.ZoneTypeFilter, - zoneTagFilter: awsConfig.ZoneTagFilter, - zoneMatchParent: awsConfig.ZoneMatchParent, - batchChangeSize: awsConfig.BatchChangeSize, + client: client, + domainFilter: awsConfig.DomainFilter, + zoneIDFilter: awsConfig.ZoneIDFilter, + zoneTypeFilter: awsConfig.ZoneTypeFilter, + zoneTagFilter: awsConfig.ZoneTagFilter, + zoneMatchParent: awsConfig.ZoneMatchParent, + batchChangeSize: awsConfig.BatchChangeSize, batchChangeSizeBytes: awsConfig.BatchChangeSizeBytes, batchChangeSizeValues: awsConfig.BatchChangeSizeValues, - batchChangeInterval: awsConfig.BatchChangeInterval, - evaluateTargetHealth: awsConfig.EvaluateTargetHealth, - preferCNAME: awsConfig.PreferCNAME, - dryRun: awsConfig.DryRun, - zonesCache: &zonesListCache{duration: awsConfig.ZoneCacheDuration}, - failedChangesQueue: make(map[string]Route53Changes), + batchChangeInterval: awsConfig.BatchChangeInterval, + evaluateTargetHealth: awsConfig.EvaluateTargetHealth, + preferCNAME: awsConfig.PreferCNAME, + dryRun: awsConfig.DryRun, + zonesCache: &zonesListCache{duration: awsConfig.ZoneCacheDuration}, + failedChangesQueue: make(map[string]Route53Changes), } return provider, nil