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

Keyring File Modification Date Incorrectly Updated #1196

Open
miluxhd opened this issue Sep 4, 2024 · 7 comments
Open

Keyring File Modification Date Incorrectly Updated #1196

miluxhd opened this issue Sep 4, 2024 · 7 comments

Comments

@miluxhd
Copy link

miluxhd commented Sep 4, 2024

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

  • puppet version: 8.8.1
  • Platform: Debian

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
Copy link

kenyon commented Sep 4, 2024

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:

file { $file:
ensure => 'file',
mode => $mode,
owner => 'root',
group => 'root',
source => $source,
content => $content,
}

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

NeatNerdPrime added a commit to NeatNerdPrime/puppetlabs-apt that referenced this issue Sep 7, 2024
puppetlabs#1196

* Adds checksum and checksum_value parameter to apt::keyring, this should
  address issue/1196 as commented here puppetlabs#1196 (comment)
* Includes tests, all green.
@NeatNerdPrime
Copy link

NeatNerdPrime commented Sep 7, 2024

Hello,

I've written the PR as suggested by @kenyon

#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
Copy link
Collaborator

smortex commented Sep 9, 2024

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 added a commit to NeatNerdPrime/puppetlabs-apt that referenced this issue Sep 9, 2024
puppetlabs#1196

* Adds checksum and checksum_value parameter to apt::keyring, this should
  address issue/1196 as commented here puppetlabs#1196 (comment)
* Includes tests, all green.
@NeatNerdPrime
Copy link

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
Copy link

Oh, sorry, BTW:

#1199

@jay7x
Copy link

jay7x commented Dec 7, 2024

Meanwhile, I'm starting to think that Etag support can be useful in the File resource. As long as there is no better way, but Etag header is present (and it's the same as recorded), it's ok to assume that file was not changed on remote side.

@kenyon
Copy link

kenyon commented Dec 7, 2024

@jay7x there was some discussion of that in puppetlabs/puppet#9319.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants