search

apocas / docker-modem

Docker Remote API network stack driver.
Apache License 2.0
234 stars 112 forks source link

Fix #followProgress not catching certain errors #146

Closed weiyuan95 closed 2 years ago

weiyuan95 commented 2 years ago

MRE repo here: https://github.com/weiyuan95/dockerode-followprogress-mre - just run npm i && npm run repro to see some logged output.

Issue

modem.followProgress seems to not be able to catch certain kinds of errors when building the image, and incorrectly resolves when it should be erroring out.

Looking at the Dockerfile from the MRE:

FROM node:16-alpine3.15

WORKDIR /app

COPY NONEXISTENTFILE ./somewhere

The image build should obviously be failing since NONEXISTSTENTFILE doesn't exist and so can't be copied, yet followProgress successfully resolves. I was tracing the code in buildPayload, and it looks like the docker API returns a status code of 200, even though there is an error like this from the stream. I've copied the output from the repro as an example.

successful response object ->> [
  { stream: 'Step 1/3 : FROM node:16-alpine3.15' },
  { stream: '\n' },
  { stream: ' ---> d550799ce1e9\n' },
  { stream: 'Step 2/3 : WORKDIR /app' },
  { stream: '\n' },
  { stream: ' ---> Using cache\n' },
  { stream: ' ---> 92527246500f\n' },
  { stream: 'Step 3/3 : COPY NONEXISTENTFILE ./somewhere' },
  { stream: '\n' },
  {
    errorDetail: {
      message: 'COPY failed: file not found in build context or excluded by .dockerignore: stat NONEXISTENTFILE: file does not exist'
    },
    error: 'COPY failed: file not found in build context or excluded by .dockerignore: stat NONEXISTENTFILE: file does not exist'
  }
]

followProgress successfully resolved

This 200 is probably why this error isn't caught, since only 400s and 500s are considered to be errors. It isn't caught in followProgress as well, since errors should already have been caught in buildPayload as far as I can tell.

Fix

Since the Docker API seems to return a 200 (ie. a fundamental issue which we can't really change), I've implemented a simple approach to just check if the object parsed from the stream has an error key, and if it does, emit an error event. This ensures that the error is correctly passed to the onFinished callback in followProgress.

Comments

apocas commented 2 years ago

Not sure about the impact of this. Won't we be hitting false positives? (ex: data with error key)

This may be a breaking change, there may be people already doing something similar to this in "user land" and not expecting an error to be thrown.

weiyuan95 commented 2 years ago

Won't we be hitting false positives? (ex: data with error key)

FWIW, I've only seen { stream: ... and { error: ... , errorDetails: ..., with the latter being an error with the docker build, so i'm not sure if there could be cases where there could be data with an error key.

there may be people already doing something similar to this in "user land"

100% agree with this, since users could be waiting for the promise to successfully resolve, then checking the result for errors (That's the workaround we were planning to do, actually 😅 ). I'm not really sure if that's the behaviour that the library wants to have though, since there is an error when building the image.

If the intent of followProgress is to provide a means for users to simply check the progress of the docker build, and the build 'finishes'/resolves with an error log, then I can understand that as well!

I would defer to your judgement on this, if you think that there could be a better approach I'll be willing to take a shot at implementing it! If not please feel free to close the issue if you think that this is expected behaviour.