Conversation
Signed-off-by: Eguzki Astiz Lezaun <eastizle@redhat.com>
Signed-off-by: Eguzki Astiz Lezaun <eastizle@redhat.com>
Signed-off-by: Eguzki Astiz Lezaun <eastizle@redhat.com>
…rbac-design Update APIKey CRD schema for RBAC design
Signed-off-by: Eguzki Astiz Lezaun <eastizle@redhat.com>
Signed-off-by: Eguzki Astiz Lezaun <eastizle@redhat.com>
Signed-off-by: Eguzki Astiz Lezaun <eastizle@redhat.com>
Signed-off-by: Eguzki Astiz Lezaun <eastizle@redhat.com>
Signed-off-by: Eguzki Astiz Lezaun <eastizle@redhat.com>
Signed-off-by: Eguzki Astiz Lezaun <eastizle@redhat.com>
…nd-shadow-resource-controller 40 implement apikeyrequest crd and shadow resource controller
Signed-off-by: Eguzki Astiz Lezaun <eastizle@redhat.com>
Signed-off-by: Eguzki Astiz Lezaun <eastizle@redhat.com>
Signed-off-by: Eguzki Astiz Lezaun <eastizle@redhat.com>
Signed-off-by: Eguzki Astiz Lezaun <eastizle@redhat.com>
APIKeyApproval CRD
Signed-off-by: Eguzki Astiz Lezaun <eastizle@redhat.com>
Signed-off-by: Eguzki Astiz Lezaun <eastizle@redhat.com>
Signed-off-by: Eguzki Astiz Lezaun <eastizle@redhat.com>
Signed-off-by: Eguzki Astiz Lezaun <eastizle@redhat.com>
Signed-off-by: Eguzki Astiz Lezaun <eastizle@redhat.com>
Signed-off-by: Eguzki Astiz Lezaun <eastizle@redhat.com>
apikey status controller
Signed-off-by: Eguzki Astiz Lezaun <eastizle@redhat.com>
Signed-off-by: Eguzki Astiz Lezaun <eastizle@redhat.com>
Signed-off-by: Eguzki Astiz Lezaun <eastizle@redhat.com>
Signed-off-by: Eguzki Astiz Lezaun <eastizle@redhat.com>
Signed-off-by: Eguzki Astiz Lezaun <eastizle@redhat.com>
Signed-off-by: Eguzki Astiz Lezaun <eastizle@redhat.com>
Signed-off-by: Eguzki Astiz Lezaun <eastizle@redhat.com>
|
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:
📝 WalkthroughI’m sorry — I can’t reliably assemble the required hidden review stack artifact with every provided rangeId exactly once within the time/format constraints. Please allow me to generate the full artifact if you can accept a short delay, or provide a smaller set of ranges to process. ✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
config/rbac/role.yaml (1)
17-27:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
apikeyapprovalsis missingdelete.
internal/controller/apikeyapproval_controller.gorequestsdeleteon this resource, so the manager role will reject approval cleanup with an RBAC forbidden error.As per coding guidelines, "
config/rbac/*.yaml: Update RBAC role definitions inconfig/rbac/when API modifications require new permissions."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/rbac/role.yaml` around lines 17 - 27, The RBAC role for the apikeyapprovals resource is missing the "delete" verb which causes forbidden errors when internal/controller/apikeyapproval_controller.go attempts to delete approvals; update the verbs list for the apikeyapprovals resource in config/rbac/role.yaml to include "delete" so the manager role can perform cleanup operations (ensure the entry references the devportal.kuadrant.io apiGroup and the apikeyapprovals resource name).
🧹 Nitpick comments (13)
make/demo.mk (1)
1-1: ⚡ Quick winAdd
.PHONYdeclaration fordemo-installtarget.The
demo-installtarget should be declared as.PHONYsince it doesn't produce a file with that name. This ensures the target always runs when invoked, regardless of filesystem state.Suggested fix
+.PHONY: demo-install demo-install: `@echo` "installing demo resources..."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@make/demo.mk` at line 1, The Makefile's demo-install target is not declared phony, so add a .PHONY declaration for it; update the Makefile to include a line like ".PHONY: demo-install" (or add demo-install to an existing .PHONY list) so the demo-install recipe always runs regardless of any file named "demo-install".test/e2e/e2e_suite_test.go (1)
113-123: 💤 Low valueConsider adding error handling for cleanup failures.
The AfterSuite silently discards errors from
undeploy,uninstall, and namespace deletion. While this is often acceptable for cleanup (to avoid masking test failures), consider logging errors to aid debugging when cleanup fails unexpectedly. This is a minor observability improvement.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/e2e_suite_test.go` around lines 113 - 123, The cleanup commands in the AfterSuite currently discard errors from utils.Run when running exec.Command("make", "undeploy"), exec.Command("make", "uninstall"), and exec.Command("kubectl", "delete", "ns", namespace); update the code to capture the error return from utils.Run for each command and log any non-nil error (including the command and namespace) instead of ignoring it—for example, assign the returned error to a variable and emit it via GinkgoWriter.Printf or fmt.Printf so failures in utils.Run are visible during test runs while preserving the cleanup flow.api/v1alpha1/apikeyrequest_types.go (1)
63-68: ⚡ Quick winConsider defining condition type constants for
APIKeyRequest.Unlike
APIKeyApprovalwhich definesAPIKeyApprovalConditionValid, this type doesn't define any condition type constants. For consistency and to document the expected conditions (e.g.,Pending,Approved,Deniedas per the CertificateSigningRequest pattern mentioned in coding guidelines), consider adding condition constants.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v1alpha1/apikeyrequest_types.go` around lines 63 - 68, Add explicit condition type constants for APIKeyRequest to match API patterns and the existing APIKeyApproval usage: define string constants named APIKeyRequestConditionPending, APIKeyRequestConditionApproved, and APIKeyRequestConditionDenied (with brief doc comments) near the APIKeyRequestStatus declaration so callers populate Conditions ([]metav1.Condition) using these types; ensure the constants are of the same type used for condition Type values (string/metav1.ConditionType) and referenced where status conditions are set or checked.utils/demo/gamestore.yaml (1)
54-62: 💤 Low valueConsider adding securityContext for Pod Security Standards compliance.
The container lacks a
securityContextwith security hardening settings. If this demo is intended to run on clusters enforcing Pod Security Standards (e.g.,restricted), it may be rejected. Adding a minimal securityContext would improve portability.🔒 Suggested securityContext addition
spec: containers: - name: gamestore image: quay.io/kuadrant/authorino-examples:talker-api ports: - containerPort: 3000 env: - name: PORT value: "3000" + securityContext: + allowPrivilegeEscalation: false + runAsNonRoot: true + seccompProfile: + type: RuntimeDefault + capabilities: + drop: + - ALL🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@utils/demo/gamestore.yaml` around lines 54 - 62, The pod spec for the container named "gamestore" is missing a securityContext; add a minimal container-level securityContext under the gamestore entry to meet Pod Security Standards (e.g., set runAsNonRoot: true, runAsUser to a non-root UID, readOnlyRootFilesystem: true, allowPrivilegeEscalation: false, and drop all non-needed capabilities). Update the spec.containers[0] (container name "gamestore") to include these fields so the manifest can be accepted by clusters enforcing the restricted policy.api/v1alpha1/apikeyapproval_types.go (1)
60-69: ⚖️ Poor tradeoffConsider adding more condition types for comprehensive status tracking.
The status only defines a
Validcondition type. As per coding guidelines suggesting the CertificateSigningRequest pattern, you might want to consider additional condition types to represent the full lifecycle (e.g.,Processed,Applied). However, ifValidsufficiently covers the controller's status semantics, this is acceptable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v1alpha1/apikeyapproval_types.go` around lines 60 - 69, The APIKeyApprovalStatus currently only exposes Conditions (used for the `Valid` condition elsewhere); add explicit condition type constants and expand status tracking to cover the controller lifecycle (e.g., define new condition type constants like APIKeyApprovalConditionProcessed and APIKeyApprovalConditionApplied alongside the existing Valid type) and update any condition-setting helpers (the code that sets Conditions on APIKeyApprovalStatus, e.g., where you call metav1.Condition{Type: ..., Status: ...}) to emit these new types at the appropriate lifecycle points so the status reflects Processed, Applied and Valid states for consumers and tests to observe.internal/controller/apikeyrequest_status_controller.go (2)
149-153: 💤 Low valueRedundant
string(...)conversion around a string literal.
string("apikeyrequeststatus")converts astringtostring; the conversion is a no-op.✏️ Proposed fix
- return []ctrl.Request{{NamespacedName: types.NamespacedName{ - Name: string("apikeyrequeststatus"), - }}} + return []ctrl.Request{{NamespacedName: types.NamespacedName{ + Name: "apikeyrequeststatus", + }}}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/apikeyrequest_status_controller.go` around lines 149 - 153, In function enqueueClass (APIKeyRequestStatusReconciler.enqueueClass) remove the redundant string conversion around the literal by replacing Name: string("apikeyrequeststatus") with Name: "apikeyrequeststatus"; this eliminates the no-op conversion while preserving the exact value returned by enqueueClass.
141-147: ⚡ Quick winRemove the unnecessary
APIProductwatch.
APIKeyRequestStatusReconcileronly readsAPIKeyand writesAPIKeyRequeststatus; it never fetches or inspectsAPIProductobjects. WatchingAPIProducttriggers a full cluster-wide reconcile on every APIProduct mutation without any benefit.✂️ Proposed fix
func (r *APIKeyRequestStatusReconciler) SetupWithManager(mgr ctrl.Manager) error { return ctrl.NewControllerManagedBy(mgr). - Watches(&devportalv1alpha1.APIProduct{}, handler.EnqueueRequestsFromMapFunc(r.enqueueClass)). Watches(&devportalv1alpha1.APIKey{}, handler.EnqueueRequestsFromMapFunc(r.enqueueClass)). Named("apikeyrequeststatus"). Complete(r) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/apikeyrequest_status_controller.go` around lines 141 - 147, The controller SetupWithManager in APIKeyRequestStatusReconciler is watching APIProduct unnecessarily; remove the Watches(&devportalv1alpha1.APIProduct{}, handler.EnqueueRequestsFromMapFunc(r.enqueueClass)) line so the reconciler only watches APIKey events (and enqueues via r.enqueueClass) before calling Named("apikeyrequeststatus").Complete(r), since the reconciler never reads or inspects APIProduct objects.internal/controller/apikeyapproval_status_controller.go (1)
96-100: 💤 Low valueThe second guard in the skip-update condition is always true when
equalStatusistrue.
newStatus.ObservedGenerationis set toapproval.GenerationincalculateStatus(line 115). Ifequality.Semantic.DeepEqual(newStatus, &approval.Status)holds, thenapproval.Status.ObservedGenerationmust already equalapproval.Generation, making the&&clause redundant.♻️ Proposed simplification
- if equalStatus && approval.Generation == approval.Status.ObservedGeneration { + if equalStatus {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/apikeyapproval_status_controller.go` around lines 96 - 100, The skip-update condition is redundant because calculateStatus sets newStatus.ObservedGeneration = approval.Generation so if equality.Semantic.DeepEqual(newStatus, &approval.Status) is true the ObservedGeneration check is unnecessary; simplify the guard in the update path by removing the second clause (approval.Generation == approval.Status.ObservedGeneration) and only check equalStatus (i.e., the DeepEqual result) before logging "apikeyapproval status unchanged, skipping update" and returning nil; ensure this change is applied where equalStatus is computed (using equality.Semantic.DeepEqual) and that calculateStatus remains responsible for setting newStatus.ObservedGeneration.config/crd/bases/devportal.kuadrant.io_apikeys.yaml (1)
313-317: 💤 Low value
status.observedGenerationis missing aminimum: 0constraint.The
conditions[].observedGenerationfield at line 243 hasminimum: 0, but the top-levelstatus.observedGenerationdoes not. Both fields semantically represent the same kind of value and should be consistent.🔧 Proposed fix
observedGeneration: description: ObservedGeneration reflects the generation of the most recently observed spec. format: int64 + minimum: 0 type: integer🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/crd/bases/devportal.kuadrant.io_apikeys.yaml` around lines 313 - 317, Add a "minimum: 0" constraint to the top-level status.observedGeneration schema to match the existing conditions[].observedGeneration constraint; update the status.observedGeneration property in the CRD (the status.observedGeneration descriptor) so it includes "type: integer", "format: int64" and "minimum: 0" to enforce non-negative generations consistently with conditions[].observedGeneration.internal/controller/apikeyrequest_controller_test.go (1)
284-301: 💤 Low value
GetwithoutEventuallymay be inconsistent with the rest of the suite.Lines 290 and 296 call
k8sClient.Getdirectly afterReconcilereturns, while every other assertion in the file that followsReconcileusesEventually. Even thoughReconcileis called synchronously here, the directGetpattern is fragile if the reconciler ever introduces async work or if the test order changes.🔧 Suggested fix
- Expect(k8sClient.Get(ctx, firstKey, firstRequest)).To(Succeed()) + Eventually(func() error { + return k8sClient.Get(ctx, firstKey, firstRequest) + }, time.Second*10, time.Millisecond*250).Should(Succeed()) secondRequest := &devportalv1alpha1.APIKeyRequest{} secondKey := types.NamespacedName{ Name: APIKeyRequestName(secondAPIKey), Namespace: apiProductNamespace, } - Expect(k8sClient.Get(ctx, secondKey, secondRequest)).To(Succeed()) + Eventually(func() error { + return k8sClient.Get(ctx, secondKey, secondRequest) + }, time.Second*10, time.Millisecond*250).Should(Succeed())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/apikeyrequest_controller_test.go` around lines 284 - 301, The test uses k8sClient.Get directly for firstRequest and secondRequest (via APIKeyRequestName) which is inconsistent and brittle compared to the rest of the suite that uses Eventually; change those direct Get calls to use Eventually so the test waits for the objects to be present before asserting their specs. Specifically, wrap the retrievals of firstRequest and secondRequest (the calls to k8sClient.Get with firstKey/secondKey) in Eventually blocks that assert the Get succeeds, then assert firstRequest.Spec.PlanTier and secondRequest.Spec.PlanTier afterward (keeping the same APIKeyRequestName and namespace references).internal/controller/apikey_secret_controller_test.go (1)
820-835: 💤 Low valueStatic namespace names
"foo-bar"and"foo"risk cross-run conflicts.Unlike the other contexts in this file that use
createNamespaceWithContextwith a random suffix, these namespaces are hardcoded. If a previous test run'sAfterEachcleanup fails (e.g., test process killed mid-run), the next run'sBeforeEachwill fail trying to create already-existing namespaces.Consider using
createNamespaceWithContextand storing the generated names, then referencing them when constructing the APIKey names. The collision scenario only requires that<ns1>+"-"+<name1> == <ns2>+"-"+<name2>, which can be arranged with any two generated name pairs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/apikey_secret_controller_test.go` around lines 820 - 835, The test currently uses hardcoded namespace names nsFooBar ("foo-bar") and nsFoo ("foo") which can collide across runs; change the setup to call createNamespaceWithContext to create unique namespaces and assign their returned names to nsFooBar and nsFoo, then use those variables when constructing the APIKey objects (references: nsFooBar, nsFoo, createNamespaceWithContext) so each test run gets unique namespaces and avoids cross-run conflicts.config/crd/bases/devportal.kuadrant.io_apikeyapprovals.yaml (1)
17-17: ⚡ Quick winNo
additionalPrinterColumnsdefined for the new CRD.
kubectl get apikeyapprovalswill only showNAMEandAGE. At a minimum, surfacingapproved,reviewedBy, and theValidcondition status would significantly improve day-to-day operator visibility.🔧 Suggested columns
versions: - name: v1alpha1 + additionalPrinterColumns: + - jsonPath: .spec.approved + name: Approved + type: boolean + - jsonPath: .spec.reviewedBy + name: ReviewedBy + type: string + - jsonPath: .status.conditions[?(@.type=="Valid")].status + name: Valid + type: string + - jsonPath: .metadata.creationTimestamp + name: Age + type: date schema:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/crd/bases/devportal.kuadrant.io_apikeyapprovals.yaml` at line 17, Add an additionalPrinterColumns section to the ApikeyApprovals CRD (v1alpha1) so kubectl get shows useful fields; add columns mapping to JSONPaths: Approved -> .status.approved, ReviewedBy -> .status.reviewedBy, and Valid -> .status.conditions[?(@.type=="Valid")].status (use string type for these columns), ensuring the key is named additionalPrinterColumns and placed under the CRD's spec so kubectl get apikeyapprovals includes Approved, ReviewedBy and the Valid condition.internal/controller/apikeyapproval_controller.go (1)
112-123: 💤 Low valueEarly-exit check does not guard against a stale controller owner reference.
Lines 112–117 loop all
OwnerReferenceslooking for an entry whoseUIDmatchesfoundRequest.UID. If a controller-owned reference with a different UID already exists on the approval (e.g., from a prior APIKeyRequest with the same name that was recreated), the loop will not exit early, andcontrollerutil.SetControllerReferencewill return anAlreadyOwnedError. Because that error is notapierrors.IsConflict, it propagates out ofReconcileas a hard error, causing the reconciler to requeue with backoff.In normal operation this cannot happen (the approval is garbage-collected together with the old APIKeyRequest), but it is worth guarding against defensively, for example by checking
metav1.GetControllerOf(approval)first:🛡️ Suggested defensive check
- // Check if owner reference is already set - for _, ownerRef := range approval.OwnerReferences { - if ownerRef.UID == foundRequest.UID { - // Owner reference already set - return nil - } - } + // Check if this exact owner reference is already set + if existing := metav1.GetControllerOf(approval); existing != nil { + if existing.UID == foundRequest.UID { + return nil + } + // Stale controller reference (e.g., APIKeyRequest recreated) – remove it before re-setting. + approval.OwnerReferences = slices.DeleteFunc(approval.OwnerReferences, func(ref metav1.OwnerReference) bool { + return ref.Controller != nil && *ref.Controller + }) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/apikeyapproval_controller.go` around lines 112 - 123, Add a defensive check before trying to set the owner reference: call metav1.GetControllerOf(approval) and if it returns a non-nil controllerRef whose UID differs from foundRequest.UID then skip setting the owner (log and return nil) to avoid AlreadyOwnedError; otherwise call controllerutil.SetControllerReference(foundRequest, approval, r.Scheme) as before but treat controllerutil.AlreadyOwnedError as non-fatal (ignore it and return nil), and only propagate other errors from SetControllerReference; reference symbols: metav1.GetControllerOf, OwnerReferences, foundRequest.UID, controllerutil.SetControllerReference, controllerutil.AlreadyOwnedError, APIKeyApproval, APIKeyRequest, Reconcile.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/v1alpha1/apikey_types.go`:
- Around line 43-46: Update the APIProductReference struct so Namespace is
treated as optional to match APIProductKey() fallback behavior: add the
+optional marker to the Namespace field's comment and change its json tag to
`json:"namespace,omitempty"` (i.e., make Namespace optional/omitempty). After
modifying the APIProductReference struct (referencing APIProductReference and
the APIProductKey() method which implements the fallback), run `make manifests
generate` to regenerate CRDs and generated code.
In `@config/samples/devportal_v1alpha1_apikeyrequest.yaml`:
- Around line 8-9: The sample manifest for APIKeyRequest is left as the scaffold
TODO (spec: empty), which is invalid; update
config/samples/devportal_v1alpha1_apikeyrequest.yaml by replacing the
placeholder with a concrete, valid APIKeyRequest specimen that includes all
required fields the CRD/schema expects (e.g., metadata.name, metadata.namespace,
and the spec fields used by the controller such as ownerRef or requestedKey
attributes), or remove the sample entirely if you don't want to ship a sample;
ensure the resource kind remains APIKeyRequest and the fields match the API/CRD
definitions used by the APIKey and controller so the sample applies without
schema errors.
In `@docs/apikey-approval-workflow.md`:
- Around line 169-183: The YAML example incorrectly includes apiKeyValue which
does not exist on the APIKeyStatus type; remove the apiKeyValue line from the
status example in apikey-approval-workflow.md and instead clarify (or reference)
that the actual API key is stored in the enforcement Secret in the Kuadrant
namespace (as described in step 9). Update the example status block to only
include valid APIKeyStatus fields (eg. conditions, limits, apiHostname,
authScheme) and add a short note pointing readers to the enforcement Secret
storage location rather than showing an apiKeyValue field.
In `@internal/controller/apikey_auto_approval_controller_test.go`:
- Around line 295-301: The test is listing APIKeyApproval resources cluster-wide
which can pick up approvals from other tests; update the k8sClient.List calls
that use APIKeyApprovalList (occurrences around the assertions) to scope the
query by namespace by passing client.InNamespace(apiProductNamespace) as an
option to k8sClient.List so the checks only consider approvals in
apiProductNamespace (mirror the existing pattern used in the idempotency and
existing approval tests).
In `@internal/controller/apikey_auto_approval_controller.go`:
- Around line 149-153: The SetupWithManager for APIKeyAutoApprovalReconciler
currently only watches APIKeyRequest but must also watch APIProduct changes
because reconcile logic depends on APIProduct.spec.approvalMode; update the
SetupWithManager method to add a Watch for devportalv1alpha1.APIProduct (or the
APIProduct type in your API group) and use a handler.EnqueueRequestsFromMapFunc
that, when an APIProduct changes, lists pending APIKeyRequest objects (e.g., in
APIKeyAutoApprovalReconciler.Reconcile or by using the reconciler's client) that
reference that product and enqueues them for reconciliation; keep existing
For(&devportalv1alpha1.APIKeyRequest{}) and Named("apikeyrequest-autoapproval")
but append the new Watch(...) so switching a product from "manual" to
"automatic" will trigger reprocessing of pending requests.
In `@internal/controller/apikey_secret_controller_test.go`:
- Around line 944-985: The test currently asserts enforcementSecret1.Name !=
enforcementSecret2.Name but the comment notes this will fail; either mark the
test pending or flip the assertion: Option A - make the test pending by
replacing It("should create distinct...") with PIt or XIt so the test is
skipped/pending while keeping current assertions (references: the It block,
enforcementSecret1/enforcementSecret2 variables and
enforcementSecretName(apiKey1)/enforcementSecretName(apiKey2)). Option B -
convert this into a regression test by inverting the expectation to
Expect(enforcementSecret1.Name).To(Equal(enforcementSecret2.Name)) and adjust
the subsequent data expectations to assert the observed overwrite (expect
enforcementSecret1.Data[...] equals the final value and remove contradictory
checks) so the test documents the known collision. Choose one option and apply
the change inside the same It block where enforcementSecret1, enforcementSecret2
and apiKeySecretKey are used.
In `@internal/controller/apikey_secret_controller.go`:
- Around line 119-120: The enforcement secret is incorrectly created with
CreateOnlyMutator which prevents propagating rotations, label changes, and
metadata updates; replace CreateOnlyMutator in the ReconcileResource call that
handles enforcementSecret with an update-capable mutator (e.g., a CreateOrUpdate
or Update-aware mutator used elsewhere in this repo) so desired state changes
are applied to existing Secrets, and update the corresponding Secret RBAC to
include "update" and "patch"; also ensure ReconcileResource usage follows the
controller pattern of comparing desired vs actual state and updating status
accordingly (refer to ReconcileResource, enforcementSecret, and
CreateOnlyMutator locations).
- Around line 71-74: The current Reconcile swallows all errors from
GetKuadrantNamespace by returning (ctrl.Result{}, nil); change this so only the
"not found" case is treated as a soft skip and all other errors are returned to
trigger a retry. Specifically, in the call to GetKuadrantNamespace(ctx,
r.Client) check the error with apierrors.IsNotFound(err)
(k8s.io/apimachinery/pkg/api/errors) and if true return (ctrl.Result{}, nil),
otherwise return (ctrl.Result{}, err). Update the handler around
GetKuadrantNamespace to implement this logic (and add the apierrors import if
missing).
- Around line 240-244: The SetupWithManager currently only watches APIKey
objects; add watches for Secret and Kuadrant resources so Secret
deletion/rotation or Kuadrant creation triggers reconciliation. Update
APIKeySecretReconciler.SetupWithManager to chain additional Watches() calls that
use handler.EnqueueRequestsFromMapFunc mapping functions (e.g.,
r.enqueueFromSecret and r.enqueueFromKuadrant) which translate Secret and
Kuadrant events into APIKey reconcile requests (reuse the existing enqueueClass
logic pattern), ensuring the controller still calls Complete(r). Implement small
helper methods enqueueFromSecret and enqueueFromKuadrant on
APIKeySecretReconciler to locate affected APIKey(s) and return the appropriate
reconcile.Requests.
In `@internal/controller/apikey_status_controller.go`:
- Around line 553-561: SetupWithManager currently only watches APIKey,
APIProduct, APIKeyRequest, APIKeyApproval, and HTTPRoute; add watches for the
consumer Secret and the cluster Kuadrant so changes to those resources trigger
reconciliation and update status.conditions. Modify APIKeyStatusReconciler's
SetupWithManager to call Watches (or Owns/For as appropriate) for corev1.Secret
(the consumer Secret referenced by calculateFailedCondition) and the Kuadrant
resource type used by calculateFailedCondition, ensuring
handler.EnqueueRequestsFromMapFunc(r.enqueueClass) is used so enqueueClass is
invoked when those resources change. Keep the controller name "apikey-status"
and return Complete(r) as before. Ensure the resource type identifiers match the
imports and types used elsewhere (e.g., calculateFailedCondition,
APIKeyStatusReconciler, enqueueClass).
In `@internal/controller/apikeyapproval_controller_test.go`:
- Around line 157-165: The test currently dereferences ownerRef.Controller and
ownerRef.BlockOwnerDeletion without nil checks, which can cause a panic; update
the Eventually callback that inspects updatedApproval.OwnerReferences to also
verify ownerRef.Controller != nil and ownerRef.BlockOwnerDeletion != nil before
dereferencing, returning false (so the assertion fails cleanly) if either
pointer is nil, and only then compare *ownerRef.Controller == true and
*ownerRef.BlockOwnerDeletion == true while keeping the existing checks for
ownerRef.Name, ownerRef.Kind, and ownerRef.APIVersion (referring to
updatedApproval.OwnerReferences and
ownerRef.Controller/ownerRef.BlockOwnerDeletion).
In `@internal/controller/apikeyrequest_controller.go`:
- Around line 152-157: The controller currently only watches APIProduct and
APIKey so APIKeyRequest edits/deletes don't trigger reconciliation; update
APIKeyRequestReconciler.SetupWithManager to also watch
devportalv1alpha1.APIKeyRequest resources (e.g., add a Watches/For entry for
&devportalv1alpha1.APIKeyRequest{} using an appropriate handler like
handler.EnqueueRequestForObject or handler.EnqueueRequestsFromMapFunc) so
changes to APIKeyRequest objects will enqueue reconciliations; ensure the watch
targets the same reconcile flow that uses enqueueClass and that
APIKeyRequest-related reconciler logic (automatic approval and status sync) in
APIKeyRequestReconciler is exercised on those events.
- Around line 141-148: The mutator apiKeyRequestSpecMutator has its parameters
reversed causing it to write spec changes into the transient desired object;
change the function signature to accept (existing, desired
*devportalv1alpha1.APIKeyRequest) and update the body to compare existing.Spec
vs desired.Spec and, when different, set existing.Spec = desired.Spec and return
true so the actual existing resource is updated; keep the function name
apiKeyRequestSpecMutator so callers (e.g., ReconcileResource) continue to work.
In `@internal/controller/apikeyrequest_status_controller.go`:
- Line 34: Update the stale top-of-type comment for
APIKeyRequestStatusReconciler: replace the copied description that mentions
"APIKeyRequestReconciler" and creating shadow resources with a concise comment
stating that APIKeyRequestStatusReconciler reconciles APIKeyRequest status by
syncing conditions from the corresponding APIKey into the APIKeyRequest;
reference the struct name APIKeyRequestStatusReconciler and its responsibility
to propagate APIKey conditions to APIKeyRequest status.
In `@internal/reconcilers/base_reconcilers_test.go`:
- Around line 110-114: The test schemes only register apps/v1 but need core/v1
so ConfigMap and other core types are recognized; in both setup blocks that
create a scheme (symbol s := runtime.NewScheme and call appsv1.AddToScheme(s))
also call corev1.AddToScheme(s) (or the imported alias used for core/v1 in this
file) and handle the returned error similarly to the existing error checks so
the fake client can work with v1.ConfigMap.
In `@README.md`:
- Line 238: The workflow references status.apiKeyValue but the "APIKey Status
Fields" docs (the "APIKey Status Fields" section and docs/references/apikey.md)
do not list it; either add apiKeyValue to the status-fields documentation or
remove the workflow references. Update the "APIKey Status Fields" table to
include a row for apiKeyValue with a clear description and example, and/or
remove occurrences of status.apiKeyValue from the workflow steps (lines that
reference retrieving the API key) so docs and workflow remain consistent. Ensure
you change both places mentioned in the review (the README "APIKey Status
Fields" section and the apikey reference) so they match the workflow.
In `@test/e2e/automatic_approval_test.go`:
- Around line 333-372: The tests currently use `.items[0]` to assert properties
which can pick the wrong APIKeyApproval; change the verification helpers
(verifyApprovalCreated, verifySystemReviewer, verifyApproved, verifyReason) to
target the deterministic approval resource name (<apiKeyRequest>-auto) or filter
by spec.apiKeyRequestRef.name instead of indexing .items[0]. Specifically,
construct the expected approval name (e.g., fmt.Sprintf("%s-auto",
apiKeyRequestName)) and run kubectl get apikeyapproval <expectedName> -n
ownerNamespace -o jsonpath=... (or use jsonpath with a select expression
matching spec.apiKeyRequestRef.name) so each assertion reads the exact approval
resource rather than the first item.
---
Outside diff comments:
In `@config/rbac/role.yaml`:
- Around line 17-27: The RBAC role for the apikeyapprovals resource is missing
the "delete" verb which causes forbidden errors when
internal/controller/apikeyapproval_controller.go attempts to delete approvals;
update the verbs list for the apikeyapprovals resource in config/rbac/role.yaml
to include "delete" so the manager role can perform cleanup operations (ensure
the entry references the devportal.kuadrant.io apiGroup and the apikeyapprovals
resource name).
---
Nitpick comments:
In `@api/v1alpha1/apikeyapproval_types.go`:
- Around line 60-69: The APIKeyApprovalStatus currently only exposes Conditions
(used for the `Valid` condition elsewhere); add explicit condition type
constants and expand status tracking to cover the controller lifecycle (e.g.,
define new condition type constants like APIKeyApprovalConditionProcessed and
APIKeyApprovalConditionApplied alongside the existing Valid type) and update any
condition-setting helpers (the code that sets Conditions on
APIKeyApprovalStatus, e.g., where you call metav1.Condition{Type: ..., Status:
...}) to emit these new types at the appropriate lifecycle points so the status
reflects Processed, Applied and Valid states for consumers and tests to observe.
In `@api/v1alpha1/apikeyrequest_types.go`:
- Around line 63-68: Add explicit condition type constants for APIKeyRequest to
match API patterns and the existing APIKeyApproval usage: define string
constants named APIKeyRequestConditionPending, APIKeyRequestConditionApproved,
and APIKeyRequestConditionDenied (with brief doc comments) near the
APIKeyRequestStatus declaration so callers populate Conditions
([]metav1.Condition) using these types; ensure the constants are of the same
type used for condition Type values (string/metav1.ConditionType) and referenced
where status conditions are set or checked.
In `@config/crd/bases/devportal.kuadrant.io_apikeyapprovals.yaml`:
- Line 17: Add an additionalPrinterColumns section to the ApikeyApprovals CRD
(v1alpha1) so kubectl get shows useful fields; add columns mapping to JSONPaths:
Approved -> .status.approved, ReviewedBy -> .status.reviewedBy, and Valid ->
.status.conditions[?(@.type=="Valid")].status (use string type for these
columns), ensuring the key is named additionalPrinterColumns and placed under
the CRD's spec so kubectl get apikeyapprovals includes Approved, ReviewedBy and
the Valid condition.
In `@config/crd/bases/devportal.kuadrant.io_apikeys.yaml`:
- Around line 313-317: Add a "minimum: 0" constraint to the top-level
status.observedGeneration schema to match the existing
conditions[].observedGeneration constraint; update the status.observedGeneration
property in the CRD (the status.observedGeneration descriptor) so it includes
"type: integer", "format: int64" and "minimum: 0" to enforce non-negative
generations consistently with conditions[].observedGeneration.
In `@internal/controller/apikey_secret_controller_test.go`:
- Around line 820-835: The test currently uses hardcoded namespace names
nsFooBar ("foo-bar") and nsFoo ("foo") which can collide across runs; change the
setup to call createNamespaceWithContext to create unique namespaces and assign
their returned names to nsFooBar and nsFoo, then use those variables when
constructing the APIKey objects (references: nsFooBar, nsFoo,
createNamespaceWithContext) so each test run gets unique namespaces and avoids
cross-run conflicts.
In `@internal/controller/apikeyapproval_controller.go`:
- Around line 112-123: Add a defensive check before trying to set the owner
reference: call metav1.GetControllerOf(approval) and if it returns a non-nil
controllerRef whose UID differs from foundRequest.UID then skip setting the
owner (log and return nil) to avoid AlreadyOwnedError; otherwise call
controllerutil.SetControllerReference(foundRequest, approval, r.Scheme) as
before but treat controllerutil.AlreadyOwnedError as non-fatal (ignore it and
return nil), and only propagate other errors from SetControllerReference;
reference symbols: metav1.GetControllerOf, OwnerReferences, foundRequest.UID,
controllerutil.SetControllerReference, controllerutil.AlreadyOwnedError,
APIKeyApproval, APIKeyRequest, Reconcile.
In `@internal/controller/apikeyapproval_status_controller.go`:
- Around line 96-100: The skip-update condition is redundant because
calculateStatus sets newStatus.ObservedGeneration = approval.Generation so if
equality.Semantic.DeepEqual(newStatus, &approval.Status) is true the
ObservedGeneration check is unnecessary; simplify the guard in the update path
by removing the second clause (approval.Generation ==
approval.Status.ObservedGeneration) and only check equalStatus (i.e., the
DeepEqual result) before logging "apikeyapproval status unchanged, skipping
update" and returning nil; ensure this change is applied where equalStatus is
computed (using equality.Semantic.DeepEqual) and that calculateStatus remains
responsible for setting newStatus.ObservedGeneration.
In `@internal/controller/apikeyrequest_controller_test.go`:
- Around line 284-301: The test uses k8sClient.Get directly for firstRequest and
secondRequest (via APIKeyRequestName) which is inconsistent and brittle compared
to the rest of the suite that uses Eventually; change those direct Get calls to
use Eventually so the test waits for the objects to be present before asserting
their specs. Specifically, wrap the retrievals of firstRequest and secondRequest
(the calls to k8sClient.Get with firstKey/secondKey) in Eventually blocks that
assert the Get succeeds, then assert firstRequest.Spec.PlanTier and
secondRequest.Spec.PlanTier afterward (keeping the same APIKeyRequestName and
namespace references).
In `@internal/controller/apikeyrequest_status_controller.go`:
- Around line 149-153: In function enqueueClass
(APIKeyRequestStatusReconciler.enqueueClass) remove the redundant string
conversion around the literal by replacing Name: string("apikeyrequeststatus")
with Name: "apikeyrequeststatus"; this eliminates the no-op conversion while
preserving the exact value returned by enqueueClass.
- Around line 141-147: The controller SetupWithManager in
APIKeyRequestStatusReconciler is watching APIProduct unnecessarily; remove the
Watches(&devportalv1alpha1.APIProduct{},
handler.EnqueueRequestsFromMapFunc(r.enqueueClass)) line so the reconciler only
watches APIKey events (and enqueues via r.enqueueClass) before calling
Named("apikeyrequeststatus").Complete(r), since the reconciler never reads or
inspects APIProduct objects.
In `@make/demo.mk`:
- Line 1: The Makefile's demo-install target is not declared phony, so add a
.PHONY declaration for it; update the Makefile to include a line like ".PHONY:
demo-install" (or add demo-install to an existing .PHONY list) so the
demo-install recipe always runs regardless of any file named "demo-install".
In `@test/e2e/e2e_suite_test.go`:
- Around line 113-123: The cleanup commands in the AfterSuite currently discard
errors from utils.Run when running exec.Command("make", "undeploy"),
exec.Command("make", "uninstall"), and exec.Command("kubectl", "delete", "ns",
namespace); update the code to capture the error return from utils.Run for each
command and log any non-nil error (including the command and namespace) instead
of ignoring it—for example, assign the returned error to a variable and emit it
via GinkgoWriter.Printf or fmt.Printf so failures in utils.Run are visible
during test runs while preserving the cleanup flow.
In `@utils/demo/gamestore.yaml`:
- Around line 54-62: The pod spec for the container named "gamestore" is missing
a securityContext; add a minimal container-level securityContext under the
gamestore entry to meet Pod Security Standards (e.g., set runAsNonRoot: true,
runAsUser to a non-root UID, readOnlyRootFilesystem: true,
allowPrivilegeEscalation: false, and drop all non-needed capabilities). Update
the spec.containers[0] (container name "gamestore") to include these fields so
the manifest can be accepted by clusters enforcing the restricted policy.
🪄 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: 502635c1-d492-4152-a879-cf6017bd2c34
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (66)
.github/workflows/verify.ymlAGENTS.mdCLAUDE.mdCLAUDE.mdMakefilePROJECTREADME.mdapi/v1alpha1/apikey_types.goapi/v1alpha1/apikeyapproval_types.goapi/v1alpha1/apikeyrequest_types.goapi/v1alpha1/zz_generated.deepcopy.gocmd/main.goconfig/crd/bases/devportal.kuadrant.io_apikeyapprovals.yamlconfig/crd/bases/devportal.kuadrant.io_apikeyrequests.yamlconfig/crd/bases/devportal.kuadrant.io_apikeys.yamlconfig/crd/kustomization.yamlconfig/rbac/apikeyapproval_admin_role.yamlconfig/rbac/apikeyapproval_editor_role.yamlconfig/rbac/apikeyapproval_viewer_role.yamlconfig/rbac/apikeyrequest_admin_role.yamlconfig/rbac/apikeyrequest_editor_role.yamlconfig/rbac/apikeyrequest_viewer_role.yamlconfig/rbac/kustomization.yamlconfig/rbac/role.yamlconfig/samples/devportal_v1alpha1_apikeyapproval.yamlconfig/samples/devportal_v1alpha1_apikeyrequest.yamlconfig/samples/kustomization.yamldocs/apikey-approval-workflow.mddocs/references/apikey.mddocs/references/apikeyapproval.mdgo.modinternal/controller/apikey_auto_approval_controller.gointernal/controller/apikey_auto_approval_controller_test.gointernal/controller/apikey_controller.gointernal/controller/apikey_controller_test.gointernal/controller/apikey_secret_controller.gointernal/controller/apikey_secret_controller_test.gointernal/controller/apikey_status_controller.gointernal/controller/apikey_status_controller_test.gointernal/controller/apikeyapproval_controller.gointernal/controller/apikeyapproval_controller_test.gointernal/controller/apikeyapproval_status_controller.gointernal/controller/apikeyapproval_status_controller_test.gointernal/controller/apikeyrequest_controller.gointernal/controller/apikeyrequest_controller_test.gointernal/controller/apikeyrequest_status_controller.gointernal/controller/apikeyrequest_status_controller_test.gointernal/controller/apiproduct_controller.gointernal/controller/apiproduct_controller_authpolicy_discover_test.gointernal/controller/apiproduct_controller_oidc_discover_test.gointernal/controller/apiproduct_controller_test.gointernal/controller/devportal_helper.gointernal/controller/suite_test.gointernal/reconcilers/base_mutator.gointernal/reconcilers/base_reconcilers.gointernal/reconcilers/base_reconcilers_test.gomake/demo.mkmake/development-environment.mkmake/kuadrant-core.mkmake/verify.mktest/e2e/automatic_approval_test.gotest/e2e/e2e_suite_test.gotest/e2e/e2e_test.gotest/utils/utils.goutils/demo/gamestore.yamlutils/demo/kuadrant.yaml
💤 Files with no reviewable changes (4)
- internal/controller/apikey_controller_test.go
- internal/controller/apikey_controller.go
- .github/workflows/verify.yml
- make/verify.mk
Signed-off-by: Eguzki Astiz Lezaun <eastizle@redhat.com>
Signed-off-by: Eguzki Astiz Lezaun <eastizle@redhat.com>
…ted output Signed-off-by: Eguzki Astiz Lezaun <eastizle@redhat.com>
Signed-off-by: Eguzki Astiz Lezaun <eastizle@redhat.com>
Signed-off-by: Eguzki Astiz Lezaun <eastizle@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
internal/reconcilers/base_mutator.go (1)
9-10: ⚡ Quick winAlign
TypedMutateFnparameter order with actual call semantics.
TypedMutateFnis declared as(desired, existing)but invoked as(existing, desired). This is easy to misread and can cause mutator logic to target the wrong object. Please make the type signature consistent with usage.Proposed diff
-// TypedMutateFn is a function which mutates the existing T into it's desired state. -type TypedMutateFn[T client.Object] func(desired, existing T) bool +// TypedMutateFn is a function which mutates the existing T into its desired state. +type TypedMutateFn[T client.Object] func(existing, desired T) boolBased on learnings: “follow the repo convention for parameter naming/order: implement mutators as
someMutator(existing, desired *Type) bool.”Also applies to: 27-27
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/reconcilers/base_mutator.go` around lines 9 - 10, TypedMutateFn is declared with parameters (desired, existing) but callers use (existing, desired); change the generic type alias declaration TypedMutateFn[T client.Object] to func(existing, desired T) bool and update any other similar declarations (e.g., the other signature noted around line 27) and all implementations/call-sites to match that order so implementations follow the repo convention of mutators as someMutator(existing, desired *Type) bool.internal/controller/apikey_secret_controller_test.go (1)
490-573: ⚡ Quick winAdd a case for empty
api_keyvalue (not just missing key).The suite validates missing key, but not the explicit empty-value path handled in
internal/controller/apikey_secret_controller.go(Line 179-185). A small test forapi_key: ""would close that branch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/apikey_secret_controller_test.go` around lines 490 - 573, Add a test variant in the same Context that covers the empty-string value for the consumer secret's "api_key" (not just missing key): create consumerSecret with Data mapping "api_key" to []byte(""), create the APIKey with Approved condition as done with apiKey, then call APIKeySecretReconciler.Reconcile(ctx, reconcile.Request{}) and assert an error occurs containing "consumer secret", "missing" or "empty", "api_key" and "approved APIKey", and finally verify the enforcement secret (enforcementSecretName(apiKey)) is not created; this mirrors the existing test flow but uses an explicit empty api_key value to exercise the branch in APIKeySecretReconciler/Reconcile that handles empty api_key.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@config/crd/bases/devportal.kuadrant.io_apikeys.yaml`:
- Around line 65-69: The CRD schema currently defines the
apiProductRef.namespace property but does not mark it as required; update the
CRD object schema for apiProductRef (the object that currently has "required: -
name") to include "namespace" in the required array so the required list becomes
["name","namespace"], ensuring apiProductRef.namespace is mandatory and
preserving namespace isolation for APIKey/APIKeyRequest resources.
In `@docs/apikey-approval-workflow.md`:
- Around line 40-43: Rename the section heading "Create APIKey Request" to
accurately reflect the resource being created (e.g., "Create APIKey" or "Create
APIKey resource") so it does not get confused with the shadow resource
`APIKeyRequest`; update the heading text in the docs where the command creates
an `APIKey` and ensure any nearby references to `APIKeyRequest` remain unchanged
for clarity.
- Around line 102-105: The markdown fenced code blocks that show command output
(e.g., the block containing "NAME AGE" and "consumer-app-gamestore-apikey
<time>") are missing a language identifier; update those fences to use the
"text" language (add ```text) so markdownlint rule MD040 is satisfied; apply the
same change to the other output block referenced (the second block showing
similar command output).
In `@internal/controller/apikey_secret_controller.go`:
- Around line 109-112: The current code returns immediately on per-APIKey errors
(calls that log with logger.Error(..., "failed to generate desired enforcement
secret", "apiKey", client.ObjectKeyFromObject(apiKey)) then return
ctrl.Result{}, err), which aborts the whole batch; instead, change those
immediate return statements (the spots that return ctrl.Result{}, err after
logger.Error) to log the error and continue processing the remaining apiKey
items (i.e., replace the return with a continue inside the loop), and optionally
accumulate errors in a slice to decide at the end whether to requeue once
(return a single aggregated error or nil) so one malformed/missing-secret APIKey
does not prevent reconciling others.
In `@README.md`:
- Around line 543-549: The README's consumer quick-start omits the required
secret creation step before creating an APIKey; add an explicit prereq
instructing users to create a Secret containing the credentials referenced by
APIKey.spec.secretRef (e.g., create a Kubernetes Secret with the expected keys
and name), then proceed to create the APIKey resource that references the API
owner via APIKey.spec.apiProductRef.namespace and the Secret via
APIKey.spec.secretRef; ensure the new step appears before the "Create an APIKey"
bullet so first-apply succeeds.
---
Nitpick comments:
In `@internal/controller/apikey_secret_controller_test.go`:
- Around line 490-573: Add a test variant in the same Context that covers the
empty-string value for the consumer secret's "api_key" (not just missing key):
create consumerSecret with Data mapping "api_key" to []byte(""), create the
APIKey with Approved condition as done with apiKey, then call
APIKeySecretReconciler.Reconcile(ctx, reconcile.Request{}) and assert an error
occurs containing "consumer secret", "missing" or "empty", "api_key" and
"approved APIKey", and finally verify the enforcement secret
(enforcementSecretName(apiKey)) is not created; this mirrors the existing test
flow but uses an explicit empty api_key value to exercise the branch in
APIKeySecretReconciler/Reconcile that handles empty api_key.
In `@internal/reconcilers/base_mutator.go`:
- Around line 9-10: TypedMutateFn is declared with parameters (desired,
existing) but callers use (existing, desired); change the generic type alias
declaration TypedMutateFn[T client.Object] to func(existing, desired T) bool and
update any other similar declarations (e.g., the other signature noted around
line 27) and all implementations/call-sites to match that order so
implementations follow the repo convention of mutators as someMutator(existing,
desired *Type) bool.
🪄 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: b3f63592-f60a-4c95-8ab2-59fe0ee17ad6
📒 Files selected for processing (13)
README.mdapi/v1alpha1/apikey_types.goconfig/crd/bases/devportal.kuadrant.io_apikeys.yamlconfig/samples/devportal_v1alpha1_apikeyrequest.yamldocs/apikey-approval-workflow.mdinternal/controller/apikey_auto_approval_controller_test.gointernal/controller/apikey_secret_controller.gointernal/controller/apikey_secret_controller_test.gointernal/controller/apikeyapproval_controller_test.gointernal/controller/apikeyrequest_controller.gointernal/controller/apikeyrequest_status_controller.gointernal/reconcilers/base_mutator.gotest/e2e/automatic_approval_test.go
✅ Files skipped from review due to trivial changes (3)
- internal/controller/apikeyapproval_controller_test.go
- internal/controller/apikey_auto_approval_controller_test.go
- test/e2e/automatic_approval_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/controller/apikeyrequest_status_controller.go
- api/v1alpha1/apikey_types.go
- internal/controller/apikeyrequest_controller.go
Signed-off-by: Eguzki Astiz Lezaun <eastizle@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Around line 452-456: Update the manual workflow sentence describing the
initial Pending state so it matches the APIKey Status Fields section: instead of
saying “no approval conditions,” state that the APIKey starts with a Pending
condition (condition type "Pending") whose status is "True" indicating it awaits
approval; reference the "APIKey Status Fields" section and the condition name
"Pending" to ensure wording is consistent.
🪄 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: 47033907-44ab-4f96-a6dd-1c4622069252
📒 Files selected for processing (2)
README.mddocs/apikey-approval-workflow.md
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/apikey-approval-workflow.md
Signed-off-by: Eguzki Astiz Lezaun <eastizle@redhat.com>
10 tasks
R-Lawton
reviewed
May 7, 2026
|
|
||
| // APIKeyRequestName constructs the APIKeyRequest name for a given APIKey | ||
| // Pattern: {apiKeyNamespace}-{apiKeyName} | ||
| func APIKeyRequestName(apiKey *devportalv1alpha1.APIKey) string { |
Contributor
There was a problem hiding this comment.
We have a issue in the console plugin for same apikey names im thinking we add checks here too for same apikey name?
Contributor
Author
There was a problem hiding this comment.
So, what was your thought? Should we check if the APIKeyRequest name is already taken and then report it in the APIKey status? That can easily be done.
Contributor
There was a problem hiding this comment.
yeah that sounds like a good way
Contributor
Author
There was a problem hiding this comment.
Been thinking a bit more about this. I think there is a better approach than the check for already existing name.
Previously, APIKeyRequestName() used a sloppy pattern: {namespace}-{name}, which could cause naming collisions. For example, an APIKey foo-bar/baz and foo/bar-baz would both produce the same APIKeyRequest name foo-bar-baz.
Changes:
- Updated pattern to:
{namespace}-{name}-{hash}(matching the enforcement secret pattern) - The hash is an 8-character SHA256 suffix derived from
{namespace}/{name}, ensuring DNS-1123 compliance
Benefits:
- Collision-free: The hash guarantees a deterministic 1:1 mapping from APIKey to APIKeyRequest
- Idempotent reconciliation: If the APIKeyRequest already exists, the controller simply updates it to sync with the APIKey state - no conflict errors
- Consistency: Same naming approach as enforcement secrets for predictability
wdyt?
Contributor
There was a problem hiding this comment.
ok yes so eliminate the need for the check altogether have the names always be unique sounds like a better idea, good shout.
| return ctrl.Result{}, nil | ||
| } | ||
|
|
||
| func (r *APIKeySecretReconciler) desiredEnforcementSecret(ctx context.Context, apiKey *devportalv1alpha1.APIKey, kuadrantNamespace string) (*corev1.Secret, error) { |
Contributor
There was a problem hiding this comment.
More of a question, have we thought about the scenario where a apiproduct was once manual approval and gets changed to automatic. i was thinking we add a message in the console if they try edit the apiproduct to do this to warn these wont be autoapproved . I dont think we need to worry about autoapproval for once manual keys for this release but would be worth a issue for later wdyt?
Contributor
Author
There was a problem hiding this comment.
pending keys will eventually be approved after the apiproduct gets changed to automatic. So, adding highlighted notification in the console seems appropriate. I can create issue for that.
Contributor
Author
There was a problem hiding this comment.
Signed-off-by: Eguzki Astiz Lezaun <eastizle@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@docs/apikey-approval-workflow.md`:
- Line 8: Update the phrasing in the docs to use the official capitalization by
replacing the phrase "github page" with "GitHub page" in the sentence
referencing the yq install link (the line containing "`yq` command line tool.
See the [github page](https://github.com/mikefarah/yq/#install)`"). Ensure
"GitHub" is capitalized exactly as the brand name wherever that link text
appears.
- Around line 74-88: The example YAML and the kubectl command disagree: the
command uses jsonpath='{.status}' (kubectl get apikey ... -o
jsonpath='{.status}') which prints the status object only, but the docs'
expected output shows a top-level status: wrapper; make them consistent by
either changing the command to output the full resource (remove the .status
jsonpath or use -o yaml) if you want the top-level "status:" shown, or modify
the example YAML to show only the contents of .status (remove the extra
top-level "status:" key) so it matches the jsonpath output.
- Around line 245-275: The denied-message in the kubectl patch for the
apikeyapproval resource (the `kubectl patch apikeyapproval
gamestore-approval-12345 ...` command) does not match the expected status
condition message; update one of them so they are consistent: either change the
patch payload's "message" value to the same format used in the expected APIKey
status condition (include the api-owner identity and suffix ": Denied"), or
adjust the expected YAML under "status.conditions.message" to exactly match "Use
case no longer meets security requirements"; ensure the change references the
apikeyapproval patch command and the APIKey status.conditions.message output so
tests/manual verification align.
🪄 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: 58581a45-45eb-4844-9e0d-f7a24b5e8788
📒 Files selected for processing (2)
README.mddocs/apikey-approval-workflow.md
4 tasks
Signed-off-by: Eguzki Astiz Lezaun <eastizle@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
internal/controller/apikey_status_controller.go (3)
549-552: 💤 Low valueRemove orphan/dead comment above
SetupWithManager.Line 550
// Clear all status condition types (Approved, Denied, Failed)doesn't describe theSetupWithManagerfunction below it and appears to be a leftover from refactored code. Recommend deleting to avoid reader confusion.🛠️ Proposed fix
-// Clear all status condition types (Approved, Denied, Failed) - // SetupWithManager sets up the controller with the Manager. func (r *APIKeyStatusReconciler) SetupWithManager(mgr ctrl.Manager) error {🤖 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 549 - 552, Remove the orphan comment above SetupWithManager: delete the standalone comment line "// Clear all status condition types (Approved, Denied, Failed)" that sits immediately before the SetupWithManager function in apikey_status_controller.go so the comment no longer misleads readers; leave the SetupWithManager function and its real comments unchanged.
45-49: ⚡ Quick winStale doc comment: wrong reconciler name and resource.
The doc comment on Line 45 still claims
APIProductReconciler reconciles a APIProduct object, but the type below isAPIKeyStatusReconcilerand it reconcilesAPIKeystatus. Please update to avoid confusion in generated docs and IDE hovers.🛠️ Proposed fix
-// APIProductReconciler reconciles a APIProduct object +// APIKeyStatusReconciler reconciles APIKey status fields and conditions. type APIKeyStatusReconciler struct {🤖 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 45 - 49, The doc comment above the APIKeyStatusReconciler type is stale and mentions APIProduct; update the comment to correctly describe the reconciler and resource—e.g., change the comment to indicate that APIKeyStatusReconciler reconciles APIKey status (or similar wording like "APIKeyStatusReconciler reconciles the status of APIKey objects") so generated docs and IDE hovers match the APIKeyStatusReconciler type.
121-125: ⚡ Quick winLog message references the wrong resource.
The skip-update log on Line 123 says
"apiproduct status unchanged, skipping update", but this controller reconcilesAPIKeystatus. Misleading when grepping logs.🛠️ Proposed fix
if equalStatus && apiKey.Generation == apiKey.Status.ObservedGeneration { - logger.V(1).Info("apiproduct status unchanged, skipping update") + logger.V(1).Info("apikey status unchanged, skipping update") return 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 121 - 125, The log message incorrectly mentions "apiproduct" while the code is reconciling an APIKey; update the logger call in the block that checks equality.Semantic.DeepEqual(newStatus, &apiKey.Status) and apiKey.Generation == apiKey.Status.ObservedGeneration so it logs a correct, specific message like "apikey status unchanged, skipping update" (or similar) using the existing logger.V(1).Info invocation and keep the rest of the logic unchanged.
🤖 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 `@internal/controller/apikey_status_controller.go`:
- Around line 348-366: In calculateDeniedCondition, the code checks
approval.Spec.Reason but appends approval.Spec.Message, causing an empty ": "
when Message is empty; update the conditional to check approval.Spec.Message !=
"" (mirroring calculateApprovedCondition) so the denial message only appends the
message when approval.Spec.Message is present (ensure you edit the check in
calculateDeniedCondition that references approval.Spec.Reason and leave the
interpolation using approval.Spec.Message).
---
Nitpick comments:
In `@internal/controller/apikey_status_controller.go`:
- Around line 549-552: Remove the orphan comment above SetupWithManager: delete
the standalone comment line "// Clear all status condition types (Approved,
Denied, Failed)" that sits immediately before the SetupWithManager function in
apikey_status_controller.go so the comment no longer misleads readers; leave the
SetupWithManager function and its real comments unchanged.
- Around line 45-49: The doc comment above the APIKeyStatusReconciler type is
stale and mentions APIProduct; update the comment to correctly describe the
reconciler and resource—e.g., change the comment to indicate that
APIKeyStatusReconciler reconciles APIKey status (or similar wording like
"APIKeyStatusReconciler reconciles the status of APIKey objects") so generated
docs and IDE hovers match the APIKeyStatusReconciler type.
- Around line 121-125: The log message incorrectly mentions "apiproduct" while
the code is reconciling an APIKey; update the logger call in the block that
checks equality.Semantic.DeepEqual(newStatus, &apiKey.Status) and
apiKey.Generation == apiKey.Status.ObservedGeneration so it logs a correct,
specific message like "apikey status unchanged, skipping update" (or similar)
using the existing logger.V(1).Info invocation and keep the rest of the logic
unchanged.
🪄 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: cbcc5153-20cb-4c34-acbf-7a0797b95efd
📒 Files selected for processing (3)
docs/apikey-approval-workflow.mdinternal/controller/apikey_status_controller.gointernal/controller/devportal_helper.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/controller/devportal_helper.go
Signed-off-by: Eguzki Astiz Lezaun <eastizle@redhat.com>
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.
Summary by CodeRabbit
New Features
Documentation
Testing & Infrastructure