Refactor Istio code before adding Mixer precondition checks (#1403) #1650

pcalcado · 2017-09-20T18:25:41Z

This consolidates some of the Istio-specific logic previously duplicated across the H2 and HTTP modules and increases test coverage in preparation for #1606

Reviewers will likely be interested in:

The refactoring of the io.buoyant.k8s.istio package in an attempt to reduce duplication and improve (unit-)testability: https://github.com/linkerd/linkerd/tree/8d8634105dee7744908ad93346943fa1def4bde8/istio/src/main/scala/io/buoyant/k8s/istio and the HTTP and H2 Identifiers
The new tests for the above: 10f416d#diff-9bf8c4461d3ac3933b6a57b134a2f492R1

adleong

Sorry that it took so long for me to get to this review. I hope my comments make sense. I'm happy to chat more about this.

adleong · 2017-09-20T20:19:39Z

istio/src/main/scala/io/buoyant/k8s/istio/ClusterCache.scala

+  def get(domain: String): Future[Option[Cluster]]
+}
+
+class ClusterCacheBackedByApi(client: DiscoveryClient) extends ClusterCache {


This feels a bit verbose. How do you feel about something like ApiClusterCache?

I tend to prefer more descriptive names, but I am happy with the suggestion. The only thing I'd suggest is that we keep the same naming pattern around things, do we usually go with Qualifier + Thing, as in Api ClusterCache or do we go the other way around? Looking around the code, it seems like so far we have relied on packages to namespace different implementations but keep the same name, should I do this here?

third option: ClusterCache with Api (Alex will hate this :D)

:P I actually find the cake pattern super cute, but had way too many problems debugging badly wired software 😤

I don't have a super strong opinion about naming. I have a mild preference for a more concise name. Using package namespace to differentiate would also be fine.

adleong · 2017-09-20T20:24:13Z

istio/src/main/scala/io/buoyant/k8s/istio/IstioConfigurator.scala

+import io.buoyant.k8s.ClientConfig
+import io.buoyant.k8s.istio.mixer.MixerClient
+
+trait IstioConfigurator {


As far as I can tell, nothing there depends on the current object's state. Could these methods just be static method instead? (ie turn this trait into an object)

I tend to shy away from the mixin pattern when possible.

Hm.. that's a good point. I think this had some template pattern logic at first but was gradually removed. Let me explore it, brb

adleong · 2017-09-20T20:30:05Z

istio/src/main/scala/io/buoyant/k8s/istio/IstioDataFlow.scala

+ * are common in both types of objects.
+ *
+ */
+trait IstioDataFlow {


All of the information this trait provides is derived from the DstBoundCtx and not from any underlying request or response object. So it seems a bit misleading to attach this to IstioRequest and IstioResponse objects. It is also confusing that the vals on this trait are computed from the DstBoundCtx and saved at constructor time. This means that the exact position in the stack where this object is constructed will determine this values, which is pretty non-intuitive.

I would try to keep IstioDataFlow separate from the Request/Response objects and try to make it clear that constructing an IstioDataFlow takes a snapshot of the current DstBoundCtx.

You are absolutely right, and I had felt some of this already. Unfortunately this branch didn't pick the changes, I've added them: https://github.com/linkerd/linkerd/pull/1650/files#diff-042aed18959e3a7caabf44f4294034ef

I am still not happy with CurrentIstioPath, but I ran out of ideas here.

adleong · 2017-09-20T20:31:47Z

istio/src/main/scala/io/buoyant/k8s/istio/IstioLoggerBase.scala

-      duration
+      response.responseCode,
+      request.requestedPath,
+      response.targetService,


Values from the IstioDataFlow trait are derived from the local context so response.targetService could just as easily be request.targetService or even new IstioDataFlow {}.targetService

Yup, fixed with the same merge as the one above

adleong · 2017-09-20T20:33:07Z

istio/src/main/scala/io/buoyant/k8s/istio/IstioResponse.scala

+
+import com.twitter.util.Duration
+
+case class IstioResponse(statusCode: Int, duration: Duration) extends IstioDataFlow {


Any particular reason why IstioRequest is parameterized on the underlying request type and contains the underlying request but IstioResponse is not parameterized and does not include the underlying response?

Fixed on the same as above

adleong · 2017-09-20T20:33:44Z

istio/src/main/scala/io/buoyant/k8s/istio/RouteCache.scala

+  val routeRules: Activity[Map[String, RouteRule]]
+}
+
+class RouteCacheBackedByApi(api: ApiserverClient) extends RouteCache {


Name feels a bit verbose.

Waiting for resolution on similar comment

adleong · 2017-09-20T20:34:56Z

istio/src/main/scala/io/buoyant/k8s/istio/mixer/MixerApiRequests.scala

+import com.twitter.util.Duration
+import io.buoyant.k8s.istio._
+import istio.mixer.v1._
+


rm blank line between imports

Fixed. Interestingly enough, this seems to be a default on IntelliJ. Maybe we should upload a common IntelliJ formatting configuration file?

adleong · 2017-09-20T20:35:48Z

istio/src/test/scala/io/buoyant/k8s/istio/ApiserverClientTest.scala

@@ -6,7 +6,7 @@ import com.twitter.finagle.http.{Request, Response}
 import com.twitter.util.Future
 import io.buoyant.k8s.istio.ApiserverClient.RouteRuleConfig
 import io.buoyant.test.{Awaits, Exceptions}
-import istio.proxy.v1.config.{DestinationWeight, RouteRule}
+import _root_.istio.proxy.v1.config.{DestinationWeight, RouteRule}


what is the root prefix?

Intellij being silly with protobuf imports -_- fixed

adleong · 2017-09-20T20:43:43Z

linkerd/protocol/h2/src/main/scala/io/buoyant/linkerd/protocol/h2/istio/IstioIdentifier.scala

+  val istioIdentifier = new InternalTrafficIdentifier[Request](pfx, baseDtab, routeCache, clusterCache, mixerClient, new H2IstioRequestHandler)
+
+  override def apply(originalRequest: Request): Future[RequestIdentification[Request]] = {
+    val req = H2IstioRequest(originalRequest)


At this point the DstBoundCtx context has not been populated so attempting to call any of the methods from IstioDataFlow on this IstioRequest will fail.

Yup, this was fixed when we moved it to where Logger was. Current Identifier has no DstBoundCtx: https://github.com/linkerd/linkerd/pull/1650/files#diff-64056934f2ceb2827c6a08c5026e0ddeR20

adleong · 2017-09-20T20:48:08Z

istio/src/main/scala/io/buoyant/k8s/istio/ClusterCache.scala

-  import ClusterCache._
+case class Cluster(dest: String, port: String)
+
+trait ClusterCache extends Closable {


I assume this is factored this way for test-ability.

Instantiating a ClusterCacheBackedByApi with a test client that returns mock data would test more of the codepath than implementing a test ClusterCache. But it would also be a lot less ergonomic. I just wanted to bring it up so we can consider the tradeoff.

Yeah, I'm not really a big fan of the trait X/class XBackedByApi pattern that shows up a couple times in this PR. Elsewhere in Linkerd, when we need to mock an external API for testing, I think the general pattern is to inject a mock API with overrides, like this. Like Alex said, this would exercise more of the implementation, and I think it also results in a less complex non-test codebase?

Interesting, the idea behind the use of an interface was to increase ergonomics. My thinking here is that the usual pattern of implementing a fake Service[Req,Resp] on tests have a few drawbacks:

I think that it leaks a lot of "infrastructure" types into higher level tests. These tests shouldn't have any reason to even know about Finagle or any other piece of infrastructure

Maybe more important, I also think that it also creates coupling between the consumer of a type and its implementation. For example, consider https://github.com/linkerd/linkerd/pull/1650/files#diff-bcd0445a689106c21b39b7304f665801R36 . If we follow the usual pattern of implementing a fake Service[Req,Resp] there then the test for InternalTrafficIdentifier would make assumptions about the API used by ClusterCacheBackedByApi. If the next version of the API used by this class changes the URL or any other implementation detail we would have to change not only the code and tests for ClusterCacheBackedByApi but also the tests for InternalTrafficIdentifier. This feels like accidental coupling.

I am not sure if I agree that this would exercise the codebase more, at least in any useful manner. One rule of thumb that I like to follow when it comes to testing is "don't mock what you don't own", and in this case I think that we might be trying to use unit tests for what should be integration tests. Instead, I would suggest that we keep as much as possible of our own logic independent from these dependencies (which drives back to the 1st point) and use integration tests for those instead of mocking APIs that aren't under our control.

The concept of ergonomics or complex code that we are discussing here are subjective, but I have the impression that one of the reasons for what I perceive to be low test coverage around these boundary classes is because it is actually a lot of work to have those fake Service[Req,Resp], so much so that we tend to have a single "fixture" reused across multiple tests, which requires tests to use special requests to know which part of the fixture will be hit.

But with all that said, Linkerd is an existing code base and I am happy to comply with the existing testing strategy if we collectively think that's the way to go.

I have a lot of opinions about testing that we can discuss at great length sometime in the future. But I think this is fine.

hawkw

I agree with most of @adleong's comments, plus some minor style nits that you're free to ignore.

hawkw · 2017-09-22T15:16:28Z

istio/src/main/scala/io/buoyant/k8s/istio/ClusterCache.scala

-  import ClusterCache._
+case class Cluster(dest: String, port: String)
+
+trait ClusterCache extends Closable {


Yeah, I'm not really a big fan of the trait X/class XBackedByApi pattern that shows up a couple times in this PR. Elsewhere in Linkerd, when we need to mock an external API for testing, I think the general pattern is to inject a mock API with overrides, like this. Like Alex said, this would exercise more of the implementation, and I think it also results in a less complex non-test codebase?

hawkw · 2017-09-22T15:17:50Z

istio/src/main/scala/io/buoyant/k8s/istio/IstioAttribute.scala

+
+case class SourceDomainIstioAttribute(val value: String) extends IstioAttribute[String]("source.domain", StringAttribute, "The domain suffix part of the source service, excluding the name and the namespace.	svc.cluster.local")
+
+case class SourceUidIstioAttribute(val value: String) extends IstioAttribute[String]("source.uid", StringAttribute, "Platform-specific unique identifier for the client instance of the source service.	kubernetes://redis-master-2353460263-1ecey.my-namespace")


this might be overly nitpicky, but can we insert some line breaks here? I think lines this long are hard to read especially when my editor wraps them weirdly.

Can you let me know what you think of: https://github.com/linkerd/linkerd/pull/1650/files#diff-0a76be1349b81581e237715901125ebcR35 ?

I don't have a particular preference—I usually go with the Editor's default.n Like I mentioned before, it would probably be a good idea if we all shared a single formatter configuration file.

I like that alignment style for parameters, but most of the rest of the Linkerd codebase puts parameter lists on the next line, one tab level in, like

class Foo( a: A, b: B ) extends Bar { ... }

rather than

class Foo(a: A, b: B) extends Bar { ... }

I agree that we definitely ought to have a shared config file for formatting. Do note that, while we don't have one you can plug into IDEA at the moment, running sbt test will also run Scalariform on everything.

hawkw · 2017-09-22T15:20:36Z

istio/src/main/scala/io/buoyant/k8s/istio/IstioResponse.scala

+
+  def responseCode: ResponseCodeIstioAttribute = ResponseCodeIstioAttribute(statusCode)
+
+  def responseDuration: ResponseDurationIstioAttribute = ResponseDurationIstioAttribute(duration)


once again, I'd really like to see some more line breaks in this file. I'd find something like

package io.buoyant.k8s.istio import com.twitter.util.Duration case class IstioResponse[Resp]( statusCode: Int, duration: Duration, resp: Option[Resp] ) extends IstioAttributesHolder { def responseCode: ResponseCodeIstioAttribute = ResponseCodeIstioAttribute(statusCode) def responseDuration: ResponseDurationIstioAttribute = ResponseDurationIstioAttribute(duration) }

much more readable.

Note that this is entirely a matter of personal taste & I won't hold it against you :)

This one is similar to the one above, but different in one important way. As I mentioned there, I don't really have a personal preference, all I would lobby for is for us to share a single formatter configuration—my general philosophy is that I usually don't care about these things as long as there's one way to do it and we all follow, whatever this way might be.

All that said, my editor right now wraps at 120 columns and the Effective Scala guide mentions 100 columns as the standard:

Indent by two spaces. Try to avoid lines greater than 100 columns in length. Use one blank line between method, class, and object definitions.

What rule should we follow, wrap at 100, always wrap...?

hawkw · 2017-09-22T15:24:58Z

istio/src/main/scala/io/buoyant/k8s/istio/mixer/MixerApiRequests.scala

+    val allStringAttributes = attributes.filter(_.attributeType == StringAttribute).map(_.asInstanceOf[IstioAttribute[String]])
+    val allInt64Attributes = attributes.filter(_.attributeType == Int64Attribute).map(_.asInstanceOf[IstioAttribute[Long]])
+    val allStringMapAttributes = attributes.filter(_.attributeType == StringMapAttribute).map(_.asInstanceOf[IstioAttribute[Map[String, String]]])
+    val allDurationAttributes = attributes.filter(_.attributeType == DurationAttribute).map(_.asInstanceOf[IstioAttribute[Duration]])


Is there any way to do this w/o casting?

I looked at this for a bit and couldn't think of anything, due to attributes: Seq[IstioAttribute[_]]. Did I miss something?

Hmm. What about a function on IstioAttribute taking an attribute type and optionally returning that attribute if it is of the given type?

Another potential concern here – which might be a premature optimization – is that the repeated calls to filter mean that we traverse the sequence of attributes 4 times.

As an alternative, consider using mutable collections (gasp!) and traversing the sequence a single time and appending the attribute to a different collection for each attribute type? If this is in the hot path, it might be worth using mutability.

Alternatively, if you really want to avoid mutable collections, could always do something like this

val (stringAttrOpts, int64AttrOpts, stringMapAttrOpts, durationAttrOpts) = attributes.map { case attr: IstioAttribute[String] => (Some(attr), None, None, None) case attr: IstioAttribute[Long] => (None, Some(attr), None, None) case attr: IstioAttribute[Map[String, String]] => (None, None, Some(attr), None) case attr: IstioAttribute[Duration] => (None, None, None, Some(attr)) }.unzip // the resultant collections are now `Seq[Option[IstioAttribute[T]]]`, which you'd then have to `.flatten`...

which is kind of ugly, but is purely functional!

With the change to IstioAttributes I suggest above, this could be something like:

val allStringAttributes = attributes.collect { case attr@StringAttribute(_) => attr }

I've made the changes @adleong mentioned. @hawkw that's a valid concern and interesting solution, but I would argue that we should rather get through with istio 0.1.x features and address performance on our 0.2 work. Istio 0.1.x doesn't focus on performance as much and I find it unlikely that this would be an actual bottleneck as opposed to, for e.g. the lack of caching in this release.

hawkw · 2017-09-22T15:28:35Z

istio/src/main/scala/io/buoyant/k8s/istio/mixer/MixerApiRequests.scala

+    CheckRequest(TODO, Some(attributesUpdate))
+  }
+
+  private def mkDictionary(


This whole function seems really convoluted, is there any way it can be simplified or broken apart?

I think the current version might be a little bit better? https://github.com/linkerd/linkerd/pull/1650/files#diff-00e114bad2c9fdd832ebe5854d4a1a4fR50

hawkw · 2017-09-22T15:29:20Z

istio/src/main/scala/io/buoyant/k8s/istio/mixer/MixerApiRequests.scala

+    ReportRequest(attributeUpdate = Some(updated))
+  }
+
+  def mkCheckRequest(istioREquest: IstioRequest[_]) = {


is there a reason for the capitalization in istioREquest?

Also, TIOLI, but it seems to me that a lot of this function might be better represented as a series of functions on IstioRequest?

Yup, the reason is that I am still not used to the new mac's keyboard and keep fat fingering everything -_-

Fixed it, thanks :)

On the second comment, I think that most of this stuff is only relevant in a Mixer API context, and I would not like to create any coupling between the request object and Mixer. What did you have in mind?

hawkw · 2017-09-26T15:30:44Z

istio/src/main/scala/io/buoyant/k8s/istio/mixer/MixerApiRequests.scala

+    val allStringAttributes = attributes.filter(_.attributeType == StringAttribute).map(_.asInstanceOf[IstioAttribute[String]])
+    val allInt64Attributes = attributes.filter(_.attributeType == Int64Attribute).map(_.asInstanceOf[IstioAttribute[Long]])
+    val allStringMapAttributes = attributes.filter(_.attributeType == StringMapAttribute).map(_.asInstanceOf[IstioAttribute[Map[String, String]]])
+    val allDurationAttributes = attributes.filter(_.attributeType == DurationAttribute).map(_.asInstanceOf[IstioAttribute[Duration]])


Another potential concern here – which might be a premature optimization – is that the repeated calls to filter mean that we traverse the sequence of attributes 4 times.

As an alternative, consider using mutable collections (gasp!) and traversing the sequence a single time and appending the attribute to a different collection for each attribute type? If this is in the hot path, it might be worth using mutability.

hawkw · 2017-09-26T15:37:54Z

istio/src/main/scala/io/buoyant/k8s/istio/mixer/MixerApiRequests.scala

+    val allStringAttributes = attributes.filter(_.attributeType == StringAttribute).map(_.asInstanceOf[IstioAttribute[String]])
+    val allInt64Attributes = attributes.filter(_.attributeType == Int64Attribute).map(_.asInstanceOf[IstioAttribute[Long]])
+    val allStringMapAttributes = attributes.filter(_.attributeType == StringMapAttribute).map(_.asInstanceOf[IstioAttribute[Map[String, String]]])
+    val allDurationAttributes = attributes.filter(_.attributeType == DurationAttribute).map(_.asInstanceOf[IstioAttribute[Duration]])


Alternatively, if you really want to avoid mutable collections, could always do something like this

val (stringAttrOpts, int64AttrOpts, stringMapAttrOpts, durationAttrOpts) = attributes.map { case attr: IstioAttribute[String] => (Some(attr), None, None, None) case attr: IstioAttribute[Long] => (None, Some(attr), None, None) case attr: IstioAttribute[Map[String, String]] => (None, None, Some(attr), None) case attr: IstioAttribute[Duration] => (None, None, None, Some(attr)) }.unzip // the resultant collections are now `Seq[Option[IstioAttribute[T]]]`, which you'd then have to `.flatten`...

which is kind of ugly, but is purely functional!

hawkw · 2017-09-26T15:45:55Z

istio/src/main/scala/io/buoyant/k8s/istio/mixer/MixerClient.scala

+    )
+    log.trace("MixerClient.report: %s", reportRequest)
+    client.report(Stream.value(reportRequest))
+    Future.Done


Should this return a finished Future, or return a future that's satisfied when the report stream finishes?

if this is supposed to be fire-and-forget, maybe the return type of this method should just be Unit?

It's definitely fire-and-forget, and because nobody cares about when it's completed I decided to just return a constant. On the return type, I generally like methods from a single type to be symmetric, i.e. given a set A it always map from A to S[A], but no strong preference.

Hmmm... I don't understand your comment about symmetric return types. What would be the difference between having the return type be Unit instead of Future[Unit]?

I mean that I would expect all functions in a given type/namespace to return values of the same container, or at least the same kind. In this particular case, it means that I would expect the published interface of MixerClient to be either this:

class MixerClient(client: Mixer) { def report(/* ... */): Future[Unit] def checkPreconditions(/* ... */): Future[MixerCheckStatus] }

Or:

class MixerClient(client: Mixer) { def report(/* ... */): Unit def checkPreconditions(/* ... */): MixerCheckStatus }

But not:

class MixerClient(client: Mixer) { def report(/* ... */): Unit def checkPreconditions(/* ... */): Future[MixerCheckStatus] }

But, again, this is just a preference, I'm happy to make it Unit if it makes it more aligned with our internal philosophy.

Ah, I see. I think Unit is more aligned with my personal philosophy anyway. (since this method is fire-and-forget)

adleong · 2017-09-26T19:03:03Z

istio/src/main/scala/io/buoyant/k8s/istio/mixer/MixerClient.scala

+    )
+    log.trace("MixerClient.report: %s", reportRequest)
+    client.report(Stream.value(reportRequest))
+    Future.Done


if this is supposed to be fire-and-forget, maybe the return type of this method should just be Unit?

adleong · 2017-09-26T20:27:06Z

istio/src/main/scala/io/buoyant/k8s/istio/ClusterCache.scala

-  import ClusterCache._
+case class Cluster(dest: String, port: String)
+
+trait ClusterCache extends Closable {


I have a lot of opinions about testing that we can discuss at great length sometime in the future. But I think this is fine.

adleong · 2017-09-26T20:29:38Z

istio/src/main/scala/io/buoyant/k8s/istio/ClusterCache.scala

+  def get(domain: String): Future[Option[Cluster]]
+}
+
+class ClusterCacheBackedByApi(client: DiscoveryClient) extends ClusterCache {


I don't have a super strong opinion about naming. I have a mild preference for a more concise name. Using package namespace to differentiate would also be fine.

adleong · 2017-09-26T22:57:27Z

istio/src/main/scala/io/buoyant/k8s/istio/IstioConfigurator.scala

+import io.buoyant.config.types.Port
+import io.buoyant.k8s.istio.mixer.MixerClient
+
+object IstioServices {


rename file to IstioServices.scala?

adleong · 2017-09-26T22:59:38Z

istio/src/main/scala/io/buoyant/k8s/istio/IstioConfigurator.scala

+import io.buoyant.k8s.istio.mixer.MixerClient
+
+object IstioServices {
+  protected val log = Logger.get(this.getClass.getSimpleName)


I think this is equivalent to Logger() which is more common

adleong · 2017-09-26T23:29:57Z

istio/src/main/scala/io/buoyant/k8s/istio/mixer/MixerApiRequests.scala

+    val allStringAttributes = attributes.filter(_.attributeType == StringAttribute).map(_.asInstanceOf[IstioAttribute[String]])
+    val allInt64Attributes = attributes.filter(_.attributeType == Int64Attribute).map(_.asInstanceOf[IstioAttribute[Long]])
+    val allStringMapAttributes = attributes.filter(_.attributeType == StringMapAttribute).map(_.asInstanceOf[IstioAttribute[Map[String, String]]])
+    val allDurationAttributes = attributes.filter(_.attributeType == DurationAttribute).map(_.asInstanceOf[IstioAttribute[Duration]])


With the change to IstioAttributes I suggest above, this could be something like:

val allStringAttributes = attributes.collect { case attr@StringAttribute(_) => attr }

adleong · 2017-09-26T23:30:47Z

istio/src/main/scala/io/buoyant/k8s/istio/mixer/MixerClient.scala

+  }
+
+  /**
+   * Checks this request agains  ixer' Pre-Condition system, using the [[https://istio.io/docs/reference/api/mixer/mixer-service.html#check Check API]]


typo: ixer'

Fixed, thanks!

adleong · 2017-09-26T23:33:48Z

istio/src/main/scala/io/buoyant/k8s/istio/mixer/MixerClient.scala

+  }
+
+  private[this] def mkStatus(stream: Stream.Releasable[CheckResponse]) = {
+    val result = stream.value.`result`.get


Instead of get and getOrElse, I think we can chain all these options together with flatMap and then convert to a Future with Future.const

we also have to call stream.release()

On the chaining, I am not sure I understand: each option can be present or not, and if not we're using the default value. How would we deal with the default values on a flatMap chain?

Oh, and on the release() comment, do we have to manually release each stream item in our gRPC implementation? The current implementation seems to be a noop and I couldn't find an example of something else consuming gRPC like this, so I'm not super sure what's the usual idiom.

adleong · 2017-09-26T23:34:37Z

istio/src/main/scala/io/buoyant/k8s/istio/mixer/MixerClient.scala

+    Future.value(MixerCheckStatus(codeUsedWhenUnknownStatus))
+  }
+
+  private[this] def mkStatus(stream: Stream.Releasable[CheckResponse]) = {


I think a Stream.Releasable is an item from the stream, not the stream itself.

Fixed, thanks!

adleong · 2017-09-26T23:37:48Z

...d/protocol/http/src/main/scala/io/buoyant/linkerd/protocol/http/istio/HttpIstioRequest.scala

+import io.buoyant.k8s.istio.{CurrentIstioPath, IstioRequest}
+
+object HttpIstioRequest {
+  def apply(req: Request): IstioRequest[Request] =


I think we should pass Option[IstioPath] in here so that callers are explicit about when they are pulling the local context.

This means that code in the identifiers that construct an HttpIstioRequest can explicitly pass None for IstioPath

adleong

Thanks for putting up with all my nit-picking! This is looking great! I'd like to deploy it to GKE and poke at it a bit since it's such a large change. I should have time to do that tomorrow morning.

adleong · 2017-09-28T23:21:20Z

istio/src/main/scala/io/buoyant/k8s/istio/IstioAttribute.scala

+  description: String
+) extends IstioAttribute[Duration](name, description)
+
+case class SourceIpIstioAttribute(val value: String)


no need for val on case class params

adleong · 2017-09-28T23:25:01Z

istio/src/main/scala/io/buoyant/k8s/istio/IstioRequestAuthorizerFilter.scala

+
+abstract class IstioRequestAuthorizerFilter[Req, Resp](mixerClient: MixerClient) extends Filter[Req, Resp, Req, Resp] {
+
+  def report(request: IstioRequest[Req], response: IstioResponse[Resp], duration: Duration) = {


I like explicit return type on public methods

adleong · 2017-09-28T23:25:28Z

istio/src/main/scala/io/buoyant/k8s/istio/IstioServices.scala

+object IstioServices {
+  protected val log = Logger()
+
+  def mkMixerClient(mixerHost: Option[String], mixerPort: Option[Port]) = {


I like explicit return types on public methods

Me too, will fix it. I think Intellij has a setting for this somewhere?

adleong · 2017-09-28T23:27:45Z

istio/src/main/scala/io/buoyant/k8s/istio/mixer/MixerClient.scala

+    )
+    log.trace("MixerClient.report: %s", reportRequest)
+    client.report(Stream.value(reportRequest))
+    Future.Done


Ah, I see. I think Unit is more aligned with my personal philosophy anyway. (since this method is fire-and-forget)

adleong · 2017-09-28T23:33:01Z

linkerd/protocol/h2/src/main/scala/io/buoyant/linkerd/protocol/h2/istio/IstioIdentifier.scala

+  val istioIdentifier = new InternalTrafficIdentifier[Request](pfx, baseDtab, routeCache, clusterCache, mixerClient, new H2IstioRequestHandler)
+
+  override def apply(originalRequest: Request): Future[RequestIdentification[Request]] = {
+    val req = H2IstioRequest(originalRequest, None)


😍 explicit None

this phrase gives me unpleasant FORTRAN flashbacks

pcalcado · 2017-09-29T15:04:43Z

@adleong cool! In that case, I'd recommend we also review/merge and test #1606 , as the only difference between that PR and this one is this commit: b9dd7df

adleong

⭐ 🎚 🍄

This is needed to add pre-condition and eventually quota checks.

pcalcado mentioned this pull request Sep 20, 2017

Support for Istio Mixer's Conditions Check (#1403) #1606

Merged

pcalcado requested review from olix0r, hawkw and adleong September 20, 2017 18:30

pcalcado self-assigned this Sep 20, 2017

pcalcado added istio reviewable labels Sep 20, 2017

adleong reviewed Sep 20, 2017

View reviewed changes

hawkw added this to the 1.3.0 milestone Sep 22, 2017

hawkw reviewed Sep 22, 2017

View reviewed changes

pcalcado force-pushed the phil/1433-istio-refactoring branch 7 times, most recently from d1ec4df to 152166e Compare September 26, 2017 03:43

hawkw reviewed Sep 26, 2017

View reviewed changes

adleong reviewed Sep 26, 2017

View reviewed changes

pcalcado force-pushed the phil/1433-istio-refactoring branch 5 times, most recently from 82fb2d9 to fd1cccc Compare September 28, 2017 17:31

adleong reviewed Sep 28, 2017

View reviewed changes

pcalcado force-pushed the phil/1433-istio-refactoring branch 2 times, most recently from a1ab2ff to de759da Compare September 29, 2017 14:35

adleong approved these changes Oct 5, 2017

View reviewed changes

Extract Mixer API request building to own object (#1403)

cdc0d36

Phil Calcado added 12 commits October 6, 2017 13:19

Make IstioRequest usable from other classes

d6279e8

Make both loggers code look the same

a90e0b9

Make Istio logger work with attributes

3a9ce96

Add google protobuf files needed for precondition checks

376e9b0

Add check precondition to mixer client

2ae9f9e

Extract common Istio config code

7ab6fcb

Make MixClient dependency for IstioIdentifiers

5487548

Add comment about making behaviour on network failure configurable

a059fd1

De-duplicate Istio Identification logic

c792335

This is needed to add pre-condition and eventually quota checks.

Add tests for istio routing of Ingress and Internal traffic

00466ba

Change Logger and its Initializers to RequestAuthorizer

32e14a3

Add test for CurrentIstioPath

c7ee24d

pcalcado force-pushed the phil/1433-istio-refactoring branch from cda3f1b to c7ee24d Compare October 6, 2017 18:06

pcalcado merged commit 62f853c into master Oct 12, 2017


		import com.twitter.util.Duration

		case class IstioResponse(statusCode: Int, duration: Duration) extends IstioDataFlow {


		case class SourceDomainIstioAttribute(val value: String) extends IstioAttribute[String]("source.domain", StringAttribute, "The domain suffix part of the source service, excluding the name and the namespace. svc.cluster.local")

		case class SourceUidIstioAttribute(val value: String) extends IstioAttribute[String]("source.uid", StringAttribute, "Platform-specific unique identifier for the client instance of the source service. kubernetes://redis-master-2353460263-1ecey.my-namespace")


		def responseCode: ResponseCodeIstioAttribute = ResponseCodeIstioAttribute(statusCode)

		def responseDuration: ResponseDurationIstioAttribute = ResponseDurationIstioAttribute(duration)


		abstract class IstioRequestAuthorizerFilter[Req, Resp](mixerClient: MixerClient) extends Filter[Req, Resp, Req, Resp] {

		def report(request: IstioRequest[Req], response: IstioResponse[Resp], duration: Duration) = {

Refactor Istio code before adding Mixer precondition checks (#1403) #1650

Refactor Istio code before adding Mixer precondition checks (#1403) #1650

Conversation

pcalcado commented Sep 20, 2017 • edited Loading

adleong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hawkw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pcalcado Sep 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pcalcado commented Sep 20, 2017 •

edited

Loading

pcalcado Sep 26, 2017 •

edited

Loading