Skip to content
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.

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

Fix List() calls that didn't fill in revision #9599

Merged
merged 10 commits into from
Jan 9, 2025
2 changes: 1 addition & 1 deletion calicoctl/tests/st/calicoctl/test_crud.py
Original file line number Diff line number Diff line change
Expand Up @@ -2485,7 +2485,7 @@ def check_only_default_profile_returned(self, testdata):
' metadata:\n'
' creationTimestamp: null\n'
' name: projectcalico-default-allow\n'
' resourceVersion: "0"\n'
' resourceVersion: "1"\n'
' spec:\n'
' egress:\n'
' - action: Allow\n'
Expand Down
17 changes: 16 additions & 1 deletion cni-plugin/pkg/plugin/plugin.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ import (
"github.com/projectcalico/calico/cni-plugin/pkg/k8s"
"github.com/projectcalico/calico/cni-plugin/pkg/types"
libapi "github.com/projectcalico/calico/libcalico-go/lib/apis/v3"
"github.com/projectcalico/calico/libcalico-go/lib/backend/k8s/resources"
"github.com/projectcalico/calico/libcalico-go/lib/clientv3"
cerrors "github.com/projectcalico/calico/libcalico-go/lib/errors"
"github.com/projectcalico/calico/libcalico-go/lib/logutils"
Expand Down Expand Up @@ -260,7 +261,21 @@ func cmdAdd(args *skel.CmdArgs) (err error) {
}

// Check if there's an existing endpoint by listing the existing endpoints based on the WEP name prefix.
endpoints, err := calicoClient.WorkloadEndpoints().List(ctx, options.ListOptions{Name: wepPrefix, Namespace: wepIDs.Namespace, Prefix: true})

// We know that, in KDD, even though there may be >1 endpoint, we're only
// looking up one backing Pod. Send it a hint that we really want it to
// do a Get instead of a list. CNI plugin only has RBAC permissions to do
// a get on Pod resources. The default used to be to do a Get if possible,
Copy link
Member

Choose a reason for hiding this comment

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

CNI plugin only has RBAC permissions

Any reason to not just switch to a List() with the field selector specified, and give the CNI plugin the necessary permissions?

Copy link
Member Author

Choose a reason for hiding this comment

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

Mainly just thinking that it was quite a big loosening to give CNI plugin list permissions when it could make do with get before. That, and the fact that I'd need to adjust manifests and operator to loosen permissions, which felt like extra work to go in the wrong direction.

Copy link
Member

Choose a reason for hiding this comment

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

I suppose I just don't see a huge security distinction between "Get any pod in the cluster" vs "List any pod in the cluster"

But I'm happy to leave it as is.

// and we relied on that here, but the default was changed to fix an issue
// with watching from the returned resource revision. We don't watch here
// so we can opt in to the old behavior.
ctx = resources.ContextWithWorkloadEndpointListMode(ctx, resources.WorkloadEndpointListModeForceGet)

endpoints, err := calicoClient.WorkloadEndpoints().List(ctx, options.ListOptions{
Name: wepPrefix,
Namespace: wepIDs.Namespace,
Prefix: true,
})
if err != nil {
return
}
Expand Down
40 changes: 12 additions & 28 deletions libcalico-go/lib/backend/k8s/resources/customresource.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@ import (
"github.com/projectcalico/calico/libcalico-go/lib/backend/api"
"github.com/projectcalico/calico/libcalico-go/lib/backend/k8s/conversion"
"github.com/projectcalico/calico/libcalico-go/lib/backend/model"
cerrors "github.com/projectcalico/calico/libcalico-go/lib/errors"
)

// customK8sResourceClient implements the K8sResourceClient interface and provides a generic
Expand Down Expand Up @@ -343,40 +342,25 @@ func (c *customK8sResourceClient) List(ctx context.Context, list model.ListInter
})
logContext.Debug("Received List request")

// Attempt to convert the ListInterface to a Key. If possible, the parameters
// indicate a fully qualified resource, and we'll need to use Get instead of
// List.
if key := c.listInterfaceToKey(list); key != nil {
logContext.Debug("Performing List using Get")
kvps := []*model.KVPair{}
if kvp, err := c.Get(ctx, key, revision); err != nil {
// The error will already be a Calico error type. Ignore
// error that it doesn't exist - we'll return an empty
// list.
if _, ok := err.(cerrors.ErrorResourceDoesNotExist); !ok {
logContext.WithField("Resource", c.resource).WithError(err).Debug("Error listing resource")
return nil, err
}
return &model.KVPairList{
KVPairs: kvps,
Revision: revision,
}, nil
} else {
kvps = append(kvps, kvp)
return &model.KVPairList{
KVPairs: kvps,
Revision: revision,
}, nil
}
}

// If it is a namespaced resource, then we'll need the namespace.
namespace := list.(model.ResourceListOptions).Namespace
key := c.listInterfaceToKey(list)

// listFunc performs a list with the given options.
listFunc := func(ctx context.Context, opts metav1.ListOptions) (runtime.Object, error) {
out := reflect.New(c.k8sListType).Interface().(ResourceList)

if key != nil {
// Being asked to list a single resource, add the filter.
key := key.(model.ResourceKey)
name, err := c.keyToName(key)
if err != nil {
logContext.WithError(err).Error("Failed to convert key to name of resource.")
return nil, err
}
opts.FieldSelector = fmt.Sprintf("metadata.name=%s", name)
}

// Build the request.
req := c.restClient.Get().
NamespaceIfScoped(namespace, c.namespaced).
Expand Down
5 changes: 4 additions & 1 deletion libcalico-go/lib/backend/k8s/resources/ipam_affinity.go
Original file line number Diff line number Diff line change
Expand Up @@ -368,7 +368,10 @@ func (c *blockAffinityClient) listV1(ctx context.Context, list model.BlockAffini
affinityType := list.AffinityType
requestedIPVersion := list.IPVersion

kvpl := &model.KVPairList{KVPairs: []*model.KVPair{}}
kvpl := &model.KVPairList{
KVPairs: []*model.KVPair{},
Revision: v3list.Revision,
}
for _, i := range v3list.KVPairs {
v1kvp, err := c.toV1(i)
if err != nil {
Expand Down
5 changes: 4 additions & 1 deletion libcalico-go/lib/backend/k8s/resources/ipam_block.go
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,10 @@ func (c *ipamBlockClient) List(ctx context.Context, list model.ListInterface, re
return nil, err
}

kvpl := &model.KVPairList{KVPairs: []*model.KVPair{}}
kvpl := &model.KVPairList{
KVPairs: []*model.KVPair{},
Revision: v3list.Revision,
}
for _, i := range v3list.KVPairs {
v1kvp, err := IPAMBlockV3toV1(i)
if err != nil {
Expand Down
5 changes: 4 additions & 1 deletion libcalico-go/lib/backend/k8s/resources/ipam_handle.go
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,10 @@ func (c *ipamHandleClient) List(ctx context.Context, list model.ListInterface, r
return nil, err
}

kvpl := &model.KVPairList{KVPairs: []*model.KVPair{}}
kvpl := &model.KVPairList{
KVPairs: []*model.KVPair{},
Revision: v3list.Revision,
}
for _, i := range v3list.KVPairs {
v1kvp := c.toV1(i)
kvpl.KVPairs = append(kvpl.KVPairs, v1kvp)
Expand Down
29 changes: 7 additions & 22 deletions libcalico-go/lib/backend/k8s/resources/k8sservice.go
Original file line number Diff line number Diff line change
Expand Up @@ -93,31 +93,16 @@ func (c *serviceClient) Get(ctx context.Context, key model.Key, revision string)
func (c *serviceClient) List(ctx context.Context, list model.ListInterface, revision string) (*model.KVPairList, error) {
log.Debug("Received List request on Kubernetes Service type")
rl := list.(model.ResourceListOptions)
kvps := []*model.KVPair{}

if rl.Name != "" {
// The service is already fully qualified, so perform a Get instead.
// If the entry does not exist then we just return an empty list.
kvp, err := c.Get(ctx, model.ResourceKey{Name: rl.Name, Namespace: rl.Namespace, Kind: model.KindKubernetesService}, revision)
if err != nil {
if _, ok := err.(cerrors.ErrorResourceDoesNotExist); !ok {
return nil, err
}
return &model.KVPairList{
KVPairs: kvps,
Revision: revision,
}, nil
}

kvps = append(kvps, kvp)
return &model.KVPairList{
KVPairs: kvps,
Revision: revision,
}, nil
}

// Listing all services.
listFunc := func(ctx context.Context, opts metav1.ListOptions) (runtime.Object, error) {
if rl.Name != "" {
// Filtering to a single Service.
opts.FieldSelector = fields.AndSelectors(
fields.OneTermEqualSelector("metadata.name", rl.Name),
fields.OneTermEqualSelector("metadata.namespace", rl.Namespace),
).String()
}
return c.clientSet.CoreV1().Services(rl.Namespace).List(ctx, opts)
}
convertFunc := func(r Resource) ([]*model.KVPair, error) {
Expand Down
26 changes: 4 additions & 22 deletions libcalico-go/lib/backend/k8s/resources/node.go
Original file line number Diff line number Diff line change
Expand Up @@ -140,31 +140,13 @@ func (c *nodeClient) List(ctx context.Context, list model.ListInterface, revisio
logContext := log.WithField("Resource", "Node")
logContext.Debug("Received List request")
nl := list.(model.ResourceListOptions)
kvps := []*model.KVPair{}

if nl.Name != "" {
// The node is already fully qualified, so perform a Get instead.
// If the entry does not exist then we just return an empty list.
kvp, err := c.Get(ctx, model.ResourceKey{Name: nl.Name, Kind: libapiv3.KindNode}, revision)
if err != nil {
if _, ok := err.(cerrors.ErrorResourceDoesNotExist); !ok {
return nil, err
}
return &model.KVPairList{
KVPairs: kvps,
Revision: revision,
}, nil
}

kvps = append(kvps, kvp)
return &model.KVPairList{
KVPairs: kvps,
Revision: revision,
}, nil
}

// List all nodes.
listFunc := func(ctx context.Context, opts metav1.ListOptions) (runtime.Object, error) {
if nl.Name != "" {
// Filtering to a single node.
opts.FieldSelector = fields.OneTermEqualSelector("metadata.name", nl.Name).String()
}
nodes, err := c.clientSet.CoreV1().Nodes().List(ctx, opts)
if err != nil {
return nil, err
Expand Down
73 changes: 53 additions & 20 deletions libcalico-go/lib/backend/k8s/resources/profile.go
Original file line number Diff line number Diff line change
Expand Up @@ -160,34 +160,42 @@ func (c *profileClient) List(ctx context.Context, list model.ListInterface, revi
logContext.Debug("Received List request")
nl := list.(model.ResourceListOptions)

// If a name is specified, then do an exact lookup.
if nl.Name == resources.DefaultAllowProfileName {
// Special case, we synthesize the default allow profile. Revision
// is always 1, and it cannot change (a watch on that profile returns
// no events).
return &model.KVPairList{
KVPairs: []*model.KVPair{resources.DefaultAllowProfile()},
Revision: "1",
}, nil
}

var nsName, saName string
if nl.Name != "" {
kvps := []*model.KVPair{}
kvp, err := c.Get(ctx, model.ResourceKey{Name: nl.Name, Kind: nl.Kind}, revision)
// If we're listing a specific profile, then we need to determine
// whether it's a namespace or service account.
var err error
nsName, saName, err = c.parseProfileName(nl.Name)
if err != nil {
if _, ok := err.(cerrors.ErrorResourceDoesNotExist); !ok {
return nil, err
}
return &model.KVPairList{
KVPairs: kvps,
Revision: revision,
}, nil
return nil, err
}

kvps = append(kvps, kvp)
return &model.KVPairList{
KVPairs: kvps,
Revision: revision,
}, nil
}

nsRev, saRev, err := c.SplitProfileRevision(revision)
if err != nil {
return nil, err
}

// Enumerate all namespaces, paginated.
// Enumerate matching namespaces, paginated.
listFunc := func(ctx context.Context, opts metav1.ListOptions) (runtime.Object, error) {
if saName != "" {
// We've been asked to list a particular service account, skip
// listing namespaces.
return &v1.NamespaceList{}, nil
}
if nsName != "" {
opts.FieldSelector = fields.OneTermEqualSelector("metadata.name", nsName).String()
}
return c.clientSet.CoreV1().Namespaces().List(ctx, opts)
}
convertFunc := func(r Resource) ([]*model.KVPair, error) {
Expand All @@ -203,9 +211,17 @@ func (c *profileClient) List(ctx context.Context, list model.ListInterface, revi
return nil, err
}

// Enumerate all service accounts, paginated.
// Enumerate matching service accounts, paginated.
listFunc = func(ctx context.Context, opts metav1.ListOptions) (runtime.Object, error) {
return c.clientSet.CoreV1().ServiceAccounts(v1.NamespaceAll).List(ctx, opts)
if nsName != "" && saName == "" {
// We've been asked to list a particular namespace, skip
// listing service accounts.
return &v1.ServiceAccountList{}, nil
}
if saName != "" {
opts.FieldSelector = fields.OneTermEqualSelector("metadata.name", saName).String()
}
return c.clientSet.CoreV1().ServiceAccounts(nsName).List(ctx, opts)
}
convertFunc = func(r Resource) ([]*model.KVPair, error) {
sa := r.(*v1.ServiceAccount)
Expand All @@ -221,14 +237,31 @@ func (c *profileClient) List(ctx context.Context, list model.ListInterface, revi
}

// Return a merged KVPairList including both results as well as the default-allow profile.
kvps := append([]*model.KVPair{resources.DefaultAllowProfile()}, nsKVPs.KVPairs...)
var kvps []*model.KVPair
if nsName == "" {
kvps = append(kvps, resources.DefaultAllowProfile())
}
kvps = append(kvps, nsKVPs.KVPairs...)
kvps = append(kvps, saKVPs.KVPairs...)
return &model.KVPairList{
KVPairs: kvps,
Revision: c.JoinProfileRevisions(nsKVPs.Revision, saKVPs.Revision),
}, nil
}

func (c *profileClient) parseProfileName(name string) (ns, sa string, err error) {
ns, err = c.ProfileNameToNamespace(name)
if err == nil {
return
}
ns, sa, err = c.ProfileNameToServiceAccount(name)
if err == nil {
return
}
err = fmt.Errorf("profile name neither namespace or service account %s", name)
return
}

func (c *profileClient) EnsureInitialized() error {
return nil
}
Expand Down
Loading
Loading