Send ports forwarded to control server #2392

jagaimoworks · 2024-08-03T12:40:09Z

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.

qdm12

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.

qdm12 · 2024-08-03T13:47:40Z

internal/portforward/loop.go

+	err := l.service.SetPortsForwarded(l.runCtx, ports)
+	if err != nil {
+		l.logger.Error(err.Error())
+	}


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.

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?

I suggested exactly that 😄 I should had read unresolved conversations better!

qdm12 · 2024-08-03T13:48:35Z

internal/portforward/loop.go

+	if l.service == nil {
+		return
+	}


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 😄

Yeah, that might be beyond me for now. 😅

No problem, let's keep this unresolved and I'll jump at implementing it later 😉

internal/portforward/service/service.go

internal/server/interfaces.go

internal/server/openvpn.go

…d vice versa

qdm12 · 2024-08-09T14:46:34Z

(Sort of) blocked by #1785

qdm12 · 2024-08-23T06:07:37Z

Blocked by #2238 as well.

andy3469 · 2024-11-09T10:12:52Z

Hello!
Any news on this PR ?

qdm12 · 2024-11-22T22:05:34Z

internal/portforward/service/service.go

+	for i, port := range s.ports {
+		err := s.portAllower.RemoveAllowedPort(ctx, port)
+		if err != nil {
+			for j := 0; j < i; j++ {


nit you can now use the 'intrange' introduced in Go 1.23

Suggested change

for j := 0; j < i; j++ {

for j := range i {

"Fixed" with a1e7f12

qdm12 · 2024-11-22T22:12:14Z

internal/portforward/service/service.go

+			s.logger.Error(err.Error())
+			return err


Let's remove the log here and let the caller handle the error

Suggested change

s.logger.Error(err.Error())

return err

return fmt.Errorf("removing allowed port: %w", err)

Fixed with 25fd6ff

qdm12 · 2024-11-22T22:12:54Z

internal/portforward/service/service.go

+			s.logger.Error(err.Error())
+			return err


Let's remove the log here and let the caller handle the error

Suggested change

s.logger.Error(err.Error())

return err

return fmt.Errorf("setting allowed port: %w", err)

Fixed with 25fd6ff

qdm12 · 2024-11-22T22:14:10Z

internal/server/openvpn.go

+	if len(data.Ports) == 0 {
+		http.Error(w, "no port specified", http.StatusBadRequest)
+		return
+	}


Maybe we could handle that as "remove forwarded ports"?

Simply removing the statement with 3b633e4 will clear forwarded ports, because we already remove the old port forwards when setting the new ones anyways.

qdm12 · 2024-11-22T22:16:12Z

internal/server/openvpn.go

+	if err := h.pf.SetPortsForwarded(data.Ports); err != nil {
+		http.Error(w, err.Error(), http.StatusInternalServerError)


Let's log out as warning the error from the function call, and only say "failed setting forwarded ports" in the http response (for the sake of not exposing too much details to the http caller who might not control the gluetun instance)

Suggested change

if err := h.pf.SetPortsForwarded(data.Ports); err != nil {

http.Error(w, err.Error(), http.StatusInternalServerError)

if err := h.pf.SetPortsForwarded(data.Ports); err != nil {

h.warner.Warn(fmt.Sprintf("failed setting forwarded ports: %s", err))

http.Error(w, "failed setting forwarded ports", http.StatusInternalServerError)

Done with 32a7f1f.
Also, I assumed that if we want to limit detail exposure through http responses we should give the json deconding logic just above the same treatment.

qdm12 · 2024-11-22T22:17:16Z

internal/portforward/loop.go

+	if l.service == nil {
+		return
+	}


No problem, let's keep this unresolved and I'll jump at implementing it later 😉

qdm12 · 2024-11-22T22:17:44Z

internal/portforward/loop.go

+	if err != nil {
+		l.logger.Error(err.Error())
+		return err


Let's remove the logs in the port forwarding code and let the calling layers log out the error if necessary (in this case in the control server code) - sorry if I might had changed my mind on this!

Suggested change

if err != nil {

l.logger.Error(err.Error())

return err

if err != nil {

return err

No worries, see e9aaa97

qdm12 · 2024-11-22T22:19:55Z

@jagaimoworks By the way:

Great work! ❤️
Sorry for the delay re-reviewing this 🕐
Just a few minor comments ✅

qdm12 · 2024-11-22T22:21:00Z

And @andy3469 I'm curious, what do you plan to use this PR for 😃?

…ngs instead

jagaimoworks added 2 commits August 3, 2024 14:17

added capability to set forwarded ports at runtime

ec3a983

added control server route for dynamic port forwarding

8be9055

jagaimoworks mentioned this pull request Aug 3, 2024

Bug: cannot remove allowed input port from firewall #2334

Closed

qdm12 reviewed Aug 3, 2024

View reviewed changes

qdm12 added Status: 🔴 Blocked Blocked by another issue or pull request Status: 🔒 After next release Will be done after the next release labels Aug 3, 2024

qdm12 force-pushed the master branch from 4d53eb6 to 10f43d7 Compare August 4, 2024 08:36

jagaimoworks added 6 commits August 5, 2024 17:35

addressing renaming nits

b1826bd

try to silently undo changes when port set/remove fail

1d8e3e1

added http error response for failing to set forwarded ports

f18cdb8

changed http error response to accuratly depict the error

8fbb43d

moved statement to improve readability

00dc345

fix: don´t readd failed port forwardings that failed to be removed an…

52522df

…d vice versa

qdm12 mentioned this pull request Aug 9, 2024

Feature request: run a script on interface up/down #1785

Closed

qdm12 removed the Status: 🔒 After next release Will be done after the next release label Aug 9, 2024

qdm12 added Status: 🔴 Blocked Blocked by another issue or pull request Status: 🟡 Nearly resolved This might be resolved or is about to be resolved and removed Status: 🔴 Blocked Blocked by another issue or pull request labels Aug 17, 2024

qdm12 reviewed Nov 22, 2024

View reviewed changes

jagaimoworks added 5 commits November 29, 2024 14:55

addressing intrange nit

a1e7f12

moved logging responsibility to caller

25fd6ff

reduced verbosity of the http responses and log the errors as a warni…

32a7f1f

…ngs instead

allow setting NO port forwards => clears forwarded ports

3b633e4

removed logging

e9aaa97

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Send ports forwarded to control server #2392

Send ports forwarded to control server #2392

jagaimoworks commented Aug 3, 2024 •

edited

Loading

qdm12 left a comment

qdm12 Aug 3, 2024

jagaimoworks Aug 5, 2024

qdm12 Nov 22, 2024

qdm12 Aug 3, 2024

jagaimoworks Aug 5, 2024

qdm12 Nov 22, 2024

qdm12 commented Aug 9, 2024

qdm12 commented Aug 23, 2024

andy3469 commented Nov 9, 2024

qdm12 Nov 22, 2024

jagaimoworks Nov 29, 2024

qdm12 Nov 22, 2024

jagaimoworks Nov 29, 2024

qdm12 Nov 22, 2024

jagaimoworks Nov 29, 2024

qdm12 Nov 22, 2024

jagaimoworks Nov 29, 2024

qdm12 Nov 22, 2024

jagaimoworks Nov 29, 2024

qdm12 Nov 22, 2024

qdm12 Nov 22, 2024

jagaimoworks Nov 29, 2024

qdm12 commented Nov 22, 2024

qdm12 commented Nov 22, 2024

	s.logger.Error(err.Error())
	return err
	return fmt.Errorf("removing allowed port: %w", err)

	s.logger.Error(err.Error())
	return err
	return fmt.Errorf("setting allowed port: %w", err)

		if err := h.pf.SetPortsForwarded(data.Ports); err != nil {
		http.Error(w, err.Error(), http.StatusInternalServerError)

Send ports forwarded to control server #2392

Are you sure you want to change the base?

Send ports forwarded to control server #2392

Conversation

jagaimoworks commented Aug 3, 2024 • edited Loading

qdm12 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qdm12 commented Aug 9, 2024

qdm12 commented Aug 23, 2024

andy3469 commented Nov 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qdm12 commented Nov 22, 2024

qdm12 commented Nov 22, 2024

jagaimoworks commented Aug 3, 2024 •

edited

Loading