Merge pull request #8165 from fangn2/config-options-followup

[transfer]Config options followup

cmd/ctr/commands/images/import.go

@@ -130,10 +130,10 @@ If foobar.tar contains an OCI ref named "latest" and anonymous ref "sha256:deadb
 			}
 
 			var platSpec ocispec.Platform
-			//Only when all-platforms not specified, we will check platform value
+			// Only when all-platforms not specified, we will check platform value
-			//Implicitly if the platforms is empty, it means all-platforms
+			// Implicitly if the platforms is empty, it means all-platforms
 			if !context.Bool("all-platforms") {
-				//If platform specified, use that one, if not use default
+				// If platform specified, use that one, if not use default
 				if platform := context.String("platform"); platform != "" {
 					platSpec, err = platforms.Parse(platform)
 					if err != nil {
@@ -147,12 +147,12 @@ If foobar.tar contains an OCI ref named "latest" and anonymous ref "sha256:deadb
 
 			if !context.Bool("no-unpack") {
 				snapshotter := context.String("snapshotter")
-				//If OS field is not empty, it means platSpec was updated in the above block
+				// If OS field is not empty, it means platSpec was updated in the above block
-				//i.e all-platforms was not specified
+				// i.e all-platforms was not specified
 				if platSpec.OS != "" {
 					opts = append(opts, image.WithUnpack(platSpec, snapshotter))
 				} else {
-					//empty spec means all platforms
+					// Empty spec means all platforms
 					var emptySpec ocispec.Platform
 					opts = append(opts, image.WithUnpack(emptySpec, snapshotter))
 				}

cmd/ctr/commands/images/pull.go

@@ -114,7 +114,7 @@ command. As part of this process, we do the following:
 				}
 				sopts = append(sopts, image.WithPlatforms(p...))
 
-				//set unpack configuration
+				// Set unpack configuration
 				for _, platform := range p {
 					sopts = append(sopts, image.WithUnpack(platform, context.String("snapshotter")))
 				}
@@ -125,8 +125,8 @@ command. As part of this process, we do the following:
 			if context.Bool("metadata-only") {
 				sopts = append(sopts, image.WithAllMetadata)
 				// Any with an empty set is None
-				// TODO: Specify way to specify not default platorm
+				// TODO: Specify way to specify not default platform
-				//config.PlatformMatcher = platforms.Any()
+				// config.PlatformMatcher = platforms.Any()
 			} else if context.Bool("all-metadata") {
 				sopts = append(sopts, image.WithAllMetadata)
 			}

pkg/transfer/local/import.go

@@ -89,7 +89,7 @@ func (ts *localTransferService) importStream(ctx context.Context, i transfer.Ima
 	handler = images.Handlers(handlerFunc)
 
 	// First find suitable platforms to unpack into
-	//If image storer is also an unpacker type, i.e implemented UnpackPlatforms() func
+	// If image storer is also an unpacker type, i.e implemented UnpackPlatforms() func
 	if iu, ok := is.(transfer.ImageUnpacker); ok {
 		unpacks := iu.UnpackPlatforms()
 		if len(unpacks) > 0 {

pkg/transfer/local/pull.go

@@ -116,7 +116,7 @@ func (ts *localTransferService) pull(ctx context.Context, ir transfer.ImageFetch
 		return err
 	}
 
-	//Set up baseHandlers from service configuration if present or create a new one
+	// Set up baseHandlers from service configuration if present or create a new one
 	if ts.config.BaseHandlers != nil {
 		baseHandlers = ts.config.BaseHandlers
 	} else {
@@ -151,12 +151,12 @@ func (ts *localTransferService) pull(ctx context.Context, ir transfer.ImageFetch
 	)...)
 
 	// First find suitable platforms to unpack into
-	//If image storer is also an unpacker type, i.e implemented UnpackPlatforms() func
+	// If image storer is also an unpacker type, i.e implemented UnpackPlatforms() func
 	if iu, ok := is.(transfer.ImageUnpacker); ok {
 		unpacks := iu.UnpackPlatforms()
 		if len(unpacks) > 0 {
 			uopts := []unpack.UnpackerOpt{}
-			//Only unpack if requested unpackconfig matches default/supported unpackconfigs
+			// Only unpack if requested unpackconfig matches default/supported unpackconfigs
 			for _, u := range unpacks {
 				matched, mu := getSupportedPlatform(u, ts.config.UnpackPlatforms)
 				if matched {
@@ -254,11 +254,11 @@ func fetchHandler(ingester content.Ingester, fetcher remotes.Fetcher, pt *Progre
 func getSupportedPlatform(uc transfer.UnpackConfiguration, supportedPlatforms []unpack.Platform) (bool, unpack.Platform) {
 	var u unpack.Platform
 	for _, sp := range supportedPlatforms {
-		//If both platform and snapshotter match, return the supportPlatform
+		// If both platform and snapshotter match, return the supportPlatform
-		//If platform matched and SnapshotterKey is empty, we assume client didn't pass SnapshotterKey
+		// If platform matched and SnapshotterKey is empty, we assume client didn't pass SnapshotterKey
-		//use default Snapshotter
+		// use default Snapshotter
 		if sp.Platform.Match(uc.Platform) {
-			//assuming sp.SnapshotterKey is not empty
+			// Assume sp.SnapshotterKey is not empty
 			if uc.Snapshotter == sp.SnapshotterKey {
 				return true, sp
 			} else if uc.Snapshotter == "" && sp.SnapshotterKey == containerd.DefaultSnapshotter {

pkg/transfer/local/pull_test.go

@@ -0,0 +1,153 @@
+/*
+   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 local
+
+import (
+	"testing"
+
+	"github.com/containerd/containerd"
+	"github.com/containerd/containerd/pkg/transfer"
+	"github.com/containerd/containerd/pkg/unpack"
+	"github.com/containerd/containerd/platforms"
+)
+
+func TestGetSupportedPlatform(t *testing.T) {
+	supportedPlatforms := []unpack.Platform{
+		{
+			Platform:       platforms.OnlyStrict(platforms.MustParse("linux/amd64")),
+			SnapshotterKey: "native",
+		},
+		{
+			Platform:       platforms.OnlyStrict(platforms.MustParse("linux/amd64")),
+			SnapshotterKey: "devmapper",
+		},
+		{
+			Platform:       platforms.OnlyStrict(platforms.MustParse("linux/arm64")),
+			SnapshotterKey: "native",
+		},
+		{
+			Platform:       platforms.OnlyStrict(platforms.MustParse("linux/arm")),
+			SnapshotterKey: "native",
+		},
+		{
+			Platform:       platforms.DefaultStrict(),
+			SnapshotterKey: containerd.DefaultSnapshotter,
+		},
+	}
+
+	for _, testCase := range []struct {
+		// Name is the name of the test
+		Name string
+
+		// Input
+		UnpackConfig       transfer.UnpackConfiguration
+		SupportedPlatforms []unpack.Platform
+
+		// Expected
+		Match            bool
+		ExpectedPlatform transfer.UnpackConfiguration
+	}{
+		{
+			Name: "No match on input linux/arm64 and devmapper snapshotter",
+			UnpackConfig: transfer.UnpackConfiguration{
+				Platform:    platforms.MustParse("linux/arm64"),
+				Snapshotter: "devmapper",
+			},
+			SupportedPlatforms: supportedPlatforms,
+			Match:              false,
+			ExpectedPlatform:   transfer.UnpackConfiguration{},
+		},
+		{
+			Name: "No match on input linux/386 and native snapshotter",
+			UnpackConfig: transfer.UnpackConfiguration{
+				Platform:    platforms.MustParse("linux/386"),
+				Snapshotter: "native",
+			},
+			SupportedPlatforms: supportedPlatforms,
+			Match:              false,
+			ExpectedPlatform:   transfer.UnpackConfiguration{},
+		},
+		{
+			Name: "Match linux/amd64 and native snapshotter",
+			UnpackConfig: transfer.UnpackConfiguration{
+				Platform:    platforms.MustParse("linux/amd64"),
+				Snapshotter: "native",
+			},
+			SupportedPlatforms: supportedPlatforms,
+			Match:              true,
+			ExpectedPlatform: transfer.UnpackConfiguration{
+				Platform:    platforms.MustParse("linux/amd64"),
+				Snapshotter: "native",
+			},
+		},
+		{
+			Name: "Match linux/arm64 and native snapshotter",
+			UnpackConfig: transfer.UnpackConfiguration{
+				Platform:    platforms.MustParse("linux/arm64"),
+				Snapshotter: "native",
+			},
+			SupportedPlatforms: supportedPlatforms,
+			Match:              true,
+			ExpectedPlatform: transfer.UnpackConfiguration{
+				Platform:    platforms.MustParse("linux/arm64"),
+				Snapshotter: "native",
+			},
+		},
+		{
+			Name: "Default platform input only match with defaultSnapshotter",
+			UnpackConfig: transfer.UnpackConfiguration{
+				Platform: platforms.DefaultSpec(),
+			},
+			SupportedPlatforms: supportedPlatforms,
+			Match:              true,
+			ExpectedPlatform: transfer.UnpackConfiguration{
+				Platform:    platforms.DefaultSpec(),
+				Snapshotter: containerd.DefaultSnapshotter,
+			},
+		},
+	} {
+		testCase := testCase
+		t.Run(testCase.Name, func(t *testing.T) {
+			m, sp := getSupportedPlatform(testCase.UnpackConfig, testCase.SupportedPlatforms)
+
+			// Match result should match expected
+			if m != testCase.Match {
+				t.Fatalf("Expect match result %v, but got %v", testCase.Match, m)
+			}
+
+			// If match result is false, the Platform should be nil too
+			if !m && sp.Platform != nil {
+				t.Fatalf("Expect nil Platform when we don't have a match")
+			}
+
+			// Snapshotter should match, empty string can be compared too
+			if sp.SnapshotterKey != testCase.ExpectedPlatform.Snapshotter {
+				t.Fatalf("Expect SnapshotterKey %v, but got %v", testCase.ExpectedPlatform.Snapshotter, sp.SnapshotterKey)
+			}
+
+			// If the matched Platform is not nil, it should match the expected Platform
+			if sp.Platform != nil && !sp.Platform.Match(testCase.ExpectedPlatform.Platform) {
+				t.Fatalf("Expect Platform %v doesn't match", testCase.ExpectedPlatform.Platform)
+			}
+			// If the ExectedPlatform is not empty, the matched Platform shoule not be nil either
+			if sp.Platform == nil && testCase.ExpectedPlatform.Platform.OS != "" {
+				t.Fatalf("Expect Platform %v doesn't match", testCase.ExpectedPlatform.Platform)
+			}
+		})
+	}
+
+}

pkg/transfer/local/transfer.go

@@ -39,9 +39,9 @@ type localTransferService struct {
 	leases  leases.Manager
 	content content.Store
 	images  images.Store
-	//limiter for upload
+	// limiter for upload
 	limiterU *semaphore.Weighted
-	//limiter for download operation
+	// limiter for download operation
 	limiterD *semaphore.Weighted
 	config   TransferConfig
 }
@@ -168,7 +168,7 @@ type TransferConfig struct {
 	// handlers.
 	BaseHandlers []images.Handler
 
-	//UnpackPlatforms are used to specify supported combination of platforms and snapshotters
+	// UnpackPlatforms are used to specify supported combination of platforms and snapshotters
 	UnpackPlatforms []unpack.Platform `toml:"unpack_platforms"`
 
 	// RegistryConfigPath is a path to the root directory containing registry-specific configurations