knife bootstrap should only request license when installing Chef 15knife_license

Also updating the transmission of the license acceptance through the
config.rb because that brings it in line with existing patterns
(Test Kitchen).

Signed-off-by: tyler-ball <tball@chef.io>

8 files changed, 85 insertions, 32 deletions

diff --git a/Gemfile.lock b/Gemfile.lock
index c89b920cac..2c771557fd 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -41,7 +41,7 @@ PATH
       ffi-yajl (~> 2.2)
       highline (>= 1.6.9, < 2)
       iniparse (~> 1.4)
-      license-acceptance (~> 1.0)
+      license-acceptance (~> 1.0, >= 1.0.5)
       mixlib-archive (>= 0.4, < 2.0)
       mixlib-authentication (~> 2.1)
       mixlib-cli (>= 1.7, < 3.0)
@@ -72,7 +72,7 @@ PATH
       highline (>= 1.6.9, < 2)
       iniparse (~> 1.4)
       iso8601 (~> 0.12.1)
-      license-acceptance (~> 1.0)
+      license-acceptance (~> 1.0, >= 1.0.5)
       mixlib-archive (>= 0.4, < 2.0)
       mixlib-authentication (~> 2.1)
       mixlib-cli (>= 1.7, < 3.0)
diff --git a/chef.gemspec b/chef.gemspec
index 975d51dba3..4524da6d9a 100644
--- a/chef.gemspec
+++ b/chef.gemspec
@@ -18,7 +18,7 @@ Gem::Specification.new do |s|
   s.add_dependency "chef-config", "= #{Chef::VERSION}"
   s.add_dependency "train-core", "~> 2.0", ">= 2.0.12"
 
-  s.add_dependency "license-acceptance", "~> 1.0"
+  s.add_dependency "license-acceptance", "~> 1.0", ">= 1.0.5"
   s.add_dependency "mixlib-cli", ">= 1.7", "< 3.0"
   s.add_dependency "mixlib-log", ">= 2.0.3", "< 4.0"
   s.add_dependency "mixlib-authentication", "~> 2.1"
diff --git a/lib/chef/knife/bootstrap.rb b/lib/chef/knife/bootstrap.rb
index 90dd2417fc..6f6b31f2ec 100644
--- a/lib/chef/knife/bootstrap.rb
+++ b/lib/chef/knife/bootstrap.rb
@@ -398,9 +398,7 @@ class Chef
                                on: :tail)
       end
 
-      attr_accessor :client_builder
-      attr_accessor :chef_vault_handler
-      attr_reader   :connection
+      attr_reader :connection
 
       deps do
         require "erubis"
@@ -414,20 +412,35 @@ class Chef
 
       banner "knife bootstrap [PROTOCOL://][USER@]FQDN (options)"
 
