enhancement(http sink): improve config UX

[KowalczykBartek]

Fix for https://github.com/vectordotdev/vector/pull/21583

[KowalczykBartek]

@pront what do you think about this https://github.com/vectordotdev/vector/pull/21857/commits/bc3bc2863f758e3ad51240898c376ab680ad5107 ? this tls makes sense only for oauth2

Problem is that, I had to remove those eq and partial eq (otherwise how to implement it for Option), I am not sure how important it is, the only thing it breaks is

    fn test_parse(input: &str, expected_uri: &'static str, expected_auth: Option<(&str, &str)>) {
        let UriSerde { uri, auth } = input.parse().unwrap();
        assert_eq!(uri, Uri::from_static(expected_uri));
        assert_eq!(
            auth,
            expected_auth.map(|(user, password)| {
                Auth::Basic {
                    user: user.to_owned(),
                    password: password.to_owned().into(),
                }
            })
        );
    }

[pront]

@pront what do you think about this bc3bc28 ? this tls makes sense only for oauth2

Hi @KowalczykBartek, I appreciate you jumping on this so fast!

I am going to push a few commits to this PR and then I would really appreciate if you can run some manual tests again and share the config you used in this PR.

[KowalczykBartek]

@pront you removed

pub tls: Option<TlsConfig>,

now, there is no option to pass certs for mtls scenario - that was the idea behind this ticket, your http rest client can use different tls settings than cert/key used to acquire bearer token :/ this is the minimal working solution https://github.com/vectordotdev/vector/commit/bc3bc2863f758e3ad51240898c376ab680ad5107 (I didn't put this tls there by accident, definitely not a duplicate https://github.com/vectordotdev/vector/pull/21857/commits/f599b37c3a7ce5751eaef84538cdfaac1f1df781#diff-0820c0d03ae75b35f0b5bbd8dbc8e4d7ca048d19d60351bef069ac00c236910dL670)

share the config you used in this PR.

https://github.com/vectordotdev/vector/pull/21583#issuecomment-2433783674

[pront]

@pront you removed

pub tls: Option<TlsConfig>,

now, there is no option to pass certs for mtls scenario - that was the idea behind this ticket, your http rest client can use different tls settings than cert/key used to acquire bearer token

Ah yes indeed, feel free to add this back. Please just push commits directly to this PR and I will take a look.

If you add that back, you can use make generate-component-docs to regenerate the docs.

You can also do cd website && make run-production-site-locally to see the result after the CUE files are updated. But that might need Hugo version 0.84.0.

share the config you used in this PR.

#21583 (comment)

Apologies if I wasn't clear, I meant that after this PR is finalized the config schema will change e.g. the http_client_authorization_strategy field should be removed right?

[KowalczykBartek]

 feel free to add this back

but now there is a problem I wrote already : https://github.com/vectordotdev/vector/pull/21857#issuecomment-2489802458 I don't know how to fix this.

[pront]

 feel free to add this back

but now there is a problem I wrote already : #21857 (comment) I don't know how to fix this.

I see, it's because TlsConfig doesn't derive PartialEq and Eq. But you can just add them and everything should just work:

/// TLS configuration.
#[configurable_component]
#[configurable(metadata(docs::advanced))]
#[derive(Clone, Debug, Default, PartialEq, Eq)]
#[serde(deny_unknown_fields)]
pub struct TlsConfig {

[KowalczykBartek]

Ok, i think we need to revert this from master. This will just not work - https://github.com/vectordotdev/vector/pull/21857/commits/da06b02e2f1c5eb2d1bf7dd1def570b307cbf350

sinks:
  my_sink_id:
    auth:
      strategy: "o_auth2"
      client_id: "xoxoxoxoxoxox"
      grace_period: 43100
      token_endpoint: "https://xoxoxoxoxox/oauth/token"
      tls:
        crt_path: xoxoxoxoxox/cert.pem
        key_file: xoxoxoxoxoxo/key.pem
    type: http
    encoding:
      codec: raw_message
    inputs:
      - my_source_id
    uri: http://localhost:9091/output

this will also kicks a

    pub fn apply_headers_map(&self, map: &mut HeaderMap) {
        match &self {
            Auth::Basic { user, password } => {
                let auth = Authorization::basic(user.as_str(), password.inner());
                map.typed_insert(auth);
            }
            Auth::Bearer { token } => match Authorization::bearer(token.inner()) {
                Ok(auth) => map.typed_insert(auth),
                Err(error) => error!(message = "Invalid bearer token.", token = %token, %error),
            },
            Auth::OAuth2 { .. } => panic!("OAuth2 authentication is not supported currently"),
        }
    }

which just crash vector, that's why I created separate property, to distinguish that from the beginning

But I fixed a major refactoring, so at least its again rfc compliant :)

[pront]

Ok, i think we need to revert this from master. This will just not work - da06b02

sinks:
  my_sink_id:
    auth:
      strategy: "o_auth2"
      client_id: "xoxoxoxoxoxox"
      grace_period: 43100
      token_endpoint: "https://xoxoxoxoxox/oauth/token"
      tls:
        crt_path: xoxoxoxoxox/cert.pem
        key_file: xoxoxoxoxoxo/key.pem
    type: http
    encoding:
      codec: raw_message
    inputs:
      - my_source_id
    uri: http://localhost:9091/output

The config format looks good 👍 However, I cannot follow all the above. Are you suggesting to revert the original PR?

this will also kicks a

    pub fn apply_headers_map(&self, map: &mut HeaderMap) {
        match &self {
            Auth::Basic { user, password } => {
                let auth = Authorization::basic(user.as_str(), password.inner());
                map.typed_insert(auth);
            }
            Auth::Bearer { token } => match Authorization::bearer(token.inner()) {
                Ok(auth) => map.typed_insert(auth),
                Err(error) => error!(message = "Invalid bearer token.", token = %token, %error),
            },
            Auth::OAuth2 { .. } => panic!("OAuth2 authentication is not supported currently"),
        }
    }

which just crash vector, that's why I created separate property, to distinguish that from the beginning

But I fixed a major refactoring, so at least its again rfc compliant :)

Does the RFC specify mandatory headers? If not, then we can replace the panic! with a () (noop).

[KowalczykBartek]

Does the RFC specify mandatory headers? If not, then we can replace the panic! with a () (noop).

no no, both properties are now used by two different mechanisms, this conceptually is wrong, there needs to be different property than auth, cuz auth is already used, so, this https://github.com/vectordotdev/vector/pull/21857/commits/66e619c14975bb2b1a8db223a2e3ba70f980f8a6 is minimal what can be done, but its redundant so... I have no more ideas

[pront]

Does the RFC specify mandatory headers? If not, then we can replace the panic! with a () (noop).

no no, both properties are now used by two different mechanisms, this conceptually is wrong, there needs to be different property than auth, cuz auth is already used, so, this 66e619c is minimal what can be done, but its redundant so... I have no more ideas

To summarize, I understand why you want TLS settings directly on your OAuth variant and on HttpSinkConfig.

Are you saying there is a need to have both auth and authorization_config on HttpSinkConfig? Is there a scenario where you would need to specify both? E.g.

sinks:
  my_sink_id:
    type: http
    encoding:
      codec: raw_message
    inputs:
      - my_source_id
    uri: http://localhost:9091/output

    # first
    auth:
      strategy: bearer
      token: xxx

   # second
   authorization_config:
      strategy: "o_auth2"
      client_id: "xoxoxoxoxoxox"
      grace_period: 43100
      token_endpoint: "https://xoxoxoxoxox/oauth/token"
      tls:
        crt_path: xoxoxoxoxox/cert.pem
        key_file: xoxoxoxoxoxo/key.pem

It would be a shame to drop all this work. If you can explain a bit better what are the challenges are here, I am confident we can find a solution.

[KowalczykBartek]

@pront I propose to run this locally, with this config https://github.com/vectordotdev/vector/pull/21857#issuecomment-2492157529 (any oauth without tls atm is ok). then you will see the panic and why I am writing about two mechanism kicking in and what have to be done is explained here https://github.com/vectordotdev/vector/pull/21583#issuecomment-2442619253

[pront]

@pront I propose to run this locally, with this config #21857 (comment) (any oauth without tls atm is ok). then you will see the panic and why I am writing about two mechanism kicking in and what have to be done is explained here #21583 (comment)

ACK. I doubt I can get to this before the release since I am occupied with some other work. Worst case, I will revert the original PR and then we can return to this without any time pressure.

[KowalczykBartek]

In the worst case this PR will be a signpost for next brave hero :)