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 <[email protected]>
metal3-io · Dec 6, 2023 · 7893564 · 7893564
1 parent 63abe2d
commit 7893564
Show file tree

Hide file tree

Showing 8 changed files with 107 additions and 45 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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{}{