[Freeipa-devel] [Test][patch-0053] Forced-client-reenrollment test fixed.

[Martin Babinsky]

On 07/26/2016 03:34 PM, Oleg Fayans wrote:
> Hi Martin,
>
> The patch was updated according to your suggestions. A separate patch
> removing outdated tests is attached.
>
> On 07/08/2016 02:10 PM, Martin Basti wrote:
>>
>>
>> On 07.07.2016 08:09, Oleg Fayans wrote:
>>> Updated version of the patch is attached with the failing tests marked
>>> as xfailed (let's make the jenkins green).
>>>
>>> On 07/04/2016 10:50 PM, Oleg Fayans wrote:
>>>> 2 out of 7 tests currently fail due to a known issue [1], others pass.
>>>>
>>>> [1] https://fedorahosted.org/freeipa/ticket/6029
>>>>
>>>>
>>>>
>>>>
>>>
>>>
>>>
>> This is wrong:
>>
>> 1)
>> you are not getting SSHFP records, just SSH public key (with your
>> changes)
>>
>> 2)
>> you are using host-find without any arguments, so it will returns SSH
>> key for all hosts, the code before was getting SSHFP only for one host.
>> Would be better to use host-show?
>>
>> 3)
>> you actually found a bug, because host-find and host-show should print
>> only SSH fingerprints not SSH keys
>> https://fedorahosted.org/freeipa/ticket/6042
>> https://fedorahosted.org/freeipa/ticket/6043
>>
>> 4)
>> don't call it SSHFP records in code, because it is not DNS related,
>> probably you want to get SSH fingerprints instead of SSH keys
>>
>> 5)
>> It may contain multiple SSH keys, you always return only the first (the
>> original code returns all values)
>>
>>      def get_sshfp_record(self):
>> -        sshfp_record = ''
>> -        client_host = self.clients[0].hostname.split('.')[0]
>> -
>>           result = self.master.run_command(
>> -            ['ipa', 'dnsrecord-show', self.master.domain.name,
>> client_host]
>> +            ['ipa', 'host-find']
>>           )
>> -
>> -        lines = result.stdout_text.splitlines()
>> -        for line in lines:
>> -            if 'SSHFP record:' in line:
>> -                sshfp_record = line.replace('SSHFP record:', '').strip()
>> -
>> -        assert sshfp_record, 'SSHFP record not found'
>> -
>> -        sshfp_record = set(sshfp_record.split(', '))
>> -        self.log.debug("SSHFP record for host %s: %s", client_host,
>> str(sshfp_record))
>> -
>> -        return sshfp_record
>> +        records = result.stdout_text.split('\n\n')
>> +        sshkey_re = re.compile('.+SSH public key: ssh-\w+ (\S+?),.+')
>> +        for hostrecord in records:
>> +            if self.clients[0].hostname in hostrecord:
>> +                sshfps = sshkey_re.findall(hostrecord)
>> +                assert sshfps, 'SSHFP record not found'
>> +                sshfp = sshfps[0]
>> +        return sshfp
>>
>>
>
>
>

Oleg,

the original forced client reenrollment suite passes for me:

"""
=========================================== 8 passed in 1859.52 seconds 
============================================
"""

I am not quite sure what are you trying to accomplish with these 
patches, since you are deleting valid test cases and then removing 
'restore_client' method that simulates the use case of e.g. removing a 
client VM while keeping old keytab and host entry and then re-enrolling.

The commit messages did not help very much, either.

Please review the original design and test cases carefully and make sure 
you understand why is the test constructed the way it is:

http://www.freeipa.org/page/V3/Forced_client_re-enrollment

NACK until you clearly state the purpose of your patches.

-- 
Martin^3 Babinsky