Convert to aws-sdk and add new commands #546 #587

kvivek1115 · 2019-06-13T12:39:21Z

Description

Builds on #546

Rebase the knife-ec2/aws_sdk changes
Created new PR based on WIP: Convert to aws-sdk and add new commands #546

Without this you have to use another CLI or jump in to the UI to get the VPC ID for server create. Signed-off-by: Tim Smith <[email protected]>

This breaks the existing commands Signed-off-by: Tim Smith <[email protected]>

Another command to make figuring out what to feed knife ec2 server create a bit easier. Signed-off-by: Tim Smith <[email protected]>

This lets us find the network interface IDs we need when using knife ec2 server create Signed-off-by: Tim Smith <[email protected]>

No one should ever rely on these Signed-off-by: Tim Smith <[email protected]>

Signed-off-by: Tim Smith <[email protected]>

You need to specify the security group ID if you're provisioning into a VPC Signed-off-by: Tim Smith <[email protected]>

Unless you have a 30" monitor those strings are too long to display things correctly and it makes the output near useless Signed-off-by: Tim Smith <[email protected]>

We already have a method that does this for us. There's really no reason to do it twice. Signed-off-by: Tim Smith <[email protected]>

Signed-off-by: Tim Smith <[email protected]>

It's not a real api call, but instead a giant hash if fog-aws. Signed-off-by: Tim Smith <[email protected]>

Signed-off-by: Tim Smith <[email protected]>

- Add name and tags values. - Implement extract_tags from struct. Signed-off-by: Vivek Singh <[email protected]>

Signed-off-by: Vivek Singh <[email protected]>

…l-mode - Fix server list error when format is json. Signed-off-by: Vivek Singh <[email protected]>

Signed-off-by: Vivek Singh <[email protected]>

- Added dry_run option. - Updated the delete method to use AWS client. Signed-off-by: Vivek Singh <[email protected]>

- Add spot instances create method support. - Replace fog to SDK for getting addresses. - Modify the server attribute construction method. - Add various helpers methods. Signed-off-by: Vivek Singh <[email protected]>

- Update output params and set required params for further actions. - Update other fog apis to aws SDK. - Update logic to detect platform as a return in response params. Signed-off-by: Vivek Singh <[email protected]>

- Update windows check server instance raise message. - Minor fixes to print instance details. Signed-off-by: Vivek Singh <[email protected]>

- Update get password related code. - Attach network interfaces related changes. - Minor fixes - Chefstyle Signed-off-by: Vivek Singh <[email protected]>

- Remove memoization of bucket_obj. - Construct s3_connection object. Signed-off-by: Vivek Singh <[email protected]>

kvivek1115 · 2019-06-20T09:52:40Z

Hey, @tas50 thanks for pointing out the typo.

I am near to finish the refactoring only specs parts are remaining.

Any suggestions would be helpful? Thanks

Signed-off-by: Vivek Singh <[email protected]>

- Add default source us-east-1 Signed-off-by: Vivek Singh <[email protected]>

- Minor changes in ec2 ami list specs. Signed-off-by: Vivek Singh <[email protected]>

tas50 · 2019-06-24T16:20:19Z

knife-ec2.gemspec

@@ -16,7 +16,7 @@ Gem::Specification.new do |s|
 s.required_ruby_version = ">= 2.5"

 s.add_dependency "chef", ">= 15.1"
- s.add_dependency "fog-aws", ">= 1", "< 4"
+ s.add_dependency "aws-sdk", "~> 3.0"


So one thing we're going to want to do is use the individual aws-sdk gems instead of the main gem. The main gem depends on 182 other gems which we really don't want to do in Chef-DK. A good start would be to see if we can get away with just the ec2 gem.

@tas50 Yes, we can do that. As per my understaning, we just only need 'aws-sdk-ec2' and aws-sdk-s3. will update and let you know in case of any issue thanks.

- Minor fixes Signed-off-by: Vivek Singh <[email protected]>

- Minor fix in ec2 server delete to fetch the region. - Initial setup for ec2 server create specs. - Remove aws related configs vars. Signed-off-by: Vivek Singh <[email protected]>

- For non-aws cli need to provide base64 encoded string value for user_data params. Signed-off-by: Vivek Singh <[email protected]>

- Fix issue chef#591. - name_args nil issue and fix windows_password not called when winrm_password is nil - Remove some of specs related to old bootstrap process. Signed-off-by: Vivek Singh <[email protected]>

Signed-off-by: Vivek Singh <[email protected]>

- Add connection_user and connection_protocol methods. - Fix minor bugs. - Rename winrm_user to connection_user. - Fix bug related to encode user_data for ssl configuration. Signed-off-by: vsingh-msys <[email protected]>

- Minor bugs fix. - Ensured chef-style but robocop is failed on Travis. Signed-off-by: Vivek Singh <[email protected]>

Signed-off-by: Vivek Singh <[email protected]>

Nimesh-Msys

And as we are using a new library now, please include well explained YARD comments and provide references wherever applicable.

Nimesh-Msys · 2019-07-04T08:55:58Z

lib/chef/knife/ec2_ami_list.rb

- else
- server_list += get_server_list(server)
+ if config[:format] == "summary"
+ ami_hashes.each_pair do |_k, v|


servers_list is not explanatory. If possible can we use a better defined variable name here.

We could also use push to insert multiple values in one go !

servers = [] h.each do |k, v| servers.push v["image_id]", v["platform"] end

Or a one liner:
servers = h.inject([]){|arr, (_, v)| arr.push(v["image_id"],v["platform"])}

Nimesh-Msys · 2019-07-05T12:14:01Z

lib/chef/knife/ec2_base.rb

- }
+ def connection_string
+ conn = {}
+ conn[:region] = locate_config_value(:region) || "us-east-1" # Default region us-east-1


We should use this as a default in option.

Nimesh-Msys · 2019-07-05T12:19:33Z

lib/chef/knife/ec2_base.rb

+ conn[:region] = locate_config_value(:region) || "us-east-1" # Default region us-east-1
+ conn[:credentials] = if locate_config_value(:use_iam_profile)
+ Aws::InstanceProfileCredentials.new
+ elsif locate_config_value(:aws_session_token)


The third argument of the constructor is anyway default, we can skip this check. Also can DRY-up last else block then.

Nimesh-Msys · 2019-07-05T12:20:39Z

lib/chef/knife/ec2_base.rb

- connection_settings[:aws_session_token] = locate_config_value(:aws_session_token)
+ # @return [Aws::EC2::Client]
+ def ec2_connection
+ @ec2_connection ||= begin


Block wouldn't be required for one liner codes.

Okay sure thanks

Nimesh-Msys · 2019-07-05T12:27:38Z

lib/chef/knife/ec2_base.rb

+ # @return [Hash]
+ def server_hashes(server_obj)
+ server_data = {}
+ %w{ebs_optimized image_id instance_id instance_type key_name platform public_dns_name public_ip_address private_dns_name private_ip_address root_device_type}.each do |id|


The sorted array of strings would be more readable.

If we could store server_obj.instances[0] in an object, method would look clean.

map(&:code) could be used at multiple places in this method

Safe navigation could be used in iam_instance_profile the way it is used for volume_id

Nimesh-Msys · 2019-07-05T12:54:56Z

lib/chef/knife/ec2_server_create.rb

 ui.error "The following network interfaces are invalid: " \
 "#{invalid_nic_ids.join(', ')}"
 exit 1
 end

 def vpc_id
 @vpc_id ||= begin
- ec2_connection.subnets.get(locate_config_value(:subnet_id)).vpc_id
+ subnet = fetch_subnet(locate_config_value(:subnet_id))


instead of splitting it in two lines, we can use method chaining. & also can avoid using a block here.

Nimesh-Msys · 2019-07-05T12:59:09Z

lib/chef/knife/ec2_server_create.rb

- end
- response.body["passwordData"]
+ response = fetch_password_data(server_id)
+ return false unless response.password_data


Can simply use !response.password_data.nil? That would return the required Boolean arguments.

Nimesh-Msys · 2019-07-05T13:09:41Z

lib/chef/knife/ec2_server_create.rb

+ # Set default port 5986 if winrm_ssl return true otherwise set it to 5985
+ # Set default port 22 for ssh protocol
+ # @return [Integer]
+ def assign_default_port


How about using such elements by a constant.

DEFAULT_PORT = winrm? ? config_value(:winrm_ssl) ? 5985 : 5986 : 22

In my opinion, As we required these value at the instance level so not recommended.
Also, Ternary operators must not be nested. Prefer if or else constructs instead. Thanks

Nimesh-Msys · 2019-07-05T13:13:30Z

lib/chef/knife/ec2_server_create.rb

+ # url values override CLI flags, if you provide both
+ # we'll use the one that you gave in the URL.
+ def connection_protocol
+ return @connection_protocol if @connection_protocol


