From efdd812cc10c11e17437b3693597fde7266463c8 Mon Sep 17 00:00:00 2001 From: Phillip Wittrock Date: Tue, 24 Mar 2020 11:18:46 -0700 Subject: [PATCH] refactor function filters --- kyaml/kio/filters/container.go | 84 ++++++++------- kyaml/kio/filters/container_test.go | 153 +++++++++++++++------------- kyaml/runfn/runfn.go | 127 +++++++++-------------- kyaml/runfn/runfn_test.go | 44 +++++--- 4 files changed, 206 insertions(+), 202 deletions(-) diff --git a/kyaml/kio/filters/container.go b/kyaml/kio/filters/container.go index c05a7a342..6602a08c4 100644 --- a/kyaml/kio/filters/container.go +++ b/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 := img != "" + isContainerResource := GetFunctionSpec(inputs[i]) != nil 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 -func GetFunction(n *yaml.RNode, meta yaml.ResourceMeta) (*yaml.RNode, error) { +// getFunction parses the config function from the object if it is found +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 -func GetContainerName(n *yaml.RNode) (string, string) { - meta, _ := n.GetMeta() +type ContainerSpec struct { + Image string `json:"image,omitempty" yaml:"image,omitempty"` + 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] - - fn, _ := GetFunction(n, meta) - if fn != nil { - image, _ := fn.Pipe(yaml.Lookup("container", "image")) - return yaml.GetValue(image), path + if fn := getFunction(n, meta); fn != nil { + fn.Network = "" + fn.Path = path + return fn } + // 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}} } - - 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 + return nil } diff --git a/kyaml/kio/filters/container_test.go b/kyaml/kio/filters/container_test.go index 43e697e3b..bc5b35721 100644 --- a/kyaml/kio/filters/container_test.go +++ b/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() - if !assert.NoError(t, err) { - t.FailNow() - } - fn, err := GetFunction(resource, meta) - if !assert.NoError(t, err) { - t.FailNow() - } + fn := GetFunctionSpec(resource) 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) - assert.Equal(t, tc.required, required) + + meta, _ := cfg.GetMeta() + fn := getFunction(cfg, meta) + assert.Equal(t, tc.required, fn.Container.Network.Required) } } diff --git a/kyaml/runfn/runfn.go b/kyaml/runfn/runfn.go index e64c6222d..ac45f080b 100644 --- a/kyaml/runfn/runfn.go +++ b/kyaml/runfn/runfn.go @@ -57,8 +57,10 @@ type RunFns struct { // and only use explicit sources NoFunctionsFromInput *bool - // for testing purposes only - containerFilterProvider func(string, string, string, *yaml.RNode) kio.Filter + // functionFilterProvider provides a filter to perform the function. + // 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 { - 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 + return r.getFunctionFilters(false, buff.Nodes...) } // 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 { - 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 + return r.getFunctionFilters(true, buff.Nodes...) } // getFunctionsFromFunctions returns the set of explicitly provided functions as // Filters func (r RunFns) getFunctionsFromFunctions() ([]kio.Filter, error) { - var fltrs []kio.Filter - for i := range r.Functions { - api := r.Functions[i] - network := "" - img, path := filters.GetContainerName(api) + return r.getFunctionFilters(true, r.Functions...) +} - 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 { - // functions provided by Functions are globally scoped + if global && ok { cf.GlobalScope = true } fltrs = append(fltrs, c) @@ -327,17 +283,26 @@ func (r *RunFns) init() { } } - // if containerFilterProvider hasn't been set, use the default - if r.containerFilterProvider == nil { - r.containerFilterProvider = func(image, path, network string, api *yaml.RNode) kio.Filter { - cf := &filters.ContainerFilter{ - Image: image, - Config: api, - Network: network, - StorageMounts: r.StorageMounts, - GlobalScope: r.GlobalScope, - } - return cf - } + // functionFilterProvider set the filter provider + if r.functionFilterProvider == nil { + r.functionFilterProvider = r.ffp } } + +// 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: spec.Network, + StorageMounts: r.StorageMounts, + GlobalScope: r.GlobalScope, + } + } + return noOpFilter +} + +var noOpFilter = kio.FilterFunc(func(in []*yaml.RNode) ([]*yaml.RNode, error) { + return in, nil +}) diff --git a/kyaml/runfn/runfn_test.go b/kyaml/runfn/runfn_test.go index 52d6ae229..b11818dc7 100644 --- a/kyaml/runfn/runfn_test.go +++ b/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() } @@ -534,9 +548,9 @@ func TestCmd_Execute_setFunctionPaths(t *testing.T) { // run the functions, providing the path to the directory of filters instance := RunFns{ - FunctionPaths: []string{tmpF.Name()}, - Path: dir, - containerFilterProvider: getFilterProvider(t), + FunctionPaths: []string{tmpF.Name()}, + Path: dir, + functionFilterProvider: getFilterProvider(t), } // initialize the defaults instance.init() @@ -566,9 +580,9 @@ func TestCmd_Execute_setOutput(t *testing.T) { out := &bytes.Buffer{} instance := RunFns{ - Output: out, // write to out - Path: dir, - containerFilterProvider: getFilterProvider(t), + Output: out, // write to out + Path: dir, + functionFilterProvider: getFilterProvider(t), } // initialize the defaults instance.init() @@ -614,9 +628,9 @@ func TestCmd_Execute_setInput(t *testing.T) { } instance := RunFns{ - Input: input, // read from input - Path: outDir, - containerFilterProvider: getFilterProvider(t), + Input: input, // read from input + Path: outDir, + 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 { - return func(s, _, _ string, node *yaml.RNode) kio.Filter { +func getFilterProvider(t *testing.T) func(filters.FunctionSpec, *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{}