Update rfc2136 provider to split out changes per zone #4107

gregsidelinger · 2023-12-12T16:17:54Z

Description
Update the rfc2136 provider to update each zone separately as the current implantation causes errors if the change spans zones.

Fixes #4106

Checklist

Unit tests updated
End user documentation updated

linux-foundation-easycla · 2023-12-12T16:17:58Z

The committers listed above are authorized under a signed CLA.

✅ login: gregsidelinger / name: Greg (e44aa65, e3b8e54, 1418a3a, 7427c8e, 35dc1fc, 2b4aa56, 8e9a416)

k8s-ci-robot · 2023-12-12T16:18:04Z

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

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.

Signed-off-by: Greg Sidelinger <[email protected]>

mloiseleur · 2023-12-18T07:59:42Z

This PR needs a test case, to reproduce this issue and ensure the fix will continue to work.

gregsidelinger · 2023-12-18T14:32:18Z

@mloiseleur I have some ideas how to test for this. It will be a few days before I get around to mocking something out but I will update the test cases.

into fix-multizone-rfc2136

Add some tests for rfc2136 multizones Signed-off-by: Greg Sidelinger <[email protected]>

Signed-off-by: Greg Sidelinger <[email protected]>

gregsidelinger · 2023-12-20T14:09:19Z

@mloiseleur I added some different test cases. Most of the test cases I added was around things that here not being tested before. I'm actually testing the test version of SendMessage to make sure it does not receive a multizone change, except for the root zone as I needed to update my change to address that.

mloiseleur · 2023-12-20T14:11:36Z

It's looking good, thanks. I'll wait that you have finished for a complete review.
Would you please also update PR's description (first line) ? :)
/ok-to-test

gregsidelinger · 2023-12-20T15:10:22Z

I think I figured out why the test suite is failing. The order of the createMsgs and other objects updated by the stubbed version of SendMessage is not fixed as I loop over map which is not an ordered object. Will get the suite updated shortly to address.

… zones Signed-off-by: Greg Sidelinger <[email protected]>

Signed-off-by: Greg Sidelinger <[email protected]>

gregsidelinger · 2024-01-02T13:26:18Z

@mloiseleur how is the PR looking now that I extended the test suite?

mloiseleur · 2024-01-02T13:45:16Z

provider/rfc2136/rfc2136.go

