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

freeipa.spec: depend on bind-pkcs11-utils #6074

Closed
wants to merge 1 commit into from
Closed

freeipa.spec: depend on bind-pkcs11-utils #6074

wants to merge 1 commit into from

Conversation

fcami
Copy link
Contributor

@fcami fcami commented Nov 4, 2021

The OpenDNSSec integration code requires:
/usr/sbin/dnssec-keyfromlabel-pkcs11
which is provided by bind-pkcs11-utils.
Currently, bind-pkcs11-utils is only installed for RHEL<9.
With this change, FreeIPA depends on bind-pkcs11-utils on all
Fedora and RHEL versions.

Fixes: https://pagure.io/freeipa/issue/9026
Signed-off-by: François Cami fcami@redhat.com

@fcami fcami added ipa-4-9 Mark for backport to ipa 4.9 needs review Pull Request is waiting for a review labels Nov 4, 2021
@abbra
Copy link
Contributor

abbra commented Nov 4, 2021

I think this is an incorrect change. We should not install bind-pkcs11 if only bind-pkcs11-utils is needed.
Perhaps, instead of redefining a location of %global with_bind_pkcs11 1, we should simply require bind-pkcs11-utils unconditionally as it only needs bind-pkcs11-libs.

So, in the package definition below, move Requires: Requires: bind-pkcs11-utils >= %{bind_version} outside of the conditional check.

%package server-dns
Summary: IPA integrated DNS server with support for automatic DNSSEC signing
BuildArch: noarch
Requires: %{name}-server = %{version}-%{release}
Requires: bind-dyndb-ldap >= 11.2-2
Requires: bind >= %{bind_version}
Requires: bind-utils >= %{bind_version}
%if %{with bind_pkcs11}
Requires: bind-pkcs11 >= %{bind_version}
Requires: bind-pkcs11-utils >= %{bind_version}
%else
Requires: softhsm >= %{softhsm_version}
Requires: openssl-pkcs11 >= %{openssl_pkcs11_version}
%endif
# See https://bugzilla.redhat.com/show_bug.cgi?id=1825812
# RHEL 8.3+ and Fedora 32+ have 2.1
Requires: opendnssec >= 2.1.6-5
%{?systemd_requires}

@abbra
Copy link
Contributor

abbra commented Nov 4, 2021

Also, a commit message implies that the change applies to all Fedora and all RHEL versions, it could be clarify it.

@flo-renaud
Copy link
Contributor

+1 for @abbra 's proposal:

So, in the package definition below, move Requires: Requires: bind-pkcs11-utils >= %{bind_version} outside of the conditional check.

@flo-renaud flo-renaud added ack Pull Request approved, can be merged and removed needs review Pull Request is waiting for a review labels Nov 4, 2021
@fcami
Copy link
Contributor Author

fcami commented Nov 5, 2021

hi @flo-renaud and @abbra thanks for the reviews (and ack).
@gkaihorodova @miskopo would you have the time to check whether this fixes the problem you've been seeing?

@fcami fcami removed the ack Pull Request approved, can be merged label Nov 5, 2021
@fcami
Copy link
Contributor Author

fcami commented Nov 5, 2021

Removing ACK, I am going to push a different fix.

@fcami fcami added ipa-next Mark as master (4.12) only and removed ipa-4-9 Mark for backport to ipa 4.9 labels Nov 5, 2021
@fcami
Copy link
Contributor Author

fcami commented Nov 5, 2021

Marking as ipa-next (ipa-4-10) only.
This change MUST not go into ipa-4-9 (clearly not into RHEL8).

@fcami fcami added needs review Pull Request is waiting for a review WIP Work in progress - not ready yet for review and removed needs review Pull Request is waiting for a review labels Nov 5, 2021
@abbra
Copy link
Contributor

abbra commented Nov 5, 2021

Looking at your latest change, why can't we do runtime detection of the tool and use it accordingly? Then it will work for both old and new variants.

@fcami
Copy link
Contributor Author

fcami commented Nov 5, 2021

Online detection would be overkill for such an issue.
I'd argue the problem lies downstream since RHEL8 and RHEL9 want different tools, but that means we either have to use different release branches (that will be necessary at some point anyway) or carry a downstream patch for either RHEL8 or RHEL9.

@rcritten
Copy link
Contributor

rcritten commented Nov 5, 2021

@fcami snuck his comment while I was typing basically the same thing. This seems like a lot of work that could be simplified in a downstream patch, unless @stanislavlevin or @tjaalton are also seeing this problem with their distros where bind ships with different tool names to do the same thing.

@fcami fcami added needs review Pull Request is waiting for a review and removed WIP Work in progress - not ready yet for review labels Nov 5, 2021
@fcami
Copy link
Contributor Author

fcami commented Nov 6, 2021

So, to clarify, this is what is done now:

freeipa.spec: depend on bind-dnssec-utils

The OpenDNSSec integration code requires:
/usr/sbin/dnssec-keyfromlabel-pkcs11
which is provided by bind-pkcs11-utils, but that package is
only available on RHEL<9.

With this change, freeipa-server-dns depends on bind-dnssec-utils
on all Fedora releases and RHEL==9+, and uses:
/usr/sbin/dnssec-keyfromlabel -E pkcs11
instead of dnssec-keyfromlabel-pkcs11.

@abbra
Copy link
Contributor

abbra commented Nov 7, 2021

I am concerned by this change. It means we have to branch for no other reason than this change. Why we cannot detect the correct tool runtime? The cost of branching and maintaining backports is much higher than adding a runtime detection for the tool.

@gkaihorodova gkaihorodova removed their request for review November 8, 2021 09:59
@fcami
Copy link
Contributor Author

fcami commented Nov 15, 2021

I respectfully disagree.

More changes like this one are bound to be required given RHEL 8 and RHEL 9 will fast diverge.
Having an ipa-4-10 branch would make differential maintenance for Fedora 36 and 35 easier too instead of what's happening for Fedora 34, where select backports are necessary rather than rebases.
However, if maintaining ipa-4-10 upstream is not palatable to the team, maintaining this change as a patch (or reverse patch for RHEL 8) in a downstream dist-git seems straightforward enough to me. Am I missing anything?

Moreover, detecting the correct tool at runtime means the downstream code path will be present but not exercised nor tested in our current upstream tests unless OpenDNSSec tests are run twice, each time with a different set of installed packages. I would argue this is hardly necessary and a waste of upstream resources.

With that said, if you really want to implement that detection, both tools are in Fedora:

# cat /etc/redhat-release 
Fedora release 36 (Rawhide)

# dnf provides "dnssec-keyfromlabel-pkcs11"
bind-pkcs11-utils-32:9.16.22-2.fc36.x86_64 : Bind tools with native PKCS#11 for using DNSSEC
Repo        : rawhide
Matched from:
Filename    : /usr/sbin/dnssec-keyfromlabel-pkcs11

# dnf provides "dnssec-keyfromlabel"
bind-dnssec-utils-32:9.16.22-2.fc36.x86_64 : DNSSEC keys and zones management utilities
Repo        : rawhide
Matched from:
Filename    : /usr/sbin/dnssec-keyfromlabel

And both tools work equally fine there, making this a 100% downstream problem.

@tjaalton
Copy link
Contributor

tjaalton commented Nov 15, 2021

@fcami snuck his comment while I was typing basically the same thing. This seems like a lot of work that could be simplified in a downstream patch, unless @stanislavlevin or @tjaalton are also seeing this problem with their distros where bind ships with different tool names to do the same thing.

Hmm right, looks like Debian would need the same value as Fedora for DNSSEC_KEYFROMLABEL. I need to only worry about the bind9 >= 9.16 packaging which ships /usr/sbin/dnssec-keyfromlabel.

EDIT: heh, so this change would modify base/paths.py so that it would work on Debian as well, so ack from me :)

@fcami
Copy link
Contributor Author

fcami commented Nov 16, 2021

Thanks for the review @tjaalton !

@fcami fcami added ipa-4-9 Mark for backport to ipa 4.9 and removed ipa-next Mark as master (4.12) only labels Nov 25, 2021
@antoniotorresm
Copy link
Contributor

Team agreed to merge this and have a reverse patch for RHEL8.

@antoniotorresm antoniotorresm added ack Pull Request approved, can be merged and removed needs review Pull Request is waiting for a review labels Nov 25, 2021
The OpenDNSSec integration code requires:
/usr/sbin/dnssec-keyfromlabel-pkcs11
which is provided by bind-pkcs11-utils, but that package is
only available on RHEL<9.

With this change, freeipa-server-dns depends on bind-dnssec-utils
on all Fedora releases and RHEL==9+, and uses:
/usr/sbin/dnssec-keyfromlabel -E pkcs11
instead of dnssec-keyfromlabel-pkcs11.

Fixes: https://pagure.io/freeipa/issue/9026
Signed-off-by: François Cami <fcami@redhat.com>
@fcami fcami added the pushed Pull Request has already been pushed label Nov 25, 2021
@fcami
Copy link
Contributor Author

fcami commented Nov 25, 2021

master:

  • 20f68d8 freeipa.spec: depend on bind-dnssec-utils

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-4-9 Mark for backport to ipa 4.9 pushed Pull Request has already been pushed
Projects
None yet
7 participants