[sanjaydilli]

Summary

• Use the runtime effective port as the SSH tunnel target for NETCONF connections.
• Track and clean up tunnels created by the connector, including rollback when session setup fails.
• Leave adopted tunnels intact when disconnecting.
• Add tests for tunneled and untunneled connections and tunnel lifecycle.

[Sripadvallabh]

Fix comments & click re-review

[Sripadvallabh]

Verified stale-target replacement and tunnel ownership handling on the current head.

[dwapstra]

This data should not be in the testbed.

[dwapstra]

[P1] This combines endpoints from opposite sides of QEMU forwarding. qemu_usernet.guest_ip is the guest address, while effective_port listens on the QEMU host. Use the generated connection host with the forwarded port, or the guest address with the IOS XE service port. This connector currently receives the host endpoint, so it should not read qemu_usernet.

Suggested replacement:

connection = self._connection_block()
target_address = connection.get("ip") or connection.get("host")
if target_address is None:
    raise AttributeError("NETCONF SSH tunnel target address is not configured")

lease = sshtunnel.auto_tunnel_add(
    self.device,
    self.via,
    target_address=str(target_address),
    target_port=effective_port,
    return_lease=True,
)
defaults["host"] = lease.bind_address
defaults["port"] = lease.port
self._tunnel_lease = lease

This assumes the lease extension proposed in Unicon PR 410. Without it, use auto_tunnel_add() and obtain the bind address from the connection’s sshtunnel block.

[dwapstra]

[P1] Use Unicon’s public validation and ownership path. This local implementation checks the connected flag, target metadata, and port, but can reuse a tunnel after the proxy reconnect has removed its listener. Keeping two lifecycle implementations will drift. Replace _add_netconf_tunnel() and _add_netconf_tunnel_locked() with sshtunnel.auto_tunnel_add(...) and use the symmetric release operation for cleanup.

Suggested cleanup:

def _cleanup_owned_tunnel(self):
    lease = getattr(self, "_tunnel_lease", None)
    if lease is None:
        return
    from unicon.sshutils import sshtunnel
    sshtunnel.auto_tunnel_release(self.device, lease)
    self._tunnel_lease = None

[dwapstra]

[P2] Replacing this live tunnel loses another consumer’s forwarding and does not restore it on disconnect. Unicon’s public API rejects target changes while a live tunnel exists. Until this connector uses the public lifecycle API, reject a mismatch rather than mutating shared tunnel state.

Suggested replacement:

if tunnel_config.get("connected"):
    if not target_matches:
        raise RuntimeError(
            "Existing NETCONF SSH tunnel targets {}:{}, requested {}:{}. "
            "Disconnect its owner before changing the target.".format(
                tunnel_config.get("target_address"),
                tunnel_config.get("target_port"),
                target_address,
                target_port,
            )
        )
    if tunnel_port:
        return tunnel_ip, tunnel_port

[dwapstra]

Please address the three inline comments on NETCONF endpoint selection and SSH tunnel lifecycle before merging.

[dwapstra]

Please remove the Yang-side changes in this PR. Normal dynamic port changes are already handled through the Genie management-port API and Yang main accepts a per-connection port override: Netconf.__init__ stores constructor kwargs on the connection, and connect() applies those attributes to the connection defaults. The caller flow is:

state = device.api.configure_management_port(
    service="netconf",
    port=requested_port,
)
try:
    device.connect(
        alias="dynamic_netconf",
        via="netconf",
        port=state.effective_port,
    )
    # Use the NETCONF session.
finally:
    device.dynamic_netconf.disconnect()
    device.api.restore_management_port(state=state)

The Genie API should own changing and restoring the IOS XE listener and QEMU forwarding. Yang does not need its own management-port transaction or custom tunnel lifecycle for the direct connection path, so please remove the connector changes, their duplicate tests, and the changelog entry.

If optional SSH-tunnel support is required, keep it narrowly scoped to the connector: use the connection ip/host with the runtime effective_port; call sshtunnel.auto_tunnel_add(..., target_address=..., target_port=..., return_lease=True); pass the returned local bind address and port to ncclient after applying connector attributes; and call auto_tunnel_release() on connect failure, disconnect, and exit cleanup. Store the lease on this NETCONF connection and do not replace tunnels created by other consumers. Do not pair a guest IP with the host-forward port or reconfigure the service from Yang.

[dwapstra]

[P1 test gap] This test asserts the invalid pair guest_ip:effective_port: guest_ip is inside the QEMU guest, while effective_port is the host-forward listener port. The mocked tunnel hides the fact that this endpoint cannot be reached. Assert the generated connection ip/host with effective_port instead, or use guest_ip with the guest service port.