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