Avoid aliasing in image configuration

[briandealwis]

Fixes: #5704

Description
In examining a debug trace, we discovered that the retrieved image configuration for the unchanged image was actually different:

• round 1:

  DEBU[0015] Retrieved local image configuration for gcr.io/gcloud-powershell-testing/nodejs-guestbook-frontend:latest@sha256:fdf561285af541e5db2483694966368287c1baced16f2c6180bcf06cfd328c06: {false false false [] [node app.js] [PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin NODE_VERSION=12.22.1 YARN_VERSION=1.22.5 WRAPPER_VERBOSE=debug] sha256:ce9a93883c04ba2b41e72a472b955dbaa171a00d7defe5c5b98ed280bd9d22cd map[] [] false false false map[] /frontend map[] false false []}

• round 2 (notice the entrypoint now has an --inspect=0.0.0.0:9229)

  DEBU[0131] Retrieved local image configuration for gcr.io/gcloud-powershell-testing/nodejs-guestbook-frontend:latest@sha256:fdf561285af541e5db2483694966368287c1baced16f2c6180bcf06cfd328c06: {false false false [] [node --inspect=0.0.0.0:9229] [PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin NODE_VERSION=12.22.1 YARN_VERSION=1.22.5 WRAPPER_VERBOSE=debug] sha256:ce9a93883c04ba2b41e72a472b955dbaa171a00d7defe5c5b98ed280bd9d22cd map[] [] false false false map[] /frontend map[] false false []}

The problem is due to aliasing of underlying arrays when copying slices. We retrieve the image manifest and configuration for the image from the local docker daemon:

skaffold/pkg/skaffold/debug/debug.go

Line 114 in 01feff8

manifest, err := apiClient.ConfigFile(ctx, artifact.Tag)

and then copy the values, but the slices and maps retain pointers to their underlying arrays:

skaffold/pkg/skaffold/debug/debug.go

Lines 120 to 129 in 01feff8

	config := manifest.Config
	logrus.Debugf("Retrieved local image configuration for %v: %v", artifact.Tag, config)
	return imageConfiguration{
	artifact: artifact.ImageName,
	env: envAsMap(config.Env),
	entrypoint: config.Entrypoint,
	arguments: config.Cmd,
	labels: config.Labels,
	workingDir: config.WorkingDir,
	}, nil

This struct is then changed with new arguments values copied into the array:

skaffold/pkg/skaffold/debug/transform_nodejs.go

Lines 196 to 203 in 01feff8

	// rewriteNodeCommandLine rewrites a node/nodemon command-line to insert a `--inspect=xxx`
	func rewriteNodeCommandLine(commandLine []string, spec inspectSpec) []string {
	// Assumes that commandLine[0] is "node" or "nodemon"
	commandLine = append(commandLine, "")
	copy(commandLine[2:], commandLine[1:]) // shift
	commandLine[1] = spec.String()
	return commandLine
	}

The solution here is to duplicate the arrays and maps taken from the image configuration.

[codecov]

Codecov Report

Merging #5804 (2f27bd8) into master (4106c53) will increase coverage by 0.11%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master    #5804      +/-   ##
==========================================
+ Coverage   70.87%   70.98%   +0.11%     
==========================================
  Files         421      440      +19     
  Lines       16087    16572     +485     
==========================================
+ Hits        11401    11764     +363     
- Misses       3850     3945      +95     
- Partials      836      863      +27     

Impacted Files	Coverage Δ
pkg/skaffold/debug/debug.go	32.83% <0.00%> (-5.77%)	⬇️
pkg/skaffold/kubernetes/colorpicker.go	80.00% <0.00%> (-20.00%)	⬇️
pkg/skaffold/deploy/deploy_mux.go	66.66% <0.00%> (-19.70%)	⬇️
cmd/skaffold/app/cmd/fix.go	74.41% <0.00%> (-7.64%)	⬇️
cmd/skaffold/app/cmd/render.go	40.74% <0.00%> (-5.69%)	⬇️
pkg/skaffold/build/jib/init.go	83.05% <0.00%> (-4.45%)	⬇️
pkg/skaffold/color/formatter.go	96.00% <0.00%> (-4.00%)	⬇️
pkg/skaffold/deploy/deploy_problems.go	80.00% <0.00%> (-2.36%)	⬇️
cmd/skaffold/app/cmd/runner.go	58.49% <0.00%> (-1.51%)	⬇️
pkg/skaffold/schema/profiles.go	88.46% <0.00%> (-1.15%)	⬇️
... and 101 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 4106c53...2f27bd8. Read the comment docs.

[briandealwis]

Verified by hand with skaffold debug --auto-build --auto-sync in examples/hot-reload (after making the hot-reload/python portion be debug-friendly (drop --user from pip install --user; this won't be necessary once #4088 is fixed).

Opened #5806 to make a test to verify this situation.

[tejal29]

If I understand correctly,
In round 2, the entry point should be node app.js and then in the debug workflow round 2, the --inspect 0.0.0.0:XXXX should be added where XXXX is the right port. In this case the port is 9230 because 9229 was taken by backend in round 2?

[briandealwis]

@tejal29: In this case the port is 9230 because 9229 was taken by backend in round 2?

Port-forwarding attempts to preserve previously-made mappings. So if a pod had been allocated port 9229, it will continue to be allocated 9229. This is useful should a port-forward become disconnected for some reason.

In trying this change scenario out with the nodejs-guestbook sample, you can see the frontend is allocated local 9230 and the backend was allocated 9229:

Port forwarding pod/nodejs-guestbook-backend-6f86cc64dd-fbrlh in namespace default, remote port 9229 -> 127.0.0.1:9229
Forwarding container nodejs-guestbook-frontend-7bf8c65ff-rlsmv/frontend to local port 9230.
Port forwarding pod/nodejs-guestbook-frontend-7bf8c65ff-rlsmv in namespace default, remote port 9229 -> 127.0.0.1:9230
[make change to backend]
...
Port forwarding pod/nodejs-guestbook-backend-676cf65b67-6v2gg in namespace default, remote port 9229 -> 127.0.0.1:9229
 - deployment/nodejs-guestbook-backend is ready.
Deployments stabilized in 5.123 seconds
Watching for changes...
Forwarding container nodejs-guestbook-frontend-7bf8c65ff-rlsmv/frontend to local port 9230.
Forwarding container nodejs-guestbook-frontend-7bf8c65ff-rlsmv/frontend to local port 9230.
Port forwarding pod/nodejs-guestbook-frontend-7bf8c65ff-rlsmv in namespace default, remote port 9229 -> 127.0.0.1:9230

[tejal29]

@tejal29: In this case the port is 9230 because 9229 was taken by backend in round 2?

Port-forwarding attempts to preserve previously-made mappings. So if a pod had been allocated port 9229, it will continue to be allocated 9229. This is useful should a port-forward become disconnected for some reason.

In trying this change scenario out with the nodejs-guestbook sample, you can see the frontend is allocated local 9230 and the backend was allocated 9229:

Port forwarding pod/nodejs-guestbook-backend-6f86cc64dd-fbrlh in namespace default, remote port 9229 -> 127.0.0.1:9229
Forwarding container nodejs-guestbook-frontend-7bf8c65ff-rlsmv/frontend to local port 9230.
Port forwarding pod/nodejs-guestbook-frontend-7bf8c65ff-rlsmv in namespace default, remote port 9229 -> 127.0.0.1:9230
[make change to backend]
...
Port forwarding pod/nodejs-guestbook-backend-676cf65b67-6v2gg in namespace default, remote port 9229 -> 127.0.0.1:9229
 - deployment/nodejs-guestbook-backend is ready.
Deployments stabilized in 5.123 seconds
Watching for changes...
Forwarding container nodejs-guestbook-frontend-7bf8c65ff-rlsmv/frontend to local port 9230.
Forwarding container nodejs-guestbook-frontend-7bf8c65ff-rlsmv/frontend to local port 9230.
Port forwarding pod/nodejs-guestbook-frontend-7bf8c65ff-rlsmv in namespace default, remote port 9229 -> 127.0.0.1:9230

Sorry. I still don't understand why this solves the issue. Is my understanding correct that

1. The entry point should have been node app.js but was found to be node --inspect 0.0.0.0:XXXX?

[briandealwis]

That correct: on the second round, the image configuration appeared to already have an --inspect argument, and so the debug transforms didn't make a change.

An alternative solution would be to change the code, like rewriteNodeCommandLine(), to use util.StrSliceInsert() which does make a new copy of the provided slice.

[nkubala]

seems good to me, restarting a few CI jobs to see if we can get around the pesky rate limit