Skip to content

pkg/kube: require namespace in Ctl.List #1121

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

Merged
chrisseto merged 1 commit into from
Oct 1, 2025
Merged

pkg/kube: require namespace in Ctl.List #1121

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
6 changes: 2 additions & 4 deletions charts/console/chart/client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,9 +72,7 @@ func NewClient(ctx context.Context, kubeCtl *kube.Ctl, dot *helmette.Dot) (*Clie
}

func (c *Client) getConsolePod(ctx context.Context) (*corev1.Pod, error) {
deploys, err := kube.List[appsv1.DeploymentList](ctx, c.Ctl,
k8sclient.InNamespace(c.Release.Namespace),
)
deploys, err := kube.List[appsv1.DeploymentList](ctx, c.Ctl, c.Release.Namespace)
if err != nil {
return nil, err
}
Expand All @@ -92,7 +90,7 @@ func (c *Client) getConsolePod(ctx context.Context) (*corev1.Pod, error) {
}

pods, err := kube.List[corev1.PodList](ctx, c.Ctl,
k8sclient.InNamespace(deployment.Namespace),
deployment.Namespace,
k8sclient.MatchingLabels(deployment.Spec.Selector.MatchLabels))
if err != nil {
return nil, err
Expand Down
6 changes: 3 additions & 3 deletions charts/redpanda/chart_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@ func TestIntegrationChart(t *testing.T) {
}),
})

pods, err := kube.List[corev1.PodList](ctx, env.Ctl(), client.MatchingLabels{
pods, err := kube.List[corev1.PodList](ctx, env.Ctl(), release.Namespace, client.MatchingLabels{
"app.kubernetes.io/instance": release.Name,
"app.kubernetes.io/component": release.Chart + "-statefulset",
})
Expand Down Expand Up @@ -433,7 +433,7 @@ func TestIntegrationChart(t *testing.T) {
}),
})

pods, err := kube.List[corev1.PodList](ctx, env.Ctl(), client.MatchingLabels{
pods, err := kube.List[corev1.PodList](ctx, env.Ctl(), release.Namespace, client.MatchingLabels{
"app.kubernetes.io/instance": release.Name,
"app.kubernetes.io/component": release.Chart + "-statefulset",
})
Expand Down Expand Up @@ -473,7 +473,7 @@ func TestIntegrationChart(t *testing.T) {
}),
})

