Opened 4 years ago

Closed 4 years ago

#6532 enhancement closed fixed (fixed)

twisted.internet.endpoints.SSHCommandClientEndpoint.connect with new connections should return a cancellable Deferred

Reported by: Itamar Turner-Trauring Owned by: Itamar Turner-Trauring
Priority: normal Milestone:
Component: core Keywords:
Cc: Branch: branches/ssh-endpoint-cancellable-6532-2
branch-diff, diff-cov, branch-cov, buildbot
Author: itamarst

Description

If you are creating an SSH endpoint with a new connection each time, the Deferred returned for endpoint connection should be cancellable. It should call transport.abortConnnection on its TCP connection to abort the connection attempt if it's already connected and is in SSH protocol negotiation mode.

(This is a separate feature from cancellable reusable connections since those presumably don't want to close the shared TCP connection.)

Change History (14)

comment:1 Changed 4 years ago by itamarst

Author: itamarst
Branch: branches/ssh-endpoint-cancellable-6532

(In [38503]) Branching to 'ssh-endpoint-cancellable-6532'

comment:2 Changed 4 years ago by Itamar Turner-Trauring

Owner: set to Itamar Turner-Trauring
Status: newassigned

I believe the necessary work is:

  1. Extend the connection helpers with a destroyConnection method that does abortConnection on the protocol transport in the case of new connections helper. Presumably it does nothing for the existing connections helper.
  2. SSHCommandClientEndpoint._executeCommand should call destroyConnection rather than cleanupConnection on errors.
  3. Cancelling the factory.connectionReady Deferred should again call abortConnection on the transport.

comment:3 Changed 4 years ago by Jean-Paul Calderone

Extend the connection helpers with a destroyConnection method that does abortConnection on the protocol transport in the case of new connections helper. Presumably it does nothing for the existing connections helper

Hmm, that's too bad - considering there's already cleanupConnection. Do we have to double all the interfaces now, so we have one API to ask for loseConnection and one API to ask for abortConnection? :/

comment:4 Changed 4 years ago by Itamar Turner-Trauring

It's necessary in so far as cleanupConnection is also called in other code paths which want a clean cleanup, as it were. We could add an extra argument to cleanupConnection, instead of adding a new method, which should be fine as long as we merge this before next release since this is new code.

comment:5 Changed 4 years ago by Jean-Paul Calderone

(In [38627]) Test cancel while waiting for exec without monkey-patching an implementation detail of the endpoint

refs #6532

comment:6 Changed 4 years ago by Jean-Paul Calderone

(In [38628]) Merge private-conn-creator-interface-6541

Author: exarkun Reviewer: jonathanj Fixes: #6541

Change ISSHConnectionCreator to _ISSHConnectionCreator so as not to make it a part of the public interface of twisted.conch.endpoints, pending some changes we probably want to make to it (at least for cancellation - see #6532).

comment:7 Changed 4 years ago by itamarst

Branch: branches/ssh-endpoint-cancellable-6532branches/ssh-endpoint-cancellable-6532-2

(In [38630]) Branching to 'ssh-endpoint-cancellable-6532-2'

comment:8 Changed 4 years ago by Itamar Turner-Trauring

Keywords: review added
Owner: changed from Itamar Turner-Trauring to Jean-Paul Calderone
Status: assignednew

I've got a build running, haven't looked at it yet (actually two, you'll want to wait for the later one to finish).

comment:9 Changed 4 years ago by Jean-Paul Calderone

Status: newassigned

comment:10 Changed 4 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: changed from Jean-Paul Calderone to Itamar Turner-Trauring
Status: assignednew

Thanks. Just a couple points, mostly about encapsulation:

  1. _CommandTransport now uses a lot of attributes from its creator which are not documented as part of _ISSHConnectionCreator. This code will really only work with a _NewConnectionHelper instance. It's true _CommandTransport was quite closely tied to _NewConnectionHelper before this change, but at least the documentation doesn't suggest otherwise. Now it has only the creator and uses a mix of documented and undocumented attributes from it. I'm not sure what resolution I'd like to see for this. Maybe the interface just needs to document all of these attributes? They're hardly well-considered though, it's just the collection of things I needed to implement the endpoint. There are clear problems with the collection as a while - for example, the issue of having keys separate from an agent. And stuff is missing from it, for example it cannot support keyboard-interactive authentication as is.
  2. The connection.transport.transport.abortConnection() necessary in the implementation of cleanupConnection makes me sad. This kind of deep object usage seems to be often necessary when working with conch. Is there something we could do to make this better? Should the conch SSH transport class implement abortConnection to help out here?

Sorry about the general vagueness of those points. Since this is all private stuff, it's not massively urgent to resolve it before this ticket can be closed. It would be nice to have at least an inkling of a plan about where we're going to go from here, though.

Please merge when you're satisfied these points have been addressed somehow, even if not necessarily by fixing all of the internals to be completely perfect.

comment:11 Changed 4 years ago by Itamar Turner-Trauring

For the first item, how about just documenting that _CommandTransport only takes a _NewConnectionHelper, and then it's using expected public attributes?

comment:12 Changed 4 years ago by Jean-Paul Calderone

For the first item, how about just documenting that _CommandTransport only takes a _NewConnectionHelper, and then it's using expected public attributes?

Sure, that works.

comment:13 Changed 4 years ago by Itamar Turner-Trauring

OK, updated _CommandTransport.__init__ docstring, and added #6548 for point 2.

comment:14 Changed 4 years ago by itamarst

Resolution: fixed
Status: newclosed

(In [38660]) Merge ssh-endpoint-cancellable-6532-2: Cancellable endpoints for new-connection SSH endpoint.

Fixes: #6532 Author: itamar, exarkun Review: exarkun, itamar

If you use an SSH command endpoint that opens a new connection, you can cancel the connection attempt and SSH negotation by cancelling the connection Deferred.

Note: See TracTickets for help on using tickets.