-
-
Notifications
You must be signed in to change notification settings - Fork 350
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
Send ports forwarded to control server #2392
base: master
Are you sure you want to change the base?
Send ports forwarded to control server #2392
Conversation
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.
That's great, thanks for the PR 💯 !
I will wait to fix the iptables removals (to create less user frustration and duplicate issues) after v3.39.0 gets released, to merge this though.
internal/portforward/loop.go
Outdated
err := l.service.SetPortsForwarded(l.runCtx, ports) | ||
if err != nil { | ||
l.logger.Error(err.Error()) | ||
} |
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.
Perhaps we should return an error here to let the http client know it failed for xyz reason 🤔
And possibly log it as well, as it is now.
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.
Commit f18cdb8 addresses this. It would probably suffice to let control server respond with a more generic error, since the original error message already gets logged anyways. What do you think?
if l.service == nil { | ||
return | ||
} |
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.
We could set the ports somehow, even if the service is not started. The ports could then be injected to the service when we create it. A bit of a futuristic approach about when we could do all kind of modifications live 😄
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.
Yeah, that might be beyond me for now. 😅
for _, port := range s.ports { | ||
err := s.portAllower.RemoveAllowedPort(ctx, port) | ||
if err != nil { | ||
s.logger.Error(err.Error()) |
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.
If this fails, we should attempt to re-add the removed ports (ignoring the possible errors) to 'revert' back to how it was, and then return an error (we can log the error and also return it).
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.
See 1d8e3e1. I probably should add comments to the loops, right?
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.
With 52522df it now also won´t reprocess the failed setting or removal.
for _, port := range ports { | ||
err := s.portAllower.SetAllowedPort(ctx, port, s.settings.Interface) | ||
if err != nil { | ||
s.logger.Error(err.Error()) |
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.
If this fails, we should, in order to have an atomic state:
- attempt to remove the ports just added (
ports[0]
and up toports[len(ports)-2]
) and ignore errors returned by the removals - attempt to re-add previously removed
s.ports
and ignore errors returned by the additions.
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.
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.
Fixed it: 52522df
s.ports = make([]uint16, len(ports)) | ||
copy(s.ports, ports) | ||
|
||
s.logger.Info("Updated: " + portsToString(s.ports)) |
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.
nit
s.logger.Info("Updated: " + portsToString(s.ports)) | |
s.logger.Info("updated: " + portsToString(s.ports)) |
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.
See 8fbb43d.
internal/server/interfaces.go
Outdated
@@ -21,8 +21,9 @@ type DNSLoop interface { | |||
GetStatus() (status models.LoopStatus) | |||
} | |||
|
|||
type PortForwardedGetter interface { | |||
type PortForwarded interface { |
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.
nit whilst we're renaming it, I think PortForwardingService
or just PortForwarding
would make more sense 😉 !
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.
See b1826bd.
internal/server/openvpn.go
Outdated
@@ -11,19 +11,19 @@ import ( | |||
) | |||
|
|||
func newOpenvpnHandler(ctx context.Context, looper VPNLooper, | |||
pfGetter PortForwardedGetter, w warner) http.Handler { | |||
portForwarded PortForwarded, w warner) http.Handler { |
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.
nit rename to portForwarding
or portForwardingService
😉
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.
See b1826bd.
internal/server/openvpn.go
Outdated
} | ||
|
||
if len(data.Ports) == 0 { | ||
http.Error(w, "invalid request", http.StatusBadRequest) |
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.
http.Error(w, "invalid request", http.StatusBadRequest) | |
http.Error(w, "no port specified", http.StatusBadRequest) |
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.
Resolved with 8fbb43d.
internal/server/openvpn.go
Outdated
|
||
func (h *openvpnHandler) setPortForwarded(w http.ResponseWriter, r *http.Request) { | ||
decoder := json.NewDecoder(r.Body) | ||
encoder := json.NewEncoder(w) |
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.
nit move this above line 164 where it's first used. I know other handler functions are ugly doing this as well, but since it's fresh new code 😸
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.
Your wish is my command 😄 (00dc345)
(Sort of) blocked by #1785 |
Blocked by #2238 as well. |
First timer here. This is a somewhat working implementation of #2369. Hit me with the improvements I can take it 😅
I say somewhat working because the removal of ports from the firewall suffers from #2334 and therefore does not reliably work right now.
The way it works right now is by sending a http PUT request with a body like
{ports: [1234, 3456]}
to/v1/openvpn/portforwarded
.