From 381442945b2e82ab1b472998d3013724220b292b Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Wed, 13 Sep 2023 02:23:05 +0200 Subject: [PATCH 1/2] platforms: remove errdefs dependency This cleans up the platforms package from dependencies that are not strictly needed. This is in preparation of making this package a separate module, which can be shared by plugins, and containerd versions (as well as external consumers), - Remove dependency on the errdefs package: most uses of these error definitions were used internally, and other errors may not be useful for external consumers as sentinel errors. - ErrInvalidArgument may be a potential exception, although a look at current uses of this package shows that there's no special handling of invalid parameters vs other errors (all would boil down to "the passed platform is invalid" (either the format, or parsing is not implemented on a specific platform) - Remove uses of the convenience "Platform" alias in favor of using the upstream (from OCI spec). Consumers of this package can still use the convenience alias, but make sure that function signatures do not imply that it's a different type (which can cause confusion). Signed-off-by: Sebastiaan van Stijn --- platforms/cpuinfo_linux.go | 13 ++++++------- platforms/cpuinfo_linux_test.go | 6 ++---- platforms/cpuinfo_other.go | 6 +----- platforms/errors.go | 30 ++++++++++++++++++++++++++++++ platforms/platforms.go | 24 ++++++++++++------------ 5 files changed, 51 insertions(+), 28 deletions(-) create mode 100644 platforms/errors.go diff --git a/platforms/cpuinfo_linux.go b/platforms/cpuinfo_linux.go index 722d86c35..98c7001f9 100644 --- a/platforms/cpuinfo_linux.go +++ b/platforms/cpuinfo_linux.go @@ -19,12 +19,12 @@ package platforms import ( "bufio" "bytes" + "errors" "fmt" "os" "runtime" "strings" - "github.com/containerd/containerd/errdefs" "golang.org/x/sys/unix" ) @@ -70,7 +70,7 @@ func getCPUInfo(pattern string) (info string, err error) { return "", err } - return "", fmt.Errorf("getCPUInfo for pattern %s: %w", pattern, errdefs.ErrNotFound) + return "", fmt.Errorf("getCPUInfo for pattern %s: %w", pattern, errNotFound) } // getCPUVariantFromArch get CPU variant from arch through a system call @@ -83,7 +83,7 @@ func getCPUVariantFromArch(arch string) (string, error) { if arch == "aarch64" { variant = "8" } else if arch[0:4] == "armv" && len(arch) >= 5 { - //Valid arch format is in form of armvXx + // Valid arch format is in form of armvXx switch arch[3:5] { case "v8": variant = "8" @@ -101,7 +101,7 @@ func getCPUVariantFromArch(arch string) (string, error) { variant = "unknown" } } else { - return "", fmt.Errorf("getCPUVariantFromArch invalid arch: %s, %w", arch, errdefs.ErrInvalidArgument) + return "", fmt.Errorf("getCPUVariantFromArch invalid arch: %s, %w", arch, errInvalidArgument) } return variant, nil } @@ -112,11 +112,10 @@ func getCPUVariantFromArch(arch string) (string, error) { // This is to cover running ARM in emulated environment on x86 host as this field in /proc/cpuinfo // was not present. func getCPUVariant() (string, error) { - variant, err := getCPUInfo("Cpu architecture") if err != nil { - if errdefs.IsNotFound(err) { - //Let's try getting CPU variant from machine architecture + if errors.Is(err, errNotFound) { + // Let's try getting CPU variant from machine architecture arch, err := getMachineArch() if err != nil { return "", fmt.Errorf("failure getting machine architecture: %v", err) diff --git a/platforms/cpuinfo_linux_test.go b/platforms/cpuinfo_linux_test.go index c0b8b0f6f..ecb11509d 100644 --- a/platforms/cpuinfo_linux_test.go +++ b/platforms/cpuinfo_linux_test.go @@ -20,8 +20,6 @@ import ( "errors" "runtime" "testing" - - "github.com/containerd/containerd/errdefs" ) func TestCPUVariant(t *testing.T) { @@ -107,13 +105,13 @@ func TestGetCPUVariantFromArch(t *testing.T) { name: "Test invalid input which doesn't start with armv", input: "armxxxx", output: "", - expectedErr: errdefs.ErrInvalidArgument, + expectedErr: errInvalidArgument, }, { name: "Test invalid input whose length is less than 5", input: "armv", output: "", - expectedErr: errdefs.ErrInvalidArgument, + expectedErr: errInvalidArgument, }, } { t.Run(testcase.name, func(t *testing.T) { diff --git a/platforms/cpuinfo_other.go b/platforms/cpuinfo_other.go index fa5f19c42..97a1fe8a3 100644 --- a/platforms/cpuinfo_other.go +++ b/platforms/cpuinfo_other.go @@ -21,8 +21,6 @@ package platforms import ( "fmt" "runtime" - - "github.com/containerd/containerd/errdefs" ) func getCPUVariant() (string, error) { @@ -49,10 +47,8 @@ func getCPUVariant() (string, error) { default: variant = "unknown" } - } else { - return "", fmt.Errorf("getCPUVariant for OS %s: %v", runtime.GOOS, errdefs.ErrNotImplemented) - + return "", fmt.Errorf("getCPUVariant for OS %s: %v", runtime.GOOS, errNotImplemented) } return variant, nil diff --git a/platforms/errors.go b/platforms/errors.go new file mode 100644 index 000000000..5ad721e77 --- /dev/null +++ b/platforms/errors.go @@ -0,0 +1,30 @@ +/* + 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 platforms + +import "errors" + +// These errors mirror the errors defined in [github.com/containerd/containerd/errdefs], +// however, they are not exported as they are not expected to be used as sentinel +// errors by consumers of this package. +// +//nolint:unused // not all errors are used on all platforms. +var ( + errNotFound = errors.New("not found") + errInvalidArgument = errors.New("invalid argument") + errNotImplemented = errors.New("not implemented") +) diff --git a/platforms/platforms.go b/platforms/platforms.go index 5aee03a7e..afcc867bb 100644 --- a/platforms/platforms.go +++ b/platforms/platforms.go @@ -120,7 +120,6 @@ import ( specs "github.com/opencontainers/image-spec/specs-go/v1" "github.com/containerd/containerd/api/types" - "github.com/containerd/containerd/errdefs" ) var ( @@ -186,14 +185,14 @@ func ParseAll(specifiers []string) ([]specs.Platform, error) { func Parse(specifier string) (specs.Platform, error) { if strings.Contains(specifier, "*") { // TODO(stevvooe): need to work out exact wildcard handling - return specs.Platform{}, fmt.Errorf("%q: wildcards not yet supported: %w", specifier, errdefs.ErrInvalidArgument) + return specs.Platform{}, fmt.Errorf("%q: wildcards not yet supported: %w", specifier, errInvalidArgument) } parts := strings.Split(specifier, "/") for _, part := range parts { if !specifierRe.MatchString(part) { - return specs.Platform{}, fmt.Errorf("%q is an invalid component of %q: platform specifier component must match %q: %w", part, specifier, specifierRe.String(), errdefs.ErrInvalidArgument) + return specs.Platform{}, fmt.Errorf("%q is an invalid component of %q: platform specifier component must match %q: %w", part, specifier, specifierRe.String(), errInvalidArgument) } } @@ -229,7 +228,7 @@ func Parse(specifier string) (specs.Platform, error) { return p, nil } - return specs.Platform{}, fmt.Errorf("%q: unknown operating system or architecture: %w", specifier, errdefs.ErrInvalidArgument) + return specs.Platform{}, fmt.Errorf("%q: unknown operating system or architecture: %w", specifier, errInvalidArgument) case 2: // In this case, we treat as a regular os/arch pair. We don't care // about whether or not we know of the platform. @@ -259,7 +258,7 @@ func Parse(specifier string) (specs.Platform, error) { return p, nil } - return specs.Platform{}, fmt.Errorf("%q: cannot parse platform specifier: %w", specifier, errdefs.ErrInvalidArgument) + return specs.Platform{}, fmt.Errorf("%q: cannot parse platform specifier: %w", specifier, errInvalidArgument) } // MustParse is like Parses but panics if the specifier cannot be parsed. @@ -297,24 +296,25 @@ func Normalize(platform specs.Platform) specs.Platform { func ToProto(platforms []Platform) []*types.Platform { ap := make([]*types.Platform, len(platforms)) for i := range platforms { - p := types.Platform{ + ap[i] = &types.Platform{ OS: platforms[i].OS, Architecture: platforms[i].Architecture, Variant: platforms[i].Variant, } - ap[i] = &p } return ap } // FromProto converts a slice of the protobuf definition [types.Platform] // to a slice of [Platform]. -func FromProto(platforms []*types.Platform) []Platform { - op := make([]Platform, len(platforms)) +func FromProto(platforms []*types.Platform) []specs.Platform { + op := make([]specs.Platform, len(platforms)) for i := range platforms { - op[i].OS = platforms[i].OS - op[i].Architecture = platforms[i].Architecture - op[i].Variant = platforms[i].Variant + op[i] = specs.Platform{ + OS: platforms[i].OS, + Architecture: platforms[i].Architecture, + Variant: platforms[i].Variant, + } } return op } From e916d77c81b8a61d7a37276e310b4a150b04f4ab Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Wed, 13 Sep 2023 15:29:44 +0200 Subject: [PATCH 2/2] platforms: move ToProto, FromProto to api/types These utilities resulted in the platforms package to have the containerd API as dependency. As this package is used in many parts of the code, as well as external consumers, we should try to keep it light on dependencies, with the potential to make it a standalone module. These utilities were added in f3b7436b61d2c3c69d7782d6bd817c2623be52e1, which has not yet been included in a release, so skipping deprecation and aliases for these. Signed-off-by: Sebastiaan van Stijn --- api/types/platform_helpers.go | 47 ++++++++++++++++++++++++++++++++ pkg/transfer/archive/exporter.go | 2 +- pkg/transfer/image/imagestore.go | 4 +-- platforms/platforms.go | 30 -------------------- services/introspection/local.go | 4 +-- 5 files changed, 52 insertions(+), 35 deletions(-) create mode 100644 api/types/platform_helpers.go diff --git a/api/types/platform_helpers.go b/api/types/platform_helpers.go new file mode 100644 index 000000000..7e662e9bb --- /dev/null +++ b/api/types/platform_helpers.go @@ -0,0 +1,47 @@ +/* + 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 types + +import oci "github.com/opencontainers/image-spec/specs-go/v1" + +// OCIPlatformToProto converts from a slice of OCI [specs.Platform] to a +// slice of the protobuf definition [Platform]. +func OCIPlatformToProto(platforms []oci.Platform) []*Platform { + ap := make([]*Platform, len(platforms)) + for i := range platforms { + ap[i] = &Platform{ + OS: platforms[i].OS, + Architecture: platforms[i].Architecture, + Variant: platforms[i].Variant, + } + } + return ap +} + +// OCIPlatformFromProto converts a slice of the protobuf definition [Platform] +// to a slice of OCI [specs.Platform]. +func OCIPlatformFromProto(platforms []*Platform) []oci.Platform { + op := make([]oci.Platform, len(platforms)) + for i := range platforms { + op[i] = oci.Platform{ + OS: platforms[i].OS, + Architecture: platforms[i].Architecture, + Variant: platforms[i].Variant, + } + } + return op +} diff --git a/pkg/transfer/archive/exporter.go b/pkg/transfer/archive/exporter.go index d71f75c4a..59ee899db 100644 --- a/pkg/transfer/archive/exporter.go +++ b/pkg/transfer/archive/exporter.go @@ -156,7 +156,7 @@ func (iis *ImageExportStream) UnmarshalAny(ctx context.Context, sm streaming.Str return err } - specified := platforms.FromProto(s.Platforms) + specified := types.OCIPlatformFromProto(s.Platforms) iis.stream = tstreaming.WriteByteStream(ctx, stream) iis.mediaType = s.MediaType iis.platforms = specified diff --git a/pkg/transfer/image/imagestore.go b/pkg/transfer/image/imagestore.go index 975fc7f25..88d115d41 100644 --- a/pkg/transfer/image/imagestore.go +++ b/pkg/transfer/image/imagestore.go @@ -363,7 +363,7 @@ func (is *Store) MarshalAny(context.Context, streaming.StreamCreator) (typeurl.A Labels: is.imageLabels, ManifestLimit: uint32(is.manifestLimit), AllMetadata: is.allMetadata, - Platforms: platforms.ToProto(is.platforms), + Platforms: types.OCIPlatformToProto(is.platforms), ExtraReferences: referencesToProto(is.extraReferences), Unpacks: unpackToProto(is.unpacks), } @@ -380,7 +380,7 @@ func (is *Store) UnmarshalAny(ctx context.Context, sm streaming.StreamGetter, a is.imageLabels = s.Labels is.manifestLimit = int(s.ManifestLimit) is.allMetadata = s.AllMetadata - is.platforms = platforms.FromProto(s.Platforms) + is.platforms = types.OCIPlatformFromProto(s.Platforms) is.extraReferences = referencesFromProto(s.ExtraReferences) is.unpacks = unpackFromProto(s.Unpacks) diff --git a/platforms/platforms.go b/platforms/platforms.go index afcc867bb..43e4ad3d8 100644 --- a/platforms/platforms.go +++ b/platforms/platforms.go @@ -118,8 +118,6 @@ import ( "strings" specs "github.com/opencontainers/image-spec/specs-go/v1" - - "github.com/containerd/containerd/api/types" ) var ( @@ -290,31 +288,3 @@ func Normalize(platform specs.Platform) specs.Platform { return platform } - -// ToProto converts from a slice of [Platform] to a slice of -// the protobuf definition [types.Platform]. -func ToProto(platforms []Platform) []*types.Platform { - ap := make([]*types.Platform, len(platforms)) - for i := range platforms { - ap[i] = &types.Platform{ - OS: platforms[i].OS, - Architecture: platforms[i].Architecture, - Variant: platforms[i].Variant, - } - } - return ap -} - -// FromProto converts a slice of the protobuf definition [types.Platform] -// to a slice of [Platform]. -func FromProto(platforms []*types.Platform) []specs.Platform { - op := make([]specs.Platform, len(platforms)) - for i := range platforms { - op[i] = specs.Platform{ - OS: platforms[i].OS, - Architecture: platforms[i].Architecture, - Variant: platforms[i].Variant, - } - } - return op -} diff --git a/services/introspection/local.go b/services/introspection/local.go index f1e1853cf..5ed328b40 100644 --- a/services/introspection/local.go +++ b/services/introspection/local.go @@ -24,9 +24,9 @@ import ( "sync" api "github.com/containerd/containerd/api/services/introspection/v1" + "github.com/containerd/containerd/api/types" "github.com/containerd/containerd/errdefs" "github.com/containerd/containerd/filters" - "github.com/containerd/containerd/platforms" "github.com/containerd/containerd/plugin" ptypes "github.com/containerd/containerd/protobuf/types" "github.com/containerd/containerd/services" @@ -222,7 +222,7 @@ func pluginsToPB(plugins []*plugin.Plugin) []*api.Plugin { Type: p.Registration.Type.String(), ID: p.Registration.ID, Requires: requires, - Platforms: platforms.ToProto(p.Meta.Platforms), + Platforms: types.OCIPlatformToProto(p.Meta.Platforms), Capabilities: p.Meta.Capabilities, Exports: p.Meta.Exports, InitErr: initErr,