From 6a445406e0df4e33044cc429fabd92b658d05743 Mon Sep 17 00:00:00 2001 From: Andrey Smirnov Date: Tue, 28 Apr 2026 20:57:02 +0400 Subject: [PATCH] fix: make lacp active nilable This fixes aset of inconsistencies, one of the examples is #13203. Make the field in the spec nilable, and treat `nil` as `on` (this is Linux default if the field is not set explicitly). When using machine config, the setting can be flipped to `off` explicitly (if needed). This brings back compatibility with raw metal network resource specs pre-1.12 which don't contain any explicit `adLacpActive: on` at all. Fixes #13203 Signed-off-by: Andrey Smirnov --- .../pkg/adapters/network/bond_master_spec.go | 11 ++- .../adapters/network/bond_master_spec_test.go | 74 +++++++++++++++---- .../network/bridge_master_spec_test.go | 2 + .../pkg/adapters/network/vlan_spec_test.go | 2 + .../adapters/network/vrf_master_spec_test.go | 2 + .../adapters/network/wireguard_spec_test.go | 6 ++ .../pkg/controllers/network/cmdline_test.go | 2 - .../controllers/network/link_config_test.go | 4 +- .../pkg/controllers/network/network.go | 9 ++- .../v1alpha1/platform/equinixmetal/equinix.go | 2 +- .../v1alpha1/platform/nocloud/metadata.go | 4 +- .../v1alpha1/platform/openstack/openstack.go | 2 +- .../config/schemas/config.schema.json | 6 +- pkg/machinery/config/types/network/bond.go | 2 +- .../config/types/network/network_doc.go | 4 +- .../resources/network/deep_copy.generated.go | 4 + pkg/machinery/resources/network/link.go | 17 ++++- .../configuration/network/bondconfig.md | 4 +- .../content/v1.14/schemas/config.schema.json | 6 +- 19 files changed, 119 insertions(+), 44 deletions(-) diff --git a/internal/app/machined/pkg/adapters/network/bond_master_spec.go b/internal/app/machined/pkg/adapters/network/bond_master_spec.go index 362bbed6a..b309039bb 100644 --- a/internal/app/machined/pkg/adapters/network/bond_master_spec.go +++ b/internal/app/machined/pkg/adapters/network/bond_master_spec.go @@ -65,10 +65,6 @@ func (a bondMaster) FillDefaults() { bond.MissedMax = 2 } } - - if bond.Mode != nethelpers.BondMode8023AD { - bond.ADLACPActive = nethelpers.ADLACPActiveOn - } } // Encode the BondMasterSpec into netlink attributes. @@ -84,7 +80,10 @@ func (a bondMaster) Encode() ([]byte, error) { if bond.Mode == nethelpers.BondMode8023AD { encoder.Uint8(unix.IFLA_BOND_AD_LACP_RATE, uint8(bond.LACPRate)) - encoder.Uint8(unix.IFLA_BOND_AD_LACP_ACTIVE, uint8(bond.ADLACPActive)) + + if bond.ADLACPActive != nil { + encoder.Uint8(unix.IFLA_BOND_AD_LACP_ACTIVE, uint8(*bond.ADLACPActive)) + } } if bond.Mode != nethelpers.BondMode8023AD && bond.Mode != nethelpers.BondModeALB && bond.Mode != nethelpers.BondModeTLB { @@ -270,7 +269,7 @@ func (a bondMaster) Decode(data []byte) error { case unix.IFLA_BOND_PEER_NOTIF_DELAY: bond.PeerNotifyDelay = decoder.Uint32() case unix.IFLA_BOND_AD_LACP_ACTIVE: - bond.ADLACPActive = nethelpers.ADLACPActive(decoder.Uint8()) + bond.ADLACPActive = new(nethelpers.ADLACPActive(decoder.Uint8())) case unix.IFLA_BOND_MISSED_MAX: bond.MissedMax = decoder.Uint8() } diff --git a/internal/app/machined/pkg/adapters/network/bond_master_spec_test.go b/internal/app/machined/pkg/adapters/network/bond_master_spec_test.go index dd88fac83..57ef7a30b 100644 --- a/internal/app/machined/pkg/adapters/network/bond_master_spec_test.go +++ b/internal/app/machined/pkg/adapters/network/bond_master_spec_test.go @@ -16,24 +16,70 @@ import ( ) func TestBondMasterSpec(t *testing.T) { - spec := network.BondMasterSpec{ - Mode: nethelpers.BondModeActiveBackup, - MIIMon: 100, - UpDelay: 200, - DownDelay: 300, + t.Parallel() + + for _, test := range []struct { + name string + spec network.BondMasterSpec + }{ + { + name: "active-backup", + spec: network.BondMasterSpec{ + Mode: nethelpers.BondModeActiveBackup, + MIIMon: 100, + UpDelay: 200, + DownDelay: 300, + }, + }, + { + name: "802.3ad no lacp", + spec: network.BondMasterSpec{ + Mode: nethelpers.BondMode8023AD, + MIIMon: 100, + UpDelay: 200, + DownDelay: 300, + }, + }, + { + name: "802.3ad lacp on", + spec: network.BondMasterSpec{ + Mode: nethelpers.BondMode8023AD, + MIIMon: 100, + UpDelay: 200, + DownDelay: 300, + LACPRate: nethelpers.LACPRateFast, + ADLACPActive: new(nethelpers.ADLACPActiveOn), + }, + }, + { + name: "802.3ad lacp off", + spec: network.BondMasterSpec{ + Mode: nethelpers.BondMode8023AD, + MIIMon: 100, + UpDelay: 200, + DownDelay: 300, + ADLACPActive: new(nethelpers.ADLACPActiveOff), + }, + }, + } { + t.Run(test.name, func(t *testing.T) { + t.Parallel() + + b, err := networkadapter.BondMasterSpec(&test.spec).Encode() + require.NoError(t, err) + + var decodedSpec network.BondMasterSpec + + require.NoError(t, networkadapter.BondMasterSpec(&decodedSpec).Decode(b)) + + require.Equal(t, test.spec, decodedSpec) + }) } - - b, err := networkadapter.BondMasterSpec(&spec).Encode() - require.NoError(t, err) - - var decodedSpec network.BondMasterSpec - - require.NoError(t, networkadapter.BondMasterSpec(&decodedSpec).Decode(b)) - - require.Equal(t, spec, decodedSpec) } func TestBondMasterSpecDecodeClearsTargets(t *testing.T) { + t.Parallel() + initial := network.BondMasterSpec{ Mode: nethelpers.BondModeActiveBackup, ARPIPTargets: []netip.Addr{netip.MustParseAddr("198.51.100.254")}, diff --git a/internal/app/machined/pkg/adapters/network/bridge_master_spec_test.go b/internal/app/machined/pkg/adapters/network/bridge_master_spec_test.go index 35ca5bbaf..b6ced5fed 100644 --- a/internal/app/machined/pkg/adapters/network/bridge_master_spec_test.go +++ b/internal/app/machined/pkg/adapters/network/bridge_master_spec_test.go @@ -14,6 +14,8 @@ import ( ) func TestBridgeMasterSpec(t *testing.T) { + t.Parallel() + spec := network.BridgeMasterSpec{ STP: network.STPSpec{ Enabled: true, diff --git a/internal/app/machined/pkg/adapters/network/vlan_spec_test.go b/internal/app/machined/pkg/adapters/network/vlan_spec_test.go index 1f6715ce4..2c1e85abe 100644 --- a/internal/app/machined/pkg/adapters/network/vlan_spec_test.go +++ b/internal/app/machined/pkg/adapters/network/vlan_spec_test.go @@ -15,6 +15,8 @@ import ( ) func TestVLANSpec(t *testing.T) { + t.Parallel() + spec := network.VLANSpec{ VID: 25, Protocol: nethelpers.VLANProtocol8021AD, diff --git a/internal/app/machined/pkg/adapters/network/vrf_master_spec_test.go b/internal/app/machined/pkg/adapters/network/vrf_master_spec_test.go index e54f4a6ee..718ad2a12 100644 --- a/internal/app/machined/pkg/adapters/network/vrf_master_spec_test.go +++ b/internal/app/machined/pkg/adapters/network/vrf_master_spec_test.go @@ -14,6 +14,8 @@ import ( ) func TestVRFMasterSpec(t *testing.T) { + t.Parallel() + spec := network.VRFMasterSpec{ Table: 4294967295, } diff --git a/internal/app/machined/pkg/adapters/network/wireguard_spec_test.go b/internal/app/machined/pkg/adapters/network/wireguard_spec_test.go index 6fd816af7..289e2de8d 100644 --- a/internal/app/machined/pkg/adapters/network/wireguard_spec_test.go +++ b/internal/app/machined/pkg/adapters/network/wireguard_spec_test.go @@ -20,6 +20,8 @@ import ( ) func TestWireguardSpecDecode(t *testing.T) { + t.Parallel() + fips140.WithoutEnforcement(func() { priv, err := wgtypes.GeneratePrivateKey() require.NoError(t, err) @@ -105,6 +107,8 @@ func TestWireguardSpecDecode(t *testing.T) { } func TestWireguardSpecDecodeStatus(t *testing.T) { + t.Parallel() + fips140.WithoutEnforcement(func() { priv, err := wgtypes.GeneratePrivateKey() require.NoError(t, err) @@ -131,6 +135,8 @@ func TestWireguardSpecDecodeStatus(t *testing.T) { } func TestWireguardSpecEncode(t *testing.T) { + t.Parallel() + fips140.WithoutEnforcement(func() { priv, err := wgtypes.GeneratePrivateKey() require.NoError(t, err) diff --git a/internal/app/machined/pkg/controllers/network/cmdline_test.go b/internal/app/machined/pkg/controllers/network/cmdline_test.go index 05b789ae7..43ec34437 100644 --- a/internal/app/machined/pkg/controllers/network/cmdline_test.go +++ b/internal/app/machined/pkg/controllers/network/cmdline_test.go @@ -58,7 +58,6 @@ func TestCmdlineParse(t *testing.T) { TLBDynamicLB: 1, UseCarrier: true, PrimaryIndex: new(uint32(0)), - ADLACPActive: nethelpers.ADLACPActiveOn, MissedMax: 2, }, }, @@ -408,7 +407,6 @@ func TestCmdlineParse(t *testing.T) { TLBDynamicLB: 1, UseCarrier: true, PrimaryIndex: new(uint32(0)), - ADLACPActive: nethelpers.ADLACPActiveOn, }, }, { diff --git a/internal/app/machined/pkg/controllers/network/link_config_test.go b/internal/app/machined/pkg/controllers/network/link_config_test.go index 9529ede13..a3b6487cd 100644 --- a/internal/app/machined/pkg/controllers/network/link_config_test.go +++ b/internal/app/machined/pkg/controllers/network/link_config_test.go @@ -457,7 +457,7 @@ func (suite *LinkConfigSuite) TestMachineConfigurationWithAliases() { asrt.Equal(network.LinkKindBond, r.TypedSpec().Kind) asrt.Equal(nethelpers.BondModeXOR, r.TypedSpec().BondMaster.Mode) asrt.True(r.TypedSpec().BondMaster.UseCarrier) - asrt.Equal(nethelpers.ADLACPActiveOn, r.TypedSpec().BondMaster.ADLACPActive) + asrt.Nil(r.TypedSpec().BondMaster.ADLACPActive) } }, ) @@ -560,7 +560,7 @@ func (suite *LinkConfigSuite) TestMachineConfigurationNewStyle() { asrt.Equal(network.LinkKindBond, r.TypedSpec().Kind) asrt.Equal(nethelpers.BondModeActiveBackup, r.TypedSpec().BondMaster.Mode) asrt.EqualValues(200, r.TypedSpec().BondMaster.UpDelay) - asrt.Equal(nethelpers.ADLACPActiveOn, r.TypedSpec().BondMaster.ADLACPActive) + asrt.Nil(r.TypedSpec().BondMaster.ADLACPActive) case "br0": asrt.True(r.TypedSpec().Up) asrt.True(r.TypedSpec().Logical) diff --git a/internal/app/machined/pkg/controllers/network/network.go b/internal/app/machined/pkg/controllers/network/network.go index fd4e84ee4..0562e2dff 100644 --- a/internal/app/machined/pkg/controllers/network/network.go +++ b/internal/app/machined/pkg/controllers/network/network.go @@ -44,7 +44,13 @@ func SendBondMaster(link *network.LinkSpecSpec, bond talosconfig.NetworkBondConf link.BondMaster.ADSelect = bond.ADSelect().ValueOrZero() link.BondMaster.ADActorSysPrio = bond.ADActorSysPrio().ValueOrZero() link.BondMaster.ADUserPortKey = bond.ADUserPortKey().ValueOrZero() - link.BondMaster.ADLACPActive = bond.ADLACPActive().ValueOr(nethelpers.ADLACPActiveOn) + + if adLACPActive, ok := bond.ADLACPActive().Get(); ok { + link.BondMaster.ADLACPActive = new(adLACPActive) + } else { + link.BondMaster.ADLACPActive = nil + } + link.BondMaster.PrimaryReselect = bond.PrimaryReselect().ValueOrZero() link.BondMaster.ResendIGMP = bond.ResendIGMP().ValueOrZero() link.BondMaster.MinLinks = bond.MinLinks().ValueOrZero() @@ -144,7 +150,6 @@ func SetBondMasterLegacy(link *network.LinkSpecSpec, bond talosconfig.Bond) erro ADActorSysPrio: bond.ADActorSysPrio(), ADUserPortKey: bond.ADUserPortKey(), PeerNotifyDelay: bond.PeerNotifyDelay(), - ADLACPActive: nethelpers.ADLACPActiveOn, } networkadapter.BondMasterSpec(&link.BondMaster).FillDefaults() diff --git a/internal/app/machined/pkg/runtime/v1alpha1/platform/equinixmetal/equinix.go b/internal/app/machined/pkg/runtime/v1alpha1/platform/equinixmetal/equinix.go index 1194dec6a..1b3c1bec3 100644 --- a/internal/app/machined/pkg/runtime/v1alpha1/platform/equinixmetal/equinix.go +++ b/internal/app/machined/pkg/runtime/v1alpha1/platform/equinixmetal/equinix.go @@ -225,7 +225,7 @@ func (p *EquinixMetal) ParseMetadata(ctx context.Context, equinixMetadata *Metad } if bondMode == nethelpers.BondMode8023AD { - bondLink.BondMaster.ADLACPActive = nethelpers.ADLACPActiveOn + bondLink.BondMaster.ADLACPActive = new(nethelpers.ADLACPActiveOn) } networkadapter.BondMasterSpec(&bondLink.BondMaster).FillDefaults() diff --git a/internal/app/machined/pkg/runtime/v1alpha1/platform/nocloud/metadata.go b/internal/app/machined/pkg/runtime/v1alpha1/platform/nocloud/metadata.go index 641f64508..7a67553a3 100644 --- a/internal/app/machined/pkg/runtime/v1alpha1/platform/nocloud/metadata.go +++ b/internal/app/machined/pkg/runtime/v1alpha1/platform/nocloud/metadata.go @@ -465,7 +465,7 @@ func (n *Nocloud) applyNetworkConfigV1(ctx context.Context, config *NetworkConfi } if mode == nethelpers.BondMode8023AD { - bondLink.BondMaster.ADLACPActive = nethelpers.ADLACPActiveOn + bondLink.BondMaster.ADLACPActive = new(nethelpers.ADLACPActiveOn) } if ntwrk.MTU != 0 { @@ -872,7 +872,7 @@ func (n *Nocloud) applyNetworkConfigV2(ctx context.Context, config *NetworkConfi } if mode == nethelpers.BondMode8023AD { - bondLink.BondMaster.ADLACPActive = nethelpers.ADLACPActiveOn + bondLink.BondMaster.ADLACPActive = new(nethelpers.ADLACPActiveOn) } networkadapter.BondMasterSpec(&bondLink.BondMaster).FillDefaults() diff --git a/internal/app/machined/pkg/runtime/v1alpha1/platform/openstack/openstack.go b/internal/app/machined/pkg/runtime/v1alpha1/platform/openstack/openstack.go index 6d2b4a8fe..4ee0100e3 100644 --- a/internal/app/machined/pkg/runtime/v1alpha1/platform/openstack/openstack.go +++ b/internal/app/machined/pkg/runtime/v1alpha1/platform/openstack/openstack.go @@ -146,7 +146,7 @@ func (o *OpenStack) ParseMetadata( } if mode == nethelpers.BondMode8023AD { - bondLink.BondMaster.ADLACPActive = nethelpers.ADLACPActiveOn + bondLink.BondMaster.ADLACPActive = new(nethelpers.ADLACPActiveOn) } networkadapter.BondMasterSpec(&bondLink.BondMaster).FillDefaults() diff --git a/pkg/machinery/config/schemas/config.schema.json b/pkg/machinery/config/schemas/config.schema.json index 70cb1ba06..9e22defa9 100644 --- a/pkg/machinery/config/schemas/config.schema.json +++ b/pkg/machinery/config/schemas/config.schema.json @@ -1385,9 +1385,9 @@ "off" ], "title": "adLACPActive", - "description": "Whether to send LACPDU frames periodically.\n", - "markdownDescription": "Whether to send LACPDU frames periodically.", - "x-intellij-html-description": "\u003cp\u003eWhether to send LACPDU frames periodically.\u003c/p\u003e\n" + "description": "Whether to send LACPDU frames periodically, defaults to “on” if mode is 802.3ad.\n", + "markdownDescription": "Whether to send LACPDU frames periodically, defaults to \"on\" if mode is 802.3ad.", + "x-intellij-html-description": "\u003cp\u003eWhether to send LACPDU frames periodically, defaults to \u0026ldquo;on\u0026rdquo; if mode is 802.3ad.\u003c/p\u003e\n" }, "primaryReselect": { "enum": [ diff --git a/pkg/machinery/config/types/network/bond.go b/pkg/machinery/config/types/network/bond.go index 8d274e6b0..5853f812b 100644 --- a/pkg/machinery/config/types/network/bond.go +++ b/pkg/machinery/config/types/network/bond.go @@ -228,7 +228,7 @@ type BondConfigV1Alpha1 struct { // 0 BondADUserPortKey *uint16 `yaml:"adUserPortKey,omitempty"` // description: | - // Whether to send LACPDU frames periodically. + // Whether to send LACPDU frames periodically, defaults to "on" if mode is 802.3ad. // examples: // - value: > // "on" diff --git a/pkg/machinery/config/types/network/network_doc.go b/pkg/machinery/config/types/network/network_doc.go index 215054c1c..a6429db81 100644 --- a/pkg/machinery/config/types/network/network_doc.go +++ b/pkg/machinery/config/types/network/network_doc.go @@ -238,8 +238,8 @@ func (BondConfigV1Alpha1) Doc() *encoder.Doc { Name: "adLACPActive", Type: "ADLACPActive", Note: "", - Description: "Whether to send LACPDU frames periodically.", - Comments: [3]string{"" /* encoder.HeadComment */, "Whether to send LACPDU frames periodically." /* encoder.LineComment */, "" /* encoder.FootComment */}, + Description: "Whether to send LACPDU frames periodically, defaults to \"on\" if mode is 802.3ad.", + Comments: [3]string{"" /* encoder.HeadComment */, "Whether to send LACPDU frames periodically, defaults to \"on\" if mode is 802.3ad." /* encoder.LineComment */, "" /* encoder.FootComment */}, Values: []string{ "on", "off", diff --git a/pkg/machinery/resources/network/deep_copy.generated.go b/pkg/machinery/resources/network/deep_copy.generated.go index 1cab6a209..d5deb2af7 100644 --- a/pkg/machinery/resources/network/deep_copy.generated.go +++ b/pkg/machinery/resources/network/deep_copy.generated.go @@ -39,6 +39,10 @@ func (o BondMasterSpec) DeepCopy() BondMasterSpec { cp.NSIP6Targets = make([]netip.Addr, len(o.NSIP6Targets)) copy(cp.NSIP6Targets, o.NSIP6Targets) } + if o.ADLACPActive != nil { + cp.ADLACPActive = new(nethelpers.ADLACPActive) + *cp.ADLACPActive = *o.ADLACPActive + } return cp } diff --git a/pkg/machinery/resources/network/link.go b/pkg/machinery/resources/network/link.go index 0e34c8fb6..e293f7f04 100644 --- a/pkg/machinery/resources/network/link.go +++ b/pkg/machinery/resources/network/link.go @@ -86,7 +86,7 @@ type BondMasterSpec struct { // Maximum of 16 targets are supported. NSIP6Targets []netip.Addr `yaml:"nsIp6Targets,omitempty" protobuf:"26"` // ADLACPActive specifies whether to send LACPDU frames periodically. - ADLACPActive nethelpers.ADLACPActive `yaml:"adLacpActive,omitempty" protobuf:"27"` + ADLACPActive *nethelpers.ADLACPActive `yaml:"adLacpActive,omitempty" protobuf:"27"` // MissedMax is the number of arp_interval monitor checks that must fail in order for an interface to be marked down by the ARP monitor. MissedMax uint8 `yaml:"missedMax,omitempty" protobuf:"28"` } @@ -211,7 +211,18 @@ func (spec *BondMasterSpec) Equal(other *BondMasterSpec) bool { } } - if spec.ADLACPActive != other.ADLACPActive { + // default value for ADLACPActive is "on" in Linux, so if both are nil or "on", consider them equal + specADLACPActive, otherADLACPActive := nethelpers.ADLACPActiveOn, nethelpers.ADLACPActiveOn + + if spec.ADLACPActive != nil { + specADLACPActive = *spec.ADLACPActive + } + + if other.ADLACPActive != nil { + otherADLACPActive = *other.ADLACPActive + } + + if specADLACPActive != otherADLACPActive { return false } @@ -254,7 +265,7 @@ func (spec *BondMasterSpec) IsZero() bool { spec.PeerNotifyDelay == 0 && len(spec.ARPIPTargets) == 0 && len(spec.NSIP6Targets) == 0 && - spec.ADLACPActive == 0 && + spec.ADLACPActive == nil && spec.MissedMax == 0 } diff --git a/website/content/v1.14/reference/configuration/network/bondconfig.md b/website/content/v1.14/reference/configuration/network/bondconfig.md index 9225baa74..f0b276261 100644 --- a/website/content/v1.14/reference/configuration/network/bondconfig.md +++ b/website/content/v1.14/reference/configuration/network/bondconfig.md @@ -64,7 +64,7 @@ routes: # # Aggregate selection policy for 802.3ad. # adSelect: stable -# # Whether to send LACPDU frames periodically. +# # Whether to send LACPDU frames periodically, defaults to "on" if mode is 802.3ad. # adLACPActive: on # # Policy under which the primary slave should be reselected. @@ -136,7 +136,7 @@ adActorSysPrio: 65535 |`adUserPortKey` |uint16 |User port key (upper 10 bits) for 802.3ad.
Show example(s){{< highlight yaml >}} adUserPortKey: 0 {{< /highlight >}}
| | -|`adLACPActive` |ADLACPActive |Whether to send LACPDU frames periodically.
Show example(s){{< highlight yaml >}} +|`adLACPActive` |ADLACPActive |Whether to send LACPDU frames periodically, defaults to "on" if mode is 802.3ad.
Show example(s){{< highlight yaml >}} adLACPActive: on {{< /highlight >}}
|`on`
`off`
| |`primaryReselect` |PrimaryReselect |Policy under which the primary slave should be reselected.
Show example(s){{< highlight yaml >}} diff --git a/website/content/v1.14/schemas/config.schema.json b/website/content/v1.14/schemas/config.schema.json index 70cb1ba06..9e22defa9 100644 --- a/website/content/v1.14/schemas/config.schema.json +++ b/website/content/v1.14/schemas/config.schema.json @@ -1385,9 +1385,9 @@ "off" ], "title": "adLACPActive", - "description": "Whether to send LACPDU frames periodically.\n", - "markdownDescription": "Whether to send LACPDU frames periodically.", - "x-intellij-html-description": "\u003cp\u003eWhether to send LACPDU frames periodically.\u003c/p\u003e\n" + "description": "Whether to send LACPDU frames periodically, defaults to “on” if mode is 802.3ad.\n", + "markdownDescription": "Whether to send LACPDU frames periodically, defaults to \"on\" if mode is 802.3ad.", + "x-intellij-html-description": "\u003cp\u003eWhether to send LACPDU frames periodically, defaults to \u0026ldquo;on\u0026rdquo; if mode is 802.3ad.\u003c/p\u003e\n" }, "primaryReselect": { "enum": [