Merge pull request #17144 from rifelpet/warmpool-containerproxy

Normalize the hardcoded images used for warmpool pre-pulling

Author: k8s-ci-robot

pkg/assets/builder.go

@@ -144,7 +144,7 @@ func (a *AssetBuilder) RemapManifest(data []byte) ([]byte, error) {
 }
 
 // RemapImage normalizes a containers location if a user sets the AssetsLocation ContainerRegistry location.
-func (a *AssetBuilder) RemapImage(image string) (string, error) {
+func (a *AssetBuilder) RemapImage(image string) string {
 	asset := &ImageAsset{
 		DownloadLocation:  image,
 		CanonicalLocation: image,
@@ -179,66 +179,27 @@ func (a *AssetBuilder) RemapImage(image string) (string, error) {
 		}
 	}
 
-	if a.AssetsLocation != nil && a.AssetsLocation.ContainerProxy != nil {
-		containerProxy := strings.TrimSuffix(*a.AssetsLocation.ContainerProxy, "/")
-		normalized := image
-
-		// If the image name contains only a single / we need to determine if the image is located on docker-hub or if it's using a convenient URL,
-		// like registry.k8s.io/<image-name> or registry.k8s.io/<image-name>
-		// In case of a hub image it should be sufficient to just prepend the proxy url, producing eg docker-proxy.example.com/weaveworks/weave-kube
-		if strings.Count(normalized, "/") <= 1 && !strings.ContainsAny(strings.Split(normalized, "/")[0], ".:") {
-			normalized = containerProxy + "/" + normalized
-		} else {
-			re := regexp.MustCompile(`^[^/]+`)
-			normalized = re.ReplaceAllString(normalized, containerProxy)
-		}
-
-		asset.DownloadLocation = normalized
-
-		// Run the new image
-		image = asset.DownloadLocation
-	}
-
-	if a.AssetsLocation != nil && a.AssetsLocation.ContainerRegistry != nil {
-		registryMirror := *a.AssetsLocation.ContainerRegistry
-		normalized := image
-
-		// Remove the 'standard' kubernetes image prefixes, just for sanity
-		normalized = strings.TrimPrefix(normalized, "registry.k8s.io/")
-
-		// When assembling the cluster spec, kops may call the option more then once until the config converges
-		// This means that this function may me called more than once on the same image
-		// It this is pass is the second one, the image will already have been normalized with the containerRegistry settings
-		// 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.DownloadLocation = registryMirror + "/" + normalized
-		}
-
-		// Run the new image
-		image = asset.DownloadLocation
-	}
+	normalized := NormalizeImage(a, image)
+	image = normalized
+	asset.DownloadLocation = normalized
 
 	a.ImageAssets = append(a.ImageAssets, asset)
 
 	if !featureflag.ImageDigest.Enabled() || os.Getenv("KOPS_BASE_URL") != "" {
-		return image, nil
+		return image
 	}
 
 	if strings.Contains(image, "@") {
-		return image, nil
+		return image
 	}
 
 	digest, err := crane.Digest(image, crane.WithAuthFromKeychain(authn.DefaultKeychain))
 	if err != nil {
 		klog.Warningf("failed to digest image %q: %s", image, err)
-		return image, nil
+		return image
 	}
 
-	return image + "@" + digest, nil
+	return image + "@" + digest
 }
 
 // RemapFile returns a remapped URL for the file, if AssetsLocation is defined.
@@ -378,3 +339,46 @@ func (a *AssetBuilder) remapURL(canonicalURL *url.URL) (*url.URL, error) {
 
 	return fileRepo, nil
 }
+
+func NormalizeImage(a *AssetBuilder, image string) string {
+	if a.AssetsLocation != nil && a.AssetsLocation.ContainerProxy != nil {
+		containerProxy := strings.TrimSuffix(*a.AssetsLocation.ContainerProxy, "/")
+		normalized := image
+
+		// If the image name contains only a single / we need to determine if the image is located on docker-hub or if it's using a convenient URL,
+		// like registry.k8s.io/<image-name> or registry.k8s.io/<image-name>
+		// In case of a hub image it should be sufficient to just prepend the proxy url, producing eg docker-proxy.example.com/weaveworks/weave-kube
+		if strings.Count(normalized, "/") <= 1 && !strings.ContainsAny(strings.Split(normalized, "/")[0], ".:") {
+			normalized = containerProxy + "/" + normalized
+		} else {
+			re := regexp.MustCompile(`^[^/]+`)
+			normalized = re.ReplaceAllString(normalized, containerProxy)
+		}
+
+		// Run the new image
+		image = normalized
+	}
+
+	if a.AssetsLocation != nil && a.AssetsLocation.ContainerRegistry != nil {
+		registryMirror := *a.AssetsLocation.ContainerRegistry
+		normalized := image
+
+		// Remove the 'standard' kubernetes image prefixes, just for sanity
+		normalized = strings.TrimPrefix(normalized, "registry.k8s.io/")
+
+		// When assembling the cluster spec, kops may call the option more then once until the config converges
+		// This means that this function may me called more than once on the same image
+		// It this is pass is the second one, the image will already have been normalized with the containerRegistry settings
+		// 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)
+			normalized = registryMirror + "/" + normalized
+		}
+		image = normalized
+	}
+	// Run the new image
+	return image
+}

