[apps] Refactor apiserver to use typed objects and fix UnstructuredList GVK - #1679

pkg/registry/core/tenantmodule/rest.go (3)

149-157: Log message uses kind wording but logs Resource

The log at Line 155 says “kind %s” but passes r.gvr.Resource, which is the resource name, not the kind. This is harmless but slightly misleading when debugging.

You could either change the format string to say “resource %s” or log r.gvk.Kind instead.

---

236-305: Prefer consistent TypeMeta construction and reuse NewList

List currently:

• Builds []corev1alpha1.TenantModule and sets each item’s TypeMeta with a hard-coded APIVersion string.
• Constructs TenantModuleList directly and sets TypeMeta.Kind to r.kindName + "List" with a hard-coded APIVersion.

Functionally this works, but you can simplify and make it more robust against future group/version changes:

• Use r.gvk.GroupVersion().String() instead of "core.cozystack.io/v1alpha1" when setting TypeMeta.APIVersion.
• Consider creating the list via r.NewList().(*corev1alpha1.TenantModuleList) so Kind/APIVersion always stay aligned with your REST storage config.

Example:

-        module.TypeMeta = metav1.TypeMeta{
-            APIVersion: "core.cozystack.io/v1alpha1",
-            Kind:       r.kindName,
-        }
+        module.TypeMeta = metav1.TypeMeta{
+            APIVersion: r.gvk.GroupVersion().String(),
+            Kind:       r.kindName,
+        }

-    moduleList := &corev1alpha1.TenantModuleList{}
-    moduleList.TypeMeta = metav1.TypeMeta{
-        APIVersion: "core.cozystack.io/v1alpha1",
-        Kind:       r.kindName + "List",
-    }
+    moduleList := r.NewList().(*corev1alpha1.TenantModuleList)

---

448-458: Watch path TypeMeta is consistent but could also use r.gvk

Setting module.TypeMeta in the watch code ensures events carry the correct apiVersion/kind, which is important for clients. For consistency with New/NewList and future-proofing across version bumps, you might want to switch APIVersion here to r.gvk.GroupVersion().String() rather than a literal string.

module.TypeMeta = metav1.TypeMeta{
-    APIVersion: "core.cozystack.io/v1alpha1",
+    APIVersion: r.gvk.GroupVersion().String(),
    Kind:       r.kindName,
}

pkg/registry/apps/application/rest.go (6)

142-188: Create: type assertion is fine, but consider apierrors.BadRequest and honoring createValidation

The explicit assertion to *appsv1alpha1.Application is good, but two improvements are worth considering:

1. For a wrong type, returning a generic fmt.Errorf will likely surface as a 500. For API correctness, it’s more appropriate to return a 4xx, e.g.:

- app, ok := obj.(*appsv1alpha1.Application)
- if !ok {
-     return nil, fmt.Errorf("expected *appsv1alpha1.Application object, got %T", obj)
- }
+ app, ok := obj.(*appsv1alpha1.Application)
+ if !ok {
+     return nil, apierrors.NewBadRequest(
+         fmt.Sprintf("expected *appsv1alpha1.Application object, got %T", obj),
+     )
+ }

2. The createValidation hook is currently unused. If callers rely on this (e.g., admission-style validation before persistence), you should invoke it before converting to HelmRelease:

if createValidation != nil {
    if err := createValidation(ctx, app); err != nil {
        return nil, err
    }
}

Also, for consistency with New/NewList, you may want to use r.gvk.GroupVersion().String() instead of the literal "apps.cozystack.io/v1alpha1" when setting convertedApp.TypeMeta.APIVersion.

---

231-239: Get: logging message uses “kind” but logs Resource

Similar to the TenantModule REST, the log line:

klog.V(6).Infof("Successfully retrieved and converted resource %s of kind %s", name, r.gvr.Resource)

labels the second value as “kind” while passing r.gvr.Resource. Either log r.gvk.Kind or change the message to “resource %s” for clarity.

---

321-385: List: typed items/list look good; tighten APIVersion handling

The typed items slice and ApplicationList construction look correct and align with the move away from unstructured. A couple of small consistency improvements:

• When setting app.TypeMeta, prefer using r.gvk.GroupVersion().String() rather than a hard-coded "apps.cozystack.io/v1alpha1" string.
• You’re already using r.NewList().(*appsv1alpha1.ApplicationList) which nicely centralizes TypeMeta initialization; that’s good and consistent with NewList().

Example for APIVersion:

app.TypeMeta = metav1.TypeMeta{
-    APIVersion: "apps.cozystack.io/v1alpha1",
+    APIVersion: r.gvk.GroupVersion().String(),
    Kind:       r.kindName,
}

---

428-501: Update: type assertion and return path are fine; align error type and APIVersion

The newObj assertion to *appsv1alpha1.Application and the conversion/update flow look sound, but similar to Create:

• Consider returning apierrors.NewBadRequest on type mismatch instead of fmt.Errorf, so the client sees a proper 4xx.
• When setting convertedApp.TypeMeta.APIVersion, use r.gvk.GroupVersion().String() instead of a literal string, to avoid divergence if group/version changes.

The final log ("Returning updated Application object: %+v") is quite verbose at V(6); if these objects grow large, you may eventually want to trim it to key fields, but that’s optional.

---

694-704: Watch: TypeMeta assignment is correct; reuse r.gvk for APIVersion

The watch path correctly:

• Filters HelmReleases.
• Converts to Application.
• Sets TypeMeta.
• Wraps &app in the event object.

To stay consistent with New/NewList and avoid hard-coded versions, I’d recommend:

app.TypeMeta = metav1.TypeMeta{
-    APIVersion: "apps.cozystack.io/v1alpha1",
+    APIVersion: r.gvk.GroupVersion().String(),
    Kind:       r.kindName,
}

The Object: &app usage here is fine because app is a fresh variable per event, not a loop variable reused across iterations.

---

727-733: helmReleaseName helper is now typed; consider extending if more callers appear

The helper now assumes a *appsv1alpha1.Application and falls back to "<unknown>" for other types. This is reasonable for internal use; if you ever pass other runtime.Object implementations, you might want to either:

• Support additional known types explicitly, or
• Fail loudly (e.g., log and return empty) instead of returning a sentinel string.

For the current usage, this is acceptable.

hack/e2e-test-openapi.bats (1)

23-43: List-level kind assertions look correct and align with the GVK fix

The added checks for .kind == "TenantList" and .kind == "IngressList" are a good regression guard for the UnstructuredList GVK behavior and complement the existing item-level kind checks. You are calling kubectl get --raw twice per endpoint, which is fine; if you ever want to shave a bit of test time, you could capture the JSON once per resource and reuse it for both assertions.

packages/system/cozystack-api/images/cozystack-api/Dockerfile (1)

6-8: Check whether apimachinery/ + patches/ and patch are still needed

Given that go.mod now uses a replace k8s.io/apimachinery => github.com/cozystack/apimachinery ..., the Dockerfile’s comment about a “local replace” plus copying apimachinery/ and patches/ (and installing patch) may be leftover from a previous approach.

If the build no longer applies local patches or uses those directories, you can drop the apk add patch and the extra COPY layers (and update the comment) to simplify the image and slightly speed up builds.

Also applies to: 14-19

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

go.mod (1)

123-124: Ensure the replace directive is removed once upstream fix is released.

This replace directive is a temporary workaround for the UnstructuredList GVK fix from kubernetes/kubernetes#135537. Document a plan to remove this once the upstream fix is released and consumed.

pkg/registry/apps/application/rest.go (2)

322-322: Pre-allocate slice capacity for performance.

Initialize the items slice with capacity equal to len(hrList.Items) to avoid potential reallocations during append operations.

Apply this diff:

-	items := make([]appsv1alpha1.Application, 0)
+	items := make([]appsv1alpha1.Application, 0, len(hrList.Items))

---

180-184: Remove redundant TypeMeta assignment.

This TypeMeta assignment is redundant as it's already set within convertHelmReleaseToApplication (lines 921-924). This pattern appears in multiple methods (Create, Get, List, Update, Watch) and should be removed for cleaner code and to avoid potential inconsistencies.

pkg/registry/core/tenantmodule/rest.go (3)

236-237: Preallocate items slice to avoid reallocations

You can avoid potential reallocations in the hot List path by preallocating items with the known upper bound from hrList.Items (this matches earlier review feedback).

