[Ovirt-devel] [PATCH server] missing ovirt wui validations
Mohammed Morsi
mmorsi at redhat.com
Tue Dec 9 16:00:10 UTC 2008
this patch is now syntactically correct and will run when invoking
db:migrate or when running the tests. several tests now fail though
and need to be debugged / possibly changed
Attached is additional validations, added to the rails model
and controller layers verifying:
bondings: arp related fields
boot_types: label uniqueness and proto inclusion
cpus: various fields are set and min values
help_sections: presence and uniqueness of fields
hosts: presence and format of various fields
ip_addresses: fk existance / integrity
nics: mac address format
permissions: presence / inclusion of various fields
pools: fields requied for tree present
quotas: minimum values
smart_pool_tags: tagged_id / type fk present
storage_pools: type, state field inclusion, min values
storage_volumes: various fields, subclasses w/ various fields
tasks: various fields present and consistensy maintained
usages: label, other fields present, unique
vms: uuid of correct format, min values, various fields present
can only add / move host and storage volumes and pools to / from pools
when you have sufficient permissions and target entities don't have
associated vms
can only destroy storage volumes / pools if there are no attached vms
---
src/app/controllers/hardware_controller.rb | 28 +++++++++++++++++++++-
src/app/controllers/storage_controller.rb | 7 +++++
src/app/models/bonding.rb | 11 +++++++++
src/app/models/boot_type.rb | 5 ++++
src/app/models/cpu.rb | 23 +++++++++++++++++++
src/app/models/help_section.rb | 34 ++++++++++++++++++++++++++++
src/app/models/host.rb | 21 +++++++++++++++++
src/app/models/ip_address.rb | 4 +++
src/app/models/iscsi_storage_volume.rb | 2 +
src/app/models/lvm_storage_volume.rb | 6 +++++
src/app/models/nfs_storage_pool.rb | 3 ++
src/app/models/nic.rb | 11 +++++++++
src/app/models/permission.rb | 7 +++++
src/app/models/pool.rb | 7 +++++
src/app/models/quota.rb | 22 ++++++++++++++++++
src/app/models/smart_pool_tag.rb | 5 ++++
src/app/models/storage_pool.rb | 17 ++++++++++++++
src/app/models/storage_volume.rb | 21 +++++++++++++++++
src/app/models/task.rb | 15 ++++++++++++
src/app/models/usage.rb | 6 +++++
src/app/models/vm.rb | 34 ++++++++++++++++++++++++++-
21 files changed, 285 insertions(+), 4 deletions(-)
diff --git a/src/app/controllers/hardware_controller.rb b/src/app/controllers/hardware_controller.rb
index 4dda736..cf45c49 100644
--- a/src/app/controllers/hardware_controller.rb
+++ b/src/app/controllers/hardware_controller.rb
@@ -303,15 +303,39 @@ class HardwareController < PoolController
resource_ids_str = params[:resource_ids]
resource_ids = resource_ids_str.split(",").collect {|x| x.to_i}
+ # if user doesn't have modify permission on both source and destination
+ unless @pool.can_modify(@user) and Pool.find(params[:target_pool_id]).can_modify(@user)
+ render :json => { :success => false,
+ :alert => "Cannot #{item_action.to_s} #{item_class.table_name.humanize} without admin permissions on both pools" }
+ return
+ end
+
+ # relay error message if movable check fails for any resource
+ success = true
+ failed_resources = ""
+ resource_ids.each {|x|
+ unless item_class.find(x).movable?
+ success = false
+ failed_resources += x.to_s + " "
+ end
+ }
+ resource_ids.delete_if { |x| ! item_class.find(x).movable? }
+
begin
@pool.transaction do
@pool.send(item_method, resource_ids, target_pool_id)
end
+ rescue
+ success = false
+ end
+
+ if success
render :json => { :success => true,
:alert => "#{item_action.to_s} #{item_class.table_name.humanize} successful." }
- rescue
+ else
render :json => { :success => false,
- :alert => "#{item_action.to_s} #{item_class.table_name.humanize} failed." }
+ :alert => "#{item_action.to_s} #{item_class.table_name.humanize} failed" +
+ (failed_resources == "" ? "." : " for " + failed_resources) }
end
end
diff --git a/src/app/controllers/storage_controller.rb b/src/app/controllers/storage_controller.rb
index 148d1be..47e9e14 100644
--- a/src/app/controllers/storage_controller.rb
+++ b/src/app/controllers/storage_controller.rb
@@ -310,6 +310,13 @@ class StorageController < ApplicationController
end
def destroy
+ unless @storage_pool.movable?
+ format.json { render :json => { :object => "storage_pool",
+ :success => false,
+ :alert => "Cannot delete storage with associated vms" } }
+ return
+ end
+
pool = @storage_pool.hardware_pool
if @storage_pool.destroy
alert="Storage Pool was successfully deleted."
diff --git a/src/app/models/bonding.rb b/src/app/models/bonding.rb
index c9af38c..fabdb67 100644
--- a/src/app/models/bonding.rb
+++ b/src/app/models/bonding.rb
@@ -30,6 +30,8 @@
# interface. They can be ignored if not used.
#
class Bonding < ActiveRecord::Base
+
+
belongs_to :host
belongs_to :bonding_type
belongs_to :vlan
@@ -58,6 +60,15 @@ class Bonding < ActiveRecord::Base
validates_presence_of :vlan_id,
:message => 'A vlan must be specified.'
+ # verify arp ping address to be ipv4 if set
+ validates_format_of :arp_ping_address,
+ :with => %r{^(\d{1,3}\.){3}\d{1,3}$},
+ :unless => Proc.new { |bonding| bonding.arp_ping_address.nil? }
+
+ validates_numericality_of :arp_interval,
+ :greater_than_or_equal_to => 0,
+ :unless => Proc.new { |bonding| bonding.arp_interval.nil? }
+
protected
def validate
if vlan.boot_type.proto == 'static' and ip_addresses.size == 0
diff --git a/src/app/models/boot_type.rb b/src/app/models/boot_type.rb
index 8ffbe67..6cfdb04 100644
--- a/src/app/models/boot_type.rb
+++ b/src/app/models/boot_type.rb
@@ -18,4 +18,9 @@
# also available at http://www.gnu.org/copyleft/gpl.html.
class BootType < ActiveRecord::Base
+ validates_presence_of :label
+ validates_uniqueness_of :label,
+ :message => 'Label must be unique'
+ validates_inclusion_of :proto,
+ :in => %w( static dhcp bootp )
end
diff --git a/src/app/models/cpu.rb b/src/app/models/cpu.rb
index 1a7d3cf..714a8ed 100644
--- a/src/app/models/cpu.rb
+++ b/src/app/models/cpu.rb
@@ -21,4 +21,27 @@
#
class Cpu < ActiveRecord::Base
belongs_to :host
+
+ validates_presence_of :host_id,
+ :message => 'A host must be specified.'
+
+ validates_numericality_of :cpu_number,
+ :greater_than_or_equal_to => 0
+
+ validates_numericality_of :core_number,
+ :greater_than_or_equal_to => 0
+
+ validates_numericality_of :number_of_cores,
+ :greater_than_or_equal_to => 1
+
+ validates_numericality_of :cpuid_level,
+ :greater_than_or_equal_to => 0
+
+ validates_numericality_of :speed,
+ :greater_than => 0
+ # also verify speed in MHz ?
+
+ validates_presence_of :vendor
+ validates_presence_of :model
+ validates_presence_of :family
end
diff --git a/src/app/models/help_section.rb b/src/app/models/help_section.rb
index a891383..1de1e5a 100644
--- a/src/app/models/help_section.rb
+++ b/src/app/models/help_section.rb
@@ -1,2 +1,36 @@
+# Copyright (C) 2008 Red Hat, Inc.
+# Written by Mohammed Morsi <mmorsi at redhat.com>
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; version 2 of the License.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston,
+# MA 02110-1301, USA. A copy of the GNU General Public License is
+# also available at http://www.gnu.org/copyleft/gpl.html.
+#
+# +HelpSection+ defines a section of the web help document to be
+# available for a specific controller / action
+#
class HelpSection < ActiveRecord::Base
+
+ validates_uniqueness_of :action,
+ :scope => :controller
+ :message => 'Controller / Action must be unique'
+
+ validates_presence_of :controller,
+ :message => 'A controller must be specified.'
+
+ validates_presence_of :action,
+ :message => 'An action must be specified.'
+
+ validates_presence_of :section,
+ :message => 'A section must be specified.'
end
diff --git a/src/app/models/host.rb b/src/app/models/host.rb
index 640782d..ef495ff 100644
--- a/src/app/models/host.rb
+++ b/src/app/models/host.rb
@@ -51,6 +51,17 @@ class Host < ActiveRecord::Base
[ :search_users, 'U', "search_users" ] ],
:eager_load => :smart_pools
+ validates_presence_of :hardware_pool_id,
+ :message => 'A hardware pool id must be specified.'
+
+ validates_presence_of :uuid,
+ :message => 'A uuid must be specified.'
+
+ validates_presence_of :hostname,
+ :message => 'A hostname must be specified.'
+
+ validates_presence_of :arch,
+ :message => 'An arch must be specified.'
KVM_HYPERVISOR_TYPE = "KVM"
HYPERVISOR_TYPES = [KVM_HYPERVISOR_TYPE]
@@ -58,6 +69,12 @@ class Host < ActiveRecord::Base
STATE_AVAILABLE = "available"
STATES = [STATE_UNAVAILABLE, STATE_AVAILABLE]
+ validates_inclusion_of :hypervisor_type,
+ :in => HYPERVISOR_TYPES
+
+ validates_inclusion_of :state,
+ :in => STATES + Task::COMPLETED_STATES + TASK::WORKING_STATES
+
def memory_in_mb
kb_to_mb(memory)
end
@@ -99,4 +116,8 @@ class Host < ActiveRecord::Base
hardware_pool.search_users
end
+ def movable?
+ return vms.size == 0
+ end
+
end
diff --git a/src/app/models/ip_address.rb b/src/app/models/ip_address.rb
index 5d2e6af..3f246b1 100644
--- a/src/app/models/ip_address.rb
+++ b/src/app/models/ip_address.rb
@@ -24,4 +24,8 @@ class IpAddress < ActiveRecord::Base
belongs_to :network
belongs_to :nic
belongs_to :bonding
+
+ def validate
+ errors.add("id", "ip must be associated with foreign entity") if network_id.nil? and nic_id.nil? and bonding_id.nil?
+ end
end
diff --git a/src/app/models/iscsi_storage_volume.rb b/src/app/models/iscsi_storage_volume.rb
index 48edbd8..fe2cbf5 100644
--- a/src/app/models/iscsi_storage_volume.rb
+++ b/src/app/models/iscsi_storage_volume.rb
@@ -31,4 +31,6 @@ class IscsiStorageVolume < StorageVolume
return true
end
+ validates_presence_of :lun
+
end
diff --git a/src/app/models/lvm_storage_volume.rb b/src/app/models/lvm_storage_volume.rb
index 8eb1f0e..38949ce 100644
--- a/src/app/models/lvm_storage_volume.rb
+++ b/src/app/models/lvm_storage_volume.rb
@@ -29,4 +29,10 @@ class LvmStorageVolume < StorageVolume
def volume_create_params
return lv_name, size, lv_owner_perms, lv_group_perms, lv_mode_perms
end
+
+ validates_presence_of :lv_name
+ validates_presence_of :lv_owner_perms
+ validates_presence_of :lv_group_perms
+ validates_presence_of :lv_mode_perms
+
end
diff --git a/src/app/models/nfs_storage_pool.rb b/src/app/models/nfs_storage_pool.rb
index 398e3f9..da0d4b4 100644
--- a/src/app/models/nfs_storage_pool.rb
+++ b/src/app/models/nfs_storage_pool.rb
@@ -29,4 +29,7 @@ class NfsStoragePool < StoragePool
def user_subdividable
true
end
+
+ validates_presence_of :filename
+
end
diff --git a/src/app/models/nic.rb b/src/app/models/nic.rb
index 5649763..aa9c740 100644
--- a/src/app/models/nic.rb
+++ b/src/app/models/nic.rb
@@ -24,12 +24,23 @@ class Nic < ActiveRecord::Base
has_and_belongs_to_many :bondings, :join_table => 'bondings_nics'
+ validates_presence_of :mac,
+ :message => 'A mac must be specified.'
+
+ validates_format_of :mac,
+ :with => %r{^([0-9a-f]{2}([:-]|$)){6}$}
+
validates_presence_of :host_id,
:message => 'A host must be specified.'
validates_presence_of :physical_network_id,
:message => 'A network must be specified.'
+ validates_numericality_of :bandwidth,
+ :greater_than_or_equal_to => 0
+
+ # validate 'bridge' or 'usage_type' attribute ?
+
protected
def validate
if physical_network.boot_type.proto == 'static' and ip_addresses.size == 0
diff --git a/src/app/models/permission.rb b/src/app/models/permission.rb
index ece3da5..b3929ad 100644
--- a/src/app/models/permission.rb
+++ b/src/app/models/permission.rb
@@ -24,9 +24,12 @@ class Permission < ActiveRecord::Base
has_many :child_permissions, :dependent => :destroy,
:class_name => "Permission", :foreign_key => "inherited_from_id"
+ validates_presence_of :pool_id
+ validates_presence_of :uid
validates_uniqueness_of :uid, :scope => [:pool_id, :inherited_from_id]
+
ROLE_SUPER_ADMIN = "Super Admin"
ROLE_ADMIN = "Administrator"
ROLE_USER = "User"
@@ -44,6 +47,10 @@ class Permission < ActiveRecord::Base
ROLE_USER => [PRIV_VIEW, PRIV_VM_CONTROL],
ROLE_MONITOR => [PRIV_VIEW]}
+
+ validates_inclusion_of :user_role,
+ :in => ROLES.keys
+
def self.invert_roles
return_hash = {}
ROLES.each do |role, privs|
diff --git a/src/app/models/pool.rb b/src/app/models/pool.rb
index 7034e79..5cfc947 100644
--- a/src/app/models/pool.rb
+++ b/src/app/models/pool.rb
@@ -52,6 +52,13 @@ class Pool < ActiveRecord::Base
validates_presence_of :name
validates_uniqueness_of :name, :scope => :parent_id
+ validates_presence_of :parent_id
+ validates_presence_of :lft
+ validates_presence_of :rgt
+
+ validates_inclusion_of :type,
+ :in => %w( DirectoryPool HardwarePool VmResourcePool SmartPool )
+
# overloading this method such that we can use permissions.admins to get all the admins for an object
has_many :permissions, :dependent => :destroy, :order => "id ASC" do
def super_admins
diff --git a/src/app/models/quota.rb b/src/app/models/quota.rb
index fc52177..2a9e0f0 100644
--- a/src/app/models/quota.rb
+++ b/src/app/models/quota.rb
@@ -22,6 +22,28 @@ require 'util/ovirt'
class Quota < ActiveRecord::Base
belongs_to :pool
+ validates_presence_of :pool_id
+
+
+ validates_numericality_of :total_vcpus,
+ :greater_than_or_equal_to => 0,
+ :unless => Proc.new{ |quota| quota.total_vcpus.nil? }
+
+ validates_numericality_of :total_vmemory,
+ :greater_than_or_equal_to => 0,
+ :unless => Proc.new{ |quota| quota.total_vmemory.nil? }
+
+ validates_numericality_of :total_vnics,
+ :greater_than_or_equal_to => 0,
+ :unless => Proc.new{ |quota| quota.total_vnics.nil? }
+
+ validates_numericality_of :total_storage,
+ :greater_than_or_equal_to => 0,
+ :unless => Proc.new{ |quota| quota.total_storage.nil? }
+
+ validates_numericality_of :total_vms,
+ :greater_than_or_equal_to => 0,
+ :unless => Proc.new{ |quota| quota.total_vms.nil? }
def total_vmemory_in_mb
kb_to_mb(total_vmemory)
diff --git a/src/app/models/smart_pool_tag.rb b/src/app/models/smart_pool_tag.rb
index e135ddf..00bb5a7 100644
--- a/src/app/models/smart_pool_tag.rb
+++ b/src/app/models/smart_pool_tag.rb
@@ -31,6 +31,11 @@ class SmartPoolTag < ActiveRecord::Base
validates_uniqueness_of :smart_pool_id, :scope => [:tagged_id, :tagged_type]
+ validates_presence_of :tagged_id
+
+ validates_inclusion_of :tagged_type,
+ :in => %w( Pool StoragePool Host Vm )
+
def tagged_type=(sType)
super(sType.to_s.classify.constantize.base_class.to_s)
end
diff --git a/src/app/models/storage_pool.rb b/src/app/models/storage_pool.rb
index bab7031..f9abb55 100644
--- a/src/app/models/storage_pool.rb
+++ b/src/app/models/storage_pool.rb
@@ -39,6 +39,13 @@ class StoragePool < ActiveRecord::Base
validates_presence_of :hardware_pool_id
+ validates_inclusion_of :type,
+ :in => %w( IscsiStoragePool LvmStoragePool NfsStoragePool )
+
+
+ validates_numericality_of :capacity,
+ :greater_than_or_equal_to => 0
+
acts_as_xapian :texts => [ :ip_addr, :target, :export_path, :type ],
:terms => [ [ :search_users, 'U', "search_users" ] ],
:eager_load => :smart_pools
@@ -54,6 +61,9 @@ class StoragePool < ActiveRecord::Base
STATE_PENDING_DELETION = "pending_deletion"
STATE_AVAILABLE = "available"
+ validates_inclusion_of :state,
+ :in => [ STATE_PENDING_SETUP, STATE_PENDING_DELETION, STATE_AVAILABLE]
+
def self.factory(type, params = {})
params[:state] = STATE_PENDING_SETUP unless params[:state]
case type
@@ -121,4 +131,11 @@ class StoragePool < ActiveRecord::Base
end
return_hash
end
+
+ def movable?
+ storage_volumes.each{ |x|
+ return false unless x.movable?
+ }
+ return true
+ end
end
diff --git a/src/app/models/storage_volume.rb b/src/app/models/storage_volume.rb
index 39b72d5..385c126 100644
--- a/src/app/models/storage_volume.rb
+++ b/src/app/models/storage_volume.rb
@@ -32,10 +32,23 @@ class StorageVolume < ActiveRecord::Base
end
end
+ validates_presence_of :storage_pool_id
+
+
+ validates_numericality_of :size,
+ :greater_than_or_equal_to => 0,
+ :unless => Proc.new { |storage_volume| storage_volume.nil? }
+
+ validates_inclusion_of :type,
+ :in => %w( IscsiStorageVolume LvmStorageVolume NfsStorageVolume )
+
STATE_PENDING_SETUP = "pending_setup"
STATE_PENDING_DELETION = "pending_deletion"
STATE_AVAILABLE = "available"
+ validates_inclusion_of :state,
+ :in => [ STATE_PENDING_SETUP, STATE_PENDING_DELETION, STATE_AVAILABLE]
+
def self.factory(type, params = {})
params[:state] = STATE_PENDING_SETUP unless params[:state]
case type
@@ -131,4 +144,12 @@ class StorageVolume < ActiveRecord::Base
return_hash
end
+ def movable?
+ if vms.size > 0 or
+ (not lvm_storage_pool.nil? and not lvm_stoage_pool.movable?)
+ return false
+ end
+ return true
+ end
+
end
diff --git a/src/app/models/task.rb b/src/app/models/task.rb
index f231c18..e6b9079 100644
--- a/src/app/models/task.rb
+++ b/src/app/models/task.rb
@@ -45,6 +45,16 @@ class Task < ActiveRecord::Base
COMPLETED_STATES = [STATE_FINISHED, STATE_FAILED, STATE_CANCELED]
WORKING_STATES = [STATE_QUEUED, STATE_RUNNING, STATE_PAUSED]
+ validates_inclusion_of :type,
+ :in => %w( HostTask StorageTask StorageVolumeTask VmTask )
+
+ validates_inclusion_of :state,
+ :in => COMPLETED_STATES + WORKING_STATES
+
+ # FIXME validate action depending on type / subclass
+ # validate task_target_id, task_type_id, arg, message
+ # depending on subclass, action, state
+
def cancel
self[:state] = STATE_CANCELED
save!
@@ -71,4 +81,9 @@ class Task < ActiveRecord::Base
""
end
+ def validate
+ errors.add("time_ended", "Tasks ends before its started") unless time_ended > time_started
+ errors.add("time_started", "Tasks starts before its created") unless time_started > time_created
+ end
+
end
diff --git a/src/app/models/usage.rb b/src/app/models/usage.rb
index 353e8f4..59b0e48 100644
--- a/src/app/models/usage.rb
+++ b/src/app/models/usage.rb
@@ -18,4 +18,10 @@
class Usage < ActiveRecord::Base
has_and_belongs_to_many :networks, :join_table => 'networks_usages'
+
+ validates_presence_of :label
+ validates_presence_of :usage
+
+ validates_uniqueness_of :usage
+
end
diff --git a/src/app/models/vm.rb b/src/app/models/vm.rb
index 227f343..4d66c63 100644
--- a/src/app/models/vm.rb
+++ b/src/app/models/vm.rb
@@ -34,7 +34,32 @@ class Vm < ActiveRecord::Base
validates_presence_of :uuid, :description, :num_vcpus_allocated,
:boot_device, :memory_allocated_in_mb,
- :memory_allocated, :vnic_mac_addr
+ :memory_allocated, :vnic_mac_addr,
+ :vm_resource_pool_id
+
+ validates_format_of :uuid,
+ :with => %r([a-f0-9]{8}-[a-f0-9]{4}-[a-f0-9]{4}-[a-f0-9]{4}-[a-f0-9]{12})
+
+ validates_numericality_of :needs_restart,
+ :greater_than_or_equal_to => 0,
+ :less_than_or_equal_to => 1,
+ :unless => Proc.new{ |vm| vm.needs_restart.nil? }
+
+ validates_numericality_of :memory_used,
+ :greater_than_or_equal_to => 0,
+ :unless => Proc.new{ |vm| vm.memory_used.nil? }
+
+ validates_numericality_of :vnc_port,
+ :greater_than_or_equal_to => 0,
+ :unless => Proc.new{ |vm| vm.vnc_port.nil? }
+
+ validates_numericality_of :num_vcpus_allocated,
+ :greater_than_or_equal_to => 0,
+ :unless => Proc.new{ |vm| vm.num_vcpus_allocated.nil? }
+
+ validates_numericality_of :memory_allocated,
+ :greater_than_or_equal_to => 0,
+ :unless => Proc.new{ |vm| vm.memory_allocated.nil? }
acts_as_xapian :texts => [ :uuid, :description, :vnic_mac_addr, :state ],
:terms => [ [ :search_users, 'U', "search_users" ] ],
@@ -117,9 +142,14 @@ class Vm < ActiveRecord::Base
STATE_SAVED => STATE_SAVED,
STATE_RESTORING => STATE_RUNNING,
STATE_MIGRATING => STATE_RUNNING,
- STATE_CREATE_FAILED => STATE_CREATE_FAILED}
+ STATE_CREATE_FAILED => STATE_CREATE_FAILED,
+ STATE_INVALID => STATE_INVALID}
TASK_STATE_TRANSITIONS = []
+ validates_inclusion_of :state,
+ :in => EFFECTIVE_STATE.keys
+
+
def get_hardware_pool
pool = vm_resource_pool
pool = pool.get_hardware_pool if pool
--
1.6.0.4
More information about the ovirt-devel
mailing list