Re: [Freeipa-devel] [PATCH 0146-0147] Server Roles: basic infrastructure

[Martin Babinsky]

On 05/19/2016 04:59 PM, Martin Babinsky wrote:

Patch 0146 implements lower-lever infrastructure for querying server
roles/attributes

Patch 0147 are some basic tests slapped together for the `serverroles`
backend to ensure that it works as expected.

The new/modified CLI commands specified in the design page [1] will be
coming soon.

Also coming soon will be an interface to construct DNS records for each
role requested by Petr^2 and Martin^2 which I will start implement when
we agree on the backend implementation.

https://fedorahosted.org/freeipa/ticket/5181

[1] https://www.freeipa.org/page/V4/Server_Roles#CLI



Ignore this thread please, all other revisions will be sent together 
with user-facing part in separate thread.


--
Martin^3 Babinsky

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCH 0146-0147] Server Roles: basic infrastructure

[Martin Babinsky]

On 05/27/2016 05:30 PM, Martin Babinsky wrote:

On 05/19/2016 04:59 PM, Martin Babinsky wrote:

Patch 0146 implements lower-lever infrastructure for querying server
roles/attributes

Patch 0147 are some basic tests slapped together for the `serverroles`
backend to ensure that it works as expected.

The new/modified CLI commands specified in the design page [1] will be
coming soon.

Also coming soon will be an interface to construct DNS records for each
role requested by Petr^2 and Martin^2 which I will start implement when
we agree on the backend implementation.

https://fedorahosted.org/freeipa/ticket/5181

[1] https://www.freeipa.org/page/V4/Server_Roles#CLI




Thanks Martin and Honza for review.

I have reworked the patches based on your comments. I have split patch
146 into two (role/attribute definitions and backend code) so patches
146-147 are for code and 148 hosts test suite.

I hope that I will send API patches on monday after I resolve some
framework-related questions with the local guru.





Another revision of backend patches attached.

--
Martin^3 Babinsky
From 27e5dbcab36d8c9155cbc8dab695fa31dbf715c1 Mon Sep 17 00:00:00 2001
From: Martin Babinsky 
Date: Thu, 26 May 2016 19:24:22 +0200
Subject: [PATCH 1/3] Server roles: definitions of server roles and attributes

This patch introduces classes which define the properties of server roles and
attributes and their relationship to LDAP attributes representing the
role/attribute.

A brief documentation about defining and using roles is given at the beginning
of the module.

http://www.freeipa.org/page/V4/Server_Roles
https://fedorahosted.org/freeipa/ticket/5181
---
 ipaserver/servroles.py | 612 +
 1 file changed, 612 insertions(+)
 create mode 100644 ipaserver/servroles.py

