Fixed bugs with creation & deletion of role assignments when using intents v1 Azure roles.

src/operator/controllers/intents_reconcilers/iam/iam_reconciler.go

@@ -122,7 +122,7 @@ func (r *IAMIntentsReconciler) applyTypedIAMIntents(ctx context.Context, pod cor
 	}
 
 	filteredIntents := intents.GetFilteredTargetList(intentType)
-	err = agent.AddRolePolicyFromIntents(ctx, pod.Namespace, serviceAccountName, intents.Spec.Workload.Name, filteredIntents, pod)
+	err = agent.AddRolePolicyFromIntents(ctx, pod.Namespace, serviceAccountName, intents.Spec.Workload.Name, intents, filteredIntents, pod)
 	if err != nil {
 		r.RecordWarningEventf(&intents, consts.ReasonReconcilingIAMPoliciesFailed, "Failed to reconcile IAM policies of type %s due to error: %s", intentType, err.Error())
 		return errors.Wrap(err)

src/operator/controllers/intents_reconcilers/iam/iam_reconciler_test.go

@@ -158,7 +158,19 @@ func (s *IAMIntentsReconcilerTestSuite) TestCreateIAMIntentCallingTheiamAgent()
 		gomock.AssignableToTypeOf(&otterizev2alpha1.ClientIntentsList{}),
 		&client.ListOptions{Namespace: testNamespace},
 	).Return(nil)
-	s.iamAgent.EXPECT().AddRolePolicyFromIntents(gomock.Any(), testNamespace, testClientServiceAccount, testServiceName, filteredIntents, clientPod).Return(nil)
+
+	clientIntents := otterizev2alpha1.ClientIntents{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      testClientIntentsName,
+			Namespace: testNamespace,
+		},
+		Spec: &otterizev2alpha1.IntentsSpec{
+			Workload: otterizev2alpha1.Workload{Name: testServiceName},
+			Targets:  allIntents,
+		},
+	}
+
+	s.iamAgent.EXPECT().AddRolePolicyFromIntents(gomock.Any(), testNamespace, testClientServiceAccount, testServiceName, clientIntents, clientPod).Return(nil)
 
 	res, err := s.Reconciler.Reconcile(context.Background(), req)
 	s.NoError(err)
@@ -205,7 +217,8 @@ func (s *IAMIntentsReconcilerTestSuite) TestCreateIAMIntentPartialDeleteCallingT
 		gomock.AssignableToTypeOf(&otterizev2alpha1.ClientIntentsList{}),
 		&client.ListOptions{Namespace: testNamespace},
 	).Return(nil)
-	s.iamAgent.EXPECT().AddRolePolicyFromIntents(gomock.Any(), testNamespace, testClientServiceAccount, testServiceName, []otterizev2alpha1.Target{}, clientPod).Return(nil)
+
+	s.iamAgent.EXPECT().AddRolePolicyFromIntents(gomock.Any(), testNamespace, testClientServiceAccount, testServiceName, clientIntents, clientPod).Return(nil)
 
 	res, err := s.Reconciler.Reconcile(context.Background(), req)
 	s.NoError(err)
@@ -246,7 +259,7 @@ func (s *IAMIntentsReconcilerTestSuite) TestRoleNotFoundErrorReQueuesEvent() {
 	).Return(nil)
 
 	// Throw the sentinel error
-	s.iamAgent.EXPECT().AddRolePolicyFromIntents(gomock.Any(), testNamespace, testClientServiceAccount, testServiceName, []otterizev2alpha1.Target{}, clientPod).Return(
+	s.iamAgent.EXPECT().AddRolePolicyFromIntents(gomock.Any(), testNamespace, testClientServiceAccount, testServiceName, iamIntents, clientPod).Return(
 		errors.Errorf("%w: %s", agentutils.ErrCloudIdentityNotFound, "test error"),
 	)
 

src/operator/controllers/intents_reconcilers/iam/iampolicyagents/awspolicyagent/agent.go

@@ -64,8 +64,8 @@ func (a *Agent) createPolicyFromIntents(intents []otterizev2alpha1.Target) awsag
 	return policy
 }
 
