Skip to content

git: fix issue with checking out annotated tags by full ref - #6244

Merged
tonistiigi merged 1 commit into
moby:masterfrom
tonistiigi:git-annotated-fix
Sep 30, 2025
Merged

tonistiigi merged 1 commit into
moby:masterfrom
tonistiigi:git-annotated-fix

Conversation

@tonistiigi

Copy link
Copy Markdown
Member

If the tag was already pulled by --tags or without refs/tags, that creates an ambiguous reference in the shared repository that fails when fetching to the snapshot.

Example: https://github.com/moby/buildkit/actions/runs/17962081814/job/51087390963

via #2570 @alexcb @rrjjvv

Looking at this I'm also confused about #4473 . Atm it looks like we are reporting wrong SHA in provenance for annotated tags (although it seems to come from ref order in server side that may not be fully standardized). The git commit will point to the SHA of commit rather than the tag itself, meaning we have already lost the additional tag information. If we add policy validation by signature, we definitely should validate the signature on the tag, not just on the commit as it carries much more specific information. I think we should change that, but the current selection seems intentional. @jedevc

@tonistiigi

Copy link
Copy Markdown
Member Author

Looks like alpine openssh is broken.

@thaJeztah

Copy link
Copy Markdown
Member

Failure in "validate" - I kicked CI; https://github.com/moby/buildkit/actions/runs/17995633801/job/51194337569?pr=6244

#2 [internal] load git source https://github.com/moby/buildkit.git#refs/pull/6244/merge
#2 135.6 fatal: unable to access 'https://github.com/moby/buildkit.git/': Failed to connect to github.com port 443 after 135579 ms: Could not connect to server
#2 ERROR: failed to fetch remote https://github.com/moby/buildkit.git: git stderr:
fatal: unable to access 'https://github.com/moby/buildkit.git/': Failed to connect to github.com port 443 after 135579 ms: Could not connect to server
: exit status 128
------
 > [internal] load git source https://github.com/moby/buildkit.git#refs/pull/6244/merge:
135.6 fatal: unable to access 'https://github.com/moby/buildkit.git/': Failed to connect to github.com port 443 after 135579 ms: Could not connect to server
------
ERROR: failed to solve: failed to read dockerfile: failed to load cache key: failed to fetch remote https://github.com/moby/buildkit.git: git stderr:
fatal: unable to access 'https://github.com/moby/buildkit.git/': Failed to connect to github.com port 443 after 135579 ms: Could not connect to server
: exit status 128
Build references
Check build summary support
Error: buildx bake failed with: : exit status 128

@jedevc

jedevc commented Sep 25, 2025

Copy link
Copy Markdown
Member

Ahh there's a lot of funky things in this logic that I would like to fix, this being one of them (others mostly being based on the fact that the git state in #4557 can get into weirdly strange states).

Agree on the annotated tags - intentional at the time, but yup, wrong. We shouldn't fully resolve it. That said, I think if you do this that some other more subtle things might(?) break, and may need to be corrected - since if you checkout an annotated tag tag, it will actually put HEAD at tag^{}. So, HEAD won't actually be set to the pin returned from provenance in this case.

If tag was already pulled by --tags or without refs/tags
that creates ambigous reference in the shared repository.

Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
Comment thread source/git/source_test.go
Comment on lines +666 to +669
expLen := 40
if format == "sha256" {
expLen = 64
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you make the expected length into a parameter to this function?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this should be separated from format as then it is possible for the values to diverge. This could be cleaned up though, eg. with a helper function, but as this same check is used tens of times in this test file, I think these should all be done together in a separate PR.

@jsternberg

Copy link
Copy Markdown
Collaborator

Removing this from the v0.25.0 milestone because there is another PR that seems intended to replace this one and it is marked as being ready for v0.26.0. If I'm wrong, we can do a patch release to include this.

@jsternberg jsternberg modified the milestones: v0.25.0, v0.26.0 Sep 30, 2025
@jsternberg

Copy link
Copy Markdown
Collaborator

@tonistiigi has informed me that I was mistaken offline. Placing back into the v0.25.0 milestone.

@tonistiigi
tonistiigi merged commit 15f7052 into moby:master Sep 30, 2025
167 of 169 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants