New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove hard dependency on ipaplatform from ipapython, ipalib and ipaclient #271

Closed
wants to merge 18 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@HonzaCholasta
Contributor

HonzaCholasta commented Nov 24, 2016

paths: remove DEV_NULL

The platform-specific path to /dev/null is provided by the Python standard
library in os.devnull.

Replace all uses of paths.DEV_NULL with os.devnull and remove DEV_NULL.

custodiainstance: automatic restart on config file update

Automatically restart Custodia during IPA server upgrade if custodia.conf
was updated.

Use the new store class name in custodia.conf.template.

ipapython: move dnssec, p11helper and secrets to ipaserver

The dnssec and secrets subpackages and the p11helper module depend on
ipaplatform.

Move them to ipaserver as they are used only on the server.

ipapython: move certmonger and sysrestore to ipalib.install

The certmonger and sysrestore modules depend on ipaplatform.

Move them to ipalib.install as they are used only from installers.

certdb: move IPA NSS DB install functions to ipaclient.install

The create_ipa_nssdb() and update_ipa_nssdb() depend on ipaplatform.

Move them to ipaclient.install.client as they are used only from the client
installer.

certdb: use a temporary file to pass password to pk12util

Currently the PKCS#12 file password is passed via stdin and pk12util reads
it from /dev/stdin, which is platform-specific.

Use a temporary file instead.

ipautil: remove SHARE_DIR and PLUGIN_SHARE_DIR

SHARE_DIR and PLUGIN_SHARE_DIR depend on ipaplatform.

Replace all uses of SHARE_DIR with paths.USR_SHARE_IPA_DIR and remove
both SHARE_DIR and PLUGIN_SHARE_DIR.

ipautil: remove get_domain_name()

get_domain_name() and related code depends on ipaplatform.

Replace all uses of get_domain_name() with api.env.domain and remove
get_domain_name() and all of the related code.

ipautil: remove the timeout argument of run()

The argument depends on the platform-specific timeout binary and is used
only in ipaclient.ntpconf.

Call the timeout binary explicitly in ipaclient.ntpconf and remove the
argument.

ipautil: move is_fips_enabled() to ipaplatform.tasks

The FIPS setting is platform-specific.

ipautil: move kinit functions to ipalib.install

kinit_password() depends on ipaplatform.

Move kinit_password() as well as kinit_keytab() to a new
ipalib.install.kinit module, as they are used only from installers.

ipautil: move file encryption functions to installutils

The encrypt_file() and decrypt_file() functions depend on ipaplatform.

Move them to ipaserver.install.installutils, as they are only used for the
server installer.

ipapython: remove hard dependency on ipaplatform

Use hard-coded paths to certutil, pk12util and openssl in certdb if
ipaplatform is not available.

Hard-coded the path to setpasswd in ipautil.run() doc string.

Remove ipaplatform dependency from ipapython's setup.py and add ipapython
dependency to ipaplatform's setup.py.

ipalib: move certstore to the install subpackage

The certstore module depends on ipaplatform.

Move it to ipalib.install, as it is used only from installers.

constants: remove CACERT

CACERT depends on ipaplatform.

Replace all uses of CACERT with paths.IPA_CA_CRT and remove CACERT.

ipalib: remove hard dependency on ipapython

Hard-code the path to /bin/false in SubprocessError doc string.

Remove ipaplatform dependency from ipalib's setup.py and add it as optional
installer dependency to ipalib's and ipaclient's setup.py.

ipaclient: move install modules to the install subpackage

The ipa_certupdate, ipachangeconf, ipadiscovery and ntpconf modules depend
on ipaplatform.

Move them to ipaclient.install as they are used only from the client
installer.

ipaclient: remove hard dependency on ipaplatform

Hard-code the user cache directory path in ipaclient.remote_plugins.schema.

https://fedorahosted.org/freeipa/ticket/6474

@stlaz stlaz self-assigned this Nov 24, 2016

@tiran

This comment has been minimized.

Show comment
Hide comment
@tiran

tiran Nov 24, 2016

Member

The PR is too large. Please split it up in multiple small PRs.

Member

tiran commented Nov 24, 2016

The PR is too large. Please split it up in multiple small PRs.

@stlaz

This comment has been minimized.

Show comment
Hide comment
@stlaz

stlaz Nov 24, 2016

Contributor

I do not have much trouble reviewing the whole PR, also it does not do that much and does not break tests (did not try integration) so I believe it's fine.
I am not sure if adding all the os.path.joins is actually really necessary here but it'd be more foolproof for the future.

Contributor

stlaz commented Nov 24, 2016

I do not have much trouble reviewing the whole PR, also it does not do that much and does not break tests (did not try integration) so I believe it's fine.
I am not sure if adding all the os.path.joins is actually really necessary here but it'd be more foolproof for the future.

@@ -831,7 +832,7 @@ def cert_restore_prepare(self):
def cert_restore(self):
try:
certdb.update_ipa_nssdb()
update_ipa_nssdb()

This comment has been minimized.

@stlaz

stlaz Nov 24, 2016

Contributor

NOTE: This is probably the only exception of update_ipa_nssdb usage on server side contrary to what the commit message says but it seems fine to be used like this.

@stlaz

stlaz Nov 24, 2016

Contributor

NOTE: This is probably the only exception of update_ipa_nssdb usage on server side contrary to what the commit message says but it seems fine to be used like this.

This comment has been minimized.

@HonzaCholasta

HonzaCholasta Nov 24, 2016

Contributor

Right, that's because ipa_restore is not integrated well (or at all) with installers. Also I would like to keep the code in ipaclient, as the client owns the database.

@HonzaCholasta

HonzaCholasta Nov 24, 2016

Contributor

Right, that's because ipa_restore is not integrated well (or at all) with installers. Also I would like to keep the code in ipaclient, as the client owns the database.

Show outdated Hide outdated ipapython/certdb.py
if not config.default_server:
raise IPAConfigError("IPA server not found in DNS, in the config file (/etc/ipa/default.conf) or on the command line.")
if not config.default_domain:
raise IPAConfigError("IPA domain not found in the config file (/etc/ipa/default.conf) or on the command line.")

This comment has been minimized.

@stlaz

stlaz Nov 24, 2016

Contributor

With this deletion, there's nothing that raises IPAConfigError. Is there a reason to leaving it there?

@stlaz

stlaz Nov 24, 2016

Contributor

With this deletion, there's nothing that raises IPAConfigError. Is there a reason to leaving it there?

This comment has been minimized.

@HonzaCholasta

HonzaCholasta Nov 24, 2016

Contributor

No, I will remove it as well.

@HonzaCholasta

HonzaCholasta Nov 24, 2016

Contributor

No, I will remove it as well.

@HonzaCholasta

This comment has been minimized.

Show comment
Hide comment
@HonzaCholasta

HonzaCholasta Nov 24, 2016

Contributor

@tiran, how much granular PRs would you prefer? As @stlaz pointed out, there isn't actually much going on in this PR besides moving stuff around.

Contributor

HonzaCholasta commented Nov 24, 2016

@tiran, how much granular PRs would you prefer? As @stlaz pointed out, there isn't actually much going on in this PR besides moving stuff around.

@stlaz

This comment has been minimized.

Show comment
Hide comment
@stlaz

stlaz Nov 25, 2016

Contributor

The changes seem fine, I especially dig moving parts only used in ipaserver/ipaclient to their respective submodules.
I can see why ipaplatform is being removed from certain modules. It seems like a workaround/another solution for the dynamic platform linking which is proposed in https://fedorahosted.org/freeipa/ticket/6474. I wonder what the plan with ipaplatform is, then. Could it be that its removal from certain parts of the code would be redundant with the proposed changes? Or do we just keep these changes instead?

Contributor

stlaz commented Nov 25, 2016

The changes seem fine, I especially dig moving parts only used in ipaserver/ipaclient to their respective submodules.
I can see why ipaplatform is being removed from certain modules. It seems like a workaround/another solution for the dynamic platform linking which is proposed in https://fedorahosted.org/freeipa/ticket/6474. I wonder what the plan with ipaplatform is, then. Could it be that its removal from certain parts of the code would be redundant with the proposed changes? Or do we just keep these changes instead?

