From 0d19b79260f4fca812daab6b3ff21c54fc12cfb8 Mon Sep 17 00:00:00 2001 From: Dan Williams Date: Wed, 9 Nov 2016 15:11:18 -0600 Subject: [PATCH] types: make Result an interface and move existing Result to separate package --- invoke/delegate.go | 2 +- invoke/exec.go | 16 +-- invoke/exec_test.go | 6 +- ipam/ipam.go | 5 +- testutils/cmd.go | 16 ++- types/current/types.go | 157 ++++++++++++++++++++++++++++ types/current/types_suite_test.go | 27 +++++ types/current/types_test.go | 128 +++++++++++++++++++++++ types/types.go | 85 +++++---------- version/legacy_examples/examples.go | 5 +- version/reconcile.go | 20 ++-- version/reconcile_test.go | 4 +- version/version.go | 29 +++++ 13 files changed, 412 insertions(+), 88 deletions(-) create mode 100644 types/current/types.go create mode 100644 types/current/types_suite_test.go create mode 100644 types/current/types_test.go diff --git a/invoke/delegate.go b/invoke/delegate.go index ddf1d172..f25beddc 100644 --- a/invoke/delegate.go +++ b/invoke/delegate.go @@ -22,7 +22,7 @@ import ( "github.com/containernetworking/cni/pkg/types" ) -func DelegateAdd(delegatePlugin string, netconf []byte) (*types.Result, error) { +func DelegateAdd(delegatePlugin string, netconf []byte) (types.Result, error) { if os.Getenv("CNI_COMMAND") != "ADD" { return nil, fmt.Errorf("CNI_COMMAND is not ADD") } diff --git a/invoke/exec.go b/invoke/exec.go index 167d38f5..fc47e7c8 100644 --- a/invoke/exec.go +++ b/invoke/exec.go @@ -15,7 +15,6 @@ package invoke import ( - "encoding/json" "fmt" "os" @@ -23,7 +22,7 @@ import ( "github.com/containernetworking/cni/pkg/version" ) -func ExecPluginWithResult(pluginPath string, netconf []byte, args CNIArgs) (*types.Result, error) { +func ExecPluginWithResult(pluginPath string, netconf []byte, args CNIArgs) (types.Result, error) { return defaultPluginExec.WithResult(pluginPath, netconf, args) } @@ -49,15 +48,20 @@ type PluginExec struct { } } -func (e *PluginExec) WithResult(pluginPath string, netconf []byte, args CNIArgs) (*types.Result, error) { +func (e *PluginExec) WithResult(pluginPath string, netconf []byte, args CNIArgs) (types.Result, error) { stdoutBytes, err := e.RawExec.ExecPlugin(pluginPath, netconf, args.AsEnv()) if err != nil { return nil, err } - res := &types.Result{} - err = json.Unmarshal(stdoutBytes, res) - return res, err + // Plugin must return result in same version as specified in netconf + versionDecoder := &version.ConfigDecoder{} + confVersion, err := versionDecoder.Decode(netconf) + if err != nil { + return nil, err + } + + return version.NewResult(confVersion, stdoutBytes) } func (e *PluginExec) WithoutResult(pluginPath string, netconf []byte, args CNIArgs) error { diff --git a/invoke/exec_test.go b/invoke/exec_test.go index 2b9c9bf9..3e207c14 100644 --- a/invoke/exec_test.go +++ b/invoke/exec_test.go @@ -20,6 +20,7 @@ import ( "github.com/containernetworking/cni/pkg/invoke" "github.com/containernetworking/cni/pkg/invoke/fakes" + "github.com/containernetworking/cni/pkg/types/current" "github.com/containernetworking/cni/pkg/version" . "github.com/onsi/ginkgo" @@ -56,7 +57,10 @@ var _ = Describe("Executing a plugin, unit tests", func() { Describe("returning a result", func() { It("unmarshals the result bytes into the Result type", func() { - result, err := pluginExec.WithResult(pluginPath, netconf, cniargs) + r, err := pluginExec.WithResult(pluginPath, netconf, cniargs) + Expect(err).NotTo(HaveOccurred()) + + result, err := current.GetResult(r) Expect(err).NotTo(HaveOccurred()) Expect(result.IP4.IP.IP.String()).To(Equal("1.2.3.4")) }) diff --git a/ipam/ipam.go b/ipam/ipam.go index d9fbff74..8dd861a6 100644 --- a/ipam/ipam.go +++ b/ipam/ipam.go @@ -21,11 +21,12 @@ import ( "github.com/containernetworking/cni/pkg/invoke" "github.com/containernetworking/cni/pkg/ip" "github.com/containernetworking/cni/pkg/types" + "github.com/containernetworking/cni/pkg/types/current" "github.com/vishvananda/netlink" ) -func ExecAdd(plugin string, netconf []byte) (*types.Result, error) { +func ExecAdd(plugin string, netconf []byte) (types.Result, error) { return invoke.DelegateAdd(plugin, netconf) } @@ -35,7 +36,7 @@ func ExecDel(plugin string, netconf []byte) error { // ConfigureIface takes the result of IPAM plugin and // applies to the ifName interface -func ConfigureIface(ifName string, res *types.Result) error { +func ConfigureIface(ifName string, res *current.Result) error { link, err := netlink.LinkByName(ifName) if err != nil { return fmt.Errorf("failed to lookup %q: %v", ifName, err) diff --git a/testutils/cmd.go b/testutils/cmd.go index 0118f61c..5883c08f 100644 --- a/testutils/cmd.go +++ b/testutils/cmd.go @@ -15,11 +15,11 @@ package testutils import ( - "encoding/json" "io/ioutil" "os" "github.com/containernetworking/cni/pkg/types" + "github.com/containernetworking/cni/pkg/version" ) func envCleanup() { @@ -29,7 +29,7 @@ func envCleanup() { os.Unsetenv("CNI_IFNAME") } -func CmdAddWithResult(cniNetns, cniIfname string, conf []byte, f func() error) (*types.Result, []byte, error) { +func CmdAddWithResult(cniNetns, cniIfname string, conf []byte, f func() error) (types.Result, []byte, error) { os.Setenv("CNI_COMMAND", "ADD") os.Setenv("CNI_PATH", os.Getenv("PATH")) os.Setenv("CNI_NETNS", cniNetns) @@ -57,13 +57,19 @@ func CmdAddWithResult(cniNetns, cniIfname string, conf []byte, f func() error) ( return nil, nil, err } - result := types.Result{} - err = json.Unmarshal(out, &result) + // Plugin must return result in same version as specified in netconf + versionDecoder := &version.ConfigDecoder{} + confVersion, err := versionDecoder.Decode(conf) if err != nil { return nil, nil, err } - return &result, out, nil + result, err := version.NewResult(confVersion, out) + if err != nil { + return nil, nil, err + } + + return result, out, nil } func CmdDelWithResult(cniNetns, cniIfname string, f func() error) error { diff --git a/types/current/types.go b/types/current/types.go new file mode 100644 index 00000000..338b3fd2 --- /dev/null +++ b/types/current/types.go @@ -0,0 +1,157 @@ +// Copyright 2016 CNI authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package current + +import ( + "encoding/json" + "fmt" + "net" + "os" + + "github.com/containernetworking/cni/pkg/types" +) + +const implementedSpecVersion string = "0.2.0" + +var SupportedVersions = []string{"", "0.1.0", implementedSpecVersion} + +func NewResult(data []byte) (types.Result, error) { + result := &Result{} + if err := json.Unmarshal(data, result); err != nil { + return nil, err + } + return result, nil +} + +func GetResult(r types.Result) (*Result, error) { + newResult, err := r.GetAsVersion(implementedSpecVersion) + if err != nil { + return nil, err + } + result, ok := newResult.(*Result) + if !ok { + return nil, fmt.Errorf("failed to convert result") + } + return result, nil +} + +var resultConverters = []struct { + versions []string + convert func(types.Result) (*Result, error) +}{ + {SupportedVersions, convertFrom020}, +} + +func convertFrom020(result types.Result) (*Result, error) { + newResult, ok := result.(*Result) + if !ok { + return nil, fmt.Errorf("failed to convert result") + } + return newResult, nil +} + +func NewResultFromResult(result types.Result) (*Result, error) { + version := result.Version() + for _, converter := range resultConverters { + for _, supportedVersion := range converter.versions { + if version == supportedVersion { + return converter.convert(result) + } + } + } + return nil, fmt.Errorf("unsupported CNI result version %q", version) +} + +// Result is what gets returned from the plugin (via stdout) to the caller +type Result struct { + IP4 *IPConfig `json:"ip4,omitempty"` + IP6 *IPConfig `json:"ip6,omitempty"` + DNS types.DNS `json:"dns,omitempty"` +} + +func (r *Result) Version() string { + return implementedSpecVersion +} + +func (r *Result) GetAsVersion(version string) (types.Result, error) { + for _, supportedVersion := range SupportedVersions { + if version == supportedVersion { + return r, nil + } + } + return nil, fmt.Errorf("cannot convert version %q to %s", SupportedVersions, version) +} + +func (r *Result) Print() error { + data, err := json.MarshalIndent(r, "", " ") + if err != nil { + return err + } + _, err = os.Stdout.Write(data) + return err +} + +// String returns a formatted string in the form of "[IP4: $1,][ IP6: $2,] DNS: $3" where +// $1 represents the receiver's IPv4, $2 represents the receiver's IPv6 and $3 the +// receiver's DNS. If $1 or $2 are nil, they won't be present in the returned string. +func (r *Result) String() string { + var str string + if r.IP4 != nil { + str = fmt.Sprintf("IP4:%+v, ", *r.IP4) + } + if r.IP6 != nil { + str += fmt.Sprintf("IP6:%+v, ", *r.IP6) + } + return fmt.Sprintf("%sDNS:%+v", str, r.DNS) +} + +// IPConfig contains values necessary to configure an interface +type IPConfig struct { + IP net.IPNet + Gateway net.IP + Routes []types.Route +} + +// net.IPNet is not JSON (un)marshallable so this duality is needed +// for our custom IPNet type + +// JSON (un)marshallable types +type ipConfig struct { + IP types.IPNet `json:"ip"` + Gateway net.IP `json:"gateway,omitempty"` + Routes []types.Route `json:"routes,omitempty"` +} + +func (c *IPConfig) MarshalJSON() ([]byte, error) { + ipc := ipConfig{ + IP: types.IPNet(c.IP), + Gateway: c.Gateway, + Routes: c.Routes, + } + + return json.Marshal(ipc) +} + +func (c *IPConfig) UnmarshalJSON(data []byte) error { + ipc := ipConfig{} + if err := json.Unmarshal(data, &ipc); err != nil { + return err + } + + c.IP = net.IPNet(ipc.IP) + c.Gateway = ipc.Gateway + c.Routes = ipc.Routes + return nil +} diff --git a/types/current/types_suite_test.go b/types/current/types_suite_test.go new file mode 100644 index 00000000..42a47a25 --- /dev/null +++ b/types/current/types_suite_test.go @@ -0,0 +1,27 @@ +// Copyright 2016 CNI authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package current_test + +import ( + . "github.com/onsi/ginkgo" + . "github.com/onsi/gomega" + + "testing" +) + +func TestTypes010(t *testing.T) { + RegisterFailHandler(Fail) + RunSpecs(t, "0.1.0 Types Suite") +} diff --git a/types/current/types_test.go b/types/current/types_test.go new file mode 100644 index 00000000..3810999d --- /dev/null +++ b/types/current/types_test.go @@ -0,0 +1,128 @@ +// Copyright 2016 CNI authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package current_test + +import ( + "io/ioutil" + "net" + "os" + + "github.com/containernetworking/cni/pkg/types" + "github.com/containernetworking/cni/pkg/types/current" + + . "github.com/onsi/ginkgo" + . "github.com/onsi/gomega" +) + +var _ = Describe("Ensures compatibility with the 0.1.0 spec", func() { + It("correctly encodes a 0.1.0 Result", func() { + ipv4, err := types.ParseCIDR("1.2.3.30/24") + Expect(err).NotTo(HaveOccurred()) + Expect(ipv4).NotTo(BeNil()) + + routegwv4, routev4, err := net.ParseCIDR("15.5.6.8/24") + Expect(err).NotTo(HaveOccurred()) + Expect(routev4).NotTo(BeNil()) + Expect(routegwv4).NotTo(BeNil()) + + ipv6, err := types.ParseCIDR("abcd:1234:ffff::cdde/64") + Expect(err).NotTo(HaveOccurred()) + Expect(ipv6).NotTo(BeNil()) + + routegwv6, routev6, err := net.ParseCIDR("1111:dddd::aaaa/80") + Expect(err).NotTo(HaveOccurred()) + Expect(routev6).NotTo(BeNil()) + Expect(routegwv6).NotTo(BeNil()) + + // Set every field of the struct to ensure source compatibility + res := current.Result{ + IP4: ¤t.IPConfig{ + IP: *ipv4, + Gateway: net.ParseIP("1.2.3.1"), + Routes: []types.Route{ + {Dst: *routev4, GW: routegwv4}, + }, + }, + IP6: ¤t.IPConfig{ + IP: *ipv6, + Gateway: net.ParseIP("abcd:1234:ffff::1"), + Routes: []types.Route{ + {Dst: *routev6, GW: routegwv6}, + }, + }, + DNS: types.DNS{ + Nameservers: []string{"1.2.3.4", "1::cafe"}, + Domain: "acompany.com", + Search: []string{"somedomain.com", "otherdomain.net"}, + Options: []string{"foo", "bar"}, + }, + } + + Expect(res.String()).To(Equal("IP4:{IP:{IP:1.2.3.30 Mask:ffffff00} Gateway:1.2.3.1 Routes:[{Dst:{IP:15.5.6.0 Mask:ffffff00} GW:15.5.6.8}]}, IP6:{IP:{IP:abcd:1234:ffff::cdde Mask:ffffffffffffffff0000000000000000} Gateway:abcd:1234:ffff::1 Routes:[{Dst:{IP:1111:dddd:: Mask:ffffffffffffffffffff000000000000} GW:1111:dddd::aaaa}]}, DNS:{Nameservers:[1.2.3.4 1::cafe] Domain:acompany.com Search:[somedomain.com otherdomain.net] Options:[foo bar]}")) + + // Redirect stdout to capture JSON result + oldStdout := os.Stdout + r, w, err := os.Pipe() + Expect(err).NotTo(HaveOccurred()) + + os.Stdout = w + err = res.Print() + w.Close() + Expect(err).NotTo(HaveOccurred()) + + // parse the result + out, err := ioutil.ReadAll(r) + os.Stdout = oldStdout + Expect(err).NotTo(HaveOccurred()) + + Expect(string(out)).To(Equal(`{ + "ip4": { + "ip": "1.2.3.30/24", + "gateway": "1.2.3.1", + "routes": [ + { + "dst": "15.5.6.0/24", + "gw": "15.5.6.8" + } + ] + }, + "ip6": { + "ip": "abcd:1234:ffff::cdde/64", + "gateway": "abcd:1234:ffff::1", + "routes": [ + { + "dst": "1111:dddd::/80", + "gw": "1111:dddd::aaaa" + } + ] + }, + "dns": { + "nameservers": [ + "1.2.3.4", + "1::cafe" + ], + "domain": "acompany.com", + "search": [ + "somedomain.com", + "otherdomain.net" + ], + "options": [ + "foo", + "bar" + ] + } +}`)) + }) +}) diff --git a/types/types.go b/types/types.go index c1fddcd5..2ceffebc 100644 --- a/types/types.go +++ b/types/types.go @@ -16,7 +16,6 @@ package types import ( "encoding/json" - "fmt" "net" "os" ) @@ -59,10 +58,9 @@ func (n *IPNet) UnmarshalJSON(data []byte) error { type NetConf struct { CNIVersion string `json:"cniVersion,omitempty"` - Name string `json:"name,omitempty"` - Type string `json:"type,omitempty"` - PrevResult *Result `json:"prevResult,omitempty"` - IPAM struct { + Name string `json:"name,omitempty"` + Type string `json:"type,omitempty"` + IPAM struct { Type string `json:"type,omitempty"` } `json:"ipam,omitempty"` DNS DNS `json:"dns"` @@ -76,36 +74,31 @@ type NetConfList struct { Plugins []*NetConf `json:"plugins,omitempty"` } -// Result is what gets returned from the plugin (via stdout) to the caller -type Result struct { - IP4 *IPConfig `json:"ip4,omitempty"` - IP6 *IPConfig `json:"ip6,omitempty"` - DNS DNS `json:"dns,omitempty"` +type ResultFactoryFunc func([]byte) (Result, error) + +// Result is an interface that provides the result of plugin execution +type Result interface { + // The highest CNI specification result verison the result supports + // without having to convert + Version() string + + // Returns the result converted into the requested CNI specification + // result version, or an error if conversion failed + GetAsVersion(version string) (Result, error) + + // Prints the result in JSON format to stdout + Print() error + + // Returns a JSON string representation of the result + String() string } -func (r *Result) Print() error { - return prettyPrint(r) -} - -// String returns a formatted string in the form of "[IP4: $1,][ IP6: $2,] DNS: $3" where -// $1 represents the receiver's IPv4, $2 represents the receiver's IPv6 and $3 the -// receiver's DNS. If $1 or $2 are nil, they won't be present in the returned string. -func (r *Result) String() string { - var str string - if r.IP4 != nil { - str = fmt.Sprintf("IP4:%+v, ", *r.IP4) +func PrintResult(result Result, version string) error { + newResult, err := result.GetAsVersion(version) + if err != nil { + return err } - if r.IP6 != nil { - str += fmt.Sprintf("IP6:%+v, ", *r.IP6) - } - return fmt.Sprintf("%sDNS:%+v", str, r.DNS) -} - -// IPConfig contains values necessary to configure an interface -type IPConfig struct { - IP net.IPNet - Gateway net.IP - Routes []Route + return newResult.Print() } // DNS contains values interesting for DNS resolvers @@ -147,39 +140,11 @@ func (e *Error) Print() error { // for our custom IPNet type // JSON (un)marshallable types -type ipConfig struct { - IP IPNet `json:"ip"` - Gateway net.IP `json:"gateway,omitempty"` - Routes []Route `json:"routes,omitempty"` -} - type route struct { Dst IPNet `json:"dst"` GW net.IP `json:"gw,omitempty"` } -func (c *IPConfig) MarshalJSON() ([]byte, error) { - ipc := ipConfig{ - IP: IPNet(c.IP), - Gateway: c.Gateway, - Routes: c.Routes, - } - - return json.Marshal(ipc) -} - -func (c *IPConfig) UnmarshalJSON(data []byte) error { - ipc := ipConfig{} - if err := json.Unmarshal(data, &ipc); err != nil { - return err - } - - c.IP = net.IPNet(ipc.IP) - c.Gateway = ipc.Gateway - c.Routes = ipc.Routes - return nil -} - func (r *Route) UnmarshalJSON(data []byte) error { rt := route{} if err := json.Unmarshal(data, &rt); err != nil { diff --git a/version/legacy_examples/examples.go b/version/legacy_examples/examples.go index 57162312..8b079a3d 100644 --- a/version/legacy_examples/examples.go +++ b/version/legacy_examples/examples.go @@ -23,6 +23,7 @@ import ( "sync" "github.com/containernetworking/cni/pkg/types" + "github.com/containernetworking/cni/pkg/types/current" "github.com/containernetworking/cni/pkg/version/testhelpers" ) @@ -114,8 +115,8 @@ func main() { skel.PluginMain(c, c) } // // As we change the CNI spec, the Result type and this value may change. // The text of the example plugins should not. -var ExpectedResult = &types.Result{ - IP4: &types.IPConfig{ +var ExpectedResult = ¤t.Result{ + IP4: ¤t.IPConfig{ IP: net.IPNet{ IP: net.ParseIP("10.1.2.3"), Mask: net.CIDRMask(24, 32), diff --git a/version/reconcile.go b/version/reconcile.go index f61ef653..25c3810b 100644 --- a/version/reconcile.go +++ b/version/reconcile.go @@ -17,12 +17,12 @@ package version import "fmt" type ErrorIncompatible struct { - Config string - Plugin []string + Config string + Supported []string } func (e *ErrorIncompatible) Details() string { - return fmt.Sprintf("config is %q, plugin supports %q", e.Config, e.Plugin) + return fmt.Sprintf("config is %q, plugin supports %q", e.Config, e.Supported) } func (e *ErrorIncompatible) Error() string { @@ -31,17 +31,19 @@ func (e *ErrorIncompatible) Error() string { type Reconciler struct{} -func (*Reconciler) Check(configVersion string, pluginInfo PluginInfo) *ErrorIncompatible { - pluginVersions := pluginInfo.SupportedVersions() +func (r *Reconciler) Check(configVersion string, pluginInfo PluginInfo) *ErrorIncompatible { + return r.CheckRaw(configVersion, pluginInfo.SupportedVersions()) +} - for _, pluginVersion := range pluginVersions { - if configVersion == pluginVersion { +func (*Reconciler) CheckRaw(configVersion string, supportedVersions []string) *ErrorIncompatible { + for _, supportedVersion := range supportedVersions { + if configVersion == supportedVersion { return nil } } return &ErrorIncompatible{ - Config: configVersion, - Plugin: pluginVersions, + Config: configVersion, + Supported: supportedVersions, } } diff --git a/version/reconcile_test.go b/version/reconcile_test.go index 19a9e23f..0c964cea 100644 --- a/version/reconcile_test.go +++ b/version/reconcile_test.go @@ -41,8 +41,8 @@ var _ = Describe("Reconcile versions of net config with versions supported by pl err := reconciler.Check("0.1.0", pluginInfo) Expect(err).To(Equal(&version.ErrorIncompatible{ - Config: "0.1.0", - Plugin: []string{"1.2.3", "4.3.2"}, + Config: "0.1.0", + Supported: []string{"1.2.3", "4.3.2"}, })) Expect(err.Error()).To(Equal(`incompatible CNI versions: config is "0.1.0", plugin supports ["1.2.3" "4.3.2"]`)) diff --git a/version/version.go b/version/version.go index e39c3b55..e777e52c 100644 --- a/version/version.go +++ b/version/version.go @@ -14,6 +14,13 @@ package version +import ( + "fmt" + + "github.com/containernetworking/cni/pkg/types" + "github.com/containernetworking/cni/pkg/types/current" +) + // Current reports the version of the CNI spec implemented by this library func Current() string { return "0.2.0" @@ -27,3 +34,25 @@ func Current() string { // Any future CNI spec versions which meet this definition should be added to // this list. var Legacy = PluginSupports("0.1.0", "0.2.0") + +var resultFactories = []struct { + supportedVersions []string + newResult types.ResultFactoryFunc +}{ + {current.SupportedVersions, current.NewResult}, +} + +// Finds a Result object matching the requested version (if any) and asks +// that object to parse the plugin result, returning an error if parsing failed. +func NewResult(version string, resultBytes []byte) (types.Result, error) { + reconciler := &Reconciler{} + for _, resultFactory := range resultFactories { + err := reconciler.CheckRaw(version, resultFactory.supportedVersions) + if err == nil { + // Result supports this version + return resultFactory.newResult(resultBytes) + } + } + + return nil, fmt.Errorf("unsupported CNI result version %q", version) +}