lima
lima copied to clipboard
lima-vm
feat(ipv6): Add support to IPv6 port-forwarding
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.
@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)
@AkihiroSuda Would you mind if I rebase and force-push?
Please rebase and force-push 👍
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.
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.
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?