Make the TestDebug/buildpacks integration tests pass

Tests were failing as I wasn't rewriting the entrypoint, so that Platform 0.4-style process executables (/cnb/process/web) continued through and the debug transformers wouldn't recognize the executable. - fix adjustCommandLine() and add tests - fix testutil.isNil to handle function pointers - narrow isCNBImage() to only allow entrypoint to be 1-element array and add tests - go style nits (rename isCnbImage -> isCNBImage, godoc style)
GoogleContainerTools · Nov 27, 2020 · ebd77a5 · ebd77a5
1 parent 18169aa
commit ebd77a5
Show file tree

Hide file tree

Showing 4 changed files with 140 additions and 18 deletions.
diff --git a/pkg/skaffold/debug/cnb.go b/pkg/skaffold/debug/cnb.go
@@ -44,14 +44,20 @@ func init() {
 	entrypointLaunchers = append(entrypointLaunchers, cnbLauncher)
 }
 
+// isCNBImage returns true if this image is a CNB-produced image.
 // CNB images use a special launcher as the entrypoint. In CNB Platform API 0.3,
 // this was always `/cnb/lifecycle/launcher`, but Platform API 0.4 (introduced in pack 0.13)
 // allows using a symlink to a file in `/cnb/process/<type>`.  More below.
-func isCnbImage(ic imageConfiguration) bool {
+func isCNBImage(ic imageConfiguration) bool {
 	if _, found := ic.labels["io.buildpacks.stack.id"]; !found {
 		return false
 	}
-	return len(ic.entrypoint) > 0 && (ic.entrypoint[0] == cnbLauncher || strings.HasPrefix(ic.entrypoint[0], cnbProcessLauncherPrefix))
+	return len(ic.entrypoint) == 1 && (ic.entrypoint[0] == cnbLauncher || strings.HasPrefix(ic.entrypoint[0], cnbProcessLauncherPrefix))
+}
+
+// hasCNBLauncherEntrypoint returns true if the entrypoint is the cnbLauncher.
+func hasCNBLauncherEntrypoint(ic imageConfiguration) bool {
+	return len(ic.entrypoint) == 1 && ic.entrypoint[0] == cnbLauncher
 }
 
 // updateForCNBImage normalizes a CNB image by rewriting the CNB launch configuration into
@@ -171,13 +177,15 @@ func adjustCommandLine(m cnb.BuildMetadata, ic imageConfiguration) (imageConfigu
 			} else {
 				args := append([]string{p.Command}, p.Args...)
 				args = append(args, clArgs...)
+				ic.entrypoint = []string{cnbLauncher}
 				ic.arguments = args
 				return ic, func(transformed []string) []string {
 					return append([]string{"--"}, transformed...)
 				}
 			}
 		}
 		// Script type: split p.Command, pass it through the transformer, and then reassemble in the rewriter.
