Correctly convert fields that have vanished from v1alpha2

On-behalf-of: SAP <[email protected]>
Signed-off-by: Marvin Beckers <[email protected]>

Author: xmudrii

sdk/apis/apis/v1alpha2/types_apibinding_conversion.go

@@ -29,11 +29,13 @@ const (
 	AcceptablePermissionClaimsAnnotation = "apis.v1alpha2.kcp.io/acceptable-permission-claims"
 )
 
-// v1alpha2 -> v1alpha1
+// v1alpha2 -> v1alpha1 conversions.
 
 func Convert_v1alpha2_APIBinding_To_v1alpha1_APIBinding(in *APIBinding, out *apisv1alpha1.APIBinding, s kubeconversion.Scope) error {
 	out.ObjectMeta = *in.ObjectMeta.DeepCopy()
 
+	// before converting the spec, figure out which PermissionClaims could not be represented in v1alpha1 and
+	// retain them via an annotation
 	_, overhangingAPC, err := Convert_v1alpha2_AcceptablePermissionClaims_To_v1alpha1_AcceptablePermissionClaims(in.Spec.PermissionClaims, s)
 	if err != nil {
 		return err
@@ -54,6 +56,8 @@ func Convert_v1alpha2_APIBinding_To_v1alpha1_APIBinding(in *APIBinding, out *api
 		return err
 	}
 
+	// after converting the spec, read the retained information from the annotation and update PermissionClaims
+	// with All and ResourceSelectors that are not present in v1alpha2 (but saved in the annotation)
 	if originalPermissionClaims, ok := in.Annotations[PermissionClaimsV1Alpha1Annotation]; ok {
 		permissionClaims := []apisv1alpha1.AcceptablePermissionClaim{}
 		if err := json.Unmarshal([]byte(originalPermissionClaims), &permissionClaims); err != nil {
@@ -63,6 +67,7 @@ func Convert_v1alpha2_APIBinding_To_v1alpha1_APIBinding(in *APIBinding, out *api
 		for _, pc := range permissionClaims {
 			for i, opc := range out.Spec.PermissionClaims {
 				if pc.PermissionClaim.EqualGRI(opc.PermissionClaim) {
+					out.Spec.PermissionClaims[i].All = pc.All
 					out.Spec.PermissionClaims[i].ResourceSelector = pc.ResourceSelector
 				}
 			}
@@ -79,9 +84,12 @@ func Convert_v1alpha2_APIBinding_To_v1alpha1_APIBinding(in *APIBinding, out *api
 	return Convert_v1alpha2_APIBindingStatus_To_v1alpha1_APIBindingStatus(&in.Status, &out.Status, s)
 }
 
+// Convert_v1alpha2_AcceptablePermissionClaims_To_v1alpha1_AcceptablePermissionClaims converts v1alpha2.AcceptablePermissionClaims
+// to v1alpha1.AcceptablePermissionClaims. This is not a loseless conversion, verbs and selectors are lost in this conversion.
+// For loseless conversion use Convert_v1alpha2_APIBinding_To_v1alpha1_APIBinding.
 func Convert_v1alpha2_AcceptablePermissionClaims_To_v1alpha1_AcceptablePermissionClaims(in []AcceptablePermissionClaim, s kubeconversion.Scope) (out []apisv1alpha1.AcceptablePermissionClaim, overhanging []AcceptablePermissionClaim, err error) {
 	for _, apc := range in {
-		if len(apc.PermissionClaim.Verbs) == 1 && apc.PermissionClaim.Verbs[0] == "*" {
+		if len(apc.PermissionClaim.Verbs) == 1 && apc.PermissionClaim.Verbs[0] == "*" && apc.Selector.MatchAll {
 			var v1apc apisv1alpha1.AcceptablePermissionClaim
 
 			if err := Convert_v1alpha2_AcceptablePermissionClaim_To_v1alpha1_AcceptablePermissionClaim(&apc, &v1apc, s); err != nil {
@@ -97,8 +105,9 @@ func Convert_v1alpha2_AcceptablePermissionClaims_To_v1alpha1_AcceptablePermissio
 	return
 }
 
-// Convert_v1alpha2_AcceptablePermissionClaim_To_v1alpha1_AcceptablePermissionClaim converts v1alpha2.AcceptablePermissionClaim to v1alpha1.AcceptablePermissionClaim.
-// This is not a lossless conversion.
+// Convert_v1alpha2_AcceptablePermissionClaim_To_v1alpha1_AcceptablePermissionClaim converts v1alpha2.AcceptablePermissionClaim
+// to v1alpha1.AcceptablePermissionClaim. This is not a lossless conversion, selectors are lost in this conversion.
+// For loseless conversion use Convert_v1alpha2_APIBinding_To_v1alpha1_APIBinding.
 func Convert_v1alpha2_AcceptablePermissionClaim_To_v1alpha1_AcceptablePermissionClaim(in *AcceptablePermissionClaim, out *apisv1alpha1.AcceptablePermissionClaim, s kubeconversion.Scope) error {
 	if err := Convert_v1alpha2_ScopedPermissionClaim_To_v1alpha1_PermissionClaim(&in.ScopedPermissionClaim, &out.PermissionClaim, s); err != nil {
 		return err
@@ -107,6 +116,8 @@ func Convert_v1alpha2_AcceptablePermissionClaim_To_v1alpha1_AcceptablePermission
 	return nil
 }
 
+// Convert_v1alpha2_ScopedPermissionClaim_To_v1alpha1_PermissionClaim converts v1alhpa2.ScopedPermissionClaim to v1alpha1.PermissionClaim.
+// This is not a lossless conversion, for loseless conversion use Convert_v1alpha2_APIBinding_To_v1alpha1_APIBinding.
 func Convert_v1alpha2_ScopedPermissionClaim_To_v1alpha1_PermissionClaim(in *ScopedPermissionClaim, out *apisv1alpha1.PermissionClaim, s kubeconversion.Scope) error {
 	if err := Convert_v1alpha2_PermissionClaim_To_v1alpha1_PermissionClaim(&in.PermissionClaim, out, s); err != nil {
 		return err
@@ -115,7 +126,7 @@ func Convert_v1alpha2_ScopedPermissionClaim_To_v1alpha1_PermissionClaim(in *Scop
 	return nil
 }
 
-// v1alpha1 -> v1alpha2
+// v1alpha1 -> v1alpha2 conversions.
 
 func Convert_v1alpha1_APIBinding_To_v1alpha2_APIBinding(in *apisv1alpha1.APIBinding, out *APIBinding, s kubeconversion.Scope) error {
 	out.ObjectMeta = *in.ObjectMeta.DeepCopy()
@@ -127,19 +138,25 @@ func Convert_v1alpha1_APIBinding_To_v1alpha2_APIBinding(in *apisv1alpha1.APIBind
 		return err
 	}
 
-	// store acceptable permission claims in annotation. this is necessary for a clean conversion of
+	// store v1alpha1 acceptable permission claims in annotation. this is necessary for a clean conversion of
 	// ResourceSelector, which went away in v1alpha2.
-	if len(in.Spec.PermissionClaims) > 0 {
+	_, overhangingV1PC, err := Convert_v1alpha1_AcceptablePermissionClaims_To_v1alpha2_AcceptablePermissionClaims(in.Spec.PermissionClaims, s)
+	if err != nil {
+		return err
+	}
+	if len(overhangingV1PC) > 0 {
 		if out.Annotations == nil {
 			out.Annotations = make(map[string]string)
 		}
-		encoded, err := json.Marshal(in.Spec.PermissionClaims)
+		encoded, err := json.Marshal(overhangingV1PC)
 		if err != nil {
 			return err
 		}
 		out.Annotations[PermissionClaimsV1Alpha1Annotation] = string(encoded)
 	}
 
+	// store v1alpha2 permission claims in annotation. this is necessary for a clean conversion of
+	// verbs and label selectors, which were not existing in v1alpha1.
 	if overhangingAPC, ok := in.Annotations[AcceptablePermissionClaimsAnnotation]; ok {
 		acceptablePermissionClaims := []AcceptablePermissionClaim{}
 		if err := json.Unmarshal([]byte(overhangingAPC), &acceptablePermissionClaims); err != nil {
@@ -150,6 +167,7 @@ func Convert_v1alpha1_APIBinding_To_v1alpha2_APIBinding(in *apisv1alpha1.APIBind
 			for i, opc := range out.Spec.PermissionClaims {
 				if pc.EqualGRI(opc.PermissionClaim) {
 					out.Spec.PermissionClaims[i].PermissionClaim.Verbs = pc.Verbs
+					out.Spec.PermissionClaims[i].Selector = pc.Selector
 				}
 			}
 		}
@@ -166,48 +184,56 @@ func Convert_v1alpha1_APIBinding_To_v1alpha2_APIBinding(in *apisv1alpha1.APIBind
 		if len(opc.PermissionClaim.Verbs) == 0 {
 			out.Spec.PermissionClaims[i].PermissionClaim.Verbs = []string{"*"}
 		}
+		// This is handling a special case where PermissionClaim had ResourceSelector in v1alpha1.
+		// That field doesn't exist in v1alpha2 and it always resulted in `MatchAll = true` behavior,
+		// so we set it here explicitly.
+		if !opc.Selector.MatchAll && len(opc.Selector.MatchLabels) == 0 && len(opc.Selector.MatchExpressions) == 0 {
+			out.Spec.PermissionClaims[i].Selector.MatchAll = true
+		}
 	}
 
 	return nil
 }
 
+// Convert_v1alpha1_AcceptablePermissionClaims_To_v1alpha2_AcceptablePermissionClaims converts []v1alpha1.AcceptablePermissionClaim
+// to []v1alpha2.AcceptablePermissionClaim. This is not a lossless conversion, for loseless conversion use
+// Convert_v1alpha1_APIBinding_To_v1alpha2_APIBinding.
 func Convert_v1alpha1_AcceptablePermissionClaims_To_v1alpha2_AcceptablePermissionClaims(in []apisv1alpha1.AcceptablePermissionClaim, s kubeconversion.Scope) (out []AcceptablePermissionClaim, overhanging []apisv1alpha1.AcceptablePermissionClaim, err error) {
-	for _, apc := range in {
-		// ResourceSelector has been removed from PermissionClaims, we can't cleanly convert it.
-		if apc.PermissionClaim.All && len(apc.PermissionClaim.ResourceSelector) == 0 {
-			var v2apc AcceptablePermissionClaim
-			if err := Convert_v1alpha1_AcceptablePermissionClaim_To_v1alpha2_AcceptablePermissionClaim(&apc, &v2apc, s); err != nil {
+	for _, pc := range in {
+		if len(pc.ResourceSelector) > 0 {
+			overhanging = append(overhanging, pc)
+		} else {
+			var v2pc AcceptablePermissionClaim
+			if err := Convert_v1alpha1_AcceptablePermissionClaim_To_v1alpha2_AcceptablePermissionClaim(&pc, &v2pc, s); err != nil {
 				return nil, nil, err
 			}
-			out = append(out, v2apc)
-		} else {
-			overhanging = append(overhanging, apc)
+			out = append(out, v2pc)
 		}
 	}
 
-	return
+	return out, overhanging, nil
 }
 
-// Convert_v1alpha1_AcceptablePermissionClaim_To_v1alpha2_AcceptablePermissionClaim converts v1alpha1.AcceptablePermissionClaim to v1alpha2.AcceptablePermissionClaim.
-// This is not a lossless conversion.
+// Convert_v1alpha1_AcceptablePermissionClaim_To_v1alpha2_AcceptablePermissionClaim converts v1alpha1.AcceptablePermissionClaim
+// to v1alpha2.AcceptablePermissionClaim. This is not a lossless conversion, for loseless conversion use
+// Convert_v1alpha1_APIBinding_To_v1alpha2_APIBinding.
 func Convert_v1alpha1_AcceptablePermissionClaim_To_v1alpha2_AcceptablePermissionClaim(in *apisv1alpha1.AcceptablePermissionClaim, out *AcceptablePermissionClaim, s kubeconversion.Scope) error {
-	if err := Convert_v1alpha1_PermissionClaim_To_v1alpha2_PermissionClaim(&in.PermissionClaim, &out.PermissionClaim, s); err != nil {
+	if err := Convert_v1alpha1_PermissionClaim_To_v1alpha2_ScopedPermissionClaim(&in.PermissionClaim, &out.ScopedPermissionClaim, s); err != nil {
 		return err
 	}
-
 	out.State = AcceptablePermissionClaimState(in.State)
-	out.Selector.MatchAll = in.PermissionClaim.All
-	if len(out.PermissionClaim.Verbs) == 0 {
-		out.PermissionClaim.Verbs = []string{"*"}
-	}
 
 	return nil
 }
 
+// Convert_v1alpha1_PermissionClaim_To_v1alpha2_ScopedPermissionClaim converts v1alpha1.PermissionClaim
+// to v1alpha2.PermissionClaim. This is not a lossless conversion, for loseless conversion use
+// Convert_v1alpha1_APIBinding_To_v1alpha2_APIBinding.
 func Convert_v1alpha1_PermissionClaim_To_v1alpha2_ScopedPermissionClaim(in *apisv1alpha1.PermissionClaim, out *ScopedPermissionClaim, s kubeconversion.Scope) error {
 	if err := Convert_v1alpha1_PermissionClaim_To_v1alpha2_PermissionClaim(in, &out.PermissionClaim, s); err != nil {
 		return err
 	}
 	out.Selector.MatchAll = in.All
+
 	return nil
 }

sdk/apis/apis/v1alpha2/types_apibinding_conversion_test.go

@@ -21,6 +21,7 @@ import (
 
 	"github.com/google/go-cmp/cmp"
 
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 
 	apisv1alpha1 "github.com/kcp-dev/kcp/sdk/apis/apis/v1alpha1"
@@ -154,6 +155,66 @@ func TestConvertV1Alpha2APIBindings(t *testing.T) {
 				}},
 			},
 		},
+		{
+			Spec: APIBindingSpec{
+				Reference: BindingReference{
+					Export: &ExportBindingReference{
+						Path: "foo",
+						Name: "bar",
+					},
+				},
+				PermissionClaims: []AcceptablePermissionClaim{{
+					ScopedPermissionClaim: ScopedPermissionClaim{
+						PermissionClaim: PermissionClaim{
+							GroupResource: GroupResource{
+								Resource: "configmaps",
+							},
+							Verbs: []string{"get"},
+						},
+						Selector: PermissionClaimSelector{
+							LabelSelector: metav1.LabelSelector{
+								MatchLabels: map[string]string{
+									"test": "label",
+								},
+							},
+						},
+					},
+					State: ClaimAccepted,
+				}},
+			},
+		},
+		{
+			Spec: APIBindingSpec{
+				Reference: BindingReference{
+					Export: &ExportBindingReference{
+						Path: "foo",
+						Name: "bar",
+					},
+				},
+				PermissionClaims: []AcceptablePermissionClaim{{
+					ScopedPermissionClaim: ScopedPermissionClaim{
+						PermissionClaim: PermissionClaim{
+							GroupResource: GroupResource{
+								Resource: "configmaps",
+							},
+							Verbs: []string{"get"},
+						},
+						Selector: PermissionClaimSelector{
+							LabelSelector: metav1.LabelSelector{
+								MatchExpressions: []metav1.LabelSelectorRequirement{
+									{
+										Key:      "test",
+										Operator: metav1.LabelSelectorOpIn,
+										Values:   []string{"expression"},
+									},
+								},
+							},
+						},
+					},
+					State: ClaimAccepted,
+				}},
+			},
+		},
 	}
 
 	scheme := runtime.NewScheme()

sdk/apis/apis/v1alpha2/types_apiexport_conversion.go

@@ -51,6 +51,8 @@ func Convert_v1alpha2_APIExport_To_v1alpha1_APIExport(in *APIExport, out *apisv1
 		out.Annotations[ResourceSchemasAnnotation] = string(encoded)
 	}
 
+	// before converting the spec, figure out which PermissionClaims could not be represented in v1alpha1 and
+	// retain them via an annotation
 	_, overhangingPC, err := Convert_v1alpha2_PermissionClaims_To_v1alpha1_PermissionClaims(in.Spec.PermissionClaims, s)
 	if err != nil {
 		return err
@@ -71,10 +73,12 @@ func Convert_v1alpha2_APIExport_To_v1alpha1_APIExport(in *APIExport, out *apisv1
 		return err
 	}
 
+	// if the object was converted from v1alpha1 to v1alpha2 to v1alpha1, retain All and ResourceSelector
+	// fields in PermissionClaims by reading the original (v1alpha1) PermissionClaims from an annotation
 	if originalPermissionClaims, ok := in.Annotations[PermissionClaimsV1Alpha1Annotation]; ok {
 		permissionClaims := []apisv1alpha1.PermissionClaim{}
 		if err := json.Unmarshal([]byte(originalPermissionClaims), &permissionClaims); err != nil {
-			return fmt.Errorf("failed to decode schemas from JSON: %w", err)
+			return fmt.Errorf("failed to decode v1alpha1 claims from JSON: %w", err)
 		}
 
 		for _, pc := range permissionClaims {
@@ -138,8 +142,8 @@ func Convert_v1alpha2_PermissionClaims_To_v1alpha1_PermissionClaims(in []Permiss
 }
 
 // Convert_v1alpha2_APIExportSpec_To_v1alpha1_APIExportSpec is *not* lossless, as it will drop all non-CRD
-// resource schemas present in the APIExport's spec. To have a full, lossless conversion, use
-// Convert_v1alpha2_APIExport_To_v1alpha1_APIExport instead.
+// resource schemas and all non-wildcard PermissionClaims present in the APIExport's spec.
+// To have a full, lossless conversion, use Convert_v1alpha2_APIExport_To_v1alpha1_APIExport instead.
 func Convert_v1alpha2_APIExportSpec_To_v1alpha1_APIExportSpec(in *APIExportSpec, out *apisv1alpha1.APIExportSpec, s kubeconversion.Scope) error {
 	if in.Identity != nil {
 		out.Identity = &apisv1alpha1.Identity{}
@@ -281,6 +285,9 @@ func Convert_v1alpha1_APIExportSpec_To_v1alpha2_APIExportSpec(in *apisv1alpha1.A
 		}
 	}
 
+	// This will not convert non-wildcard PermissionClaims verbs and All/ResourceSelector fields. Those are still
+	// tucked away in an annotation and are converted after this function has completed, in
+	// Convert_v1alpha1_APIExport_To_v1alpha2_APIExport.
 	if claims := in.PermissionClaims; claims != nil {
 		newClaims := []PermissionClaim{}
 		for _, claim := range claims {
@@ -365,7 +372,7 @@ func Convert_v1alpha1_LatestResourceSchema_To_v1alpha2_ResourceSchema(in []strin
 }
 
 // Convert_v1alpha1_PermissionClaim_To_v1alpha2_PermissionClaim ensures we do the default conversion for
-// PermissionClaims. Fields "All" and "ResourceSelector" are stored in annotation.
+// PermissionClaims. Fields "All", "ResourceSelector", and "Verbs" are stored in relevant annotations.
 func Convert_v1alpha1_PermissionClaim_To_v1alpha2_PermissionClaim(in *apisv1alpha1.PermissionClaim, out *PermissionClaim, s kubeconversion.Scope) error {
 	return autoConvert_v1alpha1_PermissionClaim_To_v1alpha2_PermissionClaim(in, out, s)
 }