[cozystack-api] Implement TenantNamespace resource - #1267
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThis update introduces a new "core" API group ( Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CozyAPI
participant KubernetesAPI
participant AuthzAPI
User->>CozyAPI: GET /apis/core.cozystack.io/v1alpha1/tenantnamespaces
CozyAPI->>KubernetesAPI: List Namespaces
KubernetesAPI-->>CozyAPI: NamespaceList
loop For each tenant- namespace
CozyAPI->>AuthzAPI: SubjectAccessReview (can user get workloadmonitors in ns?)
AuthzAPI-->>CozyAPI: Allowed/Denied
end
CozyAPI-->>User: TenantNamespaceList (filtered by access)
sequenceDiagram
participant User
participant CozyAPI
participant KubernetesAPI
User->>CozyAPI: GET/POST/PATCH /apis/core.cozystack.io/v1alpha1/namespaces/{ns}/tenantsecrets
CozyAPI->>KubernetesAPI: Corresponding Secret operation (with label filter)
KubernetesAPI-->>CozyAPI: Secret/SecretList/WatchEvent
CozyAPI-->>User: TenantSecret/TenantSecretList/Table
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (25)
✅ Files skipped from review due to trivial changes (4)
🚧 Files skipped from review as they are similar to previous changes (20)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (5)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Summary of Changes
Hello Andrei Kvapil (@kvaps), I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request introduces a new TenantNamespace resource to the cozystack-api, providing a dedicated read-only API view for Kubernetes Namespaces that are prefixed with 'tenant-'. Concurrently, it refines the API server's type registration mechanism and enhances the dynamic Application resource handling by enabling OpenAPI schema-based defaulting for application specifications.
Highlights
- New TenantNamespace Resource: I've implemented a new
TenantNamespaceAPI resource (apps.cozystack.io/v1alpha1/tenantnamespaces). This resource provides a read-only view of Kubernetes Namespaces whose names begin with the 'tenant-' prefix, allowing Cozystack to expose tenant-specific namespaces through its own API. - API Server Integration: The new
TenantNamespaceresource has been integrated into thecozystack-apiserver, making it discoverable and accessible. This includes adding its OpenAPI definitions and registering its REST storage. - Static vs. Dynamic Type Registration: I've refactored the API registration process to explicitly distinguish and register 'static' compile-time resources (like
TenantNamespace) before 'dynamic' runtime-configuredApplicationresources. This improves clarity and organization of API type registration. - Dynamic Application Spec Defaulting: The
Applicationresource handling has been enhanced to support defaulting of itsspecfield based on an OpenAPI schema provided in the resource configuration. This improves the robustness and usability of dynamically defined application types by automatically applying default values.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | Gemini (@gemini-code-assist) | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on Gemini (@gemini-code-assist) comments or fill out our survey to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces a new cluster-scoped, read-only resource TenantNamespace which represents Kubernetes namespaces with a tenant- prefix. The implementation includes the API type definition, REST storage, and registration with the API server. The PR also refactors the type registration logic to distinguish between static and dynamic resources and adds schema-based defaulting for Application resources. I've identified a duplicated block of code and a case of an ignored error that should be addressed for improved reliability and maintainability.
| for _, resConfig := range c.ResourceConfig.Resources { | ||
| storage := applicationstorage.NewREST(dynamicClient, &resConfig) | ||
| v1alpha1storage[resConfig.Application.Plural] = appsregistry.RESTInPeace(storage) |
There was a problem hiding this comment.
This for loop appears to be a duplicate of the preceding one (lines 134-137). It iterates over the same c.ResourceConfig.Resources and registers the same storage handlers, potentially overwriting the intended configuration. This redundant code should be removed.
| if err := json.Unmarshal([]byte(raw), &js); err == nil { | ||
| specSchema, _ = structuralschema.NewStructural(&js) | ||
| } |
There was a problem hiding this comment.
The error returned by structuralschema.NewStructural is being ignored. This can lead to silent failures if an invalid OpenAPI schema is provided in the configuration. The resource would be created without any spec defaulting, and it would be difficult to debug. The error should be checked and logged to provide better visibility into configuration issues.
var structuralErr error
specSchema, structuralErr = structuralschema.NewStructural(&js)
if structuralErr != nil {
klog.Warningf("Invalid OpenAPI schema for kind %q: %v", config.Application.Kind, structuralErr)
specSchema = nil
}There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
pkg/cmd/server/start.go (1)
171-174: Minor: replace smart quotes and keep comments ASCII-clean.The comment uses the typographic ’ which can trip some linters.
-// Register *run-time* resources (from the user’s config file). +// Register *run-time* resources (from the user's config file).pkg/apiserver/apiserver.go (1)
37-38: Import grouping nit.
tenantnamespacestoragebelongs next to other internal imports; rungoimportsto group correctly.pkg/registry/registry.go (1)
24-33: Update docstring to match implementation.The wrapper no longer exposes
GroupVersionKind; adjust the comment to avoid misleading future readers.pkg/registry/apps/application/rest.go (1)
91-97: Consider logging schema parsing errors.The schema parsing silently ignores errors, which could make debugging configuration issues difficult.
var specSchema *structuralschema.Structural if raw := strings.TrimSpace(config.Application.OpenAPISchema); raw != "" { var js internalapiext.JSONSchemaProps - if err := json.Unmarshal([]byte(raw), &js); err == nil { - specSchema, _ = structuralschema.NewStructural(&js) + if err := json.Unmarshal([]byte(raw), &js); err != nil { + klog.Warningf("Failed to parse OpenAPI schema for %s: %v", config.Application.Kind, err) + } else if s, err := structuralschema.NewStructural(&js); err != nil { + klog.Warningf("Failed to create structural schema for %s: %v", config.Application.Kind, err) + } else { + specSchema = s } }pkg/registry/apps/tenantnamespace/rest.go (1)
199-199: Remove non-English comment.The comment "← важно" (Russian) should be removed or replaced with an English comment if necessary.
- Object: runtime.RawExtension{Object: o}, // ← важно + Object: runtime.RawExtension{Object: o},
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pkg/generated/openapi/zz_generated.openapi.gois excluded by!**/generated/**
📒 Files selected for processing (9)
packages/system/cozystack-api/values.yaml(1 hunks)pkg/apis/apps/v1alpha1/register.go(3 hunks)pkg/apis/apps/v1alpha1/tenantnamespace_types.go(1 hunks)pkg/apis/apps/v1alpha1/zz_generated.deepcopy.go(1 hunks)pkg/apiserver/apiserver.go(2 hunks)pkg/cmd/server/start.go(1 hunks)pkg/registry/apps/application/rest.go(6 hunks)pkg/registry/apps/tenantnamespace/rest.go(1 hunks)pkg/registry/registry.go(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
packages/system/cozystack-api/values.yaml (3)
Learnt from: NickVolynkin
PR: #1117
File: packages/apps/mysql/Makefile:8-8
Timestamp: 2025-06-26T04:29:24.830Z
Learning: The cozystack project uses yq v4+ on their CI runner, so yq v4 syntax (-o json --indent 4) is compatible and version checks are not needed.
Learnt from: NickVolynkin
PR: #1120
File: packages/apps/ferretdb/README.md:35-37
Timestamp: 2025-07-02T09:58:11.406Z
Learning: In the cozystack repository, the maintainer NickVolynkin prefers to keep realistic-looking example credentials in README documentation rather than using generic placeholders like <ACCESS_KEY>, even though they are just examples and not real secrets.
Learnt from: lllamnyp
PR: #1130
File: hack/e2e-apps/kubernetes.bats:101-101
Timestamp: 2025-07-11T06:11:25.438Z
Learning: In cozystack, the plural form for the Kubernetes custom resource is kuberneteses.apps.cozystack.io, not kubernetes.apps.cozystack.io. This is defined in the API schema even though it's not grammatically perfect.
pkg/apis/apps/v1alpha1/register.go (1)
Learnt from: lllamnyp
PR: #1130
File: hack/e2e-apps/kubernetes.bats:101-101
Timestamp: 2025-07-11T06:11:25.438Z
Learning: In cozystack, the plural form for the Kubernetes custom resource is kuberneteses.apps.cozystack.io, not kubernetes.apps.cozystack.io. This is defined in the API schema even though it's not grammatically perfect.
pkg/registry/apps/application/rest.go (1)
Learnt from: lllamnyp
PR: #1130
File: hack/e2e-apps/kubernetes.bats:101-101
Timestamp: 2025-07-11T06:11:25.438Z
Learning: In cozystack, the plural form for the Kubernetes custom resource is kuberneteses.apps.cozystack.io, not kubernetes.apps.cozystack.io. This is defined in the API schema even though it's not grammatically perfect.
🧬 Code Graph Analysis (4)
pkg/cmd/server/start.go (2)
pkg/apis/apps/v1alpha1/register.go (1)
RegisterStaticTypes(57-64)pkg/apiserver/apiserver.go (1)
Scheme(42-42)
pkg/apis/apps/v1alpha1/register.go (4)
pkg/config/config.go (1)
Resource(32-35)pkg/apis/apps/v1alpha1/tenantnamespace_types.go (2)
TenantNamespace(18-21)TenantNamespaceList(26-30)api/v1alpha1/workload_types.go (1)
init(68-70)pkg/apis/apps/v1alpha1/zz_generated.conversion.go (1)
RegisterConversions(34-36)
pkg/registry/apps/application/rest.go (3)
pkg/registry/apps/tenantnamespace/rest.go (2)
NewREST(54-63)REST(49-52)pkg/config/config.go (1)
Resource(32-35)pkg/apis/apps/v1alpha1/application_types.go (1)
Application(56-64)
pkg/registry/apps/tenantnamespace/rest.go (3)
pkg/registry/apps/application/rest.go (2)
REST(79-87)NewREST(90-115)pkg/apis/apps/v1alpha1/register.go (2)
GroupName(19-19)Resource(48-50)pkg/apis/apps/v1alpha1/tenantnamespace_types.go (2)
TenantNamespace(18-21)TenantNamespaceList(26-30)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
🔇 Additional comments (14)
pkg/apis/apps/v1alpha1/zz_generated.deepcopy.go (1)
118-175: LGTM! Autogenerated deepcopy methods follow the correct pattern.The deepcopy implementations for
TenantNamespaceandTenantNamespaceListare properly generated and consistent with the existing patterns in the file.pkg/apis/apps/v1alpha1/register.go (4)
1-2: Copyright header properly updated.The SPDX license identifier and copyright year are correctly formatted.
14-54: Excellent code organization with clear section comments.The added section comments and inline documentation significantly improve code readability and maintainability.
56-64: Well-implemented static type registration function.The
RegisterStaticTypesfunction properly registers the compile-time known TenantNamespace types with appropriate logging.
66-83: Clean refactoring of dynamic type registration.The updated comments and consistent logging level improve the function's clarity.
pkg/registry/apps/application/rest.go (2)
936-938: Schema defaulting implementation looks good.The error handling allows the conversion to proceed even if defaulting fails, which is appropriate for backward compatibility.
1193-1213: Well-implemented spec defaulting logic.The method properly handles nil specs and applies schema-based defaults. The implementation is defensive and follows best practices.
pkg/registry/apps/tenantnamespace/rest.go (7)
1-35: Well-structured file with clear constants and documentation.The file header clearly describes the purpose, and the constants are appropriately defined.
48-81: Clean REST storage implementation with proper interface satisfaction.The struct definition and helper methods are correctly implemented for a cluster-scoped, read-only resource.
87-130: Efficient List implementation with proper filtering.The method correctly filters namespaces by the "tenant-" prefix and constructs appropriate TenantNamespace objects.
132-160: Well-implemented Get method with appropriate access control.The prefix check before fetching prevents unauthorized access to non-tenant namespaces.
166-188: Efficient Watch implementation using built-in filtering.Good use of
watch.Filterto efficiently filter events at the watch level.
194-242: Comprehensive table conversion with good type handling.The implementation handles all expected object types and provides appropriate table output.
254-268: Standard error type implementation.The custom error type correctly implements the Status() method for proper API error responses.
| @@ -1,2 +1,2 @@ | |||
| cozystackAPI: | |||
| image: ghcr.io/cozystack/cozystack/cozystack-api:v0.34.0@sha256:dad88c168719dcaa0b2e6bab352a90f44f4e23c58f525f96841cfce357920bdf | |||
| image: ghcr.io/cozystack/cozystack/cozystack-api:latest@sha256:c4621f51887fc2693d459524dcc45b9b7fdea833b359395566a2e480ee2445e1 | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid the latest tag for released charts.
Even with the digest pinned, the latest tag obscures which version is deployed and complicates roll-backs. Prefer an explicit version tag (e.g. v0.35.0) and bump appVersion accordingly.
🤖 Prompt for AI Agents
In packages/system/cozystack-api/values.yaml at line 2, replace the 'latest' tag
in the image reference with a specific version tag such as 'v0.35.0' to clearly
indicate the deployed version. Also, update the appVersion field in the chart
metadata to match this explicit version. This change improves clarity on the
deployed version and facilitates easier rollbacks.
| // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object | ||
|
|
||
| // TenantNamespace is a thin wrapper around ObjectMeta. It has no spec/status | ||
| // because it merely reflects an existing Namespace object. | ||
| type TenantNamespace struct { | ||
| metav1.TypeMeta `json:",inline"` | ||
| metav1.ObjectMeta `json:"metadata,omitempty"` | ||
| } | ||
|
|
||
| // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object | ||
|
|
||
| // TenantNamespaceList is the list variant for TenantNamespace. | ||
| type TenantNamespaceList struct { | ||
| metav1.TypeMeta `json:",inline"` | ||
| metav1.ListMeta `json:"metadata,omitempty"` | ||
| Items []TenantNamespace `json:"items"` | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add kubebuilder markers so controller-gen can emit a valid CRD.
-// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object
+// +kubebuilder:object:root=true
+// +kubebuilder:resource:scope=Cluster
+// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.ObjectApply similarly to the List type.
Without these markers make generate will skip CRD output.
🤖 Prompt for AI Agents
In pkg/apis/apps/v1alpha1/tenantnamespace_types.go around lines 14 to 30, the
TenantNamespace and TenantNamespaceList types lack kubebuilder markers needed
for controller-gen to generate valid CRDs. Add the appropriate kubebuilder
markers above both the TenantNamespace and TenantNamespaceList type definitions
to ensure make generate includes CRD output. This typically involves adding
markers like +kubebuilder:object:root=true and other relevant annotations for
both types.
de078a1 to
3e40869
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
pkg/apiserver/apiserver.go (1)
140-143: Remove duplicate resource registration loop.This loop duplicates the registration logic from lines 135-138.
🧹 Nitpick comments (5)
pkg/apis/core/v1alpha1/types.go (1)
1-2: Consider using consistent license headers across the codebase.This file uses SPDX license identifier while other files use the full Apache 2.0 header. Consider standardizing the license header format.
pkg/registry/core/tenantnamespace/rest.go (4)
87-87: Consider implementing ListOptions support for better scalability.The
ListOptionsparameter is currently ignored, which means pagination, field selectors, and label selectors from clients won't be honored. For large clusters with many namespaces, this could impact performance.Consider converting the internal ListOptions to metav1.ListOptions to support pagination and filtering:
-func (r *REST) List(ctx context.Context, _ *metainternal.ListOptions) (runtime.Object, error) { +func (r *REST) List(ctx context.Context, opts *metainternal.ListOptions) (runtime.Object, error) { klog.V(6).Info("Listing tenant namespaces") + listOpts := metav1.ListOptions{} + if opts != nil { + if opts.Continue != nil { + listOpts.Continue = *opts.Continue + } + if opts.Limit != nil { + listOpts.Limit = *opts.Limit + } + // Add more field conversions as needed + } + nsList, err := r.dynamic.Resource(schema.GroupVersionResource{ Group: coreNSGroup, Version: coreNSVersion, Resource: coreNSRes, - }).List(ctx, metav1.ListOptions{}) + }).List(ctx, listOpts)
171-174: Watch implementation doesn't honor all ListOptions fields.Similar to the List method, the Watch implementation only uses ResourceVersion from ListOptions. Label selectors, field selectors, and other options are ignored.
Consider converting more ListOptions fields:
}).Watch(ctx, metav1.ListOptions{ ResourceVersion: opts.ResourceVersion, Watch: true, + LabelSelector: opts.LabelSelector, + FieldSelector: opts.FieldSelector, })
199-199: Use English for code comments.The comment contains non-English text. Please use English for consistency.
- Object: runtime.RawExtension{Object: o}, // ← важно + Object: runtime.RawExtension{Object: o}, // important for kubectl
113-127: Consider copying additional ObjectMeta fields for completeness.The current implementation copies basic metadata fields but omits some that might be relevant for tenant namespace management:
DeletionTimestamp: Important for showing deletion statusFinalizers: Might be relevant for tenant cleanup coordinationOwnerReferences: Could represent tenant ownership relationshipsAdd the missing fields to both List and Get methods:
ObjectMeta: metav1.ObjectMeta{ Name: u.GetName(), UID: u.GetUID(), ResourceVersion: u.GetResourceVersion(), CreationTimestamp: u.GetCreationTimestamp(), + DeletionTimestamp: u.GetDeletionTimestamp(), Labels: u.GetLabels(), Annotations: u.GetAnnotations(), + Finalizers: u.GetFinalizers(), + OwnerReferences: u.GetOwnerReferences(), },Also applies to: 146-159
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pkg/generated/openapi/zz_generated.openapi.gois excluded by!**/generated/**
📒 Files selected for processing (21)
cmd/cozystack-api/main.go(1 hunks)packages/system/cozystack-api/templates/apiservice.yaml(1 hunks)packages/system/cozystack-api/values.yaml(1 hunks)pkg/apis/apps/v1alpha1/register.go(3 hunks)pkg/apis/core/fuzzer/fuzzer.go(1 hunks)pkg/apis/core/install/install.go(1 hunks)pkg/apis/core/install/roundtrip_test.go(1 hunks)pkg/apis/core/register.go(1 hunks)pkg/apis/core/v1alpha1/doc.go(1 hunks)pkg/apis/core/v1alpha1/register.go(1 hunks)pkg/apis/core/v1alpha1/types.go(1 hunks)pkg/apis/core/v1alpha1/zz_generated.conversion.go(1 hunks)pkg/apis/core/v1alpha1/zz_generated.deepcopy.go(1 hunks)pkg/apis/core/v1alpha1/zz_generated.defaults.go(1 hunks)pkg/apis/core/validation/validation.go(1 hunks)pkg/apiserver/apiserver.go(5 hunks)pkg/apiserver/scheme_test.go(1 hunks)pkg/cmd/server/start.go(14 hunks)pkg/cmd/server/start_test.go(2 hunks)pkg/registry/core/tenantnamespace/rest.go(1 hunks)pkg/registry/registry.go(1 hunks)
✅ Files skipped from review due to trivial changes (11)
- packages/system/cozystack-api/values.yaml
- pkg/apiserver/scheme_test.go
- cmd/cozystack-api/main.go
- pkg/apis/core/register.go
- pkg/cmd/server/start_test.go
- pkg/apis/core/v1alpha1/doc.go
- pkg/apis/core/v1alpha1/zz_generated.conversion.go
- pkg/apis/core/install/install.go
- packages/system/cozystack-api/templates/apiservice.yaml
- pkg/apis/core/v1alpha1/zz_generated.deepcopy.go
- pkg/apis/core/v1alpha1/zz_generated.defaults.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/apis/apps/v1alpha1/register.go
- pkg/registry/registry.go
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: lllamnyp
PR: cozystack/cozystack#1130
File: hack/e2e-apps/kubernetes.bats:101-101
Timestamp: 2025-07-11T06:11:25.438Z
Learning: In cozystack, the plural form for the Kubernetes custom resource is `kuberneteses.apps.cozystack.io`, not `kubernetes.apps.cozystack.io`. This is defined in the API schema even though it's not grammatically perfect.
Learnt from: NickVolynkin
PR: cozystack/cozystack#1120
File: packages/apps/clickhouse/README.md:60-67
Timestamp: 2025-07-03T05:54:51.264Z
Learning: The `cozy-lib.resources.sanitize` function in packages/library/cozy-lib/templates/_resources.tpl supports both standard Kubernetes resource format (with limits:/requests: sections) and flat format (direct resource specifications). The flat format takes priority over nested values. CozyStack apps include cozy-lib as a chart dependency through symlinks in packages/apps/*/charts/cozy-lib directories.
📚 Learning: in cozystack, the plural form for the kubernetes custom resource is `kuberneteses.apps.cozystack.io`...
Learnt from: lllamnyp
PR: cozystack/cozystack#1130
File: hack/e2e-apps/kubernetes.bats:101-101
Timestamp: 2025-07-11T06:11:25.438Z
Learning: In cozystack, the plural form for the Kubernetes custom resource is `kuberneteses.apps.cozystack.io`, not `kubernetes.apps.cozystack.io`. This is defined in the API schema even though it's not grammatically perfect.
Applied to files:
pkg/apiserver/apiserver.gopkg/cmd/server/start.go
🧬 Code Graph Analysis (6)
pkg/apis/core/validation/validation.go (1)
pkg/apis/core/v1alpha1/types.go (1)
TenantNamespace(18-21)
pkg/apis/core/install/roundtrip_test.go (4)
pkg/apiserver/scheme_test.go (1)
TestRoundTripTypes(27-30)pkg/apis/core/install/install.go (1)
Install(26-29)pkg/apis/core/fuzzer/fuzzer.go (1)
Funcs(27-33)pkg/apis/apps/install/roundtrip_test.go (1)
TestRoundTripTypes(26-30)
pkg/apiserver/apiserver.go (8)
pkg/apis/core/install/install.go (1)
Install(26-29)pkg/apis/apps/install/install.go (1)
Install(26-29)pkg/registry/registry.go (1)
RESTInPeace(33-33)pkg/registry/core/tenantnamespace/rest.go (1)
NewREST(54-63)pkg/registry/apps/application/rest.go (1)
NewREST(90-117)pkg/config/config.go (1)
ResourceConfig(20-22)pkg/apis/apps/v1alpha1/register.go (1)
GroupName(19-19)pkg/apis/core/v1alpha1/register.go (1)
GroupName(18-18)
pkg/apis/core/v1alpha1/register.go (4)
pkg/apis/apps/v1alpha1/register.go (5)
GroupName(19-19)SchemeBuilder(30-30)AddToScheme(32-32)SchemeGroupVersion(22-22)Resource(48-50)pkg/apis/core/register.go (1)
GroupName(21-21)pkg/apiserver/apiserver.go (1)
Scheme(44-44)pkg/apis/core/v1alpha1/types.go (2)
TenantNamespace(18-21)TenantNamespaceList(26-30)
pkg/cmd/server/start.go (6)
pkg/apiserver/apiserver.go (4)
Codecs(47-47)CozyComponentName(48-48)Config(75-78)Scheme(44-44)pkg/apis/apps/v1alpha1/register.go (3)
SchemeGroupVersion(22-22)AddToScheme(32-32)RegisterDynamicTypes(58-73)pkg/apis/core/v1alpha1/register.go (3)
SchemeGroupVersion(21-21)AddToScheme(31-31)RegisterStaticTypes(56-63)api/v1alpha1/groupversion_info.go (1)
AddToScheme(35-35)api/v1alpha1/cozystackresourcedefinitions_types.go (1)
CozystackResourceDefinitionList(36-40)pkg/config/config.go (1)
ResourceConfig(20-22)
pkg/registry/core/tenantnamespace/rest.go (4)
pkg/registry/registry.go (1)
REST(26-28)pkg/apis/core/register.go (1)
GroupName(21-21)pkg/apis/core/v1alpha1/register.go (2)
GroupName(18-18)Resource(47-49)pkg/apis/core/v1alpha1/types.go (2)
TenantNamespace(18-21)TenantNamespaceList(26-30)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
🔇 Additional comments (4)
pkg/apis/core/fuzzer/fuzzer.go (1)
17-33: LGTM!The fuzzer implementation correctly follows the established pattern for Kubernetes API fuzzing, using
FuzzNoCustomto avoid infinite recursion.pkg/apis/core/install/roundtrip_test.go (1)
26-30: LGTM!The roundtrip test correctly validates serialization/deserialization for the core API group, following the same pattern as the apps API group tests.
pkg/apis/core/v1alpha1/register.go (1)
1-63: LGTM! Standard API registration implementation.The file follows the standard Kubernetes API registration pattern correctly. The separation between
addKnownTypes(for metav1 registration) andRegisterStaticTypes(for actual type registration) is a clean approach that allows for flexible registration strategies.pkg/cmd/server/start.go (1)
28-293: Well-structured refactoring from Apps to Cozy naming.The refactoring successfully:
- Renames all components from "Apps" to "Cozy" to reflect the broader scope
- Properly registers both
coreandappsAPI groups- Maintains correct registration order (static types before dynamic types)
- Updates all references consistently throughout the file
The separation of static and dynamic type registration in the
Config()method (lines 215-222) is particularly clean.
| type TenantNamespace struct { | ||
| metav1.TypeMeta `json:",inline"` | ||
| metav1.ObjectMeta `json:"metadata,omitempty"` | ||
| } |
There was a problem hiding this comment.
TenantNamespace type lacks a Spec field referenced in validation code.
The validation code in pkg/apis/core/validation/validation.go calls ValidateTenantNamespaceSpec(&f.Spec, ...) on line 28, but the TenantNamespace struct doesn't have a Spec field. This will cause a compilation error.
Since the comment indicates this is a thin wrapper with no spec/status, the validation code should be updated to remove the spec validation:
- allErrs = append(allErrs, ValidateTenantNamespaceSpec(&f.Spec, field.NewPath("spec"))...)Or if a spec is intended, add it to the type definition:
type TenantNamespace struct {
metav1.TypeMeta `json:",inline"`
metav1.ObjectMeta `json:"metadata,omitempty"`
+ Spec TenantNamespaceSpec `json:"spec,omitempty"`
}🤖 Prompt for AI Agents
In pkg/apis/core/v1alpha1/types.go around lines 18 to 21, the TenantNamespace
struct lacks a Spec field, but the validation code expects one, causing a
compilation error. To fix this, either add a Spec field to the TenantNamespace
struct matching the expected type used in validation, or update the validation
code in pkg/apis/core/validation/validation.go to remove or skip the call to
ValidateTenantNamespaceSpec since the struct is intended as a thin wrapper
without a Spec.
| func ValidateTenantNamespace(f *core.TenantNamespace) field.ErrorList { | ||
| allErrs := field.ErrorList{} | ||
|
|
||
| allErrs = append(allErrs, ValidateTenantNamespaceSpec(&f.Spec, field.NewPath("spec"))...) | ||
|
|
||
| return allErrs | ||
| } | ||
|
|
||
| // ValidateTenantNamespaceSpec validates a TenantNamespaceSpec. | ||
| func ValidateTenantNamespaceSpec(s *core.TenantNamespaceSpec, fldPath *field.Path) field.ErrorList { | ||
| allErrs := field.ErrorList{} | ||
|
|
||
| // TODO validation | ||
|
|
||
| return allErrs | ||
| } |
There was a problem hiding this comment.
Validation code references non-existent TenantNamespaceSpec type.
This validation code will not compile because:
- Line 28 references
f.SpecbutTenantNamespacehas noSpecfield - Line 34 expects
*core.TenantNamespaceSpectype which is not defined
Since TenantNamespace is documented as having "no spec/status", these validation functions should be simplified.
Apply this fix to align with the actual type definition:
-// ValidateTenantNamespace validates a TenantNamespace.
-func ValidateTenantNamespace(f *core.TenantNamespace) field.ErrorList {
- allErrs := field.ErrorList{}
-
- allErrs = append(allErrs, ValidateTenantNamespaceSpec(&f.Spec, field.NewPath("spec"))...)
-
- return allErrs
-}
-
-// ValidateTenantNamespaceSpec validates a TenantNamespaceSpec.
-func ValidateTenantNamespaceSpec(s *core.TenantNamespaceSpec, fldPath *field.Path) field.ErrorList {
- allErrs := field.ErrorList{}
-
- // TODO validation
-
- return allErrs
-}
+// ValidateTenantNamespace validates a TenantNamespace.
+func ValidateTenantNamespace(f *core.TenantNamespace) field.ErrorList {
+ allErrs := field.ErrorList{}
+
+ // TenantNamespace only has metadata, no spec to validate
+ // Could add metadata validation here if needed
+
+ return allErrs
+}🤖 Prompt for AI Agents
In pkg/apis/core/validation/validation.go around lines 25 to 40, the validation
functions incorrectly reference a non-existent Spec field and
TenantNamespaceSpec type. Since TenantNamespace has no Spec or status fields,
remove the ValidateTenantNamespaceSpec function entirely and simplify
ValidateTenantNamespace to return an empty error list without attempting to
access f.Spec or call any other validation. This aligns the validation code with
the actual TenantNamespace type definition.
| "github.com/cozystack/cozystack/pkg/apis/apps" | ||
| "github.com/cozystack/cozystack/pkg/apis/apps/install" | ||
| appsinstall "github.com/cozystack/cozystack/pkg/apis/apps/install" | ||
| coreinstall "github.com/cozystack/cozystack/pkg/apis/apps/install" |
There was a problem hiding this comment.
Fix incorrect import path for coreinstall.
The import alias coreinstall incorrectly imports from the apps package instead of the core package.
Apply this fix:
- coreinstall "github.com/cozystack/cozystack/pkg/apis/apps/install"
+ coreinstall "github.com/cozystack/cozystack/pkg/apis/core/install"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| coreinstall "github.com/cozystack/cozystack/pkg/apis/apps/install" | |
| -coreinstall "github.com/cozystack/cozystack/pkg/apis/apps/install" | |
| +coreinstall "github.com/cozystack/cozystack/pkg/apis/core/install" |
🤖 Prompt for AI Agents
In pkg/apiserver/apiserver.go at line 34, the import alias coreinstall is
incorrectly importing from the apps package. Change the import path to point to
the core package instead of apps by updating the import statement to the correct
core package path.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
pkg/apiserver/apiserver.go (2)
35-35: Fix incorrect import path for coreinstall.The import alias
coreinstallis importing from the apps package instead of the core package, which is incorrect based on the variable name and usage context.
133-153: Remove obsolete storage registration code.This entire block represents the old implementation that mixes resources from different API groups in a single storage map and contains a duplicate loop (lines 150-153). This code has been superseded by the properly separated API group registration below (lines 155-183).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/apiserver/apiserver.go(5 hunks)pkg/registry/core/tenantnamespace/rest.go(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: lllamnyp
PR: cozystack/cozystack#1130
File: hack/e2e-apps/kubernetes.bats:101-101
Timestamp: 2025-07-11T06:11:25.438Z
Learning: In cozystack, the plural form for the Kubernetes custom resource is `kuberneteses.apps.cozystack.io`, not `kubernetes.apps.cozystack.io`. This is defined in the API schema even though it's not grammatically perfect.
Learnt from: NickVolynkin
PR: cozystack/cozystack#1120
File: packages/apps/clickhouse/README.md:60-67
Timestamp: 2025-07-03T05:54:51.264Z
Learning: The `cozy-lib.resources.sanitize` function in packages/library/cozy-lib/templates/_resources.tpl supports both standard Kubernetes resource format (with limits:/requests: sections) and flat format (direct resource specifications). The flat format takes priority over nested values. CozyStack apps include cozy-lib as a chart dependency through symlinks in packages/apps/*/charts/cozy-lib directories.
📚 Learning: in cozystack, the plural form for the kubernetes custom resource is `kuberneteses.apps.cozystack.io`...
Learnt from: lllamnyp
PR: cozystack/cozystack#1130
File: hack/e2e-apps/kubernetes.bats:101-101
Timestamp: 2025-07-11T06:11:25.438Z
Learning: In cozystack, the plural form for the Kubernetes custom resource is `kuberneteses.apps.cozystack.io`, not `kubernetes.apps.cozystack.io`. This is defined in the API schema even though it's not grammatically perfect.
Applied to files:
pkg/apiserver/apiserver.go
🔇 Additional comments (16)
pkg/apiserver/apiserver.go (7)
30-30: LGTM: Added kubernetes client import.The addition of the kubernetes clientset import is necessary to support the new TenantNamespace resource implementation that requires authorization checks.
49-49: LGTM: Component name updated appropriately.The component name change from "Apps" to "Cozy" correctly reflects the expanded scope to include both apps and core API groups.
53-54: LGTM: Scheme installation updated for both API groups.The explicit installation of both apps and core API groups into the scheme is correct and necessary for the new architecture.
81-84: LGTM: Server struct renamed consistently.The rename from
AppsServertoCozyServeris consistent with the expanded functionality and aligns with the component name change.
106-108: LGTM: Server initialization updated consistently.The server name change to "cozy-apiserver" and struct initialization are consistent with the overall refactoring.
128-131: LGTM: Kubernetes clientset added for authorization.The addition of the kubernetes clientset is necessary to support the SubjectAccessReview functionality in the TenantNamespace implementation.
155-183: LGTM: Proper API group separation implemented.The separation of static cluster-scoped resources (core API group) and dynamic per-tenant resources (apps API group) into distinct storage maps and API group registrations is architecturally sound and correctly implemented.
pkg/registry/core/tenantnamespace/rest.go (9)
42-51: LGTM: Interface conformance verification.The compile-time interface conformance checks ensure that the REST struct correctly implements all required Kubernetes API server interfaces.
54-75: LGTM: Well-structured REST implementation.The REST struct design with dynamic client, authorization client, and configurable worker pool is appropriate for the tenant namespace filtering use case.
81-81: LGTM: Correct cluster-scoped resource.TenantNamespace is correctly declared as cluster-scoped since it represents a filtered view of cluster-level Namespace resources.
134-162: LGTM: Proper Get implementation with authorization.The Get method correctly validates the tenant prefix and converts the underlying Namespace to a TenantNamespace object with proper metadata mapping.
168-190: LGTM: Efficient watch filtering.The watch implementation uses Kubernetes' built-in watch.Filter for efficient prefix-based filtering without additional overhead.
196-244: LGTM: Comprehensive table conversion.The table converter handles all relevant object types and provides appropriate column definitions for kubectl output.
256-270: LGTM: Proper error type implementation.The custom error type correctly implements the HTTP status interface for API server error responses.
286-327: LGTM: Well-implemented concurrent authorization filtering.The concurrent SubjectAccessReview processing with configurable worker pool provides good performance while maintaining security. The error handling and logging are appropriate.
339-367: Authorization resource and group validatedConfirmed that the SAR uses Resource="workloadmonitors" and Group="cozystack.io", which match:
- CRD definition in packages/system/cozystack-controller/templates/crds/cozystack.io_workloadmonitors.yaml
- Controller RBAC annotations in internal/controller/workloadmonitor_controller.go
- Helm RoleBindings across all tenant dashboards
No changes required.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
pkg/registry/core/tenantsecret/rest.go (1)
86-105: Consider removing redundant StringData fieldThe function sets both
DataandStringDatafields in the TenantSecret. SinceStringDatais typically a write-only field in Kubernetes Secrets (used for input convenience), consider only populatingDatain the read path to avoid confusion and redundancy.func secretToTenant(sec *corev1.Secret) *corev1alpha1.TenantSecret { return &corev1alpha1.TenantSecret{ TypeMeta: metav1.TypeMeta{ APIVersion: corev1alpha1.SchemeGroupVersion.String(), Kind: kindTenantSecret, }, ObjectMeta: metav1.ObjectMeta{ Name: sec.Name, Namespace: sec.Namespace, UID: sec.UID, ResourceVersion: sec.ResourceVersion, CreationTimestamp: sec.CreationTimestamp, Labels: stripInternal(sec.Labels), Annotations: stripInternal(sec.Annotations), }, Type: string(sec.Type), Data: sec.Data, - StringData: decodeStringData(sec.Data), } }pkg/apis/core/v1alpha1/tenantsecret_types.go (1)
1-2: Add missing copyright headerThis file is missing the copyright header that's present in other files. Consider adding it for consistency.
// SPDX-License-Identifier: Apache-2.0 +// Copyright 2025 The Cozystack Authors. + package v1alpha1pkg/registry/core/tenantnamespace/rest.go (1)
270-274: Simplify worker count calculationThe use of
math.Minwith float64 conversions is unnecessary for integer comparison.- workers := int(math.Min(float64(r.maxWorkers), float64(len(names)))) + workers := r.maxWorkers + if len(names) < workers { + workers = len(names) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pkg/generated/openapi/zz_generated.openapi.gois excluded by!**/generated/**
📒 Files selected for processing (11)
packages/apps/tenant/templates/tenant.yaml(4 hunks)packages/system/cozystack-api/templates/rbac.yaml(1 hunks)packages/system/cozystack-api/templates/tenantnamespaces-rbac.yaml(1 hunks)packages/system/cozystack-api/values.yaml(1 hunks)pkg/apis/core/v1alpha1/register.go(1 hunks)pkg/apis/core/v1alpha1/tenantnamespace_types.go(1 hunks)pkg/apis/core/v1alpha1/tenantsecret_types.go(1 hunks)pkg/apis/core/v1alpha1/zz_generated.deepcopy.go(1 hunks)pkg/apiserver/apiserver.go(5 hunks)pkg/registry/core/tenantnamespace/rest.go(1 hunks)pkg/registry/core/tenantsecret/rest.go(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- packages/system/cozystack-api/values.yaml
- packages/system/cozystack-api/templates/rbac.yaml
- packages/system/cozystack-api/templates/tenantnamespaces-rbac.yaml
- packages/apps/tenant/templates/tenant.yaml
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/apiserver/apiserver.go
- pkg/apis/core/v1alpha1/register.go
- pkg/apis/core/v1alpha1/zz_generated.deepcopy.go
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: lllamnyp
PR: cozystack/cozystack#1130
File: hack/e2e-apps/kubernetes.bats:101-101
Timestamp: 2025-07-11T06:11:25.438Z
Learning: In cozystack, the plural form for the Kubernetes custom resource is `kuberneteses.apps.cozystack.io`, not `kubernetes.apps.cozystack.io`. This is defined in the API schema even though it's not grammatically perfect.
🔇 Additional comments (8)
pkg/registry/core/tenantsecret/rest.go (4)
234-276: LGTM!The List method correctly filters secrets by the UI label, properly handles label and field selectors, and ensures consistent ordering of results.
363-398: LGTM!The Watch implementation correctly filters secrets by the UI label and properly transforms watch events from Secret to TenantSecret objects.
404-435: LGTM!The table conversion implementation properly handles both TenantSecret and TenantSecretList objects with appropriate columns.
48-62: stripInternal and conversion helpers correctly manage the UI labelThe
stripInternalfunction intentionally removes all internal keys—includingcozystack.io/ui—but the conversion helpers re-add the UI label before returning or persisting a TenantSecret:
- In
convertFromSecret(aroundout.Labels[uiLabelKey] = uiLabelValue), the UI label is restored on the TenantSecret object.- In
convertToSecret(and update logic, whereif out.Labels[uiLabelKey] != uiLabelValue { … }), the UI label is ensured on the underlying Kubernetes Secret.This guarantees that consumers always see
cozystack.io/ui=true, so no change tostripInternalis required.pkg/apis/core/v1alpha1/tenantnamespace_types.go (1)
1-30: LGTM!The TenantNamespace type definitions are clean and appropriate for a resource that reflects existing Kubernetes Namespaces. The deepcopy annotations are correctly placed.
pkg/apis/core/v1alpha1/tenantsecret_types.go (1)
8-24: LGTM!The TenantSecret type definitions properly mirror the Kubernetes Secret structure with appropriate field types and annotations.
pkg/registry/core/tenantnamespace/rest.go (2)
119-147: LGTM!The Get method correctly validates the tenant namespace prefix and returns appropriate errors for non-tenant namespaces.
153-191: LGTM!The Watch implementation correctly filters namespace events by the tenant prefix and properly transforms them to TenantNamespace objects.
| func (r *REST) sar(ctx context.Context, ns string) (bool, error) { | ||
| u, ok := request.UserFrom(ctx) | ||
| if !ok || u == nil { | ||
| return false, fmt.Errorf("user missing in context") | ||
| } | ||
|
|
||
| sar := &authorizationv1.SubjectAccessReview{ | ||
| Spec: authorizationv1.SubjectAccessReviewSpec{ | ||
| User: u.GetName(), | ||
| Groups: u.GetGroups(), | ||
| ResourceAttributes: &authorizationv1.ResourceAttributes{ | ||
| Group: "cozystack.io", | ||
| Resource: "workloadmonitors", | ||
| Verb: "get", | ||
| Namespace: ns, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| rsp, err := r.authClient.SubjectAccessReviews(). | ||
| Create(ctx, sar, metav1.CreateOptions{}) | ||
| if err != nil { | ||
| return false, err | ||
| } | ||
| return rsp.Status.Allowed, nil | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the authorization model for tenant access
The SAR check uses access to the "workloadmonitors" resource as a proxy for tenant namespace visibility. Please verify this is the intended authorization model.
🌐 Web query:
Is checking access to the "workloadmonitors" resource in Cozystack the standard way to determine if a user has access to a tenant namespace?
💡 Result:
In Cozystack, the "workloadmonitors" resource is not typically used to determine if a user has access to a tenant namespace. Instead, access control is managed through Role-Based Access Control (RBAC) policies, which define permissions for users and service accounts within specific namespaces. By configuring appropriate RBAC roles and bindings, you can control access to resources within a tenant's namespace. (kubernetes.io)
Therefore, to assess a user's access to a tenant namespace, you should review the RBAC configurations associated with that namespace, rather than checking the "workloadmonitors" resource.
Citations:
Inappropriate authorization resource: switch SAR to verify namespace access via RBAC
The current SAR checks get on workloadmonitors.cozystack.io in the target namespace, but Cozystack uses standard Kubernetes RBAC to control tenant-namespace access. You should verify namespace visibility by checking permissions on the namespace itself (or reviewing the RBAC Roles/RoleBindings), not on workloadmonitors.
Please update the SAR invocation in pkg/registry/core/tenantnamespace/rest.go (around lines 317–342) to something like:
func (r *REST) sar(ctx context.Context, ns string) (bool, error) {
@@
- ResourceAttributes: &authorizationv1.ResourceAttributes{
- Group: "cozystack.io",
- Resource: "workloadmonitors",
- Verb: "get",
- Namespace: ns,
- },
+ ResourceAttributes: &authorizationv1.ResourceAttributes{
+ Group: "",
+ Resource: "namespaces",
+ Verb: "get",
+ Name: ns,
+ },Alternatively, ensure your RBAC Roles/RoleBindings grant get on the namespace to the user and rely on those policies.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (r *REST) sar(ctx context.Context, ns string) (bool, error) { | |
| u, ok := request.UserFrom(ctx) | |
| if !ok || u == nil { | |
| return false, fmt.Errorf("user missing in context") | |
| } | |
| sar := &authorizationv1.SubjectAccessReview{ | |
| Spec: authorizationv1.SubjectAccessReviewSpec{ | |
| User: u.GetName(), | |
| Groups: u.GetGroups(), | |
| ResourceAttributes: &authorizationv1.ResourceAttributes{ | |
| Group: "cozystack.io", | |
| Resource: "workloadmonitors", | |
| Verb: "get", | |
| Namespace: ns, | |
| }, | |
| }, | |
| } | |
| rsp, err := r.authClient.SubjectAccessReviews(). | |
| Create(ctx, sar, metav1.CreateOptions{}) | |
| if err != nil { | |
| return false, err | |
| } | |
| return rsp.Status.Allowed, nil | |
| } | |
| func (r *REST) sar(ctx context.Context, ns string) (bool, error) { | |
| u, ok := request.UserFrom(ctx) | |
| if !ok || u == nil { | |
| return false, fmt.Errorf("user missing in context") | |
| } | |
| sar := &authorizationv1.SubjectAccessReview{ | |
| Spec: authorizationv1.SubjectAccessReviewSpec{ | |
| User: u.GetName(), | |
| Groups: u.GetGroups(), | |
| ResourceAttributes: &authorizationv1.ResourceAttributes{ | |
| Group: "", | |
| Resource: "namespaces", | |
| Verb: "get", | |
| Name: ns, | |
| }, | |
| }, | |
| } | |
| rsp, err := r.authClient.SubjectAccessReviews(). | |
| Create(ctx, sar, metav1.CreateOptions{}) | |
| if err != nil { | |
| return false, err | |
| } | |
| return rsp.Status.Allowed, nil | |
| } |
🤖 Prompt for AI Agents
In pkg/registry/core/tenantnamespace/rest.go around lines 317 to 342, the SAR
currently checks 'get' permission on the 'workloadmonitors.cozystack.io'
resource, which is incorrect for verifying namespace access. Update the SAR to
check 'get' permission on the 'namespaces' resource in the core API group
instead, targeting the specific namespace name. Modify the ResourceAttributes in
the SAR Spec to have Group as an empty string, Resource as "namespaces", Verb as
"get", and Name as the namespace string 'ns'. This change aligns the SAR with
standard Kubernetes RBAC for namespace access verification.
| func decodeStringData(d map[string][]byte) map[string]string { | ||
| if len(d) == 0 { | ||
| return nil | ||
| } | ||
| out := make(map[string]string, len(d)) | ||
| for k, v := range d { | ||
| out[k] = base64.StdEncoding.EncodeToString(v) | ||
| } | ||
| return out | ||
| } |
There was a problem hiding this comment.
Fix double base64 encoding issue
The decodeStringData function base64-encodes byte data, but in Kubernetes Secrets, the Data field already contains decoded bytes in memory (Kubernetes handles base64 encoding/decoding during serialization). This results in double encoding when converting to TenantSecret.
func decodeStringData(d map[string][]byte) map[string]string {
if len(d) == 0 {
return nil
}
out := make(map[string]string, len(d))
for k, v := range d {
- out[k] = base64.StdEncoding.EncodeToString(v)
+ out[k] = string(v)
}
return out
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func decodeStringData(d map[string][]byte) map[string]string { | |
| if len(d) == 0 { | |
| return nil | |
| } | |
| out := make(map[string]string, len(d)) | |
| for k, v := range d { | |
| out[k] = base64.StdEncoding.EncodeToString(v) | |
| } | |
| return out | |
| } | |
| func decodeStringData(d map[string][]byte) map[string]string { | |
| if len(d) == 0 { | |
| return nil | |
| } | |
| out := make(map[string]string, len(d)) | |
| for k, v := range d { | |
| out[k] = string(v) | |
| } | |
| return out | |
| } |
🤖 Prompt for AI Agents
In pkg/registry/core/tenantsecret/rest.go around lines 75 to 84, the
decodeStringData function incorrectly base64-encodes the byte slices from the
input map, causing double encoding since Kubernetes Secrets Data is already
decoded bytes. To fix this, change the function to convert the byte slices
directly to strings without base64 encoding, ensuring the output map contains
the correct string representation of the secret data.
| func (r *REST) Create( | ||
| ctx context.Context, | ||
| obj runtime.Object, | ||
| _ rest.ValidateObjectFunc, | ||
| opts *metav1.CreateOptions, | ||
| ) (runtime.Object, error) { | ||
| in, ok := obj.(*corev1alpha1.TenantSecret) | ||
| if !ok { | ||
| return nil, fmt.Errorf("expected TenantSecret, got %T", obj) | ||
| } | ||
|
|
||
| sec := tenantToSecret(in, nil) | ||
| out, err := r.core.Secrets(sec.Namespace).Create(ctx, sec, *opts) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return secretToTenant(out), nil | ||
| } |
There was a problem hiding this comment.
Add namespace validation in Create method
The method uses the namespace from the TenantSecret object without validating it against the request context. This could allow users to create secrets in unauthorized namespaces.
func (r *REST) Create(
ctx context.Context,
obj runtime.Object,
_ rest.ValidateObjectFunc,
opts *metav1.CreateOptions,
) (runtime.Object, error) {
in, ok := obj.(*corev1alpha1.TenantSecret)
if !ok {
return nil, fmt.Errorf("expected TenantSecret, got %T", obj)
}
+ ns, err := nsFrom(ctx)
+ if err != nil {
+ return nil, err
+ }
+ in.Namespace = ns
+
sec := tenantToSecret(in, nil)
out, err := r.core.Secrets(sec.Namespace).Create(ctx, sec, *opts)
if err != nil {
return nil, err
}
return secretToTenant(out), nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (r *REST) Create( | |
| ctx context.Context, | |
| obj runtime.Object, | |
| _ rest.ValidateObjectFunc, | |
| opts *metav1.CreateOptions, | |
| ) (runtime.Object, error) { | |
| in, ok := obj.(*corev1alpha1.TenantSecret) | |
| if !ok { | |
| return nil, fmt.Errorf("expected TenantSecret, got %T", obj) | |
| } | |
| sec := tenantToSecret(in, nil) | |
| out, err := r.core.Secrets(sec.Namespace).Create(ctx, sec, *opts) | |
| if err != nil { | |
| return nil, err | |
| } | |
| return secretToTenant(out), nil | |
| } | |
| func (r *REST) Create( | |
| ctx context.Context, | |
| obj runtime.Object, | |
| _ rest.ValidateObjectFunc, | |
| opts *metav1.CreateOptions, | |
| ) (runtime.Object, error) { | |
| in, ok := obj.(*corev1alpha1.TenantSecret) | |
| if !ok { | |
| return nil, fmt.Errorf("expected TenantSecret, got %T", obj) | |
| } | |
| ns, err := nsFrom(ctx) | |
| if err != nil { | |
| return nil, err | |
| } | |
| in.Namespace = ns | |
| sec := tenantToSecret(in, nil) | |
| out, err := r.core.Secrets(sec.Namespace).Create(ctx, sec, *opts) | |
| if err != nil { | |
| return nil, err | |
| } | |
| return secretToTenant(out), nil | |
| } |
🤖 Prompt for AI Agents
In pkg/registry/core/tenantsecret/rest.go around lines 199 to 216, the Create
method uses the namespace from the TenantSecret object directly without
validating it against the request context, which risks unauthorized namespace
access. To fix this, extract the namespace from the context (usually via a
helper function or from the request info), then compare it with the namespace in
the TenantSecret object. If they do not match, return an error denying the
operation. This ensures that the creation is authorized only within the allowed
namespace.
| func (r *REST) Update( | ||
| ctx context.Context, | ||
| name string, | ||
| objInfo rest.UpdatedObjectInfo, | ||
| _ rest.ValidateObjectFunc, | ||
| _ rest.ValidateObjectUpdateFunc, | ||
| forceCreate bool, | ||
| opts *metav1.UpdateOptions, | ||
| ) (runtime.Object, bool, error) { | ||
| ns, err := nsFrom(ctx) | ||
| if err != nil { | ||
| return nil, false, err | ||
| } | ||
|
|
||
| cur, err := r.core.Secrets(ns).Get(ctx, name, metav1.GetOptions{}) | ||
| if err != nil && !apierrors.IsNotFound(err) { | ||
| return nil, false, err | ||
| } | ||
|
|
||
| newObj, err := objInfo.UpdatedObject(ctx, nil) | ||
| if err != nil { | ||
| return nil, false, err | ||
| } | ||
| in := newObj.(*corev1alpha1.TenantSecret) | ||
|
|
||
| newSec := tenantToSecret(in, cur) | ||
| if cur == nil { | ||
| if !forceCreate && err == nil { | ||
| return nil, false, apierrors.NewNotFound(r.gvr.GroupResource(), name) | ||
| } | ||
| out, err := r.core.Secrets(ns).Create(ctx, newSec, metav1.CreateOptions{}) | ||
| return secretToTenant(out), true, err | ||
| } | ||
|
|
||
| newSec.ResourceVersion = cur.ResourceVersion | ||
| out, err := r.core.Secrets(ns).Update(ctx, newSec, *opts) | ||
| return secretToTenant(out), false, err | ||
| } |
There was a problem hiding this comment.
Fix logic error in force create condition
Line 305 has an incorrect condition - it checks err == nil when it should check for err != nil (NotFound error) to handle the force create case.
newSec := tenantToSecret(in, cur)
if cur == nil {
- if !forceCreate && err == nil {
+ if !forceCreate {
return nil, false, apierrors.NewNotFound(r.gvr.GroupResource(), name)
}
out, err := r.core.Secrets(ns).Create(ctx, newSec, metav1.CreateOptions{})
return secretToTenant(out), true, err
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (r *REST) Update( | |
| ctx context.Context, | |
| name string, | |
| objInfo rest.UpdatedObjectInfo, | |
| _ rest.ValidateObjectFunc, | |
| _ rest.ValidateObjectUpdateFunc, | |
| forceCreate bool, | |
| opts *metav1.UpdateOptions, | |
| ) (runtime.Object, bool, error) { | |
| ns, err := nsFrom(ctx) | |
| if err != nil { | |
| return nil, false, err | |
| } | |
| cur, err := r.core.Secrets(ns).Get(ctx, name, metav1.GetOptions{}) | |
| if err != nil && !apierrors.IsNotFound(err) { | |
| return nil, false, err | |
| } | |
| newObj, err := objInfo.UpdatedObject(ctx, nil) | |
| if err != nil { | |
| return nil, false, err | |
| } | |
| in := newObj.(*corev1alpha1.TenantSecret) | |
| newSec := tenantToSecret(in, cur) | |
| if cur == nil { | |
| if !forceCreate && err == nil { | |
| return nil, false, apierrors.NewNotFound(r.gvr.GroupResource(), name) | |
| } | |
| out, err := r.core.Secrets(ns).Create(ctx, newSec, metav1.CreateOptions{}) | |
| return secretToTenant(out), true, err | |
| } | |
| newSec.ResourceVersion = cur.ResourceVersion | |
| out, err := r.core.Secrets(ns).Update(ctx, newSec, *opts) | |
| return secretToTenant(out), false, err | |
| } | |
| func (r *REST) Update( | |
| ctx context.Context, | |
| name string, | |
| objInfo rest.UpdatedObjectInfo, | |
| _ rest.ValidateObjectFunc, | |
| _ rest.ValidateObjectUpdateFunc, | |
| forceCreate bool, | |
| opts *metav1.UpdateOptions, | |
| ) (runtime.Object, bool, error) { | |
| ns, err := nsFrom(ctx) | |
| if err != nil { | |
| return nil, false, err | |
| } | |
| cur, err := r.core.Secrets(ns).Get(ctx, name, metav1.GetOptions{}) | |
| if err != nil && !apierrors.IsNotFound(err) { | |
| return nil, false, err | |
| } | |
| newObj, err := objInfo.UpdatedObject(ctx, nil) | |
| if err != nil { | |
| return nil, false, err | |
| } | |
| in := newObj.(*corev1alpha1.TenantSecret) | |
| newSec := tenantToSecret(in, cur) | |
| if cur == nil { | |
| if !forceCreate { | |
| return nil, false, apierrors.NewNotFound(r.gvr.GroupResource(), name) | |
| } | |
| out, err := r.core.Secrets(ns).Create(ctx, newSec, metav1.CreateOptions{}) | |
| return secretToTenant(out), true, err | |
| } | |
| newSec.ResourceVersion = cur.ResourceVersion | |
| out, err := r.core.Secrets(ns).Update(ctx, newSec, *opts) | |
| return secretToTenant(out), false, err | |
| } |
🤖 Prompt for AI Agents
In pkg/registry/core/tenantsecret/rest.go around lines 278 to 315, the condition
on line 305 incorrectly checks if err == nil when deciding to handle the force
create case after a NotFound error. Change this condition to check if err != nil
to correctly detect the NotFound error and proceed with creation when
forceCreate is true.
| // Ensure UI label is preserved | ||
| if out.Labels[uiLabelKey] != uiLabelValue { | ||
| out.Labels[uiLabelKey] = uiLabelValue | ||
| out, _ = r.core.Secrets(ns).Update(ctx, out, metav1.UpdateOptions{}) | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle error from label preservation update
The error from the update operation to preserve the UI label is ignored. This could lead to secrets losing their UI label silently.
// Ensure UI label is preserved
if out.Labels[uiLabelKey] != uiLabelValue {
+ if out.Labels == nil {
+ out.Labels = make(map[string]string)
+ }
out.Labels[uiLabelKey] = uiLabelValue
- out, _ = r.core.Secrets(ns).Update(ctx, out, metav1.UpdateOptions{})
+ out, err = r.core.Secrets(ns).Update(ctx, out, metav1.UpdateOptions{})
+ if err != nil {
+ return nil, err
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Ensure UI label is preserved | |
| if out.Labels[uiLabelKey] != uiLabelValue { | |
| out.Labels[uiLabelKey] = uiLabelValue | |
| out, _ = r.core.Secrets(ns).Update(ctx, out, metav1.UpdateOptions{}) | |
| } | |
| // Ensure UI label is preserved | |
| if out.Labels[uiLabelKey] != uiLabelValue { | |
| if out.Labels == nil { | |
| out.Labels = make(map[string]string) | |
| } | |
| out.Labels[uiLabelKey] = uiLabelValue | |
| out, err = r.core.Secrets(ns).Update(ctx, out, metav1.UpdateOptions{}) | |
| if err != nil { | |
| return nil, err | |
| } | |
| } |
🤖 Prompt for AI Agents
In pkg/registry/core/tenantsecret/rest.go around lines 350 to 355, the error
returned from the update operation that preserves the UI label is currently
ignored. Modify the code to capture and handle this error properly, such as
logging it or returning it, to ensure that failures in updating the secret's
label do not go unnoticed.
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
c1a4b0b to
8b97d87
Compare
Signed-off-by: Andrei Kvapil kvapss@gmail.com
What this PR does
Release note
Summary by CodeRabbit
New Features
RBAC and API Registration
Improvements
Bug Fixes
Tests