cli: Fix JSON output for BPF conntrack & NAT tables dump by qmonnet · Pull Request #10904 · cilium/cilium

qmonnet · 2020-04-08T22:35:52Z

JSON output is broken for conntrack and NAT tables dump with the cli (See #2976). Let's fix it. Dump arrays containing key/value tuples, so we can let the JSON parser decompose the entries into atomic values.

qmonnet · 2020-04-08T22:36:55Z

test-me-please

coveralls · 2020-04-08T23:01:07Z

Coverage decreased (-2.2%) to 44.664% when pulling 39029ce on pr/qmonnet/ct_list_json into 7bb9ed6 on master.

qmonnet · 2020-04-08T23:05:21Z

Travis failed with:

E: Failed to fetch http://ports.ubuntu.com/ubuntu-ports/pool/main/s/sgml-base/sgml-base_1.29_all.deb  Temporary failure resolving 'ports.ubuntu.com'

E: Unable to fetch some archives, maybe run apt-get update or try with --fix-missing?
apt-get.diagnostics

apt-get install failed

The command "sudo -E apt-get -yq --no-install-suggests --no-install-recommends $(travis_apt_get_options) install kernel-package" failed and exited with 100 during .

Probably not relevant. Is there a keyword to restart Travis only?

joestringer · 2020-04-08T23:05:49Z

I don't think so. You can browse to the Travis UI and restart it (assuming you're logged in and your github account is linked IIRC)

pchaigno

Looks good! I have one question below. Also wondering if we should add unit tests for this? Could be as simple as checking the output if correctly formatted JSON (e.g., | python -m json.tool).

qmonnet · 2020-04-09T08:14:55Z

test-with-kernel
(please)

qmonnet · 2020-04-09T22:55:30Z

So I added (third commit) unit tests for JSON dump of the CT map (based on newly added mock maps for ctmap). I'm pretty satisfied with the result, except that I didn't find a way to Unmarshal() the JSON output into a mix of records for both IPv4 and IPv6 flows (ctRecord4 and ctRecord6). I tried to fiddle with interfaces but didn't manage to do it, if anyone is more savvy at Go, some help here would be appreciated. In the meantime, there are two test functions in the file.

I haven't done any test for NAT dump yet, I can add it if reviewers think the approach for CT maps is good.

Run with make start-kvstore && make unit-tests TESTPKGS=cilium/cmd.

Endianness for RevNAT, as pointed by Paul, has been fixed (only change on first two commits):

