fixes auto_join for mDNS provider #25080
Conversation
|
|
biazmoreira
added
bug
|
Hi @biazmoreira, I've recently encountered this problem when i try to use the mdns as the auto discovery mechanism as well, it seems that this PR would resolve this issue. I was hoping that this will get merged at a reasonable time. |
vault/raft.go
Outdated
| } else if count > 0 && !strings.HasSuffix(ip, "]") { | ||
| tmpIp, portStr, err := net.SplitHostPort(ip) | ||
| if err == nil { | ||
| ip = tmpIp | ||
| tmpPort, err := strconv.Atoi(portStr) | ||
| if err == nil { | ||
| port = uint(tmpPort) | ||
| } | ||
| } |
There was a problem hiding this comment.
@aubrich would you be able to add unit testing for this? it might be interesting to isolate this in a function and unit test the function. this would give us more confidence we're not introducing bugs to the codebase.
There was a problem hiding this comment.
As you suggested I wrote a dedicated function and associated unit tests. I did not write test cases where disco.Addrs gives us something which is not an address.
There was a problem hiding this comment.
Hi there! This looks good to me, and I'd love to approve it. I've left a few comments, and we'd also need a changelog describing concisely what's being fixed. There's some examples in the changelog directory but it'd be a file called 25080.txt that follows the same format as the other files in that directory (there should be a lot of inspiration, but if you want a hand let me know!)
I'm sorry this lingered so long, but if you're still interested in getting this in I'd love to help. I'm out of office tomorrow but will be back afterwards, and I'd love to help you get this over the line.
vault/raft.go
Outdated
| @@ -1459,3 +1458,19 @@ func newDiscover() (*discover.Discover, error) { | |||
| discover.WithProviders(providers), | |||
| ) | |||
| } | |||
|
|
|||
| // formatAddr join ip and port if addr does not already contain a port | |||
There was a problem hiding this comment.
| // formatAddr join ip and port if addr does not already contain a port | |
| // formatDiscoveredAddr joins ip and port if addr does not already contain a port |
| "testing" | ||
| ) | ||
|
|
||
| func TestFormatDiscoveredAddr(t *testing.T) { |
There was a problem hiding this comment.
Would be great if we could get a godoc for this function! Something starting with // TestFormatDiscoveredAddr and with a brief description of what it does
| port uint | ||
| res string | ||
| } | ||
| cases := []TestCase{ |
There was a problem hiding this comment.
This is mostly a question out of curiosity as opposed to something you should change, but out of interest: which of these would have failed with the old implementation?
There was a problem hiding this comment.
All cases where the port is returned with the address failed before. For example with this case 192.168.137.1:8201 vault would try to connect to 192.168.137.1:8201:8200
|
@VioletHynes sorry for the delay I was sick. Here are the modifications you requested. I hope the godoc and changelog are good enough, English is not my native language. |
Fix issue with go-discover mDNS provider and auto-join (#25066)