Re: Review Request 48614: Added validation for UUID's to master/agent validation helpers.

[Mesos ReviewBot]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/48614/#review138583
---



Patch looks great!

Reviews applied: [48613, 48901, 48614]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker_build.sh

- Mesos ReviewBot


On June 20, 2016, 5:41 a.m., Deshna Jain wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48614/
> ---
> 
> (Updated June 20, 2016, 5:41 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-5224
> https://issues.apache.org/jira/browse/MESOS-5224
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change adds logic to the master/agent validation helpers
> to see if the status update acknowledgments have a valid UUID
> set conforming to one of the supported versions.
> 
> 
> Diffs
> -
> 
>   src/master/validation.cpp 7f139fb0a67281c56fcb2bc2afe086c3382dff30 
>   src/slave/validation.cpp 86e95a26fc7b7ee156a17f3ab3ffa2393e40f98a 
>   src/tests/scheduler_http_api_tests.cpp 
> c12205f90e4f2da2c6ad7a0ea75777601eb0ef13 
> 
> Diff: https://reviews.apache.org/r/48614/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Deshna Jain
> 
>

Re: Review Request 48614: Added validation for UUID's to master/agent validation helpers.

[Deshna Jain]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/48614/
---

(Updated June 20, 2016, 5:41 a.m.)


Review request for mesos and Vinod Kone.


Changes
---

Run review bot.


Bugs: MESOS-5224
https://issues.apache.org/jira/browse/MESOS-5224


Repository: mesos


Description
---

This change adds logic to the master/agent validation helpers
to see if the status update acknowledgments have a valid UUID
set conforming to one of the supported versions.


Diffs (updated)
-

  src/master/validation.cpp 7f139fb0a67281c56fcb2bc2afe086c3382dff30 
  src/slave/validation.cpp 86e95a26fc7b7ee156a17f3ab3ffa2393e40f98a 
  src/tests/scheduler_http_api_tests.cpp 
c12205f90e4f2da2c6ad7a0ea75777601eb0ef13 

Diff: https://reviews.apache.org/r/48614/diff/


Testing
---


Thanks,

Deshna Jain

Re: Review Request 48614: Added validation for UUID's to master/agent validation helpers.

[Vinod Kone]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/48614/#review138505
---


Ship it!




Ship It!

- Vinod Kone


On June 18, 2016, 9:16 p.m., Deshna Jain wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48614/
> ---
> 
> (Updated June 18, 2016, 9:16 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-5224
> https://issues.apache.org/jira/browse/MESOS-5224
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change adds logic to the master/agent validation helpers
> to see if the status update acknowledgments have a valid UUID
> set conforming to one of the supported versions.
> 
> 
> Diffs
> -
> 
>   src/master/validation.cpp 7f139fb0a67281c56fcb2bc2afe086c3382dff30 
>   src/slave/validation.cpp 86e95a26fc7b7ee156a17f3ab3ffa2393e40f98a 
>   src/tests/scheduler_http_api_tests.cpp 
> c12205f90e4f2da2c6ad7a0ea75777601eb0ef13 
> 
> Diff: https://reviews.apache.org/r/48614/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Deshna Jain
> 
>

Re: Review Request 48614: Added validation for UUID's to master/agent validation helpers.

[Mesos ReviewBot]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/48614/#review138441
---



Patch looks great!

Reviews applied: [48613, 48901, 48614]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker_build.sh

- Mesos ReviewBot


On June 18, 2016, 9:16 p.m., Deshna Jain wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48614/
> ---
> 
> (Updated June 18, 2016, 9:16 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-5224
> https://issues.apache.org/jira/browse/MESOS-5224
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change adds logic to the master/agent validation helpers
> to see if the status update acknowledgments have a valid UUID
> set conforming to one of the supported versions.
> 
> 
> Diffs
> -
> 
>   src/master/validation.cpp 7f139fb0a67281c56fcb2bc2afe086c3382dff30 
>   src/slave/validation.cpp 86e95a26fc7b7ee156a17f3ab3ffa2393e40f98a 
>   src/tests/scheduler_http_api_tests.cpp 
> c12205f90e4f2da2c6ad7a0ea75777601eb0ef13 
> 
> Diff: https://reviews.apache.org/r/48614/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Deshna Jain
> 
>

Re: Review Request 48614: Added validation for UUID's to master/agent validation helpers.

[Deshna Jain]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/48614/
---

(Updated June 18, 2016, 9:16 p.m.)


Review request for mesos and Vinod Kone.


Changes
---

Rebased.


Bugs: MESOS-5224
https://issues.apache.org/jira/browse/MESOS-5224


Repository: mesos


Description
---

This change adds logic to the master/agent validation helpers
to see if the status update acknowledgments have a valid UUID
set conforming to one of the supported versions.


Diffs (updated)
-

  src/master/validation.cpp 7f139fb0a67281c56fcb2bc2afe086c3382dff30 
  src/slave/validation.cpp 86e95a26fc7b7ee156a17f3ab3ffa2393e40f98a 
  src/tests/scheduler_http_api_tests.cpp 
c12205f90e4f2da2c6ad7a0ea75777601eb0ef13 

Diff: https://reviews.apache.org/r/48614/diff/


Testing
---


Thanks,

Deshna Jain

Re: Review Request 48614: Added validation for UUID's to master/agent validation helpers.

[Mesos ReviewBot]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/48614/#review138394
---



Bad patch!

Reviews applied: [48614, 48901, 48613]

Failed command: ./support/apply-review.sh -n -r 48901

Error:
2016-06-18 09:56:56 URL:https://reviews.apache.org/r/48901/diff/raw/ 
[15903/15903] -> "48901.patch" [1]
error: patch failed: src/master/master.cpp:4340
error: src/master/master.cpp: patch does not apply

Full log: https://builds.apache.org/job/mesos-reviewbot/13871/console

- Mesos ReviewBot


On June 18, 2016, 8:21 a.m., Deshna Jain wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48614/
> ---
> 
> (Updated June 18, 2016, 8:21 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-5224
> https://issues.apache.org/jira/browse/MESOS-5224
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change adds logic to the master/agent validation helpers
> to see if the status update acknowledgments have a valid UUID
> set conforming to one of the supported versions.
> 
> 
> Diffs
> -
> 
>   src/master/validation.cpp 7b9c2281b2ab1295211f0dd385cd77947fbd63be 
>   src/slave/validation.cpp 189d10caf4517a1aff62b523d8eb99d9d1ca4b6a 
>   src/tests/scheduler_http_api_tests.cpp 
> c12205f90e4f2da2c6ad7a0ea75777601eb0ef13 
> 
> Diff: https://reviews.apache.org/r/48614/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Deshna Jain
> 
>

Re: Review Request 48614: Added validation for UUID's to master/agent validation helpers.

[Deshna Jain]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/48614/
---

(Updated June 18, 2016, 8:21 a.m.)


Review request for mesos and Vinod Kone.


Changes
---

Update deps


Bugs: MESOS-5224
https://issues.apache.org/jira/browse/MESOS-5224


Repository: mesos


Description
---

This change adds logic to the master/agent validation helpers
to see if the status update acknowledgments have a valid UUID
set conforming to one of the supported versions.


Diffs
-

  src/master/validation.cpp 7b9c2281b2ab1295211f0dd385cd77947fbd63be 
  src/slave/validation.cpp 189d10caf4517a1aff62b523d8eb99d9d1ca4b6a 
  src/tests/scheduler_http_api_tests.cpp 
c12205f90e4f2da2c6ad7a0ea75777601eb0ef13 

Diff: https://reviews.apache.org/r/48614/diff/


Testing
---


Thanks,

Deshna Jain

Re: Review Request 48614: Added validation for UUID's to master/agent validation helpers.

[Deshna Jain]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/48614/
---

(Updated June 18, 2016, 8:20 a.m.)


Review request for mesos and Vinod Kone.


Changes
---

Update deps


Bugs: MESOS-5224
https://issues.apache.org/jira/browse/MESOS-5224


Repository: mesos


Description
---

This change adds logic to the master/agent validation helpers
to see if the status update acknowledgments have a valid UUID
set conforming to one of the supported versions.


Diffs
-

  src/master/validation.cpp 7b9c2281b2ab1295211f0dd385cd77947fbd63be 
  src/slave/validation.cpp 189d10caf4517a1aff62b523d8eb99d9d1ca4b6a 
  src/tests/scheduler_http_api_tests.cpp 
c12205f90e4f2da2c6ad7a0ea75777601eb0ef13 

Diff: https://reviews.apache.org/r/48614/diff/


Testing
---


Thanks,

Deshna Jain

Re: Review Request 48614: Added validation for UUID's to master/agent validation helpers.

[Deshna Jain]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/48614/
---

(Updated June 18, 2016, 7:58 a.m.)


Review request for mesos and Vinod Kone.


Changes
---

Fix up.


Bugs: MESOS-5224
https://issues.apache.org/jira/browse/MESOS-5224


Repository: mesos


Description
---

This change adds logic to the master/agent validation helpers
to see if the status update acknowledgments have a valid UUID
set conforming to one of the supported versions.


Diffs (updated)
-

  src/master/validation.cpp 7b9c2281b2ab1295211f0dd385cd77947fbd63be 
  src/slave/validation.cpp 189d10caf4517a1aff62b523d8eb99d9d1ca4b6a 
  src/tests/scheduler_http_api_tests.cpp 
c12205f90e4f2da2c6ad7a0ea75777601eb0ef13 

Diff: https://reviews.apache.org/r/48614/diff/


Testing
---


Thanks,

Deshna Jain

Re: Review Request 48614: Added validation for UUID's to master/agent validation helpers.

[Deshna Jain]

> On June 13, 2016, 7:41 a.m., Neil Conway wrote:
> > src/tests/scheduler_http_api_tests.cpp, line 974
> > 
> >
> > Why are we fetching the stream ID?
> 
> Vinod Kone wrote:
> looks like you forgot to set the stream ID header when sending the 
> acknowledge call. if you don't set it the BadRequest response might be due to 
> the missing stream id and not malformed uuid. after you fix this, make sure 
> you set an expectation on the response body and not just response status code.

@Neil - The Scheduler API needs a valid stream id header for subsequent non 
subscribe calls. We need this for the second acknowledge call.
@Vinod - Thanks for the pointing it out. I have added the check for response 
body in addition to setting stream Id header.


- Deshna


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/48614/#review137239
---


On June 18, 2016, 7:58 a.m., Deshna Jain wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48614/
> ---
> 
> (Updated June 18, 2016, 7:58 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-5224
> https://issues.apache.org/jira/browse/MESOS-5224
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change adds logic to the master/agent validation helpers
> to see if the status update acknowledgments have a valid UUID
> set conforming to one of the supported versions.
> 
> 
> Diffs
> -
> 
>   src/master/validation.cpp 7b9c2281b2ab1295211f0dd385cd77947fbd63be 
>   src/slave/validation.cpp 189d10caf4517a1aff62b523d8eb99d9d1ca4b6a 
>   src/tests/scheduler_http_api_tests.cpp 
> c12205f90e4f2da2c6ad7a0ea75777601eb0ef13 
> 
> Diff: https://reviews.apache.org/r/48614/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Deshna Jain
> 
>

Re: Review Request 48614: Added validation for UUID's to master/agent validation helpers.

[Vinod Kone]

> On June 13, 2016, 7:41 a.m., Neil Conway wrote:
> > src/tests/scheduler_http_api_tests.cpp, line 974
> > 
> >
> > Why are we fetching the stream ID?

looks like you forgot to set the stream ID header when sending the acknowledge 
call. if you don't set it the BadRequest response might be due to the missing 
stream id and not malformed uuid. after you fix this, make sure you set an 
expectation on the response body and not just response status code.


- Vinod


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/48614/#review137239
---


On June 13, 2016, 6:51 a.m., Deshna Jain wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48614/
> ---
> 
> (Updated June 13, 2016, 6:51 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-5224
> https://issues.apache.org/jira/browse/MESOS-5224
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change adds logic to the master/agent validation helpers
> to see if the status update acknowledgments have a valid UUID
> set conforming to one of the supported versions.
> 
> 
> Diffs
> -
> 
>   src/master/validation.cpp 309fbed81c9ff0ccc4ff4ee3ee70cf8f1fb2ac55 
>   src/slave/validation.cpp 189d10caf4517a1aff62b523d8eb99d9d1ca4b6a 
>   src/tests/scheduler_http_api_tests.cpp 
> c12205f90e4f2da2c6ad7a0ea75777601eb0ef13 
> 
> Diff: https://reviews.apache.org/r/48614/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Deshna Jain
> 
>

Re: Review Request 48614: Added validation for UUID's to master/agent validation helpers.

[Vinod Kone]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/48614/#review137407
---




src/master/validation.cpp (line 308)


`uuid` is a required field so don't need to do the if check here.

after addressing the previous comments this should be,

```
Try uuid = UUID::fromBytes(call.acknowledge.uuid());
if (uuid.isError()) {
  return Error("Invalid UUID: " + uuid.error());
}

```



src/slave/validation.cpp (line 148)


if blocks have "{" on the same line as the if statement. see above.

```
Try uuid = UUID::fromBytes(status.uuid());
if (uuid.isError()) {
  return Error("Invalid UUID: " + uuid.error());
}

```


- Vinod Kone


On June 13, 2016, 6:51 a.m., Deshna Jain wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48614/
> ---
> 
> (Updated June 13, 2016, 6:51 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-5224
> https://issues.apache.org/jira/browse/MESOS-5224
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change adds logic to the master/agent validation helpers
> to see if the status update acknowledgments have a valid UUID
> set conforming to one of the supported versions.
> 
> 
> Diffs
> -
> 
>   src/master/validation.cpp 309fbed81c9ff0ccc4ff4ee3ee70cf8f1fb2ac55 
>   src/slave/validation.cpp 189d10caf4517a1aff62b523d8eb99d9d1ca4b6a 
>   src/tests/scheduler_http_api_tests.cpp 
> c12205f90e4f2da2c6ad7a0ea75777601eb0ef13 
> 
> Diff: https://reviews.apache.org/r/48614/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Deshna Jain
> 
>

Re: Review Request 48614: Added validation for UUID's to master/agent validation helpers.

[Neil Conway]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/48614/#review137239
---



Nice first patch to Mesos!

I wonder whether it would be safer to include validation in `UUID::fromBytes()` 
instead -- e.g., to change `fromBytes()` to return `Try` instead. This 
would require changing more places in Mesos, but my concern with requiring 
separate validation is that we construct UUIDs from potentially untrusted data 
in many places; requiring validation to be done separately seems error-prone. 
What do you think?


src/tests/scheduler_http_api_tests.cpp (line 974)


Why are we fetching the stream ID?



src/tests/scheduler_http_api_tests.cpp (line 990)


Would be a little more concise to do:

```
frameworkId = event.get().get().subscribed().framework_id();
EXPECT_NE("", frameworkId.value());
```


- Neil Conway


On June 13, 2016, 6:51 a.m., Deshna Jain wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48614/
> ---
> 
> (Updated June 13, 2016, 6:51 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-5224
> https://issues.apache.org/jira/browse/MESOS-5224
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change adds logic to the master/agent validation helpers
> to see if the status update acknowledgments have a valid UUID
> set conforming to one of the supported versions.
> 
> 
> Diffs
> -
> 
>   src/master/validation.cpp 309fbed81c9ff0ccc4ff4ee3ee70cf8f1fb2ac55 
>   src/slave/validation.cpp 189d10caf4517a1aff62b523d8eb99d9d1ca4b6a 
>   src/tests/scheduler_http_api_tests.cpp 
> c12205f90e4f2da2c6ad7a0ea75777601eb0ef13 
> 
> Diff: https://reviews.apache.org/r/48614/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Deshna Jain
> 
>

Review Request 48614: Added validation for UUID's to master/agent validation helpers.

[Deshna Jain]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/48614/
---

Review request for mesos and Vinod Kone.


Bugs: MESOS-5224
https://issues.apache.org/jira/browse/MESOS-5224


Repository: mesos


Description
---

This change adds logic to the master/agent validation helpers
to see if the status update acknowledgments have a valid UUID
set conforming to one of the supported versions.


Diffs
-

  src/master/validation.cpp 309fbed81c9ff0ccc4ff4ee3ee70cf8f1fb2ac55 
  src/slave/validation.cpp 189d10caf4517a1aff62b523d8eb99d9d1ca4b6a 
  src/tests/scheduler_http_api_tests.cpp 
c12205f90e4f2da2c6ad7a0ea75777601eb0ef13 

Diff: https://reviews.apache.org/r/48614/diff/


Testing
---


Thanks,

Deshna Jain