[Freeipa-devel] [PATCH] 664 entitlement support

[Rob Crittenden]

Jakub Hrozek wrote:
> On 02/01/2011 08:25 PM, Rob Crittenden wrote:
>> Jakub Hrozek wrote:
>>> -----BEGIN PGP SIGNED MESSAGE-----
>>> Hash: SHA1
>>>
>>> On 02/01/2011 04:15 PM, Rob Crittenden wrote:
>>>> Jakub Hrozek wrote:
>>>>> -----BEGIN PGP SIGNED MESSAGE-----
>>>>> Hash: SHA1
>>>>>
>>>>> On 01/31/2011 04:29 PM, Rob Crittenden wrote:
>>>>>> Jakub Hrozek wrote:
>>>>>>> -----BEGIN PGP SIGNED MESSAGE-----
>>>>>>> Hash: SHA1
>>>>>>>
>>>>>>> On 01/05/2011 04:38 PM, Rob Crittenden wrote:
>>>>>>>> This patch adds a plugin and tools for managing entitlements for
>>>>>>>> host
>>>>>>>> machines.
>>>>>>>>
>>>>>>>> Testing is rather complex so I've attached a script to help set
>>>>>>>> up the
>>>>>>>> Candlepin server. You'll need to ping me out of band for the
>>>>>>>> backend
>>>>>>>> data. This configures the Candlepin server with an in-memory
>>>>>>>> database so
>>>>>>>> any time tomcat6 is restarted you'll need to reload the data.
>>>>>>>>
>>>>>>>> You have to run candlepin.setup as root. This will configure your
>>>>>>>> Fedora
>>>>>>>> tomcat6 instance.
>>>>>>>>
>>>>>>>> Once your candlepin server is setup and IPA is installed do
>>>>>>>> something
>>>>>>>> like:
>>>>>>>>
>>>>>>>> $ ipa entitle-register admin
>>>>>>>> (password is admin)
>>>>>>>>
>>>>>>>> $ ipa entitle-consume 25
>>>>>>>>
>>>>>>>> $ ipa entitle-status
>>>>>>>> (verify that it is 25)
>>>>>>>>
>>>>>>>> # ipa-compliance
>>>>>>>> (should be 1 of 50)
>>>>>>>>
>>>>>>>> Our tools can consume only, not return entitlements.
>>>>>>>>
>>>>>>>> tickets 28, 79 and 278.
>>>>>>>>
>>>>>>>> rob
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>> can you rebase the patch so it applies cleanly on the current
>>>>>>> master?
>>>>>>
>>>>>> attached
>>>>>>
>>>>>> rob
>>>>>
>>>>> Functionally, the patch seems to be working fine -- great job!.
>>>>>
>>>>> I just have a couple of minor comments:
>>>>> * I think a recent change to delegation.ldif conflicts with the patch.
>>>>> I was able to do a 3-way merge, but please check it merges OK.
>>>>>
>>>>> * During build, rpm-build complains about /etc/cron.d/ipa-compliance
>>>>> being listed twice
>>>>>
>>>>> * the two commented lines in ipa-compliance that test Bind using DM
>>>>> and
>>>>> Bind using GSSAPI should be removed
>>>>>
>>>>> * I think that the ipa-compliance tool never deletes the directory
>>>>> with
>>>>> the ccache (tmpdir)
>>>>>
>>>>> * in ipa-compliance:
>>>>> + if not truncated:
>>>>> + hostcount = len(entries)
>>>>> + else:
>>>>> + # FIXME: raise an error
>>>>> + pass
>>>>> I'm not opposed to FIXMEs in the code, but maybe there should be a
>>>>> ticket so we don't forget them. Also, hostcount should be
>>>>> initialized in
>>>>> the else: branch, later on, the code accesses it and would blow up.
>>>>>
>>>>> * In the entitlement plugin, the 'hidden' attributes could have
>>>>> flags=['no_option', 'no_output'] so they don't show up in the UI
>>>>>
>>>>> * If I consume all the entitlements with ipa entitle-consume and ask
>>>>> for more, I get an internal server error - we should probably catch
>>>>> the
>>>>> RestlibException from candlepin
>>>>>
>>>>> * when I started testing I made a typo in the candlepin instance
>>>>> hostname. ipa entitle-register then blew up.. The traceback looks like
>>>>> it comes from rhsm. I don't think we absolutely need to fix it now,
>>>>> but
>>>>> we should at least track it in a ticket.
>>>>
>>>> Here is a diff of the changes you suggested, I think they cover all the
>>>> bases.
>>>>
>>>> rob
>>>
>>> Looks good, thank you. If you can send a new patch with these squashed
>>> in, I'll just run a couple of quick tests and ack.
>>> -----BEGIN PGP SIGNATURE-----
>>> Version: GnuPG v1.4.11 (GNU/Linux)
>>> Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
>>>
>>> iEYEARECAAYFAk1ISqEACgkQHsardTLnvCUQDgCfbHeiSCEhhyzepiEkr6Qp6S/W
>>> CtkAoKmz9r+b6bVck0Cviul4eiyskc0D
>>> =6Jh9
>>> -----END PGP SIGNATURE-----
>>
>> attached
>
> Ack but please check that the 3-way rebase is OK and also please import
> socket in ipalib/plugins/entitle.py, currently it is an undefined symbol.

Fixed, rebased and pushed to master. I also fixed up a couple of 
permissions, adding the ipapermission objectclass.

Thanks for the review, it is a relief to get this off my plate.

rob