Add validate namespace endpoint to m3ctl #50

m-sandusky · 2018-01-03T20:44:32Z

This PR adds a namespace validation endpoint to m3ctl. Will probably add a couple more tests before merging.

coveralls · 2018-01-03T21:26:07Z

Coverage decreased (-0.06%) to 52.727% when pulling 136e0a5 on add-validation-endpoint into 706b2dd on master.

coveralls · 2018-01-03T21:26:07Z

Coverage decreased (-0.06%) to 52.727% when pulling 136e0a5 on add-validation-endpoint into 706b2dd on master.

coveralls · 2018-01-03T21:35:57Z

Coverage decreased (-0.4%) to 52.42% when pulling e4dd37f on add-validation-endpoint into 706b2dd on master.

coveralls · 2018-01-03T21:35:57Z

Coverage decreased (-0.4%) to 52.42% when pulling e4dd37f on add-validation-endpoint into 706b2dd on master.

xichen2020 · 2018-01-05T21:36:13Z

service/r2/kv/store.go

 }

 // NewStore returns a new service that knows how to talk to a kv backed r2 store.
-func NewStore(rs rules.Store, opts StoreOptions) r2.Store {
+func NewStore(rs rules.Store, v rules.Validator, opts StoreOptions) r2.Store {


Perhaps make validator part of the StoreOptions?

I thought about this, and to me if we're going to throw an error on the validator no being present (from the below comment) then it would make sense to have the Validator be a required param.

However, if passing a Validator should be optional then the endpoint should have a successful validation for every request if no validator was configured for the store. I've added the logic for this but can change it back.

xichen2020 · 2018-01-05T21:36:42Z

service/r2/kv/store.go

@@ -73,6 +75,10 @@ func (s *store) FetchNamespaces() (*rules.NamespacesView, error) {
 	}, nil
 }

+func (s *store) ValidateNamespace(rs *rules.RuleSetSnapshot) error {


Perhaps call this ValidateRuleSet to be more accurate?

xichen2020 · 2018-01-05T21:38:06Z

service/r2/kv/store.go

@@ -73,6 +75,10 @@ func (s *store) FetchNamespaces() (*rules.NamespacesView, error) {
 	}, nil
 }

+func (s *store) ValidateNamespace(rs *rules.RuleSetSnapshot) error {
+	return s.validator.ValidateSnapshot(rs)


I'd check if s.validator is nil and return an errNilValidator = errors.New("validator not provided") just to be defensive.

would it be reasonable to panic on NewStore if no validator is passed in? We have a stub validator so even for tests there should always be one available.

That's interesting - essentially we want the validator that is used in the instance of kv.TxnStore in the construction of NewStore to be the same. We allow for a nil validator to be passed in there (and bypass any validation if so) so if that's the case I'd assume everything passed to the validation endpoint should succeed.

xichen2020 · 2018-01-05T21:39:35Z

service/r2/routes.go

+	}
+	if vars[namespaceIDVar] != rsj.Namespace {
+		return nil, fmt.Errorf(
+			"NamespaceID param: %s and RuleSet namespaceID: %s do not match",


nit: error messages should start with lowercase letters. Also might be easier on the eyes to say
namespaceID param %s and ruleset namespaceID %s do not match?

xichen2020 · 2018-01-05T21:43:53Z

service/r2/routes.go

+	if err := s.store.ValidateNamespace(rss); err != nil {
+		return nil, err
+	}
+	return fmt.Sprintf("Namespace and rule-set are valid"), nil


Perhaps just return Ruleset is valid?

xichen2020 · 2018-01-05T21:50:25Z

service/r2/service.go

@@ -192,6 +193,7 @@ func (s *service) RegisterHandlers(router *mux.Router) {

 	router.Handle(namespacePath, h.wrap(s.fetchNamespaces)).Methods(http.MethodGet)
 	router.Handle(namespacePath, h.wrap(s.createNamespace)).Methods(http.MethodPost)
+	router.Handle(namespaceValidatePath, h.wrap(s.validateNamespace)).Methods(http.MethodPost)


How is auth going to work for validation endpoint? Is it "if a user has read access, then he/she can validate?" Or does he/she need to have write access to validate?

Left a comment on this earlier - not sure why it didn't post. In any case this is a good point. If the requester does not have write access, due to how the current auth is set up, they will not be validated for the endpoint. The way around this is to make the endpoint GET or to change how we do auth.

I think we can make the NewAuthHandler take an additional parameter authType:

type authorizationType int const ( none authorizationType readOnly writeOnly readWrite ) func (a simpleAuth) NewAuthHandler( authType authorizationType, next http.Handler, errHandler errorResponseHandler, ) http.Handler { ... } return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { ... switch authType { case none: ... case readOnly: ... case writeOnly: ... case readWrite: ... }

Then in service.go, register the auth type

func registerRoute(router *mux.Router, path, method string, handler http.Handler) error { authType, exists := authRegistry[endpoint{Path:path, Method: method}] if !exists { // Should make this a method instead. switch method: case http.GET: authType = readOnly case http.POST, ...: authType = writeOnly default: return errUnknownHTTPMethod } fn := h.wrap(authType, handler) router.Handle(path, fn).Methods(method) return nil } func (h r2Handler) wrap(authType authorizationType, fn r2HandlerFunc) http.Handler { f := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if err := fn(w, r); err != nil { h.handleError(w, err) } }) return h.auth.NewAuthHandler(authType, f, writeAPIResponse) } type endpoint struct { Path string Method string } var authRegistry = map[endpoint]authorizationType{ {Path: namespaceValidePath, Method: http.POST}: readOnly }

xichen2020 · 2018-01-05T21:50:56Z

service/r2/service.go

+	}
+
+	for _, mr := range r.MappingRules {
+		rss.MappingRules[mr.Name] = mr.mappingRuleView()


Hm we should use the UUID rather than the name no?

Yup, changed.

xichen2020 · 2018-01-05T21:51:14Z

service/r2/service.go

+		rss.MappingRules[mr.Name] = mr.mappingRuleView()
+	}
+	for _, rr := range r.RollupRules {
+		rss.RollupRules[rr.Name] = rr.rollupRuleView()


Hm we should use the UUID rather than the name no?

xichen2020 · 2018-01-05T21:51:32Z

service/r2/stub/store.go

@@ -304,6 +304,11 @@ func (s *store) CreateNamespace(namespaceID string, uOpts r2.UpdateOptions) (*ru
 	}
 }

+func (s *store) ValidateNamespace(rs *rules.RuleSetSnapshot) error {
+	// Assumes no validation config for stub store so all rule sets are valid


nit: add a period at the end.

xichen2020 · 2018-01-05T21:59:48Z

service/r2/store.go

@@ -27,6 +27,9 @@ type Store interface {
 	// FetchNamespaces fetches namespaces.
 	FetchNamespaces() (*rules.NamespacesView, error)

+	// ValidateNamespace validates a namespace's ruleset.
+	ValidateNamespace(rs *rules.RuleSetSnapshot) error


ValidateRuleSet is probably more accurate in this case.

jskelcy · 2018-01-05T22:12:10Z

service/r2/routes.go

+		)
+	}
+
+	rss := rsj.ruleSetSnapshot()


NIT:
Given that this function is only called in this file, and ruleSetSnapshot would be the only stuct with methods on it. Perhaps make this function take a ruleSetJSON and return a ruleSetSnapshot. The It might also have it as sort of public util function.

I was following the general convention with converting the JSON structs to View/Snapshots (for better or for worse), open to change if you feel strongly about it :)

jskelcy · 2018-01-05T22:33:08Z

services/r2ctl/config/r2ctl.go

@@ -131,5 +131,5 @@ func (c kvStoreConfig) NewStore(instrumentOpts instrument.Options) (r2.Store, er
 	r2StoreOpts := r2kv.NewStoreOptions().
 		SetInstrumentOptions(instrumentOpts).
 		SetRuleUpdatePropagationDelay(c.PropagationDelay)
-	return r2kv.NewStore(rulesStore, r2StoreOpts), nil
+	return r2kv.NewStore(rulesStore, validator, r2StoreOpts), nil


I dont know enough to say either way, but could the validator go on the r2StoreOptions? If we already have a catch-all for parameters to this function perhaps we should just add it there instead.

Yup, added there

m-sandusky

@xichen2020 @jskelcy Thanks for the review! I've addressed your comments.

m-sandusky · 2018-01-08T17:21:37Z

service/r2/kv/store.go

 }

 // NewStore returns a new service that knows how to talk to a kv backed r2 store.
-func NewStore(rs rules.Store, opts StoreOptions) r2.Store {
+func NewStore(rs rules.Store, v rules.Validator, opts StoreOptions) r2.Store {


I thought about this, and to me if we're going to throw an error on the validator no being present (from the below comment) then it would make sense to have the Validator be a required param.

However, if passing a Validator should be optional then the endpoint should have a successful validation for every request if no validator was configured for the store. I've added the logic for this but can change it back.

m-sandusky · 2018-01-08T17:31:38Z

service/r2/kv/store.go

@@ -73,6 +75,10 @@ func (s *store) FetchNamespaces() (*rules.NamespacesView, error) {
 	}, nil
 }

+func (s *store) ValidateNamespace(rs *rules.RuleSetSnapshot) error {
+	return s.validator.ValidateSnapshot(rs)


That's interesting - essentially we want the validator that is used in the instance of kv.TxnStore in the construction of NewStore to be the same. We allow for a nil validator to be passed in there (and bypass any validation if so) so if that's the case I'd assume everything passed to the validation endpoint should succeed.

m-sandusky · 2018-01-08T17:32:55Z

service/r2/routes.go

@@ -64,6 +64,27 @@ func createNamespace(s *service, r *http.Request) (data interface{}, err error)
 	return newNamespaceJSON(view), nil
 }

+func validateNamespace(s *service, r *http.Request) (data interface{}, err error) {


Should we edit the actual endpoint to reflect this as well? ie (/namespaces/{namespace_id}/validate_rule_set)

m-sandusky · 2018-01-08T17:36:34Z

service/r2/routes.go

+		)
+	}
+
+	rss := rsj.ruleSetSnapshot()


I was following the general convention with converting the JSON structs to View/Snapshots (for better or for worse), open to change if you feel strongly about it :)

m-sandusky · 2018-01-08T17:40:04Z

service/r2/service.go

+	}
+
+	for _, mr := range r.MappingRules {
+		rss.MappingRules[mr.Name] = mr.mappingRuleView()


Yup, changed.

m-sandusky · 2018-01-08T17:41:00Z

services/r2ctl/config/r2ctl.go

@@ -131,5 +131,5 @@ func (c kvStoreConfig) NewStore(instrumentOpts instrument.Options) (r2.Store, er
 	r2StoreOpts := r2kv.NewStoreOptions().
 		SetInstrumentOptions(instrumentOpts).
 		SetRuleUpdatePropagationDelay(c.PropagationDelay)
-	return r2kv.NewStore(rulesStore, r2StoreOpts), nil
+	return r2kv.NewStore(rulesStore, validator, r2StoreOpts), nil


Yup, added there

coveralls · 2018-01-08T18:27:14Z

Coverage increased (+4.0%) to 56.772% when pulling 658b6a7 on add-validation-endpoint into 706b2dd on master.

coveralls · 2018-01-08T18:27:14Z

Coverage increased (+4.0%) to 56.772% when pulling 658b6a7 on add-validation-endpoint into 706b2dd on master.

jskelcy · 2018-01-08T18:56:50Z

service/r2/kv/options.go

@@ -96,3 +104,13 @@ func (o *storeOptions) SetRuleUpdatePropagationDelay(value time.Duration) StoreO
 func (o *storeOptions) RuleUpdatePropagationDelay() time.Duration {
 	return o.ruleUpdatePropagationDelay
 }
+
+func (o *storeOptions) SetValidator(value rules.Validator) StoreOptions {
+	opts := *o


This appears to be the convention but is there a need for this deref @xichen2020 ?

Yeah so this is to ensure the options are immutable.

jskelcy · 2018-01-08T19:06:02Z

service/r2/routes.go

+	if err := s.store.ValidateRuleSet(rss); err != nil {
+		return nil, err
+	}
+	return fmt.Sprintf("Ruleset is valid"), nil


I think you can just return a string, no need to use fmt.Sprintf

Given that this just returns a string that tells the caller that seems a little self-explanatory do you think this function could just return an error?

Thanks - nice catch.

Following convention on the other endpoints that return string values in the service (ie deleteMappingRule, deleteRollupRule, etc.) I'm open to whatever either way.

jskelcy · 2018-01-08T19:15:07Z

service/r2/service.go

@@ -36,6 +36,7 @@ import (
 	"github.com/m3db/m3metrics/rules"
 	"github.com/m3db/m3x/clock"
 	"github.com/m3db/m3x/instrument"
+	"github.com/pborman/uuid"


Im double checking on this for OSS but I think we may want to use github.com/satori/go.uuid

Yeah, I've used this on in the past as well. m3ctl uses "github.com/pborman/uuid" in other places though so I didn't want add a new dependency for a similar use case. @xichen2020 Do you have any thoughts?

Fine w/ either, as long as there's only one for the repo.

Also, please add a blank line between m3db imports and 3rd party imports.

jskelcy · 2018-01-08T19:16:53Z

service/r2/service.go

+
+// Creates a new RuleSetSnapshot from a rulesetJSON. If the ruleSetJSON has no IDs for any of its
+// mapping rules or rollup rules, it generates missing IDs and sets as a string UUID string so they
+// can be stored in a mapping (id -> rule).


If a rule is being added, not updated is that when there would not be an ID for a rule? I might update the comment to explain what the scenario is where this happens.

Yeah that's exactly it - I'll add a comment for this scenario.

nit: Change the comment to ruleSetSnapshot creates a ....

You might also make this behavior controlled by an enum passed in though (e.g., genIDType), e.g., if this is an update request, we probably shouldn't generate the missing IDs since that should be a validation error.

coveralls · 2018-01-08T19:41:02Z

Coverage increased (+4.0%) to 56.772% when pulling 3965184 on add-validation-endpoint into 706b2dd on master.

jskelcy · 2018-01-08T20:00:02Z

service/r2/service.go

+			id = uuid.New()
+			mr.ID = id
+		}
+		rss.MappingRules[id] = mr.mappingRuleView()


If the mapping id is an empty string and opts.generateMissingID is false this will continuously overwrite rss.MappingRules[""]. Im not sure what the best behavior is, maybe just leave it out of the list? But I think just taking whatever the last rule is and adding that to the snapshot while dropping all the other ones is kind of confusing.

Updated per in person conversation

jskelcy

address the empty string rule id issue.

coveralls · 2018-01-08T23:19:35Z

Coverage increased (+4.0%) to 56.772% when pulling abc04c3 on add-validation-endpoint into 706b2dd on master.

coveralls · 2018-01-08T23:19:36Z

Coverage increased (+4.0%) to 56.772% when pulling abc04c3 on add-validation-endpoint into 706b2dd on master.

xichen2020 · 2018-01-09T23:45:24Z

service/r2/kv/options.go

@@ -96,3 +104,13 @@ func (o *storeOptions) SetRuleUpdatePropagationDelay(value time.Duration) StoreO
 func (o *storeOptions) RuleUpdatePropagationDelay() time.Duration {
 	return o.ruleUpdatePropagationDelay
 }
+
+func (o *storeOptions) SetValidator(value rules.Validator) StoreOptions {
+	opts := *o


Yeah so this is to ensure the options are immutable.

xichen2020 · 2018-01-09T23:46:55Z

service/r2/kv/store.go

+	validator := s.opts.Validator()
+	// No validator is set so by default the ruleSetSnapshot is valid
+	if validator == nil {
+		return nil


Perhaps we should return an error here instead? If we accidentally forget setting the validator in the options, and someone calls the validation endpoint, we should return an error saying the validator is not set instead of returning ok?

xichen2020 · 2018-01-09T23:49:52Z

service/r2/service.go

@@ -36,6 +36,7 @@ import (
 	"github.com/m3db/m3metrics/rules"
 	"github.com/m3db/m3x/clock"
 	"github.com/m3db/m3x/instrument"
+	"github.com/pborman/uuid"


Fine w/ either, as long as there's only one for the repo.

Also, please add a blank line between m3db imports and 3rd party imports.

xichen2020 · 2018-01-10T00:17:15Z

service/r2/service.go

@@ -192,6 +193,7 @@ func (s *service) RegisterHandlers(router *mux.Router) {

 	router.Handle(namespacePath, h.wrap(s.fetchNamespaces)).Methods(http.MethodGet)
 	router.Handle(namespacePath, h.wrap(s.createNamespace)).Methods(http.MethodPost)
+	router.Handle(namespaceValidatePath, h.wrap(s.validateNamespace)).Methods(http.MethodPost)


I think we can make the NewAuthHandler take an additional parameter authType:

type authorizationType int const ( none authorizationType readOnly writeOnly readWrite ) func (a simpleAuth) NewAuthHandler( authType authorizationType, next http.Handler, errHandler errorResponseHandler, ) http.Handler { ... } return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { ... switch authType { case none: ... case readOnly: ... case writeOnly: ... case readWrite: ... }

Then in service.go, register the auth type

func registerRoute(router *mux.Router, path, method string, handler http.Handler) error { authType, exists := authRegistry[endpoint{Path:path, Method: method}] if !exists { // Should make this a method instead. switch method: case http.GET: authType = readOnly case http.POST, ...: authType = writeOnly default: return errUnknownHTTPMethod } fn := h.wrap(authType, handler) router.Handle(path, fn).Methods(method) return nil } func (h r2Handler) wrap(authType authorizationType, fn r2HandlerFunc) http.Handler { f := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if err := fn(w, r); err != nil { h.handleError(w, err) } }) return h.auth.NewAuthHandler(authType, f, writeAPIResponse) } type endpoint struct { Path string Method string } var authRegistry = map[endpoint]authorizationType{ {Path: namespaceValidePath, Method: http.POST}: readOnly }

xichen2020 · 2018-01-10T00:19:31Z

service/r2/service.go

+
+// Creates a new RuleSetSnapshot from a rulesetJSON. If the ruleSetJSON has no IDs for any of its
+// mapping rules or rollup rules, it generates missing IDs and sets as a string UUID string so they
+// can be stored in a mapping (id -> rule).


nit: Change the comment to ruleSetSnapshot creates a ....

You might also make this behavior controlled by an enum passed in though (e.g., genIDType), e.g., if this is an update request, we probably shouldn't generate the missing IDs since that should be a validation error.

xichen2020 · 2018-01-10T00:23:57Z

service/r2/service.go

+	rss := rules.RuleSetSnapshot{
+		Namespace:    r.Namespace,
+		Version:      r.Version,
+		MappingRules: map[string]*rules.MappingRuleView{},


nit: it's more efficient to do the following to avoid repeated map reallocations.

mappingRules := make(map[string]*rules.MappingRuleView, len(r.MappingRules)) for _, mr := r.MappingRules { ... } rollupRules := make(map[string]*rules.RollupRuleView, len(r.RollupRules)) for _, rr := range r.RollupRules { .. } return rules.RuleSetSnapshot{ Namespace: r.Namespace, Version: r.Version, MappingRules: mappingRules, RollupRules: rollupRules, }

xichen2020 · 2018-01-10T00:24:15Z

service/r2/service_test.go

+import (
+	"testing"
+
+	"github.com/pborman/uuid"


Reorder the imports.

xichen2020 · 2018-01-10T00:25:15Z

service/r2/service_test.go

@@ -0,0 +1,481 @@
+// Copyright (c) 2017 Uber Technologies, Inc.


Brilliant, thanks for the tests! 👍

coveralls · 2018-01-10T21:46:29Z

Coverage increased (+4.2%) to 56.993% when pulling be98154 on add-validation-endpoint into 706b2dd on master.

coveralls · 2018-01-10T21:46:29Z

Coverage increased (+4.2%) to 56.993% when pulling be98154 on add-validation-endpoint into 706b2dd on master.

jskelcy · 2018-01-10T23:18:21Z

auth/simple.go

 		return a.authorizeUser(a.readWhitelistEnabled, a.readWhitelistedUserIDs, userID)
-	case http.MethodPost, http.MethodPut, http.MethodPatch, http.MethodDelete:
+	case AuthorizationTypeWriteOnly:


What is a write-only user? Who would fall into this bucket?

Write only users would be those that are say whitelisted to make writes but not whitelisted for reads. I can't really think of a current situation where that would be the case but theoretically it could happen (ie a service that writes data potentially sensitive and doesn't have permissions to access any of the read endpoints). I realize that this doesn't really fit our current uses but this is why it was included.

jskelcy · 2018-01-10T23:20:45Z

Looks like there is a significant overhaul to how auth works. Can you give a quick rundown on why you made the changes?

jskelcy · 2018-01-10T23:33:04Z

service/r2/kv/store.go

+	validator := s.opts.Validator()
+	// If no validator is set, then the validation functionality is not applicable
+	if validator == nil {
+		return errors.New("no validator set on StoreOptions so validation is not applicable")


You can use fmt.Errorf here dont need to mess with imports.

The general rule of thumb is actually to only use fmt.Errorf when you need to format some values as part of the error message. For errors that are just a const string, errors.New is preferred.

nit: make this a errNilValidator = errors.New("no validator set on StoreOptions so validation is not applicable")

coveralls · 2018-01-11T16:30:13Z

Coverage increased (+4.6%) to 57.4% when pulling c25d9fe on add-validation-endpoint into 706b2dd on master.

m-sandusky · 2018-01-11T16:31:47Z

@jskelcy In reference to explanation for the auth revamp:

This was done because previously, the Auth handler would authorize users based on HTTP verb sent by the request. Ie In order to be authorized for a POST request, you needed to be a whiltelisted writer and for GET requests, you needed to be a whitelisted reader. The validation endpoint brought up the situation where it is a POST request but you really shouldn't need write whitelist credentials to access it. Xi and I discussed that certain endpoints can have different needs for auth which is where this notion of authorization type comes into play. Each endpoint has an authorization type which is configured when registering the routes for the service (with defaults falling back to GET -> need read access, POST/PUT/DELETE/PATCH need write credentials).

xichen2020 · 2018-01-11T18:40:05Z

auth/auth.go

 type errorResponseHandler func(w http.ResponseWriter, code int, msg string) error

 const (
 	// UserIDField is a key
 	UserIDField keyType = iota
 )

+const (
+	// AuthorizationTypeNone is the no authorizationType case.
+	AuthorizationTypeNone AuthorizationType = iota


nit: These are a bit of verbose, perhaps just None, ReadOnly, WriteOnly, and ReadWrite?

Agreed that they're verbose but I'd argue for them based on clarity for a couple reasons:

Changing them to None/ReadOnly/etc reserves those names for the entire auth package

Due to the way Go does (or rather doesn't do) enums it's not clear that something like None or ReadOnly would be associated with an AuthorizationType

That's fair, in that case perhaps call them UnknownAuthorization, NoAuthorization, ReadOnlyAuthorization, etc.?

xichen2020 · 2018-01-11T18:40:38Z

auth/noop.go

@@ -37,7 +37,7 @@ func NewNoopAuth() HTTPAuthService {
 	return noopAuth{}
 }

-func (a noopAuth) NewAuthHandler(next http.Handler, errHandler errorResponseHandler) http.Handler {
+func (a noopAuth) NewAuthHandler(authType AuthorizationType, next http.Handler, errHandler errorResponseHandler) http.Handler {


nit: omit the parameter name authType or use _ since it's not used here.

xichen2020 · 2018-01-11T18:41:46Z

auth/simple.go

+	switch authType {
+	case AuthorizationTypeNone:
+		return nil
+	case AuthorizationTypeReadOnly:
 		return a.authorizeUser(a.readWhitelistEnabled, a.readWhitelistedUserIDs, userID)


nit: This can be moved to a a.authorizeUserForRead method

xichen2020 · 2018-01-11T18:42:53Z

auth/simple.go

 		return a.authorizeUser(a.readWhitelistEnabled, a.readWhitelistedUserIDs, userID)
-	case http.MethodPost, http.MethodPut, http.MethodPatch, http.MethodDelete:
+	case AuthorizationTypeWriteOnly:
+		return a.authorizeUser(a.writeWhitelistEnabled, a.writeWhitelistedUserIDs, userID)


nit: This can be moved to a a.authorizeUserForWrite method.

xichen2020 · 2018-01-11T18:43:22Z

auth/simple.go

+	case AuthorizationTypeWriteOnly:
+		return a.authorizeUser(a.writeWhitelistEnabled, a.writeWhitelistedUserIDs, userID)
+	case AuthorizationTypeReadWrite:
+		err := a.authorizeUser(a.readWhitelistEnabled, a.readWhitelistedUserIDs, userID)


nit: This can reusea.authorizeUserForRead and a.authorizeUserForWrite methods defined above.

xichen2020 · 2018-01-11T18:52:39Z

service/r2/service.go

-	router.Handle(rollupRuleWithIDPath, h.wrap(s.updateRollupRule)).Methods(http.MethodPut, http.MethodPatch)
-	router.Handle(rollupRuleWithIDPath, h.wrap(s.deleteRollupRule)).Methods(http.MethodDelete)
+	registerRoute(router, rollupRuleWithIDPath, http.MethodGet, h, s.fetchRollupRule)
+	registerRoute(router, rollupRuleWithIDPath, http.MethodPut, h, s.updateRollupRule)


Same question as above.

xichen2020 · 2018-01-11T18:53:27Z

service/r2/service.go

@@ -553,3 +600,49 @@ type ruleSetJSON struct {
 	MappingRules  []mappingRuleJSON `json:"mappingRules"`
 	RollupRules   []rollupRuleJSON  `json:"rollupRules"`
 }
+
+type genID int


nit: idGenType might be better.

xichen2020 · 2018-01-11T18:53:34Z

service/r2/service.go

+type genID int
+
+const (
+	genIDFalse genID = iota


generateID

xichen2020 · 2018-01-11T18:53:46Z

service/r2/service.go

+
+const (
+	genIDFalse genID = iota
+	genIDTrue


dontGenerateID.

xichen2020 · 2018-01-11T18:54:10Z

service/r2/service.go

+// for any of its mapping rules or rollup rules, it generates missing IDs and sets as a string UUID
+// string so they can be stored in a mapping (id -> rule).
+func (r ruleSetJSON) ruleSetSnapshot(genID genID) (*rules.RuleSetSnapshot, error) {
+


Remove blank line.

coveralls · 2018-01-11T22:04:07Z

Coverage increased (+2.6%) to 55.832% when pulling c4c62a8 on add-validation-endpoint into 98073fe on master.

coveralls · 2018-01-11T22:04:07Z

Coverage increased (+2.6%) to 55.832% when pulling c4c62a8 on add-validation-endpoint into 98073fe on master.

jskelcy · 2018-01-11T22:32:40Z

service/r2/convert.go

+type idGenType int
+
+const (
+	generateID idGenType = iota


NIT: Given that this is a flag, and probably always will be, this name is a little misleading. Making it sound like there could be more than 2 values.

jskelcy · 2018-01-11T22:39:24Z

service/health/service.go

@@ -61,10 +61,11 @@ func (s *service) URLPrefix() string {
 	return healthURL
 }

-func (s *service) RegisterHandlers(router *mux.Router) {
+func (s *service) RegisterHandlers(router *mux.Router) error {


What does this function need to return something now?

jskelcy · 2018-01-11T22:41:25Z

service/r2/service.go

+	if !exists {
+		var err error
+		if authType = defaultAuthorizationTypeForHTTPMethod(method); authType == auth.AuthorizationTypeUnknown {
+			return err


are we returning an empty error here?

coveralls · 2018-01-11T23:09:24Z

Coverage increased (+2.7%) to 55.906% when pulling 4e8d3a2 on add-validation-endpoint into 98073fe on master.

xichen2020 · 2018-01-12T00:56:43Z

auth/simple.go

+	}
+}
+
+func (a simpleAuthorization) authorizeUserForRead(userID string) error {


Can we pull the common code from authorizeUserForRead and authorizeUserForWrite into a common authorizeUserForAccess function and reuse it? The authorizeUserForAccess implementation can just be the same as the old authorizeUser function.

xichen2020 · 2018-01-12T00:57:52Z

service/r2/convert.go

 	"github.com/m3db/m3metrics/policy"
 	"github.com/m3db/m3metrics/rules"
+	"github.com/pborman/uuid"


Blank line between m3db imports and 3rd party imports.

xichen2020 · 2018-01-12T00:58:07Z

service/r2/convert.go

@@ -195,3 +198,49 @@ func newRuleSetJSON(latest *rules.RuleSetSnapshot) ruleSetJSON {
 		RollupRules:   rrJSON,
 	}
 }
+
+// TODO(@SANDUSKY) MOVE THIS


Is this TODO still necessary?

xichen2020 · 2018-01-12T00:59:03Z

service/r2/convert_test.go

+	"testing"
+
+	"github.com/m3db/m3metrics/policy"
+	"github.com/m3db/m3metrics/rules"


Blank line between m3db imports and 3rd party imports.

xichen2020 · 2018-01-12T01:00:02Z

service/r2/kv/store.go

@@ -37,6 +38,8 @@ type store struct {
 	updateHelper rules.RuleSetUpdateHelper
 }

+var errNilValidator = errors.New("no validator set on StoreOptions so validation is not applicable")


super nit: might read a little better to say "no validator set so validation is not applicable".

xichen2020 · 2018-01-12T01:00:17Z

service/r2/kv/store.go

@@ -73,6 +76,15 @@ func (s *store) FetchNamespaces() (*rules.NamespacesView, error) {
 	}, nil
 }

+func (s *store) ValidateRuleSet(rs *rules.RuleSetSnapshot) error {
+	validator := s.opts.Validator()
+	// If no validator is set, then the validation functionality is not applicable


Comments should end w/ a period.

xichen2020 · 2018-01-12T01:01:45Z

service/r2/service.go

@@ -96,6 +99,39 @@ func newServiceMetrics(scope tally.Scope, samplingRate float64) serviceMetrics {
 	}
 }

+type route struct {


super nit: might wanna consider moving route to the bottom so the service related pieces are in one piece.

xichen2020 · 2018-01-12T01:02:58Z

service/r2/service.go

+	{path: validateRuleSetPath, method: http.MethodPost}: auth.AuthorizationTypeReadOnly,
+}
+
+func defaultAuthorizationTypeForHTTPMethod(method string) auth.AuthorizationType {


I'd still return an error here just to make it very explicit that if we don't see one of the expected methods, it should be an error, and check error in registerRoute.

xichen2020 · 2018-01-12T01:04:04Z

services/r2ctl/main/main.go

-	server := http.NewServer(listenAddr, httpServerOpts, r2Service, healthService)
+	server, err := http.NewServer(listenAddr, httpServerOpts, r2Service, healthService)
+	if err != nil {
+		logger.Fatalf("could not initialize new server: %v", err)


super nit: initialize -> create.

xichen2020 · 2018-01-12T01:08:44Z

service/r2/service.go

 	// Namespaces action
 	h := r2Handler{s.logger, s.authService}

-	router.Handle(namespacePath, h.wrap(s.fetchNamespaces)).Methods(http.MethodGet)
-	router.Handle(namespacePath, h.wrap(s.createNamespace)).Methods(http.MethodPost)
+	// Namespaces actions


Might be more cleaner to do:

// Register routes. routeWithHandlers := []struct{ route route handler r2HandlerFnc } { {route: {path: namespacePath: method: http.MethodGet}, handler: s.fetchNamespaces}, {route: {path: namespacePath: method: http.MethodPost}, handler: s.createNamespace}, ... } for _, rh := range routeWithHandlers { if err := registerRoute(router, rh.route.path, rh.route.method, h, rh.handler); err != nil { return err } } return nil

coveralls · 2018-01-12T16:49:36Z

Coverage increased (+3.8%) to 56.991% when pulling e94056e on add-validation-endpoint into 98073fe on master.

coveralls · 2018-01-12T16:49:36Z

Coverage increased (+3.8%) to 56.991% when pulling e94056e on add-validation-endpoint into 98073fe on master.

xichen2020 · 2018-01-12T17:14:04Z

auth/simple.go

-	}
-
-	for _, u := range a.readWhitelistedUserIDs {
+func authorizeUserForAccess(userID string, whitelistedUserIDs []string) error {


nit: could take a boolean parameter enabled and further extract the enabled check in the read and write methods below.

xichen2020 · 2018-01-12T17:15:41Z

service/r2/service.go

-			return fmt.Errorf("unknown authorization type for method %s at path %s", method, path)
+		var err error
+		if authType, err = defaultAuthorizationTypeForHTTPMethod(method); err != nil {
+			return fmt.Errorf("could not register route for path %s and method %s, error: %v", method, path, err)


swap method and path?

Thanks for the catch.

xichen2020 · 2018-01-12T17:34:07Z

LGTM, good stuff!

Might wanna wait for travis though.

xichen2020

LGTM

coveralls · 2018-01-12T17:34:49Z

Coverage increased (+4.1%) to 57.333% when pulling 88cc790 on add-validation-endpoint into 98073fe on master.

coveralls · 2018-01-12T17:34:49Z

Coverage increased (+4.1%) to 57.333% when pulling 88cc790 on add-validation-endpoint into 98073fe on master.

Add validate namespace endpoint to m3ctl

2bbda24

m-sandusky requested review from jskelcy and xichen2020 January 3, 2018 20:44

Refactor endpoint and store to use RuleSetSnapshot

136e0a5

Add namespaceID check in route

e4dd37f

xichen2020 reviewed Jan 5, 2018

View reviewed changes

jskelcy reviewed Jan 5, 2018

View reviewed changes

m-sandusky added 2 commits January 8, 2018 12:04

Add tests for rules JSON and View conversion functions

6171e09

Address review comments

658b6a7

m-sandusky commented Jan 8, 2018

View reviewed changes

jskelcy reviewed Jan 8, 2018

View reviewed changes

Further address review comments

3965184

jskelcy reviewed Jan 8, 2018

View reviewed changes

jskelcy suggested changes Jan 8, 2018

View reviewed changes

Update ruleSetSnapshot method

abc04c3

xichen2020 reviewed Jan 10, 2018

View reviewed changes

Address review comments. Add authType functionality to AuthHandler.

be98154

jskelcy reviewed Jan 10, 2018

View reviewed changes

Change auth type for post,put,del

c25d9fe

xichen2020 reviewed Jan 11, 2018

View reviewed changes

m-sandusky added 3 commits January 11, 2018 16:00

Address further review feedback

3b35e7a

Propagate route register errors upwards

2e53da3

Merge branch 'master' into add-validation-endpoint

c4c62a8

jskelcy reviewed Jan 11, 2018

View reviewed changes

jskelcy approved these changes Jan 11, 2018

View reviewed changes

Fix err check in register routes

4e8d3a2

xichen2020 reviewed Jan 12, 2018

View reviewed changes

Update with review feedback

e94056e

xichen2020 reviewed Jan 12, 2018

View reviewed changes

Address comments

88cc790

xichen2020 approved these changes Jan 12, 2018

View reviewed changes

m-sandusky merged commit 09cd37e into master Jan 12, 2018

		@@ -0,0 +1,481 @@
		// Copyright (c) 2017 Uber Technologies, Inc.

Add validate namespace endpoint to m3ctl #50

Add validate namespace endpoint to m3ctl #50

Conversation

m-sandusky commented Jan 3, 2018

coveralls commented Jan 3, 2018

coveralls commented Jan 3, 2018

coveralls commented Jan 3, 2018

coveralls commented Jan 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m-sandusky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jan 8, 2018

coveralls commented Jan 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jskelcy Jan 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jan 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jskelcy left a comment

Choose a reason for hiding this comment

coveralls commented Jan 8, 2018

coveralls commented Jan 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jan 10, 2018

coveralls commented Jan 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jskelcy commented Jan 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jan 11, 2018 • edited Loading

m-sandusky commented Jan 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jskelcy Jan 8, 2018 •

edited

Loading

coveralls commented Jan 8, 2018 •

edited

Loading

coveralls commented Jan 11, 2018 •

edited

Loading