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.

Sign up for GitHub

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

Jump to bottom

Fix failing copy_ami_driver tests #35

Merged
merged 2 commits into from
Dec 8, 2023
Merged

Fix failing copy_ami_driver tests #35

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f55e385
Fix failing copy_ami_driver tests
mvach Dec 4, 2023
0928693
Prevent flaky copy_ami_driver tests
mvach Dec 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
146 changes: 91 additions & 55 deletions driver/copy_ami_driver_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,25 +16,21 @@ import (
uuid "github.com/satori/go.uuid"
)

type AmiCopyConfig struct {
amiId string
encrypted bool
kmsKeyId string
sharedWithAccounts []string
}

var _ = Describe("CopyAmiDriver", func() {
It("copies an existing AMI to a new region while preserving its properties", func() {
copyAmi(false, "", amiFixtureID, func(ec2Client *ec2.EC2, reqOutput *ec2.DescribeImagesOutput) {
snapshotIDptr := getSnapshotID(reqOutput)

snapshotAttributes, err := ec2Client.DescribeSnapshotAttribute(&ec2.DescribeSnapshotAttributeInput{
SnapshotId: snapshotIDptr,
Attribute: aws.String("createVolumePermission"),
})
Expect(err).ToNot(HaveOccurred())

Expect(len(snapshotAttributes.CreateVolumePermissions)).To(Equal(1))
Expect(*snapshotAttributes.CreateVolumePermissions[0].Group).To(Equal("all"))
})
})

Context("when encrypted flag is set to true", func() {
It("does NOT make snapshot public", func() {
copyAmi(true, "", amiFixtureID, func(ec2Client *ec2.EC2, reqOutput *ec2.DescribeImagesOutput) {
copyAmi(
AmiCopyConfig{
amiId: amiFixtureID,
encrypted: false,
kmsKeyId: ""},
func(ec2Client *ec2.EC2, reqOutput *ec2.DescribeImagesOutput) {
snapshotIDptr := getSnapshotID(reqOutput)

snapshotAttributes, err := ec2Client.DescribeSnapshotAttribute(&ec2.DescribeSnapshotAttributeInput{
Expand All @@ -43,72 +39,112 @@ var _ = Describe("CopyAmiDriver", func() {
})
Expect(err).ToNot(HaveOccurred())

Expect(len(snapshotAttributes.CreateVolumePermissions)).To(Equal(0))
Expect(len(snapshotAttributes.CreateVolumePermissions)).To(Equal(1))
Expect(*snapshotAttributes.CreateVolumePermissions[0].Group).To(Equal("all"))
})
})
})

It("encrypts destination AMI using default AWS KMS key", func() {
copyAmi(true, "", amiFixtureID, func(ec2Client *ec2.EC2, reqOutput *ec2.DescribeImagesOutput) {
respSnapshots, err := ec2Client.DescribeSnapshots(&ec2.DescribeSnapshotsInput{SnapshotIds: []*string{reqOutput.Images[0].BlockDeviceMappings[0].Ebs.SnapshotId}})
Expect(err).ToNot(HaveOccurred())
Context("when encrypted flag is set to true", func() {
It("does NOT make snapshot public", func() {
copyAmi(
AmiCopyConfig{
amiId: amiFixtureID,
encrypted: true,
kmsKeyId: "",
},
func(ec2Client *ec2.EC2, reqOutput *ec2.DescribeImagesOutput) {
snapshotIDptr := getSnapshotID(reqOutput)

snapshotAttributes, err := ec2Client.DescribeSnapshotAttribute(&ec2.DescribeSnapshotAttributeInput{
SnapshotId: snapshotIDptr,
Attribute: aws.String("createVolumePermission"),
})
Expect(err).ToNot(HaveOccurred())

Expect(*respSnapshots.Snapshots[0].Encrypted).To(BeTrue())
})
Expect(len(snapshotAttributes.CreateVolumePermissions)).To(Equal(0))
})
})

Context("when kms_key_id is provided", func() {
It("encrypts destination AMI using provided kms key", func() {
copyAmi(true, kmsKeyId, amiFixtureID, func(ec2Client *ec2.EC2, reqOutput *ec2.DescribeImagesOutput) {
It("encrypts destination AMI using default AWS KMS key", func() {
copyAmi(
AmiCopyConfig{
amiId: amiFixtureID,
encrypted: true,
kmsKeyId: "",
},
func(ec2Client *ec2.EC2, reqOutput *ec2.DescribeImagesOutput) {
respSnapshots, err := ec2Client.DescribeSnapshots(&ec2.DescribeSnapshotsInput{SnapshotIds: []*string{reqOutput.Images[0].BlockDeviceMappings[0].Ebs.SnapshotId}})
Expect(err).ToNot(HaveOccurred())

Expect(*respSnapshots.Snapshots[0].Encrypted).To(BeTrue())
Expect(*respSnapshots.Snapshots[0].KmsKeyId).To(Equal(kmsKeyId))
})
})

Context("when kms_key_id is provided", func() {
It("encrypts destination AMI using the kms key in the destination region", func() {
destinationRegionKmsKeyId := strings.ReplaceAll(multiRegionKey, creds.Region, destinationRegion)
copyAmi(
AmiCopyConfig{
amiId: privateAmiFixtureID,
encrypted: true,
kmsKeyId: destinationRegionKmsKeyId,
},
func(ec2Client *ec2.EC2, reqOutput *ec2.DescribeImagesOutput) {
respSnapshots, err := ec2Client.DescribeSnapshots(&ec2.DescribeSnapshotsInput{SnapshotIds: []*string{reqOutput.Images[0].BlockDeviceMappings[0].Ebs.SnapshotId}})
Expect(err).ToNot(HaveOccurred())

Expect(*respSnapshots.Snapshots[0].Encrypted).To(BeTrue())
Expect(*respSnapshots.Snapshots[0].KmsKeyId).To(Equal(destinationRegionKmsKeyId))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please note that we change the test expectation here.

We never managed to share an AMI with a single region kms key across regions. It's as well documented that this is not working.

After a discussion with @ramonskie it turned out that no one is using encrypted AMIs so far. Therefor we adapted the behaviour in a way that a multi region kms key need to be provided when copying encrypted AMIs across regions. Due to that we check the replicated destination key here.

})
})
})
})

Context("when shared_with_accounts is provided", func() {
It("shares the AMI with other accounts", func() {
copyAmi(true, multiregionKmsKeyId, privateAmiFixtureID, func(ec2Client *ec2.EC2, reqOutput *ec2.DescribeImagesOutput) {
attribute := "launchPermission"
output, err := ec2Client.DescribeImageAttribute(&ec2.DescribeImageAttributeInput{
ImageId: reqOutput.Images[0].ImageId,
Attribute: &attribute,
destinationRegionKmsKeyId := strings.ReplaceAll(multiRegionKey, creds.Region, destinationRegion)
copyAmi(AmiCopyConfig{
amiId: privateAmiFixtureID,
encrypted: true,
kmsKeyId: destinationRegionKmsKeyId,
sharedWithAccounts: []string{awsAccount},
},
func(ec2Client *ec2.EC2, reqOutput *ec2.DescribeImagesOutput) {
attribute := "launchPermission"
output, err := ec2Client.DescribeImageAttribute(&ec2.DescribeImageAttributeInput{
ImageId: reqOutput.Images[0].ImageId,
Attribute: &attribute,
})
Expect(err).ToNot(HaveOccurred())
Expect(*output.LaunchPermissions[0].UserId).To(Equal(awsAccount))
})
Expect(err).ToNot(HaveOccurred())
Expect(*output.LaunchPermissions[0].UserId).To(Equal(awsAccount))
})
})
})
})

func copyAmi(encrypted bool, kmsKeyId string, amiId string, cb ...func(*ec2.EC2, *ec2.DescribeImagesOutput)) {
func copyAmi(amiCopyConfig AmiCopyConfig, cb ...func(*ec2.EC2, *ec2.DescribeImagesOutput)) {
accessibility := resources.PublicAmiAccessibility
if encrypted {
if amiCopyConfig.encrypted {
accessibility = resources.PrivateAmiAccessibility
}

var sharedWithAccounts []string
if kmsKeyId != "" {
sharedWithAccounts = []string{awsAccount}
} else {
sharedWithAccounts = []string{}
amiProperties := resources.AmiProperties{
Name: fmt.Sprintf("BOSH-%s", strings.ToUpper(uuid.NewV4().String())),
VirtualizationType: resources.HvmAmiVirtualization,
Description: "bosh cpi test ami",
Accessibility: accessibility,
Encrypted: amiCopyConfig.encrypted,
KmsKeyId: amiCopyConfig.kmsKeyId,
}
if len(amiCopyConfig.sharedWithAccounts) > 0 {
amiProperties.SharedWithAccounts = amiCopyConfig.sharedWithAccounts
}

amiDriverConfig := resources.AmiDriverConfig{
ExistingAmiID: amiId,
ExistingAmiID: amiCopyConfig.amiId,
DestinationRegion: destinationRegion,
AmiProperties: resources.AmiProperties{
Name: fmt.Sprintf("BOSH-%s", strings.ToUpper(uuid.NewV4().String())),
VirtualizationType: resources.HvmAmiVirtualization,
Description: "bosh cpi test ami",
Accessibility: accessibility,
Encrypted: encrypted,
KmsKeyId: kmsKeyId,
SharedWithAccounts: sharedWithAccounts,
},
AmiProperties: amiProperties,
KmsKey: resources.KmsKey{ARN: amiCopyConfig.kmsKeyId},
}

amiCopyDriver := driverset.NewStandardRegionDriverSet(GinkgoWriter, creds).CopyAmiDriver()
Expand All @@ -133,7 +169,7 @@ func copyAmi(encrypted bool, kmsKeyId string, amiId string, cb ...func(*ec2.EC2,
Expect(*firstImage.Name).To(Equal(amiDriverConfig.Name))
Expect(*firstImage.Architecture).To(Equal(resources.AmiArchitecture))
Expect(*firstImage.VirtualizationType).To(Equal(amiDriverConfig.VirtualizationType))
if !encrypted {
if !amiCopyConfig.encrypted {
Expect(*firstImage.Public).To(BeTrue())
}

Expand Down
9 changes: 6 additions & 3 deletions driver/driver_suite_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ var ebsVolumeID, ebsSnapshotID string
var machineImagePath, machineImageFormat string
var s3MachineImageUrl, s3MachineImageFormat string

var kmsKeyId, multiregionKmsKeyId string
var kmsKeyId, multiRegionKey, multiRegionKeyReplicationTest string

var awsAccount string

Expand Down Expand Up @@ -78,8 +78,11 @@ var _ = SynchronizedBeforeSuite(
kmsKeyId = os.Getenv("AWS_KMS_KEY_ID")
Expect(kmsKeyId).ToNot(BeEmpty(), "AWS_KMS_KEY_ID must be set")

multiregionKmsKeyId = os.Getenv("MULTI_REGION_AWS_KMS_KEY_ID")
Expect(multiregionKmsKeyId).ToNot(BeEmpty(), "MULTI_REGION_AWS_KMS_KEY_ID must be set")
multiRegionKey = os.Getenv("MULTI_REGION_KEY")
Expect(multiRegionKey).ToNot(BeEmpty(), "MULTI_REGION_KEY must be set")

multiRegionKeyReplicationTest = os.Getenv("MULTI_REGION_KEY_REPLICATION_TEST")
Expect(multiRegionKeyReplicationTest).ToNot(BeEmpty(), "MULTI_REGION_KEY_REPLICATION_TEST must be set")

awsAccount = os.Getenv("AWS_ACCOUNT")
Expect(awsAccount).ToNot(BeEmpty(), "AWS_ACCOUNT must be set")
Expand Down
4 changes: 2 additions & 2 deletions driver/kms_driver_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ var _ = Describe("KmsDriver", func() {

It("replicates a given kms key to another region", func() {
driverConfig := resources.KmsReplicateKeyDriverConfig{
KmsKeyId: multiregionKmsKeyId,
KmsKeyId: multiRegionKeyReplicationTest,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test triggers the cleanup of the replicated key in line 81. That makes the private AMI tests in the copy_ami_driver_tests flakey. Therefore we have to separate the used multiregion kms keys here.

SourceRegion: creds.Region,
TargetRegion: destinationRegion,
}
Expand All @@ -74,7 +74,7 @@ var _ = Describe("KmsDriver", func() {
//defer cleanup of the created key replica, sadly we can only schedule it to be deleted after 7 days
//therefore this test will reuse the replicated key for 7 days and only afterward create a new one
defer func(aliasCreationResult resources.KmsKey) {
destinationKeyId := strings.ReplaceAll(kmsKeyId, originalRegion, destinationRegion)
destinationKeyId := strings.ReplaceAll(multiRegionKeyReplicationTest, originalRegion, destinationRegion)
awsSession, _ := session.NewSession(creds.GetAwsConfig())
kmsClient := kms.New(awsSession)

Expand Down