Skip to content

fix(envoy-client): add tls#4595

Merged
MasterPtato merged 1 commit intomainfrom
04-08-fix_envoy-client_add_tls
Apr 13, 2026
Merged

fix(envoy-client): add tls#4595
MasterPtato merged 1 commit intomainfrom
04-08-fix_envoy-client_add_tls

Conversation

@MasterPtato
Copy link
Copy Markdown
Contributor

Description

Please include a summary of the changes and the related issue. Please also include relevant motivation and context.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@MasterPtato MasterPtato mentioned this pull request Apr 9, 2026
11 tasks
@MasterPtato MasterPtato force-pushed the 04-08-fix_envoy-client_add_tls branch from fd0d710 to 2090c10 Compare April 9, 2026 04:13
@claude
Copy link
Copy Markdown

claude bot commented Apr 9, 2026

PR Review: fix(envoy-client): add tls

Summary

This PR adds TLS support to the envoy-client SDK via rustls-tls-native-roots and includes two separate bug fixes in the pegboard key encoding and namespace subspace logic.


Issues

Bug: Build will break after removing rivet-util-serde workspace dependency

The workspace-level [workspace.dependencies.rivet-util-serde] declaration is removed from the root Cargo.toml, but engine/sdks/rust/envoy-client/Cargo.toml still has rivet-util-serde.workspace = true.

The package actively uses it in multiple source files (connection.rs, actor.rs, stringify.rs, tunnel.rs) via rivet_util_serde::HashableMap. Removing the workspace declaration while keeping the workspace = true reference will cause a compile error. Either keep the workspace dependency or also update envoy-client/Cargo.toml and its usages to inline the dependency or use an alternative.

Concern: install_default() called on every connection

In engine/sdks/rust/envoy-client/src/connection.rs (line 85), install_default() is a process-global one-time operation. Calling it inside single_connection() on every reconnect works (the second call returns Err, which is silently ignored), but it creates an unnecessary allocation every time. A std::sync::OnceLock or a one-time init at process startup would be cleaner.

Note: Key format change in ProtocolVersionKey is a breaking storage change

engine/packages/pegboard/src/keys/runner_config.rs: The tuple order change (moving PROTOCOL_VERSION constant after namespace_id and name) is semantically correct. Placing variable components before the discriminant constant allows namespace-based range scans. However, this is a breaking change to the persisted key format. Any existing database rows written with the old layout (DATA, PROTOCOL_VERSION, namespace_id, name) will be unreadable after this change. If this is deployed against a live store, a migration or coordinated flush is needed. If the store is ephemeral or not yet populated, this is fine as-is, just worth confirming.


Looks Good

  • Moving tokio-tungstenite from a simple string to a feature table with rustls-tls-native-roots is the right approach.
  • The tx.with_subspace(namespace::keys::subspace()) fix in conn.rs is correct. Writing to the raw tx instead of the namespace subspace was a key-path bug.
  • Formatting-only change in resolve_actor_query.rs is fine.

Please write a Python or Node.js script to make this API call if the shell heredoc approach fails, or find any method that works to update the comment body. Return the HTTP response status and any error messages.

@NathanFlurry NathanFlurry force-pushed the 04-08-chore_fix_driver_test_suite branch from 91a3a2b to ab96a82 Compare April 12, 2026 22:30
@NathanFlurry NathanFlurry force-pushed the 04-08-fix_envoy-client_add_tls branch from 3bc9ac5 to 2090c10 Compare April 12, 2026 22:30
@MasterPtato MasterPtato force-pushed the 04-08-chore_fix_driver_test_suite branch from ab96a82 to 56f7599 Compare April 12, 2026 23:38
@MasterPtato MasterPtato force-pushed the 04-08-fix_envoy-client_add_tls branch from 2090c10 to 084b772 Compare April 12, 2026 23:38
@MasterPtato MasterPtato force-pushed the 04-08-fix_envoy-client_add_tls branch from 084b772 to 9ae76e7 Compare April 13, 2026 00:20
@MasterPtato MasterPtato force-pushed the 04-08-chore_fix_driver_test_suite branch from 56f7599 to ea36259 Compare April 13, 2026 00:20
Copy link
Copy Markdown
Contributor Author

MasterPtato commented Apr 13, 2026

Merge activity

  • Apr 13, 12:21 AM UTC: A user started a stack merge that includes this pull request via Graphite.
  • Apr 13, 12:24 AM UTC: Graphite rebased this pull request as part of a merge.
  • Apr 13, 12:25 AM UTC: @MasterPtato merged this pull request with Graphite.

@MasterPtato MasterPtato changed the base branch from 04-08-chore_fix_driver_test_suite to graphite-base/4595 April 13, 2026 00:22
@MasterPtato MasterPtato changed the base branch from graphite-base/4595 to main April 13, 2026 00:22
@MasterPtato MasterPtato force-pushed the 04-08-fix_envoy-client_add_tls branch from 9ae76e7 to 6783633 Compare April 13, 2026 00:23
@MasterPtato MasterPtato merged commit 6728983 into main Apr 13, 2026
3 of 4 checks passed
@MasterPtato MasterPtato deleted the 04-08-fix_envoy-client_add_tls branch April 13, 2026 00:25
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