Explicitly pass default subnets into network creation#1424
Explicitly pass default subnets into network creation#1424noah-thor wants to merge 1 commit intoapple:mainfrom
Conversation
The CLI excludes the Before we do any other checking in the client, we should probably guard |
Yes, but it should be |
24716a8 to
e23e656
Compare
jglogan
left a comment
There was a problem hiding this comment.
One comment regarding the unit tests.
|
|
||
| public actor AllocationOnlyVmnetNetwork: Network { | ||
| // The IPv4 subnet to be used if none explicitly passed in the `NetworkConfiguration` | ||
| private static let defaultIPv4Subnet = try! CIDRv4("192.168.64.1/24") |
There was a problem hiding this comment.
We should avoid force-try, though here it should always work. Fine for now though.
If we ever turn on lint checks we'll either have to add exclusions for these, or rework the code.
Unfortunately Prefix in ContainerizationExtras is init? or we could do something like:
private static let defaultIPv4Subnet = CIDRv4(IPv4Address([192, 168, 64, 1]), prefix: .ipv4(24))
There was a problem hiding this comment.
am in favor of leaving the forced unwrap for now, the string format reads better in my option and the surface area for errors should be caught in tests. If we do decide to make that a linter rule we should explore a standard way of handling such "known safe" constants and or have a override mechanism
This changes the semantics around `ReservedVmnetNetwork` and
`AllocationOnlyVmnetNetwork` to only create ipv4/6 subnets for networks
when the values are explicitly passed in the `NetworkConfiguration`.
Previously if the values in the `NetworkConfiguration` were not present
it would attempt to source them via `DefaultsStore`.
The change solves an issue that caused `container network create <name>` to fail
when `UserDefaults` was set to a value that is identical to the hard coded default
("192.168.64.1/24"/"fd00::/64"). It would fail because a network with
these parameters would already exist, and thus could not be reserved.
e23e656 to
bdfbbc7
Compare
|
In the latest commit
|
This changes the semantics around
ReservedVmnetNetworkandAllocationOnlyVmnetNetworkto only create ipv4/6 subnets for networks when the values are explicitly passed in theNetworkConfiguration. Previously if the values in theNetworkConfigurationwere not present it would attempt to source them viaDefaultsStore.Type of Change
Motivation and Context
The change solves an issue that caused
container network create <name>to fail whenUserDefaultswas set to a value that is identical to the hard coded default ("192.168.64.1/24"/"fd00::/64"). It would fail because a network with these parameters would already exist, and thus could not be reserved.Testing