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

Add ipv6 support to local server #1221

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Add ipv6 support to local server #1221

wants to merge 2 commits into from

Conversation

sgourdas
Copy link
Collaborator

This adds ipv6 support to the local kiwix server of the app.

Fixes #1119

kelson42
kelson42 requested changes Sep 29, 2024
View reviewed changes
Copy link
Collaborator

@kelson42 kelson42 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I have compiled with libkiwix #1132 and even if I see now ::1 in the list, each time I start the server, then whole kiwix-desktop crashes. Actually this is the same with kiwix-desktop main.

kelson42
kelson42 approved these changes Oct 2, 2024
View reviewed changes
Copy link
Collaborator

@kelson42 kelson42 left a comment

Choose a reason for hiding this comment

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

Works as expected from a user perspective. Ready for code review.

@sgourdas
Copy link
Collaborator Author

sgourdas commented Oct 2, 2024

I would like to do some additional checks here before reviews, if possible.

@kelson42 kelson42 marked this pull request as draft October 2, 2024 19:52
@sgourdas
Copy link
Collaborator Author

sgourdas commented Oct 5, 2024

@veloman-yunkan this is ready for code review.

P.S. It would be good for testing even to have in place the fix of Server::setAddress from kiwix/libkiwix#1147.

@sgourdas sgourdas marked this pull request as ready for review October 5, 2024 12:29
@kelson42
Copy link
Collaborator

kelson42 commented Oct 8, 2024 •
edited
Loading

So I guess this is the last PR of our IPv6 effort :)

@sgourdas
Copy link
Collaborator Author

sgourdas commented Oct 8, 2024

Now that ipv6 works correctly on Windows give me some additional time to catch any issues before we close this up please

@sgourdas sgourdas marked this pull request as draft October 8, 2024 17:11
@sgourdas
Copy link
Collaborator Author

sgourdas commented Oct 9, 2024

It would be good to merge kiwix/libkiwix#1151 first, as noted here, before finalizing this as well.

@sgourdas
Copy link
Collaborator Author

sgourdas commented Oct 9, 2024 •
edited
Loading

It seems like the root cause helping the discovery of kiwix/libkiwix#1148 is the fact that the Kiwix Desktop local server uses getNetworkInterfacesIPv4Or6 which does return 169.254 prefixed IPs. These are added to the dropdown of IPs "available" for use, which as we discussed leads to issues.

@veloman-yunkan should I filter these out when adding to the list, or should we modify the getNetworkInterfacesIPv4Or6 behavior to not include them?

@veloman-yunkan
Copy link
Collaborator

@sgourdas Neither seems a good option to me. Why don't we present the full list of IPs to the users and let them find out which one works best for them? We can add a note that not all of the IPs in the list may actually work. We can filter the list later after we have enough experience with problematic IPs and/or complaints from our users.

@sgourdas
Copy link
Collaborator Author

sgourdas commented Oct 9, 2024

@sgourdas Neither seems a good option to me. Why don't we present the full list of IPs to the users and let them find out which one works best for them? We can add a note that not all of the IPs in the list may actually work. We can filter the list later after we have enough experience with problematic IPs and/or complaints from our users.

I can see that being best in a "text box scenario" where the input is coming from the user. IMHO if an option is given from a dropdown it should work correctly and have a reason to be presented/add clutter in our app.

On the other hand, filtering after we get more experience with problematic IPs is a good approach. But despite that, this prefix seems to get in the way and kind of obscure what works and what doesn't with how many IPs it provides.

@veloman-yunkan
Copy link
Collaborator

@sgourdas If you can do some research, find out the peculiarity of 169.254.x.x addresses and substantiate why they must be excluded from the IP addresses that we consider, then we can proceed based on that information.

@sgourdas
Copy link
Collaborator Author

sgourdas commented Oct 9, 2024 •
edited
Loading

@sgourdas If you can do some research, find out the peculiarity of 169.254.x.x addresses and substantiate why they must be excluded from the IP addresses that we consider, then we can proceed based on that information.

@veloman-yunkan 169.254.0.0/16 addresses are link local addresses which IMO should be excluded in getNetworkInterfacesWin since its already being done with the ipv6 equivalent. Their use is not destined for this purpose since they are non routable.

@sgourdas
Copy link
Collaborator Author

sgourdas commented Oct 9, 2024

@sgourdas If you can do some research, find out the peculiarity of 169.254.x.x addresses and substantiate why they must be excluded from the IP addresses that we consider, then we can proceed based on that information.

@veloman-yunkan 169.254.0.0/16 addresses are link local addresses which IMO should be excluded in getNetworkInterfacesWin since its already being done with the ipv6 equivalent. Their use is not destined for this purpose since they are non routable.

The corresponding change should also be probably done in getNetworkInterfacesPosix in the same logic.

@veloman-yunkan
Copy link
Collaborator

Their use is not destined for this purpose since they are non routable.

@sgourdas Can you provide a reference to some document supporting your statement?

@veloman-yunkan
Copy link
Collaborator

Their use is not destined for this purpose since they are non routable.

@sgourdas Can you provide a reference to some document supporting your statement?

Such addresses are not globally routable, but they should be usable within the local subnetwork, in which respect they are no different from, for example, 192.168.x.x addresses.

@sgourdas
Copy link
Collaborator Author

sgourdas commented Oct 9, 2024 •
edited
Loading

@veloman-yunkan main sources of my response were this and this.

@sgourdas
Copy link
Collaborator Author

@veloman-yunkan is this enough info to justify such a change?

@veloman-yunkan
Copy link
Collaborator

@sgourdas I am not convinced. The main argument for suppressing 169.254.x.x IPs is that they seem to become of significance only when there is a misconfiguration or some other issue with the network, however that also can be thought of as an argument for not suppressing those IPs. I'll let @kelson42 decide.

@sgourdas
Copy link
Collaborator Author

@veloman-yunkan do you happen to know what is the reason we are excluding IPv6 link locals but not doing the same for IPv4 currently?

@veloman-yunkan
Copy link
Collaborator

@veloman-yunkan do you happen to know what is the reason we are excluding IPv6 link locals but not doing the same for IPv4 currently?

No, I don't.

@sgourdas sgourdas marked this pull request as ready for review October 11, 2024 12:33
@sgourdas
Copy link
Collaborator Author

@veloman-yunkan do you happen to know what is the reason we are excluding IPv6 link locals but not doing the same for IPv4 currently?

No, I don't.

Alright, lets continue with the main purpose of this PR maybe for the moment then.

veloman-yunkan
veloman-yunkan reviewed Oct 11, 2024
View reviewed changes
src/localkiwixserver.cpp Outdated Show resolved Hide resolved
src/localkiwixserver.cpp Outdated Show resolved Hide resolved
@kelson42
Copy link
Collaborator

@sgourdas I am not convinced. The main argument for suppressing 169.254.x.x IPs is that they seem to become of significance only when there is a misconfiguration or some other issue with the network, however that also can be thought of as an argument for not suppressing those IPs. I'll let @kelson42 decide.

I'm pretty much in favour of not proposing this kind of IPs - if they are not reachable by non-local users - in Kiwix Desktop. This will do more arm than good IMHO. I believe the answer is slightly different for Kiwix Server, which is used by tech-savy users. So I would just propose to mask it here in Kiwix Desktop, but keep it available in libkiwix/kiwix-serve (but never choose it per default). Hope this helps.

veloman-yunkan
veloman-yunkan requested changes Oct 12, 2024
View reviewed changes
resources/i18n/en.json Outdated Show resolved Hide resolved
resources/i18n/qqq.json Outdated Show resolved Hide resolved
@sgourdas sgourdas force-pushed the feature/server-ipv6-support branch 2 times, most recently from 2cb2821 to 12da879 Compare October 12, 2024 10:33
veloman-yunkan
veloman-yunkan requested changes Oct 13, 2024
View reviewed changes
src/localkiwixserver.cpp Outdated Show resolved Hide resolved
src/localkiwixserver.cpp Outdated Show resolved Hide resolved
src/localkiwixserver.cpp Outdated Show resolved Hide resolved
@sgourdas sgourdas force-pushed the feature/server-ipv6-support branch 2 times, most recently from f78a795 to 59de95c Compare October 14, 2024 07:43
veloman-yunkan
veloman-yunkan requested changes Oct 14, 2024
View reviewed changes
src/localkiwixserver.cpp Outdated Show resolved Hide resolved
src/localkiwixserver.cpp Outdated Show resolved Hide resolved
veloman-yunkan
veloman-yunkan reviewed Oct 14, 2024
View reviewed changes
src/localkiwixserver.cpp
for (const auto &interface : interfaces) {
ui->IpChooser->addItem(interface);
}
ui->IpChooser->setCurrentText(KiwixApp::instance()->getSettingsManager()->getKiwixServerIpAddress());
QString ipAddress = KiwixApp::instance()->getSettingsManager()->getKiwixServerIpAddress();
ui->IpChooser->setCurrentText(ipAddress == "all_ips" || ipAddress == "ipv4" || ipAddress == "ipv6" ? gt(ipAddress) : ipAddress);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add backward compatibility by converting "0.0.0.0" to "ipv4".

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch, have to start being more aware of that

Copy link
Collaborator Author

@sgourdas sgourdas Oct 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Shouldnt it be converted to "all" though?

Edit: In which case, we do not have to make a change.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I thought about that too, but chose to be conservative.

@kelson42 What's your opinion on this? Should we upgrade the old setting of the local server IP of "0.0.0.0" to all IPs (dual stack IPv4 & IPv6 mode) or just all IPv4 IPs?

Copy link
Collaborator Author

@sgourdas sgourdas Oct 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@kelson42 do you have any suggestion on how to handle this? Should we handle old saved "All" settings as "IPv4 Mode" or "Dual Stack Mode"?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@veloman-yunkan veloman-yunkan veloman-yunkan requested changes

@kelson42 kelson42 kelson42 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add support of IPv6 IP to the HTTP daemon mode
3 participants
@sgourdas @kelson42 @veloman-yunkan