Merge pull request #10338 from blackpiglet/jxun/resolve_duplicate_initCotainer_name

Avoid duplicated InitContainer names generated in velero install CLI.

changelogs/unreleased/10338-blackpiglet

@@ -0,0 +1 @@
+Avoid duplicated InitContainer names generated in velero install CLI.

pkg/builder/container_builder.go

@@ -18,10 +18,12 @@ package builder
 
 import (
 	"encoding/json"
+	"fmt"
 	"strings"
 
 	corev1api "k8s.io/api/core/v1"
 	apimachineryRuntime "k8s.io/apimachinery/pkg/runtime"
+	utilrand "k8s.io/apimachinery/pkg/util/rand"
 
 	"github.com/vmware-tanzu/velero/pkg/label"
 )
@@ -42,15 +44,22 @@ func ForContainer(name, image string) *ContainerBuilder {
 }
 
 // ForPluginContainer is a helper builder specifically for plugin init containers
-func ForPluginContainer(image string, pullPolicy corev1api.PullPolicy) *ContainerBuilder {
+func ForPluginContainer(image string, pullPolicy corev1api.PullPolicy, existingContainers []corev1api.Container) *ContainerBuilder {
 	volumeMount := ForVolumeMount("plugins", "/target").Result()
-	return ForContainer(getName(image), image).PullPolicy(pullPolicy).VolumeMounts(volumeMount)
+	return ForContainer(getName(image, existingContainers), image).PullPolicy(pullPolicy).VolumeMounts(volumeMount)
 }
 
 // getName returns the 'name' component of a docker image that includes the entire string
 // except the registry name, and transforms the combined string into a DNS-1123 compatible name
 // that fits within the 63-character limit for Kubernetes container names.
-func getName(image string) string {
+// It appends a random string if there is a collision with existing container names.
+func getName(image string, existingContainers []corev1api.Container) string {
+	// Convert existingContainers to a map for O(1) collision lookups
+	existingNames := make(map[string]bool, len(existingContainers))
+	for _, c := range existingContainers {
+		existingNames[c.Name] = true
+	}
+
 	slashIndex := strings.Index(image, "/")
 	slashCount := 0
 	if slashIndex >= 0 {
@@ -88,7 +97,20 @@ func getName(image string) string {
 	name := re.Replace(image[start:end])
 
 	// Ensure the name doesn't exceed Kubernetes container name length limit
-	return label.GetValidName(name)
+	name = label.GetValidName(name)
+
+	for existingNames[name] {
+		name = re.Replace(image[start:end])
+		if len(name) > 57 {
+			// Leave 6 characters for "-xxxxx" random string
+			name = name[:57]
+			name = strings.TrimSuffix(name, "-")
+		}
+		name = fmt.Sprintf("%s-%s", name, utilrand.String(5))
+		name = label.GetValidName(name)
+	}
+
+	return name
 }
 
 // Result returns the built Container.

pkg/builder/container_builder_test.go

@@ -16,16 +16,19 @@ limitations under the License.
 package builder
 
 import (
+	"strings"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
+	corev1api "k8s.io/api/core/v1"
 )
 
 func TestGetName(t *testing.T) {
 	tests := []struct {
-		name     string
-		image    string
-		expected string
+		name               string
+		image              string
+		existingContainers []corev1api.Container
+		expected           string
 	}{
 		{
 			name:     "image name with registry hostname and tag",
@@ -92,11 +95,25 @@ func TestGetName(t *testing.T) {
 			image:    "quay.io/vmware-tanzu/velero@sha256:a75f9e8c3ced3943515f249597be389f8233e1258d289b11184796edceaa7dab",
 			expected: "vmware-tanzu-velero",
 		},
+		{
+			name:  "duplicate plugin name",
+			image: "gcr.io/my-repo/my-image:latest",
+			existingContainers: []corev1api.Container{
+				{Name: "my-repo-my-image"},
+			},
+			expected: "my-repo-my-image-", // we will check it has the prefix
+		},
 	}
 
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
-			assert.Equal(t, test.expected, getName(test.image))
+			if test.name == "duplicate plugin name" {
+				result := getName(test.image, test.existingContainers)
+				assert.True(t, strings.HasPrefix(result, test.expected), "expected prefix %s in %s", test.expected, result)
+				assert.Len(t, result, len(test.expected)+5)
+			} else {
+				assert.Equal(t, test.expected, getName(test.image, test.existingContainers))
+			}
 		})
 	}
 }
@@ -117,7 +134,7 @@ func TestGetNameWithLongPaths(t *testing.T) {
 				// Should be exactly 63 characters (truncated with hash)
 				assert.Len(t, result, 63)
 				// Should be deterministic
-				result2 := getName("arohcpsvcdev.azurecr.io/redhat-user-workloads/ocp-art-tenant/oadp-hypershift-oadp-plugin-main@sha256:adb840bf3890b4904a8cdda1a74c82cf8d96c52eba9944ac10e795335d6fd450")
+				result2 := getName("arohcpsvcdev.azurecr.io/redhat-user-workloads/ocp-art-tenant/oadp-hypershift-oadp-plugin-main@sha256:adb840bf3890b4904a8cdda1a74c82cf8d96c52eba9944ac10e795335d6fd450", nil)
 				assert.Equal(t, result, result2)
 			},
 		},
@@ -142,7 +159,7 @@ func TestGetNameWithLongPaths(t *testing.T) {
 
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
-			result := getName(test.image)
+			result := getName(test.image, nil)
 			test.validate(t, result)
 		})
 	}

pkg/cmd/cli/plugin/add.go

@@ -111,7 +111,7 @@ func NewAddCommand(f client.Factory) *cobra.Command {
 			}
 
 			// add the plugin as an init container
-			plugin := *builder.ForPluginContainer(args[0], corev1api.PullPolicy(imagePullPolicyFlag.String())).Result()
+			plugin := *builder.ForPluginContainer(args[0], corev1api.PullPolicy(imagePullPolicyFlag.String()), veleroDeploy.Spec.Template.Spec.InitContainers).Result()
 
 			veleroDeploy.Spec.Template.Spec.InitContainers = append(veleroDeploy.Spec.Template.Spec.InitContainers, plugin)
 

pkg/install/deployment.go

@@ -532,7 +532,7 @@ func Deployment(namespace string, opts ...podTemplateOption) *appsv1api.Deployme
 
 	if len(c.plugins) > 0 {
 		for _, image := range c.plugins {
-			container := *builder.ForPluginContainer(image, pullPolicy).Result()
+			container := *builder.ForPluginContainer(image, pullPolicy, deployment.Spec.Template.Spec.InitContainers).Result()
 			deployment.Spec.Template.Spec.InitContainers = append(deployment.Spec.Template.Spec.InitContainers, container)
 		}
 	}