diff --git a/api/naming/naming.go b/api/naming/naming.go new file mode 100644 index 00000000..b6c82865 --- /dev/null +++ b/api/naming/naming.go @@ -0,0 +1,56 @@ +/* +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 naming + +import ( + "crypto/sha256" + "fmt" + "strings" +) + +// HelmClusterAddonChartName derives the name of the HelmClusterAddonChart object +// that mirrors one chart of a repository. It lives in the api module because +// operator-helm-controller writes those objects while chart-values-controller reads +// them: the name is a truncated hash, so both must derive it identically. +func HelmClusterAddonChartName(repoName, chartName string) string { + hash := hash(fmt.Sprintf("%s-chart-%s", repoName, chartName)) + + var result, postfix string + + if len(repoName) > 20 { + result += repoName[:20] + "-chart-" + postfix = "-" + hash + } else { + result += repoName + "-chart-" + } + + if len(chartName) > 20 { + result += chartName[:20] + postfix = "-" + hash + } else { + result += chartName + } + + return strings.TrimRight(result, "-") + postfix +} + +func hash(s string) string { + h := sha256.New() + h.Write([]byte(s)) + + return fmt.Sprintf("%x", h.Sum(nil))[:12] +} diff --git a/api/naming/naming_test.go b/api/naming/naming_test.go new file mode 100644 index 00000000..2b5f8df9 --- /dev/null +++ b/api/naming/naming_test.go @@ -0,0 +1,55 @@ +/* +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 naming + +import "testing" + +func TestHelmClusterAddonChartName(t *testing.T) { + cases := []struct { + name string + repo string + chart string + want string + }{ + { + name: "short names are joined verbatim", + repo: "example", + chart: "podinfo", + want: "example-chart-podinfo", + }, + { + name: "long names are truncated and suffixed with a hash", + repo: "yandex-cloud-marketplace-mirror", + chart: "cert-manager-webhook-yandex", + want: "yandex-cloud-marketp-chart-cert-manager-webhook-a3ee4a8a584e", + }, + { + name: "an empty chart name leaves no trailing dash", + repo: "repo", + chart: "", + want: "repo-chart", + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := HelmClusterAddonChartName(tc.repo, tc.chart); got != tc.want { + t.Fatalf("HelmClusterAddonChartName(%q, %q) = %q, want %q", tc.repo, tc.chart, got, tc.want) + } + }) + } +} diff --git a/api/v1alpha1/conditions.go b/api/v1alpha1/conditions.go index efaffe7d..d19e002c 100644 --- a/api/v1alpha1/conditions.go +++ b/api/v1alpha1/conditions.go @@ -51,6 +51,12 @@ const ( ReasonSourceRejectedRequest = "SourceRejectedRequest" ReasonInvalidRepositoryURL = "InvalidRepositoryURL" ReasonUnsupportedRepositoryType = "UnsupportedRepositoryType" + // ReasonChartVersionRemoved marks an addon whose chart version is still recorded in + // the catalog but is no longer offered by the repository. + ReasonChartVersionRemoved = "ChartVersionRemoved" + // ReasonPartialSync marks a repository whose first catalog read left some chart + // versions unresolved. + ReasonPartialSync = "PartialSync" // HelmRelease error reasons ReasonReleaseFailed = "ReleaseFailed" diff --git a/api/v1alpha1/helm_cluster_addon_chart.go b/api/v1alpha1/helm_cluster_addon_chart.go index dcfc94b7..d93dea1a 100644 --- a/api/v1alpha1/helm_cluster_addon_chart.go +++ b/api/v1alpha1/helm_cluster_addon_chart.go @@ -25,6 +25,22 @@ const ( HelmClusterAddonChartResource = "helmclusteraddoncharts" HelmClusterAddonChartLabelSourceName = "helm.deckhouse.io/cluster-addon-chart" + + // UnavailableReason* are the values of HelmClusterAddonChartVersion.UnavailableReason. + // They are field values rather than condition reasons, so they live next to the + // type that carries them instead of conditions.go. + // + // UnavailableReasonRemovedFromRepository means the tag is no longer offered by the + // repository. The entry is retained only because an addon still references it, and + // the marker is dropped automatically once the tag is listed again. + UnavailableReasonRemovedFromRepository = "RemovedFromRepository" + // UnavailableReasonUnsupportedMediaType means the manifest was read but the artifact + // is not a packaged Helm chart. It is a verdict about the artifact, so it is kept + // until a force reconcile re-examines every tag. + UnavailableReasonUnsupportedMediaType = "UnsupportedMediaType" + // UnavailableReasonResolvePending means the manifest request failed and no verdict + // was reached. Such a tag is re-examined on every normal synchronization. + UnavailableReasonResolvePending = "ResolvePending" ) // HelmClusterAddonChart represents a specific Helm chart discovered within a HelmClusterAddonRepository. These resources are automatically managed during repository synchronization and are immutable to user modifications. @@ -71,7 +87,9 @@ type HelmClusterAddonChartStatus struct { Conditions []metav1.Condition `json:"conditions,omitempty"` // Generation represents resource generation that was last processed by the controller. ObservedGeneration int64 `json:"observedGeneration,omitempty"` - // Available helm chart versions + // Versions lists every chart version the controller has examined. A version is + // usable when it has no unavailableReason; for an OCI repository a usable version + // also carries the media type of the layer that holds it. // +optional Versions []HelmClusterAddonChartVersion `json:"versions"` } @@ -80,6 +98,19 @@ type HelmClusterAddonChartVersion struct { // Helm chart version // +kubebuilder:validation:MinLength=1 Version string `json:"version"` + // MediaType is the OCI media type of the layer that holds this chart version. It is + // set only for versions from an OCI repository, and only when the layer is supported: + // an empty value means the version cannot be deployed. + // +optional + MediaType string `json:"mediaType,omitempty"` + // UnavailableReason explains why this version cannot be deployed. Its absence means + // the version is usable. + // +optional + // +kubebuilder:validation:Enum=RemovedFromRepository;UnsupportedMediaType;ResolvePending + UnavailableReason string `json:"unavailableReason,omitempty"` + // UnavailableMessage carries human readable detail for UnavailableReason. + // +optional + UnavailableMessage string `json:"unavailableMessage,omitempty"` } // HelmClusterAddonChartList contains a list of HelmClusterAddonCharts. diff --git a/crds/doc-ru-helmclusteraddoncharts.yaml b/crds/doc-ru-helmclusteraddoncharts.yaml index 4788076b..5ce3804f 100644 --- a/crds/doc-ru-helmclusteraddoncharts.yaml +++ b/crds/doc-ru-helmclusteraddoncharts.yaml @@ -19,8 +19,14 @@ spec: observedGeneration: description: Поколение ресурса, обработанное контроллером последним. versions: - description: Доступные версии Helm-чарта. + description: Список всех версий Helm-чарта, изученных контроллером. Версия пригодна к использованию, если у неё нет unavailableReason; для OCI-репозитория у пригодной версии также заполнен media type слоя, который её содержит. items: properties: version: description: Версия Helm-чарта. + mediaType: + description: "OCI media type слоя, содержащего эту версию чарта. Заполняется только для версий из OCI-репозитория и только когда слой поддерживается: пустое значение означает, что версию нельзя задеплоить." + unavailableReason: + description: Причина, по которой версию нельзя задеплоить. Отсутствие поля означает, что версия пригодна. + unavailableMessage: + description: Человекочитаемые подробности к unavailableReason. diff --git a/crds/helmclusteraddoncharts.yaml b/crds/helmclusteraddoncharts.yaml index 3ef471bf..087e15c9 100644 --- a/crds/helmclusteraddoncharts.yaml +++ b/crds/helmclusteraddoncharts.yaml @@ -111,9 +111,31 @@ spec: format: int64 type: integer versions: - description: Available helm chart versions + description: |- + Versions lists every chart version the controller has examined. A version is + usable when it has no unavailableReason; for an OCI repository a usable version + also carries the media type of the layer that holds it. items: properties: + mediaType: + description: |- + MediaType is the OCI media type of the layer that holds this chart version. It is + set only for versions from an OCI repository, and only when the layer is supported: + an empty value means the version cannot be deployed. + type: string + unavailableMessage: + description: UnavailableMessage carries human readable detail + for UnavailableReason. + type: string + unavailableReason: + description: |- + UnavailableReason explains why this version cannot be deployed. Its absence means + the version is usable. + enum: + - RemovedFromRepository + - UnsupportedMediaType + - ResolvePending + type: string version: description: Helm chart version minLength: 1 diff --git a/images/chart-values-controller/internal/resolver/resolver.go b/images/chart-values-controller/internal/resolver/resolver.go index a78fe896..c7e947e1 100644 --- a/images/chart-values-controller/internal/resolver/resolver.go +++ b/images/chart-values-controller/internal/resolver/resolver.go @@ -39,13 +39,10 @@ import ( "github.com/deckhouse/chart-values-controller/internal/cache" "github.com/deckhouse/chart-values-controller/internal/labels" "github.com/deckhouse/chart-values-controller/internal/naming" + apinaming "github.com/deckhouse/operator-helm/api/naming" helmv1alpha1 "github.com/deckhouse/operator-helm/api/v1alpha1" ) -// helmChartLayerMediaType is the OCI media type of the layer that holds a -// packaged Helm chart. -const helmChartLayerMediaType = "application/vnd.cncf.helm.chart.content.v1.tar+gzip" - // RepositoryKind identifies the kind of repository a chart lives in. New // repository kinds are added as new constants plus a case in Resolve. type RepositoryKind string @@ -126,6 +123,64 @@ func (r *Resolver) Resolve(ctx context.Context, req Request) (Result, error) { } } +// chartVersionMediaType reads the OCI layer media type recorded for the requested +// version by operator-helm-controller. That status is the single source of truth: +// resolving the media type here would duplicate the logic and spend registry requests +// on an answer that is already in the cluster. +// +// A non-nil Result means the caller must stop and return it. +func (r *Resolver) chartVersionMediaType(ctx context.Context, req Request) (string, *Result, error) { + chart := &helmv1alpha1.HelmClusterAddonChart{} + key := types.NamespacedName{Name: apinaming.HelmClusterAddonChartName(req.RepositoryName, req.Chart)} + + if err := r.client.Get(ctx, key, chart); err != nil { + if apierrors.IsNotFound(err) { + // The chart object is created by operator-helm-controller when it synchronizes + // the repository: until then the catalog simply has not caught up. + return "", &Result{Outcome: OutcomePending}, nil + } + + return "", nil, fmt.Errorf("getting chart: %w", err) + } + + for _, version := range chart.Status.Versions { + if version.Version != req.Version { + continue + } + + if version.MediaType == "" { + if version.UnavailableReason == helmv1alpha1.UnavailableReasonResolvePending || version.UnavailableReason == "" { + // Both an explicit ResolvePending and an empty reason mean the catalog has + // not reached a verdict yet, so the caller should retry rather than being + // told the version is permanently unreadable. An empty reason alongside an + // empty media type is the pre-upgrade shape of a version entry (written + // before this controller recorded verdicts at all): the client's + // KnownVersions treats it as never examined and re-resolves it on the very + // next normal synchronization, exactly like ResolvePending. + return "", &Result{Outcome: OutcomePending}, nil + } + + // Every other reason is a durable verdict that will not change without a + // change in the repository (e.g. an unsupported media type, or a removed tag + // with no media type on record), so it is reported as values-not-found, + // naming why. + detail := version.UnavailableReason + if version.UnavailableMessage != "" { + detail += ": " + version.UnavailableMessage + } + + return "", &Result{ + Outcome: OutcomeValuesNotFound, + Message: fmt.Sprintf("chart version %s is not readable (%s)", req.Version, detail), + }, nil + } + + return version.MediaType, nil, nil + } + + return "", &Result{Outcome: OutcomePending}, nil +} + // resolveHelmClusterAddon ensures the auxiliary source resource for a chart from // a HelmClusterAddonRepository exists, inspects its status and returns the // chart's values.yaml once the artifact is ready. @@ -154,7 +209,15 @@ func (r *Resolver) resolveHelmClusterAddon(ctx context.Context, req Request) (Re switch { case isOCI(repo.Spec.URL): - ociRepo, err := r.ensureOCIRepository(ctx, repo, req, name, expiresAt) + mediaType, done, err := r.chartVersionMediaType(ctx, req) + if err != nil { + return Result{}, err + } + if done != nil { + return *done, nil + } + + ociRepo, err := r.ensureOCIRepository(ctx, repo, req, name, expiresAt, mediaType) if err != nil { return Result{}, err } @@ -234,7 +297,7 @@ func (r *Resolver) ensureHelmChart(ctx context.Context, repo *helmv1alpha1.HelmC return chart, false, nil } -func (r *Resolver) ensureOCIRepository(ctx context.Context, repo *helmv1alpha1.HelmClusterAddonRepository, req Request, name, expiresAt string) (*sourcev1.OCIRepository, error) { +func (r *Resolver) ensureOCIRepository(ctx context.Context, repo *helmv1alpha1.HelmClusterAddonRepository, req Request, name, expiresAt, mediaType string) (*sourcev1.OCIRepository, error) { authSecret, tlsSecret, err := r.findRepositorySecretNames(ctx, repo.Name) if err != nil { return nil, err @@ -254,7 +317,7 @@ func (r *Resolver) ensureOCIRepository(ctx context.Context, repo *helmv1alpha1.H ociRepo.Spec.Interval = r.sourceInterval ociRepo.Spec.Insecure = repo.Spec.InsecureSkipVerify ociRepo.Spec.LayerSelector = &sourcev1.OCILayerSelector{ - MediaType: helmChartLayerMediaType, + MediaType: mediaType, Operation: "copy", } diff --git a/images/chart-values-controller/internal/resolver/resolver_test.go b/images/chart-values-controller/internal/resolver/resolver_test.go new file mode 100644 index 00000000..533f0aaa --- /dev/null +++ b/images/chart-values-controller/internal/resolver/resolver_test.go @@ -0,0 +1,179 @@ +/* +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 resolver + +import ( + "context" + "strings" + "testing" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + clientgoscheme "k8s.io/client-go/kubernetes/scheme" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + + "github.com/deckhouse/operator-helm/api/naming" + helmv1alpha1 "github.com/deckhouse/operator-helm/api/v1alpha1" +) + +func newTestResolver(t *testing.T, objects ...client.Object) *Resolver { + t.Helper() + + scheme := runtime.NewScheme() + if err := clientgoscheme.AddToScheme(scheme); err != nil { + t.Fatalf("registering client-go scheme: %v", err) + } + if err := helmv1alpha1.AddToScheme(scheme); err != nil { + t.Fatalf("registering helm scheme: %v", err) + } + + c := fake.NewClientBuilder().WithScheme(scheme).WithObjects(objects...).Build() + + return &Resolver{client: c} +} + +func chartWithVersions(repoName, chartName string, versions ...helmv1alpha1.HelmClusterAddonChartVersion) *helmv1alpha1.HelmClusterAddonChart { + return &helmv1alpha1.HelmClusterAddonChart{ + ObjectMeta: metav1.ObjectMeta{Name: naming.HelmClusterAddonChartName(repoName, chartName)}, + Status: helmv1alpha1.HelmClusterAddonChartStatus{Versions: versions}, + } +} + +func TestChartVersionMediaType(t *testing.T) { + req := Request{Kind: RepositoryKindHelmClusterAddon, RepositoryName: "example", Chart: "podinfo", Version: "6.7.1"} + + t.Run("a usable version returns its media type", func(t *testing.T) { + resolver := newTestResolver(t, chartWithVersions("example", "podinfo", + helmv1alpha1.HelmClusterAddonChartVersion{Version: "6.7.1", MediaType: "application/tar+gzip"}, + )) + + mediaType, done, err := resolver.chartVersionMediaType(context.Background(), req) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if done != nil { + t.Fatalf("expected to continue, got outcome %q", done.Outcome) + } + if mediaType != "application/tar+gzip" { + t.Fatalf("media type is %q", mediaType) + } + }) + + t.Run("a missing chart is pending", func(t *testing.T) { + resolver := newTestResolver(t) + + _, done, err := resolver.chartVersionMediaType(context.Background(), req) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if done == nil || done.Outcome != OutcomePending { + t.Fatalf("outcome is %+v, want pending", done) + } + }) + + t.Run("a missing version is pending", func(t *testing.T) { + resolver := newTestResolver(t, chartWithVersions("example", "podinfo", + helmv1alpha1.HelmClusterAddonChartVersion{Version: "6.7.0", MediaType: "application/tar+gzip"}, + )) + + _, done, err := resolver.chartVersionMediaType(context.Background(), req) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if done == nil || done.Outcome != OutcomePending { + t.Fatalf("outcome is %+v, want pending", done) + } + }) + + t.Run("an unusable version is values_not_found with the reason", func(t *testing.T) { + resolver := newTestResolver(t, chartWithVersions("example", "podinfo", + helmv1alpha1.HelmClusterAddonChartVersion{ + Version: "6.7.1", + UnavailableReason: helmv1alpha1.UnavailableReasonUnsupportedMediaType, + UnavailableMessage: "config media type \"application/vnd.unknown.config.v1+json\" is not a helm chart config", + }, + )) + + _, done, err := resolver.chartVersionMediaType(context.Background(), req) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if done == nil || done.Outcome != OutcomeValuesNotFound { + t.Fatalf("outcome is %+v, want values_not_found", done) + } + if !strings.Contains(done.Message, helmv1alpha1.UnavailableReasonUnsupportedMediaType) { + t.Fatalf("message %q must name the reason", done.Message) + } + }) + + t.Run("a resolve-pending version is pending, not values_not_found", func(t *testing.T) { + resolver := newTestResolver(t, chartWithVersions("example", "podinfo", + helmv1alpha1.HelmClusterAddonChartVersion{ + Version: "6.7.1", + UnavailableReason: helmv1alpha1.UnavailableReasonResolvePending, + }, + )) + + _, done, err := resolver.chartVersionMediaType(context.Background(), req) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if done == nil || done.Outcome != OutcomePending { + t.Fatalf("outcome is %+v, want pending: a resolve-pending verdict is self-healing and must be retried, not reported as a permanent failure", done) + } + }) + + t.Run("a pre-upgrade version with no verdict at all is pending, not values_not_found", func(t *testing.T) { + resolver := newTestResolver(t, chartWithVersions("example", "podinfo", + // Neither MediaType nor UnavailableReason set: the shape a version entry had + // before this controller started recording verdicts. The migration path + // (client.KnownVersions) treats this exactly like ResolvePending and + // re-resolves it on the next normal synchronization. + helmv1alpha1.HelmClusterAddonChartVersion{Version: "6.7.1"}, + )) + + _, done, err := resolver.chartVersionMediaType(context.Background(), req) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if done == nil || done.Outcome != OutcomePending { + t.Fatalf("outcome is %+v, want pending: an empty verdict is the pre-upgrade migration state and must be retried, not reported as a permanent failure", done) + } + }) + + t.Run("a removed version keeps its media type usable", func(t *testing.T) { + resolver := newTestResolver(t, chartWithVersions("example", "podinfo", + helmv1alpha1.HelmClusterAddonChartVersion{ + Version: "6.7.1", + MediaType: "application/tar+gzip", + UnavailableReason: helmv1alpha1.UnavailableReasonRemovedFromRepository, + }, + )) + + mediaType, done, err := resolver.chartVersionMediaType(context.Background(), req) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if done != nil { + t.Fatalf("expected to continue, got outcome %q", done.Outcome) + } + if mediaType != "application/tar+gzip" { + t.Fatalf("media type is %q", mediaType) + } + }) +} diff --git a/images/operator-helm-controller/cmd/operator-helm-controller/main.go b/images/operator-helm-controller/cmd/operator-helm-controller/main.go index ee75a9ab..fdf9f8d0 100644 --- a/images/operator-helm-controller/cmd/operator-helm-controller/main.go +++ b/images/operator-helm-controller/cmd/operator-helm-controller/main.go @@ -33,7 +33,7 @@ import ( helmv1alpha1 "github.com/deckhouse/operator-helm/api/v1alpha1" "github.com/deckhouse/operator-helm/internal/controller/helmclusteraddon" "github.com/deckhouse/operator-helm/internal/controller/helmclusteraddonrepository" - "github.com/deckhouse/operator-helm/internal/utils" + "github.com/deckhouse/operator-helm/internal/index" helmclusteraddonwebhook "github.com/deckhouse/operator-helm/internal/webhook/helmclusteraddon" ) @@ -77,8 +77,8 @@ func main() { os.Exit(1) } - if err := utils.SetupAddonRepositoryIndex(mgr); err != nil { - logger.Error(err, "unable to setup addon repository index") + if err := index.SetupAddonRepository(mgr); err != nil { + logger.Error(err, "unable to setup indexes", "index", index.AddonRepository) os.Exit(1) } @@ -92,8 +92,8 @@ func main() { os.Exit(1) } - if err = helmclusteraddonwebhook.SetupIndexes(mgr); err != nil { - logger.Error(err, "unable to setup indexes", "webhook", "HelmClusterAddon") + if err = index.SetupAddonChart(mgr); err != nil { + logger.Error(err, "unable to setup indexes", "index", index.AddonChart) os.Exit(1) } diff --git a/images/operator-helm-controller/go.mod b/images/operator-helm-controller/go.mod index 0a2e57d6..4db359c9 100644 --- a/images/operator-helm-controller/go.mod +++ b/images/operator-helm-controller/go.mod @@ -15,6 +15,7 @@ require ( github.com/werf/3p-helm-controller/api v0.1.5 github.com/werf/nelm-source-controller/api v0.1.5 go.yaml.in/yaml/v3 v3.0.4 + golang.org/x/sync v0.19.0 helm.sh/helm/v3 v3.20.2 k8s.io/api v0.35.1 k8s.io/apimachinery v0.35.1 @@ -75,7 +76,6 @@ require ( go.yaml.in/yaml/v2 v2.4.3 // indirect golang.org/x/net v0.48.0 // indirect golang.org/x/oauth2 v0.30.0 // indirect - golang.org/x/sync v0.19.0 // indirect golang.org/x/sys v0.40.0 // indirect golang.org/x/term v0.39.0 // indirect golang.org/x/text v0.33.0 // indirect diff --git a/images/operator-helm-controller/internal/client/repository/client.go b/images/operator-helm-controller/internal/client/repository/client.go index 69cd889c..8d537ca7 100644 --- a/images/operator-helm-controller/internal/client/repository/client.go +++ b/images/operator-helm-controller/internal/client/repository/client.go @@ -24,6 +24,8 @@ import ( "net/http" "github.com/Masterminds/semver/v3" + + helmv1alpha1 "github.com/deckhouse/operator-helm/api/v1alpha1" "github.com/deckhouse/operator-helm/internal/utils" ) @@ -35,10 +37,58 @@ type Chart struct { type ChartVersion struct { Version *semver.Version IconURL string + + // MediaType is the OCI media type of the layer holding this chart version. It is + // empty for helm repositories and for OCI versions that are not usable. + MediaType string + // UnavailableReason and UnavailableMessage are set when the version cannot be + // deployed and are empty for a usable one. + UnavailableReason string + UnavailableMessage string +} + +// KnownVersion is the verdict a previous pass reached for one tag. +type KnownVersion struct { + MediaType string + UnavailableReason string + UnavailableMessage string +} + +// KnownVersions maps a tag to its recorded verdict. +type KnownVersions map[string]KnownVersion + +// KnownCharts maps a chart name to the tags already examined for it. +type KnownCharts map[string]KnownVersions + +// FetchOptions carries the incremental state of the previous pass, so a client can +// skip the tags whose verdict is already recorded. +type FetchOptions struct { + Known KnownCharts + Full bool +} + +// NeedsExamination reports whether a listed tag has to be examined again. A recorded +// media type is authoritative; an unsupported artifact is a verdict and is not +// re-examined until a full pass; anything else — never seen, seen without a verdict +// (an entry written before media types were recorded), or left pending — is examined. +func (o FetchOptions) NeedsExamination(chartName, tag string) bool { + if o.Full { + return true + } + + known, recorded := o.Known[chartName][tag] + switch { + case !recorded: + return true + case known.MediaType != "": + return false + default: + return known.UnavailableReason != helmv1alpha1.UnavailableReasonUnsupportedMediaType + } } type ClientInterface interface { - FetchCharts(ctx context.Context, url string, config *RepoConfig) ([]Chart, error) + FetchCharts(ctx context.Context, url string, config *RepoConfig, opts FetchOptions) ([]Chart, error) } func NewClient(repoType utils.InternalRepositoryType) (ClientInterface, error) { diff --git a/images/operator-helm-controller/internal/client/repository/helm.go b/images/operator-helm-controller/internal/client/repository/helm.go index 685fa7e2..9740d875 100644 --- a/images/operator-helm-controller/internal/client/repository/helm.go +++ b/images/operator-helm-controller/internal/client/repository/helm.go @@ -47,7 +47,10 @@ type HelmRepositoryChartVersion struct { Removed bool `json:"removed,omitempty"` } -func (c *helmRepositoryClient) FetchCharts(ctx context.Context, url string, config *RepoConfig) ([]Chart, error) { +// FetchCharts reads the repository index. FetchOptions is ignored: a helm index is a +// single document that already carries every version, so there is nothing to resolve +// incrementally. +func (c *helmRepositoryClient) FetchCharts(ctx context.Context, url string, config *RepoConfig, _ FetchOptions) ([]Chart, error) { if !strings.HasSuffix(url, "/index.yaml") { url += "/index.yaml" } diff --git a/images/operator-helm-controller/internal/client/repository/helm_test.go b/images/operator-helm-controller/internal/client/repository/helm_test.go index f52d16e5..19f2a9e7 100644 --- a/images/operator-helm-controller/internal/client/repository/helm_test.go +++ b/images/operator-helm-controller/internal/client/repository/helm_test.go @@ -54,7 +54,7 @@ func TestFetchChartsTerminalStatusCodes(t *testing.T) { })) defer srv.Close() - _, err := HelmRepositoryDefaultClient.FetchCharts(context.Background(), srv.URL, nil) + _, err := HelmRepositoryDefaultClient.FetchCharts(context.Background(), srv.URL, nil, FetchOptions{}) if err == nil { t.Fatalf("expected error for status %d", tc.statusCode) } @@ -76,7 +76,7 @@ func TestFetchChartsServerErrorIsNotTerminal(t *testing.T) { })) defer srv.Close() - _, err := HelmRepositoryDefaultClient.FetchCharts(context.Background(), srv.URL, nil) + _, err := HelmRepositoryDefaultClient.FetchCharts(context.Background(), srv.URL, nil, FetchOptions{}) if err == nil { t.Fatal("expected error for repeated 500 responses") } @@ -96,7 +96,7 @@ func TestFetchChartsSkipsInvalidVersion(t *testing.T) { })) defer srv.Close() - charts, err := HelmRepositoryDefaultClient.FetchCharts(context.Background(), srv.URL, nil) + charts, err := HelmRepositoryDefaultClient.FetchCharts(context.Background(), srv.URL, nil, FetchOptions{}) if err != nil { t.Fatalf("expected invalid version to be skipped, got error: %v", err) } diff --git a/images/operator-helm-controller/internal/client/repository/oci.go b/images/operator-helm-controller/internal/client/repository/oci.go index e20e4e3d..7ad6140d 100644 --- a/images/operator-helm-controller/internal/client/repository/oci.go +++ b/images/operator-helm-controller/internal/client/repository/oci.go @@ -17,17 +17,22 @@ limitations under the License. package repository import ( + "bytes" "context" "errors" "fmt" + "net/http" "strings" "time" + "unicode/utf8" "github.com/Masterminds/semver/v3" "github.com/google/go-containerregistry/pkg/authn" "github.com/google/go-containerregistry/pkg/name" + v1 "github.com/google/go-containerregistry/pkg/v1" "github.com/google/go-containerregistry/pkg/v1/remote" "github.com/google/go-containerregistry/pkg/v1/remote/transport" + "golang.org/x/sync/errgroup" helmv1alpha1 "github.com/deckhouse/operator-helm/api/v1alpha1" ) @@ -36,7 +41,15 @@ var OCIRepositoryDefaultClient ClientInterface = &ociRepositoryClient{} type ociRepositoryClient struct{} -func (c *ociRepositoryClient) FetchCharts(ctx context.Context, url string, config *RepoConfig) ([]Chart, error) { +// chartResolveConcurrency bounds the manifest requests of one pass. The requests are +// small, but a repository with hundreds of new tags would otherwise open hundreds of +// connections at once. +const chartResolveConcurrency = 8 + +// unavailableMessageLimit keeps a registry error from bloating the chart status. +const unavailableMessageLimit = 256 + +func (c *ociRepositoryClient) FetchCharts(ctx context.Context, url string, config *RepoConfig, opts FetchOptions) ([]Chart, error) { url = trimSchemaPrefixes(url) url = strings.TrimSuffix(url, "/") @@ -93,7 +106,48 @@ func (c *ociRepositoryClient) FetchCharts(ctx context.Context, url string, confi return nil, classifyRemoteError(err, url) } - var chartVersions []ChartVersion + versions, err := resolveChartVersions(ctx, repo, chartName, tags, options, opts) + if err != nil { + return nil, err + } + + return []Chart{ + { + Name: chartName, + Versions: versions, + }, + }, nil +} + +// resolveChartVersions turns the listed tags into one version entry per tag. A tag +// whose verdict is already recorded is carried through without a request; the rest are +// examined concurrently. A per-tag failure becomes a ResolvePending entry, not a +// returned error, so the rest of the pass is still published. resolveChartVersions +// itself returns an error in three cases, none of them a per-tag verdict, and all of +// them a failure of the whole pass rather than of one tag: +// - building the shared puller fails: a transport/auth setup step that happens once +// for the repository, not per tag, so its failure cannot be attributed to any tag; +// - a per-tag request is rejected with 401/403: credentials rejected for one tag are +// rejected for all of them, so resolveChartVersion escalates it to a terminal +// error instead of a ResolvePending verdict, and group.Wait propagates it here; +// - the parent context is cancelled while tags are still in flight, which would +// otherwise make every in-flight remote.Get fail and be misreported as a pass full +// of fabricated ResolvePending verdicts instead of the abandoned pass it is. +func resolveChartVersions( + ctx context.Context, + repo name.Repository, + chartName string, + tags []string, + options []remote.Option, + opts FetchOptions, +) ([]ChartVersion, error) { + type candidate struct { + tag string + version *semver.Version + known *KnownVersion + } + + candidates := make([]candidate, 0, len(tags)) for _, tag := range tags { if isCosignTag(tag) { @@ -105,15 +159,170 @@ func (c *ociRepositoryClient) FetchCharts(ctx context.Context, url string, confi continue } - chartVersions = append(chartVersions, ChartVersion{Version: semVersion}) + c := candidate{tag: tag, version: semVersion} + if !opts.NeedsExamination(chartName, tag) { + known := opts.Known[chartName][tag] + c.known = &known + } + + candidates = append(candidates, c) } - return []Chart{ - { - Name: chartName, - Versions: chartVersions, - }, - }, nil + resolved := make([]*ChartVersion, len(candidates)) + + group, groupCtx := errgroup.WithContext(ctx) + group.SetLimit(chartResolveConcurrency) + + // A fresh slice per pass: appending to the shared options slice from several + // goroutines would race on its backing array. + tagOptions := make([]remote.Option, 0, len(options)+2) + tagOptions = append(tagOptions, options...) + tagOptions = append(tagOptions, + remote.WithContext(groupCtx), + // One attempt per tag: a failure is recorded as pending and retried by the next + // synchronization anyway, and retrying here would multiply the duration of a + // pass over a degraded registry. + remote.WithRetryBackoff(remote.Backoff{Duration: time.Millisecond, Factor: 1.0, Steps: 1}), + ) + + // One puller for the whole repository: it caches its fetcher (and therefore the + // auth handshake) per repository behind a sync.Map/sync.Once and is safe for + // concurrent use. remote.Get would build a fresh Puller per call, paying a fresh + // /v2/ ping - and, against a bearer registry, a fresh token request - for every + // examined tag. + puller, err := remote.NewPuller(tagOptions...) + if err != nil { + return nil, fmt.Errorf("building the registry puller: %w", err) + } + + for i := range candidates { + index, c := i, candidates[i] + + group.Go(func() error { + if c.known != nil { + resolved[index] = carryKnown(c.version, *c.known) + + return nil + } + + version, err := resolveChartVersion(groupCtx, puller, repo.Tag(c.tag), c.version) + if err != nil { + return err + } + resolved[index] = version + + return nil + }) + } + + if err := group.Wait(); err != nil { + return nil, err + } + + // A cancelled parent context makes every remote.Get fail without surfacing as a + // transport error, so every goroutine above would otherwise return a fabricated + // ResolvePending verdict instead of an error. That is indistinguishable from a real + // registry failure and must not be reported as a completed pass. + if err := ctx.Err(); err != nil { + return nil, err + } + + versions := make([]ChartVersion, 0, len(resolved)) + for _, version := range resolved { + if version == nil { + continue + } + versions = append(versions, *version) + } + + return versions, nil +} + +// carryKnown reuses a recorded verdict. A tag that is listed again is by definition no +// longer removed from the repository, so that marker - and the message describing the +// absence it no longer holds - is dropped here: presence in the listing is registry +// truth, which is this client's domain. +func carryKnown(version *semver.Version, known KnownVersion) *ChartVersion { + reason, message := known.UnavailableReason, known.UnavailableMessage + if reason == helmv1alpha1.UnavailableReasonRemovedFromRepository { + reason, message = "", "" + } + + return &ChartVersion{ + Version: version, + MediaType: known.MediaType, + UnavailableReason: reason, + UnavailableMessage: message, + } +} + +// resolveChartVersion examines one tag. A nil version with a nil error means the tag +// vanished between the listing and this request and must be treated as unlisted. A +// non-nil error is always terminal: credentials rejected for one tag are rejected for +// all of them, so there is no point in requesting the rest. puller is shared across all +// tags of the pass so the auth handshake happens once for the repository rather than +// once per tag. +func resolveChartVersion(ctx context.Context, puller *remote.Puller, ref name.Reference, version *semver.Version) (*ChartVersion, error) { + desc, err := puller.Get(ctx, ref) + if err != nil { + var transportErr *transport.Error + if errors.As(err, &transportErr) { + switch transportErr.StatusCode { + case http.StatusNotFound: + return nil, nil + case http.StatusUnauthorized, http.StatusForbidden: + terminal := TerminalFromStatusCode(transportErr.StatusCode, ref.String()) + terminal.Err = err + + return nil, terminal + } + } + + return pendingVersion(version, err.Error()), nil + } + + manifest, err := v1.ParseManifest(bytes.NewReader(desc.Manifest)) + if err != nil { + // An unreadable manifest is a verdict about the artifact, not a transport + // problem, so it is recorded the same way as an unsupported media type. + return unsupportedVersion(version, "cannot parse the manifest: "+err.Error()), nil + } + + verdict := examineManifest(desc.MediaType, manifest) + if !verdict.OK() { + return unsupportedVersion(version, verdict.Message), nil + } + + return &ChartVersion{Version: version, MediaType: verdict.MediaType}, nil +} + +func pendingVersion(version *semver.Version, message string) *ChartVersion { + return &ChartVersion{ + Version: version, + UnavailableReason: helmv1alpha1.UnavailableReasonResolvePending, + UnavailableMessage: truncate(message), + } +} + +func unsupportedVersion(version *semver.Version, message string) *ChartVersion { + return &ChartVersion{ + Version: version, + UnavailableReason: helmv1alpha1.UnavailableReasonUnsupportedMediaType, + UnavailableMessage: truncate(message), + } +} + +func truncate(message string) string { + if len(message) <= unavailableMessageLimit { + return message + } + + cut := unavailableMessageLimit + for cut > 0 && !utf8.RuneStart(message[cut]) { + cut-- + } + + return message[:cut] + "…" } func trimSchemaPrefixes(url string) string { @@ -124,11 +333,6 @@ func trimSchemaPrefixes(url string) string { return url } -func isSemverCompliantTag(tag string) bool { - _, err := semver.NewVersion(tag) - return err == nil -} - func isCosignTag(tag string) bool { for _, suffix := range []string{".att", ".sbom", ".sig"} { if strings.HasSuffix(tag, suffix) { diff --git a/images/operator-helm-controller/internal/client/repository/oci_chart.go b/images/operator-helm-controller/internal/client/repository/oci_chart.go new file mode 100644 index 00000000..3411a2ea --- /dev/null +++ b/images/operator-helm-controller/internal/client/repository/oci_chart.go @@ -0,0 +1,100 @@ +/* +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 repository + +import ( + "fmt" + "strings" + + v1 "github.com/google/go-containerregistry/pkg/v1" + "github.com/google/go-containerregistry/pkg/v1/types" +) + +// supportedChartConfigMediaTypes identify an OCI artifact as a packaged Helm chart. +// The layer media type alone cannot: application/tar+gzip is generic and any tarball +// may carry it, so the config is the authoritative marker. +var supportedChartConfigMediaTypes = []types.MediaType{ + "application/vnd.cncf.helm.config.v1+json", +} + +// supportedChartLayerMediaTypes hold a packaged chart, in priority order. The first +// entry present in the manifest wins regardless of the order of layers inside it, so +// an artifact carrying two supported layers resolves deterministically. +var supportedChartLayerMediaTypes = []types.MediaType{ + "application/vnd.cncf.helm.chart.content.v1.tar+gzip", + "application/tar+gzip", +} + +// chartVerdict is the outcome of examining one tag. MediaType is set only for an +// artifact recognized as a chart; Message explains a negative verdict and is meant +// for the status of the chart version. +type chartVerdict struct { + MediaType string + Message string +} + +// OK reports whether the artifact is a usable chart. +func (v chartVerdict) OK() bool { return v.MediaType != "" } + +// examineManifest decides whether a manifest describes a packaged Helm chart and, if +// it does, which layer media type holds it. descMediaType is the media type of the +// manifest descriptor: an index is rejected here rather than left to fail later, +// because an index is never a chart and a failure would be re-probed forever. +func examineManifest(descMediaType types.MediaType, manifest *v1.Manifest) chartVerdict { + if descMediaType.IsIndex() { + return chartVerdict{ + Message: fmt.Sprintf("the tag points to an index (%s), not to a chart manifest", descMediaType), + } + } + + if !isSupportedChartConfig(manifest.Config.MediaType) { + return chartVerdict{ + Message: fmt.Sprintf("config media type %q is not a helm chart config", manifest.Config.MediaType), + } + } + + for _, supported := range supportedChartLayerMediaTypes { + for _, layer := range manifest.Layers { + if layer.MediaType == supported { + return chartVerdict{MediaType: string(supported)} + } + } + } + + return chartVerdict{ + Message: fmt.Sprintf("no supported chart layer, the artifact has [%s]", layerMediaTypes(manifest)), + } +} + +func isSupportedChartConfig(mediaType types.MediaType) bool { + for _, supported := range supportedChartConfigMediaTypes { + if mediaType == supported { + return true + } + } + + return false +} + +func layerMediaTypes(manifest *v1.Manifest) string { + found := make([]string, 0, len(manifest.Layers)) + for _, layer := range manifest.Layers { + found = append(found, string(layer.MediaType)) + } + + return strings.Join(found, ", ") +} diff --git a/images/operator-helm-controller/internal/client/repository/oci_chart_test.go b/images/operator-helm-controller/internal/client/repository/oci_chart_test.go new file mode 100644 index 00000000..7280a53b --- /dev/null +++ b/images/operator-helm-controller/internal/client/repository/oci_chart_test.go @@ -0,0 +1,127 @@ +/* +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 repository + +import ( + "strings" + "testing" + + v1 "github.com/google/go-containerregistry/pkg/v1" + "github.com/google/go-containerregistry/pkg/v1/types" +) + +func manifestWith(configType types.MediaType, layerTypes ...types.MediaType) *v1.Manifest { + manifest := &v1.Manifest{ + SchemaVersion: 2, + MediaType: types.OCIManifestSchema1, + Config: v1.Descriptor{MediaType: configType}, + } + for _, layerType := range layerTypes { + manifest.Layers = append(manifest.Layers, v1.Descriptor{MediaType: layerType}) + } + + return manifest +} + +func TestExamineManifest(t *testing.T) { + const ( + helmConfig = types.MediaType("application/vnd.cncf.helm.config.v1+json") + currentType = "application/vnd.cncf.helm.chart.content.v1.tar+gzip" + legacyType = "application/tar+gzip" + provType = types.MediaType("application/vnd.cncf.helm.chart.provenance.v1.prov") + ) + + cases := []struct { + name string + descMediaType types.MediaType + manifest *v1.Manifest + wantMediaType string + wantMessage string + }{ + { + name: "current layer type", + descMediaType: types.OCIManifestSchema1, + manifest: manifestWith(helmConfig, currentType), + wantMediaType: currentType, + }, + { + name: "legacy layer type", + descMediaType: types.OCIManifestSchema1, + manifest: manifestWith(helmConfig, legacyType), + wantMediaType: legacyType, + }, + { + name: "chart layer is not the first one", + descMediaType: types.OCIManifestSchema1, + manifest: manifestWith(helmConfig, provType, legacyType), + wantMediaType: legacyType, + }, + { + name: "the list order wins over the manifest order", + descMediaType: types.OCIManifestSchema1, + manifest: manifestWith(helmConfig, legacyType, currentType), + wantMediaType: currentType, + }, + { + name: "an index is not a chart", + descMediaType: types.OCIImageIndex, + manifest: manifestWith(helmConfig, currentType), + wantMessage: "index", + }, + { + name: "a foreign config is not a chart", + descMediaType: types.OCIManifestSchema1, + manifest: manifestWith("application/vnd.unknown.config.v1+json", legacyType), + wantMessage: "application/vnd.unknown.config.v1+json", + }, + { + name: "no supported layer", + descMediaType: types.OCIManifestSchema1, + manifest: manifestWith(helmConfig, provType), + wantMessage: string(provType), + }, + { + name: "no layers at all", + descMediaType: types.OCIManifestSchema1, + manifest: manifestWith(helmConfig), + wantMessage: "no supported chart layer", + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + verdict := examineManifest(tc.descMediaType, tc.manifest) + + if tc.wantMediaType != "" { + if !verdict.OK() { + t.Fatalf("expected a chart verdict, got message %q", verdict.Message) + } + if verdict.MediaType != tc.wantMediaType { + t.Fatalf("media type is %q, want %q", verdict.MediaType, tc.wantMediaType) + } + return + } + + if verdict.OK() { + t.Fatalf("expected a negative verdict, got media type %q", verdict.MediaType) + } + if !strings.Contains(verdict.Message, tc.wantMessage) { + t.Fatalf("message %q does not mention %q", verdict.Message, tc.wantMessage) + } + }) + } +} diff --git a/images/operator-helm-controller/internal/client/repository/oci_test.go b/images/operator-helm-controller/internal/client/repository/oci_test.go index 5884ac8d..31d05456 100644 --- a/images/operator-helm-controller/internal/client/repository/oci_test.go +++ b/images/operator-helm-controller/internal/client/repository/oci_test.go @@ -19,16 +19,30 @@ package repository import ( "context" "errors" + "io" + "log" "net/http" + "net/http/httptest" + "strings" + "sync" "testing" + "unicode/utf8" + "github.com/google/go-containerregistry/pkg/name" + "github.com/google/go-containerregistry/pkg/registry" + "github.com/google/go-containerregistry/pkg/v1/empty" + "github.com/google/go-containerregistry/pkg/v1/mutate" + "github.com/google/go-containerregistry/pkg/v1/random" + "github.com/google/go-containerregistry/pkg/v1/remote" "github.com/google/go-containerregistry/pkg/v1/remote/transport" + "github.com/google/go-containerregistry/pkg/v1/static" + "github.com/google/go-containerregistry/pkg/v1/types" helmv1alpha1 "github.com/deckhouse/operator-helm/api/v1alpha1" ) func TestFetchChartsOCIRejectsURLWithoutImageName(t *testing.T) { - _, err := OCIRepositoryDefaultClient.FetchCharts(context.Background(), "oci://ghcr.io", nil) + _, err := OCIRepositoryDefaultClient.FetchCharts(context.Background(), "oci://ghcr.io", nil, FetchOptions{}) if err == nil { t.Fatal("expected error for url without an image name") } @@ -89,3 +103,405 @@ func TestClassifyRemoteError(t *testing.T) { }) } } + +const ( + helmConfigMediaType = types.MediaType("application/vnd.cncf.helm.config.v1+json") + currentChartMediaType = "application/vnd.cncf.helm.chart.content.v1.tar+gzip" + legacyChartMediaType = "application/tar+gzip" +) + +// fakeRegistry is an in-memory OCI registry that can count manifest reads and force a +// status code for a specific tag. Counting is what proves the incremental behaviour: +// the number of manifest requests, not the returned versions, is the point of D3. +type fakeRegistry struct { + inner http.Handler + mu sync.Mutex + gets map[string]int + forced map[string]int +} + +func newFakeRegistry(t *testing.T) (*fakeRegistry, string) { + t.Helper() + + f := &fakeRegistry{ + inner: registry.New(registry.Logger(log.New(io.Discard, "", 0))), + gets: map[string]int{}, + forced: map[string]int{}, + } + server := httptest.NewServer(f) + t.Cleanup(server.Close) + + return f, strings.TrimPrefix(server.URL, "http://") +} + +func (f *fakeRegistry) ServeHTTP(w http.ResponseWriter, r *http.Request) { + if tag, ok := manifestTag(r); ok { + f.mu.Lock() + f.gets[tag]++ + status := f.forced[tag] + f.mu.Unlock() + + if status != 0 { + w.WriteHeader(status) + _, _ = w.Write([]byte(`{"errors":[{"code":"UNKNOWN"}]}`)) + + return + } + } + + f.inner.ServeHTTP(w, r) +} + +// manifestTag reports the tag of a manifest read request. Digest references are +// ignored: only tag reads are counted and forced. +func manifestTag(r *http.Request) (string, bool) { + if r.Method != http.MethodGet && r.Method != http.MethodHead { + return "", false + } + parts := strings.Split(strings.Trim(r.URL.Path, "/"), "/") + if len(parts) < 4 || parts[0] != "v2" || parts[len(parts)-2] != "manifests" { + return "", false + } + ref := parts[len(parts)-1] + if strings.Contains(ref, ":") { + return "", false + } + + return ref, true +} + +func (f *fakeRegistry) force(tag string, status int) { + f.mu.Lock() + defer f.mu.Unlock() + f.forced[tag] = status +} + +func (f *fakeRegistry) manifestGets(tag string) int { + f.mu.Lock() + defer f.mu.Unlock() + + return f.gets[tag] +} + +// reset clears the recorded manifest request counts. remote.Write can itself read +// manifests while pushing test fixtures, so tests that assert on request counts must +// reset the counter after the setup pushes and before the measured call. +func (f *fakeRegistry) reset() { + f.mu.Lock() + defer f.mu.Unlock() + f.gets = map[string]int{} +} + +// pushChart writes a single-layer artifact with the given config and layer media +// types under host/podinfo:tag. +func pushChart(t *testing.T, host, tag string, configType types.MediaType, layerTypes ...types.MediaType) { + t.Helper() + + img := empty.Image + for _, layerType := range layerTypes { + appended, err := mutate.Append(img, mutate.Addendum{ + Layer: static.NewLayer([]byte("chart-"+tag), layerType), + MediaType: layerType, + }) + if err != nil { + t.Fatalf("appending layer: %v", err) + } + img = appended + } + + img = mutate.MediaType(img, types.OCIManifestSchema1) + img = mutate.ConfigMediaType(img, configType) + + ref, err := name.NewTag(host + "/podinfo:" + tag) + if err != nil { + t.Fatalf("parsing tag: %v", err) + } + if err := remote.Write(ref, img); err != nil { + t.Fatalf("pushing %s: %v", ref, err) + } +} + +func fetchOne(t *testing.T, host string, opts FetchOptions) Chart { + t.Helper() + + charts, err := OCIRepositoryDefaultClient.FetchCharts(context.Background(), "oci://"+host+"/podinfo", nil, opts) + if err != nil { + t.Fatalf("FetchCharts returned %v", err) + } + if len(charts) != 1 { + t.Fatalf("expected one chart, got %d", len(charts)) + } + + return charts[0] +} + +func versionByTag(t *testing.T, chart Chart, tag string) ChartVersion { + t.Helper() + + for _, version := range chart.Versions { + if version.Version.Original() == tag { + return version + } + } + t.Fatalf("version %q is missing from %v", tag, chart.Versions) + + return ChartVersion{} +} + +func TestFetchChartsOCIResolvesLayerMediaTypes(t *testing.T) { + _, host := newFakeRegistry(t) + pushChart(t, host, "1.0.0", helmConfigMediaType, currentChartMediaType) + pushChart(t, host, "1.0.1", helmConfigMediaType, legacyChartMediaType) + pushChart(t, host, "1.0.2", "application/vnd.unknown.config.v1+json", legacyChartMediaType) + + chart := fetchOne(t, host, FetchOptions{}) + + if got := versionByTag(t, chart, "1.0.0").MediaType; got != currentChartMediaType { + t.Fatalf("1.0.0 media type is %q, want %q", got, currentChartMediaType) + } + if got := versionByTag(t, chart, "1.0.1").MediaType; got != legacyChartMediaType { + t.Fatalf("1.0.1 media type is %q, want %q", got, legacyChartMediaType) + } + + unsupported := versionByTag(t, chart, "1.0.2") + if unsupported.MediaType != "" { + t.Fatalf("1.0.2 must not carry a media type, got %q", unsupported.MediaType) + } + if unsupported.UnavailableReason != helmv1alpha1.UnavailableReasonUnsupportedMediaType { + t.Fatalf("1.0.2 reason is %q, want %q", unsupported.UnavailableReason, helmv1alpha1.UnavailableReasonUnsupportedMediaType) + } + if !strings.Contains(unsupported.UnavailableMessage, "application/vnd.unknown.config.v1+json") { + t.Fatalf("1.0.2 message %q must name the observed config type", unsupported.UnavailableMessage) + } +} + +func TestFetchChartsOCIRejectsIndexTag(t *testing.T) { + _, host := newFakeRegistry(t) + + idx, err := random.Index(256, 1, 2) + if err != nil { + t.Fatalf("building index: %v", err) + } + ref, err := name.NewTag(host + "/podinfo:2.0.0") + if err != nil { + t.Fatalf("parsing tag: %v", err) + } + if err := remote.WriteIndex(ref, idx); err != nil { + t.Fatalf("pushing index: %v", err) + } + + version := versionByTag(t, fetchOne(t, host, FetchOptions{}), "2.0.0") + if version.UnavailableReason != helmv1alpha1.UnavailableReasonUnsupportedMediaType { + t.Fatalf("an index must be a verdict, got reason %q", version.UnavailableReason) + } +} + +func TestFetchChartsOCIMarksTransportFailurePending(t *testing.T) { + reg, host := newFakeRegistry(t) + pushChart(t, host, "1.0.0", helmConfigMediaType, currentChartMediaType) + pushChart(t, host, "1.0.1", helmConfigMediaType, currentChartMediaType) + reg.force("1.0.1", http.StatusInternalServerError) + + chart := fetchOne(t, host, FetchOptions{}) + + if got := versionByTag(t, chart, "1.0.0").MediaType; got != currentChartMediaType { + t.Fatalf("a healthy tag must still be published, got media type %q", got) + } + + pending := versionByTag(t, chart, "1.0.1") + if pending.UnavailableReason != helmv1alpha1.UnavailableReasonResolvePending { + t.Fatalf("reason is %q, want %q", pending.UnavailableReason, helmv1alpha1.UnavailableReasonResolvePending) + } + if pending.UnavailableMessage == "" { + t.Fatal("a pending version must carry the error message") + } +} + +func TestFetchChartsOCIDropsVanishedTag(t *testing.T) { + reg, host := newFakeRegistry(t) + pushChart(t, host, "1.0.0", helmConfigMediaType, currentChartMediaType) + pushChart(t, host, "1.0.1", helmConfigMediaType, currentChartMediaType) + reg.force("1.0.1", http.StatusNotFound) + + chart := fetchOne(t, host, FetchOptions{}) + + if got := versionByTag(t, chart, "1.0.0").MediaType; got != currentChartMediaType { + t.Fatalf("a healthy tag must still be published, got media type %q", got) + } + + for _, version := range chart.Versions { + if version.Version.Original() == "1.0.1" { + t.Fatal("a tag that 404s on its manifest must be omitted") + } + } +} + +func TestFetchChartsOCIEscalatesUnauthorized(t *testing.T) { + reg, host := newFakeRegistry(t) + pushChart(t, host, "1.0.0", helmConfigMediaType, currentChartMediaType) + reg.force("1.0.0", http.StatusUnauthorized) + + _, err := OCIRepositoryDefaultClient.FetchCharts(context.Background(), "oci://"+host+"/podinfo", nil, FetchOptions{}) + if err == nil { + t.Fatal("expected an error") + } + terminal, ok := AsTerminal(err) + if !ok { + t.Fatalf("expected a terminal error, got %v", err) + } + if terminal.Reason != helmv1alpha1.ReasonAuthenticationFailed { + t.Fatalf("reason is %q, want %q", terminal.Reason, helmv1alpha1.ReasonAuthenticationFailed) + } +} + +// TestFetchChartsOCIResolveVersionsSurfacesCancelledContext exercises resolveChartVersions +// directly rather than through FetchCharts: remote.List already fails a cancelled context +// on its own (unaffected by this fix), so reproducing the bug end-to-end would require +// cancelling the context after the listing but during the per-tag resolve, which is timing +// dependent. Calling the resolve stage directly reproduces it deterministically: without +// the ctx.Err() check, every goroutine's remote.Get fails with a wrapped context.Canceled +// that is not a *transport.Error, so it falls through to a fabricated ResolvePending verdict +// and resolveChartVersions returns a clean result with a nil error. +func TestFetchChartsOCIResolveVersionsSurfacesCancelledContext(t *testing.T) { + _, host := newFakeRegistry(t) + pushChart(t, host, "1.0.0", helmConfigMediaType, currentChartMediaType) + + repo, err := name.NewRepository(host + "/podinfo") + if err != nil { + t.Fatalf("parsing repository: %v", err) + } + + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + _, err = resolveChartVersions(ctx, repo, "podinfo", []string{"1.0.0"}, []remote.Option{remote.WithContext(ctx)}, FetchOptions{}) + if err == nil { + t.Fatal("a cancelled context must not produce a completed pass") + } + if !errors.Is(err, context.Canceled) { + t.Fatalf("expected an error wrapping context.Canceled, got %v", err) + } +} + +func TestFetchChartsOCISkipsKnownTags(t *testing.T) { + reg, host := newFakeRegistry(t) + pushChart(t, host, "1.0.0", helmConfigMediaType, currentChartMediaType) + pushChart(t, host, "1.0.1", helmConfigMediaType, legacyChartMediaType) + pushChart(t, host, "1.0.2", helmConfigMediaType, currentChartMediaType) + pushChart(t, host, "1.0.3", helmConfigMediaType, currentChartMediaType) + reg.reset() + + // The message a previous pass recorded for the unsupported verdict. A skipped tag + // is carried through verbatim, so this is test data rather than a message that + // examineManifest would actually produce for this fixture. + const unsupportedMessage = "config media type recorded by a previous pass is not a helm chart config" + + known := KnownCharts{"podinfo": KnownVersions{ + "1.0.0": {MediaType: currentChartMediaType}, + "1.0.1": { + UnavailableReason: helmv1alpha1.UnavailableReasonUnsupportedMediaType, + UnavailableMessage: unsupportedMessage, + }, + "1.0.2": {UnavailableReason: helmv1alpha1.UnavailableReasonResolvePending}, + // An entry written before media types were recorded: the upgrade migration. + "1.0.3": {}, + }} + + chart := fetchOne(t, host, FetchOptions{Known: known}) + + if got := reg.manifestGets("1.0.0"); got != 0 { + t.Fatalf("a resolved tag must not be requested, got %d requests", got) + } + if got := reg.manifestGets("1.0.1"); got != 0 { + t.Fatalf("an unsupported tag must not be requested, got %d requests", got) + } + if got := reg.manifestGets("1.0.2"); got != 1 { + t.Fatalf("a pending tag must be requested once, got %d requests", got) + } + if got := reg.manifestGets("1.0.3"); got != 1 { + t.Fatalf("an entry with no verdict must be requested once, got %d requests", got) + } + + if got := versionByTag(t, chart, "1.0.0").MediaType; got != currentChartMediaType { + t.Fatalf("a skipped tag must keep its media type, got %q", got) + } + if got := versionByTag(t, chart, "1.0.1").UnavailableMessage; got != unsupportedMessage { + t.Fatalf("a skipped unsupported tag must keep its recorded message, got %q, want %q", got, unsupportedMessage) + } + if got := versionByTag(t, chart, "1.0.2").MediaType; got != currentChartMediaType { + t.Fatalf("a re-examined tag must be resolved, got %q", got) + } + if got := versionByTag(t, chart, "1.0.3").MediaType; got != currentChartMediaType { + t.Fatalf("a migrated tag must be resolved, got %q", got) + } +} + +func TestFetchChartsOCIFullIgnoresKnown(t *testing.T) { + reg, host := newFakeRegistry(t) + pushChart(t, host, "1.0.0", helmConfigMediaType, currentChartMediaType) + reg.reset() + + known := KnownCharts{"podinfo": KnownVersions{"1.0.0": {MediaType: currentChartMediaType}}} + fetchOne(t, host, FetchOptions{Known: known, Full: true}) + + if got := reg.manifestGets("1.0.0"); got != 1 { + t.Fatalf("a full pass must re-examine every tag, got %d requests", got) + } +} + +func TestFetchChartsOCIClearsRemovedFromRepository(t *testing.T) { + reg, host := newFakeRegistry(t) + pushChart(t, host, "1.0.0", helmConfigMediaType, currentChartMediaType) + reg.reset() + + known := KnownCharts{"podinfo": KnownVersions{"1.0.0": { + MediaType: currentChartMediaType, + UnavailableReason: helmv1alpha1.UnavailableReasonRemovedFromRepository, + UnavailableMessage: "no longer offered by the repository", + }}} + + version := versionByTag(t, fetchOne(t, host, FetchOptions{Known: known}), "1.0.0") + + // This must be the carry path, not a re-examination that happens to come back + // clean: a NeedsExamination bug that sent RemovedFromRepository down the examine + // path would also clear the reason, so the assertion above alone is not evidence + // of which path ran. + if got := reg.manifestGets("1.0.0"); got != 0 { + t.Fatalf("a tag carried from a known verdict must not be requested, got %d requests", got) + } + if version.UnavailableReason != "" { + t.Fatalf("a listed tag must not stay removed, got reason %q", version.UnavailableReason) + } + if version.UnavailableMessage != "" { + t.Fatalf("clearing the removed marker must also clear its message, got %q", version.UnavailableMessage) + } + if version.MediaType != currentChartMediaType { + t.Fatalf("the recorded media type must survive the carry, got %q", version.MediaType) + } +} + +func TestTruncateCutsOnARuneBoundary(t *testing.T) { + // Every rune here is a 3-byte UTF-8 sequence (☃, U+2603), chosen so that the byte + // limit lands in the middle of one of them: a naive byte slice would corrupt it. + message := strings.Repeat("☃", unavailableMessageLimit/3+2) + + got := truncate(message) + + if !utf8.ValidString(got) { + t.Fatalf("truncate produced invalid UTF-8: %q", got) + } + if len(got) > unavailableMessageLimit+len("…") { + t.Fatalf("truncated message is %d bytes, want at most %d", len(got), unavailableMessageLimit+len("…")) + } + if !strings.HasSuffix(got, "…") { + t.Fatalf("a truncated message must end with the ellipsis marker, got %q", got) + } +} + +func TestTruncateLeavesAShortMessageUnchanged(t *testing.T) { + message := "config media type is not a helm chart config" + + if got := truncate(message); got != message { + t.Fatalf("truncate(%q) = %q, want it unchanged", message, got) + } +} diff --git a/images/operator-helm-controller/internal/index/index.go b/images/operator-helm-controller/internal/index/index.go new file mode 100644 index 00000000..aa9114e5 --- /dev/null +++ b/images/operator-helm-controller/internal/index/index.go @@ -0,0 +1,70 @@ +/* +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 index + +import ( + "context" + + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + + helmv1alpha1 "github.com/deckhouse/operator-helm/api/v1alpha1" +) + +// AddonChart indexes HelmClusterAddon objects by the repository/chart pair they +// reference. The webhook uses it to enforce that a pair is claimed by one addon, and +// the repository synchronization uses it to find the addon that still references a +// chart before pruning anything. +const AddonChart = ".spec.chart.repoAndChart" + +// AddonChartValue builds the index value of a repository/chart pair. +func AddonChartValue(repoName, chartName string) string { + return repoName + "/" + chartName +} + +// SetupAddonChart registers the AddonChart index on the manager's cache. +func SetupAddonChart(mgr ctrl.Manager) error { + return mgr.GetFieldIndexer().IndexField( + context.Background(), &helmv1alpha1.HelmClusterAddon{}, AddonChart, + func(obj client.Object) []string { + addon := obj.(*helmv1alpha1.HelmClusterAddon) + + return []string{AddonChartValue( + addon.Spec.Chart.HelmClusterAddonRepository, + addon.Spec.Chart.HelmClusterAddonChartName, + )} + }, + ) +} + +// AddonRepository indexes HelmClusterAddon objects by the repository they reference. +const AddonRepository = ".spec.chart.helmClusterAddonRepository" + +// SetupAddonRepository registers the AddonRepository index on the manager's cache. +func SetupAddonRepository(mgr ctrl.Manager) error { + return mgr.GetFieldIndexer().IndexField( + context.Background(), &helmv1alpha1.HelmClusterAddon{}, AddonRepository, + func(obj client.Object) []string { + addon := obj.(*helmv1alpha1.HelmClusterAddon) + if addon.Spec.Chart.HelmClusterAddonRepository == "" { + return nil + } + + return []string{addon.Spec.Chart.HelmClusterAddonRepository} + }, + ) +} diff --git a/images/operator-helm-controller/internal/reconcile/helmclusteraddon/reconciler.go b/images/operator-helm-controller/internal/reconcile/helmclusteraddon/reconciler.go index ded26aee..623e83bd 100644 --- a/images/operator-helm-controller/internal/reconcile/helmclusteraddon/reconciler.go +++ b/images/operator-helm-controller/internal/reconcile/helmclusteraddon/reconciler.go @@ -34,6 +34,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/reconcile" + "github.com/deckhouse/operator-helm/api/naming" helmv1alpha1 "github.com/deckhouse/operator-helm/api/v1alpha1" "github.com/deckhouse/operator-helm/internal/manager/status" "github.com/deckhouse/operator-helm/internal/services" @@ -188,7 +189,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (reco var repoRes services.OCIRepoResult var releaseRes services.ReleaseResult - _, addonChartErr := r.getHelmClusterAddonChart(ctx, addon) + _, chartVersion, addonChartErr := r.getHelmClusterAddonChart(ctx, addon, repoType) switch repoType { case utils.InternalHelmRepository: @@ -212,8 +213,10 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (reco chartRes = r.chartService.EnsureHelmChart(ctx, addon) case utils.InternalOCIRepository: if addonChartErr != nil { + // addonChartErr, not err: err is the (nil) result of GetRepositoryType above, + // so passing it dropped the real cause. repoRes = services.OCIRepoResult{ - Status: status.Failed(addon, helmv1alpha1.ReasonFailed, "failed to get desired chart version", err), + Status: status.Failed(addon, helmv1alpha1.ReasonFailed, "failed to get desired chart version", addonChartErr), } break @@ -226,7 +229,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (reco break } - repoRes = r.ociRepositoryService.EnsureInternalOCIRepository(ctx, addon, repo) + repoRes = r.ociRepositoryService.EnsureInternalOCIRepository(ctx, addon, repo, chartVersion) default: return reconcile.Result{}, r.statusManager.Update(ctx, addon, status.NoopStatusMutator, status.NoopStatusMapper, services.ReleaseResult{Status: status.Failed( addon, @@ -414,22 +417,55 @@ func (r *Reconciler) reconcileForceAnnotation(ctx context.Context, req reconcile return nil } -func (r *Reconciler) getHelmClusterAddonChart(ctx context.Context, addon *helmv1alpha1.HelmClusterAddon) (*helmv1alpha1.HelmClusterAddonChart, error) { - addonChartName := utils.GetHelmClusterAddonChartName(addon.Spec.Chart.HelmClusterAddonRepository, addon.Spec.Chart.HelmClusterAddonChartName) +// getHelmClusterAddonChart resolves the catalog entry for the version the addon asks +// for. For an OCI repository an entry is usable only when it carries a media type: +// that is exactly "we know enough to build the internal OCIRepository". A version +// retained after its tag disappeared keeps its media type, so this gate stays open for +// it and the addon keeps reconciling everything else — its values, its maintenance +// mode, its removal. +func (r *Reconciler) getHelmClusterAddonChart( + ctx context.Context, + addon *helmv1alpha1.HelmClusterAddon, + repoType utils.InternalRepositoryType, +) (*helmv1alpha1.HelmClusterAddonChart, *helmv1alpha1.HelmClusterAddonChartVersion, error) { + addonChartName := naming.HelmClusterAddonChartName( + addon.Spec.Chart.HelmClusterAddonRepository, addon.Spec.Chart.HelmClusterAddonChartName, + ) addonChart := &helmv1alpha1.HelmClusterAddonChart{} - err := r.Get(ctx, types.NamespacedName{Name: addonChartName}, addonChart) - if err != nil { - return nil, fmt.Errorf("getting helm cluster addon chart: %w", err) + if err := r.Get(ctx, types.NamespacedName{Name: addonChartName}, addonChart); err != nil { + return nil, nil, fmt.Errorf("getting helm cluster addon chart: %w", err) } - for _, version := range addonChart.Status.Versions { - if version.Version == addon.Spec.Chart.Version { - return addonChart, nil + for i := range addonChart.Status.Versions { + version := &addonChart.Status.Versions[i] + if version.Version != addon.Spec.Chart.Version { + continue + } + + if repoType == utils.InternalOCIRepository && version.MediaType == "" { + return nil, nil, fmt.Errorf( + "chart version %q cannot be deployed: %s", + version.Version, versionUnavailableDetail(*version), + ) } + + return addonChart, version, nil } - return nil, fmt.Errorf("helm cluster addon chart does not have version %q", addon.Spec.Chart.Version) + return nil, nil, fmt.Errorf("helm cluster addon chart does not have version %q", addon.Spec.Chart.Version) +} + +// versionUnavailableDetail explains why a catalog entry is not deployable. +func versionUnavailableDetail(version helmv1alpha1.HelmClusterAddonChartVersion) string { + switch { + case version.UnavailableReason == "": + return "the repository catalog has not resolved it yet" + case version.UnavailableMessage == "": + return version.UnavailableReason + default: + return version.UnavailableReason + ": " + version.UnavailableMessage + } } func setStatusAttrs(repoType utils.InternalRepositoryType, chartRes services.ChartResult, repoRes services.OCIRepoResult, releaseRes services.ReleaseResult) status.MutatorFunc { diff --git a/images/operator-helm-controller/internal/reconcile/helmclusteraddon/reconciler_test.go b/images/operator-helm-controller/internal/reconcile/helmclusteraddon/reconciler_test.go new file mode 100644 index 00000000..5c03c0b7 --- /dev/null +++ b/images/operator-helm-controller/internal/reconcile/helmclusteraddon/reconciler_test.go @@ -0,0 +1,236 @@ +/* +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 helmclusteraddon + +import ( + "context" + "strings" + "testing" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + clientgoscheme "k8s.io/client-go/kubernetes/scheme" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + + "github.com/deckhouse/operator-helm/api/naming" + helmv1alpha1 "github.com/deckhouse/operator-helm/api/v1alpha1" + "github.com/deckhouse/operator-helm/internal/utils" +) + +func testScheme(t *testing.T) *runtime.Scheme { + t.Helper() + + scheme := runtime.NewScheme() + if err := clientgoscheme.AddToScheme(scheme); err != nil { + t.Fatalf("registering client-go scheme: %v", err) + } + if err := helmv1alpha1.AddToScheme(scheme); err != nil { + t.Fatalf("registering helm scheme: %v", err) + } + + return scheme +} + +func newTestReconciler(t *testing.T, objects ...client.Object) *Reconciler { + t.Helper() + + scheme := testScheme(t) + c := fake.NewClientBuilder().WithScheme(scheme).WithObjects(objects...).Build() + + return &Reconciler{Client: c} +} + +func testAddon() *helmv1alpha1.HelmClusterAddon { + return &helmv1alpha1.HelmClusterAddon{ + ObjectMeta: metav1.ObjectMeta{Name: "consumer", Generation: 1}, + Spec: helmv1alpha1.HelmClusterAddonSpec{ + Namespace: "app", + Chart: helmv1alpha1.HelmClusterAddonChartRef{ + HelmClusterAddonRepository: "example", + HelmClusterAddonChartName: "podinfo", + Version: "6.7.1", + }, + }, + } +} + +func addonChartFixture(repoName, chartName string, versions ...helmv1alpha1.HelmClusterAddonChartVersion) *helmv1alpha1.HelmClusterAddonChart { + return &helmv1alpha1.HelmClusterAddonChart{ + ObjectMeta: metav1.ObjectMeta{ + Name: naming.HelmClusterAddonChartName(repoName, chartName), + }, + Status: helmv1alpha1.HelmClusterAddonChartStatus{Versions: versions}, + } +} + +// TestGetHelmClusterAddonChart pins the gate that decides whether an addon has +// enough information to be deployed. For an OCI repository, a catalog entry is +// usable exactly when it carries a media type; for a Helm repository the media +// type is never checked, so an entry is usable as soon as the version is present. +func TestGetHelmClusterAddonChart(t *testing.T) { + addon := testAddon() + + tests := []struct { + name string + // version is the sole entry seeded into the HelmClusterAddonChart's + // Status.Versions. Its own Version field decides whether the lookup by + // addon.Spec.Chart.Version ("6.7.1") hits or misses. + version helmv1alpha1.HelmClusterAddonChartVersion + repoType utils.InternalRepositoryType + wantErr bool + wantErrContain string + }{ + { + name: "oci version with a media type passes", + version: helmv1alpha1.HelmClusterAddonChartVersion{ + Version: "6.7.1", + MediaType: "application/vnd.cncf.helm.chart.content.v1.tar+gzip", + }, + repoType: utils.InternalOCIRepository, + }, + { + // Deliberate: the tag disappeared from the repository, but the entry is + // retained with its media type so the addon keeps reconciling everything + // else. The real pull failure is reported by the source controller. + name: "oci version removed from repository but with a media type still passes", + version: helmv1alpha1.HelmClusterAddonChartVersion{ + Version: "6.7.1", + MediaType: "application/tar+gzip", + UnavailableReason: helmv1alpha1.UnavailableReasonRemovedFromRepository, + }, + repoType: utils.InternalOCIRepository, + }, + { + name: "oci version stuck resolving is rejected with reason and message", + version: helmv1alpha1.HelmClusterAddonChartVersion{ + Version: "6.7.1", + UnavailableReason: helmv1alpha1.UnavailableReasonResolvePending, + UnavailableMessage: "manifest request failed", + }, + repoType: utils.InternalOCIRepository, + wantErr: true, + wantErrContain: "ResolvePending: manifest request failed", + }, + { + name: "oci version with unsupported media type and no message is rejected with reason alone", + version: helmv1alpha1.HelmClusterAddonChartVersion{ + Version: "6.7.1", + UnavailableReason: helmv1alpha1.UnavailableReasonUnsupportedMediaType, + }, + repoType: utils.InternalOCIRepository, + wantErr: true, + wantErrContain: "UnsupportedMediaType", + }, + { + // Same empty-media-type entry as above, but a Helm repository's versions + // never carry a media type: a stricter gate here would break every Helm + // addon, so the presence check alone must let it through. + name: "the same empty media type entry passes for a helm repository", + version: helmv1alpha1.HelmClusterAddonChartVersion{ + Version: "6.7.1", + UnavailableReason: helmv1alpha1.UnavailableReasonUnsupportedMediaType, + }, + repoType: utils.InternalHelmRepository, + }, + { + // The repository's URL just switched from oci:// to https://: the + // catalog entry is still OCI-era (it carries a media type from the last + // OCI sync), but the Helm gate never reads the media type, so it passes. + name: "oci-era entry with a media type still passes right after switching to a helm repository", + version: helmv1alpha1.HelmClusterAddonChartVersion{ + Version: "6.7.1", + MediaType: "application/vnd.cncf.helm.chart.content.v1.tar+gzip", + }, + repoType: utils.InternalHelmRepository, + }, + { + // The repository's URL just switched from https:// to oci://, but the + // first OCI sync has not resolved the tag's media type yet: the entry is + // still Helm-era (no media type, no reason), so the OCI gate must reject + // it rather than let an unresolved layer through. + name: "helm-era entry with no media type is rejected right after switching to an oci repository", + version: helmv1alpha1.HelmClusterAddonChartVersion{ + Version: "6.7.1", + }, + repoType: utils.InternalOCIRepository, + wantErr: true, + wantErrContain: "has not resolved it yet", + }, + { + name: "a version the addon does not reference is rejected", + version: helmv1alpha1.HelmClusterAddonChartVersion{Version: "9.9.9"}, + repoType: utils.InternalOCIRepository, + wantErr: true, + wantErrContain: `does not have version "6.7.1"`, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + chart := addonChartFixture( + addon.Spec.Chart.HelmClusterAddonRepository, addon.Spec.Chart.HelmClusterAddonChartName, tt.version, + ) + r := newTestReconciler(t, chart) + + gotChart, gotVersion, err := r.getHelmClusterAddonChart(context.Background(), addon, tt.repoType) + + if tt.wantErr { + if err == nil { + t.Fatalf("expected an error, got version %+v", gotVersion) + } + if !strings.Contains(err.Error(), tt.wantErrContain) { + t.Fatalf("error %q does not contain %q", err.Error(), tt.wantErrContain) + } + if gotChart != nil || gotVersion != nil { + t.Fatalf("expected nil chart and version on error, got chart=%v version=%v", gotChart, gotVersion) + } + + return + } + + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if gotChart == nil { + t.Fatal("expected the chart to be returned") + } + if gotVersion == nil { + t.Fatal("expected the matched version to be returned") + } + if gotVersion.Version != tt.version.Version { + t.Fatalf("returned version = %q, want %q", gotVersion.Version, tt.version.Version) + } + if gotVersion.MediaType != tt.version.MediaType { + t.Fatalf("returned version media type = %q, want %q", gotVersion.MediaType, tt.version.MediaType) + } + }) + } +} + +func TestGetHelmClusterAddonChartMissingChart(t *testing.T) { + addon := testAddon() + r := newTestReconciler(t) + + gotChart, gotVersion, err := r.getHelmClusterAddonChart(context.Background(), addon, utils.InternalOCIRepository) + if err == nil { + t.Fatalf("expected an error when the addon chart does not exist, got version %+v", gotVersion) + } + if gotChart != nil || gotVersion != nil { + t.Fatalf("expected nil chart and version on error, got chart=%v version=%v", gotChart, gotVersion) + } +} diff --git a/images/operator-helm-controller/internal/reconcile/helmclusteraddonrepository/evaluate.go b/images/operator-helm-controller/internal/reconcile/helmclusteraddonrepository/evaluate.go index 54257a39..78c725f9 100644 --- a/images/operator-helm-controller/internal/reconcile/helmclusteraddonrepository/evaluate.go +++ b/images/operator-helm-controller/internal/reconcile/helmclusteraddonrepository/evaluate.go @@ -17,6 +17,7 @@ limitations under the License. package helmclusteraddonrepository import ( + "fmt" "math/rand/v2" "time" @@ -109,7 +110,7 @@ func Evaluate(in Inputs) Decision { status.ConsecutiveFetchFailures = failures if in.Attempted { - if fetchSucceeded && !catalogFailed { + if fetchSucceeded && !catalogFailed && in.Fetch.Pending == 0 { status.LastSuccessfulSyncTime = &metav1.Time{Time: in.Now} } @@ -177,6 +178,18 @@ func evaluateSynced(in Inputs, fetchFailed, catalogFailed bool) (metav1.Conditio case catalogFailed: return metav1.ConditionFalse, helmv1alpha1.ReasonCatalogUpdateFailed, "Failed to update the chart catalog: " + in.Catalog.Err.Error() + case in.Fetch != nil && in.Fetch.Pending > 0 && in.Current.LastSuccessfulSyncTime == nil: + // On the very first pass there is no other signal that the read was incomplete: + // lastSuccessfulSyncTime is empty either way, so a user who just created the + // repository would see Synced=True over a partial catalog. Once a full pass has + // happened, the frozen Last Sync column carries that signal instead and Synced + // stops flapping because of a single unreadable tag. The state does not escalate: + // the failure counter is untouched and Stalled is never reached. The nil guard on + // in.Fetch protects against a pre-fetch cluster failure, which leaves Fetch nil + // while still setting Attempted; that case is matched by fetchFailed/catalogFailed + // above today, but the invariant lives elsewhere and must not be relied on here. + return metav1.ConditionFalse, helmv1alpha1.ReasonPartialSync, + fmt.Sprintf("The first repository read left %d chart versions unresolved", in.Fetch.Pending) default: return metav1.ConditionTrue, helmv1alpha1.ReasonSuccess, "" } @@ -424,6 +437,13 @@ func nextFailureCount(failures int32, attempted, fetchFailed bool, fetch *servic return failures } + if fetch == nil { + // The pass ran but never reached the registry (a cluster-side failure, such + // as knownCharts, happened first). That is not evidence the registry + // recovered, so the counter is carried forward rather than reset. + return failures + } + if !fetchFailed { return 0 } diff --git a/images/operator-helm-controller/internal/reconcile/helmclusteraddonrepository/evaluate_test.go b/images/operator-helm-controller/internal/reconcile/helmclusteraddonrepository/evaluate_test.go index b1614d45..e43d75fb 100644 --- a/images/operator-helm-controller/internal/reconcile/helmclusteraddonrepository/evaluate_test.go +++ b/images/operator-helm-controller/internal/reconcile/helmclusteraddonrepository/evaluate_test.go @@ -476,6 +476,153 @@ func TestEvaluateDecisionErr(t *testing.T) { } } +// TestEvaluatePreservesFailuresWhenNoFetchWasAttempted covers the case where a +// synchronization pass ran (Attempted: true, e.g. its knownCharts read failed +// before the registry was ever contacted) but Fetch is nil: nextFailureCount must +// carry ConsecutiveFetchFailures forward rather than resetting it as if the +// registry had just answered successfully, Ready must not be written +// True/Success off that phantom fetch, and Synced must report the catalog +// failure. +func TestEvaluatePreservesFailuresWhenNoFetchWasAttempted(t *testing.T) { + catalogErr := errors.New("listing charts of repository \"example\": etcdserver: request timed out") + + in := Inputs{ + Generation: 1, + Now: testNow, + Current: helmv1alpha1.HelmClusterAddonRepositoryStatus{ + ObservedGeneration: 1, + ConsecutiveFetchFailures: 3, + }, + InternalRepository: services.InternalRepositoryState{Present: true, Ready: true}, + Attempted: true, + Fetch: nil, + Catalog: &services.CatalogOutcome{Err: catalogErr}, + } + + got := Evaluate(in) + + if got.Status.ConsecutiveFetchFailures != 3 { + t.Fatalf("ConsecutiveFetchFailures is %d, want 3 (preserved, not reset by a pass that never reached the registry)", + got.Status.ConsecutiveFetchFailures) + } + + ready := conditionOf(t, got.Status, helmv1alpha1.ConditionTypeReady) + if ready == nil { + t.Fatal("Ready condition must always be present") + } + if ready.Status == metav1.ConditionTrue && ready.Reason == helmv1alpha1.ReasonSuccess { + t.Fatal("Ready must not be written True/Success off a fetch that was never attempted") + } + + synced := conditionOf(t, got.Status, helmv1alpha1.ConditionTypeSynced) + if synced == nil { + t.Fatal("Synced condition is missing") + } + if synced.Status != metav1.ConditionFalse { + t.Fatalf("Synced status is %q, want False", synced.Status) + } + if synced.Reason != helmv1alpha1.ReasonCatalogUpdateFailed { + t.Fatalf("Synced reason is %q, want %q", synced.Reason, helmv1alpha1.ReasonCatalogUpdateFailed) + } +} + +func TestEvaluatePartialFirstSyncIsNotSynced(t *testing.T) { + in := Inputs{ + Generation: 1, + Now: time.Now().UTC(), + Attempted: true, + Fetch: &services.FetchOutcome{Pending: 2}, + Catalog: &services.CatalogOutcome{}, + } + + decision := Evaluate(in) + + synced := apimeta.FindStatusCondition(decision.Status.Conditions, helmv1alpha1.ConditionTypeSynced) + if synced == nil || synced.Status != metav1.ConditionFalse { + t.Fatalf("Synced is %+v, want False", synced) + } + if synced.Reason != helmv1alpha1.ReasonPartialSync { + t.Fatalf("Synced reason is %q, want %q", synced.Reason, helmv1alpha1.ReasonPartialSync) + } + + ready := apimeta.FindStatusCondition(decision.Status.Conditions, helmv1alpha1.ConditionTypeReady) + if ready == nil || ready.Status != metav1.ConditionTrue { + t.Fatalf("Ready is %+v, want True: reading the repository did succeed", ready) + } + if cond := apimeta.FindStatusCondition(decision.Status.Conditions, helmv1alpha1.ConditionTypeReconciling); cond != nil { + t.Fatalf("Reconciling must be absent, got %+v", cond) + } + if decision.Status.LastSuccessfulSyncTime != nil { + t.Fatal("a partial pass must not advance lastSuccessfulSyncTime") + } + if decision.Status.ConsecutiveFetchFailures != 0 { + t.Fatalf("failures are %d, want 0", decision.Status.ConsecutiveFetchFailures) + } +} + +func TestEvaluatePartialSyncAfterFullOneStaysSynced(t *testing.T) { + earlier := metav1.NewTime(time.Now().UTC().Add(-time.Hour)) + in := Inputs{ + Generation: 1, + Now: time.Now().UTC(), + Attempted: true, + Fetch: &services.FetchOutcome{Pending: 1}, + Catalog: &services.CatalogOutcome{}, + Current: helmv1alpha1.HelmClusterAddonRepositoryStatus{ + ObservedGeneration: 1, + LastSuccessfulSyncTime: &earlier, + }, + } + + decision := Evaluate(in) + + synced := apimeta.FindStatusCondition(decision.Status.Conditions, helmv1alpha1.ConditionTypeSynced) + if synced == nil || synced.Status != metav1.ConditionTrue { + t.Fatalf("Synced is %+v, want True", synced) + } + if !decision.Status.LastSuccessfulSyncTime.Equal(&earlier) { + t.Fatalf("lastSuccessfulSyncTime is %v, want it unchanged at %v", decision.Status.LastSuccessfulSyncTime, earlier) + } +} + +func TestEvaluatePartialSyncNeverStalls(t *testing.T) { + current := helmv1alpha1.HelmClusterAddonRepositoryStatus{ObservedGeneration: 1} + + for range MaxFetchFailures + 2 { + decision := Evaluate(Inputs{ + Generation: 1, + Now: time.Now().UTC(), + Attempted: true, + Fetch: &services.FetchOutcome{Pending: 1}, + Catalog: &services.CatalogOutcome{}, + Current: current, + }) + current = decision.Status + } + + if cond := apimeta.FindStatusCondition(current.Conditions, helmv1alpha1.ConditionTypeStalled); cond != nil { + t.Fatalf("repeated partial passes must not stall the repository, got %+v", cond) + } + if current.ConsecutiveFetchFailures != 0 { + t.Fatalf("failures are %d, want 0", current.ConsecutiveFetchFailures) + } +} + +func TestEvaluateFullSyncAdvancesLastSuccessfulSyncTime(t *testing.T) { + now := time.Now().UTC() + decision := Evaluate(Inputs{ + Generation: 1, + Now: now, + Attempted: true, + Fetch: &services.FetchOutcome{}, + Catalog: &services.CatalogOutcome{}, + }) + + if decision.Status.LastSuccessfulSyncTime == nil || !decision.Status.LastSuccessfulSyncTime.Time.Equal(now) { + t.Fatalf("lastSuccessfulSyncTime is %v, want %v", decision.Status.LastSuccessfulSyncTime, now) + } +} + func assertAbnormal(t *testing.T, status helmv1alpha1.HelmClusterAddonRepositoryStatus, conditionType, wantReason string) { t.Helper() diff --git a/images/operator-helm-controller/internal/reconcile/helmclusteraddonrepository/reconciler.go b/images/operator-helm-controller/internal/reconcile/helmclusteraddonrepository/reconciler.go index 5b7dfdbe..fb3bdc3d 100644 --- a/images/operator-helm-controller/internal/reconcile/helmclusteraddonrepository/reconciler.go +++ b/images/operator-helm-controller/internal/reconcile/helmclusteraddonrepository/reconciler.go @@ -135,7 +135,13 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (reco outcome := r.chartSyncService.Sync(ctx, &repo, repoType) in.Attempted = true - in.Fetch = &outcome.Fetch + if outcome.FetchAttempted { + // A cluster-side failure before the fetch (see RepoSyncService.Sync) + // leaves outcome.FetchAttempted false; in.Fetch must stay nil then, or + // its zero-value Err == nil would be read as a successful fetch and + // reset ConsecutiveFetchFailures / mark Ready=True off nothing. + in.Fetch = &outcome.Fetch + } in.Catalog = &outcome.Catalog } diff --git a/images/operator-helm-controller/internal/reconcile/helmclusteraddonrepository/reconciler_test.go b/images/operator-helm-controller/internal/reconcile/helmclusteraddonrepository/reconciler_test.go index bc896aa7..284e0e29 100644 --- a/images/operator-helm-controller/internal/reconcile/helmclusteraddonrepository/reconciler_test.go +++ b/images/operator-helm-controller/internal/reconcile/helmclusteraddonrepository/reconciler_test.go @@ -36,6 +36,7 @@ import ( helmv1alpha1 "github.com/deckhouse/operator-helm/api/v1alpha1" repoclient "github.com/deckhouse/operator-helm/internal/client/repository" + "github.com/deckhouse/operator-helm/internal/index" "github.com/deckhouse/operator-helm/internal/manager/status" "github.com/deckhouse/operator-helm/internal/services" "github.com/deckhouse/operator-helm/internal/utils" @@ -48,7 +49,7 @@ type stubRepoClient struct { // The receiver is a pointer so a test can change what the repository returns // between reconcile passes. -func (s *stubRepoClient) FetchCharts(_ context.Context, _ string, _ *repoclient.RepoConfig) ([]repoclient.Chart, error) { +func (s *stubRepoClient) FetchCharts(_ context.Context, _ string, _ *repoclient.RepoConfig, _ repoclient.FetchOptions) ([]repoclient.Chart, error) { return s.charts, s.err } @@ -73,6 +74,14 @@ func newReconciler(t *testing.T, stub *stubRepoClient, objects ...client.Object) &helmv1alpha1.HelmClusterAddonRepository{}, &helmv1alpha1.HelmClusterAddonChart{}, ). + WithIndex(&helmv1alpha1.HelmClusterAddon{}, index.AddonChart, func(obj client.Object) []string { + addon := obj.(*helmv1alpha1.HelmClusterAddon) + + return []string{index.AddonChartValue( + addon.Spec.Chart.HelmClusterAddonRepository, + addon.Spec.Chart.HelmClusterAddonChartName, + )} + }). Build() factory := func(_ utils.InternalRepositoryType) (repoclient.ClientInterface, error) { diff --git a/images/operator-helm-controller/internal/services/chart_service.go b/images/operator-helm-controller/internal/services/chart_service.go index 6b4bd52a..59f3c7b2 100644 --- a/images/operator-helm-controller/internal/services/chart_service.go +++ b/images/operator-helm-controller/internal/services/chart_service.go @@ -29,6 +29,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "sigs.k8s.io/controller-runtime/pkg/log" + "github.com/deckhouse/operator-helm/api/naming" helmv1alpha1 "github.com/deckhouse/operator-helm/api/v1alpha1" "github.com/deckhouse/operator-helm/internal/manager/status" "github.com/deckhouse/operator-helm/internal/utils" @@ -145,7 +146,7 @@ func applyHelmChartSpec(addon *helmv1alpha1.HelmClusterAddon, existing *sourcev1 existing.Labels[helmv1alpha1.LabelManagedBy] = helmv1alpha1.LabelManagedByValue existing.Labels[helmv1alpha1.HelmClusterAddonLabelSourceName] = addon.Name - existing.Labels[helmv1alpha1.HelmClusterAddonChartLabelSourceName] = utils.GetHelmClusterAddonChartName( + existing.Labels[helmv1alpha1.HelmClusterAddonChartLabelSourceName] = naming.HelmClusterAddonChartName( addon.Spec.Chart.HelmClusterAddonRepository, addon.Spec.Chart.HelmClusterAddonChartName, ) diff --git a/images/operator-helm-controller/internal/services/oci_repo_service.go b/images/operator-helm-controller/internal/services/oci_repo_service.go index ce6ccbb6..1a64b043 100644 --- a/images/operator-helm-controller/internal/services/oci_repo_service.go +++ b/images/operator-helm-controller/internal/services/oci_repo_service.go @@ -83,7 +83,12 @@ func (r OCIRepoResult) GetConditionType() string { return helmv1alpha1.ConditionTypeReady } -func (s *OCIRepoService) EnsureInternalOCIRepository(ctx context.Context, addon *helmv1alpha1.HelmClusterAddon, repo *helmv1alpha1.HelmClusterAddonRepository) OCIRepoResult { +func (s *OCIRepoService) EnsureInternalOCIRepository( + ctx context.Context, + addon *helmv1alpha1.HelmClusterAddon, + repo *helmv1alpha1.HelmClusterAddonRepository, + version *helmv1alpha1.HelmClusterAddonChartVersion, +) OCIRepoResult { logger := log.FromContext(ctx) existing := &sourcev1.OCIRepository{ @@ -94,7 +99,7 @@ func (s *OCIRepoService) EnsureInternalOCIRepository(ctx context.Context, addon } op, err := controllerutil.CreateOrPatch(ctx, s.Client, existing, func() error { - applyOCIRepositorySpec(addon, repo, existing) + applyOCIRepositorySpec(addon, repo, version.MediaType, existing) return nil }) @@ -117,6 +122,18 @@ func (s *OCIRepoService) EnsureInternalOCIRepository(ctx context.Context, addon existing.Status.Conditions, existing.Generation, addon, ociRepositoryErrorRules, ) + if version.UnavailableReason == helmv1alpha1.UnavailableReasonRemovedFromRepository && + processedStatus.Status != metav1.ConditionTrue { + // The version is still recorded — that is what keeps this addon reconcilable — + // but the repository no longer offers the tag, so the pull cannot succeed. Name + // that cause instead of leaving only the source controller's "not found". + processedStatus.Reason = helmv1alpha1.ReasonChartVersionRemoved + processedStatus.Message = fmt.Sprintf( + "Version %s is no longer offered by repository %s: %s", + version.Version, repo.Name, processedStatus.Message, + ) + } + return OCIRepoResult{ Artifact: existing.Status.Artifact, Status: processedStatus, @@ -167,7 +184,12 @@ func (s *OCIRepoService) RemoveOCIRepository(ctx context.Context, addon *helmv1a return ociRepo, nil } -func applyOCIRepositorySpec(addon *helmv1alpha1.HelmClusterAddon, repo *helmv1alpha1.HelmClusterAddonRepository, existing *sourcev1.OCIRepository) { +func applyOCIRepositorySpec( + addon *helmv1alpha1.HelmClusterAddon, + repo *helmv1alpha1.HelmClusterAddonRepository, + mediaType string, + existing *sourcev1.OCIRepository, +) { if repo.ForceReconcileRequired() { if existing.Annotations == nil { existing.Annotations = map[string]string{} @@ -198,8 +220,11 @@ func applyOCIRepositorySpec(addon *helmv1alpha1.HelmClusterAddon, repo *helmv1al } } + // The media type is the one recorded for this chart version by the repository + // synchronization: it differs between charts pushed by current and by older tooling. + // The caller guarantees it is non-empty. existing.Spec.LayerSelector = &sourcev1.OCILayerSelector{ - MediaType: "application/vnd.cncf.helm.chart.content.v1.tar+gzip", + MediaType: mediaType, Operation: "copy", } diff --git a/images/operator-helm-controller/internal/services/oci_repo_service_test.go b/images/operator-helm-controller/internal/services/oci_repo_service_test.go new file mode 100644 index 00000000..ee6cce40 --- /dev/null +++ b/images/operator-helm-controller/internal/services/oci_repo_service_test.go @@ -0,0 +1,182 @@ +/* +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 services + +import ( + "context" + "testing" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + + sourcev1 "github.com/werf/nelm-source-controller/api/v1" + + helmv1alpha1 "github.com/deckhouse/operator-helm/api/v1alpha1" + "github.com/deckhouse/operator-helm/internal/utils" +) + +func newOCIRepoService(t *testing.T, objects ...client.Object) (*OCIRepoService, client.Client) { + t.Helper() + + scheme := testScheme(t) + if err := sourcev1.AddToScheme(scheme); err != nil { + t.Fatalf("registering source scheme: %v", err) + } + + c := fake.NewClientBuilder().WithScheme(scheme).WithObjects(objects...).Build() + + return NewOCIRepoService(c, scheme, testNamespace), c +} + +func testAddon() *helmv1alpha1.HelmClusterAddon { + return &helmv1alpha1.HelmClusterAddon{ + ObjectMeta: metav1.ObjectMeta{Name: "consumer", Generation: 1}, + Spec: helmv1alpha1.HelmClusterAddonSpec{ + Namespace: "app", + Chart: helmv1alpha1.HelmClusterAddonChartRef{ + HelmClusterAddonRepository: "example", + HelmClusterAddonChartName: "podinfo", + Version: "6.7.1", + }, + }, + } +} + +func ociTestRepository() *helmv1alpha1.HelmClusterAddonRepository { + return &helmv1alpha1.HelmClusterAddonRepository{ + ObjectMeta: metav1.ObjectMeta{Name: "example", Generation: 1}, + Spec: helmv1alpha1.HelmClusterAddonRepositorySpec{URL: "oci://example.invalid/podinfo"}, + } +} + +func TestEnsureInternalOCIRepositoryUsesRecordedMediaType(t *testing.T) { + addon, repo := testAddon(), ociTestRepository() + service, c := newOCIRepoService(t, addon, repo) + + version := &helmv1alpha1.HelmClusterAddonChartVersion{ + Version: "6.7.1", + MediaType: "application/tar+gzip", + } + + service.EnsureInternalOCIRepository(context.Background(), addon, repo, version) + + ociRepo := &sourcev1.OCIRepository{} + key := client.ObjectKey{Name: utils.GetInternalOCIRepositoryName(addon.Name), Namespace: testNamespace} + if err := c.Get(context.Background(), key, ociRepo); err != nil { + t.Fatalf("oci repository was not created: %v", err) + } + + if ociRepo.Spec.LayerSelector == nil { + t.Fatal("layer selector must be set") + } + if ociRepo.Spec.LayerSelector.MediaType != "application/tar+gzip" { + t.Fatalf("layer media type is %q, want the recorded legacy one", ociRepo.Spec.LayerSelector.MediaType) + } +} + +func TestEnsureInternalOCIRepositoryReportsRemovedVersion(t *testing.T) { + addon, repo := testAddon(), ociTestRepository() + service, _ := newOCIRepoService(t, addon, repo) + + version := &helmv1alpha1.HelmClusterAddonChartVersion{ + Version: "6.7.1", + MediaType: "application/tar+gzip", + UnavailableReason: helmv1alpha1.UnavailableReasonRemovedFromRepository, + } + + result := service.EnsureInternalOCIRepository(context.Background(), addon, repo, version) + + if result.Status.Reason != helmv1alpha1.ReasonChartVersionRemoved { + t.Fatalf("reason is %q, want %q", result.Status.Reason, helmv1alpha1.ReasonChartVersionRemoved) + } + if result.Status.Message == "" { + t.Fatal("a removed version must be explained in the message") + } +} + +// TestEnsureInternalOCIRepositoryDoesNotRelabelReadyChildOnRemovedVersion covers the +// complement of TestEnsureInternalOCIRepositoryReportsRemovedVersion: a version can +// still carry UnavailableReasonRemovedFromRepository after the tag reappears (the +// marker is only dropped on the next synchronization), and by then the child +// OCIRepository may already be healthy again. The override must not fire in that +// case - a ready addon must not be relabeled with a failure reason. +// +// The internal object is seeded with the exact spec and labels applyOCIRepositorySpec +// writes (same trick as TestEnsureInternalHelmRepositoryStalledPrecedesReady in +// helm_repo_service_test.go): that makes CreateOrPatch a no-op, so the object's +// generation stays at 1 and the seeded Ready condition (ObservedGeneration: 1) counts +// as observed. +func TestEnsureInternalOCIRepositoryDoesNotRelabelReadyChildOnRemovedVersion(t *testing.T) { + addon, repo := testAddon(), ociTestRepository() + + version := &helmv1alpha1.HelmClusterAddonChartVersion{ + Version: "6.7.1", + MediaType: "application/tar+gzip", + UnavailableReason: helmv1alpha1.UnavailableReasonRemovedFromRepository, + } + + internal := &sourcev1.OCIRepository{ + ObjectMeta: metav1.ObjectMeta{ + Name: utils.GetInternalOCIRepositoryName(addon.Name), + Namespace: testNamespace, + Generation: 1, + Labels: map[string]string{ + helmv1alpha1.LabelManagedBy: helmv1alpha1.LabelManagedByValue, + helmv1alpha1.HelmClusterAddonLabelSourceName: addon.Name, + }, + }, + Spec: sourcev1.OCIRepositorySpec{ + URL: repo.Spec.URL, + Reference: &sourcev1.OCIRepositoryRef{Tag: addon.Spec.Chart.Version}, + Interval: metav1.Duration{Duration: InternalRepositoryInterval}, + LayerSelector: &sourcev1.OCILayerSelector{ + MediaType: version.MediaType, + Operation: "copy", + }, + }, + Status: sourcev1.OCIRepositoryStatus{ + Conditions: []metav1.Condition{ + { + Type: helmv1alpha1.ConditionTypeReady, + Status: metav1.ConditionTrue, + Reason: "Succeeded", + Message: "stored artifact for revision 6.7.1", + ObservedGeneration: 1, + LastTransitionTime: metav1.Now(), + }, + }, + }, + } + + service, _ := newOCIRepoService(t, addon, repo, internal) + + result := service.EnsureInternalOCIRepository(context.Background(), addon, repo, version) + + if result.Status.Status != metav1.ConditionTrue { + t.Fatalf("expected the ready child's status to be mirrored as True, got %v", result.Status.Status) + } + if result.Status.Reason == helmv1alpha1.ReasonChartVersionRemoved { + t.Fatalf("a ready child must not be relabeled with %q", helmv1alpha1.ReasonChartVersionRemoved) + } + if result.Status.Reason != "Succeeded" { + t.Fatalf("reason is %q, want the child's own %q untouched", result.Status.Reason, "Succeeded") + } + if result.Status.Message != "stored artifact for revision 6.7.1" { + t.Fatalf("message is %q, want the child's own message untouched", result.Status.Message) + } +} diff --git a/images/operator-helm-controller/internal/services/outcomes.go b/images/operator-helm-controller/internal/services/outcomes.go index ca6477da..deb756a8 100644 --- a/images/operator-helm-controller/internal/services/outcomes.go +++ b/images/operator-helm-controller/internal/services/outcomes.go @@ -28,12 +28,15 @@ type InternalRepositoryState struct { } // FetchOutcome is the result of reading the chart catalog from the remote -// repository. Terminal marks a failure that will not resolve by retrying. +// repository. Terminal marks a failure that will not resolve by retrying. Pending +// counts the chart versions that were listed but could not be examined in this pass: +// they are retried by the next synchronization and are not failures. type FetchOutcome struct { Err error Terminal bool Reason string Message string + Pending int } // CatalogOutcome is the result of writing the chart catalog into the cluster. @@ -49,8 +52,14 @@ type ConfigOutcome struct { Err error } -// SyncOutcome carries both phases of a synchronization attempt. +// SyncOutcome carries both phases of a synchronization attempt. FetchAttempted is +// true exactly when the registry was actually contacted (fetchCharts ran), on both +// its success and its failure path. It is false when a cluster-side read (such as +// knownCharts) failed before the fetch was ever issued: without this field the +// caller cannot tell that case apart from a zero-value, already-succeeded fetch, +// and would report a repository read failure as a read success. type SyncOutcome struct { - Fetch FetchOutcome - Catalog CatalogOutcome + FetchAttempted bool + Fetch FetchOutcome + Catalog CatalogOutcome } diff --git a/images/operator-helm-controller/internal/services/repo_sync_service.go b/images/operator-helm-controller/internal/services/repo_sync_service.go index 4ccf9744..ecd5aa37 100644 --- a/images/operator-helm-controller/internal/services/repo_sync_service.go +++ b/images/operator-helm-controller/internal/services/repo_sync_service.go @@ -19,8 +19,9 @@ package services import ( "context" "fmt" + "sort" - "github.com/samber/lo" + "github.com/Masterminds/semver/v3" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" @@ -29,8 +30,10 @@ import ( "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "sigs.k8s.io/controller-runtime/pkg/log" + "github.com/deckhouse/operator-helm/api/naming" helmv1alpha1 "github.com/deckhouse/operator-helm/api/v1alpha1" repoclient "github.com/deckhouse/operator-helm/internal/client/repository" + "github.com/deckhouse/operator-helm/internal/index" "github.com/deckhouse/operator-helm/internal/utils" ) @@ -75,18 +78,73 @@ func (s *RepoSyncService) Sync( repo *helmv1alpha1.HelmClusterAddonRepository, repoType utils.InternalRepositoryType, ) SyncOutcome { - charts, fetch := s.fetchCharts(ctx, repo, repoType) + known, err := s.knownCharts(ctx, repo) + if err != nil { + // The registry was never contacted: FetchAttempted stays false so the + // caller does not mistake this cluster-side read failure for a fetch that + // ran (let alone succeeded). + return SyncOutcome{Catalog: CatalogOutcome{Err: err}} + } + + charts, fetch := s.fetchCharts(ctx, repo, repoType, repoclient.FetchOptions{ + Known: known, + Full: repo.ForceReconcileRequired(), + }) if fetch.Err != nil { - return SyncOutcome{Fetch: fetch} + return SyncOutcome{FetchAttempted: true, Fetch: fetch} + } + + return SyncOutcome{FetchAttempted: true, Fetch: fetch, Catalog: s.reconcileCatalog(ctx, repo, charts)} +} + +// knownCharts collects the verdicts recorded by previous passes, so the client can +// skip the tags it has already examined. The chart objects are the only store of that +// state: keeping a separate fingerprint would be one more thing to drift. +func (s *RepoSyncService) knownCharts( + ctx context.Context, + repo *helmv1alpha1.HelmClusterAddonRepository, +) (repoclient.KnownCharts, error) { + var charts helmv1alpha1.HelmClusterAddonChartList + if err := s.Client.List(ctx, &charts, client.MatchingLabels{LabelRepositoryName: repo.Name}); err != nil { + return nil, fmt.Errorf("listing charts of repository %q: %w", repo.Name, err) + } + + logger := log.FromContext(ctx) + known := make(repoclient.KnownCharts, len(charts.Items)) + + for _, chart := range charts.Items { + chartName := chart.Labels[LabelChartName] + if chartName == "" { + // The chart label is the only way back from the object name (a + // truncated hash) to the chart name it belongs to. Without it the + // recorded verdicts for this chart cannot be looked up here, so every + // tag is re-examined on the next fetch; that is safe but not free, so + // it is worth surfacing. + logger.Info("Chart object has no chart label, dropping its recorded verdicts", "addonChartName", chart.Name) + + continue + } + + versions := make(repoclient.KnownVersions, len(chart.Status.Versions)) + for _, version := range chart.Status.Versions { + versions[version.Version] = repoclient.KnownVersion{ + MediaType: version.MediaType, + UnavailableReason: version.UnavailableReason, + UnavailableMessage: version.UnavailableMessage, + } + } + + known[chartName] = versions } - return SyncOutcome{Fetch: fetch, Catalog: s.reconcileCatalog(ctx, repo, charts)} + return known, nil } func (s *RepoSyncService) fetchCharts( ctx context.Context, repo *helmv1alpha1.HelmClusterAddonRepository, repoType utils.InternalRepositoryType, + opts repoclient.FetchOptions, ) ([]repoclient.Chart, FetchOutcome) { repoClient, err := s.clientFactory(repoType) if err != nil { @@ -98,9 +156,9 @@ func (s *RepoSyncService) fetchCharts( } } - charts, err := repoClient.FetchCharts(ctx, repo.Spec.URL, buildRepoConfig(repo)) + charts, err := repoClient.FetchCharts(ctx, repo.Spec.URL, buildRepoConfig(repo), opts) if err == nil { - return charts, FetchOutcome{} + return charts, FetchOutcome{Pending: countPending(charts)} } if terminal, ok := repoclient.AsTerminal(err); ok { @@ -119,6 +177,20 @@ func (s *RepoSyncService) fetchCharts( } } +// countPending counts the versions that were listed but not examined in this pass. +func countPending(charts []repoclient.Chart) int { + pending := 0 + for _, chart := range charts { + for _, version := range chart.Versions { + if version.UnavailableReason == helmv1alpha1.UnavailableReasonResolvePending { + pending++ + } + } + } + + return pending +} + func buildRepoConfig(repo *helmv1alpha1.HelmClusterAddonRepository) *repoclient.RepoConfig { if repo.Spec.Auth == nil && repo.Spec.CACertificate == "" && !repo.Spec.InsecureSkipVerify { return nil @@ -147,11 +219,10 @@ func (s *RepoSyncService) reconcileCatalog( desiredCharts := make(map[string]struct{}, len(charts)) for _, chart := range charts { - if len(chart.Versions) == 0 { - continue - } - - addonChartName := utils.GetHelmClusterAddonChartName(repo.Name, chart.Name) + addonChartName := naming.HelmClusterAddonChartName(repo.Name, chart.Name) + // A chart with no usable version is still created: it carries the reason each of + // its versions is unusable, and skipping it here would let the pruning loop below + // delete a chart whose tags merely failed to resolve. existing := &helmv1alpha1.HelmClusterAddonChart{ ObjectMeta: metav1.ObjectMeta{Name: addonChartName}, } @@ -185,12 +256,17 @@ func (s *RepoSyncService) reconcileCatalog( logger.Info("Reconciled HelmClusterAddonChart", "operation", op, "addonChartName", addonChartName) } + inUse, err := s.inUseVersions(ctx, repo.Name, chart.Name) + if err != nil { + return CatalogOutcome{Err: err} + } + base := existing.DeepCopy() - existing.Status.IconURL = chart.Versions[0].IconURL - existing.Status.Versions = lo.Map(chart.Versions, func(v repoclient.ChartVersion, _ int) helmv1alpha1.HelmClusterAddonChartVersion { - return helmv1alpha1.HelmClusterAddonChartVersion{Version: v.Version.Original()} - }) + if len(chart.Versions) > 0 { + existing.Status.IconURL = chart.Versions[0].IconURL + } + existing.Status.Versions = mergeChartVersions(chart.Versions, existing.Status.Versions, inUse) if err := s.Client.Status().Patch(ctx, existing, client.MergeFrom(base)); err != nil { return CatalogOutcome{Err: fmt.Errorf("updating versions of chart %q: %w", addonChartName, err)} @@ -207,6 +283,29 @@ func (s *RepoSyncService) reconcileCatalog( continue } + chartName := chart.Labels[LabelChartName] + if chartName == "" { + // The chart label is the only way back from the object name (a + // truncated hash) to the chart name an addon references, so + // inUseVersions cannot find anything to protect and this chart is + // pruned even if an addon still uses it. That fail-open is unavoidable + // as written, so at least make it diagnosable. + logger.Info("Pruning a chart with no chart label; in-use protection could not be checked", "addonChartName", chart.Name) + } + + inUse, err := s.inUseVersions(ctx, repo.Name, chartName) + if err != nil { + return CatalogOutcome{Err: err} + } + if len(inUse) > 0 { + // An addon still references this chart: deleting the object would make the + // addon's own reconciliation fail on a missing chart and block every change + // to it, including its removal. + logger.Info("Keeping a chart referenced by an addon", "addonChartName", chart.Name) + + continue + } + if err := s.ensureResourceDeleted(ctx, types.NamespacedName{Name: chart.Name}, &chart); err != nil { return CatalogOutcome{Err: fmt.Errorf("deleting stale charts: %w", err)} } @@ -214,3 +313,134 @@ func (s *RepoSyncService) reconcileCatalog( return CatalogOutcome{} } + +// inUseVersions returns the chart versions referenced by the addon that uses this +// repository/chart pair. The webhook and the claim Lease enforce one addon per pair, +// so at most one is found; both its desired and its last applied version count, since +// they differ during an upgrade. +func (s *RepoSyncService) inUseVersions(ctx context.Context, repoName, chartName string) (map[string]struct{}, error) { + if chartName == "" { + return nil, nil + } + + var addons helmv1alpha1.HelmClusterAddonList + if err := s.Client.List(ctx, &addons, client.MatchingFields{ + index.AddonChart: index.AddonChartValue(repoName, chartName), + }); err != nil { + return nil, fmt.Errorf("listing addons of chart %q: %w", chartName, err) + } + + inUse := make(map[string]struct{}, 2) + + for _, addon := range addons.Items { + inUse[addon.Spec.Chart.Version] = struct{}{} + + // LastAppliedChart carries its own repository/chart identity and can lag + // behind Spec.Chart when an addon is switched to a different chart: only + // credit it here when it still names this repository/chart pair, or a + // stale entry would protect a phantom version on the new chart while no + // longer protecting the version actually applied on the old one. + if last := addon.Status.LastAppliedChart; last != nil && + last.HelmClusterAddonChartName == chartName && last.HelmClusterAddonRepository == repoName { + inUse[last.Version] = struct{}{} + } + } + + return inUse, nil +} + +// mergeChartVersions builds the desired version list from the fetched entries and the +// ones already recorded. A recorded version the registry no longer lists is dropped, +// unless an addon still references it: then it is retained with RemovedFromRepository +// and keeps its media type, without which the addon's internal OCIRepository could not +// be built at all. +// +// The same protection applies to a version that is still listed but whose tag was +// re-pushed as a non-chart artifact: the fresh verdict carries no media type, but if an +// addon still references the version, its previously recorded media type is carried +// forward alongside the fresh UnsupportedMediaType reason and message. Without the old +// media type the internal OCIRepository could not be built at all, which would block +// every change to the running addon (values, maintenance mode, ...) rather than just +// the pull that the new artifact actually breaks; the real pull failure is reported by +// the source controller instead. +func mergeChartVersions( + fetched []repoclient.ChartVersion, + current []helmv1alpha1.HelmClusterAddonChartVersion, + inUse map[string]struct{}, +) []helmv1alpha1.HelmClusterAddonChartVersion { + merged := make([]helmv1alpha1.HelmClusterAddonChartVersion, 0, len(fetched)+len(current)) + listed := make(map[string]struct{}, len(fetched)) + + currentByVersion := make(map[string]helmv1alpha1.HelmClusterAddonChartVersion, len(current)) + for _, version := range current { + currentByVersion[version.Version] = version + } + + for _, version := range fetched { + name := version.Version.Original() + listed[name] = struct{}{} + + mediaType := version.MediaType + if mediaType == "" { + if _, referenced := inUse[name]; referenced { + if old, recorded := currentByVersion[name]; recorded && old.MediaType != "" { + mediaType = old.MediaType + } + } + } + + merged = append(merged, helmv1alpha1.HelmClusterAddonChartVersion{ + Version: name, + MediaType: mediaType, + UnavailableReason: version.UnavailableReason, + UnavailableMessage: version.UnavailableMessage, + }) + } + + for _, version := range current { + if _, stillListed := listed[version.Version]; stillListed { + continue + } + if _, referenced := inUse[version.Version]; !referenced { + continue + } + + version.UnavailableReason = helmv1alpha1.UnavailableReasonRemovedFromRepository + version.UnavailableMessage = "the repository no longer offers this version" + merged = append(merged, version) + } + + sortChartVersions(merged) + + return merged +} + +// sortChartVersions orders versions by descending semver, breaking ties by a reverse +// string comparison. A version that does not parse as semver sorts after every +// version that does, ordered among themselves by the same reverse string comparison. +// Parsability has to be the primary key: comparing a parsable and an unparsable +// version by semver on one pair and by string on another can produce a cycle (e.g. +// "6.10.0" > "6.9.0" by semver, "6.9.0" > "6.5.x" and "6.5.x" > "6.10.0" by string), +// which is not a valid ordering for sort.SliceStable. Today's clients never write an +// unparsable version, but legacy status data can still carry one, and the order has to +// be deterministic regardless: the merge goes through maps, and an unstable order +// would produce a status patch on every synchronization for a catalog that did not +// change. +func sortChartVersions(versions []helmv1alpha1.HelmClusterAddonChartVersion) { + sort.SliceStable(versions, func(i, j int) bool { + left, leftErr := semver.NewVersion(versions[i].Version) + right, rightErr := semver.NewVersion(versions[j].Version) + + leftParses, rightParses := leftErr == nil, rightErr == nil + + if leftParses != rightParses { + return leftParses + } + + if leftParses && !left.Equal(right) { + return left.GreaterThan(right) + } + + return versions[i].Version > versions[j].Version + }) +} diff --git a/images/operator-helm-controller/internal/services/repo_sync_service_test.go b/images/operator-helm-controller/internal/services/repo_sync_service_test.go index fcbb27e8..3efccee5 100644 --- a/images/operator-helm-controller/internal/services/repo_sync_service_test.go +++ b/images/operator-helm-controller/internal/services/repo_sync_service_test.go @@ -25,9 +25,12 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" + "github.com/deckhouse/operator-helm/api/naming" helmv1alpha1 "github.com/deckhouse/operator-helm/api/v1alpha1" repoclient "github.com/deckhouse/operator-helm/internal/client/repository" + "github.com/deckhouse/operator-helm/internal/index" "github.com/deckhouse/operator-helm/internal/utils" ) @@ -36,10 +39,21 @@ type stubRepoClient struct { err error } -func (s stubRepoClient) FetchCharts(_ context.Context, _ string, _ *repoclient.RepoConfig) ([]repoclient.Chart, error) { +func (s stubRepoClient) FetchCharts(_ context.Context, _ string, _ *repoclient.RepoConfig, _ repoclient.FetchOptions) ([]repoclient.Chart, error) { return s.charts, s.err } +type recordingRepoClient struct { + charts []repoclient.Chart + opts repoclient.FetchOptions +} + +func (s *recordingRepoClient) FetchCharts(_ context.Context, _ string, _ *repoclient.RepoConfig, opts repoclient.FetchOptions) ([]repoclient.Chart, error) { + s.opts = opts + + return s.charts, nil +} + func newRepoSyncService(t *testing.T, stub stubRepoClient, objects ...client.Object) (*RepoSyncService, client.Client) { t.Helper() @@ -48,6 +62,14 @@ func newRepoSyncService(t *testing.T, stub stubRepoClient, objects ...client.Obj WithScheme(scheme). WithObjects(objects...). WithStatusSubresource(&helmv1alpha1.HelmClusterAddonChart{}, &helmv1alpha1.HelmClusterAddonRepository{}). + WithIndex(&helmv1alpha1.HelmClusterAddon{}, index.AddonChart, func(obj client.Object) []string { + addon := obj.(*helmv1alpha1.HelmClusterAddon) + + return []string{index.AddonChartValue( + addon.Spec.Chart.HelmClusterAddonRepository, + addon.Spec.Chart.HelmClusterAddonChartName, + )} + }). Build() factory := func(_ utils.InternalRepositoryType) (repoclient.ClientInterface, error) { @@ -57,6 +79,46 @@ func newRepoSyncService(t *testing.T, stub stubRepoClient, objects ...client.Obj return NewRepoSyncService(c, scheme, factory), c } +func ociVersion(version, mediaType string) repoclient.ChartVersion { + return repoclient.ChartVersion{Version: semver.MustParse(version), MediaType: mediaType} +} + +func existingChart(repoName, chartName string, versions ...helmv1alpha1.HelmClusterAddonChartVersion) *helmv1alpha1.HelmClusterAddonChart { + return &helmv1alpha1.HelmClusterAddonChart{ + ObjectMeta: metav1.ObjectMeta{ + Name: naming.HelmClusterAddonChartName(repoName, chartName), + Labels: map[string]string{LabelRepositoryName: repoName, LabelChartName: chartName}, + }, + Status: helmv1alpha1.HelmClusterAddonChartStatus{Versions: versions}, + } +} + +func addonUsing(repoName, chartName, version string) *helmv1alpha1.HelmClusterAddon { + return &helmv1alpha1.HelmClusterAddon{ + ObjectMeta: metav1.ObjectMeta{Name: "consumer"}, + Spec: helmv1alpha1.HelmClusterAddonSpec{ + Namespace: "app", + Chart: helmv1alpha1.HelmClusterAddonChartRef{ + HelmClusterAddonRepository: repoName, + HelmClusterAddonChartName: chartName, + Version: version, + }, + }, + } +} + +func chartStatus(t *testing.T, c client.Client, repoName, chartName string) helmv1alpha1.HelmClusterAddonChartStatus { + t.Helper() + + chart := &helmv1alpha1.HelmClusterAddonChart{} + key := client.ObjectKey{Name: naming.HelmClusterAddonChartName(repoName, chartName)} + if err := c.Get(context.Background(), key, chart); err != nil { + t.Fatalf("getting chart: %v", err) + } + + return chart.Status +} + func chartFixture(name, version string) repoclient.Chart { return repoclient.Chart{ Name: name, @@ -77,7 +139,7 @@ func TestSyncCreatesChartsAndRecordsVersions(t *testing.T) { } chart := &helmv1alpha1.HelmClusterAddonChart{} - key := client.ObjectKey{Name: utils.GetHelmClusterAddonChartName(repo.Name, "podinfo")} + key := client.ObjectKey{Name: naming.HelmClusterAddonChartName(repo.Name, "podinfo")} if err := c.Get(context.Background(), key, chart); err != nil { t.Fatalf("chart was not created: %v", err) } @@ -90,7 +152,7 @@ func TestSyncPrunesStaleCharts(t *testing.T) { repo := testRepository() stale := &helmv1alpha1.HelmClusterAddonChart{ ObjectMeta: metav1.ObjectMeta{ - Name: utils.GetHelmClusterAddonChartName(repo.Name, "removed"), + Name: naming.HelmClusterAddonChartName(repo.Name, "removed"), Labels: map[string]string{LabelRepositoryName: repo.Name, LabelChartName: "removed"}, }, } @@ -144,3 +206,292 @@ func TestSyncReportsTransientFetchFailure(t *testing.T) { t.Fatalf("fetch reason is %q, want %q", outcome.Fetch.Reason, helmv1alpha1.ReasonSyncFailed) } } + +func TestSyncPassesKnownVersionsToTheClient(t *testing.T) { + repo := testRepository() + chart := existingChart(repo.Name, "podinfo", + helmv1alpha1.HelmClusterAddonChartVersion{Version: "6.7.1", MediaType: "application/tar+gzip"}, + helmv1alpha1.HelmClusterAddonChartVersion{ + Version: "6.7.2", + UnavailableReason: helmv1alpha1.UnavailableReasonUnsupportedMediaType, + UnavailableMessage: "layer media type application/vnd.example is not a chart", + }, + ) + + stub := &recordingRepoClient{charts: []repoclient.Chart{{ + Name: "podinfo", + Versions: []repoclient.ChartVersion{ociVersion("6.7.1", "application/tar+gzip")}, + }}} + + service, _ := newRepoSyncService(t, stubRepoClient{}, repo, chart) + service.clientFactory = func(_ utils.InternalRepositoryType) (repoclient.ClientInterface, error) { + return stub, nil + } + + if outcome := service.Sync(context.Background(), repo, utils.InternalOCIRepository); outcome.Fetch.Err != nil { + t.Fatalf("fetch failed: %v", outcome.Fetch.Err) + } + + known := stub.opts.Known["podinfo"] + if known["6.7.1"].MediaType != "application/tar+gzip" { + t.Fatalf("known media type is %q", known["6.7.1"].MediaType) + } + if known["6.7.2"].UnavailableReason != helmv1alpha1.UnavailableReasonUnsupportedMediaType { + t.Fatalf("known reason is %q", known["6.7.2"].UnavailableReason) + } + if known["6.7.2"].UnavailableMessage != "layer media type application/vnd.example is not a chart" { + t.Fatalf("known message is %q", known["6.7.2"].UnavailableMessage) + } + if stub.opts.Full { + t.Fatal("a normal pass must not request a full re-index") + } +} + +func TestSyncRequestsFullPassOnForceReconcile(t *testing.T) { + repo := testRepository() + repo.Annotations = map[string]string{helmv1alpha1.AnnotationForceReconcile: ""} + + stub := &recordingRepoClient{charts: []repoclient.Chart{{ + Name: "podinfo", + Versions: []repoclient.ChartVersion{ociVersion("6.7.1", "application/tar+gzip")}, + }}} + + service, _ := newRepoSyncService(t, stubRepoClient{}, repo) + service.clientFactory = func(_ utils.InternalRepositoryType) (repoclient.ClientInterface, error) { + return stub, nil + } + + service.Sync(context.Background(), repo, utils.InternalOCIRepository) + + if !stub.opts.Full { + t.Fatal("force reconcile must request a full re-index") + } +} + +func TestSyncRetainsReferencedVersionRemovedFromRepository(t *testing.T) { + repo := testRepository() + chart := existingChart(repo.Name, "podinfo", + helmv1alpha1.HelmClusterAddonChartVersion{Version: "6.7.1", MediaType: "application/tar+gzip"}, + helmv1alpha1.HelmClusterAddonChartVersion{Version: "6.7.0", MediaType: "application/tar+gzip"}, + ) + addon := addonUsing(repo.Name, "podinfo", "6.7.1") + + stub := stubRepoClient{charts: []repoclient.Chart{{ + Name: "podinfo", + Versions: []repoclient.ChartVersion{ociVersion("6.8.0", "application/tar+gzip")}, + }}} + + service, c := newRepoSyncService(t, stub, repo, chart, addon) + if outcome := service.Sync(context.Background(), repo, utils.InternalOCIRepository); outcome.Catalog.Err != nil { + t.Fatalf("catalog update failed: %v", outcome.Catalog.Err) + } + + status := chartStatus(t, c, repo.Name, "podinfo") + + var retained, pruned bool + for _, version := range status.Versions { + switch version.Version { + case "6.7.1": + retained = true + if version.UnavailableReason != helmv1alpha1.UnavailableReasonRemovedFromRepository { + t.Fatalf("retained version reason is %q", version.UnavailableReason) + } + if version.MediaType != "application/tar+gzip" { + t.Fatalf("retained version lost its media type: %q", version.MediaType) + } + case "6.7.0": + pruned = true + } + } + + if !retained { + t.Fatal("a referenced version must be retained") + } + if pruned { + t.Fatal("an unreferenced version must be pruned") + } +} + +// TestSyncRetainsMediaTypeForReferencedUnsupportedVersion covers D4's fourth row: a +// version whose tag was re-pushed as a non-chart artifact must keep its previously +// recorded media type as long as an addon still references it, so the addon's internal +// OCIRepository can still be built and the pull failure surfaces from the source +// controller instead of the deploy gate. The same version, unreferenced, must not carry +// the old media type forward: retention is scoped to in-use versions only. +func TestSyncRetainsMediaTypeForReferencedUnsupportedVersion(t *testing.T) { + repo := testRepository() + chart := existingChart(repo.Name, "podinfo", + helmv1alpha1.HelmClusterAddonChartVersion{Version: "6.7.1", MediaType: "application/tar+gzip"}, + helmv1alpha1.HelmClusterAddonChartVersion{Version: "6.7.0", MediaType: "application/tar+gzip"}, + ) + addon := addonUsing(repo.Name, "podinfo", "6.7.1") + + stub := stubRepoClient{charts: []repoclient.Chart{{ + Name: "podinfo", + Versions: []repoclient.ChartVersion{ + { + Version: semver.MustParse("6.7.1"), + UnavailableReason: helmv1alpha1.UnavailableReasonUnsupportedMediaType, + UnavailableMessage: "layer media type \"application/vnd.oci.image.layer.v1.tar\" is not a helm chart layer", + }, + { + Version: semver.MustParse("6.7.0"), + UnavailableReason: helmv1alpha1.UnavailableReasonUnsupportedMediaType, + UnavailableMessage: "layer media type \"application/vnd.oci.image.layer.v1.tar\" is not a helm chart layer", + }, + }, + }}} + + service, c := newRepoSyncService(t, stub, repo, chart, addon) + if outcome := service.Sync(context.Background(), repo, utils.InternalOCIRepository); outcome.Catalog.Err != nil { + t.Fatalf("catalog update failed: %v", outcome.Catalog.Err) + } + + status := chartStatus(t, c, repo.Name, "podinfo") + + var sawReferenced, sawUnreferenced bool + for _, version := range status.Versions { + switch version.Version { + case "6.7.1": + sawReferenced = true + if version.UnavailableReason != helmv1alpha1.UnavailableReasonUnsupportedMediaType { + t.Fatalf("referenced version reason is %q, want UnsupportedMediaType", version.UnavailableReason) + } + if version.MediaType != "application/tar+gzip" { + t.Fatalf("referenced version lost its media type: %q", version.MediaType) + } + case "6.7.0": + sawUnreferenced = true + if version.MediaType != "" { + t.Fatalf("unreferenced version must not carry its old media type forward: %q", version.MediaType) + } + if version.UnavailableReason != helmv1alpha1.UnavailableReasonUnsupportedMediaType { + t.Fatalf("unreferenced version reason is %q, want UnsupportedMediaType", version.UnavailableReason) + } + } + } + + if !sawReferenced { + t.Fatal("referenced version 6.7.1 must be present") + } + if !sawUnreferenced { + t.Fatal("unreferenced version 6.7.0 must be present") + } +} + +func TestSyncOrdersVersionsBySemverDescending(t *testing.T) { + repo := testRepository() + stub := stubRepoClient{charts: []repoclient.Chart{{ + Name: "podinfo", + Versions: []repoclient.ChartVersion{ + ociVersion("6.7.1", "application/tar+gzip"), + ociVersion("6.10.0", "application/tar+gzip"), + ociVersion("6.8.0", "application/tar+gzip"), + }, + }}} + + service, c := newRepoSyncService(t, stub, repo) + service.Sync(context.Background(), repo, utils.InternalOCIRepository) + + got := chartStatus(t, c, repo.Name, "podinfo").Versions + want := []string{"6.10.0", "6.8.0", "6.7.1"} + if len(got) != len(want) { + t.Fatalf("versions are %v, want %v", got, want) + } + for i, version := range want { + if got[i].Version != version { + t.Fatalf("versions are %v, want %v", got, want) + } + } +} + +func TestSyncCreatesChartWithNoUsableVersions(t *testing.T) { + repo := testRepository() + stub := stubRepoClient{charts: []repoclient.Chart{{ + Name: "podinfo", + Versions: []repoclient.ChartVersion{{ + Version: semver.MustParse("6.7.1"), + UnavailableReason: helmv1alpha1.UnavailableReasonResolvePending, + UnavailableMessage: "registry returned 500", + }}, + }}} + + service, c := newRepoSyncService(t, stub, repo) + outcome := service.Sync(context.Background(), repo, utils.InternalOCIRepository) + + if outcome.Catalog.Err != nil { + t.Fatalf("catalog update failed: %v", outcome.Catalog.Err) + } + if outcome.Fetch.Pending != 1 { + t.Fatalf("pending count is %d, want 1", outcome.Fetch.Pending) + } + + status := chartStatus(t, c, repo.Name, "podinfo") + if len(status.Versions) != 1 || status.Versions[0].UnavailableReason != helmv1alpha1.UnavailableReasonResolvePending { + t.Fatalf("chart status is %+v", status.Versions) + } +} + +func TestSyncKeepsChartReferencedByAddon(t *testing.T) { + repo := testRepository() + chart := existingChart(repo.Name, "podinfo", + helmv1alpha1.HelmClusterAddonChartVersion{Version: "6.7.1", MediaType: "application/tar+gzip"}, + ) + addon := addonUsing(repo.Name, "podinfo", "6.7.1") + + service, c := newRepoSyncService(t, stubRepoClient{charts: nil}, repo, chart, addon) + if outcome := service.Sync(context.Background(), repo, utils.InternalOCIRepository); outcome.Catalog.Err != nil { + t.Fatalf("catalog update failed: %v", outcome.Catalog.Err) + } + + key := client.ObjectKey{Name: naming.HelmClusterAddonChartName(repo.Name, "podinfo")} + if err := c.Get(context.Background(), key, &helmv1alpha1.HelmClusterAddonChart{}); err != nil { + t.Fatalf("a chart referenced by an addon must not be deleted: %v", err) + } +} + +// TestSyncReportsNoFetchAttemptOnClusterReadFailure covers the knownCharts failure +// path: the registry is never contacted, so the outcome must say so. Reporting +// FetchAttempted: true here (or a zero-value Fetch with Err == nil) would make a +// cluster-side read failure look like a successful repository fetch to a caller +// that only checks Fetch.Err. +func TestSyncReportsNoFetchAttemptOnClusterReadFailure(t *testing.T) { + repo := testRepository() + scheme := testScheme(t) + + sentinel := errors.New("synthetic chart list failure") + c := fake.NewClientBuilder(). + WithScheme(scheme). + WithObjects(repo). + WithStatusSubresource(&helmv1alpha1.HelmClusterAddonChart{}, &helmv1alpha1.HelmClusterAddonRepository{}). + WithInterceptorFuncs(interceptor.Funcs{ + List: func(ctx context.Context, wc client.WithWatch, list client.ObjectList, opts ...client.ListOption) error { + if _, ok := list.(*helmv1alpha1.HelmClusterAddonChartList); ok { + return sentinel + } + + return wc.List(ctx, list, opts...) + }, + }). + Build() + + factory := func(_ utils.InternalRepositoryType) (repoclient.ClientInterface, error) { + return stubRepoClient{}, nil + } + service := NewRepoSyncService(c, scheme, factory) + + outcome := service.Sync(context.Background(), repo, utils.InternalOCIRepository) + + if outcome.FetchAttempted { + t.Fatal("a cluster-side read failure before the fetch must not report FetchAttempted") + } + if outcome.Fetch.Err != nil { + t.Fatalf("Fetch must stay zero-valued on this path, got Err: %v", outcome.Fetch.Err) + } + if outcome.Catalog.Err == nil { + t.Fatal("expected a catalog error from the failed chart listing") + } + if !errors.Is(outcome.Catalog.Err, sentinel) { + t.Fatalf("catalog error must wrap the underlying failure, got %v", outcome.Catalog.Err) + } +} diff --git a/images/operator-helm-controller/internal/utils/mapper.go b/images/operator-helm-controller/internal/utils/mapper.go index 01255cd5..c33a4f70 100644 --- a/images/operator-helm-controller/internal/utils/mapper.go +++ b/images/operator-helm-controller/internal/utils/mapper.go @@ -20,13 +20,13 @@ import ( "context" "k8s.io/apimachinery/pkg/types" - ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/handler" "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/reconcile" helmv1alpha1 "github.com/deckhouse/operator-helm/api/v1alpha1" + "github.com/deckhouse/operator-helm/internal/index" ) func MapInternalResources(controllerName, targetNamespace, labelManagedBy, labelManagedByValue, labelSourceName string) handler.MapFunc { @@ -61,24 +61,10 @@ func MapInternalResources(controllerName, targetNamespace, labelManagedBy, label } } -const AddonRepositoryIndex = ".spec.chart.helmClusterAddonRepository" - -func SetupAddonRepositoryIndex(mgr ctrl.Manager) error { - return mgr.GetFieldIndexer().IndexField(context.Background(), &helmv1alpha1.HelmClusterAddon{}, AddonRepositoryIndex, - func(obj client.Object) []string { - addon := obj.(*helmv1alpha1.HelmClusterAddon) - if addon.Spec.Chart.HelmClusterAddonRepository == "" { - return nil - } - return []string{addon.Spec.Chart.HelmClusterAddonRepository} - }, - ) -} - func MapRepositoryToAddons(c client.Client) handler.MapFunc { return func(ctx context.Context, obj client.Object) []reconcile.Request { addonList := &helmv1alpha1.HelmClusterAddonList{} - if err := c.List(ctx, addonList, client.MatchingFields{AddonRepositoryIndex: obj.GetName()}); err != nil { + if err := c.List(ctx, addonList, client.MatchingFields{index.AddonRepository: obj.GetName()}); err != nil { log.FromContext(ctx).Error(err, "Failed to list HelmClusterAddons for repository mapping") return nil } diff --git a/images/operator-helm-controller/internal/utils/name.go b/images/operator-helm-controller/internal/utils/name.go index 29b4e132..12edbc56 100644 --- a/images/operator-helm-controller/internal/utils/name.go +++ b/images/operator-helm-controller/internal/utils/name.go @@ -67,28 +67,6 @@ func GetInternalRepositoryTLSSecretName(internalRepoName string) string { return strings.TrimRight(result, "-") + postfix } -func GetHelmClusterAddonChartName(repoName, chartName string) string { - hash := GetHash(fmt.Sprintf("%s-chart-%s", repoName, chartName)) - - var result, postfix string - - if len(repoName) > 20 { - result += repoName[:20] + "-chart-" - postfix = "-" + hash - } else { - result += repoName + "-chart-" - } - - if len(chartName) > 20 { - result += chartName[:20] - postfix = "-" + hash - } else { - result += chartName - } - - return strings.TrimRight(result, "-") + postfix -} - func GetInternalHelmReleaseName(addonName string) string { prefix := "hca" hash := GetHash(fmt.Sprintf("%s-%s", prefix, addonName)) diff --git a/images/operator-helm-controller/internal/webhook/helmclusteraddon/webhook.go b/images/operator-helm-controller/internal/webhook/helmclusteraddon/webhook.go index ccb605e8..4bb12afd 100644 --- a/images/operator-helm-controller/internal/webhook/helmclusteraddon/webhook.go +++ b/images/operator-helm-controller/internal/webhook/helmclusteraddon/webhook.go @@ -26,26 +26,15 @@ import ( "sigs.k8s.io/controller-runtime/pkg/webhook/admission" helmv1alpha1 "github.com/deckhouse/operator-helm/api/v1alpha1" + "github.com/deckhouse/operator-helm/internal/index" "github.com/deckhouse/operator-helm/internal/services" "github.com/deckhouse/operator-helm/internal/utils" ) -const addonChartIndex = ".spec.chart.repoAndChart" - var uniquenessBypassUsernames = []string{ "system:serviceaccount:d8-operator-helm:operator-helm-controller", } -func SetupIndexes(mgr ctrl.Manager) error { - return mgr.GetFieldIndexer().IndexField( - context.Background(), &helmv1alpha1.HelmClusterAddon{}, addonChartIndex, - func(obj client.Object) []string { - addon := obj.(*helmv1alpha1.HelmClusterAddon) - return []string{addon.Spec.Chart.HelmClusterAddonRepository + "/" + addon.Spec.Chart.HelmClusterAddonChartName} - }, - ) -} - func SetupWebhookWithManager(mgr ctrl.Manager) error { return ctrl.NewWebhookManagedBy(mgr, &helmv1alpha1.HelmClusterAddon{}). WithValidator(&HelmClusterAddonWebhookValidator{ @@ -134,9 +123,9 @@ func (v *HelmClusterAddonWebhookValidator) checkUniqueness(ctx context.Context, } list := &helmv1alpha1.HelmClusterAddonList{} - indexValue := addon.Spec.Chart.HelmClusterAddonRepository + "/" + addon.Spec.Chart.HelmClusterAddonChartName + indexValue := index.AddonChartValue(addon.Spec.Chart.HelmClusterAddonRepository, addon.Spec.Chart.HelmClusterAddonChartName) - if err := v.Client.List(ctx, list, client.MatchingFields{addonChartIndex: indexValue}); err != nil { + if err := v.Client.List(ctx, list, client.MatchingFields{index.AddonChart: indexValue}); err != nil { return err } diff --git a/templates/chart-values-controller/rbac-for-us.yaml b/templates/chart-values-controller/rbac-for-us.yaml index c447991f..06f859b8 100644 --- a/templates/chart-values-controller/rbac-for-us.yaml +++ b/templates/chart-values-controller/rbac-for-us.yaml @@ -60,6 +60,8 @@ rules: resources: - helmclusteraddonrepositories - helmclusteraddonrepositories/status + - helmclusteraddoncharts + - helmclusteraddoncharts/status verbs: - get - list diff --git a/templates/operator-helm-controller/deployment.yaml b/templates/operator-helm-controller/deployment.yaml index ee259811..41a92b93 100644 --- a/templates/operator-helm-controller/deployment.yaml +++ b/templates/operator-helm-controller/deployment.yaml @@ -71,6 +71,7 @@ spec: app: operator-helm-controller annotations: kubectl.kubernetes.io/default-container: operator-helm-controller + checksum/cert: {{ printf "%s%s" .Values.operatorHelm.internal.controller.cert.ca .Values.operatorHelm.internal.controller.cert.crt | sha256sum }} spec: containers: {{- include "kube_api_rewriter.sidecar_container" . | nindent 8 }} diff --git a/tests/e2e/helmclusteraddonrepository/lifecycle.go b/tests/e2e/helmclusteraddonrepository/lifecycle.go index d13fde19..588e8d45 100644 --- a/tests/e2e/helmclusteraddonrepository/lifecycle.go +++ b/tests/e2e/helmclusteraddonrepository/lifecycle.go @@ -114,7 +114,13 @@ func DefineLifecycleTests(repoType, repoURL string) { By("HelmClusterAddonChart should have versions") for _, chart := range charts.Items { - Expect(chart.Status.Versions).NotTo(BeEmpty()) + usable := 0 + for _, version := range chart.Status.Versions { + if version.UnavailableReason == "" { + usable++ + } + } + Expect(usable).To(BeNumerically(">", 0), "chart must expose at least one usable version") } }) }) diff --git a/tests/e2e/internal/util/moduleconfig.go b/tests/e2e/internal/util/moduleconfig.go index dfab6515..2702b3d7 100644 --- a/tests/e2e/internal/util/moduleconfig.go +++ b/tests/e2e/internal/util/moduleconfig.go @@ -24,11 +24,13 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" + apiv1alpha1 "github.com/deckhouse/operator-helm/api/v1alpha1" "github.com/deckhouse/operator-helm/tests/e2e/internal/framework" ) @@ -218,10 +220,11 @@ func UntilModuleEnabled(deployAt metav1.Time, timeout time.Duration) { Pods(moduleNamespace). List(context.Background(), metav1.ListOptions{}) g.Expect(err).NotTo(HaveOccurred()) - g.Expect(pods.Items).NotTo(BeEmpty(), + activePods := notTerminating(pods.Items) + g.Expect(activePods).NotTo(BeEmpty(), "no pods found in namespace %s", moduleNamespace) - for _, pod := range pods.Items { + for _, pod := range activePods { g.Expect(pod.CreationTimestamp.After(deployAt.UTC().Add(-1*time.Second))).To(BeTrue(), "pod was created at %v, which is not after %v", pod.CreationTimestamp, deployAt) g.Expect(pod.Status.Phase).To(Equal(corev1.PodRunning), @@ -235,20 +238,25 @@ func UntilModuleEnabled(deployAt metav1.Time, timeout time.Duration) { Eventually(func(g Gomega) { err := framework.GetClients().KubeClient().CoreV1().Pods("d8-system").DeleteCollection(context.TODO(), metav1.DeleteOptions{}, metav1.ListOptions{LabelSelector: "app=webhook-handler"}) - Expect(err).NotTo(HaveOccurred(), "should remove deckhouse webhook-hander pods in d8-system namespace") - }).WithTimeout(framework.ShortTimeout).WithPolling(framework.PollingInterval) + g.Expect(err).NotTo(HaveOccurred(), "should remove deckhouse webhook-handler pods in d8-system namespace") + }).WithTimeout(framework.ShortTimeout).WithPolling(framework.PollingInterval).Should(Succeed()) UntilAllPodsReady("d8-system", "app=webhook-handler", 1, timeout) + // The delete above may still be tearing down the old pod when this Consistently + // starts: the terminating pod lingers in List results alongside the already-Running + // replacement UntilAllPodsReady just confirmed, so it must be filtered out here too + // or a single healthy pod counts as two. Consistently(func(g Gomega) { pods, err := framework.GetClients().KubeClient().CoreV1(). Pods("d8-system"). - List(context.Background(), metav1.ListOptions{LabelSelector: "app=webhook-hander"}) + List(context.Background(), metav1.ListOptions{LabelSelector: "app=webhook-handler"}) g.Expect(err).NotTo(HaveOccurred()) - g.Expect(len(pods.Items)).To(Equal(1), - "expected %d pods, got %d", 1, len(pods.Items)) + activePods := notTerminating(pods.Items) + g.Expect(len(activePods)).To(Equal(1), + "expected %d pods, got %d", 1, len(activePods)) - for _, pod := range pods.Items { + for _, pod := range activePods { g.Expect(pod.Status.Phase).To(Equal(corev1.PodRunning), "pod %s phase: %s", pod.Name, pod.Status.Phase) for _, cs := range pod.Status.ContainerStatuses { @@ -256,7 +264,65 @@ func UntilModuleEnabled(deployAt metav1.Time, timeout time.Duration) { "pod %s container %s not ready", pod.Name, cs.Name) } } - }, "60s", "1s") + }, "60s", "1s").Should(Succeed()) + + // The caBundle check above only proves that two API objects agree with each + // other: the ValidatingWebhookConfiguration's caBundle equals ca.crt in the + // controller's TLS Secret. It does not prove the API server can actually + // complete a TLS handshake with the certificate the running webhook pod + // serves. Only HelmClusterAddon goes through that webhook, so a dry-run + // create of one is the faithful, side-effect-free way to exercise the real + // admission path before any spec relies on it. + By("Verifying the HelmClusterAddon validating webhook is reachable") + + probe := &apiv1alpha1.HelmClusterAddon{ + ObjectMeta: metav1.ObjectMeta{ + Name: "e2e-webhook-probe-preflight", + }, + Spec: apiv1alpha1.HelmClusterAddonSpec{ + Chart: apiv1alpha1.HelmClusterAddonChartRef{ + HelmClusterAddonChartName: "e2e-webhook-probe", + HelmClusterAddonRepository: "e2e-webhook-probe", + Version: "0.0.0", + }, + Namespace: "default", + }, + } + + Eventually(func(g Gomega) { + _, err := framework.GetClients().OperatorClient().HelmV1alpha1(). + HelmClusterAddons(). + Create(context.TODO(), probe, metav1.CreateOptions{DryRun: []string{metav1.DryRunAll}}) + + // Schema validation runs before admission webhooks in the API server's + // pipeline, so an Invalid response means the request never reached the + // webhook at all. That is not "not ready yet" — it means the probe object + // above has drifted from the CRD's own constraints (a new required field, + // a tightened MinLength, ...) and this check has stopped proving anything + // about the webhook. Fail the setup immediately rather than retrying a + // broken probe until the timeout. + if apierrors.IsInvalid(err) { + Expect(err).NotTo(HaveOccurred(), + "the webhook-reachability probe object is no longer valid against the "+ + "HelmClusterAddon CRD (%v); fix the probe built in UntilModuleEnabled "+ + "to satisfy the current CRD constraints — as written it can no longer "+ + "prove the validating webhook is reachable", err) + } + + if err == nil || !apierrors.IsInternalError(err) { + // Either the dry-run create was admitted, or it was rejected with a + // verdict that only the webhook itself could have produced (e.g. a + // namespace or uniqueness violation). Both prove the webhook was + // actually called, which is all this probe needs to establish. + return + } + + g.Expect(err).NotTo(HaveOccurred(), + "the HelmClusterAddon validating webhook is still not reachable: %v. "+ + "This is usually caused by the webhook serving a certificate the "+ + "API server does not yet trust, even though the caBundle/ca.crt "+ + "check above already passed.", err) + }).WithTimeout(timeout).WithPolling(framework.PollingInterval).Should(Succeed()) } func UntilModuleDisabled(timeout time.Duration) { diff --git a/tests/e2e/internal/util/pod.go b/tests/e2e/internal/util/pod.go index 97972c89..5acd5730 100644 --- a/tests/e2e/internal/util/pod.go +++ b/tests/e2e/internal/util/pod.go @@ -28,6 +28,25 @@ import ( "github.com/deckhouse/operator-helm/tests/e2e/internal/framework" ) +// notTerminating filters out pods that are being deleted. +// +// A terminating pod (DeletionTimestamp set) keeps appearing in List results +// until the kubelet finishes tearing it down, so counting it alongside its +// already-Running replacement makes a rollout or a deliberate delete look +// like the workload has twice as many pods as it actually does. Every +// assertion here that counts pods or makes a per-pod Running/Ready claim +// should filter through this first, so it only ever sees the pods that are +// actually meant to be there. +func notTerminating(pods []corev1.Pod) []corev1.Pod { + filtered := make([]corev1.Pod, 0, len(pods)) + for _, pod := range pods { + if pod.DeletionTimestamp == nil { + filtered = append(filtered, pod) + } + } + return filtered +} + // UntilControllerReady waits for all controller pods to be Running with all // containers Ready and zero restarts. func UntilControllerReady(namespace, labelSelector string, timeout time.Duration) { @@ -37,10 +56,11 @@ func UntilControllerReady(namespace, labelSelector string, timeout time.Duration Pods(namespace). List(context.Background(), metav1.ListOptions{LabelSelector: labelSelector}) g.Expect(err).NotTo(HaveOccurred()) - g.Expect(pods.Items).NotTo(BeEmpty(), + activePods := notTerminating(pods.Items) + g.Expect(activePods).NotTo(BeEmpty(), "no controller pods found with selector %s in namespace %s", labelSelector, namespace) - for _, pod := range pods.Items { + for _, pod := range activePods { g.Expect(pod.Status.Phase).To(Equal(corev1.PodRunning), "pod %s is %s, not Running", pod.Name, pod.Status.Phase) @@ -62,9 +82,10 @@ func AssertPodsExist(namespace, labelSelector string, minCount int) { Pods(namespace). List(context.Background(), metav1.ListOptions{LabelSelector: labelSelector}) Expect(err).NotTo(HaveOccurred()) - Expect(len(pods.Items)).To(BeNumerically(">=", minCount), + activePods := notTerminating(pods.Items) + Expect(len(activePods)).To(BeNumerically(">=", minCount), "expected >= %d pods in %s with selector %s, got %d", - minCount, namespace, labelSelector, len(pods.Items)) + minCount, namespace, labelSelector, len(activePods)) } // UntilPodsExist waits until at least minCount pods appear. @@ -75,8 +96,9 @@ func UntilPodsExist(namespace, labelSelector string, minCount int, timeout time. Pods(namespace). List(context.Background(), metav1.ListOptions{LabelSelector: labelSelector}) g.Expect(err).NotTo(HaveOccurred()) - g.Expect(len(pods.Items)).To(BeNumerically(">=", minCount), - "waiting for >= %d pods, got %d", minCount, len(pods.Items)) + activePods := notTerminating(pods.Items) + g.Expect(len(activePods)).To(BeNumerically(">=", minCount), + "waiting for >= %d pods, got %d", minCount, len(activePods)) }).WithTimeout(timeout).WithPolling(framework.PollingInterval).Should(Succeed()) } @@ -112,10 +134,11 @@ func UntilAllPodsReady(namespace, labelSelector string, expectedCount int, timeo Pods(namespace). List(context.Background(), metav1.ListOptions{LabelSelector: labelSelector}) g.Expect(err).NotTo(HaveOccurred()) - g.Expect(len(pods.Items)).To(Equal(expectedCount), - "expected %d pods, got %d", expectedCount, len(pods.Items)) + activePods := notTerminating(pods.Items) + g.Expect(len(activePods)).To(Equal(expectedCount), + "expected %d pods, got %d", expectedCount, len(activePods)) - for _, pod := range pods.Items { + for _, pod := range activePods { g.Expect(pod.Status.Phase).To(Equal(corev1.PodRunning), "pod %s phase: %s", pod.Name, pod.Status.Phase) for _, cs := range pod.Status.ContainerStatuses {