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

fix(meta): remove runtime.HttpBodyMarshaler from gateway stack #1277

Merged
merged 6 commits into from
Jan 18, 2023

Conversation

GeorgeMac
Copy link
Member

@GeorgeMac GeorgeMac commented Jan 18, 2023

Fixes FLI-176

This fixes the case where the /meta prefixed endpoints would return 500 (and panic server-side) when authentication was required and no authentication was provided.
Additionally, this only occurred when the Accept header was explicitly set to application/json.

The accept header caused the HttpBodyMarshaler to be invoked. When authentication was not provided, the resulting body was an error and not the expected HttpBody instance. This marshaller was fallback to an embedded marshaller in this instance. This wasn't being set and was therefore nil, causing a panic at runtime.

Update: After reflecting on the behaviour that this only failed when the Accept header was presented, I looked into whether you need to explicitly set the HttpBodyMarshaler and it turns out you don't.
The Marshal stack handles the presence of HttpBody without it.

In 8d2f433 I drop them altogether and it behaves as it should.

@codecov-commenter
Copy link

codecov-commenter commented Jan 18, 2023

Codecov Report

Merging #1277 (9385f51) into main (5af0757) will decrease coverage by 0.19%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #1277      +/-   ##
==========================================
- Coverage   79.82%   79.63%   -0.19%     
==========================================
  Files          42       42              
  Lines        3217     3217              
==========================================
- Hits         2568     2562       -6     
- Misses        521      525       +4     
- Partials      128      130       +2     
Impacted Files Coverage Δ
internal/storage/oplock/sql/sql.go 90.82% <0.00%> (-5.51%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@GeorgeMac
Copy link
Member Author

The last commit from this message has the failing reproduction test case:

GET http://0.0.0.0:8080/meta/info
 ✘ status 401 (actual: 500)

GET http://0.0.0.0:8080/meta/config
 ✘ status 401 (actual: 500)

@GeorgeMac GeorgeMac marked this pull request as ready for review January 18, 2023 10:57
@GeorgeMac GeorgeMac requested a review from a team as a code owner January 18, 2023 10:57
@GeorgeMac GeorgeMac changed the title fix(meta): add default marshaler to meta endpoints fix(meta): remove runtime.HttpBodyMarshaler from gateway stack Jan 18, 2023
@GeorgeMac GeorgeMac added the automerge Used by Kodiak bot to automerge PRs label Jan 18, 2023
@kodiakhq kodiakhq bot merged commit 1223459 into main Jan 18, 2023
@kodiakhq kodiakhq bot deleted the gm/fix-marshaler-panic branch January 18, 2023 14:15
markphelps added a commit that referenced this pull request Jan 18, 2023
* main:
  fix(meta): remove `runtime.HttpBodyMarshaler` from gateway stack (#1277)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Used by Kodiak bot to automerge PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants