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 digest images being lost on load of hauls (Signed). #259

Merged
merged 4 commits into from
Jul 31, 2024

Conversation

KaminFay
Copy link
Contributor

Please check below, if the PR fulfills these requirements:

  • Commit(s) and code follow the repositories guidelines.
  • Test(s) have been added or updated to support these change(s).
  • Doc(s) have been added or updated to support these change(s).

Associated Links:

Types of Changes:

  • Bugfix

Proposed Changes:

Note:

Had to republish the PR after fixing my laptops commit signing setup.

  • Current when the image reference is taken by the Pusher() method and split, it is done so based on the first instance of the @ symbol:
var baseRef, hash string
parts := strings.SplitN(ref, "@", 2)
baseRef = parts[0]
if len(parts) > 1 {
    hash = parts[1]
}
  • Proposed way of splitting:
	var baseRef, hash string

	parts := strings.Split(ref, "@")
	baseRef = strings.Join(parts[:len(parts)-1], "@")
	if len(parts) > 1 {
		hash = parts[len(parts)-1]
	}

	return baseRef, hash
  • In reality that will cause the hash to appear with a lot of extra junk if there is more than one @ symbol in the reference string, like there is with digests.... Ex:
sha256:95b01e2e9ab0702ce2f1a8f05f90e6408fd1f4b5e5006c6088ba5a864ed42064-library/nginx@sha256:95b01e2e9ab0702ce2f1a8f05f90e6408fd1f4b5e5006c6088ba5a864ed42064-dev.cosignproject.cosign/imageIndex@sha256:95b01e2e9ab0702ce2f1a8f05f90e6408fd1f4b5e5006c6088ba5a864ed42064"
  • The proposed solution will instead filter on the last @ symbol instead of the first. Resulting in a proper hash being pulled out.
  • A long with this the splitting on the hash and base reference string has been pulled out into its own function so that it can be tested using the newly included testing methods. Just to make sure digests are not broken again in the future.
    Verification/Testing of Changes:
  • The change can be verified by following the steps in the bug report and seeing that we now have the correct index on unpacking and the info command shows everything is where it should be.

@zackbradys zackbradys added this to the Hauler v1.0.5 milestone Jul 6, 2024
@zackbradys zackbradys added bug Something isn't working size/M Denotes an issue/PR requiring a relatively moderate amount of work labels Jul 6, 2024
pkg/content/oci_test.go Outdated Show resolved Hide resolved
pkg/content/oci.go Outdated Show resolved Hide resolved
Copy link
Contributor

@dweomer dweomer left a comment

Choose a reason for hiding this comment

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

As noted here I would very much prefer we try a more standard approach and try to leverage https://github.com/distribution/reference for correctness if not concision.

@zackbradys
Copy link
Member

Thanks for reviewing and approving @dweomer... will test this tonight before merging.

@KaminFay do you notice any issues?

@KaminFay
Copy link
Contributor Author

Thanks for reviewing and approving @dweomer... will test this tonight before merging.

@KaminFay do you notice any issues?

Nope no issues seen on my side

@zackbradys
Copy link
Member

Glad to hear it! @KaminFay

Copy link
Member

@zackbradys zackbradys left a comment

Choose a reason for hiding this comment

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

I was finally able to run a few tests and LGTM!!

@zackbradys zackbradys merged commit 0c5cf20 into hauler-dev:main Jul 31, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working size/M Denotes an issue/PR requiring a relatively moderate amount of work
Projects
Status: Resolved
Development

Successfully merging this pull request may close these issues.

3 participants