From df3fc43289839ff1e005ff3811c375a5b76ab20a Mon Sep 17 00:00:00 2001
From: Samuel Beaulieu <samuel+14401380@puppet.com>
Date: Tue, 26 Jul 2022 14:39:23 -0500
Subject: [PATCH 1/9] fix lifetime, fix name to use depending on domain set or
 not

---
 lib/vmpooler/providers/ec2.rb | 26 +++++++++++++++++++++++---
 1 file changed, 23 insertions(+), 3 deletions(-)

diff --git a/lib/vmpooler/providers/ec2.rb b/lib/vmpooler/providers/ec2.rb
index 7ef5e36..8f8f871 100644
--- a/lib/vmpooler/providers/ec2.rb
+++ b/lib/vmpooler/providers/ec2.rb
@@ -192,6 +192,13 @@ module Vmpooler
           raise("Instance creation not attempted, #{new_vmname} already exists") if get_vm(pool_name, new_vmname)
 
           subnet_id = get_subnet_id(pool_name)
+          domain_set = domain
+          if domain_set.nil?
+            name_to_use = new_vmname
+          else
+            name_to_use = "#{new_vmname}.#{domain_set}"
+          end
+
           tag = [
             {
               resource_type: 'instance', # accepts capacity-reservation, client-vpn-endpoint, customer-gateway, carrier-gateway, dedicated-host, dhcp-options, egress-only-internet-gateway, elastic-ip, elastic-gpu, export-image-task, export-instance-task, fleet, fpga-image, host-reservation, image, import-image-task, import-snapshot-task, instance, instance-event-window, internet-gateway, ipam, ipam-pool, ipam-scope, ipv4pool-ec2, ipv6pool-ec2, key-pair, launch-template, local-gateway, local-gateway-route-table, local-gateway-virtual-interface, local-gateway-virtual-interface-group, local-gateway-route-table-vpc-association, local-gateway-route-table-virtual-interface-group-association, natgateway, network-acl, network-interface, network-insights-analysis, network-insights-path, network-insights-access-scope, network-insights-access-scope-analysis, placement-group, prefix-list, replace-root-volume-task, reserved-instances, route-table, security-group, security-group-rule, snapshot, spot-fleet-request, spot-instances-request, subnet, subnet-cidr-reservation, traffic-mirror-filter, traffic-mirror-session, traffic-mirror-target, transit-gateway, transit-gateway-attachment, transit-gateway-connect-peer, transit-gateway-multicast-domain, transit-gateway-route-table, volume, vpc, vpc-endpoint, vpc-endpoint-service, vpc-peering-connection, vpn-connection, vpn-gateway, vpc-flow-log
@@ -206,7 +213,7 @@ module Vmpooler
                 },
                 {
                   key: 'lifetime', # required by AWS reaper
-                  value: get_current_lifetime(new_vmname)
+                  value: get_max_lifetime
                 },
                 {
                   key: 'created_by', # required by AWS reaper
@@ -226,7 +233,7 @@ module Vmpooler
                 },
                 {
                   key: 'Name',
-                  value: new_vmname
+                  value: name_to_use
                 }
               ]
             }
@@ -451,6 +458,12 @@ module Vmpooler
           end
         end
 
+        # returns max_lifetime_upper_limit in hours in the format Xh defaults to 12h
+        def get_max_lifetime
+          max_hours = global_config[:config]['max_lifetime_upper_limit'] || '12'
+          "#{max_hours}h"
+        end
+
         def get_current_job_url(vm_name)
           @redis.with_metrics do |redis|
             job = redis.hget("vmpooler__vm__#{vm_name}", 'tag:jenkins_build_url') || ''
@@ -464,8 +477,15 @@ module Vmpooler
           pool_configuration = pool_config(pool_name)
           return nil if pool_configuration.nil?
 
