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

Initial IPv6 support #2309

Closed

Conversation

samip5
Copy link
Contributor

@samip5 samip5 commented Sep 17, 2021

Description

This adds initial IPv6 support, and fixes the defaulting to A record issue.

Fixes #2300
Fixes #1812
Fixes #2051
Somewhat related #2044
Somewhat related #1877

Checklist

  • Unit tests updated
  • End user documentation updated

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Sep 17, 2021
@k8s-ci-robot
Copy link
Contributor

Welcome @samip5!

It looks like this is your first PR to kubernetes-sigs/external-dns 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/external-dns has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 17, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: samip5
To complete the pull request process, please assign njuettner after the PR has been reviewed.
You can assign the PR to them by writing /assign @njuettner 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

@samip5
Copy link
Contributor Author

samip5 commented Sep 17, 2021

/kind feature

@k8s-ci-robot k8s-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Sep 17, 2021
@samip5 samip5 marked this pull request as ready for review September 17, 2021 07:00
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 17, 2021
@krish7919
Copy link

Bump? Please check...

source/source.go Outdated Show resolved Hide resolved
@samip5
Copy link
Contributor Author

samip5 commented Oct 2, 2021

/auto-cc

plan/plan.go Outdated Show resolved Hide resolved
source/source.go Show resolved Hide resolved
source/source.go Show resolved Hide resolved
plan/plan_test.go Show resolved Hide resolved
source/service.go Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 7, 2021
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Nov 7, 2021
@johngmyers
Copy link
Contributor

I suggest you squash the commits.

@samip5
Copy link
Contributor Author

samip5 commented Nov 10, 2021

I suggest you squash the commits.

Not possible due to merge commits.

@johngmyers
Copy link
Contributor

I suggest you rebase or do a soft reset.

@johngmyers
Copy link
Contributor

@samip5 Check out your branch. Then run:

git reset --soft c9e0c919
git commit -m "Initial IPv6 support"

Verify the commit has the changes you want, then force push.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Nov 13, 2021

CLA Signed

The committers are authorized under a signed CLA.

@johngmyers
Copy link
Contributor

A number of unit tests are failing. Looking into TestRunOnce, it is creating when it should be updating.

@johngmyers
Copy link
Contributor

Line 121 of plan.go

diff --git a/plan/plan.go b/plan/plan.go
index 1e1cf353..4e421512 100644
--- a/plan/plan.go
+++ b/plan/plan.go
@@ -118,7 +118,7 @@ func (t planTable) addCandidate(e *endpoint.Endpoint) {
        if _, ok := t.rows[dnsName][e.SetIdentifier]; !ok {
                t.rows[dnsName][e.SetIdentifier] = make(map[string]*planTableRow)
        }
-       if _, ok := t.rows[e.SetIdentifier][e.RecordType]; !ok {
+       if _, ok := t.rows[dnsName][e.SetIdentifier][e.RecordType]; !ok {
                t.rows[dnsName][e.SetIdentifier][e.RecordType] = &planTableRow{}
        }
        t.rows[dnsName][e.SetIdentifier][e.RecordType].candidates = append(t.rows[dnsName][e.SetIdentifier][e.RecordType].candidates, e)

@johngmyers
Copy link
Contributor

johngmyers commented Nov 13, 2021

With that change, still 3 failing tests.

TestNewEndpointWithIPv6 needs this change:

diff --git a/endpoint/endpoint_test.go b/endpoint/endpoint_test.go
index 80b7cb04..a4b227bc 100644
--- a/endpoint/endpoint_test.go
+++ b/endpoint/endpoint_test.go
@@ -37,7 +37,7 @@ func TestNewEndpoint(t *testing.T) {
 
 func TestNewEndpointWithIPv6(t *testing.T) {
        e := NewEndpoint("example.org", "AAAA", "foo.com")
-       if e.DNSName != "example.com" || e.Targets[0] != "foo.com" || e.RecordType != "AAAA" {
+       if e.DNSName != "example.org" || e.Targets[0] != "foo.com" || e.RecordType != "AAAA" {
                t.Error("Endpoint is not initialized correctly")
        }
 
@@ -46,7 +46,7 @@ func TestNewEndpointWithIPv6(t *testing.T) {
        }
 
        w := NewEndpoint("example.org", "", "load-balancer.com.")
-       if w.DNSName != "example.org" || e.Targets[0] != "load-balancer.com" || w.RecordType != "" {
+       if w.DNSName != "example.org" || w.Targets[0] != "load-balancer.com" || w.RecordType != "" {
                t.Error("Endpoint is not initialized correctly")
        }
 }

TestDifferentTypes is failing, suggesting that splitting by record type like we're doing might not be correct, or at least we need to look into the wider implications of the change. Maybe the splitting should be done in the Route53 provider?

@johngmyers
Copy link
Contributor

@samip5 I've sent you skyssolutions#3 to fix the unit tests. Could you take a look?

@samip5
Copy link
Contributor Author

samip5 commented Dec 3, 2021

I'm waiting on an developer from external
-dns to review this before continuing.

@johngmyers johngmyers mentioned this pull request Dec 5, 2021
2 tasks
@johngmyers
Copy link
Contributor

In its current state, the code in this PR repeatedly tries to create AAAA records in the provider, even though they're already there. It also doesn't delete AAAA records. It also fails for dual-stack targets when using the TXT registry, as it attempts to create two TXT ownership records for the same domain.

As the author has expressed that they are not continuing to work on this PR at this time, I have filed #2461 to continue the work.

@samip5
Copy link
Contributor Author

samip5 commented Dec 5, 2021

In its current state, the code in this PR repeatedly tries to create AAAA records in the provider, even though they're already there. It also doesn't delete AAAA records. It also fails for dual-stack targets when using the TXT registry, as it attempts to create two TXT ownership records for the same domain.

As the author has expressed that they are not continuing to work on this PR at this time, I have filed #2461 to continue the work.

I only expressed the need for me to see some comment from the actual project maintainers, which I believe you're not one of.

@johngmyers
Copy link
Contributor

@samip5 Thank you for your contribution so far.

The community expectations on code review state that all active participants in the community are expected to be active reviewers. In fact, performing code review is one of the requirements for progressing to be an owner.

A direct consequence of your statement is that you will not be working at this time to resolve the identified defects in this PR. As this is a volunteer organization, that is your choice to make. As you will not be doing this, I choose to take the work forward in a different PR.

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

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

@samip5 samip5 closed this Jan 26, 2022
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. kind/feature Categorizes issue or PR as related to a new feature. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
5 participants