fix: stop leaking servers and stranding instances on partial failures

README.md

@@ -512,7 +512,9 @@ clouds:
 
 Either is passed through to the HTTP connection. `verify: false` in
 `clouds.yaml` disables verification entirely, equivalent to setting
-`disable_ssl_validation: true`.
+`disable_ssl_validation: true` — unless you set `disable_ssl_validation`
+yourself, in which case `kitchen.yml` wins as it does everywhere else, and
+`disable_ssl_validation: false` keeps verification on.
 
 ## Troubleshooting
 
@@ -528,6 +530,7 @@ kitchen create --log-level=debug
 | Symptom | Likely cause |
 | --- | --- |
 | `Image not found` / `Flavor not found` | The `_ref` matched nothing. Check `openstack image list`. A `/regex/` needs the surrounding slashes. |
+| `Could not parse <...> as a regular expression` | A `/regex/` ref that does not compile — usually an unbalanced bracket. |
 | `Cannot specify both image_ref and image_id` | Set one, not both. Same for flavor and network. |
 | `Could not find an IP` | The instance has no address of the family you asked for. Check `use_ipv6`, and whether you need a floating IP. |
 | `Server is not attached to network <name>` | `openstack_network_name` does not match any network on the instance. |
@@ -537,6 +540,7 @@ kitchen create --log-level=debug
 | `The security_groups config must be an array` | Use a list, even for a single group. |
 | Hangs at "Waiting for server to be ready" | The instance booted but SSH is unreachable. Check the security group allows port 22, that a floating IP is attached if you need one, and that `transport: username:` matches the image's default user. |
 | Times out reaching `ACTIVE` | Raise `glance_cache_wait_timeout`. First boot of a large image can be slow. |
+| `Cinder accepted the volume but returned no id` | The volume create succeeded but the response was unreadable; check the Cinder endpoint and API microversion. |
 | TLS errors | Set `ssl_ca_file` (or `OS_CACERT`) to your CA bundle. |
 
 The instance is destroyed automatically if it never becomes reachable, so a

lib/kitchen/driver/openstack.rb

@@ -156,14 +156,7 @@ def create(state)
         end
         info "OpenStack server ID <#{state[:server_id]}> created"
 
-        if config[:floating_ip]
-          attach_ip(server, config[:floating_ip])
-        elsif config[:floating_ip_pool]
-          attach_ip_from_pool(server, config[:floating_ip_pool])
-        end
-        state[:hostname] = get_ip(server)
-        wait_for_server(state)
-        add_ohai_hint(state)
+        finish_create(server, state)
       rescue Fog::Errors::Error, Excon::Errors::Error => e
         raise ActionFailed, e.message
       end
@@ -195,6 +188,8 @@ def destroy(state)
         info "OpenStack instance <#{state[:server_id]}> destroyed."
         state.delete(:server_id)
         state.delete(:hostname)
+      rescue Fog::Errors::Error, Excon::Errors::Error => e
+        raise ActionFailed, e.message
       end
 
       # Reports what Nova currently thinks of the server.
@@ -233,6 +228,50 @@ def doctor(state) # rubocop:disable Lint/UnusedMethodArgument
 
       private
 
+      # Everything +create+ does once the server is up and billing.
+      #
+      # Each step here can fail on its own -- a pool with no free addresses, a
+      # server with no address of the requested family, a transport that never
+      # comes up -- and every one of those leaves a running server behind that
+      # nothing else is going to clean up. Worse, when
+      # +allocate_floating_ip+ is set the run may also be holding a brand new
+      # floating IP, and +destroy+ can only give that back while it can still
+      # read it off the server. So a failure here tears the server down before
+      # re-raising, which is what +wait_for_server+ already did for its own
+      # step alone.
+      #
+      # @param server [Fog::OpenStack::Compute::Server] the server just built
+      # @param state [Hash] mutable instance state; gains `:hostname`
+      # @return [void]
+      # @raise [StandardError] whatever the failing step raised, after cleanup
+      def finish_create(server, state)
+        if config[:floating_ip]
+          attach_ip(server, config[:floating_ip])
+        elsif config[:floating_ip_pool]
+          attach_ip_from_pool(server, config[:floating_ip_pool])
+        end
+        state[:hostname] = get_ip(server)
+        wait_for_server(state)
+        add_ohai_hint(state)
+      rescue StandardError
+        # wait_for_server destroys the server itself and destroy deletes
+        # :server_id, so by the time that step fails there is nothing left to
+        # clean up and no second message worth printing.
+        raise if state[:server_id].nil?
+
+        error "Destroying OpenStack server ID <#{state[:server_id]}> after a failed create."
+        begin
+          destroy(state)
+        rescue StandardError => cleanup_error
+          # The user needs to hear why the create failed, not why the tidying
+          # up afterwards also failed -- but they do need to know a server was
+          # left behind for them to deal with.
+          error "Could not destroy OpenStack server ID <#{state[:server_id]}>: " \
+                "#{cleanup_error.message}. It may still be running."
+        end
+        raise
+      end
+
       # Looks a server up without turning an unreachable cloud into a failure.
       #
       # @param server_id [String] the Nova server ID

