aurora-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Kevin Sweeney" <kevi...@apache.org>
Subject Re: Review Request 30325: Implementing pulseJobUpdate RPC.
Date Tue, 03 Feb 2015 00:48:48 GMT


> On Feb. 2, 2015, 4:16 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java,
line 1535
> > <https://reviews.apache.org/r/30325/diff/3/?file=841928#file841928line1535>
> >
> >     This is too restrictive.  It means that _only_ the update coordinator role may
call this RPC, and that a user cannot build something to pulse their own updates.
> >     
> >     You should instead do what `isAdmin` does and fall back to the coordinator role
if direct auth fails.
> >     
> >     This is an unfortunate state of affairs, and hopefully the move to shiro dramatically
improves all this.
> 
> Maxim Khutornenko wrote:
>     I don't see how it's necessarily better. Pulsing can always be done under UPDATE_COORDINATOR
membership, which is specifically covering heartbeats only. The isAdmin() requires ROOT that
opens up everything else, including the ability to kill anyone's tasks in the claster. Isn't
that more restrictive in real life? We don't expect regular users having ROOT access, meaning
they will unlikely to get to write their own pulse routine due to that. 
>     
>     Or maybe you suggesting an approach similar to killTasks(), where an admin check
followed by a role validation? The problem here is that we don't have a job key to extract
the role for authorization. Perhaps we can change the RPC to accept a job key instead but
that would open up for a race we don't want to see (e.g. late pulse arrives for a wrong update
ID). We could probably get away with both updateId and jobKey in the API to avoid ambiguity,
or perhpas just updateId and role? I am open to suggestions.

In Shiro this will look something like

subject.checkPermission("update:resume:mesos:prod:labrat");

With role update_coordinator having:
```
update:*
```

role root having:
```
*
```

and user mesos having:
```
*:*:mesos
```

So they'd all be allowed.


- Kevin


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


On Jan. 30, 2015, 9:23 a.m., Maxim Khutornenko wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30325/
> -----------------------------------------------------------
> 
> (Updated Jan. 30, 2015, 9:23 a.m.)
> 
> 
> Review request for Aurora, David McLaughlin, Joshua Cohen, and Bill Farner.
> 
> 
> Bugs: AURORA-1009
>     https://issues.apache.org/jira/browse/AURORA-1009
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> Implemented the `pulseJobUpdate` RPC. Also, moved it into AuroraAdmin interface to support
AOP capability validation. 
> 
> The RB is diffed against https://reviews.apache.org/r/30225/
> 
> 
> Diffs
> -----
> 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 4f603f9e7ed004e53937a45ea8edf7241e15f5cf

>   src/main/java/org/apache/aurora/auth/CapabilityValidator.java 45ef643ebe57c1517cdae373574331ea302a8b74

>   src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 8c19f3b08135eb5f3098591ebf9931b42a086318

>   src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
03d1fba76c23570c2c4102a48daf5ce035ecaaa3 
> 
> Diff: https://reviews.apache.org/r/30325/diff/
> 
> 
> Testing
> -------
> 
> ./gradlew -Pq build
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message