From 5fbac75bd5f7137005b8ff882c39867a320848de Mon Sep 17 00:00:00 2001 From: Lucas Bajolet Date: Thu, 13 Feb 2025 16:23:44 -0500 Subject: [PATCH] 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 | 18 +++-- builder/googlecompute/config_test.go | 69 +++++++++++++++++++ builder/googlecompute/step_create_image.go | 13 ---- .../googlecompute/step_create_image_test.go | 42 ----------- 4 files changed, 82 insertions(+), 60 deletions(-) diff --git a/builder/googlecompute/config.go b/builder/googlecompute/config.go index 4b216e42..ae600e63 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,9 +663,11 @@ func ApplyIAPTunnel(c *communicator.Config, port int) error { } } +var labelKeyRegex = regexp.MustCompile(`^[a-z][a-z0-9_-]{0,62}$`) +var labelValueRegex = regexp.MustCompile(`^[a-z0-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)) @@ -670,11 +678,11 @@ func (c *Config) AreLabelsValid() []error { 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 72c3354f..b8d96f6f 100644 --- a/builder/googlecompute/config_test.go +++ b/builder/googlecompute/config_test.go @@ -712,6 +712,75 @@ func TestConfigPrepareImageArchitecture(t *testing.T) { } } +func TestLabelsValidity(t *testing.T) { + cases := []struct { + Name string + Labels map[string]string + ExpectError bool + }{ + { + "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, + }, + { + "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, + }, + } + + 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 e728f1be..d9ad454c 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 4e289ca5..ba249baa 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.") -}