Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix confusing error msg invalid cidr #638

Merged
merged 5 commits into from
Sep 16, 2021
Merged

Fix confusing error msg invalid cidr #638

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5a40818
static ipam: show confusing error msg
maiqueb Jun 22, 2021
0db5882
static ipam: stop wrapping net.ParseCIDR errors
maiqueb Jun 22, 2021
a786b12
static ipam: decide wrong cidr error msg
maiqueb Jun 23, 2021
2052c30
static ipam: improve error msgs when provisioning invalid CIDR
maiqueb Jun 25, 2021
8ab2336
static ipam: do not parse the CIDR twice
maiqueb Aug 24, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 23 additions & 10 deletions plugins/ipam/static/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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, "", fmt.Errorf("the 'ip' field is expected to be in CIDR notation, got: '%s'", ipstr)
maiqueb marked this conversation as resolved.
Show resolved Hide resolved
}

addr := Address{
Expand Down Expand Up @@ -192,17 +192,27 @@ 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 {
n.IPAM.Addresses = append(n.IPAM.Addresses, Address{AddressStr: 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'", addrStr)
}
addr.IP = ip
n.IPAM.Addresses = append(n.IPAM.Addresses, Address{AddressStr: addrStr, Address: *addr})
}
}

// import address from runtimeConfig
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 {
n.IPAM.Addresses = append(n.IPAM.Addresses, Address{AddressStr: 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'", addrStr)
}
addr.IP = ip
n.IPAM.Addresses = append(n.IPAM.Addresses, Address{AddressStr: addrStr, Address: *addr})
}
}

Expand All @@ -211,12 +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("invalid CIDR %s: %s", n.IPAM.Addresses[i].AddressStr, err)
if n.IPAM.Addresses[i].Address.IP == nil {
maiqueb marked this conversation as resolved.
Show resolved Hide resolved
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)
Expand Down
131 changes: 131 additions & 0 deletions plugins/ipam/static/static_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -546,6 +546,137 @@ var _ = Describe("static Operations", func() {
})
Expect(err).Should(MatchError("IPAM config missing 'ipam' key"))
})

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"

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("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)))
})
}
})

Expand Down