Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New features: migrate txt-owner #2466

Closed
wants to merge 2 commits into from

Conversation

Inorysky
Copy link

@Inorysky Inorysky commented Dec 7, 2021

Description

When changing --txt-owner-id on an existing external-dns resource, it does not update the existing TXT records it owns, therefore losing ownership. Meaning that we have to manually delete the records in order to have external-dns take ownership again.
To solve this problem, I added the ability to update the original txt-owner by setting -- migrate-txt-owner to overwrite the old txt-owner.
I have successfully modified thousands of pieces of data using this code, so far without any bugs.

Fixes #ISSUE
#2036

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Dec 7, 2021

CLA Signed

The committers are authorized under a signed CLA.

@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Dec 7, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Inorysky
To complete the pull request process, please assign raffo after the PR has been reviewed.
You can assign the PR to them by writing /assign @raffo in a comment when ready.

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@Inorysky Inorysky changed the title New features: set migrate txt-owner New features: migrate txt-owner Dec 7, 2021
@k8s-triage-robot
Copy link

Unknown CLA label state. Rechecking for CLA labels.

Send feedback to sig-contributor-experience at kubernetes/community.

/check-cla

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 7, 2021
@jgrumboe
Copy link
Contributor

jgrumboe commented Dec 10, 2021

@Inorysky
The idea of having such a switch is great!
As far as I understand the code, it will overwrite any txt-owner it finds in DNS records. I believe that there are many users outside running multiple external-dns deployments in several clusters but each of them operating probably within the same DNS zone. There would be clashes if one of them does a txt-owner migration!
You could extend your current code with a from-txt-owner (holding the old txt-owner value) to play it safe.

@Inorysky
Copy link
Author

@Inorysky The idea of having such a switch is great! As far as I understand the code, it will overwrite any txt-owner it finds in DNS records. I believe that there are many users outside running multiple external-dns deployments in several clusters but each of them operating probably within the same DNS zone. There would be clashes if one of them does a txt-owner migration! You could extend your current code with a from-txt-owner (holding the old txt-owner value) to play it safe.

Thank you for your suggestion, I have now made a change according to your suggestion: the from-txt-owner tag has been added to support modifying the old txt-owner specified. :)

@jgrumboe
Copy link
Contributor

@Inorysky I like it 👍 Thanks.
I'm not a maintainer thus you need to wait for others to review this PR.

@Inorysky
Copy link
Author

@Inorysky I like it 👍 Thanks. I'm not a maintainer thus you need to wait for others to review this PR.

Thank you very much!(・ω・)ノ

@runzexia
Copy link

cc @Raffo, could help review this? we adopt this feature in 30 of our clusters, it helps a lot

@darkpixel
Copy link

@runzexia Same here. I have about 400 domains that need to be updated. Doing it by hand would be a huge pain in the rear. ;)

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 19, 2022
@k8s-ci-robot
Copy link
Contributor

@Inorysky: 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.

@sebastiangaiser
Copy link

+1

@mkilchhofer
Copy link

We at @swisspost would really apreciate this feature since we are already in such a scenario where migration of the owner is needed.

@CarpathianUA
Copy link

Also waiting for this MR to be merged!

@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle stale
  • Mark this issue or PR as rotten with /lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 2, 2023
@mkilchhofer
Copy link

Not stale. 😢

We @swisspost still wait for this feature.

@seanmalloy
Copy link
Member

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 3, 2023
@erimerdal
Copy link

Wish this was merged

@mkilchhofer
Copy link

mkilchhofer commented Jan 19, 2023

@seanmalloy @njuettner @Raffo @szuecs How can we push this further?

@rikatz
Copy link
Contributor

rikatz commented Jan 19, 2023

This PR needs:

  1. A rebase
  2. some unit tests

I suggest pinging someone on #external-dns on kubernetes slack, so they can provide some feedback on it :)

Thanks!

@szuecs
Copy link
Contributor

szuecs commented Jan 23, 2023

@rikatz we are seeing this here, too. I see all GH notifications, but if the PR is not green I won't review, because use the time elsewhere is more efficient for the project.

@rikatz
Copy link
Contributor

rikatz commented Jan 23, 2023

Yes agreed. Let's wait some return of the author here

@ashuec90
Copy link

Hi took this PR changes and built docker image and used that, i could see those migrate-txt-owner --from-txt-owner option , but still it is skipping the updation of record saying owner id does not match, found: default and resquired: abc,

can you confirm if this works, or am i missing something,?

i ran the external-dns with below options

external-dns --dry-run --metrics-address=":7979" --log-level=debug --log-format=text --interval=30m --source=service --source=ingress --source=istio-virtualservice --source=istio-gateway --policy=upsert-only --registry=txt --domain-filter=example.com --provider=aws --aws-assume-role=arn:aws:iam::1234567890:role/route53role --txt-owner-id=abc --from-txt-owner=default --migrate-txt-owner

cc: @Inorysky

@k8s-triage-robot
Copy link

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/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was 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

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 24, 2023
@mkilchhofer
Copy link

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 24, 2023
@mkilchhofer
Copy link

mkilchhofer commented May 25, 2023

FYI: I tried to take over the work from here / @Inorysky and filed a new PR over there:

Rebased @Inorysky 's work and added some unit tests.

@mloiseleur
Copy link
Contributor

Since #3631 supersede this PR, I'm closing it.
/close

@k8s-ci-robot
Copy link
Contributor

@mloiseleur: Closed this PR.

In response to this:

Since #3631 supersede this PR, I'm closing it.
/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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.