Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Client should fall back on cache if operating offline when updating #982

Merged

Conversation

cyli
Copy link
Contributor

@cyli cyli commented Sep 29, 2016

If the client is in offline mode, or encounters some kind of network error when downloading a new timestamp, it should fall back on using the version of the repo in the cache.

Since there a huge number of possible network errors, just wrap any errors received when actually making the HTTP request using the remote store in a NetworkError.

Copy link
Contributor

@riyazdf riyazdf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for fixing this so quickly! I have a couple of non-blocking nits but overall LGTM 👍

@@ -5,6 +5,7 @@
+ Output message to CLI when repo changes have been successfully published [#974](https://github.com/docker/notary/pull/974)
+ Improved error messages for client authentication errors and for the witness command [#972](https://github.com/docker/notary/pull/972)
+ Support for finding keys that are anywhere in the notary directory's "private" directory, not just under "private/root_keys" or "private/tuf_keys" [#981](https://github.com/docker/notary/pull/981)
+ Previously, on any error updating, the client would fall back on the cache. Now we only do so if there is a network error or if the server is unavailable or missing the TUF data (and fails on invalid root rotation). [#982](https://github.com/docker/notary/pull/982)
Copy link
Contributor

@riyazdf riyazdf Sep 30, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we split out the parenthesized bit for clarity? Something like Invalid TUF data will cause the update to fail, for example if there was an invalid root rotation

}
defer resp.Body.Close()
return translateStatusToError(resp, "POST "+name)
return s.SetMulti(map[string][]byte{name: blob})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is an awesome cleanup 👍

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh man yeah! Nice one!

defer os.RemoveAll(tempBaseDir)
offlineRepo, err := NewNotaryRepository(tempBaseDir, "docker.com/notary", "https://nope",
nil, passphrase.ConstantRetriever("pass"), trustpinning.TrustPinConfig{})
require.NoError(t, err)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we call offlineRepo.Update(false) and verify that it errors?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, yes I was intending to and just forgot. :) Thanks!

@cyli cyli force-pushed the update-uses-cache-if-offline branch 2 times, most recently from ec8d772 to 2b3cac2 Compare September 30, 2016 00:55
@GordonTheTurtle
Copy link

Please sign your commits following these rules:
https://github.com/docker/docker/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ git clone -b "update-uses-cache-if-offline" git@github.com:cyli/notary.git somewhere
$ cd somewhere
$ git rebase -i HEAD~74
editor opens
change each 'pick' to 'edit'
save the file and quit
$ git commit --amend -s --no-edit
$ git rebase --continue # and repeat the amend for each commit
$ git push -f

Amending updates the existing PR. You DO NOT need to open a new one.

Signed-off-by: Ying Li <ying.li@docker.com>
Signed-off-by: Ying Li <ying.li@docker.com>
Copy link
Contributor

@endophage endophage left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Great job catching this 👍

@endophage endophage merged commit 4ab9224 into notaryproject:hotfix/0.4.1 Sep 30, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants