----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36720/#review93499 -----------------------------------------------------------
Looks like this needs a rebase? Wasn't able to get it applied without `--3way`. I'm going to pull out the call Call validation to hopefully make this a bit easier. Per my comment below, I'd suggest we try to tackle the 'validate' refactor in an independent patch to pull apart the authorization vs authentication checks. src/master/http.cpp (lines 324 - 379) <https://reviews.apache.org/r/36720/#comment147888> JSON should be do-able already, here's a suggestion to clean this up a bit (note that it is handling JSON): ``` Future<Response> Master::Http::call(const Request& request) const { scheduler::Call call; // TODO(anand): Content type values are case-insensitive. Option<string> contentType = request.headers.get("Content-Type"); if (contentType.isNone()) { return BadRequest("Expecting 'Content-Type' to be present"); } if (contentType.get() == APPLICATION_PROTOBUF) { if (!call.ParseFromString(request.body)) { return BadRequest("Failed to deserialize body into Call protobuf"); } } else if (contentType.get() == APPLICATION_JSON) { Try<JSON::Value> value = JSON::parse(request.body); if (value.isError()) { return BadRequest("Failed to parse body into JSON: " + value.error()); } Try<scheduler::Call> parse = ::protobuf::parse<scheduler::Call>(value.get()); if (parse.isError()) { return BadRequest("Failed to convert JSON into Call protobuf: " + parse.error()); } call = parse.get(); } // Default to sending back JSON. ContentType responseContentType = ContentType::JSON; // TODO(anand): Use request.accepts() once available. Option<string> acceptType = request.headers.get("Accept"); if (acceptType.get() == APPLICATION_PROTOBUF) { responseContentType = ContentType::PROTOBUF; } switch (call.type()) { case scheduler::Call::SUBSCRIBE: { Pipe pipe; OK ok; ok.type = Response::PIPE; ok.reader = pipe.reader(); master->subscribeHttp( call.subscribe(), responseContentType, pipe.writer()); return ok; } default: break; } return NotImplemented(); } ``` src/master/master.cpp (lines 2110 - 2141) <https://reviews.apache.org/r/36720/#comment147890> Any reason you're skipping validation (authorization) of the framework? Looks like we should pull out the pid specific code from validate for now: it looks like we already have to do synchronous checks inside the continuations anyway. For example, `_reregisterFramework` checks to see if it is authenticated, but it is missing the check that it is the right principal! Looks like we should s/validate/authorize/ and pull out an `isAuthenticated` that we can call synchronously as well. src/master/master.cpp (lines 2144 - 2175) <https://reviews.apache.org/r/36720/#comment147889> This was a bit confusing, since it says "registering" but you're planning to also have it include re-registration? This looks like `_subscribe` given it is the continuation. - Ben Mahler On July 25, 2015, 2:32 p.m., Anand Mazumdar wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/36720/ > ----------------------------------------------------------- > > (Updated July 25, 2015, 2:32 p.m.) > > > Review request for mesos and Ben Mahler. > > > Bugs: MESOS-2294 > https://issues.apache.org/jira/browse/MESOS-2294 > > > Repository: mesos > > > Description > ------- > > Split review out of r36318. This change adds the functionality of making a > http call for subscribe and the master responding with a subscribed event on > the persistent stream. > > > Diffs > ----- > > src/master/http.cpp 3a1598fad4db03e5f62fd4a6bd26b2bedeee4070 > src/master/master.hpp 827d0d599912b2936beb9615610f627f6c9a2d43 > src/master/master.cpp 613a011e205611702921179b6c436d62447e2dca > src/tests/http_api_tests.cpp 64bbeb6699171e85a5be293919ad9f32ded0ebac > > Diff: https://reviews.apache.org/r/36720/diff/ > > > Testing > ------- > > make check + test. > > > Thanks, > > Anand Mazumdar > >