Using @connection_protocol ||= would be nice and symmetrical to connection_user

Nimesh-Msys · 2019-07-05T13:18:35Z

lib/chef/knife/ec2_server_delete.rb

+ ui.warn("Deleted server #{instance_id}")
+
+ if config[:purge]
+ if config[:chef_node_name]


thing_to_delete = config[:chef_node_name] || fetch_node_name(instance_id)

can we replace this variable name as just delete

- Revert AWS key proc to get the value in s3_source. - Fix style at some places. Signed-off-by: Vivek Singh <[email protected]>

kvivek1115 · 2019-07-08T06:41:24Z

@Nimesh-Msys thanks for the review comments. Updated the code accordingly.

Signed-off-by: Vivek Singh <[email protected]>

tas50 · 2019-07-08T18:31:29Z

lib/chef/knife/ec2_ami_list.rb

+
+ def image_params
+ params = {}
+ owner = locate_config_value(:owner) || "aws-marketplace" # aws-marketplace, microsoft


I'm not following what this comment means

these are the values that we can use. Will move in options for better understanding.

tas50 · 2019-07-08T18:31:54Z

lib/chef/knife/ec2_ami_list.rb

+ filters = []
+ filters << { platform: locate_config_value(:platform) } if locate_config_value(:platform)
+
+ # TODO: Need to find substring to match in the description


can we get an issue cut for this todo item so we don't forget it?

Noted @tas50 thanks

lamont-granquist · 2019-07-08T19:25:24Z

spec/unit/ec2_server_create_spec.rb

+ state: state,
+ security_groups: security_groups,
+ tags: tags
+ )


So as a general pattern this concerns me because OpenStruct is pretty terrible and instance_double would be better. So please never copy this a general way to mock objects. But if all the aws gem is passing back is an OpenStruct then you have to do this...

Okay sure @lamont-granquist thanks!

lamont-granquist · 2019-07-08T19:27:08Z

lib/chef/knife/ec2_base.rb

+ # @return [Struct]
+ def normalize_server_data(server_hashes)
+ require "ostruct"
+ OpenStruct.new(server_hashes)


This also makes me a bit worried, but it looks like the blast zone of this is relatively contained and probably necessary due to aws-sdk using OpenStruct everywhere.

@lamont-granquist to make data consistent same as aws-sdk response OpenStruct used here. will look into it what we can do. Thanks

tas50 · 2019-07-08T20:36:18Z

@vsingh-msys I'm going to merge this as-is since we need to ship this gem, but can you followup with the feedback that @Nimesh-Msys gave in another PR? Thanks for wrapping this all up. It's a huge amount of tech debt that we just paid down and it'll make this project usable for years to come.

kvivek1115 · 2019-07-09T03:19:29Z

@vsingh-msys I'm going to merge this as-is since we need to ship this gem, but can you follow up with the feedback that @Nimesh-Msys gave in another PR?

@tas50 I have almost completed the @Nimesh-Msys feedback already and some of them are not required such as

Rename variable servers_list,
Use one liner servers = h.inject([]){|arr, (_, v)| arr.push(v["image_id"],v["platform"])}
Use of push for mutiple variable.
etc.

As of now, we are fine. Thanks

kvivek1115 · 2019-08-13T05:33:51Z

Fixes for issue #529

tas50 and others added 15 commits June 13, 2019 17:07

Add knife ec2 vpc list command

5552b7f

Without this you have to use another CLI or jump in to the UI to get the VPC ID for server create. Signed-off-by: Tim Smith <[email protected]>

Very basic cutover to aws-sdk

1330d5f

This breaks the existing commands Signed-off-by: Tim Smith <[email protected]>

Add knife ec2 subnet list command

dc0896f

Another command to make figuring out what to feed knife ec2 server create a bit easier. Signed-off-by: Tim Smith <[email protected]>

Add knife ec2 eip list command

97af0d2

This lets us find the network interface IDs we need when using knife ec2 server create Signed-off-by: Tim Smith <[email protected]>

Make the helper methods private

ddd9138

No one should ever rely on these Signed-off-by: Tim Smith <[email protected]>

Clarify we want ENI IDs

5fd9c56

Signed-off-by: Tim Smith <[email protected]>

Add new knife ec2 securitygroup list command

3f028e7

You need to specify the security group ID if you're provisioning into a VPC Signed-off-by: Tim Smith <[email protected]>

Make amiprofile optional in knife server list

4cc515d

Unless you have a 30" monitor those strings are too long to display things correctly and it makes the output near useless Signed-off-by: Tim Smith <[email protected]>

Stop using procs to put the CLI options into the knife config

9fe08a4

We already have a method that does this for us. There's really no reason to do it twice. Signed-off-by: Tim Smith <[email protected]>

Convert knife ec2 server list to use aws sdk

6ac3ee1

Signed-off-by: Tim Smith <[email protected]>

Deprecate knife ec2 flavor list

4212698

It's not a real api call, but instead a giant hash if fog-aws. Signed-off-by: Tim Smith <[email protected]>

Require aws SDK v3

e8f86e7

Signed-off-by: Tim Smith <[email protected]>

Fix undefined method validate!

56be07b

- Add name and tags values. - Implement extract_tags from struct. Signed-off-by: Vivek Singh <[email protected]>

Replace connection to ec2_connection

effa1b4

Signed-off-by: Vivek Singh <[email protected]>

Update -z with -a for --availability-zone as -z already used for loca…

963f239

…l-mode - Fix server list error when format is json. Signed-off-by: Vivek Singh <[email protected]>

kvivek1115 force-pushed the VSingh/aws_sdk branch from c07e819 to 963f239 Compare June 14, 2019 09:29

Vivek Singh added 7 commits June 17, 2019 11:51

Refactor ec2 ami list

82bbab8

Signed-off-by: Vivek Singh <[email protected]>

Refactor ec2 server delete options

3ea148b

- Added dry_run option. - Updated the delete method to use AWS client. Signed-off-by: Vivek Singh <[email protected]>

Refactored server create

3b87798

- Add spot instances create method support. - Replace fog to SDK for getting addresses. - Modify the server attribute construction method. - Add various helpers methods. Signed-off-by: Vivek Singh <[email protected]>

Server create describe images params

0dccc2b

- Update output params and set required params for further actions. - Update other fog apis to aws SDK. - Update logic to detect platform as a return in response params. Signed-off-by: Vivek Singh <[email protected]>

Wait until server ready while creating the instance.

b6cdc93

- Update windows check server instance raise message. - Minor fixes to print instance details. Signed-off-by: Vivek Singh <[email protected]>

Move aws sdk related methods to ec2_base

249c8ee

- Update get password related code. - Attach network interfaces related changes. - Minor fixes - Chefstyle Signed-off-by: Vivek Singh <[email protected]>

Replace fog with Aws::S3::Client and fix typo

a03882e

- Remove memoization of bucket_obj. - Construct s3_connection object. Signed-off-by: Vivek Singh <[email protected]>

Vivek Singh added 4 commits June 20, 2019 17:58

Fix s3 source deps specs and remove flavor list specs

78bdc69

Signed-off-by: Vivek Singh <[email protected]>

S3 source specs

af6e0c7

Signed-off-by: Vivek Singh <[email protected]>

Ec2 ami list specs

79ccc8d

- Add default source us-east-1 Signed-off-by: Vivek Singh <[email protected]>

Fix ec2 server list specs

430889f

- Minor changes in ec2 ami list specs. Signed-off-by: Vivek Singh <[email protected]>

tas50 reviewed Jun 24, 2019

View reviewed changes

Use aws-sdk-ec2 and aws-sdk-s3 instead of aws-sdk.

c91fa30

- Minor fixes Signed-off-by: Vivek Singh <[email protected]>

Vivek Singh added 2 commits June 26, 2019 15:17

Fix ec2 server delete specs.

011e201

- Minor fix in ec2 server delete to fetch the region. - Initial setup for ec2 server create specs. - Remove aws related configs vars. Signed-off-by: Vivek Singh <[email protected]>

Fix user_data encoding issue

0834145

- For non-aws cli need to provide base64 encoded string value for user_data params. Signed-off-by: Vivek Singh <[email protected]>

dheerajd-msys mentioned this pull request Jun 27, 2019

ec2 server create fails with error ERROR: undefined local variable or method `i' for #<Chef::Knife::Ec2ServerCreate:0x00000000090946e8> for option --iam-profile #591

Closed

Vivek Singh added 6 commits June 27, 2019 15:08

Fix knife ec2 server create specs

2a8adc8

- Fix issue chef#591. - name_args nil issue and fix windows_password not called when winrm_password is nil - Remove some of specs related to old bootstrap process. Signed-off-by: Vivek Singh <[email protected]>

Configuration process related ec2 server create specs.

f260a07

Signed-off-by: Vivek Singh <[email protected]>

Fix specs related to connection options

53fa400

Signed-off-by: Vivek Singh <[email protected]>

Fix ec2 server create specs

4fd06e5

Signed-off-by: Vivek Singh <[email protected]>

Apply and ensure chefstyle

2814e9e

Signed-off-by: Vivek Singh <[email protected]>

Require chef 15.1+

0be2566

Signed-off-by: Vivek Singh <[email protected]>

kvivek1115 changed the title ~~WIP: Convert to aws-sdk and add new commands #546~~ Convert to aws-sdk and add new commands #546 Jul 1, 2019

Safe navigation for AMI EBS

8520a4f

Signed-off-by: Vivek Singh <[email protected]>

dheerajd-msys mentioned this pull request Jul 1, 2019

knife ec2 server create fails with error ERROR: undefined method `delete_on_termination' for nil:NilClass #592

Closed

vsingh-msys added 3 commits July 3, 2019 14:08

Fix winrm_ssl related config

ac330d6

- Add connection_user and connection_protocol methods. - Fix minor bugs. - Rename winrm_user to connection_user. - Fix bug related to encode user_data for ssl configuration. Signed-off-by: vsingh-msys <[email protected]>

Fix specs and Travis failure

83a3e17

- Minor bugs fix. - Ensured chef-style but robocop is failed on Travis. Signed-off-by: Vivek Singh <[email protected]>

Fix travis build failure

d673858

Signed-off-by: Vivek Singh <[email protected]>

kvivek1115 mentioned this pull request Jul 4, 2019

Convert this gem to use the aws-sdk instead of fog #529

Closed

Fix platform return nil for linux AMI's

942f03f

Signed-off-by: Vivek Singh <[email protected]>

dheerajd-msys mentioned this pull request Jul 5, 2019

knife ec2 server create fails with error undefined method `platform' for nil:NilClass (NoMethodError) #593

Closed

Nimesh-Msys reviewed Jul 5, 2019

View reviewed changes

Minor coding style update

757b4b5

- Revert AWS key proc to get the value in s3_source. - Fix style at some places. Signed-off-by: Vivek Singh <[email protected]>

Minor coding style update

f745334

Signed-off-by: Vivek Singh <[email protected]>

tas50 reviewed Jul 8, 2019

View reviewed changes

lamont-granquist reviewed Jul 8, 2019

View reviewed changes

tas50 merged commit 22d4f55 into chef:master Jul 8, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert to aws-sdk and add new commands #546 #587

Convert to aws-sdk and add new commands #546 #587

kvivek1115 commented Jun 13, 2019

kvivek1115 commented Jun 20, 2019

tas50 Jun 24, 2019

kvivek1115 Jun 25, 2019

Nimesh-Msys left a comment

Nimesh-Msys Jul 4, 2019

Nimesh-Msys Jul 5, 2019

Nimesh-Msys Jul 5, 2019

Nimesh-Msys Jul 5, 2019

kvivek1115 Jul 8, 2019

Nimesh-Msys Jul 5, 2019

Nimesh-Msys Jul 5, 2019

Nimesh-Msys Jul 5, 2019

Nimesh-Msys Jul 5, 2019

kvivek1115 Jul 8, 2019 •

edited

Loading

Nimesh-Msys Jul 5, 2019

Nimesh-Msys Jul 5, 2019

kvivek1115 commented Jul 8, 2019

tas50 Jul 8, 2019

kvivek1115 Jul 9, 2019

tas50 Jul 8, 2019

kvivek1115 Jul 9, 2019

lamont-granquist Jul 8, 2019

kvivek1115 Jul 9, 2019

lamont-granquist Jul 8, 2019

kvivek1115 Jul 9, 2019

tas50 commented Jul 8, 2019

kvivek1115 commented Jul 9, 2019

kvivek1115 commented Aug 13, 2019

Convert to aws-sdk and add new commands #546 #587

Convert to aws-sdk and add new commands #546 #587

Conversation

kvivek1115 commented Jun 13, 2019

Description

kvivek1115 commented Jun 20, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nimesh-Msys left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kvivek1115 Jul 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kvivek1115 commented Jul 8, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tas50 commented Jul 8, 2019

kvivek1115 commented Jul 9, 2019

kvivek1115 commented Aug 13, 2019

kvivek1115 Jul 8, 2019 •

edited

Loading