Add proptest for testing RBAC permissions#2596
Conversation
a7a7cd6 to
03853c7
Compare
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark 'Rust Benchmark'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 2.
| Benchmark suite | Current: 03853c7 | Previous: 9823ef7 | Ratio |
|---|---|---|---|
lotr_graph/num_edges |
5 ns/iter (± 0) |
0 ns/iter (± 0) |
+∞ |
lotr_graph/num_nodes |
126 ns/iter (± 32) |
1 ns/iter (± 0) |
126 |
lotr_graph/graph_latest |
3 ns/iter (± 0) |
0 ns/iter (± 0) |
+∞ |
lotr_graph_materialise/materialize |
7039156 ns/iter (± 23250) |
1564816 ns/iter (± 35303) |
4.50 |
lotr_graph_window_100/num_edges |
29 ns/iter (± 0) |
8 ns/iter (± 0) |
3.63 |
lotr_graph_window_100/num_nodes |
140 ns/iter (± 34) |
5 ns/iter (± 0) |
28 |
lotr_graph_window_100_materialise/materialize |
7427428 ns/iter (± 26556) |
1669150 ns/iter (± 10700) |
4.45 |
lotr_graph_window_10/has_node_existing |
147 ns/iter (± 9) |
62 ns/iter (± 11) |
2.37 |
lotr_graph_window_10/iterate nodes |
36569 ns/iter (± 172) |
11339 ns/iter (± 40) |
3.23 |
lotr_graph_window_10/iterate edges |
101521 ns/iter (± 189) |
48684 ns/iter (± 211) |
2.09 |
lotr_graph_window_10_materialise/materialize |
3088293 ns/iter (± 18110) |
971980 ns/iter (± 4278) |
3.18 |
lotr_graph_subgraph_10pc/has_edge_existing |
261 ns/iter (± 4) |
93 ns/iter (± 1) |
2.81 |
lotr_graph_subgraph_10pc/num_nodes |
123 ns/iter (± 27) |
4 ns/iter (± 0) |
30.75 |
lotr_graph_subgraph_10pc/has_node_existing |
118 ns/iter (± 0) |
34 ns/iter (± 0) |
3.47 |
lotr_graph_subgraph_10pc/iterate nodes |
2613 ns/iter (± 88) |
839 ns/iter (± 5) |
3.11 |
lotr_graph_subgraph_10pc_materialise/materialize |
1568204 ns/iter (± 13207) |
334634 ns/iter (± 1287) |
4.69 |
lotr_graph_subgraph_10pc_windowed/has_node_existing |
139 ns/iter (± 7) |
62 ns/iter (± 14) |
2.24 |
lotr_graph_subgraph_10pc_windowed/iterate nodes |
4930 ns/iter (± 97) |
1365 ns/iter (± 3) |
3.61 |
lotr_graph_subgraph_10pc_windowed_materialise/materialize |
945183 ns/iter (± 10472) |
230399 ns/iter (± 2617) |
4.10 |
lotr_graph_window_50_layered/num_edges_temporal |
154077 ns/iter (± 2292) |
70121 ns/iter (± 7586) |
2.20 |
lotr_graph_window_50_layered/num_nodes |
58418 ns/iter (± 1169) |
21435 ns/iter (± 536) |
2.73 |
lotr_graph_window_50_layered/has_node_existing |
786 ns/iter (± 74) |
129 ns/iter (± 12) |
6.09 |
lotr_graph_window_50_layered/max_id |
65171 ns/iter (± 2846) |
25556 ns/iter (± 252) |
2.55 |
lotr_graph_window_50_layered/iterate nodes |
122429 ns/iter (± 414) |
19308 ns/iter (± 47) |
6.34 |
lotr_graph_window_50_layered/iterate edges |
197698 ns/iter (± 421) |
83616 ns/iter (± 1318) |
2.36 |
lotr_graph_window_50_layered/graph_latest |
95150 ns/iter (± 1272) |
36649 ns/iter (± 916) |
2.60 |
lotr_graph_window_50_layered_materialise/materialize |
27991179 ns/iter (± 69479) |
3488825 ns/iter (± 24948) |
8.02 |
lotr_graph_persistent_window_50_layered/num_edges_temporal |
675615 ns/iter (± 13087) |
192686 ns/iter (± 1569) |
3.51 |
lotr_graph_persistent_window_50_layered/num_nodes |
74334 ns/iter (± 1664) |
31517 ns/iter (± 779) |
2.36 |
lotr_graph_persistent_window_50_layered/has_node_existing |
934 ns/iter (± 198) |
174 ns/iter (± 83) |
5.37 |
lotr_graph_persistent_window_50_layered/max_id |
82114 ns/iter (± 1304) |
38024 ns/iter (± 490) |
2.16 |
lotr_graph_persistent_window_50_layered/iterate nodes |
159392 ns/iter (± 415) |
35886 ns/iter (± 191) |
4.44 |
lotr_graph_persistent_window_50_layered/iterate edges |
190222 ns/iter (± 580) |
84161 ns/iter (± 596) |
2.26 |
lotr_graph_persistent_window_50_layered/iterate_exploded_edges |
4415761 ns/iter (± 9181) |
1659940 ns/iter (± 19402) |
2.66 |
lotr_graph_persistent_window_50_layered/graph_latest |
141621 ns/iter (± 821) |
57549 ns/iter (± 4809) |
2.46 |
lotr_graph_persistent_window_50_layered_materialise/materialize |
47382628 ns/iter (± 63586) |
5298035 ns/iter (± 147912) |
8.94 |
lotr_graph/proto_encode |
8658464 ns/iter (± 64508) |
1157897 ns/iter (± 73709) |
7.48 |
This comment was automatically generated by workflow using github-action-benchmark.
| @@ -1288,6 +1288,53 @@ def test_child_namespace_restriction_overrides_parent(): | |||
| assert "team/restricted/secret" in paths | |||
There was a problem hiding this comment.
test_permissions is moved to pometry_storage
There was a problem hiding this comment.
Probably also makes sense to move the rbac prop tests as well to pometry-storage
| ProptestConfig::with_cases(PROPTEST_CASES), | ||
| |(case in permissions_strategy(NAMESPACE_SIZE, NUM_GRANTS, NUM_USERS))| { | ||
| let total_graphs = case.graph_paths.len() as u64; | ||
| let (_server, _tempdir) = start_server(PORT, PUB_KEY, total_graphs); |
There was a problem hiding this comment.
The server is never explicitly stopped between iterations. _server is just dropped, which aborts the background task without sending a shutdown signal. The OS socket is left open, so the next iteration's start_server call might fail to bind port 43871 with "address already in use". The abort via drop is fast enough that the OS usually reclaims the socket before the next iteration tries to bind but it's a race condition. Under load, a slow CI machine, or with OS scheduling delays, the socket won't be released in time and the test will fail.
There was a problem hiding this comment.
Probably, we reuse the same server for all iterations, cleaning up before next iteration or use OS-assigned port per iteration?
There was a problem hiding this comment.
Also we don't seem to be cleaning up the _tempdir. Leaves directories on disk permanently.
| match permission { | ||
| Some(permission_enum) => match permission_enum { | ||
| Permission::Discover => { | ||
| assert!( |
There was a problem hiding this comment.
missing:
assert!(
!can_introspect_namespace(path, children, client).unwrap(),
"namespace {path} should not be introspectable"
);
assert!(
!can_write_namespace(path, client).unwrap(),
"namespace {path} should not be writable"
);
| ); | ||
| } | ||
| Permission::Introspect => { | ||
| assert!( |
There was a problem hiding this comment.
missing:
assert!(
can_discover_namespace(path, client).unwrap(),
"namespace {path} should be discoverable"
);
| ); | ||
| } | ||
| Permission::Read => { | ||
| assert!( |
There was a problem hiding this comment.
missing:
assert!(
can_discover_namespace(path, client).unwrap(),
"namespace {path} should be discoverable"
);
| } | ||
| Permission::Write => { | ||
| assert!( | ||
| can_introspect_namespace(path, children, client).unwrap(), |
There was a problem hiding this comment.
missing:
assert!(
can_discover_namespace(path, client).unwrap(),
"namespace {path} should be discoverable"
);
| return Err(ClientError::GraphQLErrors(format!( | ||
| "After sending query to the server:\n\t{}\nGot the following errors:\n\t{}", | ||
| query, message | ||
| "Error while executing query: {message}" |
There was a problem hiding this comment.
This makes debugging harder. When a test fails you no longer know which query caused the error. Worth keeping the query in the message, even if reformatted.
What changes were proposed in this pull request?
Why are the changes needed?
Does this PR introduce any user-facing change? If yes is this documented?
How was this patch tested?
Are there any further changes required?