Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix annotate client-side length check #2725

Merged
merged 1 commit into from Apr 15, 2024
Merged

Conversation

rianmcguire
Copy link
Contributor

@rianmcguire rianmcguire commented Apr 11, 2024

Description

Client-side validation of the annotation body length (< 1MiB) isn't working when the annotation body is read from STDIN - instead it always falls through to the server-side validation.

Before:

$ /bin/bash -e -c "ruby -e \"a = ('A'..'Z').to_a; (2 * 1024 * 1024).times { putc a.sample }\" | buildkite-agent annotate"
2024-04-12 09:23:30 INFO   Reading annotation body from STDIN command=annotate
fatal: failed to annotate build: POST https://agent.buildkite.com/v3/jobs/018ecf79-1b8c-48a7-9252-7a33cd980743/annotations: 400 Bad Request: The annotation body must be less than 1 MB

After:

$ /bin/bash -e -c "ruby -e \"a = ('A'..'Z').to_a; (2 * 1024 * 1024).times { putc a.sample }\" | buildkite-agent annotate"
2024-04-12 09:24:54 INFO   Reading annotation body from STDIN command=annotate
fatal: annotation body size (2097152B) exceeds maximum (1048576B)

Context

Support escalation

Testing

  • Tests have run locally (with go test ./...). Buildkite employees may check this if the pipeline has run automatically.
  • Code is formatted (with go fmt ./...)

When the body was read from STDIN, the length check would always pass because `len(cfg.Body) == 0`.
@rianmcguire
Copy link
Contributor Author

I considered adding a test for this, but it would require some additional indirection around stdin to make it testable. The added complication didn't seem worthwhile for the low risk here. Let me know if you disagree!

@rianmcguire rianmcguire marked this pull request as ready for review April 12, 2024 00:29
@rianmcguire rianmcguire requested a review from a team April 12, 2024 00:29
Copy link
Contributor

@DrJosh9000 DrJosh9000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@rianmcguire rianmcguire merged commit 2046c8e into main Apr 15, 2024
1 check passed
@rianmcguire rianmcguire deleted the annotate-stdin-max-size branch April 15, 2024 01:43
Copy link
Contributor

@mdb mdb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants