Skip to content
Open
Show file tree
Hide file tree
Changes from 6 commits
Commits
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
6 changes: 3 additions & 3 deletions FAQ.md
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
#### Does the user running this terraform need specialized knowledge of AWS services?

> Typically, to run terraform the user must know to configure awscli with the right credentials and run `terraform apply`. It is recommended
> to have a certain level of knowledge on [these list of resources](https://github.com/wandb/terraform-aws-wandb/tree/venky/add-faq-section#aws-services-used) to be able to deploy and maintain W&B successfully.
> to have a certain level of knowledge on [this list of resources](https://github.com/wandb/terraform-aws-wandb/tree/main#aws-services-used) to be able to deploy and maintain W&B successfully.

#### How does the W&B private cloud architecture look like?

Expand All @@ -28,7 +28,7 @@

#### What are the different subnets created as part of the W&B deployment?

> The W&B AWS terraform spins up atleast two subnets, one per availability zone in a region to account for fail over. If the deployment is private, then W&B creates two public subnets and two private subnets.
> The W&B AWS terraform spins up at least two subnets, one per availability zone in a region to account for failover. If the deployment is private, then W&B creates two public subnets and two private subnets.

#### What permissions should be granted to the W&B deployment?

Expand Down Expand Up @@ -84,7 +84,7 @@ You can supply your own KMS key to our terraform module via the **kms_key_alias*

> Customer sensitive data is stored in an S3 bucket. Once authenticated a user can access S3 bucket resource through pre-signed urls. The user must have access to the project to view these resources.

#### What are the different data encrytion configurations in place?
#### What are the different data encryption configurations in place?

```
- S3 bucket is encrypted by a customer defined KMS key
Expand Down
24 changes: 12 additions & 12 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ audit logging and SAML single sign-on.
This module is intended to run in an AWS account with minimal preparation,
however it does have the following pre-requisites:

### Terrafom version >= 1.9
### Terraform version >= 1.9
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Find the Terraform required_version constraint
rg -n "required_version" --include="*.tf" -A1 -B1

Repository: wandb/terraform-aws-wandb

Length of output: 510


🏁 Script executed:

# Check README.md at the mentioned lines
head -n 160 README.md | tail -n 140 | cat -n

Repository: wandb/terraform-aws-wandb

Length of output: 7136


🏁 Script executed:

# Search for required_version in terraform files with correct syntax
rg "required_version" -t tf -A1 -B1

Repository: wandb/terraform-aws-wandb

Length of output: 357


🏁 Script executed:

# Also check for any terraform blocks that might define this
rg "terraform\s*{" -t tf -A5

Repository: wandb/terraform-aws-wandb

Length of output: 158


Update auto-generated requirements documentation to reflect the actual Terraform constraint.

The required_version in versions.tf is ~> 1.9, but the auto-generated requirements table in the README (line 129, within the BEGIN_TF_DOCS section) still shows ~> 1.0. This mismatch will mislead users about the module's Terraform version requirements. Regenerate the documentation to ensure it reflects the actual constraint.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@README.md` at line 21, The auto-generated Terraform requirements table in the
README (inside the BEGIN_TF_DOCS section) shows an incorrect Terraform
constraint; regenerate the tf-docs so the README reflects the actual
required_version (~> 1.9) declared in versions.tf. Update the documentation by
running the docs generation step (e.g., terraform-docs or your repo's doc
generation script) that updates the BEGIN_TF_DOCS block so the table shows
required_version = "~> 1.9" and commit the changed README.


### Credentials / Permissions

Expand Down Expand Up @@ -55,8 +55,8 @@ resource](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/reso

### ACM Certificate

While this is not required, it is recommend to already have an existing ACM
certification. Certificate validation can take up two hours, causing timeouts
While this is not required, it is recommended to already have an existing ACM
certificate. Certificate validation can take up to two hours, causing timeouts
during module apply if the cert is generated as one of the resources contained
in the module.

Expand Down Expand Up @@ -103,14 +103,14 @@ All the values set via `deployment-size.tf` can be overridden by setting the app
- `database_instance_class` - The instance type for the database

## Bring Your Own Bucket (BYOB)
We have added additional variable that make enabling BYOB easier to enable.
We have added additional variables that make enabling BYOB easier.
`bucket_permissions_mode` accepts 1 of 3 values;
- `strict` the default requires an explict list of the buckets for proper access, the same as byob before `7.3.0`.
- `restricted` makes use of the new variable `bucket_restricted_accounts` which is a list of AWS account Id's where the BYOBs can be hosted from. ex: `["1234567890", "1234876590"]`
- `public` enables access to any BYOB properly configured not present in the the calling account. Effectively this enables cross account s3 access to ANY aws s3 account.
- `strict` the default requires an explicit list of the buckets for proper access, the same as byob before `7.3.0`.
- `restricted` makes use of the new variable `bucket_restricted_accounts` which is a list of AWS account IDs where the BYOBs can be hosted from. ex: `["1234567890", "1234876590"]`
- `public` enables access to any BYOB properly configured not present in the calling account. Effectively this enables cross account s3 access to ANY aws s3 account.
Comment on lines +106 to +110
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Minor grammar: "cross account" should be hyphenated as "cross-account" (line 110); "s3" should be capitalised "S3".

✏️ Suggested fix
-- `public` enables access to any BYOB properly configured not present in the calling account. Effectively this enables cross account s3 access to ANY aws s3 account.
+- `public` enables access to any BYOB properly configured not present in the calling account. Effectively this enables cross-account S3 access to ANY AWS S3 account.
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
We have added additional variables that make enabling BYOB easier.
`bucket_permissions_mode` accepts 1 of 3 values;
- `strict` the default requires an explict list of the buckets for proper access, the same as byob before `7.3.0`.
- `restricted` makes use of the new variable `bucket_restricted_accounts` which is a list of AWS account Id's where the BYOBs can be hosted from. ex: `["1234567890", "1234876590"]`
- `public` enables access to any BYOB properly configured not present in the the calling account. Effectively this enables cross account s3 access to ANY aws s3 account.
- `strict` the default requires an explicit list of the buckets for proper access, the same as byob before `7.3.0`.
- `restricted` makes use of the new variable `bucket_restricted_accounts` which is a list of AWS account IDs where the BYOBs can be hosted from. ex: `["1234567890", "1234876590"]`
- `public` enables access to any BYOB properly configured not present in the calling account. Effectively this enables cross account s3 access to ANY aws s3 account.
We have added additional variables that make enabling BYOB easier.
`bucket_permissions_mode` accepts 1 of 3 values;
- `strict` the default requires an explicit list of the buckets for proper access, the same as byob before `7.3.0`.
- `restricted` makes use of the new variable `bucket_restricted_accounts` which is a list of AWS account IDs where the BYOBs can be hosted from. ex: `["1234567890", "1234876590"]`
- `public` enables access to any BYOB properly configured not present in the calling account. Effectively this enables cross-account S3 access to ANY AWS S3 account.
🧰 Tools
🪛 LanguageTool

[grammar] ~110-~110: Use a hyphen to join words.
Context: ... account. Effectively this enables cross account s3 access to ANY aws s3 account....

(QB_NEW_EN_HYPHEN)

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@README.md` around lines 106 - 110, Update the README text to fix minor
grammar: hyphenate "cross-account" and capitalize "S3" where it currently reads
"cross account" and "s3" (in the explanatory text for bucket_permissions_mode /
bucket_restricted_accounts and the `public` option); keep all variable names
(`bucket_permissions_mode`, `bucket_restricted_accounts`, `BYOB`) unchanged.


> [!IMPORTANT]
> Enabling BYOB or cross-account reguardless of `bucket_permissions_mode` still requires a policy attached to that bucket to allowing the eks node role to perform s3 actions.
> Enabling BYOB or cross-account regardless of `bucket_permissions_mode` still requires a policy attached to that bucket allowing the eks node role to perform s3 actions.
Comment thread
dgreeninger-wandb marked this conversation as resolved.
Outdated
>
> To find out the role which needs to be allowed access to your BYOB go to bucket section of `https://YOUR_WANDB_DEPLOYMENT/console/settings/system` or see the output of the module `cluster_node_role`
>
Expand All @@ -123,7 +123,7 @@ installation scenarios for Weights & Biases, as well as examples for supporting
resources that lack official modules.

- [Private Access Only](https://github.com/wandb/terraform-aws-wandb/tree/main/examples/private-access-only)
- [Private Existing Network](https://github.com/wandb/terraform-aws-wandb/tree/main/examples/private-existing-network)
- [Private Existing Network](https://github.com/wandb/terraform-aws-wandb/tree/main/examples/byo-vpc)
- [Public External DNS](https://github.com/wandb/terraform-aws-wandb/tree/main/examples/public-dns-external)
- [Public Route 53 DNS](https://github.com/wandb/terraform-aws-wandb/tree/main/examples/public-dns-with-route53)

Expand Down Expand Up @@ -352,7 +352,7 @@ module "wandb" {
}
```

### Alow customer specific customer-managed keys for S3 and RDS
### Allow customer-specific customer-managed keys for S3 and RDS

- we can provide external kms key to encrypt database, redis and S3 buckets.
- To provide kms keys we need to provide kms arn values in
Expand All @@ -364,7 +364,7 @@ bucket_kms_key_arn

### In order to allow cross account KMS keys. we need to allow kms keys to be accessed by WandB account.

This can be donw by adding the following policy document.
This can be done by adding the following policy document.

```
{
Expand All @@ -389,7 +389,7 @@ This can be donw by adding the following policy document.

### 6.x -> 7.x

`v7` changes how the module references storage from using terraform's `count` to always creating a "defaultBucket" which can be overidden latter or but providing some initial bucket.
`v7` changes how the module references storage from using terraform's `count` to always creating a "defaultBucket" which can be overridden later or by providing some initial bucket.

We are considering this a major change because of the terraform `moved` block which migrates the resource. After moving to a `v7` applying an earlier version of the module may result in terraform deleting your bucket.

Expand Down
2 changes: 1 addition & 1 deletion main.tf
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ locals {
fqdn = var.subdomain == null ? var.domain_name : "${var.subdomain}.${var.domain_name}"
}

#Create SSL Ceritifcation if applicable
#Create SSL Certification if applicable
module "acm" {
source = "terraform-aws-modules/acm/aws"
version = "~> 3.0"
Expand Down
4 changes: 2 additions & 2 deletions modules/app_eks/variables.tf
Original file line number Diff line number Diff line change
Expand Up @@ -133,12 +133,12 @@ variable "k8s_namespace" {
}

variable "network_id" {
description = "(Required) The identity of the VPC in which the security group attached to the MySQL Aurora instances will be deployed."
description = "(Required) The identity of the VPC in which the security group attached to the EKS cluster will be deployed."
type = string
}

variable "network_private_subnets" {
description = "(Required) A list of the identities of the private subnetworks in which the MySQL Aurora instances will be deployed."
description = "(Required) A list of the identities of the private subnetworks in which the EKS worker nodes will be deployed."
type = list(string)
}

Expand Down
2 changes: 1 addition & 1 deletion modules/app_lb/variables.tf
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ variable "allowed_inbound_ipv6_cidr" {
}

variable "network_id" {
description = "(Required) The identity of the VPC in which the security group attached to the MySQL Aurora instances will be deployed."
description = "(Required) The identity of the VPC in which the security group attached to the ALB will be deployed."
type = string
}

Expand Down
8 changes: 4 additions & 4 deletions modules/database/variables.tf
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ variable "kms_key_arn" {
}

variable "performance_insights_kms_key_arn" {
description = "Specifies an existing KMS key ARN to encrypt the performance insights data if performance_insights_enabled is was enabled out of band"
description = "Specifies an existing KMS key ARN to encrypt the performance insights data if performance_insights_enabled was enabled out of band"
type = string
}

Expand All @@ -19,13 +19,13 @@ variable "vpc_id" {
}

variable "engine_version" {
description = "Version for MySQL Auora to use -- major version only"
description = "Version for MySQL Aurora to use -- major version only"
type = string
default = "8.0"
}

variable "create_db_subnet_group" {
description = "Determines whether to create the databae subnet group or use existing"
description = "Determines whether to create the database subnet group or use existing"
type = string
default = true
}
Comment on lines 21 to 31
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Pre-existing type mismatch on create_db_subnet_group: type = string with default = true.

default = true is a boolean literal, yet type = string is declared. Terraform silently coerces it to "true", but the variable semantically controls creation (on/off) and should be typed as bool.

🔧 Suggested fix
 variable "create_db_subnet_group" {
   description = "Determines whether to create the database subnet group or use existing"
-  type        = string
+  type        = bool
   default     = true
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
variable "engine_version" {
description = "Version for MySQL Auora to use -- major version only"
description = "Version for MySQL Aurora to use -- major version only"
type = string
default = "8.0"
}
variable "create_db_subnet_group" {
description = "Determines whether to create the databae subnet group or use existing"
description = "Determines whether to create the database subnet group or use existing"
type = string
default = true
}
variable "engine_version" {
description = "Version for MySQL Aurora to use -- major version only"
type = string
default = "8.0"
}
variable "create_db_subnet_group" {
description = "Determines whether to create the database subnet group or use existing"
type = bool
default = true
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@modules/database/variables.tf` around lines 21 - 31, The variable declaration
for create_db_subnet_group is typed as string but uses a boolean default; change
the variable's type to bool (variable "create_db_subnet_group") so the default =
true remains a boolean, and then scan usages of var.create_db_subnet_group
(e.g., conditional expressions or resource count/for_each) to ensure they treat
it as a boolean (remove any string comparisons or quotes) so the module
consistently uses a bool value.

Expand Down Expand Up @@ -114,7 +114,7 @@ variable "innodb_lru_scan_depth" {
default = 128
}

# Cluster parametes
# Cluster parameters
variable "binlog_row_image" {
description = "Value for binlog_row_image"
type = string
Expand Down
2 changes: 1 addition & 1 deletion modules/file_storage/main.tf
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ resource "aws_s3_bucket_server_side_encryption_configuration" "file_storage" {
}

# Give the bucket permission to send messages onto the queue. Looks like we
# overide this value.
# override this value.
resource "aws_sqs_queue_policy" "file_storage" {
count = var.create_queue && var.create_queue_policy ? 1 : 0

Expand Down
2 changes: 1 addition & 1 deletion modules/file_storage/variables.tf
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ variable "kms_key_arn" {
}

variable "deletion_protection" {
description = "If the DB instance should have deletion protection enabled. The database can't be deleted when this value is set to `true`."
description = "If the S3 bucket should have deletion protection enabled. The bucket can't be deleted when this value is set to `true`."
type = bool
default = true
}
Expand Down
2 changes: 1 addition & 1 deletion modules/networking/main.tf
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ resource "aws_vpc_endpoint" "clickhouse" {
private_dns_enabled = true
}

# VPC FLow Logs
# VPC Flow Logs
resource "aws_flow_log" "vpc_flow_logs" {
count = var.create_vpc && var.enable_flow_log ? 1 : 0

Expand Down
2 changes: 1 addition & 1 deletion modules/networking/outputs.tf
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
output "vpc_id" {
value = module.vpc.vpc_id
description = "The identity of the VPC in which resources will be delpoyed."
description = "The identity of the VPC in which resources will be deployed."
}

output "vgw_id" {
Expand Down
2 changes: 1 addition & 1 deletion modules/redis/variables.tf
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ variable "redis_subnets" {

variable "redis_create_subnet_group" {
default = false
description = "Whether to create a new subnet group atop subnets provided via `redis_subnets`. If we bringing our own VPC this will not be created via the `network` module, and we must generate it."
description = "Whether to create a new subnet group atop subnets provided via `redis_subnets`. If we are bringing our own VPC this will not be created via the `network` module, and we must generate it."
type = bool
}

Expand Down
18 changes: 9 additions & 9 deletions variables.tf
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ variable "database_master_username" {

variable "database_performance_insights_kms_key_arn" {
default = ""
description = "Specifies an existing KMS key ARN to encrypt the performance insights data if performance_insights_enabled is was enabled out of band"
description = "Specifies an existing KMS key ARN to encrypt the performance insights data if performance_insights_enabled was enabled out of band"
nullable = true
type = string
}
Expand All @@ -110,7 +110,7 @@ variable "database_kms_key_arn" {
variable "public_access" {
type = bool
default = false
description = "Is this instance accessable a public domain."
description = "Is this instance accessible via a public domain."
}

variable "external_dns" {
Expand All @@ -125,8 +125,8 @@ variable "custom_domain_filter" {
default = null
}

# Sometimes domain name and zone name dont match, so lets explicitly ask for
# both. Also is just life easier to have both even though in most cause it may
# Sometimes domain name and zone name don't match, so let's explicitly ask for
# both. Also it's just easier to have both even though in most cases it may
# be redundant info.
# https://github.com/hashicorp/terraform-aws-terraform-enterprise/pull/41#issuecomment-563501858
variable "zone_id" {
Expand All @@ -142,7 +142,7 @@ variable "domain_name" {
variable "subdomain" {
type = string
default = null
description = "Subdomain for accessing the Weights & Biases UI. Default creates record at Route53 Route."
description = "Subdomain for accessing the Weights & Biases UI. Default creates record at Route53 zone apex."
}

variable "extra_fqdn" {
Expand Down Expand Up @@ -264,7 +264,7 @@ variable "network_cidr" {

variable "network_public_subnet_cidrs" {
type = list(string)
description = "List of private subnet CIDR ranges to create in VPC."
description = "List of public subnet CIDR ranges to create in VPC."
default = ["10.10.0.0/24", "10.10.1.0/24"]
}

Expand Down Expand Up @@ -328,7 +328,7 @@ variable "kubernetes_alb_internet_facing" {

variable "kubernetes_alb_subnets" {
type = list(string)
description = "List of subnet ID's the ALB will use for ingress traffic."
description = "List of subnet IDs the ALB will use for ingress traffic."
default = []
}

Expand Down Expand Up @@ -457,7 +457,7 @@ variable "eks_addon_kube_proxy_version" {
}

variable "eks_addon_vpc_cni_version" {
description = "The version of the VPC CNI addon to install. Check the docs for more information about the compatibility https://docs.aws.amazon.com/eks/latest/userguide/vpc-add-on-update.html.s"
description = "The version of the VPC CNI addon to install. Check the docs for more information about the compatibility https://docs.aws.amazon.com/eks/latest/userguide/vpc-add-on-update.html."
type = string
default = "v1.18.3-eksbuild.3"
}
Expand All @@ -482,7 +482,7 @@ variable "enable_s3_https_only" {
##########################################
# External Bucket #
##########################################
# Most users will not need these settings. They are ment for users who want a
# Most users will not need these settings. They are meant for users who want a
# bucket and sqs that are in a different account.
variable "bucket_name" {
type = string
Expand Down
Loading