refactor Kptfile-label filtering introduced in PR 375

- add spec.packageMetadata.labels to existing selectable-fields
  framework
- add handling in fieldselector.go for "select key from map"-type
  selectors
  - handle them generically - pick out field name from square-bracket
    syntax and use this to fit them into existing selectable-fields
    framework
- make field selectors' KptfileLabels a Kubernetes labels.Selector
  to make matching less reliant on bespoke map matching

Author: JamesMcDermott

api/porch/v1alpha1/types.go

@@ -48,6 +48,7 @@ const (
 	PkgRevSelectorRepository    PkgRevFieldSelector = "spec.repository"
 	PkgRevSelectorWorkspaceName PkgRevFieldSelector = "spec.workspaceName"
 	PkgRevSelectorLifecycle     PkgRevFieldSelector = "spec.lifecycle"
+	PkgRevSelectorKptfileLabels PkgRevFieldSelector = "spec.packageMetadata.labels"
 )
 
 var PackageRevisionSelectableFields = []PkgRevFieldSelector{
@@ -58,6 +59,7 @@ var PackageRevisionSelectableFields = []PkgRevFieldSelector{
 	PkgRevSelectorRepository,
 	PkgRevSelectorWorkspaceName,
 	PkgRevSelectorLifecycle,
+	PkgRevSelectorKptfileLabels,
 }
 
 // PackageRevisionList

pkg/cache/dbcache/dbsqlfiltering.go

@@ -20,6 +20,7 @@ import (
 
 	porchapi "github.com/nephio-project/porch/api/porch/v1alpha1"
 	"github.com/nephio-project/porch/pkg/repository"
+	"k8s.io/apimachinery/pkg/labels"
 )
 
 func pkgListFilter2WhereClause(filter repository.ListPackageFilter) string {
@@ -134,15 +135,19 @@ func filter2SubClauseLifecycle(whereStatement string, filterField []porchapi.Pac
 	}
 }
 
-func filter2SubClauseKptfileLabels(whereStatement string, filterLabels map[string]string, first bool) (string, bool) {
-	if len(filterLabels) == 0 {
+func filter2SubClauseKptfileLabels(whereStatement string, filterLabels labels.Selector, first bool) (string, bool) {
+	if filterLabels == nil {
+		return whereStatement, first
+	}
+	requirements, _ := filterLabels.Requirements()
+	if len(requirements) == 0 {
 		return whereStatement, first
 	}
 
 	var subClauses []string
-	for labelKey, labelValue := range filterLabels {
+	for _, req := range requirements {
 		subClause := fmt.Sprintf("(package_revisions.spec::jsonb->'packageMetadata'->'labels'->>'%s' = '%s')",
-			labelKey, labelValue)
+			req.Key(), req.Values().List()[0])
 		subClauses = append(subClauses, subClause)
 	}
 

pkg/registry/porch/fieldselector.go

@@ -24,6 +24,7 @@ import (
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metainternalversion "k8s.io/apimachinery/pkg/apis/meta/internalversion"
 	"k8s.io/apimachinery/pkg/fields"
+	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/selection"
 )
 
@@ -74,6 +75,15 @@ var (
 			}
 			return err
 		},
+		porchapi.PkgRevSelectorKptfileLabels: func(filter *repository.ListPackageRevisionFilter, labelRequirement string) error {
+			parsedReq, err := labels.ParseToRequirements(labelRequirement)
+			if err != nil {
+				return apierrors.NewBadRequest(err.Error())
+			}
+
+			filter.KptfileLabels = filter.KptfileLabels.Add(parsedReq[0])
+			return nil
+		},
 	}
 )
 
@@ -93,18 +103,14 @@ func convertPackageFieldSelector(label, value string) (internalLabel, internalVa
 
 // convertPackageRevisionFieldSelector is the schema conversion function for normalizing the FieldSelector for PackageRevision
 func convertPackageRevisionFieldSelector(label, value string) (internalLabel, internalValue string, err error) {
-	if slices.Contains(porchapi.PackageRevisionSelectableFields, porchapi.PkgRevFieldSelector(label)) {
-		return label, value, nil
-	}
-
-	if strings.HasPrefix(label, "spec.packageMetadata.labels[") && strings.HasSuffix(label, "]") {
-		start := len("spec.packageMetadata.labels[")
-		end := len(label) - 1
-		labelKey := label[start:end]
-		if labelKey == "" {
+	mapSelectorSplit := strings.Split(label, "[")
+	if len(mapSelectorSplit) > 1 {
+		if labelKey := mapSelectorSplit[1]; labelKey == "]" {
 			return "", "", fmt.Errorf("label key cannot be empty in field selector %q", label)
 		}
+	}
 
+	if isSelectable := slices.Contains(porchapi.PackageRevisionSelectableFields, porchapi.PkgRevFieldSelector(mapSelectorSplit[0])); isSelectable {
 		return label, value, nil
 	}
 
@@ -159,8 +165,9 @@ func parsePackageFieldSelector(fieldSelector fields.Selector) (repository.ListPa
 // parsePackageRevisionFieldSelector parses client-provided fields.Selector into a ListPackageRevisionFilter
 func parsePackageRevisionFieldSelector(options *metainternalversion.ListOptions) (*repository.ListPackageRevisionFilter, error) {
 	filter := &repository.ListPackageRevisionFilter{
-		Label: options.LabelSelector,
-		Key:   repository.PackageRevisionKey{},
+		Label:         options.LabelSelector,
+		Key:           repository.PackageRevisionKey{},
+		KptfileLabels: labels.Everything(),
 	}
 
 	fieldSelector := options.FieldSelector
@@ -178,23 +185,10 @@ func parsePackageRevisionFieldSelector(options *metainternalversion.ListOptions)
 			return filter, apierrors.NewBadRequest(fmt.Sprintf("unsupported fieldSelector operator %q for field %q", requirement.Operator, requirement.Field))
 		}
 
-		if strings.HasPrefix(requirement.Field, "spec.packageMetadata.labels[") && strings.HasSuffix(requirement.Field, "]") {
-			start := len("spec.packageMetadata.labels[")
-			end := len(requirement.Field) - 1
-			labelKey := requirement.Field[start:end]
-
-			if labelKey == "" {
-				return filter, apierrors.NewBadRequest(fmt.Sprintf("label key cannot be empty in field selector %q", requirement.Field))
-			}
-
-			if filter.KptfileLabels == nil {
-				filter.KptfileLabels = make(map[string]string)
-			}
-
-			filter.KptfileLabels[labelKey] = requirement.Value
-
-			// skip the upcoming requirement.Field check
-			return filter, nil
+		mapSelectorSplit := strings.Split(requirement.Field, "[")
+		if len(mapSelectorSplit) > 1 {
+			requirement.Field = mapSelectorSplit[0]
+			requirement.Value = strings.Split(mapSelectorSplit[1], "]")[0] + "=" + requirement.Value
 		}
 
 		filteredField := porchapi.PkgRevFieldSelector(requirement.Field)

pkg/repository/repository.go

@@ -330,7 +330,7 @@ type ListPackageRevisionFilter struct {
 	Lifecycles []porchapi.PackageRevisionLifecycle
 
 	// KptfileLabels matches labels specified in the Kptfile
-	KptfileLabels map[string]string
+	KptfileLabels labels.Selector
 
 	Label labels.Selector
 }
@@ -345,22 +345,8 @@ func (f *ListPackageRevisionFilter) Matches(ctx context.Context, p PackageRevisi
 		return false
 	}
 
-	if len(f.KptfileLabels) > 0 {
-		packageRevision, err := p.GetPackageRevision(ctx)
-		if err != nil {
-			return false
-		}
-
-		if packageRevision.Spec.PackageMetadata == nil {
-			return false
-		}
-
-		for labelKey, expectedlValue := range f.KptfileLabels {
-			actualValue, exists := packageRevision.Spec.PackageMetadata.Labels[labelKey]
-			if !exists || actualValue != expectedlValue {
-				return false
-			}
-		}
+	if !f.MatchesKptfileLabels(ctx, p) {
+		return false
 	}
 
 	if !f.MatchesLabels(ctx, p) {
@@ -390,6 +376,20 @@ func (f *ListPackageRevisionFilter) MatchesLabels(ctx context.Context, p Package
 	return true
 }
 
+// MatchesKptfileLabels returns true if the filter either:
+//   - does not filter on labels in package metadata (nil KptfileLabels field), OR
+//   - matches on labels in the Kptfile of the resources of the provided PackageRevision
+func (f *ListPackageRevisionFilter) MatchesKptfileLabels(ctx context.Context, p PackageRevision) bool {
+	if f.KptfileLabels != nil {
+		packageRevision, err := p.GetPackageRevision(ctx)
+		if err != nil || packageRevision.Spec.PackageMetadata == nil {
+			return f.KptfileLabels.Matches(labels.Set(packageRevision.Spec.PackageMetadata.Labels))
+		}
+	}
+
+	return true
+}
+
 // getPkgRevLabels returns the metadata labels of a given PackageRevision for filtering purposes.
 // The labels are returned in the form of a Kubernetes labels.Set which can be easily matched
 // against a labels.Selector which came in in a list request.