Add IPv6 support to HTTP daemon #1074

aryanA101a · 2024-04-20T10:12:39Z

resolves #1065

Summary of changes:

Updated the APIs for retrieving network interfaces on posix-compliant systems and Windows to enable ipv6 support
Added dual stack flag from libmicrohttp to support both ipv4 and ipv6 clients

Compatibility notes
iphlpapi.h-getadaptersaddresses() : Minimum supported client Windows XP

Tested on
Ubuntu 22.04.4 LTS

src/server/internalServer.cpp

veloman-yunkan · 2024-04-22T11:33:29Z

src/tools/networkTools.cpp

+  if (dwRetVal == NO_ERROR) {
+    PIP_ADAPTER_UNICAST_ADDRESS pUnicast = NULL;
+    unsigned int i = 0;
+    for (PIP_ADAPTER_ADDRESSES temp = interfacesHead; temp != NULL;
+    temp = temp->Next) {
+      pUnicast = temp->FirstUnicastAddress;
+            if (pUnicast != NULL) {
+                for (i = 0; pUnicast != NULL; i++){
+                  if (pUnicast->Address.lpSockaddr->sa_family == AF_INET)
+                  {
+                      sockaddr_in *si = (sockaddr_in *)(pUnicast->Address.lpSockaddr);
+                      char host[INET_ADDRSTRLEN]={0};
+                      inet_ntop(AF_INET, &(si->sin_addr), host, sizeof(host));
+                      interfaces[temp->AdapterName].addr=host;
+                  }
+                  else if (pUnicast->Address.lpSockaddr->sa_family == AF_INET6)
+                  {
+                      sockaddr_in6 *si = (sockaddr_in6 *)(pUnicast->Address.lpSockaddr);
+                      char host[INET6_ADDRSTRLEN]={0};
+                      inet_ntop(AF_INET6, &(si->sin6_addr), host, sizeof(host));
+                      if (!IN6_IS_ADDR_LINKLOCAL(&(si->sin6_addr)))
+                        interfaces[temp->AdapterName].addr6=host;
+                  }
+                  pUnicast = pUnicast->Next;
+                }
+            }
    }
+  } else {
+    std::cerr << "Call to GetAdaptersAddresses failed with error: "<< dwRetVal << std::endl;
  }


Fix indentation (replace tabs with spaces)

There are no tabs.

src/tools/networkTools.cpp

veloman-yunkan · 2024-04-22T12:00:06Z

src/server/internalServer.cpp

+      if (0 != INADDR_ANY) {
+        sockAddr6.sin6_addr = in6addr_any;
+      }
+      m_addr = (kiwix::getBestPublicIp(m_ipv6)).addr6;


Well, it looks like this is why we may need the ipv6 boolean parameter. But are there realistic situations where the user trusts the application to select the IP address on its own but enforces the type of the IP address?

None that I can think of.

src/server/internalServer.cpp

aryanA101a · 2024-04-23T09:19:03Z

With the latest refactoring, any type of ip can be used with -i flag without the necessity of specifying -6 flag.

veloman-yunkan

I am writing this summary after having added the review comments and having deleted/edited some of them (as I felt that my feedback starts being inconsistent). I read the discussion at kiwix/kiwix-tools#545 a little too late. My feeling is that the option --ipv6 is not a good way to instruct the kiwix server to support IPv6. Is it supposed to enforce IPv6 or merely enables it?

An alternative may be to use special values for the -i/--address options. For example,

all - listen on all available IP addresses (IPv4 and IPv6)
ipv4 - listen on all available IPv4 addresses
ipv6 - listen on all available IPv6 addresses
auto
network interface name (e.g. eth0, wlan0) - will be resolved to the IP address of the named interface

I think that we have to close in on the use model of this enhancement before proceeding with the PR.

include/tools.h

veloman-yunkan · 2024-04-24T08:37:47Z

include/server.h

@@ -65,6 +65,7 @@ namespace kiwix
       void setIPv6(bool ipv6) { m_ipv6 = ipv6; }
       int getPort();
       std::string getAddress();
+       bool isAddressIPv6();


