From c2bb101429fd41fc821c7d0c46df7889e2746a2c Mon Sep 17 00:00:00 2001 From: Dennis Lanov Date: Mon, 20 Jul 2026 10:54:30 -0500 Subject: [PATCH] client: reject namespace for cluster-scoped resources Signed-off-by: Dennis Lanov --- pkg/client/client.go | 51 ++++++++++ pkg/client/client_test.go | 198 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 249 insertions(+) diff --git a/pkg/client/client.go b/pkg/client/client.go index ad946daeaa..c7f1d4c350 100644 --- a/pkg/client/client.go +++ b/pkg/client/client.go @@ -331,6 +331,24 @@ func (c *client) Delete(ctx context.Context, obj Object, opts ...DeleteOption) e // DeleteAllOf implements client.Client. func (c *client) DeleteAllOf(ctx context.Context, obj Object, opts ...DeleteAllOfOption) error { + // Parse opts into a normalized DeleteAllOfOptions exactly once, then pass only + // that normalized value downstream. *DeleteAllOfOptions implements + // DeleteAllOfOption, so downstream layers can copy its already-resolved fields + // without re-invoking any caller-supplied option a second time. + deleteAllOfOpts := DeleteAllOfOptions{} + deleteAllOfOpts.ApplyOptions(opts) + opts = []DeleteAllOfOption{&deleteAllOfOpts} + + if deleteAllOfOpts.Namespace != "" { + gvk, err := c.GroupVersionKindFor(obj) + if err != nil { + return err + } + if err := c.rejectNamespaceForClusterScoped("DeleteAllOf", gvk, deleteAllOfOpts.Namespace); err != nil { + return err + } + } + switch obj.(type) { case runtime.Unstructured: return c.unstructuredClient.DeleteAllOf(ctx, obj, opts...) @@ -341,6 +359,19 @@ func (c *client) DeleteAllOf(ctx context.Context, obj Object, opts ...DeleteAllO } } +// rejectNamespaceForClusterScoped returns an error if gvk is cluster-scoped and +// namespace is non-empty; it is a no-op for namespace-scoped resources. +func (c *client) rejectNamespaceForClusterScoped(op string, gvk schema.GroupVersionKind, namespace string) error { + namespaced, err := apiutil.IsGVKNamespaced(gvk, c.mapper) + if err != nil { + return fmt.Errorf("failed to determine if %s is namespace-scoped: %w", gvk, err) + } + if namespaced { + return nil + } + return fmt.Errorf("failed to %s: %s is cluster-scoped and does not support namespace %q", op, gvk, namespace) +} + // Patch implements client.Client. func (c *client) Patch(ctx context.Context, obj Object, patch Patch, opts ...PatchOption) error { defer c.resetGroupVersionKind(obj, obj.GetObjectKind().GroupVersionKind()) @@ -388,6 +419,26 @@ func (c *client) Get(ctx context.Context, key ObjectKey, obj Object, opts ...Get // List implements client.Client. func (c *client) List(ctx context.Context, obj ObjectList, opts ...ListOption) error { + // Parse opts into a normalized ListOptions exactly once, then pass only that + // normalized value downstream. *ListOptions implements ListOption, so + // downstream layers (cache, typed, unstructured, metadata) can copy its + // already-resolved fields without re-invoking any caller-supplied option a + // second time. + listOpts := ListOptions{} + listOpts.ApplyOptions(opts) + opts = []ListOption{&listOpts} + + if listOpts.Namespace != "" { + gvk, err := c.GroupVersionKindFor(obj) + if err != nil { + return err + } + gvk.Kind = strings.TrimSuffix(gvk.Kind, "List") + if err := c.rejectNamespaceForClusterScoped("List", gvk, listOpts.Namespace); err != nil { + return err + } + } + if isUncached, err := c.shouldBypassCache(obj); err != nil { return err } else if !isUncached { diff --git a/pkg/client/client_test.go b/pkg/client/client_test.go index 77835eb5e8..8afcc324c4 100644 --- a/pkg/client/client_test.go +++ b/pkg/client/client_test.go @@ -23,6 +23,7 @@ import ( "encoding/json" "errors" "fmt" + "net/http" "reflect" "strings" "sync/atomic" @@ -142,6 +143,72 @@ func metaOnlyFromObj(obj interface { return &metaObj } +// recordingRoundTripper wraps an http.RoundTripper and records the method and +// URL path of every request that passes through it, then forwards the request +// unchanged. It is used to prove whether a request was (or, notably, was not) +// sent to the API server for a given client call. +type recordingRoundTripper struct { + delegate http.RoundTripper + requests []string +} + +func (rt *recordingRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) { + rt.requests = append(rt.requests, req.Method+" "+req.URL.Path) + return rt.delegate.RoundTrip(req) +} + +// newRecordingClient returns a client.Client backed by a copy of cfg whose +// outgoing HTTP requests are captured by the returned *recordingRoundTripper. +func newRecordingClient() (client.Client, *recordingRoundTripper) { + rt := &recordingRoundTripper{} + cfgCopy := rest.CopyConfig(cfg) + cfgCopy.WrapTransport = func(delegate http.RoundTripper) http.RoundTripper { + rt.delegate = delegate + return rt + } + cl, err := client.New(cfgCopy, client.Options{}) + Expect(err).NotTo(HaveOccurred()) + return cl, rt +} + +// expectClusterScopedNamespaceError asserts that err is the rejection produced +// for calling op ("List" or "DeleteAllOf") with InNamespace(namespace) against +// a cluster-scoped resource: it must name the operation, say the resource is +// cluster-scoped, and quote the offending namespace. +func expectClusterScopedNamespaceError(err error, op, namespace string) { + GinkgoHelper() + Expect(err).To(MatchError(SatisfyAll( + ContainSubstring(op), + ContainSubstring("cluster-scoped"), + ContainSubstring(fmt.Sprintf("%q", namespace)), + ))) +} + +// countingListOption wraps a ListOption and counts how many times +// ApplyToList is invoked, to prove options are applied exactly once. +type countingListOption struct { + inner client.ListOption + applied int +} + +func (o *countingListOption) ApplyToList(opts *client.ListOptions) { + o.applied++ + o.inner.ApplyToList(opts) +} + +// countingDeleteAllOfOption wraps a DeleteAllOfOption and counts how many +// times ApplyToDeleteAllOf is invoked, to prove options are applied exactly +// once. +type countingDeleteAllOfOption struct { + inner client.DeleteAllOfOption + applied int +} + +func (o *countingDeleteAllOfOption) ApplyToDeleteAllOf(opts *client.DeleteAllOfOptions) { + o.applied++ + o.inner.ApplyToDeleteAllOf(opts) +} + var _ = Describe("Client", func() { var scheme *runtime.Scheme @@ -2089,6 +2156,43 @@ U5wwSivyi7vmegHKmblOzNVKA5qPO8zWzqBC _, err = clientset.AppsV1().Deployments(ns).Get(ctx, dep2Name, metav1.GetOptions{}) Expect(err).To(HaveOccurred()) }) + + It("should error when InNamespace targets a cluster-scoped object, without sending a request (issue #988)", func(ctx SpecContext) { + cl, rt := newRecordingClient() + + err := cl.DeleteAllOf(ctx, node, client.InNamespace("some-namespace")) + expectClusterScopedNamespaceError(err, "DeleteAllOf", "some-namespace") + Expect(rt.requests).NotTo(ContainElement("DELETE /api/v1/nodes")) + }) + + It("should still allow a cluster-wide delete collection of a cluster-scoped object when no namespace is specified", func(ctx SpecContext) { + cl, rt := newRecordingClient() + + By("deleting a collection of Nodes matching a label that does not exist, so nothing is actually removed") + err := cl.DeleteAllOf(ctx, node, client.MatchingLabels{"this-label-does-not-exist-on-any-node": "true"}) + Expect(err).NotTo(HaveOccurred()) + + By("validating that the request was not scoped to any namespace") + Expect(rt.requests).To(ContainElement("DELETE /api/v1/nodes")) + }) + + It("should invoke each caller-supplied DeleteAllOfOption exactly once", func(ctx SpecContext) { + cl, err := client.New(cfg, client.Options{}) + Expect(err).NotTo(HaveOccurred()) + + dep, err = clientset.AppsV1().Deployments(ns).Create(ctx, dep, metav1.CreateOptions{}) + Expect(err).NotTo(HaveOccurred()) + depName := dep.Name + + counting := &countingDeleteAllOfOption{inner: client.InNamespace(ns)} + err = cl.DeleteAllOf(ctx, dep, counting, client.MatchingLabels(dep.ObjectMeta.Labels)) + Expect(err).NotTo(HaveOccurred()) + Expect(counting.applied).To(Equal(1)) + + By("validating the namespaced delete reached the downstream client") + _, err = clientset.AppsV1().Deployments(ns).Get(ctx, depName, metav1.GetOptions{}) + Expect(err).To(HaveOccurred()) + }) }) Context("with unstructured objects", func() { It("should delete an existing object from a go struct", func(ctx SpecContext) { @@ -2195,6 +2299,22 @@ U5wwSivyi7vmegHKmblOzNVKA5qPO8zWzqBC _, err = clientset.AppsV1().Deployments(ns).Get(ctx, dep2Name, metav1.GetOptions{}) Expect(err).To(HaveOccurred()) }) + + It("should error when InNamespace targets a cluster-scoped object, without sending a request (issue #988)", func(ctx SpecContext) { + cl, rt := newRecordingClient() + + u := &unstructured.Unstructured{} + Expect(scheme.Convert(node, u, nil)).To(Succeed()) + u.SetGroupVersionKind(schema.GroupVersionKind{ + Group: "", + Kind: "Node", + Version: "v1", + }) + + err := cl.DeleteAllOf(ctx, u, client.InNamespace("some-namespace")) + expectClusterScopedNamespaceError(err, "DeleteAllOf", "some-namespace") + Expect(rt.requests).NotTo(ContainElement("DELETE /api/v1/nodes")) + }) }) Context("with metadata objects", func() { It("should delete an existing object from a go struct", func(ctx SpecContext) { @@ -2275,6 +2395,16 @@ U5wwSivyi7vmegHKmblOzNVKA5qPO8zWzqBC _, err = clientset.AppsV1().Deployments(ns).Get(ctx, dep2Name, metav1.GetOptions{}) Expect(err).To(HaveOccurred()) }) + + It("should error when InNamespace targets a cluster-scoped object, without sending a request (issue #988)", func(ctx SpecContext) { + cl, rt := newRecordingClient() + + metaObj := metaOnlyFromObj(node, scheme) + + err := cl.DeleteAllOf(ctx, metaObj, client.InNamespace("some-namespace")) + expectClusterScopedNamespaceError(err, "DeleteAllOf", "some-namespace") + Expect(rt.requests).NotTo(ContainElement("DELETE /api/v1/nodes")) + }) }) }) @@ -3067,6 +3197,44 @@ U5wwSivyi7vmegHKmblOzNVKA5qPO8zWzqBC Expect(deps.Items[1].Name).To(Equal(dep4.Name)) }) + It("should error when InNamespace targets a cluster-scoped object, without sending a request (issue #988)", func(ctx SpecContext) { + cl, rt := newRecordingClient() + + nodeList := &corev1.NodeList{} + err := cl.List(ctx, nodeList, client.InNamespace("some-namespace")) + expectClusterScopedNamespaceError(err, "List", "some-namespace") + Expect(rt.requests).NotTo(ContainElement("GET /api/v1/nodes")) + }) + + It("should still allow listing a cluster-scoped object when no namespace is specified", func(ctx SpecContext) { + cl, rt := newRecordingClient() + + nodeList := &corev1.NodeList{} + err := cl.List(ctx, nodeList) + Expect(err).NotTo(HaveOccurred()) + + By("validating that the request was not scoped to any namespace") + Expect(rt.requests).To(ContainElement("GET /api/v1/nodes")) + }) + + It("should invoke each caller-supplied ListOption exactly once", func(ctx SpecContext) { + cl, err := client.New(cfg, client.Options{}) + Expect(err).NotTo(HaveOccurred()) + + dep, err = clientset.AppsV1().Deployments(ns).Create(ctx, dep, metav1.CreateOptions{}) + Expect(err).NotTo(HaveOccurred()) + + counting := &countingListOption{inner: client.InNamespace(ns)} + deps := &appsv1.DeploymentList{} + err = cl.List(ctx, deps, counting, client.MatchingLabels(dep.ObjectMeta.Labels)) + Expect(err).NotTo(HaveOccurred()) + Expect(counting.applied).To(Equal(1)) + + By("validating the namespaced list reached the downstream client") + Expect(deps.Items).To(HaveLen(1)) + Expect(deps.Items[0].Name).To(Equal(dep.Name)) + }) + PIt("should fail if the object doesn't have meta", func() { }) @@ -3344,6 +3512,21 @@ U5wwSivyi7vmegHKmblOzNVKA5qPO8zWzqBC deleteNamespace(ctx, tns4) }) + It("should error when InNamespace targets a cluster-scoped object, without sending a request (issue #988)", func(ctx SpecContext) { + cl, rt := newRecordingClient() + + nodeList := &unstructured.UnstructuredList{} + nodeList.SetGroupVersionKind(schema.GroupVersionKind{ + Group: "", + Kind: "NodeList", + Version: "v1", + }) + + err := cl.List(ctx, nodeList, client.InNamespace("some-namespace")) + expectClusterScopedNamespaceError(err, "List", "some-namespace") + Expect(rt.requests).NotTo(ContainElement("GET /api/v1/nodes")) + }) + PIt("should fail if the object doesn't have meta", func() { }) @@ -3791,6 +3974,21 @@ U5wwSivyi7vmegHKmblOzNVKA5qPO8zWzqBC Expect(metaList.Items[1].Name).To(Equal(dep4.Name)) }) + It("should error when InNamespace targets a cluster-scoped object, without sending a request (issue #988)", func(ctx SpecContext) { + cl, rt := newRecordingClient() + + metaList := &metav1.PartialObjectMetadataList{} + metaList.SetGroupVersionKind(schema.GroupVersionKind{ + Group: "", + Version: "v1", + Kind: "NodeList", + }) + + err := cl.List(ctx, metaList, client.InNamespace("some-namespace")) + expectClusterScopedNamespaceError(err, "List", "some-namespace") + Expect(rt.requests).NotTo(ContainElement("GET /api/v1/nodes")) + }) + PIt("should fail if the object doesn't have meta", func() { })