Add an explicit way to auto-detect ChecksumType

In preparation for the eventual removal of MD5 checksums, add a new ChecksumType "auto" that instructs IPA to detect the type Requires IPA from the 2023.2 release series. Users with older IPA should provide the algorithm explicitly. Signed-off-by: Dmitry Tantsur <[email protected]>
metal3-io · Dec 13, 2023 · 0ed5eb1 · 0ed5eb1
1 parent 63abe2d
commit 0ed5eb1
Show file tree

Hide file tree

Showing 10 changed files with 149 additions and 52 deletions.
diff --git a/apis/metal3.io/v1alpha1/baremetalhost_types.go b/apis/metal3.io/v1alpha1/baremetalhost_types.go
@@ -450,7 +450,7 @@ const (
 )
 
 // ChecksumType holds the algorithm name for the checksum
-// +kubebuilder:validation:Enum=md5;sha256;sha512
+// +kubebuilder:validation:Enum=md5;sha256;sha512;auto
 type ChecksumType string
 
 const (
@@ -462,6 +462,9 @@ const (
 
 	// SHA512 checksum type
 	SHA512 ChecksumType = "sha512"
+
+	// Automatically detect
+	AutoChecksum ChecksumType = "auto"
 )
 
 // Image holds the details of an image either to provisioned or that
@@ -473,8 +476,9 @@ 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.
+	// The special value "auto" can be used to detect the algorithm from the checksum.
+	// If missing, MD5 is used. If in doubt, use "auto".
 	ChecksumType ChecksumType `json:"checksumType,omitempty"`
 
 	// DiskFormat contains the format of the image (raw, qcow2, ...).
@@ -1096,6 +1100,8 @@ func (image *Image) GetChecksum() (checksum, checksumType string, ok bool) {
 		checksumType = string(MD5)
 	case MD5, SHA256, SHA512:
 		checksumType = string(image.ChecksumType)
+	case AutoChecksum:
+		// No type, let Ironic detect
 	default:
 		return
 	}

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",
@@ -350,7 +352,25 @@ func TestGetImageChecksum(t *testing.T) {
 					},
 				},
 			},
-			Expected: true,
+			Expected:     true,
+			ExpectedType: "md5",
+		},
+		{
+			Scenario: "checksum value specified, auto type",
+			Host: BareMetalHost{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "myhost",
+					Namespace: "myns",
+				},
+				Spec: BareMetalHostSpec{
+					Image: &Image{
+						Checksum:     "md5hash",
+						ChecksumType: AutoChecksum,
+					},
+				},
+			},
+			Expected:     true,
+			ExpectedType: "",
 		},
 		{
 			Scenario: "sha256 checksum value and type specified",
@@ -366,7 +386,8 @@ func TestGetImageChecksum(t *testing.T) {
 					},
 				},
 			},
-			Expected: true,
+			Expected:     true,
+			ExpectedType: "sha256",
 		},
 		{
 			Scenario: "sha512 checksum value and type specified",
@@ -382,7 +403,8 @@ func TestGetImageChecksum(t *testing.T) {
 					},
 				},
 			},
