Address review comments.

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

client/backwards_compatibility_test.go

@@ -77,7 +77,7 @@ func Test0Dot1RepoFormat(t *testing.T) {
 	assert.NoError(t, err, "error creating repo: %s", err)
 
 	// rotate the timestamp key, since the server doesn't have that one
-	timestampPubKey, err := getRemoteKey(ts.URL, gun, data.CanonicalTimestampRole, http.DefaultTransport, false)
+	timestampPubKey, err := getRemoteKey(ts.URL, gun, data.CanonicalTimestampRole, http.DefaultTransport)
 	assert.NoError(t, err)
 	assert.NoError(
 		t, repo.rootFileKeyChange(data.CanonicalTimestampRole, changelist.ActionCreate, timestampPubKey))

client/client.go

@@ -236,7 +236,7 @@ func (r *NotaryRepository) Initialize(rootKeyID string, serverManagedRoles ...st
 	}
 	for _, role := range remotelyManagedKeys {
 		// This key is generated by the remote server.
-		key, err := getRemoteKey(r.baseURL, r.gun, role, r.roundTrip, false)
+		key, err := getRemoteKey(r.baseURL, r.gun, role, r.roundTrip)
 		if err != nil {
 			return err
 		}
@@ -1007,11 +1007,11 @@ func (r *NotaryRepository) RotateKey(role string, serverManagesKey bool) error {
 		err    error
 	)
 	if serverManagesKey {
-		pubKey, err = getRemoteKey(r.baseURL, r.gun, role, r.roundTrip, true)
+		pubKey, err = rotateRemoteKey(r.baseURL, r.gun, role, r.roundTrip)
 		if _, ok := err.(store.ErrInvalidOperation); ok {
 			logrus.Errorf(
 				"Server unable to rotate key: %s. Getting the most recently created remote key instead.", err)
-			pubKey, err = getRemoteKey(r.baseURL, r.gun, role, r.roundTrip, false)
+			pubKey, err = getRemoteKey(r.baseURL, r.gun, role, r.roundTrip)
 		}
 	} else {
 		pubKey, err = r.CryptoService.Create(role, data.ECDSAKey)

client/client_test.go

@@ -2580,19 +2580,22 @@ func TestRemoteRotationRateLimited(t *testing.T) {
 	role := data.CanonicalSnapshotRole
 
 	// original key is the key on the repo
-	origKey, err := getRemoteKey(ts.URL, repo.gun, role, repo.roundTrip, false)
+	origKey, err := getRemoteKey(ts.URL, repo.gun, role, repo.roundTrip)
 	assert.NoError(t, err)
 	assert.Len(t, repo.tufRepo.Root.Signed.Roles[role].KeyIDs, 1)
 	assert.Equal(t, origKey.ID(), repo.tufRepo.Root.Signed.Roles[role].KeyIDs[0])
 
 	// rotate keys, and assert that the first rotation doesn't fail but doesn't rotate,
-	// because the original key has not actually been published yet.  But the
+	// because the original key has not actually been published yet.
 	assert.NoError(t, repo.RotateKey(role, true))
 
-	rotateKey1, err := getRemoteKey(ts.URL, repo.gun, role, repo.roundTrip, false)
+	rotateKey1, err := getRemoteKey(ts.URL, repo.gun, role, repo.roundTrip)
 	assert.NoError(t, err)
 	assert.Equal(t, origKey.ID(), rotateKey1.ID())
 
+	// publish
+	assert.NoError(t, repo.Publish())
+
 	// the final key is the original key, since it was not rotated
 	assert.Len(t, repo.tufRepo.Root.Signed.Roles[role].KeyIDs, 1)
 	assert.Equal(t, rotateKey1.ID(), repo.tufRepo.Root.Signed.Roles[role].KeyIDs[0])

client/helpers.go

@@ -238,17 +238,23 @@ func nearExpiry(r *data.SignedRoot) bool {
 }
 
 // Fetches a public key from a remote store, given a gun and role
-func getRemoteKey(url, gun, role string, rt http.RoundTripper, rotate bool) (data.PublicKey, error) {
+func getRemoteKey(url, gun, role string, rt http.RoundTripper) (data.PublicKey, error) {
 	remote, err := getRemoteStore(url, gun, rt)
 	if err != nil {
 		return nil, err
 	}
-	if rotate {
-		return remote.RotateKey(role)
-	}
 	return remote.GetKey(role)
 }
 
+// Rotates a public key from a remote store, given a gun and role
+func rotateRemoteKey(url, gun, role string, rt http.RoundTripper) (data.PublicKey, error) {
+	remote, err := getRemoteStore(url, gun, rt)
+	if err != nil {
+		return nil, err
+	}
+	return remote.RotateKey(role)
+}
+
 // add a key to a KeyDB, and create a role for the key and add it.
 func addKeyForRole(kdb *keys.KeyDB, role string, key data.PublicKey) error {
 	theRole, err := data.NewRole(role, 1, []string{key.ID()}, nil, nil)

const.go

@@ -18,4 +18,7 @@ const (
 	Sha256HexSize = 64
 	// TrustedCertsDir is the directory, under the notary repo base directory, where trusted certs are stored
 	TrustedCertsDir = "trusted_certificates"
+
+	// HTTPStatusTooManyRequests is the http status 429 - will be an exported constant in http library in Go 1.6
+	HTTPStatusTooManyRequests = 429
 )

server/errors/errors.go

@@ -4,6 +4,7 @@ import (
 	"net/http"
 
 	"github.com/docker/distribution/registry/api/errcode"
+	"github.com/docker/notary"
 )
 
 // The notary API is on version 1, but URLs start with /v2/ to be consistent
@@ -83,14 +84,14 @@ var (
 	ErrNoKeyAlgorithm = errcode.Register(errGroup, errcode.ErrorDescriptor{
 		Value:          "NO_KEYALGORITHM",
 		Message:        "The server does not have a key algorithm configured.",
-		Description:    "No key algorihtm has been configured for the server and it has been asked to perform an operation that requires generation.",
+		Description:    "No key algorithm has been configured for the server and it has been asked to perform an operation that requires generation.",
 		HTTPStatusCode: http.StatusInternalServerError,
 	})
 	ErrCannotRotateKey = errcode.Register(errGroup, errcode.ErrorDescriptor{
 		Value:          "CANNOT_ROTATE_KEY",
 		Message:        "Key has already been rotated recently.",
 		Description:    "The key has been rotated too recently, and cannot be rotated again at this time.",
-		HTTPStatusCode: 429, // 429 is Too Many Requests - this will be added as a status constant in Go 1.6
+		HTTPStatusCode: notary.HTTPStatusTooManyRequests,
 	})
 	ErrUnknown = errcode.ErrorCodeUnknown
 )

server/server_test.go

@@ -14,6 +14,7 @@ import (
 	"testing"
 
 	_ "github.com/docker/distribution/registry/auth/silly"
+	"github.com/docker/notary"
 	"github.com/docker/notary/server/storage"
 	"github.com/docker/notary/tuf/data"
 	"github.com/docker/notary/tuf/signed"
@@ -102,8 +103,8 @@ func TestRotateKeyEndpoint(t *testing.T) {
 	defer ts.Close()
 
 	rolesToStatus := map[string]int{
-		data.CanonicalTimestampRole: 429, // not implemented yet
-		data.CanonicalSnapshotRole:  429, // not implemented yet
+		data.CanonicalTimestampRole: notary.HTTPStatusTooManyRequests, // not implemented yet
+		data.CanonicalSnapshotRole:  notary.HTTPStatusTooManyRequests, // not implemented yet
 		data.CanonicalTargetsRole:   http.StatusNotFound,
 		data.CanonicalRootRole:      http.StatusNotFound,
 		"somerandomrole":            http.StatusNotFound,

tuf/store/httpstore.go

@@ -134,7 +134,7 @@ func translateStatusToError(resp *http.Response, resource string) error {
 		return ErrMetaNotFound{Resource: resource}
 	case http.StatusBadRequest:
 		return tryUnmarshalError(resp, ErrInvalidOperation{})
-	case 429: // HTTP over limit error - will be added as a constnat in Go 1.6
+	case notary.HTTPStatusTooManyRequests:
 		return ErrInvalidOperation{fmt.Sprintf("%s rate limited", resource)}
 	default:
 		return ErrServerUnavailable{code: resp.StatusCode}

tuf/store/httpstore_test.go

@@ -14,6 +14,7 @@ import (
 	"testing"
 
 	"github.com/docker/go/canonical/json"
+	"github.com/docker/notary"
 	"github.com/docker/notary/tuf/data"
 	"github.com/docker/notary/tuf/signed"
 	"github.com/docker/notary/tuf/validation"
@@ -328,7 +329,7 @@ func TestErrServerUnavailable(t *testing.T) {
 	}
 }
 
-// If successful, GetKey and RotateKey both return public keys
+// GetKey and RotateKey both succeed if valid public keys are returned from the server with a 200 status
 func TestGetKeyAndRotateKeySuccess(t *testing.T) {
 	c := signed.NewEd25519()
 	role := data.CanonicalSnapshotRole
@@ -395,13 +396,13 @@ func TestGetKeyAndRotateKeyServerUnreachable(t *testing.T) {
 	require.IsType(t, &net.OpError{}, err)
 }
 
-// GetKey and RotateKey both fail with ErrInvalidOperation if a 429 is returned
+// GetKey and RotateKey both fail with ErrInvalidOperation if a notary.HTTPStatusTooManyRequests is returned
 func TestGetKeyAndRotateKeyServerLimitError(t *testing.T) {
 	role := data.CanonicalSnapshotRole
 
 	// Set up a simple handler and server for our store
 	handler := func(w http.ResponseWriter, r *http.Request) {
-		w.WriteHeader(429)
+		w.WriteHeader(notary.HTTPStatusTooManyRequests)
 	}
 	server := httptest.NewServer(http.HandlerFunc(handler))
 	defer server.Close()