Merge remote-tracking branch 'origin/topic/johanna/1033'

- Fixed signed/unsigned comparison compiler warning
- Did other minor changes to address efficiency nitpicks

* origin/topic/johanna/1033:
  BrokerStore <-> Zeek Tables: support complex indices

CHANGES

@@ -1,4 +1,16 @@
 
+3.3.0-dev.548 | 2020-11-17 13:32:57 -0800
+
+  * GH-1033: BrokerStore <-> Zeek Tables: support complex indices (Johanna Amann, Corelight)
+
+    This change adds support for complex indexes for sets and tables. With
+    this change, sets with indexes like:
+
+    set[string, count, count]
+
+    will function. Before this change, Zeek raised an error message in these
+    cases.
+
 3.3.0-dev.546 | 2020-11-17 11:50:47 +0000
 
   * Fix memory leak in deprecated Analyzer::ConnectionEvent() (Jon Siwek, Corelight)

VERSION

@@ -1 +1 @@
-3.3.0-dev.546
+3.3.0-dev.548

src/Attr.cc

@@ -674,11 +674,6 @@ void Attributes::CheckAttr(Attr* a)
 			break;
 			}
 
-		// Temporary since Broker does not support ListVals - and we
-		// cannot easily convert to set/vector
-		if ( type->AsTableType()->GetIndexTypes().size() != 1 )
-			Error("&backend only supports one-element set/table indexes");
-
 		// Only support atomic types for the moment, unless
 		// explicitly overriden
 		if ( ! type->AsTableType()->IsSet() &&
@@ -714,11 +709,6 @@ void Attributes::CheckAttr(Attr* a)
 			break;
 			}
 
