Repository navigation
Commit 474d171
feat: create all connectivity SG rules unconditionally (DND-1522) (#58)
* feat!: create all connectivity SG rules unconditionally (DND-1522)
Connectivity to an STSaaS environment is never a per-environment decision,
but the module treated all of it as opt-in. Drop the four enable_* toggles
(all default false — their only correct value was true) and create their SG
ingress rules unconditionally:
- enable_argocd_management_eks_access
- enable_vpn_eks_api_access
- enable_ci_runners_eks_api_access
- enable_vpn_redis_access
Add a matching unconditional VPN ingress on mysql_sg (port 3306), mirroring
redis_vpn in modules/comet_elasticache, so Aurora is reachable from the VPN
through the module (new vpn_client_cidr var on comet_rds + root passthrough).
Re-export mysql_sg_id from the root outputs.tf so wrappers no longer have to
look the SG up by name (comet_rds already output it; root did not).
The CIDR inputs (argocd_management_cidrs, vpn_client_cidr, ci_runners_cidr)
stay as variables — those are real values.
BREAKING CHANGE: the four enable_* input variables are removed. Wrappers that
still set them must drop them (their value is now always applied). Warrants a
v6.0.0 bump. Only stsaasuat runs v5.x today; every other env picks this up when
it migrates. The next stsaasuat apply will add the SG rules.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix: address review — moved block for redis_vpn, redis_sg_id output, CIDR dedup
Review feedback on DND-1522.
1. redis_vpn moved block. Dropping count changed the state address from
redis_vpn[0] to redis_vpn, which plans an unordered destroy + create of two
unrelated addresses — it can race into InvalidPermission.Duplicate, and even
on the happy path leaves a window with no VPN ingress to Redis. This is live
on stsaasuat (enable_vpn_redis_access = true). The address is internal to the
module, so without the moved block every migrating env needs a hand-run
terraform state mv. No-op where the toggle was off.
2. Export redis_sg_id. The motivation for re-exporting mysql_sg_id applies
identically to Redis, and more widely: four wrappers (fetch, netflix, si,
waystar) look the Redis SG up by name versus two for MySQL. Wrappers are
edited on the version bump anyway, so shipping both now avoids a second pass.
3. Dedupe eks_api_ingress_rules across all sources, not just within
argocd_management_cidrs. AWS dedupes ingress on (protocol, port range,
source), so a cross-source CIDR collision would fail the apply, and the
enable_* toggles that used to make it avoidable are gone. Candidates are now
ordered and the first per CIDR wins. Verified against the fleet defaults: the
for_each keys are byte-for-byte unchanged, so there is no state churn.
Also fixes the comment naming cluster_security_group_id where the code uses
cluster_primary_security_group_id, and makes vpn_client_cidr a required input in
the three submodules — the root always passes a value, so the submodule defaults
were dead copies that could drift.
Refs DND-1522
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix: validate connectivity CIDRs are canonical and not overly broad
Review feedback. Removing the enable_* toggles means a bad CIDR can no longer
be neutralised by leaving the rule off — it always lands in an SG ingress rule,
so the value is now validated at the root, where operators set it.
Canonical form is required (network address, no host bits, unpadded prefix).
The eks_api for_each keys and the duplicate filter derive from the raw string,
so 10.126.0.0/015 and 10.126.0.0/15 would be two Terraform addresses for one
AWS rule and collide as InvalidPermission.Duplicate. Rejecting beats silently
canonicalising: it fails at plan rather than rewriting operator intent.
Prefixes broader than /8 are rejected, which covers 0.0.0.0/0. Deliberately not
an RFC1918 or fleet-range allowlist — a CI-runner NAT egress address is
legitimately a public /32, and hardcoding the fleet's ranges into a reusable
module recreates the "only one correct value" input DND-1522 removes.
Verified against the root module: 0.0.0.0/0, 10.126.0.0/015 and 10.126.0.1/15
are all rejected with their specific message; the four fleet CIDRs and a public
/32 pass.
Refs DND-1522
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix: reject IPv6 CIDRs at plan; drop remaining submodule CIDR defaults
Three non-blocking review items from @liyaka on #58.
1. IPv6 passed validation. cidrhost() and split() are family-agnostic, so
2001:db8::/32 satisfied both conditions and only failed at apply, against
cidr_ipv4, with an AWS error rather than the variable message that already
said "canonical IPv4 CIDR". Add can(cidrnetmask(...)) to the canonical
condition on all three inputs — it errors on IPv6 and accepts every IPv4
case, including the public /32 a CI-runner NAT egress can legitimately be.
2. The anti-drift rationale was applied to one variable of three.
vpn_client_cidr was made a required submodule input with the default only at
the root, but argocd_management_cidrs and ci_runners_cidr kept their
comet_eks defaults, and ci_runners_cidr's description was never updated.
Drop both defaults and align both descriptions. No behaviour change: the
root passes all three unconditionally (main.tf:361-363).
3. Document the dedup ordering hazard, not just its rationale. Introducing a
CIDR collision moves the surviving rule's Terraform address — vpn_client_cidr
equal to an argocd CIDR retires the "vpn" key, so an env holding that rule
plans a destroy plus a create at the new address and loses VPN reach to the
EKS API for the apply. Comment only.
Verified by extracting the three variable blocks verbatim into an isolated
module and planning each case: IPv6 (v4 and v6 forms), host bits, padded
prefix and 0.0.0.0/0 all rejected with the right message; fleet defaults and a
public /32 accepted. terraform fmt clean; terraform validate Success with only
the pre-existing upstream iam-role-for-service-accounts-eks warnings.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix: derive rds_proxy_allowed_cidrs from vpn_client_cidr
Baz review on #58: rds_proxy_allowed_cidrs carried its own hardcoded
["10.126.0.0/15"] default, duplicated again in the submodule, independent of
vpn_client_cidr. Renumber the VPN and the direct mysql_vpn rule this PR adds
moves while the proxy keeps trusting the old range — the new pool can reach
Aurora directly but is refused through the proxy, and the retired pool keeps
proxy access. Exactly the drift the DND-1522 anti-duplication rationale is
about, and this PR is what creates the divergence: before it, vpn_client_cidr
had nothing to do with MySQL, so an independent proxy CIDR contradicted nothing.
Make the root input a documented tri-state instead of concatenating, because
the submodule documents [] as "SG-only ingress" and an unconditional concat
would silently break that escape hatch:
null (default) -> [var.vpn_client_cidr] both MySQL paths open to one pool
[] -> [] SG-only ingress, preserved
[c, ...] -> verbatim explicit override still wins
Also drop the submodule's duplicate default (required input, default only at
the root) and add the same canonical-IPv4 + /8-or-narrower validations the
other three CIDR inputs got, null-tolerant.
No state churn: under defaults the resolved list is byte-identical to the old
hardcoded value, and proxy_from_cidr keys on the CIDR string, so every for_each
key is unchanged. No consumer is affected either way — no env in comet-devops
sets enable_rds_proxy yet.
Verified each branch by plan (defaults, [], renumbered VPN, explicit list) and
each validation case (IPv6, host bits, padded prefix, 0.0.0.0/0 rejected; [],
null and multi-entry IPv4 accepted). fmt clean; validate Success with only the
pre-existing upstream iam-role-for-service-accounts-eks warnings.
Not fixed here: vpc_interface_endpoints_allowed_cidrs and tgw_propagated_cidrs
hardcode the same management surface (variables.tf:1378,1396 + comet_vpc
mirrors). They predate this PR, sit in the VPC layer rather than this
connectivity-SG class, and tgw_propagated_cidrs is entangled with the TGW
toggles already deferred. Noted as follow-up.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix: resolve rds_proxy_allowed_cidrs' tri-state in a local
Baz review on #58, and a real bug in 536ccf5. Making the default null left
main.tf:122's precondition calling length(var.rds_proxy_allowed_cidrs), and null
is not a list, so it raises "Invalid function argument" at plan.
Narrower than reported, and worse placed. HCL's || does short-circuit
(verified), so enable_eks = true or a non-empty rds_proxy_allowed_sg_ids never
reaches the third operand. It breaks only with enable_eks = false and no
rds_proxy_allowed_sg_ids — which is exactly the configuration this precondition
exists to explain. The operator with no ingress source got a type error about
length() instead of "enable_rds_proxy requires at least one ingress source".
Resolve the tri-state once into local.rds_proxy_effective_cidrs and use it for
both the precondition and the module argument. This also keeps the source count
honest: under null the proxy does get one CIDR source, so null must satisfy that
check, not fail it. coalesce(var.rds_proxy_allowed_cidrs, []) would have fixed
the crash and got that backwards, firing "no ingress source" on the default.
Full precondition matrix by plan, before vs after:
enable_eks sg_ids cidrs before after
true [] null pass pass
false [] null Invalid function argument pass
false [sg-1] null pass (short-circuits) pass
false [] [] no ingress source no ingress source
false [] [10.4.0.0/16] pass pass
One cell changes, from a crash to the correct pass. The genuine no-ingress case
still fires. fmt clean; validate Success with only the pre-existing upstream
iam-role-for-service-accounts-eks warnings.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>1 parent 2e396b7 commit 474d171
11 files changed
Lines changed: 216 additions & 96 deletions
File tree
- modules
- comet_eks
- comet_elasticache
- comet_rds_proxy
- comet_rds
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
21 | 21 | | |
22 | 22 | | |
23 | 23 | | |
| 24 | + | |
| 25 | + | |
| 26 | + | |
| 27 | + | |
| 28 | + | |
| 29 | + | |
| 30 | + | |
| 31 | + | |
| 32 | + | |
| 33 | + | |
| 34 | + | |
| 35 | + | |
| 36 | + | |
| 37 | + | |
| 38 | + | |
| 39 | + | |
| 40 | + | |
24 | 41 | | |
25 | 42 | | |
26 | 43 | | |
| |||
128 | 145 | | |
129 | 146 | | |
130 | 147 | | |
131 | | - | |
| 148 | + | |
132 | 149 | | |
133 | 150 | | |
134 | 151 | | |
| |||
393 | 410 | | |
394 | 411 | | |
395 | 412 | | |
396 | | - | |
397 | | - | |
398 | | - | |
399 | | - | |
400 | | - | |
401 | | - | |
402 | | - | |
| 413 | + | |
| 414 | + | |
| 415 | + | |
| 416 | + | |
403 | 417 | | |
404 | 418 | | |
405 | 419 | | |
| |||
429 | 443 | | |
430 | 444 | | |
431 | 445 | | |
432 | | - | |
433 | | - | |
| 446 | + | |
434 | 447 | | |
435 | 448 | | |
436 | 449 | | |
| |||
447 | 460 | | |
448 | 461 | | |
449 | 462 | | |
| 463 | + | |
| 464 | + | |
450 | 465 | | |
451 | 466 | | |
452 | 467 | | |
| |||
515 | 530 | | |
516 | 531 | | |
517 | 532 | | |
518 | | - | |
| 533 | + | |
519 | 534 | | |
520 | 535 | | |
521 | 536 | | |
| |||
525 | 540 | | |
526 | 541 | | |
527 | 542 | | |
528 | | - | |
| 543 | + | |
| 544 | + | |
| 545 | + | |
529 | 546 | | |
530 | 547 | | |
531 | 548 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
1484 | 1484 | | |
1485 | 1485 | | |
1486 | 1486 | | |
1487 | | - | |
1488 | | - | |
1489 | | - | |
1490 | | - | |
| 1487 | + | |
| 1488 | + | |
| 1489 | + | |
| 1490 | + | |
| 1491 | + | |
1491 | 1492 | | |
1492 | 1493 | | |
1493 | | - | |
1494 | | - | |
1495 | | - | |
1496 | | - | |
| 1494 | + | |
| 1495 | + | |
| 1496 | + | |
| 1497 | + | |
| 1498 | + | |
1497 | 1499 | | |
1498 | 1500 | | |
1499 | 1501 | | |
1500 | 1502 | | |
1501 | | - | |
1502 | | - | |
1503 | | - | |
| 1503 | + | |
| 1504 | + | |
| 1505 | + | |
| 1506 | + | |
1504 | 1507 | | |
1505 | 1508 | | |
1506 | 1509 | | |
1507 | 1510 | | |
1508 | | - | |
1509 | | - | |
1510 | | - | |
| 1511 | + | |
| 1512 | + | |
| 1513 | + | |
| 1514 | + | |
1511 | 1515 | | |
1512 | 1516 | | |
1513 | 1517 | | |
1514 | 1518 | | |
1515 | | - | |
| 1519 | + | |
1516 | 1520 | | |
| 1521 | + | |
| 1522 | + | |
| 1523 | + | |
| 1524 | + | |
| 1525 | + | |
| 1526 | + | |
| 1527 | + | |
| 1528 | + | |
| 1529 | + | |
| 1530 | + | |
| 1531 | + | |
| 1532 | + | |
| 1533 | + | |
| 1534 | + | |
| 1535 | + | |
| 1536 | + | |
| 1537 | + | |
| 1538 | + | |
| 1539 | + | |
| 1540 | + | |
| 1541 | + | |
| 1542 | + | |
| 1543 | + | |
| 1544 | + | |
| 1545 | + | |
| 1546 | + | |
| 1547 | + | |
| 1548 | + | |
| 1549 | + | |
| 1550 | + | |
1517 | 1551 | | |
1518 | 1552 | | |
1519 | 1553 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
846 | 846 | | |
847 | 847 | | |
848 | 848 | | |
849 | | - | |
850 | | - | |
851 | | - | |
852 | | - | |
853 | | - | |
854 | | - | |
855 | 849 | | |
856 | | - | |
| 850 | + | |
857 | 851 | | |
858 | | - | |
859 | | - | |
860 | | - | |
861 | | - | |
862 | | - | |
863 | | - | |
864 | | - | |
865 | 852 | | |
866 | 853 | | |
867 | 854 | | |
868 | | - | |
| 855 | + | |
869 | 856 | | |
870 | | - | |
871 | | - | |
872 | | - | |
873 | | - | |
874 | | - | |
875 | | - | |
876 | | - | |
877 | 857 | | |
878 | 858 | | |
879 | 859 | | |
880 | | - | |
| 860 | + | |
881 | 861 | | |
882 | | - | |
883 | 862 | | |
884 | 863 | | |
885 | 864 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
79 | 79 | | |
80 | 80 | | |
81 | 81 | | |
82 | | - | |
83 | | - | |
84 | | - | |
85 | | - | |
86 | | - | |
87 | 82 | | |
| 83 | + | |
| 84 | + | |
| 85 | + | |
| 86 | + | |
88 | 87 | | |
89 | 88 | | |
90 | 89 | | |
| |||
94 | 93 | | |
95 | 94 | | |
96 | 95 | | |
| 96 | + | |
| 97 | + | |
| 98 | + | |
| 99 | + | |
| 100 | + | |
| 101 | + | |
| 102 | + | |
| 103 | + | |
| 104 | + | |
| 105 | + | |
| 106 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
8 | 8 | | |
9 | 9 | | |
10 | 10 | | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
11 | 16 | | |
12 | 17 | | |
13 | 18 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
95 | 95 | | |
96 | 96 | | |
97 | 97 | | |
98 | | - | |
99 | | - | |
100 | | - | |
101 | | - | |
102 | | - | |
103 | | - | |
104 | 98 | | |
105 | | - | |
| 99 | + | |
106 | 100 | | |
107 | | - | |
108 | 101 | | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
352 | 352 | | |
353 | 353 | | |
354 | 354 | | |
| 355 | + | |
| 356 | + | |
| 357 | + | |
| 358 | + | |
| 359 | + | |
| 360 | + | |
| 361 | + | |
| 362 | + | |
| 363 | + | |
| 364 | + | |
| 365 | + | |
| 366 | + | |
| 367 | + | |
| 368 | + | |
| 369 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
185 | 185 | | |
186 | 186 | | |
187 | 187 | | |
| 188 | + | |
| 189 | + | |
| 190 | + | |
| 191 | + | |
| 192 | + | |
188 | 193 | | |
189 | 194 | | |
190 | 195 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
26 | 26 | | |
27 | 27 | | |
28 | 28 | | |
29 | | - | |
| 29 | + | |
30 | 30 | | |
31 | | - | |
32 | 31 | | |
33 | 32 | | |
34 | 33 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
72 | 72 | | |
73 | 73 | | |
74 | 74 | | |
| 75 | + | |
| 76 | + | |
| 77 | + | |
| 78 | + | |
| 79 | + | |
75 | 80 | | |
76 | 81 | | |
77 | 82 | | |
| |||
92 | 97 | | |
93 | 98 | | |
94 | 99 | | |
| 100 | + | |
| 101 | + | |
| 102 | + | |
| 103 | + | |
| 104 | + | |
95 | 105 | | |
96 | 106 | | |
97 | 107 | | |
| |||
0 commit comments