feat(service-monitor): support bearer token authentication on metrics endpoint #637

Closed
hiteshnayak305 wants to merge 6 commits from refs/pull/637/head into main
hiteshnayak305 commented 2024-04-03 21:05:28 +00:00 (Migrated from gitea.com)

Description of the change

Benefits

Can protect metrics endpoint when monitoring using ServiceMonitor of prometheus-operator.

Possible drawbacks

No possible drawbacks

Applicable issues

Additional information

gitea:
  config:
    metrics:
      # ENABLED: true
      TOKEN: "somepassword"
  metrics:
    enabled: true
    serviceMonitor:
      enabled: true

Using above configuration will not give 401 authentication after changes.

⚠ BREAKING

No breaking changes

Checklist

  • Parameters are documented in the values.yaml and added to the README.md using readme-generator-for-helm
  • Breaking changes are documented in the README.md
  • Templating unittests are added

Signed-off-by: Hitesh Nayak hiteshnayak305@gmail.com

<!-- Before you open the request please review the following guidelines and tips to help it be more easily integrated: - Describe the scope of your change - i.e. what the change does. - Describe any known limitations with your change. - Please run any tests or examples that can exercise your modified code. Thank you for contributing! We will try to review, test and integrate the change as soon as we can. --> ### Description of the change <!-- Describe the scope of your change - i.e. what the change does. --> ### Benefits <!-- What benefits will be realized by the code change? --> Can protect metrics endpoint when monitoring using ServiceMonitor of prometheus-operator. ### Possible drawbacks No possible drawbacks <!-- Describe any known limitations with your change --> ### Applicable issues <!-- Enter any applicable Issues here (You can reference an issue using #). Please remove this section if there is no referenced issue. --> - fixes #635 ### Additional information <!-- If there's anything else that's important and relevant to your pull request, mention that information here. Please remove this section if it remains empty. --> ``` gitea: config: metrics: # ENABLED: true TOKEN: "somepassword" metrics: enabled: true serviceMonitor: enabled: true ``` Using above configuration will not give 401 authentication after changes. ### ⚠ BREAKING <!-- If there's a breaking change, please shortly describe in which way users are affected and how they can mitigate it. If there are no breakings, please remove this section. --> No breaking changes ### Checklist <!-- [Place an '[X]' (no spaces) in all applicable fields. Please remove unrelated fields.] --> - ~~Parameters are documented in the `values.yaml` and added to the `README.md` using [readme-generator-for-helm](https://github.com/bitnami-labs/readme-generator-for-helm)~~ - ~~Breaking changes are documented in the `README.md`~~ - [X] Templating unittests are added Signed-off-by: Hitesh Nayak <hiteshnayak305@gmail.com>
justusbunsi commented 2024-04-09 15:49:22 +00:00 (Migrated from gitea.com)

@hiteshnayak305 You checked the "unittests are added" checkbox in the checklist. I cannot find them. Please add meaningful ones. Think of edge cases, if any.
Unittests are required to get this PR merged. 🙂

@hiteshnayak305 You checked the "unittests are added" checkbox in the checklist. I cannot find them. Please add meaningful ones. Think of edge cases, if any. Unittests are required to get this PR merged. 🙂
hiteshnayak305 commented 2024-04-10 05:57:52 +00:00 (Migrated from gitea.com)

👍 will try this weekend

👍 will try this weekend
hiteshnayak305 commented 2024-04-10 17:42:25 +00:00 (Migrated from gitea.com)

In current configuration ServiceMonitor is created even if gitea.metrics.enabled = false and gitea.metrics.serviceMonitor.enabled = true. It will be in Prometheus targets but failing with 404. Is it alright ?

In current configuration `ServiceMonitor` is created even if `gitea.metrics.enabled = false` and `gitea.metrics.serviceMonitor.enabled = true`. It will be in Prometheus targets but failing with 404. Is it alright ?
justusbunsi commented 2024-04-10 18:16:06 +00:00 (Migrated from gitea.com)

@hiteshnayak305

In current configuration ServiceMonitor is created even if gitea.metrics.enabled = false and gitea.metrics.serviceMonitor.enabled = true. It will be in Prometheus targets but failing with 404. Is it alright ?

Good catch. I guess that's a tricky one to fix, if possible right now. You can configure the metrics settings for app.ini within configmaps or secrets that are read when a release is already applied to the cluster. At that point you already had to make the decision to render the servicemonitor or not to render it.
I think the current situation is ok. If an admin intend to monitor Gitea, this will be noticed shortly after applying the release. 🙂

@hiteshnayak305 >In current configuration `ServiceMonitor` is created even if `gitea.metrics.enabled = false` and `gitea.metrics.serviceMonitor.enabled = true`. It will be in Prometheus targets but failing with 404. Is it alright ? Good catch. I guess that's a tricky one to fix, if possible right now. You can configure the metrics settings for app.ini within configmaps or secrets that are read when a release is already applied to the cluster. At that point you already had to make the decision to render the servicemonitor or not to render it. I think the current situation is ok. If an admin intend to monitor Gitea, this will be noticed shortly after applying the release. 🙂
pat-s commented 2024-04-16 08:22:20 +00:00 (Migrated from gitea.com)

Maybe worth adding a note into the README?

Maybe worth adding a note into the README?
pat-s (Migrated from gitea.com) reviewed 2024-04-23 07:15:42 +00:00
pat-s (Migrated from gitea.com) commented 2024-04-23 07:15:42 +00:00

Metrics endpoint /metrics can be secured by using Bearer token authentication.
Note: Providing a non-empty TOKEN value will also require authentication for ServiceMonitor.

Metrics endpoint `/metrics` can be secured by using `Bearer` token authentication. **Note**: Providing a non-empty `TOKEN` value will also require authentication for `ServiceMonitor`.
pat-s (Migrated from gitea.com) approved these changes 2024-05-02 08:22:00 +00:00
pat-s (Migrated from gitea.com) left a comment

LGTM. Let's wait for @justusbunsi review, I'd queue it to merge for the 1.22 release.

LGTM. Let's wait for @justusbunsi review, I'd queue it to merge for the 1.22 release.
justusbunsi (Migrated from gitea.com) reviewed 2024-05-03 17:10:47 +00:00
justusbunsi (Migrated from gitea.com) left a comment

The current solution forces you to configure the metrics token within gitea.config in values.yaml. We have a feature where you can load those configuration snippets from external resources like Secrets and ConfigMaps. If a user doesn't want to have custom inline app.ini settings in their values.yaml, they are not able to benefit from this feature, as we are not able to retrieve the token while rendering the templates for a helm install/upgrade.

To resolve this issue, my suggestion would be to have the token configured outside of gitea.config. Either as gitea.metrics.token or gitea.metrics.serviceMonitor.token. That way, the user doesn't need to configure metrics related settings at 2 different places. Having it that way would allows us to assist the user by adding that config.metrics.TOKEN for them. Like we do for database related settings when using the subcharts.

It would also simplify the checks when to render it.

(By the way, I appreciate the written test cases👍. Especially those that ensure correct behavior for misconfiguration)

The current solution forces you to configure the metrics token within `gitea.config` in values.yaml. We have a feature where you can load those configuration snippets from external resources like Secrets and ConfigMaps. If a user doesn't want to have custom inline app.ini settings in their values.yaml, they are not able to benefit from this feature, as we are not able to retrieve the token while rendering the templates for a `helm install/upgrade`. To resolve this issue, my suggestion would be to have the token configured outside of `gitea.config`. Either as `gitea.metrics.token` or `gitea.metrics.serviceMonitor.token`. That way, the user doesn't need to configure metrics related settings at 2 different places. Having it that way would allows us to assist the user by adding that `config.metrics.TOKEN` for them. Like we do for database related settings when using the subcharts. It would also simplify the checks when to render it. (By the way, I appreciate the written test cases👍. Especially those that ensure correct behavior for misconfiguration)
pat-s commented 2024-07-13 12:09:37 +00:00 (Migrated from gitea.com)

@justusbunsi Good proposal!

@hiteshnayak305 Would you be interested in following the suggestions from @justusbunsi? I am aware this complicated things a bit and involves more work, even though this seems like a "small change". Just let us know what you think, even if you don't wanna do this or need more information about the implementation.

@justusbunsi Good proposal! @hiteshnayak305 Would you be interested in following the suggestions from @justusbunsi? I am aware this complicated things a bit and involves more work, even though this seems like a "small change". Just let us know what you think, even if you don't wanna do this or need more information about the implementation.
hiteshnayak305 commented 2024-10-21 07:51:29 +00:00 (Migrated from gitea.com)

Hello,

I had a busy schedule, i will start again with new pr this weekend whatever changes applicable.

Hello, I had a busy schedule, i will start again with new pr this weekend whatever changes applicable.

Pull request closed

Sign in to join this conversation.
No description provided.