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.

Sign up for GitHub

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

Jump to bottom

Refactor getBestPublicIp for all valid ips #1132

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
+94 −54
Open

Refactor getBestPublicIp for all valid ips #1132

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
19aa325
Make server getters const
sgourdas Sep 17, 2024
fc15cf7
Line cleanup
sgourdas Sep 19, 2024
ad3de64
Refactor getBestPublicIp for both protocols
sgourdas Sep 23, 2024
c8a11be
Add ip availability check in server start
sgourdas Sep 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions include/tools.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,6 +225,15 @@ IpAddress getBestPublicIps(IpMode mode);
*/
std::string getBestPublicIp();

/** Checks if IP address is available
*
* Check if the given IP address is configured on any network interface of the system
*
* @param addr the IP address to check.
* @return true if the IP address is available on the system.
*/
bool ipAvailable(const IpAddress& addr);
Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. This function need to be a part of the public API?
  2. IpAddress can be a pair of addresses (one IPv4 and the other IPv6). How is this function supposed to work if both components are non-empty?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This function need to be a part of the public API?

I think it can be a useful function to have available

IpAddress can be a pair of addresses (one IPv4 and the other IPv6). How is this function supposed to work if both components are non-empty?

Current use case was to check only one ip (in the case of the user giving one). I will refactor it to take as a parameter the mode as well and check accordingly.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This function need to be a part of the public API?

I think it can be a useful function to have available

When dealing with public API that's not a good argument. Public API is a commitment. Exposing something just because it's useful may lead to unjustified maintenance overhead (like having to deal with backward compatibility issues). Let's not bloat our public API without necessity.


/** Converts file size to human readable format.
*
* This function will convert a number to its equivalent size using units.
Expand Down
5 changes: 5 additions & 0 deletions src/server/internalServer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -462,8 +462,8 @@
sockAddr6.sin6_port = htons(m_port);

if (m_addr.addr.empty() && m_addr.addr6.empty()) { // No specific ip address provided
sockAddr6.sin6_addr = in6addr_any;
sockAddr4.sin_addr.s_addr = htonl(INADDR_ANY);

Check warning on line 466 in src/server/internalServer.cpp

View check run for this annotation

Codecov / codecov/patch

src/server/internalServer.cpp#L465-L466 

Added lines #L465 - L466 were not covered by tests
m_addr = kiwix::getBestPublicIps(m_ipMode);
} else {
bool ipv6 = inet_pton(AF_INET6, m_addr.addr6.c_str(), &(sockAddr6.sin6_addr.s6_addr)) == 1;
Expand All @@ -472,8 +472,13 @@
if (!ipv4 && !ipv6) {
const std::string& addr = m_addr.addr.empty() ? m_addr.addr6 : m_addr.addr;
std::cerr << "IP address " << addr << " is not valid" << std::endl;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please restore the previous text of the error message

return false;

Check warning on line 475 in src/server/internalServer.cpp

View check run for this annotation

Codecov / codecov/patch

src/server/internalServer.cpp#L475 

Added line #L475 was not covered by tests
}

if (!ipAvailable(m_addr)) {
std::cerr << "IP address " << (m_addr.addr.empty() ? m_addr.addr6 : m_addr.addr) << " is not available on this system" << std::endl;
return false;
}
}

if (m_ipMode == IpMode::all) {
Expand Down
12 changes: 12 additions & 0 deletions src/tools/networkTools.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,39 +211,51 @@
return result;
}

IpAddress getBestPublicIps(IpMode mode) {
IpAddress bestPublicIps = IpAddress{"127.0.0.1", "::1"};
std::map<std::string, IpAddress> interfaces = getNetworkInterfacesIPv4Or6();
bool validMode4 = (mode == IpMode::ipv4 || mode == IpMode::all); // Mode is valid to support ipv4
bool validMode6 = (mode == IpMode::ipv6 || mode == IpMode::all); // Mode is valid to support ipv6
#ifndef _WIN32
const char* const prioritizedNames[] = { "eth0", "eth1", "wlan0", "wlan1", "en0", "en1" };
for(const auto& name: prioritizedNames) {
const auto it = interfaces.find(name);
if(it == interfaces.end()) continue;
const IpAddress& interfaceIps = it->second;
if(!bestPublicIps.addr.empty() && validMode4 && !interfaceIps.addr.empty())
bestPublicIps.addr = interfaceIps.addr;
if(!bestPublicIps.addr6.empty() && validMode6 && !interfaceIps.addr6.empty())
bestPublicIps.addr6 = interfaceIps.addr6;
}
#endif
const char* const prefixes[] = { "192.168", "172.16.", "10.0" };
for (const auto& prefix : prefixes) {
for (const auto& [_, interfaceIps] : interfaces) {
if (!bestPublicIps.addr.empty() && validMode4 && !interfaceIps.addr.empty() && interfaceIps.addr.find(prefix) == 0)
bestPublicIps.addr = interfaceIps.addr;
if (!bestPublicIps.addr6.empty() && validMode6 && !interfaceIps.addr6.empty())
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the interfaceIps.addr.find(prefix) == 0 check omitted intentionally?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, the reason it is omitted is because we only provide v4 prefixes.

Copy link
Collaborator Author

@sgourdas sgourdas Sep 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Would it be beneficial to provide some for v6?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, the reason it is omitted is because we only provide v4 prefixes.

The purpose of this piece of code is to select an IP address from a local network, though I am not sure why the 169.254 prefix (see https://en.wikipedia.org/wiki/Link-local_address) has not been included. Your selection of an IPv6 address fails to meet that criteria. Why don't you filter based on the same IPv4 check (interfaceIps.addr.find(prefix) == 0) but use the IPv6 address (interfaceIps.addr6)?

Would it be beneficial to provide some for v6?

According to https://en.wikipedia.org/wiki/Link-local_address#IPv6

  • In IPv6, unicast link-local addresses are assigned from the block fe80::/10

however, unfortunately, it seems like fe80 cannot be used as a prefix because only its higher 10 bits matter. If we decide to support it a slightly more complex check will be required. But let's not go after it now.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok so if I understand correctly, for this point, I should just add 169.254 for the v4 prefixes, right?

bestPublicIps.addr6 = interfaceIps.addr6;
}
}

return bestPublicIps;
}

Check notice on line 243 in src/tools/networkTools.cpp

View check run for this annotation

codefactor.io / CodeFactor

src/tools/networkTools.cpp#L214-L243 

Complex Method
std::string getBestPublicIp()
{
return getBestPublicIps(IpMode::ipv4).addr;
}

bool ipAvailable(const IpAddress& addr)
{
auto interfaces = kiwix::getNetworkInterfacesIPv4Or6();
for (const auto& interface : interfaces) {
IpAddress interfaceIps = interface.second;
if (!interfaceIps.addr.empty() && interfaceIps.addr == addr.addr) return true;
if (!interfaceIps.addr6.empty() && interfaceIps.addr6 == addr.addr6) return true;
}

return false;

Check warning on line 258 in src/tools/networkTools.cpp

View check run for this annotation

Codecov / codecov/patch

src/tools/networkTools.cpp#L258 

Added line #L258 was not covered by tests
}

} // namespace kiwix
Loading