Skip to content

Conversation

@NathanFlurry
Copy link
Member

No description provided.

@vercel
Copy link

vercel bot commented Oct 27, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

3 Skipped Deployments
Project Deployment Preview Comments Updated (UTC)
rivet-cloud Ignored Ignored Oct 27, 2025 4:01pm
rivet-inspector Ignored Ignored Preview Oct 27, 2025 4:01pm
rivet-site Ignored Ignored Preview Oct 27, 2025 4:01pm

Copy link
Member Author

NathanFlurry commented Oct 27, 2025


How to use the Graphite Merge Queue

Add the label merge-queue to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

@claude
Copy link

claude bot commented Oct 27, 2025

Code Review

Summary

This PR modifies the valid_hosts configuration behavior for datacenters. The changes disable the valid_hosts filter when not explicitly configured (single datacenter case) while requiring it for multi-datacenter setups.

Positive Observations ✅

  1. Clear Intent: The change makes the valid_hosts configuration optional for single-datacenter deployments, which simplifies development and testing workflows.

  2. Validation Logic: The new validation in validate_and_set_defaults() (mod.rs:179-190) ensures that multi-datacenter configurations explicitly set valid_hosts, preventing misconfiguration.

  3. Backward Compatibility: Single datacenter deployments continue to work without requiring valid_hosts configuration.

  4. Error Messaging: The error message clearly indicates which datacenter is missing the required configuration.

Concerns and Suggestions 🔍

1. Security Consideration - Medium Priority

The change in is_valid_regional_host() (topology.rs:100-102) now returns true when valid_hosts is not configured. This effectively disables host validation entirely.

Current behavior:

} else {
    // Ignore this behavior if not configured
    true
}

Concern: In production multi-datacenter environments, if someone accidentally omits valid_hosts from the config, the validation at startup would catch it, but the old behavior (checking against public_url.host_str()) provided a fallback security mechanism. The new behavior is more permissive.

Recommendation: This is acceptable given the validation check, but consider adding a comment in the docs/code explaining that single-datacenter setups don't need host validation since there's no routing ambiguity.

2. Test Coverage - High Priority

The PR doesn't include tests for the new validation logic.

Suggested tests:

#[test]
fn test_single_datacenter_without_valid_hosts() {
    // Should pass validation
}

#[test]
fn test_multi_datacenter_without_valid_hosts() {
    // Should fail validation with appropriate error
}

#[test]
fn test_multi_datacenter_with_valid_hosts() {
    // Should pass validation
}

Location: Consider adding these to engine/packages/config/src/config/mod.rs or a new test file.

3. Documentation Gap

The change in behavior should be documented, particularly:

  • When valid_hosts is required vs optional
  • The security implications of the change
  • Migration path for existing deployments

Where it's used: The is_valid_regional_host() method is called in guard/src/routing/runner.rs:26, which is a critical routing path for runner connections.

4. Edge Case - Low Priority

In guard/src/routing/runner.rs:30-38, the error message generation tries to handle both cases:

let valid_hosts = if let Some(hosts) = &current_dc.valid_hosts {
    hosts.join(", ")
} else {
    current_dc.public_url.host_str()...
}

However, with the new logic, if valid_hosts is None, the validation would have already passed (returned true), so this else branch in the error handling becomes unreachable in practice. Consider simplifying this or adding a comment explaining the defensive coding.

Code Quality ✅

  • Follows CLAUDE.md conventions:

    • Uses anyhow::* import
    • Proper error handling with bail!
    • Lowercase log messages (though no logging in this PR)
    • Structured code organization
  • Naming Conventions: ✅ Clear and consistent

  • Error Messages: ✅ Descriptive and actionable

Performance Considerations

✅ No performance concerns - the validation happens once at config load time.

Security Considerations

⚠️ See concern #1 above. The change is reasonable but reduces defense-in-depth slightly. The validation check mitigates this.

Recommended Actions

  1. Add unit tests for the validation logic (high priority)
  2. Consider adding a tracing::warn! in is_valid_regional_host() when valid_hosts is None and we're returning true, to help debug multi-datacenter routing issues
  3. Document the behavior change in code comments and/or migration guides
  4. Consider updating test-deps/src/lib.rs:54 to set valid_hosts when creating multi-datacenter test configurations to match the new requirement

Overall Assessment

The changes are well-structured and solve a legitimate usability issue. The validation logic provides a good safety net. The main gap is test coverage. With tests added, this would be ready to merge.

Recommendation: Approve with minor changes (add tests)

@pkg-pr-new
Copy link

pkg-pr-new bot commented Oct 27, 2025

More templates

@rivetkit/actor

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/actor@3274

@rivetkit/cloudflare-workers

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/cloudflare-workers@3274

@rivetkit/core

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/core@3274

@rivetkit/db

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/db@3274

@rivetkit/framework-base

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/framework-base@3274

@rivetkit/next-js

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/next-js@3274

@rivetkit/react

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/react@3274

rivetkit

pnpm add https://pkg.pr.new/rivet-dev/rivet/rivetkit@3274

@rivetkit/sql-loader

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/sql-loader@3274

@rivetkit/engine-runner

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/engine-runner@3274

@rivetkit/engine-runner-protocol

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/engine-runner-protocol@3274

commit: 3cc024b

@NathanFlurry NathanFlurry merged commit 6550101 into main Oct 27, 2025
9 of 12 checks passed
@NathanFlurry NathanFlurry deleted the 10-27-chore_config_disable_valid_hosts_filter_unless_explicitly_configured branch October 27, 2025 20:07
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.

2 participants