Skip to content
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

feat(portforwarding): Allow running script upon port forwarding success #2399

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

lavalleeale
Copy link

@lavalleeale lavalleeale commented Aug 6, 2024

This PR allows an environment variable (VPN_PORT_FORWARDING_STATUS_SCRIPT) to be set that will run a script whenever there is an update to port forwarding status that could be used to sync with other programs that need to know what ports have been forwarded.

EDIT by qdm12:

Copy link
Owner

@qdm12 qdm12 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, thank you for this PR 👍

We need to notably:

  • change this to a run command (instead of script file)
  • make code ready for a down command (later 🙏, if anyone needs such command)
  • make a PR to the github wiki to document this (probably in the setup/advance/vpn-port-forwarding document) and mention the command is a single command, which doesn't understand shell specific syntax such as &&, and one should use /bin/sh -c "my shell syntax" to do so if they want (bash isn't installed by default, only sh)

Thanks!

internal/configuration/settings/portforward.go Outdated Show resolved Hide resolved
internal/configuration/settings/portforward.go Outdated Show resolved Hide resolved
internal/configuration/settings/portforward.go Outdated Show resolved Hide resolved
internal/configuration/settings/portforward.go Outdated Show resolved Hide resolved
internal/portforward/service/script.go Outdated Show resolved Hide resolved
internal/portforward/service/script.go Outdated Show resolved Hide resolved
internal/portforward/service/script.go Outdated Show resolved Hide resolved
internal/portforward/service/start.go Outdated Show resolved Hide resolved
internal/portforward/service/start.go Outdated Show resolved Hide resolved
@lavalleeale
Copy link
Author

Ok, I believe that I have resolved all the issues with the initial implementation, I am not a huge fan of comma separating the ports, but that is how the current code works to give maximum compatibility

Copy link
Owner

@qdm12 qdm12 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice job! 👍
Just 2 renames to do 😉
And once done I'll handle the using-that-command-package-and-plug-lines-to-the-logger. I just worked on it yesterday so it's still fresh in my mind 😉 !

internal/portforward/service/command.go Show resolved Hide resolved
)

func (s *Service) runUpCommand(ctx context.Context, ports []uint16) (err error) {
// run command replacing {{PORTS}} with the ports (space separated)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any better suggestion than using CSV to seperate values? That could be added in #1113 !

internal/portforward/service/settings.go Show resolved Hide resolved
internal/portforward/service/start.go Show resolved Hide resolved
"strings"
)

func (s *Service) runUpCommand(ctx context.Context, ports []uint16) (err error) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll add a test to make sure for example this works: /bin/sh -c "echo {{PORTS}}" - given it plugs arguments by splitting them by spaces, I'm not sure it would handle that case with double quotes correctly. Will check!

@@ -29,6 +29,11 @@ type PortForwarding struct {
// to write to a file. It cannot be nil for the
// internal state
Filepath *string `json:"status_file_path"`
// Command is the port forwarding status command
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Self comment to self-resolve: don't forget to merge PR qdm12/gluetun-wiki#89 !

@qdm12 qdm12 added the Status: 🟡 Nearly resolved This might be resolved or is about to be resolved label Aug 17, 2024
@qdm12
Copy link
Owner

qdm12 commented Aug 21, 2024

FYI I'm working on your branch right now, almost done rebasing+fixing the last feedback I left, so please hold 😉 Thanks!!

@qdm12
Copy link
Owner

qdm12 commented Aug 22, 2024

Hi @lavalleeale could you give me (force) push access to your Gluetun branch master? I'm done with the changes, just waiting to force push them there.

@thibautd
Copy link

thibautd commented Sep 2, 2024

Thanks for working on this, can't wait !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature request: Webhook call when forwarded port change Feature request: run a script on interface up/down
3 participants