Skip to content

Commit

Permalink
Split subnet check and storage account reconciliation steps
Browse files Browse the repository at this point in the history
  • Loading branch information
@tsatam
tsatam committed Nov 8, 2023
1 parent 3aada81 commit 84ee524
Show file tree
Hide file tree
Showing 4 changed files with 268 additions and 174 deletions.
98 changes: 75 additions & 23 deletions pkg/operator/controllers/storageaccounts/storageaccount_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"context"
"fmt"

"github.com/Azure/go-autorest/autorest"
"github.com/Azure/go-autorest/autorest/azure"
imageregistryv1 "github.com/openshift/api/imageregistry/v1"
machinev1beta1 "github.com/openshift/api/machine/v1beta1"
Expand Down Expand Up @@ -66,60 +67,111 @@ func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl.
}

r.log.Debug("running")
clusterResource, err := azure.ParseResourceID(instance.Spec.ResourceID)

location := instance.Spec.Location
resource, err := azure.ParseResourceID(instance.Spec.ResourceID)
if err != nil {
return reconcile.Result{}, err
}
resourceGroup := stringutils.LastTokenByte(instance.Spec.ClusterResourceGroupID, '/')
subscriptionId := resource.SubscriptionID
managedResourceGroupName := stringutils.LastTokenByte(instance.Spec.ClusterResourceGroupID, '/')

clusterSubnets, err := subnet.NewKubeManager(r.client, clusterResource.SubscriptionID).List(ctx)
azEnv, authorizer, err := r.getAzureAuthorizer(ctx, instance)
if err != nil {
return reconcile.Result{}, err
}

rc := &imageregistryv1.Config{}
err = r.client.Get(ctx, types.NamespacedName{Name: "cluster"}, rc)
manager := r.newManager(
r.log,
location, subscriptionId, managedResourceGroupName,
azEnv, authorizer,
)

subnets, err := r.getSubnetsToReconcile(ctx, instance, subscriptionId, manager)
if err != nil {
return reconcile.Result{}, err
}

