🐛 fix(placement): return correct HTTP status codes from debug endpoint (#1593)

PlacementDebugServer previously returned HTTP 200 for all error responses because reportErr never called WriteHeader. Classify errors as 400/401/403/404/500, reject incomplete debug paths without a namespace, and add unit test coverage for all reachable reportErr paths. Fixes open-cluster-management-io/ocm#1592 Signed-off-by: Roke Jung <roke@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
2026-08-23 22:26:49 +00:00 · 2026-06-23 02:41:47 +00:00
parent 89836e066a
commit 09ddee7b8f
2 changed files with 357 additions and 27 deletions
@@ -10,6 +10,7 @@ import (
 	"strings"

 	authorizationv1 "k8s.io/api/authorization/v1"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apiserver/pkg/endpoints/request"
 	"k8s.io/client-go/kubernetes"
@@ -84,34 +85,42 @@ func (d *Debugger) Handler(w http.ResponseWriter, r *http.Request) {
 		// POST: Parse Placement from request body
 		placement, err = d.parsePlacementFromBody(r)
 		if err != nil {
-			d.reportErr(w, err)
+			d.reportErr(w, http.StatusBadRequest, err)
 			return
 		}
 	} else {
 		// GET: Fetch Placement from API (original behavior)
 		namespace, name, err := d.parsePath(r.URL.Path)
 		if err != nil {
-			d.reportErr(w, err)
+			d.reportErr(w, http.StatusBadRequest, err)
+			return
+		}
+		if namespace == "" {
+			d.reportErr(w, http.StatusBadRequest, fmt.Errorf("invalid debug path: namespace and name required"))
 			return
 		}

 		placement, err = d.placementLister.Placements(namespace).Get(name)
 		if err != nil {
-			d.reportErr(w, err)
+			if apierrors.IsNotFound(err) {
+				d.reportErr(w, http.StatusNotFound, fmt.Errorf("placement '%s' not found", name))
+			} else {
+				d.reportErr(w, http.StatusInternalServerError, err)
+			}
 			return
 		}
 	}

 	// Check if user has permission to create placements in this namespace
 	if err := d.checkPermission(r, placement.Namespace); err != nil {
-		d.reportErr(w, err)
+		d.reportPermissionErr(w, err)
 		return
 	}

 	// Get valid clustersetbindings in the placement namespace
 	bindings, err := scheduling.GetValidManagedClusterSetBindings(placement.Namespace, d.clusterSetBindingLister, d.clusterSetLister)
 	if err != nil {
-		d.reportErr(w, err)
+		d.reportErr(w, http.StatusInternalServerError, err)
 		return
 	}

@@ -121,7 +130,7 @@ func (d *Debugger) Handler(w http.ResponseWriter, r *http.Request) {
 	// Get available clusters for the placement
 	clusters, err := scheduling.GetAvailableClusters(clusterSetNames, d.clusterSetLister, d.clusterLister)
 	if err != nil {
-		d.reportErr(w, err)
+		d.reportErr(w, http.StatusInternalServerError, err)
 		return
 	}

@@ -182,14 +191,28 @@ func (d *Debugger) parsePlacementFromBody(r *http.Request) (*clusterv1beta1.Plac
 	return &placement, nil
 }

-func (d *Debugger) reportErr(w http.ResponseWriter, err error) {
+func (d *Debugger) reportErr(w http.ResponseWriter, statusCode int, err error) {
 	result := &DebugResult{Error: err.Error()}

 	resultByte, _ := json.Marshal(result)

+	w.Header().Set("Content-Type", "application/json")
+	w.WriteHeader(statusCode)
 	_, _ = w.Write(resultByte)
 }

+func (d *Debugger) reportPermissionErr(w http.ResponseWriter, err error) {
+	statusCode := http.StatusInternalServerError
+	msg := err.Error()
+	switch {
+	case strings.Contains(msg, "user information not found"):
+		statusCode = http.StatusUnauthorized
+	case strings.Contains(msg, "does not have permission"):
+		statusCode = http.StatusForbidden
+	}
+	d.reportErr(w, statusCode, err)
+}
+
 // checkPermission checks if the user has permission to create placements in the namespace using SAR
 func (d *Debugger) checkPermission(r *http.Request, namespace string) error {
 	// Get user from request context (authenticated by GenericAPIServer)
@@ -15,6 +15,7 @@ import (

 	authorizationv1 "k8s.io/api/authorization/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apiserver/pkg/authentication/user"
 	"k8s.io/apiserver/pkg/endpoints/request"
@@ -22,6 +23,8 @@ import (
 	k8stesting "k8s.io/client-go/testing"

 	clusterfake "open-cluster-management.io/api/client/cluster/clientset/versioned/fake"
+	clusterlisterv1beta1 "open-cluster-management.io/api/client/cluster/listers/cluster/v1beta1"
+	clusterlisterv1beta2 "open-cluster-management.io/api/client/cluster/listers/cluster/v1beta2"
 	clusterapiv1 "open-cluster-management.io/api/cluster/v1"
 	clusterapiv1beta1 "open-cluster-management.io/api/cluster/v1beta1"
 	clusterapiv1beta2 "open-cluster-management.io/api/cluster/v1beta2"
@@ -72,6 +75,301 @@ func (r *testResult) RequeueAfter() *time.Duration {
 	return nil
 }

+func assertErrorResponse(t *testing.T, res *http.Response, expectStatus int, errorContains string) {
+	t.Helper()
+
+	if res.StatusCode != expectStatus {
+		t.Errorf("Expected HTTP status %d, got %d", expectStatus, res.StatusCode)
+	}
+	if ct := res.Header.Get("Content-Type"); ct != "application/json" {
+		t.Errorf("Expected Content-Type application/json, got %q", ct)
+	}
+
+	var result DebugResult
+	if err := json.NewDecoder(res.Body).Decode(&result); err != nil {
+		t.Fatalf("Failed to decode response: %v", err)
+	}
+	if result.Error == "" {
+		t.Fatal("Expected error in response body")
+	}
+	if errorContains != "" && !strings.Contains(result.Error, errorContains) {
+		t.Errorf("Expected error to contain %q, got: %s", errorContains, result.Error)
+	}
+}
+
+func newAuthenticatedDebuggerServer(t *testing.T, initObjs []runtime.Object, mutate func(*Debugger)) (*httptest.Server, func()) {
+	t.Helper()
+
+	clusterClient := clusterfake.NewSimpleClientset(initObjs...)
+	clusterInformerFactory := testinghelpers.NewClusterInformerFactory(clusterClient, initObjs...)
+	kubeClient := kubefake.NewClientset()
+	kubeClient.PrependReactor("create", "subjectaccessreviews", func(action k8stesting.Action) (bool, runtime.Object, error) {
+		sar := action.(k8stesting.CreateAction).GetObject().(*authorizationv1.SubjectAccessReview)
+		sar.Status.Allowed = true
+		return true, sar, nil
+	})
+
+	debugger := NewDebugger(
+		&testScheduler{result: &testResult{}},
+		kubeClient,
+		clusterInformerFactory.Cluster().V1beta1().Placements(),
+		clusterInformerFactory.Cluster().V1().ManagedClusters(),
+		clusterInformerFactory.Cluster().V1beta2().ManagedClusterSets(),
+		clusterInformerFactory.Cluster().V1beta2().ManagedClusterSetBindings(),
+	)
+	if mutate != nil {
+		mutate(debugger)
+	}
+
+	handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		userInfo := &user.DefaultInfo{
+			Name:   "test-user",
+			Groups: []string{"system:authenticated"},
+		}
+		ctx := request.WithUser(r.Context(), userInfo)
+		debugger.Handler(w, r.WithContext(ctx))
+	})
+
+	server := httptest.NewServer(handler)
+	return server, server.Close
+}
+
+type fakePlacementLister struct {
+	getErr error
+}
+
+func (f *fakePlacementLister) List(labels.Selector) ([]*clusterapiv1beta1.Placement, error) {
+	return nil, nil
+}
+
+func (f *fakePlacementLister) Placements(string) clusterlisterv1beta1.PlacementNamespaceLister {
+	return fakePlacementNamespaceLister{getErr: f.getErr}
+}
+
+type fakePlacementNamespaceLister struct {
+	getErr error
+}
+
+func (f fakePlacementNamespaceLister) List(labels.Selector) ([]*clusterapiv1beta1.Placement, error) {
+	return nil, nil
+}
+
+func (f fakePlacementNamespaceLister) Get(string) (*clusterapiv1beta1.Placement, error) {
+	return nil, f.getErr
+}
+
+type fakeClusterSetBindingLister struct {
+	listErr error
+}
+
+func (f *fakeClusterSetBindingLister) List(labels.Selector) ([]*clusterapiv1beta2.ManagedClusterSetBinding, error) {
+	return nil, nil
+}
+
+func (f *fakeClusterSetBindingLister) ManagedClusterSetBindings(string) clusterlisterv1beta2.ManagedClusterSetBindingNamespaceLister {
+	return fakeClusterSetBindingNamespaceLister{listErr: f.listErr}
+}
+
+type fakeClusterSetBindingNamespaceLister struct {
+	listErr error
+}
+
+func (f fakeClusterSetBindingNamespaceLister) List(labels.Selector) ([]*clusterapiv1beta2.ManagedClusterSetBinding, error) {
+	return nil, f.listErr
+}
+
+func (f fakeClusterSetBindingNamespaceLister) Get(string) (*clusterapiv1beta2.ManagedClusterSetBinding, error) {
+	return nil, nil
+}
+
+type fakeManagedClusterLister struct {
+	listErr error
+}
+
+func (f *fakeManagedClusterLister) List(labels.Selector) ([]*clusterapiv1.ManagedCluster, error) {
+	return nil, f.listErr
+}
+
+func (f *fakeManagedClusterLister) Get(string) (*clusterapiv1.ManagedCluster, error) {
+	return nil, nil
+}
+
+func TestReportErr(t *testing.T) {
+	debugger := &Debugger{}
+	rec := httptest.NewRecorder()
+
+	debugger.reportErr(rec, http.StatusBadRequest, fmt.Errorf("test error"))
+
+	if rec.Code != http.StatusBadRequest {
+		t.Errorf("Expected HTTP status %d, got %d", http.StatusBadRequest, rec.Code)
+	}
+	if ct := rec.Header().Get("Content-Type"); ct != "application/json" {
+		t.Errorf("Expected Content-Type application/json, got %q", ct)
+	}
+
+	var result DebugResult
+	if err := json.Unmarshal(rec.Body.Bytes(), &result); err != nil {
+		t.Fatalf("Failed to decode response: %v", err)
+	}
+	if result.Error != "test error" {
+		t.Errorf("Expected error %q, got %q", "test error", result.Error)
+	}
+}
+
+func TestReportPermissionErr(t *testing.T) {
+	debugger := &Debugger{}
+
+	cases := []struct {
+		name         string
+		err          error
+		expectStatus int
+	}{
+		{
+			name:         "missing user",
+			err:          fmt.Errorf("user information not found in request context"),
+			expectStatus: http.StatusUnauthorized,
+		},
+		{
+			name:         "forbidden",
+			err:          fmt.Errorf("user does not have permission to create placements in namespace test: denied"),
+			expectStatus: http.StatusForbidden,
+		},
+		{
+			name:         "internal",
+			err:          fmt.Errorf("failed to check permissions: boom"),
+			expectStatus: http.StatusInternalServerError,
+		},
+	}
+
+	for _, c := range cases {
+		t.Run(c.name, func(t *testing.T) {
+			rec := httptest.NewRecorder()
+			debugger.reportPermissionErr(rec, c.err)
+			if rec.Code != c.expectStatus {
+				t.Errorf("Expected HTTP status %d, got %d", c.expectStatus, rec.Code)
+			}
+		})
+	}
+}
+
+func TestDebuggerHandlerReportErr(t *testing.T) {
+	const (
+		placementNamespace = "test-ns"
+		placementName      = "test-placement"
+	)
+
+	validObjs := []runtime.Object{
+		testinghelpers.NewPlacement(placementNamespace, placementName).WithClusterSets("test-set").Build(),
+		testinghelpers.NewManagedCluster("cluster1").WithLabel("clusterset", "test-set").Build(),
+		testinghelpers.NewClusterSet("test-set").WithClusterSelector(clusterapiv1beta2.ManagedClusterSelector{
+			SelectorType: clusterapiv1beta2.LabelSelector,
+			LabelSelector: &metav1.LabelSelector{
+				MatchLabels: map[string]string{"clusterset": "test-set"},
+			},
+		}).Build(),
+		testinghelpers.NewClusterSetBinding(placementNamespace, "test-set"),
+	}
+
+	cases := []struct {
+		name          string
+		initObjs      []runtime.Object
+		method        string
+		pathSuffix    string
+		body          []byte
+		mutate        func(*Debugger)
+		expectStatus  int
+		errorContains string
+	}{
+		{
+			name:          "POST invalid JSON",
+			method:        http.MethodPost,
+			body:          []byte("{"),
+			expectStatus:  http.StatusBadRequest,
+			errorContains: "failed to unmarshal placement JSON",
+		},
+		{
+			name:          "GET incomplete path",
+			method:        http.MethodGet,
+			pathSuffix:    "placement-only",
+			expectStatus:  http.StatusBadRequest,
+			errorContains: "namespace and name required",
+		},
+		{
+			name:          "GET placement not found",
+			method:        http.MethodGet,
+			pathSuffix:    placementNamespace + "/does-not-exist",
+			expectStatus:  http.StatusNotFound,
+			errorContains: "does-not-exist",
+		},
+		{
+			name:       "GET placement lister failure",
+			initObjs:   validObjs,
+			method:     http.MethodGet,
+			pathSuffix: placementNamespace + "/" + placementName,
+			mutate: func(d *Debugger) {
+				d.placementLister = &fakePlacementLister{getErr: fmt.Errorf("lister unavailable")}
+			},
+			expectStatus:  http.StatusInternalServerError,
+			errorContains: "lister unavailable",
+		},
+		{
+			name:       "GET clusterset binding list failure",
+			initObjs:   validObjs,
+			method:     http.MethodGet,
+			pathSuffix: placementNamespace + "/" + placementName,
+			mutate: func(d *Debugger) {
+				d.clusterSetBindingLister = &fakeClusterSetBindingLister{listErr: fmt.Errorf("binding lister unavailable")}
+			},
+			expectStatus:  http.StatusInternalServerError,
+			errorContains: "binding lister unavailable",
+		},
+		{
+			name:       "GET managed cluster list failure",
+			initObjs:   validObjs,
+			method:     http.MethodGet,
+			pathSuffix: placementNamespace + "/" + placementName,
+			mutate: func(d *Debugger) {
+				d.clusterLister = &fakeManagedClusterLister{listErr: fmt.Errorf("cluster lister unavailable")}
+			},
+			expectStatus:  http.StatusInternalServerError,
+			errorContains: "cluster lister unavailable",
+		},
+	}
+
+	for _, c := range cases {
+		t.Run(c.name, func(t *testing.T) {
+			server, closeServer := newAuthenticatedDebuggerServer(t, c.initObjs, c.mutate)
+			defer closeServer()
+
+			method := c.method
+			if method == "" {
+				method = http.MethodGet
+			}
+
+			var body io.Reader
+			if len(c.body) > 0 {
+				body = bytes.NewBuffer(c.body)
+			}
+
+			req, err := http.NewRequest(method, fmt.Sprintf("%s%s%s", server.URL, DebugPath, c.pathSuffix), body)
+			if err != nil {
+				t.Fatalf("Failed to create request: %v", err)
+			}
+			if method == http.MethodPost {
+				req.Header.Set("Content-Type", "application/json")
+			}
+
+			res, err := http.DefaultClient.Do(req)
+			if err != nil {
+				t.Fatalf("Unexpected error: %v", err)
+			}
+			defer res.Body.Close()
+
+			assertErrorResponse(t, res, c.expectStatus, c.errorContains)
+		})
+	}
+}
+
 func TestDebuggerGETMethod(t *testing.T) {
 	placementNamespace := "test"

@@ -476,34 +774,39 @@ func TestDebuggerPermissionCheck(t *testing.T) {
 		sarAllowed       bool
 		sarError         error
 		expectError      bool
+		expectStatusCode int
 		errorContains    string
 		verifySARRequest func(t *testing.T, sar *authorizationv1.SubjectAccessReview)
 	}{
 		{
-			name:          "No user in context - should reject",
-			injectUser:    false,
-			expectError:   true,
-			errorContains: "user information not found in request context",
+			name:             "No user in context - should reject",
+			injectUser:       false,
+			expectError:      true,
+			expectStatusCode: http.StatusUnauthorized,
+			errorContains:    "user information not found in request context",
 		},
 		{
-			name:        "User with permission - should allow",
-			injectUser:  true,
-			sarAllowed:  true,
-			expectError: false,
+			name:             "User with permission - should allow",
+			injectUser:       true,
+			sarAllowed:       true,
+			expectError:      false,
+			expectStatusCode: http.StatusOK,
 		},
 		{
-			name:          "User without permission - should reject",
-			injectUser:    true,
-			sarAllowed:    false,
-			expectError:   true,
-			errorContains: "does not have permission",
+			name:             "User without permission - should reject",
+			injectUser:       true,
+			sarAllowed:       false,
+			expectError:      true,
+			expectStatusCode: http.StatusForbidden,
+			errorContains:    "does not have permission",
 		},
 		{
-			name:          "SAR API call fails",
-			injectUser:    true,
-			sarError:      fmt.Errorf("API server error"),
-			expectError:   true,
-			errorContains: "failed to check permissions",
+			name:             "SAR API call fails",
+			injectUser:       true,
+			sarError:         fmt.Errorf("API server error"),
+			expectError:      true,
+			expectStatusCode: http.StatusInternalServerError,
+			errorContains:    "failed to check permissions",
 		},
 		{
 			name:       "User with Extra fields - should pass all fields to SAR",
@@ -516,8 +819,9 @@ func TestDebuggerPermissionCheck(t *testing.T) {
 					"custom.example.com/department":       {"engineering"},
 				},
 			},
-			sarAllowed:  true,
-			expectError: false,
+			sarAllowed:       true,
+			expectError:      false,
+			expectStatusCode: http.StatusOK,
 			verifySARRequest: func(t *testing.T, sar *authorizationv1.SubjectAccessReview) {
 				expected := map[string]authorizationv1.ExtraValue{
 					"authentication.kubernetes.io/scopes": {"read", "write"},
@@ -609,6 +913,9 @@ func TestDebuggerPermissionCheck(t *testing.T) {
 			}

 			// Verify result
+			if res.StatusCode != c.expectStatusCode {
+				t.Errorf("Expected HTTP status %d, got %d", c.expectStatusCode, res.StatusCode)
+			}
 			if c.expectError {
 				if result.Error == "" {
 					t.Errorf("Expected error but got none")