Merge pull request #1620 from jrakas-dev/use-existing-ap

Return existing access point if one already exists during create workflow

Author: k8s-ci-robot

pkg/cloud/cloud.go

@@ -204,6 +204,9 @@ func (c *cloud) CreateAccessPoint(ctx context.Context, clientToken string, acces
 		if isAccessDenied(err) {
 			return nil, ErrAccessDenied
 		}
+		if isAccessPointAlreadyExists(err) {
+			return nil, ErrAlreadyExists
+		}
 		return nil, fmt.Errorf("Failed to create access point: %v", err)
 	}
 	klog.V(5).Infof("Create AP response : %+v", res)
@@ -289,6 +292,10 @@ func (c *cloud) FindAccessPointByClientToken(ctx context.Context, clientToken, f
 				AccessPointId:      *ap.AccessPointId,
 				FileSystemId:       *ap.FileSystemId,
 				AccessPointRootDir: *ap.RootDirectory.Path,
+				PosixUser: &PosixUser{
+					Gid: *ap.PosixUser.Gid,
+					Uid: *ap.PosixUser.Uid,
+				},
 			}, nil
 		}
 	}
@@ -435,6 +442,16 @@ func isAccessDenied(err error) bool {
 	return false
 }
 
+func isAccessPointAlreadyExists(err error) bool {
+	var apiErr smithy.APIError
+	if errors.As(err, &apiErr) {
+		if apiErr.ErrorCode() == AccessPointAlreadyExists {
+			return true
+		}
+	}
+	return false
+}
+
 func isDriverBootedInECS() bool {
 	ecsContainerMetadataUri := os.Getenv(taskMetadataV4EnvName)
 	return ecsContainerMetadataUri != ""

pkg/cloud/cloud_test.go

@@ -158,6 +158,38 @@ func TestCreateAccessPoint(t *testing.T) {
 				mockCtl.Finish()
 			},
 		},
+		{
+			name: "Error: Access Point already exists",
+			testFunc: func(t *testing.T) {
+				mockCtl := gomock.NewController(t)
+				mockEfs := mocks.NewMockEfs(mockCtl)
+				c := &cloud{efs: mockEfs}
+
+				req := &AccessPointOptions{
+					FileSystemId:   fsId,
+					Uid:            uid,
+					Gid:            gid,
+					DirectoryPerms: directoryPerms,
+					DirectoryPath:  directoryPath,
+				}
+
+				ctx := context.Background()
+				mockEfs.EXPECT().CreateAccessPoint(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil,
+					&smithy.GenericAPIError{
+						Code:    AccessPointAlreadyExists,
+						Message: "Access point already exists",
+					})
+
+				_, err := c.CreateAccessPoint(ctx, clientToken, req)
+				if err == nil {
+					t.Fatalf("CreateAccessPoint did not return error")
+				}
+				if err != ErrAlreadyExists {
+					t.Fatalf("Failed. Expected: %v, Actual:%v", ErrAlreadyExists, err)
+				}
+				mockCtl.Finish()
+			},
+		},
 	}
 
 	for _, tc := range testCases {
@@ -1085,6 +1117,16 @@ func Test_findAccessPointByPath(t *testing.T) {
 		FileSystemId:       fsId,
 	}
 
+	expectedSingleAPWithPosixUser := &AccessPoint{
+		AccessPointId:      "testApId",
+		AccessPointRootDir: dirPath,
+		PosixUser: &PosixUser{
+			Uid: 1000,
+			Gid: 1000,
+		},
+		FileSystemId: fsId,
+	}
+
 	type args struct {
 		clientToken     string
 		accessPointOpts *AccessPointOptions
@@ -1105,10 +1147,16 @@ func Test_findAccessPointByPath(t *testing.T) {
 			mockEfs.EXPECT().DescribeAccessPoints(gomock.Any(), gomock.Any(), gomock.Any()).Return(&efs.DescribeAccessPointsOutput{
 				AccessPoints: []types.AccessPointDescription{
 					{FileSystemId: aws.String(fsId), ClientToken: diffClientToken, AccessPointId: aws.String("differentApId"), RootDirectory: &types.RootDirectory{Path: aws.String(expectedSingleAP.AccessPointRootDir)}},
-					{FileSystemId: aws.String(fsId), ClientToken: &clientToken, AccessPointId: aws.String(expectedSingleAP.AccessPointId), RootDirectory: &types.RootDirectory{Path: aws.String(expectedSingleAP.AccessPointRootDir)}},
+					{
+						FileSystemId:  aws.String(fsId),
+						ClientToken:   &clientToken,
+						AccessPointId: aws.String(expectedSingleAPWithPosixUser.AccessPointId),
+						RootDirectory: &types.RootDirectory{Path: aws.String(expectedSingleAPWithPosixUser.AccessPointRootDir)},
+						PosixUser:     &types.PosixUser{Gid: aws.Int64(1000), Uid: aws.Int64(1000)},
+					},
 				},
 			}, nil)
-		}, wantAccessPoint: expectedSingleAP, wantErr: false},
+		}, wantAccessPoint: expectedSingleAPWithPosixUser, wantErr: false},
 		{name: "Fail_DescribeAccessPoints", args: args{clientToken, &AccessPointOptions{FileSystemId: fsId, DirectoryPath: dirPath}}, prepare: func(mockEfs *mocks.MockEfs) {
 			mockEfs.EXPECT().DescribeAccessPoints(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, errors.New("access_denied"))
 		}, wantAccessPoint: nil, wantErr: true},

pkg/driver/controller.go

@@ -351,11 +351,23 @@ func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest)
 		if err != nil {
 			if err == cloud.ErrAccessDenied {
 				return nil, status.Errorf(codes.Unauthenticated, "Access Denied. Please ensure you have the right AWS permissions: %v", err)
+			} else if err == cloud.ErrAlreadyExists {
+				klog.V(4).Infof("Access point already exists for client token %s. Retrieving existing access point details.", clientToken)
+				existingAccessPoint, err := localCloud.FindAccessPointByClientToken(ctx, clientToken, accessPointsOptions.FileSystemId)
+				if err != nil {
+					return nil, fmt.Errorf("Error attempting to retrieve existing access point for client token %s: %v", clientToken, err)
+				}
+				if existingAccessPoint == nil {
+					return nil, fmt.Errorf("No access point for client token %s was returned: %v", clientToken, err)
+				}
+				err = validateExistingAccessPoint(existingAccessPoint, basePath, gidMin, gidMax)
+				if err != nil {
+					return nil, status.Errorf(codes.AlreadyExists, "Invalid existing access point: %v", err)
+				}
+				accessPoint = existingAccessPoint
+			} else {
+				return nil, status.Errorf(codes.Internal, "Failed to create Access point in File System %v : %v", accessPointsOptions.FileSystemId, err)
 			}
-			if err == cloud.ErrAlreadyExists {
-				return nil, status.Errorf(codes.AlreadyExists, "Access Point already exists")
-			}
-			return nil, status.Errorf(codes.Internal, "Failed to create Access point in File System %v : %v", accessPointsOptions.FileSystemId, err)
 		}
 
 		// Lock on the new access point to prevent accidental deletion before creation is done
@@ -711,3 +723,26 @@ func get64LenHash(text string) string {
 	h.Write([]byte(text))
 	return fmt.Sprintf("%x", h.Sum(nil))
 }
+
+func validateExistingAccessPoint(existingAccessPoint *cloud.AccessPoint, basePath string, gidMin int64, gidMax int64) error {
+
+	normalizedBasePath := strings.TrimPrefix(basePath, "/")
+	normalizedAccessPointPath := strings.TrimPrefix(existingAccessPoint.AccessPointRootDir, "/")
+	if !strings.HasPrefix(normalizedAccessPointPath, normalizedBasePath) {
+		return fmt.Errorf("Access point found but has different base path than what's specified in storage class")
+	}
+
+	if existingAccessPoint.PosixUser == nil {
+		return fmt.Errorf("Access point found but PosixUser is nil")
+	}
+
+	if existingAccessPoint.PosixUser.Gid < gidMin || existingAccessPoint.PosixUser.Gid > gidMax {
+		return fmt.Errorf("Access point found but its GID is outside the specified range")
+	}
+
+	if existingAccessPoint.PosixUser.Uid < gidMin || existingAccessPoint.PosixUser.Uid > gidMax {
+		return fmt.Errorf("Access point found but its UID is outside the specified range")
+	}
+
+	return nil
+}