YAML parsing causes 500 error: nested lists in step's "plugins" field.

jwarner112 commented 4 years ago

Context

When testing that a pipeline was properly configured, I encountered a 500 error during the pipeline upload:

2019-08-21 14:25:07 WARN POST https://agent.buildkite.com/v3/jobs/f4ff24ce-c7f9-4ff5-9305-1e84efe33ac0/pipelines: 500 There was an unknown error uploading these steps. Please email us at support@buildkite.com and include this UUID `42bfc187-4117-4ac2-b5b1-df0961e92433` so we can investigate and get it fixed. (Attempt 2/60 Retrying in 5s)

Investigation

By process of elimination, I've determined the error occurs when the plugins field of a step receives a list of lists, as opposed to the list of maps that it's anticipating.

Consider the following document:

steps:
  - label: "first step"
    command: "echo Hello World"
    agents:
      - "queue=default"
    plugins:
      - plugin_one#v0.0.1:
          config_one: ""
          config_two: ""
      - plugin_two#v0.0.2
          config_one: ""
          config_two: ""

This is valid YAML that I've verified with Python 3.4.3 and PyYAML 5.1.2 by the following bash invocation:

$ python -c "import yaml; from pprint import pprint; pprint(yaml.load(open('test.yml'), Loader=yaml.Loader))"

The YAML above does not produce the error. However, I hate YAML and in the pursuit of cleaning it up a bit, stumbled into the bug. I was refactoring the above to:

a: &plugin_one
  plugin_one#v0.0.1:
    config_one: ""
    config_two: ""

b: &plugin_two
  plugin_two#v0.0.2:
    config_one: ""
    config_two: ""

steps:
  - label: "first step"
    command: "echo Hello World"
    agents:
      - "queue=default"
    plugins:
      - *plugin_one
      - *plugin_two

This particular pipeline will have an arbitrary number of steps, each with one of several plugins. There would be a lot of duplication, but inconsistently so, that would not be easily addressed without the refactor. Most importantly, I won't be maintaining this going forward so I'm trying to make it clean and easy to understand for the maintainers.

Bug

Unfortunately, one weakness of the approach above is that it's easy to accidentally provide a list via these aliases instead of a map, as the plugin expects:

intended YAML

a: &plugin_one
  plugin_one#v0.0.1:
    config_one: ""
    config_two: ""

b: &plugin_two
  plugin_two#v0.0.2:
    config_one: ""
    config_two: ""

typo'd YAML

a: &plugin_one
- plugin_one#v0.0.1:
    config_one: ""
    config_two: ""

b: &plugin_two
- plugin_two#v0.0.2:
    config_one: ""
    config_two: ""

Both of these are valid YAML, but the latter is not what plugins expects to see and causes the 500 error. I verified this by removing the aliases and hardcoding the entry as a list of lists, and reproduced the error.

Proposed Resolution

I suppose whether or not this is/isn't expected behavior can be debated, but I'm hoping that at the minimum, this case can be detected and the error code changed from 500 to 422, as suggested by this StackOverflow answer.

Considering the other error codes:

500 codes imply that the impetus for resolution rests with BuildKite
300, 200, and 100 blocks are wholly inappropriate returns

Considering 422 (Unprocessable Entity) (source):

The 422 (Unprocessable Entity) status code means the server understands the content type of the request entity (hence a 415 (Unsupported Media Type) status code is inappropriate), and the syntax of the request entity is correct (thus a 400 (Bad Request) status code is inappropriate) but was unable to process the contained instructions. For example, this error condition may occur if an XML request body contains well-formed (i.e., syntactically correct), but semantically erroneous, XML instructions.

ticky commented 4 years ago

Hey @jwarner112, thanks for the report! I'm currently deploying a fix for this, so we'll now be correctly handling weird-looking plugin stanzas, and in the event it's not something we can handle, return the 422 error we intend to.

jwarner112 commented 4 years ago

Thanks very much! I appreciate the prompt response and fix. :+1:

buildkite / feedback