Ensure ssh search paginates correctlytm/fix_ssh_search

Also use partial search for much small on-the-wire sizes

Signed-off-by: Thom May <thom@chef.io>

3 files changed, 74 insertions, 65 deletions

diff --git a/lib/chef/knife/ssh.rb b/lib/chef/knife/ssh.rb
index 53801afffd..6797f6cab2 100644
--- a/lib/chef/knife/ssh.rb
+++ b/lib/chef/knife/ssh.rb
@@ -166,10 +166,10 @@ class Chef
       def configure_session
         list = config[:manual] ? @name_args[0].split(" ") : search_nodes
         if list.length == 0
-          if @action_nodes.length == 0
+          if @search_count == 0
             ui.fatal("No nodes returned from search")
           else
-            ui.fatal("#{@action_nodes.length} #{@action_nodes.length > 1 ? "nodes" : "node"} found, " +
+            ui.fatal("#{@search_count} #{@search_count > 1 ? "nodes" : "node"} found, " +
                      "but does not have the required attribute to establish the connection. " +
                      "Try setting another attribute to open the connection using --attribute.")
           end
@@ -184,41 +184,56 @@ class Chef
         # 2) configuration file
         # 3) cloud attribute
         # 4) fqdn
-        if config[:attribute]
-          Chef::Log.debug("Using node attribute '#{config[:attribute]}' as the ssh target")
-          attribute = config[:attribute]
+        if node["config"]
+          Chef::Log.debug("Using node attribute '#{config[:attribute]}' as the ssh target: #{node["config"]}")
+          node["config"]
         elsif Chef::Config[:knife][:ssh_attribute]
-          Chef::Log.debug("Using node attribute #{Chef::Config[:knife][:ssh_attribute]}")
-          attribute = Chef::Config[:knife][:ssh_attribute]
-        elsif node[:cloud] &&
-            node[:cloud][:public_hostname] &&
-            !node[:cloud][:public_hostname].empty?
-          Chef::Log.debug("Using node attribute 'cloud[:public_hostname]' automatically as the ssh target")
-          attribute = "cloud.public_hostname"
+          Chef::Log.debug("Using node attribute #{Chef::Config[:knife][:ssh_attribute]}: #{node["knife_config"]}")
+          node["knife_config"]
+        elsif node["cloud"] &&
+            node["cloud"]["public_hostname"] &&
+            !node["cloud"]["public_hostname"].empty?
+          Chef::Log.debug("Using node attribute 'cloud[:public_hostname]' automatically as the ssh target: #{node["cloud"]["public_hostname"]}")
+          node["cloud"]["public_hostname"]
         else
           # falling back to default of fqdn
-          Chef::Log.debug("Using node attribute 'fqdn' as the ssh target")
-          attribute = "fqdn"
+          Chef::Log.debug("Using node attribute 'fqdn' as the ssh target: #{node["fqdn"]}")
+          node["fqdn"]
         end
-        attribute
       end
 
       def search_nodes
         list = Array.new
         query = Chef::Search::Query.new
-        @action_nodes = query.search(:node, @name_args[0])[0]
-        @action_nodes.each do |item|
+        required_attributes = { fqdn: ["fqdn"], cloud: ["cloud"] }
+
+        separator = ui.presenter.attribute_field_separator
+
+        # if we've set an attribute to use on the command line
+        if config[:attribute]
+          required_attributes[:config] = config[:attribute].split(separator)
+        end
+
+        # if we've configured an attribute in our config
+        if Chef::Config[:knife][:ssh_attribute]
+          required_attributes[:knife_config] = Chef::Config[:knife][:ssh_attribute].split(separator)
+        end
+
+        @search_count = 0
+        query.search(:node, @name_args[0], filter_result: required_attributes) do |item|
+          @search_count += 1
           # we should skip the loop to next iteration if the item
           # returned by the search is nil
           next if item.nil?
           # next if we couldn't find the specified attribute in the
           # returned node object
-          host = extract_nested_value(item, get_ssh_attribute(item))
+          host = get_ssh_attribute(item)
           next if host.nil?
           ssh_port = item[:cloud].nil? ? nil : item[:cloud][:public_ssh_port]
           srv = [host, ssh_port]
           list.push(srv)
         end
+
         list
       end
 
@@ -309,9 +324,9 @@ class Chef
         subsession ||= session
         command = fixup_sudo(command)
         command.force_encoding("binary") if command.respond_to?(:force_encoding)
-        subsession.open_channel do |ch|
-          ch.request_pty
-          ch.exec command do |ch, success|
+        subsession.open_channel do |chan|
+          chan.request_pty
+          chan.exec command do |ch, success|
             raise ArgumentError, "Cannot execute #{command}" unless success
             ch.on_data do |ichannel, data|
               print_data(ichannel[:host], data)
@@ -532,10 +547,6 @@ class Chef
         config[:ssh_identity_file] = get_stripped_unfrozen_value(config[:ssh_identity_file] || config[:identity_file] || Chef::Config[:knife][:ssh_identity_file])
       end
 
-      def extract_nested_value(data_structure, path_spec)
-        ui.presenter.extract_nested_value(data_structure, path_spec)
-      end
-
       def run
         @longest = 0
 
diff --git a/spec/functional/knife/ssh_spec.rb b/spec/functional/knife/ssh_spec.rb
index 065b646ac6..aea7585bb2 100644
--- a/spec/functional/knife/ssh_spec.rb
+++ b/spec/functional/knife/ssh_spec.rb
@@ -181,7 +181,7 @@ describe Chef::Knife::Ssh do
 
       it "uses the ssh_attribute" do
         @knife.run
-        expect(@knife.get_ssh_attribute(Chef::Node.new)).to eq("ec2.public_hostname")
+        expect(@knife.get_ssh_attribute({ "knife_config" => "ec2.public_hostname" })).to eq("ec2.public_hostname")
       end
     end
 
@@ -193,11 +193,11 @@ describe Chef::Knife::Ssh do
 
       it "uses the default" do
         @knife.run
-        expect(@knife.get_ssh_attribute(Chef::Node.new)).to eq("fqdn")
+        expect(@knife.get_ssh_attribute({ "fqdn" => "fqdn" })).to eq("fqdn")
       end
     end
 
-    context "when -a ec2.public_ipv4 is provided" do
+    context "when -a ec2.public_public_hostname is provided" do
       before do
         setup_knife(["-a ec2.public_hostname", "*:*", "uptime"])
         Chef::Config[:knife][:ssh_attribute] = nil
@@ -276,8 +276,8 @@ describe Chef::Knife::Ssh do
     Chef::Config[:client_key] = nil
     Chef::Config[:chef_server_url] = "http://localhost:9000"
 
-    @api.get("/search/node?q=*:*&sort=X_CHEF_id_CHEF_X%20asc&start=0", 200) do
-      %({"total":1, "start":0, "rows":[{"name":"i-xxxxxxxx", "json_class":"Chef::Node", "automatic":{"fqdn":"the.fqdn", "ec2":{"public_hostname":"the_public_hostname"}},"recipes":[]}]})
+    @api.post("/search/node?q=*:*&sort=X_CHEF_id_CHEF_X%20asc&start=0", 200) do
+      %({"total":1, "start":0, "rows":[{"data": {"fqdn":"the.fqdn", "config": "the_public_hostname", "knife_config": "the_public_hostname" }}]})
     end
   end
 
diff --git a/spec/unit/knife/ssh_spec.rb b/spec/unit/knife/ssh_spec.rb
index 9263a0b8e5..6141a8a6df 100644
--- a/spec/unit/knife/ssh_spec.rb
+++ b/spec/unit/knife/ssh_spec.rb
@@ -21,54 +21,53 @@ require "net/ssh"
 require "net/ssh/multi"
 
 describe Chef::Knife::Ssh do
-  before(:each) do
-    Chef::Config[:client_key] = CHEF_SPEC_DATA + "/ssl/private_key.pem"
-  end
+  let(:query_result) { double("chef search results") }
 
   before do
+    Chef::Config[:client_key] = CHEF_SPEC_DATA + "/ssl/private_key.pem"
     @knife = Chef::Knife::Ssh.new
     @knife.merge_configs
-    @node_foo = Chef::Node.new
-    @node_foo.automatic_attrs[:fqdn] = "foo.example.org"
-    @node_foo.automatic_attrs[:ipaddress] = "10.0.0.1"
+    @node_foo = {}
+    @node_foo["fqdn"] = "foo.example.org"
+    @node_foo["ipaddress"] = "10.0.0.1"
+    @node_foo["cloud"] = {}
+
+    @node_bar = {}
+    @node_bar["fqdn"] = "bar.example.org"
+    @node_bar["ipaddress"] = "10.0.0.2"
+    @node_bar["cloud"] = {}
 
-    @node_bar = Chef::Node.new
-    @node_bar.automatic_attrs[:fqdn] = "bar.example.org"
-    @node_bar.automatic_attrs[:ipaddress] = "10.0.0.2"
   end
 
   describe "#configure_session" do
     context "manual is set to false (default)" do
       before do
         @knife.config[:manual] = false
-        @query = Chef::Search::Query.new
-      end
-
-      def configure_query(node_array)
-        allow(@query).to receive(:search).and_return([node_array])
-        allow(Chef::Search::Query).to receive(:new).and_return(@query)
+        allow(query_result).to receive(:search).with(any_args).and_yield(@node_foo).and_yield(@node_bar)
+        allow(Chef::Search::Query).to receive(:new).and_return(query_result)
       end
 
       def self.should_return_specified_attributes
         it "returns an array of the attributes specified on the command line OR config file, if only one is set" do
+          @node_bar["config"] = "10.0.0.2"
+          @node_foo["config"] = "10.0.0.1"
           @knife.config[:attribute] = "ipaddress"
           Chef::Config[:knife][:ssh_attribute] = "ipaddress" # this value will be in the config file
-          configure_query([@node_foo, @node_bar])
           expect(@knife).to receive(:session_from_list).with([["10.0.0.1", nil], ["10.0.0.2", nil]])
           @knife.configure_session
         end
 
         it "returns an array of the attributes specified on the command line even when a config value is set" do
+          @node_bar["config"] = "10.0.0.2"
+          @node_foo["config"] = "10.0.0.1"
           Chef::Config[:knife][:ssh_attribute] = "config_file" # this value will be in the config file
           @knife.config[:attribute] = "ipaddress" # this is the value of the command line via #configure_attribute
-          configure_query([@node_foo, @node_bar])
           expect(@knife).to receive(:session_from_list).with([["10.0.0.1", nil], ["10.0.0.2", nil]])
           @knife.configure_session
         end
       end
 
       it "searchs for and returns an array of fqdns" do
-        configure_query([@node_foo, @node_bar])
         expect(@knife).to receive(:session_from_list).with([
           ["foo.example.org", nil],
           ["bar.example.org", nil],
@@ -80,11 +79,10 @@ describe Chef::Knife::Ssh do
 
       context "when cloud hostnames are available" do
         before do
-          @node_foo.automatic_attrs[:cloud][:public_hostname] = "ec2-10-0-0-1.compute-1.amazonaws.com"
-          @node_bar.automatic_attrs[:cloud][:public_hostname] = "ec2-10-0-0-2.compute-1.amazonaws.com"
+          @node_foo["cloud"]["public_hostname"] = "ec2-10-0-0-1.compute-1.amazonaws.com"
+          @node_bar["cloud"]["public_hostname"] = "ec2-10-0-0-2.compute-1.amazonaws.com"
         end
         it "returns an array of cloud public hostnames" do
-          configure_query([@node_foo, @node_bar])
           expect(@knife).to receive(:session_from_list).with([
             ["ec2-10-0-0-1.compute-1.amazonaws.com", nil],
             ["ec2-10-0-0-2.compute-1.amazonaws.com", nil],
@@ -97,12 +95,11 @@ describe Chef::Knife::Ssh do
 
       context "when cloud hostnames are available but empty" do
         before do
-          @node_foo.automatic_attrs[:cloud][:public_hostname] = ""
-          @node_bar.automatic_attrs[:cloud][:public_hostname] = ""
+          @node_foo["cloud"]["public_hostname"] = ""
+          @node_bar["cloud"]["public_hostname"] = ""
         end
 
         it "returns an array of fqdns" do
-          configure_query([@node_foo, @node_bar])
           expect(@knife).to receive(:session_from_list).with([
             ["foo.example.org", nil],
             ["bar.example.org", nil],
@@ -114,7 +111,7 @@ describe Chef::Knife::Ssh do
       end
 
       it "should raise an error if no host are found" do
-        configure_query([ ])
+        allow(query_result).to receive(:search).with(any_args)
         expect(@knife.ui).to receive(:fatal)
         expect(@knife).to receive(:exit).with(10)
         @knife.configure_session
@@ -122,10 +119,8 @@ describe Chef::Knife::Ssh do
 
       context "when there are some hosts found but they do not have an attribute to connect with" do
         before do
-          allow(@query).to receive(:search).and_return([[@node_foo, @node_bar]])
-          @node_foo.automatic_attrs[:fqdn] = nil
-          @node_bar.automatic_attrs[:fqdn] = nil
-          allow(Chef::Search::Query).to receive(:new).and_return(@query)
+          @node_foo["fqdn"] = nil
+          @node_bar["fqdn"] = nil
         end
 
         it "should raise a specific error (CHEF-3402)" do
@@ -158,31 +153,34 @@ describe Chef::Knife::Ssh do
     before do
       Chef::Config[:knife][:ssh_attribute] = nil
       @knife.config[:attribute] = nil
-      @node_foo.automatic_attrs[:cloud][:public_hostname] = "ec2-10-0-0-1.compute-1.amazonaws.com"
-      @node_bar.automatic_attrs[:cloud][:public_hostname] = ""
+      @node_foo["cloud"]["public_hostname"] = "ec2-10-0-0-1.compute-1.amazonaws.com"
+      @node_bar["cloud"]["public_hostname"] = ""
     end
 
     it "should return fqdn by default" do
-      expect(@knife.get_ssh_attribute(Chef::Node.new)).to eq("fqdn")
+      expect(@knife.get_ssh_attribute({ "fqdn" => "fqdn" })).to eq("fqdn")
     end
 
     it "should return cloud.public_hostname attribute if available" do
-      expect(@knife.get_ssh_attribute(@node_foo)).to eq("cloud.public_hostname")
+      expect(@knife.get_ssh_attribute(@node_foo)).to eq("ec2-10-0-0-1.compute-1.amazonaws.com")
     end
 
     it "should favor to attribute_from_cli over config file and cloud" do
       @knife.config[:attribute] = "command_line"
       Chef::Config[:knife][:ssh_attribute] = "config_file"
+      @node_foo["config"] = "command_line"
+      @node_foo["knife_config"] = "config_file"
       expect( @knife.get_ssh_attribute(@node_foo)).to eq("command_line")
     end
 
     it "should favor config file over cloud and default" do
       Chef::Config[:knife][:ssh_attribute] = "config_file"
+      @node_foo["knife_config"] = "config_file"
       expect( @knife.get_ssh_attribute(@node_foo)).to eq("config_file")
     end
 
     it "should return fqdn if cloud.hostname is empty" do
-      expect( @knife.get_ssh_attribute(@node_bar)).to eq("fqdn")
+      expect( @knife.get_ssh_attribute(@node_bar)).to eq("bar.example.org")
     end
   end
 
@@ -294,7 +292,7 @@ describe Chef::Knife::Ssh do
   describe "#run" do
     before do
       @query = Chef::Search::Query.new
-      expect(@query).to receive(:search).and_return([[@node_foo]])
+      expect(@query).to receive(:search).and_yield(@node_foo)
       allow(Chef::Search::Query).to receive(:new).and_return(@query)
       allow(@knife).to receive(:ssh_command).and_return(exit_code)
       @knife.name_args = ["*:*", "false"]