feat(lambda-promtail): Add possibility to set additional HTTP headers for promtail-client requests #11883
Conversation
…romtail-client requests
|
|
|
@cstyan would you be able to review this PR? |
| skipTlsVerify bool | ||
| printLogLine bool | ||
| extraHeaders map[string]string | ||
| httpHeaderKeyRegex = regexp.MustCompile("([A-Za-z0-9-])+") |
There was a problem hiding this comment.
I think the set of characters that are valid for HTTP header keys is much more than this, is it not?
There was a problem hiding this comment.
You seem to be right about this from an RFC perspective, since AFAICS there is no real specification for the format of a valid HTTP header field name. Nevertheless some services still impose such restrictions - especially the ones stated by AWS ELB led me to idea to implement this:
- https://docs.aws.amazon.com/elasticloadbalancing/latest/application/application-load-balancers.html#:~:text=default%20is%20defensive.-,routing.http.drop_invalid_header_fields.enabled,-Indicates%20whether%20HTTP
- https://developers.cloudflare.com/rules/transform/request-header-modification/reference/header-format/
- https://pkg.go.dev/golang.org/x/net/http/httpguts#ValidHeaderFieldName
| func TestLambdaPromtail_ExtraHeadersValid(t *testing.T) { | ||
| extraHeaders, err := parseExtraHeaders("X-Custom-Header,This!sATota\\yCu$t0mHe4der,My-Server_WantsThis,What_ever could go here?,Expected4Entry,yLKc+QSB5VF/Gp3VPN7oOxa98yxWMxeHOAo+CW6trow=") | ||
| require.Nil(t, err) | ||
| require.Len(t, extraHeaders, 3) | ||
| require.Equal(t, extraHeaders["X-Custom-Header"], "This!sATota\\yCu$t0mHe4der") | ||
| require.Equal(t, extraHeaders["My-Server_WantsThis"], "What_ever could go here?") | ||
| require.Equal(t, extraHeaders["Expected4Entry"], "yLKc+QSB5VF/Gp3VPN7oOxa98yxWMxeHOAo+CW6trow=") | ||
| } | ||
|
|
||
| func TestLambdaPromtail_ExtraHeadersInvalidHeaderKey(t *testing.T) { | ||
| extraHeaders, err := parseExtraHeaders("Th.s_Shou|d-Fa!l,a") | ||
| require.Nil(t, extraHeaders) | ||
| require.ErrorContains(t, err, "HTTP header key is invalid:") | ||
| extraHeaders, err = parseExtraHeaders("Also Not Valid ,b") | ||
| require.Nil(t, extraHeaders) | ||
| require.ErrorContains(t, err, "HTTP header key is invalid:") | ||
| } | ||
|
|
||
| func TestLambdaPromtail_ExtraHeadersMissingValue(t *testing.T) { | ||
| extraHeaders, err := parseExtraHeaders("A,a,B,b,C,c,D") | ||
| require.Nil(t, extraHeaders) | ||
| require.Errorf(t, err, invalidExtraHeadersError) | ||
| } | ||
|
|
||
| func TestLambdaPromtail_TestParseHeadersNoneProvided(t *testing.T) { | ||
| extraLabels, err := parseExtraHeaders("") | ||
| require.Len(t, extraLabels, 0) | ||
| require.Nil(t, err) | ||
| } |
There was a problem hiding this comment.
lets simplify a bit and make this a table driven test
There was a problem hiding this comment.
I am not really sure how a table driven test simplifies my test, but I am also not too experience with this kind of testing. Maybe you could give me a hint and what you had in mind here?
There was a problem hiding this comment.
rather than declaring 4 new tests, one for each of these success or failure scenarios, we would have one test with an array of input/expected output, see here https://go.dev/wiki/TableDrivenTests
| if len(extraHeaders) > 0 { | ||
| for key, value := range extraHeaders { | ||
| req.Header.Set(key, value) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
I'm am wondering if, to avoid any potential unexpected issues, we should include some validation here. For example, not allowing overwriting of the headers we set earlier in this function, and logging if we overwrite an existing header value (such as someone not noticing that in their configured extra headers they're setting SOME-EXTRA-HEADER=a, ... , SOME-EXTRA-HEADER=b.
There was a problem hiding this comment.
I implemented a simple check to see if a key has already been set and added a warning for the user to check its env vars: 6a8a19d
Not sure about the logging part since I could not really find a clear way the logging is implemented in the code. Maybe you could have a closer look at that.
There was a problem hiding this comment.
@trc-ikeskin thanks! this is exactly what I had in mind. Logging in this context just meant fmt.Println or using the logger which you've done.
I still think we should guard against people overwriting the headers that are set earlier in this same function, such as Authorization or X-Scope-OrgID. We can do that here or in a follow up PR. Up to you.
cstyan
changed the title
|
@trc-ikeskin IMO there's still some minor changes we should make here |
| @@ -25,19 +26,23 @@ const ( | |||
|
|
|||
| maxErrMsgLen = 1024 | |||
|
|
|||
| invalidExtraLabelsError = "invalid value for environment variable EXTRA_LABELS. Expected a comma separated list with an even number of entries. " | |||
| invalidExtraLabelsError = "invalid value for environment variable EXTRA_LABELS. Expected a comma separated list with an even number of entries. " | |||
| invalidEvenExtraHeadersError = "invalid value for environment variable EXTRA_HTTP_HEADERS. Expected a comma separated list with an even number of entries." | |||
There was a problem hiding this comment.
hey @trc-ikeskin either the name here or on line 89 of the tests needs to change, I think invalidExtraHeadersError is enough
|
@trc-ikeskin did you still want to proceed with this PR? |
@cstyan I still want to continue eventually but I cannot find the time right now - sorry. |
👍 no worries |
What this PR does / why we need it:
This PR adds the ability to set additional, custom HTTP headers for requests made by the promtail client in the
lambda-promtailtool. Additional headers can be passed to the AWS Lambda function via the environment variableEXTRA_HTTP_HEADERSin the form<HTTP header 1 key>,<HTTP header 1 value>,<HTTP header 2 key>,<HTTP header 2 value>, ....This functionality is needed when for example requests pass a AWS Web Application Firewall that expects a certain custom header to be set on every request passed to a load balancer. If this load balancer must be traversed to access the Loki gateway push endpoint, then there is currently no easy way to achieve this.
Special notes for your reviewer:
Simple header key validation was implemented using regex, but there is currently no validation for header values.