@HonzaCholasta

This comment has been minimized.

Show comment
Hide comment
@HonzaCholasta

HonzaCholasta Nov 25, 2016

Contributor

@stlaz, this thread at freeipa-devel should answer your question.

Contributor

HonzaCholasta commented Nov 25, 2016

@stlaz, this thread at freeipa-devel should answer your question.

@tiran

This comment has been minimized.

Show comment
Hide comment
@tiran

tiran Nov 25, 2016

Member

@jcholast I prefer small patches, that change just one aspect and are easily reviewable in a couple of minutes. The PR touches the entire code base. With 600 additions, more than 700 removals and 316 QC high-severity issues, it is going to take a week to merge it. Merge conflicts are already cumulating, too.

You have already split up your PR in a bunch of commits. It looks like most to all commits are unrelated and don't depend on each other. Basically your PR is an epic with a bunch of independent improvements. How about use the iterative approach and create a PR for each commit?

Member

tiran commented Nov 25, 2016

@jcholast I prefer small patches, that change just one aspect and are easily reviewable in a couple of minutes. The PR touches the entire code base. With 600 additions, more than 700 removals and 316 QC high-severity issues, it is going to take a week to merge it. Merge conflicts are already cumulating, too.

You have already split up your PR in a bunch of commits. It looks like most to all commits are unrelated and don't depend on each other. Basically your PR is an epic with a bunch of independent improvements. How about use the iterative approach and create a PR for each commit?

@stlaz

This comment has been minimized.

Show comment
Hide comment
@stlaz

stlaz Nov 25, 2016

Contributor

@jcholast Thanks, I'll add it as a comment to that ticket so that it's more visible to a potential community :)
@tiran I already did the review, the conflicts are very easily resolvable (ntpconf was moved, two functions are moved from ipa_replica_prepare.) I can see where you're heading and I guess it'd be better to split the PR for the future, although I prefer 1 PR for 1 ticket if that is doable and it is in this case. Can you please rather check if it matches your use-case and bless this PR with functional ACK so that we can get it pushed?

edit: Removed the LGTM till the outlined necessary issues are fixed, I expect that to come with the rebase.

Contributor

stlaz commented Nov 25, 2016

@jcholast Thanks, I'll add it as a comment to that ticket so that it's more visible to a potential community :)
@tiran I already did the review, the conflicts are very easily resolvable (ntpconf was moved, two functions are moved from ipa_replica_prepare.) I can see where you're heading and I guess it'd be better to split the PR for the future, although I prefer 1 PR for 1 ticket if that is doable and it is in this case. Can you please rather check if it matches your use-case and bless this PR with functional ACK so that we can get it pushed?

edit: Removed the LGTM till the outlined necessary issues are fixed, I expect that to come with the rebase.

@tiran

This comment has been minimized.

Show comment
Hide comment
@tiran

tiran Nov 28, 2016

Member

This PR is just too big and has too many CI errors to even begin a sensible review. I would need at least half a day without any interruption to perform even a basic review. Given my other responsibilities and daily meetings, I won't have time until Thursday.

Member

tiran commented Nov 28, 2016

This PR is just too big and has too many CI errors to even begin a sensible review. I would need at least half a day without any interruption to perform even a basic review. Given my other responsibilities and daily meetings, I won't have time until Thursday.

HonzaCholasta added some commits Nov 22, 2016

ipapython: move dnssec, p11helper and secrets to ipaserver
The dnssec and secrets subpackages and the p11helper module depend on
ipaplatform.

Move them to ipaserver as they are used only on the server.

https://fedorahosted.org/freeipa/ticket/6474
ipapython: move certmonger and sysrestore to ipalib.install
The certmonger and sysrestore modules depend on ipaplatform.

Move them to ipalib.install as they are used only from installers.

https://fedorahosted.org/freeipa/ticket/6474
paths: remove DEV_NULL
The platform-specific path to /dev/null is provided by the Python standard
library in os.devnull.

Replace all uses of paths.DEV_NULL with os.devnull and remove DEV_NULL.

https://fedorahosted.org/freeipa/ticket/6474
custodiainstance: automatic restart on config file update
Automatically restart Custodia during IPA server upgrade if custodia.conf
was updated.

Use the new store class name in custodia.conf.template.

https://fedorahosted.org/freeipa/ticket/6474

HonzaCholasta added some commits Nov 23, 2016

certdb: move IPA NSS DB install functions to ipaclient.install
The create_ipa_nssdb() and update_ipa_nssdb() depend on ipaplatform.

Move them to ipaclient.install.client as they are used only from the client
installer and ipa-restore.

https://fedorahosted.org/freeipa/ticket/6474
certdb: use a temporary file to pass password to pk12util
Currently the PKCS#12 file password is passed via stdin and pk12util reads
it from /dev/stdin, which is platform-specific.

Use a temporary file instead.

https://fedorahosted.org/freeipa/ticket/6474
ipautil: remove SHARE_DIR and PLUGIN_SHARE_DIR
SHARE_DIR and PLUGIN_SHARE_DIR depend on ipaplatform.

Replace all uses of SHARE_DIR with paths.USR_SHARE_IPA_DIR and remove
both SHARE_DIR and PLUGIN_SHARE_DIR.

https://fedorahosted.org/freeipa/ticket/6474
ipautil: remove get_domain_name()
get_domain_name() and related code depends on ipaplatform.

Replace all uses of get_domain_name() with api.env.domain and remove
get_domain_name() and all of the related code.

https://fedorahosted.org/freeipa/ticket/6474
ipautil: remove the timeout argument of run()
The argument depends on the platform-specific timeout binary and is used
only in ipaclient.ntpconf.

Call the timeout binary explicitly in ipaclient.ntpconf and remove the
argument.

https://fedorahosted.org/freeipa/ticket/6474
ipautil: move kinit functions to ipalib.install
kinit_password() depends on ipaplatform.

Move kinit_password() as well as kinit_keytab() to a new
ipalib.install.kinit module, as they are used only from installers.

https://fedorahosted.org/freeipa/ticket/6474
ipautil: move file encryption functions to installutils
The encrypt_file() and decrypt_file() functions depend on ipaplatform.

Move them to ipaserver.install.installutils, as they are only used for the
server installer.

https://fedorahosted.org/freeipa/ticket/6474
constants: remove CACERT
CACERT depends on ipaplatform.

Replace all uses of CACERT with paths.IPA_CA_CRT and remove CACERT.

https://fedorahosted.org/freeipa/ticket/6474
ipapython: remove hard dependency on ipaplatform
Use hard-coded paths to certutil, pk12util and openssl in certdb if
ipaplatform is not available.

Hard-coded the path to setpasswd in ipautil.run() doc string.

Remove ipaplatform dependency from ipapython's setup.py and add ipapython
dependency to ipaplatform's setup.py.

https://fedorahosted.org/freeipa/ticket/6474
ipalib: move certstore to the install subpackage
The certstore module depends on ipaplatform.

Move it to ipalib.install, as it is used only from installers.

https://fedorahosted.org/freeipa/ticket/6474
ipaclient: move install modules to the install subpackage
The ipa_certupdate, ipachangeconf, ipadiscovery and ntpconf modules depend
on ipaplatform.

Move them to ipaclient.install as they are used only from the client
installer.

https://fedorahosted.org/freeipa/ticket/6474
ipalib: remove hard dependency on ipapython
Hard-code the path to /bin/false in SubprocessError doc string.

Remove ipaplatform dependency from ipalib's setup.py and add it as optional
installer dependency to ipalib's and ipaclient's setup.py.

https://fedorahosted.org/freeipa/ticket/6474
ipaclient: remove hard dependency on ipaplatform
Hard-code the user cache directory path in ipaclient.remote_plugins.schema.

https://fedorahosted.org/freeipa/ticket/6474
@stlaz

This comment has been minimized.

Show comment
Hide comment
@stlaz

stlaz Nov 29, 2016

Contributor

I checked the rebase again as well as ran the tests. The changes in the PR clean the code nicely aside from doing what's proposed in the given ticket. The issues from CI and QuantifiedCode are only caused by moving the code in between modules. ACK.

Contributor

stlaz commented Nov 29, 2016

