-
Notifications
You must be signed in to change notification settings - Fork 21
feat: Allow specifying vmnet network UUID to disable DHCP (on vmnet.h… #141
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.
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
base: master
Are you sure you want to change the base?
Conversation
…ost network only) This commit introduces a new `--vmnet-network-uuid` command-line option to allow setting the `vmnet_network_identifier_key` for vmnet. This property is only applicable to a vmnet_interface in VMNET_HOST_MODE. If this property is set, the vmnet_interface is added to an isolated network with the specified identifier. No DHCP service is provided on this network. This is useful for certain applications where the users need an isolated network and are running their own dhcp to assign IPs in such network. See issue [lima-vm#139](lima-vm#139) Signed-off-by: Angelo Failla <[email protected]>
1d3ead9
to
88d975d
Compare
Signed-off-by: Angelo Failla <[email protected]>
Signed-off-by: Angelo Failla <[email protected]>
Signed-off-by: Angelo Failla <[email protected]>
Signed-off-by: Angelo Failla <[email protected]>
9d431ca
to
9750c51
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pallotron thanks! This Looks like an interesting feature but it is not clear what is the purpose of the new identifier.
printf("--vmnet-interface-id=UUID vmnet interface ID (default: " | ||
"random)\n"); | ||
printf("--vmnet-network-identifier=UUID vmnet network identifier (UUID string, \"random\", " | ||
"or \"\")\n" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the meaning of empty string?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it was suggested in here but I had to abandon that PR because I messed it up :D (I am learning jj
to replace git
and I messed things up).
I actually do not like it myself. should I remove this option? "random" work ok for my project.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current code has this behavior:
Good:
- option not used: vment_network_identifier is UUID_NULL since we calloc() the options struct
--vment-network-identifier C2A4A7C3-1912-412F-B928-12CA9B1762A0
: use the provided uuid--vment-network-identifier invalid-uuid-value
: fails with invalid input error
Bad:
--vment-network-identifier ""
: call uuid_clear() which resets value of UUID variable to the NULL value - not needed since the uuid is already the NULL value. Supporting empty value makes the online help more confusing and makes not sense.--vment-network-identifier random
: create a random uuid. Not needed since the user can provide this easily with uuidgen or with the programing language uuid facility. Supporting this complicates the the online help for no benefit.
cli.c
Outdated
"or \"\")\n" | ||
" When vmnet mode is \"host\" and --vmnet-gateway is " | ||
"not set, the internal DHCP will be disabled.\n" | ||
" (default: \"random\")\n"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How the default can be random? We always set the network identifier? This sounds wrong. If you don't specify --vmnet-network-identifier you should not set anything int he connection dictionrary. If user specify an identifier, set it. Keep it simple.
if (strcmp(optarg, "random") == 0) { | ||
uuid_generate_random(res->vmnet_network_identifier); | ||
} else if (strcmp(optarg, "") == 0) { | ||
uuid_clear(res->vmnet_network_identifier); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like invalid value, fail instead of trying to make it work. If this option is set, the user must provide a valid uuid.
main.c
Outdated
xpc_dictionary_set_uuid(dict, vmnet_network_identifier_key, cliopt->vmnet_network_identifier); | ||
uuid_string_t uuid_str; | ||
uuid_unparse(cliopt->vmnet_network_identifier, uuid_str); | ||
INFOF("Using network identifier \"%s\" and no vmnet gateway -> NO DHCP will be enabled on this vmnet", uuid_str); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should not log this here - this value should be logged when parsing options.
If not setting vment_gateway disables DHCP, what it the purpose of the network identifier? Is it used in some other place in the system?
If you create more than one vms with the same network identified, they can access each other?
When we understand the purpose of the identifier, we should document in the new section the README.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If not setting vment_gateway disables DHCP, what it the purpose of the network identifier? Is it used in some other place in the system?
The only explanation I found is on/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/System/Library/Frameworks/vmnet.framework/Headers/vmnet.h
The apple API is extremely bad/confusing, I believe this is why they are coming up with the new vmnet_network_configuration_disable_dhcp
API.. because they know themselves this current way of setting it up is confusing :D
If you create more than one vms with the same network identified, they can access each other?
yes ofc that is the whole point, they are on the same network, this solves my problem for my lab project :)
When we understand the purpose of the identifier, we should document in the new section the README.
The identifier + absence of gateway is the trigger for no dhcp, I know... it's shit... this is Apple... I have a feeling they know it's shit and as I said vmnet_network_configuration_disable_dhcp
will fix this confusion.
Thanks for the feedback! You've raised a great point about the purpose of the new identifier. Let me clarify. The primary goal of this change is to enable running a custom DHCP server on a vmnet network, which is essential for specialized setups like a PXE boot environment. The ProblemI'm building a virtual lab to test our production PXE boot stack. This involves a "provisioner" VM (running dhcpd, tftp, etc.) and multiple "client" VMs that network-boot from it. For this to work, the client VMs and the provisioner need to be on an isolated network where my custom dhcpd can manage IP allocation. The issue is that vmnet.framework runs its own DHCP server on VMNET_[HOST|SHARED]_MODE networks by default because it provides IP addresses to the VMs that your are booting. The SolutionWhile digging into the vmnet.framework headers, I found a solution provided by Apple. According to
By setting the vmnet_network_identifier_key, we can create an isolated VMNET_HOST_MODE network without the conflicting DHCP service. This allows the custom DHCP server in the provisioner VM to function correctly. This feature has been available since macOS 11.0. This allows me to start multiple client VMs on the same socket_vmnet socket, they can take IP addresses from provisioner VM just fine, the provisioner VM also work as a router and can forward client VMs traffic to the internet. It's also worth noting that a more explicit API is coming in the future. The same header file mentions:
This I hope this clears things up! Let me know if you have any other questions. |
This sounds interesting. I think it would be nice to also provide a template with DHCP, router, and maybe even firewall and proxy, to show users how they can set up isolated networks for testing. |
Are you saying in this PR, I think it would be out of scope for this particular PR. But I agree with you it would be nice. |
printf("--vmnet-interface-id=UUID vmnet interface ID (default: " | ||
"random)\n"); | ||
printf("--vmnet-network-identifier=UUID vmnet network identifier (UUID string, \"random\", " | ||
"or \"\")\n" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current code has this behavior:
Good:
- option not used: vment_network_identifier is UUID_NULL since we calloc() the options struct
--vment-network-identifier C2A4A7C3-1912-412F-B928-12CA9B1762A0
: use the provided uuid--vment-network-identifier invalid-uuid-value
: fails with invalid input error
Bad:
--vment-network-identifier ""
: call uuid_clear() which resets value of UUID variable to the NULL value - not needed since the uuid is already the NULL value. Supporting empty value makes the online help more confusing and makes not sense.--vment-network-identifier random
: create a random uuid. Not needed since the user can provide this easily with uuidgen or with the programing language uuid facility. Supporting this complicates the the online help for no benefit.
ERRORF("Failed to parse UUID \"%s\"", optarg); | ||
goto error; | ||
} | ||
break; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets replace the section with:
case CLI_OPT_VMNET_NETWORK_IDENTIFIER:
if (uuid_parse(optarg, res->vmnet_network_identifier) < 0) {
ERRORF("Failed to parse UUID \"%s\"", optarg);
goto error;
}
break;
xpc_dictionary_set_string(dict, vmnet_start_address_key, cliopt->vmnet_gateway); | ||
xpc_dictionary_set_string(dict, vmnet_end_address_key, cliopt->vmnet_dhcp_end); | ||
xpc_dictionary_set_string(dict, vmnet_subnet_mask_key, cliopt->vmnet_mask); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will be better to keep the block setting the gateway address as is. vmnet docs do not mention that you must not use gateway address and network identifier together, and the purpose of the new flag is not to disable DHCP, but to expose additional vment option, that can be used to disable DHCP.
INFOF("Using network identifier \"%s\" and no vmnet gateway -> NO DHCP will be enabled on " | ||
"this vmnet", | ||
uuid_str); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on vmnet.h, vmnet_network_identifier_key can only be used in VMNET_HOST_MODE, but vment_start_address_key, vment_end_addres_key, and vment_subnet_mask are not mentioned.
It makes sense that they should not be used together, since these are all DHCP options, and vmnet_network_identifier_key disables DHCP, but did you try vment_start_address_key together with the DHCP options? What is the outcome? Can this be useful to someone?
If using both options can have some value, we should limit this option only when the DHCP option are not specified.
If using both options is not useful, we should document this here to explain this limit.
The vment.h header also mentions these options:
/*!
* @constant vmnet_host_ip_address_key
* The IPv4 address (string) to be set on the host interface.
*
* This property is only applicable if vmnet_network_identifier_key
* is also specified.
*/
extern const char * const
vmnet_host_ip_address_key API_AVAILABLE(macos(11.0)) API_UNAVAILABLE(ios, watchos, tvos);
/*!
* @constant vmnet_host_subnet_mask_key
* The IPv4 subnet mask (string) to be set on the host interface.
*
* Must be specified with vmnet_host_ip_address_key.
*/
extern const char * const
vmnet_host_subnet_mask_key API_AVAILABLE(macos(11.0)) API_UNAVAILABLE(ios, watchos, tvos);
/*!
* @constant vmnet_host_ipv6_address_key
* The IPv6 address (string) to be set on the host interface.
*
* This property is only applicable if vmnet_network_identifier_key
* is also specified.
*/
extern const char * const
vmnet_host_ipv6_address_key API_AVAILABLE(macos(11.0)) API_UNAVAILABLE(ios, watchos, tvos);
All of these can be used only in vmnet_network_identifier_key is specified, so we if we provide this option, we should also provide the additional options.
I know this may not be related to your use case of testing pre server, but socket_vment can be used or other purposes.
- [How to setup a vmnet host network without DHCP?](#how-to-setup-a-vmnet-host-network-without-dhcp) | ||
- [Links](#links) | ||
- [Troubleshooting](#troubleshooting) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new toc looks wrong. This was not part of the top before:
- [socket_vmnet: vmnet.framework support for rootless and VDE-less QEMU](#socket_vmnet-vmnetframework-support-for-rootless-and-vde-less-qemu)
And the rest of the options are over indented now.
You should should be only:
+ - [How to setup a vmnet host network without DHCP?](#how-to-setup-a-vmnet-host-network-without-dhcp)
Maybe the instructions for running doctor are not correct, or you are running a different version?
Lets separate the README.md change to another PR. We ned to find how to update the README.md without adding unwanted changes.
This commit introduces a new
--vmnet-network-uuid
command-line option to allow setting thevmnet_network_identifier_key
for vmnet.This property is only applicable to a vmnet_interface in
VMNET_HOST_MODE
.If this property is set, the vmnet_interface is added to an isolated network with the specified identifier.
No DHCP service is provided on this network.
This is useful for certain applications where the users need an isolated network and are running their own dhcp to assign IPs in such network.
See issue #139