Merge pull request #677 from ashiqueps/CHEF-3502-knife-ec-2-server-cr…

…eate-fails Added support for the gp3 and io2 ebs volume types
chef · Apr 2, 2024 · 49d08c4 · 49d08c4
2 parents e63d0f6 + de142b8
commit 49d08c4
Show file tree

Hide file tree

Showing 9 changed files with 98 additions and 106 deletions.
diff --git a/.expeditor/verify.pipeline.yml b/.expeditor/verify.pipeline.yml
@@ -11,17 +11,10 @@ expeditor:
 
 steps:
 
-- label: run-specs-ruby-2.6
+- label: run-specs-ruby-3.1
   command:
     - .expeditor/run_linux_tests.sh rake
   expeditor:
     executor:
       docker:
-        image: ruby:2.6-buster
-- label: run-specs-ruby-2.7
-  command:
-    - .expeditor/run_linux_tests.sh rake
-  expeditor:
-    executor:
-      docker:
-        image: ruby:2.7-buster
+        image: ruby:3.1-buster
diff --git a/knife-ec2.gemspec b/knife-ec2.gemspec
@@ -12,9 +12,9 @@ Gem::Specification.new do |s|
   s.license      = "Apache-2.0"
 
   s.files        = %w{LICENSE} + Dir.glob("lib/**/*")
-  s.required_ruby_version = ">= 2.6"
+  s.required_ruby_version = ">= 3.1"
 
-  s.add_dependency "chef", ">= 15.11" # needs this version for Chef 16 backports
+  s.add_dependency "knife", "~> 18.0"
   s.add_dependency "aws-sdk-s3", "~> 1.43"
   s.add_dependency "aws-sdk-ec2", "~> 1.95"
 

diff --git a/lib/chef/knife/ec2_server_create.rb b/lib/chef/knife/ec2_server_create.rb
@@ -25,6 +25,10 @@ class Chef
   class Knife
     class Ec2ServerCreate < Chef::Knife::Bootstrap
 
+      GP_VOLUME_TYPES = %w{gp2 gp3}.freeze
+      IOPS_VOLUME_TYPES = %w{io1 io2}.freeze
+      EBS_VOLUME_TYPES = (%w{standard st1 sc1} + GP_VOLUME_TYPES + IOPS_VOLUME_TYPES).freeze
+
       include Knife::Ec2Base
 
       deps do
@@ -161,12 +165,12 @@ class Ec2ServerCreate < Chef::Knife::Bootstrap
 
       option :ebs_volume_type,
         long: "--ebs-volume-type TYPE",
-        description: "Possible values are standard (magnetic) | io1 | gp2 | sc1 | st1. Default is gp2",
-        default: "gp2"
+        description: "Possible values are #{EBS_VOLUME_TYPES.join(" | ")}. Default is gp3",
+        default: "gp3"
 
       option :ebs_provisioned_iops,
         long: "--provisioned-iops IOPS",
-        description: "IOPS rate, only used when ebs volume type is 'io1'",
+        description: "IOPS rate, only used when ebs volume type is '#{IOPS_VOLUME_TYPES.join(" or ")}'",
         default: nil
 
       option :validation_key_url,
@@ -495,13 +499,11 @@ def default_bootstrap_template
       end
 
       def validation_key_path
-        @validation_key_path ||= begin
-          if URI(config[:validation_key_url]).scheme == "file"
-            URI(config[:validation_key_url]).path
-          else
-            validation_key_tmpfile.path
-          end
-        end
+        @validation_key_path ||= if URI(config[:validation_key_url]).scheme == "file"
+                                   URI(config[:validation_key_url]).path
+                                 else
+                                   validation_key_tmpfile.path
+                                 end
       end
 
       # @return [Tempfile]
@@ -521,9 +523,7 @@ def download_validation_key(tempfile)
       end
 
       def s3_validation_key