I checked the rebase again as well as ran the tests. The changes in the PR clean the code nicely aside from doing what's proposed in the given ticket. The issues from CI and QuantifiedCode are only caused by moving the code in between modules. ACK.

@stlaz stlaz added the ack label Nov 29, 2016

@MartinBasti

This comment has been minimized.

Show comment
Hide comment
@MartinBasti

MartinBasti Nov 29, 2016

Contributor

Ticket https://fedorahosted.org/freeipa/ticket/6474 is closed as wontfix and even doesn't seems right to me.

Contributor

MartinBasti commented Nov 29, 2016

Ticket https://fedorahosted.org/freeipa/ticket/6474 is closed as wontfix and even doesn't seems right to me.

@stlaz

This comment has been minimized.

Show comment
Hide comment
@stlaz

stlaz Nov 29, 2016

Contributor

Last I checked the ticket was still open. The ticket was trying to solve the same issue as this PR although its aim shifted (see the link I posted in the comments).

Contributor

stlaz commented Nov 29, 2016

Last I checked the ticket was still open. The ticket was trying to solve the same issue as this PR although its aim shifted (see the link I posted in the comments).

@MartinBasti

This comment has been minimized.

Show comment
Hide comment
@MartinBasti

MartinBasti Nov 29, 2016

Contributor

Ticket updated.

Contributor

MartinBasti commented Nov 29, 2016

Ticket updated.

@stlaz stlaz added the pushed label Nov 30, 2016

@stlaz

This comment has been minimized.

Show comment
Hide comment
@stlaz

stlaz Nov 30, 2016

Contributor

The patch's already been pushed, could you, @mbasti-rh, supply the automated message?

Contributor

stlaz commented Nov 30, 2016

The patch's already been pushed, could you, @mbasti-rh, supply the automated message?

@MartinBasti

This comment has been minimized.

Show comment
Hide comment
@MartinBasti

MartinBasti Nov 30, 2016

Contributor

master:

9117a5d paths: remove DEV_NULL
8e5d2c7 custodiainstance: automatic restart on config file update
a1f260d ipapython: move dnssec, p11helper and secrets to ipaserver
26c46a4 ipapython: move certmonger and sysrestore to ipalib.install
f919ab4 certdb: use a temporary file to pass password to pk12util
d6b755e ipautil: remove SHARE_DIR and PLUGIN_SHARE_DIR
7b966e8 ipautil: remove get_domain_name()
d911f49 ipautil: remove the timeout argument of run()
75b70e3 ipautil: move is_fips_enabled() to ipaplatform.tasks
7d5c680 ipautil: move kinit functions to ipalib.install
6e50fae ipautil: move file encryption functions to installutils
528012f ipapython: remove hard dependency on ipaplatform
a2c5888 ipalib: move certstore to the install subpackage
977050c constants: remove CACERT
d43b57d ipalib: remove hard dependency on ipapython
70c3cd7 ipaclient: move install modules to the install subpackage
a260fd8 ipaclient: remove hard dependency on ipaplatform

Contributor

MartinBasti commented Nov 30, 2016

master:

9117a5d paths: remove DEV_NULL
8e5d2c7 custodiainstance: automatic restart on config file update
a1f260d ipapython: move dnssec, p11helper and secrets to ipaserver
26c46a4 ipapython: move certmonger and sysrestore to ipalib.install
f919ab4 certdb: use a temporary file to pass password to pk12util
d6b755e ipautil: remove SHARE_DIR and PLUGIN_SHARE_DIR
7b966e8 ipautil: remove get_domain_name()
d911f49 ipautil: remove the timeout argument of run()
75b70e3 ipautil: move is_fips_enabled() to ipaplatform.tasks
7d5c680 ipautil: move kinit functions to ipalib.install
6e50fae ipautil: move file encryption functions to installutils
528012f ipapython: remove hard dependency on ipaplatform
a2c5888 ipalib: move certstore to the install subpackage
977050c constants: remove CACERT
d43b57d ipalib: remove hard dependency on ipapython
70c3cd7 ipaclient: move install modules to the install subpackage
a260fd8 ipaclient: remove hard dependency on ipaplatform

This was referenced Nov 30, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment