From ddb8855198ea7a5bc9547e205dbfda8b7b5f1b80 Mon Sep 17 00:00:00 2001 From: Meredith Lancaster Date: Thu, 30 Jan 2025 09:12:48 -0700 Subject: [PATCH 1/2] flip bundle fetching logic Signed-off-by: Meredith Lancaster --- pkg/cmd/attestation/api/client.go | 24 ++++++++++++------------ pkg/cmd/attestation/api/client_test.go | 8 ++++++-- 2 files changed, 18 insertions(+), 14 deletions(-) diff --git a/pkg/cmd/attestation/api/client.go b/pkg/cmd/attestation/api/client.go index 37579b7bc..746b04411 100644 --- a/pkg/cmd/attestation/api/client.go +++ b/pkg/cmd/attestation/api/client.go @@ -176,23 +176,23 @@ func (c *LiveClient) fetchBundleFromAttestations(attestations []*Attestation) ([ return fmt.Errorf("attestation has no bundle or bundle URL") } - // for now, we fallback to the bundle field if the bundle URL is empty - if a.BundleURL == "" { - c.logger.VerbosePrintf("Bundle URL is empty. Falling back to bundle field\n\n") - fetched[i] = &Attestation{ - Bundle: a.Bundle, + // If the bundle field is nil, try to fetch the bundle with the provided URL + if a.Bundle == nil { + b, err := c.GetBundle(a.BundleURL) + if err != nil { + return fmt.Errorf("failed to fetch bundle with URL: %w", err) + } + fetched[i] = &Attestation{ + Bundle: b, } - return nil } - // otherwise fetch the bundle with the provided URL - b, err := c.GetBundle(a.BundleURL) - if err != nil { - return fmt.Errorf("failed to fetch bundle with URL: %w", err) - } + // otherwise fall back to the bundle field + c.logger.VerbosePrintf("Bundle URL is empty. Falling back to bundle field\n\n") fetched[i] = &Attestation{ - Bundle: b, + Bundle: a.Bundle, } + return nil }) } diff --git a/pkg/cmd/attestation/api/client_test.go b/pkg/cmd/attestation/api/client_test.go index 65f8d59ca..083a5c5c8 100644 --- a/pkg/cmd/attestation/api/client_test.go +++ b/pkg/cmd/attestation/api/client_test.go @@ -180,7 +180,7 @@ func TestGetByDigest_Error(t *testing.T) { require.Nil(t, attestations) } -func TestFetchBundleFromAttestations(t *testing.T) { +func TestFetchBundleFromAttestations_Fallback_Bundle_Field(t *testing.T) { httpClient := &mockHttpClient{} client := LiveClient{ httpClient: httpClient, @@ -194,7 +194,7 @@ func TestFetchBundleFromAttestations(t *testing.T) { require.NoError(t, err) require.Len(t, fetched, 2) require.Equal(t, "application/vnd.dev.sigstore.bundle.v0.3+json", fetched[0].Bundle.GetMediaType()) - httpClient.AssertNumberOfCalls(t, "OnGetSuccess", 2) + httpClient.AssertNumberOfCalls(t, "OnGetSuccess", 0) } func TestFetchBundleFromAttestations_InvalidAttestation(t *testing.T) { @@ -220,7 +220,10 @@ func TestFetchBundleFromAttestations_Fail(t *testing.T) { } att1 := makeTestAttestation() + att1.Bundle = nil att2 := makeTestAttestation() + att2.Bundle = nil + // zero out the bundle field so it tries fetching by URL attestations := []*Attestation{&att1, &att2} fetched, err := c.fetchBundleFromAttestations(attestations) require.Error(t, err) @@ -237,6 +240,7 @@ func TestFetchBundleFromAttestations_FetchByURLFail(t *testing.T) { } a := makeTestAttestation() + a.Bundle = nil attestations := []*Attestation{&a} bundle, err := c.fetchBundleFromAttestations(attestations) require.Error(t, err) From 70ae9f39ef3668bab984a77be34b2ec3be4162b5 Mon Sep 17 00:00:00 2001 From: Meredith Lancaster Date: Thu, 30 Jan 2025 09:26:41 -0700 Subject: [PATCH 2/2] update tests to account for logic flip Signed-off-by: Meredith Lancaster --- pkg/cmd/attestation/api/client.go | 4 +++- pkg/cmd/attestation/api/client_test.go | 11 +++++++---- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/pkg/cmd/attestation/api/client.go b/pkg/cmd/attestation/api/client.go index 746b04411..6054bc98e 100644 --- a/pkg/cmd/attestation/api/client.go +++ b/pkg/cmd/attestation/api/client.go @@ -178,6 +178,7 @@ func (c *LiveClient) fetchBundleFromAttestations(attestations []*Attestation) ([ // If the bundle field is nil, try to fetch the bundle with the provided URL if a.Bundle == nil { + c.logger.VerbosePrintf("Bundle field is empty. Trying to fetch with bundle URL\n\n") b, err := c.GetBundle(a.BundleURL) if err != nil { return fmt.Errorf("failed to fetch bundle with URL: %w", err) @@ -185,10 +186,11 @@ func (c *LiveClient) fetchBundleFromAttestations(attestations []*Attestation) ([ fetched[i] = &Attestation{ Bundle: b, } + return nil } // otherwise fall back to the bundle field - c.logger.VerbosePrintf("Bundle URL is empty. Falling back to bundle field\n\n") + c.logger.VerbosePrintf("Fetching bundle from Bundle field\n\n") fetched[i] = &Attestation{ Bundle: a.Bundle, } diff --git a/pkg/cmd/attestation/api/client_test.go b/pkg/cmd/attestation/api/client_test.go index 083a5c5c8..3d180af8f 100644 --- a/pkg/cmd/attestation/api/client_test.go +++ b/pkg/cmd/attestation/api/client_test.go @@ -180,7 +180,7 @@ func TestGetByDigest_Error(t *testing.T) { require.Nil(t, attestations) } -func TestFetchBundleFromAttestations_Fallback_Bundle_Field(t *testing.T) { +func TestFetchBundleFromAttestations_BundleURL(t *testing.T) { httpClient := &mockHttpClient{} client := LiveClient{ httpClient: httpClient, @@ -188,13 +188,16 @@ func TestFetchBundleFromAttestations_Fallback_Bundle_Field(t *testing.T) { } att1 := makeTestAttestation() + att1.Bundle = nil att2 := makeTestAttestation() + att2.Bundle = nil + // zero out the bundle field so it tries fetching by URL attestations := []*Attestation{&att1, &att2} fetched, err := client.fetchBundleFromAttestations(attestations) require.NoError(t, err) require.Len(t, fetched, 2) - require.Equal(t, "application/vnd.dev.sigstore.bundle.v0.3+json", fetched[0].Bundle.GetMediaType()) - httpClient.AssertNumberOfCalls(t, "OnGetSuccess", 0) + require.NotNil(t, "application/vnd.dev.sigstore.bundle.v0.3+json", fetched[0].Bundle.GetMediaType()) + httpClient.AssertNumberOfCalls(t, "OnGetSuccess", 2) } func TestFetchBundleFromAttestations_InvalidAttestation(t *testing.T) { @@ -211,7 +214,7 @@ func TestFetchBundleFromAttestations_InvalidAttestation(t *testing.T) { require.Nil(t, fetched, 2) } -func TestFetchBundleFromAttestations_Fail(t *testing.T) { +func TestFetchBundleFromAttestations_Fail_BundleURL(t *testing.T) { httpClient := &failAfterOneCallHttpClient{} c := &LiveClient{