-
Notifications
You must be signed in to change notification settings - Fork 8
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
initial commit of unreal cloud ddc module and sample #341
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checkov found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets grab some time and you can explain a lot of the little nit-picky things. Generally looks really good!
variable "oidc_provider_arn" { | ||
type = string | ||
description = "ARN of the OIDC Provider from EKS Cluster" | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from or for EKS cluster?
variable "external_secrets_secret_manager_arn_list" { | ||
type = list(string) | ||
description = "List of ARNS for Secret Manager Secrets to use in Unreal Cloud DDC" | ||
default = [] | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are these for? How do they get used?
Your most recent merge commit should be removed. Rebase main onto this feature branch instead. Also, signing is missing on the first commit. |
#checkov:skip=CKV2_AWS_38:Ensure Domain Name System Security Extensions (DNSSEC) signing is enabled for Amazon Route 53 public hosted zones | ||
#checkov:skip=CKV2_AWS_39:Ensure Domain Name System (DNS) query logging is enabled for Amazon Route 53 hosted zones |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment should explain why a checkov check is skipped, not what is skipped.
vpc_id = var.vpc_id | ||
|
||
tags = { | ||
Name = "unreal-cloud-ddc-system-sg" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Parameterizing this name (like we do in
value = "${local.name_prefix}-${each.key}-build-farm" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same applies in other locations in this file; allowing a prefix allows deploying multiple DDCs in an account in a way which makes them easily distinguishable.
s3_bucket_id = module.unreal_cloud_ddc_infra.s3_bucket_id | ||
|
||
unreal_cloud_ddc_helm_values = [ | ||
templatefile("./assets/unreal_cloud_ddc_region_values.yaml", { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this template file is missing; is it generated or do we expect users to provide it themselves?
region = data.aws_region.current.name | ||
bucket_name = module.unreal_cloud_ddc_infra.s3_bucket_id | ||
}), | ||
templatefile("./assets/unreal_cloud_ddc_base.yaml", { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this template file is missing; is it generated or do we expect users to provide it themselves?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It still sits in modules/unreal/samples/unreal-cloud-ddc-single-region/assets
instead of the root assets
directory. I presume we still think it should migrate higher up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think Henry hit most of the important points. Added some comments for minor things.
scylla_user_data = jsonencode( | ||
{ "scylla_yaml" : { | ||
"cluster_name" : local.scylla_variables.scylla-cluster-name, | ||
"seed_provider" : [ | ||
{ "class_name" : "org.apache.cassandra.locator.SimpleSeedProvider", | ||
"parameters" : [ | ||
{ "seeds" : "test-ip" } | ||
] | ||
} | ||
] | ||
}, | ||
"start_scylla_on_first_boot" : true }) | ||
nvme-pre-bootstrap-userdata = <<EOF | ||
MIME-Version: 1.0 | ||
Content-Type: multipart/mixed; boundary="//" | ||
|
||
--// | ||
Content-Type: text/x-shellscript; charset="us-ascii" | ||
|
||
#!/bin/bash | ||
sudo mkfs.ext4 -E nodiscard /dev/nvme1n1 | ||
sudo mkdir /data | ||
sudo mount /dev/nvme1n1 /data | ||
--//--\ | ||
EOF |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
description = "Self SG Egress" | ||
} | ||
|
||
# ################################################################################ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are these commented SGs? Should they be removed if not in use?
type = list(string) | ||
default = [] | ||
description = "Private subnets you want scylla and the worker nodes to be installed into." | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Always describe exactly what you expected to be passed for a variable. For example, Subnets have a subnet ID and a ARN, so it can be confusing for new users to know which to use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Already some great feedback from others in here. For me, biggest question is EKS versioning and how we want to handle this. What prevents you from using latest EKS version (1.31 I believe)? I'd like to see a runbook of steps for testing the module for EKS version compatibility before we release this. If game devs will take a dependency on this module then we don't want our trailing EKS versioning to be a reason they cannot update their clusters to latest version or need to incur additional cost for EKS extended support for older version.
resource "aws_s3_bucket" "unreal_ddc_s3_bucket" { | ||
#checkov:skip=CKV_AWS_21:Ensure all data stored in the S3 bucket have versioning enabled | ||
#checkov:skip=CKV2_AWS_61:Ensure that an S3 bucket has a lifecycle configuration | ||
#checkov:skip=CKV2_AWS_62:This bucket doesnt have any triggers needed as its only an object store | ||
#checkov:skip=CKV_AWS_144:This bucket hosts ephemeral recreatable assets | ||
bucket_prefix = "${var.name}-s3-bucket" | ||
force_destroy = true | ||
} |
Check warning
Code scanning / checkov
Ensure S3 buckets should have event notifications enabled Warning
resource "aws_s3_bucket" "unreal_ddc_s3_bucket" { | ||
#checkov:skip=CKV_AWS_21:Ensure all data stored in the S3 bucket have versioning enabled | ||
#checkov:skip=CKV2_AWS_61:Ensure that an S3 bucket has a lifecycle configuration | ||
#checkov:skip=CKV2_AWS_62:This bucket doesnt have any triggers needed as its only an object store | ||
#checkov:skip=CKV_AWS_144:This bucket hosts ephemeral recreatable assets | ||
bucket_prefix = "${var.name}-s3-bucket" | ||
force_destroy = true | ||
} |
Check warning
Code scanning / checkov
Ensure that S3 bucket has cross-region replication enabled Warning
resource "aws_s3_bucket" "unreal_ddc_s3_bucket" { | ||
#checkov:skip=CKV_AWS_21:Ensure all data stored in the S3 bucket have versioning enabled | ||
#checkov:skip=CKV2_AWS_61:Ensure that an S3 bucket has a lifecycle configuration | ||
#checkov:skip=CKV2_AWS_62:This bucket doesnt have any triggers needed as its only an object store | ||
#checkov:skip=CKV_AWS_144:This bucket hosts ephemeral recreatable assets | ||
bucket_prefix = "${var.name}-s3-bucket" | ||
force_destroy = true | ||
} |
Check warning
Code scanning / checkov
Ensure all data stored in the S3 bucket have versioning enabled Warning
resource "aws_s3_bucket" "unreal_ddc_s3_bucket" { | ||
#checkov:skip=CKV_AWS_21:Ensure all data stored in the S3 bucket have versioning enabled | ||
#checkov:skip=CKV2_AWS_61:Ensure that an S3 bucket has a lifecycle configuration | ||
#checkov:skip=CKV2_AWS_62:This bucket doesnt have any triggers needed as its only an object store | ||
#checkov:skip=CKV_AWS_144:This bucket hosts ephemeral recreatable assets | ||
bucket_prefix = "${var.name}-s3-bucket" | ||
force_destroy = true | ||
} |
Check warning
Code scanning / checkov
Ensure that an S3 bucket has a lifecycle configuration Warning
…arsing issue on sample
@danielw-aws you need to address the existing requested changes (close out conversations) and re-request reviews for this. We're still failing checkov as well :) |
@danielw-aws any updates here that you want me to review? |
resource "aws_eks_cluster" "unreal_cloud_ddc_eks_cluster" { | ||
#checkov:skip=CKV_AWS_39:Ensure Amazon EKS public endpoint disabled | ||
#checkov:skip=CKV_AWS_58:Ensure EKS Cluster has Secrets Encryption Enabled | ||
#checkov:skip=CKV_AWS_339:Ensure EKS clusters run on a supported Kubernetes version | ||
#checkov:skip=CKV_AWS_38:IP restriction set in module variables | ||
name = var.name | ||
role_arn = aws_iam_role.eks_cluster_role.arn | ||
version = var.kubernetes_version | ||
enabled_cluster_log_types = ["api", "audit", "authenticator", "controllerManager", "scheduler"] | ||
|
||
|
||
|
||
access_config { | ||
authentication_mode = "API_AND_CONFIG_MAP" | ||
bootstrap_cluster_creator_admin_permissions = true | ||
} | ||
|
||
vpc_config { | ||
subnet_ids = var.private_subnets | ||
endpoint_private_access = true | ||
endpoint_public_access = true | ||
public_access_cidrs = var.eks_cluster_access_cidr | ||
security_group_ids = [ | ||
aws_security_group.system_security_group.id, | ||
aws_security_group.worker_security_group.id, | ||
aws_security_group.nvme_security_group.id | ||
] | ||
} | ||
} |
Check warning
Code scanning / checkov
Ensure Amazon EKS public endpoint not accessible to 0.0.0.0/0 Warning
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As long as the access is restricted by SG and defaulted to empty access we can suppress this.
resource "aws_cloudwatch_log_group" "unreal_cluster_cloudwatch" { | ||
#checkov:skip=CKV_AWS_158:Ensure that CloudWatch Log Group is encrypted by KMS | ||
name_prefix = "/aws/eks/${var.name}/cluster" | ||
retention_in_days = 365 | ||
|
||
} |
Check warning
Code scanning / checkov
Ensure that CloudWatch Log Group is encrypted by KMS Warning
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
module "eks_blueprints_all_other_addons" { | ||
#checkov:skip=CKV_TF_1:Ensure Terraform module sources use a commit hash | ||
#checkov:skip=CKV_AWS_109:Ensure IAM policies does not allow permissions management / resource exposure without constraints | ||
#checkov:skip=CKV_AWS_111:Ensure IAM policies does not allow write access without constraints | ||
#checkov:skip=CKV_AWS_356:Ensure no IAM policies documents allow "*" as a statement's resource for restrictable actions | ||
source = "git::https://github.com/aws-ia/terraform-aws-eks-blueprints-addons.git?ref=a9963f4a0e168f73adb033be594ac35868696a91" | ||
|
||
eks_addons = { | ||
coredns = { | ||
most_recent = true | ||
} | ||
kube-proxy = { | ||
most_recent = true | ||
} | ||
vpc-cni = { | ||
most_recent = true | ||
} | ||
aws-ebs-csi-driver = { | ||
most_recent = true | ||
service_account_role_arn = module.ebs_csi_irsa_role.iam_role_arn | ||
} | ||
} | ||
|
||
|
||
cluster_name = data.aws_eks_cluster.unreal_cloud_ddc_cluster.name | ||
cluster_endpoint = data.aws_eks_cluster.unreal_cloud_ddc_cluster.endpoint | ||
cluster_version = data.aws_eks_cluster.unreal_cloud_ddc_cluster.version | ||
oidc_provider_arn = data.aws_iam_openid_connect_provider.oidc_provider.arn | ||
|
||
enable_aws_load_balancer_controller = true | ||
enable_aws_cloudwatch_metrics = true | ||
|
||
tags = { | ||
Environment = var.cluster_name | ||
} | ||
} |
Check warning
Code scanning / checkov
Ensure Terraform module sources use a commit hash Warning
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More updates to address - include comments ASAP, and we can divy up what needs to be fixed. I'll hop on this as well.
name = var.name | ||
role_arn = aws_iam_role.eks_cluster_role.arn | ||
version = var.kubernetes_version | ||
enabled_cluster_log_types = ["api", "audit", "authenticator", "controllerManager", "scheduler"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
parameterize?
version = aws_eks_cluster.unreal_cloud_ddc_eks_cluster.version | ||
release_version = nonsensitive(data.aws_ssm_parameter.eks_ami_latest_release.value) | ||
node_role_arn = aws_iam_role.worker_node_group_role.arn | ||
subnet_ids = var.private_subnets |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the solution be subnet type "agnostic"? Meaning these don't HAVE to be private, you just recommend it when deploying the module?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure ill parameterize this
bucket_name = module.unreal_cloud_ddc_infra.s3_bucket_id | ||
}), | ||
templatefile("./assets/unreal_cloud_ddc_base.yaml", { | ||
scylla_dns = "scylla.example.com" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an internal private hosted zone created BY the module. I also see a problem with this in situations where networking is handled by a distinct account from the DDC deployment account (cross-account VPC hosted zone association)
okta_domain = var.okta_domain | ||
okta_auth_server_id = var.okta_auth_server_id | ||
jwt_audience = var.jwt_audience | ||
jwt_authority = var.jwt_authority | ||
okta_domain = jsondecode(data.aws_secretsmanager_secret_version.current.secret_string)["okta_domain"] | ||
okta_auth_server_id = jsondecode(data.aws_secretsmanager_secret_version.current.secret_string)["okta_auth_server_id"] | ||
jwt_audience = jsondecode(data.aws_secretsmanager_secret_version.current.secret_string)["jwt_audience"] | ||
jwt_authority = jsondecode(data.aws_secretsmanager_secret_version.current.secret_string)["jwt_authority"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am confused by this block.
okta_domain = jsondecode(data.aws_secretsmanager_secret_version.current.secret_string)["okta_domain"] | ||
okta_auth_server_id = jsondecode(data.aws_secretsmanager_secret_version.current.secret_string)["okta_auth_server_id"] | ||
jwt_audience = jsondecode(data.aws_secretsmanager_secret_version.current.secret_string)["jwt_audience"] | ||
jwt_authority = jsondecode(data.aws_secretsmanager_secret_version.current.secret_string)["jwt_authority"] | ||
}), | ||
file("./assets/unreal_cloud_ddc_values.yaml") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would rather see us use the Terraform path object
variable "oidc_credential_arn" { | ||
type = string | ||
sensitive = true | ||
description = "Okta Domain" | ||
description = "OIDC Secrets Credential ARN" | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better description and instructions for creating this secret
variable "eks_cluster_ip_allow_list" { | ||
type = list(string) | ||
default = [] | ||
description = "IPs that will be allow listed to access cluster over internet" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be managed externally. Cluster SG ID is output from the module. Rules are created at the sample level.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this is so terraform can set up the eks cluster. if this is not enabled we have to have a server or bastion running in a private subnet which is hard to showcase with an example. Lets talk about how to fix this
resource "aws_eks_cluster" "unreal_cloud_ddc_eks_cluster" { | ||
#checkov:skip=CKV_AWS_39:This needs to be open to configure the eks cluster. | ||
#checkov:skip=CKV_AWS_58:This will need to be enabled in a future update | ||
#checkov:skip=CKV_AWS_38:IP restriction set in module variables | ||
name = var.name | ||
role_arn = aws_iam_role.eks_cluster_role.arn | ||
version = var.kubernetes_version | ||
enabled_cluster_log_types = ["api", "audit", "authenticator", "controllerManager", "scheduler"] | ||
|
||
|
||
|
||
access_config { | ||
authentication_mode = "API_AND_CONFIG_MAP" | ||
bootstrap_cluster_creator_admin_permissions = true | ||
} | ||
|
||
vpc_config { | ||
subnet_ids = var.private_subnets | ||
endpoint_private_access = true | ||
endpoint_public_access = true | ||
public_access_cidrs = var.eks_cluster_access_cidr | ||
security_group_ids = [ | ||
aws_security_group.system_security_group.id, | ||
aws_security_group.worker_security_group.id, | ||
aws_security_group.nvme_security_group.id | ||
] | ||
} | ||
} |
Check warning
Code scanning / checkov
Ensure Amazon EKS public endpoint not accessible to 0.0.0.0/0 Warning
resource "aws_eks_cluster" "unreal_cloud_ddc_eks_cluster" { | ||
#checkov:skip=CKV_AWS_39:This needs to be open to configure the eks cluster. | ||
#checkov:skip=CKV_AWS_58:This will need to be enabled in a future update | ||
#checkov:skip=CKV_AWS_38:IP restriction set in module variables | ||
name = var.name | ||
role_arn = aws_iam_role.eks_cluster_role.arn | ||
version = var.kubernetes_version | ||
enabled_cluster_log_types = ["api", "audit", "authenticator", "controllerManager", "scheduler"] | ||
|
||
|
||
|
||
access_config { | ||
authentication_mode = "API_AND_CONFIG_MAP" | ||
bootstrap_cluster_creator_admin_permissions = true | ||
} | ||
|
||
vpc_config { | ||
subnet_ids = var.private_subnets | ||
endpoint_private_access = true | ||
endpoint_public_access = true | ||
public_access_cidrs = var.eks_cluster_access_cidr | ||
security_group_ids = [ | ||
aws_security_group.system_security_group.id, | ||
aws_security_group.worker_security_group.id, | ||
aws_security_group.nvme_security_group.id | ||
] | ||
} | ||
} |
Check warning
Code scanning / checkov
Ensure Amazon EKS public endpoint disabled Warning
resource "aws_eks_cluster" "unreal_cloud_ddc_eks_cluster" { | ||
#checkov:skip=CKV_AWS_39:This needs to be open to configure the eks cluster. | ||
#checkov:skip=CKV_AWS_58:This will need to be enabled in a future update | ||
#checkov:skip=CKV_AWS_38:IP restriction set in module variables | ||
name = var.name | ||
role_arn = aws_iam_role.eks_cluster_role.arn | ||
version = var.kubernetes_version | ||
enabled_cluster_log_types = ["api", "audit", "authenticator", "controllerManager", "scheduler"] | ||
|
||
|
||
|
||
access_config { | ||
authentication_mode = "API_AND_CONFIG_MAP" | ||
bootstrap_cluster_creator_admin_permissions = true | ||
} | ||
|
||
vpc_config { | ||
subnet_ids = var.private_subnets | ||
endpoint_private_access = true | ||
endpoint_public_access = true | ||
public_access_cidrs = var.eks_cluster_access_cidr | ||
security_group_ids = [ | ||
aws_security_group.system_security_group.id, | ||
aws_security_group.worker_security_group.id, | ||
aws_security_group.nvme_security_group.id | ||
] | ||
} | ||
} |
Check warning
Code scanning / checkov
Ensure EKS Cluster has Secrets Encryption Enabled Warning
resource "aws_s3_bucket" "unreal_ddc_s3_bucket" { | ||
#checkov:skip=CKV_AWS_21:Ensure all data stored in the S3 bucket have versioning enabled | ||
#checkov:skip=CKV2_AWS_61:Ensure that an S3 bucket has a lifecycle configuration | ||
#checkov:skip=CKV2_AWS_62:This bucket doesnt have any triggers needed as its only an object store | ||
#checkov:skip=CKV_AWS_144:This bucket hosts ephemeral recreatable assets | ||
#checkov:skip=CKV_AWS_18:Logging bucket cna be configured by customer | ||
bucket_prefix = "${var.name}-s3-bucket" | ||
force_destroy = true | ||
} |
Check warning
Code scanning / checkov
Ensure the S3 bucket has access logging enabled Warning
Issue number:
Summary
Added single region example and Unreal Cloud DDC modules to CGD
Changes
User experience
Checklist
If your change doesn't seem to apply, please leave them unchecked.
Is this a breaking change? No
Acknowledgment
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
Disclaimer: We value your time and bandwidth. As such, any pull requests created might not be successful.