[api] Fix representation of dynamic list kinds - #1630

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

1061-1088: Handle unstructured lists correctly in ConvertToTable.

The updated logic extracts items from an *unstructured.Unstructured that contains an "items" field, which is appropriate for the new list representation. The fall-through to single-object handling when items are not present maintains backward compatibility.

One minor note: the for { ... break } pattern on lines 1062-1083 is an unconventional way to handle conditional logic. Consider refactoring to explicit if statements for clarity:

-		var apps []appsv1alpha1.Application
-		for {
-			var items interface{}
-			var ok bool
-			var objects []unstructured.Unstructured
-			if items, ok = obj.Object["items"]; !ok {
-				break
-			}
-			if objects, ok = items.([]unstructured.Unstructured); !ok {
-				break
-			}
-			apps = make([]appsv1alpha1.Application, 0, len(objects))
-			var a appsv1alpha1.Application
-			for i := range objects {
-				err := runtime.DefaultUnstructuredConverter.FromUnstructured(objects[i].Object, &a)
-				if err != nil {
-					klog.Errorf("Failed to convert Unstructured to Application: %v", err)
-					continue
-				}
-				apps = append(apps, a)
-			}
-			break
-		}
-		if apps != nil {
+		if items, ok := obj.Object["items"]; ok {
+			if objects, ok := items.([]unstructured.Unstructured); ok {
+				apps := make([]appsv1alpha1.Application, 0, len(objects))
+				for i := range objects {
+					var a appsv1alpha1.Application
+					err := runtime.DefaultUnstructuredConverter.FromUnstructured(objects[i].Object, &a)
+					if err != nil {
+						klog.Errorf("Failed to convert Unstructured to Application: %v", err)
+						continue
+					}
+					apps = append(apps, a)
+				}
+				table = r.buildTableFromApplications(apps)
+				table.ListMeta.ResourceVersion = obj.GetResourceVersion()
+			}
+		} else {
-			table = r.buildTableFromApplications(apps)
-			table.ListMeta.ResourceVersion = obj.GetResourceVersion()
-			break
-		}
-		var app appsv1alpha1.Application
-		err := runtime.DefaultUnstructuredConverter.FromUnstructured(obj.UnstructuredContent(), &app)
-		if err != nil {
-			klog.Errorf("Failed to convert Unstructured to Application: %v", err)
-			return nil, fmt.Errorf("failed to convert Unstructured to Application: %v", err)
-		}
-		table = r.buildTableFromApplication(app)
-		table.ListMeta.ResourceVersion = obj.GetResourceVersion()
+			var app appsv1alpha1.Application
+			err := runtime.DefaultUnstructuredConverter.FromUnstructured(obj.UnstructuredContent(), &app)
+			if err != nil {
+				klog.Errorf("Failed to convert Unstructured to Application: %v", err)
+				return nil, fmt.Errorf("failed to convert Unstructured to Application: %v", err)
+			}
+			table = r.buildTableFromApplication(app)
+			table.ListMeta.ResourceVersion = obj.GetResourceVersion()
+		}

packages/system/cozystack-api/Makefile (1)

7-14: Consider certificate cleanup and reuse.

The run-local target generates new certificates each time it's invoked. Consider these improvements:

1. Certificate cleanup: Add a cleanup mechanism or use the ignored apiserver.local.config/ directory instead of /tmp for better organization.
2. Certificate reuse: Check if certificates already exist before regenerating them to speed up subsequent runs.

Example refactor:

 run-local:
-	openssl req -nodes -new -x509 -keyout /tmp/ca.key -out /tmp/ca.crt -subj "/CN=kube-ca"
-	openssl req -out /tmp/client.csr -new -newkey rsa:2048 -nodes -keyout /tmp/client.key -subj "/C=US/ST=SomeState/L=L/OU=Dev/CN=development/O=system:masters"
-	openssl x509 -req -days 365 -in /tmp/client.csr -CA /tmp/ca.crt -CAkey /tmp/ca.key -set_serial 01 -sha256 -out /tmp/client.crt
-	openssl req -out /tmp/apiserver.csr -new -newkey rsa:2048 -nodes -keyout /tmp/apiserver.key -subj "/CN=cozystack-api" -config cozystack-api-openssl.cnf
-	openssl x509 -req -days 365 -in /tmp/apiserver.csr -CA /tmp/ca.crt -CAkey /tmp/ca.key -set_serial 01 -sha256 -out /tmp/apiserver.crt -extensions v3_req -extfile cozystack-api-openssl.cnf
+	mkdir -p apiserver.local.config
+	test -f apiserver.local.config/ca.crt || openssl req -nodes -new -x509 -keyout apiserver.local.config/ca.key -out apiserver.local.config/ca.crt -subj "/CN=kube-ca"
+	test -f apiserver.local.config/client.crt || openssl req -out apiserver.local.config/client.csr -new -newkey rsa:2048 -nodes -keyout apiserver.local.config/client.key -subj "/C=US/ST=SomeState/L=L/OU=Dev/CN=development/O=system:masters"
+	test -f apiserver.local.config/client.crt || openssl x509 -req -days 365 -in apiserver.local.config/client.csr -CA apiserver.local.config/ca.crt -CAkey apiserver.local.config/ca.key -set_serial 01 -sha256 -out apiserver.local.config/client.crt
+	test -f apiserver.local.config/apiserver.crt || openssl req -out apiserver.local.config/apiserver.csr -new -newkey rsa:2048 -nodes -keyout apiserver.local.config/apiserver.key -subj "/CN=cozystack-api" -config cozystack-api-openssl.cnf
+	test -f apiserver.local.config/apiserver.crt || openssl x509 -req -days 365 -in apiserver.local.config/apiserver.csr -CA apiserver.local.config/ca.crt -CAkey apiserver.local.config/ca.key -set_serial 01 -sha256 -out apiserver.local.config/apiserver.crt -extensions v3_req -extfile cozystack-api-openssl.cnf
 	CGO_ENABLED=0 go build -o /tmp/cozystack-api ../../../cmd/cozystack-api/main.go
-	/tmp/cozystack-api --client-ca-file /tmp/ca.crt --tls-cert-file /tmp/apiserver.crt --tls-private-key-file /tmp/apiserver.key --secure-port 6443 --kubeconfig $(KUBECONFIG) --authorization-kubeconfig $(KUBECONFIG) --authentication-kubeconfig $(KUBECONFIG)
+	/tmp/cozystack-api --client-ca-file apiserver.local.config/ca.crt --tls-cert-file apiserver.local.config/apiserver.crt --tls-private-key-file apiserver.local.config/apiserver.key --secure-port 6443 --kubeconfig $(KUBECONFIG) --authorization-kubeconfig $(KUBECONFIG) --authentication-kubeconfig $(KUBECONFIG)

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

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

23-44: Store kubectl output in variables to avoid redundant API calls.

The kubectl commands for tenants and ingresses are executed multiple times. Store each output in a variable and reuse it for subsequent checks to improve efficiency.

Apply this diff:

 @test "Test kinds" {
-  val=$(kubectl get --raw /apis/apps.cozystack.io/v1alpha1/tenants | jq -r '.kind')
+  tenants_raw=$(kubectl get --raw /apis/apps.cozystack.io/v1alpha1/tenants)
+  val=$(echo "$tenants_raw" | jq -r '.kind')
   if [ "$val" != "TenantList" ]; then
     echo "Expected kind to be TenantList, got $val"
     exit 1
   fi
-  val=$(kubectl get --raw /apis/apps.cozystack.io/v1alpha1/tenants | jq -r '.items[0].kind')
+  val=$(echo "$tenants_raw" | jq -r '.items[0].kind')
   if [ "$val" != "Tenant" ]; then
     echo "Expected kind to be Tenant, got $val"
     exit 1
   fi
-  val=$(kubectl get --raw /apis/apps.cozystack.io/v1alpha1/ingresses | jq -r '.kind')
+  ingresses_raw=$(kubectl get --raw /apis/apps.cozystack.io/v1alpha1/ingresses)
+  val=$(echo "$ingresses_raw" | jq -r '.kind')
   if [ "$val" != "IngressList" ]; then
     echo "Expected kind to be IngressList, got $val"
     exit 1
   fi
-  val=$(kubectl get --raw /apis/apps.cozystack.io/v1alpha1/ingresses | jq -r '.items[0].kind')
+  val=$(echo "$ingresses_raw" | jq -r '.items[0].kind')
   if [ "$val" != "Ingress" ]; then
     echo "Expected kind to be Ingress, got $val"
     exit 1
   fi
 }

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

1068-1095: Simplify control flow with nested if statements.

The for {} loop with break statements (lines 1069-1090) is unconventional and reduces readability. Using nested if statements would make the intent clearer: attempt to parse as a list, and fall through to single-object parsing if that fails.

Apply this diff:

 	case *unstructured.Unstructured:
-		var apps []appsv1alpha1.Application
-		for {
-			var items interface{}
-			var ok bool
-			var objects []unstructured.Unstructured
-			if items, ok = obj.Object["items"]; !ok {
-				break
-			}
-			if objects, ok = items.([]unstructured.Unstructured); !ok {
-				break
-			}
+		if items, ok := obj.Object["items"]; ok {
+			if objects, ok := items.([]unstructured.Unstructured); ok {
-			apps = make([]appsv1alpha1.Application, 0, len(objects))
-			var a appsv1alpha1.Application
-			for i := range objects {
-				err := runtime.DefaultUnstructuredConverter.FromUnstructured(objects[i].Object, &a)
-				if err != nil {
-					klog.Errorf("Failed to convert Unstructured to Application: %v", err)
-					continue
+				apps := make([]appsv1alpha1.Application, 0, len(objects))
+				var a appsv1alpha1.Application
+				for i := range objects {
+					err := runtime.DefaultUnstructuredConverter.FromUnstructured(objects[i].Object, &a)
+					if err != nil {
+						klog.Errorf("Failed to convert Unstructured to Application: %v", err)
+						continue
+					}
+					apps = append(apps, a)
 				}
-				apps = append(apps, a)
+				table = r.buildTableFromApplications(apps)
+				table.ListMeta.ResourceVersion = obj.GetResourceVersion()
+				break
 			}
-			break
 		}
-		if apps != nil {
-			table = r.buildTableFromApplications(apps)
-			table.ListMeta.ResourceVersion = obj.GetResourceVersion()
-			break
-		}
+		// Fall through to single-object handling
 		var app appsv1alpha1.Application
 		err := runtime.DefaultUnstructuredConverter.FromUnstructured(obj.UnstructuredContent(), &app)

---

1238-1243: Fix type inconsistency in items initialization.

Line 1241 initializes items as make([]interface{}, 0), but List() (line 394) populates it with []unstructured.Unstructured, and ConvertToTable() (lines 1076-1078) type-asserts it as []unstructured.Unstructured. If NewList() is called without going through List(), the type assertion in ConvertToTable() will fail, causing a runtime panic.

Apply this diff:

 func (r *REST) NewList() runtime.Object {
 	obj := &unstructured.Unstructured{}
 	obj.SetGroupVersionKind(r.gvk.GroupVersion().WithKind(r.kindName + "List"))
-	obj.Object["items"] = make([]interface{}, 0)
+	obj.Object["items"] = make([]unstructured.Unstructured, 0)
 	return obj
 }

pkg/apiserver/apiserver.go (1)

68-81: Add corev1alpha1 types to mgrScheme for REST storage client compatibility.

The REST storage implementations for tenantsecrets, tenantnamespaces, and tenantmodules (lines 181-189) use clients built with mgrScheme (lines 145, 175). However, corev1alpha1 types are only registered to the global Scheme (line 69), not to mgrScheme. This will cause serialization/deserialization failures when these REST endpoints handle tenant resources.

Add corev1alpha1 registration to mgrScheme:

 	if err := helmv2.AddToScheme(mgrScheme); err != nil {
 		panic(fmt.Errorf("Failed to add HelmRelease types to scheme: %w", err))
 	}
 
+	if err := corev1alpha1.AddToScheme(mgrScheme); err != nil {
+		panic(fmt.Errorf("Failed to add corev1alpha1 types to scheme: %w", err))
+	}
+
 	if err := corev1.AddToScheme(mgrScheme); err != nil {
 		panic(fmt.Errorf("Failed to add core types to scheme: %w", err))
 	}

Note: You'll need to import the corev1alpha1 package:

import (
	// ... existing imports ...
	corev1alpha1 "github.com/cozystack/cozystack/pkg/apis/core/v1alpha1"
)

packages/system/cozystack-api/Makefile (1)

16-17: Align debug target build flags with run-local.

The debug target invokes dlv debug on the source directly without CGO_ENABLED=0, whereas run-local explicitly disables CGO. For consistency and to avoid potential build issues, dlv debug should use the same CGO setting.

Apply this diff to align the build flags:

 debug:
-	dlv debug ../../../cmd/cozystack-api/main.go -- --client-ca-file /tmp/ca.crt --tls-cert-file /tmp/apiserver.crt --tls-private-key-file /tmp/apiserver.key --secure-port 6443 --kubeconfig $(KUBECONFIG) --authorization-kubeconfig $(KUBECONFIG) --authentication-kubeconfig $(KUBECONFIG)
+	CGO_ENABLED=0 dlv debug ../../../cmd/cozystack-api/main.go -- --client-ca-file /tmp/ca.crt --tls-cert-file /tmp/apiserver.crt --tls-private-key-file /tmp/apiserver.key --secure-port 6443 --kubeconfig $(KUBECONFIG) --authorization-kubeconfig $(KUBECONFIG) --authentication-kubeconfig $(KUBECONFIG)

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Learnt from: lllamnyp
Repo: cozystack/cozystack PR: 870
File: packages/system/keycloak/templates/ingress.yaml:8-15
Timestamp: 2025-05-05T05:50:23.971Z
Learning: In Go templates for Helm charts, variables that will hold objects from resource lookups should be initialized with `dict` rather than `list` or `nil`. This is because an empty map (`dict`) provides safer behavior when checking for nested properties in conditional statements like `{{- if and $var $var.spec $var.spec.values }}`.

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

23-44: Cache kubectl results and validate items array.

The past review correctly identified that kubectl get --raw is executed multiple times for the same resource, which is inefficient. Additionally, accessing .items[0] without verifying the array is non-empty could cause test failures if no resources exist.

Consider this refactored approach:

 @test "Test kinds" {
+  # Cache API responses
+  tenants_raw=$(kubectl get --raw /apis/apps.cozystack.io/v1alpha1/tenants)
+  ingresses_raw=$(kubectl get --raw /apis/apps.cozystack.io/v1alpha1/ingresses)
+  
+  # Validate tenants
-  val=$(kubectl get --raw /apis/apps.cozystack.io/v1alpha1/tenants | jq -r '.kind')
+  val=$(echo "$tenants_raw" | jq -r '.kind')
   if [ "$val" != "TenantList" ]; then
     echo "Expected kind to be TenantList, got $val"
     exit 1
   fi
-  val=$(kubectl get --raw /apis/apps.cozystack.io/v1alpha1/tenants | jq -r '.items[0].kind')
+  val=$(echo "$tenants_raw" | jq -r '.items[0].kind // empty')
   if [ "$val" != "Tenant" ]; then
-    echo "Expected kind to be Tenant, got $val"
+    echo "Expected kind to be Tenant, got $val (or no items found)"
     exit 1
   fi
+  
+  # Validate ingresses
-  val=$(kubectl get --raw /apis/apps.cozystack.io/v1alpha1/ingresses | jq -r '.kind')
+  val=$(echo "$ingresses_raw" | jq -r '.kind')
   if [ "$val" != "IngressList" ]; then
     echo "Expected kind to be IngressList, got $val"
     exit 1
   fi
-  val=$(kubectl get --raw /apis/apps.cozystack.io/v1alpha1/ingresses | jq -r '.items[0].kind')
+  val=$(echo "$ingresses_raw" | jq -r '.items[0].kind // empty')
   if [ "$val" != "Ingress" ]; then
-    echo "Expected kind to be Ingress, got $val"
+    echo "Expected kind to be Ingress, got $val (or no items found)"
     exit 1
   fi
 }

The // empty operator in jq returns an empty string if .items[0] doesn't exist, preventing jq errors.

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

1247-1250: Address the type inconsistency in items initialization.

The items field is initialized as []interface{}{}, but the List method (line 402) populates it with []unstructured.Unstructured, and ConvertToTable (line 1084) performs a type assertion expecting []unstructured.Unstructured. If an unpopulated list created by NewList() reaches ConvertToTable, the type assertion will fail, causing the code to fall through to the single-object case and likely fail.

Apply this fix as suggested in past reviews:

-	obj.Object["items"] = make([]interface{}, 0)
+	obj.Object["items"] = make([]unstructured.Unstructured, 0)

Based on learnings

packages/system/cozystack-api/Makefile (1)

16-17: Document dlv debugger requirement and consider build optimizations for debugging.

The debug target invokes dlv debug but does not verify that Delve is installed. Additionally, for optimal debugging, consider adding explicit debug symbols (-gcflags="all=-N -l").

1. Document in a README or comment that dlv (Delve debugger) must be installed: go install github.com/go-delve/delve/cmd/dlv@latest.
2. Consider adding debug symbols to the debug target for better debugging experience:

 debug:
-	dlv debug ../../../cmd/cozystack-api/main.go -- --client-ca-file /tmp/ca.crt --tls-cert-file /tmp/apiserver.crt --tls-private-key-file /tmp/apiserver.key --secure-port 6443 --kubeconfig $(KUBECONFIG) --authorization-kubeconfig $(KUBECONFIG) --authentication-kubeconfig $(KUBECONFIG)
+	dlv debug -gcflags="all=-N -l" ../../../cmd/cozystack-api/main.go -- --client-ca-file /tmp/ca.crt --tls-cert-file /tmp/apiserver.crt --tls-private-key-file /tmp/apiserver.key --secure-port 6443 --kubeconfig $(KUBECONFIG) --authorization-kubeconfig $(KUBECONFIG) --authentication-kubeconfig $(KUBECONFIG)

However, verify that dlv actually accepts -gcflags in this context; alternatively, the flags may need to be passed differently.

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

145-156: Consider clarifying the error message.

The error message on line 153 states "returned unstructured.Unstructured object was not an Application," but the actual issue is that the conversion from unstructured to Application failed. The object was received as input (via the obj parameter), not returned.

Consider this revision:

 	if err := runtime.DefaultUnstructuredConverter.FromUnstructured(us.Object, app); err != nil {
-		errMsg := fmt.Sprintf("returned unstructured.Unstructured object was not an Application")
+		errMsg := fmt.Sprintf("failed to convert unstructured object to Application: %v", err)
 		klog.Errorf(errMsg)
-		return nil, fmt.Errorf(errMsg)
+		return nil, fmt.Errorf("%s", errMsg)
 	}

---

450-462: Consider clarifying the error message.

Similar to the Create method, the error message on line 459 could be more precise about the actual failure (conversion error) rather than stating "returned unstructured.Unstructured object was not an Application."

Apply this diff:

 	if err := runtime.DefaultUnstructuredConverter.FromUnstructured(us.Object, app); err != nil {
-		errMsg := fmt.Sprintf("returned unstructured.Unstructured object was not an Application")
+		errMsg := fmt.Sprintf("failed to convert unstructured object to Application: %v", err)
 		klog.Errorf(errMsg)
 		return nil, false, fmt.Errorf(errMsg)
 	}

---

1076-1103: Consider refactoring the control flow for clarity.

The for {} loop with breaks is being used for early-exit control flow, making the code harder to follow. Nested if statements would express the intent more clearly.

Apply the refactor suggested in past reviews:

-	var apps []appsv1alpha1.Application
-	for {
-		var items interface{}
-		var ok bool
-		var objects []unstructured.Unstructured
-		if items, ok = obj.Object["items"]; !ok {
-			break
-		}
-		if objects, ok = items.([]unstructured.Unstructured); !ok {
-			break
-		}
-		apps = make([]appsv1alpha1.Application, 0, len(objects))
-		var a appsv1alpha1.Application
-		for i := range objects {
-			err := runtime.DefaultUnstructuredConverter.FromUnstructured(objects[i].Object, &a)
-			if err != nil {
-				klog.Errorf("Failed to convert Unstructured to Application: %v", err)
-				continue
-			}
-			apps = append(apps, a)
-		}
-		break
-	}
-	if apps != nil {
+	if items, ok := obj.Object["items"]; ok {
+		if objects, ok := items.([]unstructured.Unstructured); ok {
+			apps := make([]appsv1alpha1.Application, 0, len(objects))
+			var a appsv1alpha1.Application
+			for i := range objects {
+				err := runtime.DefaultUnstructuredConverter.FromUnstructured(objects[i].Object, &a)
+				if err != nil {
+					klog.Errorf("Failed to convert Unstructured to Application: %v", err)
+					continue
+				}
+				apps = append(apps, a)
+			}
+			table = r.buildTableFromApplications(apps)
+			table.ListMeta.ResourceVersion = obj.GetResourceVersion()
+			break
+		}
+	}
+ 	// Fall through to single object case
-		table = r.buildTableFromApplications(apps)
-		table.ListMeta.ResourceVersion = obj.GetResourceVersion()
-		break
-	}
 	var app appsv1alpha1.Application

Based on learnings

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Learnt from: lllamnyp
Repo: cozystack/cozystack PR: 870
File: packages/system/keycloak/templates/ingress.yaml:8-15
Timestamp: 2025-05-05T05:50:23.971Z
Learning: In Go templates for Helm charts, variables that will hold objects from resource lookups should be initialized with `dict` rather than `list` or `nil`. This is because an empty map (`dict`) provides safer behavior when checking for nested properties in conditional statements like `{{- if and $var $var.spec $var.spec.values }}`.