search

kiwix / libkiwix

Common code base for all Kiwix ports
https://download.kiwix.org/release/libkiwix/
GNU General Public License v3.0
112 stars 54 forks source link

Add IPv6 support to HTTP daemon #1074

Closed aryanA101a closed 1 month ago

aryanA101a commented 2 months ago

resolves #1065

Summary of changes:

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

Tested on Ubuntu 22.04.4 LTS

aryanA101a commented 2 months ago

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

veloman-yunkan commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

Sure

kelson42 commented 1 month ago

@aryanA101a Is that ready for a new review?

aryanA101a commented 1 month ago

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

kelson42 commented 1 month ago

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

kelson42 commented 1 month ago

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

kelson42 commented 1 month ago

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

aryanA101a commented 1 month ago

@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 commented 1 month ago

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

aryanA101a commented 1 month ago

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

https://github.com/kiwix/kiwix-build/pull/697

I'll need some input from @mgautierfr

kelson42 commented 1 month ago

@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 commented 1 month ago

@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 commented 1 month ago

OK, then merging so we can finally move forward.

veloman-yunkan commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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