diff --git a/ipaserver/servroles.py b/ipaserver/servroles.py
new file mode 100644
index ..6f732f71a3bcaa0c08fad94c529979873fdcc429
--- /dev/null
+++ b/ipaserver/servroles.py
@@ -0,0 +1,612 @@
+#
+# Copyright (C) 2016 FreeIPA Contributors see COPYING for license
+#
+
+
+"""
+This module contains the set of classes which abstract various bits and pieces
+of information present in the LDAP tree about functionalities such as DNS
+server, Active Directory trust controller etc. These properties come in two
+distinct groups:
+
+server roles
+this group represents a genral functionality provided by one or more
+IPA servers, such as DNS server, certificate authority and such. In
+this case there is a many-to-many mapping between the roles and the
+masters which provide them.
+
+server attributes
+these represent a functionality associated with the whole topology,
+such as CA renewal master or DNSSec key master.
+
+See the corresponding design page (http://www.freeipa.org/page/V4/Server_Roles)
+for more info.
+
+Both of these groups use `LDAPBasedProperty` class as a base.
+
+Server Roles
+
+
+Server role objects are usually consuming information from the master's service
+container (cn=FQDN,cn=masters,cn=ipa,cn=etc,$SUFFIX) are represented by
+`ServiceBasedRole`class. To create an instance of such role, you only need to
+specify role name and individual services comprising the role (more systemd
+services may be enabled to provide some function):
+
+>>> example_role = ServiceBasedRole(
+... "Example Role",
+... component_services = ['SERVICE1', 'SERVICE2'])
+>>> example_role.name
+'Example Role'
+
+Each role object has an `attr_name` property which returns its name transformed
+into an LDAP-like attribute name useful in higher-level commands implemented in
+API framework
+
+>>> example_role.attr_name
+'ipaexamplerole'
+
+The role object can then be queried for the status of the role in the whole
+topology or on a single master by using its `status` method. This method
+returns a list of dictionaries akin to LDAP entries comprised from server name,
+role name and role status (enabled if role is enabled, configured if the
+service entries are present but not marked as enabled by 'enabledService'
+config string, absent if the service entries are not present).
+
+Note that 'AD trust agent' role is based on membership of the master in the
+'adtrust agents' sysaccount group and is thus an instance of different class
+(`ADTrustBasedRole`). This role also does not have 'configured' status, since
+the master is either member of the group ('enabled') or not ('absent')
+
+Server Attributes
+=
+
+Server attributes are implemented as instances of `ServerAttribute` class. The
+attribute is defined by some flag set ornf 'ipaConfigString' attribute of some
+service entry. To create your own server attribute, see the following example:
+
+>>> example_attribute = ServerAttribute("Example Attribute", example_role,
+... "SERVICE1", "roleMaster")
+>>> 

Re: [Freeipa-devel] [PATCH 0146-0147] Server Roles: basic infrastructure

[Martin Babinsky]

On 05/19/2016 04:59 PM, Martin Babinsky wrote:

Patch 0146 implements lower-lever infrastructure for querying server
roles/attributes

Patch 0147 are some basic tests slapped together for the `serverroles`
backend to ensure that it works as expected.

The new/modified CLI commands specified in the design page [1] will be
coming soon.

Also coming soon will be an interface to construct DNS records for each
role requested by Petr^2 and Martin^2 which I will start implement when
we agree on the backend implementation.

https://fedorahosted.org/freeipa/ticket/5181

[1] https://www.freeipa.org/page/V4/Server_Roles#CLI




Thanks Martin and Honza for review.

I have reworked the patches based on your comments. I have split patch 
146 into two (role/attribute definitions and backend code) so patches 
146-147 are for code and 148 hosts test suite.


I hope that I will send API patches on monday after I resolve some 
framework-related questions with the local guru.


--
Martin^3 Babinsky
From 6994428e527760fdd6d5cb442af0a1f321a4e542 Mon Sep 17 00:00:00 2001
From: Martin Babinsky 
Date: Thu, 26 May 2016 19:24:22 +0200
Subject: [PATCH 1/3] Server roles: definitions of server roles and attributes

This patch introduces classes which define the properties of server roles and
attributes and their relationship to LDAP attributes representing the
role/attribute.

A brief documentation about defining and using roles is given at the beginning
of the module.

http://www.freeipa.org/page/V4/Server_Roles
https://fedorahosted.org/freeipa/ticket/5181
---
 ipaserver/servroles.py | 574 +
 1 file changed, 574 insertions(+)
 create mode 100644 ipaserver/servroles.py

diff --git a/ipaserver/servroles.py b/ipaserver/servroles.py
new file mode 100644
index ..0d5c997c9022fa256a5488772b15514570bc0dc2
--- /dev/null
+++ b/ipaserver/servroles.py
@@ -0,0 +1,574 @@
+#
+# Copyright (C) 2016 FreeIPA Contributors see COPYING for license
+#
+
+
+"""
+This module contains the set of classes which abstract various bits and pieces
+of information present in the LDAP tree about functionalities such as DNS
+server, Active Directory trust controller etc. These properties come in two
+distinct groups:
+
+server roles
+this group represents a genral functionality provided by one or more
+IPA servers, such as DNS server, certificate authority and such. In
+this case there is a many-to-many mapping between the roles and the
+masters which provide them.
+
+server attributes
+these represent a functionality associated with the whole topology,
+such as CA renewal master or DNSSec key master.
+
+See the corresponding design page (http://www.freeipa.org/page/V4/Server_Roles)
+for more info.
+
+Both of these groups use `LDAPBasedProperty` class as a base.
+
+Server Roles
+
+
+Server role objects are usually consuming information from the master's service
+container (cn=FQDN,cn=masters,cn=ipa,cn=etc,$SUFFIX) are represented by
+`ServiceBasedRole`class. To create an instance of such role, you only need to
+specify role name and individual services comprising the role (more systemd
+services may be enabled to provide some function):
+
+>>> example_role = ServiceBasedRole(
+... "Example Role",
+... component_services = ['SERVICE1', 'SERVICE2'])
+>>> example_role.name
+'Example Role'
+
+Each role object has an `attr_name` property which returns its name transformed
+into an LDAP-like attribute name useful in higher-level commands implemented in
+API framework
+
+>>> example_role.attr_name
+'ipaexamplerole'
+
+The role object can then be queried for the status of the role in the whole
+topology or on a single master by using its `status` method. This method
+returns a list of dictionaries akin to LDAP entries comprised from server name,
+role name and role status (enabled if role is enabled, configured if the
+service entries are present but not marked as enabled by 'enabledService'
+config string, absent if the service entries are not present).
+
+Note that 'AD trust agent' role is based on membership of the master in the
+'adtrust agents' sysaccount group and is thus an instance of different class
+(`ADTrustBasedRole`). This role also does not have 'configured' status, since
+the master is either member of the group ('enabled') or not ('absent')
+
+Server Attributes
+=
+
+Server attributes are implemented as instances of `ServerAttribute` class. The
+attribute is defined by some flag set ornf 'ipaConfigString' attribute of some
+service entry. To create your own server attribute, see the following example:
+
+>>> example_attribute = ServerAttribute("Example Attribute", example_role,
+... "SERVICE1", "roleMaster")
+>>> example_attribute.name
+'Example Attribute'
+
+As in the case of serverroles, each instance has the `attr_name` 

Re: [Freeipa-devel] [PATCH 0146-0147] Server Roles: basic infrastructure

[Martin Basti]

On 19.05.2016 16:59, Martin Babinsky wrote:
Patch 0146 implements lower-lever infrastructure for querying server 
roles/attributes


Patch 0147 are some basic tests slapped together for the `serverroles` 
backend to ensure that it works as expected.


The new/modified CLI commands specified in the design page [1] will be 
coming soon.


Also coming soon will be an interface to construct DNS records for 
each role requested by Petr^2 and Martin^2 which I will start 
implement when we agree on the backend implementation.


https://fedorahosted.org/freeipa/ticket/5181

[1] https://www.freeipa.org/page/V4/Server_Roles#CLI





Hello, I have a few comments

0)
I'm afraid that your docstrings contains docstest syntax and it may fail

1)
IIRC the factory should return just one particular object, not dict of 
objects, should be these function called differently?


+def role_factory(api_instance):
+"""
+return a dict of server role instances keyed by lower-cased role name
+"""
+
+return {key: role_cls(api_instance) for key, role_cls in
+role_registry.items()}
+
+
+def attribute_factory(api_instance):
+"""
+return a dict of server attribute instances keyed by lower-cased 
attribute

+name
+"""
+return {key: attr_cls(api_instance) for key, attr_cls in
+attribute_registry.items()}

2)
+class LDAPBasedProperty(object):

+def __init__(self, api_instance):
+self.api = api_instance
+self.ldap_conn = self.api.Backend.ldap2

self.ldap_conn, this may bite us later,  ldap2 may not exist when object 
is created an thus this may cause the AttributeErrors. IMO in method 
there should be used directly

ldap = self.api.Backend.ldap2
ldap.search()

Otherwise you will not be able to create objects without connected LDAP 
(just future thinking) we had simillar issue in DNS that in upgrade code 
it was failing


3)
+class BaseServerAttribute(LDAPBasedProperty):
...
+# pylint: disable=not-callable
+self.role_instance = self.associated_role(
+api_instance)
+# pylint: enable=not-callable

I don't like this, twice; don't disable pylint, do some check in 
constructor if associated_role was defined


