Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughA new Kubernetes custom resource Changes
Possibly related issues
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
eguzki
force-pushed
the
40-implement-apikeyrequest-crd-and-shadow-resource-controller
branch
from
33e2285 to
20af371
Compare
eguzki
force-pushed
the
APIKeyApproval-crd
branch
from
3be1825 to
d3f8420
Compare
eguzki
force-pushed
the
40-implement-apikeyrequest-crd-and-shadow-resource-controller
branch
3 times, most recently
from
3dc2081 to
a540077
Compare
eguzki
force-pushed
the
APIKeyApproval-crd
branch
from
d3f8420 to
dcee43e
Compare
Signed-off-by: Eguzki Astiz Lezaun <eastizle@redhat.com>
eguzki
force-pushed
the
APIKeyApproval-crd
branch
from
dcee43e to
f8701e7
Compare
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
![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
🧹 Nitpick comments (3)
config/samples/devportal_v1alpha1_apikeyapproval.yaml (1)
3-8: Add an explicit namespace in the sample manifest.To reflect the intended namespace-scoped approval flow, include
metadata.namespaceso the example demonstrates correct placement ofAPIKeyApprovalalongside related resources.Based on learnings: APIKeyApproval namespace must match the APIProduct namespace and RBAC is namespace-based.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/samples/devportal_v1alpha1_apikeyapproval.yaml` around lines 3 - 8, Add an explicit metadata.namespace entry to the sample manifest (the resource with metadata.name: apikeyapproval-sample) so the APIKeyApproval example is namespace-scoped; set metadata.namespace to the intended namespace (e.g., developer-portal) and ensure the APIKeyApproval’s namespace matches the APIProduct namespace and the namespace used in your RBAC rules so the approval flow and permissions work as shown.api/v1alpha1/apikeyapproval_types.go (2)
54-58:APIKeyApprovalStatusis an empty placeholder — addconditionsandobservedGeneration.With the
statussubresource enabled but no fields defined, the controller has no durable way to record whether the approval has been processed, propagated to the referencedAPIKey, or failed. This also diverges from the conditions-based lifecycle documented inAGENTS.mdline 27 and from theAPIKeyRequestStatuspattern already used in this package (zz_generated.deepcopy.golines 289-298, which carries[]metav1.Condition).♻️ Suggested status shape
// APIKeyApprovalStatus defines the observed state of APIKeyApproval. type APIKeyApprovalStatus struct { - // INSERT ADDITIONAL STATUS FIELD - define observed state of cluster - // Important: Run "make" to regenerate code after modifying this file + // ObservedGeneration reflects the generation most recently reconciled. + // +optional + ObservedGeneration int64 `json:"observedGeneration,omitempty"` + + // Conditions represent the latest observations of the approval's state + // (e.g. Processed, Propagated, Failed). + // +optional + // +patchMergeKey=type + // +patchStrategy=merge + // +listType=map + // +listMapKey=type + Conditions []metav1.Condition `json:"conditions,omitempty" patchStrategy:"merge" patchMergeKey:"type"` }Remember to run
make manifests generateafter this change.As per coding guidelines: "Run
make manifests generateafter modifying API types to regenerate CRDs and DeepCopy methods".🤖 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 54 - 58, APIKeyApprovalStatus is currently empty; add a conditions slice and observedGeneration to persist status: add Conditions []metav1.Condition `json:"conditions,omitempty"` (with +optional comment) and ObservedGeneration int64 `json:"observedGeneration,omitempty"` (with +optional) to the APIKeyApprovalStatus struct so the controller can record lifecycle state; reference metav1.Condition for the Conditions type and ensure matching json tags and optional comments, then run "make manifests generate" to regenerate CRDs and deepcopy code (zz_generated.deepcopy.go).
23-25: Consider renaming for same-namespace convention.
APIKeyRequestReferenceholds onlyNameand is implicitly same-namespace. The package already usesLocalAPIProductReference(seeapi/v1alpha1/zz_generated.deepcopy.goline 620) to encode that semantic in the name. Renaming toLocalAPIKeyRequestReferencewould make the scoping rule self-documenting and consistent with the existing reference types.🤖 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 23 - 25, Rename the struct type APIKeyRequestReference to LocalAPIKeyRequestReference to make same-namespace semantics explicit: change the type declaration name (type APIKeyRequestReference -> type LocalAPIKeyRequestReference) while keeping the existing field Name and json tag, then update all usages/imports that reference APIKeyRequestReference (e.g., in other types, method signatures, and tests) to the new LocalAPIKeyRequestReference identifier and regenerate/update any generated files (deepcopy/zz_generated) so the new name is reflected consistently.
🤖 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/apikeyapproval_types.go`:
- Around line 37-43: The ReviewedBy and ReviewedAt spec fields are
client-supplied and spoofable; move them out of spec and into status (e.g., add
ReviewedBy and ReviewedAt to APIKeyApprovalStatus and remove the Required tags
from the spec) and have the controller or an admission webhook populate them
from the authenticated admission userInfo and
metav1.ObjectMeta.CreationTimestamp (or if you must keep optional informational
fields in spec, make them optional and implement a validating webhook that
asserts spec.ReviewedBy == admission.userInfo.username and spec.ReviewedAt is
within an allowed skew of ObjectMeta.CreationTimestamp). Update references to
ReviewedBy/ReviewedAt in the controller, webhook, and CRD markers to use the new
status fields and ensure the admission/controller writes the trusted values
rather than relying on client input.
In `@config/rbac/apikeyapproval_admin_role.yaml`:
- Around line 20-21: The RBAC role currently uses a wildcard verb entry ('verbs:
- '*'') which grants excessive permissions; update the
apikeyapproval_admin_role.yaml to replace the wildcard with an explicit,
least-privilege list (e.g., get, list, watch, create, update, patch, delete —
only include the verbs actually required by the apikey approval admin helper)
under the verbs key and remove '*' so the role grants only the necessary
operations.
In `@PROJECT`:
- Line 43: PROJECT marks APIKeyApproval as controller-managed but cmd/main.go
lacks wiring; either add the reconciler setup call or change the PROJECT entry
to controller: false. To fix, open cmd/main.go and register the reconciler by
creating/using APIKeyApprovalReconciler and calling its SetupWithManager(mgr)
(consistent with how APIProductReconciler, APIKeyReconciler,
APIKeyRequestReconciler, and APIKeyRequestStatusReconciler are registered), or
edit the PROJECT file line for APIKeyApproval to set controller: false so
metadata matches the missing SetupWithManager wiring.
---
Nitpick comments:
In `@api/v1alpha1/apikeyapproval_types.go`:
- Around line 54-58: APIKeyApprovalStatus is currently empty; add a conditions
slice and observedGeneration to persist status: add Conditions
[]metav1.Condition `json:"conditions,omitempty"` (with +optional comment) and
ObservedGeneration int64 `json:"observedGeneration,omitempty"` (with +optional)
to the APIKeyApprovalStatus struct so the controller can record lifecycle state;
reference metav1.Condition for the Conditions type and ensure matching json tags
and optional comments, then run "make manifests generate" to regenerate CRDs and
deepcopy code (zz_generated.deepcopy.go).
- Around line 23-25: Rename the struct type APIKeyRequestReference to
LocalAPIKeyRequestReference to make same-namespace semantics explicit: change
the type declaration name (type APIKeyRequestReference -> type
LocalAPIKeyRequestReference) while keeping the existing field Name and json tag,
then update all usages/imports that reference APIKeyRequestReference (e.g., in
other types, method signatures, and tests) to the new
LocalAPIKeyRequestReference identifier and regenerate/update any generated files
(deepcopy/zz_generated) so the new name is reflected consistently.
In `@config/samples/devportal_v1alpha1_apikeyapproval.yaml`:
- Around line 3-8: Add an explicit metadata.namespace entry to the sample
manifest (the resource with metadata.name: apikeyapproval-sample) so the
APIKeyApproval example is namespace-scoped; set metadata.namespace to the
intended namespace (e.g., developer-portal) and ensure the APIKeyApproval’s
namespace matches the APIProduct namespace and the namespace used in your RBAC
rules so the approval flow and permissions work as shown.
🪄 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: cc30c720-3f29-4117-9a49-2472e2c7c3fd
📒 Files selected for processing (13)
AGENTS.mdPROJECTREADME.mdapi/v1alpha1/apikeyapproval_types.goapi/v1alpha1/zz_generated.deepcopy.goconfig/crd/bases/devportal.kuadrant.io_apikeyapprovals.yamlconfig/crd/kustomization.yamlconfig/rbac/apikeyapproval_admin_role.yamlconfig/rbac/apikeyapproval_editor_role.yamlconfig/rbac/apikeyapproval_viewer_role.yamlconfig/rbac/kustomization.yamlconfig/samples/devportal_v1alpha1_apikeyapproval.yamlconfig/samples/kustomization.yaml
|
I think it would be helpful to add a api reference for this crd in https://github.com/Kuadrant/developer-portal-controller/tree/main/docs/references |
| } | ||
|
|
||
| // APIKeyApprovalSpec defines the desired state of APIKeyApproval. | ||
| type APIKeyApprovalSpec struct { |
There was a problem hiding this comment.
I won't be blocking this, but I'd suggest we simplify the amount of objects around the API Key feature to the APIKey in the consumer's namespace and just one object in the owner's namespace, something like APIKeyManagement/APIKeyRequest/APIKeyControl/etc as I commented in #46 (comment)
There was a problem hiding this comment.
Regarding the APIKeyRequest CRD, without it, the owner would need permission out of their own namespace to read APIKey objects. That is considered a security issue.
Regarding the APIKeyApproval CRD, that type represents a registry of decisions made by the owner in the owner's namespace. The decision is made by the owner that does not have permission over APIKey resources in the consumer namespace.
Happy to discuss all about it. The design document is the source of this implementation. If this implementation needs to be changed, let's go first to the design doc
Regarding the linters failing. That's ok, for now (targeting feature branch). I had to comment some code out and that left some functions not being called from anywhere in the code. Hence the complain. |
️✅ There are no secrets present in this pull request anymore.If these secrets were true positive and are still valid, we highly recommend you to revoke them. 🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request. |
Signed-off-by: Eguzki Astiz Lezaun <eastizle@redhat.com>
eguzki
force-pushed
the
APIKeyApproval-crd
branch
from
70415df to
0a19710
Compare
|
@R-Lawton added api reference for this crd. BTW I realized the apikey api reference was outdated, so fixed it as well. |
|
|
||
| APIKeyApproval **must** reference an existing APIKeyRequest via `apiKeyRequestRef`. The APIKeyRequest represents a developer's request for API access and contains information about the requested APIProduct, plan tier, use case, and requester details. | ||
|
|
||
| When an APIKeyApproval is created with `approved: true`, the controller processes the approval and updates the corresponding APIKey resource to transition it from `Pending` to `Approved` phase, triggering the creation of the API key secret. |
There was a problem hiding this comment.
Do we want to use the term condition rather then phase since we removed it from the crd
|
|
||
| When an APIKeyApproval is created with `approved: true`, the controller processes the approval and updates the corresponding APIKey resource to transition it from `Pending` to `Approved` phase, triggering the creation of the API key secret. | ||
|
|
||
| When an APIKeyApproval is created with `approved: false`, the controller updates the corresponding APIKey resource to transition it to `Rejected` phase, and no secret is created. |
There was a problem hiding this comment.
I think rejected should be denied?
There was a problem hiding this comment.
i think its used in a few places
Signed-off-by: Eguzki Astiz Lezaun <eastizle@redhat.com>
Signed-off-by: Eguzki Astiz Lezaun <eastizle@redhat.com>
3 participants
Depends on #44
Part of #39
Summary by CodeRabbit
Release Notes
New Features
Documentation