-      def initialize(argv = [])
-        super
-        LicenseAcceptance::Acceptor.check_and_persist!("infra-client", Chef::VERSION.to_s, logger: Chef::Log, provided: Chef::Config[:chef_license])
-        @client_builder = Chef::Knife::Bootstrap::ClientBuilder.new(
+      def client_builder
+        @client_builder ||= Chef::Knife::Bootstrap::ClientBuilder.new(
           chef_config: Chef::Config,
           knife_config: config,
           ui: ui
         )
-        @chef_vault_handler = Chef::Knife::Bootstrap::ChefVaultHandler.new(
+      end
+
+      def chef_vault_handler
+        @chef_vault_handler ||= Chef::Knife::Bootstrap::ChefVaultHandler.new(
           knife_config: config,
           ui: ui
         )
       end
 
+      # Determine if we need to accept the Chef Infra license locally in order to successfully bootstrap
+      # the remote node. Remote 'chef-client' run will fail if it is >= 15 and the license is not accepted locally.
+      def check_license
+        Chef::Log.debug("Checking if we need to accept Chef license to bootstrap node")
+        version = config[:bootstrap_version] || Chef::VERSION.split(".").first
+        acceptor = LicenseAcceptance::Acceptor.new(logger: Chef::Log, provided: Chef::Config[:chef_license])
+        if acceptor.license_required?("chef", version)
+          Chef::Log.debug("License acceptance required for chef version: #{version}")
+          license_id = acceptor.id_from_mixlib("chef")
+          acceptor.check_and_persist(license_id, version)
+          Chef::Config[:chef_license] ||= acceptor.acceptance_value
+        end
+      end
+
       # The default bootstrap template to use to bootstrap a server.
       # This is a public API hook which knife plugins use or inherit and override.
       #
@@ -523,6 +536,7 @@ class Chef
       end
 
       def run
+        check_license
         verify_deprecated_flags!
 
         validate_name_args!
diff --git a/lib/chef/knife/core/bootstrap_context.rb b/lib/chef/knife/core/bootstrap_context.rb
index f9ebf52b31..3dccb048ec 100644
--- a/lib/chef/knife/core/bootstrap_context.rb
+++ b/lib/chef/knife/core/bootstrap_context.rb
@@ -92,6 +92,10 @@ class Chef
             validation_client_name "#{@chef_config[:validation_client_name]}"
           CONFIG
 
+          unless @chef_config[:chef_license].nil?
+            client_rb << "chef_license \"#{@chef_config[:chef_license]}\""
+          end
+
           if !(@chef_config[:config_log_level].nil? || @chef_config[:config_log_level].empty?)
             client_rb << %Q{log_level   :#{@chef_config[:config_log_level]}\n}
           end
@@ -171,8 +175,7 @@ class Chef
         def start_chef
           # If the user doesn't have a client path configure, let bash use the PATH for what it was designed for
           client_path = @chef_config[:chef_client_path] || "#{Chef::Dist::CLIENT}"
-          # We know we can hardcode CHEF_LICENSE because the user cannot get here without accepting the license locally
-          s = "CHEF_LICENSE=accept #{client_path} -j /etc/chef/first-boot.json"
+          s = "#{client_path} -j /etc/chef/first-boot.json"
           if @config[:verbosity] && @config[:verbosity] >= 3
             s << " -l trace"
           elsif @config[:verbosity] && @config[:verbosity] >= 2
diff --git a/lib/chef/knife/core/windows_bootstrap_context.rb b/lib/chef/knife/core/windows_bootstrap_context.rb
index 109f8e6f37..a1b7311f51 100644
--- a/lib/chef/knife/core/windows_bootstrap_context.rb
+++ b/lib/chef/knife/core/windows_bootstrap_context.rb
@@ -63,6 +63,11 @@ class Chef
             file_backup_path  "c:/chef/backup"
             cache_options     ({:path => "c:/chef/cache/checksums", :skip_expires => true})
           CONFIG
+
+          unless @chef_config[:chef_license].nil?
+            client_rb << "chef_license \"#{@chef_config[:chef_license]}\""
+          end
+
           if @config[:chef_node_name]
             client_rb << %Q{node_name "#{@config[:chef_node_name]}"\n}
           else
@@ -154,7 +159,6 @@ class Chef
         def start_chef
           bootstrap_environment_option = bootstrap_environment.nil? ? "" : " -E #{bootstrap_environment}"
           start_chef = "SET \"PATH=%SystemRoot%\\system32;%SystemRoot%;%SystemRoot%\\System32\\Wbem;%SYSTEMROOT%\\System32\\WindowsPowerShell\\v1.0\\;C:\\ruby\\bin;C:\\opscode\\chef\\bin;C:\\opscode\\chef\\embedded\\bin\"\n"
-          start_chef << "SET \"CHEF_LICENSE=accept\"\n"
           start_chef << "chef-client -c c:/chef/client.rb -j c:/chef/first-boot.json#{bootstrap_environment_option}\n"
         end
 
diff --git a/spec/unit/knife/bootstrap_spec.rb b/spec/unit/knife/bootstrap_spec.rb
index b8141d496f..f24dec6fa7 100644
--- a/spec/unit/knife/bootstrap_spec.rb
+++ b/spec/unit/knife/bootstrap_spec.rb
@@ -40,7 +40,6 @@ describe Chef::Knife::Bootstrap do
   let(:knife) do
     Chef::Log.logger = Logger.new(StringIO.new)
     Chef::Config[:knife][:bootstrap_template] = bootstrap_template unless bootstrap_template.nil?
-    expect(LicenseAcceptance::Acceptor).to receive(:check_and_persist!)
 
     k = Chef::Knife::Bootstrap.new(bootstrap_cli_options)
     allow(k.ui).to receive(:stderr).and_return(stderr)
@@ -50,9 +49,31 @@ describe Chef::Knife::Bootstrap do
     k
   end
 
-  it "fails when LicenseAcceptance fails" do
-    expect(LicenseAcceptance::Acceptor).to receive(:check_and_persist!).and_raise("foo")
-    expect { k = Chef::Knife::Bootstrap.new(bootstrap_cli_options) }.to raise_error("foo")
+  context "#check_license" do
+    let(:acceptor) { instance_double(LicenseAcceptance::Acceptor) }
+
+    before do
+      expect(LicenseAcceptance::Acceptor).to receive(:new).and_return(acceptor)
+    end
+
+    describe "when a license is not required" do
+      it "does not set the chef_license" do
+        expect(acceptor).to receive(:license_required?).and_return(false)
+        knife.check_license
+        expect(Chef::Config[:chef_license]).to eq(nil)
+      end
+    end
+
+    describe "when a license is required" do
+      it "sets the chef_license" do
+        expect(acceptor).to receive(:license_required?).and_return(true)
+        expect(acceptor).to receive(:id_from_mixlib).and_return("id")
+        expect(acceptor).to receive(:check_and_persist)
+        expect(acceptor).to receive(:acceptance_value).and_return("accept-no-persist")
+        knife.check_license
+        expect(Chef::Config[:chef_license]).to eq("accept-no-persist")
+      end
+    end
   end
 
   context "#bootstrap_template" do
@@ -303,6 +324,7 @@ describe Chef::Knife::Bootstrap do
         knife.parse_options(["--json-attribute-file", jsonfile.path])
         knife.merge_configs
         allow(knife).to receive(:validate_name_args!)
+        expect(knife).to receive(:check_license)
 
         expect { knife.run }.to raise_error(Chef::Exceptions::BootstrapCommandInputError)
         jsonfile.close
@@ -329,7 +351,6 @@ describe Chef::Knife::Bootstrap do
 
   describe "specifying no_proxy with various entries" do
     subject(:knife) do
-      expect(LicenseAcceptance::Acceptor).to receive(:check_and_persist!)
       k = described_class.new
       Chef::Config[:knife][:bootstrap_template] = template_file
       allow(k).to receive(:connection).and_return connection
@@ -1582,6 +1603,7 @@ describe Chef::Knife::Bootstrap do
     end
 
     it "performs the steps we expect to run a bootstrap" do
+      expect(knife).to receive(:check_license)
       expect(knife).to receive(:verify_deprecated_flags!).ordered
       expect(knife).to receive(:validate_name_args!).ordered
       expect(knife).to receive(:validate_protocol!).ordered
@@ -1803,6 +1825,7 @@ describe Chef::Knife::Bootstrap do
 
   it "verifies that a server to bootstrap was given as a command line arg" do
     knife.name_args = nil
+    expect(knife).to receive(:check_license)
     expect { knife.run }.to raise_error(SystemExit)
     expect(stderr.string).to match(/ERROR:.+FQDN or ip/)
   end
diff --git a/spec/unit/knife/core/bootstrap_context_spec.rb b/spec/unit/knife/core/bootstrap_context_spec.rb
index d57e254793..029dd90875 100644
--- a/spec/unit/knife/core/bootstrap_context_spec.rb
+++ b/spec/unit/knife/core/bootstrap_context_spec.rb
@@ -46,21 +46,21 @@ describe Chef::Knife::Core::BootstrapContext do
     expect { described_class.new(config, run_list, chef_config) }.not_to raise_error
   end
 
-  it "runs chef with the first-boot.json with no environment other than chef-license acceptance specified" do
-    expect(bootstrap_context.start_chef).to eq "CHEF_LICENSE=accept chef-client -j /etc/chef/first-boot.json"
+  it "runs chef with the first-boot.json with no environment" do
+    expect(bootstrap_context.start_chef).to eq "chef-client -j /etc/chef/first-boot.json"
   end
 
   describe "when in verbosity mode" do
     let(:config) { { verbosity: 2, color: true } }
     it "adds '-l debug' when verbosity is >= 2" do
-      expect(bootstrap_context.start_chef).to eq "CHEF_LICENSE=accept chef-client -j /etc/chef/first-boot.json -l debug"
+      expect(bootstrap_context.start_chef).to eq "chef-client -j /etc/chef/first-boot.json -l debug"
     end
   end
 
   describe "when no color value has been set in config" do
     let(:config) { { color: false } }
     it "adds '--no-color' when color is false" do
-      expect(bootstrap_context.start_chef).to eq "CHEF_LICENSE=accept chef-client -j /etc/chef/first-boot.json --no-color"
+      expect(bootstrap_context.start_chef).to eq "chef-client -j /etc/chef/first-boot.json --no-color"
     end
   end
 
@@ -79,10 +79,17 @@ describe Chef::Knife::Core::BootstrapContext do
     expect(bootstrap_context.config_content).to eq expected
   end
 
+  describe "when chef_license is set" do
+    let(:chef_config) { { chef_license: "accept-no-persist" } }
+    it "sets chef_license in the generated config file" do
+      expect(bootstrap_context.config_content).to include("chef_license \"accept-no-persist\"")
+    end
+  end
+
   describe "alternate chef-client path" do
     let(:chef_config) { { chef_client_path: "/usr/local/bin/chef-client" } }
     it "runs chef-client from another path when specified" do
-      expect(bootstrap_context.start_chef).to eq "CHEF_LICENSE=accept /usr/local/bin/chef-client -j /etc/chef/first-boot.json"
+      expect(bootstrap_context.start_chef).to eq "/usr/local/bin/chef-client -j /etc/chef/first-boot.json"
     end
   end
 
@@ -105,7 +112,7 @@ describe Chef::Knife::Core::BootstrapContext do
   describe "when bootstrapping into a specific environment" do
     let(:config) { { environment: "prodtastic", color: true } }
     it "starts chef in the configured environment" do
-      expect(bootstrap_context.start_chef).to eq("CHEF_LICENSE=accept chef-client -j /etc/chef/first-boot.json -E prodtastic")
+      expect(bootstrap_context.start_chef).to eq("chef-client -j /etc/chef/first-boot.json -E prodtastic")
     end
   end
 
diff --git a/spec/unit/knife/core/windows_bootstrap_context_spec.rb b/spec/unit/knife/core/windows_bootstrap_context_spec.rb
index 561e43eefc..639a7bf73a 100644
--- a/spec/unit/knife/core/windows_bootstrap_context_spec.rb
+++ b/spec/unit/knife/core/windows_bootstrap_context_spec.rb
@@ -174,6 +174,15 @@ describe Chef::Knife::Core::WindowsBootstrapContext do
       EXPECTED
       expect(bootstrap_context.config_content).to eq expected
     end
+
+    describe "when chef_license is set" do
+      before do
+        bootstrap_context.instance_variable_set(:@chef_config, Mash.new(chef_license: "accept-no-persist"))
+      end
+      it "sets chef_license in the generated config file" do
+        expect(bootstrap_context.config_content).to include("chef_license \"accept-no-persist\"")
+      end
+    end
   end
 
   describe "msi_url" do
@@ -272,11 +281,4 @@ describe Chef::Knife::Core::WindowsBootstrapContext do
     end
   end
 
-  describe "#start_chef" do
-    it "the command includes the license acceptance environment variable" do
-      expect(bootstrap_context.start_chef).to match(/SET "CHEF_LICENSE=accept"\n/m)
-    end
-
-  end
-
 end