Skip to content

Commit

Permalink
Fix Azure API version error when applying Azure ClientIntents on KeyV…
Browse files Browse the repository at this point in the history 
…ault scopes, by querying for supported API versions by resource type (#546)
  • Loading branch information
@amitlicht
amitlicht authored Jan 19, 2025
1 parent c30c1b1 commit da510b8
Show file tree
Hide file tree
Showing 15 changed files with 390 additions and 255 deletions.
2 changes: 1 addition & 1 deletion src/go.mod
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions src/go.sum
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

11 changes: 6 additions & 5 deletions src/operator/controllers/intents_reconcilers/iam/iampolicyagents/azurepolicyagent/agent.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,11 +43,6 @@ func (a *Agent) getIntentScope(ctx context.Context, intent otterizev2alpha1.Targ

// If the scope is already a full scope, validate and return it
if strings.HasPrefix(name, "/subscriptions/") {
err := a.ValidateScope(ctx, name)
if err != nil {
return "", errors.Wrap(err)
}

return name, nil
}

Expand Down Expand Up @@ -390,6 +385,12 @@ func (a *Agent) ensureCustomRoleForIntent(ctx context.Context, userAssignedIdent
return errors.Wrap(err)
}
} else {
if err := a.ValidateScope(ctx, scope); err != nil {
// Prevent using non-existing scopes for custom roles,
// as they may cause issues when deleting the custom role
return errors.Wrap(err)
}

newRole, err := a.CreateCustomRole(ctx, scope, userAssignedIdentity, actions, dataActions)
if err != nil {
return errors.Wrap(err)
Expand Down
22 changes: 17 additions & 5 deletions ...ollers/intents_reconcilers/iam/iampolicyagents/azurepolicyagent/agent_customroles_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ type AzureAgentPoliciesCustomRolesSuite struct {
suite.Suite

mockResourcesClient *mock_azureagent.MockAzureARMResourcesClient
mockProviderResourceTypesClient *mock_azureagent.MockAzureARMResourcesProviderResourceTypesClient
mockSubscriptionsClient *mock_azureagent.MockAzureARMSubscriptionsClient
mockResourceGroupsClient *mock_azureagent.MockAzureARMResourcesResourceGroupsClient
mockManagedClustersClient *mock_azureagent.MockAzureARMContainerServiceManagedClustersClient
Expand All @@ -51,6 +52,16 @@ type AzureAgentPoliciesCustomRolesSuite struct {
}

func (s *AzureAgentPoliciesCustomRolesSuite) expectGetByIDReturnsResource(scope string) {
s.mockProviderResourceTypesClient.EXPECT().List(gomock.Any(), gomock.Any(), gomock.Any()).Return(armresources.ProviderResourceTypesClientListResponse{
ProviderResourceTypeListResult: armresources.ProviderResourceTypeListResult{
Value: []*armresources.ProviderResourceType{
{
ResourceType: to.Ptr("storageAccounts"),
DefaultAPIVersion: lo.ToPtr("2022-09-01"),
},
},
},
}, nil)
s.mockResourcesClient.EXPECT().GetByID(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(armresources.ClientGetByIDResponse{
GenericResource: armresources.GenericResource{
ID: &scope,
Expand Down Expand Up @@ -128,6 +139,7 @@ func (s *AzureAgentPoliciesCustomRolesSuite) SetupTest() {
controller := gomock.NewController(s.T())

s.mockResourcesClient = mock_azureagent.NewMockAzureARMResourcesClient(controller)
s.mockProviderResourceTypesClient = mock_azureagent.NewMockAzureARMResourcesProviderResourceTypesClient(controller)
s.mockSubscriptionsClient = mock_azureagent.NewMockAzureARMSubscriptionsClient(controller)
s.mockResourceGroupsClient = mock_azureagent.NewMockAzureARMResourcesResourceGroupsClient(controller)
s.mockManagedClustersClient = mock_azureagent.NewMockAzureARMContainerServiceManagedClustersClient(controller)
Expand Down Expand Up @@ -155,6 +167,7 @@ func (s *AzureAgentPoliciesCustomRolesSuite) SetupTest() {
},
nil,
s.mockResourcesClient,
s.mockProviderResourceTypesClient,
s.mockSubscriptionsClient,
s.mockResourceGroupsClient,
s.mockManagedClustersClient,
Expand Down Expand Up @@ -216,9 +229,6 @@ var azureCustomRoleTestCases = []AzureCustomRoleTestCase{
Roles: []string{
"Storage Blob Data Reader",
},
Actions: []otterizev2alpha1.AzureAction{
"Microsoft.Storage/storageAccounts/blobServices/containers/read",
},
ExisingRoles: []*armauthorization.RoleDefinition{
{
ID: to.Ptr("Storage Blob Data Reader"),
Expand Down Expand Up @@ -273,7 +283,9 @@ func (s *AzureAgentPoliciesCustomRolesSuite) TestAddRolePolicyFromIntents_Custom
s.expectCreateRoleAssignmentReturnsEmpty()
}

s.expectGetByIDReturnsResource(targetScope)
if testCase.ExisingRoles == nil {
s.expectGetByIDReturnsResource(targetScope)
}

// Make sure the custom role is created
var customRoleDefinition armauthorization.RoleDefinition
Expand All @@ -282,7 +294,7 @@ func (s *AzureAgentPoliciesCustomRolesSuite) TestAddRolePolicyFromIntents_Custom
}

err := s.agent.AddRolePolicyFromIntents(context.Background(), testNamespace, testAccountName, testIntentsServiceName, intents, corev1.Pod{})
s.NoError(err)
s.Require().NoError(err)

if testCase.UpdateExpected {
s.Require().Len(customRoleDefinition.Properties.Permissions, 1)
Expand Down
3 changes: 3 additions & 0 deletions ...rollers/intents_reconcilers/iam/iampolicyagents/azurepolicyagent/agent_identities_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ type AzureAgentIdentitiesSuite struct {
suite.Suite

mockResourcesClient *mock_azureagent.MockAzureARMResourcesClient
mockProviderResourceTypesClient *mock_azureagent.MockAzureARMResourcesProviderResourceTypesClient
mockSubscriptionsClient *mock_azureagent.MockAzureARMSubscriptionsClient
mockResourceGroupsClient *mock_azureagent.MockAzureARMResourcesResourceGroupsClient
mockManagedClustersClient *mock_azureagent.MockAzureARMContainerServiceManagedClustersClient
Expand All @@ -39,6 +40,7 @@ func (s *AzureAgentIdentitiesSuite) SetupTest() {
controller := gomock.NewController(s.T())

s.mockResourcesClient = mock_azureagent.NewMockAzureARMResourcesClient(controller)
s.mockProviderResourceTypesClient = mock_azureagent.NewMockAzureARMResourcesProviderResourceTypesClient(controller)
s.mockSubscriptionsClient = mock_azureagent.NewMockAzureARMSubscriptionsClient(controller)
s.mockResourceGroupsClient = mock_azureagent.NewMockAzureARMResourcesResourceGroupsClient(controller)
s.mockManagedClustersClient = mock_azureagent.NewMockAzureARMContainerServiceManagedClustersClient(controller)
Expand Down Expand Up @@ -66,6 +68,7 @@ func (s *AzureAgentIdentitiesSuite) SetupTest() {
},
nil,
s.mockResourcesClient,
s.mockProviderResourceTypesClient,
s.mockSubscriptionsClient,
s.mockResourceGroupsClient,
s.mockManagedClustersClient,
Expand Down
17 changes: 3 additions & 14 deletions ...ntrollers/intents_reconcilers/iam/iampolicyagents/azurepolicyagent/agent_keyvault_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import (
"github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/authorization/armauthorization/v2"
"github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/keyvault/armkeyvault"
"github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/msi/armmsi"
"github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armresources"
"github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armsubscriptions"
"github.com/google/uuid"
otterizev2alpha1 "github.com/otterize/intents-operator/src/operator/api/v2alpha1"
Expand All @@ -25,6 +24,7 @@ type AzureAgentPoliciesKeyVaultSuite struct {
suite.Suite

mockResourcesClient *mock_azureagent.MockAzureARMResourcesClient
mockProviderResourceTypesClient *mock_azureagent.MockAzureARMResourcesProviderResourceTypesClient
mockSubscriptionsClient *mock_azureagent.MockAzureARMSubscriptionsClient
mockResourceGroupsClient *mock_azureagent.MockAzureARMResourcesResourceGroupsClient
mockManagedClustersClient *mock_azureagent.MockAzureARMContainerServiceManagedClustersClient
Expand All @@ -44,6 +44,7 @@ func (s *AzureAgentPoliciesKeyVaultSuite) SetupTest() {
controller := gomock.NewController(s.T())

s.mockResourcesClient = mock_azureagent.NewMockAzureARMResourcesClient(controller)
s.mockProviderResourceTypesClient = mock_azureagent.NewMockAzureARMResourcesProviderResourceTypesClient(controller)
s.mockSubscriptionsClient = mock_azureagent.NewMockAzureARMSubscriptionsClient(controller)
s.mockResourceGroupsClient = mock_azureagent.NewMockAzureARMResourcesResourceGroupsClient(controller)
s.mockManagedClustersClient = mock_azureagent.NewMockAzureARMContainerServiceManagedClustersClient(controller)
Expand Down Expand Up @@ -71,6 +72,7 @@ func (s *AzureAgentPoliciesKeyVaultSuite) SetupTest() {
},
nil,
s.mockResourcesClient,
s.mockProviderResourceTypesClient,
s.mockSubscriptionsClient,
s.mockResourceGroupsClient,
s.mockManagedClustersClient,
Expand All @@ -87,14 +89,6 @@ func (s *AzureAgentPoliciesKeyVaultSuite) SetupTest() {
}
}

func (s *AzureAgentPoliciesKeyVaultSuite) expectGetByIDReturnsResource(scope string) {
s.mockResourcesClient.EXPECT().GetByID(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(armresources.ClientGetByIDResponse{
GenericResource: armresources.GenericResource{
ID: &scope,
},
}, nil)
}

func (s *AzureAgentPoliciesKeyVaultSuite) expectGetUserAssignedIdentityReturnsClientID(clientId string) {
userAssignedIndentityName := s.agent.GenerateUserAssignedIdentityName(testNamespace, testIntentsServiceName)
s.mockUserAssignedIdentitiesClient.EXPECT().Get(gomock.Any(), testResourceGroup, userAssignedIndentityName, nil).Return(
Expand Down Expand Up @@ -279,11 +273,6 @@ func (s *AzureAgentPoliciesKeyVaultSuite) TestAddRolePolicyFromIntents_AzureKeyV
s.expectUpdateKeyVaultAccessPolicyWritesPolicy(testKeyVaultName, testCase.UpdateKind, &updatedPolicy)
}

// The scope should be validated when passing valid intent:
if intents[0].Azure.KeyVaultPolicy != nil {
s.expectGetByIDReturnsResource(scope)
}

// Act
err := s.agent.AddRolePolicyFromIntents(context.Background(), testNamespace, testAccountName, testIntentsServiceName, intents, corev1.Pod{})

Expand Down
40 changes: 3 additions & 37 deletions .../controllers/intents_reconcilers/iam/iampolicyagents/azurepolicyagent/agent_scope_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import (
otterizev2alpha1 "github.com/otterize/intents-operator/src/operator/api/v2alpha1"
"github.com/otterize/intents-operator/src/shared/azureagent"
mock_azureagent "github.com/otterize/intents-operator/src/shared/azureagent/mocks"
"github.com/otterize/intents-operator/src/shared/errors"
"github.com/samber/lo"
"github.com/stretchr/testify/suite"
"go.uber.org/mock/gomock"
Expand All @@ -20,6 +19,7 @@ type AzureAgentScopeSuite struct {
suite.Suite

mockResourcesClient *mock_azureagent.MockAzureARMResourcesClient
mockProviderResourceTypesClient *mock_azureagent.MockAzureARMResourcesProviderResourceTypesClient
mockSubscriptionsClient *mock_azureagent.MockAzureARMSubscriptionsClient
mockResourceGroupsClient *mock_azureagent.MockAzureARMResourcesResourceGroupsClient
mockManagedClustersClient *mock_azureagent.MockAzureARMContainerServiceManagedClustersClient
Expand All @@ -39,6 +39,7 @@ func (s *AzureAgentScopeSuite) SetupTest() {
controller := gomock.NewController(s.T())

s.mockResourcesClient = mock_azureagent.NewMockAzureARMResourcesClient(controller)
s.mockProviderResourceTypesClient = mock_azureagent.NewMockAzureARMResourcesProviderResourceTypesClient(controller)
s.mockSubscriptionsClient = mock_azureagent.NewMockAzureARMSubscriptionsClient(controller)
s.mockResourceGroupsClient = mock_azureagent.NewMockAzureARMResourcesResourceGroupsClient(controller)
s.mockManagedClustersClient = mock_azureagent.NewMockAzureARMContainerServiceManagedClustersClient(controller)
Expand Down Expand Up @@ -66,6 +67,7 @@ func (s *AzureAgentScopeSuite) SetupTest() {
},
nil,
s.mockResourcesClient,
s.mockProviderResourceTypesClient,
s.mockSubscriptionsClient,
s.mockResourceGroupsClient,
s.mockManagedClustersClient,
Expand All @@ -82,21 +84,6 @@ func (s *AzureAgentScopeSuite) SetupTest() {
}
}

func (s *AzureAgentScopeSuite) expectGetByIDReturnsResource(scope string) {
s.mockResourcesClient.EXPECT().GetByID(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(armresources.ClientGetByIDResponse{
GenericResource: armresources.GenericResource{
ID: &scope,
},
}, nil)
}

func (s *AzureAgentScopeSuite) expectGetByIDReturnsError() {
s.mockResourcesClient.EXPECT().GetByID(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(
armresources.ClientGetByIDResponse{},
errors.Errorf("resource not found"),
)
}

func (s *AzureAgentScopeSuite) expectListSubscriptionsReturnsPager() {
s.mockSubscriptionsClient.EXPECT().NewListPager(nil).Return(azureagent.NewListPager[armsubscriptions.ClientListResponse](
armsubscriptions.ClientListResponse{
Expand Down Expand Up @@ -134,8 +121,6 @@ func (s *AzureAgentScopeSuite) TestGetIntentScopeFullScope() {
},
}

s.expectGetByIDReturnsResource(targetScope)

// Act
scope, err := s.agent.getIntentScope(context.Background(), intent)

Expand Down Expand Up @@ -183,25 +168,6 @@ func (s *AzureAgentScopeSuite) TestGetIntentScopeInvalidScope() {
s.Empty(scope)
}

func (s *AzureAgentScopeSuite) TestGetIntentScopeNotFoundFullScope() {
// Arrange
targetScope := fmt.Sprintf("/subscriptions/%s/resourceGroups/%s/providers/Microsoft.Storage/storageAccounts/test/blobServices/default/containers/container", testSubscriptionID, testResourceGroup)
intent := otterizev2alpha1.Target{
Azure: &otterizev2alpha1.AzureTarget{
Scope: targetScope,
},
}

s.expectGetByIDReturnsError()

// Act
scope, err := s.agent.getIntentScope(context.Background(), intent)

// Assert
s.Require().Error(err)
s.Empty(scope)
}

func (s *AzureAgentScopeSuite) TestGetIntentScopeNotFoundPartialScope() {
// Arrange
targetScope := "/providers/Microsoft.Storage/storageAccounts/test/blobServices/default/containers/container"
Expand Down
11 changes: 11 additions & 0 deletions src/shared/azureagent/agent.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,11 @@ import (
"github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/msi/armmsi"
"github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armresources"
"github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armsubscriptions"
"github.com/hashicorp/golang-lru/v2/expirable"
"github.com/otterize/intents-operator/src/shared/errors"
"github.com/sirupsen/logrus"
corev1 "k8s.io/api/core/v1"
"time"
)

const (
Expand All @@ -32,6 +34,7 @@ type Agent struct {
Conf Config
credentials *azidentity.DefaultAzureCredential
resourceClient AzureARMResourcesClient
providerResourceTypesClient AzureARMResourcesProviderResourceTypesClient
subscriptionClient AzureARMSubscriptionsClient
resourceGroupsClient AzureARMResourcesResourceGroupsClient
managedClustersClient AzureARMContainerServiceManagedClustersClient
Expand All @@ -42,6 +45,8 @@ type Agent struct {
vaultsClient AzureARMKeyVaultVaultsClient
subscriptionToResourceClient map[string]AzureARMResourcesClient
subscriptionToRoleAssignmentsClient map[string]AzureARMAuthorizationRoleAssignmentsClient

providerResourceTypesCache *expirable.LRU[string, map[string]armresources.ProviderResourceType]
}

func NewAzureAgent(ctx context.Context, conf Config) (*Agent, error) {
Expand Down Expand Up @@ -88,6 +93,7 @@ func NewAzureAgent(ctx context.Context, conf Config) (*Agent, error) {
}

resourceClient := armResourcesClientFactory.NewClient()
providerResourceTypesClient := armResourcesClientFactory.NewProviderResourceTypesClient()
subscriptionClient := armsubscriptionsClientFactory.NewClient()
userAssignedIdentitiesClient := armmsiClientFactory.NewUserAssignedIdentitiesClient()
federatedIdentityCredentialsClient := armmsiClientFactory.NewFederatedIdentityCredentialsClient()
Expand All @@ -107,6 +113,7 @@ func NewAzureAgent(ctx context.Context, conf Config) (*Agent, error) {
conf,
credentials,
resourceClient,
providerResourceTypesClient,
subscriptionClient,
resourceGroupsClient,
managedClustersClient,
Expand All @@ -130,6 +137,7 @@ func NewAzureAgentFromClients(
conf Config,
credentials *azidentity.DefaultAzureCredential,
resourceClient AzureARMResourcesClient,
providerResourceTypesClient AzureARMResourcesProviderResourceTypesClient,
subscriptionClient AzureARMSubscriptionsClient,
resourceGroupsClient AzureARMResourcesResourceGroupsClient,
managedClustersClient AzureARMContainerServiceManagedClustersClient,
Expand All @@ -145,6 +153,7 @@ func NewAzureAgentFromClients(
Conf: conf,
credentials: credentials,
resourceClient: resourceClient,
providerResourceTypesClient: providerResourceTypesClient,
subscriptionClient: subscriptionClient,
resourceGroupsClient: resourceGroupsClient,
managedClustersClient: managedClustersClient,
Expand All @@ -155,6 +164,8 @@ func NewAzureAgentFromClients(
vaultsClient: vaultsClient,
subscriptionToResourceClient: subscriptionToResourceClient,
subscriptionToRoleAssignmentsClient: subscriptionToRoleAssignmentsClient,

providerResourceTypesCache: expirable.NewLRU[string, map[string]armresources.ProviderResourceType](100, nil, time.Hour),
}
}

Expand Down
4 changes: 4 additions & 0 deletions src/shared/azureagent/azureclients.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,10 @@ type AzureARMResourcesClient interface {
NewListPager(options *armresources.ClientListOptions) *runtime.Pager[armresources.ClientListResponse]
}

type AzureARMResourcesProviderResourceTypesClient interface {
List(ctx context.Context, resourceProviderNamespace string, options *armresources.ProviderResourceTypesClientListOptions) (armresources.ProviderResourceTypesClientListResponse, error)
}

type AzureARMSubscriptionsClient interface {
Get(ctx context.Context, subscriptionID string, options *armsubscriptions.ClientGetOptions) (armsubscriptions.ClientGetResponse, error)
NewListPager(options *armsubscriptions.ClientListOptions) *runtime.Pager[armsubscriptions.ClientListResponse]
Expand Down
Loading

0 comments on commit da510b8

Please sign in to comment.