Add transport for Cisco IOS

[jerryaldrichiii]

This adds transport support for Cisco IOS devices.

Given that the IOS SSH server closes the SSH channel after the first read (see: net-ssh/net-ssh#24), it is required that we ensure the SSH channel stays open.

This is done here by creating a channel, creating a buffer to receive data/manipulate that data on that channel, and requesting a shell. This, coupled with the handling of the lack of exit codes and providing a method for escalating to enable mode, should allow support for Cisco IOS devices.

[jquick]

@jerryaldrichiii Really nice work on this! Just a few comments.

[jquick]

Do you need this if your going to just call super on it? It should just call the superclass one if not defined.

[jerryaldrichiii]

Nope, you're right. Will remove.

[jquick]

You mind adding a unit test for this? I can see this growing as we get more error texts.

[jerryaldrichiii]

Yup, should be done in latest commit.

[jquick]

Also a unit test here would be nice for all the output subing

[jquick]

Looking good! Thanks @jerryaldrichiii

[TrevorBramble]

As far as better ways to organize this, it seems like they're all just string literals (when 'Bad IP address' and so forth would have worked too) and they all should be treated as exit 1, so this is what came to mind. Basically just extract the data from the logical flow.

ERROR_STRINGS = [
  'Bad IP address',
  'Incomplete command',
  'Invalid input detected',
  'Unrecognized host'
]

# IOS commands do not have an exit code, so we must capture known errors
def format_result(result)
  stderr_with_exit_1 = ['', result, 1]
  stdout_with_exit_0 = [result, '', 0]

  ERROR_STRINGS.include?(result) ? stderr_with_exit_1 : stdout_with_exit_0
end

I debated inlining the return values, but I really like those explaining variables!

(Also I'm not in love with the name ERROR_STRINGS.)

[TrevorBramble]

Previously this was @options.clone.delete_if { |_key, value| value.nil? } and I presumed the intent was to avoid mutating @options even if you don't want the entries with nil values for this statement, and that's why I suggested @options.reject { |_, value| value.nil? } (which doesn't mutate anything at all) instead.

So I just wanted to make sure there wasn't confusion here. Maybe it's OK to strip the empty pairs here, but it seems like a thing that could cause trouble if you're not expecting it elsewhere.

[jerryaldrichiii]

Nope, I see what you mean now, I'll go with reject here.

[TrevorBramble]

String#sub and String#gsub return the modified string, so you can chain them. Also, giving the regular expression matchers explaining variables can help avoid an almost certain fate of the regexp being changed, but not the comment that explains it.

def format_output(output, cmd)
  leading_prompt        = /(\r\n|^)\S+[>#]/
  command_string        = /#{cmd}\r\n/
  trailing_prompt       = /\S+[>#](\r\n|$)/
  trailing_line_endings = /(\r\n)+$/

  output
    .sub(leading_prompt, '')
    .sub(command_string, '')
    .gsub(trailing_prompt, '')
    .gsub(trailing_line_endings, '')
end

[TrevorBramble]

Do you actually want to validate the options every time or should this instead return an existing connection or validate and connect?

Oh, I looked up validate_options; it returns the validated options, so you can pass it right through. =^)

def connection
  @connection ||= Connection.new(validate_options(@options))
end

[jerryaldrichiii]

I was unable to get that to work. Looks like it adds some other stuff in the return object

See: https://gist.github.com/00c569cfe7ac85ff6eb397f8d74b7a3d

I could do:

@connection ||= Connection.new(validate_options(@options).options)

I pushed that up in the latest, commit. Let me know if that is incorrect in some way. I definitely only want to call it if @connection doesn't exist.

[TrevorBramble]

So much better! =^) One thing to maybe ignore, and then that one fiddly patch of code could I think be seriously improved. Gave an example of one possible approach.

[TrevorBramble]

[nit] Is there a reason for these? In Ruby, instance variables are nil by default, and attempting to access the value of one without having declared it already just returns nil. @session should only ever be assigned or accessed via session(), even. @buf may be shared by those two methods, but run_command_via_connection() clears it on each invocation anyway.

[jerryaldrichiii]

Nope, you make a solid argument. Killing those with fire.

[TrevorBramble]

I put my finger on why these bother me. One of them is always a spurious assignment. At this point I'd definitely either extract them as methods (format_err(result), format_out(result)) or inline the arrays, or inline CommandResult.new().

def format_result(result)
  if ERROR_MATCHERS.none? { |e| result.include?(e) }
    CommandResult.new(result, '', 0)
  else
    CommandResult.new('', result, 1)
  end
end

I do wish there were CommandResult subclasses that revealed intent though. ErrorResult.new(result) which automatically assigns result to stderr and returns 1, etc.

[jerryaldrichiii]

I'm glad you put a finger on it. I took your code suggestion and ran with it. Let me know if it's any better.

[TrevorBramble]

Love it! Great work!