external-dns
external-dns copied to clipboard
kubernetes-sigs
Rework provider/designate
Description
This PR is a larger rework of provider/designate to address a couple of related issues.
- Fix
policy=sync#1122 - Fix txt record migration https://github.com/kubernetes-sigs/external-dns/issues/2790#issuecomment-1211719494
- Add support for multi-target Endpoints
- Remove custom Labels on Endpoints
- Add cache to store data between
.Recodsand.ApplyChanges - Increase test coverage
- Decouple test cases
- Improve logging
The mentioned issues were caused by using endpoint.Labels to pass information between .Records and .ApplyChanges, which is kind of wrong. Also ProviderSpecific seems to be the wrong place, because TXT records are filtered and explicitly generated for each Endpoint. Result is, in .ApplyChanges there is no relation between the new TXT Endpoints and the original ones. I took inspiration from other providers that need to pass information between those methods. They all re-fetch needed data in .ApplyChanges, so my changes are doing that as well. Maybe in the future, we (as in external-dns community) can find a way to pass data around, but for now the described approach works sufficient enough.
Fixes #3023
Checklist
- [x] Unit tests updated
- [x] End user documentation updated
Note: no changes to the documentation needed. provider/designate works the same as before.
[APPROVALNOTIFIER] This PR is NOT APPROVED
This pull-request has been approved by: chrigl, hikhvar
Once this PR has been reviewed and has the lgtm label, please assign seanmalloy for approval by writing /assign @seanmalloy in a comment. For more information see:The Kubernetes Code Review Process.
The full list of commands accepted by this bot can be found here.
Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment
I think we should merge this first. After that I will rebase https://github.com/kubernetes-sigs/external-dns/pull/3049 based on the master. I solved also the problems which are introduced by the custom labels on the endpoints. My solution is kind of a hack, this one has solved the root of the problem.
@njuettner @seanmalloy can you please review this PR? I think this solves many problems within the designate provider.
This is a great initiative, thanks for all the work put in here! :clap:
I've followed the plugin provider work that's being done in https://github.com/kubernetes-sigs/external-dns/pull/3063. I personally think that we still need these things being addressed here now since the Designate provider has had a bunch of problems from time to time. But is there a point in waiting and maybe help out on that one first?
Somewhat related to the rework done in this PR, we've had problems exactly like the one described here: https://github.com/kubernetes-sigs/external-dns/issues/1837. To me it looks like that the Designate provider never picks that TTL annotation value up, or even checking if it's set using e.g. TTL.IsConfigured() helper func as other providers do. Maybe that would be in-scope here also given the list of PRs being addressed.
thanks for the PR we will not review/merge any new providers. We are working on a plugin provider which will enable you to integrate via webhook. All or most providers will be moved out of tree after plugin https://github.com/kubernetes-sigs/external-dns/pull/3063 provider was created and we are sure that we can run a major provider out of tree.
@szuecs I quite like your push for out-of-tree providers. But frankly there then needs to be an externally maintained designate provider which does not exist yet, or does it? Until then bugfixes run stale and effort to fix things is lost.
So my question is about the path for Designate for the new out-of-tree design to then be the repo / PR target for little fixes like https://github.com/kubernetes-sigs/external-dns/pull/3049
@frittentheke I approved and lgtm the other PR. It was small enough to review quickly.
PR needs rebase.
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.
@frittentheke I approved and lgtm the other PR. It was small enough to review quickly.
Thanks a lot @szuecs. More than I could have (actually did) asked for.
As I am quite keen on having a well working Designate provider and I don't want to overly stress your patience with me. May I ask if there are migration plans for moving the existing plugins out of tree already and how they look like time-wise and technically?
I understand you don't want to merge in new providers. But while this PR is a big rework, it is intended to be a bugfix PR to an existing in-tree provider. @chrigl invested time finding and fixing architectural issues. Would it be such a burden to review and apply these fixes to the provider in-tree first before externalizing it later? I suppose the business logic towards the external DNS service (and thus any bugfixes) likely will remains even for the new design.
@frittentheke I and others don't have the time to review provider XXL PRs anymore, that's why we enable you to have providers out of tree, so we can focus on the remaining code.
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.
This bot triages PRs according to the following rules:
- After 90d of inactivity,
lifecycle/staleis applied - After 30d of inactivity since
lifecycle/stalewas applied,lifecycle/rottenis applied - After 30d of inactivity since
lifecycle/rottenwas applied, the PR is closed
You can:
- Mark this PR as fresh with
/remove-lifecycle stale - Close this PR with
/close - Offer to help out with Issue Triage
Please send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle stale
@chrigl: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:
| Test name | Commit | Details | Required | Rerun command |
|---|---|---|---|---|
| pull-external-dns-licensecheck | fb7625ff331885577d2a43ed1166ad4a7a59cbfc | link | true | /test pull-external-dns-licensecheck |
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.
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.
Now that webhook provider is available in public version of external-dns, I think this PR can be closed.
Feel free to create a webhook provider on designate.
/close
@mloiseleur: Closed this PR.
In response to this:
Now that webhook provider is available in public version of
external-dns, I think this PR can be closed. Feel free to create a webhook provider ondesignate. /close
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.
@chrigl would you consider converting (I know I am trivializing the task) your already reworked implementation to a webhook (https://github.com/kubernetes-sigs/external-dns/pull/3063) provider for OpenStack Designate?
The list (https://github.com/kubernetes-sigs/external-dns#new-providers) is growing and this would then allow for decoupled and out-of-tree development
I would also greatly appreciate a webhook port of this provider. Would be a shame for this work to go to waste.
