Abstract k8s container representation from debug transformers

[nkubala]

Related: #6107

Description

This change removes references to k8sv1.Container objects in the debug transformer functions. This will allow these to be referenced by other deploy-related packages which don't operate on Kubernetes-specific objects.

To accomplish this, a ContainerAdapter interface is introduced, which provides an abstraction away from the underlying container representation. The ContainerAdapter will make use of the ExectuableContainer, which effectively intercepts the transforms being applied by the various transformers, and can then be used later to retroactively apply these transformations to the underlying container representation. This will allow the k8sv1.Container object to still be used by the Kubernetes debug transforms.

Follow-up Work (remove if N/A)

Implement debugging transformations for containers deployed by the Docker deployer.

[codecov]

Codecov Report

Merging #6335 (cae1c55) into main (a99ffc5) will increase coverage by 0.15%.
The diff coverage is 88.25%.

@@            Coverage Diff             @@
##             main    #6335      +/-   ##
==========================================
+ Coverage   70.27%   70.42%   +0.15%     
==========================================
  Files         510      515       +5     
  Lines       22988    23121     +133     
==========================================
+ Hits        16154    16284     +130     
- Misses       5776     5779       +3     
  Partials     1058     1058              

Impacted Files	Coverage Δ
cmd/skaffold/app/cmd/filter.go	25.00% <ø> (ø)
pkg/skaffold/debug/apply_transforms.go	23.07% <0.00%> (-12.83%)	⬇️
pkg/skaffold/initializer/deploy/kubectl.go	88.88% <ø> (+3.17%)	⬆️
pkg/skaffold/runner/v1/dev.go	73.12% <0.00%> (ø)
pkg/skaffold/runner/v1/runner.go	0.00% <ø> (ø)
pkg/skaffold/schema/latest/v1/config.go	62.16% <ø> (ø)
pkg/skaffold/test/test_factory.go	76.19% <ø> (-1.09%)	⬇️
pkg/skaffold/hooks/types.go	75.00% <75.00%> (ø)
pkg/skaffold/debug/transform_nodejs.go	80.18% <76.00%> (+0.55%)	⬆️
pkg/skaffold/kubernetes/debugging/transform.go	84.44% <84.44%> (ø)
... and 22 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0925a54...cae1c55. Read the comment docs.

[briandealwis]

I'd recommend a bigger refactor. Basically the containerTransformers need to:

• possibly rewrite the command-line
• possibly rewrite (add to) the environment
• allocate and a port with a name (recorded in the ContainerDebugConfiguration)
• return a debug-setup image reference

The Kubernetes manifest transformer turns that into a set of changes against a podspec, altering the container Command and Args, adding a ContainerPort, and adding an initContainer + volume. And it provides a port-allocator that looks through the podspec to ensure it doesn't allocate a clashing port.

So I think you could change the containerTransformer.Apply() method to drop the v1.Container and instead return an updated imageConfiguration object. You could then get rid of the Adapter, ExecutableContainer, and ContainerPort objects.

Then the Kubernetes code would be the rest of the rewriteContainers().
The Docker code would create a volume and set up a first round of containers to initialize the volume before launching the real containers.

[briandealwis]

Why does the order matter?

[nkubala]

to be honest, it really only matters for testing. marshalling/unmarshalling the env map back and forth made the ordering undeterministic, meaning any tests examining the env would flake.

the alternative would be implementing some custom comparison logic in the test - however, it's relatively cheap to maintain here, so unless you see problems with it i'm inclined to leave it

[nkubala]

@briandealwis thanks for the review. I'm onboard with taking this further, but I don't think it's as simple as it looks on the surface.

So I think you could change the containerTransformer.Apply() method to drop the v1.Container and instead return an updated imageConfiguration object. You could then get rid of the Adapter, ExecutableContainer, and ContainerPort objects.

I experimented with this, and I'm not confident that we can use the imageConfiguration object as a drop-in replacement for accepting the modifications done by the transformer. the underlying container representation is used several times when determining whether changes should be incorporated into the imageConfiguration. for example, in the buildpacks transformer the container is referenced to determine whether it has received changes:

skaffold/pkg/skaffold/debug/cnb.go

Lines 145 to 152 in 41375f3

	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.
	if needsCnbLauncher {
	container.Command = []string{cnbLauncher}
	}
	container.Args = rewriter(container.Args)
	}

so I think the way this code is written, there are scenarios where we need the container to be passed in along with the imageConfiguration. we could maintain a reference to the container's fields that we need to compare inside of the imageConfiguration, but that's just a different indirection for the same logic.

Then the Kubernetes code would be the rest of the rewriteContainers().

worth noting that without doing your previously suggested change, this is still the end result right now! so it's quite close to your original suggestion I feel.

I'm open to suggestions on how this can be restructured, but I'm inclined to try and get this merged as is and iterate on it later since this PR is already getting quite large.

[briandealwis]

There are a few small issues