Something like if not isinstance(self.associated_role, 
): raise TypeError(Define role)


Or put as default ABCMetaclass (BaseServerRole), that should blow up

4)
ipa_config_string_value = None

IMO this should be empty list (in the same class)

5)

associated_service_name = None

IMO there should be check in constructor, that this variable is defined with 
something else than None, to avoid hard to not ot nice to debug errors


6)
@property
def providers(self):
"""
:returns: list of masters providing the attribute. In the case of
singular attributes returns a list containing single object
"""
try:
entries = self.search(self.search_base)
except errors.EmptyResult:
return set()

I'm afraid this may break sometimes, because what exception is raised depends 
on implementation of search(), so IMO would be better to catch exception in 
search() and return set() from search method, or define new exception which 
should be used for this case and document it in search() method

7)

class ServiceBasedRole(BaseServerRole):
...

enabled = all(

value in entry.get(attribute, []) for (attribute, value) in
self.enabled_attrs_value.items()
)

Here ipaConfigString is case insensitive, so I'm afraid that this may not work 
in cases that there is value 'enabledservice' instead of 'enabledService'

But if you are sure that this cannot happen in IPA, we can leave it as is, 
otherwise you have to distinguish case sensitive and case insensitive attrs

IMO in this case generalization may cause more harm than gain. IMO should be 
better to create extra method (maybe abstract) which will return state if 
service is enabled or not in LDAP.
Or create default implementation that compares just ipaConfigScript and 
subclasses should call it via super() and add it owns check by super

8)

class ServiceBasedRole(BaseServerRole):
...

try:
entries = self.search(search_base)

services = [self._get_service(e) for e in entries]
self._validate_component_services(services)

return (ENABLED if self._are_services_enabled(services) else
CONFIGURED)
except errors.EmptyResult:
return ABSENT

Same as 6). IMO you can put there else statement as part of first try-except 
block

9)
class ServiceBasedRole(BaseServerRole):
...
@property
def providers(self):

There is no except catching EmptyResults (not needed if you reimplement it as I 
suggested above)

10) Nitpick alert

for master in services_by_master:
services = services_by_master[master]

for master, services in services_by_master.items() (or using six) is IMO better

11)
providers = []
for master in services_by_master:
services = services_by_master[master]

Re: [Freeipa-devel] [PATCH 0146-0147] Server Roles: basic infrastructure

[Martin Babinsky]

On 05/19/2016 04:59 PM, Martin Babinsky wrote:

Patch 0146 implements lower-lever infrastructure for querying server
roles/attributes

Patch 0147 are some basic tests slapped together for the `serverroles`
backend to ensure that it works as expected.

The new/modified CLI commands specified in the design page [1] will be
coming soon.

Also coming soon will be an interface to construct DNS records for each
role requested by Petr^2 and Martin^2 which I will start implement when
we agree on the backend implementation.

https://fedorahosted.org/freeipa/ticket/5181

[1] https://www.freeipa.org/page/V4/Server_Roles#CLI




I have found some small errors in the code, attaching updated patch 146

--
Martin^3 Babinsky
From dbc25cd338eebcaf40ba95363c8b160a243ed03b Mon Sep 17 00:00:00 2001
From: Martin Babinsky 
Date: Thu, 19 May 2016 11:24:18 +0200
Subject: [PATCH 1/5] Server Roles: infrastructure for role/attribute
 definition and query

This patch implements basic infrastructure for querying and manipulating
server roles and server attributes. This consists of:

* class hierarchy for definition of service/membership based roles and
  server attributes. The definition of these classes is briefly documented
  in the module docstring

* `serverroles` backed which consumes the information provided by these
  classes and provides an API for actions such as displaying status of
  server roles on a master, listing all masters providing the
  role/attribute, setting/unsetting attribute on master, etc.

https://www.freeipa.org/page/V4/Server_Roles
https://fedorahosted.org/freeipa/ticket/5181
---
 ipalib/errors.py |  47 +++
 ipaserver/plugins/serverroles.py | 771 +++
 2 files changed, 818 insertions(+)
 create mode 100644 ipaserver/plugins/serverroles.py

