Fix confusing error msg invalid cidr #638
Conversation
|
You pasted the same link twice: |
Good catch. Corrected the second link. |
maiqueb
force-pushed
the
fix-confusing-error-msg-invalid-cidr
branch
from
a819b97
to
fc3c05e
Compare
|
/lgtm |
Signed-off-by: Miguel Duarte Barroso <mdbarroso@redhat.com>
Signed-off-by: Miguel Duarte Barroso <mdbarroso@redhat.com>
Signed-off-by: Miguel Duarte Barroso <mdbarroso@redhat.com>
maiqueb
force-pushed
the
fix-confusing-error-msg-invalid-cidr
branch
from
fc3c05e
to
b370fa0
Compare
plugins/ipam/static/main.go
Outdated
| @@ -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) | |||
There was a problem hiding this comment.
the string-format IP(or CIDR) will be parsed later, so why we check this twice?
There was a problem hiding this comment.
This was the way I found of indicating which parameter was the offending one (the string format CIDR validation will be performed for cni args / ipam config later on, and its impossible to know the parameter name by then).
But I agree with you - IMO, just saying we expect CIDR notation should be enough.
There was a problem hiding this comment.
OK, since the parse and validation already done here, could we append a parsed Address instead of a single string to avoid this to be parsed later?
There was a problem hiding this comment.
I am not entirely fond of how it looks, but have provided a new commit w/ this change.
Let me know if that's OK.
This commit addresses the scenarios when the invalid CIDR is provisioned via: - CNI_ARGS - RuntimeConfig Signed-off-by: Miguel Duarte Barroso <mdbarroso@redhat.com>
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 <mdbarroso@redhat.com>
maiqueb
force-pushed
the
fix-confusing-error-msg-invalid-cidr
branch
from
b370fa0
to
8ab2336
Compare
|
@mars1024 also, for whatever reason, CI stopped running the tests automatically; it complains a maintainer must approve testing for it. Can you grant it ? |
There was a problem hiding this comment.
Thanks for the improvements, I think with this error messaging, it's more actionable for a user (they now can see that we're saying "hey, a.b.c.d that's not in CIDR format, you should change it." instead of just "this is wrong: a.b.c.d." (which doesn't tell us it's in the wrong format)
|
/lgtm |
By wrapping the error msg we ended up actually duplicating the exact same msg
net.ParseCIDRreplies, as seen in its implementation - [0] & [1]This PR makes the error msg less confusing.
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1897431
[0] - https://github.com/golang/go/blob/2ebe77a2fda1ee9ff6fd9a3e08933ad1ebaea039/src/net/net.go#L540
[1] - https://github.com/golang/go/blob/2ebe77a2fda1ee9ff6fd9a3e08933ad1ebaea039/src/net/ip.go#L739