5 ms·

> Our service discovery system is resilient to various failure modes within etcd, however, we did not anticipate a client being able to write a corrupt record d

> Our service discovery system is resilient to various failure modes within etcd, however, we did not anticipate a client being able to write a corrupt record due to improper HTTP handling. golang's net/http module — which is in violation of the HTTP 1.1 specification in this specific case — introduced an additional failure case which we did not anticipate. It is expected that a Content-Length header and an incomplete body should be rejected by a server as being incomplete. Unfortunately net/http does not check that the bytes read from the body portion of the request match the Content-Length header. Whichever engineer wrote puts too much weight on the theoretical aspects of HTTP. The reality is that any of Discord's software could have had erroneous code that announced an empty service list. This would have crashed all of Discord regardless -- transient network errors and mismatched HTTP Content-Length headers need not apply. A resilient HTTP server will try to handle whatever the HTTP client sends it, and then the specific endpoint can enforce whatever levels of strictness are needed by the application. It's nice to be able to point a finger at "not my code", but how many popular web server libraries (not standalone web servers like nginx or apache) out there will actually flat out reject the entire request because of a simple Content-Length mismatch, rather than handing that decision off to the endpoint? If an empty string is not a valid value for your service discovery system, you should reject those from the database with a CHECK CONSTRAINT if possible, or with the code that sits in front of the database if CONSTRAINTS don't exist (like I believe is the case for etcd). It's also not a bad idea for the clients to validate what they receive and filter out values they can't handle to enhance robustness. You definitely shouldn't rely on nothing ever attempting to put a bad value into the database. The web server's willingness to accept "strictly" malformed requests when the engineers were unaware of this fact was certainly a contributing factor, but I don't think it was the root cause in this case, as they're heavily trying to suggest. (I think it would be neat to have a "strict" mode that you could turn on for every web server, especially when being used as an internally-facing service, but most companies want to make it as easy as possible for their customers/partners to integrate with their service... and that means accepting requests that aren't always academically perfect.)

jchw 7y ago
I think it would be nice if the HTTP server could at least optionally validate that the length is expected. For example I would really not expect a malformed chunked encoding lacking a terminating zero length chunk to silently succeed. However, I think you are dead on the money. Etcd is not validating the values but Discord’s software seems to assume the etcd values must be valid. That seems like a recipe for future outages if the mentality were kept this way.
- q3k 7y ago
  Yeah, this is reliability engineering 101 - don't ever expect that outside content won't be malformed, even from 'trusted' systems. There was a codepath somewhere that went from an unmarshal error into a crash - and that's something that should not pass any code review.
  - toast0 7y ago
    In Erlang (and Elixir) systems, turning unexpected errors into crashes is what you're supposed to do. Unfortunately, it's pretty easy for that to spiral into node shutdown/restarts, everybody ends up learning that one the hard way. Supervision trees really do need to be considered, but what are you expected to do when the service discovery system is unreachable, broken, or filled with bad data? There's certainly options, giving up isn't necessarily the wrong one.
    - jchw 7y ago
      This may be true, but malformed input that is unchecked is not, imo, an “exceptional” case but nominally expected, and kind of dangerous since it is one of those things that isn’t likely to happen often since it requires another bug or fault to reveal itself. It’s probably an unpopular opinion at this point but I definitely appreciate Go’s approach to error handling here. Error cases are in-your-face by default for the most part, leaving nearly only programming errors and critical faults to panic ideally.
      - toast0 7y ago
Thaxll 7y ago
> You definitely shouldn't rely on nothing ever attempting to put a bad value into the database. It's a deeper problem than that because etcd doesn't know what Dicord considers as a "right" value, etcd stores whatever you send to it, empty values etc ... The question is why a single bad formated JSON object created an entire fleet of healthy nodes to fail. There is some JSON parsing somewhere that went really wrong.
- bcrosby95 7y ago
  I've only used Erlang in hobby projects, but it seems like a consequence of the "let it crash" philosophy being applied in an awkward situation. But I don't have the experience to know if this philosophy is something Erlang/Elixir programmers ignore in a circumstance like this, and if so how they determine when circumstances are such that "let it crash" should be ignored.
  - SolarNet 7y ago
    I feel like this is a misapplication of that philosophy. Though my first exposure to (serious) exception handling was through the common lisp signal/restart system. So I view software has layers and the communication of errors up and down those layers. Rust for example provides this through Result types and either composing or implementing error types. My point being that the parsing of a record of a service discovery system should crash just that parser process. Not the whole system of processes, the manager of those parser processes should note (log) that the parser crashed on a record and then ignore it (probably as part of a broader refresh system I would imagine). That would be the correct way to handle errors up and down because the point of a process is to do a computation and communicate through reliable channels. Crashing can be a reliable way to communicate (it is communication of an unreliable state, but the point of letting it crash is that the resulting crash can be seen as a reliable communication) and the manager of the parsing process should have had a case for it that wasn't just crash.
  - mmartinson 7y ago
    The sort of crash looping that took down their nodes is one of the parts of OTP that I’ve found to be both quite unintuitive and dangerous, for exactly the reasons described. There are controls for max retry attempts within a time window, but no obvious way from what I can tell to require a supervisor to halt rather than propagate a crash loop in a way that is easily auditable. This has caused me bad failures as well, and I’d be curious to hear what others are doing here. It’s not that it’s a hrs problem to solve, but that it’s an easy thing to overlook.
mst 7y ago
While I agree that discord's code needed to be modified to handle bad data in etcd just in general ... this is not really a theoretical aspect, checking the content length so you don't try and deserialise a partial body and send crap to your business logic is http server writing 101. Discord should've been more defensive. The net/http devs should file this under "mistakes to admit to over beer to cheer up newbies who've just done something really dumb." (I have a number of such mistakes under my belt, we all make mistakes this stupid eventually ;)

jchw 7y ago

I think it would be nice if the HTTP server could at least optionally validate that the length is expected. For example I would really not expect a malformed chunked encoding lacking a terminating zero length chunk to silently succeed. However, I think you are dead on the money. Etcd is not validating the values but Discord’s software seems to assume the etcd values must be valid. That seems like a recipe for future outages if the mentality were kept this way.

q3k 7y ago
Yeah, this is reliability engineering 101 - don't ever expect that outside content won't be malformed, even from 'trusted' systems. There was a codepath somewhere that went from an unmarshal error into a crash - and that's something that should not pass any code review.
- toast0 7y ago
  In Erlang (and Elixir) systems, turning unexpected errors into crashes is what you're supposed to do. Unfortunately, it's pretty easy for that to spiral into node shutdown/restarts, everybody ends up learning that one the hard way. Supervision trees really do need to be considered, but what are you expected to do when the service discovery system is unreachable, broken, or filled with bad data? There's certainly options, giving up isn't necessarily the wrong one.
  - jchw 7y ago
    This may be true, but malformed input that is unchecked is not, imo, an “exceptional” case but nominally expected, and kind of dangerous since it is one of those things that isn’t likely to happen often since it requires another bug or fault to reveal itself. It’s probably an unpopular opinion at this point but I definitely appreciate Go’s approach to error handling here. Error cases are in-your-face by default for the most part, leaving nearly only programming errors and critical faults to panic ideally.
    - toast0 7y ago

Thaxll 7y ago

> You definitely shouldn't rely on nothing ever attempting to put a bad value into the database. It's a deeper problem than that because etcd doesn't know what Dicord considers as a "right" value, etcd stores whatever you send to it, empty values etc ... The question is why a single bad formated JSON object created an entire fleet of healthy nodes to fail. There is some JSON parsing somewhere that went really wrong.

bcrosby95 7y ago
I've only used Erlang in hobby projects, but it seems like a consequence of the "let it crash" philosophy being applied in an awkward situation. But I don't have the experience to know if this philosophy is something Erlang/Elixir programmers ignore in a circumstance like this, and if so how they determine when circumstances are such that "let it crash" should be ignored.
- SolarNet 7y ago
  I feel like this is a misapplication of that philosophy. Though my first exposure to (serious) exception handling was through the common lisp signal/restart system. So I view software has layers and the communication of errors up and down those layers. Rust for example provides this through Result types and either composing or implementing error types. My point being that the parsing of a record of a service discovery system should crash just that parser process. Not the whole system of processes, the manager of those parser processes should note (log) that the parser crashed on a record and then ignore it (probably as part of a broader refresh system I would imagine). That would be the correct way to handle errors up and down because the point of a process is to do a computation and communicate through reliable channels. Crashing can be a reliable way to communicate (it is communication of an unreliable state, but the point of letting it crash is that the resulting crash can be seen as a reliable communication) and the manager of the parsing process should have had a case for it that wasn't just crash.
- mmartinson 7y ago
  The sort of crash looping that took down their nodes is one of the parts of OTP that I’ve found to be both quite unintuitive and dangerous, for exactly the reasons described. There are controls for max retry attempts within a time window, but no obvious way from what I can tell to require a supervisor to halt rather than propagate a crash loop in a way that is easily auditable. This has caused me bad failures as well, and I’d be curious to hear what others are doing here. It’s not that it’s a hrs problem to solve, but that it’s an easy thing to overlook.

mst 7y ago

While I agree that discord's code needed to be modified to handle bad data in etcd just in general ... this is not really a theoretical aspect, checking the content length so you don't try and deserialise a partial body and send crap to your business logic is http server writing 101. Discord should've been more defensive. The net/http devs should file this under "mistakes to admit to over beer to cheer up newbies who've just done something really dumb." (I have a number of such mistakes under my belt, we all make mistakes this stupid eventually ;)