Fix case where streaming can become disabled #2332

taer · 2019-09-25T19:53:30Z

Fix for #2331

Signed-off-by: Robert Macaulay <robert.macaulay@gmail.com>

zaharidichev · 2019-09-25T20:44:34Z

linkerd/protocol/http/src/test/scala/io/buoyant/linkerd/protocol/http/HttpConfigTest.scala

+    val config = parse(yaml)
+
+    val streaming = config.routerParams(Stack.Params.empty)[Streaming]
+    assert(streaming.disabled)


I do not think this will be disabled

Yup.. I had it right. Then formatted and changed the conditions to clean it up.. And just got lost in the code. :)

Updated PR

zaharidichev · 2019-09-25T20:46:28Z

linkerd/protocol/http/src/test/scala/io/buoyant/linkerd/protocol/http/HttpConfigTest.scala

+    val config = parse(yaml)
+
+    val streaming = config.routerParams(Stack.Params.empty)[Streaming]
+    assert(streaming.enabled)


Same here, according to your logic, this will not disable streaming as it will hit the (_, Some(streamAfter)) case

stupid brain fart commit. Fixed.

zaharidichev

Very good catch. Now that I think about it, I wonder whether there is even point of having the streamEnabled config. What if we just leave streamAfterContentLengthKB and rely on that to enable streaming for messages that are above this threshold ? What do you think @taer

taer · 2019-09-26T00:01:00Z

I can't imagine anyone would want to disable streaming ever. But from what I can tell, disabling streaming forces a total buffer and a Content-length. The streamAfter I think seems to tell linkerd to convert Content-Length into chunked? I honestly don't 100% understand what that does. But if someone were to want to Content-Length everything, then you'd have to keep that streamEnable option. And you don't want to prevent someone who relied on that from upgrading.

They are dependent on each other.

You could have contentLengthEncodeAll: true -- but that's just the inverse.
You can't really convert chunkToContentLength based on a size, so no parameter makes sense there.
I assume streamAfterContentLengthKB means if CL is > X, then change the entire thing to chunked encoding response.

I think you're stuck w/ a boolean and the now optional steamAfter setting. We could fail startup for the illegal cases

Did I get the streamAfterContentLengthKB concept correct?

Swapped the assertions when I was organizing the file Signed-off-by: Robert Macaulay <robert.macaulay@gmail.com>

taer · 2019-09-26T00:39:04Z

linkerd/protocol/http/src/test/scala/io/buoyant/linkerd/protocol/http/HttpConfigTest.scala

+    val config = parse(yaml)
+
+    val streaming = config.routerParams(Stack.Params.empty)[Streaming]
+    assert(streaming.enabled)


@zaharidichev Any ideas on how to test the payload here?

Unfortunatelly finagle has that private, so I dont see a way of accessing it. Quite a pity indeed.

zaharidichev · 2019-09-26T14:48:51Z

@taer
To get some more additonal context on things. I made an attempt at explaining some of the differences around chunked, streaming, etc right here. Hope this is useful.

I assume streamAfterContentLengthKB means if CL is > X, then change the entire thing to chunked encoding response.

I beleive you are right here.

The real question now is whether we should actually check for the illigal case. Can we really have streaming disabled and streamAfterContentLengthKB. And by the looks of it, the answer is no, because if you look at the finagle params you cna quickly see that when streaming is disabled, we are setting it to disabled and having streamAfterContentLengthKB does not play any role. So I would say, if you go ahead and add a check to fail the config initialization when this logical conflict arises, that will be great.

taer · 2019-10-03T16:15:45Z

Should we just throw an exception with the message in it? Or does linkerd have a more fanciful way to handle this?

zaharidichev · 2019-10-07T12:29:20Z

@taer
I think you can resort to using the config package and do some validation there. You can also throw
a ConfigError. You an take a look at how these are used around the codebase.

zaharidichev · 2019-10-15T08:35:59Z

@taer Are you still keen on moving that forward, or I should just take it from here?

taer · 2019-10-15T18:26:29Z

@zaharidichev Heya. Sorry about the delay. Had "normal" job things to work on. I'll do the simple throwing of the configException real quick and push that up

Signed-off-by: Robert Macaulay <rmacaulay@homeaway.com>

zaharidichev

Nice work, just some minor nits :)

zaharidichev · 2019-10-21T11:09:09Z

linkerd/protocol/http/src/test/scala/io/buoyant/linkerd/protocol/http/HttpConfigTest.scala

+    try {
+      parse(yaml)
+      fail()
+    } catch {
+      case _: InvalidDefinitionException => //Expected
+      case e: Throwable => fail(e)
+    }


I think you can rewrite this in a more compressed manner:

intercept[InvalidDefinitionException] { parse(yaml) }

zaharidichev · 2019-10-21T11:13:05Z

