Simplify Debug Transformer interface and allow Apply to fail on images

- avoid panics by returning a ContainerDebugConfiguration instead of a pointer
- allow returning a different debug-helper image
- return a more helpful error message

pkg/skaffold/debug/debug_test.go

@@ -111,18 +111,14 @@ func (t testTransformer) IsApplicable(config imageConfiguration) bool {
 	return true
 }
 
-func (t testTransformer) RuntimeSupportImage() string {
-	return ""
-}
-
-func (t testTransformer) Apply(container *v1.Container, config imageConfiguration, portAlloc portAllocator) *ContainerDebugConfiguration {
+func (t testTransformer) Apply(container *v1.Container, config imageConfiguration, portAlloc portAllocator) (ContainerDebugConfiguration, string, error) {
 	port := portAlloc(9999)
 	container.Ports = append(container.Ports, v1.ContainerPort{Name: "test", ContainerPort: port})
 
 	testEnv := v1.EnvVar{Name: "KEY", Value: "value"}
 	container.Env = append(container.Env, testEnv)
 
-	return &ContainerDebugConfiguration{Runtime: "test"}
+	return ContainerDebugConfiguration{Runtime: "test"}, "", nil
 }
 
 func TestApplyDebuggingTransforms(t *testing.T) {

pkg/skaffold/debug/transform.go

@@ -102,11 +102,11 @@ type containerTransformer interface {
 	// IsApplicable determines if this container is suitable to be transformed.
 	IsApplicable(config imageConfiguration) bool
 
-	// RuntimeSupportImage returns the associated duct-tape helper image required or empty string
-	RuntimeSupportImage() string
-
-	// Apply configures a container definition for debugging, returning the debug configuration details or `nil` if it could not be done
-	Apply(container *v1.Container, config imageConfiguration, portAlloc portAllocator) *ContainerDebugConfiguration
+	// Apply configures a container definition for debugging, returning the debug configuration details
+	// and required initContainer (an empty string if not required), or return a non-nil error if
+	// the container could not be transformed.  The initContainer image is intended to install any
+	// required debug support tools.
+	Apply(container *v1.Container, config imageConfiguration, portAlloc portAllocator) (ContainerDebugConfiguration, string, error)
 }
 
 const (
@@ -195,18 +195,19 @@ func transformPodSpec(metadata *metav1.ObjectMeta, podSpec *v1.PodSpec, retrieve
 			continue
 		}
 		// requiredImage, if not empty, is the image ID providing the debugging support files
+		// `err != nil` means that the container did not or could not be transformed
 		if configuration, requiredImage, err := transformContainer(container, imageConfig, portAlloc); err == nil {
 			configuration.Artifact = imageConfig.artifact
 			configuration.WorkingDir = imageConfig.workingDir
-			configurations[container.Name] = *configuration
+			configurations[container.Name] = configuration
 			if len(requiredImage) > 0 {
 				logrus.Infof("%q requires debugging support image %q", container.Name, requiredImage)
 				containersRequiringSupport = append(containersRequiringSupport, container)
 				requiredSupportImages[requiredImage] = true
 			}
 			// todo: add this artifact to the watch list?
 		} else {
-			logrus.Infof("Image %q not configured for debugging: %v", container.Name, err)
+			logrus.Warnf("Image %q not configured for debugging: %v", container.Name, err)
 		}
 	}
 
@@ -282,7 +283,7 @@ func isPortAvailable(podSpec *v1.PodSpec, port int32) bool {
 // transformContainer rewrites the container definition to enable debugging.
 // Returns a debugging configuration description with associated language runtime support
 // container image, or an error if the rewrite was unsuccessful.
-func transformContainer(container *v1.Container, config imageConfiguration, portAlloc portAllocator) (*ContainerDebugConfiguration, string, error) {
+func transformContainer(container *v1.Container, config imageConfiguration, portAlloc portAllocator) (ContainerDebugConfiguration, string, error) {
 	// update image configuration values with those set in the k8s manifest
 	for _, envVar := range container.Env {
 		// FIXME handle ValueFrom?
@@ -301,10 +302,10 @@ func transformContainer(container *v1.Container, config imageConfiguration, port
 
 	for _, transform := range containerTransforms {
 		if transform.IsApplicable(config) {
-			return transform.Apply(container, config, portAlloc), transform.RuntimeSupportImage(), nil
+			return transform.Apply(container, config, portAlloc)
 		}
 	}
-	return nil, "", fmt.Errorf("unable to determine runtime for %q", container.Name)
+	return ContainerDebugConfiguration{}, "", fmt.Errorf("unable to determine runtime for %q", container.Name)
 }
 
 func encodeConfigurations(configurations map[string]ContainerDebugConfiguration) string {

pkg/skaffold/debug/transform_go.go

@@ -73,13 +73,9 @@ func (t dlvTransformer) IsApplicable(config imageConfiguration) bool {
 	return false
 }
 
-func (t dlvTransformer) RuntimeSupportImage() string {
-	return "go"
-}
-
 // Apply configures a container definition for Go with Delve.
-// Returns a simple map describing the debug configuration details.
-func (t dlvTransformer) Apply(container *v1.Container, config imageConfiguration, portAlloc portAllocator) *ContainerDebugConfiguration {
+// Returns the debug configuration details, with the "go" support image
+func (t dlvTransformer) Apply(container *v1.Container, config imageConfiguration, portAlloc portAllocator) (ContainerDebugConfiguration, string, error) {
 	logrus.Infof("Configuring %q for Go/Delve debugging", container.Name)
 
 	// try to find existing `dlv` command
@@ -96,17 +92,16 @@ func (t dlvTransformer) Apply(container *v1.Container, config imageConfiguration
 			container.Args = rewriteDlvCommandLine(config.arguments, *spec, container.Args)
 
 		default:
-			logrus.Warnf("Skipping %q as does not appear to be Go-based", container.Name)
-			return nil
+			return ContainerDebugConfiguration{}, "", fmt.Errorf("container %q has no command-line", container.Name)
 		}
 	}
 
 	container.Ports = exposePort(container.Ports, "dlv", int32(spec.port))
 
-	return &ContainerDebugConfiguration{
+	return ContainerDebugConfiguration{
 		Runtime: "go",
 		Ports:   map[string]uint32{"dlv": uint32(spec.port)},
-	}
+	}, "go", nil
 }
 
 func retrieveDlvSpec(config imageConfiguration) *dlvSpec {

pkg/skaffold/debug/transform_go_test.go

@@ -114,22 +114,21 @@ func TestDlvTransformer_IsApplicable(t *testing.T) {
 	}
 }
 
-func TestDlvTransformer_RuntimeSupportImage(t *testing.T) {
-	testutil.CheckDeepEqual(t, "go", dlvTransformer{}.RuntimeSupportImage())
-}
-
 func TestDlvTransformerApply(t *testing.T) {
 	tests := []struct {
 		description   string
 		containerSpec v1.Container
 		configuration imageConfiguration
+		shouldErr     bool
 		result        v1.Container
+		debugConfig   ContainerDebugConfiguration
+		image         string
 	}{
 		{
 			description:   "empty",
 			containerSpec: v1.Container{},
 			configuration: imageConfiguration{},
-			result:        v1.Container{},
+			shouldErr:     true,
 		},
 		{
 			description:   "basic",
@@ -139,6 +138,8 @@ func TestDlvTransformerApply(t *testing.T) {
 				Command: []string{"/dbg/go/bin/dlv", "exec", "--headless", "--continue", "--accept-multiclient", "--listen=localhost:56268", "--api-version=2", "app", "--", "arg"},
 				Ports:   []v1.ContainerPort{{Name: "dlv", ContainerPort: 56268}},
 			},
+			debugConfig: ContainerDebugConfiguration{Runtime: "go", Ports: map[string]uint32{"dlv": 56268}},
+			image:       "go",
 		},
 		{
 			description: "existing port",
@@ -150,6 +151,8 @@ func TestDlvTransformerApply(t *testing.T) {
 				Command: []string{"/dbg/go/bin/dlv", "exec", "--headless", "--continue", "--accept-multiclient", "--listen=localhost:56268", "--api-version=2", "app", "--", "arg"},
 				Ports:   []v1.ContainerPort{{Name: "http-server", ContainerPort: 8080}, {Name: "dlv", ContainerPort: 56268}},
 			},
+			debugConfig: ContainerDebugConfiguration{Runtime: "go", Ports: map[string]uint32{"dlv": 56268}},
+			image:       "go",
 		},
 		{
 			description: "existing dlv port",
@@ -161,6 +164,8 @@ func TestDlvTransformerApply(t *testing.T) {
 				Command: []string{"/dbg/go/bin/dlv", "exec", "--headless", "--continue", "--accept-multiclient", "--listen=localhost:56268", "--api-version=2", "app", "--", "arg"},
 				Ports:   []v1.ContainerPort{{Name: "dlv", ContainerPort: 56268}},
 			},
+			debugConfig: ContainerDebugConfiguration{Runtime: "go", Ports: map[string]uint32{"dlv": 56268}},
+			image:       "go",
 		},
 		{
 			description:   "command not entrypoint",
@@ -170,6 +175,8 @@ func TestDlvTransformerApply(t *testing.T) {
 				Args:  []string{"/dbg/go/bin/dlv", "exec", "--headless", "--continue", "--accept-multiclient", "--listen=localhost:56268", "--api-version=2", "app", "--", "arg"},
 				Ports: []v1.ContainerPort{{Name: "dlv", ContainerPort: 56268}},
 			},
+			debugConfig: ContainerDebugConfiguration{Runtime: "go", Ports: map[string]uint32{"dlv": 56268}},
+			image:       "go",
 		},
 		{
 			description: "entrypoint with args in container spec",
@@ -182,16 +189,21 @@ func TestDlvTransformerApply(t *testing.T) {
 				Args:    []string{"arg1", "arg2"},
 				Ports:   []v1.ContainerPort{{Name: "dlv", ContainerPort: 56268}},
 			},
+			debugConfig: ContainerDebugConfiguration{Runtime: "go", Ports: map[string]uint32{"dlv": 56268}},
+			image:       "go",
 		},
 	}
 	var identity portAllocator = func(port int32) int32 {
 		return port
 	}
 	for _, test := range tests {
 		testutil.Run(t, test.description, func(t *testutil.T) {
-			dlvTransformer{}.Apply(&test.containerSpec, test.configuration, identity)
+			config, image, err := dlvTransformer{}.Apply(&test.containerSpec, test.configuration, identity)
 
+			t.CheckError(test.shouldErr, err)
 			t.CheckDeepEqual(test.result, test.containerSpec)
+			t.CheckDeepEqual(test.debugConfig, config)
+			t.CheckDeepEqual(test.image, image)
 		})
 	}
 }

pkg/skaffold/debug/transform_jvm.go

@@ -63,14 +63,9 @@ type jdwpSpec struct {
 	server  bool
 }
 
-func (t jdwpTransformer) RuntimeSupportImage() string {
-	// no additional support required
-	return ""
-}
-
 // Apply configures a container definition for JVM debugging.
 // Returns a simple map describing the debug configuration details.
-func (t jdwpTransformer) Apply(container *v1.Container, config imageConfiguration, portAlloc portAllocator) *ContainerDebugConfiguration {
+func (t jdwpTransformer) Apply(container *v1.Container, config imageConfiguration, portAlloc portAllocator) (ContainerDebugConfiguration, string, error) {
 	logrus.Infof("Configuring %q for JVM debugging", container.Name)
 	// try to find existing JAVA_TOOL_OPTIONS or jdwp command argument
 	spec := retrieveJdwpSpec(config)
@@ -89,10 +84,10 @@ func (t jdwpTransformer) Apply(container *v1.Container, config imageConfiguratio
 
 	container.Ports = exposePort(container.Ports, "jdwp", port)
 
-	return &ContainerDebugConfiguration{
+	return ContainerDebugConfiguration{
 		Runtime: "jvm",
 		Ports:   map[string]uint32{"jdwp": uint32(port)},
-	}
+	}, "", nil
 }
 
 func retrieveJdwpSpec(config imageConfiguration) *jdwpSpec {

pkg/skaffold/debug/transform_jvm_test.go

@@ -29,10 +29,6 @@ import (
 	"github.com/GoogleContainerTools/skaffold/testutil"
 )
 
-func TestJdwpTransformer_RuntimeSupportImage(t *testing.T) {
-	testutil.CheckDeepEqual(t, "", jdwpTransformer{}.RuntimeSupportImage())
-}
-
 func TestJdwpTransformer_IsApplicable(t *testing.T) {
 	tests := []struct {
 		description string
@@ -95,6 +91,8 @@ func TestJdwpTransformerApply(t *testing.T) {
 		containerSpec v1.Container
 		configuration imageConfiguration
 		result        v1.Container
+		debugConfig   ContainerDebugConfiguration
+		image         string
 	}{
 		{
 			description:   "empty",
@@ -104,6 +102,7 @@ func TestJdwpTransformerApply(t *testing.T) {
 				Env:   []v1.EnvVar{{Name: "JAVA_TOOL_OPTIONS", Value: "-agentlib:jdwp=transport=dt_socket,server=y,address=5005,suspend=n,quiet=y"}},
 				Ports: []v1.ContainerPort{{Name: "jdwp", ContainerPort: 5005}},
 			},
+			debugConfig: ContainerDebugConfiguration{Runtime: "jvm", Ports: map[string]uint32{"jdwp": 5005}},
 		},
 		{
 			description: "existing port",
@@ -115,6 +114,7 @@ func TestJdwpTransformerApply(t *testing.T) {
 				Env:   []v1.EnvVar{{Name: "JAVA_TOOL_OPTIONS", Value: "-agentlib:jdwp=transport=dt_socket,server=y,address=5005,suspend=n,quiet=y"}},
 				Ports: []v1.ContainerPort{{Name: "http-server", ContainerPort: 8080}, {Name: "jdwp", ContainerPort: 5005}},
 			},
+			debugConfig: ContainerDebugConfiguration{Runtime: "jvm", Ports: map[string]uint32{"jdwp": 5005}},
 		},
 		{
 			description: "existing jdwp spec",
@@ -127,6 +127,7 @@ func TestJdwpTransformerApply(t *testing.T) {
 				Env:   []v1.EnvVar{{Name: "JAVA_TOOL_OPTIONS", Value: "-agentlib:jdwp=transport=dt_socket,server=y,address=8000,suspend=n,quiet=y"}},
 				Ports: []v1.ContainerPort{{ContainerPort: 5005}, {Name: "jdwp", ContainerPort: 8000}},
 			},
+			debugConfig: ContainerDebugConfiguration{Runtime: "jvm", Ports: map[string]uint32{"jdwp": 8000}},
 		},
 		{
 			description: "existing jdwp port and JAVA_TOOL_OPTIONS",
@@ -139,16 +140,21 @@ func TestJdwpTransformerApply(t *testing.T) {
 				Env:   []v1.EnvVar{{Name: "FOO", Value: "BAR"}, {Name: "JAVA_TOOL_OPTIONS", Value: "-Xms1g -agentlib:jdwp=transport=dt_socket,server=y,address=5005,suspend=n,quiet=y"}},
 				Ports: []v1.ContainerPort{{Name: "jdwp", ContainerPort: 5005}},
 			},
+			debugConfig: ContainerDebugConfiguration{Runtime: "jvm", Ports: map[string]uint32{"jdwp": 5005}},
 		},
 	}
 	var identity portAllocator = func(port int32) int32 {
 		return port
 	}
 	for _, test := range tests {
 		testutil.Run(t, test.description, func(t *testutil.T) {
-			jdwpTransformer{}.Apply(&test.containerSpec, test.configuration, identity)
+			config, image, err := jdwpTransformer{}.Apply(&test.containerSpec, test.configuration, identity)
 
+			// Apply never fails since there's always the option to set JAVA_TOOL_OPTIONS
+			t.CheckNil(err)
 			t.CheckDeepEqual(test.result, test.containerSpec)
+			t.CheckDeepEqual(test.debugConfig, config)
+			t.CheckDeepEqual(test.image, image)
 		})
 	}
 }

pkg/skaffold/debug/transform_nodejs.go

@@ -67,14 +67,9 @@ func (t nodeTransformer) IsApplicable(config imageConfiguration) bool {
 	return false
 }
 
-func (t nodeTransformer) RuntimeSupportImage() string {
-	// no additional support required
-	return ""
-}
-
 // Apply configures a container definition for NodeJS Chrome V8 Inspector.
 // Returns a simple map describing the debug configuration details.
-func (t nodeTransformer) Apply(container *v1.Container, config imageConfiguration, portAlloc portAllocator) *ContainerDebugConfiguration {
+func (t nodeTransformer) Apply(container *v1.Container, config imageConfiguration, portAlloc portAllocator) (ContainerDebugConfiguration, string, error) {
 	logrus.Infof("Configuring %q for node.js debugging", container.Name)
 
 	// try to find existing `--inspect` command
@@ -102,10 +97,10 @@ func (t nodeTransformer) Apply(container *v1.Container, config imageConfiguratio
 
 	container.Ports = exposePort(container.Ports, "devtools", spec.port)
 
-	return &ContainerDebugConfiguration{
+	return ContainerDebugConfiguration{
 		Runtime: "nodejs",
 		Ports:   map[string]uint32{"devtools": uint32(spec.port)},
-	}
+	}, "", nil
 }
 
 func retrieveNodeInspectSpec(config imageConfiguration) *inspectSpec {