Code review comment for lp://staging/~cmars/juju-core/cs-httpauth-feedback

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/client.go
File state/apiserver/client/client.go (right):

https://codereview.appspot.com/74350043/diff/20001/state/apiserver/client/client.go#newcode890
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/client_test.go
File state/apiserver/client/client_test.go (left):

https://codereview.appspot.com/74350043/diff/20001/state/apiserver/client/client_test.go#oldcode1772
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

https://codereview.appspot.com/74350043/