feat: add APIKey expiration support and test - #88
Conversation
|
Warning Review limit reached
Next review available in: 47 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (11)
📝 WalkthroughWalkthroughAPIKey and APIKeyRequest resources gain optional ChangesAPIKey expiry lifecycle
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant APIKey
participant SecretController
participant EnforcementSecret
participant StatusController
APIKey->>SecretController: Provide expiresAt
SecretController->>EnforcementSecret: Set expires-at annotation
StatusController->>APIKey: Reconcile at expiry
StatusController->>APIKey: Set Expired condition
Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/controller/apikey_status_controller.go (1)
191-246: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Expiredcondition is never cleared once set.All four other branches of
calculateStatusConditions(Failed, Denied, Approved-not-expired, Pending) omitmeta.RemoveStatusCondition(&conditions, devportalv1alpha1.APIKeyConditionExpired). Once a key transitions toExpired=True, extendingexpiresAt(or any other state change) will leave the staleExpiredcondition set alongside the new condition, e.g.Approved=TrueandExpired=Trueat once.🐛 Proposed fix — add Expired removal to every branch
if failedCondition != nil { meta.RemoveStatusCondition(&conditions, devportalv1alpha1.APIKeyConditionApproved) meta.RemoveStatusCondition(&conditions, devportalv1alpha1.APIKeyConditionDenied) meta.RemoveStatusCondition(&conditions, devportalv1alpha1.APIKeyConditionPending) + meta.RemoveStatusCondition(&conditions, devportalv1alpha1.APIKeyConditionExpired) meta.SetStatusCondition(&conditions, *failedCondition) return conditions, nil } // Check for Denied condition - if denied, we're done deniedCondition := r.calculateDeniedCondition(ctx, apiKey) if deniedCondition != nil { meta.RemoveStatusCondition(&conditions, devportalv1alpha1.APIKeyConditionApproved) meta.RemoveStatusCondition(&conditions, devportalv1alpha1.APIKeyConditionFailed) meta.RemoveStatusCondition(&conditions, devportalv1alpha1.APIKeyConditionPending) + meta.RemoveStatusCondition(&conditions, devportalv1alpha1.APIKeyConditionExpired) meta.SetStatusCondition(&conditions, *deniedCondition) return conditions, nil } // Check for Approved condition - if approved, check if also expired approvedCondition := r.calculateApprovedCondition(ctx, apiKey) if approvedCondition != nil { if apiKey.Spec.ExpiresAt != nil && time.Now().After(apiKey.Spec.ExpiresAt.Time) { ... return conditions, nil } meta.RemoveStatusCondition(&conditions, devportalv1alpha1.APIKeyConditionDenied) meta.RemoveStatusCondition(&conditions, devportalv1alpha1.APIKeyConditionFailed) meta.RemoveStatusCondition(&conditions, devportalv1alpha1.APIKeyConditionPending) + meta.RemoveStatusCondition(&conditions, devportalv1alpha1.APIKeyConditionExpired) meta.SetStatusCondition(&conditions, *approvedCondition) return conditions, nil } // Check for Pending condition - if no approval, denial, or failure pendingCondition := r.calculatePendingCondition(ctx, apiKey) if pendingCondition != nil { meta.RemoveStatusCondition(&conditions, devportalv1alpha1.APIKeyConditionApproved) meta.RemoveStatusCondition(&conditions, devportalv1alpha1.APIKeyConditionDenied) meta.RemoveStatusCondition(&conditions, devportalv1alpha1.APIKeyConditionFailed) + meta.RemoveStatusCondition(&conditions, devportalv1alpha1.APIKeyConditionExpired) meta.SetStatusCondition(&conditions, *pendingCondition) return conditions, nil }🤖 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 `@internal/controller/apikey_status_controller.go` around lines 191 - 246, Add meta.RemoveStatusCondition(&conditions, devportalv1alpha1.APIKeyConditionExpired) to each non-expired branch of calculateStatusConditions: the failedCondition, deniedCondition, approvedCondition when not expired, and pendingCondition paths. Preserve the existing expired path while ensuring any transition back to Failed, Denied, Approved, or Pending clears the stale Expired condition.
🧹 Nitpick comments (1)
test/e2e/expiry_test.go (1)
60-203: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winTest doesn't verify secret deletion/preservation despite its name.
The test title states it verifies enforcement-secret deletion, but only APIKey status conditions are asserted (lines 188-202). Consider adding checks that the enforcement secret (label-selected in
kuadrantNamespaceby APIKey name/namespace, perapikey_secret_controller.go) is deleted after expiry, and that the consumer's original secret (%s-secretinconsumerNamespace) still exists — both are explicit requirements from the linked issue.By("verifying enforcement secret is deleted") Eventually(func(g Gomega) { cmd := exec.Command("kubectl", "get", "secrets", "-n", kuadrantNamespace, "-l", fmt.Sprintf("devportal.kuadrant.io/apikey-name=%s", apiKeyName), // adjust label key to match controller "-o", "jsonpath={.items}") output, err := utils.Run(cmd) g.Expect(err).NotTo(HaveOccurred()) g.Expect(output).To(Equal("[]")) }).Should(Succeed()) By("verifying consumer's original secret is preserved") cmd = exec.Command("kubectl", "get", "secret", apiKeyName+"-secret", "-n", consumerNamespace) _, err = utils.Run(cmd) Expect(err).NotTo(HaveOccurred())🤖 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/expiry_test.go` around lines 60 - 203, Extend the expiry test after verifying the Expired condition to assert that the controller-generated enforcement secret selected by the APIKey name/namespace labels in kuadrantNamespace is deleted. Also verify the original consumer secret created as apiKeyName+"-secret" in consumerNamespace still exists, using the controller’s exact label keys and preserving the existing condition assertions.
🤖 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.
Outside diff comments:
In `@internal/controller/apikey_status_controller.go`:
- Around line 191-246: Add meta.RemoveStatusCondition(&conditions,
devportalv1alpha1.APIKeyConditionExpired) to each non-expired branch of
calculateStatusConditions: the failedCondition, deniedCondition,
approvedCondition when not expired, and pendingCondition paths. Preserve the
existing expired path while ensuring any transition back to Failed, Denied,
Approved, or Pending clears the stale Expired condition.
---
Nitpick comments:
In `@test/e2e/expiry_test.go`:
- Around line 60-203: Extend the expiry test after verifying the Expired
condition to assert that the controller-generated enforcement secret selected by
the APIKey name/namespace labels in kuadrantNamespace is deleted. Also verify
the original consumer secret created as apiKeyName+"-secret" in
consumerNamespace still exists, using the controller’s exact label keys and
preserving the existing condition assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4eff3671-f489-484d-ae2c-10217e7218b0
📒 Files selected for processing (7)
api/v1alpha1/apikey_types.goapi/v1alpha1/zz_generated.deepcopy.goconfig/crd/bases/devportal.kuadrant.io_apikeys.yamlinternal/controller/apikey_secret_controller.gointernal/controller/apikey_status_controller.gointernal/controller/apikey_status_controller_test.gotest/e2e/expiry_test.go
|
👀 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@AGENTS.md`:
- Around line 124-125: Update the AGENTS.md “Controller Pattern” section and its
related internal/controller list to replace the nonexistent APIKeyReconciler
with APIKeyStatusReconciler for expiry and condition handling, and
APIKeySecretReconciler for enforcement-secret creation and deletion; ensure the
documented reconciler names and responsibilities match the registered
implementations.
🪄 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 Plus
Run ID: 10c347e9-5bac-4b4b-8668-4783bc0cafb5
📒 Files selected for processing (6)
AGENTS.mdapi/v1alpha1/apikeyrequest_types.goapi/v1alpha1/zz_generated.deepcopy.goconfig/crd/bases/devportal.kuadrant.io_apikeyrequests.yamlinternal/controller/apikey_status_controller.gointernal/controller/apikeyrequest_controller.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/controller/apikey_status_controller.go
Signed-off-by: Anton-Fil <a.filkach@gmail.com>
1bc41c4 to
275d95c
Compare
|
hack/test-expiry/ directory is missing — the manual verification steps can't be followed as written. do you want to add them? not a big deal as not in docs |
R-Lawton
left a comment
R-Lawton
left a comment
There was a problem hiding this comment.
Really nice work Antonio, Verified it works as expected
2 participants
Summary
Add expiresAt field to APIKeySpec so consumers can set a desired expiration when requesting an API key. The controller automatically revokes the key when it expires.
Closes #87
Design
Changes
api/v1alpha1/apikey_types.go
internal/controller/apikey_status_controller.go
internal/controller/apikey_secret_controller.go
Expired logic
calculateStatusConditions priority:
Failed → Denied → Approved? → expiresAt passed? → Expired
→ not passed? → Approved
→ Pending
When Expired=True:
Tests added
Unit tests (internal/controller/apikey_status_controller_test.go):
E2E test (test/e2e/expiry_test.go):
How to manually verify
Start Kind cluster
make setup-test-e2e
Run controller locally
make run
In new terminal — create namespaces and Kuadrant
kubectl create ns owner-ns consumer-ns kuadrant-ns
kubectl apply -f hack/test-expiry/kuadrant.yaml
Create APIProduct, Secret, APIKey with short expiresAt
kubectl create secret generic my-secret
--from-literal=api_key=test-value -n consumer-ns
kubectl apply -f - <<EOF
apiVersion: devportal.kuadrant.io/v1alpha1
kind: APIKey
metadata:
name: my-key
namespace: consumer-ns
spec:
apiProductRef:
name: my-api
namespace: owner-ns
secretRef:
name: my-secret
planTier: basic
requestedBy:
userId: test-user
email: test@example.com
expiresAt: "$(date -u -v+120S '+%Y-%m-%dT%H:%M:%SZ')"
EOF
Approve the key
kubectl apply -f hack/test-expiry/approval.yaml
Watch status
kubectl get apikey my-key -n consumer-ns -o jsonpath='{.status.conditions}' | python3 -m json.tool
After 2 minutes — verify Expired
kubectl get apikey my-key -n consumer-ns -o jsonpath='{.status.conditions}' | python3 -m json.tool
kubectl get secrets -n kuadrant-ns -l devportal.kuadrant.io/apikey=my-key
Expected: Expired=True, no secrets
make cleanup-test-e2e
Summary by CodeRabbit
expiresAt), alongside API key requests.