diff --git a/pkg/registry/apis/iam/team_binding_hooks.go b/pkg/registry/apis/iam/team_binding_hooks.go index c43c3a91ab1..dcad58cae0a 100644 --- a/pkg/registry/apis/iam/team_binding_hooks.go +++ b/pkg/registry/apis/iam/team_binding_hooks.go @@ -86,6 +86,7 @@ func (b *IdentityAccessManagementAPIBuilder) AfterTeamBindingCreate(obj runtime. "name", tb.Name, "subject", tb.Spec.Subject.Name, "teamRef", tb.Spec.TeamRef.Name, + "permission", tb.Spec.Permission, "err", err, ) status = "failure" @@ -117,6 +118,7 @@ func (b *IdentityAccessManagementAPIBuilder) AfterTeamBindingCreate(obj runtime. "name", tb.Name, "subject", tb.Spec.Subject.Name, "teamRef", tb.Spec.TeamRef.Name, + "permission", tb.Spec.Permission, ) } else { // Record successful tuple write @@ -143,6 +145,20 @@ func (b *IdentityAccessManagementAPIBuilder) BeginTeamBindingUpdate(ctx context. return nil, nil } + if oldTB.Spec.Subject.Name == newTB.Spec.Subject.Name && oldTB.Spec.TeamRef.Name == newTB.Spec.TeamRef.Name && oldTB.Spec.Permission == newTB.Spec.Permission { + return nil, nil // No changes to the team binding + } + + if newTB.Spec.Subject.Name == "" || newTB.Spec.TeamRef.Name == "" { + b.logger.Error("invalid team binding", + "namespace", newTB.Namespace, + "name", newTB.Name, + "subject", newTB.Spec.Subject.Name, + "teamRef", newTB.Spec.TeamRef.Name, + ) + return nil, nil + } + // Convert old team binding to tuple for deletion var oldTuple *v1.TupleKey var oldErr error @@ -154,6 +170,7 @@ func (b *IdentityAccessManagementAPIBuilder) BeginTeamBindingUpdate(ctx context. "name", oldTB.Name, "err", oldErr, ) + return nil, nil } } @@ -168,18 +185,16 @@ func (b *IdentityAccessManagementAPIBuilder) BeginTeamBindingUpdate(ctx context. "name", newTB.Name, "err", newErr, ) + return nil, nil } } // Return a finish function that performs the zanzana write only on success return func(ctx context.Context, success bool) { if !success { - // Update failed, don't write to zanzana return } - // Grab a ticket to write to Zanzana - // This limits the amount of concurrent connections to Zanzana wait := time.Now() b.zTickets <- true hooksWaitHistogram.WithLabelValues("teambinding", "update").Observe(time.Since(wait).Seconds()) diff --git a/pkg/registry/apis/iam/team_binding_hooks_test.go b/pkg/registry/apis/iam/team_binding_hooks_test.go index 869060bcacb..2f56ba12c87 100644 --- a/pkg/registry/apis/iam/team_binding_hooks_test.go +++ b/pkg/registry/apis/iam/team_binding_hooks_test.go @@ -487,6 +487,223 @@ func TestBeginTeamBindingUpdate(t *testing.T) { require.NoError(t, err) require.Nil(t, finishFunc) // Should return nil when zClient is nil }) + + t.Run("should handle empty old binding subject name gracefully", func(t *testing.T) { + wg.Add(1) + oldBinding := iamv0.TeamBinding{ + ObjectMeta: metav1.ObjectMeta{ + Name: "binding-6", + Namespace: "org-6", + }, + Spec: iamv0.TeamBindingSpec{ + Subject: iamv0.TeamBindingspecSubject{ + Name: "", // Empty name - conversion will be skipped + }, + TeamRef: iamv0.TeamBindingTeamRef{ + Name: "team-1", + }, + Permission: iamv0.TeamBindingTeamPermissionMember, + }, + } + + newBinding := iamv0.TeamBinding{ + ObjectMeta: metav1.ObjectMeta{ + Name: "binding-6", + Namespace: "org-6", + }, + Spec: iamv0.TeamBindingSpec{ + Subject: iamv0.TeamBindingspecSubject{ + Name: "user-2", + }, + TeamRef: iamv0.TeamBindingTeamRef{ + Name: "team-1", + }, + Permission: iamv0.TeamBindingTeamPermissionMember, + }, + } + + testEmptyOldBinding := func(ctx context.Context, req *v1.WriteRequest) error { + defer wg.Done() + require.NotNil(t, req) + require.Equal(t, "org-6", req.Namespace) + + // Should not delete old binding (it was skipped due to empty name) + require.Nil(t, req.Deletes) + + // Should write new binding + require.NotNil(t, req.Writes) + require.Len(t, req.Writes.TupleKeys, 1) + require.Equal( + t, + req.Writes.TupleKeys[0], + &v1.TupleKey{User: "user:user-2", Relation: "member", Object: "team:team-1"}, + ) + + return nil + } + + b.zClient = &FakeZanzanaClient{writeCallback: testEmptyOldBinding} + + finishFunc, err := b.BeginTeamBindingUpdate(context.Background(), &newBinding, &oldBinding, nil) + require.NoError(t, err) + require.NotNil(t, finishFunc) // Should still return finish function + + finishFunc(context.Background(), true) + wg.Wait() + }) + + t.Run("should return nil finish func when bindings are identical", func(t *testing.T) { + oldBinding := iamv0.TeamBinding{ + ObjectMeta: metav1.ObjectMeta{ + Name: "binding-7", + Namespace: "org-7", + }, + Spec: iamv0.TeamBindingSpec{ + Subject: iamv0.TeamBindingspecSubject{ + Name: "user-1", + }, + TeamRef: iamv0.TeamBindingTeamRef{ + Name: "team-1", + }, + Permission: iamv0.TeamBindingTeamPermissionMember, + }, + } + + newBinding := iamv0.TeamBinding{ + ObjectMeta: metav1.ObjectMeta{ + Name: "binding-7", + Namespace: "org-7", + }, + Spec: iamv0.TeamBindingSpec{ + Subject: iamv0.TeamBindingspecSubject{ + Name: "user-1", + }, + TeamRef: iamv0.TeamBindingTeamRef{ + Name: "team-1", + }, + Permission: iamv0.TeamBindingTeamPermissionMember, + }, + } + + writeCalled := false + testNoWriteOnNoChange := func(ctx context.Context, req *v1.WriteRequest) error { + writeCalled = true + require.Fail(t, "Write should not be called when bindings are identical") + return nil + } + + b.zClient = &FakeZanzanaClient{writeCallback: testNoWriteOnNoChange} + + finishFunc, err := b.BeginTeamBindingUpdate(context.Background(), &newBinding, &oldBinding, nil) + require.NoError(t, err) + require.Nil(t, finishFunc) // Should return nil when bindings are identical + + // Verify write was never called + time.Sleep(100 * time.Millisecond) + require.False(t, writeCalled, "Write callback should not be called when bindings are identical") + }) + + t.Run("should return nil finish func when new binding has empty subject name", func(t *testing.T) { + oldBinding := iamv0.TeamBinding{ + ObjectMeta: metav1.ObjectMeta{ + Name: "binding-8", + Namespace: "org-8", + }, + Spec: iamv0.TeamBindingSpec{ + Subject: iamv0.TeamBindingspecSubject{ + Name: "user-1", + }, + TeamRef: iamv0.TeamBindingTeamRef{ + Name: "team-1", + }, + Permission: iamv0.TeamBindingTeamPermissionMember, + }, + } + + newBinding := iamv0.TeamBinding{ + ObjectMeta: metav1.ObjectMeta{ + Name: "binding-8", + Namespace: "org-8", + }, + Spec: iamv0.TeamBindingSpec{ + Subject: iamv0.TeamBindingspecSubject{ + Name: "", // Empty name - should cause early return + }, + TeamRef: iamv0.TeamBindingTeamRef{ + Name: "team-1", + }, + Permission: iamv0.TeamBindingTeamPermissionMember, + }, + } + + writeCalled := false + testNoWriteOnInvalidBinding := func(ctx context.Context, req *v1.WriteRequest) error { + writeCalled = true + require.Fail(t, "Write should not be called when new binding has empty subject name") + return nil + } + + b.zClient = &FakeZanzanaClient{writeCallback: testNoWriteOnInvalidBinding} + + finishFunc, err := b.BeginTeamBindingUpdate(context.Background(), &newBinding, &oldBinding, nil) + require.NoError(t, err) + require.Nil(t, finishFunc) // Should return nil when new binding has empty subject name + + // Verify write was never called + time.Sleep(100 * time.Millisecond) + require.False(t, writeCalled, "Write callback should not be called when new binding has empty subject name") + }) + + t.Run("should return nil finish func when new binding has empty team ref name", func(t *testing.T) { + oldBinding := iamv0.TeamBinding{ + ObjectMeta: metav1.ObjectMeta{ + Name: "binding-9", + Namespace: "org-9", + }, + Spec: iamv0.TeamBindingSpec{ + Subject: iamv0.TeamBindingspecSubject{ + Name: "user-1", + }, + TeamRef: iamv0.TeamBindingTeamRef{ + Name: "team-1", + }, + Permission: iamv0.TeamBindingTeamPermissionMember, + }, + } + + newBinding := iamv0.TeamBinding{ + ObjectMeta: metav1.ObjectMeta{ + Name: "binding-9", + Namespace: "org-9", + }, + Spec: iamv0.TeamBindingSpec{ + Subject: iamv0.TeamBindingspecSubject{ + Name: "user-2", + }, + TeamRef: iamv0.TeamBindingTeamRef{ + Name: "", // Empty name - should cause early return + }, + Permission: iamv0.TeamBindingTeamPermissionMember, + }, + } + + writeCalled := false + testNoWriteOnInvalidBinding := func(ctx context.Context, req *v1.WriteRequest) error { + writeCalled = true + require.Fail(t, "Write should not be called when new binding has empty team ref name") + return nil + } + + b.zClient = &FakeZanzanaClient{writeCallback: testNoWriteOnInvalidBinding} + + finishFunc, err := b.BeginTeamBindingUpdate(context.Background(), &newBinding, &oldBinding, nil) + require.NoError(t, err) + require.Nil(t, finishFunc) // Should return nil when new binding has empty team ref name + + // Verify write was never called + time.Sleep(100 * time.Millisecond) + require.False(t, writeCalled, "Write callback should not be called when new binding has empty team ref name") + }) } func TestAfterTeamBindingDelete(t *testing.T) {