diff --git a/ipalib/errors.py b/ipalib/errors.py
index 52fa25f02e02d1d71c012f32d761b64a838917be..ef1ad941cb6e54484ddf23143737f04178bcf280 100644
--- a/ipalib/errors.py
+++ b/ipalib/errors.py
@@ -1384,6 +1384,53 @@ class InvalidDomainLevelError(ExecutionError):
 errno = 4032
 format = _('%(reason)s')
 
+
+class MasterAlreadyProvidesRole(ExecutionError):
+"""
+**4033** Raised when setting role/attribute on a master that already
+ provides it.
+"""
+errno = 4033
+format = _('%(hostname)s is already %(role)s')
+
+
+class MasterDoesNotProvideRole(ExecutionError):
+"""
+**4034** Raised when removing role/attribute from a master that does not
+ provide it.
+"""
+errno = 4034
+format = _('%(hostname)s is not %(role)s')
+
+
+class ReadOnlyServerAttribute(ExecutionError):
+"""
+**4035** Raised when setting/unsetting read-only attribute on a server.
+"""
+errno = 4035
+format = _("%(attribute)s can not be set/unset using LDAP operations only")
+
+
+class SingleServerAttributeProviderExpected(ExecutionError):
+"""
+**4036** Raised when an server attribute that should be provided by single
+ server is present on multiple entries.
+"""
+errno = 4036
+format = _("%(attribute)s should be provided by one IPA master, "
+   "not %(count)s")
+
+
+class ServerAttributeSetWithoutAssociatedRole(ExecutionError):
+"""
+**4037** Raised when an server attribute is provided by master that does
+not provide the associated role
+"""
+errno = 4037
+format = _("%(attribute)s attribute set on master(s) with"
+   " required %(role)s role not enabled")
+
+
 class BuiltinError(ExecutionError):
 """
 **4100** Base class for builtin execution errors (*4100 - 4199*).
diff --git a/ipaserver/plugins/serverroles.py b/ipaserver/plugins/serverroles.py
new file mode 100644
index ..8dc30356cdb4933e47323282c622aae04efae910
--- /dev/null
+++ b/ipaserver/plugins/serverroles.py
@@ -0,0 +1,771 @@
+#
+# Copyright (C) 2016 FreeIPA Contributors see COPYING for license
+#
+
+
+"""
+This module contains the `serverroles` backend which serves as and API for
+retrieval (and sometimes manipulation) of functionality provided by masters
+present in FreeIPA topology. The backend itself leverages a set of classes
+which abstract various bits and pieces of information present in the LDAP tree
+about functionalities such as DNS server, Active Directory trust controller
+etc. These properties come in two distinct groups:
+
+server roles
+this group represents a genral functionality provided by one or more
+IPA servers, such as DNS server, certificate authority and such. In
+this case there is a many-to-many mapping between the roles and the
+masters which provide them.
+
+server attributes
+these are currently associated only to one server among the topology
+(singular attributes). For example, there is only one CA renewal
+master and one DNSSec key master 

[Freeipa-devel] [PATCH 0146-0147] Server Roles: basic infrastructure

[Martin Babinsky]

Patch 0146 implements lower-lever infrastructure for querying server 
roles/attributes


Patch 0147 are some basic tests slapped together for the `serverroles` 
backend to ensure that it works as expected.


The new/modified CLI commands specified in the design page [1] will be 
coming soon.


Also coming soon will be an interface to construct DNS records for each 
role requested by Petr^2 and Martin^2 which I will start implement when 
we agree on the backend implementation.


https://fedorahosted.org/freeipa/ticket/5181

[1] https://www.freeipa.org/page/V4/Server_Roles#CLI

--
Martin^3 Babinsky
From 1b9c4c0462bdef291c50e82f974441a56a3b5db8 Mon Sep 17 00:00:00 2001
From: Martin Babinsky 
Date: Thu, 19 May 2016 11:24:18 +0200
Subject: [PATCH 1/2] Server Roles: infrastructure for role/attribute
 definition and query

This patch implements basic infrastructure for querying and manipulating
server roles and server attributes. This consists of:

* class hierarchy for definition of service/membership based roles and
  server attributes. The definition of these classes is briefly documented
  in the module docstring

* `serverroles` backed which consumes the information provided by these
  classes and provides an API for actions such as displaying status of
  server roles on a master, listing all masters providing the
  role/attribute, setting/unsetting attribute on master, etc.

https://www.freeipa.org/page/V4/Server_Roles
https://fedorahosted.org/freeipa/ticket/5181
---
 ipalib/errors.py |  29 +-
 ipaserver/plugins/serverroles.py | 769 +++
 2 files changed, 797 insertions(+), 1 deletion(-)
 create mode 100644 ipaserver/plugins/serverroles.py

diff --git a/ipalib/errors.py b/ipalib/errors.py
index 52fa25f02e02d1d71c012f32d761b64a838917be..01340cbc540c87fca72fc8925a4b98255ff04611 100644
--- a/ipalib/errors.py
+++ b/ipalib/errors.py
@@ -1384,6 +1384,33 @@ class InvalidDomainLevelError(ExecutionError):
 errno = 4032
 format = _('%(reason)s')
 
+
+class MasterAlreadyProvidesRole(ExecutionError):
+"""
+**4033** Raised when setting role/attribute on a master that already
+ provides it.
+"""
+errno = 4033
+format = _('%(hostname)s is already %(role)s')
+
+
+class MasterDoesNotProvideRole(ExecutionError):
+"""
+**4034** Raised when removing role/attribute from a master that does not
+ provide it.
+"""
+errno = 4034
+format = _('%(hostname)s is not %(role)s')
+
+
+class ReadOnlyServerAttribute(ExecutionError):
+"""
+**4035** Raised when setting/unsetting read-only attribute on a server.
+"""
+errno = 4035
+format = _("%(attribute)s can not be set/unset using LDAP operations only")
+
+
 class BuiltinError(ExecutionError):
 """
 **4100** Base class for builtin execution errors (*4100 - 4199*).
@@ -1812,7 +1839,7 @@ class CertificateInvalidError(CertificateError):
 """
 **4310** Raised when a certificate is not valid
 
-For example:
+For example
 
 >>> raise CertificateInvalidError(name=_(u'CA'))
 Traceback (most recent call last):
diff --git a/ipaserver/plugins/serverroles.py b/ipaserver/plugins/serverroles.py
new file mode 100644
index ..86e6b626dde4ef3802b6c02d2f7c038f519d02fa
--- /dev/null
+++ b/ipaserver/plugins/serverroles.py
@@ -0,0 +1,769 @@
+#
+# Copyright (C) 2016 FreeIPA Contributors see COPYING for license
+#
+
+
+"""
+This module contains the `serverroles` backend which serves as and API for
+retrieval (and sometimes manipulation) of functionality provided by masters
+present in FreeIPA topology. The backend itself leverages a set of classes
+which abstract various bits and pieces of information present in the LDAP tree
+about functionalities such as DNS server, Active Directory trust controller
+etc. These properties come in two distinct groups:
+
+server roles
+this group represents a genral functionality provided by one or more
+IPA servers, such as DNS server, certificate authority and such. In
+this case there is a many-to-many mapping between the roles and the
+masters which provide them.
+
+server attributes
+these are currently associated only to one server among the topology
+(singular attributes). For example, there is only one CA renewal
+master and one DNSSec key master in the topology.
+
+See the corresponding design page (http://www.freeipa.org/page/V4/Server_Roles)
+for more info.
+
+Both of these groups use `LDAPBasedProperty` class as a base.
+
+Server Roles
+
+
+Server roles come in two flavors. Those that are consuming information from the
+master's service container (cn=FQDN,cn=masters,cn=ipa,cn=etc,$SUFFIX) inherit
+from the `ServiceBasedRole` subclass. To create such role, you only need to
+specify role name and individual services comprising the role