From 6bfd7cff72b2a8da3e51029606e1497391a4300b Mon Sep 17 00:00:00 2001 From: jregan Date: Sun, 24 Feb 2019 10:17:11 -0800 Subject: [PATCH] Improve error handling during var resolution. --- pkg/commands/build/build_test.go | 6 ++ pkg/commands/edit/add/flagsandargs.go | 2 +- pkg/expansion/expand.go | 1 - pkg/expansion/expand_test.go | 4 +- pkg/resmap/resmap.go | 9 -- pkg/resmap/resmap_test.go | 29 +++---- pkg/target/resaccumulator.go | 25 ++++-- pkg/target/resaccumulator_test.go | 118 ++++++++++++++++++++++++-- 8 files changed, 150 insertions(+), 44 deletions(-) diff --git a/pkg/commands/build/build_test.go b/pkg/commands/build/build_test.go index 169de1ad4d..b06ccdea2a 100644 --- a/pkg/commands/build/build_test.go +++ b/pkg/commands/build/build_test.go @@ -22,6 +22,12 @@ import ( "sigs.k8s.io/kustomize/pkg/constants" ) +func TestNewOptionsToSilenceCodeInspectionError(t *testing.T) { + if NewOptions("foo", "bar") == nil { + t.Fatal("could not make new options") + } +} + func TestBuildValidate(t *testing.T) { var cases = []struct { name string diff --git a/pkg/commands/edit/add/flagsandargs.go b/pkg/commands/edit/add/flagsandargs.go index a842ed8171..1cfc7d8dc2 100644 --- a/pkg/commands/edit/add/flagsandargs.go +++ b/pkg/commands/edit/add/flagsandargs.go @@ -94,7 +94,7 @@ func (a *flagsAndArgs) ExpandFileSource(fSys fs.FileSystem) error { if key != "" { if len(result) != 1 { return fmt.Errorf( - "'pattern '%s' catches files %v, should catch only one.", pattern, result) + "'pattern '%s' catches files %v, should catch only one", pattern, result) } fileSource := fmt.Sprintf("%s=%s", key, result[0]) results = append(results, fileSource) diff --git a/pkg/expansion/expand.go b/pkg/expansion/expand.go index c2a81ed6f2..c4f0e0ba35 100644 --- a/pkg/expansion/expand.go +++ b/pkg/expansion/expand.go @@ -44,7 +44,6 @@ func MappingFuncFor(context ...map[string]string) func(string) string { return val } } - return syntaxWrap(input) } } diff --git a/pkg/expansion/expand_test.go b/pkg/expansion/expand_test.go index e418e33a1d..4b6a982117 100644 --- a/pkg/expansion/expand_test.go +++ b/pkg/expansion/expand_test.go @@ -46,9 +46,7 @@ func TestMapReference(t *testing.T) { "BLU": "$(ZOO)-2", } - serviceEnv := map[string]string{} - - mapping := MappingFuncFor(declaredEnv, serviceEnv) + mapping := MappingFuncFor(declaredEnv) for _, env := range envs { declaredEnv[env.Name] = Expand(env.Value, mapping) diff --git a/pkg/resmap/resmap.go b/pkg/resmap/resmap.go index bb6dec478a..ca1e723980 100644 --- a/pkg/resmap/resmap.go +++ b/pkg/resmap/resmap.go @@ -46,15 +46,6 @@ func (m ResMap) GetMatchingIds(matches IdMatcher) []resid.ResId { return result } -// DemandOneGvknMatchForId find the matched resource by Group/Version/Kind and Name -func (m ResMap) DemandOneGvknMatchForId(inputId resid.ResId) (*resource.Resource, bool) { - result := m.GetMatchingIds(inputId.GvknEquals) - if len(result) == 1 { - return m[result[0]], true - } - return nil, false -} - // EncodeAsYaml encodes a ResMap to YAML; encoded objects separated by `---`. func (m ResMap) EncodeAsYaml() ([]byte, error) { var ids []resid.ResId diff --git a/pkg/resmap/resmap_test.go b/pkg/resmap/resmap_test.go index 74a43cdd6e..05341cb035 100644 --- a/pkg/resmap/resmap_test.go +++ b/pkg/resmap/resmap_test.go @@ -92,35 +92,34 @@ func TestDemandOneGvknMatchForId(t *testing.T) { }), } - _, ok := rm1.DemandOneGvknMatchForId( - resid.NewResIdWithPrefixNamespace(cmap, "cm2", "prefix1", "ns1")) - if !ok { - t.Fatal("Expected single map entry but got none") + result := rm1.GetMatchingIds( + resid.NewResIdWithPrefixNamespace(cmap, "cm2", "prefix1", "ns1").GvknEquals) + if len(result) != 1 { + t.Fatalf("Expected single map entry but got %v", result) } // confirm that ns and prefix are not included in match - _, ok = rm1.DemandOneGvknMatchForId( - resid.NewResIdWithPrefixNamespace(cmap, "cm2", "prefix", "ns")) - if !ok { - t.Fatal("Expected single map entry but got none") + result = rm1.GetMatchingIds( + resid.NewResIdWithPrefixNamespace(cmap, "cm2", "prefix", "ns").GvknEquals) + if len(result) != 1 { + t.Fatalf("Expected single map entry but got %v", result) } // confirm that name is matched correctly - result, ok := rm1.DemandOneGvknMatchForId( - resid.NewResIdWithPrefixNamespace(cmap, "cm3", "prefix1", "ns1")) - if ok { + result = rm1.GetMatchingIds( + resid.NewResIdWithPrefixNamespace(cmap, "cm3", "prefix1", "ns1").GvknEquals) + if len(result) > 0 { t.Fatalf("Expected no map entries but got %v", result) } cmap2 := gvk.Gvk{Version: "v2", Kind: "ConfigMap"} // confirm that gvk is matched correctly - result, ok = rm1.DemandOneGvknMatchForId( - resid.NewResIdWithPrefixNamespace(cmap2, "cm2", "prefix1", "ns1")) - if ok { + result = rm1.GetMatchingIds( + resid.NewResIdWithPrefixNamespace(cmap2, "cm2", "prefix1", "ns1").GvknEquals) + if len(result) > 0 { t.Fatalf("Expected no map entries but got %v", result) } - } func TestFilterBy(t *testing.T) { diff --git a/pkg/target/resaccumulator.go b/pkg/target/resaccumulator.go index c6ba0cd84e..7894a8fb29 100644 --- a/pkg/target/resaccumulator.go +++ b/pkg/target/resaccumulator.go @@ -18,7 +18,6 @@ package target import ( "fmt" - "log" "sigs.k8s.io/kustomize/pkg/resid" "sigs.k8s.io/kustomize/pkg/resmap" "sigs.k8s.io/kustomize/pkg/transformers" @@ -101,17 +100,27 @@ func (ra *ResAccumulator) MergeAccumulator(other *ResAccumulator) (err error) { // for substitution wherever the $(var.Name) occurs. func (ra *ResAccumulator) makeVarReplacementMap() (map[string]string, error) { result := map[string]string{} - for _, v := range ra.varSet.Set() { - id := resid.NewResId(v.ObjRef.GVK(), v.ObjRef.Name) - if r, found := ra.resMap.DemandOneGvknMatchForId(id); found { - s, err := r.GetFieldValue(v.FieldRef.FieldPath) + for _, v := range ra.Vars() { + matched := ra.resMap.GetMatchingIds( + resid.NewResId(v.ObjRef.GVK(), v.ObjRef.Name).GvknEquals) + if len(matched) > 1 { + return nil, fmt.Errorf( + "found %d resId matches for var %s "+ + "(unable to disambiguate)", + len(matched), v) + } + if len(matched) == 1 { + s, err := ra.resMap[matched[0]].GetFieldValue(v.FieldRef.FieldPath) if err != nil { - return nil, fmt.Errorf("field path err for var: %+v", v) + return nil, fmt.Errorf( + "field specified in var '%v' "+ + "not found in corresponding resource", v) } result[v.Name] = s } else { - // Should this be an error? - log.Printf("var %v defined but not used", v) + return nil, fmt.Errorf( + "var '%v' cannot be mapped to a field "+ + "in the set of known resources", v) } } return result, nil diff --git a/pkg/target/resaccumulator_test.go b/pkg/target/resaccumulator_test.go index 0bf1eedaab..55dab77f6c 100644 --- a/pkg/target/resaccumulator_test.go +++ b/pkg/target/resaccumulator_test.go @@ -30,11 +30,11 @@ import ( "sigs.k8s.io/kustomize/pkg/types" ) -func TestResolveVars(t *testing.T) { +func makeResAccumulator() (*ResAccumulator, *resource.Factory, error) { ra := MakeEmptyAccumulator() err := ra.MergeConfig(config.MakeDefaultConfig()) if err != nil { - t.Fatalf("unexpected err: %v", err) + return nil, nil, err } rf := resource.NewFactory( kunstruct.NewKunstructuredFactoryImpl()) @@ -55,8 +55,8 @@ func TestResolveVars(t *testing.T) { map[string]interface{}{ "command": []interface{}{ "myserver", - "--somebackendService $(FOO)", - "--yetAnother $(BAR)", + "--somebackendService $(SERVICE_ONE)", + "--yetAnother $(SERVICE_TWO)", }, }, }, @@ -85,14 +85,23 @@ func TestResolveVars(t *testing.T) { }, }), }) + return ra, rf, nil +} + +func TestResolveVarsHappy(t *testing.T) { + ra, _, err := makeResAccumulator() + if err != nil { + t.Fatalf("unexpected err: %v", err) + } err = ra.MergeVars([]types.Var{ { - Name: "FOO", + Name: "SERVICE_ONE", ObjRef: types.Target{ Gvk: gvk.Gvk{Version: "v1", Kind: "Service"}, Name: "backendOne"}, - }, { - Name: "BAR", + }, + { + Name: "SERVICE_TWO", ObjRef: types.Target{ Gvk: gvk.Gvk{Version: "v1", Kind: "Service"}, Name: "backendTwo"}, @@ -111,6 +120,101 @@ func TestResolveVars(t *testing.T) { } } +func TestResolveVarsVarNeedsDisambiguation(t *testing.T) { + ra, rf, err := makeResAccumulator() + if err != nil { + t.Fatalf("unexpected err: %v", err) + } + ra.MergeResourcesWithErrorOnIdCollision(resmap.ResMap{ + resid.NewResIdWithPrefixNamespace( + gvk.Gvk{Version: "v1", Kind: "Service"}, + "backendOne", "", "fooNamespace"): rf.FromMap( + map[string]interface{}{ + "apiVersion": "v1", + "kind": "Service", + "metadata": map[string]interface{}{ + "name": "backendOne", + }, + }), + }) + + err = ra.MergeVars([]types.Var{ + { + Name: "SERVICE_ONE", + ObjRef: types.Target{ + Gvk: gvk.Gvk{Version: "v1", Kind: "Service"}, + Name: "backendOne", + }, + }, + }) + if err != nil { + t.Fatalf("unexpected err: %v", err) + } + err = ra.ResolveVars() + if err == nil { + t.Fatalf("expected error") + } + if !strings.Contains( + err.Error(), "unable to disambiguate") { + t.Fatalf("unexpected err: %v", err) + } +} + +func TestResolveVarsGoodResIdBadField(t *testing.T) { + ra, _, err := makeResAccumulator() + if err != nil { + t.Fatalf("unexpected err: %v", err) + } + err = ra.MergeVars([]types.Var{ + { + Name: "SERVICE_ONE", + ObjRef: types.Target{ + Gvk: gvk.Gvk{Version: "v1", Kind: "Service"}, + Name: "backendOne"}, + FieldRef: types.FieldSelector{FieldPath: "nope_nope_nope"}, + }, + }) + if err != nil { + t.Fatalf("unexpected err: %v", err) + } + err = ra.ResolveVars() + if err == nil { + t.Fatalf("expected error") + } + if !strings.Contains( + err.Error(), + "not found in corresponding resource") { + t.Fatalf("unexpected err: %v", err) + } +} + +func TestResolveVarsUnmappableVar(t *testing.T) { + ra, _, err := makeResAccumulator() + if err != nil { + t.Fatalf("unexpected err: %v", err) + } + err = ra.MergeVars([]types.Var{ + { + Name: "SERVICE_THREE", + ObjRef: types.Target{ + Gvk: gvk.Gvk{Version: "v1", Kind: "Service"}, + Name: "doesNotExist"}, + }, + }) + if err != nil { + t.Fatalf("unexpected err: %v", err) + } + err = ra.ResolveVars() + if err == nil { + t.Fatalf("expected error") + } + if !strings.Contains( + err.Error(), + "cannot be mapped to a field in the set of known resources") { + t.Fatalf("unexpected err: %v", err) + } +} + func find(name string, resMap resmap.ResMap) *resource.Resource { for k, v := range resMap { if k.Name() == name {