Centralize git cli operations #4106

jedevc · 2023-08-04T15:22:38Z

Inspired by @vito's changes to the git operations in #3960 (comment)

Essentially, this helps to reduce the number of arguments to each call to git, and makes any changes less error-prone, and makes the whole thing more reusable. When this is merged, we can use it in buildx as well, where we already have a similar implementation in https://github.com/docker/buildx/blob/master/build/git.go (since docker/buildx#1297).

At the moment, this just has the basic Run operation, but I'm wondering if we want to add more utilities, as in the buildx implementation? I'll have a look and try and put together a list of utilities to include.

crazy-max · 2023-08-04T15:55:22Z

Is the idea to also port https://github.com/docker/buildx/tree/master/util/gitutil in BuildKit?

jedevc · 2023-08-04T16:04:04Z

@crazy-max I'd like to take as much as is reasonable. I don't think we should take the gitpath WSL stuff, since that doesn't make much sense to me for use in BuildKit, or the testutils.

But otherwise, I think we should take the other helpers? We should take as much as possible, but I want to avoid having any buildx-specific stuff in BuildKit as much as is possible, so we might need to leave some stuff downstream.

crazy-max · 2023-08-04T16:04:58Z

@crazy-max I'd like to take as much as is reasonable. I don't think we should take the gitpath WSL stuff, since that doesn't make much sense to me for use in BuildKit, or the testutils.

But otherwise, I think we should take the other helpers? We should take as much as possible, but I want to avoid having any buildx-specific stuff in BuildKit as much as is possible, so we might need to leave some stuff downstream.

Sounds good!

jedevc · 2023-08-16T14:34:44Z

But otherwise, I think we should take the other helpers? We should take as much as possible, but I want to avoid having any buildx-specific stuff in BuildKit as much as is possible, so we might need to leave some stuff downstream.

For now, I've just got WorkTree() and GitDir - for the others, I'm not necessarily sure - I think for now, we can keep those definitions in buildx, and upstream them later if we want.

crazy-max · 2023-08-17T08:38:36Z

source/git/source_test.go

@@ -578,7 +586,7 @@ func setupGitRepo(t *testing.T) gitRepoFixture {
 		"echo sbb > foo13",
 		"git add foo13",
 		"git commit -m third",
-		"git tag lightweight-tag",
+		"git tag --no-sign lightweight-tag",


Is it to skip GPG sign to suppress warning?

Oops, this was from https://github.com/moby/buildkit/pull/3960/files#diff-a7d654e4edba8b86b4853225da6c0e915b401becf96b20c380ac7105c3850610R585 - I don't think this needs to change.

Yeah, I added this because I was running the tests directly on my machine which has various things integrated with git for commit signing and such. Feel free to remove it if you only care about running the tests in a container.

Aha, this is a good call IMO, I'll split this to a separate commit 🎉

tonistiigi

Is this worth 300 extra lines?

Another issue is that while the previous code is "secure by default" then now, because it is in reusable package, caller needs to configure it properly every time which is more error-prone. Eg. runWithUmask is now in multiple places where previously it was just impossible to not have umask on. If it is forgotten in one caller then it is a bug that is very hard to track down. Is there a user for the new package? If not then can we take this code, make it private and maybe remove the umask/streams etc configuration. Or alternatively, if we want to keep the package make sure that source/git only makes its calls through helper functions so there is only one configuration to gitutil.

jedevc · 2023-08-24T11:01:52Z

Is this worth 300 extra lines?

I'd argue definitely yes - keep in mind, that a lot of these lines are adding the verbose GitOption patterns, along with doc-strings, and splitting initialization into multiple lines. I actually think that even though the line count is increased, maintainability is improved.

For example,

_, err := gitWithinDir(ctx, dir, "", "", "", auth, "remote", "add", "origin", remote)

becomes

git := gitCLI(
	gitutil.WithGitDir(dir),
	gitutil.WithArgs(authArgs...),
)
_, err := git.Run(ctx, "remote", "add", "origin", remote)

Yes, this is longer, but I think it's much clearer and more readable - given the security sensitivity of this code as you point out, I think it's better to be more explicit here. The previous collection of all the args as unnamed parameters is hard to read and track. In writing new code, and reviewing code, it would be easy to switch the work tree and git directories, or to pass accidental values into the known hosts.

The options add some bloat - we could just use a GitCliOpt struct, though I have a slight preference for the options pattern - but this would be roughly equivalent in line count IMO.

Is there a user for the new package?

See docker/buildx#2005 (not up-to-date with the latest changes I've made here, but it's a working demo). It doesn't make sense that we have split logic for calling into git, and we have to duplicate helpers, etc.

However, if we want to keep it private - I could be persuaded. My original idea was to have a reusable git helper that I could use both in the client and on the server to be able to do some context transfer related changes - but I'm not as interested in this anymore, so I have less need for the helper.

Or alternatively, if we want to keep the package make sure that source/git only makes its calls through helper functions so there is only one configuration to gitutil.

I've updated to do this.

thaJeztah · 2023-08-24T11:09:26Z

Yes, this is longer, but I think it's much clearer and more readable

I have not dug into the changes; just a quick question:

are there any arguments that are (or should be) required?
are there any arguments that depend on "other" arguments (so order of applying them matters?)

I love functional arguments, but for some case like above, details may matter, so just double-checking if they are taken into account (again for the security angle of things)

jedevc · 2023-08-24T11:16:56Z

are there any arguments that are (or should be) required?

I don't think so. Maybe the WithDir option at a stretch? However, if it's not set, it'll default to the current working directory, which I think is still valid and expected.

are there any arguments that depend on "other" arguments (so order of applying them matters?)

No dependents, each option just sets a single property - there's no intersection. The last applied option takes precedence, which allows overriding it later.

thaJeztah · 2023-08-24T11:20:10Z

Thanks! I'll step out of the discussion beyond that 😂 as I'm not familiar in-depth with this.

Move all of the git command line logic into a single object, inspired by the object already in buildx. The basic implemenation allows for configuring a git cli for a specific repository, along with various authorization settings and custom binaries. Commands can be run for that repository, and a few helpers are provided for accessing data on it - more to come in the future hopefully. Signed-off-by: Justin Chadwell <me@jedevc.com>

Co-authored-by: Alex Suraci <suraci.alex@gmail.com> Signed-off-by: Justin Chadwell <me@jedevc.com>

tonistiigi · 2024-06-21T06:56:40Z

source/git/source.go

@@ -528,49 +504,50 @@ func (gs *gitSourceHandler) Snapshot(ctx context.Context, g session.Group) (out
 		if err := os.MkdirAll(checkoutDir, 0711); err != nil {
 			return nil, err
 		}
-		_, err = gitWithinDir(ctx, checkoutDirGit, "", sock, knownHosts, nil, "-c", "init.defaultBranch=master", "init")
+		checkoutGit := git.New(gitutil.WithWorkTree(checkoutDir), gitutil.WithGitDir(checkoutDirGit))


If I'm understanding correctly, the simplest fix for #5066 is including gitutil.WithExec(runWithStandardUmask) here?

... I guess all instances of WithWorkTree in this file technically need/should have it?

@tianon Yes, assuming there isn't more cases like this.

But I think we need some refactoring here that would better guarantee such bugs not reappearing as described in as described in #4106 (review) and also make sure test can reliably cover it. cc @jedevc We can still take your patch as a quick mitigation.

Hm, we actually specify this option - the issue was is that we weren't correctly propagating it down to this level. Opened an alternative PR here that fixes the root cause: #5096.

jedevc mentioned this pull request Aug 4, 2023

Dynamic network configuration via session attachable #3960

Closed

4 tasks

jedevc force-pushed the git-cli-upstreaming branch from 1aa6885 to 310ea33 Compare August 4, 2023 15:26

jedevc force-pushed the git-cli-upstreaming branch 6 times, most recently from 3d2370c to 6cc6832 Compare August 16, 2023 14:33

jedevc marked this pull request as ready for review August 16, 2023 14:34

jedevc requested a review from crazy-max August 16, 2023 14:34

jedevc force-pushed the git-cli-upstreaming branch 2 times, most recently from 36cc47b to 96097aa Compare August 16, 2023 14:50

jedevc mentioned this pull request Aug 16, 2023

git: wrap upstream gitutil cli docker/buildx#2005

Draft

crazy-max reviewed Aug 17, 2023

View reviewed changes

jedevc force-pushed the git-cli-upstreaming branch from 96097aa to 6d5b061 Compare August 17, 2023 08:42

crazy-max approved these changes Aug 17, 2023

View reviewed changes

tonistiigi reviewed Aug 24, 2023

View reviewed changes

jedevc force-pushed the git-cli-upstreaming branch from dba10ef to ecd43b0 Compare August 24, 2023 10:50

jedevc force-pushed the git-cli-upstreaming branch from ecd43b0 to 4e60b1d Compare August 24, 2023 11:04

jedevc force-pushed the git-cli-upstreaming branch from 4e60b1d to 319130d Compare August 24, 2023 11:22

jedevc added 2 commits August 24, 2023 12:22

test: git tag test should not sign tags

319130d

Co-authored-by: Alex Suraci <suraci.alex@gmail.com> Signed-off-by: Justin Chadwell <me@jedevc.com>

tonistiigi approved these changes Aug 25, 2023

View reviewed changes

tonistiigi merged commit 8c7b417 into moby:master Aug 25, 2023
55 checks passed

jedevc deleted the git-cli-upstreaming branch August 25, 2023 11:57

tonistiigi mentioned this pull request Jun 21, 2024

[v0.13+] unexpected permissions on COPY'd files #5066

Closed

tonistiigi reviewed Jun 21, 2024

View reviewed changes

tianon mentioned this pull request Jun 26, 2024

Add gitutil.WithExec(runWithStandardUmask) to Git invocations #5092

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Centralize git cli operations #4106

Centralize git cli operations #4106

jedevc commented Aug 4, 2023

crazy-max commented Aug 4, 2023

jedevc commented Aug 4, 2023

crazy-max commented Aug 4, 2023

jedevc commented Aug 16, 2023

crazy-max Aug 17, 2023

jedevc Aug 17, 2023

vito Aug 17, 2023

jedevc Aug 22, 2023

tonistiigi left a comment

jedevc commented Aug 24, 2023

thaJeztah commented Aug 24, 2023

jedevc commented Aug 24, 2023

thaJeztah commented Aug 24, 2023

tonistiigi Jun 21, 2024

tianon Jun 26, 2024

tianon Jun 26, 2024

tonistiigi Jun 27, 2024

jedevc Jun 27, 2024

Centralize git cli operations #4106

Centralize git cli operations #4106

Conversation

jedevc commented Aug 4, 2023

crazy-max commented Aug 4, 2023

jedevc commented Aug 4, 2023

crazy-max commented Aug 4, 2023

jedevc commented Aug 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tonistiigi left a comment

Choose a reason for hiding this comment

jedevc commented Aug 24, 2023

thaJeztah commented Aug 24, 2023

jedevc commented Aug 24, 2023

thaJeztah commented Aug 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment