Merge pull request #15984 from spowelljr/fixAddons

Fix image related bugs when enabling addons

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)

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.

pkg/minikube/assets/addons_test.go

@@ -16,7 +16,16 @@ limitations under the License.
 
 package assets
 
-import "testing"
+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`.
 func mapsEqual(a, b map[string]string) bool {
@@ -142,3 +151,225 @@ func TestOverrideDefautls(t *testing.T) {
 		}
 	}
 }
+
+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
+		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])
+	}
+}