-----------------------------------------------------------
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
> 
>

Reply via email to