Support proper parsing of IPs with leading zeros #3138

arkadeepsen · 2022-09-02T19:20:11Z

Changed all ParseIP and ParseCIDR functions of net package to ParseIPSloppy and ParseCIDRSloppy of k8s.io/utils/net package respectively for k8s API objects. This is done to allow IPs and CIDRs with leading zeros to be parsed without any error.

Additionally have added conversion of any IP in string format to net.IP format and then conversion to string format again to remove any leading zeros.

Following set of IP strings are validated:

service.Spec.ClusterIP
service.Spec.ClusterIPs
service.Spec.ExternalIPs
service.Status.LoadBalancer.Ingress[*].IP
node.Status.Addresses[*].Address (for address types NodeInternalIP and NodeExternalIP)
endpointSlice.Endpoints[*].Addresses
pod.Status.PodIP
pod.Status.PodIPs[*].IP

Signed-off-by: Arkadeep Sen arsen@redhat.com

Fixes: https://issues.redhat.com/browse/OCPBUGS-463

coveralls · 2022-09-03T14:34:55Z

Coverage increased (+0.003%) to 52.794% when pulling dbe0a91 on arkadeepsen:leading-zeros into c4fd149 on ovn-org:master.

arkadeepsen · 2022-09-05T05:28:50Z

@martinkennelly PTAL

arkadeepsen · 2022-09-05T12:13:59Z

/retest-failed

npinaeva · 2022-09-08T08:18:38Z

Hey @arkadeepsen, as we discussed, we need to add ips validation for all k8s objects that can set ips (and CIDRs)
So I'd suggest to have a list of these objects, and then create parsing functions, to make sure we parse every ip/cidr before working with it.
So we would have functions like util.GetClusterIPs(service) and it should return properly parsed ips, then we won't need duplicate code like this https://github.com/ovn-org/ovn-kubernetes/pull/3138/files#diff-bb96390aa4d292c5ff6e2bc554446182b8abd55e12ba2a0290853b92aa4e1155R417-R426
wdyt?

arkadeepsen · 2022-09-08T09:24:21Z

Hey @arkadeepsen, as we discussed, we need to add ips validation for all k8s objects that can set ips (and CIDRs) So I'd suggest to have a list of these objects, and then create parsing functions, to make sure we parse every ip/cidr before working with it. So we would have functions like util.GetClusterIPs(service) and it should return properly parsed ips, then we won't need duplicate code like this https://github.com/ovn-org/ovn-kubernetes/pull/3138/files#diff-bb96390aa4d292c5ff6e2bc554446182b8abd55e12ba2a0290853b92aa4e1155R417-R426 wdyt?

Makes more sense @npinaeva. I'll make the appropriate changes.

arkadeepsen · 2022-09-08T15:26:41Z

/retest

arkadeepsen · 2022-09-08T19:48:06Z

/retest-failed

ovn-robot · 2022-09-08T20:04:54Z

Oops, something went wrong:

This workflow is already running

arkadeepsen · 2022-09-09T06:32:55Z

/retest-failed

ovn-robot · 2022-09-09T06:34:04Z

Oops, something went wrong:

This workflow run cannot be retried

npinaeva · 2022-09-15T15:16:31Z

go-controller/pkg/cni/cniserver_test.go