Make const (in general, shortcomings of existing code shouldn't be copied).

src/tools/networkTools.cpp

include/tools.h

include/server.h

veloman-yunkan · 2024-04-24T08:58:38Z

include/server.h

@@ -78,6 +80,7 @@ namespace kiwix
       bool m_withTaskbar = true;
       bool m_withLibraryButton = true;
       bool m_blockExternalLinks = false;
+       bool m_ipv6 = false;


Get rid of this data member.

src/server/internalServer.h

src/tools/networkTools.cpp

veloman-yunkan · 2024-04-24T09:47:45Z

With the latest refactoring, any type of ip can be used with -i flag without the necessity of specifying -6 flag.

What happens if you specify an IPv4 address together with the -6 flag?

aryanA101a · 2024-04-24T16:58:46Z

I am writing this summary after having added the review comments and having deleted/edited some of them (as I felt that my feedback starts being inconsistent). I read the discussion at kiwix/kiwix-tools#545 a little too late. My feeling is that the option --ipv6 is not a good way to instruct the kiwix server to support IPv6. Is it supposed to enforce IPv6 or merely enables it?

Current model:
-i lets you provide both types of ip addr
-6 just tries to enforce ipv6. In case no ip address is provided it makes sure best public ip is ipv6
In either case an explicit ipv6 address or -6 ipv6 enforce flag, dual stack flag is turned on from libmicrohttp to support connections from both ipv4 and ipv6 clients

An alternative may be to use special values for the -i/--address options. For example,

ipv4 - listen on all available IPv4 addresses

ipv6 - listen on all available IPv6 addresses

all - listen on all available IP addresses (IPv4 and IPv6)

auto

Seems a good approach.

network interface name (e.g. eth0, wlan0) - will be resolved to the IP address of the named interface

I don't think a normal person will care about the specific network interfaces, so adding this much sugar seems redundant.
I think that ipv6 support is more about ensuring compatibility with ipv6 clients, although it does let us serve on ipv6 addresses, but that's not the sauce.

I think that we have to close in on the use model of this enhancement before proceeding with the PR.

      -f, -family <FAMILY>
              Specifies  the  protocol family to use. The protocol family identifier can be one of inet, inet6, bridge, mpls
              or link.  If this option is not present, the protocol family is guessed from other arguments. If the  rest  of
              the  command line does not give enough information to guess the family, ip falls back to the default one, usu‐
              ally inet or any.  link is a special family identifier meaning that no networking protocol is involved.

       -4     shortcut for -family inet.

       -6     shortcut for -family inet6.

from the man page of ip command

I'd still argue that the -6 flag is a concise way to enforce IPv6, though your suggestion seems like a good approach as well.

@veloman-yunkan @kelson42 What do you think?

aryanA101a · 2024-04-24T17:02:36Z

With the latest refactoring, any type of ip can be used with -i flag without the necessity of specifying -6 flag.

What happens if you specify an IPv4 address together with the -6 flag?

@aryanA101a ➜ /workspaces/kiwix-tools (main) $ kiwix-serve wikipedia_en_100_mini_2023-08.zim -p 2002 -6 -i 127.0.0.1
Ip address 127.0.0.1  is not a valid ipv6 address

kelson42 · 2024-05-04T16:29:18Z

An alternative may be to use special values for the -i/--address options. For example,

ipv4 - listen on all available IPv4 addresses

ipv6 - listen on all available IPv6 addresses

all - listen on all available IP addresses (IPv4 and IPv6)

auto

Seems a good approach.

@aryanA101a Thank you for your patience. I have discussed the case with @veloman-yunkan and we agreed that this proposal was appropriate and leads to the removal of -4 and/or -6. I also don't see what is the value of auto!? Would you be able to change the interface that way?

aryanA101a · 2024-05-06T17:18:32Z

Sure

kelson42 · 2024-05-11T09:24:43Z

@aryanA101a Is that ready for a new review?

aryanA101a · 2024-05-11T09:46:33Z

@mgautierfr I have modified the meson.build, but still win32-dyn build is failing.

kelson42 · 2024-05-18T09:36:04Z

@aryanA101a @veloman-yunkan Not sure about the status here, ready to review?

veloman-yunkan

Please rebase the PR and beautify its history (in the worst case it should consist of a single commit, but I would appreciate if you can split it into a few meaningful incremental changes).

veloman-yunkan · 2024-05-18T13:27:10Z

include/common.h

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

 namespace kiwix {

+enum IpMode { ipv4, ipv6, all };


Change to enum class (so that the members of this enumeration - and especially all - can be used only when qualified with the type name).

kelson42 · 2024-05-20T17:56:10Z

@aryanA101a We are that far that we are almost ready to merge, any chance we can do that soon?

kelson42 · 2024-05-22T08:56:03Z

@aryanA101a @veloman-yunkan I can not merge with what looks like legit CI errors.

aryanA101a · 2024-05-22T09:15:33Z

@mgautierfr I have modified the meson.build, but still win32-dyn build is failing.

https://github.com/kiwix/kiwix-build/blob/82500c545b386e102819a901508eeb7fbc59a048/kiwixbuild/configs/win32.py#L12
https://github.com/kiwix/libkiwix/actions/runs/9172701816/job/25267685133?pr=1074#step:5:349

kelson42 · 2024-05-22T09:33:05Z

@aryanA101a If this PR requires a change at Kiwix Build, please create an issue explaining what is needed.

aryanA101a · 2024-05-23T09:37:13Z

@aryanA101a If this PR requires a change at Kiwix Build, please create an issue explaining what is needed.

kiwix/kiwix-build#697

I'll need some input from @mgautierfr

mgautierfr · 2024-05-23T13:41:21Z

meson.build

@@ -48,7 +48,10 @@ if host_machine.system() == 'windows' and static_deps
 endif

 if host_machine.system() == 'windows'
-  add_project_arguments('-DNOMINMAX', language: 'cpp')
+     add_project_arguments('-DNOMINMAX', language: 'cpp')
+     extra_link_args = ['-lshlwapi', '-lwinmm', '-lWs2_32', '-liphlpapi']


extra_link_args is use as link arguments when compiling tests.
When compiling the library itself (and setting pkg_config link flags which will be used by projects linking with libzim), we use extra_libs.
That's why CI is still failing for Windows.

BTW, we should probably use only one variable extra_libs to avoid this kind of mistake. When fixing the pr, please also switch to extra_libs only.

kelson42 · 2024-05-25T08:20:05Z

@aryanA101a @veloman-yunkan Last commit seems to fix the compilation problem to create binary for Windows. But this commit in significant and we have to request new review

veloman-yunkan

To the best of my understanding the only change since the last review was in meson.build. Then my previous approval still holds.

However, regarding the commit structure of this PR, I think that all changes to the source code (*.cpp, *.h files) should belong to one commit. The fix for building under windows should be in a separate commit.

veloman-yunkan · 2024-05-29T15:23:13Z

@aryanA101a @veloman-yunkan Last commit seems to fix the compilation problem to create binary for Windows. But this commit in significant and we have to request new review

That's a false impression. The last commit (which combines two unrelated changes) was amended with a small change that finally fixed the windows build. In a more reviewer friendly flow a fixup commit should have been created.

kelson42 · 2024-05-29T19:05:49Z

OK, then merging so we can finally move forward.

veloman-yunkan · 2024-05-31T11:32:21Z

Note that this PR is a backward incompatible API change. Which means that the next release of libkiwix must be 14.0 (unless backward compatibility is added).

@mgautierfr BTW, why are the versions of openzim and kiwix projects defined in the kiwix-build repository instead of in every project in a standardized way?

mgautierfr · 2024-05-31T12:02:31Z

BTW, why are the versions of openzim and kiwix projects defined in the kiwix-build repository instead of in every project in a standardized way?

It is defined in every project as a dependency constraint. See for example
https://github.com/kiwix/kiwix-tools/blob/main/meson.build#L21-L22
Or each project define it own version like in https://github.com/kiwix/libkiwix/blob/main/meson.build#L2

kiwix-build is defining what we are building in our CI/release (and it must fulfills the constraints define in every project). Other packagers are free to use different version (as long as constraints are respected)

veloman-yunkan · 2024-05-31T12:35:21Z

Indeed. I somehow failed to locate the version in meson.build. Sorry for the silly question.

aryanA101a mentioned this pull request Apr 20, 2024

Add IPv6 support to Kiwix Server kiwix/kiwix-tools#673

Merged

kelson42 changed the title ~~add ipv6 support~~ Add IPv6 support to HTTP daemon Apr 20, 2024

kelson42 requested review from mgautierfr and veloman-yunkan April 20, 2024 17:46

aryanA101a mentioned this pull request Apr 22, 2024

Add -iphlpapi compilation flag to windows config kiwix/kiwix-build#696

Closed

veloman-yunkan requested changes Apr 22, 2024

View reviewed changes

aryanA101a force-pushed the main branch 2 times, most recently from 13c5dbd to bd8db7e Compare April 24, 2024 09:22

veloman-yunkan requested changes Apr 24, 2024

View reviewed changes

veloman-yunkan mentioned this pull request Apr 24, 2024

kiwix-serve does not support IPv6 kiwix/kiwix-tools#545

Closed

aryanA101a force-pushed the main branch from 98fc671 to bb8775b Compare May 8, 2024 16:44

kelson42 requested a review from veloman-yunkan May 11, 2024 10:53

veloman-yunkan requested changes May 18, 2024

View reviewed changes

aryanA101a force-pushed the main branch from bb8775b to 6ed4118 Compare May 21, 2024 09:58

veloman-yunkan approved these changes May 22, 2024

View reviewed changes

mgautierfr reviewed May 23, 2024

View reviewed changes

aryanA101a force-pushed the main branch from 6ed4118 to 4354007 Compare May 25, 2024 08:11

kelson42 requested a review from veloman-yunkan May 25, 2024 08:19

kelson42 mentioned this pull request May 27, 2024

Add support of IPv6 IP to the HTTP daemon mode kiwix/kiwix-desktop#1119

Closed

veloman-yunkan reviewed May 29, 2024

View reviewed changes

aryanA101a added 2 commits May 29, 2024 21:05

add ipv6 support to HTTP daemon

a6cf161

handle ip modes & add compilation flags for windows build

b7eadf9

kelson42 force-pushed the main branch from 4354007 to b7eadf9 Compare May 29, 2024 19:05

kelson42 merged commit a1ce3d1 into kiwix:main May 29, 2024
14 checks passed

mgautierfr mentioned this pull request Jun 3, 2024

Fix wrong usage of extra_link_args variable. #1092

Merged

veloman-yunkan mentioned this pull request Jun 3, 2024

Backward compatible support for IPv6 #1093

Merged

veloman-yunkan mentioned this pull request Sep 23, 2024

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add IPv6 support to HTTP daemon #1074

Add IPv6 support to HTTP daemon #1074

aryanA101a commented Apr 20, 2024 •

edited

Loading

veloman-yunkan Apr 22, 2024

aryanA101a Apr 23, 2024

veloman-yunkan Apr 22, 2024

aryanA101a Apr 22, 2024 •

edited

Loading

aryanA101a commented Apr 23, 2024

veloman-yunkan left a comment

veloman-yunkan Apr 24, 2024

veloman-yunkan Apr 24, 2024

veloman-yunkan commented Apr 24, 2024

aryanA101a commented Apr 24, 2024 •

edited

Loading

aryanA101a commented Apr 24, 2024

kelson42 commented May 4, 2024

aryanA101a commented May 6, 2024

kelson42 commented May 11, 2024

aryanA101a commented May 11, 2024

kelson42 commented May 18, 2024

veloman-yunkan left a comment

veloman-yunkan May 18, 2024

kelson42 commented May 20, 2024

kelson42 commented May 22, 2024

aryanA101a commented May 22, 2024

kelson42 commented May 22, 2024

aryanA101a commented May 23, 2024

mgautierfr May 23, 2024

kelson42 commented May 25, 2024

veloman-yunkan left a comment

veloman-yunkan commented May 29, 2024 •

edited

Loading

kelson42 commented May 29, 2024

veloman-yunkan commented May 31, 2024

mgautierfr commented May 31, 2024

veloman-yunkan commented May 31, 2024

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

		namespace kiwix {

		enum IpMode { ipv4, ipv6, all };

Add IPv6 support to HTTP daemon #1074

Add IPv6 support to HTTP daemon #1074

Conversation

aryanA101a commented Apr 20, 2024 • edited Loading

veloman-yunkan Apr 22, 2024

Choose a reason for hiding this comment

aryanA101a Apr 23, 2024

Choose a reason for hiding this comment

veloman-yunkan Apr 22, 2024

Choose a reason for hiding this comment

aryanA101a Apr 22, 2024 • edited Loading

Choose a reason for hiding this comment

aryanA101a commented Apr 23, 2024

veloman-yunkan left a comment

Choose a reason for hiding this comment

veloman-yunkan Apr 24, 2024

Choose a reason for hiding this comment

veloman-yunkan Apr 24, 2024

Choose a reason for hiding this comment

veloman-yunkan commented Apr 24, 2024

aryanA101a commented Apr 24, 2024 • edited Loading

aryanA101a commented Apr 24, 2024

kelson42 commented May 4, 2024

aryanA101a commented May 6, 2024

kelson42 commented May 11, 2024

aryanA101a commented May 11, 2024

kelson42 commented May 18, 2024

veloman-yunkan left a comment

Choose a reason for hiding this comment

veloman-yunkan May 18, 2024

Choose a reason for hiding this comment

kelson42 commented May 20, 2024

kelson42 commented May 22, 2024

aryanA101a commented May 22, 2024

kelson42 commented May 22, 2024

aryanA101a commented May 23, 2024

mgautierfr May 23, 2024

Choose a reason for hiding this comment

kelson42 commented May 25, 2024

veloman-yunkan left a comment

Choose a reason for hiding this comment

veloman-yunkan commented May 29, 2024 • edited Loading

kelson42 commented May 29, 2024

veloman-yunkan commented May 31, 2024

mgautierfr commented May 31, 2024

veloman-yunkan commented May 31, 2024

aryanA101a commented Apr 20, 2024 •

edited

Loading

aryanA101a Apr 22, 2024 •

edited

Loading

aryanA101a commented Apr 24, 2024 •

edited

Loading

veloman-yunkan commented May 29, 2024 •

edited

Loading