Simplify istio rate limits by nicu-da · Pull Request #6933 · canton-network/splice

nicu-da · 2026-08-24T13:21:32Z

By default only have a global rate limit

[static]

fixes https://github.com/DACH-NY/canton-network-internal/issues/2115

Pull Request Checklist

Cluster Testing

If a cluster test is required, comment /cluster_test on this PR to request it, and ping someone with access to the DA-internal system to approve it.
If an upgrade test is required, comment /upgrade_test on this PR to request it, and ping someone with access to the DA-internal system to approve it.
If a hard-migration test is required (from the latest release), comment /hdm_test on this PR to request it, and ping someone with access to the DA-internal system to approve it.
If a logical synchronizer upgrade test is required (from canton-3.5), comment /lsu_test on this PR to request it, and ping someone with access to the DA-internal system to approve it.

PR Guidelines

Include any change that might be observable by our partners or affect their deployment in the release notes.
Specify fixed issues with Fixes #n, and mention issues worked on using #n
Include a screenshot for frontend-related PRs - see README or use your favorite screenshot tool

Merge Guidelines

Make the git commit message look sensible when squash-merging on GitHub (most likely: just copy your PR description).

canton-network-da · 2026-08-24T13:21:52Z

[backport] Reminder

Please consider backporting to the following branches:

release-line-0.7.4
release-line-0.7.3
release-line-0.7.1
release-line-0.7.0

▶️ Please check the boxes for branches that you wish to backport to and backport PRs will
automatically be created when you merge this PR.

And your PR is currently against base branch: main.

Note: Any PR comment containing [backport] will be considered for auto-backporting upon merge,
you can always add those manually for PRs that did not get these reminders. You can also edit
this comment manually and add more branches that this should be backported to.

By default only have a global rate limit [static] Signed-off-by: Nicu Reut <nicu.reut@digitalasset.com>

martinflorian-da

Thanks!

martinflorian-da · 2026-08-26T08:46:38Z

  scan:
    externalRateLimits:
      !include(./rate-limits/v0-acs.yaml;./rate-limits/unlimited.yaml;./rate-limits/public-banned.yaml;./rate-limits/token-registry.yaml)
+      enablePerEndpointRateLimits: false


So the idea is to keep the functionality around in case we want to use it after all, at least for our deployments?

Yes, pretty much, as a safety precaution

martinflorian-da · 2026-08-26T08:50:00Z

  });
 });

