feat(RHINENG-22377): Use Kessel's CheckBulkRequest for bulk requests #3297
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
Reviewer's GuideRefactors Kessel inventory permission checks to introduce a shared resource reference builder, use Kessel’s CheckBulk API for bulk read checks with stricter validation, and tighten semantics and typing for update checks and bulk calls. Sequence diagram for new Kessel CheckBulk read permission flowsequenceDiagram
participant Caller
participant KesselClient
participant InventoryService
Caller->>KesselClient: _check_bulk_resources(subject_ref, permission, resource_ids)
KesselClient->>KesselClient: validate resource_ids not empty
KesselClient->>KesselClient: build items with _build_object_reference(permission, resource_id) for each id
KesselClient->>InventoryService: CheckBulk(CheckBulkRequest(items), timeout)
InventoryService-->>KesselClient: CheckBulkResponse(pairs)
KesselClient->>KesselClient: compare len(response.pairs) to len(resource_ids)
alt missing_pairs
KesselClient->>KesselClient: log warning and return False
else all_pairs_present
loop each pair in response.pairs
alt pair has error
KesselClient->>KesselClient: log warning and return False
else not allowed
KesselClient->>KesselClient: return False
end
end
KesselClient-->>Caller: True
end
Class diagram for updated Kessel inventory permission checksclassDiagram
class KesselClient {
- inventory_svc
- timeout
+ _build_subject_reference(current_identity Identity) subject_reference_pb2.SubjectReference
+ _build_object_reference(permission KesselPermission, resource_id str) resource_reference_pb2.ResourceReference
+ _check_single_resource(subject_ref subject_reference_pb2.SubjectReference, permission KesselPermission, resource_id str) bool
+ _check_bulk_resources(subject_ref subject_reference_pb2.SubjectReference, permission KesselPermission, resource_ids list~str~) bool
+ _check_single_resource_for_update(subject_ref subject_reference_pb2.SubjectReference, permission KesselPermission, resource_id str) bool
+ _check_bulk_resources_for_update(subject_ref subject_reference_pb2.SubjectReference, permission KesselPermission, resource_ids list~str~) bool
}
class KesselPermission {
+ resource_type ResourceType
+ resource_permission str
}
class ResourceType {
+ name str
+ namespace str
}
class InventoryService {
+ Check(request check_request_pb2.CheckRequest, timeout float) CheckResponse
+ CheckBulk(request check_bulk_request_pb2.CheckBulkRequest, timeout float) CheckBulkResponse
+ CheckForUpdate(request check_request_pb2.CheckRequest, timeout float) CheckForUpdateResponse
}
KesselClient --> InventoryService : uses
KesselClient --> KesselPermission : uses
KesselPermission --> ResourceType : uses
KesselClient ..> subject_reference_pb2.SubjectReference : builds
KesselClient ..> resource_reference_pb2.ResourceReference : builds
KesselClient ..> check_request_pb2.CheckRequest : builds
KesselClient ..> check_bulk_request_pb2.CheckBulkRequest : builds
InventoryService ..> CheckResponse
InventoryService ..> CheckBulkResponse
InventoryService ..> CheckForUpdateResponse
Flow diagram for single and bulk update permission checksflowchart TD
A[Start bulk update check
_check_bulk_resources_for_update] --> B{resource_ids empty?}
B -- Yes --> C[Raise ValueError
resource_ids can't be empty]
B -- No --> D[Iterate each resource_id]
D --> E[_check_single_resource_for_update]
subgraph SingleUpdateCheck
E --> F{permission.resource_permission == view?}
F -- Yes --> G[Log error
Raise ValueError]
F -- No --> H[Build object_reference
via _build_object_reference]
H --> I[Build CheckRequest
with relation = permission.resource_permission]
I --> J[Call inventory_svc.CheckForUpdate]
J --> K{response.allowed == ALLOWED_TRUE?}
K -- No --> L[Return False to bulk]
K -- Yes --> M[Return True to bulk]
end
L --> N[Bulk returns False]
M --> O{all resource_ids processed and allowed?}
O -- Yes --> P[Bulk returns True]
O -- No --> N
N --> Q[End]
P --> Q[End]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
fstavela
force-pushed
the
fstavela/kessel-bulk-check
branch
from
3879767 to
ad0d76d
Compare
Contributor
Author
|
@sourcery-ai review |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- In
_check_bulk_resources, consider validating thatlen(response.pairs)matches the number of requestitemsand logging or failing if they differ, to guard against partial or malformed bulk responses. - The
update_permission = permission.resource_permission.replace("view", "edit")pattern is a bit brittle (e.g., if the string format changes or containsviewelsewhere); consider centralizing permission mapping in a helper or enum rather than relying on string replacement.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `_check_bulk_resources`, consider validating that `len(response.pairs)` matches the number of request `items` and logging or failing if they differ, to guard against partial or malformed bulk responses.
- The `update_permission = permission.resource_permission.replace("view", "edit")` pattern is a bit brittle (e.g., if the string format changes or contains `view` elsewhere); consider centralizing permission mapping in a helper or enum rather than relying on string replacement.
## Individual Comments
### Comment 1
<location> `lib/kessel.py:164-165` </location>
<code_context>
+ for resource_id in resource_ids
+ ]
+
+ bulk_request = check_bulk_request_pb2.CheckBulkRequest(items=items)
+ response: CheckBulkResponse = self.inventory_svc.CheckBulk(bulk_request, timeout=self.timeout)
+
+ # Check that all resources are allowed
</code_context>
<issue_to_address>
**issue:** Consider short‑circuiting for empty `resource_ids` instead of calling `CheckBulk` with no items.
The old version returned `True` for an empty `resource_ids` list via `all(...)` and made no RPCs. The new version will always send `CheckBulk(items=[])`, which may be wasted work or even fail if the service doesn’t treat empty batches as a no‑op. Adding a guard like `if not resource_ids: return True` before constructing `items` would preserve the previous behavior and avoid a potentially problematic call.
</issue_to_address>
### Comment 2
<location> `lib/kessel.py:202-205` </location>
<code_context>
- if not self._check_single_resource_for_update(subject_ref, permission, resource_id):
- return False
- return True
+ # For updates, we might need a different relation like "edit" or "write"
+ # This depends on how permissions are modeled in Kessel
+ update_permission = permission.resource_permission.replace("view", "edit")
+ return self._check_bulk_resources(subject_ref, permission, resource_ids, relation=update_permission)
def ListAllowedWorkspaces(self, current_identity: Identity, relation) -> list[str]:
</code_context>
<issue_to_address>
**🚨 question (security):** Clarify whether bulk update checks should use `CheckBulk` or the `CheckForUpdate` RPC for consistency with single‑resource checks.
Single‑resource updates use `CheckForUpdate`, but the bulk path now goes through `_check_bulk_resources`, which calls `CheckBulk` on the standard check service and only swaps the relation string. If `Check`/`CheckBulk` differ from `CheckForUpdate` in semantics or auditing, bulk updates may be authorized under different or weaker rules. Please either add a bulk variant that mirrors `CheckForUpdate` semantics, or explicitly confirm/document that `CheckBulk` is the correct mechanism for update checks as well.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
fstavela
force-pushed
the
fstavela/kessel-bulk-check
branch
4 times, most recently
from
a147d86 to
e03fba2
Compare
Contributor
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The new
ValueErroron emptyresource_idsin both bulk methods changes behavior from a no-op to an exception; consider whether callers expect empty lists and, if so, either treat them as trivially allowed or validate earlier at the boundary where the IDs are collected. - The update check logic relies on the literal permission name
'view'for validation; using an enum/constant or a more explicit notion of read vs write permissions would reduce brittleness if permission names change. - In the bulk check, you treat any length mismatch between
response.pairsandresource_idsas a blanket denial; if the API can partially succeed, consider correlating pairs to resource IDs and failing only the missing ones rather than all of them.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `ValueError` on empty `resource_ids` in both bulk methods changes behavior from a no-op to an exception; consider whether callers expect empty lists and, if so, either treat them as trivially allowed or validate earlier at the boundary where the IDs are collected.
- The update check logic relies on the literal permission name `'view'` for validation; using an enum/constant or a more explicit notion of read vs write permissions would reduce brittleness if permission names change.
- In the bulk check, you treat any length mismatch between `response.pairs` and `resource_ids` as a blanket denial; if the API can partially succeed, consider correlating pairs to resource IDs and failing only the missing ones rather than all of them.
## Individual Comments
### Comment 1
<location> `lib/kessel.py:176-179` </location>
<code_context>
+ # Check that all resources are allowed
+ for pair in response.pairs:
+ # If there's an error for this item, treat as denied
+ if pair.HasField("error"):
+ logger.warning(f"Kessel CheckBulk error for resource: {pair.error.message}")
+ return False
+ if pair.item.allowed != allowed_pb2.Allowed.ALLOWED_TRUE:
</code_context>
<issue_to_address>
**suggestion:** Include resource context in CheckBulk error logs to ease debugging.
Right now the warning only logs `pair.error.message`, which makes it hard to know which resource failed when many IDs are checked. Please include at least the resource ID (and ideally the relation) in the log message, e.g. `logger.warning("Kessel CheckBulk error for resource %s (%s): %s", pair.item.object.resource_id, pair.item.relation, pair.error.message)` to make production debugging easier.
```suggestion
# If there's an error for this item, treat as denied
if pair.HasField("error"):
logger.warning(
"Kessel CheckBulk error for resource %s (%s): %s",
pair.item.object.resource_id,
pair.item.relation,
pair.error.message,
)
return False
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
fstavela
force-pushed
the
fstavela/kessel-bulk-check
branch
4 times, most recently
from
3ead85d to
479616b
Compare
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
_check_bulk_resources, when logging item-level errors, consider including theresource_id(e.g., by correlatingpairback to the original request item) to make troubleshooting which resource failed more straightforward. - The new
ValueErroron emptyresource_idsin both bulk check methods changes the previous behavior; if that’s intentional, double-check that all callers validate input or handle this exception to avoid unexpected failures at runtime.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `_check_bulk_resources`, when logging item-level errors, consider including the `resource_id` (e.g., by correlating `pair` back to the original request item) to make troubleshooting which resource failed more straightforward.
- The new `ValueError` on empty `resource_ids` in both bulk check methods changes the previous behavior; if that’s intentional, double-check that all callers validate input or handle this exception to avoid unexpected failures at runtime.
## Individual Comments
### Comment 1
<location> `lib/kessel.py:179-180` </location>
<code_context>
+ for resource_id in resource_ids
+ ]
+
+ bulk_request = check_bulk_request_pb2.CheckBulkRequest(items=items)
+ response: CheckBulkResponse = self.inventory_svc.CheckBulk(bulk_request, timeout=self.timeout)
+
+ # Check that all resources are present in the response
</code_context>
<issue_to_address>
**question (bug_risk):** Consider clarifying/guaranteeing how response pairs map to requested resource_ids
The logic currently only checks `len(response.pairs) == len(resource_ids)` and then relies on iteration order, which assumes:
- The backend returns pairs in the same order as `items`, and
- There is at most one result per `resource_id` (even if there are duplicates).
If either assumption is false, pairs may be misaligned with the intended `resource_id`, leading to incorrect accept/reject decisions. If the protocol can echo back an identifier (e.g., `resource_id` or correlation id), consider mapping by that field instead of relying on count and order alone.
</issue_to_address>
### Comment 2
<location> `lib/kessel.py:191-196` </location>
<code_context>
+ return False
+
+ # Check that all resources are allowed
+ for pair in response.pairs:
+ # If there's an error for this item, treat as denied
+ if pair.HasField("error"):
+ logger.warning(f"Kessel CheckBulk error for resource: {pair.error.message}")
+ return False
+ if pair.item.allowed != allowed_pb2.Allowed.ALLOWED_TRUE:
+ return False
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Double-check that `allowed` belongs on `pair.item` rather than on `pair` or a separate result field
This code assumes `allowed` is on `pair.item` (`pair.item.allowed`). Many bulk APIs instead attach the decision at the pair level (e.g., `pair.allowed`, `pair.result.allowed`), with `item` echoed unchanged. If that’s true here, `pair.item.allowed` will always be unset and this check will misread the result. Please confirm the bulk response schema and ensure we’re reading `allowed` from the correct field, consistent with the non-bulk `CheckResponse.allowed` path.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
ezr-ondrej
approved these changes
Dec 22, 2025
Member
ezr-ondrej
left a comment
ezr-ondrej
left a comment
There was a problem hiding this comment.
I like the code 👍
I'm bit uneasy about the pipeline not testing it, but I'll trust your judgement, I belive you've tested this manually, right?
ezr-ondrej
force-pushed
the
fstavela/kessel-bulk-check
branch
from
479616b to
2634843
Compare
fstavela
force-pushed
the
fstavela/kessel-bulk-check
branch
2 times, most recently
from
a673775 to
c0f6db5
Compare
ezr-ondrej
force-pushed
the
fstavela/kessel-bulk-check
branch
from
c0f6db5 to
a360a5d
Compare
ezr-ondrej
force-pushed
the
fstavela/kessel-bulk-check
branch
from
a360a5d to
04623f3
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview
This PR is being created to address RHINENG-22377.
It implements the use of bulk access check against Kessel, which makes the API requests faster.
Unfortunately, when we enable Kessel Phase 1, the entire test run takes longer now, despite the API requests actually being faster. I don't think this is caused by an issue on the HBI side though. It looks like the bulk access check uses some caching, or something else that makes it take longer until the hosts sync gets propagated to these checks. Here is a detailed description of the issue: https://redhat-internal.slack.com/archives/C08GTNAFZ55/p1766401500404089?thread_ts=1763656513.607469&cid=C08GTNAFZ55
PR Checklist
Secure Coding Practices Documentation Reference
You can find documentation on this checklist here.
Secure Coding Checklist
Summary by Sourcery
Adopt Kessel bulk permission checks for inventory resources and tighten validation and error handling around permission lookups.
New Features:
Bug Fixes:
Enhancements:
Summary by Sourcery
Adopt Kessel bulk permission checks for inventory resources and tighten validation for permission lookups.
New Features:
Bug Fixes:
Enhancements: