Respond to API requests with 503 when read-only mode

Review Request #8657 - Created Jan. 21, 2017 and updated

Kanghee Park
Review Board
master
8648
8810, 8861, 8811, 8812, 8824, 8847, 8677
e04ec08...
reviewboard, students

Read-only mode is a setting an admin can enable to prevent writes to
the database. This can be used when the site is under maintenence or
being upgraded. The previous commit added the siteconfig attribute and
a checkbox to toggle the mode. This commit adds a new 503 error to
Review Board's webapi. Furthermore, all requests that hit RB's API
using PUT, POST, DELETE methods while read-only mode is on, and
the user is not a superuser, will return the new 503 error. A utility
file read_only.py has been added which deals with the logic and
caching of determining when a user should be affected by read-only
mode.

Added new Python unit tests that send API requests as superuser and
non-superuser
Ran the Python unit test suite
Check that modifying/deleting review requests and comments works
properly

  • 0
  • 0
  • 20
  • 1
  • 21
Description From Last Updated
Review Bot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/base.py
        reviewboard/webapi/tests/test_base.py
        reviewboard/webapi/errors.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/base.py
        reviewboard/webapi/tests/test_base.py
        reviewboard/webapi/errors.py
    
    
  2. reviewboard/webapi/base.py (Diff revision 1)
     
     
    Col: 80
     E501 line too long (80 > 79 characters)
    
  3. reviewboard/webapi/tests/test_base.py (Diff revision 1)
     
     
     'SiteConfiguration' imported but unused
    
  4. reviewboard/webapi/tests/test_base.py (Diff revision 1)
     
     
    Col: 1
     E302 expected 2 blank lines, found 1
    
  5. 
      
Barret Rennie
  1. 
      
  2. reviewboard/webapi/base.py (Diff revision 1)
     
     
     
     
     

    request.user should always be an object. If the user is logged in, it will be a User. Otherwise it will be an AnonymousUser (for which is_superuser will be False).

    1. Hey Barret, I added this in because the unit tests were failing since the are manually created and do not include a user (as far as I could tell).

      AttributeError: 'WSGIRequest' object has no attribute 'user'
      

      Not only my new test, but at least the other tests in the file were failing without the check. Any alternative ideas?

    2. So when you use a RequestFactory, the default middleware do not get added to the request. Review Board uses AuthenticationMiddleware which adds the user attribute to requests. In the feature tests you will have to add request.user = AnonymousUser() aroun line 338.

    3. Thanks!!

  3. reviewboard/webapi/base.py (Diff revision 1)
     
     

    This needs a default value in reviewboard/admin/siteconfig.py

    1. I believe this is set in the previous commit. There, I set

      defaults.update({
          ...
          'site_read_only': False,
      
  4. reviewboard/webapi/base.py (Diff revision 1)
     
     

    The performance difference between set and tuple here is probably negligible, but we should probably just use tuple (i.e., ('POST', 'PUT', 'DELETE')).

    set is (I believe) backed by a red-black tree and so will have to allocate memory for the nodes, as well as do bunch of other overhead work.

  5. 
      
Kanghee Park
Review Bot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/admin/siteconfig.py
        reviewboard/admin/forms.py
        reviewboard/webapi/tests/test_base.py
        reviewboard/webapi/errors.py
        reviewboard/webapi/base.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/admin/siteconfig.py
        reviewboard/admin/forms.py
        reviewboard/webapi/tests/test_base.py
        reviewboard/webapi/errors.py
        reviewboard/webapi/base.py
    
    
  2. reviewboard/webapi/tests/test_base.py (Diff revision 2)
     
     
    Col: 39
     E703 statement ends with a semicolon
    
  3. 
      
Kanghee Park
Review Bot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/base.py
        reviewboard/webapi/tests/test_base.py
        reviewboard/webapi/errors.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/base.py
        reviewboard/webapi/tests/test_base.py
        reviewboard/webapi/errors.py
    
    
  2. 
      
Barret Rennie
  1. 
      
  2. reviewboard/webapi/tests/test_base.py (Diff revision 3)
     
     

    Missing docstring.

  3. reviewboard/webapi/tests/test_base.py (Diff revision 3)
     
     

    Docstrings for tests should begin with "Testing"

    1. Done. Reworded all of them to match the change to "Testing" better.

  4. reviewboard/webapi/tests/test_base.py (Diff revision 3)
     
     

    Can you pass the boolean params as keywords? It makes the test cases clearer.

    1. Thanks, I was thinking that too. Is the way its done in the new diff ok?

  5. 
      
Kanghee Park
Review Bot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/base.py
        reviewboard/webapi/tests/test_base.py
        reviewboard/webapi/errors.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/base.py
        reviewboard/webapi/tests/test_base.py
        reviewboard/webapi/errors.py
    
    
  2. reviewboard/webapi/tests/test_base.py (Diff revision 4)
     
     
    Col: 45
     E201 whitespace after '['
    
  3. 
      
Kanghee Park
Review Bot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/base.py
        reviewboard/webapi/tests/test_base.py
        reviewboard/webapi/errors.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/base.py
        reviewboard/webapi/tests/test_base.py
        reviewboard/webapi/errors.py
    
    
  2. 
      
Barret Rennie
  1. 
      
  2. Can you wrap your description at 72 characters?

  3. reviewboard/webapi/base.py (Diff revision 5)
     
     
     
     

    How about

    "If read-only mode is enabled, all PUT, POST, and DELETE requests will be rejected with a 503 Service Unavailable unless the user is a superuser."

  4. reviewboard/webapi/base.py (Diff revision 5)
     
     

    "these"

  5. 
      
Christian Hammond
  1. 
      
  2. reviewboard/webapi/base.py (Diff revision 5)
     
     

    There are other mutable types of requests that may be supported down the road. I'd change this to request.method not in ('GET', 'HEAD', 'OPTIONS'), since those are all safe.

  3. reviewboard/webapi/tests/test_base.py (Diff revision 5)
     
     
     

    Can you verify that all unit tests for Review Board pass? My concern is that the siteconfig changes made in the unit tests could leak. Just in case, could you have the tearDown() method reset site_read_only back to defaults?

    1. Ran ./tests/runtests.py and js-tests with the new changes and everything looks fine other than in py-tests, there are 2 failed tests due to perforce not being setup and 107 skipped. Is that sufficient?

  4. reviewboard/webapi/tests/test_base.py (Diff revision 5)
     
     
     

    Blank line between these.

  5. reviewboard/webapi/tests/test_base.py (Diff revision 5)
     
     
     

    Blank line between these.

  6. 
      
Christian Hammond
  1. 
      
  2. reviewboard/webapi/tests/test_base.py (Diff revision 5)
     
     
     
     

    Two blank lines.

  3. 
      
Kanghee Park
Review Bot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/base.py
        reviewboard/webapi/tests/test_base.py
        reviewboard/webapi/errors.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/base.py
        reviewboard/webapi/tests/test_base.py
        reviewboard/webapi/errors.py
    
    
  2. 
      
Kanghee Park
Review Bot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/base.py
        reviewboard/admin/read_only.py
        reviewboard/webapi/tests/test_base.py
        reviewboard/webapi/errors.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/base.py
        reviewboard/admin/read_only.py
        reviewboard/webapi/tests/test_base.py
        reviewboard/webapi/errors.py
    
    
  2. 
      
Kanghee Park
Barret Rennie
  1. 
      
  2. reviewboard/admin/read_only.py (Diff revision 7)
     
     
     
     

    I don't know that we need to cache this, since SiteConfiguration.objects.get_current() will only hit the database the first time its called. Every other call is cached.

    1. Hey Barret, this optimization was something Christian suggested here: https://reviews.reviewboard.org/r/8677/#comment36453
      Since this potentially gets called multiple times per page load, perhaps it's worth keeping this around?

  3. reviewboard/webapi/tests/test_base.py (Diff revision 7)
     
     
     

    Docstring.

  4. 
      
Kanghee Park
Review request changed

Checks run (3 succeeded)

JSHint passed.
PEP8 Style Checker passed.
Pyflakes passed.
Loading...