if rc.Spec.Storage.Azure == nil {
return reconcile.Result{}, fmt.Errorf("azure storage field is nil in image registry config")
storageAccounts, err := r.getStorageAccounts(ctx, instance)
if err != nil {
return reconcile.Result{}, err
}

storageAccounts := []string{
"cluster" + instance.Spec.StorageSuffix, // this is our creation, so name is deterministic
rc.Spec.Storage.Azure.AccountName,
err = manager.reconcileAccounts(ctx, subnets, storageAccounts)
if err != nil {
return reconcile.Result{}, err
}

return reconcile.Result{}, nil
}

func (r *Reconciler) getAzureAuthorizer(ctx context.Context, instance *arov1alpha1.Cluster) (azureclient.AROEnvironment, autorest.Authorizer, error) {
// Get endpoints from operator
azEnv, err := azureclient.EnvironmentFromName(instance.Spec.AZEnvironment)
if err != nil {
return reconcile.Result{}, err
return azureclient.AROEnvironment{}, nil, err
}

// create refreshable authorizer from token
azRefreshAuthorizer, err := clusterauthorizer.NewAzRefreshableAuthorizer(r.log, &azEnv, r.client)
if err != nil {
return reconcile.Result{}, err
return azureclient.AROEnvironment{}, nil, err
}

authorizer, err := azRefreshAuthorizer.NewRefreshableAuthorizerToken(ctx)
if err != nil {
return reconcile.Result{}, err
return azureclient.AROEnvironment{}, nil, err
}

manager := r.newManager(
r.log,
instance.Spec.Location, clusterResource.SubscriptionID, resourceGroup,
azEnv,
authorizer,
instance.Spec.ServiceSubnets,
clusterSubnets,
storageAccounts,
)
return azEnv, authorizer, nil
}

func (r *Reconciler) getSubnetsToReconcile(ctx context.Context, instance *arov1alpha1.Cluster, subscriptionId string, m manager) ([]string, error) {
subnets := []string{}
subnets = append(subnets, instance.Spec.ServiceSubnets...)

return reconcile.Result{}, manager.reconcileAccounts(ctx)
clusterSubnets, err := r.getClusterSubnets(ctx, subscriptionId)
if err != nil {
return nil, err
}
clusterSubnetsToReconcile, err := m.checkClusterSubnetsToReconcile(ctx, clusterSubnets)
if err != nil {
return nil, err
}
subnets = append(subnets, clusterSubnetsToReconcile...)

return subnets, nil
}

func (r *Reconciler) getClusterSubnets(ctx context.Context, subscriptionId string) ([]string, error) {
kubeManager := subnet.NewKubeManager(r.client, subscriptionId)

subnets := []string{}

clusterSubnets, err := kubeManager.List(ctx)
if err != nil {
return nil, err
}
for _, subnet := range clusterSubnets {
subnets = append(subnets, subnet.ResourceID)
}
return subnets, nil
}

func (r *Reconciler) getStorageAccounts(ctx context.Context, instance *arov1alpha1.Cluster) ([]string, error) {
rc := &imageregistryv1.Config{}
err := r.client.Get(ctx, types.NamespacedName{Name: "cluster"}, rc)
if err != nil {
return nil, err
}
if rc.Spec.Storage.Azure == nil {
return nil, fmt.Errorf("azure storage field is nil in image registry config")
}

return []string{
"cluster" + instance.Spec.StorageSuffix, // this is our creation, so name is deterministic
rc.Spec.Storage.Azure.AccountName,
}, nil
}

// SetupWithManager creates the controller
Expand Down
73 changes: 37 additions & 36 deletions pkg/operator/controllers/storageaccounts/storageaccount_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,7 @@ import (
arov1alpha1 "github.com/Azure/ARO-RP/pkg/operator/apis/aro.openshift.io/v1alpha1"
"github.com/Azure/ARO-RP/pkg/util/azureclient"
"github.com/Azure/ARO-RP/pkg/util/clusterauthorizer"
"github.com/Azure/ARO-RP/pkg/util/cmp"
_ "github.com/Azure/ARO-RP/pkg/util/scheme"
utilsubnet "github.com/Azure/ARO-RP/pkg/util/subnet"
utilerror "github.com/Azure/ARO-RP/test/util/error"
)

Expand All @@ -50,15 +48,15 @@ var (
clusterStorageAccountName = "cluster" + storageSuffix
registryStorageAccountName = "image-registry-account"

resourceIdMaster = "/subscriptions/" + subscriptionId + "/resourceGroups/" + vnetResourceGroup + "/providers/Microsoft.Network/virtualNetworks/" + vnetName + "/subnets/" + subnetNameMaster
resourceIdWorker = "/subscriptions/" + subscriptionId + "/resourceGroups/" + vnetResourceGroup + "/providers/Microsoft.Network/virtualNetworks/" + vnetName + "/subnets/" + subnetNameWorker
masterSubnetId = "/subscriptions/" + subscriptionId + "/resourceGroups/" + vnetResourceGroup + "/providers/Microsoft.Network/virtualNetworks/" + vnetName + "/subnets/" + subnetNameMaster
workerSubnetId = "/subscriptions/" + subscriptionId + "/resourceGroups/" + vnetResourceGroup + "/providers/Microsoft.Network/virtualNetworks/" + vnetName + "/subnets/" + subnetNameWorker
rpPeSubnetId = "/subscriptions/" + rpSubscriptionId + "/resourceGroups/" + rpResourceGroup + "/providers/Microsoft.Network/virtualNetworks/rp-pe-vnet-001/subnets/rp-pe-subnet"
rpSubnetId = "/subscriptions/" + rpSubscriptionId + "/resourceGroups/" + rpResourceGroup + "/providers/Microsoft.Network/virtualNetworks/rp-vnet/subnets/rp-subnet"
gwySubnetId = "/subscriptions/" + rpSubscriptionId + "/resourceGroups/" + gwyResourceGroup + "/providers/Microsoft.Network/virtualNetworks/gateway-vnet/subnets/gateway-subnet"

serviceSubnets = []string{
"/subscriptions/" + rpSubscriptionId + "/resourceGroups/" + rpResourceGroup + "/providers/Microsoft.Network/virtualNetworks/rp-pe-vnet-001/subnets/rp-pe-subnet",
"/subscriptions/" + rpSubscriptionId + "/resourceGroups/" + rpResourceGroup + "/providers/Microsoft.Network/virtualNetworks/rp-vnet/subnets/rp-subnet",
"/subscriptions/" + rpSubscriptionId + "/resourceGroups/" + gwyResourceGroup + "/providers/Microsoft.Network/virtualNetworks/gateway-vnet/subnets/gateway-subnet",
}
clusterSubnets = []utilsubnet.Subnet{{ResourceID: resourceIdMaster, IsMaster: true}, {ResourceID: resourceIdWorker, IsMaster: false}}
serviceSubnets = []string{rpPeSubnetId, rpSubnetId, gwySubnetId}

cmpoptsSortStringSlices = cmpopts.SortSlices(func(a, b string) bool { return a < b })
)

func getValidClusterInstance(operatorFlag bool) *arov1alpha1.Cluster {
Expand Down Expand Up @@ -132,30 +130,37 @@ func getValidMachines() *machinev1beta1.MachineList {
}

type fakeManager struct {
fakeReconcileAccounts func(ctx context.Context) error
fakeCheckClusterSubnetsToReconcile func(ctx context.Context, clusterSubnets []string) ([]string, error)
fakeReconcileAccounts func(ctx context.Context, subnets, storageAccounts []string) error
}

func (f *fakeManager) checkClusterSubnetsToReconcile(ctx context.Context, clusterSubnets []string) ([]string, error) {
if f.fakeCheckClusterSubnetsToReconcile == nil {
return clusterSubnets, nil
}
return f.fakeCheckClusterSubnetsToReconcile(ctx, clusterSubnets)
}

func (f *fakeManager) reconcileAccounts(ctx context.Context) error {
func (f *fakeManager) reconcileAccounts(ctx context.Context, subnets, storageAccounts []string) error {
if f.fakeReconcileAccounts == nil {
return nil
}
return f.fakeReconcileAccounts(ctx)
return f.fakeReconcileAccounts(ctx, subnets, storageAccounts)
}

func TestReconcile(t *testing.T) {
log := logrus.NewEntry(logrus.StandardLogger())
cmpoptsSortStringSlices := cmpopts.SortSlices(func(a, b string) bool { return a < b })
cmpoptsSortSubnetSlices := cmpopts.SortSlices(func(a, b utilsubnet.Subnet) bool { return a.ResourceID < b.ResourceID })

errUnknown := fmt.Errorf("unknown err")

for _, tt := range []struct {
name string
instance func(*arov1alpha1.Cluster)
operatorFlag bool
fakeReconcileAccounts func(ctx context.Context) error
wantRequeueAfter time.Duration
wantErr string
name string
instance func(*arov1alpha1.Cluster)
operatorFlag bool
fakeCheckClusterSubnetsToReconcile func(ctx context.Context, clusterSubnets []string) ([]string, error)
fakeReconcileAccounts func(ctx context.Context, subnets, storageAccounts []string) error
wantRequeueAfter time.Duration
wantErr string
}{
{
name: "no cluster object - returns error",
Expand All @@ -179,10 +184,18 @@ func TestReconcile(t *testing.T) {
name: "correct prerequisites - works as expected",
operatorFlag: true,
},
{
name: "error during subnet checks - returns direct error",
operatorFlag: true,
fakeCheckClusterSubnetsToReconcile: func(ctx context.Context, subnets []string) ([]string, error) {
return nil, errUnknown
},
wantErr: errUnknown.Error(),
},
{
name: "error during account reconciliation - returns direct error",
operatorFlag: true,
fakeReconcileAccounts: func(ctx context.Context) error {
fakeReconcileAccounts: func(ctx context.Context, subnets []string, storageAccounts []string) error {
return errUnknown
},
wantErr: errUnknown.Error(),
Expand Down Expand Up @@ -230,7 +243,6 @@ func TestReconcile(t *testing.T) {
log *logrus.Entry,
gotLocation, gotSubscriptionID, gotResourceGroup string,
azenv azureclient.AROEnvironment, authorizer autorest.Authorizer,
gotServiceSubnets []string, gotClusterSubnets []utilsubnet.Subnet, gotStorageAccounts []string,
) manager {
t.Helper()

Expand All @@ -244,20 +256,9 @@ func TestReconcile(t *testing.T) {
t.Errorf("wanted resource group %s but got %s", managedResourceGroupName, gotResourceGroup)
}

if diff := cmp.Diff(gotServiceSubnets, serviceSubnets, cmpoptsSortStringSlices); diff != "" {
t.Errorf("wanted service subnets %v but got %v, diff: %s", serviceSubnets, gotServiceSubnets, diff)
}
if diff := cmp.Diff(gotClusterSubnets, gotClusterSubnets, cmpoptsSortSubnetSlices); diff != "" {
t.Errorf("wanted cluster subnets %v but got %v, diff: %s", clusterSubnets, gotClusterSubnets, diff)
}

wantStorageAccounts := []string{clusterStorageAccountName, registryStorageAccountName}
if diff := cmp.Diff(gotStorageAccounts, wantStorageAccounts, cmpoptsSortStringSlices); diff != "" {
t.Errorf("wanted storage accounts %v but got %v, diff: %s", wantStorageAccounts, gotStorageAccounts, diff)
}

return &fakeManager{
fakeReconcileAccounts: tt.fakeReconcileAccounts,
fakeCheckClusterSubnetsToReconcile: tt.fakeCheckClusterSubnetsToReconcile,
fakeReconcileAccounts: tt.fakeReconcileAccounts,
}
}

Expand Down
38 changes: 15 additions & 23 deletions pkg/operator/controllers/storageaccounts/storageaccounts.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,14 +18,14 @@ import (
)

type manager interface {
reconcileAccounts(ctx context.Context) error
checkClusterSubnetsToReconcile(ctx context.Context, clusterSubnets []string) ([]string, error)
reconcileAccounts(ctx context.Context, subnets []string, storageAccounts []string) error
}

type newManager func(
log *logrus.Entry,
location, subscriptionID, resourceGroup string,
azenv azureclient.AROEnvironment, authorizer autorest.Authorizer,
serviceSubnets []string, clusterSubnets []subnet.Subnet, storageAccounts []string,
) manager

// reconcileManager is instance of manager instantiated per request
Expand All @@ -34,10 +34,6 @@ type reconcileManager struct {

location, resourceGroup string

serviceSubnets []string
clusterSubnets []subnet.Subnet
storageAccounts []string

subnet subnet.Manager
storage storage.AccountsClient
}
Expand All @@ -49,35 +45,27 @@ func newReconcileManager(

azenv azureclient.AROEnvironment,
authorizer autorest.Authorizer,

serviceSubnets []string,
clusterSubnets []subnet.Subnet,
storageAccounts []string,
) manager {
return &reconcileManager{
log: log,

location: location,
resourceGroup: resourceGroup,

serviceSubnets: serviceSubnets,
clusterSubnets: clusterSubnets,
storageAccounts: storageAccounts,

subnet: subnet.NewManager(&azenv, subscriptionID, authorizer),
storage: storage.NewAccountsClient(&azenv, subscriptionID, authorizer),
}
}

func (r *reconcileManager) reconcileAccounts(ctx context.Context) error {
subnets := r.serviceSubnets
// checkClusterSubnetsToReconcile will check cluster subnets for the Microsoft.Storage service endpoint.
// If the subnet has the service endpoint, it needs to be included in the storage account vnet rules.
func (r *reconcileManager) checkClusterSubnetsToReconcile(ctx context.Context, clusterSubnets []string) ([]string, error) {
subnetsToReconcile := []string{}

// Check each of the cluster subnets for the Microsoft.Storage service endpoint. If the subnet has
// the service endpoint, it needs to be included in the storage account vnet rules.
for _, subnet := range r.clusterSubnets {
mgmtSubnet, err := r.subnet.Get(ctx, subnet.ResourceID)
for _, subnet := range clusterSubnets {
mgmtSubnet, err := r.subnet.Get(ctx, subnet)
if err != nil {
return err
return nil, err
}

if mgmtSubnet.SubnetPropertiesFormat != nil && mgmtSubnet.SubnetPropertiesFormat.ServiceEndpoints != nil {
Expand All @@ -95,14 +83,18 @@ func (r *reconcileManager) reconcileAccounts(ctx context.Context) error {
}

if isStorageEndpoint && matchesClusterLocation {
subnets = append(subnets, subnet.ResourceID)
subnetsToReconcile = append(subnetsToReconcile, subnet)
break
}
}
}
}

for _, accountName := range r.storageAccounts {
return subnetsToReconcile, nil
}

func (r *reconcileManager) reconcileAccounts(ctx context.Context, subnets, storageAccounts []string) error {
for _, accountName := range storageAccounts {
var changed bool

account, err := r.storage.GetProperties(ctx, r.resourceGroup, accountName, "")
Expand Down
Loading

0 comments on commit 84ee524

Please sign in to comment.