From 09ddee7b8f9409314c0a16808f1ca9384c81a5bf Mon Sep 17 00:00:00 2001 From: Roke Jung Date: Mon, 22 Jun 2026 22:41:47 -0400 Subject: [PATCH] :bug: 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 Co-authored-by: Cursor --- pkg/placement/debugger/debugger.go | 37 ++- pkg/placement/debugger/debugger_test.go | 347 ++++++++++++++++++++++-- 2 files changed, 357 insertions(+), 27 deletions(-) diff --git a/pkg/placement/debugger/debugger.go b/pkg/placement/debugger/debugger.go index b140e2d69..627989940 100644 --- a/pkg/placement/debugger/debugger.go +++ b/pkg/placement/debugger/debugger.go @@ -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) diff --git a/pkg/placement/debugger/debugger_test.go b/pkg/placement/debugger/debugger_test.go index 8e4b3e1e1..5d1ec58d4 100644 --- a/pkg/placement/debugger/debugger_test.go +++ b/pkg/placement/debugger/debugger_test.go @@ -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")