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

vendor in runc at the latest commit #1570

Closed
wants to merge 1 commit into from
Closed

Conversation

cdoern
Copy link

@cdoern cdoern commented Jun 7, 2022

this is a large go.mod change vendor since runc updated a dependency to a versiom that requires go 1.17
this vendor is necessary for containers/common#936 which is currently downgrading c/image due to it's runc version

Signed-off-by: cdoern cdoern@redhat.com

this is a large vendor since runc updated a dependency to a versiom that requires go 1.17
this vendor is necessary for containers/common#936 which is currently downgrading c/image due to it's runc version

Signed-off-by: cdoern <cdoern@redhat.com>
@rhatdan
Copy link
Member

rhatdan commented Jun 7, 2022

LGTM

@cdoern
Copy link
Author

cdoern commented Jun 7, 2022

@rhatdan does this make sense? with your go.mod you gave me everything now compiles correctly, though, this'll have to happen eventually.

@rhatdan
Copy link
Member

rhatdan commented Jun 7, 2022

/approve
/lgtm

Copy link
Collaborator

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

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

@cdoern could you explain in more detail what is going on, please?

In general, Go modules use the latest requested version of any dependency. So if c/common needs a newer version, it can just refer to it, without any c/image cooperation.

This looks like a downgrade of the run dependency from 1.1.2 to <1.1.1. It seems that it isn’t actually a downgrade, because the commit is from a different branch, is that the core of the issue?

Nevertheless, that still looks like a downgrade, in particular to Go itself, so any other user of c/image that refers to 1.1.1 or 1.1.2 would cause Go to “upgrade” and use that version instead of the main branch. All of the tooling will fight against us, e.g. Dependabot warning about a security vulnerability and suggesting an update to a release, like #1559 .

What other ways to solve this have been tried? This one can work only within the tight ecosystem of the GitHub.com/containers tools, as long as no-one ever forgets and accidentally “updates” runc again.


Separately, the go 1.16 directive in go.mod (https://go.dev/doc/modules/gomod-ref#go ) specifies the language version used by c/image. Why does that need changing?

(I don’t mind moving to 1.17 in principle, and as soon as any dependency of c/image starts requiring 1.17, we will have to move anyway — but I think it’s useful to do that as a separate effort, and to review the language spec for possible new features to benefit from; compare e.g. https://github.com/containers/image/pull/1493/files. The go 1.16 version right now means that that was done for 1.16. So, if we have to move now, sure, let’s do that, but do we have to?)

@mtrmac
Copy link
Collaborator

mtrmac commented Jun 8, 2022

(Mechanically, if we do need to move to go 1.17, it would be very useful to have the move to go 1.17 a separate commit, and then the runc dependency change a separate commit, to make it clear what is just a mechanism change and what is substantial.)

@cdoern
Copy link
Author

cdoern commented Jun 8, 2022

@mtrmac we have been fighting against this for a day now, I agree it is weird that this is a downgrade. we actually figured it out for containers/common#936 so we could close this until runc 1.1.3 is released.

as for 1.16 vs 1.17, runc has merged a commit opencontainers/runc@e0406b4 that bumped a dependency to 0.9.0 which requires go 1.17. This means when we next update runc we will need to bump go.

Either way, if there are worries about this PR we can close it as the underlying issue was "figured" out. But we will have to swing back to this eventually.

@mtrmac
Copy link
Collaborator

mtrmac commented Jun 8, 2022

What was the c/common fix? Note that a replace directive only works at the top level, i.e. a replace directive in c/common does nothing for building Podman.

I have looked at this now for a bit, and I can’t see a good solution either. Maybe a merge commit in the Runc repo, from the 1.1.2 branch into main, would allow us to name the commit reference v1.1.2-0.… , but that looks pretty risky to me, especially with the Go proxy remembering everything.


Locally for c/image, it might well make sense to just drop the dependency, and live with Dependabot warnings. Of course that does nothing for actual users like c/storage…


Actually:

go get github.com/opencontainers/runc@main 
# Uses the 1.1.1-0 pseudo-version, triggers a downgrade of c/storage, so:
go get github.com/containers/storage@8951d0153bf6
# Fine, runc uses the right version, and c/storage is not downgraded. But now:
go mod tidy
# go.mod now doesn’t record any runc dependency

So at this point just bumping c/storage (which needs runc), and dropping the runc reference from c/image, would work fine (… until the next runc security advisory, which we would just have to remember to ignore), and I’d be fine with doing that immediately.

WRT Go 1.17, let me read the release notes… at the very least splitting this PR into two commits would be helpful.

@mtrmac
Copy link
Collaborator

mtrmac commented Jun 8, 2022

Go 1.17 migration:

  • Drop the PreferCipherSuites value in one case in c/image.
  • Use testing.T.Setenv instead of doing the cleanup manually (incl. dropping of comments about T.Parallel tests)

That looks pretty small (and smaller tests are always nice) — but I’d much prefer for that to be a separate PR.

@mtrmac
Copy link
Collaborator

mtrmac commented Jun 8, 2022

Just to coordinate, I’ll work on that Go 1.17 upgrade now, both for c/image and Skopeo. We can then leave this PR just to talk about the runc dependency.

@mtrmac
Copy link
Collaborator

mtrmac commented Jun 8, 2022

#1571 updates to Go 1.17 features.

@rhatdan
Copy link
Member

rhatdan commented Jun 15, 2022

I don't think this is needed any longer, Closing.

@rhatdan rhatdan closed this Jun 15, 2022
@mtrmac
Copy link
Collaborator

mtrmac commented Jun 15, 2022

@cdoern out of curiosity, how was this ultimately handled? I see containers/common#936 merged with a replace directive, which I wouldn’t expect to make a difference…

If this is still necessary, please reopen — after #1571 it should be about a 5-line change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants