Persist custom addon image/registry settings.

pkg/addons/addons.go

@@ -185,6 +185,12 @@ https://github.com/kubernetes/minikube/issues/7332`, out.V{"driver_name": cc.Dri
 		exit.Error(reason.GuestCpConfig, "Error getting primary control plane", err)
 	}
 
+	// Persist images even if the machine is running so starting gets the correct images.
+	images, customRegistries, err := assets.SelectAndPersistImages(addon, cc)
+	if err != nil {
+		exit.Error(reason.HostSaveProfile, "Failed to persist images", err)
+	}
+
 	mName := config.MachineName(*cc, cp)
 	host, err := machine.LoadHost(api, mName)
 	if err != nil || !machine.IsRunning(api, mName) {
@@ -224,7 +230,7 @@ https://github.com/kubernetes/minikube/issues/7332`, out.V{"driver_name": cc.Dri
 		out.WarningT("At least needs control plane nodes to enable addon")
 	}
 
-	data := assets.GenerateTemplateData(addon, cc.KubernetesConfig, networkInfo)
+	data := assets.GenerateTemplateData(addon, cc.KubernetesConfig, networkInfo, images, customRegistries)
 	return enableOrDisableAddonInternal(cc, addon, runner, data, enable)
 }
 

pkg/minikube/assets/addons.go

@@ -660,8 +660,110 @@ var Addons = map[string]*Addon{
 	}),
 }
 
+// parseMapString creates a map based on `str` which is encoded as <key1>=<value1>,<key2>=<value2>,...
+func parseMapString(str string) map[string]string {
+	mapResult := make(map[string]string)
+	if str == "" {
+		return mapResult
+	}
+	for _, pairText := range strings.Split(str, ",") {
+		vals := strings.Split(pairText, "=")
+		if len(vals) != 2 {
+			out.WarningT("Ignoring invalid pair entry {{.pair}}", out.V{"pair": pairText})
+			continue
+		}
+		mapResult[vals[0]] = vals[1]
+	}
+	return mapResult
+}
+
+// mergeMaps creates a map with the union of `sourceMap` and `overrideMap` where collisions take the value of `overrideMap`.
+func mergeMaps(sourceMap, overrideMap map[string]string) map[string]string {
+	result := make(map[string]string)
+	for name, value := range sourceMap {
+		result[name] = value
+	}
+	for name, value := range overrideMap {
+		result[name] = value
+	}
+	return result
+}
+
+// filterKeySpace creates a map of the values in `targetMap` where the keys are also in `keySpace`.
+func filterKeySpace(keySpace map[string]string, targetMap map[string]string) map[string]string {
+	result := make(map[string]string)
+	for name := range keySpace {
+		if value, ok := targetMap[name]; ok {
+			result[name] = value
+		}
+	}
+	return result
+}
+
+// overrideDefaults creates a copy of `defaultMap` where `overrideMap` replaces any of its values that `overrideMap` contains.
+func overrideDefaults(defaultMap, overrideMap map[string]string) map[string]string {
+	return mergeMaps(defaultMap, filterKeySpace(defaultMap, overrideMap))
+}
+
+// SelectAndPersistImages selects which images to use based on addon default images, previously persisted images, and newly requested images - which are then persisted for future enables.
+func SelectAndPersistImages(addon *Addon, cc *config.ClusterConfig) (images, customRegistries map[string]string, err error) {
+	addonDefaultImages := addon.Images
+	if addonDefaultImages == nil {
+		addonDefaultImages = make(map[string]string)
+	}
+
+	// Use previously configured custom images.
+	images = overrideDefaults(addonDefaultImages, cc.CustomAddonImages)
+	if viper.IsSet(config.AddonImages) {
+		// Parse the AddonImages flag if present.
+		newImages := parseMapString(viper.GetString(config.AddonImages))
+		for name, image := range newImages {
+			if image == "" {
+				out.WarningT("Ignoring empty custom image {{.name}}", out.V{"name": name})
+				delete(newImages, name)
+				continue
+			}
+			if _, ok := addonDefaultImages[name]; !ok {
+				out.WarningT("Ignoring unknown custom image {{.name}}", out.V{"name": name})
+			}
+		}
+		// Use newly configured custom images.
+		images = overrideDefaults(addonDefaultImages, newImages)
+		// Store custom addon images to be written.
+		cc.CustomAddonImages = mergeMaps(cc.CustomAddonImages, images)
+	}
+
+	// Use previously configured custom registries.
+	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 {
+			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)
+			}
+		}
+		// 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)
+		}
+		// Merge newly set registries into custom addon registries to be written.
+		cc.CustomAddonRegistries = mergeMaps(cc.CustomAddonRegistries, customRegistries)
+	}
+
+	err = nil
+	// If images or registries were specified, save the config afterward.
+	if viper.IsSet(config.AddonImages) || viper.IsSet(config.AddonRegistries) {
+		// Since these values are only set when a user enables an addon, it is safe to refer to the profile name.
+		err = config.Write(viper.GetString(config.ProfileName), cc)
+		// Whether err is nil or not we still return here.
+	}
+	return images, customRegistries, err
+}
+
 // GenerateTemplateData generates template data for template assets
-func GenerateTemplateData(addon *Addon, cfg config.KubernetesConfig, netInfo NetworkInfo) interface{} {
+func GenerateTemplateData(addon *Addon, cfg config.KubernetesConfig, netInfo NetworkInfo, images, customRegistries map[string]string) interface{} {
 
 	a := runtime.GOARCH
 	// Some legacy docker images still need the -arch suffix
@@ -670,6 +772,7 @@ func GenerateTemplateData(addon *Addon, cfg config.KubernetesConfig, netInfo Net
 	if runtime.GOARCH != "amd64" {
 		ea = "-" + runtime.GOARCH
 	}
+
 	opts := struct {
 		Arch                string
 		ExoticArch          string
@@ -688,58 +791,21 @@ func GenerateTemplateData(addon *Addon, cfg config.KubernetesConfig, netInfo Net
 		LoadBalancerStartIP: cfg.LoadBalancerStartIP,
 		LoadBalancerEndIP:   cfg.LoadBalancerEndIP,
 		CustomIngressCert:   cfg.CustomIngressCert,
-		Images:              addon.Images,
+		Images:              images,
 		Registries:          addon.Registries,
-		CustomRegistries:    make(map[string]string),
+		CustomRegistries:    customRegistries,
 		NetworkInfo:         make(map[string]string),
 	}
 	if opts.ImageRepository != "" && !strings.HasSuffix(opts.ImageRepository, "/") {
 		opts.ImageRepository += "/"
 	}
-
-	// Network info for generating template
-	opts.NetworkInfo["ControlPlaneNodeIP"] = netInfo.ControlPlaneNodeIP
-	opts.NetworkInfo["ControlPlaneNodePort"] = fmt.Sprint(netInfo.ControlPlaneNodePort)
-
-	if opts.Images == nil {
-		opts.Images = make(map[string]string) // Avoid nil access when rendering
-	}
-
-	images := viper.GetString(config.AddonImages)
-	if images != "" {
-		for _, image := range strings.Split(images, ",") {
-			vals := strings.Split(image, "=")
-			if len(vals) != 2 || vals[1] == "" {
-				out.WarningT("Ignoring invalid custom image {{.conf}}", out.V{"conf": image})
-				continue
-			}
-			if _, ok := opts.Images[vals[0]]; ok {
-				opts.Images[vals[0]] = vals[1]
-			} else {
-				out.WarningT("Ignoring unknown custom image {{.name}}", out.V{"name": vals[0]})
-			}
-		}
-	}
-
 	if opts.Registries == nil {
 		opts.Registries = make(map[string]string)
 	}
 
-	registries := viper.GetString(config.AddonRegistries)
-	if registries != "" {
-		for _, registry := range strings.Split(registries, ",") {
-			vals := strings.Split(registry, "=")
-			if len(vals) != 2 {
-				out.WarningT("Ignoring invalid custom registry {{.conf}}", out.V{"conf": registry})
-				continue
-			}
-			if _, ok := opts.Images[vals[0]]; ok { // check images map because registry map may omitted default registry
-				opts.CustomRegistries[vals[0]] = vals[1]
-			} else {
-				out.WarningT("Ignoring unknown custom registry {{.name}}", out.V{"name": vals[0]})
-			}
-		}
-	}
+	// Network info for generating template
+	opts.NetworkInfo["ControlPlaneNodeIP"] = netInfo.ControlPlaneNodeIP
+	opts.NetworkInfo["ControlPlaneNodePort"] = fmt.Sprint(netInfo.ControlPlaneNodePort)
 
 	// Append postfix "/" to registries
 	for k, v := range opts.Registries {

pkg/minikube/assets/addons_test.go

@@ -0,0 +1,144 @@
+/*
+Copyright 2016 The Kubernetes Authors All rights reserved.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package assets
+
+import "testing"
+
+// mapsEqual returns true if and only if `a` contains all the same pairs as `b`.
+func mapsEqual(a, b map[string]string) bool {
+	for aKey, aValue := range a {
+		if bValue, ok := b[aKey]; !ok || aValue != bValue {
+			return false
+		}
+	}
+
+	for bKey := range b {
+		if _, ok := a[bKey]; !ok {
+			return false
+		}
+	}
+	return true
+}
+
+func TestParseMapString(t *testing.T) {
+	cases := map[string]map[string]string{
+		"Ardvark=1,B=2,Cantaloupe=3":         {"Ardvark": "1", "B": "2", "Cantaloupe": "3"},
+		"A=,B=2,C=":                          {"A": "", "B": "2", "C": ""},
+		"":                                   {},
+		"malformed,good=howdy,manyequals==,": {"good": "howdy"},
+	}
+	for actual, expected := range cases {
+		if parsedMap := parseMapString(actual); !mapsEqual(parsedMap, expected) {
+			t.Errorf("Parsed map from string \"%s\" differs from expected map: Actual: %v Expected: %v", actual, parsedMap, expected)
+		}
+	}
+}
+
+func TestMergeMaps(t *testing.T) {
+	type TestCase struct {
+		sourceMap   map[string]string
+		overrideMap map[string]string
+		expectedMap map[string]string
+	}
+	cases := []TestCase{
+		{
+			sourceMap:   map[string]string{"A": "1", "B": "2"},
+			overrideMap: map[string]string{"B": "7", "C": "3"},
+			expectedMap: map[string]string{"A": "1", "B": "7", "C": "3"},
+		},
+		{
+			sourceMap:   map[string]string{"B": "7", "C": "3"},
+			overrideMap: map[string]string{"A": "1", "B": "2"},
+			expectedMap: map[string]string{"A": "1", "B": "2", "C": "3"},
+		},
+		{
+			sourceMap:   map[string]string{"B": "7", "C": "3"},
+			overrideMap: map[string]string{},
+			expectedMap: map[string]string{"B": "7", "C": "3"},
+		},
+		{
+			sourceMap:   map[string]string{},
+			overrideMap: map[string]string{"B": "7", "C": "3"},
+			expectedMap: map[string]string{"B": "7", "C": "3"},
+		},
+	}
+	for _, test := range cases {
+		if actualMap := mergeMaps(test.sourceMap, test.overrideMap); !mapsEqual(actualMap, test.expectedMap) {
+			t.Errorf("Merging maps (source=%v, override=%v) differs from expected map: Actual: %v Expected: %v", test.sourceMap, test.overrideMap, actualMap, test.expectedMap)
+		}
+	}
+}
+
+func TestFilterKeySpace(t *testing.T) {
+	type TestCase struct {
+		keySpace    map[string]string
+		targetMap   map[string]string
+		expectedMap map[string]string
+	}
+	cases := []TestCase{
+		{
+			keySpace:    map[string]string{"A": "0", "B": ""},
+			targetMap:   map[string]string{"B": "1", "C": "2", "D": "3"},
+			expectedMap: map[string]string{"B": "1"},
+		},
+		{
+			keySpace:    map[string]string{},
+			targetMap:   map[string]string{"B": "1", "C": "2", "D": "3"},
+			expectedMap: map[string]string{},
+		},
+		{
+			keySpace:    map[string]string{"B": "1", "C": "2", "D": "3"},
+			targetMap:   map[string]string{},
+			expectedMap: map[string]string{},
+		},
+	}
+	for _, test := range cases {
+		if actualMap := filterKeySpace(test.keySpace, test.targetMap); !mapsEqual(actualMap, test.expectedMap) {
+			t.Errorf("Filtering keyspace of map (keyspace=%v, target=%v) differs from expected map: Actual: %v Expected: %v", test.keySpace, test.targetMap, actualMap, test.expectedMap)
+		}
+	}
+}
+
+func TestOverrideDefautls(t *testing.T) {
+	type TestCase struct {
+		defaultMap  map[string]string
+		overrideMap map[string]string
+		expectedMap map[string]string
+	}
+	cases := []TestCase{
+		{
+			defaultMap:  map[string]string{"A": "1", "B": "2", "C": "3"},
+			overrideMap: map[string]string{"B": "7", "C": "8"},
+			expectedMap: map[string]string{"A": "1", "B": "7", "C": "8"},
+		},
+		{
+			defaultMap:  map[string]string{"A": "1", "B": "2", "C": "3"},
+			overrideMap: map[string]string{"B": "7", "D": "8", "E": "9"},
+			expectedMap: map[string]string{"A": "1", "B": "7", "C": "3"},
+		},
+		{
+			defaultMap:  map[string]string{"A": "1", "B": "2", "C": "3"},
+			overrideMap: map[string]string{"B": "7", "D": "8", "E": "9"},
+			expectedMap: map[string]string{"A": "1", "B": "7", "C": "3"},
+		},
+	}
+	for _, test := range cases {
+		if actualMap := overrideDefaults(test.defaultMap, test.overrideMap); !mapsEqual(actualMap, test.expectedMap) {
+			t.Errorf("Override defaults (defaults=%v, overrides=%v) differs from expected map: Actual: %v Expected: %v", test.defaultMap, test.overrideMap, actualMap, test.expectedMap)
+		}
+	}
+}

pkg/minikube/config/types.go

@@ -74,6 +74,8 @@ type ClusterConfig struct {
 	KubernetesConfig        KubernetesConfig
 	Nodes                   []Node
 	Addons                  map[string]bool
+	CustomAddonImages       map[string]string
+	CustomAddonRegistries   map[string]string
 	VerifyComponents        map[string]bool // map of components to verify and wait for after start.
 	StartHostTimeout        time.Duration
 	ScheduledStop           *ScheduledStopConfig

site/content/en/docs/contrib/tests.en.md

@@ -332,6 +332,9 @@ runs the initial minikube start
 #### validateDeploying
 deploys an app the minikube cluster
 
+#### validateEnableAddonWhileActive
+makes sure addons can be enabled while cluster is active.
+
 #### validateStop
 tests minikube stop