Skip to content

feat(rules): add storage and node generation cost rules - #135

Merged
dannysteenman merged 6 commits into
mainfrom
worktree-bridge-cse_01BMrzpzuDJE1F6Z1QgqKf67
Aug 3, 2026
Merged

feat(rules): add storage and node generation cost rules#135
dannysteenman merged 6 commits into
mainfrom
worktree-bridge-cse_01BMrzpzuDJE1F6Z1QgqKf67

Conversation

@axonstone

Copy link
Copy Markdown
Contributor

Summary

  • Adds three rules that all flag the same shape of waste: a resource paying for a previous-generation or untiered storage option when a cheaper equivalent exists.
  • Batched together because they share machinery rather than a service: all three are static-property checks that run in both iac and discovery, none needs a new CloudWatch metric dataset, and two of the three extend datasets that already existed.
  • Supporting SDK work: the RDS instance datasets now carry storageType in both scan modes, and a new static aws-elasticache-clusters dataset normalizes node types from Terraform cache clusters and replication groups plus their CloudFormation equivalents.

Two deliberate scoping decisions, both documented in docs/reference/rule-ids.md:

  • CLDBRN-AWS-RDS-11 skips provisioned IOPS types (io1/io2). gp3 cannot always match their attainable IOPS, so the substitution is not universally cheaper. It also skips IaC resources that leave the storage type unresolved or unset, because the documented default differs between the Terraform provider and the RDS API.
  • CLDBRN-AWS-S3-5 only covers buckets with no lifecycle configuration at all. Lifecycle-managed buckets stay with CLDBRN-AWS-S3-2, so the two storage-class rules never flag the same bucket. Issue Add S3 Intelligent-Tiering recommendation rule #113 asked for that boundary to be settled during implementation.

Diagram

flowchart LR
  subgraph iac["IaC sources"]
    TF["Terraform"]
    CFN["CloudFormation"]
  end

  subgraph sdk["@cloudburn/sdk datasets"]
    S3["aws-s3-bucket-analyses<br/>(unchanged)"]
    RDS["aws-rds-instances<br/>+ storageType"]
    EC["aws-elasticache-clusters<br/>(new static dataset)"]
  end

  subgraph rules["@cloudburn/rules"]
    R1["CLDBRN-AWS-S3-5<br/>Intelligent-Tiering"]
    R2["CLDBRN-AWS-RDS-11<br/>gp2 / magnetic storage"]
    R3["CLDBRN-AWS-ELASTICACHE-3<br/>previous-gen node type"]
  end

  AWS["Live AWS discovery"]

  TF --> S3
  TF --> RDS
  TF --> EC
  CFN --> S3
  CFN --> RDS
  CFN --> EC
  AWS --> S3
  AWS --> RDS
  AWS --> EC

  S3 --> R1
  RDS --> R2
  EC --> R3
Loading

Scope

  • cloudburn (cli)
  • @cloudburn/sdk
  • @cloudburn/rules
  • docs/community files

Release Notes

  • Added a .changeset/*.md file for published package changes
  • No published package changes in this PR

Verification

  • pnpm lint
  • pnpm typecheck
  • pnpm test
  • pnpm build
  • pnpm verify

Also confirmed through the built CLI against a scratch directory holding both a main.tf and a template.yaml: all three rules report the expected resource, path, line, and column for each source kind.

Boundary Checks

  • No engine/parser/provider logic added to @cloudburn/rules
  • CLI delegates scan logic to SDK
  • README/CONTRIBUTING/docs updated when behavior changed

pnpm exec turbo boundaries reports no issues.

Related Issues

Closes #113
Closes #121

Adds CLDBRN-AWS-S3-5 for buckets that declare no lifecycle configuration
and no enabled Intelligent-Tiering configuration. Lifecycle-managed
buckets stay with CLDBRN-AWS-S3-2 so the two storage-class rules never
flag the same bucket.

Closes #113
Adds CLDBRN-AWS-RDS-11 for DB instances on gp2 or magnetic storage,
where gp3 costs less per GB and provisions IOPS independently. The RDS
datasets now carry the storage type in both scan modes.

Refs #121
Adds CLDBRN-AWS-ELASTICACHE-3 and the static ElastiCache cluster dataset
it reads, covering Terraform cache clusters and replication groups plus
their CloudFormation equivalents.

Closes #121
Closes #113
@axonstone axonstone added the enhancement New feature or request label Jul 30, 2026
@axonstone
axonstone requested a review from dannysteenman July 30, 2026 15:05
@axonstone

axonstone commented Jul 30, 2026

Copy link
Copy Markdown
Contributor Author

roborev: Combined Review (43b4e25)

Verdict: Fail — two high-severity correctness gaps can produce false negatives in the new S3 and RDS rules.

High

  • S3 Intelligent-Tiering configuration is incorrectly treated as proof of adoption. An Intelligent-Tiering configuration only controls archive tiers for objects already using that storage class. Adding the configuration can therefore silence the finding without transitioning or uploading any objects to Intelligent-Tiering, contradicting the rule’s intended outcome. Require a lifecycle transition to INTELLIGENT_TIERING or another observable adoption signal; otherwise report unknown/lower confidence and clarify the remediation.

  • RDS IaC normalization ignores source-specific default storage types. An omitted Terraform aws_db_instance.storage_type effectively becomes gp2 without iops and io1 with iops, while CloudFormation omission defaults to gp3. Treating all omissions as unset causes common Terraform instances using implicit gp2—the rule’s primary target—to pass unnoticed. Normalize the effective type by source and preserve a distinct unknown state for unresolved expressions.


Reviewers: 3 done | Synthesis: codex, 15s | Total: 7m56s

roborev review findings:
- AwsStaticRdsInstance.storageType is optional again so the publicly
  exported type stays additive for consumers building their own
  fixtures. The loader still always sets it and the evaluator already
  handled a missing value.
- Record why an Intelligent-Tiering configuration counts as adoption
  intent rather than proof of tiering in CLDBRN-AWS-S3-5, since AWS
  applies that configuration only to objects already in the
  Intelligent-Tiering storage class.
@axonstone

Copy link
Copy Markdown
Contributor Author

Re: comment 5132678701

Both findings reviewed. One fixed, one accepted as accurate but resolved with documentation rather than a behavior change.

Breaking public type change — fixed in b4187c4. AwsStaticRdsInstance.storageType is optional again (storageType?: string | null), matching the sibling performanceInsightsEnabled? field and the already-optional discovery counterpart. Verified against the emitted dist/index.d.ts, so a consumer's existing object literal compiles unchanged. The loader still always populates it, so the dataset expectations in aws-static.test.ts stay correct, and there is now a regression test that constructs the type without the field. Good catch — the repo's own policy is to treat exported type changes as breaking, and the 8 fixture edits this required were the tell.

Intelligent-Tiering signal — accurate, documented, behavior unchanged. You are right on the AWS semantics, and I confirmed it in the API reference for PutBucketIntelligentTieringConfiguration: 'You only need S3 Intelligent-Tiering enabled on a bucket if you want to automatically move objects stored in the S3 Intelligent-Tiering storage class to the Archive Access or Deep Archive Access tier.' The configuration governs archive tiers; objects enter the storage class through their upload storage class or a lifecycle transition.

Keeping the skip anyway, deliberately:

  • Neither the IaC datasets nor the bucket-level discovery dataset can see per-object storage classes, so the signal is ambiguous in both directions. Dropping the skip would tell owners who adopted Intelligent-Tiering through direct uploads to 'enable Intelligent-Tiering' — a false positive they cannot act on.
  • Issue Add S3 Intelligent-Tiering recommendation rule #113 specified this boundary ('skip buckets already covered by either'), and CLDBRN-AWS-S3-2 already treats the same flag as satisfying its policy. Changing it here alone would make the two S3 storage-class rules inconsistent.
  • Nothing user-visible overclaims: the rule only fires on buckets with no lifecycle configuration at all, where the recommendation is sound. The imprecision was in what the rule silently skips.

What changed instead: the gap is now recorded in the shouldRecommendIntelligentTiering doc comment and in the CLDBRN-AWS-S3-5 row of docs/reference/rule-ids.md, both stating that an Intelligent-Tiering configuration counts as adoption intent rather than proof of tiering, and that a bucket with an archive-tier configuration but Standard objects stays unflagged. Narrowing this further needs per-object storage-class data, which would be a separate rule and dataset.

pnpm verify and turbo boundaries pass.

@dannysteenman
dannysteenman merged commit 375f489 into main Aug 3, 2026
7 checks passed
@dannysteenman
dannysteenman deleted the worktree-bridge-cse_01BMrzpzuDJE1F6Z1QgqKf67 branch August 3, 2026 06:30
@github-actions github-actions Bot mentioned this pull request Aug 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add RDS storage type and ElastiCache generation rules Add S3 Intelligent-Tiering recommendation rule

2 participants