From 24927155deb6368f4f7830d3b5cf7baf9b9745f8 Mon Sep 17 00:00:00 2001 From: Sung-Kyu Yoo Date: Sun, 9 Aug 2026 12:40:04 +0900 Subject: [PATCH 1/5] Add a compatibility page and pin the exported API surface State in one place what will not break and what deliberately might, so a reader deciding whether to depend on kvs can answer that from a page rather than from the source. A promise nobody checks rots, so the Go half of it is enforced: TestPublicAPISurface renders every exported declaration in the root package and compares it against testdata/api-surface.txt. A symbol cannot join or leave without the diff landing in review. Comments are left unparsed and function bodies dropped, which keeps the golden file about signatures and free of everything else. SetReplicator, ReplaceWith, and ApplyReplicated are exported so internal/cluster can reach them across the package boundary, and pkg/resp exists so the server can speak RESP2 rather than as a library for other programs. Both now say so where someone reading godoc will find it. The page also writes down the trust boundary as it stands: no authentication, no authorization, no TLS, and HTTP and gRPC listening on every interface by default while RESP stays on loopback. --- README.md | 7 + api_surface_test.go | 166 ++++++++++++++++++ changes/unreleased/Added-20260809-000000.yaml | 3 + content/docs/compatibility.md | 124 +++++++++++++ content/docs/contributing.md | 2 +- content/docs/overview.md | 5 + content/docs/release.md | 2 +- pkg/resp/reader.go | 5 + replication.go | 9 + testdata/api-surface.txt | 124 +++++++++++++ 10 files changed, 445 insertions(+), 2 deletions(-) create mode 100644 api_surface_test.go create mode 100644 changes/unreleased/Added-20260809-000000.yaml create mode 100644 content/docs/compatibility.md create mode 100644 testdata/api-surface.txt diff --git a/README.md b/README.md index 55d6bbc..fcdf75a 100644 --- a/README.md +++ b/README.md @@ -170,6 +170,13 @@ What that does and does not promise: The [durability and clustering docs](https://skyoo2003.github.io/kvs/docs/clustering/) go through each of these, and what to do about them. +## Compatibility + +What `v1` promises not to break, and what it deliberately leaves out, is on the +[compatibility page](https://skyoo2003.github.io/kvs/docs/compatibility/). Read it before +pinning kvs, and before opening a port: kvs has no authentication, no authorization, and no +TLS, and HTTP and gRPC listen on every interface by default. + ## Documentation Full documentation is available at [skyoo2003.github.io/kvs](https://skyoo2003.github.io/kvs). diff --git a/api_surface_test.go b/api_surface_test.go new file mode 100644 index 0000000..3288daf --- /dev/null +++ b/api_surface_test.go @@ -0,0 +1,166 @@ +package kvs + +import ( + "bytes" + "flag" + "fmt" + "go/ast" + "go/parser" + "go/printer" + "go/token" + "os" + "strings" + "testing" +) + +// goldenPath holds the exported surface of this package. It is the machine-readable half of +// content/docs/compatibility.md: the page says what is promised, this file says what is there. +const goldenPath = "testdata/api-surface.txt" + +// headerSep ends the human-facing preamble of the golden file. Everything after it is the +// surface itself, so the warning can be reworded without touching the comparison. +const headerSep = "# ---\n" + +const surfaceHeader = `# Exported API surface promised for v1 - see content/docs/compatibility.md. +# A line changed or removed below is a breaking change and needs a major version. +# A line added is a new promise: it cannot be taken back within v1. +# Regenerate deliberately: go test -run TestPublicAPISurface . -update +` + headerSep + +var updateSurface = flag.Bool("update", false, "rewrite "+goldenPath+" from the current source") + +// TestPublicAPISurface fails when the exported surface of this package changes, so that +// widening or breaking the v1 promise is a deliberate act rather than a side effect of some +// other edit. Doc comments are deliberately not part of it: the parser is told to skip them, +// so rewording one costs nothing here. +func TestPublicAPISurface(t *testing.T) { + got := exportedSurface(t) + + if *updateSurface { + if err := os.MkdirAll("testdata", 0o750); err != nil { + t.Fatalf("create testdata: %v", err) + } + + if err := os.WriteFile(goldenPath, []byte(surfaceHeader+got), 0o600); err != nil { + t.Fatalf("write %s: %v", goldenPath, err) + } + + t.Logf("wrote %s", goldenPath) + + return + } + + raw, err := os.ReadFile(goldenPath) + if err != nil { + t.Fatalf("read %s: %v (regenerate with -update)", goldenPath, err) + } + + _, want, ok := strings.Cut(string(raw), headerSep) + if !ok { + t.Fatalf("%s has no %q separator; regenerate with -update", goldenPath, headerSep) + } + + if got != want { + t.Errorf("exported API surface changed.\n%s\n\n"+ + "A changed or removed line is a breaking change; an added line is a new promise.\n"+ + "See content/docs/compatibility.md. If deliberate, regenerate with:\n"+ + " go test -run TestPublicAPISurface . -update", + firstDifference(want, got)) + } +} + +// exportedSurface renders every exported declaration in the package directory, one block per +// file in name order so the output is stable across runs. +func exportedSurface(t *testing.T) string { + t.Helper() + + entries, err := os.ReadDir(".") + if err != nil { + t.Fatalf("read package directory: %v", err) + } + + fset := token.NewFileSet() + + var buf bytes.Buffer + + for _, entry := range entries { + name := entry.Name() + if entry.IsDir() || !strings.HasSuffix(name, ".go") || strings.HasSuffix(name, "_test.go") { + continue + } + + // Mode 0 leaves comments unparsed, which is what keeps a reworded doc comment from + // failing this test: only the declarations are the promise. + file, err := parser.ParseFile(fset, name, nil, 0) + if err != nil { + t.Fatalf("parse %s: %v", name, err) + } + + // FileExports drops unexported declarations, struct fields, and interface methods. + // It keeps the bodies of the exported functions, which is why signaturesOnly runs + // after it: how a method is written is not what callers depend on. + if !ast.FileExports(file) { + continue + } + + file.Decls = signaturesOnly(file.Decls) + + fmt.Fprintf(&buf, "==== %s ====\n", name) + + if err := printer.Fprint(&buf, fset, file); err != nil { + t.Fatalf("print %s: %v", name, err) + } + + buf.WriteString("\n\n") + } + + // One trailing newline, which is what the repository's end-of-file hook normalises the + // golden file to. Generating anything else makes committing it fail this test. + return strings.TrimRight(buf.String(), "\n") + "\n" +} + +// signaturesOnly drops the import block and every function body, leaving the declarations a +// caller can actually depend on. Without it an edit to the inside of an exported method would +// read as a change to the promise. +func signaturesOnly(decls []ast.Decl) []ast.Decl { + kept := decls[:0] + + for _, decl := range decls { + switch d := decl.(type) { + case *ast.GenDecl: + if d.Tok == token.IMPORT { + continue + } + case *ast.FuncDecl: + d.Body = nil + } + + kept = append(kept, decl) + } + + return kept +} + +// firstDifference points at the first line that does not match. The whole change is in +// `git diff` on the golden file; what a failing run needs is where to start looking. +func firstDifference(want, got string) string { + wantLines, gotLines := strings.Split(want, "\n"), strings.Split(got, "\n") + + for i := 0; i < len(wantLines) || i < len(gotLines); i++ { + wantLine, gotLine := lineAt(wantLines, i), lineAt(gotLines, i) + if wantLine != gotLine { + return fmt.Sprintf("first difference at line %d:\n promised: %q\n found: %q", + i+1, wantLine, gotLine) + } + } + + return "no line differs, but the text does (trailing whitespace?)" +} + +func lineAt(lines []string, i int) string { + if i >= len(lines) { + return "" + } + + return lines[i] +} diff --git a/changes/unreleased/Added-20260809-000000.yaml b/changes/unreleased/Added-20260809-000000.yaml new file mode 100644 index 0000000..f6db11f --- /dev/null +++ b/changes/unreleased/Added-20260809-000000.yaml @@ -0,0 +1,3 @@ +kind: Added +body: A compatibility page states what v1 promises not to break, what it leaves out, and the trust boundary kvs assumes; the exported Go surface is pinned in testdata/api-surface.txt and checked by a test, so it cannot widen or narrow unnoticed +time: 2026-08-09T00:00:00.000000+09:00 diff --git a/content/docs/compatibility.md b/content/docs/compatibility.md new file mode 100644 index 0000000..c4970c8 --- /dev/null +++ b/content/docs/compatibility.md @@ -0,0 +1,124 @@ +--- +title: "Compatibility" +weight: 6 +--- + +A `v1` tag is a promise: what this page lists will not break until `v2`, and what it does not +list may change in any release. This is the page to read before pinning kvs in something you +have to keep running. + +Versions follow [Semantic Versioning](https://semver.org/). Within `v1.x.y`, a new minor +release may add to the list below and a patch release may only fix behaviour that already +contradicts it. + +## What v1 covers + +### The wire protocols + +All three protocols share one keyspace, and that stays true: a value written over HTTP is +readable over RESP and gRPC, and the reverse. + +| Protocol | Promised | Details | +|---|---|---| +| HTTP | `PUT`, `GET`, `DELETE` on `/v1/keys/{key}`, and `/healthz` — paths, methods, request and response bodies, status codes | [HTTP API](../http-api/) | +| gRPC | The service defined by the protobuf files under `api/kvsv1`, and the standard gRPC health service | `api/kvsv1` | +| RESP2 | The commands listed on the Redis API page, with the replies documented there | [Redis API](../redis-api/) | + +For gRPC the `.proto` file is the contract. The Go identifiers `protoc` generates from it +follow `protoc`'s own rules, so they are promised only insofar as the proto is. + +A RESP command that is not on the Redis API page is not promised, even if the server happens +to answer it. + +### The command line + +Every flag on the [CLI Usage](../cli/) page keeps its name, its meaning, and its default, +along with the config-file key and environment variable that set it. `kvs serve` with no flags +keeps listening on `:3456` for HTTP, `:3457` for gRPC, and `127.0.0.1:6379` for RESP. +`kvs version` keeps printing a version. + +New flags may be added. Existing ones will not change under you. + +### The Go library + +Importing `github.com/skyoo2003/kvs` gets you: + +- `Store` and its constructors `NewStore` and `Open`, with `Get`, `Put`, `Delete`, `Read`, + `Write`, `Snapshot`, `Speculate`, `Watch`, `SetCodec`, and `Close` +- The transaction types `ReadTx` and `Tx`, and `Watch` +- `Entry`, and the `Codec` interface with `StringCodec` +- The sentinel errors `ErrKeyNotFound`, `ErrNoCodec`, `ErrUnsupportedValue`, and `ErrNotLeader` + with `NotLeaderError` + +The exact signatures live in [`testdata/api-surface.txt`][surface], which is generated from +the source and compared against it by a test on every run. Nothing can join or leave that file +without the change showing up in review. + +[surface]: https://github.com/skyoo2003/kvs/blob/main/testdata/api-surface.txt + +## What v1 does not cover + +**Cluster plumbing on `Store`.** `SetReplicator`, `ReplaceWith`, and `ApplyReplicated` are +exported so `internal/cluster` can reach them across the package boundary, not for callers +importing this package. They may change or disappear in a minor release. + +**`github.com/skyoo2003/kvs/pkg/resp`.** It exists so the server can speak RESP2, not as a +RESP library for other programs. The protocol kvs answers on the wire is promised; this Go +package is not. + +**Anything under `internal/`.** The Go toolchain already stops you importing it; this is the +same statement in words. + +**The on-disk format.** The append log in `--data-dir`, and the Raft directory beside it, carry +no format version yet. Data written by one release is not promised to be readable by another. +Until that changes, treat an upgrade as: drain, start the new version against an empty +directory, reload. + +**Performance.** Throughput, latency, and memory are not part of the promise. kvs is built for +not losing writes, not for being fast, and a release may trade one for the other. + +**The Go version.** kvs builds with the Go release named in `go.mod`. A minor release may +require a newer one. + +## Breaking it anyway + +Something on this page can only be removed or changed in `v2`. Before that it gets deprecated +in a `v1.x` release — still working, marked in its documentation and in the release notes — +and stays that way until the next major. + +One exception: a fix for a security vulnerability may break a promise in a minor release. When +that happens the release notes say so explicitly, in those words. + +## Trust boundary + +**kvs has no authentication, no authorization, and no TLS.** Anyone who can open a connection +to any of the three listeners can read, change, and delete everything in the keyspace. On a +clustered node they can also join the cluster, because `--join` goes through the RESP listener +rather than a second port with its own credentials. + +The defaults reflect that unevenly, and it is worth knowing which is which: + +| Listener | Default | Reachable from | +|---|---|---| +| HTTP | `:3456` | every interface | +| gRPC | `:3457` | every interface | +| RESP | `127.0.0.1:6379` | loopback only | + +RESP defaults to loopback because port 6379 is scanned continuously across the public +internet. HTTP and gRPC do not, so a kvs started with defaults on a machine with a public +address is exposed on two ports. + +Run kvs on a network you trust, or put something that authenticates in front of it. Do not put +a listener on a public address. + +This is a description of v1, not a permanent position. Adding authentication later adds to the +promise rather than breaking it, so it can arrive in a `v1.x` release. + +## Further Reading + +- [Overview](../overview/) — installation and library usage +- [CLI Usage](../cli/) — every flag, config key, and environment variable +- [HTTP API](../http-api/) — REST endpoint details +- [Redis API](../redis-api/) — supported RESP commands and behaviour notes +- [Durability and Clustering](../clustering/) — what `--data-dir` and `--raft-addr` promise +- [Release Process](../release/) — how a version gets cut diff --git a/content/docs/contributing.md b/content/docs/contributing.md index fecc7b3..301b7c5 100644 --- a/content/docs/contributing.md +++ b/content/docs/contributing.md @@ -1,6 +1,6 @@ --- title: "Contributing" -weight: 6 +weight: 7 --- Contributions are welcome! See [`CONTRIBUTING.md`](https://github.com/skyoo2003/kvs/blob/main/CONTRIBUTING.md) for the full guide. diff --git a/content/docs/overview.md b/content/docs/overview.md index d955643..dc9a575 100644 --- a/content/docs/overview.md +++ b/content/docs/overview.md @@ -69,6 +69,11 @@ func main() { } ``` +## Compatibility + +What `v1` promises not to break, what it deliberately leaves out, and the trust boundary kvs +assumes are on the [Compatibility](../compatibility/) page. + ## License MIT License. See [LICENSE](https://github.com/skyoo2003/kvs/blob/main/LICENSE) for details. diff --git a/content/docs/release.md b/content/docs/release.md index 16a4ce6..af64dbf 100644 --- a/content/docs/release.md +++ b/content/docs/release.md @@ -1,6 +1,6 @@ --- title: "Release Process" -weight: 7 +weight: 8 --- The project uses [Changie](https://github.com/miniscruff/changie) for changelog management and [GoReleaser](https://goreleaser.com/) for automated releases. diff --git a/pkg/resp/reader.go b/pkg/resp/reader.go index 0329731..1b99414 100644 --- a/pkg/resp/reader.go +++ b/pkg/resp/reader.go @@ -1,4 +1,9 @@ // Package resp implements the RESP2 wire protocol spoken by Redis and Valkey clients. +// +// It exists for the server in this module to speak that protocol, not as a RESP library for +// other programs, and is outside the v1 compatibility promise: see +// content/docs/compatibility.md. The protocol kvs answers on the wire is promised; this Go +// package is not. package resp import ( diff --git a/replication.go b/replication.go index 4051e0d..9ece8af 100644 --- a/replication.go +++ b/replication.go @@ -33,6 +33,9 @@ func (e *NotLeaderError) Is(target error) bool { // // Set it before anything serves: it is read without the lock a write would take, on the // understanding that it is wired up once at startup. +// +// Exported for internal/cluster to reach across the package boundary, and outside the v1 +// compatibility promise: see content/docs/compatibility.md. func (s *Store) SetReplicator(replicate func(fn func(tx *Tx) error) error) { s.mu.Lock() defer s.mu.Unlock() @@ -60,6 +63,9 @@ func (s *Store) SetCodec(codec Codec) { // ReplaceWith throws the keyspace away and rebuilds it from snapshot, which is what a node // restored from a cluster snapshot needs: the agreed state is the only authority, so whatever the // node held before is worth keeping only until it arrives. +// +// Exported for internal/cluster to reach across the package boundary, and outside the v1 +// compatibility promise: see content/docs/compatibility.md. func (s *Store) ReplaceWith(snapshot [][]byte) error { s.mu.Lock() defer s.mu.Unlock() @@ -74,6 +80,9 @@ func (s *Store) ReplaceWith(snapshot [][]byte) error { // ApplyReplicated applies one frame the cluster has agreed on. A frame is one transaction on the // node that took the write, and applying it inside one transaction here is what keeps a MULTI // atomic on every node. +// +// Exported for internal/cluster to reach across the package boundary, and outside the v1 +// compatibility promise: see content/docs/compatibility.md. func (s *Store) ApplyReplicated(lines [][]byte) error { s.mu.Lock() defer s.mu.Unlock() diff --git a/testdata/api-surface.txt b/testdata/api-surface.txt new file mode 100644 index 0000000..9d4ec0d --- /dev/null +++ b/testdata/api-surface.txt @@ -0,0 +1,124 @@ +# Exported API surface promised for v1 - see content/docs/compatibility.md. +# A line changed or removed below is a breaking change and needs a major version. +# A line added is a new promise: it cannot be taken back within v1. +# Regenerate deliberately: go test -run TestPublicAPISurface . -update +# --- +==== kvs.go ==== +package kvs + +var ( + ErrKeyNotFound = errors.New("key not found") + + ErrNoCodec = errors.New("no codec set") +) + +type Entry struct { + Value interface{} + ExpiresAt time.Time +} + +type Store struct { + // contains filtered or unexported fields +} + +func NewStore() *Store + +func Open(dir string, codec Codec) (*Store, error) + +func (s *Store) Close() error + +func (s *Store) Read(fn func(tx *ReadTx) error) error + +func (s *Store) Write(fn func(tx *Tx) error) error + +func (s *Store) Speculate(fn func(tx *Tx) error) ([][]byte, error) + +func (s *Store) Snapshot() ([][]byte, error) + +func (s *Store) Watch(keys ...string) *Watch + +func (s *Store) Get(key string) (interface{}, error) + +func (s *Store) Put(key string, value interface{}) error + +func (s *Store) Delete(key string) error + +type ReadTx struct { + // contains filtered or unexported fields +} + +func (tx *ReadTx) Now() time.Time + +func (tx *ReadTx) Get(key string) (Entry, bool) + +func (tx *ReadTx) Keys() []string + +func (tx *ReadTx) SortedKeys() []string + +func (tx *ReadTx) Len() int + +func (tx *ReadTx) Expiring() int + +type Tx struct { + ReadTx + // contains filtered or unexported fields +} + +func (tx *Tx) Get(key string) (Entry, bool) + +func (tx *Tx) Set(key string, entry Entry) + +func (tx *Tx) Delete(key string) bool + +func (tx *Tx) Flush() + +type Watch struct { + // contains filtered or unexported fields +} + +func (w *Watch) Conflicted() bool + +func (w *Watch) Close() + + +==== log.go ==== +package kvs + +var ErrUnsupportedValue = errors.New("unsupported value type") + +type Codec interface { + Encode(value interface{}) ([]byte, error) + Decode(data []byte) (interface{}, error) + + Clone(value interface{}) interface{} +} + +type StringCodec struct{} + +func (StringCodec) Encode(value interface{}) ([]byte, error) + +func (StringCodec) Decode(data []byte) (interface{}, error) + +func (StringCodec) Clone(value interface{}) interface{} + + +==== replication.go ==== +package kvs + +var ErrNotLeader = errors.New("not the leader") + +type NotLeaderError struct { + Leader string +} + +func (e *NotLeaderError) Error() string + +func (e *NotLeaderError) Is(target error) bool + +func (s *Store) SetReplicator(replicate func(fn func(tx *Tx) error) error) + +func (s *Store) SetCodec(codec Codec) + +func (s *Store) ReplaceWith(snapshot [][]byte) error + +func (s *Store) ApplyReplicated(lines [][]byte) error From 44992a19adf1b72a5055741d45a1f59477973125 Mon Sep 17 00:00:00 2001 From: Sung-Kyu Yoo Date: Sun, 9 Aug 2026 12:47:06 +0900 Subject: [PATCH 2/5] Record the issue number on the changelog fragment --- changes/unreleased/Added-20260809-000000.yaml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/changes/unreleased/Added-20260809-000000.yaml b/changes/unreleased/Added-20260809-000000.yaml index f6db11f..13e6eba 100644 --- a/changes/unreleased/Added-20260809-000000.yaml +++ b/changes/unreleased/Added-20260809-000000.yaml @@ -1,3 +1,5 @@ kind: Added body: A compatibility page states what v1 promises not to break, what it leaves out, and the trust boundary kvs assumes; the exported Go surface is pinned in testdata/api-surface.txt and checked by a test, so it cannot widen or narrow unnoticed time: 2026-08-09T00:00:00.000000+09:00 +custom: + Issue: "244" From 9e24a97a0dc6d3cdc4e8dbfd8e03560f35da8545 Mon Sep 17 00:00:00 2001 From: Sung-Kyu Yoo Date: Sun, 9 Aug 2026 21:42:31 +0900 Subject: [PATCH 3/5] Correct three claims on the compatibility page The page said all three protocols read each other's values, that kvs has no authentication at all, and that `kvs serve` keeps listening on the default RESP address. None of the three holds: HTTP and gRPC report an error for a key holding a RESP list, hash, set, or sorted set; `resp_password` makes the RESP listener require AUTH before anything else, cluster joins included; and a default RESP port already taken is logged and skipped rather than being fatal. The golden surface keeps the exempt cluster plumbing, because it records what the package exports rather than what v1 promises, and its header now says where the exemptions are written down. --- api_surface_test.go | 11 ++++++----- content/docs/compatibility.md | 37 +++++++++++++++++++++-------------- testdata/api-surface.txt | 7 ++++--- 3 files changed, 32 insertions(+), 23 deletions(-) diff --git a/api_surface_test.go b/api_surface_test.go index 3288daf..e6e3418 100644 --- a/api_surface_test.go +++ b/api_surface_test.go @@ -21,9 +21,10 @@ const goldenPath = "testdata/api-surface.txt" // surface itself, so the warning can be reworded without touching the comparison. const headerSep = "# ---\n" -const surfaceHeader = `# Exported API surface promised for v1 - see content/docs/compatibility.md. -# A line changed or removed below is a breaking change and needs a major version. -# A line added is a new promise: it cannot be taken back within v1. +const surfaceHeader = `# Exported API surface of this package - see content/docs/compatibility.md +# for how much of it v1 promises, and for the cluster plumbing it exempts by name. +# A line changed or removed below is a breaking change and needs a major version, unless the +# page exempts it. A line added is a new promise: it cannot be taken back within v1. # Regenerate deliberately: go test -run TestPublicAPISurface . -update ` + headerSep @@ -62,8 +63,8 @@ func TestPublicAPISurface(t *testing.T) { if got != want { t.Errorf("exported API surface changed.\n%s\n\n"+ - "A changed or removed line is a breaking change; an added line is a new promise.\n"+ - "See content/docs/compatibility.md. If deliberate, regenerate with:\n"+ + "A changed or removed line is a breaking change unless compatibility.md exempts it;\n"+ + "an added line is a new promise. See content/docs/compatibility.md. If deliberate:\n"+ " go test -run TestPublicAPISurface . -update", firstDifference(want, got)) } diff --git a/content/docs/compatibility.md b/content/docs/compatibility.md index c4970c8..cfa3951 100644 --- a/content/docs/compatibility.md +++ b/content/docs/compatibility.md @@ -15,8 +15,10 @@ contradicts it. ### The wire protocols -All three protocols share one keyspace, and that stays true: a value written over HTTP is -readable over RESP and gRPC, and the reverse. +All three protocols share one keyspace, and that stays true for the type all three can carry: a +string written over HTTP is readable over RESP and gRPC, and the reverse. RESP's lists, hashes, +sets, and sorted sets have no HTTP or gRPC representation, so asking either for a key holding +one is an error rather than a value. | Protocol | Promised | Details | |---|---|---| @@ -34,8 +36,9 @@ to answer it. Every flag on the [CLI Usage](../cli/) page keeps its name, its meaning, and its default, along with the config-file key and environment variable that set it. `kvs serve` with no flags -keeps listening on `:3456` for HTTP, `:3457` for gRPC, and `127.0.0.1:6379` for RESP. -`kvs version` keeps printing a version. +keeps asking for `:3456` for HTTP, `:3457` for gRPC, and `127.0.0.1:6379` for RESP — the +addresses are the promise, not that each one binds, because a default RESP port already taken +is logged and skipped rather than being fatal. `kvs version` keeps printing a version. New flags may be added. Existing ones will not change under you. @@ -91,18 +94,22 @@ that happens the release notes say so explicitly, in those words. ## Trust boundary -**kvs has no authentication, no authorization, and no TLS.** Anyone who can open a connection -to any of the three listeners can read, change, and delete everything in the keyspace. On a -clustered node they can also join the cluster, because `--join` goes through the RESP listener -rather than a second port with its own credentials. +**kvs has no authorization and no TLS, and only one of its three listeners can ask who you +are.** Set `resp_password` in the config file or `KVS_RESP_PASSWORD` in the environment — there +is deliberately no flag, because a flag puts the password in the process list — and the RESP +listener requires `AUTH` before it answers anything, cluster joins included, since `--join` goes +through that listener rather than a second port with its own credentials. HTTP and gRPC have +nothing of the kind: anyone who can open a connection to either can read, change, and delete +everything in the keyspace, and no listener encrypts anything, so a RESP password crosses the +wire in the clear. The defaults reflect that unevenly, and it is worth knowing which is which: -| Listener | Default | Reachable from | -|---|---|---| -| HTTP | `:3456` | every interface | -| gRPC | `:3457` | every interface | -| RESP | `127.0.0.1:6379` | loopback only | +| Listener | Default | Reachable from | Can require a password | +|---|---|---|---| +| HTTP | `:3456` | every interface | No | +| gRPC | `:3457` | every interface | No | +| RESP | `127.0.0.1:6379` | loopback only | Yes, with `resp_password` | RESP defaults to loopback because port 6379 is scanned continuously across the public internet. HTTP and gRPC do not, so a kvs started with defaults on a machine with a public @@ -111,8 +118,8 @@ address is exposed on two ports. Run kvs on a network you trust, or put something that authenticates in front of it. Do not put a listener on a public address. -This is a description of v1, not a permanent position. Adding authentication later adds to the -promise rather than breaking it, so it can arrive in a `v1.x` release. +This is a description of v1, not a permanent position. Giving the other two listeners the same +option adds to the promise rather than breaking it, so it can arrive in a `v1.x` release. ## Further Reading diff --git a/testdata/api-surface.txt b/testdata/api-surface.txt index 9d4ec0d..ffc008b 100644 --- a/testdata/api-surface.txt +++ b/testdata/api-surface.txt @@ -1,6 +1,7 @@ -# Exported API surface promised for v1 - see content/docs/compatibility.md. -# A line changed or removed below is a breaking change and needs a major version. -# A line added is a new promise: it cannot be taken back within v1. +# Exported API surface of this package - see content/docs/compatibility.md +# for how much of it v1 promises, and for the cluster plumbing it exempts by name. +# A line changed or removed below is a breaking change and needs a major version, unless the +# page exempts it. A line added is a new promise: it cannot be taken back within v1. # Regenerate deliberately: go test -run TestPublicAPISurface . -update # --- ==== kvs.go ==== From ddf792ef9a5347f441a6cd17a7a7449c8827ef70 Mon Sep 17 00:00:00 2001 From: Sung-Kyu Yoo Date: Mon, 10 Aug 2026 22:40:35 +0900 Subject: [PATCH 4/5] Give an HTTP 405 the JSON body the docs promise The HTTP API page says all errors return a JSON body and lists 405 among them, but both method checks answered with a bare WriteHeader: no body, no JSON content type. The compatibility page now makes those response bodies part of v1, so this is the last release where the empty one can be corrected rather than kept. The 405 path had no test at all, which is how it stayed wrong. --- changes/unreleased/Fixed-20260810-100000.yaml | 5 ++++ internal/server/http.go | 4 +-- internal/server/http_test.go | 28 +++++++++++++++++++ 3 files changed, 35 insertions(+), 2 deletions(-) create mode 100644 changes/unreleased/Fixed-20260810-100000.yaml diff --git a/changes/unreleased/Fixed-20260810-100000.yaml b/changes/unreleased/Fixed-20260810-100000.yaml new file mode 100644 index 0000000..389cef8 --- /dev/null +++ b/changes/unreleased/Fixed-20260810-100000.yaml @@ -0,0 +1,5 @@ +kind: Fixed +body: An HTTP 405 now carries the same JSON error body as every other HTTP error, instead of an empty response the documented contract did not allow +time: 2026-08-10T10:00:00.000000+09:00 +custom: + Issue: "244" diff --git a/internal/server/http.go b/internal/server/http.go index 6b09df1..8e7576c 100644 --- a/internal/server/http.go +++ b/internal/server/http.go @@ -42,7 +42,7 @@ func NewHTTPHandler(store *kvs.Store) http.Handler { func (h *httpHandler) handleHealth(w http.ResponseWriter, r *http.Request) { if r.Method != http.MethodGet { - w.WriteHeader(http.StatusMethodNotAllowed) + writeJSONError(w, http.StatusMethodNotAllowed, "method not allowed") return } @@ -64,7 +64,7 @@ func (h *httpHandler) handleKey(w http.ResponseWriter, r *http.Request) { case http.MethodDelete: h.handleDelete(w, key) default: - w.WriteHeader(http.StatusMethodNotAllowed) + writeJSONError(w, http.StatusMethodNotAllowed, "method not allowed") } } diff --git a/internal/server/http_test.go b/internal/server/http_test.go index 811a917..c514b34 100644 --- a/internal/server/http_test.go +++ b/internal/server/http_test.go @@ -67,6 +67,34 @@ func TestHTTPHandlerPutGetDelete(t *testing.T) { } } +// TestHTTPHandlerRejectsUnsupportedMethod covers the promise the HTTP API page makes about every +// error carrying a JSON body, which a bare WriteHeader would break for 405 alone. +func TestHTTPHandlerRejectsUnsupportedMethod(t *testing.T) { + for _, target := range []string{"/healthz", "/v1/keys/language"} { + handler := NewHTTPHandler(kvs.NewStore()) + req := newTestRequest(t, http.MethodPost, target, http.NoBody) + res := httptest.NewRecorder() + + handler.ServeHTTP(res, req) + + if res.Code != http.StatusMethodNotAllowed { + t.Errorf("POST %s status = %d, want %d", target, res.Code, http.StatusMethodNotAllowed) + } + if got := res.Header().Get("Content-Type"); got != "application/json" { + t.Errorf("POST %s content type = %q, want application/json", target, got) + } + + var body errorResponse + if err := json.Unmarshal(res.Body.Bytes(), &body); err != nil { + t.Errorf("POST %s json.Unmarshal(%q) error = %v", target, res.Body.String(), err) + continue + } + if body.Error == "" { + t.Errorf("POST %s error message is empty", target) + } + } +} + func TestHTTPHandlerMissingKey(t *testing.T) { handler := NewHTTPHandler(kvs.NewStore()) req := newTestRequest(t, http.MethodGet, "/v1/keys/missing", http.NoBody) From 67cc72c83c54c502b72be36317d74fbafa252243 Mon Sep 17 00:00:00 2001 From: Sung-Kyu Yoo Date: Mon, 10 Aug 2026 22:40:52 +0900 Subject: [PATCH 5/5] Correct the authentication warning and the flag promise The README repeated the claim that kvs has no authentication, which the compatibility page had already stopped making: `resp_password` gives the RESP listener an AUTH requirement, and only HTTP and gRPC are unauthenticated whatever the operator does. The command-line promise covered every flag on the CLI page, including the config-file key and environment variable that set it. `--config`, `--version`, and `--help` have neither, because the CLI answers them itself rather than passing them to the server, so that half of the promise is now scoped to the `serve` settings that do have Viper mappings. The CLI page overstated the same thing and now says `serve` too. --- README.md | 4 ++-- content/docs/cli.md | 2 +- content/docs/compatibility.md | 6 ++++-- 3 files changed, 7 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index fcdf75a..5b61283 100644 --- a/README.md +++ b/README.md @@ -174,8 +174,8 @@ each of these, and what to do about them. What `v1` promises not to break, and what it deliberately leaves out, is on the [compatibility page](https://skyoo2003.github.io/kvs/docs/compatibility/). Read it before -pinning kvs, and before opening a port: kvs has no authentication, no authorization, and no -TLS, and HTTP and gRPC listen on every interface by default. +pinning kvs, and before opening a port: kvs has no authorization and no TLS, only the RESP +listener can be given a password, and HTTP and gRPC listen on every interface by default. ## Documentation diff --git a/content/docs/cli.md b/content/docs/cli.md index 81719e9..417dddb 100644 --- a/content/docs/cli.md +++ b/content/docs/cli.md @@ -47,7 +47,7 @@ surfacing later. What a cluster does and does not promise is on the ### Configuration without flags -Every flag has a config file and environment equivalent. The RESP password has **only** +Every `serve` flag has a config file and environment equivalent. The RESP password has **only** those: a credential passed as an argument is visible to anything that can list processes. | Setting | Config key | Environment | diff --git a/content/docs/compatibility.md b/content/docs/compatibility.md index cfa3951..dba7cf9 100644 --- a/content/docs/compatibility.md +++ b/content/docs/compatibility.md @@ -34,8 +34,10 @@ to answer it. ### The command line -Every flag on the [CLI Usage](../cli/) page keeps its name, its meaning, and its default, -along with the config-file key and environment variable that set it. `kvs serve` with no flags +Every flag on the [CLI Usage](../cli/) page keeps its name, its meaning, and its default. Each +`kvs serve` setting keeps the config-file key and environment variable that set it too; +`--config`, `--version`, and `--help` have neither, because the CLI answers those itself rather +than passing them to the server. `kvs serve` with no flags keeps asking for `:3456` for HTTP, `:3457` for gRPC, and `127.0.0.1:6379` for RESP — the addresses are the promise, not that each one binds, because a default RESP port already taken is logged and skipped rather than being fatal. `kvs version` keeps printing a version.