ContainerRegistry remapping should be atomic

Fixes #5061

pkg/assets/builder.go

@@ -178,10 +178,17 @@ func (a *AssetBuilder) RemapImage(image string) (string, error) {
 			normalized = strings.TrimPrefix(normalized, "k8s.gcr.io/")
 		}
 
-		// We can't nest arbitrarily
+		// When assembling the cluster spec, kops may call the option more then once until the config converges
-		// Some risk of collisions, but also -- and __ in the names appear to be blocked by docker hub
+		// This means that this function may me called more than once on the same image
-		normalized = strings.Replace(normalized, "/", "-", -1)
+		// It this is pass is the second one, the image will already have been normalized with the containerRegistry settings
-		asset.DockerImage = registryMirror + "/" + normalized
+		// If this is the case, passing though the process again will re-prepend the container registry again
+		// and again, causing the spec to never converge and the config build to fail.
+		if !strings.HasPrefix(normalized, registryMirror+"/") {
+			// We can't nest arbitrarily
+			// Some risk of collisions, but also -- and __ in the names appear to be blocked by docker hub
+			normalized = strings.Replace(normalized, "/", "-", -1)
+			asset.DockerImage = registryMirror + "/" + normalized
+		}
 
 		asset.CanonicalLocation = image
 

pkg/assets/builder_test.go

@@ -17,6 +17,7 @@ limitations under the License.
 package assets
 
 import (
+	"errors"
 	"testing"
 
 	"k8s.io/kops/pkg/apis/kops"
@@ -130,3 +131,30 @@ func TestValidate_RemapImage_ContainerProxy_AppliesToImagesWithTags(t *testing.T
 		t.Errorf("Error remapping image (Expecting: %s, got %s)", expected, remapped)
 	}
 }
+
+func TestValidate_RemapImage_ContainerRegistry_MappingMultipleTimesConverges(t *testing.T) {
+	builder := buildAssetBuilder(t)
+
+	mirrorUrl := "proxy.example.com"
+	image := "kube-apiserver:1.2.3"
+	expected := "proxy.example.com/kube-apiserver:1.2.3"
+	version, _ := util.ParseKubernetesVersion("1.10")
+
+	builder.KubernetesVersion = *version
+	builder.AssetsLocation.ContainerRegistry = &mirrorUrl
+
+	remapped := image
+	err := errors.New("")
+	iterations := make([]map[int]int, 2)
+	for i := range iterations {
+		remapped, err = builder.RemapImage(remapped)
+		if err != nil {
+			t.Errorf("Error remapping image (iteration %d): %s", i, err)
+		}
+
+		if remapped != expected {
+			t.Errorf("Error remapping image (Expecting: %s, got %s, iteration: %d)", expected, remapped, i)
+		}
+	}
+
+}