fix: fixing unit test for cleanup Job#952
Conversation
jland-redhat
requested review from
ryancham715 and
yu-teo
and removed request for
a team
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jland-redhat The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (8)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (2)
📝 WalkthroughWalkthroughThis PR migrates ephemeral API key cleanup from a Kubernetes CronJob to an in-process background loop inside maas-api, removes the cleanup-image parameter and runtime CronJob image patching, prunes legacy CronJob/NetworkPolicy resources during platform reconcile, narrows cronjob RBAC to get/delete, updates deployment params/scripts and CI trigger, and revises docs and e2e tests to validate CLEANUP_INTERVAL_MINUTES on the maas-api Deployment. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/content/configuration-and-management/api-key-administration.md (1)
84-89:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAlign the manual cleanup example with the deployed endpoint.
This command now documents
http://localhost:8080, but the updated e2e path for the same flow useshttps://localhost:8443/internal/v1/api-keys/cleanup. One of these is wrong, and the current docs will give operators a failing troubleshooting step unless the scheme/port are reconciled.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/content/configuration-and-management/api-key-administration.md` around lines 84 - 89, The manual cleanup example uses http://localhost:8080 but the deployed e2e flow uses https://localhost:8443/internal/v1/api-keys/cleanup; update the documented command (the oc exec deploy/maas-api invocation that calls the /internal/v1/api-keys/cleanup endpoint) to use the correct scheme and port (https://localhost:8443) and ensure any curl flags required for HTTPS are present so the example matches the deployed endpoint.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@maas-api/cmd/main.go`:
- Around line 249-252: The comment describing the cleanup call-path is
incorrect: the in-process cleanup loop in serve() deletes expired ephemeral keys
directly from the store, so the HTTP endpoint mounted in internalRoutes
(router.Group("/internal/v1") with CleanupExpiredEphemeralKeys) is only a
manual/internal trigger rather than the production cleanup path; update the
inline comment above internalRoutes (and any related docs) to state that serve()
performs automatic in-process cleanup and that the POST
/internal/v1/api-keys/cleanup endpoint is an internal/manual trigger for ad-hoc
cleanup, referencing internalRoutes and the CleanupExpiredEphemeralKeys handler
and serve() to locate the code to change.
In `@test/e2e/tests/test_api_keys.py`:
- Around line 906-935: The test_cleanup_interval_configured currently only
asserts CLEANUP_INTERVAL_MINUTES; re-add a check that the cleanup endpoint
(/internal/v1/api-keys/cleanup) is protected by cluster network boundaries by
asserting the existence and correct selectors of the NetworkPolicy (or
equivalent) that isolates maas-api from arbitrary in-cluster callers: after
loading the Deployment and maas_api_container, query the cluster for the
NetworkPolicy resources in the same namespace and assert one matches a
name/selector that targets the maas-api pod (match labels from
deploy["spec"]["template"]["metadata"]["labels"]) and restricts ingress to only
allowed pods/namespace(s) (no 0.0.0.0/0 or allow-all rules); keep the
CLEANUP_INTERVAL_MINUTES assertions intact and fail the test if the network
isolation policy is missing or permissive so the internal-only trust boundary
for /internal/v1/api-keys/cleanup is enforced.
- Around line 924-928: The test currently falls back to containers[0] when no
container named "maas-api" is found, which can mask missing/renamed containers;
change the logic so maas_api_container is None when not found (use next(...)
without a containers[0] default), then immediately assert or raise a test
failure with a clear message (e.g., "maas-api container not found in pod
containers") before computing env_vars; update the env_vars computation to run
only after that assertion using maas_api_container.get("env", []).
---
Outside diff comments:
In `@docs/content/configuration-and-management/api-key-administration.md`:
- Around line 84-89: The manual cleanup example uses http://localhost:8080 but
the deployed e2e flow uses https://localhost:8443/internal/v1/api-keys/cleanup;
update the documented command (the oc exec deploy/maas-api invocation that calls
the /internal/v1/api-keys/cleanup endpoint) to use the correct scheme and port
(https://localhost:8443) and ensure any curl flags required for HTTPS are
present so the example matches the deployed endpoint.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: c84847df-3d4f-40fc-9c2d-4f40d59c2327
📒 Files selected for processing (14)
.github/workflows/maas-controller-ci.ymldeployment/base/maas-controller/default/params.envdocs/content/configuration-and-management/api-key-administration.mddocs/content/reference/maas-api-overview.mddocs/content/user-guide/api-key-management.mdmaas-api/cmd/main.gomaas-api/internal/api_keys/handler.gomaas-api/internal/api_keys/service.gomaas-controller/pkg/platform/tenantreconcile/constants.gomaas-controller/pkg/platform/tenantreconcile/params.gomaas-controller/pkg/platform/tenantreconcile/params_test.goscripts/deploy.shtest/e2e/scripts/local-deploy.shtest/e2e/tests/test_api_keys.py
💤 Files with no reviewable changes (6)
- .github/workflows/maas-controller-ci.yml
- test/e2e/scripts/local-deploy.sh
- scripts/deploy.sh
- maas-controller/pkg/platform/tenantreconcile/params.go
- maas-controller/pkg/platform/tenantreconcile/constants.go
- deployment/base/maas-controller/default/params.env
| // Internal routes (no auth required - called by Authorino / in-process cleanup) | ||
| internalRoutes := router.Group("/internal/v1") | ||
| internalRoutes.POST("/api-keys/validate", apiKeyHandler.ValidateAPIKeyHandler) | ||
| internalRoutes.POST("/api-keys/cleanup", apiKeyHandler.CleanupExpiredEphemeralKeys) // TODO: consider remove endpoint if not public access |
There was a problem hiding this comment.
Fix the cleanup call-path description.
Line 249 is backwards: the in-process cleanup loop does not call /internal/v1 at all; it deletes directly from the store in serve() (Line 135). Leaving this wording in place will mislead follow-up docs/tests into treating the HTTP endpoint as the production cleanup path when it is only the manual/internal trigger.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@maas-api/cmd/main.go` around lines 249 - 252, The comment describing the
cleanup call-path is incorrect: the in-process cleanup loop in serve() deletes
expired ephemeral keys directly from the store, so the HTTP endpoint mounted in
internalRoutes (router.Group("/internal/v1") with CleanupExpiredEphemeralKeys)
is only a manual/internal trigger rather than the production cleanup path;
update the inline comment above internalRoutes (and any related docs) to state
that serve() performs automatic in-process cleanup and that the POST
/internal/v1/api-keys/cleanup endpoint is an internal/manual trigger for ad-hoc
cleanup, referencing internalRoutes and the CleanupExpiredEphemeralKeys handler
and serve() to locate the code to change.
| maas_api_container = next( | ||
| (c for c in containers if c.get("name") == "maas-api"), | ||
| containers[0] if containers else {}, | ||
| ) | ||
| if result.returncode != 0: | ||
| pytest.skip( | ||
| f"NetworkPolicy maas-api-cleanup-restrict not found in " | ||
| f"{deployment_namespace}: {result.stderr.strip()}" | ||
| ) | ||
|
|
||
| import json as _json | ||
| np = _json.loads(result.stdout) | ||
| spec = np["spec"] | ||
|
|
||
| # Verify it targets cleanup pods | ||
| selector = spec.get("podSelector", {}).get("matchLabels", {}) | ||
| assert selector.get("app") == "maas-api-cleanup", \ | ||
| f"NetworkPolicy should target app=maas-api-cleanup, got: {selector}" | ||
|
|
||
| # Verify policy types include both Egress and Ingress | ||
| policy_types = spec.get("policyTypes", []) | ||
| assert "Egress" in policy_types, "NetworkPolicy should control egress" | ||
| assert "Ingress" in policy_types, "NetworkPolicy should control ingress" | ||
| env_vars = {e["name"]: e.get("value") for e in maas_api_container.get("env", []) if "name" in e} |
There was a problem hiding this comment.
Fail if the maas-api container is missing instead of probing containers[0].
The fallback to containers[0] can make this test pass against an unrelated sidecar if the maas-api container is renamed or absent. That weakens the regression signal for exactly the deployment contract this test is supposed to enforce.
Suggested fix
- maas_api_container = next(
- (c for c in containers if c.get("name") == "maas-api"),
- containers[0] if containers else {},
- )
+ maas_api_container = next(
+ (c for c in containers if c.get("name") == "maas-api"),
+ None,
+ )
+ assert maas_api_container is not None, "maas-api container not found in Deployment"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/e2e/tests/test_api_keys.py` around lines 924 - 928, The test currently
falls back to containers[0] when no container named "maas-api" is found, which
can mask missing/renamed containers; change the logic so maas_api_container is
None when not found (use next(...) without a containers[0] default), then
immediately assert or raise a test failure with a clear message (e.g., "maas-api
container not found in pod containers") before computing env_vars; update the
env_vars computation to run only after that assertion using
maas_api_container.get("env", []).
There was a problem hiding this comment.
@jland-redhat I think this is a good point. If the container is renamed or missing, containers[0] silently picks up whatever container happens to be first (e.g., an init container or sidecar), and the test would either pass incorrectly or fail with a confusing assertion about CLEANUP_INTERVAL_MINUTES not being found. Would it be better and more informative if we rather say "maas-api container not found.", would you agree?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
…onJob Remove stale CronJob patching, deploy params, and RBAC from opendatahub-io#934 fallout. Add PruneLegacyCleanupResources on tenant reconcile, run maas-controller CI on every PR, and update docs/E2E for CLEANUP_INTERVAL_MINUTES.
|
@jland-redhat: The following test has Succeeded: OCI Artifact Browser URLInspecting Test Artifacts ManuallyTo inspect your test artifacts manually, follow these steps:
mkdir -p oras-artifacts
cd oras-artifacts
oras pull quay.io/opendatahub/odh-ci-artifacts:maas-group-test-q45p9 |
3 participants
A few cleanup items were missed in 934 inside of the controller, so I have update those as well as some minor documentation misses.
Also updated the
maas-controllerworkflow to run everytime since it is relatively quick to run the unit test (and such as in this case) changes that are external to the controller codebase can effect the controller.Summary by CodeRabbit
Documentation
Tests
Chores