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

data-source/aws_kms_alias: Prevent crash on aliases without target key #3203

Merged
merged 2 commits into from
Jan 31, 2018
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
30 changes: 17 additions & 13 deletions aws/data_source_aws_kms_alias.go
Original file line number Diff line number Diff line change
Expand Up @@ -64,20 +64,24 @@ func dataSourceAwsKmsAliasRead(d *schema.ResourceData, meta interface{}) error {
d.SetId(time.Now().UTC().String())
d.Set("arn", alias.AliasArn)

aliasARN, err := arn.Parse(*alias.AliasArn)
if err != nil {
return err
}
targetKeyARN := arn.ARN{
Partition: aliasARN.Partition,
Service: aliasARN.Service,
Region: aliasARN.Region,
AccountID: aliasARN.AccountID,
Resource: fmt.Sprintf("key/%s", *alias.TargetKeyId),
}
d.Set("target_key_arn", targetKeyARN.String())
// AWS service aliases do not return TargetKeyId:
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: I would probably enhance this message so that we add the context ... do not return TargetKeyId when not associated with a Customer Managed Key (CMK). What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree with @Ninir, there are cases where the key is associated, it is just not the case all the time

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll update this comment 👍

// https://docs.aws.amazon.com/kms/latest/APIReference/API_ListAliases.html
if alias.TargetKeyId != nil {
aliasARN, err := arn.Parse(*alias.AliasArn)
if err != nil {
return err
}
targetKeyARN := arn.ARN{
Partition: aliasARN.Partition,
Service: aliasARN.Service,
Region: aliasARN.Region,
AccountID: aliasARN.AccountID,
Resource: fmt.Sprintf("key/%s", *alias.TargetKeyId),
}
d.Set("target_key_arn", targetKeyARN.String())

d.Set("target_key_id", alias.TargetKeyId)
d.Set("target_key_id", alias.TargetKeyId)
}

return nil
}
72 changes: 58 additions & 14 deletions aws/data_source_aws_kms_alias_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package aws

import (
"fmt"
"regexp"
"strings"
"testing"

Expand All @@ -10,32 +11,69 @@ import (
"github.com/hashicorp/terraform/terraform"
)

func TestAccDataSourceAwsKmsAlias(t *testing.T) {
func TestAccDataSourceAwsKmsAlias_AwsService(t *testing.T) {
name := "alias/aws/redshift"
resourceName := "data.aws_kms_alias.test"

resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
Steps: []resource.TestStep{
resource.TestStep{
Config: testAccDataSourceAwsKmsAlias_name(name),
Check: resource.ComposeTestCheckFunc(
testAccDataSourceAwsKmsAliasCheckExists(resourceName),
resource.TestMatchResourceAttr(resourceName, "arn", regexp.MustCompile(fmt.Sprintf("^arn:[^:]+:kms:[^:]+:[^:]+:%s$", name))),
resource.TestCheckResourceAttr(resourceName, "name", name),
resource.TestCheckNoResourceAttr(resourceName, "target_key_arn"),
resource.TestCheckNoResourceAttr(resourceName, "target_key_id"),
),
},
},
})
}

func TestAccDataSourceAwsKmsAlias_CMK(t *testing.T) {
rInt := acctest.RandInt()
aliasResourceName := "aws_kms_alias.test"
datasourceAliasResourceName := "data.aws_kms_alias.test"

resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
Steps: []resource.TestStep{
resource.TestStep{
Config: testAccDataSourceAwsKmsAlias(rInt),
Config: testAccDataSourceAwsKmsAlias_CMK(rInt),
Check: resource.ComposeTestCheckFunc(
testAccDataSourceAwsKmsAliasCheck("data.aws_kms_alias.by_name"),
testAccDataSourceAwsKmsAliasCheckExists(datasourceAliasResourceName),
testAccDataSourceAwsKmsAliasCheckCMKAttributes(aliasResourceName, datasourceAliasResourceName),
),
},
},
})
}

func testAccDataSourceAwsKmsAliasCheck(name string) resource.TestCheckFunc {
func testAccDataSourceAwsKmsAliasCheckExists(name string) resource.TestCheckFunc {
return func(s *terraform.State) error {
rs, ok := s.RootModule().Resources[name]
_, ok := s.RootModule().Resources[name]
if !ok {
return fmt.Errorf("root module has no resource called %s", name)
}

kmsKeyRs, ok := s.RootModule().Resources["aws_kms_alias.single"]
return nil
}
}

func testAccDataSourceAwsKmsAliasCheckCMKAttributes(aliasResourceName, datasourceAliasResourceName string) resource.TestCheckFunc {
return func(s *terraform.State) error {
rs, ok := s.RootModule().Resources[datasourceAliasResourceName]
if !ok {
return fmt.Errorf("root module has no resource called %s", datasourceAliasResourceName)
}

kmsKeyRs, ok := s.RootModule().Resources[aliasResourceName]
if !ok {
return fmt.Errorf("can't find aws_kms_alias.single in state")
return fmt.Errorf("can't find %s in state", aliasResourceName)
}

attr := rs.Primary.Attributes
Expand Down Expand Up @@ -69,19 +107,25 @@ func testAccDataSourceAwsKmsAliasCheck(name string) resource.TestCheckFunc {
}
}

func testAccDataSourceAwsKmsAlias(rInt int) string {
func testAccDataSourceAwsKmsAlias_name(name string) string {
return fmt.Sprintf(`
data "aws_kms_alias" "test" {
name = "%s"
}`, name)
}

func testAccDataSourceAwsKmsAlias_CMK(rInt int) string {
return fmt.Sprintf(`
resource "aws_kms_key" "one" {
resource "aws_kms_key" "test" {
description = "Terraform acc test"
deletion_window_in_days = 7
}

resource "aws_kms_alias" "single" {
resource "aws_kms_alias" "test" {
name = "alias/tf-acc-key-alias-%d"
target_key_id = "${aws_kms_key.one.key_id}"
target_key_id = "${aws_kms_key.test.key_id}"
}

data "aws_kms_alias" "by_name" {
name = "${aws_kms_alias.single.name}"
}`, rInt)
%s
`, rInt, testAccDataSourceAwsKmsAlias_name("${aws_kms_alias.test.name}"))
}