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

Jump to bottom

OCPBUGS-39403 - Fix parseIPList to Continue Processing After Invalid IPs and Return Valid IPs #621

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

OCPBUGS-39403 - Fix parseIPList to Continue Processing After Invalid IPs and Return Valid IPs #621

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
30 changes: 22 additions & 8 deletions pkg/router/template/template_helper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -367,26 +367,40 @@ func parseIPList(list string) string {

trimmedList := strings.TrimSpace(list)
if trimmedList == "" {
log.V(0).Info("parseIPList empty list found")
return ""
}

// same behavior as the previous approach with regexp
if trimmedList != list {
log.V(7).Info("parseIPList leading/trailing spaces found")
log.V(0).Info("parseIPList leading/trailing spaces found")
return ""
}

var validIPs []string

ipList := strings.Fields(list)
for _, ip := range ipList {
if net.ParseIP(ip) == nil {
if _, _, err := net.ParseCIDR(ip); err != nil {
log.V(7).Info("parseIPList found not IP/CIDR item", "value", ip, "err", err)
return ""
}
// check if it's a valid IP
if net.ParseIP(ip) != nil {
validIPs = append(validIPs, ip)
} else if _, _, err := net.ParseCIDR(ip); err == nil {
// check if it's a valid CIDR
validIPs = append(validIPs, ip)
} else {
// Log invalid IP/CIDR
log.V(0).Info("parseIPList found invalid IP/CIDR", ip)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this changes behavior, we need to make sure this error is noticed, and indicate that the invalid IP is now ignored. Something like this:

Suggested change
log.V(0).Info("parseIPList found invalid IP/CIDR", ip)
log.Error("error parsing IP list, ignoring invalid IP/CIDR", ip)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a customErr for IP/CIDR since only net.parseCIDR returns err.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering whether this one should be an error, taking into account that now we don't stop the parsing and there can be some valid IPs still. I think that the error would make more sense down the code where we find that the list is empty. However in the similar conditions in the same function (when we return "") we use log.V(7).Info(). So it seems to be inconsistent.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alebedev87 That’s why I use log.V(0).Info at the beginning.
I reviewed all scenarios that call log.Error() in template_helper.go, and they all return "" immediately.
Since, in this case, we will continue parsing the rest of the IP, is it okay to use log.Info but at a higher log level?
If yes, what level should I use here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default log level is 2 so if we want this message to be visible it has to be <= 2. From what I can see in template_helper.go the log level 0 is what's used in similar cases (example) - when the user needs to be informed but the flow may continue. Similarly, looking at the other functions - failing the parsing with an error if the list of valid IPs/CIDRs is empty seems to be the way to go.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw the condition about the trimmer list being compared to the given list doesn't seem to use the right logging level:

	if trimmedList != list {
		log.V(7).Info("parseIPList leading/trailing spaces found")
		return ""
	}

I would prefer it to be a level 0 too as we want the user to see this.

}
}
log.V(7).Info("parseIPList parsed the list", "value", list)
return list

if len(validIPs) == 0 {
log.V(0).Info("No valid IP/CIDR in the list")
return ""
}

result := strings.Join(validIPs, " ")
log.V(7).Info("parseIPList parsed the list", "validIPs", result)
return result
}

var helperFunctions = template.FuncMap{
Expand Down
90 changes: 71 additions & 19 deletions pkg/router/template/template_helper_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -981,33 +981,40 @@ func TestGenerateHAProxyWhiteListFile(t *testing.T) {

func TestParseIPList(t *testing.T) {
testCases := []struct {
name string
input string
expectedEmpty bool
name string
input string
expectedEmpty bool
expectedReturn string
}{
{
name: "All mixed",
input: "192.168.1.0 2001:0db8:85a3:0000:0000:8a2e:0370:7334 172.16.14.10/24 2001:0db8:85a3::8a2e:370:10/64 64:ff9b::192.168.0.1 2600:14a0::/40",
name: "All mixed",
input: "192.168.1.0 2001:0db8:85a3:0000:0000:8a2e:0370:7334 172.16.14.10/24 2001:0db8:85a3::8a2e:370:10/64 64:ff9b::192.168.0.1 2600:14a0::/40",
expectedReturn: "192.168.1.0 2001:0db8:85a3:0000:0000:8a2e:0370:7334 172.16.14.10/24 2001:0db8:85a3::8a2e:370:10/64 64:ff9b::192.168.0.1 2600:14a0::/40",
},
{
name: "IPs only",
input: "192.168.1.0 2001:0db8:85a3:0000:0000:8a2e:0370:7334 64:ff9b::192.168.0.1 172.16.14.10",
name: "IPs only",
input: "192.168.1.0 2001:0db8:85a3:0000:0000:8a2e:0370:7334 64:ff9b::192.168.0.1 172.16.14.10",
expectedReturn: "192.168.1.0 2001:0db8:85a3:0000:0000:8a2e:0370:7334 64:ff9b::192.168.0.1 172.16.14.10",
},
{
name: "CIDRs only",
input: "192.168.1.0/16 2001:0db8:85a3:0000:0000:8a2e:0370:7334/48 172.16.14.10/24 2001:0db8:85a3::8a2e:0370:10/64 2600:14a0::/40",
name: "CIDRs only",
input: "192.168.1.0/16 2001:0db8:85a3:0000:0000:8a2e:0370:7334/48 172.16.14.10/24 2001:0db8:85a3::8a2e:0370:10/64 2600:14a0::/40",
expectedReturn: "192.168.1.0/16 2001:0db8:85a3:0000:0000:8a2e:0370:7334/48 172.16.14.10/24 2001:0db8:85a3::8a2e:0370:10/64 2600:14a0::/40",
},
{
name: "IPv6 only",
input: "2001:0db8:85a3:0000:0000:8a2e:0370:7334 2001:0db8:85a3::8a2e:370:10/64 2001:db8::2:1 ::ffff:192.168.0.1 2600:14a0::/40",
name: "IPv6 only",
input: "2001:0db8:85a3:0000:0000:8a2e:0370:7334 2001:0db8:85a3::8a2e:370:10/64 2001:db8::2:1 ::ffff:192.168.0.1 2600:14a0::/40",
expectedReturn: "2001:0db8:85a3:0000:0000:8a2e:0370:7334 2001:0db8:85a3::8a2e:370:10/64 2001:db8::2:1 ::ffff:192.168.0.1 2600:14a0::/40",
},
{
name: "IPv4 only",
input: "192.168.10.10 10.168.12.10/8 8.8.8.8 172.16.0.0/24",
name: "IPv4 only",
input: "192.168.10.10 10.168.12.10/8 8.8.8.8 172.16.0.0/24",
expectedReturn: "192.168.10.10 10.168.12.10/8 8.8.8.8 172.16.0.0/24",
},
{
name: "Single IP",
input: "192.168.15.15",
name: "Single IP",
input: "192.168.15.15",
expectedReturn: "192.168.15.15",
},
{
// as behavior as the previous (regexp) approach
Expand Down Expand Up @@ -1046,9 +1053,39 @@ func TestParseIPList(t *testing.T) {
expectedEmpty: true,
},
{
name: "Wrong IP in a list",
input: "192.168.1.0 2001:0db8:85a3:0000:0000:8a2e:0370:7334 172.16.14.10/24 2001:0db8:85a3::8a2e:370:10/64 64:ff9b::192.168.0.1 10.",
expectedEmpty: true,
name: "Wrong IPv4 in an IPs only list",
input: "192.168.1.0 2001:0db8:85a3:0000:0000:8a2e:0370:7334 172.16.14.10/24 2001:0db8:85a3::8a2e:370:10/64 64:ff9b::192.168.0.1 10.",
expectedReturn: "192.168.1.0 2001:0db8:85a3:0000:0000:8a2e:0370:7334 172.16.14.10/24 2001:0db8:85a3::8a2e:370:10/64 64:ff9b::192.168.0.1",
},
{
name: "Wrong IPv6 in an IPs only list",
input: "192.168.1.0 2001:0db8:85a3:0000:0000:8a2e:0370 172.16.14.10/24 2001:0db8:85a3::8a2e:370:10/64 64:ff9b::192.168.0.1 10.",
expectedReturn: "192.168.1.0 172.16.14.10/24 2001:0db8:85a3::8a2e:370:10/64 64:ff9b::192.168.0.1",
},
{
name: "Wrong IPv4 in an IPv4 list",
input: "192.168.1.0 10.10.0.1 192.168. 10.",
expectedReturn: "192.168.1.0 10.10.0.1",
},
{
name: "Wrong IPv6 in an IPv6 list",
input: "2001:0db8:85a3:0000:8a2e:0370:7334 2001:0db8:85a3::8a2e:370:10/64 2001:db8::2:1 ::ffff:192.168.0.1 :/40",
expectedReturn: "2001:0db8:85a3::8a2e:370:10/64 2001:db8::2:1 ::ffff:192.168.0.1",
},
{
name: "All mixed type with invalid IPv4",
input: "192.168.1 2001:0db8:85a3:0000:0000:8a2e:0370:7334 172.16.14.10/24 2001:0db8:85a3::8a2e:370:10/64 64:ff9b::192.168.0.1 2600:14a0::/40",
expectedReturn: "2001:0db8:85a3:0000:0000:8a2e:0370:7334 172.16.14.10/24 2001:0db8:85a3::8a2e:370:10/64 64:ff9b::192.168.0.1 2600:14a0::/40",
},
{
name: "Wrong IPv4 CIDR in a CIDRs only list",
input: "192.168.1./16 2001:0db8:85a3:0000:0000:8a2e:0370:7334/48 172.16.14.10/24 2001:0db8:85a3::8a2e:0370:10/64 2600:14a0::/40",
expectedReturn: "2001:0db8:85a3:0000:0000:8a2e:0370:7334/48 172.16.14.10/24 2001:0db8:85a3::8a2e:0370:10/64 2600:14a0::/40",
},
{
name: "Wrong IPv6 CIDR in a CIDRs only list",
input: "192.168.1.0/16 2001:0db8:85a3:0000:0000:8a2e:0370/48 172.16.14.10/24 2001:0db8:85a3::8a2e:0370:10/64 2600:14a0::/40",
expectedReturn: "192.168.1.0/16 172.16.14.10/24 2001:0db8:85a3::8a2e:0370:10/64 2600:14a0::/40",
},
}

Expand All @@ -1061,9 +1098,24 @@ func TestParseIPList(t *testing.T) {
}
return
}
if got != tc.input {
if tc.expectedEmpty && got != "" || got != tc.expectedReturn {
t.Errorf("Failure: expected %q, got %q", tc.input, got)
Copy link
Contributor

@candita candita Oct 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
t.Errorf("Failure: expected %q, got %q", tc.input, got)
if expectedEmpty {
log.Errorf("Failure: expected none, got %q", got)
} else {
t.Errorf("Failure: expected %q, got %q", tc.expectedReturn, got)
}

}
})
}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
got := parseIPList(tc.input)
if tc.expectedEmpty {
if got != "" {
t.Errorf("Expected empty, but got %q", got)
}
return
}
if got != tc.expectedReturn {
t.Errorf("Failure: expected %q, got %q", tc.expectedReturn, got)
}
})
}
}