CNF-23437: Wrap bare error returns for better debuggability

controllers/ipc_config_handlers.go

@@ -894,7 +894,7 @@ func validateAddressChanges(ipc *ipcv1.IPConfig) error {
 			v4,
 			ipc.Status.IPv4,
 		); err != nil {
-			return err
+			return fmt.Errorf("failed to validate IPv4 address changes: %w", err)
 		}
 	}
 
@@ -904,7 +904,7 @@ func validateAddressChanges(ipc *ipcv1.IPConfig) error {
 			v6,
 			ipc.Status.IPv6,
 		); err != nil {
-			return err
+			return fmt.Errorf("failed to validate IPv6 address changes: %w", err)
 		}
 	}
 

controllers/ipc_config_handlers_test.go

@@ -1414,6 +1414,70 @@ func TestStatusIPsMatchSpec(t *testing.T) {
 	})
 }
 
+func TestValidateAddressChanges(t *testing.T) {
+	t.Run("nil status network returns nil", func(t *testing.T) {
+		ipc := mkConfigIPC(t, false)
+		ipc.Spec.IPv4 = &ipcv1.IPv4Config{Address: "192.0.2.10"}
+		assert.NoError(t, validateAddressChanges(ipc))
+	})
+
+	t.Run("IPv4 validation error is wrapped", func(t *testing.T) {
+		ipc := mkConfigIPC(t, false)
+		ipc.Spec.IPv4 = &ipcv1.IPv4Config{
+			Address:        "192.0.2.10",
+			MachineNetwork: "192.0.3.0/24",
+		}
+		ipc.Status.IPv4 = &ipcv1.IPv4Status{Address: "192.0.2.10", MachineNetwork: "192.0.2.0/24"}
+		err := validateAddressChanges(ipc)
+		if assert.Error(t, err) {
+			assert.Contains(t, err.Error(), "failed to validate IPv4 address changes:")
+			assert.Contains(t, err.Error(), "machineNetwork can be changed only if address is also changed")
+		}
+	})
+
+	t.Run("IPv6 validation error is wrapped", func(t *testing.T) {
+		ipc := mkConfigIPC(t, false)
+		ipc.Spec.IPv6 = &ipcv1.IPv6Config{
+			Address: "2001:db8::10",
+			Gateway: "fe80::2",
+		}
+		ipc.Status.IPv6 = &ipcv1.IPv6Status{Address: "2001:db8::10", Gateway: "fe80::1"}
+		err := validateAddressChanges(ipc)
+		if assert.Error(t, err) {
+			assert.Contains(t, err.Error(), "failed to validate IPv6 address changes:")
+		}
+	})
+
+	t.Run("both families valid returns nil", func(t *testing.T) {
+		ipc := mkConfigIPC(t, false)
+		ipc.Spec.IPv4 = &ipcv1.IPv4Config{Address: "192.0.2.11"}
+		ipc.Status.IPv4 = &ipcv1.IPv4Status{Address: "192.0.2.10"}
+		assert.NoError(t, validateAddressChanges(ipc))
+	})
+
+	t.Run("nil IPC returns nil", func(t *testing.T) {
+		assert.NoError(t, validateAddressChanges(nil))
+	})
+
+	t.Run("equal DNS servers returns nil early", func(t *testing.T) {
+		ipc := mkConfigIPC(t, false)
+		ipc.Spec.IPv4 = &ipcv1.IPv4Config{Address: "192.0.2.10"}
+		ipc.Status.IPv4 = &ipcv1.IPv4Status{Address: "192.0.2.10"}
+		ipc.Spec.DNSServers = []ipcv1.IPAddress{"192.0.2.53"}
+		ipc.Status.DNSServers = []string{"192.0.2.53"}
+		assert.NoError(t, validateAddressChanges(ipc))
+	})
+
+	t.Run("IPv6 address change allows DNS server change", func(t *testing.T) {
+		ipc := mkConfigIPC(t, false)
+		ipc.Spec.IPv6 = &ipcv1.IPv6Config{Address: "2001:db8::11"}
+		ipc.Status.IPv6 = &ipcv1.IPv6Status{Address: "2001:db8::10"}
+		ipc.Spec.DNSServers = []ipcv1.IPAddress{"2001:db8::53"}
+		ipc.Status.DNSServers = []string{"2001:db8::52"}
+		assert.NoError(t, validateAddressChanges(ipc))
+	})
+}
+
 func TestIPAndCIDRHelpers(t *testing.T) {
 	t.Run("ipEqual normalizes CIDR", func(t *testing.T) {
 		assert.True(t, ipEqual("192.0.2.10/24", "192.0.2.10"))

controllers/ipc_idle_handlers.go

@@ -204,7 +204,7 @@ func (h *IPCIdleStageHandler) cleanuoUnbootedStateroots(logger logr.Logger) erro
 	}
 
 	if err := removeBootDirsByStaterootPrefixes(logger, h.ChrootOps, staterootsToRemove); err != nil {
-		return err
+		return fmt.Errorf("failed to remove boot dirs by stateroot prefixes: %w", err)
 	}
 
 	if err := CleanupUnbootedStateroots(logger, h.ChrootOps, h.OstreeClient, h.RPMOstreeClient); err != nil {

controllers/ipc_idle_handlers_test.go

@@ -422,6 +422,57 @@ func TestIPCIdleStageHandler_Handle(t *testing.T) {
 		assertStatusInvariants(t, updated, ipc)
 	})
 
+	t.Run("cleanup failing at removeBootDirsByStaterootPrefixes (ReadDir error) => idle=false failed and requeueWithError", func(t *testing.T) {
+		gc := gomock.NewController(t)
+		defer gc.Finish()
+
+		mockOps := ops.NewMockOps(gc)
+		mockRpm := rpmostreeclient.NewMockIClient(gc)
+		mockOstree := ostreeclient.NewMockIClient(gc)
+
+		ipc := mkIPCForIdle(t)
+		ipc.Status.Conditions = nil
+		ipc.Status.ValidNextStages = []ipcv1.IPConfigStage{ipcv1.IPStages.Idle}
+
+		k8sClient := newFakeClientWithIPC(t, scheme, ipc)
+		h := &IPCIdleStageHandler{
+			Client:          k8sClient,
+			NoncachedClient: k8sClient,
+			ChrootOps:       mockOps,
+			OstreeClient:    mockOstree,
+			RPMOstreeClient: mockRpm,
+		}
+
+		oldHC := CheckHealth
+		defer func() { CheckHealth = oldHC }()
+		CheckHealth = func(ctx context.Context, c client.Reader, l logr.Logger) error { return nil }
+
+		// QueryStatus returns an unbooted deployment so getStaterootsToRemove returns a non-empty list
+		status := &rpmostreeclient.Status{
+			Deployments: []rpmostreeclient.Deployment{
+				{OSName: "rhcos", Booted: true},
+				{OSName: "old-stateroot", Booted: false},
+			},
+		}
+
+		mockOps.EXPECT().RemountSysroot().Return(nil).Times(1)
+		mockRpm.EXPECT().QueryStatus().Return(status, nil).Times(1)
+		mockOps.EXPECT().RemountBoot().Return(nil).Times(1)
+		// ReadDir returns an error (not IsNotExist) to trigger the wrapped error path
+		mockOps.EXPECT().ReadDir(gomock.Any()).Return(nil, errors.New("permission denied")).Times(1)
+		mockOps.EXPECT().IsNotExist(gomock.Any()).Return(false).Times(1)
+
+		res, err := h.Handle(ctx, ipc)
+		assert.Error(t, err)
+		wantRes, _ := requeueWithError(err)
+		assert.Equal(t, wantRes, res)
+		assert.Contains(t, err.Error(), "failed to remove boot dirs by stateroot prefixes")
+
+		updated := mustGetIPC(t, k8sClient, common.IPConfigName)
+		assertIdleCond(t, updated, metav1.ConditionFalse, controllerutils.ConditionReasons.Failed, "failed to clean up unbooted stateroots")
+		assertStatusInvariants(t, updated, ipc)
+	})
+
 	t.Run("cleanup failing at workspace cleanup (RemoveAllFiles error) => idle=false failed and requeueWithError", func(t *testing.T) {
 		gc := gomock.NewController(t)
 		defer gc.Finish()

controllers/ipc_rollback_handlers.go

@@ -306,7 +306,7 @@ func (r *IPCRollbackTwoPhaseHandler) scheduleIPConfigRollback(
 
 func (h *IPCRollbackStageHandler) validateRollbackStart() error {
 	if err := h.validateUnbootedStaterootAvailable(); err != nil {
-		return err
+		return fmt.Errorf("failed to validate unbooted stateroot availability: %w", err)
 	}
 
 	return nil

internal/clusterconfig/clusterconfig.go

@@ -69,22 +69,22 @@ func (r *UpgradeClusterConfigGather) FetchClusterConfig(ctx context.Context, ost
 
 	clusterConfigPath, err := r.configDir(ostreeVarDir)
 	if err != nil {
-		return err
+		return fmt.Errorf("failed to get cluster config directory: %w", err)
 	}
 	manifestsDir := filepath.Join(clusterConfigPath, ManifestDir)
 
 	if err := r.fetchIDMS(ctx, manifestsDir); err != nil {
-		return err
+		return fmt.Errorf("failed to fetch IDMS: %w", err)
 	}
 
 	if err := r.fetchClusterInfo(ctx, clusterConfigPath); err != nil {
-		return err
+		return fmt.Errorf("failed to fetch cluster info: %w", err)
 	}
 	if err := r.fetchICSPs(ctx, manifestsDir); err != nil {
-		return err
+		return fmt.Errorf("failed to fetch ICSPs: %w", err)
 	}
 	if err := r.fetchNetworkConfig(ostreeVarDir); err != nil {
-		return err
+		return fmt.Errorf("failed to fetch network config: %w", err)
 	}
 
 	r.Log.Info("Successfully fetched cluster configuration")
@@ -350,42 +350,42 @@ func (r *UpgradeClusterConfigGather) fetchClusterInfo(ctx context.Context, clust
 
 	infraID, err := r.getInfraID(ctx)
 	if err != nil {
-		return err
+		return fmt.Errorf("failed to get infra ID: %w", err)
 	}
 
 	pullSecret, err := r.getPullSecret(ctx)
 	if err != nil {
-		return err
+		return fmt.Errorf("failed to retrieve pull secret: %w", err)
 	}
 
 	kubeadminPasswordHash, err := r.GetKubeadminPasswordHash(ctx)
 	if err != nil {
-		return err
+		return fmt.Errorf("failed to retrieve kubeadmin password hash: %w", err)
 	}
 
 	serverSSHKeys, err := r.GetServerSSHKeys(ctx)
 	if err != nil {
-		return err
+		return fmt.Errorf("failed to get server SSH keys: %w", err)
 	}
 
 	proxy, statusProxy, err := r.GetProxy(ctx)
 	if err != nil {
-		return err
+		return fmt.Errorf("failed to get proxy config: %w", err)
 	}
 
 	additionalTrustBundle, err := r.GetAdditionalTrustBundle(ctx)
 	if err != nil {
-		return err
+		return fmt.Errorf("failed to get additional trust bundle: %w", err)
 	}
 
 	installConfig, err := r.GetInstallConfig(ctx)
 	if err != nil {
-		return err
+		return fmt.Errorf("failed to get install config: %w", err)
 	}
 
 	chronyConfig, err := r.getChronyConfig()
 	if err != nil {
-		return err
+		return fmt.Errorf("failed to get chrony config: %w", err)
 	}
 
 	seedReconfiguration := SeedReconfigurationFromClusterInfo(clusterInfo, seedReconfigurationKubeconfigRetention,
@@ -414,7 +414,7 @@ func (r *UpgradeClusterConfigGather) fetchIDMS(ctx context.Context, manifestsDir
 	r.Log.Info("Fetching IDMS")
 	idms, err := r.getIDMSs(ctx)
 	if err != nil {
-		return fmt.Errorf("failed to fetch IDMS")
+		return fmt.Errorf("failed to get IDMS list: %w", err)
 	}
 
 	if len(idms.Items) < 1 {
@@ -514,7 +514,7 @@ func (r *UpgradeClusterConfigGather) fetchICSPs(ctx context.Context, manifestsDi
 		}
 		typeMeta, err := r.typeMetaForObject(&icp) //nolint:gosec
 		if err != nil {
-			return err
+			return fmt.Errorf("failed to get type meta for ICSP %s: %w", icp.Name, err)
 		}
 		obj.TypeMeta = *typeMeta
 		obj.Labels = icp.Labels
@@ -523,7 +523,7 @@ func (r *UpgradeClusterConfigGather) fetchICSPs(ctx context.Context, manifestsDi
 	}
 	typeMeta, err := r.typeMetaForObject(iscpsList)
 	if err != nil {
-		return err
+		return fmt.Errorf("failed to get type meta for ICSP list: %w", err)
 	}
 	iscpsList.TypeMeta = *typeMeta
 

internal/clusterconfig/clusterconfig_test.go

@@ -42,6 +42,7 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/client-go/kubernetes/scheme"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
@@ -737,3 +738,58 @@ func genSelfSignedKeyPair(cert *x509.Certificate) (*pem.Block, *ecdsa.PrivateKey
 
 	return &pem.Block{Type: "CERTIFICATE", Bytes: signedCert}, key, err
 }
+
+func TestFetchIDMS_ErrorWrapping(t *testing.T) {
+	// Use a minimal scheme without IDMS registered so List fails
+	minimalScheme := runtime.NewScheme()
+	_ = corev1.AddToScheme(minimalScheme)
+
+	c := fake.NewClientBuilder().WithScheme(minimalScheme).Build()
+	ucc := &UpgradeClusterConfigGather{
+		Client: c,
+		Log:    logr.Discard(),
+		Scheme: minimalScheme,
+	}
+
+	err := ucc.fetchIDMS(context.Background(), t.TempDir())
+	if assert.Error(t, err) {
+		assert.Contains(t, err.Error(), "failed to get IDMS list:")
+	}
+}
+
+func TestFetchICSPs_ErrorWrapping(t *testing.T) {
+	// Use a minimal scheme without ICSPs registered so List fails
+	minimalScheme := runtime.NewScheme()
+	_ = corev1.AddToScheme(minimalScheme)
+
+	c := fake.NewClientBuilder().WithScheme(minimalScheme).Build()
+	ucc := &UpgradeClusterConfigGather{
+		Client: c,
+		Log:    logr.Discard(),
+		Scheme: minimalScheme,
+	}
+
+	err := ucc.fetchICSPs(context.Background(), t.TempDir())
+	if assert.Error(t, err) {
+		assert.Contains(t, err.Error(), "failed list ImageContentSourcePolicy:")
+	}
+}
+
+func TestFetchClusterConfig_FetchIDMSErrorWrapping(t *testing.T) {
+	// Use a scheme that supports enough types for configDir to work,
+	// but NOT IDMS so fetchIDMS fails inside FetchClusterConfig.
+	minimalScheme := runtime.NewScheme()
+	_ = corev1.AddToScheme(minimalScheme)
+
+	c := fake.NewClientBuilder().WithScheme(minimalScheme).Build()
+	ucc := &UpgradeClusterConfigGather{
+		Client: c,
+		Log:    logr.Discard(),
+		Scheme: minimalScheme,
+	}
+
+	err := ucc.FetchClusterConfig(context.Background(), t.TempDir())
+	if assert.Error(t, err) {
+		assert.Contains(t, err.Error(), "failed to fetch IDMS:")
+	}
+}

internal/clusterconfig/lvmconfig.go

@@ -45,12 +45,12 @@ func (r *UpgradeClusterConfigGather) FetchLvmConfig(ctx context.Context, ostreeD
 	}
 
 	if err := r.fetchLvmDevices(lvmConfigPath); err != nil {
-		return err
+		return fmt.Errorf("failed to fetch LVM devices: %w", err)
 	}
 
 	manifestsDir := filepath.Join(ostreeDir, common.OptOpenshift, common.ClusterConfigDir, ManifestDir)
 	if err := r.fetchLocalVolumes(ctx, manifestsDir); err != nil {
-		return err
+		return fmt.Errorf("failed to fetch local volumes: %w", err)
 	}
 
 	r.Log.Info("Successfully fetched lvm configuration")

internal/clusterconfig/lvmconfig_test.go

@@ -198,3 +198,35 @@ func TestFetchLvmConfig(t *testing.T) {
 		})
 	}
 }
+
+func TestFetchLvmConfig_FetchLocalVolumesErrorWrapping(t *testing.T) {
+	tmpDir := t.TempDir()
+	hostPath = filepath.Join(tmpDir, "host")
+
+	// Register the CRD and a LocalVolume so fetchLocalVolumes reaches the
+	// MarshalToFile call, then sabotage the manifestsDir so writing fails.
+	c := fake.NewClientBuilder().WithScheme(testscheme).WithObjects(lvCRD, lv1, sc1).Build()
+	ucc := &UpgradeClusterConfigGather{
+		Client: c,
+		Log:    logr.Discard(),
+		Scheme: c.Scheme(),
+	}
+
+	// Create manifestsDir as a FILE instead of a directory so that
+	// MarshalToFile will fail when trying to write the LocalVolume JSON.
+	manifestsDir := filepath.Join(tmpDir, common.OptOpenshift, common.ClusterConfigDir, ManifestDir)
+	parentDir := filepath.Dir(manifestsDir)
+	if err := os.MkdirAll(parentDir, 0o700); err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	f, err := os.Create(manifestsDir)
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	f.Close()
+
+	err = ucc.FetchLvmConfig(context.Background(), tmpDir)
+	if assert.Error(t, err) {
+		assert.Contains(t, err.Error(), "failed to fetch local volumes:")
+	}
+}