Close leaked response bodies

[andrewstucki]

This ensures that we close all response bodies (or documents when they should be closed, but aren't). Working on some tests/validations, but throwing this up here for some eyes.

[gene-redpanda]

LGTM

[chrisseto]

Kinda seems like moving maybeUnmarshalRespInto into this closure would make everything much easier to think about. (That and an errgroup would be much better suited than a multierrgroup but I'll pick one hill to die on here...)

var succeeded atomic.Bool

// ...

defer myRes.Body.Close()

if !succeeded.CompareAndSwap(false, true) {
   // Someone else beat us.  
  return nil
}

// Unmarshal the successful call.
return maybeUnmarshalRespInto(method, resURL, res, into)

// ... Out of closure

if succeeded.Load() {
  // Success or error un unmarshal
}
return grp.Errs()

[chrisseto]

Whoops, meant to hit approve.

[andrewstucki]

@chrisseto @gene-redpanda @RafalKorepta can y'all re-review with the janky test I added with a global response tracker in our tests to ensure we're actually closing connections? I couldn't get the sendAll use-case tested, but that looks like it's for old Redpanda instances as a fallback, but the added test fails without the change due to a non 2xx response.

[chrisseto]

LGTM, nice testing solution!

[chrisseto]

You could also consider adding an Opt that allows setting the transport (line 190 in admin.go) and wrap this up in a transport wrapper. Then we'd catch any leaks that might be stemming from pester or a misuse there of.

This alone is a huge improvement though!