[Ovirt-devel] [PATCH server] missing ovirt wui validations (round 1)
Mohammed Morsi
mmorsi at redhat.com
Mon Dec 8 21:05:22 UTC 2008
note this is a review only patch, as I haven't tested it yet
(or even run it) so there will most likely be syntax and
other errors
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
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 move host and storage 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 | 36 ++++++++++++++++++++++++++++
src/app/controllers/storage_controller.rb | 8 ++++++
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/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 | 9 +++++++
src/app/models/storage_volume.rb | 15 +++++++++++
src/app/models/task.rb | 16 ++++++++++++
src/app/models/usage.rb | 6 ++++
src/app/models/vm.rb | 30 ++++++++++++++++++++++-
18 files changed, 269 insertions(+), 1 deletions(-)
diff --git a/src/app/controllers/hardware_controller.rb b/src/app/controllers/hardware_controller.rb
index 4dda736..a329f90 100644
--- a/src/app/controllers/hardware_controller.rb
+++ b/src/app/controllers/hardware_controller.rb
@@ -285,6 +285,23 @@ class HardwareController < PoolController
end
def move_hosts
+ # if vm's present on host, fail
+ resource_ids_str = params[:resource_ids]
+ resource_ids_str.split(",").each {|x|
+ if Host.find(x).vms > 0
+ render :json => { :success => false,
+ :alert => "Cannot move hosts currently associated w/ vms" }
+ return
+ end
+ }
+
+ # 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 move hosts without admin permissions on both pools" }
+ return
+ end
+
edit_items(Host, :move_hosts, params[:target_pool_id], :move)
end
@@ -293,6 +310,25 @@ class HardwareController < PoolController
end
def move_storage
+ # if vm's present on host, fail
+ resource_ids_str = params[:resource_ids]
+ resource_ids_str.split(",").each {|x|
+ StoragePool.find(x).volumes.each { |v|
+ if v.vms.size > 0
+ render :json => { :success => false,
+ :alert => "Cannot move storage currently associated w/ vms" }
+ return
+ end
+ }
+ }
+
+ # 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 move storage without admin permissions on both pools" }
+ return
+ end
+
edit_items(StoragePool, :move_storage, params[:target_pool_id], :move)
end
diff --git a/src/app/controllers/storage_controller.rb b/src/app/controllers/storage_controller.rb
index 148d1be..804ba75 100644
--- a/src/app/controllers/storage_controller.rb
+++ b/src/app/controllers/storage_controller.rb
@@ -310,6 +310,14 @@ class StorageController < ApplicationController
end
def destroy
+ @storage_pool.volumes.each { |volume|
+ if volume.vms.size > 0
+ 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..1f30e8a 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..5328500 100644
--- a/src/app/models/host.rb
+++ b/src/app/models/host.rb
@@ -51,6 +51,27 @@ 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_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_presence_of :hostname,
+ :message => 'A hostname must be specified.'
+
+ # validate hostname doesn't contain invalid chars?
+
+ # validates presence, format, or inclusion of arch?
+
+ validates_inclusion_of :hypervisor_type,
+ :in => HYPERVISOR_TYPES
+
+ validates_inclusion_of :state,
+ :in => STATES + Task::COMPLETED_STATES + TASK::WORKING_STATES
KVM_HYPERVISOR_TYPE = "KVM"
HYPERVISOR_TYPES = [KVM_HYPERVISOR_TYPE]
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/nic.rb b/src/app/models/nic.rb
index 5649763..c9f970d 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..cffb52f 100644
--- a/src/app/models/storage_pool.rb
+++ b/src/app/models/storage_pool.rb
@@ -39,6 +39,15 @@ class StoragePool < ActiveRecord::Base
validates_presence_of :hardware_pool_id
+ validates_inclusion_of :type,
+ :in => %w( IscsiStoragePool LvmStoragePool NfsStoragePool )
+
+ validates_inclustion_of :state,
+ :in => [ STATE_PENDING_SETUP, STATE_PENDING_DELETION, STATE_AVAILABLE]
+
+ 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
diff --git a/src/app/models/storage_volume.rb b/src/app/models/storage_volume.rb
index 39b72d5..0c05378 100644
--- a/src/app/models/storage_volume.rb
+++ b/src/app/models/storage_volume.rb
@@ -32,6 +32,21 @@ class StorageVolume < ActiveRecord::Base
end
end
+ validates_presence_of :storage_pool_id
+
+ validates_inclustion_of :state,
+ :in => [ STATE_PENDING_SETUP, STATE_PENDING_DELETION, STATE_AVAILABLE]
+
+ 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 )
+
+ # FIXME need to validate lun, filename, lv_name, lv_owner_perms,
+ # lv_group_perms, lv_mode_perms (which depends on what subclasses?)
+
STATE_PENDING_SETUP = "pending_setup"
STATE_PENDING_DELETION = "pending_deletion"
STATE_AVAILABLE = "available"
diff --git a/src/app/models/task.rb b/src/app/models/task.rb
index f231c18..bf5fbbd 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_inclustion_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,10 @@ class Task < ActiveRecord::Base
""
end
+ def validate
+ errors.add("id", "Parent must be specified") if hardware_pool_id.nil? and vm_resource_pool_id.nil?
+ 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..6038ac6 100644
--- a/src/app/models/vm.rb
+++ b/src/app/models/vm.rb
@@ -34,7 +34,35 @@ 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_inclusion_of :state,
+ :in => RUNNING_STATES + DESTROYABLE_STATES
+
+ 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" ] ],
--
1.6.0.4
More information about the ovirt-devel
mailing list