Does this also work if temporary addresses are used? The address being published via global discovery would need to match the IP we're requesting the pinhole for. see #8453 for more context.

Does this also work if temporary addresses are used? The address being published via global discovery would need to match the IP we're requesting the pinhole for. see #8453 for more context.

This seems to work. In my testing setup, it opens a pinhole to the SLAAC-assigned IPv6 which seems to be the one advertised to the discovery service

@maxi0604 I just tested this on my home LAN, with an IPv6 and CGNAT IPv4 connection to the ISP. It seems to work partially, but I couldn't really make sense of the debugging log messages. Would you care to look at the debug logs? How can I send them to you privately?

@maxi0604 I just tested this on my home LAN, with an IPv6 and CGNAT IPv4 connection to the ISP. It seems to work partially, but I couldn't really make sense of the debugging log messages. Would you care to look at the debug logs? How can I send them to you privately?

@acolomb I just noticed that my e-mail wasn't visible on github, you should be able to mail them to me now

Sent the logs and tried once more with explicit listen addresses set for quic6://[::]:22123 and tcp6://[::]:22123. Strange enough, the router still says the device can open ports on its own but doesn't have any active mappings. The log does mention that the mapping was added, but it gets removed again exactly 7 minutes later.

Now it does open a port, but the router says it's for IPv4 (TCP/22123).

Still wondering about those connect: invalid argument and Error getting new lease on NAT-PMP@192.168.0.1 unexpected result size 12, expected 16 errors by the way.

I haven't changed anything substantial for NAT-PMP so that error message is weird but likely not related.

With UPnP I think the code has to become a little bit more complicated. The assumption I made was that I could just have the code create a NAT device for IPv4 which contacts the gateway via IPv4 and one for IPv6 which contacts the gateway via IPv6. Then I could just use the local IP used to connect to the gateway for the request, like in the IPv4 case.

There's already a comment in the code about how that is a roundabout approach for IPv4 but in the log you sent it breaks completely for IPv6. I am thinking that the reason is that your router advertises the IPv6 UPnP service under a link-local address which a) we can't connect to without a zone index b) even if we could just connect, as mentioned before, the IP guessing mechanism breaks here since connecting to a link-local local address will probably select the link-local address of the interface the syncthing instance is running on, which we can't open pinhole to by definition.

I'm thinking the solution looks something like this:

1. Allow delivering both port mapping and pinholing requests via IPv4 and IPv6. This is done with commit 5a16ea4.
2. Somehow get the LocalIP of an UPNP nat.Device to be equal to the GUA IPv6 we're listening on if it's a WANIPv6FirewallControl device or equal to the local IPv4 if it is one of the other types. As mentioned before here, ideally the IPv6 here is the same one sent to the discovery service
3. Handle zone indices so that UPnP traffic can be delivered via link-local IPv6 addresses, making this work on networks that are truly IPv6 only with a router configured like the one in your logs (This is lower priority as pinholing should already work when the first two are implemented but would depend on either IPv4 being available on the local network or the gateway not sending a link-local IPv6)

Fine with me to get this finally completed, without further digging into the deduplication stuff or the Windows failure for now. Of course, a follow-up will be very welcome and I do want to help there with further testing and discussion. But let's get this out to users in a basic form first. Hope I can find some time to test and review again soon. We just missed the -rc.1 release so I guess there's time until the next cycle.

I'll have to think about your comments on the proposed logic, it's been too long since. Just one thought: The fact that the actual listeners are not considered when starting discovery and contacting mapping services, is something we should fundamentally reconsider. I think that will solve our current problems with duplicate mappings much easier.

Should we add a GOOS check with a Todo?

I think it might be clever to only check for GOOS after the call has failed and provide a more useful error message. That way, no one needs to remember to change it once the issue in Go is fixed.

@calmh Where do we go from here? We've missed the release candidate again, and I'm not sure the code has had enough review to be merged for RC2.

I see a hilarious amount of discussion, no approvals (the opposite, two open "changes requested" reviews), and the last comments hint towards outstanding issues ("should we add" ... "I think ..."). I also actively avoid getting involved in PR review when there's already multiple other people reviewing it, so as to avoid pile-on. RC timing has nothing to do with merging PRs. Especially not the latest RC which is outside the regular schedule.

I can only judge the functionality, which works as intended on Linux and maybe other Unix OS based on my last tests.

Well the two of you are loudly declaring that the current state is not good for merging, so maybe fix that then:

My point was very clear, it's better to get it out the door and postpone further improvements until later. Sorry it hasn't worked out to do another thorough review in a timely manner. Will do so as time permits. If anything, the last release with nil dereference panics has shown us that one more long hard stare at the code may help to avoid issues early on. And that's what I was planning to do. No investments in the other open comments from @bt90 .

I'm not sure if we're maybe talking past each other. It sounds to me like you're both saying "this is fine, why is it not merged, calmh?" while what I'm saying is that you've both put stakes in the ground saying "this shall not pass!" by filing "changes required" reviews and then not dismissing them or changing them to "approved". If you, as reviewers, think this should be merged then you need to file approving pull requests, and then merge it. This is how code review works on GitHub. It says so in bold, red, text down below.

I'm saying that we shouldn't add more features before merging. I want to review again (and test, mainly) with the current "final" state, because I think my last tests were with a prior iteration. When that's done, I will mark it appropriately. Not trying to hurry anyone.

Sorry it has taken so long on my side. I basically backed off when seeing there were open questions from @bt90. Then I got busy with the Android Weblate integration instead, but that should be done now so next thing will be getting back to this PR :-)

I'm not sure if we're maybe talking past each other. It sounds to me like you're both saying "this is fine, why is it not merged, calmh?" while what I'm saying is that you've both put stakes in the ground saying "this shall not pass!" by filing "changes required" reviews and then not dismissing them or changing them to "approved".

Fair point. I hesitated because my approval doesn't carry much weight, or might even give a false sense of security, to be honest. There is a clear difference between a Go developer being able to properly judge code quality and maintainability, and me being happy with the state I have tested.

In other words, I'm happy with the way it works, but I can't guarantee that the code meets the project's quality standards.

In retrospect, my comment was open to misinterpretation. It wasn't my intention to accuse anyone of inaction, but rather to make sure that my inability to do a proper review wouldn't hold up the progress of this PR.

Fair point. I hesitated because my approval doesn't carry much weight, or might even give a false sense of security, to be honest. There is a clear difference between a Go developer being able to properly judge code quality and maintainability, and me being happy with the state I have tested.

You can also dismiss your review - you don't need to approve if you are not comfortable reviewing. However you shouldn't keep a "red"/"changes requested" review in place if you don't want to block a PR from being merged - that's what this kind of review means. Next to your review there are three dots leaving to a menu that gives you the option to dismiss it.

You can also dismiss your review - you don't need to approve if you are not comfortable reviewing.

Unfortunately, the Github app and the web UI on mobile were not up to the task. The last few days have been a bit chaotic for me. But I guess that's pretty much guaranteed when you become a father 😆

@bt90 Wow, congratulations!