coredns: support IPv6 record set

[pmichali]

Added support for AAAA record for coredns and included unit test.
Refactored common test code to reduce duplication from added test and
existing tests.
Fixed function names in comments for Google and AWS tests to match
actual test name in this area.

What this PR does / why we need it:

Adding IPv6 support to kubernetes, once piece at a time. :)

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #44351

Special notes for your reviewer:
In addition to the change and unit test method, I did some minor refactoring (since the UT was a near clone of an existing test). Fixed typos in related test methods' comment lines. Please let me know if this is OK (I was thinking it was a small change, but don't know the protocol here), or if I need to break it into multiple commits.

Release note:

[k8s-ci-robot]

Hi @pmichali. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with @k8s-bot 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.

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.

[k8s-reviewable]

This change is 

[grodrigues3]

@k8s-bot ok to test

[ghost]

Do we not want to test ip.To6 here, instead of defaulting to ipv6?

[pmichali]

Thanks for the review comment. I was thinking it wasn't necessary, because it looks like ParseIP() will either eval the string to an IP (v4 or v6) or return nil. As a result, I figured that there are cases for nil and IPv4, so the value would have to be Ipv6, if those two cases didn't match. Is that acceptable?

[pmichali]

Should I:

• Leave it as-is
• Add a comment indicating the default is for IPv6
• Add a case with test for To6(), and then have default case with comment indicating it should never occur?

[pmichali]

Will take a guess at option #2, adding a comment, so that this can move forward.

[pmichali]

Any feedback on the latest change set?

[pmichali]

Change is ready for review - please.

[shashidharatd]

Reviewed 4 of 5 files at r1, 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

federation/pkg/dnsprovider/providers/coredns/coredns_test.go, line 265 at r2 (raw file):

/* TestResourceRecordSetsWithMixedTypes  verifies that we can add records with different types.
   Note: Can not run CommonTestResourceRecordSetsDifferentTypes for coredns, because ResoureRecordSets.List()
   is not implemented, and Get() API will aggregate rrdata (IPs) for rrsets with the same name, causing

This comment may not be completely true after this PR #44626. In that PR @madhusudancs has changed Get() to return multiple rrsets for a name with different types. PTAL and update.

---

federation/pkg/dnsprovider/tests/commontests.go, line 92 at r2 (raw file):

CommonTestResourceRecordSetsWithMixedTypes
The name is ambiguous, gives a feeling to me that the test is for checking multiple record types can be written for the same name. But actually the test is just writing different record types with different names. is this test required at all?

---

Comments from Reviewable

[pmichali]

Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

federation/pkg/dnsprovider/providers/coredns/coredns_test.go, line 265 at r2 (raw file):

Previously, shashidharatd (Shashidhara T D) wrote…

This comment may not be completely true after this PR #44626. In that PR @madhusudancs has changed Get() to return multiple rrsets for a name with different types. PTAL and update.

Thanks for the update about the other commit. I'll rebase to pick up the changes and update this commit.

---

federation/pkg/dnsprovider/tests/commontests.go, line 92 at r2 (raw file):

Previously, shashidharatd (Shashidhara T D) wrote…

CommonTestResourceRecordSetsWithMixedTypes
The name is ambiguous, gives a feeling to me that the test is for checking multiple record types can be written for the same name. But actually the test is just writing different record types with different names. is this test required at all?

It's been a few weeks since I wrote this, and the PR #44626 may change things, so I'll revisit it. Originally, I wanted to use the CommonTestResourceRecordSetsDifferentTypes test, which had IPv4 and IPv6 IPs for the same name, but coredns didn't support that, so I created this test, using different names.

---

Comments from Reviewable

[pmichali]

Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

federation/pkg/dnsprovider/providers/coredns/rrsets.go, line 78 at r1 (raw file):

Previously, pmichali (Paul Michali) wrote…

Will take a guess at option #2, adding a comment, so that this can move forward.

Done.

---

Comments from Reviewable

[pmichali]

Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

federation/pkg/dnsprovider/providers/coredns/coredns_test.go, line 265 at r2 (raw file):

Previously, pmichali (Paul Michali) wrote…

Thanks for the update about the other commit. I'll rebase to pick up the changes and update this commit.

Pulling this test method, as existing CommonTestResourceRecordSetsDifferentTypes will work now that Get() has been changed.

---

federation/pkg/dnsprovider/tests/commontests.go, line 92 at r2 (raw file):

Previously, pmichali (Paul Michali) wrote…

It's been a few weeks since I wrote this, and the PR #44626 may change things, so I'll revisit it. Originally, I wanted to use the CommonTestResourceRecordSetsDifferentTypes test, which had IPv4 and IPv6 IPs for the same name, but coredns didn't support that, so I created this test, using different names.

Yes, this is no longer needed, as we can use CommonTestResourceRecordSetsDifferentTypes now, because Get() now returns a list, instead of single rrset. Updating.

---

Comments from Reviewable

[pmichali]

Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

federation/pkg/dnsprovider/providers/coredns/coredns_test.go, line 265 at r2 (raw file):

Previously, pmichali (Paul Michali) wrote…

Pulling this test method, as existing CommonTestResourceRecordSetsDifferentTypes will work now that Get() has been changed.

Done.

---

federation/pkg/dnsprovider/tests/commontests.go, line 92 at r2 (raw file):

Previously, pmichali (Paul Michali) wrote…

Yes, this is no longer needed, as we can use CommonTestResourceRecordSetsDifferentTypes now, because Get() now returns a list, instead of single rrset. Updating.

Done.

---

Comments from Reviewable

[pmichali]

Pushed up a new commit, PTAL

[shashidharatd]

/lgtm
Thanks @pmichali !

---

Reviewed 3 of 3 files at r3.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

Comments from Reviewable

[shashidharatd]

@madhusudancs, PTAL and approve this PR. Thanks.

[pmichali]

@shashidharatd thank you for reviewing!

[madhusudancs]

Reviewed 2 of 5 files at r1, 3 of 3 files at r3.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

federation/pkg/dnsprovider/providers/coredns/rrsets.go, line 78 at r1 (raw file):

Previously, pmichali (Paul Michali) wrote…

Done.

I would have preferred 3 for clarity and/or readability, but this is fine for now.

---

federation/pkg/dnsprovider/tests/commontests.go, line 34 at r3 (raw file):

	rrset := rrsets.New("alpha.test.com", []string{"8.8.4.4"}, 40, rrstype.A)
	addRrsetOrFail(t, sets, rrset)
	defer sets.StartChangeset().Remove(rrset).Apply()

This logic is clever and definitely fewer lines of code, but IMO, it obscures what is really happening. It makes comprehending this code difficult. Why is whatever is returned by add is deferred?

I would really urge to revert back to the previous code. Even though, it is verbose, less clever, it is clear.

---

Comments from Reviewable

[pmichali]

Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

federation/pkg/dnsprovider/providers/coredns/rrsets.go, line 78 at r1 (raw file):

Previously, madhusudancs (Madhusudan.C.S) wrote…

I would have preferred 3 for clarity and/or readability, but this is fine for now.

Since I'm changing, I'll modify this.

---

federation/pkg/dnsprovider/tests/commontests.go, line 34 at r3 (raw file):

Previously, madhusudancs (Madhusudan.C.S) wrote…

This logic is clever and definitely fewer lines of code, but IMO, it obscures what is really happening. It makes comprehending this code difficult. Why is whatever is returned by add is deferred?

I would really urge to revert back to the previous code. Even though, it is verbose, less clever, it is clear.

Sure. Will change this back and post an update.

---

Comments from Reviewable

[pmichali]

PTAL I pushed a new version of the changes to revert the changes to addRrsetOrFail(), change the switch statement, and added UT code to check the Zone() method, which was not covered. Rebased this morning (hopefully that is recent enough).

[pmichali]

PTAL as I pushed up a new version that modifies the switch statement, reverts the addRrsetOrFail() changes to not return a method to clean up. Left the logging for success, to reduce some duplication, and added a bit of code to cover the Zone() method for coverage.

---

Review status: 2 of 5 files reviewed at latest revision, 2 unresolved discussions.

---

Comments from Reviewable

[madhusudancs]

Thanks for making these changes!

/lgtm

---

Reviewed 3 of 3 files at r4.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

Comments from Reviewable

[k8s-github-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: madhusudancs, pmichali, shashidharatd

Needs approval from an approver in each of these OWNERS Files:

• federation/OWNERS [madhusudancs]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

[k8s-github-robot]

Automatic merge from submit-queue (batch tested with PRs 45860, 45119, 44525, 45625, 44403)

[pmichali]

@madhusudancs @shashidharatd Thanks for the prompt review and approval!