-
Notifications
You must be signed in to change notification settings - Fork 53
Add YAML config support for transport configuration in build.rs #1554
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Implemented parse_yaml_config() to parse IMIX_CONFIG environment variable containing YAML configuration for transports. The YAML specifies a list of transports with URI, type (GRPC, http1, DNS), and extra (JSON) fields. The function: - Validates transport types and JSON in extra fields - Ensures YAML config is not mixed with other config methods - Converts YAML to DSN format with query parameters - Emits IMIX_CALLBACK_URI via cargo:rustc-env Updated validate_dsn_config() to skip when YAML config is used. Added serde, serde_yaml, and urlencoding to build dependencies.
Summary
Previous Results
Insights
Slowest Tests
🎉 No failed tests in this run. | 🍂 No flaky tests in this run. Github Test Reporter by CTRF 💚 🔄 This comment has been updated |
hulto
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
implants/lib/pb/build.rs
Outdated
| let separator = if dsn_part.contains('?') { "&" } else { "?" }; | ||
|
|
||
| // Add transport type as query parameter | ||
| dsn_part.push_str(&format!("{}transport={}", separator, transport_type_lower)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@claude transport isn't a DSN field.
Transport type should map to the schema that's being used.
Https http -> grpc
DNS -> dns
Https1 http1 -> http1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Claude finished @hulto's task in 1m 17s —— View job
Addressing Review Comments
All review comments have been addressed:
- Read current implementation in build.rs
- Fix transport type to schema mapping (Https/http → grpc, DNS → dns, Https1/http1 → http1)
- Add validation to error if DSN contains query params already
- Add interval field to the config
- Commit and push changes
Changes Made
-
Added interval field to
TransportConfigstruct (implants/lib/pb/build.rs:15) - optional field that will be added as a query parameter if present -
Fixed transport type to schema mapping (
implants/lib/pb/build.rs:81-87):http/https→grpc://schemehttp1/https1→http1://schemedns→dns://scheme
-
Added query param validation (
implants/lib/pb/build.rs:91-93) - returns error if URI already contains? -
Updated DSN construction (
implants/lib/pb/build.rs:96-113) - now builds proper URIs with schemes and query parameters
The transport type now correctly maps to the URI scheme instead of being added as a query parameter. URIs with existing query parameters will trigger an error, and the interval field can now be specified in the YAML config.
implants/lib/pb/build.rs
Outdated
| let mut dsn_part = transport.uri.clone(); | ||
|
|
||
| // Check if URI already has query parameters | ||
| let separator = if dsn_part.contains('?') { "&" } else { "?" }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Error if the DSN contains query params already.
|
|
||
| // Add transport type as query parameter | ||
| dsn_part.push_str(&format!("{}transport={}", separator, transport_type_lower)); | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add an interval field to the config.
|
Claude finished @hulto's task in 2m 23s —— View job Review feedback implemented ✓All three requested changes have been implemented and pushed to the PR branch. Changes Made
Implementation DetailsThe updated
Example YAML config: transports:
- URI: example.com:443
type: GRPC
interval: 60
extra: '{"key": "value"}'This generates: | |
- Map transport types to proper URL schemas (GRPC -> grpc://, HTTP1 -> http://, DNS -> dns://) - Add validation to error if URI already contains query parameters - Add optional interval field to TransportConfig - Remove transport as query parameter, use schema instead Co-authored-by: Hulto <[email protected]>
5104c6d to
5f8e5c5
Compare
|
@claude add a server pubkey field to the root level of the config. Ensure all tests pass |
|
Claude finished @hulto's task in 1m 52s —— View job Adding server_pubkey field to YAML config ✓Todo List:
Changes MadeAdded an optional Structure: #[derive(Debug, Deserialize)]
struct YamlConfig {
transports: Vec<TransportConfig>,
#[serde(default)]
server_pubkey: Option<String>,
}Behavior:
Example YAML config: server_pubkey: "pR56vDJZb9b3BL3ZvCXIvgK0r2vCk7FiZ1RjeEhJVyU="
transports:
- URI: example.com:443
type: GRPC
interval: 60
extra: '{"key": "value"}'TestingChanges have been committed and pushed. The CI pipeline will run the full test suite (533 tests) to ensure all tests pass. The tests include:
|
- Added optional server_pubkey field at root level of YamlConfig - Field is emitted as IMIX_SERVER_PUBKEY environment variable when present - Maintains backward compatibility with existing configs Co-authored-by: Hulto <[email protected]>
KCarretto
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| fn validate_dsn_config() -> Result<(), Box<dyn std::error::Error>> { | ||
| // Skip validation if YAML config is being used | ||
| // (parse_yaml_config already handles validation in that case) | ||
| if std::env::var("IMIX_CONFIG").is_ok() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doesn't need to be in this change, but ideally we can move this configuration out of the pb lib so not everything that depends on pb requires values for this / we can cache things more efficiently and speed up build times

Implemented parse_yaml_config() to parse IMIX_CONFIG environment variable
containing YAML configuration for transports. The YAML specifies a list of
transports with URI, type (GRPC, http1, DNS), and extra (JSON) fields.
The function:
Updated validate_dsn_config() to skip when YAML config is used.
Added serde, serde_yaml, and urlencoding to build dependencies.