[api] support zone/env overrides in ns handler #1427
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1427 +/- ##
========================================
+ Coverage 70.8% 70.8% +<.1%
========================================
Files 834 835 +1
Lines 71487 71498 +11
========================================
+ Hits 50660 50676 +16
+ Misses 17522 17521 -1
+ Partials 3305 3301 -4
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #1427 +/- ##
========================================
+ Coverage 70.8% 70.8% +<.1%
========================================
Files 834 835 +1
Lines 71487 71498 +11
========================================
+ Hits 50645 50676 +31
+ Misses 17534 17521 -13
+ Partials 3308 3301 -7
Continue to review full report at Codecov.
|
| @@ -205,7 +205,8 @@ func (h *createHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { | |||
| return | |||
| } | |||
|
|
|||
| nsRegistry, err = h.namespaceAddHandler.Add(namespaceRequest) | |||
| opts := handler.NewServiceOptions("kv", r.Header, nil) | |||
There was a problem hiding this comment.
Why are you passing "kv" as the service name? Shouldn't it be M3DB?
There was a problem hiding this comment.
Yeah I didn't quite know what to pass here. The namespace handler doesn't really use the service value it gets passed (only placement cares about that), as namespaces are stored at _kv/$env/$key. Because it's controlling KV flags I passed kv as the "service name" but can change this to m3db if it reads less awkwardly. Even "" would probably be fine
There was a problem hiding this comment.
Yeah I think the reason the namespace handler doesn't use a service value is because the only service that has namespaces is M3DB. Still I think it would read a little more cleanly if we passed the M3DB constant here, and then maybe a guard statement in the namespace handler to make sure the service is set to M3DB or return an error
|
|
||
| // Validate ensures the service options are valid. | ||
| func (opts *ServiceOptions) Validate() error { | ||
| if opts.ServiceName == "" { |
There was a problem hiding this comment.
Can we have a slice of allowed service names and do that validation here
There was a problem hiding this comment.
I had trouble with this as well. We have the concept of "allowed services" in the placement handler: https://github.com/m3db/m3/blob/f8508a401e1055c7d7fdaee22982fd48858b3e37/src/query/api/v1/handler/placement/common.go#L151
I tried moving this to the common handler package, but it's used in a few other places in the placement handler. Since it turns out that the placement handler is the only thing that cares about service name (see above comment), I left it there since namespace doesn't need it and I didn't want to have to expose it. Can change that depending on how we feel about having to expose it or not
There was a problem hiding this comment.
I would just add a new slice in this package called ValidServices or something and put every M3Coordinator/M3Aggregator/M3DB in it and call it a day. It would just be nice to know that anywhere we're using this thing that value won't be garbage
|
@richardartoul good call, refactored the allowed services part |
| func AllowedServices() []string { | ||
| svcs := make([]string, 0, len(allowedServices)) | ||
| for svc, allowed := range allowedServices { | ||
| if allowed { |
There was a problem hiding this comment.
Its a little weird to me that in IsAllowedService you ignore the value of the boolean, but here you care about it? Maybe never check the boolean or use struct{}{} as the value to avoid the issue
The placement APIs support zone/environment overrides via HTTP headers. This adds that functionality to the namespace APIs so users can manage multiple M3DB clusters from a single coordinator.
schallert
force-pushed
the
schallert/namespace_http_headers
branch
from
7a2c64e to
f911a3e
Compare
The placement APIs support zone/environment overrides via HTTP headers.
This adds that functionality to the namespace APIs so users can manage
multiple M3DB clusters from a single coordinator.