[Freeipa-devel] [PATCHES 202-222] Ipaplatform refactoring
Tomas Babej
tbabej at redhat.com
Tue Jun 17 13:19:59 UTC 2014
On 06/17/2014 03:12 PM, Petr Spacek wrote:
> On 17.6.2014 14:50, Tomas Babej wrote:
>>
>> On 06/17/2014 02:44 PM, Petr Spacek wrote:
>>> On 17.6.2014 14:15, Tomas Babej wrote:
>>>>
>>>> On 06/17/2014 12:03 PM, Timo Aaltonen wrote:
>>>>> On 17.06.2014 11:16, Martin Kosek wrote:
>>>>>> On 06/16/2014 07:50 PM, Petr Viktorin wrote:
>>>>>>> On 06/16/2014 02:53 PM, Tomas Babej wrote:
>>>>>>>> On 06/10/2014 05:07 PM, Petr Viktorin wrote:
>>>>>>>>> On 06/10/2014 10:13 AM, Tomas Babej wrote:
>>>>>>>>>> On 06/06/2014 01:04 PM, Petr Viktorin wrote:
>>>>>>>>>>> On 06/05/2014 03:14 PM, Petr Viktorin wrote:
>>>>>>>>>>>> On 06/04/2014 11:42 AM, Tomas Babej wrote:
>>>>>>>>>>>>> Hi,
>>>>>>>>>>>>>
>>>>>>>>>>>>> the following set of patches implements the ticket:
>>>>>>>>>>>>>
>>>>>>>>>>>>> https://fedorahosted.org/freeipa/ticket/4052
>>>>>>>>>>>>>
>>>>>>>>> [...]
>>>>>>> 0202: OK
>>>>>>> 0203: OK
>>>>>>> 0204: OK
>>>>>>> 0205: OK
>>>>>>> 0206: OK
>>>>>>> 0207: OK
>>>>>>> (sorry for the conflict!)
>>>>>>>
>>>>>>> 0208: OK
>>>>>>> 0209: OK
>>>>>>> 0210: OK
>>>>>>> 0211: OK
>>>>>>> 0212: OK
>>>>>>> 0213: OK
>>>>>>> 0214: OK
>>>>>>> 0215: OK
>>>>>>> 0216: OK
>>>>>>> 0217: OK
>>>>>>> 0218: OK
>>>>>>> 0219: OK
>>>>>>> 0220: OK
>>>>>>> 0221: OK
>>>>>>> 0222: OK
>>>>>>>
>>>>>>> modify_nsswitch_pam_stack and modify_pam_to_use_krb5 are missing
>>>>>>> the `self`
>>>>>>> argument.
>>>>>>>
>>>>>>> Rebasing this all the time must be painful, so to avoid another
>>>>>>> review
>>>>>>> round-trip I've had Tomáš ACK the attached four-liner on IRC.
>>>>>> Thanks guys!
>>>>>>
>>>>>> I looked at the changes and have couple questions:
>>>>>>
>>>>>> 1) What is the motivation for keeping AuthConfig infrastructure
>>>>>> around? I
>>>>>> thought it is replaced by the tasks concept that are easier to
>>>>>> customize in
>>>>>> other platforms. IMO, it just obfuscates the process.
>>>>>>
>>>>>> How is
>>>>>> def modify_pam_to_use_krb5(self, statestore):
>>>>>> auth_config = FedoraAuthConfig()
>>>>>> statestore.backup_state('authconfig', 'krb5', True)
>>>>>> auth_config.enable("krb5")
>>>>>> auth_config.add_option("nostart")
>>>>>> auth_config.execute()
>>>>>> more readable than
>>>>>> def modify_pam_to_use_krb5(self, statestore):
>>>>>> statestore.backup_state('authconfig', 'krb5', True)
>>>>>> ipautil.run("authconfig --enablekrb5 --nostart")
>>>>>> ? And this was just the easy example. Also, documentation in
>>>>>> AuthConfig base
>>>>>> class refers to nonexistent paths.
>>>>>>
Also, regarding the Authconfig class, attached is a diff that refactors
a more complex method (than the one Martin used) to not use Authconfig
class. I can see some value in keeping it, as it makes the code somewhat
clearer (enable/disable methods as opposed to long-winded options).
However, at the very least we should remove the Authconfig class from
the base platform module.
Please look at the diff and speak out your opinions.
>>>>>> 2) There are still many non-converted paths in ipa-client
>>>>>> installers:
>>>>>>
>>>>>> $ git grep "bin/" ipa-client/
>>>>>> ...
>>>>>> ipa-client/ipa-install/ipa-client-install:SSH_AUTHORIZEDKEYSCOMMAND
>>>>>> =
>>>>>> '/usr/bin/sss_ssh_authorizedkeys'
>>>>>> ipa-client/ipa-install/ipa-client-install:SSH_PROXYCOMMAND =
>>>>>> '/usr/bin/sss_ssh_knownhostsproxy'
>>>>>> ipa-client/ipa-install/ipa-client-install: (sout, serr,
>>>>>> returncode) =
>>>>>> run(["/usr/bin/certutil", "-L", "-d", "/etc/pki/nssdb", "-n",
>>>>>> nickname],
>>>>>> raiseonerr=False)
>>>>>> ipa-client/ipa-install/ipa-client-install: run(["/usr/bin/certutil",
>>>>>> "-D", "-d", "/etc/pki/nssdb", "-n", "IPA CA"])
>>>>>> ipa-client/ipa-install/ipa-client-install: run(["/usr/bin/certutil",
>>>>>> "-D", "-d", "/etc/pki/nssdb", "-n", client_nss_nickname])
>>>>>> ...
>>>>>>
>>>>>> We should convert at least those as ipa-client will be the most
>>>>>> platformized
>>>>>> module (more than the server, IMO).
>>>>> and many others all over the place, just 'git grep /etc/'
>>>>>
>>>>> working on the debian client patches now..
>>>>>
>>>>>
>>>>
>>>> Attached is a new version of patch 226, and a new patch 228, which
>>>> moves
>>>> the paths from installers to the paths module.
>>>>
>>>> I greped the repository, and I do not see many paths lurking around
>>>> any
>>>> more, there are only some in the error messages (as these can't be
>>>> reliably replaced automatically, and will need some manual love).
>>> I don't know the code but why it is not possible to use
>>> "%s" % (paths.blahblah) ?
>>>
>>> IMHO paths should never be hardcoded in strings/messages shown to user.
>>>
>>> Petr^2 Spacek
>>
>> Of course they can, they just cannot be replaced by the tool I developed
>> for this refactoring, and need to be replaced manually, as the tool
>> lacks the capability.
>>
>> They need to be replaced manually.
>
> Ah, sure. I didn't notice you were speaking about automatic
> replacement. I'm sorry!
>
> Petr^2 Spacek
>
>>
>>>
>>>> If you see any forgotten paths, which should be added to the
>>>> module, let
>>>> me know.
--
Tomas Babej
Associate Software Engineer | Red Hat | Identity Management
RHCE | Brno Site | IRC: tbabej | freeipa.org
-------------- next part --------------
diff --git a/ipaplatform/fedora/tasks.py b/ipaplatform/fedora/tasks.py
index c20ecd3..54d97f1 100644
--- a/ipaplatform/fedora/tasks.py
+++ b/ipaplatform/fedora/tasks.py
@@ -92,7 +92,8 @@ class FedoraTaskNamespace(BaseTaskNamespace):
was_sssd_installed,
was_sssd_configured):
- auth_config = FedoraAuthConfig()
+ args = []
+
if statestore.has_state('authconfig'):
# disable only those configurations that we enabled during install
for conf in ('ldap', 'krb5', 'sssd', 'sssdauth', 'mkhomedir'):
@@ -101,20 +102,20 @@ class FedoraTaskNamespace(BaseTaskNamespace):
# uses. Remove it from statestore however, so that it becomes
# empty at the end of uninstall process.
if cnf and conf != 'sssd':
- auth_config.disable(conf)
+ args.append('--disable%s' % conf)
else:
# There was no authconfig status store
# It means the code was upgraded after original install
# Fall back to old logic
- auth_config.disable("ldap")
- auth_config.disable("krb5")
+ args.append('--disableldap')
+ args.append('--disablekrb5')
if not(was_sssd_installed and was_sssd_configured):
# Only disable sssdauth. Disabling sssd would cause issues
# with its later uses.
- auth_config.disable("sssdauth")
- auth_config.disable("mkhomedir")
+ args.append("--disablesssdauth")
+ args.append("--disablemkhomedir")
- auth_config.execute()
+ ipautil.run([paths.AUTHCONFIG] + args + ['--update'])
def set_nisdomain(self, nisdomain):
# Let authconfig setup the permanent configuration
More information about the Freeipa-devel
mailing list