From 99bde729672973cb913eff655802ae1ded129979 Mon Sep 17 00:00:00 2001 From: Andreas Holstenson Date: Sun, 14 Jan 2024 09:09:08 +0100 Subject: [PATCH] feat(state): Improve errors for validation failures --- internal/api/state/v1alpha1/server.go | 34 ++++++++- internal/state/errors.go | 25 +++++-- internal/state/manager.go | 75 ++++++++++++-------- internal/state/names.go | 14 ++++ proto/windshift/state/v1alpha1/service.proto | 57 +++++++++------ 5 files changed, 145 insertions(+), 60 deletions(-) create mode 100644 internal/state/names.go diff --git a/internal/api/state/v1alpha1/server.go b/internal/api/state/v1alpha1/server.go index 496dcb7..7046c16 100644 --- a/internal/api/state/v1alpha1/server.go +++ b/internal/api/state/v1alpha1/server.go @@ -9,6 +9,8 @@ import ( "go.opentelemetry.io/otel/propagation" "go.uber.org/fx" "go.uber.org/zap" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" "google.golang.org/protobuf/types/known/timestamppb" ) @@ -48,7 +50,14 @@ func (s *StateServiceServer) EnsureStore(ctx context.Context, req *statev1alpha1 err := s.state.EnsureStore(ctx, &state.StoreConfig{ Name: req.Store, }) - if err != nil { + + if errors.Is(err, context.Canceled) { + return nil, status.Error(codes.Canceled, "context canceled") + } else if errors.Is(err, context.DeadlineExceeded) { + return nil, status.Error(codes.DeadlineExceeded, "timed out") + } else if state.IsValidationError(err) { + return nil, status.Error(codes.InvalidArgument, err.Error()) + } else if err != nil { return nil, err } @@ -62,6 +71,12 @@ func (s *StateServiceServer) Get(ctx context.Context, req *statev1alpha1.GetRequ return &statev1alpha1.GetResponse{ Revision: 0, }, nil + } else if errors.Is(err, context.Canceled) { + return nil, status.Error(codes.Canceled, "context canceled") + } else if errors.Is(err, context.DeadlineExceeded) { + return nil, status.Error(codes.DeadlineExceeded, "timed out") + } else if state.IsValidationError(err) { + return nil, status.Error(codes.InvalidArgument, err.Error()) } else if err != nil { return nil, err } @@ -84,7 +99,13 @@ func (s *StateServiceServer) Set(ctx context.Context, req *statev1alpha1.SetRequ revision, err = s.state.Set(ctx, req.Store, req.Key, req.Value) } - if err != nil { + if errors.Is(err, context.Canceled) { + return nil, status.Error(codes.Canceled, "context canceled") + } else if errors.Is(err, context.DeadlineExceeded) { + return nil, status.Error(codes.DeadlineExceeded, "timed out") + } else if state.IsValidationError(err) { + return nil, status.Error(codes.InvalidArgument, err.Error()) + } else if err != nil { return nil, err } @@ -100,7 +121,14 @@ func (s *StateServiceServer) Delete(ctx context.Context, req *statev1alpha1.Dele } else { err = s.state.Delete(ctx, req.Store, req.Key) } - if err != nil { + + if errors.Is(err, context.Canceled) { + return nil, status.Error(codes.Canceled, "context canceled") + } else if errors.Is(err, context.DeadlineExceeded) { + return nil, status.Error(codes.DeadlineExceeded, "timed out") + } else if state.IsValidationError(err) { + return nil, status.Error(codes.InvalidArgument, err.Error()) + } else if err != nil { return nil, err } diff --git a/internal/state/errors.go b/internal/state/errors.go index 5e0a489..8e3755b 100644 --- a/internal/state/errors.go +++ b/internal/state/errors.go @@ -2,14 +2,8 @@ package state import "errors" -// ErrStoreRequired is returned when a store is not provided. -var ErrStoreRequired = errors.New("store required") - // ErrStoreNotFound is returned when a store is not found. -var ErrStoreNotFound = errors.New("store not found") - -// ErrKeyRequired is returned when a key is not provided. -var ErrKeyRequired = errors.New("missing key") +var ErrStoreNotFound = &validationError{err: "store not found"} // ErrKeyNotFound is returned when a key is not found in a store. var ErrKeyNotFound = errors.New("key not found") @@ -21,3 +15,20 @@ var ErrKeyAlreadyExists = errors.New("key already exists") // ErrRevisionMismatch is returned when a revision does not match the current // revision of a key. var ErrRevisionMismatch = errors.New("revision mismatch") + +type validationError struct { + err string +} + +func (e *validationError) Error() string { + return e.err +} + +func newValidationError(err string) error { + return &validationError{err: err} +} + +func IsValidationError(err error) bool { + _, ok := err.(*validationError) + return ok +} diff --git a/internal/state/manager.go b/internal/state/manager.go index 0355202..a78b0cf 100644 --- a/internal/state/manager.go +++ b/internal/state/manager.go @@ -2,7 +2,6 @@ package state import ( "context" - "strings" "time" "github.com/cockroachdb/errors" @@ -82,13 +81,12 @@ func (m *Manager) EnsureStore(ctx context.Context, config *StoreConfig) error { ) defer span.End() - storeName := strings.TrimSpace(config.Name) - if storeName == "" { - span.SetStatus(codes.Error, "store required") - return errors.WithStack(ErrStoreRequired) + if !IsValidStoreName(config.Name) { + span.SetStatus(codes.Error, "invalid store name") + return newValidationError("invalid store name: " + config.Name) } - _, err := m.stores.Get(ctx, storeName) + _, err := m.stores.Get(ctx, config.Name) if errors.Is(err, ErrStoreNotFound) { _, err = m.js.CreateKeyValue(ctx, jetstream.KeyValueConfig{ Bucket: config.Name, @@ -124,17 +122,10 @@ func (m *Manager) Get(ctx context.Context, store string, key string) (*Entry, er ) defer span.End() - store = strings.TrimSpace(store) - key = strings.TrimSpace(key) - - if store == "" { - span.SetStatus(codes.Error, "store required") - return nil, errors.WithStack(ErrStoreRequired) - } - - if key == "" { - span.SetStatus(codes.Error, "key required") - return nil, errors.WithStack(ErrKeyRequired) + err := validatePreconditions(store, key) + if err != nil { + span.SetStatus(codes.Error, err.Error()) + return nil, err } bucket, err := m.stores.Get(ctx, store) @@ -187,17 +178,10 @@ func (m *Manager) Create(ctx context.Context, store string, key string, value *a ) defer span.End() - store = strings.TrimSpace(store) - key = strings.TrimSpace(key) - - if store == "" { - span.SetStatus(codes.Error, "store required") - return 0, errors.WithStack(ErrStoreRequired) - } - - if key == "" { - span.SetStatus(codes.Error, "key required") - return 0, errors.WithStack(ErrKeyRequired) + err := validatePreconditions(store, key) + if err != nil { + span.SetStatus(codes.Error, err.Error()) + return 0, err } bucket, err := m.stores.Get(ctx, store) @@ -244,6 +228,12 @@ func (m *Manager) Set(ctx context.Context, store string, key string, value *anyp ) defer span.End() + err := validatePreconditions(store, key) + if err != nil { + span.SetStatus(codes.Error, err.Error()) + return 0, err + } + bucket, err := m.stores.Get(ctx, store) if err != nil { span.RecordError(err) @@ -286,6 +276,12 @@ func (m *Manager) Update(ctx context.Context, store string, key string, value *a ) defer span.End() + err := validatePreconditions(store, key) + if err != nil { + span.SetStatus(codes.Error, err.Error()) + return 0, err + } + bucket, err := m.stores.Get(ctx, store) if err != nil { span.RecordError(err) @@ -339,6 +335,12 @@ func (m *Manager) Delete(ctx context.Context, store string, key string) error { ) defer span.End() + err := validatePreconditions(store, key) + if err != nil { + span.SetStatus(codes.Error, err.Error()) + return err + } + bucket, err := m.stores.Get(ctx, store) if err != nil { span.RecordError(err) @@ -373,6 +375,12 @@ func (m *Manager) DeleteWithRevision(ctx context.Context, store string, key stri ) defer span.End() + err := validatePreconditions(store, key) + if err != nil { + span.SetStatus(codes.Error, err.Error()) + return err + } + bucket, err := m.stores.Get(ctx, store) if err != nil { span.RecordError(err) @@ -390,3 +398,14 @@ func (m *Manager) DeleteWithRevision(ctx context.Context, store string, key stri span.SetStatus(codes.Ok, "") return nil } + +func validatePreconditions(store string, key string) error { + if !IsValidStoreName(store) { + return newValidationError("invalid store name: " + store) + } + + if !IsValidKey(key) { + return newValidationError("invalid key: " + key) + } + return nil +} diff --git a/internal/state/names.go b/internal/state/names.go new file mode 100644 index 0000000..b771eec --- /dev/null +++ b/internal/state/names.go @@ -0,0 +1,14 @@ +package state + +import "windshift/service/internal/events" + +// IsValidStoreName checks if the store name is valid. +func IsValidStoreName(name string) bool { + return events.IsValidStreamName(name) +} + +// IsValidKey checks if the key name is valid. The NATS documentation says +// that keys follow the same rules as subjects so we delegate to events.IsValidSubject. +func IsValidKey(name string) bool { + return events.IsValidSubject(name, false) +} diff --git a/proto/windshift/state/v1alpha1/service.proto b/proto/windshift/state/v1alpha1/service.proto index fc9cb63..905c49a 100644 --- a/proto/windshift/state/v1alpha1/service.proto +++ b/proto/windshift/state/v1alpha1/service.proto @@ -5,59 +5,66 @@ package windshift.state.v1alpha1; import "google/protobuf/any.proto"; import "google/protobuf/timestamp.proto"; -/** +/* * StateService is a service for storing and retrieving arbitrary data. It can * be used by clients to store and retrieve stateful information, providing a * a convenient way to store data without having to manage a database. */ service StateService { - /** + /* * EnsureStore ensures that a store exists. If the store does not exist, it * will be created. + * + * Stores are used to collect related data. For example, a store could be + * used to store the state of a single application. */ rpc EnsureStore(EnsureStoreRequest) returns (EnsureStoreResponse); - /** + /* * Get retrieves the value of a key in a store. */ rpc Get(GetRequest) returns (GetResponse); - /** + /* * Set sets the value of a key in a store. */ rpc Set(SetRequest) returns (SetResponse); - /** + /* * Delete deletes a key from a store. */ rpc Delete(DeleteRequest) returns (DeleteResponse); } -/** +/* * EnsureStoreRequest creates or updates a state store. */ message EnsureStoreRequest { - /** - * The name of the store to create or update. + /* + * The name of the store to create or update. Store names are case-sensitive + * and should only contain the following characters: + * + * - `a` to `z`, `A` to `Z` and `0` to `9` are allowed. + * - `_` and `-` are allowed for separating words. */ string store = 1; } message EnsureStoreResponse {} -/** +/* * GetRequest is the message sent to retrieve a value from a store. */ message GetRequest { - /** + /* * Store to retrieve the value from. */ string store = 1; - /** + /* * Key to retrieve the value for. */ string key = 2; } -/** +/* * GetResponse is the message returned when retrieving a value from a store. */ message GetResponse { @@ -76,25 +83,31 @@ message GetResponse { } message SetRequest { - /** + /* * Store to set the value in. */ string store = 1; - /** - * Key to set the value for. + /* + * Key to set the value for. Keys are case-sensitive and should only contain + * the following characters: + * + * - `a` to `z`, `A` to `Z` and `0` to `9` are allowed. + * - `_` and `-` are allowed for separating words, but the use of camelCase + * is recommended. + * - `.` is allowed and used a hierarchy separator. */ string key = 2; - /** + /* * Value to set. */ google.protobuf.Any value = 3; - /** + /* * If set the operation will only succeed if the key does not already * exist in the store. */ optional bool create_only = 4; - /** + /* * If set the operation will only succeed if the current revision of the * key matches the given revision. */ @@ -102,23 +115,23 @@ message SetRequest { } message SetResponse { - /** + /* * The revision of the key. */ uint64 revision = 1; } message DeleteRequest { - /** + /* * Store to delete the key from. */ string store = 1; - /** + /* * Key to delete. */ string key = 2; - /** + /* * If set the operation will only succeed if the current revision of the * key matches the given revision. */