Skip to content

datastore: allow pagination tokens larger than 32 bits - #7201

Open
pujitha24 wants to merge 3 commits into
spiffe:mainfrom
pujitha24:auto/issue-5501
Open

datastore: allow pagination tokens larger than 32 bits#7201
pujitha24 wants to merge 3 commits into
spiffe:mainfrom
pujitha24:auto/issue-5501

Conversation

@pujitha24

Copy link
Copy Markdown
Contributor

I have all the details needed from the commit. Here's the filled template:


Pull Request check list

  • Commit conforms to CONTRIBUTING.md?
  • Proper tests/regressions included?
  • Documentation updated?

Affected functionality

Pagination token parsing in the SQL datastore (pkg/server/datastore/sqlstore/sqlstore.go), used when listing records (e.g. bundles) with pagination.

Description of change

Pagination tokens are parsed with strconv.ParseUint(token, 10, 32) in four places in pkg/server/datastore/sqlstore/sqlstore.go, even though the underlying row ID is a 64-bit uint (and, for CockroachDB users, an actual bigint populated via unique_rowid()). ParseUint always returns a uint64 regardless of bitSize, so the 32 argument only restricts which input values are accepted; it does not reflect any real limitation of the ID column.

As a result, any pagination token above math.MaxUint32 (4294967295) is rejected with "could not parse token '<token>'", even when SPIRE itself generated that token from a real row ID. This is not hypothetical: a comment on the linked issue reports it occurring with CockroachDB's unique_rowid(), and Postgres/MySQL sequences can also realistically grow past 4 billion over the life of a long-running deployment.

This change widens all four ParseUint call sites from bitSize 32 to bitSize 64 to match the actual ID type. This is a strict superset of previously accepted values, so it is backward compatible with existing tokens.

Note this does not address the primary-key-sequence-overflow problem originally reported in the linked issue (migrating int primary keys to bigint is a separate, larger schema-migration change and is out of scope here). It only fixes the narrower bug where an otherwise-valid large pagination token is rejected.

Validation:

go build ./...
go test ./pkg/server/datastore/...

Both pass, including a new regression test case ("token larger than 32 bits") added to TestListBundlesWithPagination in pkg/server/datastore/sqltest/datastore_suite.go. Confirmed the new test fails on the pre-fix code with "could not parse token '5000000000'" and passes after the fix.

Which issue this PR fixes

fixes #5501


AI assistance: this change was drafted with Claude Code.

Motivation:

Pagination tokens are parsed with strconv.ParseUint(token, 10, 32) in
four places in pkg/server/datastore/sqlstore/sqlstore.go, even though
the underlying row ID is a 64-bit uint (and, for CockroachDB users, an
actual bigint populated via unique_rowid()). ParseUint always returns
a uint64 regardless of bitSize, so the 32 argument only restricts
which input values are accepted; it does not reflect any real
limitation of the ID column.

As a result, any pagination token above math.MaxUint32
(4294967295) is rejected with "could not parse token '<token>'", even
when SPIRE itself generated that token from a real row ID. This is
not hypothetical: a comment on the linked issue reports it occurring
with CockroachDB's unique_rowid(), and Postgres/MySQL sequences can
also realistically grow past 4 billion over the life of a long-running
deployment.

Note this does not address the primary-key-sequence-overflow problem
originally reported in the linked issue (migrating int primary keys
to bigint is a separate, larger schema-migration change and is out of
scope here). It only fixes the narrower bug where an otherwise-valid
large pagination token is rejected.

Approach:

Widen all four ParseUint call sites from bitSize 32 to bitSize 64 to
match the actual ID type. This is a strict superset of previously
accepted values, so it is backward compatible with existing tokens.

Validation:

  go build ./...
  go test ./pkg/server/datastore/...

Both pass, including a new regression test case ("token larger than
32 bits") added to TestListBundlesWithPagination in
pkg/server/datastore/sqltest/datastore_suite.go. Confirmed the new
test fails on the pre-fix code with "could not parse token
'5000000000'" and passes after the fix.

Report: spiffe#5501
Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This pull request fixes a pagination edge case in SPIRE Server’s SQL datastore by allowing pagination tokens to represent full-width row IDs, avoiding erroneous InvalidArgument failures once IDs exceed 32-bit values.

Changes:

  • Widened pagination token parsing from 32-bit to 64-bit in all affected SQL datastore query builders and the shared pagination helper.
  • Added a regression test ensuring large (32-bit+) pagination tokens no longer fail parsing for bundle listing.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
pkg/server/datastore/sqlstore/sqlstore.go Expands pagination token parsing to 64-bit at all call sites so large row IDs can be used as tokens.
pkg/server/datastore/sqltest/datastore_suite.go Adds regression coverage for listing bundles with a pagination token larger than 32 bits.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

The new "token larger than 32 bits" case fails CI on the
datastore-postgres and datastore-postgres-replication integration
suites: postgres itself rejects the literal 5000000000 as
out-of-range for its 32-bit integer ID column before our widened
ParseUint ever runs. This scenario can't occur on real postgres
deployments (the column can't hold an ID that large), so skip it
for that dialect; sqlite and mysql still exercise it.

Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>
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.

Database primary key reached maximum value of sequence

3 participants