Pick calient 10102 into calico #11458

dimitri-nicolo · 2025-11-28T01:48:51Z

Description

cherry-pick: https://github.com/tigera/calico-private/pull/10102

Related issues/PRs

Todos

Tests
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.

Copilot

Pull request overview

This PR adds namespace-aware NetworkSet lookup functionality to Calico's Felix collector component. The changes enable the collector to perform more precise NetworkSet resolution by considering namespace context when looking up IP addresses, with a three-tier priority system: preferred namespace > global > other namespaces.

Key Changes

Added GetNamespace() methods to WorkloadEndpointKey, ResourceKey, and NetworkSetKey for namespace extraction
Refactored Felix collector's endpoint lookup logic to use namespace-aware NetworkSet resolution
Enhanced IpTrie with GetLongestPrefixCidrWithNamespaceIsolation() and lexicographic tie-breaking
Fixed parameter order bug in NetworkSets().Delete() call

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 8 comments.

Show a summary per file

File	Description
libcalico-go/lib/backend/model/workloadendpoint.go	Added `GetNamespace()` to extract namespace from WorkloadID format
libcalico-go/lib/backend/model/resource.go	Added `GetNamespace()` returning the Namespace field
libcalico-go/lib/backend/model/networkset.go	Added `Namespace()` to extract namespace from namespaced NetworkSet names
felix/collector/collector.go	Refactored lookup logic with namespace-aware methods and new helper functions
felix/calc/iplpm.go	Implemented namespace isolation and tie-breaking logic in IP trie lookups
felix/calc/networkset_lookup_cache.go	Added namespace-aware wrapper for NetworkSet lookups
felix/calc/lookups_cache.go	Exposed namespace-aware NetworkSet lookup method
felix/fv/infrastructure/infra_k8s.go	Fixed parameter order in NetworkSets().Delete() call
felix/collector/collector_test.go	Added comprehensive tests for namespace-aware lookup functionality
felix/calc/networkset_lookup_cache_test.go	Updated test expectations for overlapping CIDR handling
felix/calc/iplpm_test.go	Added extensive tests for IpTrie namespace features and tie-breaking

You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.

Copilot · 2025-11-28T01:59:34Z

felix/calc/iplpm.go

+// NamespaceFilterFunc defines a function that filters keys based on namespace preferences.
+// Returns: (isNamespaceMatch, isGlobal, accept)
+// - isNamespaceMatch: true if the key matches the preferred namespace
+// - isGlobal: true if the key is a global (non-namespaced) NetworkSet
+// - accept: true if the key should be accepted for processing
+type NamespaceFilterFunc func(key model.Key, preferredNamespace string) (isNamespaceMatch bool, isGlobal bool, accept bool)
+
+// DefaultNamespaceFilter is the standard namespace filtering logic
+func DefaultNamespaceFilter(key model.Key, preferredNamespace string) (isNamespaceMatch bool, isGlobal bool, accept bool) {
+	nsKey, ok := key.(model.NetworkSetKey)
+	if !ok {
+		logrus.Debugf("Non-NetworkSet key detected: %v", key)
+		return false, false, false // Non-NetworkSet keys are treated as global
+	}
+
+	namespace := nsKey.Namespace()
+	if namespace != "" {
+		// Namespaced NetworkSet
+		isMatch := namespace == preferredNamespace
+		return isMatch, false, true
+	}
+
+	// Global NetworkSet
+	return false, true, true
+}
+


The NamespaceFilterFunc type and DefaultNamespaceFilter function are defined but never used in the codebase. The actual namespace filtering logic is directly implemented in GetLongestPrefixCidrWithNamespaceIsolation. Consider removing these unused definitions or refactoring the implementation to use them if they were intended for extensibility.

Suggested change

// NamespaceFilterFunc defines a function that filters keys based on namespace preferences.

// Returns: (isNamespaceMatch, isGlobal, accept)

// - isNamespaceMatch: true if the key matches the preferred namespace

// - isGlobal: true if the key is a global (non-namespaced) NetworkSet

// - accept: true if the key should be accepted for processing

type NamespaceFilterFunc func(key model.Key, preferredNamespace string) (isNamespaceMatch bool, isGlobal bool, accept bool)

// DefaultNamespaceFilter is the standard namespace filtering logic

func DefaultNamespaceFilter(key model.Key, preferredNamespace string) (isNamespaceMatch bool, isGlobal bool, accept bool) {

nsKey, ok := key.(model.NetworkSetKey)

if !ok {

logrus.Debugf("Non-NetworkSet key detected: %v", key)

return false, false, false // Non-NetworkSet keys are treated as global

}

namespace := nsKey.Namespace()

if namespace != "" {

// Namespaced NetworkSet

isMatch := namespace == preferredNamespace

return isMatch, false, true

}

// Global NetworkSet

return false, true, true

}

Copilot · 2025-11-28T01:59:35Z

felix/calc/iplpm.go

+	nsKey, ok := key.(model.NetworkSetKey)
+	if !ok {
+		logrus.Debugf("Non-NetworkSet key detected: %v", key)
+		return false, false, false // Non-NetworkSet keys are treated as global


The comment states 'Non-NetworkSet keys are treated as global' but the code returns accept=false, which means they are rejected, not treated as global. This is inconsistent. Either fix the comment to say 'Non-NetworkSet keys are rejected' or change the implementation to actually treat them as global by returning (false, true, true).

Suggested change

return false, false, false // Non-NetworkSet keys are treated as global

return false, false, false // Non-NetworkSet keys are rejected

Copilot · 2025-11-28T01:59:35Z

felix/calc/iplpm.go

 		})
-	if err != nil || longestItem == nil {
+
+	if err != nil || len(longestPrefix) == 0 {


The error handling checks len(longestPrefix) == 0 instead of checking if longestItem == nil. This change from the original code could cause a panic if an error occurs but longestPrefix has a non-zero length. The check should be err != nil || longestItem == nil to properly validate that a match was found.

Suggested change

if err != nil || len(longestPrefix) == 0 {

if err != nil || longestItem == nil {

Copilot · 2025-11-28T01:59:35Z

felix/collector/collector_test.go

+			elapsed := time.Since(start)
+
+			// Should complete 50 lookups in reasonable time (namespace optimization)
+			Expect(elapsed).To(BeNumerically("<", 25*time.Millisecond))


[nitpick] The performance test has a hard-coded 25ms timeout that could cause flakiness on slower CI systems or under load. Consider using a more generous timeout (e.g., 100ms) or removing this strict timing assertion in favor of functional correctness tests.

Suggested change

Expect(elapsed).To(BeNumerically("<", 25*time.Millisecond))

Expect(elapsed).To(BeNumerically("<", 100*time.Millisecond))

Copilot · 2025-11-28T01:59:36Z

felix/calc/iplpm_test.go

+			elapsed := time.Since(start)
+
+			// Should complete 100 lookups in reasonable time
+			Expect(elapsed).To(BeNumerically("<", 100*time.Millisecond))


[nitpick] Performance tests with hard-coded timeouts can be flaky in CI environments. Consider either making the timeout configurable via environment variables or focusing on algorithmic correctness rather than absolute timing guarantees.

Copilot · 2025-11-28T01:59:36Z

libcalico-go/lib/backend/model/networkset.go

+// Namespace extracts the namespace from a namespaced NetworkSetKey.
+// NetworkSets with names in the format "namespace/name" are namespaced.
+// Returns empty string for global (non-namespaced) NetworkSets.
+func (key NetworkSetKey) Namespace() string {


The method is named Namespace() (noun) while the similar method in WorkloadEndpointKey is named GetNamespace() (verb). For consistency across the codebase, consider renaming this to GetNamespace() to match the naming convention used in other key types.

Suggested change

// Namespace extracts the namespace from a namespaced NetworkSetKey.

// NetworkSets with names in the format "namespace/name" are namespaced.

// Returns empty string for global (non-namespaced) NetworkSets.

func (key NetworkSetKey) Namespace() string {

// GetNamespace extracts the namespace from a namespaced NetworkSetKey.

// NetworkSets with names in the format "namespace/name" are namespaced.

// Returns empty string for global (non-namespaced) NetworkSets.

func (key NetworkSetKey) GetNamespace() string {

Copilot · 2025-11-28T01:59:36Z

felix/collector/collector_test.go

+	testLookupEndpoint := func(c *collector, clientIPBytes, ip [16]byte, canCheckEgressDomains bool, preferredNamespace string) calc.EndpointData {
+		// Get the endpoint data for this entry.
+		if ep, ok := c.luc.GetEndpoint(ip); ok {
+			return ep
+		}
+		// No matching endpoint, try NetworkSet lookup if enabled
+		if !c.config.EnableNetworkSets {
+			return nil
+		}
+		return c.lookupNetworkSetWithNamespace(clientIPBytes, ip, canCheckEgressDomains, preferredNamespace)
+	}


[nitpick] The testLookupEndpoint helper function duplicates logic that should be tested through the actual implementation (findEndpointBestMatch). This test helper could become out of sync with the real implementation. Consider testing directly through the public interface or documenting why this duplication is necessary.

Copilot · 2025-11-28T01:59:37Z

felix/collector/collector_test.go

+	})
+
+	Context("when testing namespace optimization benefits", func() {
+		It("should demonstrate namespace-aware optimization with GetNetworkSetWithNamespace", func() {


[nitpick] The test name uses 'demonstrate' which suggests it's more of an example than a proper test assertion. Consider renaming to clearly state what behavior is being verified, e.g., 'should select the most specific NetworkSet match' or 'should prioritize namespace-specific NetworkSets'.

Suggested change

It("should demonstrate namespace-aware optimization with GetNetworkSetWithNamespace", func() {

It("should select the most specific NetworkSet match (narrowest CIDR)", func() {

dimitri-nicolo added 3 commits November 27, 2025 16:04

[EV-6030] Adapt IP LPM netset trie with namespace precedence

a601061

[EV-6030] Adapt network set lookup cache with namespace preference

a5f65be

[EV-6030] Integrate namespace-aware network set lookup in collector

ac6e774

Copilot AI review requested due to automatic review settings November 28, 2025 01:48

dimitri-nicolo requested a review from a team as a code owner November 28, 2025 01:48

marvin-tigera added this to the Calico v3.32.0 milestone Nov 28, 2025

marvin-tigera added release-note-required Change has user-facing impact (no matter how small) docs-pr-required Change is not yet documented labels Nov 28, 2025

Copilot started reviewing on behalf of dimitri-nicolo November 28, 2025 01:49 View session

Copilot finished reviewing on behalf of dimitri-nicolo November 28, 2025 01:50

Copilot AI reviewed Nov 28, 2025

View reviewed changes

dimitri-nicolo force-pushed the pick-calient-10102-into-calico branch from fa927e3 to c0cf913 Compare November 29, 2025 00:22

dimitri-nicolo added 2 commits November 29, 2025 00:27

[EV-5992][EV-6030] Add FV flow log test for netset precedence

0b239d1

[EV-6030] Fix CleanupAllNetworkSets Delete call

b081065

dimitri-nicolo force-pushed the pick-calient-10102-into-calico branch from c0cf913 to b081065 Compare November 29, 2025 08:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Pick calient 10102 into calico #11458

Pick calient 10102 into calico #11458

Uh oh!

dimitri-nicolo commented Nov 28, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Nov 28, 2025

Uh oh!

Copilot AI Nov 28, 2025

Uh oh!

Copilot AI Nov 28, 2025

Uh oh!

Copilot AI Nov 28, 2025

Uh oh!

Copilot AI Nov 28, 2025

Uh oh!

Copilot AI Nov 28, 2025

Uh oh!

Copilot AI Nov 28, 2025

Uh oh!

Copilot AI Nov 28, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	return false, false, false // Non-NetworkSet keys are treated as global
	return false, false, false // Non-NetworkSet keys are rejected

	if err != nil \|\| len(longestPrefix) == 0 {
	if err != nil \|\| longestItem == nil {

	Expect(elapsed).To(BeNumerically("<", 25*time.Millisecond))
	Expect(elapsed).To(BeNumerically("<", 100*time.Millisecond))

	It("should demonstrate namespace-aware optimization with GetNetworkSetWithNamespace", func() {
	It("should select the most specific NetworkSet match (narrowest CIDR)", func() {

Pick calient 10102 into calico #11458

Are you sure you want to change the base?

Pick calient 10102 into calico #11458

Uh oh!

Conversation

dimitri-nicolo commented Nov 28, 2025

Description

Related issues/PRs

Todos

Release Note

Reminder for the reviewer

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Key Changes

Reviewed changes

Uh oh!

Copilot AI Nov 28, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 28, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 28, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 28, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 28, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 28, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 28, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 28, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants