Inititial support for multiple targets per record

[grimmy]

This PR is to add support for multiple targets per #239 . I changed the signature of endpoint.NewEndpoint and endpoint.NewEndpointWithTTL moving target to the end of as a variadic parameter. This caused the size of the PR to balloon, but I figured making is variadic was better than littering the code with []string{"1.2.3.4"} everywhere by changing the target parameter to a string slice.

I only added tests for AWS right now, but it should be relatively straight forward to add the tests to other providers.

[k8s-ci-robot]

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

---

• If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
• If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
• If you have done the above and are still having issues with the CLA being reported as unsigned, please email the CNCF helpdesk: [email protected]

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.

[linki]

/cc @ideahitme @multi-io @dereulenspiegel @sethpollack @jrnt30

[k8s-ci-robot]

@linki: GitHub didn't allow me to request PR reviews from the following users: multi-io, dereulenspiegel, sethpollack, jrnt30.

Note that only kubernetes-incubator members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @ideahitme @multi-io @dereulenspiegel @sethpollack @jrnt30

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.

[linki]

@grimmy would you mind splitting the PR into two with the positional arg switch isolated in its own? It would be easier to see the diff that's relevant to the multi-target functionality.

[grimmy]

They're literally one in the same.. the Only difference is that NewEndpointWithTTL is no longer setting Targets: []string{target}, and instead just using Targets: targets.

[grimmy]

If I were to split this into two pull requests...

The first one would be 19 files changed by changing endpoint.NewEndpoint from (dnsName, target, recordType string) to (dnsName string, targets []string, recordType string), which will require every endpoint.NewEndpoint call in the code base to have it's second parameter changed to []string{"1.2.3.4"} (for example). This also of course affects endpoint.NewEndpointWithTTL as well.

The second pr would also change 19 files as the signature of endpoint.NewEndpoint would change from (dnsName string, targets []string, recordType string) to (dnsName, recordType string, targets ...string) Which would again change ever call to endpoint.NewEndpoint to deal with the arguments changing (again).

If you really want to review double the work, sure I can split them. But you're saying you want smaller PRs. The current PR is the smallest this change can be.

[szuecs]

👍

[szuecs]

@linki please merge

[linki]

This looks wrong: the target IPs aren't variadic at the end of the function. I guess this compiles because they are all strings and variadic at the end?

Sad that the tests don't fail 😒 I just dicussed with @szuecs that we could check in NewEndpoint that the recordType value is a valid endpoint.RecordType* or don't use string but a specific type. Then the tests would fail here.

[grimmy]

What do you mean aren't variadic at the end of the function? Any number of parameters at that point is valid, whether it's 0, 1, 2, 10, 1024, etc.. Perhaps I'm not understanding what you're saying?

Just to be clear, if you're saying that the first record in currentRecords should be a failure according to the endpoint.NewEndpoint's that are visible, they're unrelated (check the record name). Or perhaps github isn't showing the context you expected?

[grimmy]

Whoops, it was just pointed out to me that the error you're talking about is line 406. Sorry about that, totally glossed over it... It must have compile errored for the test and it just got lost in the mess of test output from all the providers being in the same test sweet/package.

[grimmy]

I'm fixing the tests now.

While not the best idea, record type could become an enum implementing the Stringer interface and that should catch all of these. That should be relatively low impact too as everything is already using the const.

[grimmy]

Updated. I used the following to verify correctness

for a line with just endpoint.NewEndpoint on it

$ grep -rn endpoint.NewEndpoint | grep -v WithTTL | awk '($2 ~ /^endpoint\.NewEndpoint\(./) && ($3 !~ /^endpoint\.RecordType/) { print $1 " " $3 " " $4}'

And then manually verified those that had endpoint.NewEndpoint elsewhere in the line with

$ grep -rn endpoint.NewEndpoint | grep -v WithTTL | awk '$2 !~ /^endpoint.NewEndpoint/ { getline; print $0 }'

[grimmy]

The test passing was bugging me so I wrote some tests for endpoint.Targets.Same. They're doing what they should be doing, so I'm very confused why internal.testutils.SameEndpoint is passing. Any thoughts? I can commit/pr my additions to endpoint/endpoint_test.go if you like as well.

[linki]

@grimmy yes, feel free to add your tests 👍

Switching the parameters around in the aforementioned line still doesn't made the tests fail, unfortunately. I tested locally that it doesn't enter the NewEndpoint* functions with a wrong record type by adding the following quick hack to NewEndpointWithTTL:

func NewEndpointWithTTL(dnsName, recordType string, ttl TTL, targets ...string) *Endpoint {
	switch recordType {
	case "", RecordTypeA, RecordTypeTXT, RecordTypeCNAME:
	default:
		panic(recordType)
	}
        ...

When I switch the parameters around in one of the tests this indeed paniced, so I guess the current implementation is correct.

However, to avoid similar issues in the future I would either prefer a cleaner version of the above so tests somehow break or make it not compile by either making the RecordType its own type or if we kept the multiple targets as a []string instead of as variadic arguments. In both cases those misplaced parameters to NewEndpoint* would lead to compilation errors, I believe.

Let me know what you think and if you want to tackle one of the options. Since this PR is now correct I'm also fine with taking it as is.

[k8s-ci-robot]

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

---

• If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
• If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
• If you have done the above and are still having issues with the CLA being reported as unsigned, please email the CNCF helpdesk: [email protected]

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.

[grimmy]

Just pushed the tests for Endpoint.Same. Should be good to go from here. Actually committed with the wrong username, fixing.

Clearly this went very bad, trying to fix it up now :-/

[linki]

@grimmy fixed it, I believe.

[grimmy]

Alright, everything should be good to go now and I've learned my lesson and will never use git filter-branch again :)

[linki]

LGTM, thanks @grimmy