Merge "Add RBAC tests for v3 auth policy actions"
diff --git a/HACKING.rst b/HACKING.rst
index bb337b0..b1d730d 100644
--- a/HACKING.rst
+++ b/HACKING.rst
@@ -1,6 +1,62 @@
Patrole Style Commandments
==========================
-Read the OpenStack Style Commandments: `<http://docs.openstack.org/developer/hacking/>`__
+- Step 1: Read the OpenStack Style Commandments: `<http://docs.openstack.org/developer/hacking/>`__
+- Step 2: Review Tempest's Style Commandments: `<https://docs.openstack.org/developer/tempest/HACKING.html>`__
+- Step 3: Read on
-Also review Tempest's Style Commandments: `<https://docs.openstack.org/developer/tempest/HACKING.html>`__
+Patrole Specific Commandments
+------------------------------
+
+Patrole borrows the following commandments from Tempest; refer to
+`Tempest's Commandments <https://docs.openstack.org/developer/tempest/HACKING.html>`__
+for more information:
+
+.. note::
+
+ 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
+- [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()
+- [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 (the check fails if the decorator is not one of the
+ two decorators directly above the function declaration)
+- [P101] RBAC test filenames must end with "_rbac.py"; for example,
+ test_servers_rbac.py, not test_servers.py
+- [P102] RBAC test class names must end in 'RbacTest'
+
+Role Switching
+--------------
+
+Correct role switching is vital to correct RBAC testing within Patrole. If a
+test does not call ``rbac_utils.switch_role`` with ``toggle_rbac_role=True``
+within the RBAC test, then the test is *not* a valid RBAC test: The API
+endpoint under test will be performed with admin credentials, which is always
+wrong unless ``CONF.rbac_test_role`` is admin.
+
+.. note::
+
+ Switching back to the admin role for setup and clean up is automatically
+ performed. Toggling ``switch_role`` with ``toggle_rbac_role=False`` within
+ the context of a test should *never* be performed and doing so will likely
+ result in an error being thrown.
+..
+
+Patrole does not have a hacking check for role switching, but does use a
+built-in mechanism for verifying that role switching is being correctly
+executed across tests. If a test does not call ``switch_role`` with
+``toggle_rbac_role=True``, then an ``RbacResourceSetupFailed`` exception
+will be raised.
diff --git a/patrole_tempest_plugin/hacking/__init__.py b/patrole_tempest_plugin/hacking/__init__.py
new file mode 100644
index 0000000..e69de29
--- /dev/null
+++ b/patrole_tempest_plugin/hacking/__init__.py
diff --git a/patrole_tempest_plugin/hacking/checks.py b/patrole_tempest_plugin/hacking/checks.py
new file mode 100644
index 0000000..e7e5cb0
--- /dev/null
+++ b/patrole_tempest_plugin/hacking/checks.py
@@ -0,0 +1,216 @@
+# Copyright 2013 IBM Corp.
+# Copyright 2017 AT&T Corporation.
+# All Rights Reserved.
+#
+# Licensed under the Apache License, Version 2.0 (the "License"); you may
+# not use this file except in compliance with the License. You may obtain
+# a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+# License for the specific language governing permissions and limitations
+# under the License.
+
+import os
+import re
+
+import pep8
+
+
+PYTHON_CLIENTS = ['cinder', 'glance', 'keystone', 'nova', 'swift', 'neutron',
+ 'ironic', 'heat', 'sahara']
+
+PYTHON_CLIENT_RE = re.compile('import (%s)client' % '|'.join(PYTHON_CLIENTS))
+TEST_DEFINITION = re.compile(r'^\s*def test.*')
+SETUP_TEARDOWN_CLASS_DEFINITION = re.compile(r'^\s+def (setUp|tearDown)Class')
+SCENARIO_DECORATOR = re.compile(r'\s*@.*services\((.*)\)')
+VI_HEADER_RE = re.compile(r"^#\s+vim?:.+")
+RAND_NAME_HYPHEN_RE = re.compile(r".*rand_name\(.+[\-\_][\"\']\)")
+MUTABLE_DEFAULT_ARGS = re.compile(r"^\s*def .+\((.+=\{\}|.+=\[\])")
+TESTTOOLS_SKIP_DECORATOR = re.compile(r'\s*@testtools\.skip\((.*)\)')
+TEST_METHOD = re.compile(r"^ def test_.+")
+CLASS = re.compile(r"^class .+")
+RBAC_CLASS_NAME_RE = re.compile(r'class .+RbacTest')
+RULE_VALIDATION_DECORATOR = re.compile(
+ r'\s*@.*rbac_rule_validation.action\((.*)\)')
+IDEMPOTENT_ID_DECORATOR = re.compile(r'\s*@decorators\.idempotent_id\((.*)\)')
+
+previous_decorator = None
+
+
+def import_no_clients_in_api_tests(physical_line, filename):
+ """Check for client imports from patrole_tempest_plugin/tests/api
+
+ T102: Cannot import OpenStack python clients
+ """
+ if "patrole_tempest_plugin/tests/api" in filename:
+ res = PYTHON_CLIENT_RE.match(physical_line)
+ if res:
+ return (physical_line.find(res.group(1)),
+ ("T102: python clients import not allowed "
+ "in patrole_tempest_plugin/tests/api/* or "
+ "patrole_tempest_plugin/tests/scenario/* tests"))
+
+
+def no_setup_teardown_class_for_tests(physical_line, filename):
+ """Check that tests do not use setUpClass/tearDownClass
+
+ T105: Tests cannot use setUpClass/tearDownClass
+ """
+ if pep8.noqa(physical_line):
+ return
+
+ if SETUP_TEARDOWN_CLASS_DEFINITION.match(physical_line):
+ return (physical_line.find('def'),
+ "T105: (setUp|tearDown)Class can not be used in tests")
+
+
+def no_vi_headers(physical_line, line_number, lines):
+ """Check for vi editor configuration in source files.
+
+ By default vi modelines can only appear in the first or
+ last 5 lines of a source file.
+
+ T106
+ """
+ # NOTE(gilliard): line_number is 1-indexed
+ if line_number <= 5 or line_number > len(lines) - 5:
+ if VI_HEADER_RE.match(physical_line):
+ return 0, "T106: Don't put vi configuration in source files"
+
+
+def service_tags_not_in_module_path(physical_line, filename):
+ """Check that a service tag isn't in the module path
+
+ A service tag should only be added if the service name isn't already in
+ the module path.
+
+ T107
+ """
+ matches = SCENARIO_DECORATOR.match(physical_line)
+ if matches:
+ services = matches.group(1).split(',')
+ for service in services:
+ service_name = service.strip().strip("'")
+ modulepath = os.path.split(filename)[0]
+ if service_name in modulepath:
+ return (physical_line.find(service_name),
+ "T107: service tag should not be in path")
+
+
+def no_hyphen_at_end_of_rand_name(logical_line, filename):
+ """Check no hyphen at the end of rand_name() argument
+
+ T108
+ """
+ msg = "T108: hyphen should not be specified at the end of rand_name()"
+ if RAND_NAME_HYPHEN_RE.match(logical_line):
+ return 0, msg
+
+
+def no_mutable_default_args(logical_line):
+ """Check that mutable object isn't used as default argument
+
+ N322: Method's default argument shouldn't be mutable
+ """
+ msg = "N322: Method's default argument shouldn't be mutable!"
+ if MUTABLE_DEFAULT_ARGS.match(logical_line):
+ yield (0, msg)
+
+
+def no_testtools_skip_decorator(logical_line):
+ """Check that methods do not have the testtools.skip decorator
+
+ T109
+ """
+ if TESTTOOLS_SKIP_DECORATOR.match(logical_line):
+ yield (0, "T109: Cannot use testtools.skip decorator; instead use "
+ "decorators.skip_because from tempest.lib")
+
+
+def use_rand_uuid_instead_of_uuid4(logical_line, filename):
+ """Check that tests use data_utils.rand_uuid() instead of uuid.uuid4()
+
+ T113
+ """
+ if 'uuid.uuid4()' not in logical_line:
+ return
+
+ msg = ("T113: Tests should use data_utils.rand_uuid()/rand_uuid_hex() "
+ "instead of uuid.uuid4()/uuid.uuid4().hex")
+ yield (0, msg)
+
+
+def no_rbac_rule_validation_decorator(physical_line, filename,
+ previous_logical):
+ """Check that each test has the ``rbac_rule_validation.action`` decorator.
+
+ Checks whether the test function has "@rbac_rule_validation.action"
+ above it; otherwise checks that it has "@decorators.idempotent_id" above
+ it and "@rbac_rule_validation.action" above that.
+
+ Assumes that ``rbac_rule_validation.action`` decorator is either the first
+ or second decorator above the test function; otherwise this check fails.
+
+ P100
+ """
+ global previous_decorator
+
+ if "patrole_tempest_plugin/tests/api" in filename:
+
+ if IDEMPOTENT_ID_DECORATOR.match(physical_line):
+ previous_decorator = previous_logical
+ return
+
+ if TEST_METHOD.match(physical_line):
+ if not RULE_VALIDATION_DECORATOR.match(previous_logical) and \
+ not RULE_VALIDATION_DECORATOR.match(previous_decorator):
+ return (0, "Must use rbac_rule_validation.action "
+ "decorator for API and scenario tests")
+
+
+def no_rbac_suffix_in_test_filename(physical_line, filename, previous_logical):
+ """Check that RBAC filenames end with "_rbac" suffix.
+
+ P101
+ """
+ if "patrole_tempest_plugin/tests/api" in filename:
+
+ if filename.endswith('rbac_base.py'):
+ return
+
+ if not filename.endswith('_rbac.py'):
+ return 0, "RBAC test filenames must end in _rbac suffix"
+
+
+def no_rbac_test_suffix_in_test_class_name(physical_line, filename,
+ previous_logical):
+ """Check that RBAC class names end with "RbacTest"
+
+ P102
+ """
+ if "patrole_tempest_plugin/tests/api" in filename:
+
+ if filename.endswith('rbac_base.py'):
+ return
+
+ if CLASS.match(physical_line):
+ if not RBAC_CLASS_NAME_RE.match(physical_line):
+ return 0, "RBAC test class names must end in 'RbacTest'"
+
+
+def factory(register):
+ register(import_no_clients_in_api_tests)
+ register(no_setup_teardown_class_for_tests)
+ register(no_vi_headers)
+ register(no_hyphen_at_end_of_rand_name)
+ register(no_mutable_default_args)
+ register(no_testtools_skip_decorator)
+ register(use_rand_uuid_instead_of_uuid4)
+ register(service_tags_not_in_module_path)
+ register(no_rbac_rule_validation_decorator)
+ register(no_rbac_suffix_in_test_filename)
+ register(no_rbac_test_suffix_in_test_class_name)
diff --git a/patrole_tempest_plugin/rbac_policy_parser.py b/patrole_tempest_plugin/rbac_policy_parser.py
index 8256889..d4e989b 100644
--- a/patrole_tempest_plugin/rbac_policy_parser.py
+++ b/patrole_tempest_plugin/rbac_policy_parser.py
@@ -40,8 +40,7 @@
each role, whether a given rule is allowed using oslo policy.
"""
- def __init__(self, project_id, user_id, service=None,
- extra_target_data={}):
+ def __init__(self, project_id, user_id, service, extra_target_data=None):
"""Initialization of Rbac Policy Parser.
Parses a policy file to create a dictionary, mapping policy actions to
@@ -64,6 +63,9 @@
:param path: type string
"""
+ if extra_target_data is None:
+ extra_target_data = {}
+
# First check if the service is valid
service = service.lower().strip() if service else None
self.admin_mgr = credentials.AdminManager()
diff --git a/patrole_tempest_plugin/rbac_rule_validation.py b/patrole_tempest_plugin/rbac_rule_validation.py
index 8de3d97..d77b2d6 100644
--- a/patrole_tempest_plugin/rbac_rule_validation.py
+++ b/patrole_tempest_plugin/rbac_rule_validation.py
@@ -31,7 +31,7 @@
def action(service, rule='', admin_only=False, expected_error_code=403,
- extra_target_data={}):
+ extra_target_data=None):
"""A decorator which does a policy check and matches it against test run.
A decorator which allows for positive and negative RBAC testing. Given
@@ -62,6 +62,10 @@
:raises Forbidden: for bullet (2) above.
:raises RbacOverPermission: for bullet (3) above.
"""
+
+ if extra_target_data is None:
+ extra_target_data = {}
+
def decorator(func):
role = CONF.rbac.rbac_test_role
diff --git a/patrole_tempest_plugin/tests/api/compute/test_volume_rbac.py b/patrole_tempest_plugin/tests/api/compute/test_volume_rbac.py
new file mode 100644
index 0000000..505ec18
--- /dev/null
+++ b/patrole_tempest_plugin/tests/api/compute/test_volume_rbac.py
@@ -0,0 +1,70 @@
+# Copyright 2017 AT&T Corporation.
+# All Rights Reserved.
+#
+# Licensed under the Apache License, Version 2.0 (the "License"); you may
+# not use this file except in compliance with the License. You may obtain
+# a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+# License for the specific language governing permissions and limitations
+# under the License.
+
+from tempest.lib import decorators
+
+from patrole_tempest_plugin import rbac_rule_validation
+from patrole_tempest_plugin.tests.api.compute import rbac_base
+
+from tempest import config
+
+CONF = config.CONF
+
+
+class VolumeV235RbacTest(rbac_base.BaseV2ComputeRbacTest):
+ """RBAC tests for the Nova Volume client."""
+
+ # These tests will fail with a 404 starting from microversion 2.36.
+ # For more information, see:
+ # https://developer.openstack.org/api-ref/compute/volume-extension-os-volumes-os-snapshots-deprecated
+ min_microversion = '2.10'
+ max_microversion = '2.35'
+
+ @decorators.idempotent_id('2402013e-a624-43e3-9518-44a5d1dbb32d')
+ @rbac_rule_validation.action(
+ service="nova",
+ rule="os_compute_api:os-volumes")
+ def test_create_volume(self):
+ self.rbac_utils.switch_role(self, toggle_rbac_role=True)
+ volume = self.volumes_extensions_client.create_volume(
+ size=CONF.volume.volume_size)['volume']
+ self.addCleanup(self.volumes_extensions_client.delete_volume,
+ volume['id'])
+
+ @decorators.idempotent_id('69b3888c-dff2-47b0-9fa4-0672619c9054')
+ @rbac_rule_validation.action(
+ service="nova",
+ rule="os_compute_api:os-volumes")
+ def test_list_volumes(self):
+ self.rbac_utils.switch_role(self, toggle_rbac_role=True)
+ self.volumes_extensions_client.list_volumes()
+
+ @decorators.idempotent_id('4ba0a820-040f-488b-86bb-be2e920ea12c')
+ @rbac_rule_validation.action(
+ service="nova",
+ rule="os_compute_api:os-volumes")
+ def test_show_volume(self):
+ volume = self.create_volume()
+ self.rbac_utils.switch_role(self, toggle_rbac_role=True)
+ self.volumes_extensions_client.show_volume(volume['id'])
+
+ @decorators.idempotent_id('6e7870f2-1bb2-4b58-96f8-6782071ef327')
+ @rbac_rule_validation.action(
+ service="nova",
+ rule="os_compute_api:os-volumes")
+ def test_delete_volume(self):
+ volume = self.create_volume()
+ self.rbac_utils.switch_role(self, toggle_rbac_role=True)
+ self.volumes_extensions_client.delete_volume(volume['id'])
diff --git a/patrole_tempest_plugin/tests/api/image/rbac_base.py b/patrole_tempest_plugin/tests/api/image/rbac_base.py
index 2a45ccb..a825c71 100644
--- a/patrole_tempest_plugin/tests/api/image/rbac_base.py
+++ b/patrole_tempest_plugin/tests/api/image/rbac_base.py
@@ -28,7 +28,7 @@
super(BaseV1ImageRbacTest, cls).skip_checks()
if not CONF.rbac.enable_rbac:
raise cls.skipException(
- "%s skipped as RBAC Flag not enabled" % cls.__name__)
+ "%s skipped as RBAC testing not enabled" % cls.__name__)
@classmethod
def setup_clients(cls):
@@ -47,7 +47,7 @@
super(BaseV2ImageRbacTest, cls).skip_checks()
if not CONF.rbac.enable_rbac:
raise cls.skipException(
- "%s skipped as RBAC Flag not enabled" % cls.__name__)
+ "%s skipped as RBAC testing not enabled" % cls.__name__)
@classmethod
def setup_clients(cls):
diff --git a/patrole_tempest_plugin/tests/api/network/rbac_base.py b/patrole_tempest_plugin/tests/api/network/rbac_base.py
index 629f0ca..d033174 100644
--- a/patrole_tempest_plugin/tests/api/network/rbac_base.py
+++ b/patrole_tempest_plugin/tests/api/network/rbac_base.py
@@ -30,7 +30,7 @@
super(BaseNetworkRbacTest, cls).skip_checks()
if not CONF.rbac.enable_rbac:
raise cls.skipException(
- "%s skipped as RBAC Flag not enabled" % cls.__name__)
+ "%s skipped as RBAC testing not enabled" % cls.__name__)
@classmethod
def setup_clients(cls):
diff --git a/patrole_tempest_plugin/tests/api/volume/test_volume_metadata_rbac.py b/patrole_tempest_plugin/tests/api/volume/test_volume_metadata_rbac.py
index d07e5c6..8ee8713 100644
--- a/patrole_tempest_plugin/tests/api/volume/test_volume_metadata_rbac.py
+++ b/patrole_tempest_plugin/tests/api/volume/test_volume_metadata_rbac.py
@@ -23,6 +23,7 @@
class VolumeMetadataRbacTest(rbac_base.BaseVolumeRbacTest):
+
@classmethod
def setup_clients(cls):
super(VolumeMetadataRbacTest, cls).setup_clients()
@@ -31,36 +32,38 @@
cls.image_client = cls.os_primary.image_client
elif CONF.image_feature_enabled.api_v2:
cls.image_client = cls.os_primary.image_client_v2
- cls.image_id = CONF.compute.image_ref
@classmethod
def resource_setup(cls):
super(VolumeMetadataRbacTest, cls).resource_setup()
cls.volume = cls.create_volume()
- cls._add_metadata(cls.volume)
cls.image_id = CONF.compute.image_ref
- @classmethod
- def _add_metadata(cls, volume):
+ def setUp(self):
+ super(VolumeMetadataRbacTest, self).setUp()
+ self._add_metadata(self.volume)
+
+ def tearDown(self):
+ self.volumes_client.update_volume_metadata(self.volume['id'], {})
+ super(VolumeMetadataRbacTest, self).tearDown()
+
+ def _add_metadata(self, volume):
# Create metadata for the volume
- metadata = {"key1": "value1",
- "key2": "value2",
- "key3": "value3",
- "key4": "<value&special_chars>"}
- cls.client.create_volume_metadata(cls.volume['id'],
- metadata)['metadata']
+ metadata = {"key1": "value1"}
+ self.client.create_volume_metadata(self.volume['id'], metadata)[
+ 'metadata']
@rbac_rule_validation.action(service="cinder",
- rule="volume:update_volume_metadata")
+ rule="volume:create_volume_metadata")
@decorators.idempotent_id('232bbb8b-4c29-44dc-9077-b1398c20b738')
def test_create_volume_metadata(self):
self.rbac_utils.switch_role(self, toggle_rbac_role=True)
self._add_metadata(self.volume)
@rbac_rule_validation.action(service="cinder",
- rule="volume:get")
+ rule="volume:get_volume_metadata")
@decorators.idempotent_id('87ea37d9-23ab-47b2-a59c-16fc4d2c6dfa')
- def test_get_volume_metadata(self):
+ def test_show_volume_metadata(self):
self.rbac_utils.switch_role(self, toggle_rbac_role=True)
self.client.show_volume_metadata(self.volume['id'])['metadata']
@@ -85,21 +88,18 @@
rule="volume:update_volume_metadata")
@decorators.idempotent_id('8ce2ff80-99ba-49ae-9bb1-7e96729ee5af')
def test_update_volume_metadata_item(self):
- # Metadata to update
- update_item = {"key3": "value3_update"}
+ updated_metadata_item = {"key1": "value1_update"}
self.rbac_utils.switch_role(self, toggle_rbac_role=True)
- self.client.update_volume_metadata_item(self.volume['id'], "key3",
- update_item)['meta']
+ self.client.update_volume_metadata_item(
+ self.volume['id'], "key1", updated_metadata_item)['meta']
@decorators.idempotent_id('a231b445-97a5-4657-b05f-245895e88da9')
@rbac_rule_validation.action(service="cinder",
rule="volume:update_volume_metadata")
def test_update_volume_metadata(self):
- # Metadata to update
- update = {"key1": "value1",
- "key3": "value3"}
+ updated_metadata = {"key1": "value1"}
self.rbac_utils.switch_role(self, toggle_rbac_role=True)
- self.client.update_volume_metadata(self.volume['id'], update)
+ self.client.update_volume_metadata(self.volume['id'], updated_metadata)
@decorators.idempotent_id('a9d9e825-5ea3-42e6-96f3-7ac4e97b2ed0')
@rbac_rule_validation.action(
@@ -107,9 +107,5 @@
rule="volume:update_volume_metadata")
def test_update_volume_image_metadata(self):
self.rbac_utils.switch_role(self, toggle_rbac_role=True)
- self.client.update_volume_image_metadata(self.volume['id'],
- image_id=self.image_id)
-
-
-class VolumeMetadataV3RbacTest(VolumeMetadataRbacTest):
- _api_version = 3
+ self.client.update_volume_image_metadata(
+ self.volume['id'], image_id=self.image_id)
diff --git a/releasenotes/notes/nova_volume_client-75e153a1c84e4ff8.yaml b/releasenotes/notes/nova_volume_client-75e153a1c84e4ff8.yaml
new file mode 100644
index 0000000..0562137
--- /dev/null
+++ b/releasenotes/notes/nova_volume_client-75e153a1c84e4ff8.yaml
@@ -0,0 +1,5 @@
+---
+features:
+ - |
+ Adds RBAC tests for the Nova os-volumes API which is deprecated
+ from microversion 2.36 onward.
diff --git a/requirements.txt b/requirements.txt
index ffc6abe..6d2c7f1 100644
--- a/requirements.txt
+++ b/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!=0.13.0,<0.14,>=0.12.0 # Apache-2.0
pbr>=1.8 # Apache-2.0
urllib3>=1.15.1 # MIT
oslo.log>=3.11.0 # Apache-2.0
diff --git a/tox.ini b/tox.ini
index a004c6e..be35509 100644
--- a/tox.ini
+++ b/tox.ini
@@ -59,3 +59,6 @@
ignore = E123,E125
builtins = _
exclude=.venv,.git,.tox,dist,doc,*lib/python*,*egg,build
+
+[hacking]
+local-check-factory = patrole_tempest_plugin.hacking.checks.factory