rekor: do not cancel http context

[Luap99]

The context must be valid for the entire lifetime of the request including reading the body, as such it is invalid to return the body after cancelling the context, depending of if all the response data arrived already or not this then might fail.

To fix this simply do not cancel the context here, we could move the cancel() up to the caller function that processes the body. However as that one already closes the body there should be no need to cancel the context to end the request.

Fixes: #2936

[Luap99]

@mtrmac PTAL

[mtrmac]

Thanks for working on the reproducer!

we could move the cancel() up to the caller function that processes the body. However as that one already closes the body there should be no need to cancel the context to end the request.

Mostly… best I can tell, the defer cancel() is basically a protection against forgetting to consume/close things. (Otherwise, net/http.persistConn.readLoop will be stuck forever, with the FD open.)

It’s a tiny bit valuable, but not enough that I’d advocate copy&pasting these two lines into … the two callers.

LGTM. I don’t have an opinion on timing vs. the monorepo move — feel free to merge now, if it makes things easier.

[Luap99]

we could move the cancel() up to the caller function that processes the body. However as that one already closes the body there should be no need to cancel the context to end the request.

Mostly… best I can tell, the defer cancel() is basically a protection against forgetting to consume/close things. (Otherwise, net/http.persistConn.readLoop will be stuck forever, with the FD open.)

It’s a tiny bit valuable, but not enough that I’d advocate copy&pasting these two lines into … the two callers.

right but since the caller already does res.Body.Close() it has the same protection as the caller doing a defer cancel() I think?

LGTM. I don’t have an opinion on timing vs. the monorepo move — feel free to merge now, if it makes things easier.

I mean merging it now mean it will be in and I don't need to have you review it again, but yeah it is a two line patch that is trival to copy paste around. I guess the best benifit is we can do a "clean" backport to the RHEL branch if needed.

[TomSweeneyRedHat]

Cherry picked to the release-5.36 branch with: #2943