Skip to content
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

httpinstance.disable_system_trust: Don't fail if module 'Root Certs' … #655

Closed
wants to merge 1 commit into from
Closed

Conversation

ghost
Copy link

@ghost ghost commented Mar 27, 2017

…is not available

Server installation failed when attmpting to disable module 'Root Certs' and
the module was not available in HTTP_ALIAS_DIR. When the module is not
available there's no need to disable it and the error may be treated as
success.

https://pagure.io/freeipa/issue/6803

tiran
tiran previously requested changes Mar 27, 2017
Copy link
Member

@tiran tiran left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

only ignore ERROR: Module "Root Certs" not found in database.

env={},
capture_output=True)
except ipautil.CalledProcessError:
root_logger.debug(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This catches and ignores all call error. I would rather limit the exception to exit code 29:

$ mkdir /tmp/nssdb
$ certutil -N -d /tmp/nssdb/ --empty-password
$ modutil -dbdir /tmp/nssdb/ -force -list 'Root Certs'
ERROR: Module "Root Certs" not found in database.
$ echo $?
29

@stlaz
Copy link
Contributor

stlaz commented Mar 27, 2017

Hm, I believe the -list operation was there just to check whether the module is there. If modutil fails like this no matter the situation, it'd be better to just try to disable it whatever happens and go away with "meh, so what" if the disable fails because the module is not available.

@stlaz
Copy link
Contributor

stlaz commented Mar 27, 2017

For the record:

[slaznick@machine ~]$ sudo modutil -dbdir nssdb/ -disable 'Root Certs' -force
ERROR: Module "Root Certs" not found in database.
[slaznick@machine ~]$ echo $?
29
[slaznick@machine ~]$ 

@tiran
Copy link
Member

tiran commented Mar 27, 2017

@stlaz The broad except also catches and ignores typos in the command line or missing modutil binary.

@stlaz
Copy link
Contributor

stlaz commented Mar 27, 2017

@tiran I of course agree on narrowing the broad except down, my point is we should rather remove the whole -list part and just try to do -disable.

edit: I would like to better base it on return code rather than stderr string.

@ghost
Copy link
Author

ghost commented Mar 27, 2017

@tiran @stlaz Makes sense. I will update it accordingly. Thanks for suggestions.

@HonzaCholasta
Copy link
Contributor

@stlaz, you can't do just -disable, as that would break upgrade (note that the -list is there because -disable always reports success).

…is not available

Server installation failed when attmpting to disable module 'Root Certs' and
the module was not available in HTTP_ALIAS_DIR. When the module is not
available there's no need to disable it and the error may be treated as
success.

https://pagure.io/freeipa/issue/6803
@stlaz
Copy link
Contributor

stlaz commented Mar 27, 2017

@HonzaCholasta You're right, I completely forgot about that one.

@stlaz stlaz self-assigned this Mar 28, 2017
@stlaz
Copy link
Contributor

stlaz commented Mar 28, 2017

This fixes the mentioned issue. I did not test whether the actual disable works but I should hope so as I don't see how this could break it.

@stlaz stlaz added the ack Pull Request approved, can be merged label Mar 28, 2017
@stlaz stlaz dismissed tiran’s stale review March 28, 2017 10:33

The change was worked in.

@tkrizek
Copy link
Contributor

tkrizek commented Mar 28, 2017

ipa-4-5:

  • 2a49955 httpinstance.disable_system_trust: Don't fail if module 'Root Certs' is not available
    master:

  • 0128e80 httpinstance.disable_system_trust: Don't fail if module 'Root Certs' is not available

@tkrizek tkrizek added the pushed Pull Request has already been pushed label Mar 28, 2017
@tkrizek tkrizek closed this Mar 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ack Pull Request approved, can be merged pushed Pull Request has already been pushed
Projects
None yet
4 participants