From f16dd121b8f60afc701516424f06fda432096a62 Mon Sep 17 00:00:00 2001 From: Nick Townsend Date: Fri, 6 Feb 2015 10:24:59 -0800 Subject: [PATCH 1/3] Add tests for duplication of servers with users and ports When a server is specified more than once, either in a role or server definition and the user or port property is set a seperate instance is created. There should be only one definition with the last user & port properties winning --- spec/integration/dsl_spec.rb | 32 +++++++++++++++---- .../capistrano/configuration/server_spec.rb | 13 ++++++-- .../capistrano/configuration/servers_spec.rb | 24 +++++++++++++- 3 files changed, 59 insertions(+), 10 deletions(-) diff --git a/spec/integration/dsl_spec.rb b/spec/integration/dsl_spec.rb index e4014fd4..a43b4bbc 100644 --- a/spec/integration/dsl_spec.rb +++ b/spec/integration/dsl_spec.rb @@ -241,25 +241,45 @@ describe Capistrano::DSL do end describe 'fetching all servers' do - subject { dsl.roles(:all).map { |server| "#{server.user}@#{server.hostname}:#{server.port}" } } - - it 'creates a server instance for each unique user@host:port combination' do - expect(subject).to eq %w{db@example1.com:1234 root@example1.com:1234 @example1.com:5678 deployer@example1.com:1234} + it 'creates one server per hostname, ignoring user and port combinations' do + expect(dsl.roles(:all).size).to eq(1) end end describe 'fetching servers for a role' do it 'roles defined using the `server` syntax are included' do - expect(dsl.roles(:web).size).to eq(2) + as = dsl.roles(:web).map { |server| "#{server.user}@#{server.hostname}:#{server.port}" } + expect(as.size).to eq(1) + expect(as[0]).to eq("deployer@example1.com:5678") end it 'roles defined using the `role` syntax are included' do - expect(dsl.roles(:app).size).to eq(2) + as = dsl.roles(:app).map { |server| "#{server.user}@#{server.hostname}:#{server.port}" } + expect(as.size).to eq(1) + expect(as[0]).to eq("deployer@example1.com:5678") end end end + describe 'when setting user and port' do + subject { dsl.roles(:all).map { |server| "#{server.user}@#{server.hostname}:#{server.port}" }.first } + + describe "using the :user property" do + it "takes precedence over in the host string" do + dsl.server 'db@example1.com:1234', roles: %w{db}, active: true, user: 'brian' + expect(subject).to eq("brian@example1.com:1234") + end + end + + describe "using the :port property" do + it "takes precedence over in the host string" do + dsl.server 'db@example1.com:9090', roles: %w{db}, active: true, port: 1234 + expect(subject).to eq("db@example1.com:1234") + end + end + end + end describe 'setting and fetching variables' do diff --git a/spec/lib/capistrano/configuration/server_spec.rb b/spec/lib/capistrano/configuration/server_spec.rb index 1028dc09..ad81f3de 100644 --- a/spec/lib/capistrano/configuration/server_spec.rb +++ b/spec/lib/capistrano/configuration/server_spec.rb @@ -33,7 +33,7 @@ module Capistrano end describe 'comparing identity' do - subject { server.matches? Server[hostname] } + subject { server.hostname == Server[hostname].hostname } context 'with the same user, hostname and port' do let(:hostname) { 'root@hostname:1234' } @@ -42,12 +42,12 @@ module Capistrano context 'with a different user' do let(:hostname) { 'deployer@hostname:1234' } - it { expect(subject).to be_falsey } + it { expect(subject).to be_truthy } end context 'with a different port' do let(:hostname) { 'root@hostname:5678' } - it { expect(subject).to be_falsey } + it { expect(subject).to be_truthy } end context 'with a different hostname' do @@ -94,6 +94,10 @@ module Capistrano it 'sets the user' do expect(server.user).to eq 'tomc' end + + it 'sets the netssh_options user' do + expect(server.netssh_options[:user]).to eq 'tomc' + end end context 'properties contains port' do @@ -285,6 +289,9 @@ module Capistrano it 'contains correct user' do expect(server.netssh_options[:user]).to eq 'another_user' end + it 'does not affect server user in host' do + expect(server.user).to eq 'user_name' + end it 'contains keys' do expect(server.netssh_options[:keys]).to eq %w(/home/another_user/.ssh/id_rsa) end diff --git a/spec/lib/capistrano/configuration/servers_spec.rb b/spec/lib/capistrano/configuration/servers_spec.rb index efacd189..6582524f 100644 --- a/spec/lib/capistrano/configuration/servers_spec.rb +++ b/spec/lib/capistrano/configuration/servers_spec.rb @@ -18,6 +18,12 @@ module Capistrano expect(servers.count).to eq 1 end + it 'handles de-duplification within roles with users' do + servers.add_role(:app, %w{1}, user: 'nick') + servers.add_role(:app, %w{1}, user: 'fred') + expect(servers.count).to eq 1 + end + it 'accepts instances of server objects' do servers.add_role(:app, [Capistrano::Configuration::Server.new('example.net'), 'example.com']) expect(servers.roles_for([:app]).length).to eq 2 @@ -134,7 +140,23 @@ module Capistrano servers.add_host('1', roles: [:app, 'web'], test: :value, user: 'root', port: 34) servers.add_host('1', roles: [:app, 'web'], test: :value, user: 'deployer', port: 34) servers.add_host('1', roles: [:app, 'web'], test: :value, user: 'deployer', port: 56) - expect(servers.count).to eq(8) + expect(servers.count).to eq(1) + end + + describe "with a :user property" do + + it 'sets the server ssh username' do + servers.add_host('1', roles: [:app, 'web'], user: 'nick') + expect(servers.count).to eq(1) + expect(servers.roles_for([:all]).first.user).to eq 'nick' + end + + it 'overwrites the value of a user specified in the hostname' do + servers.add_host('brian@1', roles: [:app, 'web'], user: 'nick') + expect(servers.count).to eq(1) + expect(servers.roles_for([:all]).first.user).to eq 'nick' + end + end it 'overwrites the value of a previously defined scalar property' do From 8125102ae62abe2c7eb1dc116af2966fe2eb0312 Mon Sep 17 00:00:00 2001 From: Nick Townsend Date: Fri, 6 Feb 2015 10:31:40 -0800 Subject: [PATCH 2/3] Match servers only on hostname not user and port Servers must now be kept in an array and not a Set as user and port are part of the hash value for an SSHKit Host. So changing them after they're in a Set is not advised. More role_properties tests --- lib/capistrano/configuration/server.rb | 2 +- lib/capistrano/configuration/servers.rb | 18 +++++++++--------- spec/integration/dsl_spec.rb | 20 ++++++++++++++++++++ 3 files changed, 30 insertions(+), 10 deletions(-) diff --git a/lib/capistrano/configuration/server.rb b/lib/capistrano/configuration/server.rb index 9cab82b1..6d3cbd67 100644 --- a/lib/capistrano/configuration/server.rb +++ b/lib/capistrano/configuration/server.rb @@ -62,7 +62,7 @@ module Capistrano end def matches?(other) - user == other.user && hostname == other.hostname && port == other.port + hostname == other.hostname end private diff --git a/lib/capistrano/configuration/servers.rb b/lib/capistrano/configuration/servers.rb index 9f2809a9..7bc2fddc 100644 --- a/lib/capistrano/configuration/servers.rb +++ b/lib/capistrano/configuration/servers.rb @@ -8,7 +8,14 @@ module Capistrano include Enumerable def add_host(host, properties={}) - servers.add server(host, properties).with(properties) + new_host = Server[host] + if server = servers.find { |s| s.matches? new_host } + server.user = new_host.user if new_host.user + server.port = new_host.port if new_host.port + server.with(properties) + else + servers << new_host.with(properties) + end end def add_role(role, hosts, options={}) @@ -50,15 +57,8 @@ module Capistrano private - def server(host, properties) - new_host = Server[host] - new_host.with({user: properties[:user]}) unless properties[:user].nil? - new_host.with({port: properties[:port]}) unless properties[:port].nil? - servers.find { |server| server.matches? new_host } || new_host - end - def servers - @servers ||= Set.new + @servers ||= [] end def extract_options(array) diff --git a/spec/integration/dsl_spec.rb b/spec/integration/dsl_spec.rb index a43b4bbc..49653a3d 100644 --- a/spec/integration/dsl_spec.rb +++ b/spec/integration/dsl_spec.rb @@ -558,6 +558,26 @@ describe Capistrano::DSL do recipient.doit(host, role, props) end end + + it 'yields the merged properties for multiple roles' do + recipient = mock('recipient') + recipient.expects(:doit).with('example1.com', :redis, { port: 6379, type: :slave}) + recipient.expects(:doit).with('example2.com', :redis, { port: 6379, type: :master}) + recipient.expects(:doit).with('example1.com', :web, { port: 80 }) + recipient.expects(:doit).with('example2.com', :web, { port: 81 }) + dsl.role_properties(:redis, :web) do |host, role, props| + recipient.doit(host, role, props) + end + end + + it 'honours a property filter before yielding' do + recipient = mock('recipient') + recipient.expects(:doit).with('example1.com', :redis, { port: 6379, type: :slave}) + recipient.expects(:doit).with('example1.com', :web, { port: 80 }) + dsl.role_properties(:redis, :web, select: :active) do |host, role, props| + recipient.doit(host, role, props) + end + end end end From 837eaca8b1585c414ddc0718a55fe0f592b3c79a Mon Sep 17 00:00:00 2001 From: Nick Townsend Date: Thu, 12 Feb 2015 17:34:09 -0800 Subject: [PATCH 3/3] Copy the servers yielded to on() This allows the SSH connection attributes to be temporarily overridden --- CHANGELOG.md | 9 +++++++++ features/sshconnect.feature | 17 +++++++++++++++++ features/step_definitions/setup.rb | 4 ++++ lib/capistrano/configuration/server.rb | 2 +- lib/capistrano/dsl.rb | 4 ++-- spec/support/Vagrantfile | 11 +++++++++++ spec/support/tasks/root.rake | 7 +++++++ spec/support/test_app.rb | 2 +- 8 files changed, 52 insertions(+), 4 deletions(-) create mode 100644 features/sshconnect.feature create mode 100644 spec/support/tasks/root.rake diff --git a/CHANGELOG.md b/CHANGELOG.md index 3e8df3c4..7090179c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,15 @@ Reverse Chronological Order: * release_roles did not honour additional property filtering (@townsen) * Refactored and simplified property filtering code (@townsen) +* Breaking Changes + * Hosts with the same name are now consolidated into one irrespective of the + user and port. This allows multiple declarations of a server to be made safely. + The last declared properties will win. See capistrnorb.com Properties documentation + for details. + * Inside the on() block the host variable is now a copy of the host, so changes can be + made within the block (such as dynamically overriding the user) that will not persist. + This is very convenient for switching the SSH user temporarily to 'root' for example. + * Minor changes * Add role_properties() method (see capistrano.github.io PR for doc) (@townsen) * Add equality syntax ( eg. port: 1234) for property filtering (@townsen) diff --git a/features/sshconnect.feature b/features/sshconnect.feature new file mode 100644 index 00000000..addc1c5c --- /dev/null +++ b/features/sshconnect.feature @@ -0,0 +1,17 @@ +Feature: SSH Connection + + Background: + Given a test app with the default configuration + And servers with the roles app and web + And a task which executes as root + + Scenario: Switching from default user to root and back again + When I run cap "git:check" + Then the task is successful + And references in the remote repo are listed + When I run cap "am_i_root" + Then the task is successful + And contains "root" in the output + Then I run cap "git:check" + Then the task is successful + And references in the remote repo are listed diff --git a/features/step_definitions/setup.rb b/features/step_definitions/setup.rb index b7c72f39..8fc1e1c8 100644 --- a/features/step_definitions/setup.rb +++ b/features/step_definitions/setup.rb @@ -27,6 +27,10 @@ Given(/^a custom task to generate a file$/) do TestApp.copy_task_to_test_app('spec/support/tasks/database.rake') end +Given(/^a task which executes as root$/) do + TestApp.copy_task_to_test_app('spec/support/tasks/root.rake') +end + Given(/config stage file has line "(.*?)"/) do |line| TestApp.append_to_deploy_file(line) end diff --git a/lib/capistrano/configuration/server.rb b/lib/capistrano/configuration/server.rb index 6d3cbd67..35c55d34 100644 --- a/lib/capistrano/configuration/server.rb +++ b/lib/capistrano/configuration/server.rb @@ -98,7 +98,7 @@ module Capistrano @properties[key] end - def respond_to?(method) + def respond_to?(method, include_all=false) @properties.has_key?(method) end diff --git a/lib/capistrano/dsl.rb b/lib/capistrano/dsl.rb index b93199f4..431967e3 100644 --- a/lib/capistrano/dsl.rb +++ b/lib/capistrano/dsl.rb @@ -51,8 +51,8 @@ module Capistrano end def on(hosts, options={}, &block) - subset = Configuration.env.filter hosts - SSHKit::Coordinator.new(subset).each(options, &block) + subset_copy = Marshal.dump(Configuration.env.filter(hosts)) + SSHKit::Coordinator.new(Marshal.load(subset_copy)).each(options, &block) end def run_locally(&block) diff --git a/spec/support/Vagrantfile b/spec/support/Vagrantfile index f29e253d..7ef80db9 100644 --- a/spec/support/Vagrantfile +++ b/spec/support/Vagrantfile @@ -1,3 +1,5 @@ +require 'open-uri' + Vagrant.configure("2") do |config| config.ssh.insert_key = false @@ -8,6 +10,15 @@ Vagrant.configure("2") do |config| config.vm.box = 'hashicorp/precise64' config.vm.network "forwarded_port", guest: 22, host: "222#{i}".to_i config.vm.provision :shell, inline: 'sudo apt-get -y install git-core' + + vagrantkey = open("https://raw.githubusercontent.com/mitchellh/vagrant/master/keys/vagrant.pub", "r",&:read) + + config.vm.provision :shell, + inline: <<-INLINE + install -d -m 700 /root/.ssh + echo -e "#{vagrantkey}" > /root/.ssh/authorized_keys + chmod 0600 /root/.ssh/authorized_keys + INLINE end end end diff --git a/spec/support/tasks/root.rake b/spec/support/tasks/root.rake new file mode 100644 index 00000000..82e31143 --- /dev/null +++ b/spec/support/tasks/root.rake @@ -0,0 +1,7 @@ +task :am_i_root do + on roles(:all) do |host| + host.user = 'root' + ident = capture :id, '-a' + info "I am #{ident}" + end +end diff --git a/spec/support/test_app.rb b/spec/support/test_app.rb index 36d007ad..c5d9eaac 100644 --- a/spec/support/test_app.rb +++ b/spec/support/test_app.rb @@ -13,7 +13,7 @@ module TestApp set :deploy_to, '#{deploy_to}' set :repo_url, 'git://github.com/capistrano/capistrano.git' set :branch, 'master' - set :ssh_options, { keys: "\#{ENV['HOME']}/.vagrant.d/insecure_private_key" } + set :ssh_options, { keys: "\#{ENV['HOME']}/.vagrant.d/insecure_private_key", auth_methods: ['publickey'] } server 'vagrant@localhost:2220', roles: %w{web app} set :linked_files, #{linked_files} set :linked_dirs, #{linked_dirs}