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.

Sign up for GitHub

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

servercfg.GetPublicIP fixes and unit tests #2616

Closed
wants to merge 8 commits into from
Closed

servercfg.GetPublicIP fixes and unit tests #2616

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
574e2ff
Fix: errors in GetPublicIP ignored, err is always nil
yunginnanet Oct 8, 2023
bf77715
Feat[testing]: Add unit tests for servercfg.GetPublicIP
yunginnanet Oct 8, 2023
6b60dc0
Chore: gofmt
yunginnanet Oct 8, 2023
93fdc71
Chore: trim comment
yunginnanet Oct 8, 2023
13978d7
Testing: Assert we fixed the bug in https://github.com/gravitl/netmak…
yunginnanet Oct 8, 2023
90849ec
Merge branch 'develop' into kayos-serverconf
yunginnanet Oct 11, 2023
56e59a1
Merge branch 'develop' into kayos-serverconf
yunginnanet Oct 19, 2023
6ab5707
Merge branch 'develop' into kayos-serverconf
yunginnanet Oct 26, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 25 additions & 6 deletions servercfg/serverconf.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -479,17 +479,36 @@ func GetPublicIP() (string, error) {
iplist = append([]string{publicIpService}, iplist...)
}

// regular timeout
timeoutTarget := time.Duration(10) * time.Second

// our unit test sets a bad ip service to test the timeout and fallback mechanics
// if that env variable is set, lower the timeout so the test doesn't take forever
testCheck := os.Getenv("NETMAKER_TEST_BAD_IP_SERVICE")
Copy link
Member

Choose a reason for hiding this comment

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

tests shouldn't part of the main code logic

if testCheck != "" {
// we also don't really need to be checking all the services in the list after the planned timeout
// we just need to make sure that it falls back to the following service, which will be a mock
iplist = iplist[:1]
iplist = append(iplist, os.Getenv("NETMAKER_TEST_IP_SERVICE"))
}

for _, ipserver := range iplist {
if testCheck != "" && strings.EqualFold(testCheck, ipserver) {
timeoutTarget = time.Duration(2) * time.Second
}

client := &http.Client{
Timeout: time.Second * 10,
Timeout: timeoutTarget,
}
resp, err := client.Get(ipserver)
var resp *http.Response
resp, err = client.Get(ipserver)
if err != nil {
continue
}
defer resp.Body.Close()
if resp.StatusCode == http.StatusOK {
bodyBytes, err := io.ReadAll(resp.Body)
var bodyBytes []byte
Copy link
Contributor

Choose a reason for hiding this comment

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

no need to declare a var here bodyBytes, err :=io.ReadAll(resp.Body)

Copy link
Contributor Author

@yunginnanet yunginnanet Oct 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Well... this actually starts to touch on the bug that I'm fixing in the first place. The problem here is that we were checking for a non nil err afterwards that was... always nil... what I'm doing here is making sure that we are propagating any errors within the for loop to that err we declared in our encompassing scope.

Previously we were declaring a fresh err variable in our local scope, meaning that the original err was always going to be nil.

do note that yes of course that error may be replaced by another error within the for loop, but the way I see it is it's a hell of a lot better than just not knowing what happened (or even that it had an error at all).

In a function like this i figure as long as I get the value of one of the errors that's likely enough for me to troubleshoot the problem.

As long as the last IP endpoint we tried worked, we should get a nil error value and go about our business.

Copy link
Contributor

Choose a reason for hiding this comment

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

the non-nil check after completing the loop is not required. It does not provide any additional information. Once the loop is completed, if the endpoint string is still empty, the error "public address not found" should be returned.

Copy link
Contributor Author

@yunginnanet yunginnanet Oct 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

okay, so wouldn't we like to know.. why? as my unit test shows, with this PR, we do (at least in regard to the error this function returns)

I'd like to know the reason that this function failed if it failed. even if it only shows me the last error received, chances are high that they were all the same error, and still much better than "He's dead, Jim." with no additional information.

Are you saying we're not handling the error from this function? if so then that'd be what I'll attack next.

Copy link
Contributor

Choose a reason for hiding this comment

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

for the most part, this error is ignored by calling functions. In the one case where it is checked, an err will result in a fatalLog.

if we need the actual error for troubleshooting then just add a slog.Debug() when the error occurs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah that fatalLog is exactly what prompted this fix, because it was a fatality that gave no information.. sure tho

bodyBytes, err = io.ReadAll(resp.Body)
if err != nil {
continue
}
Expand Down Expand Up @@ -619,7 +638,7 @@ func GetEmqxRestEndpoint() string {

// IsBasicAuthEnabled - checks if basic auth has been configured to be turned off
func IsBasicAuthEnabled() bool {
var enabled = true //default
var enabled = true // default
if os.Getenv("BASIC_AUTH") != "" {
enabled = os.Getenv("BASIC_AUTH") == "yes"
} else if config.Config.Server.BasicAuth != "" {
Expand Down Expand Up @@ -648,7 +667,7 @@ func GetNetmakerTenantID() string {

// GetStunPort - Get the port to run the stun server on
func GetStunPort() int {
port := 3478 //default
port := 3478 // default
if os.Getenv("STUN_PORT") != "" {
portInt, err := strconv.Atoi(os.Getenv("STUN_PORT"))
if err == nil {
Expand All @@ -662,7 +681,7 @@ func GetStunPort() int {

// GetTurnPort - Get the port to run the turn server on
func GetTurnPort() int {
port := 3479 //default
port := 3479 // default
if os.Getenv("TURN_PORT") != "" {
portInt, err := strconv.Atoi(os.Getenv("TURN_PORT"))
if err == nil {
Expand Down
116 changes: 116 additions & 0 deletions servercfg/serverconf_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,116 @@
package servercfg

import (
"context"
"net/http"
"net/http/httptest"
"os"
"strings"
"testing"
"time"

"github.com/gravitl/netmaker/config"
)

func TestGetPublicIP(t *testing.T) {
var testIP = "55.55.55.55"
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
if _, err := w.Write([]byte(testIP)); err != nil {
t.Errorf("expected no error, got %v", err)
}
}))
defer server.Close()
if err := os.Setenv("NETMAKER_TEST_IP_SERVICE", server.URL); err != nil {
t.Logf("WARNING: could not set NETMAKER_TEST_IP_SERVICE env var")
}

t.Run("Use PUBLIC_IP_SERVICE if set", func(t *testing.T) {

// set the environment variable
if err := os.Setenv("PUBLIC_IP_SERVICE", server.URL); err != nil {
t.Fatalf("expected no error, got %v", err)
}
defer func() {
_ = os.Unsetenv("PUBLIC_IP_SERVICE")
}()

var ip string
var err error
if ip, err = GetPublicIP(); err != nil {
t.Fatalf("expected no error, got %v", err)
}
if !strings.EqualFold(ip, testIP) {
t.Errorf("expected IP to be %s, got %s", testIP, ip)
}
})

t.Run("Use config.Config.Server.PublicIPService if PUBLIC_IP_SERVICE isn't set", func(t *testing.T) {
config.Config.Server.PublicIPService = server.URL
defer func() { config.Config.Server.PublicIPService = "" }()

ip, err := GetPublicIP()
if err != nil {
t.Fatalf("expected no error, got %v", err)
}
if ip != testIP {
t.Fatalf("expected IP to be %s, got %s", testIP, ip)
}
})

t.Run("Handle service timeout", func(t *testing.T) {
if os.Getenv("NETMAKER_TEST_IP_SERVICE") == "" {
t.Skip("NETMAKER_TEST_IP_SERVICE not set")
}

var badTestIP = "123.45.67.91"
badServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
// intentionally delay to simulate a timeout
time.Sleep(3 * time.Second)
_, _ = w.Write([]byte(badTestIP))
}))
defer badServer.Close()

// we set this so that we can lower the timeout for the test to not hold up CI
if err := os.Setenv("NETMAKER_TEST_BAD_IP_SERVICE", badServer.URL); err != nil {
// but if we can't set it, we can't test it
t.Skip("failed to set NETMAKER_TEST_BAD_IP_SERVICE, skipping test because we won't timeout")
}
defer func() {
_ = os.Unsetenv("NETMAKER_TEST_BAD_IP_SERVICE")
}()

// mock the config
oldConfig := config.Config.Server.PublicIPService
config.Config.Server.PublicIPService = badServer.URL
defer func() { config.Config.Server.PublicIPService = oldConfig }()

res, err := GetPublicIP()
if err != nil {
t.Errorf("GetPublicIP() fallback has failed: %v", err)
}
if strings.EqualFold(res, badTestIP) {
t.Errorf("GetPublicIP() returned the response from the server that should have timed out: %v", res)
}
if !strings.EqualFold(res, testIP) {
t.Errorf("GetPublicIP() did not fallback to the correct IP: %v", res)
}

t.Run("Assert error is passed down", func(t *testing.T) {
oldConfig = config.Config.Server.PublicIPService
oldEnv := os.Getenv("NETMAKER_TEST_IP_SERVICE")

// make sure that even the fallback fails
if err = os.Setenv("NETMAKER_TEST_IP_SERVICE", badServer.URL); err != nil {
t.Skipf("could not set NETMAKER_TEST_IP_SERVICE env var")
}
defer func() { _ = os.Setenv("NETMAKER_TEST_IP_SERVICE", oldEnv) }()

// https://github.com/golang/go/issues/63445
// if _, err = GetPublicIP(); !errors.Is(err, context.DeadlineExceeded) {
if _, err = GetPublicIP(); err == nil || !strings.Contains(err.Error(), "context deadline exceeded") {
t.Errorf("expected error to be %v, got %v", context.DeadlineExceeded, err)
}
})
})

}
Loading