Contributing Vault Coding skills - #44
Conversation
Tessl Skill Review Results
Checks: frontmatter validity, required fields, body structure, examples, line count. |
| - EE adds significant capabilities | ||
| - Single codebase needed |
There was a problem hiding this comment.
No sure if I understand what these two mean. Can you ellaborate?
| sdk/ # Plugin SDK | ||
| ``` | ||
|
|
||
| Everything in `vault/` is internal - never import directly. |
There was a problem hiding this comment.
This only applies to external projects though.
|
|
||
| ``` | ||
| vault/ # Core server (INTERNAL) | ||
| ├── logical/ # Backend interfaces |
| └── credential/ # Auth methods | ||
|
|
||
| command/ # CLI commands | ||
| http/ # HTTP API handlers |
There was a problem hiding this comment.
This might be confusing because most of the HTTP API handlers that don't live here. This is more like the lower-level parts of our HTTP API and surrounding tooling, together with some of the less conventional HTTP handlers that don't use the SDK so much.
| | Stop a node | `cluster.StopCore(t, nodeIndex)` | Call `WaitForActiveNode(t, cluster)` to verify another node activated | Assert stopped node's client returns errors (unhealthy/sealed), not just that a new active exists | | ||
| | Seal all nodes | `testhelpers.SealCores(t, cluster)` | Before DR promotion: assert `health.Sealed == true` on all old-primary nodes | No assertion that writes to the old primary during sealed state return appropriate errors | | ||
| | Unseal all nodes | `testhelpers.EnsureCoresUnsealed(t, cluster)` | Assert `health.Sealed == false` and `health.Initialized == true` on each node | No re-check of replication state post-unseal | | ||
| | Inject replication failure mode | `vault.SetReplicationFailureMode(core, vault.ReplicationFailureModeReindexNeeded)` | Assert `sys/replication/status` surfaces a warning; then assert promote/demote succeeds or fails as intended | Tests check the result of `promote` but do not verify the failure mode appears in status warnings before the operation | |
|
|
||
| --- | ||
|
|
||
| ### Simulate Node/Primary Failure |
There was a problem hiding this comment.
Most of these don't belong in the replication testing section IMO.
| secondaryClient.WithRequestCallbacks(api.RequireState(state)).Logical().Read(path) | ||
| ``` | ||
|
|
||
| To verify 412 actually fired (invalidation was async): |
There was a problem hiding this comment.
There's some confusion here I think. These callbacks (and client-controlled consistency aka CCC in general) doesn't actually take into account async invalidation, they only check to ensure that the upstream WAL has been received and applied to storage locally. This is actually a big limitation and so if the code being tested uses async invalidation, you need to poll instead of relying solely on CCC.
|
|
||
| ### Pattern 3: Force Synchronous Invalidation (Deterministic Tests) | ||
|
|
||
| Set `AsyncInvalidationChannelSize = 0` to force FSM to block until invalidation completes: |
There was a problem hiding this comment.
I think I'm the only one who's ever done this, and it's very much a last resort. We should avoid it whenever possible because it precludes using t.Parallel.
| |----------------|---------|----------| | ||
| | `NewTestCluster` | `vault.NewTestCluster` | Default choice for core tests | | ||
| | `NewTestDockerCluster` | Docker containers | Cross-binary testing | | ||
| | `NewTestExecDevCluster` | Subprocesses | External process testing | |
|
I would drastically scale back the replication testing notes. We can distinguish between two kinds of replication testing: tests written by devs modifying how replication works, and tests written by devs who want to verify that their code behaves properly in replication scenarios. We should cater more to the latter, for whom most of what you wrote about replication is overkill. |
| "time" | ||
|
|
||
| "github.com/hashicorp/vault/sdk/helper/testcluster" | ||
| "github.com/stretchr/testify/require" |
There was a problem hiding this comment.
Is there a way to add a skill to make sure we avoid using t.Fatalf instead use testify/require?
3 participants
Summary
Adds 7 agent skills for contributing code to the HashiCorp Vault codebase. These skills provide structured, actionable guidance for common development tasks ranging from terminology lookups to security-critical implementation patterns.
Skills Added
Reference Documents
The test-authoring skill includes 3 reference files:
The acronym-helper skill includes a full acronyms.md reference.