From 93b7656774f861ce5975b64599f7e28ada835a34 Mon Sep 17 00:00:00 2001
From: Mohammed Naser <mnaser@vexxhost.com>
Date: Wed, 26 Jul 2017 11:50:45 -0400
Subject: [PATCH] Switch nova_security_rule to openstack provider

The current nova provider for nova_security_rule is broken at the
moment.  Due to the fact that the commands are getting deprecated,
the warnings are confusing the text parsing and result in repeated
creation of security group rules.

This patch resolves this issue by switching it to the new openstack
provider.  It also adds the instances and prefetch methods which
will allow `puppet resource` CLI usage.

Change-Id: I9f84728f89a8344685f64b79c8079f3bf74ff979
---
 .../provider/nova_security_rule/nova.rb       |  51 --------
 .../provider/nova_security_rule/openstack.rb  | 122 ++++++++++++++++++
 lib/puppet/type/nova_security_rule.rb         |  38 +++---
 .../provider/nova_security_rule/nova_spec.rb  |  48 -------
 .../nova_security_rule/openstack_spec.rb      |  61 +++++++++
 5 files changed, 200 insertions(+), 120 deletions(-)
 delete mode 100644 lib/puppet/provider/nova_security_rule/nova.rb
 create mode 100644 lib/puppet/provider/nova_security_rule/openstack.rb
 delete mode 100644 spec/unit/provider/nova_security_rule/nova_spec.rb
 create mode 100644 spec/unit/provider/nova_security_rule/openstack_spec.rb

diff --git a/lib/puppet/provider/nova_security_rule/nova.rb b/lib/puppet/provider/nova_security_rule/nova.rb
deleted file mode 100644
index cca71b3b3..000000000
--- a/lib/puppet/provider/nova_security_rule/nova.rb
+++ /dev/null
@@ -1,51 +0,0 @@
-require File.join(File.dirname(__FILE__), '..','..','..',
-                  'puppet/provider/nova')
-
-Puppet::Type.type(:nova_security_rule).provide(
-  :nova,
-  :parent => Puppet::Provider::Nova
-) do
-
-  desc "Manage nova security rules"
-
-  commands :nova => 'nova'
-
-  mk_resource_methods
-
-  def exists?
-    @property_hash[:ensure] == :present
-  end
-
-  def destroy
-    args = Array.new
-
-    args << "#{@resource[:security_group]}"
-    args << "#{@resource[:ip_protocol]}"
-    args << "#{@resource[:from_port]}"
-    args << "#{@resource[:to_port]}"
-    if not @resource[:ip_range].nil?
-      args << "#{@resource[:ip_range]}"
-    else
-      args << "#{@resource[:source_group]}"
-    end
-
-    auth_nova("secgroup-delete-rule", args)
-    @property_hash[:ensure] = :absent
-  end
-
-  def create
-    args = Array.new
-
-    args << "#{@resource[:security_group]}"
-    args << "#{@resource[:ip_protocol]}"
-    args << "#{@resource[:from_port]}"
-    args << "#{@resource[:to_port]}"
-    if not @resource[:ip_range].nil?
-      args << "#{@resource[:ip_range]}"
-    else
-      args << "#{@resource[:source_group]}"
-    end
-
-    result = auth_nova("secgroup-add-rule", args)
-  end
-end
diff --git a/lib/puppet/provider/nova_security_rule/openstack.rb b/lib/puppet/provider/nova_security_rule/openstack.rb
new file mode 100644
index 000000000..70747f7db
--- /dev/null
+++ b/lib/puppet/provider/nova_security_rule/openstack.rb
@@ -0,0 +1,122 @@
+require File.join(File.dirname(__FILE__), '..','..','..', 'puppet/provider/nova')
+
+Puppet::Type.type(:nova_security_rule).provide(
+  :openstack,
+  :parent => Puppet::Provider::Nova
+) do
+  desc <<-EOT
+      Manage nova security rules
+  EOT
+
+  @credentials = Puppet::Provider::Openstack::CredentialsV3.new
+
+  def create
+    opts = [@resource[:security_group]]
+    opts << '--protocol' << @resource[:ip_protocol]
+
+    if @resource[:ip_protocol].to_s == 'icmp'
+      unless @resource[:from_port].to_i == -1 and @resource[:to_port].to_i == -1
+        opts << "--icmp-type" << @resource[:from_port]
+        opts << "--icmp-code" << @resource[:to_port]
+      end
+    else
+      opts << "--dst-port" << "#{@resource[:from_port]}:#{@resource[:to_port]}"
+    end
+
+    unless @resource[:ip_range].nil?
+      opts << "--src-ip" << @resource[:ip_range]
+    else
+      opts << "--src-group" << @resource[:source_group]
+    end
+
+    @property_hash = self.class.nova_request('security group rule', 'create', nil, opts)
+    @property_hash[:ensure] = :present
+  end
+
+  def exists?
+    @property_hash[:ensure] == :present
+  end
+
+  def destroy
+    self.class.request('security group rule', 'delete', @property_hash[:name])
+    @property_hash[:ensure] == :absent
+  end
+
+  mk_resource_methods
+
+  def self.instances
+    rules = []
+    secgroup_provider = Puppet::Type.type(:nova_security_group).provider(:openstack)
+    groups = secgroup_provider.instances
+
+    groups.each do |g|
+      self.nova_request('security group rule', 'list', nil, ['--long', g.id]).each do |attrs|
+        # NOTE(mnaser): Originally, security groups were ingress only so to maintain
+        #               backwards compatibility, we ignore all egress rules.
+        next if attrs[:direction] == 'egress'
+
+        # NOTE(mnaser): With Neutron, an empty ip_range means all networks, therefore
+        #               we replace it by '0.0.0.0/0' for backwards compatibility.
+        attrs[:ip_range] = '0.0.0.0/0' if attrs[:ip_range].empty? and attrs[:remote_security_group].empty?
+
+        # NOTE(mnaser): Another quirk, Neutron can have an empty port range which means
+        #               all ports, we adjust the field accordingly for the protocol.
+        if attrs[:port_range].empty?
+          if ['tcp', 'udp'].include? attrs[:ip_protocol]
+            attrs[:from_port] = 0
+            attrs[:to_port] = 65536
+          else
+            attrs[:from_port] = -1
+            attrs[:to_port] = -1
+          end
+        else
+          attrs[:from_port], attrs[:to_port] = attrs[:port_range].split(':')
+        end
+
+        rule = {
+          :ensure         => :present,
+          :name           => attrs[:id],
+          :security_group => g.name,
+          :from_port      => attrs[:from_port],
+          :to_port        => attrs[:to_port],
+        }
+
+        # NOTE(mnaser): The puppet type does not like getting source_group even if it's not set.
+        unless attrs[:ip_range].empty?
+          rule[:ip_range] = attrs[:ip_range]
+        else
+          rule[:source_group] = attrs[:remote_security_group]
+        end
+
+        # NOTE(mnaser): With Neutron, it is possible to have the ip_protocol empty
+        #               which means all 3 protocols are allowed.  We create three
+        #               resources to maintain backwards compatible.
+        if attrs[:ip_protocol].empty?
+          rules << new(rule.merge(:ip_protocol => 'tcp', :from_port => 0, :to_port => 65536))
+          rules << new(rule.merge(:ip_protocol => 'udp', :from_port => 0, :to_port => 65536))
+          rules << new(rule.merge(:ip_protocol => 'icmp', :from_port => -1, :to_port => -1))
+        else
+          rules << new(rule.merge(:ip_protocol => attrs[:ip_protocol]))
+        end
+      end
+    end
+
+    rules
+  end
+
+  def self.prefetch(resources)
+    security_group_rules = instances
+    resources.keys.each do |name|
+      resource = resources[name].to_hash
+
+      rule = security_group_rules.find do |r|
+        r.security_group == resource[:security_group] && \
+        r.ip_protocol.to_s == resource[:ip_protocol].to_s && \
+        r.from_port.to_s == resource[:from_port].to_s && \
+        r.to_port.to_s == resource[:to_port].to_s
+      end
+
+      resources[name].provider = rule if rule
+    end
+  end
+end
\ No newline at end of file
diff --git a/lib/puppet/type/nova_security_rule.rb b/lib/puppet/type/nova_security_rule.rb
index 540f919aa..3f57df4ed 100644
--- a/lib/puppet/type/nova_security_rule.rb
+++ b/lib/puppet/type/nova_security_rule.rb
@@ -58,29 +58,22 @@ Puppet::Type.newtype(:nova_security_rule) do
   end
 
   newparam(:ip_protocol) do
