Fix handling of old oci hooks #6631
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rhatdan The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci-robot
added
the
approved
|
Fixes: #6629 |
|
LGTM |
|
I have a guess you were looking for @heatmiser as well 😃 |
| @@ -95,4 +96,6 @@ func ReadDir(path string, extensionStages []string, hooks map[string]*current.Ho | |||
|
|
|||
| func init() { | |||
| Readers[current.Version] = current.Read | |||
| Readers[old.Version] = old.Read | |||
| Readers[""] = old.Read | |||
There was a problem hiding this comment.
Well, it works, I guess, but this usage is new to me so of course I had to edit /usr/share/containers/oci/hooks.d/oci-umount.json and add "version": "xx", for several values of xx. Results:
| Version | Error |
|---|---|
| "0.1.0" | works fine |
| "0.1.1" | error setting up OCI Hooks ... unrecognized hook version: "0.1.1" |
| "1.0.0" | error setting up OCI Hooks ... 1.0.0: json: cannot unmarshal string into Go struct field Hook.hook of type specs.Hook |
I'll buy that my 1.0.0 is my own problem, probably because the format has changed somehow and I can't just add "version" without changing other things.
But, is there ever a possibility of a version other than 0.1.0, 1.0.0, or (absent)? Is there a way to specify a fallback Reader?
There was a problem hiding this comment.
Sure, it's code. I will default to current.Read
There was a problem hiding this comment.
Not sure if this helps...but I just tried editing /usr/share/containers/oci/hooks.d/oci-umount.json and add "version": "0.1.0", with result:
Error: error setting up OCI Hooks: parsing hook "/usr/share/containers/oci/hooks.d/oci-umount.json": unrecognized hook version: "0.1.0"
There was a problem hiding this comment.
@heatmiser Try that with my patch and it should work.
pkg/hooks/read.go
Outdated
| @@ -49,7 +49,7 @@ func read(content []byte) (hook *current.Hook, err error) { | |||
| } | |||
| reader, ok := Readers[ver.Version] | |||
| if !ok { | |||
| return nil, fmt.Errorf("unrecognized hook version: %q", ver.Version) | |||
| reader = current.Read | |||
There was a problem hiding this comment.
Thanks, I'm comforted that this is more futureproof. At the risk of annoying you even further, WDYT of:
logrus.Warnf("unrecognized hook version %q; defaulting to %q reader", ver.Version, current.Version)
(I know it will not show by default, but could help in unforeseen errors if someone remembers to use --log-level)
There was a problem hiding this comment.
I'm a little iffy here. We want to support "" and "0.0.1" - I'm less sure we want to support "-1", "this is not a number", and "1/0" as valid hook versions.
pkg/hooks/read_test.go
Outdated
| @@ -171,27 +171,6 @@ func TestUnknownDir(t *testing.T) { | |||
| } | |||
| } | |||
|
|
|||
| func TestBadDir(t *testing.T) { | |||
There was a problem hiding this comment.
Why are we dropping this? I think we should still fail on hooks with an invalid version
There was a problem hiding this comment.
In order to handle Ed's vision which is more future proof, we had to drop this test. Let's discuss today. BTW Maybe we should just merge the original and then talk about this at standup.
There was a problem hiding this comment.
Maybe my suggestion isn't more futureproof. Maybe if there's a v2.0.0 or v1.2.3 it will be incompatible enough that podman should abort early. I have no sense for these hooks, for the json format, for when/how that format changes. I just think we need to think about it in advance.
There was a problem hiding this comment.
If we had a more easily parsed version number, I'd say we should accept anything with major version 1 and deny anything else, assuming semantic versioning.
There was a problem hiding this comment.
Is there a (relatively easy) workaround that can implemented in the meantime, for users to be able to launch containers on their systems while the details for the fix are worked out?
There was a problem hiding this comment.
@heatmiser you already tried adding "version":"0.1.0", to the json... I wonder, could you try "1.0.0"? I apologize for not trying it myself first.
There was a problem hiding this comment.
| Version | Error |
|---|---|
| "0.1.0" | error setting up OCI Hooks ... unrecognized hook version: "0.1.0" |
| "0.1.1" | error setting up OCI Hooks ... unrecognized hook version: "0.1.1" |
| "1.0.0" | error setting up OCI Hooks ... 1.0.0: json: cannot unmarshal string into Go struct field Hook.hook of type specs.Hook |
Fedora 31
# podman -v
podman version 2.0.0-dev
# rpm -q podman
podman-2.0.0-1592321502.git908bc3fb.fc31.x86_64
...from Copr repo for Upstream_CRIO_Family owned by baude
Recently executed package updates, not sure what previous version of podman was, but it worked
Note that I am also seeing the same error messages for /usr/share/containers/oci/hooks.d/oci-systemd-hook.json as I am for /usr/share/containers/oci/hooks.d/oci-umount.json when I attempt to start containers via podman.
Podman is blowing up with oci-umount hook, because it was never rewritten to support the v1.0.0 value. This PR adds support for the older version and cleans up the hook handling. Signed-off-by: Daniel J Walsh <[email protected]>
|
@baude @mheon @jwhonce @TomSweeneyRedHat @QiWang19 @ashley-cui PTAL and get this merged. |
|
LGTM. |
|
/lgtm |
github-actions
bot
added
the
locked - please file new issue/PR
Podman is blowing up with oci-umount hook, because
it was never rewritten to support the v1.0.0 value.
This PR adds support for the older version and cleans
up the hook handling.
Signed-off-by: Daniel J Walsh [email protected]