Skip to content

Commit

Permalink
Merge pull request #656 from kubernetes-sigs/refine-updatesvc-endpoint
Browse files Browse the repository at this point in the history 
chore: refine updateSubnetServiceEndpoints
  • Loading branch information
@andyzhangx
andyzhangx authored May 16, 2021
2 parents ee1f59b + ac3c703 commit fad3618
Show file tree
Hide file tree
Showing 4 changed files with 55 additions and 128 deletions.
37 changes: 18 additions & 19 deletions pkg/azurefile/azure.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down Expand Up @@ -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
Expand Down
139 changes: 34 additions & 105 deletions pkg/azurefile/azure_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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)
}
Expand All @@ -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)
}
Expand All @@ -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)
}
Expand All @@ -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)
}
Expand All @@ -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 {
Expand Down
5 changes: 2 additions & 3 deletions pkg/azurefile/controllerserver.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}

Expand Down
2 changes: 1 addition & 1 deletion pkg/azurefile/controllerserver_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down

0 comments on commit fad3618

Please # to comment.