Skip to content

🐛 Reject namespace filters for cluster-scoped resources #3556

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
dlanov wants to merge 1 commit into
base: main
Choose a base branch
from
+249 −0
Open

🐛 Reject namespace filters for cluster-scoped resources #3556

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 51 additions & 0 deletions pkg/client/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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...)
Expand All @@ -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())
Expand Down Expand Up @@ -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 {
Expand Down
198 changes: 198 additions & 0 deletions pkg/client/client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (
"encoding/json"
"errors"
"fmt"
"net/http"
"reflect"
"strings"
"sync/atomic"
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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"))
})
})
})

Expand Down Expand Up @@ -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() {

})
Expand Down Expand Up @@ -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() {

})
Expand Down Expand Up @@ -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() {

})
Expand Down