Add MachineAudit support

[jwreagor]

Ref: #25

Support the MachineAudit endpoint in CloudAPI.

[kwilczynski]

Hi @cheapRoc, I am working on this and a Pull Request would be coming. But, before I open one, I have a question about a design of the client API concerning handling nested objects.

As per: https://apidocs.joyent.com/cloudapi/#MachineAudit; the results include caller object which might or might not have certain fields set. I was thinking how to handle this so that the experience of using the client is consistent - client already handles nested objects (e.g. metadata, etc.) as either a map of a strings or it returns a nested struct.

Therefore, we have these two possible options:

1. Use a map (of strings) for the caller object inside the Audit struct:

type Audit struct {
	Action  string            `json:"action"`
	Success string            `json:"success"`
	Time    time.Time         `json:"time"`
	Caller  map[string]string `json:"caller"`
}

2. Use a struct to capture details of the caller object, and then nest it inside Audit struct:

(Note: the type field is always set; others might be empty/unset)

type AuditCaller struct {
	Type  string `json:"type"`
	User  string `json:"user,omitempty"`
	IP    string `json:"ip,omitempty"`
	KeyID string `json:"keyId,omitempty"`
}

type Audit struct {
	Action  string      `json:"action"`
	Success string      `json:"success"`
	Time    time.Time   `json:"time"`
	Caller  AuditCaller `json:"caller"`
}

In the case of using a map as per (1), the fields which are not set in the response are not going to be present in the resulting map, for example:

map[string]string{"type":"operator"}

But, in the case of using nested struct, the missing fields would be set to their respective zero-values (this is a side-effect of using nested structs when de-marshalling JSON), for example:

compute.AuditCaller{Type:"operator", User:"", IP:"", KeyID:""}

This happens regardless of the JSON attribute omitempty being set for these fields.

We don't seem to be using string pointers (like for example the AWS SDK for Go), albeit it would have a very similar result as the zero-value for a pointer in Go is nil, thus for example:

compute.AuditCaller{Type:"operator", User:<nil>, IP:<nil>, KeyID:<nil>}

The one obvious side effect of using string pointers, would be more work for the user using the client, who would have to make provisions to check for nil in order not to accidentally using read from it or try to do something with it (as it might result in a panic).

Therefore, my question here would be - what would be the best way to handle nested objects in the response? Especially, since there are already different approaches to handling such things in the code.

[jwreagor]

I'd like to hear @sean-'s input as well but I'd prefer nested structs and leveraging zero values. I'm "ok" with empty strings as default values only because they're generally expected by a majority of Go consumers. We have had consumer issues in the past with zero values but I believe those cases were due to using them where they weren't expected before. Does that make sense?

Try to keep note and maybe open a new issue if we're doing things different across triton-go today. We're already consuming this library across a number of tools so we don't want to make radical changes, however, we're in a holding pattern for pre-1.0 release so it's possible now would be the time to make said changes.

[jwreagor]

Also to add, this should be similar to how we handle machine CNS tags (here)

[sean-]

@cheapRoc Yes, definitely nested structs over map[string]interface{} or anything similar. This is one of those weird situations where from a convenience perspective, I'm conflicted about having the zero value of an empty string or a pointer to a string. It's more work up front to have a *string, but that optional semantic is really useful a lot of times. Because this is auditing, I'm in favor of the more pedantic *string and would sacrifice the usability. I would be 100% okay with a helper function, however, that presents an idiomatic interface:

type AuditCaller struct {
	Type  string `json:"type"`
	RawUser  *string `json:"user,omitempty"`
	RawIP    *string `json:"ip,omitempty"`
	RawKeyID *string `json:"keyId,omitempty"`
}

func (ac *AuditCaller) IP() (string, bool) {
  if ac.RawIP == nil {
    return "", false
  }
  return *ac.RawIP, true
}

Doing that repeatedly at the call sites is irritating, at best. For instance, this forces the implementation onto the caller:

if ac.IP == nil {
  return foo
}
return fmt.Sprintf("something %s", *ac.IP)

Because we're inclined to change and improve things in the future, I'd even go so far as to consider:

// auditCaller is the struct that we decode into before foisting the values into AuditCaller
type auditCaller struct {
	Type  string `json:"type"`
	User  *string `json:"user,omitempty"`
	IP    *string `json:"ip,omitempty"`
	KeyID *string `json:"keyId,omitempty"`
}

// AuditCaller represents the interface of 
type AuditCaller struct {
	raw auditCaller
}

// IP returns the IP of the caller.  If the IP was not set the second argument will be false.
func (ac *AuditCaller) IP() (string, bool) {
  if ac.raw.IP == nil {
    return "", false
  }

  return *ac.raw.IP, true
}

And again, if this weren't an exported API, I wouldn't care so much about adding the abstraction, but since it is, I'm erring on the side of decoupling the library (which we own and control) from the consuming applications (which we don't). We loose something when we round-trip an AuditCaller back to json with this approach so we'd want a helper method that would snag raw and return the result.

wtb friend keyword? Anyone? self: demerit

[kwilczynski]

Hi @cheapRoc and @sean-,

Thank you for taking the time to give me some guidance!

I really like the idea proposed by @sean-, and if nobody minds, then I will use this approach.

[sean-]

@kwilczynski SGTM, let us know!