← Seth Bergman

Testing · Terraform

health_check_type == "ELB"

A green assertion that guaranteed the cluster could never start.

I had to delete a passing test to fix the bug

An assertion is a claim about the world. Green means the code agrees with the claim — not that the claim is true. When the claim is wrong, the test stops being a safety net and becomes a lock.

Here is a test. It passed on every pull request for weeks. Read the comment first, because the comment is the interesting part:

terraform/aws/tests/cluster.tftest.hcl — before
run "health_check_uses_the_load_balancer_not_just_ec2" {
  command = plan

  # EC2 health only knows whether the instance is running. A node that is
  # up but sealed, or wedged, is useless and must be replaced — only the
  # load balancer's check can tell the difference.
  assert {
    condition     = aws_autoscaling_group.vault.health_check_type == "ELB"
    error_message = "ASG health check must be ELB so sealed-but-running nodes are replaced."
  }
}

Everything in that comment is true. A Vault node that is running but sealed is useless. EC2 health checks genuinely cannot see it. The load balancer's check genuinely can. If you were reviewing this, you would approve it, and you would be right to.

It also guaranteed that the cluster could never start.

What it was protecting

The Terraform profile deliberately does not issue TLS certificates. The user-data script says so in as many words and defers to the Ansible layer that runs afterwards. Vault will not start without a certificate — which is correct behaviour, since a Vault serving plaintext is worse than one that does not serve.

So between terraform apply and the Ansible run, there is a window where every node is up and Vault is not listening.

With health_check_type = "ELB", the autoscaling group spends that window asking the load balancer whether each instance is healthy. The load balancer asks Vault. Vault is not there. At the end of the grace period the group marks the instance unhealthy, terminates it, and launches a replacement — which boots, fails to start Vault, and is terminated in turn.

A bare terraform apply never converges. It bills EC2, three NAT gateways and EBS for as long as you let it run, and it looks like a slow bootstrap rather than a configuration that cannot work.

The fix is one word: EC2 until Ansible has run, ELB afterwards. But to make that change I had to delete a green assertion whose reasoning was correct.

That is a strange feeling, and it is the reason I am writing this down. Deleting a passing test feels like vandalism. Every instinct says the test knows something you have forgotten.

The one that looked even more responsible

Same repository, same week, a different assertion:

terraform/aws/tests/security.tftest.hcl — before
# The node security group must never take an ingress CIDR rule at all:
# the only ingress paths are references to the load balancer's group or
# to itself. A cidr_ipv4 rule appearing here means someone opened the
# API to a network range directly.
assert {
  condition     = aws_vpc_security_group_ingress_rule.vault_api_from_lb.cidr_ipv4 == null
  error_message = "Vault API ingress must come from the load balancer's security group, not a CIDR."
}

This one is even harder to argue with, because it reads as a security control. A CIDR rule on the node group would be a way to reach Vault without going through the load balancer. Forbidding it looks like exactly the invariant you would want.

The target group uses target_type = "instance". For instance targets on a Network Load Balancer, client IP preservation is enabled and cannot be turned off. The load balancer forwards each packet with the original client's address as its source — not its own.

So the security-group reference matched the health checks, which come from the load balancer's own network interface, and nothing else. Client traffic arrived from client addresses, matched no rule, and was dropped.

Every target reported healthy. The autoscaling group was content. Dashboards were green. No client could connect.

To fix it I had to add the CIDR rule the test forbade — and rewrite the assertion that forbade it.

Then I did it to myself

Later the same week I split one KMS key into two, and wrote what I thought was a careful test:

condition = ...ebs[0].kms_key_id != aws_kms_key.vault_autounseal.arn

It failed. Not because the code was wrong — because of this, in the mock provider the test suite runs against:

terraform/aws/tests/mocks/aws/main.tfmock.hcl
mock_resource "aws_kms_key" {
  defaults = {
    arn = "arn:aws:kms:us-east-1:123456789012:key/12345678-..."
  }
}

Every aws_kms_key gets the same arn. So != could never hold, and == would have passed no matter which key the volume actually used. Both directions were meaningless. Had I written the assertion the other way round, it would have gone green and told me nothing.

The repository's own testing notes warn about this in as many words:

assert on config values and locals; treat any assertion that reads a mocked data source as suspect

I had read that file. I wrote the assertion anyway.

What these have in common

Not that the tests were sloppy. All three were written deliberately, with comments explaining the reasoning, by someone trying to pin down a property that mattered.

The common shape is narrower: an assertion is a claim about the world, and a green test means the code agrees with the claim. It says nothing about whether the claim is true.

When the claim is wrong, green is not merely useless — it actively defends the defect. Fixing the bug now requires deleting a passing test, and every review instinct you have says do not do that.

The claimWhy it was wrong
health checks must be ELBtrue once serving, fatal before Ansible runs
node group must take no CIDRtrue if the LB were the source, and it is not
these two arns must differthe mock gives every key one arn

The tells

Having been caught three times in a week, here is what I now look at:

An exact value where a property was meant. == "ELB" pins one value for all time. What was actually wanted was "the group can detect a node that is up but not serving" — which is true of ELB only under a precondition nobody wrote down.

An assertion written in the same sitting as the code. It then encodes the same assumptions, and can only confirm them. This is the mock problem in a different coat: when the code and the check share an author and an hour, they share a belief.

A comment that argues rather than states. All three of these have persuasive comments. Persuasion in a test comment usually means the author was reasoning about why the value is right, rather than what would have to be true for it to be wrong.

And the loudest one: you are editing a test in order to make a fix. That moment is worth stopping at every single time. Sometimes the test is stale. Sometimes it is telling you the fix is wrong. And sometimes it has been holding the bug in place since the day it was written, which is the case you will not consider unless you make yourself.

What I changed

Mutation, not counting. Every assertion here has to be made to fail on purpose. Reverting the health check default to ELB turns exactly one red. If an assertion cannot be made to fail, it is not yet known to test anything.

Assert properties, not values, where the property is what you mean. The health check test now asserts two things: that the default is EC2, so a bare apply terminates, and that ELB is reachable, so the profile is not trapped. Neither alone is the point; the pair is.

Put the assertion where it can be answered. The KMS arn comparison moved out of the mocked suite entirely and into a run against an emulated AWS API, where the arns are real and distinct. The mocked suite now asserts only configuration — two keys, different descriptions, different deletion windows — which is all it can honestly see.

None of that would have helped with the first two. Those needed something duller: asking, of a green test, what would have to be true for this claim to be false — and noticing that for the health check, the answer was "the thing it protects has not been provisioned yet", which was written down in the very script the test was covering.