[api] Use shared informer cache - #1539
Conversation
Summary of ChangesHello Timofei Larkin (@lllamnyp), 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 undertakes a significant refactoring of the Cozystack API server's client-side interactions with Kubernetes. The primary goal is to enhance performance and streamline the codebase by transitioning from dynamic clients and unstructured object manipulation to the more efficient and type-safe Highlights
Using Gemini Code AssistThe 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 by creating a comment using either
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 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. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. 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
|
|
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 PR migrates the API server and multiple REST storages from client-go/dynamic and typed client interfaces to controller-runtime Manager and controller-runtime clients, replacing unstructured/dynamic interactions with typed HelmRelease handling, a watch-enabled client, informer validation, and manager-driven cache sync and startup. Changes
Sequence Diagram(s)sequenceDiagram
participant API as API Server
participant Mgr as ControllerRuntime Manager
participant Cache as Informers/Cache
participant Cli as ControllerRuntime Client
participant WCli as Watch Client (WithWatch)
participant REST as REST Storage
participant K8s as Kubernetes API
API->>Mgr: GetConfig() / NewManager(SyncPeriod)
Mgr->>Cache: init caches & informers
Cache->>K8s: verify required resource types
K8s-->>Cache: resource presence
Mgr-->>Cli: provide client
API->>WCli: build watch client from config
API->>REST: NewREST(Cli, WCli)
REST->>K8s: List/Get/Watch via Cli/WCli (HelmRelease/Secret/Namespace)
K8s-->>REST: typed objects
REST-->>REST: convert HelmRelease ↔ Application/TenantModule
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (7)
🧰 Additional context used🧬 Code graph analysis (6)pkg/apiserver/apiserver.go (6)
pkg/registry/apps/application/rest.go (7)
pkg/registry/core/tenantmodule/rest.go (3)
pkg/registry/core/tenantsecretstable/rest.go (1)
pkg/registry/core/tenantnamespace/rest.go (4)
pkg/registry/core/tenantsecret/rest.go (4)
⏰ 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 (24)
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 |
There was a problem hiding this comment.
Code Review
This pull request refactors the Cozystack API server to use typed, cache-backed clients from controller-runtime instead of dynamic clients and standard clientsets. This is a great improvement for performance and code maintainability, as it leverages shared informers and typed objects. The changes are well-implemented across all affected registry packages.
I've found one significant issue regarding a potential resource leak due to incorrect context usage when initializing informers. I've also pointed out a minor issue where an error is ignored, which could lead to inconsistent state. Apart from these points, the refactoring is solid and achieves its goal effectively.
| ctx := context.Background() | ||
|
|
||
| if err = mustGetInformers(ctx, mgr, | ||
| &helmv2.HelmRelease{}, | ||
| &corev1.Secret{}, | ||
| &corev1.Namespace{}, | ||
| &corev1.Service{}, | ||
| &rbacv1.RoleBinding{}, | ||
| ); err != nil { | ||
| return nil, fmt.Errorf("failed to get informers: %w", err) | ||
| } | ||
|
|
||
| go mgr.Start(ctrl.SetupSignalHandler()) |
There was a problem hiding this comment.
The mustGetInformers function is called with context.Background(). The GetInformer method from controller-runtime's cache will start an informer if it's not already running, using the provided context to control its lifecycle. By using context.Background(), the informers started here will never be stopped when the application receives a termination signal, leading to a resource leak.
To fix this, you should create a single cancellable context that is used for both mustGetInformers and mgr.Start. This ensures that all components managed by the manager have the same lifecycle and are properly shut down.
| ctx := context.Background() | |
| if err = mustGetInformers(ctx, mgr, | |
| &helmv2.HelmRelease{}, | |
| &corev1.Secret{}, | |
| &corev1.Namespace{}, | |
| &corev1.Service{}, | |
| &rbacv1.RoleBinding{}, | |
| ); err != nil { | |
| return nil, fmt.Errorf("failed to get informers: %w", err) | |
| } | |
| go mgr.Start(ctrl.SetupSignalHandler()) | |
| ctx := ctrl.SetupSignalHandler() | |
| if err = mustGetInformers(ctx, mgr, | |
| &helmv2.HelmRelease{}, | |
| &corev1.Secret{}, | |
| &corev1.Namespace{}, | |
| &corev1.Service{}, | |
| &rbacv1.RoleBinding{}, | |
| ); err != nil { | |
| return nil, fmt.Errorf("failed to get informers: %w", err) | |
| } | |
| go mgr.Start(ctx) |
| if out.Labels[tsLabelKey] != tsLabelValue { | ||
| out.Labels[tsLabelKey] = tsLabelValue | ||
| out, _ = r.core.Secrets(ns).Update(ctx, out, metav1.UpdateOptions{}) | ||
| _ = r.c.Update(ctx, out, &client.UpdateOptions{Raw: &metav1.UpdateOptions{}}) |
There was a problem hiding this comment.
The error returned by r.c.Update is being ignored. If this update fails, the tsLabelKey label might be missing from the secret, which could cause it to be invisible to this API layer in subsequent operations. It's safer to handle this potential error, for example by logging it or returning it to the caller.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
pkg/registry/core/tenantsecret/rest.go (1)
294-319: forceCreate path is broken; cur is never nil, Update will fail with missing ResourceVersionYou allocate cur before Get, so cur is never nil; on NotFound you fall into Update with empty ResourceVersion → 409/invalid. Handle IsNotFound explicitly.
Apply:
- cur := &corev1.Secret{} - err = r.c.Get(ctx, types.NamespacedName{Namespace: ns, Name: name}, cur, &client.GetOptions{Raw: &metav1.GetOptions{}}) - if err != nil && !apierrors.IsNotFound(err) { + cur := &corev1.Secret{} + err = r.c.Get(ctx, types.NamespacedName{Namespace: ns, Name: name}, cur, &client.GetOptions{Raw: &metav1.GetOptions{}}) + if err != nil && !apierrors.IsNotFound(err) { return nil, false, err - } + } + notFound := apierrors.IsNotFound(err) ... - if cur == nil { - if !forceCreate && err == nil { + if notFound { + if !forceCreate { return nil, false, apierrors.NewNotFound(r.gvr.GroupResource(), name) } - err := r.c.Create(ctx, newSec, &client.CreateOptions{Raw: &metav1.CreateOptions{}}) + err := r.c.Create(ctx, newSec, &client.CreateOptions{Raw: &metav1.CreateOptions{}}) return secretToTenant(newSec), true, err } newSec.ResourceVersion = cur.ResourceVersionpkg/registry/core/tenantsecretstable/rest.go (1)
129-151: Label selector from ListOptions is ignoredYou build sel with user LabelSelector but pass only the ts label in List; user filters are dropped.
- sel := labels.NewSelector() + sel := labels.NewSelector() req, _ := labels.NewRequirement(tsLabelKey, selection.Equals, []string{tsLabelValue}) sel = sel.Add(*req) ... - list := &corev1.SecretList{} - err = r.c.List(ctx, list, - &client.ListOptions{ - Namespace: ns, - Raw: &metav1.ListOptions{ - LabelSelector: labels.Set{tsLabelKey: tsLabelValue}.AsSelector().String(), - FieldSelector: fieldSel, - }, - }) + list := &corev1.SecretList{} + err = r.c.List(ctx, list, &client.ListOptions{ + Namespace: ns, + Raw: &metav1.ListOptions{ + LabelSelector: sel.String(), + FieldSelector: fieldSel, + }, + })pkg/apiserver/apiserver.go (1)
39-39: Wrong import path for core install; build will failLikely should be core API install, not apps.
- coreinstall "github.com/cozystack/cozystack/pkg/apis/apps/install" + coreinstall "github.com/cozystack/cozystack/pkg/apis/core/install"pkg/registry/core/tenantnamespace/rest.go (1)
142-181: Watch leaks tenant namespace names across RBAC; apply same accessibility filter as ListCurrent Watch emits all tenant-* namespaces the user can’t access. Filter events by user membership similar to filterAccessible().
func (r *REST) Watch(ctx context.Context, opts *metainternal.ListOptions) (watch.Interface, error) { - nsList := &corev1.NamespaceList{} + nsList := &corev1.NamespaceList{} nsWatch, err := r.w.Watch(ctx, nsList, &client.ListOptions{Raw: &metav1.ListOptions{ Watch: true, ResourceVersion: opts.ResourceVersion, }}) ... - go func() { + // Precompute allowed set at start (best-effort); consider refreshing on RB changes if needed. + allowed, _ := r.filterAccessible(ctx, []string{}) // if empty, treat as no prefilter + allowedSet := map[string]struct{}{} + for _, n := range allowed { allowedSet[n] = struct{}{} } + go func() { - defer pw.Stop() + defer func() { nsWatch.Stop(); pw.Stop() }() for ev := range nsWatch.ResultChan() { ns, ok := ev.Object.(*corev1.Namespace) if !ok || !strings.HasPrefix(ns.Name, prefix) { continue } + if len(allowedSet) > 0 { + if _, ok := allowedSet[ns.Name]; !ok { + continue + } + } out := &corev1alpha1.TenantNamespace{Note: For stronger guarantees, recompute allowedSet or watch RoleBindings; above is a minimal fix.
pkg/registry/apps/application/rest.go (3)
143-170: Create: preserve API error types and strip server-managed fields; enforce namespace.
- Wrapping client errors with fmt.Errorf loses StatusReason (409/AlreadyExists, etc.). Return the original error.
- Ensure server-managed metadata (resourceVersion, UID, generation, managedFields) are empty on create.
- Enforce metadata.namespace matches the request namespace to prevent cross-namespace writes.
func (r *REST) Create(ctx context.Context, obj runtime.Object, createValidation rest.ValidateObjectFunc, options *metav1.CreateOptions) (runtime.Object, error) { // Assert the object is of type Application app, ok := obj.(*appsv1alpha1.Application) if !ok { return nil, fmt.Errorf("expected Application object, got %T", obj) } + // Enforce namespace from request + ns, err := r.getNamespace(ctx) + if err != nil { + return nil, err + } + if app.Namespace == "" { + app.Namespace = ns + } else if app.Namespace != ns { + return nil, apierrors.NewBadRequest("metadata.namespace does not match request namespace") + } // Convert Application to HelmRelease helmRelease, err := r.ConvertApplicationToHelmRelease(app) if err != nil { klog.Errorf("Conversion error: %v", err) return nil, fmt.Errorf("conversion error: %v", err) } + // Strip server-managed fields for create + helmRelease.SetResourceVersion("") + helmRelease.SetUID("") + helmRelease.SetGeneration(0) + helmRelease.ManagedFields = nil // Merge system labels (from config) directly helmRelease.Labels = mergeMaps(r.releaseConfig.Labels, helmRelease.Labels) // Merge user labels with prefix helmRelease.Labels = mergeMaps(helmRelease.Labels, addPrefixedMap(app.Labels, LabelPrefix)) // Note: Annotations from config are not handled as r.releaseConfig.Annotations is undefined klog.V(6).Infof("Creating HelmRelease %s in namespace %s", helmRelease.Name, app.Namespace) // Create HelmRelease in Kubernetes - err = r.c.Create(ctx, helmRelease, &client.CreateOptions{Raw: options}) - if err != nil { - klog.Errorf("Failed to create HelmRelease %s: %v", helmRelease.Name, err) - return nil, fmt.Errorf("failed to create HelmRelease: %v", err) - } + if err := r.c.Create(ctx, helmRelease, &client.CreateOptions{Raw: options}); err != nil { + klog.Errorf("Failed to create HelmRelease %s: %v", helmRelease.Name, err) + return nil, err + }
458-466: Update: preserve API error types; keep server-managed fields consistent.
- Don’t wrap Update errors; preserve 409 Conflict, 404, etc.
- Fetching current object for missing ResourceVersion is good; ensure no UID/managedFields changes are sent unintentionally.
-err = r.c.Update(ctx, helmRelease, &client.UpdateOptions{Raw: &metav1.UpdateOptions{}}) -if err != nil { - klog.Errorf("Failed to update HelmRelease %s: %v", helmRelease.Name, err) - return nil, false, fmt.Errorf("failed to update HelmRelease: %v", err) -} +if err := r.c.Update(ctx, helmRelease, &client.UpdateOptions{Raw: &metav1.UpdateOptions{}}); err != nil { + klog.Errorf("Failed to update HelmRelease %s: %v", helmRelease.Name, err) + return nil, false, err +}Also applies to: 485-489, 493-506
543-571: Delete: preserve API error types.Return the original error from client.Delete to keep correct HTTP status propagation.
-err = r.c.Delete(ctx, helmRelease, &client.DeleteOptions{Raw: options}) -if err != nil { - klog.Errorf("Failed to delete HelmRelease %s: %v", helmReleaseName, err) - return nil, false, fmt.Errorf("failed to delete HelmRelease: %v", err) -} +if err := r.c.Delete(ctx, helmRelease, &client.DeleteOptions{Raw: options}); err != nil { + klog.Errorf("Failed to delete HelmRelease %s: %v", helmReleaseName, err) + return nil, false, err +}
🧹 Nitpick comments (9)
pkg/registry/core/tenantsecret/rest.go (2)
381-388: Stop underlying watch to avoid leaksDefer base.Stop() so the upstream watch closes when proxy ends.
- base, err := r.w.Watch(ctx, secList, &client.ListOptions{Namespace: ns, Raw: &metav1.ListOptions{ + base, err := r.w.Watch(ctx, secList, &client.ListOptions{Namespace: ns, Raw: &metav1.ListOptions{ Watch: true, LabelSelector: ls, ResourceVersion: opts.ResourceVersion, }}) if err != nil { ... } ch := make(chan watch.Event) proxy := watch.NewProxyWatcher(ch) go func() { - defer proxy.Stop() + defer func() { base.Stop(); proxy.Stop() }() for ev := range base.ResultChan() {
70-79: Misleading name: decodeStringData actually base64-encodesOptional: rename to encodeToBase64Strings or document intent to avoid confusion.
pkg/registry/core/tenantsecretstable/rest.go (2)
80-86: Return proper API error for missing namespaceUse apierrors.NewBadRequest to get HTTP 400 instead of a generic 500.
- if !ok { - return "", fmt.Errorf("namespace required") - } + if !ok { + return "", apierrors.NewBadRequest("namespace required") + }
189-196: Stop underlying watch to avoid leaksSame as other RESTs: defer base.Stop().
- base, err := r.w.Watch(ctx, secList, &client.ListOptions{Namespace: ns, Raw: &metav1.ListOptions{ + base, err := r.w.Watch(ctx, secList, &client.ListOptions{Namespace: ns, Raw: &metav1.ListOptions{ Watch: true, LabelSelector: ls, ResourceVersion: opts.ResourceVersion, }}) if err != nil { ... } ... - go func() { - defer proxy.Stop() + go func() { + defer func() { base.Stop(); proxy.Stop() }()pkg/registry/core/tenantnamespace/rest.go (1)
283-286: List RoleBindings with cache is fine; consider restricting fields for perfOptional: add FieldSelector to namespaces under consideration to reduce load.
pkg/registry/core/tenantmodule/rest.go (1)
386-488: Stop underlying HelmRelease watcher when custom watcher stopsAvoid goroutine/resource leak; tie lifecycle together.
- helmWatcher, err := r.w.Watch(ctx, hrList, &client.ListOptions{ + helmWatcher, err := r.w.Watch(ctx, hrList, &client.ListOptions{ Namespace: namespace, Raw: &metaOptions, }) ... customW := &customWatcher{ resultChan: make(chan watch.Event), stopChan: make(chan struct{}) } go func() { - defer close(customW.resultChan) + defer func() { helmWatcher.Stop(); close(customW.resultChan) }() for { select {pkg/registry/apps/application/rest.go (3)
79-87: Guard against nil watch client to avoid panic.If REST is constructed without a non-nil client.WithWatch, Watch() will panic. Add a nil check and return ServiceUnavailable.
func (r *REST) Watch(ctx context.Context, options *metainternalversion.ListOptions) (watch.Interface, error) { + if r.w == nil { + return nil, apierrors.NewServiceUnavailable("watch client not configured") + }
324-329: List: Raw selectors are ignored by cache-backed client; use MatchingLabelsSelector to prefilter.With controller-runtime’s cached client, ListOptions.Raw is not applied; you correctly re-filter after conversion, but you can reduce work by also passing MatchingLabelsSelector. For fields, only indexed fields are efficient; keep post-filter as fallback.
-hrList := &helmv2.HelmReleaseList{} -err = r.c.List(ctx, hrList, &client.ListOptions{ - Namespace: namespace, - Raw: &metaOptions, -}) +hrList := &helmv2.HelmReleaseList{} +listOpts := []client.ListOption{client.InNamespace(namespace)} +if helmLabelSelector != "" { + if sel, perr := labels.Parse(helmLabelSelector); perr == nil { + listOpts = append(listOpts, client.MatchingLabelsSelector{Selector: sel}) + } +} +// Keep post-conversion filtering for correctness; field selectors via cache may require indexers. +err = r.c.List(ctx, hrList, listOpts...)Also applies to: 338-389
985-1028: Avoid setting server-managed fields in converter; prefer typed constants for Flux.
- Converter sets UID/ResourceVersion; that’s risky for Create. Either clear in Create (as suggested) or make the converter omit server-managed fields.
- Use helmv2.ReconcileStrategyRevision constant instead of the raw string.
ObjectMeta: metav1.ObjectMeta{ Name: r.releaseConfig.Prefix + app.Name, Namespace: app.Namespace, Labels: addPrefixedMap(app.Labels, LabelPrefix), Annotations: addPrefixedMap(app.Annotations, AnnotationPrefix), - ResourceVersion: app.ObjectMeta.ResourceVersion, - UID: app.ObjectMeta.UID, + // ResourceVersion and UID are server-managed; set during Update path, not in generic converter. }, Spec: helmv2.HelmReleaseSpec{ Chart: &helmv2.HelmChartTemplate{ Spec: helmv2.HelmChartTemplateSpec{ Chart: r.releaseConfig.Chart.Name, Version: ">= 0.0.0-0", - ReconcileStrategy: "Revision", + ReconcileStrategy: helmv2.ReconcileStrategyRevision,If you want to preserve ResourceVersion for Update, consider passing a flag to the converter or setting it only in the Update path.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
pkg/apiserver/apiserver.go(6 hunks)pkg/registry/apps/application/rest.go(19 hunks)pkg/registry/core/tenantmodule/rest.go(9 hunks)pkg/registry/core/tenantnamespace/rest.go(7 hunks)pkg/registry/core/tenantsecret/rest.go(10 hunks)pkg/registry/core/tenantsecretstable/rest.go(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
pkg/registry/core/tenantmodule/rest.go (4)
pkg/registry/apps/application/rest.go (2)
NewREST(90-130)REST(78-87)pkg/registry/core/tenantnamespace/rest.go (2)
NewREST(55-68)REST(49-53)pkg/registry/core/tenantsecret/rest.go (8)
NewREST(165-175)REST(159-163)REST(181-181)REST(182-182)REST(183-185)REST(186-186)REST(190-190)REST(454-454)pkg/registry/core/tenantsecretstable/rest.go (8)
NewREST(46-56)REST(40-44)REST(68-68)REST(69-69)REST(70-72)REST(73-73)REST(77-77)REST(78-78)
pkg/registry/core/tenantsecretstable/rest.go (3)
pkg/registry/apps/application/rest.go (2)
NewREST(90-130)REST(78-87)pkg/registry/core/tenantmodule/rest.go (2)
NewREST(82-98)REST(72-79)pkg/registry/core/tenantnamespace/rest.go (8)
NewREST(55-68)REST(49-53)REST(74-74)REST(75-75)REST(76-78)REST(79-79)REST(83-83)REST(328-328)
pkg/registry/core/tenantsecret/rest.go (4)
pkg/registry/apps/application/rest.go (2)
NewREST(90-130)REST(78-87)pkg/registry/core/tenantmodule/rest.go (2)
NewREST(82-98)REST(72-79)pkg/registry/core/tenantnamespace/rest.go (8)
NewREST(55-68)REST(49-53)REST(74-74)REST(75-75)REST(76-78)REST(79-79)REST(83-83)REST(328-328)pkg/registry/core/tenantsecretstable/rest.go (8)
NewREST(46-56)REST(40-44)REST(68-68)REST(69-69)REST(70-72)REST(73-73)REST(77-77)REST(78-78)
pkg/apiserver/apiserver.go (6)
pkg/registry/registry.go (1)
RESTInPeace(33-33)pkg/registry/apps/application/rest.go (1)
NewREST(90-130)pkg/registry/core/tenantmodule/rest.go (1)
NewREST(82-98)pkg/registry/core/tenantnamespace/rest.go (1)
NewREST(55-68)pkg/registry/core/tenantsecret/rest.go (1)
NewREST(165-175)pkg/registry/core/tenantsecretstable/rest.go (1)
NewREST(46-56)
pkg/registry/apps/application/rest.go (7)
pkg/registry/core/tenantmodule/rest.go (2)
NewREST(82-98)REST(72-79)pkg/registry/core/tenantnamespace/rest.go (8)
NewREST(55-68)REST(49-53)REST(74-74)REST(75-75)REST(76-78)REST(79-79)REST(83-83)REST(328-328)pkg/registry/core/tenantsecret/rest.go (8)
NewREST(165-175)REST(159-163)REST(181-181)REST(182-182)REST(183-185)REST(186-186)REST(190-190)REST(454-454)pkg/registry/core/tenantsecretstable/rest.go (2)
NewREST(46-56)REST(40-44)pkg/config/config.go (1)
Resource(25-28)api/v1alpha1/cozystackresourcedefinitions_types.go (1)
SourceRef(71-80)pkg/apis/apps/v1alpha1/types.go (1)
Application(59-67)
pkg/registry/core/tenantnamespace/rest.go (4)
pkg/registry/apps/application/rest.go (2)
NewREST(90-130)REST(78-87)pkg/registry/core/tenantmodule/rest.go (2)
NewREST(82-98)REST(72-79)pkg/registry/core/tenantsecret/rest.go (8)
NewREST(165-175)REST(159-163)REST(181-181)REST(182-182)REST(183-185)REST(186-186)REST(190-190)REST(454-454)pkg/registry/core/tenantsecretstable/rest.go (8)
NewREST(46-56)REST(40-44)REST(68-68)REST(69-69)REST(70-72)REST(73-73)REST(77-77)REST(78-78)
⏰ 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 (6)
pkg/apiserver/apiserver.go (1)
147-155: Informer presence check is good; consider also Secrets, Namespaces, Services, RoleBindings resync dependenceMinor: Align with syncPeriod; nothing to change now.
pkg/registry/core/tenantmodule/rest.go (1)
510-559: hasTenantModuleLabel/Convert helpers look goodClear typing and label filtering.
pkg/registry/apps/application/rest.go (4)
90-130: Constructor changes LGTM.Signature and stored fields align with other REST constructors in this PR. Based on learnings.
205-228: Get flow LGTM, including NotFound translation and inclusion filter.Typed Get via cache-backed client and proper NotFound mapping look correct.
41-41: Controller-runtime client adoption is properly aligned with k8s.io/ dependencies.*Verified: sigs.k8s.io/controller-runtime v0.19.0 is correctly paired with k8s.io/* v0.31.2 packages (api, apimachinery, client-go).
916-932: No issues found.The
applySpecDefaultsmethod is defined inpkg/registry/apps/application/rest_defaulting.goat line 29 with the correct signature. The method is properly wired to the REST struct and correctly accepts an Application pointer, matching the call at line 926 in rest.go.
| // Ensure tenant secret label is preserved | ||
| if out.Labels[tsLabelKey] != tsLabelValue { | ||
| out.Labels[tsLabelKey] = tsLabelValue | ||
| out, _ = r.core.Secrets(ns).Update(ctx, out, metav1.UpdateOptions{}) | ||
| _ = r.c.Update(ctx, out, &client.UpdateOptions{Raw: &metav1.UpdateOptions{}}) | ||
| } |
There was a problem hiding this comment.
Potential panic: write into nil Labels after Patch
out.Labels can be nil; assignment will panic. Initialize before setting and handle Update error.
- if out.Labels[tsLabelKey] != tsLabelValue {
- out.Labels[tsLabelKey] = tsLabelValue
- _ = r.c.Update(ctx, out, &client.UpdateOptions{Raw: &metav1.UpdateOptions{}})
- }
+ if out.Labels == nil || out.Labels[tsLabelKey] != tsLabelValue {
+ if out.Labels == nil {
+ out.Labels = map[string]string{}
+ }
+ out.Labels[tsLabelKey] = tsLabelValue
+ if uerr := r.c.Update(ctx, out, &client.UpdateOptions{Raw: &metav1.UpdateOptions{}}); uerr != nil {
+ return nil, uerr
+ }
+ }📝 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 tenant secret label is preserved | |
| if out.Labels[tsLabelKey] != tsLabelValue { | |
| out.Labels[tsLabelKey] = tsLabelValue | |
| out, _ = r.core.Secrets(ns).Update(ctx, out, metav1.UpdateOptions{}) | |
| _ = r.c.Update(ctx, out, &client.UpdateOptions{Raw: &metav1.UpdateOptions{}}) | |
| } | |
| // Ensure tenant secret label is preserved | |
| if out.Labels == nil || out.Labels[tsLabelKey] != tsLabelValue { | |
| if out.Labels == nil { | |
| out.Labels = map[string]string{} | |
| } | |
| out.Labels[tsLabelKey] = tsLabelValue | |
| if uerr := r.c.Update(ctx, out, &client.UpdateOptions{Raw: &metav1.UpdateOptions{}}); uerr != nil { | |
| return nil, uerr | |
| } | |
| } |
🤖 Prompt for AI Agents
In pkg/registry/core/tenantsecret/rest.go around lines 362 to 366, the code
assigns to out.Labels[tsLabelKey] without ensuring out.Labels is non-nil which
can panic; before setting the label, initialize out.Labels = map[string]string{}
if it is nil, set the label, then call r.c.Update and capture its error (do not
ignore it) — handle the error by returning it or logging appropriately so
failures are surfaced instead of being discarded.
8cc3489 to
4d819e5
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
pkg/apiserver/apiserver.go (1)
38-41: Wrong import path for core API install.
coreinstallpoints to apps/install; core types won't be registered.- appsinstall "github.com/cozystack/cozystack/pkg/apis/apps/install" - coreinstall "github.com/cozystack/cozystack/pkg/apis/apps/install" + appsinstall "github.com/cozystack/cozystack/pkg/apis/apps/install" + coreinstall "github.com/cozystack/cozystack/pkg/apis/core/install"pkg/registry/core/tenantsecretstable/rest.go (1)
280-296: Fix object identity: use composed name.List/Watch return items named after Secret, but Get expects “secret-key”. Set metadata.name to composedName to make API consistent.
- Name: sec.Name, + Name: composedName(sec.Name, key),pkg/registry/core/tenantsecret/rest.go (1)
294-319: Fix NotFound/forceCreate path in Update.
curis allocated before Get, socur == nilis never true. On NotFound you should create (if allowed) instead of updating with empty ResourceVersion.- cur := &corev1.Secret{} - err = r.c.Get(ctx, types.NamespacedName{Namespace: ns, Name: name}, cur, &client.GetOptions{Raw: &metav1.GetOptions{}}) - if err != nil && !apierrors.IsNotFound(err) { - return nil, false, err - } + cur := &corev1.Secret{} + err = r.c.Get(ctx, types.NamespacedName{Namespace: ns, Name: name}, cur, &client.GetOptions{Raw: &metav1.GetOptions{}}) + if err != nil && !apierrors.IsNotFound(err) { + return nil, false, err + } @@ - newSec := tenantToSecret(in, cur) + var existing *corev1.Secret + if apierrors.IsNotFound(err) { + existing = nil + } else { + existing = cur + } + newSec := tenantToSecret(in, existing) newSec.Namespace = ns - if cur == nil { - if !forceCreate && err == nil { - return nil, false, apierrors.NewNotFound(r.gvr.GroupResource(), name) - } - err := r.c.Create(ctx, newSec, &client.CreateOptions{Raw: &metav1.CreateOptions{}}) - return secretToTenant(newSec), true, err - } + if apierrors.IsNotFound(err) { + if !forceCreate { + return nil, false, apierrors.NewNotFound(r.gvr.GroupResource(), name) + } + cerr := r.c.Create(ctx, newSec, &client.CreateOptions{Raw: &metav1.CreateOptions{}}) + return secretToTenant(newSec), true, cerr + } newSec.ResourceVersion = cur.ResourceVersion err = r.c.Update(ctx, newSec, &client.UpdateOptions{Raw: opts}) return secretToTenant(newSec), false, err
♻️ Duplicate comments (2)
pkg/registry/core/tenantsecret/rest.go (1)
362-370: Don’t ignore Update error after enforcing label (duplicate of earlier feedback).Handle and surface the error to avoid silently losing the tenant label.
if out.Labels[tsLabelKey] != tsLabelValue { out.Labels[tsLabelKey] = tsLabelValue - _ = r.c.Update(ctx, out, &client.UpdateOptions{Raw: &metav1.UpdateOptions{}}) + if uerr := r.c.Update(ctx, out, &client.UpdateOptions{Raw: &metav1.UpdateOptions{}}); uerr != nil { + return nil, uerr + } }pkg/registry/apps/application/rest.go (1)
649-783: Watch leak fixed as requested.The watch resource leak identified in the previous review has been properly addressed:
- The
underlyingwatch is now stored incustomWatcher(line 663, 767)- It's stopped via
deferwhen the goroutine exits (line 668)- The
Stop()method now stops the underlying watch (lines 774-776)This ensures the server-side watch and goroutine are properly cleaned up.
🧹 Nitpick comments (9)
pkg/registry/core/tenantmodule/rest.go (4)
64-69: Remove unused variablehelmReleaseGVR.It's not referenced; keep code lean.
-// Define the GroupVersionResource for HelmRelease -var helmReleaseGVR = schema.GroupVersionResource{ - Group: "helm.toolkit.fluxcd.io", - Version: "v2", - Resource: "helmreleases", -}
150-166: Avoid duplicate TypeMeta and prefer version constant.TypeMeta is already set in convertHelmReleaseToTenantModule; also use corev1alpha1.SchemeGroupVersion for safety.
- // Explicitly set apiVersion and kind for TenantModule - convertedModule.TypeMeta = metav1.TypeMeta{ - APIVersion: "core.cozystack.io/v1alpha1", - Kind: r.kindName, - } - - // Convert TenantModule to unstructured format + // Convert TenantModule to unstructured format unstructuredModule, err := runtime.DefaultUnstructuredConverter.ToUnstructured(&convertedModule) @@ - // Explicitly set apiVersion and kind in unstructured object - unstructuredModule["apiVersion"] = "core.cozystack.io/v1alpha1" - unstructuredModule["kind"] = r.kindNameAnd in the converter:
- TypeMeta: metav1.TypeMeta{ - APIVersion: "core.cozystack.io/v1alpha1", - Kind: r.kindName, - }, + TypeMeta: metav1.TypeMeta{ + APIVersion: corev1alpha1.SchemeGroupVersion.String(), + Kind: r.kindName, + },Also applies to: 579-585
137-141: Downgrade log level for label-miss.Missing internal label is a normal filter, not an error. Use V(4) Info.
- klog.Errorf("HelmRelease %s does not have the required label %s=%s", name, TenantModuleLabelKey, TenantModuleLabelValue) + klog.V(4).Infof("HelmRelease %s missing label %s=%s", name, TenantModuleLabelKey, TenantModuleLabelValue)
411-415: Align comment with behavior or implement retry.The code returns on closed channel; either remove “re-establish” comment or add retry/backoff.
pkg/apiserver/apiserver.go (1)
131-135: Update stale comment.Reflect manager/clients creation instead of “dynamic client”.
- // Create a dynamic client for HelmRelease using InClusterConfig. + // Build controller-runtime Manager and clients using the cluster config.pkg/registry/core/tenantnamespace/rest.go (2)
89-112: Support selectors in List (filter after read).You ignore ListOptions; implement label/field filtering after r.c.List for parity with kubernetes semantics.
-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) { nsList := &corev1.NamespaceList{} err := r.c.List(ctx, nsList) @@ - return r.makeList(nsList, allowed), nil + out := r.makeList(nsList, allowed) + // Optional: apply label/field filters here using opts (parse and match). + return out, nil
142-151: Watch: consider honoring client selectors.Plumb LabelSelector through and filter by FieldSelector inside the loop to reduce noise.
Also applies to: 155-177
pkg/registry/core/tenantsecret/rest.go (1)
300-306: Optional: pass old object to UpdatedObject.Provide the previous TenantSecret to admission/strategies for better validations/merges.
oldTenant := (*corev1alpha1.TenantSecret)(nil) if !apierrors.IsNotFound(err) { oldTenant = secretToTenant(cur) } newObj, err := objInfo.UpdatedObject(ctx, oldTenant)pkg/registry/apps/application/rest.go (1)
809-814: Consider removing redundant nil check.The nil check for
hr.Spec.Chartat lines 811-814 is redundant becauseshouldIncludeHelmReleasealready performs this check at lines 788-791 before callingmatchesSourceRefAndPrefix(line 805).While defensive programming is valuable, removing this duplication would reduce cognitive load. If you want
matchesSourceRefAndPrefixto be safely callable independently, consider adding a comment explaining the defensive check.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
pkg/apiserver/apiserver.go(6 hunks)pkg/registry/apps/application/rest.go(21 hunks)pkg/registry/core/tenantmodule/rest.go(11 hunks)pkg/registry/core/tenantnamespace/rest.go(7 hunks)pkg/registry/core/tenantsecret/rest.go(10 hunks)pkg/registry/core/tenantsecretstable/rest.go(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
pkg/apiserver/apiserver.go (6)
pkg/registry/registry.go (1)
RESTInPeace(33-33)pkg/registry/apps/application/rest.go (1)
NewREST(90-130)pkg/registry/core/tenantmodule/rest.go (1)
NewREST(82-98)pkg/registry/core/tenantnamespace/rest.go (1)
NewREST(55-68)pkg/registry/core/tenantsecret/rest.go (1)
NewREST(165-175)pkg/registry/core/tenantsecretstable/rest.go (1)
NewREST(46-56)
pkg/registry/core/tenantnamespace/rest.go (4)
pkg/registry/apps/application/rest.go (2)
NewREST(90-130)REST(78-87)pkg/registry/core/tenantmodule/rest.go (2)
NewREST(82-98)REST(72-79)pkg/registry/core/tenantsecret/rest.go (8)
NewREST(165-175)REST(159-163)REST(181-181)REST(182-182)REST(183-185)REST(186-186)REST(190-190)REST(458-458)pkg/registry/core/tenantsecretstable/rest.go (8)
NewREST(46-56)REST(40-44)REST(68-68)REST(69-69)REST(70-72)REST(73-73)REST(77-77)REST(78-78)
pkg/registry/core/tenantsecretstable/rest.go (3)
pkg/registry/apps/application/rest.go (2)
NewREST(90-130)REST(78-87)pkg/registry/core/tenantmodule/rest.go (2)
NewREST(82-98)REST(72-79)pkg/registry/core/tenantnamespace/rest.go (8)
NewREST(55-68)REST(49-53)REST(74-74)REST(75-75)REST(76-78)REST(79-79)REST(83-83)REST(328-328)
pkg/registry/core/tenantsecret/rest.go (4)
pkg/registry/apps/application/rest.go (2)
NewREST(90-130)REST(78-87)pkg/registry/core/tenantmodule/rest.go (2)
NewREST(82-98)REST(72-79)pkg/registry/core/tenantnamespace/rest.go (8)
NewREST(55-68)REST(49-53)REST(74-74)REST(75-75)REST(76-78)REST(79-79)REST(83-83)REST(328-328)pkg/registry/core/tenantsecretstable/rest.go (8)
NewREST(46-56)REST(40-44)REST(68-68)REST(69-69)REST(70-72)REST(73-73)REST(77-77)REST(78-78)
pkg/registry/core/tenantmodule/rest.go (3)
pkg/registry/apps/application/rest.go (2)
NewREST(90-130)REST(78-87)pkg/registry/core/tenantnamespace/rest.go (2)
NewREST(55-68)REST(49-53)pkg/registry/core/tenantsecret/rest.go (8)
NewREST(165-175)REST(159-163)REST(181-181)REST(182-182)REST(183-185)REST(186-186)REST(190-190)REST(458-458)
pkg/registry/apps/application/rest.go (4)
pkg/registry/core/tenantmodule/rest.go (2)
NewREST(82-98)REST(72-79)pkg/config/config.go (1)
Resource(25-28)api/v1alpha1/cozystackresourcedefinitions_types.go (1)
SourceRef(71-80)pkg/apis/apps/v1alpha1/types.go (1)
Application(59-67)
⏰ 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 (5)
pkg/apiserver/apiserver.go (1)
157-165: Manager start and cache sync: LGTM.Proper Start error handling and WaitForCacheSync added; lifecycle tied to one context.
pkg/registry/core/tenantnamespace/rest.go (1)
123-135: Get via controller-runtime client for Namespaces: LGTM.Cluster-scoped get with empty namespace and Raw options is correct.
pkg/registry/apps/application/rest.go (3)
78-130: LGTM: Clean migration to typed clients.The REST struct and constructor have been properly updated to use
client.Clientandclient.WithWatchfrom controller-runtime, replacing the previous dynamic client approach. This aligns well with the PR objectives to improve performance with cache-backed clients.
143-574: LGTM: CRUD operations correctly use typed clients.All CRUD operations (Create, Get, List, Update, Delete) have been properly migrated to use the typed
client.Clientwith appropriate error handling:
- Proper use of
client.ObjectKeyfor Get operations- Correct use of
client.CreateOptions,client.GetOptions,client.ListOptions,client.UpdateOptions, andclient.DeleteOptions- Type-safe operations with
*helmv2.HelmReleaseinstead of unstructured objects- Appropriate filtering with
shouldIncludeHelmReleaseto ensure only relevant resources are exposedThe implementation aligns with the PR objective to replace unstructured/dynamic clients with typed, cache-backed clients.
922-1034: LGTM: Conversion functions updated for typed clients.The conversion functions have been correctly updated to accept and work with
*helmv2.HelmReleaseinstead of unstructured objects. The conversion logic remains sound, properly mapping between HelmRelease and Application resources while handling labels, annotations, status conditions, and metadata appropriately.
| list := &corev1.SecretList{} | ||
| err = r.c.List(ctx, list, | ||
| &client.ListOptions{ | ||
| Namespace: ns, | ||
| Raw: &metav1.ListOptions{ | ||
| LabelSelector: labels.Set{tsLabelKey: tsLabelValue}.AsSelector().String(), | ||
| FieldSelector: fieldSel, | ||
| }, | ||
| }) | ||
| if err != nil { |
There was a problem hiding this comment.
Honor label/field selectors; don’t push FieldSelector down to Secrets.
- You build
selbut pass only the internal label; user LabelSelector is ignored. - FieldSelector on TenantSecretsTable refers to composed names; pushing it to Secret list is wrong. Filter after materialization.
- err = r.c.List(ctx, list,
- &client.ListOptions{
- Namespace: ns,
- Raw: &metav1.ListOptions{
- LabelSelector: labels.Set{tsLabelKey: tsLabelValue}.AsSelector().String(),
- FieldSelector: fieldSel,
- },
- })
+ err = r.c.List(ctx, list, &client.ListOptions{
+ Namespace: ns,
+ Raw: &metav1.ListOptions{
+ LabelSelector: sel.String(),
+ // FieldSelector applies to Secrets, not flattened entries; filter below.
+ },
+ })
@@
for i := range list.Items {
sec := &list.Items[i]
@@
}
- sort.Slice(out.Items, func(i, j int) bool { return out.Items[i].Name < out.Items[j].Name })
+ // Apply field selector on composed names if provided
+ if opts.FieldSelector != nil && !opts.FieldSelector.Empty() {
+ fs, ferr := fields.ParseSelector(opts.FieldSelector.String())
+ if ferr == nil {
+ filtered := out.Items[:0]
+ for i := range out.Items {
+ fset := fields.Set{
+ "metadata.name": out.Items[i].Name,
+ "metadata.namespace": out.Items[i].Namespace,
+ }
+ if fs.Matches(fset) {
+ filtered = append(filtered, out.Items[i])
+ }
+ }
+ out.Items = filtered
+ }
+ }
+ sort.Slice(out.Items, func(i, j int) bool { return out.Items[i].Name < out.Items[j].Name })Add import:
import "k8s.io/apimachinery/pkg/fields"Also applies to: 160-176
🤖 Prompt for AI Agents
In pkg/registry/core/tenantsecretstable/rest.go around lines 142 to 151 (and
similarly lines 160–176), the current List call only uses the internal label
selector and ignores the caller's combined label/field selector, and it
incorrectly pushes a TenantSecretsTable FieldSelector down to the Secret list;
instead, pass the full user selector (labels+fields) when listing Secrets for
label filtering only, then materialize Secrets and apply the FieldSelector
filtering in Go against the composed TenantSecret names; add the import
"k8s.io/apimachinery/pkg/fields" and ensure you build and use sel :=
labelsAndFieldsSelector (or equivalent) for labels when calling r.c.List, remove
passing FieldSelector into the ListOptions.Raw for Secrets, and post-process the
returned SecretList to apply the FieldSelector logic that maps Secret metadata
to TenantSecretsTable rows.
| secList := &corev1.SecretList{} | ||
| ls := labels.Set{tsLabelKey: tsLabelValue}.AsSelector().String() | ||
| base, err := r.core.Secrets(ns).Watch(ctx, metav1.ListOptions{ | ||
| base, err := r.w.Watch(ctx, secList, &client.ListOptions{Namespace: ns, Raw: &metav1.ListOptions{ | ||
| Watch: true, | ||
| LabelSelector: ls, | ||
| ResourceVersion: opts.ResourceVersion, | ||
| }) | ||
| }}) | ||
| if err != nil { |
There was a problem hiding this comment.
Watch: include user selectors and filter by FieldSelector; stop base watcher.
Currently only internal label is used; FieldSelector on composed names is ignored.
- secList := &corev1.SecretList{}
- ls := labels.Set{tsLabelKey: tsLabelValue}.AsSelector().String()
- base, err := r.w.Watch(ctx, secList, &client.ListOptions{Namespace: ns, Raw: &metav1.ListOptions{
- Watch: true,
- LabelSelector: ls,
- ResourceVersion: opts.ResourceVersion,
- }})
+ secList := &corev1.SecretList{}
+ sel := labels.NewSelector()
+ req, _ := labels.NewRequirement(tsLabelKey, selection.Equals, []string{tsLabelValue})
+ sel = sel.Add(*req)
+ if opts.LabelSelector != nil {
+ if reqs, _ := opts.LabelSelector.Requirements(); len(reqs) > 0 {
+ sel = sel.Add(reqs...)
+ }
+ }
+ base, err := r.w.Watch(ctx, secList, &client.ListOptions{Namespace: ns, Raw: &metav1.ListOptions{
+ Watch: true,
+ LabelSelector: sel.String(),
+ ResourceVersion: opts.ResourceVersion,
+ }})
@@
- go func() {
- defer proxy.Stop()
+ go func() {
+ defer proxy.Stop()
+ defer base.Stop()
for ev := range base.ResultChan() {
@@
- for k, v := range sec.Data {
- obj := secretKeyToObj(sec, k, v)
- ch <- watch.Event{Type: ev.Type, Object: obj}
- }
+ for k, v := range sec.Data {
+ obj := secretKeyToObj(sec, k, v)
+ // FieldSelector on composed names
+ if opts.FieldSelector != nil && !opts.FieldSelector.Empty() {
+ if fs, ferr := fields.ParseSelector(opts.FieldSelector.String()); ferr == nil {
+ fset := fields.Set{
+ "metadata.name": obj.Name,
+ "metadata.namespace": obj.Namespace,
+ }
+ if !fs.Matches(fset) {
+ continue
+ }
+ }
+ }
+ ch <- watch.Event{Type: ev.Type, Object: obj}
+ }
}
}()Add import:
import "k8s.io/apimachinery/pkg/fields"Also applies to: 203-216
This patch changes all clients in the Cozystack API server to typed ones from the controller runtime. This should improve the performance of the API server and simplifies the code by removing work with unstructured objects and dynamic clients. ```release-note [api] Use typed and cache-backed k8s clients in the Cozystack API to improve performance. Get rid of operations on unstructured objects and use of dynamic clients. ``` Signed-off-by: Timofei Larkin <lllamnyp@gmail.com>
4d819e5 to
f4e0145
Compare
## What this PR does This patch changes all clients in the Cozystack API server to typed ones from the controller runtime. This should improve the performance of the API server and simplifies the code by removing work with unstructured objects and dynamic clients. ### Release note ```release-note [api] Use typed and cache-backed k8s clients in the Cozystack API to improve performance. Get rid of operations on unstructured objects and use of dynamic clients. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Refactor** * Backend migrated to a controller-runtime manager with typed clients for Kubernetes resources, improving watch reliability and cache sync. * Storage paths for applications, tenant modules, namespaces, and secrets now use strongly-typed resource handling for more consistent behavior. * **Chores** * Cluster role expanded to include services in core API permissions. * **Notes** * No user-facing API schema changes. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
What this PR does
This patch changes all clients in the Cozystack API server to typed ones from the controller runtime. This should improve the performance of the API server and simplifies the code by removing work with unstructured objects and dynamic clients.
Release note
Summary by CodeRabbit
Refactor
Chores
Notes