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

Auto-join support for IPv6 discovery #12366

Merged
merged 2 commits into from
Sep 7, 2021

Conversation

dekimsey
Copy link
Collaborator

The go-discover library returns IP addresses and not URLs. It just so
happens net.URL parses "127.0.0.1", which isn't a valid URL.

Instead, we construct the URL ourselves. Being careful to check if it's
an ipv6 address and making sure it's in explicit form if so.

Fixes #12323

@vercel vercel bot temporarily deployed to Preview – vault-storybook August 19, 2021 14:51 Inactive
@vercel vercel bot temporarily deployed to Preview – vault August 19, 2021 14:51 Inactive
The go-discover library returns IP addresses and not URLs. It just so
happens net.URL parses "127.0.0.1", which isn't a valid URL.

Instead, we construct the URL ourselves. Being careful to check if it's
an ipv6 address and making sure it's in explicit form if so.

Fixes hashicorp#12323
@dekimsey dekimsey force-pushed the 12323-autojoin-ipv6-support branch from e9de18a to 8351e5c Compare August 19, 2021 14:56
@vercel vercel bot temporarily deployed to Preview – vault August 19, 2021 14:56 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook August 19, 2021 14:56 Inactive
@pmmukh
Copy link
Contributor

pmmukh commented Sep 3, 2021

The PR looks pretty great! One thing, could you maybe try to add a test similar to this https://github.com/hashicorp/vault/blob/main/vault/external_tests/raft/raft_test.go#L129, that does the same but for ipv6 addresses ? It's probably gonna be fairly tricky, I can't exactly tell how much, but a test to show this change works would be nice i think.

@dekimsey
Copy link
Collaborator Author

dekimsey commented Sep 3, 2021

I would be happy to try. I wanted to add a test when I wrote the PR up, but I couldn't figure out how to do that. I didn't see this area of the codebase though, so I'll give it a whirl!

@pmmukh
Copy link
Contributor

pmmukh commented Sep 3, 2021

sounds great, and thanks!

@ncabatoff
Copy link
Collaborator

The test TestRaft_RetryAutoJoin is of questionable value, because it doesn't actually exercise autojoin. In fact, if you remove the AutoJoin field from the LeaderJoinInfo struct, or mangle it to be invalid go-discover syntax, the test doesn't fail, so I'm kind of inclined to remove it if we can't improve it.

Sadly we don't currently have a good way of running our automated CI tests with access to AWS, so it may well be that there's no good way to test this change other than manually.

@pmmukh
Copy link
Contributor

pmmukh commented Sep 3, 2021

aha gotcha, thanks for the clarification @ncabatoff ! In that case, please feel free to ignore the request for a test addition @dekimsey , once I've validated the fix works on my end, will merge this in.

vault/raft.go Outdated Show resolved Hide resolved
vault/raft.go Outdated Show resolved Hide resolved
vault/raft.go Outdated Show resolved Hide resolved
Rename addrs to clusterIPs to improve clarity and intent

Tighten up our IPv6 address detection to be more correct and to ensure
it's actually in implicit form
@vercel vercel bot temporarily deployed to Preview – vault September 3, 2021 18:37 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook September 3, 2021 18:37 Inactive
@pmmukh pmmukh merged commit e79b352 into hashicorp:main Sep 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

storage raft retry_join does not support public_v6, incorrectly parses IP addresses as URLs
3 participants