Repository: srl-labs/containerlab (go)

Issue:
allow binds to merge along the `defaults->kind->node` route
right now the binds are defined on either of topology levels
It makes sense to merge them, so that the common binds might be defined on the defaults/kind level, and node specific binds will be added on the node level

Pull request:
binds merging
fix #745

Patch:
diff --git a/docs/manual/nodes.md b/docs/manual/nodes.md
index 83c6ca40..92813fdb 100644
--- a/docs/manual/nodes.md
+++ b/docs/manual/nodes.md
@@ -146,6 +146,8 @@ binds:
 
     Notice how `$nodeDir` hides the directory structure and node names and removes the verbosity of the previous approach.
 
+Bind defined on multiple levels (defaults -> kind -> node) will be merged with the duplicated binds removed.
+
 ### ports
 To bind the ports between the lab host and the containers the users can populate the `ports` object inside the node:
 
diff --git a/types/topology.go b/types/topology.go
index 312c9f9a..0b9d544e 100644
--- a/types/topology.go
+++ b/types/topology.go
@@ -71,13 +71,12 @@ func (t *Topology) GetNodeKind(name string) string {
 
 func (t *Topology) GetNodeBinds(name string) []string {
 	if ndef, ok := t.Nodes[name]; ok {
-		if len(ndef.GetBinds()) > 0 {
-			return ndef.GetBinds()
-		}
-		if len(t.GetKind(t.GetNodeKind(name)).GetBinds()) > 0 {
-			return t.GetKind(t.GetNodeKind(name)).GetBinds()
-		}
-		return t.GetDefaults().GetBinds()
+		// return merge product of bind slices
+		return utils.MergeStringSlices(
+			ndef.GetBinds(),
+			t.GetKind(t.GetNodeKind(name)).GetBinds(),
+			t.GetDefaults().GetBinds(),
+		)
 	}
 	return nil
 }
diff --git a/utils/env.go b/utils/env.go
index 8630735a..6f634a06 100644
--- a/utils/env.go
+++ b/utils/env.go
@@ -85,3 +85,31 @@ func StringInSlice(slice []string, val string) (int, bool) {
 	}
 	return -1, false
 }
+
+// MergeStringSlices merges string slices with duplicates removed
+func MergeStringSlices(ss ...[]string) []string {
+	res := make([]string, 0)
+	allNils := true // switch to track if all of the passed slices are nils
+	for _, s := range ss {
+		res = append(res, s...)
+		if s != nil {
+			allNils = false
+		}
+	}
+
+	// if all slices are nil, return nil instead of an empty slice
+	if allNils {
+		return nil
+	}
+
+	m := map[string]struct{}{}
+	uniques := make([]string, 0)
+	for _, val := range res {
+		if _, ok := m[val]; !ok {
+			m[val] = struct{}{}
+			uniques = append(uniques, val)
+		}
+	}
+
+	return uniques
+}


Test patch:
diff --git a/clab/config_test.go b/clab/config_test.go
index 4daaab10..afc07e59 100644
--- a/clab/config_test.go
+++ b/clab/config_test.go
@@ -12,8 +12,8 @@ import (
 	"strings"
 	"testing"
 
+	"github.com/containers/podman/v3/pkg/util"
 	"github.com/google/go-cmp/cmp"
-	"github.com/srl-labs/containerlab/types"
 )
 
 func TestLicenseInit(t *testing.T) {
@@ -64,7 +64,7 @@ func TestBindsInit(t *testing.T) {
 		got  string
 		want []string
 	}{
-		"node_sing_bind": {
+		"node_single_bind": {
 			got:  "test_data/topo1.yml",
 			want: []string{"test_data/node1.lic:/dst"},
 		},
@@ -72,17 +72,17 @@ func TestBindsInit(t *testing.T) {
 			got:  "test_data/topo2.yml",
 			want: []string{"test_data/node1.lic:/dst1", "test_data/kind.lic:/dst2"},
 		},
-		"kind_binds": {
+		"kind_and_node_binds": {
 			got:  "test_data/topo5.yml",
-			want: []string{"test_data/kind.lic:/dst"},
+			want: []string{"test_data/kind.lic:/dst", "test_data/node1.lic:/dst2"},
 		},
 		"default_binds": {
 			got:  "test_data/topo3.yml",
 			want: []string{"test_data/default.lic:/dst"},
 		},
-		"node_binds_override": {
+		"default_and_kind_and_node_binds": {
 			got:  "test_data/topo4.yml",
-			want: []string{"test_data/node1.lic:/dst"},
+			want: []string{"test_data/node1.lic:/dst1", "test_data/kind.lic:/dst2", "test_data/default.lic:/dst3"},
 		},
 	}
 
@@ -96,19 +96,17 @@ func TestBindsInit(t *testing.T) {
 				t.Fatal(err)
 			}
 
-			nodeCfg := c.Config.Topology.Nodes["node1"]
-			node := types.NodeConfig{}
-			nodeCfg.Kind = strings.ToLower(c.Config.Topology.GetNodeKind("node1"))
-
-			// binds := c.bindsInit(nodeCfg)
-			binds := c.Config.Topology.GetNodeBinds("node1")
+			binds := c.Nodes["node1"].Config().Binds
 			// resolve wanted paths as the binds paths are resolved as part of the c.ParseTopology
-			err = resolveBindPaths(tc.want, node.LabDir)
+			err = resolveBindPaths(tc.want, c.Nodes["node1"].Config().LabDir)
 			if err != nil {
 				t.Fatal(err)
 			}
-			if !reflect.DeepEqual(binds, tc.want) {
-				t.Fatalf("wanted %q got %q", tc.want, binds)
+
+			for _, b := range tc.want {
+				if !util.StringInSlice(b, binds) {
+					t.Fatalf("bind %q is not found in resulting binds %q", b, binds)
+				}
 			}
 		})
 	}
diff --git a/clab/test_data/topo4.yml b/clab/test_data/topo4.yml
index bae806e8..3ce11883 100644
--- a/clab/test_data/topo4.yml
+++ b/clab/test_data/topo4.yml
@@ -3,7 +3,7 @@ topology:
   defaults:
     license: test_data/default.lic
     binds:
-      - test_data/default.lic:/dst
+      - test_data/default.lic:/dst3
     env:
       env1: global
       env2: global
@@ -13,7 +13,7 @@ topology:
     srl:
       license: test_data/kind.lic
       binds:
-        - test_data/kind.lic:/dst
+        - test_data/kind.lic:/dst2
       env:
         env2: kind
         env4: kind
@@ -24,7 +24,7 @@ topology:
       type: ixr6
       license: test_data/node1.lic
       binds:
-        - test_data/node1.lic:/dst
+        - test_data/node1.lic:/dst1
       env:
         env1: node
         env5: node
diff --git a/clab/test_data/topo5.yml b/clab/test_data/topo5.yml
index 7b2192ef..33128c6e 100644
--- a/clab/test_data/topo5.yml
+++ b/clab/test_data/topo5.yml
@@ -9,3 +9,5 @@ topology:
       kind: srl
       type: ixr6
       license: test_data/node1.lic
+      binds:
+        - test_data/node1.lic:/dst2
diff --git a/types/topology_test.go b/types/topology_test.go
index 0768d86a..eed70b8d 100644
--- a/types/topology_test.go
+++ b/types/topology_test.go
@@ -114,9 +114,10 @@ var topologyTestSet = map[string]struct {
 	"node_kind_default": {
 		input: &Topology{
 			Defaults: &NodeDefinition{
-				Kind: "srl",
-				User: "user1",
-				CPU:  1,
+				Kind:  "srl",
+				User:  "user1",
+				CPU:   1,
+				Binds: []string{"x:z"},
 			},
 			Kinds: map[string]*NodeDefinition{
 				"srl": {
@@ -151,7 +152,9 @@ var topologyTestSet = map[string]struct {
 				},
 			},
 			Nodes: map[string]*NodeDefinition{
-				"node1": {},
+				"node1": {
+					Binds: []string{"e:f"},
+				},
 			},
 		},
 		want: map[string]*NodeDefinition{
@@ -170,8 +173,10 @@ var topologyTestSet = map[string]struct {
 					"bash test2.sh",
 				},
 				Binds: []string{
+					"e:f",
 					"a:b",
 					"c:d",
+					"x:z",
 				},
 				Ports: []string{
 					"80:8080",
diff --git a/utils/env_test.go b/utils/env_test.go
index b1fbe4ad..d35f9f8a 100644
--- a/utils/env_test.go
+++ b/utils/env_test.go
@@ -185,3 +185,52 @@ func TestMergeMapsLists(t *testing.T) {
 	assert(t, MergeMaps(d1, nil), d1)
 	assert(t, MergeMaps(d1, d2), d2)
 }
+
+func TestMergeStringSlices(t *testing.T) {
+	type args struct {
+		slices [][]string
+	}
+	tt := map[string]struct {
+		got  args
+		want []string
+	}{
+		"three-non-empty-unique-slices": {
+			got: args{
+				slices: [][]string{{"1", "2"}, {"3"}, {"4", "5"}},
+			},
+			want: []string{"1", "2", "3", "4", "5"},
+		},
+		"three-non-empty-non-unique-slices": {
+			got: args{
+				slices: [][]string{{"1", "2"}, {"1", "3"}, {"2", "4", "5"}},
+			},
+			want: []string{"1", "2", "3", "4", "5"},
+		},
+		"three-non-unique-slices-one-empty": {
+			got: args{
+				slices: [][]string{{"1", "2"}, {}, {"2", "4", "5"}},
+			},
+			want: []string{"1", "2", "4", "5"},
+		},
+		"empty-slices": {
+			got: args{
+				slices: [][]string{{}, {}, nil},
+			},
+			want: []string{},
+		},
+		"nil-slices": {
+			got: args{
+				slices: [][]string{nil, nil},
+			},
+			want: nil,
+		},
+	}
+
+	for _, tc := range tt {
+		res := MergeStringSlices(tc.got.slices...)
+		if !cmp.Equal(res, tc.want) {
+			t.Fatalf("wanted %q got %q", tc.want, res)
+		}
+
+	}
+}