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

Basic uninstaller for the CA #764

Closed
wants to merge 1 commit into from
Closed

Conversation

rcritten
Copy link
Contributor

@rcritten rcritten commented May 4, 2017

This in response to watching users flounder with repeated failed replica installations and ipa-ca-install attempts that require a complete uninstall. Review it with whatever priority you desire.

This is meant ONLY to be able to re-try an installation if the
CA cloning fails for some reason. It is not intended to be used
to remove the CA as a service on a given master.

This is to avoid having to stand up a whole new master just
because the CA installation failed.

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

@martbab
Copy link
Contributor

martbab commented May 5, 2017

I would avoid having half-effective CA uninstaller given that other components like Samba, DNS(Sec), and now also KRA (given the amount of bugs the uninstaller caused) do not support their uninstallation.

Either we have to design some unified framework for proper optional component uninstallation, or we can go in vein of AD trust and DNS installers which are idempotent to a degree. I have an impression that this PR will cause users more problems than it aims to solve.

@rcritten
Copy link
Contributor Author

rcritten commented May 5, 2017

What do you mean half-effective? Did you try it?

There already IS a unified framework for component uninstallation: each service provides it. There are edge cases when trying to uninstall one particular piece but that is relatively straightforward to handle and as outlined in the PR this is not intended or expected to clean up every last element, just enough to be able to cleanly attempt the installation again.

Forcing users to uninstall an entire master just to (try to) re-install the CA is a major pain point.

Other services not having uninstall options is not relevant to this case IMHO.

@martbab
Copy link
Contributor

martbab commented May 9, 2017

@rcritten If it is expected to not clean up properly after a fai;ed installation then I would rather not advertise it as an uninstaller, otherwise users will start to get ideas like "I do not want to use built-in CA anymore, let's just uninstall it and use 3rd party certs everywhere" and will run into problems with leftover certificates and such.

I would rather provide some rollback after failed install but again, I think there should be a more extensive discussion about a generic solution applicable to all service installers.

Also I would not claim that we actually do not have a service uninstaller framework since every service installer has a copy-pasted code in an ad-hoc coded uninstall method repeated ad nauseam. From what I have glimpsed from ipa-4-5 branch, Service class does not even provide uninstall abstract method to override, only SimpleServiceInstance does that.

@pvoborni
Copy link
Member

pvoborni commented May 9, 2017

Let's first clarify the problem to solve. If I understand @rcritten right, the problem is that if ipa-ca-install fail then one must reinstall the whole replica because the failed installation left a garbage and subsequent installer is not able to handle the garbage.

Uninstallation of successful CA installation is not the intend, right? If so then it seems to me that both of you are in agreement. And I would add that I completely agree with CA uninstall not being a goal because it would add just another use case to support with a benefit I don't see.

So if goal is repeatable ipa-ca-install then let's not talk about creating a CA uninstaller but rather about CA cleanup and let's hide/remove the --uninstall option and figure out how it should behave - i.e. let it be internal.

@martbab
Copy link
Contributor

martbab commented May 9, 2017

@pvoborni We can try to move the uninstaller logic to the beginning of the install, or make the affected steps idempotent. But still I would be hesitant to merge this PR without some design in place.

@stlaz
Copy link
Contributor

stlaz commented May 9, 2017

@pvoborni @rcritten @martbab This discussion at this PR makes no sense. Clearly we can see that the impact is much higher and should be discussed on designated channels, meaning either freeipa-devel mailing list or in our issue tracking system (the former would be preferable with having the result in the latter). I believe that the guys from the Dogtag project could also have a great insight on this.

Here's questions which should answer why I want this to be discussed there:

  • how to handle users so they don't use ipa-ca-install --uninstall any time?
  • at which point is the installation recoverable and when it's not?
  • describe what happens in each and every step, mention which files and entries are created
    • on master
    • on replica
  • describe what has to be done in case a step fails for each and every step
    • on master
    • on replica
  • describe how ipa-ca-install rollback should behave when installing first CA in a CA-less setup

These problems are just from the top of my head and I am a CA installation noob. I would however be very cautious not knowing an answer to either of those.

@rcritten if you do know the answers, please, share them with us (or maybe just me because I sure don't know them), it would help a lot with deciding on where to go from here.

@rcritten
Copy link
Contributor Author

rcritten commented May 9, 2017

As far as I can tell it is always recoverable using this. I wasn't able to force a failure of replication, that could be a potential show-stopper. The PR doesn't touch the replication agreements at all except to allow them to already be there, so if things were in some sort of halfway state I couldn't say for sure what would happen.

The code is there for examination to determine what steps are done, but in short:

  • call the existing CA uninstaller which mostly just calls pki-destroy (it also does some state cleanup, removes the CRLs and untracks the CA certs via certmonger)
  • A side-effect of the uninstaller is to shutdown certmonger. I start that back up
  • The service is removed from cn=masters
  • The cached services list is removed so ipactl won't fail starting a non-existent tomcat instance

To be idempotent would require changes in dogtag, it is that which blows up on a re-install attempt.

I would not be in favor of automatically uninstalling dogtag on another ipa-ca-install call.

ipa-ca-install would/should never be run on the original master. It already prints a big fat warning. I'd be ok making it fatter and requiring (no joke) multiple "Are you sure" prompts.

There is no CA install for CAless so not a case I'm interested in.

If you want to rename options I'm ok with that as well, maybe --try-again or something of that nature (in which case I WOULD be in favor of doing the uninstall automatically).

@pvoborni
Copy link
Member

We need to develop something like this, but right now it is not the best time for it. First we need to stabilize 4.5.1 (seems that's almost done). Then focus on testing - current test coverage + on pull request CI. When this is ready we can focus on python3 porting and existing PRs including this one. The reason is that I'm hesitant implementing this to not introduce other regressions because it touches more areas than it seems.

For the parts above:

  • +1 for denying uninstall on successful install
  • there is actually a path from CA less to CA so we need to think about it as well

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.

I think the feature is useful, but I'm not fond of the option name --uninstall. It's partial uninstallation. What do you think about either a different option name (e.g. --remove-instance) or a --reinstall option that removes and reinstalls just Dogtag?

@@ -106,8 +106,9 @@ def pytest_cmdline_main(config):

# XXX workaround until https://fedorahosted.org/freeipa/ticket/6408 has
# been resolved.
if ipaserver is not None and installutils.is_ipa_configured():
api.finalize()
# if ipaserver is not None and installutils.is_ipa_configured():
Copy link
Member

Choose a reason for hiding this comment

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

This change seems unrelated and should not be required. In fact it is going to break Python package tests and make fastcheck.

import ipatests
HERE = os.path.dirname(os.path.abspath(ipatests.__file__))
except ImportError:
HERE = os.getcwd()
Copy link
Member

Choose a reason for hiding this comment

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

This is also unrelated and should not be required.

This is meant ONLY to be able to re-try an installation if the
CA cloning fails for some reason. It is not intended to be used
to remove the CA as a service on a given master.

This is to avoid having to stand up a whole new master just
because the CA installation failed.

https://pagure.io/freeipa/issue/6595
@rcritten
Copy link
Contributor Author

rcritten commented May 3, 2018

I'll eventually open a ticket with a design and then resubmit once that is accepted.

@rcritten rcritten closed this May 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants