Enable projects and domains pagination

We have identified that keystone does not support pagination properly
like other services do. Use oslo_db.sqlalchemy helper to paginate
projects (and implicitly domains) list requests.

Introduce `max_db_limit` configuration option to allow operator to limit
maximum amount of entries to be fetched from the database (takes
precedence over user requested page size or dedicated default resource
limit).

Change-Id: I6d19a8d8827b7fb5c4b88f5b3b9dd899c5f3db5f

api-ref/source/v3/domains.inc

@@ -41,6 +41,8 @@ Parameters
 
    - name: domain_name_query
    - enabled: domain_enabled_query
+   - limit: limit_query
+   - marker: marker_query
 
 Response
 --------

api-ref/source/v3/parameters.yaml

@@ -261,6 +261,23 @@ is_domain_query:
   required: false
   type: boolean
   min_version: 3.6
+limit_query:
+  description: |
+    Requests a page size of items. Returns a number of items up to a limit
+    value. Use the limit parameter to make an initial limited request and use
+    the ID of the last-seen item from the response as the marker parameter
+    value in a subsequent limited request.
+  in: query
+  required: false
+  type: integer
+marker_query:
+  description: |
+    The ID of the last-seen item. Use the limit parameter to make an
+    initial limited request and use the ID of the last-seen item from the
+    response as the marker parameter value in a subsequent limited request
+  in: query
+  required: false
+  type: string
 name_user_query:
   description: |
     Filters the response by a user name.

api-ref/source/v3/projects.inc

@@ -63,6 +63,8 @@ Parameters
    - is_domain: is_domain_query
    - name: project_name_query
    - parent_id: parent_id_query
+   - limit: limit_query
+   - marker: marker_query
 
 Response
 --------

keystone/api/projects.py

@@ -175,7 +175,9 @@ class ProjectsResource(ks_flask.ResourceBase):
             filters=filters,
             target_attr=target,
         )
-        hints = self.build_driver_hints(filters)
+        hints = self.build_driver_hints(
+            filters, default_limit=PROVIDERS.resource_api._get_list_limit()
+        )
 
         # If 'is_domain' has not been included as a query, we default it to
         # False (which in query terms means '0')

keystone/common/driver_hints.py

@@ -12,12 +12,15 @@
 # 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 functools
+import typing as ty
 
+import keystone.conf
 from keystone import exception
 from keystone.i18n import _
 
+CONF = keystone.conf.CONF
+
 
 def truncated(f):
     """Ensure list truncation is detected in Driver list entity methods.
@@ -51,7 +54,7 @@ def truncated(f):
         ref_list = f(self, hints, *args, **kwargs)
 
         # If we got more than the original limit then trim back the list and
-        # mark it truncated.  In both cases, make sure we set the limit back
+        # mark it truncated. In both cases, make sure we set the limit back
         # to its original value.
         if len(ref_list) > list_limit:
             hints.set_limit(list_limit, truncated=True)
@@ -95,12 +98,17 @@ class Hints:
     """
 
     def __init__(self):
-        self.limit = None
+        self.limit: ty.Optional[dict[str, ty.Any]] = None
+        self.marker: ty.Optional[str] = None
         self.filters = []
         self.cannot_match = False
 
     def add_filter(
-        self, name, value, comparator='equals', case_sensitive=False
+        self,
+        name,
+        value,
+        comparator: str = 'equals',
+        case_sensitive: bool = False,
     ):
         """Add a filter to the filters list, which is publicly accessible."""
         self.filters.append(
@@ -118,6 +126,57 @@ class Hints:
             if entry['name'] == name and entry['comparator'] == 'equals':
                 return entry
 
-    def set_limit(self, limit, truncated=False):
+    def set_limit(self, limit: int, truncated: bool = False):
         """Set a limit to indicate the list should be truncated."""
         self.limit = {'limit': limit, 'truncated': truncated}
+
+    def get_limit_or_max(self) -> int:
+        """Get page limit or max page size
+
+        Return page limit (size) as requested by user (or API flow) or the
+        maximum page size if not present. This method is invoked by the SQL
+        drivers.
+
+        :returns int: Page size
+        """
+        limit: ty.Optional[int] = (
+            self.limit.get("limit") if self.limit else None
+        )
+        return int(limit) if limit else CONF.max_db_limit
+
+    def get_limit_with_default(
+        self, default_limit: ty.Optional[int] = None
+    ) -> int:
+        """Return page limit for the query.
+
+        1. `limit` was set in the query parameters:
+            `min(limit, MAX_LIMIT)`
+
+        2. `limit` is not set and `default_limit` is set:
+            `min(default_limit, MAX_LIMIT)`
+
+        2. `limit` is null, `default_limit` is null, `CONF.list_limit` is set:
+            `min(CONF.list_limit, MAX_LIMIT)`
+
+        3. `limit` is null, `default_limit` is null, `CONF.list_limit` is null:
+            `CONF.max_db_limit`
+
+        """
+
+        _user_limit: ty.Optional[int] = (
+            self.limit.get("limit") if self.limit else None
+        )
+        _absolute_limit: int = CONF.max_db_limit
+        # when default (as resource specific) is not set try to get the global
+        # default
+        _default_limit: ty.Optional[int] = default_limit or CONF.list_limit
+        if _user_limit:
+            return min(_user_limit, _absolute_limit)
+        elif _default_limit:
+            return min(_default_limit, _absolute_limit)
+        else:
+            return CONF.max_db_limit
+
+    def set_marker(self, marker: str):
+        """Set a marker pointing to the last entry of the page."""
+        self.marker = marker

keystone/common/manager.py

@@ -20,6 +20,7 @@ import types
 from oslo_log import log
 import stevedore
 
+from keystone.common.driver_hints import Hints
 from keystone.common import provider_api
 from keystone.i18n import _
 
@@ -56,9 +57,14 @@ def response_truncated(f):
         if kwargs.get('hints') is None:
             return f(self, *args, **kwargs)
 
-        list_limit = self.driver._get_list_limit()
-        if list_limit:
-            kwargs['hints'].set_limit(list_limit)
+        # Set default limit if not there already
+        if (
+            isinstance(kwargs['hints'], Hints)
+            and kwargs['hints'].limit is None
+        ):
+            list_limit = self.driver._get_list_limit()
+            if list_limit:
+                kwargs['hints'].set_limit(list_limit)
         return f(self, *args, **kwargs)
 
     return wrapper

keystone/common/sql/core.py

@@ -493,6 +493,40 @@ def _limit(query, hints):
     return query
 
 
+def filter_query(model, query, hints):
+    """Apply filtering to a query.
+
+    :param model: table model
+    :param query: query to apply filters to
+    :param hints: contains the list of filters and limit details.  This may
+                  be None, indicating that there are no filters or limits
+                  to be applied. If it's not None, then any filters
+                  satisfied here will be removed so that the caller will
+                  know if any filters remain.
+
+    :returns: query updated with any filters and limits satisfied
+
+    """
+    if hints is None:
+        return query
+
+    # First try and satisfy any filters
+    query = _filter(model, query, hints)
+
+    if hints.cannot_match:
+        # Nothing's going to match, so don't bother with the query.
+        return []
+
+    # NOTE(henry-nash): Any unsatisfied filters will have been left in
+    # the hints list for the controller to handle. We can only try and
+    # limit here if all the filters are already satisfied since, if not,
+    # doing so might mess up the final results. If there are still
+    # unsatisfied filters, we have to leave any limiting to the controller
+    # as well.
+
+    return query
+
+
 def filter_limit_query(model, query, hints):
     """Apply filtering and limit to a query.
 

keystone/conf/default.py

@@ -97,6 +97,20 @@ projects from placing an unnecessary load on the system.
     ),
 )
 
+max_db_limit = cfg.IntOpt(
+    'max_db_limit',
+    default=1000,
+    min=0,
+    help=utils.fmt(
+        """
+As a query can potentially return many thousands of items, you can limit the
+maximum number of items in a single response by setting this option.
+While `list_limit` is used to set the default page size this parameter sets
+global maximum that cannot be exceded.
+"""
+    ),
+)
+
 strict_password_check = cfg.BoolOpt(
     'strict_password_check',
     default=False,
@@ -177,6 +191,7 @@ ALL_OPTS = [
     max_param_size,
     max_token_size,
     list_limit,
+    max_db_limit,
     strict_password_check,
     insecure_debug,
     default_publisher_id,

keystone/exception.py

@@ -443,6 +443,10 @@ class NotFound(Error):
     title = http.client.responses[http.client.NOT_FOUND]
 
 
+class MarkerNotFound(NotFound):
+    message_format = _("Marker %(marker)s could not be found.")
+
+
 class EndpointNotFound(NotFound):
     message_format = _("Could not find endpoint: %(endpoint_id)s.")
 

keystone/resource/backends/sql.py

@@ -10,17 +10,19 @@
 # License for the specific language governing permissions and limitations
 # under the License.
 
+from oslo_db.sqlalchemy import utils as sqlalchemyutils
 from oslo_log import log
 from sqlalchemy import orm
 from sqlalchemy.sql import expression
 
-from keystone.common import driver_hints
 from keystone.common import resource_options
 from keystone.common import sql
+import keystone.conf
 from keystone import exception
 from keystone.resource.backends import base
 from keystone.resource.backends import sql_model
 
+CONF = keystone.conf.CONF
 LOG = log.getLogger(__name__)
 
 
@@ -63,7 +65,6 @@ class Resource(base.ResourceDriverBase):
                 raise exception.ProjectNotFound(project_id=project_name)
             return project_ref.to_dict()
 
-    @driver_hints.truncated
     def list_projects(self, hints):
         # If there is a filter on domain_id and the value is None, then to
         # ensure that the sql filtering works correctly, we need to patch
@@ -77,10 +78,38 @@ class Resource(base.ResourceDriverBase):
         with sql.session_for_read() as session:
             query = session.query(sql_model.Project)
             query = query.filter(sql_model.Project.id != base.NULL_DOMAIN_ID)
-            project_refs = sql.filter_limit_query(
-                sql_model.Project, query, hints
+            query = sql.filter_query(sql_model.Project, query, hints)
+            marker_row = None
+            if hints.marker is not None:
+                marker_row = (
+                    session.query(sql_model.Project)
+                    .filter_by(id=hints.marker)
+                    .first()
+                )
+                if not marker_row:
+                    raise exception.MarkerNotFound(marker=hints.marker)
+
+            project_refs = sqlalchemyutils.paginate_query(
+                query,
+                sql_model.Project,
+                hints.get_limit_or_max(),
+                ["id"],
+                marker=marker_row,
             )
-            return [project_ref.to_dict() for project_ref in project_refs]
+            # Query the data
+            data = project_refs.all()
+            if hints.limit:
+                # the `common.manager.response_truncated` decorator expects
+                # that when driver truncates results it should also raise
+                # 'truncated' flag to indicate that. Since we do not really
+                # know whether there are more records once we applied filters
+                # we can only "assume" and set the flag when count of records
+                # is equal to what we have limited to.
+                # NOTE(gtema) get rid of that once proper pagination is
+                # enabled for all resources
+                if len(data) >= hints.limit["limit"]:
+                    hints.limit["truncated"] = True
+            return [project_ref.to_dict() for project_ref in data]
 
     def list_projects_from_ids(self, ids):
         if not ids:

keystone/resource/schema.py

@@ -115,6 +115,11 @@ project_index_request_query = {
         "tags-any": parameter_types.tags,
         "not-tags": parameter_types.tags,
         "not-tags-any": parameter_types.tags,
+        "marker": {
+            "type": "string",
+            "description": "ID of the last fetched entry",
+        },
+        "limit": {"type": ["integer", "string"]},
     },
 }
 

keystone/server/flask/common.py

@@ -733,6 +733,23 @@ class ResourceBase(flask_restful.Resource):
         container = {collection: refs}
         self_url = full_url(flask.request.environ['PATH_INFO'])
         container['links'] = {'next': None, 'self': self_url, 'previous': None}
+        # When pagination is enabled generate link to the next page
+        if (
+            hints
+            and (hints.limit or hints.marker)
+            and len(refs) > 0
+            # We do not know whether there are entries on the next page,
+            # let's just emit link to the next page if current page is full.
+            and list_limited
+        ):
+            args = flask.request.args.to_dict()
+            # Update the marker with the ID of last entry
+            args["marker"] = refs[-1]["id"]
+            args["limit"] = hints.limit.get("limit", args.get("limit"))
+            container["links"]["next"] = base_url(
+                flask.url_for(flask.request.endpoint, **args)
+            )
+
         if list_limited:
             container['truncated'] = True
 
@@ -878,11 +895,14 @@ class ResourceBase(flask_restful.Resource):
         return flask.request.get_json(silent=True, force=True) or {}
 
     @staticmethod
-    def build_driver_hints(supported_filters):
+    def build_driver_hints(
+        supported_filters, default_limit: ty.Optional[int] = None
+    ):
         """Build list hints based on the context query string.
 
         :param supported_filters: list of filters supported, so ignore any
                                   keys in query_dict that are not in this list.
+        :param default_limit: default page size (PROVIDER._get_list_limit)
 
         """
         hints = driver_hints.Hints()
@@ -929,9 +949,20 @@ class ResourceBase(flask_restful.Resource):
                     case_sensitive=case_sensitive,
                 )
 
-        # NOTE(henry-nash): If we were to support pagination, we would pull any
-        # pagination directives out of the query_dict here, and add them into
-        # the hints list.
+        marker = flask.request.args.get("marker")
+        if marker:
+            hints.set_marker(marker)
+        limit = flask.request.args.get("limit", default_limit)
+        # Set page limit as whatever requested in query parameters
+        # or whatever provider uses as a limit. In any way do not exceed the
+        # maximum page size limit.
+        # NOTE(gtema): It is important to set limit here (in the API flow)
+        # otherwise limited pagination is being applied also for internal
+        # invocations what is most likely not desired.
+        if limit:
+            # NOTE(gtema) remember limit is still conditional
+            hints.set_limit(min(int(limit), CONF.max_db_limit), False)
+
         return hints
 
     @classmethod

keystone/tests/unit/test_v3.py

@@ -12,8 +12,10 @@
 # License for the specific language governing permissions and limitations
 # under the License.
 
+import abc
 import datetime
 import http.client
+import typing as ty
 import uuid
 
 import oslo_context.context
@@ -1658,3 +1660,189 @@ class AssignmentTestMixin:
         )
 
         return entity
+
+
+class PaginationTestCaseBase(RestfulTestCase):
+    """Base test for the resource pagination."""
+
+    resource_name: ty.Optional[str] = None
+
+    def setUp(self):
+        super().setUp()
+        if not self.resource_name:
+            self.skipTest("Not testing the base")
+
+    @abc.abstractmethod
+    def _create_resources(self, count: int):
+        """Method creating given count of test resources
+
+        This is an abstract method and must be implemented by every test
+        class inheriting from this base class.
+        """
+        pass
+
+    def test_list_requested_limit(self):
+        """Test requesting certain page size"""
+        count_resources: int = 6
+        self._create_resources(count_resources)
+
+        for expected_length in range(1, count_resources):
+            response = self.get(
+                f"/{self.resource_name}s?limit={expected_length}"
+            )
+            res_list = response.json_body[f"{self.resource_name}s"]
+            res_links = response.json_body["links"]
+            self.assertEqual(
+                expected_length,
+                len(res_list),
+                "Requested count of entries returned",
+            )
+            self.assertEqual(
+                f"http://localhost/v3/{self.resource_name}s?limit={expected_length}&marker={res_list[-1]['id']}",
+                res_links.get("next"),
+                "Next page link contains limit and marker",
+            )
+
+        # Try fetching with explicitly very high page size
+        response = self.get(
+            f"/{self.resource_name}s?limit={count_resources+100}"
+        )
+        res_list = response.json_body[f"{self.resource_name}s"]
+        res_links = response.json_body["links"]
+        self.assertGreaterEqual(
+            len(res_list), count_resources, "All test resources are returned"
+        )
+        self.assertIsNone(res_links.get("next"), "Next page link is empty")
+
+        response = self.get(f"/{self.resource_name}s?limit=1&sort_key=name")
+        res_list = response.json_body[f"{self.resource_name}s"]
+        res_links = response.json_body["links"]
+        self.assertEqual(
+            f"http://localhost/v3/{self.resource_name}s?limit=1&sort_key=name&marker={res_list[-1]['id']}",
+            res_links.get("next"),
+            "Next page link contains limit and marker as well as other passed keys",
+        )
+
+        self.config_fixture.config(list_limit=2)
+        response = self.get(f"/{self.resource_name}s?limit=10")
+        res_list = response.json_body[f"{self.resource_name}s"]
+        self.assertGreaterEqual(
+            len(response.json_body[f"{self.resource_name}s"]),
+            count_resources,
+            "Requested limit higher then default wins",
+        )
+
+    def test_list_requested_limit_too_high(self):
+        """Test passing limit exceeding the system max"""
+        self.config_fixture.config(max_db_limit=4)
+
+        count_resources: int = 6
+        self._create_resources(count_resources)
+
+        response = self.get(f"/{self.resource_name}s?limit={count_resources}")
+        res_list = response.json_body[f"{self.resource_name}s"]
+        res_links = response.json_body['links']
+        self.assertEqual(
+            4,
+            len(res_list),
+            "max_db_limit overrides requested count of entries",
+        )
+        self.assertEqual(
+            f"http://localhost/v3/{self.resource_name}s?limit=4&marker={res_list[-1]['id']}",
+            res_links.get("next"),
+            "Next page link contains corrected limit and marker",
+        )
+
+    def test_list_default_limit(self):
+        """Tests listing resources without limit set"""
+        count_resources: int = 6
+        self._create_resources(count_resources)
+
+        self.config_fixture.config(list_limit=2)
+
+        response = self.get(f'/{self.resource_name}s')
+        res_list = response.json_body[f"{self.resource_name}s"]
+        res_links = response.json_body['links']
+        self.assertEqual(2, len(res_list), "default limit is applied")
+        self.assertEqual(
+            f"http://localhost/v3/{self.resource_name}s?marker={res_list[-1]['id']}&limit=2",
+            res_links.get("next"),
+            "Next page link contains corrected limit and marker",
+        )
+
+        self.config_fixture.config(group="resource", list_limit=3)
+
+        response = self.get(f'/{self.resource_name}s')
+        res_list = response.json_body[f"{self.resource_name}s"]
+        res_links = response.json_body['links']
+        self.assertEqual(3, len(res_list), "default resource limit is applied")
+        self.assertEqual(
+            f"http://localhost/v3/{self.resource_name}s?marker={res_list[-1]['id']}&limit=3",
+            res_links.get("next"),
+            "Next page link contains corrected limit and marker",
+        )
+
+    def _consume_paginated_list(
+        self, limit: ty.Optional[int] = None
+    ) -> tuple[list[dict], int]:
+        """Fetch all paginated resources"""
+        found_resources: list = []
+        pages: int = 0
+        if limit:
+            response = self.get(f"/{self.resource_name}s?limit={limit}")
+        else:
+            response = self.get(f"/{self.resource_name}s")
+            pages = 1
+        res_links = response.json_body['links']
+        while "next" in res_links:
+            # Put fetched resources into the found list
+            found_resources.extend(
+                response.json_body.get(f"{self.resource_name}s")
+            )
+
+            # Get the relative url of the next page (since testhelper only supports that)
+            next_url = res_links.get("next", "")
+            if next_url:
+                next_rel_url = next_url[next_url.find("/v3") + 3 :]
+                # fetch next page
+                response = self.get(next_rel_url)
+                res_links = response.json_body['links']
+                pages += 1
+            else:
+                break
+        return (found_resources, pages)
+
+    def test_list_consume_all_resources(self):
+        """Test paginating through all resources (simulate user SDK)"""
+        count_resources: int = 50
+        page_size: int = 5
+        self._create_resources(count_resources)
+        response = self.get(f"/{self.resource_name}s")
+        current_count = len(response.json_body[f"{self.resource_name}s"])
+
+        # Set pagination default at 5
+        self.config_fixture.config(group="resource", list_limit=page_size)
+
+        (found_resources, pages) = self._consume_paginated_list()
+        self.assertGreaterEqual(
+            len(found_resources),
+            count_resources,
+            "Fetched at least all pre-created resources",
+        )
+        self.assertGreaterEqual(
+            pages,
+            current_count // page_size,
+            "At least as many pages consumed as expected",
+        )
+
+        (found_resources, pages) = self._consume_paginated_list(limit=100)
+        self.assertGreaterEqual(
+            len(found_resources),
+            count_resources,
+            "Fetched at least all pre-created resources",
+        )
+        self.assertGreaterEqual(
+            pages,
+            current_count // 100,
+            "At least as many pages consumed as expected",
+        )

keystone/tests/unit/test_v3_resource.py

@@ -1011,7 +1011,8 @@ class ResourceTestCase(test_v3.RestfulTestCase, test_v3.AssignmentTestMixin):
         expected_list = [projects[1]['project']]
 
         # projects[0] has projects[1] as child
-        self.assertEqual(expected_list, projects_result)
+        for project in expected_list:
+            self.assertIn(project, projects_result)
 
         # Query for projects[1] immediate children - it will
         # be projects[2] and projects[3]
@@ -1026,7 +1027,8 @@ class ResourceTestCase(test_v3.RestfulTestCase, test_v3.AssignmentTestMixin):
         expected_list = [projects[2]['project'], projects[3]['project']]
 
         # projects[1] has projects[2] and projects[3] as children
-        self.assertEqual(expected_list, projects_result)
+        for project in expected_list:
+            self.assertIn(project, projects_result)
 
         # Query for projects[2] immediate children - it will be an empty list
         r = self.get(
@@ -1040,7 +1042,8 @@ class ResourceTestCase(test_v3.RestfulTestCase, test_v3.AssignmentTestMixin):
         expected_list = []
 
         # projects[2] has no child, projects_result must be an empty list
-        self.assertEqual(expected_list, projects_result)
+        for project in expected_list:
+            self.assertIn(project, projects_result)
 
     def test_create_hierarchical_project(self):
         """Call ``POST /projects``."""
@@ -2045,3 +2048,25 @@ class StrictTwoLevelLimitsResourceTestCase(ResourceTestCase):
             body={'project': new_ref},
             expected_status=http.client.FORBIDDEN,
         )
+
+
+class DomainPaginationTestCase(test_v3.PaginationTestCaseBase):
+    """Test domain list pagination."""
+
+    resource_name: str = "domain"
+
+    def _create_resources(self, count: int):
+        for x in range(count):
+            res = {"domain": unit.new_domain_ref()}
+            response = self.post("/domains", body=res)
+
+
+class ProjectPaginationTestCase(test_v3.PaginationTestCaseBase):
+    """Test project list pagination."""
+
+    resource_name: str = "project"
+
+    def _create_resources(self, count: int):
+        for x in range(count):
+            res = {"project": unit.new_project_ref()}
+            response = self.post("/projects", body=res)

releasenotes/notes/pagination-projects-ea311579da4bb83b.yaml

@@ -0,0 +1,12 @@
+---
+features:
+  - |
+    New configuration variable `max_db_limit` is added to set an absolute limit
+    amount of entries fetched from the database at a single time. It is used in
+    resource pagination. Existing option `list_limit` is optional and describes
+    preferred count of entries while `max_db_limit` sets top limit applied to
+    user input and individual `list_limit` options.
+  - |
+    Project and domain listing supports pagination. Query parameters `limit`
+    and `marker` are added and work as described in `API-SIG doc
+    <https://specs.openstack.org/openstack/api-sig/guidelines/pagination_filter_sort.html>`_