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

fix: fingerprint mismatch when using -tags that affect injected packages #429

Merged
merged 5 commits into from
Nov 28, 2024

Conversation

RomainMuller
Copy link
Contributor

In certain conditions, using -tags that affect injected packages, such as -tags=appsec when CGO_ENABLED=0 results in a fingerprint mismatch at link time, due to the go build flags being incorrectly discovered.

This is fixed by making several changes:

  • Changes how the "shared" build flags are set in the job server's pkgs.Resolve operation, so that they are resolved during execution, instead of when creating the request; ensuring the pre-set flags are used in cases where "driver mode" is used;
  • Actively parse -a and -toolexec from go process args, so we explicitly prevent them from being forwarded (-a in particular causes builds to be A LOT slower due to re-building many injected packages over and over again);
  • Adds trace logging here and there to make this type of issues a little easier to debug in the future (essentially persisted the debugging logs I've added when investigating this issue),
  • Set GOTMPDIR when building packages in pkgs.Resolve so that the downstream builds are easy to find, and are organized in a way that matches the original build's process hierarchy;
  • The build ID for the version now also gets the original build flags, as they may affect what files get compiled or not (which in turns will affect the content checksum).

…kages

In certain conditions, using `-tags` that affect injected packages, such
as `-tags=appsec` when `CGO_ENABLED=0` results in a fingerprint mismatch
at link time, due to the go build flags being incorrectly discovered.

This is fixed by making several changes:
- Changes how the "shared" build flags are set in the job server's
  `pkgs.Resolve` operation, so that they are resolved during execution,
  instead of when creating the request; ensuring the pre-set flags are
  used in cases where "driver mode" is used;
- Actively parse `-a` and `-toolexec` from go process args, so we
  explicitly prevent them from being forwarded (`-a` in particular
  causes builds to be A LOT slower due to re-building many injected
  packages over and over again);
- Adds trace logging here and there to make this type of issues a little
  easier to debug in the future (essentially persisted the debugging
  logs I've added when investigating this issue),
- Set `GOTMPDIR` when building packages in `pkgs.Resolve` so that the
  downstream builds are easy to find, and are organized in a way that
  matches the original build's process hierarchy;
- The build ID for the version now also gets the original build flags,
  as they may affect what files get compiled or not (which in turns will
  affect the content checksum).
@RomainMuller RomainMuller requested a review from a team as a code owner November 27, 2024 13:37
@RomainMuller RomainMuller added this pull request to the merge queue Nov 28, 2024
Merged via the queue into main with commit 922b312 Nov 28, 2024
35 checks passed
@RomainMuller RomainMuller deleted the romain.marcadier/fptr-mismatch branch November 28, 2024 10:54
Copy link

codecov bot commented Nov 28, 2024

Codecov Report

Attention: Patch coverage is 72.22222% with 20 lines in your changes missing coverage. Please review.

Project coverage is 63.96%. Comparing base (c5b11d0) to head (835912b).
Report is 6 commits behind head on main.

Files with missing lines Patch % Lines
internal/jobserver/pkgs/resolve.go 79.06% 7 Missing and 2 partials ⚠️
internal/binpath/cmd.go 25.00% 2 Missing and 1 partial ⚠️
internal/cmd/toolexec.go 40.00% 2 Missing and 1 partial ⚠️
internal/goflags/flags.go 75.00% 1 Missing and 1 partial ⚠️
internal/jobserver/buildid/version.go 60.00% 1 Missing and 1 partial ⚠️
internal/cmd/go.go 0.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #429      +/-   ##
==========================================
+ Coverage   62.75%   63.96%   +1.20%     
==========================================
  Files         181      181              
  Lines       12950    10989    -1961     
==========================================
- Hits         8127     7029    -1098     
+ Misses       4301     3433     -868     
- Partials      522      527       +5     
Components Coverage Δ
Generators 76.69% <ø> (+4.17%) ⬆️
Instruments 73.41% <100.00%> (+0.38%) ⬆️
Go Driver 78.62% <75.00%> (+0.77%) ⬆️
Toolexec Driver 73.81% <100.00%> (+4.37%) ⬆️
Aspects 78.57% <ø> (+2.99%) ⬆️
Injector 77.13% <ø> (+2.80%) ⬆️
Job Server 67.07% <77.55%> (+2.41%) ⬆️
Integration Test Suite 57.58% <ø> (+0.31%) ⬆️
Other 63.96% <72.22%> (+1.20%) ⬆️
Files with missing lines Coverage Δ
instrument/instrument.go 66.66% <100.00%> (+1.66%) ⬆️
internal/goproxy/proxy.go 77.04% <ø> (+2.69%) ⬆️
internal/jobserver/client/env.go 69.49% <100.00%> (+7.95%) ⬆️
internal/toolexec/aspect/resolve.go 61.90% <100.00%> (+19.59%) ⬆️
internal/cmd/go.go 37.50% <0.00%> (-2.50%) ⬇️
internal/goflags/flags.go 80.62% <75.00%> (-2.08%) ⬇️
internal/jobserver/buildid/version.go 70.31% <60.00%> (+3.23%) ⬆️
internal/binpath/cmd.go 50.00% <25.00%> (ø)
internal/cmd/toolexec.go 55.17% <40.00%> (-4.29%) ⬇️
internal/jobserver/pkgs/resolve.go 80.90% <79.06%> (-5.30%) ⬇️

... and 158 files with indirect coverage changes

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

Successfully merging this pull request may close these issues.

2 participants