Skip to content

Add proptest for testing RBAC permissions#2596

Open
fabubaker wants to merge 33 commits into
db_v4from
features/rbac-proptest
Open

Add proptest for testing RBAC permissions#2596
fabubaker wants to merge 33 commits into
db_v4from
features/rbac-proptest

Conversation

@fabubaker
Copy link
Copy Markdown
Contributor

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?

@fabubaker fabubaker force-pushed the features/rbac-proptest branch from a7a7cd6 to 03853c7 Compare May 5, 2026 15:16
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ 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
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test_permissions is moved to pometry_storage

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably, we reuse the same server for all iterations, cleaning up before next iteration or use OS-assigned port per iteration?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!(
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!(
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing:

assert!(
    can_discover_namespace(path, client).unwrap(),
    "namespace {path} should be discoverable"
);

);
}
Permission::Read => {
assert!(
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing:

assert!(
    can_discover_namespace(path, client).unwrap(),
    "namespace {path} should be discoverable"
);

}
Permission::Write => {
assert!(
can_introspect_namespace(path, children, client).unwrap(),
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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}"
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants