From 789356467e9acd8ba56bd3bc2afd74839a5529a7 Mon Sep 17 00:00:00 2001 From: Dmitry Tantsur Date: Tue, 5 Dec 2023 14:48:59 +0100 Subject: [PATCH] Stop defaulting to MD5 for checksumType In preparation for the eventual removal of MD5 checksums, change the default to guess the algorithm, which IPA is capable of starting with the 2023.2 release. Users with older IPA should provide the algorithm explicitly. Signed-off-by: Dmitry Tantsur --- .../metal3.io/v1alpha1/baremetalhost_types.go | 6 +-- .../v1alpha1/baremetalhost_types_test.go | 23 +++++---- .../crds/bases/metal3.io_baremetalhosts.yaml | 8 +-- config/render/capm3.yaml | 8 +-- docs/api.md | 4 +- pkg/provisioner/ironic/ironic.go | 50 ++++++++++--------- pkg/provisioner/ironic/provision_test.go | 29 +++++++++++ .../ironic/validatemanagementaccess_test.go | 24 ++++++++- 8 files changed, 107 insertions(+), 45 deletions(-) diff --git a/apis/metal3.io/v1alpha1/baremetalhost_types.go b/apis/metal3.io/v1alpha1/baremetalhost_types.go index 15189eac06..4e33bc6e4a 100644 --- a/apis/metal3.io/v1alpha1/baremetalhost_types.go +++ b/apis/metal3.io/v1alpha1/baremetalhost_types.go @@ -473,8 +473,8 @@ type Image struct { // Checksum is the checksum for the image. Checksum string `json:"checksum,omitempty"` - // ChecksumType is the checksum algorithm for the image. - // e.g md5, sha256, sha512 + // ChecksumType is the checksum algorithm for the image, e.g md5, sha256 or sha512. + // If missing, detected from the checksum length. ChecksumType ChecksumType `json:"checksumType,omitempty"` // DiskFormat contains the format of the image (raw, qcow2, ...). @@ -1093,7 +1093,7 @@ func (image *Image) GetChecksum() (checksum, checksumType string, ok bool) { switch image.ChecksumType { case "": - checksumType = string(MD5) + // No type, let Ironic detect case MD5, SHA256, SHA512: checksumType = string(image.ChecksumType) default: diff --git a/apis/metal3.io/v1alpha1/baremetalhost_types_test.go b/apis/metal3.io/v1alpha1/baremetalhost_types_test.go index 5e1435a6ce..e710c373dc 100644 --- a/apis/metal3.io/v1alpha1/baremetalhost_types_test.go +++ b/apis/metal3.io/v1alpha1/baremetalhost_types_test.go @@ -317,9 +317,10 @@ func TestCredentialStatusMatch(t *testing.T) { func TestGetImageChecksum(t *testing.T) { for _, tc := range []struct { - Scenario string - Host BareMetalHost - Expected bool + Scenario string + Host BareMetalHost + Expected bool + ExpectedType string }{ { Scenario: "both checksum value and type specified", @@ -335,7 +336,8 @@ func TestGetImageChecksum(t *testing.T) { }, }, }, - Expected: true, + Expected: true, + ExpectedType: "md5", }, { Scenario: "checksum value specified but not type", @@ -366,7 +368,8 @@ func TestGetImageChecksum(t *testing.T) { }, }, }, - Expected: true, + Expected: true, + ExpectedType: "sha256", }, { Scenario: "sha512 checksum value and type specified", @@ -382,7 +385,8 @@ func TestGetImageChecksum(t *testing.T) { }, }, }, - Expected: true, + Expected: true, + ExpectedType: "sha512", }, { Scenario: "checksum value not specified", @@ -443,9 +447,10 @@ func TestGetImageChecksum(t *testing.T) { }, } { t.Run(tc.Scenario, func(t *testing.T) { - _, _, actual := tc.Host.Spec.Image.GetChecksum() - if actual != tc.Expected { - t.Errorf("expected %v but got %v", tc.Expected, actual) + _, checksumType, actual := tc.Host.Spec.Image.GetChecksum() + assert.Equal(t, tc.Expected, actual) + if tc.Expected { + assert.Equal(t, tc.ExpectedType, checksumType) } }) } diff --git a/config/base/crds/bases/metal3.io_baremetalhosts.yaml b/config/base/crds/bases/metal3.io_baremetalhosts.yaml index 77a5756599..bac864b2e8 100644 --- a/config/base/crds/bases/metal3.io_baremetalhosts.yaml +++ b/config/base/crds/bases/metal3.io_baremetalhosts.yaml @@ -221,8 +221,9 @@ spec: description: Checksum is the checksum for the image. type: string checksumType: - description: ChecksumType is the checksum algorithm for the image. - e.g md5, sha256, sha512 + description: ChecksumType is the checksum algorithm for the image, + e.g md5, sha256 or sha512. If missing, detected from the checksum + length. enum: - md5 - sha256 @@ -871,7 +872,8 @@ spec: type: string checksumType: description: ChecksumType is the checksum algorithm for the - image. e.g md5, sha256, sha512 + image, e.g md5, sha256 or sha512. If missing, detected from + the checksum length. enum: - md5 - sha256 diff --git a/config/render/capm3.yaml b/config/render/capm3.yaml index ba6d83f1c2..6af061c40f 100644 --- a/config/render/capm3.yaml +++ b/config/render/capm3.yaml @@ -221,8 +221,9 @@ spec: description: Checksum is the checksum for the image. type: string checksumType: - description: ChecksumType is the checksum algorithm for the image. - e.g md5, sha256, sha512 + description: ChecksumType is the checksum algorithm for the image, + e.g md5, sha256 or sha512. If missing, detected from the checksum + length. enum: - md5 - sha256 @@ -871,7 +872,8 @@ spec: type: string checksumType: description: ChecksumType is the checksum algorithm for the - image. e.g md5, sha256, sha512 + image, e.g md5, sha256 or sha512. If missing, detected from + the checksum length. enum: - md5 - sha256 diff --git a/docs/api.md b/docs/api.md index 5e9432b350..bef9286a8b 100644 --- a/docs/api.md +++ b/docs/api.md @@ -91,8 +91,8 @@ The sub-fields are * *checksum* -- The actual checksum or a URL to a file containing the checksum for the image at *image.url*. * *checksumType* -- Checksum algorithms can be specified. Currently - only `md5`, `sha256`, `sha512` are recognized. If nothing is specified - `md5` is assumed. + only `md5`, `sha256`, `sha512` are recognized. If nothing is specified, + the algorithm is guessed from the checksum length. * *format* -- This is the disk format of the image. It can be one of `raw`, `qcow2`, `vdi`, `vmdk`, `live-iso` or be left unset. Setting it to raw enables raw image streaming in Ironic agent for that image. diff --git a/pkg/provisioner/ironic/ironic.go b/pkg/provisioner/ironic/ironic.go index 88ed19e07f..45b4d1b2c3 100644 --- a/pkg/provisioner/ironic/ironic.go +++ b/pkg/provisioner/ironic/ironic.go @@ -965,26 +965,21 @@ func (p *ironicProvisioner) setDirectDeployUpdateOptsForNode(ironicNode *nodes.N return } - // FIXME: For older versions of ironic that do not have - // https://review.opendev.org/#/c/711816/ failing to include the - // 'image_checksum' causes ironic to refuse to provision the - // image, even if the other hash value parameters are given. We - // only want to do that for MD5, however, because those versions - // of ironic only support MD5 checksums. - var legacyChecksum *string - if checksumType == string(metal3api.MD5) { - legacyChecksum = &checksum - } - optValues := optionsData{ // Remove any boot_iso field - "boot_iso": nil, + "boot_iso": nil, + "image_source": imageData.URL, + "image_disk_format": imageData.DiskFormat, + } - "image_source": imageData.URL, - "image_os_hash_algo": checksumType, - "image_os_hash_value": checksum, - "image_checksum": legacyChecksum, - "image_disk_format": imageData.DiskFormat, + if checksumType == "" { + optValues["image_checksum"] = checksum + optValues["image_os_hash_algo"] = nil + optValues["image_os_hash_value"] = nil + } else { + optValues["image_checksum"] = nil + optValues["image_os_hash_algo"] = checksumType + optValues["image_os_hash_value"] = checksum } updater. SetInstanceInfoOpts(optValues, ironicNode) @@ -1282,15 +1277,24 @@ func (p *ironicProvisioner) ironicHasSameImage(ironicNode *nodes.Node, image met "provisionState", ironicNode.ProvisionState) } else { checksum, checksumType, _ := image.GetChecksum() - sameImage = (ironicNode.InstanceInfo["image_source"] == image.URL && - ironicNode.InstanceInfo["image_os_hash_algo"] == checksumType && - ironicNode.InstanceInfo["image_os_hash_value"] == checksum) + if checksumType == "" { + sameImage = (ironicNode.InstanceInfo["image_source"] == image.URL && + ironicNode.InstanceInfo["image_checksum"] == checksum && + ironicNode.InstanceInfo["image_os_hash_algo"] == nil && + ironicNode.InstanceInfo["image_os_hash_value"] == nil) + } else { + sameImage = (ironicNode.InstanceInfo["image_source"] == image.URL && + ironicNode.InstanceInfo["image_checksum"] == nil && + ironicNode.InstanceInfo["image_os_hash_algo"] == checksumType && + ironicNode.InstanceInfo["image_os_hash_value"] == checksum) + } p.log.Info("checking image settings", "source", ironicNode.InstanceInfo["image_source"], - "image_os_hash_algo", checksumType, - "image_os_has_value", checksum, + "checksumType", checksumType, + "checksum", checksum, "same", sameImage, - "provisionState", ironicNode.ProvisionState) + "provisionState", ironicNode.ProvisionState, + "iinfo", ironicNode.InstanceInfo) } return sameImage } diff --git a/pkg/provisioner/ironic/provision_test.go b/pkg/provisioner/ironic/provision_test.go index 121b0d99fb..1620cf946e 100644 --- a/pkg/provisioner/ironic/provision_test.go +++ b/pkg/provisioner/ironic/provision_test.go @@ -343,6 +343,20 @@ func TestIronicHasSameImage(t *testing.T) { hostChecksum: "thechecksum", hostChecksumType: v1alpha1.MD5, }, + { + name: "image same - default checksum", + expected: true, + liveImage: false, + node: nodes.Node{ + InstanceInfo: map[string]interface{}{ + "image_source": "theimage", + "image_checksum": "thechecksum", + }, + }, + hostImage: "theimage", + hostChecksum: "thechecksum", + hostChecksumType: "", + }, { name: "image different", expected: false, @@ -373,6 +387,21 @@ func TestIronicHasSameImage(t *testing.T) { hostChecksum: "different", hostChecksumType: v1alpha1.MD5, }, + { + name: "image checksum changed to default", + expected: false, + liveImage: false, + node: nodes.Node{ + InstanceInfo: map[string]interface{}{ + "image_source": "theimage", + "image_os_hash_value": "thechecksum", + "image_os_hash_algo": "md5", + }, + }, + hostImage: "theimage", + hostChecksum: "thechecksum", + hostChecksumType: "", + }, { name: "image checksum type different", expected: false, diff --git a/pkg/provisioner/ironic/validatemanagementaccess_test.go b/pkg/provisioner/ironic/validatemanagementaccess_test.go index eb1f99e6b0..040ecd1338 100644 --- a/pkg/provisioner/ironic/validatemanagementaccess_test.go +++ b/pkg/provisioner/ironic/validatemanagementaccess_test.go @@ -394,11 +394,31 @@ func TestValidateManagementAccessExistingSteadyStateNoUpdate(t *testing.T) { URL: "theimage", Checksum: "thechecksum", }, + InstanceInfo: map[string]interface{}{ + "image_source": "theimage", + "image_checksum": "thechecksum", + "capabilities": map[string]interface{}{}, + }, + DriverInfo: map[string]interface{}{ + "force_persistent_boot_device": "Default", + "deploy_kernel": "http://deploy.test/ipa.kernel", + "deploy_ramdisk": "http://deploy.test/ipa.initramfs", + "test_address": "test.bmc", + "test_username": "", + "test_password": "******", // ironic returns a placeholder + "test_port": "42", + }, + }, + { + Image: &metal3api.Image{ + URL: "theimage", + Checksum: "thechecksum", + ChecksumType: "sha256", + }, InstanceInfo: map[string]interface{}{ "image_source": "theimage", - "image_os_hash_algo": "md5", + "image_os_hash_algo": "sha256", "image_os_hash_value": "thechecksum", - "image_checksum": "thechecksum", "capabilities": map[string]interface{}{}, }, DriverInfo: map[string]interface{}{