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 all commits
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
2 changes: 1 addition & 1 deletion include/common.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@

namespace kiwix {

enum class IpMode { ipv4, ipv6, all };
enum class IpMode { ipv4, ipv6, all, flex };
Copy link
Collaborator

Choose a reason for hiding this comment

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

To me flex is not an intuitive term for the intended mode of operation.

In the coding style that I got used to, enum members names must be in UPPERCASE. In this case that would have helped to avoid the problem with auto being a keyword in C++. Please rename existing members of IpMode in a separate commit, then add a new member IpMode::AUTO.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I have the feeling I have missed something... what is his "glex" option!? Where does it come from?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@kelson42 It started from here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I assume we are ok to proceed right?

typedef zim::size_type size_type;
typedef zim::offset_type offset_type;

Expand Down
12 changes: 6 additions & 6 deletions include/server.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@

#include <string>
#include <memory>
#include "common.h"
#include "tools.h"

namespace kiwix
{
Expand Down Expand Up @@ -52,7 +52,7 @@ namespace kiwix
void stop();

void setRoot(const std::string& root);
void setAddress(const std::string& addr) { m_addr = addr; }
void setAddress(const std::string& addr);
void setPort(int port) { m_port = port; }
void setNbThreads(int threads) { m_nbThreads = threads; }
void setMultiZimSearchLimit(unsigned int limit) { m_multizimSearchLimit = limit; }
Expand All @@ -64,15 +64,15 @@ namespace kiwix
void setBlockExternalLinks(bool blockExternalLinks)
{ m_blockExternalLinks = blockExternalLinks; }
void setIpMode(IpMode mode) { m_ipMode = mode; }
int getPort();
std::string getAddress();
int getPort() const;
IpAddress getAddress() const;
sgourdas marked this conversation as resolved.
Show resolved Hide resolved
IpMode getIpMode() const;

protected:
std::shared_ptr<Library> mp_library;
std::shared_ptr<NameMapper> mp_nameMapper;
std::string m_root = "";
std::string m_addr = "";
IpAddress m_addr;
std::string m_indexTemplateString = "";
int m_port = 80;
int m_nbThreads = 1;
Expand All @@ -81,7 +81,7 @@ namespace kiwix
bool m_withTaskbar = true;
bool m_withLibraryButton = true;
bool m_blockExternalLinks = false;
IpMode m_ipMode = IpMode::ipv4;
IpMode m_ipMode = IpMode::flex;
int m_ipConnectionLimit = 0;
std::unique_ptr<InternalServer> mp_server;
};
Expand Down
12 changes: 11 additions & 1 deletion include/tools.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
#include <vector>
#include <map>
#include <cstdint>
#include "common.h"

namespace kiwix
{
Expand Down Expand Up @@ -216,14 +217,23 @@ std::map<std::string, std::string> getNetworkInterfaces();
/** Provides the best IP address
* This function provides the best IP address from the list given by getNetworkInterfacesIPv4Or6()
*/
std::string getBestPublicIp(bool ipv6);
IpAddress getBestPublicIps(IpMode mode);

/** Provides the best IPv4 adddress
* Equivalent to getBestPublicIp(false). Provided for backward compatibility
* with libkiwix v13.1.0.
*/
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
15 changes: 13 additions & 2 deletions src/server.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,12 +75,23 @@
}
}

int Server::getPort()
void Server::setAddress(const std::string& addr)
{
if (addr.empty()) return;

if (addr.find(':') != std::string::npos) { // IPv6
m_addr.addr6 = (addr[0] == '[') ? addr.substr(1, addr.length() - 2) : addr; // Remove brackets if any
sgourdas marked this conversation as resolved.
Show resolved Hide resolved
} else {
m_addr.addr = addr;
}
}

int Server::getPort() const

Check warning on line 89 in src/server.cpp

View check run for this annotation

Codecov / codecov/patch

src/server.cpp#L89 

Added line #L89 was not covered by tests
{
return mp_server->getPort();
}

std::string Server::getAddress()
IpAddress Server::getAddress() const

Check warning on line 94 in src/server.cpp

View check run for this annotation

Codecov / codecov/patch

src/server.cpp#L94 

Added line #L94 was not covered by tests
{
return mp_server->getAddress();
}
Expand Down
29 changes: 16 additions & 13 deletions src/server/internalServer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -407,7 +407,7 @@

InternalServer::InternalServer(LibraryPtr library,
std::shared_ptr<NameMapper> nameMapper,
std::string addr,
IpAddress addr,
int port,
std::string root,
int nbThreads,
Expand Down Expand Up @@ -461,19 +461,22 @@
sockAddr6.sin6_family = AF_INET6;
sockAddr6.sin6_port = htons(m_port);

if (m_addr.empty()) {
if (0 != INADDR_ANY) {
sockAddr6.sin6_addr = in6addr_any;
sockAddr4.sin_addr.s_addr = htonl(INADDR_ANY);
}
m_addr = kiwix::getBestPublicIp(m_ipMode == IpMode::ipv6 || m_ipMode == IpMode::all);
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.c_str(), &(sockAddr6.sin6_addr.s6_addr)) == 1;
bool ipv4 = inet_pton(AF_INET, m_addr.c_str(), &(sockAddr4.sin_addr.s_addr)) == 1;
if (ipv6){
m_ipMode = IpMode::all;
} else if (!ipv4) {
std::cerr << "Ip address " << m_addr << " is not a valid ip address" << std::endl;
bool ipv6 = inet_pton(AF_INET6, m_addr.addr6.c_str(), &(sockAddr6.sin6_addr.s6_addr)) == 1;
bool ipv4 = inet_pton(AF_INET, m_addr.addr.c_str(), &(sockAddr4.sin_addr.s_addr)) == 1;

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;
}
}
Expand Down
9 changes: 5 additions & 4 deletions src/server/internalServer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@

#include "library.h"
#include "name_mapper.h"
#include "tools.h"

#include <zim/search.h>
#include <zim/suggestion.h>
Expand Down Expand Up @@ -94,7 +95,7 @@
public:
InternalServer(LibraryPtr library,
std::shared_ptr<NameMapper> nameMapper,
std::string addr,
IpAddress addr,
int port,
std::string root,
int nbThreads,
Expand All @@ -117,8 +118,8 @@
void** cont_cls);
bool start();
void stop();
std::string getAddress() { return m_addr; }
int getPort() { return m_port; }
IpAddress getAddress() const { return m_addr; }
int getPort() const { return m_port; }

Check warning on line 122 in src/server/internalServer.h

View check run for this annotation

Codecov / codecov/patch

src/server/internalServer.h#L121-L122 

Added lines #L121 - L122 were not covered by tests
IpMode getIpMode() const { return m_ipMode; }

private: // functions
Expand Down Expand Up @@ -166,7 +167,7 @@
typedef ConcurrentCache<std::string, std::shared_ptr<LockableSuggestionSearcher>> SuggestionSearcherCache;

private: // data
std::string m_addr;
IpAddress m_addr;
int m_port;
std::string m_root; // URI-encoded
std::string m_rootPrefixOfDecodedURL; // URI-decoded
Expand Down
57 changes: 34 additions & 23 deletions src/tools/networkTools.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,40 +211,51 @@
return result;
}


std::string getBestPublicIp(bool ipv6) {
IpAddress bestPublicIp = IpAddress{"127.0.0.1","::1"};
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
Comment on lines +217 to +218
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that these can be moved to the bottom of the function. The function will consist of two parts

  • select the best public IPv4 and IPv6 addresses regardless of the mode requested (the logic of this will be slightly simpler compared to the current code)
  • compose the result from those IPv4 and IPv6 addresses based on the requested mode

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am debating whether we should do a mode check inside this function at all. The name getBestPublicIps I think better suits a behavior where the whole IpAdress struct is filled. IMO mode checking should be moved outside this function.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agree. In fact, when writing my previous comment I considered proposing to package the first part (covered by the first bullet) into a separate function IpAddress getBestPublicIps(void). I even think that you proposed something like that earlier.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I even think that you proposed something like that earlier.

Indeed, you did.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, this has been all over the place, but let me correct it to close this up.

#ifndef _WIN32
const char* const prioritizedNames[] =
{ "eth0", "eth1", "wlan0", "wlan1", "en0", "en1" };
for(auto name: prioritizedNames) {
auto it=interfaces.find(name);
if(it != interfaces.end() && !(ipv6 && (*it).second.addr6.empty())) {
bestPublicIp = (*it).second;
break;
}
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;
sgourdas marked this conversation as resolved.
Show resolved Hide resolved
if(!bestPublicIps.addr6.empty() && validMode6 && !interfaceIps.addr6.empty())
bestPublicIps.addr6 = interfaceIps.addr6;
}
#endif

const char* const prefixes[] = { "192.168", "172.16.", "10.0" };
for(auto prefix : prefixes){
for(auto& itr : interfaces) {
std::string interfaceIp(itr.second.addr);
if (interfaceIp.find(prefix) == 0 && !(ipv6 && itr.second.addr6.empty())) {
bestPublicIp = itr.second;
break;
}
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 ipv6 ? bestPublicIp.addr6 : bestPublicIp.addr;
}

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 getBestPublicIp(false);
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
12 changes: 8 additions & 4 deletions test/otherTools.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
*/

#include "gtest/gtest.h"
#include "../include/tools.h"
#include "../src/tools/otherTools.h"
#include "zim/suggestion_iterator.h"
#include "../src/server/i18n_utils.h"
Expand Down Expand Up @@ -252,11 +253,14 @@ TEST(networkTools, getNetworkInterfaces)
}
}

TEST(networkTools, getBestPublicIp)
TEST(networkTools, getBestPublicIps)
{
using kiwix::getBestPublicIps;
using kiwix::getBestPublicIp;
using kiwix::IpMode;

std::cout << "getBestPublicIp(true) " << getBestPublicIp(true) << std::endl;
std::cout << "getBestPublicIp(false) " << getBestPublicIp(false) << std::endl;
std::cout << "getBestPublicIp() " << getBestPublicIp() << std::endl;
std::cout << "getBestPublicIps(IpMode::ipv4) : [" << getBestPublicIps(IpMode::ipv4).addr << ", " << getBestPublicIps(IpMode::ipv4).addr6 << "]" << std::endl;
std::cout << "getBestPublicIps(IpMode::ipv6) : [" << getBestPublicIps(IpMode::ipv6).addr << ", " << getBestPublicIps(IpMode::ipv6).addr6 << "]" << std::endl;
std::cout << "getBestPublicIps(IpMode::all) : [" << getBestPublicIps(IpMode::all).addr << ", " << getBestPublicIps(IpMode::all).addr6 << "]" << std::endl;
sgourdas marked this conversation as resolved.
Show resolved Hide resolved
std::cout << "getBestPublicIp() : " << getBestPublicIp() << std::endl;
}
Loading