Skip to content

Commit

Permalink
fix: make route resources ID match closer routing table primary key
Browse files Browse the repository at this point in the history 
New primary key is the following:

* table (if `main`, leave it empty)
* family
* gateway
* destination
* metric (priority)

There might be more ways we can improve this (probably gateway should be
removed?), but this should be much better model to represent routes in
the system.

This was discovered in KubeSpan development (@Ulexus).

Signed-off-by: Andrey Smirnov <andrey.smirnov@talos-systems.com>
  • Loading branch information
@smira @talos-bot
smira authored and talos-bot committed Aug 10, 2021
1 parent 585f633 commit b68ed1e
Show file tree
Hide file tree
Showing 10 changed files with 47 additions and 40 deletions.
1 change: 0 additions & 1 deletion internal/app/machined/pkg/controllers/network/address_config.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,6 @@ func (ctrl *AddressConfigController) Run(ctx context.Context, r controller.Runti
}
}

//nolint:dupl
func (ctrl *AddressConfigController) apply(ctx context.Context, r controller.Runtime, addresses []network.AddressSpecSpec) ([]resource.ID, error) {
ids := make([]string, 0, len(addresses))

Expand Down
2 changes: 0 additions & 2 deletions internal/app/machined/pkg/controllers/network/address_merge.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,6 @@
// 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 (
Expand Down
2 changes: 1 addition & 1 deletion internal/app/machined/pkg/controllers/network/operator_spec.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -260,7 +260,7 @@ func (ctrl *OperatorSpecController) reconcileOperatorOutputs(ctx context.Context
if err := apply(
network.NewRouteSpec(
network.ConfigNamespaceName,
fmt.Sprintf("%s/%s", op.Operator.Prefix(), network.RouteID(routeSpec.Destination, routeSpec.Gateway)),
fmt.Sprintf("%s/%s", op.Operator.Prefix(), network.RouteID(routeSpec.Table, routeSpec.Family, routeSpec.Destination, routeSpec.Gateway, routeSpec.Priority)),
),
func(r resource.Resource) {
*r.(*network.RouteSpec).TypedSpec() = routeSpec
Expand Down
3 changes: 1 addition & 2 deletions internal/app/machined/pkg/controllers/network/route_config.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -142,13 +142,12 @@ func (ctrl *RouteConfigController) Run(ctx context.Context, r controller.Runtime
}
}

//nolint:dupl
func (ctrl *RouteConfigController) apply(ctx context.Context, r controller.Runtime, routes []network.RouteSpecSpec) ([]resource.ID, error) {
ids := make([]string, 0, len(routes))

for _, route := range routes {
route := route
id := network.LayeredID(route.ConfigLayer, network.RouteID(route.Destination, route.Gateway))
id := network.LayeredID(route.ConfigLayer, network.RouteID(route.Table, route.Family, route.Destination, route.Gateway, route.Priority))

if err := r.Modify(
ctx,
Expand Down
14 changes: 7 additions & 7 deletions internal/app/machined/pkg/controllers/network/route_config_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ func (suite *RouteConfigSuite) TestCmdline() {
suite.Assert().NoError(retry.Constant(3*time.Second, retry.WithUnits(100*time.Millisecond)).Retry(
func() error {
return suite.assertRoutes([]string{
"cmdline/172.20.0.1/",
"cmdline/inet4/172.20.0.1//1024",
}, func(r *network.RouteSpec) error {
suite.Assert().Equal("eth1", r.TypedSpec().OutLinkName)
suite.Assert().Equal(network.ConfigCmdline, r.TypedSpec().ConfigLayer)
Expand Down Expand Up @@ -195,20 +195,20 @@ func (suite *RouteConfigSuite) TestMachineConfiguration() {
suite.Assert().NoError(retry.Constant(3*time.Second, retry.WithUnits(100*time.Millisecond)).Retry(
func() error {
return suite.assertRoutes([]string{
"configuration/2001:470:6d:30e:8ed2:b60c:9d2f:803b/",
"configuration/10.0.3.1/10.0.3.0/24",
"configuration/192.168.0.25/192.168.0.0/18",
"configuration/inet6/2001:470:6d:30e:8ed2:b60c:9d2f:803b//1024",
"configuration/inet4/10.0.3.1/10.0.3.0/24/1024",
"configuration/inet4/192.168.0.25/192.168.0.0/18/25",
}, func(r *network.RouteSpec) error {
switch r.Metadata().ID() {
case "configuration/2001:470:6d:30e:8ed2:b60c:9d2f:803b/":
case "configuration/inet6/2001:470:6d:30e:8ed2:b60c:9d2f:803b//1024":
suite.Assert().Equal("eth2", r.TypedSpec().OutLinkName)
suite.Assert().Equal(nethelpers.FamilyInet6, r.TypedSpec().Family)
suite.Assert().EqualValues(netctrl.DefaultRouteMetric, r.TypedSpec().Priority)
case "configuration/10.0.3.1/10.0.3.0/24":
case "configuration/inet4/10.0.3.1/10.0.3.0/24/1024":
suite.Assert().Equal("eth0.24", r.TypedSpec().OutLinkName)
suite.Assert().Equal(nethelpers.FamilyInet4, r.TypedSpec().Family)
suite.Assert().EqualValues(netctrl.DefaultRouteMetric, r.TypedSpec().Priority)
case "configuration/192.168.0.25/192.168.0.0/18":
case "configuration/inet4/192.168.0.25/192.168.0.0/18/25":
suite.Assert().Equal("eth3", r.TypedSpec().OutLinkName)
suite.Assert().Equal(nethelpers.FamilyInet4, r.TypedSpec().Family)
suite.Assert().EqualValues(25, r.TypedSpec().Priority)
Expand Down
4 changes: 1 addition & 3 deletions internal/app/machined/pkg/controllers/network/route_merge.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,6 @@
// 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 (
Expand Down Expand Up @@ -75,7 +73,7 @@ func (ctrl *RouteMergeController) Run(ctx context.Context, r controller.Runtime,

for _, res := range list.Items {
route := res.(*network.RouteSpec) //nolint:errcheck,forcetypeassert
id := network.RouteID(route.TypedSpec().Destination, route.TypedSpec().Gateway)
id := network.RouteID(route.TypedSpec().Table, route.TypedSpec().Family, route.TypedSpec().Destination, route.TypedSpec().Gateway, route.TypedSpec().Priority)

existing, ok := routes[id]
if ok && existing.TypedSpec().ConfigLayer > route.TypedSpec().ConfigLayer {
Expand Down
45 changes: 25 additions & 20 deletions internal/app/machined/pkg/controllers/network/route_merge_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -114,36 +114,39 @@ func (suite *RouteMergeSuite) assertNoRoute(id string) error {
}

func (suite *RouteMergeSuite) TestMerge() {
cmdline := network.NewRouteSpec(network.ConfigNamespaceName, "cmdline//10.5.0.3")
cmdline := network.NewRouteSpec(network.ConfigNamespaceName, "cmdline/inet4//10.5.0.3/50")
*cmdline.TypedSpec() = network.RouteSpecSpec{
Gateway: netaddr.MustParseIP("10.5.0.3"),
OutLinkName: "eth0",
Family: nethelpers.FamilyInet4,
Scope: nethelpers.ScopeGlobal,
Type: nethelpers.TypeUnicast,
Priority: 1024,
Table: nethelpers.TableMain,
Priority: 50,
ConfigLayer: network.ConfigCmdline,
}

dhcp := network.NewRouteSpec(network.ConfigNamespaceName, "dhcp//10.5.0.3")
dhcp := network.NewRouteSpec(network.ConfigNamespaceName, "dhcp/inet4//10.5.0.3/50")
*dhcp.TypedSpec() = network.RouteSpecSpec{
Gateway: netaddr.MustParseIP("10.5.0.3"),
OutLinkName: "eth0",
Family: nethelpers.FamilyInet4,
Scope: nethelpers.ScopeGlobal,
Type: nethelpers.TypeUnicast,
Table: nethelpers.TableMain,
Priority: 50,
ConfigLayer: network.ConfigOperator,
}

static := network.NewRouteSpec(network.ConfigNamespaceName, "configuration/10.0.0.35/32/10.0.0.34")
static := network.NewRouteSpec(network.ConfigNamespaceName, "configuration/inet4/10.0.0.35/32/10.0.0.34/1024")
*static.TypedSpec() = network.RouteSpecSpec{
Destination: netaddr.MustParseIPPrefix("10.0.0.35/32"),
Gateway: netaddr.MustParseIP("10.0.0.34"),
OutLinkName: "eth0",
Family: nethelpers.FamilyInet4,
Scope: nethelpers.ScopeGlobal,
Type: nethelpers.TypeUnicast,
Table: nethelpers.TableMain,
Priority: 1024,
ConfigLayer: network.ConfigMachineConfiguration,
}
Expand All @@ -155,15 +158,15 @@ func (suite *RouteMergeSuite) TestMerge() {
suite.Assert().NoError(retry.Constant(3*time.Second, retry.WithUnits(100*time.Millisecond)).Retry(
func() error {
return suite.assertRoutes([]string{
"10.5.0.3/",
"10.0.0.34/10.0.0.35/32",
"inet4/10.5.0.3//50",
"inet4/10.0.0.34/10.0.0.35/32/1024",
}, func(r *network.RouteSpec) error {
suite.Assert().Equal(resource.PhaseRunning, r.Metadata().Phase())

switch r.Metadata().ID() {
case "10.5.0.3/":
case "inet4/10.5.0.3//50":
suite.Assert().Equal(*dhcp.TypedSpec(), *r.TypedSpec())
case "10.0.0.34/10.0.0.35/32":
case "inet4/10.0.0.34/10.0.0.35/32/1024":
suite.Assert().Equal(*static.TypedSpec(), *r.TypedSpec())
}

Expand All @@ -176,18 +179,18 @@ func (suite *RouteMergeSuite) TestMerge() {
suite.Assert().NoError(retry.Constant(3*time.Second, retry.WithUnits(100*time.Millisecond)).Retry(
func() error {
return suite.assertRoutes([]string{
"10.5.0.3/",
"10.0.0.34/10.0.0.35/32",
"inet4/10.5.0.3//50",
"inet4/10.0.0.34/10.0.0.35/32/1024",
}, func(r *network.RouteSpec) error {
suite.Assert().Equal(resource.PhaseRunning, r.Metadata().Phase())

switch r.Metadata().ID() {
case "10.5.0.3/":
case "inet4/10.5.0.3//50":
if *cmdline.TypedSpec() != *r.TypedSpec() {
// using retry here, as it might not be reconciled immediately
return retry.ExpectedError(fmt.Errorf("not equal yet"))
}
case "10.0.0.34/10.0.0.35/32":
case "inet4/10.0.0.34/10.0.0.35/32/1024":
suite.Assert().Equal(*static.TypedSpec(), *r.TypedSpec())
}

Expand All @@ -199,31 +202,33 @@ func (suite *RouteMergeSuite) TestMerge() {

suite.Assert().NoError(retry.Constant(3*time.Second, retry.WithUnits(100*time.Millisecond)).Retry(
func() error {
return suite.assertNoRoute("10.0.0.34/10.0.0.35/32")
return suite.assertNoRoute("inet4/10.0.0.34/10.0.0.35/32/1024")
}))
}

//nolint:gocyclo
func (suite *RouteMergeSuite) TestMergeFlapping() {
// simulate two conflicting default route definitions which are getting removed/added constantly
cmdline := network.NewRouteSpec(network.ConfigNamespaceName, "cmdline//10.5.0.3")
cmdline := network.NewRouteSpec(network.ConfigNamespaceName, "cmdline/inet4//10.5.0.3/50")
*cmdline.TypedSpec() = network.RouteSpecSpec{
Gateway: netaddr.MustParseIP("10.5.0.3"),
OutLinkName: "eth0",
Family: nethelpers.FamilyInet4,
Scope: nethelpers.ScopeGlobal,
Type: nethelpers.TypeUnicast,
Priority: 1024,
Table: nethelpers.TableMain,
Priority: 50,
ConfigLayer: network.ConfigCmdline,
}

dhcp := network.NewRouteSpec(network.ConfigNamespaceName, "dhcp//10.5.0.3")
dhcp := network.NewRouteSpec(network.ConfigNamespaceName, "dhcp/inet4//10.5.0.3/50")
*dhcp.TypedSpec() = network.RouteSpecSpec{
Gateway: netaddr.MustParseIP("10.5.0.3"),
OutLinkName: "eth0",
OutLinkName: "eth1",
Family: nethelpers.FamilyInet4,
Scope: nethelpers.ScopeGlobal,
Type: nethelpers.TypeUnicast,
Table: nethelpers.TableMain,
Priority: 50,
ConfigLayer: network.ConfigOperator,
}
Expand Down Expand Up @@ -255,7 +260,7 @@ func (suite *RouteMergeSuite) TestMergeFlapping() {
eg.Go(func() error {
// add/remove finalizer to the merged resource
for i := 0; i < 1000; i++ {
if err := suite.state.AddFinalizer(suite.ctx, resource.NewMetadata(network.NamespaceName, network.RouteSpecType, "10.5.0.3/", resource.VersionUndefined), "foo"); err != nil {
if err := suite.state.AddFinalizer(suite.ctx, resource.NewMetadata(network.NamespaceName, network.RouteSpecType, "inet4/10.5.0.3//50", resource.VersionUndefined), "foo"); err != nil {
if !state.IsNotFoundError(err) {
return err
}
Expand All @@ -267,7 +272,7 @@ func (suite *RouteMergeSuite) TestMergeFlapping() {

time.Sleep(10 * time.Millisecond)

if err := suite.state.RemoveFinalizer(suite.ctx, resource.NewMetadata(network.NamespaceName, network.RouteSpecType, "10.5.0.3/", resource.VersionUndefined), "foo"); err != nil {
if err := suite.state.RemoveFinalizer(suite.ctx, resource.NewMetadata(network.NamespaceName, network.RouteSpecType, "inet4/10.5.0.3//50", resource.VersionUndefined), "foo"); err != nil {
if err != nil && !state.IsNotFoundError(err) {
return err
}
Expand All @@ -282,7 +287,7 @@ func (suite *RouteMergeSuite) TestMergeFlapping() {
suite.Assert().NoError(retry.Constant(15*time.Second, retry.WithUnits(100*time.Millisecond)).Retry(
func() error {
return suite.assertRoutes([]string{
"10.5.0.3/",
"inet4/10.5.0.3//50",
}, func(r *network.RouteSpec) error {
if r.Metadata().Phase() != resource.PhaseRunning {
return retry.ExpectedErrorf("resource phase is %s", r.Metadata().Phase())
Expand Down
2 changes: 1 addition & 1 deletion internal/app/machined/pkg/controllers/network/route_status.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ func (ctrl *RouteStatusController) Run(ctx context.Context, r controller.Runtime
srcAddr, _ := netaddr.FromStdIPRaw(route.Attributes.Src)
srcPrefix := netaddr.IPPrefixFrom(srcAddr, route.SrcLength)
gatewayAddr, _ := netaddr.FromStdIPRaw(route.Attributes.Gateway)
id := network.RouteID(dstPrefix, gatewayAddr)
id := network.RouteID(nethelpers.RoutingTable(route.Table), nethelpers.Family(route.Family), dstPrefix, gatewayAddr, route.Attributes.Priority)

if err = r.Modify(ctx, network.NewRouteStatus(network.NamespaceName, id), func(r resource.Resource) error {
status := r.(*network.RouteStatus).TypedSpec()
Expand Down
2 changes: 1 addition & 1 deletion internal/app/machined/pkg/controllers/network/route_status_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ func (suite *RouteStatusSuite) assertRoutes(requiredIDs []string, check func(*ne
func (suite *RouteStatusSuite) TestRoutes() {
suite.Assert().NoError(retry.Constant(3*time.Second, retry.WithUnits(100*time.Millisecond)).Retry(
func() error {
return suite.assertRoutes([]string{"/127.0.0.0/8"}, func(r *network.RouteStatus) error {
return suite.assertRoutes([]string{"local/inet4//127.0.0.0/8/0"}, func(r *network.RouteStatus) error {
suite.Assert().True(r.TypedSpec().Source.IP().IsLoopback())
suite.Assert().Equal("lo", r.TypedSpec().OutLinkName)
suite.Assert().Equal(nethelpers.TableLocal, r.TypedSpec().Table)
Expand Down
12 changes: 10 additions & 2 deletions pkg/resources/network/network.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ import (

"github.com/cosi-project/runtime/pkg/resource"
"inet.af/netaddr"

"github.com/talos-systems/talos/pkg/machinery/nethelpers"
)

// NamespaceName contains resources related to networking.
Expand All @@ -31,11 +33,17 @@ func LinkID(linkName string) string {
}

// RouteID builds ID (primary key) for the route.
func RouteID(destination netaddr.IPPrefix, gateway netaddr.IP) string {
func RouteID(table nethelpers.RoutingTable, family nethelpers.Family, destination netaddr.IPPrefix, gateway netaddr.IP, priority uint32) string {
dst, _ := destination.MarshalText() //nolint:errcheck
gw, _ := gateway.MarshalText() //nolint:errcheck

return fmt.Sprintf("%s/%s", string(gw), string(dst))
tablePrefix := ""

if table != nethelpers.TableMain {
tablePrefix = fmt.Sprintf("%s/", table)
}

return fmt.Sprintf("%s%s/%s/%s/%d", tablePrefix, family, string(gw), string(dst), priority)
}

// OperatorID builds ID (primary key) for the operators.
Expand Down

0 comments on commit b68ed1e

Please sign in to comment.