search

canonical / lxd

Powerful system container and virtual machine manager
https://canonical.com/lxd
GNU Affero General Public License v3.0
4.38k stars 931 forks source link

bgp.ipv*.nexthop keys fail validation #12122

Closed mandrav closed 1 year ago

mandrav commented 1 year ago

Required information

Issue description

Trying to set bgp.ipv4.nexthop and/or bgp.ipv6.nexthop fails with error:

Error: Invalid network configuration key: bgp.ipv4.nexthop

Steps to reproduce

  1. lxc network set lxdbr0 bgp.ipv4.nexthop=x.y.z.w results in Error: Invalid network configuration key: bgp.ipv4.nexthop
  2. same goes for ipv6

Information to attach

I already had a look through the code and it seems any configuration key starting with bgp. and length of fields != 4 will not work. I did fix this issue in a dev environment but I 'm not sure if it breaks something for someone else. Hence this report, so someone who knows the codebase better than me can have a look.

The fix for me was this diff:

diff --git a/lxd/network/driver_common.go b/lxd/network/driver_common.go
index 7b0a435c4..0232aeacb 100644
--- a/lxd/network/driver_common.go
+++ b/lxd/network/driver_common.go
@@ -585,8 +585,10 @@ func (n *common) bgpValidationRules(config map[string]string) (map[string]func(v

                // Validate remote name in key.
                fields := strings.Split(k, ".")
-               if len(fields) != 4 {
+               if len(fields) == 4 && fields[1] != "peers" {
                        return nil, fmt.Errorf("Invalid network configuration key: %s", k)
+               } else if len(fields) != 4 {
+                       continue
                }

                bgpKey := fields[3]

If we can confirm this is the right fix, feel free to apply it to master. Or, if I must, I could submit a PR.

Thanks for the great work!

roosterfish commented 1 year ago

Hi @mandrav, looks like that's a bug. If we would just extend the condition line to if !strings.HasPrefix(k, "bgp.peers") { the check will only validate the four bgp.peer config settings. The check for bpg.ipv[46].nexthop is already in lxd/network/driver_bridge.go (*bridge).Validate.

Since those two settings are valid for both bridged and physical nets, I would suspect we should also add the two validation rules to the physical driver. They are missing there currently:

lxd/network/driver_physical.go:

// Validate network config.
func (n *physical) Validate(config map[string]string) error {
    rules := map[string]func(value string) error{
        "parent":                      validate.Required(validate.IsNotEmpty, validate.IsInterfaceName),
        "mtu":                         validate.Optional(validate.IsNetworkMTU),
        "vlan":                        validate.Optional(validate.IsNetworkVLAN),
        "gvrp":                        validate.Optional(validate.IsBool),
...

If you could open up a PR that would be great.

roosterfish commented 1 year ago

Maybe it's even cleaner to move the validation into bgpValidationRules() since this is already called from both drivers.

mandrav commented 1 year ago

Thanks @roosterfish for the comments. So, I will move the bgp.ip*.nexthop stuff in bgpValidationRules, update the checks accordingly and open a PR.

mandrav commented 1 year ago

Opened PR #12125

tomponline commented 1 year ago

https://github.com/canonical/lxd/pull/12125