Refactor getBestPublicIp for all valid ips #1132

sgourdas · 2024-09-12T15:02:17Z

This refactors getBestPublicIp(bool) to getBestPublicIps(IpMode) so it can correctly return all attached ips for both protocols ipv4 and ipv6.

Fixes kiwix/kiwix-tools#703
Fixes kiwix/kiwix-tools#709

codecov · 2024-09-12T15:07:56Z

Codecov Report

Attention: Patch coverage is 32.65306% with 33 lines in your changes missing coverage. Please review.

Project coverage is 41.38%. Comparing base (0b14fda) to head (c8a11be).

Files with missing lines	Patch %	Lines
src/tools/networkTools.cpp	42.85%	3 Missing and 13 partials ⚠️
src/server/internalServer.cpp	16.66%	7 Missing and 3 partials ⚠️
src/server.cpp	28.57%	3 Missing and 2 partials ⚠️
src/server/internalServer.h	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1132      +/-   ##
==========================================
- Coverage   41.43%   41.38%   -0.06%     
==========================================
  Files          59       59              
  Lines        4245     4265      +20     
  Branches     2323     2338      +15     
==========================================
+ Hits         1759     1765       +6     
- Misses        990      996       +6     
- Partials     1496     1504       +8

Flag	Coverage Δ
	`41.38% <32.65%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

kelson42 · 2024-09-13T07:32:36Z

@sgourdas thx for the PR but can you please fix the code so the CI can pass?

sgourdas · 2024-09-13T07:51:28Z

@kelson42 ready

kelson42

Almost good, but

$ kiwix-serve --port=8080 -i 127.0.0.1 /home/kelson/Downloads/wikipedia_en_ray_charles_maxi_2023-12.zim 
The Kiwix server is running and can be accessed in the local network at: http://192.168.0.168:8080 and http://::1:808

Should return only http://127.0.0.1:8080 as kiwix-serve should only listen on this IP

include/server.h

src/server/internalServer.cpp

kelson42 · 2024-09-17T09:00:58Z

Still not working fine:

$ kiwix-serve --port=8081 -i ipv4 /home/kelson/Downloads/wikipedia_fr_climate_change_maxi_2022-05.zim 
The Kiwix server is running and can be accessed in the local network at: http://:8081

@sgourdas Please test all of this completly.

sgourdas · 2024-09-17T09:21:47Z

Still not working fine:

$ kiwix-serve --port=8081 -i ipv4 /home/kelson/Downloads/wikipedia_fr_climate_change_maxi_2022-05.zim 
The Kiwix server is running and can be accessed in the local network at: http://:8081

@sgourdas Please test all of this completly.

@kelson42 I tested this and it was working fine for me. Tested again and I get:

kiwix-serve --port 8081 -l /home/ubunter/.local/share/kiwix-desktop/library.xml -i ipv4
Loading the library from the following files:
        /home/ubunter/.local/share/kiwix-desktop/library.xml
The library was successfully loaded.
The Kiwix server is running and can be accessed in the local network at: http://172.27.70.65:8081

sgourdas · 2024-09-17T09:22:47Z

Have you built both new kiwix tools and libkiwix versions?

If yes, can you provide more details like if despite the printed message the server is available in browser?

src/server/internalServer.cpp

include/server.h

include/tools.h

src/server/internalServer.cpp

src/server/internalServer.h

test/server_testing_tools.h

include/tools.h

kelson42 · 2024-09-17T14:58:01Z

@sgourdas This will wait tomorrow, I kind of suspect something wrong in my testing because of the renaming of the .pc file done earlier today and not fully rolled-out.

kelson42 · 2024-09-18T15:02:52Z

@sgourdas Still wrong! You are "cheating" (taking for IPV4 the localhost instead of the public IP)!

$ kiwix-serve --port=8383 -i ipv4 /home/kelson/Downloads/wikipedia_fr_climate_change_maxi_2022-05.zim 
The Kiwix server is running and can be accessed in the local network at: http://127.0.0.1:8383

veloman-yunkan

I think I started reviewing while the PR was being updated. My feeling is that my feedback doesn't pave the path to this PR being merged on next iteration, it seems like we will have to make at least a couple more back-and-forths.

include/server.h

src/server/internalServer.cpp

src/tools/networkTools.cpp

include/server.h

sgourdas · 2024-09-18T15:38:11Z

I think I started reviewing while the PR was being updated. My feeling is that my feedback doesn't pave the path to this PR being merged on next iteration, it seems like we will have to make at least a couple more back-and-forths.

Sorry for this. This PR is not up to standard and I have to revise all the changes. When it started, I expected it to be more straight forward and it has become very unstructured.

Will try to get back ASAP.

kelson42 · 2024-09-19T19:29:25Z

Seems to work now! How do we secure that only valid IP are given/accepted for -i?

sgourdas · 2024-09-20T06:41:02Z

Seems to work now! How do we secure that only valid IP are given/accepted for -i?

Perfect. Currently, we are using inet_pton() accordingly for v4 or v6 addresses. If the conversion is successful we know that it is valid and of that type.

kelson42 · 2024-09-20T19:10:07Z

@sgourdas This IP 192.168.0.0 is not available on my system, but I have no error telling me the IP is not valid.

kelson@camber:~/code/kiwix-tools$ kiwix-serve --port=8081 -i 192.168.0.0 /home/kelson/Downloads/wikipedia_en_ray_charles_maxi_2023-12.zim 
Unable to instantiate the HTTP daemon. The port 8081 is maybe already occupied or need more permissions to be open. Please try as root or with a port number higher or equal to 1024.

sgourdas · 2024-09-20T21:44:34Z

@sgourdas This IP 192.168.0.0 is not available on my system, but I have no error telling me the IP is not valid.


kelson@camber:~/code/kiwix-tools$ kiwix-serve --port=8081 -i 192.168.0.0 /home/kelson/Downloads/wikipedia_en_ray_charles_maxi_2023-12.zim 

Unable to instantiate the HTTP daemon. The port 8081 is maybe already occupied or need more permissions to be open. Please try as root or with a port number higher or equal to 1024.

What should be the proper behaviour for this case in your opinion? I think this should be a topic for another PR if it add features that were not present before.

kelson42 · 2024-09-21T14:53:34Z

@sgourdas OK, I have open kiwix/kiwix-tools#709
@sgourdas @veloman-yunkan We should move aheqd with code review IMO now that it works from a user perspective.

kelson42

LGTM from a user perspective

veloman-yunkan

I think that we should close #1136 and bring its enhancement into this PR.

include/server.h

include/tools.h

src/server.cpp

test/otherTools.cpp

veloman-yunkan · 2024-09-24T07:52:04Z

include/common.h

@@ -16,7 +16,7 @@

 namespace kiwix {

-enum class IpMode { ipv4, ipv6, all };
+enum class IpMode { ipv4, ipv6, all, flex };


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.

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

@kelson42 It started from here

src/server.cpp

veloman-yunkan · 2024-09-24T07:57:12Z

src/server/internalServer.cpp

+
+    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;


Please restore the previous text of the error message

src/tools/networkTools.cpp

veloman-yunkan · 2024-09-24T08:47:14Z

src/tools/networkTools.cpp

+  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


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

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.

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.

I even think that you proposed something like that earlier.

Indeed, you did.

veloman-yunkan · 2024-09-24T08:49:05Z

src/tools/networkTools.cpp

+    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())


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

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

Would it be beneficial to provide some for v6?

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.

test/otherTools.cpp

veloman-yunkan · 2024-09-24T08:56:59Z

include/tools.h

+ * @param addr the IP address to check.
+ * @return true if the IP address is available on the system.
+ */
+bool ipAvailable(const IpAddress& addr);


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

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?

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.

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.

sgourdas · 2024-09-27T14:39:39Z

Sorry for the lack of feedback here. Getting back tomorrow morning.

kelson42 self-requested a review September 13, 2024 07:32

kelson42 mentioned this pull request Sep 13, 2024

kiwix-serve displays both protocol attached ips kiwix/kiwix-tools#704

Open

kelson42 added this to the 14.0.0 milestone Sep 13, 2024

kelson42 requested a review from veloman-yunkan September 13, 2024 15:32

kelson42 requested changes Sep 14, 2024

View reviewed changes

veloman-yunkan reviewed Sep 16, 2024

View reviewed changes

include/server.h Outdated Show resolved Hide resolved

sgourdas commented Sep 16, 2024

View reviewed changes

src/server/internalServer.cpp Outdated Show resolved Hide resolved

sgourdas force-pushed the feature/best-public-ip branch from 5276f1c to b2a7704 Compare September 16, 2024 18:54

sgourdas requested review from veloman-yunkan and kelson42 September 16, 2024 18:55

veloman-yunkan requested changes Sep 17, 2024

View reviewed changes

sgourdas force-pushed the feature/best-public-ip branch from b2a7704 to eb3e76a Compare September 18, 2024 14:17

sgourdas requested a review from veloman-yunkan September 18, 2024 14:25

kelson42 force-pushed the feature/best-public-ip branch from 6803fa4 to 1a09276 Compare September 18, 2024 14:56

sgourdas removed the request for review from veloman-yunkan September 18, 2024 15:16

sgourdas marked this pull request as draft September 18, 2024 15:33

veloman-yunkan requested changes Sep 18, 2024

View reviewed changes

sgourdas force-pushed the feature/best-public-ip branch 4 times, most recently from d87643d to 97ad823 Compare September 19, 2024 08:47

sgourdas marked this pull request as ready for review September 19, 2024 12:55

sgourdas requested a review from veloman-yunkan September 19, 2024 16:29

kelson42 mentioned this pull request Sep 21, 2024

Starting Kiwix Serve with invalid IP should generate an error kiwix/kiwix-tools#709

Open

kelson42 approved these changes Sep 21, 2024

View reviewed changes

sgourdas mentioned this pull request Sep 21, 2024

Introduce ipAvailable network function #1136

Closed

veloman-yunkan requested changes Sep 23, 2024

View reviewed changes

include/server.h Show resolved Hide resolved

include/tools.h Outdated Show resolved Hide resolved

src/server.cpp Outdated Show resolved Hide resolved

test/otherTools.cpp Outdated Show resolved Hide resolved

sgourdas added 3 commits September 23, 2024 13:56

Make server getters const

19aa325

Line cleanup

fc15cf7

Refactor getBestPublicIp for both protocols

ad3de64

sgourdas force-pushed the feature/best-public-ip branch from 97ad823 to ad3de64 Compare September 23, 2024 16:05

Add ip availability check in server start

c8a11be

sgourdas force-pushed the feature/best-public-ip branch from 5cf5c95 to c8a11be Compare September 23, 2024 16:22

sgourdas requested a review from veloman-yunkan September 23, 2024 16:28

veloman-yunkan requested changes Sep 24, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor getBestPublicIp for all valid ips #1132

Refactor getBestPublicIp for all valid ips #1132

sgourdas commented Sep 12, 2024 •

edited by kelson42

Loading

codecov bot commented Sep 12, 2024 •

edited

Loading

kelson42 commented Sep 13, 2024

sgourdas commented Sep 13, 2024

kelson42 left a comment •

edited

Loading

kelson42 commented Sep 17, 2024

sgourdas commented Sep 17, 2024

sgourdas commented Sep 17, 2024

kelson42 commented Sep 17, 2024

kelson42 commented Sep 18, 2024

veloman-yunkan left a comment

sgourdas commented Sep 18, 2024

kelson42 commented Sep 19, 2024

sgourdas commented Sep 20, 2024

kelson42 commented Sep 20, 2024

sgourdas commented Sep 20, 2024

kelson42 commented Sep 21, 2024

kelson42 left a comment

veloman-yunkan left a comment

veloman-yunkan Sep 24, 2024

kelson42 Sep 24, 2024

veloman-yunkan Sep 25, 2024

veloman-yunkan Sep 24, 2024

veloman-yunkan Sep 24, 2024

sgourdas Sep 25, 2024

veloman-yunkan Sep 25, 2024

veloman-yunkan Sep 25, 2024

veloman-yunkan Sep 24, 2024

sgourdas Sep 24, 2024

sgourdas Sep 24, 2024 •

edited

Loading

veloman-yunkan Sep 25, 2024

veloman-yunkan Sep 24, 2024

sgourdas Sep 24, 2024

veloman-yunkan Sep 25, 2024

sgourdas commented Sep 27, 2024

		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

Refactor getBestPublicIp for all valid ips #1132

Are you sure you want to change the base?

Refactor getBestPublicIp for all valid ips #1132

Conversation

sgourdas commented Sep 12, 2024 • edited by kelson42 Loading

codecov bot commented Sep 12, 2024 • edited Loading

Codecov Report

kelson42 commented Sep 13, 2024

sgourdas commented Sep 13, 2024

kelson42 left a comment • edited Loading

Choose a reason for hiding this comment

kelson42 commented Sep 17, 2024

sgourdas commented Sep 17, 2024

sgourdas commented Sep 17, 2024

kelson42 commented Sep 17, 2024

kelson42 commented Sep 18, 2024

veloman-yunkan left a comment

Choose a reason for hiding this comment

sgourdas commented Sep 18, 2024

kelson42 commented Sep 19, 2024

sgourdas commented Sep 20, 2024

kelson42 commented Sep 20, 2024

sgourdas commented Sep 20, 2024

kelson42 commented Sep 21, 2024

kelson42 left a comment

Choose a reason for hiding this comment

veloman-yunkan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sgourdas Sep 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sgourdas commented Sep 27, 2024

sgourdas commented Sep 12, 2024 •

edited by kelson42

Loading

codecov bot commented Sep 12, 2024 •

edited

Loading

kelson42 left a comment •

edited

Loading

sgourdas Sep 24, 2024 •

edited

Loading