+          domain_set = domain
+          if domain_set.nil?
+            name_to_use = vm_object.private_dns_name
+          else
+            name_to_use = vm_object.tags.detect { |f| f.key == 'Name' }&.value
+          end
+
           {
-            'name' => vm_object.tags.detect { |f| f.key == 'vm_name' }&.value,
+            'name' => name_to_use,
             # 'hostname' => vm_object.hostname,
             'template' => pool_configuration&.key?('template') ? pool_configuration['template'] : nil, # was expecting to get it from API, not from config, but this is what vSphere does too!
             'poolname' => vm_object.tags.detect { |f| f.key == 'pool' }&.value,

From 48ecaa73208399f755b66667e8ab85d3ac29e22e Mon Sep 17 00:00:00 2001
From: Samuel Beaulieu <samuel+14401380@puppet.com>
Date: Tue, 26 Jul 2022 15:14:49 -0500
Subject: [PATCH 2/9] set a new redis key to the fqdn used so it can be queried
 in floaty

---
 lib/vmpooler/providers/ec2.rb | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/vmpooler/providers/ec2.rb b/lib/vmpooler/providers/ec2.rb
index 8f8f871..b142675 100644
--- a/lib/vmpooler/providers/ec2.rb
+++ b/lib/vmpooler/providers/ec2.rb
@@ -267,6 +267,7 @@ module Vmpooler
           @logger.log('s', "[>] [#{pool_name}] '#{new_vmname}' instance ready to accept traffic")
           created_instance = get_vm(pool_name, new_vmname)
 
+          @redis.hset("vmpooler__vm__#{new_vmname}", 'host', created_instance['name'])
           # extra setup steps
           provision_node_aws(created_instance['private_dns_name'], pool_name, new_vmname) if to_provision(pool_name) == 'true' || to_provision(pool_name) == true
 

From c028000a618dcec6d7a945f1bb783b11359940dd Mon Sep 17 00:00:00 2001
From: Samuel Beaulieu <samuel+14401380@puppet.com>
Date: Wed, 27 Jul 2022 10:19:22 -0500
Subject: [PATCH 3/9] integrtae GCP Cloud DNS from the gce provider

---
 README.md                       | 14 +++++++++++---
 lib/vmpooler/providers/ec2.rb   | 28 ++++++++++++++++++++++++----
 spec/unit/providers/ec2_spec.rb | 14 ++++++++++----
 vmpooler-provider-ec2.gemspec   |  4 +++-
 vmpooler.yaml.example           |  6 +++++-
 5 files changed, 53 insertions(+), 13 deletions(-)

diff --git a/README.md b/README.md
index 2fb385c..2909f3f 100644
--- a/README.md
+++ b/README.md
@@ -25,7 +25,16 @@ These steps expect two environment vars
 ### DNS
 AWS will setup a private ip and private dns hostname for the VM once running. Optionally we can setup a human readable DNS entry to resolve the VMPooler provider `spicy-proton` fqdn
 
-DNS is integrated via Google's CloudDNS service. To enable, a CloudDNS zone name must be provided in the config (see the example yaml file dns_zone_resource_name)
+DNS is integrated via Google's CloudDNS service.
+GCE authorization is handled via a service account (or personal account) private key (json format) and can be configured via
+
+1. GOOGLE_APPLICATION_CREDENTIALS environment variable eg GOOGLE_APPLICATION_CREDENTIALS=/my/home/directory/my_account_key.json
+
+Provider config needed:
+1. domain
+2. project
+3. dns_zone_resource_name
+(see the example yaml file)
 
 An A record is then created in that zone upon instance creation with the VM's internal IP, and deleted when the instance is destroyed.
 
@@ -44,8 +53,7 @@ do not have the pool label, and can be configured to allow a specific list of un
 ### Pre-requisite
 
 - An IAM user must exist in the target AWS account with permissions to create, delete vms etc
-- if using DNS, a DNS zone needs to be created in CloudDNS, and configured in the provider's config section with the name of that zone (dns_zone_resource_name). When not specified, the DNS setup and teardown is skipped.
-
+- if using DNS see section above, and a service account with permissions to change Cloud DNS need to exist
 
 ## License
 
diff --git a/lib/vmpooler/providers/ec2.rb b/lib/vmpooler/providers/ec2.rb
index b142675..9dfc108 100644
--- a/lib/vmpooler/providers/ec2.rb
+++ b/lib/vmpooler/providers/ec2.rb
@@ -3,6 +3,7 @@
 require 'bigdecimal'
 require 'bigdecimal/util'
 require 'vmpooler/providers/base'
+require 'vmpooler/cloud_dns'
 require 'aws-sdk-ec2'
 require 'vmpooler/aws_setup'
 
@@ -61,8 +62,6 @@ module Vmpooler
           end
         end
 
-        attr_reader :dns
-
         # main configuration options
         def region
           return provider_config['region'] if provider_config['region']
@@ -85,6 +84,10 @@ module Vmpooler
         end
 
         # dns
+        def project
+          provider_config['project']
+        end
+
         def domain
           provider_config['domain']
         end
@@ -267,10 +270,13 @@ module Vmpooler
           @logger.log('s', "[>] [#{pool_name}] '#{new_vmname}' instance ready to accept traffic")
           created_instance = get_vm(pool_name, new_vmname)
 
-          @redis.hset("vmpooler__vm__#{new_vmname}", 'host', created_instance['name'])
+          @redis.with_metrics do |redis|
+            redis.hset("vmpooler__vm__#{new_vmname}", 'host', created_instance['name'])
+          end
           # extra setup steps
           provision_node_aws(created_instance['private_dns_name'], pool_name, new_vmname) if to_provision(pool_name) == 'true' || to_provision(pool_name) == true
 
+          dns_setup(created_instance) if domain
           created_instance
         end
 
@@ -363,7 +369,7 @@ module Vmpooler
         #   [String] vm_name    : Name of the existing VM
         # returns
         #   [boolean] true : once the operations are finished
-        def destroy_vm(_pool_name, vm_name)
+        def destroy_vm(pool_name, vm_name)
           debug_logger('destroy_vm')
           deleted = false
 
@@ -374,6 +380,7 @@ module Vmpooler
           instances = connection.instances(filters: filters).first
           return true if instances.nil?
 
+          instance_hash = get_vm(pool_name, vm_name)
           debug_logger("trigger delete_instance #{vm_name}")
           # vm_hash = get_vm(pool_name, vm_name)
           instances.terminate
@@ -384,6 +391,8 @@ module Vmpooler
             debug_logger("failed waiting for instance terminated #{vm_name}: #{e}")
           end
 
+          dns_teardown(instance_hash) if domain
+
           deleted
         end
 
@@ -437,6 +446,16 @@ module Vmpooler
 
         # END BASE METHODS
 
+        def dns_setup(created_instance)
+          dns = Vmpooler::PoolManager::CloudDns.new(project, dns_zone_resource_name)
+          dns.dns_create_or_replace(created_instance)
+        end
+
+        def dns_teardown(created_instance)
+          dns = Vmpooler::PoolManager::CloudDns.new(project, dns_zone_resource_name)
+          dns.dns_teardown(created_instance)
+        end
+
         def get_current_user(vm_name)
           @redis.with_metrics do |redis|
             user = redis.hget("vmpooler__vm__#{vm_name}", 'token:user')
@@ -494,6 +513,7 @@ module Vmpooler
             'status' => vm_object.state&.name, # One of the following values: pending, running, shutting-down, terminated, stopping, stopped
             # 'zone' => vm_object.zone,
             'image_size' => vm_object.instance_type,
+            'ip' => vm_object.private_ip_address,
             'private_ip_address' => vm_object.private_ip_address,
             'private_dns_name' => vm_object.private_dns_name
           }
diff --git a/spec/unit/providers/ec2_spec.rb b/spec/unit/providers/ec2_spec.rb
index 5225835..9f33376 100644
--- a/spec/unit/providers/ec2_spec.rb
+++ b/spec/unit/providers/ec2_spec.rb
@@ -53,23 +53,28 @@ EOT
 
   describe '#manual tests live' do
     context 'in itsysops' do
-      let(:vmname) { "instance-50" }
-      let(:poolname) { "ubuntu-2004-arm64" }
+      let(:vmname) { "instance-60" }
+      let(:poolname) { "amazon-7-x86_64-local" }
+      let(:amisize) { "c5.xlarge" }
       let(:config) { YAML.load(<<~EOT
   ---
   :config:
     max_tries: 3
     retry_factor: 10
+    site_name: 'vmpooler-local-dev'
   :providers:
     :ec2:
       connection_pool_timeout: 1
       zone: '#{zone}'
       region: '#{region}'
+      project: 'vmpooler-test'
+      dns_zone_resource_name: 'vmpooler-test-puppet-net'
+      domain: 'vmpooler-test.puppet.net'
   :pools:
     - name: '#{poolname}'
       alias: [ 'mockpool' ]
-      amisize: 'a1.large'
-      template: 'ami-03c1b544a7566b3e5'
+      amisize: '#{amisize}'
+      template: 'ami-31394949'
       size: 5
       timeout: 10
       ready_ttl: 1440
@@ -83,6 +88,7 @@ EOT
       }
       skip 'gets a vm' do
         result = subject.create_vm(poolname, vmname)
+        result = subject.destroy_vm(poolname, vmname)
         #subject.vms_in_pool("amazon-6-x86_64-ec2")
         #subject.provision_node_aws("ip-10-227-4-97.amz-dev.puppet.net", poolname)
         # subject.create_snapshot(poolname, vmname, "foo")
diff --git a/vmpooler-provider-ec2.gemspec b/vmpooler-provider-ec2.gemspec
index dd6986a..e084e70 100644
--- a/vmpooler-provider-ec2.gemspec
+++ b/vmpooler-provider-ec2.gemspec
@@ -16,9 +16,11 @@ Gem::Specification.new do |s|
   s.files         = Dir[ "lib/**/*" ]
   s.require_paths = ["lib"]
   s.add_dependency 'aws-sdk-ec2', '~> 1'
-  s.add_dependency 'net-ssh', '~> 6.2.0.rc2'
+  s.add_dependency 'net-ssh', '>= 6.2', '< 7.1'
   
   s.add_development_dependency 'vmpooler', '>= 1.3.0', '~> 2.3'
+  #s.add_development_dependency 'vmpooler-provider-gce', '>= 0.4.0', '~> 0.4'
+  s.add_development_dependency 'vmpooler-provider-gce', '>= 0.4.0', '~> 0.4'
 
   # Testing dependencies
   s.add_development_dependency 'climate_control', '>= 0.2.0'
diff --git a/vmpooler.yaml.example b/vmpooler.yaml.example
index 28b92a3..24fdb64 100644
--- a/vmpooler.yaml.example
+++ b/vmpooler.yaml.example
@@ -81,7 +81,10 @@
 #     Overwrites the global domain parameter. This should match the dns zone domain set for the dns_zone_resource_name.
 #     It is used to infer the domain part of the FQDN ie $vm_name.$domain
 #     When setting multiple providers at the same time, this value should be set for each GCE pools.
-#     (optional) If not explicitely set, the FQDN is inferred using the global 'domain' config parameter
+#     (optional) when not set, the dns setup / teardown is skipped and the instance is reachable via the private_dns_name
+#   - project
+#     The GCP project name where the DNS zone resource exists
+#     (optional)
 # Example:
 
   :aws:
@@ -91,6 +94,7 @@
     volume_size: '10'
     dns_zone_resource_name: 'subdomain-example-com'
     domain: 'subdomain.example.com'
+    project: 'gcp-project-1'
 
 # :pools:
 #

From a09deaf1f3722d111c62f504a06441811fc68eb6 Mon Sep 17 00:00:00 2001
From: Samuel Beaulieu <samuel+14401380@puppet.com>
Date: Wed, 27 Jul 2022 13:55:22 -0500
Subject: [PATCH 4/9] fix rubocop offences

---
 lib/vmpooler/providers/ec2.rb | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/lib/vmpooler/providers/ec2.rb b/lib/vmpooler/providers/ec2.rb
index 9dfc108..b018119 100644
--- a/lib/vmpooler/providers/ec2.rb
+++ b/lib/vmpooler/providers/ec2.rb
@@ -196,11 +196,11 @@ module Vmpooler
 
           subnet_id = get_subnet_id(pool_name)
           domain_set = domain
-          if domain_set.nil?
-            name_to_use = new_vmname
-          else
-            name_to_use = "#{new_vmname}.#{domain_set}"
-          end
+          name_to_use = if domain_set.nil?
+                          new_vmname
+                        else
+                          "#{new_vmname}.#{domain_set}"
+                        end
 
           tag = [
             {
@@ -216,7 +216,7 @@ module Vmpooler
                 },
                 {
                   key: 'lifetime', # required by AWS reaper
-                  value: get_max_lifetime
+                  value: max_lifetime
                 },
                 {
                   key: 'created_by', # required by AWS reaper
@@ -479,7 +479,7 @@ module Vmpooler
         end
 
         # returns max_lifetime_upper_limit in hours in the format Xh defaults to 12h
-        def get_max_lifetime
+        def max_lifetime
           max_hours = global_config[:config]['max_lifetime_upper_limit'] || '12'
           "#{max_hours}h"
         end
@@ -498,11 +498,11 @@ module Vmpooler
           return nil if pool_configuration.nil?
 
           domain_set = domain
-          if domain_set.nil?
-            name_to_use = vm_object.private_dns_name
-          else
-            name_to_use = vm_object.tags.detect { |f| f.key == 'Name' }&.value
-          end
+          name_to_use = if domain_set.nil?
+                          vm_object.private_dns_name
+                        else
+                          vm_object.tags.detect { |f| f.key == 'Name' }&.value
+                        end
 
           {
             'name' => name_to_use,

From 5f4a215621bc28e7dfaf935d59ae92e89f235c1c Mon Sep 17 00:00:00 2001
From: Samuel Beaulieu <samuel+14401380@puppet.com>
Date: Wed, 27 Jul 2022 14:34:16 -0500
Subject: [PATCH 5/9] fix spec tests

---
 spec/unit/providers/ec2_spec.rb | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/spec/unit/providers/ec2_spec.rb b/spec/unit/providers/ec2_spec.rb
index 9f33376..f657824 100644
--- a/spec/unit/providers/ec2_spec.rb
+++ b/spec/unit/providers/ec2_spec.rb
@@ -159,7 +159,10 @@ EOT
 
     context 'when VM exists but is missing information' do
       before(:each) do
-        tags = [MockTag.new(key: "vm_name", value: vmname)]
+        tags = [
+          MockTag.new(key: "name", value: vmname),
+          MockTag.new(key: "vm_name", value: vmname)
+        ]
         allow(connection).to receive(:instances).and_return([MockInstance.new(tags: tags)])
       end
 
@@ -194,7 +197,7 @@ EOT
           instance_type: "a1.large",
           private_ip_address: "1.1.1.1",
           tags: [
-                  MockTag.new(key: "vm_name", value: vmname),
+                  MockTag.new(key: "name", value: vmname),
                   MockTag.new(key: "pool", value: poolname)
                 ]
         )
@@ -328,6 +331,8 @@ EOT
         allow(connection).to receive(:client).and_return(client)
         allow(client).to receive(:wait_until)
         allow(instance).to receive(:id)
+        allow(subject).to receive(:get_vm).and_return({})
+        allow(subject).to receive(:dns_teardown).and_return(true)
       end
 
       it 'should return true' do

From 4df7c2b74695ce3db102174848101ac2912b5a5f Mon Sep 17 00:00:00 2001
From: Samuel Beaulieu <samuel+14401380@puppet.com>
Date: Thu, 28 Jul 2022 08:42:28 -0500
Subject: [PATCH 6/9] standardize Name tag, implement tag_vm_user method

---
 lib/vmpooler/providers/ec2.rb   | 47 +++++++++++++--------------------
 spec/unit/providers/ec2_spec.rb | 10 ++++---
 2 files changed, 24 insertions(+), 33 deletions(-)

diff --git a/lib/vmpooler/providers/ec2.rb b/lib/vmpooler/providers/ec2.rb
index b018119..33231ae 100644
--- a/lib/vmpooler/providers/ec2.rb
+++ b/lib/vmpooler/providers/ec2.rb
@@ -64,23 +64,23 @@ module Vmpooler
 
         # main configuration options
         def region
-          return provider_config['region'] if provider_config['region']
+          provider_config['region'] if provider_config['region']
         end
 
         # main configuration options, overridable for each pool
         def zone(pool_name)
           return pool_config(pool_name)['zone'] if pool_config(pool_name)['zone']
-          return provider_config['zone'] if provider_config['zone']
+          provider_config['zone'] if provider_config['zone']
         end
 
         def amisize(pool_name)
           return pool_config(pool_name)['amisize'] if pool_config(pool_name)['amisize']
-          return provider_config['amisize'] if provider_config['amisize']
+          provider_config['amisize'] if provider_config['amisize']
         end
 
         def volume_size(pool_name)
           return pool_config(pool_name)['volume_size'] if pool_config(pool_name)['volume_size']
-          return provider_config['volume_size'] if provider_config['volume_size']
+          provider_config['volume_size'] if provider_config['volume_size']
         end
 
         # dns
@@ -107,7 +107,7 @@ module Vmpooler
         end
 
         def to_provision(pool_name)
-          return pool_config(pool_name)['provision'] if pool_config(pool_name)['provision']
+          pool_config(pool_name)['provision'] if pool_config(pool_name)['provision']
         end
 
         # Base methods that are implemented:
@@ -271,7 +271,7 @@ module Vmpooler
           created_instance = get_vm(pool_name, new_vmname)
 
           @redis.with_metrics do |redis|
-            redis.hset("vmpooler__vm__#{new_vmname}", 'host', created_instance['name'])
+            redis.hset("vmpooler__vm__#{new_vmname}", 'host', created_instance['private_dns_name'])
           end
           # extra setup steps
           provision_node_aws(created_instance['private_dns_name'], pool_name, new_vmname) if to_provision(pool_name) == 'true' || to_provision(pool_name) == true
@@ -382,7 +382,6 @@ module Vmpooler
 
           instance_hash = get_vm(pool_name, vm_name)
           debug_logger("trigger delete_instance #{vm_name}")
-          # vm_hash = get_vm(pool_name, vm_name)
           instances.terminate
           begin
             connection.client.wait_until(:instance_terminated, { instance_ids: [instances.id] })
@@ -398,10 +397,9 @@ module Vmpooler
 
         # check if a vm is ready by opening a socket on port 22
         # if a domain is set, it will use vn_name.domain,
-        # if not then it will use the ip directly (AWS workaround)
+        # if not then it will use the private dns name directly (AWS workaround)
         def vm_ready?(pool_name, vm_name)
           begin
-            # TODO: we could use a healthcheck resource attached to instance
             domain_set = domain
             if domain_set.nil?
               vm_ip = get_vm(pool_name, vm_name)['private_dns_name']
@@ -428,20 +426,18 @@ module Vmpooler
           vm_hash = get_vm(pool, vm_name)
           return false if vm_hash.nil?
 
-          new_labels = vm_hash['labels']
-          # bailing in this case since labels should exist, and continuing would mean losing them
-          return false if new_labels.nil?
+          filters = [{
+                       name: 'tag:vm_name',
+                       values: [vm_name]
+                     }]
+          instances = connection.instances(filters: filters).first
+          return false if instances.nil?
 
           # add new label called token-user, with value as user
-          new_labels['token-user'] = user
-          begin
-            instances_set_labels_request_object = Google::Apis::ComputeV1::InstancesSetLabelsRequest.new(label_fingerprint: vm_hash['label_fingerprint'], labels: new_labels)
-            result = connection.set_instance_labels(project, zone(pool), vm_name, instances_set_labels_request_object)
-            wait_for_zone_operation(project, zone(pool), result)
-          rescue StandardError => _e
-            return false
-          end
+          instances.create_tags(tags:[key:"token-user", value: user])
           true
+        rescue StandardError => _e
+          return false
         end
 
         # END BASE METHODS
@@ -497,15 +493,8 @@ module Vmpooler
           pool_configuration = pool_config(pool_name)
           return nil if pool_configuration.nil?
 
-          domain_set = domain
-          name_to_use = if domain_set.nil?
-                          vm_object.private_dns_name
-                        else
-                          vm_object.tags.detect { |f| f.key == 'Name' }&.value
-                        end
-
           {
-            'name' => name_to_use,
+            'name' => vm_object.tags.detect { |f| f.key == 'Name' }&.value,
             # 'hostname' => vm_object.hostname,
             'template' => pool_configuration&.key?('template') ? pool_configuration['template'] : nil, # was expecting to get it from API, not from config, but this is what vSphere does too!
             'poolname' => vm_object.tags.detect { |f| f.key == 'pool' }&.value,
@@ -513,7 +502,7 @@ module Vmpooler
             'status' => vm_object.state&.name, # One of the following values: pending, running, shutting-down, terminated, stopping, stopped
             # 'zone' => vm_object.zone,
             'image_size' => vm_object.instance_type,
-            'ip' => vm_object.private_ip_address,
+            'ip' => vm_object.private_ip_address, # used by the cloud dns class to set the record to this value
             'private_ip_address' => vm_object.private_ip_address,
             'private_dns_name' => vm_object.private_dns_name
           }
diff --git a/spec/unit/providers/ec2_spec.rb b/spec/unit/providers/ec2_spec.rb
index f657824..1125100 100644
--- a/spec/unit/providers/ec2_spec.rb
+++ b/spec/unit/providers/ec2_spec.rb
@@ -88,7 +88,8 @@ EOT
       }
       skip 'gets a vm' do
         result = subject.create_vm(poolname, vmname)
-        result = subject.destroy_vm(poolname, vmname)
+        subject.tag_vm_user(poolname, vmname)
+        #result = subject.destroy_vm(poolname, vmname)
         #subject.vms_in_pool("amazon-6-x86_64-ec2")
         #subject.provision_node_aws("ip-10-227-4-97.amz-dev.puppet.net", poolname)
         # subject.create_snapshot(poolname, vmname, "foo")
@@ -160,7 +161,7 @@ EOT
     context 'when VM exists but is missing information' do
       before(:each) do
         tags = [
-          MockTag.new(key: "name", value: vmname),
+          MockTag.new(key: "Name", value: vmname),
           MockTag.new(key: "vm_name", value: vmname)
         ]
         allow(connection).to receive(:instances).and_return([MockInstance.new(tags: tags)])
@@ -170,7 +171,8 @@ EOT
         expect(subject.get_vm(poolname, vmname)).to be_kind_of(Hash)
       end
 
-      it 'should return the VM name' do
+      it 'should return the VM name when domain set' do
+        config[:providers][:ec2]['domain'] = "foobar.com"
         result = subject.get_vm(poolname, vmname)
 
         expect(result['name']).to eq(vmname)
@@ -197,7 +199,7 @@ EOT
           instance_type: "a1.large",
           private_ip_address: "1.1.1.1",
           tags: [
-                  MockTag.new(key: "name", value: vmname),
+                  MockTag.new(key: "Name", value: vmname),
                   MockTag.new(key: "pool", value: poolname)
                 ]
         )

From b563239e76e3186c5bf9343856c7331c22020c0d Mon Sep 17 00:00:00 2001
From: Samuel Beaulieu <samuel+14401380@puppet.com>
Date: Thu, 28 Jul 2022 09:13:38 -0500
Subject: [PATCH 7/9] fix rubocop offenses

---
 lib/vmpooler/providers/ec2.rb | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/lib/vmpooler/providers/ec2.rb b/lib/vmpooler/providers/ec2.rb
index 33231ae..541eade 100644
--- a/lib/vmpooler/providers/ec2.rb
+++ b/lib/vmpooler/providers/ec2.rb
@@ -64,23 +64,26 @@ module Vmpooler
 
         # main configuration options
         def region
-          provider_config['region'] if provider_config['region']
+          provider_config['region']
         end
 
         # main configuration options, overridable for each pool
         def zone(pool_name)
           return pool_config(pool_name)['zone'] if pool_config(pool_name)['zone']
-          provider_config['zone'] if provider_config['zone']
+
+          provider_config['zone']
         end
 
         def amisize(pool_name)
           return pool_config(pool_name)['amisize'] if pool_config(pool_name)['amisize']
-          provider_config['amisize'] if provider_config['amisize']
+
+          provider_config['amisize']
         end
 
         def volume_size(pool_name)
           return pool_config(pool_name)['volume_size'] if pool_config(pool_name)['volume_size']
-          provider_config['volume_size'] if provider_config['volume_size']
+
+          provider_config['volume_size']
         end
 
         # dns
@@ -107,7 +110,7 @@ module Vmpooler
         end
 
         def to_provision(pool_name)
-          pool_config(pool_name)['provision'] if pool_config(pool_name)['provision']
+          pool_config(pool_name)['provision']
         end
 
         # Base methods that are implemented:
@@ -427,17 +430,17 @@ module Vmpooler
           return false if vm_hash.nil?
 
           filters = [{
-                       name: 'tag:vm_name',
-                       values: [vm_name]
-                     }]
+            name: 'tag:vm_name',
+            values: [vm_name]
+          }]
           instances = connection.instances(filters: filters).first
           return false if instances.nil?
 
           # add new label called token-user, with value as user
