(MODULES-9695) Debian: use modern APT keyring format

kenyon commented 8 months ago

This updates puppet_agent::osfamily::debian to use modern APT keyrings (https://github.com/puppetlabs/puppetlabs-apt/pull/1128) instead of the deprecated apt-key method used by apt::key and apt::source.key without name.

For this to work properly, it needs a release of puppetlabs-apt which contains https://github.com/puppetlabs/puppetlabs-apt/pull/1128, which should be the release after v9.1.0.

This also removes the legacy key, because keys not used for signing package sources aren't needed.

/etc/pki is not needed anymore (also this directory is a RedHatism) because keyrings are now stored in the default location of /etc/apt/keyrings. We don't clean it up though, in case people are using the files there for something else.

kenyon commented 8 months ago

The commit message check wants only a PA jira issue ID, but the issue for this is in the MODULES jira project. Caused by #642.

mhashizume commented 8 months ago

Thanks for this @kenyon , could you rebase to pick up the changes from https://github.com/puppetlabs/puppetlabs-puppet_agent/pull/682 ?

kenyon commented 8 months ago

@mhashizume already did that. That's what the last force push was. Commit e48620a6c61c5c4c753d817eb8004a753c4bb887 is on top of current main.

kenyon commented 8 months ago

FWIW I'm using this plus puppetlabs-apt at https://github.com/puppetlabs/puppetlabs-apt/commit/cb21d0b93f3b44130adef034b58ef3f2193640e3 on my infrastructure, and it does the right thing on Ubuntu 18.04, 20.04, and 22.04.

kenyon commented 8 months ago

Modern APT keyring format also needs to be used in puppet_enterprise::repo::config, which doesn't use puppetlabs-apt. Bug report: https://github.com/puppetlabs/puppet-enterprise_issues/issues/2

kenyon commented 7 months ago

I'm not seeing why the Debian tests would be running on Windows agents, and why my changes would cause these test failures.

janit42 commented 5 months ago

as we have nightly puppet-agent packages for Debian 12 now it would be great to have this merged if possible

joshcooper commented 4 months ago

@kenyon can you resolve the test failures so we can get this merged?

kenyon commented 4 months ago

@kenyon can you resolve the test failures so we can get this merged?

I'd like to, but like I mentioned in https://github.com/puppetlabs/puppetlabs-puppet_agent/pull/681#issuecomment-1852750322, I investigated and couldn't figure out why these changes have anything to do with Windows tests. So I'll need a little assistance.

kenyon commented 4 months ago

My guess is that something more fundamental is wrong with the tests, like there are tests executing on platforms that they shouldn't be executing on.

linuxdaemon commented 2 months ago

My guess is that something more fundamental is wrong with the tests, like there are tests executing on platforms that they shouldn't be executing on.

Looking at the log, the current failures look to be dependency related. It is happening in main as well, so it seems to be an issue with the Gemfile.

mhashizume commented 4 weeks ago

Thanks for all of your work on this @kenyon , would it make sense to bump the puppetlabs-apt requirement in metadata.json since this is only supported in >= 9.2.0?

kenyon commented 4 weeks ago

Thanks for all of your work on this @kenyon , would it make sense to bump the puppetlabs-apt requirement in metadata.json since this is only supported in >= 9.2.0?

@mhashizume good catch (I started this PR before puppetlabs-apt 9.2.0 was released 😄). Updated metadata.json.

mhashizume commented 3 weeks ago

We'll also want to update puppetlab-apt in tests here: https://github.com/puppetlabs/puppetlabs-puppet_agent/blob/3ce04a2b0504b00bbd8f753991f3820887fa426a/acceptance/helpers.rb#L182

Edit: And here https://github.com/puppetlabs/puppetlabs-puppet_agent/blob/3ce04a2b0504b00bbd8f753991f3820887fa426a/spec/spec_helper_acceptance.rb#L119

puppetlabs-apt is also pinned to an older version in our various dockerfiles.

kenyon commented 3 weeks ago

@mhashizume I updated the versions in those helpers. The Dockerfiles don't specify versions (there are some branch specifications, but those lines are commented out).

mhashizume commented 3 weeks ago

@kenyon A few more things:

After bumping puppetlabs-apt in metadata.json and helpers, we should also bump its dependency, stdlib (to >= 9.0.0). Because inifile depends on stdlib, we should also bump it to >= 6.0.0 in metadata.json and helpers.
With your PR and the above changes, I ran into errors with our upgrade tests from Puppet 6 to Puppet 7. I believe the issue is that if there are multiple Puppet repos (like for different major version) and one of them does not include the signed-by option, apt errors:

E: Conflicting values set for option Signed-By regarding source http://nightlies.puppet.com/apt/ buster: /etc/apt/keyrings/GPG-KEY-puppet-20250406.asc !=

I'm not sure what the best solution is for the second issue, if we need to update anything on our end or if we just need to update documentation to let users know to update their repo configuration to point to the key.

Thoughts?

kenyon commented 3 weeks ago

@mhashizume I updated the metadata. Without looking at it myself, I'm not sure either what to do about that upgrade issue. I still have one PE 2019 instance that I need to upgrade, but the puppet_agent and apt modules would remain on Puppet 6-compatible versions until after the upgrade to Puppet 7. So I don't think it would be a problem. Which brings up this line:

https://github.com/puppetlabs/puppetlabs-puppet_agent/blob/3da119fde89d55ce1e35349cbcb1050fd4f5f932/metadata.json#L75-L78

IMO this should be bumped to require Puppet 7 since the apt, stdlib, and inifile dependencies require it. Should I make that change too?

kenyon commented 3 weeks ago

@mhashizume I added a commit that bumps the Puppet version requirement to 7. I think this is the only safe way to go if the dependent modules require Puppet 7. Otherwise there is no guarantee that the Ruby interpreter used in Puppet 5 or 6 would work with the Ruby code in these newer modules, for example.

mhashizume commented 2 weeks ago

The bump makes sense to me. I found a few more things that needed to be updated:

inifile needs to be a minimum of 6.1.0 to work with newer stdlib
a hacky change to the tests so there aren't conflicts that make apt sad

You can see what I did in addition to what you have here in this commit: https://github.com/puppetlabs/puppetlabs-puppet_agent/commit/627d06c966843c658f7b796ffd6a4d1b12d87ea1

I think after you incorporate those changes, this PR should be good to go.

kenyon commented 2 weeks ago

@mhashizume done.

Not sure where the proper fix would go for that acceptance helper, maybe around here: https://github.com/puppetlabs/beaker-puppet/blob/1146cb9783f74812421ebd52dc556f68710c57fe/lib/beaker-puppet/install_utils/foss_utils.rb#L1091

mhashizume commented 2 weeks ago

Thanks again @kenyon ! Really appreciate all your work.

I'm not sure what the upstream fix is for the issue we had to work around in the acceptance helper. The fundamental issue is that we had two sources.list with the same URL, but only one had the signed-by option enabled. The sources.list without the signed-by option is our release package installed by beaker-puppet. Our release packages don't use apt-key (AFAIK) and doesn't otherwise cause any warnings or errors with apt, so I don't think we need to change anything with beaker-puppet or our release packages.

Maybe we should document this circumstance somewhere, but I'm not sure where makes the most sense (and I'm not sure how often users will run into it).

puppetlabs / puppetlabs-puppet_agent

(MODULES-9695) Debian: use modern APT keyring format #681