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

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 {

Looks like this was taken care of.

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

openshift-ci-robot · 2025-01-21T09:46:59Z

@mJace: This pull request references Jira Issue OCPBUGS-39403, which is invalid:

expected the bug to be open, but it isn't
expected the bug to target the "4.19.0" version, but no target version was set
expected the bug to be in one of the following states: NEW, ASSIGNED, POST, but it is Closed (Won't Do) instead

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

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.

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 openshift-eng/jira-lifecycle-plugin repository.

alebedev87 · 2025-01-21T09:49:18Z

/jira refresh

openshift-ci-robot · 2025-01-21T09:49:23Z

@alebedev87: This pull request references Jira Issue OCPBUGS-39403, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.19.0) matches configured target version for branch (4.19.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @lihongan

In response to this:

/jira refresh

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 openshift-eng/jira-lifecycle-plugin repository.

alebedev87 · 2025-01-21T10:30:08Z

/lgtm
/approve
/hold

Holding for @candita to have another look.

openshift-ci · 2025-01-21T10:30:37Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alebedev87

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [alebedev87]

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

alebedev87 · 2025-01-21T16:39:53Z

/retest

mJace · 2025-01-21T23:58:56Z

/retest

alebedev87 · 2025-01-23T14:10:06Z

openshift/origin#29466 should fix the image registry failure of e2e-aws-serial.

gcs278 · 2025-01-23T19:13:48Z

openshift/origin#29466 is merged
/test e2e-aws-serial

candita · 2025-01-24T20:56:41Z

/lgtm
/unhold

rhamini3 · 2025-02-11T20:46:29Z

Pre-merge verified on cluster version: 4.18.0-0.ci.test-2025-02-12-162751-ci-ln-2i21xl2-latest

iamin@iamin-mac Downloads % oc get clusterversion 
NAME      VERSION                                                   AVAILABLE   PROGRESSING   SINCE   STATUS
version   4.18.0-0.ci.test-2025-02-12-162751-ci-ln-2i21xl2-latest   True        False         50m     Cluster version is 4.18.0-0.ci.test-2025-02-12-162751-ci-ln-2i21xl2-latest

annotate the route with valid and invalid IPs

iamin@iamin-mac cluster-ingress-operator % oc annotate route edge-route --overwrite haproxy.router.openshift.io/ip_whitelist='0.0.0.0 192.abc.0.d 192.168.47.9'
route.route.openshift.io/edge-route annotated

view the pod logs to see the invalid IP

iamin@iamin-mac Downloads % oc -n openshift-ingress logs router-default-5b7c764c88-kjvss | grep parseIPList

I0212 17:56:43.909529       1 template_helper.go:366] "msg"="parseIPList called" "logger"="template" "value"="0.0.0.0 192.abc.0.d 192.168.47.9"
I0212 17:56:43.909568       1 template_helper.go:392] "msg"="parseIPList found invalid IP/CIDR" "192.abc.0.d"="(MISSING)" "logger"="template"
I0212 17:56:43.909599       1 template_helper.go:402] "msg"="parseIPList parsed the list" "logger"="template" "validIPs"="0.0.0.0 192.168.47.9"

check haProxy to make sure only valid logs are present and invalid logs aren't

iamin@iamin-mac cluster-ingress-operator % oc -n openshift-ingress rsh  router-default-5b7c764c88-kjvss
sh-5.1$ cat haproxy.config | grep edge-route
backend be_edge_http:default:edge-route
sh-5.1$ cat haproxy.config | grep edge-route -A25
backend be_edge_http:default:edge-route
  mode http
  option redispatch
  option forwardfor
  balance random
  acl allowlist src 0.0.0.0 192.168.47.9
  tcp-request content reject if !allowlist

annotate route with all invalid IPs

iamin@iamin-mac cluster-ingress-operator % oc annotate route edge-route --overwrite haproxy.router.openshift.io/ip_whitelist='0.0.0.e 192.abc.0.d 192.168.47.'
route.route.openshift.io/edge-route annotated

check logs to make sure all IPs are flagged as invalid

iamin@iamin-mac cluster-ingress-operator % oc -n openshift-ingress logs router-default-5b7c764c88-kjvss                                                     

I0212 18:01:24.498486       1 template_helper.go:366] "msg"="parseIPList called" "logger"="template" "value"="0.0.0.e 192.abc.0.d 192.168.47."
I0212 18:01:24.498498       1 template_helper.go:392] "msg"="parseIPList found invalid IP/CIDR" "0.0.0.e"="(MISSING)" "logger"="template"
I0212 18:01:24.498527       1 template_helper.go:392] "msg"="parseIPList found invalid IP/CIDR" "192.abc.0.d"="(MISSING)" "logger"="template"
I0212 18:01:24.498539       1 template_helper.go:392] "msg"="parseIPList found invalid IP/CIDR" "192.168.47."="(MISSING)" "logger"="template"
I0212 18:01:24.499173       1 template_helper.go:366] "msg"="parseIPList called" "logger"="template" "value"=""
I0212 18:01:24.499178       1 template_helper.go:370] "msg"="parseIPList empty list found" "logger"="template"

haProxy should not contain any IPs

iamin@iamin-mac cluster-ingress-operator % oc -n openshift-ingress rsh  router-default-5b7c764c88-kjvss                                                       
sh-5.1$ cat haproxy.config | grep edge-route -A25
backend be_edge_http:default:edge-route
  mode http
  option redispatch
  option forwardfor
  balance random

marking bug as verified
/label qe-approved

openshift-ci-robot · 2025-02-11T20:46:51Z

@mJace: This pull request references Jira Issue OCPBUGS-39403, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.19.0) matches configured target version for branch (4.19.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)

No GitHub users were found matching the public email listed for the QA contact in Jira (iamin@redhat.com), skipping review request.

In response to this:

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.

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 openshift-eng/jira-lifecycle-plugin repository.

openshift-ci-robot · 2025-02-20T00:30:06Z

/retest-required

Remaining retests: 0 against base HEAD b447c4d and 2 for PR HEAD f0926cb in total

openshift-ci · 2025-02-20T04:18:36Z

@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-robot · 2025-02-20T04:21:06Z

@mJace: Jira Issue OCPBUGS-39403: All pull requests linked via external trackers have merged:

openshift/router#621

Jira Issue OCPBUGS-39403 has been moved to the MODIFIED state.

In response to this:

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.

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 openshift-eng/jira-lifecycle-plugin repository.

openshift-bot · 2025-02-20T07:59:52Z

[ART PR BUILD NOTIFIER]

Distgit: ose-haproxy-router-base
This PR has been included in build ose-haproxy-router-base-container-v4.19.0-202502200708.p0.gffb6637.assembly.stream.el9.
All builds following this will include this PR.

openshift-bot · 2025-02-20T08:15:02Z

[ART PR BUILD NOTIFIER]

Distgit: openshift-enterprise-haproxy-router
This PR has been included in build openshift-enterprise-haproxy-router-container-v4.19.0-202502200708.p0.gffb6637.assembly.stream.el9.
All builds following this will include this PR.

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

openshift-ci bot added the needs-ok-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 and removed needs-ok-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 3 times, most recently from 09efe4a to aeb7e77 Compare October 24, 2024 23:01

openshift-ci-robot added the jira/valid-reference label Jan 21, 2025

openshift-ci-robot added the jira/invalid-bug label Jan 21, 2025

openshift-ci-robot added jira/valid-bug and removed jira/invalid-bug labels Jan 21, 2025

openshift-ci bot requested a review from lihongan January 21, 2025 09:49

Enhance func:parseIPList to handle mixed validity IP list.

Loading
Loading status checks…

f0926cb

mJace force-pushed the fix_whitelist_ip_parser branch from 81ad24e to f0926cb Compare January 21, 2025 10:10

openshift-ci bot added do-not-merge/hold lgtm labels Jan 21, 2025

openshift-ci bot added the approved label Jan 21, 2025

openshift-ci bot removed the do-not-merge/hold label Jan 24, 2025

openshift-ci bot added the qe-approved label Feb 11, 2025

openshift-merge-bot bot merged commit ffb6637 into openshift:master Feb 20, 2025
9 checks passed

	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

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

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

Choose a reason for hiding this comment

candita Oct 24, 2024 • edited Loading

Choose a reason for hiding this comment

openshift-ci-robot commented Jan 21, 2025

Summary

Changes

Benefits

Additional Notes

alebedev87 commented Jan 21, 2025

openshift-ci-robot commented Jan 21, 2025

alebedev87 commented Jan 21, 2025

openshift-ci bot commented Jan 21, 2025

alebedev87 commented Jan 21, 2025

mJace commented Jan 21, 2025

alebedev87 commented Jan 23, 2025

gcs278 commented Jan 23, 2025

candita commented Jan 24, 2025

rhamini3 commented Feb 11, 2025 • edited Loading

openshift-ci-robot commented Feb 11, 2025

Summary

Changes

Benefits

Additional Notes

openshift-ci-robot commented Feb 20, 2025

openshift-ci bot commented Feb 20, 2025

openshift-ci-robot commented Feb 20, 2025

Summary

Changes

Benefits

Additional Notes

openshift-bot commented Feb 20, 2025

openshift-bot commented Feb 20, 2025

mJace commented Aug 30, 2024 •

edited

Loading

mJace Oct 24, 2024 •

edited

Loading

candita Oct 24, 2024 •

edited

Loading

rhamini3 commented Feb 11, 2025 •

edited

Loading