Don't copy gw from IP4.Gateway to Route.GW When converting from 0.2.0

[luksa]

When converting from 0.2.0 to 0.3.x, we used to copy the Gateway defined
under IP4.Gateway to each route that had GW set to nil. This is wrong
because the result of converting from 0.2.0 to 0.3.x and back to 0.2.0
doesn't match the original config.

Also, the spec says the following about routes.gw: "IP of the gateway.
If omitted, a default gateway is assumed (as determined by the CNI
plugin)."

So, it should be up to the CNI plugin to determine what to do when the
gateway in a route isn't specified. Therefore, when converting from
0.2.0 to 0.3.x, we should NOT populate the gateway field.

[dcbw]

/lgtm

I tested this change with the plugins repo and it passes the plugins/main/* tests. That's likely because the ConfigureIface() function that they all use does the same operations as this PR removes when actually sending the routes to the kernel.

[dcbw]

Could we add a testcase to assert that conversion from 0.2.0 -> 0.3.0 -> 0.2.0 works?

[bboreham]

I agree the previous code looks wrong. Thanks!

[dcbw]

I just ended up writing the testcase, @luksa could you apply this diff to your PR and repush? Thanks!

diff --git a/pkg/types/current/types_test.go b/pkg/types/current/types_test.go
index cfa9fb1125c8d..35b82c5573279 100644
--- a/pkg/types/current/types_test.go
+++ b/pkg/types/current/types_test.go
@@ -21,6 +21,7 @@ import (
 	"os"
 
 	"github.com/containernetworking/cni/pkg/types"
+	types020 "github.com/containernetworking/cni/pkg/types/020"
 	"github.com/containernetworking/cni/pkg/types/current"
 
 	. "github.com/onsi/ginkgo"
@@ -214,6 +215,126 @@ var _ = Describe("Current types operations", func() {
 }`))
 	})
 
+	It("correctly round-trips a 0.2.0 Result with route gateways", 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 := &types020.Result{
+			CNIVersion: types020.ImplementedSpecVersion,
+			IP4: &types020.IPConfig{
+				IP:      *ipv4,
+				Gateway: net.ParseIP("1.2.3.1"),
+				Routes: []types.Route{
+					{Dst: *routev4, GW: routegwv4},
+				},
+			},
+			IP6: &types020.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]}"))
+
+		// Convert to current
+		newRes, err := current.NewResultFromResult(res)
+		Expect(err).NotTo(HaveOccurred())
+		// Convert back to 0.2.0
+		oldRes, err := newRes.GetAsVersion("0.2.0")
+		Expect(err).NotTo(HaveOccurred())
+
+		// Match JSON so we can figure out what's wrong if something fails the test
+		origJson, err := json.Marshal(res)
+		Expect(err).NotTo(HaveOccurred())
+		oldJson, err := json.Marshal(oldRes)
+		Expect(err).NotTo(HaveOccurred())
+		Expect(oldJson).To(MatchJSON(origJson))
+	})
+
+	It("correctly round-trips a 0.2.0 Result without route gateways", func() {
+		ipv4, err := types.ParseCIDR("1.2.3.30/24")
+		Expect(err).NotTo(HaveOccurred())
+		Expect(ipv4).NotTo(BeNil())
+
+		_, routev4, err := net.ParseCIDR("15.5.6.0/24")
+		Expect(err).NotTo(HaveOccurred())
+		Expect(routev4).NotTo(BeNil())
+
+		ipv6, err := types.ParseCIDR("abcd:1234:ffff::cdde/64")
+		Expect(err).NotTo(HaveOccurred())
+		Expect(ipv6).NotTo(BeNil())
+
+		_, routev6, err := net.ParseCIDR("1111:dddd::aaaa/80")
+		Expect(err).NotTo(HaveOccurred())
+		Expect(routev6).NotTo(BeNil())
+
+		// Set every field of the struct to ensure source compatibility
+		res := &types020.Result{
+			CNIVersion: types020.ImplementedSpecVersion,
+			IP4: &types020.IPConfig{
+				IP:      *ipv4,
+				Gateway: net.ParseIP("1.2.3.1"),
+				Routes: []types.Route{
+					{Dst: *routev4},
+				},
+			},
+			IP6: &types020.IPConfig{
+				IP:      *ipv6,
+				Gateway: net.ParseIP("abcd:1234:ffff::1"),
+				Routes: []types.Route{
+					{Dst: *routev6},
+				},
+			},
+			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:<nil>}]}, IP6:{IP:{IP:abcd:1234:ffff::cdde Mask:ffffffffffffffff0000000000000000} Gateway:abcd:1234:ffff::1 Routes:[{Dst:{IP:1111:dddd:: Mask:ffffffffffffffffffff000000000000} GW:<nil>}]}, DNS:{Nameservers:[1.2.3.4 1::cafe] Domain:acompany.com Search:[somedomain.com otherdomain.net] Options:[foo bar]}"))
+
+		// Convert to current
+		newRes, err := current.NewResultFromResult(res)
+		Expect(err).NotTo(HaveOccurred())
+		// Convert back to 0.2.0
+		oldRes, err := newRes.GetAsVersion("0.2.0")
+		Expect(err).NotTo(HaveOccurred())
+
+		// Match JSON so we can figure out what's wrong if something fails the test
+		origJson, err := json.Marshal(res)
+		Expect(err).NotTo(HaveOccurred())
+		oldJson, err := json.Marshal(oldRes)
+		Expect(err).NotTo(HaveOccurred())
+		Expect(oldJson).To(MatchJSON(origJson))
+	})
+
 	It("correctly marshals and unmarshals interface index 0", func() {
 		ipc := &current.IPConfig{
 			Version:   "4",

[luksa]

@dcbw Added your tests & rebased to latest master

[dcbw]

/lgtm