diff --git a/controllers/cli_download_controller.go b/controllers/cli_download_controller.go index 73dedb3216..6c07d7558d 100644 --- a/controllers/cli_download_controller.go +++ b/controllers/cli_download_controller.go @@ -167,11 +167,18 @@ func (c *CLIDownloadSetup) reconcileCLIResources(ctx context.Context, operatorDe } else { desired := buildCLIServerDeployment(c.Namespace, cliServerImage) needsUpdate := false - if len(deployment.Spec.Template.Spec.Containers) > 0 && - deployment.Spec.Template.Spec.Containers[0].ReadinessProbe == nil { - deployment.Spec.Template.Spec.Containers[0].ReadinessProbe = desired.Spec.Template.Spec.Containers[0].ReadinessProbe - deployment.Spec.Template.Spec.Containers[0].LivenessProbe = desired.Spec.Template.Spec.Containers[0].LivenessProbe - needsUpdate = true + if idx := findContainerIndexByName(deployment.Spec.Template.Spec.Containers, "oadp-cli-server"); idx != -1 { + desiredContainer := desired.Spec.Template.Spec.Containers[0] + currentContainer := &deployment.Spec.Template.Spec.Containers[idx] + if currentContainer.ReadinessProbe == nil { + currentContainer.ReadinessProbe = desiredContainer.ReadinessProbe + currentContainer.LivenessProbe = desiredContainer.LivenessProbe + needsUpdate = true + } + if currentContainer.StartupProbe == nil { + currentContainer.StartupProbe = desiredContainer.StartupProbe + needsUpdate = true + } } if deployment.Spec.Template.Spec.ServiceAccountName != cliServerServiceAccountName { deployment.Spec.Template.Spec.ServiceAccountName = desired.Spec.Template.Spec.ServiceAccountName @@ -187,7 +194,7 @@ func (c *CLIDownloadSetup) reconcileCLIResources(ctx context.Context, operatorDe if err := c.Client.Update(ctx, deployment); err != nil { return fmt.Errorf("failed to update CLI server deployment: %w", err) } - c.Log.Info("Updated CLI server deployment with readiness/liveness probes and/or service account") + c.Log.Info("Updated CLI server deployment with probes and/or service account") } } @@ -517,3 +524,16 @@ func buildCLIServerRoute(namespace string) *routev1.Route { func int64Ptr(i int64) *int64 { return &i } + +// findContainerIndexByName returns the index of the container with the given +// name, or -1 if no such container exists. Used to avoid assuming the target +// server container is always at index 0, which may not hold if a sidecar is +// injected or the container order changes. +func findContainerIndexByName(containers []corev1.Container, name string) int { + for i := range containers { + if containers[i].Name == name { + return i + } + } + return -1 +} diff --git a/controllers/cli_download_controller_test.go b/controllers/cli_download_controller_test.go index 99d6dffec2..5fbad91faa 100644 --- a/controllers/cli_download_controller_test.go +++ b/controllers/cli_download_controller_test.go @@ -197,3 +197,138 @@ func TestReconcileCLIResources_ReconcilesExistingServiceAccount(t *testing.T) { t.Error("expected an owner reference to be added") } } + +func TestReconcileCLIResources_BackfillsMissingStartupProbe(t *testing.T) { + namespace := "openshift-adp" + testScheme := getCLIDownloadTestScheme(t) + + operatorDeployment := &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{Name: "oadp-operator", Namespace: namespace}, + } + + // Simulate a Deployment created after readiness/liveness probes existed but + // before StartupProbe was added: ReadinessProbe/LivenessProbe are already + // set, so the old backfill gate (keyed only on ReadinessProbe == nil) would + // never fire and StartupProbe would be stuck missing forever. + existingDeployment := &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{Name: cliServerDeploymentName, Namespace: namespace}, + Spec: appsv1.DeploymentSpec{ + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + ServiceAccountName: cliServerServiceAccountName, + Containers: []corev1.Container{ + { + Name: "oadp-cli-server", + Image: "old-image", + ReadinessProbe: &corev1.Probe{ + ProbeHandler: corev1.ProbeHandler{ + HTTPGet: &corev1.HTTPGetAction{Path: "/", Port: intstr.FromString("http")}, + }, + }, + LivenessProbe: &corev1.Probe{ + ProbeHandler: corev1.ProbeHandler{ + HTTPGet: &corev1.HTTPGetAction{Path: "/", Port: intstr.FromString("http")}, + }, + }, + }, + }, + }, + }, + }, + } + + fakeClient := fake.NewClientBuilder(). + WithScheme(testScheme). + WithObjects(existingDeployment, newTestCLIRoute(namespace)). + Build() + + setup := &CLIDownloadSetup{ + Client: fakeClient, + Namespace: namespace, + OperatorName: "oadp-operator", + OperatorNamespace: namespace, + Log: logr.Discard(), + } + + if err := setup.reconcileCLIResources(context.Background(), operatorDeployment, "test-image"); err != nil { + t.Fatalf("reconcileCLIResources returned error: %v", err) + } + + updated := &appsv1.Deployment{} + if err := fakeClient.Get(context.Background(), client.ObjectKey{Name: cliServerDeploymentName, Namespace: namespace}, updated); err != nil { + t.Fatalf("failed to get updated deployment: %v", err) + } + container := updated.Spec.Template.Spec.Containers[0] + + if container.StartupProbe == nil { + t.Error("expected StartupProbe to be backfilled even though ReadinessProbe/LivenessProbe already existed") + } + // The image on an existing deployment should not be clobbered by the backfill. + if container.Image != "old-image" { + t.Errorf("expected existing image to be preserved, got %q", container.Image) + } +} + +func TestReconcileCLIResources_DoesNotOverwriteExistingStartupProbe(t *testing.T) { + namespace := "openshift-adp" + testScheme := getCLIDownloadTestScheme(t) + + operatorDeployment := &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{Name: "oadp-operator", Namespace: namespace}, + } + + customProbe := &corev1.Probe{ + ProbeHandler: corev1.ProbeHandler{ + HTTPGet: &corev1.HTTPGetAction{Path: "/custom", Port: intstr.FromString("http")}, + }, + InitialDelaySeconds: 99, + PeriodSeconds: 99, + } + + existingDeployment := &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{Name: cliServerDeploymentName, Namespace: namespace}, + Spec: appsv1.DeploymentSpec{ + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + ServiceAccountName: cliServerServiceAccountName, + Containers: []corev1.Container{ + { + Name: "oadp-cli-server", + Image: "old-image", + ReadinessProbe: customProbe.DeepCopy(), + LivenessProbe: customProbe.DeepCopy(), + StartupProbe: customProbe.DeepCopy(), + }, + }, + }, + }, + }, + } + + fakeClient := fake.NewClientBuilder(). + WithScheme(testScheme). + WithObjects(existingDeployment, newTestCLIRoute(namespace)). + Build() + + setup := &CLIDownloadSetup{ + Client: fakeClient, + Namespace: namespace, + OperatorName: "oadp-operator", + OperatorNamespace: namespace, + Log: logr.Discard(), + } + + if err := setup.reconcileCLIResources(context.Background(), operatorDeployment, "test-image"); err != nil { + t.Fatalf("reconcileCLIResources returned error: %v", err) + } + + updated := &appsv1.Deployment{} + if err := fakeClient.Get(context.Background(), client.ObjectKey{Name: cliServerDeploymentName, Namespace: namespace}, updated); err != nil { + t.Fatalf("failed to get updated deployment: %v", err) + } + container := updated.Spec.Template.Spec.Containers[0] + + if container.StartupProbe.InitialDelaySeconds != 99 { + t.Errorf("expected existing StartupProbe to be preserved, got InitialDelaySeconds=%d", container.StartupProbe.InitialDelaySeconds) + } +}