Fine grained resource selection

[knotseaborg]

I noticed that this issue hasn't been active for a month, so I'm giving it a try.

Description

This draft addresses an enhancement suggestion for fine-grained selection of resources such as GlobalNetworkSet.

Solution

I forced calico to set the label projectcalico.org/kind when a resource is created using the clientv3 package. It seems to offer a consistent mechanism for resource selection.

Tests

• Existing tests have been modified to verify the label after creation of a resource.
• Some tests in calico/libcalico-go/lib/backend/syncersv1 have also been fixed by including the new label to prevent breakage.

---

@caseydavenport @sebhoss Let me know if this approach aligns with your intent, I can rework this draft.

Related issues/PRs

fixes #9857

Todos

• Tests - Passed tests for calico/libcalico-go
• Documentation
• Release note

Release Note

TBD

Reminder for the reviewer

Make sure that this PR has the correct labels and milestone set.

Every PR needs one docs-* label.

• docs-pr-required: This change requires a change to the documentation that has not been completed yet.
• docs-completed: This change has all necessary documentation completed.
• docs-not-required: This change has no user-facing impact and requires no docs.

Every PR needs one release-note-* label.

• release-note-required: This PR has user-facing changes. Most PRs should have this label.
• release-note-not-required: This PR has no user-facing changes.

Other optional labels:

• cherry-pick-candidate: This PR should be cherry-picked to an earlier release. For bug fixes only.
• needs-operator-pr: This PR is related to install and requires a corresponding change to the operator.

[CLAassistant]

All committers have signed the CLA.

[sebhoss]

I think it's great! I'm not quite sure how the selection of clientv3 packages interacts with something like auto creation of host endpoints though. Does it use v3 as well or does it directly use v1 and therefore bypass the code in this PR?

CC @fasaxc

[fasaxc]

Thanks for the PR. IIUC, this will add the label on write to the datastore as long as you go through our client package. TBH, I'm not sure if that's the best place to add the function because:

• It won't add the label to existing resources. So, on upgrade, you'll have a mix of resources with and without it.
• It won't help folks who bypass our Go API and write CRDs directly. Sadly there are quite a few folks who do this.
• It'll add the label to all our resources, not just the relevant ones. This might cause datastore bloat or confuse controllers that try to reconcile labels.
• Someone malicious could change the label to bypass policy rules.

For the other labels that we auto-populate, we've tended to add them on read, when we convert datastore objects into our in-memory representation. This has the downside that users can't see the labels with kubectl, but it does mean that we can easily retrofit a new special label and it prevents any sort of tampering. I think I'd be inclined to do it that way (And then, if we later decide that making the labels "real" is important, we could do it for all the labels that we synthesize.)

It's not as generic, but perhaps the right place to change is the individual clients in libcalico-go/lib/backend/k8s/resources.

• WorkloadEndpoints are backed by pods, so there's a conversion function that's invoked that could add the label.
• The others are backed by a general-purpose CRD client, I might add an opt-in applyKindLabel flag to customK8sResourceClient that tells the generic client to add the extra label to those on read (need to cover the Get,List,Watch cases).

[knotseaborg]

@fasaxc @sebhoss Thank you for the feedback. I am revising the draft PR and will push changes soon based on this suggestion.

It's not as generic, but perhaps the right place to change is the individual clients in libcalico-go/lib/backend/k8s/resources.

• WorkloadEndpoints are backed by pods, so there's a conversion function that's invoked that could add the label.
• The others are backed by a general-purpose CRD client, I might add an opt-in applyKindLabel flag to customK8sResourceClient that tells the generic client to add the extra label to those on read (need to cover the Get,List,Watch cases).

[fasaxc]

/sem-approve

[knotseaborg]

Sorry, I didn't mean to close this draft (In my attempt to achieve a clean reset for the new approach, I accidentally closed it)

Some updates

1. Commit 1 - add kind label to customresource

   • When applyKindLabel is enabled, customk8sResourceClient now adds the projectcalico.org/kind label to resources during Get, List, and Watch operations.

2. Commit 2 - test kind label with globalnetworkset CRUD

   • To validate this approach, I set applyKindLabel=true from the NewGlobalNetworkSetClient and updated related e2e tests.
   • Added label matching for GlobalNetworkSet-reads from the k8s datastore. I'm unsure if these test modifications fully align with expectations.

Did the tests pass?

No, but libcalico-go tests failed for both the master & this branch. The tests don't reach the part where updates can be validated.

@fasaxc Could you please take a look at my commits to make sure that they align with your intent? Any feedback will be helpful.

[fasaxc]

Yes, that's the kind of change I had in mind for the CRD client; but now I think about it, it'll be confusing if we don't also do this for etcd. I still think we should add the labels on read, but we should make sure that both datastore drivers return the new label for that subset of resources. Perhaps you could call a common function from both dataplanes that checks if the resource is one of the kinds that we should label and, if so, does the labeling. You also need to tweak the workload endpoint client, which doesn't use the CRD client.

I think the resources that should get labels are:

• NetSets
• GlobalNetsets
• WorkloadEndpoints
• HostEndpoints

[knotseaborg]

@fasaxc Thank you for the feedback! I will work on implementing this approach.

Perhaps you could call a common function from both dataplanes that checks if the resource is one of the kinds that we should label and, if so, does the labeling. You also need to tweak the workload endpoint client, which doesn't use the CRD client.

[knotseaborg]

Hello @fasaxc

Since our intention is to let the label live when calico uses it but never have it stored, I have a few concerns.

1. I think create which is used to store a resource, acts as a read which returns the created resource. Should we add the label to the resource returned by create as well for consistency?
2. I suspect that the update operation could potentially write the label into storage. Should it be tweaked so as to strip the kind label before write?

[github-actions]

This PR is stale because it has been open for 60 days with no activity.

[caseydavenport]

@knotseaborg @fasaxc what's the status on this guy? Seems like a nice one to have if we can!

[knotseaborg]

@caseydavenport @fasaxc @sebhoss Sorry for the stale draft, guys.

The last time I looked, I was blocked by non-deterministic test failures. I will revisit it this weekend and report back with the status.

If you've got any additional context or direction, I can adjust to that.