-          instances.create_tags(tags:[key:"token-user", value: user])
+          instances.create_tags(tags: [key: 'token-user', value: user])
           true
         rescue StandardError => _e
-          return false
+          false
         end
 
         # END BASE METHODS

From 6c0e7ac12bb7780ec5705e9d905938e532c84308 Mon Sep 17 00:00:00 2001
From: Samuel Beaulieu <samuel+14401380@puppet.com>
Date: Thu, 28 Jul 2022 13:04:24 -0500
Subject: [PATCH 8/9] add a condition for the aws node setup (ssh) if a domain
 is set then we use the 'spicy-proton' hostname otherwise it uses the private
 dns name

---
 lib/vmpooler/providers/ec2.rb | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/lib/vmpooler/providers/ec2.rb b/lib/vmpooler/providers/ec2.rb
index 541eade..aa0a91f 100644
--- a/lib/vmpooler/providers/ec2.rb
+++ b/lib/vmpooler/providers/ec2.rb
@@ -276,10 +276,14 @@ module Vmpooler
           @redis.with_metrics do |redis|
             redis.hset("vmpooler__vm__#{new_vmname}", 'host', created_instance['private_dns_name'])
           end
-          # extra setup steps
-          provision_node_aws(created_instance['private_dns_name'], pool_name, new_vmname) if to_provision(pool_name) == 'true' || to_provision(pool_name) == true
 
