Fix a part of the issues found during code review

cmd/skywire-cli/commands/node/routes.go

@@ -47,7 +47,7 @@ var ruleCmd = &cobra.Command{
 		rule, err := rpcClient().RoutingRule(routing.RouteID(id))
 		internal.Catch(err)
 
-		printRoutingRules(routing.RuleEntry{RouteID: rule.KeyRouteID(), Rule: rule})
+		printRoutingRules(rule)
 	},
 }
 
@@ -107,13 +107,17 @@ var addRuleCmd = &cobra.Command{
 			)
 			rule = routing.IntermediaryForwardRule(keepAlive, 0, nextRouteID, nextTpID)
 		}
-		rIDKey, err := rpcClient().AddRoutingRule(rule)
-		internal.Catch(err)
+		var rIDKey routing.RouteID
+		if rule != nil {
+			rIDKey = rule.KeyRouteID()
+		}
+
+		internal.Catch(rpcClient().SaveRoutingRule(rule))
 		fmt.Println("Routing Rule Key:", rIDKey)
 	},
 }
 
-func printRoutingRules(entries ...routing.RuleEntry) {
+func printRoutingRules(rules ...routing.Rule) {
 	printConsumeRule := func(w io.Writer, id routing.RouteID, s *routing.RuleSummary) {
 		_, err := fmt.Fprintf(w, "%d\t%s\t%d\t%d\t%s\t%s\t%s\t%s\t%s\n", id, s.Type,
 			s.ConsumeFields.RouteDescriptor.SrcPort, s.ConsumeFields.RouteDescriptor.DstPort,
@@ -128,11 +132,11 @@ func printRoutingRules(entries ...routing.RuleEntry) {
 	w := tabwriter.NewWriter(os.Stdout, 0, 0, 5, ' ', tabwriter.TabIndent)
 	_, err := fmt.Fprintln(w, "id\ttype\tlocal-port\tremote-port\tremote-pk\tresp-id\tnext-route-id\tnext-transport-id\texpire-at")
 	internal.Catch(err)
-	for _, entry := range entries {
-		if entry.Rule.Summary().ConsumeFields != nil {
-			printConsumeRule(w, entry.RouteID, entry.Rule.Summary())
+	for _, rule := range rules {
+		if rule.Summary().ConsumeFields != nil {
+			printConsumeRule(w, rule.KeyRouteID(), rule.Summary())
 		} else {
-			printFwdRule(w, entry.RouteID, entry.Rule.Summary())
+			printFwdRule(w, rule.NextRouteID(), rule.Summary())
 		}
 	}
 	internal.Catch(w.Flush())

pkg/hypervisor/hypervisor.go

@@ -459,14 +459,14 @@ func (m *Node) getRoutes() http.HandlerFunc {
 			httputil.WriteJSON(w, r, http.StatusBadRequest, err)
 			return
 		}
-		entries, err := ctx.RPC.RoutingRules()
+		rules, err := ctx.RPC.RoutingRules()
 		if err != nil {
 			httputil.WriteJSON(w, r, http.StatusInternalServerError, err)
 			return
 		}
-		resp := make([]routingRuleResp, len(entries))
-		for i, entry := range entries {
-			resp[i] = makeRoutingRuleResp(entry.RouteID, entry.Rule, qSummary)
+		resp := make([]routingRuleResp, len(rules))
+		for i, rule := range rules {
+			resp[i] = makeRoutingRuleResp(rule.KeyRouteID(), rule, qSummary)
 		}
 		httputil.WriteJSON(w, r, http.StatusOK, resp)
 	})
@@ -484,12 +484,12 @@ func (m *Node) postRoute() http.HandlerFunc {
 			httputil.WriteJSON(w, r, http.StatusBadRequest, err)
 			return
 		}
-		tid, err := ctx.RPC.AddRoutingRule(rule)
-		if err != nil {
+
+		if err := ctx.RPC.SaveRoutingRule(rule); err != nil {
 			httputil.WriteJSON(w, r, http.StatusInternalServerError, err)
 			return
 		}
-		httputil.WriteJSON(w, r, http.StatusOK, makeRoutingRuleResp(tid, rule, true))
+		httputil.WriteJSON(w, r, http.StatusOK, makeRoutingRuleResp(rule.KeyRouteID(), rule, true))
 	})
 }
 
@@ -521,7 +521,7 @@ func (m *Node) putRoute() http.HandlerFunc {
 			httputil.WriteJSON(w, r, http.StatusBadRequest, err)
 			return
 		}
-		if err := ctx.RPC.SetRoutingRule(ctx.RtKey, rule); err != nil {
+		if err := ctx.RPC.SaveRoutingRule(rule); err != nil {
 			httputil.WriteJSON(w, r, http.StatusInternalServerError, err)
 			return
 		}

pkg/router/route_manager.go

@@ -113,7 +113,7 @@ func (rm *routeManager) handleSetupConn(conn net.Conn) error {
 	var respBody interface{}
 	switch t {
 	case setup.PacketAddRules:
-		err = rm.setRoutingRules(body)
+		err = rm.saveRoutingRules(body)
 	case setup.PacketDeleteRules:
 		respBody, err = rm.deleteRoutingRules(body)
 	case setup.PacketConfirmLoop:
@@ -170,34 +170,32 @@ func (rm *routeManager) RemoveLoopRule(loop routing.Loop) {
 	remote := loop.Remote
 	local := loop.Local
 
-	entries := rm.rt.AllRules()
-	for _, entry := range entries {
-		rule := entry.Rule
+	rules := rm.rt.AllRules()
+	for _, rule := range rules {
 		if rule.Type() != routing.RuleConsume {
 			continue
 		}
 
 		rd := rule.RouteDescriptor()
 		if rd.DstPK() == remote.PubKey && rd.DstPort() == remote.Port && rd.SrcPort() == local.Port {
-			rm.rt.DelRules([]routing.RouteID{entry.RouteID})
+			rm.rt.DelRules([]routing.RouteID{rule.KeyRouteID()})
 			return
 		}
 	}
 }
 
-func (rm *routeManager) setRoutingRules(data []byte) error {
+func (rm *routeManager) saveRoutingRules(data []byte) error {
 	var rules []routing.Rule
 	if err := json.Unmarshal(data, &rules); err != nil {
 		return err
 	}
 
 	for _, rule := range rules {
-		routeID := rule.KeyRouteID()
-		if err := rm.rt.SaveRule(routeID, rule); err != nil {
+		if err := rm.rt.SaveRule(rule); err != nil {
 			return fmt.Errorf("routing table: %s", err)
 		}
 
-		rm.Logger.Infof("Set new Routing Rule with ID %d %s", routeID, rule)
+		rm.Logger.Infof("Save new Routing Rule with ID %d %s", rule.KeyRouteID(), rule)
 	}
 
 	return nil
@@ -227,17 +225,16 @@ func (rm *routeManager) confirmLoop(data []byte) error {
 	var appRouteID routing.RouteID
 	var consumeRule routing.Rule
 
-	entries := rm.rt.AllRules()
-	for _, entry := range entries {
-		rule := entry.Rule
+	rules := rm.rt.AllRules()
+	for _, rule := range rules {
 		if rule.Type() != routing.RuleConsume {
 			continue
 		}
 
 		rd := rule.RouteDescriptor()
 		if rd.DstPK() == remote.PubKey && rd.DstPort() == remote.Port && rd.SrcPort() == local.Port {
 
-			appRouteID = entry.RouteID
+			appRouteID = rule.KeyRouteID()
 			consumeRule = make(routing.Rule, len(rule))
 			copy(consumeRule, rule)
 
@@ -263,8 +260,8 @@ func (rm *routeManager) confirmLoop(data []byte) error {
 	}
 
 	rm.Logger.Infof("Setting reverse route ID %d for rule with ID %d", ld.RouteID, appRouteID)
-	consumeRule.SetKeyRouteID(ld.RouteID)
-	if rErr := rm.rt.SaveRule(appRouteID, consumeRule); rErr != nil {
+	consumeRule.SetKeyRouteID(appRouteID)
+	if rErr := rm.rt.SaveRule(consumeRule); rErr != nil {
 		return fmt.Errorf("routing table: %s", rErr)
 	}
 

pkg/router/route_manager_test.go

@@ -22,34 +22,36 @@ func TestNewRouteManager(t *testing.T) {
 	env := snettest.NewEnv(t, []snettest.KeyPair{{PK: pk, SK: sk}})
 	defer env.Teardown()
 
-	rt := routing.NewWithConfig(routing.Config{GCInterval: 100 * time.Millisecond})
+	rt := routing.NewTable(routing.Config{GCInterval: 100 * time.Millisecond})
 
 	rm, err := newRouteManager(env.Nets[0], rt, RMConfig{})
 	require.NoError(t, err)
 	defer func() { require.NoError(t, rm.Close()) }()
 
 	// CLOSURE: Delete all routing rules.
 	clearRules := func() {
-		entries := rt.AllRules()
-		for _, entry := range entries {
-			rt.DelRules([]routing.RouteID{entry.RouteID})
+		rules := rt.AllRules()
+		for _, rule := range rules {
+			rt.DelRules([]routing.RouteID{rule.KeyRouteID()})
 		}
 	}
 
 	// TEST: Set and get expired and unexpired rule.
 	t.Run("GetRule", func(t *testing.T) {
 		clearRules()
 
-		expiredRule := routing.IntermediaryForwardRule(-10*time.Minute, 1, 3, uuid.New())
 		expiredID, err := rm.rt.ReserveKey()
 		require.NoError(t, err)
-		err = rm.rt.SaveRule(expiredID, expiredRule)
+
+		expiredRule := routing.IntermediaryForwardRule(-10*time.Minute, expiredID, 3, uuid.New())
+		err = rm.rt.SaveRule(expiredRule)
 		require.NoError(t, err)
 
-		rule := routing.IntermediaryForwardRule(10*time.Minute, 2, 3, uuid.New())
 		id, err := rm.rt.ReserveKey()
 		require.NoError(t, err)
-		err = rm.rt.SaveRule(id, rule)
+
+		rule := routing.IntermediaryForwardRule(10*time.Minute, id, 3, uuid.New())
+		err = rm.rt.SaveRule(rule)
 		require.NoError(t, err)
 
 		defer rm.rt.DelRules([]routing.RouteID{id, expiredID})
@@ -74,11 +76,12 @@ func TestNewRouteManager(t *testing.T) {
 		clearRules()
 
 		pk, _ := cipher.GenerateKeyPair()
-		rule := routing.ConsumeRule(10*time.Minute, 1, pk, 2, 3)
 
 		id, err := rm.rt.ReserveKey()
 		require.NoError(t, err)
-		err = rm.rt.SaveRule(id, rule)
+
+		rule := routing.ConsumeRule(10*time.Minute, id, pk, 2, 3)
+		err = rm.rt.SaveRule(rule)
 		require.NoError(t, err)
 
 		loop := routing.Loop{Local: routing.Addr{Port: 3}, Remote: routing.Addr{PubKey: pk, Port: 3}}
@@ -143,7 +146,7 @@ func TestNewRouteManager(t *testing.T) {
 	})
 
 	// TEST: Ensure DeleteRule requests from SetupNode is handled properly.
-	t.Run("DelRules", func(t *testing.T) {
+	t.Run("DeleteRules", func(t *testing.T) {
 		clearRules()
 
 		in, out := net.Pipe()
@@ -159,11 +162,12 @@ func TestNewRouteManager(t *testing.T) {
 
 		proto := setup.NewSetupProtocol(in)
 
-		rule := routing.IntermediaryForwardRule(10*time.Minute, 1, 3, uuid.New())
-
 		id, err := rm.rt.ReserveKey()
 		require.NoError(t, err)
-		err = rm.rt.SaveRule(id, rule)
+
+		rule := routing.IntermediaryForwardRule(10*time.Minute, id, 3, uuid.New())
+
+		err = rm.rt.SaveRule(rule)
 		require.NoError(t, err)
 
 		assert.Equal(t, 1, rt.Count())
@@ -199,11 +203,12 @@ func TestNewRouteManager(t *testing.T) {
 
 		proto := setup.NewSetupProtocol(in)
 		pk, _ := cipher.GenerateKeyPair()
+
 		rule := routing.ConsumeRule(10*time.Minute, 2, pk, 2, 3)
-		require.NoError(t, rt.SaveRule(2, rule))
+		require.NoError(t, rt.SaveRule(rule))
 
 		rule = routing.IntermediaryForwardRule(10*time.Minute, 1, 3, uuid.New())
-		require.NoError(t, rt.SaveRule(1, rule))
+		require.NoError(t, rt.SaveRule(rule))
 
 		ld := routing.LoopData{
 			Loop: routing.Loop{
@@ -251,11 +256,11 @@ func TestNewRouteManager(t *testing.T) {
 		proto := setup.NewSetupProtocol(in)
 		pk, _ := cipher.GenerateKeyPair()
 
-		rule := routing.ConsumeRule(10*time.Minute, 0, pk, 2, 3)
-		require.NoError(t, rt.SaveRule(2, rule))
+		rule := routing.ConsumeRule(10*time.Minute, 2, pk, 2, 3)
+		require.NoError(t, rt.SaveRule(rule))
 
 		rule = routing.IntermediaryForwardRule(10*time.Minute, 1, 3, uuid.New())
-		require.NoError(t, rt.SaveRule(1, rule))
+		require.NoError(t, rt.SaveRule(rule))
 
 		ld := routing.LoopData{
 			Loop: routing.Loop{

pkg/router/router_test.go

@@ -61,9 +61,9 @@ func TestRouter_Serve(t *testing.T) {
 	// CLOSURE: clear all rules in all router.
 	clearRules := func(routers ...*Router) {
 		for _, r := range routers {
-			entries := r.rm.rt.AllRules()
-			for _, entry := range entries {
-				r.rm.rt.DelRules([]routing.RouteID{entry.RouteID})
+			rules := r.rm.rt.AllRules()
+			for _, rule := range rules {
+				r.rm.rt.DelRules([]routing.RouteID{rule.KeyRouteID()})
 			}
 		}
 	}
@@ -75,12 +75,11 @@ func TestRouter_Serve(t *testing.T) {
 		defer clearRules(r0, r1)
 
 		// Add a FWD rule for r0.
-		fwdRule := routing.IntermediaryForwardRule(1*time.Hour, routing.RouteID(0), routing.RouteID(5), tp1.Entry.ID)
 		fwdRtID, err := r0.rm.rt.ReserveKey()
 		require.NoError(t, err)
-		err = r0.rm.rt.SaveRule(fwdRtID, fwdRule)
-		require.NoError(t, err)
 
+		fwdRule := routing.IntermediaryForwardRule(1*time.Hour, fwdRtID, routing.RouteID(5), tp1.Entry.ID)
+		err = r0.rm.rt.SaveRule(fwdRule)
 		require.NoError(t, err)
 
 		// Call handlePacket for r0 (this should in turn, use the rule we added).
@@ -213,7 +212,7 @@ func (e *TestEnv) GenRouterConfig(i int) *Config {
 		PubKey:           e.TpMngrConfs[i].PubKey,
 		SecKey:           e.TpMngrConfs[i].SecKey,
 		TransportManager: e.TpMngrs[i],
-		RoutingTable:     routing.New(),
+		RoutingTable:     routing.NewTable(routing.DefaultConfig()),
 		RouteFinder:      routeFinder.NewMock(),
 		SetupNodes:       nil, // TODO
 	}

pkg/routing/rule.go

@@ -386,12 +386,12 @@ func (r Rule) Summary() *RuleSummary {
 }
 
 // ConsumeRule constructs a new Consume rule.
-func ConsumeRule(keepAlive time.Duration, keyRouteID RouteID, remotePK cipher.PubKey, localPort, remotePort Port) Rule {
+func ConsumeRule(keepAlive time.Duration, key RouteID, remotePK cipher.PubKey, localPort, remotePort Port) Rule {
 	rule := Rule(make([]byte, RuleHeaderSize+routeDescriptorSize))
 
 	rule.setKeepAlive(keepAlive)
 	rule.setType(RuleConsume)
-	rule.SetKeyRouteID(keyRouteID)
+	rule.SetKeyRouteID(key)
 
 	rule.setDstPK(remotePK)
 	rule.setSrcPK(cipher.PubKey{})
@@ -402,12 +402,12 @@ func ConsumeRule(keepAlive time.Duration, keyRouteID RouteID, remotePK cipher.Pu
 }
 
 // ForwardRule constructs a new Forward rule.
-func ForwardRule(keepAlive time.Duration, keyRouteID, nextRoute RouteID, nextTransport uuid.UUID, remotePK cipher.PubKey, localPort, remotePort Port) Rule {
+func ForwardRule(keepAlive time.Duration, key, nextRoute RouteID, nextTransport uuid.UUID, remotePK cipher.PubKey, localPort, remotePort Port) Rule {
 	rule := Rule(make([]byte, RuleHeaderSize+routeDescriptorSize+4+pkSize))
 
 	rule.setKeepAlive(keepAlive)
 	rule.setType(RuleForward)
-	rule.SetKeyRouteID(keyRouteID)
+	rule.SetKeyRouteID(key)
 	rule.setNextRouteID(nextRoute)
 	rule.setNextTransportID(nextTransport)
 
@@ -420,12 +420,12 @@ func ForwardRule(keepAlive time.Duration, keyRouteID, nextRoute RouteID, nextTra
 }
 
 // IntermediaryForwardRule constructs a new IntermediaryForward rule.
-func IntermediaryForwardRule(keepAlive time.Duration, keyRouteID, nextRoute RouteID, nextTransport uuid.UUID) Rule {
+func IntermediaryForwardRule(keepAlive time.Duration, key, nextRoute RouteID, nextTransport uuid.UUID) Rule {
 	rule := Rule(make([]byte, RuleHeaderSize+4+pkSize))
 
 	rule.setKeepAlive(keepAlive)
 	rule.setType(RuleIntermediaryForward)
-	rule.SetKeyRouteID(keyRouteID)
+	rule.SetKeyRouteID(key)
 	rule.setNextRouteID(nextRoute)
 	rule.setNextTransportID(nextTransport)