-			Expected: true,
+			Expected:     true,
+			ExpectedType: "sha512",
 		},
 		{
 			Scenario: "checksum value not specified",
@@ -443,9 +465,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/cmd/make-bm-worker/main.go b/cmd/make-bm-worker/main.go
@@ -62,9 +62,9 @@ func main() {
 	}
 
 	switch *imageChecksumType {
-	case "", "md5", "sha256", "sha512":
+	case "", "md5", "sha256", "sha512", "auto":
 	default:
-		fmt.Fprintf(os.Stderr, "Invalid image checksum type %q, use \"md5\", \"sha256\" or \"sha512\"\n", *imageChecksumType)
+		fmt.Fprintf(os.Stderr, "Invalid image checksum type %q, use \"md5\", \"sha256\", \"sha512\" or \"auto\"\n", *imageChecksumType)
 		os.Exit(1)
 	}
 

diff --git a/config/base/crds/bases/metal3.io_baremetalhosts.yaml b/config/base/crds/bases/metal3.io_baremetalhosts.yaml
@@ -221,12 +221,15 @@ 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. The special value "auto" can be used
+                      to detect the algorithm from the checksum. If missing, MD5 is
+                      used. If in doubt, use "auto".
                     enum:
                     - md5
                     - sha256
                     - sha512
+                    - auto
                     type: string
                   format:
                     description: DiskFormat contains the format of the image (raw,
@@ -871,11 +874,14 @@ 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. The special value "auto"
+                          can be used to detect the algorithm from the checksum. If
+                          missing, MD5 is used. If in doubt, use "auto".
                         enum:
                         - md5
                         - sha256
                         - sha512
+                        - auto
                         type: string
                       format:
                         description: DiskFormat contains the format of the image (raw,

diff --git a/config/render/capm3.yaml b/config/render/capm3.yaml
@@ -221,12 +221,15 @@ 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. The special value "auto" can be used
+                      to detect the algorithm from the checksum. If missing, MD5 is
+                      used. If in doubt, use "auto".
                     enum:
                     - md5
                     - sha256
                     - sha512
+                    - auto
                     type: string
                   format:
                     description: DiskFormat contains the format of the image (raw,
@@ -871,11 +874,14 @@ 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. The special value "auto"
+                          can be used to detect the algorithm from the checksum. If
+                          missing, MD5 is used. If in doubt, use "auto".
                         enum:
                         - md5
                         - sha256
                         - sha512
+                        - auto
                         type: string
                       format:
                         description: DiskFormat contains the format of the image (raw,

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. Use `auto` to auto-detect
+  the algorithm from the value. If nothing is specified, `md5` is assumed.
 * *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 - auto checksum",
+			expected:  true,
+			liveImage: false,
+			node: nodes.Node{
+				InstanceInfo: map[string]interface{}{
+					"image_source":   "theimage",
+					"image_checksum": "thechecksum",
+				},
+			},
+			hostImage:        "theimage",
+			hostChecksum:     "thechecksum",
+			hostChecksumType: "auto",
+		},
 		{
 			name:      "image different",
 			expected:  false,
@@ -373,6 +387,21 @@ func TestIronicHasSameImage(t *testing.T) {
 			hostChecksum:     "different",
 			hostChecksumType: v1alpha1.MD5,
 		},
+		{
+			name:      "image checksum changed to auto",
+			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: "auto",
+		},
 		{
 			name:      "image checksum type different",
 			expected:  false,

diff --git a/pkg/provisioner/ironic/validatemanagementaccess_test.go b/pkg/provisioner/ironic/validatemanagementaccess_test.go
@@ -153,6 +153,7 @@ func TestValidateManagementAccessCreateWithImage(t *testing.T) {
 	host.Status.Provisioning.ID = "" // so we don't lookup by uuid
 	host.Spec.Image.URL = "theimagefoo"
 	host.Spec.Image.Checksum = "thechecksumxyz"
+	host.Spec.Image.ChecksumType = "auto"
 
 	var createdNode *nodes.Node
 
@@ -391,14 +392,35 @@ func TestValidateManagementAccessExistingSteadyStateNoUpdate(t *testing.T) {
 		},
 		{
 			Image: &metal3api.Image{
-				URL:      "theimage",
-				Checksum: "thechecksum",
+				URL:          "theimage",
+				Checksum:     "thechecksum",
+				ChecksumType: "auto",
+			},
+			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{}{

diff --git a/test/e2e/basic_provisioning_test.go b/test/e2e/basic_provisioning_test.go
@@ -83,8 +83,9 @@ var _ = Describe("BMH Provisioning and Annotation Management", func() {
 		helper, err := patch.NewHelper(&bmh, clusterProxy.GetClient())
 		Expect(err).NotTo(HaveOccurred())
 		bmh.Spec.Image = &metal3api.Image{
-			URL:      e2eConfig.GetVariable("IMAGE_URL"),
-			Checksum: e2eConfig.GetVariable("IMAGE_CHECKSUM"),
+			URL:          e2eConfig.GetVariable("IMAGE_URL"),
+			Checksum:     e2eConfig.GetVariable("IMAGE_CHECKSUM"),
+			ChecksumType: metal3api.AutoChecksum,
 		}
 		bmh.Spec.RootDeviceHints = &metal3api.RootDeviceHints{
 			DeviceName: "/dev/vda",