+test('buildGlobalPerIpRateLimitAction keys only on the non-spoofable client address', () => {


what does "keys" even mean in this context 😕

the key of the attribute used for the ip basically. making sure we don't change something unintentionally to not use the masked address

martinflorian-da · 2026-08-26T08:54:55Z

+
+  globalPerIpLimits?: Limits;
+
+  enablePerEndpointRateLimits?: boolean;


Hm can we just skip this flag and simply leave the per-endpoint-rate limits unset if we don't want these?

Having something explicitly set in a config that is actually unused because of a boolean flag somewhere seems a bit meh.

Agree, issue was that it would've made the PR more complex. Let me do it though

[static] Signed-off-by: Nicu Reut <nicu.reut@digitalasset.com>

nicu-da · 2026-08-26T12:24:00Z

/cluster_test

github-actions · 2026-08-26T12:24:23Z

Deploy cluster test triggered for Commit f6558566cd2a4ccab237245cc9198a1f88c5a87c in , please contact a Contributor to approve it in CircleCI: https://app.circleci.com/pipelines/github/DACH-NY/canton-network-internal/79853

martinflorian-da

Removes more LOC than it adds, must be good. Thank you!

martinflorian-da · 2026-08-26T14:18:11Z

-    tokensPerFill: 500
-    fillInterval: 60s
-    perIpLimits: # per-IP limit
-      maxTokens: 10


Talking to myself: this should be fine:

splice/cluster/images/scan-app/app.conf

Line 78 in 9af4459

getAcsSnapshot.rate-per-second = 20

martinflorian-da · 2026-08-26T14:18:58Z

  scan:
    externalRateLimits:
-      !include(./rate-limits/v0-acs.yaml;./rate-limits/unlimited.yaml;./rate-limits/public-banned.yaml;./rate-limits/token-registry.yaml)
+      !include(./rate-limits/public-banned.yaml;./rate-limits/token-registry.yaml)


For token registry - is the medium-term to remove those as well in favor of relying only on the splice rate limits? (Shall we add a TODO if so?)

Let me create an issue to revisit right before opening it up.
The idea is to remove it unless we explicitly want some higher limits for that part of the API

higher limits for that part of the API

Can we do actually do that? Successful requests will not deplete the global and default per-IP buckets?

no, the first match consumes the limit, the global one is basically the "default" and used if nothing else matches
so we can sort of partition out certain parts of the API

martinflorian-da · 2026-08-26T14:22:40Z

  const effectiveRateLimits = Object.fromEntries(
    Object.entries(args.rateLimits || {}).filter(
      (ent): ent is [string, LocalLimit<MatchedLimits>] => {
        // TODO (#4201): in banned case, implement actual banning with special short-circuit for whitelisted IPs


Do we need to explicitly track this as nice-to-have for MainNet opening?

tbh not sure if it's even worth doing.
we can just set the limit to 0 in the splice rate limiter, as we need to ship it to everyone anyway

IMHO that would be a valid way to resolve this, but AFAIU banned currently is a NOP, which can mislead us in the future.

good point, let me just delete the banned stuff

[ci] Signed-off-by: Nicu Reut <nicu.reut@digitalasset.com>

nicu-da · 2026-08-27T08:18:33Z

/cluster_test

github-actions · 2026-08-27T08:18:59Z

Deploy cluster test triggered for Commit 5f2d2f663b5607798ab45572ec5ee8ddb0c16996 in , please contact a Contributor to approve it in CircleCI: https://app.circleci.com/pipelines/github/DACH-NY/canton-network-internal/79996

[static ] Signed-off-by: Nicu Reut <nicu.reut@digitalasset.com>

[static] Signed-off-by: Nicu Reut <nicu.reut@digitalasset.com>

nicu-da · 2026-08-27T10:00:43Z

/cluster_test

github-actions · 2026-08-27T10:01:06Z

Deploy cluster test triggered for Commit aedd3997ef7faf42b5017581c1348da90ea943aa in , please contact a Contributor to approve it in CircleCI: https://app.circleci.com/pipelines/github/DACH-NY/canton-network-internal/80012

Simplify istio rate limits

8b03586

By default only have a global rate limit [static] Signed-off-by: Nicu Reut <nicu.reut@digitalasset.com>

nicu-da force-pushed the nicu/rate_limits/simplify_istio branch from 2edaaeb to 8b03586 Compare August 24, 2026 13:36

nicu-da marked this pull request as ready for review August 26, 2026 08:29

nicu-da requested a review from martinflorian-da August 26, 2026 08:29

martinflorian-da approved these changes Aug 26, 2026

View reviewed changes

remove config flag and use just list of rate limits

f655856

[static] Signed-off-by: Nicu Reut <nicu.reut@digitalasset.com>

nicu-da requested a review from martinflorian-da August 26, 2026 12:23

martinflorian-da approved these changes Aug 26, 2026

View reviewed changes

.

5f2d2f6

[ci] Signed-off-by: Nicu Reut <nicu.reut@digitalasset.com>

nicu-da added 2 commits August 27, 2026 09:58

remove the banned stuff

3598659

[static ] Signed-off-by: Nicu Reut <nicu.reut@digitalasset.com>

remove the banned stuff

aedd399

[static] Signed-off-by: Nicu Reut <nicu.reut@digitalasset.com>


		globalPerIpLimits?: Limits;

		enablePerEndpointRateLimits?: boolean;

Uh oh!

Conversation

nicu-da commented Aug 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Checklist

Cluster Testing

PR Guidelines

Merge Guidelines

Uh oh!

canton-network-da commented Aug 24, 2026

[backport] Reminder

Please consider backporting to the following branches:

Uh oh!

martinflorian-da left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicu-da commented Aug 26, 2026

Uh oh!

github-actions Bot commented Aug 26, 2026

Uh oh!

martinflorian-da left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicu-da commented Aug 27, 2026

Uh oh!

github-actions Bot commented Aug 27, 2026

Uh oh!

nicu-da commented Aug 27, 2026

Uh oh!

github-actions Bot commented Aug 27, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

nicu-da commented Aug 24, 2026 •

edited

Loading