feat(http source): Add custom response header configuration #20811

ChrisCanCompute · 2024-07-08T10:38:21Z

Add a custom_response_headers option to SimpleHttpConfig to add the given headers to all Http responses.

Closes: #20395

Add a "custom_response_headers" option to `SimpleHttpConfig` to add to all Http responses. Closes: vectordotdev#20395

bits-bot · 2024-07-08T10:38:26Z

All committers have signed the CLA.

jszwedko

Thanks for this contribution @ChrisCanCompute !

I think it just needs a few things:

The documentation to be regenerated. You can run make generate-component-docs to do so
A changelog entry added. See https://github.com/vectordotdev/vector/blob/master/changelog.d/README.md for details on how to do that
Could we add a test? I think adding it to src/sources/http_server.rs would be appropriate

ChrisCanCompute · 2024-07-10T13:40:02Z

Hi, I have seen your comment. I've been having a hard time running make generate-component-docs, but I have got a test.
I'm unlikely to get any changes before next week, but hopefully should get the rest of this up soon after that.

Thanks for the quick review!

jszwedko · 2024-07-10T13:48:28Z

Hi, I have seen your comment. I've been having a hard time running make generate-component-docs, but I have got a test. I'm unlikely to get any changes before next week, but hopefully should get the rest of this up soon after that.

Thanks for the quick review!

Ah, I think this is actually failing to compile at the moment, which is why make generate-component-docs is failing 😄

ChrisCanCompute · 2024-07-19T22:08:07Z

The build should be fixed now.
I'll sort out docs and a test soon.

ChrisCanCompute · 2024-07-29T10:08:13Z

The code should all now be good.
I have:

An old machine connected to the internet.
A new machine that's air gapped.

I haven't been able to get either of them to generate the docs.
I can try and do it manually, but it would be much quicker if someone else was able to run the relevant command on my branch. Otherwise I can try and write them by hand?

I'll sort out the commits that are missing author emails (not sure how that happened).

jszwedko · 2024-07-29T22:36:41Z

Thanks @ChrisCanCompute . It compiles now, but I'm getting this error when generating the docs:

For map fields (`HashMap<...>`, etc), a description (in the singular form) must be provided by via `#[configurable(metadata(docs::additional_props_description = "Description of the field."))]`.

The description on the field, derived from the code comments, is shown specifically for `field`, while the description provided via `docs::additional_props_description` is shown for the special `field.*` entry that denotes that the field is actually a map.
[2024-07-29T15:34:13] ERROR Relevant schema: {
  "description": "Custom response headers to be added to the HTTP response",
  "default": {
  },
  "type": "object",
  "additionalProperties": {
    "type": "string"
  },
  "_metadata": {
    "docs::examples": {
      "Access-Control-Allow-Origin": "my-cool-server"
    },
    "docs::human_name": "Custom Response Headers"
  }
}
Error: command: cd "/Users/jesse.szwedko/workspace/vector" && "/Users/jesse.szwedko/workspace/vector/scripts/generate-component-docs.rb" "/tmp/vector-config-schema.json"
  failed with exit code: 1
make: *** [generate-component-docs] Error 1

I think we need to add the field description as suggested there.

ChrisCanCompute · 2024-08-05T10:53:04Z

Got the docs updated as well.
Just need to fix the branch to have an email on the two commits that are missing one.

rtrieu

LGTM

ChrisCanCompute · 2024-08-05T21:08:08Z

I've done a force push to my branch to fix the authors (so that the CLA check passes).
There's no code diff since your approval.

Let me know if there's anything else you need from me on this PR.

jszwedko

Thanks @ChrisCanCompute ! I made one suggestion for the changelog, but otherwise this looks good!

changelog.d/20395_custom_response_headers_from_http_server.feature.md

…ture.md Co-authored-by: Jesse Szwedko <jesse@szwedko.me>

jszwedko · 2024-08-09T14:21:47Z

@ChrisCanCompute Apologies, can you merge in master here to fix CI? I tried to, but don't have write access to this branch.

xiazuojie · 2024-08-22T17:23:37Z

HTTP allows set multiple headers with the same name, e.g. multiple set-cookie headers. So the data structure for custom_response_headers would better be HashMap<String, List<String>>

jszwedko · 2024-08-22T19:02:20Z

HTTP allows set multiple headers with the same name, e.g. multiple set-cookie headers. So the data structure for custom_response_headers would better be HashMap<String, List<String>>

Good point. I'm curious what you think of changing this to take a map of header name to list of header values @ChrisCanCompute ? Ideally it could handle either one value or a list of values since single value for each header is more common but that could be more difficult so I'd be happy to see it take a list of values.

Merge branch 'vectordotdev:master' into master

ChrisCanCompute · 2024-08-28T09:36:27Z

I've updated this to take multiple strings.
I initially looped through each value and called warp::reply::with_header(key, value), but the test showed that the values were being overwritten (which surprised me a bit). So I've gone for .join(", ") instead.

Let me know what you think, and thank you for the feedback so far!

jorgehermo9 · 2024-08-28T11:38:13Z

I think it would be better to use https://docs.rs/http/latest/http/header/struct.HeaderMap.html#method.try_append and get the HeaderMap through https://docs.rs/warp/latest/warp/reply/type.Response.html#method.headers

Insert the first value, and then append the rest

@ChrisCanCompute

ChrisCanCompute · 2024-08-28T13:33:12Z

@jorgehermo9 I've updated the PR to use append and insert. We're on http 0.2.9, so don't have try_append and try_insert and I didn't fancy doing a major version upgrade of the http dependency in this PR.

jorgehermo9 · 2024-08-28T17:04:04Z

I think it would be a possible panic that the user specifies a lot of headers (for any reason) in the config and then the append or `insert’ calls panics instead of giving a proper error.

Maybe should we create an issue stating this possible enhancement so it is tracked?

jszwedko

Thanks for the additional changes @ChrisCanCompute ! I think this is looking better. I left a couple of comments inline.

v0.2 of the http crate has try_append too: https://docs.rs/http/0.2.12/http/header/struct.HeaderMap.html#method.try_append

Just for reference, regarding the upgrade to v1.0.0 of the http crate, we are tracking that upgrade here: #19179. We are currently blocked by some dependencies. Again, v0.2 seems to have try_append though.

jszwedko · 2024-08-29T22:06:18Z

src/sources/http_server.rs

-            ));
+            let header_name: HeaderName = key.parse().unwrap();
+            if let Some((first, rest)) = values.split_first() {
+                header_map.insert(header_name.clone(), first.parse().unwrap());


I think you can just use append rather than needing to split and insert the first one:

Inserts a key-value pair into the map.

If the map did not previously have this key present, then false is returned.

If the map did have this key present, the new value is pushed to the end of the list of values currently associated with the key. The key is not updated, though; this matters for types that can be == without being identical.

https://docs.rs/http/0.2.12/http/header/struct.HeaderMap.html#method.append

From the docs you point, I understand that if you append without inserting first, it would fail

Thats why I suggested to insert the first and append the rest

I don't think it fails. It just returns false if the key didn't already exist and true if it did but either way the value is inserted.

See the implementation: https://docs.rs/http/0.2.12/src/http/header/map.rs.html#1415-1449

Oh, you're right, didn't see the implementation. I think I understood wrong the documentation. I see that Inserts a key-value pair into the map is not fallible and will be done always. Thanks!!!

For sure :) I can see why you would have been confused.

jszwedko · 2024-08-29T22:07:47Z

src/sources/http_server.rs

-                key,
-                values.join(", "),
-            ));
+            let header_name: HeaderName = key.parse().unwrap();


Could we do this parsing and validation earlier? That is: parse the configured headers into a HeaderMap during source start time? I think that would have the advantage of:

Only parsing the headers once

Being able to return an error at start time if a name is invalid, rather than panic'ing at runtime

ChrisCanCompute · 2024-11-11T14:03:08Z

@jszwedko thanks for the feedback.
I've addressed it and this is hopefully good to go now!

jszwedko

Thanks @kates ! I left one more comment, but otherwise this is looking good to me.

src/sources/http_server.rs

ChrisCanCompute · 2024-11-11T14:31:48Z

src/sources/http_server.rs

+            for value in values {
+                header_map
+                    .try_append(key.clone(), value.clone())
+                    .expect("Failed to append header. Too many custom http headers specified in custom_response_headers config.");


Not sure if this is any better than it was before. Open to feedback on it.

I don't think we want to panic while adding headers (but I didn't look into this PR extensively)

jszwedko

Great, thanks @ChrisCanCompute !

pront · 2024-11-14T14:13:13Z

Note: A few CI checks are failing.

feat(http_server): Add custom response header configuration

abe8f47

Add a "custom_response_headers" option to `SimpleHttpConfig` to add to all Http responses. Closes: vectordotdev#20395

ChrisCanCompute requested a review from a team as a code owner July 8, 2024 10:38

github-actions bot added the domain: sources Anything related to the Vector's sources label Jul 8, 2024

Update http_server.rs

87d8679

jszwedko reviewed Jul 8, 2024

View reviewed changes

Fix the build

d3c2c3c

ChrisCanCompute requested review from a team as code owners August 5, 2024 10:51

github-actions bot added the domain: external docs Anything related to Vector's external, public documentation label Aug 5, 2024

rtrieu approved these changes Aug 5, 2024

View reviewed changes

ChrisCanCompute added 4 commits August 5, 2024 22:03

Add changelog

38f210d

Add a test

a76b5fa

Fix test compile

27352cc

Update docs

dc670fd

ChrisCanCompute force-pushed the master branch from 5880029 to dc670fd Compare August 5, 2024 21:04

jszwedko approved these changes Aug 6, 2024

View reviewed changes

changelog.d/20395_custom_response_headers_from_http_server.feature.md Outdated Show resolved Hide resolved

Update changelog.d/20395_custom_response_headers_from_http_server.fea…

8859463

…ture.md Co-authored-by: Jesse Szwedko <jesse@szwedko.me>

jszwedko enabled auto-merge August 6, 2024 16:09

jszwedko disabled auto-merge August 9, 2024 14:20

Merge branch 'vectordotdev:master' into master

1d3445c

ChrisCanCompute added 2 commits August 28, 2024 10:16

Merge pull request #1 from vectordotdev/master

01d7e41

Merge branch 'vectordotdev:master' into master

Allow multiple values for each http header

aa06e83

Use header map append and insert

63c97ed

jszwedko reviewed Aug 29, 2024

View reviewed changes

ChrisCanCompute added 4 commits November 11, 2024 13:57

Parse custom headers earlier

46d23f8

Update http.cue

0da494a

Update http_server.cue

1ea1d69

Merge branch 'master' into master

a4f9137

ChrisCanCompute added 2 commits November 11, 2024 14:25

Update http package (#2)

0e36d4c

use try_append for headers

6e802fc

jszwedko reviewed Nov 11, 2024

View reviewed changes

src/sources/http_server.rs Outdated Show resolved Hide resolved

ChrisCanCompute commented Nov 11, 2024

View reviewed changes

Pass the error up

21c5c4c

jszwedko approved these changes Nov 13, 2024

View reviewed changes

jszwedko enabled auto-merge November 13, 2024 21:04

jszwedko changed the title ~~feat(http_server): Add custom response header configuration~~ feat(http source): Add custom response header configuration Nov 13, 2024

jszwedko changed the title ~~feat(http source): Add custom response header configuration~~ feat(http_server source): Add custom response header configuration Nov 13, 2024

ChrisCanCompute changed the title ~~feat(http_server source): Add custom response header configuration~~ feat(http source): Add custom response header configuration Dec 13, 2024

Formatting

bbce50d

auto-merge was automatically disabled December 13, 2024 10:23
Head branch was pushed to by a user without write access

Merge branch 'master' into master

0eb0663

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(http source): Add custom response header configuration #20811

feat(http source): Add custom response header configuration #20811

ChrisCanCompute commented Jul 8, 2024

bits-bot commented Jul 8, 2024 •

edited

Loading

jszwedko left a comment

ChrisCanCompute commented Jul 10, 2024

jszwedko commented Jul 10, 2024

ChrisCanCompute commented Jul 19, 2024

ChrisCanCompute commented Jul 29, 2024

jszwedko commented Jul 29, 2024

ChrisCanCompute commented Aug 5, 2024

rtrieu left a comment

ChrisCanCompute commented Aug 5, 2024

jszwedko left a comment

jszwedko commented Aug 9, 2024

xiazuojie commented Aug 22, 2024 •

edited

Loading

jszwedko commented Aug 22, 2024

ChrisCanCompute commented Aug 28, 2024 •

edited

Loading

jorgehermo9 commented Aug 28, 2024 •

edited

Loading

ChrisCanCompute commented Aug 28, 2024

jorgehermo9 commented Aug 28, 2024

jszwedko left a comment

jszwedko Aug 29, 2024

jorgehermo9 Aug 29, 2024

jszwedko Aug 29, 2024

jorgehermo9 Aug 30, 2024 •

edited

Loading

jszwedko Aug 30, 2024

jszwedko Aug 29, 2024 •

edited

Loading

ChrisCanCompute commented Nov 11, 2024

jszwedko left a comment

ChrisCanCompute Nov 11, 2024

pront Dec 13, 2024

jszwedko left a comment

pront commented Nov 14, 2024

feat(http source): Add custom response header configuration #20811

Are you sure you want to change the base?

feat(http source): Add custom response header configuration #20811

Conversation

ChrisCanCompute commented Jul 8, 2024

bits-bot commented Jul 8, 2024 • edited Loading

jszwedko left a comment

Choose a reason for hiding this comment

ChrisCanCompute commented Jul 10, 2024

jszwedko commented Jul 10, 2024

ChrisCanCompute commented Jul 19, 2024

ChrisCanCompute commented Jul 29, 2024

jszwedko commented Jul 29, 2024

ChrisCanCompute commented Aug 5, 2024

rtrieu left a comment

Choose a reason for hiding this comment

ChrisCanCompute commented Aug 5, 2024

jszwedko left a comment

Choose a reason for hiding this comment

jszwedko commented Aug 9, 2024

xiazuojie commented Aug 22, 2024 • edited Loading

jszwedko commented Aug 22, 2024

ChrisCanCompute commented Aug 28, 2024 • edited Loading

jorgehermo9 commented Aug 28, 2024 • edited Loading

ChrisCanCompute commented Aug 28, 2024

jorgehermo9 commented Aug 28, 2024

jszwedko left a comment

Choose a reason for hiding this comment

jszwedko Aug 29, 2024

Choose a reason for hiding this comment

jorgehermo9 Aug 29, 2024

Choose a reason for hiding this comment

jszwedko Aug 29, 2024

Choose a reason for hiding this comment

jorgehermo9 Aug 30, 2024 • edited Loading

Choose a reason for hiding this comment

jszwedko Aug 30, 2024

Choose a reason for hiding this comment

jszwedko Aug 29, 2024 • edited Loading

Choose a reason for hiding this comment

ChrisCanCompute commented Nov 11, 2024

jszwedko left a comment

Choose a reason for hiding this comment

ChrisCanCompute Nov 11, 2024

Choose a reason for hiding this comment

pront Dec 13, 2024

Choose a reason for hiding this comment

jszwedko left a comment

Choose a reason for hiding this comment

pront commented Nov 14, 2024

bits-bot commented Jul 8, 2024 •

edited

Loading

xiazuojie commented Aug 22, 2024 •

edited

Loading

ChrisCanCompute commented Aug 28, 2024 •

edited

Loading

jorgehermo9 commented Aug 28, 2024 •

edited

Loading

jorgehermo9 Aug 30, 2024 •

edited

Loading

jszwedko Aug 29, 2024 •

edited

Loading