-
Notifications
You must be signed in to change notification settings - Fork 455
feat(parser): add support for sourceIp with port #7315
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.
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
Conversation
Please retry analysis of this Pull-Request directly on SonarQube Cloud |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, I'll open an issue for TypeScript to do the same.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #7315 +/- ##
========================================
Coverage 96.35% 96.36%
========================================
Files 275 275
Lines 12980 13002 +22
Branches 965 966 +1
========================================
+ Hits 12507 12529 +22
Misses 366 366
Partials 107 107 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
IPvAnyNetwork(value) | ||
except ValueError: | ||
try: | ||
ip_part = value.split(":")[0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Has "IPv6-with-port" been taken into account?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I understand, the IPvAnyAddress model we're using supports both v4 and v6 - source
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I think this can break ipv6 yes in some situations. I need to run some tests before confirming this.
Thanks @iBug
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I think the way we split the port might actually not work with IPv6 which contains multiple :
. To make it work we should've taken everything but the last item of the split list instead of just the first. cc @leandrodamascena
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I think this can break ipv6 yes in some situations.
That's what I meant. I don't think it would parse when value.split(":")[0]
returns something like [2001
. A better approach might be
ip_part = value.rsplit(":", 1)[0]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And probably with .strip("[]")
as well, if IPvAnyNetwork
doesn't expected bracketed IPv6 addresses.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reoepned this issue: #7288
Issue number: closes #7288
Summary
Changes
This PR add supports for IP address with port (e.g., 10.1.15.242:39870) in APIGateway models.
User experience
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
Disclaimer: We value your time and bandwidth. As such, any pull requests created on non-triaged issues might not be successful.