krt: fix FilterSelectsNonEmpty when the object has no labels (istio#5…

…2930) See the test; without this change, objects with no labels (WE, pod, etc) will get selected as part of a service.
MorrisLaw · Aug 30, 2024 · 273ffd0 · 273ffd0
1 parent 8bc1521
commit 273ffd0
Showing 4 changed files with 116 additions and 1 deletion.
diff --git a/pilot/pkg/serviceregistry/kube/controller/ambient/workloads.go b/pilot/pkg/serviceregistry/kube/controller/ambient/workloads.go
@@ -31,6 +31,7 @@ import (
 	"istio.io/istio/pilot/pkg/features"
 	"istio.io/istio/pilot/pkg/model"
 	"istio.io/istio/pilot/pkg/serviceregistry/kube"
+	"istio.io/istio/pilot/pkg/serviceregistry/serviceentry"
 	labelutil "istio.io/istio/pilot/pkg/serviceregistry/util/label"
 	"istio.io/istio/pkg/config/constants"
 	"istio.io/istio/pkg/config/labels"
@@ -130,6 +131,8 @@ func (a *index) workloadEntryWorkloadBuilder(
 	namespaces krt.Collection[*v1.Namespace],
 ) krt.TransformationSingle[*networkingclient.WorkloadEntry, model.WorkloadInfo] {
 	return func(ctx krt.HandlerContext, wle *networkingclient.WorkloadEntry) *model.WorkloadInfo {
+		// WLE can put labels in multiple places; normalize this
+		wle = serviceentry.ConvertClientWorkloadEntry(wle)
 		meshCfg := krt.FetchOne(ctx, meshConfig.AsCollection())
 		policies := a.buildWorkloadPolicies(ctx, authorizationPolicies, peerAuths, meshCfg, wle.Labels, wle.Namespace)
 		var waypoint *Waypoint

diff --git a/pilot/pkg/serviceregistry/kube/controller/ambient/workloads_test.go b/pilot/pkg/serviceregistry/kube/controller/ambient/workloads_test.go
@@ -723,6 +723,98 @@ func TestWorkloadEntryWorkloads(t *testing.T) {
 				},
 			},
 		},
+		{
+			name: "we without labels",
+			inputs: []any{
+				model.ServiceInfo{
+					Service: &workloadapi.Service{
+						Name:      "svc",
+						Namespace: "ns",
+						Hostname:  "hostname",
+						Ports: []*workloadapi.Port{{
+							ServicePort: 80,
+							TargetPort:  8080,
+						}},
+					},
+					LabelSelector: model.NewSelector(map[string]string{"app": "foo"}),
+				},
+			},
+			we: &networkingclient.WorkloadEntry{
+				TypeMeta: metav1.TypeMeta{},
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "name",
+					Namespace: "ns",
+				},
+				Spec: networking.WorkloadEntry{
+					Address: "1.2.3.4",
+				},
+			},
+			result: &workloadapi.Workload{
+				Uid:               "cluster0/networking.istio.io/WorkloadEntry/ns/name",
+				Name:              "name",
+				Namespace:         "ns",
+				Addresses:         [][]byte{netip.AddrFrom4([4]byte{1, 2, 3, 4}).AsSlice()},
+				Network:           testNW,
+				CanonicalName:     "name",
+				CanonicalRevision: "latest",
+				WorkloadType:      workloadapi.WorkloadType_POD,
+				WorkloadName:      "name",
+				Status:            workloadapi.WorkloadStatus_HEALTHY,
+				ClusterId:         testC,
+			},
+		},
+		{
+			name: "we spec labels",
+			inputs: []any{
+				model.ServiceInfo{
+					Service: &workloadapi.Service{
+						Name:      "svc",
+						Namespace: "ns",
+						Hostname:  "hostname",
+						Ports: []*workloadapi.Port{{
+							ServicePort: 80,
+							TargetPort:  8080,
+						}},
+					},
+					LabelSelector: model.NewSelector(map[string]string{"app": "foo"}),
+				},
+			},
+			we: &networkingclient.WorkloadEntry{
+				TypeMeta: metav1.TypeMeta{},
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "name",
+					Namespace: "ns",
+				},
+				Spec: networking.WorkloadEntry{
+					Address: "1.2.3.4",
+					// Labels in spec instead of metadata
+					Labels: map[string]string{
+						"app": "foo",
+					},
+				},
+			},
+			result: &workloadapi.Workload{
+				Uid:               "cluster0/networking.istio.io/WorkloadEntry/ns/name",
+				Name:              "name",
+				Namespace:         "ns",
+				Addresses:         [][]byte{netip.AddrFrom4([4]byte{1, 2, 3, 4}).AsSlice()},
+				Network:           testNW,
+				CanonicalName:     "foo",
+				CanonicalRevision: "latest",
+				WorkloadType:      workloadapi.WorkloadType_POD,
+				WorkloadName:      "name",
+				Status:            workloadapi.WorkloadStatus_HEALTHY,
+				ClusterId:         testC,
+				Services: map[string]*workloadapi.PortList{
+					"ns/hostname": {
+						Ports: []*workloadapi.Port{{
+							ServicePort: 80,
+							TargetPort:  8080,
+						}},
+					},
+				},
+			},
+		},
 		{
 			name: "pod with service named ports",
 			inputs: []any{

diff --git a/pilot/pkg/serviceregistry/serviceentry/controller.go b/pilot/pkg/serviceregistry/serviceentry/controller.go
@@ -24,6 +24,7 @@ import (
 	"k8s.io/apimachinery/pkg/types"
 
 	networking "istio.io/api/networking/v1alpha3"
+	clientnetworking "istio.io/client-go/pkg/apis/networking/v1"
 	"istio.io/istio/pilot/pkg/features"
 	"istio.io/istio/pilot/pkg/model"
 	"istio.io/istio/pilot/pkg/model/status"
@@ -197,6 +198,20 @@ func newController(store model.ConfigStore, xdsUpdater model.XDSUpdater, meshCon
 	return s
 }
 
+// ConvertClientWorkloadEntry merges the metadata.labels and spec.labels
+func ConvertClientWorkloadEntry(cfg *clientnetworking.WorkloadEntry) *clientnetworking.WorkloadEntry {
+	if cfg.Spec.Labels == nil {
+		// Short circuit, we don't have to do any conversion
+		return cfg
+	}
+	cfg = cfg.DeepCopy()
+	// Set both fields to be the merged result, so either can be used
+	cfg.Spec.Labels = maps.MergeCopy(cfg.Spec.Labels, cfg.Labels)
+	cfg.Labels = cfg.Spec.Labels
+
+	return cfg
+}
+
 // ConvertWorkloadEntry convert wle from Config.Spec and populate the metadata labels into it.
 func ConvertWorkloadEntry(cfg config.Config) *networking.WorkloadEntry {
 	wle := cfg.Spec.(*networking.WorkloadEntry)

diff --git a/pkg/kube/krt/filter.go b/pkg/kube/krt/filter.go
@@ -98,13 +98,18 @@ func FilterIndex[K comparable, I any](idx Index[K, I], k K) FetchOption {
 	}
 }
 
-// FilterSelectsNonEmpty only includes objects that select this label. If the selector is empty, it is not a match.
+// FilterSelectsNonEmpty only includes objects that select this label. If the selector is empty, it is NOT a match.
 func FilterSelectsNonEmpty(lbls map[string]string) FetchOption {
 	return func(h *dependency) {
+		// Need to distinguish empty vs unset. A user may pass in 'lbls' as nil, this doesn't mean they do not want it to filter at all.
+		if lbls == nil {
+			lbls = make(map[string]string)
+		}
 		h.filter.selectsNonEmpty = lbls
 	}
 }
 
+// FilterLabel only includes objects that match the provided labels. If the selector is empty, it IS a match.
 func FilterLabel(lbls map[string]string) FetchOption {
 	return func(h *dependency) {
 		h.filter.labels = lbls