warmchang · pull · May 19, 2025 · May 9, 2025 · May 19, 2025
diff --git a/cli/pkg/workspace/plugin/use.go b/cli/pkg/workspace/plugin/use.go
@@ -30,6 +30,7 @@ import (
 
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/cli-runtime/pkg/genericclioptions"
 	"k8s.io/client-go/tools/clientcmd"
 	clientcmdapi "k8s.io/client-go/tools/clientcmd/api"
@@ -424,20 +425,27 @@ func getAPIBindings(ctx context.Context, kcpClusterClient kcpclientset.ClusterIn
 func findUnresolvedPermissionClaims(out io.Writer, apiBindings []apisv1alpha2.APIBinding) error {
 	for _, binding := range apiBindings {
 		for _, exportedClaim := range binding.Status.ExportPermissionClaims {
-			var found, ack bool
+			var found, ack, verbsMatch bool
+			var verbsExpected, verbsActual sets.Set[string]
 			for _, specClaim := range binding.Spec.PermissionClaims {
 				if !exportedClaim.Equal(specClaim.PermissionClaim) {
 					continue
 				}
 				found = true
 				ack = (specClaim.State == apisv1alpha2.ClaimAccepted) || specClaim.State == apisv1alpha2.ClaimRejected
+				verbsExpected = sets.New(exportedClaim.Verbs...)
+				verbsActual = sets.New(specClaim.Verbs...)
+				verbsMatch = verbsActual.Difference(verbsExpected).Len() == 0 && verbsExpected.Difference(verbsActual).Len() == 0
 			}
 			if !found {
 				fmt.Fprintf(out, "Warning: claim for %s exported but not specified on APIBinding %s\nAdd this claim to the APIBinding's Spec.\n", exportedClaim.String(), binding.Name)
 			}
 			if !ack {
 				fmt.Fprintf(out, "Warning: claim for %s specified on APIBinding %s but not accepted or rejected.\n", exportedClaim.String(), binding.Name)
 			}
+			if !verbsMatch {
+				fmt.Fprintf(out, "Warning: allowed verbs (%s) on claim for %s on APIBinding %s do not match expected verbs (%s).\n", strings.Join(verbsActual.UnsortedList(), ","), exportedClaim.String(), binding.Name, strings.Join(verbsExpected.UnsortedList(), ","))
+			}
 		}
 	}
 	return nil

diff --git a/config/crds/apis.kcp.io_apibindings.yaml b/config/crds/apis.kcp.io_apibindings.yaml
@@ -535,9 +535,20 @@ spec:
                       - Accepted
                       - Rejected
                       type: string
+                    verbs:
+                      description: |-
+                        verbs is a list of supported API operation types (this includes
+                        but is not limited to get, list, watch, create, update, patch,
+                        delete, deletecollection, and proxy).
+                      items:
+                        type: string
+                      minItems: 1
+                      type: array
+                      x-kubernetes-list-type: set
                   required:
                   - resource
                   - state
+                  - verbs
                   type: object
                   x-kubernetes-validations:
                   - message: either "all" or "resourceSelector" must be set
@@ -641,8 +652,19 @@ spec:
                         - message: at least one field must be set
                           rule: has(self.__namespace__) || has(self.name)
                       type: array
+                    verbs:
+                      description: |-
+                        verbs is a list of supported API operation types (this includes
+                        but is not limited to get, list, watch, create, update, patch,
+                        delete, deletecollection, and proxy).
+                      items:
+                        type: string
+                      minItems: 1
+                      type: array
+                      x-kubernetes-list-type: set
                   required:
                   - resource
+                  - verbs
                   type: object
                   x-kubernetes-validations:
                   - message: either "all" or "resourceSelector" must be set
@@ -821,8 +843,19 @@ spec:
                         - message: at least one field must be set
                           rule: has(self.__namespace__) || has(self.name)
                       type: array
+                    verbs:
+                      description: |-
+                        verbs is a list of supported API operation types (this includes
+                        but is not limited to get, list, watch, create, update, patch,
+                        delete, deletecollection, and proxy).
+                      items:
+                        type: string
+                      minItems: 1
+                      type: array
+                      x-kubernetes-list-type: set
                   required:
                   - resource
+                  - verbs
                   type: object
                   x-kubernetes-validations:
                   - message: either "all" or "resourceSelector" must be set

diff --git a/config/crds/apis.kcp.io_apiexports.yaml b/config/crds/apis.kcp.io_apiexports.yaml
@@ -443,8 +443,19 @@ spec:
                         - message: at least one field must be set
                           rule: has(self.__namespace__) || has(self.name)
                       type: array
+                    verbs:
+                      description: |-
+                        verbs is a list of supported API operation types (this includes
+                        but is not limited to get, list, watch, create, update, patch,
+                        delete, deletecollection, and proxy).
+                      items:
+                        type: string
+                      minItems: 1
+                      type: array
+                      x-kubernetes-list-type: set
                   required:
                   - resource
+                  - verbs
                   type: object
                   x-kubernetes-validations:
                   - message: either "all" or "resourceSelector" must be set

diff --git a/pkg/admission/apibinding/apibinding_admission.go b/pkg/admission/apibinding/apibinding_admission.go
@@ -18,6 +18,7 @@ package apibinding
 
 import (
 	"context"
+	"encoding/json"
 	"errors"
 	"fmt"
 	"io"
@@ -92,6 +93,13 @@ func (o *apiBindingAdmission) Admit(ctx context.Context, a admission.Attributes,
 		return apierrors.NewInternalError(err)
 	}
 
+	if a.GetResource().GroupResource() == apisv1alpha1.Resource("apibindings") {
+		ab := &apisv1alpha1.APIBinding{}
+		if err := validateOverhangingPermissionClaims(ctx, a, ab); err != nil {
+			return admission.NewForbidden(a, err)
+		}
+	}
+
 	if a.GetResource().GroupResource() != apisv1alpha2.Resource("apibindings") {
 		return nil
 	}
@@ -312,3 +320,43 @@ func (o *apiBindingAdmission) SetKcpInformers(local, global kcpinformers.SharedI
 		indexers.ByLogicalClusterPathAndName: indexers.IndexByLogicalClusterPathAndName,
 	})
 }
+
+func validateOverhangingPermissionClaims(_ context.Context, _ admission.Attributes, ab *apisv1alpha1.APIBinding) error {
+	// TODO(xmudrii): Remove this once we are sure that all APIExport objects are
+	// converted to v1alpha2.
+	if _, ok := ab.Annotations[apisv1alpha2.PermissionClaimsAnnotation]; ok {
+		// validate if we can decode overhanging permission claims. If not, we will fail.
+		var overhanging []apisv1alpha2.PermissionClaim
+		if err := json.Unmarshal([]byte(ab.Annotations[apisv1alpha2.PermissionClaimsAnnotation]), &overhanging); err != nil {
+			return field.Invalid(field.NewPath("metadata").Child("annotations").Key(apisv1alpha2.PermissionClaimsAnnotation), ab.Annotations[apisv1alpha2.PermissionClaimsAnnotation], "failed to decode overhanging permission claims")
+		}
+
+		// validate mismatches. We could have mismatches between the spec and the annotation
+		// (e.g. a resource present in the annotation, but not in the spec).
+		// We convert to v2 to check for mismatches.
+		v2Claims := make([]apisv1alpha2.PermissionClaim, len(ab.Spec.PermissionClaims))
+		for i, v1pc := range ab.Spec.PermissionClaims {
+			var v2pc apisv1alpha2.PermissionClaim
+			err := apisv1alpha2.Convert_v1alpha1_PermissionClaim_To_v1alpha2_PermissionClaim(&v1pc.PermissionClaim, &v2pc, nil)
+			if err != nil {
+				return field.Invalid(field.NewPath("spec").Child("permissionClaims").Index(i), ab.Spec.PermissionClaims, "failed to convert spec.PermissionClaims")
+			}
+			v2Claims = append(v2Claims, v2pc)
+		}
+
+		for _, o := range overhanging {
+			var found bool
+			for _, pc := range v2Claims {
+				if pc.Equal(o) {
+					found = true
+
+					break
+				}
+			}
+			if !found {
+				return field.Invalid(field.NewPath("metadata").Child("annotations").Key(apisv1alpha2.PermissionClaimsAnnotation), ab.Annotations[apisv1alpha2.PermissionClaimsAnnotation], "permission claims defined in annotation do not match permission claims defined in spec")
+			}
+		}
+	}
+	return nil
+}
diff --git a/pkg/admission/apibinding/apibinding_admission_test.go b/pkg/admission/apibinding/apibinding_admission_test.go
@@ -19,6 +19,7 @@ package apibinding
 import (
 	"context"
 	"crypto/sha256"
+	"encoding/json"
 	"errors"
 	"math/big"
 	"strings"
@@ -555,3 +556,154 @@ func newExport(path logicalcluster.Path, name string) apiExportBuilder {
 		},
 	}}
 }
+
+func TestValidateOverhangingPermissionClaims(t *testing.T) {
+	tests := map[string]struct {
+		annotations      func() map[string]string
+		permissionClaims []apisv1alpha1.AcceptablePermissionClaim
+		expectedError    string
+	}{
+		"NoAnnotations": {
+			annotations:      func() map[string]string { return nil },
+			permissionClaims: nil,
+			expectedError:    "",
+		},
+		"EmptyJSON": {
+			annotations: func() map[string]string {
+				pc := apisv1alpha2.PermissionClaim{}
+				data, err := json.Marshal(pc)
+				if err != nil {
+					t.Fatalf("failed to marshal: %v", err)
+				}
+				return map[string]string{
+					apisv1alpha2.PermissionClaimsAnnotation: string(data),
+				}
+			},
+			permissionClaims: nil,
+			expectedError:    "failed to decode overhanging permission claims",
+		},
+		"EmptyPermissionClaimsAndAnnotation": {
+			annotations: func() map[string]string {
+				return map[string]string{
+					apisv1alpha2.PermissionClaimsAnnotation: "[]",
+				}
+			},
+			permissionClaims: []apisv1alpha1.AcceptablePermissionClaim{},
+			expectedError:    "",
+		},
+		"ValidJSON": {
+			annotations: func() map[string]string {
+				s := []apisv1alpha2.PermissionClaim{{
+					GroupResource: apisv1alpha2.GroupResource{
+						Group:    "foo",
+						Resource: "bar",
+					},
+					All:          true,
+					IdentityHash: "baz",
+					Verbs:        []string{"get", "list"},
+				}}
+				data, err := json.Marshal(s)
+				if err != nil {
+					t.Fatalf("failed to marshal: %v", err)
+				}
+				return map[string]string{
+					apisv1alpha2.ResourceSchemasAnnotation: string(data),
+				}
+			},
+			permissionClaims: []apisv1alpha1.AcceptablePermissionClaim{
+				{
+					PermissionClaim: apisv1alpha1.PermissionClaim{
+						GroupResource: apisv1alpha1.GroupResource{
+							Group:    "foo",
+							Resource: "bar",
+						},
+						All:          true,
+						IdentityHash: "baz",
+					},
+				},
+			},
+			expectedError: "",
+		},
+		"MismatchInAnnotations": {
+			annotations: func() map[string]string {
+				s := []apisv1alpha2.PermissionClaim{
+					{
+						GroupResource: apisv1alpha2.GroupResource{
+							Group:    "foo",
+							Resource: "bar",
+						},
+						All:          true,
+						IdentityHash: "baz",
+						Verbs:        []string{"get", "list"},
+					},
+					{
+						GroupResource: apisv1alpha2.GroupResource{
+							Group:    "foo",
+							Resource: "baz",
+						},
+						All:          true,
+						IdentityHash: "bar",
+						Verbs:        []string{"get"},
+					},
+				}
+				data, err := json.Marshal(s)
+				if err != nil {
+					t.Fatalf("failed to marshal: %v", err)
+				}
+				return map[string]string{
+					apisv1alpha2.PermissionClaimsAnnotation: string(data),
+				}
+			},
+			permissionClaims: []apisv1alpha1.AcceptablePermissionClaim{
+				{
+					PermissionClaim: apisv1alpha1.PermissionClaim{
+						GroupResource: apisv1alpha1.GroupResource{
+							Group:    "foo",
+							Resource: "bar",
+						},
+						All:          true,
+						IdentityHash: "baz",
+					},
+				},
+				{
+					PermissionClaim: apisv1alpha1.PermissionClaim{
+						GroupResource: apisv1alpha1.GroupResource{
+							Group:    "test",
+							Resource: "schema",
+						},
+						All:          true,
+						IdentityHash: "random",
+					},
+				},
+			},
+			expectedError: "permission claims defined in annotation do not match permission claims defined in spec",
+		},
+		"InvalidJSON": {
+			annotations: func() map[string]string {
+				return map[string]string{
+					apisv1alpha2.PermissionClaimsAnnotation: "invalid json",
+				}
+			},
+			permissionClaims: nil,
+			expectedError:    "failed to decode overhanging permission claims",
+		},
+	}
+	for name, tc := range tests {
+		t.Run(name, func(t *testing.T) {
+			ae := &apisv1alpha1.APIBinding{
+				ObjectMeta: metav1.ObjectMeta{
+					Annotations: tc.annotations(),
+				},
+				Spec: apisv1alpha1.APIBindingSpec{
+					PermissionClaims: tc.permissionClaims,
+				},
+			}
+			err := validateOverhangingPermissionClaims(context.TODO(), nil, ae)
+			if tc.expectedError == "" {
+				require.NoError(t, err)
+			} else {
+				require.Contains(t, err.Error(), tc.expectedError)
+			}
+		})
+	}
+}