-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add flag for specifying an extra http header in RPC requests #48
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,4 +5,5 @@ import "time" | |
type Config struct { | ||
URL string | ||
PollInterval time.Duration | ||
HTTPHeaders map[string]string | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,6 +2,8 @@ package config | |
|
||
import ( | ||
"errors" | ||
"fmt" | ||
"strings" | ||
"time" | ||
|
||
"github.com/duneanalytics/blockchain-ingester/models" | ||
|
@@ -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 | ||
} | ||
|
||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, that's better! Thanks! Hadn't realized that was spec, but also nice to be consistent with curl |
||
if len(header) != 2 { | ||
return fmt.Errorf("invalid rpc http header: expected 'key,value', got '%s'", r.ExtraHTTPHeader) | ||
} | ||
} | ||
return nil | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could split on comma then?
RPC_HTTP_HEADERS='Header1: value1, Header2: value2, ...'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.