Use JSON serialization for balena build secrets

[pipex]

Build secrets make use of a volumes option passed to the build image docker API. This option is only available in balenaEngine. With the changes on apocas/docker-modem#181, this array will be serialized using URL serialization by dockerode, which is not accepted by balenaEngine.

Change-type: patch
See: https://balena.zendesk.com/agent/tickets/4782

[klutchell]

No issues from my side

[thgreasi]

@pipex I think that we need to also update the package.json to use docker-modem@^5.0.6 since if I get this right, this change wouldn't work w/ previous versions, right?

[pipex]

hmm, I'm not sure @thgreasi, because docker-modem will only stringify Object or Array fields, if you give it a string it won't touch it right?

[kb2ma]

@pipex I am able to recreate the error locally when building on a local mode device. I set up build secrets as described in the docs. The commit here fixes the original issue, but I am able to demonstrate another instance of the problem.

Using the same command, I now see the error in the transcript below. It looks like it is originating in removeSecrets() (link). Notice the transcript shows a SecretRemovalError, occurs later in the build process, and has a different stack trace than the error as reported in the CLI test PR (balena-io/balena-cli#2935).

A couple of other notes:

I also can recreate the SecretRemovalError with a balena build command:

balena build -h 192.168.1.127 -p 2375 --debug -f myfleet

Also I wonder about runBuildTask() (link) because it also updates an array-based Docker option. That function is called by performBuilds() via performSingleBuild(). I'm not convinced the code in runBuildTask() is an error though. I have not seen an error generated by it.

SecretRemovalError:

$ balena --debug push -m 192.168.1.127

[debug] new argv=[/home/kbee/opt/balena-cli/balena,/snapshot/balena-cli/bin/run.js,push,-m,192.168.1.127] length=5
[debug] Deprecation check: 3.15714 days since last npm registry query for next major version release date.
[debug] Will not query the registry again until at least 7 days have passed.
[Debug]   Using build source directory: . 
[Debug]   Pushing to local device: 192.168.1.127
[Debug]   Checking we can access device
[Debug]   Sending request to http://192.168.1.127:48484/ping
[Debug]   Checking device supervisor version: 16.12.4
[Info]    Starting build on device 192.168.1.127
[Debug]   Loading project...
[Debug]   Resolving project...
[Debug]   docker-compose.yml file found at "."
[Debug]   Creating project...
[debug] Event tracking error: Timeout awaiting 'response' for 0ms
[Debug]   Tarring all non-ignored files...
[Debug]   Tarring complete in 11 ms
[Debug]   Fetching device information...
[Debug]   Sending request to http://192.168.1.127:48484/v2/local/device-info
[Debug]   Found build tasks:
[Debug]       ncat-web-server: build [./svc1]
[Debug]       ncat2: build [./svc2]
[Debug]   Resolving services with [raspberrypi4-64|aarch64]
[Debug]   Found project types:
[Debug]       ncat-web-server: Dockerfile.template
[Debug]       ncat2: Dockerfile.template
[Debug]   Probing remote daemon for cache images
[Debug]   Using 7 on-device images for cache...
[Debug]   Starting builds...
[Debug]   ncat-web-server: Using platform option for build: linux/arm64/v8
[Debug]   ncat2: Using platform option for build: linux/arm64/v8
HTTP code 500) server error - invalid character '/' looking for beginning of value 

SecretRemovalError: (HTTP code 500) server error - invalid character '/' looking for beginning of value 
    at Object.performBuilds (/snapshot/balena-cli/node_modules/@balena/compose/dist/multibuild/index.js:154:19)
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
    at async performBuilds (/snapshot/balena-cli/build/utils/device/deploy.js:225:25)
    at async awaitInterruptibleTask (/snapshot/balena-cli/build/utils/helpers.js:283:16)
    at async Object.deployToDevice (/snapshot/balena-cli/build/utils/device/deploy.js:127:24)
    at async PushCmd.pushToDevice (/snapshot/balena-cli/build/commands/push/index.js:96:13)
    at async PushCmd.run (/snapshot/balena-cli/build/commands/push/index.js:35:17)
    at async PushCmd._run (/snapshot/balena-cli/node_modules/@oclif/core/lib/command.js:312:22)
    at async Config.runCommand (/snapshot/balena-cli/node_modules/@oclif/core/lib/config/config.js:435:25)

[kb2ma]

See comment above.

[pipex]

Updated the code @kb2ma, let me know if that works

[pipex]

Hmm, I had not seen the reference to runBuildTask, you are right that is another potential source of issues. I think my PR is wrong because the secrets for that function are generated here by calling populateSecrets. I think the JSON serialization needs to happen at a later step

[pipex]

@kb2ma nice catch with runBuildTask I updated the code to stringify volumes later in the build process

[kb2ma]

My tests above in balenaCLI pass now. The CLI build was using docker-modem v5.0.6.

@pipex , @thgreasi -- Regarding the need to use docker-modem ^5.0.6:

With this PR we modify balena-compose to pre-stringify some arrays that no longer are being stringified in docker-modem v5.0.6. So, if these modifications run on docker-modem <v5.0.6, docker-modem will just stringify something that already is a string, which should be a no-op.

Having said that, I agree we should force docker-modem ^5.0.6 just so we don't have to think about use of older versions. Any new builds with docker-modem as a dependency probably will bring in v5.0.6 anyway, right?

Once we have consensus on this question, I think the PR is ready to go.

[kb2ma]

This position for the stringify looks like a good place to catch code paths coming through.

The latest code addresses my concern. Not ready to approve PR yet though.

[pipex]

docker-modem <v5.0.6, docker-modem will just stringify something that already is a string, which should be a no-op.

As I understand it, pre 5.0.6, docker-modem would only JSON stringify arrays (or objects). If given a string, it would pass it through so there should not be any problems with this code.

That being said, I agree that it probably better to require docker-modem 5.0.6 (as peer dependency maybe?) @kb2ma @thgreasi

[pipex]

Added a new commit requiring docker-modem as peer dependency

[kb2ma]

This looks like a whitespace issue throughout the file.

[pipex]

done

[kb2ma]

@thgreasi I just tested balena-io/balena-cli#2935 based on the latest commit here, ebfc9b6. It works fine.

The use of peerDependencies for docker-modem seems OK, but I'm not familiar with it. If it looks fine to you, I support approving this PR.

[kb2ma]

@pipex I have not used a peer dependency in the past. Research shows the common use is for a plugin to specify a peer dependency on the framework it is built on. The idea is to say that a package depends on general version of the framework, like v3.x, to avoid installation with other plugins that require for example v4.x of the framework.

I'm also confused about also specifying a separate devDependency in addition to the peerDependency.

So, let's compare:

If we use a regular dependency like ^5.0.6, we are saying that v5.0.6 to <v5.1.0 are OK.

Can you describe how peerDependency ^5.0.6 plus devDependency 5.0.6 works better? What case does it cover that the regular dependency does not? Also, how does this relate to an app like balena-cli that specifies balena-compose, docker-modem, and dockerode versions?

https://docs.npmjs.com/cli/v11/configuring-npm/package-json#peerdependencies

[pipex]

@kb2ma AFAIU Dev dependencies are for developing the project itself, e.g. compiling and running tests. Peer dependencies tell users of the library what version of the dependency they need to use to work with the library.

I'm not entirely sure the dev dependency is needed in this specific case but it doesn't hurt.

The exact 5.0.6 version required for the devDependency was an oversight

[pipex]

The build is now failing because of the automated docker-progress bump and a bug that was introduced in the latest minor version. This fixes that problem balena-io-modules/docker-progress#76

[thgreasi]

Should we bump dockerode to ^4.0.4, which is the version that bumped docker-modem to ^5.0.6 ?
PS: fwiw the cli atm is on 4.0.5

[thgreasi]

I guess that docker-modem became a devDependency now b/c we only use it in a type import.
I don't think we need it as a peerDepenency though 🤔
Wouldn't bumping dockerrode be enough?

If you just want to precautions and make 100% sure that docker-modem is present, then I'm fine w/ keeping it as a peerDependency.

[thgreasi]

@pipex I would say let's have docker-modem only in the devDependencies (by only using it in the tests)
#85
and then just bump dockerode in the dependencies (to make sure it's using the docker-model version we want).

[kb2ma]

@pipex, we created #85 to limit docker-modem runtime exposure to a transitive runtime dependency via dockerode. So then this PR should update dockerode dependency to ^4.0.5 and remove the peerDependency. The devDependency on docker-modem can remain for the tests.