diff --git a/controllers/ipc_config_handlers.go b/controllers/ipc_config_handlers.go index 82c10fd7d1..64c59fd450 100644 --- a/controllers/ipc_config_handlers.go +++ b/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) } } diff --git a/controllers/ipc_config_handlers_test.go b/controllers/ipc_config_handlers_test.go index 09b22f2915..bc4514e8f6 100644 --- a/controllers/ipc_config_handlers_test.go +++ b/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")) diff --git a/controllers/ipc_idle_handlers.go b/controllers/ipc_idle_handlers.go index 25dfc5e2b0..d9902505e7 100644 --- a/controllers/ipc_idle_handlers.go +++ b/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 { diff --git a/controllers/ipc_idle_handlers_test.go b/controllers/ipc_idle_handlers_test.go index dd609bf7da..b6fc7cd302 100644 --- a/controllers/ipc_idle_handlers_test.go +++ b/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() diff --git a/controllers/ipc_rollback_handlers.go b/controllers/ipc_rollback_handlers.go index 4862f89f22..79e6f1d8fb 100644 --- a/controllers/ipc_rollback_handlers.go +++ b/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 diff --git a/internal/clusterconfig/clusterconfig.go b/internal/clusterconfig/clusterconfig.go index 122b0d2590..5af2f02c7d 100644 --- a/internal/clusterconfig/clusterconfig.go +++ b/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 diff --git a/internal/clusterconfig/clusterconfig_test.go b/internal/clusterconfig/clusterconfig_test.go index 0505e38d70..be374f3f63 100644 --- a/internal/clusterconfig/clusterconfig_test.go +++ b/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:") + } +} diff --git a/internal/clusterconfig/lvmconfig.go b/internal/clusterconfig/lvmconfig.go index 245375b832..74ca00f798 100644 --- a/internal/clusterconfig/lvmconfig.go +++ b/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") diff --git a/internal/clusterconfig/lvmconfig_test.go b/internal/clusterconfig/lvmconfig_test.go index 9fddc4d911..96fd0cd770 100644 --- a/internal/clusterconfig/lvmconfig_test.go +++ b/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:") + } +}