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()) 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) + } + }) + } +} 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) + } + }) + } +} 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") + }) + } +} 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..6416be889 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,25 @@ 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. 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) + } + } + + // 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 +867,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 +898,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 +928,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 +936,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 +2254,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 +2310,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 +2386,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..a4d4694c7 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,248 @@ 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 + insecure 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, + }, + { + // 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: targetURL, + BearerToken: "must-not-leak", + TLSClientConfig: restclient.TLSClientConfig{ + Insecure: tc.insecure, + CAData: configCAData, + }, + } + + importer := &clusterVolumeImporter{ + sc: transport.NewClientForConfig(config), + publish: tc.publish, + } + + 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, targetURL, 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 an untrusted origin") + } + + if tc.insecure && receivedAuthHeader != "" { + t.Fatalf("Authorization header leaked to an untrusted server: %q", receivedAuthHeader) + } + + 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 +5620,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 +5642,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 +5668,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 +5723,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 +5779,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 +5796,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") diff --git a/internal/snapshot/transport/http.go b/internal/snapshot/transport/http.go index cd5ff0710..4114478b1 100644 --- a/internal/snapshot/transport/http.go +++ b/internal/snapshot/transport/http.go @@ -1179,7 +1179,26 @@ 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) { + // 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() @@ -1195,6 +1214,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,11 +1237,21 @@ func (c *Client) SetTLSCAData(caData []byte) { } clonedTransport.TLSClientConfig.RootCAs = sysPool + clonedTransport.TLSClientConfig.InsecureSkipVerify = false + clonedTransport.TLSClientConfig.ServerName = "" return clonedTransport } } +// 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..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. @@ -1212,6 +1299,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, @@ -1248,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() diff --git a/pkg/libsaferequest/client/http.go b/pkg/libsaferequest/client/http.go index ef967a83c..5b071a146 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 @@ -186,20 +193,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 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) + }) +} 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)") + } +}