Skip to content

Extract client IP and TLS info once at adapter boundary (PR7)#599

Open
prk-Jr wants to merge 23 commits intofeature/edgezero-pr6-backend-http-clientfrom
feature/edgezero-pr7-geo-client-info
Open

Extract client IP and TLS info once at adapter boundary (PR7)#599
prk-Jr wants to merge 23 commits intofeature/edgezero-pr6-backend-http-clientfrom
feature/edgezero-pr7-geo-client-info

Conversation

@prk-Jr
Copy link
Copy Markdown
Collaborator

@prk-Jr prk-Jr commented Mar 31, 2026

Summary

  • Eliminates all req.get_client_ip_addr(), req.get_tls_protocol(), and req.get_tls_cipher_openssl_name() calls from active code in trusted-server-core; the adapter extracts these once into ClientInfo and threads it through RuntimeServices, keeping core fully platform-agnostic
  • Adds client_info: &'a ClientInfo to AuctionContext, changes RequestInfo::from_request and generate_synthetic_id to take &ClientInfo / &RuntimeServices instead of calling the Fastly SDK directly, and applies a warn-and-continue pattern for all geo lookups
  • Removes the last Fastly-specific IP/TLS SDK calls from core logic — after this PR, trusted-server-core contains zero direct calls to Fastly request introspection APIs

Changes

File Change
src/auction/types.rs Add client_info: &'a ClientInfo field to AuctionContext
src/auction/endpoints.rs Thread client_info into AuctionContext; replace deprecated geo call with warn-and-continue
src/auction/formats.rs Add services: &RuntimeServices and geo: Option<GeoInfo> params; use services.client_info.client_ip
src/auction/orchestrator.rs Thread client_info through mediator and provider context construction sites
src/http_util.rs RequestInfo::from_request takes &ClientInfo; detect_request_scheme takes TLS params directly
src/integrations/didomi.rs copy_headers takes client_ip: Option<IpAddr>; removes internal SDK call
src/integrations/prebid.rs Thread client_info into all RequestInfo::from_request call sites and AuctionContext construction
src/integrations/registry.rs Pass services to get_or_generate_synthetic_id
src/publisher.rs Thread services through; replace deprecated geo and synthetic ID calls
src/synthetic.rs generate_synthetic_id takes &RuntimeServices; use services.client_info.client_ip
src/platform/test_support.rs Add noop_services_with_client_ip test helper
adapter-fastly/src/main.rs Thread runtime_services into handle_publisher_request
src/auction/README.md Update route table to match current main.rs

Closes

Closes #488

Test plan

  • cargo test --workspace
  • cargo clippy --workspace --all-targets --all-features -- -D warnings
  • cargo fmt --all -- --check
  • JS tests: cd crates/js/lib && npx vitest run
  • JS format: cd crates/js/lib && npm run format
  • Docs format: cd docs && npm run format
  • WASM build: cargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1

Checklist

  • Changes follow CLAUDE.md conventions
  • No unwrap() in production code — use expect("should ...")
  • Uses log macros (not println!)
  • New code has tests
  • No secrets or credentials committed

prk-Jr added 20 commits March 30, 2026 18:00
Documents the call site migration plan: five Fastly SDK extraction
points in trusted-server-core replaced by RuntimeServices::client_info
reads, following Phase 1 injection pattern from the EdgeZero migration design.
- Correct erroneous claim about generate_synthetic_id being called twice
  via DeviceInfo; it is called once (line 91 for fresh_id), DeviceInfo.ip
  is a separate req.get_client_ip_addr() call fixed independently
- Add before/after snippet for handle_publisher_request call site in main.rs
- Add noop_services import instruction for http_util.rs test module
- Clarify _services rename (drop underscore, not add new param) in didomi.rs
- Clarify nextjs #[allow(deprecated)] annotations are out of scope (different function)
- Change RequestInfo::from_request signature to &ClientInfo (not
  &RuntimeServices) so prebid can call it with context.client_info
- Scope SDK-call acceptance criteria to active non-deprecated code only
- List all six AuctionContext construction sites including two production
  sites in orchestrator.rs and three test helpers in orchestrator/prebid
- Add explicit warn-and-continue pattern for publisher.rs geo lookup
- Correct testing table: formats.rs and endpoints.rs have no test modules;
  add orchestrator.rs and prebid.rs test helper update rows
Plan covers 6 tasks in compilation-safe order: AuctionContext struct change
first, then from_request signature, then synthetic.rs cascade, then publisher
geo, then didomi. Includes two new copy_headers unit tests (Some/None).

Spec fixes: clarify injection pattern exceptions for &ClientInfo and
Option<IpAddr>; reword acceptance criterion to reflect that provider-layer
reads flow through AuctionContext.client_info.
- Finding 1 (High): Add missing publisher.rs test call site at line ~695
  for get_or_generate_synthetic_id — was omitted from Task 3 Step 6
- Finding 2 (Medium): Remove crate::geo::GeoInfo import from endpoints.rs
  rather than replacing it — type is not used by name after the change,
  keeping any import fails clippy -D warnings
- Finding 3 (Low): Replace interactive git add -p in Task 6 with explicit
  file staging instruction
- Open Q1: Add Task 2 step to update stale handle_publisher_request
  signature in auction/README.md
- Open Q2: Add Task 2 step to update from_request doc comment to reflect
  ClientInfo-based TLS detection instead of Fastly SDK calls
- Step 7: cover all four stale Fastly-SDK-specific locations in
  http_util.rs (SPOOFABLE_FORWARDED_HEADERS doc, RequestInfo struct doc,
  from_request doc, detect_request_scheme doc)
- Step 8: replace the whole routing snippet in auction/README.md, not
  just the one handle_publisher_request line — handle_auction and
  integration_registry.handle_proxy are also stale in that snippet
- Add missing Location 2 (RequestInfo.scheme field doc, line ~67) to
  Step 7; renumber subsequent locations 3-5
- Replace &runtime_services with runtime_services in Step 5 and README
  snippet — runtime_services is already &RuntimeServices in route_request
Fix multi-line function call style in didomi.rs, line-break wrapping in
publisher.rs test, and import ordering in synthetic.rs test module.
Adds noop_services_with_client_ip helper to test_support and a new
test that verifies the client_ip path through generate_synthetic_id
by asserting the HMAC differs when the IP changes.
@prk-Jr prk-Jr self-assigned this Mar 31, 2026
@prk-Jr prk-Jr changed the title Extract client IP and TLS info once at adapter boundary Extract client IP and TLS info once at adapter boundary (PR7) Mar 31, 2026
@prk-Jr prk-Jr linked an issue Mar 31, 2026 that may be closed by this pull request
…edgezero-pr7-geo-client-info

# Conflicts:
#	crates/trusted-server-core/src/auction/endpoints.rs
#	crates/trusted-server-core/src/auction/formats.rs
#	crates/trusted-server-core/src/integrations/registry.rs
#	crates/trusted-server-core/src/publisher.rs
#	crates/trusted-server-core/src/synthetic.rs
Copy link
Copy Markdown
Collaborator

@aram356 aram356 left a comment

Choose a reason for hiding this comment

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

Summary

Clean, well-scoped PR that achieves exactly what it claims: removing all active Fastly SDK calls for client IP and TLS info from trusted-server-core by threading ClientInfo through RuntimeServices. The changes are mechanical but correct — no logic errors, no missing call sites, proper warn-and-continue for geo lookups, and good test coverage for the new paths.

Non-blocking

👍 praise

  • Clean extraction pattern — each function that previously called Fastly SDK methods now reads from services.client_info or context.client_info. No unnecessary abstractions introduced.
  • Warn-and-continue for geo lookups — all three geo lookup sites use unwrap_or_else(|e| { log::warn!(...); None }). Correct — geo is best-effort enrichment and should never hard-fail the request.
  • Good test coverage — new test_generate_ec_id_uses_client_ip, two TLS detection tests, two copy_headers tests. Validates that client IP actually influences output through RuntimeServices.
  • noop_services_with_client_ip helper — clean addition for tests that need IP-dependent paths.

🤔 thinking

  • Remaining deprecated GeoInfo::from_request still has Fastly SDK call in core — the #[deprecated] method still exists with a live req.get_client_ip_addr() inside core. No active code calls it, but worth tracking for removal in a follow-up.
  • AuctionContext carries both client_info and can access it via RuntimeServices — the field is redundant with what's available through RuntimeServices (both get passed together). Works fine since values are identical, but could confuse future contributors about which source to use.

♻️ refactor

  • Repeated ClientInfo construction in Prebid tests — ~19 test call sites each inline ClientInfo { client_ip: None, tls_protocol: None, tls_cipher: None }. Consider implementing Default for ClientInfo (all fields are Option) or extracting an empty_client_info() test helper to reduce noise.

📌 out of scope

  • RequestSigner::from_config() still uses Fastly ConfigStore directly in core — separate concern from IP/TLS extraction but worth noting for EdgeZero migration tracking.

CI Status

  • fmt: PASS
  • clippy: PASS
  • rust tests: PASS
  • js tests: PASS

Copy link
Copy Markdown
Collaborator

@ChristianPavilonis ChristianPavilonis left a comment

Choose a reason for hiding this comment

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

Please fix conflicts

Copy link
Copy Markdown
Collaborator

@aram356 aram356 left a comment

Choose a reason for hiding this comment

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

Summary

Clean, well-executed migration. The extract-once pattern threads ClientInfo from the adapter boundary through RuntimeServices and AuctionContext, eliminating all active Fastly IP/TLS SDK calls from trusted-server-core. Test coverage is solid — both positive and negative paths for new code.

Non-blocking

♻️ refactor

  • Repeated ClientInfo literal in tests: ~20 prebid/orchestrator test sites construct identical ClientInfo { client_ip: None, ... } inline. A shared constant or helper would reduce boilerplate. (prebid.rs, orchestrator.rs)

🤔 thinking

  • convert_tsjs_to_auction_request at 7-param limit: Now at exactly the CLAUDE.md convention limit. Next addition will need a struct. (formats.rs:83)

🌱 seedling

  • Deprecated GeoInfo::from_request cleanup: The deprecated method in geo.rs still calls req.get_client_ip_addr() directly. Now that all active call sites go through services.geo().lookup(), this could be removed in a follow-up.

👍 praise

  • Extract-once pattern: Well-designed ClientInfo injection — single adapter-boundary extraction, clean threading through the call chain.
  • Warn-and-continue geo pattern: Consistent across endpoints.rs and publisher.rs.
  • generate_ec_id_uses_client_ip test: Smart HMAC prefix comparison proves IP input flows through.
  • Didomi copy_headers tests: Both Some(ip) and None paths covered.

CI Status

  • integration tests: PASS
  • browser integration tests: PASS
  • prepare integration artifacts: PASS

&settings,
&request,
&crate::platform::ClientInfo {
client_ip: None,
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

♻️ refactor — The identical ClientInfo { client_ip: None, tls_protocol: None, tls_cipher: None } literal is repeated ~20 times across prebid and orchestrator tests. A shared constant or helper would reduce boilerplate and make future field additions less painful.

@@ -83,14 +83,17 @@ pub struct BannerUnit {
pub fn convert_tsjs_to_auction_request(
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🤔 thinking — This function now takes exactly 7 parameters, which is the project convention limit. It passes today, but one more addition will require restructuring.

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.

Geo lookup + client info (extract-once)

3 participants