From f71c8019a21e2fb649a366be3b8540263e1b0e19 Mon Sep 17 00:00:00 2001 From: vuong-nguyen <44292934+nkvuong@users.noreply.github.com> Date: Mon, 18 Mar 2024 16:57:32 +0000 Subject: [PATCH] improve `databricks_aws_crossaccount_policy` data source (#3343) * improve `databricks_aws_crossaccount_policy` data source * update guide * feedback --- aws/data_aws_crossaccount_policy.go | 427 +++++++++++++------ aws/data_aws_crossaccount_policy_test.go | 101 ++++- docs/data-sources/aws_crossaccount_policy.md | 6 + docs/guides/aws-workspace.md | 1 + 4 files changed, 401 insertions(+), 134 deletions(-) diff --git a/aws/data_aws_crossaccount_policy.go b/aws/data_aws_crossaccount_policy.go index 8275aecdb3..04b8a6e7e5 100644 --- a/aws/data_aws_crossaccount_policy.go +++ b/aws/data_aws_crossaccount_policy.go @@ -3,149 +3,314 @@ package aws import ( "context" "encoding/json" + "fmt" + "regexp" + "slices" + "github.com/databricks/databricks-sdk-go" "github.com/databricks/terraform-provider-databricks/common" - "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" ) // DataAwsCrossaccountPolicy defines the cross-account policy func DataAwsCrossaccountPolicy() common.Resource { - return common.Resource{ - Read: func(ctx context.Context, d *schema.ResourceData, m *common.DatabricksClient) error { - policy := awsIamPolicy{ - Version: "2012-10-17", - Statements: []*awsIamPolicyStatement{ - { - Effect: "Allow", - Actions: []string{ - "ec2:AllocateAddress", - "ec2:AssignPrivateIpAddresses", - "ec2:AssociateDhcpOptions", - "ec2:AssociateIamInstanceProfile", - "ec2:AssociateRouteTable", - "ec2:AttachInternetGateway", - "ec2:AttachVolume", - "ec2:AuthorizeSecurityGroupEgress", - "ec2:AuthorizeSecurityGroupIngress", - "ec2:CancelSpotInstanceRequests", - "ec2:CreateDhcpOptions", - "ec2:CreateFleet", - "ec2:CreateInternetGateway", - "ec2:CreateKeyPair", - "ec2:CreateLaunchTemplate", - "ec2:CreateLaunchTemplateVersion", - "ec2:CreateNatGateway", - "ec2:CreatePlacementGroup", - "ec2:CreateRoute", - "ec2:CreateRouteTable", - "ec2:CreateSecurityGroup", - "ec2:CreateSubnet", - "ec2:CreateTags", - "ec2:CreateVolume", - "ec2:CreateVpc", - "ec2:CreateVpcEndpoint", - "ec2:DeleteDhcpOptions", - "ec2:DeleteFleets", - "ec2:DeleteInternetGateway", - "ec2:DeleteKeyPair", - "ec2:DeleteLaunchTemplate", - "ec2:DeleteLaunchTemplateVersions", - "ec2:DeleteNatGateway", - "ec2:DeletePlacementGroup", - "ec2:DeleteRoute", - "ec2:DeleteRouteTable", - "ec2:DeleteSecurityGroup", - "ec2:DeleteSubnet", - "ec2:DeleteTags", - "ec2:DeleteVolume", - "ec2:DeleteVpc", - "ec2:DeleteVpcEndpoints", - "ec2:DescribeAvailabilityZones", - "ec2:DescribeFleetHistory", - "ec2:DescribeFleetInstances", - "ec2:DescribeFleets", - "ec2:DescribeIamInstanceProfileAssociations", - "ec2:DescribeInstanceStatus", - "ec2:DescribeInstances", - "ec2:DescribeInternetGateways", - "ec2:DescribeLaunchTemplates", - "ec2:DescribeLaunchTemplateVersions", - "ec2:DescribeNatGateways", - "ec2:DescribeNetworkAcls", - "ec2:DescribePlacementGroups", - "ec2:DescribePrefixLists", - "ec2:DescribeReservedInstancesOfferings", - "ec2:DescribeRouteTables", - "ec2:DescribeSecurityGroups", - "ec2:DescribeSpotInstanceRequests", - "ec2:DescribeSpotPriceHistory", - "ec2:DescribeSubnets", - "ec2:DescribeVolumes", - "ec2:DescribeVpcAttribute", - "ec2:DescribeVpcs", - "ec2:DetachInternetGateway", - "ec2:DetachVolume", - "ec2:DisassociateIamInstanceProfile", - "ec2:DisassociateRouteTable", - "ec2:GetLaunchTemplateData", - "ec2:GetSpotPlacementScores", - "ec2:ModifyFleet", - "ec2:ModifyLaunchTemplate", - "ec2:ModifyVpcAttribute", - "ec2:ReleaseAddress", - "ec2:ReplaceIamInstanceProfileAssociation", - "ec2:RequestSpotInstances", - "ec2:RevokeSecurityGroupEgress", - "ec2:RevokeSecurityGroupIngress", - "ec2:RunInstances", - "ec2:TerminateInstances", - }, - Resources: "*", + type AwsCrossAccountPolicy struct { + PolicyType string `json:"policy_type,omitempty" tf:"default:managed"` + PassRole []string `json:"pass_roles,omitempty"` + JSON string `json:"json" tf:"computed"` + AwsAccountId string `json:"aws_account_id,omitempty"` + VpcId string `json:"vpc_id,omitempty"` + Region string `json:"region,omitempty"` + SecurityGroupId string `json:"security_group_id,omitempty"` + } + return common.WorkspaceData(func(ctx context.Context, data *AwsCrossAccountPolicy, w *databricks.WorkspaceClient) error { + if !slices.Contains([]string{"managed", "customer", "restricted"}, data.PolicyType) { + return fmt.Errorf("policy_type must be either 'managed', 'customer' or 'restricted'") + } + + if data.PolicyType == "restricted" { + match, _ := regexp.MatchString(`^\d{12}$`, data.AwsAccountId) + if !match { + return fmt.Errorf("aws_account_id must be a 12 digit number") + } + match, _ = regexp.MatchString(`^vpc-.*$`, data.VpcId) + if !match { + return fmt.Errorf("vpc_id must begin with 'vpc-'") + } + if data.Region == "" { + return fmt.Errorf("region must be set") + } + match, _ = regexp.MatchString(`^sg-.*$`, data.SecurityGroupId) + if !match { + return fmt.Errorf("security_group_id must begin with 'sg-'") + } + } + // non resource-based permissions + actions := []string{ + "ec2:AssignPrivateIpAddresses", + "ec2:CancelSpotInstanceRequests", + "ec2:DescribeAvailabilityZones", + "ec2:DescribeIamInstanceProfileAssociations", + "ec2:DescribeInstanceStatus", + "ec2:DescribeInstances", + "ec2:DescribeInternetGateways", + "ec2:DescribeNatGateways", + "ec2:DescribeNetworkAcls", + "ec2:DescribePrefixLists", + "ec2:DescribeReservedInstancesOfferings", + "ec2:DescribeRouteTables", + "ec2:DescribeSecurityGroups", + "ec2:DescribeSpotInstanceRequests", + "ec2:DescribeSpotPriceHistory", + "ec2:DescribeSubnets", + "ec2:DescribeVolumes", + "ec2:DescribeVpcAttribute", + "ec2:DescribeVpcs", + "ec2:CreateTags", + "ec2:DeleteTags", + "ec2:GetSpotPlacementScores", + "ec2:RequestSpotInstances", + "ec2:DescribeFleetHistory", + "ec2:ModifyFleet", + "ec2:DeleteFleets", + "ec2:DescribeFleetInstances", + "ec2:DescribeFleets", + "ec2:CreateFleet", + "ec2:DeleteLaunchTemplate", + "ec2:GetLaunchTemplateData", + "ec2:CreateLaunchTemplate", + "ec2:DescribeLaunchTemplates", + "ec2:DescribeLaunchTemplateVersions", + "ec2:ModifyLaunchTemplate", + "ec2:DeleteLaunchTemplateVersions", + "ec2:CreateLaunchTemplateVersion", + } + if data.PolicyType != "restricted" { + actions = append(actions, []string{ + "ec2:AssociateIamInstanceProfile", + "ec2:AttachVolume", + "ec2:AuthorizeSecurityGroupEgress", + "ec2:AuthorizeSecurityGroupIngress", + "ec2:CreateVolume", + "ec2:DeleteVolume", + "ec2:DetachVolume", + "ec2:DisassociateIamInstanceProfile", + "ec2:ReplaceIamInstanceProfileAssociation", + "ec2:RevokeSecurityGroupEgress", + "ec2:RevokeSecurityGroupIngress", + "ec2:RunInstances", + "ec2:TerminateInstances", + }...) + } + // additional permissions for Databricks-managed VPC policy + if data.PolicyType == "managed" { + actions = append(actions, []string{ + "ec2:CreateDhcpOptions", + "ec2:CreateInternetGateway", + "ec2:CreateNatGateway", + "ec2:CreateRoute", + "ec2:CreateRouteTable", + "ec2:CreateSecurityGroup", + "ec2:CreateSubnet", + "ec2:CreateVpc", + "ec2:CreateVpcEndpoint", + "ec2:DeleteDhcpOptions", + "ec2:DeleteInternetGateway", + "ec2:DeleteNatGateway", + "ec2:DeleteRoute", + "ec2:DeleteRouteTable", + "ec2:DeleteSecurityGroup", + "ec2:DeleteSubnet", + "ec2:DeleteVpc", + "ec2:DeleteVpcEndpoints", + "ec2:DetachInternetGateway", + "ec2:DisassociateRouteTable", + "ec2:ModifyVpcAttribute", + "ec2:ReleaseAddress", + }...) + } + policy := awsIamPolicy{ + Version: "2012-10-17", + Statements: []*awsIamPolicyStatement{ + { + Effect: "Allow", + Actions: actions, + Resources: "*", + }, + { + Effect: "Allow", + Actions: []string{ + "iam:CreateServiceLinkedRole", + "iam:PutRolePolicy", }, - { - Effect: "Allow", - Actions: []string{ - "iam:CreateServiceLinkedRole", - "iam:PutRolePolicy", - }, - Resources: "arn:aws:iam::*:role/aws-service-role/spot.amazonaws.com/AWSServiceRoleForEC2Spot", - Condition: map[string]map[string]string{ - "StringLike": { - "iam:AWSServiceName": "spot.amazonaws.com", - }, + Resources: "arn:aws:iam::*:role/aws-service-role/spot.amazonaws.com/AWSServiceRoleForEC2Spot", + Condition: map[string]map[string]string{ + "StringLike": { + "iam:AWSServiceName": "spot.amazonaws.com", }, }, }, - } - if passRoleARNs, ok := d.GetOk("pass_roles"); ok { - policy.Statements = append(policy.Statements, &awsIamPolicyStatement{ + }, + } + // pass role permissions + if len(data.PassRole) > 0 { + policy.Statements = append(policy.Statements, + &awsIamPolicyStatement{ Effect: "Allow", Actions: "iam:PassRole", - Resources: passRoleARNs, + Resources: data.PassRole, }) - } - policyJSON, err := json.MarshalIndent(policy, "", " ") - if err != nil { - return err - } - d.SetId("cross-account") - // nolint - d.Set("json", string(policyJSON)) - return nil - }, - Schema: map[string]*schema.Schema{ - "pass_roles": { - Type: schema.TypeList, - Elem: &schema.Schema{ - Type: schema.TypeString, + } + + // resource-based permissions + if data.PolicyType == "restricted" { + region := data.Region + aws_account_id := data.AwsAccountId + vpc_id := data.VpcId + security_group_id := data.SecurityGroupId + policy.Statements = append(policy.Statements, + &awsIamPolicyStatement{ + Sid: "InstancePoolsSupport", + Effect: "Allow", + Actions: []string{ + "ec2:AssociateIamInstanceProfile", + "ec2:DisassociateIamInstanceProfile", + "ec2:ReplaceIamInstanceProfileAssociation", + }, + Resources: fmt.Sprintf("arn:aws:ec2:%s:%s:instance/*", region, aws_account_id), + Condition: map[string]map[string]string{ + "StringEquals": { + "ec2:ResourceTag/Vendor": "Databricks", + }, + }, }, - Optional: true, - }, - "json": { - Type: schema.TypeString, - Computed: true, - ForceNew: true, - }, - }, - } + &awsIamPolicyStatement{ + Sid: "AllowEc2RunInstancePerTag", + Effect: "Allow", + Actions: "ec2:RunInstances", + Resources: []string{ + "arn:aws:ec2:%s:%s:volume/*", + "arn:aws:ec2:%s:%s:instance/*", + }, + Condition: map[string]map[string]string{ + "StringEquals": { + "aws:RequestTag/Vendor": "Databricks", + }, + }, + }, + &awsIamPolicyStatement{ + Sid: "AllowEc2RunInstanceImagePerTag", + Effect: "Allow", + Actions: "ec2:RunInstances", + Resources: fmt.Sprintf("arn:aws:ec2:%s:%s:image/*", region, aws_account_id), + Condition: map[string]map[string]string{ + "StringEquals": { + "aws:ResourceTag/Vendor": "Databricks", + }, + }, + }, + &awsIamPolicyStatement{ + Sid: "AllowEc2RunInstancePerVPCid", + Effect: "Allow", + Actions: "ec2:RunInstances", + Resources: []string{ + fmt.Sprintf("arn:aws:ec2:%s:%s:network-interface/*", region, aws_account_id), + fmt.Sprintf("arn:aws:ec2:%s:%s:subnet/*", region, aws_account_id), + fmt.Sprintf("arn:aws:ec2:%s:%s:security-group/*", region, aws_account_id), + }, + Condition: map[string]map[string]string{ + "StringEquals": { + "ec2:vpc": fmt.Sprintf("arn:aws:ec2:%s:%s:vpc/%s", region, aws_account_id, vpc_id), + }, + }, + }, + &awsIamPolicyStatement{ + Sid: "AllowEc2RunInstanceOtherResources", + Effect: "Allow", + Actions: "ec2:RunInstances", + NotResources: []string{ + fmt.Sprintf("arn:aws:ec2:%s:%s:image/*", region, aws_account_id), + fmt.Sprintf("arn:aws:ec2:%s:%s:network-interface/*", region, aws_account_id), + fmt.Sprintf("arn:aws:ec2:%s:%s:subnet/*", region, aws_account_id), + fmt.Sprintf("arn:aws:ec2:%s:%s:security-group/*", region, aws_account_id), + fmt.Sprintf("arn:aws:ec2:%s:%s:volume/*", region, aws_account_id), + fmt.Sprintf("arn:aws:ec2:%s:%s:instance/*", region, aws_account_id), + }, + }, + &awsIamPolicyStatement{ + Sid: "EC2TerminateInstancesTag", + Effect: "Allow", + Actions: "ec2:TerminateInstances", + Resources: fmt.Sprintf("arn:aws:ec2:%s:%s:instance/*", region, aws_account_id), + Condition: map[string]map[string]string{ + "StringEquals": { + "ec2:ResourceTag/Vendor": "Databricks", + }, + }, + }, + &awsIamPolicyStatement{ + Sid: "EC2AttachDetachVolumeTag", + Effect: "Allow", + Actions: []string{ + "ec2:AttachVolume", + "ec2:DetachVolume", + }, + Resources: []string{ + fmt.Sprintf("arn:aws:ec2:%s:%s:instance/*", region, aws_account_id), + fmt.Sprintf("arn:aws:ec2:%s:%s:volume/*", region, aws_account_id), + }, + Condition: map[string]map[string]string{ + "StringEquals": { + "ec2:ResourceTag/Vendor": "Databricks", + }, + }, + }, + &awsIamPolicyStatement{ + Sid: "EC2CreateVolumeByTag", + Effect: "Allow", + Actions: "ec2:CreateVolume", + Resources: fmt.Sprintf("arn:aws:ec2:%s:%s:volume/*", region, aws_account_id), + Condition: map[string]map[string]string{ + "StringEquals": { + "aws:RequestTag/Vendor": "Databricks", + }, + }, + }, + &awsIamPolicyStatement{ + Sid: "EC2DeleteVolumeByTag", + Effect: "Allow", + Actions: "ec2:DeleteVolume", + Resources: []string{ + fmt.Sprintf("arn:aws:ec2:%s:%s:volume/*", region, aws_account_id), + }, + Condition: map[string]map[string]string{ + "StringEquals": { + "ec2:ResourceTag/Vendor": "Databricks", + }, + }, + }, + &awsIamPolicyStatement{ + Sid: "VpcNonresourceSpecificActions", + Effect: "Allow", + Actions: []string{ + "ec2:AuthorizeSecurityGroupEgress", + "ec2:AuthorizeSecurityGroupIngress", + "ec2:RevokeSecurityGroupEgress", + "ec2:RevokeSecurityGroupIngress", + }, + Resources: fmt.Sprintf("arn:aws:ec2:%s:%s:security-group/%s", region, aws_account_id, security_group_id), + Condition: map[string]map[string]string{ + "StringEquals": { + "ec2:vpc": fmt.Sprintf("arn:aws:ec2:%s:%s:vpc/%s", region, aws_account_id, vpc_id), + }, + }, + }, + ) + } + policyJSON, err := json.MarshalIndent(policy, "", " ") + if err != nil { + return err + } + data.JSON = string(policyJSON) + return nil + }) } diff --git a/aws/data_aws_crossaccount_policy_test.go b/aws/data_aws_crossaccount_policy_test.go index 71a123c72f..f2ee96514a 100644 --- a/aws/data_aws_crossaccount_policy_test.go +++ b/aws/data_aws_crossaccount_policy_test.go @@ -7,7 +7,7 @@ import ( "github.com/stretchr/testify/assert" ) -func TestDataAwsCrossAccountPolicy(t *testing.T) { +func TestDataAwsCrossAccountDatabricksManagedPolicy(t *testing.T) { d, err := qa.ResourceFixture{ Read: true, Resource: DataAwsCrossaccountPolicy(), @@ -16,7 +16,20 @@ func TestDataAwsCrossAccountPolicy(t *testing.T) { }.Apply(t) assert.NoError(t, err) j := d.Get("json") - assert.Lenf(t, j, 3340, "Strange length for policy: %s", j) + assert.Lenf(t, j, 3032, "Strange length for policy: %s", j) +} + +func TestDataAwsCrossAccountCustomerManagedPolicy(t *testing.T) { + d, err := qa.ResourceFixture{ + Read: true, + Resource: DataAwsCrossaccountPolicy(), + NonWritable: true, + HCL: `policy_type = "customer"`, + ID: ".", + }.Apply(t) + assert.NoError(t, err) + j := d.Get("json") + assert.Lenf(t, j, 2328, "Strange length for policy: %s", j) } func TestDataAwsCrossAccountPolicy_WithPassRoles(t *testing.T) { @@ -29,5 +42,87 @@ func TestDataAwsCrossAccountPolicy_WithPassRoles(t *testing.T) { }.Apply(t) assert.NoError(t, err) j := d.Get("json") - assert.Lenf(t, j, 3476, "Strange length for policy: %s", j) + assert.Lenf(t, j, 3168, "Strange length for policy: %s", j) +} + +func TestDataAwsCrossAccountRestrictedPolicy(t *testing.T) { + d, err := qa.ResourceFixture{ + Read: true, + Resource: DataAwsCrossaccountPolicy(), + NonWritable: true, + HCL: ` + policy_type = "restricted" + aws_account_id = "123456789012" + vpc_id = "vpc-12345678" + region = "us-west-2" + security_group_id = "sg-12345678"`, + ID: ".", + }.Apply(t) + assert.NoError(t, err) + j := d.Get("json") + assert.Lenf(t, j, 5691, "Strange length for policy: %s", j) +} + +func TestDataAwsCrossAccountInvalidPolicy(t *testing.T) { + qa.ResourceFixture{ + Read: true, + Resource: DataAwsCrossaccountPolicy(), + NonWritable: true, + HCL: `policy_type = "something"`, + ID: ".", + }.ExpectError(t, "policy_type must be either 'managed', 'customer' or 'restricted'") +} + +func TestDataAwsCrossAccountInvalidAccountId(t *testing.T) { + qa.ResourceFixture{ + Read: true, + Resource: DataAwsCrossaccountPolicy(), + NonWritable: true, + HCL: ` + policy_type = "restricted" + aws_account_id = "12345678901212"`, + ID: ".", + }.ExpectError(t, "aws_account_id must be a 12 digit number") +} + +func TestDataAwsCrossAccountInvalidVpcId(t *testing.T) { + qa.ResourceFixture{ + Read: true, + Resource: DataAwsCrossaccountPolicy(), + NonWritable: true, + HCL: ` + policy_type = "restricted" + aws_account_id = "123456789012" + vpc_id = "2345678"`, + ID: ".", + }.ExpectError(t, "vpc_id must begin with 'vpc-'") +} + +func TestDataAwsCrossAccountMissingRegion(t *testing.T) { + qa.ResourceFixture{ + Read: true, + Resource: DataAwsCrossaccountPolicy(), + NonWritable: true, + HCL: ` + policy_type = "restricted" + aws_account_id = "123456789012" + vpc_id = "vpc-12345678" + security_group_id = "sg-12345678"`, + ID: ".", + }.ExpectError(t, "region must be set") +} + +func TestDataAwsCrossAccountInvalidSgGroup(t *testing.T) { + qa.ResourceFixture{ + Read: true, + Resource: DataAwsCrossaccountPolicy(), + NonWritable: true, + HCL: ` + policy_type = "restricted" + aws_account_id = "123456789012" + vpc_id = "vpc-12345678" + region = "us-west-2" + security_group_id = "12345678"`, + ID: ".", + }.ExpectError(t, "security_group_id must begin with 'sg-'") } diff --git a/docs/data-sources/aws_crossaccount_policy.md b/docs/data-sources/aws_crossaccount_policy.md index 05835954d9..5bdd085860 100644 --- a/docs/data-sources/aws_crossaccount_policy.md +++ b/docs/data-sources/aws_crossaccount_policy.md @@ -15,7 +15,13 @@ data "databricks_aws_crossaccount_policy" "this" {} ## Argument Reference +* `policy_type` (Optional) The type of cross account policy to generated: `managed` for Databricks-managed VPC and `customer` for customer-managed VPC, `restricted` for customer-managed VPC with policy restrictions * `pass_roles` (Optional) (List) List of Data IAM role ARNs that are explicitly granted `iam:PassRole` action. +The below arguments are only valid for `restricted` policy type +* `aws_account_id` — Your AWS account ID, which is a number. +* `vpc_id` — ID of the AWS VPC where you want to launch workspaces. +* `region` — AWS Region name for your VPC deployment, for example `us-west-2`. +* `security_group_id` — ID of your AWS security group. When you add a security group restriction, you cannot reuse the cross-account IAM role or reference a credentials ID (`credentials_id`) for any other workspaces. For those other workspaces, you must create separate roles, policies, and credentials objects. ## Attribute Reference diff --git a/docs/guides/aws-workspace.md b/docs/guides/aws-workspace.md index ae454a20fe..068f985465 100644 --- a/docs/guides/aws-workspace.md +++ b/docs/guides/aws-workspace.md @@ -93,6 +93,7 @@ resource "aws_iam_role" "cross_account_role" { } data "databricks_aws_crossaccount_policy" "this" { + policy_type = "customer" } resource "aws_iam_role_policy" "this" {