httpd: added listener index to http request

[travisdowns]

Seastar http server implementation supports multiple listeners. It may be required for the handler logic to know which listener the connection is coming from. Added listener_idx field to httpd::request to allow handler recognize listener.

We have been using this in production for a couple of years. In Redpanda, we need to know which listener a request was made on to apply different authentication methods.

[avikivity]

Index seems wrong, we should pass some interface that one can call methods on.

[travisdowns]

So instead of:

connection(http_server& server, connected_socket&& fd, bool tls, int listener_idx)

something like:

struct listener_opaque {
  virtual ~listener_opaque() = default;
};

using opaque_ptr = std::unique_ptr<listener_opaque>;

connection(http_server& server, connected_socket&& fd, bool tls, opaque_ptr opaque);

and then application creates subclasses of listener_opaque and application gets them from the request and dynamic casts them to the expected type?

[xemul]

How do you map listener index to the data you need? Presumably when you call http_server::listen() you "map" the invocation sequence number (the index itself) to the context it was made from? If so, can the addr be used as the mapping key?

[travisdowns]

How do you map listener index to the data you need? Presumably when you call http_server::listen() you "map" the invocation sequence number (the index itself) to the context it was made from?

N endpoints are listened on and we rely on the indexes being 0..N-1 in that order:

https://github.com/redpanda-data/redpanda/blob/43a18933f071b7f2db8b774b047ff054b0ef2f31/src/v/pandaproxy/server.cc#L297

If so, can the addr be used as the mapping key?

It looks possible, I'll ask.

[travisdowns]

@xemul - BTW, I suppose we are guaranteed that addr are unique keys at least for a single http_server object, since it would not be possible to listen on the same address twice on the same server?

[avikivity]

So instead of:

connection(http_server& server, connected_socket&& fd, bool tls, int listener_idx)

something like:

struct listener_opaque {
  virtual ~listener_opaque() = default;
};

using opaque_ptr = std::unique_ptr<listener_opaque>;

connection(http_server& server, connected_socket&& fd, bool tls, opaque_ptr opaque);

and then application creates subclasses of listener_opaque and application gets them from the request and dynamic casts them to the expected type?

I feel a little bad about having an abstract class that has no virtual methods, it basically says static_cast me because there is no elegant way to reach the virtual method.

The routes thing is completely orthogonal, yes? You want each listener to respond to exactly the same routes, but still each request wants to know what port/IP it listened on.

Anyway, your proposed solution looks okay. I don't think unique_ptr makes sense though, the listener should be owned by the user. And no need for _opaque to remind me forever that I don't like it.

[avikivity]

@xemul - BTW, I suppose we are guaranteed that addr are unique keys at least for a single http_server object, since it would not be possible to listen on the same address twice on the same server?

You could indeed use (addr, port) as a lookup key in a hash table, if it's not performance sensitive. That doesn't require adding anything except some accessors to the connection.

[travisdowns]

I feel a little bad about having an abstract class that has no virtual methods, it basically says static_cast me because there is no elegant way to reach the virtual method.

Yes, that's the price we pay for type-erasure I guess?

The routes thing is completely orthogonal, yes? You want each listener to respond to exactly the same routes, but still each request wants to know what port/IP it listened on.

That's right.

[avikivity]

I feel a little bad about having an abstract class that has no virtual methods, it basically says static_cast me because there is no elegant way to reach the virtual method.

Yes, that's the price we pay for type-erasure I guess?

The routes thing is completely orthogonal, yes? You want each listener to respond to exactly the same routes, but still each request wants to know what port/IP it listened on.

That's right.

Then why not look up the address/port?

I don't mind a type-erased thing (I don't think the unique_ptr is necessary), but maybe the whole thing is unneeded.

[travisdowns]

Then why not look up the address/port?

It's a good question and as above I'm not sure: I wasn't the original user, just trying to upstream things we are carrying in our fork that might make sense upstream.

I'll move this into draft while I I look into it on our end.