`ociclient.Options` does not make clear that `HTTPClient` and `Authorizer` are actually exclusive

[tianon]

The godoc for ociclient.Options does not make it clear that it will use either HTTPClient or Authorizer to make requests, and will not use both:

https://github.com/cue-labs/oci/blob/5ebe80b0a9a67ae83802d1fb1a189a8f0d089fb0/ociregistry/ociclient/client.go#L278-L282

I found this while I was trying to debug what actually turned out to be #27 (which consequently was a bug my own auto-retrying HTTPDoer implementation had too! :joy:)

This is complicated a bit by the fact that ociauth.Authorizer.DoRequest does not have any affordance for receiving an HTTPClient/"HTTPDoer".

In my case, I'm just using ociauth.StdAuthorizer, so I adjusted my code to set Authorizer after it's decided whether or not we need a custom HTTPClient so I can copy it into ociauth.StdAuthorizerParams.HTTPClient, which appears to have the desired effect, but I think it's probably a more general problem that might need either API changes or documentation changes so that the "gotcha" here is more clear. :sweat_smile:

Perhaps something like http.Request.WithContext or http.Request.Clone could be used to inject the scope value into the req.Context() such that Authorizer could be deprecated entirely in favor of just being an HTTPDoer interface? :eyes:

[rogpeppe]

Thanks for raising this issue. Docs (and possibly the API) definitely need fixing here.

Perhaps something like http.Request.WithContext or http.Request.Clone could be used to inject the scope value into the req.Context() such that Authorizer could be deprecated entirely in favor of just being an HTTPDoer interface

I thought quite a bit about that possibility. The thing I found awkward is that in that case we'd actually want two scopes in the context: the required scope and the scope to be used when acquiring new auth tokens. I think it's worth keeping them separate, because if there's an existing token that has sufficient scope, we should avoid the round trip to get a new one, but if we need to acquire a new one, we want to acquire it with the full scope passed down by the caller.

We could have an extra context key in the context that holds the required context, and also we could try to deduce the required scope from the HTTP request, but neither seem ideal to me. Given that an authorizer must know about that key, it seemed to me that it was nicer just to pass it down explicitly; deducing the scope from the HTTP request ends up doing a bunch of logic just to work out something that is already trivially known by the caller.

To my mind, the current situation (with a bit more documentation) seems like it's probably the best of a slightly awkward situation. WDYT?

[rogpeppe]

Fixed by https://review.gerrithub.io/c/cue-labs/oci/+/1177730.

[tianon]

Thank you! 😄