feat: add new flags to allow migration of OwnerID #4823
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
Welcome @troll-os! |
k8s-ci-robot
added
the
needs-ok-to-test
|
Hi @troll-os. 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 Once the patch is verified, the new status will be reflected by the 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
added
the
size/L
troll-os
changed the title
|
Thanks @troll-os. It looks good. |
|
@mloiseleur Sure ! Where do you think it would fit the best? TXT registry ? |
|
With this PR, it works only with TXT registry, so yes. |
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
|
/lgtm /assign @Raffo |
k8s-ci-robot
added
the
lgtm
| if im.isMigrationEnabled { | ||
| filteredChanges.UpdateOld = append(filteredChanges.UpdateOld, endpoint.FilterEndpointsByOwnerID(im.oldOwnerID, changes.UpdateOld)...) | ||
| filteredChanges.Delete = append(filteredChanges.Delete, endpoint.FilterEndpointsByOwnerID(im.oldOwnerID, changes.Delete)...) | ||
| } |
There was a problem hiding this comment.
Can you explain the purpose of those lines?
There was a problem hiding this comment.
We want to apply the changes which should include the deletion of the old entries in the registry, otherwise we're left we unmanaged and unwanted records
|
@troll-os answered to your comments. Could you please also provide a way to test this manually with manifests and kubectl commands? I would be eager to verify myself that the migration works and then keep an end to end test around. |
k8s-ci-robot
removed
the
lgtm
|
New changes are detected. LGTM label has been removed. |
k8s-ci-robot
added
size/XL
troll-os
force-pushed
the
migration-owner
branch
from
b5673a8 to
08dd8b4
Compare
k8s-ci-robot
added
size/L
|
So I had to make a change for the OVH provider to ensure record targets are properly formatted, I ran another batch of test to migrate from the Meaning that we couldn't map the record ID and the deletion would fail I don't know why this happens on a specific zones, the other zones covered seem fine... When I make direct calls to the OVH API the formatting is good... I didn't forget your query for manifests to tests the changes @Raffo, I'll make them ASAP |
…y are properly formatted for all operations, fixed regexp usage
|
So I did a little of homework and made this work with Gandi with a domain I own for personal use and minikube Steps used to reproduce on another setup than my work dev and prod environments :
apiVersion: apps/v1
kind: Deployment
metadata:
name: nginx
spec:
replicas: 1
selector:
matchLabels:
app: nginx
template:
metadata:
labels:
app: nginx
spec:
containers:
- image: nginx
name: nginx
ports:
- containerPort: 80
---
apiVersion: v1
kind: Service
metadata:
name: nginx
annotations:
external-dns.alpha.kubernetes.io/hostname: helloworld.whatever.tld
spec:
selector:
app: nginx
type: LoadBalancer
ports:
- protocol: TCP
port: 80
targetPort: 80
---
apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
name: nginx
spec:
ingressClassName: nginx
rules:
- host: helloworld.whatever.tld
http:
paths:
- path: /
pathType: Prefix
backend:
service:
name: nginx
port:
number: 80
apiVersion: apps/v1
kind: Deployment
metadata:
name: external-dns
spec:
replicas: 1
selector:
matchLabels:
app: external-dns
strategy:
type: Recreate
template:
metadata:
labels:
app: external-dns
spec:
containers:
- name: external-dns
image: yourRegistry.com/external-dns:v-someBuildNumber # <<<<<<<<<<<<
args:
- "--txt-prefix=%{record_type}-"
- "--txt-cache-interval=15m"
- "--log-level=info"
- "--log-format=text"
- "--txt-owner-id=old-owner"
- "--policy=sync"
- "--provider=youDecide" # <<<<<<<<<<<<<<<<
- "--registry=txt"
- "--interval=1m"
- "--domain-filter=whatever.tld"
- "--source=ingress"
env:
- name: whatever_env_var_provider_needs
value: "hello"
apiVersion: apps/v1
kind: Deployment
metadata:
name: external-dns
spec:
replicas: 1
selector:
matchLabels:
app: external-dns
strategy:
type: Recreate
template:
metadata:
labels:
app: external-dns
spec:
containers:
- name: external-dns
image: yourRegistry.com/external-dns:v-someBuildNumber # <<<<<<<<<<<<
args:
- "--txt-prefix=%{record_type}-"
- "--txt-cache-interval=15m"
- "--log-level=info"
- "--log-format=text"
- "--txt-owner-id=new-owner"
- "--from-txt-owner=old-owner"
- "--migrate-txt-owner"
- "--policy=sync"
- "--provider=youDecide" # <<<<<<<<<<<<
- "--registry=txt"
- "--interval=1m"
- "--domain-filter=whatever.tld"
- "--source=ingress"
env:
- name: whatever_env_var_provider_needs
value: "hello"
|
|
Also I tried to follow the CoreDNS setup from the external-dns docs to have a non "external" provider dependent way to tests but didn't manage to make it work for some reason and I didn't want to spend too much time on it (probably a minikube issue) knowing that it worked with a classic provider |
|
Hello Hope you guys had a great end to 2024 and best wishes for 2025 :) Do you know if you'll be able to test this we the information I provided ? We've used this code in prod on our side without issues for OVH and confirmed it works for Gandi, in theory should be the same for others aswell. I might be able to test on Azure DNS soon |
k8s-ci-robot
added
the
needs-rebase
|
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-sigs/prow repository. |
There was a problem hiding this comment.
I appreciate that the proposed changes are predominantly the work of others; however it shouldn't be necessary to modify anything above the registry.TXTRegistry level (i.e. neither controller.Controller nor plan.Plan).
The registry system acts as a wrapper around various provider implementations. The TXTRegistry works by generating DNS TXT records describing the set of Labels associated with a given Endpoint and injecting any mutations required to create/update/delete those DNS TXT records as part of the ApplyChanges(…) processing. Any such DNS TXT records are subsequently filtered during the retrieval of DNS records from the provider, via the Records(…) function, and the information that they convey (i.e. the Labels) merged into the corresponding Endpoint object describing the real (i.e. non-TXT) DNS record being managed. Therefore migration of a Label value (e.g. Owner) from an old value to a new value should be achievable entirely within the TXTRegistry implementation.
The required processing would involve identifying DNS TXT records describing ownership associated with any provided old owner value and treating them as if they had been associated with the new/current owner during the filtering stage and then ensuring that any DNS TXT records associated with the old owner value are replaced by DNS TXT records associated with the new/current owner value during the generation stage.
There is perhaps also a case for providing wider support for migrating the configuration of the TXTRegistry; for example, also supporting the migration of txt-prefix and txt-suffix configurations.
| @@ -199,6 +199,10 @@ type Controller struct { | |||
| ExcludeRecordTypes []string | |||
| // MinEventSyncInterval is used as window for batching events | |||
| MinEventSyncInterval time.Duration | |||
| // migrate txt-owner flag | |||
| TXTOwnerMigrate bool | |||
| // old txt-owner whitch needed to modify | |||
There was a problem hiding this comment.
| // old txt-owner whitch needed to modify | |
| // old txt-owner from which to migrate |
There was a problem hiding this comment.
These changes appear to be unrelated to the "TXT registry owner migration" functionality. Can they be separated into another pull-request?
| // migrate txt-owner flag | ||
| TXTOwnerMigrate bool |
There was a problem hiding this comment.
The state represented by TXTOwnerMigrate appears to be equivalent to len(TXTOwnerOld) != 0; what's the benefit of requiring an additional flag?
|
@Dadeos-Menlo: changing LGTM is restricted to collaborators In response to this:
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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@troll-os Do you think you can address review comments from @Dadeos-Menlo ? |
|
Hi @mloiseleur Yes I noted his feedback, I didn't find time to work on this yet, but definitely not set aside Are there any deadlines on your side that I should take into account ? |
|
@troll-os nope, just wanting to check if you were still here and did see the comments |
Description
This pull requests aims to complete the work initiated in these PR : #2466 and #3631
Most credits go to the creators of the initial work, as I mostly just rebased their work on the current state of master
Initial PR description:
As for me I've successfully managed records without any issue using the newly added flags on the OVH provider
If anyone is willing to try these changes on other providers I'd be happy to troubleshoot their issues
Regarding the documentation I'm not sure either in which category these changes are relevant, so taking any recommendations on that
Note: this is my first contribution to such a project and first time using Go aswell, I've went through the contribution guidelines but if I missed something please let me know so that I can fix it
Fixes #2036
Checklist