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.

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

[19778][20296] Add netmask filter transport configuration + interface allowlist and blocklist #4241

Merged
merged 16 commits into from
Mar 17, 2024

Conversation

juanlofer-eprosima
Copy link
Contributor

@juanlofer-eprosima juanlofer-eprosima commented Jan 12, 2024

Description

This PR adds two new features:

  • Netmask filtering: allows to configure participants, (UDP/TCP) transports and/or specific interfaces (via allowlist) so that no messages are sent from an interface to remote locators from outside its network domain (given by network mask).
  • Blocklist: allows to ignore the interfaces present in this collection. These can be specified by IP or device name, and this list takes precedence over allowlist/whitelist.

Behaviour changes:

  • System's network interfaces are now cached in SystemInfo singleton, in order to reduce the number of system calls performed and promote consistency across separate Fast-DDS code sections.
  • Transformation of received remote locators to local ones is now only attempted when they correspond to (Fast-DDS) entities created in the same host. More specifically, now it is the GUID of the corresponding entity what is used to determine whether the locator is local, rather than by looking for a match in the local interfaces. This avoids the erroneous transformation of a truly remote locator to localhost when there is a coincidence in the IPs assigned to two machines' interfaces (example: docker0 -> 172.17.0.1).

Contributor Checklist

  • Commit messages follow the project guidelines.
  • The code follows the style guidelines of this project.
  • ❌ Tests that thoroughly check the new feature have been added/Regression tests checking the bug and its fix have been added; the added tests pass locally
  • Any new/modified methods have been properly documented using Doxygen.
  • ❌ Changes are ABI compatible.
  • Changes are API compatible.
  • New feature has been added to the versions.md file (if applicable).
  • New feature has been documented/Current behavior is correctly described in the documentation.
    Related documentation PR: eProsima/Fast-DDS-docs# (PR)
  • N/A; Applicable backports have been included in the description.

Reviewer Checklist

  • The PR has a milestone assigned.
  • The title and description correctly express the PR's purpose.
  • Check contributor checklist is correct.
  • Check CI results: changes do not issue any warning.
  • Check CI results: failing tests are unrelated with the changes.

@juanlofer-eprosima juanlofer-eprosima changed the title [19778] Add netmask filter transport configuration option + interface blocklist [19778] Add netmask filter transport configuration + interface blocklist Jan 12, 2024
@EduPonz EduPonz added this to the v3.0.0 milestone Jan 15, 2024
@juanlofer-eprosima juanlofer-eprosima force-pushed the feature/netmask-filter branch 3 times, most recently from 9d1dd95 to d950405 Compare February 1, 2024 10:36
@juanlofer-eprosima juanlofer-eprosima marked this pull request as ready for review February 1, 2024 10:53
@EduPonz EduPonz modified the milestones: v3.0.0, v2.14.0 Feb 1, 2024
@EduPonz EduPonz added doc-pending Issue or PR which is pending to be documented versions-pending labels Feb 1, 2024
@juanlofer-eprosima juanlofer-eprosima changed the title [19778] Add netmask filter transport configuration + interface blocklist [19778][20296] Add netmask filter transport configuration + interface blocklist Feb 29, 2024
Copy link
Member

@Mario-DL Mario-DL left a comment

Choose a reason for hiding this comment

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

Overall, great job in this PR! bringing important corrections and refactor.
I am leaving a partial review here. Still pending review the transport implementations, the xmlparsing related things, and testing that the issue is gone.

I could perfectly reproduce the issue. In the attached image the same data packet was being delivered to different destinations: lan address (correctly) and localhost (wrongly, after a failed docker-ip interpretation)

networkissue

Leaving some questions and suggestions though.
In addition, some fixes should be made to correct windows compilation and I think that the PR should be paired with a Fast DDS Docs documentation PR.

include/fastdds/rtps/transport/NetmaskFilterKind.hpp Outdated Show resolved Hide resolved
include/fastdds/rtps/transport/NetmaskFilterKind.hpp Outdated Show resolved Hide resolved
include/fastdds/rtps/transport/ChainingTransport.h Outdated Show resolved Hide resolved
include/fastdds/rtps/transport/TransportInterface.h Outdated Show resolved Hide resolved
src/cpp/rtps/network/NetworkFactory.h Outdated Show resolved Hide resolved
src/cpp/fastdds/subscriber/SubscriberImpl.cpp Outdated Show resolved Hide resolved
src/cpp/rtps/network/NetmaskFilterUtils.hpp Outdated Show resolved Hide resolved
src/cpp/utils/IPFinder.cpp Outdated Show resolved Hide resolved
src/cpp/fastdds/publisher/PublisherImpl.cpp Outdated Show resolved Hide resolved
src/cpp/rtps/transport/TCPv4Transport.cpp Outdated Show resolved Hide resolved
src/cpp/rtps/transport/UDPv4Transport.cpp Outdated Show resolved Hide resolved
src/cpp/rtps/transport/UDPv4Transport.cpp Show resolved Hide resolved
src/cpp/rtps/network/NetworkFactory.cpp Show resolved Hide resolved
src/cpp/rtps/transport/UDPTransportInterface.h Outdated Show resolved Hide resolved
include/fastrtps/xmlparser/XMLParser.h Show resolved Hide resolved
include/fastrtps/xmlparser/XMLParser.h Show resolved Hide resolved
include/fastrtps/xmlparser/XMLParser.h Show resolved Hide resolved
Copy link
Member

@Mario-DL Mario-DL left a comment

Choose a reason for hiding this comment

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

I left the second part of the review.

I checked that the feature fixes the locator transformation mangling but I still need to properly test the blocklist and netmask filter.
The documentation has been linked so I remove the docs pending label

@Mario-DL Mario-DL removed the doc-pending Issue or PR which is pending to be documented label Mar 1, 2024
@juanlofer-eprosima juanlofer-eprosima changed the title [19778][20296] Add netmask filter transport configuration + interface blocklist [19778][20296] Add netmask filter transport configuration + interface allowlist and blocklist Mar 11, 2024
@Mario-DL
Copy link
Member

@richiprosima please test this

Copy link
Member

@Mario-DL Mario-DL left a comment

Choose a reason for hiding this comment

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

Nice effort on the review and refactors! looks really nice.
Leaving some minor suggestions

src/cpp/fastdds/publisher/PublisherImpl.cpp Outdated Show resolved Hide resolved
src/cpp/fastdds/publisher/PublisherImpl.cpp Outdated Show resolved Hide resolved
include/fastdds/rtps/participant/RTPSParticipant.h Outdated Show resolved Hide resolved
include/fastdds/rtps/participant/RTPSParticipant.h Outdated Show resolved Hide resolved
Mario-DL
Mario-DL previously approved these changes Mar 14, 2024
Copy link
Member

@Mario-DL Mario-DL left a comment

Choose a reason for hiding this comment

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

LGTM with green CI

@Mario-DL
Copy link
Member

@richiprosima please test this

@Mario-DL
Copy link
Member

@richiprosima please test this

Copy link
Member

@Mario-DL Mario-DL left a comment

Choose a reason for hiding this comment

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

LGTM with green CI

Signed-off-by: Juan Lopez Fernandez <juanlopez@eprosima.com>
Signed-off-by: EduPonz <eduardoponz@eprosima.com>
Signed-off-by: Juan Lopez Fernandez <juanlopez@eprosima.com>
Signed-off-by: EduPonz <eduardoponz@eprosima.com>
Signed-off-by: Juan Lopez Fernandez <juanlopez@eprosima.com>
Signed-off-by: EduPonz <eduardoponz@eprosima.com>
Signed-off-by: Juan Lopez Fernandez <juanlopez@eprosima.com>
Signed-off-by: EduPonz <eduardoponz@eprosima.com>
Signed-off-by: Juan Lopez Fernandez <juanlopez@eprosima.com>
Signed-off-by: EduPonz <eduardoponz@eprosima.com>
Signed-off-by: Juan Lopez Fernandez <juanlopez@eprosima.com>
Signed-off-by: EduPonz <eduardoponz@eprosima.com>
Signed-off-by: Juan Lopez Fernandez <juanlopez@eprosima.com>
Signed-off-by: EduPonz <eduardoponz@eprosima.com>
Signed-off-by: Juan Lopez Fernandez <juanlopez@eprosima.com>
Signed-off-by: EduPonz <eduardoponz@eprosima.com>
Signed-off-by: Juan Lopez Fernandez <juanlopez@eprosima.com>
Signed-off-by: EduPonz <eduardoponz@eprosima.com>
…o cpp

Signed-off-by: Juan Lopez Fernandez <juanlopez@eprosima.com>
Signed-off-by: EduPonz <eduardoponz@eprosima.com>
Signed-off-by: Juan Lopez Fernandez <juanlopez@eprosima.com>
Signed-off-by: EduPonz <eduardoponz@eprosima.com>
Signed-off-by: Juan Lopez Fernandez <juanlopez@eprosima.com>
Signed-off-by: EduPonz <eduardoponz@eprosima.com>
Signed-off-by: Juan Lopez Fernandez <juanlopez@eprosima.com>
Signed-off-by: EduPonz <eduardoponz@eprosima.com>
Signed-off-by: Juan Lopez Fernandez <juanlopez@eprosima.com>
Signed-off-by: EduPonz <eduardoponz@eprosima.com>
Signed-off-by: Juan Lopez Fernandez <juanlopez@eprosima.com>
Signed-off-by: EduPonz <eduardoponz@eprosima.com>
Signed-off-by: Juan Lopez Fernandez <juanlopez@eprosima.com>
Signed-off-by: EduPonz <eduardoponz@eprosima.com>
@EduPonz
Copy link

EduPonz commented Mar 15, 2024

@richiprosima please test this

@EduPonz
Copy link

EduPonz commented Mar 16, 2024

@richiprosima please test windows test mac

@EduPonz
Copy link

EduPonz commented Mar 17, 2024

@richiprosima please check style

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants