From 722b0131f0136501d56d6e27b93cfe514d4a79c8 Mon Sep 17 00:00:00 2001 From: Natasha Sarkar Date: Wed, 3 Mar 2021 12:03:36 -0800 Subject: [PATCH] return error for duplicate keys rather than panicking --- api/builtins/NamespaceTransformer.go | 8 +++++-- api/ifc/ifc.go | 2 +- .../accumulator/resaccumulator_test.go | 3 ++- api/internal/target/multitransformer.go | 6 ++++- api/internal/wrappy/factory_test.go | 9 ++++--- api/internal/wrappy/wnode.go | 2 +- api/internal/wrappy/wnode_test.go | 7 ++++-- api/resmap/merginator.go | 7 +++++- api/resmap/reswrangler.go | 24 +++++++++++++++---- api/resource/factory.go | 6 ++++- api/resource/resource.go | 13 ++++++---- kustomize/commands/build/writer.go | 6 ++++- kyaml/yaml/rnode.go | 9 +++---- kyaml/yaml/rnode_test.go | 7 ++++-- .../NamespaceTransformer.go | 8 +++++-- 15 files changed, 86 insertions(+), 31 deletions(-) diff --git a/api/builtins/NamespaceTransformer.go b/api/builtins/NamespaceTransformer.go index f97800f84..0ed2796a4 100644 --- a/api/builtins/NamespaceTransformer.go +++ b/api/builtins/NamespaceTransformer.go @@ -30,12 +30,16 @@ func (p *NamespaceTransformerPlugin) Transform(m resmap.ResMap) error { return nil } for _, r := range m.Resources() { - if r.IsEmpty() { + empty, err := r.IsEmpty() + if err != nil { + return err + } + if empty { // Don't mutate empty objects? continue } r.StorePreviousId() - err := r.ApplyFilter(namespace.Filter{ + err = r.ApplyFilter(namespace.Filter{ Namespace: p.Namespace, FsSlice: p.FieldSpecs, }) diff --git a/api/ifc/ifc.go b/api/ifc/ifc.go index d13043e3a..ee1ce0338 100644 --- a/api/ifc/ifc.go +++ b/api/ifc/ifc.go @@ -76,7 +76,7 @@ type Kunstructured interface { GetString(string) (string, error) // Several uses. - Map() map[string]interface{} + Map() (map[string]interface{}, error) // Used by Resource.AsYAML and Resource.String MarshalJSON() ([]byte, error) diff --git a/api/internal/accumulator/resaccumulator_test.go b/api/internal/accumulator/resaccumulator_test.go index e10972298..26000ff66 100644 --- a/api/internal/accumulator/resaccumulator_test.go +++ b/api/internal/accumulator/resaccumulator_test.go @@ -399,7 +399,8 @@ func find(name string, resMap resmap.ResMap) *resource.Resource { func getCommand(r *resource.Resource) string { var m map[string]interface{} var c []interface{} - m, _ = r.Map()["spec"].(map[string]interface{}) + resourceMap, _ := r.Map() + m, _ = resourceMap["spec"].(map[string]interface{}) m, _ = m["template"].(map[string]interface{}) m, _ = m["spec"].(map[string]interface{}) c, _ = m["containers"].([]interface{}) diff --git a/api/internal/target/multitransformer.go b/api/internal/target/multitransformer.go index 98d6378d5..1896bdf04 100644 --- a/api/internal/target/multitransformer.go +++ b/api/internal/target/multitransformer.go @@ -43,7 +43,11 @@ func (o *multiTransformer) transform(m resmap.ResMap) error { } } for _, r := range m.Resources() { - if r.IsEmpty() { + empty, err := r.IsEmpty() + if err != nil { + return err + } + if empty { err := m.Remove(r.CurId()) if err != nil { return err diff --git a/api/internal/wrappy/factory_test.go b/api/internal/wrappy/factory_test.go index 6d96aed1a..cbc73c565 100644 --- a/api/internal/wrappy/factory_test.go +++ b/api/internal/wrappy/factory_test.go @@ -306,13 +306,16 @@ items: assert.False(t, tc.exp.isErr) assert.Equal(t, len(tc.exp.out), len(rs)) for i := range rs { + rsMap, err := rs[i].Map() + assert.NoError(t, err) assert.Equal( - t, fmt.Sprintf("%v", tc.exp.out[i]), fmt.Sprintf("%v", rs[i].Map())) + t, fmt.Sprintf("%v", tc.exp.out[i]), fmt.Sprintf("%v", rsMap)) if n != "listWithAnchors" { // https://github.com/kubernetes-sigs/kustomize/issues/3271 - if !reflect.DeepEqual(tc.exp.out[i], rs[i].Map()) { + m, _ := rs[i].Map() + if !reflect.DeepEqual(tc.exp.out[i], m) { t.Fatalf("%s:\nexpected: %v\n actual: %v", - n, tc.exp.out[i], rs[i].Map()) + n, tc.exp.out[i], m) } } } diff --git a/api/internal/wrappy/wnode.go b/api/internal/wrappy/wnode.go index 3459f0302..81a35d174 100644 --- a/api/internal/wrappy/wnode.go +++ b/api/internal/wrappy/wnode.go @@ -220,7 +220,7 @@ func (wn *WNode) GetString(path string) (string, error) { } // Map implements ifc.Kunstructured. -func (wn *WNode) Map() map[string]interface{} { +func (wn *WNode) Map() (map[string]interface{}, error) { return wn.node.Map() } diff --git a/api/internal/wrappy/wnode_test.go b/api/internal/wrappy/wnode_test.go index 692fefa7e..c23f8d430 100644 --- a/api/internal/wrappy/wnode_test.go +++ b/api/internal/wrappy/wnode_test.go @@ -559,7 +559,9 @@ func TestGetSlice(t *testing.T) { } func TestMapEmpty(t *testing.T) { - assert.Equal(t, 0, len(NewWNode().Map())) + newNodeMap, err := NewWNode().Map() + assert.NoError(t, err) + assert.Equal(t, 0, len(newNodeMap)) } func TestMap(t *testing.T) { @@ -577,7 +579,8 @@ func TestMap(t *testing.T) { }, } - actual := wn.Map() + actual, err := wn.Map() + assert.NoError(t, err) if diff := cmp.Diff(expected, actual); diff != "" { t.Fatalf("actual map does not deep equal expected map:\n%v", diff) } diff --git a/api/resmap/merginator.go b/api/resmap/merginator.go index 12979063b..c09b5aacb 100644 --- a/api/resmap/merginator.go +++ b/api/resmap/merginator.go @@ -90,9 +90,14 @@ func (m *merginator) makeError(cd resource.ConflictDetector, index int) error { if conflict == nil { return fmt.Errorf("expected conflict for %s", m.incoming[index].OrgId()) } + conflictMap, _ := conflict.Map() + incomingIndexMap, _ := m.incoming[index].Map() return fmt.Errorf( "conflict between %#v at index %d and %#v", - m.incoming[index].Map(), index, conflict.Map()) + incomingIndexMap, + index, + conflictMap, + ) } // findConflict looks for a conflict in a resource slice. diff --git a/api/resmap/reswrangler.go b/api/resmap/reswrangler.go index e803488d5..f76276c48 100644 --- a/api/resmap/reswrangler.go +++ b/api/resmap/reswrangler.go @@ -119,7 +119,11 @@ func (m *resWrangler) Debug(title string) { fmt.Println("---") } fmt.Printf("# %d %s\n", i, r.OrgId()) - blob, err := yaml.Marshal(r.Map()) + m, err := r.Map() + if err != nil { + panic(err) + } + blob, err := yaml.Marshal(m) if err != nil { panic(err) } @@ -272,7 +276,8 @@ func (m *resWrangler) AsYaml() ([]byte, error) { for _, res := range m.Resources() { out, err := res.AsYAML() if err != nil { - return nil, errors.Wrapf(err, "%#v", res.Map()) + m, _ := res.Map() + return nil, errors.Wrapf(err, "%#v", m) } if firstObj { firstObj = false @@ -602,14 +607,23 @@ func (m *resWrangler) ApplySmPatch( // Some unknown error, let it through. return err } - if !res.IsEmpty() { + empty, err := res.IsEmpty() + if err != nil { + return err + } + if !empty { + m, _ := res.Map() return errors.Wrapf( err, "with unexpectedly non-empty object map of size %d", - len(res.Map())) + len(m)) } // Fall through to handle deleted object. } - if !res.IsEmpty() { + empty, err := res.IsEmpty() + if err != nil { + return err + } + if !empty { // IsEmpty means all fields have been removed from the object. // This can happen if a patch required deletion of the // entire resource (not just a part of it). This means diff --git a/api/resource/factory.go b/api/resource/factory.go index 017b04b36..df08c5dbf 100644 --- a/api/resource/factory.go +++ b/api/resource/factory.go @@ -114,7 +114,11 @@ func (rf *Factory) SliceFromBytes(in []byte) ([]*Resource, error) { u := kunStructs[0] kunStructs = kunStructs[1:] if strings.HasSuffix(u.GetKind(), "List") { - items := u.Map()["items"] + m, err := u.Map() + if err != nil { + return nil, err + } + items := m["items"] itemsSlice, ok := items.([]interface{}) if !ok { if items == nil { diff --git a/api/resource/resource.go b/api/resource/resource.go index 5d545e780..d9f2b107b 100644 --- a/api/resource/resource.go +++ b/api/resource/resource.go @@ -98,11 +98,12 @@ func (r *Resource) GetString(p string) (string, error) { return r.kunStr.GetString(p) } -func (r *Resource) IsEmpty() bool { - return len(r.kunStr.Map()) == 0 +func (r *Resource) IsEmpty() (bool, error) { + m, err := r.kunStr.Map() + return len(m) == 0, err } -func (r *Resource) Map() map[string]interface{} { +func (r *Resource) Map() (map[string]interface{}, error) { return r.kunStr.Map() } @@ -488,7 +489,11 @@ func (r *Resource) ApplySmPatch(patch *Resource) error { if err != nil { return err } - if !r.IsEmpty() { + empty, err := r.IsEmpty() + if err != nil { + return err + } + if !empty { r.SetName(n) r.SetNamespace(ns) } diff --git a/kustomize/commands/build/writer.go b/kustomize/commands/build/writer.go index 0ead0ddac..89e4c2201 100644 --- a/kustomize/commands/build/writer.go +++ b/kustomize/commands/build/writer.go @@ -46,7 +46,11 @@ func (w Writer) WriteIndividualFiles(dirPath string, m resmap.ResMap) error { } func (w Writer) write(path, fName string, res *resource.Resource) error { - yml, err := yaml.Marshal(res.Map()) + m, err := res.Map() + if err != nil { + return err + } + yml, err := yaml.Marshal(m) if err != nil { return err } diff --git a/kyaml/yaml/rnode.go b/kyaml/yaml/rnode.go index d317bb5c4..22e5d326e 100644 --- a/kyaml/yaml/rnode.go +++ b/kyaml/yaml/rnode.go @@ -816,17 +816,18 @@ func FromMap(m map[string]interface{}) (*RNode, error) { return Parse(string(c)) } -func (rn *RNode) Map() map[string]interface{} { +func (rn *RNode) Map() (map[string]interface{}, error) { if rn == nil || rn.value == nil { - return make(map[string]interface{}) + return make(map[string]interface{}), nil } var result map[string]interface{} if err := rn.value.Decode(&result); err != nil { // Should not be able to create an RNode that cannot be decoded; // this is an unrecoverable error. - log.Fatalf("failed to decode ynode: %v", err) + str, _ := rn.String() + return nil, fmt.Errorf("received error %w for the following resource:\n%s", err, str) } - return result + return result, nil } // ConvertJSONToYamlNode parses input json string and returns equivalent yaml node diff --git a/kyaml/yaml/rnode_test.go b/kyaml/yaml/rnode_test.go index 57f0276ed..c14e61939 100644 --- a/kyaml/yaml/rnode_test.go +++ b/kyaml/yaml/rnode_test.go @@ -386,7 +386,9 @@ func TestRNodeGetValidatedMetadata(t *testing.T) { } func TestRNodeMapEmpty(t *testing.T) { - assert.Equal(t, 0, len(NewRNode(nil).Map())) + newRNodeMap, err := NewRNode(nil).Map() + assert.NoError(t, err) + assert.Equal(t, 0, len(newRNodeMap)) } func TestRNodeMap(t *testing.T) { @@ -411,7 +413,8 @@ func TestRNodeMap(t *testing.T) { }, } - actual := wn.Map() + actual, err := wn.Map() + assert.NoError(t, err) if diff := cmp.Diff(expected, actual); diff != "" { t.Fatalf("actual map does not deep equal expected map:\n%v", diff) } diff --git a/plugin/builtin/namespacetransformer/NamespaceTransformer.go b/plugin/builtin/namespacetransformer/NamespaceTransformer.go index 38d1e4d79..6d42db29d 100644 --- a/plugin/builtin/namespacetransformer/NamespaceTransformer.go +++ b/plugin/builtin/namespacetransformer/NamespaceTransformer.go @@ -34,12 +34,16 @@ func (p *plugin) Transform(m resmap.ResMap) error { return nil } for _, r := range m.Resources() { - if r.IsEmpty() { + empty, err := r.IsEmpty() + if err != nil { + return err + } + if empty { // Don't mutate empty objects? continue } r.StorePreviousId() - err := r.ApplyFilter(namespace.Filter{ + err = r.ApplyFilter(namespace.Filter{ Namespace: p.Namespace, FsSlice: p.FieldSpecs, })