linkerd/protocol/http/src/main/scala/io/buoyant/linkerd/protocol/HttpConfig.scala

+    case (Some(true), None) => hparam.Streaming(5.kilobytes)
+    case (None, None) => hparam.Streaming(5.kilobytes)
+    case (Some(false), None) => hparam.Streaming(false)
+    case (Some(false), Some(_)) => throw ConflictingStreamingOptions(_label.getOrElse("unknown"))  //tried label property, but it NPE's in test


If you are talking about HttpConfigTest, it does not NPE for me if use the def label

Strange. It did once. Now it's happy.

zaharidichev · 2019-10-21T11:16:22Z

linkerd/protocol/http/src/test/scala/io/buoyant/linkerd/protocol/http/HttpConfigTest.scala

+import com.fasterxml.jackson.databind.exc.InvalidDefinitionException
+import com.twitter.conversions.StorageUnitOps._
 import com.twitter.finagle.buoyant.Dst
+import com.twitter.finagle.http.param.{FixedLengthStreamedAfter, Streaming}
 import com.twitter.finagle.http.{Method, Request}
 import com.twitter.finagle.{Dtab, Path, Stack}
 import com.twitter.util.Future
-import io.buoyant.config.Parser
+import io.buoyant.config.{ConflictingStreamingOptions, Parser}
 import io.buoyant.linkerd.{IdentifierInitializer, RouterConfig}
 import io.buoyant.linkerd.protocol.{HttpConfig, HttpIdentifierConfig, HttpInitializer}
 import io.buoyant.router.Http


The com.twitter.conversions.StorageUnitOps._, FixedLengthStreamedAfter and ConflictingStreamingOptions seem to be unused, can we just remove them ?

Signed-off-by: Robert Macaulay <rmacaulay@homeaway.com>

zaharidichev

Thanks a lot for helping with that @taer! Nice work!

zaharidichev · 2019-11-05T13:56:17Z

config/src/main/scala/io/buoyant/config/ConfigError.scala

@@ -17,6 +17,9 @@ case class ConflictingSubtypes(t0: NamedType, t1: NamedType) extends ConfigError
 case class ConflictingLabels(name: String) extends ConfigError {
  def message = s"Multiple routers with the label $name"
 }
+case class ConflictingStreamingOptions(name: String) extends ConfigError {
+  def message = s"Conflicting streaming options set. Can't disable streaming and set streamAfterContentLengthKB in $name"


Any reason this cannot be a simple val?

I just cloned ConflictingLabels error.

taer requested review from adleong and olix0r as code owners September 25, 2019 19:53

taer force-pushed the defaultsWrong branch from a9bdb7c to 75fba11 Compare September 25, 2019 19:56

Fix case where streaming can become disabled

309cfb2

Signed-off-by: Robert Macaulay <robert.macaulay@gmail.com>

taer force-pushed the defaultsWrong branch from 75fba11 to 309cfb2 Compare September 25, 2019 20:04

zaharidichev reviewed Sep 25, 2019

View reviewed changes

Swapped assertions

c7056fd

Swapped the assertions when I was organizing the file Signed-off-by: Robert Macaulay <robert.macaulay@gmail.com>

taer commented Sep 26, 2019

View reviewed changes

Throw error on conflicting config options

136f029

Signed-off-by: Robert Macaulay <rmacaulay@homeaway.com>

taer force-pushed the defaultsWrong branch from 270973d to 136f029 Compare October 15, 2019 19:08

taer requested a review from zaharidichev October 15, 2019 20:32

zaharidichev reviewed Oct 21, 2019

View reviewed changes

PR feedback update

cc08259

Signed-off-by: Robert Macaulay <rmacaulay@homeaway.com>

taer force-pushed the defaultsWrong branch from 484b1c6 to cc08259 Compare October 23, 2019 21:07

zaharidichev approved these changes Nov 5, 2019

View reviewed changes

zaharidichev reviewed Nov 5, 2019

View reviewed changes

adleong approved these changes Nov 12, 2019

View reviewed changes

adleong merged commit b503741 into linkerd:master Nov 12, 2019

adleong mentioned this pull request Dec 10, 2019

Streaming is disabled if you do not set streamingEnabled in 1.7.0 #2331

Closed

Fix case where streaming can become disabled #2332

Fix case where streaming can become disabled #2332

Uh oh!

Conversation

taer commented Sep 25, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zaharidichev left a comment

Choose a reason for hiding this comment

Uh oh!

taer commented Sep 26, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zaharidichev commented Sep 26, 2019

Uh oh!

taer commented Oct 3, 2019

Uh oh!

zaharidichev commented Oct 7, 2019

Uh oh!

zaharidichev commented Oct 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

taer commented Oct 15, 2019

Uh oh!

zaharidichev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zaharidichev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

zaharidichev commented Oct 15, 2019 •

edited

Loading