[Synthtrace] Fix wrong url build in the Kibana client by jennypavlova · Pull Request #217678 · elastic/kibana

jennypavlova · 2025-04-09T13:22:37Z

Summary

The issue was introduced in the PR here: basically, the URL will look like: http:/user:pass@localhost:5620/api/fleet/epm/packages/apm?prerelease=false because Path.join will strip the / which is needed in this case - this URL is also passed to getFetchAgent. This PR will fix this issue.

elasticmachine · 2025-04-09T13:22:41Z

Pinging @elastic/obs-ux-infra_services-team (Team:obs-ux-infra_services)

rmyz

LGTM

dgieselaar · 2025-04-09T13:29:22Z

src/platform/packages/shared/kbn-apm-synthtrace/src/lib/shared/base_kibana_client.ts


  fetch<T>(pathname: string, options: KibanaClientFetchOptions): Promise<T> {
-    const url = Path.join(this.target, pathname);
+    const url = new URL(`${this.target}${pathname}`).toString();


i used Path.join() to prevent double slashes from leaking in - e.g. people sometimes define target with a trailing slash or not. Ideally that would still be the case. I didn't know about Path.join() stripping slashes in the protocol! I'm not sure why this wasn't caught earlier though. Why did this test start failing just now and how is the target configured in the test?

I'm not sure if this is the actual issue, because http:/ strangely enough seem to work with both curl and fetch, node-fetch, and axios:

 ~/dev/kibana  node -e ' fetch("http:/example.com").then(res => console.log(res.status));'  ✔  20.18.2 Node  15:35:01  200

 ~/dev/kibana  node -e ' require("axios").get("http:/example.com").then(res => console.log(res.status));'  127 ✘  20.18.2 Node  15:36:00  200

 ~/dev/kibana  curl -sS -D - http:/example.com -o /dev/null  ✔  20.18.2 Node  15:34:46  HTTP/1.1 200 OK Content-Type: text/html ETag: "84238dfc8092e5d9c0dac8ef93371a07:1736799080.121134" Last-Modified: Mon, 13 Jan 2025 20:11:20 GMT Cache-Control: max-age=1256 Date: Wed, 09 Apr 2025 13:35:01 GMT Content-Length: 1256 Connection: keep-alive

i used Path.join() to prevent double slashes from leaking in - e.g. people sometimes define target with a trailing slash or not.

I see - I saw we put the / in the path but it could be the case, I agree. I will add a logic for that so strip the slash at the beginning of the pathname and set it in the URL 👍

Why did this test start failing just now and how is the target configured in the test?

I don't know why it fails now, I saw that today. Probably because it was not failing to get the prerelease=true and it happened in prerelease=false (locally, the prerelease=true case is passing - I couldn't manage to make it fail in order to fully reproduce this)

I did see synthtrace failling locally but that seemed like the usual longstanding issue we have around cutting release branches (which sucks and we should fix it).

I'm not sure if this is the actual issue, because http:/ strangely enough seem to work with both curl and fetch, node-fetch, and axios:

Not enough to build the URL here -> I got an error 'Invalid Url' when I tried to "force" it to fail

It's a 500, so I would try to find logs, but honestly, I think the last two tests are an intermittent failure that we always have when we cut a release branch, and Kibana moves to a new version, and it tries to fetch a package that has not been updated in the registry yet.

Thanks for the suggestion! Unfortunately, I couldn't find anything useful in the build logs linked in the issue 😞 I saw that this fallback was created to actually fix some of those failures by using an older version - so it could be that they will fail because this version is not available yet... there's not much we can do in this case, is there? I mean, it doesn't matter which PR broke that flow - it could be that something in the fleet logic changed and this fallback is no longer valid. I just wanted to start fixing potential problems I found when investigating and this was a small thing I decided to fix on the way - if it is not helping, then we can think of something else 🤔

Do we expect the target to also contain parts of the path or only the authority part?

I would expect the target to be http://user:pas@localhost:5620 and nothing after the port, but I think what we try to prevent here is having a target set like http://elastic_serverless:changeme@localhost:5620/ - I wouldn't expect an extra path/search there - right @dgieselaar ?

I think some people will add a path part, because of basePath. I don't think this needs to be more complicated than joining the strings with a slash and then removing double slashes. But it's also something that I would myself ask an LLM to confirm

I don't think this needs to be more complicated than joining the strings with a slash and then removing double slashes.

@dgieselaar Thanks for the answer! That's fair, considering the way it's used. Having a path there will work with the current join I added here, so we are good. I don't see a reason to include search parameters as a consumer in the target TBH. I wouldn't complicate it more either by handling the search params logic.

Based on GitHub copilot, the target might include also search params as we receive the target as param here and the consumer could send any URL as kibana URL (if kibanaServerUrlWithAuth and kibanaUrlWithAuth include a path and query parameters they will be in the target as well, however I couldn't find any case with params based on the usage the copilot mentioned and I don't think this is an actual use case).

So is this solution OK in this case or do we want to change it?
cc @weltenwort

assuming we don't care about search params (which I think is the case), then we can use:

return new URL(pathname, target).toString();

e.g.:

$ node -e 'console.log(new URL("/foo/bar?prerelease=true", URL.parse("http://example.com:8043/kbn/?blah")).toString());' http://example.com:8043/foo/bar?prerelease=true

as you can see it drops the search from the base url, but it handles double slashes

The problem was that if you have the base path in the target the pathname will override it. I updated the solution to use a normalization function on top of the initial solution

dgieselaar · 2025-04-09T13:46:08Z

src/platform/packages/shared/kbn-apm-synthtrace/src/lib/shared/base_kibana_client.ts


  fetch<T>(pathname: string, options: KibanaClientFetchOptions): Promise<T> {
-    const url = Path.join(this.target, pathname);
+    const pathnameWithoutLeadingSlash = pathname.startsWith('/') ? pathname.slice(1) : pathname;


you need to replace // in the final URL, because this doesn't work if target has a trailing slash

Fixed, thanks - I was not expecting that in the target but it doesn't hurt to have it 👍

kibanamachine · 2025-04-09T17:16:29Z

Flaky Test Runner Stats

🎉 All tests passed! - kibana-flaky-test-suite-runner#8161

[✅] x-pack/test/api_integration/deployment_agnostic/configs/serverless/oblt.apm.serverless.config.ts: 25/25 tests passed.
[✅] x-pack/test/api_integration/deployment_agnostic/configs/stateful/oblt.apm.stateful.config.ts: 25/25 tests passed.

see run history

kibanamachine · 2025-04-10T13:32:35Z

Flaky Test Runner Stats

🟠 Some tests failed. - kibana-flaky-test-suite-runner#8169

[❌] x-pack/test/api_integration/deployment_agnostic/configs/serverless/oblt.apm.serverless.config.ts: 0/25 tests passed.
[❌] x-pack/test/api_integration/deployment_agnostic/configs/stateful/oblt.apm.stateful.config.ts: 0/25 tests passed.

see run history

kibanamachine · 2025-04-22T21:48:41Z

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

kibanamachine · 2025-04-23T22:49:00Z

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

kibanamachine · 2025-04-24T23:49:04Z