Skip to content

test(load): jwt loadtests with complex jwt-role-claim-key #5013

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
taimoorzaeem wants to merge 1 commit into from
Closed

test(load): jwt loadtests with complex jwt-role-claim-key #5013

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion nix/tools/generate_targets.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ def generate_jwt(
payload = {
"sub": f"user_{random.getrandbits(32)}",
"iat": now,
"role": "postgrest_test_author",
"postgrest": {"roles": ["postgrest_test_author", "other"]},
}

if exp_inc is not None:
Expand Down
1 change: 1 addition & 0 deletions nix/tools/loadtest.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ let
export PGRST_DB_TX_END="rollback-allow-override"
export PGRST_LOG_LEVEL="crit"
export PGRST_JWT_SECRET="reallyreallyreallyreallyverysafe"
export PGRST_JWT_ROLE_CLAIM_KEY=".postgrest.roles[?(@ ^== \"postgrest\")]"

@taimoorzaeem taimoorzaeem Jun 17, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

In #4984, this will be replaced by $.postgrest.roles[?search(@, '^postgrest')]. We can then compare the load test results.

@wolfgangwalther wolfgangwalther Jun 17, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We can then compare the load test results.

Not really, I think. To compare loadtest results for #4984, the loadtest in question must be supported by both the old and the new version. It's always the loadtest code from the head branch that runs against the older branch.

We'd need to be a bit smarter than that, I'm afraid.

@taimoorzaeem taimoorzaeem Jun 17, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

To compare loadtest results for #4984, the loadtest in question must be supported by both the old and the new version. It's always the loadtest code from the head branch that runs against the older branch.

Wait, I don't understand. Once this gets merged, we'll have this on main branch.

I'll then update it #4984 so the PR branch will compare to main i.e comparing .postgrest.roles[?(@ ^== \"postgrest\")] with $.postgrest.roles[?search(@, '^postgrest')]. What am I missing?

@wolfgangwalther wolfgangwalther Jun 17, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The loadtest consists of two components:

  • All the nix-based tooling. This is always taken from the head branch, because this is the script you're running with postgrest-loadtest-against and such.
  • The postgrest executable, which is built via nix on each tested branch.

This means that the configuration that PostgREST is started with comes from the head branch, not the main branch. In #4984, you will run both executables with $.postgrest.roles[?search(@, '^postgrest')] - and that will just fail for the main branch. For example see 4f9bc89, which removed some of the old fallback configuration that we had to keep in place to be able to compare loadtests against older branches, which still required some configuration options.

With how things are currently set up, it would be easiest to do a manual loadtest locally, where you run postgrest-loadtest (not postgrest-loadtest-against) on both branches, where each branch has the respective configuration option set, and then run the reporting script manually.

@taimoorzaeem taimoorzaeem Jun 18, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This means that the configuration that PostgREST is started with comes from the head branch, not the main branch.

Oh I see now. This explains it. Thanks for the clarification.

@steve-chavez steve-chavez Jun 18, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This means that the configuration that PostgREST is started with comes from the head branch, not the main branch. In #4984, you will run both executables with $.postgrest.roles[?search(@, '^postgrest')] - and that will just fail for the main branch

@wolfgangwalther Why did we did that? Was it to be able to run the loadtests on older versions?

It is confusing and IIRC @robx was also confused before somewhere, so I'm wondering if we can switch to running the Nix tooling from head and main branch?

@wolfgangwalther wolfgangwalther Jun 21, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why did we did that? Was it to be able to run the loadtests on older versions?

That was one of the reasons, yes: We needed to do it this way, otherwise there was no way to run the loadtests against older commits which did not have the loadtests introduced, yet.

But that's only one reason. There are other very good reasons to do it like this: Once you start using loadtest scripts themselves from older commits, you will create a maintenance headache: Suppose you fix a bug in these scripts or make some other significant change to them. You can't get any meaningful results from running these against an older commit anymore, because you will be stuck on that older commit.

You might even end up comparing test runs with different versions of certain tools, for example vegeta itself etc.

I believe it creates a lot more problems and pitfalls to do it that way. The case we're having right now should really be the exception anyway, so I don't see a need to change anything here.

@steve-chavez steve-chavez Jun 22, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Added the above as a comment on the command ded9be6


mkdir -p "$(dirname "$_arg_output")"
abs_output="$(realpath "$_arg_output")"
Expand Down