-          dns_setup(created_instance) if domain
+          if domain
+            dns_setup(created_instance)
+            provision_node_aws(created_instance['name'], pool_name, new_vmname) if to_provision(pool_name) == 'true' || to_provision(pool_name) == true
+          elsif to_provision(pool_name) == 'true' || to_provision(pool_name) == true
+            provision_node_aws(created_instance['private_dns_name'], pool_name, new_vmname)
+          end
+
           created_instance
         end
 

From 4153391eaf8ebd4af566949bba24e7297dd122fc Mon Sep 17 00:00:00 2001
From: Samuel Beaulieu <samuel+14401380@puppet.com>
Date: Thu, 28 Jul 2022 13:22:11 -0500
Subject: [PATCH 9/9] do the dns setup earlier, as soon as the vm is ready, to
 give enough time for the DNS record to propagate

---
 lib/vmpooler/providers/ec2.rb | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/lib/vmpooler/providers/ec2.rb b/lib/vmpooler/providers/ec2.rb
index aa0a91f..f857f86 100644
--- a/lib/vmpooler/providers/ec2.rb
+++ b/lib/vmpooler/providers/ec2.rb
@@ -265,20 +265,21 @@ module Vmpooler
           instance_id = batch_instance.first.instance_id
           connection.client.wait_until(:instance_running, { instance_ids: [instance_id] })
           @logger.log('s', "[>] [#{pool_name}] '#{new_vmname}' instance running")
+          created_instance = get_vm(pool_name, new_vmname)
+          dns_setup(created_instance) if domain
+
           ### System status checks
           # This check verifies that your instance is reachable. Amazon EC2 tests that network packets can get to your instance.
           ### Instance status checks
           # This check verifies that your instance's operating system is accepting traffic.
           connection.client.wait_until(:instance_status_ok, { instance_ids: [instance_id] })
           @logger.log('s', "[>] [#{pool_name}] '#{new_vmname}' instance ready to accept traffic")
-          created_instance = get_vm(pool_name, new_vmname)
 
           @redis.with_metrics do |redis|
             redis.hset("vmpooler__vm__#{new_vmname}", 'host', created_instance['private_dns_name'])
           end
 
           if domain
-            dns_setup(created_instance)
             provision_node_aws(created_instance['name'], pool_name, new_vmname) if to_provision(pool_name) == 'true' || to_provision(pool_name) == true
           elsif to_provision(pool_name) == 'true' || to_provision(pool_name) == true
             provision_node_aws(created_instance['private_dns_name'], pool_name, new_vmname)