[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, 
<BaseServerRole>): 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]
             self._validate_component_services(services)
             if all(s.enabled for s in services):
                 providers.append(master)

I was quite puzzled with this code until I realized that you are actually finding just subset of services that belongs to a role, not all services on particular master, maybe comment may help or variable naming

also please use _are_services_enabled() instead of copy-paste

12)
def obj_pkey_from_hostname

This is used just once and it just return param as result without any change, do you have any future plans with it? If not then please remove it.


13)
************* Module ipaserver.plugins.serverroles
ipaserver/plugins/serverroles.py:579: [W0223(abstract-method), ADTrustBasedRole] Method 'fqdn_from_entry' is abstract in class 'MembershipBasedRole' but is not overridden)


IMO there is unneeded inheritance, keep number of levels as low as possible, IMO ADTrustAgent can inherit directly from MembershipBasedRole and implement group_dn property, because ADTrustBasedRole is used just once.

14)
I'm not sure if self.group_dn express the right thing, because

class ADTrustBasedRole(MembershipBasedRole):
     @property
     def group_dn(self):
         return DN(
             ('cn', 'adtrust agents'), ('cn', 'sysaccounts'),
             ('cn', 'etc'), self.api.env.basedn)

It is account not group, I'm not sure what get_dn should express

15)
I'm not a big fan of MembershipBasedRole class, if this circus is done just because ADTrustAgent service is nonstandard, I would rather have single implementation of that magic instead of having baseclass that is never used for anything else and wasting time with implementation of a general solution.

I don't like that in current implementation you mixed show commands, direct ldap searches, for the same thing, and also using 'api.env.container_*' does not look safe for me as we do not enforce the exact format and it may break in future or with different objects.


16)
class serverroles(Backend):

Please don't do any lower() operation with options here, it should be just obj_type="role"
def _get_obj_store(self, obj_type="Role"):
     if obj_type.lower() == "role":

this is internal API so in case that somebody does not meet the lower case just raise exception violently


The rest tomorrow :)
Martin^2



-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160524/358faf76/attachment.htm>