Unified limits API shouldn't return a list of all limits

Bug #1754184 reported by Lance Bragstad
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
OpenStack Identity (keystone)
Fix Released
Medium
wangxiyuan

Bug Description

During the Rocky PTG, we reviewed the unified limit API as a group. One of the things that became apparent during the discussion was that the API shouldn't return a list of all limits when updating limits or creating new limits.

Originally, the API was designed this way so that an operator, or user, could double check their work after making a change. Where things get a bit complicated is if you attempt to delegate limit management to other users. For example, say a system administrator creates a new doamin for a customer and sets some limits on that domain. Let's also assume the customer has the ability to create projects within their domain and manage their limits with respect to the limits the system administrator set on the domain. If the customer makes a change to a limit within their domain, they will get a response that contains limit information for all projects, essentially leaking project information to someone who isn't authorized to see that information.

We should change the unified limit API to account for this by not returning a list of all limits on POST and PUT operations. This will be a backwards incompatible change, but we should be able to make it because the API is still marked as experimental.

Tags: limits
Changed in keystone:
status: New → Triaged
importance: Undecided → Medium
tags: added: limits
Revision history for this message
wangxiyuan (wangxiyuan) wrote :

now unified limits apis support batch create/update, the behavior is (registered limit and project limit are the same) :
POST one or more limits, return all the limits.
PUT one or more limits, return all the limits.

So we should change to:
Only return the limits which are created/updated in the request.
POST one or more limits, return them.
PUT one or more limits, return them.

According to the discussion during PTG, we still have some other concern, here is my opinion:
1. should the unified limits support batch create/update?
   yes. for admin operators, they usually want to create/update all the limits for one service or project in one request.
2. should the unified limits contain uuid?
   yes. The tuple (service_id, region_id, project_id, resource_name) can indicate a unique limits, it seems that the uuid can be removed. But actually "region_id" for a service can be none, in this case, the url /services/{service_id}/regions/{region_id} will not work. If we still want to remove uuid, we should find a way to solve the region problem.

BTW, Collen pointed out that we can add a new API: PATCH /limits/{limit_id} to update the specified limits is also a good idea.

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to keystone (master)

Fix proposed to branch: master
Review: https://review.openstack.org/550736

Changed in keystone:
assignee: nobody → wangxiyuan (wangxiyuan)
status: Triaged → In Progress
Revision history for this message
Chris Friesen (cbf123) wrote :

I like the idea of only returning information about the ones you actually created/modified.

What about the GET call? Will there be a way to optionally filter based on (potentially nested) project ID?

Revision history for this message
Lance Bragstad (lbragstad) wrote :

The current implementation supports filtering based on service_id, region_id (if applicable), and resource_name [0]. As for the discussion about batch processing for create and update, I've started a thread and asked the API SIG to weigh in on it [1].

[0] https://developer.openstack.org/api-ref/identity/v3/index.html#unified-limits
[1] http://lists.openstack.org/pipermail/openstack-dev/2018-March/128027.html

Revision history for this message
Lance Bragstad (lbragstad) wrote :

To recap - Chris Dent (from the API sig) provided some valuable input on the openstack-dev mailing list when we asked for feedback about this specific characteristic of the API [0].

[0] http://lists.openstack.org/pipermail/openstack-dev/2018-March/128127.html

Revision history for this message
OpenStack Infra (hudson-openstack) wrote :

Fix proposed to branch: master
Review: https://review.openstack.org/559552

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix merged to keystone (master)

Reviewed: https://review.openstack.org/550736
Committed: https://git.openstack.org/cgit/openstack/keystone/commit/?id=6ec45b127d780ea68dcb9a2c919a079c297292e3
Submitter: Zuul
Branch: master

commit 6ec45b127d780ea68dcb9a2c919a079c297292e3
Author: wangxiyuan <email address hidden>
Date: Thu Mar 8 15:41:50 2018 +0800

    Do not return all the limits for POST request.

    During Rocky PTG, We decided that the create apis for
    registered limit and project limit should not return all
    the limits.

    They should only return the ones in the request body.

    Change-Id: I26c37198861735bf85eb79570f192c71c7dd9941
    Partial-Bug: #1754184

Revision history for this message
OpenStack Infra (hudson-openstack) wrote :

Reviewed: https://review.openstack.org/559552
Committed: https://git.openstack.org/cgit/openstack/keystone/commit/?id=b385864c5d8c85c8911483b76c7787b33ebd84a3
Submitter: Zuul
Branch: master

commit b385864c5d8c85c8911483b76c7787b33ebd84a3
Author: wangxiyuan <email address hidden>
Date: Sun Apr 8 14:57:18 2018 +0800

    Unified limit update APIs Refactor

    According to the API-WG's suggestion, the update registered
    limit/project limit APIs should be refactored as:
    1. Change PUT to PATCH
    2. Remove batch update limits support for PATCH

    Closes-Bug: #1754184
    Change-Id: I1102166ab425a55d8eaf85c75d8fd3a7dfbaceb6

Changed in keystone:
status: In Progress → Fix Released
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix included in openstack/keystone 14.0.0.0rc1

This issue was fixed in the openstack/keystone 14.0.0.0rc1 release candidate.

Changed in keystone:
milestone: none → rocky-3
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.