pgreco / rpms / ipa

Forked from forks/areguera/rpms/ipa 4 years ago
Clone
Blob Blame History Raw
From a3801d66f5cf3f08062c3aa67a7b33d13fae56b7 Mon Sep 17 00:00:00 2001
From: Martin Babinsky <mbabinsk@redhat.com>
Date: Thu, 11 May 2017 15:55:53 +0200
Subject: [PATCH] Allow for multivalued server attributes

In order to achieve the task, the following changes were required:

* vectorize the base class for server attributes
* add a child class that enforces single-value attributes. It still
  accepts/returns single-value lists in order to not break Liskov
  substitution principle
* Existing attributes inherit from the child class

https://pagure.io/freeipa/issue/6937

Reviewed-By: Jan Cholasta <jcholast@redhat.com>
Reviewed-By: Stanislav Laznicka <slaznick@redhat.com>
---
 ipaserver/plugins/serverroles.py            |   4 +-
 ipaserver/servroles.py                      | 109 +++++++++++++++++++---------
 ipatests/test_ipaserver/test_serverroles.py |  10 +--
 3 files changed, 79 insertions(+), 44 deletions(-)

diff --git a/ipaserver/plugins/serverroles.py b/ipaserver/plugins/serverroles.py
index e22eadd7b163469cc9fc4472640aa64d21c9d38f..e81635c3315cc3fca84450f43fb7df883aae57d9 100644
--- a/ipaserver/plugins/serverroles.py
+++ b/ipaserver/plugins/serverroles.py
@@ -136,9 +136,7 @@ class serverroles(Backend):
 
         for name, attr in assoc_attributes.items():
             attr_value = attr.get(self.api)
-
-            if attr_value is not None:
-                result.update({name: attr_value})
+            result.update({name: attr_value})
 
         return result
 
diff --git a/ipaserver/servroles.py b/ipaserver/servroles.py
index cf4599995118c589a5b51236c68e13f14ac1257b..84fed1046b3b46dc9f5f8bbe6e03354725f1136c 100644
--- a/ipaserver/servroles.py
+++ b/ipaserver/servroles.py
@@ -277,29 +277,33 @@ class ServerAttribute(LDAPBasedProperty):
         try:
             entries = ldap2.get_entries(search_base, filter=search_filter)
         except errors.EmptyResult:
-            return
+            return []
 
-        master_cn = entries[0].dn[1]['cn']
+        master_cns = {e.dn[1]['cn'] for e in entries}
 
         associated_role_providers = set(
             self._get_assoc_role_providers(api_instance))
 
-        if master_cn not in associated_role_providers:
+        if not master_cns.issubset(associated_role_providers):
             raise errors.ValidationError(
                 name=self.name,
                 error=_("all masters must have %(role)s role enabled" %
                         {'role': self.associated_role.name})
             )
 
-        return master_cn
+        return sorted(master_cns)
 
-    def _get_master_dn(self, api_instance, server):
-        return DN(('cn', server), api_instance.env.container_masters,
-                  api_instance.env.basedn)
+    def _get_master_dns(self, api_instance, servers):
+        return [
+            DN(('cn', server), api_instance.env.container_masters,
+               api_instance.env.basedn) for server in servers]
+
+    def _get_masters_service_entries(self, ldap, master_dns):
+        service_dns = [
+            DN(('cn', self.associated_service_name), master_dn) for master_dn
+            in master_dns]
 
-    def _get_masters_service_entry(self, ldap, master_dn):
-        service_dn = DN(('cn', self.associated_service_name), master_dn)
-        return ldap.get_entry(service_dn)
+        return [ldap.get_entry(service_dn) for service_dn in service_dns]
 
     def _add_attribute_to_svc_entry(self, ldap, service_entry):
         """
@@ -341,65 +345,98 @@ class ServerAttribute(LDAPBasedProperty):
             r[u'server_server'] for r in self.associated_role.status(
                 api_instance) if r[u'status'] == ENABLED]
 
-    def _remove(self, api_instance, master):
+    def _remove(self, api_instance, masters):
         """
-        remove attribute from the master
+        remove attribute from one or more masters
 
         :param api_instance: API instance
-        :param master: master FQDN
+        :param master: list or iterable containing master FQDNs
         """
 
         ldap = api_instance.Backend.ldap2
 
-        master_dn = self._get_master_dn(api_instance, master)
-        service_entry = self._get_masters_service_entry(ldap, master_dn)
-        self._remove_attribute_from_svc_entry(ldap, service_entry)
+        master_dns = self._get_master_dns(api_instance, masters)
+        service_entries = self._get_masters_service_entries(ldap, master_dns)
+
+        for service_entry in service_entries:
+            self._remove_attribute_from_svc_entry(ldap, service_entry)
 
-    def _add(self, api_instance, master):
+    def _add(self, api_instance, masters):
         """
         add attribute to the master
         :param api_instance: API instance
-        :param master: master FQDN
+        :param master: iterable containing master FQDNs
 
         :raises: * errors.ValidationError if the associated role is not enabled
                    on the master
         """
 
-        assoc_role_providers = self._get_assoc_role_providers(api_instance)
+        assoc_role_providers = set(
+            self._get_assoc_role_providers(api_instance))
+        masters_set = set(masters)
         ldap = api_instance.Backend.ldap2
 
-        if master not in assoc_role_providers:
+        masters_without_role = masters_set - assoc_role_providers
+
+        if masters_without_role:
             raise errors.ValidationError(
-                name=master,
+                name=', '.join(sorted(masters_without_role)),
                 error=_("must have %(role)s role enabled" %
                         {'role': self.associated_role.name})
             )
 
-        master_dn = self._get_master_dn(api_instance, master)
-        service_entry = self._get_masters_service_entry(ldap, master_dn)
-        self._add_attribute_to_svc_entry(ldap, service_entry)
+        master_dns = self._get_master_dns(api_instance, masters)
+        service_entries = self._get_masters_service_entries(ldap, master_dns)
+        for service_entry in service_entries:
+            self._add_attribute_to_svc_entry(ldap, service_entry)
 
-    def set(self, api_instance, master):
+    def set(self, api_instance, masters):
         """
-        set the attribute on master
+        set the attribute on masters
 
         :param api_instance: API instance
-        :param master: FQDN of the new master
+        :param masters: an interable with FQDNs of the new masters
 
-        the attribute is automatically unset from previous master if present
+        the attribute is automatically unset from previous masters if present
 
         :raises: errors.EmptyModlist if the new masters is the same as
-                 the original on
+                 the original ones
         """
-        old_master = self.get(api_instance)
+        old_masters = self.get(api_instance)
 
-        if old_master == master:
+        if sorted(old_masters) == sorted(masters):
             raise errors.EmptyModlist
 
-        self._add(api_instance, master)
+        if old_masters:
+            self._remove(api_instance, old_masters)
+
+        self._add(api_instance, masters)
+
+
+class SingleValuedServerAttribute(ServerAttribute):
+    """
+    Base class for server attributes that are forced to be single valued
+
+    this means that `get` method will return a one-element list, and `set`
+    method will accept only one-element list
+    """
+
+    def set(self, api_instance, masters):
+        if len(masters) > 1:
+            raise errors.ValidationError(
+                name=self.attr_name,
+                error=_("must be enabled only on a single master"))
+
+        super(SingleValuedServerAttribute, self).set(api_instance, masters)
+
+    def get(self, api_instance):
+        masters = super(SingleValuedServerAttribute, self).get(api_instance)
+        num_masters = len(masters)
+
+        if num_masters > 1:
+            raise errors.SingleMatchExpected(found=num_masters)
 
-        if old_master is not None:
-            self._remove(api_instance, old_master)
+        return masters
 
 
 _Service = namedtuple('Service', ['name', 'enabled'])
@@ -574,14 +611,14 @@ role_instances = (
 )
 
 attribute_instances = (
-    ServerAttribute(
+    SingleValuedServerAttribute(
         u"ca_renewal_master_server",
         u"CA renewal master",
         u"ca_server_server",
         u"CA",
         u"caRenewalMaster",
     ),
-    ServerAttribute(
+    SingleValuedServerAttribute(
         u"dnssec_key_master_server",
         u"DNSSec key master",
         u"dns_server_server",
diff --git a/ipatests/test_ipaserver/test_serverroles.py b/ipatests/test_ipaserver/test_serverroles.py
index d8844df3007f076532a34d625379ab552ef45363..e671272783d8d71c2ee56074459433b98b79dd0a 100644
--- a/ipatests/test_ipaserver/test_serverroles.py
+++ b/ipatests/test_ipaserver/test_serverroles.py
@@ -706,7 +706,7 @@ class TestServerAttributes(object):
         actual_attr_masters = self.config_retrieve(
             assoc_role, mock_api)[attr_name]
 
-        assert actual_attr_masters == fqdn
+        assert actual_attr_masters == [fqdn]
 
     def test_set_attribute_on_the_same_provider_raises_emptymodlist(
             self, mock_api, mock_masters):
@@ -727,7 +727,7 @@ class TestServerAttributes(object):
         non_ca_fqdn = mock_masters.get_fqdn('trust-controller-dns')
 
         with pytest.raises(errors.ValidationError):
-            self.config_update(mock_api, **{attr_name: non_ca_fqdn})
+            self.config_update(mock_api, **{attr_name: [non_ca_fqdn]})
 
     def test_set_unknown_attribute_on_master_raises_notfound(
             self, mock_api, mock_masters):
@@ -735,7 +735,7 @@ class TestServerAttributes(object):
         fqdn = mock_masters.get_fqdn('trust-controller-ca')
 
         with pytest.raises(errors.NotFound):
-            self.config_update(mock_api, **{attr_name: fqdn})
+            self.config_update(mock_api, **{attr_name: [fqdn]})
 
     def test_set_ca_renewal_master_on_other_ca_and_back(self, mock_api,
                                                         mock_masters):
@@ -747,7 +747,7 @@ class TestServerAttributes(object):
         other_ca_server = mock_masters.get_fqdn('trust-controller-ca')
 
         for host in (other_ca_server, original_renewal_master):
-            self.config_update(mock_api, **{attr_name: host})
+            self.config_update(mock_api, **{attr_name: [host]})
 
             assert (
-                self.config_retrieve(role_name, mock_api)[attr_name] == host)
+                self.config_retrieve(role_name, mock_api)[attr_name] == [host])
-- 
2.9.4