Reduce AWS Route53 API calls #2010

tjamet · 2021-03-12T14:21:42Z

Currently, planning instructs to create all records even
those which does not match any zone.
Later, those records will be checked towards the existing
records and filtered whether they match or not a hosted zone.

This causes a problem, at least in the specific case of the Route53
implementation as it always calls the ApplyChanges method, which in its
turn always retrieves all records in all zones.

This causes high pressure on Route53 APIs, for non-necessary actions.

By being able to filter all unmanaged records from the plan, we can
prevent from calling ApplyChanges when nothing has to be done and hence
prevent an unnecessary listing of records.

By doing so, the rate of API calls to AWS Route53 is expected to be
reduced by 2

To do this change, providers now export, through registries, a DomainFilter, empty by default, provided by the BaseProvider.
This caused some conflicts on dyn and rcode0 provider structures that have been adapted to be closer to other provider ones.

Any provider may now implement such a feature to restrict the planning output before it enters the registry.

Helps to improve #1293

With this change, in the case of a single instance of external-dns running for a single owner, we can expect no-op changes to reduce by 2 the number of calls to the AWS Route53 API while there are no changes to perform.

Checklist

Unit tests updated
End user documentation updated

tjamet · 2021-03-12T14:31:23Z

/assign @njuettner

njuettner

Hey thanks your awesome work, really appreciated. I added some comments.

I'm happy to merge it if we can address my comments 🙂

njuettner · 2021-03-30T07:53:38Z

plan/plan.go

