Merge pull request #3233 from monopole/sliceFromBytes

Implement WNodeFactory.SliceFromBytes

api/internal/wrappy/factory.go

@@ -4,26 +4,71 @@
 package wrappy
 
 import (
+	"bytes"
+	"fmt"
+
 	"sigs.k8s.io/kustomize/api/ifc"
+	"sigs.k8s.io/kustomize/api/konfig"
 	"sigs.k8s.io/kustomize/api/types"
+	"sigs.k8s.io/kustomize/kyaml/kio"
+	"sigs.k8s.io/kustomize/kyaml/yaml"
 )
 
 // WNodeFactory makes instances of WNode.
+//
 // These instances in turn adapt
 //   sigs.k8s.io/kustomize/kyaml/yaml.RNode
 // to implement ifc.Unstructured.
 // This factory is meant to implement ifc.KunstructuredFactory.
+//
+// This implementation should be thin, as both WNode and WNodeFactory must be
+// factored away (deleted) along with ifc.Kunstructured in favor of direct use
+// of RNode methods upon completion of
+// https://github.com/kubernetes-sigs/kustomize/issues/2506.
+//
+// See also api/krusty/internal/provider/depprovider.go
 type WNodeFactory struct {
 }
 
 var _ ifc.KunstructuredFactory = (*WNodeFactory)(nil)
 
 func (k *WNodeFactory) SliceFromBytes(bs []byte) ([]ifc.Kunstructured, error) {
-	panic("TODO(#WNodeFactory): implement SliceFromBytes")
+	r := kio.ByteReader{OmitReaderAnnotations: true}
+	r.Reader = bytes.NewBuffer(bs)
+	yamlRNodes, err := r.Read()
+	if err != nil {
+		return nil, err
+	}
+	var result []ifc.Kunstructured
+	for i := range yamlRNodes {
+		rn := yamlRNodes[i]
+		meta, err := rn.GetValidatedMetadata()
+		if err != nil {
+			return nil, err
+		}
+		if !shouldDropObject(meta) {
+			if foundNil, path := rn.HasNilEntryInList(); foundNil {
+				return nil, fmt.Errorf("empty item at %v in object %v", path, rn)
+			}
+			result = append(result, FromRNode(rn))
+		}
+	}
+	return result, nil
+}
+
+// shouldDropObject returns true if the resource should not be accumulated.
+func shouldDropObject(m yaml.ResourceMeta) bool {
+	_, y := m.ObjectMeta.Annotations[konfig.IgnoredByKustomizeResourceAnnotation]
+	return y
 }
 
 func (k *WNodeFactory) FromMap(m map[string]interface{}) ifc.Kunstructured {
-	panic("TODO(#WNodeFactory): implement FromMap")
+	rn, err := FromMap(m)
+	if err != nil {
+		// TODO(#WNodeFactory): handle or bubble error"
+		panic(err)
+	}
+	return rn
 }
 
 func (k *WNodeFactory) Hasher() ifc.KunstructuredHasher {

api/internal/wrappy/factory_test.go

@@ -2,3 +2,223 @@
 // SPDX-License-Identifier: Apache-2.0
 
 package wrappy
+
+import (
+	"fmt"
+	"reflect"
+	"testing"
+)
+
+func TestSliceFromBytes(t *testing.T) {
+	factory := &WNodeFactory{}
+	testConfigMap :=
+		map[string]interface{}{
+			"apiVersion": "v1",
+			"kind":       "ConfigMap",
+			"metadata": map[string]interface{}{
+				"name": "winnie",
+			},
+		}
+	testConfigMapList :=
+		map[string]interface{}{
+			"apiVersion": "v1",
+			"kind":       "ConfigMapList",
+			"items": []interface{}{
+				testConfigMap,
+				testConfigMap,
+			},
+		}
+
+	type expected struct {
+		out   []map[string]interface{}
+		isErr bool
+	}
+
+	testCases := map[string]struct {
+		input []byte
+		exp   expected
+	}{
+		"garbage": {
+			input: []byte("garbageIn: garbageOut"),
+			exp: expected{
+				isErr: true,
+			},
+		},
+		"noBytes": {
+			input: []byte{},
+			exp: expected{
+				out: []map[string]interface{}{},
+			},
+		},
+		"goodJson": {
+			input: []byte(`
+{"apiVersion":"v1","kind":"ConfigMap","metadata":{"name":"winnie"}}
+`),
+			exp: expected{
+				out: []map[string]interface{}{testConfigMap},
+			},
+		},
+		"goodYaml1": {
+			input: []byte(`
+apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: winnie
+`),
+			exp: expected{
+				out: []map[string]interface{}{testConfigMap},
+			},
+		},
+		"goodYaml2": {
+			input: []byte(`
+apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: winnie
+---
+apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: winnie
+`),
+			exp: expected{
+				out: []map[string]interface{}{testConfigMap, testConfigMap},
+			},
+		},
+		"localConfigYaml": {
+			input: []byte(`
+apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: winnie-skip
+  annotations:
+    # this annotation causes the Resource to be ignored by kustomize
+    config.kubernetes.io/local-config: ""
+---
+apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: winnie
+`),
+			exp: expected{
+				out: []map[string]interface{}{testConfigMap},
+			},
+		},
+		"garbageInOneOfTwoObjects": {
+			input: []byte(`
+apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: winnie
+---
+WOOOOOOOOOOOOOOOOOOOOOOOOT:  woot
+`),
+			exp: expected{
+				isErr: true,
+			},
+		},
+		"emptyObjects": {
+			input: []byte(`
+---
+#a comment
+
+---
+
+`),
+			exp: expected{
+				out: []map[string]interface{}{},
+			},
+		},
+		"Missing .metadata.name in object": {
+			input: []byte(`
+apiVersion: v1
+kind: Namespace
+metadata:
+  annotations:
+    foo: bar
+`),
+			exp: expected{
+				isErr: true,
+			},
+		},
+		"nil value in list": {
+			input: []byte(`
+apiVersion: builtin
+kind: ConfigMapGenerator
+metadata:
+  name: kube100-site
+	labels:
+	  app: web
+testList:
+- testA
+-
+`),
+			exp: expected{
+				isErr: true,
+			},
+		},
+		"List": {
+			input: []byte(`
+apiVersion: v1
+kind: List
+items:
+- apiVersion: v1
+  kind: ConfigMap
+  metadata:
+    name: winnie
+- apiVersion: v1
+  kind: ConfigMap
+  metadata:
+    name: winnie
+`),
+			exp: expected{
+				out: []map[string]interface{}{
+					testConfigMap,
+					testConfigMap},
+			},
+		},
+		"ConfigMapList": {
+			input: []byte(`
+apiVersion: v1
+kind: ConfigMapList
+items:
+- apiVersion: v1
+  kind: ConfigMap
+  metadata:
+    name: winnie
+- apiVersion: v1
+  kind: ConfigMap
+  metadata:
+    name: winnie
+`),
+			exp: expected{
+				out: []map[string]interface{}{testConfigMapList},
+			},
+		},
+	}
+
+	for n := range testCases {
+		tc := testCases[n]
+		t.Run(n, func(t *testing.T) {
+			rs, err := factory.SliceFromBytes(tc.input)
+			if tc.exp.isErr && err == nil {
+				t.Fatalf("%v: should return error", n)
+			}
+			if !tc.exp.isErr && err != nil {
+				t.Fatalf("%v: unexpected error: %s", n, err)
+			}
+			if len(tc.exp.out) != len(rs) {
+				fmt.Printf("%s: \nexpected:%v\nactual: %v\n",
+					n, tc.exp.out, rs)
+				t.Fatalf("%s: length mismatch; expected %d, actual %d",
+					n, len(tc.exp.out), len(rs))
+			}
+			for i := range rs {
+				if !reflect.DeepEqual(tc.exp.out[i], rs[i].Map()) {
+					t.Fatalf("%s: Got: %v\nexpected:%v",
+						n, rs[i].Map(), tc.exp.out[i])
+				}
+			}
+		})
+	}
+}

api/k8sdeps/kunstruct/factory.go

@@ -14,6 +14,7 @@ import (
 	"k8s.io/apimachinery/pkg/util/yaml"
 	"sigs.k8s.io/kustomize/api/ifc"
 	"sigs.k8s.io/kustomize/api/internal/k8sdeps/configmapandsecret"
+	"sigs.k8s.io/kustomize/api/konfig"
 	"sigs.k8s.io/kustomize/api/types"
 )
 
@@ -116,10 +117,6 @@ func (kf *KunstructuredFactoryImpl) validate(u unstructured.Unstructured) error
 	return nil
 }
 
-// nonKustomizableResourceAnnotation if set on a Resource will cause Kustomize to
-// ignore the Resource rather than Kustomize it.
-const ignoredByKustomizeResourceAnnotation = "config.kubernetes.io/local-config"
-
 // skipResource returns true if the Resource should not be accumulated
 func (kf *KunstructuredFactoryImpl) skipResource(u unstructured.Unstructured) bool {
 	an := u.GetAnnotations()
@@ -128,7 +125,7 @@ func (kf *KunstructuredFactoryImpl) skipResource(u unstructured.Unstructured) bo
 		return false
 	}
 	// check if the Resource has opt-ed out of kustomize
-	_, found := an[ignoredByKustomizeResourceAnnotation]
+	_, found := an[konfig.IgnoredByKustomizeResourceAnnotation]
 	return found
 }
 

api/konfig/general.go

@@ -31,6 +31,9 @@ const (
 	// A program name, for use in help, finding the XDG_CONFIG_DIR, etc.
 	ProgramName = "kustomize"
 
+	// If a resource has this annotation, kustomize will drop it.
+	IgnoredByKustomizeResourceAnnotation = "config.kubernetes.io/local-config"
+
 	// Label key that indicates the resources are built from Kustomize
 	ManagedbyLabelKey = "app.kubernetes.io/managed-by"
 

kyaml/yaml/rnode.go

@@ -7,6 +7,8 @@ import (
 	"encoding/json"
 	"fmt"
 	"io/ioutil"
+	"strconv"
+	"strings"
 
 	"gopkg.in/yaml.v3"
 	"sigs.k8s.io/kustomize/kyaml/errors"
@@ -458,8 +460,8 @@ func (rn *RNode) Elements() ([]*RNode, error) {
 	return elements, nil
 }
 
-// ElementValues returns a list of all observed values for a given field name in a
-// list of elements.
+// ElementValues returns a list of all observed values for a given field name
+// in a list of elements.
 // Returns error for non-SequenceNodes.
 func (rn *RNode) ElementValues(key string) ([]string, error) {
 	if err := ErrorIfInvalid(rn, yaml.SequenceNode); err != nil {
@@ -602,6 +604,55 @@ func (rn *RNode) UnmarshalJSON(b []byte) error {
 	return nil
 }
 
+// GetValidatedMetadata returns metadata after subjecting it to some tests.
+func (rn *RNode) GetValidatedMetadata() (ResourceMeta, error) {
+	m, err := rn.GetMeta()
+	if err != nil {
+		return m, err
+	}
+	if m.Kind == "" {
+		return m, fmt.Errorf("missing kind in object %v", m)
+	}
+	if strings.HasSuffix(m.Kind, "List") {
+		// A list doesn't require a name.
+		return m, nil
+	}
+	if m.NameMeta.Name == "" {
+		return m, fmt.Errorf("missing metadata.name in object %v", m)
+	}
+	return m, nil
+}
+
+// HasNilEntryInList returns true if the RNode contains a list which has
+// a nil item, along with the path to the missing item.
+// TODO(broken): This was copied from
+// api/k8sdeps/kunstruct/factory.go//checkListItemNil
+// and doesn't do what it claims to do (see TODO in unit test and pr 1513).
+func (rn *RNode) HasNilEntryInList() (bool, string) {
+	return hasNilEntryInList(rn.value)
+}
+
+func hasNilEntryInList(in interface{}) (bool, string) {
+	switch v := in.(type) {
+	case map[string]interface{}:
+		for key, s := range v {
+			if result, path := hasNilEntryInList(s); result {
+				return result, key + "/" + path
+			}
+		}
+	case []interface{}:
+		for index, s := range v {
+			if s == nil {
+				return true, ""
+			}
+			if result, path := hasNilEntryInList(s); result {
+				return result, "[" + strconv.Itoa(index) + "]/" + path
+			}
+		}
+	}
+	return false, ""
+}
+
 func FromMap(m map[string]interface{}) (*RNode, error) {
 	c, err := Marshal(m)
 	if err != nil {

kyaml/yaml/rnode_test.go

@@ -11,6 +11,212 @@ import (
 	"github.com/stretchr/testify/assert"
 )
 
+func TestRNodeHasNilEntryInList(t *testing.T) {
+	testConfigMap := map[string]interface{}{
+		"apiVersion": "v1",
+		"kind":       "ConfigMap",
+		"metadata": map[string]interface{}{
+			"name": "winnie",
+		},
+	}
+	type resultExpected struct {
+		hasNil bool
+		path   string
+	}
+
+	testCases := map[string]struct {
+		theMap map[string]interface{}
+		rsExp  resultExpected
+	}{
+		"actuallyNil": {
+			theMap: nil,
+			rsExp:  resultExpected{},
+		},
+		"empty": {
+			theMap: map[string]interface{}{},
+			rsExp:  resultExpected{},
+		},
+		"list": {
+			theMap: map[string]interface{}{
+				"apiVersion": "v1",
+				"kind":       "List",
+				"items": []interface{}{
+					testConfigMap,
+					testConfigMap,
+				},
+			},
+			rsExp: resultExpected{},
+		},
+		"listWithNil": {
+			theMap: map[string]interface{}{
+				"apiVersion": "v1",
+				"kind":       "List",
+				"items": []interface{}{
+					testConfigMap,
+					nil,
+				},
+			},
+			rsExp: resultExpected{
+				hasNil: false, // TODO: This should be true.
+				path:   "this/should/be/non-empty",
+			},
+		},
+	}
+
+	for n := range testCases {
+		tc := testCases[n]
+		t.Run(n, func(t *testing.T) {
+			rn, err := FromMap(tc.theMap)
+			if !assert.NoError(t, err) {
+				t.FailNow()
+			}
+			hasNil, path := rn.HasNilEntryInList()
+			if tc.rsExp.hasNil {
+				if !assert.True(t, hasNil) {
+					t.FailNow()
+				}
+				if !assert.Equal(t, tc.rsExp.path, path) {
+					t.FailNow()
+				}
+			} else {
+				if !assert.False(t, hasNil) {
+					t.FailNow()
+				}
+				if !assert.Empty(t, path) {
+					t.FailNow()
+				}
+			}
+		})
+	}
+}
+
+func TestRNodeGetValidatedMetadata(t *testing.T) {
+	testConfigMap := map[string]interface{}{
+		"apiVersion": "v1",
+		"kind":       "ConfigMap",
+		"metadata": map[string]interface{}{
+			"name": "winnie",
+		},
+	}
+	type resultExpected struct {
+		out    ResourceMeta
+		errMsg string
+	}
+
+	testCases := map[string]struct {
+		theMap map[string]interface{}
+		rsExp  resultExpected
+	}{
+		"actuallyNil": {
+			theMap: nil,
+			rsExp: resultExpected{
+				errMsg: "missing Resource metadata",
+			},
+		},
+		"empty": {
+			theMap: map[string]interface{}{},
+			rsExp: resultExpected{
+				errMsg: "missing Resource metadata",
+			},
+		},
+		"mostlyEmpty": {
+			theMap: map[string]interface{}{
+				"hey": "there",
+			},
+			rsExp: resultExpected{
+				errMsg: "missing Resource metadata",
+			},
+		},
+		"noNameConfigMap": {
+			theMap: map[string]interface{}{
+				"apiVersion": "v1",
+				"kind":       "ConfigMap",
+			},
+			rsExp: resultExpected{
+				errMsg: "missing metadata.name",
+			},
+		},
+		"configmap": {
+			theMap: testConfigMap,
+			rsExp: resultExpected{
+				out: ResourceMeta{
+					TypeMeta: TypeMeta{
+						APIVersion: "v1",
+						Kind:       "ConfigMap",
+					},
+					ObjectMeta: ObjectMeta{
+						NameMeta: NameMeta{
+							Name: "winnie",
+						},
+					},
+				},
+			},
+		},
+		"list": {
+			theMap: map[string]interface{}{
+				"apiVersion": "v1",
+				"kind":       "List",
+				"items": []interface{}{
+					testConfigMap,
+					testConfigMap,
+				},
+			},
+			rsExp: resultExpected{
+				out: ResourceMeta{
+					TypeMeta: TypeMeta{
+						APIVersion: "v1",
+						Kind:       "List",
+					},
+				},
+			},
+		},
+		"configmaplist": {
+			theMap: map[string]interface{}{
+				"apiVersion": "v1",
+				"kind":       "ConfigMapList",
+				"items": []interface{}{
+					testConfigMap,
+					testConfigMap,
+				},
+			},
+			rsExp: resultExpected{
+				out: ResourceMeta{
+					TypeMeta: TypeMeta{
+						APIVersion: "v1",
+						Kind:       "ConfigMapList",
+					},
+				},
+			},
+		},
+	}
+
+	for n := range testCases {
+		tc := testCases[n]
+		t.Run(n, func(t *testing.T) {
+			rn, err := FromMap(tc.theMap)
+			if !assert.NoError(t, err) {
+				t.FailNow()
+			}
+			m, err := rn.GetValidatedMetadata()
+			if tc.rsExp.errMsg == "" {
+				if !assert.NoError(t, err) {
+					t.FailNow()
+				}
+				if !assert.Equal(t, tc.rsExp.out, m) {
+					t.FailNow()
+				}
+			} else {
+				if !assert.Error(t, err) {
+					t.FailNow()
+				}
+				if !assert.Contains(t, err.Error(), tc.rsExp.errMsg) {
+					t.FailNow()
+				}
+			}
+		})
+	}
+}
+
 func TestRNodeFromMap(t *testing.T) {
 	testConfigMap := map[string]interface{}{
 		"apiVersion": "v1",
@@ -19,25 +225,25 @@ func TestRNodeFromMap(t *testing.T) {
 			"name": "winnie",
 		},
 	}
-	type thingExpected struct {
+	type resultExpected struct {
 		out string
 		err error
 	}
 
 	testCases := map[string]struct {
-		theMap   map[string]interface{}
-		expected thingExpected
+		theMap map[string]interface{}
+		rsExp  resultExpected
 	}{
 		"actuallyNil": {
 			theMap: nil,
-			expected: thingExpected{
+			rsExp: resultExpected{
 				out: `{}`,
 				err: nil,
 			},
 		},
 		"empty": {
 			theMap: map[string]interface{}{},
-			expected: thingExpected{
+			rsExp: resultExpected{
 				out: `{}`,
 				err: nil,
 			},
@@ -46,14 +252,14 @@ func TestRNodeFromMap(t *testing.T) {
 			theMap: map[string]interface{}{
 				"hey": "there",
 			},
-			expected: thingExpected{
+			rsExp: resultExpected{
 				out: `hey: there`,
 				err: nil,
 			},
 		},
 		"configmap": {
 			theMap: testConfigMap,
-			expected: thingExpected{
+			rsExp: resultExpected{
 				out: `
 apiVersion: v1
 kind: ConfigMap
@@ -72,7 +278,7 @@ metadata:
 					testConfigMap,
 				},
 			},
-			expected: thingExpected{
+			rsExp: resultExpected{
 				out: `
 apiVersion: v1
 items:
@@ -98,7 +304,7 @@ kind: List
 					testConfigMap,
 				},
 			},
-			expected: thingExpected{
+			rsExp: resultExpected{
 				out: `
 apiVersion: v1
 items:
@@ -121,12 +327,12 @@ kind: ConfigMapList
 		tc := testCases[n]
 		t.Run(n, func(t *testing.T) {
 			rn, err := FromMap(tc.theMap)
-			if tc.expected.err == nil {
+			if tc.rsExp.err == nil {
 				if !assert.NoError(t, err) {
 					t.FailNow()
 				}
 				if !assert.Equal(t,
-					strings.TrimSpace(tc.expected.out),
+					strings.TrimSpace(tc.rsExp.out),
 					strings.TrimSpace(rn.MustString())) {
 					t.FailNow()
 				}
@@ -134,7 +340,7 @@ kind: ConfigMapList
 				if !assert.Error(t, err) {
 					t.FailNow()
 				}
-				if !assert.Equal(t, tc.expected.err, err) {
+				if !assert.Equal(t, tc.rsExp.err, err) {
 					t.FailNow()
 				}
 			}