Skip to content

[Limiter] Add admin rule book endpoints and push to local cache #4691

Closed
tillrohrmann wants to merge 0 commit intorestatedev:mainfrom
tillrohrmann:issues/4655-4
Closed

[Limiter] Add admin rule book endpoints and push to local cache #4691
tillrohrmann wants to merge 0 commit intorestatedev:mainfrom
tillrohrmann:issues/4655-4

Conversation

@tillrohrmann
Copy link
Copy Markdown
Contributor

@tillrohrmann tillrohrmann commented May 5, 2026

Two batch endpoints write the cluster-global rule book through
read_modify_write, mirroring RuleBook::apply_changes so a request
either commits in full or leaves the book untouched:

  • PUT /limits/rules — body is a list of UpsertRuleRequests.
    Each entry carries a fully-specified rule body plus an optional
    Precondition (internally-tagged enum, defaults to none):

    • { "type": "none" } → unconditional upsert,
    • { "type": "matches", "version": v } → reject unless the
      rule's current version is v,
    • { "type": "does_not_exist" } → strict insert.
      Returns the post-batch state of every entry so callers chaining
      Matches(v) get the new versions without a second read.
  • POST /limits/rules/bulk-delete — body is a list of
    { pattern, expected_version: Option<u32> }. Missing version is
    an unconditional, idempotent delete; present version maps to
    Precondition::Matches(v). Returns the patterns this batch
    actually removed (idempotent no-ops are omitted).

Precondition is consumed directly by the request DTOs. To make that
possible, this commit also opts the limiter's serde and schema
features into deriving Serialize/Deserialize/ToSchema on
Precondition, and gives restate_types::Version a transparent
utoipa::ToSchema derive (#[schema(value_type = u32)]) under the
existing utoipa-schema feature, so the OpenAPI spec sees Version as
a plain integer.

When a worker role runs in the same process, both handlers also push
the freshly written book into the local RuleBookCache via a
fire-and-forget observer threaded down from the node wiring, shaving
the metadata-store poll latency.

This PR is based on #4690

Copy link
Copy Markdown

@claude claude Bot left a comment

Choose a reason for hiding this comment

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

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

@tillrohrmann
Copy link
Copy Markdown
Contributor Author

@codex review

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 22184cda80

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +89 to +90
"422".to_string(),
RefOr::Ref(Ref::from_response_name("UnprocessableEntity")),
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Define the referenced 422 response component

RulesApiError::responses now returns a $ref to UnprocessableEntity for HTTP 422, but the OpenAPI components list only registers BadRequest, NotFound, MethodNotAllowed, Conflict, and InternalServerError (crates/admin/src/rest_api/mod.rs). This creates a dangling response reference in the generated spec, which can break OpenAPI validation/codegen and hide the 422 contract for /rules endpoints; define and register a real UnprocessableEntity response (or inline it) before referencing it.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is a good catch. Will update it.

@tillrohrmann tillrohrmann force-pushed the issues/4655-4 branch 2 times, most recently from 91fee1a to 925e1fd Compare May 5, 2026 18:23
Comment thread crates/admin/src/rest_api/rules.rs Outdated
Comment on lines +163 to +182
/// Patch a rule.
///
/// JSON Merge Patch on `limits.action_concurrency`/`reason` (omit to
/// keep, `null` to clear, value to set) and `disabled` (omit to keep,
/// boolean to set). Pattern cannot be changed — delete and recreate.
#[utoipa::path(
patch,
path = "/limits/rules/{rule_id}",
operation_id = "update_rule",
tag = "rule",
params(
("rule_id" = String, Path, description = "Rule identifier (rul_…)"),
),
request_body = PatchRuleRequest,
responses(
(status = 200, description = "Rule updated", body = RuleResponse),
RulesApiError,
)
)]
pub async fn update_rule<Metadata, Discovery, Telemetry, Invocations, Transport>(
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I implemented the modification of rules using the PATCH method by providing a patch request. A different approach could have been to use the PUT method with providing the whole rule value.

For the latter approach we probably wouldn't have needed the RuleId. This would have left only the DELETE method as an open question for how to reference the rule that the user wants to delete.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This has changed. Now we are only offering a PUT handler which accepts a set of rules and preconditions.

@tillrohrmann tillrohrmann requested a review from AhmedSoliman May 5, 2026 18:26
@tillrohrmann
Copy link
Copy Markdown
Contributor Author

tillrohrmann commented May 5, 2026

This PR depends a little bit on the outcome of #4677 (comment). Right now it's build under the assumption that we have the RuleId which identifies stored rules uniquely. If we drop this bit then the look of the REST handlers will be a little bit different.

We settled on removing the RuleId. The PR has been updated accordingly.

@tillrohrmann
Copy link
Copy Markdown
Contributor Author

@nikrooz and @slinkydeveloper it would be great if you could give this PR a review. The important bit would be the shape of the REST API. Thanks a lot!

@tillrohrmann tillrohrmann force-pushed the issues/4655-4 branch 2 times, most recently from 6fb5197 to 8844c0b Compare May 7, 2026 13:07
Copy link
Copy Markdown
Contributor

@nikrooz nikrooz left a comment

Choose a reason for hiding this comment

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

Thanks Till, lgtm.
My only API-shape comment is that it might be nice to add a GET /limits/rules as the read side of these endpoints, so the REST API is complete. Otherwise clients that create/update/delete rules through REST have to discover current rules via the sys_rules SQL table, which feels a bit split-brain for a basic CRUD surface.

One small API feedback, when I first read this, I expected each rule to have an id that I’d use to update or delete it. It took me a bit to realise that pattern is the id. Looking at the comments, I think we had an id and decided to remove it for good reasons, so I don’t want to reopen that discussion, but I thought it was worth mentioning.

@tillrohrmann
Copy link
Copy Markdown
Contributor Author

tillrohrmann commented May 8, 2026

Thanks for the review @nikrooz.

My only API-shape comment is that it might be nice to add a GET /limits/rules as the read side of these endpoints, so the REST API is complete. Otherwise clients that create/update/delete rules through REST have to discover current rules via the sys_rules SQL table, which feels a bit split-brain for a basic CRUD surface.

We can certainly add this. Initially I thought that most of the read queries in the UI go through SQL and it gives more expressive filtering and sorting options. If the GET /limits/rules endpoint that returns all rules makes the UI integration nicer, then I'll add it.

One small API feedback, when I first read this, I expected each rule to have an id that I’d use to update or delete it. It took me a bit to realise that pattern is the id. Looking at the comments, I think we had an id and decided to remove it for good reasons, so I don’t want to reopen that discussion, but I thought it was worth mentioning.

Fair point. Initially we did have an explicit rule id. The reason why we removed it was because the pattern uniquely identifies the rule (there was a 1:1 mapping between the pattern and the rule id). Moreover, rules have a maximum size of 108 characters (usually they are a lot shorter). That's why we thought that the additional indirection of a rule id was not worth it. Open to discuss this decision if it makes a difference for how the UI interacts with the rules.

@nikrooz
Copy link
Copy Markdown
Contributor

nikrooz commented May 8, 2026

Thanks Till. For the UI the SQL table is preferred, my comment was more about the admin API as a standalone interface.

@tillrohrmann
Copy link
Copy Markdown
Contributor Author

Ah ok, then I'll defer this to when people are asking for it.

@github-actions github-actions Bot locked and limited conversation to collaborators May 8, 2026
@tillrohrmann tillrohrmann deleted the issues/4655-4 branch May 8, 2026 13:37
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants