From d0bc28d37734a35c332e3e60ba56ccff5e037018 Mon Sep 17 00:00:00 2001 From: root Date: Fri, 4 Apr 2008 16:34:48 -0400 Subject: Working on adding checks for duplicate names/ips/macs to cobbler --- cobbler/api.py | 16 +++--- cobbler/collection.py | 69 +++++++++++++++++++++-- cobbler/commands.py | 30 +++++++++- cobbler/modules/cli_system.py | 7 ++- cobbler/settings.py | 2 + config/settings | 2 + templates/dnsmasq.template | 2 +- tests/tests.py | 125 ++++++++++++++++++++++++++++++++++++++++++ 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): -- cgit