Skip to content

Commit 1f0884a

Browse files
sean-Jakob3xD
andauthored
Backport transport correctness fixes to v4 (#859) (#869)
* fix(opensearchtransport): honor per-request header override over global headers setReqGlobalHeader compared the existing header *value* against the header *name* (req.Header.Get(k) != k), which is effectively always true, so global headers were appended even when the caller had already set the same key on the request. Compare for presence instead so a per-request header suppresses the matching global default. Signed-off-by: Sean Chittenden <sean.chittenden@crowdstrike.com> * fix(opensearchtransport): prevent gzip buffer-pool nil poisoning on compress error When io.Copy or writer.Close failed, compress() returned (nil, err) but Perform had already armed `defer collectBuffer(buf)` with that nil, which Put a typed-nil *bytes.Buffer into the sync.Pool. The next compress() call would Get() the nil and panic on buf.Reset(). Return the buffer on the error path so the deferred collectBuffer recycles it, and nil-guard collectBuffer for belt-and-suspenders. Signed-off-by: Sean Chittenden <sean.chittenden@crowdstrike.com> * fix(opensearchtransport): surface body-read errors during response buffering The post-RoundTrip buffering block in Perform declared a fresh `err` via :=, shadowing the outer return error. On a truncated body or context cancellation mid-read, the read error was silently dropped and res.Body was left as the (now closed) original, so callers received a 2xx Response with an unreadable body and err == nil. Always replace res.Body with the bytes that were read (so callers see a partial body rather than a closed reader), and propagate the read error when no earlier error is pending. Adjust Client.Do to handle the new (resp != nil, err != nil) case from Perform: branch on resp == nil for the hard-failure path and wrap the error in ErrReadBody when a response is available, preserving the documented contract that TestClientInterfe asserts. Signed-off-by: Sean Chittenden <sean.chittenden@crowdstrike.com> * fix: close error-response body in ParseError and v5preview do() With Config.DisableResponseBuffering=true, Perform returns the live http.Response.Body. ParseError() drained it via io.ReadAll but never called Close(), so the connection was not returned to http.Transport's idle pool until GC ran the finalizer. Under sustained 4xx/5xx load this exhausts FDs. Close the body in ParseError after reading (a no-op NopCloser in the buffered case), and drain+close in the v5preview do() branch where dataPointer == nil and ParseError is bypassed entirely. The legacy opensearchapi and plugins/{ism,security} do() helpers already route the non-nil-body case through ParseError, so they're covered by the same change. Signed-off-by: Sean Chittenden <sean.chittenden@crowdstrike.com> * fix(transport): classify Perform body-read errors via sentinel, keep v5preview error body readable Address review feedback on #859: - Add opensearchtransport.ErrResponseBodyRead sentinel; Perform wraps response-buffering read failures with it. Client.Do now classifies via errors.Is so an unrelated transport error returned alongside a response (e.g. context cancellation during retry backoff after a retryable status) is no longer mislabeled ErrReadBody. errors.Is(err, context.Canceled) still holds, but the misleading "failed to read body" prefix is gone. - v5preview do() no-decode error path reads the body to EOF then re-wraps it in a NopCloser instead of discarding it, keeping resp.Body readable and consistent with the ParseError path while still freeing the connection under DisableResponseBuffering. - Document the (resp != nil, err != nil) invariant on the Interface contract and Perform godoc: callers must treat resp == nil, not err != nil, as the signal for a hard transport failure. - Scope the CHANGELOG body-close claim to non-buffered mode (in the default buffered mode Perform already drains and closes the body). - Add unit tests for the gzip nil-poisoning fix, header-override suppression (asserting len == 1, which the prior value-only test could not detect), body-read error surfacing, the Do classification split, and the v5preview body-readability regression. Signed-off-by: Sean Chittenden <sean.chittenden@crowdstrike.com> * chore(osgen): silence gochecknoglobals on decodeEquivalentGroups lookup table Pre-existing lint warning introduced in f95f305 (#844); the table is a static codegen lookup intentionally kept package-level next to its doc comment and the funcs that consult it. Signed-off-by: Sean Chittenden <sean.chittenden@crowdstrike.com> * fix(transport,signer): drain bodies on raw RoundTrip paths and close request body on signer read error Follow-up to the review on #859, addressing the pre-existing instances of the same "close/drain the body" bug class that Jakob's audit surfaced. These are on direct RoundTrip paths that lack Perform's response-buffering safety net, so closing a partially-read body genuinely defeats HTTP keep-alive. - cluster_health.go stats poller and discovery.go's /_cat/shards, /_cluster/state/metadata, and /_nodes paths now drain to EOF before close via a deferred drain-then-close. This covers both the non-200 early returns and the json.Decode success paths, which stop at the end of the JSON value without consuming trailing bytes. The drain/close is inlined at each defer (rather than extracted to a helper) so bodyclose can verify the close statically. - opensearch.Response.String() is now non-consuming: it restores Body with an in-memory reader after rendering, so logging a response no longer empties a body other code expects to read. Receiver changed to a pointer so the restore is visible to callers (Response is always used as *Response). - The AWS v1 and v2 signers now close the request body on the read-error path in hexEncodedSha256OfRequest. - Add tests: Response.String non-consuming, and both signers closing the request body when the read fails. Signed-off-by: Sean Chittenden <sean.chittenden@crowdstrike.com> * refactor(transport): split Perform into Stream + buffering wrapper; drop DisableResponseBuffering Add (*Client).Stream(req) holding the full request/retry/RoundTrip loop with seed fallback, returning the raw RoundTrip body. The caller owns reading and closing res.Body; the (res != nil, err != nil) invariant and the req.URL.Host rewrite side effect are preserved. (*Client).Perform becomes a thin buffering wrapper over Stream: io.ReadAll + bytes.Reader + NopCloser, with body-read failures surfaced via ErrResponseBodyRead. Marked Deprecated for removal in v5. Add (*opensearch.Client).Stream as a passthrough so raw consumers do not need to type-assert c.Transport. The transport's Stream is exposed via the new opensearch.Streamer interface; callers reach it through ErrTransportMissingMethodStream when the configured transport does not satisfy it. Mark (*opensearch.Client).Perform as deprecated. Remove the untagged DisableResponseBuffering field from opensearch.Config and opensearchtransport.Config, the disableResponseBuffering struct field, and the constructor assignment. Perform now always buffers; Stream never does. Add a TODO on opensearchtransport.Interface noting v5 should add Stream and remove Perform. Replace the two DisableResponseBuffering test sites with table-driven TestPerformStreamBuffering and TestStreamNilBody covering both entry points, body lifecycle, and the req.URL.Host rewrite. Preserve TestPerformSurfacesBodyReadError and TestDoPerformErrorClassification. Migrate opensearchutil/bulk_indexer_integration_test.go's raw Perform polling loop to the typed client.Cluster.Health call so the new Perform deprecation does not trigger staticcheck SA1019. Signed-off-by: Sean Chittenden <sean.chittenden@crowdstrike.com> * build(lint): run golangci-lint across all build-tag combinations golangci-lint compiles one point in the build-tag space per run, so a single invocation can never lint every file. Two mutually-exclusive boolean axes partition the tree: - integration vs !integration: every unit *_test.go is `!integration`; every integration test is `integration`. Setting `integration` drops all unit test files from the type-check; omitting it drops all integration ones. - multinode vs !multinode (within integration): the single-node integration files are `!multinode`; the multinode ones require `multinode`. make lint.local and the CI workflow previously passed a single tag set that included `integration` and `multinode`, so they silently skipped every unit test file AND every single-node integration file -- those were never linted. Feature tags (core, plugins, plugin_security, plugin_index_management) are pure-OR alternatives that union harmlessly into every run, so complete coverage needs one run per (integration, multinode) combination. Introduce GOLANGCI_LINT_TAG_SETS enumerating the three runs, loop over it in lint.local and the Docker `linters` target, and add a matrix to the CI workflow. Signed-off-by: Sean Chittenden <sean.chittenden@crowdstrike.com> * test(lint): resolve issues surfaced by linting all build-tag combinations Running golangci-lint across every (integration, multinode) build-tag combination (see the preceding build(lint) commit) compiles source files that the previous single-tag-set invocation never type-checked -- every !integration unit test and every !multinode single-node integration test. That exposed pre-existing lint findings in files that had never been linted. This commit clears them; it is a test/quality cleanup with no runtime behavior change. - testifylint: replace assert.* error assertions with require.* (require-error) across opensearchapi, opensearchtransport, opensearchutil, plugins, signer, and root tests; switch require.IsType on error values to ErrorAs; convert exact float comparisons to require.InDelta (float-compare). - staticcheck: pass t.Context() instead of a nil context in opensearch_integration_test.go (SA1012); drop a dead append (SA4010). - gci: fix the import grouping in opensearch_integration_test.go. - thelper: add t.Helper() to table-driven check closures in error_test.go and api_indices_response_test.go. - tparallel: call t.Parallel() in the TestConnectionPoolPromotion subtests. - gosec: validate PPROF_ADDR and stop echoing the env value into a log call in connection_benchmark_test.go (G706). - gocritic: avoid append-to-different-slice aliasing in discovery_internal_test.go. - unparam: drop the always-nil roles arg path and unused name param in test helpers, and assert WriteTo's byte count so its result is used; add a RolePolicy test that exercises a dead, role-bearing connection. - goconst: replace repeated action/policy-name string literals with constants in opensearchutil/bulk_indexer.go and opensearchtransport policy files. - lll: wrap over-long JSON fixtures and string literals. No behavior change; test-only. Signed-off-by: Sean Chittenden <sean.chittenden@crowdstrike.com> * refactor(osgen): emit net/http method constants in generated Req tests The Req-test fragment template hard-coded the HTTP method as a quoted string literal (e.g. "POST"), so every generated *_gen_test.go pinned the wantMethod field to a magic string. The dispatch and plugin fragments already route methods through HTTPMethodConst to produce http.MethodPost et al.; the test fragment was the lone holdout. Wire the existing HTTPMethodConst helper into the reqTest template via a methodExpr func and add net/http to the fragment's import set so the emitted test files reference http.MethodPost instead of "POST". Unknown methods still fall through to a quoted literal, preserving today's behavior for non-standard verbs. Signed-off-by: Sean Chittenden <sean.chittenden@crowdstrike.com> * chore(v5preview): regenerate Req tests to use net/http method constants Regenerated output of the osgen test fragment change: every wantMethod literal in v5preview/opensearchapi/*_gen_test.go switches from a quoted string ("GET", "POST", ...) to the corresponding net/http constant (http.MethodGet, http.MethodPost, ...), and net/http is added to each file's import set. No behavioral change — the emitted constants resolve to the same strings at runtime; this commit is purely the result of re-running osgen after the template update. Signed-off-by: Sean Chittenden <sean.chittenden@crowdstrike.com> * remove unused file Signed-off-by: Sean Chittenden <sean.chittenden@crowdstrike.com> * fix(osgen): make errwrap catalogs functions to drop mutable globals errwrap.Wrappers and errwrap.OperationWrappers were exported mutable package vars carrying //nolint:gochecknoglobals directives. Convert both to functions that return a fresh slice/map. This removes the gochecknoglobals findings honestly (no suppression), and makes the read-only catalogs immutable from a caller's perspective -- callers can no longer mutate the shared backing slice/map. The only consumers are in-package (For, sortedCanonical); update them to call the functions. Signed-off-by: Sean Chittenden <sean.chittenden@crowdstrike.com> * chore(osgen): drop dead body-drain in plugin do() no-decode path opensearch.Do routes through the buffered (*opensearchtransport.Client).Perform, so resp.Body in the plugin do[T] helpers is already an io.NopCloser over a bytes.Reader -- the connection has been drained and returned to the pool. The no-decode error-path drain added in PR #859 was only meaningful when DisableResponseBuffering=true, which has been removed. Strip the drain from the cmd/osgen plugin client template and the four hand-written copies (opensearchapi, v5preview/opensearchapi, plugins/security, plugins/ism). The helper now reads: if resp.IsError() { if dataPointer != nil { return resp, opensearch.ParseError(resp) } return resp, fmt.Errorf("status: %s", resp.Status()) } with a doc comment noting resp.Body has already been buffered and closed by Perform. Signed-off-by: Sean Chittenden <sean.chittenden@crowdstrike.com> * chore(v5preview): regenerate plugin client_gen.go Output of `make regen` after the cmd/osgen template change in the preceding commit. No hand edits. Signed-off-by: Sean Chittenden <sean.chittenden@crowdstrike.com> * docs(buffering): rewrite guide for Do[T] vs Stream Rewrite guides/response_buffering.md around the two entry points: opensearch.Do[T] (typed, buffered, default; SDK owns the body) versus opensearchtransport.Client.Stream (raw, unbuffered, caller owns the body). Document the reason there is intentionally no typed streaming helper, and update the proxy example to use client.Stream(req). CHANGELOG: replace the DisableResponseBuffering "Added" entry with the Stream/Client.Stream "Added" entry; add a Deprecated entry for Perform on both opensearchtransport.Client and opensearch.Client (removal in v5); drop the stale DisableResponseBuffering reference from the #859 Fixed entry. Signed-off-by: Sean Chittenden <sean.chittenden@crowdstrike.com> * test(benchmarks): fix goroutine leak and stateful body in transport benches Three correctness/quality fixes on the transport-adjacent benchmarks surfaced while running locally with PGO collection. No production code changes. opensearch_benchmark_test.go (BenchmarkClient/Create client with defaults): Each iteration constructed a fresh opensearch.NewClient, which spawns the transport's per-client cluster-health and node-stats goroutines. Nothing closed them, so goroutines leaked linearly with b.N and at high iteration counts the Go runtime starved the bench loop itself. Close the underlying *opensearchtransport.Client in each iteration so the background goroutines exit. opensearchtransport/opensearchtransport_benchmark_test.go (BenchmarkTransport): - The pre-existing FakeTransport stored a single *http.Response with a strings.Reader body and returned the same pointer from every RoundTrip. strings.Reader is stateful: after the first Perform drained it the next iteration saw EOF, so the bench was measuring the EOF-handling path rather than steady-state Perform. Build a fresh response (with a fresh body) per RoundTrip. - Hoist opensearchtransport.New out of the per-iteration loop. Real callers build one transport per process; constructing one per iteration both inflates the measurement and (on this branch) leaks health-check goroutines. Disable the load-shedding poller via NodeStatsInterval = -1 so its tick rate doesn't bleed into the measurement. opensearchtransport/logger_benchmark_test.go (BenchmarkTransportLogger): Same construction-per-iteration anti-pattern across all four Text/Text-Body/JSON/JSON-Body sub-benches. Collapse the four copy-pasted bodies into a single closure, hoist New out of the loop, add b.Cleanup to close the transport, and disable the load-shedding poller. The Text-Body case had a separate bug: it called res.Body.Close() before io.ReadAll(res.Body), so the read always returned 0 bytes against a closed body and the len < 13 branch was silently flagged. Read first, then close. Drive-by: switch error format verbs from %s to %q in the touched Fatalf/Errorf sites for safer rendering of error chains. Signed-off-by: Sean Chittenden <sean.chittenden@crowdstrike.com> * feat(github/workflows): pin remaining actions to SHA (#882) Signed-off-by: Sean Chittenden <sean.chittenden@crowdstrike.com> --------- Signed-off-by: Sean Chittenden <sean.chittenden@crowdstrike.com> Co-authored-by: Jakob <jakob.hahn@hetzner.com>
1 parent 4fd2edf commit 1f0884a

614 files changed

Lines changed: 2860 additions & 1615 deletions

File tree

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

.github/workflows/check-gen.yml

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,8 +8,8 @@ jobs:
88
runs-on: ubuntu-latest
99
continue-on-error: true
1010
steps:
11-
- uses: actions/checkout@v6
12-
- uses: actions/setup-go@v6
11+
- uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6
12+
- uses: actions/setup-go@4a3601121dd01d1626a1e23e37211e3254c1c06c # v6
1313
with: { go-version-file: 'go.mod' }
1414
- name: Fetch OpenAPI spec
1515
run: make fetch-opensearch-spec

.github/workflows/lint.yml

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,18 @@ jobs:
99
lint:
1010
name: Lint
1111
runs-on: ubuntu-latest
12+
strategy:
13+
matrix:
14+
# golangci-lint can only compile one point in the build-tag space per
15+
# run. Two mutually-exclusive boolean axes (integration/!integration and
16+
# multinode/!multinode) partition the source tree, so a single tag set
17+
# can never lint every file. Feature tags (core, plugins, ...) are
18+
# pure-OR alternatives and union harmlessly into every run, so complete
19+
# coverage needs one run per (integration, multinode) combination.
20+
build-tags:
21+
- "core plugins plugin_security plugin_index_management"
22+
- "integration core plugins plugin_security plugin_index_management"
23+
- "integration core plugins plugin_security plugin_index_management multinode"
1224
steps:
1325
- uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6
1426
- uses: actions/setup-go@4a3601121dd01d1626a1e23e37211e3254c1c06c # v6
@@ -17,7 +29,7 @@ jobs:
1729
uses: golangci/golangci-lint-action@82606bf257cbaff209d206a39f5134f0cfbfd2ee # v9
1830
with:
1931
version: v2.12.2
20-
args: --fix --build-tags "integration core plugins plugin_security plugin_index_management multinode"
32+
args: --fix --build-tags "${{ matrix.build-tags }}"
2133

2234
prettify:
2335
name: Prettify

.github/workflows/test-compatibility.yml

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -51,9 +51,9 @@ jobs:
5151
- { opensearch_version: 3.6.0, node_count: 3 }
5252
- { opensearch_version: latest, node_count: 3 }
5353
steps:
54-
- uses: actions/checkout@v6
54+
- uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6
5555

56-
- uses: actions/setup-go@v6
56+
- uses: actions/setup-go@4a3601121dd01d1626a1e23e37211e3254c1c06c # v6
5757
with: { go-version-file: 'go.mod' }
5858

5959
- run: go version

CHANGELOG.md

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
1818
- Add `OPENSEARCH_GO_ROUTER` environment variable to enable the DefaultRouter without code changes; set to `true` to opt in (off by default in v4, on by default in v5, removed in v6) ([#815](https://github.com/opensearch-project/opensearch-go/pull/815))
1919
- Add client-side metrics guide covering Metrics API, ConnectionMetric, PolicySnapshot, and RouterSnapshot ([#812](https://github.com/opensearch-project/opensearch-go/pull/812))
2020
- Add `InsecureSkipVerify` config option to disable TLS certificate verification without constructing a custom `http.Transport`, preserving `DefaultTransport` connection pooling, HTTP/2, and timeout defaults ([#786](https://github.com/opensearch-project/opensearch-go/issues/786))
21-
- Add `DisableResponseBuffering` config option to skip eager `io.ReadAll` buffering of response bodies in `Perform()`, reducing per-request allocations and TTFB for proxy and streaming use cases ([#786](https://github.com/opensearch-project/opensearch-go/issues/786))
21+
- Add `(*opensearchtransport.Client).Stream(*http.Request) (*http.Response, error)` and a `(*opensearch.Client).Stream` passthrough for raw byte forwarding (proxy and streaming use cases). Stream returns the unbuffered response body from `RoundTrip`; the caller owns reading and closing `res.Body`. Pairs with `opensearch.Do[T]` for typed, decoded results (the SDK owns the body). Stream is exposed only on the concrete `*Client` in v4; v5 will add it to `opensearchtransport.Interface` and remove the deprecated `Perform` ([#786](https://github.com/opensearch-project/opensearch-go/issues/786))
2222
- Add per-attempt `RequestTimeout` to bound individual HTTP round-trips, preventing indefinite hangs on stalled connections ([#786](https://github.com/opensearch-project/opensearch-go/issues/786))
2323
- Add `opensearchutil/shardhash` package with exported `Hash` and `ForRouting` functions for computing OpenSearch shard routing
2424
- Enhanced cluster readiness checking for improved test reliability: `testutil.NewClient()` now includes readiness validation (health + cluster state + nodes info)
@@ -202,6 +202,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
202202

203203
### Deprecated
204204

205+
- Mark `opensearchtransport.Client.Perform` and the `opensearch.Client.Perform` passthrough as deprecated; both remain fully functional in v4 (still buffering the response body via `io.ReadAll` + `NopCloser`) and will be removed in v5. New code should call `opensearch.Do[T]` for typed, decoded results or `opensearchtransport.Client.Stream` / `opensearch.Client.Stream` for raw byte forwarding.
205206
- Mark `Client.Do()` with a `Deprecated` doc annotation in favor of `opensearch.Do[T]()` for compile-time pointer safety; `Client.Do()` remains fully functional and will not be removed, but `staticcheck` SA1019 will nudge cross-package callers toward the safer generic alternative
206207
- Mark `opensearch.ToPointer` and `opensearchapi.ToPointer` as deprecated; they remain fully functional but will be removed in v5. Once the module's go directive moves to 1.26, callers can drop the helper entirely in favor of native `new(value)` literal syntax (e.g. `new(false)`)
207208

@@ -210,6 +211,11 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
210211
### Fixed
211212

212213
- Fix `BulkIndexerStats.NumAdded` overcounting items rejected by `Add()` when the caller's context is cancelled before the item could be enqueued: increment `NumAdded` only after the queue accepts the item, and add a new `BulkAddFailCount` counter for items dropped on the `<-ctx.Done()` branch. Migrate `bulkIndexerStats` fields to `sync/atomic.Uint64` typed values so future direct access is a compile-time error rather than a `-race`-only finding ([#783](https://github.com/opensearch-project/opensearch-go/issues/783))
214+
- Fix `opensearchtransport.Client.setReqGlobalHeader` comparing the per-request header value against the global header name, so a request-level header never suppressed the matching global default and both were sent ([#859](https://github.com/opensearch-project/opensearch-go/pull/859))
215+
- Fix gzip buffer-pool nil poisoning on compress error: `gzipCompressor.compress` returned `(nil, err)` while the caller's deferred `collectBuffer` still ran, putting a typed-nil `*bytes.Buffer` into the `sync.Pool` that panics on the next `Get().Reset()` ([#859](https://github.com/opensearch-project/opensearch-go/pull/859))
216+
- Fix `opensearchtransport.Client.Perform` silently dropping `io.ReadAll` errors during response buffering via `:=` shadowing; the read error now propagates wrapped in the new `opensearchtransport.ErrResponseBodyRead` sentinel, and `opensearch.Client.Do` classifies the `(resp != nil, err != nil)` case via `errors.Is` so only genuine body-read failures are labeled `ErrReadBody` (an unrelated transport error returned alongside a response, such as context cancellation during retry backoff, is no longer misreported as a read failure). As a consequence, `opensearch.Client.Do` now returns a non-nil `*Response` alongside a non-nil error in this case where it previously returned `(nil, err)`; callers detecting a hard transport failure should check `resp == nil` rather than `err != nil` ([#859](https://github.com/opensearch-project/opensearch-go/pull/859))
217+
- Fix error-response body not being closed in `opensearch.ParseError`. `ParseError` now closes the original body before re-wrapping the read bytes in a `NopCloser`. The v5preview `do()` no-decode error path no longer needs its own drain: `opensearch.Do` routes through the buffered `opensearchtransport.Client.Perform`, so the returned `resp.Body` is already an in-memory `NopCloser` over the full payload and stays readable for the caller ([#859](https://github.com/opensearch-project/opensearch-go/pull/859))
218+
- Fix response-body lifecycle on the raw `RoundTrip` paths that lack `Perform`'s buffering safety net, where closing a partially-read body defeated HTTP keep-alive: the stats poller (`cluster_health.go`), discovery's `/_cat/shards`, `/_cluster/state/metadata`, and `/_nodes` paths, and the `fetchClusterHealth`/`baselineHealthCheck`/`hardwareInfoHealthCheck` pollers now drain to EOF (`io.Copy(io.Discard, ...)`) before close (covering both non-200 returns and `json.Decode` success paths that stop before EOF). The AWS v1 and v2 signers now close the request body on the read-error path in `hexEncodedSha256OfRequest` ([#859](https://github.com/opensearch-project/opensearch-go/pull/859))
213219
- Add typed response-format defaults for `v5preview/opensearchapi/` cat, list, ppl, and sql operations: when the caller leaves `Format` unset, the SDK now emits the value the typed Resp struct expects (`json` for cat/list/explain, `jdbc` for ppl/sql query) instead of letting the server fall back to a default the JSON decoder cannot handle.
214220
- Replace `WaitForAllNodesReady` inline `require.Eventually` loop with a layered readiness FSM (`internal/test/readiness`) that observes per-node progression through `LayerTCP -> LayerHTTP -> LayerClusterJoin -> LayerStatsReady`, records transitions including regressions, and emits a structured per-node diagnostic with the full last cat-nodes response on timeout. Per-layer budgets are tuned for CI pessimism (cold JVM startup is the long pole); total budget for `TargetClusterReady` is 6.5 minutes. ([#650](https://github.com/opensearch-project/opensearch-go/issues/650))
215221
- Fix bulk indexer HTML-escaping `_id` and `routing` values containing `<`, `>`, or `&` characters, causing OpenSearch to store escaped values (e.g., `\u003croot_account\u003e` stored instead of `<root_account>`), leading to duplicate documents, unreachable data on read-by-ID paths, and potential shard routing mismatches. Present since the `json.Marshal` migration in 2021 (commit `3da59092`). Replace `json.Marshal` with `json.NewEncoder` + `SetEscapeHTML(false)` in `opensearchutil.worker.writeMeta` and `opensearchutil.JSONReader`; replace per-worker `aux []byte` with `sync.Pool`-backed `*bytes.Buffer`; add table-driven test coverage for `writeMeta` edge cases and refactor remaining `TestBulkIndexer` subtests to table-driven `require`-based style ([#824](https://github.com/opensearch-project/opensearch-go/pull/824))

Makefile

Lines changed: 33 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,29 @@ SHELL := /bin/bash
33
# Tool versions
44
GOLANGCI_LINT_VERSION := v2.12.2
55

6-
GOLANGCI_LINT_BUILD_TAGS := "integration core plugins plugin_security plugin_index_management multinode"
6+
# Build tags for linting.
7+
#
8+
# golangci-lint can only compile ONE point in the build-tag space per run, so a
9+
# single invocation can never lint every file. Two mutually-exclusive boolean
10+
# axes partition the tree:
11+
# - integration vs !integration: every unit *_test.go is `!integration`; every
12+
# integration test is `integration`. Setting `integration` drops all unit
13+
# test files from the type-check, and omitting it drops all integration ones.
14+
# - multinode vs !multinode (within integration only): the single-node
15+
# integration files are `!multinode`; the multinode ones require `multinode`.
16+
# The feature tags below are pure-OR alternatives -- `core` satisfies every
17+
# `(core || X)` constraint and `plugins` every `(plugins || X)` one -- so they
18+
# union harmlessly into every run. Complete coverage therefore requires running
19+
# golangci-lint once per (integration, multinode) combination with the feature
20+
# tags unioned in. GOLANGCI_LINT_TAG_SETS enumerates those runs; each element is
21+
# one quoted --build-tags argument. GOLANGCI_LINT_BUILD_TAGS is retained as the
22+
# maximal set for the osgen module, which has no build-constrained files.
23+
GOLANGCI_LINT_FEATURE_TAGS := core plugins plugin_security plugin_index_management
24+
GOLANGCI_LINT_BUILD_TAGS := "integration $(GOLANGCI_LINT_FEATURE_TAGS) multinode"
25+
GOLANGCI_LINT_TAG_SETS := \
26+
"$(GOLANGCI_LINT_FEATURE_TAGS)" \
27+
"integration $(GOLANGCI_LINT_FEATURE_TAGS)" \
28+
"integration $(GOLANGCI_LINT_FEATURE_TAGS) multinode"
729

830
# Container runtime detection: prefer nerdctl (containerd), fall back to docker.
931
# Override with CONTAINER_RUNTIME=docker or CONTAINER_RUNTIME=nerdctl.
@@ -204,9 +226,12 @@ lint: ## Run lint on the package
204226
lint.headers: ## Check license headers on all Go files (same check as CI)
205227
@.github/check-license-headers.sh
206228

207-
lint.local: ## Run lint locally (not in Docker) with all build tags
208-
@printf "\033[2m-> Running golangci-lint locally with all build tags...\033[0m\n"
209-
golangci-lint run --fix --build-tags $(GOLANGCI_LINT_BUILD_TAGS) --timeout=5m -v ./...
229+
lint.local: ## Run lint locally (not in Docker) across all build-tag combinations
230+
@printf "\033[2m-> Running golangci-lint locally across all build-tag sets...\033[0m\n"
231+
@for tags in $(GOLANGCI_LINT_TAG_SETS); do \
232+
printf "\033[2m --build-tags %s\033[0m\n" "$$tags"; \
233+
golangci-lint run --fix --build-tags "$$tags" --timeout=5m -v ./... || exit $$?; \
234+
done
210235
@printf "\033[2m-> Running golangci-lint in cmd/osgen (separate Go module)...\033[0m\n"
211236
cd cmd/osgen && golangci-lint run --fix --build-tags $(GOLANGCI_LINT_BUILD_TAGS) --timeout=5m -v ./...
212237

@@ -781,7 +806,10 @@ cluster.latency.show: ## Show current tc qdisc rules on each node
781806
done
782807

783808
linters:
784-
$(CTR) run -t --rm -v $$(pwd):/app -v ~/.cache/golangci-lint/$(GOLANGCI_LINT_VERSION):/root/.cache -w /app golangci/golangci-lint:$(GOLANGCI_LINT_VERSION) golangci-lint run --fix --build-tags $(GOLANGCI_LINT_BUILD_TAGS) --timeout=5m -v ./...
809+
@for tags in $(GOLANGCI_LINT_TAG_SETS); do \
810+
printf "\033[2m-> golangci-lint --build-tags %s\033[0m\n" "$$tags"; \
811+
$(CTR) run -t --rm -v $$(pwd):/app -v ~/.cache/golangci-lint/$(GOLANGCI_LINT_VERSION):/root/.cache -w /app golangci/golangci-lint:$(GOLANGCI_LINT_VERSION) golangci-lint run --fix --build-tags "$$tags" --timeout=5m -v ./... || exit $$?; \
812+
done
785813

786814
##@ GitHub CI
787815
#------------------------------------------------------------------------------

cmd/osgen/emit/frag_dispatch_test.go

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -343,22 +343,22 @@ func TestPerOpErrorTypeName_CatalogConsistency(t *testing.T) {
343343

344344
// (1) Forward: every group the catalog names with a per-op
345345
// aggregator type must declare 2+ wrappers there.
346-
for group := range errwrap.OperationWrappers {
346+
for group := range errwrap.OperationWrappers() {
347347
typeName := emit.PerOpErrorTypeName(group)
348348
if typeName == "" {
349349
continue
350350
}
351351
t.Run("type_for_"+group, func(t *testing.T) {
352352
t.Parallel()
353-
require.GreaterOrEqual(t, len(errwrap.OperationWrappers[group]), 2,
353+
require.GreaterOrEqual(t, len(errwrap.OperationWrappers()[group]), 2,
354354
"group %q has per-op error type %q but only %d wrapper(s) in OperationWrappers; either add wrappers or remove the switch arm",
355-
group, typeName, len(errwrap.OperationWrappers[group]))
355+
group, typeName, len(errwrap.OperationWrappers()[group]))
356356
})
357357
}
358358

359359
// (2) Reverse: every catalog entry with 2+ wrappers must name a
360360
// per-op aggregator type.
361-
for group, wrappers := range errwrap.OperationWrappers {
361+
for group, wrappers := range errwrap.OperationWrappers() {
362362
if len(wrappers) < 2 {
363363
continue
364364
}
@@ -378,7 +378,7 @@ func TestPerOpErrorTypeName_CatalogConsistency(t *testing.T) {
378378
for _, group := range switchGroups {
379379
t.Run("switch_arm_in_catalog_"+group, func(t *testing.T) {
380380
t.Parallel()
381-
_, ok := errwrap.OperationWrappers[group]
381+
_, ok := errwrap.OperationWrappers()[group]
382382
require.True(t, ok,
383383
"perOpErrorTypeName has a switch arm for group %q but the group is absent from errwrap.OperationWrappers; remove the arm or restore the catalog entry",
384384
group)

cmd/osgen/emit/frag_plugin.go

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -146,6 +146,11 @@ func NewClient(client *opensearch.Client) *Client {
146146
}
147147
148148
// do calls [opensearch.Do] and checks the response for errors.
149+
//
150+
// [opensearch.Do] routes through the buffered [opensearchtransport.Client.Perform],
151+
// so resp.Body here is already an [io.NopCloser] over a [bytes.Reader] -- the
152+
// connection has been drained and returned to the pool. The helper only needs
153+
// to translate IsError into a typed error.
149154
func do[T any](ctx context.Context, c *Client, method string, req opensearch.Request, dataPointer *T) (*opensearch.Response, error) {
150155
resp, err := opensearch.Do(ctx, c.Client, method, req, dataPointer)
151156
if err != nil {

cmd/osgen/emit/frag_tests.go

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -109,6 +109,7 @@ type ReqTestFragment struct {
109109
// Imports returns the imports the Req-test fragment needs.
110110
func (f *ReqTestFragment) Imports() []Import {
111111
imps := []Import{
112+
{Path: "net/http"},
112113
{Path: "testing"},
113114
{Path: "github.com/stretchr/testify/require"},
114115
{Path: f.ImportPath},
@@ -134,7 +135,8 @@ func (f *ReqTestFragment) Body() (string, error) {
134135

135136
//nolint:gochecknoglobals // const-ish read-only template
136137
var reqTestFragTmpl = template.Must(template.New("reqTest").Funcs(template.FuncMap{
137-
"quote": func(s string) string { return fmt.Sprintf("%q", s) },
138+
"quote": func(s string) string { return fmt.Sprintf("%q", s) },
139+
"methodExpr": HTTPMethodConst,
138140
}).Parse(`func Test{{.TypePrefix}}Req_GetRequest(t *testing.T) {
139141
t.Parallel()
140142
tests := []struct {
@@ -148,7 +150,7 @@ var reqTestFragTmpl = template.Must(template.New("reqTest").Funcs(template.FuncM
148150
{
149151
name: {{quote .Name}},
150152
req: {{$.PkgName}}.{{$.TypePrefix}}Req{ {{.FieldAssign}} },
151-
wantMethod: {{quote .WantMethod}},
153+
wantMethod: {{methodExpr .WantMethod}},
152154
wantPath: {{quote .WantPath}},
153155
wantErr: {{.WantErr}},
154156
},

0 commit comments

Comments
 (0)