[Feat] Prometheus metrics #1299
Conversation
Very nice! |
|
lgtm. but please stop fixing other stuff (like formating) completely unrelated to your feature |
I made a mistake in the commit commentary there. So I rearranged |
Or even better split this into 2 PRs please! @AKEB |
This reverts commit a998f6b.
[Feat]: Simple Stats API wg-easy#1285
|
Could also please post your prometheus config for other users to reference? |
|
|
What if the user has authentication setup? |
@offizium-berndstorath What kind of authentication setup do you mean? Authelia? |
No I mean if Password in wg-easy is set. Right now Prometheus can't scrape the endpoint as it would require a valid session or the correct headers |
No. You're completely wrong. The password is only needed if the path contains /api/ |
|
If the feature should be accepted a Rebase is needed, as I sayed before due, all new PRs should be at the top of older commits. |
Co-authored-by: Vadim Babadzhanyan <[email protected]>
Closes wg-easy#1302 Co-authored-by: Bernd Storath <[email protected]>
Signed-off-by: Philip H <[email protected]>
Yes ok, I think that that every route underneath the middleware is grouped logically to be handled with that middleware. But shouldn't the route be secured? Anybody just knowing your throughput, which clients are connected etc seems like a bad idea |
If you are afraid, you don't need to use it. Or you can do separately as suggested by other users on the net. There are many solutions on github. For example https://github.com/tolkonepiu/wg-easy-extended |
This comment was marked as outdated.
This comment was marked as outdated.
Separate port for prometheus metrics Add Prometheus metrics [Feat]: Simple Stats API wg-easy#1285
Separate port for prometheus metrics Add Prometheus metrics [Feat]: Simple Stats API wg-easy#1285
Makes sense. This does increase security. On a different note this feature will not work anymore with #1244 as this spins up a whole server just for prometheus which i think doesn't make much sense anyway. Prometheus supports passing headers to the route, so I don't see a reason against hiding it behind auth and just extending the prometheus config a bit |
This reverts commit a7376ab.
This reverts commit 9760bde.
This reverts commit 58f5b68.
This reverts commit 6d246ea.
[Feat]: Simple Stats API wg-easy#1285
|
@offizium-berndstorath are you happy now? ) |
Somewhat. i made some reviews. Also I dont really think theres a reason to add basic-auth package if prometheus supports e.g. bearer auth1 but im fine with the current implementation. but a extra password for prometheus seems like a good solution. Footnotes |
| if (client.endpoint !== null) { | ||
| wireguardConnectedPeersCount++; | ||
| } |
There was a problem hiding this comment.
unrelated to this pr but could this be used to check if client is online in frontend instead of latest handshake? #1305 (comment)
[Feat]: Simple Stats API wg-easy#1285
src/lib/Server.js
Outdated
| return { error: 'Not Logged In' }; | ||
| } | ||
|
|
||
| if (req.url.startsWith('/metrics') && user.pass) { |
There was a problem hiding this comment.
req.url.startsWith('/metrics') is redundant
There was a problem hiding this comment.
// WireGuard
app.use(
fromNodeMiddleware((req, res, next) => {
if (!requiresPassword || !req.url.startsWith('/api/')) {
return next();
}
if (req.session && req.session.authenticated) {
return next();
}
if (req.url.startsWith('/api/') && req.headers['authorization']) { # The Same
if (isPasswordValid(req.headers['authorization'], PASSWORD_HASH)) {
return next();
}
return res.status(401).json({ # This ERROR!!! 500
error: 'Incorrect Password',
});
}
return res.status(401).json({ # This ERROR!!! 500
error: 'Not Logged In',
});
}),
);
There was a problem hiding this comment.
You could be right. I'll think about it later
There was a problem hiding this comment.
@AKEB what did you mean with # This ERROR!!! 500?
There was a problem hiding this comment.
There is no res.status() function and at this point the code returns a status of 500 instead of 401
You need to do it like this:
res.statusCode = 401;
return { error: ‘Not Logged In’ };
There was a problem hiding this comment.
understood.
we are currently doing a rewrite using nuxt and typescript #1244 where this isn't the case. so i think its fine by now
|
@pheiduck Is there anything else you need from me? |
|
@AKEB i dont think. but i'll wait for his review |
Description
This update adds metrics for prometheus.
Dashboard for grafana can be imported by id 21733 or with JSON
WireGuard-1724075031304.json
Motivation and Context
#1285
How has this been tested?
On my server
Screenshots (if appropriate):
Types of changes
Checklist: