git: when `revision` is set, owner/group ownership is enforced even in noop mode

[bugfood]

Describe the Bug

For a vcsrepo resource that has the revision parameter and the owner and/or group parameters set, vcsrepo will change the ownership of an existing directory even when the --noop option is set.

Expected Behavior

A noop run should make no changes to the target system.

Steps to Reproduce

This is an example for the group parameter; the owner parameter behaves the same way.

1. Create a test.pp file with contents:

vcsrepo { '/tmp/repo':
    ensure   => 'present',
    user     => 'root',
    group    => 'root',
    provider => 'git',
    source   => 'https://github.com/puppetlabs/puppetlabs-vcsrepo.git',
    revision => 'main',
}

2. Apply the file; example output:

$ sudo puppet apply test.pp
Notice: Compiled catalog for <redacted> in environment production in 0.11 seconds
Notice: /Stage[main]/Main/Vcsrepo[/tmp/repo]/ensure: created
Notice: Applied catalog in 0.83 seconds

3. Check ownership of the new repo directory. Change ownership and check again to ensure the change was made; example output:

$ ls -ld /tmp/repo
drwxr-xr-x 10 root root 4096 Dec  8 22:58 /tmp/repo
$ sudo chown -R :nobody /tmp/repo
$ ls -ld /tmp/repo
drwxr-xr-x 10 root nobody 4096 Dec  8 22:58 /tmp/repo

4. Do a noop run. Example output:

$ sudo puppet apply test.pp --noop
Notice: Compiled catalog for <redacted> in environment production in 0.10 seconds
Notice: Applied catalog in 0.72 seconds

5. Check the ownership again, which should be unchanged (still nobody as the group).

$ ls -ld /tmp/repo
drwxr-xr-x 10 root root 4096 Dec  8 22:58 /tmp/repo

Environment

• Vcsrepo current main: 3d78c5b129e4758a12df7e36a009bdc305509dbd
• AlmaLinux 8.7
• Puppet 7.14.0

Additional Context

• Removing the revision parameter makes this problem go away.
• The ownership change is recursive--not just to the top-level directory.

Thanks,
Corey

[bugfood]

This appears to be the set_ownership method; I added a raise to my local code so that I could get a stack trace. Here is the pertinent part:

/var/lib/puppet/lib/puppet/provider/vcsrepo.rb:22:in `set_ownership'
/var/lib/puppet/lib/puppet/provider/vcsrepo/git.rb:603:in `update_owner_and_excludes'
/var/lib/puppet/lib/puppet/provider/vcsrepo/git.rb:242:in `block in update_references'
/var/lib/puppet/lib/puppet/provider/vcsrepo.rb:53:in `block in at_path'
/var/lib/puppet/lib/puppet/provider/vcsrepo.rb:52:in `chdir'
/var/lib/puppet/lib/puppet/provider/vcsrepo.rb:52:in `at_path'
/var/lib/puppet/lib/puppet/provider/vcsrepo/git.rb:239:in `update_references'
/var/lib/puppet/lib/puppet/provider/vcsrepo/git.rb:578:in `get_revision'
/var/lib/puppet/lib/puppet/provider/vcsrepo/git.rb:76:in `revision'

I think the key line for this is:

puppetlabs-vcsrepo/lib/puppet/provider/vcsrepo/git.rb

Line 242 in 3d78c5b

update_owner_and_excludes

To reproduce the method here:

  def update_references
    fetch_tags_args = ['fetch', '--tags']
    git_ver = git_version
    if Gem::Version.new(git_ver) >= Gem::Version.new('2.20.0')
      fetch_tags_args.push('--force')
    end
    at_path do
      git_remote_action('fetch', @resource.value(:remote))
      git_remote_action(*fetch_tags_args, @resource.value(:remote))
      update_owner_and_excludes
    end
  end

All that runs under noop as part of getting the revision.

I think it's fair that vcsrepo does a git fetch within a noop; technically, this does alter the target system, but I think it is necessary in order for vcsrepo to be able to know if it needs to change anything, and the internal workings of git are abstracted away when vcsrepo is in charge of managing the repo.

I don't think that updating the owner of the repo is a good thing to do within a noop; this changes the working copy and is definitely a visible effect that can have ramifications to whatever users and programs are accessing the repo.

I don't know about "excludes" either way; I don't use that feature.

-Corey

[bugfood]

This line was introduced in:
328d88e

Based on the commit message there, it seems like removing update_owner_and_excludes from update_references could introduce a regression.

Perhaps vcsrepo should only change the ownership within the .git dir (with consideration for bare repos) within update_references. That wouldn't be perfect--a noop change could still affect all of .git, but at least it wouldn't affect the working copy. I haven't thought of a better suggestion, though.

-Corey