diff --git a/builder/googlecompute/config.go b/builder/googlecompute/config.go index 4b216e4..c80e05c 100644 --- a/builder/googlecompute/config.go +++ b/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 diff --git a/builder/googlecompute/config_test.go b/builder/googlecompute/config_test.go index 72c3354..c9a2fa0 100644 --- a/builder/googlecompute/config_test.go +++ b/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) { diff --git a/builder/googlecompute/step_create_image.go b/builder/googlecompute/step_create_image.go index e728f1b..d9ad454 100644 --- a/builder/googlecompute/step_create_image.go +++ b/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) diff --git a/builder/googlecompute/step_create_image_test.go b/builder/googlecompute/step_create_image_test.go index 4e289ca..ba249ba 100644 --- a/builder/googlecompute/step_create_image_test.go +++ b/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.") -}