@@ -40,7 +43,7 @@ type Plan struct {
 	// Populated after calling Calculate()
 	Changes *Changes
 	// DomainFilter matches DNS names
-	DomainFilter endpoint.DomainFilter
+	DomainFilter endpoint.DomainFilterInterface


nit, I wouldn't name it Interface. Just change the description showing it's an interface

I thought about changing endpoints.DomainFilter to be an interface. The problem I face is that it is several providers like AlibabaCloud are relying on the internals ( mostly the Filters entry) I would like to keep the PR as small as possible even though it may lead to a need fo refactoring

njuettner · 2021-03-30T07:58:54Z

controller/controller_test.go

@@ -192,3 +216,44 @@ func TestShouldRunOnce(t *testing.T) {
 	// But not two times
 	assert.False(t, ctrl.ShouldRunOnce(now))
 }
+
+func TestControllerSkipsEmptyChanges(t *testing.T) {


Can you add tests for several domain filter and without a domain filter?

njuettner · 2021-03-30T08:02:40Z

provider/dyn/dyn.go

@@ -145,7 +154,14 @@ type ZonePublishResponse struct {
 // NewDynProvider initializes a new Dyn Provider.
 func NewDynProvider(config DynConfig) (provider.Provider, error) {
 	return &dynProviderState{


Please create a separate PR for dyn provider, I'd like to keep the PR as small as possible

seanmalloy · 2021-04-13T04:23:14Z

/kind bug

njuettner · 2021-04-22T06:59:24Z

@tjamet 👋🏻 any progress since my last my comment? anything we can help with? We'd really like to merge it ❤️ . Is it possible to split it into smaller PR's, so we can focus for now only on Route53 changes?

tjamet · 2021-04-22T22:13:20Z

Hi @njuettner
Yes, I am working on making smaller PRs. I am slown down by some naming conflicts between field and function.
I keep you posted very soon

Currently, planning instructs to create all records even those which does not match any zone. Later, those records will be checked towards the existing records and filtered whether they match or not a hosted zone. This causes a problem, at least in the specific case of the Route53 implementation as it always calls the ApplyChanges method, which in its turn always retrieves all records in all zones. This causes high pressure on Route53 APIs, for non-necessary actions. By being able to filter all unmanaged records from the plan, we can prevent from calling ApplyChanges when nothing has to be done and hence prevent an unnecessary listing of records. By doing so, the rate of API calls to AWS Route53 is expected to be reduced by 2

cmsmith7 · 2021-04-29T09:35:18Z

Hi .. please could you advise when you think this PR will be merged? Many thanks.

njuettner

small nit

controller/controller.go

Co-authored-by: Nick Jüttner <nick@juni.io>

njuettner · 2021-05-26T06:43:51Z

@tjamet is this ready for another review? Will the DomainFilterInterface break anything for other providers? You mentioned it is used by different providers? Anything we need to test here as well?

tjamet · 2021-05-27T07:27:39Z

I think it is ready for review yes.
I don't think, in the current implementation, the domain filter interface would break other providers, as it is added on top. The problem I have seen was with naming it a DomainFilter that conflicts with other providers internal implementations

njuettner · 2021-05-27T13:44:39Z

I think it is ready for review yes.
I don't think, in the current implementation, the domain filter interface would break other providers, as it is added on top. The problem I have seen was with naming it a DomainFilter that conflicts with other providers internal implementations

Thanks again, as long we don't introduce a break and all providers keep running, I'm fine merging it 🙂

tjamet · 2021-06-01T21:58:55Z

as long we don't introduce a break and all providers keep running

Indeed, I have been careful to do the smallest change possible and keep backward compatibility for this specific purpose.
Re-reading the change, I don't see a risk for any other provider. the added filter is indeed added as an and filter. The provided default DomainFilter that defaults to match all
hence the added and is equivalent to the previous DomainFilter.Match([...) && true

This makes me think this change should be safe for other providers

njuettner

Ok @tjamet you wanted long enough, sorry about that. Once again thank you for your contribution and sorry about taking longer to merge this PR.

/lgtm

k8s-ci-robot · 2021-06-23T06:59:24Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: njuettner, tjamet

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~~ [njuettner]

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

tjamet · 2021-06-28T10:15:12Z

no worries @njuettner thanks for yout support

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 12, 2021

k8s-ci-robot requested review from njuettner and Raffo March 12, 2021 14:21

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 12, 2021

tjamet force-pushed the limit-required-aws-calls-only branch from b10bf4b to 11e44d3 Compare March 12, 2021 14:26

tjamet mentioned this pull request Mar 12, 2021

AWS provider triggers lots of "Throttling: Rate exceeded\n\tstatus code: 400" #1293

Closed

k8s-ci-robot assigned njuettner Mar 12, 2021

tjamet force-pushed the limit-required-aws-calls-only branch from 11e44d3 to 5afaa9c Compare March 12, 2021 14:38

njuettner requested changes Mar 30, 2021

View reviewed changes

njuettner added the provider/aws label Mar 30, 2021

k8s-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Apr 13, 2021

tjamet force-pushed the limit-required-aws-calls-only branch from 5afaa9c to 17fb881 Compare April 25, 2021 16:10

njuettner requested changes May 5, 2021

View reviewed changes

controller/controller.go Outdated Show resolved Hide resolved

Update controller/controller.go

0eebf53

Co-authored-by: Nick Jüttner <nick@juni.io>

njuettner approved these changes Jun 23, 2021

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 23, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 23, 2021

k8s-ci-robot merged commit 55b7119 into kubernetes-sigs:master Jun 23, 2021

orirawlings mentioned this pull request Jun 17, 2022

Domain exclusion filter not being considered since >= v0.9.0 #2821

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reduce AWS Route53 API calls #2010

Reduce AWS Route53 API calls #2010

tjamet commented Mar 12, 2021

tjamet commented Mar 12, 2021

njuettner left a comment

njuettner Mar 30, 2021

tjamet Apr 25, 2021

njuettner Mar 30, 2021

njuettner Mar 30, 2021

seanmalloy commented Apr 13, 2021

njuettner commented Apr 22, 2021

tjamet commented Apr 22, 2021

cmsmith7 commented Apr 29, 2021

njuettner left a comment

njuettner commented May 26, 2021

tjamet commented May 27, 2021

njuettner commented May 27, 2021

tjamet commented Jun 1, 2021

njuettner left a comment

k8s-ci-robot commented Jun 23, 2021

tjamet commented Jun 28, 2021

Reduce AWS Route53 API calls #2010

Reduce AWS Route53 API calls #2010

Conversation

tjamet commented Mar 12, 2021

tjamet commented Mar 12, 2021

njuettner left a comment

Choose a reason for hiding this comment

njuettner Mar 30, 2021

Choose a reason for hiding this comment

tjamet Apr 25, 2021

Choose a reason for hiding this comment

njuettner Mar 30, 2021

Choose a reason for hiding this comment

njuettner Mar 30, 2021

Choose a reason for hiding this comment

seanmalloy commented Apr 13, 2021

njuettner commented Apr 22, 2021

tjamet commented Apr 22, 2021

cmsmith7 commented Apr 29, 2021

njuettner left a comment

Choose a reason for hiding this comment

njuettner commented May 26, 2021

tjamet commented May 27, 2021

njuettner commented May 27, 2021

tjamet commented Jun 1, 2021

njuettner left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jun 23, 2021

tjamet commented Jun 28, 2021