Abstract k8s container representation from debug transformers (#6335)

* Abstract k8s container representation from debug transformers

* Use ContainerAdapter to abstract away kubernetes packages from debug package

* Move all k8s-related code out of pkg/skaffold/debug

* consolidate annotations and types packages

* preserve ValueFrom from k8s env

* make containerTransformers a map, simplify ValueFrom in adapter

Author: nkubala

cmd/skaffold/app/cmd/debug.go

@@ -22,7 +22,8 @@ import (
 
 	"github.com/spf13/cobra"
 
-	debugging "github.com/GoogleContainerTools/skaffold/pkg/skaffold/debug"
+	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/debug"
+	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/kubernetes/debugging"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/kubernetes/manifest"
 )
 
@@ -38,7 +39,7 @@ func NewCmdDebug() *cobra.Command {
 			"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."},
+			{Value: &debug.Protocols, Name: "protocols", DefValue: []string{}, Usage: "Priority sorted order of debugger protocols to support."},
 		}).
 		WithExample("Launch with port-forwarding", "debug --port-forward").
 		WithHouseKeepingMessages().

cmd/skaffold/app/cmd/filter.go

@@ -26,8 +26,8 @@ import (
 
 	"github.com/GoogleContainerTools/skaffold/cmd/skaffold/app/flags"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/config"
-	debugging "github.com/GoogleContainerTools/skaffold/pkg/skaffold/debug"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/graph"
+	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/kubernetes/debugging"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/kubernetes/manifest"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/runner"
 	latestV1 "github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest/v1"

integration/debug_test.go

@@ -23,7 +23,7 @@ import (
 	"time"
 
 	"github.com/GoogleContainerTools/skaffold/integration/skaffold"
-	debugannotations "github.com/GoogleContainerTools/skaffold/pkg/skaffold/debug/annotations"
+	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/debug/types"
 	"github.com/GoogleContainerTools/skaffold/proto/v1"
 	"github.com/GoogleContainerTools/skaffold/testutil"
 )
@@ -77,7 +77,7 @@ func TestDebug(t *testing.T) {
 			skaffold.Debug(test.args...).InDir(test.dir).InNs(ns.Name).RunBackground(t)
 
 			verifyDebugAnnotations := func(annotations map[string]string) {
-				var configs map[string]debugannotations.ContainerDebugConfiguration
+				var configs map[string]types.ContainerDebugConfiguration
 				if anno, found := annotations["debug.cloud.google.com/config"]; !found {
 					t.Errorf("deployment missing debug annotation: %v", annotations)
 				} else if err := json.Unmarshal([]byte(anno), &configs); err != nil {

pkg/skaffold/debug/apply_transforms.go

@@ -17,71 +17,21 @@ limitations under the License.
 package debug
 
 import (
-	"bufio"
-	"bytes"
 	"context"
 	"fmt"
 	"strings"
 
-	"github.com/sirupsen/logrus"
-	"k8s.io/apimachinery/pkg/runtime"
-	"k8s.io/apimachinery/pkg/runtime/serializer/json"
-	"k8s.io/client-go/kubernetes/scheme"
-
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/docker"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/graph"
-	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/kubernetes/manifest"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/output/log"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/runner/runcontext"
 )
 
-var (
-	decodeFromYaml = scheme.Codecs.UniversalDeserializer().Decode
-	encodeAsYaml   = func(o runtime.Object) ([]byte, error) {
-		s := json.NewYAMLSerializer(json.DefaultMetaFactory, scheme.Scheme, scheme.Scheme)
-		var b bytes.Buffer
-		w := bufio.NewWriter(&b)
-		if err := s.Encode(o, w); err != nil {
-			return nil, err
-		}
-		w.Flush()
-		return b.Bytes(), nil
-	}
-)
-
-// ApplyDebuggingTransforms applies language-platform-specific transforms to a list of manifests.
-func ApplyDebuggingTransforms(l manifest.ManifestList, builds []graph.Artifact, registries manifest.Registries) (manifest.ManifestList, error) {
-	ctx, cancel := context.WithCancel(context.Background())
-	defer cancel()
-
-	retriever := func(image string) (imageConfiguration, error) {
-		if artifact := findArtifact(image, builds); artifact != nil {
-			return retrieveImageConfiguration(ctx, artifact, registries.InsecureRegistries)
-		}
-		return imageConfiguration{}, fmt.Errorf("no build artifact for %q", image)
-	}
-	return applyDebuggingTransforms(l, retriever, registries.DebugHelpersRegistry)
-}
-
-func applyDebuggingTransforms(l manifest.ManifestList, retriever configurationRetriever, debugHelpersRegistry string) (manifest.ManifestList, error) {
-	var updated manifest.ManifestList
-	for _, manifest := range l {
-		obj, _, err := decodeFromYaml(manifest, nil, nil)
-		if err != nil {
-			log.Entry(context.TODO()).Debugf("Unable to interpret manifest for debugging: %v\n", err)
-		} else if transformManifest(obj, retriever, debugHelpersRegistry) {
-			manifest, err = encodeAsYaml(obj)
-			if err != nil {
-				return nil, fmt.Errorf("marshalling yaml: %w", err)
-			}
-			if logrus.IsLevelEnabled(logrus.DebugLevel) {
-				log.Entry(context.TODO()).Debugln("Applied debugging transform:\n", string(manifest))
-			}
-		}
-		updated = append(updated, manifest)
+var ConfigRetriever = func(ctx context.Context, image string, builds []graph.Artifact, registries map[string]bool) (ImageConfiguration, error) {
+	if artifact := findArtifact(image, builds); artifact != nil {
+		return retrieveImageConfiguration(ctx, artifact, registries)
 	}
-
-	return updated, nil
+	return ImageConfiguration{}, fmt.Errorf("no build artifact for %q", image)
 }
 
 // findArtifact finds the corresponding artifact for the given image.
@@ -102,32 +52,32 @@ func findArtifact(image string, builds []graph.Artifact) *graph.Artifact {
 
 // retrieveImageConfiguration retrieves the image container configuration for
 // the given build artifact
-func retrieveImageConfiguration(ctx context.Context, artifact *graph.Artifact, insecureRegistries map[string]bool) (imageConfiguration, error) {
+func retrieveImageConfiguration(ctx context.Context, artifact *graph.Artifact, insecureRegistries map[string]bool) (ImageConfiguration, error) {
 	// TODO: use the proper RunContext
 	apiClient, err := docker.NewAPIClient(ctx, &runcontext.RunContext{
 		InsecureRegistries: insecureRegistries,
 	})
 	if err != nil {
-		return imageConfiguration{}, fmt.Errorf("could not connect to local docker daemon: %w", err)
+		return ImageConfiguration{}, fmt.Errorf("could not connect to local docker daemon: %w", err)
 	}
 
 	// the apiClient will go to the remote registry if local docker daemon is not available
 	manifest, err := apiClient.ConfigFile(ctx, artifact.Tag)
 	if err != nil {
 		log.Entry(ctx).Debugf("Error retrieving image manifest for %v: %v", artifact.Tag, err)
-		return imageConfiguration{}, fmt.Errorf("retrieving image config for %q: %w", artifact.Tag, err)
+		return ImageConfiguration{}, fmt.Errorf("retrieving image config for %q: %w", artifact.Tag, err)
 	}
 
 	config := manifest.Config
 	log.Entry(ctx).Debugf("Retrieved local image configuration for %v: %v", artifact.Tag, config)
 	// need to duplicate slices as apiClient caches requests
-	return imageConfiguration{
-		artifact:   artifact.ImageName,
-		env:        envAsMap(config.Env),
-		entrypoint: dupArray(config.Entrypoint),
-		arguments:  dupArray(config.Cmd),
-		labels:     dupMap(config.Labels),
-		workingDir: config.WorkingDir,
+	return ImageConfiguration{
+		Artifact:   artifact.ImageName,
+		Env:        envAsMap(config.Env),
+		Entrypoint: dupArray(config.Entrypoint),
+		Arguments:  dupArray(config.Cmd),
+		Labels:     dupMap(config.Labels),
+		WorkingDir: config.WorkingDir,
 	}, nil
 }
 

pkg/skaffold/debug/cnb.go

@@ -25,9 +25,8 @@ import (
 	cnb "github.com/buildpacks/lifecycle"
 	cnbl "github.com/buildpacks/lifecycle/launch"
 	shell "github.com/kballard/go-shellquote"
-	v1 "k8s.io/api/core/v1"
 
-	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/debug/annotations"
+	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/debug/types"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/output/log"
 )
 
@@ -51,16 +50,16 @@ func init() {
 // 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 {
-	if _, found := ic.labels["io.buildpacks.stack.id"]; !found {
+func isCNBImage(ic ImageConfiguration) bool {
+	if _, found := ic.Labels["io.buildpacks.stack.id"]; !found {
 		return false
 	}
-	return len(ic.entrypoint) == 1 && (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
+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
@@ -106,43 +105,44 @@ func hasCNBLauncherEntrypoint(ic imageConfiguration) bool {
 //     the default process type.  `CNB_PROCESS_TYPE` is ignored in this situation.  A different process
 //     can be used by overriding the image entrypoint.  Direct and script launches are supported by
 //     setting the entrypoint to `/cnb/lifecycle/launcher` and providing the appropriate arguments.
-func updateForCNBImage(container *v1.Container, ic imageConfiguration, transformer func(container *v1.Container, ic imageConfiguration) (annotations.ContainerDebugConfiguration, string, error)) (annotations.ContainerDebugConfiguration, string, error) {
+func updateForCNBImage(adapter types.ContainerAdapter, ic ImageConfiguration, transformer func(adapter types.ContainerAdapter, ic ImageConfiguration) (types.ContainerDebugConfiguration, string, error)) (types.ContainerDebugConfiguration, string, error) {
 	// buildpacks/lifecycle 0.6.0 embeds the process definitions into a special image label.
 	// The build metadata isn't absolutely required as the image args could be
 	// a command line (e.g., `python xxx`) but it likely indicates the
 	// image was built with an older lifecycle.
-	metadataJSON, found := ic.labels["io.buildpacks.build.metadata"]
+	metadataJSON, found := ic.Labels["io.buildpacks.build.metadata"]
 	if !found {
-		return annotations.ContainerDebugConfiguration{}, "", fmt.Errorf("image is missing buildpacks metadata; perhaps built with older lifecycle?")
+		return types.ContainerDebugConfiguration{}, "", fmt.Errorf("image is missing buildpacks metadata; perhaps built with older lifecycle?")
 	}
 	m := cnb.BuildMetadata{}
 	if err := json.Unmarshal([]byte(metadataJSON), &m); err != nil {
-		return annotations.ContainerDebugConfiguration{}, "", fmt.Errorf("unable to parse image buildpacks metadata")
+		return types.ContainerDebugConfiguration{}, "", fmt.Errorf("unable to parse image buildpacks metadata")
 	}
 	if len(m.Processes) == 0 {
-		return annotations.ContainerDebugConfiguration{}, "", fmt.Errorf("buildpacks metadata has no processes")
+		return types.ContainerDebugConfiguration{}, "", fmt.Errorf("buildpacks metadata has no processes")
 	}
 
-	needsCnbLauncher := ic.entrypoint[0] != cnbLauncher
+	needsCnbLauncher := ic.Entrypoint[0] != cnbLauncher
 	// Rewrites the command-line with cnbLauncher as the entrypoint
 	ic, rewriter := adjustCommandLine(m, ic)
 
 	// The CNB launcher uses CNB_APP_DIR (defaults to /workspace) and ignores the image's working directory.
-	if appDir := ic.env["CNB_APP_DIR"]; appDir != "" {
-		ic.workingDir = appDir
+	if appDir := ic.Env["CNB_APP_DIR"]; appDir != "" {
+		ic.WorkingDir = appDir
 	} else {
-		ic.workingDir = "/workspace"
+		ic.WorkingDir = "/workspace"
 	}
 
-	c, img, err := transformer(container, ic)
+	c, img, err := transformer(adapter, ic)
 	if err != nil {
 		return c, img, err
 	}
 	// must explicitly modify the working dir as the imageConfig is lost after we return
 	if c.WorkingDir == "" {
-		c.WorkingDir = ic.workingDir
+		c.WorkingDir = ic.WorkingDir
 	}
 
+	container := adapter.GetContainer()
 	if container.Args != nil && rewriter != nil {
 		// Only rewrite the container if the arguments were changed: some transforms only alter
 		// env vars, and the image's arguments are not changed.
@@ -158,11 +158,11 @@ func updateForCNBImage(container *v1.Container, ic imageConfiguration, transform
 // in a form suitable for the normal `skaffold debug` transformations.  It returns an
 // amended configuration with a function to re-transform the command-line to the form
 // expected by cnbLauncher.
-func adjustCommandLine(m cnb.BuildMetadata, ic imageConfiguration) (imageConfiguration, func([]string) []string) {
+func adjustCommandLine(m cnb.BuildMetadata, ic ImageConfiguration) (ImageConfiguration, func([]string) []string) {
 	// check for direct exec
-	if hasCNBLauncherEntrypoint(ic) && len(ic.arguments) > 0 && ic.arguments[0] == "--" {
+	if hasCNBLauncherEntrypoint(ic) && len(ic.Arguments) > 0 && ic.Arguments[0] == "--" {
 		// strip and then restore the "--"
-		ic.arguments = ic.arguments[1:]
+		ic.Arguments = ic.Arguments[1:]
 		return ic, func(transformed []string) []string {
 			return append([]string{"--"}, transformed...)
 		}
@@ -180,19 +180,19 @@ 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
+				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}
+		ic.Entrypoint = []string{cnbLauncher}
 		if args, err := shell.Split(p.Command); err == nil {
-			ic.arguments = args
+			ic.Arguments = args
 		} else {
-			ic.arguments = []string{p.Command}
+			ic.Arguments = []string{p.Command}
 		}
 		return ic, func(transformed []string) []string {
 			// reassemble back into a script with arguments
@@ -202,16 +202,16 @@ func adjustCommandLine(m cnb.BuildMetadata, ic imageConfiguration) (imageConfigu
 		}
 	}
 
-	if len(ic.arguments) == 0 {
-		log.Entry(context.TODO()).Warnf("no CNB launch found for %s", ic.artifact)
+	if len(ic.Arguments) == 0 {
+		log.Entry(context.TODO()).Warnf("no CNB launch found for %s", ic.Artifact)
 		return ic, nil
 	}
 
 	// 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 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
+	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...)
@@ -222,11 +222,11 @@ func adjustCommandLine(m cnb.BuildMetadata, ic imageConfiguration) (imageConfigu
 
 // findCNBProcess tries to resolve a CNB process definition given the image configuration.
 // It is assumed that the image is a CNB image.
-func findCNBProcess(ic imageConfiguration, m cnb.BuildMetadata) (cnbl.Process, []string, bool) {
-	if hasCNBLauncherEntrypoint(ic) && len(ic.arguments) > 0 {
+func findCNBProcess(ic ImageConfiguration, m cnb.BuildMetadata) (cnbl.Process, []string, bool) {
+	if hasCNBLauncherEntrypoint(ic) && len(ic.Arguments) > 0 {
 		// the launcher accepts the first argument as a process type
-		if len(ic.arguments) == 1 {
-			processType := ic.arguments[0]
+		if len(ic.Arguments) == 1 {
+			processType := ic.Arguments[0]
 			for _, p := range m.Processes {
 				if p.Type == processType {
 					return p, nil, true // drop the argument
@@ -238,20 +238,20 @@ func findCNBProcess(ic imageConfiguration, m cnb.BuildMetadata) (cnbl.Process, [
 
 	// determine process-type
 	processType := "web" // default buildpacks process type
-	platformAPI := ic.env["CNB_PLATFORM_API"]
+	platformAPI := ic.Env["CNB_PLATFORM_API"]
 	if platformAPI == "0.4" {
 		// Platform API 0.4-style /cnb/process/xxx
-		if !strings.HasPrefix(ic.entrypoint[0], cnbProcessLauncherPrefix) {
+		if !strings.HasPrefix(ic.Entrypoint[0], cnbProcessLauncherPrefix) {
 			return cnbl.Process{}, nil, false
 		}
-		processType = ic.entrypoint[0][len(cnbProcessLauncherPrefix):]
-	} else if len(ic.env["CNB_PROCESS_TYPE"]) > 0 {
-		processType = ic.env["CNB_PROCESS_TYPE"]
+		processType = ic.Entrypoint[0][len(cnbProcessLauncherPrefix):]
+	} else if len(ic.Env["CNB_PROCESS_TYPE"]) > 0 {
+		processType = ic.Env["CNB_PROCESS_TYPE"]
 	}
 
 	for _, p := range m.Processes {
 		if p.Type == processType {
-			return p, ic.arguments, true
+			return p, ic.Arguments, true
 		}
 	}
 	return cnbl.Process{}, nil, false