-        @s3_validation_key ||= begin
-          Chef::Knife::S3Source.fetch(config[:validation_key_url], config)
-        end
+        @s3_validation_key ||= Chef::Knife::S3Source.fetch(config[:validation_key_url], config)
       end
 
       def s3_secret
@@ -616,18 +616,18 @@ def plugin_validate_options!
           end
         end
 
-        if config[:ebs_provisioned_iops] && (config[:ebs_volume_type] != "io1")
-          ui.error("--provisioned-iops option is only supported for volume type of 'io1'")
+        if config[:ebs_provisioned_iops] && !IOPS_VOLUME_TYPES.include?(config[:ebs_volume_type])
+          ui.error("--provisioned-iops option is only supported for volume type of '#{IOPS_VOLUME_TYPES.join(" or ")}'")
           exit 1
         end
 
-        if (config[:ebs_volume_type] == "io1") && config[:ebs_provisioned_iops].nil?
-          ui.error("--provisioned-iops option is required when using volume type of 'io1'")
+        if IOPS_VOLUME_TYPES.include?(config[:ebs_volume_type]) && config[:ebs_provisioned_iops].nil?
+          ui.error("--provisioned-iops option is required when using volume type of '#{IOPS_VOLUME_TYPES.join(" or ")}'")
           exit 1
         end
 
-        if config[:ebs_volume_type] && ! %w{gp2 io1 standard st1 sc1}.include?(config[:ebs_volume_type])
-          ui.error("--ebs-volume-type must be 'standard' or 'io1' or 'gp2' or 'st1' or 'sc1'")
+        if config[:ebs_volume_type] && ! EBS_VOLUME_TYPES.include?(config[:ebs_volume_type])
+          ui.error("--ebs-volume-type must be '#{EBS_VOLUME_TYPES.join("' or '")}'")
           msg opt_parser
           exit 1
         end
@@ -682,10 +682,12 @@ def plugin_validate_options!
           # validation for ebs_size and ebs_volume_type and ebs_encrypted
           if !config[:ebs_size]
             errors << "--ebs-encrypted option requires valid --ebs-size to be specified."
-          elsif (config[:ebs_volume_type] == "gp2") && ! config[:ebs_size].to_i.between?(1, 16384)
-            errors << "--ebs-size should be in between 1-16384 for 'gp2' ebs volume type."
+          elsif (GP_VOLUME_TYPES.include?(config[:ebs_volume_type])) && ! config[:ebs_size].to_i.between?(1, 16384)
+            errors << "--ebs-size should be in between 1-16384 for '#{GP_VOLUME_TYPES.join("' or '")}' ebs volume type."
           elsif (config[:ebs_volume_type] == "io1") && ! config[:ebs_size].to_i.between?(4, 16384)
             errors << "--ebs-size should be in between 4-16384 for 'io1' ebs volume type."
+          elsif (config[:ebs_volume_type] == "io2") && ! config[:ebs_size].to_i.between?(4, 65536)
+            errors << "--ebs-size should be in between 4-65536 for 'io2' ebs volume type."
           elsif (config[:ebs_volume_type] == "standard") && ! config[:ebs_size].to_i.between?(1, 1024)
             errors << "--ebs-size should be in between 1-1024 for 'standard' ebs volume type."
           end
@@ -757,8 +759,8 @@ def ssl_config_user_data
         user = connection_user.split("\\")
         if (user[0] == ".") || (user[0] == "") || (user.length == 1)
           user_related_commands = <<~EOH
-            net user /add #{connection_user.delete('.\\')} #{windows_password} #{@allow_long_password};
-            net localgroup Administrators /add #{connection_user.delete('.\\')};
+            net user /add #{connection_user.delete(".\\")} #{windows_password} #{@allow_long_password};
+            net localgroup Administrators /add #{connection_user.delete(".\\")};
           EOH
         end
         <<~EOH
@@ -953,7 +955,7 @@ def server_attributes
                ebs: {
                   delete_on_termination: delete_term,
                   volume_size: ebs_size,
-                  volume_type: config[:ebs_volume_type], # accepts standard, io1, gp2, sc1, st1
+                  volume_type: config[:ebs_volume_type], # accepts standard, io1, io2, gp2, gp3, sc1, st1
                 },
              }]
           attributes[:block_device_mappings][0][:ebs][:iops] = iops_rate unless iops_rate.nil? || iops_rate.empty?
@@ -1139,15 +1141,13 @@ def connection_host
       # Otherwise assign public DNS or public IP.
       # @return [String]
       def connect_attribute
-        @connect_attribute ||= begin
-          if !!config[:server_connect_attribute]
-            config[:server_connect_attribute]
-          elsif vpc_mode? && !(subnet_public_ip_on_launch? || config[:associate_public_ip] || config[:associate_eip])
-            "private_ip_address"
-          else
-            server.public_dns_name ? "public_dns_name" : "public_ip_address"
-          end
-        end
+        @connect_attribute ||= if !!config[:server_connect_attribute]
+                                 config[:server_connect_attribute]
+                               elsif vpc_mode? && !(subnet_public_ip_on_launch? || config[:associate_public_ip] || config[:associate_eip])
+                                 "private_ip_address"
+                               else
+                                 server.public_dns_name ? "public_dns_name" : "public_ip_address"
+                               end
       end
 
       def create_tags(hashed_tags)

diff --git a/lib/chef/knife/helpers/ec2_base.rb b/lib/chef/knife/helpers/ec2_base.rb
@@ -234,13 +234,11 @@ def validate_aws_config!(keys = %i{aws_access_key_id aws_secret_access_key})
     # if default location exists on disk fallback to that
     # @return [String, nil] location to aws credentials file or nil if none exists
     def aws_cred_file_location
-      @cred_file ||= begin
-        if !config[:aws_credential_file].nil?
-          config[:aws_credential_file]
-        else
-          Chef::Util::PathHelper.home(".aws", "credentials") if ::File.exist?(Chef::Util::PathHelper.home(".aws", "credentials"))
-        end
-      end
+      @cred_file ||= if !config[:aws_credential_file].nil?
+                       config[:aws_credential_file]
+                     else
+                       Chef::Util::PathHelper.home(".aws", "credentials") if ::File.exist?(Chef::Util::PathHelper.home(".aws", "credentials"))
+                     end
     end
 
     # @return [String]

diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb
@@ -24,10 +24,10 @@ def self.from(system_exit)
   end
 
   c.around(:example) do |ex|
-    begin
-      ex.run
-    rescue SystemExit => e
-      raise UnexpectedSystemExit.from(e)
-    end
+
+    ex.run
+  rescue SystemExit => e
+    raise UnexpectedSystemExit.from(e)
+
   end
 end
diff --git a/spec/unit/ec2_ami_list_spec.rb b/spec/unit/ec2_ami_list_spec.rb
@@ -141,9 +141,10 @@
       context "When owner is invalid" do
         it "raises error" do
           allow(knife_ec2_ami_list).to receive(:puts)
-          expect(lambda { knife_ec2_ami_list.parse_options(["--owner", "xyz"]) }).to raise_error(SystemExit)
+          expect { knife_ec2_ami_list.parse_options(["--owner", "xyz"]) }.to raise_error(SystemExit)
         end
       end
+
     end
 
     context "when --platform is passed" do
@@ -233,7 +234,7 @@
       context "When platform is invalid" do
         it "raises error" do
           allow(knife_ec2_ami_list).to receive(:puts)
-          expect(lambda { knife_ec2_ami_list.parse_options(["--platform", "xyz"]) }).to raise_error(SystemExit)
+          expect { knife_ec2_ami_list.parse_options(["--platform", "xyz"]) }.to raise_error(SystemExit)
         end
       end
     end