From 5a408187d4185058a3910fdca4a80ad51195f43b Mon Sep 17 00:00:00 2001 From: Miguel Duarte Barroso Date: Tue, 22 Jun 2021 19:17:00 +0200 Subject: [PATCH 1/5] static ipam: show confusing error msg Signed-off-by: Miguel Duarte Barroso --- plugins/ipam/static/static_test.go | 32 ++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/plugins/ipam/static/static_test.go b/plugins/ipam/static/static_test.go index deb1bbadc..a109cc173 100644 --- a/plugins/ipam/static/static_test.go +++ b/plugins/ipam/static/static_test.go @@ -546,6 +546,38 @@ var _ = Describe("static Operations", func() { }) Expect(err).Should(MatchError("IPAM config missing 'ipam' key")) }) + + It(fmt.Sprintf("[%s] errors when passed an invalid CIDR", ver), func() { + const ifname string = "eth0" + const nspath string = "/some/where" + const ipStr string = "10.10.0.1" + + conf := fmt.Sprintf(`{ + "cniVersion": "%s", + "name": "mynet", + "type": "bridge", + "ipam": { + "type": "static", + "addresses": [ { + "address": "%s" + }] + } + }`, ver, ipStr) + + args := &skel.CmdArgs{ + ContainerID: "dummy", + Netns: nspath, + IfName: ifname, + StdinData: []byte(conf), + } + + // Allocate the IP + _, _, err := testutils.CmdAddWithArgs(args, func() error { + return cmdAdd(args) + }) + Expect(err).Should( + MatchError(fmt.Sprintf("invalid CIDR %s: invalid CIDR address: %s", ipStr, ipStr))) + }) } }) From 0db5882a127da0d45982e1453694a32fd3615f58 Mon Sep 17 00:00:00 2001 From: Miguel Duarte Barroso Date: Tue, 22 Jun 2021 19:26:00 +0200 Subject: [PATCH 2/5] static ipam: stop wrapping net.ParseCIDR errors Signed-off-by: Miguel Duarte Barroso --- plugins/ipam/static/main.go | 4 ++-- plugins/ipam/static/static_test.go | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/plugins/ipam/static/main.go b/plugins/ipam/static/main.go index d7eaeab11..ed75d1fe2 100644 --- a/plugins/ipam/static/main.go +++ b/plugins/ipam/static/main.go @@ -161,7 +161,7 @@ func LoadIPAMConfig(bytes []byte, envArgs string) (*IPAMConfig, string, error) { ip, subnet, err := net.ParseCIDR(ipstr) if err != nil { - return nil, "", fmt.Errorf("invalid CIDR %s: %s", ipstr, err) + return nil, "", err } addr := Address{ @@ -213,7 +213,7 @@ func LoadIPAMConfig(bytes []byte, envArgs string) (*IPAMConfig, string, error) { for i := range n.IPAM.Addresses { ip, addr, err := net.ParseCIDR(n.IPAM.Addresses[i].AddressStr) if err != nil { - return nil, "", fmt.Errorf("invalid CIDR %s: %s", n.IPAM.Addresses[i].AddressStr, err) + return nil, "", err } n.IPAM.Addresses[i].Address = *addr n.IPAM.Addresses[i].Address.IP = ip diff --git a/plugins/ipam/static/static_test.go b/plugins/ipam/static/static_test.go index a109cc173..47a0078ae 100644 --- a/plugins/ipam/static/static_test.go +++ b/plugins/ipam/static/static_test.go @@ -576,7 +576,7 @@ var _ = Describe("static Operations", func() { return cmdAdd(args) }) Expect(err).Should( - MatchError(fmt.Sprintf("invalid CIDR %s: invalid CIDR address: %s", ipStr, ipStr))) + MatchError(fmt.Sprintf("invalid CIDR address: %s", ipStr))) }) } }) From a786b12b68ac5bf01a594da0faf51e58659b22b7 Mon Sep 17 00:00:00 2001 From: Miguel Duarte Barroso Date: Wed, 23 Jun 2021 12:27:58 +0200 Subject: [PATCH 3/5] static ipam: decide wrong cidr error msg Signed-off-by: Miguel Duarte Barroso --- plugins/ipam/static/main.go | 5 +++-- plugins/ipam/static/static_test.go | 4 ++-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/plugins/ipam/static/main.go b/plugins/ipam/static/main.go index ed75d1fe2..9156cad4a 100644 --- a/plugins/ipam/static/main.go +++ b/plugins/ipam/static/main.go @@ -161,7 +161,7 @@ func LoadIPAMConfig(bytes []byte, envArgs string) (*IPAMConfig, string, error) { ip, subnet, err := net.ParseCIDR(ipstr) if err != nil { - return nil, "", err + return nil, "", fmt.Errorf("the 'ip' field is expected to be in CIDR notation, got: '%s'", ipstr) } addr := Address{ @@ -213,7 +213,8 @@ func LoadIPAMConfig(bytes []byte, envArgs string) (*IPAMConfig, string, error) { for i := range n.IPAM.Addresses { ip, addr, err := net.ParseCIDR(n.IPAM.Addresses[i].AddressStr) if err != nil { - return nil, "", err + return nil, "", fmt.Errorf( + "the 'address' field is expected to be in CIDR notation, got: '%s'", n.IPAM.Addresses[i].AddressStr) } n.IPAM.Addresses[i].Address = *addr n.IPAM.Addresses[i].Address.IP = ip diff --git a/plugins/ipam/static/static_test.go b/plugins/ipam/static/static_test.go index 47a0078ae..5ce78096c 100644 --- a/plugins/ipam/static/static_test.go +++ b/plugins/ipam/static/static_test.go @@ -575,8 +575,8 @@ var _ = Describe("static Operations", func() { _, _, err := testutils.CmdAddWithArgs(args, func() error { return cmdAdd(args) }) - Expect(err).Should( - MatchError(fmt.Sprintf("invalid CIDR address: %s", ipStr))) + Expect(err).Should(MatchError( + fmt.Sprintf("the 'address' field is expected to be in CIDR notation, got: '%s'", ipStr))) }) } }) From 2052c30acd353052959034c6323ac9d844834de8 Mon Sep 17 00:00:00 2001 From: Miguel Duarte Barroso Date: Fri, 25 Jun 2021 12:57:37 +0100 Subject: [PATCH 4/5] static ipam: improve error msgs when provisioning invalid CIDR This commit addresses the scenarios when the invalid CIDR is provisioned via: - CNI_ARGS - RuntimeConfig Signed-off-by: Miguel Duarte Barroso --- plugins/ipam/static/main.go | 8 +++ plugins/ipam/static/static_test.go | 101 ++++++++++++++++++++++++++++- 2 files changed, 108 insertions(+), 1 deletion(-) diff --git a/plugins/ipam/static/main.go b/plugins/ipam/static/main.go index 9156cad4a..514622b1f 100644 --- a/plugins/ipam/static/main.go +++ b/plugins/ipam/static/main.go @@ -193,6 +193,10 @@ func LoadIPAMConfig(bytes []byte, envArgs string) (*IPAMConfig, string, error) { // args IP overwrites IP, so clear IPAM Config n.IPAM.Addresses = make([]Address, 0, len(n.Args.A.IPs)) for _, addr := range n.Args.A.IPs { + _, _, err := net.ParseCIDR(addr) + if err != nil { + return nil, "", fmt.Errorf("an entry in the 'ips' field is NOT in CIDR notation, got: '%s'", addr) + } n.IPAM.Addresses = append(n.IPAM.Addresses, Address{AddressStr: addr}) } } @@ -202,6 +206,10 @@ func LoadIPAMConfig(bytes []byte, envArgs string) (*IPAMConfig, string, error) { // runtimeConfig IP overwrites IP, so clear IPAM Config n.IPAM.Addresses = make([]Address, 0, len(n.RuntimeConfig.IPs)) for _, addr := range n.RuntimeConfig.IPs { + _, _, err := net.ParseCIDR(addr) + if err != nil { + return nil, "", fmt.Errorf("an entry in the 'ips' field is NOT in CIDR notation, got: '%s'", addr) + } n.IPAM.Addresses = append(n.IPAM.Addresses, Address{AddressStr: addr}) } } diff --git a/plugins/ipam/static/static_test.go b/plugins/ipam/static/static_test.go index 5ce78096c..f1d4c017a 100644 --- a/plugins/ipam/static/static_test.go +++ b/plugins/ipam/static/static_test.go @@ -547,7 +547,7 @@ var _ = Describe("static Operations", func() { Expect(err).Should(MatchError("IPAM config missing 'ipam' key")) }) - It(fmt.Sprintf("[%s] errors when passed an invalid CIDR", ver), func() { + It(fmt.Sprintf("[%s] errors when passed an invalid CIDR via ipam config", ver), func() { const ifname string = "eth0" const nspath string = "/some/where" const ipStr string = "10.10.0.1" @@ -578,6 +578,105 @@ var _ = Describe("static Operations", func() { Expect(err).Should(MatchError( fmt.Sprintf("the 'address' field is expected to be in CIDR notation, got: '%s'", ipStr))) }) + + It(fmt.Sprintf("[%s] errors when passed an invalid CIDR via Args", ver), func() { + const ifname string = "eth0" + const nspath string = "/some/where" + const ipStr string = "10.10.0.1" + + conf := fmt.Sprintf(`{ + "cniVersion": "%s", + "name": "mynet", + "type": "bridge", + "ipam": { + "type": "static", + "routes": [{ "dst": "0.0.0.0/0" }] + } + }`, ver) + + args := &skel.CmdArgs{ + ContainerID: "dummy", + Netns: nspath, + IfName: ifname, + StdinData: []byte(conf), + Args: fmt.Sprintf("IP=%s", ipStr), + } + + // Allocate the IP + _, _, err := testutils.CmdAddWithArgs(args, func() error { + return cmdAdd(args) + }) + Expect(err).Should(MatchError( + fmt.Sprintf("the 'ip' field is expected to be in CIDR notation, got: '%s'", ipStr))) + }) + + It(fmt.Sprintf("[%s] errors when passed an invalid CIDR via CNI_ARGS", ver), func() { + const ifname string = "eth0" + const nspath string = "/some/where" + const ipStr string = "10.10.0.1" + + conf := fmt.Sprintf(`{ + "cniVersion": "%s", + "name": "mynet", + "type": "bridge", + "ipam": { + "type": "static", + "routes": [{ "dst": "0.0.0.0/0" }] + }, + "args": { + "cni": { + "ips" : ["%s"] + } + } + }`, ver, ipStr) + + args := &skel.CmdArgs{ + ContainerID: "dummy", + Netns: nspath, + IfName: ifname, + StdinData: []byte(conf), + } + + // Allocate the IP + _, _, err := testutils.CmdAddWithArgs(args, func() error { + return cmdAdd(args) + }) + Expect(err).Should(MatchError( + fmt.Sprintf("an entry in the 'ips' field is NOT in CIDR notation, got: '%s'", ipStr))) + }) + + It(fmt.Sprintf("[%s] errors when passed an invalid CIDR via RuntimeConfig", ver), func() { + const ifname string = "eth0" + const nspath string = "/some/where" + const ipStr string = "10.10.0.1" + + conf := fmt.Sprintf(`{ + "cniVersion": "%s", + "name": "mynet", + "type": "bridge", + "ipam": { + "type": "static", + "routes": [{ "dst": "0.0.0.0/0" }] + }, + "RuntimeConfig": { + "ips" : ["%s"] + } + }`, ver, ipStr) + + args := &skel.CmdArgs{ + ContainerID: "dummy", + Netns: nspath, + IfName: ifname, + StdinData: []byte(conf), + } + + // Allocate the IP + _, _, err := testutils.CmdAddWithArgs(args, func() error { + return cmdAdd(args) + }) + Expect(err).Should(MatchError( + fmt.Sprintf("an entry in the 'ips' field is NOT in CIDR notation, got: '%s'", ipStr))) + }) } }) From 8ab23366fb09065838013bb464d83aaca1ac2f51 Mon Sep 17 00:00:00 2001 From: Miguel Duarte Barroso Date: Tue, 24 Aug 2021 13:19:31 +0200 Subject: [PATCH 5/5] static ipam: do not parse the CIDR twice With this patch, when the IPs are provisioned via CNI args or via `RuntimeConfig` the CIDR is only parsed once. Signed-off-by: Miguel Duarte Barroso --- plugins/ipam/static/main.go | 32 ++++++++++++++++++-------------- 1 file changed, 18 insertions(+), 14 deletions(-) diff --git a/plugins/ipam/static/main.go b/plugins/ipam/static/main.go index 514622b1f..38cd76161 100644 --- a/plugins/ipam/static/main.go +++ b/plugins/ipam/static/main.go @@ -192,12 +192,13 @@ func LoadIPAMConfig(bytes []byte, envArgs string) (*IPAMConfig, string, error) { if n.Args != nil && n.Args.A != nil && len(n.Args.A.IPs) != 0 { // args IP overwrites IP, so clear IPAM Config n.IPAM.Addresses = make([]Address, 0, len(n.Args.A.IPs)) - for _, addr := range n.Args.A.IPs { - _, _, err := net.ParseCIDR(addr) + for _, addrStr := range n.Args.A.IPs { + ip, addr, err := net.ParseCIDR(addrStr) if err != nil { - return nil, "", fmt.Errorf("an entry in the 'ips' field is NOT in CIDR notation, got: '%s'", addr) + return nil, "", fmt.Errorf("an entry in the 'ips' field is NOT in CIDR notation, got: '%s'", addrStr) } - n.IPAM.Addresses = append(n.IPAM.Addresses, Address{AddressStr: addr}) + addr.IP = ip + n.IPAM.Addresses = append(n.IPAM.Addresses, Address{AddressStr: addrStr, Address: *addr}) } } @@ -205,12 +206,13 @@ func LoadIPAMConfig(bytes []byte, envArgs string) (*IPAMConfig, string, error) { if len(n.RuntimeConfig.IPs) != 0 { // runtimeConfig IP overwrites IP, so clear IPAM Config n.IPAM.Addresses = make([]Address, 0, len(n.RuntimeConfig.IPs)) - for _, addr := range n.RuntimeConfig.IPs { - _, _, err := net.ParseCIDR(addr) + for _, addrStr := range n.RuntimeConfig.IPs { + ip, addr, err := net.ParseCIDR(addrStr) if err != nil { - return nil, "", fmt.Errorf("an entry in the 'ips' field is NOT in CIDR notation, got: '%s'", addr) + return nil, "", fmt.Errorf("an entry in the 'ips' field is NOT in CIDR notation, got: '%s'", addrStr) } - n.IPAM.Addresses = append(n.IPAM.Addresses, Address{AddressStr: addr}) + addr.IP = ip + n.IPAM.Addresses = append(n.IPAM.Addresses, Address{AddressStr: addrStr, Address: *addr}) } } @@ -219,13 +221,15 @@ func LoadIPAMConfig(bytes []byte, envArgs string) (*IPAMConfig, string, error) { numV6 := 0 for i := range n.IPAM.Addresses { - ip, addr, err := net.ParseCIDR(n.IPAM.Addresses[i].AddressStr) - if err != nil { - return nil, "", fmt.Errorf( - "the 'address' field is expected to be in CIDR notation, got: '%s'", n.IPAM.Addresses[i].AddressStr) + if n.IPAM.Addresses[i].Address.IP == nil { + ip, addr, err := net.ParseCIDR(n.IPAM.Addresses[i].AddressStr) + if err != nil { + return nil, "", fmt.Errorf( + "the 'address' field is expected to be in CIDR notation, got: '%s'", n.IPAM.Addresses[i].AddressStr) + } + n.IPAM.Addresses[i].Address = *addr + n.IPAM.Addresses[i].Address.IP = ip } - n.IPAM.Addresses[i].Address = *addr - n.IPAM.Addresses[i].Address.IP = ip if err := canonicalizeIP(&n.IPAM.Addresses[i].Address.IP); err != nil { return nil, "", fmt.Errorf("invalid address %d: %s", i, err)