feat(coredns): use txt-owner-id to strictly separated external-dns instances

[farodin91]

What does it do ?

It adds a field to the service value in etcd to keep track of which service is provided by which coredns.

Motivation

Replacement of #5860. This keeps the behavior inside of the coredns provider.

More

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

[k8s-ci-robot]

Hi @farodin91. Thanks for your PR.

I'm waiting for a github.com 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-sigs/prow repository.

[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 mloiseleur for approval. For more information see the Code Review Process.

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

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

[farodin91]

@ivankatliarchuk I moved to behavior inside the provider only. It would possible to built this with a etcd proxy, that would require much more work with only a little gain. Going for advanced multi cluster solution is a different step.

[ivankatliarchuk]

What is missing with this PR. It does not describe well enough the problem, either a configuration that we could execute locally and test it. I'd assume coredns may require a specific configuration as well.

Records managed by external-dns, not by coredns. For external dns we already have an owner, and adding flag managed-by on top of that, does not make sense.

As well as flag --coredns-strict-managed-by, are we planning to add flag to enable another flag? It just ads up more complexity.

We don't have any sort of specific roadmap or product vision around multi-cluster .External-dns currently don't handle multi cluster, due to complexity, but not strictly against that. This sounds more like has to be resolved with cluster mesh or coredns multicluster plugin. Due to nature, as external-dns writes data to external etcd, is not as difficult to add multi cluster support.

For other reviewers, the PR adds a field to records in etcd. Something like

Kubernetes manifests

---
apiVersion: v1
kind: Service
metadata:
  name: a
  annotations:
    external-dns.alpha.kubernetes.io/hostname: a.example.org
    external-dns.alpha.kubernetes.io/coredns-group: "g1"
    cluster-name: "cluster1"
  namespace: default
spec:
  type: LoadBalancer
  ports:
  - port: 80
    name: http
    targetPort: 80
  selector:
    app: test-app
---
apiVersion: v1
kind: Service
metadata:
  name: a-cluster2
  annotations:
    external-dns.alpha.kubernetes.io/hostname: a.example.org
    external-dns.alpha.kubernetes.io/coredns-group: "g1"
    cluster-name: "cluster2"
  namespace: default
spec:
  type: LoadBalancer
  ports:
  - port: 80
    name: http
    targetPort: 80
  selector:
    app: test-app

before, the second service will not get created, as there is a clash - same host, same values. We could swap the group from g1 to g2 and it will work

/skydns/org/example/a-a/575b6e02
{"text":"\"heritage=external-dns,external-dns/owner=cluster2,external-dns/resource=service/default/a-cluster2\"","targetstrip":1,}
/skydns/org/example/a/22d5d6f3
{"host":"127.0.0.1","targetstrip":1,"group":"g1"}

with the change

/skydns/org/example/a-a/3ce5ad20
{"text":"\"heritage=external-dns,external-dns/owner=cluster1,external-dns/resource=service/default/a\"","targetstrip":1,"managedby":"cluster1"}
/skydns/org/example/a-a/5276c4e8
{"text":"\"heritage=external-dns,external-dns/owner=cluster2,external-dns/resource=service/default/a-cluster2\"","targetstrip":1,"managedby":"cluster2"}
/skydns/org/example/a-a/575b6e02
{"text":"\"heritage=external-dns,external-dns/owner=cluster2,external-dns/resource=service/default/a-cluster2\"","targetstrip":1,"managedby":"cluster2"}
/skydns/org/example/a/22d5d6f3
{"host":"127.0.0.1","targetstrip":1,"group":"g1","managedby":"cluster1"}
/skydns/org/example/a/315642d0
{"host":"127.0.12.1","targetstrip":1,"group":"g1","managedby":"cluster2"}

[farodin91]

@ivankatliarchuk I added another Commit use txt-owner-id instead. Do you think this way is better?

[ivankatliarchuk]

/ok-to-test

Have you tried in your environment? Could you share similar results for this PR #5085 (comment). Need to make sure it works before we merge, I'll try to smoke test right after as well.

[farodin91]

@ivankatliarchuk I've tried the setup in our staging env with managed by variant. As we had issues without such a filter.

The test should be fixed with my next push.

[coveralls]

Pull Request Test Coverage Report for Build 18900103742

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

• For more information on this, see Tracking coverage changes with pull request builds.
• To avoid this issue with future PRs, see these Recommended CI Configurations.
• For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

• 0 of 0 changed or added relevant lines in 0 files are covered.
• 38 unchanged lines in 4 files lost coverage.
• Overall coverage decreased (-0.004%) to 78.662%

---

Files with Coverage Reduction	New Missed Lines	%
openshift_route.go	1	79.49%
apis/externaldns/types.go	1	99.71%
zz_generated.deepcopy.go	5	0.0%
coredns/coredns.go	31	90.33%

Totals
Change from base Build 18774334562:	-0.004%
Covered Lines:	15900
Relevant Lines:	20213

---

💛 - Coveralls

[farodin91]

@ivankatliarchuk tests are fixed.