Add new method to provider interface to implement provider specific changes

[Raffo]

This PR builds on #1849 and #1860 and introduces a system to have providers morify/adjust the endpoints before submitting them to a plan. This allows us to stop adding patches to the plan which should be as provider independent as possible and be very simple. The OR condition on line 144 of provider.go is already a bit hard to read and I don't want to add any complexity to that part of the code.
Also, this was built similarly to #1849 but without introducing an additional interface and then starting to deal with more complicated parts of code, rather I ended up adding a method to the provider interface and its implementation in the baseprovider. This keeps the changes for other providers minimal and keeps the code honest: we need to access the provider because we want to ask the provider "you know better, please adjust the endpoints for me".

As I didn't really participate closely to the definition of the interfaces back in 2017, I'm asking @linki and @njuettner if this looks fine or if it is a big violation of the way ExternalDNS is designed.

I originally wanted to do an even bigger refactor, but while I started mostly adding tests (as you can see from the commit history), it became clear that the abstractions that we had assumed that from the controller we should have never asked the provider and instead implemented provider specific things via IMO-hard-to-understand mechanisms inside the Calculate method of the plan.

Note: this does not replace #1860, but I would love to understand if we can use this as a base to build a simpler version of #1860.

[linki]

Adding the "raw" Provider here makes sense but it could lead to confusion.

Controller is supposed to connect Source with Provider, hence it has references to it. However, we introduced Registry at some point which acts as a "Proxy" for a Provider that "knows how to handle ownership". If you look at Registry's interface it's identical to Provider.

The idea was:

• Don't do any ownership related stuff in the Providers.
• Wrap a Provider with a Registry to implement the ownership pieces (e.g. filtering)
• Use the Registry wherever you'd use a Provider.

Unfortunately, we never embraced that similarity. If we did Registry would be renamed to Provider here and it would be more obvious why we don't need an additional field.

So, if it's not too confusing we could go through the Registry here so that we don't have two entry points into the Provider's records.

If that's not desirable we should document somewhere for which purposes the Registry and the Provider fields should be used. For AdjustEndpoints it might be totally fine to go directly to the Provider (not through the Registry). If that's the case we can use it this way but should document it. Otherwise following PRs might use the Provider directly for other purposes and bypass the ownership code unintentionally.

[sheerun]

This was my initial approach in #1849 but then I've switched from adding Provider to adding NormalizeDesiredEndpoint which is AdjustEndpoints equivalent. I think for consistency all methods should be called on registry. In #1849 they are simple proxies: https://github.com/kubernetes-sigs/external-dns/pull/1849/files#diff-3cdcf094f15e7824ba9d019ebb5acc1134c560c724b8ed35b6ce5fef2202b607

[Raffo]

Controller is supposed to connect Source with Provider, hence it has references to it. However, we introduced Registry at some point which acts as a "Proxy" for a Provider that "knows how to handle ownership". If you look at Registry's interface it's identical to Provider.

I know this and absolutely thought about that. I was sitting on the idea of using registry as proxy and I'd be up to it if we can make it in a similar way that is straightforward as it is right now. I'm not particularly fond of the idea of the registry TBH, as it seems we are making things harder to understand just to have a "cache". But I'll play with it and post an update.

[Raffo]

@linki I'm also not sure why we have two interfaces that are identical... if both registry and providers implement the same functionality, shouldn't we define the interface once?

[sheerun]

I think such refactor would be fine, but for separate PR

[linki]

@Raffo Back then we didn't come to agreement to go this far. They had the same interface but Registry wasn't quite conceived as a "full provider" and it might be confusing. I'm fine with with combining them into one.

[linki]

same different => different

[linki]

Should this use test.propertyComparator?

[sheerun]

PropertyComparator should be removed as in #1849 because it can be implemented with AdjustEndpoint. There's no need for duplication in logic and PropertyComparator is used only by CloudFlare provider which I currently own.

[Raffo]

+1, we can remove it.

[Raffo]

EDIT: I'm not so sure anymore. What about

external-dns/provider/aws/aws.go

Lines 209 to 211 in 904bf83

	if name == "aws/evaluate-target-health" {
	return true
	}

? I think we need the two functionality: one is about doing side effects on the records in a provider specific way because the provider "know better", the other is implementing a comparison. In #1849 those were mixed (like in https://github.com/kubernetes-sigs/external-dns/pull/1849/files#diff-efafdf56d71f1a3f19d003c8bad9fbadb8ee34e873cfa41362ca880e9f7c5fd3R382-R384 ), but I'm not thinking that we should probably keep the distinction.

[sheerun]

