Skip to content
New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Workspaces Directory Access Properties #16688

Merged
merged 8 commits into from
Jan 13, 2021
Merged

Workspaces Directory Access Properties #16688

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
1ea2aff
aws_workspaces_directory: Add access properties
Tensho Dec 10, 2020
dda6087
data aws_workspaces_directory: Add access properties
Tensho Dec 10, 2020
2656870
data aws_workspaces_directory: Add missed workspace_creation_properties
Tensho Dec 10, 2020
0b2efc3
Fix resource name interpolation into tags in test configs
Tensho Dec 10, 2020
6a78fdc
Shift workspace_access_attributes from bool to string type
Tensho Dec 10, 2020
ea785d2
Test workspace_access_properties default values
Tensho Dec 10, 2020
412f7de
Add acctest Name tag to all test workspaces
Tensho Dec 10, 2020
408b892
Remove Otional and MaxItems from data source schema
Tensho Dec 14, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 44 additions & 10 deletions aws/data_source_aws_workspaces_directory.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,36 +86,66 @@ func dataSourceAwsWorkspacesDirectory() *schema.Resource {
Elem: &schema.Schema{Type: schema.TypeString},
},
"tags": tagsSchema(),
"workspace_access_properties": {
Type: schema.TypeList,
Computed: true,
Elem: &schema.Resource{
Schema: map[string]*schema.Schema{
"device_type_android": {
Type: schema.TypeString,
Computed: true,
},
"device_type_chromeos": {
Type: schema.TypeString,
Computed: true,
},
"device_type_ios": {
Type: schema.TypeString,
Computed: true,
},
"device_type_osx": {
Type: schema.TypeString,
Computed: true,
},
"device_type_web": {
Type: schema.TypeString,
Computed: true,
},
"device_type_windows": {
Type: schema.TypeString,
Computed: true,
},
"device_type_zeroclient": {
Type: schema.TypeString,
Computed: true,
},
},
},
},
"workspace_creation_properties": {
Type: schema.TypeList,
Computed: true,
Optional: true,
MaxItems: 1,
Elem: &schema.Resource{
Schema: map[string]*schema.Schema{
"custom_security_group_id": {
Type: schema.TypeString,
Optional: true,
Computed: true,
},
"default_ou": {
Type: schema.TypeString,
Optional: true,
Computed: true,
},
"enable_internet_access": {
Type: schema.TypeBool,
Optional: true,
Default: false,
Computed: true,
},
"enable_maintenance_mode": {
Type: schema.TypeBool,
Optional: true,
Default: false,
Computed: true,
},
"user_enabled_as_local_administrator": {
Type: schema.TypeBool,
Optional: true,
Default: false,
Computed: true,
},
},
},
Expand Down Expand Up @@ -161,6 +191,10 @@ func dataSourceAwsWorkspacesDirectoryRead(d *schema.ResourceData, meta interface
return fmt.Errorf("error setting self_service_permissions: %s", err)
}

if err := d.Set("workspace_access_properties", flattenWorkspaceAccessProperties(directory.WorkspaceAccessProperties)); err != nil {
return fmt.Errorf("error setting workspace_access_properties: %w", err)
}

if err := d.Set("workspace_creation_properties", flattenWorkspaceCreationProperties(directory.WorkspaceCreationProperties)); err != nil {
return fmt.Errorf("error setting workspace_creation_properties: %s", err)
}
Expand Down
44 changes: 42 additions & 2 deletions aws/data_source_aws_workspaces_directory_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package aws

import (
"fmt"
"testing"

"github.com/hashicorp/terraform-plugin-sdk/v2/helper/acctest"
Expand Down Expand Up @@ -34,6 +35,14 @@ func TestAccDataSourceAwsWorkspacesDirectory_basic(t *testing.T) {
resource.TestCheckResourceAttrPair(dataSourceName, "self_service_permissions.0.rebuild_workspace", resourceName, "self_service_permissions.0.rebuild_workspace"),
resource.TestCheckResourceAttrPair(dataSourceName, "self_service_permissions.0.restart_workspace", resourceName, "self_service_permissions.0.restart_workspace"),
resource.TestCheckResourceAttrPair(dataSourceName, "self_service_permissions.0.switch_running_mode", resourceName, "self_service_permissions.0.switch_running_mode"),
resource.TestCheckResourceAttrPair(dataSourceName, "workspace_access_properties.#", resourceName, "workspace_access_properties.#"),
resource.TestCheckResourceAttrPair(dataSourceName, "workspace_access_properties.0.device_type_android", resourceName, "workspace_access_properties.0.device_type_android"),
resource.TestCheckResourceAttrPair(dataSourceName, "workspace_access_properties.0.device_type_chromeos", resourceName, "workspace_access_properties.0.device_type_chromeos"),
resource.TestCheckResourceAttrPair(dataSourceName, "workspace_access_properties.0.device_type_ios", resourceName, "workspace_access_properties.0.device_type_ios"),
resource.TestCheckResourceAttrPair(dataSourceName, "workspace_access_properties.0.device_type_osx", resourceName, "workspace_access_properties.0.device_type_osx"),
resource.TestCheckResourceAttrPair(dataSourceName, "workspace_access_properties.0.device_type_web", resourceName, "workspace_access_properties.0.device_type_web"),
resource.TestCheckResourceAttrPair(dataSourceName, "workspace_access_properties.0.device_type_windows", resourceName, "workspace_access_properties.0.device_type_windows"),
resource.TestCheckResourceAttrPair(dataSourceName, "workspace_access_properties.0.device_type_zeroclient", resourceName, "workspace_access_properties.0.device_type_zeroclient"),
resource.TestCheckResourceAttrPair(dataSourceName, "subnet_ids.#", resourceName, "subnet_ids.#"),
resource.TestCheckResourceAttrPair(dataSourceName, "tags.%", resourceName, "tags.%"),
resource.TestCheckResourceAttrPair(dataSourceName, "workspace_creation_properties.#", resourceName, "workspace_creation_properties.#"),
Expand All @@ -52,7 +61,16 @@ func TestAccDataSourceAwsWorkspacesDirectory_basic(t *testing.T) {
func testAccDataSourceAwsWorkspacesDirectoryConfig(rName string) string {
return composeConfig(
testAccAwsWorkspacesDirectoryConfig_Prerequisites(rName),
`
fmt.Sprintf(`
resource "aws_security_group" "test" {
name = "tf-testacc-workspaces-directory-%[1]s"
vpc_id = aws_vpc.main.id

tags = {
Name = "tf-testacc-workspaces-directory-%[1]s"
}
}

resource "aws_workspaces_directory" "test" {
directory_id = aws_directory_service_directory.main.id

Expand All @@ -63,6 +81,28 @@ resource "aws_workspaces_directory" "test" {
restart_workspace = false
switch_running_mode = true
}

workspace_access_properties {
device_type_android = "ALLOW"
device_type_chromeos = "ALLOW"
device_type_ios = "ALLOW"
device_type_osx = "ALLOW"
device_type_web = "DENY"
device_type_windows = "DENY"
device_type_zeroclient = "DENY"
}

workspace_creation_properties {
custom_security_group_id = aws_security_group.test.id
default_ou = "OU=AWS,DC=Workgroup,DC=Example,DC=com"
enable_internet_access = true
enable_maintenance_mode = false
user_enabled_as_local_administrator = false
}

tags = {
Name = "tf-testacc-workspaces-directory-%[1]s"
}
}

data "aws_workspaces_directory" "test" {
Expand All @@ -72,5 +112,5 @@ data "aws_workspaces_directory" "test" {
data "aws_iam_role" "workspaces-default" {
name = "workspaces_DefaultRole"
}
`)
`, rName))
}
136 changes: 135 additions & 1 deletion aws/resource_aws_workspaces_directory.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"github.com/aws/aws-sdk-go/aws"
"github.com/aws/aws-sdk-go/service/workspaces"
"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/keyvaluetags"
"github.com/terraform-providers/terraform-provider-aws/aws/internal/service/workspaces/waiter"
)
Expand Down Expand Up @@ -104,6 +105,51 @@ func resourceAwsWorkspacesDirectory() *schema.Resource {
Elem: &schema.Schema{Type: schema.TypeString},
},
"tags": tagsSchema(),
"workspace_access_properties": {
Type: schema.TypeList,
Computed: true,
Optional: true,
MaxItems: 1,
Elem: &schema.Resource{
Schema: map[string]*schema.Schema{
"device_type_android": {
Copy link
Collaborator

Choose a reason for hiding this comment

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

lets keep the API structure here and use string and validate values.

Copy link
Contributor Author

@Tensho Tensho Dec 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I don't mind, but in this case self_service_permissions block attributes should be changed too as far as they have a similar semantic. If it makes sense to you, please could you suggest the best way to handle self_service_permissions backward compatibility?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It may require a breaking change in the future but lets keep to the scope of this PR as this is a new block added. from my experience maintainers usually prefer(and i agree) to keep the original type used in the SDK. we dont know the reason why this was not bool and in the future this opens a door for more values (as unlikely as it looks now) and it means breaking this interface or adding extra arguments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Make total sense to me. Thank you, Ilia 🙇 Revamping...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Contributor Author

@Tensho Tensho Dec 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Indeed API doesn't specify the defaults, here is my best effort to guess the sensible values. Is it OK to rely on the current non-specified default values from API response? I mean they may be changed in the future by AWS.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, they are defacto defaults. they still may change but more unlikely. just adding the test to basic would be fine we can use optional with default. i would avoid computed to catch config drift. so the way it was before is good just keep the check for basic test as well.

sorry for the multiple cycles if i wasnt clear.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No worries, I appreciate your thoughtful review 🙇

  • Removed Default schema options in favor of API default values
  • Added default values assertions to the basic test

Could you elaborate on Computed schema option removal? If I understand correctly, user may skip workspace_access_properties attribute configuration and API returns default values, which are populated to state on Read. This kind of behavior should be marked as Computed. Also, basic acc test should contain assertions exactly for Computed + Optional attributes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Doesnt the current setting cause config drift?
there should be a computed: true or default value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

workspace_access_properties doesn't have a default value right now. The plan is empty at the end of each test step.

Type: schema.TypeString,
Optional: true,
ValidateFunc: validation.StringInSlice(workspaces.AccessPropertyValue_Values(), false),
},
"device_type_chromeos": {
Type: schema.TypeString,
Optional: true,
ValidateFunc: validation.StringInSlice(workspaces.AccessPropertyValue_Values(), false),
},
"device_type_ios": {
Type: schema.TypeString,
Optional: true,
ValidateFunc: validation.StringInSlice(workspaces.AccessPropertyValue_Values(), false),
},
"device_type_osx": {
Type: schema.TypeString,
Optional: true,
ValidateFunc: validation.StringInSlice(workspaces.AccessPropertyValue_Values(), false),
},
"device_type_web": {
Type: schema.TypeString,
Optional: true,
ValidateFunc: validation.StringInSlice(workspaces.AccessPropertyValue_Values(), false),
},
"device_type_windows": {
Type: schema.TypeString,
Optional: true,
ValidateFunc: validation.StringInSlice(workspaces.AccessPropertyValue_Values(), false),
},
"device_type_zeroclient": {
Type: schema.TypeString,
Optional: true,
ValidateFunc: validation.StringInSlice(workspaces.AccessPropertyValue_Values(), false),
},
},
},
},
"workspace_creation_properties": {
Type: schema.TypeList,
Computed: true,
Expand Down Expand Up @@ -184,11 +230,23 @@ func resourceAwsWorkspacesDirectoryCreate(d *schema.ResourceData, meta interface
SelfservicePermissions: expandSelfServicePermissions(v.([]interface{})),
})
if err != nil {
return fmt.Errorf("error setting WorkSpaces Directory (%s) self service permissions: %w", directoryID, err)
return fmt.Errorf("error setting WorkSpaces Directory (%s) self-service permissions: %w", directoryID, err)
}
log.Printf("[INFO] Modified WorkSpaces Directory (%s) self-service permissions", directoryID)
}

if v, ok := d.GetOk("workspace_access_properties"); ok {
log.Printf("[DEBUG] Modifying WorkSpaces Directory (%s) access properties", directoryID)
_, err := conn.ModifyWorkspaceAccessProperties(&workspaces.ModifyWorkspaceAccessPropertiesInput{
ResourceId: aws.String(directoryID),
WorkspaceAccessProperties: expandWorkspaceAccessProperties(v.([]interface{})),
})
if err != nil {
return fmt.Errorf("error setting WorkSpaces Directory (%s) access properties: %w", directoryID, err)
}
log.Printf("[INFO] Modified WorkSpaces Directory (%s) access properties", directoryID)
}

if v, ok := d.GetOk("workspace_creation_properties"); ok {
log.Printf("[DEBUG] Modifying WorkSpaces Directory (%s) creation properties", directoryID)
_, err := conn.ModifyWorkspaceCreationProperties(&workspaces.ModifyWorkspaceCreationPropertiesInput{
Expand Down Expand Up @@ -247,6 +305,10 @@ func resourceAwsWorkspacesDirectoryRead(d *schema.ResourceData, meta interface{}
return fmt.Errorf("error setting self_service_permissions: %w", err)
}

if err := d.Set("workspace_access_properties", flattenWorkspaceAccessProperties(directory.WorkspaceAccessProperties)); err != nil {
return fmt.Errorf("error setting workspace_access_properties: %w", err)
}

if err := d.Set("workspace_creation_properties", flattenWorkspaceCreationProperties(directory.WorkspaceCreationProperties)); err != nil {
return fmt.Errorf("error setting workspace_creation_properties: %w", err)
}
Expand Down Expand Up @@ -288,6 +350,20 @@ func resourceAwsWorkspacesDirectoryUpdate(d *schema.ResourceData, meta interface
log.Printf("[INFO] Modified WorkSpaces Directory (%s) self-service permissions", d.Id())
}

if d.HasChange("workspace_access_properties") {
log.Printf("[DEBUG] Modifying WorkSpaces Directory (%s) access properties", d.Id())
properties := d.Get("workspace_access_properties").([]interface{})

_, err := conn.ModifyWorkspaceAccessProperties(&workspaces.ModifyWorkspaceAccessPropertiesInput{
ResourceId: aws.String(d.Id()),
WorkspaceAccessProperties: expandWorkspaceAccessProperties(properties),
})
if err != nil {
return fmt.Errorf("error updating WorkSpaces Directory (%s) access properties: %w", d.Id(), err)
}
log.Printf("[INFO] Modified WorkSpaces Directory (%s) access properties", d.Id())
}

if d.HasChange("workspace_creation_properties") {
log.Printf("[DEBUG] Modifying WorkSpaces Directory (%s) creation properties", d.Id())
properties := d.Get("workspace_creation_properties").([]interface{})
Expand Down Expand Up @@ -370,6 +446,46 @@ func workspacesDirectoryDelete(id string, conn *workspaces.WorkSpaces) error {
return nil
}

func expandWorkspaceAccessProperties(properties []interface{}) *workspaces.WorkspaceAccessProperties {
if len(properties) == 0 || properties[0] == nil {
return nil
}

result := &workspaces.WorkspaceAccessProperties{}

p := properties[0].(map[string]interface{})

if p["device_type_android"].(string) != "" {
result.DeviceTypeAndroid = aws.String(p["device_type_android"].(string))
}

if p["device_type_chromeos"].(string) != "" {
result.DeviceTypeChromeOs = aws.String(p["device_type_chromeos"].(string))
}

if p["device_type_ios"].(string) != "" {
result.DeviceTypeIos = aws.String(p["device_type_ios"].(string))
}

if p["device_type_osx"].(string) != "" {
result.DeviceTypeOsx = aws.String(p["device_type_osx"].(string))
}

if p["device_type_web"].(string) != "" {
result.DeviceTypeWeb = aws.String(p["device_type_web"].(string))
}

if p["device_type_windows"].(string) != "" {
result.DeviceTypeWindows = aws.String(p["device_type_windows"].(string))
}

if p["device_type_zeroclient"].(string) != "" {
result.DeviceTypeZeroClient = aws.String(p["device_type_zeroclient"].(string))
}

return result
}

func expandSelfServicePermissions(permissions []interface{}) *workspaces.SelfservicePermissions {
if len(permissions) == 0 || permissions[0] == nil {
return nil
Expand Down Expand Up @@ -436,6 +552,24 @@ func expandWorkspaceCreationProperties(properties []interface{}) *workspaces.Wor
return result
}

func flattenWorkspaceAccessProperties(properties *workspaces.WorkspaceAccessProperties) []interface{} {
if properties == nil {
return []interface{}{}
}

return []interface{}{
map[string]interface{}{
"device_type_android": aws.StringValue(properties.DeviceTypeAndroid),
"device_type_chromeos": aws.StringValue(properties.DeviceTypeChromeOs),
"device_type_ios": aws.StringValue(properties.DeviceTypeIos),
"device_type_osx": aws.StringValue(properties.DeviceTypeOsx),
"device_type_web": aws.StringValue(properties.DeviceTypeWeb),
"device_type_windows": aws.StringValue(properties.DeviceTypeWindows),
"device_type_zeroclient": aws.StringValue(properties.DeviceTypeZeroClient),
},
}
}

func flattenSelfServicePermissions(permissions *workspaces.SelfservicePermissions) []interface{} {
if permissions == nil {
return []interface{}{}
Expand Down
Loading