Re: [Freeipa-devel] [PATCHES 466-468] install: Add common base class for server and replica install

[Martin Babinsky]

On 09/16/2015 10:44 AM, Jan Cholasta wrote:

On 16.9.2015 08:11, Jan Cholasta wrote:

On 15.9.2015 07:22, Jan Cholasta wrote:

On 10.8.2015 16:58, Martin Babinsky wrote:

On 08/06/2015 08:22 AM, Jan Cholasta wrote:

Hi,

the attached patch fixes part of
.

See also Martin Babinsky's patch 51:
.





Honza



Sorry but NACK, see below:

1.) it seems that passing kwargs to Server components doesn't work as
expected. See these logs (install on fresh F22 VM):

http://fpaste.org/253416/21363814/
http://fpaste.org/253419/43921374/


Fixed.



2.) the following code blows up in BaseServers' __init__:
(http://fpaste.org/253400/21225314/)

392 if not self.dns.setup_dns:
393 if self.dns.forwarders:
394 raise RuntimeError(
395 "You cannot specify a --forwarder option
without
the "
396 "--setup-dns option")


I think that the check should be:

392 if not self.setup_dns:
393 if self.dns.forwarders:


Fixed.



IMHO BaseServerDNS class shouldn't have setup_dns knob, that should be
set in the parent class (BaseServer)


Fixed.



3.) Is there any reason why BaseServer doesn't have 'master_password',
'idmax' and 'idstart' knobs? I know that these are then brought in by
the derived Server class, but the check for them is in parent's
__init__() method and it is IMHO a bit confusing


The check should be in Server, fixed.



4.) please add license header to the beginning of
'ipaserver/install/server/common.py' file


Added.

Updated patches attached.


Self-NACK, I broke ipa-server-install --uninstall.


Fixed.



ACK to all three patches.

--
Martin^3 Babinsky

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

Re: [Freeipa-devel] [PATCHES 466-468] install: Add common base class for server and replica install

[Jan Cholasta]

On 22.9.2015 10:29, Martin Babinsky wrote:

On 09/16/2015 10:44 AM, Jan Cholasta wrote:

On 16.9.2015 08:11, Jan Cholasta wrote:

On 15.9.2015 07:22, Jan Cholasta wrote:

On 10.8.2015 16:58, Martin Babinsky wrote:

On 08/06/2015 08:22 AM, Jan Cholasta wrote:

Hi,

the attached patch fixes part of
.

See also Martin Babinsky's patch 51:
.






Honza



Sorry but NACK, see below:

1.) it seems that passing kwargs to Server components doesn't work as
expected. See these logs (install on fresh F22 VM):

http://fpaste.org/253416/21363814/
http://fpaste.org/253419/43921374/


Fixed.



2.) the following code blows up in BaseServers' __init__:
(http://fpaste.org/253400/21225314/)

392 if not self.dns.setup_dns:
393 if self.dns.forwarders:
394 raise RuntimeError(
395 "You cannot specify a --forwarder option
without
the "
396 "--setup-dns option")


I think that the check should be:

392 if not self.setup_dns:
393 if self.dns.forwarders:


Fixed.



IMHO BaseServerDNS class shouldn't have setup_dns knob, that should be
set in the parent class (BaseServer)


Fixed.



3.) Is there any reason why BaseServer doesn't have 'master_password',
'idmax' and 'idstart' knobs? I know that these are then brought in by
the derived Server class, but the check for them is in parent's
__init__() method and it is IMHO a bit confusing


The check should be in Server, fixed.



4.) please add license header to the beginning of
'ipaserver/install/server/common.py' file


Added.

Updated patches attached.


Self-NACK, I broke ipa-server-install --uninstall.


Fixed.



ACK to all three patches.



Thanks.

Pushed to:
master: 86edd6abeb9749e159a529b83cfce6443fff4ba5
ipa-4-2: 42d16b02cd153ac89ebd8ae07c98611dc3b6e471

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

Re: [Freeipa-devel] [PATCHES 466-468] install: Add common base class for server and replica install

[Jan Cholasta]

On 15.9.2015 07:22, Jan Cholasta wrote:

On 10.8.2015 16:58, Martin Babinsky wrote:

On 08/06/2015 08:22 AM, Jan Cholasta wrote:

Hi,

the attached patch fixes part of
.

See also Martin Babinsky's patch 51:
.



Honza



Sorry but NACK, see below:

1.) it seems that passing kwargs to Server components doesn't work as
expected. See these logs (install on fresh F22 VM):

http://fpaste.org/253416/21363814/
http://fpaste.org/253419/43921374/


Fixed.



2.) the following code blows up in BaseServers' __init__:
(http://fpaste.org/253400/21225314/)

392 if not self.dns.setup_dns:
393 if self.dns.forwarders:
394 raise RuntimeError(
395 "You cannot specify a --forwarder option without
the "
396 "--setup-dns option")


I think that the check should be:

392 if not self.setup_dns:
393 if self.dns.forwarders:


Fixed.



IMHO BaseServerDNS class shouldn't have setup_dns knob, that should be
set in the parent class (BaseServer)


Fixed.



3.) Is there any reason why BaseServer doesn't have 'master_password',
'idmax' and 'idstart' knobs? I know that these are then brought in by
the derived Server class, but the check for them is in parent's
__init__() method and it is IMHO a bit confusing


The check should be in Server, fixed.



4.) please add license header to the beginning of
'ipaserver/install/server/common.py' file


Added.

Updated patches attached.


Self-NACK, I broke ipa-server-install --uninstall.

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

Re: [Freeipa-devel] [PATCHES 466-468] install: Add common base class for server and replica install

[Jan Cholasta]

On 16.9.2015 08:11, Jan Cholasta wrote:

On 15.9.2015 07:22, Jan Cholasta wrote:

On 10.8.2015 16:58, Martin Babinsky wrote:

On 08/06/2015 08:22 AM, Jan Cholasta wrote:

Hi,

the attached patch fixes part of
.

See also Martin Babinsky's patch 51:
.




Honza



Sorry but NACK, see below:

1.) it seems that passing kwargs to Server components doesn't work as
expected. See these logs (install on fresh F22 VM):

http://fpaste.org/253416/21363814/
http://fpaste.org/253419/43921374/


Fixed.



2.) the following code blows up in BaseServers' __init__:
(http://fpaste.org/253400/21225314/)

392 if not self.dns.setup_dns:
393 if self.dns.forwarders:
394 raise RuntimeError(
395 "You cannot specify a --forwarder option without
the "
396 "--setup-dns option")


I think that the check should be:

392 if not self.setup_dns:
393 if self.dns.forwarders:


Fixed.



IMHO BaseServerDNS class shouldn't have setup_dns knob, that should be
set in the parent class (BaseServer)


Fixed.



3.) Is there any reason why BaseServer doesn't have 'master_password',
'idmax' and 'idstart' knobs? I know that these are then brought in by
the derived Server class, but the check for them is in parent's
__init__() method and it is IMHO a bit confusing


The check should be in Server, fixed.



4.) please add license header to the beginning of
'ipaserver/install/server/common.py' file


Added.

Updated patches attached.


Self-NACK, I broke ipa-server-install --uninstall.


Fixed.

--
Jan Cholasta
From 414f92cf8b820f34ffc336a604a947f7de8fc558 Mon Sep 17 00:00:00 2001
From: Jan Cholasta 
Date: Thu, 6 Aug 2015 08:16:22 +0200
Subject: [PATCH 2/3] install: Add common base class for server and replica
 install

https://fedorahosted.org/freeipa/ticket/4517
---
 ipaserver/install/server/common.py | 454 +
 ipaserver/install/server/install.py| 411 ++
 ipaserver/install/server/replicainstall.py | 224 --
 3 files changed, 525 insertions(+), 564 deletions(-)
 create mode 100644 ipaserver/install/server/common.py

