diff --git a/internal/data/common.go b/internal/data/common.go index 89e6e2aff..37f442c12 100644 --- a/internal/data/common.go +++ b/internal/data/common.go @@ -10,6 +10,8 @@ import ( "time" "golang.org/x/term" + "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) const ( @@ -112,6 +114,57 @@ func KindToGroup(kind string) (string, error) { } } +// Condition types and reasons shared by DataExport and DataImport. +const ( + // ConditionTypeReady is the readiness condition both producers set. + ConditionTypeReady = "Ready" + + // ConditionTypeExpired is the standalone expiry condition storage-volume-data-manager sets. + // storage-foundation dropped it in favour of ReasonExpired on Ready. + ConditionTypeExpired = "Expired" + + // ReasonExpired is the Ready-condition reason both producers use for idle expiry. + ReasonExpired = "Expired" +) + +// IsExpired reports whether the conditions say the DataExport or DataImport has terminally +// idle-expired, so the caller must recreate it rather than keep polling. After expiry the +// producer's garbage collector only removes the object once its retention TTL runs out, so +// waiting it out would stall for as long as that retention lasts. +// +// Both spellings of expiry are accepted, because the producers do not agree on one and a client +// that reads only its own producer's spelling silently waits forever against the other: +// +// - storage-volume-data-manager raises a standalone Expired condition (and also reports it as a +// Ready reason); +// - storage-foundation has no Expired condition type at all and reports it only as +// Ready=False with reason Expired. +// +// The two cannot be confused for one another: neither producer uses either spelling to mean +// anything but expiry. +func IsExpired(conditions []metav1.Condition) bool { + if expired := meta.FindStatusCondition(conditions, ConditionTypeExpired); expired != nil && + expired.Status == metav1.ConditionTrue { + return true + } + + ready := meta.FindStatusCondition(conditions, ConditionTypeReady) + + return ready != nil && ready.Status == metav1.ConditionFalse && ready.Reason == ReasonExpired +} + +// NotReady returns the Ready condition when it is present and not True, and nil otherwise — +// including when the object carries no Ready condition at all, which callers treat as "nothing +// said yet" rather than as a failure. +func NotReady(conditions []metav1.Condition) *metav1.Condition { + ready := meta.FindStatusCondition(conditions, ConditionTypeReady) + if ready == nil || ready.Status == metav1.ConditionTrue { + return nil + } + + return ready +} + func ParseArgs(args []string) ( /*deName*/ string /*srcPath*/, string, error) { var deName, srcPath string diff --git a/internal/data/conditions_test.go b/internal/data/conditions_test.go new file mode 100644 index 000000000..6db4d7b50 --- /dev/null +++ b/internal/data/conditions_test.go @@ -0,0 +1,134 @@ +/* +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 dataio + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +func cond(condType string, status metav1.ConditionStatus, reason string) metav1.Condition { + return metav1.Condition{ + Type: condType, + Status: status, + Reason: reason, + Message: reason, + } +} + +// TestIsExpired covers both producers' spellings of expiry plus the states that must not be read as +// expiry. Reading only one spelling is not a cosmetic bug: the caller keeps polling an object the +// producer will not revive, for as long as that producer's retention TTL lasts. +func TestIsExpired(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + conditions []metav1.Condition + want bool + }{ + { + name: "storage-foundation spelling: Ready=False with reason Expired", + conditions: []metav1.Condition{cond(ConditionTypeReady, metav1.ConditionFalse, ReasonExpired)}, + want: true, + }, + { + // The importer pod raises this before the controller mirrors it onto Ready, so a real + // object passes through exactly this pairing. + name: "older producer spelling: standalone Expired=True while Ready is still True", + conditions: []metav1.Condition{ + cond(ConditionTypeExpired, metav1.ConditionTrue, ReasonExpired), + cond(ConditionTypeReady, metav1.ConditionTrue, "PodReady"), + }, + want: true, + }, + { + name: "older producer, after the controller mirrored it", + conditions: []metav1.Condition{ + cond(ConditionTypeExpired, metav1.ConditionTrue, ReasonExpired), + cond(ConditionTypeReady, metav1.ConditionFalse, ReasonExpired), + }, + want: true, + }, + { + name: "an Expired condition that is False says the object has not expired", + conditions: []metav1.Condition{cond(ConditionTypeExpired, metav1.ConditionFalse, "Pending")}, + want: false, + }, + { + // Guards against too broad a predicate: a not-Ready object is not an expired one, and + // recreating it would destroy an import that was merely still working. + name: "Ready=False for another reason is not expiry", + conditions: []metav1.Condition{cond(ConditionTypeReady, metav1.ConditionFalse, "Completed")}, + want: false, + }, + { + name: "healthy object", + conditions: []metav1.Condition{cond(ConditionTypeReady, metav1.ConditionTrue, "PodReady")}, + want: false, + }, + { + name: "no conditions at all", + conditions: nil, + want: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + assert.Equal(t, tt.want, IsExpired(tt.conditions)) + }) + } +} + +// TestNotReady pins the distinction between "reported as not ready" and "has not reported yet". An +// object carrying no Ready condition has not been reconciled, and treating that as a failure turns +// the first poll of a freshly created object into an error. +func TestNotReady(t *testing.T) { + t.Parallel() + + t.Run("absent Ready condition is not a failure", func(t *testing.T) { + t.Parallel() + assert.Nil(t, NotReady(nil)) + assert.Nil(t, NotReady([]metav1.Condition{cond(ConditionTypeExpired, metav1.ConditionFalse, "Pending")})) + }) + + t.Run("Ready=True is not a failure", func(t *testing.T) { + t.Parallel() + assert.Nil(t, NotReady([]metav1.Condition{cond(ConditionTypeReady, metav1.ConditionTrue, "PodReady")})) + }) + + t.Run("Ready=False is returned with its reason", func(t *testing.T) { + t.Parallel() + + got := NotReady([]metav1.Condition{cond(ConditionTypeReady, metav1.ConditionFalse, "TargetNotFound")}) + require.NotNil(t, got) + assert.Equal(t, "TargetNotFound", got.Reason) + }) + + t.Run("Ready=Unknown is returned too", func(t *testing.T) { + t.Parallel() + + got := NotReady([]metav1.Condition{cond(ConditionTypeReady, metav1.ConditionUnknown, "Pending")}) + require.NotNil(t, got) + assert.Equal(t, "Pending", got.Reason) + }) +} diff --git a/internal/data/dataapi/groups.go b/internal/data/dataapi/groups.go new file mode 100644 index 000000000..ca67d7408 --- /dev/null +++ b/internal/data/dataapi/groups.go @@ -0,0 +1,87 @@ +/* +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 dataapi resolves which of the two API groups a cluster serves DataExport and +// DataImport under, and which of them the calling user is actually authorized to use. +// +// Two different modules produce the same pair of CRDs: +// +// - storage-foundation serves them under FoundationGroup. It supersedes the older module +// and is what a cluster with storage-foundation enabled exposes. +// - storage-volume-data-manager serves them under LegacyGroup. Editions that ship that +// module alone expose this group and nothing else. +// +// A single d8 binary has to work against both, so the group is a runtime decision rather than a +// compile-time constant. The decision cannot be made from the module list: `d8 data` is run by +// ordinary users, who are not authorized to read ModuleConfig, and whose RBAC may cover only one +// of the two groups even when the cluster serves both. +package dataapi + +import "k8s.io/apimachinery/pkg/runtime/schema" + +const ( + // FoundationGroup is the API group under which storage-foundation serves DataExport and + // DataImport. Preferred whenever the cluster serves it and the user is authorized for it. + FoundationGroup = "storage-foundation.deckhouse.io" + + // LegacyGroup is the API group under which storage-volume-data-manager serves DataExport + // and DataImport. Used when the cluster does not serve FoundationGroup, or serves it but + // denies the user access to it. + LegacyGroup = "storage.deckhouse.io" + + // Version is the version both groups serve these CRDs under. + Version = "v1alpha1" +) + +// Resource plurals this package can resolve a group for. Resolution is per resource rather +// than per group because a cluster is free to serve one CRD of the pair and not the other. +const ( + ResourceDataExports = "dataexports" + ResourceDataImports = "dataimports" +) + +// Module names, used only in operator-facing messages that name what to enable. +const ( + foundationModule = "storage-foundation" + legacyModule = "storage-volume-data-manager" +) + +var ( + // FoundationGroupVersion is the storage-foundation GroupVersion of DataExport/DataImport. + FoundationGroupVersion = schema.GroupVersion{Group: FoundationGroup, Version: Version} + + // LegacyGroupVersion is the storage-volume-data-manager GroupVersion of the same pair. + LegacyGroupVersion = schema.GroupVersion{Group: LegacyGroup, Version: Version} +) + +// Backend is a resolved answer: the GroupVersion to address the CRD through, plus the module +// that serves it for messages. +type Backend struct { + GroupVersion schema.GroupVersion + Module string +} + +// Legacy reports whether the resolved backend is storage-volume-data-manager's group. Callers +// that build a request body differing between the two producers branch on this; callers that +// only address the object by GroupVersion do not need it. +func (b Backend) Legacy() bool { + return b.GroupVersion.Group == LegacyGroup +} + +// String renders the backend as its GroupVersion, e.g. "storage-foundation.deckhouse.io/v1alpha1". +func (b Backend) String() string { + return b.GroupVersion.String() +} diff --git a/internal/data/dataapi/resolve.go b/internal/data/dataapi/resolve.go new file mode 100644 index 000000000..ca8e48a23 --- /dev/null +++ b/internal/data/dataapi/resolve.go @@ -0,0 +1,335 @@ +/* +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 dataapi + +import ( + "context" + "errors" + "fmt" + "log/slog" + "strings" + "time" + + authv1 "k8s.io/api/authorization/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/client-go/discovery" + "k8s.io/client-go/kubernetes" + "k8s.io/client-go/rest" +) + +// discoveryTimeout bounds one discovery request. +// +// It exists because the discovery client cannot be handed a context: client-go's +// ServerResourcesForGroupVersion takes none and issues its request with context.TODO(). So the +// calling command's deadline does not reach this request, and neither does Ctrl-C — and every +// `d8 data` subcommand now starts with it, before doing any work of its own. +// +// Leaving the configuration's timeout at zero does NOT mean the request is unbounded: client-go's +// setDiscoveryDefaults substitutes 32s. It means the bound is someone else's, fixed, and unrelated +// to how long the caller was willing to wait — `d8 data export delete` gives itself 25s and would +// still sit here for 32. +const discoveryTimeout = 30 * time.Second + +// discriminatorVerb is the verb resolution asks about. Every `d8 data` subcommand reads the CR it +// operates on, so "get" is the one permission all of them need; resolving on the per-subcommand +// verb instead would let two subcommands pick different groups within the same cluster and give +// the user an inconsistent view. +const discriminatorVerb = "get" + +// ErrNoBackend reports that no candidate group serves the resource at all: neither producing +// module is installed, or neither has finished installing its CRDs. +var ErrNoBackend = errors.New("no module in this cluster serves this resource") + +// ErrForbidden reports that the cluster does serve the resource, but the calling user is not +// authorized for any group that serves it. This is deliberately distinct from ErrNoBackend: the +// fix is an RBAC grant, not enabling a module. +var ErrForbidden = errors.New("not authorized to use this resource") + +// ResourceLister reports which resources one group/version serves. +// *discovery.DiscoveryClient satisfies it. +type ResourceLister interface { + ServerResourcesForGroupVersion(groupVersion string) (*metav1.APIResourceList, error) +} + +// AccessReviewer answers "may the current user do this", via SelfSubjectAccessReview. +// client-go's typed SelfSubjectAccessReviewInterface satisfies it. +// +// SelfSubjectAccessReview is used rather than a trial request because it is the only check every +// authenticated user may perform (ClusterRole system:basic-user is bound to system:authenticated) +// and because it does not create, read or mutate anything in the target namespace. +type AccessReviewer interface { + Create(ctx context.Context, ssar *authv1.SelfSubjectAccessReview, opts metav1.CreateOptions) (*authv1.SelfSubjectAccessReview, error) +} + +// access is the outcome of asking whether the user may use one candidate group. It is +// three-valued on purpose: a cluster whose role bindings deny SelfSubjectAccessReview itself +// leaves the question open, and an open question must not read as a denial. +type access int + +const ( + accessUnknown access = iota + accessAllowed + accessDenied +) + +// candidates lists the groups to consider, most preferred first. storage-foundation wins a tie +// because it is the module that supersedes the other: a cluster serving both is one that has +// storage-foundation enabled. +func candidates() []Backend { + return []Backend{ + {GroupVersion: FoundationGroupVersion, Module: foundationModule}, + {GroupVersion: LegacyGroupVersion, Module: legacyModule}, + } +} + +// Resolve picks the group to address resource through, for a user acting in namespace. +// +// It answers two independent questions per candidate and combines them, because each alone is +// ambiguous: +// +// - discovery: does the API server serve this group at all? RBAC does not affect the answer — +// a role naming a group that no CRD backs still parses, so permission alone cannot tell an +// installed module from an uninstalled one. +// - SelfSubjectAccessReview: may this user read the resource in this namespace? Discovery does +// not affect the answer, so a served group alone cannot tell an authorized user from one +// whose grants were left behind by a previous edition. +// +// The first candidate that is both served and not denied wins. When nothing qualifies, the +// returned error distinguishes "no module serves it" (ErrNoBackend) from "it is served but you +// may not use it" (ErrForbidden), naming what was found either way. +func Resolve(ctx context.Context, cfg *rest.Config, resource, namespace string, log *slog.Logger) (Backend, error) { + if cfg == nil { + return Backend{}, fmt.Errorf("resolve %s API group: no REST config", resource) + } + + if err := ctx.Err(); err != nil { + // Wrapped like every other failure of this function: unwrapped, the user sees a bare + // "context canceled" with no hint that it was group resolution that ran out of time. + // errors.Is still reaches context.Canceled through the %w. + return Backend{}, fmt.Errorf("resolve %s API group: %w", resource, err) + } + + // The discovery client gets its own bounded copy of the configuration; see discoveryTimeout + // for why the caller's context cannot bound it instead. + discoveryConfig := rest.CopyConfig(cfg) + discoveryConfig.Timeout = discoveryBudget(ctx, discoveryConfig.Timeout) + + discoveryClient, err := discovery.NewDiscoveryClientForConfig(discoveryConfig) + if err != nil { + return Backend{}, fmt.Errorf("resolve %s API group: build discovery client: %w", resource, err) + } + + clientset, err := kubernetes.NewForConfig(cfg) + if err != nil { + return Backend{}, fmt.Errorf("resolve %s API group: build Kubernetes client: %w", resource, err) + } + + return resolve(ctx, discoveryClient, clientset.AuthorizationV1().SelfSubjectAccessReviews(), resource, namespace, log) +} + +// resolve is Resolve with its two clients injected, so the decision table can be tested without +// a cluster. +func resolve( + ctx context.Context, + lister ResourceLister, + reviewer AccessReviewer, + resource, namespace string, + log *slog.Logger, +) (Backend, error) { + if log == nil { + log = slog.Default() + } + + // Candidates are consulted one at a time, in preference order, and the first that qualifies + // ends the search. Probing all of them up front would make the command depend on the health + // of a group it was never going to use: on a cluster that runs storage-foundation, a failing + // discovery endpoint for the other group would break `d8 data` outright, even though nothing + // in the run would have addressed it. It also spends two round trips where one settles the + // question. + var denied []Backend + + for _, backend := range candidates() { + served, err := serves(lister, backend.GroupVersion, resource) + if err != nil { + return Backend{}, fmt.Errorf("resolve %s API group: discovering %s: %w", resource, backend, err) + } + + if !served { + continue + } + + // accessUnknown counts as usable: the review machinery was unavailable, and refusing to + // act on a question we could not ask would break clusters whose only fault is a + // non-standard binding of system:basic-user. A real denial still surfaces, as a 403 on + // the request that follows. + if mayGet(ctx, reviewer, backend.GroupVersion, resource, namespace, log) == accessDenied { + denied = append(denied, backend) + continue + } + + log.Debug("Resolved data API group", + slog.String("resource", resource), + slog.String("group_version", backend.String()), + slog.String("module", backend.Module)) + + return backend, nil + } + + return Backend{}, resolutionError(denied, resource, namespace) +} + +// discoveryBudget picks the timeout to put on the discovery configuration: the caller's remaining +// time when it is shorter than discoveryTimeout, and discoveryTimeout otherwise. A timeout already +// configured by the caller is never lengthened. +// +// Never returns zero. Zero is not "no limit" here — client-go would substitute its own 32s — it is +// "a limit nobody in this call chain chose", which is the whole thing this function replaces. +func discoveryBudget(ctx context.Context, configured time.Duration) time.Duration { + budget := discoveryTimeout + + if deadline, ok := ctx.Deadline(); ok { + if remaining := time.Until(deadline); remaining < budget { + budget = remaining + } + } + + if configured > 0 && configured < budget { + budget = configured + } + + if budget < time.Millisecond { + budget = time.Millisecond + } + + return budget +} + +// resolutionError renders the failure that fits what the probes actually found, so the message +// names the one thing that has to change. +func resolutionError(denied []Backend, resource, namespace string) error { + if len(denied) > 0 { + served := make([]string, 0, len(denied)) + for _, b := range denied { + served = append(served, b.String()) + } + + return fmt.Errorf( + "%w: this cluster serves %s as %s, but your account may not %s it in namespace %q; "+ + "check with: d8 k auth can-i %s %s.%s -n %s", + ErrForbidden, + resource, strings.Join(served, " and "), + discriminatorVerb, namespace, + discriminatorVerb, resource, denied[0].GroupVersion.Group, namespace, + ) + } + + return fmt.Errorf( + "%w: %s is served by the %s module (%s) and by the %s module (%s), and this cluster serves neither; "+ + "enable one of them, or check that its CRDs finished installing", + ErrNoBackend, + resource, + foundationModule, FoundationGroupVersion, + legacyModule, LegacyGroupVersion, + ) +} + +// serves reports whether the API server serves resource under gv. A group the server does not +// know is a 404, which is an answer ("not served") rather than a failure; anything else is a +// failure, because guessing past a broken discovery endpoint would silently pick the wrong group. +func serves(lister ResourceLister, gv schema.GroupVersion, resource string) (bool, error) { + list, err := lister.ServerResourcesForGroupVersion(gv.String()) + if err != nil { + if apierrors.IsNotFound(err) { + return false, nil + } + + return false, err + } + + if list == nil { + return false, nil + } + + for _, r := range list.APIResources { + // Exact match only: the same listing carries subresources such as "dataexports/status", + // whose presence says nothing about the resource itself being servable. + if r.Name == resource { + return true, nil + } + } + + return false, nil +} + +// mayGet asks the API server whether the current user may read resource in namespace. A failed +// review is reported as accessUnknown rather than a denial — see the caller for why. +func mayGet( + ctx context.Context, + reviewer AccessReviewer, + gv schema.GroupVersion, + resource, namespace string, + log *slog.Logger, +) access { + if reviewer == nil { + return accessUnknown + } + + review := &authv1.SelfSubjectAccessReview{ + Spec: authv1.SelfSubjectAccessReviewSpec{ + ResourceAttributes: &authv1.ResourceAttributes{ + Namespace: namespace, + Verb: discriminatorVerb, + Group: gv.Group, + Version: gv.Version, + Resource: resource, + }, + }, + } + + result, err := reviewer.Create(ctx, review, metav1.CreateOptions{}) + if err != nil { + log.Debug("Access review unavailable, falling back to discovery order", + slog.String("group_version", gv.String()), + slog.String("resource", resource), + slog.String("error", err.Error())) + + return accessUnknown + } + + if result == nil { + return accessUnknown + } + + if result.Status.Allowed { + return accessAllowed + } + + // An evaluation error means the authorizer could not decide, which is not the same as a + // decision to deny; treating it as denial would skip a group the user can in fact use. + if result.Status.EvaluationError != "" { + log.Debug("Access review could not evaluate, falling back to discovery order", + slog.String("group_version", gv.String()), + slog.String("resource", resource), + slog.String("evaluation_error", result.Status.EvaluationError)) + + return accessUnknown + } + + return accessDenied +} diff --git a/internal/data/dataapi/resolve_test.go b/internal/data/dataapi/resolve_test.go new file mode 100644 index 000000000..40be1ef17 --- /dev/null +++ b/internal/data/dataapi/resolve_test.go @@ -0,0 +1,503 @@ +/* +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 dataapi + +import ( + "context" + "errors" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + authv1 "k8s.io/api/authorization/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/client-go/rest" +) + +// errDiscoveryBroken stands in for a discovery endpoint that fails for a reason other than "this +// group does not exist". +var errDiscoveryBroken = errors.New("discovery unreachable") + +// fakeLister answers discovery from a fixed table. A group absent from served is reported the way +// a real API server reports an unknown group: 404, not an empty listing. +type fakeLister struct { + served map[string][]string + err error + // errFor fails discovery for one group/version only, the way a single broken endpoint does. + errFor map[string]error + asked []string +} + +func (f *fakeLister) ServerResourcesForGroupVersion(groupVersion string) (*metav1.APIResourceList, error) { + f.asked = append(f.asked, groupVersion) + + if f.err != nil { + return nil, f.err + } + + if err, ok := f.errFor[groupVersion]; ok { + return nil, err + } + + resources, ok := f.served[groupVersion] + if !ok { + return nil, apierrors.NewNotFound(schema.GroupResource{Resource: "groupversion"}, groupVersion) + } + + list := &metav1.APIResourceList{GroupVersion: groupVersion} + for _, name := range resources { + list.APIResources = append(list.APIResources, metav1.APIResource{Name: name}) + } + + return list, nil +} + +// fakeReviewer answers SelfSubjectAccessReview from a fixed table keyed by API group, and records +// what it was asked so the tests can pin the question as well as the answer. +type fakeReviewer struct { + allow map[string]bool + evalErr map[string]string + err error + asked []authv1.ResourceAttributes + askCount int +} + +func (f *fakeReviewer) Create( + _ context.Context, + ssar *authv1.SelfSubjectAccessReview, + _ metav1.CreateOptions, +) (*authv1.SelfSubjectAccessReview, error) { + f.askCount++ + + if ssar.Spec.ResourceAttributes != nil { + f.asked = append(f.asked, *ssar.Spec.ResourceAttributes) + } + + if f.err != nil { + return nil, f.err + } + + group := "" + if ssar.Spec.ResourceAttributes != nil { + group = ssar.Spec.ResourceAttributes.Group + } + + out := ssar.DeepCopy() + out.Status.Allowed = f.allow[group] + out.Status.EvaluationError = f.evalErr[group] + + return out, nil +} + +func bothGroupsServing(resource string) map[string][]string { + return map[string][]string{ + FoundationGroupVersion.String(): {resource}, + LegacyGroupVersion.String(): {resource}, + } +} + +// TestResolve_DecisionTable pins the full cross product of the two questions resolution asks. +// Each row states a cluster the CLI has to work against, and neither question alone separates the +// rows: "served but forbidden" and "authorized but not served" differ only in which of the two +// answers is negative, and they demand opposite fixes from the operator. +func TestResolve_DecisionTable(t *testing.T) { + t.Parallel() + + const resource = ResourceDataExports + + tests := []struct { + name string + served map[string][]string + allow map[string]bool + wantGroup string + wantErrIs error + // wantErrHas lists every substring the message must carry. A single expected substring + // would not distinguish "named both groups" from "named the last one it looked at", and + // an operator reading a half-message goes and grants RBAC on the wrong group. + wantErrHas []string + }{ + { + name: "both served and both authorized: storage-foundation wins", + served: bothGroupsServing(resource), + allow: map[string]bool{FoundationGroup: true, LegacyGroup: true}, + wantGroup: FoundationGroup, + }, + { + name: "only storage-foundation served", + served: map[string][]string{FoundationGroupVersion.String(): {resource}}, + allow: map[string]bool{FoundationGroup: true}, + wantGroup: FoundationGroup, + }, + { + name: "only the older producer served", + served: map[string][]string{LegacyGroupVersion.String(): {resource}}, + allow: map[string]bool{LegacyGroup: true}, + wantGroup: LegacyGroup, + }, + { + // The case this whole mechanism exists for: an edition that carries both CRDs but + // grants the user rights on the older one only. + name: "both served, authorized on the older producer only", + served: bothGroupsServing(resource), + allow: map[string]bool{LegacyGroup: true}, + wantGroup: LegacyGroup, + }, + { + name: "both served, authorized on neither", + served: bothGroupsServing(resource), + allow: map[string]bool{}, + wantErrIs: ErrForbidden, + wantErrHas: []string{ + "auth can-i", + FoundationGroupVersion.String(), + LegacyGroupVersion.String(), + }, + }, + { + name: "served by storage-foundation only, and forbidden", + served: map[string][]string{FoundationGroupVersion.String(): {resource}}, + allow: map[string]bool{LegacyGroup: true}, + wantErrIs: ErrForbidden, + wantErrHas: []string{FoundationGroup}, + }, + { + // Rights left behind by a previous edition must not be mistaken for an installed + // module: nothing serves the resource, so no group can be addressed at all. + name: "authorized everywhere, served nowhere", + served: map[string][]string{}, + allow: map[string]bool{FoundationGroup: true, LegacyGroup: true}, + wantErrIs: ErrNoBackend, + wantErrHas: []string{"neither"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + backend, err := resolve( + context.Background(), + &fakeLister{served: tt.served}, + &fakeReviewer{allow: tt.allow}, + resource, "my-ns", nil, + ) + + if tt.wantErrIs != nil { + require.Error(t, err) + assert.ErrorIs(t, err, tt.wantErrIs) + + for _, want := range tt.wantErrHas { + assert.Contains(t, err.Error(), want) + } + + return + } + + require.NoError(t, err) + assert.Equal(t, tt.wantGroup, backend.GroupVersion.Group) + assert.Equal(t, Version, backend.GroupVersion.Version) + assert.Equal(t, tt.wantGroup == LegacyGroup, backend.Legacy()) + }) + } +} + +// TestResolve_NoBackendMessageNamesBothModules guards the operator-facing half of the ErrNoBackend +// message: the error has to say which modules would provide the resource, because "not served" on +// its own leaves the reader with nothing to enable. +func TestResolve_NoBackendMessageNamesBothModules(t *testing.T) { + t.Parallel() + + _, err := resolve( + context.Background(), + &fakeLister{served: map[string][]string{}}, + &fakeReviewer{}, + ResourceDataImports, "my-ns", nil, + ) + + require.Error(t, err) + assert.ErrorIs(t, err, ErrNoBackend) + assert.Contains(t, err.Error(), foundationModule) + assert.Contains(t, err.Error(), legacyModule) + assert.Contains(t, err.Error(), ResourceDataImports) +} + +// TestResolve_AsksAboutTheCallersNamespace pins the question, not just the answer. A review run +// against the wrong namespace (or the empty one, which asks about cluster-wide permission) returns +// a confidently wrong verdict for a user who holds rights in exactly one namespace. +func TestResolve_AsksAboutTheCallersNamespace(t *testing.T) { + t.Parallel() + + reviewer := &fakeReviewer{allow: map[string]bool{FoundationGroup: true}} + + _, err := resolve( + context.Background(), + &fakeLister{served: bothGroupsServing(ResourceDataExports)}, + reviewer, + ResourceDataExports, "team-a", nil, + ) + require.NoError(t, err) + + require.NotEmpty(t, reviewer.asked) + first := reviewer.asked[0] + assert.Equal(t, "team-a", first.Namespace) + assert.Equal(t, "get", first.Verb) + assert.Equal(t, ResourceDataExports, first.Resource) + assert.Equal(t, FoundationGroup, first.Group) + assert.Equal(t, Version, first.Version) +} + +// TestResolve_SkipsReviewForUnservedGroups keeps resolution from spending a round trip asking about +// a group the server does not serve, whose answer could not change the outcome. +func TestResolve_SkipsReviewForUnservedGroups(t *testing.T) { + t.Parallel() + + reviewer := &fakeReviewer{allow: map[string]bool{FoundationGroup: true, LegacyGroup: true}} + + _, err := resolve( + context.Background(), + &fakeLister{served: map[string][]string{FoundationGroupVersion.String(): {ResourceDataExports}}}, + reviewer, + ResourceDataExports, "my-ns", nil, + ) + require.NoError(t, err) + + assert.Equal(t, 1, reviewer.askCount, "only the served group is worth asking about") +} + +// TestResolve_UnavailableReviewFallsBackToDiscoveryOrder covers a cluster whose role bindings deny +// SelfSubjectAccessReview itself. The question could not be asked, which is not an answer of "no": +// refusing to act would break a user whose only fault is a non-standard binding of +// system:basic-user, and a real denial still arrives as a 403 on the request that follows. +func TestResolve_UnavailableReviewFallsBackToDiscoveryOrder(t *testing.T) { + t.Parallel() + + for _, tt := range []struct { + name string + reviewer *fakeReviewer + }{ + { + name: "review request rejected", + reviewer: &fakeReviewer{err: apierrors.NewForbidden(schema.GroupResource{}, "", errors.New("denied"))}, + }, + { + name: "authorizer could not evaluate", + reviewer: &fakeReviewer{evalErr: map[string]string{FoundationGroup: "webhook timeout"}}, + }, + } { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + backend, err := resolve( + context.Background(), + &fakeLister{served: bothGroupsServing(ResourceDataExports)}, + tt.reviewer, + ResourceDataExports, "my-ns", nil, + ) + + require.NoError(t, err) + assert.Equal(t, FoundationGroup, backend.GroupVersion.Group) + }) + } +} + +// TestResolve_BrokenDiscoveryIsAnError separates "the server says this group does not exist" from +// "the server could not be asked". Only the first is an answer; guessing past the second would +// silently address the wrong producer. +func TestResolve_BrokenDiscoveryIsAnError(t *testing.T) { + t.Parallel() + + _, err := resolve( + context.Background(), + &fakeLister{err: errDiscoveryBroken}, + &fakeReviewer{allow: map[string]bool{FoundationGroup: true}}, + ResourceDataExports, "my-ns", nil, + ) + + require.Error(t, err) + assert.ErrorIs(t, err, errDiscoveryBroken) + assert.NotErrorIs(t, err, ErrNoBackend, "an unreachable discovery endpoint is not proof of an absent module") +} + +// TestServes_IgnoresSubresources pins the exact-match rule: a listing that carries +// "dataexports/status" but not "dataexports" describes a group that cannot serve the resource, and +// a prefix match would read it as one that can. +func TestServes_IgnoresSubresources(t *testing.T) { + t.Parallel() + + lister := &fakeLister{served: map[string][]string{ + FoundationGroupVersion.String(): {"dataexports/status", "dataimports"}, + }} + + found, err := serves(lister, FoundationGroupVersion, ResourceDataExports) + require.NoError(t, err) + assert.False(t, found, "a subresource entry must not count as the resource itself") + + found, err = serves(lister, FoundationGroupVersion, ResourceDataImports) + require.NoError(t, err) + assert.True(t, found) +} + +// TestResolve_ResolvesPerResource covers a cluster that serves one CRD of the pair from each +// producer. Resolving per group instead of per resource would pick one producer for both and then +// address a resource it does not serve. +func TestResolve_ResolvesPerResource(t *testing.T) { + t.Parallel() + + lister := &fakeLister{served: map[string][]string{ + FoundationGroupVersion.String(): {ResourceDataExports}, + LegacyGroupVersion.String(): {ResourceDataImports}, + }} + allow := map[string]bool{FoundationGroup: true, LegacyGroup: true} + + exports, err := resolve(context.Background(), lister, &fakeReviewer{allow: allow}, ResourceDataExports, "my-ns", nil) + require.NoError(t, err) + assert.Equal(t, FoundationGroup, exports.GroupVersion.Group) + + imports, err := resolve(context.Background(), lister, &fakeReviewer{allow: allow}, ResourceDataImports, "my-ns", nil) + require.NoError(t, err) + assert.Equal(t, LegacyGroup, imports.GroupVersion.Group) +} + +// TestResolve_ExpiredContextNamesTheStep pins that the earliest exit still says which step ran out +// of time. Unwrapped, the caller chain hands cobra a bare "context canceled", and the user is told +// only that something timed out — not that it was group resolution, which is the one step they had +// no way to know their command performs. +func TestResolve_ExpiredContextNamesTheStep(t *testing.T) { + t.Parallel() + + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + _, err := Resolve(ctx, &rest.Config{Host: "https://127.0.0.1:1"}, ResourceDataExports, "my-ns", nil) + require.Error(t, err) + assert.ErrorIs(t, err, context.Canceled, "the cause must stay recognisable through the wrapping") + assert.Contains(t, err.Error(), ResourceDataExports) + assert.Contains(t, err.Error(), "API group") +} + +// TestResolve_NilConfig keeps the exported entry point from panicking on a client that never +// obtained a configuration. +func TestResolve_NilConfig(t *testing.T) { + t.Parallel() + + _, err := Resolve(context.Background(), nil, ResourceDataExports, "my-ns", nil) + require.Error(t, err) + assert.Contains(t, err.Error(), "no REST config") +} + +// TestResolve_StopsAtTheFirstQualifyingCandidate pins that resolution consults candidates one at a +// time and stops. Probing both up front is not merely wasteful: it makes every `d8 data` run +// depend on the health of a group the run was never going to address — see the sibling test for +// the failure that causes. +func TestResolve_StopsAtTheFirstQualifyingCandidate(t *testing.T) { + t.Parallel() + + lister := &fakeLister{served: bothGroupsServing(ResourceDataExports)} + reviewer := &fakeReviewer{allow: map[string]bool{FoundationGroup: true, LegacyGroup: true}} + + backend, err := resolve(context.Background(), lister, reviewer, ResourceDataExports, "my-ns", nil) + require.NoError(t, err) + assert.Equal(t, FoundationGroup, backend.GroupVersion.Group) + + assert.Equal(t, []string{FoundationGroupVersion.String()}, lister.asked, + "the second candidate must not be discovered once the first one qualifies") + assert.Equal(t, 1, reviewer.askCount, "the second candidate must not be reviewed either") +} + +// TestResolve_UnusedGroupHealthDoesNotMatter covers the cluster this whole ordering exists for: a +// normal storage-foundation installation where the OTHER group's discovery endpoint is broken. +// +// storage.deckhouse.io is not exclusive to storage-volume-data-manager — other Deckhouse modules +// serve resources under it — so its endpoint can be unhealthy on a cluster that has nothing to do +// with that module. Before resolution became lazy, that unhealthy endpoint failed `d8 data` +// outright, on a cluster where the command had every reason to work. +func TestResolve_UnusedGroupHealthDoesNotMatter(t *testing.T) { + t.Parallel() + + lister := &fakeLister{ + served: bothGroupsServing(ResourceDataExports), + errFor: map[string]error{LegacyGroupVersion.String(): errDiscoveryBroken}, + } + + backend, err := resolve( + context.Background(), lister, + &fakeReviewer{allow: map[string]bool{FoundationGroup: true}}, + ResourceDataExports, "my-ns", nil, + ) + + require.NoError(t, err) + assert.Equal(t, FoundationGroup, backend.GroupVersion.Group) +} + +// TestDiscoveryBudget pins the timeout put on the discovery configuration. +// +// The value that must never come out is zero, and not because zero would leave the request +// unbounded — client-go's setDiscoveryDefaults substitutes 32s for it. Because that 32s is a limit +// nobody in this call chain chose: it ignores the caller's remaining time, which is the only reason +// this budget exists. Zero is also exactly what a naive "just use the remaining time" would produce +// for an already-expired context. +func TestDiscoveryBudget(t *testing.T) { + t.Parallel() + + t.Run("no deadline and no configured timeout: the default", func(t *testing.T) { + t.Parallel() + assert.Equal(t, discoveryTimeout, discoveryBudget(context.Background(), 0)) + }) + + t.Run("a nearer deadline wins over the default", func(t *testing.T) { + t.Parallel() + + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) + defer cancel() + + got := discoveryBudget(ctx, 0) + assert.Positive(t, got) + assert.Less(t, got, discoveryTimeout) + }) + + t.Run("a farther deadline does not lengthen the default", func(t *testing.T) { + t.Parallel() + + ctx, cancel := context.WithTimeout(context.Background(), time.Hour) + defer cancel() + + assert.Equal(t, discoveryTimeout, discoveryBudget(ctx, 0)) + }) + + t.Run("a shorter configured timeout is kept", func(t *testing.T) { + t.Parallel() + assert.Equal(t, time.Second, discoveryBudget(context.Background(), time.Second)) + }) + + t.Run("a longer configured timeout is not honoured", func(t *testing.T) { + t.Parallel() + assert.Equal(t, discoveryTimeout, discoveryBudget(context.Background(), time.Hour)) + }) + + t.Run("an expired deadline still yields a non-zero timeout", func(t *testing.T) { + t.Parallel() + + ctx, cancel := context.WithTimeout(context.Background(), -time.Second) + defer cancel() + + assert.Positive(t, discoveryBudget(ctx, 0), + "zero hands the bound to client-go's 32s default instead of the caller's deadline") + }) +} diff --git a/internal/data/dataexport/README.md b/internal/data/dataexport/README.md index ef1460199..92cffb57f 100644 --- a/internal/data/dataexport/README.md +++ b/internal/data/dataexport/README.md @@ -11,6 +11,34 @@ derived from it). Supported target kinds and their CLI aliases: | `VirtualDisk` | `vd`, `virtualdisk` | | `VirtualDiskSnapshot` | `vds`, `virtualdisksnapshot` | +### Which module serves DataExport + +Two Deckhouse modules serve the same CRD under different API groups: + +| Module | API group | +| --- | --- | +| `storage-foundation` | `storage-foundation.deckhouse.io/v1alpha1` | +| `storage-volume-data-manager` | `storage.deckhouse.io/v1alpha1` | + +`d8 data` picks one per invocation instead of being built against a fixed group, because editions +differ in which module they ship: `storage-foundation` supersedes the other, but an edition without +it carries `storage-volume-data-manager` alone. + +The choice is made from two questions the API server is asked before the command does any work: +which of the two groups it serves (discovery), and which of them the calling user may read in the +target namespace (`SelfSubjectAccessReview`). `storage-foundation` wins when both answers are yes +for it; otherwise the other module is used. Both questions are answerable by any authenticated +user, so this works for the ordinary users who run `d8 data` and not only for cluster admins. + +The two answers are kept apart on purpose, so the error you get names the one thing that has to +change: a group that nothing serves means the module is not enabled, while a served group you are +not authorized for means an RBAC grant is missing. Check the latter with: + +```shell +d8 k auth can-i get dataexports.storage-foundation.deckhouse.io -n NAMESPACE +d8 k auth can-i get dataexports.storage.deckhouse.io -n NAMESPACE +``` + ### Available Commands: * create - Create k8s DataExport object. diff --git a/internal/data/dataexport/api/v1alpha1/register.go b/internal/data/dataexport/api/v1alpha1/register.go index a33b633d0..2ee1d313b 100644 --- a/internal/data/dataexport/api/v1alpha1/register.go +++ b/internal/data/dataexport/api/v1alpha1/register.go @@ -20,30 +20,51 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" + + "github.com/deckhouse/deckhouse-cli/internal/data/dataapi" ) const ( - APIGroup = "storage-foundation.deckhouse.io" - APIVersion = "v1alpha1" + // APIGroup is the group storage-foundation serves DataExport under. It is the default this + // package registers, not the only group these types are ever addressed through: a cluster + // running storage-volume-data-manager instead serves the same kind under + // dataapi.LegacyGroup, and callers reach it with AddToSchemeFor. + APIGroup = dataapi.FoundationGroup + APIVersion = dataapi.Version ) // SchemeGroupVersion is group version used to register these objects var ( - SchemeGroupVersion = schema.GroupVersion{ - Group: APIGroup, - Version: APIVersion, - } - SchemeBuilder = runtime.NewSchemeBuilder(AddKnownTypes) - AddToScheme = SchemeBuilder.AddToScheme + SchemeGroupVersion = dataapi.FoundationGroupVersion + SchemeBuilder = runtime.NewSchemeBuilder(AddKnownTypes) + AddToScheme = SchemeBuilder.AddToScheme ) -// Adds the list of known types to Scheme. +// AddKnownTypes registers the DataExport types under SchemeGroupVersion (storage-foundation). +// Callers that resolved the served group at runtime use AddToSchemeFor instead. func AddKnownTypes(scheme *runtime.Scheme) error { - scheme.AddKnownTypes(SchemeGroupVersion, + return addKnownTypesFor(SchemeGroupVersion, scheme) +} + +// AddToSchemeFor returns a scheme builder that registers the DataExport types under gv. +// +// The Go types are shared by both producers because the wire shapes agree on everything the CLI +// sends and reads; only the group differs, and TargetRefSpec.Group — which the older CRD has no +// property for — is pruned by that CRD's structural schema rather than rejected. Registering one +// scheme per resolved group keeps exactly one GroupVersionKind mapped to each type, which is what +// the controller-runtime client requires to address the object at all. +func AddToSchemeFor(gv schema.GroupVersion) func(*runtime.Scheme) error { + return func(scheme *runtime.Scheme) error { + return addKnownTypesFor(gv, scheme) + } +} + +func addKnownTypesFor(gv schema.GroupVersion, scheme *runtime.Scheme) error { + scheme.AddKnownTypes(gv, &DataExport{}, &DataExportList{}, ) - metav1.AddToGroupVersion(scheme, SchemeGroupVersion) + metav1.AddToGroupVersion(scheme, gv) return nil } diff --git a/internal/data/dataexport/api/v1alpha1/register_test.go b/internal/data/dataexport/api/v1alpha1/register_test.go new file mode 100644 index 000000000..762a0e66b --- /dev/null +++ b/internal/data/dataexport/api/v1alpha1/register_test.go @@ -0,0 +1,125 @@ +/* +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 v1alpha1 + +import ( + "encoding/json" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + + "github.com/deckhouse/deckhouse-cli/internal/data/dataapi" +) + +// TestAPIGroup_DefaultsToStorageFoundation pins the DataExport default API group as a literal +// rather than through the constant it is defined from, which would only restate itself. +// +// This is the group used whenever the caller does not resolve one — every caller outside +// `d8 data`, notably `d8 snapshot download`, which needs storage-foundation and nothing else. +func TestAPIGroup_DefaultsToStorageFoundation(t *testing.T) { + t.Parallel() + + assert.Equal(t, "storage-foundation.deckhouse.io", APIGroup) + assert.Equal(t, "storage-foundation.deckhouse.io/v1alpha1", SchemeGroupVersion.String()) + assert.NotEqual(t, "storage.deckhouse.io", APIGroup) +} + +// TestAddToSchemeFor_RegistersUnderTheRequestedGroup covers the runtime-selected registration: the +// scheme has to map the Go types to whichever group was resolved, and to exactly one of them. +// +// Both halves matter. Registering the requested group is what makes the older producer reachable +// at all; registering only it is what keeps the client able to address the object — a type mapped +// to two GroupVersionKinds makes controller-runtime refuse the request as ambiguous, which no test +// asserting the happy group alone would catch. +func TestAddToSchemeFor_RegistersUnderTheRequestedGroup(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + gv schema.GroupVersion + wantGroup string + }{ + { + name: "storage-foundation group", + gv: dataapi.FoundationGroupVersion, + wantGroup: "storage-foundation.deckhouse.io", + }, + { + name: "storage-volume-data-manager group", + gv: dataapi.LegacyGroupVersion, + wantGroup: "storage.deckhouse.io", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + scheme := runtime.NewScheme() + require.NoError(t, AddToSchemeFor(tt.gv)(scheme)) + + for _, obj := range []runtime.Object{&DataExport{}, &DataExportList{}} { + gvks, _, err := scheme.ObjectKinds(obj) + require.NoError(t, err) + require.Len(t, gvks, 1, "the type must map to exactly one GroupVersionKind") + assert.Equal(t, tt.wantGroup, gvks[0].Group) + assert.Equal(t, "v1alpha1", gvks[0].Version) + } + }) + } +} + +// TestAddToScheme_RegistersUnderTheDefaultGroup guards the drift between the APIGroup constant and +// what the default scheme builder actually registers, which an assertion on the constant alone +// would miss. +func TestAddToScheme_RegistersUnderTheDefaultGroup(t *testing.T) { + t.Parallel() + + scheme := runtime.NewScheme() + require.NoError(t, AddToScheme(scheme)) + + gvks, _, err := scheme.ObjectKinds(&DataExport{}) + require.NoError(t, err) + require.Len(t, gvks, 1) + assert.Equal(t, "storage-foundation.deckhouse.io", gvks[0].Group) + assert.Equal(t, "DataExport", gvks[0].Kind) +} + +// TestTargetRefSpec_OmitsGroupWhenEmpty pins the serialised shape of the one field the two +// producers disagree about. +// +// A core-group target (PersistentVolumeClaim) has an empty group, and the key must then be absent +// rather than sent as "": the older producer's schema declares no group property and prunes it +// either way, but storage-foundation reads it, and an explicit empty string there is a claim about +// the target rather than the absence of one. +func TestTargetRefSpec_OmitsGroupWhenEmpty(t *testing.T) { + t.Parallel() + + raw, err := json.Marshal(TargetRefSpec{Kind: "PersistentVolumeClaim", Name: "my-pvc"}) + require.NoError(t, err) + + assert.NotContains(t, string(raw), `"group"`) + assert.Contains(t, string(raw), `"kind":"PersistentVolumeClaim"`) + assert.Contains(t, string(raw), `"name":"my-pvc"`) + + raw, err = json.Marshal(TargetRefSpec{Group: "snapshot.storage.k8s.io", Kind: "VolumeSnapshot", Name: "my-vs"}) + require.NoError(t, err) + assert.Contains(t, string(raw), `"group":"snapshot.storage.k8s.io"`) +} diff --git a/internal/data/dataexport/cmd/create/create.go b/internal/data/dataexport/cmd/create/create.go index 5c11eb928..9225c8cc3 100644 --- a/internal/data/dataexport/cmd/create/create.go +++ b/internal/data/dataexport/cmd/create/create.go @@ -26,7 +26,6 @@ import ( "github.com/spf13/cobra" dataio "github.com/deckhouse/deckhouse-cli/internal/data" - "github.com/deckhouse/deckhouse-cli/internal/data/dataexport/api/v1alpha1" "github.com/deckhouse/deckhouse-cli/internal/data/dataexport/util" safeClient "github.com/deckhouse/deckhouse-cli/pkg/libsaferequest/client" ) @@ -117,7 +116,7 @@ func Run(ctx context.Context, log *slog.Logger, cmd *cobra.Command, args []strin return err } - rtClient, err := sc.NewRTClient(v1alpha1.AddToScheme) + _, rtClient, err := util.ResolveClientFunc(ctx, sc, namespace, log) if err != nil { return err } diff --git a/internal/data/dataexport/cmd/delete/delete.go b/internal/data/dataexport/cmd/delete/delete.go index 7365b3c40..172834d5a 100644 --- a/internal/data/dataexport/cmd/delete/delete.go +++ b/internal/data/dataexport/cmd/delete/delete.go @@ -26,7 +26,6 @@ import ( "github.com/spf13/cobra" dataio "github.com/deckhouse/deckhouse-cli/internal/data" - "github.com/deckhouse/deckhouse-cli/internal/data/dataexport/api/v1alpha1" "github.com/deckhouse/deckhouse-cli/internal/data/dataexport/util" safeClient "github.com/deckhouse/deckhouse-cli/pkg/libsaferequest/client" ) @@ -88,7 +87,7 @@ func Run(ctx context.Context, log *slog.Logger, cmd *cobra.Command, args []strin return err } - rtClient, err := safeClient.NewRTClient(v1alpha1.AddToScheme) + _, rtClient, err := util.ResolveClientFunc(ctx, safeClient, namespace, log) if err != nil { return err } diff --git a/internal/data/dataexport/cmd/download/download.go b/internal/data/dataexport/cmd/download/download.go index d8b94a76b..eeaa26400 100644 --- a/internal/data/dataexport/cmd/download/download.go +++ b/internal/data/dataexport/cmd/download/download.go @@ -32,7 +32,6 @@ import ( "github.com/spf13/cobra" dataio "github.com/deckhouse/deckhouse-cli/internal/data" - "github.com/deckhouse/deckhouse-cli/internal/data/dataexport/api/v1alpha1" "github.com/deckhouse/deckhouse-cli/internal/data/dataexport/util" safeClient "github.com/deckhouse/deckhouse-cli/pkg/libsaferequest/client" ) @@ -292,7 +291,7 @@ func Run(ctx context.Context, log *slog.Logger, cmd *cobra.Command, args []strin return err } - rtClient, err := sClient.NewRTClient(v1alpha1.AddToScheme) + backend, rtClient, err := util.ResolveClientFunc(ctx, sClient, namespace, log) if err != nil { return err } @@ -314,7 +313,7 @@ func Run(ctx context.Context, log *slog.Logger, cmd *cobra.Command, args []strin log.Info("DataExport created", slog.String("name", deName), slog.String("namespace", namespace)) - url, volumeMode, subClient, err := util.PrepareDownloadFunc(ctx, log, deName, namespace, publish, sClient) + url, volumeMode, subClient, err := util.PrepareDownloadFunc(ctx, log, backend, deName, namespace, publish, sClient) if err != nil { return err } diff --git a/internal/data/dataexport/cmd/download/download_http_test.go b/internal/data/dataexport/cmd/download/download_http_test.go index acef92ef8..c2b097f5a 100644 --- a/internal/data/dataexport/cmd/download/download_http_test.go +++ b/internal/data/dataexport/cmd/download/download_http_test.go @@ -13,7 +13,9 @@ import ( "github.com/stretchr/testify/require" ctrlclient "sigs.k8s.io/controller-runtime/pkg/client" + fakeclient "sigs.k8s.io/controller-runtime/pkg/client/fake" + "github.com/deckhouse/deckhouse-cli/internal/data/dataapi" "github.com/deckhouse/deckhouse-cli/internal/data/dataexport/util" safereq "github.com/deckhouse/deckhouse-cli/pkg/libsaferequest/client" ) @@ -43,7 +45,7 @@ func TestDownloadFilesystem_OK(t *testing.T) { // stub PrepareDownload / CreateDataExporterIfNeeded origPrep := util.PrepareDownloadFunc origCreate := util.CreateDataExporterIfNeededFunc - util.PrepareDownloadFunc = func(_ context.Context, _ *slog.Logger, _, _ string, _ bool, _ *safereq.SafeClient) (string, string, *safereq.SafeClient, error) { + util.PrepareDownloadFunc = func(_ context.Context, _ *slog.Logger, _ dataapi.Backend, _, _ string, _ bool, _ *safereq.SafeClient) (string, string, *safereq.SafeClient, error) { return srv.URL + "/api/v1/files", "Filesystem", newNoAuthSafe(), nil } util.CreateDataExporterIfNeededFunc = func(_ context.Context, _ *slog.Logger, de, _ string, _ bool, _ string, _ ctrlclient.Client) (string, error) { @@ -78,7 +80,7 @@ func TestDownloadFilesystem_BadPath(t *testing.T) { origPrep := util.PrepareDownloadFunc origCreate := util.CreateDataExporterIfNeededFunc - util.PrepareDownloadFunc = func(_ context.Context, _ *slog.Logger, _, _ string, _ bool, _ *safereq.SafeClient) (string, string, *safereq.SafeClient, error) { + util.PrepareDownloadFunc = func(_ context.Context, _ *slog.Logger, _ dataapi.Backend, _, _ string, _ bool, _ *safereq.SafeClient) (string, string, *safereq.SafeClient, error) { return srv.URL + "/api/v1/files", "Block", newNoAuthSafe(), nil } util.CreateDataExporterIfNeededFunc = func(_ context.Context, _ *slog.Logger, de, _ string, _ bool, _ string, _ ctrlclient.Client) (string, error) { @@ -102,7 +104,7 @@ func TestDownloadBlock_OK(t *testing.T) { origPrep := util.PrepareDownloadFunc origCreate := util.CreateDataExporterIfNeededFunc - util.PrepareDownloadFunc = func(_ context.Context, _ *slog.Logger, _, _ string, _ bool, _ *safereq.SafeClient) (string, string, *safereq.SafeClient, error) { + util.PrepareDownloadFunc = func(_ context.Context, _ *slog.Logger, _ dataapi.Backend, _, _ string, _ bool, _ *safereq.SafeClient) (string, string, *safereq.SafeClient, error) { return srv.URL + "/api/v1/block", "Block", newNoAuthSafe(), nil } util.CreateDataExporterIfNeededFunc = func(_ context.Context, _ *slog.Logger, de, _ string, _ bool, _ string, _ ctrlclient.Client) (string, error) { @@ -155,7 +157,7 @@ func TestDownloadFilesystem_SocketInDirIsSkipped(t *testing.T) { origPrep := util.PrepareDownloadFunc origCreate := util.CreateDataExporterIfNeededFunc - util.PrepareDownloadFunc = func(_ context.Context, _ *slog.Logger, _, _ string, _ bool, _ *safereq.SafeClient) (string, string, *safereq.SafeClient, error) { + util.PrepareDownloadFunc = func(_ context.Context, _ *slog.Logger, _ dataapi.Backend, _, _ string, _ bool, _ *safereq.SafeClient) (string, string, *safereq.SafeClient, error) { return srv.URL + "/api/v1/files", "Filesystem", newNoAuthSafe(), nil } util.CreateDataExporterIfNeededFunc = func(_ context.Context, _ *slog.Logger, de, _ string, _ bool, _ string, _ ctrlclient.Client) (string, error) { @@ -218,7 +220,7 @@ func TestDownloadFilesystem_RecursiveWithSocketsCompletes(t *testing.T) { origPrep := util.PrepareDownloadFunc origCreate := util.CreateDataExporterIfNeededFunc - util.PrepareDownloadFunc = func(_ context.Context, _ *slog.Logger, _, _ string, _ bool, _ *safereq.SafeClient) (string, string, *safereq.SafeClient, error) { + util.PrepareDownloadFunc = func(_ context.Context, _ *slog.Logger, _ dataapi.Backend, _, _ string, _ bool, _ *safereq.SafeClient) (string, string, *safereq.SafeClient, error) { return srv.URL + "/api/v1/files", "Filesystem", newNoAuthSafe(), nil } util.CreateDataExporterIfNeededFunc = func(_ context.Context, _ *slog.Logger, de, _ string, _ bool, _ string, _ ctrlclient.Client) (string, error) { @@ -256,7 +258,7 @@ func TestDownloadBlock_WrongEndpoint(t *testing.T) { origPrep := util.PrepareDownloadFunc origCreate := util.CreateDataExporterIfNeededFunc - util.PrepareDownloadFunc = func(_ context.Context, _ *slog.Logger, _, _ string, _ bool, _ *safereq.SafeClient) (string, string, *safereq.SafeClient, error) { + util.PrepareDownloadFunc = func(_ context.Context, _ *slog.Logger, _ dataapi.Backend, _, _ string, _ bool, _ *safereq.SafeClient) (string, string, *safereq.SafeClient, error) { return srv.URL + "/api/v1/block", "Filesystem", newNoAuthSafe(), nil } util.CreateDataExporterIfNeededFunc = func(_ context.Context, _ *slog.Logger, de, _ string, _ bool, _ string, _ ctrlclient.Client) (string, error) { @@ -270,3 +272,20 @@ func TestDownloadBlock_WrongEndpoint(t *testing.T) { cmd.SetErr(io.Discard) require.NoError(t, cmd.Execute()) } + +// TestMain stubs the API-group resolution for every test in this package. +// +// Resolution is the one step of Run that talks to a real API server, and it now runs before any +// of the behaviour these tests cover. Left unstubbed, each test would dial whatever cluster the +// developer's kubeconfig happens to point at — which is how these tests started failing against a +// live stand rather than against their own httptest server. The decision table resolution +// implements is covered in internal/data/dataapi instead. +func TestMain(m *testing.M) { + util.ResolveClientFunc = func(_ context.Context, _ *safereq.SafeClient, _ string, _ *slog.Logger) (dataapi.Backend, ctrlclient.Client, error) { + return dataapi.Backend{GroupVersion: dataapi.FoundationGroupVersion, Module: "storage-foundation"}, + fakeclient.NewClientBuilder().Build(), + nil + } + + os.Exit(m.Run()) +} diff --git a/internal/data/dataexport/cmd/list/list.go b/internal/data/dataexport/cmd/list/list.go index e71bd94f6..e39ee8739 100644 --- a/internal/data/dataexport/cmd/list/list.go +++ b/internal/data/dataexport/cmd/list/list.go @@ -32,7 +32,7 @@ import ( "k8s.io/apimachinery/pkg/api/resource" dataio "github.com/deckhouse/deckhouse-cli/internal/data" - "github.com/deckhouse/deckhouse-cli/internal/data/dataexport/api/v1alpha1" + "github.com/deckhouse/deckhouse-cli/internal/data/dataapi" "github.com/deckhouse/deckhouse-cli/internal/data/dataexport/util" safeClient "github.com/deckhouse/deckhouse-cli/pkg/libsaferequest/client" ) @@ -91,12 +91,13 @@ func parseArgs(args []string) ( /*deName*/ string /*srcPath*/, string, error) { func downloadFunc( ctx context.Context, log *slog.Logger, + backend dataapi.Backend, namespace, deName, srcPath string, publish bool, sClient *safeClient.SafeClient, foo func(body io.Reader) error, ) error { - url, volumeMode, subClient, err := util.PrepareDownloadFunc(ctx, log, deName, namespace, publish, sClient) + url, volumeMode, subClient, err := util.PrepareDownloadFunc(ctx, log, backend, deName, namespace, publish, sClient) if err != nil { return err } @@ -187,7 +188,7 @@ func Run(ctx context.Context, log *slog.Logger, cmd *cobra.Command, args []strin return err } - rtClient, err := sClient.NewRTClient(v1alpha1.AddToScheme) + backend, rtClient, err := util.ResolveClientFunc(ctx, sClient, namespace, log) if err != nil { return err } @@ -209,7 +210,7 @@ func Run(ctx context.Context, log *slog.Logger, cmd *cobra.Command, args []strin log.Info("DataExport created", slog.String("name", deName), slog.String("namespace", namespace)) - err = downloadFunc(ctx, log, namespace, deName, srcPath, publish, sClient, func(body io.Reader) error { + err = downloadFunc(ctx, log, backend, namespace, deName, srcPath, publish, sClient, func(body io.Reader) error { _, err := io.Copy(os.Stdout, body) if err == io.EOF { err = nil diff --git a/internal/data/dataexport/cmd/list/list_http_test.go b/internal/data/dataexport/cmd/list/list_http_test.go index 1a8c5f3d5..8af8d2b33 100644 --- a/internal/data/dataexport/cmd/list/list_http_test.go +++ b/internal/data/dataexport/cmd/list/list_http_test.go @@ -12,7 +12,9 @@ import ( "github.com/stretchr/testify/require" ctrlclient "sigs.k8s.io/controller-runtime/pkg/client" + fakeclient "sigs.k8s.io/controller-runtime/pkg/client/fake" + "github.com/deckhouse/deckhouse-cli/internal/data/dataapi" "github.com/deckhouse/deckhouse-cli/internal/data/dataexport/util" safereq "github.com/deckhouse/deckhouse-cli/pkg/libsaferequest/client" ) @@ -40,7 +42,7 @@ func TestListFilesystem_OK(t *testing.T) { origPrep := util.PrepareDownloadFunc origCreate := util.CreateDataExporterIfNeededFunc - util.PrepareDownloadFunc = func(_ context.Context, _ *slog.Logger, _, _ string, _ bool, _ *safereq.SafeClient) (string, string, *safereq.SafeClient, error) { + util.PrepareDownloadFunc = func(_ context.Context, _ *slog.Logger, _ dataapi.Backend, _, _ string, _ bool, _ *safereq.SafeClient) (string, string, *safereq.SafeClient, error) { // Re-enable support for unauthenticated requests inside unit tests. safereq.SupportNoAuth = true return srv.URL + "/api/v1/files", "Filesystem", newSafe(), nil @@ -76,7 +78,7 @@ func TestListBlock_OK(t *testing.T) { origPrep := util.PrepareDownloadFunc origCreate := util.CreateDataExporterIfNeededFunc - util.PrepareDownloadFunc = func(_ context.Context, _ *slog.Logger, _, _ string, _ bool, _ *safereq.SafeClient) (string, string, *safereq.SafeClient, error) { + util.PrepareDownloadFunc = func(_ context.Context, _ *slog.Logger, _ dataapi.Backend, _, _ string, _ bool, _ *safereq.SafeClient) (string, string, *safereq.SafeClient, error) { // Re-enable support for unauthenticated requests inside unit tests. safereq.SupportNoAuth = true return srv.URL + "/api/v1/block", "Block", newSafe(), nil @@ -111,7 +113,7 @@ func TestListFilesystem_NotDir(t *testing.T) { origPrep := util.PrepareDownloadFunc origCreate := util.CreateDataExporterIfNeededFunc - util.PrepareDownloadFunc = func(_ context.Context, _ *slog.Logger, _, _ string, _ bool, _ *safereq.SafeClient) (string, string, *safereq.SafeClient, error) { + util.PrepareDownloadFunc = func(_ context.Context, _ *slog.Logger, _ dataapi.Backend, _, _ string, _ bool, _ *safereq.SafeClient) (string, string, *safereq.SafeClient, error) { // Re-enable support for unauthenticated requests inside unit tests. safereq.SupportNoAuth = true return srv.URL + "/api/v1/files", "Filesystem", newSafe(), nil @@ -129,3 +131,20 @@ func TestListFilesystem_NotDir(t *testing.T) { require.Error(t, err) require.Contains(t, err.Error(), "invalid source path") } + +// TestMain stubs the API-group resolution for every test in this package. +// +// Resolution is the one step of Run that talks to a real API server, and it now runs before any +// of the behaviour these tests cover. Left unstubbed, each test would dial whatever cluster the +// developer's kubeconfig happens to point at — which is how these tests started failing against a +// live stand rather than against their own httptest server. The decision table resolution +// implements is covered in internal/data/dataapi instead. +func TestMain(m *testing.M) { + util.ResolveClientFunc = func(_ context.Context, _ *safereq.SafeClient, _ string, _ *slog.Logger) (dataapi.Backend, ctrlclient.Client, error) { + return dataapi.Backend{GroupVersion: dataapi.FoundationGroupVersion, Module: "storage-foundation"}, + fakeclient.NewClientBuilder().Build(), + nil + } + + os.Exit(m.Run()) +} diff --git a/internal/data/dataexport/util/util.go b/internal/data/dataexport/util/util.go index 54607c20b..073116d7c 100644 --- a/internal/data/dataexport/util/util.go +++ b/internal/data/dataexport/util/util.go @@ -31,6 +31,7 @@ import ( ctrlrtclient "sigs.k8s.io/controller-runtime/pkg/client" dataio "github.com/deckhouse/deckhouse-cli/internal/data" + "github.com/deckhouse/deckhouse-cli/internal/data/dataapi" "github.com/deckhouse/deckhouse-cli/internal/data/dataexport/api/v1alpha1" safeClient "github.com/deckhouse/deckhouse-cli/pkg/libsaferequest/client" ) @@ -39,11 +40,41 @@ import ( var ( PrepareDownloadFunc = PrepareDownload CreateDataExporterIfNeededFunc = CreateDataExporterIfNeeded + // ResolveClientFunc is stubbable because resolution is the first thing a command does and + // the only step that contacts the API server before any of the work under test. + ResolveClientFunc = ResolveClient ) // var instead of const to allow test override. var maxRetryAttempts = 60 +// ResolveClient resolves which of the two producers serves DataExport to this user in this +// namespace, and returns both the answer and a control-plane client bound to it. +// +// Every `d8 data export` subcommand starts here rather than registering a fixed group, because +// the same binary ships to clusters that serve only storage-foundation's group, only +// storage-volume-data-manager's, or both with the user authorized for one of them. The resolved +// backend is returned alongside the client because callers that build their own client later +// (PrepareDownload) must bind it to the same group. +func ResolveClient( + ctx context.Context, + sClient *safeClient.SafeClient, + namespace string, + log *slog.Logger, +) (dataapi.Backend, ctrlrtclient.Client, error) { + backend, err := dataapi.Resolve(ctx, sClient.RESTConfig(), dataapi.ResourceDataExports, namespace, log) + if err != nil { + return dataapi.Backend{}, nil, err + } + + rtClient, err := sClient.NewRTClient(v1alpha1.AddToSchemeFor(backend.GroupVersion)) + if err != nil { + return dataapi.Backend{}, nil, err + } + + return backend, rtClient, nil +} + func GetDataExport(ctx context.Context, deName, namespace string, rtClient ctrlrtclient.Client) (*v1alpha1.DataExport, error) { deObj := &v1alpha1.DataExport{} @@ -52,17 +83,12 @@ func GetDataExport(ctx context.Context, deName, namespace string, rtClient ctrlr return nil, fmt.Errorf("kube Get dataexport: %s", err.Error()) } - // check DataExport is Ready. No status in new version of dataexport - for _, condition := range deObj.Status.Conditions { - if condition.Type == "Ready" { - if condition.Status != "True" { - return nil, fmt.Errorf("DataExport %s/%s is not Ready: %s (%s)", - deObj.ObjectMeta.Namespace, deObj.ObjectMeta.Name, - condition.Message, condition.Reason) - } - - break - } + // An object with no Ready condition at all has not been reconciled yet rather than failed, + // so only a Ready condition that is present and not True is an error here. + if notReady := dataio.NotReady(deObj.Status.Conditions); notReady != nil { + return nil, fmt.Errorf("DataExport %s/%s is not Ready: %s (%s)", + deObj.ObjectMeta.Namespace, deObj.ObjectMeta.Name, + notReady.Message, notReady.Reason) } return deObj, nil @@ -80,38 +106,39 @@ func GetDataExportWithRestart(ctx context.Context, deName, namespace string, rtC return nil, fmt.Errorf("kube Get dataexport with restart: %s", err.Error()) } - for _, condition := range deObj.Status.Conditions { - // The DataExport catalog no longer carries a standalone "Expired" condition; expiry is now the - // Ready condition with Status=False and Reason="Expired" (plus status.phase=Expired). Detect it - // on the Ready condition and auto-restart the export, rather than waiting for the producer's GC - // (which only deletes an expired DataExport after its retention TTL). - if condition.Type != "Ready" { - continue + // An expired export is recreated here rather than waited out: after expiry the producer's + // garbage collector only removes the object once its retention TTL runs out, so polling + // would stall for the whole of that retention. Both producers' spellings of expiry are + // recognised by dataio.IsExpired. + switch { + case dataio.IsExpired(deObj.Status.Conditions): + // Resolved BEFORE the delete, and never fatal. Everything needed to rebuild the + // export has to be in hand before the existing one is destroyed: failing between the + // two would leave the user with neither, and this object is theirs, not ours. + group := recreateTargetGroup(deObj, log) + + if err := DeleteDataExport(ctx, deName, namespace, rtClient); err != nil { + return nil, err } - switch { - case condition.Status == "False" && condition.Reason == "Expired": - if err := DeleteDataExport(ctx, deName, namespace, rtClient); err != nil { - return nil, err - } - - if err := CreateDataExport( - ctx, - deName, namespace, "", - deObj.Spec.TargetRef.Group, - deObj.Spec.TargetRef.Kind, - deObj.Spec.TargetRef.Name, - deObj.Spec.Publish, rtClient, - ); err != nil { - return nil, err - } - // Recreated: keep retrying until the fresh export becomes Ready. - returnErr = fmt.Errorf("DataExport %s/%s expired; recreated, waiting for the new export to become Ready", - deObj.ObjectMeta.Namespace, deObj.ObjectMeta.Name) - case condition.Status != "True": + if err := CreateDataExport( + ctx, + deName, namespace, "", + group, + deObj.Spec.TargetRef.Kind, + deObj.Spec.TargetRef.Name, + deObj.Spec.Publish, rtClient, + ); err != nil { + return nil, err + } + // Recreated: keep retrying until the fresh export becomes Ready. + returnErr = fmt.Errorf("DataExport %s/%s expired; recreated, waiting for the new export to become Ready", + deObj.ObjectMeta.Namespace, deObj.ObjectMeta.Name) + default: + if notReady := dataio.NotReady(deObj.Status.Conditions); notReady != nil { returnErr = fmt.Errorf("DataExport %s/%s is not Ready: %s (%s)", deObj.ObjectMeta.Namespace, deObj.ObjectMeta.Name, - condition.Message, condition.Reason) + notReady.Message, notReady.Reason) } } // check DataExport Url @@ -150,6 +177,31 @@ func GetDataExportWithRestart(ctx context.Context, deName, namespace string, rtC return deObj, nil } +// recreateTargetGroup picks the API group to stamp on an export being rebuilt after expiry. +// +// The group is derived from the kind rather than read back off the object, because +// storage-volume-data-manager's schema has no targetRef.group property and prunes the one the CLI +// sent: an export read from that producer never carries a group, and copying the empty value would +// retarget the rebuilt export at the core group. +// +// An unrecognised kind is not an error here. storage-foundation puts no enum on targetRef.kind and +// documents that any snapshot kind is resolved generically, so a DataExport may legitimately target +// a kind this CLI has never heard of. For those, whatever group the server already recorded is the +// best available answer — and far better than refusing, which at this point in the recreate would +// destroy an object the user still owns. +func recreateTargetGroup(deObj *v1alpha1.DataExport, log *slog.Logger) string { + group, err := dataio.KindToGroup(deObj.Spec.TargetRef.Kind) + if err == nil { + return group + } + + log.Warn("Rebuilding an expired DataExport for a target kind this CLI does not know; keeping the group recorded on the object", + slog.String("kind", deObj.Spec.TargetRef.Kind), + slog.String("group", deObj.Spec.TargetRef.Group)) + + return deObj.Spec.TargetRef.Group +} + func CreateDataExporterIfNeeded(ctx context.Context, log *slog.Logger, deName, namespace string, publish bool, ttl string, rtClient ctrlrtclient.Client) (string, error) { var volumeKind, volumeName string @@ -223,12 +275,11 @@ func CreateDataExport(ctx context.Context, deName, namespace, ttl, group, kind, ttl = dataio.DefaultTTL } - // Create dataexport object + // Create dataexport object. TypeMeta is left empty on purpose: the client stamps the + // apiVersion from the scheme it was built with, which is the group resolved for this run — + // a literal here would name one producer's group on every request, including requests to + // the other one. deCfg := &v1alpha1.DataExport{ - TypeMeta: metav1.TypeMeta{ - APIVersion: "deckhouse.io/v1alpha1", - Kind: "DataExport", - }, ObjectMeta: metav1.ObjectMeta{ Name: deName, Namespace: namespace, @@ -321,14 +372,18 @@ func getExportStatus(ctx context.Context, log *slog.Logger, deName, namespace st return podURL, volumeMode, internalCAData, nil } -func PrepareDownload(ctx context.Context, log *slog.Logger, deName, namespace string, publish bool, sClient *safeClient.SafeClient) (string, string, *safeClient.SafeClient, error) { +// PrepareDownload waits for the export to be usable and returns the data-plane URL, the volume +// mode and a client trusting the exporter's CA. backend is the group resolved for this run; it +// builds this function's own control-plane client, which would otherwise default to +// storage-foundation's group regardless of what the cluster serves. +func PrepareDownload(ctx context.Context, log *slog.Logger, backend dataapi.Backend, deName, namespace string, publish bool, sClient *safeClient.SafeClient) (string, string, *safeClient.SafeClient, error) { var ( url, volumeMode string subClient *safeClient.SafeClient decodedBytes []byte ) - rtClient, err := sClient.NewRTClient(v1alpha1.AddToScheme) + rtClient, err := sClient.NewRTClient(v1alpha1.AddToSchemeFor(backend.GroupVersion)) if err != nil { return "", "", nil, err } diff --git a/internal/data/dataexport/util/util_test.go b/internal/data/dataexport/util/util_test.go index f08dcb6a5..32142168e 100644 --- a/internal/data/dataexport/util/util_test.go +++ b/internal/data/dataexport/util/util_test.go @@ -13,6 +13,7 @@ import ( ctrlclient "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" + "github.com/deckhouse/deckhouse-cli/internal/data/dataapi" "github.com/deckhouse/deckhouse-cli/internal/data/dataexport/api/v1alpha1" ) @@ -342,6 +343,118 @@ func TestGetDataExportWithRestart_ExpiredRecreates(t *testing.T) { assert.True(t, recreated.Spec.Publish, "Publish must be carried over to the fresh export") } +// TestGetDataExportWithRestart_StandaloneExpiredConditionRecreates covers the other producer's +// spelling of expiry: a standalone Type=="Expired" condition must trigger the same recreate as +// storage-foundation's Ready=False/Reason=Expired. +// +// The fixture pairs Expired=True with a still-True Ready deliberately, because that is the state a +// real storage-volume-data-manager cluster passes through rather than an invented one: its exporter +// pod raises the standalone condition, and only the next controller reconcile mirrors it onto +// Ready. A client that waited for the Ready spelling would keep polling a dead exporter for however +// long that reconcile takes, and then for the producer's whole retention TTL if it never lands. +// +// Deliberately NOT t.Parallel: it overrides the package-level maxRetryAttempts. +func TestGetDataExportWithRestart_StandaloneExpiredConditionRecreates(t *testing.T) { + scheme := runtime.NewScheme() + require.NoError(t, v1alpha1.AddToSchemeFor(dataapi.LegacyGroupVersion)(scheme)) + + ctx := context.Background() + logger := slog.Default() + + orig := maxRetryAttempts + maxRetryAttempts = -1 + t.Cleanup(func() { maxRetryAttempts = orig }) + + expired := &v1alpha1.DataExport{ + ObjectMeta: metav1.ObjectMeta{Name: "test-de", Namespace: "test-ns"}, + Spec: v1alpha1.DataexportSpec{ + // No group: the older producer's schema has no such property and prunes the one the + // CLI sent, so this is how the object reads back from it. + TargetRef: v1alpha1.TargetRefSpec{Kind: "VolumeSnapshot", Name: "my-vs"}, + }, + Status: v1alpha1.DataExportStatus{ + URL: "https://10.0.0.1:8085/", + Conditions: []metav1.Condition{ + {Type: "Expired", Status: metav1.ConditionTrue, Reason: "Expired", Message: "ttl reached"}, + readyCond(metav1.ConditionTrue, "PodReady", "Pod is ready"), + }, + }, + } + + c := fake.NewClientBuilder().WithScheme(scheme).WithObjects(expired).Build() + + _, err := GetDataExportWithRestart(ctx, "test-de", "test-ns", c, logger) + require.Error(t, err) + assert.Contains(t, err.Error(), "expired; recreated, waiting") + + var recreated v1alpha1.DataExport + require.NoError(t, c.Get(ctx, ctrlclient.ObjectKey{Name: "test-de", Namespace: "test-ns"}, &recreated)) + assert.Empty(t, recreated.Status.Conditions, "the stale status must be gone after the recreate") + assert.Equal(t, "VolumeSnapshot", recreated.Spec.TargetRef.Kind) + assert.Equal(t, "my-vs", recreated.Spec.TargetRef.Name) + + // The group is derived from the kind rather than copied off the stale object. Copying would + // carry the empty group above into the fresh export, which addresses the core group — a + // different object entirely on a cluster that also has a core-group resource by that name. + assert.Equal(t, "snapshot.storage.k8s.io", recreated.Spec.TargetRef.Group, + "the recreate must derive the group from the kind, not inherit the pruned empty one") +} + +// TestGetDataExportWithRestart_UnknownTargetKindKeepsTheObject covers a DataExport whose target +// kind this CLI does not know. storage-foundation puts no enum on targetRef.kind and documents that +// any snapshot kind is resolved generically through the leaf's bound SnapshotContent, so such an +// object is legitimate rather than corrupt. +// +// The property under test is that the user keeps their object. Resolving the group after the delete +// turns an unrecognised kind into a destroyed DataExport: the delete lands, the group lookup fails, +// and nothing is recreated. Whatever else the CLI does with a kind it cannot classify, it must not +// be that. +// +// Deliberately NOT t.Parallel: it overrides the package-level maxRetryAttempts. +func TestGetDataExportWithRestart_UnknownTargetKindKeepsTheObject(t *testing.T) { + scheme := runtime.NewScheme() + require.NoError(t, v1alpha1.AddToScheme(scheme)) + + ctx := context.Background() + logger := slog.Default() + + orig := maxRetryAttempts + maxRetryAttempts = -1 + t.Cleanup(func() { maxRetryAttempts = orig }) + + expired := &v1alpha1.DataExport{ + ObjectMeta: metav1.ObjectMeta{Name: "test-de", Namespace: "test-ns"}, + Spec: v1alpha1.DataexportSpec{ + TargetRef: v1alpha1.TargetRefSpec{ + Group: "demo.deckhouse.io", + Kind: "SomeDomainSnapshot", + Name: "my-target", + }, + }, + Status: v1alpha1.DataExportStatus{ + URL: "https://10.0.0.1:8085/", + Conditions: []metav1.Condition{ + readyCond(metav1.ConditionFalse, "Expired", "export idle timeout reached"), + }, + }, + } + + c := fake.NewClientBuilder().WithScheme(scheme).WithObjects(expired).Build() + + _, err := GetDataExportWithRestart(ctx, "test-de", "test-ns", c, logger) + require.Error(t, err) + assert.Contains(t, err.Error(), "expired; recreated, waiting", + "an unknown kind must not turn the recreate into a bare failure") + + var recreated v1alpha1.DataExport + require.NoError(t, c.Get(ctx, ctrlclient.ObjectKey{Name: "test-de", Namespace: "test-ns"}, &recreated), + "the object must still exist: it is the user's, and we deleted the previous one") + assert.Equal(t, "SomeDomainSnapshot", recreated.Spec.TargetRef.Kind) + assert.Equal(t, "my-target", recreated.Spec.TargetRef.Name) + assert.Equal(t, "demo.deckhouse.io", recreated.Spec.TargetRef.Group, + "with no derivation available, the group recorded on the object is the best answer left") +} + func TestEnsureDataExportPublish(t *testing.T) { scheme := runtime.NewScheme() require.NoError(t, v1alpha1.AddToScheme(scheme)) diff --git a/internal/data/dataimport/README.md b/internal/data/dataimport/README.md index 688cd4dcd..424c0ccdf 100644 --- a/internal/data/dataimport/README.md +++ b/internal/data/dataimport/README.md @@ -1,15 +1,47 @@ # DataImport Subcommand for the Deckhouse CLI to create/import/delete data via DataImport resources. -This command drives the **standalone PVC import** mode of `DataImport` -(`spec.mode: CreatePVC`): the target PVC is fully defined by the +This command drives the **standalone PVC import**: the target PVC is fully defined by the PVC template you pass to `create`, data is uploaded straight into it, and no snapshot/`VolumeSnapshotContent` artifact is produced. (The snapshot-leaf import mode is driven separately by `d8 snapshot upload`.) +The two modules that serve `DataImport` spell this destination differently, and `d8 data import` +writes whichever spelling belongs to the module it resolved (see below): `spec.mode: CreatePVC` +plus a root `spec.pvcTemplate` for `storage-foundation`, and `spec.targetRef` carrying the same +template for `storage-volume-data-manager`. + The PVC template **must** carry `metadata.name` — the DataImport targets the PVC by that name; `create` rejects a template without it before contacting the API server. +### Which module serves DataImport + +Two Deckhouse modules serve the same CRD under different API groups: + +| Module | API group | +| --- | --- | +| `storage-foundation` | `storage-foundation.deckhouse.io/v1alpha1` | +| `storage-volume-data-manager` | `storage.deckhouse.io/v1alpha1` | + +`d8 data` picks one per invocation instead of being built against a fixed group, because editions +differ in which module they ship: `storage-foundation` supersedes the other, but an edition without +it carries `storage-volume-data-manager` alone. + +The choice is made from two questions the API server is asked before the command does any work: +which of the two groups it serves (discovery), and which of them the calling user may read in the +target namespace (`SelfSubjectAccessReview`). `storage-foundation` wins when both answers are yes +for it; otherwise the other module is used. Both questions are answerable by any authenticated +user, so this works for the ordinary users who run `d8 data` and not only for cluster admins. + +The two answers are kept apart on purpose, so the error you get names the one thing that has to +change: a group that nothing serves means the module is not enabled, while a served group you are +not authorized for means an RBAC grant is missing. Check the latter with: + +```shell +d8 k auth can-i get dataimports.storage-foundation.deckhouse.io -n NAMESPACE +d8 k auth can-i get dataimports.storage.deckhouse.io -n NAMESPACE +``` + ### Available Commands - create – ensure PVC (from template) and create DataImport - upload – upload file contents to the DataImport endpoint diff --git a/internal/data/dataimport/api/v1alpha1/data_import.go b/internal/data/dataimport/api/v1alpha1/data_import.go index c817b2bdf..80fd4b018 100644 --- a/internal/data/dataimport/api/v1alpha1/data_import.go +++ b/internal/data/dataimport/api/v1alpha1/data_import.go @@ -41,8 +41,9 @@ type DataImportList struct { } // DataImportMode is spec.mode — the explicit discriminator that selects what a DataImport does -// with the imported bytes. It replaced the former polymorphic targetRef.kind discrimination; the -// current CRD has no spec.targetRef at all (it is pruned by the structural schema). +// with the imported bytes. storage-foundation introduced it in place of the polymorphic +// targetRef.kind discrimination its predecessor used, and its schema has no spec.targetRef at +// all. Left empty when addressing storage-volume-data-manager, which is the other way round. // See storage-foundation/api/v1alpha1/data_import.go for the SSOT. type DataImportMode string @@ -53,9 +54,22 @@ type DataImportMode string // unstructured spec. const DataImportModeCreatePVC DataImportMode = "CreatePVC" -// DataImportSpec mirrors the CreatePVC subset of the unified DataImport CRD spec that the CLI -// produces. The CRD CEL rules require pvcTemplate and forbid snapshotRef/storageParams when -// mode == CreatePVC, so those PopulateData-only fields are deliberately absent here. +// DataImportSpec mirrors the PVC-creating subset of the DataImport CRD spec that the CLI +// produces, in both shapes that subset has been given. +// +// The two producers disagree on how the destination is expressed, and unlike DataExport the +// disagreement is structural rather than an extra field: +// +// - storage-foundation discriminates on Mode and reads PvcTemplate from the spec root. Its CEL +// rules require pvcTemplate and forbid snapshotRef/storageParams when mode == CreatePVC, so +// those PopulateData-only fields are deliberately absent from this struct. +// - storage-volume-data-manager has no mode at all and requires TargetRef, carrying the same +// template one level down. +// +// Exactly one of the two shapes is filled per request, chosen from the resolved backend; the +// other stays nil and is omitted from the wire. Filling both would survive today only because +// each CRD prunes what it does not declare, and would start writing a field with different +// meaning the moment either producer grows the other's key. // +k8s:deepcopy-gen=true type DataImportSpec struct { TTL string `json:"ttl"` @@ -67,11 +81,53 @@ type DataImportSpec struct { // `--wffc=false` (and hence the flag's own default) impossible to express. WaitForFirstConsumer bool `json:"waitForFirstConsumer"` + // Mode is the storage-foundation discriminator. Left empty for the older producer, whose + // schema has no such property. Mode DataImportMode `json:"mode,omitempty"` - // PvcTemplate fully describes the destination PVC. Its metadata.name is mandatory — the - // controller names the imported PVC after it and the server CEL rejects an empty name. + // PvcTemplate fully describes the destination PVC for storage-foundation. Its metadata.name + // is mandatory — the controller names the imported PVC after it and the server CEL rejects + // an empty name. PvcTemplate *PersistentVolumeClaimTemplateSpec `json:"pvcTemplate,omitempty"` + + // TargetRef is the storage-volume-data-manager destination, which nests the same template + // under a required targetRef. Left nil for storage-foundation, whose schema prunes it. + TargetRef *DataImportTargetRefSpec `json:"targetRef,omitempty"` +} + +// DataImportTargetRefSpec is the storage-volume-data-manager shape of the import destination. +// Its Kind enum admits PersistentVolumeClaim only, which is also the only destination +// `d8 data import` creates. +// +k8s:deepcopy-gen=true +type DataImportTargetRefSpec struct { + // Kind is the destination kind; PersistentVolumeClaimKind is the only accepted value. + Kind string `json:"kind"` + + // PvcTemplate fully describes the destination PVC, exactly as the storage-foundation shape + // spells it one level up. + PvcTemplate *PersistentVolumeClaimTemplateSpec `json:"pvcTemplate,omitempty"` +} + +// PersistentVolumeClaimKind is the only DataImport destination kind either producer accepts. +const PersistentVolumeClaimKind = "PersistentVolumeClaim" + +// DestinationTemplate returns the destination PVC template whichever shape carries it, so +// readers of an object fetched from the cluster do not have to know which producer wrote it. +// Returns nil when neither shape is filled. +func (s *DataImportSpec) DestinationTemplate() *PersistentVolumeClaimTemplateSpec { + if s == nil { + return nil + } + + if s.PvcTemplate != nil { + return s.PvcTemplate + } + + if s.TargetRef != nil { + return s.TargetRef.PvcTemplate + } + + return nil } // +k8s:deepcopy-gen=true diff --git a/internal/data/dataimport/api/v1alpha1/data_import_test.go b/internal/data/dataimport/api/v1alpha1/data_import_test.go index ef8bb7ed4..dfb3140e1 100644 --- a/internal/data/dataimport/api/v1alpha1/data_import_test.go +++ b/internal/data/dataimport/api/v1alpha1/data_import_test.go @@ -47,10 +47,12 @@ func newCreatePVCTemplate() *PersistentVolumeClaimTemplateSpec { // TestDataImportSpec_JSONWireShape pins the exact set of spec keys this type puts on the wire, // because the CRD's behaviour differs per key and the difference is invisible in Go: the schema is -// non-preserving (an unknown key such as the removed spec.targetRef is pruned without an error), -// and waitForFirstConsumer is defaulted to true (so an absent key is NOT the same as false). -// A stray omitempty on the wrong field therefore changes what the server does while every -// struct-level assertion keeps passing. +// non-preserving (a key it does not declare is pruned without an error), and waitForFirstConsumer +// is defaulted to true (so an absent key is NOT the same as false). A stray omitempty on the wrong +// field therefore changes what the server does while every struct-level assertion keeps passing. +// +// Every row here builds the storage-foundation shape. The other producer's shape is pinned +// separately by TestDataImportSpec_ShapesAreMutuallyExclusiveOnTheWire. func TestDataImportSpec_JSONWireShape(t *testing.T) { t.Parallel() @@ -120,14 +122,129 @@ func TestDataImportSpec_JSONWireShape(t *testing.T) { assert.ElementsMatch(t, tt.wantKeys, gotKeys, "exact set of spec keys sent to the apiserver") assert.Equal(t, tt.wantWaitForFirstConsumer, string(decoded["waitForFirstConsumer"])) - // The discriminator is spec.mode; spec.targetRef was removed from the CRD and would be - // pruned silently, so it must never reappear here under any input. + // Every row above builds the storage-foundation shape, whose discriminator is + // spec.mode and whose schema declares no targetRef. This says nothing about the + // other producer, which requires targetRef and has no mode — that shape is pinned by + // TestDataImportSpec_ShapesAreMutuallyExclusiveOnTheWire. assert.NotContains(t, decoded, "targetRef") assert.Equal(t, `"CreatePVC"`, string(decoded["mode"])) }) } } +// TestDataImportSpec_DestinationTemplate covers the read side of the two-shaped spec: a caller +// that has fetched an object must find the PVC template regardless of which module wrote it. +// +// The recreate-on-expiry path depends on this. Reading Spec.PvcTemplate directly finds nothing in +// an object written by storage-volume-data-manager, and the recreate then aborts with "requires a +// PVC template with metadata.name set" — an error that names the template rather than the shape, +// and so points the reader at the wrong thing. +func TestDataImportSpec_DestinationTemplate(t *testing.T) { + t.Parallel() + + tpl := newCreatePVCTemplate() + + tests := []struct { + name string + spec *DataImportSpec + want *PersistentVolumeClaimTemplateSpec + }{ + { + name: "storage-foundation shape: root pvcTemplate", + spec: &DataImportSpec{Mode: DataImportModeCreatePVC, PvcTemplate: tpl}, + want: tpl, + }, + { + name: "storage-volume-data-manager shape: nested under targetRef", + spec: &DataImportSpec{TargetRef: &DataImportTargetRefSpec{ + Kind: PersistentVolumeClaimKind, + PvcTemplate: tpl, + }}, + want: tpl, + }, + { + name: "neither shape filled", + spec: &DataImportSpec{}, + want: nil, + }, + { + name: "targetRef present but carrying no template", + spec: &DataImportSpec{TargetRef: &DataImportTargetRefSpec{Kind: PersistentVolumeClaimKind}}, + want: nil, + }, + { + name: "nil receiver", + spec: nil, + want: nil, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + assert.Equal(t, tt.want, tt.spec.DestinationTemplate()) + }) + } +} + +// TestDataImportSpec_ShapesAreMutuallyExclusiveOnTheWire pins that each shape leaves the other +// key off the wire entirely. +// +// Sending both would be accepted today only because each CRD prunes what it does not declare, and +// relying on that means the request stops being correct the moment either producer grows the +// other's key — at which point the CLI would be writing a field it never meant to set. +func TestDataImportSpec_ShapesAreMutuallyExclusiveOnTheWire(t *testing.T) { + t.Parallel() + + foundation, err := json.Marshal(DataImportSpec{ + TTL: "15m", + Mode: DataImportModeCreatePVC, + PvcTemplate: newCreatePVCTemplate(), + }) + require.NoError(t, err) + assert.Contains(t, string(foundation), `"mode":"CreatePVC"`) + assert.Contains(t, string(foundation), `"pvcTemplate"`) + assert.NotContains(t, string(foundation), `"targetRef"`) + + legacy, err := json.Marshal(DataImportSpec{ + TTL: "15m", + TargetRef: &DataImportTargetRefSpec{ + Kind: PersistentVolumeClaimKind, + PvcTemplate: newCreatePVCTemplate(), + }, + }) + require.NoError(t, err) + assert.Contains(t, string(legacy), `"targetRef"`) + assert.Contains(t, string(legacy), `"kind":"PersistentVolumeClaim"`) + assert.NotContains(t, string(legacy), `"mode"`) +} + +// TestDataImportSpec_DeepCopyCarriesTargetRef guards the hand-written deepcopy against the field +// added for the older producer. A deepcopy that skips it silently shares the template between the +// original and the copy, so a mutation through one is visible through the other. +func TestDataImportSpec_DeepCopyCarriesTargetRef(t *testing.T) { + t.Parallel() + + original := &DataImportSpec{TargetRef: &DataImportTargetRefSpec{ + Kind: PersistentVolumeClaimKind, + PvcTemplate: newCreatePVCTemplate(), + }} + + copied := original.DeepCopy() + + require.NotNil(t, copied.TargetRef) + require.NotNil(t, copied.TargetRef.PvcTemplate) + assert.Equal(t, original.TargetRef.PvcTemplate.Name, copied.TargetRef.PvcTemplate.Name) + + assert.NotSame(t, original.TargetRef, copied.TargetRef, "targetRef must not be shared with the copy") + assert.NotSame(t, original.TargetRef.PvcTemplate, copied.TargetRef.PvcTemplate, + "the nested template must not be shared with the copy") + + copied.TargetRef.PvcTemplate.Name = "mutated" + assert.Equal(t, "restored-pvc", original.TargetRef.PvcTemplate.Name, + "mutating the copy must not reach the original") +} + // TestDataImportSpec_DeepCopy_DoesNotAliasPvcTemplate is not redundant boilerplate: the deepcopy // in this package is maintained by hand (there is no controller-gen or //go:generate wired up), so // a field added to PersistentVolumeClaimSpec without a matching line in DeepCopyInto produces a diff --git a/internal/data/dataimport/api/v1alpha1/register.go b/internal/data/dataimport/api/v1alpha1/register.go index b172de608..8a61617e4 100644 --- a/internal/data/dataimport/api/v1alpha1/register.go +++ b/internal/data/dataimport/api/v1alpha1/register.go @@ -20,30 +20,50 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" + + "github.com/deckhouse/deckhouse-cli/internal/data/dataapi" ) const ( - APIGroup = "storage-foundation.deckhouse.io" - APIVersion = "v1alpha1" + // APIGroup is the group storage-foundation serves DataImport under. It is the default this + // package registers, not the only group these types are ever addressed through: a cluster + // running storage-volume-data-manager instead serves the same kind under + // dataapi.LegacyGroup, and callers reach it with AddToSchemeFor. + APIGroup = dataapi.FoundationGroup + APIVersion = dataapi.Version ) // SchemeGroupVersion is group version used to register these objects var ( - SchemeGroupVersion = schema.GroupVersion{ - Group: APIGroup, - Version: APIVersion, - } - SchemeBuilder = runtime.NewSchemeBuilder(AddKnownTypes) - AddToScheme = SchemeBuilder.AddToScheme + SchemeGroupVersion = dataapi.FoundationGroupVersion + SchemeBuilder = runtime.NewSchemeBuilder(AddKnownTypes) + AddToScheme = SchemeBuilder.AddToScheme ) -// Adds the list of known types to Scheme. +// AddKnownTypes registers the DataImport types under SchemeGroupVersion (storage-foundation). +// Callers that resolved the served group at runtime use AddToSchemeFor instead. func AddKnownTypes(scheme *runtime.Scheme) error { - scheme.AddKnownTypes(SchemeGroupVersion, + return addKnownTypesFor(SchemeGroupVersion, scheme) +} + +// AddToSchemeFor returns a scheme builder that registers the DataImport types under gv. +// +// One Go type serves both producers: the spec carries both destination shapes and fills the one +// the resolved backend understands (see DataImportSpec). Registering one scheme per resolved +// group keeps exactly one GroupVersionKind mapped to each type, which is what the +// controller-runtime client requires to address the object at all. +func AddToSchemeFor(gv schema.GroupVersion) func(*runtime.Scheme) error { + return func(scheme *runtime.Scheme) error { + return addKnownTypesFor(gv, scheme) + } +} + +func addKnownTypesFor(gv schema.GroupVersion, scheme *runtime.Scheme) error { + scheme.AddKnownTypes(gv, &DataImport{}, &DataImportList{}, ) - metav1.AddToGroupVersion(scheme, SchemeGroupVersion) + metav1.AddToGroupVersion(scheme, gv) return nil } diff --git a/internal/data/dataimport/api/v1alpha1/register_test.go b/internal/data/dataimport/api/v1alpha1/register_test.go index d807e7a87..49d2a7b01 100644 --- a/internal/data/dataimport/api/v1alpha1/register_test.go +++ b/internal/data/dataimport/api/v1alpha1/register_test.go @@ -21,23 +21,71 @@ import ( "github.com/stretchr/testify/require" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + + "github.com/deckhouse/deckhouse-cli/internal/data/dataapi" ) -// TestAPIGroup_IsStorageFoundationGroup pins the DataImport API group to -// storage-foundation.deckhouse.io. The legacy storage.deckhouse.io group is removed by the -// storage-foundation 025-migrate-legacy-crds migration hook, so a regression here would take -// down every `d8 data import`/`d8 snapshot upload` client silently. +// TestAPIGroup_IsStorageFoundationGroup pins the DataImport default API group to +// storage-foundation.deckhouse.io, spelled out as a literal rather than through the constant it is +// defined from — otherwise the assertion would restate whatever the constant became. +// +// This is the group used whenever the caller does not resolve one, which is every caller outside +// `d8 data` (notably `d8 snapshot upload`). Reaching the other producer is a deliberate act, via +// AddToSchemeFor; drifting into it by default is not. func TestAPIGroup_IsStorageFoundationGroup(t *testing.T) { t.Parallel() require.Equal(t, "storage-foundation.deckhouse.io", APIGroup) require.Equal(t, "storage-foundation.deckhouse.io/v1alpha1", SchemeGroupVersion.String()) - - // Explicit negative assertion: the legacy group is being deleted by the migration - // hook, so silently drifting back to it must fail loudly rather than compile clean. require.NotEqual(t, "storage.deckhouse.io", APIGroup) } +// TestAddToSchemeFor_RegistersUnderTheRequestedGroup covers the runtime-selected registration: the +// scheme has to map the Go types to whichever group was resolved, and to exactly one of them. +// +// Both halves matter. Registering the requested group is what makes the older producer reachable +// at all; registering only it is what keeps the client able to address the object — a type mapped +// to two GroupVersionKinds makes controller-runtime refuse the request as ambiguous, which no test +// asserting the happy group alone would catch. +func TestAddToSchemeFor_RegistersUnderTheRequestedGroup(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + gv schema.GroupVersion + wantGroup string + }{ + { + name: "storage-foundation group", + gv: dataapi.FoundationGroupVersion, + wantGroup: "storage-foundation.deckhouse.io", + }, + { + name: "storage-volume-data-manager group", + gv: dataapi.LegacyGroupVersion, + wantGroup: "storage.deckhouse.io", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + scheme := runtime.NewScheme() + require.NoError(t, AddToSchemeFor(tt.gv)(scheme)) + + for _, obj := range []runtime.Object{&DataImport{}, &DataImportList{}} { + gvks, _, err := scheme.ObjectKinds(obj) + require.NoError(t, err) + require.Len(t, gvks, 1, "the type must map to exactly one GroupVersionKind") + require.Equal(t, tt.wantGroup, gvks[0].Group) + require.Equal(t, "v1alpha1", gvks[0].Version) + } + }) + } +} + // TestAddToScheme_RegistersUnderStorageFoundationGroup verifies that AddKnownTypes registers // DataImport/DataImportList under the current APIGroup, catching a drift between the constant // and the scheme registration that unit tests on APIGroup alone would miss. diff --git a/internal/data/dataimport/api/v1alpha1/zz_generated_data_export.deepcopy.go b/internal/data/dataimport/api/v1alpha1/zz_generated_data_export.deepcopy.go index e86fcba41..c9f236d36 100644 --- a/internal/data/dataimport/api/v1alpha1/zz_generated_data_export.deepcopy.go +++ b/internal/data/dataimport/api/v1alpha1/zz_generated_data_export.deepcopy.go @@ -98,9 +98,35 @@ func (in *DataImportSpec) DeepCopyInto(out *DataImportSpec) { *out = new(PersistentVolumeClaimTemplateSpec) (*in).DeepCopyInto(*out) } + if in.TargetRef != nil { + in, out := &in.TargetRef, &out.TargetRef + *out = new(DataImportTargetRefSpec) + (*in).DeepCopyInto(*out) + } return } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *DataImportTargetRefSpec) DeepCopyInto(out *DataImportTargetRefSpec) { + *out = *in + if in.PvcTemplate != nil { + in, out := &in.PvcTemplate, &out.PvcTemplate + *out = new(PersistentVolumeClaimTemplateSpec) + (*in).DeepCopyInto(*out) + } + return +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new DataImportTargetRefSpec. +func (in *DataImportTargetRefSpec) DeepCopy() *DataImportTargetRefSpec { + if in == nil { + return nil + } + out := new(DataImportTargetRefSpec) + in.DeepCopyInto(out) + return out +} + // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new DataImportSpec. func (in *DataImportSpec) DeepCopy() *DataImportSpec { if in == nil { diff --git a/internal/data/dataimport/cmd/create/create.go b/internal/data/dataimport/cmd/create/create.go index 810d0bb4c..0377cdf14 100644 --- a/internal/data/dataimport/cmd/create/create.go +++ b/internal/data/dataimport/cmd/create/create.go @@ -89,11 +89,6 @@ func Run(ctx context.Context, log *slog.Logger, cmd *cobra.Command, args []strin return err } - rtClient, err := sc.NewRTClient(v1alpha1.AddToScheme) - if err != nil { - return err - } - data, err := os.ReadFile(pvcFilePath) if err != nil { return err @@ -112,6 +107,14 @@ func Run(ctx context.Context, log *slog.Logger, cmd *cobra.Command, args []strin namespace = pvcSpec.Namespace } + // Resolved only once the namespace is final: the producer is picked partly from what the user + // may do in that namespace, and asking about the empty namespace would ask about cluster-wide + // permission instead — which a user holding rights in one namespace does not have. + backend, rtClient, err := util.ResolveClientFunc(ctx, sc, namespace, log) + if err != nil { + return err + } + publishFlag, err := dataio.ParsePublishFlag(cmd.Flags()) if err != nil { return err @@ -122,7 +125,7 @@ func Run(ctx context.Context, log *slog.Logger, cmd *cobra.Command, args []strin return err } - if err := util.CreateDataImport(ctx, name, namespace, ttl, publish, wffc, pvcSpec, rtClient); err != nil { + if err := util.CreateDataImport(ctx, backend, name, namespace, ttl, publish, wffc, pvcSpec, rtClient); err != nil { return err } diff --git a/internal/data/dataimport/cmd/delete/delete.go b/internal/data/dataimport/cmd/delete/delete.go index 3b5396309..94d168a79 100644 --- a/internal/data/dataimport/cmd/delete/delete.go +++ b/internal/data/dataimport/cmd/delete/delete.go @@ -9,7 +9,6 @@ import ( "github.com/spf13/cobra" - "github.com/deckhouse/deckhouse-cli/internal/data/dataimport/api/v1alpha1" "github.com/deckhouse/deckhouse-cli/internal/data/dataimport/util" safeClient "github.com/deckhouse/deckhouse-cli/pkg/libsaferequest/client" ) @@ -71,7 +70,7 @@ func Run(ctx context.Context, log *slog.Logger, cmd *cobra.Command, args []strin return err } - rtClient, err := safeClient.NewRTClient(v1alpha1.AddToScheme) + _, rtClient, err := util.ResolveClientFunc(ctx, safeClient, namespace, log) if err != nil { return err } diff --git a/internal/data/dataimport/cmd/upload/upload.go b/internal/data/dataimport/cmd/upload/upload.go index 03d2fa1a0..42eb833ab 100644 --- a/internal/data/dataimport/cmd/upload/upload.go +++ b/internal/data/dataimport/cmd/upload/upload.go @@ -17,7 +17,6 @@ import ( "github.com/spf13/cobra" dataio "github.com/deckhouse/deckhouse-cli/internal/data" - v1alpha1 "github.com/deckhouse/deckhouse-cli/internal/data/dataimport/api/v1alpha1" "github.com/deckhouse/deckhouse-cli/internal/data/dataimport/util" client "github.com/deckhouse/deckhouse-cli/pkg/libsaferequest/client" ) @@ -86,8 +85,9 @@ func Run(ctx context.Context, log *slog.Logger, cmd *cobra.Command, args []strin log.Info("Run") - // Create runtime client for publish auto-detection and reconciliation. - rtClient, err := httpClient.NewRTClient(v1alpha1.AddToScheme) + // Resolve the producer that serves DataImport here, and build the runtime client bound to + // it for publish auto-detection and reconciliation. + backend, rtClient, err := util.ResolveClientFunc(ctx, httpClient, namespace, log) if err != nil { return err } @@ -116,7 +116,7 @@ func Run(ctx context.Context, log *slog.Logger, cmd *cobra.Command, args []strin } } - podURL, baseURL, _, subClient, err := util.PrepareUpload(ctx, diName, namespace, publish, httpClient, log) + podURL, baseURL, _, subClient, err := util.PrepareUpload(ctx, backend, diName, namespace, publish, httpClient, log) if err != nil { return err } diff --git a/internal/data/dataimport/cmd/upload/upload_windows.go b/internal/data/dataimport/cmd/upload/upload_windows.go index f496bd06e..641671a94 100644 --- a/internal/data/dataimport/cmd/upload/upload_windows.go +++ b/internal/data/dataimport/cmd/upload/upload_windows.go @@ -16,7 +16,6 @@ import ( "github.com/spf13/cobra" dataio "github.com/deckhouse/deckhouse-cli/internal/data" - v1alpha1 "github.com/deckhouse/deckhouse-cli/internal/data/dataimport/api/v1alpha1" "github.com/deckhouse/deckhouse-cli/internal/data/dataimport/util" client "github.com/deckhouse/deckhouse-cli/pkg/libsaferequest/client" ) @@ -92,8 +91,9 @@ func Run(ctx context.Context, log *slog.Logger, cmd *cobra.Command, args []strin } } - // Create runtime client for publish auto-detection and reconciliation. - rtClient, err := httpClient.NewRTClient(v1alpha1.AddToScheme) + // Resolve the producer that serves DataImport here, and build the runtime client bound to + // it for publish auto-detection and reconciliation. + backend, rtClient, err := util.ResolveClientFunc(ctx, httpClient, namespace, log) if err != nil { return err } @@ -108,7 +108,7 @@ func Run(ctx context.Context, log *slog.Logger, cmd *cobra.Command, args []strin return err } - podUrl, baseUrl, _, subClient, err := util.PrepareUpload(ctx, diName, namespace, publish, httpClient, log) + podUrl, baseUrl, _, subClient, err := util.PrepareUpload(ctx, backend, diName, namespace, publish, httpClient, log) if err != nil { return err } diff --git a/internal/data/dataimport/util/util.go b/internal/data/dataimport/util/util.go index 103e997cf..302bb6c3c 100644 --- a/internal/data/dataimport/util/util.go +++ b/internal/data/dataimport/util/util.go @@ -16,6 +16,7 @@ import ( ctrlrtclient "sigs.k8s.io/controller-runtime/pkg/client" dataio "github.com/deckhouse/deckhouse-cli/internal/data" + "github.com/deckhouse/deckhouse-cli/internal/data/dataapi" "github.com/deckhouse/deckhouse-cli/internal/data/dataimport/api/v1alpha1" safeClient "github.com/deckhouse/deckhouse-cli/pkg/libsaferequest/client" ) @@ -23,6 +24,10 @@ import ( // var instead of const to allow test override. var maxRetryAttempts = 60 +// ResolveClientFunc is a function pointer for test stubbing: resolution is the first thing a +// command does and the only step that contacts the API server before any of the work under test. +var ResolveClientFunc = ResolveClient + const ( retryInterval = 3 @@ -31,6 +36,34 @@ const ( uploadFinishedSubpath = "api/v1/finished" ) +// ResolveClient resolves which of the two producers serves DataImport to this user in this +// namespace, and returns both the answer and a control-plane client bound to it. +// +// Every `d8 data import` subcommand starts here rather than registering a fixed group, because +// the same binary ships to clusters that serve only storage-foundation's group, only +// storage-volume-data-manager's, or both with the user authorized for one of them. The resolved +// backend is returned alongside the client because the request body differs between the two +// producers (see CreateDataImport) and because callers that build their own client later +// (PrepareUpload) must bind it to the same group. +func ResolveClient( + ctx context.Context, + sClient *safeClient.SafeClient, + namespace string, + log *slog.Logger, +) (dataapi.Backend, ctrlrtclient.Client, error) { + backend, err := dataapi.Resolve(ctx, sClient.RESTConfig(), dataapi.ResourceDataImports, namespace, log) + if err != nil { + return dataapi.Backend{}, nil, err + } + + rtClient, err := sClient.NewRTClient(v1alpha1.AddToSchemeFor(backend.GroupVersion)) + if err != nil { + return dataapi.Backend{}, nil, err + } + + return backend, rtClient, nil +} + func GetDataImport(ctx context.Context, diName, namespace string, rtClient ctrlrtclient.Client) (*v1alpha1.DataImport, error) { diObj := &v1alpha1.DataImport{} @@ -39,16 +72,12 @@ func GetDataImport(ctx context.Context, diName, namespace string, rtClient ctrlr return nil, fmt.Errorf("kube Get dataimport: %s", err.Error()) } - for _, condition := range diObj.Status.Conditions { - if condition.Type == "Ready" { - if condition.Status != "True" { - return nil, fmt.Errorf("DataImport %s/%s is not Ready: %s (%s)", - diObj.ObjectMeta.Namespace, diObj.ObjectMeta.Name, - condition.Message, condition.Reason) - } - - break - } + // An object with no Ready condition at all has not been reconciled yet rather than failed, + // so only a Ready condition that is present and not True is an error here. + if notReady := dataio.NotReady(diObj.Status.Conditions); notReady != nil { + return nil, fmt.Errorf("DataImport %s/%s is not Ready: %s (%s)", + diObj.ObjectMeta.Namespace, diObj.ObjectMeta.Name, + notReady.Message, notReady.Reason) } return diObj, nil @@ -66,8 +95,16 @@ func DeleteDataImport(ctx context.Context, diName, namespace string, rtClient ct return err } +// CreateDataImport creates a DataImport that streams the uploaded bytes into a newly created PVC, +// in the request shape the resolved backend understands. +// +// backend selects that shape, and there is no shape that satisfies both producers: whichever one +// is addressed rejects a body written for the other. storage-foundation requires spec.mode plus a +// spec.pvcTemplate at the root and has no targetRef property; storage-volume-data-manager requires +// spec.targetRef carrying the same template and has no mode. func CreateDataImport( ctx context.Context, + backend dataapi.Backend, name, namespace, ttl string, publish, waitForFirstConsumer bool, pvcTpl *v1alpha1.PersistentVolumeClaimTemplateSpec, @@ -77,32 +114,40 @@ func CreateDataImport( ttl = dataio.DefaultTTL } - // CreatePVC requires a pvcTemplate whose metadata.name is set: the controller names the - // imported PVC after it, and the server CEL rejects an empty name. Fail early with a - // clear message instead of surfacing an opaque admission error. + // Both producers name the imported PVC after the template's metadata.name and reject an + // empty one. Fail early with a clear message instead of surfacing an opaque admission error. if pvcTpl == nil || pvcTpl.Name == "" { return fmt.Errorf("DataImport %s/%s requires a PVC template with metadata.name set", namespace, name) } + spec := v1alpha1.DataImportSpec{ + TTL: ttl, + Publish: publish, + WaitForFirstConsumer: waitForFirstConsumer, + } + + if backend.Legacy() { + spec.TargetRef = &v1alpha1.DataImportTargetRefSpec{ + Kind: v1alpha1.PersistentVolumeClaimKind, + PvcTemplate: pvcTpl, + } + } else { + // Mode is sent explicitly even though the CRD defaults it: mode is immutable after + // creation, so relying on the server-side default would silently bind the object to + // whatever default a future CRD revision ships. + spec.Mode = v1alpha1.DataImportModeCreatePVC + spec.PvcTemplate = pvcTpl + } + + // TypeMeta is left empty on purpose: the client stamps the apiVersion from the scheme it was + // built with, which is the group resolved for this run — a literal here would name one + // producer's group on every request, including requests to the other one. obj := &v1alpha1.DataImport{ - TypeMeta: metav1.TypeMeta{ - APIVersion: v1alpha1.SchemeGroupVersion.String(), - Kind: "DataImport", - }, ObjectMeta: metav1.ObjectMeta{ Name: name, Namespace: namespace, }, - Spec: v1alpha1.DataImportSpec{ - TTL: ttl, - Publish: publish, - WaitForFirstConsumer: waitForFirstConsumer, - // Sent explicitly even though the CRD defaults it: mode is immutable after creation, - // so relying on the server-side default would silently bind the object to whatever - // default a future CRD revision ships. - Mode: v1alpha1.DataImportModeCreatePVC, - PvcTemplate: pvcTpl, - }, + Spec: spec, } if err := rtClient.Create(ctx, obj); err != nil && !apierrors.IsAlreadyExists(err) { @@ -114,6 +159,7 @@ func CreateDataImport( func GetDataImportWithRestart( ctx context.Context, + backend dataapi.Backend, diName, namespace string, rtClient ctrlrtclient.Client, log *slog.Logger, @@ -130,49 +176,48 @@ func GetDataImportWithRestart( var notReadyErr error - for _, condition := range diObj.Status.Conditions { - // The DataImport controller no longer carries a standalone "Expired" condition; expiry is - // now the Ready condition with Status=False and Reason="Expired" (symmetric with - // DataExport). Detect it on the Ready condition and auto-restart the import, rather than - // waiting for the producer's GC (which only deletes an expired DataImport after its - // retention TTL). - if condition.Type != "Ready" { - continue + // An expired import is recreated here rather than waited out: after expiry the producer's + // garbage collector only removes the object once its retention TTL runs out, so polling + // would stall for the whole of that retention. Both producers' spellings of expiry are + // recognised by dataio.IsExpired. + switch { + case dataio.IsExpired(diObj.Status.Conditions): + if err := DeleteDataImport(ctx, diName, namespace, rtClient); err != nil { + return nil, err } - switch { - case condition.Status == "False" && condition.Reason == "Expired": - if err := DeleteDataImport(ctx, diName, namespace, rtClient); err != nil { - return nil, err - } - - pvcTemplate := &v1alpha1.PersistentVolumeClaimTemplateSpec{} - if diObj.Spec.PvcTemplate != nil { - pvcTemplate = diObj.Spec.PvcTemplate - } + // DestinationTemplate reads the template from whichever of the two spec shapes the + // producer that wrote this object uses; reading Spec.PvcTemplate directly would + // recreate a storage-volume-data-manager import with an empty template. + pvcTemplate := diObj.Spec.DestinationTemplate() + if pvcTemplate == nil { + pvcTemplate = &v1alpha1.PersistentVolumeClaimTemplateSpec{} + } - if err := CreateDataImport( - ctx, - diName, - namespace, - diObj.Spec.TTL, - diObj.Spec.Publish, - diObj.Spec.WaitForFirstConsumer, - pvcTemplate, - rtClient, - ); err != nil { - return nil, err - } - // Recreated: the stale object's status.url/status.volumeMode still belong to the - // dead importer until retention-GC reaps it, so we must not let this object fall - // through to the readiness checks below as if it were done. Keep retrying until the - // fresh import becomes Ready. - notReadyErr = fmt.Errorf("DataImport %s/%s expired; recreated, waiting for the new import to become Ready", - diObj.ObjectMeta.Namespace, diObj.ObjectMeta.Name) - case condition.Status != "True": + if err := CreateDataImport( + ctx, + backend, + diName, + namespace, + diObj.Spec.TTL, + diObj.Spec.Publish, + diObj.Spec.WaitForFirstConsumer, + pvcTemplate, + rtClient, + ); err != nil { + return nil, err + } + // Recreated: the stale object's status.url/status.volumeMode still belong to the + // dead importer until retention-GC reaps it, so we must not let this object fall + // through to the readiness checks below as if it were done. Keep retrying until the + // fresh import becomes Ready. + notReadyErr = fmt.Errorf("DataImport %s/%s expired; recreated, waiting for the new import to become Ready", + diObj.ObjectMeta.Namespace, diObj.ObjectMeta.Name) + default: + if notReady := dataio.NotReady(diObj.Status.Conditions); notReady != nil { notReadyErr = fmt.Errorf("DataImport %s/%s is not Ready: %s (%s)", diObj.ObjectMeta.Namespace, diObj.ObjectMeta.Name, - condition.Message, condition.Reason) + notReady.Message, notReady.Reason) } } @@ -209,8 +254,13 @@ func GetDataImportWithRestart( } } +// PrepareUpload waits for the import to be usable and returns the data-plane URL, the importer +// base URL, the volume mode and a client trusting the importer's CA. backend is the group +// resolved for this run; it builds this function's own control-plane client, which would +// otherwise default to storage-foundation's group regardless of what the cluster serves. func PrepareUpload( ctx context.Context, + backend dataapi.Backend, diName, namespace string, publish bool, sClient *safeClient.SafeClient, @@ -222,7 +272,7 @@ func PrepareUpload( decodedBytes []byte ) - rtClient, err := sClient.NewRTClient(v1alpha1.AddToScheme) + rtClient, err := sClient.NewRTClient(v1alpha1.AddToSchemeFor(backend.GroupVersion)) if err != nil { return "", "", "", nil, err } @@ -243,7 +293,7 @@ func PrepareUpload( return "", "", "", nil, err } - diObj, err = GetDataImportWithRestart(ctx, diName, namespace, rtClient, log) + diObj, err = GetDataImportWithRestart(ctx, backend, diName, namespace, rtClient, log) if err != nil { return "", "", "", nil, err } diff --git a/internal/data/dataimport/util/util_test.go b/internal/data/dataimport/util/util_test.go index 527a42b12..3148a9e54 100644 --- a/internal/data/dataimport/util/util_test.go +++ b/internal/data/dataimport/util/util_test.go @@ -32,10 +32,23 @@ import ( ctrlclient "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" + "github.com/deckhouse/deckhouse-cli/internal/data/dataapi" "github.com/deckhouse/deckhouse-cli/internal/data/dataimport/api/v1alpha1" safeClient "github.com/deckhouse/deckhouse-cli/pkg/libsaferequest/client" ) +// foundationBackend is the storage-foundation answer a resolution would return. Tests that do +// not exercise the older producer use it so the shape assertions stay about one producer at a time. +func foundationBackend() dataapi.Backend { + return dataapi.Backend{GroupVersion: dataapi.FoundationGroupVersion, Module: "storage-foundation"} +} + +// legacyBackend is the storage-volume-data-manager answer, used by the tests that pin that +// producer's request shape. +func legacyBackend() dataapi.Backend { + return dataapi.Backend{GroupVersion: dataapi.LegacyGroupVersion, Module: "storage-volume-data-manager"} +} + // readyCond builds a Ready condition with the given status/reason/message. func readyCond(status metav1.ConditionStatus, reason, message string) metav1.Condition { return metav1.Condition{ @@ -61,7 +74,7 @@ func TestCreateDataImport_BuildsCreatePVCSpec(t *testing.T) { ObjectMeta: metav1.ObjectMeta{Name: "restored-pvc"}, } - require.NoError(t, CreateDataImport(ctx, "import-into-pvc", "my-ns", "15m", false, true, pvcTpl, c)) + require.NoError(t, CreateDataImport(ctx, foundationBackend(), "import-into-pvc", "my-ns", "15m", false, true, pvcTpl, c)) var stored v1alpha1.DataImport require.NoError(t, c.Get(ctx, ctrlclient.ObjectKey{Name: "import-into-pvc", Namespace: "my-ns"}, &stored)) @@ -89,7 +102,7 @@ func TestCreateDataImport_BuildsCreatePVCSpec(t *testing.T) { // as an explicit key. If it is ever dropped as a zero value, the server flips it back to // true and `--wffc=false` becomes silently inoperative. cWithoutWFFC := fake.NewClientBuilder().WithScheme(scheme).Build() - require.NoError(t, CreateDataImport(ctx, "no-wffc", "my-ns", "15m", false, false, pvcTpl, cWithoutWFFC)) + require.NoError(t, CreateDataImport(ctx, foundationBackend(), "no-wffc", "my-ns", "15m", false, false, pvcTpl, cWithoutWFFC)) var withoutWFFC v1alpha1.DataImport require.NoError(t, cWithoutWFFC.Get(ctx, ctrlclient.ObjectKey{Name: "no-wffc", Namespace: "my-ns"}, &withoutWFFC)) @@ -101,6 +114,52 @@ func TestCreateDataImport_BuildsCreatePVCSpec(t *testing.T) { }) } +// TestCreateDataImport_BuildsLegacyTargetRefSpec pins the wire shape the CLI must produce for +// storage-volume-data-manager, which is not a subset or superset of the storage-foundation shape +// but a different one: its schema requires spec.targetRef and declares no spec.mode or root +// spec.pvcTemplate, so a body written for storage-foundation is rejected outright for a missing +// required field. +// +// The negative assertions carry as much weight as the positive ones. Sending mode alongside +// targetRef would be accepted today only because the structural schema prunes it, and would start +// meaning something the day either producer grows the other's key. +func TestCreateDataImport_BuildsLegacyTargetRefSpec(t *testing.T) { + scheme := runtime.NewScheme() + require.NoError(t, v1alpha1.AddToSchemeFor(dataapi.LegacyGroupVersion)(scheme)) + + ctx := context.Background() + c := fake.NewClientBuilder().WithScheme(scheme).Build() + + pvcTpl := &v1alpha1.PersistentVolumeClaimTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{Name: "restored-pvc"}, + } + + require.NoError(t, CreateDataImport(ctx, legacyBackend(), "import-into-pvc", "my-ns", "15m", false, true, pvcTpl, c)) + + var stored v1alpha1.DataImport + require.NoError(t, c.Get(ctx, ctrlclient.ObjectKey{Name: "import-into-pvc", Namespace: "my-ns"}, &stored)) + + require.NotNil(t, stored.Spec.TargetRef) + assert.Equal(t, v1alpha1.PersistentVolumeClaimKind, stored.Spec.TargetRef.Kind) + require.NotNil(t, stored.Spec.TargetRef.PvcTemplate) + assert.Equal(t, "restored-pvc", stored.Spec.TargetRef.PvcTemplate.Name) + assert.Equal(t, "15m", stored.Spec.TTL) + assert.True(t, stored.Spec.WaitForFirstConsumer) + + assert.Empty(t, stored.Spec.Mode, "mode belongs to the other producer's schema") + assert.Nil(t, stored.Spec.PvcTemplate, "the root pvcTemplate belongs to the other producer's schema") + + // Guard the serialised shape as well as the Go fields: a wrong json tag would keep the + // assertions above passing while changing what the apiserver receives. Same scope as the + // sibling storage-foundation test — this covers this package's json tags only, not + // server-side pruning or CEL. + raw, err := json.Marshal(stored.Spec) + require.NoError(t, err) + assert.Contains(t, string(raw), `"targetRef"`) + assert.Contains(t, string(raw), `"kind":"PersistentVolumeClaim"`) + assert.NotContains(t, string(raw), `"mode"`) +} + func TestCreateDataImport_RejectsTemplateWithoutName(t *testing.T) { scheme := runtime.NewScheme() require.NoError(t, v1alpha1.AddToScheme(scheme)) @@ -115,7 +174,7 @@ func TestCreateDataImport_RejectsTemplateWithoutName(t *testing.T) { for name, tpl := range cases { t.Run(name, func(t *testing.T) { - err := CreateDataImport(ctx, "di", "my-ns", "15m", false, false, tpl, c) + err := CreateDataImport(ctx, foundationBackend(), "di", "my-ns", "15m", false, false, tpl, c) require.Error(t, err) var stored v1alpha1.DataImport @@ -308,7 +367,7 @@ func TestGetDataImportWithRestart_ExpiredRecreates(t *testing.T) { c := fake.NewClientBuilder().WithScheme(scheme).WithObjects(expired).Build() - _, err := GetDataImportWithRestart(ctx, "test-di", "test-ns", c, logger) + _, err := GetDataImportWithRestart(ctx, foundationBackend(), "test-di", "test-ns", c, logger) require.Error(t, err) assert.Contains(t, err.Error(), "expired; recreated, waiting") @@ -403,7 +462,7 @@ func TestGetDataImportWithRestart_RecreatesFromServerEncodedSpec(t *testing.T) { c := fake.NewClientBuilder().WithScheme(scheme).WithObjects(expired).Build() - _, err := GetDataImportWithRestart(ctx, "test-di", "test-ns", c, logger) + _, err := GetDataImportWithRestart(ctx, foundationBackend(), "test-di", "test-ns", c, logger) require.Error(t, err) assert.Contains(t, err.Error(), "expired; recreated, waiting") @@ -426,14 +485,20 @@ func TestGetDataImportWithRestart_RecreatesFromServerEncodedSpec(t *testing.T) { assert.Contains(t, string(recreatedRaw), `"waitForFirstConsumer":false`) } -// TestGetDataImportWithRestart_LegacyExpiredConditionIsNotUsed asserts the contract change: a -// legacy standalone Type=="Expired" condition (the old, now version-skewed, detection signal) must -// not trigger a recreate anymore. Only the current controller's Ready=False/Reason=Expired signal -// does. +// TestGetDataImportWithRestart_StandaloneExpiredConditionRecreates covers the other producer's +// spelling of expiry: a standalone Type=="Expired" condition must trigger the same recreate as +// storage-foundation's Ready=False/Reason=Expired. +// +// The fixture pairs Expired=True with a still-True Ready deliberately, because that is the state a +// real storage-volume-data-manager cluster passes through rather than an invented one: its importer +// pod raises the standalone Expired condition, and only the next controller reconcile mirrors it +// onto Ready. A client that waited for the Ready spelling would keep polling a dead importer for +// however long that reconcile takes, and then for the producer's whole retention TTL if the mirror +// never lands. // // Deliberately NOT t.Parallel: it overrides the package-level maxRetryAttempts (see the note on // TestGetDataImportWithRestart_ExpiredRecreates). -func TestGetDataImportWithRestart_LegacyExpiredConditionIsNotUsed(t *testing.T) { +func TestGetDataImportWithRestart_StandaloneExpiredConditionRecreates(t *testing.T) { scheme := runtime.NewScheme() require.NoError(t, v1alpha1.AddToScheme(scheme)) @@ -446,11 +511,22 @@ func TestGetDataImportWithRestart_LegacyExpiredConditionIsNotUsed(t *testing.T) di := &v1alpha1.DataImport{ ObjectMeta: metav1.ObjectMeta{Name: "test-di", Namespace: "test-ns"}, + Spec: v1alpha1.DataImportSpec{ + TTL: "15m", + WaitForFirstConsumer: true, + // Written in the older producer's shape, as an object read back from it would be. + TargetRef: &v1alpha1.DataImportTargetRefSpec{ + Kind: v1alpha1.PersistentVolumeClaimKind, + PvcTemplate: &v1alpha1.PersistentVolumeClaimTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{Name: "restored-pvc"}, + }, + }, + }, Status: v1alpha1.DataExportImportStatus{ URL: "https://10.0.0.1:8085/", VolumeMode: "Filesystem", Conditions: []metav1.Condition{ - // Legacy signal: a standalone Expired condition, distinct from Ready. + // The standalone Expired condition, raised before Ready is mirrored. {Type: "Expired", Status: metav1.ConditionTrue}, readyCond(metav1.ConditionTrue, "PodReady", "Pod is ready and import completed"), }, @@ -459,14 +535,26 @@ func TestGetDataImportWithRestart_LegacyExpiredConditionIsNotUsed(t *testing.T) c := fake.NewClientBuilder().WithScheme(scheme).WithObjects(di).Build() - got, err := GetDataImportWithRestart(ctx, "test-di", "test-ns", c, logger) - require.NoError(t, err) - assert.Equal(t, "https://10.0.0.1:8085/", got.Status.URL) + _, err := GetDataImportWithRestart(ctx, legacyBackend(), "test-di", "test-ns", c, logger) + require.Error(t, err) + assert.Contains(t, err.Error(), "expired; recreated, waiting") - // Confirm no delete+recreate happened: the object in the store is still the original. - var stored v1alpha1.DataImport - require.NoError(t, c.Get(ctx, ctrlclient.ObjectKey{Name: "test-di", Namespace: "test-ns"}, &stored)) - assert.Len(t, stored.Status.Conditions, 2, "legacy Expired condition must not trigger a recreate") + var recreated v1alpha1.DataImport + require.NoError(t, c.Get(ctx, ctrlclient.ObjectKey{Name: "test-di", Namespace: "test-ns"}, &recreated)) + assert.Empty(t, recreated.Status.Conditions, "the stale status must be gone after the recreate") + + // The template has to survive a round trip through the older producer's nesting: reading + // Spec.PvcTemplate directly would find nothing here and abort the recreate with "requires a + // PVC template with metadata.name set". + require.NotNil(t, recreated.Spec.TargetRef, "the recreate must keep addressing the older producer") + require.NotNil(t, recreated.Spec.TargetRef.PvcTemplate) + assert.Equal(t, "restored-pvc", recreated.Spec.TargetRef.PvcTemplate.Name) + assert.Empty(t, recreated.Spec.Mode, "mode must not be sent to a producer whose schema has no such property") + + recreatedRaw, marshalErr := json.Marshal(recreated.Spec) + require.NoError(t, marshalErr) + assert.Contains(t, string(recreatedRaw), `"targetRef"`) + assert.NotContains(t, string(recreatedRaw), `"mode"`) } // TestGetDataImportWithRestart_CompletedIsNotTreatedAsExpired guards against too broad a predicate: @@ -496,7 +584,7 @@ func TestGetDataImportWithRestart_CompletedIsNotTreatedAsExpired(t *testing.T) { c := fake.NewClientBuilder().WithScheme(scheme).WithObjects(di).Build() - _, err := GetDataImportWithRestart(ctx, "test-di", "test-ns", c, logger) + _, err := GetDataImportWithRestart(ctx, foundationBackend(), "test-di", "test-ns", c, logger) require.Error(t, err) assert.Contains(t, err.Error(), "not Ready") assert.Contains(t, err.Error(), "(Completed)") diff --git a/pkg/libsaferequest/client/http.go b/pkg/libsaferequest/client/http.go index ef967a83c..be2bcfac9 100644 --- a/pkg/libsaferequest/client/http.go +++ b/pkg/libsaferequest/client/http.go @@ -206,3 +206,16 @@ func (c *SafeClient) SetTLSCAData(caData []byte) { func (c *SafeClient) Copy() *SafeClient { return &SafeClient{rest.CopyConfig(c.restConfig)} } + +// RESTConfig returns a copy of the client's Kubernetes configuration, for callers that need a +// client-go client this type does not build itself (discovery, SelfSubjectAccessReview). A copy +// rather than the original: the caller's client must not be able to retarget or re-authenticate +// every other client derived from this one, the way SetProbeEndpoint deliberately does. +// Returns nil when the client holds no configuration. +func (c *SafeClient) RESTConfig() *rest.Config { + if c == nil || c.restConfig == nil { + return nil + } + + return rest.CopyConfig(c.restConfig) +}