Fix ReplaceSignatures

[jonjohnsonjr]

This was correctly replacing the base of the signatures but was also
appending those signatures to the Get() response. So the v1.Image
representation was getting out of sync with the oci.Signatures
representation.

Normally you won't see this because cosign doesn't do multiple
attestations in the same command for the same image, so you'd roundtrip
the signatures through the v1.Image being pushed and pulled, which
corrects the discrepancy.

If you (specifically, me) attempt to attach an attestation multiple
times, the Get() will get very out of sync with the v1.Image and Replace
no longer does what you'd expect because it's operating on incorrect
Get() results instead of directly on the v1.Image representation.

[jonjohnsonjr]

cc @developer-guy I might be missing something

[codecov]

Codecov Report

Merging #3292 (1534cc9) into main (4c5669d) will increase coverage by 0.26%.
Report is 2 commits behind head on main.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #3292      +/-   ##
==========================================
+ Coverage   30.32%   30.59%   +0.26%     
==========================================
  Files         155      155              
  Lines        9853     9858       +5     
==========================================
+ Hits         2988     3016      +28     
+ Misses       6418     6392      -26     
- Partials      447      450       +3     

Files	Coverage Δ
pkg/oci/mutate/signatures.go	64.00% <100.00%> (+30.00%)	⬆️

... and 5 files with indirect coverage changes

[hectorj2f]

@jonjohnsonjr I assume it is related to this issue #923 but it isn't doing what it was meant to do.

[hectorj2f]

I can see how the replace tests (added here) are failing now.

[jonjohnsonjr]

I can see how the replace tests (added here) are failing now.

Thanks! This highlights my confusion. The ReplaceSignatures bit was updating the wrapped v1.Image correctly but was generating incorrect results for the Get() because it was appending the sigs to the base sigs, which meant they were there twice.

New diff is much smaller :)

[jonjohnsonjr]

Added a test, without my diff it fails with:

--- FAIL: TestSignEntity (0.01s)
    --- FAIL: TestSignEntity/with_replace_op_(attestation) (0.00s)
        mutate_test.go:459: len(Get()) = 2, wanted 1
        mutate_test.go:459: len(Get()) = 2, wanted 1
        mutate_test.go:459: len(Get()) = 2, wanted 1
        mutate_test.go:459: len(Get()) = 2, wanted 1
        mutate_test.go:459: len(Get()) = 2, wanted 1
        mutate_test.go:459: len(Get()) = 2, wanted 1
        mutate_test.go:459: len(Get()) = 2, wanted 1
        mutate_test.go:459: len(Get()) = 2, wanted 1
        mutate_test.go:459: len(Get()) = 2, wanted 1
        mutate_test.go:459: len(Get()) = 2, wanted 1
        mutate_test.go:459: len(Get()) = 2, wanted 1
        mutate_test.go:459: len(Get()) = 2, wanted 1
        mutate_test.go:459: len(Get()) = 2, wanted 1
        mutate_test.go:459: len(Get()) = 2, wanted 1
        mutate_test.go:459: len(Get()) = 2, wanted 1
        mutate_test.go:459: len(Get()) = 2, wanted 1
        mutate_test.go:459: len(Get()) = 2, wanted 1
        mutate_test.go:459: len(Get()) = 2, wanted 1
        mutate_test.go:459: len(Get()) = 2, wanted 1
        mutate_test.go:459: len(Get()) = 2, wanted 1
        mutate_test.go:459: len(Get()) = 2, wanted 1
        mutate_test.go:459: len(Get()) = 2, wanted 1
        mutate_test.go:459: len(Get()) = 2, wanted 1
        mutate_test.go:459: len(Get()) = 2, wanted 1

Which confirms my assertion about the duplicated Get() results.

[hectorj2f]

lgtm