<@U4H0M349G> There is an issue when dealing with h...

# http4k

[Marco Garofalo]

@dave There is an issue when dealing with http tracing in testing, specifically when a client interact with a fake server the results are different whether the fake is running as a function or as an embedded server. When client tracing hits a functional fake, the trace does not present the xUriTemplate attribute.

It seems like a small difference, but has quite problematic consequences if you think about testing for compliance, for instance we had to redact PII in the URL in some upstream interactions, and because we were running our tests against functional fakes, we did not catch the xUriTemplate exposing PII.

We had to switch all the functional fakes to be embedded servers, which obviously makes the whole test suite heavier to run, especially in parallel with lots of (short-living) running servers.

Is this deliberate?

[dave]

It's not "deliberate", but I'm wondering:

1. why there is PII in a path... URLs are not private?
2. why "PII" is relevant in a test scenario?

Can you put a repro together so we can understand better?

[s4nchez]

There is also a subtle difference when chaining functions vs embedded servers.

When chaining functions directly, it makes a difference if the type is RoutingHttpHandler vs HttpHandler

For instance:

import io.ktor.http.HttpHeaders
import org.http4k.core.HttpHandler
import org.http4k.core.Method
import org.http4k.core.Request
import org.http4k.core.Response
import org.http4k.core.Status.Companion.OK
import org.http4k.core.then
import org.http4k.filter.OpenTelemetryTracing
import org.http4k.filter.ServerFilters
import org.http4k.routing.RequestWithContext
import org.http4k.routing.RoutingHttpHandler
import org.http4k.routing.bind
import org.http4k.routing.routes

fun main() {
    val serverAsRouting: RoutingHttpHandler  = routes("/foo/{*}" bind { _: Request -> Response(OK) });
    val serverAsHandler: HttpHandler  = serverAsRouting

    val clientForRouting = ServerFilters.OpenTelemetryTracing(spanCompletionMutator =  { _, r, _ -> println((r as? RequestWithContext)?.xUriTemplate)}).then(serverAsRouting)
    val clientForHandler = ServerFilters.OpenTelemetryTracing(spanCompletionMutator =  { _, r, _ -> println((r as? RequestWithContext)?.xUriTemplate)}).then(serverAsHandler)

    println("--- Routing ---")
    println(clientForRouting(Request(Method.GET, "/foo/bar")).bodyString())
    println("--- Handler ---")
    println(clientForHandler(Request(Method.GET, "/foo/bar")).bodyString())
}

will produce:

--- Routing ---
foo/{*}

--- Handler ---
null

The routing info is guaranteed to be there if you're running the server independently, but it should still be possible to test this particular behaviour in-memory

[Marco Garofalo]

1. Unfortunately is an upstream service we consume, ideally PII should not be in there ...
2. We have some tests that make sure no structured logs and tracing are leaking specific types of PII

[James Richardson]

Sometimes you need a routing handler - like if you use a path lens. Same here i think.

I definitely get that services you have to call, you just have to call them as they are written.. but this is pretty unusual - you have to assume that every proxy , reverse proxy, telemetry agent, etc is logging request paths, so putting PII in there isnt ideal.
You can use value types from values4k, and use their masking rules to make sure that an event will mask itself, then use strong types for all things. This makes these kind of tests pretty trivial.

[James Richardson]

In short - definitely no need to use actual servers..

[Marco Garofalo]

I think we are discussing about several topics here:
1. Yeah nobody should have PII in their URLs, etc, also each company has their own set of PII (in our case is the car VIN, which IMHO should not be considered PII, but anyway)
2. We do have tiny types for all things we care about, no primitive obsession
3. Even though tiny types have a masking mechanism, it doesn’t ensure me that’s how they are used in the codebase, mistakes happen, that's why I introduced some tests to catch regressions/mistakes

[James Richardson]

Agreed, yes a few different considerations. For 3, currently I use values4k types with an Events serialiser that is configured to redact ValueType properly, so this is reliably going to redact things on a type-by-type basis. This seems to work so far.

Agreed that tiny types dont make things perfect, but we pay particular attention to any unwrapping of the value... it should only happen at very edges of the system.

It is easy to replace the Incoming/Outgoing with your own events, and this could definitely be useful here.

Hope thats useful!

[Marco Garofalo]

@James Richardson appreciate your input, we do all of the above already, that being said all of those are implementation details. My original question/point was mostly on the testing side, which should not be coupled with implementation details. Specifically our regression test would not be able to catch a tracing mistake if we were to use functional servers as opposed to embedded ones, because of that small difference, that's all.

[dave]

@Marco Garofalo did the routing fix work for you? A repro case would really help 🙂

[Marco Garofalo]

@dave I'll put up some repo example, yeah

[Marco Garofalo]

@dave here you go: github.com/th3n3rd/http4k-reporting-diff-issue/blob/…/ReportingDiffIssueTests.kt

Slight deviation from what I reported/observed earlier, the problem is not the "tracing" is the "reporting", apologies for the confusion, I should have create the repo in the first place as you asked earlier instead of going from memory.

Anyway, the repo has 2 tests, same setup and assertion, the diff is only on the output of the filter.

The test agains the functional server report an xUriTemplate with the actual templated uri, while the embedded server one has a xUriTemplate that contains the resolved uri.

[James Richardson]

When reporting Incoming or Outgoing, you can say something like:

uritemplate =If request is RoutedMessage and request.uritemplate != null {
 request.uritemplate.toString
} else { <http://request.uri.path.tr|request.uri.path.tr>imstart('/') }

Then when making outgoing requests, you can use a uritemplate too

Val template = Request(GET, "/thing/{thing}")
Val thingpath = Path.of("thing")
client(template).with(thingpath of "secret")

It wont show up in the outgoing or incoming events

[James Richardson]

Sorry, slack mobile doesn't make it easy to type kotlin

[James Richardson]

This patten also makes it easy to aggregate events by incoming or outgoing xuritempalte

[Marco Garofalo]

The first part is already implemented in http4k, if you create an HttpEvent.Outgoing(tx) you will run exactly that logic.

The second one, it's very useful for the implementation side, did not know you can keep the value hidden by in a client request. thanks for sharing.

[James Richardson]

Well, it will show up in the path bit, but not the xuritempplate but..

[s4nchez]

@Marco Garofalo I reviewed the example you put together and can confirm it matches the subtlety I mentioned earlier in this thread.

ChaoticHttpHandler is a simple HttpHandler , so if you replace the FakeUpstream class with the code below you'll get the same result as the embedded server:

fun FakeUpstream(): RoutingHttpHandler = routes(
    "/details/{pii}" bind GET to { Response(Status.OK) }
)

[Marco Garofalo]

Hey @s4nchez thanks for spending the time to help on this.

I understand the issue, although switching to a simple function is not a feasible solution for us, since our fakes tend to have state and methods to control that state.

Mhm, Interestingly enough I just tried to wrap a routing handler around it in few ways, but despite successfully make it into a RoutingHttpHandler, the diff persist, unless like in your example the template is declared in the outermost handler.

[Marco Garofalo]

Basically Server-side routing metadata affects the client-side HttpTransaction.xUriTemplate , I tried adding a ClientFilters.CleanProxy() before forwarding to the functional upstream and it did solve my issue.

The doc for that filter says

This Filter is used to clean the Request and Response when proxying directly to another system. The purpose
of this is to remove any routing metadata that we may have attached to it before sending it onwards.

That would mean that the outermost handler we would inject in our app while testing would look something like:

externalSystems = ClientFilters.CleanProxy().then(reverseProxy(
    "upstream1-host" to upstream1,
    // ...
    "upstreamN-host" to upstreamN
))

Are there any major drawbacks for the above?

[Marco Garofalo]

Here's some comparison, with the request and response classes in each scenario.

Untitled.md

[s4nchez]

Are there any major drawbacks for the above?

Not that I can think of. Using the CleanProxy ensures RequestWithContext is not propagated which is what you need in this case.