Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for creating encrypted volume #80

Merged
merged 1 commit into from
Oct 26, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
74 changes: 70 additions & 4 deletions pkg/cloud/cloud.go
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,10 @@ type DiskOptions struct {
VolumeType string
IOPSPerGB int
AvailabilityZone string
Encrypted bool
// KmsKeyID represents a fully qualified resource name to the key to use for encryption.
// example: arn:aws:kms:us-east-1:012345678910:key/abcd1234-a123-456a-a12b-a123b4cd56ef
leakingtapan marked this conversation as resolved.
Show resolved Hide resolved
KmsKeyID string
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to have only this field (and get rid of Encrypted)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above comment

}

// EC2 abstracts aws.EC2 to facilitate its mocking.
Expand Down Expand Up @@ -215,6 +219,11 @@ func (c *cloud) CreateDisk(ctx context.Context, volumeName string, diskOptions *
Size: aws.Int64(capacityGiB),
VolumeType: aws.String(createType),
TagSpecifications: []*ec2.TagSpecification{&tagSpec},
Encrypted: aws.Bool(diskOptions.Encrypted),
}
if len(diskOptions.KmsKeyID) > 0 {
request.KmsKeyId = aws.String(diskOptions.KmsKeyID)
request.Encrypted = aws.Bool(true)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happen if diskOptions.Encrypted is true and diskOptions.KmsKeyID is empty? Is this something we want to handle?

Copy link
Contributor Author

@leakingtapan leakingtapan Oct 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In case of KmsKeyID is empty, EBS will use a default key to create the volume if Encrypted is set true. See: https://docs.aws.amazon.com/sdk-for-go/api/service/ec2/#CreateVolumeInput

}
if iops > 0 {
request.Iops = aws.Int64(iops)
Expand All @@ -235,16 +244,23 @@ func (c *cloud) CreateDisk(ctx context.Context, volumeName string, diskOptions *
return nil, fmt.Errorf("disk size was not returned by CreateVolume")
}

if len(diskOptions.KmsKeyID) > 0 {
err := c.waitForCreate(ctx, volumeID)
if err != nil {
if isAWSErrorVolumeNotFound(err) {
return nil, fmt.Errorf("failed to create encrypted volume: the volume disappeared after creation, most likely due to inaccessible KMS encryption key")
}
}
}

return &Disk{CapacityGiB: size, VolumeID: volumeID, AvailabilityZone: zone}, nil
}

func (c *cloud) DeleteDisk(ctx context.Context, volumeID string) (bool, error) {
request := &ec2.DeleteVolumeInput{VolumeId: &volumeID}
if _, err := c.ec2.DeleteVolumeWithContext(ctx, request); err != nil {
if awsErr, ok := err.(awserr.Error); ok {
if awsErr.Code() == "InvalidVolume.NotFound" {
return false, ErrNotFound
}
if isAWSErrorVolumeNotFound(err) {
return false, ErrNotFound
}
return false, fmt.Errorf("DeleteDisk could not delete volume: %v", err)
}
Expand Down Expand Up @@ -487,3 +503,53 @@ func (c *cloud) waitForAttachmentState(ctx context.Context, volumeID, state stri

return wait.ExponentialBackoff(backoff, verifyVolumeFunc)
}

// waitForCreate waits for volume to be created for encrypted volume only
// it polls for created volume to check it has not been silently removed by AWS.
// On a random AWS account (shared among several developers) it took 4s on average.
func (c *cloud) waitForCreate(ctx context.Context, volumeID string) error {
var (
checkInterval = 1 * time.Second
checkTimeout = 30 * time.Second
)

request := &ec2.DescribeVolumesInput{
VolumeIds: []*string{
aws.String(volumeID),
},
}

err := wait.Poll(checkInterval, checkTimeout, func() (done bool, err error) {
vol, err := c.getVolume(ctx, request)
if err != nil {
return true, err
}
if vol.State != nil {
switch *vol.State {
case "available":
// The volume is Available, it won't be deleted now.
return true, nil
case "creating":
return false, nil
default:
return true, fmt.Errorf("unexpected State of newly created AWS EBS volume %s: %q", volumeID, *vol.State)
}
}
return false, nil
})

return err
}

// Helper function for describeVolume callers. Tries to retype given error to AWS error
// and returns true in case the AWS error is "InvalidVolume.NotFound", false otherwise
func isAWSErrorVolumeNotFound(err error) bool {
if awsError, ok := err.(awserr.Error); ok {
// https://docs.aws.amazon.com/AWSEC2/latest/APIReference/errors-overview.html
if awsError.Code() == "InvalidVolume.NotFound" {
return true
}
}

return false
}
21 changes: 21 additions & 0 deletions pkg/cloud/cloud_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,22 @@ func TestCreateDisk(t *testing.T) {
},
expErr: fmt.Errorf("CreateVolume generic error"),
},
{
name: "success: normal with encrypted volume",
volumeName: "vol-test-name",
diskOptions: &DiskOptions{
CapacityBytes: util.GiBToBytes(1),
Tags: map[string]string{VolumeNameTagKey: "vol-test"},
AvailabilityZone: "us-west-2",
Encrypted: true,
KmsKeyID: "arn:aws:kms:us-east-1:012345678910:key/abcd1234-a123-456a-a12b-a123b4cd56ef",
},
expDisk: &Disk{
VolumeID: "vol-test",
CapacityGiB: 1,
},
expErr: nil,
},
}

for _, tc := range testCases {
Expand All @@ -90,12 +106,17 @@ func TestCreateDisk(t *testing.T) {
vol = &ec2.Volume{
VolumeId: aws.String(tc.diskOptions.Tags[VolumeNameTagKey]),
Size: aws.Int64(util.BytesToGiB(tc.diskOptions.CapacityBytes)),
State: aws.String("available"),
}
}

ctx := context.Background()
mockEC2.EXPECT().CreateVolumeWithContext(gomock.Eq(ctx), gomock.Any()).Return(vol, tc.expErr)

if tc.diskOptions.Encrypted {
mockEC2.EXPECT().DescribeVolumesWithContext(gomock.Eq(ctx), gomock.Any()).Return(&ec2.DescribeVolumesOutput{Volumes: []*ec2.Volume{vol}}, nil)
}

disk, err := c.CreateDisk(ctx, tc.volumeName, tc.diskOptions)
if err != nil {
if tc.expErr == nil {
Expand Down
11 changes: 11 additions & 0 deletions pkg/driver/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -86,12 +86,23 @@ func (d *Driver) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest)
}
}

var (
isEncrypted bool
kmsKeyId string
)
if volumeParams["encrypted"] == "true" {
isEncrypted = true
kmsKeyId = volumeParams["kmsKeyId"]
}

opts := &cloud.DiskOptions{
CapacityBytes: volSizeBytes,
Tags: map[string]string{cloud.VolumeNameTagKey: volName},
VolumeType: volumeType,
IOPSPerGB: iopsPerGB,
AvailabilityZone: zone,
Encrypted: isEncrypted,
KmsKeyID: kmsKeyId,
}
disk, err = d.cloud.CreateDisk(ctx, volName, opts)
if err != nil {
Expand Down
33 changes: 33 additions & 0 deletions pkg/driver/controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,39 @@ func TestCreateVolume(t *testing.T) {
Attributes: map[string]string{"fsType": ""},
},
},
{
name: "success with volume encrpytion",
req: &csi.CreateVolumeRequest{
Name: "vol-test",
CapacityRange: stdCapRange,
VolumeCapabilities: stdVolCap,
Parameters: map[string]string{
"encrypted": "true",
},
},
expVol: &csi.Volume{
CapacityBytes: stdVolSize,
Id: "vol-test",
Attributes: map[string]string{"fsType": ""},
},
},
{
name: "success with volume encrpytion with KMS key",
req: &csi.CreateVolumeRequest{
Name: "vol-test",
CapacityRange: stdCapRange,
VolumeCapabilities: stdVolCap,
Parameters: map[string]string{
"encrypted": "true",
"kmsKeyId": "arn:aws:kms:us-east-1:012345678910:key/abcd1234-a123-456a-a12b-a123b4cd56ef",
},
},
expVol: &csi.Volume{
CapacityBytes: stdVolSize,
Id: "vol-test",
Attributes: map[string]string{"fsType": ""},
},
},
}

for _, tc := range testCases {
Expand Down