Working on adding checks for duplicate names/ips/macs to cobbler

8 files changed, 238 insertions, 15 deletions

diff --git a/cobbler/api.py b/cobbler/api.py
index 13fc5f35..54c315be 100644
--- a/cobbler/api.py
+++ b/cobbler/api.py
@@ -221,21 +221,21 @@ class BootAPI:
         self.log("new_repo",[is_subobject])
         return self._config.new_repo(is_subobject=is_subobject)
 
-    def add_distro(self, ref):
+    def add_distro(self, ref, check_for_duplicate_names=False):
         self.log("add_distro",[ref.name])
-        return self._config.distros().add(ref,save=True)
+        return self._config.distros().add(ref,save=True,check_for_duplicate_names=check_for_duplicate_names)
 
-    def add_profile(self, ref):
+    def add_profile(self, ref, check_for_duplicate_names=False):
         self.log("add_profile",[ref.name])
-        return self._config.profiles().add(ref,save=True)
+        return self._config.profiles().add(ref,save=True,check_for_duplicate_names=check_for_duplicate_names)
 
-    def add_system(self,ref):
+    def add_system(self, ref, check_for_duplicate_names=False, check_for_duplicate_netinfo=False):
         self.log("add_system",[ref.name])
-        return self._config.systems().add(ref,save=True)
+        return self._config.systems().add(ref,save=True,check_for_duplicate_names=check_for_duplicate_names,check_for_duplicate_netinfo=check_for_duplicate_netinfo)
 
-    def add_repo(self,ref):
+    def add_repo(self, ref, check_for_duplicate_names=False):
         self.log("add_repo",[ref.name])
-        return self._config.repos().add(ref,save=True)
+        return self._config.repos().add(ref,save=True,check_for_duplicate_names=check_for_duplicate_names)
 
     def find_distro(self, name=None, return_list=False, **kargs):
         return self._config.distros().find(name=name, return_list=return_list, **kargs)
diff --git a/cobbler/collection.py b/cobbler/collection.py
index 339a4b2b..14bba570 100644
--- a/cobbler/collection.py
+++ b/cobbler/collection.py
@@ -35,7 +35,8 @@ class Collection(serializable.Serializable):
         """
         self.config = config
         self.clear()
-        self.log_func = self.config.api.log
+        self.api = self.config.api
+        self.log_func = self.api.log
         self.lite_sync = None
 
     def factory_produce(self,config,seed_data):
@@ -125,10 +126,10 @@ class Collection(serializable.Serializable):
                   k.set_parent(newname)
                else:
                   k.set_distro(newname)
-               self.config.api.profiles().add(k, save=True, with_sync=with_sync, with_triggers=with_triggers)
+               self.api.profiles().add(k, save=True, with_sync=with_sync, with_triggers=with_triggers)
             elif k.COLLECTION_TYPE == "system":
                k.set_profile(newname)
-               self.config.api.systems().add(k, save=True, with_sync=with_sync, with_triggers=with_triggers)
+               self.api.systems().add(k, save=True, with_sync=with_sync, with_triggers=with_triggers)
             elif k.COLLECTION_TYPE == "repo":
                raise CX(_("internal error, not expected to have repo child objects"))
             else:
@@ -139,7 +140,7 @@ class Collection(serializable.Serializable):
         return True
 
 
-    def add(self,ref,save=False,with_copy=False,with_triggers=True,with_sync=True,quick_pxe_update=False):
+    def add(self,ref,save=False,with_copy=False,with_triggers=True,with_sync=True,quick_pxe_update=False,check_for_duplicate_names=False,check_for_duplicate_netinfo=False):
         """
         Add an object to the collection, if it's valid.  Returns True
         if the object was added to the collection.  Returns False if the
@@ -167,6 +168,11 @@ class Collection(serializable.Serializable):
             # if not saving the object, you can't run these features
             with_triggers = False
             with_sync = False
+        
+        # Avoid adding objects to the collection
+        # if an object of the same/ip/mac already exists.
+        self.__duplication_checks(ref,check_for_duplicate_names,check_for_duplicate_netinfo)
+
 
         if ref is None or not ref.is_valid():
             raise CX(_("insufficient or invalid arguments supplied"))
@@ -220,6 +226,61 @@ class Collection(serializable.Serializable):
     def _run_triggers(self,ref,globber):
         return utils.run_triggers(ref,globber)
 
+    def __duplication_checks(self,ref,check_for_duplicate_names,check_for_duplicate_netinfo):
+        """
+        Prevents adding objects with the same name.
+        Prevents adding or editing to provide the same IP, or MAC.
+        This applies to new "add" commands, 
+        Edits that are not copies/renames should only yelp if they match
+        an object that is not the same as the object being edited. (FIXME)
+        """
+
+        # always protect against duplicate names
+        if check_for_duplicate_names:
+            match = None
+            if isinstance(ref, item_system.System):
+                match = self.api.find_system(ref.name)
+            elif isinstance(ref, item_profile.Profile):
+                match = self.api.find_profile(ref.name)
+            elif isinstance(ref, item_distro.Distro):
+                match = self.api.find_distro(ref.name)
+            elif isinstance(ref, item_repo.Repo):
+                match = self.api.find_repo(ref.name)
+
+            if match:
+                raise CX(_("An object already exists with that name.  Try 'edit'?"))
+        
+        # the duplicate mac/ip checks can be disabled.
+        if not check_for_duplicate_netinfo:
+            return
+       
+        # FIXME: if we run this command on edits it may yield false
+        # positives when ref is set to replace an object of an existing
+        # name, so we should probably /not/ run this on edits yet at this
+        # point.  Logic to deal with renames vs. edit/copies is somewhat
+        # involved.
+        if isinstance(ref, item_system.System):
+           for (name, intf) in ref.interfaces.iteritems():
+               match_ip = []
+               match_mac = []
+               input_mac = intf["mac_address"] 
+               input_ip  = intf["ip_address"]
+               if not self.api.settings().allow_duplicate_macs and input_mac is not None and input_mac != "":
+                   match_mac = self.api.find_system(mac_address=input_mac,return_list=True)   
+               if not self.api.settings().allow_duplicate_ips and input_ip is not None and input_ip != "":
+                   match_ip  = self.api.find_system(ip_address=input_ip,return_list=True) 
+               # it's ok to conflict with your own net info.
+               # FIXME: copies won't ever work this way, so they should NOT
+               # use the flags that engadge these checks.  Renames should
+               # be exempt also
+
+               for x in match_mac:
+                   if x.name != ref.name:
+                       raise CX(_("Can't save system %s. The MAC address (%s) is already used by system %s (%s)") % (ref.name, intf["mac_address"], x.name, name))
+               for x in match_ip:
+                   if x.name != ref.name:
+                       raise CX(_("Can't save system %s. The IP address (%s) is already used by system %s (%s)") % (ref.name, intf["ip_address"], x.name, name))
+
     def printable(self):
         """
         Creates a printable representation of the collection suitable
diff --git a/cobbler/commands.py b/cobbler/commands.py
index d97a6e10..6bb04191 100644
--- a/cobbler/commands.py
+++ b/cobbler/commands.py
@@ -251,10 +251,38 @@ class CobblerFunction:
         opt_sync     = not options.nosync
         opt_triggers = not options.notriggers
 
+        # ** WARNING: COMPLICATED **
+        # what operation we call depends on what type of object we are editing
+        # and what the operation is.  The details behind this is that the
+        # add operation has special semantics around adding objects that might
+        # clobber other objects, and we don't want that to happen.  Edit
+        # does not have to check for named clobbering but still needs
+        # to check for IP/MAC clobbering in some scenarios (FIXME).
+        # this is all enforced by collections.py though we need to make
+        # the apppropriate call to add to invoke the safety code in the right
+        # places -- and not in places where the safety code will generate
+        # errors under legit circumstances.
+
         if not ("rename" in self.args):
-            rc = collect_fn().add(obj, save=True, with_sync=opt_sync, with_triggers=opt_triggers)
+            if "add" in self.args:
+               if obj.COLLECTION_TYPE == "system":
+                   # duplicate names and netinfo are both bad.
+                   rc = collect_fn().add(obj, save=True, with_sync=opt_sync, with_triggers=opt_triggers, check_for_duplicate_names=True, check_for_duplicate_netinfo=True)
+               else:
+                   # duplicate names are bad
+                   rc = collect_fn().add(obj, save=True, with_sync=opt_sync, with_triggers=opt_triggers, check_for_duplicate_names=True, check_for_duplicate_netinfo=False)
+            else:
+               # editing or copying (but not renaming), so duplicate netinfo
+               # CAN be bad, duplicate names are already handled, though
+               # we need to clean up checks around duplicate netinfo here
+               # (FIXME) so they are made and work.
+               rc = collect_fn().add(obj, save=True, with_sync=opt_sync, with_triggers=opt_triggers)
+
         else:
+            # we are renaming here, so duplicate netinfo checks also
+            # need to be made.(FIXME)
             rc = collect_fn().rename(obj, self.options.newname)
+
         return rc
 
     def reporting_sorter(self, a, b):
diff --git a/cobbler/modules/cli_system.py b/cobbler/modules/cli_system.py
index 01ead353..01f2ba60 100644
--- a/cobbler/modules/cli_system.py
+++ b/cobbler/modules/cli_system.py
@@ -103,8 +103,13 @@ class SystemFunction(commands.CobblerFunction):
         if self.options.owners:
             obj.set_owners(self.options.owners)
 
-        return self.object_manipulator_finish(obj, self.api.systems, self.options)
+        rc = self.object_manipulator_finish(obj, self.api.systems, self.options)
 
+        if ["copy"] in self.args:
+           # run through and find conflicts       
+           print _("WARNING: after copying systems, be sure that the ip/mac information is unique").
+
+        return rc
 
 
 ########################################################
diff --git a/cobbler/settings.py b/cobbler/settings.py
index 0ef2ab79..c201ddd1 100644
--- a/cobbler/settings.py
+++ b/cobbler/settings.py
@@ -24,6 +24,8 @@ TESTMODE = False
 DEFAULTS = {
     "allow_cgi_mac_registration"  : 0,
     "allow_cgi_profile_change"    : 0,
+    "allow_duplicate_macs"        : 0,
+    "allow_duplicate_ips"         : 0,
     "bootloaders"                 : {
         "standard"                : "/usr/lib/syslinux/pxelinux.0",
         "ia64"                    : "/var/lib/cobbler/elilo-3.6-ia64.efi"
diff --git a/config/settings b/config/settings
index 36cd9b4c..8fc0fdf3 100644
--- a/config/settings
+++ b/config/settings
@@ -1,6 +1,8 @@
 ---
 allow_cgi_mac_registration: 0
 allow_cgi_profile_change: 0
+allow_duplicate_macs: 0
+allow_duplicate_ips: 0
 bootloaders:
     ia64: /var/lib/cobbler/elilo-3.6-ia64.efi
     standard: /usr/lib/syslinux/pxelinux.0
diff --git a/templates/dnsmasq.template b/templates/dnsmasq.template
index 0646b710..28297d2d 100644
--- a/templates/dnsmasq.template
+++ b/templates/dnsmasq.template
@@ -6,7 +6,7 @@
 #no-poll
 #enable-dbus
 read-ethers
-addn-hosts = /var/lib/cobbler/hosts_cobbler/
+addn-hosts = /var/lib/cobbler/cobbler_hosts
 
 dhcp-range=192.168.1.5,192.168.1.200
 dhcp-option=3,$next_server
diff --git a/tests/tests.py b/tests/tests.py
index 8a86babd..4060eef1 100644
--- a/tests/tests.py
+++ b/tests/tests.py
@@ -8,6 +8,7 @@ import os
 import subprocess
 import tempfile
 import shutil
+import traceback
 
 from cobbler.cexceptions import *  
 
@@ -84,6 +85,7 @@ class BootTest(unittest.TestCase):
         system = self.api.new_system()
         self.assertTrue(system.set_name("drwily.rdu.redhat.com"))
         self.assertTrue(system.set_mac_address("BB:EE:EE:EE:EE:FF","intf0"))
+        self.assertTrue(system.set_ip_address("192.51.51.50","intf0"))
         self.assertTrue(system.set_profile("testprofile0"))
         self.assertTrue(self.api.add_system(system))
         self.assertTrue(self.api.find_system(name="drwily.rdu.redhat.com"))
@@ -100,6 +102,129 @@ class BootTest(unittest.TestCase):
         self.assertTrue(repo.set_mirror("/tmp/test_example_cobbler_repo"))
         self.assertTrue(self.api.repos().add(repo))
 
+class DuplicateNamesAndIpPrevention(BootTest):
+
+    """
+    The command line (and WebUI) have checks to prevent new system
+    additions from conflicting with existing systems and overwriting
+    them inadvertantly. This class tests that code.  NOTE: General API
+    users will /not/ encounter these checks.
+    """
+
+    def test_duplicate_prevention(self):
+
+        # find things we are going to test with
+        distro1 = self.api.find_distro(name="testdistro0")
+        profile1 = self.api.find_profile(name="testprofile0")
+        system1 = self.api.find_system(name="drwily.rdu.redhat.com")
+        repo1 = self.api.find_repo(name="test_repo")
+
+        # make sure we can't overwrite a previous distro with
+        # the equivalent of an "add" (not an edit) on the
+        # command line.
+        distro2 = self.api.new_distro()
+        self.assertTrue(distro2.set_name("testdistro0"))
+        self.assertTrue(distro2.set_kernel(self.fk_kernel))
+        self.assertTrue(distro2.set_initrd(self.fk_initrd))
+        self.assertTrue(distro2.set_owners("canary"))
+        # this should fail
+        try:
+           self.api.add_distro(distro2,check_for_duplicate_names=True)
+           self.assertTrue(1==2,"distro add should fail")
+        except CobblerException:
+           pass
+        except:
+           self.assertTrue(1==2,"exception type")
+        # we caught the exception but make doubly sure there was no write
+        distro_check = self.api.find_distro(name="testdistro0")
+        self.assertTrue("canary" not in distro_check.owners)
+
+        # repeat the check for profiles
+        profile2 = self.api.new_profile()
+        self.assertTrue(profile2.set_name("testprofile0"))
+        self.assertTrue(profile2.set_distro("testdistro0"))
+        # this should fail
+        try:
+            self.api.add_profile(profile2,check_for_duplicate_names=True)
+            self.assertTrue(1==2,"profile add should fail")
+        except CobblerException:
+            pass
+        except:
+            traceback.print_exc()
+            self.assertTrue(1==2,"exception type")
+
+        # repeat the check for systems (just names this time)
+        system2 = self.api.new_system()
+        self.assertTrue(system2.set_name("drwily.rdu.redhat.com"))
+        self.assertTrue(system2.set_profile("testprofile0"))
+        # this should fail
+        try:
+            self.api.add_system(system2,check_for_duplicate_names=True)
+            self.assertTrue(1==2,"system add should fail")
+        except CobblerException:
+            pass
+        except:
+            traceback.print_exc()
+            self.assertTrue(1==2,"exception type")
+
+        # repeat the check for repos
+        repo2 = self.api.new_repo()
+        self.assertTrue(repo2.set_name("test_repo"))
+        self.assertTrue(repo2.set_mirror("http://imaginary"))
+        # self.failUnlessRaises(CobblerException,self.api.add_repo,[repo,check_for_duplicate_names=True])
+        try:
+            self.api.add_repo(repo2,check_for_duplicate_names=True)
+            self.assertTrue(1==2,"repo add should fail")
+        except CobblerException:
+            pass
+        except:
+            self.assertTrue(1==2,"exception type")
+
+        # now one more check to verify we can't add a system
+        # of a different name but duplicate netinfo.  
+        system3 = self.api.new_system()
+        self.assertTrue(system3.set_name("unused_name"))
+        self.assertTrue(system3.set_profile("testprofile0"))
+        # MAC is initially accepted
+        self.assertTrue(system3.set_mac_address("BB:EE:EE:EE:EE:FF","intf3"))
+        # can't add as this MAC already exists!  
+
+        #self.failUnlessRaises(CobblerException,self.api.add_system,[system3,check_for_duplicate_names=True,check_for_duplicate_netinfo=True)
+        try:
+           self.api.add_system(system3,check_for_duplicate_names=True,check_for_duplicate_netinfo=True)
+        except CobblerException: 
+           pass
+        except:
+           traceback.print_exc()
+           self.assertTrue(1==2,"wrong exception type")
+
+        # set the MAC to a different value and try again
+        self.assertTrue(system3.set_mac_address("FF:EE:EE:EE:EE:DD","intf3"))
+        # it should work
+        self.assertTrue(self.api.add_system(system3,check_for_duplicate_names=True,check_for_duplicate_netinfo=True))
+        # now set the IP so that collides
+        self.assertTrue(system3.set_ip_address("192.51.51.50","intf6"))
+        # this should also fail
+
+        # self.failUnlessRaises(CobblerException,self.api.add_system,[system3,check_for_duplicate_names=True,check_for_duplicate_netinfo=True)
+        try:
+           self.api.add_system(system3,check_for_duplicate_names=True,check_for_duplicate_netinfo=True)
+           self.assertTrue(1==2,"system add should fail")
+        except CobblerException:
+           pass
+        except:
+           self.assertTrue(1==2,"wrong exception type")
+
+        # fix the IP and Mac back 
+        self.assertTrue(system3.set_ip_address("192.86.75.30","intf6"))
+        self.assertTrue(system3.set_mac_address("AE:BE:DE:CE:AE:EE","intf3"))
+        # now it works again
+        # note that we will not check for duplicate names as we want
+        # to test this as an 'edit' operation.
+        self.assertTrue(self.api.add_system(system3,check_for_duplicate_names=False,check_for_duplicate_netinfo=True))
+
+        # FIXME: note -- how netinfo is handled when doing renames/copies/edits
+        # is more involved and we probably should add tests for that also.
 
 class Ownership(BootTest):