Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Aws Flow module #10

Closed
wants to merge 6 commits into from
Closed

Aws Flow module #10

wants to merge 6 commits into from

Conversation

shireesh-illumio
Copy link
Collaborator

  • fixed issue in aws_account example
  • changed aws_account external id generator to random_password to not show it in the plan/apply/destroy making it sensitive
  • example for aws flow usage.

version bump 1.2.0

shireesh-illumio and others added 3 commits October 4, 2024 14:16
- fixed issue in aws_account example
- changed aws_account external id generator to random_password to not show it in the plan/apply/destroy making it sensitive
- example for aws flow usage.

version bump 1.2.0
@shireesh-illumio shireesh-illumio changed the title - added aws flows module init Aws Flow module Oct 4, 2024
Copy link
Collaborator

@rlenglet rlenglet left a comment

Choose a reason for hiding this comment

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

All those comments are addressed in the PR I have been working on: #11


resource "aws_iam_role_policy" "IllumioCloudBucketListPolicy" {
count = length(local.flow_logs_bucket_list) > 0 ? 1 : 0
name = "IllumioCloudBucketListPolicy"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't hardcode names. Follow the same pattern as for aws_account.

Comment on lines 30 to 32
data "aws_iam_role" "illumio_cloud_integration_role" {
name = var.role_arn
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just modify aws_acocunt to output the role ID instead of the ARN.


resource "aws_iam_role_policy" "IllumioCloudBucketReadPolicy" {
count = length(local.flow_logs_bucket_read) > 0 ? 1 : 0
name = "IllumioCloudBucketReadPolicy"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't hardcode names. Follow the same pattern as for aws_account.

Comment on lines +11 to +14
random = {
source = "hashicorp/random"
version = "~> 3.0"
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove. Not needed.

Suggested change
random = {
source = "hashicorp/random"
version = "~> 3.0"
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Keep the names consistent with the Terraform provider's resources.
Rename this module into aws_flow_logs_s3_buckets.

Comment on lines +1 to +16
terraform {
required_providers {
illumio-cloudsecure = {
source = "illumio/illumio-cloudsecure"
version = "~> 1.0.11"
}
aws = {
source = "hashicorp/aws"
version = "~> 3.0"
}
random = {
source = "hashicorp/random"
version = "~> 3.0"
}
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Move this into a separate versions.tf file to follow the standard conventions.


# Use the full ARN with path for GetObject actions
flow_logs_bucket_read = [
for bucket_arn in var.flow_logs_bucket_arns : "${bucket_arn}/*"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Also replace //* with /* as in the original CloudFormation Template.

Comment on lines +37 to +66
role = data.aws_iam_role.illumio_cloud_integration_role.id
policy = jsonencode({
Version = "2012-10-17"
Statement = [{
Effect = "Allow"
Sid = "IllumioBucketListAccess"
Action = [
"s3:ListBucket",
"s3:ListBucketVersion",
"s3:GetBucketLocation"
]
Resource = local.flow_logs_bucket_list
}]
})
}

resource "aws_iam_role_policy" "IllumioCloudBucketReadPolicy" {
count = length(local.flow_logs_bucket_read) > 0 ? 1 : 0
name = "IllumioCloudBucketReadPolicy"
role = data.aws_iam_role.illumio_cloud_integration_role.id
policy = jsonencode({
Version = "2012-10-17"
Statement = [{
Effect = "Allow"
Sid = "IllumioBucketReadAccess"
Action = ["s3:GetObject"]
Resource = local.flow_logs_bucket_read
}]
})
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just define one policy with two statements.

@@ -0,0 +1,17 @@
variable "flow_logs_bucket_arns" {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Keep the naming of variables with the naming of attributes of the provider resource:
https://github.com/illumio/terraform-provider-illumio-cloudsecure/blob/010d3d1dc7ac76005ab893aa6d4064968f4ae533/api/schema/aws_flow_logs_s3_bucket.go#L32

Suggested change
variable "flow_logs_bucket_arns" {
variable "s3_bucket_arns" {

}

resource "aws_iam_role_policy" "IllumioCloudBucketReadPolicy" {
count = length(local.flow_logs_bucket_read) > 0 ? 1 : 0
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove. The flow_logs_bucket_arns variable is validated to never be empty.

Suggested change
count = length(local.flow_logs_bucket_read) > 0 ? 1 : 0

}

resource "aws_iam_role_policy" "IllumioCloudBucketListPolicy" {
count = length(local.flow_logs_bucket_list) > 0 ? 1 : 0
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove. The flow_logs_bucket_arns variable is validated to never be empty.

Suggested change
count = length(local.flow_logs_bucket_list) > 0 ? 1 : 0


module "aws_flows_dev" {
source = "github.com/illumio/terraform-illumio-cloudsecure//modules/aws_flows?ref=v1.2.0"
role_arn = var.illumio_cloudsecure_role_arn
Copy link
Collaborator

Choose a reason for hiding this comment

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

Modify this example to add an aws_account to show how those should be used together, instead of taking the role ARN as a variable.

Comment on lines +1 to +9
variable "flow_logs_bucket_arns" {
type = list(string)
description = "List of S3 buckets having flow logs"
validation {
condition = length(var.flow_logs_bucket_arns) > 0
error_message = "The flow_logs_bucket_arns value must not be empty."
}
}

Copy link
Collaborator

@rlenglet rlenglet Oct 4, 2024

Choose a reason for hiding this comment

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

Remove this. Give a concrete list of literal ARNs in the example to illustrate how to use the module.

Suggested change
variable "flow_logs_bucket_arns" {
type = list(string)
description = "List of S3 buckets having flow logs"
validation {
condition = length(var.flow_logs_bucket_arns) > 0
error_message = "The flow_logs_bucket_arns value must not be empty."
}
}

You can see how we removed all variables other than client_id and client_secret from the other examples.

Comment on lines +29 to +37
variable "illumio_cloudsecure_role_arn" {
type = string
description = "The ARN of the IAM role granted to the CloudSecure account."
validation {
condition = length(var.illumio_cloudsecure_role_arn) > 1
error_message = "The illumio_cloudsecure_role_arn value must not be empty."
}
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove this. Add an aws_account into the example to illustrate how those two modules should be used together.

Suggested change
variable "illumio_cloudsecure_role_arn" {
type = string
description = "The ARN of the IAM role granted to the CloudSecure account."
validation {
condition = length(var.illumio_cloudsecure_role_arn) > 1
error_message = "The illumio_cloudsecure_role_arn value must not be empty."
}
}

You can see how we removed all variables other than client_id and client_secret from the other examples.

@shireesh-illumio
Copy link
Collaborator Author

closing as covered in #11

@shireesh-illumio shireesh-illumio deleted the aws_flows_module branch October 4, 2024 23:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants