Add TOML configuration to the autopilot#4147
Conversation
jmg-duarte
force-pushed
the
jmgd/config-drivers
branch
from
abe19c9 to
7152f44
Compare
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces TOML-based configuration for the autopilot service, migrating the drivers argument from a CLI flag. While this is a positive step for configuration management, a critical security vulnerability has been identified: sensitive external service URLs (including node URLs and solver URLs) are logged in plain text, potentially exposing API keys or authentication tokens. It is crucial to implement masking utilities for all URLs that may contain secrets before logging. Furthermore, the PR has critical functional issues: a necessary validation check for drivers configuration was removed without replacement, risking an invalid service state, and the fairness_threshold configuration for solvers appears to be removed without documentation, which could lead to a significant functional regression, with misleading tests.
Fix to_temp_path() to actually serialize and write the TOML config content instead of creating an empty file. Replace manual temp dir/file creation in 6 e2e test files (12 occurrences) with the cleaner to_temp_path() helper, which uses tempfile::NamedTempFile for automatic cleanup on drop.
jmg-duarte
force-pushed
the
jmgd/config-drivers
branch
from
8952364 to
16f93f6
Compare
| #[derive(Debug, Clone, PartialEq, Eq, Hash, Deserialize, Serialize)] | ||
| #[serde(rename_all = "kebab-case")] | ||
| pub enum Account { | ||
| /// AWS KMS is used to retrieve the solver public key | ||
| #[serde(deserialize_with = "deserialize_arn")] | ||
| Kms(Arn), | ||
| /// Solver public key | ||
| Address(Address), | ||
| } | ||
|
|
||
| // Wrapper type for AWS ARN identifiers | ||
| #[derive(Debug, Clone, PartialEq, Eq, Hash, Serialize)] | ||
| pub struct Arn(pub String); | ||
|
|
||
| fn deserialize_arn<'de, D>(deserializer: D) -> Result<Arn, D::Error> | ||
| where | ||
| D: Deserializer<'de>, | ||
| { | ||
| let raw_arn = String::deserialize(deserializer)?; | ||
| if raw_arn.starts_with("arn:aws:kms:") { | ||
| Ok(Arn(raw_arn)) | ||
| } else { | ||
| Err(serde::de::Error::invalid_value( | ||
| serde::de::Unexpected::Str(raw_arn.as_str()), | ||
| &"expected value starting with \"arn:aws:kms\"", | ||
| )) | ||
| } | ||
| } |
There was a problem hiding this comment.
Does it duplicate the logic implemented in the driver crate? Maybe it makes sense to extract it somewhere, so it is accessible by both crates.
There was a problem hiding this comment.
Slightly, but the driver does things a little differently. I'd like to keep this PR focused and simple enough for review
4 participants
Description
Kicks off #4005; to start only one of the more annoying arguments was migrated to validate the approach.
Changes
How to test