From aa18cb0f6c4b818fa12e7c67310b1362e28bd11b Mon Sep 17 00:00:00 2001 From: kurwang Date: Mon, 3 Aug 2026 12:09:16 -0400 Subject: [PATCH 1/3] fix: handle collection requests without hostingCluster to unblock namespace deletion The HCP proxy's handleRoute unconditionally required the hostingCluster query parameter, causing the Kubernetes namespace controller's cleanup DELETE/LIST requests to be rejected with 400 BadRequest. This blocked namespace deletion cluster-wide since the namespace controller cannot confirm resource cleanup for the hcp.ocm.io/v1alpha1 API group. Return an empty HostedClusterList for collection-level requests without hostingCluster, which is semantically correct since the proxy stores no resources locally. Also add list and deletecollection to the discovery verbs so the namespace controller knows these operations are supported. Ref: https://issues.redhat.com/browse/ACM-39570 Signed-off-by: kurwang Co-authored-by: Cursor --- pkg/manager/hcp_proxy.go | 27 ++++++++++++++++++++++++- pkg/manager/hcp_proxy_test.go | 37 +++++++++++++++++++++++++++++++++-- 2 files changed, 61 insertions(+), 3 deletions(-) diff --git a/pkg/manager/hcp_proxy.go b/pkg/manager/hcp_proxy.go index bc08bd7a..b3b8c7f8 100644 --- a/pkg/manager/hcp_proxy.go +++ b/pkg/manager/hcp_proxy.go @@ -385,7 +385,7 @@ func (p *hcpProxy) handleDiscovery(w http.ResponseWriter, r *http.Request) { "singularName": "hostedcluster", "namespaced": true, "kind": "HostedCluster", - "verbs": []string{"create", "delete", "get"}, + "verbs": []string{"create", "delete", "deletecollection", "get", "list"}, }, { // Alias subresource: same as GET|PUT /{name} but with an explicit /resources suffix. @@ -408,6 +408,15 @@ func (p *hcpProxy) handleRoute(w http.ResponseWriter, r *http.Request) { hostingCluster, err := sanitizeProxyName(r.URL.Query().Get("hostingCluster")) if err != nil { + // When hostingCluster is absent the caller cannot target any spoke. + // The Kubernetes namespace controller sends collection-level DELETE and + // LIST requests during namespace cleanup without this parameter; return + // an empty success so namespace deletion is not blocked. + isCollection := len(parts) == 3 && parts[0] == "namespaces" && parts[2] == hcpProxyResource + if isCollection { + p.handleEmptyCollection(w, r) + return + } writeJSONError(w, "hostingCluster query parameter is required and must be a valid DNS-1123 subdomain", http.StatusBadRequest) @@ -479,6 +488,22 @@ func (p *hcpProxy) dispatchNamed(w http.ResponseWriter, r *http.Request, nsRaw, } } +// handleEmptyCollection returns an empty success response for collection-level +// operations (LIST / DELETE-collection) that arrive without a hostingCluster +// query parameter. The Kubernetes namespace controller sends these during +// namespace cleanup to enumerate and remove all resources of every registered +// API type. Since the proxy does not store resources locally (it proxies to +// spoke clusters identified by hostingCluster), an empty list is correct. +func (p *hcpProxy) handleEmptyCollection(w http.ResponseWriter, r *http.Request) { + w.Header().Set(headerContentType, contentTypeJSON) + _ = json.NewEncoder(w).Encode(map[string]interface{}{ + "apiVersion": hcpProxyAPIGroup + "/" + hcpProxyAPIVersion, + "kind": "HostedClusterList", + "metadata": map[string]interface{}{"resourceVersion": ""}, + "items": []interface{}{}, + }) +} + // checkSpokeHealth verifies that the named ManagedCluster is Available. func (p *hcpProxy) checkSpokeHealth(ctx context.Context, spokeName string) error { mc := &clusterv1.ManagedCluster{} diff --git a/pkg/manager/hcp_proxy_test.go b/pkg/manager/hcp_proxy_test.go index 7ce032b1..6aa68508 100644 --- a/pkg/manager/hcp_proxy_test.go +++ b/pkg/manager/hcp_proxy_test.go @@ -317,21 +317,54 @@ func Test_handleDiscovery_WhenVersionPath_ItShouldReturnAPIResourceList(t *testi assert.Len(t, resources, 2) first := resources[0].(map[string]interface{}) assert.Equal(t, hcpProxyResource, first["name"]) + verbs := first["verbs"].([]interface{}) + assert.Contains(t, verbs, "list") + assert.Contains(t, verbs, "deletecollection") second := resources[1].(map[string]interface{}) assert.Equal(t, hcpProxyResource+"/resources", second["name"]) } // --- handleRoute --- -func Test_handleRoute_WhenMissingHostingCluster_ItShouldReturn400(t *testing.T) { +func Test_handleRoute_WhenMissingHostingCluster_OnNamedEndpoint_ItShouldReturn400(t *testing.T) { p := newTestProxy(t) w := httptest.NewRecorder() - path := "/apis/" + hcpProxyAPIGroup + "/" + hcpProxyAPIVersion + "/namespaces/clusters/hostedclusters" + path := "/apis/" + hcpProxyAPIGroup + "/" + hcpProxyAPIVersion + "/namespaces/clusters/hostedclusters/my-hc" r := httptest.NewRequest(http.MethodGet, path, nil) // no ?hostingCluster p.handleRoute(w, r) assert.Equal(t, http.StatusBadRequest, w.Code) } +func Test_handleRoute_WhenMissingHostingCluster_OnCollectionGET_ItShouldReturnEmptyList(t *testing.T) { + p := newTestProxy(t) + w := httptest.NewRecorder() + path := "/apis/" + hcpProxyAPIGroup + "/" + hcpProxyAPIVersion + "/namespaces/clusters/hostedclusters" + r := httptest.NewRequest(http.MethodGet, path, nil) // no ?hostingCluster + p.handleRoute(w, r) + + assert.Equal(t, http.StatusOK, w.Code) + var doc map[string]interface{} + require.NoError(t, json.Unmarshal(w.Body.Bytes(), &doc)) + assert.Equal(t, "HostedClusterList", doc["kind"]) + items := doc["items"].([]interface{}) + assert.Empty(t, items) +} + +func Test_handleRoute_WhenMissingHostingCluster_OnCollectionDELETE_ItShouldReturnEmptyList(t *testing.T) { + p := newTestProxy(t) + w := httptest.NewRecorder() + path := "/apis/" + hcpProxyAPIGroup + "/" + hcpProxyAPIVersion + "/namespaces/clusters/hostedclusters" + r := httptest.NewRequest(http.MethodDelete, path, nil) // no ?hostingCluster + p.handleRoute(w, r) + + assert.Equal(t, http.StatusOK, w.Code) + var doc map[string]interface{} + require.NoError(t, json.Unmarshal(w.Body.Bytes(), &doc)) + assert.Equal(t, "HostedClusterList", doc["kind"]) + items := doc["items"].([]interface{}) + assert.Empty(t, items) +} + func Test_handleRoute_WhenSpokeNotAvailable_ItShouldReturn503(t *testing.T) { // Spoke exists but is not available mc := &clusterv1.ManagedCluster{ From d13019dbaf43244a541ae95cf62b6c358955cc02 Mon Sep 17 00:00:00 2001 From: kurwang Date: Mon, 3 Aug 2026 12:44:47 -0400 Subject: [PATCH 2/3] adding changes from coderabbit Signed-off-by: kurwang --- pkg/manager/hcp_proxy.go | 20 +++++++++++++------- pkg/manager/hcp_proxy_test.go | 18 ++++++++++++++++++ 2 files changed, 31 insertions(+), 7 deletions(-) diff --git a/pkg/manager/hcp_proxy.go b/pkg/manager/hcp_proxy.go index b3b8c7f8..5abda4c0 100644 --- a/pkg/manager/hcp_proxy.go +++ b/pkg/manager/hcp_proxy.go @@ -406,17 +406,23 @@ func (p *hcpProxy) handleRoute(w http.ResponseWriter, r *http.Request) { remaining := strings.TrimPrefix(r.URL.Path, prefix) parts := strings.Split(remaining, "/") - hostingCluster, err := sanitizeProxyName(r.URL.Query().Get("hostingCluster")) - if err != nil { - // When hostingCluster is absent the caller cannot target any spoke. - // The Kubernetes namespace controller sends collection-level DELETE and - // LIST requests during namespace cleanup without this parameter; return - // an empty success so namespace deletion is not blocked. + hostingClusterParam := r.URL.Query().Get("hostingCluster") + + // When hostingCluster is completely absent, the Kubernetes namespace + // controller may be sending collection-level GET (list) or DELETE + // (delete-collection) requests during namespace cleanup. Return an + // empty HostedClusterList so namespace deletion is not blocked. + // POST (create) still requires a spoke target → fall through to 400. + if hostingClusterParam == "" { isCollection := len(parts) == 3 && parts[0] == "namespaces" && parts[2] == hcpProxyResource - if isCollection { + if isCollection && (r.Method == http.MethodGet || r.Method == http.MethodDelete) { p.handleEmptyCollection(w, r) return } + } + + hostingCluster, err := sanitizeProxyName(hostingClusterParam) + if err != nil { writeJSONError(w, "hostingCluster query parameter is required and must be a valid DNS-1123 subdomain", http.StatusBadRequest) diff --git a/pkg/manager/hcp_proxy_test.go b/pkg/manager/hcp_proxy_test.go index 6aa68508..dcf76cb5 100644 --- a/pkg/manager/hcp_proxy_test.go +++ b/pkg/manager/hcp_proxy_test.go @@ -365,6 +365,24 @@ func Test_handleRoute_WhenMissingHostingCluster_OnCollectionDELETE_ItShouldRetur assert.Empty(t, items) } +func Test_handleRoute_WhenMissingHostingCluster_OnCollectionPOST_ItShouldReturn400(t *testing.T) { + p := newTestProxy(t) + w := httptest.NewRecorder() + path := "/apis/" + hcpProxyAPIGroup + "/" + hcpProxyAPIVersion + "/namespaces/clusters/hostedclusters" + r := httptest.NewRequest(http.MethodPost, path, nil) // no ?hostingCluster + p.handleRoute(w, r) + assert.Equal(t, http.StatusBadRequest, w.Code) +} + +func Test_handleRoute_WhenInvalidHostingCluster_OnCollection_ItShouldReturn400(t *testing.T) { + p := newTestProxy(t) + w := httptest.NewRecorder() + path := "/apis/" + hcpProxyAPIGroup + "/" + hcpProxyAPIVersion + "/namespaces/clusters/hostedclusters?hostingCluster=../evil" + r := httptest.NewRequest(http.MethodDelete, path, nil) + p.handleRoute(w, r) + assert.Equal(t, http.StatusBadRequest, w.Code) +} + func Test_handleRoute_WhenSpokeNotAvailable_ItShouldReturn503(t *testing.T) { // Spoke exists but is not available mc := &clusterv1.ManagedCluster{ From 0bb167649b2875f599e6b2ea94a97eb841bfa084 Mon Sep 17 00:00:00 2001 From: kurwang Date: Mon, 3 Aug 2026 12:50:56 -0400 Subject: [PATCH 3/3] fixing e2e tests Signed-off-by: kurwang --- test/e2e/hcp_proxy_test.go | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/test/e2e/hcp_proxy_test.go b/test/e2e/hcp_proxy_test.go index ae6050d0..6b1fe9b8 100644 --- a/test/e2e/hcp_proxy_test.go +++ b/test/e2e/hcp_proxy_test.go @@ -144,13 +144,17 @@ var _ = ginkgo.Describe("HCP Proxy", func() { gomega.Expect(names).To(gomega.ContainElements("hostedclusters", "hostedclusters/resources")) }) - ginkgo.It("should return 400 when hostingCluster is missing from a spoke request", func() { + ginkgo.It("should return empty list when collection GET is missing hostingCluster", func() { client := insecureHTTPClient() url := proxyURL(proxyHost, "/apis/"+hcpProxyAPIGroup+"/"+hcpProxyAPIVersion+"/namespaces/clusters/hostedclusters") resp, err := client.Get(url) // no ?hostingCluster gomega.Expect(err).ToNot(gomega.HaveOccurred()) defer resp.Body.Close() - gomega.Expect(resp.StatusCode).To(gomega.Equal(http.StatusBadRequest)) + gomega.Expect(resp.StatusCode).To(gomega.Equal(http.StatusOK)) + var doc map[string]interface{} + gomega.Expect(json.NewDecoder(resp.Body).Decode(&doc)).To(gomega.Succeed()) + gomega.Expect(doc["kind"]).To(gomega.Equal("HostedClusterList")) + gomega.Expect(doc["items"]).To(gomega.BeEmpty()) }) ginkgo.It("should return 503 when the hosting cluster does not exist", func() { @@ -368,21 +372,21 @@ var _ = ginkgo.Describe("HCP Proxy", func() { "hcp.ocm.io should appear in server API groups") }) - ginkgo.It("should return 400 via the APIService route when hostingCluster is absent", func() { + ginkgo.It("should return empty list via the APIService route when hostingCluster is absent", func() { ginkgo.By("Making raw REST call to /apis/hcp.ocm.io/v1alpha1/namespaces/clusters/hostedclusters") restClient, err := util.NewKubeClient() gomega.Expect(err).ToNot(gomega.HaveOccurred()) - // The proxy returns 400 because hostingCluster is not set; - // the kube-apiserver may wrap this as a 400 or 503. - // Either way the call should not succeed with 200. + // Collection GET without hostingCluster returns an empty + // HostedClusterList so the namespace controller can complete + // cleanup without being blocked. gomega.Eventually(func() int { var statusCode int restClient.CoreV1().RESTClient().Get(). AbsPath("/apis/hcp.ocm.io/v1alpha1/namespaces/clusters/hostedclusters"). Do(ctx).StatusCode(&statusCode) return statusCode - }, eventuallyTimeout, eventuallyInterval).ShouldNot(gomega.Equal(http.StatusOK)) + }, eventuallyTimeout, eventuallyInterval).Should(gomega.Equal(http.StatusOK)) }) })