-    defaultto do
-      raise Puppet::Error, 'You should give protocol!'
-    end
     newvalues 'tcp', 'udp', 'icmp'
   end
 
   newparam(:from_port) do
-    defaultto do
-      raise Puppet::Error, 'You should give the source port!'
-    end
+    newvalues(/\d+/)
     validate do |value|
-      if value !~ /\d+/ or value.to_i <= -1 or value.to_i >= 65536
+      if value.to_i < -1 or value.to_i >= 65536
         raise Puppet::Error, 'Incorrect from port!'
       end
     end
   end
 
   newparam(:to_port) do
-    defaultto do
-      raise Puppet::Error, 'You should give the destination port!'
-    end
+    newvalues(/\d+/)
     validate do |value|
-      if value !~ /\d+/ or value.to_i <= -1 or value.to_i >= 65536
+      if value.to_i < -1 or value.to_i >= 65536
         raise Puppet::Error, 'Incorrect to port!'
       end
     end
@@ -116,23 +109,26 @@ Puppet::Type.newtype(:nova_security_rule) do
     end
   end
 
-  newparam(:source_group) do
-  end
-
-  newparam(:security_group) do
-    defaultto do
-      raise Puppet::Error, 'You should provide the security group to add this rule to!'
-    end
-  end
+  newparam(:source_group)
+  newparam(:security_group)
 
   validate do
-    unless !!self[:ip_range] ^ !!self[:source_group]
+    unless self[:from_port]
+      raise Puppet::Error, 'You should give the source port!'
+    end
+    unless self[:to_port]
+      raise Puppet::Error, 'You should give the destination port!'
+    end
+    unless self[:security_group]
+      raise Puppet::Error, 'You should provide the security group to add this rule to!'
+    end
+    unless self[:ip_range].to_s.empty? ^ self[:source_group].to_s.empty?
       raise Puppet::Error, 'You should give either ip_range or source_group. Not none or both!'
     end
     unless self[:from_port].to_i <= self[:to_port].to_i
       raise Puppet::Error, 'From_port should be lesser or equal to to_port!'
     end
-    if self[:ip_protocol] != 'icmp' and (self[:from_port].to_i <= 0 || self[:to_port].to_i <= 0)
+    if self[:ip_protocol].to_s != 'icmp' and (self[:from_port].to_i <= 0 || self[:to_port].to_i <= 0)
       raise Puppet::Error, 'From_port and To_port should not be less than 0 unless IP protocol is ICMP'
     end
   end
diff --git a/spec/unit/provider/nova_security_rule/nova_spec.rb b/spec/unit/provider/nova_security_rule/nova_spec.rb
deleted file mode 100644
index 253094476..000000000
--- a/spec/unit/provider/nova_security_rule/nova_spec.rb
+++ /dev/null
@@ -1,48 +0,0 @@
-require 'puppet'
-require 'puppet/provider/nova_security_rule/nova'
-require 'tempfile'
-
-provider_class = Puppet::Type.type(:nova_security_rule).provider(:nova)
-
-describe provider_class do
-
-  let :secrule_attrs do
-    {
-      :name => "scr0",
-      :ip_protocol => "tcp",
-      :from_port => '22',
-      :to_port => '23',
-      :ip_range => '0.0.0.0/0',
-      :security_group => 'scg0'
-    }
-  end
-
-  let :resource do
-    Puppet::Type::Nova_security_rule.new(secrule_attrs)
-  end
-
-  let :provider do
-    provider_class.new(resource)
-  end
-
-  shared_examples "nova_security_rule" do
-    describe "#create" do
-      it 'should create security rule' do
-        provider.expects(:auth_nova).with('secgroup-add-rule', ['scg0', 'tcp', '22', '23', '0.0.0.0/0']).returns('+-------------+-----------+---------+-----------+--------------+\n| IP Protocol | From Port | To Port | IP Range  | Source Group |\n+-------------+-----------+---------+-----------+--------------+\n| tcp         | 22        | 23      | 0.0.0.0/0 |              |\n+-------------+-----------+---------+-----------+--------------+')
-
-        provider.create
-      end 
-    end
-
-
-    describe "#destroy" do
-      it 'should destroy security rule' do
-        provider.expects(:auth_nova).with('secgroup-delete-rule', ['scg0', 'tcp', '22', '23', '0.0.0.0/0']).returns('+-------------+-----------+---------+-----------+--------------+\n| IP Protocol | From Port | To Port | IP Range  | Source Group |\n+-------------+-----------+---------+-----------+--------------+\n| tcp         | 22        | 23      | 0.0.0.0/0 |              |\n+-------------+-----------+---------+-----------+--------------+')
-
-        provider.destroy
-      end 
-    end
-  end
-
-  it_behaves_like('nova_security_rule')
-end
diff --git a/spec/unit/provider/nova_security_rule/openstack_spec.rb b/spec/unit/provider/nova_security_rule/openstack_spec.rb
new file mode 100644
index 000000000..b6f229dc3
--- /dev/null
+++ b/spec/unit/provider/nova_security_rule/openstack_spec.rb
@@ -0,0 +1,61 @@
+require 'puppet'
+require 'spec_helper'
+require 'puppet/provider/nova_security_rule/openstack'
+
+provider_class = Puppet::Type.type(:nova_security_rule).provider(:openstack)
+
+describe provider_class do
+
+  shared_examples 'authenticated with environment variables' do
+    ENV['OS_USERNAME']     = 'test'
+    ENV['OS_PASSWORD']     = 'abc123'
+    ENV['OS_PROJECT_NAME'] = 'test'
+    ENV['OS_AUTH_URL']     = 'http://127.0.0.1:35357/v3'
+  end
+
+  describe 'managing security group rules' do
+    let :secrule_attrs do
+      {
+        :name => "scr0",
+        :ip_protocol => "tcp",
+        :from_port => '22',
+        :to_port => '23',
+        :ip_range => '0.0.0.0/0',
+        :security_group => 'scg0'
+      }
+    end
+
+    let :resource do
+      Puppet::Type::Nova_security_rule.new(secrule_attrs)
+    end
+
+    let :provider do
+      provider_class.new(resource)
+    end
+
+    it_behaves_like 'authenticated with environment variables' do
+      describe "#create" do
+        it 'should create security group rule' do
+          provider.class.stubs(:openstack)
+                        .with('security group rule', 'create', ['scg0', '--protocol', 'tcp', '--dst-port', '22:23', '--src-ip', '0.0.0.0/0'])
+                        .returns('id="021114fb-67e0-4882-b2ed-e7c5328d8aa8"
+  protocol="tcp"
+  port_range_max="22"
+  port_range_min="23"
+  remote_ip_prefix="0.0.0.0/0"
+  security_group_id="4812fe3c-69d4-4b27-992b-163a20dc82d1"')
+        end
+      end
+
+      describe '#destroy' do
+        it 'removes security group rule' do
+          provider_class.expects(:openstack)
+            .with('security group rule', 'delete', 'scr0')
+          provider.instance_variable_set(:@property_hash, secrule_attrs)
+          provider.destroy
+          expect(provider.exists?).to be_falsey
+        end
+      end
+    end
+  end
+end