diff --git a/ipaserver/install/server/common.py b/ipaserver/install/server/common.py
new file mode 100644
index 000..e7fb2ac
--- /dev/null
+++ b/ipaserver/install/server/common.py
@@ -0,0 +1,454 @@
+#
+# Copyright (C) 2015  FreeIPA Contributors see COPYING for license
+#
+
+import os
+import sys
+
+from ipapython.dn import DN
+from ipapython.install import common, core
+from ipapython.install.core import Knob
+from ipalib.util import validate_domain_name
+from ipaserver.install import bindinstance
+
+VALID_SUBJECT_ATTRS = ['st', 'o', 'ou', 'dnqualifier', 'c',
+   'serialnumber', 'l', 'title', 'sn', 'givenname',
+   'initials', 'generationqualifier', 'dc', 'mail',
+   'uid', 'postaladdress', 'postalcode', 'postofficebox',
+   'houseidentifier', 'e', 'street', 'pseudonym',
+   'incorporationlocality', 'incorporationstate',
+   'incorporationcountry', 'businesscategory']
+
+
+class BaseServerCA(common.Installable, core.Group, core.Composite):
+description = "certificate system"
+
+external_ca = Knob(
+bool, False,
+description=("Generate a CSR for the IPA CA certificate to be signed "
+ "by an external CA"),
+)
+
+external_ca_type = Knob(
+{'generic', 'ms-cs'}, None,
+description="Type of the external CA",
+)
+
+external_cert_files = Knob(
+(list, str), None,
+description=("File containing the IPA CA certificate and the external "
+ "CA certificate chain"),
+cli_name='external-cert-file',
+cli_aliases=['external_cert_file', 'external_ca_file'],
+cli_metavar='FILE',
+)
+
+@external_cert_files.validator
+def external_cert_files(self, value):
+if any(not os.path.isabs(path) for path in value):
+raise ValueError("must use an absolute path")
+
+dirsrv_cert_files = Knob(
+(list, str), None,
+description=("File containing the Directory Server SSL certificate "
+ "and private key"),
+cli_name='dirsrv-cert-file',
+cli_aliases=['dirsrv_pkcs12'],
+cli_metavar='FILE',
+)
+
+http_cert_files = Knob(
+(list, str), None,
+description=("File containing the Apache Server SSL certificate and "
+ "private key"),
+cli_name='http-cert-file',
+cli_aliases=['http_pkcs12'],
+cli_metavar='FILE',
+)
+
+pkinit_cert_files = Knob(
+(list, str), None,
+description=("File containing the Kerberos KDC SSL certificate and "
+

Re: [Freeipa-devel] [PATCHES 466-468] install: Add common base class for server and replica install

[Jan Cholasta]

On 10.8.2015 16:58, Martin Babinsky wrote:

On 08/06/2015 08:22 AM, Jan Cholasta wrote:

Hi,

the attached patch fixes part of
.

See also Martin Babinsky's patch 51:
.


Honza



Sorry but NACK, see below:

1.) it seems that passing kwargs to Server components doesn't work as
expected. See these logs (install on fresh F22 VM):

http://fpaste.org/253416/21363814/
http://fpaste.org/253419/43921374/


Fixed.



2.) the following code blows up in BaseServers' __init__:
(http://fpaste.org/253400/21225314/)

392 if not self.dns.setup_dns:
393 if self.dns.forwarders:
394 raise RuntimeError(
395 "You cannot specify a --forwarder option without
the "
396 "--setup-dns option")


I think that the check should be:

392 if not self.setup_dns:
393 if self.dns.forwarders:


Fixed.



IMHO BaseServerDNS class shouldn't have setup_dns knob, that should be
set in the parent class (BaseServer)


Fixed.



3.) Is there any reason why BaseServer doesn't have 'master_password',
'idmax' and 'idstart' knobs? I know that these are then brought in by
the derived Server class, but the check for them is in parent's
__init__() method and it is IMHO a bit confusing


The check should be in Server, fixed.



4.) please add license header to the beginning of
'ipaserver/install/server/common.py' file


Added.

Updated patches attached.

--
Jan Cholasta
From 5a86a592fca3cee57283de7b3adc41baad64f7c5 Mon Sep 17 00:00:00 2001
From: Jan Cholasta 
Date: Thu, 6 Aug 2015 08:14:17 +0200
Subject: [PATCH 1/3] install: Support overriding knobs in subclasses

https://fedorahosted.org/freeipa/ticket/4517
---
 ipapython/install/cli.py  |   2 -
 ipapython/install/core.py | 216 ++
 2 files changed, 124 insertions(+), 94 deletions(-)

diff --git a/ipapython/install/cli.py b/ipapython/install/cli.py
index 54ca58d..a9ebe36 100644
--- a/ipapython/install/cli.py
+++ b/ipapython/install/cli.py
@@ -86,8 +86,6 @@ class ConfigureTool(admintool.AdminTool):
 transformed_cls = cls._transform(cls.configurable_class)
 for owner_cls, name in transformed_cls.knobs():
 knob_cls = getattr(owner_cls, name)
-if not knob_cls.initializable:
-continue
 if cls.positional_arguments and name in cls.positional_arguments:
 continue
 
