mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Joseph Wu <jos...@mesosphere.io>
Subject Re: Review Request 52310: Switch the uid of the binary if a user is passed from the lib_logrotate.
Date Tue, 04 Oct 2016 01:26:03 GMT

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




src/slave/container_loggers/logrotate.cpp (lines 39 - 41)
<https://reviews.apache.org/r/52310/#comment219624>

    `#include <stout/os/su.hpp>` should go in alphabetical order (middle).



src/slave/container_loggers/logrotate.cpp 
<https://reviews.apache.org/r/52310/#comment219625>

    We tend to put two newlines between certain sections of the code.  
    
    A non-exhaustive list is:
    * include-headers
    * after `using ...;` statements
    * between function implementations (but not declarations)



src/slave/container_loggers/logrotate.cpp (lines 248 - 252)
<https://reviews.apache.org/r/52310/#comment219626>

    My bad for not saying this clearly ( https://reviews.apache.org/r/52310/#comment218796
).
    
    I actually meant the user should be switched one line further up.  There's no need to
split this logical block in half:
    ```
    // Asynchronously control the flow and size of logs.
    LogrotateLoggerProcess process(flags);
    spawn(&process);
    ```



src/slave/container_loggers/logrotate.cpp (lines 248 - 249)
<https://reviews.apache.org/r/52310/#comment219628>

    Typo: `logroate`



src/slave/container_loggers/logrotate.cpp (line 251)
<https://reviews.apache.org/r/52310/#comment219627>

    You should catch the return value here `Try<Nothing>` and see if it actually succeeded.
    
    In the case of the Logrotate logger, we're mostly concerned with the logger being run
as root, when the user's container is non-root.  It turns out that `logrotate` will handle
this situation safely (but it might refuse to rotate any logs).
    
    We can handle this in two ways:
    1) Exit the logger immediately.  The task won't be able to log anything, but an error
message will most likely end up in the agent's logs (unless the agent crashes exactly before
this).
    2) Log something (i.e. `LOG(WARNING)`) and continue.
    
    Because this is right near the beginning of the logger, I'd recommend (1).


- Joseph Wu


On Sept. 30, 2016, 6:34 p.m., Sivaram Kannan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52310/
> -----------------------------------------------------------
> 
> (Updated Sept. 30, 2016, 6:34 p.m.)
> 
> 
> Review request for mesos and Joseph Wu.
> 
> 
> Bugs: MESOS-5856
>     https://issues.apache.org/jira/browse/MESOS-5856
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Switch the uid of the binary if a user is passed from the lib_logrotate.
> 
> 
> Diffs
> -----
> 
>   src/slave/container_loggers/logrotate.cpp 431bc3cbb54e94359078e4dae0b32ad301393640

> 
> Diff: https://reviews.apache.org/r/52310/diff/
> 
> 
> Testing
> -------
> 
> 1. Run the mesos-logrotate-logger with un-priviledged user and verify whether the logs
are getting rotated. 
> 2. Run the mesos-logrotate-logger as root user and verify whether the logs are getting
rotated.
> 
> 
> Thanks,
> 
> Sivaram Kannan
> 
>


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