From 8ddc68b648f16fe4a1096745cc2cd4b6fb7833bb Mon Sep 17 00:00:00 2001 From: Jack Batzner Date: Wed, 31 Mar 2021 11:00:15 -0500 Subject: [PATCH 01/13] Write test to confirm bug #18446 --- ...urce_aws_lakeformation_permissions_test.go | 94 +++++++++++++++++++ aws/resource_aws_lakeformation_test.go | 3 +- 2 files changed, 96 insertions(+), 1 deletion(-) diff --git a/aws/resource_aws_lakeformation_permissions_test.go b/aws/resource_aws_lakeformation_permissions_test.go index 2f0c3ccd85b..31f437b3d08 100644 --- a/aws/resource_aws_lakeformation_permissions_test.go +++ b/aws/resource_aws_lakeformation_permissions_test.go @@ -238,6 +238,30 @@ func testAccAWSLakeFormationPermissions_selectPermissions(t *testing.T) { }) } +func testAccAWSLakeFormationPermissions_wildcardPermissions(t *testing.T) { + rName := acctest.RandomWithPrefix("tf-acc-test") + resourceName := "aws_lakeformation_permissions.test" + roleName := "aws_iam_role.test" + + resource.Test(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t); testAccPartitionHasServicePreCheck(lakeformation.EndpointsID, t) }, + ErrorCheck: testAccErrorCheck(t, lakeformation.EndpointsID), + Providers: testAccProviders, + CheckDestroy: testAccCheckAWSLakeFormationPermissionsDestroy, + Steps: []resource.TestStep{ + { + Config: testAccAWSLakeFormationPermissionsConfig_wildcardPermissions(rName), + Check: resource.ComposeTestCheckFunc( + testAccCheckAWSLakeFormationPermissionsExists(resourceName), + resource.TestCheckResourceAttrPair(resourceName, "principal", roleName, "arn"), + resource.TestCheckResourceAttr(resourceName, "permissions.#", "7"), + resource.TestCheckResourceAttr(resourceName, "permissions_with_grant_option.#", "7"), + ), + }, + }, + }) +} + func testAccCheckAWSLakeFormationPermissionsDestroy(s *terraform.State) error { conn := testAccProvider.Meta().(*AWSClient).lakeformationconn @@ -835,3 +859,73 @@ resource "aws_lakeformation_permissions" "test" { } `, rName) } + +func testAccAWSLakeFormationPermissionsConfig_wildcardPermissions(rName string) string { + return fmt.Sprintf(` +data "aws_partition" "current" {} + +resource "aws_iam_role" "test" { + name = %[1]q + path = "/" + + assume_role_policy = < Date: Wed, 31 Mar 2021 11:44:18 -0500 Subject: [PATCH 02/13] Don't throw error when no permissions exist --- .changelog/18505.txt | 3 +++ aws/resource_aws_lakeformation_permissions.go | 3 ++- aws/resource_aws_lakeformation_permissions_test.go | 2 +- aws/resource_aws_lakeformation_test.go | 6 +++--- 4 files changed, 9 insertions(+), 5 deletions(-) create mode 100644 .changelog/18505.txt diff --git a/.changelog/18505.txt b/.changelog/18505.txt new file mode 100644 index 00000000000..4f277694495 --- /dev/null +++ b/.changelog/18505.txt @@ -0,0 +1,3 @@ +```release-note:bug + resource/aws_lakeformation_permissions: aws_lakeformation_permissions throwing "no permissions found" for wildcard table + ``` \ No newline at end of file diff --git a/aws/resource_aws_lakeformation_permissions.go b/aws/resource_aws_lakeformation_permissions.go index 1a4ed699153..a68fc50339a 100644 --- a/aws/resource_aws_lakeformation_permissions.go +++ b/aws/resource_aws_lakeformation_permissions.go @@ -325,7 +325,8 @@ func resourceAwsLakeFormationPermissionsRead(d *schema.ResourceData, meta interf } if len(principalResourcePermissions) == 0 { - return fmt.Errorf("error reading Lake Formation permissions: %s", "no permissions found") + log.Printf("[INFO] No Lake Formation permissions found") + return nil } if len(principalResourcePermissions) > 2 { diff --git a/aws/resource_aws_lakeformation_permissions_test.go b/aws/resource_aws_lakeformation_permissions_test.go index 31f437b3d08..7affaf15b2a 100644 --- a/aws/resource_aws_lakeformation_permissions_test.go +++ b/aws/resource_aws_lakeformation_permissions_test.go @@ -924,7 +924,7 @@ resource "aws_lakeformation_permissions" "test" { table { database_name = aws_glue_catalog_table.test.database_name - wildcard = true + wildcard = true } } `, rName) diff --git a/aws/resource_aws_lakeformation_test.go b/aws/resource_aws_lakeformation_test.go index 06ed7210893..82a2f0b2c91 100644 --- a/aws/resource_aws_lakeformation_test.go +++ b/aws/resource_aws_lakeformation_test.go @@ -13,9 +13,9 @@ func TestAccAWSLakeFormation_serial(t *testing.T) { "dataSource": testAccAWSLakeFormationDataLakeSettingsDataSource_basic, }, "Permissions": { - "basic": testAccAWSLakeFormationPermissions_basic, - "dataLocation": testAccAWSLakeFormationPermissions_dataLocation, - "database": testAccAWSLakeFormationPermissions_database, + "basic": testAccAWSLakeFormationPermissions_basic, + "dataLocation": testAccAWSLakeFormationPermissions_dataLocation, + "database": testAccAWSLakeFormationPermissions_database, "selectPermissions": testAccAWSLakeFormationPermissions_selectPermissions, "wildcardPermissions": testAccAWSLakeFormationPermissions_wildcardPermissions, }, From 68dee8e5bd766b666fb3c1eecf1cbf79fadb22a6 Mon Sep 17 00:00:00 2001 From: Dirk Avery Date: Tue, 4 May 2021 19:10:25 -0400 Subject: [PATCH 03/13] utils: Add slice comparison helpers --- aws/utils.go | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/aws/utils.go b/aws/utils.go index 9c0490769d5..d96e6022e8a 100644 --- a/aws/utils.go +++ b/aws/utils.go @@ -5,7 +5,9 @@ import ( "encoding/json" "reflect" "regexp" + "sort" + "github.com/aws/aws-sdk-go/aws" "github.com/terraform-providers/terraform-provider-aws/aws/internal/tfresource" ) @@ -59,3 +61,28 @@ func appendUniqueString(slice []string, elem string) []string { } return append(slice, elem) } + +func StringSlicesEqualIgnoreOrder(s1, s2 []*string) bool { + if len(s1) != len(s2) { + return false + } + + v1 := aws.StringValueSlice(s1) + v2 := aws.StringValueSlice(s2) + + sort.Strings(v1) + sort.Strings(v2) + + return reflect.DeepEqual(v1, v2) +} + +func StringSlicesEqual(s1, s2 []*string) bool { + if len(s1) != len(s2) { + return false + } + + v1 := aws.StringValueSlice(s1) + v2 := aws.StringValueSlice(s2) + + return reflect.DeepEqual(v1, v2) +} From 35b253fd165b4c4eb9818bc73a64c166708a8aec Mon Sep 17 00:00:00 2001 From: Dirk Avery Date: Tue, 4 May 2021 19:10:49 -0400 Subject: [PATCH 04/13] tests/utils: Test slice comparison helpers --- aws/utils_test.go | 114 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 114 insertions(+) diff --git a/aws/utils_test.go b/aws/utils_test.go index 785ad141ed3..53107fad89c 100644 --- a/aws/utils_test.go +++ b/aws/utils_test.go @@ -2,6 +2,8 @@ package aws import ( "testing" + + "github.com/aws/aws-sdk-go/aws" ) var base64encodingTests = []struct { @@ -72,3 +74,115 @@ func TestJsonBytesEqualWhitespaceAndNoWhitespace(t *testing.T) { t.Errorf("Expected jsonBytesEqual to return false for %s == %s", noWhitespaceDiff, whitespaceDiff) } } + +func TestStringSlicesEqualIgnoreOrder(t *testing.T) { + equal := []interface{}{ + []interface{}{ + []string{"a", "b", "c"}, + []string{"a", "b", "c"}, + }, + []interface{}{ + []string{"b", "a", "c"}, + []string{"a", "b", "c"}, + }, + []interface{}{ + []string{"apple", "carrot", "tomato"}, + []string{"tomato", "apple", "carrot"}, + }, + []interface{}{ + []string{"Application", "Barrier", "Chilly", "Donut"}, + []string{"Barrier", "Application", "Donut", "Chilly"}, + }, + } + for _, v := range equal { + if !StringSlicesEqualIgnoreOrder(aws.StringSlice(v.([]interface{})[0].([]string)), aws.StringSlice(v.([]interface{})[1].([]string))) { + t.Fatalf("%v should be equal: %v", v.([]interface{})[0].([]string), v.([]interface{})[1].([]string)) + } + } + + notEqual := []interface{}{ + []interface{}{ + []string{"c", "b", "c"}, + []string{"a", "b", "c"}, + }, + []interface{}{ + []string{"b", "a", "c"}, + []string{"a", "bread", "c"}, + }, + []interface{}{ + []string{"apple", "carrot", "tomato"}, + []string{"tomato", "apple"}, + }, + []interface{}{ + []string{"Application", "Barrier", "Chilly", "Donut"}, + []string{"Barrier", "Applications", "Donut", "Chilly"}, + }, + []interface{}{ + []string{}, + []string{"Barrier", "Applications", "Donut", "Chilly"}, + }, + } + for _, v := range notEqual { + if StringSlicesEqualIgnoreOrder(aws.StringSlice(v.([]interface{})[0].([]string)), aws.StringSlice(v.([]interface{})[1].([]string))) { + t.Fatalf("%v should not be equal: %v", v.([]interface{})[0].([]string), v.([]interface{})[1].([]string)) + } + } +} + +func TestStringSlicesEqual(t *testing.T) { + equal := []interface{}{ + []interface{}{ + []string{"a", "b", "c"}, + []string{"a", "b", "c"}, + }, + []interface{}{ + []string{"b", "a", "c"}, + []string{"b", "a", "c"}, + }, + []interface{}{ + []string{"apple", "carrot", "tomato"}, + []string{"apple", "carrot", "tomato"}, + }, + []interface{}{ + []string{"Application", "Barrier", "Chilly", "Donut"}, + []string{"Application", "Barrier", "Chilly", "Donut"}, + }, + []interface{}{ + []string{}, + []string{}, + }, + } + for _, v := range equal { + if !StringSlicesEqual(aws.StringSlice(v.([]interface{})[0].([]string)), aws.StringSlice(v.([]interface{})[1].([]string))) { + t.Fatalf("%v should be equal: %v", v.([]interface{})[0].([]string), v.([]interface{})[1].([]string)) + } + } + + notEqual := []interface{}{ + []interface{}{ + []string{"a", "b", "c"}, + []string{"a", "b"}, + }, + []interface{}{ + []string{"a", "b", "c"}, + []string{"b", "a", "c"}, + }, + []interface{}{ + []string{"apple", "carrot", "tomato"}, + []string{"apple", "carrot", "tomato", "zucchini"}, + }, + []interface{}{ + []string{"Application", "Barrier", "Chilly", "Donut"}, + []string{"Application", "Barrier", "Chilly", "Donuts"}, + }, + []interface{}{ + []string{}, + []string{"Application", "Barrier", "Chilly", "Donuts"}, + }, + } + for _, v := range notEqual { + if StringSlicesEqual(aws.StringSlice(v.([]interface{})[0].([]string)), aws.StringSlice(v.([]interface{})[1].([]string))) { + t.Fatalf("%v should not be equal: %v", v.([]interface{})[0].([]string), v.([]interface{})[1].([]string)) + } + } +} From dc9cb7df9812c49011c3b2207765186606fb17bb Mon Sep 17 00:00:00 2001 From: Dirk Avery Date: Tue, 4 May 2021 19:11:20 -0400 Subject: [PATCH 05/13] docs/r/lakeformation_permissions: Revise docs --- .../r/lakeformation_permissions.html.markdown | 21 ++++++++----------- 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/website/docs/r/lakeformation_permissions.html.markdown b/website/docs/r/lakeformation_permissions.html.markdown index ecfa0cd71ad..4d824c32c6e 100644 --- a/website/docs/r/lakeformation_permissions.html.markdown +++ b/website/docs/r/lakeformation_permissions.html.markdown @@ -10,7 +10,7 @@ description: |- Grants permissions to the principal to access metadata in the Data Catalog and data organized in underlying data storage such as Amazon S3. Permissions are granted to a principal, in a Data Catalog, relative to a Lake Formation resource, which includes the Data Catalog, databases, and tables. For more information, see [Security and Access Control to Metadata and Data in Lake Formation](https://docs.aws.amazon.com/lake-formation/latest/dg/security-data-access.html). -~> **NOTE:** This resource deals with explicitly granted permissions. Lake Formation grants implicit permissions to data lake administrators, database creators, and table creators. For more information, see [Implicit Lake Formation Permissions](https://docs.aws.amazon.com/lake-formation/latest/dg/implicit-permissions.html). +~> **NOTE:** Lake Formation grants implicit permissions to data lake administrators, database creators, and table creators. These implicit permissions cannot be revoked _per se_. If this resource reads implicit permissions, it will attempt to revoke them, which causes an error when the resource is destroyed. There are two ways to avoid these errors. First, grant explicit permissions (and `permissions_with_grant_option`) to "overwrite" a principal's implicit permissions, which you can then revoke with this resource. Second, avoid using this resource with principals that have implicit permissions. For more information, see [Implicit Lake Formation Permissions](https://docs.aws.amazon.com/lake-formation/latest/dg/implicit-permissions.html). ## Example Usage @@ -46,7 +46,9 @@ resource "aws_lakeformation_permissions" "test" { The following arguments are required: * `permissions` – (Required) List of permissions granted to the principal. Valid values may include `ALL`, `ALTER`, `CREATE_DATABASE`, `CREATE_TABLE`, `DATA_LOCATION_ACCESS`, `DELETE`, `DESCRIBE`, `DROP`, `INSERT`, and `SELECT`. For details on each permission, see [Lake Formation Permissions Reference](https://docs.aws.amazon.com/lake-formation/latest/dg/lf-permissions-reference.html). -* `principal` – (Required) Principal to be granted the permissions on the resource. Supported principals include IAM roles, users, groups, OUs, and organizations as well as AWS account IDs for cross-account permissions. For more information, see [Lake Formation Permissions Reference](https://docs.aws.amazon.com/lake-formation/latest/dg/lf-permissions-reference.html). +* `principal` – (Required) Principal to be granted the permissions on the resource. Supported principals include IAM roles, users, groups, SAML groups and users, QuickSight groups, OUs, and organizations as well as AWS account IDs for cross-account permissions. For more information, see [Lake Formation Permissions Reference](https://docs.aws.amazon.com/lake-formation/latest/dg/lf-permissions-reference.html). + +~> **NOTE:** If the `principal` is also a data lake administrator, AWS grants implicit permissions that can cause errors using this resource. For example, AWS implicitly grants a `principal`/administrator `permissions` and `permissions_with_grant_option` of `ALL`, `ALTER`, `DELETE`, `DESCRIBE`, `DROP`, `INSERT`, and `SELECT` on a table. If you use this resource to explicitly grant the `principal`/administrator `permissions` but _not_ `permissions_with_grant_option` of `ALL`, `ALTER`, `DELETE`, `DESCRIBE`, `DROP`, `INSERT`, and `SELECT` on the table, this resource will read the implicit `permissions_with_grant_option` and attempt to revoke them when the resource is destroyed. Doing so will cause an `InvalidInputException: No permissions revoked` error because you cannot revoke implicit permissions _per se_. To workaround this problem, explicitly grant the `principal`/administrator `permissions` _and_ `permissions_with_grant_option`, which can then be revoked. Similarly, granting a `principal`/administrator permissions on a table with columns and providing `column_names`, will result in a `InvalidInputException: Permissions modification is invalid` error because you are narrowing the implicit permissions. Instead, set `wildcard` to `true` and remove the `column_names`. One of the following is required: @@ -86,11 +88,8 @@ The following argument is optional: The following argument is required: * `database_name` – (Required) Name of the database for the table. Unique to a Data Catalog. - -At least one of the following is required: - -* `name` - (Optional) Name of the table. -* `wildcard` - (Optional) Whether to use a wildcard representing every table under a database. Defaults to `false`. +* `name` - (Required, at least one of `name` or `wildcard`) Name of the table. +* `wildcard` - (Required, at least one of `name` or `wildcard`) Whether to use a wildcard representing every table under a database. Defaults to `false`. The following arguments are optional: @@ -100,17 +99,15 @@ The following arguments are optional: The following arguments are required: +* `column_names` - (Required, at least one of `column_names` or `wildcard`) List of column names for the table. * `database_name` – (Required) Name of the database for the table with columns resource. Unique to the Data Catalog. * `name` – (Required) Name of the table resource. - -At least one of the following is required: - -* `column_names` - (Optional) List of column names for the table. -* `excluded_column_names` - (Optional) List of column names for the table to exclude. +* `wildcard` - (Required, at least one of `column_names` or `wildcard`) Whether to use a column wildcard. If `excluded_column_names` is included, `wildcard` must be set to `true` to avoid Terraform reporting a difference. The following arguments are optional: * `catalog_id` - (Optional) Identifier for the Data Catalog. By default, it is the account ID of the caller. +* `excluded_column_names` - (Optional) List of column names for the table to exclude. If `excluded_column_names` is included, `wildcard` must be set to `true` to avoid Terraform reporting a difference. ## Attributes Reference From f21b343b3967d85f813c4c24b44572b7020f822e Mon Sep 17 00:00:00 2001 From: Dirk Avery Date: Tue, 4 May 2021 19:11:52 -0400 Subject: [PATCH 06/13] tests/lakeformation: Add new tests --- aws/resource_aws_lakeformation_test.go | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/aws/resource_aws_lakeformation_test.go b/aws/resource_aws_lakeformation_test.go index 82a2f0b2c91..11a8488e00b 100644 --- a/aws/resource_aws_lakeformation_test.go +++ b/aws/resource_aws_lakeformation_test.go @@ -12,18 +12,20 @@ func TestAccAWSLakeFormation_serial(t *testing.T) { "withoutCatalogId": testAccAWSLakeFormationDataLakeSettings_withoutCatalogId, "dataSource": testAccAWSLakeFormationDataLakeSettingsDataSource_basic, }, - "Permissions": { - "basic": testAccAWSLakeFormationPermissions_basic, - "dataLocation": testAccAWSLakeFormationPermissions_dataLocation, - "database": testAccAWSLakeFormationPermissions_database, - "selectPermissions": testAccAWSLakeFormationPermissions_selectPermissions, - "wildcardPermissions": testAccAWSLakeFormationPermissions_wildcardPermissions, + "BasicPermissions": { + "basic": testAccAWSLakeFormationPermissions_basic, + "dataLocation": testAccAWSLakeFormationPermissions_dataLocation, + "database": testAccAWSLakeFormationPermissions_database, }, "TablePermissions": { - "tableName": testAccAWSLakeFormationPermissions_table_name, - "tableWildcard": testAccAWSLakeFormationPermissions_table_wildcard, - "tableWithColumns": testAccAWSLakeFormationPermissions_tableWithColumns, - "tableWithColumnsAndTable": testAccAWSLakeFormationPermissions_tableWithColumnsAndTable, + "columnWildcardPermissions": testAccAWSLakeFormationPermissions_columnWildcardPermissions, + "implicitTableWithColumnsPermissions": testAccAWSLakeFormationPermissions_implicitTableWithColumnsPermissions, + "implicitTablePermissions": testAccAWSLakeFormationPermissions_implicitTablePermissions, + "selectPermissions": testAccAWSLakeFormationPermissions_selectPermissions, + "tableName": testAccAWSLakeFormationPermissions_tableName, + "tableWildcard": testAccAWSLakeFormationPermissions_tableWildcard, + "tableWildcardPermissions": testAccAWSLakeFormationPermissions_tableWildcardPermissions, + "tableWithColumns": testAccAWSLakeFormationPermissions_tableWithColumns, }, "DataSourcePermissions": { "basicDataSource": testAccAWSLakeFormationPermissionsDataSource_basic, From 1b207788552c8af85ee6cb42c9098135d243645b Mon Sep 17 00:00:00 2001 From: Dirk Avery Date: Tue, 4 May 2021 19:13:09 -0400 Subject: [PATCH 07/13] tests/r/lakeformation_permissions: Rework tests --- ...urce_aws_lakeformation_permissions_test.go | 559 ++++++++++++++---- 1 file changed, 433 insertions(+), 126 deletions(-) diff --git a/aws/resource_aws_lakeformation_permissions_test.go b/aws/resource_aws_lakeformation_permissions_test.go index 7affaf15b2a..67a65be96ff 100644 --- a/aws/resource_aws_lakeformation_permissions_test.go +++ b/aws/resource_aws_lakeformation_permissions_test.go @@ -2,14 +2,18 @@ package aws import ( "fmt" + "log" + "strconv" "testing" - "time" "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/lakeformation" + "github.com/hashicorp/aws-sdk-go-base/tfawserr" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/acctest" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource" "github.com/hashicorp/terraform-plugin-sdk/v2/terraform" + iamwaiter "github.com/terraform-providers/terraform-provider-aws/aws/internal/service/iam/waiter" + "github.com/terraform-providers/terraform-provider-aws/aws/internal/tfresource" ) func testAccAWSLakeFormationPermissions_basic(t *testing.T) { @@ -98,7 +102,7 @@ func testAccAWSLakeFormationPermissions_database(t *testing.T) { }) } -func testAccAWSLakeFormationPermissions_table_name(t *testing.T) { +func testAccAWSLakeFormationPermissions_tableName(t *testing.T) { rName := acctest.RandomWithPrefix("tf-acc-test") resourceName := "aws_lakeformation_permissions.test" roleName := "aws_iam_role.test" @@ -111,7 +115,7 @@ func testAccAWSLakeFormationPermissions_table_name(t *testing.T) { CheckDestroy: testAccCheckAWSLakeFormationPermissionsDestroy, Steps: []resource.TestStep{ { - Config: testAccAWSLakeFormationPermissionsConfig_table_name(rName), + Config: testAccAWSLakeFormationPermissionsConfig_tableName(rName), Check: resource.ComposeTestCheckFunc( testAccCheckAWSLakeFormationPermissionsExists(resourceName), resource.TestCheckResourceAttrPair(roleName, "arn", resourceName, "principal"), @@ -128,7 +132,7 @@ func testAccAWSLakeFormationPermissions_table_name(t *testing.T) { }) } -func testAccAWSLakeFormationPermissions_table_wildcard(t *testing.T) { +func testAccAWSLakeFormationPermissions_tableWildcard(t *testing.T) { rName := acctest.RandomWithPrefix("tf-acc-test") resourceName := "aws_lakeformation_permissions.test" databaseResourceName := "aws_glue_catalog_database.test" @@ -140,7 +144,7 @@ func testAccAWSLakeFormationPermissions_table_wildcard(t *testing.T) { CheckDestroy: testAccCheckAWSLakeFormationPermissionsDestroy, Steps: []resource.TestStep{ { - Config: testAccAWSLakeFormationPermissionsConfig_table_wildcard(rName), + Config: testAccAWSLakeFormationPermissionsConfig_tableWildcard(rName), Check: resource.ComposeTestCheckFunc( testAccCheckAWSLakeFormationPermissionsExists(resourceName), resource.TestCheckResourceAttr(resourceName, "table.#", "1"), @@ -183,7 +187,7 @@ func testAccAWSLakeFormationPermissions_tableWithColumns(t *testing.T) { }) } -func testAccAWSLakeFormationPermissions_tableWithColumnsAndTable(t *testing.T) { +func testAccAWSLakeFormationPermissions_implicitTableWithColumnsPermissions(t *testing.T) { rName := acctest.RandomWithPrefix("tf-acc-test") resourceName := "aws_lakeformation_permissions.test" roleName := "aws_iam_role.test" @@ -196,18 +200,44 @@ func testAccAWSLakeFormationPermissions_tableWithColumnsAndTable(t *testing.T) { CheckDestroy: testAccCheckAWSLakeFormationPermissionsDestroy, Steps: []resource.TestStep{ { - Config: testAccAWSLakeFormationPermissionsConfig_tableWithColumnsAndTable(rName), + Config: testAccAWSLakeFormationPermissionsConfig_implicitTableWithColumnsPermissions(rName), Check: resource.ComposeTestCheckFunc( testAccCheckAWSLakeFormationPermissionsExists(resourceName), resource.TestCheckResourceAttrPair(resourceName, "principal", roleName, "arn"), resource.TestCheckResourceAttr(resourceName, "table_with_columns.#", "1"), resource.TestCheckResourceAttrPair(resourceName, "table_with_columns.0.database_name", tableName, "database_name"), resource.TestCheckResourceAttrPair(resourceName, "table_with_columns.0.name", tableName, "name"), - resource.TestCheckResourceAttr(resourceName, "table_with_columns.0.column_names.#", "2"), - resource.TestCheckResourceAttr(resourceName, "table_with_columns.0.column_names.0", "event"), - resource.TestCheckResourceAttr(resourceName, "table_with_columns.0.column_names.1", "timestamp"), - resource.TestCheckResourceAttr(resourceName, "permissions.#", "1"), - resource.TestCheckResourceAttr(resourceName, "permissions.0", lakeformation.PermissionSelect), + resource.TestCheckResourceAttr(resourceName, "table_with_columns.0.wildcard", "true"), + resource.TestCheckResourceAttr(resourceName, "permissions.#", "7"), + resource.TestCheckResourceAttr(resourceName, "permissions_with_grant_option.#", "7"), + ), + }, + }, + }) +} + +func testAccAWSLakeFormationPermissions_implicitTablePermissions(t *testing.T) { + rName := acctest.RandomWithPrefix("tf-acc-test") + resourceName := "aws_lakeformation_permissions.test" + roleName := "aws_iam_role.test" + tableName := "aws_glue_catalog_table.test" + + resource.Test(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t); testAccPartitionHasServicePreCheck(lakeformation.EndpointsID, t) }, + ErrorCheck: testAccErrorCheck(t, lakeformation.EndpointsID), + Providers: testAccProviders, + CheckDestroy: testAccCheckAWSLakeFormationPermissionsDestroy, + Steps: []resource.TestStep{ + { + Config: testAccAWSLakeFormationPermissionsConfig_implicitTablePermissions(rName), + Check: resource.ComposeTestCheckFunc( + testAccCheckAWSLakeFormationPermissionsExists(resourceName), + resource.TestCheckResourceAttrPair(resourceName, "principal", roleName, "arn"), + resource.TestCheckResourceAttr(resourceName, "table.#", "1"), + resource.TestCheckResourceAttrPair(resourceName, "table.0.database_name", tableName, "database_name"), + resource.TestCheckResourceAttrPair(resourceName, "table.0.name", tableName, "name"), + resource.TestCheckResourceAttr(resourceName, "permissions.#", "7"), + resource.TestCheckResourceAttr(resourceName, "permissions_with_grant_option.#", "7"), ), }, }, @@ -238,7 +268,7 @@ func testAccAWSLakeFormationPermissions_selectPermissions(t *testing.T) { }) } -func testAccAWSLakeFormationPermissions_wildcardPermissions(t *testing.T) { +func testAccAWSLakeFormationPermissions_tableWildcardPermissions(t *testing.T) { rName := acctest.RandomWithPrefix("tf-acc-test") resourceName := "aws_lakeformation_permissions.test" roleName := "aws_iam_role.test" @@ -250,7 +280,7 @@ func testAccAWSLakeFormationPermissions_wildcardPermissions(t *testing.T) { CheckDestroy: testAccCheckAWSLakeFormationPermissionsDestroy, Steps: []resource.TestStep{ { - Config: testAccAWSLakeFormationPermissionsConfig_wildcardPermissions(rName), + Config: testAccAWSLakeFormationPermissionsConfig_tableWildcardPermissions(rName), Check: resource.ComposeTestCheckFunc( testAccCheckAWSLakeFormationPermissionsExists(resourceName), resource.TestCheckResourceAttrPair(resourceName, "principal", roleName, "arn"), @@ -262,6 +292,30 @@ func testAccAWSLakeFormationPermissions_wildcardPermissions(t *testing.T) { }) } +func testAccAWSLakeFormationPermissions_columnWildcardPermissions(t *testing.T) { + rName := acctest.RandomWithPrefix("tf-acc-test") + resourceName := "aws_lakeformation_permissions.test" + roleName := "aws_iam_role.test" + + resource.Test(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t); testAccPartitionHasServicePreCheck(lakeformation.EndpointsID, t) }, + ErrorCheck: testAccErrorCheck(t, lakeformation.EndpointsID), + Providers: testAccProviders, + CheckDestroy: testAccCheckAWSLakeFormationPermissionsDestroy, + Steps: []resource.TestStep{ + { + Config: testAccAWSLakeFormationPermissionsConfig_columnWildcardPermissions(rName), + Check: resource.ComposeTestCheckFunc( + testAccCheckAWSLakeFormationPermissionsExists(resourceName), + resource.TestCheckResourceAttrPair(resourceName, "principal", roleName, "arn"), + resource.TestCheckResourceAttr(resourceName, "permissions.#", "7"), + resource.TestCheckResourceAttr(resourceName, "permissions_with_grant_option.#", "0"), + ), + }, + }, + }) +} + func testAccCheckAWSLakeFormationPermissionsDestroy(s *terraform.State) error { conn := testAccProvider.Meta().(*AWSClient).lakeformationconn @@ -270,20 +324,17 @@ func testAccCheckAWSLakeFormationPermissionsDestroy(s *terraform.State) error { continue } - principal := rs.Primary.Attributes["principal"] - catalogId := rs.Primary.Attributes["catalog_id"] + permCount, err := permissionCountForLakeFormationResource(conn, rs) - input := &lakeformation.ListPermissionsInput{ - CatalogId: aws.String(catalogId), - Principal: &lakeformation.DataLakePrincipal{ - DataLakePrincipalIdentifier: aws.String(principal), - }, + if err != nil { + return fmt.Errorf("error listing Lake Formation permissions (%s): %w", rs.Primary.ID, err) } - _, err := conn.ListPermissions(input) - if err == nil { - return fmt.Errorf("Resource still registered: %s %s", catalogId, principal) + if permCount != 0 { + return fmt.Errorf("Lake Formation permissions (%s) still exist: %d", rs.Primary.ID, permCount) } + + return nil } return nil @@ -298,118 +349,209 @@ func testAccCheckAWSLakeFormationPermissionsExists(resourceName string) resource conn := testAccProvider.Meta().(*AWSClient).lakeformationconn - input := &lakeformation.ListPermissionsInput{ - MaxResults: aws.Int64(1), - Principal: &lakeformation.DataLakePrincipal{ - DataLakePrincipalIdentifier: aws.String(rs.Primary.Attributes["principal"]), - }, + permCount, err := permissionCountForLakeFormationResource(conn, rs) + + if err != nil { + return fmt.Errorf("error listing Lake Formation permissions (%s): %w", rs.Primary.ID, err) } - if v, ok := rs.Primary.Attributes["catalog_resource"]; ok && v != "" && v == "true" { - input.ResourceType = aws.String(lakeformation.DataLakeResourceTypeCatalog) - input.Resource = &lakeformation.Resource{ - Catalog: &lakeformation.CatalogResource{}, - } + if permCount == 0 { + return fmt.Errorf("Lake Formation permissions (%s) do not exist or could not be found", rs.Primary.ID) } - if v, ok := rs.Primary.Attributes["data_location.#"]; ok && v != "" && v != "0" { - input.ResourceType = aws.String(lakeformation.DataLakeResourceTypeDataLocation) - res := &lakeformation.DataLocationResource{ - ResourceArn: aws.String(rs.Primary.Attributes["data_location.0.arn"]), - } - if rs.Primary.Attributes["data_location.0.catalog_id"] != "" { - res.CatalogId = aws.String(rs.Primary.Attributes["data_location.0.catalog_id"]) - } - input.Resource = &lakeformation.Resource{ - DataLocation: res, - } + return nil + } +} + +func permissionCountForLakeFormationResource(conn *lakeformation.LakeFormation, rs *terraform.ResourceState) (int, error) { + input := &lakeformation.ListPermissionsInput{ + Principal: &lakeformation.DataLakePrincipal{ + DataLakePrincipalIdentifier: aws.String(rs.Primary.Attributes["principal"]), + }, + Resource: &lakeformation.Resource{}, + } + + if v, ok := rs.Primary.Attributes["catalog_id"]; ok && v != "" { + input.CatalogId = aws.String(v) + } + + if v, ok := rs.Primary.Attributes["catalog_resource"]; ok && v != "" && v == "true" { + input.Resource.Catalog = expandLakeFormationCatalogResource() + } + + if v, ok := rs.Primary.Attributes["data_location.#"]; ok && v != "" && v != "0" { + tfMap := map[string]interface{}{} + + if v := rs.Primary.Attributes["data_location.0.catalog_id"]; v != "" { + tfMap["catalog_id"] = v } - if v, ok := rs.Primary.Attributes["database.#"]; ok && v != "" && v != "0" { - input.ResourceType = aws.String(lakeformation.DataLakeResourceTypeDatabase) - res := &lakeformation.DatabaseResource{ - Name: aws.String(rs.Primary.Attributes["database.0.name"]), - } - if rs.Primary.Attributes["database.0.catalog_id"] != "" { - res.CatalogId = aws.String(rs.Primary.Attributes["database.0.catalog_id"]) - } - input.Resource = &lakeformation.Resource{ - Database: res, - } + if v := rs.Primary.Attributes["data_location.0.arn"]; v != "" { + tfMap["arn"] = v } - if v, ok := rs.Primary.Attributes["table.#"]; ok && v != "" && v != "0" { - input.ResourceType = aws.String(lakeformation.DataLakeResourceTypeTable) - res := &lakeformation.TableResource{ - DatabaseName: aws.String(rs.Primary.Attributes["table.0.database_name"]), - } - if rs.Primary.Attributes["table.0.catalog_id"] != "" { - res.CatalogId = aws.String(rs.Primary.Attributes["table.0.catalog_id"]) - } - if rs.Primary.Attributes["table.0.name"] != "" { - res.Name = aws.String(rs.Primary.Attributes["table.0.name"]) - } - if rs.Primary.Attributes["table.0.wildcard"] == "true" { - res.TableWildcard = &lakeformation.TableWildcard{} - } - input.Resource = &lakeformation.Resource{ - Table: res, - } + input.Resource.DataLocation = expandLakeFormationDataLocationResource(tfMap) + } + + if v, ok := rs.Primary.Attributes["database.#"]; ok && v != "" && v != "0" { + tfMap := map[string]interface{}{} + + if v := rs.Primary.Attributes["database.0.catalog_id"]; v != "" { + tfMap["catalog_id"] = v } - // ListPermissions does not support getting privileges on a table with columns. - // Instead, call this operation on the table, and the operation returns the - // table and the table w columns. - // https://docs.aws.amazon.com/sdk-for-go/api/service/lakeformation/#ListPermissionsInput - if v, ok := rs.Primary.Attributes["table_with_columns.#"]; ok && v != "" && v != "0" { - input.ResourceType = aws.String(lakeformation.DataLakeResourceTypeTable) - res := &lakeformation.TableResource{ - DatabaseName: aws.String(rs.Primary.Attributes["table_with_columns.0.database_name"]), - Name: aws.String(rs.Primary.Attributes["table_with_columns.0.name"]), - } - if rs.Primary.Attributes["table.0.catalog_id"] != "" { - res.CatalogId = aws.String(rs.Primary.Attributes["table.0.catalog_id"]) + if v := rs.Primary.Attributes["database.0.name"]; v != "" { + tfMap["name"] = v + } + + input.Resource.Database = expandLakeFormationDatabaseResource(tfMap) + } + + tableType := "" + + if v, ok := rs.Primary.Attributes["table.#"]; ok && v != "" && v != "0" { + tableType = TableTypeTable + + tfMap := map[string]interface{}{} + + if v := rs.Primary.Attributes["table.0.catalog_id"]; v != "" { + tfMap["catalog_id"] = v + } + + if v := rs.Primary.Attributes["table.0.database_name"]; v != "" { + tfMap["database_name"] = v + } + + if v := rs.Primary.Attributes["table.0.name"]; v != "" && v != TableNameAllTables { + tfMap["name"] = v + } + + if v := rs.Primary.Attributes["table.0.wildcard"]; v != "" && v == "true" { + tfMap["wildcard"] = true + } + + input.Resource.Table = expandLakeFormationTableResource(tfMap) + } + + if v, ok := rs.Primary.Attributes["table_with_columns.#"]; ok && v != "" && v != "0" { + tableType = TableTypeTableWithColumns + + tfMap := map[string]interface{}{} + + if v := rs.Primary.Attributes["table_with_columns.0.catalog_id"]; v != "" { + tfMap["catalog_id"] = v + } + + if v := rs.Primary.Attributes["table_with_columns.0.database_name"]; v != "" { + tfMap["database_name"] = v + } + + if v := rs.Primary.Attributes["table_with_columns.0.name"]; v != "" { + tfMap["name"] = v + } + + input.Resource.Table = expandLakeFormationTableWithColumnsResourceAsTable(tfMap) + } + + log.Printf("[DEBUG] Reading Lake Formation permissions: %v", input) + var allPermissions []*lakeformation.PrincipalResourcePermissions + + err := resource.Retry(iamwaiter.PropagationTimeout, func() *resource.RetryError { + err := conn.ListPermissionsPages(input, func(resp *lakeformation.ListPermissionsOutput, lastPage bool) bool { + for _, permission := range resp.PrincipalResourcePermissions { + if permission == nil { + continue + } + + allPermissions = append(allPermissions, permission) } - input.Resource = &lakeformation.Resource{ - Table: res, + return !lastPage + }) + + if err != nil { + if tfawserr.ErrMessageContains(err, lakeformation.ErrCodeInvalidInputException, "Invalid principal") { + return resource.RetryableError(err) } + + return resource.NonRetryableError(fmt.Errorf("error listing Lake Formation Permissions: %w", err)) } + return nil + }) - err := resource.Retry(2*time.Minute, func() *resource.RetryError { - var err error - _, err = conn.ListPermissions(input) - if err != nil { - if isAWSErr(err, lakeformation.ErrCodeInvalidInputException, "Invalid principal") { - return resource.RetryableError(err) - } - if isAWSErr(err, lakeformation.ErrCodeInvalidInputException, "Grantee has no permissions") { - return resource.RetryableError(err) - } - if isAWSErr(err, lakeformation.ErrCodeInvalidInputException, "register the S3 path") { - return resource.RetryableError(err) - } - if isAWSErr(err, lakeformation.ErrCodeConcurrentModificationException, "") { - return resource.RetryableError(err) - } - if isAWSErr(err, "AccessDeniedException", "is not authorized to access requested permissions") { - return resource.RetryableError(err) + if tfresource.TimedOut(err) { + err = conn.ListPermissionsPages(input, func(resp *lakeformation.ListPermissionsOutput, lastPage bool) bool { + for _, permission := range resp.PrincipalResourcePermissions { + if permission == nil { + continue } - return resource.NonRetryableError(fmt.Errorf("unable to get Lake Formation Permissions: %w", err)) + allPermissions = append(allPermissions, permission) } - return nil + return !lastPage }) + } + + if tfawserr.ErrCodeEquals(err, lakeformation.ErrCodeEntityNotFoundException) { + return 0, nil + } + + if tfawserr.ErrMessageContains(err, "AccessDeniedException", "Resource does not exist") { + return 0, nil + } + + if err != nil { + return 0, fmt.Errorf("error listing Lake Formation permissions after retry: %w", err) + } + + // clean permissions = filter out permissions that do not pertain to this specific resource + + var cleanPermissions []*lakeformation.PrincipalResourcePermissions - if isResourceTimeoutError(err) { - _, err = conn.ListPermissions(input) + if input.Resource.Catalog != nil { + cleanPermissions = filterLakeFormationCatalogPermissions(allPermissions) + } + + if input.Resource.DataLocation != nil { + cleanPermissions = filterLakeFormationDataLocationPermissions(allPermissions) + } + + if input.Resource.Database != nil { + cleanPermissions = filterLakeFormationDatabasePermissions(allPermissions) + } + + if tableType == TableTypeTable { + cleanPermissions = filterLakeFormationTablePermissions( + aws.StringValue(input.Resource.Table.Name), + input.Resource.Table.TableWildcard != nil, + allPermissions, + ) + } + + var columnNames []string + if cols, err := strconv.Atoi(rs.Primary.Attributes["table_with_columns.0.column_names.#"]); err == nil { + for i := 0; i < cols; i++ { + columnNames = append(columnNames, rs.Primary.Attributes[fmt.Sprintf("table_with_columns.0.column_names.%d", i)]) } + } - if err != nil { - return fmt.Errorf("unable to get Lake Formation permissions (%s): %w", rs.Primary.ID, err) + var excludedColumnNames []string + if cols, err := strconv.Atoi(rs.Primary.Attributes["table_with_columns.0.excluded_column_names.#"]); err == nil { + for i := 0; i < cols; i++ { + excludedColumnNames = append(excludedColumnNames, rs.Primary.Attributes[fmt.Sprintf("table_with_columns.0.excluded_column_names.%d", i)]) } + } - return nil + if tableType == TableTypeTableWithColumns { + cleanPermissions = filterLakeFormationTableWithColumnsPermissions( + rs.Primary.Attributes["table_with_columns.0.database_name"], + rs.Primary.Attributes["table_with_columns.0.wildcard"] == "true", + aws.StringSlice(columnNames), + aws.StringSlice(excludedColumnNames), + allPermissions, + ) } + + return len(cleanPermissions), nil } func testAccAWSLakeFormationPermissionsConfig_basic(rName string) string { @@ -446,6 +588,9 @@ resource "aws_lakeformation_permissions" "test" { principal = aws_iam_role.test.arn permissions = ["CREATE_DATABASE"] catalog_resource = true + + # for consistency, ensure that admins are setup before testing + depends_on = [aws_lakeformation_data_lake_settings.test] } `, rName) } @@ -498,6 +643,9 @@ resource "aws_lakeformation_permissions" "test" { data_location { arn = aws_s3_bucket.test.arn } + + # for consistency, ensure that admins are setup before testing + depends_on = [aws_lakeformation_data_lake_settings.test] } `, rName) } @@ -546,12 +694,13 @@ resource "aws_lakeformation_permissions" "test" { name = aws_glue_catalog_database.test.name } - depends_on = ["aws_lakeformation_data_lake_settings.test"] + # for consistency, ensure that admins are setup before testing + depends_on = [aws_lakeformation_data_lake_settings.test] } `, rName) } -func testAccAWSLakeFormationPermissionsConfig_table_name(rName string) string { +func testAccAWSLakeFormationPermissionsConfig_tableName(rName string) string { return fmt.Sprintf(` data "aws_partition" "current" {} @@ -599,11 +748,14 @@ resource "aws_lakeformation_permissions" "test" { database_name = aws_glue_catalog_table.test.database_name name = aws_glue_catalog_table.test.name } + + # for consistency, ensure that admins are setup before testing + depends_on = [aws_lakeformation_data_lake_settings.test] } `, rName) } -func testAccAWSLakeFormationPermissionsConfig_table_wildcard(rName string) string { +func testAccAWSLakeFormationPermissionsConfig_tableWildcard(rName string) string { return fmt.Sprintf(` data "aws_partition" "current" {} @@ -647,6 +799,9 @@ resource "aws_lakeformation_permissions" "test" { database_name = aws_glue_catalog_database.test.name wildcard = true } + + # for consistency, ensure that admins are setup before testing + depends_on = [aws_lakeformation_data_lake_settings.test] } `, rName) } @@ -716,12 +871,13 @@ resource "aws_lakeformation_permissions" "test" { column_names = ["event", "timestamp"] } - depends_on = ["aws_lakeformation_data_lake_settings.test"] + # for consistency, ensure that admins are setup before testing + depends_on = [aws_lakeformation_data_lake_settings.test] } `, rName) } -func testAccAWSLakeFormationPermissionsConfig_tableWithColumnsAndTable(rName string) string { +func testAccAWSLakeFormationPermissionsConfig_implicitTableWithColumnsPermissions(rName string) string { return fmt.Sprintf(` data "aws_partition" "current" {} @@ -773,24 +929,28 @@ resource "aws_glue_catalog_table" "test" { } resource "aws_lakeformation_data_lake_settings" "test" { - # this will result in multiple permissions for iam role + # this will give the principal implicit permissions admins = [aws_iam_role.test.arn, data.aws_caller_identity.current.arn] } resource "aws_lakeformation_permissions" "test" { - permissions = ["SELECT"] - principal = aws_iam_role.test.arn + principal = aws_iam_role.test.arn + permissions = ["ALL", "ALTER", "DELETE", "DESCRIBE", "DROP", "INSERT", "SELECT"] + permissions_with_grant_option = ["ALL", "ALTER", "DELETE", "DESCRIBE", "DROP", "INSERT", "SELECT"] table_with_columns { database_name = aws_glue_catalog_table.test.database_name name = aws_glue_catalog_table.test.name - column_names = ["event", "timestamp"] + wildcard = true } + + # for consistency, ensure that admins are setup before testing + depends_on = [aws_lakeformation_data_lake_settings.test] } `, rName) } -func testAccAWSLakeFormationPermissionsConfig_selectPermissions(rName string) string { +func testAccAWSLakeFormationPermissionsConfig_implicitTablePermissions(rName string) string { return fmt.Sprintf(` data "aws_partition" "current" {} @@ -842,10 +1002,81 @@ resource "aws_glue_catalog_table" "test" { } resource "aws_lakeformation_data_lake_settings" "test" { - # this will result in multiple permissions for iam role + # this will give the principal implicit permissions admins = [aws_iam_role.test.arn, data.aws_caller_identity.current.arn] } +resource "aws_lakeformation_permissions" "test" { + principal = aws_iam_role.test.arn + permissions = ["ALL", "ALTER", "DELETE", "DESCRIBE", "DROP", "INSERT", "SELECT"] + permissions_with_grant_option = ["ALL", "ALTER", "DELETE", "DESCRIBE", "DROP", "INSERT", "SELECT"] + + table { + database_name = aws_glue_catalog_table.test.database_name + name = aws_glue_catalog_table.test.name + } + + # for consistency, ensure that admins are setup before testing + depends_on = [aws_lakeformation_data_lake_settings.test] +} +`, rName) +} + +func testAccAWSLakeFormationPermissionsConfig_selectPermissions(rName string) string { + return fmt.Sprintf(` +data "aws_partition" "current" {} + +resource "aws_iam_role" "test" { + name = %[1]q + path = "/" + + assume_role_policy = < Date: Tue, 4 May 2021 19:13:40 -0400 Subject: [PATCH 08/13] r/lakeformation_permissions: Rework permissions --- aws/resource_aws_lakeformation_permissions.go | 545 +++++++++++++----- 1 file changed, 395 insertions(+), 150 deletions(-) diff --git a/aws/resource_aws_lakeformation_permissions.go b/aws/resource_aws_lakeformation_permissions.go index a68fc50339a..23286da77b8 100644 --- a/aws/resource_aws_lakeformation_permissions.go +++ b/aws/resource_aws_lakeformation_permissions.go @@ -14,6 +14,7 @@ import ( "github.com/hashicorp/terraform-plugin-sdk/v2/helper/validation" "github.com/terraform-providers/terraform-provider-aws/aws/internal/hashcode" iamwaiter "github.com/terraform-providers/terraform-provider-aws/aws/internal/service/iam/waiter" + "github.com/terraform-providers/terraform-provider-aws/aws/internal/tfresource" ) func resourceAwsLakeFormationPermissions() *schema.Resource { @@ -34,13 +35,26 @@ func resourceAwsLakeFormationPermissions() *schema.Resource { Type: schema.TypeBool, Optional: true, Default: false, + ExactlyOneOf: []string{ + "catalog_resource", + "data_location", + "database", + "table", + "table_with_columns", + }, }, "data_location": { - Type: schema.TypeList, - Optional: true, - Computed: true, - MaxItems: 1, - ConflictsWith: []string{"database", "table", "table_with_columns"}, + Type: schema.TypeList, + Optional: true, + Computed: true, + MaxItems: 1, + ExactlyOneOf: []string{ + "catalog_resource", + "data_location", + "database", + "table", + "table_with_columns", + }, Elem: &schema.Resource{ Schema: map[string]*schema.Schema{ "arn": { @@ -58,11 +72,17 @@ func resourceAwsLakeFormationPermissions() *schema.Resource { }, }, "database": { - Type: schema.TypeList, - Optional: true, - Computed: true, - MaxItems: 1, - ConflictsWith: []string{"data_location", "table", "table_with_columns"}, + Type: schema.TypeList, + Optional: true, + Computed: true, + MaxItems: 1, + ExactlyOneOf: []string{ + "catalog_resource", + "data_location", + "database", + "table", + "table_with_columns", + }, Elem: &schema.Resource{ Schema: map[string]*schema.Schema{ "catalog_id": { @@ -105,11 +125,17 @@ func resourceAwsLakeFormationPermissions() *schema.Resource { ValidateFunc: validatePrincipal, }, "table": { - Type: schema.TypeList, - Optional: true, - Computed: true, - MaxItems: 1, - ConflictsWith: []string{"data_location", "database", "table_with_columns"}, + Type: schema.TypeList, + Optional: true, + Computed: true, + MaxItems: 1, + ExactlyOneOf: []string{ + "catalog_resource", + "data_location", + "database", + "table", + "table_with_columns", + }, Elem: &schema.Resource{ Schema: map[string]*schema.Schema{ "catalog_id": { @@ -126,21 +152,35 @@ func resourceAwsLakeFormationPermissions() *schema.Resource { Type: schema.TypeString, Optional: true, Computed: true, + AtLeastOneOf: []string{ + "table.0.name", + "table.0.wildcard", + }, }, "wildcard": { Type: schema.TypeBool, Optional: true, Default: false, + AtLeastOneOf: []string{ + "table.0.name", + "table.0.wildcard", + }, }, }, }, }, "table_with_columns": { - Type: schema.TypeList, - Optional: true, - Computed: true, - MaxItems: 1, - ConflictsWith: []string{"data_location", "database", "table"}, + Type: schema.TypeList, + Optional: true, + Computed: true, + MaxItems: 1, + ExactlyOneOf: []string{ + "catalog_resource", + "data_location", + "database", + "table", + "table_with_columns", + }, Elem: &schema.Resource{ Schema: map[string]*schema.Schema{ "catalog_id": { @@ -156,6 +196,10 @@ func resourceAwsLakeFormationPermissions() *schema.Resource { Type: schema.TypeString, ValidateFunc: validation.NoZeroValues, }, + AtLeastOneOf: []string{ + "table_with_columns.0.column_names", + "table_with_columns.0.wildcard", + }, }, "database_name": { Type: schema.TypeString, @@ -173,6 +217,15 @@ func resourceAwsLakeFormationPermissions() *schema.Resource { Type: schema.TypeString, Required: true, }, + "wildcard": { + Type: schema.TypeBool, + Optional: true, + Default: false, + AtLeastOneOf: []string{ + "table_with_columns.0.column_names", + "table_with_columns.0.wildcard", + }, + }, }, }, }, @@ -180,6 +233,17 @@ func resourceAwsLakeFormationPermissions() *schema.Resource { } } +// The challenges with Lake Formation permissions are many. These are largely undocumented and +// discovered through trial and error. These are specific problems discovered thus far: +// 1. Implicit permissions granted by Lake Formation to data lake administrators are indistinguishable +// from explicit permissions. However, implicit permissions cannot be changed, revoked, or narrowed. +// 2. One set of permissions for one LF Resource going in, can come back from AWS as multiple sets of +// permissions for multiple LF Resources (e.g., SELECT, Table, TableWithColumns). +// 3. Valid permissions for a Table LF resource can come back in TableWithColumns and vice versa. + +// For 2 & 3, some peeking at the config (i.e., d.Get()) is necessary to filter the permissions AWS +// returns. + func resourceAwsLakeFormationPermissionsCreate(d *schema.ResourceData, meta interface{}) error { conn := meta.(*AWSClient).lakeformationconn @@ -188,6 +252,7 @@ func resourceAwsLakeFormationPermissionsCreate(d *schema.ResourceData, meta inte Principal: &lakeformation.DataLakePrincipal{ DataLakePrincipalIdentifier: aws.String(d.Get("principal").(string)), }, + Resource: &lakeformation.Resource{}, } if v, ok := d.GetOk("catalog_id"); ok { @@ -198,26 +263,44 @@ func resourceAwsLakeFormationPermissionsCreate(d *schema.ResourceData, meta inte input.PermissionsWithGrantOption = expandStringList(v.([]interface{})) } - input.Resource = expandLakeFormationResource(d, false) + if _, ok := d.GetOk("catalog_resource"); ok { + input.Resource.Catalog = expandLakeFormationCatalogResource() + } + + if v, ok := d.GetOk("data_location"); ok && len(v.([]interface{})) > 0 && v.([]interface{})[0] != nil { + input.Resource.DataLocation = expandLakeFormationDataLocationResource(v.([]interface{})[0].(map[string]interface{})) + } + + if v, ok := d.GetOk("database"); ok && len(v.([]interface{})) > 0 && v.([]interface{})[0] != nil { + input.Resource.Database = expandLakeFormationDatabaseResource(v.([]interface{})[0].(map[string]interface{})) + } + + if v, ok := d.GetOk("table"); ok && len(v.([]interface{})) > 0 && v.([]interface{})[0] != nil { + input.Resource.Table = expandLakeFormationTableResource(v.([]interface{})[0].(map[string]interface{})) + } + + if v, ok := d.GetOk("table_with_columns"); ok && len(v.([]interface{})) > 0 && v.([]interface{})[0] != nil { + input.Resource.TableWithColumns = expandLakeFormationTableWithColumnsResource(v.([]interface{})[0].(map[string]interface{})) + } var output *lakeformation.GrantPermissionsOutput err := resource.Retry(iamwaiter.PropagationTimeout, func() *resource.RetryError { var err error output, err = conn.GrantPermissions(input) if err != nil { - if isAWSErr(err, lakeformation.ErrCodeInvalidInputException, "Invalid principal") { + if tfawserr.ErrMessageContains(err, lakeformation.ErrCodeInvalidInputException, "Invalid principal") { return resource.RetryableError(err) } - if isAWSErr(err, lakeformation.ErrCodeInvalidInputException, "Grantee has no permissions") { + if tfawserr.ErrMessageContains(err, lakeformation.ErrCodeInvalidInputException, "Grantee has no permissions") { return resource.RetryableError(err) } - if isAWSErr(err, lakeformation.ErrCodeInvalidInputException, "register the S3 path") { + if tfawserr.ErrMessageContains(err, lakeformation.ErrCodeInvalidInputException, "register the S3 path") { return resource.RetryableError(err) } - if isAWSErr(err, lakeformation.ErrCodeConcurrentModificationException, "") { + if tfawserr.ErrCodeEquals(err, lakeformation.ErrCodeConcurrentModificationException) { return resource.RetryableError(err) } - if isAWSErr(err, "AccessDeniedException", "is not authorized to access requested permissions") { + if tfawserr.ErrMessageContains(err, "AccessDeniedException", "is not authorized to access requested permissions") { return resource.RetryableError(err) } @@ -226,7 +309,7 @@ func resourceAwsLakeFormationPermissionsCreate(d *schema.ResourceData, meta inte return nil }) - if isResourceTimeoutError(err) { + if tfresource.TimedOut(err) { output, err = conn.GrantPermissions(input) } @@ -250,19 +333,40 @@ func resourceAwsLakeFormationPermissionsRead(d *schema.ResourceData, meta interf Principal: &lakeformation.DataLakePrincipal{ DataLakePrincipalIdentifier: aws.String(d.Get("principal").(string)), }, + Resource: &lakeformation.Resource{}, } if v, ok := d.GetOk("catalog_id"); ok { input.CatalogId = aws.String(v.(string)) } - input.Resource = expandLakeFormationResource(d, true) - matchResource := expandLakeFormationResource(d, false) - // AWS treats SELECT permissions differently. A separate resource is created for the {db}.{table}.* to grant select on all columns - selectPermissionsResource := expandLakeFormationResourceForSelectPermissions(d) + if _, ok := d.GetOk("catalog_resource"); ok { + input.Resource.Catalog = expandLakeFormationCatalogResource() + } + + if v, ok := d.GetOk("data_location"); ok && len(v.([]interface{})) > 0 && v.([]interface{})[0] != nil { + input.Resource.DataLocation = expandLakeFormationDataLocationResource(v.([]interface{})[0].(map[string]interface{})) + } + + if v, ok := d.GetOk("database"); ok && len(v.([]interface{})) > 0 && v.([]interface{})[0] != nil { + input.Resource.Database = expandLakeFormationDatabaseResource(v.([]interface{})[0].(map[string]interface{})) + } + + tableType := "" + + if v, ok := d.GetOk("table"); ok && len(v.([]interface{})) > 0 && v.([]interface{})[0] != nil { + input.Resource.Table = expandLakeFormationTableResource(v.([]interface{})[0].(map[string]interface{})) + tableType = TableTypeTable + } + + if v, ok := d.GetOk("table_with_columns"); ok && len(v.([]interface{})) > 0 && v.([]interface{})[0] != nil { + // can't ListPermissions for TableWithColumns, so use Table instead + input.Resource.Table = expandLakeFormationTableWithColumnsResourceAsTable(v.([]interface{})[0].(map[string]interface{})) + tableType = TableTypeTableWithColumns + } log.Printf("[DEBUG] Reading Lake Formation permissions: %v", input) - var principalResourcePermissions []*lakeformation.PrincipalResourcePermissions + var allPermissions []*lakeformation.PrincipalResourcePermissions err := resource.Retry(iamwaiter.PropagationTimeout, func() *resource.RetryError { err := conn.ListPermissionsPages(input, func(resp *lakeformation.ListPermissionsOutput, lastPage bool) bool { @@ -271,44 +375,28 @@ func resourceAwsLakeFormationPermissionsRead(d *schema.ResourceData, meta interf continue } - if resourceAwsLakeFormationPermissionsCompareResource(*matchResource, *permission.Resource) { - principalResourcePermissions = append(principalResourcePermissions, permission) - continue - } - - // AWS treats SELECT permissions differently. A separate resource is created for the {db}.{table}.* to grant select on all columns - if selectPermissionsResource != nil && resourceAwsLakeFormationPermissionsCompareResource(*selectPermissionsResource, *permission.Resource) { - principalResourcePermissions = append(principalResourcePermissions, permission) - } + allPermissions = append(allPermissions, permission) } return !lastPage }) if err != nil { - if isAWSErr(err, lakeformation.ErrCodeInvalidInputException, "Invalid principal") { + if tfawserr.ErrMessageContains(err, lakeformation.ErrCodeInvalidInputException, "Invalid principal") { return resource.RetryableError(err) } - return resource.NonRetryableError(fmt.Errorf("error creating Lake Formation Permissions: %w", err)) + return resource.NonRetryableError(fmt.Errorf("error reading Lake Formation Permissions: %w", err)) } return nil }) - if isResourceTimeoutError(err) { + if tfresource.TimedOut(err) { err = conn.ListPermissionsPages(input, func(resp *lakeformation.ListPermissionsOutput, lastPage bool) bool { for _, permission := range resp.PrincipalResourcePermissions { if permission == nil { continue } - if resourceAwsLakeFormationPermissionsCompareResource(*matchResource, *permission.Resource) { - principalResourcePermissions = append(principalResourcePermissions, permission) - continue - } - - // AWS treats SELECT permissions differently. A separate resource is created for the {db}.{table}.* to grant select on all columns - if selectPermissionsResource != nil && resourceAwsLakeFormationPermissionsCompareResource(*selectPermissionsResource, *permission.Resource) { - principalResourcePermissions = append(principalResourcePermissions, permission) - } + allPermissions = append(allPermissions, permission) } return !lastPage }) @@ -320,48 +408,128 @@ func resourceAwsLakeFormationPermissionsRead(d *schema.ResourceData, meta interf return nil } + if !d.IsNewResource() && tfawserr.ErrMessageContains(err, "AccessDeniedException", "Resource does not exist") { + log.Printf("[WARN] Resource Lake Formation permissions (%s) not found, removing from state: %s", d.Id(), err) + d.SetId("") + return nil + } + + if !d.IsNewResource() && len(allPermissions) == 0 { + log.Printf("[WARN] Resource Lake Formation permissions (%s) not found, removing from state (0 permissions)", d.Id()) + d.SetId("") + return nil + } + if err != nil { return fmt.Errorf("error reading Lake Formation permissions: %w", err) } - if len(principalResourcePermissions) == 0 { - log.Printf("[INFO] No Lake Formation permissions found") + // clean permissions = filter out permissions that do not pertain to this specific resource + + var cleanPermissions []*lakeformation.PrincipalResourcePermissions + + if input.Resource.Catalog != nil { + cleanPermissions = filterLakeFormationCatalogPermissions(allPermissions) + } + + if input.Resource.DataLocation != nil { + cleanPermissions = filterLakeFormationDataLocationPermissions(allPermissions) + } + + if input.Resource.Database != nil { + cleanPermissions = filterLakeFormationDatabasePermissions(allPermissions) + } + + if tableType == TableTypeTable { + cleanPermissions = filterLakeFormationTablePermissions( + aws.StringValue(input.Resource.Table.Name), + input.Resource.Table.TableWildcard != nil, + allPermissions, + ) + } + + if tableType == TableTypeTableWithColumns { + cleanPermissions = filterLakeFormationTableWithColumnsPermissions( + d.Get("table_with_columns.0.database_name").(string), + d.Get("table_with_columns.0.wildcard").(bool), + expandStringList(d.Get("table_with_columns.0.column_names").([]interface{})), + expandStringList(d.Get("table_with_columns.0.excluded_column_names").([]interface{})), + allPermissions, + ) + } + + if len(cleanPermissions) == 0 { + log.Printf("[WARN] Resource Lake Formation permissions (%s) not found, removing from state", d.Id()) + d.SetId("") return nil } - if len(principalResourcePermissions) > 2 { - return fmt.Errorf("error reading Lake Formation permissions: %s", "multiple permissions found for same resource") + if len(cleanPermissions) != len(allPermissions) { + log.Printf("[INFO] Resource Lake Formation clean permissions (%d) and all permissions (%d) have different lengths (this is not necessarily a problem): %s", len(cleanPermissions), len(allPermissions), d.Id()) } - d.Set("principal", principalResourcePermissions[0].Principal.DataLakePrincipalIdentifier) - d.Set("permissions", flattenLakeFormationPermissions(principalResourcePermissions)) - d.Set("permissions_with_grant_option", flattenLakeFormationGrantPermissions(principalResourcePermissions)) + d.Set("principal", cleanPermissions[0].Principal.DataLakePrincipalIdentifier) + d.Set("permissions", flattenLakeFormationPermissions(cleanPermissions)) + d.Set("permissions_with_grant_option", flattenLakeFormationGrantPermissions(cleanPermissions)) - if principalResourcePermissions[0].Resource.Catalog != nil { + if cleanPermissions[0].Resource.Catalog != nil { d.Set("catalog_resource", true) } - if principalResourcePermissions[0].Resource.DataLocation != nil { - d.Set("data_location", []interface{}{flattenLakeFormationDataLocationResource(principalResourcePermissions[0].Resource.DataLocation)}) + if cleanPermissions[0].Resource.DataLocation != nil { + if err := d.Set("data_location", []interface{}{flattenLakeFormationDataLocationResource(cleanPermissions[0].Resource.DataLocation)}); err != nil { + return fmt.Errorf("error setting data_location: %w", err) + } } else { d.Set("data_location", nil) } - if principalResourcePermissions[0].Resource.Database != nil { - d.Set("database", []interface{}{flattenLakeFormationDatabaseResource(principalResourcePermissions[0].Resource.Database)}) + if cleanPermissions[0].Resource.Database != nil { + if err := d.Set("database", []interface{}{flattenLakeFormationDatabaseResource(cleanPermissions[0].Resource.Database)}); err != nil { + return fmt.Errorf("error setting database: %w", err) + } } else { d.Set("database", nil) } - // table with columns permissions will include the table and table with columns - if principalResourcePermissions[0].Resource.TableWithColumns != nil { - d.Set("table_with_columns", []interface{}{flattenLakeFormationTableWithColumnsResource(principalResourcePermissions[0].Resource.TableWithColumns)}) - } else if principalResourcePermissions[0].Resource.Table != nil { - d.Set("table_with_columns", nil) - d.Set("table", []interface{}{flattenLakeFormationTableResource(principalResourcePermissions[0].Resource.Table)}) - } else { + tableSet := false + + if v, ok := d.GetOk("table"); ok && len(v.([]interface{})) > 0 { + // since perm list could include TableWithColumns, get the right one + for _, perm := range cleanPermissions { + if perm.Resource.Table != nil { + if err := d.Set("table", []interface{}{flattenLakeFormationTableResource(perm.Resource.Table)}); err != nil { + return fmt.Errorf("error setting table: %w", err) + } + tableSet = true + break + } + } + } + + if !tableSet { d.Set("table", nil) } + + twcSet := false + + if v, ok := d.GetOk("table_with_columns"); ok && len(v.([]interface{})) > 0 { + // since perm list could include Table, get the right one + for _, perm := range cleanPermissions { + if perm.Resource.TableWithColumns != nil { + if err := d.Set("table_with_columns", []interface{}{flattenLakeFormationTableWithColumnsResource(perm.Resource.TableWithColumns)}); err != nil { + return fmt.Errorf("error setting table_with_columns: %w", err) + } + twcSet = true + break + } + } + } + + if !twcSet { + d.Set("table_with_columns", nil) + } + return nil } @@ -373,6 +541,7 @@ func resourceAwsLakeFormationPermissionsDelete(d *schema.ResourceData, meta inte Principal: &lakeformation.DataLakePrincipal{ DataLakePrincipalIdentifier: aws.String(d.Get("principal").(string)), }, + Resource: &lakeformation.Resource{}, } if v, ok := d.GetOk("catalog_id"); ok { @@ -383,16 +552,43 @@ func resourceAwsLakeFormationPermissionsDelete(d *schema.ResourceData, meta inte input.PermissionsWithGrantOption = expandStringList(v.([]interface{})) } - input.Resource = expandLakeFormationResource(d, false) + if _, ok := d.GetOk("catalog_resource"); ok { + input.Resource.Catalog = expandLakeFormationCatalogResource() + } + + if v, ok := d.GetOk("data_location"); ok && len(v.([]interface{})) > 0 && v.([]interface{})[0] != nil { + input.Resource.DataLocation = expandLakeFormationDataLocationResource(v.([]interface{})[0].(map[string]interface{})) + } + + if v, ok := d.GetOk("database"); ok && len(v.([]interface{})) > 0 && v.([]interface{})[0] != nil { + input.Resource.Database = expandLakeFormationDatabaseResource(v.([]interface{})[0].(map[string]interface{})) + } + + if v, ok := d.GetOk("table"); ok && len(v.([]interface{})) > 0 && v.([]interface{})[0] != nil { + input.Resource.Table = expandLakeFormationTableResource(v.([]interface{})[0].(map[string]interface{})) + } + + if v, ok := d.GetOk("table_with_columns"); ok && len(v.([]interface{})) > 0 && v.([]interface{})[0] != nil { + input.Resource.TableWithColumns = expandLakeFormationTableWithColumnsResource(v.([]interface{})[0].(map[string]interface{})) + } + + if input.Resource == nil || reflect.DeepEqual(input.Resource, &lakeformation.Resource{}) { + // if resource is empty, don't delete = it won't delete anything since this is the predicate + log.Printf("[WARN] No Lake Formation Resource with permissions to revoke") + return nil + } err := resource.Retry(2*time.Minute, func() *resource.RetryError { var err error _, err = conn.RevokePermissions(input) if err != nil { - if isAWSErr(err, lakeformation.ErrCodeInvalidInputException, "register the S3 path") { + if tfawserr.ErrMessageContains(err, lakeformation.ErrCodeInvalidInputException, "register the S3 path") { + return resource.RetryableError(err) + } + if tfawserr.ErrCodeEquals(err, lakeformation.ErrCodeConcurrentModificationException) { return resource.RetryableError(err) } - if isAWSErr(err, lakeformation.ErrCodeConcurrentModificationException, "") { + if tfawserr.ErrMessageContains(err, "AccessDeniedException", "is not authorized to access requested permissions") { return resource.RetryableError(err) } @@ -401,7 +597,7 @@ func resourceAwsLakeFormationPermissionsDelete(d *schema.ResourceData, meta inte return nil }) - if isResourceTimeoutError(err) { + if tfresource.TimedOut(err) { _, err = conn.RevokePermissions(input) } @@ -412,104 +608,124 @@ func resourceAwsLakeFormationPermissionsDelete(d *schema.ResourceData, meta inte return nil } -func resourceAwsLakeFormationPermissionsCompareResource(in, out lakeformation.Resource) bool { - if in.DataLocation != nil && out.DataLocation != nil && in.DataLocation.CatalogId == nil { - in.DataLocation.CatalogId = out.DataLocation.CatalogId - } +const ( + TableNameAllTables = "ALL_TABLES" + TableTypeTable = "Table" + TableTypeTableWithColumns = "TableWithColumns" +) - if in.Database != nil && out.Database != nil && in.Database.CatalogId == nil { - in.Database.CatalogId = out.Database.CatalogId - } +func filterLakeFormationTablePermissions(tableName string, tableWildcard bool, allPermissions []*lakeformation.PrincipalResourcePermissions) []*lakeformation.PrincipalResourcePermissions { + // CREATE PERMS = ALL, ALTER, DELETE, DESCRIBE, DROP, INSERT, SELECT on Table, Name = (Table Name) + // LIST PERMS = ALL, ALTER, DELETE, DESCRIBE, DROP, INSERT on Table, Name = (Table Name) + // LIST PERMS = SELECT on TableWithColumns, Name = (Table Name), ColumnWildcard - if in.Table != nil && out.Table != nil && in.Table.CatalogId == nil { - in.Table.CatalogId = out.Table.CatalogId - } + // CREATE PERMS = ALL, ALTER, DELETE, DESCRIBE, DROP, INSERT, SELECT on Table, TableWildcard + // LIST PERMS = ALL, ALTER, DELETE, DESCRIBE, DROP, INSERT on Table, TableWildcard, Name = ALL_TABLES + // LIST PERMS = SELECT on TableWithColumns, Name = ALL_TABLES, ColumnWildcard + + var cleanPermissions []*lakeformation.PrincipalResourcePermissions + + for _, perm := range allPermissions { + if perm.Resource.TableWithColumns != nil && perm.Resource.TableWithColumns.ColumnWildcard != nil { + if aws.StringValue(perm.Resource.TableWithColumns.Name) == tableName || (tableWildcard && aws.StringValue(perm.Resource.TableWithColumns.Name) == TableNameAllTables) { + if len(perm.Permissions) > 0 && aws.StringValue(perm.Permissions[0]) == lakeformation.PermissionSelect { + cleanPermissions = append(cleanPermissions, perm) + continue + } + + if len(perm.PermissionsWithGrantOption) > 0 && aws.StringValue(perm.PermissionsWithGrantOption[0]) == lakeformation.PermissionSelect { + cleanPermissions = append(cleanPermissions, perm) + continue + } + } + } + + if perm.Resource.Table != nil { + if aws.StringValue(perm.Resource.Table.Name) == tableName { + cleanPermissions = append(cleanPermissions, perm) + continue + } - if in.TableWithColumns != nil && out.TableWithColumns != nil && in.TableWithColumns.CatalogId == nil { - in.TableWithColumns.CatalogId = out.TableWithColumns.CatalogId + if perm.Resource.Table.TableWildcard != nil && tableWildcard { + cleanPermissions = append(cleanPermissions, perm) + continue + } + } + continue } - return reflect.DeepEqual(in, out) + return cleanPermissions } -// expandLakeFormationResourceType returns the Lake Formation resource type represented by the resource. -// This is helpful in distinguishing between TABLE and TABLE_WITH_COLUMNS types when filtering ListPermission results. -func expandLakeFormationResourceType(d *schema.ResourceData) string { - if d.Get("catalog_resource").(bool) { - return lakeformation.DataLakeResourceTypeCatalog - } +func filterLakeFormationTableWithColumnsPermissions(tableName string, columnWildcard bool, columnNames []*string, excludedColumnNames []*string, allPermissions []*lakeformation.PrincipalResourcePermissions) []*lakeformation.PrincipalResourcePermissions { + // CREATE PERMS = ALL, ALTER, DELETE, DESCRIBE, DROP, INSERT, SELECT on TableWithColumns, Name = (Table Name), ColumnWildcard + // LIST PERMS = ALL, ALTER, DELETE, DESCRIBE, DROP, INSERT on Table, Name = (Table Name) + // LIST PERMS = SELECT on TableWithColumns, Name = (Table Name), ColumnWildcard - if v, ok := d.GetOk("data_location"); ok && len(v.([]interface{})) > 0 && v.([]interface{})[0] != nil { - return lakeformation.DataLakeResourceTypeDataLocation - } + var cleanPermissions []*lakeformation.PrincipalResourcePermissions - if v, ok := d.GetOk("database"); ok && len(v.([]interface{})) > 0 && v.([]interface{})[0] != nil { - return lakeformation.DataLakeResourceTypeDatabase - } + for _, perm := range allPermissions { + if perm.Resource.TableWithColumns != nil && perm.Resource.TableWithColumns.ColumnNames != nil { + if StringSlicesEqualIgnoreOrder(perm.Resource.TableWithColumns.ColumnNames, columnNames) { + cleanPermissions = append(cleanPermissions, perm) + continue + } + } - if v, ok := d.GetOk("table"); ok && len(v.([]interface{})) > 0 && v.([]interface{})[0] != nil { - return lakeformation.DataLakeResourceTypeTable + if perm.Resource.TableWithColumns != nil && perm.Resource.TableWithColumns.ColumnWildcard != nil && (columnWildcard || len(excludedColumnNames) > 0) { + if (perm.Resource.TableWithColumns.ColumnWildcard.ExcludedColumnNames == nil && len(excludedColumnNames) == 0) || StringSlicesEqualIgnoreOrder(perm.Resource.TableWithColumns.ColumnWildcard.ExcludedColumnNames, excludedColumnNames) { + cleanPermissions = append(cleanPermissions, perm) + continue + } + } + + if perm.Resource.Table != nil && aws.StringValue(perm.Resource.Table.Name) == tableName { + cleanPermissions = append(cleanPermissions, perm) + continue + } } - return DataLakeResourceTypeTableWithColumns + return cleanPermissions } -const DataLakeResourceTypeTableWithColumns = "TABLE_WITH_COLUMNS" // no lakeformation package enum value for this type - -func expandLakeFormationResource(d *schema.ResourceData, squashTableWithColumns bool) *lakeformation.Resource { - res := &lakeformation.Resource{} - - switch expandLakeFormationResourceType(d) { - case lakeformation.DataLakeResourceTypeCatalog: - res.Catalog = &lakeformation.CatalogResource{} - case lakeformation.DataLakeResourceTypeDataLocation: - res.DataLocation = expandLakeFormationDataLocationResource(d.Get("data_location").([]interface{})[0].(map[string]interface{})) - case lakeformation.DataLakeResourceTypeDatabase: - res.Database = expandLakeFormationDatabaseResource(d.Get("database").([]interface{})[0].(map[string]interface{})) - case lakeformation.DataLakeResourceTypeTable: - res.Table = expandLakeFormationTableResource(d.Get("table").([]interface{})[0].(map[string]interface{})) - case DataLakeResourceTypeTableWithColumns: - if squashTableWithColumns { - // ListPermissions does not support getting privileges by tables with columns. Instead, - // use the table which will return both table and table with columns. - res.Table = expandLakeFormationTableResource(d.Get("table_with_columns").([]interface{})[0].(map[string]interface{})) - } else { - res.TableWithColumns = expandLakeFormationTableWithColumnsResource(d.Get("table_with_columns").([]interface{})[0].(map[string]interface{})) +func filterLakeFormationCatalogPermissions(allPermissions []*lakeformation.PrincipalResourcePermissions) []*lakeformation.PrincipalResourcePermissions { + var cleanPermissions []*lakeformation.PrincipalResourcePermissions + + for _, perm := range allPermissions { + if perm.Resource.Catalog != nil { + cleanPermissions = append(cleanPermissions, perm) } } - return res + return cleanPermissions } -func expandLakeFormationResourceForSelectPermissions(d *schema.ResourceData) *lakeformation.Resource { - tableMapSchema := d.Get("table").([]interface{}) - if len(tableMapSchema) == 0 { - return nil - } +func filterLakeFormationDataLocationPermissions(allPermissions []*lakeformation.PrincipalResourcePermissions) []*lakeformation.PrincipalResourcePermissions { + var cleanPermissions []*lakeformation.PrincipalResourcePermissions - tableSchema := tableMapSchema[0].(map[string]interface{}) - if tableSchema == nil { - return nil + for _, perm := range allPermissions { + if perm.Resource.DataLocation != nil { + cleanPermissions = append(cleanPermissions, perm) + } } - databaseName, ok := tableSchema["database_name"].(string) - if !ok { - return nil - } - name, ok := tableSchema["name"].(string) - if !ok { - return nil - } + return cleanPermissions +} - res := &lakeformation.Resource{ - TableWithColumns: &lakeformation.TableWithColumnsResource{ - DatabaseName: aws.String(databaseName), - Name: aws.String(name), - ColumnWildcard: &lakeformation.ColumnWildcard{}, // A wildcard is used for SELECT permissions - }, +func filterLakeFormationDatabasePermissions(allPermissions []*lakeformation.PrincipalResourcePermissions) []*lakeformation.PrincipalResourcePermissions { + var cleanPermissions []*lakeformation.PrincipalResourcePermissions + + for _, perm := range allPermissions { + if perm.Resource.Database != nil { + cleanPermissions = append(cleanPermissions, perm) + } } - return res + return cleanPermissions +} + +func expandLakeFormationCatalogResource() *lakeformation.CatalogResource { + return &lakeformation.CatalogResource{} } func expandLakeFormationDataLocationResource(tfMap map[string]interface{}) *lakeformation.DataLocationResource { @@ -610,6 +826,28 @@ func expandLakeFormationTableResource(tfMap map[string]interface{}) *lakeformati return apiObject } +func expandLakeFormationTableWithColumnsResourceAsTable(tfMap map[string]interface{}) *lakeformation.TableResource { + if tfMap == nil { + return nil + } + + apiObject := &lakeformation.TableResource{} + + if v, ok := tfMap["catalog_id"].(string); ok && v != "" { + apiObject.CatalogId = aws.String(v) + } + + if v, ok := tfMap["database_name"].(string); ok && v != "" { + apiObject.DatabaseName = aws.String(v) + } + + if v, ok := tfMap["name"].(string); ok && v != "" { + apiObject.Name = aws.String(v) + } + + return apiObject +} + func flattenLakeFormationTableResource(apiObject *lakeformation.TableResource) map[string]interface{} { if apiObject == nil { return nil @@ -626,7 +864,9 @@ func flattenLakeFormationTableResource(apiObject *lakeformation.TableResource) m } if v := apiObject.Name; v != nil { - tfMap["name"] = aws.StringValue(v) + if aws.StringValue(v) != TableNameAllTables || apiObject.TableWildcard == nil { + tfMap["name"] = aws.StringValue(v) + } } if v := apiObject.TableWildcard; v != nil { @@ -665,6 +905,10 @@ func expandLakeFormationTableWithColumnsResource(tfMap map[string]interface{}) * apiObject.Name = aws.String(v) } + if v, ok := tfMap["wildcard"].(bool); ok && v && apiObject.ColumnWildcard == nil { + apiObject.ColumnWildcard = &lakeformation.ColumnWildcard{} + } + return apiObject } @@ -686,6 +930,7 @@ func flattenLakeFormationTableWithColumnsResource(apiObject *lakeformation.Table } if v := apiObject.ColumnWildcard; v != nil { + tfMap["wildcard"] = true tfMap["excluded_column_names"] = flattenStringList(v.ExcludedColumnNames) } From 1f0971e48eabba2b893f752f791457ee220336a9 Mon Sep 17 00:00:00 2001 From: Dirk Avery Date: Tue, 4 May 2021 19:14:20 -0400 Subject: [PATCH 09/13] ds/lakeformation_permissions: Revise for permissions rework --- ...ta_source_aws_lakeformation_permissions.go | 167 +++++++++++++----- 1 file changed, 124 insertions(+), 43 deletions(-) diff --git a/aws/data_source_aws_lakeformation_permissions.go b/aws/data_source_aws_lakeformation_permissions.go index 71fb02d8721..2529d491d12 100644 --- a/aws/data_source_aws_lakeformation_permissions.go +++ b/aws/data_source_aws_lakeformation_permissions.go @@ -3,7 +3,6 @@ package aws import ( "fmt" "log" - "time" "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/lakeformation" @@ -12,6 +11,8 @@ import ( "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/validation" "github.com/terraform-providers/terraform-provider-aws/aws/internal/hashcode" + iamwaiter "github.com/terraform-providers/terraform-provider-aws/aws/internal/service/iam/waiter" + "github.com/terraform-providers/terraform-provider-aws/aws/internal/tfresource" ) func dataSourceAwsLakeFormationPermissions() *schema.Resource { @@ -156,6 +157,10 @@ func dataSourceAwsLakeFormationPermissions() *schema.Resource { Type: schema.TypeString, Required: true, }, + "wildcard": { + Type: schema.TypeBool, + Optional: true, + }, }, }, }, @@ -170,34 +175,55 @@ func dataSourceAwsLakeFormationPermissionsRead(d *schema.ResourceData, meta inte Principal: &lakeformation.DataLakePrincipal{ DataLakePrincipalIdentifier: aws.String(d.Get("principal").(string)), }, + Resource: &lakeformation.Resource{}, } if v, ok := d.GetOk("catalog_id"); ok { input.CatalogId = aws.String(v.(string)) } - input.Resource = expandLakeFormationResource(d, true) - matchResource := expandLakeFormationResource(d, false) + if _, ok := d.GetOk("catalog_resource"); ok { + input.Resource.Catalog = expandLakeFormationCatalogResource() + } + + if v, ok := d.GetOk("data_location"); ok && len(v.([]interface{})) > 0 && v.([]interface{})[0] != nil { + input.Resource.DataLocation = expandLakeFormationDataLocationResource(v.([]interface{})[0].(map[string]interface{})) + } + + if v, ok := d.GetOk("database"); ok && len(v.([]interface{})) > 0 && v.([]interface{})[0] != nil { + input.Resource.Database = expandLakeFormationDatabaseResource(v.([]interface{})[0].(map[string]interface{})) + } + + tableType := "" + + if v, ok := d.GetOk("table"); ok && len(v.([]interface{})) > 0 && v.([]interface{})[0] != nil { + input.Resource.Table = expandLakeFormationTableResource(v.([]interface{})[0].(map[string]interface{})) + tableType = TableTypeTable + } + + if v, ok := d.GetOk("table_with_columns"); ok && len(v.([]interface{})) > 0 && v.([]interface{})[0] != nil { + // can't ListPermissions for TableWithColumns, so use Table instead + input.Resource.Table = expandLakeFormationTableWithColumnsResourceAsTable(v.([]interface{})[0].(map[string]interface{})) + tableType = TableTypeTableWithColumns + } log.Printf("[DEBUG] Reading Lake Formation permissions: %v", input) - var principalResourcePermissions []*lakeformation.PrincipalResourcePermissions + var allPermissions []*lakeformation.PrincipalResourcePermissions - err := resource.Retry(2*time.Minute, func() *resource.RetryError { + err := resource.Retry(iamwaiter.PropagationTimeout, func() *resource.RetryError { err := conn.ListPermissionsPages(input, func(resp *lakeformation.ListPermissionsOutput, lastPage bool) bool { for _, permission := range resp.PrincipalResourcePermissions { if permission == nil { continue } - if resourceAwsLakeFormationPermissionsCompareResource(*matchResource, *permission.Resource) { - principalResourcePermissions = append(principalResourcePermissions, permission) - } + allPermissions = append(allPermissions, permission) } return !lastPage }) if err != nil { - if isAWSErr(err, lakeformation.ErrCodeInvalidInputException, "Invalid principal") { + if tfawserr.ErrMessageContains(err, lakeformation.ErrCodeInvalidInputException, "Invalid principal") { return resource.RetryableError(err) } return resource.NonRetryableError(fmt.Errorf("error reading Lake Formation Permissions: %w", err)) @@ -205,67 +231,122 @@ func dataSourceAwsLakeFormationPermissionsRead(d *schema.ResourceData, meta inte return nil }) - if isResourceTimeoutError(err) { + if tfresource.TimedOut(err) { err = conn.ListPermissionsPages(input, func(resp *lakeformation.ListPermissionsOutput, lastPage bool) bool { for _, permission := range resp.PrincipalResourcePermissions { if permission == nil { continue } - if resourceAwsLakeFormationPermissionsCompareResource(*matchResource, *permission.Resource) { - principalResourcePermissions = append(principalResourcePermissions, permission) - } + allPermissions = append(allPermissions, permission) } return !lastPage }) } - if !d.IsNewResource() && tfawserr.ErrCodeEquals(err, lakeformation.ErrCodeEntityNotFoundException) { - log.Printf("[WARN] Resource Lake Formation permissions (%s) not found, removing from state", d.Id()) - d.SetId("") - return nil - } + d.SetId(fmt.Sprintf("%d", hashcode.String(input.String()))) if err != nil { return fmt.Errorf("error reading Lake Formation permissions: %w", err) } - if len(principalResourcePermissions) > 1 { - return fmt.Errorf("error reading Lake Formation permissions: %s", "multiple permissions found") + var cleanPermissions []*lakeformation.PrincipalResourcePermissions + + if input.Resource.Catalog != nil { + cleanPermissions = filterLakeFormationCatalogPermissions(allPermissions) } - d.SetId(fmt.Sprintf("%d", hashcode.String(input.String()))) - for _, permissions := range principalResourcePermissions { - d.Set("principal", permissions.Principal.DataLakePrincipalIdentifier) - d.Set("permissions", permissions.Permissions) - d.Set("permissions_with_grant_option", permissions.PermissionsWithGrantOption) + if input.Resource.DataLocation != nil { + cleanPermissions = filterLakeFormationDataLocationPermissions(allPermissions) + } + + if input.Resource.Database != nil { + cleanPermissions = filterLakeFormationDatabasePermissions(allPermissions) + } + + if tableType == TableTypeTable { + cleanPermissions = filterLakeFormationTablePermissions( + aws.StringValue(input.Resource.Table.Name), + input.Resource.Table.TableWildcard != nil, + allPermissions, + ) + } + + if tableType == TableTypeTableWithColumns { + cleanPermissions = filterLakeFormationTableWithColumnsPermissions( + d.Get("table_with_columns.0.database_name").(string), + d.Get("table_with_columns.0.wildcard").(bool), + expandStringList(d.Get("table_with_columns.0.column_names").([]interface{})), + expandStringList(d.Get("table_with_columns.0.excluded_column_names").([]interface{})), + allPermissions, + ) + } + + if len(cleanPermissions) != len(allPermissions) { + log.Printf("[INFO] Resource Lake Formation clean permissions (%d) and all permissions (%d) have different lengths (this is not necessarily a problem): %s", len(cleanPermissions), len(allPermissions), d.Id()) + } + + d.Set("principal", cleanPermissions[0].Principal.DataLakePrincipalIdentifier) + d.Set("permissions", flattenLakeFormationPermissions(cleanPermissions)) + d.Set("permissions_with_grant_option", flattenLakeFormationGrantPermissions(cleanPermissions)) + + if cleanPermissions[0].Resource.Catalog != nil { + d.Set("catalog_resource", true) + } - if permissions.Resource.Catalog != nil { - d.Set("catalog_resource", true) + if cleanPermissions[0].Resource.DataLocation != nil { + if err := d.Set("data_location", []interface{}{flattenLakeFormationDataLocationResource(cleanPermissions[0].Resource.DataLocation)}); err != nil { + return fmt.Errorf("error setting data_location: %w", err) } + } else { + d.Set("data_location", nil) + } - if permissions.Resource.DataLocation != nil { - d.Set("data_location", []interface{}{flattenLakeFormationDataLocationResource(permissions.Resource.DataLocation)}) - } else { - d.Set("data_location", nil) + if cleanPermissions[0].Resource.Database != nil { + if err := d.Set("database", []interface{}{flattenLakeFormationDatabaseResource(cleanPermissions[0].Resource.Database)}); err != nil { + return fmt.Errorf("error setting database: %w", err) } + } else { + d.Set("database", nil) + } - if permissions.Resource.Database != nil { - d.Set("database", []interface{}{flattenLakeFormationDatabaseResource(permissions.Resource.Database)}) - } else { - d.Set("database", nil) + tableSet := false + + if v, ok := d.GetOk("table"); ok && len(v.([]interface{})) > 0 { + // since perm list could include TableWithColumns, get the right one + for _, perm := range cleanPermissions { + if perm.Resource.Table != nil { + if err := d.Set("table", []interface{}{flattenLakeFormationTableResource(perm.Resource.Table)}); err != nil { + return fmt.Errorf("error setting table: %w", err) + } + tableSet = true + break + } } + } + + if !tableSet { + d.Set("table", nil) + } - // table with columns permissions will include the table and table with columns - if permissions.Resource.TableWithColumns != nil { - d.Set("table_with_columns", []interface{}{flattenLakeFormationTableWithColumnsResource(permissions.Resource.TableWithColumns)}) - } else if permissions.Resource.Table != nil { - d.Set("table_with_columns", nil) - d.Set("table", []interface{}{flattenLakeFormationTableResource(permissions.Resource.Table)}) - } else { - d.Set("table", nil) + twcSet := false + + if v, ok := d.GetOk("table_with_columns"); ok && len(v.([]interface{})) > 0 { + // since perm list could include Table, get the right one + for _, perm := range cleanPermissions { + if perm.Resource.TableWithColumns != nil { + if err := d.Set("table_with_columns", []interface{}{flattenLakeFormationTableWithColumnsResource(perm.Resource.TableWithColumns)}); err != nil { + return fmt.Errorf("error setting table_with_columns: %w", err) + } + twcSet = true + break + } } } + if !twcSet { + d.Set("table_with_columns", nil) + } + return nil } From fafb6f81f2d872cd7c43492106f17748c663a100 Mon Sep 17 00:00:00 2001 From: Dirk Avery Date: Tue, 4 May 2021 19:14:42 -0400 Subject: [PATCH 10/13] tests/ds/lakeformation_permissions: Test permissions rework --- ...a_source_aws_lakeformation_permissions_test.go | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/aws/data_source_aws_lakeformation_permissions_test.go b/aws/data_source_aws_lakeformation_permissions_test.go index 479f8f25032..fe111e051e2 100644 --- a/aws/data_source_aws_lakeformation_permissions_test.go +++ b/aws/data_source_aws_lakeformation_permissions_test.go @@ -176,6 +176,9 @@ resource "aws_lakeformation_permissions" "test" { principal = aws_iam_role.test.arn permissions = ["CREATE_DATABASE"] catalog_resource = true + + # for consistency, ensure that admins are setup before testing + depends_on = [aws_lakeformation_data_lake_settings.test] } data "aws_lakeformation_permissions" "test" { @@ -234,7 +237,8 @@ resource "aws_lakeformation_permissions" "test" { arn = aws_s3_bucket.test.arn } - depends_on = ["aws_lakeformation_data_lake_settings.test"] + # for consistency, ensure that admins are setup before testing + depends_on = [aws_lakeformation_data_lake_settings.test] } data "aws_lakeformation_permissions" "test" { @@ -295,7 +299,8 @@ resource "aws_lakeformation_permissions" "test" { name = aws_glue_catalog_database.test.name } - depends_on = ["aws_lakeformation_data_lake_settings.test"] + # for consistency, ensure that admins are setup before testing + depends_on = [aws_lakeformation_data_lake_settings.test] } data "aws_lakeformation_permissions" "test" { @@ -360,6 +365,9 @@ resource "aws_lakeformation_permissions" "test" { database_name = aws_glue_catalog_table.test.database_name name = aws_glue_catalog_table.test.name } + + # for consistency, ensure that admins are setup before testing + depends_on = [aws_lakeformation_data_lake_settings.test] } data "aws_lakeformation_permissions" "test" { @@ -437,6 +445,9 @@ resource "aws_lakeformation_permissions" "test" { name = aws_glue_catalog_table.test.name column_names = ["event", "timestamp"] } + + # for consistency, ensure that admins are setup before testing + depends_on = [aws_lakeformation_data_lake_settings.test] } data "aws_lakeformation_permissions" "test" { From 1c677ead15b67d3926b74997ace649c14b14a699 Mon Sep 17 00:00:00 2001 From: Dirk Avery Date: Tue, 4 May 2021 19:19:53 -0400 Subject: [PATCH 11/13] r/lakeformation_permissions: Update changelog --- .changelog/18505.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changelog/18505.txt b/.changelog/18505.txt index 4f277694495..8082c8f1bfc 100644 --- a/.changelog/18505.txt +++ b/.changelog/18505.txt @@ -1,3 +1,3 @@ ```release-note:bug - resource/aws_lakeformation_permissions: aws_lakeformation_permissions throwing "no permissions found" for wildcard table + resource/aws_lakeformation_permissions: Fix issues related to permissions not being revoked and attempts to revoke non-existent permissions ``` \ No newline at end of file From 36fc0cd73d04be70fda5bc516cc5a466a46df83c Mon Sep 17 00:00:00 2001 From: Dirk Avery Date: Tue, 4 May 2021 19:29:50 -0400 Subject: [PATCH 12/13] tests/r/lakeformation_permissions: Lint --- aws/resource_aws_lakeformation_permissions_test.go | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/aws/resource_aws_lakeformation_permissions_test.go b/aws/resource_aws_lakeformation_permissions_test.go index 67a65be96ff..dce5858a6fa 100644 --- a/aws/resource_aws_lakeformation_permissions_test.go +++ b/aws/resource_aws_lakeformation_permissions_test.go @@ -846,10 +846,12 @@ resource "aws_glue_catalog_table" "test" { name = "event" type = "string" } + columns { name = "timestamp" type = "date" } + columns { name = "value" type = "double" @@ -917,10 +919,12 @@ resource "aws_glue_catalog_table" "test" { name = "event" type = "string" } + columns { name = "timestamp" type = "date" } + columns { name = "value" type = "double" @@ -990,10 +994,12 @@ resource "aws_glue_catalog_table" "test" { name = "event" type = "string" } + columns { name = "timestamp" type = "date" } + columns { name = "value" type = "double" @@ -1062,10 +1068,12 @@ resource "aws_glue_catalog_table" "test" { name = "event" type = "string" } + columns { name = "timestamp" type = "date" } + columns { name = "value" type = "double" @@ -1134,10 +1142,12 @@ resource "aws_glue_catalog_table" "test" { name = "event" type = "string" } + columns { name = "timestamp" type = "date" } + columns { name = "value" type = "double" @@ -1206,10 +1216,12 @@ resource "aws_glue_catalog_table" "test" { name = "event" type = "string" } + columns { name = "timestamp" type = "date" } + columns { name = "value" type = "double" @@ -1227,7 +1239,7 @@ resource "aws_lakeformation_permissions" "test" { table_with_columns { database_name = aws_glue_catalog_table.test.database_name - name = aws_glue_catalog_table.test.name + name = aws_glue_catalog_table.test.name wildcard = true } From 2110ac937a4e4f8ec96bf6925f04c6171a483de2 Mon Sep 17 00:00:00 2001 From: Dirk Avery Date: Tue, 4 May 2021 19:32:40 -0400 Subject: [PATCH 13/13] tests/r/lakeformation_permissions: Lint --- aws/resource_aws_lakeformation_permissions_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/aws/resource_aws_lakeformation_permissions_test.go b/aws/resource_aws_lakeformation_permissions_test.go index dce5858a6fa..69857161931 100644 --- a/aws/resource_aws_lakeformation_permissions_test.go +++ b/aws/resource_aws_lakeformation_permissions_test.go @@ -851,7 +851,7 @@ resource "aws_glue_catalog_table" "test" { name = "timestamp" type = "date" } - + columns { name = "value" type = "double"