errrorsource: Handle if errors are nil

[aangelisc]

The current implementation of the errorsource logic can lead to nil pointer dereferences.

An example of this can be found here. Any error value returned from the gceDefaultProjectGetter is a valid error. However, the function may also return nil if there is no error.

The DownstreamError and PluginError functions should appropriately handle if the error is nil given that they technically return the error type.

Example issue here.

[andresmgot]

yes, I agree, in my head the idiomatic way of handling errors would be:

result, err := doSomething();
if err != nil {
  // handle error
}
return result, nil

So for the example in the description, it would be:

project, err := s.gceDefaultProjectGetter(ctx, cloudMonitorScope)
if err != nil {
  return "", errorsource.DownstreamError(err, false)
}
return project, nil

[aangelisc]

Thank you for your input @marefr, @andresmgot! It's fine for us to implement this in that way but my reasoning for this logic is that the return type of DownstreamError and PluginError is error and nil is a valid error value.

At the moment the implementation of these functions implies (to me at least) that the value can also be nil and not always an error but that isn't actually the case. I think this could lead to confusion later on when attempting to run a nil check on an error that has been wrapped with a source as that nil check will always pass even if the error is nil.

[andresmgot]

It's true that the errors package supports to pass nil errors so it can be considered idiomatic.

I also understand the problem better now, I thought calling PluginError with nil would panic or something but it's actually creating a populated error as errorsource.Error{source:"plugin", status:0, err:error(nil)}. In this case, I agree it's more correct to properly handle the nil case.