From bade6478fcdd537957f0ce8a2cc5e06a14940ea0 Mon Sep 17 00:00:00 2001 From: Matt Farina Date: Fri, 15 May 2020 13:23:10 -0400 Subject: [PATCH 1/4] Fixing error with strvals parsing Closes #8140 Signed-off-by: Matt Farina --- pkg/strvals/parser.go | 30 +++++++++++++++++++++------- pkg/strvals/parser_test.go | 41 +++++++++++++++++++++++++++++++++++--- 2 files changed, 61 insertions(+), 10 deletions(-) diff --git a/pkg/strvals/parser.go b/pkg/strvals/parser.go index 03adbd3cb..db2fb60fe 100644 --- a/pkg/strvals/parser.go +++ b/pkg/strvals/parser.go @@ -17,6 +17,7 @@ package strvals import ( "bytes" + "fmt" "io" "strconv" "strings" @@ -230,14 +231,17 @@ func set(data map[string]interface{}, key string, val interface{}) { data[key] = val } -func setIndex(list []interface{}, index int, val interface{}) []interface{} { +func setIndex(list []interface{}, index int, val interface{}) ([]interface{}, error) { + if index < 0 { + return list, fmt.Errorf("negative %d index not allowed", index) + } if len(list) <= index { newlist := make([]interface{}, index+1) copy(newlist, list) list = newlist } list[index] = val - return list + return list, nil } func (t *parser) keyIndex() (int, error) { @@ -252,6 +256,9 @@ func (t *parser) keyIndex() (int, error) { } func (t *parser) listItem(list []interface{}, i int) ([]interface{}, error) { + if i < 0 { + return list, fmt.Errorf("negative %d index not allowed", i) + } stop := runeSet([]rune{'[', '.', '='}) switch k, last, err := runesUntil(t.sc, stop); { case len(k) > 0: @@ -262,16 +269,19 @@ func (t *parser) listItem(list []interface{}, i int) ([]interface{}, error) { vl, e := t.valList() switch e { case nil: - return setIndex(list, i, vl), nil + return setIndex(list, i, vl) case io.EOF: - return setIndex(list, i, ""), err + return setIndex(list, i, "") case ErrNotList: rs, e := t.val() if e != nil && e != io.EOF { return list, e } v, e := t.reader(rs) - return setIndex(list, i, v), e + if e != nil { + return list, e + } + return setIndex(list, i, v) default: return list, e } @@ -283,7 +293,10 @@ func (t *parser) listItem(list []interface{}, i int) ([]interface{}, error) { } // Now we need to get the value after the ]. list2, err := t.listItem(list, i) - return setIndex(list, i, list2), err + if err != nil { + return list, err + } + return setIndex(list, i, list2) case last == '.': // We have a nested object. Send to t.key inner := map[string]interface{}{} @@ -299,7 +312,10 @@ func (t *parser) listItem(list []interface{}, i int) ([]interface{}, error) { // Recurse e := t.key(inner) - return setIndex(list, i, inner), e + if e != nil { + return list, e + } + return setIndex(list, i, inner) default: return nil, errors.Errorf("parse error: unexpected token %v", last) } diff --git a/pkg/strvals/parser_test.go b/pkg/strvals/parser_test.go index 44d317220..fb18980cf 100644 --- a/pkg/strvals/parser_test.go +++ b/pkg/strvals/parser_test.go @@ -28,6 +28,7 @@ func TestSetIndex(t *testing.T) { expect []interface{} add int val int + err bool }{ { name: "short", @@ -35,6 +36,7 @@ func TestSetIndex(t *testing.T) { expect: []interface{}{0, 1, 2}, add: 2, val: 2, + err: false, }, { name: "equal", @@ -42,6 +44,7 @@ func TestSetIndex(t *testing.T) { expect: []interface{}{0, 2}, add: 1, val: 2, + err: false, }, { name: "long", @@ -49,17 +52,41 @@ func TestSetIndex(t *testing.T) { expect: []interface{}{0, 1, 2, 4, 4, 5}, add: 3, val: 4, + err: false, + }, + { + name: "negative", + initial: []interface{}{0, 1, 2, 3, 4, 5}, + expect: []interface{}{0, 1, 2, 3, 4, 5}, + add: -1, + val: 4, + err: true, }, } for _, tt := range tests { - got := setIndex(tt.initial, tt.add, tt.val) + got, err := setIndex(tt.initial, tt.add, tt.val) + + if err != nil && tt.err == false { + t.Fatalf("%s: Expected no error but error returned", tt.name) + } else if err == nil && tt.err == true { + t.Fatalf("%s: Expected error but no error returned", tt.name) + } + if len(got) != len(tt.expect) { t.Fatalf("%s: Expected length %d, got %d", tt.name, len(tt.expect), len(got)) } - if gg := got[tt.add].(int); gg != tt.val { - t.Errorf("%s, Expected value %d, got %d", tt.name, tt.val, gg) + if !tt.err { + if gg := got[tt.add].(int); gg != tt.val { + t.Errorf("%s, Expected value %d, got %d", tt.name, tt.val, gg) + } + } + + for k, v := range got { + if v != tt.expect[k] { + t.Errorf("%s, Expected value %d, got %d", tt.name, tt.expect[k], v) + } } } } @@ -271,6 +298,10 @@ func TestParseSet(t *testing.T) { }, }, }, + { + str: "list[0].foo=bar,list[-30].hello=world", + err: true, + }, { str: "list[0]=foo,list[1]=bar", expect: map[string]interface{}{"list": []string{"foo", "bar"}}, @@ -283,6 +314,10 @@ func TestParseSet(t *testing.T) { str: "list[0]=foo,list[3]=bar", expect: map[string]interface{}{"list": []interface{}{"foo", nil, nil, "bar"}}, }, + { + str: "list[0]=foo,list[-20]=bar", + err: true, + }, { str: "illegal[0]name.foo=bar", err: true, From 8f1f0e0db2d96fa3d85c8b7a8e461f6bec7bced9 Mon Sep 17 00:00:00 2001 From: Matt Farina Date: Fri, 15 May 2020 16:00:57 -0400 Subject: [PATCH 2/4] Recovering from panic that can occur with make Signed-off-by: Matt Farina --- pkg/strvals/parser.go | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/pkg/strvals/parser.go b/pkg/strvals/parser.go index db2fb60fe..c52f38ab1 100644 --- a/pkg/strvals/parser.go +++ b/pkg/strvals/parser.go @@ -231,7 +231,16 @@ func set(data map[string]interface{}, key string, val interface{}) { data[key] = val } -func setIndex(list []interface{}, index int, val interface{}) ([]interface{}, error) { +func setIndex(list []interface{}, index int, val interface{}) (l2 []interface{}, err error) { + // There are possible index values that are out of range on a target system + // causing a panic. This will catch the panic and return an error instead. + // The value of the index that causes a panic varies from system to system. + defer func() { + if r := recover(); r != nil { + err = fmt.Errorf("error processing index %d: %s", index, r) + } + }() + if index < 0 { return list, fmt.Errorf("negative %d index not allowed", index) } From 6857da251edd416454827ac692c85de1ade6b5e6 Mon Sep 17 00:00:00 2001 From: Matt Farina Date: Fri, 15 May 2020 16:52:04 -0400 Subject: [PATCH 3/4] Catching a potential panic in strval parsing Signed-off-by: Matt Farina --- pkg/strvals/parser.go | 7 ++++++- pkg/strvals/parser_test.go | 4 ++++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/pkg/strvals/parser.go b/pkg/strvals/parser.go index c52f38ab1..c735412e9 100644 --- a/pkg/strvals/parser.go +++ b/pkg/strvals/parser.go @@ -150,7 +150,12 @@ func runeSet(r []rune) map[rune]bool { return s } -func (t *parser) key(data map[string]interface{}) error { +func (t *parser) key(data map[string]interface{}) (reterr error) { + defer func() { + if r := recover(); r != nil { + reterr = fmt.Errorf("unable to parse key: %s", r) + } + }() stop := runeSet([]rune{'=', '[', ',', '.'}) for { switch k, last, err := runesUntil(t.sc, stop); { diff --git a/pkg/strvals/parser_test.go b/pkg/strvals/parser_test.go index fb18980cf..742256153 100644 --- a/pkg/strvals/parser_test.go +++ b/pkg/strvals/parser_test.go @@ -362,6 +362,10 @@ func TestParseSet(t *testing.T) { }, }, }, + { + str: "]={}].", + err: true, + }, } for _, tt := range tests { From 1a46c3495ab2aa101e31fe9dfbe10802bb7d0a63 Mon Sep 17 00:00:00 2001 From: Alan Zhu Date: Mon, 18 May 2020 09:25:19 +0800 Subject: [PATCH 4/4] Revert "group command for easy read" Signed-off-by: Alan Zhu --- cmd/helm/root.go | 63 ++++++++++++++++++------------------------------ 1 file changed, 23 insertions(+), 40 deletions(-) diff --git a/cmd/helm/root.go b/cmd/helm/root.go index ff4a01ad7..143745f29 100644 --- a/cmd/helm/root.go +++ b/cmd/helm/root.go @@ -26,7 +26,6 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/tools/clientcmd" - "k8s.io/kubectl/pkg/util/templates" "helm.sh/helm/v3/internal/completion" "helm.sh/helm/v3/internal/experimental/registry" @@ -133,47 +132,31 @@ func newRootCmd(actionConfig *action.Configuration, out io.Writer, args []string flags.ParseErrorsWhitelist.UnknownFlags = true flags.Parse(args) - commandGroups := templates.CommandGroups{ - { - Message: "Release Management Commands:", - Commands: []*cobra.Command{ - newInstallCmd(actionConfig, out), - newListCmd(actionConfig, out), - newGetCmd(actionConfig, out), - newStatusCmd(actionConfig, out), - newUpgradeCmd(actionConfig, out), - newHistoryCmd(actionConfig, out), - newRollbackCmd(actionConfig, out), - newReleaseTestCmd(actionConfig, out), - newUninstallCmd(actionConfig, out), - }, - }, - { - Message: "Chart Commands:", - Commands: []*cobra.Command{ - newCreateCmd(out), - newDependencyCmd(out), - newPackageCmd(out), - newTemplateCmd(actionConfig, out), - newLintCmd(out), - newVerifyCmd(out), - }, - }, - { - Message: "Chart Repository Commands:", - Commands: []*cobra.Command{ - newRepoCmd(out), - newSearchCmd(out), - newPullCmd(out), - newShowCmd(out), - }, - }, - } - commandGroups.Add(cmd) - templates.ActsAsRootCommand(cmd, []string{"options"}, commandGroups...) - // Add subcommands cmd.AddCommand( + // chart commands + newCreateCmd(out), + newDependencyCmd(out), + newPullCmd(out), + newShowCmd(out), + newLintCmd(out), + newPackageCmd(out), + newRepoCmd(out), + newSearchCmd(out), + newVerifyCmd(out), + + // release commands + newGetCmd(actionConfig, out), + newHistoryCmd(actionConfig, out), + newInstallCmd(actionConfig, out), + newListCmd(actionConfig, out), + newReleaseTestCmd(actionConfig, out), + newRollbackCmd(actionConfig, out), + newStatusCmd(actionConfig, out), + newTemplateCmd(actionConfig, out), + newUninstallCmd(actionConfig, out), + newUpgradeCmd(actionConfig, out), + newCompletionCmd(out), newEnvCmd(out), newPluginCmd(out),