From aad2b573c63c726ad98ee3e920f2787f8271479a Mon Sep 17 00:00:00 2001 From: Marek Counts Date: Thu, 16 May 2019 22:47:42 -0400 Subject: [PATCH 1/5] seperation of network calls when getting version updated the network calls to be package local so tests could pass their own implementation. A public interface was not provided as it would not be likely this would ever be needed or wanted. --- cmd/kubeadm/app/util/version.go | 14 +++++---- cmd/kubeadm/app/util/version_test.go | 45 ++++++++++++++++++---------- 2 files changed, 39 insertions(+), 20 deletions(-) diff --git a/cmd/kubeadm/app/util/version.go b/cmd/kubeadm/app/util/version.go index 4e90b27dda6..ddc139941dd 100644 --- a/cmd/kubeadm/app/util/version.go +++ b/cmd/kubeadm/app/util/version.go @@ -62,6 +62,10 @@ var ( // latest-1 (latest release in 1.x, including alpha/beta) // latest-1.0 (and similarly 1.1, 1.2, 1.3, ...) func KubernetesReleaseVersion(version string) (string, error) { + return kubernetesReleaseVersion(version, fetchFromURL) +} + +func kubernetesReleaseVersion(version string, fetcher func(string, time.Duration) (string, error)) (string, error) { ver := normalizedBuildVersion(version) if len(ver) != 0 { return ver, nil @@ -87,7 +91,7 @@ func KubernetesReleaseVersion(version string) (string, error) { clientVersion, clientVersionErr := kubeadmVersion(pkgversion.Get().String()) // Fetch version from the internet. url := fmt.Sprintf("%s/%s.txt", bucketURL, versionLabel) - body, err := fetchFromURL(url, getReleaseVersionTimeout) + body, err := fetcher(url, getReleaseVersionTimeout) if err != nil { // If the network operaton was successful but the server did not reply with StatusOK if body != "" { @@ -97,18 +101,18 @@ func KubernetesReleaseVersion(version string) (string, error) { // Handle air-gapped environments by falling back to the client version. klog.Warningf("could not fetch a Kubernetes version from the internet: %v", err) klog.Warningf("falling back to the local client version: %s", clientVersion) - return KubernetesReleaseVersion(clientVersion) + return kubernetesReleaseVersion(clientVersion, fetcher) } } if clientVersionErr != nil { if err != nil { klog.Warningf("could not obtain neither client nor remote version; fall back to: %s", constants.CurrentKubernetesVersion) - return KubernetesReleaseVersion(constants.CurrentKubernetesVersion.String()) + return kubernetesReleaseVersion(constants.CurrentKubernetesVersion.String(), fetcher) } klog.Warningf("could not obtain client version; using remote version: %s", body) - return KubernetesReleaseVersion(body) + return kubernetesReleaseVersion(body, fetcher) } // both the client and the remote version are obtained; validate them and pick a stable version @@ -117,7 +121,7 @@ func KubernetesReleaseVersion(version string) (string, error) { return "", err } // Re-validate received version and return. - return KubernetesReleaseVersion(body) + return kubernetesReleaseVersion(body, fetcher) } return "", errors.Errorf("version %q doesn't match patterns for neither semantic version nor labels (stable, latest, ...)", version) } diff --git a/cmd/kubeadm/app/util/version_test.go b/cmd/kubeadm/app/util/version_test.go index f40f261338b..39de4bd1e45 100644 --- a/cmd/kubeadm/app/util/version_test.go +++ b/cmd/kubeadm/app/util/version_test.go @@ -17,12 +17,13 @@ limitations under the License. package util import ( + "errors" "fmt" "net/http" - "net/http/httptest" "path" "strings" "testing" + "time" ) func TestEmptyVersion(t *testing.T) { @@ -50,7 +51,10 @@ func TestValidVersion(t *testing.T) { } for _, s := range validVersions { t.Run(s, func(t *testing.T) { - ver, err := KubernetesReleaseVersion(s) + fileFetcher := func(url string, timeout time.Duration) (string, error) { + return "", errors.New("Should not make internet call") + } + ver, err := kubernetesReleaseVersion(s, fileFetcher) t.Log("Valid: ", s, ver, err) if err != nil { t.Errorf("KubernetesReleaseVersion unexpected error for version %q: %v", s, err) @@ -72,7 +76,10 @@ func TestInvalidVersion(t *testing.T) { } for _, s := range invalidVersions { t.Run(s, func(t *testing.T) { - ver, err := KubernetesReleaseVersion(s) + fileFetcher := func(url string, timeout time.Duration) (string, error) { + return "", errors.New("Should not make internet call") + } + ver, err := kubernetesReleaseVersion(s, fileFetcher) t.Log("Invalid: ", s, ver, err) if err == nil { t.Errorf("KubernetesReleaseVersion error expected for version %q, but returned successfully", s) @@ -92,7 +99,10 @@ func TestValidConvenientForUserVersion(t *testing.T) { } for _, s := range validVersions { t.Run(s, func(t *testing.T) { - ver, err := KubernetesReleaseVersion(s) + fileFetcher := func(url string, timeout time.Duration) (string, error) { + return "", errors.New("Should not make internet call") + } + ver, err := kubernetesReleaseVersion(s, fileFetcher) t.Log("Valid: ", s, ver, err) if err != nil { t.Errorf("KubernetesReleaseVersion unexpected error for version %q: %v", s, err) @@ -121,22 +131,19 @@ func TestVersionFromNetwork(t *testing.T) { "garbage": {"NoSuchKeyThe specified key does not exist.", http.StatusOK, "", true}, "unknown": {"The requested URL was not found on this server.", http.StatusNotFound, "", true}, } - server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - key := strings.TrimSuffix(path.Base(r.URL.Path), ".txt") + + fileFetcher := func(url string, timeout time.Duration) (string, error) { + key := strings.TrimSuffix(path.Base(url), ".txt") res, found := cases[key] if found { - http.Error(w, res.Content, res.Status) - } else { - http.Error(w, "Unknown test case key!", http.StatusNotFound) + return res.Content, nil } - })) - defer server.Close() - - kubeReleaseBucketURL = server.URL + return "Unknown test case key!", errors.New("unknown test case key") + } for k, v := range cases { t.Run(k, func(t *testing.T) { - ver, err := KubernetesReleaseVersion(k) + ver, err := kubernetesReleaseVersion(k, fileFetcher) t.Logf("Key: %q. Result: %q, Error: %v", k, ver, err) switch { case err != nil && !v.ErrorExpected: @@ -272,7 +279,15 @@ func TestCIBuildVersion(t *testing.T) { for _, tc := range cases { t.Run(fmt.Sprintf("input:%s/expected:%s", tc.input, tc.expected), func(t *testing.T) { - ver, err := KubernetesReleaseVersion(tc.input) + + fileFetcher := func(url string, timeout time.Duration) (string, error) { + if tc.valid { + return tc.expected, nil + } + return "Unknown test case key!", fmt.Errorf("unknown test case key") + } + + ver, err := kubernetesReleaseVersion(tc.input, fileFetcher) t.Logf("Input: %q. Result: %q, Error: %v", tc.input, ver, err) switch { case err != nil && tc.valid: From c8045049f3b5ea72076a06535cf9fe11850d5d19 Mon Sep 17 00:00:00 2001 From: Marek Counts Date: Fri, 7 Jun 2019 08:36:49 -0400 Subject: [PATCH 2/5] updated tests to prevent false positive one test also proved it did not call the internet but this was not fool proof as it did not return a string and thus could be called with something expecting to fail. --- cmd/kubeadm/app/util/version_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/cmd/kubeadm/app/util/version_test.go b/cmd/kubeadm/app/util/version_test.go index 39de4bd1e45..e056f2d26d4 100644 --- a/cmd/kubeadm/app/util/version_test.go +++ b/cmd/kubeadm/app/util/version_test.go @@ -52,7 +52,7 @@ func TestValidVersion(t *testing.T) { for _, s := range validVersions { t.Run(s, func(t *testing.T) { fileFetcher := func(url string, timeout time.Duration) (string, error) { - return "", errors.New("Should not make internet call") + return "", errors.New("should not make internet call") } ver, err := kubernetesReleaseVersion(s, fileFetcher) t.Log("Valid: ", s, ver, err) @@ -77,7 +77,7 @@ func TestInvalidVersion(t *testing.T) { for _, s := range invalidVersions { t.Run(s, func(t *testing.T) { fileFetcher := func(url string, timeout time.Duration) (string, error) { - return "", errors.New("Should not make internet call") + return "should not make internet calls", errors.New("should not make internet call") } ver, err := kubernetesReleaseVersion(s, fileFetcher) t.Log("Invalid: ", s, ver, err) @@ -100,7 +100,7 @@ func TestValidConvenientForUserVersion(t *testing.T) { for _, s := range validVersions { t.Run(s, func(t *testing.T) { fileFetcher := func(url string, timeout time.Duration) (string, error) { - return "", errors.New("Should not make internet call") + return "", errors.New("should not make internet call") } ver, err := kubernetesReleaseVersion(s, fileFetcher) t.Log("Valid: ", s, ver, err) From bf376e863c1024b612b9039adf183455af878bf5 Mon Sep 17 00:00:00 2001 From: Marek Counts Date: Thu, 13 Jun 2019 09:57:17 -0400 Subject: [PATCH 3/5] update to remove unused test commit will be squashed before merge --- cmd/kubeadm/app/util/version_test.go | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/cmd/kubeadm/app/util/version_test.go b/cmd/kubeadm/app/util/version_test.go index e056f2d26d4..1f71ea6db5d 100644 --- a/cmd/kubeadm/app/util/version_test.go +++ b/cmd/kubeadm/app/util/version_test.go @@ -19,7 +19,6 @@ package util import ( "errors" "fmt" - "net/http" "path" "strings" "testing" @@ -117,19 +116,18 @@ func TestValidConvenientForUserVersion(t *testing.T) { func TestVersionFromNetwork(t *testing.T) { type T struct { Content string - Status int Expected string ErrorExpected bool } cases := map[string]T{ - "stable": {"stable-1", http.StatusOK, "v1.4.6", false}, // recursive pointer to stable-1 - "stable-1": {"v1.4.6", http.StatusOK, "v1.4.6", false}, - "stable-1.3": {"v1.3.10", http.StatusOK, "v1.3.10", false}, - "latest": {"v1.6.0-alpha.0", http.StatusOK, "v1.6.0-alpha.0", false}, - "latest-1.3": {"v1.3.11-beta.0", http.StatusOK, "v1.3.11-beta.0", false}, - "empty": {"", http.StatusOK, "", true}, - "garbage": {"NoSuchKeyThe specified key does not exist.", http.StatusOK, "", true}, - "unknown": {"The requested URL was not found on this server.", http.StatusNotFound, "", true}, + "stable": {"stable-1", "v1.4.6", false}, // recursive pointer to stable-1 + "stable-1": {"v1.4.6", "v1.4.6", false}, + "stable-1.3": {"v1.3.10", "v1.3.10", false}, + "latest": {"v1.6.0-alpha.0", "v1.6.0-alpha.0", false}, + "latest-1.3": {"v1.3.11-beta.0", "v1.3.11-beta.0", false}, + "empty": {"", "", true}, + "garbage": {"NoSuchKeyThe specified key does not exist.", "", true}, + "unknown": {"The requested URL was not found on this server.", "", true}, } fileFetcher := func(url string, timeout time.Duration) (string, error) { From b66c4e8d45c78e220b3cea033c0acf73755db364 Mon Sep 17 00:00:00 2001 From: Marek Counts Date: Fri, 14 Jun 2019 14:16:44 -0400 Subject: [PATCH 4/5] updates based off reviews --- cmd/kubeadm/app/util/version_test.go | 43 ++++++++++++++-------------- 1 file changed, 21 insertions(+), 22 deletions(-) diff --git a/cmd/kubeadm/app/util/version_test.go b/cmd/kubeadm/app/util/version_test.go index 1f71ea6db5d..48dfd02ec45 100644 --- a/cmd/kubeadm/app/util/version_test.go +++ b/cmd/kubeadm/app/util/version_test.go @@ -50,10 +50,7 @@ func TestValidVersion(t *testing.T) { } for _, s := range validVersions { t.Run(s, func(t *testing.T) { - fileFetcher := func(url string, timeout time.Duration) (string, error) { - return "", errors.New("should not make internet call") - } - ver, err := kubernetesReleaseVersion(s, fileFetcher) + ver, err := kubernetesReleaseVersion(s, errorFetcher) t.Log("Valid: ", s, ver, err) if err != nil { t.Errorf("KubernetesReleaseVersion unexpected error for version %q: %v", s, err) @@ -75,10 +72,7 @@ func TestInvalidVersion(t *testing.T) { } for _, s := range invalidVersions { t.Run(s, func(t *testing.T) { - fileFetcher := func(url string, timeout time.Duration) (string, error) { - return "should not make internet calls", errors.New("should not make internet call") - } - ver, err := kubernetesReleaseVersion(s, fileFetcher) + ver, err := kubernetesReleaseVersion(s, errorFetcher) t.Log("Invalid: ", s, ver, err) if err == nil { t.Errorf("KubernetesReleaseVersion error expected for version %q, but returned successfully", s) @@ -98,10 +92,7 @@ func TestValidConvenientForUserVersion(t *testing.T) { } for _, s := range validVersions { t.Run(s, func(t *testing.T) { - fileFetcher := func(url string, timeout time.Duration) (string, error) { - return "", errors.New("should not make internet call") - } - ver, err := kubernetesReleaseVersion(s, fileFetcher) + ver, err := kubernetesReleaseVersion(s, errorFetcher) t.Log("Valid: ", s, ver, err) if err != nil { t.Errorf("KubernetesReleaseVersion unexpected error for version %q: %v", s, err) @@ -130,17 +121,21 @@ func TestVersionFromNetwork(t *testing.T) { "unknown": {"The requested URL was not found on this server.", "", true}, } - fileFetcher := func(url string, timeout time.Duration) (string, error) { - key := strings.TrimSuffix(path.Base(url), ".txt") - res, found := cases[key] - if found { - return res.Content, nil - } - return "Unknown test case key!", errors.New("unknown test case key") - } - for k, v := range cases { t.Run(k, func(t *testing.T) { + + fileFetcher := func(url string, timeout time.Duration) (string, error) { + key := strings.TrimSuffix(path.Base(url), ".txt") + res, found := cases[key] + if found { + if v.ErrorExpected { + return "error", errors.New("expected error") + } + return res.Content, nil + } + return "Unknown test case key!", errors.New("unknown test case key") + } + ver, err := kubernetesReleaseVersion(k, fileFetcher) t.Logf("Key: %q. Result: %q, Error: %v", k, ver, err) switch { @@ -282,7 +277,7 @@ func TestCIBuildVersion(t *testing.T) { if tc.valid { return tc.expected, nil } - return "Unknown test case key!", fmt.Errorf("unknown test case key") + return "Unknown test case key!", errors.New("unknown test case key") } ver, err := kubernetesReleaseVersion(tc.input, fileFetcher) @@ -482,3 +477,7 @@ func TestValidateStableVersion(t *testing.T) { }) } } + +func errorFetcher(url string, timeout time.Duration) (string, error) { + return "should not make internet calls", fmt.Errorf("should not make internet calls, tried to request url: %s", url) +} From eb6eb11748265e47c56b8eb75c121f3fa5f80e30 Mon Sep 17 00:00:00 2001 From: Marek Counts Date: Fri, 21 Jun 2019 09:41:38 -0400 Subject: [PATCH 5/5] added comment --- cmd/kubeadm/app/util/version.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/cmd/kubeadm/app/util/version.go b/cmd/kubeadm/app/util/version.go index ddc139941dd..08a68b5c414 100644 --- a/cmd/kubeadm/app/util/version.go +++ b/cmd/kubeadm/app/util/version.go @@ -65,6 +65,9 @@ func KubernetesReleaseVersion(version string) (string, error) { return kubernetesReleaseVersion(version, fetchFromURL) } +// kubernetesReleaseVersion is a helper function to fetch +// available version information. Used for testing to eliminate +// the need for internet calls. func kubernetesReleaseVersion(version string, fetcher func(string, time.Duration) (string, error)) (string, error) { ver := normalizedBuildVersion(version) if len(ver) != 0 {