Skip to content
This repository has been archived by the owner on Oct 9, 2023. It is now read-only.

Trim error messages read from errors.pb to avoid spaming Etcd. and Admin #141

Merged
merged 4 commits into from
May 30, 2020

Conversation

EngHabu
Copy link
Contributor

@EngHabu EngHabu commented May 30, 2020

TL;DR

Containers can generate arbitrary large errors.pb files, this change protects propeller from accidentally loading a large errors.pb, saving it to etcd. and sending it to admin.

Type

  • Bug Fix
  • Feature
  • Plugin

Are all requirements met?

  • Code completed
  • Smoke tested
  • Unit tests added
  • Code documentation added
  • Any pending items have an associated Issue

Tracking Issue

flyteorg/flyte#334

@EngHabu EngHabu requested a review from kumare3 as a code owner May 30, 2020 02:15
@EngHabu EngHabu requested a review from wild-endeavor May 30, 2020 02:15
return original
}

return original[0:maxLength/2] + original[len(original)-maxLength/2:]
Copy link
Contributor

Choose a reason for hiding this comment

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

since this is just a string can we do + "\n...\n" +

// Errors can be arbitrary long since they are written by containers/potentially 3rd party plugins. This ensures
// the error message length will never be big enough to cause write failures to Etcd. or spam Admin DB with huge
// objects.
taskErr.Message = trimErrorMessage(taskErr.Message, t.cfg.MaxErrorMessageLength)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should trim the error code too? It is a string too right

@codecov-commenter
Copy link

Codecov Report

Merging #141 into master will increase coverage by 0.04%.
The diff coverage is 100.00%.

@kumare3 kumare3 self-requested a review May 30, 2020 19:47
Copy link
Contributor

@kumare3 kumare3 left a comment

Choose a reason for hiding this comment

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

LGTM

@kumare3 kumare3 merged commit 4416aee into master May 30, 2020
eapolinario pushed a commit to eapolinario/flytepropeller that referenced this pull request Aug 9, 2023
…min (flyteorg#141)


Co-authored-by: Ketan Umare <kumare@lyft.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants