feat: implement GroupMembership admission webhook to enforce existence and uniqueness constraints

cmd/milo/controller-manager/webhooks.go

@@ -100,6 +100,9 @@ func registerCoreControlPlaneWebhooksWithoutNotes(mgr controllerruntime.Manager)
 	if err := iamv1alpha1webhook.SetupUserDeactivationWebhooksWithManager(mgr, SystemNamespace); err != nil {
 		return fmt.Errorf("setting up userdeactivation webhook: %w", err)
 	}
+	if err := iamv1alpha1webhook.SetupGroupMembershipWebhooksWithManager(mgr); err != nil {
+		return fmt.Errorf("setting up groupmembership webhook: %w", err)
+	}
 	if err := identityv1alpha1webhook.SetupUserIdentityWebhooksWithManager(mgr); err != nil {
 		return fmt.Errorf("setting up useridentity webhook: %w", err)
 	}

config/webhook/manifests.yaml

@@ -284,6 +284,28 @@ kind: ValidatingWebhookConfiguration
 metadata:
   name: resourcemanager.miloapis.com
 webhooks:
+- admissionReviewVersions:
+  - v1
+  - v1beta1
+  clientConfig:
+    service:
+      name: milo-controller-manager
+      namespace: milo-system
+      path: /validate-iam-miloapis-com-v1alpha1-groupmembership
+      port: 9443
+  failurePolicy: Fail
+  name: vgroupmembership.iam.miloapis.com
+  rules:
+  - apiGroups:
+    - iam.miloapis.com
+    apiVersions:
+    - v1alpha1
+    operations:
+    - CREATE
+    - UPDATE
+    resources:
+    - groupmemberships
+  sideEffects: None
 - admissionReviewVersions:
   - v1
   - v1beta1

internal/webhooks/iam/v1alpha1/groupmembership_webhook.go

