Merge pull request #145 from laurafitzgerald/RHOAIENG-47147

CARRY: set enableIngress to false

Author: laurafitzgerald

ray-operator/controllers/ray/authentication_controller.go

@@ -214,9 +214,14 @@ func (r *AuthenticationController) handleOIDCConfiguration(ctx context.Context,
 		return nil
 	}
 
-	// Ensure ingress is enabled for httproute creation
-	if err := r.ensureIngressEnabled(ctx, rayCluster, logger); err != nil {
-		return fmt.Errorf("failed to ensure ingress enabled: %w", err)
+	// On OpenShift, enforce enableIngress: false for Gateway-only access
+	// This handles both new clusters (webhook sets it) and existing clusters (upgrade scenario)
+	// HTTPRoute creation is independent of enableIngress
+	// enableIngress controls basic Route/Ingress creation (which may be disabled by webhooks in ODH)
+	if r.options.IsOpenShift {
+		if err := r.enforceEnableIngressFalseOnOpenShift(ctx, rayCluster, logger); err != nil {
+			return err
+		}
 	}
 
 	// Add finalizer to ensure cleanup happens before cluster deletion
@@ -241,31 +246,6 @@ func (r *AuthenticationController) handleOIDCConfiguration(ctx context.Context,
 	return nil
 }
 
-// ensureIngressEnabled ensures that ingress is enabled for the RayCluster
-func (r *AuthenticationController) ensureIngressEnabled(ctx context.Context, cluster *rayv1.RayCluster, logger logr.Logger) error {
-	// Check if ingress is already enabled
-	if cluster.Spec.HeadGroupSpec.EnableIngress != nil && *cluster.Spec.HeadGroupSpec.EnableIngress {
-		logger.Info("Ingress already enabled for cluster", "cluster", cluster.Name)
-		return nil
-	}
-
-	logger.Info("Enabling ingress for Auth-secured cluster", "cluster", cluster.Name)
-
-	// Create a copy and enable ingress
-	updatedCluster := cluster.DeepCopy()
-	trueValue := true
-	updatedCluster.Spec.HeadGroupSpec.EnableIngress = &trueValue
-
-	// Update the cluster
-	if err := r.Update(ctx, updatedCluster); err != nil {
-		return fmt.Errorf("failed to enable ingress: %w", err)
-	}
-
-	logger.Info("Successfully enabled ingress for cluster", "cluster", cluster.Name)
-	r.Recorder.Event(cluster, "Normal", "IngressEnabled", "Automatically enabled ingress for Auth configuration")
-	return nil
-}
-
 func (r *AuthenticationController) ensureOIDCResources(ctx context.Context, cluster *rayv1.RayCluster, authMode utils.AuthenticationMode, logger logr.Logger) error {
 	if err := r.ensureServiceAccount(ctx, cluster, authMode, logger); err != nil {
 		return fmt.Errorf("failed to ensure service account: %w", err)
@@ -1016,6 +996,46 @@ func (r *AuthenticationController) cleanupOrphanedReferenceGrant(ctx context.Con
 	return nil
 }
 
+// enforceEnableIngressFalseOnOpenShift ensures enableIngress is set to false on OpenShift
+// This provides upgrade idempotency - handles existing clusters that weren't processed by webhook
+// The webhook sets this for new/edited clusters, this handles clusters from before the webhook was updated
+// Placed in Authentication controller because HTTPRoute requires Gateway access (not basic Routes)
+func (r *AuthenticationController) enforceEnableIngressFalseOnOpenShift(ctx context.Context, rayCluster *rayv1.RayCluster, logger logr.Logger) error {
+	// Check if enableIngress is already false
+	if rayCluster.Spec.HeadGroupSpec.EnableIngress != nil && !*rayCluster.Spec.HeadGroupSpec.EnableIngress {
+		return nil // Already false, nothing to do
+	}
+
+	// Need to set to false
+	wasTrue := rayCluster.Spec.HeadGroupSpec.EnableIngress != nil && *rayCluster.Spec.HeadGroupSpec.EnableIngress
+	wasMissing := rayCluster.Spec.HeadGroupSpec.EnableIngress == nil
+
+	logger.Info("Enforcing enableIngress to false on OpenShift (Gateway-only access)",
+		"cluster", rayCluster.Name, "previousValue", rayCluster.Spec.HeadGroupSpec.EnableIngress)
+
+	// Update the cluster spec
+	rayCluster.Spec.HeadGroupSpec.EnableIngress = ptr.To(false)
+	if err := r.Update(ctx, rayCluster); err != nil {
+		logger.Error(err, "Failed to enforce enableIngress to false", "cluster", rayCluster.Name)
+		return err
+	}
+
+	if wasTrue {
+		logger.Info("Overrode user-specified enableIngress from true to false for Gateway-only access",
+			"cluster", rayCluster.Name)
+		r.Recorder.Eventf(rayCluster, corev1.EventTypeNormal,
+			"EnableIngressEnforced",
+			"Set enableIngress to false to enforce Gateway-only access (overrode user value)")
+	} else if wasMissing {
+		logger.Info("Set enableIngress to false for Gateway-only access (was not set)", "cluster", rayCluster.Name)
+		r.Recorder.Eventf(rayCluster, corev1.EventTypeNormal,
+			"EnableIngressSet",
+			"Set enableIngress to false for Gateway-only access")
+	}
+
+	return nil
+}
+
 // SetupWithManager sets up the controller with the Manager
 func (r *AuthenticationController) SetupWithManager(mgr ctrl.Manager) error {
 	// Predicate to only reconcile RayClusters when relevant changes occur

ray-operator/controllers/ray/authentication_controller_integration_test.go

@@ -38,103 +38,6 @@ import (
 	"github.com/ray-project/kuberay/ray-operator/controllers/ray/utils"
 )
 
-// TestEnsureIngressEnabled tests that ingress is automatically enabled when auth is configured
-func TestEnsureIngressEnabled(t *testing.T) {
-	ctx := context.Background()
-	s := setupScheme()
-
-	tests := []struct {
-		cluster         *rayv1.RayCluster
-		name            string
-		expectUpdate    bool
-		expectedIngress bool
-	}{
-		{
-			name: "Ingress not enabled - should enable",
-			cluster: &rayv1.RayCluster{
-				ObjectMeta: metav1.ObjectMeta{
-					Name:      "test-cluster",
-					Namespace: "default",
-				},
-				Spec: rayv1.RayClusterSpec{
-					HeadGroupSpec: rayv1.HeadGroupSpec{
-						EnableIngress: nil, // Not set
-					},
-				},
-			},
-			expectUpdate:    true,
-			expectedIngress: true,
-		},
-		{
-			name: "Ingress explicitly disabled - should enable",
-			cluster: &rayv1.RayCluster{
-				ObjectMeta: metav1.ObjectMeta{
-					Name:      "test-cluster",
-					Namespace: "default",
-				},
-				Spec: rayv1.RayClusterSpec{
-					HeadGroupSpec: rayv1.HeadGroupSpec{
-						EnableIngress: ptr.To(false),
-					},
-				},
-			},
-			expectUpdate:    true,
-			expectedIngress: true,
-		},
-		{
-			name: "Ingress already enabled - no update",
-			cluster: &rayv1.RayCluster{
-				ObjectMeta: metav1.ObjectMeta{
-					Name:      "test-cluster",
-					Namespace: "default",
-				},
-				Spec: rayv1.RayClusterSpec{
-					HeadGroupSpec: rayv1.HeadGroupSpec{
-						EnableIngress: ptr.To(true),
-					},
-				},
-			},
-			expectUpdate:    false,
-			expectedIngress: true,
-		},
-	}
-
-	for _, tc := range tests {
-		t.Run(tc.name, func(t *testing.T) {
-			fakeClient := clientFake.NewClientBuilder().
-				WithScheme(s).
-				WithRuntimeObjects(tc.cluster).
-				Build()
-
-			controller := &AuthenticationController{
-				Client:   fakeClient,
-				Scheme:   s,
-				Recorder: record.NewFakeRecorder(10),
-				options: RayClusterReconcilerOptions{
-					IsOpenShift: true,
-				},
-			}
-
-			// Execute
-			err := controller.ensureIngressEnabled(ctx, tc.cluster, ctrl.Log)
-			require.NoError(t, err, "Should ensure ingress without error")
-
-			// Verify
-			updatedCluster := &rayv1.RayCluster{}
-			err = fakeClient.Get(ctx, types.NamespacedName{
-				Name:      tc.cluster.Name,
-				Namespace: tc.cluster.Namespace,
-			}, updatedCluster)
-			require.NoError(t, err)
-
-			assert.NotNil(t, updatedCluster.Spec.HeadGroupSpec.EnableIngress,
-				"EnableIngress should be set")
-			assert.Equal(t, tc.expectedIngress, *updatedCluster.Spec.HeadGroupSpec.EnableIngress,
-				"EnableIngress should match expected value")
-		})
-	}
-}
-
 // TestHandleDeletion tests the finalizer-based deletion flow
 func TestHandleDeletion(t *testing.T) {
 	ctx := context.Background()

ray-operator/controllers/ray/authentication_controller_unit_test.go

@@ -1030,3 +1030,143 @@ func TestCleanupOIDCResources(t *testing.T) {
 		})
 	}
 }
+
+func TestEnforceEnableIngressFalseOnOpenShift(t *testing.T) {
+	s := setupScheme()
+
+	testCases := []struct {
+		initialValue *bool
+		name         string
+		expectUpdate bool
+	}{
+		{
+			name:         "enableIngress is nil - should set to false",
+			initialValue: nil,
+			expectUpdate: true,
+		},
+		{
+			name:         "enableIngress is true - should override to false",
+			initialValue: ptr.To(true),
+			expectUpdate: true,
+		},
+		{
+			name:         "enableIngress is already false - no update needed",
+			initialValue: ptr.To(false),
+			expectUpdate: false,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			// Create test cluster
+			rayCluster := &rayv1.RayCluster{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "test-cluster",
+					Namespace: "default",
+				},
+				Spec: rayv1.RayClusterSpec{
+					HeadGroupSpec: rayv1.HeadGroupSpec{
+						EnableIngress: tc.initialValue,
+					},
+				},
+			}
+
+			// Create fake client with the cluster
+			fakeClient := clientFake.NewClientBuilder().
+				WithScheme(s).
+				WithObjects(rayCluster).
+				Build()
+
+			controller := &AuthenticationController{
+				Client:   fakeClient,
+				Scheme:   s,
+				Recorder: record.NewFakeRecorder(10),
+				options: RayClusterReconcilerOptions{
+					IsOpenShift: true,
+				},
+			}
+
+			// Execute
+			err := controller.enforceEnableIngressFalseOnOpenShift(
+				context.Background(),
+				rayCluster,
+				ctrl.Log.WithName("test"))
+
+			// Verify no error
+			require.NoError(t, err, "Enforcement should not return error")
+
+			if tc.expectUpdate {
+				// Get updated cluster from fake client
+				updated := &rayv1.RayCluster{}
+				err = fakeClient.Get(context.Background(),
+					types.NamespacedName{Name: rayCluster.Name, Namespace: rayCluster.Namespace},
+					updated)
+				require.NoError(t, err, "Should be able to get updated cluster")
+
+				// Verify enableIngress is now false
+				require.NotNil(t, updated.Spec.HeadGroupSpec.EnableIngress,
+					"EnableIngress should be set")
+				assert.False(t, *updated.Spec.HeadGroupSpec.EnableIngress,
+					"EnableIngress should be false")
+			}
+		})
+	}
+}
+
+func TestEnforceEnableIngressFalseOnOpenShift_Idempotency(t *testing.T) {
+	s := setupScheme()
+
+	// Create test cluster with enableIngress: true
+	rayCluster := &rayv1.RayCluster{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "test-cluster",
+			Namespace: "default",
+		},
+		Spec: rayv1.RayClusterSpec{
+			HeadGroupSpec: rayv1.HeadGroupSpec{
+				EnableIngress: ptr.To(true),
+			},
+		},
+	}
+
+	fakeClient := clientFake.NewClientBuilder().
+		WithScheme(s).
+		WithObjects(rayCluster).
+		Build()
+
+	controller := &AuthenticationController{
+		Client:   fakeClient,
+		Scheme:   s,
+		Recorder: record.NewFakeRecorder(10),
+		options: RayClusterReconcilerOptions{
+			IsOpenShift: true,
+		},
+	}
+
+	// Call enforcement multiple times
+	for i := 0; i < 3; i++ {
+		// Get latest version
+		err := fakeClient.Get(context.Background(),
+			types.NamespacedName{Name: rayCluster.Name, Namespace: rayCluster.Namespace},
+			rayCluster)
+		require.NoError(t, err)
+
+		// Enforce
+		err = controller.enforceEnableIngressFalseOnOpenShift(
+			context.Background(),
+			rayCluster,
+			ctrl.Log.WithName("test"))
+		require.NoError(t, err, "Enforcement should not error on iteration %d", i)
+	}
+
+	// Verify final state
+	updated := &rayv1.RayCluster{}
+	err := fakeClient.Get(context.Background(),
+		types.NamespacedName{Name: rayCluster.Name, Namespace: rayCluster.Namespace},
+		updated)
+	require.NoError(t, err)
+
+	require.NotNil(t, updated.Spec.HeadGroupSpec.EnableIngress)
+	assert.False(t, *updated.Spec.HeadGroupSpec.EnableIngress,
+		"EnableIngress should be false after multiple enforcements")
+}