+			if len(z.Ns) > 0 {
+				err := r.actions.SendMessage(z)
+				if err != nil {
+					log.Errorf("RFC2136 update failed: %v", err)


Suggested change

log.Errorf("RFC2136 update failed: %v", err)

log.Errorf("RFC2136 create record failed: %v", err)

mloiseleur · 2024-01-02T13:45:32Z

provider/rfc2136/rfc2136.go

+			if len(z.Ns) > 0 {
+				err := r.actions.SendMessage(z)
+				if err != nil {
+					log.Errorf("RFC2136 update failed: %v", err)


Suggested change

log.Errorf("RFC2136 update failed: %v", err)

log.Errorf("RFC2136 delete record failed: %v", err)

mloiseleur · 2024-01-02T13:48:08Z

provider/rfc2136/rfc2136.go

+			if len(z.Ns) > 0 {
+				err := r.actions.SendMessage(z)
+				if err != nil {
+					log.Errorf("RFC2136 update failed: %v", err)


Suggested change

log.Errorf("RFC2136 update failed: %v", err)

log.Errorf("RFC2136 update record failed: %v", err)

mloiseleur · 2024-01-02T13:51:00Z

provider/rfc2136/rfc2136.go

+				err := r.actions.SendMessage(z)
+				if err != nil {


Suggested change

err := r.actions.SendMessage(z)

if err != nil {

if err := r.actions.SendMessage(z); err != nil {

mloiseleur · 2024-01-02T13:51:23Z

provider/rfc2136/rfc2136.go

+				err := r.actions.SendMessage(z)
+				if err != nil {


Suggested change

err := r.actions.SendMessage(z)

if err != nil {

if err := r.actions.SendMessage(z); err != nil {

mloiseleur · 2024-01-02T13:51:40Z

provider/rfc2136/rfc2136.go

+				err := r.actions.SendMessage(z)
+				if err != nil {


Suggested change

err := r.actions.SendMessage(z)

if err != nil {

if err := r.actions.SendMessage(z); err != nil {

mloiseleur · 2024-01-02T13:52:51Z

It looks good to me. I have just minor suggestion.

Signed-off-by: Greg Sidelinger <[email protected]>

gregsidelinger · 2024-01-02T21:02:52Z

Ok, suggestions are in there as well as one other log msg update to make them all consistent. And the linter check finally passed after getting kicked.

mloiseleur · 2024-01-03T08:07:08Z

/lgtm

szuecs · 2024-01-05T18:30:12Z

/approve

k8s-ci-robot · 2024-01-05T18:30:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: szuecs

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [szuecs]

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 added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Dec 12, 2023

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Dec 12, 2023

k8s-ci-robot requested review from johngmyers and szuecs December 12, 2023 16:18

Update rfc2136 provider to send zone updates one zone at a time

e44aa65

Signed-off-by: Greg Sidelinger <[email protected]>

gregsidelinger force-pushed the fix-multizone-rfc2136 branch from 450a2c2 to e44aa65 Compare December 12, 2023 17:41

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Dec 12, 2023

gregsidelinger added 2 commits December 19, 2023 17:15

Merge branch 'master' of https://github.com/kubernetes-sigs/external-dns

e3b8e54

into fix-multizone-rfc2136

Update the rfc2136 mutlizone fix to support the default root zone

1418a3a

Add some tests for rfc2136 multizones Signed-off-by: Greg Sidelinger <[email protected]>

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 20, 2023

Only log zone once for the SendMessage

7427c8e

Signed-off-by: Greg Sidelinger <[email protected]>

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Dec 20, 2023

gregsidelinger added 2 commits December 20, 2023 16:26

Update rfc2136 test suite to deal with ording when dealing with multi…

35dc1fc

… zones Signed-off-by: Greg Sidelinger <[email protected]>

Cleanup extra whitespace to make the linter happy

2b4aa56

Signed-off-by: Greg Sidelinger <[email protected]>

mloiseleur reviewed Jan 2, 2024

View reviewed changes

Cleanup error msgs

8e9a416

Signed-off-by: Greg Sidelinger <[email protected]>

k8s-ci-robot assigned mloiseleur Jan 3, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 3, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 5, 2024

k8s-ci-robot merged commit 7bb79a3 into kubernetes-sigs:master Jan 5, 2024
14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update rfc2136 provider to split out changes per zone #4107

Update rfc2136 provider to split out changes per zone #4107

gregsidelinger commented Dec 12, 2023 •

edited

Loading

linux-foundation-easycla bot commented Dec 12, 2023 •

edited

Loading

k8s-ci-robot commented Dec 12, 2023

mloiseleur commented Dec 18, 2023

gregsidelinger commented Dec 18, 2023

gregsidelinger commented Dec 20, 2023

mloiseleur commented Dec 20, 2023

gregsidelinger commented Dec 20, 2023

gregsidelinger commented Jan 2, 2024

mloiseleur Jan 2, 2024 •

edited

Loading

mloiseleur Jan 2, 2024

mloiseleur Jan 2, 2024

mloiseleur Jan 2, 2024

mloiseleur Jan 2, 2024

mloiseleur Jan 2, 2024

mloiseleur commented Jan 2, 2024

gregsidelinger commented Jan 2, 2024

mloiseleur commented Jan 3, 2024

szuecs commented Jan 5, 2024

k8s-ci-robot commented Jan 5, 2024

	log.Errorf("RFC2136 update failed: %v", err)
	log.Errorf("RFC2136 create record failed: %v", err)

	log.Errorf("RFC2136 update failed: %v", err)
	log.Errorf("RFC2136 delete record failed: %v", err)

	log.Errorf("RFC2136 update failed: %v", err)
	log.Errorf("RFC2136 update record failed: %v", err)

	err := r.actions.SendMessage(z)
	if err != nil {
	if err := r.actions.SendMessage(z); err != nil {

Update rfc2136 provider to split out changes per zone #4107

Update rfc2136 provider to split out changes per zone #4107

Conversation

gregsidelinger commented Dec 12, 2023 • edited Loading

linux-foundation-easycla bot commented Dec 12, 2023 • edited Loading

k8s-ci-robot commented Dec 12, 2023

mloiseleur commented Dec 18, 2023

gregsidelinger commented Dec 18, 2023

gregsidelinger commented Dec 20, 2023

mloiseleur commented Dec 20, 2023

gregsidelinger commented Dec 20, 2023

gregsidelinger commented Jan 2, 2024

mloiseleur Jan 2, 2024 • edited Loading

Choose a reason for hiding this comment

mloiseleur Jan 2, 2024

Choose a reason for hiding this comment

mloiseleur Jan 2, 2024

Choose a reason for hiding this comment

mloiseleur Jan 2, 2024

Choose a reason for hiding this comment

mloiseleur Jan 2, 2024

Choose a reason for hiding this comment

mloiseleur Jan 2, 2024

Choose a reason for hiding this comment

mloiseleur commented Jan 2, 2024

gregsidelinger commented Jan 2, 2024

mloiseleur commented Jan 3, 2024

szuecs commented Jan 5, 2024

k8s-ci-robot commented Jan 5, 2024

gregsidelinger commented Dec 12, 2023 •

edited

Loading

linux-foundation-easycla bot commented Dec 12, 2023 •

edited

Loading

mloiseleur Jan 2, 2024 •

edited

Loading