Client should fall back on cache if operating offline when updating

CHANGELOG.md

@@ -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. Invalid TUF data will cause the update to fail - for example if there was an invalid root rotation. [#982](https://github.com/docker/notary/pull/982)
 
 ## [v0.4.0](https://github.com/docker/notary/releases/tag/v0.4.0) 9/21/2016
 + Server-managed key rotations [#889](https://github.com/docker/notary/pull/889)

client/client_update_test.go

@@ -172,6 +172,45 @@ func TestUpdateSucceedsEvenIfCannotWriteExistingRepo(t *testing.T) {
 	}
 }
 
+// If there is no local cache, update will error if it can't connect to the server.  Otherwise
+// it uses the local cache.
+func TestUpdateInOfflineMode(t *testing.T) {
+	if testing.Short() {
+		t.Skip("skipping test in short mode")
+	}
+
+	// invalid URL, no cache - errors
+	invalidURLRepo := newBlankRepo(t, "https://nothisdoesnotexist.com")
+	defer os.RemoveAll(invalidURLRepo.baseDir)
+	err := invalidURLRepo.Update(false)
+	require.Error(t, err)
+	require.IsType(t, store.NetworkError{}, err)
+
+	// offline client: no cache - errors
+	tempBaseDir, err := ioutil.TempDir("", "notary-test-")
+	require.NoError(t, err, "failed to create a temporary directory: %s", err)
+	defer os.RemoveAll(tempBaseDir)
+
+	offlineRepo, err := NewNotaryRepository(tempBaseDir, "docker.com/notary", "https://nope",
+		nil, passphrase.ConstantRetriever("pass"), trustpinning.TrustPinConfig{})
+	require.NoError(t, err)
+	err = offlineRepo.Update(false)
+	require.Error(t, err)
+	require.IsType(t, store.ErrOffline{}, err)
+
+	// set existing metadata on the repo
+	serverMeta, _, err := testutils.NewRepoMetadata("docker.com/notary", metadataDelegations...)
+	require.NoError(t, err)
+	for name, metaBytes := range serverMeta {
+		require.NoError(t, invalidURLRepo.fileStore.Set(name, metaBytes))
+		require.NoError(t, offlineRepo.fileStore.Set(name, metaBytes))
+	}
+
+	// both of these can read from cache and load repo
+	require.NoError(t, invalidURLRepo.Update(false))
+	require.NoError(t, offlineRepo.Update(false))
+}
+
 type swizzleFunc func(*testutils.MetadataSwizzler, string) error
 type swizzleExpectations struct {
 	desc       string

client/tufclient.go

@@ -116,7 +116,7 @@ func (c *TUFClient) downloadTimestamp() error {
 	switch remoteErr.(type) {
 	case nil:
 		return nil
-	case store.ErrMetaNotFound, store.ErrServerUnavailable:
+	case store.ErrMetaNotFound, store.ErrServerUnavailable, store.ErrOffline, store.NetworkError:
 		break
 	default:
 		return remoteErr

storage/httpstore.go

@@ -33,6 +33,15 @@ type ErrServerUnavailable struct {
 	code int
 }
 
+// NetworkError represents any kind of network error when attempting to make a request
+type NetworkError struct {
+	Wrapped error
+}
+
+func (n NetworkError) Error() string {
+	return n.Wrapped.Error()
+}
+
 func (err ErrServerUnavailable) Error() string {
 	if err.code == 401 {
 		return fmt.Sprintf("you are not authorized to perform this operation: server returned 401.")
@@ -152,7 +161,7 @@ func (s HTTPStore) GetSized(name string, size int64) ([]byte, error) {
 	}
 	resp, err := s.roundTrip.RoundTrip(req)
 	if err != nil {
-		return nil, err
+		return nil, NetworkError{Wrapped: err}
 	}
 	defer resp.Body.Close()
 	if err := translateStatusToError(resp, name); err != nil {
@@ -174,22 +183,9 @@ func (s HTTPStore) GetSized(name string, size int64) ([]byte, error) {
 	return body, nil
 }
 
-// Set uploads a piece of TUF metadata to the server
+// Set sends a single piece of metadata to the TUF server
 func (s HTTPStore) Set(name string, blob []byte) error {
-	url, err := s.buildMetaURL("")
-	if err != nil {
-		return err
-	}
-	req, err := http.NewRequest("POST", url.String(), bytes.NewReader(blob))
-	if err != nil {
-		return err
-	}
-	resp, err := s.roundTrip.RoundTrip(req)
-	if err != nil {
-		return err
-	}
-	defer resp.Body.Close()
-	return translateStatusToError(resp, "POST "+name)
+	return s.SetMulti(map[string][]byte{name: blob})
 }
 
 // Remove always fails, because we should never be able to delete metadata
@@ -239,7 +235,7 @@ func (s HTTPStore) SetMulti(metas map[string][]byte) error {
 	}
 	resp, err := s.roundTrip.RoundTrip(req)
 	if err != nil {
-		return err
+		return NetworkError{Wrapped: err}
 	}
 	defer resp.Body.Close()
 	// if this 404's something is pretty wrong
@@ -258,7 +254,7 @@ func (s HTTPStore) RemoveAll() error {
 	}
 	resp, err := s.roundTrip.RoundTrip(req)
 	if err != nil {
-		return err
+		return NetworkError{Wrapped: err}
 	}
 	defer resp.Body.Close()
 	return translateStatusToError(resp, "DELETE metadata for GUN endpoint")
@@ -299,7 +295,7 @@ func (s HTTPStore) GetKey(role string) ([]byte, error) {
 	}
 	resp, err := s.roundTrip.RoundTrip(req)
 	if err != nil {
-		return nil, err
+		return nil, NetworkError{Wrapped: err}
 	}
 	defer resp.Body.Close()
 	if err := translateStatusToError(resp, role+" key"); err != nil {
@@ -324,7 +320,7 @@ func (s HTTPStore) RotateKey(role string) ([]byte, error) {
 	}
 	resp, err := s.roundTrip.RoundTrip(req)
 	if err != nil {
-		return nil, err
+		return nil, NetworkError{Wrapped: err}
 	}
 	defer resp.Body.Close()
 	if err := translateStatusToError(resp, role+" key"); err != nil {

storage/httpstore_test.go

@@ -26,6 +26,12 @@ func (rt *TestRoundTripper) RoundTrip(req *http.Request) (*http.Response, error)
 	return http.DefaultClient.Do(req)
 }
 
+type failRoundTripper struct{}
+
+func (ft failRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) {
+	return nil, fmt.Errorf("FAIL")
+}
+
 func TestHTTPStoreGetSized(t *testing.T) {
 	handler := func(w http.ResponseWriter, r *http.Request) {
 		w.Write([]byte(testRoot))
@@ -46,6 +52,19 @@ func TestHTTPStoreGetSized(t *testing.T) {
 	p := &data.Signed{}
 	err = json.Unmarshal(j, p)
 	require.NoError(t, err)
+
+	// if there is a network error, it gets translated to NetworkError
+	store, err = NewHTTPStore(
+		server.URL,
+		"metadata",
+		"txt",
+		"key",
+		failRoundTripper{},
+	)
+	require.NoError(t, err)
+	_, err = store.GetSized("root", 4801)
+	require.IsType(t, NetworkError{}, err)
+	require.Equal(t, "FAIL", err.Error())
 }
 
 // Test that passing -1 to httpstore's GetSized will return all content
@@ -71,16 +90,18 @@ func TestHTTPStoreGetAllMeta(t *testing.T) {
 	require.NoError(t, err)
 }
 
-func TestSetMultiMeta(t *testing.T) {
+func TestSetSingleAndSetMultiMeta(t *testing.T) {
 	metas := map[string][]byte{
 		"root":    []byte("root data"),
 		"targets": []byte("targets data"),
 	}
 
+	var updates map[string][]byte
+
 	handler := func(w http.ResponseWriter, r *http.Request) {
 		reader, err := r.MultipartReader()
 		require.NoError(t, err)
-		updates := make(map[string][]byte)
+		updates = make(map[string][]byte)
 		for {
 			part, err := reader.NextPart()
 			if err == io.EOF {
@@ -90,21 +111,44 @@ func TestSetMultiMeta(t *testing.T) {
 			updates[role], err = ioutil.ReadAll(part)
 			require.NoError(t, err)
 		}
-		rd, rok := updates["root"]
-		require.True(t, rok)
-		require.Equal(t, rd, metas["root"])
-
-		td, tok := updates["targets"]
-		require.True(t, tok)
-		require.Equal(t, td, metas["targets"])
-
 	}
 	server := httptest.NewServer(http.HandlerFunc(handler))
 	defer server.Close()
 	store, err := NewHTTPStore(server.URL, "metadata", "json", "key", http.DefaultTransport)
 	require.NoError(t, err)
 
-	store.SetMulti(metas)
+	require.NoError(t, store.SetMulti(metas))
+	require.Len(t, updates, 2)
+	rd, rok := updates["root"]
+	require.True(t, rok)
+	require.Equal(t, rd, metas["root"])
+	td, tok := updates["targets"]
+	require.True(t, tok)
+	require.Equal(t, td, metas["targets"])
+
+	require.NoError(t, store.Set("root", metas["root"]))
+	require.Len(t, updates, 1)
+	rd, rok = updates["root"]
+	require.True(t, rok)
+	require.Equal(t, rd, metas["root"])
+
+	// if there is a network error, it gets translated to NetworkError
+	store, err = NewHTTPStore(
+		server.URL,
+		"metadata",
+		"txt",
+		"key",
+		failRoundTripper{},
+	)
+	require.NoError(t, err)
+
+	err = store.SetMulti(metas)
+	require.IsType(t, NetworkError{}, err)
+	require.Equal(t, "FAIL", err.Error())
+
+	err = store.Set("root", metas["root"])
+	require.IsType(t, NetworkError{}, err)
+	require.Equal(t, "FAIL", err.Error())
 }
 
 func testErrorCode(t *testing.T, errorCode int, errType error) {
@@ -204,10 +248,25 @@ func TestHTTPStoreRemoveAll(t *testing.T) {
 
 	err = store.RemoveAll()
 	require.NoError(t, err)
+
+	// if there is a network error, it gets translated to NetworkError
+	store, err = NewHTTPStore(
+		server.URL,
+		"metadata",
+		"txt",
+		"key",
+		failRoundTripper{},
+	)
+	require.NoError(t, err)
+	err = store.RemoveAll()
+	require.IsType(t, NetworkError{}, err)
+	require.Equal(t, "FAIL", err.Error())
 }
 
 func TestHTTPStoreRotateKey(t *testing.T) {
 	handler := func(w http.ResponseWriter, r *http.Request) {
+		require.Equal(t, "POST", r.Method)
+		require.Equal(t, "/metadata/snapshot.key", r.URL.Path)
 		w.Write([]byte(testRootKey))
 	}
 	server := httptest.NewServer(http.HandlerFunc(handler))
@@ -218,6 +277,48 @@ func TestHTTPStoreRotateKey(t *testing.T) {
 	pubKeyBytes, err := store.RotateKey(data.CanonicalSnapshotRole)
 	require.NoError(t, err)
 	require.Equal(t, pubKeyBytes, []byte(testRootKey))
+
+	// if there is a network error, it gets translated to NetworkError
+	store, err = NewHTTPStore(
+		server.URL,
+		"metadata",
+		"txt",
+		"key",
+		failRoundTripper{},
+	)
+	require.NoError(t, err)
+	_, err = store.RotateKey(data.CanonicalSnapshotRole)
+	require.IsType(t, NetworkError{}, err)
+	require.Equal(t, "FAIL", err.Error())
+}
+
+func TestHTTPStoreGetKey(t *testing.T) {
+	handler := func(w http.ResponseWriter, r *http.Request) {
+		require.Equal(t, "GET", r.Method)
+		require.Equal(t, "/metadata/snapshot.key", r.URL.Path)
+		w.Write([]byte(testRootKey))
+	}
+	server := httptest.NewServer(http.HandlerFunc(handler))
+	defer server.Close()
+	store, err := NewHTTPStore(server.URL, "metadata", "json", "key", http.DefaultTransport)
+	require.NoError(t, err)
+
+	pubKeyBytes, err := store.GetKey(data.CanonicalSnapshotRole)
+	require.NoError(t, err)
+	require.Equal(t, pubKeyBytes, []byte(testRootKey))
+
+	// if there is a network error, it gets translated to NetworkError
+	store, err = NewHTTPStore(
+		server.URL,
+		"metadata",
+		"txt",
+		"key",
+		failRoundTripper{},
+	)
+	require.NoError(t, err)
+	_, err = store.GetKey(data.CanonicalSnapshotRole)
+	require.IsType(t, NetworkError{}, err)
+	require.Equal(t, "FAIL", err.Error())
 }
 
 func TestHTTPOffline(t *testing.T) {