ctr: improve error relative shim path error msg by ginglis13 · Pull Request #6519 · containerd/containerd

ginglis13 · 2022-02-05T01:22:57Z

addresses #6464

Slice returned by strings.Split("./path/to/shim", ".") is of
length 2 - first element being the empty string "". Check for this to
see if a runtime provided is relative.

With these changes, the example from #6464 becomes:

$ sudo ctr run  --rm --runtime=./containerd-shim-runc-v2 docker.io/library/hello-world:latest hello-world
ctr: failed to start shim: failed to resolve runtime path: invalid runtime name ./containerd-shim-runc-v2, correct runtime name should be either format like `io.containerd.runc.v1` or a full path to the binary: unknown

Additionally adds context to the usage for ctr run --runtime indicating that
absolute path to shim binary must be provided.

Signed-off-by: Gavin Inglis [email protected]

k8s-ci-robot · 2022-02-05T01:23:06Z

Hi @ginglis13. Thanks for your PR.

I'm waiting for a containerd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

AkihiroSuda · 2022-02-05T17:39:22Z

 	// runtime name should format like $prefix.name.version
 	parts := strings.Split(runtime, ".")
-	if len(parts) < 2 {
+	if len(parts) < 3 {


This logic doesn’t seem correct.
The right way would be to check filepath.IsAbs in ctr.

containerd/cmd/ctr/commands/run/run_unix.go

Line 337 in 4f5ce56

cOpts = append(cOpts, containerd.WithRuntime(context.String("runtime"), runtimeOpts))

My reason for making this change here is to my understanding, the value passed to --runtime is evaluated in ShimManager's resolveRuntimePath method. This method uses filepath.IsAbs to check if the runtime provided is an absolute path to an existing file.

In resolveRuntimePath's fallthrough logic for when the provided value is not an absolute filepath, the BinaryName util is used to check for a value like io.containerd.runc.v1 and return containerd-shim-runc-v1 in that case. A string like ./containerd-shim-runc-v2 passed to BinaryName results in a parts slice ["" "/containerd-shim-runc-v2"] of length 2 and will miss this error check on the parts length.

I'm not too familiar with all the possible runtime name formats - if a format like runc.v1 is acceptable then these changes could be updated to add an additional check for if the first element of parts is ""

if a format like runc.v1 is acceptable

Yes. Actually --runtime aws.firecracker appears in https://github.com/firecracker-microvm/firecracker-containerd/blob/main/docs/getting-started.md#usage

AkihiroSuda

https://github.com/containerd/containerd/pull/6519/files#r800209416

AkihiroSuda · 2022-02-07T08:02:55Z

 	// runtime name should format like $prefix.name.version
 	parts := strings.Split(runtime, ".")
-	if len(parts) < 2 {
+	if len(parts) < 2 || parts[0] == "" {


Still not robust to catch a case like foo/bar.

Probably, we should just do :

runtime := clicontext.String("runtime") if strings.Contains(runtime, "/") && !filepath.IsAbs(runtime) { return fmt.Errorf("runtime must be a runtime name (e.g., \"io.containerd.runc.v2\") or absolute path to the runtime binary (e.g., \"/usr/local/bin/containerd-shim-runc-v2\"), got %q", runtime) }

From my testing, the existing check len(parts) < 2 already covers this case as strings.Split("foo/bar", ".") will result in a slice of length 1 : ["foo/bar"]

It doesn't however cover foo/bar.baz.foo - I'll go ahead with this suggestion 🙂

Would you like this to go in runtime/v2/manager.go here ? or would it belong better in cmd/ctr/commands/run/run_unix.go ?

I prefer to have that under runtime/ rather than cmd/

kzys

Looks good! Can you write some tests?

ginglis13 · 2022-02-18T01:23:18Z

Looks good! Can you write some tests?

sure thing - looked around for a bit and didn't see any existing unit tests for v2 ShimManager, or Shim utils. Should I make some tests in a new file manager_test.go , or are there existing tests I can update ?

mxpv · 2022-02-22T00:55:02Z

Should I make some tests in a new file manager_test.go, or are there existing tests I can update ?

It's fine to make manager_test.go

Overall LGTM

theopenlab-ci · 2022-03-01T00:12:17Z

Build succeeded.

containerd-build-arm64 : SUCCESS in 4m 48s (non-voting)
containerd-test-arm64 : FAILURE in 5m 43s (non-voting)
containerd-integration-test-arm64 : FAILURE in 25m 17s (non-voting)

theopenlab-ci · 2022-03-01T02:48:55Z

Build succeeded.

containerd-build-arm64 : SUCCESS in 5m 12s (non-voting)
containerd-test-arm64 : FAILURE in 5m 49s (non-voting)
containerd-integration-test-arm64 : FAILURE in 30m 05s (non-voting)

theopenlab-ci · 2022-03-03T03:33:03Z

Build succeeded.

containerd-build-arm64 : RETRY_LIMIT in 3m 44s (non-voting)
containerd-test-arm64 : RETRY_LIMIT in 5m 53s (non-voting)
containerd-integration-test-arm64 : RETRY_LIMIT in 28m 16s (non-voting)

kzys · 2022-03-03T19:00:00Z

+		return "", err
+	}
+
+	err = os.Setenv("PATH", tempShimDir+":"+os.Getenv("PATH"))


It is bit scary to set the PATH here since it will affect all tests in the same process. How about using t.Setenv?

kzys · 2022-03-03T19:00:59Z

+// setupAbsoluteShimPath creates a temporary directory with an empty shim executable file in it.
+// This is to test the exec.LookPath branch of resolveRuntimePath
+func setupAbsoluteShimPath() (string, error) {
+	tempShimDir, err := ioutil.TempDir("", "test-resolve-runtime-path")


You need to remove the directory

If it is okay to take testing.T, you can use t.TempDir

addresses containerd#6464 Return an error if a runtime provided is relative. Add context to the usage for `ctr run --runtime` indicating that absolute path to runtime binary must be provided. Signed-off-by: Gavin Inglis <[email protected]>

theopenlab-ci · 2022-03-04T00:22:24Z

Build succeeded.

containerd-build-arm64 : RETRY_LIMIT in 3m 33s (non-voting)
containerd-test-arm64 : RETRY_LIMIT in 5m 47s (non-voting)
containerd-integration-test-arm64 : RETRY_LIMIT in 24m 45s (non-voting)

kzys

/ok-to-test

kzys · 2022-03-11T18:12:38Z

@mxpv @AkihiroSuda PTAL

k8s-ci-robot added the needs-ok-to-test label Feb 5, 2022

AkihiroSuda reviewed Feb 5, 2022

View reviewed changes

AkihiroSuda requested changes Feb 6, 2022

View reviewed changes

ginglis13 force-pushed the ctr-runtime-path branch from 2829316 to 926e489 Compare February 6, 2022 18:36

ginglis13 requested a review from AkihiroSuda February 6, 2022 20:35

AkihiroSuda reviewed Feb 7, 2022

View reviewed changes

ginglis13 force-pushed the ctr-runtime-path branch from 926e489 to a721f38 Compare February 9, 2022 03:13

kzys requested changes Feb 17, 2022

View reviewed changes

ginglis13 force-pushed the ctr-runtime-path branch from a721f38 to 7fc1027 Compare February 28, 2022 23:45

ginglis13 marked this pull request as draft March 1, 2022 00:40

ginglis13 force-pushed the ctr-runtime-path branch from 7fc1027 to ca6bab0 Compare March 1, 2022 02:17

ginglis13 force-pushed the ctr-runtime-path branch 2 times, most recently from d9acb71 to 2effd9f Compare March 3, 2022 02:32

ginglis13 marked this pull request as ready for review March 3, 2022 04:39

kzys requested changes Mar 3, 2022

View reviewed changes

kzys reviewed Mar 3, 2022

View reviewed changes

ginglis13 force-pushed the ctr-runtime-path branch from 2effd9f to 7b045ea Compare March 3, 2022 23:28

kzys approved these changes Mar 3, 2022

View reviewed changes

kzys reviewed Mar 7, 2022

View reviewed changes

k8s-ci-robot added ok-to-test and removed needs-ok-to-test labels Mar 7, 2022

ginglis13 requested a review from AkihiroSuda March 7, 2022 23:43

mxpv approved these changes Mar 11, 2022

View reviewed changes

kzys merged commit 50ca572 into containerd:main Mar 14, 2022

kzys mentioned this pull request Mar 21, 2022

ctr: make --runtime=./relative/path/to/shim/binary error less cryptic (ctr: failed to start shim: start failed: : exec: "/": permission denied: unknown) #6464

Closed

Iceber mentioned this pull request Sep 27, 2022

runtime/v2: clean up redundant path resolve for runtime #7437

Closed

Conversation

ginglis13 commented Feb 5, 2022

Uh oh!

k8s-ci-robot commented Feb 5, 2022

Uh oh!

AkihiroSuda Feb 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ginglis13 Feb 6, 2022

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda Feb 6, 2022

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda left a comment

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda Feb 7, 2022

Choose a reason for hiding this comment

Uh oh!

ginglis13 Feb 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ginglis13 Feb 7, 2022

Choose a reason for hiding this comment

Uh oh!

kzys Feb 7, 2022

Choose a reason for hiding this comment

Uh oh!

kzys left a comment

Choose a reason for hiding this comment

Uh oh!

ginglis13 commented Feb 18, 2022

Uh oh!

mxpv commented Feb 22, 2022

Uh oh!

theopenlab-ci Bot commented Mar 1, 2022

Uh oh!

theopenlab-ci Bot commented Mar 1, 2022

Uh oh!

theopenlab-ci Bot commented Mar 3, 2022

Uh oh!

kzys Mar 3, 2022

Choose a reason for hiding this comment

Uh oh!

kzys Mar 3, 2022

Choose a reason for hiding this comment

Uh oh!

theopenlab-ci Bot commented Mar 4, 2022

Uh oh!

kzys left a comment

Choose a reason for hiding this comment

Uh oh!

kzys commented Mar 11, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

AkihiroSuda Feb 5, 2022 •

edited

Loading

ginglis13 Feb 7, 2022 •

edited

Loading