diff --git a/ipapython/install/core.py b/ipapython/install/core.py
index 45f7aef..479149b 100644
--- a/ipapython/install/core.py
+++ b/ipapython/install/core.py
@@ -6,9 +6,11 @@
 The framework core.
 """
 
-import sys
 import abc
+import collections
+import functools
 import itertools
+import sys
 
 import six
 
@@ -33,7 +35,8 @@ _missing = object()
 _counter = itertools.count()
 
 
-def _class_cmp(a, b):
+@functools.cmp_to_key
+def _class_key(a, b):
 if a is b:
 return 0
 elif issubclass(a, b):
@@ -54,26 +57,44 @@ class KnobValueError(ValueError):
 self.name = name
 
 
-class InnerClass(six.with_metaclass(util.InnerClassMeta, object)):
+class PropertyBase(six.with_metaclass(util.InnerClassMeta, object)):
+# shut up pylint
 __outer_class__ = None
 __outer_name__ = None
 
+_order = None
 
-class PropertyBase(InnerClass):
 @property
 def default(self):
 raise AttributeError('default')
 
 def __init__(self, outer):
-self.outer = outer
+pass
 
 def __get__(self, obj, obj_type):
+while obj is not None:
+try:
+return obj.__dict__[self.__outer_name__]
+except KeyError:
+pass
+obj = obj._get_fallback()
+
 try:
-return obj._get_property(self.__outer_name__)
-except AttributeError:
-if not hasattr(self, 'default'):
-raise
 return self.default
+except AttributeError:
+raise AttributeError(self.__outer_name__)
+
+def __set__(self, obj, value):
+try:
+obj.__dict__[self.__outer_name__] = value
+except KeyError:
+raise AttributeError(self.__outer_name__)
+
+def __delete__(self, obj):
+try:
+del obj.__dict__[self.__outer_name__]
+except KeyError:
+raise AttributeError(self.__outer_name__)
 
 
 def Property(default=_missing):
@@ -86,7 +107,6 @@ def Property(default=_missing):
 
 class KnobBase(PropertyBase):
 type = None
-initializable = True
 sensitive = False
 deprecated = False
 description = None
@@ -95,23 +115,8 @@ class KnobBase(PropertyBase):
 cli_aliases = None
 cli_metavar = None
 
-_order = None
-
-def __set__(self, obj, value):
-try:
-self.validate(value)
-except KnobValueError:
-raise
-

Re: [Freeipa-devel] [PATCHES 466-468] install: Add common base class for server and replica install

[Martin Babinsky]

On 08/06/2015 08:22 AM, Jan Cholasta wrote:

Hi,

the attached patch fixes part of
https://fedorahosted.org/freeipa/ticket/4517.

See also Martin Babinsky's patch 51:
https://www.redhat.com/archives/freeipa-devel/2015-August/msg00012.html.

Honza



Sorry but NACK, see below:

1.) it seems that passing kwargs to Server components doesn't work as 
expected. See these logs (install on fresh F22 VM):


http://fpaste.org/253416/21363814/
http://fpaste.org/253419/43921374/

2.) the following code blows up in BaseServers' __init__:
(http://fpaste.org/253400/21225314/)

392 if not self.dns.setup_dns:
393 if self.dns.forwarders:
394 raise RuntimeError(
395 You cannot specify a --forwarder option without 
the 

396 --setup-dns option)


I think that the check should be:

392 if not self.setup_dns:
393 if self.dns.forwarders:

IMHO BaseServerDNS class shouldn't have setup_dns knob, that should be 
set in the parent class (BaseServer)


3.) Is there any reason why BaseServer doesn't have 'master_password', 
'idmax' and 'idstart' knobs? I know that these are then brought in by 
the derived Server class, but the check for them is in parent's 
__init__() method and it is IMHO a bit confusing


4.) please add license header to the beginning of 
'ipaserver/install/server/common.py' file


--
Martin^3 Babinsky

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

[Freeipa-devel] [PATCHES 466-468] install: Add common base class for server and replica install

[Jan Cholasta]

Hi,

the attached patch fixes part of 
https://fedorahosted.org/freeipa/ticket/4517.


See also Martin Babinsky's patch 51: 
https://www.redhat.com/archives/freeipa-devel/2015-August/msg00012.html.


Honza

--
Jan Cholasta
From f8c2de11794777f406fec377ed404108cdce1655 Mon Sep 17 00:00:00 2001
From: Jan Cholasta jchol...@redhat.com
Date: Thu, 6 Aug 2015 08:14:17 +0200
Subject: [PATCH 1/3] install: Support overriding knobs in subclasses

https://fedorahosted.org/freeipa/ticket/4517
---
 ipapython/install/cli.py  |   2 -
 ipapython/install/core.py | 203 ++
 2 files changed, 117 insertions(+), 88 deletions(-)

diff --git a/ipapython/install/cli.py b/ipapython/install/cli.py
index 1ba9a81..5ee445d 100644
--- a/ipapython/install/cli.py
+++ b/ipapython/install/cli.py
@@ -86,8 +86,6 @@ class ConfigureTool(admintool.AdminTool):
 transformed_cls = cls._transform(cls.configurable_class)
 for owner_cls, name in transformed_cls.knobs():
 knob_cls = getattr(owner_cls, name)
-if not knob_cls.initializable:
-continue
 if cls.positional_arguments and name in cls.positional_arguments:
 continue
 
diff --git a/ipapython/install/core.py b/ipapython/install/core.py
index c313c27..3af9371 100644
--- a/ipapython/install/core.py
+++ b/ipapython/install/core.py
@@ -6,9 +6,11 @@
 The framework core.
 
 
-import sys
 import abc
+import collections
+import functools
 import itertools
+import sys
 
 from ipapython.ipa_log_manager import root_logger
 
@@ -31,7 +33,8 @@ _missing = object()
 _counter = itertools.count()
 
 
-def _class_cmp(a, b):
+@functools.cmp_to_key
+def _class_key(a, b):
 if a is b:
 return 0
 elif issubclass(a, b):
@@ -52,27 +55,48 @@ class KnobValueError(ValueError):
 self.name = name
 
 
-class InnerClass(object):
+class AttributeBase(object):
 __metaclass__ = util.InnerClassMeta
+
+# shut up pylint
 __outer_class__ = None
 __outer_name__ = None
 
-
-class PropertyBase(InnerClass):
 @property
 def default(self):
 raise AttributeError('default')
 
 def __init__(self, outer):
-self.outer = outer
+pass
 
 def __get__(self, obj, obj_type):
+while obj is not None:
+try:
+return obj.__dict__[self.__outer_name__]
+except KeyError:
+pass
+obj = obj._get_fallback()
+
 try:
-return obj._get_property(self.__outer_name__)
-except AttributeError:
-if not hasattr(self, 'default'):
-raise
 return self.default
+except AttributeError:
+raise AttributeError(self.__outer_name__)
+
+def __set__(self, obj, value):
+try:
+obj.__dict__[self.__outer_name__] = value
+except KeyError:
+raise AttributeError(self.__outer_name__)
+
+def __delete__(self, obj):
+try:
+del obj.__dict__[self.__outer_name__]
+except KeyError:
+raise AttributeError(self.__outer_name__)
+
+
+class PropertyBase(AttributeBase):
+pass
 
 
 def Property(default=_missing):
@@ -83,9 +107,8 @@ def Property(default=_missing):
 return util.InnerClassMeta('Property', (PropertyBase,), class_dict)
 
 
-class KnobBase(PropertyBase):
+class KnobBase(AttributeBase):
 type = None
-initializable = True
 sensitive = False
 deprecated = False
 description = None
@@ -96,21 +119,8 @@ class KnobBase(PropertyBase):
 
 _order = None
 
-def __set__(self, obj, value):
-try:
-self.validate(value)
-except KnobValueError:
-raise
-except ValueError as e:
-raise KnobValueError(self.__outer_name__, str(e))
-
-obj.__dict__[self.__outer_name__] = value
-
-def __delete__(self, obj):
-try:
-del obj.__dict__[self.__outer_name__]
-except KeyError:
-raise AttributeError(self.__outer_name__)
+def __init__(self, outer):
+self.outer = outer
 
 def validate(self, value):
 pass
@@ -134,12 +144,17 @@ class KnobBase(PropertyBase):
 return cls
 
 
-def Knob(type, default=_missing, initializable=_missing, sensitive=_missing,
+def Knob(type_or_base, default=_missing, sensitive=_missing,
  deprecated=_missing, description=_missing, cli_name=_missing,
  cli_short_name=_missing, cli_aliases=_missing, cli_metavar=_missing):
 class_dict = {}
 class_dict['_order'] = next(_counter)
-class_dict['type'] = type
+
+if (not isinstance(type_or_base, type) or
+not issubclass(type_or_base, KnobBase)):
+class_dict['type'] = type_or_base
+type_or_base = KnobBase
+
 if default is not _missing:
 class_dict['default'] = default
 if sensitive is not _missing:
@@ -157,7 +172,7 @@ def Knob(type, default=_missing,