-
Notifications
You must be signed in to change notification settings - Fork 1
feat: Add jump_host field support in config.yaml (issue #115) #120
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
Add jump_host configuration support at three levels:
- Global defaults (Defaults struct)
- Cluster-level (ClusterDefaults struct)
- Node-level (NodeConfig::Detailed variant)
Resolution priority (highest to lowest):
1. Node-level jump_host
2. Cluster-level jump_host
3. Global default jump_host
4. CLI -J option (fallback)
Empty string ("") explicitly disables jump host inheritance.
Also enables -J support for ping/upload/download commands by:
- Adding jump_hosts field to FileTransferParams
- Passing resolved jump_hosts to ping_nodes function
- Updating dispatcher to resolve and pass jump_hosts to all handlers
Includes comprehensive unit tests for all resolution scenarios.
Security & Performance ReviewAnalysis Summary
Prioritized Fix RoadmapHIGH
MEDIUM
LOW
Technical DetailsIssue 1: Missing environment variable expansion Location: The code returns return Some(jh.clone()); // Should be: return Some(expand_env_vars(jh));Issue 2: Node-level jump_host not used The dispatcher at let jump_hosts = cli.jump_hosts.clone().or_else(|| {
ctx.config.get_cluster_jump_host(...) // Does not use get_jump_host() for per-node resolution
});This design decision may be intentional since parallelized operations may not easily support per-node jump hosts, but it should be documented. Progress Log
Review StatusReview in progress. Waiting for confirmation on whether to auto-fix issues or provide patches for manual review. |
…xec/interactive - Add environment variable expansion to get_jump_host() and get_cluster_jump_host() - Add config fallback for jump_hosts in exec command (was only using CLI option) - Add config fallback for jump_hosts in interactive commands - Add test for environment variable expansion in jump_host - Update README with jump_host configuration documentation Addresses review feedback on PR #120 (issue #115)
…xec/interactive - Add environment variable expansion to get_jump_host() and get_cluster_jump_host() - Add config fallback for jump_hosts in exec command (was only using CLI option) - Add config fallback for jump_hosts in interactive commands - Add test for environment variable expansion in jump_host - Update README with jump_host configuration documentation Addresses review feedback on PR #120 (issue #115)
Add missing test cases for issue #115: Unit tests in src/config/tests.rs: - test_jump_host_out_of_bounds_node_index: Edge case for invalid node index - test_jump_host_mixed_simple_detailed_nodes: Mixed node config inheritance - test_jump_host_with_port_format: Jump host with port specification - test_jump_host_multi_hop_format: Multi-hop jump host chain Integration tests in tests/jump_host_config_test.rs: - test_config_global_jump_host_applied_to_all_clusters - test_config_cluster_jump_host_overrides_global - test_config_node_jump_host_overrides_cluster - test_config_empty_string_disables_jump_host - test_config_jump_host_env_expansion - test_config_jump_host_full_format - test_config_jump_host_multi_hop - test_config_backward_compatibility - test_config_jump_host_resolution_priority - test_config_simple_nodes_inherit_cluster_jump_host Total new tests: 14
- Update manpage with jump_host configuration examples and section - Update configuration.md with jump_host data model and resolution - Update ssh-jump-hosts.md to mark YAML config as implemented - Add jump_host examples to example-config.yaml
Summary
jump_hostfield support at three configuration levels: global defaults, cluster, and node-Jsupport for ping/upload/download commands that were previously missing this functionalityChanges
Part A: Config.yaml support for jump_host
jump_host: Option<String>toDefaultsstruct (global level)jump_host: Option<String>toClusterDefaultsstruct (cluster level)jump_host: Option<String>toNodeConfig::Detailedvariant (node level)get_jump_host()andget_cluster_jump_host()resolution methodsPart B: Enable -J support for ping/upload/download
jump_hostsfield toFileTransferParamsstructping_nodesto accept and usejump_hostsparameterjump_hoststo all handlersResolution Priority
Empty string (
"") explicitly disables jump host inheritance.Example config.yaml
Test plan
cargo build)cargo build --release)Files Changed
src/config/types.rs- Add jump_host fields to structssrc/config/resolver.rs- Add get_jump_host() methodssrc/commands/ping.rs- Add jump_hosts parametersrc/commands/upload.rs- Add jump_hosts to FileTransferParamssrc/commands/download.rs- Use jump_hosts from paramssrc/app/dispatcher.rs- Resolve and pass jump_hosts to handlerssrc/config/tests.rs- Add comprehensive unit testsCloses #115