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

Case sensitive comparison of some headers causes issues in HTTP/2 environments #1226

Closed
istyf opened this issue Sep 8, 2022 · 5 comments · Fixed by #1227
Closed

Case sensitive comparison of some headers causes issues in HTTP/2 environments #1226

istyf opened this issue Sep 8, 2022 · 5 comments · Fixed by #1227

Comments

@istyf
Copy link

istyf commented Sep 8, 2022

When moving from Orion-LD 1.0.1 to 1.1.0 we experienced a regression where our solution worked perfectly in a docker composed environment, but failed mysteriously on Kubernetes using an istio service mesh. The problem has been tracked down to the fact that the Accept header is no longer respected, when invoking GET requests for /ngsi-ld/v1/entities or /ngsi-ld/v1/entities/{entityid}, in environments where headers are automatically lowercased (i.e. when using HTTP/2).

The behaviour can easily be reproduced by using curl and passing -H "accept: application/ld+json" to one of the above endpoints. The broker will then respond with Content-Type: application/json instead. A lower case -H "link: ...." will be respected though...

The new behaviour seems to have been introduced in #1001 where the accept header handling was rewritten. A lot of headers are properly compared using strcasecmp, but a few are compared using strcmp which causes the code to fail when Accept is being received as accept.

See for example

else if (strcmp(key, "Accept") == 0)

@istyf
Copy link
Author

istyf commented Sep 8, 2022

PR submitted. If approved, then please please please consider backporting this to a 1.1.1 release as the current 1.1.0 version is unusable (if you want response payloads in anything but application/json that is) in HTTP/2 environments.

@istyf
Copy link
Author

istyf commented Sep 13, 2022

Just to follow up. Would it be possible to push out a 1.1.1 release based on this fix sometime soon?

@kzangeli
Copy link
Collaborator

kzangeli commented Sep 16, 2022

Sure, I can do that.
Before releasing I always remove most of the temporal traces, and run a full regression under valgrind, to make sure there aren't any leaks nor errors (possible SIGSEGVs).
A bit busy this week, and next (business trips), so, it's not going to be immediate.
Meanwhile, just use one of the latest tags in dockerhub (not "latest" - one of the latest, including your fix).

@kzangeli
Copy link
Collaborator

Actually, thinking just a little, there's an easier way to do a v1.1.1 - taking it from the v1.1.0 branch (which is pretty much what you propose ...). Sorry, my head is elsewhere the last couple of days.

Still very busy but, yes, I'll try to do this on Thursday, once back from next week's trip (Kick-Off meeting for a new European project).

@kzangeli
Copy link
Collaborator

Done - v1.1.1 is released

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants