-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Allow custom headers in validator client HTTP requests #15884
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
base: develop
Are you sure you want to change the base?
Conversation
} | ||
// BeaconRESTApiHeaders defines a list of headers to send with all HTTP requests to the beacon node. | ||
BeaconRESTApiHeaders = &cli.StringFlag{ | ||
Name: "beacon-rest-api-headers", |
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.
The name is a little verbose but I wanted to keep it aligned with the --beacon-rest-api-provider
flag
// Skip malformed pairs | ||
continue |
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.
This still allows for some weird malformed data like key===val=ue
but I don't think it's that important to handle all edge cases
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 should add some tests for this function (parseBeaconApiHeaders()
)
func (*nodeConnection) dummy() {} | ||
|
||
func NewNodeConnection(grpcConn *grpc.ClientConn, beaconApiUrl string, beaconApiTimeout time.Duration) NodeConnection { | ||
func NewNodeConnection(grpcConn *grpc.ClientConn, beaconApiUrl string, beaconApiHeaders map[string][]string, beaconApiTimeout time.Duration) NodeConnection { |
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.
maybe for stuff liek api headers and beacon api timeout we can turn those into functional parameters and add them in that way
// Skip malformed pairs | ||
continue |
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 should add some tests for this function (parseBeaconApiHeaders()
)
|
||
headersTransport := api.NewCustomHeadersTransport(http.DefaultTransport, v.conn.GetBeaconApiHeaders()) | ||
restHandler := beaconApi.NewBeaconApiRestHandler( | ||
http.Client{Timeout: v.conn.GetBeaconApiTimeout(), Transport: otelhttp.NewTransport(http.DefaultTransport)}, |
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.
does the DefaultTransport
do nothing? or does it add some default parameters to the request? because maybe we need to extend it instead of replacing it?
restHandler := beaconApi.NewBeaconApiRestHandler( | ||
http.Client{Timeout: v.conn.GetBeaconApiTimeout(), Transport: otelhttp.NewTransport(http.DefaultTransport)}, | ||
http.Client{Timeout: v.conn.GetBeaconApiTimeout(), Transport: otelhttp.NewTransport(headersTransport)}, |
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.
if possible, we should have tests to see if the headers get added after the Start()
headersTransport := api.NewCustomHeadersTransport(http.DefaultTransport, conn.GetBeaconApiHeaders()) | ||
restHandler := beaconApi.NewBeaconApiRestHandler( | ||
http.Client{Timeout: s.beaconApiTimeout, Transport: otelhttp.NewTransport(http.DefaultTransport)}, | ||
http.Client{Timeout: s.beaconApiTimeout, Transport: otelhttp.NewTransport(headersTransport)}, | ||
s.beaconApiEndpoint, |
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.
here too, if possible, we should test if headers are being added after registerBeaconClient()
What type of PR is this?
Feature
What does this PR do? Why is it needed?
Adds a new
--beacon-rest-api-headers
flag that allows to specify custom HTTP headers that will be sent to the beacon node with each validator client request. This feature is supported in grpc through the--grpc-headers
flag.Which issues(s) does this PR fix?
Fixes #14528
Other notes for review
Tested in Kurtosis
Acknowledgements