minvws / nl-kat-coordination

Repo nl-kat-coordination for minvws
European Union Public License 1.2
123 stars 55 forks source link

Improve boefje runner error messages on container failure #3548

Closed dekkers closed 1 week ago

dekkers commented 1 week ago

Changes

The code logged an exception with stacktrace when the container returned a non-zero exit status, but this is confusing because nothing actually goes wrong inside the boefje runner when docker returns an error. To make it more confusing the message of exception that is logged might also contain a stack trace when the output of the docker container was a stacktrace. This change will just log an error message if docker returns an error instead of an exception with the strack trace.

Reraising the exception is not necessary anymore because this was already fixed by #3506 last week.

QA notes

For testing you can force the container to return nonzero by changing the boefje code, for example change the run function of kat_dnssec/main.py to:

def run(boefje_meta: dict):
    import sys
    sys.exit(1)

Then run "make build" to build a new dnssec container image. Running dnssec should then result in only a single error message in the boefje runner. The task should also have failed status.


Checklist for code reviewers:

Copy-paste the checklist from the docs/source/templates folder into your comment.


Checklist for QA:

Copy-paste the checklist from the docs/source/templates folder into your comment.

stephanie0x00 commented 1 week ago

Checklist for QA:

What works:

The normalizer and boefje both fail after editing the file as mentioned in the QA notes. Seems to work as expected and doesn't seem to break anything obvious.

What doesn't work:

n/a

Bug or feature?:

n/a