division by 0 panic

[eikenb]

If you try to use DNSDiscovery without setting any ServiceUrls and the dns discovery fails, it causes a division by 0 panic, stemming from the choice([]string]string function in the connection go file. It depends on the passed in list not being empty without checking. A simple check in the choice() function would fix the panic.

[itsrainy]

@eikenb what is your use case where you have an empty list of ServiceUrls? Yes, that method will cause a panic when that list is empty, but I'm wondering if it would make more sense to check for that upon creation of the EurekaConnection and return an error then. Is there a legitimate use case for a EurekaConnection without any ServiceUrls?

[ryansb]

Yes, there is a legitimate case for EurekaConnection without explicit ServiceUrls.

In the case of DNS discovery, no service urls get set because those will be discovered via DNS (the DiscoveryDomain settings) so we shouldn't check for that on creation.

The best solution is likely to log and retry on a backoff if there are not ServiceUrls and discovery over DNS fails.

[itsrainy]

:+1: Thanks for weighing in @ryansb. So if dns discovery fails on retries and there are no service urls, the caller is pretty much SOL, right?

[ryansb]

Yup, the question is "what's the failure mode here".

If DNS discovery is failing, that's (likely) an ephemeral failure rather than a misconfiguration, so waiting for it to come back is preferable to crashing. I think the way to go would be to immediately retry, then backoff progressively until you're checking at minute intervals and just spin there.

You can also do different things on misconfiguration. If the query for the discovery domain fails, retrying is the way to go. If it returns an (authoritative) NXDOMAIN then a misconfiguration is most likely, so a fatal response is probably ok.

[ryansb]

cc @tysonstewart would a change like that cover your usage?

[itsrainy]

That should be fine with how we use it. I believe we always specify ServiceUrls explicitly, so a change like that shouldn't affect any of our existing tools.

[ryansb]

I'll work on this later this evening.

[eikenb]

Sorry for missing the discussion but ryansb more than adequately stood in for me. My main point was that code should never panic and a log.Fatal() would have been fine with me, though the backoff/retry setup is be great. Thanks.

[eikenb]

Reviewing the code I don't think this has fully been fixed yet. Currently if the discoverDNS() encounters an error at some point, returning the error with an empty list of servers the process will crash with the division by 0 panic. I suppose this could be a difference in styles, but I don't think panics are good for production code. Even just a log.Fatal() is better as you can explicitly call out what the real problem is.

[ryansb]

See #22