pkg/assets/builder_test.go

@@ -42,11 +42,7 @@ func TestValidate_RemapImage_ContainerProxy_AppliesToDockerHub(t *testing.T) {
 
 	builder.AssetsLocation.ContainerProxy = &proxyURL
 
-	remapped, err := builder.RemapImage(image)
-	if err != nil {
-		t.Error("Error remapping image", err)
-	}
-
+	remapped := builder.RemapImage(image)
 	if remapped != expected {
 		t.Errorf("Error remapping image (Expecting: %s, got %s)", expected, remapped)
 	}
@@ -61,11 +57,7 @@ func TestValidate_RemapImage_ContainerProxy_AppliesToSimplifiedDockerHub(t *test
 
 	builder.AssetsLocation.ContainerProxy = &proxyURL
 
-	remapped, err := builder.RemapImage(image)
-	if err != nil {
-		t.Error("Error remapping image", err)
-	}
-
+	remapped := builder.RemapImage(image)
 	if remapped != expected {
 		t.Errorf("Error remapping image (Expecting: %s, got %s)", expected, remapped)
 	}
@@ -80,11 +72,7 @@ func TestValidate_RemapImage_ContainerProxy_AppliesToSimplifiedKubernetesURL(t *
 
 	builder.AssetsLocation.ContainerProxy = &proxyURL
 
-	remapped, err := builder.RemapImage(image)
-	if err != nil {
-		t.Error("Error remapping image", err)
-	}
-
+	remapped := builder.RemapImage(image)
 	if remapped != expected {
 		t.Errorf("Error remapping image (Expecting: %s, got %s)", expected, remapped)
 	}
@@ -99,11 +87,7 @@ func TestValidate_RemapImage_ContainerProxy_AppliesToLegacyKubernetesURL(t *test
 
 	builder.AssetsLocation.ContainerProxy = &proxyURL
 
-	remapped, err := builder.RemapImage(image)
-	if err != nil {
-		t.Error("Error remapping image", err)
-	}
-
+	remapped := builder.RemapImage(image)
 	if remapped != expected {
 		t.Errorf("Error remapping image (Expecting: %s, got %s)", expected, remapped)
 	}
@@ -118,11 +102,7 @@ func TestValidate_RemapImage_ContainerProxy_AppliesToImagesWithTags(t *testing.T
 
 	builder.AssetsLocation.ContainerProxy = &proxyURL
 
-	remapped, err := builder.RemapImage(image)
-	if err != nil {
-		t.Error("Error remapping image", err)
-	}
-
+	remapped := builder.RemapImage(image)
 	if remapped != expected {
 		t.Errorf("Error remapping image (Expecting: %s, got %s)", expected, remapped)
 	}
@@ -140,11 +120,7 @@ func TestValidate_RemapImage_ContainerRegistry_MappingMultipleTimesConverges(t *
 	remapped := image
 	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)
-		}
-
+		remapped := builder.RemapImage(remapped)
 		if remapped != expected {
 			t.Errorf("Error remapping image (Expecting: %s, got %s, iteration: %d)", expected, remapped, i)
 		}

pkg/kubemanifest/images.go

@@ -17,13 +17,12 @@ limitations under the License.
 package kubemanifest
 
 import (
-	"fmt"
 	"strings"
 
 	"k8s.io/klog/v2"
 )
 
-type ImageRemapFunction func(image string) (string, error)
+type ImageRemapFunction func(image string) string
 
 func (m *Object) RemapImages(mapper ImageRemapFunction) error {
 	visitor := &imageRemapVisitor{
@@ -57,10 +56,7 @@ func (m *imageRemapVisitor) VisitString(path []string, v string, mutator func(st
 
 	image := v
 	klog.V(4).Infof("Consider image for re-mapping: %q", image)
-	remapped, err := m.mapper(v)
-	if err != nil {
-		return fmt.Errorf("error remapping image %q: %v", image, err)
-	}
+	remapped := m.mapper(v)
 	if remapped != image {
 		mutator(remapped)
 	}

pkg/model/components/context.go

@@ -150,11 +150,7 @@ func Image(component string, clusterSpec *kops.ClusterSpec, assetsBuilder *asset
 	if !kopsmodel.IsBaseURL(clusterSpec.KubernetesVersion) {
 		image := "registry.k8s.io/" + imageName + ":" + "v" + kubernetesVersion.String()
 
-		image, err := assetsBuilder.RemapImage(image)
-		if err != nil {
-			return "", fmt.Errorf("unable to remap container %q: %v", image, err)
-		}
-		return image, nil
+		return assetsBuilder.RemapImage(image), nil
 	}
 
 	// The simple name is valid when pulling.  But if we

pkg/model/components/etcdmanager/model.go

@@ -314,11 +314,7 @@ func (b *EtcdManagerBuilder) buildPod(etcdCluster kops.EtcdClusterSpec, instance
 		// Remap image via AssetBuilder
 		for i := range pod.Spec.InitContainers {
 			initContainer := &pod.Spec.InitContainers[i]
-			remapped, err := b.AssetBuilder.RemapImage(initContainer.Image)
-			if err != nil {
-				return nil, fmt.Errorf("unable to remap init container image %q: %w", container.Image, err)
-			}
-			initContainer.Image = remapped
+			initContainer.Image = b.AssetBuilder.RemapImage(initContainer.Image)
 		}
 	}
 
@@ -334,11 +330,7 @@ func (b *EtcdManagerBuilder) buildPod(etcdCluster kops.EtcdClusterSpec, instance
 		}
 
 		// Remap image via AssetBuilder
-		remapped, err := b.AssetBuilder.RemapImage(container.Image)
-		if err != nil {
-			return nil, fmt.Errorf("unable to remap container image %q: %w", container.Image, err)
-		}
-		container.Image = remapped
+		container.Image = b.AssetBuilder.RemapImage(container.Image)
 	}
 
 	var clientHost string

pkg/model/components/kubeapiserver/model.go

@@ -138,13 +138,7 @@ func (b *KubeApiserverBuilder) buildHealthcheckSidecar() (*corev1.Pod, error) {
 	}
 
 	// Remap image via AssetBuilder
-	{
-		remapped, err := b.AssetBuilder.RemapImage(container.Image)
-		if err != nil {
-			return nil, fmt.Errorf("unable to remap container image %q: %v", container.Image, err)
-		}
-		container.Image = remapped
-	}
+	container.Image = b.AssetBuilder.RemapImage(container.Image)
 
 	return pod, nil
 }

pkg/model/components/kubelet.go

@@ -166,11 +166,7 @@ func (b *KubeletOptionsBuilder) configureKubelet(cluster *kops.Cluster, kubelet
 	// Prevent image GC from pruning the pause image
 	// https://github.com/kubernetes/enhancements/tree/master/keps/sig-node/2040-kubelet-cri#pinned-images
 	image := "registry.k8s.io/pause:3.9"
-	var err error
-	if image, err = b.AssetBuilder.RemapImage(image); err != nil {
-		return err
-	}
-	kubelet.PodInfraContainerImage = image
+	kubelet.PodInfraContainerImage = b.AssetBuilder.RemapImage(image)
 
 	if kubelet.FeatureGates == nil {
 		kubelet.FeatureGates = make(map[string]string)

pkg/nodemodel/nodeupconfigbuilder.go

@@ -509,7 +509,8 @@ func (n *nodeUpConfigBuilder) buildWarmPoolImages(ig *kops.InstanceGroup) []stri
 	if assetBuilder != nil {
 		for _, image := range assetBuilder.ImageAssets {
 			for _, prefix := range desiredImagePrefixes {
-				if strings.HasPrefix(image.DownloadLocation, prefix) {
+				remappedPrefix := assets.NormalizeImage(assetBuilder, prefix)
+				if strings.HasPrefix(image.DownloadLocation, remappedPrefix) {
 					images[image.DownloadLocation] = true
 				}
 			}

tests/integration/update_cluster/minimal-warmpool/data/aws_launch_template_master-us-test-1a.masters.minimal-warmpool.example.com_user_data

@@ -130,7 +130,7 @@ ClusterName: minimal-warmpool.example.com
 ConfigBase: memfs://clusters.example.com/minimal-warmpool.example.com
 InstanceGroupName: master-us-test-1a
 InstanceGroupRole: ControlPlane
-NodeupConfigHash: BGYFM1S3GrCIH6JqycbfBr9wcltsONEgcz10QbL/9DE=
+NodeupConfigHash: MVBHdgdOuzS5NjzLCOVaRI+r8Yycu79zVfQIUF7qGc0=
 
 __EOF_KUBE_ENV
 

tests/integration/update_cluster/minimal-warmpool/data/aws_launch_template_nodes.minimal-warmpool.example.com_user_data

@@ -153,7 +153,7 @@ ConfigServer:
   - https://kops-controller.internal.minimal-warmpool.example.com:3988/
 InstanceGroupName: nodes
 InstanceGroupRole: Node
-NodeupConfigHash: qx6ZYYfv4IWM31VjEaj+OnvW1dLilS7uvTY9PVeLP0c=
+NodeupConfigHash: LyRFWE+TmVjqI8Y5S+8LBIcnd15CBZ0EyvXGvEBRZcY=
 
 __EOF_KUBE_ENV