mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Andrei Sekretenko <asekrete...@apache.org>
Subject Re: Review Request 72843: Fixed a stale comment about delayed permissions when batching requests.
Date Wed, 09 Sep 2020 15:43:48 GMT


> On Sept. 8, 2020, 7:09 p.m., Benjamin Mahler wrote:
> > src/master/http.cpp
> > Lines 2368-2370 (original), 2368-2370 (patched)
> > <https://reviews.apache.org/r/72843/diff/1/?file=2239304#file2239304line2368>
> >
> >     A bit hard for me to understand how the comment relates to the code, I guess
what I should be getting out of this comment is that the object approvers are always functionally
equivalent now?
> >     
> >     Seems like the comment belongs up above in the de-duplication search?
> >     
> >     ```
> >             // NOTE: This is not a general-purpose request comparison, but
> >             // specific to the batched requests which are always members of
> >             // `ReadOnlyHandler`, since we rely on the response only depending
> >             // on query parameters and the current master state.
> >             // 
> >             // We don't need to check that the ObjectApprovers are the same,
> >             // since ...?
> >             return handler == batchedRequest.handler &&
> >                    principal == batchedRequest.principal &&
> >                    outputContentType == batchedRequest.outputContentType &&
> >                    queryParameters == batchedRequest.queryParameters;
> >     ```
> >     
> >     Down here it seems like there's nothing related to object approvers to comment
on.

Good point!

While moving the comment, realized how fragile the apporovers reuse actually is: regardless
of synchronous authorization, this code heavily relies on the fact that each caller always
provides `approvers` for the same set of actions for the same handler.

Makes me think about moving `ObjectApprovers` creation into this method (not 100% trivial
thanks to that <VIEW_ROLE> specialization in `ObjectApprovers`).
Added a TODO.


- Andrei


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


On Sept. 9, 2020, 5:38 p.m., Andrei Sekretenko wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72843/
> -----------------------------------------------------------
> 
> (Updated Sept. 9, 2020, 5:38 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Now that synchronous authorization requires the authorizer to keep
> object approvers up-to-date by the authorizer, batched processing
> of readonly requests in the Master no longer intorduces an additional
> delay into propagation of permission changes.
> 
> 
> Diffs
> -----
> 
>   src/master/http.cpp f34ea54ec48065f526327252aa10c6d917a96601 
> 
> 
> Diff: https://reviews.apache.org/r/72843/diff/2/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>


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