From 3410e6eac72033f12a24411c926a5219a3be8f73 Mon Sep 17 00:00:00 2001 From: Tim Smith Date: Mon, 24 Aug 2026 14:24:38 -0700 Subject: [PATCH] feat: replace fog-cloudstack with cloudstack_client fog-cloudstack has had exactly one release, 0.1.0, in June 2019. This moves the driver onto cloudstack_client, which is maintained and which has no runtime dependencies at all, so fog-core, fog-json, fog-xml, excon and nokogiri all leave the dependency tree with nothing taking their place. The driver's configuration is unchanged. Internally: - Client#compute becomes Client#api and returns a CloudstackClient::Client. - cloudstack_client strips the response envelope, so responses are read directly rather than through fetch("listzonesresponse") and friends. - Every asynchronous call passes sync: true. cloudstack_client can wait for jobs itself, but hands back only the finished job result, and the driver needs the id of the resource being built so it can record it in state before waiting -- otherwise an interrupted create leaves a VM behind with nothing in state to destroy it by. Client#run_job keeps doing the waiting, so cloudstack_job_poll_interval and cloudstack_job_timeout keep working. - Teardown rescues CloudstackClient::ApiError in place of Fog::Cloudstack::Compute::BadRequest. Three things needed care because cloudstack_client validates arguments against a bundled CloudStack 4.5 API definition and silently drops anything it does not recognise: - Custom service offering sizing moves from flat details[0].cpuNumber parameters to CloudStack's details map, which is the shape the API documents. Sent flat they were dropped, and instances came up with the offering's default CPU and memory. - openfirewall and expunge are sent as strings. cloudstack_client drops falsey values, and openfirewall defaults to true on a non-VPC network, so a dropped "false" would let CloudStack open the firewall itself and create a rule teardown does not know about. - cloudstack_project_id was applied by fog to every request from the connection. cloudstack_client has no equivalent, so it is now passed to the four commands that accept it: deployVirtualMachine, listVirtualMachines, associateIpAddress and listNetworks. It was also being sent to createFirewallRule, which has no such parameter, so that is dropped. disable_ssl_validation no longer sets Excon.defaults[:ssl_verify_peer], which turned verification off process-wide for anything else sharing the process. It is now scoped to this driver's connection, and it applies to kitchen doctor as well, which previously could not check an endpoint using a self-signed certificate. The integration spec still runs a full create/status/destroy cycle with only the HTTP layer stubbed, so request signing and response parsing are exercised for real. Signed-off-by: Tim Smith --- CONTRIBUTING.md | 14 ++- Gemfile | 8 ++ README.md | 2 +- kitchen-cloudstack.gemspec | 2 +- lib/kitchen/driver/cloudstack.rb | 30 ++--- lib/kitchen/driver/cloudstack/client.rb | 65 +++++------ lib/kitchen/driver/cloudstack/networking.rb | 61 +++++----- .../driver/cloudstack/server_options.rb | 32 ++++- spec/integration/lifecycle_spec.rb | 42 +++++-- spec/kitchen/driver/cloudstack/client_spec.rb | 87 +++++++------- .../driver/cloudstack/networking_spec.rb | 110 +++++++++++++----- .../driver/cloudstack/server_options_spec.rb | 40 +++++++ spec/kitchen/driver/cloudstack_spec.rb | 47 +++++--- 13 files changed, 353 insertions(+), 187 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index cc3b0c1..babaa60 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -18,9 +18,9 @@ most useful thing you can do for this project. Other changes that would be welcome: -- Wrapping CloudStack and Excon errors in `Kitchen::ActionFailed`, so that bad - credentials produce a readable message rather than a `Fog::JSON::DecodeError` - and a stack trace. +- Wrapping CloudStack API errors in `Kitchen::ActionFailed`, so that bad + credentials produce a readable message rather than a + `CloudstackClient::ApiError` and a stack trace. - Making `create` idempotent, so that running it against an instance that already exists in state does not deploy a second one. - Looking up templates, service offerings, zones and networks by name rather @@ -72,10 +72,12 @@ Specs live under `spec/`: - `spec/kitchen/driver/cloudstack/` covers each supporting class in isolation. `ServerOptions` and `Credentials` are plain objects and are tested directly. - `spec/integration/lifecycle_spec.rb` runs a full create/status/destroy cycle - through real Test Kitchen and real fog, stubbing only the HTTP layer, so - request signing, response parsing and plugin wiring are all exercised. + through real Test Kitchen and a real `cloudstack_client`, stubbing only the + HTTP layer, so request signing, response parsing and plugin wiring are all + exercised. -Unit specs inject a fake client rather than stubbing `Fog::Compute` globally. +Unit specs inject a fake client rather than stubbing `CloudstackClient::Client` +globally. If you add behaviour that talks to CloudStack, prefer the same approach: it keeps the tests fast and makes it obvious which API calls a change actually makes. diff --git a/Gemfile b/Gemfile index c98a562..c75b538 100644 --- a/Gemfile +++ b/Gemfile @@ -1,6 +1,14 @@ source "https://rubygems.org" gemspec development_group: :test + +# TEMPORARY: cloudstack_client 1.6.0 cannot be used outside of cloudstack-cli. +# It calls ActiveSupport's `present?` on every request, and `require "base64"` +# is a LoadError on Ruby 3.4+. Both are fixed in niwo/cloudstack_client#20. +# Remove this override and pin the gemspec to the release that carries the fix. +gem "cloudstack_client", git: "https://github.com/tas50/cloudstack_client.git", + branch: "fix-standalone-library-use" + group :docs do gem "yard" end diff --git a/README.md b/README.md index e6d571d..a9f51fa 100644 --- a/README.md +++ b/README.md @@ -12,7 +12,7 @@ A [Test Kitchen](https://kitchen.ci/) driver for [Apache CloudStack](https://clo - API credentials for it: an API key, a secret key, and the API URL - Test Kitchen 3.0 or newer, including the version bundled with current Cinc Workstation and Chef Workstation -- `fog-cloudstack`, installed automatically as a dependency +- `cloudstack_client`, installed automatically as a dependency ## Installation diff --git a/kitchen-cloudstack.gemspec b/kitchen-cloudstack.gemspec index f8d1f5b..8a2fde6 100644 --- a/kitchen-cloudstack.gemspec +++ b/kitchen-cloudstack.gemspec @@ -25,5 +25,5 @@ Gem::Specification.new do |spec| } spec.add_dependency "test-kitchen", ">= 3.0", "< 5" - spec.add_dependency "fog-cloudstack", "~> 0.1.0" + spec.add_dependency "cloudstack_client", "~> 1.6" end diff --git a/lib/kitchen/driver/cloudstack.rb b/lib/kitchen/driver/cloudstack.rb index d9aed77..0104a34 100644 --- a/lib/kitchen/driver/cloudstack.rb +++ b/lib/kitchen/driver/cloudstack.rb @@ -64,7 +64,6 @@ class Cloudstack < Kitchen::Driver::Base # @return [void] def create(state) super - disable_ssl_validation! if config[:disable_ssl_validation] server_info = deploy_instance(state) @@ -88,10 +87,11 @@ def destroy(state) networking.teardown(state) if config[:associate_public_ip] - client.compute.destroy_virtual_machine( + client.api.destroy_virtual_machine({ "id" => state[:server_id], - "expunge" => !!config[:cloudstack_expunge] - ) + # Stringified because cloudstack_client drops falsey values. + "expunge" => (!!config[:cloudstack_expunge]).to_s, + }, Client::SYNC) info("CloudStack instance <#{state[:server_id]}> destroyed.") STATE_KEYS.each { |key| state.delete(key) } @@ -176,7 +176,7 @@ def endpoint_problems uri = URI.parse(config[:cloudstack_api_url]) return ["cloudstack_api_url (#{config[:cloudstack_api_url]}) has no host."] if uri.host.nil? - client.compute.list_zones + client.api.list_zones [] rescue URI::InvalidURIError => e ["cloudstack_api_url (#{config[:cloudstack_api_url]}) is not a URL: #{e.message}"] @@ -193,8 +193,7 @@ def deploy_instance(state) options = ServerOptions.new(config, instance_name: instance.name).to_h debug("Deploying CloudStack instance with #{options}") - response = client.compute.deploy_virtual_machine(options) - .fetch("deployvirtualmachineresponse") + response = client.api.deploy_virtual_machine(options, Client::SYNC) state[:server_id] = response.fetch("id") info("CloudStack instance <#{state[:server_id]}> created.") @@ -261,8 +260,10 @@ def wait_for_guest_password_sync # @return [String, nil] e.g. +"Running"+, or nil when CloudStack returns # no matching machine def lookup_instance_state(server_id) - response = client.compute.list_virtual_machines("id" => server_id) - machines = response.fetch("listvirtualmachinesresponse", {})["virtualmachine"] + machines = client.api.list_virtual_machines( + "id" => server_id, + "projectid" => config[:cloudstack_project_id] + ) return nil unless machines.is_a?(Array) && !machines.empty? machines.first["state"] @@ -282,17 +283,6 @@ def networking def transport_port instance.transport[:port] end - - # Turns off TLS certificate verification for every Excon request. - # - # This is process-wide, not scoped to this driver, which is why it is - # only done when +disable_ssl_validation+ is explicitly set. - # - # @return [void] - def disable_ssl_validation! - require "excon" unless defined?(Excon) - Excon.defaults[:ssl_verify_peer] = false - end end end end diff --git a/lib/kitchen/driver/cloudstack/client.rb b/lib/kitchen/driver/cloudstack/client.rb index 4405569..a9505d1 100644 --- a/lib/kitchen/driver/cloudstack/client.rb +++ b/lib/kitchen/driver/cloudstack/client.rb @@ -13,8 +13,7 @@ require "kitchen/driver/base" require "kitchen/errors" -require "fog/cloudstack" -require "uri" unless defined?(URI) +require "cloudstack_client" module Kitchen module Driver @@ -24,6 +23,14 @@ class Cloudstack < Kitchen::Driver::Base # Almost every CloudStack call that changes something returns a job id # rather than a result, so callers use {#run_job} to turn that job id # into the eventual result, or into an ActionFailed. + # + # cloudstack_client can wait for those jobs itself, but doing so hands + # back only the finished job result -- the caller never sees the id of + # the resource being built. The driver records that id in instance state + # before waiting, so that an interrupted +kitchen create+ still leaves + # behind enough state to destroy what it started. Every asynchronous + # call is therefore made with +sync: true+, which returns CloudStack's + # immediate response, and the waiting happens here instead. class Client # Value CloudStack reports in an async job's "jobstatus" field while # the job is still running. @@ -43,36 +50,37 @@ class Client # not configured. DEFAULT_TIMEOUT = 600 + # Options passed to every asynchronous API call, asking for + # CloudStack's immediate response rather than cloudstack_client's own + # job polling. See the class documentation for why. + SYNC = { sync: true }.freeze + # @param config [Hash] the driver configuration - # @param compute [Fog::Compute, nil] an existing connection, for tests + # @param api [CloudstackClient::Client, nil] an existing connection, + # for tests # @param sleeper [#call, nil] receives a number of seconds to wait, # for tests that must not actually sleep - def initialize(config, compute: nil, sleeper: nil) + def initialize(config, api: nil, sleeper: nil) @config = config - @compute = compute + @api = api @sleeper = sleeper || ->(seconds) { sleep(seconds) } end - # The fog CloudStack connection, built from the configured endpoint. + # The CloudStack connection, built from the configured endpoint. # - # The API URL is split into scheme, host, port, and path because fog - # wants them separately rather than as one URL. + # Certificate verification is on unless +disable_ssl_validation+ asks + # for it to be off, and unlike the fog connection this replaced, that + # choice is scoped to this connection rather than set process-wide. # - # @return [Fog::Compute] a CloudStack compute connection - def compute - @compute ||= begin - uri = URI.parse(config[:cloudstack_api_url]) - Fog::Compute.new( - provider: :cloudstack, - cloudstack_api_key: config[:cloudstack_api_key], - cloudstack_secret_access_key: config[:cloudstack_secret_key], - cloudstack_host: uri.host, - cloudstack_port: uri.port, - cloudstack_path: uri.path, - cloudstack_project_id: config[:cloudstack_project_id], - cloudstack_scheme: uri.scheme - ) - end + # @return [CloudstackClient::Client] a CloudStack API connection + def api + @api ||= CloudstackClient::Client.new( + config[:cloudstack_api_url], + config[:cloudstack_api_key], + config[:cloudstack_secret_key], + quiet: true, + ssl_verify: !config[:disable_ssl_validation] + ) end # Waits for an asynchronous CloudStack job to finish. @@ -84,7 +92,7 @@ def run_job(jobid) elapsed = 0 loop do - response = compute.query_async_job_result(jobid)["queryasyncjobresultresponse"] + response = api.query_async_job_result("jobid" => jobid) case response.fetch("jobstatus").to_i when JOB_SUCCEEDED @@ -103,15 +111,6 @@ def run_job(jobid) end end - # Runs a request that returns an async job id, and waits for it. - # - # @param response [Hash] the raw response from a fog request - # @param key [String] the response envelope key holding the job id - # @return [Hash] the job's "jobresult" payload - def run_response_job(response, key) - run_job(response.fetch(key).fetch("jobid")) - end - private attr_reader :config, :sleeper diff --git a/lib/kitchen/driver/cloudstack/networking.rb b/lib/kitchen/driver/cloudstack/networking.rb index bdbc8bc..d175a9a 100644 --- a/lib/kitchen/driver/cloudstack/networking.rb +++ b/lib/kitchen/driver/cloudstack/networking.rb @@ -12,7 +12,8 @@ # limitations under the License. require "kitchen/driver/base" -require "fog/cloudstack" +require "cloudstack_client" +require_relative "client" module Kitchen module Driver @@ -25,7 +26,7 @@ class Cloudstack < Kitchen::Driver::Base # transport connects on, so a WinRM instance gets 5985 rather than SSH's # 22 without any extra configuration. class Networking - # CloudStack reports an already-deleted resource as a BadRequest with + # CloudStack reports an already-deleted resource as an API error with # this in the message. Teardown treats it as success. ALREADY_GONE = /does not exist/ @@ -45,12 +46,13 @@ def initialize(config, client:, port:, logger: nil) # @param state [Hash] mutable instance state # @return [String] the allocated public IP address def associate_public_ip(state) - response = compute.associate_ip_address( + response = api.associate_ip_address({ "zoneid" => config[:cloudstack_zone_id], "vpcid" => vpc_id, - "networkid" => config[:cloudstack_network_id] - ) - result = client.run_response_job(response, "associateipaddressresponse") + "networkid" => config[:cloudstack_network_id], + "projectid" => config[:cloudstack_project_id], + }, Client::SYNC) + result = client.run_job(response.fetch("jobid")) address = result.fetch("ipaddress") state[:ipaddressid] = address.fetch("id") @@ -65,17 +67,21 @@ def associate_public_ip(state) # @param virtualmachine_id [String] the instance to forward to # @return [void] def create_port_forward(state, virtualmachine_id) - response = compute.create_port_forwarding_rule( + response = api.create_port_forwarding_rule({ "ipaddressid" => state[:ipaddressid], "privateport" => port, "protocol" => "TCP", "publicport" => port, "virtualmachineid" => virtualmachine_id, "networkid" => config[:cloudstack_network_id], - "openfirewall" => false - ) - state[:forwardingruleid] = response.fetch("createportforwardingruleresponse", {})["id"] - client.run_response_job(response, "createportforwardingruleresponse") + # cloudstack_client drops falsey argument values, and CloudStack + # defaults this to true on a non-VPC network, so a boolean false + # here would let CloudStack open the firewall itself -- creating a + # rule teardown does not know about. + "openfirewall" => "false", + }, Client::SYNC) + state[:forwardingruleid] = response["id"] + client.run_job(response.fetch("jobid")) end # Removes everything {#associate_public_ip} and {#create_port_forward} @@ -93,23 +99,22 @@ def teardown(state) attr_reader :config, :client, :port, :logger - # @return [Fog::Compute] the shared CloudStack connection - def compute = client.compute + # @return [CloudstackClient::Client] the shared CloudStack connection + def api = client.api # Opens the transport's port on the allocated public address. # # @param state [Hash] mutable instance state; gains +firewall_rule_id+ # @return [void] def create_firewall_rule(state) - response = compute.create_firewall_rule( - "projectid" => config[:cloudstack_project_id], + response = api.create_firewall_rule({ "cidrlist" => config[:cloudstack_firewall_cidr] || "0.0.0.0/0", "protocol" => "tcp", "startport" => port, "endport" => port, - "ipaddressid" => state[:ipaddressid] - ) - result = client.run_response_job(response, "createfirewallruleresponse") + "ipaddressid" => state[:ipaddressid], + }, Client::SYNC) + result = client.run_job(response.fetch("jobid")) rule = result["firewallrule"] state[:firewall_rule_id] = rule["id"] if rule.is_a?(Hash) end @@ -120,8 +125,8 @@ def create_firewall_rule(state) # @return [void] def delete_port_forward(state) tolerating_missing("port forwarding rule") do - response = compute.delete_port_forwarding_rule(state[:forwardingruleid]) - client.run_response_job(response, "deleteportforwardingruleresponse") + response = api.delete_port_forwarding_rule({ "id" => state[:forwardingruleid] }, Client::SYNC) + client.run_job(response.fetch("jobid")) end end @@ -131,8 +136,8 @@ def delete_port_forward(state) # @return [void] def delete_firewall_rule(state) tolerating_missing("firewall rule") do - response = compute.delete_firewall_rule(state[:firewall_rule_id]) - client.run_response_job(response, "deletefirewallruleresponse") + response = api.delete_firewall_rule({ "id" => state[:firewall_rule_id] }, Client::SYNC) + client.run_job(response.fetch("jobid")) end end @@ -142,8 +147,8 @@ def delete_firewall_rule(state) # @return [void] def release_public_ip(state) tolerating_missing("public IP address") do - response = compute.disassociate_ip_address(state[:ipaddressid]) - client.run_response_job(response, "disassociateipaddressresponse") + response = api.disassociate_ip_address({ "id" => state[:ipaddressid] }, Client::SYNC) + client.run_job(response.fetch("jobid")) end end @@ -153,11 +158,11 @@ def release_public_ip(state) # @param description [String] names the resource, for the debug message # @yield the deletion call to attempt # @return [void] - # @raise [Fog::Cloudstack::Compute::BadRequest] for any error other - # than the resource already being gone + # @raise [CloudstackClient::ApiError] for any error other than the + # resource already being gone def tolerating_missing(description) yield - rescue Fog::Cloudstack::Compute::BadRequest => e + rescue CloudstackClient::ApiError => e raise unless e.to_s.match?(ALREADY_GONE) logger&.debug("CloudStack #{description} was already gone: #{e}") @@ -165,7 +170,7 @@ def tolerating_missing(description) # A VPC network needs its vpcid passed when allocating an address. def vpc_id - networks = compute.list_networks.fetch("listnetworksresponse", {})["network"] + networks = api.list_networks("projectid" => config[:cloudstack_project_id]) return nil unless networks.is_a?(Array) network = networks.find { |n| n["id"] == config[:cloudstack_network_id] } diff --git a/lib/kitchen/driver/cloudstack/server_options.rb b/lib/kitchen/driver/cloudstack/server_options.rb index 0a7d658..09eb10b 100644 --- a/lib/kitchen/driver/cloudstack/server_options.rb +++ b/lib/kitchen/driver/cloudstack/server_options.rb @@ -40,9 +40,16 @@ class ServerOptions "diskofferingid" => :cloudstack_diskoffering_id, "size" => :cloudstack_diskoffering_size, "name" => :host_name, - "details[0].cpuNumber" => :cloudstack_serviceoffering_cpu, - "details[0].cpuSpeed" => :cloudstack_serviceoffering_cpuspeed, - "details[0].memory" => :cloudstack_serviceoffering_memory, + "projectid" => :cloudstack_project_id, + }.freeze + + # Custom service offering sizing, keyed by the name CloudStack expects + # inside the "details" map. Any entry whose config value is nil is + # dropped, so a partially specified offering sends only what was set. + DETAIL_PARAMS = { + "cpuNumber" => :cloudstack_serviceoffering_cpu, + "cpuSpeed" => :cloudstack_serviceoffering_cpuspeed, + "memory" => :cloudstack_serviceoffering_memory, }.freeze # Matches a string that is already valid base64, so user data supplied @@ -74,6 +81,9 @@ def to_h params[param] = value unless value.nil? end + details = sizing_details + params["details"] = [details] unless details.empty? + params[:userdata] = userdata if config[:cloudstack_userdata] params[:templateid] = config[:cloudstack_template_id] @@ -93,6 +103,22 @@ def display_name attr_reader :config, :instance_name, :login, :hostname + # The custom sizing to send as CloudStack's "details" map. + # + # This is a map rather than three flat parameters because that is what + # the API takes. cloudstack_client checks every argument against the + # API definition and drops the ones it does not recognise, so flat + # "details[0].cpuNumber" parameters would be discarded silently and + # the instance would come up with the offering's default sizing. + # + # @return [Hash] the set sizing values, which may be empty + def sizing_details + DETAIL_PARAMS.each_with_object({}) do |(detail, config_key), details| + value = config[config_key] + details[detail] = value unless value.nil? + end + end + # Builds a name that is unique per run and short enough for CloudStack. # # The three descriptive parts are truncated proportionally until the diff --git a/spec/integration/lifecycle_spec.rb b/spec/integration/lifecycle_spec.rb index a6870dd..9767c92 100644 --- a/spec/integration/lifecycle_spec.rb +++ b/spec/integration/lifecycle_spec.rb @@ -3,21 +3,38 @@ require "kitchen/transport/dummy" require "kitchen/provisioner/dummy" require "kitchen/verifier/dummy" -require "excon" +require "json" +require "net/http" require "tmpdir" -# Exercises the whole stack -- Test Kitchen, the driver, and fog -- with only -# the HTTP boundary stubbed, so the real request signing, response parsing and -# plugin wiring all run. +# Exercises the whole stack -- Test Kitchen, the driver, and cloudstack_client +# -- with only the HTTP boundary stubbed, so the real request signing, response +# parsing and plugin wiring all run. RSpec.describe "CloudStack instance lifecycle" do + # Stands in for Net::HTTP, answering each request from #api_response. + class StubHttp + attr_accessor :use_ssl, :verify_mode, :read_timeout + + def initialize(&responder) + @responder = responder + end + + def request(req) + body = @responder.call(req.uri || URI.parse(req.path)) + response = Net::HTTPOK.new("1.1", "200", "") + response.instance_variable_set(:@body, body) + response.instance_variable_set(:@read, true) + response + end + end + around do |example| - previous = Excon.defaults[:mock] - Excon.defaults[:mock] = true - Excon.stub({}) { |request| api_response(request) } + original = Net::HTTP.method(:new) + stub = StubHttp.new { |uri| api_response(uri) } + Net::HTTP.define_singleton_method(:new) { |*_args| stub } example.run ensure - Excon.stubs.clear - Excon.defaults[:mock] = previous + Net::HTTP.define_singleton_method(:new, original) end let(:vm) do @@ -30,8 +47,8 @@ end # Answers the handful of CloudStack commands this lifecycle issues. - def api_response(request) - command = URI.decode_www_form(request[:query].to_s).to_h["command"] + def api_response(uri) + command = URI.decode_www_form(uri.query.to_s).to_h["command"] body = case command when "deployVirtualMachine" @@ -42,6 +59,7 @@ def api_response(request) } } when "listVirtualMachines" { "listvirtualmachinesresponse" => { + "count" => 1, "virtualmachine" => [{ "id" => "vm-e2e", "state" => "Running" }], } } when "destroyVirtualMachine" @@ -50,7 +68,7 @@ def api_response(request) {} end - { status: 200, body: Fog::JSON.encode(body), headers: { "Content-Type" => "application/json" } } + JSON.generate(body) end let(:driver) do diff --git a/spec/kitchen/driver/cloudstack/client_spec.rb b/spec/kitchen/driver/cloudstack/client_spec.rb index 4d0da63..960dea7 100644 --- a/spec/kitchen/driver/cloudstack/client_spec.rb +++ b/spec/kitchen/driver/cloudstack/client_spec.rb @@ -3,7 +3,7 @@ RSpec.describe Kitchen::Driver::Cloudstack::Client do # Records the arguments it is called with and replays queued responses. - class FakeCompute + class FakeApi attr_reader :job_queries def initialize(responses) @@ -11,20 +11,22 @@ def initialize(responses) @job_queries = [] end - def query_async_job_result(jobid) - @job_queries << jobid - @responses.shift || raise("FakeCompute ran out of responses") + def query_async_job_result(args) + @job_queries << args + @responses.shift || raise("FakeApi ran out of responses") end end + # cloudstack_client strips the response envelope, so a job query comes back + # as the job payload itself rather than wrapped in a named key. def job_response(status, result = {}) - { "queryasyncjobresultresponse" => { "jobstatus" => status, "jobresult" => result } } + { "jobstatus" => status, "jobresult" => result } end def client_for(responses, config = {}) described_class.new( { cloudstack_job_poll_interval: 1, cloudstack_job_timeout: 5 }.merge(config), - compute: FakeCompute.new(responses), + api: FakeApi.new(responses), sleeper: ->(_seconds) {} ) end @@ -55,58 +57,61 @@ def client_for(responses, config = {}) expect { client.run_job("job-1") }.to raise_error(Kitchen::ActionFailed, /timed out/i) end - it "queries with the bare job id so fog cannot mutate caller state" do - compute = FakeCompute.new([job_response(1)]) + it "queries by job id" do + api = FakeApi.new([job_response(1)]) client = described_class.new( { cloudstack_job_poll_interval: 1, cloudstack_job_timeout: 5 }, - compute: compute, sleeper: ->(_s) {} + api: api, sleeper: ->(_s) {} ) client.run_job("job-1") - expect(compute.job_queries).to eq(["job-1"]) + expect(api.job_queries).to eq([{ "jobid" => "job-1" }]) end end - describe "#run_response_job" do - it "takes the job id out of the response envelope and waits for that job" do - compute = FakeCompute.new([job_response(1, { "done" => true })]) - client = described_class.new( - { cloudstack_job_poll_interval: 1, cloudstack_job_timeout: 5 }, - compute: compute, sleeper: ->(_s) {} + describe "#api" do + let(:config) do + { + cloudstack_api_url: "https://cs.example.com:8443/client/api", + cloudstack_api_key: "key", + cloudstack_secret_key: "secret", + } + end + + it "builds a cloudstack_client connection from the CloudStack API url" do + expect(CloudstackClient::Client).to receive(:new).with( + "https://cs.example.com:8443/client/api", + "key", + "secret", + hash_including(quiet: true) ) - result = client.run_response_job( - { "createfirewallruleresponse" => { "jobid" => "job-9" } }, - "createfirewallruleresponse" + described_class.new(config).api + end + + it "verifies the server certificate by default" do + expect(CloudstackClient::Client).to receive(:new).with( + anything, anything, anything, hash_including(ssl_verify: true) ) - expect(result).to eq({ "done" => true }) - expect(compute.job_queries).to eq(["job-9"]) + described_class.new(config).api end - end - describe "#compute" do - it "builds a fog connection from the CloudStack API url" do - client = described_class.new({ - cloudstack_api_url: "https://cs.example.com:8443/client/api", - cloudstack_api_key: "key", - cloudstack_secret_key: "secret", - }) - - expect(Fog::Compute).to receive(:new).with( - hash_including( - provider: :cloudstack, - cloudstack_host: "cs.example.com", - cloudstack_port: 8443, - cloudstack_path: "/client/api", - cloudstack_scheme: "https", - cloudstack_api_key: "key", - cloudstack_secret_access_key: "secret" - ) + it "stops verifying the server certificate when disable_ssl_validation is set" do + expect(CloudstackClient::Client).to receive(:new).with( + anything, anything, anything, hash_including(ssl_verify: false) ) - client.compute + described_class.new(config.merge(disable_ssl_validation: true)).api + end + + it "builds the connection only once" do + expect(CloudstackClient::Client).to receive(:new).once.and_return(double) + + client = described_class.new(config) + client.api + client.api end end end diff --git a/spec/kitchen/driver/cloudstack/networking_spec.rb b/spec/kitchen/driver/cloudstack/networking_spec.rb index ba185ea..56918d4 100644 --- a/spec/kitchen/driver/cloudstack/networking_spec.rb +++ b/spec/kitchen/driver/cloudstack/networking_spec.rb @@ -2,8 +2,12 @@ require "kitchen/driver/cloudstack/networking" RSpec.describe Kitchen::Driver::Cloudstack::Networking do - # Records every API call and replays canned responses per request name. - class RecordingCompute + # Records every API call and replays canned responses per command name. + # + # Asynchronous commands are called with +sync: true+, so each one answers + # with CloudStack's immediate response -- an id and a job id -- rather than + # the finished job result. + class RecordingApi attr_reader :calls def initialize(responses = {}) @@ -16,7 +20,7 @@ def method_missing(name, *args) response = @responses[name] raise response if response.is_a?(Exception) - response || {} + response || { "id" => "#{name}-id", "jobid" => "#{name}-job" } end def respond_to_missing?(_name, _include_private = false) = true @@ -24,40 +28,38 @@ def respond_to_missing?(_name, _include_private = false) = true def call_named(name) = calls.find { |call| call.first == name }&.last end - # Returns the "jobresult" straight from the canned response envelope. + # Answers job results by job id. class FakeClient - attr_reader :compute + attr_reader :api - def initialize(compute, job_results = {}) - @compute = compute + def initialize(api, job_results = {}) + @api = api @job_results = job_results end - def run_response_job(response, key) - @job_results[key] || response.fetch(key, {})["jobresult"] || {} - end + def run_job(jobid) = @job_results.fetch(jobid, {}) end let(:associate_response) do - { "associateipaddressresponse" => { "jobid" => "job-ip", "id" => "ip-uuid" } } + { "jobid" => "job-ip", "id" => "ip-uuid" } end let(:job_results) do { - "associateipaddressresponse" => { "ipaddress" => { "id" => "ip-uuid", "ipaddress" => "203.0.113.9" } }, - "createfirewallruleresponse" => { "firewallrule" => { "id" => "fw-1" } }, + "job-ip" => { "ipaddress" => { "id" => "ip-uuid", "ipaddress" => "203.0.113.9" } }, + "create_firewall_rule-job" => { "firewallrule" => { "id" => "fw-1" } }, } end def networking(config: {}, port: 22, responses: {}, results: job_results) - compute = RecordingCompute.new({ associate_ip_address: associate_response }.merge(responses)) + api = RecordingApi.new({ associate_ip_address: associate_response }.merge(responses)) described_class.new( { cloudstack_zone_id: "zone-1", cloudstack_network_id: "net-1" }.merge(config), - client: FakeClient.new(compute, results), port: port - ).tap { |n| n.instance_variable_set(:@recording_compute, compute) } + client: FakeClient.new(api, results), port: port + ).tap { |n| n.instance_variable_set(:@recording_api, api) } end - def compute_of(net) = net.instance_variable_get(:@recording_compute) + def api_of(net) = net.instance_variable_get(:@recording_api) describe "#associate_public_ip" do it "returns the allocated public IP address" do @@ -77,14 +79,14 @@ def compute_of(net) = net.instance_variable_get(:@recording_compute) net = networking net.associate_public_ip({}) - expect(compute_of(net).call_named(:create_firewall_rule)).to be_nil + expect(api_of(net).call_named(:create_firewall_rule)).to be_nil end it "opens the transport's port when a firewall rule is requested" do net = networking(config: { cloudstack_create_firewall_rule: true }, port: 5985) net.associate_public_ip({}) - rule = compute_of(net).call_named(:create_firewall_rule) + rule = api_of(net).call_named(:create_firewall_rule) expect(rule["startport"]).to eq(5985) expect(rule["endport"]).to eq(5985) end @@ -93,7 +95,7 @@ def compute_of(net) = net.instance_variable_get(:@recording_compute) net = networking(config: { cloudstack_create_firewall_rule: true }) net.associate_public_ip({}) - expect(compute_of(net).call_named(:create_firewall_rule)["cidrlist"]).to eq("0.0.0.0/0") + expect(api_of(net).call_named(:create_firewall_rule)["cidrlist"]).to eq("0.0.0.0/0") end it "restricts the firewall rule to a configured CIDR" do @@ -103,7 +105,7 @@ def compute_of(net) = net.instance_variable_get(:@recording_compute) }) net.associate_public_ip({}) - expect(compute_of(net).call_named(:create_firewall_rule)["cidrlist"]).to eq("198.51.100.0/24") + expect(api_of(net).call_named(:create_firewall_rule)["cidrlist"]).to eq("198.51.100.0/24") end it "records the firewall rule id so teardown can remove it" do @@ -115,12 +117,31 @@ def compute_of(net) = net.instance_variable_get(:@recording_compute) end end + describe "project scoping" do + # fog applied the project to every request from the connection itself. + # cloudstack_client has no equivalent, so each command that accepts a + # project has to be given one. + it "allocates the address inside a configured project" do + net = networking(config: { cloudstack_project_id: "proj-1" }) + net.associate_public_ip({}) + + expect(api_of(net).call_named(:associate_ip_address)["projectid"]).to eq("proj-1") + end + + it "looks for the network inside a configured project" do + net = networking(config: { cloudstack_project_id: "proj-1" }) + net.associate_public_ip({}) + + expect(api_of(net).call_named(:list_networks)["projectid"]).to eq("proj-1") + end + end + describe "#create_port_forward" do it "forwards the transport's port rather than always SSH" do net = networking(port: 5985) net.create_port_forward({ ipaddressid: "ip-uuid" }, "vm-1") - rule = compute_of(net).call_named(:create_port_forwarding_rule) + rule = api_of(net).call_named(:create_port_forwarding_rule) expect(rule["privateport"]).to eq(5985) expect(rule["publicport"]).to eq(5985) end @@ -129,7 +150,26 @@ def compute_of(net) = net.instance_variable_get(:@recording_compute) net = networking(port: 22) net.create_port_forward({ ipaddressid: "ip-uuid" }, "vm-1") - expect(compute_of(net).call_named(:create_port_forwarding_rule)["privateport"]).to eq(22) + expect(api_of(net).call_named(:create_port_forwarding_rule)["privateport"]).to eq(22) + end + + # openfirewall defaults to true on a non-VPC network, so the driver has to + # say false explicitly or CloudStack opens the firewall itself -- creating + # a rule teardown does not know about. cloudstack_client drops arguments + # whose value is falsey, so this has to be the string "false". + it "asks CloudStack not to open the firewall itself" do + net = networking(port: 22) + net.create_port_forward({ ipaddressid: "ip-uuid" }, "vm-1") + + expect(api_of(net).call_named(:create_port_forwarding_rule)["openfirewall"]).to eq("false") + end + + it "records the forwarding rule id before waiting for the job" do + net = networking(port: 22) + state = { ipaddressid: "ip-uuid" } + net.create_port_forward(state, "vm-1") + + expect(state[:forwardingruleid]).to eq("create_port_forwarding_rule-id") end end @@ -138,7 +178,7 @@ def compute_of(net) = net.instance_variable_get(:@recording_compute) net = networking net.teardown(ipaddressid: "ip-uuid", forwardingruleid: "fwd-1") - names = compute_of(net).calls.map(&:first) + names = api_of(net).calls.map(&:first) expect(names.index(:delete_port_forwarding_rule)).to be < names.index(:disassociate_ip_address) end @@ -146,21 +186,37 @@ def compute_of(net) = net.instance_variable_get(:@recording_compute) net = networking net.teardown(ipaddressid: "ip-uuid", firewall_rule_id: "fw-1") - expect(compute_of(net).calls.map(&:first)).to include(:delete_firewall_rule) + expect(api_of(net).calls.map(&:first)).to include(:delete_firewall_rule) + end + + it "deletes rules by id" do + net = networking + net.teardown(ipaddressid: "ip-uuid", forwardingruleid: "fwd-1") + + expect(api_of(net).call_named(:delete_port_forwarding_rule)).to eq({ "id" => "fwd-1" }) + expect(api_of(net).call_named(:disassociate_ip_address)).to eq({ "id" => "ip-uuid" }) end it "ignores resources CloudStack reports as already gone" do - gone = Fog::Cloudstack::Compute::BadRequest.new("Entity does not exist") + gone = CloudstackClient::ApiError.new("Status 431: Entity does not exist.") net = networking(responses: { disassociate_ip_address: gone }) expect { net.teardown(ipaddressid: "ip-uuid") }.not_to raise_error end + it "still raises errors that are not an already-gone resource" do + boom = CloudstackClient::ApiError.new("Status 431: Insufficient permissions.") + net = networking(responses: { disassociate_ip_address: boom }) + + expect { net.teardown(ipaddressid: "ip-uuid") } + .to raise_error(CloudstackClient::ApiError, /Insufficient permissions/) + end + it "does nothing when no public address was ever associated" do net = networking net.teardown({}) - expect(compute_of(net).calls).to be_empty + expect(api_of(net).calls).to be_empty end end end diff --git a/spec/kitchen/driver/cloudstack/server_options_spec.rb b/spec/kitchen/driver/cloudstack/server_options_spec.rb index 2c7f425..4b5255a 100644 --- a/spec/kitchen/driver/cloudstack/server_options_spec.rb +++ b/spec/kitchen/driver/cloudstack/server_options_spec.rb @@ -83,4 +83,44 @@ def options_for(config, instance_name: "default-ubuntu", login: "tsmith", hostna expect(opts["displayname"]).to eq("my-server") end + + it "deploys into a configured project" do + opts = options_for(base_config.merge(cloudstack_project_id: "proj-1")) + + expect(opts["projectid"]).to eq("proj-1") + end + + it "omits the project when none is configured" do + expect(options_for(base_config)).not_to have_key("projectid") + end + + describe "custom service offering sizing" do + # CloudStack takes this as a map. Sent as flat "details[0].cpuNumber" + # parameters they are rejected as unknown, so the shape matters. + it "sends the sizing as a details map" do + opts = options_for(base_config.merge( + cloudstack_serviceoffering_cpu: 2, + cloudstack_serviceoffering_cpuspeed: 2000, + cloudstack_serviceoffering_memory: 4096 + )) + + expect(opts["details"]).to eq([{ "cpuNumber" => 2, "cpuSpeed" => 2000, "memory" => 4096 }]) + end + + it "sends only the sizing values that were configured" do + opts = options_for(base_config.merge(cloudstack_serviceoffering_memory: 4096)) + + expect(opts["details"]).to eq([{ "memory" => 4096 }]) + end + + it "omits the details map when no sizing was configured" do + expect(options_for(base_config)).not_to have_key("details") + end + + it "does not send the sizing as flat parameters" do + opts = options_for(base_config.merge(cloudstack_serviceoffering_cpu: 2)) + + expect(opts.keys.grep(/details\[/)).to be_empty + end + end end diff --git a/spec/kitchen/driver/cloudstack_spec.rb b/spec/kitchen/driver/cloudstack_spec.rb index 217a0c7..fb09782 100644 --- a/spec/kitchen/driver/cloudstack_spec.rb +++ b/spec/kitchen/driver/cloudstack_spec.rb @@ -9,15 +9,18 @@ RSpec.describe Kitchen::Driver::Cloudstack do # Stands in for the CloudStack API, recording what the driver asked for. class DriverFakeClient - attr_reader :compute, :calls + attr_reader :api, :calls def initialize(vm_info:, vm_state: "Running") @vm_info = vm_info @vm_state = vm_state @calls = [] - @compute = Recorder.new(self) + @api = Recorder.new(self) end + # cloudstack_client strips the response envelope, and every asynchronous + # call is made with sync: true, so each answers with CloudStack's + # immediate response rather than the finished job result. class Recorder def initialize(owner) = @owner = owner @@ -25,11 +28,13 @@ def method_missing(name, *args) @owner.calls << [name, args.first] case name when :deploy_virtual_machine - { "deployvirtualmachineresponse" => { "id" => "vm-1", "jobid" => "job-1" } } + { "id" => "vm-1", "jobid" => "job-1" } when :list_virtual_machines - { "listvirtualmachinesresponse" => { "virtualmachine" => [{ "id" => "vm-1", "state" => @owner.vm_state }] } } + [{ "id" => "vm-1", "state" => @owner.vm_state }] + when :associate_ip_address + { "id" => "ip-uuid", "jobid" => "job-ip" } else - {} + { "id" => "#{name}-id", "jobid" => "#{name}-job" } end end @@ -38,17 +43,15 @@ def respond_to_missing?(_n, _p = false) = true attr_reader :vm_state - def run_job(_jobid) = { "virtualmachine" => @vm_info } - - def run_response_job(response, key) - @calls << [:run_response_job, key] - case key - when "associateipaddressresponse" + def run_job(jobid) + @calls << [:run_job, jobid] + case jobid + when "job-ip" { "ipaddress" => { "id" => "ip-uuid", "ipaddress" => "203.0.113.9" } } - when "createfirewallruleresponse" + when "create_firewall_rule-job" { "firewallrule" => { "id" => "fw-1" } } else - {} + { "virtualmachine" => @vm_info } end end @@ -157,7 +160,21 @@ def build_driver(config = {}) it "honours the expunge setting" do build_driver(cloudstack_expunge: true).destroy(server_id: "vm-1") - expect(client.call_named(:destroy_virtual_machine)["expunge"]).to be(true) + expect(client.call_named(:destroy_virtual_machine)["expunge"]).to eq("true") + end + + # cloudstack_client drops any argument whose value is falsey, so a + # boolean false never reaches CloudStack at all. + it "sends a disabled expunge setting as a string so it is not dropped" do + build_driver(cloudstack_expunge: false).destroy(server_id: "vm-1") + + expect(client.call_named(:destroy_virtual_machine)["expunge"]).to eq("false") + end + + it "looks the instance up inside a configured project" do + build_driver(cloudstack_project_id: "proj-1").status(server_id: "vm-1") + + expect(client.call_named(:list_virtual_machines)["projectid"]).to eq("proj-1") end it "clears the instance details from state" do @@ -259,7 +276,7 @@ def doctor_run(config = {}) driver = build_driver( cloudstack_api_key: "key", cloudstack_secret_key: "secret" ) - allow(driver.client.compute).to receive(:list_zones) + allow(driver.client.api).to receive(:list_zones) .and_raise(StandardError.new("401 unauthorized")) messages = [] allow(driver).to receive(:warn) { |m| messages << m }