lib/kitchen/driver/openstack/clouds.rb

@@ -99,7 +99,11 @@ def apply_clouds_config
           # (though its env loader does sweep up any OS_* name into an implicit
           # cloud), and ENV_VAR_MAP deliberately mirrors the documented set --
           # so for this driver clouds.yaml is the only source.
-          return unless cc[:ssl_verify_peer] == false && !config[:disable_ssl_validation]
+          #
+          # Nil, not falsy: `disable_ssl_validation: false` in kitchen.yml is a
+          # deliberate instruction to keep verifying, and kitchen.yml outranks
+          # clouds.yaml here exactly as it does for every key above.
+          return unless cc[:ssl_verify_peer] == false && config[:disable_ssl_validation].nil?
 
           config[:disable_ssl_validation] = true
         end

lib/kitchen/driver/openstack/networking.rb

@@ -75,7 +75,15 @@ def allocate_ip_from_pool(pool)
           end
 
           resp = net.create_floating_ip(networks[0]["id"])
-          ip = resp.body["floatingip"]["floating_ip_address"]
+          ip = resp.body.dig("floatingip", "floating_ip_address")
+          # Without this the nil travels on into associate_address, and the
+          # run fails several steps later with an error that says nothing
+          # about the allocation that actually went wrong.
+          if ip.nil?
+            raise ActionFailed,
+              "Allocated a floating IP from <#{pool}> but the response carried no address"
+          end
+
           info "Created floating IP <#{ip}> from <#{pool}> pool"
           ip
         end
@@ -195,11 +203,8 @@ def ip_from_named_network(server)
         # @param addresses [Array<Hash>] address hashes, each with an `"addr"` key
         # @return [Array<Hash>] the matching subset
         def filter_ips(addresses)
-          if config[:use_ipv6]
-            addresses.select { |i| IPAddr.new(i["addr"]).ipv6? }
-          else
-            addresses.select { |i| IPAddr.new(i["addr"]).ipv4? }
-          end
+          wanted = config[:use_ipv6] ? :ipv6? : :ipv4?
+          addresses.select { |i| ip_family_match?(i["addr"], wanted) }
         end
 
         # Normalizes and filters public/private address lists to the configured
@@ -215,9 +220,30 @@ def parse_ips(pub, priv)
           # the caller's list here is the Fog server model's own address data.
           wanted = config[:use_ipv6] ? :ipv6? : :ipv4?
           [Array(pub), Array(priv)].map do |addrs|
-            addrs.select { |i| IPAddr.new(i).public_send(wanted) }
+            addrs.select { |i| ip_family_match?(i, wanted) }
           end
         end
+
+        # Whether one address from Nova is of the IP family we want.
+        #
+        # IPAddr raises on anything it cannot parse, and that exception is an
+        # ArgumentError -- neither a Fog nor an Excon error, so neither
+        # +create+ nor +destroy+ would turn it into something a user can act
+        # on. A single odd entry in a server's address list used to abort a
+        # destroy before it reached +server.destroy+, stranding the instance,
+        # so an unparsable address is now logged and skipped instead.
+        #
+        # @param addr [String] an address as Nova reported it
+        # @param wanted [Symbol] `:ipv4?` or `:ipv6?`
+        # @return [Boolean] true when the address parses and matches
+        def ip_family_match?(addr, wanted)
+          IPAddr.new(addr.to_s).public_send(wanted)
+        rescue ArgumentError
+          # IPAddr::InvalidAddressError descends from IPAddr::Error, which
+          # descends from ArgumentError; catching the base covers both.
+          warn "Ignoring unparsable address <#{addr}> reported by OpenStack"
+          false
+        end
       end
     end
   end

lib/kitchen/driver/openstack/server_helper.rb

@@ -187,10 +187,15 @@ def find_network(network_ref)
         # @param collection [Enumerable] the Fog collection to search
         # @param name [String] id, name, or `/regex/`
         # @return [Object, nil] the first match, or nil
+        # @raise [Kitchen::ActionFailed] if the ref looks like a regex but is
+        #   not a valid one
         def find_matching(collection, name)
           name = name.to_s
-          if name.start_with?("/") && name.end_with?("/")
-            regex = Regexp.new(name[1...-1])
+          # A one-character "/" starts and ends with a slash, but the pattern
+          # between the slashes is empty and an empty Regexp matches the first
+          # resource in the collection -- which is never what anyone meant.
+          if name.length > 1 && name.start_with?("/") && name.end_with?("/")
+            regex = compile_ref_regex(name)
             # check for regex name match, skipping unnamed resources; Neutron
             # networks in particular are allowed to have no name
             collection.each { |single| return single if single.name && regex.match?(single.name) }
@@ -203,6 +208,22 @@ def find_matching(collection, name)
           end
           nil
         end
+
+        # Compiles the pattern out of a `/regex/` style ref.
+        #
+        # An unbalanced bracket or a stray quantifier is a typo in kitchen.yml,
+        # not a bug in the driver, so it is reported as configuration the user
+        # can go and fix rather than as a raw RegexpError backtrace from the
+        # middle of a create.
+        #
+        # @param name [String] the ref, slashes included
+        # @return [Regexp] the compiled pattern
+        # @raise [Kitchen::ActionFailed] if the pattern does not compile
+        def compile_ref_regex(name)
+          Regexp.new(name[1...-1])
+        rescue RegexpError => e
+          raise ActionFailed, "Could not parse <#{name}> as a regular expression: #{e.message}"
+        end
       end
     end
   end

lib/kitchen/driver/openstack/volume.rb

@@ -96,7 +96,11 @@ def create_volume(config, os)
               bdm[:volume_size],
               opt
             )
-          vol_id = resp[:body]["volume"]["id"]
+          vol_id = resp[:body].dig("volume", "id") if resp[:body].respond_to?(:dig)
+          # A create that answers 200 with a body we cannot read an id out of
+          # is not something to carry on from: without the id nothing can wait
+          # on the volume, attach it, or delete it again.
+          raise(ActionFailed, "Cinder accepted the volume but returned no id") if vol_id.nil?
 
           wait_for_volume(volume_service, vol_id, creation_timeout, attach_timeout)
 
@@ -145,7 +149,10 @@ def wait_for_volume(volume_service, vol_id, creation_timeout, attach_timeout)
           @logger.debug "Waiting for volume to be ready for #{creation_timeout} seconds"
           vol_model.wait_for(creation_timeout) do
             sleep(1)
-            raise(ActionFailed, "Failed to make volume #{vol_id}") if status.casecmp("error") == 0
+            # Cinder can answer with no status at all for the first moments
+            # after a create; that is "not ready yet", not a reason to raise
+            # NoMethodError out of the middle of the wait.
+            raise(ActionFailed, "Failed to make volume #{vol_id}") if status.to_s.casecmp("error") == 0
 
             ready?
           end

spec/kitchen/driver/openstack/clouds_spec.rb

@@ -609,6 +609,32 @@ def use_clouds_file(content = clouds_yaml, secure: nil, env: {})
         expect(driver[:disable_ssl_validation]).to be_nil
       end
 
+      # kitchen.yml outranks clouds.yaml for every other key, and writing
+      # `disable_ssl_validation: false` is a deliberate instruction to keep
+      # verifying. The guard used to be a plain falsy check, so the explicit
+      # false read the same as "unset" and clouds.yaml overrode it.
+      it "does not override an explicit disable_ssl_validation: false" do
+        cloud = clouds_yaml
+        cloud["clouds"]["mycloud"]["verify"] = false
+        use_clouds_file(cloud, env: { "OS_CLOUD" => "mycloud" })
+        config[:disable_ssl_validation] = false
+
+        driver.send(:apply_clouds_config)
+
+        expect(driver[:disable_ssl_validation]).to be(false)
+      end
+
+      it "leaves an explicit disable_ssl_validation: true alone" do
+        cloud = clouds_yaml
+        cloud["clouds"]["mycloud"]["verify"] = false
+        use_clouds_file(cloud, env: { "OS_CLOUD" => "mycloud" })
+        config[:disable_ssl_validation] = true
+
+        driver.send(:apply_clouds_config)
+
+        expect(driver[:disable_ssl_validation]).to be(true)
+      end
+
       it "carries a cacert from clouds.yaml into ssl_ca_file" do
         cloud = clouds_yaml
         cloud["clouds"]["mycloud"]["cacert"] = "/path/ca.crt"

