From a3801d66f5cf3f08062c3aa67a7b33d13fae56b7 Mon Sep 17 00:00:00 2001 From: Martin Babinsky 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 Reviewed-By: Stanislav Laznicka --- 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