Stateful/advanced autoresponding #369

Closed
bitprophet opened this Issue Jun 26, 2016 · 4 comments

Projects

None yet

1 participant

@bitprophet
Member
bitprophet commented Jun 26, 2016 edited

Spinoff of #289 but more specifically, #294. Also dovetails with #368 re: the possible need for the responses configuration being much more than a simple call/response dict.

Background

sudo (and many other prompt situations) can be handled via the naive implementation of autoresponding from #289: see string, respond with string, done.

However, in sudo's case at least, this can lead to annoying cases where an incorrect sudo password results in responding to the same prompt over and over until the remote sudoers config's passwd_tries number of tries is exceeded.

In Fabric v1, the authentication machinery was hardwired to look out for sudo's Sorry, try again response and to raise a failure flag in that case (which then affected the rest of the flow, typically by re-prompting the console user).

It would be nice to extend the autoresponse functionality so that sort of 'stateful' setup is possible, though I suspect it would be pretty complex.

Brainstorm

What's needed for the sudo use case to work better:

  • Some flag determining whether the response has been submitted & "failed" somehow; if this flag is tripped, the response machinery should probably error (or try something else, perhaps).
    • E.g. for the purposes of Context.sudo, raising an error feels nice and clean, we can catch it, bother the user, and then try again; other use cases can do whatever it is that's appropriate in their case.
    • If we did go this "stop fast/hard" route, we'd need to ensure we terminated the subprocess cleanly, probably by submitting an artificial Ctrl-C (same as when we "pass through" a real Ctrl-C).
  • In turn, that means each autoresponse key/value needs a way of recording what this failure state looks like.
    • E.g. for sudo it might simply be "a string which, if seen, indicates failure" and that's set to "Sorry, try again".
    • Could potentially be a callback or similar. In fact, the entire thing - naive and complex - could probably be recast as callbacks, with the dict key transitioning from active pattern to label.
      • Could also then use callback factories or whatever, e.g. responses['sudo'] = respond_to_prompt(trigger='sudo password:', response='mypassword', failure='Sorry, try again').
      • Where respond_to_prompt returns a callable taking as input the current value of the text stream, and a handle on the stdin writer (or which returns or yields the data to be written to stdin, if any).
      • The existing behavior could be implemented that way pretty easily, I think.
      • And the callables (again, which could be anything, not just the ones returned by builtin helper factories) could keep their own state (esp if yielding / acting as generators), which would allow for the statefulness needed for failure detection - or anything else users could want.
      • Also means each callable can do its own thing when it detects failure (and means we're not hard-coding the fact that failure detection is the only other thing in play besides regular responding). Basically, each of these is an independently tracked copy of the sudo parts of Fabric 1's io loop.
  • Going with the above-outlined callback oriented setup also means that the feature isn't even really "auto-responding" but simply a set of hooks on the stream processing.
    • Users (or core) could do literally anything with it, including the existing mirroring to stdout, "live" logging of output (though this is already possible if users submit their own stdout stream - but again, that circles back to the mirroring itself, which could stop being as much of a special case perhaps) and so on.
      • Insofar as it ties into overall logging, see #15
@bitprophet
Member

Realized while quickly trialing this that I was confusing regular callbacks with generators/coroutines, and also that the latter don't work well for this because the caller (the output handler) needs a response to the stream data it's passing in, not the previous stream data (which is what would happen with a loop + data = yield response or similar).

To properly track state, I think we need bona fide objects + methods here. Probably cleaner / easier to share/reuse anyways?

@bitprophet
Member

Using a basic object seems to work well, tho I took out the threadlocal indices (because each obj can now store its own index) and surprise, that's no longer threadsafe. So these Responder (off the cuff name) objects need to be made thread-safe / thread-distinct, e.g.:

  • Storing two copies of each registered one, one for stdout and one for stderr. These aren't exactly heavy objects so that's probably not a huge deal.
  • Somehow annotate member attributes of the Responder so some are clearly threadsafe (while others, like the pattern & response values, don't need to be). Feels error prone and messy.
  • Just...subclass threading.local directly, so the entire object acts thread-local? I feel like there could be some strange side effects of this, but nosing around the Python docs implies it's a thing that is done. EDIT: sure enough, simply s/object/threading.local/ fixes things up and doesn't seem otherwise mad...so gonna go with that for now.
@bitprophet
Member
bitprophet commented Jun 27, 2016 edited

Made the API StreamWatcher + .submit, seems general enough, shrug. TODO:

  • Implement the actual "keeps track of failure or not" behavior needed here...make sure it actually works
  • Add unit tests for StreamWatcher itself (and Responder specifically, its only subclass right now). Right now just been relying on the regular tests as regression tests, i.e. it's implementation specific, but since it needs to be public API, needs its own tests.
  • Finish pushing the new API upwards so it is public, i.e.:
    • change run(responses={}) to run(watchers=[]), including explicit reference to StreamWatcher API docs
    • ensure StreamWatcher and Responder (and the stateful sudo-specific responder, whatever that becomes) do appear in API docs
@bitprophet bitprophet added a commit that referenced this issue Aug 30, 2016
@bitprophet bitprophet Start generalizing prompt response stuff re #369.
Created real-enough watcher API and started using it internally.

Still needs surfacing at top of public API
f55efff
@bitprophet
Member

This has been done for a while now; some notes about it are in the end of #294 because I'm kind of dumb.

@bitprophet bitprophet added a commit that referenced this issue Sep 19, 2016
@bitprophet bitprophet Changelog re #369, closes #369 f2f2027
@bitprophet bitprophet referenced this issue Sep 23, 2016
Closed

Prompt/etc autoresponder #289

14 of 15 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment