feat(http_server): Add custom response header configuration

[ChrisCanCompute]

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

Closes: #20395

[bits-bot]

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]

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]

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]

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

[ChrisCanCompute]

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]

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]

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]

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!

[jszwedko]

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

[xiazuojie]

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]

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.

[ChrisCanCompute]

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]

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]

@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]

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]

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

[jorgehermo9]

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

[jszwedko]

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

[jorgehermo9]

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!!!

[jszwedko]

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

[jszwedko]

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