-		// Temporary since Broker does not support ListVals - and we
-		// cannot easily convert to set/vector
-		if ( type->AsTableType()->GetIndexTypes().size() != 1 && ! Find(ATTR_BROKER_STORE_ALLOW_COMPLEX) )
-			Error("&broker_store only supports one-element set/table indexes");
-
 		// Only support atomic types for the moment, unless
 		// explicitly overriden
 		if ( ! type->AsTableType()->IsSet() &&

src/Type.h

@@ -458,7 +458,7 @@ public:
 
 	TypePtr ShallowClone() override;
 
-	[[deprecated("Remove in v4.1.  Use Elements() isntead.")]]
+	[[deprecated("Remove in v4.1.  Use Elements() instead.")]]
 	detail::ListExpr* SetElements() const	{ return elements.get(); }
 
 	const detail::ListExprPtr& Elements() const

src/Val.cc

@@ -2172,23 +2172,14 @@ void TableVal::SendToStore(const Val* index, const TableEntryVal* new_entry_val,
 		if ( ! handle )
 			return;
 
-		// we either get passed the raw index_val - or a ListVal with exactly one element.
+		// For simple indexes, we either get passed the raw index_val - or a ListVal with exactly one element.
-		// Since Broker does not support ListVals, we have to unoll this in the second case.
+		// We unoll this in the second case.
+		// For complex indexes, we just pass the ListVal.
 		const Val* index_val;
-		if ( index->GetType()->Tag() == TYPE_LIST )
+		if ( index->GetType()->Tag() == TYPE_LIST && index->AsListVal()->Length() == 1)
-			{
-			if ( index->AsListVal()->Length() != 1 )
-				{
-				emit_builtin_error("table with complex index not supported for &broker_store");
-				return;
-				}
-
 			index_val = index->AsListVal()->Idx(0).get();
-			}
 		else
-			{
 			index_val = index;
-			}
 
 		auto broker_index = Broker::detail::val_to_data(index_val);
 

src/broker/Data.cc

@@ -345,6 +345,32 @@ struct val_converter {
 
 			return rval;
 			}
+		else if ( type->Tag() == TYPE_LIST )
+			{
+			// lists are just treated as vectors on the broker side.
+			auto lt = type->AsTypeList();
+			auto pure = lt->IsPure();
+			const auto& types = lt->GetTypes();
+
+			if ( ! pure && a.size() > types.size() )
+				return nullptr;
+
+			auto lt_tag = pure ? lt->GetPureType()->Tag() : TYPE_ANY;
+			auto rval = make_intrusive<ListVal>(lt_tag);
+
+			unsigned int pos = 0;
+			for ( auto& item : a )
+				{
+				auto item_val = data_to_val(move(item), pure ? lt->GetPureType().get() : types[pos].get());
+				pos++;
+
+				if ( ! item_val )
+					return nullptr;
+
+				rval->Append(std::move(item_val));
+				}
+			return rval;
+			}
 		else if ( type->Tag() == TYPE_FUNC )
 			{
 			if ( a.size() < 1 || a.size() > 2 )
@@ -955,6 +981,31 @@ broker::expected<broker::data> val_to_data(const Val* v)
 			rval.emplace_back(move(*item));
 			}
 
+		return {std::move(rval)};
+		}
+	case TYPE_LIST:
+		{
+		// We don't really support lists on the broker side.
+		// So we just pretend that it is a vector instead.
+		auto list = v->AsListVal();
+		broker::vector rval;
+		rval.reserve(list->Length());
+
+		for ( auto i = 0; i < list->Length(); ++i )
+			{
+			const auto& item_val = list->Idx(i);
+
+			if ( ! item_val )
+				continue;
+
+			auto item = val_to_data(item_val.get());
+
+			if ( ! item )
+				return broker::ec::invalid_data;
+
+			rval.emplace_back(move(*item));
+			}
+
 		return {std::move(rval)};
 		}
 	case TYPE_RECORD:

src/broker/Manager.cc

@@ -1031,8 +1031,12 @@ void Manager::ProcessStoreEventInsertUpdate(const TableValPtr& table,
 			}
 
 		const auto& its = table->GetType()->AsTableType()->GetIndexTypes();
-		assert( its.size() == 1 );
+		ValPtr zeek_key;
-		auto zeek_key = detail::data_to_val(key, its[0].get());
+		if ( its.size() == 1 )
+			zeek_key = detail::data_to_val(key, its[0].get());
+		else
+			zeek_key = detail::data_to_val(key, table->GetType()->AsTableType()->GetIndices().get());
+
 		if ( ! zeek_key )
 			{
 			reporter->Error("ProcessStoreEvent %s: could not convert key \"%s\" for store \"%s\" while receiving remote data. This probably means the tables have different types on different nodes.", type, to_string(key).c_str(), store_id.c_str());
@@ -1625,14 +1629,18 @@ void Manager::BrokerStoreToZeekTable(const std::string& name, const detail::Stor
 	auto table = handle->forward_to;
 	const auto& its = table->GetType()->AsTableType()->GetIndexTypes();
 	bool is_set = table->GetType()->IsSet();
-	assert( its.size() == 1 );
 
 	// disable &on_change notifications while filling the table.
 	table->DisableChangeNotifications();
 
 	for ( const auto& key : *set )
 		{
-		auto zeek_key = detail::data_to_val(key, its[0].get());
+		ValPtr zeek_key;
+		if ( its.size() == 1 )
+			zeek_key = detail::data_to_val(key, its[0].get());
+		else
+			zeek_key = detail::data_to_val(key, table->GetType()->AsTableType()->GetIndices().get());
+
 		if ( ! zeek_key )
 			{
 			reporter->Error("Failed to convert key \"%s\" while importing broker store to table for store \"%s\". Aborting import.", to_string(key).c_str(), name.c_str());

testing/btest/Baseline/broker.store.brokerstore-attr-simple/clone.out

@@ -1,6 +1,9 @@
+### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63.
 Peer added
 [[key=a, val=3], [key=b, val=3], [key=whatever, val=5]]
-[hi]
+{
+[hi, there] 
+}
 [[key=a, val=[a=1, b=c, c={
 elem1,
 elem2

testing/btest/Baseline/broker.store.brokerstore-backend-invalid/.stderr

@@ -1,5 +1,5 @@
-error in /this/is/a/path/brokerstore-backend-invalid.zeek, line 12: &backend only supports one-element set/table indexes (&backend=Broker::MEMORY)
+### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63.
-error in /this/is/a/path/broker.store.brokerstore-backend-invalid/brokerstore-backend-invalid.zeek, line 13: &backend only supports atomic types as table value (&backend=Broker::MEMORY)
+error in <...>/brokerstore-backend-invalid.zeek, line 12: &backend only supports atomic types as table value (&backend=Broker::MEMORY)
-error in /this/is/a/path/broker.store.brokerstore-backend-invalid/brokerstore-backend-invalid.zeek, line 14: &backend and &read_expire cannot be used simultaneously (&read_expire=5.0 secs, &backend=Broker::MEMORY)
+error in <...>/brokerstore-backend-invalid.zeek, line 13: &backend and &read_expire cannot be used simultaneously (&read_expire=5.0 secs, &backend=Broker::MEMORY)
-error in /this/is/a/path/broker.store.brokerstore-backend-invalid/brokerstore-backend-invalid.zeek, line 15: &backend and &broker_store cannot be used simultaneously (&broker_store=store, &backend=Broker::MEMORY)
+error in <...>/brokerstore-backend-invalid.zeek, line 14: &backend and &broker_store cannot be used simultaneously (&broker_store=store, &backend=Broker::MEMORY)
-error in /this/is/a/path/broker.store.brokerstore-backend-invalid/brokerstore-backend-invalid.zeek, line 16: &backend only applicable to global sets/tables (&backend=Broker::MEMORY)
+error in <...>/tables (&backend=Broker::MEMORY)

testing/btest/Baseline/broker.store.brokerstore-backend-simple-reverse/clone.out

@@ -1,3 +1,7 @@
+### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63.
 [[key=a, val=3], [key=b, val=3], [key=whatever, val=5]]
 [hi]
 [[key=a, val=[a=1, b=c, c=[elem1, elem2]]], [key=b, val=[a=2, b=d, c=[elem1, elem2]]]]
+{
+[a, [a=1, b=b, c=[elem1, elem2]]] = 2
+}

testing/btest/Baseline/broker.store.brokerstore-backend-simple-reverse/master.out

@@ -1,3 +1,7 @@
+### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63.
 [[key=a, val=3], [key=b, val=3], [key=whatever, val=5]]
 [hi]
 [[key=a, val=[a=1, b=c, c=[elem1, elem2]]], [key=b, val=[a=2, b=d, c=[elem1, elem2]]]]
+{
+[a, [a=1, b=b, c=[elem1, elem2]]] = 2
+}

testing/btest/Baseline/broker.store.brokerstore-backend-sqlite/clone.out

@@ -1,3 +1,4 @@
+### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63.
 [[key=a, val=3], [key=b, val=3], [key=whatever, val=5]]
 [hi]
 [[key=a, val=[a=1, b=c, c={
@@ -7,3 +8,6 @@ elem2
 elem1,
 elem2
 }]]]
+{
+[a, 1, 2] = 2
+}

testing/btest/Baseline/broker.store.brokerstore-backend-sqlite/master.out

@@ -1,3 +1,4 @@
+### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63.
 [[key=a, val=3], [key=b, val=3], [key=whatever, val=5]]
 [hi]
 [[key=a, val=[a=1, b=c, c={
@@ -7,3 +8,6 @@ elem2
 elem1,
 elem2
 }]]]
+{
+[a, 1, 2] = 2
+}

testing/btest/broker/store/brokerstore-attr-simple.zeek

@@ -20,7 +20,7 @@ type testrec: record {
 };
 
 global t: table[string] of count &broker_store="table";
-global s: set[string] &broker_store="set";
+global s: set[string, string] &broker_store="set";
 global r: table[string] of testrec &broker_allow_complex_type &broker_store="rec";
 @TEST-END-FILE
 
@@ -38,7 +38,7 @@ event insert_stuff()
 	print "Inserting stuff";
 	t["a"] = 5;
 	delete t["a"];
-	add s["hi"];
+	add s["hi", "there"];
 	t["a"] = 2;
 	t["a"] = 3;
 	t["b"] = 3;
@@ -49,7 +49,7 @@ event insert_stuff()
 	r["a"] = testrec($a=1, $b="c", $c=set("elem1", "elem2"));
 	r["b"] = testrec($a=2, $b="d", $c=set("elem1", "elem2"));
 	print sort_table(t);
-	print sort_set(s);
+	print s;
 	print sort_table(r);
 	}
 
@@ -75,14 +75,14 @@ event zeek_init()
 event dump_tables()
 	{
 	print sort_table(t);
-	print sort_set(s);
+	print s;
 	print sort_table(r);
 	terminate();
 	}
 
 event check_all_set()
 	{
-	if ( "whatever" in t && "hi" in s && "b" in r )
+	if ( "whatever" in t && ["hi", "there"] in s && "b" in r )
 		event dump_tables();
 	else
 		schedule 0.1sec { check_all_set() };

testing/btest/broker/store/brokerstore-backend-invalid.zeek

@@ -9,7 +9,6 @@ type testrec: record {
 	c: set[string];
 };
 
-global a: table[string, count] of count &backend=Broker::MEMORY;
 global b: table[string] of testrec &backend=Broker::MEMORY;
 global c: table[string] of count &read_expire=5sec &backend=Broker::MEMORY;
 global d: table[string] of count &broker_store="store" &backend=Broker::MEMORY;

testing/btest/broker/store/brokerstore-backend-simple-reverse.zeek

@@ -36,6 +36,7 @@ type testrec: record {
 global t: table[string] of count &backend=Broker::MEMORY;
 global s: set[string] &backend=Broker::MEMORY;
 global r: table[string] of testrec &broker_allow_complex_type &backend=Broker::MEMORY;
+global rt: table[string, testrec] of count &backend=Broker::MEMORY;
 
 event go_away()
 	{
@@ -61,6 +62,7 @@ event Broker::peer_lost(endpoint: Broker::EndpointInfo, msg: string)
 	print sort_table(t);
 	print sort_set(s);
 	print sort_table(r);
+	print rt;
 	terminate();
 	}
 
@@ -82,9 +84,12 @@ event dump_tables()
 	r["a"] = testrec($a=1, $b="b", $c=vector("elem1", "elem2"));
 	r["a"] = testrec($a=1, $b="c", $c=vector("elem1", "elem2"));
 	r["b"] = testrec($a=2, $b="d", $c=vector("elem1", "elem2"));
+	rt["a", testrec($a=1, $b="b", $c=vector("elem1", "elem2"))] = 1;
+	rt["a", testrec($a=1, $b="b", $c=vector("elem1", "elem2"))] += 1;
 	print sort_table(t);
 	print sort_set(s);
 	print sort_table(r);
+	print rt;
 	}
 
 event Cluster::node_up(name: string, id: string)
@@ -110,6 +115,7 @@ event dump_tables()
 	print sort_table(t);
 	print sort_set(s);
 	print sort_table(r);
+	print rt;
 	terminate();
 	}
 

testing/btest/broker/store/brokerstore-backend-sqlite.zeek

@@ -33,6 +33,7 @@ type testrec: record {
 global t: table[string] of count &backend=Broker::SQLITE;
 global s: set[string] &backend=Broker::SQLITE;
 global r: table[string] of testrec &broker_allow_complex_type &backend=Broker::SQLITE;
+global rt: table[string, count, count] of count &backend=Broker::SQLITE;
 
 event zeek_init()
 	{
@@ -48,9 +49,14 @@ event zeek_init()
 	r["a"] = testrec($a=1, $b="b", $c=set("elem1", "elem2"));
 	r["a"] = testrec($a=1, $b="c", $c=set("elem1", "elem2"));
 	r["b"] = testrec($a=2, $b="d", $c=set("elem1", "elem2"));
+	rt["a", 1, 2] = 1;
+	rt["a", 1, 2] += 1;
+	rt["a", 1, 3] = 2;
+	delete rt["a", 1, 3];
 	print sort_table(t);
 	print sort_set(s);
 	print sort_table(r);
+	print rt;
 	}
 
 @TEST-END-FILE
@@ -71,6 +77,7 @@ function change_function(t: table[string] of count, tpe: TableChange, idxa: stri
 global t: table[string] of count &backend=Broker::SQLITE &on_change=change_function;
 global s: set[string] &backend=Broker::SQLITE;
 global r: table[string] of testrec &broker_allow_complex_type &backend=Broker::SQLITE;
+global rt: table[string, count, count] of count &backend=Broker::SQLITE;
 
 redef Broker::table_store_db_directory = "..";
 
@@ -79,6 +86,7 @@ event zeek_init()
 	print sort_table(t);
 	print sort_set(s);
 	print sort_table(r);
+	print rt;
 	}
 
 global peers_lost = 0;
@@ -102,12 +110,16 @@ redef Log::default_rotation_interval = 0secs;
 global t: table[string] of count &backend=Broker::MEMORY;
 global s: set[string] &backend=Broker::MEMORY;
 global r: table[string] of testrec &broker_allow_complex_type &backend=Broker::MEMORY;
+# This _should_ work with both SQLITE and MEMORY, since Zeek should figure out itself that
+# the manager has the main copy.
+global rt: table[string, count, count] of count &backend=Broker::SQLITE;
 
 event dump_tables()
 	{
 	print sort_table(t);
 	print sort_set(s);
 	print sort_table(r);
+	print rt;
 	terminate();
 	}