diff --git a/pkg/azurefile/azure.go b/pkg/azurefile/azure.go index 51811622f5..1296f6b39e 100644 --- a/pkg/azurefile/azure.go +++ b/pkg/azurefile/azure.go @@ -109,23 +109,26 @@ func getKubeClient(kubeconfig string) (*kubernetes.Clientset, error) { return kubernetes.NewForConfig(config) } -func updateSubnetServiceEndpoints(ctx context.Context, az *azure.Cloud, subnetLockMap *lockMap) error { - if az == nil { - return fmt.Errorf("the cloud parameter is nil") - } - if subnetLockMap == nil { - return fmt.Errorf("the subnet lockMap parameter is nil") +func (d *Driver) updateSubnetServiceEndpoints(ctx context.Context) error { + if d.cloud.SubnetsClient == nil { + return fmt.Errorf("SubnetsClient is nil") } - resourceGroup := az.ResourceGroup - if len(az.VnetResourceGroup) > 0 { - resourceGroup = az.VnetResourceGroup + resourceGroup := d.cloud.ResourceGroup + if len(d.cloud.VnetResourceGroup) > 0 { + resourceGroup = d.cloud.VnetResourceGroup } - location := az.Location - vnetName := az.VnetName - subnetName := az.SubnetName + location := d.cloud.Location + vnetName := d.cloud.VnetName + subnetName := d.cloud.SubnetName + + klog.V(2).Infof("updateSubnetServiceEndpoints on VnetName: %s, SubnetName: %s", vnetName, subnetName) - subnet, err := az.SubnetsClient.Get(ctx, resourceGroup, vnetName, subnetName, "") + lockKey := resourceGroup + vnetName + subnetName + d.subnetLockMap.LockEntry(lockKey) + defer d.subnetLockMap.UnlockEntry(lockKey) + + subnet, err := d.cloud.SubnetsClient.Get(ctx, resourceGroup, vnetName, subnetName, "") if err != nil { return fmt.Errorf("failed to get the subnet %s under vnet %s: %v", subnetName, vnetName, err) } @@ -154,15 +157,11 @@ func updateSubnetServiceEndpoints(ctx context.Context, az *azure.Cloud, subnetLo serviceEndpoints = append(serviceEndpoints, storageServiceEndpoint) subnet.SubnetPropertiesFormat.ServiceEndpoints = &serviceEndpoints - lockKey := resourceGroup + vnetName + subnetName - subnetLockMap.LockEntry(lockKey) - defer subnetLockMap.UnlockEntry(lockKey) - - err = az.SubnetsClient.CreateOrUpdate(context.Background(), resourceGroup, vnetName, subnetName, subnet) + err = d.cloud.SubnetsClient.CreateOrUpdate(context.Background(), resourceGroup, vnetName, subnetName, subnet) if err != nil { return fmt.Errorf("failed to update the subnet %s under vnet %s: %v", subnetName, vnetName, err) } - klog.V(4).Infof("serviceEndpoint(%s) is appended in subnet(%s)", storageService, subnetName) + klog.V(2).Infof("serviceEndpoint(%s) is appended in subnet(%s)", storageService, subnetName) } return nil diff --git a/pkg/azurefile/azure_test.go b/pkg/azurefile/azure_test.go index afac98b88a..d66aa78339 100644 --- a/pkg/azurefile/azure_test.go +++ b/pkg/azurefile/azure_test.go @@ -30,8 +30,9 @@ import ( "sigs.k8s.io/azurefile-csi-driver/test/utils/testutil" "sigs.k8s.io/cloud-provider-azure/pkg/azureclients/subnetclient/mocksubnetclient" - azure "sigs.k8s.io/cloud-provider-azure/pkg/provider" "sigs.k8s.io/cloud-provider-azure/pkg/retry" + + azureprovider "sigs.k8s.io/cloud-provider-azure/pkg/provider" ) func skipIfTestingOnWindows(t *testing.T) { @@ -180,65 +181,35 @@ func createTestFile(path string) error { } func TestUpdateSubnetServiceEndpoints(t *testing.T) { + d := NewFakeDriver() + ctrl := gomock.NewController(t) + defer ctrl.Finish() + mockSubnetClient := mocksubnetclient.NewMockInterface(ctrl) + + config := azureprovider.Config{ + ResourceGroup: "rg", + Location: "loc", + VnetName: "fake-vnet", + SubnetName: "fake-subnet", + } - fakeSubnetLockMap := newLockMap() + d.cloud = &azureprovider.Cloud{ + SubnetsClient: mockSubnetClient, + Config: config, + } ctx := context.TODO() testCases := []struct { name string testFunc func(t *testing.T) }{ - { - name: "[fail] nil cloud parameter", - testFunc: func(t *testing.T) { - expectedErr := fmt.Errorf("the cloud parameter is nil") - err := updateSubnetServiceEndpoints(ctx, nil, nil) - if !reflect.DeepEqual(err, expectedErr) { - t.Errorf("Unexpected error: %v", err) - } - }, - }, - { - name: "[fail] nil subnetLockMap parameter", - testFunc: func(t *testing.T) { - fakeCloud := &azure.Cloud{ - Config: azure.Config{ - ResourceGroup: "rg", - Location: "loc", - VnetName: "fake-vnet", - SubnetName: "fake-subnet", - }, - } - - expectedErr := fmt.Errorf("the subnet lockMap parameter is nil") - err := updateSubnetServiceEndpoints(ctx, fakeCloud, nil) - if !reflect.DeepEqual(err, expectedErr) { - t.Errorf("Unexpected error: %v", err) - } - }, - }, { name: "[fail] no subnet", testFunc: func(t *testing.T) { retErr := retry.NewError(false, fmt.Errorf("the subnet does not exist")) - fakeCloud := &azure.Cloud{ - Config: azure.Config{ - ResourceGroup: "rg", - Location: "loc", - VnetName: "fake-vnet", - SubnetName: "fake-subnet", - }, - } - - ctrl := gomock.NewController(t) - defer ctrl.Finish() - mockSubnetClient := mocksubnetclient.NewMockInterface(ctrl) - fakeCloud.SubnetsClient = mockSubnetClient - mockSubnetClient.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(network.Subnet{}, retErr).Times(1) - - expectedErr := fmt.Errorf("failed to get the subnet %s under vnet %s: %v", fakeCloud.SubnetName, fakeCloud.VnetName, retErr) - err := updateSubnetServiceEndpoints(ctx, fakeCloud, fakeSubnetLockMap) + expectedErr := fmt.Errorf("failed to get the subnet %s under vnet %s: %v", config.SubnetName, config.VnetName, retErr) + err := d.updateSubnetServiceEndpoints(ctx) if !reflect.DeepEqual(err, expectedErr) { t.Errorf("Unexpected error: %v", err) } @@ -247,24 +218,10 @@ func TestUpdateSubnetServiceEndpoints(t *testing.T) { { name: "[success] subnetPropertiesFormat is nil", testFunc: func(t *testing.T) { - fakeCloud := &azure.Cloud{ - Config: azure.Config{ - ResourceGroup: "rg", - Location: "loc", - VnetName: "fake-vnet", - SubnetName: "fake-subnet", - }, - } - - ctrl := gomock.NewController(t) - defer ctrl.Finish() - mockSubnetClient := mocksubnetclient.NewMockInterface(ctrl) - fakeCloud.SubnetsClient = mockSubnetClient - mockSubnetClient.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(network.Subnet{}, nil).Times(1) mockSubnetClient.EXPECT().CreateOrUpdate(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil).Times(1) - err := updateSubnetServiceEndpoints(ctx, fakeCloud, fakeSubnetLockMap) + err := d.updateSubnetServiceEndpoints(ctx) if !reflect.DeepEqual(err, nil) { t.Errorf("Unexpected error: %v", err) } @@ -276,24 +233,11 @@ func TestUpdateSubnetServiceEndpoints(t *testing.T) { fakeSubnet := network.Subnet{ SubnetPropertiesFormat: &network.SubnetPropertiesFormat{}, } - fakeCloud := &azure.Cloud{ - Config: azure.Config{ - ResourceGroup: "rg", - Location: "loc", - VnetName: "fake-vnet", - SubnetName: "fake-subnet", - }, - } - - ctrl := gomock.NewController(t) - defer ctrl.Finish() - mockSubnetClient := mocksubnetclient.NewMockInterface(ctrl) - fakeCloud.SubnetsClient = mockSubnetClient mockSubnetClient.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(fakeSubnet, nil).Times(1) mockSubnetClient.EXPECT().CreateOrUpdate(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil).Times(1) - err := updateSubnetServiceEndpoints(ctx, fakeCloud, fakeSubnetLockMap) + err := d.updateSubnetServiceEndpoints(ctx) if !reflect.DeepEqual(err, nil) { t.Errorf("Unexpected error: %v", err) } @@ -307,24 +251,11 @@ func TestUpdateSubnetServiceEndpoints(t *testing.T) { ServiceEndpoints: &[]network.ServiceEndpointPropertiesFormat{}, }, } - fakeCloud := &azure.Cloud{ - Config: azure.Config{ - ResourceGroup: "rg", - Location: "loc", - VnetName: "fake-vnet", - SubnetName: "fake-subnet", - }, - } - - ctrl := gomock.NewController(t) - defer ctrl.Finish() - mockSubnetClient := mocksubnetclient.NewMockInterface(ctrl) - fakeCloud.SubnetsClient = mockSubnetClient mockSubnetClient.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(fakeSubnet, nil).Times(1) mockSubnetClient.EXPECT().CreateOrUpdate(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil).Times(1) - err := updateSubnetServiceEndpoints(ctx, fakeCloud, fakeSubnetLockMap) + err := d.updateSubnetServiceEndpoints(ctx) if !reflect.DeepEqual(err, nil) { t.Errorf("Unexpected error: %v", err) } @@ -342,28 +273,26 @@ func TestUpdateSubnetServiceEndpoints(t *testing.T) { }, }, } - fakeCloud := &azure.Cloud{ - Config: azure.Config{ - ResourceGroup: "rg", - Location: "loc", - VnetName: "fake-vnet", - SubnetName: "fake-subnet", - }, - } - - ctrl := gomock.NewController(t) - defer ctrl.Finish() - mockSubnetClient := mocksubnetclient.NewMockInterface(ctrl) - fakeCloud.SubnetsClient = mockSubnetClient mockSubnetClient.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(fakeSubnet, nil).Times(1) - err := updateSubnetServiceEndpoints(ctx, fakeCloud, fakeSubnetLockMap) + err := d.updateSubnetServiceEndpoints(ctx) if !reflect.DeepEqual(err, nil) { t.Errorf("Unexpected error: %v", err) } }, }, + { + name: "[fail] SubnetsClient is nil", + testFunc: func(t *testing.T) { + d.cloud.SubnetsClient = nil + expectedErr := fmt.Errorf("SubnetsClient is nil") + err := d.updateSubnetServiceEndpoints(ctx) + if !reflect.DeepEqual(err, expectedErr) { + t.Errorf("Unexpected error: %v", err) + } + }, + }, } for _, tc := range testCases { diff --git a/pkg/azurefile/controllerserver.go b/pkg/azurefile/controllerserver.go index b76b0846b3..75caa76254 100644 --- a/pkg/azurefile/controllerserver.go +++ b/pkg/azurefile/controllerserver.go @@ -178,9 +178,8 @@ func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest) klog.V(2).Infof("set vnetResourceID(%s) for NFS protocol", vnetResourceID) vnetResourceIDs = []string{vnetResourceID} - err := updateSubnetServiceEndpoints(ctx, d.cloud, d.subnetLockMap) - if err != nil { - return nil, status.Errorf(codes.Internal, "failed to update the service endpoints: %v", err) + if err := d.updateSubnetServiceEndpoints(ctx); err != nil { + return nil, status.Errorf(codes.Internal, "update service endpoints failed with error: %v", err) } } diff --git a/pkg/azurefile/controllerserver_test.go b/pkg/azurefile/controllerserver_test.go index f548637708..3b9290585f 100644 --- a/pkg/azurefile/controllerserver_test.go +++ b/pkg/azurefile/controllerserver_test.go @@ -370,7 +370,7 @@ func TestCreateVolume(t *testing.T) { csi.ControllerServiceCapability_RPC_CREATE_DELETE_VOLUME, }) - expectedErr := status.Errorf(codes.Internal, "failed to update the service endpoints: failed to get the subnet fake-subnet under vnet fake-vnet: &{false 0 0001-01-01 00:00:00 +0000 UTC the subnet does not exist}") + expectedErr := status.Errorf(codes.Internal, "update service endpoints failed with error: failed to get the subnet fake-subnet under vnet fake-vnet: &{false 0 0001-01-01 00:00:00 +0000 UTC the subnet does not exist}") _, err := d.CreateVolume(ctx, req) if !reflect.DeepEqual(err, expectedErr) { t.Errorf("Unexpected error: %v", err)