Address review comments

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

api/objects.proto

@@ -246,8 +246,8 @@ message Secret {
 	string digest = 4;
 
 	// SecretSize represents the size (number of bytes) of the secret data, and is
-	// calculated from the data contained in `Secret.Spec.data`..
-	uint32 size = 5 [(gogoproto.customname) = "SecretSize"];
+	// calculated from the data contained in `Secret.Spec.data`.
+	int64 size = 5 [(gogoproto.customname) = "SecretSize"];
 
 	// Whether the secret is an internal secret (not set by a user) or not.
 	bool internal = 6;

cmd/swarmctl/main.go

@@ -6,7 +6,7 @@ import (
 	"github.com/docker/swarmkit/cmd/swarmctl/cluster"
 	"github.com/docker/swarmkit/cmd/swarmctl/network"
 	"github.com/docker/swarmkit/cmd/swarmctl/node"
-	"github.com/docker/swarmkit/cmd/swarmctl/secrets"
+	"github.com/docker/swarmkit/cmd/swarmctl/secret"
 	"github.com/docker/swarmkit/cmd/swarmctl/service"
 	"github.com/docker/swarmkit/cmd/swarmctl/task"
 	"github.com/docker/swarmkit/version"
@@ -55,6 +55,6 @@ func init() {
 		version.Cmd,
 		network.Cmd,
 		cluster.Cmd,
-		secrets.Cmd,
+		secret.Cmd,
 	)
 }

cmd/swarmctl/secrets/cmd.go → cmd/swarmctl/secret/cmd.go

@@ -1,4 +1,4 @@
-package secrets
+package secret
 
 import "github.com/spf13/cobra"
 

cmd/swarmctl/secrets/common.go → cmd/swarmctl/secret/common.go

@@ -1,4 +1,4 @@
-package secrets
+package secret
 
 import (
 	"fmt"

cmd/swarmctl/secrets/create.go → cmd/swarmctl/secret/create.go

@@ -1,4 +1,4 @@
-package secrets
+package secret
 
 import (
 	"errors"

cmd/swarmctl/secrets/inspect.go → cmd/swarmctl/secret/inspect.go

@@ -1,14 +1,14 @@
-package secrets
+package secret
 
 import (
 	"errors"
 	"fmt"
 	"os"
 	"text/tabwriter"
-	"time"
 
 	"github.com/docker/swarmkit/api"
 	"github.com/docker/swarmkit/cmd/swarmctl/common"
+	"github.com/docker/swarmkit/protobuf/ptypes"
 	"github.com/spf13/cobra"
 )
 
@@ -27,8 +27,7 @@ func printSecretSummary(secret *api.Secret) {
 	common.FprintfIfNotEmpty(w, "Digest\t: %s\n", secret.Digest)
 	common.FprintfIfNotEmpty(w, "Size\t: %d\n", secret.SecretSize)
 
-	created := time.Unix(int64(secret.Meta.CreatedAt.Seconds), int64(secret.Meta.CreatedAt.Nanos))
-	common.FprintfIfNotEmpty(w, "Created\t: %s\n", created.Format(time.RFC822))
+	common.FprintfIfNotEmpty(w, "Created\t: %s\n", ptypes.TimestampString(secret.Meta.CreatedAt))
 }
 
 var (

cmd/swarmctl/secrets/list.go → cmd/swarmctl/secret/list.go

@@ -1,15 +1,15 @@
-package secrets
+package secret
 
 import (
 	"errors"
 	"fmt"
 	"os"
 	"sort"
 	"text/tabwriter"
-	"time"
 
 	"github.com/docker/swarmkit/api"
 	"github.com/docker/swarmkit/cmd/swarmctl/common"
+	"github.com/docker/swarmkit/protobuf/ptypes"
 	"github.com/dustin/go-humanize"
 	"github.com/spf13/cobra"
 )
@@ -19,8 +19,14 @@ type secretSorter []*api.Secret
 func (k secretSorter) Len() int      { return len(k) }
 func (k secretSorter) Swap(i, j int) { k[i], k[j] = k[j], k[i] }
 func (k secretSorter) Less(i, j int) bool {
-	iTime := time.Unix(k[i].Meta.CreatedAt.Seconds, int64(k[i].Meta.CreatedAt.Nanos))
-	jTime := time.Unix(k[j].Meta.CreatedAt.Seconds, int64(k[j].Meta.CreatedAt.Nanos))
+	iTime, err := ptypes.Timestamp(k[i].Meta.CreatedAt)
+	if err != nil {
+		panic(err)
+	}
+	jTime, err := ptypes.Timestamp(k[j].Meta.CreatedAt)
+	if err != nil {
+		panic(err)
+	}
 	return jTime.Before(iTime)
 }
 
@@ -59,7 +65,10 @@ var (
 				}()
 				common.PrintHeader(w, "ID", "Name", "Created", "Digest", "Size")
 				output = func(s *api.Secret) {
-					created := time.Unix(int64(s.Meta.CreatedAt.Seconds), int64(s.Meta.CreatedAt.Nanos))
+					created, err := ptypes.Timestamp(s.Meta.CreatedAt)
+					if err != nil {
+						panic(err)
+					}
 					fmt.Fprintf(w, "%s\t%s\t%s\t%s\t%d\n",
 						s.ID,
 						s.Spec.Annotations.Name,

cmd/swarmctl/secrets/remove.go → cmd/swarmctl/secret/remove.go

@@ -1,4 +1,4 @@
-package secrets
+package secret
 
 import (
 	"errors"

manager/controlapi/secret.go

@@ -17,14 +17,14 @@ import (
 // MaxSecretSize is the maximum byte length of the `Secret.Spec.Data` field.
 const MaxSecretSize = 500 * 1024 // 500KB
 
-var isValidSecretName = regexp.MustCompile(`^[a-zA-Z0-9](?:[a-zA-Z0-9-_.]{0,62}[a-zA-Z0-9])*$`)
+var validSecretNameRegexp = regexp.MustCompile(`^[a-zA-Z0-9]+(?:[a-zA-Z0-9-_.]*[a-zA-Z0-9])?$`)
 
 // assumes spec is not nil
 func secretFromSecretSpec(spec *api.SecretSpec) *api.Secret {
 	return &api.Secret{
 		ID:         identity.NewID(),
 		Spec:       *spec,
-		SecretSize: uint32(len(spec.Data)),
+		SecretSize: int64(len(spec.Data)),
 		Digest:     digest.FromBytes(spec.Data).String(),
 	}
 }
@@ -40,7 +40,6 @@ func (s *Server) GetSecret(ctx context.Context, request *api.GetSecretRequest) (
 	}
 
 	var secret *api.Secret
-
 	s.store.View(func(tx store.ReadTx) {
 		secret = store.GetSecret(tx, request.SecretID)
 	})
@@ -49,7 +48,7 @@ func (s *Server) GetSecret(ctx context.Context, request *api.GetSecretRequest) (
 		return nil, grpc.Errorf(codes.NotFound, "secret %s not found", request.SecretID)
 	}
 
-	secret.Spec.Data = nil
+	secret.Spec.Data = nil // clean the actual secret data so it's never returned
 	return &api.GetSecretResponse{Secret: secret}, nil
 }
 
@@ -94,7 +93,6 @@ func (s *Server) ListSecrets(ctx context.Context, request *api.ListSecretsReques
 	s.store.View(func(tx store.ReadTx) {
 		secrets, err = store.FindSecrets(tx, by)
 	})
-
 	if err != nil {
 		return nil, err
 	}
@@ -104,7 +102,7 @@ func (s *Server) ListSecrets(ctx context.Context, request *api.ListSecretsReques
 		if secret.Internal || !filterMatchLabels(secret.Spec.Annotations.Labels, labels) {
 			continue
 		}
-		secret.Spec.Data = nil
+		secret.Spec.Data = nil // clean the actual secret data so it's never returned
 		respSecrets = append(respSecrets, secret)
 	}
 
@@ -130,7 +128,7 @@ func (s *Server) CreateSecret(ctx context.Context, request *api.CreateSecretRequ
 	case store.ErrNameConflict:
 		return nil, grpc.Errorf(codes.AlreadyExists, "secret %s already exists", request.Spec.Annotations.Name)
 	case nil:
-		secret.Spec.Data = nil
+		secret.Spec.Data = nil // clean the actual secret data so it's never returned
 		return &api.CreateSecretResponse{Secret: secret}, nil
 	default:
 		return nil, err
@@ -149,7 +147,6 @@ func (s *Server) RemoveSecret(ctx context.Context, request *api.RemoveSecretRequ
 	err := s.store.Update(func(tx store.Tx) error {
 		return store.DeleteSecret(tx, request.SecretID)
 	})
-
 	switch err {
 	case store.ErrNotExist:
 		return nil, grpc.Errorf(codes.NotFound, "secret %s not found", request.SecretID)
@@ -168,16 +165,16 @@ func validateSecretSpec(spec *api.SecretSpec) error {
 		return err
 	}
 
-	if len(spec.Data) > MaxSecretSize || len(spec.Data) == 0 {
-		return grpc.Errorf(codes.InvalidArgument, "secret data must be between 1 and %d bytes inclusive", MaxSecretSize)
+	if len(spec.Data) >= MaxSecretSize || len(spec.Data) < 1 {
+		return grpc.Errorf(codes.InvalidArgument, "secret data must be larger than 0 and less than %d bytes", MaxSecretSize)
 	}
 	return nil
 }
 
 func validateSecretAnnotations(m api.Annotations) error {
 	if m.Name == "" {
 		return grpc.Errorf(codes.InvalidArgument, "name must be provided")
-	} else if !isValidSecretName.MatchString(m.Name) {
+	} else if len(m.Name) > 64 || !validSecretNameRegexp.MatchString(m.Name) {
 		// if the name doesn't match the regex
 		return grpc.Errorf(codes.InvalidArgument,
 			"invalid name, only 64 [a-zA-Z0-9-_.] characters allowed, and the start and end character must be [a-zA-Z0-9]")

manager/controlapi/secret_test.go

@@ -4,6 +4,7 @@ import (
 	"crypto/sha256"
 	"encoding/hex"
 	"fmt"
+	"strings"
 	"testing"
 
 	"github.com/docker/swarmkit/api"
@@ -57,7 +58,7 @@ func TestValidateSecretSpec(t *testing.T) {
 		"with:colon",
 		"with;semicolon",
 		"snowman☃",
-		"o_______________________________________________________________o", // exactly 65 characters
+		strings.Repeat("a", 65),
 	} {
 		err := validateSecretSpec(createSecretSpec(badName, []byte("valid secret"), nil))
 		assert.Error(t, err)
@@ -77,12 +78,12 @@ func TestValidateSecretSpec(t *testing.T) {
 		"0",
 		"a",
 		"A",
-		"name-with-dashes",
-		"name.with.dots",
-		"name_with_underscores",
+		"name-with--dashes",
+		"name.with..dots",
+		"name_with__underscores",
 		"name.with-all_special",
 		"02624name035with1699numbers015125",
-		"o______________________________________________________________o", // exactly 64 characters
+		strings.Repeat("a", 64),
 	} {
 		err := validateSecretSpec(createSecretSpec(goodName, []byte("valid secret"), nil))
 		assert.NoError(t, err)