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.

Sign up for GitHub

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

Jump to bottom

Refactor CA file handling in replica installer. #1093

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Refactor CA file handling in replica installer. #1093

wants to merge 1 commit into from

Conversation

rjeffman
Copy link
Member

@rjeffman rjeffman commented May 9, 2023

FreeIPA will refactor replica installer, and will remove function ipaserver.install.server.replicainstall.install_ca_cert() which is used by ipareplica role. As this change will be introduced in the next FreeIPA release, ansible-freeipa will not be able to deploy a replica if it is not updated.

The changes in FreeIPA can be found at freeipa/freeipa#6620

This PR brings these changes to ipareplica role.

@t-woerner
Copy link
Member

Hi Rafael, what should be done is to keep the old code for the IPA versions where install_ca_cert is available and used and only switch to ipa-certupdate when install_ca_cert does not exist anymore. The goal is to keep consistent behaviour to the command line installers as much as possible.

@t-woerner
Copy link
Member

It will be needed to add a new variable to ipareplica_test to active the old or new code path depending on the existence of install_ca_cert.

Something like this would be good:

  • ipareplica_install_ca_certs should be skipped in install.yml if install_ca_cert does not exist anymore.
  • ipa-certupdate should only be called in ipareplica_prepare if install_ca_cert does not exist anymore.

varunmylaraiah
varunmylaraiah approved these changes May 9, 2023
View reviewed changes
Copy link
Collaborator

@varunmylaraiah varunmylaraiah left a comment

Choose a reason for hiding this comment

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

Downstream tests are passed with this PR

t-woerner
t-woerner requested changes May 9, 2023
View reviewed changes
Copy link
Member

@t-woerner t-woerner left a comment

Choose a reason for hiding this comment

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

ipa-certupdate should only be used for new the IPA versions without install_ca_cert to do exactly the same as the old and new IPA command line installers.

@rjeffman rjeffman force-pushed the ipareplica_remove_install_ca_cert branch 3 times, most recently from 1057558 to 2869544 Compare May 10, 2023 21:17
@rjeffman rjeffman requested a review from t-woerner May 10, 2023 21:33
@rjeffman
Copy link
Member Author

@t-woerner I updated the PR with the requested changes.

@rjeffman rjeffman force-pushed the ipareplica_remove_install_ca_cert branch from 2869544 to b7e997b Compare May 16, 2023 17:46
@rjeffman rjeffman mentioned this pull request May 16, 2023
Open
@rjeffman
Copy link
Member Author

The "_ca_file" removal patch has been moved to PR #1096, so we can decide what to do with it later.

@rjeffman rjeffman force-pushed the ipareplica_remove_install_ca_cert branch from b7e997b to eafaa90 Compare May 30, 2023 13:38
@t-woerner t-woerner self-requested a review May 30, 2023 13:39
@t-woerner
Copy link
Member

This PR should only be merged after the FreeIPA PR freeipa/freeipa#6620 has been merged.

@rjeffman
Copy link
Member Author

rjeffman commented Aug 30, 2023 •
edited
Loading

PR freeipa/freeipa#6620 has been merged. @t-woerner can we resume review/fix to merge this one?

t-woerner
t-woerner reviewed Sep 8, 2023
View reviewed changes
roles/ipareplica/library/ipareplica_prepare.py Outdated
msg="CA cert file is not available! Please reinstall"
"the client and try again.")
else:
if is_ipa_client_configured(on_master=True):
Copy link
Member

@t-woerner t-woerner Sep 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

is_ipa_client_configured could be None according to ansible_ipa_replica.py. When is is_ipa_client_configured defined in ipalib.facts?
There should be a check to make sure that it is not None before it is used this way.

@rjeffman
ipareplica: Refactor CA file handling in replica deployment
124f801 
The call `install_ca_cert()` is not used in FreeIPA and is to be
removed in the near future (freeipa/freeipa#6620).

ipareplica can be modified to only use the function once it is
available, otherwise, `ipa-certupdate` will be used during replica
prepare.
@rjeffman rjeffman force-pushed the ipareplica_remove_install_ca_cert branch from eafaa90 to 124f801 Compare September 11, 2023 15:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@t-woerner t-woerner t-woerner requested changes

@varunmylaraiah varunmylaraiah varunmylaraiah approved these changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rjeffman @t-woerner @varunmylaraiah