test(maas): add ephemeral API key cleanup tests#1324
Merged
dbasunag merged 3 commits intoopendatahub-io:mainfrom Mar 31, 2026
Merged
test(maas): add ephemeral API key cleanup tests#1324dbasunag merged 3 commits intoopendatahub-io:mainfrom
dbasunag merged 3 commits intoopendatahub-io:mainfrom
Conversation
|
The following are automatically added/executed:
Available user actions:
Supported labels{'/lgtm', '/verified', '/cherry-pick', '/wip', '/hold', '/build-push-pr-image'} |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds pytest fixtures and tests for ephemeral MaaS API key lifecycle: pod discovery, ephemeral API key creation/revocation, CronJob and NetworkPolicy assertions, and an integration test that triggers the cleanup endpoint and verifies ephemeral key visibility and persistence. Note: port‑forwarding to an internal cleanup endpoint raises an access‑control concern (CWE‑200). Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/model_serving/maas_billing/maas_subscription/conftest.py`:
- Around line 831-836: The teardown currently calls revoke_api_key(...) and
discards its result, which can hide failures and leak API keys; update the
ephemeral_api_key teardown to check the revoke_api_key result (and/or catch
exceptions) and fail the test or log and retry on non-successful responses: call
revoke_api_key(request_session_http=request_session_http, base_url=base_url,
key_id=create_body["id"], ocp_user_token=ocp_token_for_actor), verify the
response status or returned error value, raise an exception or assert when
revocation fails, and optionally add a small retry/backoff before raising to
handle transient failures so revoke_api_key failures are not silently ignored.
- Around line 801-809: The fixture currently ignores HTTP errors from
revoke_api_key and may pick a non-ready pod; update the teardown to capture the
response from revoke_api_key and assert/raise on non-2xx status (e.g., check
response.status_code and log/raise if not OK) so revocation failures fail the
test, and change the Pod.get selection logic (where Pod.get is called with
client=admin_client, namespace=applications_namespace,
label_selector=label_selector) to filter returned pods by readiness/Running
conditions (inspect each pod's status/conditions to require Ready=True and
phase==Running) and return a deterministically ready pod (or fail the fixture if
none match) instead of returning pods[0].
In
`@tests/model_serving/maas_billing/maas_subscription/test_api_key_ephemeral_cleanup.py`:
- Around line 80-89: The test currently only asserts policyTypes and ingress but
never validates egress rules; update the assertions around spec (the spec
variable in this test) to verify spec.egress is present and strictly restricts
destinations to the maas-api pods (e.g., an egress list with a single rule whose
to[*].podSelector.matchLabels contains app=maas-api) and does not include any
to[*].ipBlock or 0.0.0.0/0 entries; add assertions that spec.egress is a
non-empty list, that each egress rule's to entries exist, that none of those to
entries contain ipBlock or cidr ranges, and that the podSelector labels match
app=maas-api to ensure only maas-api egress is allowed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 0b044733-caa2-405c-9d25-f163e76a3f1e
📒 Files selected for processing (3)
tests/model_serving/maas_billing/maas_subscription/conftest.pytests/model_serving/maas_billing/maas_subscription/test_api_key_ephemeral_cleanup.pytests/model_serving/maas_billing/maas_subscription/utils.py
tests/model_serving/maas_billing/maas_subscription/test_api_key_ephemeral_cleanup.py
Show resolved
Hide resolved
SB159
force-pushed
the
feat/maas-ephemeral-key-cleanup-tests
branch
from
7468681 to
f088102
Compare
SB159
force-pushed
the
feat/maas-ephemeral-key-cleanup-tests
branch
from
be31100 to
b12eb1f
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/model_serving/maas_billing/maas_subscription/test_api_key_ephemeral_cleanup.py (1)
89-92:⚠️ Potential issue | 🟠 MajorEgress rule content validation incomplete (CWE-284)
Docstring claims "restricts cleanup pod egress to maas-api only" but test only verifies egress rules exist (line 90), not that they restrict to
maas-apipods. A NetworkPolicy withegress: [{}](allow-all) would pass this test.egress_rules = getattr(spec, "egress", None) assert egress_rules, "NetworkPolicy should define at least one egress rule" + egress_peers = [ + peer for rule in egress_rules for peer in (getattr(rule, "to", None) or []) + ] + assert any( + getattr(getattr(peer, "podSelector", None), "matchLabels", {}).get("app") == "maas-api" + for peer in egress_peers + ), "Egress must target maas-api pods"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/model_serving/maas_billing/maas_subscription/test_api_key_ephemeral_cleanup.py` around lines 89 - 92, The test currently only checks that egress_rules exists but not that it restricts traffic to maas-api; update the assertion to validate content by inspecting spec.egress (egress_rules) and asserting at least one egress rule contains a "to" entry with a podSelector (or namespaceSelector+podSelector) that matches the maas-api selector (e.g., podSelector.matchLabels {'app': 'maas-api'} or the exact label used by the maas-api Deployment) and that there are no overly permissive "to: {}" wildcards that would allow all egress; modify the check around the egress_rules variable (and adjust LOGGER message if desired) so the test fails unless an explicit maas-api-targeting egress rule exists.
🧹 Nitpick comments (1)
tests/model_serving/maas_billing/maas_subscription/test_api_key_ephemeral_cleanup.py (1)
173-184: Add error handling for port-forward and JSON parsing failuresIf
portforward.forward()fails to establish connection withinwaiting=20, or if the cleanup endpoint returns non-JSON (e.g., 502 gateway error), the test will raise an uncaught exception with unclear error messaging.Suggested improvement
with portforward.forward( pod_or_service=maas_api_pod_name, namespace=applications_namespace, from_port=8080, to_port=8080, waiting=20, ): cleanup_response = requests.post( url="http://localhost:8080/internal/v1/api-keys/cleanup", timeout=30, ) assert cleanup_response.status_code == 200, ( f"Cleanup endpoint returned unexpected status: {cleanup_response.status_code}: " f"{(cleanup_response.text or '')[:200]}" ) - cleanup_resp = cleanup_response.json() + try: + cleanup_resp = cleanup_response.json() + except requests.exceptions.JSONDecodeError as e: + pytest.fail(f"Cleanup response not valid JSON: {cleanup_response.text[:200]}, error: {e}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/model_serving/maas_billing/maas_subscription/test_api_key_ephemeral_cleanup.py` around lines 173 - 184, Wrap the portforward.forward() call and the HTTP call/JSON parsing around the cleanup endpoint in try/except blocks so failures give clear test failures instead of uncaught exceptions: catch exceptions from portforward.forward() (the call that uses waiting=20) and raise an assertion or pytest.fail with a message that includes the original exception; likewise catch requests.exceptions.RequestException around the requests.post to "http://localhost:8080/internal/v1/api-keys/cleanup" and fail with the status code and response text if available, and catch ValueError/JSONDecodeError when calling cleanup_response.json() to fail with the raw response.text (or status) so non-JSON responses (e.g., 502) produce clear diagnostics. Ensure you reference the existing variables cleanup_response and cleanup_resp in the error messages so the test output points to the failing endpoint and payload.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@tests/model_serving/maas_billing/maas_subscription/test_api_key_ephemeral_cleanup.py`:
- Around line 89-92: The test currently only checks that egress_rules exists but
not that it restricts traffic to maas-api; update the assertion to validate
content by inspecting spec.egress (egress_rules) and asserting at least one
egress rule contains a "to" entry with a podSelector (or
namespaceSelector+podSelector) that matches the maas-api selector (e.g.,
podSelector.matchLabels {'app': 'maas-api'} or the exact label used by the
maas-api Deployment) and that there are no overly permissive "to: {}" wildcards
that would allow all egress; modify the check around the egress_rules variable
(and adjust LOGGER message if desired) so the test fails unless an explicit
maas-api-targeting egress rule exists.
---
Nitpick comments:
In
`@tests/model_serving/maas_billing/maas_subscription/test_api_key_ephemeral_cleanup.py`:
- Around line 173-184: Wrap the portforward.forward() call and the HTTP
call/JSON parsing around the cleanup endpoint in try/except blocks so failures
give clear test failures instead of uncaught exceptions: catch exceptions from
portforward.forward() (the call that uses waiting=20) and raise an assertion or
pytest.fail with a message that includes the original exception; likewise catch
requests.exceptions.RequestException around the requests.post to
"http://localhost:8080/internal/v1/api-keys/cleanup" and fail with the status
code and response text if available, and catch ValueError/JSONDecodeError when
calling cleanup_response.json() to fail with the raw response.text (or status)
so non-JSON responses (e.g., 502) produce clear diagnostics. Ensure you
reference the existing variables cleanup_response and cleanup_resp in the error
messages so the test output points to the failing endpoint and payload.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 04475368-813c-4687-ac76-7280b6e96d46
📒 Files selected for processing (3)
tests/model_serving/maas_billing/maas_subscription/conftest.pytests/model_serving/maas_billing/maas_subscription/test_api_key_ephemeral_cleanup.pytests/model_serving/maas_billing/maas_subscription/utils.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/model_serving/maas_billing/maas_subscription/utils.py
SB159
force-pushed
the
feat/maas-ephemeral-key-cleanup-tests
branch
2 times, most recently
from
bad324f to
9d24737
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/model_serving/maas_billing/maas_subscription/utils.py (1)
191-219:⚠️ Potential issue | 🟠 MajorEnforce expiration when
ephemeral=True(CWE-613, major).Exploit scenario: a caller sets
ephemeral=Truewithoutexpires_in; if backend accepts it, this creates a hidden credential that may persist indefinitely and evade cleanup expectations.Remediation diff
def create_api_key( @@ if subscription is not None: payload["subscription"] = subscription + if ephemeral and not expires_in: + raise AssertionError("ephemeral API keys must set expires_in") if ephemeral: payload["ephemeral"] = TrueAs per coding guidelines, "REVIEW PRIORITIES: 1. Security vulnerabilities (provide severity, exploit scenario, and remediation code)".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/model_serving/maas_billing/maas_subscription/utils.py` around lines 191 - 219, The function builds an API key payload but allows ephemeral=True without an expiration, which can create persistent hidden credentials; update the function that accepts expires_in and ephemeral (referencing the expires_in and ephemeral parameters and the payload dict) to enforce that when ephemeral is True expires_in must be provided (e.g., raise ValueError or AssertionError early), and ensure you do not send an ephemeral key without payload["expiresIn"]; include a clear error message mentioning ephemeral and expires_in so callers and tests fail fast.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/model_serving/maas_billing/maas_subscription/utils.py`:
- Around line 492-494: The loop over sub["model_refs"] assumes each ref is a
mapping; add an explicit type guard before key assertions by asserting
isinstance(ref, dict) with a clear message (e.g., "model_ref must be a dict:
{ref}"), then proceed to assert "name" in ref and isinstance(ref["name"], str);
update the checks around the for loop in the same scope where sub and model_refs
are validated (the for ref in sub["model_refs"] block) so malformed API
responses fail with a clear, descriptive assertion instead of a TypeError.
- Around line 484-503: The assertions currently interpolate the entire sub
payload (f"{sub}") which can leak sensitive tenant data; update each assertion
in the validation block to avoid dumping the full subscription object by
removing f"{sub}" from messages and instead use a generic, non-sensitive message
(e.g., "Missing subscription_id_header" or "subscription_id_header must be a
string") or include only a non-sensitive identifier (like the key name). Locate
the checks referencing sub and ref (the assert lines for
"subscription_id_header", "subscription_description", "priority", "model_refs",
model_ref "name", and optional keys "display_name", "organization_id",
"cost_center", "labels") and replace their f-string payloads with these generic
messages so no full payload or tenant identifiers are logged on assertion
failure.
---
Outside diff comments:
In `@tests/model_serving/maas_billing/maas_subscription/utils.py`:
- Around line 191-219: The function builds an API key payload but allows
ephemeral=True without an expiration, which can create persistent hidden
credentials; update the function that accepts expires_in and ephemeral
(referencing the expires_in and ephemeral parameters and the payload dict) to
enforce that when ephemeral is True expires_in must be provided (e.g., raise
ValueError or AssertionError early), and ensure you do not send an ephemeral key
without payload["expiresIn"]; include a clear error message mentioning ephemeral
and expires_in so callers and tests fail fast.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 74defc68-a416-4b62-af3b-67fbbff25836
📒 Files selected for processing (3)
tests/model_serving/maas_billing/maas_subscription/conftest.pytests/model_serving/maas_billing/maas_subscription/test_api_key_ephemeral_cleanup.pytests/model_serving/maas_billing/maas_subscription/utils.py
✅ Files skipped from review due to trivial changes (1)
- tests/model_serving/maas_billing/maas_subscription/test_api_key_ephemeral_cleanup.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/model_serving/maas_billing/maas_subscription/conftest.py
Signed-off-by: Swati Mukund Bagal <sbagal@redhat.com>
SB159
force-pushed
the
feat/maas-ephemeral-key-cleanup-tests
branch
from
9d24737 to
57c44ee
Compare
dbasunag
reviewed
Mar 31, 2026
dbasunag
reviewed
Mar 31, 2026
tests/model_serving/maas_billing/maas_subscription/test_api_key_ephemeral_cleanup.py
Outdated
Show resolved
Hide resolved
tests/model_serving/maas_billing/maas_subscription/test_api_key_ephemeral_cleanup.py
Outdated
Show resolved
Hide resolved
tests/model_serving/maas_billing/maas_subscription/test_api_key_ephemeral_cleanup.py
Outdated
Show resolved
Hide resolved
tests/model_serving/maas_billing/maas_subscription/test_api_key_ephemeral_cleanup.py
Show resolved
Hide resolved
tests/model_serving/maas_billing/maas_subscription/test_api_key_ephemeral_cleanup.py
Outdated
Show resolved
Hide resolved
Signed-off-by: Swati Mukund Bagal <sbagal@redhat.com>
SB159
force-pushed
the
feat/maas-ephemeral-key-cleanup-tests
branch
from
828bab8 to
312e7d8
Compare
dbasunag
approved these changes
Mar 31, 2026
sheltoncyril
approved these changes
Mar 31, 2026
Contributor
|
/lgtm |
rhods-ci-bot
approved these changes
Mar 31, 2026
|
Status of building tag latest: success. |
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
4 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.
Pull Request
Summary
add ephemeral API key cleanup tests
Related Issues
Please review and indicate how it has been tested
Additional Requirements
Summary by CodeRabbit