From 0377bd3d291cb3f3f97273496f5544cc92c187cc Mon Sep 17 00:00:00 2001 From: Andrei Kvapil Date: Thu, 18 Dec 2025 15:31:09 +0100 Subject: [PATCH] Fix review comments Signed-off-by: Andrei Kvapil --- .../cozystackresource_controller.go | 62 ++++++-------- pkg/apis/apps/v1alpha1/types.go | 7 ++ pkg/registry/apps/application/rest.go | 82 ++++++++++--------- 3 files changed, 76 insertions(+), 75 deletions(-) diff --git a/internal/controller/cozystackresource_controller.go b/internal/controller/cozystackresource_controller.go index cc237fba..21585c6f 100644 --- a/internal/controller/cozystackresource_controller.go +++ b/internal/controller/cozystackresource_controller.go @@ -39,24 +39,10 @@ type CozystackResourceDefinitionReconciler struct { } func (r *CozystackResourceDefinitionReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { - logger := log.FromContext(ctx) - - // List all CozystackResourceDefinitions - crdList := &cozyv1alpha1.CozystackResourceDefinitionList{} - if err := r.List(ctx, crdList); err != nil { - logger.Error(err, "failed to list CozystackResourceDefinitions") + if err := r.reconcileCozyRDAndUpdateHelmReleases(ctx); err != nil { return ctrl.Result{}, err } - // Update HelmReleases for each CRD - for i := range crdList.Items { - crd := &crdList.Items[i] - if err := r.updateHelmReleasesForCRD(ctx, crd); err != nil { - logger.Error(err, "failed to update HelmReleases for CRD", "crd", crd.Name) - // Continue with other CRDs even if one fails - } - } - // Continue with debounced restart logic return r.debouncedRestart(ctx) } @@ -82,29 +68,6 @@ func (r *CozystackResourceDefinitionReconciler) SetupWithManager(mgr ctrl.Manage }} }), ). - Watches( - &helmv2.HelmRelease{}, - handler.EnqueueRequestsFromMapFunc(func(ctx context.Context, obj client.Object) []reconcile.Request { - hr, ok := obj.(*helmv2.HelmRelease) - if !ok { - return nil - } - // Only watch HelmReleases with cozystack.io/ui=true label - if hr.Labels == nil || hr.Labels["cozystack.io/ui"] != "true" { - return nil - } - // Trigger reconciliation of all CRDs when a HelmRelease with the label is created/updated - r.mu.Lock() - r.lastEvent = time.Now() - r.mu.Unlock() - return []reconcile.Request{{ - NamespacedName: types.NamespacedName{ - Namespace: "cozy-system", - Name: "cozystack-api", - }, - }} - }), - ). Complete(r) } @@ -230,6 +193,29 @@ func sortCozyRDs(a, b cozyv1alpha1.CozystackResourceDefinition) int { return 1 } +// reconcileCozyRDAndUpdateHelmReleases reconciles all CozystackResourceDefinitions and updates HelmReleases from them +func (r *CozystackResourceDefinitionReconciler) reconcileCozyRDAndUpdateHelmReleases(ctx context.Context) error { + logger := log.FromContext(ctx) + + // List all CozystackResourceDefinitions + crdList := &cozyv1alpha1.CozystackResourceDefinitionList{} + if err := r.List(ctx, crdList); err != nil { + logger.Error(err, "failed to list CozystackResourceDefinitions") + return err + } + + // Update HelmReleases for each CRD + for i := range crdList.Items { + crd := &crdList.Items[i] + if err := r.updateHelmReleasesForCRD(ctx, crd); err != nil { + logger.Error(err, "failed to update HelmReleases for CRD", "crd", crd.Name) + // Continue with other CRDs even if one fails + } + } + + return nil +} + // updateHelmReleasesForCRD updates all HelmReleases that match the application labels from CozystackResourceDefinition func (r *CozystackResourceDefinitionReconciler) updateHelmReleasesForCRD(ctx context.Context, crd *cozyv1alpha1.CozystackResourceDefinition) error { logger := log.FromContext(ctx) diff --git a/pkg/apis/apps/v1alpha1/types.go b/pkg/apis/apps/v1alpha1/types.go index 8e756779..0118b0de 100644 --- a/pkg/apis/apps/v1alpha1/types.go +++ b/pkg/apis/apps/v1alpha1/types.go @@ -21,6 +21,13 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) +// Application label keys used to identify and filter HelmReleases +const ( + ApplicationKindLabel = "apps.cozystack.io/application.kind" + ApplicationGroupLabel = "apps.cozystack.io/application.group" + ApplicationNameLabel = "apps.cozystack.io/application.name" +) + // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object // ApplicationList is a list of Application objects. diff --git a/pkg/registry/apps/application/rest.go b/pkg/registry/apps/application/rest.go index 39c0ddb8..78ff8dbc 100644 --- a/pkg/registry/apps/application/rest.go +++ b/pkg/registry/apps/application/rest.go @@ -67,11 +67,11 @@ const ( AnnotationPrefix = "apps.cozystack.io-" ) -// Application label keys +// Application label keys - use constants from API package const ( - ApplicationKindLabel = "apps.cozystack.io/application.kind" - ApplicationGroupLabel = "apps.cozystack.io/application.group" - ApplicationNameLabel = "apps.cozystack.io/application.name" + ApplicationKindLabel = appsv1alpha1.ApplicationKindLabel + ApplicationGroupLabel = appsv1alpha1.ApplicationGroupLabel + ApplicationNameLabel = appsv1alpha1.ApplicationNameLabel ) // Define the GroupVersionResource for HelmRelease @@ -224,8 +224,7 @@ func (r *REST) Get(ctx context.Context, name string, options *metav1.GetOptions) } // Check if HelmRelease has required labels - if helmRelease.Labels == nil || helmRelease.Labels[ApplicationKindLabel] != r.kindName || - helmRelease.Labels[ApplicationGroupLabel] != r.gvk.Group { + if !r.hasRequiredApplicationLabels(helmRelease) { klog.Errorf("HelmRelease %s does not match the required application labels", helmReleaseName) // Return a NotFound error for the Application resource return nil, apierrors.NewNotFound(r.gvr.GroupResource(), name) @@ -338,27 +337,16 @@ func (r *REST) List(ctx context.Context, options *metainternalversion.ListOption return nil, err } - klog.V(6).Infof("Found %d HelmReleases with label selector, filtering by labels...", len(hrList.Items)) + klog.V(6).Infof("Found %d HelmReleases with label selector", len(hrList.Items)) // Initialize Application items array items := make([]appsv1alpha1.Application, 0, len(hrList.Items)) // Iterate over HelmReleases and convert to Applications - // Filter by labels to ensure only relevant HelmReleases are included - // This is a safety check in case label selectors don't work perfectly or HelmReleases were created without labels + // Note: All HelmReleases already match the required labels due to server-side label selector filtering for i := range hrList.Items { hr := &hrList.Items[i] - // Verify that HelmRelease has the required labels - if hr.Labels == nil || hr.Labels[ApplicationKindLabel] != r.kindName || - hr.Labels[ApplicationGroupLabel] != r.gvk.Group { - klog.V(6).Infof("Skipping HelmRelease %s - missing or incorrect application labels (kind: %s, expected: %s, group: %s, expected: %s)", - hr.GetName(), - getLabelValue(hr.Labels, ApplicationKindLabel), r.kindName, - getLabelValue(hr.Labels, ApplicationGroupLabel), r.gvk.Group) - continue - } - app, err := r.ConvertHelmReleaseToApplication(hr) if err != nil { klog.Errorf("Error converting HelmRelease %s to Application: %v", hr.GetName(), err) @@ -540,8 +528,7 @@ func (r *REST) Delete(ctx context.Context, name string, deleteValidation rest.Va } // Validate that the HelmRelease has required labels - if helmRelease.Labels == nil || helmRelease.Labels[ApplicationKindLabel] != r.kindName || - helmRelease.Labels[ApplicationGroupLabel] != r.gvk.Group { + if !r.hasRequiredApplicationLabelsWithName(helmRelease, name) { klog.Errorf("HelmRelease %s does not match the required application labels", helmReleaseName) // Return NotFound error for Application resource return nil, false, apierrors.NewNotFound(r.gvr.GroupResource(), name) @@ -601,6 +588,19 @@ func (r *REST) Watch(ctx context.Context, options *metainternalversion.ListOptio } // Process label.selector + // Always add application metadata label requirements + appKindReq, err := labels.NewRequirement(ApplicationKindLabel, selection.Equals, []string{r.kindName}) + if err != nil { + klog.Errorf("Error creating application kind label requirement: %v", err) + return nil, fmt.Errorf("error creating application kind label requirement: %v", err) + } + appGroupReq, err := labels.NewRequirement(ApplicationGroupLabel, selection.Equals, []string{r.gvk.Group}) + if err != nil { + klog.Errorf("Error creating application group label requirement: %v", err) + return nil, fmt.Errorf("error creating application group label requirement: %v", err) + } + labelRequirements := []labels.Requirement{*appKindReq, *appGroupReq} + if options.LabelSelector != nil { ls := options.LabelSelector.String() parsedLabels, err := labels.Parse(ls) @@ -620,9 +620,10 @@ func (r *REST) Watch(ctx context.Context, options *metainternalversion.ListOptio } prefixedReqs = append(prefixedReqs, *prefixedReq) } - helmLabelSelector = labels.NewSelector().Add(prefixedReqs...).String() + labelRequirements = append(labelRequirements, prefixedReqs...) } } + helmLabelSelector = labels.NewSelector().Add(labelRequirements...).String() // Set ListOptions for HelmRelease with selector mapping metaOptions := metav1.ListOptions{ @@ -676,13 +677,7 @@ func (r *REST) Watch(ctx context.Context, options *metainternalversion.ListOptio continue } - // Verify that HelmRelease has the required labels - if hr.Labels == nil || hr.Labels[ApplicationKindLabel] != r.kindName || - hr.Labels[ApplicationGroupLabel] != r.gvk.Group { - klog.V(6).Infof("Skipping HelmRelease %s in watch - missing or incorrect application labels", hr.GetName()) - continue - } - + // Note: All HelmReleases already match the required labels due to server-side label selector filtering // Convert HelmRelease to Application app, err := r.ConvertHelmReleaseToApplication(hr) if err != nil { @@ -768,6 +763,27 @@ func (r *REST) getNamespace(ctx context.Context) (string, error) { return namespace, nil } +// hasRequiredApplicationLabels checks if a HelmRelease has the required application labels +// matching the REST instance's kind and group +func (r *REST) hasRequiredApplicationLabels(hr *helmv2.HelmRelease) bool { + if hr.Labels == nil { + return false + } + return hr.Labels[ApplicationKindLabel] == r.kindName && + hr.Labels[ApplicationGroupLabel] == r.gvk.Group +} + +// hasRequiredApplicationLabelsWithName checks if a HelmRelease has the required application labels +// matching the REST instance's kind, group, and the specified application name +func (r *REST) hasRequiredApplicationLabelsWithName(hr *helmv2.HelmRelease, appName string) bool { + if hr.Labels == nil { + return false + } + return hr.Labels[ApplicationKindLabel] == r.kindName && + hr.Labels[ApplicationGroupLabel] == r.gvk.Group && + hr.Labels[ApplicationNameLabel] == appName +} + // mergeMaps combines two maps of labels or annotations func mergeMaps(a, b map[string]string) map[string]string { if a == nil && b == nil { @@ -816,14 +832,6 @@ func filterPrefixedMap(original map[string]string, prefix string) map[string]str return processed } -// getLabelValue safely gets a label value, returning empty string if labels is nil or key doesn't exist -func getLabelValue(labels map[string]string, key string) string { - if labels == nil { - return "" - } - return labels[key] -} - // ConvertHelmReleaseToApplication converts a HelmRelease to an Application func (r *REST) ConvertHelmReleaseToApplication(hr *helmv2.HelmRelease) (appsv1alpha1.Application, error) { klog.V(6).Infof("Converting HelmRelease to Application for resource %s", hr.GetName())