I feel like I'm missing something important while reading this report -- "Internal Server Error" appears to be the response to most of this API fuzzing, which is kind of what I'd expect the daemon to return when fed otherwise garbage inputs? 🙈 😅

Can you give some idea of how we're intended to interpret this result? Is the implication that we should update our swagger to specifically say that "Internal Server Error" means bad arguments (which IMO is a bit implied?) or that we should be updating our parameters/types to be more specific on what they accept so that the fuzzer doesn't generate quite as many invalid values? (Aren't "invalid" inputs a major part of the point of a fuzzer? 😇)

I guess there's some cases that return a 500, but should've returned a 4xx (most likely because we default to 500 if no specific error was assigned); for example, I see this one with a 500 status;

{
  "message": "Your kernel does not support CPU real-time scheduler"
}

So, the error is handled correctly, but not assigned an errdefs.InvalidParam() (hence a 500 status)

I feel like I'm missing something important while reading this report -- "Internal Server Error" appears to be the response to most of this API fuzzing, which is kind of what I'd expect the daemon to return when fed otherwise garbage inputs? 🙈 😅

That's not an unreasonable expectation, and it appears to be true of dockerd, whether or not it's correct ;)

Can you give some idea of how we're intended to interpret this result? Is the implication that we should update our swagger to specifically say that "Internal Server Error" means bad arguments (which IMO is a bit implied?) or that we should be updating our parameters/types to be more specific on what they accept so that the fuzzer doesn't generate quite as many invalid values? (Aren't "invalid" inputs a major part of the point of a fuzzer? 😇)

The hard-line position, taken by mapi by default, is that 500 means the server broke in some way, and that invalid inputs, no matter how bonkers, should result in 4xx. (And anecdotally, as someone with an operational role, I value that distinction so that I can have meaningful alerts triggered by 500s!)

If that position isn't palatable to this project (understandable!), my suggestion would be to disable the generic InternalServerError rule. Without any other work, mapi will still find some types of bug (if any exist!) And either way, if dockerd can be configured to return stack traces in the 500 payload during testing, we can generate significantly more detailed results (including a certain amount of ability to triage 500s you don't care about.)

that 500 means the server broke in some way, and that invalid inputs, no matter how bonkers, should result in 4xx.

Agreed (with some nuances); I like using the chart from https://www.codetinkerer.com/2015/12/04/choosing-an-http-status-code.html, which is quite helpful in case of doubt

As to "500 means the server broke"; perhaps this is an area where HTTP status codes are not always a good fit for APIs. Generally, I'd describe these as "it's not your fault, it's ours" (the request is likely OK), but (unfortunately) we're not always able to tell if a failure is temporary, a configuration issue (could be an issue with the host), or a (temporary?) failure in one of the components that the daemon depends on (e.g., containerd, runc). Such errors remain to be a "best effort", in which cases, we will "handle" the error (so that the daemon as a whole remains functioning), and return the error information we have as error-message (to allow the user to investigate if needed).

my suggestion would be to disable the generic InternalServerError rule

I'm on the fence on this one. I think that many of these 500 errors should be considered a bug. There are some cases where we explicitly return a 500 (using errdefs.System(), or an error that implements the errdefs.ErrSystem interface), others may be errors that are handled, but that were not assigned an error-type. I know a debug log was added for this reason (to help reveal the errors that still needed to be fixed);

While those logs may help somewhat, the work is definitely not completed (and admitted, not on top of the priority list, unless it's causing breakage); that said, your tool may help to make more of these omissions visible.

if dockerd can be configured to return stack traces in the 500 payload during testing, we can generate significantly more detailed results (including a certain amount of ability to triage 500s you don't care about.)

This should be possible, to some extend: we could make the API return the stack if an error occurred. I don't think this should be the default behaviour, but we could add an option (perhaps an environment variable) to enable this.

It won't be possible to return a stack trace for every error though; Golang errors do not contain a trace by default; we do use the https://github.com/pkg/errors package to generate some of our errors (which provides a stack trace), but that project has been "frozen", and many projects started to remove its use in favor of Golang's "native" error wrapping (which, as far as I'm aware, does not contain stack traces).

For fun, I gave this a quick go during my lunch, and pushed it as a PR: #43252

that 500 means the server broke in some way, and that invalid inputs, no matter how bonkers, should result in 4xx.

Agreed (with some nuances); I like using the chart from https://www.codetinkerer.com/2015/12/04/choosing-an-http-status-code.html, which is quite helpful in case of doubt

As to "500 means the server broke"; perhaps this is an area where HTTP status codes are not always a good fit for APIs. Generally, I'd describe these as "it's not your fault, it's ours"

Yep! "it's not your fault, it's ours" is a nicer way of phrasing what I was trying to convey with "the server broke in some way" (and those flowcharts are great, thanks!)

While those logs may help somewhat, the work is definitely not completed (and admitted, not on top of the priority list, unless it's causing breakage); that said, your tool may help to make more of these omissions visible.

Yep, I imagine it would (especially given stack traces; that's exciting!)

If the results are interesting, but fixing them isn't a high priority, it might make more sense to integrate on a schedule, rather that on pull requests. I don't love a PR-based integration until the baseline is clean, i.e. you can have some confidence that issues found were introduced in the current PR.

This is really cool.