Update `gh attestation` attestation bundle fetching logic #10185

malancas · 2025-01-06T15:31:07Z

Update the logic that fetches attestation bundles to use the bundle URL if provided in the GitHub API response.

cc #9850

Signed-off-by: Meredith Lancaster <malancas@github.com>

…-url

Signed-off-by: Meredith Lancaster <malancas@github.com>

…-url

Signed-off-by: Meredith Lancaster <malancas@github.com>

…hub.com:malancas/cli into fetch-artifact-attestation-bundles-with-sas-url

Signed-off-by: Meredith Lancaster <malancas@github.com>

…-url

Signed-off-by: Meredith Lancaster <malancas@github.com>

…hub.com:malancas/cli into fetch-artifact-attestation-bundles-with-sas-url

Signed-off-by: Meredith Lancaster <malancas@github.com>

malancas · 2025-01-07T18:48:34Z

pkg/cmd/attestation/api/client.go

+var getAttestationRetryInterval = time.Millisecond * 200
+
+// githubApiClient makes REST calls to the GitHub API
+type githubApiClient interface {


Since the client that implements this interface should only be used for making requests against the GitHub API, I'm renaming this interface to more explicitly reference the GitHub API.

malancas · 2025-01-07T18:49:01Z

pkg/cmd/attestation/api/client.go

 }

+// httpClient makes HTTP calls to all non-GitHub API endpoints
+type httpClient interface {


Because the bundle URL we use to fetch the bundle is not part of the GitHub API, we need to use regular HTTP client.

malancas · 2025-01-07T18:49:54Z

pkg/cmd/attestation/api/client.go

 }

-// Allow injecting backoff interval in tests.
-var getAttestationRetryInterval = time.Millisecond * 200


I moved this to the top of the file alongside other variables and consts.

malancas · 2025-01-07T18:54:57Z

pkg/cmd/attestation/api/client.go

-		logger: l,
+		githubAPI:  api.NewClientFromHTTP(hc),
+		host:       strings.TrimSuffix(host, "/"),
+		httpClient: hc,


We use the same http.Client provided by the Factory object when the subcommand is invoked to make requests with bundle URLs.

malancas · 2025-01-07T18:58:07Z

pkg/cmd/attestation/api/client.go

 func (c *LiveClient) GetByRepoAndDigest(repo, digest string, limit int) ([]*Attestation, error) {
 	url := c.BuildRepoAndDigestURL(repo, digest)
-	return c.getAttestations(url, repo, digest, limit)
+	attestations, err := c.getAttestations(url, repo, digest, limit)


The getAttestations method currently returns the api.Attestation type even though we really only care about the *bundle.Bundle type returned as a field within the Attestation type. I think we should consider updating this method to return *bundle.Bundle instead in a follow up PR.

Signed-off-by: Meredith Lancaster <malancas@github.com>

…-url

pkg/cmd/attestation/api/client.go

phillmv · 2025-01-07T20:26:45Z

pkg/cmd/attestation/api/client.go

+		})
+	}
+
+	if err := g.Wait(); err != nil {


For my own edification: if any of these go funcs errors out, then we bail on everything.

phillmv

Looks good! Honestly just minor naming tweaks / moving code around a bit.

pkg/cmd/attestation/api/client.go

pkg/cmd/attestation/api/client_test.go

Signed-off-by: Meredith Lancaster <malancas@github.com>

Co-authored-by: Phill MV <phillmv@github.com>

pkg/cmd/attestation/api/client.go

Signed-off-by: Meredith Lancaster <malancas@github.com>

williammartin · 2025-01-08T12:19:35Z

@malancas it looks like you requested my review specifically on this, and that all the changes are inside the attestations package. Before I spend time on it, is there something specific you want me to pay attention to, or just generally have a look at it because it's a large change? I'd never seen snappy before!

Signed-off-by: Meredith Lancaster <malancas@github.com>

malancas · 2025-01-08T15:48:47Z

@malancas it looks like you requested my review specifically on this, and that all the changes are inside the attestations package. Before I spend time on it, is there something specific you want me to pay attention to, or just generally have a look at it because it's a large change? I'd never seen snappy before!

@williammartin I think you were automatically added as a reviewer because I added the github.com/golang/snappy dependency to go.mod. To give you a quick summary, the gh attestation code that requests attestations from the GitHub API will now try to fetch the attestation bundle from a URL provided in the returned attestation object. The fetched bundles are compressed with Snappy, so gh attestation now uses Snappy to decompress them.

williammartin · 2025-01-08T16:04:32Z

Oh yeh I see it was because of the team. No problem.

phillmv

This seems fine. I left a couple comments but for the purposes of this PR let's handle it in a follow up.

To wit:

maybe getAttestations returns bundles directly? (meh)
maybe merge GetByOwner and GetByRepo
maybe add a retry to GetBundle

pkg/cmd/attestation/api/client.go

phillmv · 2025-01-10T20:45:54Z

pkg/cmd/attestation/api/client.go

+func (c *LiveClient) GetBundle(url string) (*bundle.Bundle, error) {
+	c.logger.VerbosePrintf("Fetching attestation bundle with bundle URL\n\n")
+
+	resp, err := c.httpClient.Get(url)


I don't know why this didn't occur to me to say this three days ago but – if we're opening N connections to fetch N bundles then we have N new opportunities for random flaky connections to trash the whole operation.

Does this have a retry baked in? Does it make sense for us to retry 2-3 times?

No, but it should be easy to add one in a follow up.

This MR contains the following updates: | Package | Update | Change | |---|---|---| | [cli/cli](https://github.com/cli/cli) | minor | `v2.65.0` -> `v2.66.1` | MR created with the help of [el-capitano/tools/renovate-bot](https://gitlab.com/el-capitano/tools/renovate-bot). **Proposed changes to behavior should be submitted there as MRs.** --- ### Release Notes <details> <summary>cli/cli (cli/cli)</summary> ### [`v2.66.1`](https://github.com/cli/cli/releases/tag/v2.66.1): GitHub CLI 2.66.1 [Compare Source](cli/cli@v2.66.0...v2.66.1) #### Hotfix: `gh pr view` fails with provided URL This addresses a regression in `gh pr view` was reported in [#10352](cli/cli#10352). This regression was due to a change in `v2.66.0` that no longer allowed `gh pr` subcommands to execute properly outside of a git repo. #### What's Changed - Hotfix: `gh pr view` fails with provided URL by [@jtmcg](https://github.com/jtmcg) in cli/cli#10354 **Full Changelog**: cli/cli@v2.66.0...v2.66.1 ### [`v2.66.0`](https://github.com/cli/cli/releases/tag/v2.66.0): GitHub CLI 2.66.0 [Compare Source](cli/cli@v2.65.0...v2.66.0) #### `gh pr view` and `gh pr status` now respect common triangular workflow configurations Previously, `gh pr view` and `gh pr status` would fail for pull request's (MR) open in triangular workflows. This was due to `gh` being unable to identify the MR's corresponding remote and branch refs on GitHub. Now, `gh pr view` and `gh pr status` should successfully identify the MR's refs when the following common git configurations are used: - [`branch.<branchName>.pushremote`](https://git-scm.com/docs/git-config#Documentation/git-config.txt-branchltnamegtpushRemote) is set - [`remote.pushDefault`](https://git-scm.com/docs/git-config#Documentation/git-config.txt-remotepushDefault) is set Branch specific configuration, the former, supersedes repo specific configuration, the latter. Additionally, if the [`@{push}` revision syntax](https://git-scm.com/docs/gitrevisions#Documentation/gitrevisions.txt-emltbranchnamegtpushemegemmasterpushemempushem) for git resolves for a branch, `gh pr view` and `gh pr status` should work regardless of additional config settings. For more information, see - cli/cli#9363 - cli/cli#9364 - cli/cli#9365 - cli/cli#9374 #### `gh secret list`, `gh secret set`, and `gh secret delete` now require repository selection when multiple `git` remotes are present Previously, `gh secret list`, `gh secret set`, and `gh secret delete` would determine which remote to target for interacting with GitHub Actions secrets. Remotes marked as default using `gh repo set-default` or through other `gh` commands had higher priority when figuring out which repository to interact with. This could have unexpected outcomes when using `gh secret` commands with forked repositories as the upstream repository would generally be selected. Now, `gh secret` commands require users to disambiguate which repository should be the target if multiple remotes are present and the `-R, --repo` flag is not provided. For more information, see cli/cli#4688 #### Extension update notices now notify once every 24 hours per extension and can be disabled Previously, the GitHub CLI would notify users about newer versions every time an extension was executed. This did not match GitHub CLI notices, which only notified users once every 24 hours and could be disabled through an environment variable. Now, extension update notices will behave similar to GitHub CLI notices. To disable extension update notices, set the `GH_NO_EXTENSION_UPDATE_NOTIFIER` environment variable. For more information, see cli/cli#9925 #### What's Changed ##### ✨ Features - Draft for discussing testing around extension update checking behavior by [@andyfeller](https://github.com/andyfeller) in cli/cli#9985 - Make extension update check non-blocking by [@andyfeller](https://github.com/andyfeller) in cli/cli#10239 - Ensure extension update notices only notify once within 24 hours, provide ability to disable all extension update notices by [@andyfeller](https://github.com/andyfeller) in cli/cli#9934 - feat: make the extension upgrade fancier by [@nobe4](https://github.com/nobe4) in cli/cli#10194 - fix: padded display by [@nobe4](https://github.com/nobe4) in cli/cli#10216 - Update `gh attestation` attestation bundle fetching logic by [@malancas](https://github.com/malancas) in cli/cli#10185 - Require repo disambiguation for secret commands by [@williammartin](https://github.com/williammartin) in cli/cli#10209 - show error message for rerun workflow older than a month ago by [@iamrajhans](https://github.com/iamrajhans) in cli/cli#10227 - Update `gh attestation verify` table output by [@malancas](https://github.com/malancas) in cli/cli#10104 - Enable MSI building for Windows arm64 by [@dennisameling](https://github.com/dennisameling) in cli/cli#10297 - feat: Add support for creating autolink references by [@hoffm](https://github.com/hoffm) in cli/cli#10180 - Find MRs using `@{push}` by [@Frederick888](https://github.com/Frederick888) in cli/cli#9208 - feat: Add support for viewing autolink references by [@hoffm](https://github.com/hoffm) in cli/cli#10324 - Update `gh attestation` bundle fetching logic by [@malancas](https://github.com/malancas) in cli/cli#10339 ##### 🐛 Fixes - gh gist delete: prompt for gist id by [@danochoa](https://github.com/danochoa) in cli/cli#10154 - Better handling for waiting for codespaces to become ready by [@cmbrose](https://github.com/cmbrose) in cli/cli#10198 - Fix: `gh gist view` and `gh gist edit` prompts with no TTY by [@mateusmarquezini](https://github.com/mateusmarquezini) in cli/cli#10048 - Remove naked return values from `ReadBranchConfig` and `prSelectorForCurrentBranch` by [@jtmcg](https://github.com/jtmcg) in cli/cli#10197 - Add job to deployment workflow to validate the tag name for a given release by [@jtmcg](https://github.com/jtmcg) in cli/cli#10121 - \[gh run list] Stop progress indicator on failure from `--workflow` flag by [@iamazeem](https://github.com/iamazeem) in cli/cli#10323 - Update deployment.yml by [@andyfeller](https://github.com/andyfeller) in cli/cli#10340 ##### 📚 Docs & Chores - Add affected version heading to bug report issue form by [@BagToad](https://github.com/BagToad) in cli/cli#10269 - chore: fix some comments by [@petercover](https://github.com/petercover) in cli/cli#10296 - Update triage.md to reflect FR experiment outcome by [@jtmcg](https://github.com/jtmcg) in cli/cli#10196 - Clear up --with-token fine grained PAT usage by [@williammartin](https://github.com/williammartin) in cli/cli#10186 - Correct help documentation around template use in `gh issue create` by [@andyfeller](https://github.com/andyfeller) in cli/cli#10208 - chore: fix some function names in comment by [@zhuhaicity](https://github.com/zhuhaicity) in cli/cli#10225 - Tiny typo fix by [@robmorgan](https://github.com/robmorgan) in cli/cli#10265 - add install instructions for Manjaro Linux by [@AMS21](https://github.com/AMS21) in cli/cli#10236 - Update test to be compatible with latest Glamour v0.8.0 by [@ottok](https://github.com/ottok) in cli/cli#10151 - Add more `gh attestation verify` integration tests by [@malancas](https://github.com/malancas) in cli/cli#10102 #####

Dependencies - Bump github.com/mattn/go-colorable from 0.1.13 to 0.1.14 by [@dependabot](https://github.com/dependabot) in cli/cli#10215 - Bump github.com/sigstore/protobuf-specs from 0.3.2 to 0.3.3 by [@dependabot](https://github.com/dependabot) in cli/cli#10214 - Bump github.com/gabriel-vasile/mimetype from 1.4.7 to 1.4.8 by [@dependabot](https://github.com/dependabot) in cli/cli#10184 - Bump google.golang.org/protobuf from 1.36.2 to 1.36.3 by [@dependabot](https://github.com/dependabot) in cli/cli#10250 - Bump golangci-linter and address failures to prepare for Go 1.24 strictness by [@mikelolasagasti](https://github.com/mikelolasagasti) in cli/cli#10279 - Bump github.com/google/go-containerregistry from 0.20.2 to 0.20.3 by [@dependabot](https://github.com/dependabot) in cli/cli#10257 - Bump actions/attest-build-provenance from 2.1.0 to 2.2.0 by [@dependabot](https://github.com/dependabot) in cli/cli#10300 - Bump google.golang.org/protobuf from 1.36.3 to 1.36.4 by [@dependabot](https://github.com/dependabot) in cli/cli#10306 - Upgrade sigstore-go to v0.7.0: fixes [#10114](cli/cli#10114) formatting issue by [@codysoyland](https://github.com/codysoyland) in cli/cli#10309 - Bump github.com/in-toto/attestation from 1.1.0 to 1.1.1 by [@dependabot](https://github.com/dependabot) in cli/cli#10319 #### New Contributors Big thank you to our many new *and* longtime contributors making this release happen!! ❤️ ✨ - [@zhuhaicity](https://github.com/zhuhaicity) made their first contribution in cli/cli#10225 - [@danochoa](https://github.com/danochoa) made their first contribution in cli/cli#10154 - [@robmorgan](https://github.com/robmorgan) made their first contribution in cli/cli#10265 - [@iamrajhans](https://github.com/iamrajhans) made their first contribution in cli/cli#10227 - [@AMS21](https://github.com/AMS21) made their first contribution in cli/cli#10236 - [@petercover](https://github.com/petercover) made their first contribution in cli/cli#10296 - [@ottok](https://github.com/ottok) made their first contribution in cli/cli#10151 - [@dennisameling](https://github.com/dennisameling) made their first contribution in cli/cli#10297 - [@iamazeem](https://github.com/iamazeem) made their first contribution in cli/cli#10323 - [@Frederick888](https://github.com/Frederick888) made their first contribution in cli/cli#9208 **Full Changelog**: cli/cli@v2.65.0...v2.66.0 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever MR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this MR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this MR, check this box --- This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).

malancas and others added 29 commits November 6, 2024 07:57

initial pass at fetching bundles with sas urls

bfd140c

Signed-off-by: Meredith Lancaster <malancas@github.com>

Merge branch 'trunk' into fetch-artifact-attestation-bundles-with-sas…

7160f7e

…-url

fetch bundles with sas url

5a6a796

Signed-off-by: Meredith Lancaster <malancas@github.com>

remove unused method

e51b4ef

Signed-off-by: Meredith Lancaster <malancas@github.com>

add httpClient field to LiveClient struct

6b95175

Signed-off-by: Meredith Lancaster <malancas@github.com>

go mod tidy

45121f6

Signed-off-by: Meredith Lancaster <malancas@github.com>

var naming

8f5d710

Signed-off-by: Meredith Lancaster <malancas@github.com>

update error messages

fb020f2

Signed-off-by: Meredith Lancaster <malancas@github.com>

add mock http client

e4431a3

Signed-off-by: Meredith Lancaster <malancas@github.com>

fix failing tests

ab4912f

Signed-off-by: Meredith Lancaster <malancas@github.com>

Merge branch 'trunk' into fetch-artifact-attestation-bundles-with-sas…

706314b

…-url

provide additional logging and fallback

9051da3

Signed-off-by: Meredith Lancaster <malancas@github.com>

Merge branch 'fetch-artifact-attestation-bundles-with-sas-url' of git…

fc984c9

…hub.com:malancas/cli into fetch-artifact-attestation-bundles-with-sas-url

return fetch attestations err directly

311f2b2

Signed-off-by: Meredith Lancaster <malancas@github.com>

fetch bundles in parallel

070b67e

Signed-off-by: Meredith Lancaster <malancas@github.com>

add tests for bundle url fetch and fallback

e03a36e

Signed-off-by: Meredith Lancaster <malancas@github.com>

Merge branch 'trunk' into fetch-artifact-attestation-bundles-with-sas…

b1af4b0

…-url

add test case for bundle url fetch failure

0202ca8

Signed-off-by: Meredith Lancaster <malancas@github.com>

Merge branch 'fetch-artifact-attestation-bundles-with-sas-url' of git…

fb4fc7e

…hub.com:malancas/cli into fetch-artifact-attestation-bundles-with-sas-url

add mutex for test field

6986511

Signed-off-by: Meredith Lancaster <malancas@github.com>

setup testing struct for test cases

9ecd90c

Signed-off-by: Meredith Lancaster <malancas@github.com>

add http client test constructors

e34e188

Signed-off-by: Meredith Lancaster <malancas@github.com>

use mock to assert number of http calls

ecf55c6

Signed-off-by: Meredith Lancaster <malancas@github.com>

simplify mock http client

9d88ca8

Signed-off-by: Meredith Lancaster <malancas@github.com>

more mock http client cleanup

7838e91

Signed-off-by: Meredith Lancaster <malancas@github.com>

undo name change for now

3c0280c

Signed-off-by: Meredith Lancaster <malancas@github.com>

remove spaces

37e0969

Signed-off-by: Meredith Lancaster <malancas@github.com>

undo other name change

0a602fa

Signed-off-by: Meredith Lancaster <malancas@github.com>

undo more name chanages

258c69c

Signed-off-by: Meredith Lancaster <malancas@github.com>

malancas commented Jan 7, 2025

View reviewed changes

malancas and others added 2 commits January 7, 2025 12:03

comment

f46cccb

Signed-off-by: Meredith Lancaster <malancas@github.com>

Merge branch 'trunk' into fetch-artifact-attestation-bundles-with-sas…

d4f3fbc

…-url

malancas marked this pull request as ready for review January 7, 2025 19:08

malancas requested review from a team as code owners January 7, 2025 19:08

malancas requested a review from williammartin January 7, 2025 19:08

cliAutomation added the external pull request originating outside of the CLI core team label Jan 7, 2025

phillmv reviewed Jan 7, 2025

View reviewed changes

pkg/cmd/attestation/api/client.go Show resolved Hide resolved

phillmv reviewed Jan 7, 2025

View reviewed changes

malancas and others added 3 commits January 7, 2025 14:13

remove old comment

42cb254

Signed-off-by: Meredith Lancaster <malancas@github.com>

Update pkg/cmd/attestation/api/client.go

51a74ae

Co-authored-by: Phill MV <phillmv@github.com>

Update pkg/cmd/attestation/api/client.go

8d89dd9

Co-authored-by: Phill MV <phillmv@github.com>

github-advanced-security bot found potential problems Jan 7, 2025

View reviewed changes

pkg/cmd/attestation/api/client.go Fixed Show fixed Hide fixed

update tests to use new function name

33d0002

Signed-off-by: Meredith Lancaster <malancas@github.com>

add check for invalid attestation

8ad877b

Signed-off-by: Meredith Lancaster <malancas@github.com>

williammartin approved these changes Jan 8, 2025

View reviewed changes

malancas requested a review from phillmv January 8, 2025 17:58

phillmv approved these changes Jan 10, 2025

View reviewed changes

malancas merged commit 112552f into cli:trunk Jan 13, 2025

malancas deleted the fetch-artifact-attestation-bundles-with-sas-url branch January 13, 2025 17:42

malancas mentioned this pull request Jan 13, 2025

Attestation bundle fetch improvements #10233

Merged

Update gh attestation attestation bundle fetching logic #10185

Update gh attestation attestation bundle fetching logic #10185

Uh oh!

Conversation

malancas commented Jan 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phillmv left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

williammartin commented Jan 8, 2025

Uh oh!

malancas commented Jan 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

williammartin commented Jan 8, 2025

Uh oh!

phillmv left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Update `gh attestation` attestation bundle fetching logic #10185

Update `gh attestation` attestation bundle fetching logic #10185

malancas commented Jan 6, 2025 •

edited

Loading

malancas commented Jan 8, 2025 •

edited

Loading