+		ic.entrypoint = []string{cnbLauncher}
 		if args, err := shell.Split(p.Command); err == nil {
 			ic.arguments = args
 		} else {
@@ -197,22 +205,16 @@ func adjustCommandLine(m cnb.BuildMetadata, ic imageConfiguration) (imageConfigu
 	}
 
 	// ic.arguments[0] is a shell script:  split it, pass it through the transformer, and then reassemble in the rewriter.
-	// If it can't be split, then we fall through and return it untouched, to be handled by the normal debug process.
-	var rewriter func(transformed []string) []string
-	if args, err := shell.Split(ic.arguments[0]); err == nil {
-		remnants := ic.arguments[1:]
-		ic.arguments = args
-		rewriter = func(transformed []string) []string {
-			// reassemble back into a script with arguments
-			return append([]string{shJoin(transformed)}, remnants...)
+	// If it can't be split, then we return it untouched, to be handled by the normal debug process.
+	if cmdline, err := shell.Split(ic.arguments[0]); err == nil {
+		positionals := ic.arguments[1:] // save aside the script positional arguments
+		ic.arguments = cmdline
+		return ic, func(transformed []string) []string {
+			// reassemble back into a script with the positional arguments
+			return append([]string{shJoin(transformed)}, positionals...)
 		}
 	}
-	return ic, rewriter
-}
-
-// hasCNBLauncherEntrypoint returns true if the entrypoint is the cnbLauncher.
-func hasCNBLauncherEntrypoint(ic imageConfiguration) bool {
-	return len(ic.entrypoint) > 0 && ic.entrypoint[0] == cnbLauncher
+	return ic, nil
 }
 
 // findCNBProcess tries to resolve a CNB process definition given the image configuration.

diff --git a/pkg/skaffold/debug/cnb_test.go b/pkg/skaffold/debug/cnb_test.go
@@ -22,11 +22,32 @@ import (
 
 	cnb "github.com/buildpacks/lifecycle"
 	"github.com/buildpacks/lifecycle/launch"
+	"github.com/google/go-cmp/cmp"
 	v1 "k8s.io/api/core/v1"
 
 	"github.com/GoogleContainerTools/skaffold/testutil"
 )
 
+func TestIsCNBImage(t *testing.T) {
+	tests := []struct {
+		description string
+		input       imageConfiguration
+		expected    bool
+	}{
+		{"non-cnb image", imageConfiguration{entrypoint: []string{"/usr/bin/java", "-jar", "foo.jar"}}, false},
+		{"implicit platform 0.3 with launcher missing label", imageConfiguration{entrypoint: []string{cnbLauncher}}, false},
+		{"implicit platform 0.3 with launcher", imageConfiguration{entrypoint: []string{cnbLauncher}, labels: map[string]string{"io.buildpacks.stack.id": "not checked"}}, true},
+		{"explict platform 0.3 with launcher", imageConfiguration{entrypoint: []string{cnbLauncher}, env: map[string]string{"CNB_PLATFORM_API": "0.3"}, labels: map[string]string{"io.buildpacks.stack.id": "not checked"}}, true},
+		{"platform 0.4 with launcher", imageConfiguration{entrypoint: []string{cnbLauncher}, env: map[string]string{"CNB_PLATFORM_API": "0.4"}, labels: map[string]string{"io.buildpacks.stack.id": "not checked"}}, true},
+		{"platform 0.4 with process executable", imageConfiguration{entrypoint: []string{"/cnb/process/diag"}, arguments: []string{"arg"}, env: map[string]string{"CNB_PLATFORM_API": "0.4"}, labels: map[string]string{"io.buildpacks.stack.id": "not checked"}}, true},
+		{"platform 0.4 with non-cnb entrypoint", imageConfiguration{entrypoint: []string{"/usr/bin/java", "-jar", "foo.jar"}, arguments: []string{"arg"}, env: map[string]string{"CNB_PLATFORM_API": "0.4"}, labels: map[string]string{"io.buildpacks.stack.id": "not checked"}}, false},
+	}
+	for _, test := range tests {
+		testutil.Run(t, test.description, func(t *testutil.T) {
+			t.CheckDeepEqual(test.expected, isCNBImage(test.input))
+		})
+	}
+}
 func TestHasCNBLauncherEntrypoint(t *testing.T) {
 	tests := []struct {
 		description string
@@ -80,6 +101,105 @@ func TestFindCNBProcess(t *testing.T) {
 	}
 }
 
+func TestAdjustCommandLine(t *testing.T) {
+	// metadata with default process type `web`
+	md := cnb.BuildMetadata{Processes: []launch.Process{
+		{Type: "web", Command: "webProcess arg1 arg2", Args: []string{"posArg1", "posArg2"}},
+		{Type: "diag", Command: "diagProcess", Args: []string{"posArg1", "posArg2"}, Direct: true},
+	}}
+	tests := []struct {
+		description string
+		input       imageConfiguration
+		result      imageConfiguration
+		hasRewriter bool
+	}{
+		{
+			description: "platform 0.3 default web process",
+			input:       imageConfiguration{entrypoint: []string{cnbLauncher}},
+			result:      imageConfiguration{entrypoint: []string{cnbLauncher}, arguments: []string{"webProcess", "arg1", "arg2"}},
+			hasRewriter: true,
+		},
+		{
+			description: "platform 0.3 explicit web",
+			input:       imageConfiguration{entrypoint: []string{cnbLauncher}, arguments: []string{"web"}},
+			result:      imageConfiguration{entrypoint: []string{cnbLauncher}, arguments: []string{"webProcess", "arg1", "arg2"}},
+			hasRewriter: true,
+		},
+		{
+			description: "platform 0.3 explicit diag",
+			input:       imageConfiguration{entrypoint: []string{cnbLauncher}, arguments: []string{"diag"}},
+			result:      imageConfiguration{entrypoint: []string{cnbLauncher}, arguments: []string{"diagProcess", "posArg1", "posArg2"}},
+			hasRewriter: true,
+		},
+		{
+			description: "platform 0.3 environment",
+			input:       imageConfiguration{entrypoint: []string{cnbLauncher}, env: map[string]string{"CNB_PROCESS_TYPE": "diag"}},
+			result:      imageConfiguration{entrypoint: []string{cnbLauncher}, arguments: []string{"diagProcess", "posArg1", "posArg2"}, env: map[string]string{"CNB_PROCESS_TYPE": "diag"}},
+			hasRewriter: true,
+		},
+		{
+			description: "platform 0.3 invalid process (env) should be untouched",
+			input:       imageConfiguration{entrypoint: []string{cnbLauncher}, env: map[string]string{"CNB_PROCESS_TYPE": "not-found"}},
+			result:      imageConfiguration{entrypoint: []string{cnbLauncher}, env: map[string]string{"CNB_PROCESS_TYPE": "not-found"}},
+			hasRewriter: false,
+		},
+		{
+			description: "platform 0.3 script-style with args",
+			input:       imageConfiguration{entrypoint: []string{cnbLauncher}, arguments: []string{"the command line", "arg"}},
+			result:      imageConfiguration{entrypoint: []string{cnbLauncher}, arguments: []string{"the", "command", "line"}},
+			hasRewriter: true,
+		},
+		{
+			description: "platform 0.3 direct with args",
+			input:       imageConfiguration{entrypoint: []string{cnbLauncher}, arguments: []string{"--", "the", "command", "line"}},
+			result:      imageConfiguration{entrypoint: []string{cnbLauncher}, arguments: []string{"the", "command", "line"}},
+			hasRewriter: true,
+		},
+		{
+			description: "platform 0.4 with no default should be unchanged",
+			input:       imageConfiguration{entrypoint: []string{cnbLauncher}, env: map[string]string{"CNB_PLATFORM_API": "0.4"}},
+			result:      imageConfiguration{entrypoint: []string{cnbLauncher}, env: map[string]string{"CNB_PLATFORM_API": "0.4"}},
+			hasRewriter: false,
+		},
+		{
+			description: "platform 0.4 process executable",
+			input:       imageConfiguration{entrypoint: []string{"/cnb/process/diag"}, arguments: []string{"arg"}, env: map[string]string{"CNB_PLATFORM_API": "0.4"}},
+			result:      imageConfiguration{entrypoint: []string{cnbLauncher}, arguments: []string{"diagProcess", "posArg1", "posArg2", "arg"}, env: map[string]string{"CNB_PLATFORM_API": "0.4"}},
+			hasRewriter: true,
+		},
+		{
+			description: "platform 0.4 invalid process (env) should be untouched",
+			input:       imageConfiguration{entrypoint: []string{cnbLauncher}, env: map[string]string{"CNB_PLATFORM_API": "0.4", "CNB_PROCESS_TYPE": "not-found"}},
+			result:      imageConfiguration{entrypoint: []string{cnbLauncher}, env: map[string]string{"CNB_PLATFORM_API": "0.4", "CNB_PROCESS_TYPE": "not-found"}},
+			hasRewriter: false,
+		},
+		{
+			description: "platform 0.4 direct with args",
+			input:       imageConfiguration{entrypoint: []string{cnbLauncher}, arguments: []string{"--", "the", "command", "line"}, env: map[string]string{"CNB_PLATFORM_API": "0.4"}},
+			result:      imageConfiguration{entrypoint: []string{cnbLauncher}, arguments: []string{"the", "command", "line"}, env: map[string]string{"CNB_PLATFORM_API": "0.4"}},
+			hasRewriter: true,
+		},
+		{
+			description: "platform 0.4 script-style with args",
+			input:       imageConfiguration{entrypoint: []string{cnbLauncher}, arguments: []string{"the command line", "arg"}, env: map[string]string{"CNB_PLATFORM_API": "0.4"}},
+			result:      imageConfiguration{entrypoint: []string{cnbLauncher}, arguments: []string{"the", "command", "line"}, env: map[string]string{"CNB_PLATFORM_API": "0.4"}},
+			hasRewriter: true,
+		},
+	}
+	for _, test := range tests {
+		testutil.Run(t, test.description, func(t *testutil.T) {
+			ic, rewriter := adjustCommandLine(md, test.input)
+			t.CheckDeepEqual(test.result, ic, cmp.AllowUnexported(test.result))
+			if test.hasRewriter {
+				// todo: can we test the rewriter?  We do excercise it in TestForCNBImage
+				t.CheckNotNil(rewriter)
+			} else {
+				t.CheckNil(rewriter)
+			}
+		})
+	}
+}
+
 func TestUpdateForCNBImage(t *testing.T) {
 	// metadata with default process type `web`
 	md := cnb.BuildMetadata{Processes: []launch.Process{

diff --git a/pkg/skaffold/debug/transform.go b/pkg/skaffold/debug/transform.go
@@ -333,7 +333,7 @@ func transformContainer(container *v1.Container, config imageConfiguration, port
 	next := func(container *v1.Container, config imageConfiguration) (ContainerDebugConfiguration, string, error) {
 		return performContainerTransform(container, config, portAlloc)
 	}
-	if isCnbImage(config) {
+	if isCNBImage(config) {
 		return updateForCNBImage(container, config, next)
 	}
 	return updateForShDashC(container, config, next)

diff --git a/testutil/util.go b/testutil/util.go
@@ -82,7 +82,7 @@ func (t *T) CheckNotNil(actual interface{}) {
 }
 
 func isNil(actual interface{}) bool {
-	return actual == nil || (reflect.ValueOf(actual).Kind() == reflect.Ptr && reflect.ValueOf(actual).IsNil())
+	return actual == nil || (reflect.ValueOf(actual).Kind() == reflect.Ptr && reflect.ValueOf(actual).IsNil()) || (reflect.ValueOf(actual).Kind() == reflect.Func && reflect.ValueOf(actual).IsZero())
 }
 
 func (t *T) CheckTrue(actual bool) {