Feedback HTTP service errors to AddCharm RPC call.

Feedback HTTP service errors to AddCharm RPC call.

Remove environment config 'charm-store-auth', to be replaced by per-user
credentials. Helpers to classify and handle HTTP service error conditions.
Enables client to respond to a '401 Unauthorized' by obtaining & adding
credentials, retrying.

https://code.launchpad.net/~cmars/juju-core/cs-httpauth-feedback/+merge/210524

(do not edit description out of merge proposal)

[cmars, 2014-03-12 02:08:33]

Please take a look.

[cmars, 2014-03-12 20:17:20]

Please take a look.

[cmars_, 2014-03-12 20:20:37]

(Merged with trunk)

[fwereade, 2014-03-13 22:57:26]

Bit twitchy about exposing http-implementation details over the API. Thoughts?

https://codereview.appspot.com/74350043/diff/20001/charm/repo.go
File charm/repo.go (right):

https://codereview.appspot.com/74350043/diff/20001/charm/repo.go#newcode124
charm/repo.go:124: return resp, utils.NewHttpServiceError(resp)
I'm nervous that it's really easy to mask this error, but otherthings are
starting to depend on it. (I pontificate more on this general theme later...)

https://codereview.appspot.com/74350043/diff/20001/state/apiserver/client/cli...
File state/apiserver/client/client.go (right):

https://codereview.appspot.com/74350043/diff/20001/state/apiserver/client/cli...
state/apiserver/client/client.go:890: return resp, err
There seems to be something a bit off about this http response -- it seems like
it's kinda breaking encapsulation in order to supply information that's (1) only
really relevant in the case of an error, and (2) which the eventual recipient
has no opportunity to resolve. Is there any mileage in logging the details and
returning a simpler error over the API?

In particular, your recent email described what STM to be a pretty sane scheme
for a generic auth-broker mechanism in the state server. I'd really like to see
a response that was designed more along those lines... this may not be the time,
but pretty much any API code we write now needs to be supported essentially
forever, so it'd be good to avoid exposing this particular detail (eg it's not
too hard to imagine an in-env charm store (proxy) running against the same mongo
instance and not using http at all -- the HTTP interface is one thing, but the
charm.Repo interface is possibly even more fundamental).

I'm open to arguments here fwiw.

https://codereview.appspot.com/74350043/diff/20001/state/apiserver/client/cli...
File state/apiserver/client/client_test.go (left):

https://codereview.appspot.com/74350043/diff/20001/state/apiserver/client/cli...
state/apiserver/client/client_test.go:1772: c.Assert(store.TestMode, gc.Equals,
true)
we seem to have lost (some of?) the tests for test-mode, which remains relevant
and necessary.

https://codereview.appspot.com/74350043/diff/20001/testing/charm.go
File testing/charm.go (right):

https://codereview.appspot.com/74350043/diff/20001/testing/charm.go#newcode204
testing/charm.go:204: Header:     map[string][]string{"Www-Authenticate":
[]string{"Basic realm=impossible"}},
Hmm, I observe that the charm.CharmStore methods do frequently return http
errors directly, but I'm concerned that we're coming to subtly depend on them.
If you do convince me this is a good path, I'll at least require
sufficiently-obsessive testing of the charm store Repo implementation that it'll
be hard to break unintentionally.

https://codereview.appspot.com/74350043/diff/20001/utils/http.go
File utils/http.go (right):

https://codereview.appspot.com/74350043/diff/20001/utils/http.go#newcode72
utils/http.go:72: if svcErr, is := err.(*httpServiceError); is {
s/is/ok/ please

[cmars_, 2014-03-14 00:43:11]

Ok, you've convinced me, I went too far passing HTTP responses back to the
client. If my proposed alternatives sound good, I'll come back soon with another
patch.

Thanks!
Casey

https://codereview.appspot.com/74350043/diff/20001/charm/repo.go
File charm/repo.go (right):

https://codereview.appspot.com/74350043/diff/20001/charm/repo.go#newcode124
charm/repo.go:124: return resp, utils.NewHttpServiceError(resp)
On 2014/03/13 22:57:27, fwereade wrote:
> I'm nervous that it's really easy to mask this error, but otherthings are
> starting to depend on it. (I pontificate more on this general theme later...)

A return type for 'CharmStore.get', like:

type CharmStoreResult struct {
  *http.Response
  AuthRequired []*AuthSchemes
}

would be a nicer place to add behavioral stuff to the CharmStoreResult. I agree,
certainly an alternative to error type dispatching.

https://codereview.appspot.com/74350043/diff/20001/state/apiserver/client/cli...
File state/apiserver/client/client.go (right):

https://codereview.appspot.com/74350043/diff/20001/state/apiserver/client/cli...
state/apiserver/client/client.go:890: return resp, err
On 2014/03/13 22:57:27, fwereade wrote:
> There seems to be something a bit off about this http response -- it seems
like
> it's kinda breaking encapsulation in order to supply information that's (1)
only
> really relevant in the case of an error, and (2) which the eventual recipient
> has no opportunity to resolve. Is there any mileage in logging the details and
> returning a simpler error over the API?
> 
> In particular, your recent email described what STM to be a pretty sane scheme
> for a generic auth-broker mechanism in the state server. I'd really like to
see
> a response that was designed more along those lines... this may not be the
time,
> but pretty much any API code we write now needs to be supported essentially
> forever, so it'd be good to avoid exposing this particular detail (eg it's not
> too hard to imagine an in-env charm store (proxy) running against the same
mongo
> instance and not using http at all -- the HTTP interface is one thing, but the
> charm.Repo interface is possibly even more fundamental).
> 
> I'm open to arguments here fwiw.

Yeah, it would be awkward if a non-HTTP charm store service had to to pretend to
have an http.Response, just to ask for authentication.

I'll abstract out & model what's actually needed here -- the "auth required"
feedback doesn't have to be so HTTP-specific, while still supporting RFC 2617
content.

https://codereview.appspot.com/74350043/diff/20001/state/apiserver/client/cli...
File state/apiserver/client/client_test.go (left):

https://codereview.appspot.com/74350043/diff/20001/state/apiserver/client/cli...
state/apiserver/client/client_test.go:1772: c.Assert(store.TestMode, gc.Equals,
true)
On 2014/03/13 22:57:27, fwereade wrote:
> we seem to have lost (some of?) the tests for test-mode, which remains
relevant
> and necessary.

Pretty sure I botched the bzr merge. Looking forward to having "git rebase" at
my disposal...