Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Watch request fails with InvalidOperationException #163

Closed
kick2nick opened this issue Oct 13, 2024 · 18 comments · May be fixed by ThreeMammals/Ocelot#2174
Closed

Watch request fails with InvalidOperationException #163

kick2nick opened this issue Oct 13, 2024 · 18 comments · May be fixed by ThreeMammals/Ocelot#2174
Assignees
Labels
bug Something isn't working

Comments

@kick2nick
Copy link

There is a log call introduced in 8cda77d that uses PathAndQuery property of created request.

HttpRequest request = requestFactory();
logger.LogDebug("Start streaming {RequestMethod} request for {RequestUri}...", HttpMethod.Get.Method, request.Uri.PathAndQuery);

But HttpRequest created by factory is relative URI, so on attempts to call ObserveLines I'm getting exception in Observable OnError:
Unexpected error while streaming from the Kubernetes API to \"watch v1/Endpoints 'some-service' in namespace dev\".","exceptions":["System.InvalidOperationException: This operation is not supported for a relative URI."," at System.Uri.get_PathAndQuery()"," at KubeClient.ResourceClients.KubeResourceClient.<>c__DisplayClass30_0.<<ObserveLines>b__0>d.MoveNext()

@tintoy
Copy link
Owner

tintoy commented Oct 13, 2024

Good catch! Thanks, I’ll get that fixed tomorrow 🙂

@tintoy tintoy self-assigned this Oct 13, 2024
@tintoy tintoy added the bug Something isn't working label Oct 13, 2024
@kick2nick kick2nick changed the title Watch requests fails with InvalidOperationException Watch request fails with InvalidOperationException Oct 13, 2024
@tintoy
Copy link
Owner

tintoy commented Oct 14, 2024

@kick2nick - if I can get new packages published to our development package feed, would you be able to verify whether they resolve the issue for you?

@kick2nick
Copy link
Author

Yes, I would be able

@tintoy
Copy link
Owner

tintoy commented Oct 14, 2024

Can you try v2.5.11-develop.5?

@kick2nick
Copy link
Author

Got this 😄 :
exceptions":["System.InvalidOperationException: This operation is not supported for a relative URI."," at System.Uri.get_Scheme()"," at System.UriBuilder.SetFieldsFromUri()"," at KubeClient.Utilities.UriHelper.SafeGetPathAndQuery(Uri uri)

@tintoy
Copy link
Owner

tintoy commented Oct 14, 2024

Ok, that’s odd - I thought I’d tested it pretty thoroughly but perhaps the behaviour is different in PowerShell 🤷🏻

I’ll write up some unit tests and see what’s going on (System.Uri‘s handling of relative URIs is surprisingly bad).

@tintoy
Copy link
Owner

tintoy commented Oct 14, 2024

Ugh, it is different in PowerShell (where I did my testing). Needs to be new UriBuilder(uri.OriginalString), not new UriBuilder(uri).

tintoy added a commit that referenced this issue Oct 14, 2024
The BCL’s handling of relative URIs is pretty broken.

#163
@tintoy
Copy link
Owner

tintoy commented Oct 14, 2024

I’ll add some tests once I get to my computer; it seems there are a couple of unexpected quirks in the BCL’s handling of relative URIs 🙂

@tintoy
Copy link
Owner

tintoy commented Oct 14, 2024

Ok, can you try v2.5.11-develop.7? There are a bunch of tests, now, for URI handling, so I'm pretty confident it should work correctly now (I'd integration-test this myself, but I don't have a working k8s cluster at the moment!).

@kick2nick
Copy link
Author

Tested in k8s new version, watch request works as expected.
Log entry:
Start streaming \"GET\" request for \"/api/v1/watch/namespaces/%7BNamespace%7D/endpoints/%7BServiceName%7D\
It has no route params, but probably it's ok for debug log)

@tintoy
Copy link
Owner

tintoy commented Oct 15, 2024

I’ll see what I can do about expanding the route parameters, but it may require an enhancement to the HTTP template library :)

@kick2nick
Copy link
Author

It's your choice, but as far as I understand this bug impacts all watch requests in your library, so this enhancement could be part of separate PR if it need time.

@tintoy
Copy link
Owner

tintoy commented Oct 15, 2024

Definitely agree; will leave that for later, but can at least make the logging conditional on the configured log level. Will publish a release version tomorrow morning.

@kick2nick
Copy link
Author

Cool, thank you for quick fix!

@tintoy
Copy link
Owner

tintoy commented Oct 15, 2024

BTW, I will probably remove that log statement (eventually) as KubeClient’s existing request/response logging will already include that populated URI.

@tintoy
Copy link
Owner

tintoy commented Oct 16, 2024

Publishing v2.5.12...

@raman-m
Copy link

raman-m commented Oct 24, 2024

@tintoy commented on Oct 16

Did you forget to tag the release as the latest one, or is it a pre-release?

@tintoy
Copy link
Owner

tintoy commented Oct 24, 2024

Good catch! Done 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
3 participants