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, filteredIntents, 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, []otterizev2alpha1.Target{}, 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, iamIntents.GetTargetList(), 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

@@ -12,30 +12,28 @@ import (
 	"github.com/otterize/intents-operator/src/shared/agentutils"
 	"github.com/otterize/intents-operator/src/shared/azureagent"
 	"github.com/otterize/intents-operator/src/shared/errors"
+	"github.com/otterize/intents-operator/src/shared/injectablerecorder"
 	"github.com/samber/lo"
 	corev1 "k8s.io/api/core/v1"
-	"k8s.io/apimachinery/pkg/util/wait"
 	"regexp"
 	"strings"
 	"sync"
-	"time"
 )
 
+const ReasonRoleNotFound = "RoleNotFound"
+
 var KeyVaultNameRegex = regexp.MustCompile(`^/subscriptions/[^/]+/resourceGroups/[^/]+/providers/Microsoft.KeyVault/vaults/([^/]+)$`)
 var StorageAccountRegex = regexp.MustCompile(`providers/Microsoft.Storage/storageAccounts/([^/]+)`)
 
 type Agent struct {
 	*azureagent.Agent
 	roleMutex       sync.Mutex
 	assignmentMutex sync.Mutex
+	injectablerecorder.InjectableRecorder
 }
 
 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)
-
+	agent := &Agent{Agent: azureAgent, roleMutex: sync.Mutex{}, assignmentMutex: sync.Mutex{}}
 	return agent
 }
 
@@ -75,7 +73,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 +82,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 +116,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)
@@ -142,7 +132,7 @@ func (a *Agent) ensureRoleAssignmentsForIntents(ctx context.Context, userAssigne
 		roleNames := intent.Azure.Roles
 		existingRoleAssignmentsForScope := existingRoleAssignmentsByScope[scope]
 
-		if err := a.ensureRoleAssignmentsForIntent(ctx, scope, roleNames, userAssignedIdentity, existingRoleAssignmentsForScope); err != nil {
+		if err := a.ensureRoleAssignmentsForIntent(ctx, scope, roleNames, userAssignedIdentity, existingRoleAssignmentsForScope, clientIntents); err != nil {
 			return errors.Wrap(err)
 		}
 	}
@@ -154,18 +144,29 @@ func (a *Agent) ensureRoleAssignmentsForIntents(ctx context.Context, userAssigne
 	return nil
 }
 
-func (a *Agent) ensureRoleAssignmentsForIntent(ctx context.Context, scope string, roleNames []string, userAssignedIdentity armmsi.Identity, existingRoleAssignmentsForScope []armauthorization.RoleAssignment) error {
+func (a *Agent) ensureRoleAssignmentsForIntent(
+	ctx context.Context,
+	scope string,
+	roleNames []string,
+	userAssignedIdentity armmsi.Identity,
+	existingRoleAssignmentsForScope []armauthorization.RoleAssignment,
+	intents otterizev2alpha1.ClientIntents,
+) error {
 	roleDefinitionsByName, err := a.FindRoleDefinitionByName(ctx, scope, roleNames)
 	if err != nil {
 		return errors.Wrap(err)
 	}
-
 	existingRoleDefinitionIDs := goset.FromSlice(lo.Map(existingRoleAssignmentsForScope, func(roleAssignment armauthorization.RoleAssignment, _ int) string {
 		return *roleAssignment.Properties.RoleDefinitionID
 	}))
 
 	for _, roleName := range roleNames {
-		roleDefinition := roleDefinitionsByName[roleName]
+		roleDefinition, exists := roleDefinitionsByName[roleName]
+		if !exists {
+			// The role mentioned in the intents does not exist in the subscription, record event and move on
+			a.RecordWarningEventf(&intents, ReasonRoleNotFound, "Role %s not found in scope %s", roleName, scope)
+			continue
+		}
 		roleDefinitionID := *roleDefinition.ID
 		if !existingRoleDefinitionIDs.Contains(roleDefinitionID) {
 			if err := a.CreateRoleAssignment(ctx, scope, userAssignedIdentity, roleDefinition, nil); err != nil {
@@ -233,6 +234,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 +275,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 +372,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 +382,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 +415,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 +433,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)
 		}