Skip to content

Conversation

@inureyes
Copy link
Member

Summary

  • Add jump_host field support at three configuration levels: global defaults, cluster, and node
  • Enable -J support for ping/upload/download commands that were previously missing this functionality
  • Implement proper resolution priority with inheritance and explicit disable via empty string

Changes

Part A: Config.yaml support for jump_host

  • Added jump_host: Option<String> to Defaults struct (global level)
  • Added jump_host: Option<String> to ClusterDefaults struct (cluster level)
  • Added jump_host: Option<String> to NodeConfig::Detailed variant (node level)
  • Implemented get_jump_host() and get_cluster_jump_host() resolution methods

Part B: Enable -J support for ping/upload/download

  • Added jump_hosts field to FileTransferParams struct
  • Updated ping_nodes to accept and use jump_hosts parameter
  • Updated dispatcher to resolve and pass jump_hosts to all handlers

Resolution Priority

  1. Node-level (highest)
  2. Cluster-level
  3. Global default
  4. CLI -J option (fallback)

Empty string ("") explicitly disables jump host inheritance.

Example config.yaml

defaults:
  jump_host: bastion.example.com  # Global default

clusters:
  production:
    nodes:
      - host: prod1.internal
        jump_host: prod1-bastion.example.com  # Node-level override
      - host: prod2.internal  # Uses cluster jump_host
    jump_host: prod-bastion.example.com  # Cluster-level override

  direct_access:
    nodes:
      - host: direct.example.com
    jump_host: ""  # Explicitly disable (direct access)

Test plan

  • Build passes (cargo build)
  • Release build passes (cargo build --release)
  • All existing tests pass
  • All new jump_host resolution tests pass (7 test cases)
  • Clippy passes without warnings
  • Code formatted with rustfmt

Files Changed

  • src/config/types.rs - Add jump_host fields to structs
  • src/config/resolver.rs - Add get_jump_host() methods
  • src/commands/ping.rs - Add jump_hosts parameter
  • src/commands/upload.rs - Add jump_hosts to FileTransferParams
  • src/commands/download.rs - Use jump_hosts from params
  • src/app/dispatcher.rs - Resolve and pass jump_hosts to handlers
  • src/config/tests.rs - Add comprehensive unit tests

Closes #115

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.
@inureyes inureyes added type:enhancement New feature or request status:review Under review priority:medium Medium priority issue labels Dec 19, 2025
@inureyes
Copy link
Member Author

Security & Performance Review

Analysis Summary

  • Scope: changed-files
  • Languages: Rust
  • Total issues: 4
  • Critical: 0 | High: 1 | Medium: 2 | Low: 1

Prioritized Fix Roadmap

HIGH

  • Missing environment variable expansion for jump_host values - The get_jump_host() and get_cluster_jump_host() methods return jump_host values without passing them through expand_env_vars(). Other config fields like host, user are expanded. This inconsistency could cause unexpected behavior when users expect ${BASTION_HOST} to work in config.

MEDIUM

  • Inconsistent jump_host resolution between exec and ping/upload/download - The exec command uses cli.jump_hosts.as_deref() passed directly to ExecuteCommandParams, while ping/upload/download use the new get_cluster_jump_host() method. Node-level jump_host overrides (via get_jump_host(cluster, node_index)) are not utilized in the dispatcher.
  • Missing node-level jump_host resolution in dispatcher - The dispatcher only calls get_cluster_jump_host() which ignores node-level jump_host settings. This means per-node jump_host configuration won't be applied for ping/upload/download commands.

LOW

  • Documentation update needed - The PR should update README.md or configuration documentation to document the new jump_host field at all three levels.

Technical Details

Issue 1: Missing environment variable expansion

Location: /Users/inureyes/Development/backend.ai/bssh/src/config/resolver.rs lines 135-180

The code returns jh.clone() directly without calling expand_env_vars():

return Some(jh.clone());  // Should be: return Some(expand_env_vars(jh));

Issue 2: Node-level jump_host not used

The dispatcher at /Users/inureyes/Development/backend.ai/bssh/src/app/dispatcher.rs lines 82-86 only resolves cluster-level jump_host:

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

  • Currently: Initial analysis

Review Status

Review in progress. Waiting for confirmation on whether to auto-fix issues or provide patches for manual review.

inureyes added a commit that referenced this pull request Dec 19, 2025
…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
@inureyes inureyes self-assigned this Dec 19, 2025
- 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
@inureyes inureyes merged commit 8177092 into main Dec 19, 2025
2 checks passed
@inureyes inureyes deleted the feature/issue-115-jump-host-config branch December 19, 2025 11:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

priority:medium Medium priority issue status:review Under review type:enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat: Add jump_host field support in config.yaml

2 participants