Page MenuHomePhabricator

Puppet git::clone probably does not need `umask` parameter
Open, Needs TriagePublic

Description

On a review of change to git::clone ( Gerrit #927750) I am wondering whether the umask parameter needs to be exposed or even used at all.

The first implementation from March 2014 at 74cd7ade251549fa89b3280ae237ad47c95011ae which originally used a umask parameter and git config core.sharedRepository=umask. Then some hours after on the same day 884c794e160b6be7c43006d6ddaef1b4c1c2df3c introduced core.sharedRepository=group .

Followed up in January 2015 with some more umask fix via 032c07b7c0060200bd1f5816f575d1f0465fd524

If git::clone has 'shared => true` then the previously cloned repository has in its config core.sharedRepository=group which cause git to set files and objects group-writable. A local test using git -c core.sharedRepository=group shows all files/directories are group writable.

Thus I think umask is a left over and does not serve any purpose since beside a normal clone, the sole other use case I imagine is a group shared repo.

From a grep in Puppet I found TWO usage:

modules/profile/manifests/local_dev/docker_publish.pp
git::clone { 'releng/dev-images':
    directory => '/srv/dev-images',
    owner     => 'root',
    group     => 'wikidev',
    mode      => '0775',
    umask     => '002',
    origin    => 'https://gitlab.wikimedia.org/releng/dev-images.git'
}

It comes from 3a8854054c1c6ae409f776c076d1f28cc2b980e5 and I think should instead use shared => true.

modules/profile/manifests/zuul/server.pp
git::clone { 'integration/config':
    directory => '/etc/zuul/wikimedia',
    owner     => 'zuul',
    group     => 'zuul',
    mode      => '0775',
    umask     => '002',
    origin    => 'https://gerrit.wikimedia.org/r/integration/config.git',
    branch    => $conf_server['config_git_branch'],
}

Umask comes from the previously mentioned January 2015 commit 032c07b7c0060200bd1f5816f575d1f0465fd524. Pretty sure it can be changed to shared => true as well. That repository is very old and probably even predate git::clone.

Both repositories are managed and primarily used by Release-Engineering-Team so I guess the impact is rather limited.

So we should:

  • switch both to use shared => true
  • remove the umask parameter from git::clone

Event Timeline

Change 927980 had a related patch set uploaded (by Hashar; author: Hashar):

[operations/puppet@production] zuul: remove mode/umask from config git clone

https://gerrit.wikimedia.org/r/927980

Change 927975 had a related patch set uploaded (by Hashar; author: Hashar):

[operations/puppet@production] contint: build dev-images with a system user

https://gerrit.wikimedia.org/r/927975

Change 927986 had a related patch set uploaded (by Hashar; author: Hashar):

[operations/puppet@production] git: remove mode and umask from git::clone

https://gerrit.wikimedia.org/r/927986

Change 927980 merged by Dzahn:

[operations/puppet@production] zuul: remove mode/umask from config git clone

https://gerrit.wikimedia.org/r/927980

Mentioned in SAL (#wikimedia-operations) [2023-06-28T19:13:49Z] <mutante> contint1002,2002,2001 - sudo chmod -R g-w /etc/zuul/wikimedia with deploying gerrit:927980 for T338277

deployed change to /etc/zuul/wikimedia on contint2002, contint1002, finally contint2001. zuul class does not use umask parameter anymore for this.

manually removed group writable bit.

This comment was removed by dancy.

Change 927975 merged by Clément Goubert:

[operations/puppet@production] contint: build dev-images with a system user

https://gerrit.wikimedia.org/r/927975

Change 989463 had a related patch set uploaded (by Hashar; author: Hashar):

[operations/puppet@production] stewards: remove umask parameter from git::clone

https://gerrit.wikimedia.org/r/989463

Change 989463 merged by Dzahn:

[operations/puppet@production] stewards: remove umask parameter from git::clone

https://gerrit.wikimedia.org/r/989463