From e9fa36f9ef3ffb57f9929117a6f7208131bbd2df Mon Sep 17 00:00:00 2001 From: Mahmood Ali Date: Tue, 30 Oct 2018 13:24:30 -0400 Subject: [PATCH 1/2] Allow artifacts checksum interpolation Fixes https://github.com/hashicorp/nomad/issues/4814 --- helper/args/args.go | 5 ++ helper/args/args_test.go | 27 +++++++++++ nomad/structs/structs.go | 87 ++++++++++++++++++++--------------- nomad/structs/structs_test.go | 9 ++++ 4 files changed, 91 insertions(+), 37 deletions(-) diff --git a/helper/args/args.go b/helper/args/args.go index 86f43f1f62e..fd78c377000 100644 --- a/helper/args/args.go +++ b/helper/args/args.go @@ -26,3 +26,8 @@ func ReplaceEnv(arg string, environments ...map[string]string) string { func ReplaceEnvWithPlaceHolder(arg string, placeholder string) string { return envRe.ReplaceAllString(arg, placeholder) } + +// ContainsEnv takes an arg and returns true if if contains an environment variable reference +func ContainsEnv(arg string) bool { + return envRe.MatchString(arg) +} diff --git a/helper/args/args_test.go b/helper/args/args_test.go index 974f4cbf39e..82956028ce8 100644 --- a/helper/args/args_test.go +++ b/helper/args/args_test.go @@ -75,3 +75,30 @@ func TestArgs_ReplaceEnv_Chained(t *testing.T) { t.Fatalf("ReplaceEnv(%v, %v) returned %#v; want %#v", input, envVars, act, exp) } } + +func TestArgs_ContainsEnv(t *testing.T) { + positiveCases := []string{ + "test-${env_var}", + } + for _, c := range positiveCases { + t.Run(fmt.Sprintf("positive case: %v", c), func(t *testing.T) { + if !ContainsEnv(c) { + t.Fatalf("ContainsEnv(%v) returned false; want true", c) + } + }) + } + + negativeCases := []string{ + "test", + "test-$", + "test-${asdf", + } + for _, c := range negativeCases { + t.Run(fmt.Sprintf("positive case: %v", c), func(t *testing.T) { + if ContainsEnv(c) { + t.Fatalf("ContainsEnv(%v) returned true; want false", c) + } + }) + } + +} diff --git a/nomad/structs/structs.go b/nomad/structs/structs.go index f26df1d5c35..2bdb235b25b 100644 --- a/nomad/structs/structs.go +++ b/nomad/structs/structs.go @@ -6155,49 +6155,62 @@ func (ta *TaskArtifact) Validate() error { } // Verify the checksum - if check, ok := ta.GetterOptions["checksum"]; ok { - check = strings.TrimSpace(check) - if check == "" { - mErr.Errors = append(mErr.Errors, fmt.Errorf("checksum value cannot be empty")) - return mErr.ErrorOrNil() - } + if err := ta.validateChecksum(); err != nil { + mErr.Errors = append(mErr.Errors, err) + } - parts := strings.Split(check, ":") - if l := len(parts); l != 2 { - mErr.Errors = append(mErr.Errors, fmt.Errorf(`checksum must be given as "type:value"; got %q`, check)) - return mErr.ErrorOrNil() - } + return mErr.ErrorOrNil() +} - checksumVal := parts[1] - checksumBytes, err := hex.DecodeString(checksumVal) - if err != nil { - mErr.Errors = append(mErr.Errors, fmt.Errorf("invalid checksum: %v", err)) - return mErr.ErrorOrNil() - } +func (ta *TaskArtifact) validateChecksum() error { + check, ok := ta.GetterOptions["checksum"] + if !ok { + return nil + } - checksumType := parts[0] - expectedLength := 0 - switch checksumType { - case "md5": - expectedLength = md5.Size - case "sha1": - expectedLength = sha1.Size - case "sha256": - expectedLength = sha256.Size - case "sha512": - expectedLength = sha512.Size - default: - mErr.Errors = append(mErr.Errors, fmt.Errorf("unsupported checksum type: %s", checksumType)) - return mErr.ErrorOrNil() - } + // job struct validation occurs before interpolation resolution can be effective + // skip checking if checksum contain variable reference, and artifacts fetching will + // eventually fail at pick up time if checksum is indeed invalid + if args.ContainsEnv(check) { + return nil + } - if len(checksumBytes) != expectedLength { - mErr.Errors = append(mErr.Errors, fmt.Errorf("invalid %s checksum: %v", checksumType, checksumVal)) - return mErr.ErrorOrNil() - } + check = strings.TrimSpace(check) + if check == "" { + return fmt.Errorf("checksum value cannot be empty") } - return mErr.ErrorOrNil() + parts := strings.Split(check, ":") + if l := len(parts); l != 2 { + return fmt.Errorf(`checksum must be given as "type:value"; got %q`, check) + } + + checksumVal := parts[1] + checksumBytes, err := hex.DecodeString(checksumVal) + if err != nil { + return fmt.Errorf("invalid checksum: %v", err) + } + + checksumType := parts[0] + expectedLength := 0 + switch checksumType { + case "md5": + expectedLength = md5.Size + case "sha1": + expectedLength = sha1.Size + case "sha256": + expectedLength = sha256.Size + case "sha512": + expectedLength = sha512.Size + default: + return fmt.Errorf("unsupported checksum type: %s", checksumType) + } + + if len(checksumBytes) != expectedLength { + return fmt.Errorf("invalid %s checksum: %v", checksumType, checksumVal) + } + + return nil } const ( diff --git a/nomad/structs/structs_test.go b/nomad/structs/structs_test.go index 88bebcbe880..e6e9d09eba9 100644 --- a/nomad/structs/structs_test.go +++ b/nomad/structs/structs_test.go @@ -2672,6 +2672,15 @@ func TestTaskArtifact_Validate_Checksum(t *testing.T) { }, true, }, + { + &TaskArtifact{ + GetterSource: "foo.com", + GetterOptions: map[string]string{ + "checksum": "md5:${ARTIFACT_CHECKSUM}", + }, + }, + false, + }, } for i, tc := range cases { From abd2451d527ee66428db986dc9fa709f41793c69 Mon Sep 17 00:00:00 2001 From: Mahmood Ali Date: Tue, 30 Oct 2018 13:58:52 -0400 Subject: [PATCH 2/2] address review comments --- helper/args/args_test.go | 4 +++- nomad/structs/structs.go | 7 +++---- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/helper/args/args_test.go b/helper/args/args_test.go index 82956028ce8..5b8d91b8264 100644 --- a/helper/args/args_test.go +++ b/helper/args/args_test.go @@ -92,9 +92,11 @@ func TestArgs_ContainsEnv(t *testing.T) { "test", "test-$", "test-${asdf", + "test-{asdf}", + "$test", } for _, c := range negativeCases { - t.Run(fmt.Sprintf("positive case: %v", c), func(t *testing.T) { + t.Run(fmt.Sprintf("negative case: %v", c), func(t *testing.T) { if ContainsEnv(c) { t.Fatalf("ContainsEnv(%v) returned true; want false", c) } diff --git a/nomad/structs/structs.go b/nomad/structs/structs.go index 2bdb235b25b..1f269c4c062 100644 --- a/nomad/structs/structs.go +++ b/nomad/structs/structs.go @@ -6154,7 +6154,6 @@ func (ta *TaskArtifact) Validate() error { mErr.Errors = append(mErr.Errors, fmt.Errorf("destination escapes allocation directory")) } - // Verify the checksum if err := ta.validateChecksum(); err != nil { mErr.Errors = append(mErr.Errors, err) } @@ -6168,9 +6167,9 @@ func (ta *TaskArtifact) validateChecksum() error { return nil } - // job struct validation occurs before interpolation resolution can be effective - // skip checking if checksum contain variable reference, and artifacts fetching will - // eventually fail at pick up time if checksum is indeed invalid + // Job struct validation occurs before interpolation resolution can be effective. + // Skip checking if checksum contain variable reference, and artifacts fetching will + // eventually fail, if checksum is indeed invalid. if args.ContainsEnv(check) { return nil }