puppetlabs / puppetlabs-apt

Puppet module to help manage Apt
https://forge.puppetlabs.com/puppetlabs/apt
Apache License 2.0
215 stars 460 forks source link

Keyring File Modification Date Incorrectly Updated #1196

Open miluxhd opened 1 month ago

miluxhd commented 1 month ago

Bug

The keyring file /etc/apt/keyrings/percona unexpectedly has its modification date (mtime) altered every time the Puppet catalog is applied. this behavior is inconsistent with expected behavior, where the content should only change if the file content is actually modified.

Notice: /Stage[main]/Server::Apt/Apt::Source[percona80]/Apt::Keyring[percona]/File[/etc/apt/keyrings/percona]/content: 

Notice: /Stage[main]/Server::Apt/Apt::Source[percona80]/Apt::Keyring[percona]/File[/etc/apt/keyrings/percona]/content: content changed '{mtime}2024-09-04 16:18:08 +0200' to '{mtime}2024-09-04 16:22:42 +0200'
Notice: Applied catalog in 2.25 seconds

Steps to Reproduce

Apply the Puppet catalog that includes the server::apt::sources configuration.

class server::apt($sources = {}) {

  if ! empty($sources) {
    create_resources('apt::source',$sources)
  }
}
server::apt::sources:
  percona80:
    location: 'http://repo.percona.com/ps-80/apt'
    repos: 'main'
    key:
      name: 'percona'
      source: 'https://raw.githubusercontent.com/percona/percona-repositories/main/deb/percona-keyring.gpg'
    include:
      deb: true

Environment

Additional Context

It doesn't show changes when I change the timestamp of key files to future.

touch -d "2124-08-29 12:53:46 +0200" /etc/apt/keyrings/*

kenyon commented 1 month ago

GitHub.com doesn't provide any of the HTTP headers that Puppet's file resource type needs to determine whether the file content is synchronized: https://www.puppet.com/docs/puppet/8/types/file#file-attribute-source

You would need to provide the checksum, but this needs to be passed through to here, so we need a pull request: https://github.com/puppetlabs/puppetlabs-apt/blob/9b6aa36322710bb56b38e6b49b7d8a2d7faa22e8/manifests/keyring.pp#L54-L61

By the way, no need to use create_resources, you can specify apt::sources in hiera.

NeatNerdPrime commented 1 month ago

Hello,

I've written the PR as suggested by @kenyon

https://github.com/puppetlabs/puppetlabs-apt/pull/1199

But sadly, it did not do the intended fix. It does work in the sense that apt::keyring accepts the newly added parameters, but they don't seem to trigger the intent to the File resource.

I believe this also would require to alter the puppet File type itself. That is a little bit beyond my ruby/puppet core skills, but perhaps somewhere starting here? https://github.com/puppetlabs/puppet/blob/dc0e3baae305a6d25164771661edb742ede656ee/lib/puppet/type/file.rb#L424 or here. https://github.com/puppetlabs/puppet/blob/dc0e3baae305a6d25164771661edb742ede656ee/lib/puppet/file_serving/http_metadata.rb#L28

smortex commented 1 month ago

You would need to provide the checksum, but this needs to be passed through to here, so we need a pull request:

Hum, doing so:

  1. when the file is updated in the repo, it will never be catch by Puppet unless someone think about re-computing the checksum;
  2. when a new systems gets classified after an update of the file in the repo, the new file not matching the old checksum, configuration will never converge.

That looks more like a workaround rather than a fix that would be to serve the file from some reliable/dependable source where some sane headers can be used to make sure a file is up-to-date or not.

NeatNerdPrime commented 1 month ago

That looks more like a workaround rather than a fix that would be to serve the file from some reliable/dependable source where some sane headers can be used to make sure a file is up-to-date or not.

Sadly not every source maintainer is doing that effort ( e.g. packager.io - which caused me to look up solutions to this issue in the first place). We cannot depend/rely on the assumption that the source will provide this.

I agree it's a workaround, and that the real fix is at the source, however for the sake of idempotency and thus not causing needless changes and their follwing triggers (This basically causes apt_update to trigger) every single time, I would sure welcome at least the means to work around it in a clean way.

NeatNerdPrime commented 1 month ago

Oh, sorry, BTW:

https://github.com/puppetlabs/puppetlabs-apt/pull/1199