@@ -0,0 +1,132 @@
+package v1alpha1
+
+import (
+	"context"
+	"fmt"
+
+	"k8s.io/apimachinery/pkg/api/errors"
+	"k8s.io/apimachinery/pkg/util/validation/field"
+	ctrl "sigs.k8s.io/controller-runtime"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+	logf "sigs.k8s.io/controller-runtime/pkg/log"
+	"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
+
+	iamv1alpha1 "go.miloapis.com/milo/pkg/apis/iam/v1alpha1"
+)
+
+// groupmembershiplog is for logging in this package.
+var groupmembershiplog = logf.Log.WithName("groupmembership-resource")
+
+const groupMembershipCompositeKey = "iam.miloapis.com/groupmembership-composite"
+
+func buildGroupMembershipCompositeKey(userRef iamv1alpha1.UserReference, groupRef iamv1alpha1.GroupReference) string {
+	return fmt.Sprintf("%s|%s|%s", userRef.Name, groupRef.Namespace, groupRef.Name)
+}
+
+// +kubebuilder:webhook:path=/validate-iam-miloapis-com-v1alpha1-groupmembership,mutating=false,failurePolicy=fail,sideEffects=None,groups=iam.miloapis.com,resources=groupmemberships,verbs=create;update,versions=v1alpha1,name=vgroupmembership.iam.miloapis.com,admissionReviewVersions={v1,v1beta1},serviceName=milo-controller-manager,servicePort=9443,serviceNamespace=milo-system
+
+// +kubebuilder:rbac:groups=iam.miloapis.com,resources=groupmemberships,verbs=list
+// +kubebuilder:rbac:groups=iam.miloapis.com,resources=users,verbs=get
+// +kubebuilder:rbac:groups=iam.miloapis.com,resources=groups,verbs=get
+
+// SetupGroupMembershipWebhooksWithManager sets up the groupmembership webhook.
+func SetupGroupMembershipWebhooksWithManager(mgr ctrl.Manager) error {
+	groupmembershiplog.Info("Setting up iam.miloapis.com groupmembership webhooks")
+
+	// Composite index for exact membership tuple (user name + group ns + group name)
+	if err := mgr.GetFieldIndexer().IndexField(context.Background(), &iamv1alpha1.GroupMembership{}, groupMembershipCompositeKey, func(rawObj client.Object) []string {
+		gm := rawObj.(*iamv1alpha1.GroupMembership)
+		return []string{buildGroupMembershipCompositeKey(gm.Spec.UserRef, gm.Spec.GroupRef)}
+	}); err != nil {
+		return fmt.Errorf("failed to index groupmembership composite key: %w", err)
+	}
+
+	return ctrl.NewWebhookManagedBy(mgr, &iamv1alpha1.GroupMembership{}).
+		WithValidator(&GroupMembershipValidator{
+			client: mgr.GetClient(),
+		}).
+		Complete()
+}
+
+// GroupMembershipValidator validates GroupMemberships.
+//
+// Invariants enforced on create:
+//   - The referenced User must exist.
+//   - The referenced Group must exist.
+//   - A user may be a member of a given group at most once within a namespace.
+//
+// Updates are rejected outright because a GroupMembership is a pure (user,
+// group) link with no mutable spec. Delete and recreate the membership to
+// change which user belongs to which group.
+type GroupMembershipValidator struct {
+	client client.Client
+}
+
+func (v *GroupMembershipValidator) ValidateCreate(ctx context.Context, membership *iamv1alpha1.GroupMembership) (admission.Warnings, error) {
+	groupmembershiplog.Info("Validating GroupMembership create", "name", membership.Name, "namespace", membership.Namespace)
+
+	var errs field.ErrorList
+
+	// Validate referenced User exists
+	user := &iamv1alpha1.User{}
+	if err := v.client.Get(ctx, client.ObjectKey{Name: membership.Spec.UserRef.Name}, user); err != nil {
+		if errors.IsNotFound(err) {
+			errs = append(errs, field.NotFound(field.NewPath("spec", "userRef", "name"), membership.Spec.UserRef.Name))
+		} else {
+			return nil, errors.NewInternalError(fmt.Errorf("failed to get User %q: %w", membership.Spec.UserRef.Name, err))
+		}
+	}
+
+	// Validate referenced Group exists
+	group := &iamv1alpha1.Group{}
+	if err := v.client.Get(ctx, client.ObjectKey{
+		Namespace: membership.Spec.GroupRef.Namespace,
+		Name:      membership.Spec.GroupRef.Name,
+	}, group); err != nil {
+		if errors.IsNotFound(err) {
+			errs = append(errs, field.NotFound(field.NewPath("spec", "groupRef", "name"), membership.Spec.GroupRef.Name))
+		} else {
+			return nil, errors.NewInternalError(fmt.Errorf("failed to get Group %q in namespace %q: %w", membership.Spec.GroupRef.Name, membership.Spec.GroupRef.Namespace, err))
+		}
+	}
+
+	// Check for duplicate membership in the same namespace
+	key := buildGroupMembershipCompositeKey(membership.Spec.UserRef, membership.Spec.GroupRef)
+	var existing iamv1alpha1.GroupMembershipList
+	if err := v.client.List(ctx, &existing,
+		client.InNamespace(membership.Namespace),
+		client.MatchingFields{groupMembershipCompositeKey: key}); err != nil {
+		return nil, errors.NewInternalError(fmt.Errorf("failed to list group memberships: %w", err))
+	}
+	if len(existing.Items) > 0 {
+		dup := field.Duplicate(field.NewPath("spec"), key)
+		dup.Detail = fmt.Sprintf("user %q is already a member of group %q in namespace %q",
+			membership.Spec.UserRef.Name,
+			membership.Spec.GroupRef.Name,
+			membership.Spec.GroupRef.Namespace,
+		)
+		errs = append(errs, dup)
+	}
+
+	if len(errs) > 0 {
+		return nil, errors.NewInvalid(iamv1alpha1.SchemeGroupVersion.WithKind("GroupMembership").GroupKind(), membership.Name, errs)
+	}
+
+	return nil, nil
+}
+
+func (v *GroupMembershipValidator) ValidateUpdate(ctx context.Context, oldMembership, newMembership *iamv1alpha1.GroupMembership) (admission.Warnings, error) {
+	groupmembershiplog.Info("Validating GroupMembership update", "name", newMembership.Name, "namespace", newMembership.Namespace)
+
+	var errs field.ErrorList
+	errs = append(errs, field.Forbidden(
+		field.NewPath("spec"),
+		fmt.Sprintf("cannot update group membership %q: group memberships are immutable. Delete and recreate the membership to change which user belongs to which group", newMembership.Name),
+	))
+
+	return nil, errors.NewInvalid(iamv1alpha1.SchemeGroupVersion.WithKind("GroupMembership").GroupKind(), newMembership.Name, errs)
+}
+
+func (v *GroupMembershipValidator) ValidateDelete(ctx context.Context, obj *iamv1alpha1.GroupMembership) (admission.Warnings, error) {
+	return nil, nil
+}

internal/webhooks/iam/v1alpha1/groupmembership_webhook_test.go

@@ -0,0 +1,193 @@
+package v1alpha1
+
+import (
+	"context"
+	"strings"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	iamv1alpha1 "go.miloapis.com/milo/pkg/apis/iam/v1alpha1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/client/fake"
+)
+
+func newGroupMembership(name, namespace, user, group, groupNamespace string) *iamv1alpha1.GroupMembership {
+	return &iamv1alpha1.GroupMembership{
+		ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: namespace},
+		Spec: iamv1alpha1.GroupMembershipSpec{
+			UserRef: iamv1alpha1.UserReference{Name: user},
+			GroupRef: iamv1alpha1.GroupReference{
+				Name:      group,
+				Namespace: groupNamespace,
+			},
+		},
+	}
+}
+
+func newUser(name string) *iamv1alpha1.User {
+	return &iamv1alpha1.User{
+		ObjectMeta: metav1.ObjectMeta{Name: name},
+	}
+}
+
+func newGroup(name, namespace string) *iamv1alpha1.Group {
+	return &iamv1alpha1.Group{
+		ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: namespace},
+	}
+}
+
+func newTestValidator(objects ...client.Object) *GroupMembershipValidator {
+	builder := fake.NewClientBuilder().
+		WithScheme(runtimeScheme).
+		WithIndex(&iamv1alpha1.GroupMembership{}, groupMembershipCompositeKey, func(rawObj client.Object) []string {
+			gm := rawObj.(*iamv1alpha1.GroupMembership)
+			return []string{buildGroupMembershipCompositeKey(gm.Spec.UserRef, gm.Spec.GroupRef)}
+		})
+	if len(objects) > 0 {
+		builder = builder.WithObjects(objects...)
+	}
+	return &GroupMembershipValidator{
+		client: builder.Build(),
+	}
+}
+
+func TestGroupMembership_Create_Valid(t *testing.T) {
+	v := newTestValidator(
+		newUser("alice"),
+		newGroup("eng", "ns1"),
+	)
+
+	membership := newGroupMembership("gm-1", "ns1", "alice", "eng", "ns1")
+	_, err := v.ValidateCreate(context.Background(), membership)
+	require.NoError(t, err)
+}
+
+func TestGroupMembership_Create_DuplicateRejected(t *testing.T) {
+	existing := newGroupMembership("gm-1", "ns1", "alice", "eng", "ns1")
+	v := newTestValidator(
+		newUser("alice"),
+		newGroup("eng", "ns1"),
+		existing,
+	)
+
+	membership := newGroupMembership("gm-2", "ns1", "alice", "eng", "ns1")
+	_, err := v.ValidateCreate(context.Background(), membership)
+	require.Error(t, err, "duplicate user/group pair must be rejected")
+	assert.Contains(t, err.Error(), "alice")
+	assert.Contains(t, err.Error(), "eng")
+	assert.Contains(t, strings.ToLower(err.Error()), "already a member")
+}
+
+func TestGroupMembership_Create_DuplicateInDifferentGroupAllowed(t *testing.T) {
+	existing := newGroupMembership("gm-1", "ns1", "alice", "eng", "ns1")
+	v := newTestValidator(
+		newUser("alice"),
+		newGroup("eng", "ns1"),
+		newGroup("design", "ns1"),
+		existing,
+	)
+
+	membership := newGroupMembership("gm-2", "ns1", "alice", "design", "ns1")
+	_, err := v.ValidateCreate(context.Background(), membership)
+	require.NoError(t, err, "same user in a different group is allowed")
+}
+
+func TestGroupMembership_Create_DuplicateDifferentUserAllowed(t *testing.T) {
+	existing := newGroupMembership("gm-1", "ns1", "alice", "eng", "ns1")
+	v := newTestValidator(
+		newUser("alice"),
+		newUser("bob"),
+		newGroup("eng", "ns1"),
+		existing,
+	)
+
+	membership := newGroupMembership("gm-2", "ns1", "bob", "eng", "ns1")
+	_, err := v.ValidateCreate(context.Background(), membership)
+	require.NoError(t, err, "a different user in the same group is allowed")
+}
+
+func TestGroupMembership_Create_DuplicateDifferentNamespaceAllowed(t *testing.T) {
+	existing := newGroupMembership("gm-1", "ns1", "alice", "eng", "ns1")
+	v := newTestValidator(
+		newUser("alice"),
+		newGroup("eng", "ns1"),
+		newGroup("eng", "ns2"),
+		existing,
+	)
+
+	membership := newGroupMembership("gm-2", "ns2", "alice", "eng", "ns2")
+	_, err := v.ValidateCreate(context.Background(), membership)
+	require.NoError(t, err, "memberships in different namespaces are independent")
+}
+
+func TestGroupMembership_Create_UserDoesNotExistRejected(t *testing.T) {
+	v := newTestValidator(
+		newGroup("eng", "ns1"),
+	)
+
+	membership := newGroupMembership("gm-1", "ns1", "ghost", "eng", "ns1")
+	_, err := v.ValidateCreate(context.Background(), membership)
+	require.Error(t, err, "a create referencing a missing user must be rejected")
+	assert.Contains(t, err.Error(), `"ghost"`)
+}
+
+func TestGroupMembership_Create_GroupDoesNotExistRejected(t *testing.T) {
+	v := newTestValidator(
+		newUser("alice"),
+	)
+
+	membership := newGroupMembership("gm-1", "ns1", "alice", "ghost-group", "ns1")
+	_, err := v.ValidateCreate(context.Background(), membership)
+	require.Error(t, err, "a create referencing a missing group must be rejected")
+	assert.Contains(t, err.Error(), `"ghost-group"`)
+}
+
+func TestGroupMembership_Update_AnyUpdateRejected(t *testing.T) {
+	oldMembership := newGroupMembership("gm-1", "ns1", "alice", "eng", "ns1")
+	newMembership := newGroupMembership("gm-1", "ns1", "alice", "eng", "ns1")
+	v := newTestValidator()
+
+	_, err := v.ValidateUpdate(context.Background(), oldMembership, newMembership)
+	require.Error(t, err, "any update must be rejected")
+	assert.Contains(t, err.Error(), "immutable")
+}
+
+func TestGroupMembership_Update_ChangeGroupRejected(t *testing.T) {
+	oldMembership := newGroupMembership("gm-1", "ns1", "alice", "eng", "ns1")
+	updated := newGroupMembership("gm-1", "ns1", "alice", "design", "ns1")
+	v := newTestValidator()
+
+	_, err := v.ValidateUpdate(context.Background(), oldMembership, updated)
+	require.Error(t, err, "changing the group pointer must be rejected")
+	assert.Contains(t, err.Error(), "immutable")
+}
+
+func TestGroupMembership_Update_ChangeUserRejected(t *testing.T) {
+	oldMembership := newGroupMembership("gm-1", "ns1", "alice", "eng", "ns1")
+	updated := newGroupMembership("gm-1", "ns1", "bob", "eng", "ns1")
+	v := newTestValidator()
+
+	_, err := v.ValidateUpdate(context.Background(), oldMembership, updated)
+	require.Error(t, err, "changing the user pointer must be rejected")
+	assert.Contains(t, err.Error(), "immutable")
+}
+
+func TestGroupMembership_Update_ChangeGroupNamespaceRejected(t *testing.T) {
+	oldMembership := newGroupMembership("gm-1", "ns1", "alice", "eng", "ns1")
+	updated := newGroupMembership("gm-1", "ns1", "alice", "eng", "ns2")
+	v := newTestValidator()
+
+	_, err := v.ValidateUpdate(context.Background(), oldMembership, updated)
+	require.Error(t, err, "changing the referenced group namespace must be rejected")
+	assert.Contains(t, err.Error(), "immutable")
+}
+
+func TestGroupMembership_Delete_Allowed(t *testing.T) {
+	membership := newGroupMembership("gm-1", "ns1", "alice", "eng", "ns1")
+	v := newTestValidator()
+
+	_, err := v.ValidateDelete(context.Background(), membership)
+	require.NoError(t, err, "deletion must be allowed")
+}