Installer refactoring #232
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Removed a redundant restart in server install which was there only so other methods of dsinstance would not fail as they would use the wrong connection mentioned above. https://fedorahosted.org/freeipa/ticket/6392 Reviewed-By: Martin Babinsky <mbabinsk@redhat.com> Reviewed-By: Jan Cholasta <jcholast@redhat.com>
Replica populate can be applied with other update plugins. https://fedorahosted.org/freeipa/ticket/6392 Reviewed-By: Martin Babinsky <mbabinsk@redhat.com> Reviewed-By: Jan Cholasta <jcholast@redhat.com>
To make the code more general, moved the update_dna_shared_config among other update plugins. Bugfix: DNA shared config connection protocol was compared to a method string which would result in a try to always update it even if there was no need to. https://fedorahosted.org/389/ticket/48373 causes that two shared DNA config entries are created instead of one. https://fedorahosted.org/freeipa/ticket/6392 Reviewed-By: Martin Babinsky <mbabinsk@redhat.com> Reviewed-By: Jan Cholasta <jcholast@redhat.com>
`create_from_pkcs12` method of CertDB was re-creating NSS database files during PKCS#12 bundle import. This may cause bugs because the file permissions could be re-set to wrong values causing subtle bugs. Modify the class API so that the server cert chain can be imported while preserving existing FS attributes. https://fedorahosted.org/freeipa/ticket/6429 Reviewed-By: Jan Cholasta <jcholast@redhat.com>
In order to reduce coupling between httpinstance and other service installers, the HTTP installer is now tasked with initialization of /etc/httpd/alias (RA agent database) in the beginning of server/replica installation Part of https://fedorahosted.org/freeipa/ticket/6429 Reviewed-By: Jan Cholasta <jcholast@redhat.com>
Remote master and CA host names may differ. Always use the remote CA host name and never the remote master host name in CA replica install. https://fedorahosted.org/freeipa/ticket/6392 Reviewed-By: Stanislav Laznicka <slaznick@redhat.com>
Remote master and KRA host names may differ. Always use the remote KRA host name and never the remote master host name in KRA replica install. https://fedorahosted.org/freeipa/ticket/6392 Reviewed-By: Stanislav Laznicka <slaznick@redhat.com>
Merge CA install code paths use in ipa-server-install, ipa-replica-install in either domain level and ipa-ca-install into one. https://fedorahosted.org/freeipa/ticket/6392 Reviewed-By: Stanislav Laznicka <slaznick@redhat.com>
Merge KRA install code paths use in ipa-replica-install in either domain level and ipa-kra-install into one. https://fedorahosted.org/freeipa/ticket/6392 Reviewed-By: Stanislav Laznicka <slaznick@redhat.com>
This commit only moves the code from ipa-client-install to module ipaclient/install/client.py and fixes PEP8. https://fedorahosted.org/freeipa/ticket/6392 Reviewed-By: Stanislav Laznicka <slaznick@redhat.com>
Function configure_krb5_conf always returns 0 as return state. Remove the 'return' statement and let exceptions work https://fedorahosted.org/freeipa/ticket/6392 Reviewed-By: Stanislav Laznicka <slaznick@redhat.com>
Function always returns return code 0, and this code is even not used elsewehere. https://fedorahosted.org/freeipa/ticket/6392 Reviewed-By: Stanislav Laznicka <slaznick@redhat.com>
https://fedorahosted.org/freeipa/ticket/6392 Reviewed-By: Stanislav Laznicka <slaznick@redhat.com>
https://fedorahosted.org/freeipa/ticket/6392 Reviewed-By: Stanislav Laznicka <slaznick@redhat.com>
We should use as specific import as possible, better for python memory consumption and speed, and looks better in code. https://fedorahosted.org/freeipa/ticket/6392 Reviewed-By: Stanislav Laznicka <slaznick@redhat.com>
Instead of copy&paste is better to use constant. It makes code shorter and improves readability, saves resources. https://fedorahosted.org/freeipa/ticket/6392 Reviewed-By: Stanislav Laznicka <slaznick@redhat.com>
At this point, httpd is not configured and the restart fails. https://fedorahosted.org/freeipa/ticket/6392 Reviewed-By: Stanislav Laznicka <slaznick@redhat.com>
https://fedorahosted.org/freeipa/ticket/6392 Reviewed-By: Martin Basti <mbasti@redhat.com>
https://fedorahosted.org/freeipa/ticket/6392 Reviewed-By: Martin Basti <mbasti@redhat.com>
Merge all RA cert import code paths into a single code path in CA install. https://fedorahosted.org/freeipa/ticket/6392 Reviewed-By: Stanislav Laznicka <slaznick@redhat.com>
Merge all KRA agent cert export code paths into a single code path in KRA install. https://fedorahosted.org/freeipa/ticket/6392 Reviewed-By: Stanislav Laznicka <slaznick@redhat.com>
`CertDB.request_service_cert` could re-create NSSDB files if the supplied CA certificate was not found in database. This could cause subtle bugs since the files were recreated with wrong permissions. This behavior was removed so that there are no destructive operations performed by the method. https://fedorahosted.org/freeipa/ticket/6429 Reviewed-By: Stanislav Laznicka <slaznick@redhat.com>
This functionality will be reused in the DL0 host enrollment https://fedorahosted.org/freeipa/ticket/6434 Reviewed-By: Stanislav Laznicka <slaznick@redhat.com> Reviewed-By: Jan Cholasta <jcholast@redhat.com>
In order to unify domain-level specific replica installers to a single workflow some kind of host enrollment must be done also in domain level 0 replica installation. Here the enrollment is done by directory manager using one-time password and only krb5.conf is configured to point to master KDC. Since host keytab is fetched during enrollment KDC installer no longer needs to request it during replica install. https://fedorahosted.org/freeipa/ticket/6434 Reviewed-By: Stanislav Laznicka <slaznick@redhat.com> Reviewed-By: Jan Cholasta <jcholast@redhat.com>
https://fedorahosted.org/freeipa/ticket/6434 Reviewed-By: Stanislav Laznicka <slaznick@redhat.com>
There should not be mixed statestore as global variable and as local function parameter. This commit fixes usage of sysrestore and statestore as local variables only. In future we may need to change default statestore and fstore depending on where the functions are called and this change makes it easier and less error prone. https://fedorahosted.org/freeipa/ticket/6392 Reviewed-By: Stanislav Laznicka <slaznick@redhat.com>
Move checks from ipa-client-install to clien.install_check https://fedorahosted.org/freeipa/ticket/6392 Reviewed-By: Stanislav Laznicka <slaznick@redhat.com>
client install contained installation check that have been moved to install_check function https://fedorahosted.org/freeipa/ticket/6392 Reviewed-By: Stanislav Laznicka <slaznick@redhat.com>
Checks if uninstallation is possible should be moved to uninstall_check https://fedorahosted.org/freeipa/ticket/6392 Reviewed-By: Stanislav Laznicka <slaznick@redhat.com>
There is no need to have env as parameter because this is used only once, so it can eb safely moved to client.py module NOTE: PATH should be overwritten to safe values before we execute any command https://www.securecoding.cert.org/confluence/display/c/ENV03-C.+Sanitize+the+environment+when+invoking+external+programs https://fedorahosted.org/freeipa/ticket/6392 Reviewed-By: Stanislav Laznicka <slaznick@redhat.com>
install_check() and promote_check() have some common checks that can be safely moved to common grounds. https://fedorahosted.org/freeipa/ticket/6392 Reviewed-By: Martin Basti <mbasti@redhat.com> Reviewed-By: Jan Cholasta <jcholast@redhat.com>
https://fedorahosted.org/freeipa/ticket/6392 Reviewed-By: Martin Basti <mbasti@redhat.com> Reviewed-By: Jan Cholasta <jcholast@redhat.com>
Domain levels 0 and 1 use the same mechanism of checking domain level correctness. Group them together and make it more general should there be more domain levels in the future (although lets hope there won't be). https://fedorahosted.org/freeipa/ticket/6392 Reviewed-By: Martin Basti <mbasti@redhat.com> Reviewed-By: Jan Cholasta <jcholast@redhat.com>
Instead of delegating handling of some parameters like fstore to the parent class, the *Instance installers had the logic copy-pasted in their constructors. Some other members were also moved to the Service class and the parent class constructors in children were fixed to modern standards of initializing parent class in Python. https://fedorahosted.org/freeipa/ticket/6392 Reviewed-By: Stanislav Laznicka <slaznick@redhat.com>
This will aid further refactoring of service installers, since the user will be defined only once during parent class initialization. https://fedorahosted.org/freeipa/ticket/6392 Reviewed-By: Stanislav Laznicka <slaznick@redhat.com>
The Service class now accepts keytab path and service name part of Kerberos principal as members. Kerberos principal is turned into a property computed from service prefix, FQDN and realm. the handling of Kerberos principals and keytabs in service installers was changed to use class members instead of copy-pasted constants. This shall aid in the future refactoring of principal/keytab handling code. https://fedorahosted.org/freeipa/ticket/6392 Reviewed-By: Stanislav Laznicka <slaznick@redhat.com>
Since creation of service principals and keytab retrieval are common operations, Service class should provide means to add service entry to LDAP, retrieve its keytab to designated destination and change the owner to service user. https://fedorahosted.org/freeipa/ticket/6405 Reviewed-By: Stanislav Laznicka <slaznick@redhat.com>
In DL0 directory manager password is bundled in the supplied replica file and the replica installer can use it to authenticate against master when retrieving service keytabs. In DL1, however, DM credentials are generated randomly and used during local DS instance creation. The proper DM password is imported by custodia much later to the process. We must not allow the installer to contact the remote master using this random password since it would fail. https://fedorahosted.org/freeipa/ticket/6405 Reviewed-By: Stanislav Laznicka <slaznick@redhat.com>
DS replica can now use remote API and ipa-getkeytab to create service principal and fetch the keytab in both domain levels. There is no need to use KDC installer to do it. https://fedorahosted.org/freeipa/ticket/6405 Reviewed-By: Stanislav Laznicka <slaznick@redhat.com>
This is required to enable password extension plugin right away so that services configured later can use it to request keytabs via ipa-getkeytab. https://fedorahosted.org/freeipa/ticket/6405 Reviewed-By: Stanislav Laznicka <slaznick@redhat.com>
apache keytab is now retrieved using the same method in both domain levels. The difference lies in the authentication scheme used to retrieve service keytab: * in DL0 passed in DM credentials are used * in DL1 GSSAPI is used https://fedorahosted.org/freeipa/ticket/6405 Reviewed-By: Stanislav Laznicka <slaznick@redhat.com>
This functionality was merged to Service class and is not longer used anywhere. https://fedorahosted.org/freeipa/ticket/6405 Reviewed-By: Stanislav Laznicka <slaznick@redhat.com>
Dogtag requires Directory Manager password for its installation. On Domain Level 1 a special password for Directory Manager is created and used during the installation. However, by importing the real DM password from remote LDAP, we can no longer use the temporary password from the replica installation. https://fedorahosted.org/freeipa/ticket/6392 Reviewed-By: Jan Cholasta <jcholast@redhat.com>
Instead of specifying which knobs should be positional arguments in cli.install_tool(), do it using a flag in knob definition, where the rest of CLI configuration is. As a side effect, the usage string for CLI tools can now be generated automatically. https://fedorahosted.org/freeipa/ticket/6392 Reviewed-By: Martin Basti <mbasti@redhat.com>
Let IPAOptionParser handle parsing of its supported types and use an option callback only for unsupported types. Instead of parsing positional arguments manually, parse them using a custom IPAOptionParser instance, reusing the option parsing code. https://fedorahosted.org/freeipa/ticket/6392 Reviewed-By: Martin Basti <mbasti@redhat.com>
Add new knob() knob constructor. Keep the old Knob() constructor for backward compatibility with old code. https://fedorahosted.org/freeipa/ticket/6392 Reviewed-By: Martin Basti <mbasti@redhat.com>
Use type(None) rather than bool to define knobs which are represented as
command line flags. This allows declaring both "--option" and
"--option={0,1}"-style command line options.
Use enum.Enum subclasses instead of set literals to declare enumerations.
Use typing.List[T] instead of (list, T) to declare lists. (Note that a
minimal reimplementation of typing.List is used instead of the Python 2
backport of the typing module due to non-technical reasons.)
Use CheckedIPAddress instead of 'ip' and 'ip-local' to declare IP
addresses.
https://fedorahosted.org/freeipa/ticket/6392
Reviewed-By: Martin Basti <mbasti@redhat.com>
Replace cli_name, cli_short_name and cli_positional knob arguments with a
single cli_names argument, which allows defining one or more CLI names
using the argparse convention ("--option" for long option name, "-o" for
short option name and "argument" for positional argument name).
Also replace cli_aliases with cli_deprecated_names which uses the same
convention.
https://fedorahosted.org/freeipa/ticket/6392
Reviewed-By: Martin Basti <mbasti@redhat.com>
Declare knob bases explicitly using a keyword argument instead of guessing if the type argument is a base or a type of the knob. https://fedorahosted.org/freeipa/ticket/6392 Reviewed-By: Martin Basti <mbasti@redhat.com>
Add new @group decorator to declare an installer class as a knob group instead of subclassing Group, so that subclassing the installer does not create duplicates of the original group. https://fedorahosted.org/freeipa/ticket/6392 Reviewed-By: Martin Basti <mbasti@redhat.com>
Add class hierarchy which allows inherting knob definitions between the various client and server install scripts. https://fedorahosted.org/freeipa/ticket/6392 Reviewed-By: Martin Basti <mbasti@redhat.com>
Migrate ipa-server-install and ipa-replica-install from the old installer classes to the new installer class hierarchy classes. https://fedorahosted.org/freeipa/ticket/6392 Reviewed-By: Martin Basti <mbasti@redhat.com>
https://fedorahosted.org/freeipa/ticket/6392 Reviewed-By: Martin Basti <mbasti@redhat.com>
Migrate ipa-client-install from the custom script to the new installer class hierarchy classes. https://fedorahosted.org/freeipa/ticket/6392 Reviewed-By: Martin Basti <mbasti@redhat.com>
|
ACK, we may ignore those minor PEP8 issues, it is mainly caused by copying code to other parts. |
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR contains the installer refactoring work we (@jcholast, @martbab, @mbasti-rh, @stlaz) did in the last 6 weeks.
What is included:
What is missing:
ipa-client-installin server installers.ipa-replica-prepare,ipa-ca-install,ipa-kra-install,ipa-dns-install) to make use of the single option definition.Known issues:
ipa-server-install,ipa-replica-installandipa-client-install.https://fedorahosted.org/freeipa/ticket/6392
Note that the commits in this PR were already reviewed and ACKed over at jcholast/freeipa (list of PRs).
If you find any issues, please file a Trac ticket or leave a comment in this PR.