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: Calling err.Error() on nil error causes panic #579

Merged
merged 2 commits into from
Sep 10, 2024

Conversation

dangra
Copy link
Contributor

@dangra dangra commented Sep 6, 2024

This PR

fix a panic when /v1/flag/change returns an http status code other than 200 and 304.

Related Issues

Fixes #578 , a bug introduced in #547

Notes

In my case the GOFF endpoint returned 404 but the nil error is more subtle and worth fixing.

the 404 isn't logged by go-feature-flag daemon but I captured it by capturing packets:

IP 127.0.0.1.51966 > 127.0.0.1.1031: Flags [P.], seq 1:142, ack 1, win 512, options [nop,nop,TS val 1661138460 ecr 1661138460], length 141
E.....@.@.................c..W.............
c...c...GET /v1/flag/change HTTP/1.1
Host: localhost:1031
User-Agent: Go-http-client/1.1
Content-Type: application/json
Accept-Encoding: gzip

IP 127.0.0.1.1031 > 127.0.0.1.51966: Flags [P.], seq 1:169, ack 142, win 512, options [nop,nop,TS val 1661138461 ecr 1661138460], length 168
E...&.@.@..c.............W....dk...........
c...c...HTTP/1.1 404 Not Found
Content-Type: application/json; charset=UTF-8
Vary: Origin
Date: Fri, 06 Sep 2024 21:21:13 GMT
Content-Length: 24

{"message":"Not Found"}

How to test

Point the client to a webserver that responds with 404 on /v1/flag/change

I am using go-feature-flag v1.24.0 on my setup.

@dangra dangra requested a review from a team as a code owner September 6, 2024 21:15
@dangra dangra force-pushed the gofeatureflag-panic-fix branch from a2586c1 to d58a959 Compare September 6, 2024 21:16
@dangra dangra changed the title calling Error() on nil error causes panic fix: Calling err.Error() on nil error causes panic Sep 6, 2024
Signed-off-by: Daniel Graña <dangra@gmail.com>
@beeme1mr
Copy link
Member

Thanks for the PR @dangra! I've approved, but I'll defer to @thomaspoignant for a final approval.

@thomaspoignant
Copy link
Member

Good catch @dangra and thanks for the fix.
I will release a new version of the provider including this fix.

PS: thanks @beeme1mr I missed this PR before :(

@thomaspoignant thomaspoignant merged commit 4c85501 into open-feature:main Sep 10, 2024
5 checks passed
@dangra dangra deleted the gofeatureflag-panic-fix branch September 10, 2024 19:28
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 this pull request may close these issues.

Panic on new Go-Feature-Flag provider
3 participants