fix(targetfiltersource): skip network address filtering for non-address records

[castorw]

What does it do ?

This PR update target filter source to skip target network filtering via --target-net-filter and/or --exclude-target-net on non-address records (A/AAAA) as there is no reason to apply IPv4/IPv6 filters to non-address records.

Motivation

When controller is configured to manage address and non-address record types (eg. A, AAAA, CNAME) it fails to manage CNAME records when --target-net-filter and/or --exclude-target-net is configured. Applying IP-based filtering to non-IP resources does not make sense and limits the usability of controller. A messy workaround is to launch multiple instances of external-dns, however this does not seem like a proper solution.

More

• Yes, this PR title follows Conventional Commits
• Yes, I added unit tests
• Yes, I updated end user documentation accordingly

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign ivankatliarchuk for approval. For more information see the Code Review Process.

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

Details

Needs approval from an approver in each of these files:

• OWNERS

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

[k8s-ci-robot]

Hi @castorw. 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.

Details

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-sigs/prow repository.

[ivankatliarchuk]

/ok-to-test

[coveralls]

Coverage Report for CI Build 24407478739

Coverage increased (+0.003%) to 80.527%

Details

• Coverage increased (+0.003%) from the base build.
• Patch coverage: No coverable lines changed in this PR.
• No coverage regressions found.

Uncovered Changes

No uncovered changes found.

Coverage Regressions

No coverage regressions found.

---

Coverage Stats

Relevant Lines:	21312
Covered Lines:	17162
Line Coverage:	80.53%
Coverage Strength:	1471.1 hits per line

---

💛 - Coveralls

[ivankatliarchuk]

Not sure about this proposal. Worth to search git history, to get a better idea why it was implemented that way #2693

What is stopping in future to filter targets like cnames, txt records, SRV, MX, NS, PTR and etc etc.

There is scope concern. The proposal most likely silently breaks future wrapper target filter implementations.

And non-IP targets are outside the scope of network filtering, not wrapper target filtering.

To be specific, the fix should live in TargetNetFilter.Match() itself rather than the wrapper. The filter knows it only speaks IP - the wrapper shouldn't need to know that.

 func (tf TargetNetFilter) Match(target string) bool {
      ip := net.ParseIP(target)
      if ip == nil {
          return true // non-IP targets are outside the scope of network filtering
      }
      return matchTargetNetFilter(tf.filterNets, ip, true) &&
          !matchTargetNetFilter(tf.excludeNets, ip, false)
  }

This way:

• The semantics are correct at the source - a net filter simply doesn't apply to hostnames
• The wrapper stays generic and works correctly for any future TargetFilterInterface implementation (including ones that legitimately filter CNAME targets by pattern)
• The record-type guard in the wrapper PR is unnecessary and can be removed
• Tests for the fix belong in endpoint/target_filter_test.go, not in the wrapper tests

The wrapper test case in the PR is still valid as an integration test (net filter + mixed record types), but the record-type bypass in wrapper is most likely a no go

[ivankatliarchuk]

A messy workaround is to launch multiple instances of external-dns, however this does not seem like a proper solution.

^ This is not a messy workaround, the PR assumes that sharing one instance across record types with conflicting filter semantics is the desired end state, but that assumption ins't correct. The controller per configuration is an architectural stance and is the model that is actually cleaner and safer operationally - each instance has a single, coherent responsibility with no ambiguous interactions between flags.

A CNAME target and an A record target serve different purposes and may legitimately need different filtering rules - forcing them through one filter pipeline is the actual networking topology design smell.

[castorw]

You are right about the semantics, the placement of filtering exclusion wasn't too good in the source wrapper. I have moved this to target_filter.go. As the TargetNetFilter is currently the only implementation of TargetFilterInterface I have added *Endpoint as additional argument to Match func so the filter is aware of the endpoint context.

The solution you proposed to check whether net.ParseIP produces an IP address would allow non-IP targets and invalid IP addresses to pass through the filter (eg. 271.10.30.40). So I rather implemented a record type check in TargetNetFilter.Match function.

---

^ This is not a messy workaround, the PR assumes that sharing one instance across record types with conflicting filter semantics is the desired end state, but that assumption ins't correct. The controller per configuration is an architectural stance and is the model that is actually cleaner and safer operationally - each instance has a single, coherent responsibility with no ambiguous interactions between flags.

A CNAME target and an A record target serve different purposes and may legitimately need different filtering rules - forcing them through one filter pipeline is the actual networking topology design smell.

Cannot say I would completely disagree with this statement, however when I see that a network address filter (--target-net-filter and/or --exclude-target-net) is touching completely irrelevant targets, it seems illogical and I actually spent time figuring out what the problem might be - ended up crawling sources and creating this PR. There may not be many users requiring/using address-based filtering, but there may be some and they may wonder why IP filter is cutting off their NS, MX, TXT, CNAME or other record types which have nothing to do with network addresses.

[ivankatliarchuk]

You most likely correct, and initial implementation was lot more simpler.

Current Match(target string, ep *Endpoint) bool -> couples the filter abstraction to Endpoint, which is probably we should avoid doing. The endpoint holds targets ;-), and we passing endpoint and targets for same endpoint to the filters method.

[castorw]

Well I thought about passing only record type over to Match function, however if we want to keep it open for other filter implementations (which may also use other endpoint fields) I thought having all endpoint data could be useful. I understand it seems nasty to be passing a single target and the object holding them all, but I considered it to be better option.

What would you suggest then? Pass only record type to Match or revert it to original implementation (which I don't like anymore after you pointed out it wasn't placed very well)?