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

ipa-server-install: add --setup-kra option #548

Closed
wants to merge 2 commits into from
Closed

ipa-server-install: add --setup-kra option #548

wants to merge 2 commits into from

Conversation

MartinBasti
Copy link
Contributor

@MartinBasti MartinBasti commented Mar 7, 2017

No description provided.

@@ -836,14 +836,12 @@ def install_check(installer):
root_logger.debug('No IPA DNS servers, '
'skipping forward/reverse resolution check')

kra_enabled = remote_api.Command.kra_is_enabled()['result']
Copy link
Contributor

Choose a reason for hiding this comment

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

If I understand this correctly, before this patch, KRA would be installed automatically if it was present on the master server. With this patch, --setup-kra would have to be explicitly passed to ipa-replica-install.

Is this the case or am I missing something? Is this behaviour change intended?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, you don't understand it. KRA would not be installed. See kra.py:install() for details.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

commit removed

@@ -384,6 +384,8 @@ def install_check(installer):
print(" * Create and configure an instance of Directory Server")
print(" * Create and configure a Kerberos Key Distribution Center (KDC)")
print(" * Configure Apache (httpd)")
if options.setup_kra:
Copy link
Contributor

Choose a reason for hiding this comment

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

Please update the man pages with --setup-kra option for both ipa-server-install and ipa-replica-install.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@tkrizek tkrizek self-assigned this Mar 7, 2017
@HonzaCholasta
Copy link
Contributor

NACK on the "KRA: run install and install_check only when KRA should be installed" commit. The end goal for all component installers is to make them isolated and handle their options themselves, so that they can be packaged separately (among other things). This commit takes the code in the opposite direction. Also it does not make the code more readable because it is inconsistent with the CA installer.

@@ -159,7 +159,6 @@ def domain_level(self, value):
None,
description="configure a dogtag KRA",
)
setup_kra = enroll_only(setup_kra)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you remove this line? It must be here, otherwise --setup-kra will appear in ipa-replica-prepare once it's ported to the install framework.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I somehow thought that it will not work, reverted.

This patch allows to install KRA on first IPA server in one step using
ipa-server-install

This option improves containers installation where ipa-server can be
installed with KRA using one call without need to call docker exec.

Please note the the original `kra.install()` calls in
ipaserver/install/server/install.py were empty operations as it did
nothing, so it is safe to move them out from CA block

https://pagure.io/freeipa/issue/6731
This will allow to test --setup-kra option together with
ipa-server-install in install tests

Separate installation using ipa-kra-install is already covered.

https://pagure.io/freeipa/issue/6731
@MartinBasti
Copy link
Contributor Author

Given that there is no time, I dropped commit you NACKed as it unneeded for this PR, but please note my disagreement about a way how kra.py handles --setup-kra option for the future release.

@tkrizek tkrizek added the ack Pull Request approved, can be merged label Mar 8, 2017
@ghost
Copy link

ghost commented Mar 8, 2017

master:

  • 4006cbb KRA: add --setup-kra to ipa-server-install
  • 25fa2bb tests: use --setup-kra in tests

@ghost ghost added the pushed Pull Request has already been pushed label Mar 8, 2017
@ghost ghost closed this Mar 8, 2017
@MartinBasti MartinBasti deleted the kra-master-install branch March 8, 2017 14:54
This pull request was closed.
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