diff --git a/internal/controller/cli_download_controller.go b/internal/controller/cli_download_controller.go index 537804347b..7cd9cff907 100644 --- a/internal/controller/cli_download_controller.go +++ b/internal/controller/cli_download_controller.go @@ -167,14 +167,22 @@ func (c *CLIDownloadSetup) reconcileCLIResources(ctx context.Context, operatorDe c.Log.Info("Created CLI server deployment", "image", cliServerImage) } else if err != nil { return fmt.Errorf("failed to get CLI server deployment: %w", err) - } else { + } else if idx := findContainerIndexByName(deployment.Spec.Template.Spec.Containers, "oadp-cli-server"); idx != -1 { // Deployment exists from a version before probes/service account were added; backfill them. desired := buildCLIServerDeployment(c.Namespace, cliServerImage) + desiredContainer := desired.Spec.Template.Spec.Containers[0] + currentContainer := &deployment.Spec.Template.Spec.Containers[idx] 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 + if currentContainer.ReadinessProbe == nil && desiredContainer.ReadinessProbe != nil { + currentContainer.ReadinessProbe = desiredContainer.ReadinessProbe + needsUpdate = true + } + if currentContainer.LivenessProbe == nil && desiredContainer.LivenessProbe != nil { + currentContainer.LivenessProbe = desiredContainer.LivenessProbe + needsUpdate = true + } + if currentContainer.StartupProbe == nil && desiredContainer.StartupProbe != nil { + currentContainer.StartupProbe = desiredContainer.StartupProbe needsUpdate = true } if deployment.Spec.Template.Spec.ServiceAccountName != cliServerServiceAccountName { @@ -191,7 +199,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, service account, and/or automount setting") } } @@ -424,6 +432,17 @@ func buildCLIServerDeployment(namespace, image string) *appsv1.Deployment { }, ReadOnlyRootFilesystem: &readOnlyRootFilesystem, }, + StartupProbe: &corev1.Probe{ + ProbeHandler: corev1.ProbeHandler{ + HTTPGet: &corev1.HTTPGetAction{ + Path: "/", + Port: intstr.FromString("http"), + }, + }, + InitialDelaySeconds: 5, + PeriodSeconds: 5, + FailureThreshold: 12, + }, ReadinessProbe: &corev1.Probe{ ProbeHandler: corev1.ProbeHandler{ HTTPGet: &corev1.HTTPGetAction{ @@ -524,3 +543,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/internal/controller/cli_download_controller_test.go b/internal/controller/cli_download_controller_test.go index 60bd7b095b..a73b643e39 100644 --- a/internal/controller/cli_download_controller_test.go +++ b/internal/controller/cli_download_controller_test.go @@ -2,17 +2,216 @@ package controller import ( "context" + "strings" "testing" "github.com/go-logr/logr" + consolev1 "github.com/openshift/api/console/v1" + routev1 "github.com/openshift/api/route/v1" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" + "k8s.io/apimachinery/pkg/util/intstr" + "k8s.io/client-go/kubernetes/scheme" + "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" ) +// getDownloadTestScheme returns a scheme with the console/route API groups +// registered, suitable for use with a fake client in CLI/VMDP reconciliation tests. +func getDownloadTestScheme(t *testing.T) *runtime.Scheme { + t.Helper() + if err := consolev1.AddToScheme(scheme.Scheme); err != nil { + t.Fatalf("failed to add consolev1 to scheme: %v", err) + } + if err := routev1.AddToScheme(scheme.Scheme); err != nil { + t.Fatalf("failed to add routev1 to scheme: %v", err) + } + return scheme.Scheme +} + +// newTestCLIRoute returns a CLI server route with a hostname already assigned, +// so reconcileCLIResources doesn't hit its hostname-assignment retry/backoff path. +func newTestCLIRoute(namespace string) *routev1.Route { + route := buildCLIServerRoute(namespace) + route.Spec.Host = "cli.example.com" + return route +} + +func TestBuildCLIServerDeployment_StartupProbe(t *testing.T) { + deployment := buildCLIServerDeployment("openshift-adp", "test-image") + + if len(deployment.Spec.Template.Spec.Containers) == 0 { + t.Fatal("expected at least one container") + } + container := deployment.Spec.Template.Spec.Containers[0] + + if container.StartupProbe == nil { + t.Fatal("expected StartupProbe to be set") + } + if container.StartupProbe.HTTPGet == nil { + t.Fatal("expected StartupProbe to use HTTPGet") + } + if container.StartupProbe.HTTPGet.Path != "/" { + t.Errorf("expected StartupProbe path \"/\", got %q", container.StartupProbe.HTTPGet.Path) + } + if container.StartupProbe.HTTPGet.Port != intstr.FromString("http") { + t.Errorf("expected StartupProbe port \"http\", got %v", container.StartupProbe.HTTPGet.Port) + } + if container.StartupProbe.FailureThreshold != 12 { + t.Errorf("expected StartupProbe failureThreshold 12, got %d", container.StartupProbe.FailureThreshold) + } + if container.StartupProbe.InitialDelaySeconds != 5 { + t.Errorf("expected StartupProbe initialDelaySeconds 5, got %d", container.StartupProbe.InitialDelaySeconds) + } + if container.StartupProbe.PeriodSeconds != 5 { + t.Errorf("expected StartupProbe periodSeconds 5, got %d", container.StartupProbe.PeriodSeconds) + } + + if container.ReadinessProbe == nil { + t.Fatal("expected ReadinessProbe to remain set") + } + if container.LivenessProbe == nil { + t.Fatal("expected LivenessProbe to remain set") + } +} + +func TestReconcileCLIResources_BackfillsMissingProbes(t *testing.T) { + namespace := "openshift-adp" + testScheme := getDownloadTestScheme(t) + + operatorDeployment := &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{Name: "oadp-operator", Namespace: namespace}, + } + + // Simulate a Deployment created by a pre-fix version of the operator: it has + // the server container, but no probes configured at all. + existingDeployment := &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{Name: cliServerDeploymentName, Namespace: namespace}, + Spec: appsv1.DeploymentSpec{ + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + {Name: "oadp-cli-server", Image: "old-image"}, + }, + }, + }, + }, + } + + 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.ReadinessProbe == nil { + t.Error("expected ReadinessProbe to be backfilled") + } + if container.LivenessProbe == nil { + t.Error("expected LivenessProbe to be backfilled") + } + if container.StartupProbe == nil { + t.Error("expected StartupProbe to be backfilled") + } + // 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_DoesNotOverwriteExistingProbes(t *testing.T) { + namespace := "openshift-adp" + testScheme := getDownloadTestScheme(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, + } + + // Simulate a Deployment that already has probes configured (e.g. from a + // prior reconcile, or customized by a user) to ensure the backfill logic + // doesn't clobber them. + existingDeployment := &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{Name: cliServerDeploymentName, Namespace: namespace}, + Spec: appsv1.DeploymentSpec{ + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + 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.ReadinessProbe.InitialDelaySeconds != 99 { + t.Errorf("expected existing ReadinessProbe to be preserved, got InitialDelaySeconds=%d", container.ReadinessProbe.InitialDelaySeconds) + } + if container.LivenessProbe.InitialDelaySeconds != 99 { + t.Errorf("expected existing LivenessProbe to be preserved, got InitialDelaySeconds=%d", container.LivenessProbe.InitialDelaySeconds) + } + if container.StartupProbe.InitialDelaySeconds != 99 { + t.Errorf("expected existing StartupProbe to be preserved, got InitialDelaySeconds=%d", container.StartupProbe.InitialDelaySeconds) + } +} + func TestBuildCLIServerDeployment_ServiceAccount(t *testing.T) { deployment := buildCLIServerDeployment("openshift-adp", "test-image") podSpec := deployment.Spec.Template.Spec @@ -51,14 +250,28 @@ func TestBuildCLIServerServiceAccount(t *testing.T) { // ignored: by that point the ServiceAccount step (step 1) has already run // and persisted its result, which is what these tests verify. func newCLITestScheme(t *testing.T) *runtime.Scheme { - scheme := runtime.NewScheme() - if err := corev1.AddToScheme(scheme); err != nil { + testScheme := runtime.NewScheme() + if err := corev1.AddToScheme(testScheme); err != nil { t.Fatal(err) } - if err := appsv1.AddToScheme(scheme); err != nil { + if err := appsv1.AddToScheme(testScheme); err != nil { t.Fatal(err) } - return scheme + return testScheme +} + +// assertExpectedRouteSchemeError fails the test unless err is the intentional +// error caused by newCLITestScheme omitting routev1/consolev1: reconcile is +// expected to fail once it reaches the Route step, but only after the +// ServiceAccount/Deployment steps under test have already run and persisted +// their results. Any other error indicates a real regression in an earlier +// step and must not be silently treated as "expected". +func assertExpectedRouteSchemeError(t *testing.T, err error) { + t.Helper() + if !strings.Contains(err.Error(), "no kind is registered for the type v1.Route") { + t.Fatalf("expected error from unregistered Route scheme, got unexpected error: %v", err) + } + t.Logf("got expected error (Route/ConsoleCLIDownload step unregistered in test scheme): %v", err) } func newCLITestOperatorDeployment() *appsv1.Deployment { @@ -75,15 +288,15 @@ func newCLITestOperatorDeployment() *appsv1.Deployment { // ServiceAccount is created with the desired state when it doesn't exist. func TestReconcileCLIResources_CreatesServiceAccountWhenMissing(t *testing.T) { const ns = "openshift-adp" - scheme := newCLITestScheme(t) + testScheme := newCLITestScheme(t) operatorDeploy := newCLITestOperatorDeployment() - fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(operatorDeploy).Build() + fakeClient := fake.NewClientBuilder().WithScheme(testScheme).WithObjects(operatorDeploy).Build() setup := &CLIDownloadSetup{Client: fakeClient, Namespace: ns, Log: logr.Discard()} // Expect an error once reconcileCLIResources reaches the unregistered // Route/ConsoleCLIDownload steps — irrelevant to this test. if err := setup.reconcileCLIResources(context.Background(), operatorDeploy, "test-image"); err != nil { - t.Logf("reconcileCLIResources returned expected error (Route/ConsoleCLIDownload step unregistered in test scheme): %v", err) + assertExpectedRouteSchemeError(t, err) } got := &corev1.ServiceAccount{} @@ -108,7 +321,7 @@ func TestReconcileCLIResources_CreatesServiceAccountWhenMissing(t *testing.T) { func TestReconcileCLIResources_FixesExistingServiceAccountDrift(t *testing.T) { const ns = "openshift-adp" trueVal := true - scheme := newCLITestScheme(t) + testScheme := newCLITestScheme(t) operatorDeploy := newCLITestOperatorDeployment() existing := &corev1.ServiceAccount{ @@ -120,11 +333,11 @@ func TestReconcileCLIResources_FixesExistingServiceAccountDrift(t *testing.T) { AutomountServiceAccountToken: &trueVal, // wrong: should be false } - fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(operatorDeploy, existing).Build() + fakeClient := fake.NewClientBuilder().WithScheme(testScheme).WithObjects(operatorDeploy, existing).Build() setup := &CLIDownloadSetup{Client: fakeClient, Namespace: ns, Log: logr.Discard()} if err := setup.reconcileCLIResources(context.Background(), operatorDeploy, "test-image"); err != nil { - t.Logf("reconcileCLIResources returned expected error (Route/ConsoleCLIDownload step unregistered in test scheme): %v", err) + assertExpectedRouteSchemeError(t, err) } got := &corev1.ServiceAccount{} @@ -159,7 +372,7 @@ func TestReconcileCLIResources_FixesExistingServiceAccountDrift(t *testing.T) { func TestReconcileCLIResources_FixesMissingOwnerReferenceOnly(t *testing.T) { const ns = "openshift-adp" falseVal := false - scheme := newCLITestScheme(t) + testScheme := newCLITestScheme(t) operatorDeploy := newCLITestOperatorDeployment() existing := &corev1.ServiceAccount{ @@ -176,11 +389,11 @@ func TestReconcileCLIResources_FixesMissingOwnerReferenceOnly(t *testing.T) { AutomountServiceAccountToken: &falseVal, } - fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(operatorDeploy, existing).Build() + fakeClient := fake.NewClientBuilder().WithScheme(testScheme).WithObjects(operatorDeploy, existing).Build() setup := &CLIDownloadSetup{Client: fakeClient, Namespace: ns, Log: logr.Discard()} if err := setup.reconcileCLIResources(context.Background(), operatorDeploy, "test-image"); err != nil { - t.Logf("reconcileCLIResources returned expected error (Route/ConsoleCLIDownload step unregistered in test scheme): %v", err) + assertExpectedRouteSchemeError(t, err) } got := &corev1.ServiceAccount{} @@ -202,10 +415,14 @@ func TestReconcileCLIResources_FixesMissingOwnerReferenceOnly(t *testing.T) { // TestReconcileCLIResources_NoopWhenServiceAccountAlreadyCorrect verifies a // ServiceAccount already matching the desired state is not unnecessarily -// updated (no ResourceVersion bump). +// updated. Asserts on the number of Update calls the fake client actually +// received (via an interceptor) rather than comparing ResourceVersion +// before/after, since the fake client's ResourceVersion bookkeeping is only +// an approximation of a real API server's and isn't a reliable signal that +// no update occurred. func TestReconcileCLIResources_NoopWhenServiceAccountAlreadyCorrect(t *testing.T) { const ns = "openshift-adp" - scheme := newCLITestScheme(t) + testScheme := newCLITestScheme(t) operatorDeploy := newCLITestOperatorDeployment() desired := buildCLIServerServiceAccount(ns) @@ -213,24 +430,24 @@ func TestReconcileCLIResources_NoopWhenServiceAccountAlreadyCorrect(t *testing.T {UID: operatorDeploy.UID, Name: operatorDeploy.Name, Kind: "Deployment", APIVersion: "apps/v1"}, } - fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(operatorDeploy, desired.DeepCopy()).Build() - - before := &corev1.ServiceAccount{} - if err := fakeClient.Get(context.Background(), types.NamespacedName{Name: cliServerServiceAccountName, Namespace: ns}, before); err != nil { - t.Fatalf("failed to get SA: %v", err) - } + updateCalls := 0 + fakeClient := fake.NewClientBuilder(). + WithScheme(testScheme). + WithObjects(operatorDeploy, desired.DeepCopy()). + WithInterceptorFuncs(interceptor.Funcs{ + Update: func(ctx context.Context, c client.WithWatch, obj client.Object, opts ...client.UpdateOption) error { + updateCalls++ + return c.Update(ctx, obj, opts...) + }, + }). + Build() setup := &CLIDownloadSetup{Client: fakeClient, Namespace: ns, Log: logr.Discard()} if err := setup.reconcileCLIResources(context.Background(), operatorDeploy, "test-image"); err != nil { - t.Logf("reconcileCLIResources returned expected error (Route/ConsoleCLIDownload step unregistered in test scheme): %v", err) - } - - after := &corev1.ServiceAccount{} - if err := fakeClient.Get(context.Background(), types.NamespacedName{Name: cliServerServiceAccountName, Namespace: ns}, after); err != nil { - t.Fatalf("failed to get SA: %v", err) + assertExpectedRouteSchemeError(t, err) } - if before.ResourceVersion != after.ResourceVersion { - t.Errorf("expected no update (ResourceVersion unchanged), got before=%s after=%s", before.ResourceVersion, after.ResourceVersion) + if updateCalls != 0 { + t.Errorf("expected no Update calls when ServiceAccount already matches desired state, got %d", updateCalls) } } diff --git a/internal/controller/vmdp_download_controller.go b/internal/controller/vmdp_download_controller.go index 92a19d1268..b30a38c757 100644 --- a/internal/controller/vmdp_download_controller.go +++ b/internal/controller/vmdp_download_controller.go @@ -162,14 +162,22 @@ func (v *VMDPDownloadSetup) reconcileVMDPResources(ctx context.Context, operator v.Log.Info("Created VMDP server deployment", "image", vmdpServerImage) } else if err != nil { return fmt.Errorf("failed to get VMDP server deployment: %w", err) - } else { + } else if idx := findContainerIndexByName(deployment.Spec.Template.Spec.Containers, "oadp-vmdp-server"); idx != -1 { // Deployment exists from a version before probes/service account were added; backfill them. desired := buildVMDPServerDeployment(v.Namespace, vmdpServerImage) + desiredContainer := desired.Spec.Template.Spec.Containers[0] + currentContainer := &deployment.Spec.Template.Spec.Containers[idx] 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 + if currentContainer.ReadinessProbe == nil && desiredContainer.ReadinessProbe != nil { + currentContainer.ReadinessProbe = desiredContainer.ReadinessProbe + needsUpdate = true + } + if currentContainer.LivenessProbe == nil && desiredContainer.LivenessProbe != nil { + currentContainer.LivenessProbe = desiredContainer.LivenessProbe + needsUpdate = true + } + if currentContainer.StartupProbe == nil && desiredContainer.StartupProbe != nil { + currentContainer.StartupProbe = desiredContainer.StartupProbe needsUpdate = true } if deployment.Spec.Template.Spec.ServiceAccountName != vmdpServerServiceAccountName { @@ -186,7 +194,7 @@ func (v *VMDPDownloadSetup) reconcileVMDPResources(ctx context.Context, operator if err := v.Client.Update(ctx, deployment); err != nil { return fmt.Errorf("failed to update VMDP server deployment: %w", err) } - v.Log.Info("Updated VMDP server deployment with readiness/liveness probes and/or service account") + v.Log.Info("Updated VMDP server deployment with probes, service account, and/or automount setting") } } @@ -406,6 +414,17 @@ func buildVMDPServerDeployment(namespace, image string) *appsv1.Deployment { }, ReadOnlyRootFilesystem: &readOnlyRootFilesystem, }, + StartupProbe: &corev1.Probe{ + ProbeHandler: corev1.ProbeHandler{ + HTTPGet: &corev1.HTTPGetAction{ + Path: "/", + Port: intstr.FromString("http"), + }, + }, + InitialDelaySeconds: 5, + PeriodSeconds: 5, + FailureThreshold: 12, + }, ReadinessProbe: &corev1.Probe{ ProbeHandler: corev1.ProbeHandler{ HTTPGet: &corev1.HTTPGetAction{ diff --git a/internal/controller/vmdp_download_controller_test.go b/internal/controller/vmdp_download_controller_test.go index 3212079365..52265d78cb 100644 --- a/internal/controller/vmdp_download_controller_test.go +++ b/internal/controller/vmdp_download_controller_test.go @@ -5,12 +5,197 @@ import ( "testing" "github.com/go-logr/logr" + routev1 "github.com/openshift/api/route/v1" + appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" + "k8s.io/apimachinery/pkg/util/intstr" + "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" ) +// newTestVMDPRoute returns a VMDP server route with a hostname already +// assigned, so reconcileVMDPResources doesn't hit its hostname-assignment +// retry/backoff path. +func newTestVMDPRoute(namespace string) *routev1.Route { + route := buildVMDPServerRoute(namespace) + route.Spec.Host = "vmdp.example.com" + return route +} + +func TestBuildVMDPServerDeployment_StartupProbe(t *testing.T) { + deployment := buildVMDPServerDeployment("openshift-adp", "test-image") + + if len(deployment.Spec.Template.Spec.Containers) == 0 { + t.Fatal("expected at least one container") + } + container := deployment.Spec.Template.Spec.Containers[0] + + if container.StartupProbe == nil { + t.Fatal("expected StartupProbe to be set") + } + if container.StartupProbe.HTTPGet == nil { + t.Fatal("expected StartupProbe to use HTTPGet") + } + if container.StartupProbe.HTTPGet.Path != "/" { + t.Errorf("expected StartupProbe path \"/\", got %q", container.StartupProbe.HTTPGet.Path) + } + if container.StartupProbe.HTTPGet.Port != intstr.FromString("http") { + t.Errorf("expected StartupProbe port \"http\", got %v", container.StartupProbe.HTTPGet.Port) + } + if container.StartupProbe.FailureThreshold != 12 { + t.Errorf("expected StartupProbe failureThreshold 12, got %d", container.StartupProbe.FailureThreshold) + } + if container.StartupProbe.InitialDelaySeconds != 5 { + t.Errorf("expected StartupProbe initialDelaySeconds 5, got %d", container.StartupProbe.InitialDelaySeconds) + } + if container.StartupProbe.PeriodSeconds != 5 { + t.Errorf("expected StartupProbe periodSeconds 5, got %d", container.StartupProbe.PeriodSeconds) + } + + if container.ReadinessProbe == nil { + t.Fatal("expected ReadinessProbe to remain set") + } + if container.LivenessProbe == nil { + t.Fatal("expected LivenessProbe to remain set") + } +} + +func TestReconcileVMDPResources_BackfillsMissingProbes(t *testing.T) { + namespace := "openshift-adp" + testScheme := getDownloadTestScheme(t) + + operatorDeployment := &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{Name: "oadp-operator", Namespace: namespace}, + } + + // Simulate a Deployment created by a pre-fix version of the operator: it has + // the server container, but no probes configured at all. + existingDeployment := &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{Name: vmdpServerDeploymentName, Namespace: namespace}, + Spec: appsv1.DeploymentSpec{ + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + {Name: "oadp-vmdp-server", Image: "old-image"}, + }, + }, + }, + }, + } + + fakeClient := fake.NewClientBuilder(). + WithScheme(testScheme). + WithObjects(existingDeployment, newTestVMDPRoute(namespace)). + Build() + + setup := &VMDPDownloadSetup{ + Client: fakeClient, + Namespace: namespace, + OperatorName: "oadp-operator", + OperatorNamespace: namespace, + Log: logr.Discard(), + } + + if err := setup.reconcileVMDPResources(context.Background(), operatorDeployment, "test-image"); err != nil { + t.Fatalf("reconcileVMDPResources returned error: %v", err) + } + + updated := &appsv1.Deployment{} + if err := fakeClient.Get(context.Background(), client.ObjectKey{Name: vmdpServerDeploymentName, Namespace: namespace}, updated); err != nil { + t.Fatalf("failed to get updated deployment: %v", err) + } + container := updated.Spec.Template.Spec.Containers[0] + + if container.ReadinessProbe == nil { + t.Error("expected ReadinessProbe to be backfilled") + } + if container.LivenessProbe == nil { + t.Error("expected LivenessProbe to be backfilled") + } + if container.StartupProbe == nil { + t.Error("expected StartupProbe to be backfilled") + } + // 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 TestReconcileVMDPResources_DoesNotOverwriteExistingProbes(t *testing.T) { + namespace := "openshift-adp" + testScheme := getDownloadTestScheme(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, + } + + // Simulate a Deployment that already has probes configured (e.g. from a + // prior reconcile, or customized by a user) to ensure the backfill logic + // doesn't clobber them. + existingDeployment := &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{Name: vmdpServerDeploymentName, Namespace: namespace}, + Spec: appsv1.DeploymentSpec{ + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + { + Name: "oadp-vmdp-server", + Image: "old-image", + ReadinessProbe: customProbe.DeepCopy(), + LivenessProbe: customProbe.DeepCopy(), + StartupProbe: customProbe.DeepCopy(), + }, + }, + }, + }, + }, + } + + fakeClient := fake.NewClientBuilder(). + WithScheme(testScheme). + WithObjects(existingDeployment, newTestVMDPRoute(namespace)). + Build() + + setup := &VMDPDownloadSetup{ + Client: fakeClient, + Namespace: namespace, + OperatorName: "oadp-operator", + OperatorNamespace: namespace, + Log: logr.Discard(), + } + + if err := setup.reconcileVMDPResources(context.Background(), operatorDeployment, "test-image"); err != nil { + t.Fatalf("reconcileVMDPResources returned error: %v", err) + } + + updated := &appsv1.Deployment{} + if err := fakeClient.Get(context.Background(), client.ObjectKey{Name: vmdpServerDeploymentName, Namespace: namespace}, updated); err != nil { + t.Fatalf("failed to get updated deployment: %v", err) + } + container := updated.Spec.Template.Spec.Containers[0] + + if container.ReadinessProbe.InitialDelaySeconds != 99 { + t.Errorf("expected existing ReadinessProbe to be preserved, got InitialDelaySeconds=%d", container.ReadinessProbe.InitialDelaySeconds) + } + if container.LivenessProbe.InitialDelaySeconds != 99 { + t.Errorf("expected existing LivenessProbe to be preserved, got InitialDelaySeconds=%d", container.LivenessProbe.InitialDelaySeconds) + } + if container.StartupProbe.InitialDelaySeconds != 99 { + t.Errorf("expected existing StartupProbe to be preserved, got InitialDelaySeconds=%d", container.StartupProbe.InitialDelaySeconds) + } +} + func TestBuildVMDPServerDeployment_ServiceAccount(t *testing.T) { deployment := buildVMDPServerDeployment("openshift-adp", "test-image") podSpec := deployment.Spec.Template.Spec @@ -51,13 +236,13 @@ func TestBuildVMDPServerServiceAccount(t *testing.T) { // ServiceAccount step under test has already run. func TestReconcileVMDPResources_CreatesServiceAccountWhenMissing(t *testing.T) { const ns = "openshift-adp" - scheme := newCLITestScheme(t) + testScheme := newCLITestScheme(t) operatorDeploy := newCLITestOperatorDeployment() - fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(operatorDeploy).Build() + fakeClient := fake.NewClientBuilder().WithScheme(testScheme).WithObjects(operatorDeploy).Build() setup := &VMDPDownloadSetup{Client: fakeClient, Namespace: ns, Log: logr.Discard()} if err := setup.reconcileVMDPResources(context.Background(), operatorDeploy, "test-image"); err != nil { - t.Logf("reconcileVMDPResources returned expected error (Route/ConsoleCLIDownload step unregistered in test scheme): %v", err) + assertExpectedRouteSchemeError(t, err) } got := &corev1.ServiceAccount{} @@ -82,7 +267,7 @@ func TestReconcileVMDPResources_CreatesServiceAccountWhenMissing(t *testing.T) { func TestReconcileVMDPResources_FixesExistingServiceAccountDrift(t *testing.T) { const ns = "openshift-adp" trueVal := true - scheme := newCLITestScheme(t) + testScheme := newCLITestScheme(t) operatorDeploy := newCLITestOperatorDeployment() existing := &corev1.ServiceAccount{ @@ -94,11 +279,11 @@ func TestReconcileVMDPResources_FixesExistingServiceAccountDrift(t *testing.T) { AutomountServiceAccountToken: &trueVal, // wrong: should be false } - fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(operatorDeploy, existing).Build() + fakeClient := fake.NewClientBuilder().WithScheme(testScheme).WithObjects(operatorDeploy, existing).Build() setup := &VMDPDownloadSetup{Client: fakeClient, Namespace: ns, Log: logr.Discard()} if err := setup.reconcileVMDPResources(context.Background(), operatorDeploy, "test-image"); err != nil { - t.Logf("reconcileVMDPResources returned expected error (Route/ConsoleCLIDownload step unregistered in test scheme): %v", err) + assertExpectedRouteSchemeError(t, err) } got := &corev1.ServiceAccount{} @@ -133,7 +318,7 @@ func TestReconcileVMDPResources_FixesExistingServiceAccountDrift(t *testing.T) { func TestReconcileVMDPResources_FixesMissingOwnerReferenceOnly(t *testing.T) { const ns = "openshift-adp" falseVal := false - scheme := newCLITestScheme(t) + testScheme := newCLITestScheme(t) operatorDeploy := newCLITestOperatorDeployment() existing := &corev1.ServiceAccount{ @@ -150,11 +335,11 @@ func TestReconcileVMDPResources_FixesMissingOwnerReferenceOnly(t *testing.T) { AutomountServiceAccountToken: &falseVal, } - fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(operatorDeploy, existing).Build() + fakeClient := fake.NewClientBuilder().WithScheme(testScheme).WithObjects(operatorDeploy, existing).Build() setup := &VMDPDownloadSetup{Client: fakeClient, Namespace: ns, Log: logr.Discard()} if err := setup.reconcileVMDPResources(context.Background(), operatorDeploy, "test-image"); err != nil { - t.Logf("reconcileVMDPResources returned expected error (Route/ConsoleCLIDownload step unregistered in test scheme): %v", err) + assertExpectedRouteSchemeError(t, err) } got := &corev1.ServiceAccount{} @@ -176,10 +361,14 @@ func TestReconcileVMDPResources_FixesMissingOwnerReferenceOnly(t *testing.T) { // TestReconcileVMDPResources_NoopWhenServiceAccountAlreadyCorrect verifies a // ServiceAccount already matching the desired state is not unnecessarily -// updated (no ResourceVersion bump). +// updated. Asserts on the number of Update calls the fake client actually +// received (via an interceptor) rather than comparing ResourceVersion +// before/after, since the fake client's ResourceVersion bookkeeping is only +// an approximation of a real API server's and isn't a reliable signal that +// no update occurred. func TestReconcileVMDPResources_NoopWhenServiceAccountAlreadyCorrect(t *testing.T) { const ns = "openshift-adp" - scheme := newCLITestScheme(t) + testScheme := newCLITestScheme(t) operatorDeploy := newCLITestOperatorDeployment() desired := buildVMDPServerServiceAccount(ns) @@ -187,24 +376,24 @@ func TestReconcileVMDPResources_NoopWhenServiceAccountAlreadyCorrect(t *testing. {UID: operatorDeploy.UID, Name: operatorDeploy.Name, Kind: "Deployment", APIVersion: "apps/v1"}, } - fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(operatorDeploy, desired.DeepCopy()).Build() - - before := &corev1.ServiceAccount{} - if err := fakeClient.Get(context.Background(), types.NamespacedName{Name: vmdpServerServiceAccountName, Namespace: ns}, before); err != nil { - t.Fatalf("failed to get SA: %v", err) - } + updateCalls := 0 + fakeClient := fake.NewClientBuilder(). + WithScheme(testScheme). + WithObjects(operatorDeploy, desired.DeepCopy()). + WithInterceptorFuncs(interceptor.Funcs{ + Update: func(ctx context.Context, c client.WithWatch, obj client.Object, opts ...client.UpdateOption) error { + updateCalls++ + return c.Update(ctx, obj, opts...) + }, + }). + Build() setup := &VMDPDownloadSetup{Client: fakeClient, Namespace: ns, Log: logr.Discard()} if err := setup.reconcileVMDPResources(context.Background(), operatorDeploy, "test-image"); err != nil { - t.Logf("reconcileVMDPResources returned expected error (Route/ConsoleCLIDownload step unregistered in test scheme): %v", err) - } - - after := &corev1.ServiceAccount{} - if err := fakeClient.Get(context.Background(), types.NamespacedName{Name: vmdpServerServiceAccountName, Namespace: ns}, after); err != nil { - t.Fatalf("failed to get SA: %v", err) + assertExpectedRouteSchemeError(t, err) } - if before.ResourceVersion != after.ResourceVersion { - t.Errorf("expected no update (ResourceVersion unchanged), got before=%s after=%s", before.ResourceVersion, after.ResourceVersion) + if updateCalls != 0 { + t.Errorf("expected no Update calls when ServiceAccount already matches desired state, got %d", updateCalls) } }