feat(service-monitor): support bearer token authentication on metrics endpoint #637
Reference in New Issue
Block a user
No description provided.
Delete Branch "refs/pull/637/head"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
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
Using above configuration will not give 401 authentication after changes.
⚠ BREAKING
No breaking changes
Checklist
Parameters are documented in thevalues.yaml
and added to theREADME.md
using readme-generator-for-helmBreaking changes are documented in theREADME.md
Signed-off-by: Hitesh Nayak hiteshnayak305@gmail.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. 🙂
👍 will try this weekend
In current configuration
ServiceMonitor
is created even ifgitea.metrics.enabled = false
andgitea.metrics.serviceMonitor.enabled = true
. It will be in Prometheus targets but failing with 404. Is it alright ?@hiteshnayak305
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. 🙂
Maybe worth adding a note into the README?
Metrics endpoint
/metrics
can be secured by usingBearer
token authentication.Note: Providing a non-empty
TOKEN
value will also require authentication forServiceMonitor
.LGTM. Let's wait for @justusbunsi review, I'd queue it to merge for the 1.22 release.
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 ahelm install/upgrade
.To resolve this issue, my suggestion would be to have the token configured outside of
gitea.config
. Either asgitea.metrics.token
orgitea.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 thatconfig.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)
@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.
Hello,
I had a busy schedule, i will start again with new pr this weekend whatever changes applicable.
Pull request closed