fix: Optimized IP whitelist string conversion to avoid redundant processing on every verification#4063
Conversation
… from the hot path to initialization phase issue-unkeyed#3544
Akhileshait
requested review from
Flo4604,
chronark,
imeyer and
perkinsjr
as code owners
|
@Akhileshait is attempting to deploy a commit to the Unkey Team on Vercel. A member of the Team first needs to authorize it. |
|
|
|
|
Thank you for following the naming conventions for pull request titles! 🙏 |
📝 WalkthroughWalkthroughPreparse and cache key IP whitelists by introducing Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant KeysService
participant Cache
participant DB
participant KeyVerifier
Client->>KeysService: request with API key
KeysService->>Cache: lookup key by hash
alt cache miss
KeysService->>DB: load FindKeyForVerificationRow
DB-->>KeysService: row
KeysService->>KeysService: parse IpWhitelist -> map[string]struct{}
KeysService->>Cache: store db.CachedKeyData{row, ParsedIPWhitelist}
KeysService->>KeyVerifier: construct verifier(with parsedIPWhitelist)
else cache hit
Cache-->>KeysService: db.CachedKeyData
KeysService->>KeyVerifier: construct verifier(use cached parsedIPWhitelist)
end
KeysService->>KeyVerifier: Verify(request)
alt parsedIPWhitelist empty
KeyVerifier->>KeysService: skip IP check
else
KeyVerifier->>KeyVerifier: O(1) lookup client IP in parsedIPWhitelist
alt found
KeyVerifier->>KeysService: allow
else
KeyVerifier->>KeysService: forbid
end
end
KeysService->>Client: response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)go/internal/services/keys/get.go (3)
🔇 Additional comments (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
go/internal/services/keys/get.go (1)
176-180: Consider filtering empty strings from malformed input.Malformed whitelist entries (e.g., trailing commas or consecutive commas like
"1.2.3.4,,5.6.7.8") will create empty strings in the parsed slice. While harmless (empty strings won't match real IPs), filtering them out improves data quality.Apply this diff to filter empty entries:
- ips := strings.Split(key.IpWhitelist.String, ",") - parsedIPWhitelist = make([]string, len(ips)) - for i, ip := range ips { - parsedIPWhitelist[i] = strings.TrimSpace(ip) + ips := strings.Split(key.IpWhitelist.String, ",") + parsedIPWhitelist = make([]string, 0, len(ips)) + for _, ip := range ips { + trimmed := strings.TrimSpace(ip) + if trimmed != "" { + parsedIPWhitelist = append(parsedIPWhitelist, trimmed) + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
go/internal/services/keys/get.go(2 hunks)go/internal/services/keys/validation.go(2 hunks)go/internal/services/keys/verifier.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
go/internal/services/keys/get.go (1)
go/internal/services/keys/status.go (1)
StatusValid(15-15)
🔇 Additional comments (4)
go/internal/services/keys/verifier.go (1)
42-43: LGTM! Field addition improves verification performance.The new
parsedIPWhitelistfield enables pre-parsing of the IP whitelist during key loading, eliminating redundant string operations on every verification call.go/internal/services/keys/validation.go (2)
60-62: LGTM! Correctly uses pre-parsed whitelist.The empty check properly handles both nil and empty slices, maintaining the original behavior while eliminating runtime string parsing.
71-71: LGTM! Simplified membership check.Using
slices.Contains()on the pre-parsed list is cleaner and more efficient than the previous string split and iteration approach.go/internal/services/keys/get.go (1)
197-202: LGTM! Initialization correctly includes pre-parsed whitelist.The
parsedIPWhitelistfield is properly initialized alongside existing fields, enabling the optimized validation path.
Akhileshait
changed the title
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
go/internal/services/keys/get.go (1)
7-7: LGTM! Resolves the previous critical issue.The
stringsimport has been correctly added and is necessary forstrings.Split(line 177) andstrings.TrimSpace(line 180).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
go/internal/services/keys/get.go(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
go/internal/services/keys/get.go (1)
go/internal/services/keys/status.go (1)
StatusValid(15-15)
🔇 Additional comments (1)
go/internal/services/keys/get.go (1)
198-203: LGTM! Achieves the optimization goal.The
parsedIPWhitelistfield is correctly initialized with the pre-parsed IP slice, ensuring the whitelist is parsed once during key loading rather than on every verification. This eliminates redundant string operations in the hot path as intended.
|
that looks fine to me |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
@Akhileshait can you please run |
|
@Flo4604 I have removed unused imports please run it now. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
go/internal/services/keys/validation.go (1)
69-71: Skip empty entries and normalize IPs in whitelist parsingIn go/internal/services/keys/get.go, after splitting and trimming each IP:
- Add
if trimmed == "" { continue }to skip empty entries- Use
net.ParseIP(trimmed).String()for canonical IPv4/IPv6 formatting before storing inparsedIPWhitelistThis ensures no empty keys and consistent matching for IPv6 addresses.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
go/internal/services/keys/validation.go(2 hunks)
🔇 Additional comments (1)
go/internal/services/keys/validation.go (1)
58-60: LGTM! Efficient empty-whitelist check.Using
len(k.parsedIPWhitelist) == 0correctly handles both nil and empty maps, maintaining the same "no whitelist configured" semantics while enabling the performance optimization.
The tests are still failing, you can run Otherwise If you have any questions regarding the setup please do not hesitate to ask Please also fix the merge conflicts that popped up. |
…optimize-IPwhitelist-issue-unkeyed#3544
…optimize-IPwhitelist-issue-unkeyed#3544
…/github.com/Akhileshait/unkey into fix-optimize-IPwhitelist-issue-unkeyed#3544
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
go/internal/services/keys/get.go (1)
131-140: Consider inlining the workspace-disabled case.The early initialization at lines 131-135 creates a
KeyVerifierthat is discarded if the workspace is enabled (line 194 creates a new instance). While this pattern avoids duplication, it's slightly inefficient.Consider inlining the initialization for the workspace-disabled case:
- kv := &KeyVerifier{ - Status: StatusWorkspaceDisabled, - message: "workspace is disabled", - region: s.region, - } - if !key.WorkspaceEnabled || (key.ForWorkspaceEnabled.Valid && !key.ForWorkspaceEnabled.Bool) { + kv := &KeyVerifier{ + Status: StatusWorkspaceDisabled, + message: "workspace is disabled", + region: s.region, + } // nolint:exhaustruct return kv, kv.log, nil }This avoids allocating a
KeyVerifierthat will be discarded in the common case where the workspace is enabled.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
go/apps/api/routes/v2_keys_add_permissions/handler.go(1 hunks)go/apps/api/routes/v2_keys_add_roles/handler.go(1 hunks)go/apps/api/routes/v2_keys_delete_key/handler.go(1 hunks)go/apps/api/routes/v2_keys_remove_permissions/handler.go(1 hunks)go/apps/api/routes/v2_keys_remove_roles/handler.go(1 hunks)go/apps/api/routes/v2_keys_set_permissions/handler.go(1 hunks)go/apps/api/routes/v2_keys_set_roles/handler.go(1 hunks)go/apps/api/routes/v2_keys_update_credits/handler.go(1 hunks)go/apps/api/routes/v2_keys_update_key/handler.go(1 hunks)go/internal/services/keys/get.go(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (10)
go/apps/api/routes/v2_keys_remove_roles/handler.go (1)
go/pkg/db/cached_key_data.go (1)
CachedKeyData(5-8)
go/apps/api/routes/v2_keys_delete_key/handler.go (1)
go/pkg/db/cached_key_data.go (1)
CachedKeyData(5-8)
go/apps/api/routes/v2_keys_set_permissions/handler.go (1)
go/pkg/db/cached_key_data.go (1)
CachedKeyData(5-8)
go/apps/api/routes/v2_keys_update_credits/handler.go (1)
go/pkg/db/cached_key_data.go (1)
CachedKeyData(5-8)
go/apps/api/routes/v2_keys_remove_permissions/handler.go (1)
go/pkg/db/cached_key_data.go (1)
CachedKeyData(5-8)
go/apps/api/routes/v2_keys_add_permissions/handler.go (1)
go/pkg/db/cached_key_data.go (1)
CachedKeyData(5-8)
go/apps/api/routes/v2_keys_add_roles/handler.go (1)
go/pkg/db/cached_key_data.go (1)
CachedKeyData(5-8)
go/apps/api/routes/v2_keys_update_key/handler.go (1)
go/pkg/db/cached_key_data.go (1)
CachedKeyData(5-8)
go/apps/api/routes/v2_keys_set_roles/handler.go (1)
go/pkg/db/cached_key_data.go (1)
CachedKeyData(5-8)
go/internal/services/keys/get.go (5)
go/pkg/db/cached_key_data.go (1)
CachedKeyData(5-8)go/pkg/db/retry.go (1)
WithRetry(28-67)go/pkg/db/key_find_for_verification.sql_generated.go (1)
FindKeyForVerificationRow(92-119)go/internal/services/keys/verifier.go (1)
KeyVerifier(32-57)go/internal/services/keys/status.go (2)
StatusWorkspaceDisabled(23-23)StatusValid(15-15)
🔇 Additional comments (12)
go/apps/api/routes/v2_keys_remove_roles/handler.go (1)
32-32: LGTM! Type change aligns with IP whitelist optimization.The KeyCache type now uses
db.CachedKeyData, which embedsFindKeyForVerificationRowand addsParsedIPWhitelistfor pre-parsed IP addresses. This change enables the performance optimization described in the PR objectives without breaking existing Remove calls.go/apps/api/routes/v2_keys_update_key/handler.go (1)
38-38: LGTM! Consistent type change.The KeyCache field type change to
db.CachedKeyDatais consistent with the PR-wide refactor to support pre-parsed IP whitelists.go/apps/api/routes/v2_keys_add_roles/handler.go (1)
32-32: LGTM! Type change supports performance optimization.The KeyCache field now uses
db.CachedKeyData, enabling pre-parsed IP whitelist lookups as intended by the PR.go/apps/api/routes/v2_keys_delete_key/handler.go (1)
33-33: LGTM! Consistent cache type update.The KeyCache type change to
db.CachedKeyDatamaintains consistency across all handlers and supports the IP whitelist optimization.go/apps/api/routes/v2_keys_set_roles/handler.go (1)
33-33: LGTM! Type change aligns with refactor.The KeyCache field type update to
db.CachedKeyDatais consistent with the broader PR changes to optimize IP whitelist handling.go/apps/api/routes/v2_keys_set_permissions/handler.go (1)
34-34: LGTM! Consistent type change.The KeyCache field now uses
db.CachedKeyData, maintaining consistency with other handlers and enabling pre-parsed IP whitelist functionality.go/apps/api/routes/v2_keys_update_credits/handler.go (1)
34-34: LGTM! Type change supports optimization.The KeyCache field type change to
db.CachedKeyDatais correct and consistent with the PR objectives.go/apps/api/routes/v2_keys_remove_permissions/handler.go (1)
31-31: LGTM! Final handler type change.The KeyCache field type update to
db.CachedKeyDatacompletes the consistent refactor across all API route handlers, supporting the IP whitelist optimization.go/apps/api/routes/v2_keys_add_permissions/handler.go (1)
33-33: LGTM! Cache type updated to support pre-parsed IP whitelist.The change from
db.FindKeyForVerificationRowtodb.CachedKeyDataaligns with the broader refactoring to move IP whitelist parsing into the cache population phase.go/internal/services/keys/get.go (3)
7-7: LGTM! Import required for IP parsing logic.The
stringspackage is correctly imported and used at lines 83 and 85 for splitting and trimming IP whitelist entries.
71-96: LGTM! IP parsing moved to cache population as intended.This change achieves the core objective of parsing the IP whitelist once during cache population rather than on every verification request. Key improvements:
- IP parsing (split, trim, filter) now happens inside the SWR callback
- Empty strings are correctly filtered at line 86 via the
trimmed != ""check- Result is stored as
map[string]struct{}inCachedKeyDatafor O(1) lookup during verificationdb.WithRetrywrapper provides resilience for transient database errorsPerformance impact: Parsing overhead is eliminated from the hot path and amortized across cache lifetime.
194-214: LGTM! Uses pre-parsed IP whitelist from cache.The initialization correctly:
- Accesses the embedded
FindKeyForVerificationRowviakey.FindKeyForVerificationRow(line 195)- Uses the pre-parsed
ParsedIPWhitelistmap from cache (line 210)- Avoids redundant parsing on the verification hot path
|
Hi @Flo4604 I have resolved the bug which made the build fail. Actually Handler struct needed to be updated as I had updated CachedKeyData struct. I have tested this with make test. It ran successfully and similarly there is no issue with make build as well. |
Flo4604
left a comment
Flo4604
left a comment
There was a problem hiding this comment.
apps/api/integration/root_keys tests are failing looks like a bad merge
|
Hi @chronark Resolved merging bugs, please review those changes |
|
|
Graphite Automations"Post a GIF when PR approved" took an action on this PR • (10/09/25)1 gif was posted to this PR based on Andreas Thomas's automation. |
4 participants
What does this PR do?
Fixes #3544
This PR optimizes IP whitelist validation by moving the string parsing from the hot path (executed on every key verification) to the initialization phase (executed once when the key is loaded and cached).
Performance Impact:
KeyVerifierstructImplementation:
parsedIPWhitelist []stringfield toKeyVerifierstructGet()function where keys are loadedwithIPWhitelist()to use pre-parsed slice directlySince keys are cached, this optimization ensures string processing happens only once per cache entry rather than on every verification request.
Type of change
How should this be tested?
Unit Tests
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated