feat(publicip): PUBLICIP_ENABLED replaces PUBLICIP_PERIOD

- No point periodically fetch the public IP address. Could not find anything mentioning why this was added.
- Simplification of the publicip loop code
- `PUBLICIP_ENABLED` (on, off) can be set to enable or not public ip data fetching on VPN connection
- `PUBLICIP_PERIOD=0` still works to indicate to disable public ip fetching
- `PUBLICIP_PERIOD` != 0 means to enable public ip fetching
- Warnings logged when using `PUBLICIP_PERIOD`

Dockerfile

@@ -204,7 +204,7 @@ ENV VPN_SERVICE_PROVIDER=pia \
     UPDATER_VPN_SERVICE_PROVIDERS= \
     # Public IP
     PUBLICIP_FILE="/tmp/gluetun/ip" \
-    PUBLICIP_PERIOD=12h \
+    PUBLICIP_ENABLED=on \
     PUBLICIP_API=ipinfo \
     PUBLICIP_API_TOKEN= \
     # Storage

internal/configuration/settings/errors.go

@@ -30,7 +30,6 @@ var (
 	ErrPortForwardingEnabled           = errors.New("port forwarding cannot be enabled")
 	ErrPortForwardingUserEmpty         = errors.New("port forwarding username is empty")
 	ErrPortForwardingPasswordEmpty     = errors.New("port forwarding password is empty")
-	ErrPublicIPPeriodTooShort          = errors.New("public IP address check period is too short")
 	ErrRegionNotValid                  = errors.New("the region specified is not valid")
 	ErrServerAddressNotValid           = errors.New("server listening address is not valid")
 	ErrSystemPGIDNotValid              = errors.New("process group id is not valid")

internal/configuration/settings/publicip.go

@@ -3,7 +3,6 @@ package settings
 import (
 	"fmt"
 	"path/filepath"
-	"time"
 
 	"github.com/qdm12/gluetun/internal/publicip/api"
 	"github.com/qdm12/gosettings"
@@ -13,11 +12,9 @@ import (
 
 // PublicIP contains settings for port forwarding.
 type PublicIP struct {
-	// Period is the period to get the public IP address.
-	// It can be set to 0 to disable periodic checking.
-	// It cannot be nil for the internal state.
-	// TODO change to value and add enabled field
-	Period *time.Duration
+	// Enabled is set to true to fetch the public ip address
+	// information on VPN connection. It defaults to true.
+	Enabled *bool
 	// IPFilepath is the public IP address status file path
 	// to use. It can be the empty string to indicate not
 	// to write to a file. It cannot be nil for the
@@ -48,12 +45,6 @@ func (p PublicIP) UpdateWith(partialUpdate PublicIP) (updatedSettings PublicIP,
 }
 
 func (p PublicIP) validate() (err error) {
-	const minPeriod = 5 * time.Second
-	if *p.Period < minPeriod {
-		return fmt.Errorf("%w: %s must be at least %s",
-			ErrPublicIPPeriodTooShort, p.Period, minPeriod)
-	}
-
 	if *p.IPFilepath != "" { // optional
 		_, err := filepath.Abs(*p.IPFilepath)
 		if err != nil {
@@ -71,7 +62,7 @@ func (p PublicIP) validate() (err error) {
 
 func (p *PublicIP) copy() (copied PublicIP) {
 	return PublicIP{
-		Period:     gosettings.CopyPointer(p.Period),
+		Enabled:    gosettings.CopyPointer(p.Enabled),
 		IPFilepath: gosettings.CopyPointer(p.IPFilepath),
 		API:        p.API,
 		APIToken:   gosettings.CopyPointer(p.APIToken),
@@ -79,15 +70,14 @@ func (p *PublicIP) copy() (copied PublicIP) {
 }
 
 func (p *PublicIP) overrideWith(other PublicIP) {
-	p.Period = gosettings.OverrideWithPointer(p.Period, other.Period)
+	p.Enabled = gosettings.OverrideWithPointer(p.Enabled, other.Enabled)
 	p.IPFilepath = gosettings.OverrideWithPointer(p.IPFilepath, other.IPFilepath)
 	p.API = gosettings.OverrideWithComparable(p.API, other.API)
 	p.APIToken = gosettings.OverrideWithPointer(p.APIToken, other.APIToken)
 }
 
 func (p *PublicIP) setDefaults() {
-	const defaultPeriod = 12 * time.Hour
-	p.Period = gosettings.DefaultPointer(p.Period, defaultPeriod)
+	p.Enabled = gosettings.DefaultPointer(p.Enabled, true)
 	p.IPFilepath = gosettings.DefaultPointer(p.IPFilepath, "/tmp/gluetun/ip")
 	p.API = gosettings.DefaultComparable(p.API, "ipinfo")
 	p.APIToken = gosettings.DefaultPointer(p.APIToken, "")
@@ -98,19 +88,12 @@ func (p PublicIP) String() string {
 }
 
 func (p PublicIP) toLinesNode() (node *gotree.Node) {
+	if !*p.Enabled {
+		return gotree.New("Public IP settings: disabled")
+	}
+
 	node = gotree.New("Public IP settings:")
 
-	if *p.Period == 0 {
-		node.Appendf("Enabled: no")
-		return node
-	}
-
-	updatePeriod := "disabled"
-	if *p.Period > 0 {
-		updatePeriod = "every " + p.Period.String()
-	}
-	node.Appendf("Fetching: %s", updatePeriod)
-
 	if *p.IPFilepath != "" {
 		node.Appendf("IP file path: %s", *p.IPFilepath)
 	}
@@ -124,8 +107,8 @@ func (p PublicIP) toLinesNode() (node *gotree.Node) {
 	return node
 }
 
-func (p *PublicIP) read(r *reader.Reader) (err error) {
-	p.Period, err = r.DurationPtr("PUBLICIP_PERIOD")
+func (p *PublicIP) read(r *reader.Reader, warner Warner) (err error) {
+	p.Enabled, err = readPublicIPEnabled(r, warner)
 	if err != nil {
 		return err
 	}
@@ -136,3 +119,23 @@ func (p *PublicIP) read(r *reader.Reader) (err error) {
 	p.APIToken = r.Get("PUBLICIP_API_TOKEN")
 	return nil
 }
+
+func readPublicIPEnabled(r *reader.Reader, warner Warner) (
+	enabled *bool, err error) {
+	periodPtr, err := r.DurationPtr("PUBLICIP_PERIOD") // Retro-compatibility
+	if err != nil {
+		return nil, err
+	} else if periodPtr == nil {
+		return r.BoolPtr("PUBLICIP_ENABLED")
+	}
+
+	if *periodPtr == 0 {
+		warner.Warn("please replace PUBLICIP_PERIOD=0 with PUBLICIP_ENABLED=no")
+		return ptrTo(false), nil
+	}
+
+	warner.Warn("PUBLICIP_PERIOD is no longer used. " +
+		"It is assumed from its non-zero value you want PUBLICIP_ENABLED=yes. " +
+		"Please migrate to use PUBLICIP_ENABLED only in the future.")
+	return ptrTo(true), nil
+}

internal/configuration/settings/settings.go

@@ -197,14 +197,16 @@ func (s *Settings) Read(r *reader.Reader, warner Warner) (err error) {
 		"health":         s.Health.Read,
 		"http proxy":     s.HTTPProxy.read,
 		"log":            s.Log.read,
-		"public ip":      s.PublicIP.read,
-		"shadowsocks":    s.Shadowsocks.read,
-		"storage":        s.Storage.read,
-		"system":         s.System.read,
-		"updater":        s.Updater.read,
-		"version":        s.Version.read,
-		"VPN":            s.VPN.read,
-		"profiling":      s.Pprof.Read,
+		"public ip": func(r *reader.Reader) error {
+			return s.PublicIP.read(r, warner)
+		},
+		"shadowsocks": s.Shadowsocks.read,
+		"storage":     s.Storage.read,
+		"system":      s.System.read,
+		"updater":     s.Updater.read,
+		"version":     s.Version.read,
+		"VPN":         s.VPN.read,
+		"profiling":   s.Pprof.Read,
 	}
 
 	for name, read := range readFunctions {

internal/configuration/settings/settings_test.go

@@ -78,7 +78,6 @@ func Test_Settings_String(t *testing.T) {
 |   ├── Process UID: 1000
 |   └── Process GID: 1000
 ├── Public IP settings:
-|   ├── Fetching: every 12h0m0s
 |   ├── IP file path: /tmp/gluetun/ip
 |   └── Public IP data API: ipinfo
 └── Version settings:

internal/portforward/service/start.go

@@ -79,9 +79,12 @@ func (s *Service) Start(ctx context.Context) (runError <-chan error, err error)
 	keepPortDoneCh := make(chan struct{})
 	s.keepPortDoneCh = keepPortDoneCh
 
+	readyCh := make(chan struct{})
 	go func(ctx context.Context, portForwarder PortForwarder,
-		obj utils.PortForwardObjects, runError chan<- error, doneCh chan<- struct{}) {
+		obj utils.PortForwardObjects, readyCh chan<- struct{},
+		runError chan<- error, doneCh chan<- struct{}) {
 		defer close(doneCh)
+		close(readyCh)
 		err = portForwarder.KeepPortForward(ctx, obj)
 		crashed := ctx.Err() == nil
 		if !crashed { // stopped by Stop call
@@ -91,7 +94,8 @@ func (s *Service) Start(ctx context.Context) (runError <-chan error, err error)
 		defer s.startStopMutex.Unlock()
 		_ = s.cleanup()
 		runError <- err
-	}(keepPortCtx, s.settings.PortForwarder, obj, runErrorCh, keepPortDoneCh)
+	}(keepPortCtx, s.settings.PortForwarder, obj, readyCh, runErrorCh, keepPortDoneCh)
+	<-readyCh
 
 	return runErrorCh, nil
 }

internal/publicip/loop.go

@@ -76,14 +76,8 @@ func (l *Loop) run(runCtx context.Context, runDone chan<- struct{},
 	updateTrigger <-chan settings.PublicIP, updatedResult chan<- error) {
 	defer close(runDone)
 
-	timer := time.NewTimer(time.Hour)
-	defer timer.Stop()
-	_ = timer.Stop()
-	timerIsReadyToReset := true
-	lastFetch := time.Unix(0, 0)
-
 	for {
-		singleRunCtx := runCtx
+		var singleRunCtx context.Context
 		var singleRunResult chan<- error
 		select {
 		case <-runCtx.Done():
@@ -91,26 +85,17 @@ func (l *Loop) run(runCtx context.Context, runDone chan<- struct{},
 		case singleRunCtx = <-runTrigger:
 			// Note singleRunCtx is canceled if runCtx is canceled.
 			singleRunResult = runResult
-		case <-timer.C:
-			timerIsReadyToReset = true
 		case partialUpdate := <-updateTrigger:
 			var err error
-			timerIsReadyToReset, err = l.update(partialUpdate, lastFetch, timer, timerIsReadyToReset)
+			err = l.update(partialUpdate)
 			updatedResult <- err
 			continue
 		}
 
-		lastFetch = l.timeNow()
-		timerIsReadyToReset = l.updateTimer(*l.settings.Period, lastFetch, timer, timerIsReadyToReset)
-
 		result, err := l.fetcher.FetchInfo(singleRunCtx, netip.Addr{})
 		if err != nil {
 			err = fmt.Errorf("fetching information: %w", err)
-			if singleRunResult != nil {
-				singleRunResult <- err
-			} else {
-				l.logger.Error(err.Error())
-			}
+			singleRunResult <- err
 			continue
 		}
 
@@ -128,11 +113,7 @@ func (l *Loop) run(runCtx context.Context, runDone chan<- struct{},
 			err = fmt.Errorf("persisting public ip address: %w", err)
 		}
 
-		if singleRunResult != nil {
-			singleRunResult <- err
-		} else if err != nil {
-			l.logger.Error(err.Error())
-		}
+		singleRunResult <- err
 	}
 }
 

internal/publicip/update.go

@@ -3,25 +3,16 @@ package publicip
 import (
 	"fmt"
 	"os"
-	"time"
 
 	"github.com/qdm12/gluetun/internal/configuration/settings"
 )
 
-func (l *Loop) update(partialUpdate settings.PublicIP,
-	lastTick time.Time, timer *time.Timer, timerIsReadyToReset bool) (
-	newTimerIsReadyToReset bool, err error) {
-	newTimerIsReadyToReset = timerIsReadyToReset
+func (l *Loop) update(partialUpdate settings.PublicIP) (err error) {
 	// No need to lock the mutex since it can only be written
 	// in the code below in this goroutine.
 	updatedSettings, err := l.settings.UpdateWith(partialUpdate)
 	if err != nil {
-		return newTimerIsReadyToReset, err
-	}
-
-	if *l.settings.Period != *updatedSettings.Period {
-		newTimerIsReadyToReset = l.updateTimer(*updatedSettings.Period, lastTick,
-			timer, timerIsReadyToReset)
+		return err
 	}
 
 	if *l.settings.IPFilepath != *updatedSettings.IPFilepath {
@@ -30,17 +21,17 @@ func (l *Loop) update(partialUpdate settings.PublicIP,
 			err = persistPublicIP(*updatedSettings.IPFilepath,
 				l.ipData.IP.String(), l.puid, l.pgid)
 			if err != nil {
-				return newTimerIsReadyToReset, fmt.Errorf("persisting ip data: %w", err)
+				return fmt.Errorf("persisting ip data: %w", err)
 			}
 		case *updatedSettings.IPFilepath == "":
 			err = os.Remove(*l.settings.IPFilepath)
 			if err != nil {
-				return newTimerIsReadyToReset, fmt.Errorf("removing ip data file path: %w", err)
+				return fmt.Errorf("removing ip data file path: %w", err)
 			}
 		default:
 			err = os.Rename(*l.settings.IPFilepath, *updatedSettings.IPFilepath)
 			if err != nil {
-				return newTimerIsReadyToReset, fmt.Errorf("renaming ip data file path: %w", err)
+				return fmt.Errorf("renaming ip data file path: %w", err)
 			}
 		}
 	}
@@ -49,34 +40,5 @@ func (l *Loop) update(partialUpdate settings.PublicIP,
 	l.settings = updatedSettings
 	l.settingsMutex.Unlock()
 
-	return newTimerIsReadyToReset, nil
-}
-
-func (l *Loop) updateTimer(period time.Duration, lastFetch time.Time,
-	timer *time.Timer, timerIsReadyToReset bool) (newTimerIsReadyToReset bool) {
-	disableTimer := period == 0
-	if disableTimer {
-		if !timer.Stop() {
-			<-timer.C
-		}
-		return true
-	}
-
-	if !timerIsReadyToReset {
-		if !timer.Stop() {
-			<-timer.C
-		}
-	}
-
-	var waited time.Duration
-	if lastFetch.UnixNano() > 0 {
-		waited = l.timeNow().Sub(lastFetch)
-	}
-	leftToWait := period - waited
-	if leftToWait <= 0 {
-		leftToWait = time.Nanosecond
-	}
-
-	timer.Reset(leftToWait)
-	return false
+	return nil
 }