diff --git a/changelogs/unreleased/10338-blackpiglet b/changelogs/unreleased/10338-blackpiglet new file mode 100644 index 000000000..bf45dad9e --- /dev/null +++ b/changelogs/unreleased/10338-blackpiglet @@ -0,0 +1 @@ +Avoid duplicated InitContainer names generated in velero install CLI. \ No newline at end of file diff --git a/pkg/builder/container_builder.go b/pkg/builder/container_builder.go index 762462c86..e25002629 100644 --- a/pkg/builder/container_builder.go +++ b/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. diff --git a/pkg/builder/container_builder_test.go b/pkg/builder/container_builder_test.go index b23cbddfd..e0af71f75 100644 --- a/pkg/builder/container_builder_test.go +++ b/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) }) } diff --git a/pkg/cmd/cli/plugin/add.go b/pkg/cmd/cli/plugin/add.go index 45a112a46..553a217dc 100644 --- a/pkg/cmd/cli/plugin/add.go +++ b/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) diff --git a/pkg/install/deployment.go b/pkg/install/deployment.go index f2e8219c9..642a51321 100644 --- a/pkg/install/deployment.go +++ b/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) } }