Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Cache discharge macaroons in cross model relations client #44
Conversation
babbageclunk
approved these changes
Aug 8, 2017
This looks good! Some niggles on the fiddly concurrency stuff in MacaroonCache. And it'd be nice if you could extract out the repetitive macaroon handling a bit in the client facades (not sure how easy that is - and it might be be better in a separate PR anyway).
| + return NewClientWithCache(caller, NewMacaroonCache(clock.WallClock)) | ||
| +} | ||
| + | ||
| +// NewClient creates a new client-side CrossModelRelations facade |
| @@ -53,6 +66,11 @@ func (c *Client) PublishRelationChange(change params.RemoteRelationChangeEvent) | ||
| args := params.RemoteRelationsChanges{ | ||
| Changes: []params.RemoteRelationChangeEvent{change}, | ||
| } | ||
| + // Use any previously cached discharge macaroons. | ||
| + if ms, ok := c.cache.Get(change.RelationToken); ok { |
babbageclunk
Aug 8, 2017
The repeated sequence of cache check and update (and the macaroon discharge retry loop each one wraps) is a bit worrying. It seems like what the methods are actually doing is lost in the boilerplate.
Is there a way we can abstract this pattern? Maybe using a closure to handle the differences between the cases? How do we do this in other macaroon-discharge handling code?
I guess you could argue that this is client facade code so it's where the boilerplate should be, though.
wallyworld
Aug 8, 2017
Owner
It's sorted of repeated but different. Different arg types and return value types etc so logic is slightly different. Some places it's the same. But the Go philosophy tends to be simplicity even if that means extra boilerplate. Trying to extract a closure would work for some of the methods but not all. So we would be adding cognitive overhead there. I erred on the side of simple to read code. I'll take another look to see if something simple and readable can be done.
| + | ||
| +// MacaroonCache contains macaroons which are removed at a specified interval. | ||
| +type MacaroonCache struct { | ||
| + *cacheInternal |
babbageclunk
Aug 8, 2017
Why have the separate cacheInternal type here? (Rather than having the private fields of cacheInternal in here.)
Oh, I think I understand. It's so that pointers to the cacheInternal inside it that are held by goroutines, like the argument to expiryWorker.loop, won't keep the MacaroonCache instance alive, so the finalizer will be run even though that goroutine's still going. Is that right? It's worth a comment.
| +} | ||
| + | ||
| +func (w *expiryWorker) loop(c *cacheInternal) { | ||
| + w.stop = make(chan bool) |
babbageclunk
Aug 8, 2017
Why create the stop chan here, rather than when the expiryWorker is created? I was worried that the finalizer would panic, but then I remembered that sending on a nil channel just blocks. Still, it seems like it would be less tricksy just to create it straight away.
| + c.Assert(err, jc.ErrorIsNil) | ||
| + | ||
| + cache.Upsert("token", macaroon.Slice{mac}) | ||
| + clock.Advance(6 * time.Minute) |
babbageclunk
Aug 8, 2017
Sometimes this won't test what it thinks it's testing - if the expiry goroutine hasn't gotten to the clock.After call by the time the test calls Advance it won't see the update. It won't be unreliable since the expiry check in Get will still expire it anyway, but this should probably still use WaitAdvance.
wallyworld
added some commits
Aug 8, 2017
wallyworld
merged commit 7bbd2f0
into
wireup-cmr-macaroon-auth
Aug 9, 2017
added a commit
that referenced
this pull request
Aug 9, 2017
wallyworld commentedAug 8, 2017
Description of change
Add a new expiring cache to hold macaroons. This is used by the cross model relations client so that any discharge macaroons for one api call can be reused with the next (until the macaroons expire and new ones are obtained). This avoids the need to discharge all the time.
QA steps
Run up a cmr scenario and ensure everything works as before - the change should be transparent.