From 5b53ae83ad83422e6893a0b4ffeabd5670f35d29 Mon Sep 17 00:00:00 2001 From: Sharif Elgamal Date: Thu, 21 Oct 2021 17:04:20 -0700 Subject: [PATCH 01/12] fix gcp-auth tests to avoid expiring tokens --- test/integration/addons_test.go | 74 +++++++++++++-------------------- 1 file changed, 30 insertions(+), 44 deletions(-) diff --git a/test/integration/addons_test.go b/test/integration/addons_test.go index e8730ac078..1217aee1f0 100644 --- a/test/integration/addons_test.go +++ b/test/integration/addons_test.go @@ -47,25 +47,9 @@ func TestAddons(t *testing.T) { defer Cleanup(t, profile, cancel) setupSucceeded := t.Run("Setup", func(t *testing.T) { - // We don't need a dummy file is we're on GCE - if !detect.IsOnGCE() || detect.IsCloudShell() { - // Set an env var to point to our dummy credentials file - err := os.Setenv("GOOGLE_APPLICATION_CREDENTIALS", filepath.Join(*testdataDir, "gcp-creds.json")) - t.Cleanup(func() { - os.Unsetenv("GOOGLE_APPLICATION_CREDENTIALS") - }) - if err != nil { - t.Fatalf("Failed setting GOOGLE_APPLICATION_CREDENTIALS env var: %v", err) - } - - err = os.Setenv("GOOGLE_CLOUD_PROJECT", "this_is_fake") - t.Cleanup(func() { - os.Unsetenv("GOOGLE_CLOUD_PROJECT") - }) - if err != nil { - t.Fatalf("Failed setting GOOGLE_CLOUD_PROJECT env var: %v", err) - } - } + // Set an env var to point to our dummy credentials file + t.Setenv("GOOGLE_APPLICATION_CREDENTIALS", filepath.Join(*testdataDir, "gcp-creds.json")) + t.Setenv("GOOGLE_CLOUD_PROJECT", "this_is_fake") args := append([]string{"start", "-p", profile, "--wait=true", "--memory=4000", "--alsologtostderr", "--addons=registry", "--addons=metrics-server", "--addons=olm", "--addons=volumesnapshots", "--addons=csi-hostpath-driver"}, StartArgs()...) if !NoneDriver() { // none driver does not support ingress @@ -85,24 +69,15 @@ func TestAddons(t *testing.T) { // If we're running the integration tests on GCE, which is frequently the case, first check to make sure we exit out properly, // then use force to actually test using creds. if detect.IsOnGCE() { - args = []string{"-p", profile, "addons", "enable", "gcp-auth"} + // ok, use force here since we are in GCE + // do not use --force unless absolutely necessary + args := []string{"-p", profile, "addons", "enable", "gcp-auth", "--force"} rr, err := Run(t, exec.CommandContext(ctx, Target(), args...)) if err != nil { t.Errorf("%s failed: %v", rr.Command(), err) - } else { - if !detect.IsCloudShell() && !strings.Contains(rr.Output(), "It seems that you are running in GCE") { - t.Errorf("Unexpected error message: %v", rr.Output()) - } else { - // ok, use force here since we are in GCE - // do not use --force unless absolutely necessary - args = append(args, "--force") - rr, err := Run(t, exec.CommandContext(ctx, Target(), args...)) - if err != nil { - t.Errorf("%s failed: %v", rr.Command(), err) - } - } } } + }) if !setupSucceeded { @@ -676,8 +651,31 @@ func validateGCPAuthAddon(ctx context.Context, t *testing.T, profile string) { t.Errorf("'printenv GOOGLE_CLOUD_PROJECT' returned %s, expected %s", got, expected) } + disableGCPAuth := func() error { + _, err = Run(t, exec.CommandContext(ctx, Target(), "-p", profile, "addons", "disable", "gcp-auth", "--alsologtostderr", "-v=1")) + if err != nil { + return err + } + return nil + } + + if err := retry.Expo(disableGCPAuth, Minutes(2), Minutes(10), 5); err != nil { + t.Errorf("failed to disable GCP auth addon: %v", err) + } + // If we're on GCE, we have proper credentials and can test the registry secrets with an artifact registry image if detect.IsOnGCE() && !detect.IsCloudShell() { + os.Unsetenv("GOOGLE_APPLICATION_CREDENTIALS") + os.Unsetenv("GOOGLE_CLOUD_PROJECT") + args := []string{"-p", profile, "addons", "enable", "gcp-auth"} + rr, err := Run(t, exec.CommandContext(ctx, Target(), args...)) + if err != nil { + t.Errorf("%s failed: %v", rr.Command(), err) + } else { + if !strings.Contains(rr.Output(), "It seems that you are running in GCE") { + t.Errorf("Unexpected error message: %v", rr.Output()) + } + } _, err = Run(t, exec.CommandContext(ctx, "kubectl", "--context", profile, "apply", "-f", filepath.Join(*testdataDir, "private-image.yaml"))) if err != nil { t.Fatalf("print env project: %v", err) @@ -703,16 +701,4 @@ func validateGCPAuthAddon(ctx context.Context, t *testing.T, profile string) { t.Fatalf("wait for private image: %v", err) } } - - disableGCPAuth := func() error { - _, err = Run(t, exec.CommandContext(ctx, Target(), "-p", profile, "addons", "disable", "gcp-auth", "--alsologtostderr", "-v=1")) - if err != nil { - return err - } - return nil - } - - if err := retry.Expo(disableGCPAuth, Minutes(2), Minutes(10), 5); err != nil { - t.Errorf("failed to disable GCP auth addon: %v", err) - } } From f8db8f072900be7ff59947e2ce72334fd7f6ba27 Mon Sep 17 00:00:00 2001 From: Sharif Elgamal Date: Mon, 25 Oct 2021 12:49:49 -0700 Subject: [PATCH 02/12] manually set and unset the env var again --- test/integration/addons_test.go | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/test/integration/addons_test.go b/test/integration/addons_test.go index 1217aee1f0..245042a2dd 100644 --- a/test/integration/addons_test.go +++ b/test/integration/addons_test.go @@ -48,8 +48,22 @@ func TestAddons(t *testing.T) { setupSucceeded := t.Run("Setup", func(t *testing.T) { // Set an env var to point to our dummy credentials file - t.Setenv("GOOGLE_APPLICATION_CREDENTIALS", filepath.Join(*testdataDir, "gcp-creds.json")) - t.Setenv("GOOGLE_CLOUD_PROJECT", "this_is_fake") + // don't use t.Setenv because we sometimes manually unset the env var later manually + err := os.Setenv("GOOGLE_APPLICATION_CREDENTIALS", filepath.Join(*testdataDir, "gcp-creds.json")) + t.Cleanup(func() { + os.Unsetenv("GOOGLE_APPLICATION_CREDENTIALS") + }) + if err != nil { + t.Fatalf("Failed setting GOOGLE_APPLICATION_CREDENTIALS env var: %v", err) + } + + err = os.Setenv("GOOGLE_CLOUD_PROJECT", "this_is_fake") + t.Cleanup(func() { + os.Unsetenv("GOOGLE_CLOUD_PROJECT") + }) + if err != nil { + t.Fatalf("Failed setting GOOGLE_CLOUD_PROJECT env var: %v", err) + } args := append([]string{"start", "-p", profile, "--wait=true", "--memory=4000", "--alsologtostderr", "--addons=registry", "--addons=metrics-server", "--addons=olm", "--addons=volumesnapshots", "--addons=csi-hostpath-driver"}, StartArgs()...) if !NoneDriver() { // none driver does not support ingress @@ -663,6 +677,8 @@ func validateGCPAuthAddon(ctx context.Context, t *testing.T, profile string) { t.Errorf("failed to disable GCP auth addon: %v", err) } + // maybe check that everything got cleaned up here? + // If we're on GCE, we have proper credentials and can test the registry secrets with an artifact registry image if detect.IsOnGCE() && !detect.IsCloudShell() { os.Unsetenv("GOOGLE_APPLICATION_CREDENTIALS") From bec03819df01178d51340eb86e2db845b0b48f43 Mon Sep 17 00:00:00 2001 From: Sharif Elgamal Date: Mon, 25 Oct 2021 15:32:13 -0700 Subject: [PATCH 03/12] fix disabling gcp-auth addon --- pkg/addons/addons.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/addons/addons.go b/pkg/addons/addons.go index 748a42ae0f..ce8686ae46 100644 --- a/pkg/addons/addons.go +++ b/pkg/addons/addons.go @@ -263,7 +263,7 @@ func addonSpecificChecks(cc *config.ClusterConfig, name string, enable bool, run } // If the gcp-auth credentials haven't been mounted in, don't start the pods - if name == "gcp-auth" { + if name == "gcp-auth" && enable { rr, err := runner.RunCmd(exec.Command("cat", credentialsPath)) if err != nil || rr.Stdout.String() == "" { return true, nil From bd9da426518d307fea485a0680e8d0f7ddbe62c2 Mon Sep 17 00:00:00 2001 From: Sharif Elgamal Date: Mon, 25 Oct 2021 17:31:24 -0700 Subject: [PATCH 04/12] reverse files for kubectl remove on addon disable --- deploy/addons/gcp-auth/gcp-auth-webhook.yaml.tmpl | 3 +-- pkg/addons/kubectl.go | 10 ++++++++-- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/deploy/addons/gcp-auth/gcp-auth-webhook.yaml.tmpl b/deploy/addons/gcp-auth/gcp-auth-webhook.yaml.tmpl index d3e5855914..dbac1650b7 100644 --- a/deploy/addons/gcp-auth/gcp-auth-webhook.yaml.tmpl +++ b/deploy/addons/gcp-auth/gcp-auth-webhook.yaml.tmpl @@ -1,4 +1,4 @@ -# Copyright 2017 The Kubernetes Authors. +# Copyright 2021 The Kubernetes Authors. # # Licensed under the Apache License, Version 2.0 (the "License"); # you may not use this file except in compliance with the License. @@ -12,7 +12,6 @@ # See the License for the specific language governing permissions and # limitations under the License. ---- apiVersion: v1 kind: ServiceAccount metadata: diff --git a/pkg/addons/kubectl.go b/pkg/addons/kubectl.go index 6f4003ab12..16a10c55ce 100644 --- a/pkg/addons/kubectl.go +++ b/pkg/addons/kubectl.go @@ -41,8 +41,14 @@ func kubectlCommand(cc *config.ClusterConfig, files []string, enable bool) *exec } args := []string{fmt.Sprintf("KUBECONFIG=%s", path.Join(vmpath.GuestPersistentDir, "kubeconfig")), kubectlBinary, kubectlAction} - for _, f := range files { - args = append(args, []string{"-f", f}...) + if enable { + for _, f := range files { + args = append(args, []string{"-f", f}...) + } + } else { + for i := len(files) - 1; i >= 0; i-- { + args = append(args, []string{"-f", files[i]}...) + } } return exec.Command("sudo", args...) From b0598d06e539b426aaac10a298d61de2479e49da Mon Sep 17 00:00:00 2001 From: Sharif Elgamal Date: Mon, 25 Oct 2021 18:07:45 -0700 Subject: [PATCH 05/12] ignore-not-found for kubectl delete --- pkg/addons/kubectl.go | 1 + 1 file changed, 1 insertion(+) diff --git a/pkg/addons/kubectl.go b/pkg/addons/kubectl.go index 16a10c55ce..eda42d4eab 100644 --- a/pkg/addons/kubectl.go +++ b/pkg/addons/kubectl.go @@ -46,6 +46,7 @@ func kubectlCommand(cc *config.ClusterConfig, files []string, enable bool) *exec args = append(args, []string{"-f", f}...) } } else { + args = append(args, "--ignore-not-found") for i := len(files) - 1; i >= 0; i-- { args = append(args, []string{"-f", files[i]}...) } From 1e932bc5bde17c492f6d186f669535c8fc4efa3b Mon Sep 17 00:00:00 2001 From: Sharif Elgamal Date: Mon, 25 Oct 2021 18:22:49 -0700 Subject: [PATCH 06/12] cleanup and comments --- pkg/addons/kubectl.go | 3 +++ test/integration/addons_test.go | 26 ++++++-------------------- 2 files changed, 9 insertions(+), 20 deletions(-) diff --git a/pkg/addons/kubectl.go b/pkg/addons/kubectl.go index eda42d4eab..d59c5a18f8 100644 --- a/pkg/addons/kubectl.go +++ b/pkg/addons/kubectl.go @@ -46,7 +46,10 @@ func kubectlCommand(cc *config.ClusterConfig, files []string, enable bool) *exec args = append(args, []string{"-f", f}...) } } else { + // --ignore-not-found just ignores when we try to ignore a resource that is gone, + // like a completed job with a ttlSecondsAfterFinished args = append(args, "--ignore-not-found") + // reverse the order of files to delete, sometimes file order matters for i := len(files) - 1; i >= 0; i-- { args = append(args, []string{"-f", files[i]}...) } diff --git a/test/integration/addons_test.go b/test/integration/addons_test.go index 245042a2dd..964c253aff 100644 --- a/test/integration/addons_test.go +++ b/test/integration/addons_test.go @@ -65,33 +65,18 @@ func TestAddons(t *testing.T) { t.Fatalf("Failed setting GOOGLE_CLOUD_PROJECT env var: %v", err) } - args := append([]string{"start", "-p", profile, "--wait=true", "--memory=4000", "--alsologtostderr", "--addons=registry", "--addons=metrics-server", "--addons=olm", "--addons=volumesnapshots", "--addons=csi-hostpath-driver"}, StartArgs()...) + args := append([]string{"start", "-p", profile, "--wait=true", "--memory=4000", "--alsologtostderr", "--addons=registry", "--addons=metrics-server", "--addons=olm", "--addons=volumesnapshots", "--addons=csi-hostpath-driver", "--addons=gcp-auth"}, StartArgs()...) if !NoneDriver() { // none driver does not support ingress args = append(args, "--addons=ingress", "--addons=ingress-dns") } if !arm64Platform() { args = append(args, "--addons=helm-tiller") } - if !detect.IsOnGCE() { - args = append(args, "--addons=gcp-auth") - } rr, err := Run(t, exec.CommandContext(ctx, Target(), args...)) if err != nil { t.Fatalf("%s failed: %v", rr.Command(), err) } - // If we're running the integration tests on GCE, which is frequently the case, first check to make sure we exit out properly, - // then use force to actually test using creds. - if detect.IsOnGCE() { - // ok, use force here since we are in GCE - // do not use --force unless absolutely necessary - args := []string{"-p", profile, "addons", "enable", "gcp-auth", "--force"} - rr, err := Run(t, exec.CommandContext(ctx, Target(), args...)) - if err != nil { - t.Errorf("%s failed: %v", rr.Command(), err) - } - } - }) if !setupSucceeded { @@ -677,8 +662,6 @@ func validateGCPAuthAddon(ctx context.Context, t *testing.T, profile string) { t.Errorf("failed to disable GCP auth addon: %v", err) } - // maybe check that everything got cleaned up here? - // If we're on GCE, we have proper credentials and can test the registry secrets with an artifact registry image if detect.IsOnGCE() && !detect.IsCloudShell() { os.Unsetenv("GOOGLE_APPLICATION_CREDENTIALS") @@ -710,11 +693,14 @@ func validateGCPAuthAddon(ctx context.Context, t *testing.T, profile string) { t.Fatalf("print env project: %v", err) } - // Make sure the pod is up and running, which means we successfully pulled the private image down - // 8 minutes, because 4 is not enough for images to pull in all cases. _, err = PodWait(ctx, t, profile, "default", "integration-test=private-image-eu", Minutes(8)) if err != nil { t.Fatalf("wait for private image: %v", err) } + + // Disable the addon for good now + if err := retry.Expo(disableGCPAuth, Minutes(2), Minutes(10), 5); err != nil { + t.Errorf("failed to disable GCP auth addon: %v", err) + } } } From 2ed63b65f549aead7c2fd79cc08a6f89218d87bb Mon Sep 17 00:00:00 2001 From: Sharif Elgamal Date: Tue, 26 Oct 2021 08:50:16 -0700 Subject: [PATCH 07/12] fix lint --- test/integration/addons_test.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/test/integration/addons_test.go b/test/integration/addons_test.go index 964c253aff..8ae685188e 100644 --- a/test/integration/addons_test.go +++ b/test/integration/addons_test.go @@ -670,10 +670,8 @@ func validateGCPAuthAddon(ctx context.Context, t *testing.T, profile string) { rr, err := Run(t, exec.CommandContext(ctx, Target(), args...)) if err != nil { t.Errorf("%s failed: %v", rr.Command(), err) - } else { - if !strings.Contains(rr.Output(), "It seems that you are running in GCE") { - t.Errorf("Unexpected error message: %v", rr.Output()) - } + } else if !strings.Contains(rr.Output(), "It seems that you are running in GCE") { + t.Errorf("Unexpected error message: %v", rr.Output()) } _, err = Run(t, exec.CommandContext(ctx, "kubectl", "--context", profile, "apply", "-f", filepath.Join(*testdataDir, "private-image.yaml"))) if err != nil { From 77da350d835f2cae747823b8973c91ae2af9b8ff Mon Sep 17 00:00:00 2001 From: Sharif Elgamal Date: Tue, 26 Oct 2021 09:01:58 -0700 Subject: [PATCH 08/12] fix unit test --- pkg/addons/kubectl_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/addons/kubectl_test.go b/pkg/addons/kubectl_test.go index b377c278c8..69eddce133 100644 --- a/pkg/addons/kubectl_test.go +++ b/pkg/addons/kubectl_test.go @@ -39,7 +39,7 @@ func TestKubectlCommand(t *testing.T) { description: "disable an addon", files: []string{"a", "b"}, enable: false, - expected: "sudo KUBECONFIG=/var/lib/minikube/kubeconfig /var/lib/minikube/binaries/v1.17.0/kubectl delete -f a -f b", + expected: "sudo KUBECONFIG=/var/lib/minikube/kubeconfig /var/lib/minikube/binaries/v1.17.0/kubectl delete --ignore-not-found -f a -f b", }, } From aaf85032f47df60b730344b84a2271fedc630c64 Mon Sep 17 00:00:00 2001 From: Sharif Elgamal Date: Tue, 26 Oct 2021 10:34:46 -0700 Subject: [PATCH 09/12] fix unit test part deux --- pkg/addons/kubectl_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/addons/kubectl_test.go b/pkg/addons/kubectl_test.go index 69eddce133..975f211736 100644 --- a/pkg/addons/kubectl_test.go +++ b/pkg/addons/kubectl_test.go @@ -39,7 +39,7 @@ func TestKubectlCommand(t *testing.T) { description: "disable an addon", files: []string{"a", "b"}, enable: false, - expected: "sudo KUBECONFIG=/var/lib/minikube/kubeconfig /var/lib/minikube/binaries/v1.17.0/kubectl delete --ignore-not-found -f a -f b", + expected: "sudo KUBECONFIG=/var/lib/minikube/kubeconfig /var/lib/minikube/binaries/v1.17.0/kubectl delete --ignore-not-found -f b -f a", }, } From 8cf0a18069601f18cf7f119dd50f2a1da81d74b4 Mon Sep 17 00:00:00 2001 From: Sharif Elgamal Date: Tue, 26 Oct 2021 11:46:29 -0700 Subject: [PATCH 10/12] no need to reverse file order for kubectl delete --- pkg/addons/kubectl.go | 15 +++++---------- pkg/addons/kubectl_test.go | 2 +- 2 files changed, 6 insertions(+), 11 deletions(-) diff --git a/pkg/addons/kubectl.go b/pkg/addons/kubectl.go index d59c5a18f8..60ccc2c239 100644 --- a/pkg/addons/kubectl.go +++ b/pkg/addons/kubectl.go @@ -41,18 +41,13 @@ func kubectlCommand(cc *config.ClusterConfig, files []string, enable bool) *exec } args := []string{fmt.Sprintf("KUBECONFIG=%s", path.Join(vmpath.GuestPersistentDir, "kubeconfig")), kubectlBinary, kubectlAction} - if enable { - for _, f := range files { - args = append(args, []string{"-f", f}...) - } - } else { - // --ignore-not-found just ignores when we try to ignore a resource that is gone, + if !enable { + // --ignore-not-found just ignores when we try to delete a resource that is already gone, // like a completed job with a ttlSecondsAfterFinished args = append(args, "--ignore-not-found") - // reverse the order of files to delete, sometimes file order matters - for i := len(files) - 1; i >= 0; i-- { - args = append(args, []string{"-f", files[i]}...) - } + } + for _, f := range files { + args = append(args, []string{"-f", f}...) } return exec.Command("sudo", args...) diff --git a/pkg/addons/kubectl_test.go b/pkg/addons/kubectl_test.go index 975f211736..69eddce133 100644 --- a/pkg/addons/kubectl_test.go +++ b/pkg/addons/kubectl_test.go @@ -39,7 +39,7 @@ func TestKubectlCommand(t *testing.T) { description: "disable an addon", files: []string{"a", "b"}, enable: false, - expected: "sudo KUBECONFIG=/var/lib/minikube/kubeconfig /var/lib/minikube/binaries/v1.17.0/kubectl delete --ignore-not-found -f b -f a", + expected: "sudo KUBECONFIG=/var/lib/minikube/kubeconfig /var/lib/minikube/binaries/v1.17.0/kubectl delete --ignore-not-found -f a -f b", }, } From 144ebe6498ae21a1bcbcdf74ad51d4648f399179 Mon Sep 17 00:00:00 2001 From: Sharif Elgamal Date: Tue, 26 Oct 2021 12:59:17 -0700 Subject: [PATCH 11/12] don't run disable on final gcp-auth enable, we're not actually enabling the addon --- test/integration/addons_test.go | 5 ----- 1 file changed, 5 deletions(-) diff --git a/test/integration/addons_test.go b/test/integration/addons_test.go index 8ae685188e..05be246d91 100644 --- a/test/integration/addons_test.go +++ b/test/integration/addons_test.go @@ -695,10 +695,5 @@ func validateGCPAuthAddon(ctx context.Context, t *testing.T, profile string) { if err != nil { t.Fatalf("wait for private image: %v", err) } - - // Disable the addon for good now - if err := retry.Expo(disableGCPAuth, Minutes(2), Minutes(10), 5); err != nil { - t.Errorf("failed to disable GCP auth addon: %v", err) - } } } From 67f3149c9e4c2cddb68e39469009e69f983906ce Mon Sep 17 00:00:00 2001 From: Sharif Elgamal Date: Tue, 26 Oct 2021 13:47:55 -0700 Subject: [PATCH 12/12] fix expected test output --- test/integration/addons_test.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/test/integration/addons_test.go b/test/integration/addons_test.go index 05be246d91..cba3456789 100644 --- a/test/integration/addons_test.go +++ b/test/integration/addons_test.go @@ -643,9 +643,7 @@ func validateGCPAuthAddon(ctx context.Context, t *testing.T, profile string) { got = strings.TrimSpace(rr.Stdout.String()) expected = "this_is_fake" - if detect.IsOnGCE() && !detect.IsCloudShell() { - expected = "k8s-minikube" - } + if got != expected { t.Errorf("'printenv GOOGLE_CLOUD_PROJECT' returned %s, expected %s", got, expected) }