Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Wire up new cmr macaroon authernticator #7711
Conversation
wallyworld
added some commits
Aug 7, 2017
mjs
reviewed
Aug 8, 2017
LGTM. One suggestion about the structure of the API retries.
I think someone with more macaroons knowledge should also take a look.
| +// and the resulting discharge macaroons passed back so the api call can be retried. | ||
| +func (c *Client) handleError(err error) (macaroon.Slice, error) { | ||
| + if params.ErrCode(err) != params.CodeDischargeRequired { | ||
| + return nil, err |
wallyworld
Aug 8, 2017
Owner
Not needed here IMO because the error is wrapped by the facade methods which call this handleError().
Tracing doesn't really do much for us here.
| + for j, res := range results.Results { | ||
| + result[retryIndices[j]] = res | ||
| + } | ||
| + } | ||
| } |
mjs
Aug 8, 2017
Contributor
Given that the handling for iteration 0 and iteration 1 are so different, the code might be clearer if instead of using a for loop, the API calling part was extracted to a separate method and it was called twice. Something like:
- callAPI()
- code to handle the first case, returning early if nothing else to do
- callAPI()
- code to handle second case
This goes for the other API calls too.
| @@ -39,52 +40,66 @@ func (pool statePoolShim) Get(modelUUID string) (Backend, func(), error) { | ||
| closer := func() { | ||
| releaser() | ||
| } | ||
| - return &stateShim{st}, closer, nil | ||
| + return &stateShim{st: st, Backend: commoncrossmodel.GetBackend(st)}, closer, nil |
wallyworld
Aug 8, 2017
Owner
It's needed because there's 2 lots of state interfaces (the common one and the one in this package) and they conflict. We have the same issues elsewhere too. It's messy.
| @@ -0,0 +1,80 @@ | ||
| +// Copyright 2017 Canonical Ltd. |
wallyworld
Aug 8, 2017
Owner
I've discussed verbally previously to validate the approach used in this PR but do intend to follow up again now that this has work been done.
|
As discussed on IRC, changes made to downstream branch and merged back into this one. |
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
Build failed: Tests failed |
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
wallyworld commentedAug 8, 2017
Description of change
Hook the new cross model offer authenticator into the api server. This provides a http endpoint for discharging the "has-offer-permission" third party caveat. Now, macaroons used to validate access to ffers are given a 2 minute expiry time. When they expire, a discharge error pointing to the new endpoint is generated.
The crossmodel api facade methods all now have the capability to transparently recognise the discharge error and perform the discharge. The workers which use the facade are none the wiser. The facade methods essentially try the facade call up to 2 times - once initially, and then a second time with discharge macaroon if the first time failed with a discharge error and the third party caveats could be successfully discharged. To make this all possible, the bakery client off the api connection is exposed so it can be used to do the discharge in the api client.
QA steps
Run up a CMR scenario and check logs for errors.
Everything should work as previously with the old auth.