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

mJace · 2024-08-30T06:55:39Z

Summary

This PR updates the parseIPList function to handle IP lists containing invalid IPs or CIDRs more gracefully. Previously, the function would return an empty string as soon as it encountered an invalid IP or CIDR. This update ensures that the function will now continue processing the rest of the list and return a space-separated string of all valid IPs and CIDRs.

Changes

Modified parseIPList to:
- Log all invalid entries for troubleshooting.
- Return a string of valid IPs and CIDRs, or an empty string if none are valid.

Benefits

Improved Robustness: The function now handles mixed validity IP lists more gracefully, allowing valid entries to be processed even if there are some invalid entries.
Enhanced Debugging: Logging of invalid IPs or CIDRs provides better visibility into issues with input data.

Additional Notes

The function continues to trim and validate input while logging detailed information about invalid entries.
Ensure to review and test the function with various IP list scenarios to confirm the correctness of the changes.

openshift-ci · 2024-08-30T06:55:59Z

Hi @mJace. Thanks for your PR.

I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

alebedev87 · 2024-09-03T11:55:27Z

/assign

alebedev87 · 2024-10-02T15:24:20Z

@mJace : would it be possible for you to provide some unit tests for this change?

candita · 2024-10-02T19:02:54Z

/assign

openshift-ci · 2024-10-10T08:53:46Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from alebedev87. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

mJace · 2024-10-10T08:56:52Z

@alebedev87
I added more test case to function TestParseIPList in pkg/router/template/template_helper_test.go.

candita · 2024-10-23T19:58:22Z

pkg/router/template/template_helper.go

-	return list
+
+	if len(invalidIPs) > 0 {
+		log.V(7).Info("parseIPList found invalid IP/CIDR items", "invalidIPs", invalidIPs)


I would keep the log line up at line 391 where you find an invalid IP, e.g. log it for each invalid ip. Then you don't need the invalidIPs slice, nor to take a len later.

Thank you for your advice.

candita · 2024-10-23T19:59:13Z

/ok-to-test

mJace · 2024-10-24T01:55:51Z

/retest-required

mJace · 2024-10-24T05:57:49Z

/retest

candita · 2024-10-24T16:24:56Z

pkg/router/template/template_helper.go

+			validIPs = append(validIPs, ip)
+		} else {
+			// Log invalid IP/CIDR
+			log.V(0).Info("parseIPList found invalid IP/CIDR", ip)


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)

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

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.

@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?

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.

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.

pkg/router/template/template_helper_test.go

candita · 2024-10-24T16:37:36Z

pkg/router/template/template_helper_test.go

@@ -1061,7 +1112,7 @@ func TestParseIPList(t *testing.T) {
 				}
 				return
 			}
-			if got != tc.input {
+			if got != tc.expectedReturn {


Followup to https://github.com/openshift/router/pull/621/files#r1815351390.

Suggested change

if got != tc.expectedReturn {

if expectedEmpty && got != "" || got != tc.expectedReturn {

candita · 2024-10-24T16:38:25Z

pkg/router/template/template_helper_test.go

@@ -1061,7 +1112,7 @@ func TestParseIPList(t *testing.T) {
 				}
 				return
 			}
-			if got != tc.input {
+			if got != tc.expectedReturn {
 				t.Errorf("Failure: expected %q, got %q", tc.input, got)


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)

}

pkg/router/template/template_helper_test.go

mJace · 2024-10-24T23:38:04Z

@candita
I combined your suggestions
#621 (review)
#621 (review)

so the test function is now as following,

	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 got %q", got)
				}
				return
			}
			if tc.expectedEmpty && got != "" || got != tc.expectedReturn {
				t.Errorf("Failure: expected %q, got %q", tc.input, got)
			}
		})
	}

or do you think it is ok to make it more simple?

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

mJace · 2024-10-25T02:53:48Z

/retest

alebedev87

I think it's close to LGTM. I just wanted to discuss this thread and I agree with Candace that this if should be changed to be more straightforward.

alebedev87 · 2024-10-25T16:20:46Z

pkg/router/template/template_helper_test.go

-			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.",
+			expectedEmpty:  false,


When expectedEmpty is false it can be skipped from the test struct, similar to the other tests which were modified above.

alebedev87 · 2024-10-25T16:42:52Z

Sorry for the double post of the review, some buggy behavior from github.

mJace · 2024-11-04T12:10:10Z

/retest

mJace · 2024-11-05T01:17:42Z

/retest

openshift-ci · 2024-11-05T05:06:30Z

@mJace: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

openshift-ci bot requested review from alebedev87 and gcs278 August 30, 2024 06:55

openshift-ci bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 30, 2024

mJace mentioned this pull request Aug 30, 2024

parseIPList Function fails to process all valid IPs When a invalid entries is present in router whitelist ip list #620

Closed

openshift-ci bot assigned alebedev87 Sep 3, 2024

mJace changed the title ~~Fix parseIPList to Continue Processing After Invalid IPs and Return Valid IPs~~ OCPBUGS-39403 - Fix parseIPList to Continue Processing After Invalid IPs and Return Valid IPs Sep 3, 2024

openshift-ci bot assigned candita Oct 2, 2024

mJace force-pushed the fix_whitelist_ip_parser branch from 80ce105 to 51aad66 Compare October 10, 2024 08:53

candita reviewed Oct 23, 2024

View reviewed changes

openshift-ci bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 23, 2024

mJace force-pushed the fix_whitelist_ip_parser branch from 51aad66 to 31fdfe8 Compare October 24, 2024 01:53

mJace requested a review from candita October 24, 2024 09:34

candita reviewed Oct 24, 2024

View reviewed changes

pkg/router/template/template_helper_test.go Outdated Show resolved Hide resolved

candita reviewed Oct 24, 2024

View reviewed changes

pkg/router/template/template_helper_test.go Outdated Show resolved Hide resolved

candita reviewed Oct 24, 2024

View reviewed changes

pkg/router/template/template_helper_test.go Outdated Show resolved Hide resolved

mJace force-pushed the fix_whitelist_ip_parser branch 2 times, most recently from 1e3bc18 to 09efe4a Compare October 24, 2024 22:51

mJace force-pushed the fix_whitelist_ip_parser branch 2 times, most recently from aeb7e77 to 4a700ef Compare October 24, 2024 23:29

alebedev87 reviewed Oct 25, 2024

View reviewed changes

Enhance func:parseIPList to handle mixed validity IP list.

81ad24e

mJace force-pushed the fix_whitelist_ip_parser branch from 4a700ef to 81ad24e Compare October 30, 2024 03:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

mJace commented Aug 30, 2024 •

edited

Loading

openshift-ci bot commented Aug 30, 2024

alebedev87 commented Sep 3, 2024

alebedev87 commented Oct 2, 2024

candita commented Oct 2, 2024

openshift-ci bot commented Oct 10, 2024

mJace commented Oct 10, 2024

candita Oct 23, 2024

mJace Oct 24, 2024 •

edited

Loading

candita commented Oct 23, 2024

mJace commented Oct 24, 2024

mJace commented Oct 24, 2024

candita Oct 24, 2024

mJace Oct 24, 2024

alebedev87 Oct 25, 2024

mJace Oct 28, 2024

alebedev87 Oct 28, 2024

alebedev87 Oct 28, 2024

candita Oct 24, 2024

candita Oct 24, 2024 •

edited

Loading

mJace commented Oct 24, 2024 •

edited

Loading

mJace commented Oct 25, 2024

alebedev87 left a comment

alebedev87 Oct 25, 2024

alebedev87 commented Oct 25, 2024

mJace commented Nov 4, 2024

mJace commented Nov 5, 2024

openshift-ci bot commented Nov 5, 2024

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

	if got != tc.expectedReturn {
	if expectedEmpty && got != "" \|\| got != tc.expectedReturn {

-				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)
+                }

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

Are you sure you want to change the base?

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

Conversation

mJace commented Aug 30, 2024 • edited Loading

Summary

Changes

Benefits

Additional Notes

openshift-ci bot commented Aug 30, 2024

alebedev87 commented Sep 3, 2024

alebedev87 commented Oct 2, 2024

candita commented Oct 2, 2024

openshift-ci bot commented Oct 10, 2024

mJace commented Oct 10, 2024

Choose a reason for hiding this comment

mJace Oct 24, 2024 • edited Loading

Choose a reason for hiding this comment

candita commented Oct 23, 2024

mJace commented Oct 24, 2024

mJace commented Oct 24, 2024

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

candita Oct 24, 2024 • edited Loading

Choose a reason for hiding this comment

mJace commented Oct 24, 2024 • edited Loading

mJace commented Oct 25, 2024

alebedev87 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alebedev87 commented Oct 25, 2024

mJace commented Nov 4, 2024

mJace commented Nov 5, 2024

openshift-ci bot commented Nov 5, 2024

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

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

mJace commented Aug 30, 2024 •

edited

Loading

mJace Oct 24, 2024 •

edited

Loading

candita Oct 24, 2024 •

edited

Loading

mJace commented Oct 24, 2024 •

edited

Loading