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

Refactor CA file handling in replica installer #6620

Closed
wants to merge 1 commit into from

Conversation

tiran
Copy link
Member

@tiran tiran commented Jan 12, 2023

Clean up and remove obsolete code from ipa-replica-install. For several versions replica installer first ensures that a host is an IPA client, then promotes the client to a replica. The client installer code sets up CA stores like IPA_CA_CRT already.

Related: https://pagure.io/freeipa/issue/9272

@tiran tiran added the re-run Trigger a new run of PR-CI label Jan 12, 2023
@freeipa-pr-ci freeipa-pr-ci removed the re-run Trigger a new run of PR-CI label Jan 12, 2023
@tiran tiran added ipa-4-9 Mark for backport to ipa 4.9 ipa-4-10 Mark for backport to ipa 4.10 labels Jan 12, 2023
@tiran tiran mentioned this pull request Jan 12, 2023
4 tasks
Clean up and remove obsolete code from ipa-replica-install. For several
versions replica installer first ensures that a host is an IPA client,
then promotes the client to a replica. The client installer code sets up
CA stores like IPA_CA_CRT already.

Related: https://pagure.io/freeipa/issue/9272
@tiran
Copy link
Member Author

tiran commented Jan 12, 2023

install_ca_cert is used by ansible-freeipa. Thomas Wörner recommended that I remove the function so ansible-freeipa will be able to detect the change more easily.

@rcritten
Copy link
Contributor

The change works for me. Is it important to backport this to ipa-4-9/10?

@tiran
Copy link
Member Author

tiran commented Jan 25, 2023

I don't mind if you don't backport the change for now. It's a prerequisite for ticket 9272, but HMSIDM doesn't need the feature backported at the moment. I developed some hacks to work around the problem.

@tiran tiran added ipa-next Mark as master (4.13) only and removed ipa-4-9 Mark for backport to ipa 4.9 ipa-4-10 Mark for backport to ipa 4.10 labels Jan 25, 2023
@rcritten
Copy link
Contributor

@t-woerner are we ok merging this?

@rcritten
Copy link
Contributor

@t-woerner double-checking, cool to merge to master branch?

@rcritten
Copy link
Contributor

rcritten commented May 8, 2023

@rjeffman I just want to confirm that this won't break ansible-freeipa before merging.

@@ -132,24 +132,6 @@ def install_krb(config, setup_pkinit=False, pkcs12_info=None, fstore=None):
return krb


def install_ca_cert(ldap, base_dn, realm, cafile, destfile=paths.IPA_CA_CRT):
Copy link
Member

Choose a reason for hiding this comment

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

By removing this function ansible-freeipa replica install will not work anymore.

It is my understanding that ansible-freeipa deployment role should be updated, right?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, the removal of this function is needed for ansible-freeipa to be able to detect when ipa-certupdate needs to be used instead.

rjeffman added a commit to rjeffman/ansible-freeipa that referenced this pull request May 9, 2023
Remove parameter _ca_file from ipareplica modules as the parameter is
not used.

Related to: freeipa/freeipa#6620
rjeffman added a commit to rjeffman/ansible-freeipa that referenced this pull request May 9, 2023
The call `install_ca_cert()` is not needed anymore and is to be removed
from FreeIPA (freeipa/freeipa#6620). This patch
modifies ipareplica to remove its usage from ipareplica deployment role.
@t-woerner
Copy link
Member

@t-woerner double-checking, cool to merge to master branch?

Yes, this is fine to be merged.

rjeffman added a commit to rjeffman/ansible-freeipa that referenced this pull request May 9, 2023
The call `install_ca_cert()` is not needed anymore and is to be removed
from FreeIPA (freeipa/freeipa#6620). This patch
modifies ipareplica to remove its usage from ipareplica deployment role.
rjeffman added a commit to rjeffman/ansible-freeipa that referenced this pull request May 10, 2023
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 added a commit to rjeffman/ansible-freeipa that referenced this pull request May 10, 2023
Remove parameter _ca_file from ipareplica modules as the parameter is
not used.

Related to: freeipa/freeipa#6620
rjeffman added a commit to rjeffman/ansible-freeipa that referenced this pull request May 16, 2023
Remove parameter _ca_file from ipareplica modules as the parameter is
not used.

Related to: freeipa/freeipa#6620
rjeffman added a commit to rjeffman/ansible-freeipa that referenced this pull request May 16, 2023
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 added a commit to rjeffman/ansible-freeipa that referenced this pull request May 30, 2023
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 added a commit to rjeffman/ansible-freeipa that referenced this pull request May 30, 2023
Remove parameter _ca_file from ipareplica modules as the parameter is
not used.

Related to: freeipa/freeipa#6620
@stale
Copy link

stale bot commented Aug 12, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale Stale PR [Bot] label Aug 12, 2023
@rjeffman rjeffman added the ack Pull Request approved, can be merged label Aug 14, 2023
@stale stale bot removed the stale Stale PR [Bot] label Aug 14, 2023
@rjeffman
Copy link
Member

This change is fine to be merged, isn't it? I'm providing ACK.

@rcritten rcritten added the pushed Pull Request has already been pushed label Aug 30, 2023
@rcritten
Copy link
Contributor

master:

  • 8f25b2a Refactor CA file handling in replica installer

@rcritten rcritten closed this Aug 30, 2023
@rcritten
Copy link
Contributor

@tiran I'm not 100% sure that this completely resolves the upstream ticket. If I'm wrong feel free to close it.

rjeffman added a commit to rjeffman/ansible-freeipa that referenced this pull request Sep 11, 2023
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.
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 ipa-next Mark as master (4.13) only pushed Pull Request has already been pushed
Projects
None yet
5 participants