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.
crates/e2e/src/setup/services.rs
Outdated
| Configuration { | ||
| // replace the --drivers argument with a vec of Solver structs | ||
| drivers: vec
There was a problem hiding this comment.
Let's have a helper function writing the config to a file so we can separate the logic of writing the file from building the exact configuration that's needed for a given test.
There was a problem hiding this comment.
Not sure I follow here. You mean setting up the temp dir and so on?
There was a problem hiding this comment.
Yes. Serializing the config struct and writing it to a temp file was duplicated in 2 places when a helper function is very simple for this.
There was a problem hiding this comment.
I've done that in the mean time , the to_temp_path function
crates/e2e/src/setup/services.rs
Outdated
| // Create TOML config file for the driver | ||
| let config_dir = std::env::temp_dir().join("cow-e2e-autopilot"); | ||
| std::fs::create_dir_all(&config_dir).unwrap(); |
There was a problem hiding this comment.
What's the reason for sometimes serializing a config rust struct and sometimes handwriting the toml string?
There was a problem hiding this comment.
This was all automated with Claude, I've also added a better API since so I'll clear this up
There was a problem hiding this comment.
This was all automated with Claude
In order to avoid wasting time on reviewing unfinished stuff please first make sure to review your PR yourself. Also while you are actively working on a PR mark it as a draft so we know that we can ignore notifications.
There was a problem hiding this comment.
I did, I think this was the sole one that didn't match the pattern
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
3 participants
Description
Kicks off #4005; to start only one of the more annoying arguments was migrated to validate the approach.
Changes
How to test