Fix nil dereference panic after 8563c7a

[NicholasAsimov]

Description

Fixes nil panic regression introduced by 8563c7a, if respErr != nil then body is most likely nil.

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x10 pc=0x73bc4d]
goroutine 1 [running]:
github.com/cloudflare/cloudflare-go.(*API).makeRequestWithAuthTypeAndHeaders(0xc000328000, {0xc33d38, 0xc00027bcc0}, {0xb42aa6, 0xd}, {0xc0004a83c0, 0x17}, {0x0, 0x0}, 0x1, ...)
/go/pkg/mod/github.com/cloudflare/cloudflare-go@v0.48.0/cloudflare.go:254 +0xa2d
github.com/cloudflare/cloudflare-go.(*API).makeRequestWithAuthType(...)
/go/pkg/mod/github.com/cloudflare/cloudflare-go@v0.48.0/cloudflare.go:186
github.com/cloudflare/cloudflare-go.(*API).makeRequestContext(0xb45349, {0xc33d38, 0xc00027bcc0}, {0xb42aa6, 0x54a}, {0xc0004a83c0, 0x0}, {0x0, 0x0})
/go/pkg/mod/github.com/cloudflare/cloudflare-go@v0.48.0/cloudflare.go:172 +0x57
github.com/cloudflare/cloudflare-go.(*API).Accounts(0xb43d08, {0xc33d38, 0xc00027bcc0}, {{0x0, 0x1}, {0xc000333600, 0x54a}})
/go/pkg/mod/github.com/cloudflare/cloudflare-go@v0.48.0/accounts.go:59 +0xe7
main.main()

Has your change been tested?

N/A

Types of changes

What sort of change does your code introduce/modify?

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

• My code follows the code style of this project.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have added tests to cover my changes.
• All new and existing tests passed.
• This change is using publicly documented (api.cloudflare.com or developers.cloudflare.com) and stable APIs.

[github-actions]

changelog detected ✅

[codecov-commenter]

Codecov Report

Merging #1072 (0f4e7f2) into master (e39278e) will increase coverage by 0.09%.
The diff coverage is 73.23%.

@@            Coverage Diff             @@
##           master    #1072      +/-   ##
==========================================
+ Coverage   49.94%   50.04%   +0.09%     
==========================================
  Files         115      116       +1     
  Lines       10991    11047      +56     
==========================================
+ Hits         5490     5528      +38     
- Misses       4338     4350      +12     
- Partials     1163     1169       +6     

Impacted Files	Coverage Δ
pages_project.go	50.81% <ø> (ø)
stream.go	64.91% <ø> (ø)
teams_accounts.go	53.84% <ø> (ø)
api_shield.go	53.84% <53.84%> (ø)
workers.go	69.38% <72.72%> (-0.04%)	⬇️
cloudflare.go	68.42% <94.11%> (+1.21%)	⬆️
email_routing_settings.go	60.00% <100.00%> (ø)
images.go	50.34% <100.00%> (-0.35%)	⬇️
r2_bucket.go	100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[jacobbednarz]

are you able to provide a reproduction case where this would first fail that we can put into a regression test?

[NicholasAsimov]

I'm afraid not, it only happens on production environment which would be a bit complicated. Anyways you can emulate any network error that will return resp [nil], respErr [error]

[jacobbednarz]

the testing i've tried with this using a variety of 4xx and 5xx being returned but none have actually surfaced the panic. in all cases, resp is non-nil (like the check above it) so i'm unsure how this is popping up at the moment.

[jacobbednarz]

there is no harm adding the additional safety here however, i'm curious to find out where this is popping up to handle it specifically instead of as a generic error.

[github-actions]

This functionality has been released in v0.49.0.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!

[amoiseiev]

@jacobbednarz I think the fix is appropriate and it's a good practice to be defensive, especially when you interact with remote APIs.

Having that said, after upgrading to 0.49 I realized it's likely related to context timeouts. Now, in the same environment, I'm getting: "operation aborted during backoff: context deadline exceeded", which may explain why you were getting resp with live API interaction but likely not hitting (or setting) context timeout.

[jacobbednarz]

i don't think anyone was disagreeing the change was a good idea. my reservation was with adding code and hoping it helped instead of having a solid example of it addressing the problem.

the context deadline is a good lead; thanks. have you got an example to share that is demonstrating the issue? if not, i'll have to have a play and see if i can pull something together for it.

[amoiseiev]

I created #1080 with a potential improvement.

Basically, resp is "nil" on context timeout in go http client. The way to reproduce is to impose an artificially short timeout to force the code going through this path (example in the pull request), crashes with 0.48 and generates a bit ambiguous error in 0.49, so exiting earlier will likely cover well for such issue and follow the spirit of less processing /. less bugs approach.