@@ -17,11 +16,14 @@ import (
 	"strings"
 	"testing"

+	"k8s.io/client-go/kubernetes"
+


remove newline?

Sure. Will remove it.

npinaeva · 2022-09-19T09:06:05Z

I am a bit confused by changes like
c48d854#diff-bb96390aa4d292c5ff6e2bc554446182b8abd55e12ba2a0290853b92aa4e1155L429
and then
92b8cd3#diff-bb96390aa4d292c5ff6e2bc554446182b8abd55e12ba2a0290853b92aa4e1155R429
you basically did the change in the first commit and reverted it in the next. Can you please rebase commits so that they have final changes?
Also the commits as they are right now are bit difficult to review - maybe having commit that replaces net.Parse functions with parseSloppy and nothing else, and then next commits for changes related to objects you listed in the description will be easier to review?
Also, can you add some comments for unit test changes you did? I don't see why this 4e83aae change is required - it looks for me like ClusterIP is the right service type

arkadeepsen · 2022-09-19T10:08:02Z

I am a bit confused by changes like
c48d854#diff-bb96390aa4d292c5ff6e2bc554446182b8abd55e12ba2a0290853b92aa4e1155L429
and then
92b8cd3#diff-bb96390aa4d292c5ff6e2bc554446182b8abd55e12ba2a0290853b92aa4e1155R429
you basically did the change in the first commit and reverted it in the next. Can you please rebase commits so that they have final changes?
Also the commits as they are right now are bit difficult to review - maybe having commit that replaces net.Parse functions with parseSloppy and nothing else, and then next commits for changes related to objects you listed in the description will be easier to review?

I will make these changes and push separate commits.

Also, can you add some comments for unit test changes you did? I don't see why this 4e83aae change is required - it looks for me like ClusterIP is the right service type

The Status field of the Service has Loadbalancer Ingress IP set. I updated the buildServiceLBConfigs to use GetExternalAndLBIPs which adds an additional check on the type of Service and only fetches the LB IP if the type is set to LoadBalancer. As it was set to ClusterIP the unit test was failing.

arkadeepsen · 2022-09-19T12:52:20Z

I will make these changes and push separate commits.

@npinaeva I have pushed 2 separate commits. One for replacing the net.ParseIP and net.ParseCIDR, and the other for the changes related to the objects.

npinaeva · 2022-09-21T08:02:53Z

/lgtm
great work, thanks!

danwinship · 2022-09-22T17:03:38Z

go-controller/cmd/ovnkube-trace/ovnkube-trace.go

@@ -104,14 +105,14 @@ func (pi *PodInfo) String() string {
 }

 func (si SvcInfo) getL3Ver() string {
-	if net.ParseIP(si.ClusterIP).To4() != nil {
+	if utilnet.ParseIPSloppy(si.ClusterIP).To4() != nil {


You need to use the sloppy functions when parsing k8s API objects, but for other objects, it would be better to make sure that they only ever contain valid IP addresses. So, eg, here, it would be better to change getSvcInfo to set si.ClusterIP using ValidateAndGetIPString(). Or alternatively, make si.ClusterIP a net.IP rather than a string, and set it with utilnet.ParseIPSloppy(svc.Spec.ClusterIP).

(This comment applies everywhere; I'm not going to comment in each place.)

Sure. I'll make the appropriate changes. For parsing IPs and CIDRs from the annotation of various objects like namespace, pod, etc., should we use the sloppy version of functions or the parse functions from the net package?

If you have to deal with possibly-bad values in those annotations written by old versions of ovn-kubernetes, then you need to use the sloppy functions. If not, then you don't.

I checked the value assignment to the annotations of all the objects and didn't find any bad values (IP or CIDR) being assigned.

Wherever IP strings are being used directly from the k8s API objects I have used ValidateAndGetIPString to get the valid IP address first, and then assigned it to other variables or struct fields.

danwinship · 2022-09-22T17:09:41Z

go-controller/pkg/node/gateway_init_linux_test.go

-			_, dpuSubnet, _ := net.ParseCIDR("10.0.0.101/24")
-			nodeIP := net.ParseIP("10.0.1.11")
+			_, dpuSubnet, _ := utilnet.ParseCIDRSloppy("10.0.0.101/24")
+			nodeIP := utilnet.ParseIPSloppy("10.0.1.11")


You don't need to use the sloppy functions for const strings that you know are valid.

net.ParseIP and net.ParseCIDR should still be the default for parsing IPs/CIDRs. The sloppy versions should only be used in contexts where you need to accept invalid strings as user input for backward-compatibility reasons. (Basically, the parameter passed to ParseIPSloppy or ParseCIDRSloppy should almost always be exactly one of the items in your list in the initial PR description (service.Spec.ClusterIP, endpointSlice.Endpoints[*].Addresses, etc)

(EDIT: or, as above, if we were previously copying the bad values from those fields into an annotation, then you'll need to use the sloppy functions for that annotation too.)

I have reverted the changes in the unit test files where valid const strings are being used.

danwinship · 2022-09-22T17:20:11Z

go-controller/pkg/util/util.go

+
+// ValidateAndGetIPString validates the IP address and returns the IP in
+// string format removing any leading zeros.
+func ValidateAndGetIPString(ip string) (string, error) {


If you are only using this function for IPs in kubernetes API objects, then those fields have already been validated, so you know they contain IPs that are valid according to net.ParseIP, and it's just a matter of removing the leading zeros. In which case, you don't need to return an error.

I am still returning the error from this function, but I have removed the error checking whenever it's called for any k8s API object. If this function is used for other strings which are not valid IPs then it will panic. Though this function is only being used for k8s API objects as of now.

danwinship

The existing commits should be squashed
This appears to have a bunch of "go 1.19 gofmt changes" mixed in with the actual IP parsing changes. It would be better to undo those. If you can't, then this should wait until Update comment formatting for Go 1.19 #3207 merges and then be rebased on top of that.
The PR would be a lot simpler if you removed the unused error return from ValidateAndGetIPString. If someone needed a version that also works on unvalidated strings later, then they could add that later. You could also rename the function to make it clearer that it requires an already-validated IP string so people don't mistakenly use it on unvalidated IP strings later. But anyway, I'm not going to say you have to change that if you think this way is better.

lgtm with the first two points (at least) fixed, but I feel like someone who is still active in ovn-k development should do the actual merging

…Sloppy and ParseCIDRSloppy of k8s.io/utils/net package respectively for k8s API objects. This is done to allow IPs and CIDRs with leading zeros to be parsed without any error. Additionally have added conversion of any IP in string format to net.IP format and then conversion to string format again to remove any leading zeros. Signed-off-by: arkadeepsen <arsen@redhat.com>

arkadeepsen · 2022-10-13T10:37:36Z

The existing commits should be squashed

I have squashed the existing commits.

This appears to have a bunch of "go 1.19 gofmt changes" mixed in with the actual IP parsing changes. It would be better to undo those. If you can't, then this should wait until Update comment formatting for Go 1.19 #3207 merges and then be rebased on top of that.

I have undone those changes. I also have rebased after #3207 is merged.

The PR would be a lot simpler if you removed the unused error return from ValidateAndGetIPString. If someone needed a version that also works on unvalidated strings later, then they could add that later. You could also rename the function to make it clearer that it requires an already-validated IP string so people don't mistakenly use it on unvalidated IP strings later. But anyway, I'm not going to say you have to change that if you think this way is better.

I have removed the error checking. However, after removing the error checking it just becomes a single line function. So I have removed the function altogether and replaced wherever it was called with ParseIPSloppy(ip).String().

arkadeepsen force-pushed the leading-zeros branch 2 times, most recently from 70cc1a4 to c6aec2e Compare September 3, 2022 14:15

npinaeva self-requested a review September 7, 2022 09:03

arkadeepsen force-pushed the leading-zeros branch 4 times, most recently from 8c977e4 to 4e83aae Compare September 8, 2022 13:55

npinaeva reviewed Sep 15, 2022

View reviewed changes

arkadeepsen force-pushed the leading-zeros branch from c7f1217 to 965cff9 Compare September 16, 2022 13:47

arkadeepsen force-pushed the leading-zeros branch from be580db to ec3d128 Compare September 19, 2022 12:15

danwinship suggested changes Sep 22, 2022

View reviewed changes

arkadeepsen requested a review from danwinship September 27, 2022 13:15

danwinship suggested changes Oct 7, 2022

View reviewed changes

arkadeepsen force-pushed the leading-zeros branch from ff28120 to 882fcc6 Compare October 13, 2022 10:28

arkadeepsen force-pushed the leading-zeros branch from 882fcc6 to dbe0a91 Compare October 13, 2022 10:30

trozet approved these changes Oct 14, 2022

View reviewed changes

trozet merged commit fed2993 into ovn-org:master Oct 14, 2022

tssurya mentioned this pull request Mar 31, 2023

NextIP returns the wrong IP for IPv6 address with leading zeros #3511

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support proper parsing of IPs with leading zeros #3138

Support proper parsing of IPs with leading zeros #3138

arkadeepsen commented Sep 2, 2022 •

edited

coveralls commented Sep 3, 2022 •

edited

arkadeepsen commented Sep 5, 2022 •

edited

arkadeepsen commented Sep 5, 2022

npinaeva commented Sep 8, 2022

arkadeepsen commented Sep 8, 2022

arkadeepsen commented Sep 8, 2022

arkadeepsen commented Sep 8, 2022

ovn-robot commented Sep 8, 2022

arkadeepsen commented Sep 9, 2022

ovn-robot commented Sep 9, 2022

npinaeva Sep 15, 2022

arkadeepsen Sep 15, 2022

npinaeva commented Sep 19, 2022

arkadeepsen commented Sep 19, 2022

arkadeepsen commented Sep 19, 2022

npinaeva commented Sep 21, 2022

danwinship Sep 22, 2022

arkadeepsen Sep 23, 2022

danwinship Sep 26, 2022

arkadeepsen Sep 27, 2022

danwinship Sep 22, 2022 •

edited

arkadeepsen Sep 27, 2022

danwinship Sep 22, 2022

arkadeepsen Sep 27, 2022

danwinship left a comment

arkadeepsen commented Oct 13, 2022

Support proper parsing of IPs with leading zeros #3138

Support proper parsing of IPs with leading zeros #3138

Conversation

arkadeepsen commented Sep 2, 2022 • edited

coveralls commented Sep 3, 2022 • edited

arkadeepsen commented Sep 5, 2022 • edited

arkadeepsen commented Sep 5, 2022

npinaeva commented Sep 8, 2022

arkadeepsen commented Sep 8, 2022

arkadeepsen commented Sep 8, 2022

arkadeepsen commented Sep 8, 2022

ovn-robot commented Sep 8, 2022

arkadeepsen commented Sep 9, 2022

ovn-robot commented Sep 9, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

npinaeva commented Sep 19, 2022

arkadeepsen commented Sep 19, 2022

arkadeepsen commented Sep 19, 2022

npinaeva commented Sep 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danwinship Sep 22, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danwinship left a comment

Choose a reason for hiding this comment

arkadeepsen commented Oct 13, 2022

arkadeepsen commented Sep 2, 2022 •

edited

coveralls commented Sep 3, 2022 •

edited

arkadeepsen commented Sep 5, 2022 •

edited

danwinship Sep 22, 2022 •

edited