On 14.5.2012 22:37, Jakub Hrozek wrote:
> On Mon, May 14, 2012 at 06:39:30PM +0200, Pavel Březina wrote:
>> On 9.5.2012 17:07, Pavel Březina wrote:
>>> On 05/07/2012 07:13 PM, Pavel Březina wrote:
>>>> On 04/23/2012 06:34 PM, Pavel Březina wrote:
>>>>> On 04/23/2012 05:04 PM, Pavel Březina wrote:
>>>>>> Hello everybody,
>>>>>> here is the first set of patches for the new sudo clothes. [1]
>>>>>>
>>>>>> As it does not touch the responder, I believe it does not
require
>>>>>> the
>>>>>> patches from the other preliminary sudo thread (already acked).
>>>>>> However
>>>>>> it was written atop them.
>>>>>>
>>>>>> [2/10]
>>>>>> This is the main change for the async processing of the sudo
>>>>>> rules. It
>>>>>> make sdap_sudo_refresh_send() more generic by adding there two
>>>>>> filters:
>>>>>> ldap_filter - used for search in the LDAP
>>>>>> sysdb_filter - used for a deletion of the rules from sysdb
>>>>>>
>>>>>> This way we can have many different refresh styles without
touching
>>>>>> this fundamental function.
>>>>>>
>>>>> > From this patch further the provider will return an error to
the
>>>>>> responder making it unusable.
>>>>>>
>>>>>> [...]
>>>>>> The rest of the patches has a self-describing subject. Don't
be
>>>>>> afraid
>>>>>> of the count, these patches are very small :-)
>>>>>>
>>>>>> The next set of patches is where the fun begin, so be patient
:-)
>>>>>>
>>>>>> [1]
https://fedorahosted.org/sssd/wiki/DesignDocs/SUDOCachingRules
>>>>>
>>>>> Ok, I made a mistake already in the 3rd patch, I'm sending it
>>>>> again :)
>>>>
>>>> Sending new set of patches if you want to take a look.
>>>>
>>>> Full periodical updates should run fine, there are preparations for
>>>> the
>>>> smart update. I'm going to ignore modifyTimestamp for now and use
only
>>>> entryUSN if available.
>>>>
>>>> It also does not filter the rules by host, I plan to add it as one of
>>>> the last things.
>>>
>>> The provider part is now complete except the above given limitations.
>>
>> Ok, the major changes are done in this patch set.
>>
>> What left is:
>> - download rules per host, this only appends a filter to the search so
>> it should not invoke any logic change
>>
>> - it ignores modifyTimestamp - simo suggested to use it in smart
>> refresh when enabled in configuration and USN not available, but do
>> we really want to use it? There is no other part in sssd that uses it
>> (or am I wrong?).
>>
>
> Pavel, I won't be able to review the huge patchset in a whole, so I
> started with the first couple of patches. Do you know offhand if there is
> a particular point where several patches could be applied and the
> provider
> would work so that when first N patches are acked, we can push them and
> decrease the amount of rebasing?
>
> 0001: sudo ldap provider: move async routines to sdap_async_sudo.c
> Ack, this patch just moves code around
>
> 0002: sudo ldap provider: give sdap_sudo_refresh_send() search and
> purge filters
> Ack. The sudo provider is not functional after this patch until patch #8,
> but I think that's OK. This split is as good as any.
>
> 0003: confdb: add entry_cache_sudo_timeout option
> Nack, the new option is missing from the configAPI
> Also, I assume the new option is going to be used when the responder
> changes land? So far, it's only read, not used.
Your assumption is correct.
Done. I have also fixed it in subsequent patches. I really need to
write it somewhere on my desk :-)
> 0004: sudo ldap provider: add sysdb ctx in sdap_sudo_refresh_state
> Ack, but I don't get the point?
>
> 0005: sudo ldap provider: add domain info in sdap_sudo_refresh_state
> Same as 0004, what's the intent?
Just to shorten access to these contexts.
> 0006: sudo ldap provider: add expiration time to each rule
> Ack
>
> 0007: sysdb_sudo_set_refresh_time
> Looks good, but do you anticipate that sysdb_sudo_{get,set}_refresh_time
> would be called with any other attribute name than
> SYSDB_SUDO_AT_LAST_FULL_REFRESH?
Originally it should contain SYSDB_SUDO_AT_LAST_SMART_REFRESH but I
found out later that it is not necessary.
The intention was to use that value to compute when the first smart
refresh should be scheduled after sssd is started. However, it needs to
know the current highest USN value (which would be a little tricky).
Therefore I'm doing full refresh at first and then enable periodical
smart updates (as we have discussed on site).
>
> 0008: sudo ldap provider: provide API for full refresh
> +
> + /* free filters with the request */
> + talloc_steal(subreq, ldap_filter);
> + talloc_steal(subreq, sysdb_filter);
> +
>
> Do the filters need to outlive the top-level request? It doesn't look
> like
> it's the case, but if so, then I would say it's more natural to strdup
> them
> in the subreq, so that other potential consumers of
> sdap_sudo_refresh_send()
> don't need to remember to steal the filters.
Well, you don't really need to steal them. But I don't have any use for
them outside the subreq so I think it is correct to make them
deallocate with subreq.
Those are constant input data, so there is no need to strdup them.
> _recv should come after _done in the source file, otherwise the code
> reads funny.
Ok. There would be unworthy rebasing troubles, thus I have moved it in
a new patch.
> In sdap_sudo_full_refresh_done(), you read the req and state variables
> twice
> in the patch, but not in the resulting code, so I assume it was fixed in
> a follow-up patch. Is it too much trouble to fix? If it is, don't bother.
This is an error from one of many rebases I have done.
Fixed.
> I think you meant to quit the request here with an error? If not, please
> put a comment saying that you ignore an error intentionally:
> + if (ret != EOK) {
> + DEBUG(SSSDBG_MINOR_FAILURE, ("Unable to save time of "
> + "a successful full refresh\n"));
> + }
I have added the following comment:
/* this is only a minor error that does not affect the functionality,
* therefore there is no need to report it with tevent_req_error()
* which would cause problems in the consumers */
> 0009: sudo ldap provider: add support for on demand full refresh
> + default:
> + DEBUG(SSSDBG_CRIT_FAILURE, ("Invalid request type: %d\n",
> + sudo_req->type));
> + }
> I think you should abort the request with something like EINVAL here.
> I know that the code would not continue because of the following (req
> != NULL)
> check but the OOM error would be pretty confusing for the caller.
Yes, definitely. The same problem was also in sdap_sudo_reply() where
ret would be uninitialized in ret != EOK condition.
Both fixed.
>
> 0010: provide API for refresh of specific rules
> Same comment with stealing the filters as with patch #9. Frankly, I don't
> really like how you wrapped sdap_sudo_refresh_send(). Wouldn't it be more
> readable to create both _done and _recv?
I found it necessary after all, because I needed to create there a new
request. It is converted to it in one of the last patches.
> 0011: sudo ldap provider: add support for on demand refresh of
> specific rules
> Ack
>
> I'm going to continue tomorrow. I also haven't done any testing yet, just
> reading the code.
I have also fixed typo in man page and added new patch that handles
modification of the highest USN value after a specific rules refresh.
New patches are attached.
Thanks for the review. Keep coming :-)
Rebased on the current master and patches from "sudo: send username and
uid while requesting default options" thread.