pods, err := kube.List[corev1.PodList](ctx, env.Ctl(), client.MatchingLabels{
pods, err := kube.List[corev1.PodList](ctx, env.Ctl(), release.Namespace, client.MatchingLabels{
"app.kubernetes.io/instance": release.Name,
"app.kubernetes.io/name": "console",
})
Expand Down
5 changes: 2 additions & 3 deletions charts/redpanda/client/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ import (
"github.com/twmb/franz-go/pkg/sasl/scram"
"github.com/twmb/franz-go/pkg/sr"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"sigs.k8s.io/controller-runtime/pkg/client"

"github.com/redpanda-data/redpanda-operator/charts/redpanda/v25"
Expand Down Expand Up @@ -328,9 +327,9 @@ func srvLookup(state *redpanda.RenderState, dialer DialContextFunc, service stri
// Querying for k8s-app=kube-dns is a generally accepted / safe
// way of finding the kube DNS. We could alternatively find the
// kube-dns service and use its label selector.
pods, err := kube.List[corev1.PodList](ctx, ctl, client.MatchingLabels{
pods, err := kube.List[corev1.PodList](ctx, ctl, kube.NamespaceSystem, client.MatchingLabels{
"k8s-app": "kube-dns",
}, client.InNamespace(metav1.NamespaceSystem))
})
if err != nil {
return nil, err
}
Expand Down
48 changes: 41 additions & 7 deletions go.work.sum
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion operator/internal/controller/vectorized/cluster_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -275,7 +275,7 @@ func normalizeClusterResources(ctx context.Context, testScheme *runtime.Scheme,

clusterSelector := labels.ForCluster(cluster).AsClientSelector()
for _, l := range lists {
err := ctl.List(ctx, l)
err := ctl.List(ctx, cluster.Namespace, l)
if errors.Is(err, &meta.NoKindMatchError{}) {
continue
}
Expand Down
6 changes: 3 additions & 3 deletions operator/internal/lifecycle/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -259,7 +259,7 @@ func (r *ResourceClient[T, U]) DeleteAll(ctx context.Context, owner U) (bool, er

// fetchExistingPools fetches the existing pools (StatefulSets) for a given cluster.
func (r *ResourceClient[T, U]) fetchExistingPools(ctx context.Context, cluster U) ([]*poolWithOrdinals, error) {
sets, err := kube.List[appsv1.StatefulSetList](ctx, r.ctl, client.InNamespace(cluster.GetNamespace()), client.MatchingLabels(r.ownershipResolver.GetOwnerLabels(cluster)))
sets, err := kube.List[appsv1.StatefulSetList](ctx, r.ctl, cluster.GetNamespace(), client.MatchingLabels(r.ownershipResolver.GetOwnerLabels(cluster)))
if err != nil {
return nil, errors.Wrapf(err, "listing StatefulSets")
}
Expand Down Expand Up @@ -288,7 +288,7 @@ func (r *ResourceClient[T, U]) fetchExistingPools(ctx context.Context, cluster U
}

// based on https://github.com/kubernetes/kubernetes/blob/c90a4b16b6aa849ed362ee40997327db09e3a62d/pkg/controller/history/controller_history.go#L222
revisions, err := kube.List[appsv1.ControllerRevisionList](ctx, r.ctl, client.MatchingLabelsSelector{
revisions, err := kube.List[appsv1.ControllerRevisionList](ctx, r.ctl, cluster.GetNamespace(), client.MatchingLabelsSelector{
Selector: selector,
})
if err != nil {
Expand All @@ -303,7 +303,7 @@ func (r *ResourceClient[T, U]) fetchExistingPools(ctx context.Context, cluster U
}
}

pods, err := kube.List[corev1.PodList](ctx, r.ctl, client.MatchingLabelsSelector{
pods, err := kube.List[corev1.PodList](ctx, r.ctl, cluster.GetNamespace(), client.MatchingLabelsSelector{
Selector: selector,
})
if err != nil {
Expand Down
14 changes: 12 additions & 2 deletions pkg/kube/ctl.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
corev1 "k8s.io/api/core/v1"
k8serrors "k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/api/meta"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/client-go/kubernetes/scheme"
Expand All @@ -37,8 +38,11 @@ type (
Object = client.Object
ObjectList = client.ObjectList
ObjectKey = client.ObjectKey
)

InNamespace = client.InNamespace
const (
NamespaceAll = metav1.NamespaceAll
NamespaceSystem = metav1.NamespaceSystem
)

type Option interface {
Expand Down Expand Up @@ -164,11 +168,17 @@ func (c *Ctl) GetAndWait(ctx context.Context, key ObjectKey, obj Object, cond Co
}

// List fetches a list of objects into `objs` from Kubernetes.
//
// Cluster scoped resources should pass `""` as namespace.
//
// Usage:
//
// var pods corev1.PodList
// ctl.List(ctx, &pods)
func (c *Ctl) List(ctx context.Context, objs ObjectList, opts ...client.ListOption) error {
func (c *Ctl) List(ctx context.Context, namespace string, objs ObjectList, opts ...client.ListOption) error {
Copy link
Contributor

@andrewstucki andrewstucki Oct 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with this, but wondering, does it make more sense to reverse the order of objs and namespace to be a bit more consistent with the top-level package List?

Copy link
Contributor Author

@chrisseto chrisseto Oct 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO this is consistent? It follows:

// struct methods
(c *Ctl) Blah(context.Context, RequireArgs, Object/Generic, Options...)

// generics
Blah[T](context.Context, *Ctl, RequireArgs, Options...)

// Get
(c *Ctl) Get(context.Context, ObjectKey, Object)
Get[T](context.Context, ObjectKey)

// List
(c *Ctl) List(context.Context, Namespace, ObjectList, Options...)
List[T](context.Context, Namespace, Options...)

The inclusion of Options isn't super consistent and the All methods are a bit weird to support a variadic list of objects 😅

Copy link
Contributor Author

@chrisseto chrisseto Oct 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, maybe the generic methods should have had ctl first in hindsight.

Get[T](*Ctl, context.Context, ObjectKey)

// Top level namespace parameter takes precedence over anything specified
// in opts. The other way around is less straightforward.
opts = append(opts, client.InNamespace(namespace))
if err := c.client.List(ctx, objs, opts...); err != nil {
return errors.WithStack(err)
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/kube/ctl_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ func TestCtl(t *testing.T) {
))
require.Equal(t, []string{"cm-0", "cm-1", "cm-2"}, seen)

cms, err := kube.List[corev1.ConfigMapList](ctx, ctl, kube.InNamespace("hello-world"))
cms, err := kube.List[corev1.ConfigMapList](ctx, ctl, "hello-world")
require.NoError(t, err)
require.Len(t, cms.Items, 3)

Expand Down
4 changes: 2 additions & 2 deletions pkg/kube/generics.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,9 +55,9 @@ func ApplyAndWait[T any, PT AddrOfObject[T]](ctx context.Context, ctl *Ctl, obj
}

// List is a generic equivalent of [Ctl.List].
func List[T any, L AddrOfObjectList[T]](ctx context.Context, ctl *Ctl, opts ...client.ListOption) (*T, error) {
func List[T any, L AddrOfObjectList[T]](ctx context.Context, ctl *Ctl, namespace string, opts ...client.ListOption) (*T, error) {
var list T
if err := ctl.List(ctx, L(&list), opts...); err != nil {
if err := ctl.List(ctx, namespace, L(&list), opts...); err != nil {
return nil, err
}
return &list, nil
Expand Down
2 changes: 1 addition & 1 deletion pkg/kube/syncer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,7 @@ func (s *Syncer) listInPurview(ctx context.Context) ([]Object, error) {
return nil, err
}

if err := s.Ctl.List(ctx, list, client.InNamespace(s.Namespace), client.MatchingLabels(s.OwnershipLabels)); err != nil {
if err := s.Ctl.List(ctx, s.Namespace, list, client.MatchingLabels(s.OwnershipLabels)); err != nil {
return nil, err
}

Expand Down
4 changes: 2 additions & 2 deletions pkg/kube/syncer_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,15 +207,15 @@ func TestSyncer(t *testing.T) {
// Our owning namespace hasn't been removed but the other one(s)
// have been cleaned up. NB: envtest namespaces never get fully
// deleted, so we filter to Active ones.
nss, err := kube.List[corev1.NamespaceList](ctx, ctl, client.MatchingFields{
nss, err := kube.List[corev1.NamespaceList](ctx, ctl, "", client.MatchingFields{
"status.phase": "Active",
}, client.HasLabels{"owned_by"})
require.NoError(t, err)
require.Len(t, nss.Items, 1)
require.Equal(t, ns.UID, nss.Items[0].UID)

// The only left over configmap is our unowned one.
cms, err := kube.List[corev1.ConfigMapList](ctx, ctl, client.HasLabels{"owned_by"}, client.InNamespace(ns.Name))
cms, err := kube.List[corev1.ConfigMapList](ctx, ctl, ns.Name, client.HasLabels{"owned_by"})
require.NoError(t, err)
require.Len(t, cms.Items, 1)
require.Equal(t, "not-owned", cms.Items[0].Name)
Expand Down
Loading