From 9a41a8b71ecadc8cbee9e168b197dfb763669b74 Mon Sep 17 00:00:00 2001 From: Konstantin Kozoriz Date: Tue, 25 Aug 2026 15:07:16 +0300 Subject: [PATCH 01/12] 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/12] 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/12] 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/12] 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/12] 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/12] 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/12] 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 8895687888665a29361bc78df3378dc65db96e43 Mon Sep 17 00:00:00 2001 From: Konstantin Kozoriz Date: Fri, 28 Aug 2026 14:33:18 +0300 Subject: [PATCH 08/12] fix(data): enforce TLS verification in the safe HTTP client's CA merge 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, so certificate verification was a no-op on the d8 data export/import transports and the kubeconfig bearer token could be sent to any endpoint answering that address. Force verification on and clear the inherited tls-server-name at both levels — the rest.Config and the transport clone — since client-go bakes the insecure flag into the base transport before WrapTransport runs. Also chain a previously installed WrapTransport instead of clobbering it, and return the input RoundTripper rather than a typed-nil *http.Transport for non-transport RoundTrippers. Both mirror the already-fixed twin in internal/snapshot/transport, which is now cross-referenced from each side. Signed-off-by: Konstantin Kozoriz --- internal/snapshot/transport/http.go | 3 + pkg/libsaferequest/client/http.go | 49 +++++- pkg/libsaferequest/client/http_test.go | 229 +++++++++++++++++++++++++ 3 files changed, 275 insertions(+), 6 deletions(-) diff --git a/internal/snapshot/transport/http.go b/internal/snapshot/transport/http.go index 97de8e621..f8d91e4cd 100644 --- a/internal/snapshot/transport/http.go +++ b/internal/snapshot/transport/http.go @@ -1195,6 +1195,9 @@ func (c *Client) NewRTClient(schemeFuncs ...func(s *apiruntime.Scheme) error) (c // 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. +// +// Keep in sync with the twin implementation in pkg/libsaferequest/client/http.go +// (SafeClient.SetTLSCAData); the two are deliberately separate copies. func (c *Client) SetTLSCAData(caData []byte) { sysPool, err := x509.SystemCertPool() if err != nil || sysPool == nil { diff --git a/pkg/libsaferequest/client/http.go b/pkg/libsaferequest/client/http.go index a3c416ca7..94bf01979 100644 --- a/pkg/libsaferequest/client/http.go +++ b/pkg/libsaferequest/client/http.go @@ -177,6 +177,30 @@ func (c *SafeClient) NewRTClient(schemeFuncs ...func(s *apiruntime.Scheme) error return kubeRtClient, nil } +// SetTLSCAData extends inherited server trust with a merged pool: system roots, +// the supplied caData, and any CA already configured on this SafeClient's +// rest.Config (e.g. from kubeconfig). It is used for endpoints that cannot be +// pinned to a single CA — the d8 data export/import transports, both direct +// (exporter pod, internal CA from DataExport/DataImport status) and published +// (ingress, empty CA). +// +// Because a 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 (Insecure) or check the +// wrong hostname (ServerName) regardless of how large RootCAs is, so both are +// forced off here — on the rest.Config itself and, since client-go may already +// have built a base *http.Transport with those values baked in before this +// WrapTransport runs, again on the transport clone that carries RootCAs. This +// runs unconditionally, not only for a non-empty caData: an empty caData is the +// normal case on the publish path, and verification must stay on even then. +// +// Clearing ServerName is also why SetTLSCAData must not be called after +// SetProbeEndpoint on the same client — the probe sets ServerName to the +// kubernetes service name on purpose. Today they never meet: the probe runs on +// its own Copy() (internal/data/publish_detect.go). +// +// Keep in sync with the twin implementation in internal/snapshot/transport/http.go +// (Client.SetTLSCAData); the two are deliberately separate copies. func (c *SafeClient) SetTLSCAData(caData []byte) { sysPool, err := x509.SystemCertPool() if err != nil || sysPool == nil { @@ -193,20 +217,33 @@ func (c *SafeClient) 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 { + if prev != nil { + rt = prev(rt) + } + transport, ok := rt.(*http.Transport) if !ok { - return transport + // CA-pool injection is a best-effort enhancement over *http.Transport; + // for any other RoundTripper degrade to pass-through so we never hand + // back a typed-nil transport that nil-panics on RoundTrip. + return rt } - clonedTrasport := transport.Clone() - if clonedTrasport.TLSClientConfig == nil { - clonedTrasport.TLSClientConfig = &tls.Config{} + clonedTransport := transport.Clone() + if clonedTransport.TLSClientConfig == nil { + clonedTransport.TLSClientConfig = &tls.Config{} } - clonedTrasport.TLSClientConfig.RootCAs = sysPool + clonedTransport.TLSClientConfig.RootCAs = sysPool + clonedTransport.TLSClientConfig.InsecureSkipVerify = false + clonedTransport.TLSClientConfig.ServerName = "" - return clonedTrasport + return clonedTransport } } diff --git a/pkg/libsaferequest/client/http_test.go b/pkg/libsaferequest/client/http_test.go index f62b27bed..290537f08 100644 --- a/pkg/libsaferequest/client/http_test.go +++ b/pkg/libsaferequest/client/http_test.go @@ -17,6 +17,10 @@ limitations under the License. package client import ( + "crypto/tls" + "encoding/pem" + "net/http" + "net/http/httptest" "testing" "time" @@ -59,3 +63,228 @@ func TestNewSafeClientForConfig(t *testing.T) { NewSafeClientForConfig(nil) }) } + +// testCACertificatePEM spins up a throwaway TLS server and returns its leaf +// certificate PEM-encoded, giving tests a syntactically valid CA bundle +// without depending on a real one. +func testCACertificatePEM(t *testing.T) []byte { + t.Helper() + + srv := httptest.NewTLSServer(http.HandlerFunc(func(_ http.ResponseWriter, _ *http.Request) {})) + t.Cleanup(srv.Close) + + return pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: srv.Certificate().Raw}) +} + +// stubRoundTripper is a non-*http.Transport RoundTripper used to exercise the +// pass-through branch of SetTLSCAData's WrapTransport wrapper. +type stubRoundTripper struct{} + +// RoundTrip always fails; stubRoundTripper is never actually used to send a +// request in these tests. +func (stubRoundTripper) RoundTrip(_ *http.Request) (*http.Response, error) { + return nil, http.ErrNotSupported +} + +func TestSafeClient_SetTLSCAData_ForcesVerification(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + caData []byte + presetCAData []byte + }{ + {name: "nil CA data", caData: nil}, + {name: "valid CA data", caData: testCACertificatePEM(t)}, + {name: "kubeconfig CA data", caData: nil, presetCAData: testCACertificatePEM(t)}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + sc := &SafeClient{restConfig: &rest.Config{ + TLSClientConfig: rest.TLSClientConfig{ + Insecure: true, + ServerName: "api.example", + CAData: tc.presetCAData, + }, + }} + + sc.SetTLSCAData(tc.caData) + + if sc.restConfig.TLSClientConfig.Insecure { + t.Error("TLSClientConfig.Insecure = true, want false") + } + + if sc.restConfig.TLSClientConfig.ServerName != "" { + t.Errorf("TLSClientConfig.ServerName = %q, want empty", sc.restConfig.TLSClientConfig.ServerName) + } + + if sc.restConfig.TLSClientConfig.CAData != nil { + t.Errorf("TLSClientConfig.CAData = %v, want nil", sc.restConfig.TLSClientConfig.CAData) + } + + if sc.restConfig.TLSClientConfig.CAFile != "" { + t.Errorf("TLSClientConfig.CAFile = %q, want empty", sc.restConfig.TLSClientConfig.CAFile) + } + + // exercising the inherited-insecure bypass this test guards against + orig := &http.Transport{TLSClientConfig: &tls.Config{InsecureSkipVerify: true, ServerName: "api.example"}} + + wrapped := sc.restConfig.WrapTransport(orig) + + clonedTransport, ok := wrapped.(*http.Transport) + if !ok { + t.Fatalf("wrapped transport is %T, want *http.Transport", wrapped) + } + + if clonedTransport.TLSClientConfig.InsecureSkipVerify { + t.Error("cloned TLSClientConfig.InsecureSkipVerify = true, want false") + } + + if clonedTransport.TLSClientConfig.ServerName != "" { + t.Errorf("cloned TLSClientConfig.ServerName = %q, want empty", clonedTransport.TLSClientConfig.ServerName) + } + + if clonedTransport.TLSClientConfig.RootCAs == nil { + t.Error("cloned TLSClientConfig.RootCAs = nil, want non-nil") + } + + if !orig.TLSClientConfig.InsecureSkipVerify { + t.Error("orig.TLSClientConfig.InsecureSkipVerify was mutated, want unchanged (true)") + } + + if orig.TLSClientConfig.ServerName != "api.example" { + t.Errorf("orig.TLSClientConfig.ServerName = %q, want unchanged (%q)", orig.TLSClientConfig.ServerName, "api.example") + } + }) + } +} + +func TestSafeClient_SetTLSCAData_PassThroughNonTransport(t *testing.T) { + t.Parallel() + + sc := NewSafeClientForConfig(&rest.Config{}) + sc.SetTLSCAData(nil) + + got := sc.restConfig.WrapTransport(stubRoundTripper{}) + if got == nil { + t.Fatal("wrapped RoundTripper = nil, want non-nil") + } + + stub, ok := got.(stubRoundTripper) + if !ok { + t.Fatalf("wrapped RoundTripper is %T, want stubRoundTripper", got) + } + + if stub != (stubRoundTripper{}) { + t.Error("wrapped RoundTripper is not the same stubRoundTripper instance") + } +} + +func TestSafeClient_SetTLSCAData_ChainsExistingWrapTransport(t *testing.T) { + t.Parallel() + + t.Run("success: prior wrapper and CA injection both survive", func(t *testing.T) { + t.Parallel() + + called := false + sc := NewSafeClientForConfig(&rest.Config{}) + sc.restConfig.WrapTransport = func(rt http.RoundTripper) http.RoundTripper { + called = true + + transport, ok := rt.(*http.Transport) + if !ok { + return rt + } + + clonedTransport := transport.Clone() + clonedTransport.ResponseHeaderTimeout = 42 * time.Millisecond + + return clonedTransport + } + + sc.SetTLSCAData(nil) + + wrapped := sc.restConfig.WrapTransport(&http.Transport{}) + + if !called { + t.Error("prior WrapTransport was not called") + } + + clonedTransport, ok := wrapped.(*http.Transport) + if !ok { + t.Fatalf("wrapped transport is %T, want *http.Transport", wrapped) + } + + if clonedTransport.ResponseHeaderTimeout != 42*time.Millisecond { + t.Errorf("ResponseHeaderTimeout = %v, want %v", clonedTransport.ResponseHeaderTimeout, 42*time.Millisecond) + } + + if clonedTransport.TLSClientConfig == nil || clonedTransport.TLSClientConfig.RootCAs == nil { + t.Error("TLSClientConfig.RootCAs = nil, want non-nil") + } + }) + + t.Run("success: prior wrapper returning a non-transport degrades to pass-through", func(t *testing.T) { + t.Parallel() + + sc := NewSafeClientForConfig(&rest.Config{}) + sc.restConfig.WrapTransport = func(_ http.RoundTripper) http.RoundTripper { + return stubRoundTripper{} + } + + sc.SetTLSCAData(nil) + + got := sc.restConfig.WrapTransport(&http.Transport{}) + if got == nil { + t.Fatal("wrapped RoundTripper = nil, want non-nil") + } + + if _, ok := got.(stubRoundTripper); !ok { + t.Fatalf("wrapped RoundTripper is %T, want stubRoundTripper", got) + } + }) +} + +func TestSafeClient_SetTLSCAData_ClonesTransport(t *testing.T) { + t.Parallel() + + sc := NewSafeClientForConfig(&rest.Config{}) + sc.SetTLSCAData(nil) + + orig := &http.Transport{} + + wrapped := sc.restConfig.WrapTransport(orig) + + clonedTransport, ok := wrapped.(*http.Transport) + if !ok { + t.Fatalf("wrapped transport is %T, want *http.Transport", wrapped) + } + + if clonedTransport == orig { + t.Error("wrapped transport is the same pointer as orig, want a clone") + } + + if clonedTransport.TLSClientConfig == nil { + t.Fatal("cloned TLSClientConfig = nil, want non-nil") + } + + if clonedTransport.TLSClientConfig.RootCAs == nil { + t.Error("cloned TLSClientConfig.RootCAs = nil, want non-nil") + } + + // http.Transport.Clone() itself lazily initializes the receiver's + // TLSClientConfig (nil -> non-nil) as an unrelated HTTP/2 auto-configuration + // side effect (net/http's http2configureTransports), independent of our fix; + // the invariant this test guards is that our own code never writes the + // merged CA pool onto orig, only onto the returned clone. + if clonedTransport.TLSClientConfig == orig.TLSClientConfig { + t.Error("cloned TLSClientConfig is the same pointer as orig.TLSClientConfig, want a distinct clone") + } + + if orig.TLSClientConfig != nil && orig.TLSClientConfig.RootCAs != nil { + t.Error("orig.TLSClientConfig.RootCAs was mutated, want nil (RootCAs must only be set on the clone)") + } +} From f8eceddfbcd10c6eb268e4d51344e42e2c030f8e Mon Sep 17 00:00:00 2001 From: Konstantin Kozoriz Date: Fri, 28 Aug 2026 14:35:45 +0300 Subject: [PATCH 09/12] test(libsaferequest): cover invalid CA data, client cert survival, double-call chaining SetTLSCAData's existing 4 tests already caught line-by-line regressions on the Insecure/ServerName force and the prev-chain, but left three edge cases unexercised: garbage/empty caData (AppendCertsFromPEM silently ignores bad input, must not panic or leave verification off), CertData/KeyData/CertFile/ KeyFile client-cert fields (untouched by the function, worth pinning), and a second SetTLSCAData call on the same client (prev-chaining must not turn self-referential and recurse forever). Signed-off-by: Konstantin Kozoriz --- pkg/libsaferequest/client/http_test.go | 142 +++++++++++++++++++++++++ 1 file changed, 142 insertions(+) diff --git a/pkg/libsaferequest/client/http_test.go b/pkg/libsaferequest/client/http_test.go index 290537f08..198eb61e4 100644 --- a/pkg/libsaferequest/client/http_test.go +++ b/pkg/libsaferequest/client/http_test.go @@ -248,6 +248,148 @@ func TestSafeClient_SetTLSCAData_ChainsExistingWrapTransport(t *testing.T) { }) } +func TestSafeClient_SetTLSCAData_InvalidCAData(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + caData []byte + }{ + {name: "success: garbage bytes do not panic and verification stays forced on", caData: []byte("not a certificate")}, + {name: "success: empty non-nil slice does not panic and verification stays forced on", caData: []byte{}}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + sc := &SafeClient{restConfig: &rest.Config{ + TLSClientConfig: rest.TLSClientConfig{ + Insecure: true, + ServerName: "api.example", + }, + }} + + sc.SetTLSCAData(tc.caData) + + if sc.restConfig.TLSClientConfig.Insecure { + t.Error("TLSClientConfig.Insecure = true, want false") + } + + if sc.restConfig.TLSClientConfig.ServerName != "" { + t.Errorf("TLSClientConfig.ServerName = %q, want empty", sc.restConfig.TLSClientConfig.ServerName) + } + + wrapped := sc.restConfig.WrapTransport(&http.Transport{}) + + clonedTransport, ok := wrapped.(*http.Transport) + if !ok { + t.Fatalf("wrapped transport is %T, want *http.Transport", wrapped) + } + + if clonedTransport.TLSClientConfig.InsecureSkipVerify { + t.Error("cloned TLSClientConfig.InsecureSkipVerify = true, want false") + } + + if clonedTransport.TLSClientConfig.RootCAs == nil { + t.Error("cloned TLSClientConfig.RootCAs = nil, want non-nil (system pool at minimum)") + } + }) + } +} + +func TestSafeClient_SetTLSCAData_PreservesClientCertConfig(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + }{ + {name: "success: CertData/KeyData/CertFile/KeyFile survive untouched"}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + sc := &SafeClient{restConfig: &rest.Config{ + TLSClientConfig: rest.TLSClientConfig{ + Insecure: true, + CertData: []byte("client-cert"), + KeyData: []byte("client-key"), + CertFile: "/etc/certs/client.crt", + KeyFile: "/etc/certs/client.key", + }, + }} + + sc.SetTLSCAData(nil) + + if string(sc.restConfig.TLSClientConfig.CertData) != "client-cert" { + t.Errorf("CertData = %q, want unchanged (%q)", sc.restConfig.TLSClientConfig.CertData, "client-cert") + } + + if string(sc.restConfig.TLSClientConfig.KeyData) != "client-key" { + t.Errorf("KeyData = %q, want unchanged (%q)", sc.restConfig.TLSClientConfig.KeyData, "client-key") + } + + if sc.restConfig.TLSClientConfig.CertFile != "/etc/certs/client.crt" { + t.Errorf("CertFile = %q, want unchanged (%q)", sc.restConfig.TLSClientConfig.CertFile, "/etc/certs/client.crt") + } + + if sc.restConfig.TLSClientConfig.KeyFile != "/etc/certs/client.key" { + t.Errorf("KeyFile = %q, want unchanged (%q)", sc.restConfig.TLSClientConfig.KeyFile, "/etc/certs/client.key") + } + }) + } +} + +func TestSafeClient_SetTLSCAData_CalledTwiceChainsWithoutInfiniteRecursion(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + }{ + {name: "success: second call wraps over the first and both apply their CA pool"}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + sc := NewSafeClientForConfig(&rest.Config{}) + + sc.SetTLSCAData(testCACertificatePEM(t)) + sc.SetTLSCAData(testCACertificatePEM(t)) + + // Guards against prev-chaining turning self-referential: if the second + // call's WrapTransport captured itself as prev instead of the first + // call's closure, invoking it here would recurse until stack overflow. + done := make(chan http.RoundTripper, 1) + + go func() { + done <- sc.restConfig.WrapTransport(&http.Transport{}) + }() + + select { + case wrapped := <-done: + clonedTransport, ok := wrapped.(*http.Transport) + if !ok { + t.Fatalf("wrapped transport is %T, want *http.Transport", wrapped) + } + + if clonedTransport.TLSClientConfig == nil || clonedTransport.TLSClientConfig.RootCAs == nil { + t.Error("cloned TLSClientConfig.RootCAs = nil, want non-nil") + } + + if clonedTransport.TLSClientConfig.InsecureSkipVerify { + t.Error("cloned TLSClientConfig.InsecureSkipVerify = true, want false") + } + case <-time.After(5 * time.Second): + t.Fatal("WrapTransport did not return within timeout; suspected infinite recursion in chained wrappers") + } + }) + } +} + func TestSafeClient_SetTLSCAData_ClonesTransport(t *testing.T) { t.Parallel() From 68ba3d1f1e1ba8a5718ca39bafdab1a5979f21f9 Mon Sep 17 00:00:00 2001 From: Konstantin Kozoriz Date: Fri, 28 Aug 2026 14:52:41 +0300 Subject: [PATCH 10/12] test(libsaferequest): remove tautological assertion, fix misleading subtest name Signed-off-by: Konstantin Kozoriz --- pkg/libsaferequest/client/http_test.go | 89 ++++++++++++++++++++++---- 1 file changed, 78 insertions(+), 11 deletions(-) diff --git a/pkg/libsaferequest/client/http_test.go b/pkg/libsaferequest/client/http_test.go index 198eb61e4..c0fa94e51 100644 --- a/pkg/libsaferequest/client/http_test.go +++ b/pkg/libsaferequest/client/http_test.go @@ -17,8 +17,13 @@ limitations under the License. package client import ( + "crypto/ed25519" + "crypto/rand" "crypto/tls" + "crypto/x509" + "crypto/x509/pkix" "encoding/pem" + "math/big" "net/http" "net/http/httptest" "testing" @@ -76,6 +81,36 @@ func testCACertificatePEM(t *testing.T) []byte { return pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: srv.Certificate().Raw}) } +// testSelfSignedCACertificatePEM returns a freshly generated, PEM-encoded +// self-signed certificate distinct from testCACertificatePEM's fixed +// httptest leaf, so a test can tell the two CA sources apart in an +// x509.CertPool instead of comparing byte-identical data. +func testSelfSignedCACertificatePEM(t *testing.T) []byte { + t.Helper() + + _, priv, err := ed25519.GenerateKey(rand.Reader) + if err != nil { + t.Fatalf("generate CA key: %v", err) + } + + template := &x509.Certificate{ + SerialNumber: big.NewInt(1), + Subject: pkix.Name{CommonName: "test-kubeconfig-ca"}, + NotBefore: time.Now().Add(-time.Hour), + NotAfter: time.Now().Add(time.Hour), + BasicConstraintsValid: true, + IsCA: true, + KeyUsage: x509.KeyUsageCertSign, + } + + der, err := x509.CreateCertificate(rand.Reader, template, template, priv.Public(), priv) + if err != nil { + t.Fatalf("create self-signed CA certificate: %v", err) + } + + return pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: der}) +} + // stubRoundTripper is a non-*http.Transport RoundTripper used to exercise the // pass-through branch of SetTLSCAData's WrapTransport wrapper. type stubRoundTripper struct{} @@ -173,14 +208,9 @@ func TestSafeClient_SetTLSCAData_PassThroughNonTransport(t *testing.T) { t.Fatal("wrapped RoundTripper = nil, want non-nil") } - stub, ok := got.(stubRoundTripper) - if !ok { + if _, ok := got.(stubRoundTripper); !ok { t.Fatalf("wrapped RoundTripper is %T, want stubRoundTripper", got) } - - if stub != (stubRoundTripper{}) { - t.Error("wrapped RoundTripper is not the same stubRoundTripper instance") - } } func TestSafeClient_SetTLSCAData_ChainsExistingWrapTransport(t *testing.T) { @@ -348,17 +378,26 @@ func TestSafeClient_SetTLSCAData_CalledTwiceChainsWithoutInfiniteRecursion(t *te tests := []struct { name string }{ - {name: "success: second call wraps over the first and both apply their CA pool"}, + {name: "second call does not recurse and still forces verification"}, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { t.Parallel() - sc := NewSafeClientForConfig(&rest.Config{}) + explicitCAData := testCACertificatePEM(t) + + // Simulates a CA already present on the rest.Config (e.g. from + // kubeconfig) before either SetTLSCAData call, distinct from + // explicitCAData so the two are distinguishable in the resulting pool. + kubeconfigCAData := testSelfSignedCACertificatePEM(t) - sc.SetTLSCAData(testCACertificatePEM(t)) - sc.SetTLSCAData(testCACertificatePEM(t)) + sc := NewSafeClientForConfig(&rest.Config{ + TLSClientConfig: rest.TLSClientConfig{CAData: kubeconfigCAData}, + }) + + sc.SetTLSCAData(explicitCAData) + sc.SetTLSCAData(explicitCAData) // Guards against prev-chaining turning self-referential: if the second // call's WrapTransport captured itself as prev instead of the first @@ -369,9 +408,13 @@ func TestSafeClient_SetTLSCAData_CalledTwiceChainsWithoutInfiniteRecursion(t *te done <- sc.restConfig.WrapTransport(&http.Transport{}) }() + var clonedTransport *http.Transport + select { case wrapped := <-done: - clonedTransport, ok := wrapped.(*http.Transport) + var ok bool + + clonedTransport, ok = wrapped.(*http.Transport) if !ok { t.Fatalf("wrapped transport is %T, want *http.Transport", wrapped) } @@ -386,6 +429,30 @@ func TestSafeClient_SetTLSCAData_CalledTwiceChainsWithoutInfiniteRecursion(t *te case <-time.After(5 * time.Second): t.Fatal("WrapTransport did not return within timeout; suspected infinite recursion in chained wrappers") } + + // The first call folds kubeconfigCAData into its pool and then clears + // TLSClientConfig.CAData (see SetTLSCAData), so by the time the second + // call builds its own sysPool, CAData is already nil and + // kubeconfigCAData is not folded in again. The second call's + // WrapTransport then clones over the first call's cloned transport and + // overwrites RootCAs wholesale rather than merging it with the first + // call's pool. The net effect: the final RootCAs traces only from the + // second SetTLSCAData(explicitCAData) call — identical to what a single, + // standalone call with the same explicitCAData would produce, and + // without kubeconfigCAData ever having survived into it. + soloSC := NewSafeClientForConfig(&rest.Config{}) + soloSC.SetTLSCAData(explicitCAData) + + soloWrapped := soloSC.restConfig.WrapTransport(&http.Transport{}) + + soloTransport, ok := soloWrapped.(*http.Transport) + if !ok { + t.Fatalf("solo wrapped transport is %T, want *http.Transport", soloWrapped) + } + + if !clonedTransport.TLSClientConfig.RootCAs.Equal(soloTransport.TLSClientConfig.RootCAs) { + t.Error("chained second-call RootCAs != solo second-call RootCAs; kubeconfig CA leaked into the final pool") + } }) } } From 4982e221732823a75cf13c7dfb92f2368707f623 Mon Sep 17 00:00:00 2001 From: Konstantin Kozoriz Date: Mon, 31 Aug 2026 15:59:32 +0300 Subject: [PATCH 11/12] chore(libsaferequest): remove verbose SetTLSCAData doc comment Signed-off-by: Konstantin Kozoriz --- pkg/libsaferequest/client/http.go | 24 ------------------------ 1 file changed, 24 deletions(-) diff --git a/pkg/libsaferequest/client/http.go b/pkg/libsaferequest/client/http.go index 94bf01979..5b071a146 100644 --- a/pkg/libsaferequest/client/http.go +++ b/pkg/libsaferequest/client/http.go @@ -177,30 +177,6 @@ func (c *SafeClient) NewRTClient(schemeFuncs ...func(s *apiruntime.Scheme) error return kubeRtClient, nil } -// SetTLSCAData extends inherited server trust with a merged pool: system roots, -// the supplied caData, and any CA already configured on this SafeClient's -// rest.Config (e.g. from kubeconfig). It is used for endpoints that cannot be -// pinned to a single CA — the d8 data export/import transports, both direct -// (exporter pod, internal CA from DataExport/DataImport status) and published -// (ingress, empty CA). -// -// Because a 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 (Insecure) or check the -// wrong hostname (ServerName) regardless of how large RootCAs is, so both are -// forced off here — on the rest.Config itself and, since client-go may already -// have built a base *http.Transport with those values baked in before this -// WrapTransport runs, again on the transport clone that carries RootCAs. This -// runs unconditionally, not only for a non-empty caData: an empty caData is the -// normal case on the publish path, and verification must stay on even then. -// -// Clearing ServerName is also why SetTLSCAData must not be called after -// SetProbeEndpoint on the same client — the probe sets ServerName to the -// kubernetes service name on purpose. Today they never meet: the probe runs on -// its own Copy() (internal/data/publish_detect.go). -// -// Keep in sync with the twin implementation in internal/snapshot/transport/http.go -// (Client.SetTLSCAData); the two are deliberately separate copies. func (c *SafeClient) SetTLSCAData(caData []byte) { sysPool, err := x509.SystemCertPool() if err != nil || sysPool == nil { From 8954d842eb029958b5c7facd481fcb6fe833ca9d Mon Sep 17 00:00:00 2001 From: Konstantin Kozoriz Date: Mon, 31 Aug 2026 22:21:25 +0300 Subject: [PATCH 12/12] fix(libsaferequest): make the branch squash-merge clean against #462/#463 The team merges with "Squash and merge", which drops the #462 -> #463 -> #464 ancestry. Git then falls back to origin/main as the merge base and sees #462's changes -- already present here -- as competing additions, producing two conflicts that a plain merge chain never hits: CONFLICT (content): internal/snapshot/transport/http.go CONFLICT (add/add): pkg/libsaferequest/client/http_test.go Both were purely structural, so reshape them instead of changing behaviour: - http_test.go is created by #462 too, so an add/add conflicts unless both sides match byte for byte. Restore it to #462's exact content and move the 7 SetTLSCAData tests and their 3 helpers to a new http_tls_test.go, which exists on neither side of main and merges as a clean single-side add. - The "keep in sync" note sat at the end of a doc comment block #462 had just added, so the two additions were adjacent and conflicted. Move it just inside SetTLSCAData, a region #462 does not touch. No production code changes: pkg/libsaferequest/client/http.go is untouched and internal/snapshot/transport/http.go now matches #463 apart from the relocated comment. All 8 tests are preserved and still pass under -race. Verified from origin/main: squash #462, squash #463, merge #464, merge #465 -- all four clean, then build, vet and go test -race clean over ./internal/snapshot/... and ./pkg/libsaferequest/... Signed-off-by: Konstantin Kozoriz --- internal/snapshot/transport/http.go | 6 +- pkg/libsaferequest/client/http_test.go | 438 ------------------- pkg/libsaferequest/client/http_tls_test.go | 462 +++++++++++++++++++++ 3 files changed, 465 insertions(+), 441 deletions(-) create mode 100644 pkg/libsaferequest/client/http_tls_test.go diff --git a/internal/snapshot/transport/http.go b/internal/snapshot/transport/http.go index f8d91e4cd..4114478b1 100644 --- a/internal/snapshot/transport/http.go +++ b/internal/snapshot/transport/http.go @@ -1195,10 +1195,10 @@ func (c *Client) NewRTClient(schemeFuncs ...func(s *apiruntime.Scheme) error) (c // 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. -// -// Keep in sync with the twin implementation in pkg/libsaferequest/client/http.go -// (SafeClient.SetTLSCAData); the two are deliberately separate copies. func (c *Client) SetTLSCAData(caData []byte) { + // Keep in sync with the twin implementation in + // pkg/libsaferequest/client/http.go (SafeClient.SetTLSCAData); the two are + // deliberately separate copies. sysPool, err := x509.SystemCertPool() if err != nil || sysPool == nil { sysPool = x509.NewCertPool() diff --git a/pkg/libsaferequest/client/http_test.go b/pkg/libsaferequest/client/http_test.go index c0fa94e51..f62b27bed 100644 --- a/pkg/libsaferequest/client/http_test.go +++ b/pkg/libsaferequest/client/http_test.go @@ -17,15 +17,6 @@ limitations under the License. package client import ( - "crypto/ed25519" - "crypto/rand" - "crypto/tls" - "crypto/x509" - "crypto/x509/pkix" - "encoding/pem" - "math/big" - "net/http" - "net/http/httptest" "testing" "time" @@ -68,432 +59,3 @@ func TestNewSafeClientForConfig(t *testing.T) { NewSafeClientForConfig(nil) }) } - -// testCACertificatePEM spins up a throwaway TLS server and returns its leaf -// certificate PEM-encoded, giving tests a syntactically valid CA bundle -// without depending on a real one. -func testCACertificatePEM(t *testing.T) []byte { - t.Helper() - - srv := httptest.NewTLSServer(http.HandlerFunc(func(_ http.ResponseWriter, _ *http.Request) {})) - t.Cleanup(srv.Close) - - return pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: srv.Certificate().Raw}) -} - -// testSelfSignedCACertificatePEM returns a freshly generated, PEM-encoded -// self-signed certificate distinct from testCACertificatePEM's fixed -// httptest leaf, so a test can tell the two CA sources apart in an -// x509.CertPool instead of comparing byte-identical data. -func testSelfSignedCACertificatePEM(t *testing.T) []byte { - t.Helper() - - _, priv, err := ed25519.GenerateKey(rand.Reader) - if err != nil { - t.Fatalf("generate CA key: %v", err) - } - - template := &x509.Certificate{ - SerialNumber: big.NewInt(1), - Subject: pkix.Name{CommonName: "test-kubeconfig-ca"}, - NotBefore: time.Now().Add(-time.Hour), - NotAfter: time.Now().Add(time.Hour), - BasicConstraintsValid: true, - IsCA: true, - KeyUsage: x509.KeyUsageCertSign, - } - - der, err := x509.CreateCertificate(rand.Reader, template, template, priv.Public(), priv) - if err != nil { - t.Fatalf("create self-signed CA certificate: %v", err) - } - - return pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: der}) -} - -// stubRoundTripper is a non-*http.Transport RoundTripper used to exercise the -// pass-through branch of SetTLSCAData's WrapTransport wrapper. -type stubRoundTripper struct{} - -// RoundTrip always fails; stubRoundTripper is never actually used to send a -// request in these tests. -func (stubRoundTripper) RoundTrip(_ *http.Request) (*http.Response, error) { - return nil, http.ErrNotSupported -} - -func TestSafeClient_SetTLSCAData_ForcesVerification(t *testing.T) { - t.Parallel() - - tests := []struct { - name string - caData []byte - presetCAData []byte - }{ - {name: "nil CA data", caData: nil}, - {name: "valid CA data", caData: testCACertificatePEM(t)}, - {name: "kubeconfig CA data", caData: nil, presetCAData: testCACertificatePEM(t)}, - } - - for _, tc := range tests { - t.Run(tc.name, func(t *testing.T) { - t.Parallel() - - sc := &SafeClient{restConfig: &rest.Config{ - TLSClientConfig: rest.TLSClientConfig{ - Insecure: true, - ServerName: "api.example", - CAData: tc.presetCAData, - }, - }} - - sc.SetTLSCAData(tc.caData) - - if sc.restConfig.TLSClientConfig.Insecure { - t.Error("TLSClientConfig.Insecure = true, want false") - } - - if sc.restConfig.TLSClientConfig.ServerName != "" { - t.Errorf("TLSClientConfig.ServerName = %q, want empty", sc.restConfig.TLSClientConfig.ServerName) - } - - if sc.restConfig.TLSClientConfig.CAData != nil { - t.Errorf("TLSClientConfig.CAData = %v, want nil", sc.restConfig.TLSClientConfig.CAData) - } - - if sc.restConfig.TLSClientConfig.CAFile != "" { - t.Errorf("TLSClientConfig.CAFile = %q, want empty", sc.restConfig.TLSClientConfig.CAFile) - } - - // exercising the inherited-insecure bypass this test guards against - orig := &http.Transport{TLSClientConfig: &tls.Config{InsecureSkipVerify: true, ServerName: "api.example"}} - - wrapped := sc.restConfig.WrapTransport(orig) - - clonedTransport, ok := wrapped.(*http.Transport) - if !ok { - t.Fatalf("wrapped transport is %T, want *http.Transport", wrapped) - } - - if clonedTransport.TLSClientConfig.InsecureSkipVerify { - t.Error("cloned TLSClientConfig.InsecureSkipVerify = true, want false") - } - - if clonedTransport.TLSClientConfig.ServerName != "" { - t.Errorf("cloned TLSClientConfig.ServerName = %q, want empty", clonedTransport.TLSClientConfig.ServerName) - } - - if clonedTransport.TLSClientConfig.RootCAs == nil { - t.Error("cloned TLSClientConfig.RootCAs = nil, want non-nil") - } - - if !orig.TLSClientConfig.InsecureSkipVerify { - t.Error("orig.TLSClientConfig.InsecureSkipVerify was mutated, want unchanged (true)") - } - - if orig.TLSClientConfig.ServerName != "api.example" { - t.Errorf("orig.TLSClientConfig.ServerName = %q, want unchanged (%q)", orig.TLSClientConfig.ServerName, "api.example") - } - }) - } -} - -func TestSafeClient_SetTLSCAData_PassThroughNonTransport(t *testing.T) { - t.Parallel() - - sc := NewSafeClientForConfig(&rest.Config{}) - sc.SetTLSCAData(nil) - - got := sc.restConfig.WrapTransport(stubRoundTripper{}) - if got == nil { - t.Fatal("wrapped RoundTripper = nil, want non-nil") - } - - if _, ok := got.(stubRoundTripper); !ok { - t.Fatalf("wrapped RoundTripper is %T, want stubRoundTripper", got) - } -} - -func TestSafeClient_SetTLSCAData_ChainsExistingWrapTransport(t *testing.T) { - t.Parallel() - - t.Run("success: prior wrapper and CA injection both survive", func(t *testing.T) { - t.Parallel() - - called := false - sc := NewSafeClientForConfig(&rest.Config{}) - sc.restConfig.WrapTransport = func(rt http.RoundTripper) http.RoundTripper { - called = true - - transport, ok := rt.(*http.Transport) - if !ok { - return rt - } - - clonedTransport := transport.Clone() - clonedTransport.ResponseHeaderTimeout = 42 * time.Millisecond - - return clonedTransport - } - - sc.SetTLSCAData(nil) - - wrapped := sc.restConfig.WrapTransport(&http.Transport{}) - - if !called { - t.Error("prior WrapTransport was not called") - } - - clonedTransport, ok := wrapped.(*http.Transport) - if !ok { - t.Fatalf("wrapped transport is %T, want *http.Transport", wrapped) - } - - if clonedTransport.ResponseHeaderTimeout != 42*time.Millisecond { - t.Errorf("ResponseHeaderTimeout = %v, want %v", clonedTransport.ResponseHeaderTimeout, 42*time.Millisecond) - } - - if clonedTransport.TLSClientConfig == nil || clonedTransport.TLSClientConfig.RootCAs == nil { - t.Error("TLSClientConfig.RootCAs = nil, want non-nil") - } - }) - - t.Run("success: prior wrapper returning a non-transport degrades to pass-through", func(t *testing.T) { - t.Parallel() - - sc := NewSafeClientForConfig(&rest.Config{}) - sc.restConfig.WrapTransport = func(_ http.RoundTripper) http.RoundTripper { - return stubRoundTripper{} - } - - sc.SetTLSCAData(nil) - - got := sc.restConfig.WrapTransport(&http.Transport{}) - if got == nil { - t.Fatal("wrapped RoundTripper = nil, want non-nil") - } - - if _, ok := got.(stubRoundTripper); !ok { - t.Fatalf("wrapped RoundTripper is %T, want stubRoundTripper", got) - } - }) -} - -func TestSafeClient_SetTLSCAData_InvalidCAData(t *testing.T) { - t.Parallel() - - tests := []struct { - name string - caData []byte - }{ - {name: "success: garbage bytes do not panic and verification stays forced on", caData: []byte("not a certificate")}, - {name: "success: empty non-nil slice does not panic and verification stays forced on", caData: []byte{}}, - } - - for _, tc := range tests { - t.Run(tc.name, func(t *testing.T) { - t.Parallel() - - sc := &SafeClient{restConfig: &rest.Config{ - TLSClientConfig: rest.TLSClientConfig{ - Insecure: true, - ServerName: "api.example", - }, - }} - - sc.SetTLSCAData(tc.caData) - - if sc.restConfig.TLSClientConfig.Insecure { - t.Error("TLSClientConfig.Insecure = true, want false") - } - - if sc.restConfig.TLSClientConfig.ServerName != "" { - t.Errorf("TLSClientConfig.ServerName = %q, want empty", sc.restConfig.TLSClientConfig.ServerName) - } - - wrapped := sc.restConfig.WrapTransport(&http.Transport{}) - - clonedTransport, ok := wrapped.(*http.Transport) - if !ok { - t.Fatalf("wrapped transport is %T, want *http.Transport", wrapped) - } - - if clonedTransport.TLSClientConfig.InsecureSkipVerify { - t.Error("cloned TLSClientConfig.InsecureSkipVerify = true, want false") - } - - if clonedTransport.TLSClientConfig.RootCAs == nil { - t.Error("cloned TLSClientConfig.RootCAs = nil, want non-nil (system pool at minimum)") - } - }) - } -} - -func TestSafeClient_SetTLSCAData_PreservesClientCertConfig(t *testing.T) { - t.Parallel() - - tests := []struct { - name string - }{ - {name: "success: CertData/KeyData/CertFile/KeyFile survive untouched"}, - } - - for _, tc := range tests { - t.Run(tc.name, func(t *testing.T) { - t.Parallel() - - sc := &SafeClient{restConfig: &rest.Config{ - TLSClientConfig: rest.TLSClientConfig{ - Insecure: true, - CertData: []byte("client-cert"), - KeyData: []byte("client-key"), - CertFile: "/etc/certs/client.crt", - KeyFile: "/etc/certs/client.key", - }, - }} - - sc.SetTLSCAData(nil) - - if string(sc.restConfig.TLSClientConfig.CertData) != "client-cert" { - t.Errorf("CertData = %q, want unchanged (%q)", sc.restConfig.TLSClientConfig.CertData, "client-cert") - } - - if string(sc.restConfig.TLSClientConfig.KeyData) != "client-key" { - t.Errorf("KeyData = %q, want unchanged (%q)", sc.restConfig.TLSClientConfig.KeyData, "client-key") - } - - if sc.restConfig.TLSClientConfig.CertFile != "/etc/certs/client.crt" { - t.Errorf("CertFile = %q, want unchanged (%q)", sc.restConfig.TLSClientConfig.CertFile, "/etc/certs/client.crt") - } - - if sc.restConfig.TLSClientConfig.KeyFile != "/etc/certs/client.key" { - t.Errorf("KeyFile = %q, want unchanged (%q)", sc.restConfig.TLSClientConfig.KeyFile, "/etc/certs/client.key") - } - }) - } -} - -func TestSafeClient_SetTLSCAData_CalledTwiceChainsWithoutInfiniteRecursion(t *testing.T) { - t.Parallel() - - tests := []struct { - name string - }{ - {name: "second call does not recurse and still forces verification"}, - } - - for _, tc := range tests { - t.Run(tc.name, func(t *testing.T) { - t.Parallel() - - explicitCAData := testCACertificatePEM(t) - - // Simulates a CA already present on the rest.Config (e.g. from - // kubeconfig) before either SetTLSCAData call, distinct from - // explicitCAData so the two are distinguishable in the resulting pool. - kubeconfigCAData := testSelfSignedCACertificatePEM(t) - - sc := NewSafeClientForConfig(&rest.Config{ - TLSClientConfig: rest.TLSClientConfig{CAData: kubeconfigCAData}, - }) - - sc.SetTLSCAData(explicitCAData) - sc.SetTLSCAData(explicitCAData) - - // Guards against prev-chaining turning self-referential: if the second - // call's WrapTransport captured itself as prev instead of the first - // call's closure, invoking it here would recurse until stack overflow. - done := make(chan http.RoundTripper, 1) - - go func() { - done <- sc.restConfig.WrapTransport(&http.Transport{}) - }() - - var clonedTransport *http.Transport - - select { - case wrapped := <-done: - var ok bool - - clonedTransport, ok = wrapped.(*http.Transport) - if !ok { - t.Fatalf("wrapped transport is %T, want *http.Transport", wrapped) - } - - if clonedTransport.TLSClientConfig == nil || clonedTransport.TLSClientConfig.RootCAs == nil { - t.Error("cloned TLSClientConfig.RootCAs = nil, want non-nil") - } - - if clonedTransport.TLSClientConfig.InsecureSkipVerify { - t.Error("cloned TLSClientConfig.InsecureSkipVerify = true, want false") - } - case <-time.After(5 * time.Second): - t.Fatal("WrapTransport did not return within timeout; suspected infinite recursion in chained wrappers") - } - - // The first call folds kubeconfigCAData into its pool and then clears - // TLSClientConfig.CAData (see SetTLSCAData), so by the time the second - // call builds its own sysPool, CAData is already nil and - // kubeconfigCAData is not folded in again. The second call's - // WrapTransport then clones over the first call's cloned transport and - // overwrites RootCAs wholesale rather than merging it with the first - // call's pool. The net effect: the final RootCAs traces only from the - // second SetTLSCAData(explicitCAData) call — identical to what a single, - // standalone call with the same explicitCAData would produce, and - // without kubeconfigCAData ever having survived into it. - soloSC := NewSafeClientForConfig(&rest.Config{}) - soloSC.SetTLSCAData(explicitCAData) - - soloWrapped := soloSC.restConfig.WrapTransport(&http.Transport{}) - - soloTransport, ok := soloWrapped.(*http.Transport) - if !ok { - t.Fatalf("solo wrapped transport is %T, want *http.Transport", soloWrapped) - } - - if !clonedTransport.TLSClientConfig.RootCAs.Equal(soloTransport.TLSClientConfig.RootCAs) { - t.Error("chained second-call RootCAs != solo second-call RootCAs; kubeconfig CA leaked into the final pool") - } - }) - } -} - -func TestSafeClient_SetTLSCAData_ClonesTransport(t *testing.T) { - t.Parallel() - - sc := NewSafeClientForConfig(&rest.Config{}) - sc.SetTLSCAData(nil) - - orig := &http.Transport{} - - wrapped := sc.restConfig.WrapTransport(orig) - - clonedTransport, ok := wrapped.(*http.Transport) - if !ok { - t.Fatalf("wrapped transport is %T, want *http.Transport", wrapped) - } - - if clonedTransport == orig { - t.Error("wrapped transport is the same pointer as orig, want a clone") - } - - if clonedTransport.TLSClientConfig == nil { - t.Fatal("cloned TLSClientConfig = nil, want non-nil") - } - - if clonedTransport.TLSClientConfig.RootCAs == nil { - t.Error("cloned TLSClientConfig.RootCAs = nil, want non-nil") - } - - // http.Transport.Clone() itself lazily initializes the receiver's - // TLSClientConfig (nil -> non-nil) as an unrelated HTTP/2 auto-configuration - // side effect (net/http's http2configureTransports), independent of our fix; - // the invariant this test guards is that our own code never writes the - // merged CA pool onto orig, only onto the returned clone. - if clonedTransport.TLSClientConfig == orig.TLSClientConfig { - t.Error("cloned TLSClientConfig is the same pointer as orig.TLSClientConfig, want a distinct clone") - } - - if orig.TLSClientConfig != nil && orig.TLSClientConfig.RootCAs != nil { - t.Error("orig.TLSClientConfig.RootCAs was mutated, want nil (RootCAs must only be set on the clone)") - } -} diff --git a/pkg/libsaferequest/client/http_tls_test.go b/pkg/libsaferequest/client/http_tls_test.go new file mode 100644 index 000000000..b4357b98c --- /dev/null +++ b/pkg/libsaferequest/client/http_tls_test.go @@ -0,0 +1,462 @@ +/* +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 ( + "crypto/ed25519" + "crypto/rand" + "crypto/tls" + "crypto/x509" + "crypto/x509/pkix" + "encoding/pem" + "math/big" + "net/http" + "net/http/httptest" + "testing" + "time" + + "k8s.io/client-go/rest" +) + +// testCACertificatePEM spins up a throwaway TLS server and returns its leaf +// certificate PEM-encoded, giving tests a syntactically valid CA bundle +// without depending on a real one. +func testCACertificatePEM(t *testing.T) []byte { + t.Helper() + + srv := httptest.NewTLSServer(http.HandlerFunc(func(_ http.ResponseWriter, _ *http.Request) {})) + t.Cleanup(srv.Close) + + return pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: srv.Certificate().Raw}) +} + +// testSelfSignedCACertificatePEM returns a freshly generated, PEM-encoded +// self-signed certificate distinct from testCACertificatePEM's fixed +// httptest leaf, so a test can tell the two CA sources apart in an +// x509.CertPool instead of comparing byte-identical data. +func testSelfSignedCACertificatePEM(t *testing.T) []byte { + t.Helper() + + _, priv, err := ed25519.GenerateKey(rand.Reader) + if err != nil { + t.Fatalf("generate CA key: %v", err) + } + + template := &x509.Certificate{ + SerialNumber: big.NewInt(1), + Subject: pkix.Name{CommonName: "test-kubeconfig-ca"}, + NotBefore: time.Now().Add(-time.Hour), + NotAfter: time.Now().Add(time.Hour), + BasicConstraintsValid: true, + IsCA: true, + KeyUsage: x509.KeyUsageCertSign, + } + + der, err := x509.CreateCertificate(rand.Reader, template, template, priv.Public(), priv) + if err != nil { + t.Fatalf("create self-signed CA certificate: %v", err) + } + + return pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: der}) +} + +// stubRoundTripper is a non-*http.Transport RoundTripper used to exercise the +// pass-through branch of SetTLSCAData's WrapTransport wrapper. +type stubRoundTripper struct{} + +// RoundTrip always fails; stubRoundTripper is never actually used to send a +// request in these tests. +func (stubRoundTripper) RoundTrip(_ *http.Request) (*http.Response, error) { + return nil, http.ErrNotSupported +} + +func TestSafeClient_SetTLSCAData_ForcesVerification(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + caData []byte + presetCAData []byte + }{ + {name: "nil CA data", caData: nil}, + {name: "valid CA data", caData: testCACertificatePEM(t)}, + {name: "kubeconfig CA data", caData: nil, presetCAData: testCACertificatePEM(t)}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + sc := &SafeClient{restConfig: &rest.Config{ + TLSClientConfig: rest.TLSClientConfig{ + Insecure: true, + ServerName: "api.example", + CAData: tc.presetCAData, + }, + }} + + sc.SetTLSCAData(tc.caData) + + if sc.restConfig.TLSClientConfig.Insecure { + t.Error("TLSClientConfig.Insecure = true, want false") + } + + if sc.restConfig.TLSClientConfig.ServerName != "" { + t.Errorf("TLSClientConfig.ServerName = %q, want empty", sc.restConfig.TLSClientConfig.ServerName) + } + + if sc.restConfig.TLSClientConfig.CAData != nil { + t.Errorf("TLSClientConfig.CAData = %v, want nil", sc.restConfig.TLSClientConfig.CAData) + } + + if sc.restConfig.TLSClientConfig.CAFile != "" { + t.Errorf("TLSClientConfig.CAFile = %q, want empty", sc.restConfig.TLSClientConfig.CAFile) + } + + // exercising the inherited-insecure bypass this test guards against + orig := &http.Transport{TLSClientConfig: &tls.Config{InsecureSkipVerify: true, ServerName: "api.example"}} + + wrapped := sc.restConfig.WrapTransport(orig) + + clonedTransport, ok := wrapped.(*http.Transport) + if !ok { + t.Fatalf("wrapped transport is %T, want *http.Transport", wrapped) + } + + if clonedTransport.TLSClientConfig.InsecureSkipVerify { + t.Error("cloned TLSClientConfig.InsecureSkipVerify = true, want false") + } + + if clonedTransport.TLSClientConfig.ServerName != "" { + t.Errorf("cloned TLSClientConfig.ServerName = %q, want empty", clonedTransport.TLSClientConfig.ServerName) + } + + if clonedTransport.TLSClientConfig.RootCAs == nil { + t.Error("cloned TLSClientConfig.RootCAs = nil, want non-nil") + } + + if !orig.TLSClientConfig.InsecureSkipVerify { + t.Error("orig.TLSClientConfig.InsecureSkipVerify was mutated, want unchanged (true)") + } + + if orig.TLSClientConfig.ServerName != "api.example" { + t.Errorf("orig.TLSClientConfig.ServerName = %q, want unchanged (%q)", orig.TLSClientConfig.ServerName, "api.example") + } + }) + } +} + +func TestSafeClient_SetTLSCAData_PassThroughNonTransport(t *testing.T) { + t.Parallel() + + sc := NewSafeClientForConfig(&rest.Config{}) + sc.SetTLSCAData(nil) + + got := sc.restConfig.WrapTransport(stubRoundTripper{}) + if got == nil { + t.Fatal("wrapped RoundTripper = nil, want non-nil") + } + + if _, ok := got.(stubRoundTripper); !ok { + t.Fatalf("wrapped RoundTripper is %T, want stubRoundTripper", got) + } +} + +func TestSafeClient_SetTLSCAData_ChainsExistingWrapTransport(t *testing.T) { + t.Parallel() + + t.Run("success: prior wrapper and CA injection both survive", func(t *testing.T) { + t.Parallel() + + called := false + sc := NewSafeClientForConfig(&rest.Config{}) + sc.restConfig.WrapTransport = func(rt http.RoundTripper) http.RoundTripper { + called = true + + transport, ok := rt.(*http.Transport) + if !ok { + return rt + } + + clonedTransport := transport.Clone() + clonedTransport.ResponseHeaderTimeout = 42 * time.Millisecond + + return clonedTransport + } + + sc.SetTLSCAData(nil) + + wrapped := sc.restConfig.WrapTransport(&http.Transport{}) + + if !called { + t.Error("prior WrapTransport was not called") + } + + clonedTransport, ok := wrapped.(*http.Transport) + if !ok { + t.Fatalf("wrapped transport is %T, want *http.Transport", wrapped) + } + + if clonedTransport.ResponseHeaderTimeout != 42*time.Millisecond { + t.Errorf("ResponseHeaderTimeout = %v, want %v", clonedTransport.ResponseHeaderTimeout, 42*time.Millisecond) + } + + if clonedTransport.TLSClientConfig == nil || clonedTransport.TLSClientConfig.RootCAs == nil { + t.Error("TLSClientConfig.RootCAs = nil, want non-nil") + } + }) + + t.Run("success: prior wrapper returning a non-transport degrades to pass-through", func(t *testing.T) { + t.Parallel() + + sc := NewSafeClientForConfig(&rest.Config{}) + sc.restConfig.WrapTransport = func(_ http.RoundTripper) http.RoundTripper { + return stubRoundTripper{} + } + + sc.SetTLSCAData(nil) + + got := sc.restConfig.WrapTransport(&http.Transport{}) + if got == nil { + t.Fatal("wrapped RoundTripper = nil, want non-nil") + } + + if _, ok := got.(stubRoundTripper); !ok { + t.Fatalf("wrapped RoundTripper is %T, want stubRoundTripper", got) + } + }) +} + +func TestSafeClient_SetTLSCAData_InvalidCAData(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + caData []byte + }{ + {name: "success: garbage bytes do not panic and verification stays forced on", caData: []byte("not a certificate")}, + {name: "success: empty non-nil slice does not panic and verification stays forced on", caData: []byte{}}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + sc := &SafeClient{restConfig: &rest.Config{ + TLSClientConfig: rest.TLSClientConfig{ + Insecure: true, + ServerName: "api.example", + }, + }} + + sc.SetTLSCAData(tc.caData) + + if sc.restConfig.TLSClientConfig.Insecure { + t.Error("TLSClientConfig.Insecure = true, want false") + } + + if sc.restConfig.TLSClientConfig.ServerName != "" { + t.Errorf("TLSClientConfig.ServerName = %q, want empty", sc.restConfig.TLSClientConfig.ServerName) + } + + wrapped := sc.restConfig.WrapTransport(&http.Transport{}) + + clonedTransport, ok := wrapped.(*http.Transport) + if !ok { + t.Fatalf("wrapped transport is %T, want *http.Transport", wrapped) + } + + if clonedTransport.TLSClientConfig.InsecureSkipVerify { + t.Error("cloned TLSClientConfig.InsecureSkipVerify = true, want false") + } + + if clonedTransport.TLSClientConfig.RootCAs == nil { + t.Error("cloned TLSClientConfig.RootCAs = nil, want non-nil (system pool at minimum)") + } + }) + } +} + +func TestSafeClient_SetTLSCAData_PreservesClientCertConfig(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + }{ + {name: "success: CertData/KeyData/CertFile/KeyFile survive untouched"}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + sc := &SafeClient{restConfig: &rest.Config{ + TLSClientConfig: rest.TLSClientConfig{ + Insecure: true, + CertData: []byte("client-cert"), + KeyData: []byte("client-key"), + CertFile: "/etc/certs/client.crt", + KeyFile: "/etc/certs/client.key", + }, + }} + + sc.SetTLSCAData(nil) + + if string(sc.restConfig.TLSClientConfig.CertData) != "client-cert" { + t.Errorf("CertData = %q, want unchanged (%q)", sc.restConfig.TLSClientConfig.CertData, "client-cert") + } + + if string(sc.restConfig.TLSClientConfig.KeyData) != "client-key" { + t.Errorf("KeyData = %q, want unchanged (%q)", sc.restConfig.TLSClientConfig.KeyData, "client-key") + } + + if sc.restConfig.TLSClientConfig.CertFile != "/etc/certs/client.crt" { + t.Errorf("CertFile = %q, want unchanged (%q)", sc.restConfig.TLSClientConfig.CertFile, "/etc/certs/client.crt") + } + + if sc.restConfig.TLSClientConfig.KeyFile != "/etc/certs/client.key" { + t.Errorf("KeyFile = %q, want unchanged (%q)", sc.restConfig.TLSClientConfig.KeyFile, "/etc/certs/client.key") + } + }) + } +} + +func TestSafeClient_SetTLSCAData_CalledTwiceChainsWithoutInfiniteRecursion(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + }{ + {name: "second call does not recurse and still forces verification"}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + explicitCAData := testCACertificatePEM(t) + + // Simulates a CA already present on the rest.Config (e.g. from + // kubeconfig) before either SetTLSCAData call, distinct from + // explicitCAData so the two are distinguishable in the resulting pool. + kubeconfigCAData := testSelfSignedCACertificatePEM(t) + + sc := NewSafeClientForConfig(&rest.Config{ + TLSClientConfig: rest.TLSClientConfig{CAData: kubeconfigCAData}, + }) + + sc.SetTLSCAData(explicitCAData) + sc.SetTLSCAData(explicitCAData) + + // Guards against prev-chaining turning self-referential: if the second + // call's WrapTransport captured itself as prev instead of the first + // call's closure, invoking it here would recurse until stack overflow. + done := make(chan http.RoundTripper, 1) + + go func() { + done <- sc.restConfig.WrapTransport(&http.Transport{}) + }() + + var clonedTransport *http.Transport + + select { + case wrapped := <-done: + var ok bool + + clonedTransport, ok = wrapped.(*http.Transport) + if !ok { + t.Fatalf("wrapped transport is %T, want *http.Transport", wrapped) + } + + if clonedTransport.TLSClientConfig == nil || clonedTransport.TLSClientConfig.RootCAs == nil { + t.Error("cloned TLSClientConfig.RootCAs = nil, want non-nil") + } + + if clonedTransport.TLSClientConfig.InsecureSkipVerify { + t.Error("cloned TLSClientConfig.InsecureSkipVerify = true, want false") + } + case <-time.After(5 * time.Second): + t.Fatal("WrapTransport did not return within timeout; suspected infinite recursion in chained wrappers") + } + + // The first call folds kubeconfigCAData into its pool and then clears + // TLSClientConfig.CAData (see SetTLSCAData), so by the time the second + // call builds its own sysPool, CAData is already nil and + // kubeconfigCAData is not folded in again. The second call's + // WrapTransport then clones over the first call's cloned transport and + // overwrites RootCAs wholesale rather than merging it with the first + // call's pool. The net effect: the final RootCAs traces only from the + // second SetTLSCAData(explicitCAData) call — identical to what a single, + // standalone call with the same explicitCAData would produce, and + // without kubeconfigCAData ever having survived into it. + soloSC := NewSafeClientForConfig(&rest.Config{}) + soloSC.SetTLSCAData(explicitCAData) + + soloWrapped := soloSC.restConfig.WrapTransport(&http.Transport{}) + + soloTransport, ok := soloWrapped.(*http.Transport) + if !ok { + t.Fatalf("solo wrapped transport is %T, want *http.Transport", soloWrapped) + } + + if !clonedTransport.TLSClientConfig.RootCAs.Equal(soloTransport.TLSClientConfig.RootCAs) { + t.Error("chained second-call RootCAs != solo second-call RootCAs; kubeconfig CA leaked into the final pool") + } + }) + } +} + +func TestSafeClient_SetTLSCAData_ClonesTransport(t *testing.T) { + t.Parallel() + + sc := NewSafeClientForConfig(&rest.Config{}) + sc.SetTLSCAData(nil) + + orig := &http.Transport{} + + wrapped := sc.restConfig.WrapTransport(orig) + + clonedTransport, ok := wrapped.(*http.Transport) + if !ok { + t.Fatalf("wrapped transport is %T, want *http.Transport", wrapped) + } + + if clonedTransport == orig { + t.Error("wrapped transport is the same pointer as orig, want a clone") + } + + if clonedTransport.TLSClientConfig == nil { + t.Fatal("cloned TLSClientConfig = nil, want non-nil") + } + + if clonedTransport.TLSClientConfig.RootCAs == nil { + t.Error("cloned TLSClientConfig.RootCAs = nil, want non-nil") + } + + // http.Transport.Clone() itself lazily initializes the receiver's + // TLSClientConfig (nil -> non-nil) as an unrelated HTTP/2 auto-configuration + // side effect (net/http's http2configureTransports), independent of our fix; + // the invariant this test guards is that our own code never writes the + // merged CA pool onto orig, only onto the returned clone. + if clonedTransport.TLSClientConfig == orig.TLSClientConfig { + t.Error("cloned TLSClientConfig is the same pointer as orig.TLSClientConfig, want a distinct clone") + } + + if orig.TLSClientConfig != nil && orig.TLSClientConfig.RootCAs != nil { + t.Error("orig.TLSClientConfig.RootCAs was mutated, want nil (RootCAs must only be set on the clone)") + } +}