Don't return error in ContextFromRequest function.

[pstibrany]

Previously httputil.ContextFromRequest function could return error if http.Request.RemoteAddr field didn't have correct format, but since this field explicitly has no defined format, that was little too strict.

We have run into this issue when integrating Prometheus 2.16.0 into Cortex.

[bwplotka]

I think this makes sense, thanks! 👍

[roidelapluie]

Can you please fix DCO?

[pstibrany]

Can you please fix DCO?

Done. Thanks, missed the last commit.

[roidelapluie]

Is there a real-life situation where that can error?

[roidelapluie]

This looks strange to me. I would like @brian-brazil view on this one. I think that that error should not happen in real situation. If that can happen in real life, we might trigger a 1.16.1 release.

From my understanding however, this is more about crafting http requests that don't go over http. In which case at the moment you craft the request, you could manually set a RemoteAddr, e.g. 127.0.0.2:0 or something like that.

[pstibrany]

Is there a real-life situation where that can error?

This field is documented to have undefined format, and only uses “IP:port” when request object is created by HTTP server in net/http package.

In our case, we construct request objects manually (based on requests in the frontend queue), and pass them to prometheus v1.API instance. As we don’t keep RemoteAddr, we need to fake this value now, otherwise API instance complains.

[brian-brazil]

This field is documented to have undefined format, and only uses “IP:port” when request object is created by HTTP server in net/http package.

So that's a no then in Prometheus terms. Having the code simpler and more resilient doesn't hurt though.

[pstibrany]

So that's a no then in Prometheus terms. Having the code simpler and more resilient doesn't hurt though.

I understand that Prometheus doesn't need this change, but given the size, code simplification (why should context creation fail?) and the fact that it helps your Cortex friends, perhaps we can still get it in?

[roidelapluie]

So that's a no then in Prometheus terms. Having the code simpler and more resilient doesn't hurt though.

I understand that Prometheus doesn't need this change, but given the size, code simplification (why should context creation fail?) and the fact that it helps your Cortex friends, perhaps we can still get it in?

Do we want to run a query if we are not able to log the client IP? Is there a security implication?

[pstibrany]

Do we want to run a query if we are not able to log the client IP? Is there a security implication?

I assume this question was not for me. Just a note about client IPs -- what you really get here is possibly the address of last intermediate proxy, and not real client IP, so it's already best-effort only.

[bwplotka]

I understand that Prometheus doesn't need this change, but given the size, code simplification (why should context creation fail?) and the fact that it helps your Cortex friends, perhaps we can still get it in?

I think @brian-brazil meant that we are happy to have this in, even though it does not help Prometheus in current moment (:

I tend to agree about parsing issues. It's best-effort only unfortunately and we seem to use it only for logging currently in Prometheus. But indeed we are changing logic bit here: Maybe someone relies on clientIP? This is unlikely though as you @roidelapluie just added it and hopefully, we did not guaranteed the stability of this field in QueryLogger.

This being said I will merge this if no one objects. (:

Thanks, @pstibrany!

[pstibrany]

But indeed we are changing logic bit here: Maybe someone relies on clientIP?

In Prometheus, RemoteAddr will always be in IP:port form, so there should be no change here.

[brian-brazil]

I think @brian-brazil meant that we are happy to have this in, even though it does not help Prometheus in current moment (:

Yes, that's my meaning. This was actually a niggle when I was reviewing it, but I didn't bring it up.