feat(RHINENG-24544): Add feature flag to force single-resource checks for Kessel bulk operations#3695
Open
thearifismail wants to merge 5 commits intomasterfrom
Open
feat(RHINENG-24544): Add feature flag to force single-resource checks for Kessel bulk operations#3695thearifismail wants to merge 5 commits intomasterfrom
thearifismail wants to merge 5 commits intomasterfrom
Conversation
Contributor
Reviewer's GuideIntroduces a feature flag-controlled path in Kessel bulk authorization checks that can force per-resource checks instead of using the gRPC bulk API, and adds comprehensive unit tests around bulk vs single-resource behavior and edge cases. Sequence diagram for Kessel bulk check with feature-flagged single-resource pathsequenceDiagram
actor Service
participant KesselAuthorizer
participant FeatureFlags
participant Logger
participant KesselSingleCheck
participant KesselBulkApi
Service->>KesselAuthorizer: _check_bulk_resources(subject_ref, permission, resource_ids)
KesselAuthorizer->>KesselAuthorizer: validate resource_ids not empty
KesselAuthorizer->>FeatureFlags: get_flag_value(FLAG_INVENTORY_KESSEL_FORCE_SINGLE_CHECKS_FOR_BULK)
FeatureFlags-->>KesselAuthorizer: flag_value
alt flag_value is true
KesselAuthorizer->>Logger: debug(Using single-resource checks...)
loop for each resource_id
KesselAuthorizer->>KesselSingleCheck: _check_single_resource(subject_ref, permission, resource_id)
KesselSingleCheck-->>KesselAuthorizer: is_authorized
KesselAuthorizer->>KesselAuthorizer: collect unauthorized_ids
end
KesselAuthorizer-->>Service: (len(unauthorized_ids) == 0, unauthorized_ids)
else flag_value is false
KesselAuthorizer->>KesselBulkApi: CheckBulkRequest(subject_ref, permission, resource_ids)
KesselBulkApi-->>KesselAuthorizer: (all_authorized, unauthorized_ids)
KesselAuthorizer-->>Service: (all_authorized, unauthorized_ids)
end
Class diagram for Kessel bulk vs single-resource check with feature flagclassDiagram
class KesselAuthorizer {
+bool _check_bulk_resources(subject_ref, permission, resource_ids)
+bool _check_single_resource(subject_ref, permission, resource_id)
}
class FeatureFlags {
+bool get_flag_value(flag_name)
<<constant>> FLAG_INVENTORY_KESSEL_FORCE_SINGLE_CHECKS_FOR_BULK
}
class Logger {
+debug(message, extra)
}
class KesselBulkApi {
+tuple check_bulk(subject_ref, permission, resource_ids)
}
KesselAuthorizer --> FeatureFlags : uses
KesselAuthorizer --> Logger : logs_to
KesselAuthorizer --> KesselBulkApi : invokes_for_bulk_checks
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
SC Environment Impact AssessmentOverall Impact: 🔴 CRITICAL View full reportSummary
Detailed Findings🔴 CRITICAL ImpactKessel integration change detected
Kessel integration change detected
🟢 LOW ImpactFeature flag change detected
Feature flag change detected
Required Actions
This assessment was automatically generated. Please review carefully and consult with the ROSA Core team for critical/high impact changes. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Several tests (e.g. in
TestCheckBulkResourcesFeatureFlag) patch_build_subject_referenceeven though_check_bulk_resourcesalready receives asubject_refargument and does not call_build_subject_reference, so those patches can be removed to simplify the test setup. - In the
kessel_clientfixture you patchClientBuilderbut never use it after bypassingKessel.__init__; you can drop that patch to reduce noise and make the intended construction behavior clearer.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Several tests (e.g. in `TestCheckBulkResourcesFeatureFlag`) patch `_build_subject_reference` even though `_check_bulk_resources` already receives a `subject_ref` argument and does not call `_build_subject_reference`, so those patches can be removed to simplify the test setup.
- In the `kessel_client` fixture you patch `ClientBuilder` but never use it after bypassing `Kessel.__init__`; you can drop that patch to reduce noise and make the intended construction behavior clearer.
## Individual Comments
### Comment 1
<location path="tests/test_lib_kessel.py" line_range="278-115" />
<code_context>
+ def test_check_with_multiple_ids_flag_enabled(self, kessel_client, test_identity, test_permission, mocker):
</code_context>
<issue_to_address>
**suggestion (testing):** Assert that the bulk API is not invoked when the feature flag forces single-resource checks
This test only verifies that `_check_single_resource` is used with the flag enabled, but doesn’t confirm that `inventory_svc.CheckBulk` is skipped. Please add an assertion like `assert kessel_client.inventory_svc.CheckBulk.call_count == 0` (or `assert_not_called()`) so the test guarantees the bulk path is not used when the feature flag is on.
Suggested implementation:
```python
def test_check_with_multiple_ids_flag_enabled(self, kessel_client, test_identity, test_permission, mocker):
"""
Test that check() with multiple IDs uses _check_bulk_resources which respects the feature flag.
JIRA: RHINENG-24544
"""
# Mock feature flag as enabled
mocker.patch("lib.kessel.get_flag_value", return_value=True)
# Mock _build_subject_reference
mock_subject_ref = Mock()
kessel_client._build_subject_reference = mocker.Mock(return_value=mock_subject_ref)
# Mock single-resource check implementation
kessel_client._check_single_resource = mocker.Mock(return_value=True)
# Ensure inventory_svc.CheckBulk is a mock so we can assert it is not used
kessel_client.inventory_svc.CheckBulk = mocker.Mock()
resource_ids = ["res-1", "res-2", "res-3"]
# Act: call check() with multiple IDs
result = kessel_client.check(
identity=test_identity,
resource_type="test-resource-type",
resource_ids=resource_ids,
permission=test_permission,
)
# Assert: single-resource path is used for each ID
assert kessel_client._check_single_resource.call_count == len(resource_ids)
# Assert: bulk API is NOT invoked when feature flag forces single-resource checks
kessel_client.inventory_svc.CheckBulk.assert_not_called()
# Optionally assert on result shape/value if check() has a defined aggregate behavior
# (e.g. boolean aggregate or per-resource dict); adapt as needed:
assert result
```
Depending on the existing `kessel_client` fixture and `check()` contract in your codebase, you may want/need to:
1. Adjust the `resource_type="test-resource-type"` argument to match whatever resource type your other tests use (e.g. a `test_resource_type` fixture or a specific string constant).
2. Refine the `result` assertion to match the actual return type of `check()` when given multiple `resource_ids` (it might be a boolean, a mapping of `resource_id -> bool`, etc.). Replace `assert result` with the appropriate expectation.
3. If `kessel_client.inventory_svc.CheckBulk` is already a mock in the fixture, you can drop the explicit assignment here and just keep the `assert_not_called()` assertion.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
rodrigonull
requested changes
Mar 4, 2026
tests/test_lib_kessel.py
Outdated
| mocker.patch("lib.kessel.get_flag_value", return_value=True) | ||
|
|
||
| # Mock _build_subject_reference | ||
| mocker.patch.object(kessel_client, "_build_subject_reference", return_value=mock_subject_ref) |
tests/test_lib_kessel.py
Outdated
| mocker.patch("lib.kessel.get_flag_value", return_value=True) | ||
|
|
||
| # Mock _build_subject_reference | ||
| mocker.patch.object(kessel_client, "_build_subject_reference", return_value=mock_subject_ref) |
tests/test_lib_kessel.py
Outdated
| mocker.patch("lib.kessel.get_flag_value", return_value=True) | ||
|
|
||
| # Mock _build_subject_reference | ||
| mocker.patch.object(kessel_client, "_build_subject_reference", return_value=mock_subject_ref) |
tests/test_lib_kessel.py
Outdated
| mocker.patch("lib.kessel.get_flag_value", return_value=True) | ||
|
|
||
| # Mock _build_subject_reference | ||
| mocker.patch.object(kessel_client, "_build_subject_reference", return_value=mock_subject_ref) |
tests/test_lib_kessel.py
Outdated
|
|
||
|
|
||
| @pytest.fixture | ||
| def mock_config(): |
Member
There was a problem hiding this comment.
This fixture is not used. Kessel init in the kessel_client fixture is patched to a no-op lambda that discards the config, so mock_config is never actually read. This can be removed and Kessel(mock_config) simplified to Kessel(None).
Contributor
Author
There was a problem hiding this comment.
Fixed and gone!
Contributor
Author
|
/retest |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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-24544.
Here is the 2nd PR. Since this is a temporary change, I put the test in a separate file
PR Checklist
Secure Coding Practices Documentation Reference
You can find documentation on this checklist here.
Secure Coding Checklist
Summary by Sourcery
Add a feature-flag-controlled path to force single-resource authorization checks for Kessel bulk operations and add comprehensive tests around bulk-check behavior and edge cases.
New Features:
Tests: