From aebced0f111cd563ead7e34f6362891ac9927b11 Mon Sep 17 00:00:00 2001 From: cyli Date: Mon, 26 Sep 2016 16:27:15 -0700 Subject: [PATCH] Address review comments Signed-off-by: cyli --- api/objects.pb.go | 10 +++++----- api/objects.proto | 4 ++-- cmd/swarmctl/main.go | 4 ++-- cmd/swarmctl/{secrets => secret}/cmd.go | 2 +- cmd/swarmctl/{secrets => secret}/common.go | 2 +- cmd/swarmctl/{secrets => secret}/create.go | 2 +- cmd/swarmctl/{secrets => secret}/inspect.go | 7 +++---- cmd/swarmctl/{secrets => secret}/list.go | 19 ++++++++++++++----- cmd/swarmctl/{secrets => secret}/remove.go | 2 +- manager/controlapi/secret.go | 19 ++++++++----------- manager/controlapi/secret_test.go | 11 ++++++----- 11 files changed, 44 insertions(+), 38 deletions(-) rename cmd/swarmctl/{secrets => secret}/cmd.go (94%) rename cmd/swarmctl/{secrets => secret}/common.go (98%) rename cmd/swarmctl/{secrets => secret}/create.go (98%) rename cmd/swarmctl/{secrets => secret}/inspect.go (86%) rename cmd/swarmctl/{secrets => secret}/list.go (83%) rename cmd/swarmctl/{secrets => secret}/remove.go (97%) diff --git a/api/objects.pb.go b/api/objects.pb.go index 93d515b22e..3cad399730 100644 --- a/api/objects.pb.go +++ b/api/objects.pb.go @@ -248,8 +248,8 @@ type Secret struct { // is calculated from the data contained in `Secret.Spec.data`. Digest string `protobuf:"bytes,4,opt,name=digest,proto3" json:"digest,omitempty"` // SecretSize represents the size (number of bytes) of the secret data, and is - // calculated from the data contained in `Secret.Spec.data`.. - SecretSize uint32 `protobuf:"varint,5,opt,name=size,proto3" json:"size,omitempty"` + // calculated from the data contained in `Secret.Spec.data`. + SecretSize int64 `protobuf:"varint,5,opt,name=size,proto3" json:"size,omitempty"` // Whether the secret is an internal secret (not set by a user) or not. Internal bool `protobuf:"varint,6,opt,name=internal,proto3" json:"internal,omitempty"` } @@ -3851,7 +3851,7 @@ func (m *Secret) Unmarshal(data []byte) error { } b := data[iNdEx] iNdEx++ - m.SecretSize |= (uint32(b) & 0x7F) << shift + m.SecretSize |= (int64(b) & 0x7F) << shift if b < 0x80 { break } @@ -4070,9 +4070,9 @@ var fileDescriptorObjects = []byte{ 0x36, 0x17, 0xdf, 0xa6, 0xcb, 0x52, 0x5f, 0xa6, 0x88, 0xae, 0x06, 0x34, 0xff, 0x74, 0xa0, 0x74, 0x48, 0x43, 0x41, 0xd5, 0x5b, 0x2d, 0xf8, 0xa3, 0x0b, 0x05, 0xaf, 0xe7, 0xbf, 0xc5, 0xda, 0xeb, 0x4a, 0xbd, 0xb7, 0xa0, 0x14, 0xb1, 0x01, 0x95, 0xe9, 0xd7, 0x84, 0x87, 0xad, 0x85, 0x9a, 0xe0, - 0x4a, 0xf6, 0x9a, 0x9a, 0xce, 0xaa, 0xa5, 0x0f, 0x9f, 0x55, 0x60, 0xaf, 0x29, 0x36, 0x7b, 0x68, + 0x4a, 0xf6, 0x9a, 0x9a, 0xce, 0x2a, 0xa6, 0x0f, 0x9f, 0x55, 0x60, 0xaf, 0x29, 0x36, 0x7b, 0x68, 0x1b, 0x2a, 0x2c, 0x56, 0x54, 0xc4, 0x64, 0x64, 0x32, 0xaf, 0xe0, 0x85, 0xdd, 0xd9, 0x39, 0x3d, 0xab, 0xdf, 0xf8, 0xfd, 0xac, 0x7e, 0xe3, 0x9f, 0xb3, 0xba, 0xf3, 0xfd, 0xbc, 0xee, 0x9c, 0xce, 0xeb, 0xce, 0xaf, 0xf3, 0xba, 0xf3, 0xc7, 0xbc, 0xee, 0x1c, 0x97, 0xcc, 0xbf, 0x81, 0x8f, 0xfe, - 0x0d, 0x00, 0x00, 0xff, 0xff, 0xfd, 0x7d, 0x96, 0x72, 0x7d, 0x0c, 0x00, 0x00, + 0x0d, 0x00, 0x00, 0xff, 0xff, 0x4e, 0x76, 0x6f, 0x66, 0x7d, 0x0c, 0x00, 0x00, } diff --git a/api/objects.proto b/api/objects.proto index 56b3812a0d..ab47b1a6fb 100644 --- a/api/objects.proto +++ b/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; diff --git a/cmd/swarmctl/main.go b/cmd/swarmctl/main.go index 5210830260..afccb6c461 100644 --- a/cmd/swarmctl/main.go +++ b/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, ) } diff --git a/cmd/swarmctl/secrets/cmd.go b/cmd/swarmctl/secret/cmd.go similarity index 94% rename from cmd/swarmctl/secrets/cmd.go rename to cmd/swarmctl/secret/cmd.go index 0911e0ec34..50f7eb72eb 100644 --- a/cmd/swarmctl/secrets/cmd.go +++ b/cmd/swarmctl/secret/cmd.go @@ -1,4 +1,4 @@ -package secrets +package secret import "github.com/spf13/cobra" diff --git a/cmd/swarmctl/secrets/common.go b/cmd/swarmctl/secret/common.go similarity index 98% rename from cmd/swarmctl/secrets/common.go rename to cmd/swarmctl/secret/common.go index 03586d853a..b6d5b0d4c3 100644 --- a/cmd/swarmctl/secrets/common.go +++ b/cmd/swarmctl/secret/common.go @@ -1,4 +1,4 @@ -package secrets +package secret import ( "fmt" diff --git a/cmd/swarmctl/secrets/create.go b/cmd/swarmctl/secret/create.go similarity index 98% rename from cmd/swarmctl/secrets/create.go rename to cmd/swarmctl/secret/create.go index bcd3ba0e46..4214d18fc1 100644 --- a/cmd/swarmctl/secrets/create.go +++ b/cmd/swarmctl/secret/create.go @@ -1,4 +1,4 @@ -package secrets +package secret import ( "errors" diff --git a/cmd/swarmctl/secrets/inspect.go b/cmd/swarmctl/secret/inspect.go similarity index 86% rename from cmd/swarmctl/secrets/inspect.go rename to cmd/swarmctl/secret/inspect.go index 2e63e50b0a..e7dc82d702 100644 --- a/cmd/swarmctl/secrets/inspect.go +++ b/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 ( diff --git a/cmd/swarmctl/secrets/list.go b/cmd/swarmctl/secret/list.go similarity index 83% rename from cmd/swarmctl/secrets/list.go rename to cmd/swarmctl/secret/list.go index 3367dc92d2..7565407f2d 100644 --- a/cmd/swarmctl/secrets/list.go +++ b/cmd/swarmctl/secret/list.go @@ -1,4 +1,4 @@ -package secrets +package secret import ( "errors" @@ -6,10 +6,10 @@ import ( "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, diff --git a/cmd/swarmctl/secrets/remove.go b/cmd/swarmctl/secret/remove.go similarity index 97% rename from cmd/swarmctl/secrets/remove.go rename to cmd/swarmctl/secret/remove.go index 47cbb10720..517ae6e7d6 100644 --- a/cmd/swarmctl/secrets/remove.go +++ b/cmd/swarmctl/secret/remove.go @@ -1,4 +1,4 @@ -package secrets +package secret import ( "errors" diff --git a/manager/controlapi/secret.go b/manager/controlapi/secret.go index 1994a874be..4a2a04981e 100644 --- a/manager/controlapi/secret.go +++ b/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,8 +165,8 @@ 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 } @@ -177,7 +174,7 @@ func validateSecretSpec(spec *api.SecretSpec) error { 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]") diff --git a/manager/controlapi/secret_test.go b/manager/controlapi/secret_test.go index e9995afc37..53335af3ea 100644 --- a/manager/controlapi/secret_test.go +++ b/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)