rfc2136: add batch size support #2127
Conversation
|
Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA. It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.
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. I understand the commands that are listed here. |
k8s-ci-robot
added
the
cncf-cla: no
|
Welcome @knackaron! |
k8s-ci-robot
added
the
size/L
|
I signed it. |
k8s-ci-robot
added
cncf-cla: yes
There was a problem hiding this comment.
Hey @knackaron 👋🏻 , thank you for your contribution. Do you mind adding unit tests for you changes? Let me know if you need help.
Will do. Have @knackjeff working on adding them now. Will update one we push another commit with passing unit tests for the new functionality. |
|
I signed it |
k8s-ci-robot
removed
the
cncf-cla: yes
|
Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA. It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.
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. I understand the commands that are listed here. |
k8s-ci-robot
added
cncf-cla: no
|
I signed it |
|
@knackjeff it seems linter is complaining |
Signed-off-by: Aron Parsons <aparsons@knackworks.com>
|
@njuettner sorry about that. I pulled down the golangci-linter package and have at least verified the code we've modified in this PR is linted! |
k8s-ci-robot
added
lgtm
njuettner
removed
approved
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: knackaron, njuettner 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:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
When facing large messages, one can have its dns refusing the update due to "too large message" error. The DNS library used propose to compress the message that is sent. This commit exposes this feature. It adds a new command line option named --enable-compression and adds a new field to the configuration. The default value of the field is set to false to not change the current behavior of external-dns. We had this change pending locally on our side since a long time. As of today, an alternative would be to use the rencent `batchChangeSize` option from kubernetes-sigs#2127 to overcome the same issue.
When facing large messages, one can have its dns refusing the update due to "too large message" error. The DNS library used propose to compress the message that is sent. This commit exposes this feature. It adds a new command line option named --enable-compression and adds a new field to the configuration. The default value of the field is set to false to not change the current behavior of external-dns. We had this change pending locally on our side since a long time. As of today, an alternative would be to use the rencent `batchChangeSize` option from kubernetes-sigs#2127 to overcome the same issue.
When facing large messages, one can have its dns refusing the update due to "too large message" error. The DNS library used propose to compress the message that is sent. This commit exposes this feature. It adds a new command line option named --enable-compression and adds a new field to the configuration. The default value of the field is set to false to not change the current behavior of external-dns. We had this change pending locally on our side since a long time. As of today, an alternative would be to use the rencent `batchChangeSize` option from kubernetes-sigs#2127 to overcome the same issue.
Description
This PR adds a new
--rfc2136-batch-change-sizeoption, which follows the AWS and Google providers, which already support controlling the batch size. I defaulted the batch size to 50, which seems like a sane default to prevent large updates, while at the same time keeping most transactions atomic, as was the intent with the batching support in the first place.Savvy users who want to revert to the
0.5behavior without batching support can set their batch size to 1 with this patch to achieve that, which would help address #1517.When the rfc2136 provider gained batching support (#1164), it introduced a couple of regressions for us in behavior compared to the
0.5series.Large updates, even with TCP, can exceed what the DNS server allows which can still result in
warn in dns.Client.Exchange: dns: message too largeerrors. We have observed this in production environments in normal operation when our ingress controllers changed their load balancer external IPs during a migration, requiring DNS updates to hundreds of records at once. For the purpose of developing this patch, I was able to reproduce the behavior with by creating a large number (500) of new ingresses at once.If any update in the batch fails to apply, the whole transaction fails, which leads to one bad resource in the cluster (e.g., a tenant's Ingress) being able to DoS external-dns for the whole cluster. This has also been observed in production environments for us, whereas when batching was not in use, only the bad update failed and others went through. This regression is already noted in One invalid record in ChangeBatch stops all others from updating #1517.