-	// Initialize TenantModule items array
-	items := make([]corev1alpha1.TenantModule, 0)
+	// Initialize TenantModule items array with capacity to avoid reallocations
+	items := make([]corev1alpha1.TenantModule, 0, len(hrList.Items))

Also applies to: 291-291

---

252-256: Avoid redundant TypeMeta assignment after conversion

convertHelmReleaseToTenantModule already initializes TypeMeta with APIVersion: "core.cozystack.io/v1alpha1" and Kind: r.kindName. Re‑assigning it here duplicates that responsibility and risks drift if the conversion helper is updated but this spot is not. Consider relying on the conversion function as the single source of truth and removing this block.

---

448-452: Reduce duplication of TypeMeta logic and avoid reusing the same module pointer across events

Two points here:

1. The TypeMeta assignment duplicates what convertHelmReleaseToTenantModule already does, same as in List. It’s better to keep that centralized in the converter.

2. module is a loop‑scoped variable reused across iterations of the watch goroutine. Taking &module and sending it on the channel means all events share the same underlying TenantModule instance, which can lead to subtle bugs if consumers keep references (they may observe the last value instead of the one at emission time). You can fix this by introducing a per‑iteration copy and taking its address:

-				// Explicitly set apiVersion and kind for TenantModule
-				module.TypeMeta = metav1.TypeMeta{
-					APIVersion: "core.cozystack.io/v1alpha1",
-					Kind:       r.kindName,
-				}
-
-				// Create watch event with TenantModule object
-				moduleEvent := watch.Event{
-					Type:   event.Type,
-					Object: &module,
-				}
+				// Optionally rely on conversion for TypeMeta and avoid pointer reuse across events
+				m := module // create a per-event copy to take the address of
+				moduleEvent := watch.Event{
+					Type:   event.Type,
+					Object: &m,
+				}

Also applies to: 457-457

pkg/registry/core/tenantmodule/rest.go (1)

155-156: Clarify log message: use Kind instead of Resource name

The log currently says "kind %s" but logs r.gvr.Resource, which is the resource name (tenantmodules), not the kind (TenantModule). Prefer logging r.gvk.Kind (or r.kindName) to avoid confusion when debugging.

-	klog.V(6).Infof("Successfully retrieved and converted resource %s of kind %s", name, r.gvr.Resource)
+	klog.V(6).Infof("Successfully retrieved and converted resource %s of kind %s", name, r.gvk.Kind)

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

go.mod (1)

122-124: Replace directive with future-dated pseudo-version.

The replace directive uses a pseudo-version with a future date (20251201...). This has already been flagged in a previous review. Ensure this temporary fork is tracked for removal once the upstream fix from kubernetes/kubernetes#135537 is released in a stable k8s.io/apimachinery version.

Based on learnings, go.mod and go.sum should generally be modified using go get commands rather than manual edits. However, replace directives are a valid exception since they cannot be added via go get.

pkg/registry/core/tenantmodule/rest.go (1)

638-644: Fix pointer-to-loop-variable bug in table rows.

The range variable module is reused across iterations in Go, so &module in runtime.RawExtension{Object: &module} causes every row's Object to point to the same memory location (the last item). This was flagged in a previous review.

Apply this diff to fix the issue:

 	for _, module := range modules {
+		m := module
 		row := metav1.TableRow{
 			Cells:  []interface{}{module.GetName(), getReadyStatus(module.Status.Conditions), computeAge(module.GetCreationTimestamp().Time, now), getVersion(module.Status.Version)},
-			Object: runtime.RawExtension{Object: &module},
+			Object: runtime.RawExtension{Object: &m},
 		}
 		table.Rows = append(table.Rows, row)
 	}

Alternatively, use index-based iteration:

-	for _, module := range modules {
+	for i := range modules {
 		row := metav1.TableRow{
-			Cells:  []interface{}{module.GetName(), getReadyStatus(module.Status.Conditions), computeAge(module.GetCreationTimestamp().Time, now), getVersion(module.Status.Version)},
-			Object: runtime.RawExtension{Object: &module},
+			Cells:  []interface{}{modules[i].GetName(), getReadyStatus(modules[i].Status.Conditions), computeAge(modules[i].GetCreationTimestamp().Time, now), getVersion(modules[i].Status.Version)},
+			Object: runtime.RawExtension{Object: &modules[i]},
 		}
 		table.Rows = append(table.Rows, row)
 	}

pkg/registry/core/tenantmodule/rest.go (1)

649-669: Potential issue: Address of parameter in single-item table.

The function receives module by value and takes its address for Object. While this works for a single row, it's inconsistent with best practices. Consider passing by pointer or documenting the intent.

-func (r *REST) buildTableFromTenantModule(module corev1alpha1.TenantModule) metav1.Table {
+func (r *REST) buildTableFromTenantModule(module *corev1alpha1.TenantModule) metav1.Table {
 	table := metav1.Table{
 		ColumnDefinitions: []metav1.TableColumnDefinition{
 			{Name: "NAME", Type: "string", Description: "Name of the TenantModule", Priority: 0},
 			{Name: "READY", Type: "string", Description: "Ready status of the TenantModule", Priority: 0},
 			{Name: "AGE", Type: "string", Description: "Age of the TenantModule", Priority: 0},
 			{Name: "VERSION", Type: "string", Description: "Version of the TenantModule", Priority: 0},
 		},
 		Rows: []metav1.TableRow{},
 	}
 	now := time.Now()

 	row := metav1.TableRow{
 		Cells:  []interface{}{module.GetName(), getReadyStatus(module.Status.Conditions), computeAge(module.GetCreationTimestamp().Time, now), getVersion(module.Status.Version)},
-		Object: runtime.RawExtension{Object: &module},
+		Object: runtime.RawExtension{Object: module},
 	}
 	table.Rows = append(table.Rows, row)

 	return table
 }

Then update the caller at line 598:

-		table = r.buildTableFromTenantModule(*obj)
+		table = r.buildTableFromTenantModule(obj)

pkg/registry/apps/application/rest.go (1)

1041-1061: Consider passing by pointer for consistency.

Similar to tenantmodule/rest.go, the function receives app by value and takes its address. Consider passing by pointer for consistency and to avoid the copy.

-func (r *REST) buildTableFromApplication(app appsv1alpha1.Application) metav1.Table {
+func (r *REST) buildTableFromApplication(app *appsv1alpha1.Application) metav1.Table {
 	...
 	row := metav1.TableRow{
 		Cells:  []interface{}{app.GetName(), getReadyStatus(app.Status.Conditions), computeAge(app.GetCreationTimestamp().Time, now), getVersion(app.Status.Version)},
-		Object: runtime.RawExtension{Object: &app},
+		Object: runtime.RawExtension{Object: app},
 	}

Update the caller at line 990:

-		table = r.buildTableFromApplication(*obj)
+		table = r.buildTableFromApplication(obj)

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Learnt from: CR
Repo: cozystack/cozystack PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-27T11:21:45.808Z
Learning: Applies to {go.mod,go.sum} : Do not manually modify `go.mod` and `go.sum` files; use `go get` command instead

hack/e2e-test-openapi.bats (1)

46-53: Consider bounding namespace deletion time

The create/delete namespace test is useful to catch stuck finalizers. To avoid a potentially hanging e2e run in pathological cases, consider adding an explicit timeout, e.g. kubectl delete ns ... --wait=true --timeout=5m, so failures surface as test errors rather than indefinite hangs.

pkg/registry/core/tenantmodule/rest.go (1)

149-151: Minor logging wording nit in Get

The log message says “of kind %s” but logs r.gvr.Resource (e.g. tenantmodules). Consider changing the format string or argument to avoid “kind” vs “resource” confusion; the returned object itself looks correct.

pkg/registry/apps/application/rest.go (3)

178-182: Create log message text is slightly misleading

In Create, the log line says “Successfully retrieved and converted resource …” though this path is performing a create-then-convert. Consider rephrasing to “created and converted” (or similar) to avoid confusion when debugging.

---

218-227: Get returns typed Application with consistent TypeMeta

Returning &convertedApp after ConvertHelmReleaseToApplication keeps the REST Get path fully typed. As in the TenantModule REST, the log message mentions “kind” but logs r.gvr.Resource; consider aligning wording/argument, but behavior is otherwise correct.

---

472-477: Verbose logging of full updated object may be heavy

klog.V(6).Infof("Returning updated Application object: %+v", convertedApp) can produce very large logs if specs are big. Given V(6) is already quite verbose this is acceptable, but you might consider logging only metadata (name/namespace/resourceVersion) to keep debug output leaner.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro