Merge "Docs: Add RBAC overview documentation"
diff --git a/HACKING.rst b/HACKING.rst
index f020063..28a977d 100644
--- a/HACKING.rst
+++ b/HACKING.rst
@@ -1,5 +1,5 @@
-Patrole Style Commandments
-==========================
+Patrole Coding Guide
+====================
- Step 1: Read the OpenStack Style Commandments: `<https://docs.openstack.org/hacking/latest/>`__
- Step 2: Review Tempest's Style Commandments: `<https://docs.openstack.org/tempest/latest/HACKING.html>`__
@@ -17,27 +17,28 @@
The original Tempest Commandments do not include Patrole-specific paths.
Patrole-specific paths replace the Tempest-specific paths within Patrole's
hacking checks.
-..
-- [T102] Cannot import OpenStack python clients in patrole_tempest_plugin/tests/api
+- [T102] Cannot import OpenStack python clients in
+ ``patrole_tempest_plugin/tests/api``
- [T105] Tests cannot use setUpClass/tearDownClass
- [T106] vim configuration should not be kept in source files.
- [T107] Check that a service tag isn't in the module path
- [T108] Check no hyphen at the end of rand_name() argument
- [T109] Cannot use testtools.skip decorator; instead use
- decorators.skip_because from tempest.lib
-- [T113] Check that tests use data_utils.rand_uuid() instead of uuid.uuid4()
+ ``decorators.skip_because`` from ``tempest.lib``
+- [T113] Check that tests use ``data_utils.rand_uuid()`` instead of
+ ``uuid.uuid4()``
- [N322] Method's default argument shouldn't be mutable
The following are Patrole's specific Commandments:
- [P100] The ``rbac_rule_validation.action`` decorator must be applied to
- an RBAC test
+ an RBAC test
- [P101] RBAC test filenames must end with "_rbac.py"; for example,
- test_servers_rbac.py, not test_servers.py
+ test_servers_rbac.py, not test_servers.py
- [P102] RBAC test class names must end in 'RbacTest'
- [P103] ``self.client`` must not be used as a client alias; this allows for
- code that is more maintainable and easier to read
+ code that is more maintainable and easier to read
Role Overriding
---------------
diff --git a/REVIEWING.rst b/REVIEWING.rst
new file mode 100644
index 0000000..7e3c71f
--- /dev/null
+++ b/REVIEWING.rst
@@ -0,0 +1,136 @@
+Reviewing Patrole Code
+======================
+To start read the `OpenStack Common Review Checklist
+<https://docs.openstack.org/infra/manual/developers.html#peer-review>`_
+
+
+Ensuring code is executed
+-------------------------
+Any new test or change to an existing test has to be verified in the gate. This
+means that the first thing to check with any change is that a gate job actually
+runs it. Tests which aren't executed either because of configuration or skips
+should not be accepted.
+
+
+Execution time
+--------------
+Along with checking that the jobs log that a new test is actually executed,
+also pay attention to the execution time of that test. Patrole already runs
+hundreds of tests per job in its check and gate pipelines and it is important
+that the overall runtime of the jobs be constrained as much as possible.
+Consider applying the ``@decorators.attr(type='slow')``
+`test attribute decorator`_ to a test if its runtime is longer than 30 seconds.
+
+.. _test attribute decorator: https://docs.openstack.org/tempest/latest/HACKING.html#test-attributes
+
+
+Unit Tests
+----------
+For any change that adds new functionality to common functionality unit tests
+are required. This is to ensure we don't introduce future regressions and to
+test conditions which we may not hit in the gate runs.
+
+
+API Stability
+-------------
+Tests should only be added for published stable APIs. If a patch contains
+tests for an API which hasn't been marked as stable or for an API which
+doesn't conform to the `API stability guidelines
+<https://wiki.openstack.org/wiki/Governance/Approved/APIStability>`_ then it
+should not be approved.
+
+Similarly, tests should only be added for policies that are covered by
+`policy in code documentation
+<https://specs.openstack.org/openstack/keystone-specs/specs/keystone/pike/policy-in-code.html>`_.
+Any existing tests that test policies not covered by such documentation
+are either:
+
+* part of a service that has not yet migrated to policy in code; or
+* legacy in the sense that they were created prior to policy in code
+
+For the first bullet, the tests should not be considered stable, but should be
+kept around to maintain coverage. These tests are a best-effort attempt at
+offering RBAC test coverage for the service that has not yet migrated to
+policy in code.
+
+For the second bullet, the tests should be updated to conform to policy in
+code documentation, if applicable.
+
+
+Reject Copy and Paste Test Code
+-------------------------------
+When creating new tests that are similar to existing tests it is tempting to
+simply copy the code and make a few modifications. This increases code size and
+the maintenance burden. Such changes should not be approved if it is easy to
+abstract the duplicated code into a function or method.
+
+
+Tests overlap
+-------------
+When a new test is being proposed, question whether this feature is not already
+tested with Patrole. Patrole has more than 600 tests, spread amongst many
+directories, so it's easy to introduce test duplication.
+
+Test Duplication
+^^^^^^^^^^^^^^^^
+
+Test duplication means:
+
+* testing an API endpoint in more than one test
+* testing the same policy in more than one test
+
+For the first bullet, try to avoid calling the same API inside the
+``self.rbac_utils.override_role`` call.
+
+.. note::
+
+ If the same API is tested against different policies, consider combining
+ the different tests into only 1 test, that tests the API against all
+ the policies it enforces.
+
+For the second bullet, try to avoid testing the same policies across multiple
+tests.
+
+.. note::
+
+ This is not always possible since policy granularity doesn't exist for all
+ APIs. In cases where policy granularity doesn't exist, make sure that the
+ policy overlap only exists for the non-granular APIs that enforce the same
+ policy.
+
+
+Being explicit
+--------------
+When tests are being added that depend on a configurable feature or extension,
+polling the API to discover that it is enabled should not be done. This will
+just result in bugs being masked because the test can be skipped automatically.
+Instead the config file should be used to determine whether a test should be
+skipped or not. Do not approve changes that depend on an API call to determine
+whether to skip or not.
+
+
+Release Notes
+-------------
+Release notes are how we indicate to users and other consumers of Patrole what
+has changed in a given release. There are certain types of changes that
+require release notes and we should not approve them without including a release
+note. These include but aren't limited to, any addition, deprecation or removal
+from the framework code, any change to configuration options (including
+deprecation), major feature additions, and anything backwards incompatible or
+would require a user to take note or do something extra.
+
+
+Deprecated Code
+---------------
+Sometimes we have some bugs in deprecated code. Basically, we leave it. Because
+we don't need to maintain it. However, if the bug is critical, we might need to
+fix it. When it will happen, we will deal with it on a case-by-case basis.
+
+
+When to approve
+---------------
+* Every patch needs two +2's before being approved.
+* It's OK to hold off on an approval until a subject matter expert reviews it.
+* If a patch has already been approved but requires a trivial rebase to merge,
+ you do not have to wait for a second +2, since the patch has already had
+ two +2's.
diff --git a/devstack/README.rst b/devstack/README.rst
new file mode 100644
index 0000000..053afd4
--- /dev/null
+++ b/devstack/README.rst
@@ -0,0 +1,25 @@
+====================
+Enabling in Devstack
+====================
+
+.. warning::
+
+ The ``stack.sh`` script must be run in a disposable VM that is not
+ being created automatically. See the `README file`_ in the DevStack
+ repository for more information.
+
+1. Download DevStack::
+
+ git clone https://git.openstack.org/openstack-dev/devstack.git
+ cd devstack
+
+2. Patrole can be installed like any other DevStack plugin by including the
+ ``enable_plugin`` directive inside local.conf::
+
+ > cat local.conf
+ [[local|localrc]]
+ enable_plugin patrole https://git.openstack.org/openstack/patrole
+
+3. Run ``stack.sh`` found in the DevStack repo.
+
+.. _README file: https://github.com/openstack-dev/devstack/blob/master/README.rst
diff --git a/doc/source/REVIEWING.rst b/doc/source/REVIEWING.rst
new file mode 100644
index 0000000..e8e3c1a
--- /dev/null
+++ b/doc/source/REVIEWING.rst
@@ -0,0 +1,5 @@
+======================
+Reviewing Patrole Code
+======================
+
+.. include:: ../../REVIEWING.rst
diff --git a/doc/source/index.rst b/doc/source/index.rst
index 001ca32..2dbf63b 100644
--- a/doc/source/index.rst
+++ b/doc/source/index.rst
@@ -53,6 +53,7 @@
:maxdepth: 1
HACKING
+ REVIEWING
Framework
---------
diff --git a/doc/source/installation.rst b/doc/source/installation.rst
index b9cc924..827239f 100644
--- a/doc/source/installation.rst
+++ b/doc/source/installation.rst
@@ -26,10 +26,4 @@
DevStack Installation
=====================
-Patrole can be installed like any other DevStack plugin by including the
-``install_plugin`` directive inside local.conf::
-
- [[local|localrc]]
- ...
-
- enable_plugin patrole git://git.openstack.org/openstack/patrole
+.. include:: ../../devstack/README.rst
diff --git a/lower-constraints.txt b/lower-constraints.txt
index 9422676..2b77dff 100644
--- a/lower-constraints.txt
+++ b/lower-constraints.txt
@@ -16,7 +16,7 @@
extras==1.0.0
fasteners==0.14.1
fixtures==3.0.0
-flake8==2.5.5
+flake8==2.6.2
future==0.16.0
hacking==1.0.0
idna==2.6
diff --git a/patrole_tempest_plugin/hacking/checks.py b/patrole_tempest_plugin/hacking/checks.py
index eb73ef1..d106da8 100644
--- a/patrole_tempest_plugin/hacking/checks.py
+++ b/patrole_tempest_plugin/hacking/checks.py
@@ -17,7 +17,7 @@
import os
import re
-import pep8
+import pycodestyle
PYTHON_CLIENTS = ['cinder', 'glance', 'keystone', 'nova', 'swift', 'neutron',
@@ -59,7 +59,7 @@
T105: Tests cannot use setUpClass/tearDownClass
"""
- if pep8.noqa(physical_line):
+ if pycodestyle.noqa(physical_line):
return
if SETUP_TEARDOWN_CLASS_DEFINITION.match(physical_line):
diff --git a/patrole_tempest_plugin/policy_authority.py b/patrole_tempest_plugin/policy_authority.py
index b813f88..3339a5d 100644
--- a/patrole_tempest_plugin/policy_authority.py
+++ b/patrole_tempest_plugin/policy_authority.py
@@ -136,7 +136,7 @@
if not service or service not in cls.available_services:
LOG.debug("%s is NOT a valid service.", service)
- raise rbac_exceptions.RbacInvalidService(
+ raise rbac_exceptions.RbacInvalidServiceException(
"%s is NOT a valid service." % service)
@classmethod
diff --git a/patrole_tempest_plugin/rbac_exceptions.py b/patrole_tempest_plugin/rbac_exceptions.py
index 980672a..809a7ed 100644
--- a/patrole_tempest_plugin/rbac_exceptions.py
+++ b/patrole_tempest_plugin/rbac_exceptions.py
@@ -64,7 +64,10 @@
"instead. Actual exception: %(exception)s")
-class RbacInvalidService(exceptions.TempestException):
+class RbacInvalidServiceException(exceptions.TempestException):
+ """Raised when an invalid service is passed to ``rbac_rule_validation``
+ decorator.
+ """
message = "Attempted to test an invalid service"
diff --git a/patrole_tempest_plugin/rbac_rule_validation.py b/patrole_tempest_plugin/rbac_rule_validation.py
index 2f6759d..e6d1e80 100644
--- a/patrole_tempest_plugin/rbac_rule_validation.py
+++ b/patrole_tempest_plugin/rbac_rule_validation.py
@@ -22,7 +22,7 @@
import six
from tempest import config
-from tempest.lib import exceptions
+from tempest.lib import exceptions as lib_exc
from tempest import test
from patrole_tempest_plugin import policy_authority
@@ -123,7 +123,7 @@
"os_alt.auth_provider.credentials.user_id"
})
- :raises NotFound: If ``service`` is invalid.
+ :raises RbacInvalidServiceException: If ``service`` is invalid.
:raises RbacUnderPermissionException: For item (2) above.
:raises RbacOverPermissionException: For item (3) above.
:raises RbacExpectedWrongException: When a 403 is expected but a 404
@@ -184,12 +184,13 @@
try:
test_func(*args, **kwargs)
- except rbac_exceptions.RbacInvalidService as e:
- msg = ("%s is not a valid service." % service)
- test_status = ('Error, %s' % (msg))
- LOG.error(msg)
- raise exceptions.NotFound(
- "%s RbacInvalidService was: %s" % (msg, e))
+ except rbac_exceptions.RbacInvalidServiceException:
+ with excutils.save_and_reraise_exception():
+ msg = ("%s is not a valid service." % service)
+ # FIXME(felipemonteiro): This test_status is logged too
+ # late. Need a function to log it before re-raising.
+ test_status = ('Error, %s' % (msg))
+ LOG.error(msg)
except (expected_exception,
rbac_exceptions.RbacConflictingPolicies,
rbac_exceptions.RbacMalformedResponse) as e:
@@ -382,14 +383,13 @@
raise rbac_exceptions.RbacInvalidErrorCode(msg)
if expected_error_code == 403:
- expected_exception = exceptions.Forbidden
+ expected_exception = lib_exc.Forbidden
elif expected_error_code == 404:
- expected_exception = exceptions.NotFound
+ expected_exception = lib_exc.NotFound
irregular_msg = ("NotFound exception was caught for test %s. Expected "
"policies which may have caused the error: %s. The "
"service %s throws a 404 instead of a 403, which is "
"irregular.")
-
return expected_exception, irregular_msg
@@ -431,7 +431,7 @@
def _check_for_expected_mismatch_exception(expected_exception,
actual_exception):
- permission_exceptions = (exceptions.Forbidden, exceptions.NotFound)
+ permission_exceptions = (lib_exc.Forbidden, lib_exc.NotFound)
if isinstance(actual_exception, permission_exceptions):
if not isinstance(actual_exception, expected_exception.__class__):
return True
diff --git a/patrole_tempest_plugin/tests/api/compute/test_floating_ips_bulk_rbac.py b/patrole_tempest_plugin/tests/api/compute/test_floating_ips_bulk_rbac.py
index b83cc46..3ccef73 100644
--- a/patrole_tempest_plugin/tests/api/compute/test_floating_ips_bulk_rbac.py
+++ b/patrole_tempest_plugin/tests/api/compute/test_floating_ips_bulk_rbac.py
@@ -27,12 +27,15 @@
CONF = config.CONF
+# TODO(gmann): Remove this test class once the nova queens branch goes
+# into extended maintenance mode.
class FloatingIpsBulkRbacTest(rbac_base.BaseV2ComputeRbacTest):
# Tests will fail with a 404 starting from microversion 2.36:
# See the following link for details:
# https://developer.openstack.org/api-ref/compute/#floating-ips-bulk-os-floating-ips-bulk-deprecated
max_microversion = '2.35'
+ depends_on_nova_network = True
@classmethod
def skip_checks(cls):
diff --git a/patrole_tempest_plugin/tests/api/network/test_networks_rbac.py b/patrole_tempest_plugin/tests/api/network/test_networks_rbac.py
index 1a0e186..5506d90 100644
--- a/patrole_tempest_plugin/tests/api/network/test_networks_rbac.py
+++ b/patrole_tempest_plugin/tests/api/network/test_networks_rbac.py
@@ -19,6 +19,7 @@
from tempest import config
from tempest.lib.common.utils import data_utils
from tempest.lib import decorators
+from tempest.lib import exceptions as lib_exc
from patrole_tempest_plugin import rbac_exceptions
from patrole_tempest_plugin import rbac_rule_validation
@@ -66,6 +67,9 @@
shared_network=None,
router_external=None,
router_private=None,
+ provider_network_type=None,
+ provider_physical_network=None,
+ provider_segmentation_id=None,
segments=None,
**kwargs):
if not net_id:
@@ -73,13 +77,19 @@
if admin is not None:
kwargs['admin_state_up'] = admin
- elif shared_network is not None:
+ if shared_network is not None:
kwargs['shared'] = shared_network
- elif router_external is not None:
+ if router_external is not None:
kwargs['router:external'] = router_external
- elif router_private is not None:
+ if router_private is not None:
kwargs['router:private'] = router_private
- elif segments is not None:
+ if provider_network_type is not None:
+ kwargs['provider:network_type'] = provider_network_type
+ if provider_physical_network is not None:
+ kwargs['provider:physical_network'] = provider_physical_network
+ if provider_segmentation_id is not None:
+ kwargs['provider:segmentation_id'] = provider_segmentation_id
+ if segments is not None:
kwargs['segments'] = segments
updated_network = self.networks_client.update_network(
@@ -209,6 +219,81 @@
with self.rbac_utils.override_role(self):
self._update_network(net_id=network['id'], router_external=True)
+ @utils.requires_ext(extension='provider', service='network')
+ @rbac_rule_validation.action(
+ service="neutron",
+ rules=["get_network",
+ "update_network",
+ "update_network:provider:network_type"],
+ expected_error_codes=[404, 403, 403])
+ @decorators.idempotent_id('d064ef96-662b-47b6-94b7-9106dcd7ba8c')
+ def test_update_network_provider_network_type(self):
+
+ """Update Network Provider Network Type Test
+
+ RBAC test for neutron update_network:provider:network_type policy
+ """
+ try:
+ with self.rbac_utils.override_role(self):
+ self._update_network(self.network['id'],
+ provider_network_type='vxlan')
+ except lib_exc.BadRequest as exc:
+ # Per the api documentation, most plugins don't support updating
+ # provider attributes
+ self.assertIn(
+ "Plugin does not support updating provider attributes",
+ str(exc))
+
+ @utils.requires_ext(extension='provider', service='network')
+ @rbac_rule_validation.action(
+ service="neutron",
+ rules=["get_network",
+ "update_network",
+ "update_network:provider:physical_network"],
+ expected_error_codes=[404, 403, 403])
+ @decorators.idempotent_id('e3a55660-f75c-494e-a1b1-a8b36cc789ef')
+ def test_update_network_provider_physical_network(self):
+
+ """Update Network Provider Physical Network Test
+
+ RBAC test for neutron update_network:provider:physical_network policy
+ """
+ try:
+ with self.rbac_utils.override_role(self):
+ self._update_network(self.network['id'],
+ provider_physical_network='provider')
+ except lib_exc.BadRequest as exc:
+ # Per the api documenation, most plugins don't support updating
+ # provider attributes
+ self.assertIn(
+ "Plugin does not support updating provider attributes",
+ str(exc))
+
+ @utils.requires_ext(extension='provider', service='network')
+ @rbac_rule_validation.action(
+ service="neutron",
+ rules=["get_network",
+ "update_network",
+ "update_network:provider:segmentation_id"],
+ expected_error_codes=[404, 403, 403])
+ @decorators.idempotent_id('f6164228-b670-45fd-9ff9-b101930318c7')
+ def test_update_network_provider_segmentation_id(self):
+
+ """Update Network Provider Segmentation Id Test
+
+ RBAC test for neutron update_network:provider:segmentation_id policy
+ """
+ try:
+ with self.rbac_utils.override_role(self):
+ self._update_network(self.network['id'],
+ provider_segmentation_id=400)
+ except lib_exc.BadRequest as exc:
+ # Per the api documenation, most plugins don't support updating
+ # provider attributes
+ self.assertIn(
+ "Plugin does not support updating provider attributes",
+ str(exc))
+
@rbac_rule_validation.action(service="neutron",
rule="get_network",
expected_error_code=404)
diff --git a/patrole_tempest_plugin/tests/unit/test_policy_authority.py b/patrole_tempest_plugin/tests/unit/test_policy_authority.py
index 3e2cc4c..d396a29 100644
--- a/patrole_tempest_plugin/tests/unit/test_policy_authority.py
+++ b/patrole_tempest_plugin/tests/unit/test_policy_authority.py
@@ -238,7 +238,7 @@
test_user_id = mock.sentinel.user_id
service = 'invalid_service'
- self.assertRaises(rbac_exceptions.RbacInvalidService,
+ self.assertRaises(rbac_exceptions.RbacInvalidServiceException,
policy_authority.PolicyAuthority,
test_tenant_id,
test_user_id,
@@ -253,7 +253,7 @@
test_user_id = mock.sentinel.user_id
service = None
- self.assertRaises(rbac_exceptions.RbacInvalidService,
+ self.assertRaises(rbac_exceptions.RbacInvalidServiceException,
policy_authority.PolicyAuthority,
test_tenant_id,
test_user_id,
@@ -528,8 +528,9 @@
policy_parser = None
expected_exception = 'invalid_service is NOT a valid service'
- with self.assertRaisesRegex(rbac_exceptions.RbacInvalidService,
- expected_exception):
+ with self.assertRaisesRegex(
+ rbac_exceptions.RbacInvalidServiceException,
+ expected_exception):
policy_authority.PolicyAuthority(
test_tenant_id, test_user_id, "INVALID_SERVICE")
else:
diff --git a/patrole_tempest_plugin/tests/unit/test_rbac_rule_validation.py b/patrole_tempest_plugin/tests/unit/test_rbac_rule_validation.py
index 2e275dc..1bf5510 100644
--- a/patrole_tempest_plugin/tests/unit/test_rbac_rule_validation.py
+++ b/patrole_tempest_plugin/tests/unit/test_rbac_rule_validation.py
@@ -430,6 +430,22 @@
"Allowed")
+class RBACRuleValidationNegativeTest(BaseRBACRuleValidationTest):
+
+ @mock.patch.object(rbac_rv, 'policy_authority', autospec=True)
+ def test_rule_validation_invalid_service_raises_exc(self, mock_authority):
+ """Test that invalid service raises the appropriate exception."""
+ mock_authority.PolicyAuthority.return_value.allowed.side_effect = (
+ rbac_exceptions.RbacInvalidServiceException)
+
+ @rbac_rv.action(mock.sentinel.service, mock.sentinel.action)
+ def test_policy(*args):
+ pass
+
+ self.assertRaises(rbac_exceptions.RbacInvalidServiceException,
+ test_policy, self.mock_test_args)
+
+
class RBACRuleValidationTestMultiPolicy(BaseRBACRuleValidationTest):
"""Test suite for validating multi-policy support for the
``rbac_rule_validation`` decorator.
diff --git a/test-requirements.txt b/test-requirements.txt
index 35e4e57..9085c07 100644
--- a/test-requirements.txt
+++ b/test-requirements.txt
@@ -1,7 +1,7 @@
# The order of packages is significant, because pip processes them in the order
# of appearance. Changing the order has an impact on the overall integration
# process, which may cause wedges in the gate later.
-hacking>=1.0.0,<1.1.0 # Apache-2.0
+hacking>=1.1.0,<1.2.0 # Apache-2.0
fixtures>=3.0.0 # Apache-2.0/BSD
mock>=2.0.0 # BSD
coverage!=4.4,>=4.0 # Apache-2.0