From 9a41a8b71ecadc8cbee9e168b197dfb763669b74 Mon Sep 17 00:00:00 2001 From: Konstantin Kozoriz Date: Tue, 25 Aug 2026 15:07:16 +0300 Subject: [PATCH 01/13] feat(snapshot): add publish-mode TLS primitives to transport and safe client Add ValidateHTTPSURL (transport) and NewSafeClientForConfig (safe client) as the foundational pieces needed to build an HTTPS client against a published (ingress) DataImport endpoint. Signed-off-by: Konstantin Kozoriz --- internal/snapshot/transport/http.go | 8 ++++ internal/snapshot/transport/http_test.go | 58 ++++++++++++++++++++++ pkg/libsaferequest/client/http.go | 7 +++ pkg/libsaferequest/client/http_test.go | 61 ++++++++++++++++++++++++ 4 files changed, 134 insertions(+) create mode 100644 pkg/libsaferequest/client/http_test.go diff --git a/internal/snapshot/transport/http.go b/internal/snapshot/transport/http.go index cd5ff0710..15793e15d 100644 --- a/internal/snapshot/transport/http.go +++ b/internal/snapshot/transport/http.go @@ -1221,6 +1221,14 @@ func (c *Client) SetTLSCAData(caData []byte) { } } +// ValidateHTTPSURL requires rawURL to be a well-formed HTTPS origin, without +// requiring a CA (unlike ValidateHTTPSIdentity, whose CA argument may be empty +// on the publish path). +func ValidateHTTPSURL(rawURL string) error { + _, err := parseHTTPSOrigin(rawURL) + return err +} + // ValidateHTTPSIdentity requires an HTTPS origin and a strictly parseable, // non-empty PEM certificate bundle suitable for endpoint-specific trust. func ValidateHTTPSIdentity(rawURL string, caData []byte) error { diff --git a/internal/snapshot/transport/http_test.go b/internal/snapshot/transport/http_test.go index 256059d00..31bad993e 100644 --- a/internal/snapshot/transport/http_test.go +++ b/internal/snapshot/transport/http_test.go @@ -1212,6 +1212,64 @@ func TestTLSIdentityClient_FailsClosed(t *testing.T) { } } +// TestValidateHTTPSURL verifies ValidateHTTPSURL accepts only a well-formed HTTPS origin, +// without requiring a CA argument (unlike ValidateHTTPSIdentity) -- the publish upload path +// (status.ca empty by design behind Ingress) relies on this weaker check. +func TestValidateHTTPSURL(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + rawURL string + wantErr bool + }{ + { + name: "success: well-formed HTTPS origin", + rawURL: "https://importer.example.test:8443", + }, + { + name: "error: plaintext HTTP origin", + rawURL: "http://importer.example.test", + wantErr: true, + }, + { + name: "error: empty string", + rawURL: "", + wantErr: true, + }, + { + name: "error: malformed URL", + rawURL: "://bad-url", + wantErr: true, + }, + { + name: "error: scheme-less host", + rawURL: "importer.example.test", + wantErr: true, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + err := ValidateHTTPSURL(tc.rawURL) + + if tc.wantErr { + if err == nil { + t.Fatalf("ValidateHTTPSURL(%q) = nil, want error", tc.rawURL) + } + + return + } + + if err != nil { + t.Fatalf("ValidateHTTPSURL(%q) = %v, want nil", tc.rawURL, err) + } + }) + } +} + func newPersistentTLSServer( t *testing.T, serial int64, diff --git a/pkg/libsaferequest/client/http.go b/pkg/libsaferequest/client/http.go index ef967a83c..a3c416ca7 100644 --- a/pkg/libsaferequest/client/http.go +++ b/pkg/libsaferequest/client/http.go @@ -62,6 +62,13 @@ func NewSafeClient(flags ...*pflag.FlagSet) (*SafeClient, error) { return &SafeClient{restConfig}, nil } +// NewSafeClientForConfig derives a SafeClient from an already-resolved REST +// configuration instead of re-parsing --kubeconfig/--context flags, so a probe +// built from it targets the same cluster the caller already resolved. +func NewSafeClientForConfig(config *rest.Config) *SafeClient { + return &SafeClient{restConfig: rest.CopyConfig(config)} +} + // SetProbeEndpoint configures host, TLS ServerName and timeout for probe requests. func (c *SafeClient) SetProbeEndpoint(timeout time.Duration, targetHost, kubeServiceServerName string) { c.restConfig.Host = targetHost diff --git a/pkg/libsaferequest/client/http_test.go b/pkg/libsaferequest/client/http_test.go new file mode 100644 index 000000000..f62b27bed --- /dev/null +++ b/pkg/libsaferequest/client/http_test.go @@ -0,0 +1,61 @@ +/* +Copyright 2026 Flant JSC + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package client + +import ( + "testing" + "time" + + "k8s.io/client-go/rest" +) + +func TestNewSafeClientForConfig(t *testing.T) { + t.Parallel() + + t.Run("success: mutating the derived client leaves the original config untouched", func(t *testing.T) { + t.Parallel() + + original := &rest.Config{Host: "https://original.example:6443"} + + derived := NewSafeClientForConfig(original) + derived.SetProbeEndpoint(5*time.Second, "https://probe.example:443", "probe.example") + + if original.Host != "https://original.example:6443" { + t.Errorf("original.Host = %q, want unchanged", original.Host) + } + + if original.TLSClientConfig.ServerName != "" { + t.Errorf("original.TLSClientConfig.ServerName = %q, want unchanged (empty)", original.TLSClientConfig.ServerName) + } + + if derived.restConfig.Host != "https://probe.example:443" { + t.Errorf("derived.restConfig.Host = %q, want %q", derived.restConfig.Host, "https://probe.example:443") + } + }) + + t.Run("error: nil config panics inside rest.CopyConfig", func(t *testing.T) { + t.Parallel() + + defer func() { + if recover() == nil { + t.Fatal("NewSafeClientForConfig(nil) did not panic; rest.CopyConfig no longer dereferences a nil config") + } + }() + + NewSafeClientForConfig(nil) + }) +} From cd928cec63891dd5ac59e5dcae736175dcc434be Mon Sep 17 00:00:00 2001 From: Konstantin Kozoriz Date: Tue, 25 Aug 2026 15:07:33 +0300 Subject: [PATCH 02/13] feat(snapshot): support publish endpoint in DataImport upload Add spec.publish to the DataImport built by d8 snapshot upload, align it alongside spec.ttl on reuse, wait on status.publicURL when publish is enabled, and switch to a merged TLS trust pool with 401/403 diagnostics on that path. Signed-off-by: Konstantin Kozoriz --- internal/snapshot/snapimport/fs.go | 6 +- internal/snapshot/snapimport/fs_test.go | 79 ++ internal/snapshot/snapimport/volume.go | 189 ++++- internal/snapshot/snapimport/volume_test.go | 813 +++++++++++++++++++- 4 files changed, 1039 insertions(+), 48 deletions(-) diff --git a/internal/snapshot/snapimport/fs.go b/internal/snapshot/snapimport/fs.go index 07d940287..231d8bc0a 100644 --- a/internal/snapshot/snapimport/fs.go +++ b/internal/snapshot/snapimport/fs.go @@ -394,7 +394,8 @@ func headFileOffset(ctx context.Context, client httpDoer, fileURL string, totalS return 0, false, 0, nil default: - return 0, false, 0, fmt.Errorf("HEAD %s returned status %d (%s)", fileURL, resp.StatusCode, resp.Status) + return 0, false, 0, uploadStatusError(resp.StatusCode, + fmt.Errorf("HEAD %s returned status %d (%s)", fileURL, resp.StatusCode, resp.Status)) } } @@ -445,7 +446,8 @@ func doFileChunk(client httpDoer, req *http.Request, offset, requestEnd, totalSi } if resp.StatusCode != http.StatusCreated && resp.StatusCode != http.StatusNoContent { - return 0, false, fmt.Errorf("server error at offset %d: status %d (%s)", offset, resp.StatusCode, resp.Status) + return 0, false, uploadStatusError(resp.StatusCode, + fmt.Errorf("server error at offset %d: status %d (%s)", offset, resp.StatusCode, resp.Status)) } if err := bodyReport.validateExact(); err != nil { diff --git a/internal/snapshot/snapimport/fs_test.go b/internal/snapshot/snapimport/fs_test.go index 022798d95..72269c0c1 100644 --- a/internal/snapshot/snapimport/fs_test.go +++ b/internal/snapshot/snapimport/fs_test.go @@ -621,6 +621,85 @@ func TestDoFileChunk_StrictStatusesAndOffsets(t *testing.T) { } } +// TestHeadFileOffset_ClassifiesUnauthorized verifies headFileOffset's default (non-OK/ +// non-NotFound) branch wraps errUploadUnauthorized only for 401/403 responses, mirroring +// headBlockOffset's block-path classification. +func TestHeadFileOffset_ClassifiesUnauthorized(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + statusCode int + wantWrap bool + }{ + {name: "success: 401 wraps sentinel", statusCode: http.StatusUnauthorized, wantWrap: true}, + {name: "success: 403 wraps sentinel", statusCode: http.StatusForbidden, wantWrap: true}, + {name: "success: 500 does not wrap sentinel", statusCode: http.StatusInternalServerError, wantWrap: false}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + doer := fileHTTPDoer(func(*http.Request) (*http.Response, error) { + return fileHTTPResponse(tc.statusCode, http.Header{}), nil + }) + + _, _, _, err := headFileOffset(context.Background(), doer, "https://import.example/file", 10) + if err == nil { + t.Fatal("headFileOffset unexpectedly returned nil error") + } + + if got := errors.Is(err, errUploadUnauthorized); got != tc.wantWrap { + t.Errorf("errors.Is(err, errUploadUnauthorized) = %v, want %v (err=%v)", got, tc.wantWrap, err) + } + }) + } +} + +// TestDoFileChunk_ClassifiesUnauthorized verifies doFileChunk's non-Created/non-NoContent/ +// non-Conflict branch wraps errUploadUnauthorized only for 401/403 responses; the "want status +// mismatch" branches below it can never observe 401/403 since they only run once the status is +// already Created or NoContent. +func TestDoFileChunk_ClassifiesUnauthorized(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + statusCode int + wantWrap bool + }{ + {name: "success: 401 wraps sentinel", statusCode: http.StatusUnauthorized, wantWrap: true}, + {name: "success: 403 wraps sentinel", statusCode: http.StatusForbidden, wantWrap: true}, + {name: "success: 500 does not wrap sentinel", statusCode: http.StatusInternalServerError, wantWrap: false}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + doer := fileHTTPDoer(func(*http.Request) (*http.Response, error) { + return fileHTTPResponse(tc.statusCode, http.Header{}), nil + }) + + req, err := http.NewRequest(http.MethodPut, "https://import.example/file", bytes.NewReader([]byte("x"))) + if err != nil { + t.Fatalf("build request: %v", err) + } + req.ContentLength = 1 + + _, _, err = doFileChunk(doer, req, 0, 1, 1) + if err == nil { + t.Fatal("doFileChunk unexpectedly returned nil error") + } + + if got := errors.Is(err, errUploadUnauthorized); got != tc.wantWrap { + t.Errorf("errors.Is(err, errUploadUnauthorized) = %v, want %v (err=%v)", got, tc.wantWrap, err) + } + }) + } +} + func TestPutFile_SingleShotUpload_CorrectHeaders(t *testing.T) { payload := []byte("hello, filesystem import") diff --git a/internal/snapshot/snapimport/volume.go b/internal/snapshot/snapimport/volume.go index 6106a2ed9..a5f36a223 100644 --- a/internal/snapshot/snapimport/volume.go +++ b/internal/snapshot/snapimport/volume.go @@ -108,12 +108,26 @@ var dataImportGVR = schema.GroupVersionResource{Group: "storage-foundation.deckh var ErrForeignDataImport = errors.New("foreign DataImport collision") // errDataImportRecheck signals EnsureDataImport that the DataImport changed under our feet -// while alignDataImportTTL was retrying a conflicting update — it was deleted, or it +// while alignDataImportSpec was retrying a conflicting update — it was deleted, or it // transitioned to Ready=False/Expired — so the object must be re-evaluated from the top of // EnsureDataImport's reconcile loop (recreate or delete-and-recreate) rather than patched as // if it were still the healthy object the caller started with. var errDataImportRecheck = errors.New("DataImport changed during TTL alignment") +// errUploadUnauthorized marks an importer response that rejected the client's +// identity (401) or permissions (403). +var errUploadUnauthorized = errors.New("importer rejected the client identity") + +// uploadStatusError wraps err with errUploadUnauthorized when statusCode is 401 or 403, so +// callers can detect a rejected identity via errors.Is without parsing message text. +func uploadStatusError(statusCode int, err error) error { + if statusCode == http.StatusUnauthorized || statusCode == http.StatusForbidden { + return fmt.Errorf("%w: %w", err, errUploadUnauthorized) + } + + return err +} + // VolumeImporter imports a data leaf's volume bytes by creating an SVDM DataImport, // waiting for the importer to be ready, streaming the archive bytes, finalising the // upload, and waiting for the durable artifact to be produced. It is satisfied by @@ -150,6 +164,7 @@ type clusterVolumeImporter struct { sc *transport.Client newUploadClient func([]byte, string) (uploadHTTPClient, error) ttl string + publish bool poll time.Duration wait time.Duration requestTimeout time.Duration @@ -158,27 +173,42 @@ type clusterVolumeImporter struct { log *slog.Logger } -// NewClusterVolumeImporter builds the live VolumeImporter. ttl is the DataImport TTL, -// wait bounds the per-DataImport readiness/completion waits, and poll is the polling -// cadence. Block-volume uploads stream-decode directly into the PUT (see putBlock), so -// no scratch directory for decompressed temporary files is needed. -func NewClusterVolumeImporter( - dyn dynamic.Interface, - sc *transport.Client, - ttl string, - wait, poll time.Duration, - log *slog.Logger, -) VolumeImporter { +// ClusterVolumeImporterOptions configures NewClusterVolumeImporter. +type ClusterVolumeImporterOptions struct { + // Dynamic is the dynamic client used for DataImport CR lifecycle and status polling. + Dynamic dynamic.Interface + // Transport authenticates the HTTPS byte upload to the importer pod (or, when Publish + // is set, the published ingress endpoint). + Transport *transport.Client + // TTL is the idle TTL applied to every DataImport this importer creates or reuses. + TTL string + // Publish selects the published (ingress) upload endpoint instead of the in-cluster + // importer service. + Publish bool + // Wait bounds the per-DataImport readiness/completion waits. + Wait time.Duration + // Poll is the polling cadence used while waiting on DataImport status. + Poll time.Duration + // Log receives lifecycle events; a nil Log defaults to slog.Default(). + Log *slog.Logger +} + +// NewClusterVolumeImporter builds the live VolumeImporter. Block-volume uploads +// stream-decode directly into the PUT (see putBlock), so no scratch directory for +// decompressed temporary files is needed. +func NewClusterVolumeImporter(opts ClusterVolumeImporterOptions) VolumeImporter { + log := opts.Log if log == nil { log = slog.Default() } return &clusterVolumeImporter{ - dyn: dyn, - sc: sc, - ttl: ttl, - poll: poll, - wait: wait, + dyn: opts.Dynamic, + sc: opts.Transport, + ttl: opts.TTL, + publish: opts.Publish, + poll: opts.Poll, + wait: opts.Wait, requestTimeout: DefaultControlRequestTimeout, newRequestContext: context.WithTimeout, fsDecodeDeps: defaultFSDecodeDependencies(), @@ -243,8 +273,12 @@ func (c *clusterVolumeImporter) EnsureDataImport(ctx context.Context, leaf Plann } spec := map[string]interface{}{ - "ttl": c.ttl, - "mode": dataImportModePopulateData, + "ttl": c.ttl, + // Always set explicitly, including false: alignDataImportSpec compares this + // field against the server's returned value, and an implicit server-side + // default would make that comparison depend on a value we never sent. + "publish": c.publish, + "mode": dataImportModePopulateData, "snapshotRef": map[string]interface{}{ "apiVersion": leaf.APIVersion, "kind": leaf.Kind, @@ -274,9 +308,10 @@ func (c *clusterVolumeImporter) EnsureDataImport(ctx context.Context, leaf Plann } if !conditionFalseWithReason(existing, conditionReady, reasonExpired) { - // Align spec.ttl with the current run so retrying a stalled import with a - // longer --ttl is honoured instead of keeping the first create's value. - if tErr := c.alignDataImportTTL(ctx, ri, existing, leaf); tErr != nil { + // Align spec.ttl and spec.publish with the current run so retrying a stalled + // import with a longer --ttl, or a different --publish, is honoured instead + // of keeping the first create's values. + if tErr := c.alignDataImportSpec(ctx, ri, existing, leaf); tErr != nil { if errors.Is(tErr, errDataImportRecheck) { continue } @@ -333,6 +368,10 @@ func dataImportShortID(leaf PlannedNode) string { return leaf.DataImportIdentity[:dataImportIdentityIDLength] } +// dataImportAnnotations intentionally excludes spec.publish: publish is a property of +// how the CLI transports bytes to an existing DataImport, not of the archive content it +// identifies, so a --publish value change on a retry must not turn a reused DataImport +// into a foreign one (see validateDataImportSpec) and block resume. func dataImportAnnotations(leaf PlannedNode) map[string]string { return map[string]string{ dataImportIdentityVersionAnnotation: dataImportIdentityVersion, @@ -370,6 +409,9 @@ func validateDataImportMetadata(obj *unstructured.Unstructured, leaf PlannedNode return nil } +// validateDataImportSpec does not check spec.publish for the same reason +// dataImportAnnotations does not carry it: publish is a transport property, not part of +// the leaf's content identity. func validateDataImportSpec(obj *unstructured.Unstructured, leaf PlannedNode) error { mode, found, err := unstructured.NestedString(obj.Object, "spec", "mode") if err != nil || !found || mode != dataImportModePopulateData { @@ -413,17 +455,24 @@ func validateDataImportSpec(obj *unstructured.Unstructured, leaf PlannedNode) er return nil } -// alignDataImportTTL patches a reused DataImport's spec.ttl to the current run's TTL when it -// drifted, so increasing --ttl on a retry takes effect. No-op when already aligned. +// alignDataImportSpec patches a reused DataImport's spec.ttl and spec.publish to the +// current run's intent when either drifted, so increasing --ttl on a retry, or changing +// --publish between runs, takes effect. No-op when both are already aligned. publish is +// aligned in both directions (including true -> false): a prior --publish=true run must +// not keep exposing the upload publicly once a later run asks for the in-cluster path. +// +// Both fields share one Update because they land on the same object in the same +// reconcile pass; a second Update here would open a second, redundant conflict window on +// top of the one this function's own retry loop already handles. // // The whole body runs inside retry.RetryOnConflict, mirroring reconcileExistingMarker: a // conflicting Update forces a re-Get on the next attempt (current = nil), and the re-Get's // result is re-validated against leaf and re-checked for the Expired condition before any -// patch is attempted, so a concurrent run cannot cause a stale-revision TTL patch to land on +// patch is attempted, so a concurrent run cannot cause a stale-revision patch to land on // a foreign or already-expired object. errDataImportRecheck is returned as-is (never wrapped) // when the re-Get finds the object gone or expired, so EnsureDataImport's caller can tell // "retry from scratch" apart from a genuine patch failure via errors.Is. -func (c *clusterVolumeImporter) alignDataImportTTL(ctx context.Context, ri dynamic.ResourceInterface, existing *unstructured.Unstructured, leaf PlannedNode) error { +func (c *clusterVolumeImporter) alignDataImportSpec(ctx context.Context, ri dynamic.ResourceInterface, existing *unstructured.Unstructured, leaf PlannedNode) error { current := existing didUpdate := false @@ -452,8 +501,10 @@ func (c *clusterVolumeImporter) alignDataImportTTL(ctx context.Context, ri dynam return errDataImportRecheck } - cur, _, _ := unstructured.NestedString(current.Object, "spec", "ttl") - if cur == c.ttl { + curTTL, _, _ := unstructured.NestedString(current.Object, "spec", "ttl") + curPublish, _, _ := unstructured.NestedBool(current.Object, "spec", "publish") + + if curTTL == c.ttl && curPublish == c.publish { return nil } @@ -462,6 +513,10 @@ func (c *clusterVolumeImporter) alignDataImportTTL(ctx context.Context, ri dynam return fmt.Errorf("set DataImport ttl: %w", setErr) } + if setErr := unstructured.SetNestedField(candidate.Object, c.publish, "spec", "publish"); setErr != nil { + return fmt.Errorf("set DataImport publish: %w", setErr) + } + patched, updateErr := runControlRequest(ctx, c.requestTimeout, c.newRequestContext, func(requestCtx context.Context) (*unstructured.Unstructured, error) { return ri.Update(requestCtx, candidate, metav1.UpdateOptions{}) @@ -482,12 +537,13 @@ func (c *clusterVolumeImporter) alignDataImportTTL(ctx context.Context, ri dynam return err } - return fmt.Errorf("patch DataImport %s/%s ttl: %w", existing.GetNamespace(), existing.GetName(), err) + return fmt.Errorf("patch DataImport %s/%s spec: %w", existing.GetNamespace(), existing.GetName(), err) } if didUpdate { - c.log.Info("aligned DataImport ttl", - slog.String("namespace", existing.GetNamespace()), slog.String("name", existing.GetName()), slog.String("ttl", c.ttl)) + c.log.Info("aligned DataImport spec", + slog.String("namespace", existing.GetNamespace()), slog.String("name", existing.GetName()), + slog.String("ttl", c.ttl), slog.Bool("publish", c.publish)) } return nil @@ -563,7 +619,7 @@ func (c *clusterVolumeImporter) UploadVolumeData(ctx context.Context, leaf Plann return err } - url, _, _ := unstructured.NestedString(di.Object, "status", "url") + url := c.uploadBaseURL(di) volumeMode, _, _ := unstructured.NestedString(di.Object, "status", "volumeMode") caB64, _, _ := unstructured.NestedString(di.Object, "status", "ca") @@ -574,12 +630,30 @@ func (c *clusterVolumeImporter) UploadVolumeData(ctx context.Context, leaf Plann defer httpClient.CloseIdleConnections() if err := c.sendVolumeData(ctx, httpClient, url, volumeMode, leaf, namespace, diName, setTotal, onProgress, activate); err != nil { + if c.publish && errors.Is(err, errUploadUnauthorized) { + return fmt.Errorf("%w; ingress does not forward client TLS certificates — "+ + "use a kubeconfig with a bearer token, or --publish=false from inside the cluster", err) + } + return err } return c.waitDataImportCompleted(ctx, leaf, diName, namespace) } +// uploadBaseURL returns the endpoint clients must upload to: the published +// ingress URL when publish is enabled, otherwise the in-cluster importer URL. +func (c *clusterVolumeImporter) uploadBaseURL(di *unstructured.Unstructured) string { + field := "url" + if c.publish { + field = "publicURL" + } + + url, _, _ := unstructured.NestedString(di.Object, "status", field) + + return url +} + func verifyLeafPayloadCurrent(ctx context.Context, leaf PlannedNode) error { if leaf.archiveView == nil || leaf.payloadFile == nil { return nil @@ -761,8 +835,22 @@ func (c *clusterVolumeImporter) uploadClient(caB64, rawURL string) (uploadHTTPCl return nil, fmt.Errorf("decode DataImport status.ca: %w", err) } - if err := transport.ValidateHTTPSIdentity(rawURL, ca); err != nil { - return nil, fmt.Errorf("validate DataImport upload identity: %w", err) + // Through Ingress, TLS terminates at ingress-nginx's own certificate, which never chains + // to the importer pod's internal CA — endpoint-specific pinning is impossible there, so + // publish=true trades it for a merged trust pool (below). Confined to that branch; + // insecure-skip-tls-verify inherited from kubeconfig is untouched either way. + if c.publish { + if err := transport.ValidateHTTPSURL(rawURL); err != nil { + return nil, fmt.Errorf("validate DataImport publish upload URL: %w", err) + } + } + + // status.ca is normally empty on the publish path (no internal CA to pin to there), so + // identity pinning is skipped only in that case; it stays mandatory otherwise. + if !c.publish || len(ca) > 0 { + if err := transport.ValidateHTTPSIdentity(rawURL, ca); err != nil { + return nil, fmt.Errorf("validate DataImport upload identity: %w", err) + } } if c.newUploadClient != nil { @@ -776,8 +864,14 @@ func (c *clusterVolumeImporter) uploadClient(caB64, rawURL string) (uploadHTTPCl sub := c.sc.Copy() sub.SetRequestTimeout(0) - if err := sub.SetTLSIdentityCAData(ca); err != nil { - return nil, fmt.Errorf("configure DataImport upload TLS identity: %w", err) + if c.publish { + sub.SetTLSCAData(ca) + } + + if !c.publish { + if err := sub.SetTLSIdentityCAData(ca); err != nil { + return nil, fmt.Errorf("configure DataImport upload TLS identity: %w", err) + } } if err := sub.SetNetworkTimeouts(transport.NetworkTimeouts{ @@ -801,7 +895,9 @@ func (c *clusterVolumeImporter) uploadClient(caB64, rawURL string) (uploadHTTPCl } // waitDataImportReady blocks until the DataImport reports Ready=True with a populated -// status.url and volumeMode. +// volumeMode and upload endpoint — status.url normally, or status.publicURL when c.publish +// is set (the controller never revokes Ready once granted, so a DataImport republished after +// becoming Ready can sit at Ready=True with an empty publicURL until the ingress catches up). func (c *clusterVolumeImporter) waitDataImportReady( ctx context.Context, leaf PlannedNode, @@ -829,7 +925,7 @@ func (c *clusterVolumeImporter) waitDataImportReady( return nil, fmt.Errorf("data import %s/%s expired before becoming Ready (idle TTL elapsed); increase --ttl or retry", namespace, name) } - url, _, _ := unstructured.NestedString(di.Object, "status", "url") + url := c.uploadBaseURL(di) volumeMode, _, _ := unstructured.NestedString(di.Object, "status", "volumeMode") if conditionTrue(di, conditionReady) && url != "" && volumeMode != "" { @@ -837,6 +933,14 @@ func (c *clusterVolumeImporter) waitDataImportReady( } if time.Now().After(deadline) { + if c.publish && url == "" { + return nil, fmt.Errorf( + "timeout waiting for DataImport %s/%s to become Ready: status.publicURL is still empty; "+ + "the storage-foundation ingress may not be configured, or retry with --publish=false from inside the cluster", + namespace, name, + ) + } + return nil, fmt.Errorf("timeout waiting for DataImport %s/%s to become Ready", namespace, name) } @@ -2147,7 +2251,8 @@ func headBlockOffset(ctx context.Context, httpClient httpDoer, url string, total return 0, nil default: - return 0, fmt.Errorf("HEAD %s returned status %d (%s)", url, resp.StatusCode, resp.Status) + return 0, uploadStatusError(resp.StatusCode, + fmt.Errorf("HEAD %s returned status %d (%s)", url, resp.StatusCode, resp.Status)) } } @@ -2202,7 +2307,8 @@ func doBlockChunk(httpClient httpDoer, req *http.Request, offset, requestEnd, to } if resp.StatusCode != http.StatusCreated && resp.StatusCode != http.StatusNoContent { - return 0, false, fmt.Errorf("server error at offset %d: status %d (%s)", offset, resp.StatusCode, resp.Status) + return 0, false, uploadStatusError(resp.StatusCode, + fmt.Errorf("server error at offset %d: status %d (%s)", offset, resp.StatusCode, resp.Status)) } if err := bodyReport.validateExact(); err != nil { @@ -2277,7 +2383,8 @@ func postFinished(ctx context.Context, httpClient httpDoer, baseURL string) erro } if resp.StatusCode < http.StatusOK || resp.StatusCode >= http.StatusMultipleChoices { - return fmt.Errorf("finished returned status %d (%s)", resp.StatusCode, resp.Status) + return uploadStatusError(resp.StatusCode, + fmt.Errorf("finished returned status %d (%s)", resp.StatusCode, resp.Status)) } return nil diff --git a/internal/snapshot/snapimport/volume_test.go b/internal/snapshot/snapimport/volume_test.go index 77c834969..406fa0fa8 100644 --- a/internal/snapshot/snapimport/volume_test.go +++ b/internal/snapshot/snapimport/volume_test.go @@ -20,11 +20,15 @@ import ( gotar "archive/tar" "bytes" "context" + "crypto/ed25519" + "crypto/x509" + "crypto/x509/pkix" "encoding/base64" "encoding/pem" "errors" "fmt" "io" + "math/big" "math/rand" "net" "net/http" @@ -46,6 +50,7 @@ import ( k8sruntime "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" dynamicfake "k8s.io/client-go/dynamic/fake" + restclient "k8s.io/client-go/rest" clienttesting "k8s.io/client-go/testing" diapi "github.com/deckhouse/deckhouse-cli/internal/data/dataimport/api/v1alpha1" @@ -1637,6 +1642,170 @@ func TestUploadControlEndpoints_PropagateResponseByteLimit(t *testing.T) { } } +// TestUploadStatusError verifies the errUploadUnauthorized classifier: it wraps the sentinel +// only for HTTP 401/403 status codes (by code, never by message text), leaving every other +// status's error unwrapped so errors.Is(err, errUploadUnauthorized) stays false for them. +func TestUploadStatusError(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + statusCode int + wantWrap bool + }{ + {name: "success: 401 wraps sentinel", statusCode: http.StatusUnauthorized, wantWrap: true}, + {name: "success: 403 wraps sentinel", statusCode: http.StatusForbidden, wantWrap: true}, + {name: "success: 404 does not wrap sentinel", statusCode: http.StatusNotFound, wantWrap: false}, + {name: "success: 409 does not wrap sentinel", statusCode: http.StatusConflict, wantWrap: false}, + {name: "success: 500 does not wrap sentinel", statusCode: http.StatusInternalServerError, wantWrap: false}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + base := fmt.Errorf("status %d", tc.statusCode) + + err := uploadStatusError(tc.statusCode, base) + + if got := errors.Is(err, errUploadUnauthorized); got != tc.wantWrap { + t.Errorf("errors.Is(err, errUploadUnauthorized) = %v, want %v (err=%v)", got, tc.wantWrap, err) + } + + if !errors.Is(err, base) && tc.wantWrap { + t.Errorf("wrapped error lost the original base error: %v", err) + } + }) + } +} + +// TestHeadBlockOffset_ClassifiesUnauthorized verifies headBlockOffset's default (non-OK/ +// non-NotFound) branch wraps errUploadUnauthorized only for 401/403 responses. +func TestHeadBlockOffset_ClassifiesUnauthorized(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + statusCode int + wantWrap bool + }{ + {name: "success: 401 wraps sentinel", statusCode: http.StatusUnauthorized, wantWrap: true}, + {name: "success: 403 wraps sentinel", statusCode: http.StatusForbidden, wantWrap: true}, + {name: "success: 500 does not wrap sentinel", statusCode: http.StatusInternalServerError, wantWrap: false}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + doer := testHTTPDoer(func(*http.Request) (*http.Response, error) { + return &http.Response{ + StatusCode: tc.statusCode, + Status: fmt.Sprintf("%d %s", tc.statusCode, http.StatusText(tc.statusCode)), + Header: http.Header{}, + Body: io.NopCloser(bytes.NewReader(nil)), + }, nil + }) + + _, err := headBlockOffset(context.Background(), doer, "https://importer.test/api/v1/block", 10) + if err == nil { + t.Fatal("headBlockOffset unexpectedly returned nil error") + } + + if got := errors.Is(err, errUploadUnauthorized); got != tc.wantWrap { + t.Errorf("errors.Is(err, errUploadUnauthorized) = %v, want %v (err=%v)", got, tc.wantWrap, err) + } + }) + } +} + +// TestDoBlockChunk_ClassifiesUnauthorized verifies doBlockChunk's non-Created/non-NoContent/ +// non-Conflict branch wraps errUploadUnauthorized only for 401/403 responses; the "want status +// mismatch" branches below it can never see 401/403 (they only run once the status is already +// Created or NoContent), so this exercises the only reachable wrap site. +func TestDoBlockChunk_ClassifiesUnauthorized(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + statusCode int + wantWrap bool + }{ + {name: "success: 401 wraps sentinel", statusCode: http.StatusUnauthorized, wantWrap: true}, + {name: "success: 403 wraps sentinel", statusCode: http.StatusForbidden, wantWrap: true}, + {name: "success: 500 does not wrap sentinel", statusCode: http.StatusInternalServerError, wantWrap: false}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + doer := testHTTPDoer(func(*http.Request) (*http.Response, error) { + return &http.Response{ + StatusCode: tc.statusCode, + Status: fmt.Sprintf("%d %s", tc.statusCode, http.StatusText(tc.statusCode)), + Header: http.Header{}, + Body: io.NopCloser(bytes.NewReader(nil)), + }, nil + }) + + req, err := http.NewRequestWithContext(context.Background(), http.MethodPut, "https://importer.test/api/v1/block", nil) + if err != nil { + t.Fatalf("build request: %v", err) + } + + _, _, err = doBlockChunk(doer, req, 0, 1, 1) + if err == nil { + t.Fatal("doBlockChunk unexpectedly returned nil error") + } + + if got := errors.Is(err, errUploadUnauthorized); got != tc.wantWrap { + t.Errorf("errors.Is(err, errUploadUnauthorized) = %v, want %v (err=%v)", got, tc.wantWrap, err) + } + }) + } +} + +// TestPostFinished_ClassifiesUnauthorized verifies postFinished's non-2xx branch wraps +// errUploadUnauthorized only for 401/403 responses. +func TestPostFinished_ClassifiesUnauthorized(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + statusCode int + wantWrap bool + }{ + {name: "success: 401 wraps sentinel", statusCode: http.StatusUnauthorized, wantWrap: true}, + {name: "success: 403 wraps sentinel", statusCode: http.StatusForbidden, wantWrap: true}, + {name: "success: 500 does not wrap sentinel", statusCode: http.StatusInternalServerError, wantWrap: false}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + doer := testHTTPDoer(func(*http.Request) (*http.Response, error) { + return &http.Response{ + StatusCode: tc.statusCode, + Status: fmt.Sprintf("%d %s", tc.statusCode, http.StatusText(tc.statusCode)), + Header: http.Header{}, + Body: io.NopCloser(bytes.NewReader(nil)), + }, nil + }) + + err := postFinished(context.Background(), doer, "https://importer.test") + if err == nil { + t.Fatal("postFinished unexpectedly returned nil error") + } + + if got := errors.Is(err, errUploadUnauthorized); got != tc.wantWrap { + t.Errorf("errors.Is(err, errUploadUnauthorized) = %v, want %v (err=%v)", got, tc.wantWrap, err) + } + }) + } +} + func TestSendVolumeData_WriteDeadlineLeavesResumeOffsetAndSkipsFinished(t *testing.T) { t.Parallel() @@ -4440,6 +4609,130 @@ func readyDataImportObj(leaf PlannedNode, rawURL, volumeMode, ca string) *unstru return obj } +// readyDataImportObjWithPublicURL builds a Ready DataImport with BOTH status.url and +// status.publicURL independently settable, for exercising uploadBaseURL/waitDataImportReady's +// publish-vs-non-publish branch selection precisely. +func readyDataImportObjWithPublicURL(leaf PlannedNode, url, publicURL, volumeMode, ca string) *unstructured.Unstructured { + obj := dataImportObjForLeaf(targetNS, leaf, false) + _ = unstructured.SetNestedSlice(obj.Object, readyConditions(conditionReady), "status", "conditions") + _ = unstructured.SetNestedField(obj.Object, volumeMode, "status", "volumeMode") + _ = unstructured.SetNestedField(obj.Object, ca, "status", "ca") + + if url != "" { + _ = unstructured.SetNestedField(obj.Object, url, "status", "url") + } + + if publicURL != "" { + _ = unstructured.SetNestedField(obj.Object, publicURL, "status", "publicURL") + } + + return obj +} + +// newTestVolumeImporterWithWait builds a clusterVolumeImporter like newTestVolumeImporter but +// with an explicit (short) wait budget, for tests that must observe a timeout without paying +// the default 2-second wait. +func newTestVolumeImporterWithWait(dyn *dynamicfake.FakeDynamicClient, publish bool, wait time.Duration) *clusterVolumeImporter { + imp := newTestVolumeImporter(dyn) + imp.publish = publish + imp.wait = wait + imp.poll = time.Millisecond + + return imp +} + +// TestWaitDataImportReady_PublishRequiresPublicURL is the most important regression guard for +// the publish readiness contract: the storage-foundation controller never lowers Ready back +// to False once it is True, so a DataImport that is Ready=True with status.url populated but +// status.publicURL still empty (Ingress wiring lagging behind the importer pod) must NOT be +// treated as ready when publish=true -- it must keep waiting until it times out. +func TestWaitDataImportReady_PublishRequiresPublicURL(t *testing.T) { + t.Parallel() + + leaf := volumeSnapshotLeaf("pvc-1") + di := readyDataImportObjWithPublicURL(leaf, "https://in-cluster.test", "", volumeModeBlock, "") + + dyn := newFakeDataImportDyn(di) + imp := newTestVolumeImporterWithWait(dyn, true, 60*time.Millisecond) + + start := time.Now() + _, err := imp.waitDataImportReady(context.Background(), leaf, imp.DataImportName(leaf), targetNS) + elapsed := time.Since(start) + + if err == nil { + t.Fatal("waitDataImportReady returned nil, want a timeout error (Ready=True but publicURL empty must not satisfy publish readiness)") + } + + if !strings.Contains(err.Error(), "publicURL") { + t.Errorf("error = %v, want it to mention publicURL", err) + } + + if elapsed < imp.wait { + t.Errorf("waitDataImportReady returned after %v, want it to wait out the full %v budget", elapsed, imp.wait) + } +} + +// TestWaitDataImportReady_PublishUsesPublicURL verifies that once status.publicURL is +// populated, publish=true returns immediately using it (not status.url). +func TestWaitDataImportReady_PublishUsesPublicURL(t *testing.T) { + t.Parallel() + + leaf := volumeSnapshotLeaf("pvc-1") + di := readyDataImportObjWithPublicURL(leaf, "https://in-cluster.test", "https://published.test", volumeModeBlock, "") + + dyn := newFakeDataImportDyn(di) + imp := newTestVolumeImporterWithWait(dyn, true, time.Second) + + got, err := imp.waitDataImportReady(context.Background(), leaf, imp.DataImportName(leaf), targetNS) + if err != nil { + t.Fatalf("waitDataImportReady: %v", err) + } + + if url := imp.uploadBaseURL(got); url != "https://published.test" { + t.Errorf("uploadBaseURL = %q, want the published URL https://published.test (not status.url)", url) + } +} + +// TestWaitDataImportReady_NonPublishIgnoresPublicURL is the mirror regression guard: with +// publish=false, a populated status.publicURL must never be mistaken for status.url. An empty +// status.url with a populated publicURL must still time out. +func TestWaitDataImportReady_NonPublishIgnoresPublicURL(t *testing.T) { + t.Parallel() + + leaf := volumeSnapshotLeaf("pvc-1") + di := readyDataImportObjWithPublicURL(leaf, "", "https://published.test", volumeModeBlock, "") + + dyn := newFakeDataImportDyn(di) + imp := newTestVolumeImporterWithWait(dyn, false, 60*time.Millisecond) + + _, err := imp.waitDataImportReady(context.Background(), leaf, imp.DataImportName(leaf), targetNS) + if err == nil { + t.Fatal("waitDataImportReady returned nil, want a timeout error (publish=false must never use status.publicURL)") + } +} + +// TestWaitDataImportReady_NonPublishUsesURLEvenWithPublicURLSet is a regression guard against +// crossed branches: when both status.url and status.publicURL are populated, publish=false +// must resolve to status.url. +func TestWaitDataImportReady_NonPublishUsesURLEvenWithPublicURLSet(t *testing.T) { + t.Parallel() + + leaf := volumeSnapshotLeaf("pvc-1") + di := readyDataImportObjWithPublicURL(leaf, "https://in-cluster.test", "https://published.test", volumeModeBlock, "") + + dyn := newFakeDataImportDyn(di) + imp := newTestVolumeImporterWithWait(dyn, false, time.Second) + + got, err := imp.waitDataImportReady(context.Background(), leaf, imp.DataImportName(leaf), targetNS) + if err != nil { + t.Fatalf("waitDataImportReady: %v", err) + } + + if url := imp.uploadBaseURL(got); url != "https://in-cluster.test" { + t.Errorf("uploadBaseURL = %q, want the in-cluster URL https://in-cluster.test (not publicURL)", url) + } +} + func TestUploadVolumeData_SkipsCompleted(t *testing.T) { // DataFile is set so the block-data preflight passes; the file is never opened because // the completed-import short-circuit returns before any upload. @@ -4519,6 +4812,85 @@ func TestUploadVolumeData_ClosesClientAfterRequestError(t *testing.T) { } } +// TestUploadVolumeData_UnauthorizedHintDependsOnPublish verifies UploadVolumeData's error +// wrapping around errUploadUnauthorized (a 401 from the importer's block HEAD probe): with +// publish=true it appends the bearer-token/kubeconfig hint explaining why a certificate-based +// kubeconfig fails through the published Ingress path; with publish=false the underlying +// error is returned unchanged, since that hint would be misleading for the in-cluster path. +func TestUploadVolumeData_UnauthorizedHintDependsOnPublish(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + publish bool + wantHint bool + }{ + {name: "success: publish=true appends the bearer-token hint", publish: true, wantHint: true}, + {name: "success: publish=false leaves the error unchanged", publish: false, wantHint: false}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + payload := []byte("unauthorized upload") + dataFile := filepath.Join(t.TempDir(), "data.bin") + if err := os.WriteFile(dataFile, payload, 0o600); err != nil { + t.Fatalf("write block payload: %v", err) + } + + leaf := volumeSnapshotLeaf("pvc-unauthorized") + leaf.DataFile = dataFile + leaf.Size = strconv.Itoa(len(payload)) + leaf.SizeBytes = int64(len(payload)) + leaf.DataImportIdentity = dataImportIdentity(leaf) + + ca := testUploadCA(t) + di := readyDataImportObjWithPublicURL(leaf, "https://importer.test", "https://importer.test", volumeModeBlock, base64.StdEncoding.EncodeToString(ca)) + + importer := newTestVolumeImporter(newFakeDataImportDyn(di)) + importer.publish = tc.publish + + importer.newUploadClient = func([]byte, string) (uploadHTTPClient, error) { + return &testUploadHTTPClient{ + do: func(*http.Request) (*http.Response, error) { + return &http.Response{ + StatusCode: http.StatusUnauthorized, + Status: "401 Unauthorized", + Header: http.Header{}, + Body: io.NopCloser(bytes.NewReader(nil)), + }, nil + }, + close: func() {}, + }, nil + } + + err := importer.UploadVolumeData( + context.Background(), + leaf, + importer.DataImportName(leaf), + targetNS, + nil, + nil, + nil, + ) + if err == nil { + t.Fatal("UploadVolumeData unexpectedly returned nil for a 401 response") + } + + if !errors.Is(err, errUploadUnauthorized) { + t.Fatalf("UploadVolumeData error = %v, want errors.Is(errUploadUnauthorized)", err) + } + + hasHint := strings.Contains(err.Error(), "bearer token") + + if hasHint != tc.wantHint { + t.Errorf("error contains bearer-token hint = %v, want %v (err=%v)", hasHint, tc.wantHint, err) + } + }) + } +} + func TestUploadVolumeData_CancellationClosesOnlyAfterInFlightRequestReturns(t *testing.T) { payload := []byte("cancel upload") dataFile := filepath.Join(t.TempDir(), "data.bin") @@ -4658,6 +5030,211 @@ func testUploadCA(t *testing.T) []byte { return pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: certificate.Raw}) } +// TestUploadClientPublish_ValidatesBeforeFactory mirrors +// TestUploadClientRejectsInvalidIdentityBeforeFactory for the publish=true branch of +// uploadClient: ValidateHTTPSURL still requires HTTPS, and a non-empty malformed CA is still +// rejected (fail closed), but an EMPTY CA -- expected on the publish path, since Ingress +// terminates TLS with its own certificate rather than the importer's internal CA -- must be +// accepted and reach the client factory. +func TestUploadClientPublish_ValidatesBeforeFactory(t *testing.T) { + t.Parallel() + + validCA := base64.StdEncoding.EncodeToString(testUploadCA(t)) + + tests := []struct { + name string + rawURL string + ca string + wantErr bool + wantFactory bool + }{ + { + name: "error: plaintext URL rejected even under publish", + rawURL: "http://127.0.0.1:8443", + ca: validCA, + wantErr: true, + }, + { + name: "error: malformed non-empty CA is rejected fail-closed", + rawURL: "https://127.0.0.1:8443", + ca: base64.StdEncoding.EncodeToString([]byte("not PEM")), + wantErr: true, + }, + { + name: "error: invalid base64 CA is rejected", + rawURL: "https://127.0.0.1:8443", + ca: "%%%", + wantErr: true, + }, + { + name: "success: empty CA is accepted and reaches the factory", + rawURL: "https://127.0.0.1:8443", + ca: "", + wantFactory: true, + }, + { + name: "success: valid CA is still accepted and reaches the factory", + rawURL: "https://127.0.0.1:8443", + ca: validCA, + wantFactory: true, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + var factoryCalls atomic.Int64 + + importer := &clusterVolumeImporter{ + publish: true, + newUploadClient: func([]byte, string) (uploadHTTPClient, error) { + factoryCalls.Add(1) + + return &testUploadHTTPClient{close: func() {}}, nil + }, + } + + _, err := importer.uploadClient(tc.ca, tc.rawURL) + + if tc.wantErr { + if err == nil { + t.Fatal("uploadClient unexpectedly accepted invalid identity/URL under publish") + } + } else if err != nil { + t.Fatalf("uploadClient: %v", err) + } + + wantCalls := int64(0) + if tc.wantFactory { + wantCalls = 1 + } + + if got := factoryCalls.Load(); got != wantCalls { + t.Fatalf("upload client factory calls = %d, want %d", got, wantCalls) + } + }) + } +} + +// unrelatedCertificatePEM returns a PEM-encoded self-signed certificate generated with its +// own independent key pair, standing in for a status.ca (or an operator-supplied CA) that does +// not chain to the real upload origin's certificate -- exercising the "foreign CA" side of the +// publish-vs-non-publish pinning tests below. It is deliberately NOT taken from a second +// httptest.NewTLSServer: httptest servers share one built-in default certificate unless +// explicitly configured otherwise, which would make the "unrelated" CA accidentally identical +// to the real origin's and silently defeat the test. +func unrelatedCertificatePEM(t *testing.T) []byte { + t.Helper() + + seed := bytes.Repeat([]byte{0x42}, ed25519.SeedSize) + privateKey := ed25519.NewKeyFromSeed(seed) + template := &x509.Certificate{ + SerialNumber: big.NewInt(9999), + Subject: pkix.Name{CommonName: "unrelated-test-ca"}, + NotBefore: time.Date(2020, 1, 1, 0, 0, 0, 0, time.UTC), + NotAfter: time.Date(2035, 1, 1, 0, 0, 0, 0, time.UTC), + KeyUsage: x509.KeyUsageDigitalSignature, + ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth}, + DNSNames: []string{"unrelated.invalid"}, + } + + der, err := x509.CreateCertificate(rand.New(rand.NewSource(1)), template, template, privateKey.Public(), privateKey) + if err != nil { + t.Fatalf("create unrelated test certificate: %v", err) + } + + return pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: der}) +} + +// TestUploadClient_PinningDependsOnPublish is the critical regression guard for the +// publish-path trust model: with publish=true, uploadClient merges status.ca into the +// caller's already-configured (kubeconfig) trust pool via SetTLSCAData, so a request to the +// real origin succeeds even though the supplied CA itself is unrelated to it. With +// publish=false, uploadClient calls SetTLSIdentityCAData instead, which REPLACES trust with +// exactly the supplied CA -- so the same unrelated CA must cause the request to the real +// origin to fail. A regression here (e.g. always using SetTLSCAData) would silently widen the +// in-cluster upload path's pinning. +func TestUploadClient_PinningDependsOnPublish(t *testing.T) { + t.Parallel() + + srv := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusNoContent) + })) + t.Cleanup(srv.Close) + + certificate := srv.Certificate() + if certificate == nil { + t.Fatal("TLS test server has no certificate") + } + + trustedCA := pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: certificate.Raw}) + unrelatedCA := unrelatedCertificatePEM(t) + + tests := []struct { + name string + publish bool + wantErr bool + }{ + { + name: "success: publish merges the unrelated CA with the kubeconfig-trusted pool", + publish: true, + }, + { + name: "error: non-publish pins exclusively to the unrelated CA and rejects the real origin", + publish: false, + wantErr: true, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + config := &restclient.Config{ + Host: srv.URL, + BearerToken: "must-not-leak", + TLSClientConfig: restclient.TLSClientConfig{ + CAData: trustedCA, + }, + } + + importer := &clusterVolumeImporter{ + sc: transport.NewClientForConfig(config), + publish: tc.publish, + } + + httpClient, err := importer.uploadClient(base64.StdEncoding.EncodeToString(unrelatedCA), srv.URL) + if err != nil { + t.Fatalf("uploadClient: %v", err) + } + t.Cleanup(httpClient.CloseIdleConnections) + + req, err := http.NewRequestWithContext(context.Background(), http.MethodGet, srv.URL, nil) + if err != nil { + t.Fatalf("build request: %v", err) + } + + resp, doErr := httpClient.HTTPDo(req) + if resp != nil && resp.Body != nil { + _ = resp.Body.Close() + } + + if tc.wantErr { + if doErr == nil { + t.Fatal("request unexpectedly succeeded against the real origin under strict (non-publish) pinning to an unrelated CA") + } + + return + } + + if doErr != nil { + t.Fatalf("request against the real origin unexpectedly failed under publish's merged trust pool: %v", doErr) + } + }) + } +} + func TestUploadVolumeData_CompletedReuseRejectsChangedVerifiedPayload(t *testing.T) { payload := []byte("verified completed payload") nodeSpec := archiveNode{ @@ -5006,7 +5583,7 @@ func TestEnsureDataImport_AlignsTTLRetryingConflict(t *testing.T) { // TestEnsureDataImport_TTLConflictRevalidatesFreshObject covers the race the retry loop must // close: if a concurrent writer replaces the DataImport with a foreign one in the window between -// the conflicting Update and alignDataImportTTL's re-Get, the re-Get's result must be +// the conflicting Update and alignDataImportSpec's re-Get, the re-Get's result must be // re-validated against leaf before any patch — never blindly reused from the pre-conflict // revision. func TestEnsureDataImport_TTLConflictRevalidatesFreshObject(t *testing.T) { @@ -5028,7 +5605,7 @@ func TestEnsureDataImport_TTLConflictRevalidatesFreshObject(t *testing.T) { updateCalls++ if updateCalls == 1 { // Simulate a concurrent writer swapping in a foreign DataImport in the exact - // window the conflict forces alignDataImportTTL to re-Get. + // window the conflict forces alignDataImportSpec to re-Get. if err := dyn.Tracker().Update(dataImportGVR, foreign, targetNS); err != nil { return true, nil, fmt.Errorf("swap in foreign DataImport: %w", err) } @@ -5054,7 +5631,7 @@ func TestEnsureDataImport_TTLConflictRevalidatesFreshObject(t *testing.T) { } // TestEnsureDataImport_TTLTargetVanishedRecreates covers a conflict whose re-Get finds the -// DataImport gone: alignDataImportTTL must surface errDataImportRecheck (not a hard failure) so +// DataImport gone: alignDataImportSpec must surface errDataImportRecheck (not a hard failure) so // EnsureDataImport's outer loop re-evaluates from scratch and creates a fresh DataImport. func TestEnsureDataImport_TTLTargetVanishedRecreates(t *testing.T) { leaf := volumeSnapshotLeaf("pvc-1") @@ -5109,7 +5686,7 @@ func TestEnsureDataImport_TTLTargetVanishedRecreates(t *testing.T) { // TestEnsureDataImport_TTLTargetExpiredDuringAlignmentRecreates mirrors // TestEnsureDataImport_RecreatesExpired for the conflict-retry path: if the re-Get after a -// conflicting TTL Update finds the DataImport now Ready=False/Expired, alignDataImportTTL must +// conflicting TTL Update finds the DataImport now Ready=False/Expired, alignDataImportSpec must // surface errDataImportRecheck so EnsureDataImport's outer loop deletes and recreates it instead // of patching a dying object. func TestEnsureDataImport_TTLTargetExpiredDuringAlignmentRecreates(t *testing.T) { @@ -5165,7 +5742,7 @@ func TestEnsureDataImport_TTLTargetExpiredDuringAlignmentRecreates(t *testing.T) } // TestEnsureDataImport_TTLAlreadyAlignedIssuesNoUpdate is a regression anchor for the ordering -// requirement in alignDataImportTTL: the ttl-equality check must run after any re-Get, but it +// requirement in alignDataImportSpec: the ttl-equality check must run after any re-Get, but it // must still short-circuit to zero Updates on the common already-aligned path. func TestEnsureDataImport_TTLAlreadyAlignedIssuesNoUpdate(t *testing.T) { leaf := volumeSnapshotLeaf("pvc-1") @@ -5182,6 +5759,232 @@ func TestEnsureDataImport_TTLAlreadyAlignedIssuesNoUpdate(t *testing.T) { } } +// TestEnsureDataImport_BuildsSpecPublishField verifies EnsureDataImport always sets +// spec.publish explicitly (including false), matching the importer's own Publish option -- +// so alignDataImportSpec's later comparison against the server's returned value never depends +// on an implicit server-side default. +func TestEnsureDataImport_BuildsSpecPublishField(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + publish bool + }{ + {name: "success: publish=false is explicitly set", publish: false}, + {name: "success: publish=true is explicitly set", publish: true}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + leaf := volumeSnapshotLeaf("pvc-1") + + dyn := newFakeDataImportDyn() + imp := newTestVolumeImporter(dyn) + imp.publish = tc.publish + + if _, err := imp.EnsureDataImport(context.Background(), leaf, targetNS); err != nil { + t.Fatalf("EnsureDataImport: %v", err) + } + + diName := imp.DataImportName(leaf) + + got, err := dyn.Resource(dataImportGVR).Namespace(targetNS).Get(context.Background(), diName, metav1.GetOptions{}) + if err != nil { + t.Fatalf("created DataImport not found: %v", err) + } + + publish, found, err := unstructured.NestedBool(got.Object, "spec", "publish") + if err != nil { + t.Fatalf("read spec.publish: %v", err) + } + + if !found { + t.Fatal("spec.publish was not set, want explicit value") + } + + if publish != tc.publish { + t.Errorf("spec.publish = %v, want %v", publish, tc.publish) + } + }) + } +} + +// TestEnsureDataImport_AlignsPublishOnReuse verifies alignDataImportSpec patches spec.publish +// on a reused DataImport when it drifts from the current run's --publish, in exactly one +// Update, leaving spec.ttl untouched when it already matched. +func TestEnsureDataImport_AlignsPublishOnReuse(t *testing.T) { + leaf := volumeSnapshotLeaf("pvc-1") + + // dataImportObj never sets spec.publish, so it is absent (equivalent to false). + existing := dataImportObj(targetNS, "pvc-1", false) + + dyn := newFakeDataImportDyn(existing) + imp := newTestVolumeImporter(dyn) // ttl: "1h" + imp.publish = true + + if _, err := imp.EnsureDataImport(context.Background(), leaf, targetNS); err != nil { + t.Fatalf("EnsureDataImport: %v", err) + } + + if u := countDataImportActions(dyn, "update"); u != 1 { + t.Errorf("update calls = %d, want exactly 1", u) + } + + diName := imp.DataImportName(leaf) + + got, err := dyn.Resource(dataImportGVR).Namespace(targetNS).Get(context.Background(), diName, metav1.GetOptions{}) + if err != nil { + t.Fatalf("get DataImport: %v", err) + } + + publish, _, _ := unstructured.NestedBool(got.Object, "spec", "publish") + if !publish { + t.Error("spec.publish = false, want true (aligned to the current run's --publish)") + } + + ttl, _, _ := unstructured.NestedString(got.Object, "spec", "ttl") + if ttl != "1h" { + t.Errorf("spec.ttl = %q, want unchanged 1h", ttl) + } +} + +// TestEnsureDataImport_AlignsBothTTLAndPublishInOneUpdate is a regression anchor for +// alignDataImportSpec's single-Update contract: when BOTH spec.ttl and spec.publish drift on +// the same reused object, they must be patched together in exactly one Update, never two +// separate ones (which would open a second, redundant conflict window). +func TestEnsureDataImport_AlignsBothTTLAndPublishInOneUpdate(t *testing.T) { + leaf := volumeSnapshotLeaf("pvc-1") + + existing := dataImportObj(targetNS, "pvc-1", false) + _ = unstructured.SetNestedField(existing.Object, "2m", "spec", "ttl") + _ = unstructured.SetNestedField(existing.Object, false, "spec", "publish") + + dyn := newFakeDataImportDyn(existing) + imp := newTestVolumeImporter(dyn) // ttl: "1h" + imp.publish = true + + if _, err := imp.EnsureDataImport(context.Background(), leaf, targetNS); err != nil { + t.Fatalf("EnsureDataImport: %v", err) + } + + if u := countDataImportActions(dyn, "update"); u != 1 { + t.Fatalf("update calls = %d, want exactly 1 (both fields patched together)", u) + } + + diName := imp.DataImportName(leaf) + + got, err := dyn.Resource(dataImportGVR).Namespace(targetNS).Get(context.Background(), diName, metav1.GetOptions{}) + if err != nil { + t.Fatalf("get DataImport: %v", err) + } + + ttl, _, _ := unstructured.NestedString(got.Object, "spec", "ttl") + if ttl != "1h" { + t.Errorf("spec.ttl = %q, want 1h", ttl) + } + + publish, _, _ := unstructured.NestedBool(got.Object, "spec", "publish") + if !publish { + t.Error("spec.publish = false, want true") + } +} + +// TestEnsureDataImport_PublishDowngradeInOneUpdate mirrors +// TestEnsureDataImport_AlignsBothTTLAndPublishInOneUpdate for the true->false direction: +// publish is aligned bidirectionally (unlike internal/data's upgrade-only semantics), so a +// prior --publish=true run must not keep exposing the upload publicly once a later run asks +// for the in-cluster path. +func TestEnsureDataImport_PublishDowngradeInOneUpdate(t *testing.T) { + leaf := volumeSnapshotLeaf("pvc-1") + + existing := dataImportObj(targetNS, "pvc-1", false) + _ = unstructured.SetNestedField(existing.Object, true, "spec", "publish") + + dyn := newFakeDataImportDyn(existing) + imp := newTestVolumeImporter(dyn) // ttl: "1h", publish defaults false + + if _, err := imp.EnsureDataImport(context.Background(), leaf, targetNS); err != nil { + t.Fatalf("EnsureDataImport: %v", err) + } + + if u := countDataImportActions(dyn, "update"); u != 1 { + t.Fatalf("update calls = %d, want exactly 1", u) + } + + diName := imp.DataImportName(leaf) + + got, err := dyn.Resource(dataImportGVR).Namespace(targetNS).Get(context.Background(), diName, metav1.GetOptions{}) + if err != nil { + t.Fatalf("get DataImport: %v", err) + } + + publish, _, _ := unstructured.NestedBool(got.Object, "spec", "publish") + if publish { + t.Error("spec.publish = true, want false (downgraded from a prior --publish=true run)") + } +} + +// TestEnsureDataImport_PublishAndTTLAlignedIssuesNoUpdate is a regression anchor for the +// ordering requirement in alignDataImportSpec: both the ttl- and publish-equality checks must +// run after any re-Get, but must still short-circuit to zero Updates when both are already +// aligned -- not just ttl alone, as covered by TestEnsureDataImport_TTLAlreadyAlignedIssuesNoUpdate. +func TestEnsureDataImport_PublishAndTTLAlignedIssuesNoUpdate(t *testing.T) { + leaf := volumeSnapshotLeaf("pvc-1") + + existing := dataImportObj(targetNS, "pvc-1", false) + _ = unstructured.SetNestedField(existing.Object, true, "spec", "publish") + + dyn := newFakeDataImportDyn(existing) + imp := newTestVolumeImporter(dyn) // ttl: "1h" + imp.publish = true + + if _, err := imp.EnsureDataImport(context.Background(), leaf, targetNS); err != nil { + t.Fatalf("EnsureDataImport: %v", err) + } + + if u := countDataImportActions(dyn, "update"); u != 0 { + t.Errorf("update calls = %d, want 0 (ttl and publish already aligned)", u) + } +} + +// TestEnsureDataImport_PublishChangeNeverForeign verifies the deliberate exclusion of +// spec.publish from dataImportAnnotations/validateDataImportSpec: reusing a DataImport whose +// spec.publish differs from the current run's --publish must NOT be treated as +// ErrForeignDataImport, since publish is a transport property of THIS run, not part of the +// leaf's content identity. +func TestEnsureDataImport_PublishChangeNeverForeign(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + existingPublish bool + runPublish bool + }{ + {name: "success: existing false, run requests true", existingPublish: false, runPublish: true}, + {name: "success: existing true, run requests false", existingPublish: true, runPublish: false}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + leaf := volumeSnapshotLeaf("pvc-1") + existing := dataImportObj(targetNS, "pvc-1", false) + _ = unstructured.SetNestedField(existing.Object, tc.existingPublish, "spec", "publish") + + dyn := newFakeDataImportDyn(existing) + imp := newTestVolumeImporter(dyn) + imp.publish = tc.runPublish + + if _, err := imp.EnsureDataImport(context.Background(), leaf, targetNS); err != nil { + t.Fatalf("EnsureDataImport must not fail with ErrForeignDataImport on a publish-only change: %v", err) + } + }) + } +} + func TestEnsureDataImport_RecreatesExpired(t *testing.T) { leaf := volumeSnapshotLeaf("pvc-1") From 9f900c5aebc8672af5a10b164e723f888ff50421 Mon Sep 17 00:00:00 2001 From: Konstantin Kozoriz Date: Tue, 25 Aug 2026 15:07:49 +0300 Subject: [PATCH 03/13] feat(snapshot): add --publish flag to d8 snapshot upload Wire the flag through to the DataImport importer, auto-detecting the upload mode when unset, and document the bearer-token requirement for the published path. Signed-off-by: Konstantin Kozoriz --- internal/snapshot/cmd/snapimport/import.go | 48 ++++++- .../snapshot/cmd/snapimport/import_test.go | 117 ++++++++++++++++++ 2 files changed, 162 insertions(+), 3 deletions(-) diff --git a/internal/snapshot/cmd/snapimport/import.go b/internal/snapshot/cmd/snapimport/import.go index b49700910..60fc2a995 100644 --- a/internal/snapshot/cmd/snapimport/import.go +++ b/internal/snapshot/cmd/snapimport/import.go @@ -35,12 +35,14 @@ import ( "k8s.io/client-go/dynamic" "k8s.io/client-go/rest" + dataio "github.com/deckhouse/deckhouse-cli/internal/data" "github.com/deckhouse/deckhouse-cli/internal/snapshot/aggapi" snapshotapi "github.com/deckhouse/deckhouse-cli/internal/snapshot/api/v1alpha1" "github.com/deckhouse/deckhouse-cli/internal/snapshot/progress" "github.com/deckhouse/deckhouse-cli/internal/snapshot/snapimport" "github.com/deckhouse/deckhouse-cli/internal/snapshot/transport" systemflags "github.com/deckhouse/deckhouse-cli/internal/system/flags" + safeClient "github.com/deckhouse/deckhouse-cli/pkg/libsaferequest/client" ) const ( @@ -55,6 +57,7 @@ const ( flagAllowExisting = "allow-existing" flagAllowUnauthenticatedLegacy = "allow-unauthenticated-legacy" flagSkipUnsupportedFSEntries = "skip-unsupported-fs-entries" + flagPublish = "publish" defaultImportWorkers = 5 @@ -133,7 +136,18 @@ Scope and limitations: downgraded and tampered current archive. - Uploading requires RBAC to create DataImport (storage-volume-data-manager) and to call the manifests-and-children-refs-upload subresource (e.g. an admin kubeconfig); the - read-only snapshot admin role is not sufficient.`, + read-only snapshot admin role is not sufficient. + +--publish selects how each data leaf's bytes are streamed to its DataImport importer pod. +With --publish=false (or when autodetection picks it), bytes go straight to the importer's +in-cluster service, trusting only its internal CA (status.ca). With --publish=true, bytes go +through the storage-foundation-published Ingress endpoint (status.publicURL) instead, so a +kubeconfig without direct network access to the cluster's internal service network can still +upload. If --publish is not given, the command probes whether the in-cluster importer endpoint +is reachable and picks accordingly. IMPORTANT: the publish path works only with a kubeconfig +authenticated by a bearer token. Ingress terminates TLS with its own certificate and does not +forward the client's TLS certificate to the importer pod, so a certificate-based kubeconfig +receives a 401 when --publish=true.`, Example: ` # Upload the archive in ./out into namespace "restored" d8 snapshot upload -n restored -i ./out @@ -141,7 +155,10 @@ Scope and limitations: d8 snapshot upload -n restored -i ./out --node VolumeSnapshot/pvc-1 # Upload with a longer DataImport TTL and overall timeout - d8 snapshot upload -n restored -i ./out --ttl 4h --timeout 30m`, + d8 snapshot upload -n restored -i ./out --ttl 4h --timeout 30m + + # Upload through the published Ingress endpoint (requires a bearer-token kubeconfig) + d8 snapshot upload -n restored -i ./out --publish=true`, Args: cobra.NoArgs, RunE: func(cmd *cobra.Command, args []string) error { return Run(log, cmd, args) @@ -159,6 +176,8 @@ Scope and limitations: cmd.Flags().Bool(flagAllowExisting, false, "downgrade namespace preflight conflict check to a warning (import-mode markers from a prior run are never conflicts regardless of this flag)") cmd.Flags().Bool(flagAllowUnauthenticatedLegacy, false, "allow trusted pre-version archives whose snapshot.yaml metadata is unauthenticated (unsafe; explicit compatibility mode)") cmd.Flags().Bool(flagSkipUnsupportedFSEntries, false, "skip unsupported filesystem entries and report them after upload (causes data loss for skipped paths)") + cmd.Flags().Bool(flagPublish, false, "upload volume data through the published (ingress) importer endpoint instead of the in-cluster service; "+ + "if unset, the in-cluster endpoint's reachability is auto-detected") return cmd } @@ -279,6 +298,21 @@ func Run(log *slog.Logger, cmd *cobra.Command, _ []string) error { dataPlaneClient := transport.NewClientForConfig(restConfig) + publishFlag, err := dataio.ParsePublishFlag(cmd.Flags()) + if err != nil { + return fmt.Errorf("resolving --%s: %w", flagPublish, err) + } + + // Probe from the command's already-resolved restConfig, not a fresh parse of + // --kubeconfig/--context: reparsing could target a different cluster than the one this + // command is actually uploading into. + probeClient := safeClient.NewSafeClientForConfig(restConfig) + + publish, err := dataio.ResolvePublish(ctx, publishFlag, kubeClient, probeClient, log) + if err != nil { + return fmt.Errorf("resolving --%s: %w", flagPublish, err) + } + isTTY := term.IsTerminal(int(os.Stdout.Fd())) // Upload shows Upload/Uploading/DataImport wording instead of progress.New's @@ -298,7 +332,15 @@ func Run(log *slog.Logger, cmd *cobra.Command, _ []string) error { runLog = slog.New(slog.NewTextHandler(sink.LogWriter(), &slog.HandlerOptions{Level: slog.LevelWarn})) } - volumes := snapimport.NewClusterVolumeImporter(dynClient, dataPlaneClient, ttl, timeout, 3*time.Second, runLog) + volumes := snapimport.NewClusterVolumeImporter(snapimport.ClusterVolumeImporterOptions{ + Dynamic: dynClient, + Transport: dataPlaneClient, + TTL: ttl, + Publish: publish, + Wait: timeout, + Poll: 3 * time.Second, + Log: runLog, + }) cfg := snapimport.Config{ Namespace: namespace, diff --git a/internal/snapshot/cmd/snapimport/import_test.go b/internal/snapshot/cmd/snapimport/import_test.go index af5d2aaa2..b596b0833 100644 --- a/internal/snapshot/cmd/snapimport/import_test.go +++ b/internal/snapshot/cmd/snapimport/import_test.go @@ -477,3 +477,120 @@ func TestNewCommand_AllowExistingFlagDefault(t *testing.T) { t.Fatalf("default --%s: got true, want false (opt-in flag)", flagAllowExisting) } } + +// TestNewCommand_PublishFlagDefault verifies --publish defaults to false and, crucially, that +// Changed stays false when the flag is never passed on the command line: Run's autodetection +// path (dataio.ParsePublishFlag) distinguishes "not set" from "explicitly set to false" via +// Changed, and that distinction must survive flag registration untouched. +func TestNewCommand_PublishFlagDefault(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + }{ + {name: "success: default value is false and unset"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + cmd := NewCommand(slog.Default()) + + flag := cmd.Flags().Lookup(flagPublish) + if flag == nil { + t.Fatalf("--%s flag is not registered", flagPublish) + } + + publish, err := cmd.Flags().GetBool(flagPublish) + if err != nil { + t.Fatalf("getting %s flag: %v", flagPublish, err) + } + + if publish { + t.Fatalf("default --%s: got true, want false", flagPublish) + } + + if flag.Changed { + t.Fatal("--publish.Changed = true without ever being set on the command line, want false (autodetection relies on this)") + } + }) + } +} + +// TestNewCommand_PublishFlagExplicitlySet verifies that explicitly passing --publish=true +// marks the flag Changed, so Run's dataio.ParsePublishFlag sees an explicit override rather +// than falling back to autodetection. +func TestNewCommand_PublishFlagExplicitlySet(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + value string + want bool + }{ + {name: "success: explicit true", value: "true", want: true}, + {name: "success: explicit false", value: "false", want: false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + cmd := NewCommand(slog.Default()) + + if err := cmd.Flags().Set(flagPublish, tt.value); err != nil { + t.Fatalf("setting --%s flag: %v", flagPublish, err) + } + + flag := cmd.Flags().Lookup(flagPublish) + if flag == nil { + t.Fatalf("--%s flag is not registered", flagPublish) + } + + if !flag.Changed { + t.Fatal("--publish.Changed = false after explicitly setting the flag, want true") + } + + publish, err := cmd.Flags().GetBool(flagPublish) + if err != nil { + t.Fatalf("getting %s flag: %v", flagPublish, err) + } + + if publish != tt.want { + t.Fatalf("--%s = %v, want %v", flagPublish, publish, tt.want) + } + }) + } +} + +// TestNewCommand_PublishDocumentation verifies the command's Long/Example text documents +// --publish and its bearer-token requirement, since a certificate-based kubeconfig silently +// fails (401) through the published path -- this must be discoverable from --help. +func TestNewCommand_PublishDocumentation(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + fragment string + }{ + {name: "flag mention", fragment: "--publish"}, + {name: "ingress endpoint", fragment: "storage-foundation-published Ingress endpoint"}, + {name: "bearer token requirement", fragment: "works only with a kubeconfig"}, + {name: "certificate rejection", fragment: "receives a 401 when --publish=true"}, + {name: "example usage", fragment: "--publish=true"}, + } + + cmd := NewCommand(slog.Default()) + combined := cmd.Long + "\n" + cmd.Example + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + if !strings.Contains(combined, tc.fragment) { + t.Errorf("command Long/Example does not contain %q:\n%s", tc.fragment, combined) + } + }) + } +} From cf7d998f76fa5a347ea10cebbaa088fcfff9f013 Mon Sep 17 00:00:00 2001 From: Konstantin Kozoriz Date: Tue, 25 Aug 2026 19:54:13 +0300 Subject: [PATCH 04/13] fix(snapshot): enforce TLS verification on publish upload path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SetTLSCAData merged a trust pool but never reset the caller's inherited insecure-skip-tls-verify/tls-server-name, so Go skipped certificate verification entirely on that path — any endpoint could receive the real Kubernetes bearer token. Force verification on unconditionally, in both the rest.Config and the cloned transport, since client-go may already have baked the insecure flag into a base transport before this wrapper runs. Signed-off-by: Konstantin Kozoriz --- internal/snapshot/snapimport/volume.go | 7 +- internal/snapshot/snapimport/volume_test.go | 53 +++++++++-- internal/snapshot/transport/http.go | 20 +++++ internal/snapshot/transport/http_test.go | 99 +++++++++++++++++++++ 4 files changed, 169 insertions(+), 10 deletions(-) diff --git a/internal/snapshot/snapimport/volume.go b/internal/snapshot/snapimport/volume.go index a5f36a223..6416be889 100644 --- a/internal/snapshot/snapimport/volume.go +++ b/internal/snapshot/snapimport/volume.go @@ -837,8 +837,11 @@ func (c *clusterVolumeImporter) uploadClient(caB64, rawURL string) (uploadHTTPCl // Through Ingress, TLS terminates at ingress-nginx's own certificate, which never chains // to the importer pod's internal CA — endpoint-specific pinning is impossible there, so - // publish=true trades it for a merged trust pool (below). Confined to that branch; - // insecure-skip-tls-verify inherited from kubeconfig is untouched either way. + // publish=true trades it for a merged trust pool (below). Confined to that branch. Both + // branches force server certificate verification on regardless of what the caller's + // kubeconfig set for insecure-skip-tls-verify/tls-server-name — only the trust model + // differs: publish EXTENDS trust (system roots + kubeconfig CA + status.ca merged into one + // pool), non-publish REPLACES it (pinned exclusively to the importer pod's internal CA). if c.publish { if err := transport.ValidateHTTPSURL(rawURL); err != nil { return nil, fmt.Errorf("validate DataImport publish upload URL: %w", err) diff --git a/internal/snapshot/snapimport/volume_test.go b/internal/snapshot/snapimport/volume_test.go index 406fa0fa8..a4d4694c7 100644 --- a/internal/snapshot/snapimport/volume_test.go +++ b/internal/snapshot/snapimport/volume_test.go @@ -5172,9 +5172,10 @@ func TestUploadClient_PinningDependsOnPublish(t *testing.T) { unrelatedCA := unrelatedCertificatePEM(t) tests := []struct { - name string - publish bool - wantErr bool + name string + publish bool + insecure bool + wantErr bool }{ { name: "success: publish merges the unrelated CA with the kubeconfig-trusted pool", @@ -5185,17 +5186,49 @@ func TestUploadClient_PinningDependsOnPublish(t *testing.T) { publish: false, wantErr: true, }, + { + // Regression guard for the insecure-skip-tls-verify bypass: SetTLSCAData must force + // verification on even though the caller's kubeconfig inherited Insecure: true, so a + // server whose certificate is in NEITHER the system pool NOR the explicitly supplied + // CA must still be rejected -- and, crucially, the bearer token must never reach that + // untrusted server, since a failed handshake means the request body (headers + // included) was never sent. + name: "error: publish with inherited insecure-skip-tls-verify still enforces verification and never leaks the bearer token", + publish: true, + insecure: true, + wantErr: true, + }, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { t.Parallel() + targetURL := srv.URL + configCAData := trustedCA + + var receivedAuthHeader string + + if tc.insecure { + // A dedicated server (rather than the shared srv above) guarantees its + // certificate is untrusted by construction, and lets this subtest read + // receivedAuthHeader without racing the other subtests' concurrent requests + // against the shared srv. + untrustedSrv := httptest.NewTLSServer(http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) { + receivedAuthHeader = r.Header.Get("Authorization") + })) + t.Cleanup(untrustedSrv.Close) + + targetURL = untrustedSrv.URL + configCAData = nil + } + config := &restclient.Config{ - Host: srv.URL, + Host: targetURL, BearerToken: "must-not-leak", TLSClientConfig: restclient.TLSClientConfig{ - CAData: trustedCA, + Insecure: tc.insecure, + CAData: configCAData, }, } @@ -5204,13 +5237,13 @@ func TestUploadClient_PinningDependsOnPublish(t *testing.T) { publish: tc.publish, } - httpClient, err := importer.uploadClient(base64.StdEncoding.EncodeToString(unrelatedCA), srv.URL) + httpClient, err := importer.uploadClient(base64.StdEncoding.EncodeToString(unrelatedCA), targetURL) if err != nil { t.Fatalf("uploadClient: %v", err) } t.Cleanup(httpClient.CloseIdleConnections) - req, err := http.NewRequestWithContext(context.Background(), http.MethodGet, srv.URL, nil) + req, err := http.NewRequestWithContext(context.Background(), http.MethodGet, targetURL, nil) if err != nil { t.Fatalf("build request: %v", err) } @@ -5222,7 +5255,11 @@ func TestUploadClient_PinningDependsOnPublish(t *testing.T) { if tc.wantErr { if doErr == nil { - t.Fatal("request unexpectedly succeeded against the real origin under strict (non-publish) pinning to an unrelated CA") + t.Fatal("request unexpectedly succeeded against an untrusted origin") + } + + if tc.insecure && receivedAuthHeader != "" { + t.Fatalf("Authorization header leaked to an untrusted server: %q", receivedAuthHeader) } return diff --git a/internal/snapshot/transport/http.go b/internal/snapshot/transport/http.go index 15793e15d..97de8e621 100644 --- a/internal/snapshot/transport/http.go +++ b/internal/snapshot/transport/http.go @@ -1179,6 +1179,22 @@ func (c *Client) NewRTClient(schemeFuncs ...func(s *apiruntime.Scheme) error) (c return NewRuntimeClient(c.restConfig, schemeFuncs...) } +// SetTLSCAData extends inherited server trust with a merged pool: system roots, +// the supplied caData, and any CA already configured on this Client's rest.Config +// (e.g. from kubeconfig). Unlike SetTLSIdentityCAData, which REPLACES trust with +// exactly one endpoint-specific CA, this widens it — appropriate only where +// endpoint-specific pinning is impossible (see the publish path in +// internal/snapshot/snapimport/volume.go). +// +// Because the merged pool only adds trust, it must never be reachable through a +// bypass: an inherited insecure-skip-tls-verify or tls-server-name from kubeconfig +// would make certificate verification a no-op regardless of how large RootCAs is, +// so both are forced off here — on the rest.Config itself and, since client-go may +// have already built a base *http.Transport with those values baked in before this +// WrapTransport runs, again on the transport clone that carries RootCAs. This must +// run unconditionally, not only when caData is non-empty: an empty caData is the +// normal case on the publish path (the ingress does not expose the importer pod's +// internal CA), and verification must stay on even then. func (c *Client) SetTLSCAData(caData []byte) { sysPool, err := x509.SystemCertPool() if err != nil || sysPool == nil { @@ -1195,6 +1211,8 @@ func (c *Client) SetTLSCAData(caData []byte) { c.restConfig.TLSClientConfig.CAData = nil c.restConfig.TLSClientConfig.CAFile = "" + c.restConfig.TLSClientConfig.Insecure = false + c.restConfig.TLSClientConfig.ServerName = "" prev := c.restConfig.WrapTransport c.restConfig.WrapTransport = func(rt http.RoundTripper) http.RoundTripper { @@ -1216,6 +1234,8 @@ func (c *Client) SetTLSCAData(caData []byte) { } clonedTransport.TLSClientConfig.RootCAs = sysPool + clonedTransport.TLSClientConfig.InsecureSkipVerify = false + clonedTransport.TLSClientConfig.ServerName = "" return clonedTransport } diff --git a/internal/snapshot/transport/http_test.go b/internal/snapshot/transport/http_test.go index 31bad993e..4f894bbf2 100644 --- a/internal/snapshot/transport/http_test.go +++ b/internal/snapshot/transport/http_test.go @@ -313,6 +313,93 @@ func TestClient_SetTLSCAData_PassThroughNonTransport(t *testing.T) { } } +// TestClient_SetTLSCAData_ForcesVerification is the regression guard for the +// insecure-skip-tls-verify/tls-server-name bypass: SetTLSCAData builds a merged +// trust pool, but an inherited Insecure or ServerName from kubeconfig can make +// verification a no-op (InsecureSkipVerify) or check the wrong hostname +// regardless of how large that pool is. SetTLSCAData must force both off — on +// the rest.Config itself AND on the transport clone that carries RootCAs, since +// client-go may already have baked the inherited values into a base +// *http.Transport before WrapTransport runs. +func TestClient_SetTLSCAData_ForcesVerification(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + caData []byte + }{ + { + name: "nil CA data", + caData: nil, + }, + { + name: "valid CA data", + caData: testCACertificatePEM(t), + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + sc := &Client{ + restConfig: &rest.Config{ + TLSClientConfig: rest.TLSClientConfig{ + Insecure: true, + ServerName: "api.example", + }, + }, + } + + sc.SetTLSCAData(tc.caData) + + if sc.restConfig.TLSClientConfig.Insecure { + t.Error("restConfig.TLSClientConfig.Insecure = true, want false after SetTLSCAData") + } + + if sc.restConfig.TLSClientConfig.ServerName != "" { + t.Errorf("restConfig.TLSClientConfig.ServerName = %q, want empty after SetTLSCAData", + sc.restConfig.TLSClientConfig.ServerName) + } + + orig := &http.Transport{ + TLSClientConfig: &tls.Config{ + InsecureSkipVerify: true, // exercising the inherited-insecure bypass this test guards against + ServerName: "api.example", + }, + } + + got := sc.restConfig.WrapTransport(orig) + + wrapped, ok := got.(*http.Transport) + if !ok { + t.Fatalf("wrapped transport is not an *http.Transport: %T", got) + } + + if wrapped.TLSClientConfig.InsecureSkipVerify { + t.Error("cloned transport TLSClientConfig.InsecureSkipVerify = true, want false") + } + + if wrapped.TLSClientConfig.ServerName != "" { + t.Errorf("cloned transport TLSClientConfig.ServerName = %q, want empty", wrapped.TLSClientConfig.ServerName) + } + + if wrapped.TLSClientConfig.RootCAs == nil { + t.Error("cloned transport TLSClientConfig.RootCAs is nil") + } + + if !orig.TLSClientConfig.InsecureSkipVerify { + t.Error("original transport TLSClientConfig.InsecureSkipVerify was mutated, want it untouched") + } + + if orig.TLSClientConfig.ServerName != "api.example" { + t.Errorf("original transport TLSClientConfig.ServerName = %q, want it untouched (\"api.example\")", + orig.TLSClientConfig.ServerName) + } + }) + } +} + // TestClient_SetResponseHeaderTimeout_FailsFastOnHeaderStall asserts the // configured transport aborts a request whose server accepts the connection but // never sends response headers, within the response-header timeout. @@ -1306,6 +1393,18 @@ func newPersistentTLSServer( return server } +// testCACertificatePEM returns a PEM-encoded certificate suitable as a valid, +// well-formed CA bundle input, without requiring the caller to stand up and +// tear down a dedicated TLS server just to obtain one. +func testCACertificatePEM(t *testing.T) []byte { + t.Helper() + + srv := httptest.NewTLSServer(http.HandlerFunc(func(http.ResponseWriter, *http.Request) {})) + t.Cleanup(srv.Close) + + return certificatePEM(t, srv) +} + func certificatePEM(t *testing.T, server *httptest.Server) []byte { t.Helper() From 2f8910be9db6c7eab32be38dcefa986bf565e1c5 Mon Sep 17 00:00:00 2001 From: Konstantin Kozoriz Date: Wed, 26 Aug 2026 21:13:20 +0300 Subject: [PATCH 05/13] feat(snapshot): support publish endpoint in DataExport export Add spec.publish to the DataExport built by d8 snapshot download, upgrade it (one-way, optimistic-locked) on an adopted CR, wait on status.publicURL when publish is enabled, and switch to a merged TLS trust pool with 401/403 diagnostics on that path. Signed-off-by: Konstantin Kozoriz --- internal/snapshot/exporter/dataexport.go | 230 +++++- internal/snapshot/exporter/dataexport_test.go | 779 ++++++++++++++++++ internal/snapshot/exporter/export.go | 73 +- internal/snapshot/exporter/export_test.go | 570 ++++++++++++- internal/snapshot/exporter/http.go | 73 +- internal/snapshot/exporter/http_test.go | 189 +++++ 6 files changed, 1881 insertions(+), 33 deletions(-) diff --git a/internal/snapshot/exporter/dataexport.go b/internal/snapshot/exporter/dataexport.go index 41b0edb0b..8d7ad7f1d 100644 --- a/internal/snapshot/exporter/dataexport.go +++ b/internal/snapshot/exporter/dataexport.go @@ -29,6 +29,7 @@ import ( "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" + "k8s.io/client-go/util/retry" "sigs.k8s.io/controller-runtime/pkg/client" deapi "github.com/deckhouse/deckhouse-cli/internal/data/dataexport/api/v1alpha1" @@ -56,6 +57,19 @@ var ErrTargetUIDMismatch = errors.New("existing DataExport targets a different o // lifecycle without the exact Snapshot CR UID. var ErrTargetUIDRequired = errors.New("snapshot target UID is required") +// ErrPublishForeignOwner is returned when --publish requires enabling +// spec.publish on a DataExport that ANOTHER live download run owns. Adoption of a +// foreign CR is read-only by contract, so this run refuses to mutate it and fails +// fast instead of waiting out the full readiness timeout for a status.publicURL +// that will never appear. +var ErrPublishForeignOwner = errors.New("DataExport owned by another download run has publish disabled") + +// errDataExportRecheck signals that the adopted DataExport changed under our feet +// while alignDataExportPublish was aligning spec.publish — it vanished, started +// terminating, or expired. EnsureDataExport falls through to its create path +// instead of returning the object it can no longer vouch for. +var errDataExportRecheck = errors.New("DataExport changed during publish alignment") + // defaultDataExportTTL is the fallback TTL used for DataExport when the caller // passes an empty string. Snapshot transfers can be large, so we use a longer // default than the 2-minute interactive default. @@ -80,6 +94,20 @@ func dataExportExpired(conds []metav1.Condition) bool { return c != nil && c.Status == metav1.ConditionFalse && c.Reason == reasonExpired } +// exportBaseURL returns the base URL this run must talk to: the +// storage-foundation-published Ingress URL when the public endpoint was +// requested, otherwise the in-cluster exporter URL. The public URL carries a +// path prefix (https://////) while the +// internal one does not, so callers must join request paths onto THIS value and +// never re-derive them from the origin alone. +func exportBaseURL(de *deapi.DataExport, publicEndpoint bool) string { + if publicEndpoint { + return de.Status.PublicURL + } + + return de.Status.URL +} + // dataExportGonePollInterval is the poll cadence EnsureDataExport uses while waiting // for a terminating DataExport (DeletionTimestamp set) to fully vanish before it // recreates a fresh one. It is short because the controller's finalizer unwinding @@ -219,6 +247,7 @@ type ensureOptions struct { log *slog.Logger terminatingTimeout time.Duration acquisition **DataExportAcquisition + publish bool } // EnsureOption configures optional behavior of EnsureDataExport. @@ -322,6 +351,16 @@ func WithRunOwner(runID string, log *slog.Logger) EnsureOption { } } +// WithPublish makes EnsureDataExport request the storage-foundation-published +// (Ingress) endpoint for this DataExport: spec.publish is set on any CR this call +// CREATES, and an adopted CR that still has spec.publish=false is upgraded in place +// (see alignDataExportPublish). Publish is never downgraded back to false. +func WithPublish(publish bool) EnsureOption { + return func(o *ensureOptions) { + o.publish = publish + } +} + func (o ensureOptions) recordAcquisition(de *deapi.DataExport) error { if o.acquisition == nil { return nil @@ -363,6 +402,117 @@ func (o ensureOptions) warnIfForeign(de *deapi.DataExport, deName string) { slog.String("run_id", o.runID)) } +// alignDataExportPublish upgrades an ADOPTED DataExport's spec.publish from false +// to true so this run gets a status.publicURL. It never writes false: downgrading +// makes the storage-foundation controller DELETE the public Service and Ingress +// (reconcilePublishResources), which would tear the endpoint out from under a +// concurrent run still streaming through it. Returns the up-to-date object. +// +// The patch carries an OPTIMISTIC LOCK. spec.publish is the first spec field this +// client writes on a CR it did not necessarily create, and the deterministic CR +// name means two concurrent download runs resolve to the SAME object; the +// storage-foundation controller also writes conditions, status.url and finalizers +// on it continuously. The lock guarantees the write lands on the EXACT revision +// whose identity was validated in this same attempt. +// +// A conflict is never re-sent from a stale base: retry.RetryOnConflict drops the +// cached object, re-Gets it and re-runs EVERY identity check before patching +// again. Once retry.DefaultRetry is exhausted the conflict is returned to the +// caller, which fails the leaf so the operator's next resume run recomputes the +// target from scratch. +// +// This deliberately diverges from the older, already-shipped one-way publish +// upgrade in internal/data/dataexport/util/util.go (EnsureDataExportPublish, +// used by "d8 data export download"): that helper patches with plain +// client.MergeFrom and no identity re-validation. That path predates the +// deterministic-name, multi-run-adoption model this package documents (see +// runOwnerAnnotation), so a stale write there is far less likely to collide +// with a concurrent, identity-distinct owner. Here it is not — hence the lock +// and the re-validation. EnsureDataExportPublish is intentionally left as-is; +// this function does not replace or call it. +func (o ensureOptions) alignDataExportPublish( + ctx context.Context, + c client.Client, + existing *deapi.DataExport, + group, resource, kind, leafName string, +) (*deapi.DataExport, error) { + current := existing + didPatch := false + deName := existing.Name + + err := retry.RetryOnConflict(retry.DefaultRetry, func() error { + if current == nil { + latest := new(deapi.DataExport) + + getErr := c.Get(ctx, client.ObjectKey{Namespace: existing.Namespace, Name: deName}, latest) + if kubeerrors.IsNotFound(getErr) { + return errDataExportRecheck + } + + if getErr != nil { + return fmt.Errorf("get DataExport %q after conflict: %w", deName, getErr) + } + + current = latest + } + + if !targetRefMatches(current.Spec.TargetRef, group, resource, kind, leafName) { + return targetRefMismatchError(deName, current.Spec.TargetRef, group, resource, kind, leafName) + } + + if current.Annotations[targetUIDAnnotation] != string(o.targetUID) { + return targetUIDMismatchError(deName, current.Annotations[targetUIDAnnotation], o.targetUID) + } + + if current.DeletionTimestamp != nil || dataExportExpired(current.Status.Conditions) { + return errDataExportRecheck + } + + if current.Spec.Publish { + return nil + } + + owner := current.Annotations[runOwnerAnnotation] + if o.runID != "" && owner != "" && owner != o.runID { + return fmt.Errorf( + "%w: DataExport %q is owned by download run %q and has spec.publish=false; "+ + "wait for that run to finish, or rerun without --publish to use the in-cluster endpoint", + ErrPublishForeignOwner, deName, owner) + } + + base := current.DeepCopy() + current.Spec.Publish = true + + if patchErr := c.Patch(ctx, current, + client.MergeFromWithOptions(base, client.MergeFromWithOptimisticLock{})); patchErr != nil { + current = nil + return patchErr + } + + didPatch = true + + return nil + }) + if err != nil { + if errors.Is(err, errDataExportRecheck) || + errors.Is(err, ErrPublishForeignOwner) || + errors.Is(err, ErrTargetRefMismatch) || + errors.Is(err, ErrTargetUIDMismatch) { + return nil, err + } + + return nil, fmt.Errorf("patch DataExport %q spec.publish: %w", deName, err) + } + + if didPatch && o.log != nil { + o.log.Info("enabled publish on adopted DataExport", + slog.String("name", deName), + slog.String("run_id", o.runID)) + } + + return current, nil +} + // lifecycleAnnotations always stamps the target Snapshot UID and additionally // stamps runID when the caller tracks per-run ownership. func lifecycleAnnotations(runID string, targetUID types.UID) map[string]string { @@ -461,6 +611,30 @@ func EnsureDataExport( // Ownership is intentionally NOT changed on adoption. o.warnIfForeign(existing, deName) + if o.publish { + aligned, alignErr := o.alignDataExportPublish(ctx, c, existing, group, resource, kind, leafName) + if alignErr != nil { + if !errors.Is(alignErr, errDataExportRecheck) { + return nil, alignErr + } + + // The object started terminating mid-retry (observed inside + // alignDataExportPublish, not here). Falling through to Create + // below without an explicit waitForDataExportGone is deliberate, + // not an oversight: Create already swallows AlreadyExists and every + // fallthrough path re-fetches and re-validates identity (see the + // comment above the Create call), so at worst this run adopts the + // still-terminating object for one pass and converges to a fresh, + // this-run-owned CR on the next resume attempt — the same + // "one-run delay, not a regression" the Expired branch below + // already accepts. Adding a wait here would just duplicate that + // case's handling for a narrower trigger. + break + } + + existing = aligned + } + if err := o.recordAcquisition(existing); err != nil { return nil, err } @@ -503,6 +677,11 @@ func EnsureDataExport( }, Spec: deapi.DataexportSpec{ TTL: ttl, + // Always set explicitly, including false: alignDataExportPublish compares this + // field against the server's returned value, and the CRD declares no default: + // for spec.publish, so an implicit value would make that comparison depend on + // something we never sent. + Publish: o.publish, TargetRef: deapi.TargetRefSpec{ Group: group, Resource: resource, @@ -667,8 +846,30 @@ func readyConditionStatus(conds []metav1.Condition, hasURL bool) string { return "waiting" } +// waitReadyOptions carries optional readiness criteria for WaitReady. +type waitReadyOptions struct { + publicEndpoint bool +} + +// WaitReadyOption customizes what WaitReady treats as ready. +type WaitReadyOption func(*waitReadyOptions) + +// WithPublicEndpoint makes WaitReady additionally require a non-empty +// status.publicURL. The controller can flip Ready=True BEFORE it has finished +// creating the public Service/Ingress and written status.publicURL, so returning +// on Ready alone would hand the caller an empty base URL. +// +// Named after the endpoint, not "published": in this codebase "published" already +// means an atomically committed on-disk artifact (archive.PublicationPublished). +func WithPublicEndpoint() WaitReadyOption { + return func(o *waitReadyOptions) { + o.publicEndpoint = true + } +} + // WaitReady polls the DataExport named deName until: -// - its Ready condition is True and Status.URL is populated → returns the DE, +// - its Ready condition is True and its base URL (status.url, or +// status.publicURL when WithPublicEndpoint is passed) is populated → returns the DE, // - it is Ready=False with reason Expired → returns a wrapped ErrExpired, // - ctx is cancelled or its deadline is exceeded → returns a wrapped ctx.Err() // that includes the last observed DataExport status and an inspection hint. @@ -682,9 +883,18 @@ func WaitReady( log *slog.Logger, namespace, deName string, + opts ...WaitReadyOption, ) (*deapi.DataExport, error) { + var o waitReadyOptions + + for _, opt := range opts { + opt(&o) + } + var lastStatus string + var lastPublicURL string + for attempt := 0; ; attempt++ { de := new(deapi.DataExport) @@ -696,7 +906,10 @@ func WaitReady( return nil, fmt.Errorf("DataExport %s/%s: %w", namespace, deName, ErrExpired) } - if de.Status.URL != "" { + lastPublicURL = de.Status.PublicURL + + hasURL := exportBaseURL(de, o.publicEndpoint) != "" + if hasURL { for _, cond := range de.Status.Conditions { if cond.Type == "Ready" && cond.Status == metav1.ConditionTrue { return de, nil @@ -704,7 +917,7 @@ func WaitReady( } } - lastStatus = readyConditionStatus(de.Status.Conditions, de.Status.URL != "") + lastStatus = readyConditionStatus(de.Status.Conditions, hasURL) if attempt == 0 || attempt%logEveryN == 0 { attrs := make([]slog.Attr, 0, 5) @@ -724,9 +937,16 @@ func WaitReady( select { case <-ctx.Done(): + hint := "" + if o.publicEndpoint && lastPublicURL == "" { + hint = "\n\nspec.publish is set but status.publicURL is still empty: the storage-foundation " + + "controller has not finished creating the public Service/Ingress. Check the module's " + + "ingress configuration, or rerun without --publish to use the in-cluster endpoint." + } + return nil, fmt.Errorf( - "%w; DataExport status: %s\n\nTo inspect DataExport status, run:\n d8 k -n %s get dataexport %s -o yaml", - ctx.Err(), lastStatus, namespace, deName, + "%w; DataExport status: %s\n\nTo inspect DataExport status, run:\n d8 k -n %s get dataexport %s -o yaml%s", + ctx.Err(), lastStatus, namespace, deName, hint, ) case <-time.After(3 * time.Second): } diff --git a/internal/snapshot/exporter/dataexport_test.go b/internal/snapshot/exporter/dataexport_test.go index e188d8184..3bc72e567 100644 --- a/internal/snapshot/exporter/dataexport_test.go +++ b/internal/snapshot/exporter/dataexport_test.go @@ -2266,3 +2266,782 @@ func TestEnsureDataExport_AlreadyExistsUIDMismatchNeverAcquires(t *testing.T) { assert.Equal(t, types.UID("uid-race-winner"), preserved.UID) assert.Equal(t, "uid-other-snapshot", preserved.Annotations[targetUIDAnnotationKey]) } + +// --------------------------------------------------------------------------- +// --publish tests +// --------------------------------------------------------------------------- + +// TestEnsureDataExport_SetsSpecPublish verifies WithPublish is always reflected +// explicitly in spec.publish on a newly-created DataExport, including the false +// case: the CRD declares no default for spec.publish, and alignDataExportPublish +// compares this field against the server's returned value, so an implicit +// zero-value would make that comparison depend on something never actually sent. +// The JSON assertion (not just the Go struct comparison) matters because the fake +// client does not prune zero-value fields the way pruning-aware assertions might +// hide a missing key. +func TestEnsureDataExport_SetsSpecPublish(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + publishOpt *bool + wantJSON string + }{ + { + name: "success: publish=true is set explicitly", + publishOpt: boolPtr(true), + wantJSON: `"publish":true`, + }, + { + name: "success: publish=false is set explicitly", + publishOpt: boolPtr(false), + wantJSON: `"publish":false`, + }, + { + name: "success: publish option omitted defaults to false and is still explicit", + publishOpt: nil, + wantJSON: `"publish":false`, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + c := fake.NewClientBuilder().WithScheme(newDEScheme(t)).Build() + + var opts []exporter.EnsureOption + if tt.publishOpt != nil { + opts = append(opts, exporter.WithPublish(*tt.publishOpt)) + } + + de, err := ensureDataExport(context.Background(), c, "test-ns", + aggapi.VolumeSnapshotGroup, aggapi.VolumeSnapshotResource, aggapi.VolumeSnapshotKind, + "publish-flag-vs", "1h", opts...) + require.NoError(t, err) + require.NotNil(t, de) + + want := tt.publishOpt != nil && *tt.publishOpt + assert.Equal(t, want, de.Spec.Publish) + + raw, marshalErr := json.Marshal(de.Spec) + require.NoError(t, marshalErr) + assert.Contains(t, string(raw), tt.wantJSON, + "marshaled spec must carry an explicit publish key even when false") + }) + } +} + +func boolPtr(b bool) *bool { return &b } + +// publishExistingDE builds a live (non-terminating, non-expired) DataExport CR +// this test suite's helpers can adopt, with the given publish value and owner. +func publishExistingDE(namespace, leafName string, publish bool, ownerRunID string, uid types.UID) *deapi.DataExport { + return &deapi.DataExport{ + ObjectMeta: metav1.ObjectMeta{ + Name: volumeSnapshotDataExportName(namespace, leafName), + Namespace: namespace, + UID: uid, + Annotations: targetAnnotations(ownerRunID), + }, + Spec: deapi.DataexportSpec{ + TTL: "1h", + Publish: publish, + TargetRef: volumeSnapshotTargetRef(leafName), + }, + } +} + +// TestEnsureDataExport_AlignsPublishOnAdoption verifies that adopting a live CR +// with spec.publish=false upgrades it to true in the cluster when this run's own +// WithPublish(true) is set. +func TestEnsureDataExport_AlignsPublishOnAdoption(t *testing.T) { + t.Parallel() + + const ( + namespace = "test-ns" + leafName = "align-publish-vs" + runID = "run-align-owner" + ) + + existing := publishExistingDE(namespace, leafName, false, runID, types.UID("uid-align")) + + c := fake.NewClientBuilder().WithScheme(newDEScheme(t)).WithObjects(existing).Build() + + got, err := ensureDataExport(context.Background(), c, namespace, + aggapi.VolumeSnapshotGroup, aggapi.VolumeSnapshotResource, aggapi.VolumeSnapshotKind, leafName, "1h", + exporter.WithRunOwner(runID, slog.Default()), + exporter.WithPublish(true)) + require.NoError(t, err) + require.NotNil(t, got) + assert.True(t, got.Spec.Publish, "the returned object must reflect the alignment") + + check := new(deapi.DataExport) + require.NoError(t, c.Get(context.Background(), types.NamespacedName{ + Namespace: namespace, Name: volumeSnapshotDataExportName(namespace, leafName), + }, check)) + assert.True(t, check.Spec.Publish, "spec.publish must be upgraded to true in the cluster") +} + +// TestEnsureDataExport_NeverDowngradesPublishOnAdoption verifies that an +// already-published CR is never downgraded back to false, whether WithPublish is +// omitted entirely or explicitly passed false, and that no write call happens +// either way — downgrading would make the storage-foundation controller tear +// down the public Service/Ingress out from under a concurrent reader. +func TestEnsureDataExport_NeverDowngradesPublishOnAdoption(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + opts []exporter.EnsureOption + }{ + {name: "success: WithPublish omitted leaves publish=true untouched"}, + {name: "success: WithPublish(false) leaves publish=true untouched", + opts: []exporter.EnsureOption{exporter.WithPublish(false)}}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + const ( + namespace = "test-ns" + runID = "run-no-downgrade" + ) + + leafName := "no-downgrade-" + strings.ReplaceAll(tt.name, " ", "-") + existing := publishExistingDE(namespace, leafName, true, runID, types.UID("uid-no-downgrade")) + + var patchCalls atomic.Int32 + + c := fake.NewClientBuilder().WithScheme(newDEScheme(t)).WithObjects(existing). + WithInterceptorFuncs(interceptor.Funcs{ + Patch: func(ctx context.Context, cl client.WithWatch, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { + patchCalls.Add(1) + + return cl.Patch(ctx, obj, patch, opts...) + }, + }).Build() + + opts := append([]exporter.EnsureOption{exporter.WithRunOwner(runID, slog.Default())}, tt.opts...) + + got, err := ensureDataExport(context.Background(), c, namespace, + aggapi.VolumeSnapshotGroup, aggapi.VolumeSnapshotResource, aggapi.VolumeSnapshotKind, leafName, "1h", + opts...) + require.NoError(t, err) + require.NotNil(t, got) + + assert.True(t, got.Spec.Publish, "publish must remain true") + assert.Equal(t, int32(0), patchCalls.Load(), "a no-op alignment must never issue a Patch") + }) + } +} + +// TestEnsureDataExport_PublishAlreadyAlignedIssuesNoPatch verifies idempotency: +// adopting a CR that already has publish=true with WithPublish(true) issues zero +// Patch calls. +func TestEnsureDataExport_PublishAlreadyAlignedIssuesNoPatch(t *testing.T) { + t.Parallel() + + const ( + namespace = "test-ns" + leafName = "already-aligned-vs" + runID = "run-already-aligned" + ) + + existing := publishExistingDE(namespace, leafName, true, runID, types.UID("uid-already-aligned")) + + var patchCalls atomic.Int32 + + c := fake.NewClientBuilder().WithScheme(newDEScheme(t)).WithObjects(existing). + WithInterceptorFuncs(interceptor.Funcs{ + Patch: func(ctx context.Context, cl client.WithWatch, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { + patchCalls.Add(1) + + return cl.Patch(ctx, obj, patch, opts...) + }, + }).Build() + + got, err := ensureDataExport(context.Background(), c, namespace, + aggapi.VolumeSnapshotGroup, aggapi.VolumeSnapshotResource, aggapi.VolumeSnapshotKind, leafName, "1h", + exporter.WithRunOwner(runID, slog.Default()), + exporter.WithPublish(true)) + require.NoError(t, err) + require.NotNil(t, got) + assert.True(t, got.Spec.Publish) + assert.Equal(t, int32(0), patchCalls.Load(), "an already-aligned CR must not be patched") +} + +// TestEnsureDataExport_DoesNotPatchForeignOwnedPublish verifies that when a +// foreign-owned CR still has publish=false, WithPublish(true) refuses to mutate +// it: adoption is read-only by contract, and enabling publish on another run's +// CR would be a write this run has no authority to make. +func TestEnsureDataExport_DoesNotPatchForeignOwnedPublish(t *testing.T) { + t.Parallel() + + const ( + namespace = "test-ns" + leafName = "foreign-publish-vs" + ownerRun = "run-foreign-owner" + adopterRun = "run-foreign-adopter" + ) + + existing := publishExistingDE(namespace, leafName, false, ownerRun, types.UID("uid-foreign-publish")) + + var patchCalls atomic.Int32 + + c := fake.NewClientBuilder().WithScheme(newDEScheme(t)).WithObjects(existing). + WithInterceptorFuncs(interceptor.Funcs{ + Patch: func(ctx context.Context, cl client.WithWatch, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { + patchCalls.Add(1) + + return cl.Patch(ctx, obj, patch, opts...) + }, + }).Build() + + got, err := ensureDataExport(context.Background(), c, namespace, + aggapi.VolumeSnapshotGroup, aggapi.VolumeSnapshotResource, aggapi.VolumeSnapshotKind, leafName, "1h", + exporter.WithRunOwner(adopterRun, slog.Default()), + exporter.WithPublish(true)) + require.Error(t, err) + assert.Nil(t, got) + assert.True(t, errors.Is(err, exporter.ErrPublishForeignOwner), + "error must wrap ErrPublishForeignOwner; got: %v", err) + assert.Equal(t, int32(0), patchCalls.Load(), "a foreign-owned CR must never be patched") + + check := new(deapi.DataExport) + require.NoError(t, c.Get(context.Background(), types.NamespacedName{ + Namespace: namespace, Name: volumeSnapshotDataExportName(namespace, leafName), + }, check)) + assert.False(t, check.Spec.Publish, "the foreign CR's publish must remain untouched") +} + +// TestEnsureDataExport_AdoptsForeignOwnedPublishWhenAlreadyEnabled verifies that +// a foreign-owned CR that ALREADY has publish=true is adopted without error and +// without a patch — there is nothing to align, so the foreign-owner guard never +// triggers. +func TestEnsureDataExport_AdoptsForeignOwnedPublishWhenAlreadyEnabled(t *testing.T) { + t.Parallel() + + const ( + namespace = "test-ns" + leafName = "foreign-already-published-vs" + ownerRun = "run-foreign-owner-2" + adopterRun = "run-foreign-adopter-2" + ) + + existing := publishExistingDE(namespace, leafName, true, ownerRun, types.UID("uid-foreign-already")) + + var patchCalls atomic.Int32 + + c := fake.NewClientBuilder().WithScheme(newDEScheme(t)).WithObjects(existing). + WithInterceptorFuncs(interceptor.Funcs{ + Patch: func(ctx context.Context, cl client.WithWatch, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { + patchCalls.Add(1) + + return cl.Patch(ctx, obj, patch, opts...) + }, + }).Build() + + got, err := ensureDataExport(context.Background(), c, namespace, + aggapi.VolumeSnapshotGroup, aggapi.VolumeSnapshotResource, aggapi.VolumeSnapshotKind, leafName, "1h", + exporter.WithRunOwner(adopterRun, slog.Default()), + exporter.WithPublish(true)) + require.NoError(t, err) + require.NotNil(t, got) + assert.True(t, got.Spec.Publish) + assert.Equal(t, int32(0), patchCalls.Load()) +} + +// TestEnsureDataExport_PublishPatchUsesOptimisticLock is the decisive regression +// test for the optimistic-lock requirement on alignDataExportPublish's patch: a +// concurrent writer bumps the object's resourceVersion between this run's +// identity validation and its Patch call. WITHOUT an optimistic lock, a +// MergeFrom(base) patch would silently drop the concurrent writer's edit +// (last-write-wins on the whole object diff). WITH the lock +// (MergeFromWithOptimisticLock), the fake client's Patch call must return a +// Conflict on the first attempt (proving the lock actually engaged), and +// retry.RetryOnConflict must re-Get and retry, converging on a second, successful +// Patch that lands publish=true. +func TestEnsureDataExport_PublishPatchUsesOptimisticLock(t *testing.T) { + t.Parallel() + + const ( + namespace = "test-ns" + leafName = "optimistic-lock-vs" + runID = "run-optimistic-lock" + ) + + existing := publishExistingDE(namespace, leafName, false, runID, types.UID("uid-optimistic-lock")) + deName := volumeSnapshotDataExportName(namespace, leafName) + + var ( + patchCalls atomic.Int32 + getCalls atomic.Int32 + bumpedOnce atomic.Bool + conflictSaw atomic.Bool + ) + + c := fake.NewClientBuilder().WithScheme(newDEScheme(t)).WithObjects(existing). + WithInterceptorFuncs(interceptor.Funcs{ + Get: func(ctx context.Context, cl client.WithWatch, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error { + if _, ok := obj.(*deapi.DataExport); ok && key.Name == deName { + getCalls.Add(1) + } + + return cl.Get(ctx, key, obj, opts...) + }, + Patch: func(ctx context.Context, cl client.WithWatch, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { + call := patchCalls.Add(1) + + // Simulate a concurrent writer landing a change AFTER this run + // captured its base but BEFORE this Patch call is applied: bump the + // stored object's resourceVersion out from under the in-flight patch, + // exactly once, on the first attempt only. + if call == 1 && bumpedOnce.CompareAndSwap(false, true) { + latest := new(deapi.DataExport) + if err := cl.Get(ctx, client.ObjectKey{Namespace: namespace, Name: deName}, latest); err != nil { + return fmt.Errorf("fetch latest before concurrent bump: %w", err) + } + + if latest.Annotations == nil { + latest.Annotations = map[string]string{} + } + + latest.Annotations["test.deckhouse.io/concurrent-writer"] = "true" + + if err := cl.Update(ctx, latest); err != nil { + return fmt.Errorf("apply concurrent bump: %w", err) + } + } + + err := cl.Patch(ctx, obj, patch, opts...) + if call == 1 && apierrors.IsConflict(err) { + conflictSaw.Store(true) + } + + return err + }, + }).Build() + + got, err := ensureDataExport(context.Background(), c, namespace, + aggapi.VolumeSnapshotGroup, aggapi.VolumeSnapshotResource, aggapi.VolumeSnapshotKind, leafName, "1h", + exporter.WithRunOwner(runID, slog.Default()), + exporter.WithPublish(true)) + require.NoError(t, err, "the second attempt (after re-Get) must succeed") + require.NotNil(t, got) + + assert.True(t, conflictSaw.Load(), + "the first Patch attempt must observe a Conflict from the optimistic lock; "+ + "if this fails, the lock did not engage (see this test's doc comment for the "+ + "temporary MergeFrom(base) swap used to verify the test itself catches the regression)") + assert.GreaterOrEqual(t, patchCalls.Load(), int32(2), "the conflict must be retried with a fresh Patch") + assert.GreaterOrEqual(t, getCalls.Load(), int32(1), "a re-Get must happen between the conflicting and retried Patch") + assert.True(t, got.Spec.Publish, "publish must end up true after the retry converges") + + check := new(deapi.DataExport) + require.NoError(t, c.Get(context.Background(), types.NamespacedName{Namespace: namespace, Name: deName}, check)) + assert.True(t, check.Spec.Publish) + assert.Equal(t, "true", check.Annotations["test.deckhouse.io/concurrent-writer"], + "the concurrent writer's own edit must survive (never silently dropped)") +} + +// TestEnsureDataExport_PublishPatchRetriesConflict verifies that a bounded +// number of Conflicts (below retry.DefaultRetry's budget) are transparently +// retried, each retry preceded by a re-Get, converging on success. +func TestEnsureDataExport_PublishPatchRetriesConflict(t *testing.T) { + t.Parallel() + + const ( + namespace = "test-ns" + leafName = "retries-conflict-vs" + runID = "run-retries-conflict" + conflictsN = 2 // less than retry.DefaultRetry's ~5 step budget + ) + + existing := publishExistingDE(namespace, leafName, false, runID, types.UID("uid-retries-conflict")) + deName := volumeSnapshotDataExportName(namespace, leafName) + + var ( + patchCalls atomic.Int32 + getCalls atomic.Int32 + ) + + c := fake.NewClientBuilder().WithScheme(newDEScheme(t)).WithObjects(existing). + WithInterceptorFuncs(interceptor.Funcs{ + Get: func(ctx context.Context, cl client.WithWatch, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error { + if _, ok := obj.(*deapi.DataExport); ok && key.Name == deName { + getCalls.Add(1) + } + + return cl.Get(ctx, key, obj, opts...) + }, + Patch: func(ctx context.Context, cl client.WithWatch, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { + call := patchCalls.Add(1) + if call <= conflictsN { + return apierrors.NewConflict( + schema.GroupResource{Group: "storage-foundation.deckhouse.io", Resource: "dataexports"}, + deName, errors.New("synthetic conflict")) + } + + return cl.Patch(ctx, obj, patch, opts...) + }, + }).Build() + + got, err := ensureDataExport(context.Background(), c, namespace, + aggapi.VolumeSnapshotGroup, aggapi.VolumeSnapshotResource, aggapi.VolumeSnapshotKind, leafName, "1h", + exporter.WithRunOwner(runID, slog.Default()), + exporter.WithPublish(true)) + require.NoError(t, err) + require.NotNil(t, got) + assert.True(t, got.Spec.Publish) + assert.Equal(t, int32(conflictsN+1), patchCalls.Load()) + assert.GreaterOrEqual(t, getCalls.Load(), int32(conflictsN), "each retry must re-Get before re-patching") +} + +// TestEnsureDataExport_PublishPatchGivesUpAfterConflictBudget verifies that once +// retry.DefaultRetry's bounded budget is exhausted, EnsureDataExport returns the +// Conflict as a classifiable error (apierrors.IsConflict) rather than looping +// forever or swallowing it. +func TestEnsureDataExport_PublishPatchGivesUpAfterConflictBudget(t *testing.T) { + t.Parallel() + + const ( + namespace = "test-ns" + leafName = "conflict-budget-vs" + runID = "run-conflict-budget" + ) + + existing := publishExistingDE(namespace, leafName, false, runID, types.UID("uid-conflict-budget")) + deName := volumeSnapshotDataExportName(namespace, leafName) + + var patchCalls atomic.Int32 + + c := fake.NewClientBuilder().WithScheme(newDEScheme(t)).WithObjects(existing). + WithInterceptorFuncs(interceptor.Funcs{ + Patch: func(ctx context.Context, cl client.WithWatch, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { + patchCalls.Add(1) + + return apierrors.NewConflict( + schema.GroupResource{Group: "storage-foundation.deckhouse.io", Resource: "dataexports"}, + deName, errors.New("perpetual synthetic conflict")) + }, + }).Build() + + got, err := ensureDataExport(context.Background(), c, namespace, + aggapi.VolumeSnapshotGroup, aggapi.VolumeSnapshotResource, aggapi.VolumeSnapshotKind, leafName, "1h", + exporter.WithRunOwner(runID, slog.Default()), + exporter.WithPublish(true)) + require.Error(t, err) + assert.Nil(t, got) + assert.True(t, apierrors.IsConflict(err), "the exhausted conflict must remain classifiable; got: %v", err) + + // A bounded, small number of attempts: proves this did not loop forever. + calls := patchCalls.Load() + assert.Greater(t, calls, int32(1), "at least one retry must have happened") + assert.Less(t, calls, int32(50), "the retry budget must be bounded, not an unbounded loop") +} + +// TestEnsureDataExport_PublishAlignmentRevalidatesIdentityAfterConflict verifies +// that after a Conflict forces a re-Get, EVERY identity check re-runs against the +// freshly fetched object: if the re-fetched object's target-UID annotation no +// longer matches (e.g. the Snapshot was deleted and recreated concurrently), the +// alignment must fail with ErrTargetUIDMismatch and must NOT patch publish=true +// onto an object that no longer belongs to this operation. +func TestEnsureDataExport_PublishAlignmentRevalidatesIdentityAfterConflict(t *testing.T) { + t.Parallel() + + const ( + namespace = "test-ns" + leafName = "revalidate-after-conflict-vs" + runID = "run-revalidate" + ) + + existing := publishExistingDE(namespace, leafName, false, runID, types.UID("uid-revalidate")) + deName := volumeSnapshotDataExportName(namespace, leafName) + + var patchCalls atomic.Int32 + + c := fake.NewClientBuilder().WithScheme(newDEScheme(t)).WithObjects(existing). + WithInterceptorFuncs(interceptor.Funcs{ + Patch: func(ctx context.Context, cl client.WithWatch, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { + call := patchCalls.Add(1) + + if call == 1 { + // Simulate the Snapshot being deleted and recreated concurrently: + // the DataExport's target-UID annotation changes underneath us. + latest := new(deapi.DataExport) + if err := cl.Get(ctx, client.ObjectKey{Namespace: namespace, Name: deName}, latest); err != nil { + return fmt.Errorf("fetch latest before uid swap: %w", err) + } + + latest.Annotations[targetUIDAnnotationKey] = "uid-recreated-snapshot" + if err := cl.Update(ctx, latest); err != nil { + return fmt.Errorf("apply uid swap: %w", err) + } + + return apierrors.NewConflict( + schema.GroupResource{Group: "storage-foundation.deckhouse.io", Resource: "dataexports"}, + deName, errors.New("synthetic conflict before uid swap")) + } + + return cl.Patch(ctx, obj, patch, opts...) + }, + }).Build() + + got, err := ensureDataExport(context.Background(), c, namespace, + aggapi.VolumeSnapshotGroup, aggapi.VolumeSnapshotResource, aggapi.VolumeSnapshotKind, leafName, "1h", + exporter.WithRunOwner(runID, slog.Default()), + exporter.WithPublish(true)) + require.Error(t, err) + assert.Nil(t, got) + assert.True(t, errors.Is(err, exporter.ErrTargetUIDMismatch), + "error must wrap ErrTargetUIDMismatch; got: %v", err) + + check := new(deapi.DataExport) + require.NoError(t, c.Get(context.Background(), types.NamespacedName{Namespace: namespace, Name: deName}, check)) + assert.False(t, check.Spec.Publish, "publish must not be patched onto an object whose identity changed") +} + +// TestEnsureDataExport_PublishAlignmentFallsThroughWhenObjectVanishes verifies +// that when the adopted CR vanishes (NotFound on re-Get after a Conflict), +// EnsureDataExport falls through to its create path instead of returning an +// error, and the freshly created CR carries publish=true. +func TestEnsureDataExport_PublishAlignmentFallsThroughWhenObjectVanishes(t *testing.T) { + t.Parallel() + + const ( + namespace = "test-ns" + leafName = "vanishes-during-align-vs" + runID = "run-vanishes" + ) + + existing := publishExistingDE(namespace, leafName, false, runID, types.UID("uid-vanishes")) + deName := volumeSnapshotDataExportName(namespace, leafName) + + var patchCalls atomic.Int32 + + c := fake.NewClientBuilder().WithScheme(newDEScheme(t)).WithObjects(existing). + WithInterceptorFuncs(interceptor.Funcs{ + Patch: func(ctx context.Context, cl client.WithWatch, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { + call := patchCalls.Add(1) + + if call == 1 { + // The object vanishes (e.g. deleted and GC'd) before the patch lands. + latest := new(deapi.DataExport) + if err := cl.Get(ctx, client.ObjectKey{Namespace: namespace, Name: deName}, latest); err != nil { + return fmt.Errorf("fetch latest before delete: %w", err) + } + + if err := cl.Delete(ctx, latest); err != nil { + return fmt.Errorf("delete before conflict: %w", err) + } + + return apierrors.NewConflict( + schema.GroupResource{Group: "storage-foundation.deckhouse.io", Resource: "dataexports"}, + deName, errors.New("synthetic conflict before vanish")) + } + + return cl.Patch(ctx, obj, patch, opts...) + }, + }).Build() + + got, err := ensureDataExport(context.Background(), c, namespace, + aggapi.VolumeSnapshotGroup, aggapi.VolumeSnapshotResource, aggapi.VolumeSnapshotKind, leafName, "1h", + exporter.WithRunOwner(runID, slog.Default()), + exporter.WithPublish(true)) + require.NoError(t, err, "a vanished adopted CR must fall through to the create path, not error") + require.NotNil(t, got) + assert.True(t, got.Spec.Publish, "the freshly created CR must carry publish=true") + assert.NotEqual(t, existing.UID, got.UID, "a genuinely new object must have been created") +} + +// --------------------------------------------------------------------------- +// WaitReady publish-endpoint tests +// --------------------------------------------------------------------------- + +// makePublishDE returns a DataExport with the given Ready status and URLs, named +// deterministically for leafName, for WaitReady publish-mode tests. +func makePublishDE(namespace, leafName, url, publicURL string, ready bool) *deapi.DataExport { + deName := volumeSnapshotDataExportName(namespace, leafName) + + status := metav1.ConditionFalse + if ready { + status = metav1.ConditionTrue + } + + return &deapi.DataExport{ + ObjectMeta: metav1.ObjectMeta{ + Name: deName, + Namespace: namespace, + }, + Status: deapi.DataExportStatus{ + URL: url, + PublicURL: publicURL, + Conditions: []metav1.Condition{ + {Type: "Ready", Status: status, Reason: "PodReady"}, + }, + }, + } +} + +// TestWaitReady_PublicEndpointRequiresPublicURL verifies that WithPublicEndpoint +// makes WaitReady require a non-empty status.publicURL even though Ready=True and +// status.url is already populated: the controller can flip Ready before it +// finishes wiring the public Service/Ingress, so Ready alone is not sufficient +// for the publish path. +func TestWaitReady_PublicEndpointRequiresPublicURL(t *testing.T) { + t.Parallel() + + const ( + namespace = "test-ns" + leafName = "public-url-required-vs" + ) + + de := makePublishDE(namespace, leafName, "https://internal.example.test", "", true) + c := fake.NewClientBuilder().WithScheme(newDEScheme(t)).WithObjects(de).WithStatusSubresource(de).Build() + + ctx, cancel := context.WithTimeout(context.Background(), 50*time.Millisecond) + defer cancel() + + _, err := exporter.WaitReady(ctx, c, slog.Default(), namespace, de.Name, exporter.WithPublicEndpoint()) + require.Error(t, err) + assert.True(t, errors.Is(err, context.DeadlineExceeded), + "empty status.publicURL under WithPublicEndpoint must time out; got: %v", err) +} + +// TestWaitReady_PublicEndpointUsesPublicURL verifies the happy path: Ready=True +// with a non-empty status.publicURL (path-prefixed) returns immediately under +// WithPublicEndpoint. +func TestWaitReady_PublicEndpointUsesPublicURL(t *testing.T) { + t.Parallel() + + const ( + namespace = "test-ns" + leafName = "public-url-present-vs" + publicURL = "https://api.example.test/test-ns/volumesnapshot/leaf-a/" + ) + + de := makePublishDE(namespace, leafName, "https://internal.example.test", publicURL, true) + c := fake.NewClientBuilder().WithScheme(newDEScheme(t)).WithObjects(de).WithStatusSubresource(de).Build() + + got, err := exporter.WaitReady(context.Background(), c, slog.Default(), namespace, de.Name, exporter.WithPublicEndpoint()) + require.NoError(t, err) + require.NotNil(t, got) + assert.Equal(t, publicURL, got.Status.PublicURL) +} + +// TestWaitReady_DefaultModeIgnoresPublicURL is the regression guard for the +// unchanged default (non-publish) contract: an empty status.publicURL must not +// block readiness when WithPublicEndpoint is not passed. +func TestWaitReady_DefaultModeIgnoresPublicURL(t *testing.T) { + t.Parallel() + + const ( + namespace = "test-ns" + leafName = "default-mode-ignores-public-vs" + ) + + de := makePublishDE(namespace, leafName, "https://internal.example.test", "", true) + c := fake.NewClientBuilder().WithScheme(newDEScheme(t)).WithObjects(de).WithStatusSubresource(de).Build() + + got, err := exporter.WaitReady(context.Background(), c, slog.Default(), namespace, de.Name) + require.NoError(t, err) + require.NotNil(t, got) + assert.Equal(t, "https://internal.example.test", got.Status.URL) +} + +// TestWaitReady_PublicEndpointTimeoutMessageMentionsPublicURL verifies the +// publish-specific diagnostic hint appears in the timeout error text. +func TestWaitReady_PublicEndpointTimeoutMessageMentionsPublicURL(t *testing.T) { + t.Parallel() + + const ( + namespace = "test-ns" + leafName = "public-url-hint-vs" + ) + + de := makePublishDE(namespace, leafName, "https://internal.example.test", "", true) + c := fake.NewClientBuilder().WithScheme(newDEScheme(t)).WithObjects(de).WithStatusSubresource(de).Build() + + ctx, cancel := context.WithTimeout(context.Background(), 50*time.Millisecond) + defer cancel() + + _, err := exporter.WaitReady(ctx, c, slog.Default(), namespace, de.Name, exporter.WithPublicEndpoint()) + require.Error(t, err) + assert.Contains(t, err.Error(), "status.publicURL") + assert.Contains(t, err.Error(), "rerun without --publish") +} + +// TestExportBaseURL is a direct table test on the exportBaseURL helper (internal, +// so this file must stay package exporter_test but the function is unexported — +// exercised only indirectly through WaitReady/OpenExport's observable outputs +// above; this test instead exercises it through the URL selection behavior +// visible via WaitReady's returned object, since exportBaseURL itself is +// unexported and not part of the package's public surface). +func TestExportBaseURL_ViaWaitReadySelection(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + url string + publicURL string + publicEndpoint bool + wantReady bool + }{ + { + name: "success: internal endpoint uses status.url", + url: "https://internal.example.test", + publicURL: "", + publicEndpoint: false, + wantReady: true, + }, + { + name: "success: public endpoint uses status.publicURL", + url: "https://internal.example.test", + publicURL: "https://api.example.test/ns/kind/leaf/", + publicEndpoint: true, + wantReady: true, + }, + { + name: "error: public endpoint with empty status.url is not ready by url alone", + url: "", + publicURL: "", + publicEndpoint: false, + wantReady: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + leafName := "base-url-select-" + strings.ReplaceAll(tt.name, " ", "-") + de := makePublishDE("test-ns", leafName, tt.url, tt.publicURL, true) + c := fake.NewClientBuilder().WithScheme(newDEScheme(t)).WithObjects(de).WithStatusSubresource(de).Build() + + var opts []exporter.WaitReadyOption + if tt.publicEndpoint { + opts = append(opts, exporter.WithPublicEndpoint()) + } + + ctx := context.Background() + if !tt.wantReady { + var cancel context.CancelFunc + ctx, cancel = context.WithTimeout(ctx, 50*time.Millisecond) + defer cancel() + } + + got, err := exporter.WaitReady(ctx, c, slog.Default(), "test-ns", de.Name, opts...) + if !tt.wantReady { + require.Error(t, err) + assert.Nil(t, got) + + return + } + + require.NoError(t, err) + require.NotNil(t, got) + }) + } +} diff --git a/internal/snapshot/exporter/export.go b/internal/snapshot/exporter/export.go index 821997d28..7ca2c65d2 100644 --- a/internal/snapshot/exporter/export.go +++ b/internal/snapshot/exporter/export.go @@ -134,62 +134,105 @@ func OpenExport( return nil, fmt.Errorf("ensure DataExport for leaf %q: %w", leafName, err) } - ready, err := WaitReady(ctx, c, log, namespace, de.Name) + var o ensureOptions + + for _, opt := range opts { + opt(&o) + } + + publicEndpoint := o.publish + + var waitOpts []WaitReadyOption + if publicEndpoint { + waitOpts = append(waitOpts, WithPublicEndpoint()) + } + + ready, err := WaitReady(ctx, c, log, namespace, de.Name, waitOpts...) if err != nil { return nil, fmt.Errorf("wait DataExport %q ready: %w", de.Name, err) } - dataHTTPClient, sourceHashHTTPClient, err := buildSubClients(sc, ready) + dataHTTPClient, sourceHashHTTPClient, err := buildSubClients(sc, ready, publicEndpoint) if err != nil { return nil, fmt.Errorf("build sub-clients for DataExport %q: %w", de.Name, err) } + var fetcherOpts []FetcherOption + + fetcherOpts = append(fetcherOpts, WithSourceHashDoer(sourceHashHTTPClient)) + if publicEndpoint { + fetcherOpts = append(fetcherOpts, WithPublishUnauthorizedHint()) + } + return NewExport( namespace, de.Name, ready.Status.VolumeMode, - ready.Status.URL, - NewFetcher(dataHTTPClient, WithSourceHashDoer(sourceHashHTTPClient)), + exportBaseURL(ready, publicEndpoint), + NewFetcher(dataHTTPClient, fetcherOpts...), dataHTTPClient, sourceHashHTTPClient, ), nil } // buildSubClients creates exactly two persistent, isolated HTTP clients pinned -// to the DataExport's internal HTTPS origin and status.ca. Ordinary data calls -// retain the short response-header timeout and progress-based body watchdog. -// Source-hash HEAD requests get a separate transport ceiling because the -// producer computes their response header by synchronously reading the complete -// file; SourceMD5 applies the tighter size-derived request deadline. +// to the DataExport's base URL (its internal HTTPS origin, or the public Ingress +// URL when publicEndpoint is true) and status.ca. Ordinary data calls retain the +// short response-header timeout and progress-based body watchdog. Source-hash +// HEAD requests get a separate transport ceiling because the producer computes +// their response header by synchronously reading the complete file; SourceMD5 +// applies the tighter size-derived request deadline. func buildSubClients( sc *transport.Client, de *deapi.DataExport, + publicEndpoint bool, ) (*transport.PersistentHTTPClient, *transport.PersistentHTTPClient, error) { + baseURL := exportBaseURL(de, publicEndpoint) + caBytes, err := base64.StdEncoding.DecodeString(de.Status.CA) if err != nil { return nil, nil, fmt.Errorf("decode DataExport status.ca: %w", err) } - if err := transport.ValidateHTTPSIdentity(de.Status.URL, caBytes); err != nil { - return nil, nil, fmt.Errorf("validate DataExport download identity: %w", err) + if publicEndpoint { + // The public endpoint is terminated by ingress-nginx with its own certificate, + // signed by a CA the exporter pod's status.ca knows nothing about. Pinning trust + // to status.ca (SetTLSIdentityCAData) would therefore always fail the handshake; + // the merged pool built by SetTLSCAData (system roots + kubeconfig CA + status.ca + // when present) is the correct trust source here. Verification stays MANDATORY: + // SetTLSCAData unconditionally clears Insecure and ServerName, so an + // insecure-skip-tls-verify kubeconfig cannot downgrade it. + if err := transport.ValidateHTTPSURL(baseURL); err != nil { + return nil, nil, fmt.Errorf("validate DataExport public download URL: %w", err) + } + } + + if !publicEndpoint || len(caBytes) > 0 { + if err := transport.ValidateHTTPSIdentity(baseURL, caBytes); err != nil { + return nil, nil, fmt.Errorf("validate DataExport download identity: %w", err) + } } sub := sc.Copy() - if err := sub.SetTLSIdentityCAData(caBytes); err != nil { + if publicEndpoint { + sub.SetTLSCAData(caBytes) // returns nothing — do not wrap in `if err :=` + } else if err := sub.SetTLSIdentityCAData(caBytes); err != nil { return nil, nil, fmt.Errorf("configure ordinary data TLS identity: %w", err) } sub.SetResponseHeaderTimeout(dataPlaneResponseHeaderTimeout) - dataHTTPClient, err := sub.NewPersistentHTTPSClientForOrigin(de.Status.URL) + dataHTTPClient, err := sub.NewPersistentHTTPSClientForOrigin(baseURL) if err != nil { return nil, nil, fmt.Errorf("build ordinary data HTTP client: %w", err) } sourceHashSub := sc.Copy() - if err := sourceHashSub.SetTLSIdentityCAData(caBytes); err != nil { + if publicEndpoint { + sourceHashSub.SetTLSCAData(caBytes) // returns nothing — do not wrap in `if err :=` + } else if err := sourceHashSub.SetTLSIdentityCAData(caBytes); err != nil { dataHTTPClient.CloseIdleConnections() return nil, nil, fmt.Errorf("configure source-hash TLS identity: %w", err) @@ -197,7 +240,7 @@ func buildSubClients( sourceHashSub.SetResponseHeaderTimeout(sourceHashTimeoutCeiling) - sourceHashHTTPClient, err := sourceHashSub.NewPersistentHTTPSClientForOrigin(de.Status.URL) + sourceHashHTTPClient, err := sourceHashSub.NewPersistentHTTPSClientForOrigin(baseURL) if err != nil { dataHTTPClient.CloseIdleConnections() diff --git a/internal/snapshot/exporter/export_test.go b/internal/snapshot/exporter/export_test.go index c6d78040d..9f9c432e2 100644 --- a/internal/snapshot/exporter/export_test.go +++ b/internal/snapshot/exporter/export_test.go @@ -29,6 +29,7 @@ import ( "errors" "fmt" "io" + "log/slog" "math/big" "net" "net/http" @@ -43,8 +44,15 @@ import ( "time" "github.com/spf13/pflag" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client/fake" deapi "github.com/deckhouse/deckhouse-cli/internal/data/dataexport/api/v1alpha1" + "github.com/deckhouse/deckhouse-cli/internal/snapshot/aggapi" "github.com/deckhouse/deckhouse-cli/internal/snapshot/transport" ) @@ -123,7 +131,7 @@ func TestBuildSubClients_IsolatesConcurrentExportCAs(t *testing.T) { go func() { defer wg.Done() - dataHTTPClient, sourceHashHTTPClient, err := buildSubClients(sc, exports[index]) + dataHTTPClient, sourceHashHTTPClient, err := buildSubClients(sc, exports[index], false) if err != nil { errs <- fmt.Errorf("build client pair %d: %w", index, err) @@ -207,7 +215,7 @@ func TestBuildSubClients_RejectsInvalidPublishedIdentity(t *testing.T) { URL: tc.rawURL, CA: tc.ca, }, - }) + }, false) if dataHTTPClient != nil { dataHTTPClient.CloseIdleConnections() } @@ -288,7 +296,7 @@ func TestBuildSubClients_RejectsWrongCAAndSANBeforeHTTPAuth(t *testing.T) { URL: tc.rawURL, CA: tc.ca, }, - }) + }, false) if err != nil { t.Fatalf("buildSubClients: %v", err) } @@ -356,7 +364,7 @@ func TestBuildSubClients_BindsBothClientsToPublishedOrigin(t *testing.T) { URL: source.URL, CA: encodedServerCA(t, source), }, - }) + }, false) if err != nil { t.Fatalf("buildSubClients: %v", err) } @@ -420,6 +428,560 @@ func (c *countingIdleCloser) CloseIdleConnections() { c.calls.Add(1) } +// --------------------------------------------------------------------------- +// buildSubClients / OpenExport publish-mode tests +// --------------------------------------------------------------------------- + +func newExportTestScheme(t *testing.T) *runtime.Scheme { + t.Helper() + + scheme := runtime.NewScheme() + require.NoError(t, deapi.AddToScheme(scheme)) + + return scheme +} + +func TestBuildSubClients_PublishUsesPublicURLAndMergedTrust(t *testing.T) { + t.Parallel() + + server := newTLSServer(t) + + sc, err := transport.NewClient(newTestKubeconfigFlags(t)) + require.NoError(t, err) + + dataHTTPClient, sourceHashHTTPClient, err := buildSubClients(sc, &deapi.DataExport{ + Status: deapi.DataExportStatus{ + PublicURL: server.URL, + CA: encodedServerCA(t, server), + }, + }, true) + require.NoError(t, err) + defer dataHTTPClient.CloseIdleConnections() + defer sourceHashHTTPClient.CloseIdleConnections() + + requestAndClose(t, dataHTTPClient, http.MethodGet, server.URL) + requestAndClose(t, sourceHashHTTPClient, http.MethodHead, server.URL) +} + +func TestBuildSubClients_PublishStillVerifiesCertificate(t *testing.T) { + t.Parallel() + + server := newTLSServer(t) + unrelated := newTLSServer(t) + + sc, err := transport.NewClient(newTestKubeconfigFlags(t)) + require.NoError(t, err) + + dataHTTPClient, sourceHashHTTPClient, err := buildSubClients(sc, &deapi.DataExport{ + Status: deapi.DataExportStatus{ + PublicURL: server.URL, + // CA belongs to an unrelated server: the real server's certificate must + // not verify against it. + CA: encodedServerCA(t, unrelated), + }, + }, true) + require.NoError(t, err) + defer dataHTTPClient.CloseIdleConnections() + defer sourceHashHTTPClient.CloseIdleConnections() + + assertTLSFailure(t, dataHTTPClient, http.MethodGet, server.URL, &x509.UnknownAuthorityError{}) +} + +// TestBuildSubClients_PublishIgnoresInsecureSkipTLSVerifyFromKubeconfig is the +// regression guard for the P1 fix landed in cf7d998f7 (originally on the upload +// side): an insecure-skip-tls-verify: true kubeconfig must NOT downgrade +// certificate verification on the publish download path either. SetTLSCAData +// unconditionally clears Insecure/ServerName on both the rest.Config and the +// materialized transport, so a client built from such a kubeconfig must still +// reject an untrusted certificate. +func TestBuildSubClients_PublishIgnoresInsecureSkipTLSVerifyFromKubeconfig(t *testing.T) { + t.Parallel() + + server := newTLSServer(t) + unrelated := newTLSServer(t) + + flags := newInsecureSkipTLSVerifyKubeconfigFlags(t) + + sc, err := transport.NewClient(flags) + require.NoError(t, err) + + dataHTTPClient, sourceHashHTTPClient, err := buildSubClients(sc, &deapi.DataExport{ + Status: deapi.DataExportStatus{ + PublicURL: server.URL, + CA: encodedServerCA(t, unrelated), + }, + }, true) + require.NoError(t, err) + defer dataHTTPClient.CloseIdleConnections() + defer sourceHashHTTPClient.CloseIdleConnections() + + assertTLSFailure(t, dataHTTPClient, http.MethodGet, server.URL, &x509.UnknownAuthorityError{}) + assertTLSFailure(t, sourceHashHTTPClient, http.MethodHead, server.URL, &x509.UnknownAuthorityError{}) +} + +// newInsecureSkipTLSVerifyKubeconfigFlags builds a --kubeconfig flag pointing at +// a kubeconfig whose cluster entry sets insecure-skip-tls-verify: true, so tests +// can assert that flag never reaches the publish-path transport. +func newInsecureSkipTLSVerifyKubeconfigFlags(t *testing.T) *pflag.FlagSet { + t.Helper() + + kubeconfigPath := filepath.Join(t.TempDir(), "config") + kubeconfig := []byte(`apiVersion: v1 +kind: Config +clusters: +- name: default + cluster: + server: https://test.invalid + insecure-skip-tls-verify: true +contexts: +- name: default + context: + cluster: default +current-context: default +`) + require.NoError(t, os.WriteFile(kubeconfigPath, kubeconfig, 0o600)) + + flags := pflag.NewFlagSet("test", pflag.ContinueOnError) + flags.String("kubeconfig", "", "") + require.NoError(t, flags.Set("kubeconfig", kubeconfigPath)) + + return flags +} + +func TestBuildSubClients_PublishRejectsNonHTTPSPublicURL(t *testing.T) { + t.Parallel() + + server := newTLSServer(t) + + tests := []struct { + name string + publicURL string + }{ + {name: "error: plaintext scheme", publicURL: strings.Replace(server.URL, "https://", "http://", 1)}, + {name: "error: empty URL", publicURL: ""}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + sc, err := transport.NewClient(newTestKubeconfigFlags(t)) + require.NoError(t, err) + + dataHTTPClient, sourceHashHTTPClient, err := buildSubClients(sc, &deapi.DataExport{ + Status: deapi.DataExportStatus{ + PublicURL: tt.publicURL, + CA: encodedServerCA(t, server), + }, + }, true) + if dataHTTPClient != nil { + dataHTTPClient.CloseIdleConnections() + } + if sourceHashHTTPClient != nil { + sourceHashHTTPClient.CloseIdleConnections() + } + require.Error(t, err) + }) + } +} + +func TestBuildSubClients_PublishRejectsUnparseableCA(t *testing.T) { + t.Parallel() + + server := newTLSServer(t) + + tests := []struct { + name string + ca string + }{ + {name: "error: malformed base64 CA", ca: "%%%"}, + {name: "error: malformed PEM CA", ca: base64.StdEncoding.EncodeToString([]byte("not PEM"))}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + sc, err := transport.NewClient(newTestKubeconfigFlags(t)) + require.NoError(t, err) + + dataHTTPClient, sourceHashHTTPClient, err := buildSubClients(sc, &deapi.DataExport{ + Status: deapi.DataExportStatus{ + PublicURL: server.URL, + CA: tt.ca, + }, + }, true) + if dataHTTPClient != nil { + dataHTTPClient.CloseIdleConnections() + } + if sourceHashHTTPClient != nil { + sourceHashHTTPClient.CloseIdleConnections() + } + require.Error(t, err) + }) + } +} + +func TestBuildSubClients_PublishBindsBothClientsToPublicOrigin(t *testing.T) { + t.Parallel() + + const authorization = "Bearer publish-credential" + + var ( + authFailures atomic.Int64 + targetRequests atomic.Int64 + ) + + target := newTLSServerWithIdentity(t, func(w http.ResponseWriter, _ *http.Request) { + targetRequests.Add(1) + w.WriteHeader(http.StatusNoContent) + }, []net.IP{net.ParseIP("127.0.0.1")}, nil) + + source := newTLSServerWithIdentity(t, func(w http.ResponseWriter, r *http.Request) { + if r.Header.Get("Authorization") != authorization { + authFailures.Add(1) + } + + switch r.URL.Path { + case "/ok": + w.WriteHeader(http.StatusNoContent) + case "/same-origin": + http.Redirect(w, r, "/ok", http.StatusTemporaryRedirect) + case "/cross-origin": + http.Redirect(w, r, target.URL, http.StatusTemporaryRedirect) + default: + http.NotFound(w, r) + } + }, []net.IP{net.ParseIP("127.0.0.1")}, nil) + + sc, err := transport.NewClient(newTestKubeconfigFlags(t)) + require.NoError(t, err) + + dataHTTPClient, sourceHashHTTPClient, err := buildSubClients(sc, &deapi.DataExport{ + Status: deapi.DataExportStatus{ + PublicURL: source.URL, + CA: encodedServerCA(t, source), + }, + }, true) + require.NoError(t, err) + defer dataHTTPClient.CloseIdleConnections() + defer sourceHashHTTPClient.CloseIdleConnections() + + clients := []struct { + name string + method string + client *transport.PersistentHTTPClient + }{ + {name: "ordinary data", method: http.MethodGet, client: dataHTTPClient}, + {name: "source hash", method: http.MethodHead, client: sourceHashHTTPClient}, + } + + for _, tc := range clients { + requestWithAuthAndClose(t, tc.client, tc.method, source.URL+"/same-origin", authorization) + assertAuthenticatedRequestFailure(t, tc.client, tc.method, source.URL+"/cross-origin", authorization) + assertAuthenticatedRequestFailure(t, tc.client, tc.method, target.URL, authorization) + } + + assert.Zero(t, authFailures.Load(), "same-origin requests without authorization must not occur") + assert.Zero(t, targetRequests.Load(), "cross-origin target requests must never be sent") +} + +// TestBuildSubClients_PublishPreservesResponseHeaderTimeouts asserts that the +// publish path still produces two DISTINCT, independently usable persistent HTTP +// clients (the ordinary data client and the source-hash client), each carrying +// its own response-header-timeout configuration exactly as the non-publish path +// does (dataPlaneResponseHeaderTimeout vs sourceHashTimeoutCeiling) — the two +// underlying rest.Config WrapTransport chains are not the same object, and both +// remain independently functional. +func TestBuildSubClients_PublishPreservesResponseHeaderTimeouts(t *testing.T) { + t.Parallel() + + server := newTLSServer(t) + + sc, err := transport.NewClient(newTestKubeconfigFlags(t)) + require.NoError(t, err) + + dataHTTPClient, sourceHashHTTPClient, err := buildSubClients(sc, &deapi.DataExport{ + Status: deapi.DataExportStatus{ + PublicURL: server.URL, + CA: encodedServerCA(t, server), + }, + }, true) + require.NoError(t, err) + defer dataHTTPClient.CloseIdleConnections() + defer sourceHashHTTPClient.CloseIdleConnections() + + assert.NotSame(t, dataHTTPClient, sourceHashHTTPClient, + "the ordinary data and source-hash clients must be distinct instances so each keeps its own timeout") + + requestAndClose(t, dataHTTPClient, http.MethodGet, server.URL) + requestAndClose(t, sourceHashHTTPClient, http.MethodHead, server.URL) +} + +// TestBuildSubClients_NonPublishUnchanged is the regression guard proving the +// publicEndpoint parameter addition left the !publicEndpoint path byte-for-byte +// equivalent: identity is pinned to status.ca (SetTLSIdentityCAData) rather than +// merged trust, and status.URL (not status.publicURL) is used as the origin. +func TestBuildSubClients_NonPublishUnchanged(t *testing.T) { + t.Parallel() + + server := newTLSServer(t) + unrelated := newTLSServer(t) + + sc, err := transport.NewClient(newTestKubeconfigFlags(t)) + require.NoError(t, err) + + // PublicURL deliberately left empty and pointed nowhere reachable: the + // non-publish path must never consult it. + dataHTTPClient, sourceHashHTTPClient, err := buildSubClients(sc, &deapi.DataExport{ + Status: deapi.DataExportStatus{ + URL: server.URL, + PublicURL: "http://unreachable.invalid", + CA: encodedServerCA(t, server), + }, + }, false) + require.NoError(t, err) + defer dataHTTPClient.CloseIdleConnections() + defer sourceHashHTTPClient.CloseIdleConnections() + + requestAndClose(t, dataHTTPClient, http.MethodGet, server.URL) + + // Wrong CA under pinned identity must still fail closed, exactly as before. + dataHTTPClient2, sourceHashHTTPClient2, err := buildSubClients(sc, &deapi.DataExport{ + Status: deapi.DataExportStatus{ + URL: server.URL, + CA: encodedServerCA(t, unrelated), + }, + }, false) + require.NoError(t, err) + defer dataHTTPClient2.CloseIdleConnections() + defer sourceHashHTTPClient2.CloseIdleConnections() + + assertTLSFailure(t, dataHTTPClient2, http.MethodGet, server.URL, &x509.UnknownAuthorityError{}) +} + +// publishOpenExportDE builds a Ready DataExport whose status carries both an +// (unreachable) internal URL and a path-prefixed public URL pointing at a real +// TLS test server, for OpenExport publish-mode tests. +func publishOpenExportDE( + namespace, deName, publicURL, ca string, + targetUID types.UID, + group, resource, kind, leafName string, +) *deapi.DataExport { + return &deapi.DataExport{ + ObjectMeta: metav1.ObjectMeta{ + Name: deName, + Namespace: namespace, + Annotations: map[string]string{ + targetUIDAnnotation: string(targetUID), + }, + }, + Spec: deapi.DataexportSpec{ + TTL: "1h", + Publish: true, + TargetRef: deapi.TargetRefSpec{ + Group: group, + Resource: resource, + Kind: kind, + Name: leafName, + }, + }, + Status: deapi.DataExportStatus{ + URL: "https://internal.invalid", + PublicURL: publicURL, + CA: ca, + VolumeMode: "Block", + Conditions: []metav1.Condition{ + {Type: "Ready", Status: metav1.ConditionTrue, Reason: "PodReady"}, + }, + }, + } +} + +// TestOpenExport_PublishBaseURLKeepsIngressPathPrefix is the regression guard for +// the most likely silent bug in the publish path: OpenExport must build its +// Export (and therefore every block/file request URL derived from it) from +// exportBaseURL(ready, true) — the PATH-PREFIXED public URL — not the bare +// status.url. Losing that prefix would make every subsequent data request 404 +// against the Ingress. This is proven on the wire: the test TLS server only +// answers 200 at the exact prefixed path and 404 everywhere else. +func TestOpenExport_PublishBaseURLKeepsIngressPathPrefix(t *testing.T) { + t.Parallel() + + const ( + namespace = "ns1" + leafName = "leaf-a" + group = aggapi.VolumeSnapshotGroup + resource = aggapi.VolumeSnapshotResource + kind = aggapi.VolumeSnapshotKind + blockSize = 42 + ) + + targetUID := types.UID("uid-prefix-guard") + + const prefixPath = "/" + namespace + "/volumesnapshot/" + leafName + "/" + + server := newTLSServerWithIdentity(t, func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != prefixPath+"api/v1/block" { + http.NotFound(w, r) + + return + } + + w.Header().Set("Content-Length", fmt.Sprint(blockSize)) + w.WriteHeader(http.StatusOK) + }, []net.IP{net.ParseIP("127.0.0.1")}, nil) + + publicURL := server.URL + prefixPath + + scheme := newExportTestScheme(t) + deName := DataExportName(namespace, group, resource, kind, leafName, targetUID) + de := publishOpenExportDE(namespace, deName, publicURL, encodedServerCA(t, server), targetUID, group, resource, kind, leafName) + + c := fake.NewClientBuilder().WithScheme(scheme).WithObjects(de).WithStatusSubresource(de).Build() + + sc, err := transport.NewClient(newTestKubeconfigFlags(t)) + require.NoError(t, err) + + export, err := OpenExport( + context.Background(), + slog.New(slog.NewTextHandler(io.Discard, nil)), + c, + namespace, + group, + resource, + kind, + leafName, + "1h", + sc, + WithTargetUID(targetUID), + WithPublish(true), + ) + require.NoError(t, err) + require.NotNil(t, export) + defer export.CloseIdleConnections() + + assert.Equal(t, publicURL, export.BaseURL(), + "the Export base URL must be the path-prefixed public URL, not the bare origin") + + blockURL, err := BlockURL(export.BaseURL()) + require.NoError(t, err) + assert.Equal(t, publicURL+"api/v1/block", blockURL) + + size, err := export.Fetcher().HeadVolume(context.Background(), blockURL) + require.NoError(t, err, "the prefixed URL must actually reach the server on the wire") + assert.Equal(t, int64(blockSize), size) +} + +// TestOpenExport_PublishDerivedFromEnsureOptions verifies OpenExport derives its +// publish/publicEndpoint decision from WithPublish uniformly for BOTH WaitReady +// (readiness gate) and buildSubClients (transport/base-URL selection): there is +// no way for the two to desync. With WithPublish(true) and an empty +// status.publicURL, OpenExport must NOT succeed using status.url — it must time +// out waiting for publicURL. Without WithPublish, the very same DataExport +// (status.url populated, publicURL empty) succeeds immediately. +func TestOpenExport_PublishDerivedFromEnsureOptions(t *testing.T) { + t.Parallel() + + const ( + namespace = "ns2" + leafName = "leaf-b" + group = aggapi.VolumeSnapshotGroup + resource = aggapi.VolumeSnapshotResource + kind = aggapi.VolumeSnapshotKind + ) + + targetUID := types.UID("uid-derive-guard") + server := newTLSServer(t) + + scheme := newExportTestScheme(t) + deName := DataExportName(namespace, group, resource, kind, leafName, targetUID) + + buildDE := func() *deapi.DataExport { + return &deapi.DataExport{ + ObjectMeta: metav1.ObjectMeta{ + Name: deName, + Namespace: namespace, + Annotations: map[string]string{ + targetUIDAnnotation: string(targetUID), + }, + }, + Spec: deapi.DataexportSpec{ + TTL: "1h", + TargetRef: deapi.TargetRefSpec{ + Group: group, Resource: resource, Kind: kind, Name: leafName, + }, + }, + Status: deapi.DataExportStatus{ + URL: server.URL, + PublicURL: "", + CA: encodedServerCA(t, server), + VolumeMode: "Block", + Conditions: []metav1.Condition{ + {Type: "Ready", Status: metav1.ConditionTrue, Reason: "PodReady"}, + }, + }, + } + } + + sc, err := transport.NewClient(newTestKubeconfigFlags(t)) + require.NoError(t, err) + + t.Run("error: publish requested but publicURL empty times out", func(t *testing.T) { + t.Parallel() + + de := buildDE() + de.Name = deName + "-publish" + c := fake.NewClientBuilder().WithScheme(scheme).WithObjects(de).WithStatusSubresource(de).Build() + + ctx, cancel := context.WithTimeout(context.Background(), 50*time.Millisecond) + defer cancel() + + _, err := OpenExport( + ctx, + slog.New(slog.NewTextHandler(io.Discard, nil)), + c, + namespace, + group, + resource, + kind, + leafName, + "1h", + sc, + WithTargetUID(targetUID), + WithPublish(true), + ) + require.Error(t, err) + assert.True(t, errors.Is(err, context.DeadlineExceeded), "got: %v", err) + }) + + t.Run("success: publish not requested uses status.url immediately", func(t *testing.T) { + t.Parallel() + + de := buildDE() + c := fake.NewClientBuilder().WithScheme(scheme).WithObjects(de).WithStatusSubresource(de).Build() + + export, err := OpenExport( + context.Background(), + slog.New(slog.NewTextHandler(io.Discard, nil)), + c, + namespace, + group, + resource, + kind, + leafName, + "1h", + sc, + WithTargetUID(targetUID), + ) + require.NoError(t, err) + require.NotNil(t, export) + defer export.CloseIdleConnections() + + assert.Equal(t, server.URL, export.BaseURL()) + }) +} + func newTLSServer(t *testing.T) *httptest.Server { t.Helper() diff --git a/internal/snapshot/exporter/http.go b/internal/snapshot/exporter/http.go index 2dfd8c9fb..ce42afe32 100644 --- a/internal/snapshot/exporter/http.go +++ b/internal/snapshot/exporter/http.go @@ -47,6 +47,24 @@ type Doer interface { // caller's intended offset. var ErrContentRangeMismatch = errors.New("server Content-Range does not match requested range") +// ErrExportUnauthorized classifies a 401/403 from the data-exporter endpoint. +// On the public (Ingress) path this is the expected outcome for a +// certificate-authenticated kubeconfig: Ingress terminates TLS with its own +// certificate and does not forward the client certificate to the exporter pod, so +// only bearer-token kubeconfigs authenticate through it. +var ErrExportUnauthorized = errors.New("data exporter rejected the request as unauthorized") + +// exportStatusError wraps err with ErrExportUnauthorized when the HTTP status +// indicates an authentication or authorization failure, so callers can classify it +// with errors.Is instead of matching on message text. +func exportStatusError(code int, err error) error { + if code == http.StatusUnauthorized || code == http.StatusForbidden { + return fmt.Errorf("%w: %w", ErrExportUnauthorized, err) + } + + return err +} + // ErrDataPlaneIdle is reported by a Fetcher-issued response body when no bytes // arrive for the configured idle window (see idleReadCloser). The data plane // deliberately runs without an overall request deadline — volume transfers are @@ -73,10 +91,11 @@ const ( // Fetcher wraps a Doer and exposes typed methods for the data-exporter HTTP API. type Fetcher struct { - doer Doer - sourceHashDoer Doer - idleTimeout time.Duration - sourceHashTimeout func(size int64) time.Duration + doer Doer + sourceHashDoer Doer + idleTimeout time.Duration + sourceHashTimeout func(size int64) time.Duration + publishUnauthorizedHint bool } // FetcherOption customizes a Fetcher at construction time. @@ -100,6 +119,13 @@ func WithSourceHashDoer(doer Doer) FetcherOption { } } +// WithPublishUnauthorizedHint makes this Fetcher append an actionable hint to +// ErrExportUnauthorized failures. Only the publish path sets it, so the in-cluster +// path never advertises an Ingress-specific remedy. +func WithPublishUnauthorizedHint() FetcherOption { + return func(f *Fetcher) { f.publishUnauthorizedHint = true } +} + // NewFetcher creates a Fetcher backed by the given Doer. Unless overridden via // WithIdleReadTimeout, response bodies carry an idle-read watchdog with // DefaultIdleReadTimeout. @@ -127,6 +153,23 @@ func (f *Fetcher) guardBody(ctx context.Context, body io.ReadCloser) io.ReadClos return newIdleReadCloser(ctx, body, f.idleTimeout) } +// hintPublishUnauthorized appends an actionable hint to err when it classifies as +// ErrExportUnauthorized and this Fetcher was built with WithPublishUnauthorizedHint; +// otherwise err is returned unchanged. +func (f *Fetcher) hintPublishUnauthorized(err error) error { + if err == nil { + return nil + } + + if !f.publishUnauthorizedHint || !errors.Is(err, ErrExportUnauthorized) { + return err + } + + return fmt.Errorf("%w; --publish streams through the Ingress endpoint, which only accepts "+ + "a bearer-token kubeconfig (a certificate-based kubeconfig is rejected). Rerun without "+ + "--publish to use the in-cluster endpoint.", err) +} + // BlockURL returns the block-volume endpoint for a DataExport base URL. // The block volume is served at api/v1/block. func BlockURL(baseURL string) (string, error) { @@ -163,7 +206,8 @@ func (f *Fetcher) HeadVolume(ctx context.Context, blockURL string) (int64, error defer func() { _ = resp.Body.Close() }() if resp.StatusCode != http.StatusOK { - return 0, fmt.Errorf("HEAD %s: unexpected status %s", blockURL, resp.Status) + statusErr := exportStatusError(resp.StatusCode, fmt.Errorf("HEAD %s: unexpected status %s", blockURL, resp.Status)) + return 0, f.hintPublishUnauthorized(statusErr) } if resp.ContentLength < 0 { @@ -197,7 +241,11 @@ func (f *Fetcher) RangeGet(ctx context.Context, blockURL string, start, end int6 if resp.StatusCode != http.StatusPartialContent { _ = resp.Body.Close() - return nil, fmt.Errorf("GET %s (range %d-%d): expected 206, got %s", blockURL, start, end, resp.Status) + + statusErr := exportStatusError(resp.StatusCode, + fmt.Errorf("GET %s (range %d-%d): expected 206, got %s", blockURL, start, end, resp.Status)) + + return nil, f.hintPublishUnauthorized(statusErr) } if err := validateContentRange(resp.Header.Get("Content-Range"), start, end); err != nil { @@ -314,7 +362,8 @@ func (f *Fetcher) ListDir(ctx context.Context, filesURL string, yield func(Item) defer func() { _ = resp.Body.Close() }() if resp.StatusCode != http.StatusOK { - return fmt.Errorf("GET %s: unexpected status %s", filesURL, resp.Status) + statusErr := exportStatusError(resp.StatusCode, fmt.Errorf("GET %s: unexpected status %s", filesURL, resp.Status)) + return f.hintPublishUnauthorized(statusErr) } // Guard the bounded listing body too: a stalled listing must not hang the @@ -370,7 +419,10 @@ func (f *Fetcher) SourceMD5(ctx context.Context, fileURL string, size int64) (st defer func() { _ = resp.Body.Close() }() if resp.StatusCode != http.StatusOK { - return "", fmt.Errorf("HEAD source hash for %s: unexpected status %s", fileURL, resp.Status) + statusErr := exportStatusError(resp.StatusCode, + fmt.Errorf("HEAD source hash for %s: unexpected status %s", fileURL, resp.Status)) + + return "", f.hintPublishUnauthorized(statusErr) } return resp.Header.Get(sourceHashHeader), nil @@ -414,7 +466,10 @@ func (f *Fetcher) GetFile(ctx context.Context, fileURL string) (io.ReadCloser, e if resp.StatusCode != http.StatusOK { _ = resp.Body.Close() - return nil, fmt.Errorf("GET %s: unexpected status %s", fileURL, resp.Status) + + statusErr := exportStatusError(resp.StatusCode, fmt.Errorf("GET %s: unexpected status %s", fileURL, resp.Status)) + + return nil, f.hintPublishUnauthorized(statusErr) } return f.guardBody(ctx, resp.Body), nil diff --git a/internal/snapshot/exporter/http_test.go b/internal/snapshot/exporter/http_test.go index 07645b91b..c7fa29b7b 100644 --- a/internal/snapshot/exporter/http_test.go +++ b/internal/snapshot/exporter/http_test.go @@ -1257,3 +1257,192 @@ func TestFilesURL(t *testing.T) { t.Errorf("FilesURL: got %q, want %q", got, want) } } + +// --------------------------------------------------------------------------- +// ErrExportUnauthorized classification and publish-hint tests +// --------------------------------------------------------------------------- + +func TestExportStatusError(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + code int + wantErr bool + }{ + {name: "error: 401 wraps ErrExportUnauthorized", code: http.StatusUnauthorized, wantErr: true}, + {name: "error: 403 wraps ErrExportUnauthorized", code: http.StatusForbidden, wantErr: true}, + {name: "success: 404 is returned unwrapped", code: http.StatusNotFound}, + {name: "success: 500 is returned unwrapped", code: http.StatusInternalServerError}, + {name: "success: 200 is returned unwrapped", code: http.StatusOK}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + base := errors.New("underlying status error") + got := exportStatusError(tt.code, base) + + if tt.wantErr { + if !errors.Is(got, ErrExportUnauthorized) { + t.Fatalf("exportStatusError(%d, ...) = %v, want wrapped ErrExportUnauthorized", tt.code, got) + } + + if !errors.Is(got, base) { + t.Fatalf("exportStatusError(%d, ...) lost the underlying error", tt.code) + } + + return + } + + if errors.Is(got, ErrExportUnauthorized) { + t.Fatalf("exportStatusError(%d, ...) unexpectedly wraps ErrExportUnauthorized", tt.code) + } + + if got != base { + t.Fatalf("exportStatusError(%d, ...) = %v, want the base error returned unchanged", tt.code, got) + } + }) + } +} + +// unauthorizedServer returns an httptest.Server that answers every request +// (whatever method or path) with a 401, for exercising every Fetcher method's +// unauthorized-classification path uniformly. +func unauthorizedServer(t *testing.T) *httptest.Server { + t.Helper() + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusUnauthorized) + })) + t.Cleanup(srv.Close) + + return srv +} + +// TestFetcher_ClassifiesUnauthorized verifies that all five Fetcher methods +// classify a 401 response as ErrExportUnauthorized. +func TestFetcher_ClassifiesUnauthorized(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + call func(f *Fetcher, ctx context.Context, srv *httptest.Server) error + }{ + { + name: "success: HeadVolume classifies 401", + call: func(f *Fetcher, ctx context.Context, srv *httptest.Server) error { + blockURL, err := BlockURL(srv.URL) + if err != nil { + return err + } + + _, err = f.HeadVolume(ctx, blockURL) + + return err + }, + }, + { + name: "success: RangeGet classifies 401", + call: func(f *Fetcher, ctx context.Context, srv *httptest.Server) error { + blockURL, err := BlockURL(srv.URL) + if err != nil { + return err + } + + _, err = f.RangeGet(ctx, blockURL, 0, 1) + + return err + }, + }, + { + name: "success: ListDir classifies 401", + call: func(f *Fetcher, ctx context.Context, srv *httptest.Server) error { + filesURL, err := FilesURL(srv.URL) + if err != nil { + return err + } + + return f.ListDir(ctx, filesURL, func(Item) error { return nil }) + }, + }, + { + name: "success: SourceMD5 classifies 401", + call: func(f *Fetcher, ctx context.Context, srv *httptest.Server) error { + _, err := f.SourceMD5(ctx, srv.URL+"/api/v1/files/data.txt", 4) + + return err + }, + }, + { + name: "success: GetFile classifies 401", + call: func(f *Fetcher, ctx context.Context, srv *httptest.Server) error { + _, err := f.GetFile(ctx, srv.URL+"/api/v1/files/data.txt") + + return err + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + srv := unauthorizedServer(t) + f := NewFetcher(srv.Client()) + + err := tt.call(f, context.Background(), srv) + if !errors.Is(err, ErrExportUnauthorized) { + t.Fatalf("error = %v, want wrapped ErrExportUnauthorized", err) + } + }) + } +} + +// TestFetcher_PublishHintOnlyWhenEnabled verifies the actionable bearer-token +// hint is appended to ErrExportUnauthorized only when the Fetcher was built with +// WithPublishUnauthorizedHint, and absent otherwise (the in-cluster path must +// never advertise an Ingress-specific remedy). +func TestFetcher_PublishHintOnlyWhenEnabled(t *testing.T) { + t.Parallel() + + const hintSubstring = "--publish streams through the Ingress endpoint" + + tests := []struct { + name string + opts []FetcherOption + wantHint bool + }{ + {name: "success: hint absent by default"}, + { + name: "success: hint present when WithPublishUnauthorizedHint is set", + opts: []FetcherOption{WithPublishUnauthorizedHint()}, + wantHint: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + srv := unauthorizedServer(t) + f := NewFetcher(srv.Client(), tt.opts...) + + blockURL, err := BlockURL(srv.URL) + if err != nil { + t.Fatalf("BlockURL: %v", err) + } + + _, err = f.HeadVolume(context.Background(), blockURL) + if !errors.Is(err, ErrExportUnauthorized) { + t.Fatalf("error = %v, want wrapped ErrExportUnauthorized", err) + } + + gotHint := strings.Contains(err.Error(), hintSubstring) + if gotHint != tt.wantHint { + t.Fatalf("hint present = %v, want %v (error: %v)", gotHint, tt.wantHint, err) + } + }) + } +} From 40d012d0b0616b9852146eec4b974ec266287732 Mon Sep 17 00:00:00 2001 From: Konstantin Kozoriz Date: Wed, 26 Aug 2026 21:13:38 +0300 Subject: [PATCH 06/13] feat(snapshot): thread publish through the download pipeline Signed-off-by: Konstantin Kozoriz --- internal/snapshot/pipeline/config.go | 9 ++ internal/snapshot/pipeline/config_test.go | 163 ++++++++++++++++++++++ 2 files changed, 172 insertions(+) create mode 100644 internal/snapshot/pipeline/config_test.go diff --git a/internal/snapshot/pipeline/config.go b/internal/snapshot/pipeline/config.go index 4afe77fe6..77b4cf2ef 100644 --- a/internal/snapshot/pipeline/config.go +++ b/internal/snapshot/pipeline/config.go @@ -101,6 +101,12 @@ type Config struct { // fresh RunID via crypto/rand when it is empty; tests may set it explicitly. RunID string + // Publish routes each leaf's volume bytes through the storage-foundation-published + // (Ingress) exporter endpoint (status.publicURL) instead of the in-cluster service + // (status.url). It is forwarded verbatim to exporter.WithPublish; the exporter + // derives both the DataExport spec value and the readiness/transport mode from it. + Publish bool + // KeepExports, when true, leaves the per-volume DataExport CR (and the // server-side export chain it owns: export VolumeSnapshot/VolumeSnapshotContent/ // export PVC) in the cluster after each volume stream completes, instead of @@ -273,6 +279,7 @@ func applyDefaults(cfg Config) Config { timeout := cfg.ReadinessTimeout aggClient := cfg.AggClient runID := cfg.RunID + publish := cfg.Publish cfg.OpenExportWithTargetAcquisition = func( ctx context.Context, @@ -289,6 +296,7 @@ func applyDefaults(cfg Config) Config { owner := exporter.WithRunOwner(runID, log) target := exporter.WithTargetUID(targetUID) termWait := exporter.WithTerminatingWaitTimeout(timeout) + publishOpt := exporter.WithPublish(publish) var acquisition *exporter.DataExportAcquisition @@ -309,6 +317,7 @@ func applyDefaults(cfg Config) Config { target, owner, termWait, + publishOpt, exporter.WithAcquisition(&acquisition), ) if openErr != nil { diff --git a/internal/snapshot/pipeline/config_test.go b/internal/snapshot/pipeline/config_test.go new file mode 100644 index 000000000..3c1d2da9a --- /dev/null +++ b/internal/snapshot/pipeline/config_test.go @@ -0,0 +1,163 @@ +/* +Copyright 2026 Flant JSC + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package pipeline + +import ( + "context" + "errors" + "os" + "path/filepath" + "testing" + "time" + + "github.com/spf13/pflag" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" + + deapi "github.com/deckhouse/deckhouse-cli/internal/data/dataexport/api/v1alpha1" + "github.com/deckhouse/deckhouse-cli/internal/snapshot/aggapi" + "github.com/deckhouse/deckhouse-cli/internal/snapshot/exporter" + "github.com/deckhouse/deckhouse-cli/internal/snapshot/transport" +) + +// newConfigTestKubeconfigFlags builds a --kubeconfig flag pointing at a +// throwaway kubeconfig fixture, mirroring the exporter package's test helper, +// so transport.NewClient never falls back to $HOME/.kube/config. +func newConfigTestKubeconfigFlags(t *testing.T) *pflag.FlagSet { + t.Helper() + + kubeconfigPath := filepath.Join(t.TempDir(), "config") + kubeconfig := []byte(`apiVersion: v1 +kind: Config +clusters: +- name: default + cluster: + server: https://test.invalid +contexts: +- name: default + context: + cluster: default +current-context: default +`) + require.NoError(t, os.WriteFile(kubeconfigPath, kubeconfig, 0o600)) + + flags := pflag.NewFlagSet("test", pflag.ContinueOnError) + flags.String("kubeconfig", "", "") + require.NoError(t, flags.Set("kubeconfig", kubeconfigPath)) + + return flags +} + +// TestApplyDefaults_ForwardsPublishToOpenExport verifies that Config.Publish is +// forwarded, unmodified, all the way through applyDefaults' generated +// OpenExportWithTargetAcquisition closure into exporter.WithPublish, by +// observing the OBSERVABLE effect: the DataExport EnsureDataExport creates +// carries spec.publish equal to Config.Publish. WaitReady is expected to time +// out (the fake DataExport never gets a Ready condition, since no controller is +// running against the fake client) — only the CREATED object's spec.publish is +// asserted, proving the option reached exporter.EnsureDataExport without needing +// a full working data-plane transport. +func TestApplyDefaults_ForwardsPublishToOpenExport(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + publish bool + }{ + {name: "success: publish=true is forwarded to the created DataExport", publish: true}, + {name: "success: publish=false is forwarded to the created DataExport", publish: false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + const ( + namespace = "forward-ns" + leafName = "forward-leaf" + runID = "run-forward-publish" + ) + + targetUID := types.UID("uid-forward-publish-" + tt.name) + + scheme := runtime.NewScheme() + require.NoError(t, deapi.AddToScheme(scheme)) + + // The fake client does not auto-assign a UID on Create, but + // EnsureDataExport's recordAcquisition requires a non-empty UID + // (mirroring what a real API server returns). Stamp one, matching the + // pattern already used for this in dataexport_test.go. + kubeClient := fake.NewClientBuilder().WithScheme(scheme). + WithInterceptorFuncs(interceptor.Funcs{ + Create: func(ctx context.Context, cl client.WithWatch, obj client.Object, opts ...client.CreateOption) error { + if de, ok := obj.(*deapi.DataExport); ok && de.UID == "" { + de.UID = types.UID("uid-" + tt.name) + } + + return cl.Create(ctx, obj, opts...) + }, + }).Build() + + sc, err := transport.NewClient(newConfigTestKubeconfigFlags(t)) + require.NoError(t, err) + + aggClient := aggapi.NewClient(nil, nil) + + cfg := applyDefaults(Config{ + KubeClient: kubeClient, + AggClient: aggClient, + TransportClient: sc, + RunID: runID, + Publish: tt.publish, + ReadinessTimeout: 30 * time.Millisecond, + }) + + require.NotNil(t, cfg.OpenExportWithTargetAcquisition, + "applyDefaults must wire the production OpenExportWithTargetAcquisition callback") + + leafRef := aggapi.NodeRef{ + APIVersion: aggapi.VolumeSnapshotGroup + "/v1", + Kind: aggapi.VolumeSnapshotKind, + Name: leafName, + } + + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) + defer cancel() + + _, _, openErr := cfg.OpenExportWithTargetAcquisition(ctx, namespace, leafRef, targetUID, "1h") + require.Error(t, openErr, "WaitReady must time out against a DataExport the fake client never marks Ready") + assert.True(t, errors.Is(openErr, context.DeadlineExceeded), + "expected a wrapped context.DeadlineExceeded from WaitReady; got: %v", openErr) + + deName := exporter.DataExportName( + namespace, aggapi.VolumeSnapshotGroup, aggapi.VolumeSnapshotResource, aggapi.VolumeSnapshotKind, + leafName, targetUID) + + created := new(deapi.DataExport) + require.NoError(t, kubeClient.Get(context.Background(), + client.ObjectKey{Namespace: namespace, Name: deName}, created)) + + assert.Equal(t, tt.publish, created.Spec.Publish, + "the created DataExport's spec.publish must match Config.Publish") + }) + } +} From f9063c79135764aab68ade00eaa40ec142340ec4 Mon Sep 17 00:00:00 2001 From: Konstantin Kozoriz Date: Wed, 26 Aug 2026 21:13:49 +0300 Subject: [PATCH 07/13] feat(snapshot): add --publish flag to d8 snapshot download Signed-off-by: Konstantin Kozoriz --- internal/snapshot/cmd/download/download.go | 43 +++++++++- .../snapshot/cmd/download/download_test.go | 78 +++++++++++++++++++ 2 files changed, 120 insertions(+), 1 deletion(-) diff --git a/internal/snapshot/cmd/download/download.go b/internal/snapshot/cmd/download/download.go index 765aab08b..28cb9a365 100644 --- a/internal/snapshot/cmd/download/download.go +++ b/internal/snapshot/cmd/download/download.go @@ -37,6 +37,7 @@ import ( "k8s.io/apimachinery/pkg/api/meta" "k8s.io/client-go/rest" + dataio "github.com/deckhouse/deckhouse-cli/internal/data" deapi "github.com/deckhouse/deckhouse-cli/internal/data/dataexport/api/v1alpha1" "github.com/deckhouse/deckhouse-cli/internal/snapshot/aggapi" snapshotapi "github.com/deckhouse/deckhouse-cli/internal/snapshot/api/v1alpha1" @@ -46,6 +47,7 @@ import ( "github.com/deckhouse/deckhouse-cli/internal/snapshot/progress" "github.com/deckhouse/deckhouse-cli/internal/snapshot/transport" systemflags "github.com/deckhouse/deckhouse-cli/internal/system/flags" + safeClient "github.com/deckhouse/deckhouse-cli/pkg/libsaferequest/client" ) const ( @@ -61,6 +63,7 @@ const ( flagVolumeCompression = "volume-compression" flagVolumeCompressionLevel = "volume-compression-level" flagCleanup = "cleanup" + flagPublish = "publish" ) // snapshotClientQPS/snapshotClientBurst raise the kube client's rate limiter @@ -86,6 +89,22 @@ func NewCommand(log *slog.Logger) *cobra.Command { Short: "Download a snapshot to a local directory tree", SilenceUsage: true, SilenceErrors: true, + Long: `Download a Snapshot CR's manifest tree and volume data into a local directory tree +(consumed by 'd8 snapshot upload' or 'd8 snapshot restore'). + +--publish selects how each data leaf's volume bytes are streamed from its DataExport +exporter pod. With --publish=false (or when autodetection picks it), bytes come straight +from the exporter's in-cluster service, trusting only its internal CA (status.ca). With +--publish=true, bytes come through the storage-foundation-published Ingress endpoint +(status.publicURL) instead, so a kubeconfig without direct network access to the cluster's +internal service network can still download. If --publish is not given, the command probes +whether the in-cluster exporter endpoint is reachable and picks accordingly. Reusing an +existing DataExport upgrades its spec.publish from false to true when --publish=true is +requested, but never downgrades it back to false, so a concurrent run streaming through the +public endpoint is never torn down. IMPORTANT: the publish path works only with a kubeconfig +authenticated by a bearer token. Ingress terminates TLS with its own certificate and does not +forward the client's TLS certificate to the exporter pod, so a certificate-based kubeconfig +receives a 401 when --publish=true.`, Example: ` # Download snapshot "my-snap" from namespace "default" into directory ./out d8 snapshot download my-snap -n default -o out @@ -98,7 +117,10 @@ func NewCommand(log *slog.Logger) *cobra.Command { d8 snapshot download my-snap -n default -o out --node DemoVirtualDisk/bk-disk-a # Download only the root snapshot (equivalent to a full download) - d8 snapshot download my-snap -n default -o out --node Snapshot/my-snap`, + d8 snapshot download my-snap -n default -o out --node Snapshot/my-snap + + # Download through the published Ingress endpoint (requires a bearer-token kubeconfig) + d8 snapshot download my-snap -n default -o out --publish=true`, Args: cobra.ExactArgs(1), RunE: func(cmd *cobra.Command, args []string) error { return Run(cmd.Context(), log, cmd, args) @@ -123,6 +145,9 @@ func NewCommand(log *slog.Logger) *cobra.Command { cmd.Flags().Bool(flagCleanup, true, "delete the per-volume DataExport (and its server-side export chain) after each volume completes; --cleanup=false leaves them in the cluster for debugging") + cmd.Flags().Bool(flagPublish, false, "download volume data through the published (ingress) exporter endpoint instead of the in-cluster service; "+ + "if unset, the in-cluster endpoint's reachability is auto-detected") + return cmd } @@ -274,6 +299,21 @@ func Run(ctx context.Context, log *slog.Logger, cmd *cobra.Command, args []strin return err } + publishFlag, err := dataio.ParsePublishFlag(cmd.Flags()) + if err != nil { + return fmt.Errorf("resolving --%s: %w", flagPublish, err) + } + + // Probe from the command's already-resolved restConfig, not a fresh parse of + // --kubeconfig/--context: reparsing could target a different cluster than the one this + // command is actually downloading from. + probeClient := safeClient.NewSafeClientForConfig(restConfig) + + publish, err := dataio.ResolvePublish(ctx, publishFlag, kubeClient, probeClient, log) + if err != nil { + return fmt.Errorf("resolving --%s: %w", flagPublish, err) + } + tty := term.IsTerminal(int(os.Stdout.Fd())) // progress.New defaults to progress.DirectionDownload when WithDirection is // omitted, so download intentionally relies on that default rather than @@ -302,6 +342,7 @@ func Run(ctx context.Context, log *slog.Logger, cmd *cobra.Command, args []strin PerVolumeConcurrency: perVolume, MaxParallelDownloads: maxParallel, TTL: ttl, + Publish: publish, KeepExports: !cleanup, Compression: codec, KubeClient: kubeClient, diff --git a/internal/snapshot/cmd/download/download_test.go b/internal/snapshot/cmd/download/download_test.go index 346cdba38..67f4908cd 100644 --- a/internal/snapshot/cmd/download/download_test.go +++ b/internal/snapshot/cmd/download/download_test.go @@ -32,6 +32,8 @@ import ( "time" "github.com/spf13/pflag" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "k8s.io/apimachinery/pkg/api/meta" "k8s.io/client-go/rest" @@ -937,6 +939,82 @@ func TestRun_ReleasesLockOnCancelledContext(t *testing.T) { defer func() { _ = fl.Unlock() }() } +// TestNewCommand_PublishFlagDefault verifies --publish defaults to false, so +// autodetection (dataio.ResolvePublish) decides when the user does not opt in +// explicitly. +func TestNewCommand_PublishFlagDefault(t *testing.T) { + t.Parallel() + + cmd := NewCommand(slog.Default()) + + got, err := cmd.Flags().GetBool(flagPublish) + require.NoError(t, err) + assert.False(t, got, "default --publish must be false") + + flag := cmd.Flags().Lookup(flagPublish) + require.NotNil(t, flag, "flag --publish: not registered") + assert.False(t, flag.Changed, "--publish must not be marked Changed before the user sets it") +} + +// TestNewCommand_PublishFlagExplicitlySet verifies --publish=true is parsed and +// reflected via cmd.Flags().GetBool, and that dataio.ParsePublishFlag observes it +// as explicitly set (Changed) rather than merely defaulted. +func TestNewCommand_PublishFlagExplicitlySet(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + args []string + want bool + }{ + {name: "success: --publish=true is parsed", args: []string{"--publish=true"}, want: true}, + {name: "success: --publish=false is parsed explicitly", args: []string{"--publish=false"}, want: false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + cmd := NewCommand(slog.Default()) + require.NoError(t, cmd.Flags().Parse(tt.args)) + + got, err := cmd.Flags().GetBool(flagPublish) + require.NoError(t, err) + assert.Equal(t, tt.want, got) + + flag := cmd.Flags().Lookup(flagPublish) + require.NotNil(t, flag) + assert.True(t, flag.Changed, "--publish must be marked Changed once explicitly set") + }) + } +} + +// TestNewCommand_PublishDocumentation verifies the --publish contract is +// actually documented in both the Long description and the Example block: the +// bearer-token-only caveat and an example invocation must both be present, since +// this is the only place a user learns why a certificate-based kubeconfig gets a +// 401 with --publish=true. +func TestNewCommand_PublishDocumentation(t *testing.T) { + t.Parallel() + + cmd := NewCommand(slog.Default()) + + for _, want := range []string{ + "--publish", + "bearer", + "401", + } { + assert.Contains(t, cmd.Long, want, "Long description must mention %q", want) + } + + assert.Contains(t, cmd.Example, "--publish=true", "Example must show a --publish=true invocation") + + flag := cmd.Flags().Lookup(flagPublish) + require.NotNil(t, flag, "flag --publish: not registered") + assert.NotEmpty(t, flag.Usage, "--publish usage text must not be empty") + assert.False(t, flag.Hidden, "--publish must be visible in help/completion") +} + func TestNewCommand_UsesExecutionContextInstalledAfterConstruction(t *testing.T) { cancelCause := errors.New("cancel download after command construction") ctx, cancel := context.WithCancelCause(context.Background()) From 77eb8b71f55385781169a6d07ee58a0af459893f Mon Sep 17 00:00:00 2001 From: Konstantin Kozoriz Date: Fri, 28 Aug 2026 14:39:43 +0300 Subject: [PATCH 08/13] refactor(d8-snapshot): classify transient data-plane transport failures in one place Introduce exporter.IsTransientDataPlaneError as the single place that decides whether a chunk/file transport error is worth retrying. It fails closed: anything not on the allow-list (io.ErrUnexpectedEOF, io.EOF, ErrDataPlaneIdle, ECONNRESET/ECONNABORTED/EPIPE/ETIMEDOUT, and a net.Error reporting Timeout()) is treated as fatal, so a misclassification costs a loud failure rather than a silent retry loop masking a real defect. Cancellation is checked before the net.Error timeout branch: context.DeadlineExceeded itself satisfies net.Error with Timeout() == true, so checking timeouts first would misclassify an intentional cancellation/deadline as retryable. ErrExportUnauthorized and ErrContentRangeMismatch are checked next and are never transient: they describe a request the server actively rejected or a response that cannot be trusted, not a broken transport worth re-issuing. syscall.ECONNREFUSED is deliberately excluded: it means the export never accepted the connection at all, not an abrupted mid-stream transport. HTTP 5xx statuses are excluded too: RangeGet turns a non-206 status into an ordinary status error, and none has ever been observed in production ingress logs. Signed-off-by: Konstantin Kozoriz --- internal/snapshot/exporter/retry.go | 94 ++++++++++++++ internal/snapshot/exporter/retry_test.go | 157 +++++++++++++++++++++++ 2 files changed, 251 insertions(+) create mode 100644 internal/snapshot/exporter/retry.go create mode 100644 internal/snapshot/exporter/retry_test.go diff --git a/internal/snapshot/exporter/retry.go b/internal/snapshot/exporter/retry.go new file mode 100644 index 000000000..31c9b15fd --- /dev/null +++ b/internal/snapshot/exporter/retry.go @@ -0,0 +1,94 @@ +/* +Copyright 2026 Flant JSC + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package exporter + +import ( + "context" + "errors" + "io" + "net" + "syscall" +) + +// IsTransientDataPlaneError reports whether err is a transient transport +// failure on the volume data plane — one where re-issuing the Range GET from +// the caller's durable resume offset is the correct response. +// +// It fails CLOSED: anything not explicitly listed is treated as fatal, so a +// misclassification costs a loud failure, never a silent retry loop that +// masks a real defect. In particular, sentinels defined outside this package +// (volume.ErrShortChunkRead, any *os.PathError from local disk I/O) are +// non-transient by construction, because the default is false. +func IsTransientDataPlaneError(err error) bool { + if err == nil { + return false + } + + // Cancellation/deadline must be checked BEFORE the net.Error timeout + // check below: context.DeadlineExceeded itself implements net.Error + // with Timeout() == true, so checking timeouts first would + // misclassify an intentional cancellation/deadline as retryable. + if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) { + return false + } + + // Contractual errors are never transient: they describe a request the + // server actively rejected or a response that cannot be trusted, not a + // broken transport worth re-issuing the same request against. + if errors.Is(err, ErrExportUnauthorized) || errors.Is(err, ErrContentRangeMismatch) { + return false + } + + if errors.Is(err, io.ErrUnexpectedEOF) { + return true + } + + // io.Copy treats a clean io.EOF as ordinary successful completion, so + // this can only reach us wrapped by a transport layer that itself + // decided the stream ended abnormally. + if errors.Is(err, io.EOF) { + return true + } + + if errors.Is(err, ErrDataPlaneIdle) { + return true + } + + if errors.Is(err, syscall.ECONNRESET) || + errors.Is(err, syscall.ECONNABORTED) || + errors.Is(err, syscall.EPIPE) || + errors.Is(err, syscall.ETIMEDOUT) { + return true + } + + var netErr net.Error + if errors.As(err, &netErr) && netErr.Timeout() { + return true + } + + // Deliberately NOT retried: + // - syscall.ECONNREFUSED: the export never accepted the connection at + // all, not a broken mid-stream transport — retrying just repeats a + // connection nobody is listening on. + // - HTTP 5xx status codes: RangeGet turns a non-206 status into an + // ordinary status error, and none has ever been observed in + // practice (0 occurrences across 122k lines of ingress logs). + // - volume.ErrShortChunkRead: a clean short read means the server lied + // about the range it promised, which is unreachable from this + // package and not a transport failure to retry. + return false +} diff --git a/internal/snapshot/exporter/retry_test.go b/internal/snapshot/exporter/retry_test.go new file mode 100644 index 000000000..d4ae55186 --- /dev/null +++ b/internal/snapshot/exporter/retry_test.go @@ -0,0 +1,157 @@ +/* +Copyright 2026 Flant JSC + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package exporter + +import ( + "context" + "errors" + "fmt" + "io" + "net" + "net/url" + "os" + "syscall" + "testing" +) + +func TestIsTransientDataPlaneError(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + err error + want bool + }{ + {name: "nil error is not transient", err: nil, want: false}, + { + name: "unexpected EOF is transient", + err: fmt.Errorf("stream: %w", io.ErrUnexpectedEOF), + want: true, + }, + { + name: "clean EOF wrapped by a transport layer is transient", + err: fmt.Errorf("read: %w", io.EOF), + want: true, + }, + { + name: "idle watchdog trip is transient", + err: fmt.Errorf("read chunk: %w", ErrDataPlaneIdle), + want: true, + }, + { + name: "ECONNRESET is transient", + err: fmt.Errorf("read: %w", syscall.ECONNRESET), + want: true, + }, + { + name: "ECONNABORTED is transient", + err: fmt.Errorf("read: %w", syscall.ECONNABORTED), + want: true, + }, + { + name: "EPIPE is transient", + err: fmt.Errorf("write: %w", syscall.EPIPE), + want: true, + }, + { + name: "ETIMEDOUT is transient", + err: fmt.Errorf("dial: %w", syscall.ETIMEDOUT), + want: true, + }, + { + name: "a net.Error reporting Timeout() is transient", + err: fmt.Errorf("http do: %w", &fakeNetError{timeout: true}), + want: true, + }, + { + name: "a net.Error not reporting Timeout() is fatal", + err: fmt.Errorf("http do: %w", &fakeNetError{timeout: false}), + want: false, + }, + { + name: "ErrExportUnauthorized is fatal", + err: fmt.Errorf("range get: %w", ErrExportUnauthorized), + want: false, + }, + { + name: "ErrContentRangeMismatch is fatal", + err: fmt.Errorf("range get: %w", ErrContentRangeMismatch), + want: false, + }, + { + name: "context.Canceled is fatal, never retried", + err: fmt.Errorf("do: %w", context.Canceled), + want: false, + }, + { + // Regression guard: context.DeadlineExceeded satisfies + // net.Error with Timeout() == true, so this must be checked + // (and rejected) BEFORE the net.Error branch, not fall through + // to it and be misclassified as transient. + name: "context.DeadlineExceeded is fatal despite satisfying net.Error", + err: fmt.Errorf("do: %w", context.DeadlineExceeded), + want: false, + }, + { + name: "a wrapped *url.Error without Timeout() is fatal", + err: &url.Error{ + Op: "Get", + URL: "https://example.invalid/api/v1/block", + Err: errors.New("no route to host"), + }, + want: false, + }, + { + name: "ECONNREFUSED is fatal: the export never accepted the connection", + err: fmt.Errorf("dial: %w", syscall.ECONNREFUSED), + want: false, + }, + { + name: "a local *os.PathError is fatal", + err: &os.PathError{Op: "open", Path: "/tmp/chunk.part", Err: errors.New("permission denied")}, + want: false, + }, + { + name: "an arbitrary error is fatal by default", + err: errors.New("simulated interrupt: connection dropped mid-chunk"), + want: false, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + got := IsTransientDataPlaneError(tc.err) + if got != tc.want { + t.Errorf("IsTransientDataPlaneError(%v) = %v, want %v", tc.err, got, tc.want) + } + }) + } +} + +// fakeNetError is a minimal net.Error stand-in that lets tests control +// Timeout() independently of any real network condition. +type fakeNetError struct { + timeout bool +} + +func (e *fakeNetError) Error() string { return "fake net error" } +func (e *fakeNetError) Timeout() bool { return e.timeout } +func (e *fakeNetError) Temporary() bool { return e.timeout } + +var _ net.Error = (*fakeNetError)(nil) From 17c3b1837f5804cb8df03ecc225991e4ae47b000 Mon Sep 17 00:00:00 2001 From: Konstantin Kozoriz Date: Fri, 28 Aug 2026 14:57:42 +0300 Subject: [PATCH 09/13] fix(d8-snapshot): retry a broken chunk stream from its durable offset A single transient transport failure anywhere inside a chunk's Range GET used to fail the whole block/file volume, even though an exact byte-durable resume checkpoint (fsync'd .part + offset sidecar) already existed on disk. Reuse that same mechanism as the retry unit: on a transient error, re-issue the Range GET from the durable offset the interrupted attempt persisted, with bounded exponential backoff, instead of surfacing the failure to the caller immediately. chunkRetrier wraps fetchChunkRaw (unchanged: it stays a single attempt, preserving the resume contract several existing tests pin down) with a wait.ExponentialBackoffWithContext loop. Cancellation is checked before error classification so an aborted request never gets mistaken for a retryable transport error. A no-progress guard stops attempts that keep advancing zero bytes well before the backoff budget would otherwise be spent on a link that is never going to deliver. chunkProgressLedger de-duplicates onProgress credits across attempts: each fetchChunkRaw attempt re-credits its own resume prefix, so without the ledger a retried chunk would over-report progress past the volume's true size. downloadBlockChunks creates one chunkRetrier per volume, shared by every chunk goroutine, and logs a single aggregate WARN if any retries were absorbed. stageChunkedFile downloads every non-empty file through the same downloadBlockChunks path, so the filesystem volume path gets this fix for free. Signed-off-by: Konstantin Kozoriz --- internal/snapshot/volume/block.go | 56 +- internal/snapshot/volume/block_test.go | 136 +++- internal/snapshot/volume/chunk_retry.go | 229 ++++++ .../volume/chunk_retry_internal_test.go | 676 ++++++++++++++++++ 4 files changed, 1074 insertions(+), 23 deletions(-) create mode 100644 internal/snapshot/volume/chunk_retry.go create mode 100644 internal/snapshot/volume/chunk_retry_internal_test.go diff --git a/internal/snapshot/volume/block.go b/internal/snapshot/volume/block.go index 8ef20e55b..f7576b0ac 100644 --- a/internal/snapshot/volume/block.go +++ b/internal/snapshot/volume/block.go @@ -112,6 +112,14 @@ var ErrShortChunkRead = errors.New("chunk range body ended before the requested // DATA. The final codec frame is produced, and the ".part" file consumed, // only once the raw bytes are fully durable on disk. // +// The same durable-resume mechanism now also backs an IN-RUN retry: a +// transient transport failure (see exporter.IsTransientDataPlaneError) does +// not fail the whole chunk. chunkRetrier re-issues the Range GET from the +// exact durable offset the interrupted attempt persisted, with bounded +// exponential backoff, so one broken connection mid-chunk no longer forces a +// second process run to make progress — resuming across attempts within a +// single call is exactly the same mechanism as resuming across runs. +// // Memory note: once a chunk's ".part" file is complete, finalizeChunkFrame // streams it through codec.EncodeFrameStream directly into the final chunk's // AtomicWriter — the whole raw chunk is never read into memory as a []byte @@ -211,6 +219,8 @@ func downloadBlockChunks( g, gctx := errgroup.WithContext(ctx) g.SetLimit(workers) + retrier := &chunkRetrier{policy: defaultChunkRetryPolicy()} + for i := range numChunks { chunkIdx := i @@ -226,12 +236,23 @@ func downloadBlockChunks( totalSize, fetcher, codec, + retrier, onProgress, ) }) } - return g.Wait() + if err := g.Wait(); err != nil { + return err + } + + if recovered := retrier.recovered.Load(); recovered > 0 { + log.Warn("block chunk downloads completed after transient transport failures", + slog.String("dir", chunkDir), + slog.Int64("retries", recovered)) + } + + return nil } // ensureChunkGeometry guards against resuming a chunk directory that was @@ -345,6 +366,7 @@ func downloadChunk( totalSize int64, fetcher *exporter.Fetcher, codec compress.Codec, + retrier *chunkRetrier, onProgress func(n int), ) error { finalPath := filepath.Join(chunkDir, archive.ChunkFileName(chunkIdx, codec.Ext())) @@ -383,7 +405,7 @@ func downloadChunk( return fmt.Errorf("remove stale tmp %s: %w", tmpPath, err) } - if err := fetchChunkRaw( + if err := retrier.fetchChunk( ctx, destination, log, @@ -490,6 +512,13 @@ func finalizeChunkFrame( // resulting partPath size is asserted to equal rawLen; a server that // under-sends (a short read) is reported as ErrShortChunkRead rather than // silently finalizing a truncated chunk. +// +// fetchChunkRaw remains a SINGLE attempt: it never retries internally. The +// returned int64 is the chunk-relative offset now durable in partPath — a +// PROGRESS HEURISTIC for chunkRetrier's retry loop only. It is never used as +// a resume point directly: the next attempt (or the next process run) +// always recomputes the authoritative offset via partialChunkSize, which +// trusts only the fsync-proven sidecar. func fetchChunkRaw( ctx context.Context, destination *archive.RootedDestination, @@ -500,10 +529,10 @@ func fetchChunkRaw( chunkIdx int, startByte, endByte, rawLen int64, onProgress func(n int), -) error { +) (int64, error) { have, err := partialChunkSize(destination, partPath, rawLen) if err != nil { - return fmt.Errorf("stat partial chunk %d: %w", chunkIdx, err) + return 0, fmt.Errorf("stat partial chunk %d: %w", chunkIdx, err) } if onProgress != nil && have > 0 { @@ -514,7 +543,7 @@ func fetchChunkRaw( // The durable partial already covers the whole chunk (e.g. a crash // between finishing the raw download and finalizing the frame on a // previous run): nothing left to fetch. - return nil + return have, nil } log.Debug("fetching chunk", @@ -525,14 +554,14 @@ func fetchChunkRaw( body, err := fetcher.RangeGet(ctx, blockURL, startByte+have, endByte) if err != nil { - return fmt.Errorf("range get chunk %d: %w", chunkIdx, err) + return have, fmt.Errorf("range get chunk %d: %w", chunkIdx, err) } defer func() { _ = body.Close() }() f, err := blockPathOpenAppend(destination, partPath) if err != nil { - return fmt.Errorf("open partial chunk %d: %w", chunkIdx, err) + return have, fmt.Errorf("open partial chunk %d: %w", chunkIdx, err) } sw := &syncingWriter{ @@ -556,29 +585,30 @@ func fetchChunkRaw( _, copyErr := io.Copy(sw, cr) finishErr := sw.finish() closeErr := f.Close() + durable := have + sw.written if copyErr != nil { - return fmt.Errorf("stream chunk %d body: %w", chunkIdx, copyErr) + return durable, fmt.Errorf("stream chunk %d body: %w", chunkIdx, copyErr) } if finishErr != nil { - return fmt.Errorf("finalize partial chunk %d: %w", chunkIdx, finishErr) + return durable, fmt.Errorf("finalize partial chunk %d: %w", chunkIdx, finishErr) } if closeErr != nil { - return fmt.Errorf("close partial chunk %d: %w", chunkIdx, closeErr) + return durable, fmt.Errorf("close partial chunk %d: %w", chunkIdx, closeErr) } info, statErr := blockPathStat(destination, partPath) if statErr != nil { - return fmt.Errorf("stat finalized partial chunk %d: %w", chunkIdx, statErr) + return durable, fmt.Errorf("stat finalized partial chunk %d: %w", chunkIdx, statErr) } if info.Size() != rawLen { - return fmt.Errorf("chunk %d: %w: partial file holds %d bytes, want %d", chunkIdx, ErrShortChunkRead, info.Size(), rawLen) + return durable, fmt.Errorf("chunk %d: %w: partial file holds %d bytes, want %d", chunkIdx, ErrShortChunkRead, info.Size(), rawLen) } - return nil + return rawLen, nil } // ScanBlockChunkProgress computes durably-committed raw bytes and the raw diff --git a/internal/snapshot/volume/block_test.go b/internal/snapshot/volume/block_test.go index 799d06359..40ac892b5 100644 --- a/internal/snapshot/volume/block_test.go +++ b/internal/snapshot/volume/block_test.go @@ -706,31 +706,38 @@ func TestDownloadBlockChunks_CorruptChunkMeta_PurgesAndRedownloads(t *testing.T) // without any real sleeps, timeouts, or network flakiness. var errSimulatedInterrupt = errors.New("simulated interrupt: connection dropped mid-chunk") -// truncatingBody wraps an http response body and returns errSimulatedInterrupt -// after delivering exactly budget bytes, deterministically simulating an -// interrupt partway through a chunk's Range GET body. +// truncatingBody wraps an http response body and returns cutErr (defaulting +// to errSimulatedInterrupt when nil) after delivering exactly budget bytes, +// deterministically simulating an interrupt partway through a chunk's Range +// GET body. type truncatingBody struct { r io.ReadCloser budget int64 + cutErr error } func (b *truncatingBody) Read(p []byte) (int, error) { + err := b.cutErr + if err == nil { + err = errSimulatedInterrupt + } + if b.budget <= 0 { - return 0, errSimulatedInterrupt + return 0, err } if int64(len(p)) > b.budget { p = p[:b.budget] } - n, err := b.r.Read(p) + n, readErr := b.r.Read(p) b.budget -= int64(n) - if err == nil && b.budget <= 0 { - err = errSimulatedInterrupt + if readErr == nil && b.budget <= 0 { + readErr = err } - return n, err + return n, readErr } func (b *truncatingBody) Close() error { @@ -740,11 +747,13 @@ func (b *truncatingBody) Close() error { // recordingDoer wraps a real exporter.Doer, recording every request's Range // header in call order and optionally truncating the response body of one // designated call (cutOnCall, 1-based; 0 disables truncation) after -// cutBytes bytes to simulate a mid-transfer interrupt. +// cutBytes bytes to simulate a mid-transfer interrupt. cutErr selects the +// error the truncated body reports; nil defaults to errSimulatedInterrupt. type recordingDoer struct { inner exporter.Doer cutOnCall int cutBytes int64 + cutErr error mu sync.Mutex calls int @@ -764,7 +773,7 @@ func (d *recordingDoer) Do(req *http.Request) (*http.Response, error) { } if callIdx == d.cutOnCall { - resp.Body = &truncatingBody{r: resp.Body, budget: d.cutBytes} + resp.Body = &truncatingBody{r: resp.Body, budget: d.cutBytes, cutErr: d.cutErr} } return resp, nil @@ -1601,3 +1610,110 @@ func TestDownloadBlockChunks_StreamedFrameContract(t *testing.T) { }) } } + +// onceFlakyDoer wraps a real exporter.Doer and cuts the response body of the +// FIRST request whose Range header exactly matches trigger, standing in for +// one broken connection mid-chunk. Every other request — including the +// retried request that resumes from a later offset and so carries a +// different Range value — passes through untouched. +type onceFlakyDoer struct { + inner exporter.Doer + trigger string + cutBytes int64 + + mu sync.Mutex + triggered bool + calls int +} + +func (d *onceFlakyDoer) Do(req *http.Request) (*http.Response, error) { + d.mu.Lock() + d.calls++ + + fireNow := !d.triggered && req.Header.Get("Range") == d.trigger + if fireNow { + d.triggered = true + } + + d.mu.Unlock() + + resp, err := d.inner.Do(req) + if err != nil { + return resp, err + } + + if fireNow { + resp.Body = &truncatingBody{r: resp.Body, budget: d.cutBytes, cutErr: io.ErrUnexpectedEOF} + } + + return resp, nil +} + +func (d *onceFlakyDoer) callCount() int { + d.mu.Lock() + defer d.mu.Unlock() + + return d.calls +} + +// TestDownloadBlockChunks_RetryIsPerChunk proves the in-run retry is scoped +// to the one chunk that hits a transient failure: three chunks download +// cleanly on the first attempt while a fourth is interrupted mid-stream and +// must recover via a resumed retry, without disturbing the others or +// corrupting the merged output. It deliberately exercises the DEFAULT retry +// policy (via the public DownloadBlockChunks entry point, with no policy +// override available) — the only test proving the production policy is +// actually wired into downloadBlockChunks rather than merely reachable +// through a hand-rolled test policy. +func TestDownloadBlockChunks_RetryIsPerChunk(t *testing.T) { + t.Parallel() + + const chunkSize = 5 + payload := []byte("ABCDEFGHIJKLMNOPQRST") // 20 bytes -> 4 chunks of 5 + + srv := newBlockServer(t, payload) + defer srv.Close() + + blockURL := srv.URL + "/api/v1/block" + + // Chunk 1 covers bytes [5,9]; its very first (unresumed) Range GET is cut + // after 2 bytes with a transient error. Every other chunk, and chunk 1's + // resumed retry, must succeed untouched. + doer := &onceFlakyDoer{inner: srv.Client(), trigger: "bytes=5-9", cutBytes: 2} + fetcher := exporter.NewFetcher(doer) + + codec, err := compress.New("zstd", int(compress.LevelFastest)) + require.NoError(t, err) + + nodeDir := t.TempDir() + chunkDir := filepath.Join(nodeDir, archive.BlockChunksDirName) + + err = volume.DownloadBlockChunks( + context.Background(), slog.Default(), chunkDir, blockURL, int64(len(payload)), chunkSize, 4, fetcher, codec, nil) + require.NoError(t, err, "a single transient failure on one chunk must not fail the whole volume") + + names := listChunkFiles(t, chunkDir) + require.Len(t, names, 4, "expected 4 chunks") + + wantSlices := [][]byte{ + payload[0:5], + payload[5:10], + payload[10:15], + payload[15:20], + } + + for i, name := range names { + got := decodeAll(t, filepath.Join(chunkDir, name)) + assert.Equal(t, wantSlices[i], got, "chunk %d content mismatch", i) + } + + outPath := filepath.Join(nodeDir, archive.DataBlockName(codec.Ext())) + require.NoError(t, volume.MergeBlockChunks(context.Background(), chunkDir, outPath, int64(len(payload)), chunkSize, codec.Ext())) + + merged := decodeAll(t, outPath) + assert.Equal(t, payload, merged, "merged output must be byte-identical despite the mid-chunk retry") + + // Each of the 3 clean chunks is fetched exactly once; the flaky chunk is + // fetched exactly twice (the interrupted attempt plus its resumed retry). + assert.Equal(t, 5, doer.callCount(), "expected exactly one retried request across all 4 chunks") +} diff --git a/internal/snapshot/volume/chunk_retry.go b/internal/snapshot/volume/chunk_retry.go new file mode 100644 index 000000000..41116f094 --- /dev/null +++ b/internal/snapshot/volume/chunk_retry.go @@ -0,0 +1,229 @@ +/* +Copyright 2026 Flant JSC + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package volume + +import ( + "context" + "fmt" + "log/slog" + "sync/atomic" + "time" + + "k8s.io/apimachinery/pkg/util/wait" + + "github.com/deckhouse/deckhouse-cli/internal/snapshot/archive" + "github.com/deckhouse/deckhouse-cli/internal/snapshot/exporter" +) + +// chunkFetchBackoff bounds one chunk's in-run retries: starting at 1s and +// doubling, capped at 30s. wait.Backoff's Cap does double duty as both a +// ceiling on any single sleep AND an early-termination trigger — once a +// projected next delay would exceed Cap, the step budget is forced to 0 +// immediately, ending the loop one attempt sooner than Steps alone would +// suggest. With these parameters that yields 5 attempts in the worst case +// (not 6), for a total backoff of ~30-34s added to a chunk that eventually +// succeeds — negligible against the 650-790s a single 256 MiB chunk stream +// already takes on a WAN link — while a link that keeps breaking still fails +// loudly instead of grinding for hours. +var chunkFetchBackoff = wait.Backoff{ + Steps: 6, + Duration: 1 * time.Second, + Factor: 2.0, + Jitter: 0.2, + Cap: 30 * time.Second, +} + +// maxNoProgressChunkAttempts bounds attempts that re-issue the Range GET and +// advance the durable offset by zero bytes. A server that accepts the range +// and immediately closes would otherwise burn the whole backoff budget in a +// hot loop; three such attempts is proof the far end is not going to deliver. +const maxNoProgressChunkAttempts = 3 + +// chunkRetryPolicy bounds one chunk's retry loop: chunkRetrier.fetchChunk +// stops re-issuing the Range GET once either backoff's step budget or +// maxNoProgress consecutive zero-progress attempts is exhausted. +type chunkRetryPolicy struct { + backoff wait.Backoff + maxNoProgress int +} + +// defaultChunkRetryPolicy returns the production retry policy applied to +// every chunk download. +func defaultChunkRetryPolicy() chunkRetryPolicy { + return chunkRetryPolicy{ + backoff: chunkFetchBackoff, + maxNoProgress: maxNoProgressChunkAttempts, + } +} + +// chunkRetrier carries one volume's retry policy plus the aggregate count of +// retries it absorbed. One instance per downloadBlockChunks call, shared by +// every chunk goroutine; only recovered is mutated concurrently, and it is +// atomic for that reason. +type chunkRetrier struct { + policy chunkRetryPolicy + recovered atomic.Int64 +} + +// chunkProgressLedger converts fetchChunkRaw's per-attempt credits into +// strictly monotonic, never-duplicated credits for the shared onProgress +// sink. Each fetchChunkRaw attempt credits (resume prefix) + (bytes streamed +// this attempt) — i.e. the credits inside one attempt sum to an absolute +// position within the chunk. Across attempts those positions OVERLAP: attempt +// 2 re-credits the prefix attempt 1 already streamed. The ledger forwards only +// the amount by which the attempt position exceeds the highest position ever +// forwarded, so the credits it emits sum to exactly rawLen no matter how many +// attempts the chunk took. +// +// Not synchronised: one ledger belongs to one chunk, and one chunk is one +// goroutine (downloadChunk). The onProgress it forwards to is the shared, +// already-concurrency-safe sink. +type chunkProgressLedger struct { + onProgress func(n int) + attemptPos int64 + highWater int64 +} + +// beginAttempt resets the per-attempt position counter before a new +// fetchChunkRaw attempt starts crediting from zero again. +func (l *chunkProgressLedger) beginAttempt() { + l.attemptPos = 0 +} + +// credit records n additional bytes reported by the current attempt (a resume +// credit or a streamed-bytes credit) and forwards to onProgress only the +// amount by which the attempt's cumulative position exceeds every credit +// already forwarded, so a retried attempt never double-counts bytes an +// earlier attempt already reported. +func (l *chunkProgressLedger) credit(n int) { + if n <= 0 { + return + } + + l.attemptPos += int64(n) + + if l.attemptPos <= l.highWater { + return + } + + advance := l.attemptPos - l.highWater + l.highWater = l.attemptPos + + if l.onProgress != nil { + l.onProgress(int(advance)) + } +} + +// fetchChunk retries fetchChunkRaw with bounded exponential backoff, +// resuming each attempt from the durable offset the previous attempt +// persisted, until the chunk completes, a fatal (non-transient) error +// occurs, ctx is cancelled, or the retry budget is exhausted. +// +// This is the only retry seam for a chunk's raw download: fetchChunkRaw +// itself stays a single attempt (the resume contract several tests pin down), +// and downloadBlockChunks' errgroup is not re-entered per attempt (which +// would re-run finalizeChunkFrame/ensureChunkGeometry unnecessarily). +func (r *chunkRetrier) fetchChunk( + ctx context.Context, + destination *archive.RootedDestination, + log *slog.Logger, + fetcher *exporter.Fetcher, + blockURL string, + partPath string, + chunkIdx int, + startByte, endByte, rawLen int64, + onProgress func(n int), +) error { + ledger := &chunkProgressLedger{onProgress: onProgress} + + var ( + lastErr error + attempt int + noProgress int + lastDurable = int64(-1) + ) + + // wait.Backoff.Step() mutates its receiver, so the shared policy backoff + // must be passed BY VALUE here (ExponentialBackoffWithContext takes it + // by value): every concurrent chunk goroutine gets its own independent + // copy to mutate, never the shared r.policy.backoff itself. + backoff := r.policy.backoff + steps := backoff.Steps + + backoffErr := wait.ExponentialBackoffWithContext(ctx, backoff, func(stepCtx context.Context) (bool, error) { + attempt++ + + ledger.beginAttempt() + + durable, err := fetchChunkRaw(stepCtx, destination, log, fetcher, blockURL, partPath, + chunkIdx, startByte, endByte, rawLen, ledger.credit) + + switch { + case err == nil: + return true, nil + case ctx.Err() != nil: + // Cancellation must win over classification: an aborted request + // can surface through the HTTP transport looking like an + // ordinary transient failure, and must not be retried. + return false, err + case !exporter.IsTransientDataPlaneError(err): + log.Debug("chunk fetch failed with a non-retryable error", + slog.Int("chunk", chunkIdx), + slog.String("error_type", fmt.Sprintf("%T", err))) + + return false, err + } + + if durable > lastDurable { + noProgress = 0 + } else { + noProgress++ + } + + lastDurable = durable + + if noProgress >= r.policy.maxNoProgress { + return false, fmt.Errorf("chunk %d made no progress in %d consecutive attempts: %w", + chunkIdx, noProgress, err) + } + + lastErr = err + + r.recovered.Add(1) + + if attempt < steps { + log.Warn("retrying chunk after a transient transport failure", + slog.Int("chunk", chunkIdx), + slog.Int("attempt", attempt), + slog.String("error", err.Error())) + } + + return false, nil + }) + + switch { + case backoffErr == nil: + return nil + case ctx.Err() != nil: + return fmt.Errorf("chunk %d: %w", chunkIdx, ctx.Err()) + case wait.Interrupted(backoffErr) && lastErr != nil: + return fmt.Errorf("chunk %d: exhausted %d attempts on transient transport failures: %w", + chunkIdx, steps, lastErr) + default: + return backoffErr + } +} diff --git a/internal/snapshot/volume/chunk_retry_internal_test.go b/internal/snapshot/volume/chunk_retry_internal_test.go new file mode 100644 index 000000000..c38986c3a --- /dev/null +++ b/internal/snapshot/volume/chunk_retry_internal_test.go @@ -0,0 +1,676 @@ +/* +Copyright 2026 Flant JSC + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package volume + +import ( + "context" + "errors" + "io" + "log/slog" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "strings" + "sync" + "testing" + "time" + + "k8s.io/apimachinery/pkg/util/wait" + + "github.com/deckhouse/deckhouse-cli/internal/snapshot/exporter" +) + +// fastChunkRetryPolicy is a test-only policy with the same shape as +// defaultChunkRetryPolicy but with a millisecond-scale backoff, so retry +// tests don't pay the production policy's multi-second budget. It is +// deliberately unexported and local to this file: no test in this package +// gets to change the production default via a shared knob. +// +// Cap is set well above the growth this Steps/Duration/Factor combination +// ever reaches (1ms -> 2ms -> 4ms -> 8ms for Steps=4): wait.Backoff.Step +// forces its internal step counter to 0 — ending the retry loop one +// invocation EARLIER than Steps would otherwise suggest — the moment a +// projected next duration exceeds Cap, so a tight Cap here would silently +// undercount the very attempts these tests assert on. +func fastChunkRetryPolicy() chunkRetryPolicy { + return chunkRetryPolicy{ + backoff: wait.Backoff{ + Steps: 4, + Duration: time.Millisecond, + Factor: 2, + Cap: 50 * time.Millisecond, + }, + maxNoProgress: 3, + } +} + +// newRangeServer serves data at "/block" with Range-GET support +// (http.ServeContent), mirroring the data-exporter's block endpoint contract. +func newRangeServer(t *testing.T, data []byte) *httptest.Server { + t.Helper() + + mux := http.NewServeMux() + mux.HandleFunc("/block", func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/octet-stream") + http.ServeContent(w, r, "data.img", time.Time{}, strings.NewReader(string(data))) + }) + + srv := httptest.NewServer(mux) + t.Cleanup(srv.Close) + + return srv +} + +// cutBody wraps a response body and, after delivering exactly budget bytes, +// reports err instead of continuing to read from the underlying body. +type cutBody struct { + r io.ReadCloser + budget int64 + err error +} + +func (b *cutBody) Read(p []byte) (int, error) { + if b.budget <= 0 { + return 0, b.err + } + + if int64(len(p)) > b.budget { + p = p[:b.budget] + } + + n, err := b.r.Read(p) + b.budget -= int64(n) + + if err == nil && b.budget <= 0 { + err = b.err + } + + return n, err +} + +func (b *cutBody) Close() error { + return b.r.Close() +} + +// scriptedRangeDoer wraps a real exporter.Doer and records every request's +// Range header in call order. cut, when non-nil, truncates every response +// body (every call, not just one) after cutBytes bytes with cutErr — standing +// in for a link that breaks on every attempt. +type scriptedRangeDoer struct { + inner exporter.Doer + cutBytes int64 + cutErr error // nil disables truncation + + mu sync.Mutex + ranges []string +} + +func (d *scriptedRangeDoer) Do(req *http.Request) (*http.Response, error) { + d.mu.Lock() + d.ranges = append(d.ranges, req.Header.Get("Range")) + d.mu.Unlock() + + resp, err := d.inner.Do(req) + if err != nil { + return resp, err + } + + if d.cutErr != nil { + resp.Body = &cutBody{r: resp.Body, budget: d.cutBytes, err: d.cutErr} + } + + return resp, nil +} + +func (d *scriptedRangeDoer) recordedRanges() []string { + d.mu.Lock() + defer d.mu.Unlock() + + out := make([]string, len(d.ranges)) + copy(out, d.ranges) + + return out +} + +func (d *scriptedRangeDoer) callCount() int { + d.mu.Lock() + defer d.mu.Unlock() + + return len(d.ranges) +} + +// onceCutDoer wraps a real exporter.Doer and truncates only the FIRST +// response body (with cutErr, after cutBytes bytes); every subsequent call — +// in particular the resumed retry — passes through untouched. +type onceCutDoer struct { + inner exporter.Doer + cutBytes int64 + cutErr error + + mu sync.Mutex + calls int + ranges []string +} + +func (d *onceCutDoer) Do(req *http.Request) (*http.Response, error) { + d.mu.Lock() + d.calls++ + callIdx := d.calls + d.ranges = append(d.ranges, req.Header.Get("Range")) + d.mu.Unlock() + + resp, err := d.inner.Do(req) + if err != nil { + return resp, err + } + + if callIdx == 1 { + resp.Body = &cutBody{r: resp.Body, budget: d.cutBytes, err: d.cutErr} + } + + return resp, nil +} + +func (d *onceCutDoer) recordedRanges() []string { + d.mu.Lock() + defer d.mu.Unlock() + + out := make([]string, len(d.ranges)) + copy(out, d.ranges) + + return out +} + +// TestChunkRetrier_ResumesFromDurableOffset proves the retry loop resumes +// each attempt from the exact durable offset the previous, interrupted +// attempt persisted — never from byte zero — and that onProgress credits +// across attempts sum to exactly rawLen with no double counting. +func TestChunkRetrier_ResumesFromDurableOffset(t *testing.T) { + t.Parallel() + + payload := []byte("0123456789ABCDEFGHIJ") // 20 bytes + const cutBytes = 12 // attempt 1 delivers 12 bytes, then breaks + + srv := newRangeServer(t, payload) + blockURL := srv.URL + "/block" + + doer := &onceCutDoer{cutBytes: cutBytes, cutErr: io.ErrUnexpectedEOF} + doer.inner = srv.Client() + fetcher := exporter.NewFetcher(doer) + + dir := t.TempDir() + partPath := filepath.Join(dir, "chunk_00000.part") + + retrier := &chunkRetrier{policy: fastChunkRetryPolicy()} + + var ( + mu sync.Mutex + credits []int + ) + + onProgress := func(n int) { + mu.Lock() + credits = append(credits, n) + mu.Unlock() + } + + rawLen := int64(len(payload)) + + err := retrier.fetchChunk(context.Background(), nil, slog.Default(), fetcher, blockURL, + partPath, 0, 0, rawLen-1, rawLen, onProgress) + if err != nil { + t.Fatalf("fetchChunk: %v", err) + } + + got, err := os.ReadFile(partPath) + if err != nil { + t.Fatalf("read partPath: %v", err) + } + + if string(got) != string(payload) { + t.Errorf("partPath content = %q, want %q", got, payload) + } + + ranges := doer.recordedRanges() + if len(ranges) != 2 { + t.Fatalf("expected exactly 2 requests, got %d: %v", len(ranges), ranges) + } + + if want := "bytes=0-19"; ranges[0] != want { + t.Errorf("attempt 1 range = %q, want %q", ranges[0], want) + } + + if want := "bytes=12-19"; ranges[1] != want { + t.Errorf("attempt 2 range = %q, want %q (must resume from the durable offset, not byte 0)", ranges[1], want) + } + + var sum int + + for _, c := range credits { + sum += c + } + + if int64(sum) != rawLen { + t.Errorf("onProgress credits summed to %d, want %d (rawLen): %v", sum, rawLen, credits) + } +} + +// TestChunkRetrier_ExhaustsBudget proves that a link broken on every attempt +// exhausts exactly the policy's Steps budget and returns an error that still +// satisfies errors.Is against the underlying transient sentinel. +func TestChunkRetrier_ExhaustsBudget(t *testing.T) { + t.Parallel() + + payload := make([]byte, 100) + + srv := newRangeServer(t, payload) + blockURL := srv.URL + "/block" + + doer := &scriptedRangeDoer{inner: srv.Client(), cutBytes: 5, cutErr: io.ErrUnexpectedEOF} + fetcher := exporter.NewFetcher(doer) + + dir := t.TempDir() + partPath := filepath.Join(dir, "chunk_00000.part") + + policy := fastChunkRetryPolicy() + retrier := &chunkRetrier{policy: policy} + + rawLen := int64(len(payload)) + + err := retrier.fetchChunk(context.Background(), nil, slog.Default(), fetcher, blockURL, + partPath, 0, 0, rawLen-1, rawLen, nil) + if err == nil { + t.Fatal("expected an error once the retry budget is exhausted, got nil") + } + + if !errors.Is(err, io.ErrUnexpectedEOF) { + t.Errorf("expected errors.Is(err, io.ErrUnexpectedEOF), got: %v", err) + } + + if got := doer.callCount(); got != policy.backoff.Steps { + t.Errorf("expected exactly %d requests (the full Steps budget), got %d", policy.backoff.Steps, got) + } + + // The durable partial and its offset sidecar must survive: a future + // process run still has something to resume from. + if _, statErr := os.Stat(partPath); statErr != nil { + t.Errorf("expected partPath to survive exhaustion, stat failed: %v", statErr) + } + + if _, statErr := os.Stat(partPath + partOffsetSuffix); statErr != nil { + t.Errorf("expected the durable offset sidecar to survive exhaustion, stat failed: %v", statErr) + } +} + +// TestChunkRetrier_DoesNotRetryFatal proves that every non-transient error +// stops the retry loop on the very first attempt, and that errors.Is against +// the original sentinel still holds through fetchChunk's returned error. +func TestChunkRetrier_DoesNotRetryFatal(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + buildFetch func(t *testing.T) (fetcher *exporter.Fetcher, blockURL string, callCount func() int) + wantErr error + }{ + { + name: "401 unauthorized", + buildFetch: func(t *testing.T) (*exporter.Fetcher, string, func() int) { + t.Helper() + return statusDoerFetcher(t, http.StatusUnauthorized) + }, + wantErr: exporter.ErrExportUnauthorized, + }, + { + name: "403 forbidden", + buildFetch: func(t *testing.T) (*exporter.Fetcher, string, func() int) { + t.Helper() + return statusDoerFetcher(t, http.StatusForbidden) + }, + wantErr: exporter.ErrExportUnauthorized, + }, + { + name: "content-range mismatch", + buildFetch: func(t *testing.T) (*exporter.Fetcher, string, func() int) { + t.Helper() + return mismatchedRangeFetcher(t) + }, + wantErr: exporter.ErrContentRangeMismatch, + }, + { + name: "clean short read", + buildFetch: func(t *testing.T) (*exporter.Fetcher, string, func() int) { + t.Helper() + return shortReadFetcher(t) + }, + wantErr: ErrShortChunkRead, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + fetcher, blockURL, callCount := tc.buildFetch(t) + + dir := t.TempDir() + partPath := filepath.Join(dir, "chunk_00000.part") + + retrier := &chunkRetrier{policy: fastChunkRetryPolicy()} + + err := retrier.fetchChunk(context.Background(), nil, slog.Default(), fetcher, blockURL, + partPath, 0, 0, 19, 20, nil) + if err == nil { + t.Fatal("expected a fatal error, got nil") + } + + if !errors.Is(err, tc.wantErr) { + t.Errorf("expected errors.Is(err, %v), got: %v", tc.wantErr, err) + } + + if got := callCount(); got != 1 { + t.Errorf("expected exactly 1 request for a fatal error, got %d", got) + } + }) + } +} + +// TestChunkRetrier_DoesNotRetryLocalWriteError proves a local filesystem +// failure (unreachable from exporter.IsTransientDataPlaneError's allow-list) +// is fatal on the first attempt, exactly like a rejected/mismatched response. +func TestChunkRetrier_DoesNotRetryLocalWriteError(t *testing.T) { + t.Parallel() + + payload := []byte("0123456789ABCDEFGHIJ") + + srv := newRangeServer(t, payload) + blockURL := srv.URL + "/block" + + doer := &scriptedRangeDoer{inner: srv.Client()} + fetcher := exporter.NewFetcher(doer) + + // partPath's parent directory does not exist, so opening it for append + // fails with a local *os.PathError — not in the transient allow-list. + partPath := filepath.Join(t.TempDir(), "missing-parent", "chunk_00000.part") + + retrier := &chunkRetrier{policy: fastChunkRetryPolicy()} + + rawLen := int64(len(payload)) + + err := retrier.fetchChunk(context.Background(), nil, slog.Default(), fetcher, blockURL, + partPath, 0, 0, rawLen-1, rawLen, nil) + if err == nil { + t.Fatal("expected a fatal error for a local write failure, got nil") + } + + var pathErr *os.PathError + if !errors.As(err, &pathErr) { + t.Errorf("expected the error to unwrap to *os.PathError, got: %v", err) + } + + if got := doer.callCount(); got != 1 { + t.Errorf("expected exactly 1 request before the local write failure, got %d", got) + } +} + +// TestChunkRetrier_ContextCancelStopsRetryImmediately proves that cancelling +// ctx mid-backoff aborts the retry loop promptly instead of waiting out the +// full backoff budget. +func TestChunkRetrier_ContextCancelStopsRetryImmediately(t *testing.T) { + t.Parallel() + + payload := []byte("0123456789ABCDEFGHIJ") + + srv := newRangeServer(t, payload) + blockURL := srv.URL + "/block" + + doer := &scriptedRangeDoer{inner: srv.Client(), cutBytes: 2, cutErr: io.ErrUnexpectedEOF} + fetcher := exporter.NewFetcher(doer) + + dir := t.TempDir() + partPath := filepath.Join(dir, "chunk_00000.part") + + // A long backoff so cancellation, not a natural step timeout, is what + // ends the loop. + longBackoffPolicy := chunkRetryPolicy{ + backoff: wait.Backoff{ + Steps: 6, + Duration: 10 * time.Second, + Factor: 2, + Cap: time.Minute, + }, + maxNoProgress: 3, + } + retrier := &chunkRetrier{policy: longBackoffPolicy} + + ctx, cancel := context.WithCancel(context.Background()) + time.AfterFunc(20*time.Millisecond, cancel) + + rawLen := int64(len(payload)) + + start := time.Now() + + err := retrier.fetchChunk(ctx, nil, slog.Default(), fetcher, blockURL, + partPath, 0, 0, rawLen-1, rawLen, nil) + + elapsed := time.Since(start) + + if err == nil { + t.Fatal("expected an error after context cancellation, got nil") + } + + if !errors.Is(err, context.Canceled) { + t.Errorf("expected errors.Is(err, context.Canceled), got: %v", err) + } + + if elapsed > 2*time.Second { + t.Errorf("cancellation took %s, expected it to interrupt the 10s backoff sleep promptly", elapsed) + } + + if got := doer.callCount(); got != 1 { + t.Errorf("expected exactly 1 request before cancellation stopped the retry, got %d", got) + } +} + +// TestChunkRetrier_BoundsNoProgressAttempts proves a server that accepts the +// range but delivers zero bytes on every attempt is stopped after +// maxNoProgress attempts, not the (larger) Steps budget. +func TestChunkRetrier_BoundsNoProgressAttempts(t *testing.T) { + t.Parallel() + + payload := []byte("0123456789ABCDEFGHIJ") + + srv := newRangeServer(t, payload) + blockURL := srv.URL + "/block" + + // Every attempt is cut after 0 bytes: the durable offset never advances. + doer := &scriptedRangeDoer{inner: srv.Client(), cutBytes: 0, cutErr: io.ErrUnexpectedEOF} + fetcher := exporter.NewFetcher(doer) + + dir := t.TempDir() + partPath := filepath.Join(dir, "chunk_00000.part") + + policy := chunkRetryPolicy{ + backoff: wait.Backoff{ + Steps: 6, // larger than maxNoProgress: no-progress must stop it first + Duration: time.Millisecond, + Factor: 2, + Cap: 50 * time.Millisecond, // see fastChunkRetryPolicy: keep well above the growth curve + }, + maxNoProgress: 3, + } + retrier := &chunkRetrier{policy: policy} + + rawLen := int64(len(payload)) + + err := retrier.fetchChunk(context.Background(), nil, slog.Default(), fetcher, blockURL, + partPath, 0, 0, rawLen-1, rawLen, nil) + if err == nil { + t.Fatal("expected an error once no-progress attempts are exhausted, got nil") + } + + if !strings.Contains(err.Error(), "no progress") { + t.Errorf("expected the error to name the no-progress condition, got: %v", err) + } + + // The very first attempt always establishes a baseline durable offset (0 + // bytes is still "progress" relative to no prior attempt at all — see + // chunkRetrier.fetchChunk's lastDurable := -1 sentinel), so it takes + // maxNoProgress+1 total attempts before maxNoProgress CONSECUTIVE + // zero-advancement attempts have actually occurred. + wantCalls := policy.maxNoProgress + 1 + if got := doer.callCount(); got != wantCalls { + t.Errorf("expected exactly %d requests (maxNoProgress+1 for the baseline attempt), got %d", wantCalls, got) + } +} + +// TestChunkProgressLedger_MonotonicAcrossAttempts is a pure unit test of +// chunkProgressLedger: it feeds three overlapping attempt credit sequences +// (each attempt re-crediting the prefix a previous, interrupted attempt +// already reported) and checks the ledger forwards only the strictly new +// suffix each time, summing to exactly rawLen with no double counting. +func TestChunkProgressLedger_MonotonicAcrossAttempts(t *testing.T) { + t.Parallel() + + const rawLen = 20 + + var forwarded []int + + ledger := &chunkProgressLedger{ + onProgress: func(n int) { forwarded = append(forwarded, n) }, + } + + // Attempt 1: resumes from 0, streams 5 then 3 bytes before breaking at 8. + ledger.beginAttempt() + ledger.credit(5) + ledger.credit(3) + + // Attempt 2: resumes from the now-durable 8, re-credits that prefix, then + // streams 4 more bytes before breaking at 12. + ledger.beginAttempt() + ledger.credit(8) + ledger.credit(4) + + // Attempt 3: resumes from 12, re-credits that prefix, then streams the + // remaining 8 bytes to complete the chunk. + ledger.beginAttempt() + ledger.credit(12) + ledger.credit(8) + + var sum int + + for _, n := range forwarded { + if n <= 0 { + t.Errorf("forwarded a non-positive credit: %d (all: %v)", n, forwarded) + } + + sum += n + } + + if sum != rawLen { + t.Errorf("forwarded credits summed to %d, want %d (rawLen): %v", sum, rawLen, forwarded) + } + + // The re-credited prefixes (8 and 12) must never have been forwarded at + // all: only the genuinely new suffix each attempt contributes is. + want := []int{5, 3, 4, 8} + + if len(forwarded) != len(want) { + t.Fatalf("forwarded = %v, want %v", forwarded, want) + } + + for i, n := range forwarded { + if n != want[i] { + t.Errorf("forwarded[%d] = %d, want %d (all: %v)", i, n, want[i], forwarded) + } + } +} + +// statusDoerFetcher builds a Fetcher whose RangeGet always fails with the +// given HTTP status. +func statusDoerFetcher(t *testing.T, status int) (*exporter.Fetcher, string, func() int) { + t.Helper() + + var calls int + + var mu sync.Mutex + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + mu.Lock() + calls++ + mu.Unlock() + + w.WriteHeader(status) + })) + t.Cleanup(srv.Close) + + fetcher := exporter.NewFetcher(srv.Client()) + + return fetcher, srv.URL, func() int { + mu.Lock() + defer mu.Unlock() + + return calls + } +} + +// mismatchedRangeFetcher builds a Fetcher whose RangeGet always returns 206 +// with a Content-Range header that does not match the requested range. +func mismatchedRangeFetcher(t *testing.T) (*exporter.Fetcher, string, func() int) { + t.Helper() + + var calls int + + var mu sync.Mutex + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + mu.Lock() + calls++ + mu.Unlock() + + w.Header().Set("Content-Range", "bytes 100-119/200") + w.WriteHeader(http.StatusPartialContent) + _, _ = w.Write(make([]byte, 20)) + })) + t.Cleanup(srv.Close) + + fetcher := exporter.NewFetcher(srv.Client()) + + return fetcher, srv.URL, func() int { + mu.Lock() + defer mu.Unlock() + + return calls + } +} + +// shortReadFetcher builds a Fetcher whose RangeGet always returns a +// correctly-ranged 206 that ends in a clean EOF short of the promised range, +// standing in for a server that lied about how much data it would send. +func shortReadFetcher(t *testing.T) (*exporter.Fetcher, string, func() int) { + t.Helper() + + payload := []byte("0123456789ABCDEFGHIJ") // 20 bytes + + srv := newRangeServer(t, payload) + + doer := &scriptedRangeDoer{inner: srv.Client(), cutBytes: 10, cutErr: io.EOF} + fetcher := exporter.NewFetcher(doer) + + return fetcher, srv.URL + "/block", doer.callCount +} From 25989dbeeef8178af45191ff7cefc05f37aea56a Mon Sep 17 00:00:00 2001 From: Konstantin Kozoriz Date: Fri, 28 Aug 2026 14:58:10 +0300 Subject: [PATCH 10/13] docs(d8-snapshot): note the per-volume-concurrency workaround for flaky links --per-volume-concurrency default stays 4; the help text now hints that lowering it to 1 helps on a distant/flaky link where long-lived chunk streams keep breaking, now that a broken stream retries in place rather than failing the whole volume. Signed-off-by: Konstantin Kozoriz --- internal/snapshot/cmd/download/download.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/internal/snapshot/cmd/download/download.go b/internal/snapshot/cmd/download/download.go index 28cb9a365..958ff2628 100644 --- a/internal/snapshot/cmd/download/download.go +++ b/internal/snapshot/cmd/download/download.go @@ -134,7 +134,8 @@ receives a 401 when --publish=true.`, cmd.Flags().String(flagNode, "", "restrict download to a single node subtree; format '/' (e.g. --node DemoVirtualDisk/bk-disk-a, --node Snapshot/my-snap); the generated snapshot CR name form (e.g. DemoVirtualDiskSnapshot/nss-child-abc) is still accepted") cmd.Flags().String(flagTTL, "2h", "DataExport TTL (e.g. 2h, 30m)") cmd.Flags().Int(flagWorkers, 4, "maximum number of nodes downloaded concurrently") - cmd.Flags().Int(flagPerVolumeConcurrency, 4, "maximum parallel chunk/file downloads per volume") + cmd.Flags().Int(flagPerVolumeConcurrency, 4, "maximum parallel chunk/file downloads per volume; "+ + "lower to 1 on a distant or flaky link if long-lived streams keep breaking") cmd.Flags().Int(flagMaxParallelDownloads, 5, "global cap on concurrent whole-volume-stream downloads across all nodes (independent of --workers and --per-volume-concurrency)") cmd.Flags().String(flagVolumeCompression, compress.DefaultCodecName, "volume compression codec ("+strings.Join(compress.UserSelectableNames(), ", ")+ From f1df052eea07c7462314c11515eda104c028de81 Mon Sep 17 00:00:00 2001 From: Konstantin Kozoriz Date: Fri, 28 Aug 2026 15:10:03 +0300 Subject: [PATCH 11/13] test(d8-snapshot): exercise chunkRetrier under genuine concurrent retry TestDownloadBlockChunks_RetryIsPerChunk only ever flakes one chunk out of several clean ones, which never actually exercises concurrent increments to chunkRetrier.recovered or concurrent chunkProgressLedgers feeding the shared onProgress sink at the same time. Add two internal tests that drive several chunks through fetchChunk truly concurrently (real goroutines, one shared retrier and onProgress sink): - TestChunkRetrier_ConcurrentChunksIndependentRecoveredCount flakes every chunk exactly once at the same time and asserts recovered lands on the exact expected count, part-file contents are not cross-chunk corrupted, and the shared progress sink sums to exactly the total raw bytes with no loss or double-count. - TestChunkRetrier_ConcurrentContextCancelStopsAllRetries cancels a context shared by several chunks that are all mid-backoff at once and asserts every goroutine stops promptly instead of riding out its sleep. Both pass under -race -count=5. Signed-off-by: Konstantin Kozoriz --- .../volume/chunk_retry_internal_test.go | 272 ++++++++++++++++++ 1 file changed, 272 insertions(+) diff --git a/internal/snapshot/volume/chunk_retry_internal_test.go b/internal/snapshot/volume/chunk_retry_internal_test.go index c38986c3a..6504b76f6 100644 --- a/internal/snapshot/volume/chunk_retry_internal_test.go +++ b/internal/snapshot/volume/chunk_retry_internal_test.go @@ -19,6 +19,7 @@ package volume import ( "context" "errors" + "fmt" "io" "log/slog" "net/http" @@ -601,6 +602,277 @@ func TestChunkProgressLedger_MonotonicAcrossAttempts(t *testing.T) { } } +// pathOnceFlakyDoer wraps a real exporter.Doer and truncates (with cutErr, +// after cutBytes bytes) only the FIRST request whose URL path it sees — +// tracked per distinct path — so several concurrently-downloading chunks +// backed by DIFFERENT paths on the same doer each flake exactly once, +// independently, regardless of the order or overlap in which their requests +// actually arrive. attempts records, per path, how many requests that path +// has received (also useful for asserting "exactly one retry per chunk" +// under real concurrency, not just sequential simulation). +type pathOnceFlakyDoer struct { + inner exporter.Doer + cutBytes int64 + cutErr error + + mu sync.Mutex + triggered map[string]bool + attempts map[string]int +} + +func (d *pathOnceFlakyDoer) Do(req *http.Request) (*http.Response, error) { + path := req.URL.Path + + d.mu.Lock() + + if d.triggered == nil { + d.triggered = make(map[string]bool) + d.attempts = make(map[string]int) + } + + d.attempts[path]++ + + fireNow := !d.triggered[path] + if fireNow { + d.triggered[path] = true + } + + d.mu.Unlock() + + resp, err := d.inner.Do(req) + if err != nil { + return resp, err + } + + if fireNow { + resp.Body = &cutBody{r: resp.Body, budget: d.cutBytes, err: d.cutErr} + } + + return resp, nil +} + +// attemptsFor returns how many requests path received, for post-hoc +// assertions once all concurrent goroutines have finished. +func (d *pathOnceFlakyDoer) attemptsFor(path string) int { + d.mu.Lock() + defer d.mu.Unlock() + + return d.attempts[path] +} + +// TestChunkRetrier_ConcurrentChunksIndependentRecoveredCount proves that +// chunkRetrier.recovered — the single atomic counter shared by every +// concurrently-downloading chunk in a volume — accumulates correctly when +// MULTIPLE chunks are actually retrying AT THE SAME TIME (not one flaky +// chunk against a background of clean ones), and that the shared onProgress +// sink these concurrent goroutines all feed sums to exactly the total raw +// bytes with no lost or double-counted credits. Run with -race: the only +// thing keeping this safe is recovered's atomic.Int64 and onProgress's own +// internal synchronization, both of which this test exercises under genuine +// goroutine-level concurrency (an errgroup, not a sequential loop). +func TestChunkRetrier_ConcurrentChunksIndependentRecoveredCount(t *testing.T) { + t.Parallel() + + const numChunks = 8 + + payloads := make([][]byte, numChunks) + mux := http.NewServeMux() + + for i := range numChunks { + // Distinct sizes so a chunk's content can't accidentally match another + // chunk's if the retry logic ever mixed up which durable file belongs + // to which goroutine. + payloads[i] = []byte(strings.Repeat(fmt.Sprintf("%d", i), 10+i)) + + path := fmt.Sprintf("/chunk/%d", i) + data := payloads[i] + + mux.HandleFunc(path, func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/octet-stream") + http.ServeContent(w, r, "data.img", time.Time{}, strings.NewReader(string(data))) + }) + } + + srv := httptest.NewServer(mux) + t.Cleanup(srv.Close) + + doer := &pathOnceFlakyDoer{cutBytes: 3, cutErr: io.ErrUnexpectedEOF} + doer.inner = srv.Client() + fetcher := exporter.NewFetcher(doer) + + retrier := &chunkRetrier{policy: fastChunkRetryPolicy()} + + var ( + progressMu sync.Mutex + total int + ) + + onProgress := func(n int) { + progressMu.Lock() + total += n + progressMu.Unlock() + } + + dir := t.TempDir() + + var wg sync.WaitGroup + + errs := make([]error, numChunks) + + for i := range numChunks { + wg.Add(1) + + go func(idx int) { + defer wg.Done() + + rawLen := int64(len(payloads[idx])) + partPath := filepath.Join(dir, fmt.Sprintf("chunk_%05d.part", idx)) + blockURL := srv.URL + fmt.Sprintf("/chunk/%d", idx) + + errs[idx] = retrier.fetchChunk(context.Background(), nil, slog.Default(), fetcher, + blockURL, partPath, idx, 0, rawLen-1, rawLen, onProgress) + }(i) + } + + wg.Wait() + + var wantTotal int + + for i := range numChunks { + if errs[i] != nil { + t.Errorf("chunk %d: fetchChunk failed: %v", i, errs[i]) + } + + partPath := filepath.Join(dir, fmt.Sprintf("chunk_%05d.part", i)) + + got, readErr := os.ReadFile(partPath) + if readErr != nil { + t.Errorf("chunk %d: read part file: %v", i, readErr) + continue + } + + if string(got) != string(payloads[i]) { + t.Errorf("chunk %d: part file content = %q, want %q (cross-chunk corruption?)", i, got, payloads[i]) + } + + wantTotal += len(payloads[i]) + + if got := doer.attemptsFor(fmt.Sprintf("/chunk/%d", i)); got != 2 { + t.Errorf("chunk %d: expected exactly 2 attempts (flaky + resumed retry), got %d", i, got) + } + } + + if recovered := retrier.recovered.Load(); recovered != numChunks { + t.Errorf("recovered = %d, want %d (one retry credited per concurrently-flaking chunk)", recovered, numChunks) + } + + progressMu.Lock() + defer progressMu.Unlock() + + if total != wantTotal { + t.Errorf("shared onProgress sink summed to %d, want %d (sum of all chunks' raw lengths, no loss or double-count under concurrency)", total, wantTotal) + } +} + +// TestChunkRetrier_ConcurrentContextCancelStopsAllRetries proves that +// cancelling a context SHARED by several chunks that are all mid-backoff at +// the same time stops every one of them promptly — not just the single +// goroutine that happens to observe the cancellation first, and not after +// each independently exhausts its own sleep. This generalizes +// TestChunkRetrier_ContextCancelStopsRetryImmediately (one chunk, one +// goroutine) to genuine concurrent retry. +func TestChunkRetrier_ConcurrentContextCancelStopsAllRetries(t *testing.T) { + t.Parallel() + + const numChunks = 6 + + payloads := make([][]byte, numChunks) + mux := http.NewServeMux() + + for i := range numChunks { + payloads[i] = []byte(strings.Repeat("x", 20)) + + path := fmt.Sprintf("/chunk/%d", i) + data := payloads[i] + + mux.HandleFunc(path, func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/octet-stream") + http.ServeContent(w, r, "data.img", time.Time{}, strings.NewReader(string(data))) + }) + } + + srv := httptest.NewServer(mux) + t.Cleanup(srv.Close) + + // Every chunk's first attempt is truncated, driving it into backoff. A + // long backoff (same shape as the single-chunk cancellation test) means a + // natural step timeout can never be what ends the loop — only the shared + // ctx cancellation below can. + doer := &pathOnceFlakyDoer{cutBytes: 2, cutErr: io.ErrUnexpectedEOF} + doer.inner = srv.Client() + fetcher := exporter.NewFetcher(doer) + + longBackoffPolicy := chunkRetryPolicy{ + backoff: wait.Backoff{ + Steps: 6, + Duration: 10 * time.Second, + Factor: 2, + Cap: time.Minute, + }, + maxNoProgress: 3, + } + retrier := &chunkRetrier{policy: longBackoffPolicy} + + ctx, cancel := context.WithCancel(context.Background()) + time.AfterFunc(30*time.Millisecond, cancel) + + dir := t.TempDir() + + var wg sync.WaitGroup + + errs := make([]error, numChunks) + + start := time.Now() + + for i := range numChunks { + wg.Add(1) + + go func(idx int) { + defer wg.Done() + + rawLen := int64(len(payloads[idx])) + partPath := filepath.Join(dir, fmt.Sprintf("chunk_%05d.part", idx)) + blockURL := srv.URL + fmt.Sprintf("/chunk/%d", idx) + + errs[idx] = retrier.fetchChunk(ctx, nil, slog.Default(), fetcher, + blockURL, partPath, idx, 0, rawLen-1, rawLen, nil) + }(i) + } + + wg.Wait() + + elapsed := time.Since(start) + + if elapsed > 2*time.Second { + t.Errorf("all %d concurrent retries took %s to stop after cancellation, expected them to interrupt their 10s backoff sleeps promptly", numChunks, elapsed) + } + + for i := range numChunks { + if errs[i] == nil { + t.Errorf("chunk %d: expected an error after context cancellation, got nil", i) + continue + } + + if !errors.Is(errs[i], context.Canceled) { + t.Errorf("chunk %d: expected errors.Is(err, context.Canceled), got: %v", i, errs[i]) + } + + if got := doer.attemptsFor(fmt.Sprintf("/chunk/%d", i)); got != 1 { + t.Errorf("chunk %d: expected exactly 1 request before cancellation stopped the retry, got %d", i, got) + } + } +} + // statusDoerFetcher builds a Fetcher whose RangeGet always fails with the // given HTTP status. func statusDoerFetcher(t *testing.T, status int) (*exporter.Fetcher, string, func() int) { From b8dc99dfe74ba98f8b187e71f56404d1d46f2f3c Mon Sep 17 00:00:00 2001 From: Konstantin Kozoriz Date: Fri, 28 Aug 2026 15:46:32 +0300 Subject: [PATCH 12/13] fix(d8-snapshot): use actual attempt count, not declared Steps, in chunk retry logging MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit wait.Backoff.Cap forces chunkRetrier.fetchChunk's retry loop to stop one or more attempts short of the policy's declared Steps budget, but the code compared against Steps in two places: the WARN guard (which never actually suppressed the last attempt's log line once Cap cut the loop short) and the exhausted-budget error message (which always reported the declared budget, never the real attempt count). Remove the guard entirely — the closure has no way to observe Cap's early cutoff since wait.ExponentialBackoffWithContext mutates its own copy of backoff — and report the real attempt count in the error message instead. Also unwrap the %w chain before taking %T in the non-retryable-error diagnostic, since fetchChunkRaw always wraps its errors and the previous code only ever logged *fmt.wrapError. Signed-off-by: Konstantin Kozoriz --- internal/snapshot/volume/chunk_retry.go | 51 ++++++-- .../volume/chunk_retry_internal_test.go | 112 ++++++++++++++++++ 2 files changed, 154 insertions(+), 9 deletions(-) diff --git a/internal/snapshot/volume/chunk_retry.go b/internal/snapshot/volume/chunk_retry.go index 41116f094..d82db38f5 100644 --- a/internal/snapshot/volume/chunk_retry.go +++ b/internal/snapshot/volume/chunk_retry.go @@ -18,6 +18,7 @@ package volume import ( "context" + "errors" "fmt" "log/slog" "sync/atomic" @@ -162,7 +163,6 @@ func (r *chunkRetrier) fetchChunk( // by value): every concurrent chunk goroutine gets its own independent // copy to mutate, never the shared r.policy.backoff itself. backoff := r.policy.backoff - steps := backoff.Steps backoffErr := wait.ExponentialBackoffWithContext(ctx, backoff, func(stepCtx context.Context) (bool, error) { attempt++ @@ -181,9 +181,16 @@ func (r *chunkRetrier) fetchChunk( // ordinary transient failure, and must not be retried. return false, err case !exporter.IsTransientDataPlaneError(err): + // fetchChunkRaw wraps every error it returns through fmt.Errorf's + // %w, so %T on err itself always reports *fmt.wrapError and never + // the concrete error this classifier didn't recognize. Unwrap to + // the root cause first so this diagnostic can actually inform a + // future addition to exporter.IsTransientDataPlaneError's + // allow-list. This is the only place err is logged: the caller + // receives it back unlogged. log.Debug("chunk fetch failed with a non-retryable error", slog.Int("chunk", chunkIdx), - slog.String("error_type", fmt.Sprintf("%T", err))) + slog.String("error_type", fmt.Sprintf("%T", rootCause(err)))) return false, err } @@ -205,12 +212,20 @@ func (r *chunkRetrier) fetchChunk( r.recovered.Add(1) - if attempt < steps { - log.Warn("retrying chunk after a transient transport failure", - slog.Int("chunk", chunkIdx), - slog.Int("attempt", attempt), - slog.String("error", err.Error())) - } + // wait.ExponentialBackoffWithContext mutates its OWN copy of backoff + // (passed by value above), so this closure has no way to observe + // whether backoff's Cap has already forced the step budget to 0 and + // this attempt is in fact the last one the loop will make (see the + // chunkFetchBackoff doc comment on Cap's early-termination effect). + // Rather than approximate that with the declared (and frequently + // wrong) Steps budget, always log here: on a genuinely terminal + // attempt this is the only place the failure is ever reported, since + // the error returned once the budget is exhausted (below) is not + // logged again anywhere in this call chain. + log.Warn("retrying chunk after a transient transport failure", + slog.Int("chunk", chunkIdx), + slog.Int("attempt", attempt), + slog.String("error", err.Error())) return false, nil }) @@ -221,9 +236,27 @@ func (r *chunkRetrier) fetchChunk( case ctx.Err() != nil: return fmt.Errorf("chunk %d: %w", chunkIdx, ctx.Err()) case wait.Interrupted(backoffErr) && lastErr != nil: + // attempt, not the policy's declared Steps: Cap routinely forces the + // backoff loop to stop one or more attempts short of Steps (see + // chunkFetchBackoff's doc comment), so Steps would misreport how many + // attempts actually happened. return fmt.Errorf("chunk %d: exhausted %d attempts on transient transport failures: %w", - chunkIdx, steps, lastErr) + chunkIdx, attempt, lastErr) default: return backoffErr } } + +// rootCause unwraps err through every %w wrapping layer and returns the +// deepest cause, so %T on the result reports the concrete error type instead +// of the *fmt.wrapError every fetchChunkRaw call site introduces. +func rootCause(err error) error { + for { + unwrapped := errors.Unwrap(err) + if unwrapped == nil { + return err + } + + err = unwrapped + } +} diff --git a/internal/snapshot/volume/chunk_retry_internal_test.go b/internal/snapshot/volume/chunk_retry_internal_test.go index 6504b76f6..4c7e200e0 100644 --- a/internal/snapshot/volume/chunk_retry_internal_test.go +++ b/internal/snapshot/volume/chunk_retry_internal_test.go @@ -318,6 +318,118 @@ func TestChunkRetrier_ExhaustsBudget(t *testing.T) { } } +// chunkWarnCapture is a slog.Handler that collects Warn-or-above log messages +// for assertions, mirroring volume_test's warnCapture (unavailable here: this +// file is the internal test package and cannot import volume_test). +type chunkWarnCapture struct { + mu sync.Mutex + msgs []string +} + +func (h *chunkWarnCapture) Enabled(_ context.Context, _ slog.Level) bool { return true } + +func (h *chunkWarnCapture) Handle(_ context.Context, r slog.Record) error { + if r.Level >= slog.LevelWarn { + h.mu.Lock() + h.msgs = append(h.msgs, r.Message) + h.mu.Unlock() + } + + return nil +} + +func (h *chunkWarnCapture) WithAttrs(_ []slog.Attr) slog.Handler { return h } + +func (h *chunkWarnCapture) WithGroup(_ string) slog.Handler { return h } + +func (h *chunkWarnCapture) warnMessages() []string { + h.mu.Lock() + defer h.mu.Unlock() + + out := make([]string, len(h.msgs)) + copy(out, h.msgs) + + return out +} + +// TestChunkRetrier_ExhaustsBudget_CapCutsAttemptsShortOfSteps proves that when +// wait.Backoff.Cap forces the retry loop to stop before its declared Steps +// budget is reached (see chunkFetchBackoff's doc comment), fetchChunk reports +// the ACTUAL number of attempts made — never the declared Steps — in its +// returned error, and logs each transient failure exactly once: one WARN per +// attempt, and the exhausted error is never separately logged anywhere in +// this call chain once it becomes final. +func TestChunkRetrier_ExhaustsBudget_CapCutsAttemptsShortOfSteps(t *testing.T) { + t.Parallel() + + payload := make([]byte, 100) + + srv := newRangeServer(t, payload) + blockURL := srv.URL + "/block" + + doer := &scriptedRangeDoer{inner: srv.Client(), cutBytes: 5, cutErr: io.ErrUnexpectedEOF} + fetcher := exporter.NewFetcher(doer) + + dir := t.TempDir() + partPath := filepath.Join(dir, "chunk_00000.part") + + // Steps=6 alone would suggest 6 attempts, but Cap=4ms forces the internal + // step budget to 0 early: the projected delay grows 1ms -> 2ms -> 4ms, + // and the 3rd projected delay (8ms) exceeds Cap, ending the loop after + // exactly 3 real attempts — the same arithmetic chunkFetchBackoff's doc + // comment works out for the production policy (5 of 6 there). + policy := chunkRetryPolicy{ + backoff: wait.Backoff{ + Steps: 6, + Duration: time.Millisecond, + Factor: 2, + Cap: 4 * time.Millisecond, + }, + maxNoProgress: 3, + } + + warns := &chunkWarnCapture{} + log := slog.New(warns) + + retrier := &chunkRetrier{policy: policy} + + rawLen := int64(len(payload)) + + err := retrier.fetchChunk(context.Background(), nil, log, fetcher, blockURL, + partPath, 0, 0, rawLen-1, rawLen, nil) + if err == nil { + t.Fatal("expected an error once the retry budget is exhausted, got nil") + } + + if !errors.Is(err, io.ErrUnexpectedEOF) { + t.Errorf("expected errors.Is(err, io.ErrUnexpectedEOF), got: %v", err) + } + + gotCalls := doer.callCount() + if gotCalls != 3 { + t.Fatalf("expected exactly 3 requests (Cap cuts Steps=6 short), got %d", gotCalls) + } + + wantMsg := fmt.Sprintf("exhausted %d attempts", gotCalls) + if !strings.Contains(err.Error(), wantMsg) { + t.Errorf("error = %q, want it to name the actual attempt count (%q), not the declared Steps=%d", + err.Error(), wantMsg, policy.backoff.Steps) + } + + if staleMsg := fmt.Sprintf("exhausted %d attempts", policy.backoff.Steps); strings.Contains(err.Error(), staleMsg) { + t.Errorf("error = %q, must not report the declared Steps budget (%d) as the attempt count", + err.Error(), policy.backoff.Steps) + } + + // Every attempt here is a plain transient failure (never a no-progress or + // fatal one), so each is warned about exactly once: the WARN count must + // equal the number of attempts actually made — not more (no attempt + // double-logged) and not fewer (a transient attempt silently dropped). + if got := len(warns.warnMessages()); got != gotCalls { + t.Errorf("warn log count = %d, want %d (one per attempt, no double-logging)", got, gotCalls) + } +} + // TestChunkRetrier_DoesNotRetryFatal proves that every non-transient error // stops the retry loop on the very first attempt, and that errors.Is against // the original sentinel still holds through fetchChunk's returned error. From aa3be430747b7f11e7dddcd39e7b65a0dbaedc76 Mon Sep 17 00:00:00 2001 From: Konstantin Kozoriz Date: Fri, 28 Aug 2026 16:35:43 +0300 Subject: [PATCH 13/13] test(d8-snapshot): synchronize context-cancel tests on request receipt, not wall clock TestChunkRetrier_ConcurrentContextCancelStopsAllRetries and TestChunkRetrier_ContextCancelStopsRetryImmediately raced a time.AfterFunc-scheduled cancel() against goroutines that had not necessarily issued their first HTTP request yet, so the retry loop's very first backoff iteration could already observe a cancelled ctx and make zero requests instead of the expected one per chunk (~60% flake rate on the concurrent test under -race -count=5). Both tests now signal over a channel once each doer has actually seen its first request, and cancel only after every goroutine has reached that point; elapsed-time measurement starts right before cancel() so it actually reflects time-to-stop. Also add table-driven coverage for rootCause (nil, unwrapped, single- and double-wrapped errors), note its known limitation on multi-wrap (Unwrap() []error) chains, and reword the per-attempt WARN log to state a plain fact ("chunk transfer interrupted by...") instead of "retrying", since it fires on the terminal attempt too, where no retry follows. Signed-off-by: Konstantin Kozoriz --- internal/snapshot/volume/chunk_retry.go | 8 +- .../volume/chunk_retry_internal_test.go | 102 +++++++++++++++--- 2 files changed, 95 insertions(+), 15 deletions(-) diff --git a/internal/snapshot/volume/chunk_retry.go b/internal/snapshot/volume/chunk_retry.go index d82db38f5..91520fd02 100644 --- a/internal/snapshot/volume/chunk_retry.go +++ b/internal/snapshot/volume/chunk_retry.go @@ -222,7 +222,7 @@ func (r *chunkRetrier) fetchChunk( // attempt this is the only place the failure is ever reported, since // the error returned once the budget is exhausted (below) is not // logged again anywhere in this call chain. - log.Warn("retrying chunk after a transient transport failure", + log.Warn("chunk transfer interrupted by a transient transport failure", slog.Int("chunk", chunkIdx), slog.Int("attempt", attempt), slog.String("error", err.Error())) @@ -250,6 +250,12 @@ func (r *chunkRetrier) fetchChunk( // rootCause unwraps err through every %w wrapping layer and returns the // deepest cause, so %T on the result reports the concrete error type instead // of the *fmt.wrapError every fetchChunkRaw call site introduces. +// +// Known limitation: this only follows the single-error Unwrap() error chain. +// A multi-wrap error built with fmt.Errorf("%w: %w", ...) implements +// Unwrap() []error instead, which errors.Unwrap does not see, so rootCause +// stops at such a node. No call site in this package produces multi-wrap +// errors today, so this is not tightened further here. func rootCause(err error) error { for { unwrapped := errors.Unwrap(err) diff --git a/internal/snapshot/volume/chunk_retry_internal_test.go b/internal/snapshot/volume/chunk_retry_internal_test.go index 4c7e200e0..8cfb8379f 100644 --- a/internal/snapshot/volume/chunk_retry_internal_test.go +++ b/internal/snapshot/volume/chunk_retry_internal_test.go @@ -113,9 +113,10 @@ func (b *cutBody) Close() error { // body (every call, not just one) after cutBytes bytes with cutErr — standing // in for a link that breaks on every attempt. type scriptedRangeDoer struct { - inner exporter.Doer - cutBytes int64 - cutErr error // nil disables truncation + inner exporter.Doer + cutBytes int64 + cutErr error // nil disables truncation + firstSeen chan<- struct{} // optional: signaled once, on the very first Do() call mu sync.Mutex ranges []string @@ -124,8 +125,13 @@ type scriptedRangeDoer struct { func (d *scriptedRangeDoer) Do(req *http.Request) (*http.Response, error) { d.mu.Lock() d.ranges = append(d.ranges, req.Header.Get("Range")) + firstCall := len(d.ranges) == 1 d.mu.Unlock() + if firstCall && d.firstSeen != nil { + d.firstSeen <- struct{}{} + } + resp, err := d.inner.Do(req) if err != nil { return resp, err @@ -552,7 +558,8 @@ func TestChunkRetrier_ContextCancelStopsRetryImmediately(t *testing.T) { srv := newRangeServer(t, payload) blockURL := srv.URL + "/block" - doer := &scriptedRangeDoer{inner: srv.Client(), cutBytes: 2, cutErr: io.ErrUnexpectedEOF} + firstSeen := make(chan struct{}, 1) + doer := &scriptedRangeDoer{inner: srv.Client(), cutBytes: 2, cutErr: io.ErrUnexpectedEOF, firstSeen: firstSeen} fetcher := exporter.NewFetcher(doer) dir := t.TempDir() @@ -572,14 +579,31 @@ func TestChunkRetrier_ContextCancelStopsRetryImmediately(t *testing.T) { retrier := &chunkRetrier{policy: longBackoffPolicy} ctx, cancel := context.WithCancel(context.Background()) - time.AfterFunc(20*time.Millisecond, cancel) + t.Cleanup(cancel) rawLen := int64(len(payload)) + var err error + + done := make(chan struct{}) + + go func() { + defer close(done) + + err = retrier.fetchChunk(ctx, nil, slog.Default(), fetcher, blockURL, + partPath, 0, 0, rawLen-1, rawLen, nil) + }() + + // Wait for the first request to actually be in flight before cancelling: + // only then is "cancel stops an in-flight backoff" the guaranteed + // condition under test, instead of a race against goroutine scheduling. + <-firstSeen + start := time.Now() - err := retrier.fetchChunk(ctx, nil, slog.Default(), fetcher, blockURL, - partPath, 0, 0, rawLen-1, rawLen, nil) + cancel() + + <-done elapsed := time.Since(start) @@ -714,6 +738,40 @@ func TestChunkProgressLedger_MonotonicAcrossAttempts(t *testing.T) { } } +// TestRootCause proves rootCause unwraps a chain of %w-wrapped errors down to +// the deepest cause, and passes both nil and an already-unwrapped error +// through unchanged. +func TestRootCause(t *testing.T) { + t.Parallel() + + base := errors.New("base failure") + + tests := []struct { + name string + err error + want error + }{ + {name: "nil error returns nil", err: nil, want: nil}, + {name: "unwrapped error returns itself", err: base, want: base}, + {name: "single wrap returns the base", err: fmt.Errorf("attempt 1: %w", base), want: base}, + { + name: "double wrap returns the deepest base", + err: fmt.Errorf("attempt 2: %w", fmt.Errorf("attempt 1: %w", base)), + want: base, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + if got := rootCause(tc.err); got != tc.want { + t.Errorf("rootCause(%v) = %v, want %v", tc.err, got, tc.want) + } + }) + } +} + // pathOnceFlakyDoer wraps a real exporter.Doer and truncates (with cutErr, // after cutBytes bytes) only the FIRST request whose URL path it sees — // tracked per distinct path — so several concurrently-downloading chunks @@ -723,9 +781,10 @@ func TestChunkProgressLedger_MonotonicAcrossAttempts(t *testing.T) { // has received (also useful for asserting "exactly one retry per chunk" // under real concurrency, not just sequential simulation). type pathOnceFlakyDoer struct { - inner exporter.Doer - cutBytes int64 - cutErr error + inner exporter.Doer + cutBytes int64 + cutErr error + firstSeen chan<- string // optional: signaled with path on that path's first Do() call mu sync.Mutex triggered map[string]bool @@ -751,6 +810,10 @@ func (d *pathOnceFlakyDoer) Do(req *http.Request) (*http.Response, error) { d.mu.Unlock() + if fireNow && d.firstSeen != nil { + d.firstSeen <- path + } + resp, err := d.inner.Do(req) if err != nil { return resp, err @@ -920,7 +983,8 @@ func TestChunkRetrier_ConcurrentContextCancelStopsAllRetries(t *testing.T) { // long backoff (same shape as the single-chunk cancellation test) means a // natural step timeout can never be what ends the loop — only the shared // ctx cancellation below can. - doer := &pathOnceFlakyDoer{cutBytes: 2, cutErr: io.ErrUnexpectedEOF} + firstSeen := make(chan string, numChunks) + doer := &pathOnceFlakyDoer{cutBytes: 2, cutErr: io.ErrUnexpectedEOF, firstSeen: firstSeen} doer.inner = srv.Client() fetcher := exporter.NewFetcher(doer) @@ -936,7 +1000,7 @@ func TestChunkRetrier_ConcurrentContextCancelStopsAllRetries(t *testing.T) { retrier := &chunkRetrier{policy: longBackoffPolicy} ctx, cancel := context.WithCancel(context.Background()) - time.AfterFunc(30*time.Millisecond, cancel) + t.Cleanup(cancel) dir := t.TempDir() @@ -944,8 +1008,6 @@ func TestChunkRetrier_ConcurrentContextCancelStopsAllRetries(t *testing.T) { errs := make([]error, numChunks) - start := time.Now() - for i := range numChunks { wg.Add(1) @@ -961,6 +1023,18 @@ func TestChunkRetrier_ConcurrentContextCancelStopsAllRetries(t *testing.T) { }(i) } + // Wait until every one of the numChunks goroutines has actually issued its + // first request before cancelling: only then is "cancel stops an in-flight + // backoff" the guaranteed condition under test, instead of a race against + // goroutine scheduling. + for range numChunks { + <-firstSeen + } + + start := time.Now() + + cancel() + wg.Wait() elapsed := time.Since(start)