Add flag for specifying an extra http header in RPC requests #48
Conversation
|
This stack of pull requests is managed by Graphite. Learn more about stacking. |
vegarsti
force-pushed
the
06-30-add_flag_for_specifying_an_extra_http_header_in_rpc_requests
branch
from
47e8019 to
1c264b2
Compare
vegarsti
force-pushed
the
06-30-add_flag_for_specifying_an_extra_http_header_in_rpc_requests
branch
from
1c264b2 to
00365fb
Compare
vegarsti
deleted the
06-30-add_flag_for_specifying_an_extra_http_header_in_rpc_requests
branch
| } | ||
|
|
||
| func (r RPCClient) HasError() error { | ||
| if r.NodeURL == "" { | ||
| return errors.New("RPC node URL is required") | ||
| } | ||
| if r.ExtraHTTPHeader != "" { | ||
| header := strings.Split(r.ExtraHTTPHeader, ",") |
There was a problem hiding this comment.
Should we use a colon to separate so that configuration reads in the same way as the http spec/format? It's also familiar for users or curl. We also more easily support values that contain commas which are common for things like X-Forwarded-For
There was a problem hiding this comment.
Yeah, that's better! Thanks! Hadn't realized that was spec, but also nice to be consistent with curl
| @@ -21,13 +23,20 @@ func (d DuneClient) HasError() error { | |||
| } | |||
|
|
|||
| type RPCClient struct { | |||
| NodeURL string `long:"rpc-node-url" env:"RPC_NODE_URL" description:"URL for the blockchain node"` | |||
| NodeURL string `long:"rpc-node-url" env:"RPC_NODE_URL" description:"URL for the blockchain node"` | |||
| ExtraHTTPHeader string `long:"rpc-http-header" env:"RPC_HTTP_HEADER" description:"Extra HTTP header to send with RPC requests. On the form 'key,value'"` // nolint:lll | |||
There was a problem hiding this comment.
We're supporting a singular extra http header here which was what was immediately requested.
Whilst we are here, should we also allow for multiple to be set and make this a slice of strings instead? If so we could set an env-delim if we want to support this through environment variables still (meaning selecting that delimiter carefully and checking whether this library allows delimiter escaping - they don't have any unit tests for this case so probably not) or propose people use multiple args --rpc-http-header "Header1: value1" --rpc-http-header "Header2: value2"?
There was a problem hiding this comment.
I actually started implementing with a list, but then realized they only requested one. I don't think we can make the multiple args version work with an environment variable, unfortunately.
But a list would make sense!
There was a problem hiding this comment.
We could split on comma then? RPC_HTTP_HEADERS='Header1: value1, Header2: value2, ...'
There was a problem hiding this comment.
Or should we not split on comma given a comma might be present in the header value, like the x-forwarded-for
There was a problem hiding this comment.
I should have said env-delim is specifically a directive in the library we are using.
Also, where , can appear in header values, something like | cannot in the http spec and might be a better delimiter.
|
Can we document this header in the README? |
ack! |
Following up on discussion from #48, we make a breaking change for the HTTP header config and allow multiple headers, each key value pair being separated by `:` instead of `,`.
Conduit wanted to be able to specify an http header, this would allow them to pin the indexer to a specific node.
This has been released to them (v0.10 on Docker Hub).