diff --git b/cilium/cmd/bpf_ct_list.go a/cilium/cmd/bpf_ct_list.go
index 2ee31175e904..e057668a3207 100644
--- b/cilium/cmd/bpf_ct_list.go
+++ a/cilium/cmd/bpf_ct_list.go
@@ -21,6 +21,7 @@ import (
 
        "github.com/cilium/cilium/common"
        "github.com/cilium/cilium/pkg/bpf"
+       "github.com/cilium/cilium/pkg/byteorder"
        "github.com/cilium/cilium/pkg/command"
        "github.com/cilium/cilium/pkg/maps/ctmap"
 
@@ -87,6 +88,7 @@ func dumpCt(eID string, maps []ctmap.CtMap) {
                if command.OutputJSON() {
                        callback := func(key bpf.MapKey, value bpf.MapValue) {
                                keyval := ctKeyVal{MapKey: key.(ctmap.CtKey).ToHost(), MapValue: *value.(*ctmap.CtEntry)}
+                               keyval.MapValue.RevNAT = byteorder.NetworkToHost(keyval.MapValue.RevNAT).(uint16)
                                entries = append(entries, keyval)
                        }
                        if err = m.DumpWithCallback(callback); err != nil {

qmonnet · 2020-04-14T08:52:31Z

@pchaigno @joestringer When you have a moment, could you please have a quick look at the unit test (last commit) and comment on the approach please?

pchaigno

That last commit looks good to me. Only have a few nits below.

qmonnet · 2020-04-15T11:06:39Z

test-me-please

qmonnet · 2020-04-15T11:18:54Z

test-me-please

pchaigno · 2020-04-15T14:41:32Z

That coverage decrease is kinda weird... (and doesn't look related to this pull request, which is even weirder.)

joestringer · 2020-04-15T16:23:16Z

That coverage decrease is kinda weird... (and doesn't look related to this pull request, which is even weirder.)

Yeah, I'm not overly trusting of the figures that are generated by coveralls as they do vary a lot. Unfortunately they also don't take into account any of the runtime / ginkgo testing we do either, so they're likely low-ball figures. We'd probably have to have someone dedicate a bit of time to investigate how the figures are generated, folded together & submitted to make these more reliable :(

qmonnet · 2020-04-16T16:17:47Z

New push leaves the fields in network-byte order.
It also uses the DeepEquals.

Incremental diff

diff --git a/cilium/cmd/bpf_ct_list.go b/cilium/cmd/bpf_ct_list.go
index e057668a3207..386564c6a3cc 100644
--- a/cilium/cmd/bpf_ct_list.go
+++ b/cilium/cmd/bpf_ct_list.go
@@ -21,7 +21,6 @@ import (
 
 	"github.com/cilium/cilium/common"
 	"github.com/cilium/cilium/pkg/bpf"
-	"github.com/cilium/cilium/pkg/byteorder"
 	"github.com/cilium/cilium/pkg/command"
 	"github.com/cilium/cilium/pkg/maps/ctmap"
 
@@ -87,8 +86,7 @@ func dumpCt(eID string, maps []ctmap.CtMap) {
 		// collected values from all maps to have one consistent object
 		if command.OutputJSON() {
 			callback := func(key bpf.MapKey, value bpf.MapValue) {
-				keyval := ctKeyVal{MapKey: key.(ctmap.CtKey).ToHost(), MapValue: *value.(*ctmap.CtEntry)}
-				keyval.MapValue.RevNAT = byteorder.NetworkToHost(keyval.MapValue.RevNAT).(uint16)
+				keyval := ctKeyVal{MapKey: key.(ctmap.CtKey), MapValue: *value.(*ctmap.CtEntry)}
 				entries = append(entries, keyval)
 			}
 			if err = m.DumpWithCallback(callback); err != nil {
diff --git a/cilium/cmd/bpf_ct_list_test.go b/cilium/cmd/bpf_ct_list_test.go
index fe75ba21680c..a7a41cf497ab 100644
--- a/cilium/cmd/bpf_ct_list_test.go
+++ b/cilium/cmd/bpf_ct_list_test.go
@@ -25,10 +25,10 @@ import (
 
 	"github.com/cilium/cilium/common/types"
 	"github.com/cilium/cilium/pkg/byteorder"
+	"github.com/cilium/cilium/pkg/checker"
 	"github.com/cilium/cilium/pkg/command"
 	"github.com/cilium/cilium/pkg/maps/ctmap"
 	"github.com/cilium/cilium/pkg/tuple"
-	"github.com/cilium/cilium/pkg/u8proto"
 
 	. "gopkg.in/check.v1"
 )
@@ -148,11 +148,10 @@ func (s *BPFCtListSuite) TestDumpCt4(c *C) {
 	err := json.Unmarshal([]byte(rawDump), &ctDump)
 	c.Assert(err, IsNil, Commentf("invalid JSON output: '%s', '%s'", err, rawDump))
 
-	c.Assert(ctDump[0].MapKey.SourceAddr, Equals, srcAddr4)
-	c.Assert(ctDump[0].MapKey.DestPort, Equals, uint16(80))
-	c.Assert(ctDump[0].MapKey.NextHeader, Equals, u8proto.U8proto(6))
-	c.Assert(ctDump[0].MapValue.RevNAT, Equals, uint16(27))
-	c.Assert(ctDump[0].MapValue.LastRxReport, Equals, uint32(7777))
+	// JSON output may reorder the entries, but in our case they are all
+	// the same.
+	c.Assert(ctDump[0].MapKey, checker.DeepEquals,
+		ctMaps[0].(*ctmap.CtMockMap).Entries[0].Key.(*ctmap.CtKey4).TupleKey4)
 }
 
 func (s *BPFCtListSuite) TestDumpCt6(c *C) {
@@ -186,9 +185,8 @@ func (s *BPFCtListSuite) TestDumpCt6(c *C) {
 	err := json.Unmarshal([]byte(rawDump), &ctDump)
 	c.Assert(err, IsNil, Commentf("invalid JSON output: '%s', '%s'", err, rawDump))
 
-	c.Assert(ctDump[0].MapKey.SourceAddr, Equals, srcAddr6)
-	c.Assert(ctDump[0].MapKey.DestPort, Equals, uint16(443))
-	c.Assert(ctDump[0].MapKey.NextHeader, Equals, u8proto.U8proto(17))
-	c.Assert(ctDump[0].MapValue.RevNAT, Equals, uint16(27))
-	c.Assert(ctDump[0].MapValue.LastRxReport, Equals, uint32(7777))
+	// JSON output may reorder the entries, but in our case they are all
+	// the same.
+	c.Assert(ctDump[0].MapKey, checker.DeepEquals,
+		ctMaps[0].(*ctmap.CtMockMap).Entries[0].Key.(*ctmap.CtKey6).TupleKey6)
 }
diff --git a/pkg/maps/ctmap/ctmap_mock.go b/pkg/maps/ctmap/ctmap_mock.go
index d650c2bf9ad8..fa69a9624643 100644
--- a/pkg/maps/ctmap/ctmap_mock.go
+++ b/pkg/maps/ctmap/ctmap_mock.go
@@ -27,13 +27,13 @@ type CtMockMapRecord struct {
 
 // CtMockMap implements the CtMap interface and can be used for unit tests.
 type CtMockMap struct {
-	entries []CtMockMapRecord
+	Entries []CtMockMapRecord
 }
 
 // NewCtMockMap is a constructor for a CtMockMap.
 func NewCtMockMap(records []CtMockMapRecord) *CtMockMap {
 	m := &CtMockMap{}
-	m.entries = records
+	m.Entries = records
 	return m
 }
 
@@ -63,7 +63,7 @@ func (m *CtMockMap) DumpWithCallback(cb bpf.DumpCallback) error {
 	if cb == nil {
 		return nil
 	}
-	for _, e := range m.entries {
+	for _, e := range m.Entries {
 		cb(e.Key, e.Value)
 	}
 	return nil

qmonnet · 2020-04-16T16:18:19Z

test-me-please

qmonnet · 2020-04-16T22:36:25Z

Updated according to Paul's suggestion (and added missing check on values).

New diff

diff --git a/cilium/cmd/bpf_ct_list.go b/cilium/cmd/bpf_ct_list.go
index 386564c6a3cc..01f4a9cba809 100644
--- a/cilium/cmd/bpf_ct_list.go
+++ b/cilium/cmd/bpf_ct_list.go
@@ -49,11 +49,6 @@ func init() {
 	command.AddJSONOutput(bpfCtListCmd)
 }
 
-type ctKeyVal struct {
-	MapKey   ctmap.CtKey
-	MapValue ctmap.CtEntry
-}
-
 func getMaps(eID string) []*ctmap.Map {
 	if eID == "global" {
 		return ctmap.GlobalMaps(true, true)
@@ -63,7 +58,7 @@ func getMaps(eID string) []*ctmap.Map {
 }
 
 func dumpCt(eID string, maps []ctmap.CtMap) {
-	entries := make([]ctKeyVal, 0)
+	entries := make([]ctmap.CtMapRecord, 0)
 
 	for _, m := range maps {
 		path, err := m.Path()
@@ -86,8 +81,8 @@ func dumpCt(eID string, maps []ctmap.CtMap) {
 		// collected values from all maps to have one consistent object
 		if command.OutputJSON() {
 			callback := func(key bpf.MapKey, value bpf.MapValue) {
-				keyval := ctKeyVal{MapKey: key.(ctmap.CtKey), MapValue: *value.(*ctmap.CtEntry)}
-				entries = append(entries, keyval)
+				record := ctmap.CtMapRecord{Key: key.(ctmap.CtKey), Value: *value.(*ctmap.CtEntry)}
+				entries = append(entries, record)
 			}
 			if err = m.DumpWithCallback(callback); err != nil {
 				Fatalf("Error while collecting BPF map entries: %s", err)
diff --git a/cilium/cmd/bpf_ct_list_test.go b/cilium/cmd/bpf_ct_list_test.go
index a7a41cf497ab..8434fd0b43c5 100644
--- a/cilium/cmd/bpf_ct_list_test.go
+++ b/cilium/cmd/bpf_ct_list_test.go
@@ -79,13 +79,13 @@ var (
 )
 
 type ctRecord4 struct {
-	MapKey   tuple.TupleKey4
-	MapValue ctmap.CtEntry
+	Key   tuple.TupleKey4
+	Value ctmap.CtEntry
 }
 
 type ctRecord6 struct {
-	MapKey   tuple.TupleKey6
-	MapValue ctmap.CtEntry
+	Key   tuple.TupleKey6
+	Value ctmap.CtEntry
 }
 
 func dumpAndRead(maps []ctmap.CtMap, c *C) string {
@@ -121,22 +121,22 @@ func (s *BPFCtListSuite) TestDumpCt4(c *C) {
 
 	ctMaps := [2]ctmap.CtMap{
 		ctmap.NewCtMockMap(
-			[]ctmap.CtMockMapRecord{
+			[]ctmap.CtMapRecord{
 				{
 					Key:   &ctKey4,
-					Value: &ctValue,
+					Value: ctValue,
 				},
 				{
 					Key:   &ctKey4,
-					Value: &ctValue,
+					Value: ctValue,
 				},
 			},
 		),
 		ctmap.NewCtMockMap(
-			[]ctmap.CtMockMapRecord{
+			[]ctmap.CtMapRecord{
 				{
 					Key:   &ctKey4,
-					Value: &ctValue,
+					Value: ctValue,
 				},
 			},
 		),
@@ -150,30 +150,32 @@ func (s *BPFCtListSuite) TestDumpCt4(c *C) {
 
 	// JSON output may reorder the entries, but in our case they are all
 	// the same.
-	c.Assert(ctDump[0].MapKey, checker.DeepEquals,
+	c.Assert(ctDump[0].Key, checker.DeepEquals,
 		ctMaps[0].(*ctmap.CtMockMap).Entries[0].Key.(*ctmap.CtKey4).TupleKey4)
+	c.Assert(ctDump[0].Value, checker.DeepEquals,
+		ctMaps[0].(*ctmap.CtMockMap).Entries[0].Value)
 }
 
 func (s *BPFCtListSuite) TestDumpCt6(c *C) {
 
 	ctMaps := [2]ctmap.CtMap{
 		ctmap.NewCtMockMap(
-			[]ctmap.CtMockMapRecord{
+			[]ctmap.CtMapRecord{
 				{
 					Key:   &ctKey6,
-					Value: &ctValue,
+					Value: ctValue,
 				},
 				{
 					Key:   &ctKey6,
-					Value: &ctValue,
+					Value: ctValue,
 				},
 			},
 		),
 		ctmap.NewCtMockMap(
-			[]ctmap.CtMockMapRecord{
+			[]ctmap.CtMapRecord{
 				{
 					Key:   &ctKey6,
-					Value: &ctValue,
+					Value: ctValue,
 				},
 			},
 		),
@@ -187,6 +189,8 @@ func (s *BPFCtListSuite) TestDumpCt6(c *C) {
 
 	// JSON output may reorder the entries, but in our case they are all
 	// the same.
-	c.Assert(ctDump[0].MapKey, checker.DeepEquals,
+	c.Assert(ctDump[0].Key, checker.DeepEquals,
 		ctMaps[0].(*ctmap.CtMockMap).Entries[0].Key.(*ctmap.CtKey6).TupleKey6)
+	c.Assert(ctDump[0].Value, checker.DeepEquals,
+		ctMaps[0].(*ctmap.CtMockMap).Entries[0].Value)
 }
diff --git a/pkg/maps/ctmap/ctmap.go b/pkg/maps/ctmap/ctmap.go
index 685e58023b2c..68827dfb9c9a 100644
--- a/pkg/maps/ctmap/ctmap.go
+++ b/pkg/maps/ctmap/ctmap.go
@@ -126,6 +126,14 @@ type CtMap interface {
 	DumpWithCallback(bpf.DumpCallback) error
 }
 
+// A "Record" designates a map entry (key + value), but avoid "entry" because of
+// possible confusion with "CtEntry" (actually the value part).
+// This type is used for JSON dump and mock maps.
+type CtMapRecord struct {
+	Key   CtKey
+	Value CtEntry
+}
+
 func setupMapInfo(m mapType, define string, mapKey bpf.MapKey, keySize int, maxEntries int, nat NatMap) {
 	mapInfo[m] = mapAttributes{
 		bpfDefine: define,
diff --git a/pkg/maps/ctmap/ctmap_mock.go b/pkg/maps/ctmap/ctmap_mock.go
index fa69a9624643..8c5e9cece7bd 100644
--- a/pkg/maps/ctmap/ctmap_mock.go
+++ b/pkg/maps/ctmap/ctmap_mock.go
@@ -18,20 +18,13 @@ import (
 	"github.com/cilium/cilium/pkg/bpf"
 )
 
-// A "Record" designates a map entry (key + value), but avoid "entry" because of
-// possible confusion with "CtEntry" (actually the value part).
-type CtMockMapRecord struct {
-	Key   CtKey
-	Value *CtEntry
-}
-
 // CtMockMap implements the CtMap interface and can be used for unit tests.
 type CtMockMap struct {
-	Entries []CtMockMapRecord
+	Entries []CtMapRecord
 }
 
 // NewCtMockMap is a constructor for a CtMockMap.
-func NewCtMockMap(records []CtMockMapRecord) *CtMockMap {
+func NewCtMockMap(records []CtMapRecord) *CtMockMap {
 	m := &CtMockMap{}
 	m.Entries = records
 	return m
@@ -64,7 +57,7 @@ func (m *CtMockMap) DumpWithCallback(cb bpf.DumpCallback) error {
 		return nil
 	}
 	for _, e := range m.Entries {
-		cb(e.Key, e.Value)
+		cb(e.Key, &e.Value)
 	}
 	return nil
 }

qmonnet · 2020-04-17T01:08:00Z

Added mock maps / unit tests for NAT table JSON dump.

qmonnet · 2020-04-17T01:08:14Z

test-me-please

qmonnet · 2020-04-17T10:16:11Z

Pushed with Paul's suggestion.

Incremental diff

diff --git a/cilium/cmd/bpf_ct_list_test.go b/cilium/cmd/bpf_ct_list_test.go
index 520e050e3c9b..990cee85d839 100644
--- a/cilium/cmd/bpf_ct_list_test.go
+++ b/cilium/cmd/bpf_ct_list_test.go
@@ -154,10 +154,11 @@ func (s *BPFCtListSuite) TestDumpCt4(c *C) {
 
 	// JSON output may reorder the entries, but in our case they are all
 	// the same.
-	c.Assert(ctDump[0].Key, checker.DeepEquals,
-		ctMaps[0].(*ctmap.CtMockMap).Entries[0].Key.(*ctmap.CtKey4).TupleKey4)
-	c.Assert(ctDump[0].Value, checker.DeepEquals,
-		ctMaps[0].(*ctmap.CtMockMap).Entries[0].Value)
+	ctRecordDump := ctmap.CtMapRecord{
+		Key:   &ctmap.CtKey4{TupleKey4: ctDump[0].Key},
+		Value: ctDump[0].Value,
+	}
+	c.Assert(ctRecordDump, checker.DeepEquals, ctMaps[0].(*ctmap.CtMockMap).Entries[0])
 }
 
 func (s *BPFCtListSuite) TestDumpCt6(c *C) {
@@ -197,8 +198,9 @@ func (s *BPFCtListSuite) TestDumpCt6(c *C) {
 
 	// JSON output may reorder the entries, but in our case they are all
 	// the same.
-	c.Assert(ctDump[0].Key, checker.DeepEquals,
-		ctMaps[0].(*ctmap.CtMockMap).Entries[0].Key.(*ctmap.CtKey6).TupleKey6)
-	c.Assert(ctDump[0].Value, checker.DeepEquals,
-		ctMaps[0].(*ctmap.CtMockMap).Entries[0].Value)
+	ctRecordDump := ctmap.CtMapRecord{
+		Key:   &ctmap.CtKey6{TupleKey6: ctDump[0].Key},
+		Value: ctDump[0].Value,
+	}
+	c.Assert(ctRecordDump, checker.DeepEquals, ctMaps[0].(*ctmap.CtMockMap).Entries[0])
 }
diff --git a/cilium/cmd/bpf_nat_list_test.go b/cilium/cmd/bpf_nat_list_test.go
index 61a94f107f7a..55a2c94ce8ff 100644
--- a/cilium/cmd/bpf_nat_list_test.go
+++ b/cilium/cmd/bpf_nat_list_test.go
@@ -118,10 +118,11 @@ func (s *BPFNatListSuite) TestDumpNat4(c *C) {
 
 	// JSON output may reorder the entries, but in our case they are all
 	// the same.
-	c.Assert(natDump[0].Key, checker.DeepEquals,
-		natMaps[0].(*nat.NatMockMap).Entries[0].Key.(*nat.NatKey4).TupleKey4)
-	c.Assert(natDump[0].Value, checker.DeepEquals,
-		natMaps[0].(*nat.NatMockMap).Entries[0].Value)
+	natRecordDump := nat.NatMapRecord{
+		Key:   &nat.NatKey4{TupleKey4Global: tuple.TupleKey4Global{TupleKey4: natDump[0].Key}},
+		Value: natDump[0].Value,
+	}
+	c.Assert(natRecordDump, checker.DeepEquals, natMaps[0].(*nat.NatMockMap).Entries[0])
 }
 
 func (s *BPFNatListSuite) TestDumpNat6(c *C) {
@@ -161,8 +162,9 @@ func (s *BPFNatListSuite) TestDumpNat6(c *C) {
 
 	// JSON output may reorder the entries, but in our case they are all
 	// the same.
-	c.Assert(natDump[0].Key, checker.DeepEquals,
-		natMaps[0].(*nat.NatMockMap).Entries[0].Key.(*nat.NatKey6).TupleKey6)
-	c.Assert(natDump[0].Value, checker.DeepEquals,
-		natMaps[0].(*nat.NatMockMap).Entries[0].Value)
+	natRecordDump := nat.NatMapRecord{
+		Key:   &nat.NatKey6{TupleKey6Global: tuple.TupleKey6Global{TupleKey6: natDump[0].Key}},
+		Value: natDump[0].Value,
+	}
+	c.Assert(natRecordDump, checker.DeepEquals, natMaps[0].(*nat.NatMockMap).Entries[0])
 }

qmonnet · 2020-04-17T10:16:22Z

test-me-please

qmonnet · 2020-04-17T14:13:49Z

restart-ginkgo

qmonnet · 2020-04-20T09:15:21Z

restart-ginkgo

JSON output for BPF conntrack table dump is broken: We pass map objects to the JSON printer, which prints information on the maps but does not iterate on (or print) its entries. The final output may not even a real JSON object if we call the printer on several maps (several JSON objects are dumped successively, not gathered in an array). Let's fix JSON output. Collect all the entries (key/values) from the different maps, and pass them to the json package so it can print something nice. The result now looks like this: # cilium bpf ct global list -o json [ { "Key": { "DestAddr": [ 192, 168, 33, 11 ], "SourceAddr": [ 10, 16, 196, 61 ], "DestPort": 40624, "SourcePort": 6443, "NextHeader": 6, "Flags": 1 }, "Value": { "RxPackets": 1, "RxBytes": 706, "TxPackets": 1, "TxBytes": 54, "Lifetime": 49472, "Flags": 18, "RevNAT": 0, "TxFlagsSeen": 4, "RxFlagsSeen": 2, "SourceSecurityID": 1, "LastTxReport": 27872, "LastRxReport": 27872 } }, ... ] Fixes: #2976 Signed-off-by: Quentin Monnet <quentin@isovalent.com>

Fix JSON output for dumping the NAT tables with the cli, in a way similar to what we did for the conntrack tables. Signed-off-by: Quentin Monnet <quentin@isovalent.com>

Add mock maps to the ctmap package. They can be used for unit tests. Speaking of: Add a unit test for JSON dump of the conntrack maps via the cli. We dump and parse JSON, then we check that some of the values are consistent. Signed-off-by: Quentin Monnet <quentin@isovalent.com>

Similarly to ctmap, add mock maps and unit tests for JSON dump for NAT maps. The code is very similar to the equivalent for ctmap. Make generic and reuse dumpAndRead() from bpf_ct_list_test.go. Signed-off-by: Quentin Monnet <quentin@isovalent.com>

qmonnet · 2020-04-20T14:36:58Z

test-me-please

aanm

@pchaigno @joestringer the coverage decreased because @qmonnet added a test for cilium/cmd package which was not being accounted in coveralls because it does not have any test. The coverage seems to be accounted per package that contains a _test.go

joestringer

Minor new discussion point, more future-looking and not relevant to the acceptance of this PR.

joestringer · 2020-04-20T23:31:40Z

+
+// NatMockMap implements the NatMap interface and can be used for unit tests.
+type NatMockMap struct {
+	Entries []NatMapRecord


This is nice to see as an example of the kind of mocking we would want to be able to properly unit-test the core of the daemon. I wonder if you have thought about how we might centralize these mocks so that we have a single implementation you can use to instantiate a memory-backed "BPF" map in a generic way without having to duplicate the similar base functionality in each package?

FWIW we have a pkg/testutils that hosts a bunch of things like this.

Separately, we've also been pondering about whether it makes sense to have such a mock framework as part of the cilium/ebpf repository. Without examples like these, we haven't really known what such mocks would need to look like just yet.

I did think about centralising them, but I wasn't sure there was enough interest at this time and preferred not to wander too far in that direction, apart from reusing the dumpAndRead() bit. But yeah, definitely something we could consider for the future. Most of the code for NAT and CT mock maps is very close, it should not be too hard to figure out something. Depending on what other map types or use cases you have in mind, it could add some challenge, of course.

One mock to rule them all! ...well yeah I guess just mainly the ones we're interacting with from Cilium userspace which are primarily hash-or-similar: LRU, LPM, hashmap. I don't think we even at the moment need to care too much about the fact that lookups for LPM work differently, as BPF LPM map lookups are only done from the (Envoy) proxy and not from cilium-agent [citation needed].

qmonnet requested a review from joestringer April 8, 2020 22:35

qmonnet requested a review from a team as a code owner April 8, 2020 22:35

qmonnet mentioned this pull request Apr 8, 2020

Support for IPv4 fragments #10264

Merged

pchaigno reviewed Apr 9, 2020

View reviewed changes

Comment thread cilium/cmd/bpf_ct_list.go Outdated

aanm approved these changes Apr 9, 2020

View reviewed changes

joestringer approved these changes Apr 9, 2020

View reviewed changes

qmonnet force-pushed the pr/qmonnet/ct_list_json branch from a160f85 to d2b5003 Compare April 9, 2020 22:55

qmonnet requested a review from a team April 9, 2020 22:55

qmonnet requested a review from a team as a code owner April 9, 2020 22:55

qmonnet force-pushed the pr/qmonnet/ct_list_json branch 2 times, most recently from 2ad2700 to fd9211a Compare April 9, 2020 23:10

pchaigno reviewed Apr 14, 2020

View reviewed changes

Comment thread cilium/cmd/bpf_ct_list_test.go

Comment thread cilium/cmd/bpf_ct_list_test.go Outdated

Comment thread cilium/cmd/bpf_ct_list_test.go Outdated

qmonnet force-pushed the pr/qmonnet/ct_list_json branch from fd9211a to 0cabfa5 Compare April 14, 2020 14:22

qmonnet force-pushed the pr/qmonnet/ct_list_json branch from 0cabfa5 to 60445d8 Compare April 15, 2020 11:16

pchaigno reviewed Apr 15, 2020

View reviewed changes

Comment thread cilium/cmd/bpf_ct_list_test.go Outdated

qmonnet force-pushed the pr/qmonnet/ct_list_json branch from 60445d8 to 86e6a0b Compare April 16, 2020 16:15

pchaigno reviewed Apr 16, 2020

View reviewed changes

Comment thread pkg/maps/ctmap/ctmap_mock.go Outdated

qmonnet force-pushed the pr/qmonnet/ct_list_json branch from 86e6a0b to d1468e7 Compare April 16, 2020 22:34

qmonnet force-pushed the pr/qmonnet/ct_list_json branch from d1468e7 to 9e89765 Compare April 17, 2020 01:00

pchaigno approved these changes Apr 17, 2020

View reviewed changes

Comment thread cilium/cmd/bpf_ct_list_test.go Outdated

qmonnet force-pushed the pr/qmonnet/ct_list_json branch from 9e89765 to 0eb8199 Compare April 17, 2020 10:15

qmonnet added 4 commits April 20, 2020 15:36

cli: fix JSON output for BPF NAT table dump

d765e17

Fix JSON output for dumping the NAT tables with the cli, in a way similar to what we did for the conntrack tables. Signed-off-by: Quentin Monnet <quentin@isovalent.com>

qmonnet force-pushed the pr/qmonnet/ct_list_json branch from 0eb8199 to 39029ce Compare April 20, 2020 14:36

aanm approved these changes Apr 20, 2020

View reviewed changes

joestringer approved these changes Apr 20, 2020

View reviewed changes

joestringer merged commit 5c0b426 into master Apr 20, 2020

joestringer deleted the pr/qmonnet/ct_list_json branch April 20, 2020 23:34

qmonnet mentioned this pull request Sep 28, 2020

Consolidate the mock maps used for tests, instead of re-implementing them for each map package #13305

Closed

Conversation

qmonnet commented Apr 8, 2020

Uh oh!

qmonnet commented Apr 8, 2020

Uh oh!

coveralls commented Apr 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

qmonnet commented Apr 8, 2020

Uh oh!

joestringer commented Apr 8, 2020

Uh oh!

pchaigno left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

qmonnet commented Apr 9, 2020

Uh oh!

qmonnet commented Apr 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

qmonnet commented Apr 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pchaigno left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

qmonnet commented Apr 15, 2020

Uh oh!

qmonnet commented Apr 15, 2020

Uh oh!

pchaigno commented Apr 15, 2020

Uh oh!

joestringer commented Apr 15, 2020

Uh oh!

Uh oh!

qmonnet commented Apr 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

qmonnet commented Apr 16, 2020

Uh oh!

Uh oh!

qmonnet commented Apr 16, 2020

Uh oh!

qmonnet commented Apr 17, 2020

Uh oh!

qmonnet commented Apr 17, 2020

Uh oh!

Uh oh!

qmonnet commented Apr 17, 2020

Uh oh!

qmonnet commented Apr 17, 2020

Uh oh!

qmonnet commented Apr 17, 2020

Uh oh!

qmonnet commented Apr 20, 2020

Uh oh!

qmonnet commented Apr 20, 2020

Uh oh!

aanm left a comment

Choose a reason for hiding this comment

Uh oh!

joestringer left a comment

Choose a reason for hiding this comment

Uh oh!

joestringer Apr 20, 2020

Choose a reason for hiding this comment

Uh oh!

qmonnet Apr 21, 2020

Choose a reason for hiding this comment

Uh oh!

joestringer Apr 21, 2020

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

coveralls commented Apr 8, 2020 •

edited

Loading

pchaigno left a comment •

edited

Loading

qmonnet commented Apr 9, 2020 •

edited

Loading

qmonnet commented Apr 14, 2020 •

edited

Loading

qmonnet commented Apr 16, 2020 •

edited

Loading