Skip to content

Commit

Permalink
feat: [MSSQL] dont mask invalid iops combinations
Browse files Browse the repository at this point in the history
[#185149472]

So far, other services preferred ignoring iops field when storage
type was not io1 or gp3. This implementation raises error instead.

The reasoning is to follow the principle of least surprise. If a
specific combination is invalid, enforce users to clearly specify
correct values.

There is some level of surprise though, because iops is 3000 by
default, so a customer might not be aware of this and still get
the error. However, even in that case, it might be useful to
surface the error to make the default value explicit and evident.
  • Loading branch information
fnaranjo-vmw committed Aug 7, 2023
1 parent c64ce5f commit 0ed5016
Show file tree
Hide file tree
Showing 4 changed files with 43 additions and 39 deletions.
55 changes: 19 additions & 36 deletions terraform-tests/mssql_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -330,43 +330,26 @@ var _ = Describe("mssql", Label("mssql-terraform"), Ordered, func() {
})
})

Context("storage_type gp2", func() {
BeforeAll(func() {
plan = ShowPlan(terraformProvisionDir, buildVars(defaultVars, requiredVars, map[string]any{
"storage_type": "gp2",
}))
})

It("iops should not be set", func() {
instanceData := AfterValuesForType(plan, "aws_db_instance")
Expect(instanceData).To(MatchKeys(IgnoreExtras, Keys{
"storage_type": Equal("gp2"),
}))
Expect("iops").NotTo(BeKeyOf(instanceData))
})
})

When("valid type for iops", func() {
DescribeTable("iops should be set",
func(storageTypeParam map[string]any) {
plan := ShowPlan(terraformProvisionDir, buildVars(defaultVars, requiredVars, storageTypeParam))
instanceData := AfterValuesForType(plan, "aws_db_instance")

Expect(instanceData).To(
MatchKeys(IgnoreExtras, Keys{
"storage_type": Equal(storageTypeParam["storage_type"]),
}),
)
Expect("iops").To(BeKeyOf(instanceData))
Context("storage_type and iops validation", func() {
DescribeTable("matrix with valid and invalid combinations",
func(extraParams map[string]any, expectedError string) {
if expectedError == "" {
ShowPlan(terraformProvisionDir, buildVars(defaultVars, requiredVars, extraParams))
} else {
session, _ := FailPlan(terraformProvisionDir, buildVars(defaultVars, requiredVars, extraParams))
Expect(session.ExitCode()).NotTo(Equal(0))
Expect(session).To(gbytes.Say(expectedError))

}
},
Entry(
"io1",
map[string]any{"storage_type": "io1"},
),
Entry(
"gp3",
map[string]any{"storage_type": "gp3"},
),
Entry("standard & iops", map[string]any{"storage_type": "standard", "iops": 3000}, "set `iops` to `null` or pick a valid `storage_type` such as io1, gp3"),
Entry("gp2 & iops", map[string]any{"storage_type": "gp2" , "iops": 3000}, "set `iops` to `null` or pick a valid `storage_type` such as io1, gp3"),
Entry("gp3 & iops", map[string]any{"storage_type": "gp3" , "iops": 3000}, ""),
Entry("io1 & iops", map[string]any{"storage_type": "io1" , "iops": 3000}, ""),
Entry("standard ! iops", map[string]any{"storage_type": "standard", "iops": nil}, ""),
Entry("gp2 ! iops", map[string]any{"storage_type": "gp2" , "iops": nil}, ""),
Entry("gp3 ! iops", map[string]any{"storage_type": "gp3" , "iops": nil}, "specify an `iops` value or pick a `storage_type` different than io1, gp3"),
Entry("io1 ! iops", map[string]any{"storage_type": "io1" , "iops": nil}, "specify an `iops` value or pick a `storage_type` different than io1, gp3"),
)
})
})
Expand Down
2 changes: 0 additions & 2 deletions terraform/mssql/provision/data.tf
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,6 @@ locals {
subnet_ids = try(data.aws_db_subnet_group.provided[0].subnet_ids, data.aws_subnets.in_provided_vpc[0].ids, data.aws_subnets.in_default_vpc[0].ids)
security_group_ids = try(data.aws_security_groups.provided[0].ids, [aws_security_group.rds-sg[0].id])
subnet_group_name = try(data.aws_db_subnet_group.provided[0].name, aws_db_subnet_group.rds-private-subnet[0].name)

valid_storage_types_for_iops = ["io1", "gp3"]
}

data "aws_vpc" "default" {
Expand Down
2 changes: 1 addition & 1 deletion terraform/mssql/provision/main.tf
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ resource "aws_db_instance" "db_instance" {
kms_key_id = var.kms_key_id == "" ? null : var.kms_key_id
skip_final_snapshot = true
storage_type = var.storage_type
iops = contains(local.valid_storage_types_for_iops, var.storage_type) ? var.iops : null
iops = var.iops

lifecycle {
prevent_destroy = true
Expand Down
23 changes: 23 additions & 0 deletions terraform/mssql/provision/validations.tf
Original file line number Diff line number Diff line change
Expand Up @@ -8,3 +8,26 @@ resource "terraform_data" "kms_key_was_provided" {
}
}
}

resource "terraform_data" "iops-based-storage-type" {
count = var.iops != null ? 1 : 0

lifecycle {
precondition {
condition = contains(["io1", "gp3"], var.storage_type)
error_message = "set `iops` to `null` or pick a valid `storage_type` such as io1, gp3"
}
}
}

resource "terraform_data" "non-iops-based-storage-type" {
count = var.iops == null ? 1 : 0

lifecycle {
precondition {
condition = !contains(["io1", "gp3"], var.storage_type)
error_message = "specify an `iops` value or pick a `storage_type` different than io1, gp3"
}
}
}

0 comments on commit 0ed5016

Please sign in to comment.