-func (a *Agent) AddRolePolicyFromIntents(ctx context.Context, namespace string, accountName string, intentsServiceName string, intents []otterizev2alpha1.Target, pod corev1.Pod) error {
-	policyDoc := a.createPolicyFromIntents(intents)
+func (a *Agent) AddRolePolicyFromIntents(ctx context.Context, namespace string, accountName string, intentsServiceName string, _ otterizev2alpha1.ClientIntents, filteredTargets []otterizev2alpha1.Target, _ corev1.Pod) error {
+	policyDoc := a.createPolicyFromIntents(filteredTargets)
 	return a.agent.AddRolePolicy(ctx, namespace, accountName, intentsServiceName, policyDoc.Statement)
 }
 

src/operator/controllers/intents_reconcilers/iam/iampolicyagents/awspolicyagent/multi_account_aws_policy_agent.go

@@ -29,12 +29,12 @@ func (m *MultiaccountAWSPolicyAgent) AppliesOnPod(pod *corev1.Pod) bool {
 	return multi_account_aws_agent.AppliesOnPod(pod)
 }
 
-func (m *MultiaccountAWSPolicyAgent) AddRolePolicyFromIntents(ctx context.Context, namespace string, accountName string, intentsServiceName string, intents []otterizev2alpha1.Target, pod corev1.Pod) error {
+func (m *MultiaccountAWSPolicyAgent) AddRolePolicyFromIntents(ctx context.Context, namespace string, accountName string, intentsServiceName string, intents otterizev2alpha1.ClientIntents, filteredTargets []otterizev2alpha1.Target, pod corev1.Pod) error {
 	agent, err := m.getAgentForPod(&pod)
 	if err != nil {
 		return errors.Wrap(err)
 	}
-	return agent.AddRolePolicyFromIntents(ctx, namespace, accountName, intentsServiceName, intents, pod)
+	return agent.AddRolePolicyFromIntents(ctx, namespace, accountName, intentsServiceName, intents, filteredTargets, pod)
 }
 
 func (m *MultiaccountAWSPolicyAgent) DeleteRolePolicyFromIntents(ctx context.Context, intents otterizev2alpha1.ClientIntents) error {

src/operator/controllers/intents_reconcilers/iam/iampolicyagents/azurepolicyagent/agent.go

@@ -14,11 +14,9 @@ import (
 	"github.com/otterize/intents-operator/src/shared/errors"
 	"github.com/samber/lo"
 	corev1 "k8s.io/api/core/v1"
-	"k8s.io/apimachinery/pkg/util/wait"
 	"regexp"
 	"strings"
 	"sync"
-	"time"
 )
 
 var KeyVaultNameRegex = regexp.MustCompile(`^/subscriptions/[^/]+/resourceGroups/[^/]+/providers/Microsoft.KeyVault/vaults/([^/]+)$`)
@@ -33,9 +31,6 @@ type Agent struct {
 func NewAzurePolicyAgent(ctx context.Context, azureAgent *azureagent.Agent) *Agent {
 	agent := &Agent{azureAgent, sync.Mutex{}, sync.Mutex{}}
 
-	// Start periodic tasks goroutine
-	go wait.Forever(func() { agent.PeriodicTasks(ctx) }, 5*time.Hour)
-
 	return agent
 }
 
@@ -75,7 +70,7 @@ func (a *Agent) getIntentScope(ctx context.Context, intent otterizev2alpha1.Targ
 	return fullScope, nil
 }
 
-func (a *Agent) AddRolePolicyFromIntents(ctx context.Context, namespace string, _ string, intentsServiceName string, intents []otterizev2alpha1.Target, _ corev1.Pod) error {
+func (a *Agent) AddRolePolicyFromIntents(ctx context.Context, namespace string, _ string, intentsServiceName string, intents otterizev2alpha1.ClientIntents, _ []otterizev2alpha1.Target, _ corev1.Pod) error {
 	userAssignedIdentity, exists, err := a.FindUserAssignedIdentity(ctx, namespace, intentsServiceName)
 	if err != nil {
 		return errors.Wrap(err)
@@ -84,35 +79,22 @@ func (a *Agent) AddRolePolicyFromIntents(ctx context.Context, namespace string,
 		return errors.Errorf("%w: %s-%s", agentutils.ErrCloudIdentityNotFound, namespace, intentsServiceName)
 	}
 
-	// Custom roles
-	azureCustomRolesIntents := lo.Filter(intents, func(intent otterizev2alpha1.Target, _ int) bool {
-		hasCustomRoles := intent.Azure != nil && (len(intent.Azure.Actions) > 0 || len(intent.Azure.DataActions) > 0)
-		return hasCustomRoles && len(intent.Azure.Roles) == 0
-	})
-	if err := a.ensureCustomRolesForIntents(ctx, userAssignedIdentity, azureCustomRolesIntents); err != nil {
+	if err := a.ensureCustomRolesForIntents(ctx, userAssignedIdentity, intents); err != nil {
 		return errors.Wrap(err)
 	}
 
-	// Backwards compatibility for role assignments
-	azureRBACIntents := lo.Filter(intents, func(intent otterizev2alpha1.Target, _ int) bool {
-		return intent.Azure != nil && len(intent.Azure.Roles) > 0
-	})
-	if err := a.ensureRoleAssignmentsForIntents(ctx, userAssignedIdentity, azureRBACIntents); err != nil {
+	if err := a.ensureRoleAssignmentsForIntents(ctx, userAssignedIdentity, intents); err != nil {
 		return errors.Wrap(err)
 	}
 
-	// Key Vault permissions
-	azureKeyVaultIntents := lo.Filter(intents, func(intent otterizev2alpha1.Target, _ int) bool {
-		return intent.Azure != nil && intent.Azure.KeyVaultPolicy != nil
-	})
-	if err := a.ensureKeyVaultPermissionsForIntents(ctx, userAssignedIdentity, azureKeyVaultIntents); err != nil {
+	if err := a.ensureKeyVaultPermissionsForIntents(ctx, userAssignedIdentity, intents); err != nil {
 		return errors.Wrap(err)
 	}
 
 	return nil
 }
 
-func (a *Agent) ensureRoleAssignmentsForIntents(ctx context.Context, userAssignedIdentity armmsi.Identity, intents []otterizev2alpha1.Target) error {
+func (a *Agent) ensureRoleAssignmentsForIntents(ctx context.Context, userAssignedIdentity armmsi.Identity, clientIntents otterizev2alpha1.ClientIntents) error {
 	// Lock the agent to ensure that no other goroutine is modifying the assignments
 	a.assignmentMutex.Lock()
 	defer a.assignmentMutex.Unlock()
@@ -131,8 +113,13 @@ func (a *Agent) ensureRoleAssignmentsForIntents(ctx context.Context, userAssigne
 		return *roleAssignment.Properties.Scope
 	})
 
+	// Backwards compatibility for role assignments
+	azureRBACIntents := lo.Filter(clientIntents.GetTargetList(), func(intent otterizev2alpha1.Target, _ int) bool {
+		return intent.Azure != nil && len(intent.Azure.Roles) > 0
+	})
+
 	var expectedScopes []string
-	for _, intent := range intents {
+	for _, intent := range azureRBACIntents {
 		scope, err := a.getIntentScope(ctx, intent)
 		if err != nil {
 			return errors.Wrap(err)
@@ -233,6 +220,24 @@ func (a *Agent) DeleteRolePolicyFromIntents(ctx context.Context, intents otteriz
 		}
 	}
 
+	// Custom roles
+	azureCustomRolesTargets := lo.Filter(intents.GetTargetList(), func(intent otterizev2alpha1.Target, _ int) bool {
+		hasCustomRoles := intent.Azure != nil && (len(intent.Azure.Actions) > 0 || len(intent.Azure.DataActions) > 0)
+		return hasCustomRoles && len(intent.Azure.Roles) == 0
+	})
+
+	for _, target := range azureCustomRolesTargets {
+		scope, err := a.getIntentScope(ctx, target)
+		if err != nil {
+			return errors.Wrap(err)
+		}
+
+		err = a.DeleteCustomRole(ctx, scope, intents)
+		if err != nil {
+			return errors.Wrap(err)
+		}
+	}
+
 	existingKeyVaultsAccessPolicies, err := a.GetExistingKeyVaultAccessPolicies(ctx, userAssignedIdentity)
 	if err != nil {
 		return errors.Wrap(err)
@@ -256,15 +261,20 @@ func extractKeyVaultName(scope string) (string, error) {
 	return match[1], nil
 }
 
-func (a *Agent) ensureKeyVaultPermissionsForIntents(ctx context.Context, userAssignedIdentity armmsi.Identity, intents []otterizev2alpha1.Target) error {
+func (a *Agent) ensureKeyVaultPermissionsForIntents(ctx context.Context, userAssignedIdentity armmsi.Identity, clientIntents otterizev2alpha1.ClientIntents) error {
 	existingKeyVaultsAccessPolicies, err := a.GetExistingKeyVaultAccessPolicies(ctx, userAssignedIdentity)
 	if err != nil {
 		return errors.Wrap(err)
 	}
 
 	var expectedIntentsKeyVaults []string
 
-	for _, intent := range intents {
+	// Key Vault permissions
+	azureKeyVaultIntents := lo.Filter(clientIntents.GetTargetList(), func(intent otterizev2alpha1.Target, _ int) bool {
+		return intent.Azure != nil && intent.Azure.KeyVaultPolicy != nil
+	})
+
+	for _, intent := range azureKeyVaultIntents {
 		scope, err := a.getIntentScope(ctx, intent)
 		if err != nil {
 			return errors.Wrap(err)
@@ -348,7 +358,7 @@ func (a *Agent) vaultAccessPolicyEntryFromIntent(userAssignedIdentity armmsi.Ide
 	}
 }
 
-func (a *Agent) ensureCustomRolesForIntents(ctx context.Context, userAssignedIdentity armmsi.Identity, intents []otterizev2alpha1.Target) error {
+func (a *Agent) ensureCustomRolesForIntents(ctx context.Context, userAssignedIdentity armmsi.Identity, clientIntents otterizev2alpha1.ClientIntents) error {
 	// Lock the agent to ensure that no other goroutine is modifying the custom roles in parallel
 	a.roleMutex.Lock()
 	defer a.roleMutex.Unlock()
@@ -358,21 +368,27 @@ func (a *Agent) ensureCustomRolesForIntents(ctx context.Context, userAssignedIde
 		return errors.Wrap(err)
 	}
 
-	// Filter out assignments on predefined roles
+	// Filter out non-otterize roles
 	existingRoleAssignments = lo.Filter(existingRoleAssignments, func(roleAssignment armauthorization.RoleAssignment, _ int) bool {
 		return a.IsCustomRoleAssignment(roleAssignment)
 	})
 
+	// Custom roles
+	azureCustomRolesTargets := lo.Filter(clientIntents.GetTargetList(), func(intent otterizev2alpha1.Target, _ int) bool {
+		hasCustomRoles := intent.Azure != nil && (len(intent.Azure.Actions) > 0 || len(intent.Azure.DataActions) > 0)
+		return hasCustomRoles && len(intent.Azure.Roles) == 0
+	})
+
 	var expectedScopes []string
-	for _, intent := range intents {
-		scope, err := a.getIntentScope(ctx, intent)
+	for _, target := range azureCustomRolesTargets {
+		scope, err := a.getIntentScope(ctx, target)
 		if err != nil {
 			return errors.Wrap(err)
 		}
 
 		expectedScopes = append(expectedScopes, scope)
 
-		err = a.ensureCustomRoleForIntent(ctx, userAssignedIdentity, scope, intent)
+		err = a.ensureCustomRoleForIntent(ctx, userAssignedIdentity, scope, clientIntents, target)
 		if err != nil {
 			return errors.Wrap(err)
 		}
@@ -385,11 +401,11 @@ func (a *Agent) ensureCustomRolesForIntents(ctx context.Context, userAssignedIde
 	return nil
 }
 
-func (a *Agent) ensureCustomRoleForIntent(ctx context.Context, userAssignedIdentity armmsi.Identity, scope string, intent otterizev2alpha1.Target) error {
+func (a *Agent) ensureCustomRoleForIntent(ctx context.Context, userAssignedIdentity armmsi.Identity, scope string, intents otterizev2alpha1.ClientIntents, intent otterizev2alpha1.Target) error {
 	actions := intent.Azure.Actions
 	dataActions := intent.Azure.DataActions
 
-	customRoleName := a.GenerateCustomRoleName(userAssignedIdentity, scope)
+	customRoleName := a.GenerateCustomRoleName(intents, scope)
 	role, found := a.FindCustomRoleByName(ctx, scope, customRoleName)
 	if found {
 		err := a.UpdateCustomRole(ctx, scope, role, actions, dataActions)
@@ -403,7 +419,7 @@ func (a *Agent) ensureCustomRoleForIntent(ctx context.Context, userAssignedIdent
 			return errors.Wrap(err)
 		}
 
-		newRole, err := a.CreateCustomRole(ctx, scope, userAssignedIdentity, actions, dataActions)
+		newRole, err := a.CreateCustomRole(ctx, scope, intents, actions, dataActions)
 		if err != nil {
 			return errors.Wrap(err)
 		}

src/operator/controllers/intents_reconcilers/iam/iampolicyagents/azurepolicyagent/agent_customroles_test.go

@@ -20,6 +20,7 @@ import (
 	"github.com/stretchr/testify/suite"
 	"go.uber.org/mock/gomock"
 	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"sync"
 	"testing"
 )
@@ -266,7 +267,7 @@ func (s *AzureAgentPoliciesCustomRolesSuite) TestAddRolePolicyFromIntents_Custom
 		targetScope := fmt.Sprintf("/subscriptions/%s/resourceGroups/%s/providers/Microsoft.Storage/storageAccounts/test/blobServices/default/containers/container", testSubscriptionID, testResourceGroup)
 
 		s.Run(testCase.Name, func() {
-			intents := []otterizev2alpha1.Target{
+			targets := []otterizev2alpha1.Target{
 				{
 					Azure: &otterizev2alpha1.AzureTarget{
 						Scope:       targetScope,
@@ -277,6 +278,17 @@ func (s *AzureAgentPoliciesCustomRolesSuite) TestAddRolePolicyFromIntents_Custom
 				},
 			}
 
+			clientIntents := otterizev2alpha1.ClientIntents{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      testIntentsServiceName,
+					Namespace: testNamespace,
+				},
+				Spec: &otterizev2alpha1.IntentsSpec{
+					Workload: otterizev2alpha1.Workload{Name: testIntentsServiceName},
+					Targets:  targets,
+				},
+			}
+
 			clientId := uuid.NewString()
 			s.expectGetUserAssignedIdentityReturnsClientID(clientId)
 
@@ -304,7 +316,7 @@ func (s *AzureAgentPoliciesCustomRolesSuite) TestAddRolePolicyFromIntents_Custom
 				s.expectCreateOrUpdateRoleDefinitionWriteRoleDefinition(&customRoleDefinition)
 			}
 
-			err := s.agent.AddRolePolicyFromIntents(context.Background(), testNamespace, testAccountName, testIntentsServiceName, intents, corev1.Pod{})
+			err := s.agent.AddRolePolicyFromIntents(context.Background(), testNamespace, testAccountName, testIntentsServiceName, clientIntents, corev1.Pod{})
 			s.Require().NoError(err)
 
 			if testCase.UpdateExpected {
@@ -319,7 +331,17 @@ func (s *AzureAgentPoliciesCustomRolesSuite) TestAddRolePolicyFromIntents_Custom
 func (s *AzureAgentPoliciesCustomRolesSuite) TestAddRolePolicyFromIntents_IdentityNotFound() {
 	s.expectGetUserAssignedIdentityReturnsNotFoundError()
 
-	err := s.agent.AddRolePolicyFromIntents(context.Background(), testNamespace, testAccountName, testIntentsServiceName, nil, corev1.Pod{})
+	clientIntents := otterizev2alpha1.ClientIntents{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      testIntentsServiceName,
+			Namespace: testNamespace,
+		},
+		Spec: &otterizev2alpha1.IntentsSpec{
+			Workload: otterizev2alpha1.Workload{Name: testIntentsServiceName},
+		},
+	}
+
+	err := s.agent.AddRolePolicyFromIntents(context.Background(), testNamespace, testAccountName, testIntentsServiceName, clientIntents, corev1.Pod{})
 	s.Require().ErrorIs(err, agentutils.ErrCloudIdentityNotFound)
 }
 

src/operator/controllers/intents_reconcilers/iam/iampolicyagents/azurepolicyagent/agent_keyvault_test.go

@@ -243,7 +243,7 @@ func (s *AzureAgentPoliciesKeyVaultSuite) TestAddRolePolicyFromIntents_AzureKeyV
 	for _, testCase := range azureKeyVaultPolicyTestCases {
 		s.Run(testCase.Name, func() {
 			scope := fmt.Sprintf("/subscriptions/%s/resourceGroups/%s/providers/Microsoft.KeyVault/vaults/%s", testSubscriptionID, testResourceGroup, testKeyVaultName)
-			intents := []otterizev2alpha1.Target{
+			targets := []otterizev2alpha1.Target{
 				{
 					Azure: &otterizev2alpha1.AzureTarget{
 						Scope:          scope,
@@ -252,6 +252,17 @@ func (s *AzureAgentPoliciesKeyVaultSuite) TestAddRolePolicyFromIntents_AzureKeyV
 				},
 			}
 
+			clientIntents := otterizev2alpha1.ClientIntents{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      testIntentsServiceName,
+					Namespace: testNamespace,
+				},
+				Spec: &otterizev2alpha1.IntentsSpec{
+					Workload: otterizev2alpha1.Workload{Name: testIntentsServiceName},
+					Targets:  targets,
+				},
+			}
+
 			clientId := uuid.NewString()
 			s.expectGetUserAssignedIdentityReturnsClientID(clientId)
 
@@ -274,7 +285,7 @@ func (s *AzureAgentPoliciesKeyVaultSuite) TestAddRolePolicyFromIntents_AzureKeyV
 			}
 
 			// Act
-			err := s.agent.AddRolePolicyFromIntents(context.Background(), testNamespace, testAccountName, testIntentsServiceName, intents, corev1.Pod{})
+			err := s.agent.AddRolePolicyFromIntents(context.Background(), testNamespace, testAccountName, testIntentsServiceName, clientIntents, corev1.Pod{})
 
 			// Assert
 			s.NoError(err)