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



-- 
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

Reply via email to