Add missing delete permission for APIKeyApproval owner references#64
Conversation
Signed-off-by: Eguzki Astiz Lezaun <eastizle@redhat.com>
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR enables APIKeyApproval garbage collection by splitting RBAC rules for apikeyrequests and apiproducts, granting the apikeyapproval controller delete permissions, and validating the complete lifecycle with an end-to-end test that verifies cleanup when API keys are removed. ChangesAPIKeyApproval Garbage Collection and RBAC
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
test/e2e/apikey_approval_gc_test.go (2)
306-315: 💤 Low valueBrittle assumption:
apikeyapprovalname hardcoded as<request>-auto.
apiKeyApprovalNameis derived purely by string formatting ("%s-auto"). If the controller's naming convention for auto-approved requests is ever adjusted (suffix change, hashing, truncation for length limits, etc.), every subsequent assertion in this spec — owner ref name, kind, controller flag, and GC verification — silently fails for the wrong reason. Consider discovering the approval by querying with a label selector or by listingapikeyapprovalswhose owner-reference UID matches theAPIKeyRequest, rather than reconstructing the name.🤖 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/apikey_approval_gc_test.go` around lines 306 - 315, The test currently hardcodes apiKeyApprovalName via fmt.Sprintf("%s-auto") and then checks for that exact name in verifyApprovalCreated; change this to discover the created APIKeyApproval dynamically by listing apikeyapprovals in ownerNamespace and selecting the one that references the APIKeyRequest (use ownerReference UID or a stable label on APIKeyRequest) instead of reconstructing the name—replace the apiKeyApprovalName + verifyApprovalCreated logic with a lookup that runs kubectl get apikeyapproval -n ownerNamespace and filters by .metadata.ownerReferences[?(@.uid=="<APIKeyRequest UID>")] (or by label selector derived from apiKeyRequestName) to read the actual .metadata.name and assert it exists. Ensure to use the existing apiKeyRequestName and ownerNamespace variables and propagate the discovered name to subsequent assertions rather than assuming “-auto”.
50-59: ⚡ Quick winEarly returns in failure-diagnostics hook hide the most useful evidence.
When
kubectl get podsfails or returns no controller pod, the hookreturns before fetching namespace events. The case where the controller pod is missing is precisely when the events inownerNamespace/consumerNamespaceare most valuable for diagnosing the failure. Log the issue and fall through to the events-gathering blocks instead of returning.♻️ Fall through to event gathering
- podOutput, err := utils.Run(cmd) - if err != nil { - _, _ = fmt.Fprintf(GinkgoWriter, "Failed to get controller pod name: %s\n", err) - return - } - podNames := utils.GetNonEmptyLines(podOutput) - if len(podNames) == 0 { - _, _ = fmt.Fprintf(GinkgoWriter, "No controller pod found\n") - return - } - controllerPodName := podNames[0] - - By("Fetching controller manager pod logs") - cmd = exec.Command("kubectl", "logs", controllerPodName, "-n", controllerNamespace) - controllerLogs, err := utils.Run(cmd) - if err == nil { - _, _ = fmt.Fprintf(GinkgoWriter, "Controller logs:\n%s\n", controllerLogs) - } else { - _, _ = fmt.Fprintf(GinkgoWriter, "Failed to get Controller logs: %s\n", err) - } + podOutput, err := utils.Run(cmd) + var controllerPodName string + switch { + case err != nil: + _, _ = fmt.Fprintf(GinkgoWriter, "Failed to get controller pod name: %s\n", err) + default: + if podNames := utils.GetNonEmptyLines(podOutput); len(podNames) > 0 { + controllerPodName = podNames[0] + } else { + _, _ = fmt.Fprintf(GinkgoWriter, "No controller pod found\n") + } + } + + if controllerPodName != "" { + By("Fetching controller manager pod logs") + cmd = exec.Command("kubectl", "logs", controllerPodName, "-n", controllerNamespace) + controllerLogs, err := utils.Run(cmd) + if err == nil { + _, _ = fmt.Fprintf(GinkgoWriter, "Controller logs:\n%s\n", controllerLogs) + } else { + _, _ = fmt.Fprintf(GinkgoWriter, "Failed to get Controller logs: %s\n", err) + } + }🤖 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/apikey_approval_gc_test.go` around lines 50 - 59, The current failure-diagnostics early-returns hide important namespace events: when utils.Run(cmd) returns an error or podNames is empty in the block around podOutput/podNames, change the logic to log the error/message to GinkgoWriter (using the existing fmt.Fprintf calls) but do not return; instead let execution fall through to the subsequent ownerNamespace/consumerNamespace events-gathering blocks so those events are always collected even if utils.Run(cmd) failed or podNames is empty. Ensure you reference and preserve the existing variables (podOutput, err, podNames) and logging calls to avoid losing context while removing the early return behavior.
🤖 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 `@test/e2e/apikey_approval_gc_test.go`:
- Around line 137-138: SetDefaultEventuallyTimeout and
SetDefaultEventuallyPollingInterval are being called at tree-construction time
(top-level in the Describe) and thus mutate Gomega globals for the entire test
binary; move those calls into a per-spec setup (e.g., inside the Describe's
BeforeAll or BeforeEach) so the timeout/polling changes are scoped to this
suite, or alternatively remove the global calls and pass the timeout and
interval explicitly to each Eventually invocation (the test already uses
explicit timeouts in some Eventually calls) — update the code that currently
calls SetDefaultEventuallyTimeout and SetDefaultEventuallyPollingInterval to
instead run inside BeforeAll/BeforeEach or switch to per-Eventually
timeout/interval arguments.
---
Nitpick comments:
In `@test/e2e/apikey_approval_gc_test.go`:
- Around line 306-315: The test currently hardcodes apiKeyApprovalName via
fmt.Sprintf("%s-auto") and then checks for that exact name in
verifyApprovalCreated; change this to discover the created APIKeyApproval
dynamically by listing apikeyapprovals in ownerNamespace and selecting the one
that references the APIKeyRequest (use ownerReference UID or a stable label on
APIKeyRequest) instead of reconstructing the name—replace the apiKeyApprovalName
+ verifyApprovalCreated logic with a lookup that runs kubectl get apikeyapproval
-n ownerNamespace and filters by
.metadata.ownerReferences[?(@.uid=="<APIKeyRequest UID>")] (or by label selector
derived from apiKeyRequestName) to read the actual .metadata.name and assert it
exists. Ensure to use the existing apiKeyRequestName and ownerNamespace
variables and propagate the discovered name to subsequent assertions rather than
assuming “-auto”.
- Around line 50-59: The current failure-diagnostics early-returns hide
important namespace events: when utils.Run(cmd) returns an error or podNames is
empty in the block around podOutput/podNames, change the logic to log the
error/message to GinkgoWriter (using the existing fmt.Fprintf calls) but do not
return; instead let execution fall through to the subsequent
ownerNamespace/consumerNamespace events-gathering blocks so those events are
always collected even if utils.Run(cmd) failed or podNames is empty. Ensure you
reference and preserve the existing variables (podOutput, err, podNames) and
logging calls to avoid losing context while removing the early return behavior.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8703ef76-85c4-410f-bcb7-73cbcbfdd590
📒 Files selected for processing (3)
config/rbac/role.yamlinternal/controller/apikeyapproval_controller.gotest/e2e/apikey_approval_gc_test.go
Signed-off-by: Eguzki Astiz Lezaun <eastizle@redhat.com>
|
👀 |
2 participants
Summary
deletepermission to APIKeyApprovalReconciler RBAC for setting owner referencesProblem
The controller was failing to set owner references on APIKeyApproval resources with the error:
Kubernetes requires
deletepermission on a resource before you can set owner references that enable garbage collection.Solution
deleteverb to theapikeyapprovalsRBAC marker inapikeyapproval_controller.gomake manifestsapikey_approval_gc_test.go) that validates:Testing
The new e2e test verifies the complete cleanup chain:
Summary by CodeRabbit
Release Notes
Tests
Chores