-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature monitor the resource over proxy #840
Feature monitor the resource over proxy #840
Conversation
8471ce9
to
f13bf6b
Compare
b5a6114
to
3b99517
Compare
@Saibamen Thank you for your quick reviews, I have updated them. |
d21faa1
to
5515d64
Compare
5515d64
to
39804d1
Compare
Looking good. As a note, I think the switches should be "Enabled" and "Set As Default" instead of "Activated" and "Default Enabled". |
Or radio button Radio button is also better for |
@louislam I have used them as radio buttons and multiple proxy can not selected. I prefer radios over dropdown because look more fancy but dropdown also considerable. Screen.Recording.2021-11-03.at.12.08.28.mp4
@Saibamen I did not understand this, could you please clarify? 🤔 |
39804d1
to
2b061ce
Compare
@deefdragon Thank you for your advice, agree on this and updated accordingly. Now, it is more expressive. |
It is OK. I just wanted to say in my previous comment, that you can't implement working |
@ugurerkan nice work, there are a few monitors I cannot yet create because I need to go through a proxy. Would this feature work with sock5 type proxies ? |
@thomasleveil thank you very much. Currently, the feature supports only HTTP and HTTPS protocols. But, the base proxy agent library is supporting SOCKS and PAC proxy as well. So, this could be easily integrated. I will take a look and try to improve accordingly. SOCKS proxies have common usage, it looks like this feature should have it at least. |
57bc880
to
d36373e
Compare
SOCKS proxy agent library and protocol support added. |
d36373e
to
6e2963f
Compare
@ugurerkan may I suggest using |
6e2963f
to
461105d
Compare
@thomasleveil good point, thank you. |
e4c5b0b
to
1a7da29
Compare
There are couple straightforward solutions like below; Socks Proxy
HTTP/S Proxy I suggest tinyproxy for HTTP proxy and dante or ssh for SOCKS proxies. |
1a7da29
to
6160665
Compare
I have sync the branch with new settings page. |
All credit goes to @chakflying! |
Hey @louislam, it is possible to relate and support proxy for notifications there is a couple of feature request for this #616 #915 I think it is better to implement this with an axios wrapper notification client. The notification client is going to provide support of proxy to HTTP/S requests of notification providers. The wrapper approach is will reduce copy code to implement the proxy option to each notification provider. If it is suited well to uptime kuma goals, I can contribute to the implementation of this feature after the proxy feature is merged. 🤔 |
Any plan to merge this feature ? |
6160665
to
97acf08
Compare
97acf08
to
8527f6c
Compare
Added new proxy feature based on http and https proxy agents. Proxy feature works like notifications, there is many proxy could be related one proxy entry. Supported features - Proxies can activate and disable in bulk - Proxies auto enabled by default for new monitors - Proxies could be applied in bulk to current monitors - Both authenticated and anonymous proxies supported - Export and import support for proxies
- Socks proxy support implemented. - Monitor proxy agent create flow refactored and moved under proxy class. Thanks for suggestion @thomasleveil
8527f6c
to
8078d06
Compare
@ugurerkan Does this support Proxies using |
|
||
<div v-if="proxy.auth" class="mb-3"> | ||
<label for="proxy-password" class="form-label">{{ $t("Password") }}</label> | ||
<input id="proxy-password" v-model="proxy.password" type="text" class="form-control" required> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just finished testing, it is a solid update and feature!
Just found this should be type="password"
. I will update it my branch, because I don't have a write permission of this pr.
Thank you so much for the big feature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much for your test and review. It is good to help and contribute.
@gaby Tested with Squid Proxy, there is couple solution for this;
I think, solving this above uptime kuma level would be better and more flexible for users and we can keep our interface simple. References; |
Something like Excellent work, i'm looking forward to using this recently merged feature! |
await sendProxyList(socket); | ||
|
||
if (proxy.applyExisting) { | ||
await restartMonitors(socket.userID); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just checking source code, may found some potential issues.
Should only restart monitors which belong to this proxy.
Even better, the proxy config of monitors should be updated without restarting.
@@ -222,6 +222,32 @@ | |||
{{ $t("Setup Notification") }} | |||
</button> | |||
|
|||
<!-- Proxies --> | |||
<h2 class="mt-5 mb-2">{{ $t("Proxies") }}</h2> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Proxies section should be available for http(s) monitor type only?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, it is better to show only v-if="monitor.type === 'http' || monitor.type === 'keyword'"
@@ -1490,6 +1562,19 @@ async function restartMonitor(userID, monitorID) { | |||
return await startMonitor(userID, monitorID); | |||
} | |||
|
|||
async function restartMonitors(userID) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function creates another set of monitors unexpectedly, because it does not stop current monitors.
But as the proxy config of monitors should be updated without restarting, this function is not needed anyway in my opinion.
|
||
await Proxy.delete(proxyID, socket.userID); | ||
await sendProxyList(socket); | ||
await restartMonitors(socket.userID); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should only restart monitors which belong to this proxy.
Even better, the proxy config of monitors should be updated without restarting.
@louislam thank you in detail evaluation, I'll check update proxy configuration without restart monitor and create fix. It is going work better without restart all monitors and side effect you are detected. |
Thanks, I just created a pr for related issues |
Description
Added new proxy feature based on http and https proxy agents. Proxy feature works like notifications, there is many proxy could be related one proxy entry.
Supported features
Reviews are welcome 🤩
Fixes #470
Type of change
Checklist
Screenshots