internal/fluxinstall/manifests.embed.go (1)

31-50: Consider more restrictive file permissions.

The function uses 0666 permissions when writing manifests to disk. While these files are written to a temporary directory (reducing security impact), consider using 0644 (read-write for owner, read-only for group/others) as a more defensive default.

🔎 Proposed permission adjustment

-		if err := os.WriteFile(outputPath, data, 0666); err != nil {
+		if err := os.WriteFile(outputPath, data, 0644); err != nil {

packages/core/flux-aio/Makefile (1)

17-34: Dual update workflow supports migration.

The coexistence of update-old and update-new paths provides a clean migration strategy. The update-new path correctly outputs to manifests/fluxcd.yaml and removes the hostNetwork/dnsPolicy patches per the TODO comment on lines 33-34.

The TODO on line 33 mentions solving DNS issues with hostNetwork for tenant clusters. Would you like me to help track this or investigate alternative DNS configuration approaches?

internal/fluxinstall/install.go (1)

161-197: Consider replacing hard-coded sleep with readiness polling.

The two-stage apply strategy (CRDs/Namespaces first, then other resources) is correct. However, the hard-coded 2-second sleep on line 185 is brittle and may be insufficient under load or excessive when CRDs register quickly.

Consider replacing with a readiness check that polls for CRD availability using the API server's discovery client, with a timeout. This would make the installation more reliable across different cluster conditions.

Conceptual approach

// After applying CRDs, poll for their availability
if len(stageOne) > 0 {
    logger.Info("Applying cluster definitions", "count", len(stageOne))
    if err := applyObjects(ctx, k8sClient, decoder, stageOne); err != nil {
        return fmt.Errorf("failed to apply cluster definitions: %w", err)
    }
    
    // Poll for CRD readiness with timeout
    if err := waitForCRDsReady(ctx, k8sClient, stageOne, 30*time.Second); err != nil {
        logger.Info("CRDs may not be fully ready, continuing anyway", "error", err)
    }
}

The waitForCRDsReady helper would check the Established condition on CRD status.

cmd/cozystack-operator/main.go (2)

105-107: Development mode enabled by default.

Line 106 sets Development: true for the logger, which may not be appropriate for production deployments. Consider making this configurable via a CLI flag or environment variable.

🔎 Suggested flag-based configuration

+	var devMode bool
+	flag.BoolVar(&devMode, "dev-mode", false, "Enable development mode (more verbose logging)")
+
 	opts := zap.Options{
-		Development: true,
+		Development: devMode,
 	}

---

209-270: Consider extracting common create-or-update logic.

The installFluxResourcesFunc implements create-or-update logic that is duplicated in installPlatformSourceResource (lines 307-332). Both follow the same pattern:

1. Get existing resource
2. If not found, create
3. If found, update with ResourceVersion

Consider extracting this pattern into a reusable helper function to reduce duplication and improve maintainability:

func createOrUpdateResource(ctx context.Context, k8sClient client.Client, obj *unstructured.Unstructured) error {
    existing := &unstructured.Unstructured{}
    existing.SetGroupVersionKind(obj.GroupVersionKind())
    key := client.ObjectKey{Name: obj.GetName(), Namespace: obj.GetNamespace()}
    
    err := k8sClient.Get(ctx, key, existing)
    if err != nil {
        if client.IgnoreNotFound(err) == nil {
            return k8sClient.Create(ctx, obj)
        }
        return fmt.Errorf("failed to check if resource exists: %w", err)
    }
    
    obj.SetResourceVersion(existing.GetResourceVersion())
    return k8sClient.Update(ctx, obj)
}

This would simplify both functions and ensure consistent behavior.

Configuration used: defaults

Review profile: CHILL

Plan: Pro

cmd/cozystack-operator/main.go (4)

240-266: Consider using server-side apply for consistency and robustness.

The current Get-Create/Update pattern works but server-side apply (client.Patch with client.Apply) would be more robust and consistent with the approach used in fluxinstall.Install.

🔎 Proposed refactor using server-side apply

-		// Use server-side apply or create/update
-		existing := &unstructured.Unstructured{}
-		existing.SetGroupVersionKind(obj.GroupVersionKind())
-		key := client.ObjectKey{
-			Name:      obj.GetName(),
-			Namespace: obj.GetNamespace(),
-		}
-
-		err := k8sClient.Get(ctx, key, existing)
-		if err != nil {
-			if client.IgnoreNotFound(err) == nil {
-				// Resource doesn't exist, create it
-				if err := k8sClient.Create(ctx, &obj); err != nil {
-					return fmt.Errorf("failed to create resource %s/%s: %w", obj.GetKind(), obj.GetName(), err)
-				}
-				logger.Info("Created Flux resource", "kind", obj.GetKind(), "name", obj.GetName())
-			} else {
-				return fmt.Errorf("failed to check if resource exists: %w", err)
-			}
-		} else {
-			// Resource exists, update it
-			obj.SetResourceVersion(existing.GetResourceVersion())
-			if err := k8sClient.Update(ctx, &obj); err != nil {
-				return fmt.Errorf("failed to update resource %s/%s: %w", obj.GetKind(), obj.GetName(), err)
-			}
-			logger.Info("Updated Flux resource", "kind", obj.GetKind(), "name", obj.GetName())
-		}
+		// Use server-side apply
+		patchOpts := []client.PatchOption{
+			client.ForceOwnership,
+			client.FieldOwner("cozystack-operator"),
+		}
+		if err := k8sClient.Patch(ctx, &obj, client.Apply, patchOpts...); err != nil {
+			return fmt.Errorf("failed to apply resource %s/%s: %w", obj.GetKind(), obj.GetName(), err)
+		}
+		logger.Info("Applied Flux resource", "kind", obj.GetKind(), "name", obj.GetName())

---

307-332: Consider using server-side apply for consistency and robustness.

Similar to installFluxResourcesFunc, using server-side apply here would simplify the logic and make it more robust and consistent.

🔎 Proposed refactor using server-side apply

-	existing := &unstructured.Unstructured{}
-	existing.SetGroupVersionKind(obj.GroupVersionKind())
-	key := client.ObjectKey{
-		Name:      obj.GetName(),
-		Namespace: obj.GetNamespace(),
-	}
-
-	err = k8sClient.Get(ctx, key, existing)
-	if err != nil {
-		if client.IgnoreNotFound(err) == nil {
-			// Resource doesn't exist, create it
-			if err := k8sClient.Create(ctx, obj); err != nil {
-				return fmt.Errorf("failed to create resource %s/%s: %w", obj.GetKind(), obj.GetName(), err)
-			}
-			logger.Info("Created platform source resource", "kind", obj.GetKind(), "name", obj.GetName())
-		} else {
-			return fmt.Errorf("failed to check if resource exists: %w", err)
-		}
-	} else {
-		// Resource exists, update it
-		obj.SetResourceVersion(existing.GetResourceVersion())
-		if err := k8sClient.Update(ctx, obj); err != nil {
-			return fmt.Errorf("failed to update resource %s/%s: %w", obj.GetKind(), obj.GetName(), err)
-		}
-		logger.Info("Updated platform source resource", "kind", obj.GetKind(), "name", obj.GetName())
-	}
+	patchOpts := []client.PatchOption{
+		client.ForceOwnership,
+		client.FieldOwner("cozystack-operator"),
+	}
+	if err := k8sClient.Patch(ctx, obj, client.Apply, patchOpts...); err != nil {
+		return fmt.Errorf("failed to apply resource %s/%s: %w", obj.GetKind(), obj.GetName(), err)
+	}
+	logger.Info("Applied platform source resource", "kind", obj.GetKind(), "name", obj.GetName())

---

176-187: Inconsistent error handling: platform source failure is fatal while Flux failures are not.

The platform source installation calls os.Exit(1) on failure (line 183), causing the operator pod to crash-loop, while Flux and Flux resource installations are non-fatal and allow the operator to start. This inconsistency could cause unexpected behavior.

Consider either:

1. Making platform source installation non-fatal to match the other steps (remove os.Exit(1)), allowing manual recovery or GitOps-based reconciliation, or
2. If platform source failure truly must be fatal, document why with a clear comment and return the error to main for controlled shutdown instead of calling os.Exit(1) directly.

---

344-405: Add unit tests and clarify parsing logic for edge cases.

The parsing has two issues:

1. Line 354 comment states "find the last @" but line 355 uses strings.Index() (first occurrence, not last)—though this works correctly for the @sha256: case, the comment is misleading.
2. Line 361 fallback uses strings.LastIndex("@") which could misinterpret @ characters within repository paths (e.g., oci://registry.io/org@name) as ref separators.

Add unit tests covering:

• OCI: oci://registry.io/org@name (no ref), oci://registry.io/org@name@sha256:abc (with digest)
• OCI: registry paths containing special characters
• Git: refs with various characters, multiple @ symbols
• Both: empty/invalid input handling

💡 Consider clarifying the logic

Update the comment to accurately describe the search method, and consider more robust parsing that only treats @ as a ref separator when it introduces a digest pattern (e.g., @sha256:...) or explicit ref syntax.

internal/fluxinstall/install.go (2)

184-186: Replace fixed sleep with CRD status polling for reliability.

Using time.Sleep to wait for CRDs is flaky—it can cause race conditions if 2 seconds isn't enough or unnecessary delays if CRDs establish faster. Poll the Established condition instead using k8s.io/apimachinery/pkg/util/wait.PollUntilContextTimeout.

🔎 Proposed fix using polling

// For each CRD in stageOne, poll for Established condition
for _, obj := range stageOne {
	if obj.GetKind() != "CustomResourceDefinition" {
		continue
	}
	
	err := wait.PollUntilContextTimeout(ctx, 500*time.Millisecond, 1*time.Minute, true, func(ctx context.Context) (bool, error) {
		crd := &apiextensionsv1.CustomResourceDefinition{}
		if err := k8sClient.Get(ctx, client.ObjectKey{Name: obj.GetName()}, crd); err != nil {
			logger.Info("Waiting for CRD to be available", "name", obj.GetName(), "error", err)
			return false, nil
		}
		for _, cond := range crd.Status.Conditions {
			if cond.Type == apiextensionsv1.Established && cond.Status == apiextensionsv1.ConditionTrue {
				return true, nil
			}
		}
		return false, nil
	})
	if err != nil {
		return fmt.Errorf("failed to wait for CRD %s to be established: %w", obj.GetName(), err)
	}
}

---

256-282: Consider logging errors instead of silently continuing.

Errors from unstructured.NestedMap and unstructured.NestedSlice are currently ignored with continue statements (lines 258, 266, 275, 281). While environment injection is optional, silently skipping errors could hide real issues.

Consider logging errors at debug or info level before continuing:

if !found || err != nil {
	if err != nil {
		logger.V(1).Info("Failed to access spec path, skipping", "kind", kind, "name", obj.GetName(), "error", err)
	}
	continue
}

This pattern should be applied to similar error handling in updateContainersEnv (lines 297, 316).

packages/core/flux-aio/Makefile (1)

15-16: Declare update targets as PHONY.

The update, update-old, and update-new targets should be declared as .PHONY since they don't produce file outputs.

Based on static analysis hints (checkmake).

internal/fluxinstall/manifests.embed.go (1)

44-44: Use more restrictive file permissions (0644 instead of 0666).

The file permission 0666 is overly permissive as it grants write access to all users. Use 0644 instead, which is the standard permission for files (read/write for owner, read-only for group and others).

🔎 Proposed fix

-		if err := os.WriteFile(outputPath, data, 0666); err != nil {
+		if err := os.WriteFile(outputPath, data, 0644); err != nil {

packages/core/flux-aio/Makefile (1)

33-34: Track the TODO for DNS resolution with hostNetwork.

The TODO comment indicates a known issue with DNS resolution when using hostNetwork: true for installing HelmReleases in tenant Kubernetes clusters. This affects the new manifests path while the old path (line 25) still applies the workaround.

Do you want me to open an issue to track this technical debt and ensure the DNS solution is implemented before the old manifest path is removed?

Configuration used: defaults

Review profile: CHILL

Plan: Pro

internal/fluxinstall/install.go (2)

184-186: Replace time.Sleep with robust CRD readiness polling.

Using time.Sleep to wait for CRDs is flaky—2 seconds might be insufficient under load or cause unnecessary delays when CRDs register quickly. Poll the Established condition instead.

🔎 Proposed fix using wait.PollUntilContextTimeout

+import (
+	apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
+	"k8s.io/apimachinery/pkg/util/wait"
+)

 	// Apply stage one (CRDs and Namespaces) first
 	if len(stageOne) > 0 {
 		logger.Info("Applying cluster definitions", "count", len(stageOne))
 		if err := applyObjects(ctx, k8sClient, stageOne); err != nil {
 			return fmt.Errorf("failed to apply cluster definitions: %w", err)
 		}
 
-		// Wait a bit for CRDs to be registered
-		time.Sleep(2 * time.Second)
+		// Wait for CRDs to be established
+		for _, obj := range stageOne {
+			if obj.GetKind() != "CustomResourceDefinition" {
+				continue
+			}
+			if err := wait.PollUntilContextTimeout(ctx, 500*time.Millisecond, 1*time.Minute, true, func(ctx context.Context) (bool, error) {
+				crd := &apiextensionsv1.CustomResourceDefinition{}
+				if err := k8sClient.Get(ctx, client.ObjectKey{Name: obj.GetName()}, crd); err != nil {
+					logger.V(1).Info("waiting for CRD to be available", "name", obj.GetName(), "error", err)
+					return false, nil
+				}
+				for _, cond := range crd.Status.Conditions {
+					if cond.Type == apiextensionsv1.Established && cond.Status == apiextensionsv1.ConditionTrue {
+						return true, nil
+					}
+				}
+				return false, nil
+			}); err != nil {
+				return fmt.Errorf("failed to wait for CRD %s to be established: %w", obj.GetName(), err)
+			}
+		}
 	}

Based on past review comments.

---

256-282: Propagate or log errors from unstructured operations.

Errors from unstructured.NestedMap, unstructured.NestedSlice, and unstructured.SetNestedSlice are silently ignored (lines 257, 265, 274, 281). This makes debugging difficult and may hide legitimate issues. At minimum, log these errors; ideally, collect and return them.

🔎 Proposed fix to log errors

+	logger := log.Log.WithName("fluxinstall")
+
 	for _, obj := range objects {
 		kind := obj.GetKind()
 		if kind != "Deployment" && kind != "StatefulSet" && kind != "DaemonSet" {
 			continue
 		}
 
 		// Navigate to spec.template.spec.containers
 		spec, found, err := unstructured.NestedMap(obj.Object, "spec", "template", "spec")
-		if !found || err != nil {
+		if err != nil {
+			logger.Error(err, "failed to get spec.template.spec", "kind", kind, "name", obj.GetName())
+			continue
+		}
+		if !found {
 			continue
 		}
 
 		// Update containers
 		containers, found, err := unstructured.NestedSlice(spec, "containers")
-		if found && err == nil {
+		if err != nil {
+			logger.Error(err, "failed to get containers", "kind", kind, "name", obj.GetName())
+			continue
+		}
+		if found {
 			containers = updateContainersEnv(containers, kubernetesHost, kubernetesPort)
 			if err := unstructured.SetNestedSlice(spec, containers, "containers"); err != nil {
+				logger.Error(err, "failed to set containers", "kind", kind, "name", obj.GetName())
 				continue
 			}
 		}
 
 		// Update initContainers
 		initContainers, found, err := unstructured.NestedSlice(spec, "initContainers")
-		if found && err == nil {
+		if err != nil {
+			logger.Error(err, "failed to get initContainers", "kind", kind, "name", obj.GetName())
+			continue
+		}
+		if found {
 			initContainers = updateContainersEnv(initContainers, kubernetesHost, kubernetesPort)
 			if err := unstructured.SetNestedSlice(spec, initContainers, "initContainers"); err != nil {
+				logger.Error(err, "failed to set initContainers", "kind", kind, "name", obj.GetName())
 				continue
 			}
 		}
 
 		// Update spec in the object
 		if err := unstructured.SetNestedMap(obj.Object, spec, "spec", "template", "spec"); err != nil {
+			logger.Error(err, "failed to set spec.template.spec", "kind", kind, "name", obj.GetName())
 			continue
 		}
 	}

Based on past review comments.

packages/core/flux-aio/Makefile (1)

15-15: Declare update targets as PHONY.

The update, update-old, and update-new targets should be declared as .PHONY since they don't produce file outputs.

🔎 Proposed fix

Add near the top of the Makefile (after line 4 or before line 6):

+.PHONY: show apply diff update update-old update-new
+
 show:

Based on static analysis hints (checkmake) and past review comments.

internal/fluxinstall/install.go (1)

163-163: Remove unused decoder variable.

The decoder is created but never used in applyManifests or applyObjects. Consider removing it to clean up the code.

🔎 Proposed fix

 func applyManifests(ctx context.Context, k8sClient client.Client, objects []*unstructured.Unstructured) error {
 	logger := log.FromContext(ctx)
-	decoder := yaml.NewDecodingSerializer(unstructured.UnstructuredJSONScheme)
 
 	// Separate CRDs and namespaces from other resources

And update the function signature of applyObjects:

-func applyObjects(ctx context.Context, k8sClient client.Client, decoder runtime.Decoder, objects []*unstructured.Unstructured) error {
+func applyObjects(ctx context.Context, k8sClient client.Client, objects []*unstructured.Unstructured) error {

And update the two call sites (lines 180, 191):

-		if err := applyObjects(ctx, k8sClient, decoder, stageOne); err != nil {
+		if err := applyObjects(ctx, k8sClient, stageOne); err != nil {

-		if err := applyObjects(ctx, k8sClient, decoder, stageTwo); err != nil {
+		if err := applyObjects(ctx, k8sClient, stageTwo); err != nil {

Configuration used: defaults

Review profile: CHILL

Plan: Pro

packages/core/flux-aio/Makefile (2)

15-15: Declare update targets as PHONY.

The update, update-old, and update-new targets don't produce file outputs and should be declared as .PHONY to avoid conflicts with files of the same name.

🔎 Proposed fix

Add near the top of the Makefile:

+.PHONY: update update-old update-new
+
 update: update-old update-new

Based on static analysis hints (checkmake).

---

27-34: DNS resolution will fail for Flux pods with hostNetwork: true.

The update-new target omits the dnsPolicy: ClusterFirstWithHostNet patch (line 34 is commented out), but the generated fluxcd.yaml contains pods with hostNetwork: true. Without the correct DNS policy, these pods will use the host's DNS configuration and fail to resolve cluster DNS names, breaking Flux functionality.

This must be fixed before the new installation path can be used. Either:

1. Uncomment line 34 to apply the patch during manifest generation, or
2. Implement the DNS policy correction in internal/fluxinstall/install.go before applying manifests

🔎 Option 1: Uncomment the dnsPolicy patch

 	sed -i ../../../internal/fluxinstall/manifests/fluxcd.yaml \
 		-e '/timoni/d' \
 		-e 's|\.cluster\.local\.,||g' -e 's|\.cluster\.local\,||g' -e 's|\.cluster\.local\.||g'
-	# TODO: solve dns issue with hostNetwork for installing helmreleases in tenant k8s clusters
-	#-e '/hostNetwork: true/i \      dnsPolicy: ClusterFirstWithHostNet' 
+		-e '/hostNetwork: true/i \      dnsPolicy: ClusterFirstWithHostNet'

🔎 Option 2: Add DNS policy correction in install.go

In internal/fluxinstall/install.go, add a function to patch DNS policy before applying manifests:

// patchDNSPolicy adds dnsPolicy: ClusterFirstWithHostNet to pods with hostNetwork: true
func patchDNSPolicy(objects []*unstructured.Unstructured) error {
	for _, obj := range objects {
		kind := obj.GetKind()
		if kind != "Deployment" && kind != "StatefulSet" && kind != "DaemonSet" {
			continue
		}
		
		spec, found, err := unstructured.NestedMap(obj.Object, "spec", "template", "spec")
		if !found || err != nil {
			continue
		}
		
		hostNetwork, found, _ := unstructured.NestedBool(spec, "hostNetwork")
		if found && hostNetwork {
			if err := unstructured.SetNestedField(spec, "ClusterFirstWithHostNet", "dnsPolicy"); err != nil {
				return fmt.Errorf("failed to set dnsPolicy for %s/%s: %w", kind, obj.GetName(), err)
			}
			if err := unstructured.SetNestedMap(obj.Object, spec, "spec", "template", "spec"); err != nil {
				return fmt.Errorf("failed to update spec for %s/%s: %w", kind, obj.GetName(), err)
			}
		}
	}
	return nil
}

Then call it in the Install function after parsing manifests:

 	if err := injectKubernetesServiceEnv(objects); err != nil {
 		logger.Info("Failed to inject KUBERNETES_SERVICE_* env vars, continuing anyway", "error", err)
 	}
+
+	// Patch DNS policy for pods with hostNetwork
+	if err := patchDNSPolicy(objects); err != nil {
+		return fmt.Errorf("failed to patch DNS policy: %w", err)
+	}

internal/fluxinstall/manifests.embed.go (1)

44-44: Use more restrictive file permissions.

The file permission 0666 grants write access to all users, which is overly permissive. Use 0644 instead (read/write for owner, read-only for group and others).

🔎 Proposed fix

-		if err := os.WriteFile(outputPath, data, 0666); err != nil {
+		if err := os.WriteFile(outputPath, data, 0644); err != nil {

internal/fluxinstall/install.go (1)

182-182: Replace fixed sleep with CRD status polling.

Using time.Sleep(2 * time.Second) to wait for CRDs can be unreliable—too short causes race conditions, too long wastes time. Poll the Established condition on each CRD instead.

🔎 Proposed fix using wait.PollUntilContextTimeout

+import (
+	apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
+	"k8s.io/apimachinery/pkg/util/wait"
+)

 	// Apply stage one (CRDs and Namespaces) first
 	if len(stageOne) > 0 {
 		logger.Info("Applying cluster definitions", "count", len(stageOne))
 		if err := applyObjects(ctx, k8sClient, stageOne); err != nil {
 			return fmt.Errorf("failed to apply cluster definitions: %w", err)
 		}
 
-		// Wait a bit for CRDs to be registered
-		time.Sleep(2 * time.Second)
+		// Wait for CRDs to be established
+		for _, obj := range stageOne {
+			if obj.GetKind() == "CustomResourceDefinition" {
+				if err := wait.PollUntilContextTimeout(ctx, 500*time.Millisecond, 1*time.Minute, true, func(ctx context.Context) (bool, error) {
+					crd := &apiextensionsv1.CustomResourceDefinition{}
+					if err := k8sClient.Get(ctx, client.ObjectKey{Name: obj.GetName()}, crd); err != nil {
+						logger.V(1).Info("Waiting for CRD to be available", "name", obj.GetName(), "error", err)
+						return false, nil
+					}
+					for _, cond := range crd.Status.Conditions {
+						if cond.Type == apiextensionsv1.Established && cond.Status == apiextensionsv1.ConditionTrue {
+							return true, nil
+						}
+					}
+					return false, nil
+				}); err != nil {
+					return fmt.Errorf("failed to wait for CRD %s to be established: %w", obj.GetName(), err)
+				}
+			}
+		}
 	}

internal/fluxinstall/install.go (1)

238-312: Improve error handling consistency in injectKubernetesServiceEnv.

The function collects only the first error via firstErr but silently ignores subsequent errors. While continuing on errors is reasonable (env injection is not critical), the current pattern may hide multiple failures.

Consider either:

1. Collecting all errors and returning a multi-error, or
2. Logging each error as it occurs (using the logger) instead of just tracking firstErr

🔎 Option: Log errors instead of just tracking firstErr

 func injectKubernetesServiceEnv(objects []*unstructured.Unstructured) error {
+	logger := log.Log.WithName("injectKubernetesServiceEnv")
 	kubernetesHost := os.Getenv("KUBERNETES_SERVICE_HOST")
 	kubernetesPort := os.Getenv("KUBERNETES_SERVICE_PORT")
 
 	// If neither variable is set, nothing to do
 	if kubernetesHost == "" && kubernetesPort == "" {
 		return nil
 	}
 
 	var firstErr error
 	for _, obj := range objects {
 		kind := obj.GetKind()
 		if kind != "Deployment" && kind != "StatefulSet" && kind != "DaemonSet" {
 			continue
 		}
 
 		// Navigate to spec.template.spec.containers
 		spec, found, err := unstructured.NestedMap(obj.Object, "spec", "template", "spec")
 		if !found {
 			continue
 		}
 		if err != nil {
+			logger.Error(err, "failed to get spec", "kind", kind, "name", obj.GetName())
 			if firstErr == nil {
 				firstErr = fmt.Errorf("failed to get spec for %s/%s: %w", kind, obj.GetName(), err)
 			}
 			continue
 		}
 
 		// Update containers
 		containers, found, err := unstructured.NestedSlice(spec, "containers")
 		if err != nil {
+			logger.Error(err, "failed to get containers", "kind", kind, "name", obj.GetName())
 			if firstErr == nil {
 				firstErr = fmt.Errorf("failed to get containers for %s/%s: %w", kind, obj.GetName(), err)
 			}
 			continue
 		}
 		// ... similar pattern for other error paths
 	}
 
 	return firstErr
 }

Configuration used: defaults

Review profile: CHILL

Plan: Pro

packages/core/flux-aio/Makefile (1)

15-34: LGTM for the dual-path manifest generation approach.

The split into update-old (Helm-based path with template includes) and update-new (operator-based path with embedded manifests) is a clean migration strategy. The TODO comments correctly document the temporary state and the known DNS issue.

The PHONY declaration and DNS/hostNetwork issues were already flagged in previous reviews.

cmd/cozystack-operator/main.go (2)

221-242: Consider using server-side apply for atomic create/update.

The current Get-Create/Update pattern works but server-side apply provides a single atomic operation that handles both cases, is consistent with fluxinstall.Install, and avoids potential race conditions.

🔎 Proposed refactor using server-side apply

-	existing := obj.DeepCopyObject().(client.Object)
-	key := client.ObjectKeyFromObject(obj)
-
-	err = k8sClient.Get(ctx, key, existing)
-	if err != nil {
-		if client.IgnoreNotFound(err) == nil {
-			// Resource doesn't exist, create it
-			if err := k8sClient.Create(ctx, obj); err != nil {
-				return fmt.Errorf("failed to create resource %s/%s: %w", obj.GetObjectKind().GroupVersionKind().Kind, obj.GetName(), err)
-			}
-			logger.Info("Created platform source resource", "kind", obj.GetObjectKind().GroupVersionKind().Kind, "name", obj.GetName())
-		} else {
-			return fmt.Errorf("failed to check if resource exists: %w", err)
-		}
-	} else {
-		// Resource exists, update it
-		obj.SetResourceVersion(existing.GetResourceVersion())
-		if err := k8sClient.Update(ctx, obj); err != nil {
-			return fmt.Errorf("failed to update resource %s/%s: %w", obj.GetObjectKind().GroupVersionKind().Kind, obj.GetName(), err)
-		}
-		logger.Info("Updated platform source resource", "kind", obj.GetObjectKind().GroupVersionKind().Kind, "name", obj.GetName())
-	}
+	patchOpts := []client.PatchOption{
+		client.ForceOwnership,
+		client.FieldOwner("cozystack-operator"),
+	}
+	if err := k8sClient.Patch(ctx, obj, client.Apply, patchOpts...); err != nil {
+		return fmt.Errorf("failed to apply resource %s/%s: %w", obj.GetObjectKind().GroupVersionKind().Kind, obj.GetName(), err)
+	}
+	logger.Info("Applied platform source resource", "kind", obj.GetObjectKind().GroupVersionKind().Kind, "name", obj.GetName())

---

375-378: Consider extracting the hardcoded namespace to a constant.

The namespace "cozy-system" is hardcoded in both generateOCIRepository and generateGitRepository. While likely correct for this project, extracting it to a package-level constant or making it configurable improves maintainability.

🔎 Proposed constant extraction

const platformSourceNamespace = "cozy-system"

Then use platformSourceNamespace in both generator functions.

Also applies to: 409-412

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Learnt from: CR
Repo: cozystack/cozystack PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-25T09:45:26.524Z
Learning: Applies to pkg/**/*.go : Follow controller-runtime patterns and kubebuilder style for Go code in pkg directory

cmd/cozystack-operator/main.go (1)

72-72: Unused flag: cozystackVersion is defined but never used.

The cozystackVersion flag is declared and parsed but not referenced anywhere in the code (same issue as previously flagged). Consider either using it (e.g., in startup logs, metrics, or version endpoints) or removing it to avoid confusion.

🔎 Suggested usage or removal

If you intend to use it for observability:

 	setupLog.Info("Starting controller manager")
+	setupLog.Info("Cozystack operator version", "version", cozystackVersion)
 	mgr, err := ctrl.NewManager(config, ctrl.Options{

Or remove the unused flag:

-	var cozystackVersion string
 	var platformSourceURL string

-	flag.StringVar(&cozystackVersion, "cozystack-version", "unknown",
-		"Version of Cozystack")
 	flag.StringVar(&platformSourceURL, "platform-source-url", "", "Platform source URL (oci:// or https://). If specified, generates OCIRepository or GitRepository resource.")

Also applies to: 87-88

cmd/cozystack-operator/main.go (1)

229-250: Consider using server-side apply for consistency and robustness.

The current Get-Create/Update pattern works but is less idiomatic than server-side apply (SSA) in controller-runtime. The fluxinstall.Install function already uses SSA (see internal/fluxinstall/install.go), so adopting it here would improve consistency.

Benefits of SSA:

• More atomic (single operation vs. Get + Create/Update)
• Simpler code (no need to handle Get errors and ResourceVersion)
• Better handles concurrent modifications
• Consistent with Flux installation flow

🔎 Refactor to use server-side apply

-	existing := obj.DeepCopyObject().(client.Object)
-	key := client.ObjectKeyFromObject(obj)
-
-	err = k8sClient.Get(ctx, key, existing)
-	if err != nil {
-		if client.IgnoreNotFound(err) == nil {
-			// Resource doesn't exist, create it
-			if err := k8sClient.Create(ctx, obj); err != nil {
-				return fmt.Errorf("failed to create resource %s/%s: %w", obj.GetObjectKind().GroupVersionKind().Kind, obj.GetName(), err)
-			}
-			logger.Info("Created platform source resource", "kind", obj.GetObjectKind().GroupVersionKind().Kind, "name", obj.GetName())
-		} else {
-			return fmt.Errorf("failed to check if resource exists: %w", err)
-		}
-	} else {
-		// Resource exists, update it
-		obj.SetResourceVersion(existing.GetResourceVersion())
-		if err := k8sClient.Update(ctx, obj); err != nil {
-			return fmt.Errorf("failed to update resource %s/%s: %w", obj.GetObjectKind().GroupVersionKind().Kind, obj.GetName(), err)
-		}
-		logger.Info("Updated platform source resource", "kind", obj.GetObjectKind().GroupVersionKind().Kind, "name", obj.GetName())
-	}
+	patchOpts := []client.PatchOption{
+		client.ForceOwnership,
+		client.FieldOwner("cozystack-operator"),
+	}
+	if err := k8sClient.Patch(ctx, obj, client.Apply, patchOpts...); err != nil {
+		return fmt.Errorf("failed to apply resource %s/%s: %w", obj.GetObjectKind().GroupVersionKind().Kind, obj.GetName(), err)
+	}
+	logger.Info("Applied platform source resource", "kind", obj.GetObjectKind().GroupVersionKind().Kind, "name", obj.GetName())

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Learnt from: CR
Repo: cozystack/cozystack PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-25T09:45:26.524Z
Learning: Applies to pkg/**/*.go : Follow controller-runtime patterns and kubebuilder style for Go code in pkg directory