From 93a86070440158e7f6ce7c90fe60833571762e5c Mon Sep 17 00:00:00 2001 From: Steven Powell Date: Mon, 6 Mar 2023 14:19:18 -0800 Subject: [PATCH 1/2] fix image related bugs when enabling addons --- cmd/minikube/cmd/config/enable.go | 8 +- pkg/minikube/assets/addons.go | 17 ++- pkg/minikube/assets/addons_test.go | 218 ++++++++++++++++++++++++++++- 3 files changed, 231 insertions(+), 12 deletions(-) diff --git a/cmd/minikube/cmd/config/enable.go b/cmd/minikube/cmd/config/enable.go index 4b7bc2ff53be..2ebadd761d65 100644 --- a/cmd/minikube/cmd/config/enable.go +++ b/cmd/minikube/cmd/config/enable.go @@ -66,8 +66,12 @@ You can view the list of minikube maintainers at: https://github.com/kubernetes/ } } } - viper.Set(config.AddonImages, images) - viper.Set(config.AddonRegistries, registries) + if images != "" { + viper.Set(config.AddonImages, images) + } + if registries != "" { + viper.Set(config.AddonRegistries, registries) + } err := addons.SetAndSave(ClusterFlagValue(), addon, "true") if err != nil && !errors.Is(err, addons.ErrSkipThisAddon) { exit.Error(reason.InternalAddonEnable, "enable failed", err) diff --git a/pkg/minikube/assets/addons.go b/pkg/minikube/assets/addons.go index 4761593bf04e..dbc4d0ee81df 100644 --- a/pkg/minikube/assets/addons.go +++ b/pkg/minikube/assets/addons.go @@ -815,10 +815,11 @@ func SelectAndPersistImages(addon *Addon, cc *config.ClusterConfig) (images, cus } if _, ok := addonDefaultImages[name]; !ok { out.WarningT("Ignoring unknown custom image {{.name}}", out.V{"name": name}) + delete(newImages, name) } } // Use newly configured custom images. - images = overrideDefaults(addonDefaultImages, newImages) + images = overrideDefaults(images, newImages) // Store custom addon images to be written. cc.CustomAddonImages = mergeMaps(cc.CustomAddonImages, newImages) } @@ -827,19 +828,17 @@ func SelectAndPersistImages(addon *Addon, cc *config.ClusterConfig) (images, cus customRegistries = filterKeySpace(addonDefaultImages, cc.CustomAddonRegistries) // filter by images map because registry map may omit default registry. if viper.IsSet(config.AddonRegistries) { // Parse the AddonRegistries flag if present. - customRegistries = parseMapString(viper.GetString(config.AddonRegistries)) - for name := range customRegistries { + newRegistries := parseMapString(viper.GetString(config.AddonRegistries)) + for name := range newRegistries { if _, ok := addonDefaultImages[name]; !ok { // check images map because registry map may omitted default registry out.WarningT("Ignoring unknown custom registry {{.name}}", out.V{"name": name}) - delete(customRegistries, name) + delete(newRegistries, name) } } - // Since registry map may omit default registry, any previously set custom registries for these images must be cleared. - for name := range addonDefaultImages { - delete(cc.CustomAddonRegistries, name) - } + // Use newly configured custom registries. + customRegistries = mergeMaps(customRegistries, newRegistries) // Merge newly set registries into custom addon registries to be written. - cc.CustomAddonRegistries = mergeMaps(cc.CustomAddonRegistries, customRegistries) + cc.CustomAddonRegistries = mergeMaps(cc.CustomAddonRegistries, newRegistries) } // If images or registries were specified, save the config afterward. diff --git a/pkg/minikube/assets/addons_test.go b/pkg/minikube/assets/addons_test.go index 22530cff069c..74a34142e01b 100644 --- a/pkg/minikube/assets/addons_test.go +++ b/pkg/minikube/assets/addons_test.go @@ -16,7 +16,13 @@ limitations under the License. package assets -import "testing" +import ( + "fmt" + "testing" + + "github.com/spf13/viper" + "k8s.io/minikube/pkg/minikube/config" +) // mapsEqual returns true if and only if `a` contains all the same pairs as `b`. func mapsEqual(a, b map[string]string) bool { @@ -142,3 +148,213 @@ func TestOverrideDefautls(t *testing.T) { } } } + +func TestSelectAndPersistImages(t *testing.T) { + gcpAuth := Addons["gcp-auth"] + gcpAuthImages := gcpAuth.Images + + type expected struct { + numImages int + numRegistries int + numCustomImages int + numCustomRegistries int + } + + test := func(t *testing.T, cc *config.ClusterConfig, e expected) (images, registries map[string]string) { + images, registries, err := SelectAndPersistImages(gcpAuth, cc) + if err != nil { + t.Fatal(err) + } + if len(images) != e.numImages { + t.Errorf("expected %d images but got %v", e.numImages, images) + } + if len(registries) != e.numRegistries { + t.Errorf("expected %d registries but got %v", e.numRegistries, registries) + } + if len(cc.CustomAddonImages) != e.numCustomImages { + t.Errorf("expected %d CustomAddonImages in config but got %+v", e.numCustomImages, cc.CustomAddonImages) + } + if len(cc.CustomAddonRegistries) != e.numCustomRegistries { + t.Errorf("expected %d CustomAddonRegistries in config but got %+v", e.numCustomRegistries, cc.CustomAddonRegistries) + } + return images, registries + } + + t.Run("NoCustomImage", func(t *testing.T) { + cc := &config.ClusterConfig{} + e := expected{numImages: 2} + images, _ := test(t, cc, e) + checkMatches(t, "KubeWebhookCertgen", images, gcpAuthImages) + checkMatches(t, "GCPAuthWebhook", images, gcpAuthImages) + }) + + t.Run("ExistingCustomImage", func(t *testing.T) { + cc := &config.ClusterConfig{ + CustomAddonImages: map[string]string{ + "GCPAuthWebhook": "test123", + }, + } + e := expected{numImages: 2, numCustomImages: 1} + images, _ := test(t, cc, e) + checkMatches(t, "KubeWebhookCertgen", images, gcpAuthImages) + checkMatches(t, "GCPAuthWebhook", images, cc.CustomAddonImages) + }) + + t.Run("NewCustomImage", func(t *testing.T) { + cc := &config.ClusterConfig{} + e := expected{numImages: 2, numCustomImages: 1} + addonImages := setAddonImages("GCPAuthWebhook", "test123") + defer viper.Reset() + images, _ := test(t, cc, e) + checkMatches(t, "KubeWebhookCertgen", images, gcpAuthImages) + checkMatches(t, "GCPAuthWebhook", images, addonImages) + }) + + t.Run("NewAndExistingCustomImages", func(t *testing.T) { + cc := &config.ClusterConfig{ + CustomAddonImages: map[string]string{ + "GCPAuthWebhook": "test123", + }, + } + e := expected{numImages: 2, numCustomImages: 2} + addonImages := setAddonImages("KubeWebhookCertgen", "test456") + defer viper.Reset() + images, _ := test(t, cc, e) + checkMatches(t, "KubeWebhookCertgen", images, addonImages) + checkMatches(t, "GCPAuthWebhook", images, cc.CustomAddonImages) + }) + + t.Run("NewOverwritesExistingCustomImage", func(t *testing.T) { + cc := &config.ClusterConfig{ + CustomAddonImages: map[string]string{ + "GCPAuthWebhook": "test123", + }, + } + e := expected{numImages: 2, numCustomImages: 1} + addonImages := setAddonImages("GCPAuthWebhook", "test456") + defer viper.Reset() + images, _ := test(t, cc, e) + checkMatches(t, "KubeWebhookCertgen", images, gcpAuthImages) + checkMatches(t, "GCPAuthWebhook", images, addonImages) + }) + + t.Run("NewUnrelatedCustomImageWithExistingCustomImage", func(t *testing.T) { + cc := &config.ClusterConfig{ + CustomAddonImages: map[string]string{ + "GCPAuthWebhook": "test123", + }, + } + e := expected{numImages: 2, numCustomImages: 1} + setAddonImages("IngressDNS", "test456") + defer viper.Reset() + images, _ := test(t, cc, e) + checkMatches(t, "KubeWebhookCertgen", images, gcpAuthImages) + checkMatches(t, "GCPAuthWebhook", images, cc.CustomAddonImages) + }) + + t.Run("NewCustomImageWithExistingUnrelatedCustomImage", func(t *testing.T) { + cc := &config.ClusterConfig{ + CustomAddonImages: map[string]string{ + "IngressDNS": "test123", + }, + } + e := expected{numImages: 2, numCustomImages: 2} + addonImages := setAddonImages("GCPAuthWebhook", "test456") + defer viper.Reset() + images, _ := test(t, cc, e) + checkMatches(t, "KubeWebhookCertgen", images, gcpAuthImages) + checkMatches(t, "GCPAuthWebhook", images, addonImages) + }) + + t.Run("ExistingCustomRegistry", func(t *testing.T) { + cc := &config.ClusterConfig{ + CustomAddonRegistries: map[string]string{ + "GCPAuthWebhook": "test123", + }, + } + e := expected{numImages: 2, numRegistries: 1, numCustomRegistries: 1} + _, registries := test(t, cc, e) + checkMatches(t, "GCPAuthWebhook", registries, cc.CustomAddonRegistries) + }) + + t.Run("NewCustomRegistry", func(t *testing.T) { + cc := &config.ClusterConfig{} + e := expected{numImages: 2, numRegistries: 1, numCustomRegistries: 1} + addonRegistries := setAddonRegistries("GCPAuthWebhook", "test123") + defer viper.Reset() + _, registries := test(t, cc, e) + checkMatches(t, "GCPAuthWebhook", registries, addonRegistries) + }) + + t.Run("NewAndExistingCustomRegistries", func(t *testing.T) { + cc := &config.ClusterConfig{ + CustomAddonRegistries: map[string]string{ + "GCPAuthWebhook": "test123", + }, + } + e := expected{numImages: 2, numRegistries: 2, numCustomRegistries: 2} + addonRegistries := setAddonRegistries("KubeWebhookCertgen", "test456") + defer viper.Reset() + _, registries := test(t, cc, e) + checkMatches(t, "GCPAuthWebhook", registries, cc.CustomAddonRegistries) + checkMatches(t, "KubeWebhookCertgen", registries, addonRegistries) + }) + + t.Run("NewOverwritesExistingCustomRegistry", func(t *testing.T) { + cc := &config.ClusterConfig{ + CustomAddonRegistries: map[string]string{ + "GCPAuthWebhook": "test123", + }, + } + e := expected{numImages: 2, numRegistries: 1, numCustomRegistries: 1} + addonRegistries := setAddonRegistries("GCPAuthWebhook", "test456") + defer viper.Reset() + _, registries := test(t, cc, e) + checkMatches(t, "GCPAuthWebhook", registries, addonRegistries) + }) + + t.Run("NewUnrelatedCustomRegistryWithExistingCustomRegistry", func(t *testing.T) { + cc := &config.ClusterConfig{ + CustomAddonRegistries: map[string]string{ + "GCPAuthWebhook": "test123", + }, + } + e := expected{numImages: 2, numRegistries: 1, numCustomRegistries: 1} + setAddonRegistries("IngressDNS", "test456") + defer viper.Reset() + _, registries := test(t, cc, e) + checkMatches(t, "GCPAuthWebhook", registries, cc.CustomAddonRegistries) + }) + + t.Run("NewCustomRegistryWithExistingUnrelatedCustomRegistry", func(t *testing.T) { + cc := &config.ClusterConfig{ + CustomAddonRegistries: map[string]string{ + "IngressDNS": "test123", + }, + } + e := expected{numImages: 2, numRegistries: 1, numCustomRegistries: 2} + addonRegistries := setAddonRegistries("GCPAuthWebhook", "test456") + defer viper.Reset() + _, registries := test(t, cc, e) + checkMatches(t, "GCPAuthWebhook", registries, addonRegistries) + }) +} + +func setAddonImages(k, v string) map[string]string { + return setFlag(config.AddonImages, k, v) +} + +func setAddonRegistries(k, v string) map[string]string { + return setFlag(config.AddonRegistries, k, v) +} + +func setFlag(name, k, v string) map[string]string { + viper.Set(name, fmt.Sprintf("%s=%s", k, v)) + return map[string]string{k: v} +} + +func checkMatches(t *testing.T, name string, got, expected map[string]string) { + if expected[name] != got[name] { + t.Errorf("expected %q to be %q, but got %q", name, expected[name], got[name]) + } +} From 2b41e1d4dbbc8d76a71328b9e9768cd8dcb9b9cd Mon Sep 17 00:00:00 2001 From: Steven Powell Date: Tue, 7 Mar 2023 10:12:03 -0800 Subject: [PATCH 2/2] create config.json file for test --- pkg/minikube/assets/addons_test.go | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/pkg/minikube/assets/addons_test.go b/pkg/minikube/assets/addons_test.go index 74a34142e01b..d34dd9c4098b 100644 --- a/pkg/minikube/assets/addons_test.go +++ b/pkg/minikube/assets/addons_test.go @@ -18,10 +18,13 @@ package assets import ( "fmt" + "os" + "path/filepath" "testing" "github.com/spf13/viper" "k8s.io/minikube/pkg/minikube/config" + "k8s.io/minikube/pkg/minikube/tests" ) // mapsEqual returns true if and only if `a` contains all the same pairs as `b`. @@ -153,6 +156,18 @@ func TestSelectAndPersistImages(t *testing.T) { gcpAuth := Addons["gcp-auth"] gcpAuthImages := gcpAuth.Images + // this test will write to ~/.minikube/profiles/minikube/config.json so need to create the file + home := tests.MakeTempDir(t) + profilePath := filepath.Join(home, "profiles", "minikube") + if err := os.MkdirAll(profilePath, 0777); err != nil { + t.Fatalf("failed to create profile directory: %v", err) + } + f, err := os.Create(filepath.Join(profilePath, "config.json")) + if err != nil { + t.Fatalf("failed to create config file: %v", err) + } + defer f.Close() + type expected struct { numImages int numRegistries int