Merge pull request #60 from ddelnano/ddelnano/remove_iso_checksum_typ…

…e_and_fix_tests Remove iso checksum type and fix tests
ddelnano · Apr 7, 2023 · b55b096 · b55b096
2 parents 9cc7249 + 6c808da
commit b55b096
Show file tree

Hide file tree

Showing 10 changed files with 65 additions and 115 deletions.
diff --git a/builder/xenserver/common/config.go b/builder/xenserver/common/config.go
@@ -1,4 +1,4 @@
-//go:generate mapstructure-to-hcl2 -type Config
+//go:generate packer-sdc mapstructure-to-hcl2 -type Config
 package common
 
 import (
@@ -21,11 +21,10 @@ type Config struct {
 	CloneTemplate  string            `mapstructure:"clone_template"`
 	VMOtherConfig  map[string]string `mapstructure:"vm_other_config"`
 
-	ISOChecksum     string   `mapstructure:"iso_checksum"`
-	ISOChecksumType string   `mapstructure:"iso_checksum_type"`
-	ISOUrls         []string `mapstructure:"iso_urls"`
-	ISOUrl          string   `mapstructure:"iso_url"`
-	ISOName         string   `mapstructure:"iso_name"`
+	ISOChecksum string   `mapstructure:"iso_checksum"`
+	ISOUrls     []string `mapstructure:"iso_urls"`
+	ISOUrl      string   `mapstructure:"iso_url"`
+	ISOName     string   `mapstructure:"iso_name"`
 
 	PlatformArgs map[string]string `mapstructure:"platform_args"`
 

diff --git a/builder/xenserver/common/config.hcl2spec.go b/builder/xenserver/common/config.hcl2spec.go
diff --git a/builder/xenserver/iso/builder.go b/builder/xenserver/iso/builder.go
@@ -40,7 +40,7 @@ func (self *Builder) Prepare(raws ...interface{}) (params []string, warns []stri
 	}, raws...)
 
 	if err != nil {
-		packer.MultiErrorAppend(errs, err)
+		errs = packer.MultiErrorAppend(errs, err)
 	}
 
 	errs = packer.MultiErrorAppend(
@@ -95,12 +95,11 @@ func (self *Builder) Prepare(raws ...interface{}) (params []string, warns []stri
 	// Template substitution
 
 	templates := map[string]*string{
-		"clone_template":    &self.config.CloneTemplate,
-		"iso_checksum":      &self.config.ISOChecksum,
-		"iso_checksum_type": &self.config.ISOChecksumType,
-		"iso_url":           &self.config.ISOUrl,
-		"iso_name":          &self.config.ISOName,
-		"install_timeout":   &self.config.RawInstallTimeout,
+		"clone_template":  &self.config.CloneTemplate,
+		"iso_checksum":    &self.config.ISOChecksum,
+		"iso_url":         &self.config.ISOUrl,
+		"iso_name":        &self.config.ISOName,
+		"install_timeout": &self.config.RawInstallTimeout,
 	}
 	for i := range self.config.ISOUrls {
 		templates[fmt.Sprintf("iso_urls[%d]", i)] = &self.config.ISOUrls[i]
@@ -115,23 +114,8 @@ func (self *Builder) Prepare(raws ...interface{}) (params []string, warns []stri
 	}
 
 	if self.config.ISOName == "" {
-
 		// If ISO name is not specified, assume a URL and checksum has been provided.
-
-		if self.config.ISOChecksumType == "" {
-			errs = packer.MultiErrorAppend(
-				errs, errors.New("The iso_checksum_type must be specified."))
-		} else {
-			self.config.ISOChecksumType = strings.ToLower(self.config.ISOChecksumType)
-			if self.config.ISOChecksumType != "none" {
-				if self.config.ISOChecksum == "" {
-					errs = packer.MultiErrorAppend(
-						errs, errors.New("Due to the file size being large, an iso_checksum is required."))
-				} else {
-					self.config.ISOChecksum = strings.ToLower(self.config.ISOChecksum)
-				}
-			}
-		}
+		self.config.ISOChecksum = strings.ToLower(self.config.ISOChecksum)
 
 		if len(self.config.ISOUrls) == 0 {
 			if self.config.ISOUrl == "" {
@@ -140,10 +124,25 @@ func (self *Builder) Prepare(raws ...interface{}) (params []string, warns []stri
 			} else {
 				self.config.ISOUrls = []string{self.config.ISOUrl}
 			}
+
 		} else if self.config.ISOUrl != "" {
 			errs = packer.MultiErrorAppend(
 				errs, errors.New("Only one of iso_url or iso_urls may be specified."))
 		}
+
+		//The SDK can validate the ISO checksum and other sanity checks on the url.
+		iso_config := commonsteps.ISOConfig{
+			ISOChecksum: self.config.ISOChecksum,
+			ISOUrls:     self.config.ISOUrls,
+		}
+
+		_, iso_errs := iso_config.Prepare(nil)
+		if iso_errs != nil {
+			for _, this_err := range iso_errs {
+				errs = packer.MultiErrorAppend(errs, this_err)
+			}
+		}
+
 	} else {
 
 		// An ISO name has been provided. It should be attached from an available SR.

diff --git a/builder/xenserver/iso/builder_test.go b/builder/xenserver/iso/builder_test.go
@@ -5,21 +5,22 @@ import (
 	"testing"
 
 	"github.com/hashicorp/packer-plugin-sdk/packer"
+	"github.com/hashicorp/packer-plugin-sdk/common"
 )
 
+
 func testConfig() map[string]interface{} {
 	return map[string]interface{}{
 		"remote_host":       "localhost",
 		"remote_username":   "admin",
 		"remote_password":   "admin",
 		"vm_name":           "foo",
-		"iso_checksum":      "foo",
-		"iso_checksum_type": "md5",
+		"iso_checksum":      "md5:A221725EE181A44C67E25BD6A2516742",
 		"iso_url":           "http://www.google.com/",
 		"shutdown_command":  "yes",
 		"ssh_username":      "foo",
 
-		packer.BuildNameConfigKey: "foo",
+		common.BuildNameConfigKey: "foo",
 	}
 }
 
@@ -179,64 +180,19 @@ func TestBuilderPrepare_ISOChecksum(t *testing.T) {
 	var b Builder
 	config := testConfig()
 
-	// Test bad
-	config["iso_checksum"] = ""
-	_, warns, err := b.Prepare(config)
-	if len(warns) > 0 {
-		t.Fatalf("bad: %#v", warns)
-	}
-	if err == nil {
-		t.Fatal("should have error")
-	}
-
 	// Test good
-	config["iso_checksum"] = "FOo"
-	b = Builder{}
-	_, warns, err = b.Prepare(config)
-	if len(warns) > 0 {
-		t.Fatalf("bad: %#v", warns)
-	}
-	if err != nil {
-		t.Fatalf("should not have error: %s", err)
-	}
-
-	if b.config.ISOChecksum != "foo" {
-		t.Fatalf("should've lowercased: %s", b.config.ISOChecksum)
-	}
-}
-
-func TestBuilderPrepare_ISOChecksumType(t *testing.T) {
-	var b Builder
-	config := testConfig()
 
-	// Test bad
-	config["iso_checksum_type"] = ""
-	_, warns, err := b.Prepare(config)
-	if len(warns) > 0 {
-		t.Fatalf("bad: %#v", warns)
-	}
-	if err == nil {
-		t.Fatal("should have error")
-	}
-
-	// Test good
-	config["iso_checksum_type"] = "mD5"
 	b = Builder{}
-	_, warns, err = b.Prepare(config)
+	_, warns, err := b.Prepare(config)
 	if len(warns) > 0 {
 		t.Fatalf("bad: %#v", warns)
 	}
 	if err != nil {
 		t.Fatalf("should not have error: %s", err)
 	}
 
-	if b.config.ISOChecksumType != "md5" {
-		t.Fatalf("should've lowercased: %s", b.config.ISOChecksumType)
-	}
-
-	// Test unknown
-	config["iso_checksum_type"] = "fake"
-	b = Builder{}
+	// Test bad
+	config["iso_checksum"] = ""
 	_, warns, err = b.Prepare(config)
 	if len(warns) > 0 {
 		t.Fatalf("bad: %#v", warns)
@@ -245,25 +201,10 @@ func TestBuilderPrepare_ISOChecksumType(t *testing.T) {
 		t.Fatal("should have error")
 	}
 
-	// Test none
-	config["iso_checksum_type"] = "none"
-	b = Builder{}
-	_, warns, err = b.Prepare(config)
-	// @todo: give warning in this case?
-	/*
-		if len(warns) == 0 {
-			t.Fatalf("bad: %#v", warns)
-		}
-	*/
-	if err != nil {
-		t.Fatalf("should not have error: %s", err)
-	}
 
-	if b.config.ISOChecksumType != "none" {
-		t.Fatalf("should've lowercased: %s", b.config.ISOChecksumType)
-	}
 }
 
+
 func TestBuilderPrepare_ISOUrl(t *testing.T) {
 	var b Builder
 	config := testConfig()

diff --git a/builder/xenserver/xva/builder.go b/builder/xenserver/xva/builder.go
@@ -38,7 +38,7 @@ func (self *Builder) Prepare(raws ...interface{}) (params []string, warns []stri
 	}, raws...)
 
 	if err != nil {
-		packer.MultiErrorAppend(errs, err)
+		errs = packer.MultiErrorAppend(errs, err)
 	}
 
 	errs = packer.MultiErrorAppend(

diff --git a/builder/xenserver/xva/builder_test.go b/builder/xenserver/xva/builder_test.go
@@ -4,6 +4,7 @@ import (
 	"testing"
 
 	"github.com/hashicorp/packer-plugin-sdk/packer"
+	"github.com/hashicorp/packer-plugin-sdk/common"
 )
 
 func testConfig() map[string]interface{} {
@@ -16,7 +17,7 @@ func testConfig() map[string]interface{} {
 		"ssh_username":     "foo",
 		"source_path":      ".",
 
-		packer.BuildNameConfigKey: "foo",
+		common.BuildNameConfigKey: "foo",
 	}
 }
 

diff --git a/docs/builders/iso/xenserver-iso.html.markdown b/docs/builders/iso/xenserver-iso.html.markdown
@@ -29,14 +29,27 @@ each category, the available options are alphabetized and described.
 
 * `iso_checksum` (string) - The checksum for the OS ISO file. Because ISO
   files are so large, this is required and Packer will verify it prior
-  to booting a virtual machine with the ISO attached. The type of the
-  checksum is specified with `iso_checksum_type`, documented below.
-
-* `iso_checksum_type` (string) - The type of the checksum specified in
-  `iso_checksum`. Valid values are "none", "md5", "sha1", "sha256", or
-  "sha512" currently. While "none" will skip checksumming, this is not
-  recommended since ISO files are generally large and corruption does happen
-  from time to time.
+  to booting a virtual machine with the ISO attached. The type of
+  the checksum is specified within the checksum field as a prefix, ex:
+  "md5:{$checksum}". The type of the checksum can also be omitted and
+  Packer will try to infer it based on string length. Valid values are
+  "none", "{$checksum}", "md5:{$checksum}", "sha1:{$checksum}",
+  "sha256:{$checksum}", "sha512:{$checksum}" or "file:{$path}". Here is a
+  list of valid checksum values:
+   * md5:090992ba9fd140077b0661cb75f7ce13
+   * 090992ba9fd140077b0661cb75f7ce13
+   * sha1:ebfb681885ddf1234c18094a45bbeafd91467911
+   * ebfb681885ddf1234c18094a45bbeafd91467911
+   * sha256:ed363350696a726b7932db864dda019bd2017365c9e299627830f06954643f93
+   * ed363350696a726b7932db864dda019bd2017365c9e299627830f06954643f93
+   * file:http://releases.ubuntu.com/20.04/SHA256SUMS
+   * file:file://./local/path/file.sum
+   * file:./local/path/file.sum
+   * none
+  Although the checksum will not be verified when it is set to "none",
+  this is not recommended since these files can be very large and
+  corruption does happen from time to time.
+
 
 * `iso_url` (string) - A URL to the ISO containing the installation image.
   This URL can be either an HTTP URL or a file URL (or path to a file).

diff --git a/examples/centos/centos8-local.pkr.hcl b/examples/centos/centos8-local.pkr.hcl
@@ -47,8 +47,7 @@ locals {
 }
 
 source "xenserver-iso" "centos8-local" {
-  iso_checksum      = "aaf9d4b3071c16dbbda01dfe06085e5d0fdac76df323e3bbe87cce4318052247"
-  iso_checksum_type = "sha1"
+  iso_checksum      = "sha1:aaf9d4b3071c16dbbda01dfe06085e5d0fdac76df323e3bbe87cce4318052247"
   iso_url           = "http://mirrors.ocf.berkeley.edu/centos/8.3.2011/isos/x86_64/CentOS-8.3.2011-x86_64-dvd1.iso"
 
   sr_iso_name    = var.sr_iso_name

diff --git a/examples/centos/centos8-netinstall.pkr.hcl b/examples/centos/centos8-netinstall.pkr.hcl
@@ -47,8 +47,7 @@ locals {
 }
 
 source "xenserver-iso" "centos8-netinstall" {
-  iso_checksum      = "07a8e59c42cc086ec4c49bdce4fae5a17b077dea"
-  iso_checksum_type = "sha1"
+  iso_checksum      = "sha1:07a8e59c42cc086ec4c49bdce4fae5a17b077dea"
   iso_url           = "http://mirrors.ocf.berkeley.edu/centos/8.3.2011/isos/x86_64/CentOS-8.3.2011-x86_64-boot.iso"
 
   sr_iso_name    = var.sr_iso_name

diff --git a/examples/ubuntu/ubuntu-2004.pkr.hcl b/examples/ubuntu/ubuntu-2004.pkr.hcl
@@ -74,8 +74,7 @@ variable "sr_name" {
 }
 
 source "xenserver-iso" "ubuntu-2004" {
-  iso_checksum      = local.ubuntu_sha256.0
-  iso_checksum_type = "sha256"
+  iso_checksum      = "sha256:${local.ubuntu_sha256.0}"
   iso_url           = "https://releases.ubuntu.com/${local.ubuntu_version}/ubuntu-${local.ubuntu_version}.${local.ubuntu_url_path.0}-live-server-amd64.iso"
 
   sr_iso_name    = var.sr_iso_name