spec/kitchen/driver/openstack/networking_spec.rb

@@ -128,6 +128,43 @@
             .to raise_error(Kitchen::ActionFailed, "Floating IP pool <swimmers> not found")
         end
       end
+
+      # The list_networks response is guarded above; the create response was
+      # not, so a body without an address handed a nil straight on to
+      # associate_address and failed later with an error about the wrong thing.
+      context "when the allocation response carries no address" do
+        let(:net) do
+          fog_network(
+            list_networks: fog_response(networks_body),
+            create_floating_ip: fog_response("floatingip" => {})
+          )
+        end
+
+        it "fails naming the pool" do
+          expect { driver.send(:attach_ip_from_pool, server, "swimmers") }
+            .to raise_error(Kitchen::ActionFailed, /from <swimmers> but the response carried no address/)
+        end
+
+        it "does not try to attach a nil address" do
+          expect { driver.send(:attach_ip_from_pool, server, "swimmers") }
+            .to raise_error(Kitchen::ActionFailed)
+          expect(server).not_to have_received(:associate_address)
+        end
+      end
+
+      context "when the allocation response has no floatingip key at all" do
+        let(:net) do
+          fog_network(
+            list_networks: fog_response(networks_body),
+            create_floating_ip: fog_response({})
+          )
+        end
+
+        it "fails naming the pool" do
+          expect { driver.send(:attach_ip_from_pool, server, "swimmers") }
+            .to raise_error(Kitchen::ActionFailed, /from <swimmers> but the response carried no address/)
+        end
+      end
     end
 
     it "serializes pool access across threads" do
@@ -415,6 +452,28 @@
 
       expect(driver.send(:filter_ips, [{ "addr" => "1.2.3.4" }])).to eq([])
     end
+
+    # IPAddr raises ArgumentError on anything it cannot parse, which no caller
+    # up the stack translates -- one odd entry used to take out the whole run.
+    context "with an address Nova reported that will not parse" do
+      let(:addresses) { [{ "addr" => "1.2.3.4" }, { "addr" => "not-an-ip" }] }
+
+      it "skips it and keeps the rest" do
+        expect(driver.send(:filter_ips, addresses)).to eq([{ "addr" => "1.2.3.4" }])
+      end
+
+      it "says which address it skipped" do
+        driver.send(:filter_ips, addresses)
+
+        expect(logged_output.string).to include("Ignoring unparsable address <not-an-ip>")
+      end
+    end
+
+    context "with a nil address" do
+      it "skips it" do
+        expect(driver.send(:filter_ips, [{ "addr" => nil }])).to eq([])
+      end
+    end
   end
 
   describe "#parse_ips" do
@@ -478,5 +537,12 @@
       expect(result_pub).to eq(%w{1.2.3.4})
       expect(result_pub).not_to be(pub)
     end
+
+    context "with an address that will not parse" do
+      it "skips it rather than raising out of the middle of a create" do
+        expect(driver.send(:parse_ips, ["1.2.3.4", "not-an-ip"], ["10.0.0.1 "]))
+          .to eq([%w{1.2.3.4}, []])
+      end
+    end
   end
 end

spec/kitchen/driver/openstack/server_helper_spec.rb

@@ -394,6 +394,25 @@
 
       expect(driver.send(:find_matching, unnamed, "/.*/").id).to eq("222")
     end
+
+    # A lone slash starts and ends with a slash, so it used to be compiled as
+    # the empty pattern -- which matches everything, and quietly booted
+    # whatever image happened to come back first.
+    it "does not treat a lone slash as a match-everything regex" do
+      expect(driver.send(:find_matching, collection, "/")).to be_nil
+    end
+
+    # An unbalanced bracket is a typo in kitchen.yml, and used to surface as a
+    # raw RegexpError from the middle of a create.
+    it "reports an unparsable regex as configuration to fix" do
+      expect { driver.send(:find_matching, collection, "/[/") }
+        .to raise_error(Kitchen::ActionFailed, %r{Could not parse </\[/> as a regular expression})
+    end
+
+    it "does not swallow the reason the pattern would not compile" do
+      expect { driver.send(:find_matching, collection, "/*/") }
+        .to raise_error(Kitchen::ActionFailed, /as a regular expression: /)
+    end
   end
 
   describe "#find_image" do