From ccf75cf91e6e2ab3d023ca8130c0963ecd5a36b5 Mon Sep 17 00:00:00 2001 From: Linas Medziunas Date: Wed, 18 Nov 2020 16:04:16 +0200 Subject: [PATCH 1/6] [coordinator] New namespace validation hook --- src/dbnode/storage/types.go | 2 +- src/query/api/v1/handler/database/common.go | 4 +- src/query/api/v1/handler/database/create.go | 8 ++- .../api/v1/handler/database/create_test.go | 25 +++---- src/query/api/v1/handler/namespace/add.go | 22 +++++-- .../api/v1/handler/namespace/add_test.go | 65 +++++++++++++++++-- src/query/api/v1/handler/namespace/common.go | 4 +- src/query/api/v1/handler/namespace/update.go | 2 - .../api/v1/handler/namespace/update_test.go | 6 +- src/query/api/v1/httpd/handler.go | 11 +++- src/query/api/v1/options/handler.go | 34 +++++++++- src/query/server/query.go | 2 +- src/query/ts/m3db/options.go | 8 +-- 13 files changed, 148 insertions(+), 45 deletions(-) diff --git a/src/dbnode/storage/types.go b/src/dbnode/storage/types.go index 579cae8c7d..2aa53c12ab 100644 --- a/src/dbnode/storage/types.go +++ b/src/dbnode/storage/types.go @@ -1408,7 +1408,7 @@ type NewTileAggregatorFn func(iOpts instrument.Options) TileAggregator // NamespaceHooks allows dynamic plugging into the namespace lifecycle. type NamespaceHooks interface { - // OnCreatedNamespace gets invoked after each namespace is created. + // OnCreatedNamespace gets invoked after each namespace is initialized. OnCreatedNamespace(Namespace, GetNamespaceFn) error } diff --git a/src/query/api/v1/handler/database/common.go b/src/query/api/v1/handler/database/common.go index 950bed6396..dc17007e85 100644 --- a/src/query/api/v1/handler/database/common.go +++ b/src/query/api/v1/handler/database/common.go @@ -26,6 +26,7 @@ import ( "github.com/m3db/m3/src/cmd/services/m3query/config" "github.com/m3db/m3/src/query/api/v1/handler" "github.com/m3db/m3/src/query/api/v1/handler/prometheus/handleroptions" + "github.com/m3db/m3/src/query/api/v1/options" "github.com/m3db/m3/src/x/instrument" ) @@ -45,9 +46,10 @@ func RegisterRoutes( embeddedDbCfg *dbconfig.DBConfiguration, defaults []handleroptions.ServiceOptionsDefault, instrumentOpts instrument.Options, + hooks options.NamespaceHooks, ) error { createHandler, err := NewCreateHandler(client, cfg, embeddedDbCfg, - defaults, instrumentOpts) + defaults, instrumentOpts, hooks) if err != nil { return err } diff --git a/src/query/api/v1/handler/database/create.go b/src/query/api/v1/handler/database/create.go index de9e7054f8..3dfe4983ac 100644 --- a/src/query/api/v1/handler/database/create.go +++ b/src/query/api/v1/handler/database/create.go @@ -40,6 +40,7 @@ import ( "github.com/m3db/m3/src/query/api/v1/handler/namespace" "github.com/m3db/m3/src/query/api/v1/handler/placement" "github.com/m3db/m3/src/query/api/v1/handler/prometheus/handleroptions" + "github.com/m3db/m3/src/query/api/v1/options" "github.com/m3db/m3/src/query/generated/proto/admin" "github.com/m3db/m3/src/query/util/logging" xerrors "github.com/m3db/m3/src/x/errors" @@ -148,6 +149,7 @@ func NewCreateHandler( embeddedDbCfg *dbconfig.DBConfiguration, defaults []handleroptions.ServiceOptionsDefault, instrumentOpts instrument.Options, + namespaceHooks options.NamespaceHooks, ) (http.Handler, error) { placementHandlerOptions, err := placement.NewHandlerOptions(client, cfg, nil, instrumentOpts) @@ -157,7 +159,7 @@ func NewCreateHandler( return &createHandler{ placementInitHandler: placement.NewInitHandler(placementHandlerOptions), placementGetHandler: placement.NewGetHandler(placementHandlerOptions), - namespaceAddHandler: namespace.NewAddHandler(client, instrumentOpts), + namespaceAddHandler: namespace.NewAddHandler(client, instrumentOpts, namespaceHooks), namespaceGetHandler: namespace.NewGetHandler(client, instrumentOpts), namespaceDeleteHandler: namespace.NewDeleteHandler(client, instrumentOpts), embeddedDbCfg: embeddedDbCfg, @@ -316,7 +318,7 @@ func (h *createHandler) parseAndValidateRequest( ) (*admin.DatabaseCreateRequest, []*admin.NamespaceAddRequest, *admin.PlacementInitRequest, error) { requirePlacement := existingPlacement == nil - defer r.Body.Close() + defer r.Body.Close() //nolint:errcheck rBody, err := xhttp.DurationToNanosBytes(r.Body) if err != nil { wrapped := fmt.Errorf("error converting duration to nano bytes: %s", err.Error()) @@ -582,7 +584,7 @@ func defaultedPlacementInitRequest( numShards = shardMultiplier replicationFactor = 1 instances = []*placementpb.Instance{ - &placementpb.Instance{ + { Id: DefaultLocalHostID, IsolationGroup: DefaultLocalIsolationGroup, Zone: DefaultLocalZone, diff --git a/src/query/api/v1/handler/database/create_test.go b/src/query/api/v1/handler/database/create_test.go index 35f46b2f43..1cc58fb1a9 100644 --- a/src/query/api/v1/handler/database/create_test.go +++ b/src/query/api/v1/handler/database/create_test.go @@ -40,6 +40,7 @@ import ( "github.com/m3db/m3/src/cmd/services/m3query/config" "github.com/m3db/m3/src/query/api/v1/handler/namespace" "github.com/m3db/m3/src/query/api/v1/handler/prometheus/handleroptions" + "github.com/m3db/m3/src/query/api/v1/options" "github.com/m3db/m3/src/x/instrument" xjson "github.com/m3db/m3/src/x/json" xtest "github.com/m3db/m3/src/x/test" @@ -101,7 +102,7 @@ func testLocalType(t *testing.T, providedType string, placementExists bool) { mockClient, mockKV, mockPlacementService := SetupDatabaseTest(t, ctrl) mockClient.EXPECT().Store(gomock.Any()).Return(mockKV, nil).AnyTimes() createHandler, err := NewCreateHandler(mockClient, config.Configuration{}, - testDBCfg, svcDefaultOptions, instrument.NewOptions()) + testDBCfg, svcDefaultOptions, instrument.NewOptions(), options.NoopNamespaceHooks) require.NoError(t, err) w := httptest.NewRecorder() @@ -234,7 +235,7 @@ func TestLocalTypeClusteredPlacementAlreadyExists(t *testing.T) { mockClient, _, mockPlacementService := SetupDatabaseTest(t, ctrl) createHandler, err := NewCreateHandler(mockClient, config.Configuration{}, - testDBCfg, svcDefaultOptions, instrument.NewOptions()) + testDBCfg, svcDefaultOptions, instrument.NewOptions(), options.NoopNamespaceHooks) require.NoError(t, err) w := httptest.NewRecorder() @@ -280,7 +281,7 @@ func TestLocalTypeWithNumShards(t *testing.T) { mockClient, mockKV, mockPlacementService := SetupDatabaseTest(t, ctrl) mockClient.EXPECT().Store(gomock.Any()).Return(mockKV, nil).AnyTimes() createHandler, err := NewCreateHandler(mockClient, config.Configuration{}, - testDBCfg, svcDefaultOptions, instrument.NewOptions()) + testDBCfg, svcDefaultOptions, instrument.NewOptions(), options.NoopNamespaceHooks) require.NoError(t, err) w := httptest.NewRecorder() @@ -410,7 +411,7 @@ func TestLocalWithBlockSizeNanos(t *testing.T) { mockClient, mockKV, mockPlacementService := SetupDatabaseTest(t, ctrl) mockClient.EXPECT().Store(gomock.Any()).Return(mockKV, nil).AnyTimes() createHandler, err := NewCreateHandler(mockClient, config.Configuration{}, - testDBCfg, svcDefaultOptions, instrument.NewOptions()) + testDBCfg, svcDefaultOptions, instrument.NewOptions(), options.NoopNamespaceHooks) require.NoError(t, err) w := httptest.NewRecorder() @@ -539,7 +540,7 @@ func TestLocalWithBlockSizeExpectedSeriesDatapointsPerHour(t *testing.T) { mockClient, mockKV, mockPlacementService := SetupDatabaseTest(t, ctrl) mockClient.EXPECT().Store(gomock.Any()).Return(mockKV, nil).AnyTimes() createHandler, err := NewCreateHandler(mockClient, config.Configuration{}, - testDBCfg, svcDefaultOptions, instrument.NewOptions()) + testDBCfg, svcDefaultOptions, instrument.NewOptions(), options.NoopNamespaceHooks) require.NoError(t, err) w := httptest.NewRecorder() @@ -682,7 +683,7 @@ func TestClusterTypeHostsPlacementAlreadyExistsHostsProvided(t *testing.T) { mockClient, _, mockPlacementService := SetupDatabaseTest(t, ctrl) mockClient.EXPECT().Store(gomock.Any()).Return(nil, nil).AnyTimes() createHandler, err := NewCreateHandler(mockClient, config.Configuration{}, - testDBCfg, svcDefaultOptions, instrument.NewOptions()) + testDBCfg, svcDefaultOptions, instrument.NewOptions(), options.NoopNamespaceHooks) require.NoError(t, err) w := httptest.NewRecorder() @@ -737,7 +738,7 @@ func TestClusterTypeHostsPlacementAlreadyExistsExistingIsLocal(t *testing.T) { mockClient, _, mockPlacementService := SetupDatabaseTest(t, ctrl) createHandler, err := NewCreateHandler(mockClient, config.Configuration{}, - testDBCfg, svcDefaultOptions, instrument.NewOptions()) + testDBCfg, svcDefaultOptions, instrument.NewOptions(), options.NoopNamespaceHooks) require.NoError(t, err) w := httptest.NewRecorder() @@ -783,7 +784,7 @@ func testClusterTypeHosts(t *testing.T, placementExists bool) { mockClient, mockKV, mockPlacementService := SetupDatabaseTest(t, ctrl) mockClient.EXPECT().Store(gomock.Any()).Return(mockKV, nil).AnyTimes() createHandler, err := NewCreateHandler(mockClient, config.Configuration{}, - testDBCfg, svcDefaultOptions, instrument.NewOptions()) + testDBCfg, svcDefaultOptions, instrument.NewOptions(), options.NoopNamespaceHooks) require.NoError(t, err) w := httptest.NewRecorder() @@ -953,7 +954,7 @@ func TestClusterTypeHostsWithIsolationGroup(t *testing.T) { mockClient.EXPECT().Store(gomock.Any()).Return(mockKV, nil).AnyTimes() createHandler, err := NewCreateHandler(mockClient, config.Configuration{}, - testDBCfg, svcDefaultOptions, instrument.NewOptions()) + testDBCfg, svcDefaultOptions, instrument.NewOptions(), options.NoopNamespaceHooks) require.NoError(t, err) w := httptest.NewRecorder() @@ -1109,7 +1110,7 @@ func TestClusterTypeMissingHostnames(t *testing.T) { mockPlacementService.EXPECT().Placement().Return(nil, kv.ErrNotFound) createHandler, err := NewCreateHandler(mockClient, config.Configuration{}, - testDBCfg, svcDefaultOptions, instrument.NewOptions()) + testDBCfg, svcDefaultOptions, instrument.NewOptions(), options.NoopNamespaceHooks) require.NoError(t, err) w := httptest.NewRecorder() @@ -1145,7 +1146,7 @@ func TestBadType(t *testing.T) { mockPlacementService.EXPECT().Placement().Return(nil, kv.ErrNotFound) createHandler, err := NewCreateHandler(mockClient, config.Configuration{}, - nil, svcDefaultOptions, instrument.NewOptions()) + nil, svcDefaultOptions, instrument.NewOptions(), options.NoopNamespaceHooks) require.NoError(t, err) w := httptest.NewRecorder() @@ -1180,7 +1181,7 @@ func TestLocalTypeWithAggregatedNamespace(t *testing.T) { fakeKV := fake.NewStore() mockClient.EXPECT().Store(gomock.Any()).Return(fakeKV, nil).AnyTimes() createHandler, err := NewCreateHandler(mockClient, config.Configuration{}, - testDBCfg, svcDefaultOptions, instrument.NewOptions()) + testDBCfg, svcDefaultOptions, instrument.NewOptions(), options.NoopNamespaceHooks) require.NoError(t, err) w := httptest.NewRecorder() diff --git a/src/query/api/v1/handler/namespace/add.go b/src/query/api/v1/handler/namespace/add.go index 85ebdf108c..4d936eef44 100644 --- a/src/query/api/v1/handler/namespace/add.go +++ b/src/query/api/v1/handler/namespace/add.go @@ -32,6 +32,7 @@ import ( "github.com/m3db/m3/src/dbnode/namespace" "github.com/m3db/m3/src/query/api/v1/handler" "github.com/m3db/m3/src/query/api/v1/handler/prometheus/handleroptions" + "github.com/m3db/m3/src/query/api/v1/options" "github.com/m3db/m3/src/query/generated/proto/admin" "github.com/m3db/m3/src/query/util/logging" xerrors "github.com/m3db/m3/src/x/errors" @@ -53,16 +54,24 @@ var ( ) // AddHandler is the handler for namespace adds. -type AddHandler Handler +type AddHandler struct { + Handler + + hooks options.NamespaceHooks +} // NewAddHandler returns a new instance of AddHandler. func NewAddHandler( client clusterclient.Client, instrumentOpts instrument.Options, + hooks options.NamespaceHooks, ) *AddHandler { return &AddHandler{ - client: client, - instrumentOpts: instrumentOpts, + Handler: Handler{ + client: client, + instrumentOpts: instrumentOpts, + }, + hooks: hooks, } } @@ -90,7 +99,7 @@ func (h *AddHandler) ServeHTTP( return } - logger.Error("unable to get namespace", zap.Error(err)) + logger.Error("unable to add namespace", zap.Error(err)) xhttp.WriteError(w, err) return } @@ -143,6 +152,11 @@ func (h *AddHandler) Add( return emptyReg, err } + if err := h.hooks.ValidateNewNamespace(md, currentMetadata); err != nil { + return emptyReg, xerrors.NewInvalidParamsError( + fmt.Errorf("invalid new namespace metadata: %v", err)) + } + // Since this endpoint is `/add` and not in-place update, return an error if // the NS already exists. NewMap will return an error if there's duplicate // entries with the same name, but it's abstracted away behind a MultiError so diff --git a/src/query/api/v1/handler/namespace/add_test.go b/src/query/api/v1/handler/namespace/add_test.go index d0aae7c984..2ccebf2e13 100644 --- a/src/query/api/v1/handler/namespace/add_test.go +++ b/src/query/api/v1/handler/namespace/add_test.go @@ -21,6 +21,7 @@ package namespace import ( + "errors" "io/ioutil" "net/http" "net/http/httptest" @@ -28,9 +29,11 @@ import ( "testing" "github.com/gogo/protobuf/jsonpb" + "github.com/m3db/m3/src/cluster/kv" nsproto "github.com/m3db/m3/src/dbnode/generated/proto/namespace" "github.com/m3db/m3/src/dbnode/namespace" + "github.com/m3db/m3/src/query/api/v1/options" "github.com/m3db/m3/src/query/generated/proto/admin" "github.com/m3db/m3/src/x/instrument" xjson "github.com/m3db/m3/src/x/json" @@ -79,7 +82,7 @@ func TestNamespaceAddHandler(t *testing.T) { defer ctrl.Finish() mockClient, mockKV := setupNamespaceTest(t, ctrl) - addHandler := NewAddHandler(mockClient, instrument.NewOptions()) + addHandler := NewAddHandler(mockClient, instrument.NewOptions(), options.NoopNamespaceHooks) mockClient.EXPECT().Store(gomock.Any()).Return(mockKV, nil) // Error case where required fields are not set @@ -164,7 +167,7 @@ func TestNamespaceAddHandler_Conflict(t *testing.T) { defer ctrl.Finish() mockClient, mockKV := setupNamespaceTest(t, ctrl) - addHandler := NewAddHandler(mockClient, instrument.NewOptions()) + addHandler := NewAddHandler(mockClient, instrument.NewOptions(), options.NoopNamespaceHooks) mockClient.EXPECT().Store(gomock.Any()).Return(mockKV, nil) // Ensure adding an existing namespace returns 409 @@ -173,7 +176,51 @@ func TestNamespaceAddHandler_Conflict(t *testing.T) { registry := nsproto.Registry{ Namespaces: map[string]*nsproto.NamespaceOptions{ - "testNamespace": &nsproto.NamespaceOptions{ + "testNamespace": { + BootstrapEnabled: true, + FlushEnabled: true, + SnapshotEnabled: true, + WritesToCommitLog: true, + CleanupEnabled: false, + RepairEnabled: false, + RetentionOptions: &nsproto.RetentionOptions{ + RetentionPeriodNanos: 172800000000000, + BlockSizeNanos: 7200000000000, + BufferFutureNanos: 600000000000, + BufferPastNanos: 600000000000, + BlockDataExpiry: true, + BlockDataExpiryAfterNotAccessPeriodNanos: 3600000000000, + }, + }, + }, + } + + mockValue := kv.NewMockValue(ctrl) + mockValue.EXPECT().Unmarshal(gomock.Any()).Return(nil).SetArg(0, registry) + mockValue.EXPECT().Version().Return(0) + mockKV.EXPECT().Get(M3DBNodeNamespacesKey).Return(mockValue, nil) + + w := httptest.NewRecorder() + addHandler.ServeHTTP(svcDefaults, w, req) + resp := w.Result() + assert.Equal(t, http.StatusConflict, resp.StatusCode) +} + +func TestApplyHookValidator(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + mockClient, mockKV := setupNamespaceTest(t, ctrl) + hooks := &testNamespaceHooks{} + addHandler := NewAddHandler(mockClient, instrument.NewOptions(), hooks) + mockClient.EXPECT().Store(gomock.Any()).Return(mockKV, nil) + + req := httptest.NewRequest("POST", "/namespace", strings.NewReader(testAddJSON)) + require.NotNil(t, req) + + registry := nsproto.Registry{ + Namespaces: map[string]*nsproto.NamespaceOptions{ + "firstNamespace": { BootstrapEnabled: true, FlushEnabled: true, SnapshotEnabled: true, @@ -200,7 +247,8 @@ func TestNamespaceAddHandler_Conflict(t *testing.T) { w := httptest.NewRecorder() addHandler.ServeHTTP(svcDefaults, w, req) resp := w.Result() - assert.Equal(t, http.StatusConflict, resp.StatusCode) + assert.Equal(t, http.StatusBadRequest, resp.StatusCode) + assert.Equal(t, 1, hooks.invocationCount) } func TestValidateNewMetadata(t *testing.T) { @@ -240,3 +288,12 @@ func TestValidateNewMetadata(t *testing.T) { require.Error(t, err) require.Equal(t, "index and retention block size must match (2h0m0s, 4h0m0s)", err.Error()) } + +type testNamespaceHooks struct { + invocationCount int +} + +func (h *testNamespaceHooks) ValidateNewNamespace(namespace.Metadata, []namespace.Metadata) error { + h.invocationCount++ + return errors.New("expected validation error") +} diff --git a/src/query/api/v1/handler/namespace/common.go b/src/query/api/v1/handler/namespace/common.go index 8290490395..c685c49f79 100644 --- a/src/query/api/v1/handler/namespace/common.go +++ b/src/query/api/v1/handler/namespace/common.go @@ -33,6 +33,7 @@ import ( "github.com/m3db/m3/src/dbnode/namespace" "github.com/m3db/m3/src/query/api/v1/handler" "github.com/m3db/m3/src/query/api/v1/handler/prometheus/handleroptions" + "github.com/m3db/m3/src/query/api/v1/options" "github.com/m3db/m3/src/query/storage/m3" "github.com/m3db/m3/src/x/instrument" xhttp "github.com/m3db/m3/src/x/net/http" @@ -119,6 +120,7 @@ func RegisterRoutes( clusters m3.Clusters, defaults []handleroptions.ServiceOptionsDefault, instrumentOpts instrument.Options, + hooks options.NamespaceHooks, ) error { addRoute := applyMiddlewareToRoute(addRouteFn, defaults) @@ -129,7 +131,7 @@ func RegisterRoutes( } // Add M3DB namespaces. - addHandler := NewAddHandler(client, instrumentOpts).ServeHTTP + addHandler := NewAddHandler(client, instrumentOpts, hooks).ServeHTTP if err := addRoute(M3DBAddURL, addHandler, AddHTTPMethod); err != nil { return err } diff --git a/src/query/api/v1/handler/namespace/update.go b/src/query/api/v1/handler/namespace/update.go index 01eb3eb3b2..5dd152a74e 100644 --- a/src/query/api/v1/handler/namespace/update.go +++ b/src/query/api/v1/handler/namespace/update.go @@ -54,7 +54,6 @@ var ( fieldNameRetentionPeriod = "RetentionPeriodNanos" fieldNameRuntimeOptions = "RuntimeOptions" fieldNameAggregationOptions = "AggregationOptions" - fieldNameExtendedOptions = "ExtendedOptions" errEmptyNamespaceName = errors.New("must specify namespace name") errEmptyNamespaceOptions = errors.New("update options cannot be empty") @@ -64,7 +63,6 @@ var ( fieldNameRetentionOptions: {}, fieldNameRuntimeOptions: {}, fieldNameAggregationOptions: {}, - fieldNameExtendedOptions: {}, } ) diff --git a/src/query/api/v1/handler/namespace/update_test.go b/src/query/api/v1/handler/namespace/update_test.go index 988c14b1ce..38ba2282ab 100644 --- a/src/query/api/v1/handler/namespace/update_test.go +++ b/src/query/api/v1/handler/namespace/update_test.go @@ -62,10 +62,6 @@ const ( } } ] - }, - "extendedOptions": { - "@type": "testm3db.io/m3.test.PingResponse", - "Value": "bar" } } } @@ -199,7 +195,7 @@ func TestNamespaceUpdateHandler(t *testing.T) { "schemaOptions": nil, "stagingState": xjson.Map{"status": "UNKNOWN"}, "coldWritesEnabled": false, - "extendedOptions": xtest.NewExtendedOptionsJson("bar"), + "extendedOptions": xtest.NewExtendedOptionsJson("foo"), }, }, }, diff --git a/src/query/api/v1/httpd/handler.go b/src/query/api/v1/httpd/handler.go index 8e188b6a74..35fd7efe7a 100644 --- a/src/query/api/v1/httpd/handler.go +++ b/src/query/api/v1/httpd/handler.go @@ -50,7 +50,7 @@ import ( "github.com/gorilla/mux" "github.com/opentracing-contrib/go-stdlib/nethttp" - opentracing "github.com/opentracing/opentracing-go" + "github.com/opentracing/opentracing-go" "github.com/prometheus/prometheus/util/httputil" "go.uber.org/zap" ) @@ -336,17 +336,22 @@ func (h *Handler) RegisterRoutes() error { if clusterClient != nil { if err := database.RegisterRoutes(wrappedRouteFn, clusterClient, h.options.Config(), h.options.EmbeddedDbCfg(), - serviceOptionDefaults, instrumentOpts); err != nil { + serviceOptionDefaults, instrumentOpts, + h.options.NamespaceHooks()); err != nil { return err } + if err := placement.RegisterRoutes(routeFn, serviceOptionDefaults, placementOpts); err != nil { return err } + if err := namespace.RegisterRoutes(wrappedRouteFn, clusterClient, - h.options.Clusters(), serviceOptionDefaults, instrumentOpts); err != nil { + h.options.Clusters(), serviceOptionDefaults, instrumentOpts, + h.options.NamespaceHooks()); err != nil { return err } + if err := topic.RegisterRoutes(wrappedRouteFn, clusterClient, config, instrumentOpts); err != nil { return err diff --git a/src/query/api/v1/options/handler.go b/src/query/api/v1/options/handler.go index a178b1bd33..da3da81a38 100644 --- a/src/query/api/v1/options/handler.go +++ b/src/query/api/v1/options/handler.go @@ -30,6 +30,7 @@ import ( "github.com/m3db/m3/src/cmd/services/m3coordinator/ingest" dbconfig "github.com/m3db/m3/src/cmd/services/m3dbnode/config" "github.com/m3db/m3/src/cmd/services/m3query/config" + "github.com/m3db/m3/src/dbnode/namespace" "github.com/m3db/m3/src/query/api/v1/handler/prometheus" "github.com/m3db/m3/src/query/api/v1/handler/prometheus/handleroptions" "github.com/m3db/m3/src/query/executor" @@ -212,9 +213,13 @@ type HandlerOptions interface { // SetStoreMetricsType enables/disables storing of metrics type. SetStoreMetricsType(value bool) HandlerOptions - // StoreMetricsType returns true if storing of metrics type is enabled. StoreMetricsType() bool + + // SetNamespaceHooks sets the NamespaceHooks. + SetNamespaceHooks(NamespaceHooks) HandlerOptions + // NamespaceHooks returns the NamespaceHooks. + NamespaceHooks() NamespaceHooks } // HandlerOptions represents handler options. @@ -242,6 +247,7 @@ type handlerOptions struct { instantQueryRouter QueryRouter graphiteStorageOpts graphite.M3WrappedStorageOptions m3dbOpts m3db.Options + namespaceHooks NamespaceHooks storeMetricsType bool } @@ -314,6 +320,7 @@ func NewHandlerOptions( graphiteStorageOpts: graphiteStorageOpts, m3dbOpts: m3dbOpts, storeMetricsType: storeMetricsType, + namespaceHooks: NoopNamespaceHooks, }, nil } @@ -575,3 +582,28 @@ func (o *handlerOptions) SetStoreMetricsType(value bool) HandlerOptions { func (o *handlerOptions) StoreMetricsType() bool { return o.storeMetricsType } + +func (o *handlerOptions) SetNamespaceHooks(value NamespaceHooks) HandlerOptions { + opts := *o + opts.namespaceHooks = value + return &opts +} + +func (o *handlerOptions) NamespaceHooks() NamespaceHooks { + return o.namespaceHooks +} + +// NamespaceHooks allows dynamic plugging into the namespace lifecycle. +type NamespaceHooks interface { + // ValidateNewNamespace gets invoked when creating a new namespace. + ValidateNewNamespace(newNs namespace.Metadata, existing []namespace.Metadata) error +} + +// NoopNamespaceHooks is an instance of noopNamespaceHooks. +var NoopNamespaceHooks = &noopNamespaceHooks{} + +type noopNamespaceHooks struct{} + +func (h *noopNamespaceHooks) ValidateNewNamespace(namespace.Metadata, []namespace.Metadata) error { + return nil +} diff --git a/src/query/server/query.go b/src/query/server/query.go index 8dfdfa407b..4c5524c5ec 100644 --- a/src/query/server/query.go +++ b/src/query/server/query.go @@ -72,7 +72,7 @@ import ( "github.com/go-kit/kit/log" kitlogzap "github.com/go-kit/kit/log/zap" - opentracing "github.com/opentracing/opentracing-go" + "github.com/opentracing/opentracing-go" "github.com/pkg/errors" extprom "github.com/prometheus/client_golang/prometheus" prometheuspromql "github.com/prometheus/prometheus/promql" diff --git a/src/query/ts/m3db/options.go b/src/query/ts/m3db/options.go index a808e62647..43867c654e 100644 --- a/src/query/ts/m3db/options.go +++ b/src/query/ts/m3db/options.go @@ -69,16 +69,10 @@ type encodedBlockOptions struct { instrumented bool } -type nextDetails struct { - peek peekValue - iter encoding.SeriesIterator - collector consolidators.StepCollector -} - // NewOptions creates a default encoded block options which dictates how // encoded blocks are generated. func NewOptions() Options { - bytesPool := pool.NewCheckedBytesPool([]pool.Bucket{pool.Bucket{ + bytesPool := pool.NewCheckedBytesPool([]pool.Bucket{{ Capacity: defaultCapacity, Count: defaultCount, }}, nil, func(s []pool.Bucket) pool.BytesPool { From 7fa4f8442d06f9766be958d8d0c231b2e7f97da2 Mon Sep 17 00:00:00 2001 From: Linas Medziunas Date: Tue, 24 Nov 2020 10:18:08 +0200 Subject: [PATCH 2/6] Extract new namespace validation logics --- src/dbnode/namespace/convert_test.go | 28 ++++++-- src/query/api/v1/handler/database/common.go | 2 +- src/query/api/v1/handler/database/create.go | 4 +- .../api/v1/handler/database/create_test.go | 26 ++++---- src/query/api/v1/handler/namespace/add.go | 49 +++----------- .../api/v1/handler/namespace/add_test.go | 65 ++++--------------- src/query/api/v1/handler/namespace/common.go | 2 +- src/query/api/v1/httpd/handler.go | 4 +- src/query/api/v1/options/handler.go | 38 +++++------ src/query/api/v1/validators/validators.go | 49 ++++++++++++++ .../api/v1/validators/validators_test.go | 33 ++++++++++ 11 files changed, 159 insertions(+), 141 deletions(-) create mode 100644 src/query/api/v1/validators/validators.go create mode 100644 src/query/api/v1/validators/validators_test.go diff --git a/src/dbnode/namespace/convert_test.go b/src/dbnode/namespace/convert_test.go index f1db9cad28..4f7861a281 100644 --- a/src/dbnode/namespace/convert_test.go +++ b/src/dbnode/namespace/convert_test.go @@ -66,7 +66,7 @@ var ( } validNamespaceOpts = []nsproto.NamespaceOptions{ - nsproto.NamespaceOptions{ + { BootstrapEnabled: true, FlushEnabled: true, WritesToCommitLog: true, @@ -78,7 +78,7 @@ var ( ExtendedOptions: validExtendedOpts, StagingState: &nsproto.StagingState{Status: nsproto.StagingStatus_INITIALIZING}, }, - nsproto.NamespaceOptions{ + { BootstrapEnabled: true, FlushEnabled: true, WritesToCommitLog: true, @@ -92,7 +92,7 @@ var ( } validNamespaceSchemaOpts = []nsproto.NamespaceOptions{ - nsproto.NamespaceOptions{ + { RetentionOptions: &validRetentionOpts, SchemaOptions: testSchemaOptions, }, @@ -100,7 +100,7 @@ var ( invalidRetentionOpts = []nsproto.RetentionOptions{ // block size < buffer past - nsproto.RetentionOptions{ + { RetentionPeriodNanos: toNanos(1200), // 20h BlockSizeNanos: toNanos(2), // 2m BufferFutureNanos: toNanos(12), // 12m @@ -109,7 +109,7 @@ var ( BlockDataExpiryAfterNotAccessPeriodNanos: toNanos(30), // 30m }, // block size > retention - nsproto.RetentionOptions{ + { RetentionPeriodNanos: toNanos(1200), // 20h BlockSizeNanos: toNanos(1260), // 21h BufferFutureNanos: toNanos(12), // 12m @@ -150,7 +150,21 @@ func TestNamespaceToRetentionInvalid(t *testing.T) { } } -func TestToNamespaceValid(t *testing.T) { +func TestToMetadataValid(t *testing.T) { + nsopts := validNamespaceOpts[0] + + nsopts.RetentionOptions.BlockSizeNanos = 7200000000000 / 2 + nsopts.IndexOptions = nil + + nsOpts, err := namespace.ToMetadata("id", &nsopts) + require.NoError(t, err) + assert.Equal(t, + time.Duration(nsopts.RetentionOptions.BlockSizeNanos), + nsOpts.Options().IndexOptions().BlockSize()) +} + +func TestToMetadataNilIndexOpts(t *testing.T) { + for _, nsopts := range validNamespaceOpts { nsOpts, err := namespace.ToMetadata("abc", &nsopts) require.NoError(t, err) @@ -158,7 +172,7 @@ func TestToNamespaceValid(t *testing.T) { } } -func TestToNamespaceInvalid(t *testing.T) { +func TestToMetadataInvalid(t *testing.T) { for _, nsopts := range validNamespaceOpts { _, err := namespace.ToMetadata("", &nsopts) require.Error(t, err) diff --git a/src/query/api/v1/handler/database/common.go b/src/query/api/v1/handler/database/common.go index 2d16fe44cd..976f22f9f8 100644 --- a/src/query/api/v1/handler/database/common.go +++ b/src/query/api/v1/handler/database/common.go @@ -46,7 +46,7 @@ func RegisterRoutes( embeddedDbCfg *dbconfig.DBConfiguration, defaults []handleroptions.ServiceOptionsDefault, instrumentOpts instrument.Options, - hooks options.NamespaceHooks, + hooks options.NamespaceValidator, ) error { createHandler, err := NewCreateHandler(client, cfg, embeddedDbCfg, defaults, instrumentOpts, hooks) diff --git a/src/query/api/v1/handler/database/create.go b/src/query/api/v1/handler/database/create.go index 3dfe4983ac..6860da9bdd 100644 --- a/src/query/api/v1/handler/database/create.go +++ b/src/query/api/v1/handler/database/create.go @@ -149,7 +149,7 @@ func NewCreateHandler( embeddedDbCfg *dbconfig.DBConfiguration, defaults []handleroptions.ServiceOptionsDefault, instrumentOpts instrument.Options, - namespaceHooks options.NamespaceHooks, + namespaceValidator options.NamespaceValidator, ) (http.Handler, error) { placementHandlerOptions, err := placement.NewHandlerOptions(client, cfg, nil, instrumentOpts) @@ -159,7 +159,7 @@ func NewCreateHandler( return &createHandler{ placementInitHandler: placement.NewInitHandler(placementHandlerOptions), placementGetHandler: placement.NewGetHandler(placementHandlerOptions), - namespaceAddHandler: namespace.NewAddHandler(client, instrumentOpts, namespaceHooks), + namespaceAddHandler: namespace.NewAddHandler(client, instrumentOpts, namespaceValidator), namespaceGetHandler: namespace.NewGetHandler(client, instrumentOpts), namespaceDeleteHandler: namespace.NewDeleteHandler(client, instrumentOpts), embeddedDbCfg: embeddedDbCfg, diff --git a/src/query/api/v1/handler/database/create_test.go b/src/query/api/v1/handler/database/create_test.go index 1cc58fb1a9..a313faf858 100644 --- a/src/query/api/v1/handler/database/create_test.go +++ b/src/query/api/v1/handler/database/create_test.go @@ -40,7 +40,7 @@ import ( "github.com/m3db/m3/src/cmd/services/m3query/config" "github.com/m3db/m3/src/query/api/v1/handler/namespace" "github.com/m3db/m3/src/query/api/v1/handler/prometheus/handleroptions" - "github.com/m3db/m3/src/query/api/v1/options" + "github.com/m3db/m3/src/query/api/v1/validators" "github.com/m3db/m3/src/x/instrument" xjson "github.com/m3db/m3/src/x/json" xtest "github.com/m3db/m3/src/x/test" @@ -102,7 +102,7 @@ func testLocalType(t *testing.T, providedType string, placementExists bool) { mockClient, mockKV, mockPlacementService := SetupDatabaseTest(t, ctrl) mockClient.EXPECT().Store(gomock.Any()).Return(mockKV, nil).AnyTimes() createHandler, err := NewCreateHandler(mockClient, config.Configuration{}, - testDBCfg, svcDefaultOptions, instrument.NewOptions(), options.NoopNamespaceHooks) + testDBCfg, svcDefaultOptions, instrument.NewOptions(), validators.NamespaceValidator) require.NoError(t, err) w := httptest.NewRecorder() @@ -235,7 +235,7 @@ func TestLocalTypeClusteredPlacementAlreadyExists(t *testing.T) { mockClient, _, mockPlacementService := SetupDatabaseTest(t, ctrl) createHandler, err := NewCreateHandler(mockClient, config.Configuration{}, - testDBCfg, svcDefaultOptions, instrument.NewOptions(), options.NoopNamespaceHooks) + testDBCfg, svcDefaultOptions, instrument.NewOptions(), validators.NamespaceValidator) require.NoError(t, err) w := httptest.NewRecorder() @@ -281,7 +281,7 @@ func TestLocalTypeWithNumShards(t *testing.T) { mockClient, mockKV, mockPlacementService := SetupDatabaseTest(t, ctrl) mockClient.EXPECT().Store(gomock.Any()).Return(mockKV, nil).AnyTimes() createHandler, err := NewCreateHandler(mockClient, config.Configuration{}, - testDBCfg, svcDefaultOptions, instrument.NewOptions(), options.NoopNamespaceHooks) + testDBCfg, svcDefaultOptions, instrument.NewOptions(), validators.NamespaceValidator) require.NoError(t, err) w := httptest.NewRecorder() @@ -411,7 +411,7 @@ func TestLocalWithBlockSizeNanos(t *testing.T) { mockClient, mockKV, mockPlacementService := SetupDatabaseTest(t, ctrl) mockClient.EXPECT().Store(gomock.Any()).Return(mockKV, nil).AnyTimes() createHandler, err := NewCreateHandler(mockClient, config.Configuration{}, - testDBCfg, svcDefaultOptions, instrument.NewOptions(), options.NoopNamespaceHooks) + testDBCfg, svcDefaultOptions, instrument.NewOptions(), validators.NamespaceValidator) require.NoError(t, err) w := httptest.NewRecorder() @@ -540,7 +540,7 @@ func TestLocalWithBlockSizeExpectedSeriesDatapointsPerHour(t *testing.T) { mockClient, mockKV, mockPlacementService := SetupDatabaseTest(t, ctrl) mockClient.EXPECT().Store(gomock.Any()).Return(mockKV, nil).AnyTimes() createHandler, err := NewCreateHandler(mockClient, config.Configuration{}, - testDBCfg, svcDefaultOptions, instrument.NewOptions(), options.NoopNamespaceHooks) + testDBCfg, svcDefaultOptions, instrument.NewOptions(), validators.NamespaceValidator) require.NoError(t, err) w := httptest.NewRecorder() @@ -683,7 +683,7 @@ func TestClusterTypeHostsPlacementAlreadyExistsHostsProvided(t *testing.T) { mockClient, _, mockPlacementService := SetupDatabaseTest(t, ctrl) mockClient.EXPECT().Store(gomock.Any()).Return(nil, nil).AnyTimes() createHandler, err := NewCreateHandler(mockClient, config.Configuration{}, - testDBCfg, svcDefaultOptions, instrument.NewOptions(), options.NoopNamespaceHooks) + testDBCfg, svcDefaultOptions, instrument.NewOptions(), validators.NamespaceValidator) require.NoError(t, err) w := httptest.NewRecorder() @@ -738,7 +738,7 @@ func TestClusterTypeHostsPlacementAlreadyExistsExistingIsLocal(t *testing.T) { mockClient, _, mockPlacementService := SetupDatabaseTest(t, ctrl) createHandler, err := NewCreateHandler(mockClient, config.Configuration{}, - testDBCfg, svcDefaultOptions, instrument.NewOptions(), options.NoopNamespaceHooks) + testDBCfg, svcDefaultOptions, instrument.NewOptions(), validators.NamespaceValidator) require.NoError(t, err) w := httptest.NewRecorder() @@ -784,7 +784,7 @@ func testClusterTypeHosts(t *testing.T, placementExists bool) { mockClient, mockKV, mockPlacementService := SetupDatabaseTest(t, ctrl) mockClient.EXPECT().Store(gomock.Any()).Return(mockKV, nil).AnyTimes() createHandler, err := NewCreateHandler(mockClient, config.Configuration{}, - testDBCfg, svcDefaultOptions, instrument.NewOptions(), options.NoopNamespaceHooks) + testDBCfg, svcDefaultOptions, instrument.NewOptions(), validators.NamespaceValidator) require.NoError(t, err) w := httptest.NewRecorder() @@ -954,7 +954,7 @@ func TestClusterTypeHostsWithIsolationGroup(t *testing.T) { mockClient.EXPECT().Store(gomock.Any()).Return(mockKV, nil).AnyTimes() createHandler, err := NewCreateHandler(mockClient, config.Configuration{}, - testDBCfg, svcDefaultOptions, instrument.NewOptions(), options.NoopNamespaceHooks) + testDBCfg, svcDefaultOptions, instrument.NewOptions(), validators.NamespaceValidator) require.NoError(t, err) w := httptest.NewRecorder() @@ -1110,7 +1110,7 @@ func TestClusterTypeMissingHostnames(t *testing.T) { mockPlacementService.EXPECT().Placement().Return(nil, kv.ErrNotFound) createHandler, err := NewCreateHandler(mockClient, config.Configuration{}, - testDBCfg, svcDefaultOptions, instrument.NewOptions(), options.NoopNamespaceHooks) + testDBCfg, svcDefaultOptions, instrument.NewOptions(), validators.NamespaceValidator) require.NoError(t, err) w := httptest.NewRecorder() @@ -1146,7 +1146,7 @@ func TestBadType(t *testing.T) { mockPlacementService.EXPECT().Placement().Return(nil, kv.ErrNotFound) createHandler, err := NewCreateHandler(mockClient, config.Configuration{}, - nil, svcDefaultOptions, instrument.NewOptions(), options.NoopNamespaceHooks) + nil, svcDefaultOptions, instrument.NewOptions(), validators.NamespaceValidator) require.NoError(t, err) w := httptest.NewRecorder() @@ -1181,7 +1181,7 @@ func TestLocalTypeWithAggregatedNamespace(t *testing.T) { fakeKV := fake.NewStore() mockClient.EXPECT().Store(gomock.Any()).Return(fakeKV, nil).AnyTimes() createHandler, err := NewCreateHandler(mockClient, config.Configuration{}, - testDBCfg, svcDefaultOptions, instrument.NewOptions(), options.NoopNamespaceHooks) + testDBCfg, svcDefaultOptions, instrument.NewOptions(), validators.NamespaceValidator) require.NoError(t, err) w := httptest.NewRecorder() diff --git a/src/query/api/v1/handler/namespace/add.go b/src/query/api/v1/handler/namespace/add.go index 4d936eef44..d0e864b012 100644 --- a/src/query/api/v1/handler/namespace/add.go +++ b/src/query/api/v1/handler/namespace/add.go @@ -22,7 +22,6 @@ package namespace import ( "bytes" - "errors" "fmt" "net/http" "path" @@ -33,6 +32,7 @@ import ( "github.com/m3db/m3/src/query/api/v1/handler" "github.com/m3db/m3/src/query/api/v1/handler/prometheus/handleroptions" "github.com/m3db/m3/src/query/api/v1/options" + "github.com/m3db/m3/src/query/api/v1/validators" "github.com/m3db/m3/src/query/generated/proto/admin" "github.com/m3db/m3/src/query/util/logging" xerrors "github.com/m3db/m3/src/x/errors" @@ -49,29 +49,27 @@ var ( // AddHTTPMethod is the HTTP method used with this resource. AddHTTPMethod = http.MethodPost - - errNamespaceExists = xerrors.NewInvalidParamsError(errors.New("namespace with same ID already exists")) ) // AddHandler is the handler for namespace adds. type AddHandler struct { Handler - hooks options.NamespaceHooks + validator options.NamespaceValidator } // NewAddHandler returns a new instance of AddHandler. func NewAddHandler( client clusterclient.Client, instrumentOpts instrument.Options, - hooks options.NamespaceHooks, + validator options.NamespaceValidator, ) *AddHandler { return &AddHandler{ Handler: Handler{ client: client, instrumentOpts: instrumentOpts, }, - hooks: hooks, + validator: validator, } } @@ -93,7 +91,7 @@ func (h *AddHandler) ServeHTTP( opts := handleroptions.NewServiceOptions(svc, r.Header, nil) nsRegistry, err := h.Add(md, opts) if err != nil { - if err == errNamespaceExists { + if err == validators.ErrNamespaceExists { logger.Error("namespace already exists", zap.Error(err)) xhttp.WriteError(w, xhttp.NewError(err, http.StatusConflict)) return @@ -112,7 +110,7 @@ func (h *AddHandler) ServeHTTP( } func (h *AddHandler) parseRequest(r *http.Request) (*admin.NamespaceAddRequest, error) { - defer r.Body.Close() + defer r.Body.Close() // nolint:errcheck rBody, err := xhttp.DurationToNanosBytes(r.Body) if err != nil { return nil, xerrors.NewInvalidParamsError(err) @@ -138,10 +136,6 @@ func (h *AddHandler) Add( return emptyReg, xerrors.NewInvalidParamsError(fmt.Errorf("bad namespace metadata: %v", err)) } - if err := validateNewMetadata(md); err != nil { - return emptyReg, xerrors.NewInvalidParamsError(fmt.Errorf("invalid new namespace metadata: %v", err)) - } - store, err := h.client.Store(opts.KVOverrideOptions()) if err != nil { return emptyReg, err @@ -152,20 +146,11 @@ func (h *AddHandler) Add( return emptyReg, err } - if err := h.hooks.ValidateNewNamespace(md, currentMetadata); err != nil { - return emptyReg, xerrors.NewInvalidParamsError( - fmt.Errorf("invalid new namespace metadata: %v", err)) - } - - // Since this endpoint is `/add` and not in-place update, return an error if - // the NS already exists. NewMap will return an error if there's duplicate - // entries with the same name, but it's abstracted away behind a MultiError so - // we can't easily check that it's a conflict in the handler. - for _, ns := range currentMetadata { - if ns.ID().Equal(md.ID()) { - // NB: errNamespaceExists already an invalid params error. - return emptyReg, errNamespaceExists + if err := h.validator.ValidateNewNamespace(md, currentMetadata); err != nil { + if err == validators.ErrNamespaceExists { + return emptyReg, err } + return emptyReg, xerrors.NewInvalidParamsError(err) } newMDs := append(currentMetadata, md) @@ -190,17 +175,3 @@ func (h *AddHandler) Add( return *protoRegistry, nil } - -// Validate new namespace inputs only. Validation that applies to namespaces regardless of create/update/etc -// belongs in the option-specific Validate functions which are invoked on every change operation. -func validateNewMetadata(m namespace.Metadata) error { - indexBlockSize := m.Options().RetentionOptions().BlockSize() - retentionBlockSize := m.Options().IndexOptions().BlockSize() - if indexBlockSize != retentionBlockSize { - return fmt.Errorf("index and retention block size must match (%v, %v)", - indexBlockSize, - retentionBlockSize, - ) - } - return nil -} diff --git a/src/query/api/v1/handler/namespace/add_test.go b/src/query/api/v1/handler/namespace/add_test.go index 2ccebf2e13..aaa98ff5a4 100644 --- a/src/query/api/v1/handler/namespace/add_test.go +++ b/src/query/api/v1/handler/namespace/add_test.go @@ -28,13 +28,10 @@ import ( "strings" "testing" - "github.com/gogo/protobuf/jsonpb" - "github.com/m3db/m3/src/cluster/kv" nsproto "github.com/m3db/m3/src/dbnode/generated/proto/namespace" "github.com/m3db/m3/src/dbnode/namespace" - "github.com/m3db/m3/src/query/api/v1/options" - "github.com/m3db/m3/src/query/generated/proto/admin" + "github.com/m3db/m3/src/query/api/v1/validators" "github.com/m3db/m3/src/x/instrument" xjson "github.com/m3db/m3/src/x/json" xtest "github.com/m3db/m3/src/x/test" @@ -82,7 +79,7 @@ func TestNamespaceAddHandler(t *testing.T) { defer ctrl.Finish() mockClient, mockKV := setupNamespaceTest(t, ctrl) - addHandler := NewAddHandler(mockClient, instrument.NewOptions(), options.NoopNamespaceHooks) + addHandler := NewAddHandler(mockClient, instrument.NewOptions(), validators.NamespaceValidator) mockClient.EXPECT().Store(gomock.Any()).Return(mockKV, nil) // Error case where required fields are not set @@ -167,7 +164,7 @@ func TestNamespaceAddHandler_Conflict(t *testing.T) { defer ctrl.Finish() mockClient, mockKV := setupNamespaceTest(t, ctrl) - addHandler := NewAddHandler(mockClient, instrument.NewOptions(), options.NoopNamespaceHooks) + addHandler := NewAddHandler(mockClient, instrument.NewOptions(), validators.NamespaceValidator) mockClient.EXPECT().Store(gomock.Any()).Return(mockKV, nil) // Ensure adding an existing namespace returns 409 @@ -190,8 +187,8 @@ func TestNamespaceAddHandler_Conflict(t *testing.T) { BufferPastNanos: 600000000000, BlockDataExpiry: true, BlockDataExpiryAfterNotAccessPeriodNanos: 3600000000000, - }, - }, + }, + }, }, } @@ -206,13 +203,13 @@ func TestNamespaceAddHandler_Conflict(t *testing.T) { assert.Equal(t, http.StatusConflict, resp.StatusCode) } -func TestApplyHookValidator(t *testing.T) { +func TestApplyNewNamespaceValidator(t *testing.T) { ctrl := gomock.NewController(t) defer ctrl.Finish() mockClient, mockKV := setupNamespaceTest(t, ctrl) - hooks := &testNamespaceHooks{} - addHandler := NewAddHandler(mockClient, instrument.NewOptions(), hooks) + validator := &testNamespaceValidator{} + addHandler := NewAddHandler(mockClient, instrument.NewOptions(), validator) mockClient.EXPECT().Store(gomock.Any()).Return(mockKV, nil) req := httptest.NewRequest("POST", "/namespace", strings.NewReader(testAddJSON)) @@ -248,52 +245,14 @@ func TestApplyHookValidator(t *testing.T) { addHandler.ServeHTTP(svcDefaults, w, req) resp := w.Result() assert.Equal(t, http.StatusBadRequest, resp.StatusCode) - assert.Equal(t, 1, hooks.invocationCount) -} - -func TestValidateNewMetadata(t *testing.T) { - // Valid. - addReq := new(admin.NamespaceAddRequest) - require.NoError(t, jsonpb.Unmarshal(strings.NewReader(testAddJSON), addReq)) - md, err := namespace.ToMetadata(addReq.Name, addReq.Options) - require.NoError(t, err) - require.NoError(t, validateNewMetadata(md)) - - // Valid without index options. - addReq = new(admin.NamespaceAddRequest) - require.NoError(t, jsonpb.Unmarshal(strings.NewReader(testAddJSON), addReq)) - addReq.Options.RetentionOptions.BlockSizeNanos = 7200000000000 / 2 - addReq.Options.IndexOptions = nil - md, err = namespace.ToMetadata(addReq.Name, addReq.Options) - require.NoError(t, err) - require.NoError(t, validateNewMetadata(md)) - - // Invalid without retention options. - addReq = new(admin.NamespaceAddRequest) - require.NoError(t, jsonpb.Unmarshal(strings.NewReader(testAddJSON), addReq)) - addReq.Options.RetentionOptions = nil - addReq.Options.IndexOptions.BlockSizeNanos = 7200000000000 / 2 - md, err = namespace.ToMetadata(addReq.Name, addReq.Options) - require.Error(t, err) - require.Equal(t, "retention options must be set", err.Error()) - - // Prevent mismatching block sizes. - addReq = new(admin.NamespaceAddRequest) - require.NoError(t, jsonpb.Unmarshal(strings.NewReader(testAddJSON), addReq)) - addReq.Options.RetentionOptions.BlockSizeNanos = 7200000000000 - addReq.Options.IndexOptions.BlockSizeNanos = 7200000000000 * 2 - md, err = namespace.ToMetadata(addReq.Name, addReq.Options) - require.NoError(t, err) - err = validateNewMetadata(md) - require.Error(t, err) - require.Equal(t, "index and retention block size must match (2h0m0s, 4h0m0s)", err.Error()) + assert.Equal(t, 1, validator.invocationCount) } -type testNamespaceHooks struct { +type testNamespaceValidator struct { invocationCount int } -func (h *testNamespaceHooks) ValidateNewNamespace(namespace.Metadata, []namespace.Metadata) error { - h.invocationCount++ +func (v *testNamespaceValidator) ValidateNewNamespace(namespace.Metadata, []namespace.Metadata) error { + v.invocationCount++ return errors.New("expected validation error") } diff --git a/src/query/api/v1/handler/namespace/common.go b/src/query/api/v1/handler/namespace/common.go index 2a65235862..b165769809 100644 --- a/src/query/api/v1/handler/namespace/common.go +++ b/src/query/api/v1/handler/namespace/common.go @@ -114,7 +114,7 @@ func RegisterRoutes( clusters m3.Clusters, defaults []handleroptions.ServiceOptionsDefault, instrumentOpts instrument.Options, - hooks options.NamespaceHooks, + hooks options.NamespaceValidator, ) error { applyMiddleware := func( f func(svc handleroptions.ServiceNameAndDefaults, diff --git a/src/query/api/v1/httpd/handler.go b/src/query/api/v1/httpd/handler.go index 8f098410e9..2dd8228ee0 100644 --- a/src/query/api/v1/httpd/handler.go +++ b/src/query/api/v1/httpd/handler.go @@ -390,7 +390,7 @@ func (h *Handler) RegisterRoutes() error { if clusterClient != nil { err = database.RegisterRoutes(h.registry, clusterClient, h.options.Config(), h.options.EmbeddedDbCfg(), - serviceOptionDefaults, instrumentOpts, h.options.NamespaceHooks()) + serviceOptionDefaults, instrumentOpts, h.options.NamespaceValidator()) if err != nil { return err } @@ -403,7 +403,7 @@ func (h *Handler) RegisterRoutes() error { err = namespace.RegisterRoutes(h.registry, clusterClient, h.options.Clusters(), serviceOptionDefaults, instrumentOpts, - h.options.NamespaceHooks()) + h.options.NamespaceValidator()) if err != nil { return err } diff --git a/src/query/api/v1/options/handler.go b/src/query/api/v1/options/handler.go index 4c6becc40a..429c990531 100644 --- a/src/query/api/v1/options/handler.go +++ b/src/query/api/v1/options/handler.go @@ -30,8 +30,9 @@ import ( "github.com/m3db/m3/src/cmd/services/m3coordinator/ingest" dbconfig "github.com/m3db/m3/src/cmd/services/m3dbnode/config" "github.com/m3db/m3/src/cmd/services/m3query/config" - "github.com/m3db/m3/src/dbnode/namespace" + dbnamespace "github.com/m3db/m3/src/dbnode/namespace" "github.com/m3db/m3/src/query/api/v1/handler/prometheus/handleroptions" + "github.com/m3db/m3/src/query/api/v1/validators" "github.com/m3db/m3/src/query/executor" graphite "github.com/m3db/m3/src/query/graphite/storage" "github.com/m3db/m3/src/query/models" @@ -210,10 +211,10 @@ type HandlerOptions interface { // StoreMetricsType returns true if storing of metrics type is enabled. StoreMetricsType() bool - // SetNamespaceHooks sets the NamespaceHooks. - SetNamespaceHooks(NamespaceHooks) HandlerOptions - // NamespaceHooks returns the NamespaceHooks. - NamespaceHooks() NamespaceHooks + // SetNamespaceValidator sets the NamespaceValidator. + SetNamespaceValidator(NamespaceValidator) HandlerOptions + // NamespaceValidator returns the NamespaceValidator. + NamespaceValidator() NamespaceValidator } // HandlerOptions represents handler options. @@ -240,7 +241,7 @@ type handlerOptions struct { instantQueryRouter QueryRouter graphiteStorageOpts graphite.M3WrappedStorageOptions m3dbOpts m3db.Options - namespaceHooks NamespaceHooks + namespaceValidator NamespaceValidator storeMetricsType bool } @@ -303,7 +304,7 @@ func NewHandlerOptions( graphiteStorageOpts: graphiteStorageOpts, m3dbOpts: m3dbOpts, storeMetricsType: storeMetricsType, - namespaceHooks: NoopNamespaceHooks, + namespaceValidator: validators.NamespaceValidator, }, nil } @@ -556,27 +557,18 @@ func (o *handlerOptions) StoreMetricsType() bool { return o.storeMetricsType } -func (o *handlerOptions) SetNamespaceHooks(value NamespaceHooks) HandlerOptions { +func (o *handlerOptions) SetNamespaceValidator(value NamespaceValidator) HandlerOptions { opts := *o - opts.namespaceHooks = value + opts.namespaceValidator = value return &opts } -func (o *handlerOptions) NamespaceHooks() NamespaceHooks { - return o.namespaceHooks +func (o *handlerOptions) NamespaceValidator() NamespaceValidator { + return o.namespaceValidator } -// NamespaceHooks allows dynamic plugging into the namespace lifecycle. -type NamespaceHooks interface { +// NamespaceValidator defines namespace validation logics. +type NamespaceValidator interface { // ValidateNewNamespace gets invoked when creating a new namespace. - ValidateNewNamespace(newNs namespace.Metadata, existing []namespace.Metadata) error -} - -// NoopNamespaceHooks is an instance of noopNamespaceHooks. -var NoopNamespaceHooks = &noopNamespaceHooks{} - -type noopNamespaceHooks struct{} - -func (h *noopNamespaceHooks) ValidateNewNamespace(namespace.Metadata, []namespace.Metadata) error { - return nil + ValidateNewNamespace(newNs dbnamespace.Metadata, existing []dbnamespace.Metadata) error } diff --git a/src/query/api/v1/validators/validators.go b/src/query/api/v1/validators/validators.go new file mode 100644 index 0000000000..9456c27ca4 --- /dev/null +++ b/src/query/api/v1/validators/validators.go @@ -0,0 +1,49 @@ +// package validators contains validation logics for the api. +package validators + +import ( + "errors" + "fmt" + + "github.com/m3db/m3/src/dbnode/namespace" + xerrors "github.com/m3db/m3/src/x/errors" +) + +var ( + // NamespaceValidator is an instance of namespaceValidator. + NamespaceValidator = &namespaceValidator{} + + // ErrNamespaceExists is returned when trying to create a namespace with id that already exists. + ErrNamespaceExists = errors.New("namespace with the same ID already exists") +) + +type namespaceValidator struct{} + +// Validate new namespace inputs only. Validation that applies to namespaces +// regardless of create/update/etc belongs in the option-specific Validate +// functions which are invoked on every change operation. +func (h *namespaceValidator) ValidateNewNamespace( + ns namespace.Metadata, + existing []namespace.Metadata, +) error { + var ( + id = ns.ID() + indexBlockSize = ns.Options().RetentionOptions().BlockSize() + retentionBlockSize = ns.Options().IndexOptions().BlockSize() + ) + + if indexBlockSize != retentionBlockSize { + return xerrors.NewInvalidParamsError( + fmt.Errorf("index and retention block size must match (%v, %v)", + indexBlockSize, + retentionBlockSize)) + } + + for _, existingNs := range existing { + if id.Equal(existingNs.ID()) { + return ErrNamespaceExists + } + } + + return nil +} diff --git a/src/query/api/v1/validators/validators_test.go b/src/query/api/v1/validators/validators_test.go new file mode 100644 index 0000000000..234a9a7330 --- /dev/null +++ b/src/query/api/v1/validators/validators_test.go @@ -0,0 +1,33 @@ +package validators + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/m3db/m3/src/dbnode/namespace" + "github.com/m3db/m3/src/x/ident" +) + +func TestValidateNewNamespace(t *testing.T) { + var ( + id = ident.BytesID("id") + opts = namespace.NewOptions() + ) + + valid, err := namespace.NewMetadata(id, opts) + require.NoError(t, err) + + assert.NoError(t, NamespaceValidator.ValidateNewNamespace(valid, nil)) + + // Prevent mismatching block sizes. + mismatchingBlockOpts := opts. + SetRetentionOptions(opts.RetentionOptions().SetBlockSize(7200000000000)). + SetIndexOptions(opts.IndexOptions().SetBlockSize(7200000000000 * 2)) + mismatchingBlocks, err := namespace.NewMetadata(id, mismatchingBlockOpts) + require.NoError(t, err) + + err = NamespaceValidator.ValidateNewNamespace(mismatchingBlocks, nil) + assert.EqualError(t, err, "index and retention block size must match (2h0m0s, 4h0m0s)") +} From 44c984fe5d31132837d4fc3fe186493220e49b92 Mon Sep 17 00:00:00 2001 From: Linas Medziunas Date: Tue, 24 Nov 2020 10:20:17 +0200 Subject: [PATCH 3/6] Swap tests --- src/dbnode/namespace/convert_test.go | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/src/dbnode/namespace/convert_test.go b/src/dbnode/namespace/convert_test.go index 4f7861a281..82b590f29b 100644 --- a/src/dbnode/namespace/convert_test.go +++ b/src/dbnode/namespace/convert_test.go @@ -151,6 +151,14 @@ func TestNamespaceToRetentionInvalid(t *testing.T) { } func TestToMetadataValid(t *testing.T) { + for _, nsopts := range validNamespaceOpts { + nsOpts, err := namespace.ToMetadata("abc", &nsopts) + require.NoError(t, err) + assertEqualMetadata(t, "abc", nsopts, nsOpts) + } +} + +func TestToMetadataNilIndexOpts(t *testing.T) { nsopts := validNamespaceOpts[0] nsopts.RetentionOptions.BlockSizeNanos = 7200000000000 / 2 @@ -163,15 +171,6 @@ func TestToMetadataValid(t *testing.T) { nsOpts.Options().IndexOptions().BlockSize()) } -func TestToMetadataNilIndexOpts(t *testing.T) { - - for _, nsopts := range validNamespaceOpts { - nsOpts, err := namespace.ToMetadata("abc", &nsopts) - require.NoError(t, err) - assertEqualMetadata(t, "abc", nsopts, nsOpts) - } -} - func TestToMetadataInvalid(t *testing.T) { for _, nsopts := range validNamespaceOpts { _, err := namespace.ToMetadata("", &nsopts) From bdd3de57b9dbe4410d6bdcceb08c84054c613ada Mon Sep 17 00:00:00 2001 From: Linas Medziunas Date: Tue, 24 Nov 2020 10:22:14 +0200 Subject: [PATCH 4/6] Renaming --- src/query/api/v1/handler/database/common.go | 4 ++-- src/query/api/v1/handler/namespace/common.go | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/query/api/v1/handler/database/common.go b/src/query/api/v1/handler/database/common.go index 976f22f9f8..70854ea06b 100644 --- a/src/query/api/v1/handler/database/common.go +++ b/src/query/api/v1/handler/database/common.go @@ -46,10 +46,10 @@ func RegisterRoutes( embeddedDbCfg *dbconfig.DBConfiguration, defaults []handleroptions.ServiceOptionsDefault, instrumentOpts instrument.Options, - hooks options.NamespaceValidator, + namespaceValidator options.NamespaceValidator, ) error { createHandler, err := NewCreateHandler(client, cfg, embeddedDbCfg, - defaults, instrumentOpts, hooks) + defaults, instrumentOpts, namespaceValidator) if err != nil { return err } diff --git a/src/query/api/v1/handler/namespace/common.go b/src/query/api/v1/handler/namespace/common.go index b165769809..525abfae5d 100644 --- a/src/query/api/v1/handler/namespace/common.go +++ b/src/query/api/v1/handler/namespace/common.go @@ -114,7 +114,7 @@ func RegisterRoutes( clusters m3.Clusters, defaults []handleroptions.ServiceOptionsDefault, instrumentOpts instrument.Options, - hooks options.NamespaceValidator, + namespaceValidator options.NamespaceValidator, ) error { applyMiddleware := func( f func(svc handleroptions.ServiceNameAndDefaults, @@ -142,7 +142,7 @@ func RegisterRoutes( // Add M3DB namespaces. if err := r.Register(queryhttp.RegisterOptions{ Path: M3DBAddURL, - Handler: applyMiddleware(NewAddHandler(client, instrumentOpts, hooks).ServeHTTP, defaults), + Handler: applyMiddleware(NewAddHandler(client, instrumentOpts, namespaceValidator).ServeHTTP, defaults), Methods: []string{AddHTTPMethod}, }); err != nil { return err From 3a8d88a12214dfaf8e29740b6c9deafb38ec0602 Mon Sep 17 00:00:00 2001 From: Linas Medziunas Date: Tue, 24 Nov 2020 10:35:07 +0200 Subject: [PATCH 5/6] Copyrights --- src/query/api/v1/validators/validators.go | 22 ++++++++++++++++++- .../api/v1/validators/validators_test.go | 20 +++++++++++++++++ 2 files changed, 41 insertions(+), 1 deletion(-) diff --git a/src/query/api/v1/validators/validators.go b/src/query/api/v1/validators/validators.go index 9456c27ca4..4b521699b6 100644 --- a/src/query/api/v1/validators/validators.go +++ b/src/query/api/v1/validators/validators.go @@ -1,4 +1,24 @@ -// package validators contains validation logics for the api. +// Copyright (c) 2020 Uber Technologies, Inc. +// +// Permission is hereby granted, free of charge, to any person obtaining a copy +// of this software and associated documentation files (the "Software"), to deal +// in the Software without restriction, including without limitation the rights +// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +// copies of the Software, and to permit persons to whom the Software is +// furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in +// all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN +// THE SOFTWARE. + +//package validators contains validation logics for the api. package validators import ( diff --git a/src/query/api/v1/validators/validators_test.go b/src/query/api/v1/validators/validators_test.go index 234a9a7330..84fc73f7ec 100644 --- a/src/query/api/v1/validators/validators_test.go +++ b/src/query/api/v1/validators/validators_test.go @@ -1,3 +1,23 @@ +// Copyright (c) 2020 Uber Technologies, Inc. +// +// Permission is hereby granted, free of charge, to any person obtaining a copy +// of this software and associated documentation files (the "Software"), to deal +// in the Software without restriction, including without limitation the rights +// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +// copies of the Software, and to permit persons to whom the Software is +// furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in +// all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN +// THE SOFTWARE. + package validators import ( From 2054ad509826fdd4c458026f5ff3394988fe54d1 Mon Sep 17 00:00:00 2001 From: Linas Medziunas Date: Wed, 25 Nov 2020 08:38:06 +0200 Subject: [PATCH 6/6] Address PR feedback --- .../api/v1/handler/namespace/add_test.go | 2 +- .../api/v1/validators/validators_test.go | 21 +++++++++++++------ 2 files changed, 16 insertions(+), 7 deletions(-) diff --git a/src/query/api/v1/handler/namespace/add_test.go b/src/query/api/v1/handler/namespace/add_test.go index aaa98ff5a4..0f26d83596 100644 --- a/src/query/api/v1/handler/namespace/add_test.go +++ b/src/query/api/v1/handler/namespace/add_test.go @@ -203,7 +203,7 @@ func TestNamespaceAddHandler_Conflict(t *testing.T) { assert.Equal(t, http.StatusConflict, resp.StatusCode) } -func TestApplyNewNamespaceValidator(t *testing.T) { +func TestNamespaceAddHandler_InvokesNewNamespaceValidator(t *testing.T) { ctrl := gomock.NewController(t) defer ctrl.Finish() diff --git a/src/query/api/v1/validators/validators_test.go b/src/query/api/v1/validators/validators_test.go index 84fc73f7ec..2381384113 100644 --- a/src/query/api/v1/validators/validators_test.go +++ b/src/query/api/v1/validators/validators_test.go @@ -30,18 +30,19 @@ import ( "github.com/m3db/m3/src/x/ident" ) -func TestValidateNewNamespace(t *testing.T) { - var ( - id = ident.BytesID("id") - opts = namespace.NewOptions() - ) +var ( + id = ident.BytesID("id") + opts = namespace.NewOptions() +) +func TestValidateNewNamespace(t *testing.T) { valid, err := namespace.NewMetadata(id, opts) require.NoError(t, err) assert.NoError(t, NamespaceValidator.ValidateNewNamespace(valid, nil)) +} - // Prevent mismatching block sizes. +func TestValidateNewNamespaceFailOnBlockSize(t *testing.T) { mismatchingBlockOpts := opts. SetRetentionOptions(opts.RetentionOptions().SetBlockSize(7200000000000)). SetIndexOptions(opts.IndexOptions().SetBlockSize(7200000000000 * 2)) @@ -51,3 +52,11 @@ func TestValidateNewNamespace(t *testing.T) { err = NamespaceValidator.ValidateNewNamespace(mismatchingBlocks, nil) assert.EqualError(t, err, "index and retention block size must match (2h0m0s, 4h0m0s)") } + +func TestValidateNewNamespaceFailDuplicate(t *testing.T) { + ns, err := namespace.NewMetadata(id, opts) + require.NoError(t, err) + + err = NamespaceValidator.ValidateNewNamespace(ns, []namespace.Metadata{ns}) + assert.Equal(t, ErrNamespaceExists, err) +}