Don't ignore error while doing startup check

[tanner-bruce]

Error is checked for if it is nil in Client#startupHealthcheck, but not for if the error actually contains an error.

Fixes #625 and will likely give a better message for basic auth failures as well

This code:

package main

import (
	"log"
	"os"

	"gopkg.in/olivere/elastic.v5"
)

func main() {
	host := os.Getenv("ELS_HOST")
	user := os.Getenv("ELS_USER")
	pass := os.Getenv("ELS_PASS")

	_, err := elastic.NewClient(
		elastic.SetURL(host),
		elastic.SetSniff(false),
		elastic.SetBasicAuth(user, pass),
	)
	if err != nil {
		log.Fatalf("could no create elastic client to %q: %v", host, err.Error())
	}
}

and this Dockerfile

FROM bitnami/minideb
#RUN install_packages ca-certificates
ADD elastic ./
CMD ["./elastic"]

showcase the difference this PR makes.

e.g

Instead of receiving the message

health check timeout: no Elasticsearch node available

on client creation, we will now receive [in the case where the underlying system has no ca-certificates]

Head https://domain.net: x509: failed to load system roots and no roots provided

which will be nice for UX when debugging issues.

[tanner-bruce]

One thing to note, the general approach of this is worth a discussion, as well as how it affects testing, due to how it affects multiple urls being passed to the startup healthcheck. A possible solution would be to aggregate any errors in the for, and return that.

At that point we'd likely need a separate Error type, a 'soft' error of sorts that essentially says 'one of your urls was broken, but the others work'

Once that is figured out I can fix up the tests/approach as needed.

[olivere]

@tanner-bruce Better errors codes are very welcome, of course. In #510 we introduced github.com/pkg/errors in the code base with errors.Wrap / errors.Wrapf to add context to an error. I think we should do that here as well.

Regarding your last comment: I agree that errors could be improved even more. But your code will enhance error messages, so let's just start with that.

[olivere]

@tanner-bruce This PR is not the correct solution to pass error information back to the caller. A health check checks if any node in the list is available. Your PR makes health check to return with an error if any node in the list is unavailable.

I'm working on a different solution to this.

[olivere]

This commit should now print a more helpful error message like health check timeout: Head http://localhost:9299: dial tcp 127.0.0.1:9299: getsockopt: connection refused: no Elasticsearch node available. It prints the last error found while trying to connect to any of the nodes. It is available in v5 and v6.

[tanner-bruce]

Hi @olivere, I mentioned mine wasn't correct and was looking for feedback on the best way to move forward. I actually already wrote nearly the same code as your commit, I just wasn't sure how you wanted to deal with the case of >1 ES endpoint where one of them fails to connect.

Thank you for doing it though, this is much appreciated and should help out folks a lot! The lack of ca-certificates is a pretty common error when running in Docker.