Merge lp://staging/~cmars/juju-core/cs-httpauth-feedback into lp://staging/~go-bot/juju-core/trunk

Reviewers: mp+210524_code.launchpad.net,

Message:
Please take a look.

Description:
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)

Please review this at https://codereview.appspot.com/74350043/

Affected files (+160, -176 lines):
   A [revision details]
   M charm/repo.go
   M charm/repo_test.go
   M charm/testing/mockstore.go
   M cmd/juju/deploy.go
   M cmd/juju/upgradecharm.go
   M environs/config/config.go
   M environs/config/config_test.go
   M state/api/client.go
   M state/api/params/internal.go
   M state/apiserver/client/client.go
   M state/apiserver/client/client_test.go
   M testing/charm.go
   M utils/http.go

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

Please take a look.

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

(Merged with trunk)

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

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/

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/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
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/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)
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...

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

WIP until a new version is proposed.