Skip to content

Commit

Permalink
Solve a potential race on AWS IAM integration where policy deletion w…
Browse files Browse the repository at this point in the history 
…ould error because the related IAM role was not found (#551)
  • Loading branch information
@otterobert
otterobert authored Jan 26, 2025
1 parent 4764919 commit e0a8e16
Show file tree
Hide file tree
Showing 4 changed files with 101 additions and 103 deletions.
4 changes: 4 additions & 0 deletions src/operator/controllers/intents_reconcilers/iam/iam_reconciler.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
otterizev2alpha1 "github.com/otterize/intents-operator/src/operator/api/v2alpha1"
"github.com/otterize/intents-operator/src/operator/controllers/intents_reconcilers/consts"
"github.com/otterize/intents-operator/src/operator/controllers/intents_reconcilers/iam/iampolicyagents"
"github.com/otterize/intents-operator/src/shared/agentutils"
"github.com/otterize/intents-operator/src/shared/errors"
"github.com/otterize/intents-operator/src/shared/injectablerecorder"
"github.com/otterize/intents-operator/src/shared/serviceidresolver"
Expand Down Expand Up @@ -83,6 +84,9 @@ func (r *IAMIntentsReconciler) Reconcile(ctx context.Context, req reconcile.Requ
}

if err := r.applyTypedIAMIntents(ctx, pod, intents, r.agent); err != nil {
if errors.Is(err, agentutils.ErrRoleNotFound) {
return ctrl.Result{Requeue: true}, nil
}
return ctrl.Result{}, errors.Wrap(err)
}

Expand Down
192 changes: 90 additions & 102 deletions src/operator/controllers/intents_reconcilers/iam/iam_reconciler_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ import (
"github.com/otterize/intents-operator/src/operator/controllers/intents_reconcilers/consts"
mock_iampolicyagents "github.com/otterize/intents-operator/src/operator/controllers/intents_reconcilers/iam/iampolicyagents/mocks"
mocks "github.com/otterize/intents-operator/src/operator/controllers/intents_reconcilers/mocks"
"github.com/otterize/intents-operator/src/shared/agentutils"
"github.com/otterize/intents-operator/src/shared/errors"
"github.com/stretchr/testify/suite"
"go.uber.org/mock/gomock"
corev1 "k8s.io/api/core/v1"
Expand All @@ -21,9 +23,11 @@ import (
)

const (
testNamespace = "test-namespace"

testIntentType = otterizev2alpha1.IntentTypeGCP
testNamespace = "test-namespace"
testServiceName = "test-client"
testClientIntentsName = "client-intents"
testClientServiceAccount = "test-server-sa"
testIntentType = otterizev2alpha1.IntentTypeGCP
)

type IAMIntentsReconcilerTestSuite struct {
Expand All @@ -35,6 +39,36 @@ type IAMIntentsReconcilerTestSuite struct {
scheme *runtime.Scheme
}

func getTestClientPod(serviceName string, clientServiceAccount string) corev1.Pod {
return corev1.Pod{
ObjectMeta: metav1.ObjectMeta{
Name: serviceName,
Namespace: testNamespace,
},
Spec: corev1.PodSpec{
ServiceAccountName: clientServiceAccount,
Containers: []corev1.Container{
{
Name: "real-application-who-does-something",
},
},
},
}
}

func getTestIAMIntents(clientIntentsName string, serviceName string, targets []otterizev2alpha1.Target) otterizev2alpha1.ClientIntents {
return otterizev2alpha1.ClientIntents{
ObjectMeta: metav1.ObjectMeta{
Name: clientIntentsName,
Namespace: testNamespace,
},
Spec: &otterizev2alpha1.IntentsSpec{
Workload: otterizev2alpha1.Workload{Name: serviceName},
Targets: targets,
},
}
}

func (s *IAMIntentsReconcilerTestSuite) SetupTest() {
s.MocksSuiteBase.SetupTest()
s.scheme = runtime.NewScheme()
Expand All @@ -53,47 +87,17 @@ func (s *IAMIntentsReconcilerTestSuite) SetupTest() {
}

func (s *IAMIntentsReconcilerTestSuite) TestCreateIAMIntentNoPodLabelHasNoEffect() {
clientIntentsName := "client-intents"
serviceName := "test-client"

namespacedName := types.NamespacedName{
Namespace: testNamespace,
Name: clientIntentsName,
}
namespacedName := types.NamespacedName{Namespace: testNamespace, Name: testClientIntentsName}
req := ctrl.Request{NamespacedName: namespacedName}

iamIntents := otterizev2alpha1.ClientIntents{
ObjectMeta: metav1.ObjectMeta{
Name: clientIntentsName,
Namespace: testNamespace,
},
Spec: &otterizev2alpha1.IntentsSpec{
Workload: otterizev2alpha1.Workload{Name: serviceName},
Targets: []otterizev2alpha1.Target{
{
GCP: &otterizev2alpha1.GCPTarget{
Resource: "projects/_/buckets/bucket-name",
},
},
},
},
}

clientServiceAccount := "test-server-sa"
clientPod := corev1.Pod{
ObjectMeta: metav1.ObjectMeta{
Name: serviceName,
Namespace: testNamespace,
},
Spec: corev1.PodSpec{
ServiceAccountName: clientServiceAccount,
Containers: []corev1.Container{
{
Name: "real-application-who-does-something",
},
clientPod := getTestClientPod(testServiceName, "test-server-sa")
iamIntents := getTestIAMIntents(testClientIntentsName, testServiceName, []otterizev2alpha1.Target{
{
GCP: &otterizev2alpha1.GCPTarget{
Resource: "projects/_/buckets/bucket-name",
},
},
}
})

s.Client.EXPECT().Get(gomock.Any(), req.NamespacedName, gomock.AssignableToTypeOf(&iamIntents)).DoAndReturn(
func(arg0 context.Context, arg1 types.NamespacedName, arg2 *otterizev2alpha1.ClientIntents, arg3 ...client.GetOption) error {
Expand All @@ -112,13 +116,9 @@ func (s *IAMIntentsReconcilerTestSuite) TestCreateIAMIntentNoPodLabelHasNoEffect
}

func (s *IAMIntentsReconcilerTestSuite) TestCreateIAMIntentCallingTheiamAgent() {
clientIntentsName := "client-intents"
serviceName := "test-client"
clientServiceAccount := "test-server-sa"

namespacedName := types.NamespacedName{
Namespace: testNamespace,
Name: clientIntentsName,
Name: testClientIntentsName,
}
req := ctrl.Request{NamespacedName: namespacedName}

Expand All @@ -140,31 +140,8 @@ func (s *IAMIntentsReconcilerTestSuite) TestCreateIAMIntentCallingTheiamAgent()
}
allIntents = append(allIntents, filteredIntents...)

iamIntents := otterizev2alpha1.ClientIntents{
ObjectMeta: metav1.ObjectMeta{
Name: clientIntentsName,
Namespace: testNamespace,
},
Spec: &otterizev2alpha1.IntentsSpec{
Workload: otterizev2alpha1.Workload{Name: serviceName},
Targets: allIntents,
},
}

clientPod := corev1.Pod{
ObjectMeta: metav1.ObjectMeta{
Name: serviceName,
Namespace: testNamespace,
},
Spec: corev1.PodSpec{
ServiceAccountName: clientServiceAccount,
Containers: []corev1.Container{
{
Name: "real-application-who-does-something",
},
},
},
}
clientPod := getTestClientPod(testServiceName, testClientServiceAccount)
iamIntents := getTestIAMIntents(testClientIntentsName, testServiceName, allIntents)

s.Client.EXPECT().Get(gomock.Any(), req.NamespacedName, gomock.AssignableToTypeOf(&iamIntents)).DoAndReturn(
func(arg0 context.Context, arg1 types.NamespacedName, arg2 *otterizev2alpha1.ClientIntents, arg3 ...client.GetOption) error {
Expand All @@ -181,7 +158,7 @@ func (s *IAMIntentsReconcilerTestSuite) TestCreateIAMIntentCallingTheiamAgent()
gomock.AssignableToTypeOf(&otterizev2alpha1.ClientIntentsList{}),
&client.ListOptions{Namespace: testNamespace},
).Return(nil)
s.iamAgent.EXPECT().AddRolePolicyFromIntents(gomock.Any(), testNamespace, clientServiceAccount, serviceName, filteredIntents, clientPod).Return(nil)
s.iamAgent.EXPECT().AddRolePolicyFromIntents(gomock.Any(), testNamespace, testClientServiceAccount, testServiceName, filteredIntents, clientPod).Return(nil)

res, err := s.Reconciler.Reconcile(context.Background(), req)
s.NoError(err)
Expand All @@ -196,13 +173,9 @@ func (s *IAMIntentsReconcilerTestSuite) TestCreateIAMIntentCallingTheiamAgent()
}

func (s *IAMIntentsReconcilerTestSuite) TestCreateIAMIntentPartialDeleteCallingTheiamAgent() {
clientIntentsName := "client-intents"
serviceName := "test-client"
clientServiceAccount := "test-server-sa"

namespacedName := types.NamespacedName{
Namespace: testNamespace,
Name: clientIntentsName,
Name: testClientIntentsName,
}
req := ctrl.Request{NamespacedName: namespacedName}

Expand All @@ -214,31 +187,8 @@ func (s *IAMIntentsReconcilerTestSuite) TestCreateIAMIntentPartialDeleteCallingT
},
}

clientIntents := otterizev2alpha1.ClientIntents{
ObjectMeta: metav1.ObjectMeta{
Name: clientIntentsName,
Namespace: testNamespace,
},
Spec: &otterizev2alpha1.IntentsSpec{
Workload: otterizev2alpha1.Workload{Name: serviceName},
Targets: awsIntents,
},
}

clientPod := corev1.Pod{
ObjectMeta: metav1.ObjectMeta{
Name: serviceName,
Namespace: testNamespace,
},
Spec: corev1.PodSpec{
ServiceAccountName: clientServiceAccount,
Containers: []corev1.Container{
{
Name: "real-application-who-does-something",
},
},
},
}
clientPod := getTestClientPod(testServiceName, testClientServiceAccount)
clientIntents := getTestIAMIntents(testClientIntentsName, testServiceName, awsIntents)

s.Client.EXPECT().Get(gomock.Any(), req.NamespacedName, gomock.AssignableToTypeOf(&clientIntents)).DoAndReturn(
func(arg0 context.Context, arg1 types.NamespacedName, arg2 *otterizev2alpha1.ClientIntents, arg3 ...client.GetOption) error {
Expand All @@ -255,7 +205,7 @@ func (s *IAMIntentsReconcilerTestSuite) TestCreateIAMIntentPartialDeleteCallingT
gomock.AssignableToTypeOf(&otterizev2alpha1.ClientIntentsList{}),
&client.ListOptions{Namespace: testNamespace},
).Return(nil)
s.iamAgent.EXPECT().AddRolePolicyFromIntents(gomock.Any(), testNamespace, clientServiceAccount, serviceName, []otterizev2alpha1.Target{}, clientPod).Return(nil)
s.iamAgent.EXPECT().AddRolePolicyFromIntents(gomock.Any(), testNamespace, testClientServiceAccount, testServiceName, []otterizev2alpha1.Target{}, clientPod).Return(nil)

res, err := s.Reconciler.Reconcile(context.Background(), req)
s.NoError(err)
Expand All @@ -269,6 +219,44 @@ func (s *IAMIntentsReconcilerTestSuite) TestCreateIAMIntentPartialDeleteCallingT
}
}

func (s *IAMIntentsReconcilerTestSuite) TestRoleNotFoundErrorReQueuesEvent() {
namespacedName := types.NamespacedName{
Namespace: testNamespace,
Name: testClientIntentsName,
}
req := ctrl.Request{NamespacedName: namespacedName}

clientPod := getTestClientPod(testServiceName, testClientServiceAccount)
iamIntents := getTestIAMIntents(testClientIntentsName, testServiceName, []otterizev2alpha1.Target{})

s.Client.EXPECT().Get(gomock.Any(), req.NamespacedName, gomock.AssignableToTypeOf(&iamIntents)).DoAndReturn(
func(arg0 context.Context, arg1 types.NamespacedName, arg2 *otterizev2alpha1.ClientIntents, arg3 ...client.GetOption) error {
iamIntents.DeepCopyInto(arg2)
return nil
},
)

s.serviceResolver.EXPECT().ResolveClientIntentToPod(gomock.Any(), gomock.Eq(iamIntents)).Return(clientPod, nil)
s.iamAgent.EXPECT().AppliesOnPod(gomock.AssignableToTypeOf(&clientPod)).Return(true)
s.iamAgent.EXPECT().IntentType().Return(testIntentType)
s.Client.EXPECT().List(
gomock.Any(),
gomock.AssignableToTypeOf(&otterizev2alpha1.ClientIntentsList{}),
&client.ListOptions{Namespace: testNamespace},
).Return(nil)

// Throw the sentinel error
s.iamAgent.EXPECT().AddRolePolicyFromIntents(gomock.Any(), testNamespace, testClientServiceAccount, testServiceName, []otterizev2alpha1.Target{}, clientPod).Return(
errors.Errorf("%w: %s", agentutils.ErrRoleNotFound, "test error"),
)

res, err := s.Reconciler.Reconcile(context.Background(), req)

// Expect no error to be raised in case of role not found and the event to be re-queued
s.NoError(err)
s.Equal(res.Requeue, true)
}

func TestIAMIntentsReconcilerTestSuite(t *testing.T) {
suite.Run(t, new(IAMIntentsReconcilerTestSuite))
}
3 changes: 3 additions & 0 deletions src/shared/agentutils/agentutils.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package agentutils
import (
"crypto/sha256"
"fmt"
"github.com/otterize/intents-operator/src/shared/errors"
"github.com/samber/lo"
"golang.org/x/exp/slices"
)
Expand All @@ -11,6 +12,8 @@ const (
truncatedHashLength = 6
)

var ErrRoleNotFound = errors.NewSentinelError("role not found")

// TruncateHashName truncates the given name to the given max length and appends a hash to it.
func TruncateHashName(fullName string, maxLen int) string {
maxTruncatedLen := maxLen - truncatedHashLength - 1 // add another char for the hyphen
Expand Down
5 changes: 4 additions & 1 deletion src/shared/awsagent/policies.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"github.com/aws/aws-sdk-go-v2/aws"
"github.com/aws/aws-sdk-go-v2/service/iam"
"github.com/aws/aws-sdk-go-v2/service/iam/types"
"github.com/otterize/intents-operator/src/shared/agentutils"
"github.com/otterize/intents-operator/src/shared/errors"
"github.com/samber/lo"
"github.com/sirupsen/logrus"
Expand All @@ -22,7 +23,9 @@ func (a *Agent) AddRolePolicy(ctx context.Context, namespace string, accountName
}

if !exists {
return errors.Errorf("role not found: %s", a.generateRoleName(namespace, accountName))
// Allow sentinel comparison + dynamic error message
roleName := a.generateRoleName(namespace, accountName)
return errors.Errorf("%w: %s", agentutils.ErrRoleNotFound, roleName)
}

softDeletionStrategyEnabled := HasSoftDeleteStrategyTagSet(role.Tags)
Expand Down

0 comments on commit e0a8e16

Please sign in to comment.