Skip to content

fix(redis-schema): copy policy schemas instead of mutating shared tables#13555

Open
janiussyafiq wants to merge 3 commits into
apache:masterfrom
janiussyafiq:fix/redis-schema-shared-tables
Open

fix(redis-schema): copy policy schemas instead of mutating shared tables#13555
janiussyafiq wants to merge 3 commits into
apache:masterfrom
janiussyafiq:fix/redis-schema-shared-tables

Conversation

@janiussyafiq

@janiussyafiq janiussyafiq commented Jun 12, 2026

Copy link
Copy Markdown
Contributor

Description

apisix/utils/redis-schema.lua builds limit-conn's key_ttl schema variants by mutating the shared policy_to_additional_properties tables in place:

local limit_conn_redis_cluster_schema = policy_to_additional_properties["redis-cluster"]
limit_conn_redis_cluster_schema.properties.key_ttl = {
    type = "integer", default = 3600,
}

Since this runs at module load, key_ttl (and its default of 3600) leaks into the base redis / redis-cluster tables that every other consumer of this module reuses. Observable effect today: limit-count and limit-req confs silently gain a key_ttl = 3600 field during schema validation (defaults are applied to the conf), even though neither plugin understands the property. limit-count's core.table.deepcopy(redis_schema.schema) doesn't help — it copies the already-mutated tables.

This PR builds the limit-conn variants from a shallow copy of the shared base, so the extra property stays local to limit-conn:

  • the shared redis / redis-cluster tables no longer carry key_ttl
  • limit-conn behavior is unchanged (key_ttl still accepted, default 3600 still applied)
  • limit-count / limit-req confs no longer get a foreign key_ttl injected; a stray explicit key_ttl on those plugins is still tolerated (their schemas do not reject additional properties) — it is just no longer injected

New regression test t/utils/redis-schema.t (the existing home for apisix/utils/* tests, alongside batch-processor.t / rfc5424.t): one block validates limit-count/limit-req confs across redis and redis-cluster policies — with limit-conn loaded first so any shared-table leak would be visible — and asserts no key_ttl is injected; a second block pins limit-conn's own behavior (default 3600 applied, explicit value honored). On unfixed master the first block fails with limit-count redis got key_ttl: 3600 (and the three other combinations); with the fix all cases pass.

This also unblocks new consumers of redis-schema.

Which issue(s) this PR fixes:

None (pre-existing latent bug found while building on redis-schema).

Checklist

  • I have explained the need for this PR and the problem it solves
  • I have explained the changes or the new features added to this PR
  • I have added tests corresponding to this change
  • I have updated the documentation to reflect this change (no doc change needed: internal schema utility; user-facing behavior of limit-conn unchanged)
  • I have verified that this change is backward compatible

@dosubot dosubot Bot added size:L This PR changes 100-499 lines, ignoring generated files. bug Something isn't working labels Jun 12, 2026
limit-conn's key_ttl schema variants were built by mutating the shared
policy_to_additional_properties tables in place, leaking key_ttl (and
its default of 3600) into every other consumer of
apisix/utils/redis-schema.lua: limit-count and limit-req confs silently
gained a key_ttl = 3600 field during validation.

Build the limit-conn variants from a shallow copy of the shared base so
the extra property stays local to limit-conn. A stray explicit key_ttl
on limit-count/limit-req is still tolerated (their schemas do not
reject additional properties); it is just no longer injected.
@janiussyafiq janiussyafiq force-pushed the fix/redis-schema-shared-tables branch from e0b73a9 to 64795a2 Compare June 12, 2026 17:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working size:L This PR changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant