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.

Sign up for GitHub

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

Jump to bottom

Implement multiple public IP resolvers #1344

Merged
merged 1 commit into from
May 21, 2021
Merged

Implement multiple public IP resolvers #1344

merged 1 commit into from
May 21, 2021

Conversation

mangelajo
Copy link
Contributor

@mangelajo mangelajo commented May 19, 2021 •
edited
Loading

The gateway nodes now can be labeled as:

gateway.submariner.io/public-ip=<resolver>[,resolver..]

where resolvers take the form of <method>:<parameter>

the implemented methods are:

  • api
  • lb
  • ipv4
  • dns

Fixes-Issue: #1317
Related-Issue: #1310
Related-Issue: #1071

Adds multiple API fallbacks:

In addition to https://api.ipify.org
- https://api.my-ip.io/ip
- https://ip4.seeip.org

Signed-off-by: Miguel Angel Ajo [email protected]

@submariner-bot
Copy link
Contributor

🤖 Created branch: z_pr1344/mangelajo/public-ip-methods

@mangelajo mangelajo self-assigned this May 19, 2021
skitt
skitt requested changes May 20, 2021
View reviewed changes
Copy link
Member

@skitt skitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor grammar issue; more importantly, since this means we no longer need go-ipify, please run go mod tidy to remove it from go.mod (and go.sum).

pkg/endpoint/public_ip_test.go Outdated Show resolved Hide resolved
@skitt skitt mentioned this pull request May 20, 2021
Merged
tpantelis
tpantelis requested changes May 20, 2021
View reviewed changes
pkg/endpoint/local_endpoint_suite_test.go Outdated Show resolved Hide resolved
pkg/endpoint/local_endpoint_test.go Outdated Show resolved Hide resolved
pkg/endpoint/public_ip.go Outdated Show resolved Hide resolved
pkg/endpoint/public_ip.go Outdated Show resolved Hide resolved
pkg/endpoint/public_ip.go Outdated Show resolved Hide resolved
pkg/endpoint/public_ip_test.go Outdated Show resolved Hide resolved
pkg/endpoint/public_ip_test.go Show resolved Hide resolved
pkg/apis/submariner.io/v1/types.go Outdated Show resolved Hide resolved
pkg/apis/submariner.io/v1/types.go Outdated Show resolved Hide resolved
pkg/apis/submariner.io/v1/types.go Outdated Show resolved Hide resolved
mangelajo
mangelajo commented May 20, 2021
View reviewed changes
Copy link
Contributor Author

@mangelajo mangelajo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review, I handled the comments and extended the tests.

pkg/apis/submariner.io/v1/types.go Outdated Show resolved Hide resolved
pkg/apis/submariner.io/v1/types.go Outdated Show resolved Hide resolved
pkg/apis/submariner.io/v1/types.go Outdated Show resolved Hide resolved
pkg/endpoint/local_endpoint_test.go Outdated Show resolved Hide resolved
pkg/endpoint/public_ip.go Outdated Show resolved Hide resolved
pkg/endpoint/public_ip.go Outdated Show resolved Hide resolved
pkg/endpoint/public_ip_test.go Outdated Show resolved Hide resolved
@mangelajo mangelajo requested review from skitt and tpantelis May 20, 2021 15:22
skitt
skitt requested changes May 20, 2021
View reviewed changes
Copy link
Member

@skitt skitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still needs go mod tidy ;-).

@tpantelis tpantelis mentioned this pull request May 20, 2021
Closed
@mangelajo
Copy link
Contributor Author

This still needs go mod tidy ;-).

@skitt oops, right!

@mangelajo mangelajo requested a review from skitt May 20, 2021 16:18
@mangelajo mangelajo added the backport This change requires a backport to eligible release branches label May 20, 2021
@mangelajo mangelajo added this to the 0.10-m1 milestone May 20, 2021
@mangelajo mangelajo added the ready-to-test When a PR is ready for full E2E testing label May 20, 2021
@mangelajo mangelajo force-pushed the public-ip-methods branch 2 times, most recently from acdd491 to 0c47009 Compare May 20, 2021 16:31
tpantelis
tpantelis reviewed May 20, 2021
View reviewed changes
pkg/endpoint/public_ip_test.go Outdated
Expect(net.ParseIP(ip)).NotTo(BeNil())
})
})

Copy link
Contributor

@tpantelis tpantelis May 20, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure we should test these specific sites - we may get intermittent failures. The main thing is to test the algorithm. We could start a local HTTP server for the test (on localhost:<some port>) and use that URL. If we want to verify these specific sites for validity as defaults then we can add a periodic test. But I don't think we should gate unrelated PRs if these sites get flaky.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we really should. Probably unit test is not the best place to test it.
But we definitely should test them periodically to make sure that the defaults we offer to customers are functional and healthy.

I can remove those tests if we agree to enable some periodic check for those resolvers in CI

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree we need to test the remote resolvers, not to do their monitoring for them, but just to know that they are functional as you say. However their failure should never create difficulty in evaluating or merging a PR. So a periodic test is fine for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your feedback, I agree it will be better to periodically monitor. I will create a separate PR for that, some github action + bash should suffice.

@mangelajo mangelajo requested a review from tpantelis May 21, 2021 09:30
@mangelajo
Implement multiple public IP resolvers
387e917 
The gateway nodes now can be labeled as:

gateway.submariner.io/public-ip=<resolver>[,resolver..]
where resolvers take the form of <method>:<parameter>

the implemented methods are:

- api
- lb
- ipv4
- dns

this commit also adds additional fallback API public-ip resolvers

In addition to https://api.ipify.org
- https://api.my-ip.io/ip
- https://ip4.seeip.org

Signed-off-by: Miguel Angel Ajo <[email protected]>
@mangelajo
Copy link
Contributor Author

last push to update the license headers, and squashed into a single commit for easier backport.

@mangelajo mangelajo requested a review from skitt May 21, 2021 11:16
@mangelajo
Copy link
Contributor Author

@skitt sorry about that, while handling the markdown details I updated the patch according to the new public IP resolvers too,
see the new table at the start and let me know if it looks ok.

@mangelajo
Copy link
Contributor Author

@skitt I'm an idiot, the comment and review re-request was really for: https://github.com/submariner-io/submariner-website/pull/485/files#diff-a9163e1cec4d91ef9323f3e06e5ad35552e68c096b78f5e5635607580c48e5a1R20

If you can, please re-add your vote, no changes here.

@tpantelis tpantelis merged commit ca6d02e into submariner-io:devel May 21, 2021
@submariner-bot
Copy link
Contributor

🤖 Closed branches: [z_pr1344/mangelajo/public-ip-methods]

@mangelajo
Copy link
Contributor Author

Thank you @tpantelis & @skitt

@nyechiel nyechiel linked an issue May 24, 2021 that may be closed by this pull request
If ipify.org is down, submariner-gateway is unable to start #1317
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tpantelis tpantelis tpantelis approved these changes

@Oats87 Oats87 Awaiting requested review from Oats87 Oats87 is a code owner

@skitt skitt Awaiting requested review from skitt skitt is a code owner

@sridhargaddam sridhargaddam Awaiting requested review from sridhargaddam sridhargaddam is a code owner

@dfarrell07 dfarrell07 Awaiting requested review from dfarrell07 dfarrell07 is a code owner

Assignees

@mangelajo mangelajo

Labels
backport This change requires a backport to eligible release branches backport-handled ready-to-test When a PR is ready for full E2E testing
Projects
None yet
Milestone
0.10-m1
Development

Successfully merging this pull request may close these issues.

If ipify.org is down, submariner-gateway is unable to start
4 participants
@mangelajo @submariner-bot @skitt @tpantelis