Debug support for pydevd, new --protocols debug flag (#5759)

Use the debug python launcher to add support for PyCharm.  Also improves debugging of applications using python modules (e.g., gunicorn)

Author: matthewmichihara

cmd/skaffold/app/cmd/debug.go

@@ -37,6 +37,9 @@ func NewCmdDebug() *cobra.Command {
 		WithLongDescription("Similar to `dev`, but configures the pipeline for debugging. "+
 			"Auto-build and sync is disabled by default to prevent accidentally tearing down debug sessions.").
 		WithCommonFlags().
+		WithFlags([]*Flag{
+			{Value: &debugging.Protocols, Name: "protocols", DefValue: []string{}, Usage: "Priority sorted order of debugger protocols to support."},
+		}).
 		WithExample("Launch with port-forwarding", "debug --port-forward").
 		WithHouseKeepingMessages().
 		NoArgs(func(ctx context.Context, out io.Writer) error {

docs/content/en/docs/references/cli/_index.md

@@ -435,6 +435,7 @@ Options:
   -p, --profile=[]: Activate profiles by name (prefixed with `-` to disable a profile)
       --profile-auto-activation=true: Set to false to disable profile auto activation
       --propagate-profiles=true: Setting '--propagate-profiles=false' disables propagating profiles set by the '--profile' flag across config dependencies. This mean that only profiles defined directly in the target 'skaffold.yaml' file are activated.
+      --protocols=[]: Priority sorted order of debugger protocols to support.
       --remote-cache-dir='': Specify the location of the git repositories cache (default $HOME/.skaffold/repos)
       --rpc-http-port=50052: tcp port to expose event REST API over HTTP
       --rpc-port=50051: tcp port to expose event API
@@ -488,6 +489,7 @@ Env vars:
 * `SKAFFOLD_PROFILE` (same as `--profile`)
 * `SKAFFOLD_PROFILE_AUTO_ACTIVATION` (same as `--profile-auto-activation`)
 * `SKAFFOLD_PROPAGATE_PROFILES` (same as `--propagate-profiles`)
+* `SKAFFOLD_PROTOCOLS` (same as `--protocols`)
 * `SKAFFOLD_REMOTE_CACHE_DIR` (same as `--remote-cache-dir`)
 * `SKAFFOLD_RPC_HTTP_PORT` (same as `--rpc-http-port`)
 * `SKAFFOLD_RPC_PORT` (same as `--rpc-port`)

pkg/skaffold/debug/debug_test.go

@@ -112,7 +112,7 @@ func (t testTransformer) IsApplicable(config imageConfiguration) bool {
 	return true
 }
 
-func (t testTransformer) Apply(container *v1.Container, config imageConfiguration, portAlloc portAllocator) (ContainerDebugConfiguration, string, error) {
+func (t testTransformer) Apply(container *v1.Container, config imageConfiguration, portAlloc portAllocator, overrideProtocols []string) (ContainerDebugConfiguration, string, error) {
 	port := portAlloc(9999)
 	container.Ports = append(container.Ports, v1.ContainerPort{Name: "test", ContainerPort: port})
 

pkg/skaffold/debug/transform.go

@@ -109,7 +109,7 @@ type containerTransformer interface {
 	// 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)
+	Apply(container *v1.Container, config imageConfiguration, portAlloc portAllocator, overrideProtocols []string) (ContainerDebugConfiguration, string, error)
 }
 
 const (
@@ -132,6 +132,8 @@ var containerTransforms []containerTransformer
 // as a command-line.  These entrypoints are ignored.
 var entrypointLaunchers []string
 
+var Protocols = []string{}
+
 // isEntrypointLauncher checks if the given entrypoint is a known entrypoint launcher,
 // meaning an entrypoint that treats the image's CMD as a command-line.
 func isEntrypointLauncher(entrypoint []string) bool {
@@ -454,7 +456,7 @@ func performContainerTransform(container *v1.Container, config imageConfiguratio
 	logrus.Tracef("Examining container %q with config %v", container.Name, config)
 	for _, transform := range containerTransforms {
 		if transform.IsApplicable(config) {
-			return transform.Apply(container, config, portAlloc)
+			return transform.Apply(container, config, portAlloc, Protocols)
 		}
 	}
 	return ContainerDebugConfiguration{}, "", fmt.Errorf("unable to determine runtime for %q", container.Name)

pkg/skaffold/debug/transform_go.go

@@ -92,7 +92,7 @@ func (t dlvTransformer) IsApplicable(config imageConfiguration) bool {
 
 // Apply configures a container definition for Go with Delve.
 // Returns the debug configuration details, with the "go" support image
-func (t dlvTransformer) Apply(container *v1.Container, config imageConfiguration, portAlloc portAllocator) (ContainerDebugConfiguration, string, error) {
+func (t dlvTransformer) Apply(container *v1.Container, config imageConfiguration, portAlloc portAllocator, overrideProtocols []string) (ContainerDebugConfiguration, string, error) {
 	logrus.Infof("Configuring %q for Go/Delve debugging", container.Name)
 
 	// try to find existing `dlv` command

pkg/skaffold/debug/transform_go_test.go

@@ -208,7 +208,7 @@ func TestDlvTransformerApply(t *testing.T) {
 	}
 	for _, test := range tests {
 		testutil.Run(t, test.description, func(t *testutil.T) {
-			config, image, err := dlvTransformer{}.Apply(&test.containerSpec, test.configuration, identity)
+			config, image, err := dlvTransformer{}.Apply(&test.containerSpec, test.configuration, identity, nil)
 
 			t.CheckError(test.shouldErr, err)
 			t.CheckDeepEqual(test.result, test.containerSpec)

pkg/skaffold/debug/transform_jvm.go

@@ -63,7 +63,7 @@ type jdwpSpec struct {
 
 // 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, string, error) {
+func (t jdwpTransformer) Apply(container *v1.Container, config imageConfiguration, portAlloc portAllocator, overrideProtocols []string) (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)

pkg/skaffold/debug/transform_jvm_test.go

@@ -156,7 +156,7 @@ func TestJdwpTransformerApply(t *testing.T) {
 	}
 	for _, test := range tests {
 		testutil.Run(t, test.description, func(t *testutil.T) {
-			config, image, err := jdwpTransformer{}.Apply(&test.containerSpec, test.configuration, identity)
+			config, image, err := jdwpTransformer{}.Apply(&test.containerSpec, test.configuration, identity, nil)
 
 			// Apply never fails since there's always the option to set JAVA_TOOL_OPTIONS
 			t.CheckNil(err)

pkg/skaffold/debug/transform_netcore.go

@@ -67,7 +67,7 @@ func (t netcoreTransformer) IsApplicable(config imageConfiguration) bool {
 
 // Apply configures a container definition for vsdbg.
 // Returns a simple map describing the debug configuration details.
-func (t netcoreTransformer) Apply(container *v1.Container, config imageConfiguration, portAlloc portAllocator) (ContainerDebugConfiguration, string, error) {
+func (t netcoreTransformer) Apply(container *v1.Container, config imageConfiguration, portAlloc portAllocator, overrideProtocols []string) (ContainerDebugConfiguration, string, error) {
 	logrus.Infof("Configuring %q for netcore debugging", container.Name)
 
 	return ContainerDebugConfiguration{

pkg/skaffold/debug/transform_netcore_test.go

@@ -120,7 +120,7 @@ func TestNetcoreTransformerApply(t *testing.T) {
 	}
 	for _, test := range tests {
 		testutil.Run(t, test.description, func(t *testutil.T) {
-			config, image, err := netcoreTransformer{}.Apply(&test.containerSpec, test.configuration, identity)
+			config, image, err := netcoreTransformer{}.Apply(&test.containerSpec, test.configuration, identity, nil)
 
 			t.CheckError(test.shouldErr, err)
 			t.CheckDeepEqual(test.result, test.containerSpec)

pkg/skaffold/debug/transform_nodejs.go

@@ -75,7 +75,7 @@ func (t nodeTransformer) IsApplicable(config imageConfiguration) bool {
 
 // 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, string, error) {
+func (t nodeTransformer) Apply(container *v1.Container, config imageConfiguration, portAlloc portAllocator, overrideProtocols []string) (ContainerDebugConfiguration, string, error) {
 	logrus.Infof("Configuring %q for node.js debugging", container.Name)
 
 	// try to find existing `--inspect` command

pkg/skaffold/debug/transform_nodejs_test.go

@@ -316,7 +316,7 @@ func TestNodeTransformer_Apply(t *testing.T) {
 	}
 	for _, test := range tests {
 		testutil.Run(t, test.description, func(t *testutil.T) {
-			config, image, err := nodeTransformer{}.Apply(&test.containerSpec, test.configuration, identity)
+			config, image, err := nodeTransformer{}.Apply(&test.containerSpec, test.configuration, identity, nil)
 
 			// Apply never fails since there's always the option to set NODE_OPTIONS
 			t.CheckNil(err)

pkg/skaffold/debug/transform_python.go

@@ -37,13 +37,20 @@ const (
 	// most examples use 5678
 	defaultPtvsdPort   = 5678
 	defaultDebugpyPort = 5678
+	defaultPydevdPort  = 5678
 )
 
 type pythonDebugType int
 
 const (
 	ptvsd pythonDebugType = iota
 	debugpy
+	pydevd
+)
+
+const (
+	pydevdProtocol = "pydevd"
+	dapProtocol    = "dap"
 )
 
 // pythonSpec captures the useful python-ptvsd devtools options
@@ -56,58 +63,84 @@ type pythonSpec struct {
 
 // isLaunchingPython determines if the arguments seems to be invoking python
 func isLaunchingPython(args []string) bool {
-	return len(args) > 0 &&
-		(args[0] == "python" || strings.HasSuffix(args[0], "/python") ||
-			args[0] == "python2" || strings.HasSuffix(args[0], "/python2") ||
-			args[0] == "python3" || strings.HasSuffix(args[0], "/python3"))
+	return len(args) > 0 && (args[0] == "python" || strings.HasSuffix(args[0], "/python") ||
+		args[0] == "python2" || strings.HasSuffix(args[0], "/python2") ||
+		args[0] == "python3" || strings.HasSuffix(args[0], "/python3"))
+}
+
+func hasCommonPythonEnvVars(env map[string]string) bool {
+	for _, key := range []string{
+		"PYTHON_VERSION",
+		"PYTHONVERBOSE",
+		"PYTHONINSPECT",
+		"PYTHONOPTIMIZE",
+		"PYTHONUSERSITE",
+		"PYTHONUNBUFFERED",
+		"PYTHONPATH",
+		"PYTHONUSERBASE",
+		"PYTHONWARNINGS",
+		"PYTHONHOME",
+		"PYTHONCASEOK",
+		"PYTHONIOENCODING",
+		"PYTHONHASHSEED",
+		"PYTHONDONTWRITEBYTECODE",
+	} {
+		if _, found := env[key]; found {
+			return true
+		}
+	}
+
+	return false
 }
 
 func (t pythonTransformer) IsApplicable(config imageConfiguration) bool {
-	// We can only put Python in debug mode by modifying the python command line,
-	// so looking for Python-related environment variables is insufficient.
+	if hasCommonPythonEnvVars(config.env) {
+		return true
+	}
+
 	if len(config.entrypoint) > 0 && !isEntrypointLauncher(config.entrypoint) {
 		return isLaunchingPython(config.entrypoint)
 	}
 	return isLaunchingPython(config.arguments)
 }
 
-// Apply configures a container definition for Python with ptvsd/debugpy.
+// Apply configures a container definition for Python with ptvsd/debugpy/pydevd.
 // Returns a simple map describing the debug configuration details.
-func (t pythonTransformer) Apply(container *v1.Container, config imageConfiguration, portAlloc portAllocator) (ContainerDebugConfiguration, string, error) {
+func (t pythonTransformer) Apply(container *v1.Container, config imageConfiguration, portAlloc portAllocator, overrideProtocols []string) (ContainerDebugConfiguration, string, error) {
 	logrus.Infof("Configuring %q for python debugging", container.Name)
 
 	// try to find existing `-mptvsd` or `-mdebugpy` command
 	if spec := retrievePythonDebugSpec(config); spec != nil {
-		container.Ports = exposePort(container.Ports, "dap", spec.port)
+		protocol := spec.protocol()
+		container.Ports = exposePort(container.Ports, protocol, spec.port)
 		return ContainerDebugConfiguration{
 			Runtime: "python",
-			Ports:   map[string]uint32{"dap": uint32(spec.port)},
+			Ports:   map[string]uint32{protocol: uint32(spec.port)},
 		}, "", nil
 	}
 
-	spec := &pythonSpec{debugger: debugpy, port: portAlloc(defaultDebugpyPort)}
+	spec := createPythonDebugSpec(overrideProtocols, portAlloc)
+
 	switch {
 	case isLaunchingPython(config.entrypoint):
 		container.Command = rewritePythonCommandLine(config.entrypoint, *spec)
 
 	case (len(config.entrypoint) == 0 || isEntrypointLauncher(config.entrypoint)) && isLaunchingPython(config.arguments):
 		container.Args = rewritePythonCommandLine(config.arguments, *spec)
 
+	case hasCommonPythonEnvVars(config.env):
+		container.Command = rewritePythonCommandLine(config.entrypoint, *spec)
+
 	default:
 		return ContainerDebugConfiguration{}, "", fmt.Errorf("%q does not appear to invoke python", container.Name)
 	}
 
-	pyUserBase := "/dbg/python"
-	if existing, found := config.env["PYTHONUSERBASE"]; found {
-		// todo: handle windows containers?
-		pyUserBase = pyUserBase + ":" + existing
-	}
-	container.Env = setEnvVar(container.Env, "PYTHONUSERBASE", pyUserBase)
-	container.Ports = exposePort(container.Ports, "dap", spec.port)
+	protocol := spec.protocol()
+	container.Ports = exposePort(container.Ports, protocol, spec.port)
 
 	return ContainerDebugConfiguration{
 		Runtime: "python",
-		Ports:   map[string]uint32{"dap": uint32(spec.port)},
+		Ports:   map[string]uint32{protocol: uint32(spec.port)},
 	}, "python", nil
 }
 
@@ -131,6 +164,19 @@ func extractPythonDebugSpec(args []string) *pythonSpec {
 	return nil
 }
 
+func createPythonDebugSpec(overrideProtocols []string, portAlloc portAllocator) *pythonSpec {
+	for _, p := range overrideProtocols {
+		switch p {
+		case pydevdProtocol:
+			return &pythonSpec{debugger: pydevd, port: portAlloc(defaultPydevdPort)}
+		case dapProtocol:
+			return &pythonSpec{debugger: debugpy, port: portAlloc(defaultDebugpyPort)}
+		}
+	}
+
+	return &pythonSpec{debugger: debugpy, port: portAlloc(defaultDebugpyPort)}
+}
+
 func extractPtvsdSpec(args []string) *pythonSpec {
 	if !hasPyModule("ptvsd", args) {
 		return nil
@@ -212,42 +258,45 @@ func hasPyModule(module string, args []string) bool {
 	return false
 }
 
-// rewritePythonCommandLine rewrites a python command-line to insert a `-mptvsd` etc
+// rewritePythonCommandLine rewrites a python command-line to use the debug-support's launcher.
 func rewritePythonCommandLine(commandLine []string, spec pythonSpec) []string {
 	// Assumes that commandLine[0] is "python" or "python3" etc
-	return util.StrSliceInsert(commandLine, 1, spec.asArguments())
+	return util.StrSliceInsert(commandLine, 0, spec.asArguments())
 }
 
 func (spec pythonSpec) asArguments() []string {
+	args := []string{"/dbg/python/launcher", "--mode", spec.launcherMode()}
+	if spec.port >= 0 {
+		args = append(args, "--port", strconv.FormatInt(int64(spec.port), 10))
+	}
+	if spec.wait {
+		args = append(args, "--wait")
+	}
+	args = append(args, "--")
+	return args
+}
+
+func (spec pythonSpec) launcherMode() string {
 	switch spec.debugger {
+	case pydevd:
+		return "pydevd"
 	case ptvsd:
-		args := []string{"-mptvsd"}
-		// --host is a mandatory argument
-		if spec.host == "" {
-			args = append(args, "--host", "0.0.0.0")
-		} else {
-			args = append(args, "--host", spec.host)
-		}
-		if spec.port >= 0 {
-			args = append(args, "--port", strconv.FormatInt(int64(spec.port), 10))
-		}
-		if spec.wait {
-			args = append(args, "--wait")
-		}
-		return args
-
+		return "ptvsd"
 	case debugpy:
-		args := []string{"-mdebugpy"}
+		return "debugpy"
+	}
+	logrus.Fatalf("invalid debugger type: %q", spec.debugger)
+	return ""
+}
 
-		if spec.host == "" {
-			args = append(args, "--listen", strconv.FormatInt(int64(spec.port), 10))
-		} else {
-			args = append(args, "--listen", fmt.Sprintf("%s:%d", spec.host, spec.port))
-		}
-		if spec.wait {
-			args = append(args, "--wait-for-client")
-		}
-		return args
+func (spec pythonSpec) protocol() string {
+	switch spec.debugger {
+	case pydevd:
+		return pydevdProtocol
+	case debugpy, ptvsd:
+		return dapProtocol
+	default:
+		logrus.Fatalf("invalid debugger type: %q", spec.debugger)
+		return dapProtocol
 	}
-	return nil
 }