feat(ipv6): Add support to IPv6 port-forwarding

[heyvito]

As discussed on #1540 and over Slack with @jandubois, this adds support for forwarding IPv6 ports to the host. This also updates test-port-forwarding.pl to obtain the host's IPv6 address and store it on $ipv6, allowing said script to be further developed in order to accommodate future IPv6 tests to use IPv6 addresses as required given the nature of changes in the forwarding rules.

I think I only changed rules that were expecting IPv4 bindings to expect IPv6, but please double-check it <3

Closes #1540.

[jandubois]

I haven't reviewed the changes to the pseudo loopback forwarder yet, and am out of time for now, but here is some initial feedback:

[AkihiroSuda]

@jandubois PTAL

[AkihiroSuda]

@lima-vm/maintainers What should we do with this?

[AkihiroSuda]

Sorry, needs rebase

[AkihiroSuda]

@lima-vm/maintainers
This one has been open for more than 4 months, so I'll merge this one if you have no comment, but feel free to stop me (with a comment)

[heyvito]

@AkihiroSuda Would you mind if I rebase and force-push?

[AkihiroSuda]

@AkihiroSuda Would you mind if I rebase and force-push?

Please rebase and force-push 👍

[jandubois]

This one has been open for more than 4 months, so I'll merge this one if you have no comment, but feel free to stop me (with a comment)

This is totally on me; I know I said I had a plan to make this backwards compatible, and never followed up on it. Now I cannot even remember the plan anymore, but I think I had some private conversation on Slack with @heyvito, that I will need to re-read.

Please give me until Monday evening to comment on this, and go ahead merging it if I don't manage to reply by then.

And @heyvito, please go ahead and rebase so it can be tested with the latest master code.

[jandubois]

Hi @heyvito, I want to apologize again for taking so long to get back to you about this PR!

Below are my notes from reviewing it again today, plus an alternate proposal which I think simplifies the logic, maintains backwards compatibility, and provides extended functionality.

This is the rough first draft; normally I would let it simmer and refine it over a day or two before posting, but I promised to deliver something today. So it is possible that there are some errors/thinkos in that review.

Also let's wait for some feedback on my proposal from the other maintainers before you go in and make changes...

Ok, here it goes...

The root problem

The portForwarding rules treat 0.0.0.0 the same as :: (and 127.0.0.1 the same as ::1). So it is impossible to write a rule that only matches an IPv6 address, but not also the corresponding IPv4 one as well.

This PR was created to fix #1540, which asks for forwarding IPv6 ports in the guest to IPv6 ports on the host (and the same for IPv4).

The implementation

This PR changes the way an unspecified hostIP works when the guestIP is either 0.0.0.0 or 127.0.0.1.

The current implementation sets the hostIP to the guestIP (from the rule, not the actual port binding).

portForwards:
- guestIP: 127.0.0.1
  guestPort: 8080

This will forward [::1]:8080 in the guest to 127.0.0.1:8080 on the host.

This PR changes it to forward to [::1]:8080 on the host.

The same changes apply to forwarding ports on ::.

unspecifiedFallbackRule

There is also a special exception to regular rule processing:

If the rule requires guestIPMustBeZero, the guestIP requests 0.0.0.0 or ::, and the actual IP matches, but is from a different family (IPv6 instead of IPv4, or vice versa), then the rule is ignored at the original location, but retried at the very end, in case no other rule matches.

It is actually not clear to me why this rule was needed, and why the same effect cannot be achieved with rule re-ordering.

Problems with this approach

It breaks backwards compatibility

This is obvious from the description above, and the fact that 3 test cases needed to be changed to reflect the new behaviour.

I agree that the set of users affected by this will probably be pretty small (or possibly even empty), but I like to avoid breaking backwards compatibility on principle (when possible with reasonable effort).

The solution is incomplete

While this solution solves the issue of forwarding IPv6 to IPv6 and IPv4 to IPv4 ports, it does not allow to have different rules for IPv6 and IPv4.

For example, assume you want to forward :: to :: on the host, but ignore all IPv4 bindings:

portForwards:
- guestIP: 0.0.0.0
  ignore: true
- guestIP: "::"

This will not work because the first rule will still match IPv6 ports bound to :: and ignore them. So the second rule would never be hit.

Retrying rules makes the mechanism very hard to understand

I pretty strongly oppose any change that breaks the default rule processing logic: We go through the rules in sequence, the first rule that matches is executed, and we stop processing at that point.

The "unspecified fallback" rule (which is a special case of the proposed "iterate through the rules twice, with relaxed processing on the second round" rule) breaks this model and makes it very hard (at least for me) to predict which rules will be triggered.

Using hostIP: null feels weird

I would prefer if every possible condition can be specified through a settings value. This PR introduces special semantics that only apply when a setting is not specified. The only way to make it explicit in the YAML is via hostIP: null, which does not at all indicate what it is actually doing (match the address family from the guest). If anything it looks like an alias for ignore: true to me.

Proposal

This has happened before

We ran into a similar issue before:

0.0.0.0 matches any address. Which means we did not have a mechanism to create rules only for ports bound to INADDR_ANY without matching all other specific addresses as well.

With hindsight it would have been better to e.g. support guestIP: * to match any address, and restrict guestIP: 0.0.0.0 to only exact matches (of 0.0.0.0 and ::).

But we didn't want to break backwards compatibility, so we introduced the guestIPMustBeZero setting to require the exact match.

We can try the same approach

We are in a very similar situation, in that we want to restrict matching a rule with further conditions.

We could create a guestIPAddressFamily setting with values all, ipv4, or ipv6.

On :: and ::1 you could use all and ipv6.

On 0.0.0.0 and 127.0.0.1 you could use all and ipv4.

On all other guestIP values this setting would be invalid (alternatively: would have to match the family of the address, so e.g. would have to be (and default to) ipv4 if guestIP is 192.168.5.15).

Or we could create guestIPMustMatchAddressFamily, which would be a simple boolean. Not sure what is easier to understand:

portForwards:
- guestIP: '::'
  guestIPAddressFamily: ipv6

or

portForwards:
- guestIP: '::'
  guestIPMustMatchAddressFamily: true

The boolean is less/simpler code; better name suggestions welcome!

This now allows rules to independently match IPv6 and IPv4 addresses. So this would work to implement the functionality requested above, because the first rule no longer matches an IPv6 ports.

portForwards:
- guestIP: 0.0.0.0
  guestIPMustMatchAddressFamily: true
  ignore: true
- guestIP: "::"

This new setting is orthogonal to, and can be freely combined with guestIPMustBeZero to match only INADDR_ANY, or only INADDR6_ANY bindings.

Other feedback

Missing documentation

This PR does not update the portForwards documentation in default.yaml. This should be required before merging.

I think documenting the implementation from the current PR (especially the fallback rule) would have shown that the current implementation in this PR is very difficult to explain to users.

Documentation for test cases

This PR changes 3 existing test cases without documenting why the expected result is different now.

With my proposed changes above I also expect to see additional test cases that it would make possible.

[jandubois]

I've left my comments already in the main thread of this PR.

In summary, I would like to see:

• no breakage of backwards compatibility; no changes to existing tests
• maintain iterating through rules once and stopping at first match
• be able to have separate rules for IPv4 and IPv6
• documentation for any new behaviour
• documented test cases for any new functionality

[jandubois]

Also let's wait for some feedback on my proposal from the other maintainers before you go in and make changes...

@lima-vm/maintainers Any feedback on my proposal? Any suggestions for better names for the proposed setting?

@heyvito Are you still interested in working on this? Do you have any thoughts on my proposed alternate implementation? Anything I overlooked?