diff --git a/changes/unreleased/Fixed-20260906-153000.yaml b/changes/unreleased/Fixed-20260906-153000.yaml new file mode 100644 index 00000000..76448396 --- /dev/null +++ b/changes/unreleased/Fixed-20260906-153000.yaml @@ -0,0 +1,5 @@ +kind: Fixed +body: 'The four Amazon Lex services are reachable from boto3. `lex-models`, `lex-runtime`, `lexv2-models` and `lexv2-runtime` all sign as `lex`, and no service is called `lex`, so the alias stayed contested and every Lex call died as `UnknownService` — the services were registered, counted as serving operations, and reachable by nobody. The shared-signing-name split that already separates `mediastore-data`, `sagemaker-runtime` and Timestream now recognises a signing name that is a group key rather than a member service ID, so each request is answered by the sibling whose route table models its method and path. `DeleteBot` is claimed by two of them at `DELETE /bots/{id}` and is still refused rather than guessed at. Compatibility-tested services: 199 to 203' +time: 2026-09-06T15:30:00.000000+09:00 +custom: + Issue: "146" diff --git a/changes/unreleased/Fixed-20260906-160000.yaml b/changes/unreleased/Fixed-20260906-160000.yaml new file mode 100644 index 00000000..db35a0f9 --- /dev/null +++ b/changes/unreleased/Fixed-20260906-160000.yaml @@ -0,0 +1,5 @@ +kind: Fixed +body: 'Three operations the fidelity manifest calls `hand-verified` are now reachable. `appsync.ListApis`, `eks.ListAccessPolicies` and `opensearch.ListApplications` were each implemented by a `case` clause in their provider, but the provider''s hand-written path resolver did not know the route boto3 sends — `GET /v2/apis`, `GET /access-policies` and `GET /2021-01-01/opensearch/list-applications` — so the operation arrived empty and the call answered `NotImplemented`. Each provider now falls back to its own model''s route table when its resolver recognises nothing, which recovers the operation instead of losing it. No published figure moves: these operations were already labelled served, and the label is now true' +time: 2026-09-06T16:00:00.000000+09:00 +custom: + Issue: "146" diff --git a/changes/unreleased/Fixed-20260906-170000.yaml b/changes/unreleased/Fixed-20260906-170000.yaml new file mode 100644 index 00000000..b2144bd4 --- /dev/null +++ b/changes/unreleased/Fixed-20260906-170000.yaml @@ -0,0 +1,5 @@ +kind: Fixed +body: 'The fidelity manifest no longer drops operations with short names. The scan that decides which operations are hand-verified required a name of four characters or more, so `resourcegroups.Untag` was recorded and `Tag` — implemented beside it in the same dispatch switch — was reported as unimplemented. A probe then asked for `Tag`, got the answer the provider has always given, and reported a fabricated success that was never fabricated: the code was real and the manifest was wrong about it. Short dispatch literals are now collected separately and promoted only where the service''s own model declares them, so `Tag` counts and the `GET`/`PUT` literals every path resolver switches on still cannot become operations. Hand-verified operations: 4,496 to 4,497' +time: 2026-09-06T17:00:00.000000+09:00 +custom: + Issue: "146" diff --git a/changes/unreleased/Fixed-20260906-180000.yaml b/changes/unreleased/Fixed-20260906-180000.yaml new file mode 100644 index 00000000..c6b2a918 --- /dev/null +++ b/changes/unreleased/Fixed-20260906-180000.yaml @@ -0,0 +1,5 @@ +kind: Fixed +body: 'S3 no longer answers an unimplemented bucket sub-resource with a bucket listing. `GET /{Bucket}?analytics` matched no sub-resource branch and fell through to `ListObjects`, so botocore read the resulting 200 `` as a successful `ListBucketAnalyticsConfigurations`. `?inventory`, `?metrics`, `?replication`, `?lifecycle`, `?encryption`, `?versions` and `?object-lock` all took the same path. A bucket-level GET carrying a query parameter that is neither a sub-resource this provider serves nor a parameter a listing carries now returns a clean `NotImplemented`. The check is an allow-list of listing parameters rather than a deny-list of sub-resource names, so a sub-resource AWS adds later declines instead of being served as a listing' +time: 2026-09-06T18:00:00.000000+09:00 +custom: + Issue: "146" diff --git a/docs/coverage.md b/docs/coverage.md index d6754e9e..79c2fe34 100644 --- a/docs/coverage.md +++ b/docs/coverage.md @@ -12,7 +12,7 @@ things: | **Registered** | The gateway routes the service. A call reaches DevCloud instead of falling through to real AWS. | **205** | | **Serving ≥1 operation** | At least one operation returns a real, store-backed answer — hand-written or served by the generic CRUD engine. | **201** | | **Registered-only** | Routed, but every operation declines with a clean AWS error. Nothing is served. | **4** | -| **Compatibility-tested** | A boto3 test exercises the service in CI and passes — either serving an operation or declining cleanly. | **199** | +| **Compatibility-tested** | A boto3 test exercises the service in CI and passes — either serving an operation or declining cleanly. | **203** | Every number on this page is asserted against the binary by `go test ./cmd/devcloud/`. Editing one here without the code moving fails CI, and @@ -22,9 +22,9 @@ Per operation, from the [fidelity manifest](fidelity-manifest.md): | Tier | Operations | |---|---| -| `hand-verified` | 4,496 | +| `hand-verified` | 4,497 | | `auto-crud` | 5,193 | -| `unimplemented` | 2,718 | +| `unimplemented` | 2,717 | | **total known** | **12,407** | ## Why a registered service can serve nothing @@ -63,13 +63,13 @@ and `sagemakerruntimehttp2` (`InvokeEndpoint*`), and `rds-data` (`ExecuteStatement`, `BeginTransaction`, `CommitTransaction`, `RollbackTransaction`). No protocol change reaches them. -## Why the compatibility-tested number is 199, not 205 +## Why the compatibility-tested number is 203, not 205 Every registered service is exercised by `tests/compatibility/test_service_smoke.py`, which parametrises over the generated service list rather than a hand-written one — so a service cannot be registered and quietly go untested, which is what 31 of them were until this was -measured. Six are excluded, in two groups, and neither group is a backlog. +measured. Two are excluded, and they are not a backlog. **Two have no boto3 client at all.** botocore publishes 431 clients and neither `sagemakerruntimehttp2` nor `transcribestreaming` is among them — @@ -77,22 +77,25 @@ measured. Six are excluded, in two groups, and neither group is a backlog. No boto3 test can exist for a client that does not exist. This is a property of the AWS SDK, not of DevCloud, and it is the ceiling on this number. -**Four are registered and unreachable.** All four Lex clients — `lex-models`, -`lex-runtime`, `lexv2-models`, `lexv2-runtime` — sign as `lex`, and no service -is named `lex`, so the alias stays contested and routes nowhere rather than -sending one service's traffic to another. The consequence, which this page did -not state until it was measured: the "reached by its own unambiguous name" -escape below does not exist for a boto3 caller, because boto3 signs with the -contested name and offers nothing else. The four are counted in *serving ≥1 -operation*, because their operations are classified and would be served; they -are not counted here, because nothing can ask for them. - -Splitting them on the URL is what resolved `opensearch` / `elasticsearchservice` -and API Gateway v1 / v2, and it does not work here: `/bots` is the first path -segment for three of the four. A correct split needs full path matching across -all four, which is routing work rather than coverage work. -`test_lex_services_are_unreachable_from_boto3` pins the current state, so -fixing the routing fails that test and moves this number up with it. +**The four Lex services used to be a third exclusion, and are not any more.** +All four Lex clients — `lex-models`, `lex-runtime`, `lexv2-models`, +`lexv2-runtime` — sign as `lex`, and no service is named `lex`, so the alias is +contested and still routes nowhere on its own. What this page got wrong was the +escape it offered: "reached by its own unambiguous name" does not exist for a +boto3 caller, because boto3 signs with the contested name and offers nothing +else. Measuring it found four services registered, counted as serving, and +reachable by nobody. + +The split that resolved `opensearch` / `elasticsearchservice` and API Gateway +v1 / v2 does work here, and it needed no new routing. Services that share a +signing name are already separated by asking which one's route table models the +request; the group was simply never reached, because the lookup only recognised +a *member* service ID and `lex` is the group's *key*. Each Lex request now +reaches the sibling that models its method and path — `GET /bots` is +`lex-models`, `POST /bots` is `lexv2-models`, `/bot/…/session` is `lex-runtime`, +`/bots/…/botAliases/…/sessions/…` is `lexv2-runtime`. One operation is claimed by +two of them, `DeleteBot` at `DELETE /bots/{id}`, and it is still refused rather +than guessed at: deleting the wrong bot is worse than an honest error. Being engine-servable is not the same as being served. Thirteen of the fifteen `query` services and three of the four `rest-xml` ones have hand-written @@ -182,8 +185,14 @@ so `sagemaker` is claimed by eight services at once. Where exactly one claimant carries the name as its own service ID, that service wins and the rest are treated as borrowers — see `codegen.BuildAliases`. Where none does, the alias is left unrouted rather than guessed: all four Lex services sign as `lex` and none -is called `lex`, so `lex` routes nowhere and each Lex service is reached by its -own unambiguous name. +is called `lex`, so `lex` resolves to nothing on its own. + +An unrouted alias is not the same as an unreachable service, and this page once +conflated them — it claimed each Lex service was "reached by its own unambiguous +name", which no boto3 caller has, since boto3 sends the contested name and +nothing else. The name is still not guessed; the request is instead handed to +the group of services that share it, and answered by the one whose route table +models its method and path. Two candidates, or none, and it is refused. ## The target, and the rule that set it diff --git a/internal/codegen/gen_fidelity.go b/internal/codegen/gen_fidelity.go index 9326e610..b692ac05 100644 --- a/internal/codegen/gen_fidelity.go +++ b/internal/codegen/gen_fidelity.go @@ -91,9 +91,23 @@ func BuildFidelityData( hand[op] = true universe[op] = true } + declared := make(map[string]bool, len(modelOps[serviceID])) for _, op := range modelOps[serviceID] { + declared[op] = true universe[op] = true } + // A dispatch literal too short to be told from an HTTP verb by shape is + // an operation when, and only when, the model declares it. + // resourcegroups implements Tag and Untag in one switch and only Untag + // was long enough to be scanned, so the manifest called implemented code + // unimplemented. The model is asked here rather than in the scanner + // because this is where the model already is — the scan reads Go source, + // not Smithy. + for _, op := range provider.ShortOperations { + if declared[op] { + hand[op] = true + } + } for op := range crudOps[serviceID] { universe[op] = true } diff --git a/internal/codegen/gen_fidelity_test.go b/internal/codegen/gen_fidelity_test.go index b54e7864..d4888321 100644 --- a/internal/codegen/gen_fidelity_test.go +++ b/internal/codegen/gen_fidelity_test.go @@ -60,6 +60,49 @@ func TestBuildFidelityDataRequiresEngineWiring(t *testing.T) { "a provider that never reaches the engine serves nothing, whatever the CRUD registry says") } +// TestBuildFidelityDataPromotesShortDeclaredOperations covers the operation name +// too short for the scanner's shape test. +// +// opNamePattern requires four characters, so `Untag` is scanned and `Tag` is +// not, even though resourcegroups implements both in the same switch. The +// manifest called Tag unimplemented, a probe asked for it, got the 200 the +// provider has always returned, and reported a fabricated success that was +// never fabricated — the manifest was simply wrong about served code. +// +// Loosening the pattern alone is not the fix: at two characters it admits the +// HTTP-verb literals that path-resolver switches are full of. The model is what +// separates them. `Tag` is an operation because resourcegroups.json says so; +// `GET` is not an operation in any model AWS publishes. +func TestBuildFidelityDataPromotesShortDeclaredOperations(t *testing.T) { + modelOps := map[string][]string{"svc": {"Tag", "Untag", "ListGroups"}} + providers := map[string]ProviderScan{ + "svc": { + Operations: []string{"Untag", "ListGroups"}, + // Below the strict shape test. Only the model decides which of + // these is an operation. + ShortOperations: []string{"Tag", "GET", "PUT"}, + EngineWired: false, + }, + } + + data := BuildFidelityData(modelOps, map[string]string{"svc": "rest-json"}, providers, nil) + + assert.Equal(t, "HandVerified", tierOf(t, data, "svc", "Tag"), + "the model declares Tag and the provider dispatches it, so it is served") + assert.Equal(t, "HandVerified", tierOf(t, data, "svc", "Untag"), + "the long name was never in doubt and must not regress") + + for _, svc := range data.Services { + if svc.ServiceID != "svc" { + continue + } + for _, op := range svc.Ops { + assert.NotContains(t, []string{"GET", "PUT"}, op.Name, + "a short literal no model declares must not become an operation") + } + } +} + // TestBuildFidelityDataHandVerifiedBeatsWiring keeps the documented precedence: // a hand-written implementation always wins, and it does not depend on engine // wiring because such an operation never falls through to the engine at all. diff --git a/internal/codegen/scan_handverified.go b/internal/codegen/scan_handverified.go index 246782b8..2b0dab4d 100644 --- a/internal/codegen/scan_handverified.go +++ b/internal/codegen/scan_handverified.go @@ -21,6 +21,18 @@ import ( // switch with real operations (HTTP verbs, content types, protocol tokens). var opNamePattern = regexp.MustCompile(`^[A-Z][A-Za-z0-9]{3,}$`) +// shortOpNamePattern is the same shape at two or three characters — the length +// where a real operation name and an HTTP verb stop being distinguishable by +// shape. `Tag` is an operation of resourcegroups; `GET` and `PUT` are what a +// path resolver switches on, and every provider that routes by path has them. +// +// Literals matching this are collected into ProviderScan.ShortOperations and +// are not operations yet. BuildFidelityData promotes the ones the service's +// model declares, which is the only source that can tell the two apart. Without +// the split, resourcegroups.Tag was dropped while Untag beside it in the same +// switch was kept, and the manifest reported implemented code as unimplemented. +var shortOpNamePattern = regexp.MustCompile(`^[A-Z][A-Za-z0-9]{1,2}$`) + // pathRoutedOps lists the operations of providers that route on HTTP method and // path instead of an operation name, so no operation-name literal appears in // their HandleRequest for the scan to find. Keep in sync by hand when these @@ -70,6 +82,10 @@ var pathRoutedOps = map[string][]string{ type ProviderScan struct { // Operations are the operation names the provider dispatches by hand. Operations []string + // ShortOperations are dispatch literals too short to be told apart from an + // HTTP verb by shape. They become operations only where the service's model + // declares them; see shortOpNamePattern. + ShortOperations []string // EngineWired reports whether the provider hands the operations it does not // implement to the generic CRUD engine, by returning plugin.ErrUnhandledOp. // Only a wired provider's CRUD-shaped operations are actually served, so @@ -110,9 +126,11 @@ func ScanProviders(servicesDir string) (map[string]ProviderScan, error) { return nil, err } for id, recvType := range pkg.serviceTypes { + ops, short := pkg.dispatchedOps(recvType) result[id] = ProviderScan{ - Operations: opsFor(id, pkg.dispatchedOps(recvType)), - EngineWired: pkg.wiresCRUDEngine(recvType), + Operations: opsFor(id, ops), + ShortOperations: sortedSet(short), + EngineWired: pkg.wiresCRUDEngine(recvType), } } } @@ -125,12 +143,18 @@ func opsFor(serviceID string, scanned map[string]bool) []string { if declared, ok := pathRoutedOps[serviceID]; ok { return append([]string(nil), declared...) } - ops := make([]string, 0, len(scanned)) - for op := range scanned { - ops = append(ops, op) + return sortedSet(scanned) +} + +// sortedSet flattens a literal set into a stable slice, so a regeneration diff +// shows provider changes and nothing else. +func sortedSet(set map[string]bool) []string { + out := make([]string, 0, len(set)) + for k := range set { + out = append(out, k) } - sort.Strings(ops) - return ops + sort.Strings(out) + return out } // packageScan is one service package's providers, keyed by receiver type name. @@ -191,8 +215,11 @@ func scanServiceDir(dir string) (packageScan, error) { // provider switches in HandleRequest itself: sqs branches on protocol into // handleQueryRequest/handleJSONRequest, and kafka wraps handleRequest to // camelCase the response. -func (p packageScan) dispatchedOps(recvType string) map[string]bool { - ops := make(map[string]bool) +// It returns two sets: the literals that carry an operation's shape on their +// own, and the short ones only the model can confirm. +func (p packageScan) dispatchedOps(recvType string) (ops, short map[string]bool) { + ops = make(map[string]bool) + short = make(map[string]bool) visited := make(map[string]bool) var walk func(name string) @@ -203,13 +230,13 @@ func (p packageScan) dispatchedOps(recvType string) map[string]bool { } visited[name] = true - collectCaseLiterals(fn.Body, ops) + collectCaseLiterals(fn.Body, ops, short) for _, delegate := range delegateCalls(fn) { walk(delegate) } } walk("HandleRequest") - return ops + return ops, short } // wiresCRUDEngine reports whether a provider type opts into the generic CRUD @@ -252,7 +279,7 @@ func (p packageScan) wiresCRUDEngine(recvType string) bool { // Keyed is the point: `switch op { case "GetItem": }` dispatches, whereas a // tagless `switch { case remaining == 0: }` is control flow whose cases are // expressions, not operation names. -func collectCaseLiterals(body *ast.BlockStmt, ops map[string]bool) { +func collectCaseLiterals(body *ast.BlockStmt, ops, short map[string]bool) { ast.Inspect(body, func(n ast.Node) bool { sw, ok := n.(*ast.SwitchStmt) if !ok || sw.Tag == nil { @@ -264,8 +291,15 @@ func collectCaseLiterals(body *ast.BlockStmt, ops map[string]bool) { continue } for _, expr := range clause.List { - if lit, ok := stringLiteral(expr); ok && opNamePattern.MatchString(lit) { + lit, ok := stringLiteral(expr) + if !ok { + continue + } + switch { + case opNamePattern.MatchString(lit): ops[lit] = true + case shortOpNamePattern.MatchString(lit): + short[lit] = true } } } diff --git a/internal/codegen/scan_handverified_test.go b/internal/codegen/scan_handverified_test.go index bef430ba..45d50185 100644 --- a/internal/codegen/scan_handverified_test.go +++ b/internal/codegen/scan_handverified_test.go @@ -77,6 +77,27 @@ func TestScanProviders(t *testing.T) { } } +// TestScanProvidersCollectsShortLiteralsSeparately covers the names the shape +// test is too strict for. +// +// opNamePattern needs four characters. `Tag` has three, so resourcegroups — +// which implements Tag and Untag next to each other in one switch — had one of +// them scanned and the other silently dropped. They are collected here rather +// than admitted into Operations, because at this length the scan cannot tell an +// operation from the HTTP verbs a path resolver switches on. BuildFidelityData +// asks the model, which can. +func TestScanProvidersCollectsShortLiteralsSeparately(t *testing.T) { + scans, err := ScanProviders("../services") + require.NoError(t, err) + + assert.Contains(t, scans["resourcegroups"].ShortOperations, "Tag", + "Tag is dispatched by the provider and is three characters long") + assert.NotContains(t, scans["resourcegroups"].Operations, "Tag", + "the strict set keeps its shape test; promotion happens against the model") + assert.Contains(t, scans["resourcegroups"].Operations, "Untag", + "the four-character sibling was never in doubt") +} + // TestScanProvidersKeepsOperationsTheModelOmits is the regression that made the // scan drop real coverage: it used to intersect with the Smithy model, so every // operation a provider serves beyond its in-tree model vanished from the diff --git a/internal/gateway/protocol.go b/internal/gateway/protocol.go index 3336f788..679705e8 100644 --- a/internal/gateway/protocol.go +++ b/internal/gateway/protocol.go @@ -318,7 +318,18 @@ func resolveSharedSigningName(service string, match func(candidate string) bool) // signingNameOf finds the signing name whose group contains service. The groups // are small and few, and this runs once per request that reaches a shared name. +// +// A service may also arrive *as* a signing name. Most shared names are the +// parent service's own ID, so the group is found by membership — but four Lex +// services sign as "lex" and none of them is called that, so the alias stays +// contested and normalizeServiceID hands the unresolved name straight through. +// Recognising the group key is what lets those requests reach the split at all; +// the split then answers with the sibling that models the request, or refuses +// when two do. func signingNameOf(service string) string { + if _, ok := aliases.SigningSiblings[service]; ok { + return service + } for name, group := range aliases.SigningSiblings { for _, id := range group { if id == service { diff --git a/internal/gateway/protocol_test.go b/internal/gateway/protocol_test.go index f74b8255..6be8c2f1 100644 --- a/internal/gateway/protocol_test.go +++ b/internal/gateway/protocol_test.go @@ -8,6 +8,12 @@ import ( "testing" "github.com/stretchr/testify/assert" + + // The route tables the shared-signing-name split reads are registered by + // this package's init, which only runs for an importer. cmd/devcloud has + // its own blank import; without one here the tables are empty and every + // route-based routing decision silently answers "no candidate models it". + _ "github.com/skyoo2003/devcloud/internal/generated/crudregistry" ) func TestDetectProtocol_RESTXML(t *testing.T) { @@ -65,6 +71,55 @@ func TestDetectProtocol_S3Control(t *testing.T) { } } +// TestDetectProtocol_Lex covers the four services that share a signing name no +// service is called. All four boto3 Lex clients sign as "lex", and codegen +// leaves the alias contested rather than sending three services' traffic to the +// fourth — so every Lex call died as UnknownService, which docs/coverage.md +// published as an exclusion. +// +// The split needs no new routing: resolveSharedSigningName already picks the +// sibling whose route table models the request. It was never reached, because +// signingNameOf only recognised a *member* service ID, and "lex" is the group's +// key. +// +// The route tables are the real ones (crudregistry is blank-imported by this +// package's tests), so this asserts what a boto3 caller actually gets rather +// than what a hand-built table would. +func TestDetectProtocol_Lex(t *testing.T) { + cases := []struct{ name, method, path, want string }{ + // lex-models: GetBots. lexv2-models claims /bots too, but under POST and PUT. + {"models_v1_get_bots", "GET", "/bots", "lexmodelbuildingservice"}, + {"models_v1_get_intents", "GET", "/intents", "lexmodelbuildingservice"}, + {"models_v1_builtin_intents", "GET", "/builtins/intents", "lexmodelbuildingservice"}, + // lexv2-models: ListBots is POST /bots, CreateBot is PUT /bots. + {"models_v2_list_bots", "POST", "/bots", "lexmodelsv2"}, + {"models_v2_create_bot", "PUT", "/bots", "lexmodelsv2"}, + {"models_v2_describe_bot", "GET", "/bots/b1", "lexmodelsv2"}, + // lex-runtime addresses sessions under /bot/ singular. + {"runtime_v1_get_session", "GET", "/bot/b/alias/a/user/u/session", "lexruntimeservice"}, + {"runtime_v1_put_session", "POST", "/bot/b/alias/a/user/u/session", "lexruntimeservice"}, + // lexv2-runtime spells it botAliases; lexv2-models spells it botaliases. + {"runtime_v2_get_session", "GET", "/bots/b/botAliases/a/botLocales/l/sessions/s", "lexruntimev2"}, + {"runtime_v2_delete_session", "DELETE", "/bots/b/botAliases/a/botLocales/l/sessions/s", "lexruntimev2"}, + // The one genuine collision, asserted rather than left to be found: + // lex-models' DeleteBot is DELETE /bots/{name} and lexv2-models' is + // DELETE /bots/{botId}. Two siblings model it, so the request stays + // where it was — an honest UnknownService rather than a coin flip that + // deletes the wrong bot. + {"delete_bot_is_contested", "DELETE", "/bots/b1", "lex"}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + req := httptest.NewRequest(c.method, c.path, nil) + req.Header.Set("Authorization", + "AWS4-HMAC-SHA256 Credential=AKIA/20130524/us-east-1/lex/aws4_request, Signature=abc") + proto, service := DetectProtocol(req) + assert.Equal(t, "rest-json", proto) + assert.Equal(t, c.want, service) + }) + } +} + func TestDetectProtocol_JSON10_DynamoDB(t *testing.T) { req := httptest.NewRequest("POST", "/", strings.NewReader(`{}`)) req.Header.Set("X-Amz-Target", "DynamoDB_20120810.PutItem") diff --git a/internal/generated/compat/services.json b/internal/generated/compat/services.json index 37427412..fb5831c9 100644 --- a/internal/generated/compat/services.json +++ b/internal/generated/compat/services.json @@ -8125,6 +8125,7 @@ "PutGroupConfiguration", "SearchResources", "StartTagSyncTask", + "Tag", "UngroupResources", "Untag", "UpdateAccountSettings", diff --git a/internal/generated/fidelity/manifest_gen.go b/internal/generated/fidelity/manifest_gen.go index 53a1edd1..86672349 100644 --- a/internal/generated/fidelity/manifest_gen.go +++ b/internal/generated/fidelity/manifest_gen.go @@ -10081,7 +10081,7 @@ var Services = map[string]Service{ "PutGroupConfiguration": TierHandVerified, "SearchResources": TierHandVerified, "StartTagSyncTask": TierHandVerified, - "Tag": TierUnimplemented, + "Tag": TierHandVerified, "UngroupResources": TierHandVerified, "Untag": TierHandVerified, "UpdateAccountSettings": TierHandVerified, diff --git a/internal/services/appsync/provider.go b/internal/services/appsync/provider.go index d5b3114f..6195e43b 100644 --- a/internal/services/appsync/provider.go +++ b/internal/services/appsync/provider.go @@ -15,6 +15,7 @@ import ( "github.com/skyoo2003/devcloud/internal/plugin" "github.com/skyoo2003/devcloud/internal/shared" + "github.com/skyoo2003/devcloud/internal/shared/crud" ) // Provider implements the DeepdishControlPlaneService (AppSync) service. @@ -62,6 +63,14 @@ func (p *Provider) HandleRequest(_ context.Context, op string, req *http.Request if op == "" { op = resolveOp(req.Method, path) } + // resolveOp is hand-written and knows the paths somebody thought to add. The + // model's own route table knows all of them, so a path it does not recognise + // is recovered rather than lost: ListApis is GET /v2/apis and resolveOp only + // strips /v1, which left the case clause below unreachable while the + // fidelity manifest called it hand-verified. + if op == "" { + op = crud.Route("appsync", req.Method, req.URL.RequestURI()) + } switch op { // GraphqlApi diff --git a/internal/services/eks/provider.go b/internal/services/eks/provider.go index 6688b85a..0f00a7b6 100644 --- a/internal/services/eks/provider.go +++ b/internal/services/eks/provider.go @@ -14,6 +14,7 @@ import ( "github.com/skyoo2003/devcloud/internal/plugin" "github.com/skyoo2003/devcloud/internal/shared" + "github.com/skyoo2003/devcloud/internal/shared/crud" "github.com/skyoo2003/devcloud/internal/storage/sqlite" ) @@ -62,6 +63,14 @@ func (p *Provider) HandleRequest(_ context.Context, op string, req *http.Request if op == "" { op = resolveOp(req.Method, path) } + // resolveOp is hand-written and knows the paths somebody thought to add. The + // model's own route table knows all of them, so a path it does not recognise + // is recovered rather than lost: ListAccessPolicies is GET /access-policies + // and resolveOp has no case for it, which left the case clause below + // unreachable while the fidelity manifest called it hand-verified. + if op == "" { + op = crud.Route("eks", req.Method, req.URL.RequestURI()) + } switch op { // ── Cluster ────────────────────────────────────────────────────────────── diff --git a/internal/services/opensearch/provider.go b/internal/services/opensearch/provider.go index f9fda236..cb57dd52 100644 --- a/internal/services/opensearch/provider.go +++ b/internal/services/opensearch/provider.go @@ -14,6 +14,7 @@ import ( "github.com/skyoo2003/devcloud/internal/plugin" "github.com/skyoo2003/devcloud/internal/shared" + "github.com/skyoo2003/devcloud/internal/shared/crud" ) // Provider implements the OpenSearchService service. @@ -46,6 +47,15 @@ func (p *Provider) HandleRequest(_ context.Context, op string, req *http.Request if op == "" { op = resolveOp(req.Method, req.URL.Path) } + // resolveOp is hand-written and knows the paths somebody thought to add. The + // model's own route table knows all of them, so a path it does not recognise + // is recovered rather than lost: ListApplications is + // GET /2021-01-01/opensearch/list-applications and resolveOp knows only + // GET /application, which left the case clause below unreachable while the + // fidelity manifest called it hand-verified. + if op == "" { + op = crud.Route("opensearch", req.Method, req.URL.RequestURI()) + } body, err := io.ReadAll(req.Body) if err != nil { diff --git a/internal/services/s3/provider.go b/internal/services/s3/provider.go index f14818c6..50af336f 100644 --- a/internal/services/s3/provider.go +++ b/internal/services/s3/provider.go @@ -121,6 +121,23 @@ func (p *S3Provider) HandleRequest(ctx context.Context, _ string, req *http.Requ if _, ok := q["notification"]; ok { return p.getBucketNotification(ctx, bucket) } + // Everything above named a sub-resource this provider serves. What + // is left is either a bucket listing or a sub-resource nothing here + // implements, and falling through treated both as a listing — so + // GET /{bucket}?analytics answered 200 with and + // botocore read it as a successful + // ListBucketAnalyticsConfigurations. A fabricated success, and the + // same fall-through covered ?inventory, ?metrics, ?replication, + // ?lifecycle, ?encryption, ?versions and ?object-lock. + // + // The check is on the parameters a listing actually carries rather + // than on a list of sub-resource names, so a sub-resource AWS adds + // tomorrow declines on its own instead of being served as a listing + // until somebody notices. + if sub, unhandled := unhandledSubresource(q); unhandled { + return xmlError("NotImplemented", + "the "+sub+" sub-resource is not implemented", http.StatusNotImplemented), nil + } if q.Get("list-type") == "2" { return p.listObjectsV2(ctx, bucket, q) } @@ -413,6 +430,49 @@ type locationConstraintXML struct { // splitBucketKey splits a URL path like /bucket/key/parts into (bucket, key). // The leading slash is stripped before splitting. +// listObjectsParams are the query parameters a bucket listing carries. Two +// operations share the endpoint — ListObjects and ListObjectsV2 — and between +// them this is the whole set. +// +// An allow-list rather than a deny-list of sub-resource names, because the two +// fail in opposite directions. A missing entry here declines a legitimate +// listing parameter, which a test catches immediately; a missing entry in a +// deny-list serves a sub-resource as a listing, which is silent and is exactly +// the defect being fixed. +var listObjectsParams = map[string]bool{ + "prefix": true, "delimiter": true, "max-keys": true, "encoding-type": true, + "marker": true, // ListObjects + "list-type": true, // ListObjectsV2 + "start-after": true, "continuation-token": true, "fetch-owner": true, + "expected-bucket-owner": true, "optional-object-attributes": true, + // Not a parameter of either listing, but every SDK appends it to a + // presigned URL, and dropping the request on it would break every + // presigned listing. + "x-id": true, +} + +// unhandledSubresource reports the first query parameter that is neither a +// listing parameter nor a sub-resource this provider serves, which makes the +// request an operation S3 models and DevCloud does not implement. +// +// Sub-resources the provider does serve never reach here: their branches return +// before this is called. +func unhandledSubresource(q url.Values) (string, bool) { + names := make([]string, 0, len(q)) + for name := range q { + if !listObjectsParams[name] { + names = append(names, name) + } + } + if len(names) == 0 { + return "", false + } + // Sorted so the same request always names the same sub-resource, whatever + // order Go walked the map in. + sort.Strings(names) + return names[0], true +} + func splitBucketKey(path string) (bucket, key string) { path = strings.TrimPrefix(path, "/") if path == "" { diff --git a/internal/services/s3/provider_test.go b/internal/services/s3/provider_test.go index d1662c4c..a065172d 100644 --- a/internal/services/s3/provider_test.go +++ b/internal/services/s3/provider_test.go @@ -484,3 +484,59 @@ func TestS3Provider_BucketNotification(t *testing.T) { assert.Equal(t, 200, resp.StatusCode) assert.Contains(t, string(resp.Body), "MyTopic") } + +// TestS3Provider_UnhandledBucketSubresourceDeclines covers the fall-through that +// answered a sub-resource nobody implemented with a bucket listing. +// +// GET /{bucket}?analytics misses every sub-resource check and lands on +// listObjects, which returns 200 and . botocore reads that as +// a successful ListBucketAnalyticsConfigurations — a fabricated success for an +// operation this provider does not serve, and the one thing docs/coverage.md +// calls absolute. The same fall-through served ?inventory, ?metrics, +// ?replication, ?lifecycle, ?encryption and ?versions. +// +// The real listings are asserted alongside, because the guard is only correct if +// it declines nothing a caller legitimately sends. +func TestS3Provider_UnhandledBucketSubresourceDeclines(t *testing.T) { + p := newTestProvider(t) + defer func() { _ = p.Shutdown(context.Background()) }() + + _, err := p.HandleRequest(context.Background(), "", httptest.NewRequest("PUT", "/sub-bucket", nil)) + require.NoError(t, err) + + unhandled := []string{ + "analytics", "inventory", "metrics", "replication", + "lifecycle", "encryption", "versions", "object-lock", + } + for _, sub := range unhandled { + t.Run(sub, func(t *testing.T) { + req := httptest.NewRequest("GET", "/sub-bucket?"+sub, nil) + resp, err := p.HandleRequest(context.Background(), "", req) + require.NoError(t, err) + assert.NotContains(t, string(resp.Body), "ListBucketResult", + "an unimplemented sub-resource was answered with a bucket listing") + assert.GreaterOrEqual(t, resp.StatusCode, 400, + "an unimplemented sub-resource must decline, not succeed") + }) + } + + // Every shape of a real listing still has to work. These are the query + // parameters ListObjects and ListObjectsV2 actually carry. + listings := []string{ + "/sub-bucket", + "/sub-bucket?prefix=a/", + "/sub-bucket?prefix=a/&delimiter=/", + "/sub-bucket?list-type=2", + "/sub-bucket?list-type=2&max-keys=10&fetch-owner=true", + "/sub-bucket?list-type=2&continuation-token=abc&start-after=k", + "/sub-bucket?marker=k&max-keys=5&encoding-type=url", + } + for _, uri := range listings { + t.Run("listing "+uri, func(t *testing.T) { + resp, err := p.HandleRequest(context.Background(), "", httptest.NewRequest("GET", uri, nil)) + require.NoError(t, err) + assert.Equal(t, 200, resp.StatusCode) + assert.Contains(t, string(resp.Body), "ListBucketResult") + }) + } +} diff --git a/internal/shared/crud/crud.go b/internal/shared/crud/crud.go index beaca3f3..82b1bcd1 100644 --- a/internal/shared/crud/crud.go +++ b/internal/shared/crud/crud.go @@ -105,16 +105,34 @@ func Register(service string, ops map[string]OpMeta) { routes[service] = rs } +// Route returns the operation a service models at this method and URI, or "" if +// it models none. +// +// The table is the service's own model, so this is the answer a hand-written +// path resolver inside a provider is trying to reproduce. A provider whose +// resolver does not recognise a path can end with this instead of "": the +// operation is recovered from the model rather than lost, and a `case` clause +// the provider already has stops being unreachable. +// +// It answers "" rather than an error for a service the engine holds no routes +// for. Registration happens in the generated crudregistry's init, which only +// runs for an importer — a provider unit test that does not import it sees an +// empty table and falls back on the provider's own resolver, which is the +// behaviour it had before. +func Route(service, method, uri string) string { + mu.RLock() + rs := routes[service] + mu.RUnlock() + op, _ := httproute.Match(rs, method, uri) + return op +} + // HasRoute reports whether a service models a REST operation at this method and // URI. It answers "is this request one this service could serve?", which is how // the gateway tells apart services that share a SigV4 signing name and so cannot // be separated by the credential scope alone. func HasRoute(service, method, uri string) bool { - mu.RLock() - rs := routes[service] - mu.RUnlock() - op, _ := httproute.Match(rs, method, uri) - return op != "" + return Route(service, method, uri) != "" } // RegisteredOps returns the operation metadata registered for a service. diff --git a/tests/compatibility/_coverage.py b/tests/compatibility/_coverage.py index 8e3dc563..7a025406 100644 --- a/tests/compatibility/_coverage.py +++ b/tests/compatibility/_coverage.py @@ -63,27 +63,17 @@ # Registered, counted as serving operations, and reachable by no boto3 caller. # -# All four Lex clients sign as "lex" and none of the four services is called -# "lex", so codegen.BuildAliases leaves the name contested and unrouted — by -# design, because guessing would send one service's traffic to another. The -# consequence docs/coverage.md did not state until this was measured: the -# "reached by its own unambiguous name" escape does not exist for a boto3 -# caller, because boto3 signs with the contested name and nothing else. +# Empty, and kept as the name of a category rather than deleted: a service falls +# into it again the next time two services share a name nobody is called. # -# Splitting them on the URL is what resolved opensearch/elasticsearchservice and -# apigateway v1/v2, and it does not work here: /bots is the first segment for -# lex-model-building-service, lex-models-v2 AND lex-runtime-v2. A correct split -# needs full path matching across four services, which is routing work rather -# than coverage work. -# -# Pinned by test_lex_services_are_unreachable_from_boto3 so that fixing the -# routing fails this test and forces the published figure up with it. -UNREACHABLE_FROM_BOTO3 = { - "lexmodelbuildingservice": "signs as the contested alias 'lex'", - "lexmodelsv2": "signs as the contested alias 'lex'", - "lexruntimeservice": "signs as the contested alias 'lex'", - "lexruntimev2": "signs as the contested alias 'lex'", -} +# The four Lex services were the whole membership. They still sign as "lex", no +# service is still called "lex", and the alias is still contested and unrouted — +# what changed is that the shared-signing-name split now recognises "lex" as a +# group *key*, not only as a member service ID, so each request is answered by +# the sibling whose route table models its method and path. The one operation +# two siblings both model (DELETE /bots/{id}) is still refused rather than +# guessed at. +UNREACHABLE_FROM_BOTO3: dict[str, str] = {} # A resource that does not exist in an empty store is a served answer: the engine # looked in the store and reported honestly. diff --git a/tests/compatibility/test_handverified_is_reachable.py b/tests/compatibility/test_handverified_is_reachable.py new file mode 100644 index 00000000..3d949bf9 --- /dev/null +++ b/tests/compatibility/test_handverified_is_reachable.py @@ -0,0 +1,78 @@ +"""An operation the manifest calls hand-verified must be reachable. + +`hand-verified` is the strongest label DevCloud publishes: someone wrote code for +this operation. Codegen derives it by reading the `case` clauses in a provider's +dispatch switch — which proves the code exists, not that any request arrives at +it. + +For a REST-JSON provider those are two different questions. The gateway passes no +operation name (there is no `X-Amz-Target` to read), so the provider recovers it +from the method and path with a hand-written resolver. Where that resolver does +not know a path, the operation is unreachable however carefully it was +implemented, and the manifest says `hand-verified` about code nothing can call. + +Three were found this way by Milestone 6's per-service floor test, which had +tolerated them because it needs only *one* served operation per service to pass. +They are asserted individually here, because that is the only granularity at +which the defect is visible. +""" + +import pytest +from botocore import xform_name +from botocore.exceptions import ClientError + +import _coverage + +MANIFEST = _coverage.load_manifest() + +# service id, boto3 client name, operation, and the route boto3 actually sends. +# The route is quoted from the service's own model (the generated CRUD registry +# holds the same URI), because it is what the provider's resolver has to know. +CASES = [ + ("appsync", "appsync", "ListApis", "GET /v2/apis"), + ("eks", "eks", "ListAccessPolicies", "GET /access-policies"), + ( + "opensearch", + "opensearch", + "ListApplications", + "GET /2021-01-01/opensearch/list-applications", + ), +] + + +@pytest.mark.parametrize( + "service,client_name,operation,route", + CASES, + ids=[f"{s}.{op}" for s, _, op, _ in CASES], +) +def test_handverified_operation_is_reachable( + service_client, service, client_name, operation, route +): + """The operation answers, rather than reporting itself unimplemented. + + What is asserted is deliberately weak: any answer other than an error. The + manifest claims the operation is implemented, so the failure being caught is + the provider not finding its own implementation — not the shape of what it + returns. + """ + assert operation in MANIFEST[service]["servedOps"], ( + f"{service}.{operation} is no longer listed as served, so this test " + "asserts a claim the manifest is not making. Delete the case, or fix " + "whatever stopped the manifest from making it." + ) + + client = service_client(client_name) + params = _coverage.stub_params(client_name, operation) + + try: + response = getattr(client, xform_name(operation))(**params) + except ClientError as exc: + code = exc.response.get("Error", {}).get("Code") + pytest.fail( + f"{service}.{operation} ({route}) answered {code}. The manifest calls " + "it hand-verified, so a provider case clause implements it — but the " + "provider's own path resolver never reaches that clause, so the " + "operation arrives empty and the dispatch falls through to default." + ) + + assert response["ResponseMetadata"]["HTTPStatusCode"] == 200 diff --git a/tests/compatibility/test_no_fabricated_success.py b/tests/compatibility/test_no_fabricated_success.py index 9c659c90..786a4e22 100644 --- a/tests/compatibility/test_no_fabricated_success.py +++ b/tests/compatibility/test_no_fabricated_success.py @@ -54,34 +54,28 @@ def _unserved_probe(service_id, entry): return name, None -# Two fabricated successes that this file's fix does not reach, marked strict so -# they fail the moment they start passing rather than rotting into an accepted -# state. Only what was observed is recorded — the mechanism behind each is not -# the provider `default` branch that was fixed here, and has not been identified: +# Fabricated successes this file's own fix does not reach, marked strict so they +# fail the moment they start passing rather than rotting into an accepted state. # -# s3.ListBucketAnalyticsConfigurations -# GET /{Bucket}?analytics answers 200 with an empty body. S3's provider -# default returns MethodNotAllowed, so this is not that branch. +# Empty. Both entries it carried are fixed, and each turned out to be a different +# defect from the one the entry guessed at: # -# resourcegroups.Tag -# PUT /resources/{Arn}/tags answers 200 echoing the request's Arn and -# Tags. The manifest lists the operation as unimplemented and the CRUD -# registry does not hold it; it holds GetTags at the same path under GET, -# and httproute.Match does compare methods (match.go:42), so the obvious -# explanation is ruled out rather than confirmed. +# s3.ListBucketAnalyticsConfigurations was the bucket-listing fall-through. +# GET /{Bucket}?analytics matched no sub-resource branch and landed on +# listObjects, so an unimplemented sub-resource was answered with +# . Seven others took the same path. The provider now +# declines any bucket-level query parameter that is neither a sub-resource it +# serves nor a parameter a listing carries. # -# Both are real violations of the guarantee and neither is fixed here: they are -# different defects that deserve their own diagnosis and their own change. -KNOWN_UNFIXED = { - ( - "s3", - "ListBucketAnalyticsConfigurations", - ): "empty 200 from S3's sub-resource routing", - ( - "resourcegroups", - "Tag", - ): "engine-shaped echo for an operation the manifest calls unimplemented", -} +# resourcegroups.Tag was never a fabricated success. The scan that decides +# which operations are hand-verified required four characters, so `Untag` was +# recorded and `Tag` — implemented beside it in the same switch — was dropped. +# The 200 was real code answering; the manifest was wrong, this probe believed +# the manifest, and the report inherited the error. +# +# Both were recorded as observations with the mechanism explicitly unidentified, +# which is why neither had to be un-guessed before it could be fixed. +KNOWN_UNFIXED: dict[tuple[str, str], str] = {} # Every service that is addressable, routable, and has an unserved operation to # ask for. Built at collection time so the parametrisation names the operation diff --git a/tests/compatibility/test_service_smoke.py b/tests/compatibility/test_service_smoke.py index a7cb14d1..248c69e9 100644 --- a/tests/compatibility/test_service_smoke.py +++ b/tests/compatibility/test_service_smoke.py @@ -104,37 +104,24 @@ def test_services_without_a_boto3_client_are_exactly_the_known_two(): ) -@pytest.mark.parametrize("service", sorted(_coverage.UNREACHABLE_FROM_BOTO3)) -def test_lex_services_are_unreachable_from_boto3(service_client, service): - """The four Lex services are registered, counted as served, and unroutable. - - All four boto3 Lex clients sign as "lex", no service is named "lex", so the - alias stays contested and unrouted rather than guessed. docs/coverage.md - said each Lex service is reachable "by its own unambiguous name"; a boto3 - caller has no such name to use, which is what this pins. - - It asserts UnknownService deliberately. Making Lex routable is routing work, - not coverage work, and when it lands this test fails — which is the signal to - move these four into the parametrization above and raise the published - compatibility-tested figure with them. +def test_no_registered_service_is_unreachable_from_boto3(): + """The exclusion category stays empty, or the published figure moves with it. + + This replaces test_lex_services_are_unreachable_from_boto3, which pinned the + four Lex services as unroutable and told whoever fixed the routing to delete + it. The routing was fixed — "lex" is now recognised as a signing-name group + key, so each Lex request reaches the sibling that models its path — and the + four joined the floor parametrization below. + + What survives is the direction of the check rather than the pin: a service + that becomes unreachable has to be named in UNREACHABLE_FROM_BOTO3, and + naming it lowers the compatibility-tested figure gated above. """ - entry = MANIFEST[service] - name = _coverage.boto3_name(service) - client = service_client(name) - - candidates = _coverage.probe_operations(name, entry["servedOps"]) - assert candidates, f"{service} has no served operation to probe" - operation, needs_input = candidates[0] - params = _coverage.stub_params(name, operation) if needs_input else {} - - with pytest.raises(ClientError) as excinfo: - _call(client, operation, params) - - code = excinfo.value.response.get("Error", {}).get("Code") - assert code == "UnknownService", ( - f"{service} ({name}.{operation}) answered {code}, not UnknownService. " - "If Lex routing was fixed, move these services into " - "test_registered_service_meets_the_floor and update docs/coverage.md." + assert _coverage.UNREACHABLE_FROM_BOTO3 == {}, ( + "a registered service is unreachable from boto3: " + f"{sorted(_coverage.UNREACHABLE_FROM_BOTO3)}. It drops out of the " + "compatibility-tested figure in docs/coverage.md, which is gated by " + "test_published_compatibility_tested_figure_matches_this_suite." )