builder: fix image label sanity checks

When the image sanity checks were introduced in v1.1.7, an error slipped
in as the same regex was used to validate keys and labels, however the
rules are slightly different between the two, as keys are mandated to
start with [a-z], while values are not.

This commit addresses this bug by changing to using two regexes, so we
can still validate that both are valid before attempting to build the
image, but we are not unnecessarily rejecting valid values in the map.

builder/googlecompute/config.go

@@ -632,6 +632,12 @@ func (c *Config) Prepare(raws ...interface{}) ([]string, error) {
 		c.WindowsPasswordTimeout = 3 * time.Minute
 	}
 
+	if labelErrs := c.AreLabelsValid(); len(labelErrs) > 0 {
+		for _, err := range labelErrs {
+			errs = packersdk.MultiErrorAppend(err)
+		}
+	}
+
 	return warnings, errs
 }
 
@@ -657,24 +663,17 @@ func ApplyIAPTunnel(c *communicator.Config, port int) error {
 	}
 }
 
+var labelKeyRegex = regexp.MustCompile(`^\p{Ll}[\p{Ll}0-9_-]{0,62}$`)
+var labelValueRegex = regexp.MustCompile(`^[\p{Ll}0-9_-]{0,63}$`)
+
 func (c *Config) AreLabelsValid() []error {
 	var errs []error
-	labelRegex := regexp.MustCompile(`^[a-z][a-z0-9_-]{0,62}$`)
 	for key, value := range c.ImageLabels {
-		if len(key) < 1 {
-			errs = append(errs, fmt.Errorf("key %q must be at least 1 character long", key))
-		}
-		if len(key) > 63 {
-			errs = append(errs, fmt.Errorf("key %q must be at most 63 characters long", key))
-		}
-		if len(value) > 63 {
-			errs = append(errs, fmt.Errorf("value %q for key %q must be at most 63 characters long", value, key))
-		}
-		if !labelRegex.MatchString(key) {
-			errs = append(errs, fmt.Errorf("key %q must match regex %q", key, labelRegex))
+		if !labelKeyRegex.MatchString(key) {
+			errs = append(errs, fmt.Errorf("key %q must match regex %q", key, labelKeyRegex))
 		}
-		if !labelRegex.MatchString(value) {
-			errs = append(errs, fmt.Errorf("value %q for key %q must match regex %q", value, key, labelRegex))
+		if !labelValueRegex.MatchString(value) {
+			errs = append(errs, fmt.Errorf("value %q for key %q must match regex %q", value, key, labelValueRegex))
 		}
 	}
 	return errs

builder/googlecompute/config_test.go

@@ -712,6 +712,122 @@ func TestConfigPrepareImageArchitecture(t *testing.T) {
 	}
 }
 
+func TestLabelsValidity(t *testing.T) {
+	cases := []struct {
+		Name        string
+		Labels      map[string]string
+		ExpectError bool
+	}{
+		{
+			"valid empty labels map, should not error",
+			map[string]string{},
+			false,
+		},
+		{
+			"valid non-empty keys/values, should not error",
+			map[string]string{
+				"label-1": "value",
+			},
+			false,
+		},
+		{
+			"valid non-empty key, empty value, should not error",
+			map[string]string{
+				"label-1": "",
+			},
+			false,
+		},
+		{
+			"valid non-empty key, value starting with a non a-z character, should not error",
+			map[string]string{
+				"label-1": "0644",
+			},
+			false,
+		},
+		{
+			"valid international-letter key and value, should not error",
+			map[string]string{
+				"lābêl-1": "žžž",
+			},
+			false,
+		},
+		{
+			"valid international key and value, at the limit of what is supported for both, should not error",
+			map[string]string{
+				"āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāā": "āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāā",
+			},
+			false,
+		},
+		{
+			"invalid key with upper-case international character in it, non-empty value, should error",
+			map[string]string{
+				"Ā": "abcd",
+			},
+			true,
+		},
+		{
+			"invalid key with lower-case international character in it, but upper-case character in value, should error",
+			map[string]string{
+				"ã": "Abcd",
+			},
+			true,
+		},
+		{
+			"invalid empty key, non-empty value, should error",
+			map[string]string{
+				"": "abcd",
+			},
+			true,
+		},
+		{
+			"invalid key too long, non-empty value, should error",
+			map[string]string{
+				"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa": "abcd",
+			},
+			true,
+		},
+		{
+			"invalid value too long, key OK, should error",
+			map[string]string{
+				"aaa": "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa",
+			},
+			true,
+		},
+		{
+			"invalid international key too long, should error",
+			map[string]string{
+				"āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāā": "āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāā",
+			},
+			true,
+		},
+		{
+			"invalid international value too long, should error",
+			map[string]string{
+				"āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāā": "āāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāāā",
+			},
+			true,
+		},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.Name, func(t *testing.T) {
+			cfg, tmp := testConfig(t)
+			defer os.RemoveAll(tmp)
+
+			cfg["image_labels"] = tc.Labels
+			var c Config
+			_, errs := c.Prepare(cfg)
+
+			if tc.ExpectError && errs == nil {
+				t.Errorf("expected errors with labels, got none")
+			}
+			if !tc.ExpectError && errs != nil {
+				t.Errorf("expected no error, got some: %+v", errs)
+			}
+		})
+	}
+}
+
 // Helper stuff below
 
 func testConfig(t *testing.T) (config map[string]interface{}, tempAccountFile string) {

builder/googlecompute/step_create_image.go

@@ -7,7 +7,6 @@ import (
 	"context"
 	"errors"
 	"fmt"
-	"strings"
 	"time"
 
 	"github.com/hashicorp/packer-plugin-googlecompute/lib/common"
@@ -47,18 +46,6 @@ func (s *StepCreateImage) Run(ctx context.Context, state multistep.StateBag) mul
 		}
 	}
 
-	if err := config.AreLabelsValid(); err != nil {
-		var sb strings.Builder
-		sb.WriteString(fmt.Sprintf("Image: %s has invalid labels: ", config.ImageName))
-		for _, e := range err {
-			sb.WriteString(fmt.Sprintf("%s, ", e))
-		}
-		err := fmt.Errorf(sb.String())
-		state.Put("error", err)
-		ui.Error(err.Error())
-		return multistep.ActionHalt
-	}
-
 	ui.Say("Creating image...")
 
 	sourceDiskURI := fmt.Sprintf("/compute/v1/projects/%s/zones/%s/disks/%s", config.ProjectId, config.Zone, config.imageSourceDisk)

builder/googlecompute/step_create_image_test.go

@@ -69,45 +69,3 @@ func TestStepCreateImage_errorOnChannel(t *testing.T) {
 	_, ok = state.GetOk("image_name")
 	assert.False(t, ok, "State should not have a resulting image.")
 }
-
-func TestStepCreateImage_invalidLabels(t *testing.T) {
-	state := testState(t)
-	step := new(StepCreateImage)
-	defer step.Cleanup(state)
-
-	c := state.Get("config").(*Config)
-	c.ImageLabels = map[string]string{
-		"Invalid Label": "value",
-	}
-
-	// run the step
-	action := step.Run(context.Background(), state)
-	assert.Equal(t, action, multistep.ActionHalt, "Step should not have passed with invalid labels.")
-	uncastErr, ok := state.GetOk("error")
-	assert.True(t, ok, "State should have an error due to invalid labels.")
-	err, ok := uncastErr.(error)
-	assert.True(t, ok, "Error in state is not an error type.")
-	assert.Contains(t, err.Error(), "key \"Invalid Label\" must match regex", "Error message does not contain expected text.")
-	assert.True(t, ok, "State should have an error due to invalid labels.")
-	_, ok = state.GetOk("image")
-	assert.False(t, ok, "State should not have a resulting image due to invalid labels.")
-}
-
-func TestStepCreateImage_emptyLabelKey(t *testing.T) {
-	state := testState(t)
-	step := new(StepCreateImage)
-	defer step.Cleanup(state)
-
-	c := state.Get("config").(*Config)
-	c.ImageLabels = map[string]string{
-		"": "value",
-	}
-
-	// run the step
-	action := step.Run(context.Background(), state)
-	assert.Equal(t, action, multistep.ActionHalt, "Step should not have passed with invalid labels.")
-	_, ok := state.GetOk("error")
-	assert.True(t, ok, "State should have an error due to invalid labels.")
-	_, ok = state.GetOk("image")
-	assert.False(t, ok, "State should not have a resulting image due to invalid labels.")
-}