Validate AWS record values size during batch set generation

[megum1n]

Description
There is a well known problem with AWS provider: External DNS throws an error during apply step due to AWS limitations for the record values size.

ResourceRecord elements
A request cannot contain more than 1,000 ResourceRecord elements (including alias records). When the value of the Action element is UPSERT, each ResourceRecord element is counted twice.

Maximum number of characters
The sum of the number of characters (including spaces) in all Value elements in a request cannot exceed 32,000 characters. When the value of the Action element is UPSERT, each character in a Value element is counted twice.

In this PR I added a validation step for all new changes. During batch set calculation all changes that exceed AWS limitations will be added to a separate batch.

Changes:

• Added aws-batch-change-size-bytes flag. Default: 32000
• Added aws-batch-change-size-values flag. Default: 1000
• Added validation step for AWS records values size

Fixes:

• create/upsert records fail if too many AWS records to apply #731
• Clarify AWS BatchSize and Record Sets limitations #3203
• Match batch size to payload limit in bytes for AWS #3892

Checklist

• Unit tests updated
• End user documentation updated

[k8s-ci-robot]

Hi @megum1n. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[megum1n]

Should we also validate new flags for a min value? External DNS will stuck in infinite loop if someone decides to set aws-batch-change-size-bytes to 6, for example (1.2.3.4 = 7 bytes).

[mloiseleur]

/ok-to-test

[mloiseleur]

\o Thanks for this PR.
It could help yes, to avoid infinite loop.

Wdyt about adding some docs on this subject ?
A link to AWS current limit could be helpful to know what would be a good value to set, for instance.

[mloiseleur]

cc @szuecs

[megum1n]

@mloiseleur thanks

It could help yes, to avoid infinite loop.

I added the same validation step into batchChangeSet function as for batch-size. Now there won't be any infinite loops - change will be skipped in case when it can't fit into any batch.

We can also validate those flags during program startup. We have two options to do it:

• Use validation module and return fatal error in case when batch-size-bytes or batch-size-values values are too low. In this case external-dns won't be started.
• Add validation step into main.go during AWS provider initialization step. If value is too low - throw a warning in the logs and use default value as fallback. The program will continue to work with default value instead.

I'm not sure which value should be considered as "too low". One A record with UPSERT action can take up to 30 bytes, and there are also TXT records, which take up to 510 bytes each. If we want to add such validation, I think min value should be at least 5000 bytes, maybe even more.

The same goes for batch-size-values - one UPSERT request will add at least 2 values.

Also, all changes are being grouped by ownership/name first, so there is a high chance to get a change, which won't fit in any batch.

I don't see any reason to change those values anyway :)

Wdyt about adding some docs on this subject ?

I updated docs as well 👍🏻

[mloiseleur]

I don't see any reason to change those values anyway :)

Same here 😅 .
Many thanks, the doc is clear and concise.
/lgtm

[szuecs]

@megum1n thanks for the PR, makes also sense your approach. There are a couple of comments and I think I found a bug. If you fixed everything just ping me again.

[megum1n]

@szuecs thank you for the review!
I fixed all the issues.

[szuecs]

I found only 2 small ones. I think the doc needs to be fixed and if you have that you can also drop the 0 value setting in the same.

Thanks for the work!

[megum1n]

@szuecs ping

[mloiseleur]

/lgtm

[szuecs]

/approve

[szuecs]

@megum1n can you resolve the conflicts? Thanks!

[megum1n]

@szuecs done

[megum1n]

Tests passed localy w/o any issues 🤔

/retest-required

[szuecs]

/test pull-external-dns-unit-test

[pascalgn]

I created #4240 to consolidate discussions of TestCoreDNSApplyChanges test failure

[szuecs]

please rebase the test was fixed

[szuecs]

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: szuecs

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [szuecs]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment