From ef912eaec38fad87769b7b019bea127e9de9158a Mon Sep 17 00:00:00 2001 From: Marcin Niemira Date: Thu, 29 Aug 2019 22:07:23 +1000 Subject: [PATCH 1/5] Improve test coverage for proxy package --- pkg/minikube/proxy/proxy.go | 5 ++ pkg/minikube/proxy/proxy_test.go | 111 +++++++++++++++++++++++++++++++ 2 files changed, 116 insertions(+) diff --git a/pkg/minikube/proxy/proxy.go b/pkg/minikube/proxy/proxy.go index 9d911b5649..ce67b4920a 100644 --- a/pkg/minikube/proxy/proxy.go +++ b/pkg/minikube/proxy/proxy.go @@ -57,6 +57,11 @@ func isInBlock(ip string, block string) (bool, error) { // ExcludeIP will exclude the ip from the http(s)_proxy func ExcludeIP(ip string) error { + if netIp := net.ParseIP(ip); netIp == nil { + if _, _, err := net.ParseCIDR(ip); err != nil { + return fmt.Errorf("ExcludeIP(%v) requires IP as a parameter or CIDR", ip) + } + } return updateEnv(ip, "NO_PROXY") } diff --git a/pkg/minikube/proxy/proxy_test.go b/pkg/minikube/proxy/proxy_test.go index 25dccce10d..508e82515f 100644 --- a/pkg/minikube/proxy/proxy_test.go +++ b/pkg/minikube/proxy/proxy_test.go @@ -18,8 +18,11 @@ package proxy import ( "fmt" + "net/http" "os" "testing" + + "k8s.io/client-go/rest" ) func TestIsValidEnv(t *testing.T) { @@ -55,6 +58,7 @@ func TestIsInBlock(t *testing.T) { {"192.168.0.2", "192.168.0.1/32", false, false}, {"192.168.0.1", "192.168.0.1/18", true, false}, {"abcd", "192.168.0.1/18", false, true}, + {"192.168.0.1", "foo", false, true}, } for _, tc := range testCases { t.Run(fmt.Sprintf("%s in %s", tc.ip, tc.block), func(t *testing.T) { @@ -124,6 +128,7 @@ func TestCheckEnv(t *testing.T) { {"192.168.0.13", "NO_PROXY", true, "192.168.0.13/22"}, {"192.168.0.13", "NO_PROXY", true, "10.10.0.13,192.168.0.13"}, {"192.168.0.13", "NO_PROXY", false, "10.10.0.13/22"}, + {"10.10.10.4", "NO_PROXY", true, "172.168.0.0/30,10.10.10.0/24"}, } for _, tc := range testCases { t.Run(fmt.Sprintf("%s in %s", tc.ip, tc.envName), func(t *testing.T) { @@ -149,3 +154,109 @@ func TestCheckEnv(t *testing.T) { } } + +func TestIsIPExcluded(t *testing.T) { + var testCases = []struct { + ip, env string + excluded bool + }{ + {"1.2.3.4", "7.7.7.7", false}, + {"1.2.3.4", "1.2.3.4", true}, + {"1.2.3.4", "", false}, + {"foo", "", false}, + {"foo", "bar", false}, + {"foo", "1.2.3.4", false}, + } + for _, tc := range testCases { + t.Run(fmt.Sprintf("exclude %s NO_PROXY(%v)", tc.ip, tc.env), func(t *testing.T) { + originalEnv := os.Getenv("NO_PROXY") + defer func() { // revert to pre-test env var + err := os.Setenv("NO_PROXY", originalEnv) + if err != nil { + t.Fatalf("Error reverting env NO_PROXY to its original value (%s) var after test ", originalEnv) + } + }() + if err := os.Setenv("NO_PROXY", tc.env); err != nil { + t.Errorf("Error during setting env: %v", err) + } + if excluded := IsIPExcluded(tc.ip); excluded != tc.excluded { + t.Fatalf("IsIPExcluded(%v) should return %v. NO_PROXY=%v", tc.ip, tc.excluded, tc.env) + } + }) + } +} + +func TestExcludeIP(t *testing.T) { + var testCases = []struct { + ip, env string + wantAErr bool + }{ + {"1.2.3.4", "", false}, + {"", "", true}, + {"7.7.7.7", "7.7.7.7", false}, + {"7.7.7.7", "1.2.3.4", false}, + {"foo", "", true}, + {"foo", "1.2.3.4", true}, + } + for _, tc := range testCases { + t.Run(fmt.Sprintf("exclude %s NO_PROXY(%s)", tc.ip, tc.env), func(t *testing.T) { + originalEnv := os.Getenv("NO_PROXY") + defer func() { // revert to pre-test env var + err := os.Setenv("NO_PROXY", originalEnv) + if err != nil { + t.Fatalf("Error reverting env NO_PROXY to its original value (%s) var after test ", originalEnv) + } + }() + if err := os.Setenv("NO_PROXY", tc.env); err != nil { + t.Errorf("Error during setting env: %v", err) + } + err := ExcludeIP(tc.ip) + if err != nil && !tc.wantAErr { + t.Errorf("ExcludeIP(%v) returned unexpected error %v", tc.ip, err) + } + if err == nil && tc.wantAErr { + t.Errorf("ExcludeIP(%v) should return error but error is %v", tc.ip, err) + } + }) + } +} + +func TestUpdateTransport(t *testing.T) { + t.Run("new", func(t *testing.T) { + rc := rest.Config{} + c := UpdateTransport(&rc) + tr := &http.Transport{} + tr.RegisterProtocol("file", http.NewFileTransport(http.Dir("/tmp"))) + rt := c.WrapTransport(tr) + if _, ok := rt.(http.RoundTripper); !ok { + t.Fatalf("Cannot cast rt(%v) to http.RoundTripper", rt) + } + }) + t.Run("existing", func(t *testing.T) { + // rest config initialized with WrapTransport function + rc := rest.Config{WrapTransport: func(http.RoundTripper) http.RoundTripper { + rt := &http.Transport{} + rt.RegisterProtocol("file", http.NewFileTransport(http.Dir("/tmp"))) + return rt + }} + + transport := &http.Transport{} + transport.RegisterProtocol("file", http.NewFileTransport(http.Dir("/"))) + + c := UpdateTransport(&rc) + rt := c.WrapTransport(nil) + + if rt == rc.WrapTransport(transport) { + t.Fatalf("Excpected to reuse existing RoundTripper(%v) but found %v", rt, transport) + } + + }) + t.Run("nil", func(t *testing.T) { + rc := rest.Config{} + c := UpdateTransport(&rc) + rt := c.WrapTransport(nil) + if rt != nil { + t.Fatalf("Expected RoundTripper nil for invocation WrapTransport(nil)") + } + }) +} From d4ac66eac515d5849610596035eaf5993b6435f6 Mon Sep 17 00:00:00 2001 From: Marcin Niemira Date: Thu, 29 Aug 2019 23:57:20 +1000 Subject: [PATCH 2/5] fix variable name --- pkg/minikube/proxy/proxy.go | 4 ++-- pkg/minikube/proxy/proxy_test.go | 12 +++--------- 2 files changed, 5 insertions(+), 11 deletions(-) diff --git a/pkg/minikube/proxy/proxy.go b/pkg/minikube/proxy/proxy.go index ce67b4920a..bdcc530880 100644 --- a/pkg/minikube/proxy/proxy.go +++ b/pkg/minikube/proxy/proxy.go @@ -57,9 +57,9 @@ func isInBlock(ip string, block string) (bool, error) { // ExcludeIP will exclude the ip from the http(s)_proxy func ExcludeIP(ip string) error { - if netIp := net.ParseIP(ip); netIp == nil { + if netIP := net.ParseIP(ip); netIP == nil { if _, _, err := net.ParseCIDR(ip); err != nil { - return fmt.Errorf("ExcludeIP(%v) requires IP as a parameter or CIDR", ip) + return fmt.Errorf("ExcludeIP(%v) requires IP or CIDR as a parameter", ip) } } return updateEnv(ip, "NO_PROXY") diff --git a/pkg/minikube/proxy/proxy_test.go b/pkg/minikube/proxy/proxy_test.go index 508e82515f..ab56d93614 100644 --- a/pkg/minikube/proxy/proxy_test.go +++ b/pkg/minikube/proxy/proxy_test.go @@ -36,11 +36,9 @@ func TestIsValidEnv(t *testing.T) { } for _, tc := range testCases { t.Run(tc.env, func(t *testing.T) { - got := isValidEnv(tc.env) - if got != tc.want { + if got := isValidEnv(tc.env); got != tc.want { t.Errorf("isValidEnv(\"%v\") got %v; want %v", tc.env, got, tc.want) } - }) } @@ -140,16 +138,12 @@ func TestCheckEnv(t *testing.T) { } }() - // defer os.Setenv(tc.envName, originalEnv) - err := os.Setenv(tc.envName, tc.mockEnvValue) // setting up the test case - if err != nil { + if err := os.Setenv(tc.envName, tc.mockEnvValue); err != nil { t.Error("Error setting env var for taste case") } - got := checkEnv(tc.ip, tc.envName) - if got != tc.want { + if got := checkEnv(tc.ip, tc.envName); got != tc.want { t.Errorf("CheckEnv(%v,%v) got %v ; want is %v", tc.ip, tc.envName, got, tc.want) } - }) } From 8a022356682339b00c940f6d5523a51d2f25fb30 Mon Sep 17 00:00:00 2001 From: Marcin Niemira Date: Fri, 30 Aug 2019 00:02:23 +1000 Subject: [PATCH 3/5] Improve subtests names --- pkg/minikube/proxy/proxy_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/minikube/proxy/proxy_test.go b/pkg/minikube/proxy/proxy_test.go index ab56d93614..5e59d2355e 100644 --- a/pkg/minikube/proxy/proxy_test.go +++ b/pkg/minikube/proxy/proxy_test.go @@ -59,7 +59,7 @@ func TestIsInBlock(t *testing.T) { {"192.168.0.1", "foo", false, true}, } for _, tc := range testCases { - t.Run(fmt.Sprintf("%s in %s", tc.ip, tc.block), func(t *testing.T) { + t.Run(fmt.Sprintf("%s in %s Want: %t WantAErr: %t", tc.ip, tc.block, tc.want, tc.wanntAErr), func(t *testing.T) { got, err := isInBlock(tc.ip, tc.block) gotErr := false if err != nil { @@ -90,7 +90,7 @@ func TestUpdateEnv(t *testing.T) { {"192.168.0.13", "NPROXY", true}, } for _, tc := range testCases { - t.Run(fmt.Sprintf("%s in %s", tc.ip, tc.env), func(t *testing.T) { + t.Run(fmt.Sprintf("%s in %s WantAErr: %t", tc.ip, tc.env, tc.wantErr), func(t *testing.T) { origVal := os.Getenv(tc.env) gotErr := false err := updateEnv(tc.ip, tc.env) From 6dac0467c7f054611d7c9a6c18e25ace2c9782d5 Mon Sep 17 00:00:00 2001 From: Marcin Niemira Date: Fri, 30 Aug 2019 00:07:13 +1000 Subject: [PATCH 4/5] improve function comment and move env var restoration to main test --- pkg/minikube/proxy/proxy.go | 2 +- pkg/minikube/proxy/proxy_test.go | 28 ++++++++++++++-------------- 2 files changed, 15 insertions(+), 15 deletions(-) diff --git a/pkg/minikube/proxy/proxy.go b/pkg/minikube/proxy/proxy.go index bdcc530880..97753431de 100644 --- a/pkg/minikube/proxy/proxy.go +++ b/pkg/minikube/proxy/proxy.go @@ -55,7 +55,7 @@ func isInBlock(ip string, block string) (bool, error) { return false, errors.Wrapf(err, "Error ip not in block") } -// ExcludeIP will exclude the ip from the http(s)_proxy +// ExcludeIP takes ip or CIDR as string and excludes the it from the http(s)_proxy func ExcludeIP(ip string) error { if netIP := net.ParseIP(ip); netIP == nil { if _, _, err := net.ParseCIDR(ip); err != nil { diff --git a/pkg/minikube/proxy/proxy_test.go b/pkg/minikube/proxy/proxy_test.go index 5e59d2355e..17f62e2a86 100644 --- a/pkg/minikube/proxy/proxy_test.go +++ b/pkg/minikube/proxy/proxy_test.go @@ -162,14 +162,14 @@ func TestIsIPExcluded(t *testing.T) { {"foo", "1.2.3.4", false}, } for _, tc := range testCases { + originalEnv := os.Getenv("NO_PROXY") + defer func() { // revert to pre-test env var + err := os.Setenv("NO_PROXY", originalEnv) + if err != nil { + t.Fatalf("Error reverting env NO_PROXY to its original value (%s) var after test ", originalEnv) + } + }() t.Run(fmt.Sprintf("exclude %s NO_PROXY(%v)", tc.ip, tc.env), func(t *testing.T) { - originalEnv := os.Getenv("NO_PROXY") - defer func() { // revert to pre-test env var - err := os.Setenv("NO_PROXY", originalEnv) - if err != nil { - t.Fatalf("Error reverting env NO_PROXY to its original value (%s) var after test ", originalEnv) - } - }() if err := os.Setenv("NO_PROXY", tc.env); err != nil { t.Errorf("Error during setting env: %v", err) } @@ -192,15 +192,15 @@ func TestExcludeIP(t *testing.T) { {"foo", "", true}, {"foo", "1.2.3.4", true}, } + originalEnv := os.Getenv("NO_PROXY") + defer func() { // revert to pre-test env var + err := os.Setenv("NO_PROXY", originalEnv) + if err != nil { + t.Fatalf("Error reverting env NO_PROXY to its original value (%s) var after test ", originalEnv) + } + }() for _, tc := range testCases { t.Run(fmt.Sprintf("exclude %s NO_PROXY(%s)", tc.ip, tc.env), func(t *testing.T) { - originalEnv := os.Getenv("NO_PROXY") - defer func() { // revert to pre-test env var - err := os.Setenv("NO_PROXY", originalEnv) - if err != nil { - t.Fatalf("Error reverting env NO_PROXY to its original value (%s) var after test ", originalEnv) - } - }() if err := os.Setenv("NO_PROXY", tc.env); err != nil { t.Errorf("Error during setting env: %v", err) } From fa9ddcd7f1b18b96361126fd037a6901a0a84d74 Mon Sep 17 00:00:00 2001 From: Marcin Niemira Date: Fri, 30 Aug 2019 07:54:32 +1000 Subject: [PATCH 5/5] typofix in comment --- pkg/minikube/proxy/proxy.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/minikube/proxy/proxy.go b/pkg/minikube/proxy/proxy.go index 97753431de..a65d29098c 100644 --- a/pkg/minikube/proxy/proxy.go +++ b/pkg/minikube/proxy/proxy.go @@ -55,7 +55,7 @@ func isInBlock(ip string, block string) (bool, error) { return false, errors.Wrapf(err, "Error ip not in block") } -// ExcludeIP takes ip or CIDR as string and excludes the it from the http(s)_proxy +// ExcludeIP takes ip or CIDR as string and excludes it from the http(s)_proxy func ExcludeIP(ip string) error { if netIP := net.ParseIP(ip); netIP == nil { if _, _, err := net.ParseCIDR(ip); err != nil {