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

Update go-containerregsitry dependency to 0.4.1 #1829

Merged
merged 1 commit into from
Apr 26, 2021

Conversation

imjasonh
Copy link
Contributor

PR Checklist

Please check if your PR fulfills the following requirements:

  • [n] Tests for the changes have been added (for bug fixes / features)
  • [n] Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Other... Please describe:

What is the current behavior?

Issue Number: #1814

What is the new behavior?

This updates the go-containerregistry dependency from 0.1.4 (October 2020) to 0.4.1 (March 2021)

This removes a dependency on a non-deterministic package resulting in inconsistent archive SHAs generated by GitHub's archive server. That upstream issue was also fixed in kubernetes/kubernetes#99376.

Other than removing this non-determinism, no major functional changes are expected. Some performance improvements and bugfixes have been added since 0.1.4, which might be beneficial.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@steeve
Copy link

steeve commented Apr 26, 2021

Thank you for being so reactive, but 👎

Github branch/commit tarballs are dynamically generated. Don't depend on them having a stable SHA256. If the upstream rule is not distributing built artefacts, you'll need to use git_repository or go_repository or whatever rule that is stable.

In this case, use go_repository's commit and remote to pin a specific commit.

Fetch repository using commit and importpath, instead of using
dynamically generated archive.
@imjasonh imjasonh force-pushed the go-containerregistry branch from 3bb82d7 to 7ed4277 Compare April 26, 2021 15:45
@imjasonh
Copy link
Contributor Author

I've updated this PR to use commit and importpath for go-containerregistry.

I haven't touched the other dependencies, but I can update those if you'd like.

@steeve
Copy link

steeve commented Apr 26, 2021

Not my call, but I feel this could be a good idea. Or let go_repository pull them as in module mode maybe?

@alexeagle alexeagle merged commit 881e779 into bazelbuild:master Apr 26, 2021
@uhthomas
Copy link
Collaborator

The go_repository target is missing the sum attribute. The module downloaded through this rule will not be verified and poses a security risk.

@steeve
Copy link

steeve commented Apr 27, 2021

The go_repository target is missing the sum attribute. The module downloaded through this rule will not be verified and poses a security risk.

It is not downloaded as a module, but via a git commit, which is safe.

@uhthomas
Copy link
Collaborator

The go_repository target is missing the sum attribute. The module downloaded through this rule will not be verified and poses a security risk.

It is not downloaded as a module, but via a git commit, which is safe.

Downloading an archive just based on a git commit is not safe. All content must be verified, whether it be through the sum or sha256 attributes.

tcinbis added a commit to tcinbis/scion that referenced this pull request May 1, 2021
Previously the bazel build would fail because the sha on github's side
changed.
The fix was merged upstream in PR bazelbuild/rules_docker#1829 and
released with version 0.17.0.
martaver pushed a commit to cleric-sh/rules_docker that referenced this pull request May 18, 2021
Fetch repository using commit and importpath, instead of using
dynamically generated archive.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants