From 6402ae65021e9daa2cfb3d20117167972e448d9e Mon Sep 17 00:00:00 2001 From: aantal Date: Wed, 3 Jun 2026 20:50:56 +0200 Subject: [PATCH 1/8] NO-ISSUE: make the isDiskEncryptionSetWithTpm function more robust In OCPBUGS-86731 the issue was likelly caused by DiskEncryptionEnabledOn was an empty string and the validation threated empty string as enabled. A new test is also added to test the robustness. --- internal/hardware/validator.go | 11 ++++++++--- internal/hardware/validator_test.go | 19 +++++++++++++++++++ 2 files changed, 27 insertions(+), 3 deletions(-) diff --git a/internal/hardware/validator.go b/internal/hardware/validator.go index 8309a7cf7057..50ef36ffcd45 100644 --- a/internal/hardware/validator.go +++ b/internal/hardware/validator.go @@ -399,9 +399,14 @@ func (v *validator) GetInfraEnvHostRequirements(ctx context.Context, infraEnv *c } func isDiskEncryptionSetWithTpm(c *common.Cluster) bool { - return c.DiskEncryption != nil && - swag.StringValue(c.DiskEncryption.EnableOn) != models.DiskEncryptionEnableOnNone && - swag.StringValue(c.DiskEncryption.Mode) == models.DiskEncryptionModeTpmv2 + if c.DiskEncryption == nil { + return false + } + enableOn := swag.StringValue(c.DiskEncryption.EnableOn) + if enableOn == "" || enableOn == models.DiskEncryptionEnableOnNone { + return false + } + return swag.StringValue(c.DiskEncryption.Mode) == models.DiskEncryptionModeTpmv2 } func (v *validator) GetPreflightHardwareRequirements(ctx context.Context, cluster *common.Cluster) (*models.PreflightHardwareRequirements, error) { diff --git a/internal/hardware/validator_test.go b/internal/hardware/validator_test.go index e8c2ab84bff1..74dd2a975b6a 100644 --- a/internal/hardware/validator_test.go +++ b/internal/hardware/validator_test.go @@ -1877,6 +1877,25 @@ var _ = Describe("Preflight host requirements", func() { Expect(result.Ocp.Worker.Quantitative.TpmEnabledInBios).To(BeFalse()) }) + It("TPM - unset enable_on with tpmv2 mode is treated as disabled", func() { + + diskEncryptionClusterID := strfmt.UUID(uuid.New().String()) + diskEncryptionCluster := &common.Cluster{Cluster: models.Cluster{ + ID: &diskEncryptionClusterID, + OpenshiftVersion: openShiftVersionNotInConfig, + DiskEncryption: &models.DiskEncryption{ + Mode: swag.String(models.DiskEncryptionModeTpmv2), + }, + }} + + operatorsMock.EXPECT().GetPreflightRequirementsBreakdownForCluster(gomock.Any(), gomock.Eq(diskEncryptionCluster)).Return(operatorRequirements, nil) + + result, err := hwvalidator.GetPreflightHardwareRequirements(context.TODO(), diskEncryptionCluster) + Expect(err).ToNot(HaveOccurred()) + Expect(result.Ocp.Master.Quantitative.TpmEnabledInBios).To(BeFalse()) + Expect(result.Ocp.Worker.Quantitative.TpmEnabledInBios).To(BeFalse()) + }) + It("Tang - all roles", func() { diskEncryptionClusterID := strfmt.UUID(uuid.New().String()) From 7a1a94d497dff942211ec7ee124de68e2b0cdc18 Mon Sep 17 00:00:00 2001 From: aantal Date: Tue, 9 Jun 2026 14:19:46 +0200 Subject: [PATCH 2/8] apply disk encryption defaults on cluster create and update Normalize nil or empty disk_encryption enable_on and mode to none and tpmv2 when syncing from AgentClusterInstall and when persisting cluster updates. Prevents writing an empty enable_on while mode stays tpmv2, which could leave hosts stuck in Discovering during hardware validation. --- internal/bminventory/inventory.go | 13 ++--- internal/common/disk_encryption.go | 29 ++++++++++ internal/common/disk_encryption_test.go | 58 +++++++++++++++++++ .../clusterdeployments_controller.go | 16 +++-- subsystem/kubeapi/kubeapi_test.go | 2 +- 5 files changed, 105 insertions(+), 13 deletions(-) create mode 100644 internal/common/disk_encryption.go create mode 100644 internal/common/disk_encryption_test.go diff --git a/internal/bminventory/inventory.go b/internal/bminventory/inventory.go index 30c9cded1930..3ff8dfb4d323 100644 --- a/internal/bminventory/inventory.go +++ b/internal/bminventory/inventory.go @@ -919,14 +919,7 @@ func setDiskEncryptionWithDefaultValues(c *models.Cluster, config *models.DiskEn } c.DiskEncryption = config - - if c.DiskEncryption.EnableOn == nil { - c.DiskEncryption.EnableOn = swag.String(models.DiskEncryptionEnableOnNone) - } - - if config.Mode == nil { - c.DiskEncryption.Mode = swag.String(models.DiskEncryptionModeTpmv2) - } + common.ApplyDiskEncryptionDefaults(c.DiskEncryption) } func updateSSHPublicKey(cluster *common.Cluster) error { @@ -2719,9 +2712,13 @@ func (b *bareMetalInventory) updateClusterData(_ context.Context, cluster *commo return common.NewApiError(http.StatusBadRequest, errors.New(msg)) } if params.ClusterUpdateParams.DiskEncryption.EnableOn != nil { + enableOn, _ := common.DiskEncryptionFieldDefaults(params.ClusterUpdateParams.DiskEncryption.EnableOn, nil) + params.ClusterUpdateParams.DiskEncryption.EnableOn = swag.String(enableOn) updates["disk_encryption_enable_on"] = params.ClusterUpdateParams.DiskEncryption.EnableOn } if params.ClusterUpdateParams.DiskEncryption.Mode != nil { + _, mode := common.DiskEncryptionFieldDefaults(nil, params.ClusterUpdateParams.DiskEncryption.Mode) + params.ClusterUpdateParams.DiskEncryption.Mode = swag.String(mode) updates["disk_encryption_mode"] = params.ClusterUpdateParams.DiskEncryption.Mode } if params.ClusterUpdateParams.DiskEncryption.TangServers != "" { diff --git a/internal/common/disk_encryption.go b/internal/common/disk_encryption.go new file mode 100644 index 000000000000..5c9110e13432 --- /dev/null +++ b/internal/common/disk_encryption.go @@ -0,0 +1,29 @@ +package common + +import ( + "github.com/go-openapi/swag" + "github.com/openshift/assisted-service/models" +) + +// DiskEncryptionFieldDefaults returns enable_on and mode with defaults for nil or empty values. +func DiskEncryptionFieldDefaults(enableOn, mode *string) (string, string) { + enableOnValue := swag.StringValue(enableOn) + if enableOnValue == "" { + enableOnValue = models.DiskEncryptionEnableOnNone + } + modeValue := swag.StringValue(mode) + if modeValue == "" { + modeValue = models.DiskEncryptionModeTpmv2 + } + return enableOnValue, modeValue +} + +// ApplyDiskEncryptionDefaults normalizes nil or empty disk encryption fields to their defaults. +func ApplyDiskEncryptionDefaults(diskEncryption *models.DiskEncryption) { + if diskEncryption == nil { + return + } + enableOn, mode := DiskEncryptionFieldDefaults(diskEncryption.EnableOn, diskEncryption.Mode) + diskEncryption.EnableOn = swag.String(enableOn) + diskEncryption.Mode = swag.String(mode) +} diff --git a/internal/common/disk_encryption_test.go b/internal/common/disk_encryption_test.go new file mode 100644 index 000000000000..c335f9051498 --- /dev/null +++ b/internal/common/disk_encryption_test.go @@ -0,0 +1,58 @@ +package common + +import ( + "testing" + + "github.com/go-openapi/swag" + "github.com/openshift/assisted-service/models" + "github.com/stretchr/testify/assert" +) + +func TestDiskEncryptionFieldDefaults(t *testing.T) { + enableOn, mode := DiskEncryptionFieldDefaults(nil, nil) + assert.Equal(t, models.DiskEncryptionEnableOnNone, enableOn) + assert.Equal(t, models.DiskEncryptionModeTpmv2, mode) + + enableOn, mode = DiskEncryptionFieldDefaults(swag.String(""), swag.String("")) + assert.Equal(t, models.DiskEncryptionEnableOnNone, enableOn) + assert.Equal(t, models.DiskEncryptionModeTpmv2, mode) + + enableOn, mode = DiskEncryptionFieldDefaults(swag.String(models.DiskEncryptionEnableOnMasters), swag.String(models.DiskEncryptionModeTang)) + assert.Equal(t, models.DiskEncryptionEnableOnMasters, enableOn) + assert.Equal(t, models.DiskEncryptionModeTang, mode) +} + +func TestApplyDiskEncryptionDefaults(t *testing.T) { + t.Run("nil input", func(t *testing.T) { + assert.NotPanics(t, func() { + ApplyDiskEncryptionDefaults(nil) + }) + }) + + t.Run("nil fields", func(t *testing.T) { + diskEncryption := &models.DiskEncryption{} + ApplyDiskEncryptionDefaults(diskEncryption) + assert.Equal(t, swag.String(models.DiskEncryptionEnableOnNone), diskEncryption.EnableOn) + assert.Equal(t, swag.String(models.DiskEncryptionModeTpmv2), diskEncryption.Mode) + }) + + t.Run("empty string fields", func(t *testing.T) { + diskEncryption := &models.DiskEncryption{ + EnableOn: swag.String(""), + Mode: swag.String(""), + } + ApplyDiskEncryptionDefaults(diskEncryption) + assert.Equal(t, swag.String(models.DiskEncryptionEnableOnNone), diskEncryption.EnableOn) + assert.Equal(t, swag.String(models.DiskEncryptionModeTpmv2), diskEncryption.Mode) + }) + + t.Run("explicit values", func(t *testing.T) { + diskEncryption := &models.DiskEncryption{ + EnableOn: swag.String(models.DiskEncryptionEnableOnMasters), + Mode: swag.String(models.DiskEncryptionModeTang), + } + ApplyDiskEncryptionDefaults(diskEncryption) + assert.Equal(t, swag.String(models.DiskEncryptionEnableOnMasters), diskEncryption.EnableOn) + assert.Equal(t, swag.String(models.DiskEncryptionModeTang), diskEncryption.Mode) + }) +} diff --git a/internal/controller/controllers/clusterdeployments_controller.go b/internal/controller/controllers/clusterdeployments_controller.go index c6caeeae05ae..97bb724e3b4e 100644 --- a/internal/controller/controllers/clusterdeployments_controller.go +++ b/internal/controller/controllers/clusterdeployments_controller.go @@ -1152,8 +1152,12 @@ func (r *ClusterDeploymentsReconciler) updateIfNeeded( if cluster.DiskEncryption == nil { // true when current cluster configuration does not include disk encryption cluster.DiskEncryption = &models.DiskEncryption{} } - updateString(swag.StringValue(clusterInstall.Spec.DiskEncryption.EnableOn), swag.StringValue(cluster.DiskEncryption.EnableOn), ¶ms.DiskEncryption.EnableOn) - updateString(swag.StringValue(clusterInstall.Spec.DiskEncryption.Mode), swag.StringValue(cluster.DiskEncryption.Mode), ¶ms.DiskEncryption.Mode) + enableOn, mode := common.DiskEncryptionFieldDefaults( + clusterInstall.Spec.DiskEncryption.EnableOn, + clusterInstall.Spec.DiskEncryption.Mode, + ) + updateString(enableOn, swag.StringValue(cluster.DiskEncryption.EnableOn), ¶ms.DiskEncryption.EnableOn) + updateString(mode, swag.StringValue(cluster.DiskEncryption.Mode), ¶ms.DiskEncryption.Mode) if clusterInstall.Spec.DiskEncryption.TangServers != cluster.DiskEncryption.TangServers { params.DiskEncryption.TangServers = clusterInstall.Spec.DiskEncryption.TangServers update = true @@ -1527,9 +1531,13 @@ func CreateClusterParams(clusterDeployment *hivev1.ClusterDeployment, clusterIns } if isDiskEncryptionEnabled(clusterInstall) { + enableOn, mode := common.DiskEncryptionFieldDefaults( + clusterInstall.Spec.DiskEncryption.EnableOn, + clusterInstall.Spec.DiskEncryption.Mode, + ) clusterParams.DiskEncryption = &models.DiskEncryption{ - EnableOn: clusterInstall.Spec.DiskEncryption.EnableOn, - Mode: clusterInstall.Spec.DiskEncryption.Mode, + EnableOn: swag.String(enableOn), + Mode: swag.String(mode), TangServers: clusterInstall.Spec.DiskEncryption.TangServers, } } diff --git a/subsystem/kubeapi/kubeapi_test.go b/subsystem/kubeapi/kubeapi_test.go index 93c6d0479e6f..bf810fb0bd93 100644 --- a/subsystem/kubeapi/kubeapi_test.go +++ b/subsystem/kubeapi/kubeapi_test.go @@ -3388,7 +3388,7 @@ location = "%s" } deployAgentClusterInstallCRD(ctx, kubeClient, aciSpec, clusterDeploymentSpec.ClusterInstallRef.Name) checkAgentClusterInstallCondition(ctx, installkey, hiveext.ClusterRequirementsMetCondition, hiveext.ClusterNotReadyReason) - verifyDiskEncryptionConfig(swag.String(models.DiskEncryptionEnableOnNone), nil, "") + verifyDiskEncryptionConfig(swag.String(models.DiskEncryptionEnableOnNone), swag.String(models.DiskEncryptionModeTpmv2), "") By("update deployment with disk encryption enabled with tpmv2 on master only") aciSpec = getDefaultAgentClusterInstallSpec(clusterDeploymentSpec.ClusterName) From 68c8c30a085ddc1ecc2f4d2701a4c0c9b2fa83c3 Mon Sep 17 00:00:00 2001 From: aantal Date: Fri, 12 Jun 2026 13:08:38 +0200 Subject: [PATCH 3/8] extract internal/diskencryption package --- internal/bminventory/inventory.go | 7 +- internal/common/disk_encryption.go | 29 ---- internal/common/disk_encryption_test.go | 58 -------- .../clusterdeployments_controller.go | 12 +- internal/diskencryption/disk_encryption.go | 69 +++++++++ .../diskencryption/disk_encryption_test.go | 138 ++++++++++++++++++ internal/hardware/validator.go | 14 +- .../tang_connectivity_check_cmd.go | 5 +- internal/host/hostutil/host_utils.go | 18 --- internal/host/hostutil/host_utils_test.go | 45 ------ internal/host/validator.go | 3 +- internal/network/manifests_generator.go | 3 +- 12 files changed, 224 insertions(+), 177 deletions(-) delete mode 100644 internal/common/disk_encryption.go delete mode 100644 internal/common/disk_encryption_test.go create mode 100644 internal/diskencryption/disk_encryption.go create mode 100644 internal/diskencryption/disk_encryption_test.go diff --git a/internal/bminventory/inventory.go b/internal/bminventory/inventory.go index 3ff8dfb4d323..b596b6da7bf7 100644 --- a/internal/bminventory/inventory.go +++ b/internal/bminventory/inventory.go @@ -27,6 +27,7 @@ import ( eventgen "github.com/openshift/assisted-service/internal/common/events" ignitioncommon "github.com/openshift/assisted-service/internal/common/ignition" "github.com/openshift/assisted-service/internal/constants" + "github.com/openshift/assisted-service/internal/diskencryption" "github.com/openshift/assisted-service/internal/dns" eventsapi "github.com/openshift/assisted-service/internal/events/api" "github.com/openshift/assisted-service/internal/featuresupport" @@ -919,7 +920,7 @@ func setDiskEncryptionWithDefaultValues(c *models.Cluster, config *models.DiskEn } c.DiskEncryption = config - common.ApplyDiskEncryptionDefaults(c.DiskEncryption) + diskencryption.ApplyDiskEncryptionDefaults(c.DiskEncryption) } func updateSSHPublicKey(cluster *common.Cluster) error { @@ -2712,12 +2713,12 @@ func (b *bareMetalInventory) updateClusterData(_ context.Context, cluster *commo return common.NewApiError(http.StatusBadRequest, errors.New(msg)) } if params.ClusterUpdateParams.DiskEncryption.EnableOn != nil { - enableOn, _ := common.DiskEncryptionFieldDefaults(params.ClusterUpdateParams.DiskEncryption.EnableOn, nil) + enableOn, _ := diskencryption.DiskEncryptionFieldDefaults(params.ClusterUpdateParams.DiskEncryption.EnableOn, nil) params.ClusterUpdateParams.DiskEncryption.EnableOn = swag.String(enableOn) updates["disk_encryption_enable_on"] = params.ClusterUpdateParams.DiskEncryption.EnableOn } if params.ClusterUpdateParams.DiskEncryption.Mode != nil { - _, mode := common.DiskEncryptionFieldDefaults(nil, params.ClusterUpdateParams.DiskEncryption.Mode) + _, mode := diskencryption.DiskEncryptionFieldDefaults(nil, params.ClusterUpdateParams.DiskEncryption.Mode) params.ClusterUpdateParams.DiskEncryption.Mode = swag.String(mode) updates["disk_encryption_mode"] = params.ClusterUpdateParams.DiskEncryption.Mode } diff --git a/internal/common/disk_encryption.go b/internal/common/disk_encryption.go deleted file mode 100644 index 5c9110e13432..000000000000 --- a/internal/common/disk_encryption.go +++ /dev/null @@ -1,29 +0,0 @@ -package common - -import ( - "github.com/go-openapi/swag" - "github.com/openshift/assisted-service/models" -) - -// DiskEncryptionFieldDefaults returns enable_on and mode with defaults for nil or empty values. -func DiskEncryptionFieldDefaults(enableOn, mode *string) (string, string) { - enableOnValue := swag.StringValue(enableOn) - if enableOnValue == "" { - enableOnValue = models.DiskEncryptionEnableOnNone - } - modeValue := swag.StringValue(mode) - if modeValue == "" { - modeValue = models.DiskEncryptionModeTpmv2 - } - return enableOnValue, modeValue -} - -// ApplyDiskEncryptionDefaults normalizes nil or empty disk encryption fields to their defaults. -func ApplyDiskEncryptionDefaults(diskEncryption *models.DiskEncryption) { - if diskEncryption == nil { - return - } - enableOn, mode := DiskEncryptionFieldDefaults(diskEncryption.EnableOn, diskEncryption.Mode) - diskEncryption.EnableOn = swag.String(enableOn) - diskEncryption.Mode = swag.String(mode) -} diff --git a/internal/common/disk_encryption_test.go b/internal/common/disk_encryption_test.go deleted file mode 100644 index c335f9051498..000000000000 --- a/internal/common/disk_encryption_test.go +++ /dev/null @@ -1,58 +0,0 @@ -package common - -import ( - "testing" - - "github.com/go-openapi/swag" - "github.com/openshift/assisted-service/models" - "github.com/stretchr/testify/assert" -) - -func TestDiskEncryptionFieldDefaults(t *testing.T) { - enableOn, mode := DiskEncryptionFieldDefaults(nil, nil) - assert.Equal(t, models.DiskEncryptionEnableOnNone, enableOn) - assert.Equal(t, models.DiskEncryptionModeTpmv2, mode) - - enableOn, mode = DiskEncryptionFieldDefaults(swag.String(""), swag.String("")) - assert.Equal(t, models.DiskEncryptionEnableOnNone, enableOn) - assert.Equal(t, models.DiskEncryptionModeTpmv2, mode) - - enableOn, mode = DiskEncryptionFieldDefaults(swag.String(models.DiskEncryptionEnableOnMasters), swag.String(models.DiskEncryptionModeTang)) - assert.Equal(t, models.DiskEncryptionEnableOnMasters, enableOn) - assert.Equal(t, models.DiskEncryptionModeTang, mode) -} - -func TestApplyDiskEncryptionDefaults(t *testing.T) { - t.Run("nil input", func(t *testing.T) { - assert.NotPanics(t, func() { - ApplyDiskEncryptionDefaults(nil) - }) - }) - - t.Run("nil fields", func(t *testing.T) { - diskEncryption := &models.DiskEncryption{} - ApplyDiskEncryptionDefaults(diskEncryption) - assert.Equal(t, swag.String(models.DiskEncryptionEnableOnNone), diskEncryption.EnableOn) - assert.Equal(t, swag.String(models.DiskEncryptionModeTpmv2), diskEncryption.Mode) - }) - - t.Run("empty string fields", func(t *testing.T) { - diskEncryption := &models.DiskEncryption{ - EnableOn: swag.String(""), - Mode: swag.String(""), - } - ApplyDiskEncryptionDefaults(diskEncryption) - assert.Equal(t, swag.String(models.DiskEncryptionEnableOnNone), diskEncryption.EnableOn) - assert.Equal(t, swag.String(models.DiskEncryptionModeTpmv2), diskEncryption.Mode) - }) - - t.Run("explicit values", func(t *testing.T) { - diskEncryption := &models.DiskEncryption{ - EnableOn: swag.String(models.DiskEncryptionEnableOnMasters), - Mode: swag.String(models.DiskEncryptionModeTang), - } - ApplyDiskEncryptionDefaults(diskEncryption) - assert.Equal(t, swag.String(models.DiskEncryptionEnableOnMasters), diskEncryption.EnableOn) - assert.Equal(t, swag.String(models.DiskEncryptionModeTang), diskEncryption.Mode) - }) -} diff --git a/internal/controller/controllers/clusterdeployments_controller.go b/internal/controller/controllers/clusterdeployments_controller.go index 97bb724e3b4e..a05e85de83c5 100644 --- a/internal/controller/controllers/clusterdeployments_controller.go +++ b/internal/controller/controllers/clusterdeployments_controller.go @@ -42,6 +42,7 @@ import ( "github.com/openshift/assisted-service/internal/common" "github.com/openshift/assisted-service/internal/constants" "github.com/openshift/assisted-service/internal/controller/controllers/mirrorregistry" + "github.com/openshift/assisted-service/internal/diskencryption" "github.com/openshift/assisted-service/internal/gencrypto" "github.com/openshift/assisted-service/internal/host" manifestsapi "github.com/openshift/assisted-service/internal/manifests/api" @@ -760,11 +761,6 @@ func isUserManagedNetwork(clusterInstall *hiveext.AgentClusterInstall) bool { clusterInstall.Spec.ProvisionRequirements.ControlPlaneAgents == 1 && clusterInstall.Spec.ProvisionRequirements.WorkerAgents == 0 } -func isDiskEncryptionEnabled(clusterInstall *hiveext.AgentClusterInstall) bool { - return clusterInstall.Spec.DiskEncryption != nil && - swag.StringValue(clusterInstall.Spec.DiskEncryption.EnableOn) != models.DiskEncryptionEnableOnNone -} - // see https://docs.openshift.com/container-platform/4.7/installing/installing_platform_agnostic/installing-platform-agnostic.html#installation-bare-metal-config-yaml_installing-platform-agnostic func hyperthreadingInSpec(clusterInstall *hiveext.AgentClusterInstall) bool { //check if either master or worker pool hyperthreading settings are explicitly specified @@ -1152,7 +1148,7 @@ func (r *ClusterDeploymentsReconciler) updateIfNeeded( if cluster.DiskEncryption == nil { // true when current cluster configuration does not include disk encryption cluster.DiskEncryption = &models.DiskEncryption{} } - enableOn, mode := common.DiskEncryptionFieldDefaults( + enableOn, mode := diskencryption.DiskEncryptionFieldDefaults( clusterInstall.Spec.DiskEncryption.EnableOn, clusterInstall.Spec.DiskEncryption.Mode, ) @@ -1530,8 +1526,8 @@ func CreateClusterParams(clusterDeployment *hivev1.ClusterDeployment, clusterIns clusterParams.Hyperthreading = getHyperthreading(clusterInstall) } - if isDiskEncryptionEnabled(clusterInstall) { - enableOn, mode := common.DiskEncryptionFieldDefaults( + if clusterInstall.Spec.DiskEncryption != nil && diskencryption.IsEnabled(clusterInstall.Spec.DiskEncryption.EnableOn) { + enableOn, mode := diskencryption.DiskEncryptionFieldDefaults( clusterInstall.Spec.DiskEncryption.EnableOn, clusterInstall.Spec.DiskEncryption.Mode, ) diff --git a/internal/diskencryption/disk_encryption.go b/internal/diskencryption/disk_encryption.go new file mode 100644 index 000000000000..2832a7bd5594 --- /dev/null +++ b/internal/diskencryption/disk_encryption.go @@ -0,0 +1,69 @@ +package diskencryption + +import ( + "strings" + + "github.com/go-openapi/swag" + "github.com/openshift/assisted-service/models" + "github.com/thoas/go-funk" +) + +// IsEnabled reports whether disk encryption is enabled for any role. +// Empty or "none" enable_on values are treated as disabled. +func IsEnabled(enableOn *string) bool { + v := swag.StringValue(enableOn) + return v != "" && v != models.DiskEncryptionEnableOnNone +} + +// DiskEncryptionFieldDefaults returns enable_on and mode with defaults for nil or empty values. +func DiskEncryptionFieldDefaults(enableOn, mode *string) (string, string) { + enableOnValue := swag.StringValue(enableOn) + if enableOnValue == "" { + enableOnValue = models.DiskEncryptionEnableOnNone + } + modeValue := swag.StringValue(mode) + if modeValue == "" { + modeValue = models.DiskEncryptionModeTpmv2 + } + return enableOnValue, modeValue +} + +// ApplyDiskEncryptionDefaults normalizes nil or empty disk encryption fields to their defaults. +func ApplyDiskEncryptionDefaults(diskEncryption *models.DiskEncryption) { + if diskEncryption == nil { + return + } + enableOn, mode := DiskEncryptionFieldDefaults(diskEncryption.EnableOn, diskEncryption.Mode) + diskEncryption.EnableOn = swag.String(enableOn) + diskEncryption.Mode = swag.String(mode) +} + +// IsSetWithTpm reports whether TPM-based disk encryption is configured for any role. +func IsSetWithTpm(diskEncryption *models.DiskEncryption) bool { + if diskEncryption == nil { + return false + } + if !IsEnabled(diskEncryption.EnableOn) { + return false + } + return swag.StringValue(diskEncryption.Mode) == models.DiskEncryptionModeTpmv2 +} + +// EnabledForRole reports whether disk encryption is enabled for the given host role. +func EnabledForRole(encryption models.DiskEncryption, role models.HostRole) bool { + if swag.StringValue(encryption.EnableOn) == models.DiskEncryptionEnableOnAll { + return true + } + + enabledGroups := strings.Split(swag.StringValue(encryption.EnableOn), ",") + if role == models.HostRoleMaster || role == models.HostRoleBootstrap { + return funk.ContainsString(enabledGroups, models.DiskEncryptionEnableOnMasters) + } + if role == models.HostRoleArbiter { + return funk.ContainsString(enabledGroups, models.DiskEncryptionEnableOnArbiters) + } + if role == models.HostRoleWorker { + return funk.ContainsString(enabledGroups, models.DiskEncryptionEnableOnWorkers) + } + return false +} diff --git a/internal/diskencryption/disk_encryption_test.go b/internal/diskencryption/disk_encryption_test.go new file mode 100644 index 000000000000..2d7a497b5865 --- /dev/null +++ b/internal/diskencryption/disk_encryption_test.go @@ -0,0 +1,138 @@ +package diskencryption + +import ( + "testing" + + "github.com/go-openapi/swag" + "github.com/openshift/assisted-service/models" + "github.com/stretchr/testify/assert" +) + +func TestIsEnabled(t *testing.T) { + assert.False(t, IsEnabled(nil)) + assert.False(t, IsEnabled(swag.String(""))) + assert.False(t, IsEnabled(swag.String(models.DiskEncryptionEnableOnNone))) + assert.True(t, IsEnabled(swag.String(models.DiskEncryptionEnableOnMasters))) +} + +func TestDiskEncryptionFieldDefaults(t *testing.T) { + enableOn, mode := DiskEncryptionFieldDefaults(nil, nil) + assert.Equal(t, models.DiskEncryptionEnableOnNone, enableOn) + assert.Equal(t, models.DiskEncryptionModeTpmv2, mode) + + enableOn, mode = DiskEncryptionFieldDefaults(swag.String(""), swag.String("")) + assert.Equal(t, models.DiskEncryptionEnableOnNone, enableOn) + assert.Equal(t, models.DiskEncryptionModeTpmv2, mode) + + enableOn, mode = DiskEncryptionFieldDefaults(swag.String(models.DiskEncryptionEnableOnMasters), swag.String(models.DiskEncryptionModeTang)) + assert.Equal(t, models.DiskEncryptionEnableOnMasters, enableOn) + assert.Equal(t, models.DiskEncryptionModeTang, mode) +} + +func TestApplyDiskEncryptionDefaults(t *testing.T) { + t.Run("nil input", func(t *testing.T) { + assert.NotPanics(t, func() { + ApplyDiskEncryptionDefaults(nil) + }) + }) + + t.Run("nil fields", func(t *testing.T) { + diskEncryption := &models.DiskEncryption{} + ApplyDiskEncryptionDefaults(diskEncryption) + assert.Equal(t, swag.String(models.DiskEncryptionEnableOnNone), diskEncryption.EnableOn) + assert.Equal(t, swag.String(models.DiskEncryptionModeTpmv2), diskEncryption.Mode) + }) + + t.Run("empty string fields", func(t *testing.T) { + diskEncryption := &models.DiskEncryption{ + EnableOn: swag.String(""), + Mode: swag.String(""), + } + ApplyDiskEncryptionDefaults(diskEncryption) + assert.Equal(t, swag.String(models.DiskEncryptionEnableOnNone), diskEncryption.EnableOn) + assert.Equal(t, swag.String(models.DiskEncryptionModeTpmv2), diskEncryption.Mode) + }) + + t.Run("explicit values", func(t *testing.T) { + diskEncryption := &models.DiskEncryption{ + EnableOn: swag.String(models.DiskEncryptionEnableOnMasters), + Mode: swag.String(models.DiskEncryptionModeTang), + } + ApplyDiskEncryptionDefaults(diskEncryption) + assert.Equal(t, swag.String(models.DiskEncryptionEnableOnMasters), diskEncryption.EnableOn) + assert.Equal(t, swag.String(models.DiskEncryptionModeTang), diskEncryption.Mode) + }) +} + +func TestIsSetWithTpm(t *testing.T) { + assert.False(t, IsSetWithTpm(nil)) + assert.False(t, IsSetWithTpm(&models.DiskEncryption{ + EnableOn: swag.String(""), + Mode: swag.String(models.DiskEncryptionModeTpmv2), + })) + assert.False(t, IsSetWithTpm(&models.DiskEncryption{ + EnableOn: swag.String(models.DiskEncryptionEnableOnNone), + Mode: swag.String(models.DiskEncryptionModeTpmv2), + })) + assert.False(t, IsSetWithTpm(&models.DiskEncryption{ + EnableOn: swag.String(models.DiskEncryptionEnableOnMasters), + Mode: swag.String(models.DiskEncryptionModeTang), + })) + assert.True(t, IsSetWithTpm(&models.DiskEncryption{ + EnableOn: swag.String(models.DiskEncryptionEnableOnMasters), + Mode: swag.String(models.DiskEncryptionModeTpmv2), + })) +} + +func TestEnabledForRole(t *testing.T) { + testCases := []struct { + name string + enabledOn string + role models.HostRole + isEnabled bool + }{ + {"enabledOn all, role master", models.DiskEncryptionEnableOnAll, models.HostRoleMaster, true}, + {"enabledOn all, role bootstrap", models.DiskEncryptionEnableOnAll, models.HostRoleBootstrap, true}, + {"enabledOn all, role arbiter", models.DiskEncryptionEnableOnAll, models.HostRoleArbiter, true}, + {"enabledOn all, role worker", models.DiskEncryptionEnableOnAll, models.HostRoleWorker, true}, + {"enabledOn masters,arbiters,workers, role master", models.DiskEncryptionEnableOnMastersArbitersWorkers, models.HostRoleMaster, true}, + {"enabledOn masters,arbiters,workers, role bootstrap", models.DiskEncryptionEnableOnMastersArbitersWorkers, models.HostRoleBootstrap, true}, + {"enabledOn masters,arbiters,workers, role arbiter", models.DiskEncryptionEnableOnMastersArbitersWorkers, models.HostRoleArbiter, true}, + {"enabledOn masters,arbiters,workers, role worker", models.DiskEncryptionEnableOnMastersArbitersWorkers, models.HostRoleWorker, true}, + {"enabledOn masters,arbiters, role master", models.DiskEncryptionEnableOnMastersArbiters, models.HostRoleMaster, true}, + {"enabledOn masters,arbiters, role bootstrap", models.DiskEncryptionEnableOnMastersArbiters, models.HostRoleBootstrap, true}, + {"enabledOn masters,arbiters, role arbiter", models.DiskEncryptionEnableOnMastersArbiters, models.HostRoleArbiter, true}, + {"enabledOn masters,arbiters, role worker", models.DiskEncryptionEnableOnMastersArbiters, models.HostRoleWorker, false}, + {"enabledOn masters,workers, role master", models.DiskEncryptionEnableOnMastersWorkers, models.HostRoleMaster, true}, + {"enabledOn masters,workers, role bootstrap", models.DiskEncryptionEnableOnMastersWorkers, models.HostRoleBootstrap, true}, + {"enabledOn masters,workers, role arbiter", models.DiskEncryptionEnableOnMastersWorkers, models.HostRoleArbiter, false}, + {"enabledOn masters,workers, role worker", models.DiskEncryptionEnableOnMastersWorkers, models.HostRoleWorker, true}, + {"enabledOn arbiters,workers, role master", models.DiskEncryptionEnableOnArbitersWorkers, models.HostRoleMaster, false}, + {"enabledOn arbiters,workers, role bootstrap", models.DiskEncryptionEnableOnArbitersWorkers, models.HostRoleBootstrap, false}, + {"enabledOn arbiters,workers, role arbiter", models.DiskEncryptionEnableOnArbitersWorkers, models.HostRoleArbiter, true}, + {"enabledOn arbiters,workers, role worker", models.DiskEncryptionEnableOnArbitersWorkers, models.HostRoleWorker, true}, + {"enabledOn masters, role master", models.DiskEncryptionEnableOnMasters, models.HostRoleMaster, true}, + {"enabledOn masters, role bootstrap", models.DiskEncryptionEnableOnMasters, models.HostRoleBootstrap, true}, + {"enabledOn masters, role arbiter", models.DiskEncryptionEnableOnMasters, models.HostRoleArbiter, false}, + {"enabledOn masters, role worker", models.DiskEncryptionEnableOnMasters, models.HostRoleWorker, false}, + {"enabledOn arbiters, role master", models.DiskEncryptionEnableOnArbiters, models.HostRoleMaster, false}, + {"enabledOn arbiters, role bootstrap", models.DiskEncryptionEnableOnArbiters, models.HostRoleBootstrap, false}, + {"enabledOn arbiters, role arbiter", models.DiskEncryptionEnableOnArbiters, models.HostRoleArbiter, true}, + {"enabledOn arbiters, role worker", models.DiskEncryptionEnableOnArbiters, models.HostRoleWorker, false}, + {"enabledOn workers, role master", models.DiskEncryptionEnableOnWorkers, models.HostRoleMaster, false}, + {"enabledOn workers, role bootstrap", models.DiskEncryptionEnableOnWorkers, models.HostRoleBootstrap, false}, + {"enabledOn workers, role arbiter", models.DiskEncryptionEnableOnWorkers, models.HostRoleArbiter, false}, + {"enabledOn workers, role worker", models.DiskEncryptionEnableOnWorkers, models.HostRoleWorker, true}, + {"enabledOn none, role master", models.DiskEncryptionEnableOnNone, models.HostRoleMaster, false}, + {"enabledOn none, role bootstrap", models.DiskEncryptionEnableOnNone, models.HostRoleBootstrap, false}, + {"enabledOn none, role arbiter", models.DiskEncryptionEnableOnNone, models.HostRoleArbiter, false}, + {"enabledOn none, role worker", models.DiskEncryptionEnableOnNone, models.HostRoleWorker, false}, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + diskEncryption := models.DiskEncryption{EnableOn: swag.String(tc.enabledOn)} + assert.Equal(t, tc.isEnabled, EnabledForRole(diskEncryption, tc.role)) + }) + } +} diff --git a/internal/hardware/validator.go b/internal/hardware/validator.go index 50ef36ffcd45..b389c95d6e84 100644 --- a/internal/hardware/validator.go +++ b/internal/hardware/validator.go @@ -14,6 +14,7 @@ import ( "github.com/go-openapi/swag" "github.com/openshift/assisted-service/internal/common" + "github.com/openshift/assisted-service/internal/diskencryption" "github.com/openshift/assisted-service/internal/feature" "github.com/openshift/assisted-service/internal/host/hostutil" "github.com/openshift/assisted-service/internal/network" @@ -398,17 +399,6 @@ func (v *validator) GetInfraEnvHostRequirements(ctx context.Context, infraEnv *c }, nil } -func isDiskEncryptionSetWithTpm(c *common.Cluster) bool { - if c.DiskEncryption == nil { - return false - } - enableOn := swag.StringValue(c.DiskEncryption.EnableOn) - if enableOn == "" || enableOn == models.DiskEncryptionEnableOnNone { - return false - } - return swag.StringValue(c.DiskEncryption.Mode) == models.DiskEncryptionModeTpmv2 -} - func (v *validator) GetPreflightHardwareRequirements(ctx context.Context, cluster *common.Cluster) (*models.PreflightHardwareRequirements, error) { operatorsRequirements, err := v.operatorsAPI.GetPreflightRequirementsBreakdownForCluster(ctx, cluster) if err != nil { @@ -418,7 +408,7 @@ func (v *validator) GetPreflightHardwareRequirements(ctx context.Context, cluste if err != nil { return nil, err } - if isDiskEncryptionSetWithTpm(cluster) { + if diskencryption.IsSetWithTpm(cluster.DiskEncryption) { valid := false isDiskEncryptionOnAll := swag.StringValue(cluster.DiskEncryption.EnableOn) == models.DiskEncryptionEnableOnAll enabledGroups := strings.Split(swag.StringValue(cluster.DiskEncryption.EnableOn), ",") diff --git a/internal/host/hostcommands/tang_connectivity_check_cmd.go b/internal/host/hostcommands/tang_connectivity_check_cmd.go index c3bca019a668..a4ce241415d0 100644 --- a/internal/host/hostcommands/tang_connectivity_check_cmd.go +++ b/internal/host/hostcommands/tang_connectivity_check_cmd.go @@ -7,6 +7,7 @@ import ( ignition_types "github.com/coreos/ignition/v2/config/v3_2/types" "github.com/go-openapi/swag" "github.com/openshift/assisted-service/internal/common" + "github.com/openshift/assisted-service/internal/diskencryption" "github.com/openshift/assisted-service/internal/host/hostutil" "github.com/openshift/assisted-service/models" "github.com/sirupsen/logrus" @@ -71,9 +72,9 @@ func (c *tangConnectivityCheckCmd) shouldRunTangConnectivityCheck(cluster common // 2. DiskEncryption mode is not tang based. // 3. DiskEncryption is not enabled, for the host role. if cluster.DiskEncryption == nil || - swag.StringValue(cluster.DiskEncryption.EnableOn) == models.DiskEncryptionEnableOnNone || + !diskencryption.IsEnabled(cluster.DiskEncryption.EnableOn) || swag.StringValue(cluster.DiskEncryption.Mode) == models.DiskEncryptionModeTpmv2 || - !hostutil.IsDiskEncryptionEnabledForRole(*cluster.DiskEncryption, common.GetEffectiveRole(host)) { + !diskencryption.EnabledForRole(*cluster.DiskEncryption, common.GetEffectiveRole(host)) { c.log.Debugf("skipping tangConnectivityCheck for host %s, cluster DiskEncryption config does not require validation here", host.ID.String()) return false diff --git a/internal/host/hostutil/host_utils.go b/internal/host/hostutil/host_utils.go index a6dda121b382..9db1fa396675 100644 --- a/internal/host/hostutil/host_utils.go +++ b/internal/host/hostutil/host_utils.go @@ -307,24 +307,6 @@ func SaveDiskPartitionsIsSet(installerArgs string) bool { return false } -func IsDiskEncryptionEnabledForRole(encryption models.DiskEncryption, role models.HostRole) bool { - if swag.StringValue(encryption.EnableOn) == models.DiskEncryptionEnableOnAll { - return true - } - - enabledGroups := strings.Split(swag.StringValue(encryption.EnableOn), ",") - if role == models.HostRoleMaster || role == models.HostRoleBootstrap { - return funk.ContainsString(enabledGroups, models.DiskEncryptionEnableOnMasters) - } - if role == models.HostRoleArbiter { - return funk.ContainsString(enabledGroups, models.DiskEncryptionEnableOnArbiters) - } - if role == models.HostRoleWorker { - return funk.ContainsString(enabledGroups, models.DiskEncryptionEnableOnWorkers) - } - return false -} - func GetDiskEncryptionForDay2(log logrus.FieldLogger, host *models.Host) (*ignition_types.Luks, error) { var response models.APIVipConnectivityResponse if err := json.Unmarshal([]byte(host.APIVipConnectivity), &response); err != nil { diff --git a/internal/host/hostutil/host_utils_test.go b/internal/host/hostutil/host_utils_test.go index ffab7c5214fc..0d6adfe6d10d 100644 --- a/internal/host/hostutil/host_utils_test.go +++ b/internal/host/hostutil/host_utils_test.go @@ -632,51 +632,6 @@ var _ = Describe("Get Disks of Holder", func() { }) }) -var _ = DescribeTable("IsDiskEncryptionEnabledForRole", func(enabledOn string, role models.HostRole, expectedResult bool) { - diskEncryption := models.DiskEncryption{ - EnableOn: &enabledOn, - } - isEnabled := IsDiskEncryptionEnabledForRole(diskEncryption, role) - Expect(isEnabled).To(Equal(expectedResult)) -}, - Entry("enabledOn all, role master", models.DiskEncryptionEnableOnAll, models.HostRoleMaster, true), - Entry("enabledOn all, role bootstrap", models.DiskEncryptionEnableOnAll, models.HostRoleBootstrap, true), - Entry("enabledOn all, role arbiter", models.DiskEncryptionEnableOnAll, models.HostRoleArbiter, true), - Entry("enabledOn all, role worker", models.DiskEncryptionEnableOnAll, models.HostRoleWorker, true), - Entry("enabledOn masters,arbiters,workers, role master", models.DiskEncryptionEnableOnMastersArbitersWorkers, models.HostRoleMaster, true), - Entry("enabledOn masters,arbiters,workers, role bootstrap", models.DiskEncryptionEnableOnMastersArbitersWorkers, models.HostRoleBootstrap, true), - Entry("enabledOn masters,arbiters,workers, role arbiter", models.DiskEncryptionEnableOnMastersArbitersWorkers, models.HostRoleArbiter, true), - Entry("enabledOn masters,arbiters,workers, role worker", models.DiskEncryptionEnableOnMastersArbitersWorkers, models.HostRoleWorker, true), - Entry("enabledOn masters,arbiters, role master", models.DiskEncryptionEnableOnMastersArbiters, models.HostRoleMaster, true), - Entry("enabledOn masters,arbiters, role bootstrap", models.DiskEncryptionEnableOnMastersArbiters, models.HostRoleBootstrap, true), - Entry("enabledOn masters,arbiters, role arbiter", models.DiskEncryptionEnableOnMastersArbiters, models.HostRoleArbiter, true), - Entry("enabledOn masters,arbiters, role worker", models.DiskEncryptionEnableOnMastersArbiters, models.HostRoleWorker, false), - Entry("enabledOn masters,workers, role master", models.DiskEncryptionEnableOnMastersWorkers, models.HostRoleMaster, true), - Entry("enabledOn masters,workers, role bootstrap", models.DiskEncryptionEnableOnMastersWorkers, models.HostRoleBootstrap, true), - Entry("enabledOn masters,workers, role arbiter", models.DiskEncryptionEnableOnMastersWorkers, models.HostRoleArbiter, false), - Entry("enabledOn masters,workers, role worker", models.DiskEncryptionEnableOnMastersWorkers, models.HostRoleWorker, true), - Entry("enabledOn arbiters,workers, role master", models.DiskEncryptionEnableOnArbitersWorkers, models.HostRoleMaster, false), - Entry("enabledOn arbiters,workers, role bootstrap", models.DiskEncryptionEnableOnArbitersWorkers, models.HostRoleBootstrap, false), - Entry("enabledOn arbiters,workers, role arbiter", models.DiskEncryptionEnableOnArbitersWorkers, models.HostRoleArbiter, true), - Entry("enabledOn arbiters,workers, role worker", models.DiskEncryptionEnableOnArbitersWorkers, models.HostRoleWorker, true), - Entry("enabledOn masters, role master", models.DiskEncryptionEnableOnMasters, models.HostRoleMaster, true), - Entry("enabledOn masters, role bootstrap", models.DiskEncryptionEnableOnMasters, models.HostRoleBootstrap, true), - Entry("enabledOn masters, role arbiter", models.DiskEncryptionEnableOnMasters, models.HostRoleArbiter, false), - Entry("enabledOn masters, role worker", models.DiskEncryptionEnableOnMasters, models.HostRoleWorker, false), - Entry("enabledOn arbiters, role master", models.DiskEncryptionEnableOnArbiters, models.HostRoleMaster, false), - Entry("enabledOn arbiters, role bootstrap", models.DiskEncryptionEnableOnArbiters, models.HostRoleBootstrap, false), - Entry("enabledOn arbiters, role arbiter", models.DiskEncryptionEnableOnArbiters, models.HostRoleArbiter, true), - Entry("enabledOn arbiters, role worker", models.DiskEncryptionEnableOnArbiters, models.HostRoleWorker, false), - Entry("enabledOn workers, role master", models.DiskEncryptionEnableOnWorkers, models.HostRoleMaster, false), - Entry("enabledOn workers, role bootstrap", models.DiskEncryptionEnableOnWorkers, models.HostRoleBootstrap, false), - Entry("enabledOn workers, role arbiter", models.DiskEncryptionEnableOnWorkers, models.HostRoleArbiter, false), - Entry("enabledOn workers, role worker", models.DiskEncryptionEnableOnWorkers, models.HostRoleWorker, true), - Entry("enabledOn none, role master", models.DiskEncryptionEnableOnNone, models.HostRoleMaster, false), - Entry("enabledOn none, role bootstrap", models.DiskEncryptionEnableOnNone, models.HostRoleBootstrap, false), - Entry("enabledOn none, role arbiter", models.DiskEncryptionEnableOnNone, models.HostRoleArbiter, false), - Entry("enabledOn none, role worker", models.DiskEncryptionEnableOnNone, models.HostRoleWorker, false), -) - var _ = Describe("GetHostInstallationDisk", func() { var ( hostId strfmt.UUID diff --git a/internal/host/validator.go b/internal/host/validator.go index 2de397e2ca10..19970e9f20b3 100644 --- a/internal/host/validator.go +++ b/internal/host/validator.go @@ -17,6 +17,7 @@ import ( "github.com/go-openapi/swag" "github.com/openshift/assisted-service/internal/common" "github.com/openshift/assisted-service/internal/constants" + "github.com/openshift/assisted-service/internal/diskencryption" "github.com/openshift/assisted-service/internal/hardware" "github.com/openshift/assisted-service/internal/host/hostutil" "github.com/openshift/assisted-service/internal/network" @@ -529,7 +530,7 @@ func (v *validator) diskEncryptionRequirementsSatisfied(c *validationContext) (V if role == models.HostRoleAutoAssign { return ValidationPending, "Missing role assignment" } - if !hostutil.IsDiskEncryptionEnabledForRole(*c.cluster.DiskEncryption, role) { + if !diskencryption.EnabledForRole(*c.cluster.DiskEncryption, role) { return ValidationSuccessSuppressOutput, "" } if swag.StringValue(c.cluster.DiskEncryption.Mode) == models.DiskEncryptionModeTang { diff --git a/internal/network/manifests_generator.go b/internal/network/manifests_generator.go index 293a5ee7cc81..f6961b189ef6 100644 --- a/internal/network/manifests_generator.go +++ b/internal/network/manifests_generator.go @@ -12,6 +12,7 @@ import ( "github.com/go-openapi/swag" "github.com/kelseyhightower/envconfig" "github.com/openshift/assisted-service/internal/common" + "github.com/openshift/assisted-service/internal/diskencryption" manifestsapi "github.com/openshift/assisted-service/internal/manifests/api" operatorcommon "github.com/openshift/assisted-service/internal/operators/common" "github.com/openshift/assisted-service/internal/operators/openshiftai" @@ -338,7 +339,7 @@ func (m *ManifestsGenerator) createDiskEncryptionManifest(ctx context.Context, l func (m *ManifestsGenerator) AddDiskEncryptionManifest(ctx context.Context, log logrus.FieldLogger, c *common.Cluster) error { - if swag.StringValue(c.DiskEncryption.EnableOn) == models.DiskEncryptionEnableOnNone { + if c.DiskEncryption == nil || !diskencryption.IsEnabled(c.DiskEncryption.EnableOn) { return nil } From 70fcfcd8cd2be187080c135dac701807f60ae544 Mon Sep 17 00:00:00 2001 From: aantal Date: Fri, 12 Jun 2026 16:05:43 +0200 Subject: [PATCH 4/8] fix diskencryption and hostutil unit test failures Use Ginkgo for diskencryption tests so CI ginkgo flags are accepted, and remove the unused ginkgo table import from hostutil tests. --- .../diskencryption/disk_encryption_test.go | 218 ++++++++++-------- internal/host/hostutil/host_utils_test.go | 1 - 2 files changed, 116 insertions(+), 103 deletions(-) diff --git a/internal/diskencryption/disk_encryption_test.go b/internal/diskencryption/disk_encryption_test.go index 2d7a497b5865..4f2b40fd198c 100644 --- a/internal/diskencryption/disk_encryption_test.go +++ b/internal/diskencryption/disk_encryption_test.go @@ -4,135 +4,149 @@ import ( "testing" "github.com/go-openapi/swag" + . "github.com/onsi/ginkgo" + . "github.com/onsi/ginkgo/extensions/table" + . "github.com/onsi/gomega" "github.com/openshift/assisted-service/models" - "github.com/stretchr/testify/assert" ) -func TestIsEnabled(t *testing.T) { - assert.False(t, IsEnabled(nil)) - assert.False(t, IsEnabled(swag.String(""))) - assert.False(t, IsEnabled(swag.String(models.DiskEncryptionEnableOnNone))) - assert.True(t, IsEnabled(swag.String(models.DiskEncryptionEnableOnMasters))) +func TestDiskEncryption(t *testing.T) { + RegisterFailHandler(Fail) + RunSpecs(t, "Disk encryption tests") } -func TestDiskEncryptionFieldDefaults(t *testing.T) { - enableOn, mode := DiskEncryptionFieldDefaults(nil, nil) - assert.Equal(t, models.DiskEncryptionEnableOnNone, enableOn) - assert.Equal(t, models.DiskEncryptionModeTpmv2, mode) +var _ = Describe("IsEnabled", func() { + It("returns false for nil, empty, and none", func() { + Expect(IsEnabled(nil)).To(BeFalse()) + Expect(IsEnabled(swag.String(""))).To(BeFalse()) + Expect(IsEnabled(swag.String(models.DiskEncryptionEnableOnNone))).To(BeFalse()) + }) - enableOn, mode = DiskEncryptionFieldDefaults(swag.String(""), swag.String("")) - assert.Equal(t, models.DiskEncryptionEnableOnNone, enableOn) - assert.Equal(t, models.DiskEncryptionModeTpmv2, mode) + It("returns true when encryption is enabled", func() { + Expect(IsEnabled(swag.String(models.DiskEncryptionEnableOnMasters))).To(BeTrue()) + }) +}) - enableOn, mode = DiskEncryptionFieldDefaults(swag.String(models.DiskEncryptionEnableOnMasters), swag.String(models.DiskEncryptionModeTang)) - assert.Equal(t, models.DiskEncryptionEnableOnMasters, enableOn) - assert.Equal(t, models.DiskEncryptionModeTang, mode) -} +var _ = Describe("DiskEncryptionFieldDefaults", func() { + It("defaults nil fields", func() { + enableOn, mode := DiskEncryptionFieldDefaults(nil, nil) + Expect(enableOn).To(Equal(models.DiskEncryptionEnableOnNone)) + Expect(mode).To(Equal(models.DiskEncryptionModeTpmv2)) + }) -func TestApplyDiskEncryptionDefaults(t *testing.T) { - t.Run("nil input", func(t *testing.T) { - assert.NotPanics(t, func() { - ApplyDiskEncryptionDefaults(nil) - }) + It("defaults empty strings", func() { + enableOn, mode := DiskEncryptionFieldDefaults(swag.String(""), swag.String("")) + Expect(enableOn).To(Equal(models.DiskEncryptionEnableOnNone)) + Expect(mode).To(Equal(models.DiskEncryptionModeTpmv2)) }) - t.Run("nil fields", func(t *testing.T) { + It("preserves explicit values", func() { + enableOn, mode := DiskEncryptionFieldDefaults( + swag.String(models.DiskEncryptionEnableOnMasters), + swag.String(models.DiskEncryptionModeTang), + ) + Expect(enableOn).To(Equal(models.DiskEncryptionEnableOnMasters)) + Expect(mode).To(Equal(models.DiskEncryptionModeTang)) + }) +}) + +var _ = Describe("ApplyDiskEncryptionDefaults", func() { + It("handles nil input", func() { + Expect(func() { ApplyDiskEncryptionDefaults(nil) }).NotTo(Panic()) + }) + + It("defaults nil fields", func() { diskEncryption := &models.DiskEncryption{} ApplyDiskEncryptionDefaults(diskEncryption) - assert.Equal(t, swag.String(models.DiskEncryptionEnableOnNone), diskEncryption.EnableOn) - assert.Equal(t, swag.String(models.DiskEncryptionModeTpmv2), diskEncryption.Mode) + Expect(diskEncryption.EnableOn).To(Equal(swag.String(models.DiskEncryptionEnableOnNone))) + Expect(diskEncryption.Mode).To(Equal(swag.String(models.DiskEncryptionModeTpmv2))) }) - t.Run("empty string fields", func(t *testing.T) { + It("defaults empty string fields", func() { diskEncryption := &models.DiskEncryption{ EnableOn: swag.String(""), Mode: swag.String(""), } ApplyDiskEncryptionDefaults(diskEncryption) - assert.Equal(t, swag.String(models.DiskEncryptionEnableOnNone), diskEncryption.EnableOn) - assert.Equal(t, swag.String(models.DiskEncryptionModeTpmv2), diskEncryption.Mode) + Expect(diskEncryption.EnableOn).To(Equal(swag.String(models.DiskEncryptionEnableOnNone))) + Expect(diskEncryption.Mode).To(Equal(swag.String(models.DiskEncryptionModeTpmv2))) }) - t.Run("explicit values", func(t *testing.T) { + It("preserves explicit values", func() { diskEncryption := &models.DiskEncryption{ EnableOn: swag.String(models.DiskEncryptionEnableOnMasters), Mode: swag.String(models.DiskEncryptionModeTang), } ApplyDiskEncryptionDefaults(diskEncryption) - assert.Equal(t, swag.String(models.DiskEncryptionEnableOnMasters), diskEncryption.EnableOn) - assert.Equal(t, swag.String(models.DiskEncryptionModeTang), diskEncryption.Mode) + Expect(diskEncryption.EnableOn).To(Equal(swag.String(models.DiskEncryptionEnableOnMasters))) + Expect(diskEncryption.Mode).To(Equal(swag.String(models.DiskEncryptionModeTang))) }) -} +}) -func TestIsSetWithTpm(t *testing.T) { - assert.False(t, IsSetWithTpm(nil)) - assert.False(t, IsSetWithTpm(&models.DiskEncryption{ - EnableOn: swag.String(""), - Mode: swag.String(models.DiskEncryptionModeTpmv2), - })) - assert.False(t, IsSetWithTpm(&models.DiskEncryption{ - EnableOn: swag.String(models.DiskEncryptionEnableOnNone), - Mode: swag.String(models.DiskEncryptionModeTpmv2), - })) - assert.False(t, IsSetWithTpm(&models.DiskEncryption{ - EnableOn: swag.String(models.DiskEncryptionEnableOnMasters), - Mode: swag.String(models.DiskEncryptionModeTang), - })) - assert.True(t, IsSetWithTpm(&models.DiskEncryption{ - EnableOn: swag.String(models.DiskEncryptionEnableOnMasters), - Mode: swag.String(models.DiskEncryptionModeTpmv2), - })) -} +var _ = Describe("IsSetWithTpm", func() { + It("returns false when TPM encryption is not configured", func() { + Expect(IsSetWithTpm(nil)).To(BeFalse()) + Expect(IsSetWithTpm(&models.DiskEncryption{ + EnableOn: swag.String(""), + Mode: swag.String(models.DiskEncryptionModeTpmv2), + })).To(BeFalse()) + Expect(IsSetWithTpm(&models.DiskEncryption{ + EnableOn: swag.String(models.DiskEncryptionEnableOnNone), + Mode: swag.String(models.DiskEncryptionModeTpmv2), + })).To(BeFalse()) + Expect(IsSetWithTpm(&models.DiskEncryption{ + EnableOn: swag.String(models.DiskEncryptionEnableOnMasters), + Mode: swag.String(models.DiskEncryptionModeTang), + })).To(BeFalse()) + }) -func TestEnabledForRole(t *testing.T) { - testCases := []struct { - name string - enabledOn string - role models.HostRole - isEnabled bool - }{ - {"enabledOn all, role master", models.DiskEncryptionEnableOnAll, models.HostRoleMaster, true}, - {"enabledOn all, role bootstrap", models.DiskEncryptionEnableOnAll, models.HostRoleBootstrap, true}, - {"enabledOn all, role arbiter", models.DiskEncryptionEnableOnAll, models.HostRoleArbiter, true}, - {"enabledOn all, role worker", models.DiskEncryptionEnableOnAll, models.HostRoleWorker, true}, - {"enabledOn masters,arbiters,workers, role master", models.DiskEncryptionEnableOnMastersArbitersWorkers, models.HostRoleMaster, true}, - {"enabledOn masters,arbiters,workers, role bootstrap", models.DiskEncryptionEnableOnMastersArbitersWorkers, models.HostRoleBootstrap, true}, - {"enabledOn masters,arbiters,workers, role arbiter", models.DiskEncryptionEnableOnMastersArbitersWorkers, models.HostRoleArbiter, true}, - {"enabledOn masters,arbiters,workers, role worker", models.DiskEncryptionEnableOnMastersArbitersWorkers, models.HostRoleWorker, true}, - {"enabledOn masters,arbiters, role master", models.DiskEncryptionEnableOnMastersArbiters, models.HostRoleMaster, true}, - {"enabledOn masters,arbiters, role bootstrap", models.DiskEncryptionEnableOnMastersArbiters, models.HostRoleBootstrap, true}, - {"enabledOn masters,arbiters, role arbiter", models.DiskEncryptionEnableOnMastersArbiters, models.HostRoleArbiter, true}, - {"enabledOn masters,arbiters, role worker", models.DiskEncryptionEnableOnMastersArbiters, models.HostRoleWorker, false}, - {"enabledOn masters,workers, role master", models.DiskEncryptionEnableOnMastersWorkers, models.HostRoleMaster, true}, - {"enabledOn masters,workers, role bootstrap", models.DiskEncryptionEnableOnMastersWorkers, models.HostRoleBootstrap, true}, - {"enabledOn masters,workers, role arbiter", models.DiskEncryptionEnableOnMastersWorkers, models.HostRoleArbiter, false}, - {"enabledOn masters,workers, role worker", models.DiskEncryptionEnableOnMastersWorkers, models.HostRoleWorker, true}, - {"enabledOn arbiters,workers, role master", models.DiskEncryptionEnableOnArbitersWorkers, models.HostRoleMaster, false}, - {"enabledOn arbiters,workers, role bootstrap", models.DiskEncryptionEnableOnArbitersWorkers, models.HostRoleBootstrap, false}, - {"enabledOn arbiters,workers, role arbiter", models.DiskEncryptionEnableOnArbitersWorkers, models.HostRoleArbiter, true}, - {"enabledOn arbiters,workers, role worker", models.DiskEncryptionEnableOnArbitersWorkers, models.HostRoleWorker, true}, - {"enabledOn masters, role master", models.DiskEncryptionEnableOnMasters, models.HostRoleMaster, true}, - {"enabledOn masters, role bootstrap", models.DiskEncryptionEnableOnMasters, models.HostRoleBootstrap, true}, - {"enabledOn masters, role arbiter", models.DiskEncryptionEnableOnMasters, models.HostRoleArbiter, false}, - {"enabledOn masters, role worker", models.DiskEncryptionEnableOnMasters, models.HostRoleWorker, false}, - {"enabledOn arbiters, role master", models.DiskEncryptionEnableOnArbiters, models.HostRoleMaster, false}, - {"enabledOn arbiters, role bootstrap", models.DiskEncryptionEnableOnArbiters, models.HostRoleBootstrap, false}, - {"enabledOn arbiters, role arbiter", models.DiskEncryptionEnableOnArbiters, models.HostRoleArbiter, true}, - {"enabledOn arbiters, role worker", models.DiskEncryptionEnableOnArbiters, models.HostRoleWorker, false}, - {"enabledOn workers, role master", models.DiskEncryptionEnableOnWorkers, models.HostRoleMaster, false}, - {"enabledOn workers, role bootstrap", models.DiskEncryptionEnableOnWorkers, models.HostRoleBootstrap, false}, - {"enabledOn workers, role arbiter", models.DiskEncryptionEnableOnWorkers, models.HostRoleArbiter, false}, - {"enabledOn workers, role worker", models.DiskEncryptionEnableOnWorkers, models.HostRoleWorker, true}, - {"enabledOn none, role master", models.DiskEncryptionEnableOnNone, models.HostRoleMaster, false}, - {"enabledOn none, role bootstrap", models.DiskEncryptionEnableOnNone, models.HostRoleBootstrap, false}, - {"enabledOn none, role arbiter", models.DiskEncryptionEnableOnNone, models.HostRoleArbiter, false}, - {"enabledOn none, role worker", models.DiskEncryptionEnableOnNone, models.HostRoleWorker, false}, - } + It("returns true when TPM encryption is configured", func() { + Expect(IsSetWithTpm(&models.DiskEncryption{ + EnableOn: swag.String(models.DiskEncryptionEnableOnMasters), + Mode: swag.String(models.DiskEncryptionModeTpmv2), + })).To(BeTrue()) + }) +}) - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - diskEncryption := models.DiskEncryption{EnableOn: swag.String(tc.enabledOn)} - assert.Equal(t, tc.isEnabled, EnabledForRole(diskEncryption, tc.role)) - }) - } -} +var _ = DescribeTable("EnabledForRole", + func(enabledOn string, role models.HostRole, expectedResult bool) { + diskEncryption := models.DiskEncryption{EnableOn: swag.String(enabledOn)} + Expect(EnabledForRole(diskEncryption, role)).To(Equal(expectedResult)) + }, + Entry("enabledOn all, role master", models.DiskEncryptionEnableOnAll, models.HostRoleMaster, true), + Entry("enabledOn all, role bootstrap", models.DiskEncryptionEnableOnAll, models.HostRoleBootstrap, true), + Entry("enabledOn all, role arbiter", models.DiskEncryptionEnableOnAll, models.HostRoleArbiter, true), + Entry("enabledOn all, role worker", models.DiskEncryptionEnableOnAll, models.HostRoleWorker, true), + Entry("enabledOn masters,arbiters,workers, role master", models.DiskEncryptionEnableOnMastersArbitersWorkers, models.HostRoleMaster, true), + Entry("enabledOn masters,arbiters,workers, role bootstrap", models.DiskEncryptionEnableOnMastersArbitersWorkers, models.HostRoleBootstrap, true), + Entry("enabledOn masters,arbiters,workers, role arbiter", models.DiskEncryptionEnableOnMastersArbitersWorkers, models.HostRoleArbiter, true), + Entry("enabledOn masters,arbiters,workers, role worker", models.DiskEncryptionEnableOnMastersArbitersWorkers, models.HostRoleWorker, true), + Entry("enabledOn masters,arbiters, role master", models.DiskEncryptionEnableOnMastersArbiters, models.HostRoleMaster, true), + Entry("enabledOn masters,arbiters, role bootstrap", models.DiskEncryptionEnableOnMastersArbiters, models.HostRoleBootstrap, true), + Entry("enabledOn masters,arbiters, role arbiter", models.DiskEncryptionEnableOnMastersArbiters, models.HostRoleArbiter, true), + Entry("enabledOn masters,arbiters, role worker", models.DiskEncryptionEnableOnMastersArbiters, models.HostRoleWorker, false), + Entry("enabledOn masters,workers, role master", models.DiskEncryptionEnableOnMastersWorkers, models.HostRoleMaster, true), + Entry("enabledOn masters,workers, role bootstrap", models.DiskEncryptionEnableOnMastersWorkers, models.HostRoleBootstrap, true), + Entry("enabledOn masters,workers, role arbiter", models.DiskEncryptionEnableOnMastersWorkers, models.HostRoleArbiter, false), + Entry("enabledOn masters,workers, role worker", models.DiskEncryptionEnableOnMastersWorkers, models.HostRoleWorker, true), + Entry("enabledOn arbiters,workers, role master", models.DiskEncryptionEnableOnArbitersWorkers, models.HostRoleMaster, false), + Entry("enabledOn arbiters,workers, role bootstrap", models.DiskEncryptionEnableOnArbitersWorkers, models.HostRoleBootstrap, false), + Entry("enabledOn arbiters,workers, role arbiter", models.DiskEncryptionEnableOnArbitersWorkers, models.HostRoleArbiter, true), + Entry("enabledOn arbiters,workers, role worker", models.DiskEncryptionEnableOnArbitersWorkers, models.HostRoleWorker, true), + Entry("enabledOn masters, role master", models.DiskEncryptionEnableOnMasters, models.HostRoleMaster, true), + Entry("enabledOn masters, role bootstrap", models.DiskEncryptionEnableOnMasters, models.HostRoleBootstrap, true), + Entry("enabledOn masters, role arbiter", models.DiskEncryptionEnableOnMasters, models.HostRoleArbiter, false), + Entry("enabledOn masters, role worker", models.DiskEncryptionEnableOnMasters, models.HostRoleWorker, false), + Entry("enabledOn arbiters, role master", models.DiskEncryptionEnableOnArbiters, models.HostRoleMaster, false), + Entry("enabledOn arbiters, role bootstrap", models.DiskEncryptionEnableOnArbiters, models.HostRoleBootstrap, false), + Entry("enabledOn arbiters, role arbiter", models.DiskEncryptionEnableOnArbiters, models.HostRoleArbiter, true), + Entry("enabledOn arbiters, role worker", models.DiskEncryptionEnableOnArbiters, models.HostRoleWorker, false), + Entry("enabledOn workers, role master", models.DiskEncryptionEnableOnWorkers, models.HostRoleMaster, false), + Entry("enabledOn workers, role bootstrap", models.DiskEncryptionEnableOnWorkers, models.HostRoleBootstrap, false), + Entry("enabledOn workers, role arbiter", models.DiskEncryptionEnableOnWorkers, models.HostRoleArbiter, false), + Entry("enabledOn workers, role worker", models.DiskEncryptionEnableOnWorkers, models.HostRoleWorker, true), + Entry("enabledOn none, role master", models.DiskEncryptionEnableOnNone, models.HostRoleMaster, false), + Entry("enabledOn none, role bootstrap", models.DiskEncryptionEnableOnNone, models.HostRoleBootstrap, false), + Entry("enabledOn none, role arbiter", models.DiskEncryptionEnableOnNone, models.HostRoleArbiter, false), + Entry("enabledOn none, role worker", models.DiskEncryptionEnableOnNone, models.HostRoleWorker, false), +) diff --git a/internal/host/hostutil/host_utils_test.go b/internal/host/hostutil/host_utils_test.go index 0d6adfe6d10d..c2a508778c7f 100644 --- a/internal/host/hostutil/host_utils_test.go +++ b/internal/host/hostutil/host_utils_test.go @@ -9,7 +9,6 @@ import ( "github.com/go-openapi/strfmt" "github.com/google/uuid" . "github.com/onsi/ginkgo" - . "github.com/onsi/ginkgo/extensions/table" . "github.com/onsi/gomega" "github.com/openshift/assisted-service/internal/common" "github.com/openshift/assisted-service/models" From 25822daa7a7c491c59066425cfc1b25ca27e2a53 Mon Sep 17 00:00:00 2001 From: aantal Date: Fri, 12 Jun 2026 16:32:02 +0200 Subject: [PATCH 5/8] replace inline disk encryption enabled checks with IsConfigured --- internal/bminventory/inventory.go | 4 ++-- internal/cluster/validations/validations.go | 3 ++- internal/diskencryption/disk_encryption.go | 5 +++++ internal/diskencryption/disk_encryption_test.go | 16 ++++++++++++++++ .../hostcommands/tang_connectivity_check_cmd.go | 3 +-- internal/host/validator.go | 2 +- internal/network/manifests_generator.go | 2 +- 7 files changed, 28 insertions(+), 7 deletions(-) diff --git a/internal/bminventory/inventory.go b/internal/bminventory/inventory.go index b596b6da7bf7..63ac469df9a2 100644 --- a/internal/bminventory/inventory.go +++ b/internal/bminventory/inventory.go @@ -2629,7 +2629,7 @@ func (b *bareMetalInventory) updateDhcpNetworkParams(db *gorm.DB, id *strfmt.UUI func (b *bareMetalInventory) setDiskEncryptionUsage(c *models.Cluster, diskEncryption *models.DiskEncryption, usages map[string]models.Usage) { - if c.DiskEncryption == nil || swag.StringValue(c.DiskEncryption.EnableOn) == models.DiskEncryptionEnableOnNone { + if !diskencryption.IsConfigured(c.DiskEncryption) { return } @@ -2641,7 +2641,7 @@ func (b *bareMetalInventory) setDiskEncryptionUsage(c *models.Cluster, diskEncry props["mode"] = swag.StringValue(diskEncryption.Mode) props["tang_servers"] = diskEncryption.TangServers } - b.setUsage(swag.StringValue(c.DiskEncryption.EnableOn) != models.DiskEncryptionEnableOnNone, usage.DiskEncryption, &props, usages) + b.setUsage(diskencryption.IsConfigured(c.DiskEncryption), usage.DiskEncryption, &props, usages) } func (b *bareMetalInventory) updateClusterData(_ context.Context, cluster *common.Cluster, params installer.V2UpdateClusterParams, usages map[string]models.Usage, db *gorm.DB, log logrus.FieldLogger, interactivity Interactivity, mirrorRegistryConfiguration *common.MirrorRegistryConfiguration, primaryIPStackUpdated bool, primaryIPStack *common.PrimaryIPStack) error { diff --git a/internal/cluster/validations/validations.go b/internal/cluster/validations/validations.go index 134c88b0fb71..b4174ebee14c 100644 --- a/internal/cluster/validations/validations.go +++ b/internal/cluster/validations/validations.go @@ -17,6 +17,7 @@ import ( "github.com/hashicorp/go-multierror" "github.com/openshift/assisted-image-service/pkg/isoeditor" "github.com/openshift/assisted-service/internal/common" + "github.com/openshift/assisted-service/internal/diskencryption" "github.com/openshift/assisted-service/internal/featuresupport" "github.com/openshift/assisted-service/internal/network" "github.com/openshift/assisted-service/models" @@ -776,7 +777,7 @@ func ValidateDiskEncryptionParams(diskEncryptionParams *models.DiskEncryption, D if diskEncryptionParams == nil { return nil } - if !DiskEncryptionSupport && swag.StringValue(diskEncryptionParams.EnableOn) != models.DiskEncryptionEnableOnNone { + if !DiskEncryptionSupport && diskencryption.IsEnabled(diskEncryptionParams.EnableOn) { return errors.New("Disk encryption support is not enabled. Cannot apply configurations to the cluster") } if diskEncryptionParams.Mode != nil && swag.StringValue(diskEncryptionParams.Mode) == models.DiskEncryptionModeTang { diff --git a/internal/diskencryption/disk_encryption.go b/internal/diskencryption/disk_encryption.go index 2832a7bd5594..c5254fe512e2 100644 --- a/internal/diskencryption/disk_encryption.go +++ b/internal/diskencryption/disk_encryption.go @@ -15,6 +15,11 @@ func IsEnabled(enableOn *string) bool { return v != "" && v != models.DiskEncryptionEnableOnNone } +// IsConfigured reports whether disk encryption is enabled on the cluster. +func IsConfigured(diskEncryption *models.DiskEncryption) bool { + return diskEncryption != nil && IsEnabled(diskEncryption.EnableOn) +} + // DiskEncryptionFieldDefaults returns enable_on and mode with defaults for nil or empty values. func DiskEncryptionFieldDefaults(enableOn, mode *string) (string, string) { enableOnValue := swag.StringValue(enableOn) diff --git a/internal/diskencryption/disk_encryption_test.go b/internal/diskencryption/disk_encryption_test.go index 4f2b40fd198c..b91de74ebd87 100644 --- a/internal/diskencryption/disk_encryption_test.go +++ b/internal/diskencryption/disk_encryption_test.go @@ -15,6 +15,22 @@ func TestDiskEncryption(t *testing.T) { RunSpecs(t, "Disk encryption tests") } +var _ = Describe("IsConfigured", func() { + It("returns false when disk encryption is not configured", func() { + Expect(IsConfigured(nil)).To(BeFalse()) + Expect(IsConfigured(&models.DiskEncryption{})).To(BeFalse()) + Expect(IsConfigured(&models.DiskEncryption{ + EnableOn: swag.String(models.DiskEncryptionEnableOnNone), + })).To(BeFalse()) + }) + + It("returns true when disk encryption is enabled", func() { + Expect(IsConfigured(&models.DiskEncryption{ + EnableOn: swag.String(models.DiskEncryptionEnableOnMasters), + })).To(BeTrue()) + }) +}) + var _ = Describe("IsEnabled", func() { It("returns false for nil, empty, and none", func() { Expect(IsEnabled(nil)).To(BeFalse()) diff --git a/internal/host/hostcommands/tang_connectivity_check_cmd.go b/internal/host/hostcommands/tang_connectivity_check_cmd.go index a4ce241415d0..2b436d737bf4 100644 --- a/internal/host/hostcommands/tang_connectivity_check_cmd.go +++ b/internal/host/hostcommands/tang_connectivity_check_cmd.go @@ -71,8 +71,7 @@ func (c *tangConnectivityCheckCmd) shouldRunTangConnectivityCheck(cluster common // 1. DiskEncryption not set or not enabled. // 2. DiskEncryption mode is not tang based. // 3. DiskEncryption is not enabled, for the host role. - if cluster.DiskEncryption == nil || - !diskencryption.IsEnabled(cluster.DiskEncryption.EnableOn) || + if !diskencryption.IsConfigured(cluster.DiskEncryption) || swag.StringValue(cluster.DiskEncryption.Mode) == models.DiskEncryptionModeTpmv2 || !diskencryption.EnabledForRole(*cluster.DiskEncryption, common.GetEffectiveRole(host)) { c.log.Debugf("skipping tangConnectivityCheck for host %s, cluster DiskEncryption config does not require validation here", diff --git a/internal/host/validator.go b/internal/host/validator.go index 19970e9f20b3..206ba1c55c7f 100644 --- a/internal/host/validator.go +++ b/internal/host/validator.go @@ -489,7 +489,7 @@ func (v *validator) diskEncryptionRequirementsSatisfied(c *validationContext) (V var status ValidationStatus var message string - if c.infraEnv != nil || swag.StringValue(c.cluster.DiskEncryption.EnableOn) == models.DiskEncryptionEnableOnNone { + if c.infraEnv != nil || !diskencryption.IsConfigured(c.cluster.DiskEncryption) { return ValidationSuccessSuppressOutput, "" } if c.inventory == nil { diff --git a/internal/network/manifests_generator.go b/internal/network/manifests_generator.go index f6961b189ef6..84d2597bb67d 100644 --- a/internal/network/manifests_generator.go +++ b/internal/network/manifests_generator.go @@ -339,7 +339,7 @@ func (m *ManifestsGenerator) createDiskEncryptionManifest(ctx context.Context, l func (m *ManifestsGenerator) AddDiskEncryptionManifest(ctx context.Context, log logrus.FieldLogger, c *common.Cluster) error { - if c.DiskEncryption == nil || !diskencryption.IsEnabled(c.DiskEncryption.EnableOn) { + if !diskencryption.IsConfigured(c.DiskEncryption) { return nil } From 20e1765b395d03366e5c02127a186bb13a97cc1e Mon Sep 17 00:00:00 2001 From: aantal Date: Mon, 15 Jun 2026 11:01:03 +0200 Subject: [PATCH 6/8] NO-ISSUE: reject tang disk encryption config when feature is disabled Use RequestsConfiguration at the API boundary so tang mode or tang_servers without enable_on cannot bypass the disk encryption feature gate on unsupported platforms. --- internal/cluster/validations/validations.go | 2 +- internal/diskencryption/disk_encryption.go | 11 ++++++++ .../diskencryption/disk_encryption_test.go | 27 +++++++++++++++++++ internal/host/validator.go | 8 +++++- 4 files changed, 46 insertions(+), 2 deletions(-) diff --git a/internal/cluster/validations/validations.go b/internal/cluster/validations/validations.go index b4174ebee14c..f3e60b9dbaec 100644 --- a/internal/cluster/validations/validations.go +++ b/internal/cluster/validations/validations.go @@ -777,7 +777,7 @@ func ValidateDiskEncryptionParams(diskEncryptionParams *models.DiskEncryption, D if diskEncryptionParams == nil { return nil } - if !DiskEncryptionSupport && diskencryption.IsEnabled(diskEncryptionParams.EnableOn) { + if !DiskEncryptionSupport && diskencryption.RequestsConfiguration(diskEncryptionParams) { return errors.New("Disk encryption support is not enabled. Cannot apply configurations to the cluster") } if diskEncryptionParams.Mode != nil && swag.StringValue(diskEncryptionParams.Mode) == models.DiskEncryptionModeTang { diff --git a/internal/diskencryption/disk_encryption.go b/internal/diskencryption/disk_encryption.go index c5254fe512e2..2f59c4344e8d 100644 --- a/internal/diskencryption/disk_encryption.go +++ b/internal/diskencryption/disk_encryption.go @@ -20,6 +20,17 @@ func IsConfigured(diskEncryption *models.DiskEncryption) bool { return diskEncryption != nil && IsEnabled(diskEncryption.EnableOn) } +// RequestsConfiguration reports whether an API payload carries explicit disk encryption +// settings beyond the disabled defaults, including tang configuration without enable_on. +func RequestsConfiguration(diskEncryption *models.DiskEncryption) bool { + if diskEncryption == nil { + return false + } + return IsEnabled(diskEncryption.EnableOn) || + swag.StringValue(diskEncryption.Mode) == models.DiskEncryptionModeTang || + diskEncryption.TangServers != "" +} + // DiskEncryptionFieldDefaults returns enable_on and mode with defaults for nil or empty values. func DiskEncryptionFieldDefaults(enableOn, mode *string) (string, string) { enableOnValue := swag.StringValue(enableOn) diff --git a/internal/diskencryption/disk_encryption_test.go b/internal/diskencryption/disk_encryption_test.go index b91de74ebd87..f25b7e61fd34 100644 --- a/internal/diskencryption/disk_encryption_test.go +++ b/internal/diskencryption/disk_encryption_test.go @@ -15,6 +15,33 @@ func TestDiskEncryption(t *testing.T) { RunSpecs(t, "Disk encryption tests") } +var _ = Describe("RequestsConfiguration", func() { + It("returns false for nil or disabled configuration", func() { + Expect(RequestsConfiguration(nil)).To(BeFalse()) + Expect(RequestsConfiguration(&models.DiskEncryption{})).To(BeFalse()) + Expect(RequestsConfiguration(&models.DiskEncryption{ + EnableOn: swag.String(models.DiskEncryptionEnableOnNone), + Mode: swag.String(models.DiskEncryptionModeTpmv2), + })).To(BeFalse()) + }) + + It("returns true when enable_on requests encryption", func() { + Expect(RequestsConfiguration(&models.DiskEncryption{ + EnableOn: swag.String(models.DiskEncryptionEnableOnMasters), + })).To(BeTrue()) + }) + + It("returns true when tang is configured without enable_on", func() { + Expect(RequestsConfiguration(&models.DiskEncryption{ + Mode: swag.String(models.DiskEncryptionModeTang), + TangServers: `[{"url":"http://tang.example.com:7500","thumbprint":"PLjNyRdGw03zlRoGjQYMahSZGu9"}]`, + })).To(BeTrue()) + Expect(RequestsConfiguration(&models.DiskEncryption{ + TangServers: `[{"url":"http://tang.example.com:7500","thumbprint":"PLjNyRdGw03zlRoGjQYMahSZGu9"}]`, + })).To(BeTrue()) + }) +}) + var _ = Describe("IsConfigured", func() { It("returns false when disk encryption is not configured", func() { Expect(IsConfigured(nil)).To(BeFalse()) diff --git a/internal/host/validator.go b/internal/host/validator.go index 206ba1c55c7f..f1fc99e991f4 100644 --- a/internal/host/validator.go +++ b/internal/host/validator.go @@ -489,7 +489,10 @@ func (v *validator) diskEncryptionRequirementsSatisfied(c *validationContext) (V var status ValidationStatus var message string - if c.infraEnv != nil || !diskencryption.IsConfigured(c.cluster.DiskEncryption) { + if c.infraEnv != nil { + return ValidationSuccessSuppressOutput, "" + } + if !hostutil.IsDay2Host(c.host) && !diskencryption.IsConfigured(c.cluster.DiskEncryption) { return ValidationSuccessSuppressOutput, "" } if c.inventory == nil { @@ -501,6 +504,9 @@ func (v *validator) diskEncryptionRequirementsSatisfied(c *validationContext) (V //according to that information luks, err := hostutil.GetDiskEncryptionForDay2(v.log, c.host) if err != nil { + if swag.StringValue(c.cluster.DiskEncryption.EnableOn) == models.DiskEncryptionEnableOnNone { + return ValidationSuccessSuppressOutput, "" + } return ValidationPending, "Missing ignition information" } if luks == nil || luks.Clevis == nil { From d0316be45d1087454772e38ed727538b32c907bc Mon Sep 17 00:00:00 2001 From: aantal Date: Tue, 16 Jun 2026 17:06:40 +0200 Subject: [PATCH 7/8] reverting the commit 'extract internal/diskencryption package' so common solution would be used in the whole project. a separate follow-up task will be added --- internal/bminventory/inventory.go | 11 +++++------ internal/cluster/validations/validations.go | 3 +-- .../{diskencryption => common}/disk_encryption.go | 2 +- .../disk_encryption_test.go | 9 +-------- .../controllers/clusterdeployments_controller.go | 7 +++---- internal/hardware/validator.go | 3 +-- .../host/hostcommands/tang_connectivity_check_cmd.go | 5 ++--- internal/host/validator.go | 5 ++--- internal/network/manifests_generator.go | 3 +-- subsystem/kubeapi/kubeapi_test.go | 6 ++++-- 10 files changed, 21 insertions(+), 33 deletions(-) rename internal/{diskencryption => common}/disk_encryption.go (99%) rename internal/{diskencryption => common}/disk_encryption_test.go (98%) diff --git a/internal/bminventory/inventory.go b/internal/bminventory/inventory.go index 63ac469df9a2..469c308fc913 100644 --- a/internal/bminventory/inventory.go +++ b/internal/bminventory/inventory.go @@ -27,7 +27,6 @@ import ( eventgen "github.com/openshift/assisted-service/internal/common/events" ignitioncommon "github.com/openshift/assisted-service/internal/common/ignition" "github.com/openshift/assisted-service/internal/constants" - "github.com/openshift/assisted-service/internal/diskencryption" "github.com/openshift/assisted-service/internal/dns" eventsapi "github.com/openshift/assisted-service/internal/events/api" "github.com/openshift/assisted-service/internal/featuresupport" @@ -920,7 +919,7 @@ func setDiskEncryptionWithDefaultValues(c *models.Cluster, config *models.DiskEn } c.DiskEncryption = config - diskencryption.ApplyDiskEncryptionDefaults(c.DiskEncryption) + common.ApplyDiskEncryptionDefaults(c.DiskEncryption) } func updateSSHPublicKey(cluster *common.Cluster) error { @@ -2629,7 +2628,7 @@ func (b *bareMetalInventory) updateDhcpNetworkParams(db *gorm.DB, id *strfmt.UUI func (b *bareMetalInventory) setDiskEncryptionUsage(c *models.Cluster, diskEncryption *models.DiskEncryption, usages map[string]models.Usage) { - if !diskencryption.IsConfigured(c.DiskEncryption) { + if !common.IsConfigured(c.DiskEncryption) { return } @@ -2641,7 +2640,7 @@ func (b *bareMetalInventory) setDiskEncryptionUsage(c *models.Cluster, diskEncry props["mode"] = swag.StringValue(diskEncryption.Mode) props["tang_servers"] = diskEncryption.TangServers } - b.setUsage(diskencryption.IsConfigured(c.DiskEncryption), usage.DiskEncryption, &props, usages) + b.setUsage(common.IsConfigured(c.DiskEncryption), usage.DiskEncryption, &props, usages) } func (b *bareMetalInventory) updateClusterData(_ context.Context, cluster *common.Cluster, params installer.V2UpdateClusterParams, usages map[string]models.Usage, db *gorm.DB, log logrus.FieldLogger, interactivity Interactivity, mirrorRegistryConfiguration *common.MirrorRegistryConfiguration, primaryIPStackUpdated bool, primaryIPStack *common.PrimaryIPStack) error { @@ -2713,12 +2712,12 @@ func (b *bareMetalInventory) updateClusterData(_ context.Context, cluster *commo return common.NewApiError(http.StatusBadRequest, errors.New(msg)) } if params.ClusterUpdateParams.DiskEncryption.EnableOn != nil { - enableOn, _ := diskencryption.DiskEncryptionFieldDefaults(params.ClusterUpdateParams.DiskEncryption.EnableOn, nil) + enableOn, _ := common.DiskEncryptionFieldDefaults(params.ClusterUpdateParams.DiskEncryption.EnableOn, nil) params.ClusterUpdateParams.DiskEncryption.EnableOn = swag.String(enableOn) updates["disk_encryption_enable_on"] = params.ClusterUpdateParams.DiskEncryption.EnableOn } if params.ClusterUpdateParams.DiskEncryption.Mode != nil { - _, mode := diskencryption.DiskEncryptionFieldDefaults(nil, params.ClusterUpdateParams.DiskEncryption.Mode) + _, mode := common.DiskEncryptionFieldDefaults(nil, params.ClusterUpdateParams.DiskEncryption.Mode) params.ClusterUpdateParams.DiskEncryption.Mode = swag.String(mode) updates["disk_encryption_mode"] = params.ClusterUpdateParams.DiskEncryption.Mode } diff --git a/internal/cluster/validations/validations.go b/internal/cluster/validations/validations.go index f3e60b9dbaec..c3807c3b1c28 100644 --- a/internal/cluster/validations/validations.go +++ b/internal/cluster/validations/validations.go @@ -17,7 +17,6 @@ import ( "github.com/hashicorp/go-multierror" "github.com/openshift/assisted-image-service/pkg/isoeditor" "github.com/openshift/assisted-service/internal/common" - "github.com/openshift/assisted-service/internal/diskencryption" "github.com/openshift/assisted-service/internal/featuresupport" "github.com/openshift/assisted-service/internal/network" "github.com/openshift/assisted-service/models" @@ -777,7 +776,7 @@ func ValidateDiskEncryptionParams(diskEncryptionParams *models.DiskEncryption, D if diskEncryptionParams == nil { return nil } - if !DiskEncryptionSupport && diskencryption.RequestsConfiguration(diskEncryptionParams) { + if !DiskEncryptionSupport && common.RequestsConfiguration(diskEncryptionParams) { return errors.New("Disk encryption support is not enabled. Cannot apply configurations to the cluster") } if diskEncryptionParams.Mode != nil && swag.StringValue(diskEncryptionParams.Mode) == models.DiskEncryptionModeTang { diff --git a/internal/diskencryption/disk_encryption.go b/internal/common/disk_encryption.go similarity index 99% rename from internal/diskencryption/disk_encryption.go rename to internal/common/disk_encryption.go index 2f59c4344e8d..887c8741e5a4 100644 --- a/internal/diskencryption/disk_encryption.go +++ b/internal/common/disk_encryption.go @@ -1,4 +1,4 @@ -package diskencryption +package common import ( "strings" diff --git a/internal/diskencryption/disk_encryption_test.go b/internal/common/disk_encryption_test.go similarity index 98% rename from internal/diskencryption/disk_encryption_test.go rename to internal/common/disk_encryption_test.go index f25b7e61fd34..75212f714352 100644 --- a/internal/diskencryption/disk_encryption_test.go +++ b/internal/common/disk_encryption_test.go @@ -1,8 +1,6 @@ -package diskencryption +package common import ( - "testing" - "github.com/go-openapi/swag" . "github.com/onsi/ginkgo" . "github.com/onsi/ginkgo/extensions/table" @@ -10,11 +8,6 @@ import ( "github.com/openshift/assisted-service/models" ) -func TestDiskEncryption(t *testing.T) { - RegisterFailHandler(Fail) - RunSpecs(t, "Disk encryption tests") -} - var _ = Describe("RequestsConfiguration", func() { It("returns false for nil or disabled configuration", func() { Expect(RequestsConfiguration(nil)).To(BeFalse()) diff --git a/internal/controller/controllers/clusterdeployments_controller.go b/internal/controller/controllers/clusterdeployments_controller.go index a05e85de83c5..5a841dd09e5f 100644 --- a/internal/controller/controllers/clusterdeployments_controller.go +++ b/internal/controller/controllers/clusterdeployments_controller.go @@ -42,7 +42,6 @@ import ( "github.com/openshift/assisted-service/internal/common" "github.com/openshift/assisted-service/internal/constants" "github.com/openshift/assisted-service/internal/controller/controllers/mirrorregistry" - "github.com/openshift/assisted-service/internal/diskencryption" "github.com/openshift/assisted-service/internal/gencrypto" "github.com/openshift/assisted-service/internal/host" manifestsapi "github.com/openshift/assisted-service/internal/manifests/api" @@ -1148,7 +1147,7 @@ func (r *ClusterDeploymentsReconciler) updateIfNeeded( if cluster.DiskEncryption == nil { // true when current cluster configuration does not include disk encryption cluster.DiskEncryption = &models.DiskEncryption{} } - enableOn, mode := diskencryption.DiskEncryptionFieldDefaults( + enableOn, mode := common.DiskEncryptionFieldDefaults( clusterInstall.Spec.DiskEncryption.EnableOn, clusterInstall.Spec.DiskEncryption.Mode, ) @@ -1526,8 +1525,8 @@ func CreateClusterParams(clusterDeployment *hivev1.ClusterDeployment, clusterIns clusterParams.Hyperthreading = getHyperthreading(clusterInstall) } - if clusterInstall.Spec.DiskEncryption != nil && diskencryption.IsEnabled(clusterInstall.Spec.DiskEncryption.EnableOn) { - enableOn, mode := diskencryption.DiskEncryptionFieldDefaults( + if clusterInstall.Spec.DiskEncryption != nil && common.IsEnabled(clusterInstall.Spec.DiskEncryption.EnableOn) { + enableOn, mode := common.DiskEncryptionFieldDefaults( clusterInstall.Spec.DiskEncryption.EnableOn, clusterInstall.Spec.DiskEncryption.Mode, ) diff --git a/internal/hardware/validator.go b/internal/hardware/validator.go index b389c95d6e84..414a88a92596 100644 --- a/internal/hardware/validator.go +++ b/internal/hardware/validator.go @@ -14,7 +14,6 @@ import ( "github.com/go-openapi/swag" "github.com/openshift/assisted-service/internal/common" - "github.com/openshift/assisted-service/internal/diskencryption" "github.com/openshift/assisted-service/internal/feature" "github.com/openshift/assisted-service/internal/host/hostutil" "github.com/openshift/assisted-service/internal/network" @@ -408,7 +407,7 @@ func (v *validator) GetPreflightHardwareRequirements(ctx context.Context, cluste if err != nil { return nil, err } - if diskencryption.IsSetWithTpm(cluster.DiskEncryption) { + if common.IsSetWithTpm(cluster.DiskEncryption) { valid := false isDiskEncryptionOnAll := swag.StringValue(cluster.DiskEncryption.EnableOn) == models.DiskEncryptionEnableOnAll enabledGroups := strings.Split(swag.StringValue(cluster.DiskEncryption.EnableOn), ",") diff --git a/internal/host/hostcommands/tang_connectivity_check_cmd.go b/internal/host/hostcommands/tang_connectivity_check_cmd.go index 2b436d737bf4..796d52116c3b 100644 --- a/internal/host/hostcommands/tang_connectivity_check_cmd.go +++ b/internal/host/hostcommands/tang_connectivity_check_cmd.go @@ -7,7 +7,6 @@ import ( ignition_types "github.com/coreos/ignition/v2/config/v3_2/types" "github.com/go-openapi/swag" "github.com/openshift/assisted-service/internal/common" - "github.com/openshift/assisted-service/internal/diskencryption" "github.com/openshift/assisted-service/internal/host/hostutil" "github.com/openshift/assisted-service/models" "github.com/sirupsen/logrus" @@ -71,9 +70,9 @@ func (c *tangConnectivityCheckCmd) shouldRunTangConnectivityCheck(cluster common // 1. DiskEncryption not set or not enabled. // 2. DiskEncryption mode is not tang based. // 3. DiskEncryption is not enabled, for the host role. - if !diskencryption.IsConfigured(cluster.DiskEncryption) || + if !common.IsConfigured(cluster.DiskEncryption) || swag.StringValue(cluster.DiskEncryption.Mode) == models.DiskEncryptionModeTpmv2 || - !diskencryption.EnabledForRole(*cluster.DiskEncryption, common.GetEffectiveRole(host)) { + !common.EnabledForRole(*cluster.DiskEncryption, common.GetEffectiveRole(host)) { c.log.Debugf("skipping tangConnectivityCheck for host %s, cluster DiskEncryption config does not require validation here", host.ID.String()) return false diff --git a/internal/host/validator.go b/internal/host/validator.go index f1fc99e991f4..fda73da66812 100644 --- a/internal/host/validator.go +++ b/internal/host/validator.go @@ -17,7 +17,6 @@ import ( "github.com/go-openapi/swag" "github.com/openshift/assisted-service/internal/common" "github.com/openshift/assisted-service/internal/constants" - "github.com/openshift/assisted-service/internal/diskencryption" "github.com/openshift/assisted-service/internal/hardware" "github.com/openshift/assisted-service/internal/host/hostutil" "github.com/openshift/assisted-service/internal/network" @@ -492,7 +491,7 @@ func (v *validator) diskEncryptionRequirementsSatisfied(c *validationContext) (V if c.infraEnv != nil { return ValidationSuccessSuppressOutput, "" } - if !hostutil.IsDay2Host(c.host) && !diskencryption.IsConfigured(c.cluster.DiskEncryption) { + if !hostutil.IsDay2Host(c.host) && !common.IsConfigured(c.cluster.DiskEncryption) { return ValidationSuccessSuppressOutput, "" } if c.inventory == nil { @@ -536,7 +535,7 @@ func (v *validator) diskEncryptionRequirementsSatisfied(c *validationContext) (V if role == models.HostRoleAutoAssign { return ValidationPending, "Missing role assignment" } - if !diskencryption.EnabledForRole(*c.cluster.DiskEncryption, role) { + if !common.EnabledForRole(*c.cluster.DiskEncryption, role) { return ValidationSuccessSuppressOutput, "" } if swag.StringValue(c.cluster.DiskEncryption.Mode) == models.DiskEncryptionModeTang { diff --git a/internal/network/manifests_generator.go b/internal/network/manifests_generator.go index 84d2597bb67d..3d4917565f1f 100644 --- a/internal/network/manifests_generator.go +++ b/internal/network/manifests_generator.go @@ -12,7 +12,6 @@ import ( "github.com/go-openapi/swag" "github.com/kelseyhightower/envconfig" "github.com/openshift/assisted-service/internal/common" - "github.com/openshift/assisted-service/internal/diskencryption" manifestsapi "github.com/openshift/assisted-service/internal/manifests/api" operatorcommon "github.com/openshift/assisted-service/internal/operators/common" "github.com/openshift/assisted-service/internal/operators/openshiftai" @@ -339,7 +338,7 @@ func (m *ManifestsGenerator) createDiskEncryptionManifest(ctx context.Context, l func (m *ManifestsGenerator) AddDiskEncryptionManifest(ctx context.Context, log logrus.FieldLogger, c *common.Cluster) error { - if !diskencryption.IsConfigured(c.DiskEncryption) { + if !common.IsConfigured(c.DiskEncryption) { return nil } diff --git a/subsystem/kubeapi/kubeapi_test.go b/subsystem/kubeapi/kubeapi_test.go index bf810fb0bd93..0338d172ad22 100644 --- a/subsystem/kubeapi/kubeapi_test.go +++ b/subsystem/kubeapi/kubeapi_test.go @@ -4724,8 +4724,10 @@ location = "%s" }, "30s", "10s").Should(Equal(firstAgentEventsURL)) By("Check host is removed from first backend cluster") - cluster := getClusterFromDB(ctx, kubeClient, db, clusterKey, waitForReconcileTimeout) - Expect(len(cluster.Hosts)).Should(Equal(0)) + Eventually(func() int { + cluster := getClusterFromDB(ctx, kubeClient, db, clusterKey, waitForReconcileTimeout) + return len(cluster.Hosts) + }, "30s", "10s").Should(Equal(0)) By("Delete Original Clusterdeployment") clusterDeploymentCRD := getClusterDeploymentCRD(ctx, kubeClient, clusterKey) From 93dc05c663af9eb3e44c9578b1e178453264b766 Mon Sep 17 00:00:00 2001 From: aantal Date: Wed, 17 Jun 2026 15:49:49 +0200 Subject: [PATCH 8/8] add HasMode and IsSetWithTang disk encryption helpers Introduce HasMode for raw mode checks and IsSetWithTang as the Tang counterpart to IsSetWithTpm, then replace scattered inline enable_on and mode comparisons with the shared helpers in validators, manifest generation, API validation, and tang connectivity checks. This keeps runtime paths on IsSetWithTpm/IsSetWithTang, payload validation on HasMode where enable_on may still be unset, and role targeting on EnabledForRole everywhere. --- internal/bminventory/inventory.go | 6 +-- internal/cluster/validations/validations.go | 2 +- internal/common/disk_encryption.go | 36 +++++++++---- internal/common/disk_encryption_test.go | 52 +++++++++++++++++++ .../clusterdeployments_controller.go | 7 ++- internal/hardware/validator.go | 23 +++----- .../tang_connectivity_check_cmd.go | 9 +--- internal/host/validations_test.go | 21 ++++++-- internal/host/validator.go | 6 +-- internal/network/manifests_generator.go | 19 +++---- subsystem/cluster_test.go | 4 +- 11 files changed, 127 insertions(+), 58 deletions(-) diff --git a/internal/bminventory/inventory.go b/internal/bminventory/inventory.go index 469c308fc913..6732fff89f44 100644 --- a/internal/bminventory/inventory.go +++ b/internal/bminventory/inventory.go @@ -443,11 +443,9 @@ func (b *bareMetalInventory) setDefaultRegisterClusterParams(ctx context.Context params.NewClusterParams.AdditionalNtpSource = &b.Config.DefaultNTPSource } if params.NewClusterParams.DiskEncryption == nil { - params.NewClusterParams.DiskEncryption = &models.DiskEncryption{ - EnableOn: swag.String(models.DiskEncryptionEnableOnNone), - Mode: swag.String(models.DiskEncryptionModeTpmv2), - } + params.NewClusterParams.DiskEncryption = &models.DiskEncryption{} } + common.ApplyDiskEncryptionDefaults(params.NewClusterParams.DiskEncryption) params.NewClusterParams.NetworkType, err = getDefaultNetworkType(params) if err != nil { diff --git a/internal/cluster/validations/validations.go b/internal/cluster/validations/validations.go index c3807c3b1c28..36e134e1edda 100644 --- a/internal/cluster/validations/validations.go +++ b/internal/cluster/validations/validations.go @@ -779,7 +779,7 @@ func ValidateDiskEncryptionParams(diskEncryptionParams *models.DiskEncryption, D if !DiskEncryptionSupport && common.RequestsConfiguration(diskEncryptionParams) { return errors.New("Disk encryption support is not enabled. Cannot apply configurations to the cluster") } - if diskEncryptionParams.Mode != nil && swag.StringValue(diskEncryptionParams.Mode) == models.DiskEncryptionModeTang { + if common.HasMode(diskEncryptionParams, models.DiskEncryptionModeTang) { if diskEncryptionParams.TangServers == "" { return errors.New("Setting Tang mode but tang_servers isn't set") } diff --git a/internal/common/disk_encryption.go b/internal/common/disk_encryption.go index 887c8741e5a4..c2d34d7dc120 100644 --- a/internal/common/disk_encryption.go +++ b/internal/common/disk_encryption.go @@ -26,9 +26,20 @@ func RequestsConfiguration(diskEncryption *models.DiskEncryption) bool { if diskEncryption == nil { return false } - return IsEnabled(diskEncryption.EnableOn) || - swag.StringValue(diskEncryption.Mode) == models.DiskEncryptionModeTang || - diskEncryption.TangServers != "" + return RequestsDiskEncryptionConfiguration( + diskEncryption.EnableOn, + diskEncryption.Mode, + diskEncryption.TangServers, + ) +} + +// RequestsDiskEncryptionConfiguration reports whether disk encryption fields carry explicit +// configuration beyond disabled defaults. Use this when the caller has separate fields +// instead of a models.DiskEncryption payload (for example AgentClusterInstall spec). +func RequestsDiskEncryptionConfiguration(enableOn, mode *string, tangServers string) bool { + return IsEnabled(enableOn) || + HasMode(&models.DiskEncryption{Mode: mode}, models.DiskEncryptionModeTang) || + tangServers != "" } // DiskEncryptionFieldDefaults returns enable_on and mode with defaults for nil or empty values. @@ -54,15 +65,22 @@ func ApplyDiskEncryptionDefaults(diskEncryption *models.DiskEncryption) { diskEncryption.Mode = swag.String(mode) } -// IsSetWithTpm reports whether TPM-based disk encryption is configured for any role. -func IsSetWithTpm(diskEncryption *models.DiskEncryption) bool { +// HasMode reports whether disk encryption mode equals the given value. +func HasMode(diskEncryption *models.DiskEncryption, mode string) bool { if diskEncryption == nil { return false } - if !IsEnabled(diskEncryption.EnableOn) { - return false - } - return swag.StringValue(diskEncryption.Mode) == models.DiskEncryptionModeTpmv2 + return swag.StringValue(diskEncryption.Mode) == mode +} + +// IsSetWithTpm reports whether TPM-based disk encryption is configured for any role. +func IsSetWithTpm(diskEncryption *models.DiskEncryption) bool { + return IsConfigured(diskEncryption) && HasMode(diskEncryption, models.DiskEncryptionModeTpmv2) +} + +// IsSetWithTang reports whether Tang-based disk encryption is configured for any role. +func IsSetWithTang(diskEncryption *models.DiskEncryption) bool { + return IsConfigured(diskEncryption) && HasMode(diskEncryption, models.DiskEncryptionModeTang) } // EnabledForRole reports whether disk encryption is enabled for the given host role. diff --git a/internal/common/disk_encryption_test.go b/internal/common/disk_encryption_test.go index 75212f714352..bd40bf8bc009 100644 --- a/internal/common/disk_encryption_test.go +++ b/internal/common/disk_encryption_test.go @@ -32,6 +32,11 @@ var _ = Describe("RequestsConfiguration", func() { Expect(RequestsConfiguration(&models.DiskEncryption{ TangServers: `[{"url":"http://tang.example.com:7500","thumbprint":"PLjNyRdGw03zlRoGjQYMahSZGu9"}]`, })).To(BeTrue()) + Expect(RequestsDiskEncryptionConfiguration( + nil, + swag.String(models.DiskEncryptionModeTang), + `[{"url":"http://tang.example.com:7500","thumbprint":"PLjNyRdGw03zlRoGjQYMahSZGu9"}]`, + )).To(BeTrue()) }) }) @@ -119,6 +124,25 @@ var _ = Describe("ApplyDiskEncryptionDefaults", func() { }) }) +var _ = Describe("HasMode", func() { + It("returns false for nil or non-matching mode", func() { + Expect(HasMode(nil, models.DiskEncryptionModeTang)).To(BeFalse()) + Expect(HasMode(&models.DiskEncryption{}, models.DiskEncryptionModeTang)).To(BeFalse()) + Expect(HasMode(&models.DiskEncryption{ + Mode: swag.String(models.DiskEncryptionModeTpmv2), + }, models.DiskEncryptionModeTang)).To(BeFalse()) + }) + + It("returns true when mode matches", func() { + Expect(HasMode(&models.DiskEncryption{ + Mode: swag.String(models.DiskEncryptionModeTang), + }, models.DiskEncryptionModeTang)).To(BeTrue()) + Expect(HasMode(&models.DiskEncryption{ + Mode: swag.String(models.DiskEncryptionModeTpmv2), + }, models.DiskEncryptionModeTpmv2)).To(BeTrue()) + }) +}) + var _ = Describe("IsSetWithTpm", func() { It("returns false when TPM encryption is not configured", func() { Expect(IsSetWithTpm(nil)).To(BeFalse()) @@ -144,6 +168,34 @@ var _ = Describe("IsSetWithTpm", func() { }) }) +var _ = Describe("IsSetWithTang", func() { + It("returns false when Tang encryption is not configured", func() { + Expect(IsSetWithTang(nil)).To(BeFalse()) + Expect(IsSetWithTang(&models.DiskEncryption{ + EnableOn: swag.String(""), + Mode: swag.String(models.DiskEncryptionModeTang), + })).To(BeFalse()) + Expect(IsSetWithTang(&models.DiskEncryption{ + EnableOn: swag.String(models.DiskEncryptionEnableOnNone), + Mode: swag.String(models.DiskEncryptionModeTang), + })).To(BeFalse()) + Expect(IsSetWithTang(&models.DiskEncryption{ + Mode: swag.String(models.DiskEncryptionModeTang), + })).To(BeFalse()) + Expect(IsSetWithTang(&models.DiskEncryption{ + EnableOn: swag.String(models.DiskEncryptionEnableOnMasters), + Mode: swag.String(models.DiskEncryptionModeTpmv2), + })).To(BeFalse()) + }) + + It("returns true when Tang encryption is configured", func() { + Expect(IsSetWithTang(&models.DiskEncryption{ + EnableOn: swag.String(models.DiskEncryptionEnableOnMasters), + Mode: swag.String(models.DiskEncryptionModeTang), + })).To(BeTrue()) + }) +}) + var _ = DescribeTable("EnabledForRole", func(enabledOn string, role models.HostRole, expectedResult bool) { diskEncryption := models.DiskEncryption{EnableOn: swag.String(enabledOn)} diff --git a/internal/controller/controllers/clusterdeployments_controller.go b/internal/controller/controllers/clusterdeployments_controller.go index 5a841dd09e5f..12638e1816a0 100644 --- a/internal/controller/controllers/clusterdeployments_controller.go +++ b/internal/controller/controllers/clusterdeployments_controller.go @@ -1525,7 +1525,12 @@ func CreateClusterParams(clusterDeployment *hivev1.ClusterDeployment, clusterIns clusterParams.Hyperthreading = getHyperthreading(clusterInstall) } - if clusterInstall.Spec.DiskEncryption != nil && common.IsEnabled(clusterInstall.Spec.DiskEncryption.EnableOn) { + if clusterInstall.Spec.DiskEncryption != nil && + common.RequestsDiskEncryptionConfiguration( + clusterInstall.Spec.DiskEncryption.EnableOn, + clusterInstall.Spec.DiskEncryption.Mode, + clusterInstall.Spec.DiskEncryption.TangServers, + ) { enableOn, mode := common.DiskEncryptionFieldDefaults( clusterInstall.Spec.DiskEncryption.EnableOn, clusterInstall.Spec.DiskEncryption.Mode, diff --git a/internal/hardware/validator.go b/internal/hardware/validator.go index 414a88a92596..d7776da49c4a 100644 --- a/internal/hardware/validator.go +++ b/internal/hardware/validator.go @@ -23,7 +23,6 @@ import ( "github.com/openshift/assisted-service/pkg/conversions" "github.com/samber/lo" "github.com/sirupsen/logrus" - "github.com/thoas/go-funk" "k8s.io/utils/ptr" ) @@ -408,25 +407,19 @@ func (v *validator) GetPreflightHardwareRequirements(ctx context.Context, cluste return nil, err } if common.IsSetWithTpm(cluster.DiskEncryption) { - valid := false - isDiskEncryptionOnAll := swag.StringValue(cluster.DiskEncryption.EnableOn) == models.DiskEncryptionEnableOnAll - enabledGroups := strings.Split(swag.StringValue(cluster.DiskEncryption.EnableOn), ",") + diskEncryption := *cluster.DiskEncryption + if !common.EnabledForRole(diskEncryption, models.HostRoleMaster) && + !common.EnabledForRole(diskEncryption, models.HostRoleArbiter) && + !common.EnabledForRole(diskEncryption, models.HostRoleWorker) { + return nil, fmt.Errorf("disk-encryption is enabled on non-valid role: %s", swag.StringValue(cluster.DiskEncryption.EnableOn)) + } - if isDiskEncryptionOnAll || funk.ContainsString(enabledGroups, models.DiskEncryptionEnableOnMasters) { - valid = true + if common.EnabledForRole(diskEncryption, models.HostRoleMaster) { ocpRequirements.Master.Quantitative.TpmEnabledInBios = true } - if isDiskEncryptionOnAll || funk.ContainsString(enabledGroups, models.DiskEncryptionEnableOnArbiters) { - valid = true - } - if isDiskEncryptionOnAll || funk.ContainsString(enabledGroups, models.DiskEncryptionEnableOnWorkers) { - valid = true + if common.EnabledForRole(diskEncryption, models.HostRoleWorker) { ocpRequirements.Worker.Quantitative.TpmEnabledInBios = true } - - if !valid { - return nil, fmt.Errorf("disk-encryption is enabled on non-valid role: %s", swag.StringValue(cluster.DiskEncryption.EnableOn)) - } } return &models.PreflightHardwareRequirements{ diff --git a/internal/host/hostcommands/tang_connectivity_check_cmd.go b/internal/host/hostcommands/tang_connectivity_check_cmd.go index 796d52116c3b..e74bd3c5f7db 100644 --- a/internal/host/hostcommands/tang_connectivity_check_cmd.go +++ b/internal/host/hostcommands/tang_connectivity_check_cmd.go @@ -5,7 +5,6 @@ import ( "encoding/json" ignition_types "github.com/coreos/ignition/v2/config/v3_2/types" - "github.com/go-openapi/swag" "github.com/openshift/assisted-service/internal/common" "github.com/openshift/assisted-service/internal/host/hostutil" "github.com/openshift/assisted-service/models" @@ -66,12 +65,8 @@ func (c *tangConnectivityCheckCmd) getTangServersFromHostIgnition(host *models.H } func (c *tangConnectivityCheckCmd) shouldRunTangConnectivityCheck(cluster common.Cluster, host *models.Host) bool { - // Skip tangConnectivityCheck for cases where: - // 1. DiskEncryption not set or not enabled. - // 2. DiskEncryption mode is not tang based. - // 3. DiskEncryption is not enabled, for the host role. - if !common.IsConfigured(cluster.DiskEncryption) || - swag.StringValue(cluster.DiskEncryption.Mode) == models.DiskEncryptionModeTpmv2 || + // Skip tangConnectivityCheck when Tang-based disk encryption is not required for this host. + if !common.IsSetWithTang(cluster.DiskEncryption) || !common.EnabledForRole(*cluster.DiskEncryption, common.GetEffectiveRole(host)) { c.log.Debugf("skipping tangConnectivityCheck for host %s, cluster DiskEncryption config does not require validation here", host.ID.String()) diff --git a/internal/host/validations_test.go b/internal/host/validations_test.go index 2b243f967a56..dd2e35ccca92 100644 --- a/internal/host/validations_test.go +++ b/internal/host/validations_test.go @@ -1425,7 +1425,12 @@ var _ = Describe("Validations test", func() { }) It("day2 host - disk encryption is available", func() { - createDay2Cluster() + c := generateDay2Cluster() + c.DiskEncryption = &models.DiskEncryption{ + EnableOn: swag.String(models.DiskEncryptionEnableOnMasters), + Mode: swag.String(models.DiskEncryptionModeTpmv2), + } + Expect(db.Create(c).Error).ToNot(HaveOccurred()) h := getDay2Host() //explicit set the role to worker h.Inventory = common.GenerateTestInventoryWithTpmVersion("") @@ -1442,7 +1447,12 @@ var _ = Describe("Validations test", func() { }) It("day2 host - pending on APIVipConnectivity response", func() { - createDay2Cluster() + c := generateDay2Cluster() + c.DiskEncryption = &models.DiskEncryption{ + EnableOn: swag.String(models.DiskEncryptionEnableOnMasters), + Mode: swag.String(models.DiskEncryptionModeTpmv2), + } + Expect(db.Create(c).Error).ToNot(HaveOccurred()) h := getDay2Host() h.Inventory = common.GenerateTestInventoryWithTpmVersion(models.InventoryTpmVersionNone) @@ -1458,7 +1468,12 @@ var _ = Describe("Validations test", func() { }) It("day2 host - LUKS in APIVipConnectivity response", func() { - createDay2Cluster() + c := generateDay2Cluster() + c.DiskEncryption = &models.DiskEncryption{ + EnableOn: swag.String(models.DiskEncryptionEnableOnMasters), + Mode: swag.String(models.DiskEncryptionModeTpmv2), + } + Expect(db.Create(c).Error).ToNot(HaveOccurred()) h := getDay2Host() h.Inventory = common.GenerateTestInventoryWithTpmVersion(models.InventoryTpmVersionNone) diff --git a/internal/host/validator.go b/internal/host/validator.go index fda73da66812..2df1fa5ee4af 100644 --- a/internal/host/validator.go +++ b/internal/host/validator.go @@ -503,7 +503,7 @@ func (v *validator) diskEncryptionRequirementsSatisfied(c *validationContext) (V //according to that information luks, err := hostutil.GetDiskEncryptionForDay2(v.log, c.host) if err != nil { - if swag.StringValue(c.cluster.DiskEncryption.EnableOn) == models.DiskEncryptionEnableOnNone { + if !common.IsConfigured(c.cluster.DiskEncryption) { return ValidationSuccessSuppressOutput, "" } return ValidationPending, "Missing ignition information" @@ -538,12 +538,12 @@ func (v *validator) diskEncryptionRequirementsSatisfied(c *validationContext) (V if !common.EnabledForRole(*c.cluster.DiskEncryption, role) { return ValidationSuccessSuppressOutput, "" } - if swag.StringValue(c.cluster.DiskEncryption.Mode) == models.DiskEncryptionModeTang { + if common.IsSetWithTang(c.cluster.DiskEncryption) { status, message = v.areTangServersReachable(c) if status == ValidationFailure { return status, message } - } else { // Mode TPMv2 + } else if common.IsSetWithTpm(c.cluster.DiskEncryption) { status = boolValue(c.inventory.TpmVersion == models.InventoryTpmVersionNr20) } diff --git a/internal/network/manifests_generator.go b/internal/network/manifests_generator.go index 3d4917565f1f..8c1e75a03fca 100644 --- a/internal/network/manifests_generator.go +++ b/internal/network/manifests_generator.go @@ -22,7 +22,6 @@ import ( "github.com/pkg/errors" "github.com/samber/lo" "github.com/sirupsen/logrus" - "github.com/thoas/go-funk" "gorm.io/gorm" ) @@ -346,14 +345,9 @@ func (m *ManifestsGenerator) AddDiskEncryptionManifest(ctx context.Context, log "CIPHER": m.GetDiskEncryptionCipher(log), } - switch *c.DiskEncryption.Mode { - - case models.DiskEncryptionModeTpmv2: - + if common.IsSetWithTpm(c.DiskEncryption) { manifestParams["MODE"] = "tpm" - - case models.DiskEncryptionModeTang: - + } else if common.IsSetWithTang(c.DiskEncryption) { tangServers, err := tang.UnmarshalTangServers(c.DiskEncryption.TangServers) if err != nil { log.WithError(err).Error("failed to unmarshal tang_server from cluster object") @@ -364,17 +358,16 @@ func (m *ManifestsGenerator) AddDiskEncryptionManifest(ctx context.Context, log manifestParams["TANG_SERVERS"] = tangServers } - enabledGroups := strings.Split(swag.StringValue(c.DiskEncryption.EnableOn), ",") - isDiskEncryptionOnAll := swag.StringValue(c.DiskEncryption.EnableOn) == models.DiskEncryptionEnableOnAll + diskEncryption := *c.DiskEncryption - if isDiskEncryptionOnAll || funk.ContainsString(enabledGroups, models.DiskEncryptionEnableOnMasters) { + if common.EnabledForRole(diskEncryption, models.HostRoleMaster) { manifestParams["ROLE"] = "master" if err := m.createDiskEncryptionManifest(ctx, log, c, manifestParams); err != nil { return err } } - if (isDiskEncryptionOnAll || funk.ContainsString(enabledGroups, models.DiskEncryptionEnableOnArbiters)) && + if common.EnabledForRole(diskEncryption, models.HostRoleArbiter) && common.IsClusterTopologyHighlyAvailableArbiter(c) { manifestParams["ROLE"] = "arbiter" if err := m.createDiskEncryptionManifest(ctx, log, c, manifestParams); err != nil { @@ -382,7 +375,7 @@ func (m *ManifestsGenerator) AddDiskEncryptionManifest(ctx context.Context, log } } - if isDiskEncryptionOnAll || funk.ContainsString(enabledGroups, models.DiskEncryptionEnableOnWorkers) { + if common.EnabledForRole(diskEncryption, models.HostRoleWorker) { manifestParams["ROLE"] = "worker" if err := m.createDiskEncryptionManifest(ctx, log, c, manifestParams); err != nil { return err diff --git a/subsystem/cluster_test.go b/subsystem/cluster_test.go index 0c26b1ba5b45..b72c9cbe0bef 100644 --- a/subsystem/cluster_test.go +++ b/subsystem/cluster_test.go @@ -4406,7 +4406,7 @@ func registerHostsAndSetRoles(clusterID, infraenvID strfmt.UUID, numHosts int, c } generateFullMeshConnectivity(ctx, ips[0], hosts...) cluster := utils_test.TestContext.GetCluster(clusterID) - if cluster.DiskEncryption != nil && swag.StringValue(cluster.DiskEncryption.Mode) == models.DiskEncryptionModeTang { + if common.IsSetWithTang(cluster.DiskEncryption) { utils_test.TestContext.GenerateTangPostStepReply(ctx, true, hosts...) } @@ -4468,7 +4468,7 @@ func registerHostsAndSetRolesTang(clusterID, infraenvID strfmt.UUID, numHosts in } generateFullMeshConnectivity(ctx, ips[0], hosts...) cluster := utils_test.TestContext.GetCluster(clusterID) - if cluster.DiskEncryption != nil && swag.StringValue(cluster.DiskEncryption.Mode) == models.DiskEncryptionModeTang { + if common.IsSetWithTang(cluster.DiskEncryption) { utils_test.TestContext.GenerateTangPostStepReply(ctx, tangValidated, hosts...) }