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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

(PE-36580) Add r10k_known_hosts to install plan #380

Merged
merged 3 commits into from Sep 14, 2023

Conversation

jpartlow
Copy link
Contributor

@jpartlow jpartlow commented Sep 5, 2023

Summary

Starting with PE 2023.3, updates to r10k libraries from PE-35980 changes in libgit2 now verify host keys. Because of this code manager now needs to be configured with known hosts public key information for the r10k remote host. Without this, PE will install but code manager will fail to deploy.

This patch will fail the plan early if installing PE 2023.3+, with r10k_private_key (so using ssh protocol) and no r10k_known_hosts array.

Related Issues (if any)

Checklist

  • 馃煝 Spec tests.
  • 馃煝 Acceptance tests.

Changes include test coverage?

  • Yes
  • Not needed

Have you updated the documentation?

  • Yes, I've updated the appropriate docs
  • Not needed

@jpartlow jpartlow requested review from a team as code owners September 5, 2023 17:33
Starting with PE 2023.3, updates to r10k libraries from PE-35980 changes
in libgit2 now verify host keys. Because of this code manager now needs
to be configured with known hosts public key information for the r10k
remote host. Without this, PE will install but code manager will fail to
deploy.

This patch will fail the plan early if installing PE 2023.3+, with
r10k_private_key (so using ssh protocol) and no r10k_known_hosts array.
@jpartlow jpartlow force-pushed the pe-36580-add-r10k-known-hosts branch from 83db9ed to 85ff769 Compare September 5, 2023 17:38
plans/subplans/install.pp Outdated Show resolved Hide resolved
'version' => '2023.3.0',
'r10k_remote' => 'git@github.com:puppetlabs/nothing',
'r10k_private_key_content' => '-----BEGINfoo',
'permit_unsafe_versions' => true,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also I didn't update safe versions to include 2023.3 as I'm not sure what the release process is for that.

Since we aren't testing whether r10k_remote using git@ or ssh:// also
has r10k_private_key set, testing for r10k_known_hosts seems like
overkill. Removing that plan_fail, and setting r10k_known_hosts directly
in the generate_pe_conf call alongside the other r10k parameters.

It's true that this parameter only exists in 2023.3+, but
peadm::generate_pe_conf() will drop parameters with undef values, and if
r10k_known_hosts is explicitly set by mistake by a user on an earlier
version than 2023.3, an unmatched parameter in pe.conf hiera is not
fatal.
@jpartlow
Copy link
Contributor Author

jpartlow commented Sep 7, 2023

I removed the version test and fail_plan() for missing r10k_known_hosts from the install plan since we don't test for setting r10k_private_key, for example.

On the upgrade side, we could have the upgrade plan emit a warning if you are upgrading to 2023.3+ and have an r10k_remote =~ /^(?:\w+@|ssh:)/ as Justin mentioned, but no r10k_known_hosts set.

Could also provide a peadm::upgrade::r10k_known_hosts parameter and update pe.conf if it's set.

@CoMfUcIoS
Copy link
Contributor

don't we need to update the docs for this change too?

@jpartlow
Copy link
Contributor Author

jpartlow commented Sep 14, 2023

I'll add a note to the peadm::subplans::install::r10k_known_hosts @param doc to go to the Puppet Enterprise 2023.3+ Configure Code Manager documentation for more details.

Updated.

@jpartlow jpartlow merged commit 2b90756 into puppetlabs:main Sep 14, 2023
52 checks passed
@jpartlow jpartlow deleted the pe-36580-add-r10k-known-hosts branch September 14, 2023 20:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants