Jv convert ipv4 mapped ipv6 to ipv4 #1906
Conversation
| @@ -389,6 +446,31 @@ class Connection { | |||
|
|
|||
| size_t Hash() const { return HashAll(container_, local_, remote_, flags_); } | |||
|
|
|||
| Connection ConvertToIPv4() const { | |||
There was a problem hiding this comment.
Change the name to ConvertToIPv4IfNeeded
There was a problem hiding this comment.
I think that we absolutely need to handle IPv4-mapped IPv6, thanks for starting the work 👍
Overall, I am not sure that we want to expose the conversion methods at every level of the network object stack. We could maybe handle this directly in the Address constructor and build an IPv4 when a mapped IPv6 is detected in parameters (and keep this conversion information as a new boolean attribute, for troubleshooting purpose).
| @@ -40,12 +40,22 @@ bool ContainsPrivateNetwork(Address::Family family, NRadixTree tree) { | |||
| return tree.IsAnyIPNetSubset(family, private_networks_tree) || private_networks_tree.IsAnyIPNetSubset(family, tree); | |||
| } | |||
|
|
|||
| void ConnectionTracker::UpdateConnectionNoLock(const Connection& conn, const ConnStatus& status) { | |||
| Connection ipv4Conn = conn.ConvertToIPv4(); | |||
There was a problem hiding this comment.
ipv4Conn is misleading, as as the result can be v6
There was a problem hiding this comment.
Yes. I know. I should have called it something else or left a comment to remind myself to change the name.
| @@ -63,7 +73,7 @@ void ConnectionTracker::Update( | |||
|
|
|||
| // Insert (or mark as active) all current connections and listen endpoints. | |||
| for (const auto& curr_conn : all_conns) { | |||
| EmplaceOrUpdateNoLock(curr_conn, new_status); | |||
| UpdateConnectionNoLock(curr_conn, new_status); | |||
| } | |||
| for (const auto& curr_endpoint : all_listen_endpoints) { | |||
| EmplaceOrUpdateNoLock(curr_endpoint, new_status); | |||
There was a problem hiding this comment.
| EmplaceOrUpdateNoLock(curr_endpoint, new_status); | |
| UpdateEndpointNoLock(curr_endpoint, new_status); |
| void UpdateConnectionNoLock(const Connection& conn, const ConnStatus& status); | ||
| void UpdateConnection(const Connection& conn, int64_t timestamp, bool added); | ||
| void UpdateEndpointNoLock(const ContainerEndpoint& endpoint, const ConnStatus& status); | ||
| void AddConnection(const Connection& conn, int64_t timestamp) { |
There was a problem hiding this comment.
We should keep the NoLock methods in the private scope. I suggest that we move the ones previously laying in the public section as well.
| const uint8_t* bytes = reinterpret_cast<const uint8_t*>(data_.data()); | ||
|
|
||
| // First 80 bits, 10 bytes, should be 0 if this is IPv4-Mapped IPv6 | ||
| for (int i = 0; i < 10; i++) { | ||
| if (bytes[i] != 0) { | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| // Next 2 bytes should be 0xFF if this is IPv4-Mapped IPv6 | ||
| if (bytes[10] != 0xFF || bytes[11] != 0xFF) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
For performance reasons, what about declaring a global const IPNet with the /96 mapped prefix, and using .Contains(addr) ?
| @@ -358,6 +407,14 @@ class ContainerEndpoint { | |||
|
|
|||
| size_t Hash() const { return HashAll(container_, endpoint_, l4proto_); } | |||
|
|
|||
| ContainerEndpoint ConvertToIPv4() const { | |||
There was a problem hiding this comment.
The name is misleading as it suggests that the result is always IPv4
Description
In some cases it was observed that in the network graph there were no connections between ACS components.
Checklist
Automated testing
If any of these don't apply, please comment below.
Testing Performed
TODO(replace-me)
Use this space to explain how you tested your PR, or, if you didn't test it, why you did not do so. (Valid reasons include "CI is sufficient" or "No testable changes")
In addition to reviewing your code, reviewers must also review your testing instructions, and make sure they are sufficient.
For more details, ref the Confluence page about this section.