refactor function filters

kyaml/kio/filters/container.go

@@ -389,8 +389,7 @@ type IsReconcilerFilter struct {
 func (c *IsReconcilerFilter) Filter(inputs []*yaml.RNode) ([]*yaml.RNode, error) {
 	var out []*yaml.RNode
 	for i := range inputs {
-		img, _ := GetContainerName(inputs[i])
+		isContainerResource := GetFunctionSpec(inputs[i]) != nil
-		isContainerResource := img != ""
 		if isContainerResource && !c.ExcludeReconcilers {
 			out = append(out, inputs[i])
 		}
@@ -408,52 +407,67 @@ const (
 
 var functionAnnotationKeys = []string{FunctionAnnotationKey, oldFunctionAnnotationKey}
 
-// GetFunction parses the config function from the object if it is found
+// getFunction parses the config function from the object if it is found
-func GetFunction(n *yaml.RNode, meta yaml.ResourceMeta) (*yaml.RNode, error) {
+func getFunction(n *yaml.RNode, meta yaml.ResourceMeta) *FunctionSpec {
+	var fs FunctionSpec
 	for _, s := range functionAnnotationKeys {
 		fn := meta.Annotations[s]
 		if fn != "" {
-			return yaml.Parse(fn)
+			_ = yaml.Unmarshal([]byte(fn), &fs)
+			return &fs
 		}
 	}
-	return n.Pipe(yaml.Lookup("metadata", "configFn"))
+	n, err := n.Pipe(yaml.Lookup("metadata", "configFn"))
+	if err != nil || yaml.IsEmpty(n) {
+		return nil
+	}
+	s, err := n.String()
+	if err != nil {
+		return nil
+	}
+	_ = yaml.Unmarshal([]byte(s), &fs)
+	return &fs
 }
 
-// GetContainerName returns the container image for an API if one exists
+type ContainerSpec struct {
-func GetContainerName(n *yaml.RNode) (string, string) {
+	Image   string           `json:"image,omitempty" yaml:"image,omitempty"`
-	meta, _ := n.GetMeta()
+	Network ContainerNetwork `json:"network,omitempty" yaml:"network,omitempty"`
+}
+
+type FunctionSpec struct {
+	Path      string        `json:"path,omitempty" yaml:"path,omitempty"`
+	Network   string        `json:"network,omitempty" yaml:"network,omitempty"`
+	Container ContainerSpec `json:"container,omitempty" yaml:"container,omitempty"`
+}
+
+type ContainerNetwork struct {
+	Required bool `json:"required,omitempty" yaml:"required,omitempty"`
+}
+
+// GetFunctionSpec returns the FunctionSpec for a resource.  Returns
+// nil if the resource does not have a FunctionSpec.
+//
+// The FunctionSpec is read from the resource metadata.annotation
+// "config.kubernetes.io/function"
+func GetFunctionSpec(n *yaml.RNode) *FunctionSpec {
+	meta, err := n.GetMeta()
+	if err != nil {
+		return nil
+	}
 
 	// path to the function, this will be mounted into the container
 	path := meta.Annotations[kioutil.PathAnnotation]
-
+	if fn := getFunction(n, meta); fn != nil {
-	fn, _ := GetFunction(n, meta)
+		fn.Network = ""
-	if fn != nil {
+		fn.Path = path
-		image, _ := fn.Pipe(yaml.Lookup("container", "image"))
+		return fn
-		return yaml.GetValue(image), path
 	}
 
+	// legacy function specification for backwards compatibility
 	container := meta.Annotations["config.kubernetes.io/container"]
 	if container != "" {
-		return container, path
+		return &FunctionSpec{
+			Path: path, Container: ContainerSpec{Image: container}}
 	}
-
+	return nil
-	image, err := n.Pipe(yaml.Lookup("metadata", "configFn", "container", "image"))
-	if err != nil || yaml.IsMissingOrNull(image) {
-		return "", path
-	}
-	return yaml.GetValue(image), path
-}
-
-// GetContainerNetworkRequired returns whether or not networking is required for the container
-func GetContainerNetworkRequired(n *yaml.RNode) (bool, error) {
-	meta, err := n.GetMeta()
-	if err != nil {
-		return false, err
-	}
-	f, err := GetFunction(n, meta)
-	if err != nil {
-		return false, err
-	}
-	networkRequired, _ := f.Pipe(yaml.Lookup("container", "network", "required"))
-	return yaml.GetValue(networkRequired) == "true", nil
 }

kyaml/kio/filters/container_test.go

@@ -326,9 +326,71 @@ kind: Example
 metadata:
   annotations:
     config.kubernetes.io/function: |-
-      container: foo:v1.0.0
+      container:
+        image: foo:v1.0.0
+`,
+			expectedFn: `
+container:
+    image: foo:v1.0.0`,
+		},
+
+		{
+			name: "network",
+			resource: `
+apiVersion: v1beta1
+kind: Example
+metadata:
+  annotations:
+    config.kubernetes.io/function: |-
+      container:
+        image: foo:v1.0.0
+        network:
+          required: true
+`,
+			expectedFn: `
+container:
+    image: foo:v1.0.0
+    network:
+        required: true
+`,
+		},
+
+		{
+			name: "path",
+			resource: `
+apiVersion: v1beta1
+kind: Example
+metadata:
+  annotations:
+    config.kubernetes.io/function: |-
+      path: foo
+      container:
+        image: foo:v1.0.0
+`,
+			// path should be erased
+			expectedFn: `
+container:
+    image: foo:v1.0.0
+`,
+		},
+
+		{
+			name: "network",
+			resource: `
+apiVersion: v1beta1
+kind: Example
+metadata:
+  annotations:
+    config.kubernetes.io/function: |-
+      network: foo
+      container:
+        image: foo:v1.0.0
+`,
+			// network should be erased
+			expectedFn: `
+container:
+    image: foo:v1.0.0
 `,
-			expectedFn: `container: foo:v1.0.0`,
 		},
 
 		// legacy fn style
@@ -338,9 +400,13 @@ apiVersion: v1beta1
 kind: Example
 metadata:
   configFn:
-      container: foo:v1.0.0
+      container:
+        image: foo:v1.0.0
+`,
+			expectedFn: `
+container:
+    image: foo:v1.0.0
 `,
-			expectedFn: `container: foo:v1.0.0`,
 		},
 
 		// no fn
@@ -361,20 +427,19 @@ metadata:
 		tt := tests[i]
 		t.Run(tt.name, func(t *testing.T) {
 			resource := yaml.MustParse(tt.resource)
-			meta, err := resource.GetMeta()
+			fn := GetFunctionSpec(resource)
-			if !assert.NoError(t, err) {
-				t.FailNow()
-			}
-			fn, err := GetFunction(resource, meta)
-			if !assert.NoError(t, err) {
-				t.FailNow()
-			}
 			if tt.missingFn {
 				if !assert.Nil(t, fn) {
 					t.FailNow()
 				}
 			} else {
-				if !assert.Equal(t, strings.TrimSpace(fn.MustString()), strings.TrimSpace(tt.expectedFn)) {
+				b, err := yaml.Marshal(fn)
+				if !assert.NoError(t, err) {
+					t.FailNow()
+				}
+				if !assert.Equal(t,
+					strings.TrimSpace(tt.expectedFn),
+					strings.TrimSpace(string(b))) {
 					t.FailNow()
 				}
 			}
@@ -382,61 +447,6 @@ metadata:
 	}
 }
 
-func Test_GetContainerName(t *testing.T) {
-	// make sure gcr.io works
-	n, err := yaml.Parse(`apiVersion: v1beta1
-kind: MyThing
-metadata:
-  configFn:
-    container:
-      image: gcr.io/foo/bar:something
-`)
-	if !assert.NoError(t, err) {
-		return
-	}
-	c, _ := GetContainerName(n)
-	assert.Equal(t, "gcr.io/foo/bar:something", c)
-
-	// container from config.kubernetes.io/container annotation
-	n, err = yaml.Parse(`apiVersion: v1
-kind: MyThing
-metadata:
-  annotations:
-    config.kubernetes.io/container: gcr.io/foo/bar:something
-`)
-	if !assert.NoError(t, err) {
-		return
-	}
-	c, _ = GetContainerName(n)
-	assert.Equal(t, "gcr.io/foo/bar:something", c)
-
-	// container from config.kubernetes.io/function annotation
-	n, err = yaml.Parse(`apiVersion: v1
-kind: MyThing
-metadata:
-  annotations:
-    config.kubernetes.io/function: |
-      container:
-        image: gcr.io/foo/bar:something
-`)
-	if !assert.NoError(t, err) {
-		return
-	}
-	c, _ = GetContainerName(n)
-	assert.Equal(t, "gcr.io/foo/bar:something", c)
-
-	// doesn't have a container
-	n, err = yaml.Parse(`apiVersion: v1
-kind: MyThing
-metadata:
-`)
-	if !assert.NoError(t, err) {
-		return
-	}
-	c, _ = GetContainerName(n)
-	assert.Equal(t, "", c)
-}
-
 func Test_GetContainerNetworkRequired(t *testing.T) {
 	tests := []struct {
 		input    string
@@ -501,9 +511,10 @@ metadata:
 		if !assert.NoError(t, err) {
 			return
 		}
-		required, err := GetContainerNetworkRequired(cfg)
+
-		assert.NoError(t, err)
+		meta, _ := cfg.GetMeta()
-		assert.Equal(t, tc.required, required)
+		fn := getFunction(cfg, meta)
+		assert.Equal(t, tc.required, fn.Container.Network.Required)
 	}
 }
 

kyaml/runfn/runfn.go

@@ -57,8 +57,10 @@ type RunFns struct {
 	// and only use explicit sources
 	NoFunctionsFromInput *bool
 
-	// for testing purposes only
+	// functionFilterProvider provides a filter to perform the function.
-	containerFilterProvider func(string, string, string, *yaml.RNode) kio.Filter
+	// this is a variable so it can be mocked in tests
+	functionFilterProvider func(
+		filter filters.FunctionSpec, api *yaml.RNode) kio.Filter
 }
 
 // Execute runs the command
@@ -110,21 +112,21 @@ func (r RunFns) getNodesAndFilters() (
 func (r RunFns) getFilters(nodes []*yaml.RNode) ([]kio.Filter, error) {
 	var fltrs []kio.Filter
 
-	// implicit filters from the input Resources
+	// fns from annotations on the input resources
 	f, err := r.getFunctionsFromInput(nodes)
 	if err != nil {
 		return nil, err
 	}
 	fltrs = append(fltrs, f...)
 
-	// explicit filters from a list of directories
+	// fns from directories specified on the struct
 	f, err = r.getFunctionsFromFunctionPaths()
 	if err != nil {
 		return nil, err
 	}
 	fltrs = append(fltrs, f...)
 
-	// explicit filters from a list of directories
+	// explicit fns specified on the struct
 	f, err = r.getFunctionsFromFunctions()
 	if err != nil {
 		return nil, err
@@ -156,7 +158,6 @@ func (r RunFns) getFunctionsFromInput(nodes []*yaml.RNode) ([]kio.Filter, error)
 		return nil, nil
 	}
 
-	var fltrs []kio.Filter
 	buff := &kio.PackageBuffer{}
 	err := kio.Pipeline{
 		Inputs:  []kio.Reader{&kio.PackageBuffer{Nodes: nodes}},
@@ -167,95 +168,50 @@ func (r RunFns) getFunctionsFromInput(nodes []*yaml.RNode) ([]kio.Filter, error)
 		return nil, err
 	}
 	sortFns(buff)
-	for i := range buff.Nodes {
+	return r.getFunctionFilters(false, buff.Nodes...)
-		api := buff.Nodes[i]
-		network := ""
-		img, path := filters.GetContainerName(api)
-
-		required, err := filters.GetContainerNetworkRequired(api)
-		if err != nil {
-			return nil, err
-		}
-		if required {
-			if !r.Network {
-				// TODO(eddizane): Provide error info about which function needs the network
-				return fltrs, errors.Errorf("network required but not enabled with --network")
-			}
-			network = r.NetworkName
-		}
-
-		fltrs = append(fltrs, r.containerFilterProvider(img, path, network, api))
-	}
-	return fltrs, nil
 }
 
 // getFunctionsFromFunctionPaths returns the set of functions read from r.FunctionPaths
 // as a slice of Filters
 func (r RunFns) getFunctionsFromFunctionPaths() ([]kio.Filter, error) {
-	var fltrs []kio.Filter
 	buff := &kio.PackageBuffer{}
 	for i := range r.FunctionPaths {
 		err := kio.Pipeline{
-			Inputs:  []kio.Reader{kio.LocalPackageReader{PackagePath: r.FunctionPaths[i]}},
+			Inputs: []kio.Reader{
+				kio.LocalPackageReader{PackagePath: r.FunctionPaths[i]},
+			},
 			Outputs: []kio.Writer{buff},
 		}.Execute()
 		if err != nil {
 			return nil, err
 		}
 	}
-	for i := range buff.Nodes {
+	return r.getFunctionFilters(true, buff.Nodes...)
-		api := buff.Nodes[i]
-		network := ""
-		img, path := filters.GetContainerName(api)
-
-		required, err := filters.GetContainerNetworkRequired(api)
-		if err != nil {
-			return nil, err
-		}
-		if required {
-			if !r.Network {
-				// TODO(eddiezane): Provide error info about which function needs the network
-				return fltrs, errors.Errorf("network required but not enabled with --network")
-			}
-			network = r.NetworkName
-		}
-
-		c := r.containerFilterProvider(img, path, network, api)
-		cf, ok := c.(*filters.ContainerFilter)
-		if ok {
-			// functions provided by FunctionPaths are globally scoped
-			cf.GlobalScope = true
-		}
-		fltrs = append(fltrs, c)
-	}
-	return fltrs, nil
 }
 
 // getFunctionsFromFunctions returns the set of explicitly provided functions as
 // Filters
 func (r RunFns) getFunctionsFromFunctions() ([]kio.Filter, error) {
-	var fltrs []kio.Filter
+	return r.getFunctionFilters(true, r.Functions...)
-	for i := range r.Functions {
-		api := r.Functions[i]
-		network := ""
-		img, path := filters.GetContainerName(api)
-
-		required, err := filters.GetContainerNetworkRequired(api)
-		if err != nil {
-			return nil, err
 }
-		if required {
+
+func (r RunFns) getFunctionFilters(global bool, fns ...*yaml.RNode) (
+	[]kio.Filter, error) {
+	var fltrs []kio.Filter
+	for i := range fns {
+		api := fns[i]
+		spec := filters.GetFunctionSpec(api)
+		if spec.Container.Network.Required {
 			if !r.Network {
-				// TODO(eddizane): Provide error info about which function needs the network
+				// TODO(eddiezane): Provide error info about which function needs the network
 				return fltrs, errors.Errorf("network required but not enabled with --network")
 			}
-			network = r.NetworkName
+			spec.Network = r.NetworkName
 		}
 
-		c := r.containerFilterProvider(img, path, network, api)
+		c := r.functionFilterProvider(*spec, api)
 		cf, ok := c.(*filters.ContainerFilter)
-		if ok {
+		if global && ok {
-			// functions provided by Functions are globally scoped
 			cf.GlobalScope = true
 		}
 		fltrs = append(fltrs, c)
@@ -327,17 +283,26 @@ func (r *RunFns) init() {
 		}
 	}
 
-	// if containerFilterProvider hasn't been set, use the default
+	// functionFilterProvider set the filter provider
-	if r.containerFilterProvider == nil {
+	if r.functionFilterProvider == nil {
-		r.containerFilterProvider = func(image, path, network string, api *yaml.RNode) kio.Filter {
+		r.functionFilterProvider = r.ffp
-			cf := &filters.ContainerFilter{
+	}
-				Image:         image,
+}
+
+// ffp provides function filters
+func (r *RunFns) ffp(spec filters.FunctionSpec, api *yaml.RNode) kio.Filter {
+	if spec.Container.Image != "" {
+		return &filters.ContainerFilter{
+			Image:         spec.Container.Image,
 			Config:        api,
-				Network:       network,
+			Network:       spec.Network,
 			StorageMounts: r.StorageMounts,
 			GlobalScope:   r.GlobalScope,
 		}
-			return cf
-		}
 	}
+	return noOpFilter
 }
+
+var noOpFilter = kio.FilterFunc(func(in []*yaml.RNode) ([]*yaml.RNode, error) {
+	return in, nil
+})

kyaml/runfn/runfn_test.go

@@ -47,10 +47,15 @@ func TestRunFns_init(t *testing.T) {
 	api, err := yaml.Parse(`apiVersion: apps/v1
 kind: 
 `)
+	spec := filters.FunctionSpec{
+		Container: filters.ContainerSpec{
+			Image: "example.com:version",
+		},
+	}
 	if !assert.NoError(t, err) {
 		return
 	}
-	filter := instance.containerFilterProvider("example.com:version", "", "", api)
+	filter := instance.functionFilterProvider(spec, api)
 	assert.Equal(t, &filters.ContainerFilter{Image: "example.com:version", Config: api}, filter)
 }
 
@@ -69,7 +74,16 @@ kind:
 	if !assert.NoError(t, err) {
 		return
 	}
-	filter := instance.containerFilterProvider("example.com:version", "", "", api)
+
+	spec := filters.FunctionSpec{
+		Container: filters.ContainerSpec{
+			Image: "example.com:version",
+		},
+	}
+	if !assert.NoError(t, err) {
+		return
+	}
+	filter := instance.functionFilterProvider(spec, api)
 	assert.Equal(t, &filters.ContainerFilter{
 		Image: "example.com:version", Config: api, GlobalScope: true}, filter)
 }
@@ -131,7 +145,7 @@ func TestRunFns_Execute__initDefault(t *testing.T) {
 		tt := tests[i]
 		t.Run(tt.name, func(t *testing.T) {
 			(&tt.instance).init()
-			(&tt.instance).containerFilterProvider = nil
+			(&tt.instance).functionFilterProvider = nil
 			if !assert.Equal(t, tt.expected, tt.instance) {
 				t.FailNow()
 			}
@@ -505,7 +519,7 @@ func TestCmd_Execute(t *testing.T) {
 		return
 	}
 
-	instance := RunFns{Path: dir, containerFilterProvider: getFilterProvider(t)}
+	instance := RunFns{Path: dir, functionFilterProvider: getFilterProvider(t)}
 	if !assert.NoError(t, instance.Execute()) {
 		t.FailNow()
 	}
@@ -536,7 +550,7 @@ func TestCmd_Execute_setFunctionPaths(t *testing.T) {
 	instance := RunFns{
 		FunctionPaths:          []string{tmpF.Name()},
 		Path:                   dir,
-		containerFilterProvider: getFilterProvider(t),
+		functionFilterProvider: getFilterProvider(t),
 	}
 	// initialize the defaults
 	instance.init()
@@ -568,7 +582,7 @@ func TestCmd_Execute_setOutput(t *testing.T) {
 	instance := RunFns{
 		Output:                 out, // write to out
 		Path:                   dir,
-		containerFilterProvider: getFilterProvider(t),
+		functionFilterProvider: getFilterProvider(t),
 	}
 	// initialize the defaults
 	instance.init()
@@ -616,7 +630,7 @@ func TestCmd_Execute_setInput(t *testing.T) {
 	instance := RunFns{
 		Input:                  input, // read from input
 		Path:                   outDir,
-		containerFilterProvider: getFilterProvider(t),
+		functionFilterProvider: getFilterProvider(t),
 	}
 	// initialize the defaults
 	instance.init()
@@ -659,8 +673,8 @@ func setupTest(t *testing.T) string {
 // getFilterProvider fakes the creation of a filter, replacing the ContainerFiler with
 // a filter to s/kind: Deployment/kind: StatefulSet/g.
 // this can be used to simulate running a filter.
-func getFilterProvider(t *testing.T) func(string, string, string, *yaml.RNode) kio.Filter {
+func getFilterProvider(t *testing.T) func(filters.FunctionSpec, *yaml.RNode) kio.Filter {
-	return func(s, _, _ string, node *yaml.RNode) kio.Filter {
+	return func(f filters.FunctionSpec, node *yaml.RNode) kio.Filter {
 		// parse the filter from the input
 		filter := yaml.YFilter{}
 		b := &bytes.Buffer{}