From d0f9008007738b293a52477a55baf825eb41701f Mon Sep 17 00:00:00 2001 From: Charlie Voiselle <464492+angrycub@users.noreply.github.com> Date: Mon, 13 Feb 2023 16:14:59 -0500 Subject: [PATCH] Add warnings to `var put` for non-alphanumeric keys. (#15933) * Warn when Items key isn't directly accessible Go template requires that map keys are alphanumeric for direct access using the dotted reference syntax. This warns users when they create keys that run afoul of this requirement. - cli: use regex to detect invalid indentifiers in var keys - test: fix slash in escape test case - api: share warning formatting function between API and CLI - ui: warn if var key has characters other than _, letter, or number --------- Co-authored-by: Charlie Voiselle <464492+angrycub@users.noreply.github.com> Co-authored-by: Luiz Aoqui --- .changelog/15933.txt | 7 ++ command/job_validate.go | 30 +---- command/meta.go | 10 ++ command/var_put.go | 49 ++++++++ command/var_put_test.go | 116 ++++++++++++++++++ helper/warning.go | 42 +++++++ helper/warning_test.go | 54 ++++++++ nomad/job_endpoint.go | 10 +- nomad/structs/funcs.go | 27 ---- nomad/structs/funcs_test.go | 22 ---- ui/app/components/variable-form.js | 16 ++- .../components/variable-form-test.js | 55 +++++++-- 12 files changed, 345 insertions(+), 93 deletions(-) create mode 100644 .changelog/15933.txt create mode 100644 helper/warning.go create mode 100644 helper/warning_test.go diff --git a/.changelog/15933.txt b/.changelog/15933.txt new file mode 100644 index 000000000..fec161b08 --- /dev/null +++ b/.changelog/15933.txt @@ -0,0 +1,7 @@ +```release-note:improvement +cli: Warn when variable key includes characters that require the use of the `index` function in templates +``` + +```release-note:improvement +ui: Warn when variable key includes characters that require the use of the `index` function in templates +``` diff --git a/command/job_validate.go b/command/job_validate.go index 85031a1b0..8f234793a 100644 --- a/command/job_validate.go +++ b/command/job_validate.go @@ -8,6 +8,7 @@ import ( "github.com/hashicorp/go-multierror" "github.com/hashicorp/nomad/api" "github.com/hashicorp/nomad/command/agent" + "github.com/hashicorp/nomad/helper" "github.com/hashicorp/nomad/helper/pointer" "github.com/posener/complete" ) @@ -196,8 +197,7 @@ func (c *JobValidateCommand) Run(args []string) int { // Print any warnings if there are any if jr.Warnings != "" { - c.Ui.Output( - c.Colorize().Color(fmt.Sprintf("[bold][yellow]Job Warnings:\n%s[reset]\n", jr.Warnings))) + c.Ui.Output(c.FormatWarnings("Job", jr.Warnings)) } // Done! @@ -225,30 +225,6 @@ func (c *JobValidateCommand) validateLocal(aj *api.Job) (*api.JobValidateRespons } } - out.Warnings = mergeMultierrorWarnings(job.Warnings()) + out.Warnings = helper.MergeMultierrorWarnings(job.Warnings()) return &out, nil } - -func mergeMultierrorWarnings(errs ...error) string { - if len(errs) == 0 { - return "" - } - - var mErr multierror.Error - _ = multierror.Append(&mErr, errs...) - - mErr.ErrorFormat = warningsFormatter - - return mErr.Error() -} - -func warningsFormatter(es []error) string { - sb := strings.Builder{} - sb.WriteString(fmt.Sprintf("%d warning(s):\n", len(es))) - - for i := range es { - sb.WriteString(fmt.Sprintf("\n* %s", es[i])) - } - - return sb.String() -} diff --git a/command/meta.go b/command/meta.go index 736dfcfa2..aa2b505cb 100644 --- a/command/meta.go +++ b/command/meta.go @@ -2,6 +2,7 @@ package command import ( "flag" + "fmt" "os" "strings" @@ -216,6 +217,15 @@ func (m *Meta) SetupUi(args []string) { } } +// FormatWarnings returns a string with the warnings formatted for CLI output. +func (m *Meta) FormatWarnings(header string, warnings string) string { + return m.Colorize().Color( + fmt.Sprintf("[bold][yellow]%s Warnings:\n%s[reset]\n", + header, + warnings, + )) +} + type usageOptsFlags uint8 const ( diff --git a/command/var_put.go b/command/var_put.go index fb19417d7..b979d6a47 100644 --- a/command/var_put.go +++ b/command/var_put.go @@ -7,8 +7,11 @@ import ( "io" "os" "path/filepath" + "regexp" "strings" + multierror "github.com/hashicorp/go-multierror" + "github.com/hashicorp/go-set" "github.com/hashicorp/hcl" "github.com/hashicorp/hcl/hcl/ast" "github.com/hashicorp/nomad/api" @@ -16,8 +19,13 @@ import ( "github.com/mitchellh/cli" "github.com/mitchellh/mapstructure" "github.com/posener/complete" + "golang.org/x/exp/slices" ) +// Detect characters that are not valid identifiers to emit a warning when they +// are used in as a variable key. +var invalidIdentifier = regexp.MustCompile(`[^_\pN\pL]`) + type VarPutCommand struct { Meta @@ -270,6 +278,7 @@ func (c *VarPutCommand) Run(args []string) int { return 1 } + var warnings *multierror.Error if len(args) > 0 { data, err := parseArgsData(stdin, args) if err != nil { @@ -288,6 +297,9 @@ func (c *VarPutCommand) Run(args []string) int { } continue } + if err := warnInvalidIdentifier(k); err != nil { + warnings = multierror.Append(warnings, err) + } sv.Items[k] = vs } } @@ -318,6 +330,13 @@ func (c *VarPutCommand) Run(args []string) int { successMsg := fmt.Sprintf( "Created variable %q with modify index %v", sv.Path, sv.ModifyIndex) + if warnings != nil { + c.Ui.Warn(c.FormatWarnings( + "Variable", + helper.MergeMultierrorWarnings(warnings), + )) + } + var out string switch c.outFmt { case "json": @@ -525,3 +544,33 @@ func (c *VarPutCommand) validateOutputFlag() error { return errors.New(errInvalidOutFormat) } } + +func warnInvalidIdentifier(in string) error { + invalid := invalidIdentifier.FindAllString(in, -1) + if len(invalid) == 0 { + return nil + } + + // Use %s instead of %q to avoid escaping characters. + return fmt.Errorf( + `"%s" contains characters %s that require the 'index' function for direct access in templates`, + in, + formatInvalidVarKeyChars(invalid), + ) +} + +func formatInvalidVarKeyChars(invalid []string) string { + // Deduplicate characters + chars := set.From(invalid) + + // Sort the characters for output + charList := make([]string, 0, chars.Size()) + for _, k := range chars.List() { + // Use %s instead of %q to avoid escaping characters. + charList = append(charList, fmt.Sprintf(`"%s"`, k)) + } + slices.Sort(charList) + + // Build string + return fmt.Sprintf("[%s]", strings.Join(charList, ",")) +} diff --git a/command/var_put_test.go b/command/var_put_test.go index c5bcbb9ad..8daf14728 100644 --- a/command/var_put_test.go +++ b/command/var_put_test.go @@ -2,6 +2,8 @@ package command import ( "encoding/json" + "fmt" + "regexp" "strings" "testing" @@ -9,6 +11,7 @@ import ( "github.com/hashicorp/nomad/ci" "github.com/mitchellh/cli" "github.com/posener/complete" + "github.com/shoenig/test/must" "github.com/stretchr/testify/require" ) @@ -121,3 +124,116 @@ func TestVarPutCommand_AutocompleteArgs(t *testing.T) { require.Equal(t, 1, len(res)) require.Equal(t, sv.Path, res[0]) } + +func TestVarPutCommand_KeyWarning(t *testing.T) { + // Extract invalid characters from warning message. + r := regexp.MustCompile(`contains characters \[(.*)\]`) + + tcs := []struct { + name string + goodKeys []string + badKeys []string + badChars []string + }{ + { + name: "simple", + goodKeys: []string{"simple"}, + }, + { + name: "hasDot", + badKeys: []string{"has.Dot"}, + badChars: []string{`"."`}, + }, + { + name: "unicode_letters", + goodKeys: []string{"世界"}, + }, + { + name: "unicode_numbers", + goodKeys: []string{"٣٢١"}, + }, + { + name: "two_good", + goodKeys: []string{"aardvark", "beagle"}, + }, + { + name: "one_good_one_bad", + goodKeys: []string{"aardvark"}, + badKeys: []string{"bad.key"}, + badChars: []string{`"."`}, + }, + { + name: "one_good_two_bad", + goodKeys: []string{"aardvark"}, + badKeys: []string{"bad.key", "also-bad"}, + badChars: []string{`"."`, `"-"`}, + }, + { + name: "repeated_bad_char", + goodKeys: []string{"aardvark"}, + badKeys: []string{"bad.key", "also.bad"}, + badChars: []string{`"."`, `"."`}, + }, + { + name: "repeated_bad_char_same_key", + goodKeys: []string{"aardvark"}, + badKeys: []string{"bad.key."}, + badChars: []string{`"."`}, + }, + { + name: "dont_escape", + goodKeys: []string{"aardvark"}, + badKeys: []string{"bad\\key"}, + badChars: []string{`"\"`}, + }, + } + + ci.Parallel(t) + _, _, url := testServer(t, false, nil) + + for _, tc := range tcs { + t.Run(tc.name, func(t *testing.T) { + tc := tc // capture test case + ci.Parallel(t) // make the subtests parallel + + keys := append(tc.goodKeys, tc.badKeys...) // combine keys into a single slice + for i, k := range keys { + keys[i] = k + "=value" // Make each key into a k=v pair; value is not part of test + } + + ui := cli.NewMockUi() + cmd := &VarPutCommand{Meta: Meta{Ui: ui}} + args := append([]string{"-address=" + url, "-force", "-out=json", "test/var"}, keys...) + code := cmd.Run(args) + errOut := ui.ErrorWriter.String() + + must.Eq(t, 0, code) // the command should always succeed + + badKeysLen := len(tc.badKeys) + switch badKeysLen { + case 0: + must.Eq(t, "", errOut) // cases with no bad keys shouldn't put anything to stderr + return + case 1: + must.StrContains(t, errOut, "1 warning:") // header should be singular + default: + must.StrContains(t, errOut, fmt.Sprintf("%d warnings:", badKeysLen)) // header should be plural + } + + for _, k := range tc.badKeys { + must.StrContains(t, errOut, k) // every bad key should appear in the warning output + } + + if len(tc.badChars) > 0 { + invalid := r.FindAllStringSubmatch(errOut, -1) + for i, k := range tc.badChars { + must.Eq(t, invalid[i][1], k) // every bad char should appear in the warning output + } + } + + for _, k := range tc.goodKeys { + must.StrNotContains(t, errOut, k) // good keys should not be emitted + } + }) + } +} diff --git a/helper/warning.go b/helper/warning.go new file mode 100644 index 000000000..b5733794b --- /dev/null +++ b/helper/warning.go @@ -0,0 +1,42 @@ +package helper + +import ( + "fmt" + "strings" + + "github.com/hashicorp/go-multierror" +) + +// MergeMultierrorWarnings takes warnings and merges them into a returnable +// string. This method is used to return API and CLI errors. +func MergeMultierrorWarnings(errs ...error) string { + if len(errs) == 0 { + return "" + } + + var mErr multierror.Error + _ = multierror.Append(&mErr, errs...) + mErr.ErrorFormat = warningsFormatter + + return mErr.Error() +} + +// warningsFormatter is used to format warnings. +func warningsFormatter(es []error) string { + sb := strings.Builder{} + + switch len(es) { + case 0: + return "" + case 1: + sb.WriteString("1 warning:\n") + default: + sb.WriteString(fmt.Sprintf("%d warnings:\n", len(es))) + } + + for _, err := range es { + sb.WriteString(fmt.Sprintf("\n* %s", strings.TrimSpace(err.Error()))) + } + + return sb.String() +} diff --git a/helper/warning_test.go b/helper/warning_test.go new file mode 100644 index 000000000..703ede350 --- /dev/null +++ b/helper/warning_test.go @@ -0,0 +1,54 @@ +package helper + +import ( + "errors" + "strings" + "testing" + + "github.com/hashicorp/nomad/ci" + "github.com/shoenig/test/must" +) + +func TestMergeMultierrorWarnings(t *testing.T) { + ci.Parallel(t) + + testCases := []struct { + name string + errs []error + expected string + }{ + { + name: "no warning", + errs: []error{}, + expected: "", + }, + { + name: "single warning", + errs: []error{ + errors.New("warning"), + }, + expected: ` +1 warning: + +* warning`, + }, + { + name: "multiple warnings", + errs: []error{ + errors.New("warning 1"), + errors.New("warning 2"), + }, + expected: ` +2 warnings: + +* warning 1 +* warning 2`, + }, + } + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + got := MergeMultierrorWarnings(tc.errs...) + must.Eq(t, got, strings.TrimSpace(tc.expected)) + }) + } +} diff --git a/nomad/job_endpoint.go b/nomad/job_endpoint.go index 1376d9c77..3626cee24 100644 --- a/nomad/job_endpoint.go +++ b/nomad/job_endpoint.go @@ -122,7 +122,7 @@ func (j *Job) Register(args *structs.JobRegisterRequest, reply *structs.JobRegis } // Set the warning message - reply.Warnings = structs.MergeMultierrorWarnings(warnings...) + reply.Warnings = helper.MergeMultierrorWarnings(warnings...) // Check job submission permissions aclObj, err := j.srv.ResolveACL(args) @@ -292,7 +292,7 @@ func (j *Job) Register(args *structs.JobRegisterRequest, reply *structs.JobRegis } if policyWarnings != nil { warnings = append(warnings, policyWarnings) - reply.Warnings = structs.MergeMultierrorWarnings(warnings...) + reply.Warnings = helper.MergeMultierrorWarnings(warnings...) } // Clear the Vault token @@ -586,7 +586,7 @@ func (j *Job) Validate(args *structs.JobValidateRequest, reply *structs.JobValid validateWarnings = append(validateWarnings, mutateWarnings...) // Set the warning message - reply.Warnings = structs.MergeMultierrorWarnings(validateWarnings...) + reply.Warnings = helper.MergeMultierrorWarnings(validateWarnings...) reply.DriverConfigValidated = true return nil } @@ -1711,7 +1711,7 @@ func (j *Job) Plan(args *structs.JobPlanRequest, reply *structs.JobPlanResponse) args.Job = job // Set the warning message - reply.Warnings = structs.MergeMultierrorWarnings(warnings...) + reply.Warnings = helper.MergeMultierrorWarnings(warnings...) // Check job submission permissions, which we assume is the same for plan if aclObj, err := j.srv.ResolveACL(args); err != nil { @@ -1749,7 +1749,7 @@ func (j *Job) Plan(args *structs.JobPlanRequest, reply *structs.JobPlanResponse) } if policyWarnings != nil { warnings = append(warnings, policyWarnings) - reply.Warnings = structs.MergeMultierrorWarnings(warnings...) + reply.Warnings = helper.MergeMultierrorWarnings(warnings...) } // Interpolate the job for this region diff --git a/nomad/structs/funcs.go b/nomad/structs/funcs.go index 89c6d84b2..9e497925a 100644 --- a/nomad/structs/funcs.go +++ b/nomad/structs/funcs.go @@ -10,38 +10,11 @@ import ( "strconv" "strings" - "github.com/hashicorp/go-multierror" "github.com/hashicorp/go-set" "github.com/hashicorp/nomad/acl" "golang.org/x/crypto/blake2b" ) -// MergeMultierrorWarnings takes job warnings and canonicalize warnings and -// merges them into a returnable string. Both the errors may be nil. -func MergeMultierrorWarnings(errs ...error) string { - if len(errs) == 0 { - return "" - } - - var mErr multierror.Error - _ = multierror.Append(&mErr, errs...) - mErr.ErrorFormat = warningsFormatter - - return mErr.Error() -} - -// warningsFormatter is used to format job warnings -func warningsFormatter(es []error) string { - sb := strings.Builder{} - sb.WriteString(fmt.Sprintf("%d warning(s):\n", len(es))) - - for i := range es { - sb.WriteString(fmt.Sprintf("\n* %s", es[i])) - } - - return sb.String() -} - // RemoveAllocs is used to remove any allocs with the given IDs // from the list of allocations func RemoveAllocs(allocs []*Allocation, remove []*Allocation) []*Allocation { diff --git a/nomad/structs/funcs_test.go b/nomad/structs/funcs_test.go index ec4cbe139..a1a612edd 100644 --- a/nomad/structs/funcs_test.go +++ b/nomad/structs/funcs_test.go @@ -2,7 +2,6 @@ package structs import ( "encoding/base64" - "errors" "fmt" "testing" @@ -1062,27 +1061,6 @@ func TestGenerateMigrateToken(t *testing.T) { assert.True(CompareMigrateToken("x", nodeSecret, token2)) } -func TestMergeMultierrorWarnings(t *testing.T) { - ci.Parallel(t) - - var errs []error - - // empty - str := MergeMultierrorWarnings(errs...) - require.Equal(t, "", str) - - // non-empty - errs = []error{ - errors.New("foo"), - nil, - errors.New("bar"), - } - - str = MergeMultierrorWarnings(errs...) - - require.Equal(t, "2 warning(s):\n\n* foo\n* bar", str) -} - func TestVaultPoliciesSet(t *testing.T) { input := map[string]map[string]*Vault{ "tg1": { diff --git a/ui/app/components/variable-form.js b/ui/app/components/variable-form.js index 62373fd2a..0ac54bf8b 100644 --- a/ui/app/components/variable-form.js +++ b/ui/app/components/variable-form.js @@ -20,6 +20,9 @@ const EMPTY_KV = { warnings: EmberObject.create(), }; +// Capture characters that are not _, letters, or numbers using Unicode. +const invalidKeyCharactersRegex = new RegExp(/[^_\p{Letter}\p{Number}]/gu); + export default class VariableFormComponent extends Component { @service flashMessages; @service router; @@ -146,9 +149,16 @@ export default class VariableFormComponent extends Component { @action validateKey(entry, e) { const value = e.target.value; - // No dots in key names - if (value.includes('.')) { - entry.warnings.set('dottedKeyError', 'Key should not contain a period.'); + // Only letters, numbers, and _ are allowed in keys + const invalidChars = value.match(invalidKeyCharactersRegex); + if (invalidChars) { + const invalidCharsOuput = [...new Set(invalidChars)] + .sort() + .map((c) => `'${c}'`); + entry.warnings.set( + 'dottedKeyError', + `${value} contains characters [${invalidCharsOuput}] that require the "index" function for direct access in templates.` + ); } else { delete entry.warnings.dottedKeyError; entry.warnings.notifyPropertyChange('dottedKeyError'); diff --git a/ui/tests/integration/components/variable-form-test.js b/ui/tests/integration/components/variable-form-test.js index 627b2cf1a..329942b55 100644 --- a/ui/tests/integration/components/variable-form-test.js +++ b/ui/tests/integration/components/variable-form-test.js @@ -280,15 +280,52 @@ module('Integration | Component | variable-form', function (hooks) { }) ); - await render(hbs``); - - await typeIn('.key-value label:nth-child(1) input', 'superSecret'); - assert.dom('.key-value-error').doesNotExist(); - - find('.key-value label:nth-child(1) input').value = ''; - - await typeIn('.key-value label:nth-child(1) input', 'super.secret'); - assert.dom('.key-value-error').exists(); + const testCases = [ + { + name: 'valid key', + key: 'superSecret2', + warn: false, + }, + { + name: 'invalid key with dot', + key: 'super.secret', + warn: true, + }, + { + name: 'invalid key with slash', + key: 'super/secret', + warn: true, + }, + { + name: 'invalid key with emoji', + key: 'supersecretspy🕵️', + warn: true, + }, + { + name: 'unicode letters', + key: '世界', + warn: false, + }, + { + name: 'unicode numbers', + key: '٣٢١', + warn: false, + }, + { + name: 'unicode letters and numbers', + key: '世٢界١', + warn: false, + }, + ]; + for (const tc of testCases) { + await render(hbs``); + await typeIn('[data-test-var-key]', tc.key); + if (tc.warn) { + assert.dom('.key-value-error').exists(tc.name); + } else { + assert.dom('.key-value-error').doesNotExist(tc.name); + } + } }); test('warns you when you create a duplicate key', async function (assert) {