Dne 21.5.2015 v 17:45 Endi Sukma Dewata napsal(a):
Please take a look at the new patch.

On 5/20/2015 1:53 AM, Jan Cholasta wrote:
I suppose you meant you're OK with not adding host vaults now?

Yes.

The only way to know if the design will be future proof is if we have at
least some idea how it will be used. Without that there is no guarantee.

Host principals have this form: host/<hostname>@<realm>, so with the
current code they will be considered a service and will have a service
container.

Do you want to add a new cn=hosts container just for hosts? Unless we
have a specific reason (i.e. use case) I don't see a need to add
specific code for hosts now, or at least until we get the core vault
functionality working.

The reason is consistency. Private vaults should be available for all
identities, because anything else would be an arbitrary limitation
(which is not elegant). If private vaults were available for all
identities, we would need a container for host vaults. I'm not saying
the container has to be added now, but there should at least be a check
to reject requests when the authenticated identity is a host (i.e.
context.principal.startswith('host/')).

In the previous patch, since a host was considered a service it could
have private vaults too. But anyway, I added the code to reject host
principals as you requested.

When is the self.api actually initialized? Can we initialize the
container_dn (or base_dn as in the original code) attribute
immediately
after that?

Not yet, but this will be fixed in the future. (Also, container_dn is
part of the LDAPObject API, unlike base_dn used in the original code.)

Is there a ticket for this?

I don't think there is a ticket for this particular issue.

Added TODOs in the code.

This change is not included. The code will now obtain the values from
apilib.api.env at init time and store it in class attributes so it can
be reused.

     container_dn = api.env.container_vault
     base_dn = DN(container_dn, api.env.basedn)

Sorry, but no. Please just follow the best practice instead of
trying to
invent something new. This is not the right time and place to discuss
this. We should be discussing the vault, not framework idiosyncracies.

OK.

Thanks for understanding.

Changed the code as requested.

There is an obvious inefficiency here: all containers in the path
(including the built-in ones) will be re-added on every vault-add
operation.

I don't see anything wrong with recursions, especially if it works more
efficiently since only the immediate parent will be re-added.

I tend to stick to the "don't use recursion in production code" rule,
epsecially in simple cases like this one.

I think the rule is kind of misguided. Recursion might be considered bad
if it goes very deep thus consuming so much stack space, which is not
the case here given the short and flat tree topology. Or, if it's
unnecessary such as a tail-recursion, which is not the case here either.

So for example, with the loop every time you add a private vault you're
guaranteed to have three failed LDAP Add operations whereas with the
recursion there's only one failed operation.

This is not about loop vs. recursion, this is about the used approach.
Your code can be easily transformed into a loop while keeping the same
approach:

     entries = []

     while dn:
         assert dn.endswith(self.base_dn)

         rdn = dn[0]
         entry = self.backend.make_entry(
             dn,
             {
                 'objectclass': ['nsContainer'],
                 'cn': rdn['cn'],
             })

         # if entry can be added, return
         try:
             self.backend.add_entry(entry)
             break

         except errors.NotFound:
             pass

         # otherwise, create parent entry first
         dn = DN(*dn[1:])
         entries.insert(0, entry)

     for entry in entries:
         # then create the entry itself again
         self.backend.add_entry(entry)


Do you still want to use the loop?

Yes please.

This algorithm is recursive by nature (start at the bottom, but add the
parent first). The above code basically emulates recursion with two
loops and an explicit stack of entries, but the memory requirement is
pretty much the same because it's still using a stack. With real
recursion there is no loops and the stack is implicit, so the code is at
least cleaner.

I thought you cared about efficiency in the first place, given our discussion about container_dn. The looped variant is faster, even when it has 2 for loops, and consumes less memory, because of the function call overhead in the recursive variant.


If you really want to avoid recursion you probably should use an RDN
iterator instead of a stack, but the code would be either even uglier or
less efficient.

Anyway, it's not a big deal, I included this change.


Thanks, LGTM.

Pushed to master: fde21adcbd62b9a300740d9ba237ca9e89a905e4

--
Jan Cholasta

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