From 2ab8c12fc89f4375ab9640bb818b064a36f1a843 Mon Sep 17 00:00:00 2001 From: Samuel Karp Date: Wed, 8 Jun 2022 21:12:41 -0700 Subject: [PATCH 1/6] archive: use Linkat on FreeBSD for hardlinks Calling link(2) with a symlink as the target will cause FreeBSD to attempt to create a new hard link pointing to the target of the symlink rather than a hardlink to the symlink itself. By contrast, Linux creates a hardlink to the symlink. Use linkat(2) instead, which accepts a flag controlling this behavior. If linkat(2) is called with AT_SYMLINK_FOLLOW, it will behave the same as link(2). If linkat(2) is called without AT_SYMLINK_FOLLOW, it will behave the same as Linux's link(2) instead. See FreeBSD's implementation of ln(1), which uses linkat(2) and controls this behavior by way of the -P and -L flags: https://github.com/freebsd/freebsd-src/blob/30031172534c22695ab7b26a9420bda7b20b0824/bin/ln/ln.c#L342-L343 Signed-off-by: Samuel Karp --- archive/link_default.go | 25 +++++++++++++ archive/link_freebsd.go | 82 +++++++++++++++++++++++++++++++++++++++++ archive/tar.go | 2 +- 3 files changed, 108 insertions(+), 1 deletion(-) create mode 100644 archive/link_default.go create mode 100644 archive/link_freebsd.go diff --git a/archive/link_default.go b/archive/link_default.go new file mode 100644 index 000000000..84a8997eb --- /dev/null +++ b/archive/link_default.go @@ -0,0 +1,25 @@ +//go:build !freebsd + +/* + Copyright The containerd Authors. + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. +*/ + +package archive + +import "os" + +func link(oldname, newname string) error { + return os.Link(oldname, newname) +} diff --git a/archive/link_freebsd.go b/archive/link_freebsd.go new file mode 100644 index 000000000..2097db06b --- /dev/null +++ b/archive/link_freebsd.go @@ -0,0 +1,82 @@ +/* + Copyright The containerd Authors. + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. +*/ + +package archive + +import ( + "os" + "syscall" + + "golang.org/x/sys/unix" +) + +func link(oldname, newname string) error { + e := ignoringEINTR(func() error { + return unix.Linkat(unix.AT_FDCWD, oldname, unix.AT_FDCWD, newname, 0) + }) + if e != nil { + return &os.LinkError{Op: "link", Old: oldname, New: newname, Err: e} + } + return nil +} + +// The following contents were copied from Go 1.18.2. +// Use of this source code is governed by the following +// BSD-style license: +// +// Copyright (c) 2009 The Go Authors. All rights reserved. +// +// Redistribution and use in source and binary forms, with or without +// modification, are permitted provided that the following conditions are +// met: +// +// * Redistributions of source code must retain the above copyright +// notice, this list of conditions and the following disclaimer. +// * Redistributions in binary form must reproduce the above +// copyright notice, this list of conditions and the following disclaimer +// in the documentation and/or other materials provided with the +// distribution. +// * Neither the name of Google Inc. nor the names of its +// contributors may be used to endorse or promote products derived from +// this software without specific prior written permission. +// +// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS +// "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT +// LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR +// A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT +// OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, +// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT +// LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, +// DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY +// THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT +// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE +// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + +// ignoringEINTR makes a function call and repeats it if it returns an +// EINTR error. This appears to be required even though we install all +// signal handlers with SA_RESTART: see #22838, #38033, #38836, #40846. +// Also #20400 and #36644 are issues in which a signal handler is +// installed without setting SA_RESTART. None of these are the common case, +// but there are enough of them that it seems that we can't avoid +// an EINTR loop. +func ignoringEINTR(fn func() error) error { + for { + err := fn() + if err != syscall.EINTR { + return err + } + } +} diff --git a/archive/tar.go b/archive/tar.go index a57074e77..e20a171d3 100644 --- a/archive/tar.go +++ b/archive/tar.go @@ -361,7 +361,7 @@ func createTarFile(ctx context.Context, path, extractDir string, hdr *tar.Header return err } - if err := os.Link(targetPath, path); err != nil { + if err := link(targetPath, path); err != nil { return err } From c15f0cdaf0035662c3c616f513ce63e560198f47 Mon Sep 17 00:00:00 2001 From: Samuel Karp Date: Wed, 8 Jun 2022 22:26:25 -0700 Subject: [PATCH 2/6] oci: FreeBSD devices may have major number 0 Signed-off-by: Samuel Karp --- oci/utils_unix_test.go | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/oci/utils_unix_test.go b/oci/utils_unix_test.go index 9f2c8d05e..49a5071db 100644 --- a/oci/utils_unix_test.go +++ b/oci/utils_unix_test.go @@ -23,6 +23,7 @@ import ( "errors" "fmt" "os" + "runtime" "testing" "github.com/stretchr/testify/assert" @@ -149,9 +150,11 @@ func TestHostDevicesAllValid(t *testing.T) { } for _, device := range devices { - // Devices can't have major number 0. - if device.Major == 0 { - t.Errorf("device entry %+v has zero major number", device) + if runtime.GOOS != "freebsd" { + // On Linux, devices can't have major number 0. + if device.Major == 0 { + t.Errorf("device entry %+v has zero major number", device) + } } switch device.Type { case blockDevice, charDevice: From ad8e5980606c5e4bc1bc1a140ddc24f2a626969c Mon Sep 17 00:00:00 2001 From: Samuel Karp Date: Wed, 8 Jun 2022 22:33:23 -0700 Subject: [PATCH 3/6] oci: Remove empty mount option slice for FreeBSD Mount options are marked `json:omitempty`. An empty slice in the default object caused TestWithSpecFromFile to fail. Signed-off-by: Samuel Karp --- oci/mounts_freebsd.go | 1 - 1 file changed, 1 deletion(-) diff --git a/oci/mounts_freebsd.go b/oci/mounts_freebsd.go index 42b9d7aff..ada12c1a6 100644 --- a/oci/mounts_freebsd.go +++ b/oci/mounts_freebsd.go @@ -32,7 +32,6 @@ func defaultMounts() []specs.Mount { Destination: "/dev/fd", Type: "fdescfs", Source: "fdescfs", - Options: []string{}, }, } } From 42e019e6343a10725db02e4ca37ab0e03e6f1355 Mon Sep 17 00:00:00 2001 From: Samuel Karp Date: Wed, 8 Jun 2022 22:40:45 -0700 Subject: [PATCH 4/6] cri/server: Disable tests on FreeBSD The TestPodAnnotationPassthroughContainerSpec test and the TestContainerAnnotationPassthroughContainerSpec test both depend on a platform-specific implementation of criService.containerSpec, which is unimplemented on FreeBSD. The TestSandboxContainerSpec depends on a platform-specific implementation oc criService.sandboxContainerSpec, which is unimplemented on FreeBSD. Signed-off-by: Samuel Karp --- pkg/cri/server/container_create_test.go | 10 ++++++++-- pkg/cri/server/sandbox_run_test.go | 6 ++++-- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/pkg/cri/server/container_create_test.go b/pkg/cri/server/container_create_test.go index 48e2ed0ab..151751b5f 100644 --- a/pkg/cri/server/container_create_test.go +++ b/pkg/cri/server/container_create_test.go @@ -69,8 +69,11 @@ func TestGeneralContainerSpec(t *testing.T) { } func TestPodAnnotationPassthroughContainerSpec(t *testing.T) { - if goruntime.GOOS == "darwin" { + switch goruntime.GOOS { + case "darwin": t.Skip("not implemented on Darwin") + case "freebsd": + t.Skip("not implemented on FreeBSD") } testID := "test-id" @@ -279,8 +282,11 @@ func TestVolumeMounts(t *testing.T) { } func TestContainerAnnotationPassthroughContainerSpec(t *testing.T) { - if goruntime.GOOS == "darwin" { + switch goruntime.GOOS { + case "darwin": t.Skip("not implemented on Darwin") + case "freebsd": + t.Skip("not implemented on FreeBSD") } testID := "test-id" diff --git a/pkg/cri/server/sandbox_run_test.go b/pkg/cri/server/sandbox_run_test.go index d8159aabb..91a197783 100644 --- a/pkg/cri/server/sandbox_run_test.go +++ b/pkg/cri/server/sandbox_run_test.go @@ -35,10 +35,12 @@ import ( ) func TestSandboxContainerSpec(t *testing.T) { - if goruntime.GOOS == "darwin" { + switch goruntime.GOOS { + case "darwin": t.Skip("not implemented on Darwin") + case "freebsd": + t.Skip("not implemented on FreeBSD") } - testID := "test-id" nsPath := "test-cni" for desc, test := range map[string]struct { From 95f1d797186e0ccba3c6a9ae0cf95351a63a4b1b Mon Sep 17 00:00:00 2001 From: Samuel Karp Date: Wed, 8 Jun 2022 23:06:28 -0700 Subject: [PATCH 5/6] platforms: Run goimports for FreeBSD Signed-off-by: Samuel Karp --- platforms/defaults_freebsd.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/platforms/defaults_freebsd.go b/platforms/defaults_freebsd.go index 1828b7458..d3fe89e07 100644 --- a/platforms/defaults_freebsd.go +++ b/platforms/defaults_freebsd.go @@ -17,8 +17,9 @@ package platforms import ( - specs "github.com/opencontainers/image-spec/specs-go/v1" "runtime" + + specs "github.com/opencontainers/image-spec/specs-go/v1" ) // DefaultSpec returns the current platform's default platform specification. From 5560b622d6e952213380eec7936af6a580d0c17b Mon Sep 17 00:00:00 2001 From: Samuel Karp Date: Thu, 9 Jun 2022 14:31:21 -0700 Subject: [PATCH 6/6] archive: Explicitly specify stdio for tar(1) Different tar(1) implementations default to different input and output locations when none is specified. This can include tape devices like /dev/st0 (on Linux) or /dev/sa0 (on FreeBSD), but may be overridden by compilation options or environment variables. Using the f option with the special value of - instructs tar(1) to read from stdin and write to stdout instead of the default. Signed-off-by: Samuel Karp --- archive/tar_test.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/archive/tar_test.go b/archive/tar_test.go index 3ffb697cb..ba2179c22 100644 --- a/archive/tar_test.go +++ b/archive/tar_test.go @@ -844,7 +844,7 @@ func testApply(t *testing.T, a fstest.Applier) error { return fmt.Errorf("failed to apply filesystem changes: %w", err) } - tarArgs := []string{"c", "-C", td} + tarArgs := []string{"cf", "-", "-C", td} names, err := readDirNames(td) if err != nil { return fmt.Errorf("failed to read directory names: %w", err) @@ -879,9 +879,12 @@ func testBaseDiff(t *testing.T, a fstest.Applier) error { arch := Diff(context.Background(), "", td) - cmd := exec.Command(tarCmd, "x", "-C", dest) + cmd := exec.Command(tarCmd, "xf", "-", "-C", dest) cmd.Stdin = arch + stderr := &bytes.Buffer{} + cmd.Stderr = stderr if err := cmd.Run(); err != nil { + fmt.Println(stderr.String()) return fmt.Errorf("tar command failed: %w", err) }