[WIP] add ability to skip TLS verification in http client

[jfchevrette]

This adds the ability to skip TLS verification either when using NewSession or the client builder.

This is useful when connecting to a zabbix server that has a self signed certificate.

[cavaliercoder]

Thanks for submitting this. This feature totally makes sense.

I might recommend though, that it would be more flexible to allow users to configure the HTTP client and transport in any way they please (e.g. timeout and proxy settings, etc. Rather than only exposing TLS skip.

What if instead:

type Session struct {
    ...
    client http.Client
}

func (c *Session) HTTPClient() *http.Client {
    if c.client == nil {
        c.client = &http.Client{}
    }
    return c.client
}

This way, users can configure the client and underlying transport however they please.

[jfchevrette]

@cavaliercoder Thanks for the feedback! I've updated the PR so that a http.Client can be passed to either NewSession as an extra 4th parameter or using .WithHTTPClient(client) when using the client builder.

Ideally I would have liked to avoid adding a new paramater to NewSession as that breaks the API, but since NewSession itself does 2 API calls, I had to make sure the client was set as early as possible. Another option would have to move the 2 API calls (version and login) to their own separate functions but both options break the current API/behavior.

WDYT?

[cavaliercoder]

Nice! I agree concerning the API breakage.

I think you could move all the session login code into a new private method Session.login. Then you can call this from NewSession so the API stays the same, but ClientBuilder can create a Session object without logging in. ClientBuilder can call Session.login later in ClientBuilder.Connect.

[jfchevrette]

@cavaliercoder moving out the login code into a new private method wouldn't help, we would still need to setup the client inside of NewSession prior to calling Session.login from NewSession.

I think the only way to not have to change the NewSession API would be to move the login code out of the NewSession function but require the user to use Session.Login from their code prior to any other API calls. Ex:

session := zabbix.NewSession("http://zabbix/api_jsonrpc.php", "admin", "zabbix")
if ok = session.Login(); !ok {
    // handle failure to login
}

Change here is that NewSession would only create and return the session struct w/o doing any API calls. This would allow the user to then to session.SetHTTPClient(client) before any API calls.

Another thought I had would be to replace the NewSession function parameters with a ZabbixConfig struct which will contain the URL, Username and Password as fields and also a client which can be tweaked as desired. This seem to be the more common way to do these things in go, especially when the API/parameters may evolve over time. But then again that would be break the NewSession API.

In any case, I don't see a way to avoid breaking the NewSession API because of this catch-22 between having to set the client early in NewSession and moving out the Login code into a separate function but then users have to call it themselves after NewSession and optionally after setting the session client.

[cavaliercoder]

I think the goal here is that NewSession API remains unchanged, but is constrained to simple sessions. If users want control over caching, HTTP client, etc. they should use ClientBuilder.

Check this out:

func NewSession(url string, username string, password string) (session *Session, err error) {
    // create session
    session = &Session{URL: url}
    err = session.login(username, password)
    return
}

...

func (c *Session) login(username, password string) error {
    // get Zabbix API version
    res, err := session.Do(NewRequest("apiinfo.version", nil))
    ...
}

...

func (builder *ClientBuilder) Connect() (session *Session, err error) {
    ...
    // Otherwise - login to a Zabbix server
    session = &Session{url: builder.url, client: builder.client}
    err = session.login(builder.credentials....)
    ...
}

func (c *Session) Do(req *Request) (resp *Response, err error) {
    ...
    // send request
    client := c.client
    if client == nil {
        client = http.DefaultClient
    }
    res, err := client.Do(r)
    ...

[cavaliercoder]

If we can fix this minor issue, I'm keen to merge! Scratch what I said earlier about session.HTTPClient.

[cavaliercoder]

You may also wish to update README so users have an example of how to skip TLS with your feature!

[jfchevrette]

Your suggestion makes a ton of sense. Thanks!

I've taken the liberty to initialize the default http.Client in NewSession rather than checking if it's nil on every invocation of session.Do.

I have also added an example to the docs as suggested.

[jfchevrette]

I've had issues with my branch so I had to re-do a bunch of changes.

• Implemented your suggestions as per https://github.com/cavaliercoder/go-zabbix/pull/14#issuecomment-426779793
• Move version retrieval login to GetVersion, otherwise when using client builder the session.APIVersion was nil - updated doc and readme examples
• Updated readme with http.Client example
• When restoring from cached session, the client was set to nil. Added a line to set the client upon session restore.

Please review again, sorry for the troubles. I can squash if needed.

[cavaliercoder]

Cool! Nice work. Merged!

[jfchevrette]

Thanks for merging and thank you for your patience :1st_place_medal: