luxolus
/
open-nomad
2
0
Fork 0
Code Issues 1 Pull Requests Packages Releases Wiki Activity

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 <luiz@hashicorp.com>
This commit is contained in:
Charlie Voiselle 2023-02-13 16:14:59 -05:00 committed by GitHub
parent 490c902c62
commit d93ba0cf32
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
12 changed files with 345 additions and 93 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

7
.changelog/15933.txt Normal file
View File

@ -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
```

30
command/job_validate.go
View File

@ -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()
}

10
command/meta.go
View File

@ -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 (

49
command/var_put.go
View File

@ -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, ","))
}

116
command/var_put_test.go
View File

@ -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
}
})
}
}

42
helper/warning.go Normal file
View File

@ -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()
}

54
helper/warning_test.go Normal file
View File

@ -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))
})
}
}

10
nomad/job_endpoint.go
View File

@ -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

27
nomad/structs/funcs.go
View File

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

22
nomad/structs/funcs_test.go
View File

@ -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": {

16
ui/app/components/variable-form.js
View File

@ -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');

55
ui/tests/integration/components/variable-form-test.js
View File

@ -280,15 +280,52 @@ module('Integration | Component | variable-form', function (hooks) {
})
);
await render(hbs`<VariableForm @model={{this.mockedModel}} />`);
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`<VariableForm @model={{this.mockedModel}} />`);
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) {