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) {