From b609f33cdebb0659738d4fa3802035b2b344b9b9 Mon Sep 17 00:00:00 2001 From: Andrey Smirnov Date: Wed, 16 Jun 2021 21:26:41 +0300 Subject: [PATCH] fix: update networking stack after Equnix Metal testing This PR contains multiple fixes to the networking controllers and logging improvements for easier debugging: * `LinkConfigController` now correctly merges duplicate link definitions in the machine configuration * `LinkConfigController` correctly enslaves bond interfaces even if they're not mentioned explicitly in the config * bond slaves are no longer brought down forcefully, but they're brought down before being enslaved (and brought up once they're enslaved) * route sync code ignores flags which are not managed by Talos Signed-off-by: Andrey Smirnov --- .../pkg/controllers/network/link_config.go | 56 ++++++++++++++----- .../controllers/network/link_config_test.go | 28 ++++++++-- .../pkg/controllers/network/link_merge.go | 2 + .../pkg/controllers/network/link_spec.go | 3 +- .../pkg/controllers/network/link_spec_test.go | 8 +-- .../controllers/network/operator_config.go | 6 ++ .../pkg/controllers/network/resolver_merge.go | 2 + .../pkg/controllers/network/resolver_spec.go | 8 +++ .../pkg/controllers/network/route_spec.go | 53 +++++++++++++++--- .../controllers/network/timeserver_merge.go | 2 + .../nethelpers/routeflag_string_linux.go | 16 ++++++ pkg/machinery/nethelpers/routeflags_linux.go | 22 ++++++-- pkg/resources/network/link_spec.go | 10 ++++ 13 files changed, 177 insertions(+), 39 deletions(-) diff --git a/internal/app/machined/pkg/controllers/network/link_config.go b/internal/app/machined/pkg/controllers/network/link_config.go index 37b9fdc1c..a28b336e6 100644 --- a/internal/app/machined/pkg/controllers/network/link_config.go +++ b/internal/app/machined/pkg/controllers/network/link_config.go @@ -162,6 +162,12 @@ func (ctrl *LinkConfigController) Run(ctx context.Context, r controller.Runtime, if cfgProvider != nil { for _, device := range cfgProvider.Machine().Network().Devices() { configuredLinks[device.Interface()] = struct{}{} + + if device.Bond() != nil { + for _, link := range device.Bond().Interfaces() { + configuredLinks[link] = struct{}{} + } + } } } @@ -262,7 +268,7 @@ func (ctrl *LinkConfigController) parseCmdline(logger *zap.Logger) (network.Link } //nolint:gocyclo -func (ctrl *LinkConfigController) parseMachineConfiguration(logger *zap.Logger, cfgProvider talosconfig.Provider) (links []network.LinkSpecSpec) { +func (ctrl *LinkConfigController) parseMachineConfiguration(logger *zap.Logger, cfgProvider talosconfig.Provider) []network.LinkSpecSpec { // scan for the bonds bondedLinks := map[string]string{} // mapping physical interface -> bond interface @@ -276,7 +282,7 @@ func (ctrl *LinkConfigController) parseMachineConfiguration(logger *zap.Logger, } for _, linkName := range device.Bond().Interfaces() { - if _, exists := bondedLinks[linkName]; exists { + if bondName, exists := bondedLinks[linkName]; exists && bondName != device.Interface() { logger.Sugar().Warnf("link %q is included into more than two bonds", linkName) } @@ -284,50 +290,70 @@ func (ctrl *LinkConfigController) parseMachineConfiguration(logger *zap.Logger, } } + linkMap := map[string]*network.LinkSpecSpec{} + for _, device := range cfgProvider.Machine().Network().Devices() { if device.Ignore() { continue } - link := network.LinkSpecSpec{ - Name: device.Interface(), - MTU: uint32(device.MTU()), - Up: true, - ConfigLayer: network.ConfigMachineConfiguration, + if _, exists := linkMap[device.Interface()]; !exists { + linkMap[device.Interface()] = &network.LinkSpecSpec{ + Name: device.Interface(), + Up: true, + ConfigLayer: network.ConfigMachineConfiguration, + } } - if bondName := bondedLinks[device.Interface()]; bondName != "" { - bondSlave(&link, bondName) + if device.MTU() != 0 { + linkMap[device.Interface()].MTU = uint32(device.MTU()) } if device.Bond() != nil { - if err := bondMaster(&link, device.Bond()); err != nil { + if err := bondMaster(linkMap[device.Interface()], device.Bond()); err != nil { logger.Error("error parsing bond config", zap.Error(err)) } } if device.WireguardConfig() != nil { - if err := wireguardLink(&link, device.WireguardConfig()); err != nil { + if err := wireguardLink(linkMap[device.Interface()], device.WireguardConfig()); err != nil { logger.Error("error parsing wireguard config", zap.Error(err)) } } if device.Dummy() { - dummyLink(&link) + dummyLink(linkMap[device.Interface()]) } for _, vlan := range device.Vlans() { - links = append(links, vlanLink(device.Interface(), vlan)) + vlanSpec := vlanLink(device.Interface(), vlan) + + linkMap[vlanSpec.Name] = &vlanSpec + } + } + + for slaveName, bondName := range bondedLinks { + if _, exists := linkMap[slaveName]; !exists { + linkMap[slaveName] = &network.LinkSpecSpec{ + Name: slaveName, + Up: true, + ConfigLayer: network.ConfigMachineConfiguration, + } } - links = append(links, link) + bondSlave(linkMap[slaveName], bondName) + } + + links := make([]network.LinkSpecSpec, 0, len(linkMap)) + + for _, link := range linkMap { + links = append(links, *link) } return links } func bondSlave(link *network.LinkSpecSpec, bondName string) { - link.Up = false link.MasterName = bondName } 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 b635442ab..8e238997c 100644 --- a/internal/app/machined/pkg/controllers/network/link_config_test.go +++ b/internal/app/machined/pkg/controllers/network/link_config_test.go @@ -191,6 +191,10 @@ func (suite *LinkConfigSuite) TestMachineConfiguration() { DeviceInterface: "eth1", DeviceCIDR: "192.168.0.24/28", }, + { + DeviceInterface: "eth1", + DeviceMTU: 9001, + }, { DeviceIgnore: true, DeviceInterface: "eth2", @@ -199,9 +203,6 @@ func (suite *LinkConfigSuite) TestMachineConfiguration() { { DeviceInterface: "eth2", }, - { - DeviceInterface: "eth3", - }, { DeviceInterface: "bond0", DeviceBond: &v1alpha1.Bond{ @@ -262,6 +263,12 @@ func (suite *LinkConfigSuite) TestMachineConfiguration() { case "eth0", "eth1": suite.Assert().True(r.TypedSpec().Up) suite.Assert().False(r.TypedSpec().Logical) + + if r.TypedSpec().Name == "eth0" { + suite.Assert().EqualValues(0, r.TypedSpec().MTU) + } else { + suite.Assert().EqualValues(9001, r.TypedSpec().MTU) + } case "eth0.24", "eth0.48": suite.Assert().True(r.TypedSpec().Up) suite.Assert().True(r.TypedSpec().Logical) @@ -276,7 +283,7 @@ func (suite *LinkConfigSuite) TestMachineConfiguration() { suite.Assert().EqualValues(48, r.TypedSpec().VLAN.VID) } case "eth2", "eth3": - suite.Assert().False(r.TypedSpec().Up) + suite.Assert().True(r.TypedSpec().Up) suite.Assert().False(r.TypedSpec().Logical) suite.Assert().Equal("bond0", r.TypedSpec().MasterName) case "bond0": @@ -316,7 +323,7 @@ func (suite *LinkConfigSuite) TestDefaultUp() { Cmdline: procfs.NewCmdline("talos.network.interface.ignore=eth2"), })) - for _, link := range []string{"eth0", "eth1", "eth2"} { + for _, link := range []string{"eth0", "eth1", "eth2", "eth3", "eth4"} { linkStatus := network.NewLinkStatus(network.NamespaceName, link) linkStatus.TypedSpec().Type = nethelpers.LinkEther linkStatus.TypedSpec().LinkState = true @@ -345,6 +352,15 @@ func (suite *LinkConfigSuite) TestDefaultUp() { }, }, }, + { + DeviceInterface: "bond0", + DeviceBond: &v1alpha1.Bond{ + BondInterfaces: []string{ + "eth3", + "eth4", + }, + }, + }, }, }, }, @@ -378,6 +394,8 @@ func (suite *LinkConfigSuite) TestDefaultUp() { return suite.assertNoLinks([]string{ "default/eth0", "default/eth2", + "default/eth3", + "default/eth4", }) })) } diff --git a/internal/app/machined/pkg/controllers/network/link_merge.go b/internal/app/machined/pkg/controllers/network/link_merge.go index dfb820249..4d1deaf4f 100644 --- a/internal/app/machined/pkg/controllers/network/link_merge.go +++ b/internal/app/machined/pkg/controllers/network/link_merge.go @@ -111,6 +111,8 @@ func (ctrl *LinkMergeController) Run(ctx context.Context, r controller.Runtime, }); err != nil { return fmt.Errorf("error updating resource: %w", err) } + + logger.Debug("merged link spec", zap.String("id", id), zap.Any("spec", link)) } // list link for cleanup diff --git a/internal/app/machined/pkg/controllers/network/link_spec.go b/internal/app/machined/pkg/controllers/network/link_spec.go index 4df462b46..6b5a85efb 100644 --- a/internal/app/machined/pkg/controllers/network/link_spec.go +++ b/internal/app/machined/pkg/controllers/network/link_spec.go @@ -403,7 +403,7 @@ func (ctrl *LinkSpecController) syncLink(ctx context.Context, r controller.Runti } // sync UP flag - existingUp := existing.Attributes.OperationalState == rtnetlink.OperStateUnknown || existing.Attributes.OperationalState == rtnetlink.OperStateUp + existingUp := existing.Flags&unix.IFF_UP == unix.IFF_UP if existingUp != link.TypedSpec().Up { flags := uint32(0) @@ -456,6 +456,7 @@ func (ctrl *LinkSpecController) syncLink(ctx context.Context, r controller.Runti Family: existing.Family, Type: existing.Type, Index: existing.Index, + Change: unix.IFF_UP, Attributes: &rtnetlink.LinkAttributes{ Master: pointer.ToUint32(masterIndex), }, diff --git a/internal/app/machined/pkg/controllers/network/link_spec_test.go b/internal/app/machined/pkg/controllers/network/link_spec_test.go index edd194009..f09792959 100644 --- a/internal/app/machined/pkg/controllers/network/link_spec_test.go +++ b/internal/app/machined/pkg/controllers/network/link_spec_test.go @@ -339,7 +339,7 @@ func (suite *LinkSpecSuite) TestBond() { Name: dummy0Name, Type: nethelpers.LinkEther, Kind: "dummy", - Up: false, + Up: true, Logical: true, MasterName: bondName, ConfigLayer: network.ConfigDefault, @@ -351,7 +351,7 @@ func (suite *LinkSpecSuite) TestBond() { Name: dummy1Name, Type: nethelpers.LinkEther, Kind: "dummy", - Up: false, + Up: true, Logical: true, MasterName: bondName, ConfigLayer: network.ConfigDefault, @@ -374,8 +374,8 @@ func (suite *LinkSpecSuite) TestBond() { case dummy0Name, dummy1Name: suite.Assert().Equal("dummy", r.TypedSpec().Kind) - if r.TypedSpec().OperationalState != nethelpers.OperStateDown { - return retry.ExpectedErrorf("link is not down: %s", r.TypedSpec().OperationalState) + if r.TypedSpec().OperationalState != nethelpers.OperStateUnknown { + return retry.ExpectedErrorf("link is not up: %s", r.TypedSpec().OperationalState) } if r.TypedSpec().MasterIndex == 0 { diff --git a/internal/app/machined/pkg/controllers/network/operator_config.go b/internal/app/machined/pkg/controllers/network/operator_config.go index 3801b3ce0..a69f9c793 100644 --- a/internal/app/machined/pkg/controllers/network/operator_config.go +++ b/internal/app/machined/pkg/controllers/network/operator_config.go @@ -117,6 +117,12 @@ func (ctrl *OperatorConfigController) Run(ctx context.Context, r controller.Runt continue } + if device.Bond() != nil { + for _, link := range device.Bond().Interfaces() { + configuredInterfaces[link] = struct{}{} + } + } + if device.DHCP() && device.DHCPOptions().IPv4() { routeMetric := device.DHCPOptions().RouteMetric() if routeMetric == 0 { diff --git a/internal/app/machined/pkg/controllers/network/resolver_merge.go b/internal/app/machined/pkg/controllers/network/resolver_merge.go index 09e6a50b3..109c8161e 100644 --- a/internal/app/machined/pkg/controllers/network/resolver_merge.go +++ b/internal/app/machined/pkg/controllers/network/resolver_merge.go @@ -3,6 +3,8 @@ // file, You can obtain one at http://mozilla.org/MPL/2.0/. // Package network provides controllers which manage network resources. +// +//nolint:dupl package network import ( diff --git a/internal/app/machined/pkg/controllers/network/resolver_spec.go b/internal/app/machined/pkg/controllers/network/resolver_spec.go index c7183b164..2aeafae89 100644 --- a/internal/app/machined/pkg/controllers/network/resolver_spec.go +++ b/internal/app/machined/pkg/controllers/network/resolver_spec.go @@ -87,6 +87,14 @@ func (ctrl *ResolverSpecController) Run(ctx context.Context, r controller.Runtim return fmt.Errorf("error removing finalizer: %w", err) } case resource.PhaseRunning: + resolvers := make([]string, len(spec.TypedSpec().DNSServers)) + + for i := range resolvers { + resolvers[i] = spec.TypedSpec().DNSServers[i].String() + } + + logger.Info("setting resolvers", zap.Strings("resolvers", resolvers)) + if err = r.Modify(ctx, network.NewResolverStatus(network.NamespaceName, spec.Metadata().ID()), func(r resource.Resource) error { status := r.(*network.ResolverStatus) //nolint:forcetypeassert,errcheck diff --git a/internal/app/machined/pkg/controllers/network/route_spec.go b/internal/app/machined/pkg/controllers/network/route_spec.go index d7c02b858..ec987359f 100644 --- a/internal/app/machined/pkg/controllers/network/route_spec.go +++ b/internal/app/machined/pkg/controllers/network/route_spec.go @@ -149,11 +149,20 @@ func (ctrl *RouteSpecController) syncRoute(ctx context.Context, r controller.Run linkIndex := resolveLinkName(links, route.TypedSpec().OutLinkName) destinationStr := route.TypedSpec().Destination.String() - if route.TypedSpec().Destination.IsZero() { destinationStr = "default" } + sourceStr := route.TypedSpec().Source.String() + if route.TypedSpec().Source.IsZero() { + sourceStr = "" + } + + gatewayStr := route.TypedSpec().Gateway.String() + if route.TypedSpec().Gateway.IsZero() { + gatewayStr = "" + } + switch route.Metadata().Phase() { case resource.PhaseTearingDown: for _, existing := range findRoutes(routes, route.TypedSpec().Destination, route.TypedSpec().Gateway, route.TypedSpec().Table) { @@ -162,7 +171,12 @@ func (ctrl *RouteSpecController) syncRoute(ctx context.Context, r controller.Run return fmt.Errorf("error removing route: %w", err) } - logger.Sugar().Infof("removed route to %s via %s (link %q)", destinationStr, route.TypedSpec().Gateway, route.TypedSpec().OutLinkName) + logger.Info("deleted route", + zap.String("destination", destinationStr), + zap.String("gateway", gatewayStr), + zap.Stringer("table", route.TypedSpec().Table), + zap.String("link", route.TypedSpec().OutLinkName), + ) } // now remove finalizer as address was deleted @@ -178,11 +192,10 @@ func (ctrl *RouteSpecController) syncRoute(ctx context.Context, r controller.Run matchFound := false for _, existing := range findRoutes(routes, route.TypedSpec().Destination, route.TypedSpec().Gateway, route.TypedSpec().Table) { - // check if existing matches the spec: if it does, skip update - if existing.Scope == uint8(route.TypedSpec().Scope) && existing.Flags == uint32(route.TypedSpec().Flags) && - existing.Protocol == uint8(route.TypedSpec().Protocol) && existing.Flags == uint32(route.TypedSpec().Flags) && + // check if existing route matches the spec: if it does, skip update + if existing.Scope == uint8(route.TypedSpec().Scope) && nethelpers.RouteFlags(existing.Flags).Equal(route.TypedSpec().Flags) && + existing.Protocol == uint8(route.TypedSpec().Protocol) && existing.Attributes.OutIface == linkIndex && existing.Attributes.Priority == route.TypedSpec().Priority && - existing.Attributes.Table == uint32(route.TypedSpec().Table) && (route.TypedSpec().Source.IsZero() || existing.Attributes.Src.Equal(route.TypedSpec().Source.IP.IPAddr().IP)) { matchFound = true @@ -190,12 +203,29 @@ func (ctrl *RouteSpecController) syncRoute(ctx context.Context, r controller.Run continue } - // delete route, it doesn't match the spec + // delete the route, it doesn't match the spec if err := conn.Route.Delete(existing); err != nil { return fmt.Errorf("error removing route: %w", err) } - logger.Sugar().Infof("removed route to %s via %s (link %q)", destinationStr, route.TypedSpec().Gateway, route.TypedSpec().OutLinkName) + logger.Debug("removed route due to mismatch", + zap.String("destination", destinationStr), + zap.String("gateway", gatewayStr), + zap.Stringer("table", route.TypedSpec().Table), + zap.String("link", route.TypedSpec().OutLinkName), + zap.Stringer("old_scope", nethelpers.Scope(existing.Scope)), + zap.Stringer("new_scope", route.TypedSpec().Scope), + zap.Stringer("old_flags", nethelpers.RouteFlags(existing.Flags)), + zap.Stringer("new_flags", route.TypedSpec().Flags), + zap.Stringer("old_protocol", nethelpers.RouteProtocol(existing.Protocol)), + zap.Stringer("new_protocol", route.TypedSpec().Protocol), + zap.Uint32("old_link_index", existing.Attributes.OutIface), + zap.Uint32("new_link_index", linkIndex), + zap.Uint32("old_priority", existing.Attributes.Priority), + zap.Uint32("new_priority", route.TypedSpec().Priority), + zap.Stringer("old_source", existing.Attributes.Src), + zap.String("new_source", sourceStr), + ) } if matchFound { @@ -225,7 +255,12 @@ func (ctrl *RouteSpecController) syncRoute(ctx context.Context, r controller.Run return fmt.Errorf("error adding route: %w, message %+v", err, *msg) } - logger.Sugar().Infof("created route to %s via %s (link %q)", destinationStr, route.TypedSpec().Gateway, route.TypedSpec().OutLinkName) + logger.Info("created route", + zap.String("destination", destinationStr), + zap.String("gateway", gatewayStr), + zap.Stringer("table", route.TypedSpec().Table), + zap.String("link", route.TypedSpec().OutLinkName), + ) } return nil diff --git a/internal/app/machined/pkg/controllers/network/timeserver_merge.go b/internal/app/machined/pkg/controllers/network/timeserver_merge.go index 065ed1172..5d33ad2d5 100644 --- a/internal/app/machined/pkg/controllers/network/timeserver_merge.go +++ b/internal/app/machined/pkg/controllers/network/timeserver_merge.go @@ -3,6 +3,8 @@ // file, You can obtain one at http://mozilla.org/MPL/2.0/. // Package network provides controllers which manage network resources. +// +//nolint:dupl package network import ( diff --git a/pkg/machinery/nethelpers/routeflag_string_linux.go b/pkg/machinery/nethelpers/routeflag_string_linux.go index 8636688dd..3d4c67938 100644 --- a/pkg/machinery/nethelpers/routeflag_string_linux.go +++ b/pkg/machinery/nethelpers/routeflag_string_linux.go @@ -12,6 +12,10 @@ func _() { _ = x[RouteCloned-512] _ = x[RouteEqualize-1024] _ = x[RoutePrefix-2048] + _ = x[RouteLookupTable-4096] + _ = x[RouteFIBMatch-8192] + _ = x[RouteOffload-16384] + _ = x[RouteTrap-32768] } const ( @@ -19,6 +23,10 @@ const ( _RouteFlag_name_1 = "cloned" _RouteFlag_name_2 = "equalize" _RouteFlag_name_3 = "prefix" + _RouteFlag_name_4 = "lookup_table" + _RouteFlag_name_5 = "fib_match" + _RouteFlag_name_6 = "offload" + _RouteFlag_name_7 = "trap" ) func (i RouteFlag) String() string { @@ -31,6 +39,14 @@ func (i RouteFlag) String() string { return _RouteFlag_name_2 case i == 2048: return _RouteFlag_name_3 + case i == 4096: + return _RouteFlag_name_4 + case i == 8192: + return _RouteFlag_name_5 + case i == 16384: + return _RouteFlag_name_6 + case i == 32768: + return _RouteFlag_name_7 default: return "RouteFlag(" + strconv.FormatInt(int64(i), 10) + ")" } diff --git a/pkg/machinery/nethelpers/routeflags_linux.go b/pkg/machinery/nethelpers/routeflags_linux.go index e8f334078..93d70304e 100644 --- a/pkg/machinery/nethelpers/routeflags_linux.go +++ b/pkg/machinery/nethelpers/routeflags_linux.go @@ -18,7 +18,7 @@ type RouteFlags uint32 func (flags RouteFlags) String() string { var values []string - for flag := RouteNotify; flag <= RoutePrefix; flag <<= 1 { + for flag := RouteNotify; flag <= RouteTrap; flag <<= 1 { if (RouteFlag(flags) & flag) == flag { values = append(values, flag.String()) } @@ -27,6 +27,11 @@ func (flags RouteFlags) String() string { return strings.Join(values, ",") } +// Equal tests for RouteFlags equality ignoring flags not managed by this implementation. +func (flags RouteFlags) Equal(other RouteFlags) bool { + return (flags & RouteFlags(RouteFlagsMask)) == (other & RouteFlags(RouteFlagsMask)) +} + // MarshalYAML implements yaml.Marshaler. func (flags RouteFlags) MarshalYAML() (interface{}, error) { return flags.String(), nil @@ -37,8 +42,15 @@ type RouteFlag uint32 // RouteFlag constants. const ( - RouteNotify RouteFlag = unix.RTM_F_NOTIFY // notify - RouteCloned RouteFlag = unix.RTM_F_CLONED // cloned - RouteEqualize RouteFlag = unix.RTM_F_EQUALIZE // equalize - RoutePrefix RouteFlag = unix.RTM_F_PREFIX // prefix + RouteNotify RouteFlag = unix.RTM_F_NOTIFY // notify + RouteCloned RouteFlag = unix.RTM_F_CLONED // cloned + RouteEqualize RouteFlag = unix.RTM_F_EQUALIZE // equalize + RoutePrefix RouteFlag = unix.RTM_F_PREFIX // prefix + RouteLookupTable RouteFlag = unix.RTM_F_LOOKUP_TABLE // lookup_table + RouteFIBMatch RouteFlag = unix.RTM_F_FIB_MATCH // fib_match + RouteOffload RouteFlag = unix.RTM_F_OFFLOAD // offload + RouteTrap RouteFlag = unix.RTM_F_TRAP // trap ) + +// RouteFlagsMask is a supported set of flags to manage. +const RouteFlagsMask = RouteNotify | RouteCloned | RouteEqualize | RoutePrefix diff --git a/pkg/resources/network/link_spec.go b/pkg/resources/network/link_spec.go index 6a9eb20e9..331fb13a8 100644 --- a/pkg/resources/network/link_spec.go +++ b/pkg/resources/network/link_spec.go @@ -63,6 +63,8 @@ var ( ) // Merge with other, overwriting fields from other if set. +// +//nolint:gocyclo func (spec *LinkSpecSpec) Merge(other *LinkSpecSpec) error { if spec.Logical != other.Logical { return fmt.Errorf("mismatch on logical for %q: %v != %v", spec.Name, spec.Logical, other.Logical) @@ -84,6 +86,14 @@ func (spec *LinkSpecSpec) Merge(other *LinkSpecSpec) error { spec.Type = 0 } + if other.ParentName != "" { + spec.ParentName = other.ParentName + } + + if other.MasterName != "" { + spec.MasterName = other.MasterName + } + if other.VLAN != zeroVLAN { spec.VLAN = other.VLAN }