|
|
|
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)
Patch Set 1 #Patch Set 2 : Feedback HTTP service errors to AddCharm RPC call. #
Total comments: 8
Total messages: 6
|
cmars
Please take a look.
|
11 years, 10 months ago (2014年03月12日 02:08:33 UTC) #1 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Please take a look.
Please take a look.
(Merged with trunk)
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
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...