It's not clear to me how these map_lookup_elem_ExpectAnyArgsAndReturn functions work. Do they set the values for what map_lookup_elem would return when __sock4_xlate_fwd is in the test below?

Do they set the values for what map_lookup_elem would return when __sock4_xlate_fwd is in the test below?

You are right. These map_lookup_elem_ExpectAnyArgsAndReturn functions set the expectations for function map_lookup_elem following the order in which function map_lookup_elem is called.

So this seems somewhat brittle to me. I think it's OK if the tested function does a single call, but on the above there are 5 calls and, if my understanding is correct, changing the order that map lookups are performed would make the test fail (just to give an example). Also, it is not clear from the above which return values correspond to which map, which makes it harder to write and understand the tests.

Would it be possible to use mock data structures to hold the map values and have the map operations in the bpf code work on these mock data structures? Then, for a test just lookups values (such as the above), we could define what the contents of the map are and run the tests. This seems to be a much better way for testing functions (i.e., based on this state on the maps, we expect this result) than hard-coding expectations for every lookup call.

Would it be possible to use mock data structures to hold the map values and have the map operations in the bpf code work on these mock data structures? Then, for a test just lookups values (such as the above), we could define what the contents of the map are and run the tests. This seems to be a much better way for testing functions (i.e., based on this state on the maps, we expect this result) than hard-coding expectations for every lookup call.

I think it is not possible to do that with the current testing framework. Actually we can "use mock data structures to hold the map values and have the map operations in the bpf code work on these mock data structures" by using a user-space fake map and emulating the map operations with callbacks, but this only works if the function to be tested using only one map. This is because the real maps (kernel-based) are defined before the function to be tested, and map_lookup_elem directly takes the (real) map pointer as the argument, so the only way to emulate the operation is to define a fake map globally and stub a callback function which manipulates the fake map inside. However, if we stub a callback to map_lookup_elem, map_lookup_elem will be entirely replaced by the callback function, and there is no way to stub different callbacks for different maps.

so the only way to emulate the operation is to define a fake map globally and stub a callback function which manipulates the fake map inside. However, if we stub a callback to map_lookup_elem, map_lookup_elem will be entirely replaced by the callback function, and there is no way to stub different callbacks for different maps.

Can't we have a callback for map_lookup_elem that holds a hash table that maps the map argument to the fake map and "redirects" the lookup there?

PS: I think this is discussion should be visible to other reviewers / team members, which is why I'm unresolving it :) I would definitely be interested in hearing @joestringer's thoughts on this, for example.

But here, different map_lookup_elem calls look up in different maps, so the callback will not be able to distinguish which map_lookup_elem call looks up which map.

What would be the value of the map (first argument) in this callback? Would it be different for different maps?

What would be the value of the map (first argument) in this callback? Would it be different for different maps?

The first argument of map_lookup_elem is actually the pointer of a kernel-based map, so the callback function cannot use it. Here is an example on how to emulate the map operations.

HASHMAP(struct ipv4_ct_tuple, struct ipv4_nat_entry) ipv4_ct_tuple_map;

void *ipv4_ct_tuple_map_lookup_elem_callback(const void* map, const void* key, int cmock_num_calls) {
  return fake_lookup_elem(&ipv4_ct_tuple_map, key);
}

int ipv4_ct_tuple_map_update_elem_callback(const void* map, const void* key, const void* value, __u32 flags, int cmock_num_calls) {
  return fake_update_elem(&ipv4_ct_tuple_map, key, value, flags, SNAT_MAPPING_IPV4_SIZE);
}

int ipv4_ct_tuple_map_delete_elem_callback(const void* map, const void* key, int cmock_num_calls) {
  return fake_delete_elem(&ipv4_ct_tuple_map, key);
}

You can see from the above example that we need to create a user-space map ipv4_ct_tuple_map before defining these callbacks and manipulate the fake map inside without doing anything with the map in the argument.

This is an idea of what I had in mind:

diff --git a/bpf/tests/sockv4_test.h b/bpf/tests/sockv4_test.h
index 6c07b42633..bcb3035cac 100644
--- a/bpf/tests/sockv4_test.h
+++ b/bpf/tests/sockv4_test.h
@@ -31,8 +31,23 @@ struct lb4_service svc;
 struct lb4_backend backend;
 struct lb_affinity_val val;
 
+void *mock_map_lookup_elem(const void *map, const void *key, int ntries) {
+       if map == &LB4_SERVICES_MAP_V2 {
+               // redirect to fake LB4_SERVICES_MAP_V2 map
+       } else if map == &other_map {
+               // redirect to other map
+       } else if .... {
+               // redirect to other map
+       } else {
+               // Fail with a message about unknown map for this test
+       }
+}
+
 void test___sock4_xlate_fwd() {
 
+    // create fake maps contents, and add callback
+    map_lookup_elem_AddCallback(mock_map_lookup_elem);
+
     // udp_only is set to false and sock_proto_enabled returns false.
     assert(__sock4_xlate_fwd(&ctx, &ctx_full, false) == -ENOTSUP);

The idea being that we create fake maps with their content for every test and have the callback function do the dispatching.

One non-trivial aspect of this is how to implement the fake maps. I've used https://github.com/rustyrussell/ccan/ in the past and we can probably use their hash table implementation. Not sure for LPM maps, but maybe we can find an existing implementation or just do a very simple one which will be very inefficient but obviously correct (which I think is what we need for testing).

Great! I think this idea can work. Will look into it and revise the test.

@xinyuannn thank you! I will move this PR and also #17245 into draft until there is an update on this.