feat(otelcol): add support for htpasswd file authentication#3916
feat(otelcol): add support for htpasswd file authentication#3916dehaansa merged 5 commits intografana:mainfrom
Conversation
920e168 to
f15ea16
Compare
f15ea16 to
c9b7cb7
Compare
|
|
||
| htpasswd_file = "/etc/alloy/.htpasswd" |
There was a problem hiding this comment.
| htpasswd_file = "/etc/alloy/.htpasswd" |
The usage section only lists required attributes and blocks. You could add an example to the example section though.
| }, | ||
| }, nil | ||
| c := &basicauthextension.Config{ | ||
| Htpasswd: &basicauthextension.HtpasswdSettings{}, |
There was a problem hiding this comment.
It'd be better if this is only created if args.HtpasswdFile is not empty.
| Username string `alloy:"username,attr,optional"` | ||
| Password alloytypes.Secret `alloy:"password,attr,optional"` | ||
|
|
||
| HtpasswdFile string `alloy:"htpasswd_file,attr,optional"` |
There was a problem hiding this comment.
| HtpasswdFile string `alloy:"htpasswd_file,attr,optional"` | |
| HtpasswdFile string `alloy:"htpasswd_file,block,optional"` |
To match OTel, you'll need to create an htpasswd block which has two attributes - file and inline.
There was a problem hiding this comment.
so in case the htpasswd block is provided do you want to ignore the username password that may be provided? Asking mainly cause the extension supports having both file and inline, and inline is currently created by just combining username/password, which is a behavior I tried to keep.
There was a problem hiding this comment.
@ptodev feel free to let me know if that's what you had in mind.
There was a problem hiding this comment.
Hi, apologies for the late reply! The first time reviewed the PR I didn't quite understand some aspects of the upstream component (that one config block is for server whereas the other is for client). I also didn't realise that Alloy sets the client config directly, without having a client block in Alloy syntax. This makes it more complicated...
Ideally we should match the OTel config 1:1. But we also don't want to break existing users, at least not until Alloy v2 is released. So I think the best thing for you to do now is add both a htpasswd and a client_auth block, and to mention in the docs that the current arguments are deprecated. Then the code should ignore them if there is a client_auth block. I hope this makes sense? I'm sorry that it has to be so complicated :/
If we do this then I'll also open an issue with a v2.0-breaking-change label to remove the current arguments in Alloy v2.
| if args.HtpasswdFile != "" { | ||
| c.Htpasswd.File = args.HtpasswdFile | ||
| } |
There was a problem hiding this comment.
The best way to do the conversion is to make htpasswd a struct and make a convert function for it, similarly to KafkaExporterSignalConfig.
b780c89 to
fa9cb16
Compare
| if c.File != "" { | ||
| settings.File = c.File | ||
| } | ||
| if c.Inline != "" { | ||
| settings.Inline = c.Inline | ||
| } |
There was a problem hiding this comment.
There's no need to check if they're empty. It's ok to pass them as they are.
| Username string `alloy:"username,attr,optional"` | ||
| Password alloytypes.Secret `alloy:"password,attr,optional"` | ||
|
|
||
| HtpasswdFile string `alloy:"htpasswd_file,attr,optional"` |
There was a problem hiding this comment.
Hi, apologies for the late reply! The first time reviewed the PR I didn't quite understand some aspects of the upstream component (that one config block is for server whereas the other is for client). I also didn't realise that Alloy sets the client config directly, without having a client block in Alloy syntax. This makes it more complicated...
Ideally we should match the OTel config 1:1. But we also don't want to break existing users, at least not until Alloy v2 is released. So I think the best thing for you to do now is add both a htpasswd and a client_auth block, and to mention in the docs that the current arguments are deprecated. Then the code should ignore them if there is a client_auth block. I hope this makes sense? I'm sorry that it has to be so complicated :/
If we do this then I'll also open an issue with a v2.0-breaking-change label to remove the current arguments in Alloy v2.
docs/sources/reference/components/otelcol/otelcol.auth.basic.md
Outdated
Show resolved
Hide resolved
docs/sources/reference/components/otelcol/otelcol.auth.basic.md
Outdated
Show resolved
Hide resolved
docs/sources/reference/components/otelcol/otelcol.auth.basic.md
Outdated
Show resolved
Hide resolved
docs/sources/reference/components/otelcol/otelcol.auth.basic.md
Outdated
Show resolved
Hide resolved
docs/sources/reference/components/otelcol/otelcol.auth.basic.md
Outdated
Show resolved
Hide resolved
docs/sources/reference/components/otelcol/otelcol.auth.basic.md
Outdated
Show resolved
Hide resolved
docs/sources/reference/components/otelcol/otelcol.auth.basic.md
Outdated
Show resolved
Hide resolved
docs/sources/reference/components/otelcol/otelcol.auth.basic.md
Outdated
Show resolved
Hide resolved
docs/sources/reference/components/otelcol/otelcol.auth.basic.md
Outdated
Show resolved
Hide resolved
docs/sources/reference/components/otelcol/otelcol.auth.basic.md
Outdated
Show resolved
Hide resolved
|
Docs look of for this iteration :-) I'll wait on code reviews before clicking approve for docs. |
c09bdf1 to
b7631e3
Compare
| htpasswd { | ||
| file = "/etc/alloy/.htpasswd" | ||
| inline = "<USERNAME>:<PASSWORD>" | ||
| } | ||
|
|
||
| client_auth { | ||
| username = "<USERNAME>" | ||
| password = "<PASSWORD>" | ||
| } |
There was a problem hiding this comment.
I would remove this section. the htpasswd and client_auth blocks are optional. We should keep the Usage section to just the required elements.
b7631e3 to
9c28fef
Compare
|
Can I have an update on this, or an ETA on when you expect this to get reviewed/released? |
dehaansa
left a comment
There was a problem hiding this comment.
Overall looks good, if you can resolve the outstanding comments we should be good to merge. Sorry that this slipped off our radar.
| } | ||
| ``` | ||
|
|
||
| #### Use client authentication |
There was a problem hiding this comment.
What's the difference between this example and the Forward signals to exporters example?
There was a problem hiding this comment.
The difference is that the first (forward signals to exporters uses the deprecated username and password top-level arguments whereas this uses the new client authentication block. Since I have split this into client and server authentication examples, I think I should just move this a below Forward signals to exporters.
There was a problem hiding this comment.
I think pairing them together would be better, but similar to the other thread I'm not a fan of showing deprecated fields in config examples in most cases. Let's see what our docs team says.
|
|
||
| #### Combination of htpasswd and the username and password attributes | ||
|
|
||
| This example configures [`otelcol.receiver.otlp`][otelcol.receiver.otlp] to use basic authentication using a combination |
There was a problem hiding this comment.
As this is not a recommended path, we probably shouldn't include it in the docs.
There was a problem hiding this comment.
I think that this could be a good migration path for users to client and htpasswd blocks, but if you don't agree, I have no problem removing this.
They could first add a new user to the htpasswd file, start using the new one in their systems and once the old user is not used anymore, they could just drop it. WDYT?
I can explicitly add this as a migration path if you'd like.
There was a problem hiding this comment.
My opinion is that if we clearly state fields are deprecated, we don't want to show them in configuration examples. CC @clayton-cornell any thoughts on this?
There was a problem hiding this comment.
I missed that in my earlier review.
I agree, if args/fields are clearly marked as deprecated, we should not be providing examples that use these args. The examples should show current state only. If someone needs to see the previous implementation, the docs showing the args/fields before deprecation are available in a previous doc version.
For migration path... maybe add as a note admonition? with text something like "To migrate an existing configuration to use client_auth and htpasswd blocks, you can..."
There was a problem hiding this comment.
@clayton-cornell what do you think about this? I don't have any strong feelings regardless but I would really like for this to be in a future release since I would like to reduce the components I have to support in production (and the actual hops the requests take).
There was a problem hiding this comment.
And I forgot to commit my pending comments on this PR. Sorry.
There was a problem hiding this comment.
Thank you for the insights. I have removed the example of the deprecated attributes and moved the migration guides to a note admonition as you suggested. Feel free to take a look and let me know.
(I have also rebased on top of the latest main as the pr was pretty out of date)
9c28fef to
2389d55
Compare
clayton-cornell
left a comment
There was a problem hiding this comment.
Some minor tidy suggestions on the docs markdown. There's still passive voice, but I'll leave for this PR.
|
|
||
| ### `client_auth` | ||
|
|
||
| The `client_auth` block configures credentials that client extensions (such as exporters) will use to authenticate to servers. |
There was a problem hiding this comment.
| The `client_auth` block configures credentials that client extensions (such as exporters) will use to authenticate to servers. | |
| The `client_auth` block configures credentials that client extensions (such as exporters) use to authenticate to servers. |
| | Name | Type | Description | Default | Required | | ||
| |------------|----------|-----------------------------------------------------|---------|----------| | ||
| | `password` | `string` | Password to use for basic authentication requests | | yes | | ||
| | `username` | `string` | Username to use for basic authentication requests | | yes | |
There was a problem hiding this comment.
| | Name | Type | Description | Default | Required | | |
| |------------|----------|-----------------------------------------------------|---------|----------| | |
| | `password` | `string` | Password to use for basic authentication requests | | yes | | |
| | `username` | `string` | Username to use for basic authentication requests | | yes | | |
| | Name | Type | Description | Default | Required | | |
| | ---------- | -------- | -------------------------------------------------- | ------- | -------- | | |
| | `password` | `string` | Password to use for basic authentication requests. | | yes | | |
| | `username` | `string` | Username to use for basic authentication requests. | | yes | |
| |----------------------------------|----------------------------------------------------------------------------|----------| | ||
| | [`client_auth`][client_auth] | Configures client authentication credentials for exporters | no | | ||
| | [`debug_metrics`][debug_metrics] | Configures the metrics that this component generates to monitor its state. | no | | ||
| | [`htpasswd`][htpasswd] | Configures server authentication using htpasswd format for receivers | no | |
There was a problem hiding this comment.
| |----------------------------------|----------------------------------------------------------------------------|----------| | |
| | [`client_auth`][client_auth] | Configures client authentication credentials for exporters | no | | |
| | [`debug_metrics`][debug_metrics] | Configures the metrics that this component generates to monitor its state. | no | | |
| | [`htpasswd`][htpasswd] | Configures server authentication using htpasswd format for receivers | no | | |
| |----------------------------------|----------------------------------------------------------------------------|----------| | |
| | [`client_auth`][client_auth] | Configures client authentication credentials for exporters. | no | | |
| | [`debug_metrics`][debug_metrics] | Configures the metrics that this component generates to monitor its state. | no | | |
| | [`htpasswd`][htpasswd] | Configures server authentication using htpasswd format for receivers. | no | |
| | Name | Type | Description | Default | Required | | ||
| |----------|----------|--------------------------------------------------------------------|---------|----------| | ||
| | `file` | `string` | Path to the htpasswd file to use for basic authentication requests | `""` | no | | ||
| | `inline` | `string` | The htpasswd file content in inline format | `""` | no | |
There was a problem hiding this comment.
| | Name | Type | Description | Default | Required | | |
| |----------|----------|--------------------------------------------------------------------|---------|----------| | |
| | `file` | `string` | Path to the htpasswd file to use for basic authentication requests | `""` | no | | |
| | `inline` | `string` | The htpasswd file content in inline format | `""` | no | | |
| | Name | Type | Description | Default | Required | | |
| | -------- | -------- | --------------------------------------------------------------------- | ------- | -------- | | |
| | `file` | `string` | Path to the `htpasswd` file to use for basic authentication requests. | `""` | no | | |
| | `inline` | `string` | The `htpasswd` file content in inline format. | `""` | no | |
| You can specify either `file`, `inline`, or both. When using `inline`, the format should be `username:password` with | ||
| each user on a new line. |
There was a problem hiding this comment.
| You can specify either `file`, `inline`, or both. When using `inline`, the format should be `username:password` with | |
| each user on a new line. | |
| You can specify either `file`, `inline`, or both. | |
| When you use `inline`, the format should be `username:password` with each user on a new line. |
| To migrate from the deprecated `username` and `password` attributes, move them into the `client_auth` block for client | ||
| authentication. | ||
| {{< /admonition >}} | ||
|
|
||
|
|
There was a problem hiding this comment.
| To migrate from the deprecated `username` and `password` attributes, move them into the `client_auth` block for client | |
| authentication. | |
| {{< /admonition >}} | |
| To migrate from the deprecated `username` and `password` attributes, move them into the `client_auth` block for client authentication. | |
| {{< /admonition >}} | |
| This example configures [`otelcol.receiver.otlp`][otelcol.receiver.otlp] to use basic authentication using an htpasswd | ||
| file containing the users to use for basic auth: |
There was a problem hiding this comment.
| This example configures [`otelcol.receiver.otlp`][otelcol.receiver.otlp] to use basic authentication using an htpasswd | |
| file containing the users to use for basic auth: | |
| This example configures [`otelcol.receiver.otlp`][otelcol.receiver.otlp] to use basic authentication using an `htpasswd` file containing the users to use for basic authentication: |
|
|
||
| #### Use htpasswd inline content | ||
|
|
||
| This example shows how to specify htpasswd content directly in the configuration: |
There was a problem hiding this comment.
| This example shows how to specify htpasswd content directly in the configuration: | |
| This example shows how to specify `htpasswd` content directly in the configuration: |
clayton-cornell
left a comment
There was a problem hiding this comment.
Just spotted a couple more minor tweaks we can do here consistency with the other topics.
| | `username` | `string` | Username to use for basic authentication requests. | | yes | | ||
|
|
||
| {{< admonition type="note" >}} | ||
| When you specify both the `client_auth` block and the deprecated top-level `username` and `password` attributes, the `client_auth` block takes precedence and Alloy ignores the top-level attributes for client authentication. |
There was a problem hiding this comment.
| When you specify both the `client_auth` block and the deprecated top-level `username` and `password` attributes, the `client_auth` block takes precedence and Alloy ignores the top-level attributes for client authentication. | |
| When you specify both the `client_auth` block and the deprecated top-level `username` and `password` attributes, the `client_auth` block takes precedence and {{< param "PRODUCT_NAME" >}} ignores the top-level attributes for client authentication. |
Missed this in my previous reviews. We use a product variable for the Alloy product name. The result is the same in the published docs.. just good to update this to the variable for consistency.
| When you use `inline`, the format should be `username:password` with each user on a new line. | ||
|
|
||
| {{< admonition type="note" >}} | ||
| When you specify both the `htpasswd` block and the deprecated top-level `username` and `password` attributes, Alloy automatically appends the deprecated credentials to the `inline` content. This allows authentication using credentials from both the htpasswd configuration and the deprecated attributes. |
There was a problem hiding this comment.
| When you specify both the `htpasswd` block and the deprecated top-level `username` and `password` attributes, Alloy automatically appends the deprecated credentials to the `inline` content. This allows authentication using credentials from both the htpasswd configuration and the deprecated attributes. | |
| When you specify both the `htpasswd` block and the deprecated top-level `username` and `password` attributes, {{< param "PRODUCT_NAME" >}} automatically appends the deprecated credentials to the `inline` content. | |
| This allows authentication using credentials from both the `htpasswd` configuration and the deprecated attributes. |
| ``` | ||
|
|
||
| {{< admonition type="note" >}} | ||
| To make the migration from the deprecated `username` and `password` attributes easier, you can specify both the deprecated attributes and the `htpasswd` block in the same configuration. Alloy appends the deprecated attributes to the `htpasswd` content. |
There was a problem hiding this comment.
| To make the migration from the deprecated `username` and `password` attributes easier, you can specify both the deprecated attributes and the `htpasswd` block in the same configuration. Alloy appends the deprecated attributes to the `htpasswd` content. | |
| To make the migration from the deprecated `username` and `password` attributes easier, you can specify both the deprecated attributes and the `htpasswd` block in the same configuration. | |
| {{< param "PRODUCT_NAME" >}} appends the deprecated attributes to the `htpasswd` content. |
|
|
||
| ### `htpasswd` | ||
|
|
||
| The `htpasswd` block configures how server extensions (such as receivers) authenticate incoming requests using the htpasswd format. |
There was a problem hiding this comment.
| The `htpasswd` block configures how server extensions (such as receivers) authenticate incoming requests using the htpasswd format. | |
| The `htpasswd` block configures how server extensions (such as receivers) authenticate incoming requests using the `htpasswd` format. |
027e8a8 to
13210c8
Compare
dehaansa
left a comment
There was a problem hiding this comment.
Thanks for the contribution, and thanks for your patience with the slow review cycles!
…3916) * feat(otelcol): add support for htpasswd file authentication * feat(otelcol): add client authentication block to basic auth config * fixup! feat(otelcol): add client authentication block to basic auth config * Update changelog --------- Co-authored-by: Sam DeHaan <sam.dehaan@grafana.com>
PR Description
This adds support for using
htpasswdfor server authentication. Sinceotelcol.auth.basicis a wrapper aroundbasicauthextension, which already supports both inline and file based server authentication, this adds such support for alloy as well. This would be really useful when we want to have multiple users trying to send data to Alloy and we don't want to a reverse proxy for authenticating calls.This also makes the
usernameandpasswordfields optional since we should be able to just usehtpasswdfiles for server side authentication. They must be filled in when the handler is used for client authentication.PR Checklist