The code you are linking can be implemented with AdjustEndpoints by hard-setting value of aws/evaluate-target-health custom property to what is currently defined. I think implementing this would require changing signature of AdjustEndpoints to AdjustEndpoints(current []*endpoint.Endpoint, desired []*endpoint.Endpoint) []*endpoint.Endpoint where the result are adjusted desired endpoints. Actually it would also make sense to change name of AdjustEndpoints to AdjustDesiredEndpoints or something like this.

[sheerun]

Yet somehow it doesn't feel right either. I'm thinking that it could be more reasonable to change AdjustEndpoints to ChangedEndpoints(current []*endpoint.Endpoint, desired []*endpoint.Endpoint) []*endpoint.Endpoint, which would take current and desired endpoints, and return only these that should be changed (no more diffing would be performed on the result). The default implementation in BaseProvider would be something like code in plan.Calculate(), and providers could override it (also call implementation of BaseProvider).

[sheerun]

Effectively this would render plan.go obsolete and would make planning (calculating needed changes) provider-specific with default implementation in BaseProvider. This would reflect a fact that calculating changes needed in DNS records are often not as simple as just comparing if all properties are equal, and also can be dependent on provider.

[Raffo]

Thank you for the detailed comments @sheerun , those are much appreciated. I generally agree with you, the idea that the plan can be generic was probably naive. I'll do more thinking on that during day time and update with a comment. The reason why I opened the PR in this state is exactly because I wanted to have those discussions and it's great if we can make ExternalDNS better structured, easier to maintain and to extend.

[sheerun]

If I were to redesign external-dns from scratch there would be per-provider Change type with common interface for logging, where each provider-specific Change instance maps 1:1 to API call of given provider (for easy testing). Also sources would have streaming interface (i.e. they would only return changed endpoints). The main loop would be something like:

config := ReadConfig()
source, providers := InstantiateSourceAndProviders(config)

for { 
	// source.GetChangedEndpoints blocks until there is an update or context is cancelled
	// endpoints are just changed endpoints, if endpoint is removed then endpoint.targets is empty
	endpoints := source.GetChangedEndpoints(ctx)
	for _, provider := range providers {
		// provider returns needed changes, possibly updating internal "current" state
		// also probably ignoring most of changes because domain is not handled by provider
		changes := provider.GetChanges(ctx, endpoints)
		logger.LogChanges(changes)
		provider.ApplyChanges(ctx, changes)
	}
}

This eliminates a lot of complexity, configuration, and reduces time spent on unnecessary actions (e.g. fetching dns records of domains that are not subject of changes, re-calculating needed changes). It's probably too late for per-provider changes, but a version of above could be implemented now is:

1. Make all sources to return only changed endpoints (easy, but not really necessary)
2. Make provider.Records() private for each provider (or just don't call it anywhere outside). This method is no longer necessary to be implemented.
3. Introduce provider.GetChanges(endpoints) that returns current Changes type (with common implementation in BaseProvider.GetChanges)
4. Get rid of plan.go and managing "current" records outside of providers

[linki]

Can you shed some light on the significance of the comparator here? It's the same test conditions and output as the "custom property value changed" test so it seems the comparator doesn't matter?

[sheerun]

It's duplication. It should be removed as in #1849

[sheerun]

After applying all suggested changes by me and linki, I think this PR will look the same as #1849 but with NormalizeDesiredEndpoint name replaced with AdjustEndpoints (the only difference is that NormalizeDesiredEndpoint works on single endpoint while AdjustEndpoints works on all endpoints, which is OK by me).

[sheerun]

Maybe AdjustEndpoints could return endpoints, so some could be removed or added by it on top of some being changed (e.g. changing multiple A records with the same value to one A and multiple aliases).

[Raffo]

Oh that makes sense, good point! I'll make the change.

[Raffo]

I think actually that the signature is fine as it is: we are giving the provider as much freedom as needed on the array, so they can change it as needed, removing and adding, no need to return the endpoints.

[sheerun]

Slices in Go are not mutable, so adding and removing is not possible without returning new array

[Raffo]

Oh right 🤦

[Raffo]

@sheerun after a chat with @linki and @njuettner we decided to go with smaller PRs instead of shipping a super big refactor. This should fix #1820 as is which and open up for future improvements to the rest of the codebase. I think we shouldn't touch the shouldUpdateProviderSpecific method further to keep things small and shippable. We can then think of rebasing #1860 once this is merged such that we can have that fix integrated as well.
If we do that, we can then proceed with an official release of those fixes before the Christmas holiday.

[sheerun]

ok

[Raffo]

@linki @njuettner please review.

[njuettner]

one small nit, I think we can merge it afterwards 👍

[Raffo]

Accepted your suggestion @njuettner

[njuettner]

/lgtm

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

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

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 [Raffo,njuettner]

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