lxd/instance: Allow setting lxc.net config keys through raw.lxc (from Incus) #16089
Conversation
simondeziel
changed the title
simondeziel
force-pushed
the
instance-utils-unit-tests
branch
2 times, most recently
from
9fc5ed6 to
319d3fe
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR allows setting lxc.net config keys through raw.lxc configuration by removing previous restrictions. The change originates from Incus and enables broader network configuration options for LXD instances.
Key changes:
- Removes validation restrictions on
lxc.net.*configuration keys - Improves MAC address generation using a more efficient approach
- Modernizes string splitting using
strings.Cutinstead ofstrings.SplitN
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| lxd/instance/instance_utils.go | Removes network config restrictions, updates string parsing and MAC generation |
| lxd/instance/instance_utils_test.go | Adds comprehensive test coverage for parsing and validation functions |
Comments suppressed due to low confidence (2)
lxd/instance/instance_utils_test.go:64
- The error message says "Expected empty key" but this assertion is checking if the actual key matches the expected key, not if it's empty. The message should be "Expected key to match for test case: %s"
assert.Equal(t, test.key, key, "Expected empty key for test case: %s", test.name)
lxd/instance/instance_utils_test.go:65
- The error message says "Expected empty value" but this assertion is checking if the actual value matches the expected value, not if it's empty. The message should be "Expected value to match for test case: %s"
assert.Equal(t, test.val, val, "Expected empty value for test case: %s", test.name)
229fc9f to
e15341f
Compare
Signed-off-by: Simon Deziel <[email protected]>
Signed-off-by: Simon Deziel <[email protected]>
It's still usually a bad idea but there's no strong need to special case those vs the rest. Signed-off-by: Stéphane Graber <[email protected]> (cherry picked from commit e13e674dae437564eb25e59b9145bf18a81d8171) Signed-off-by: Simon Deziel <[email protected]> License: Apache-2.0
Signed-off-by: Simon Deziel <[email protected]>
simondeziel
force-pushed
the
instance-utils-unit-tests
branch
from
e15341f to
fff3494
Compare
Signed-off-by: Simon Deziel <[email protected]>
Signed-off-by: Simon Deziel <[email protected]>
Signed-off-by: Simon Deziel <[email protected]>
simondeziel
force-pushed
the
instance-utils-unit-tests
branch
from
fff3494 to
de9b51b
Compare
|
@simondeziel whats the reason behind needing this? @skozina should we be adding more raw.* support for specific container runtimes? |
|
Yup, I thought we want to limit the number of low-level tunables in general, to avoid the cases when LXD lags behind the latest changes in the lower layer configs. Curious to hear more about the need here. |
Saw this pass by and found the (dropped) validation could have been done more efficiently. Since this was only reachable via |
It's no adding support for more top level |
|
@simondeziel thanks, is this needed for something you're working on? |
No. It seems like you have reasons not to want the cherry-pick which is fine with me (I just don't know why but that's OK :) |
tomponline
left a comment
tomponline
left a comment
There was a problem hiding this comment.
As this has plenty of tests and the actual liblxc integration change removes code I'm going to go ahead with the merge. Thanks!
|
Sound good to me. Considering this is an existing unsupported tunnable, there's really no need to have the explicit restrictions for the IP address only. |
4 participants
No description provided.