From 539c642a24164b4fc83e2a38bb724d606aa02353 Mon Sep 17 00:00:00 2001 From: Ilya Drey Date: Wed, 2 Sep 2026 23:40:36 +0300 Subject: [PATCH 01/19] feat(api): add media type and availability to chart versions Signed-off-by: Ilya Drey --- api/naming/naming.go | 56 +++++++++++++++++++ api/naming/naming_test.go | 55 ++++++++++++++++++ api/v1alpha1/conditions.go | 6 ++ api/v1alpha1/helm_cluster_addon_chart.go | 33 ++++++++++- crds/doc-ru-helmclusteraddoncharts.yaml | 8 ++- crds/helmclusteraddoncharts.yaml | 24 +++++++- .../reconcile/helmclusteraddon/reconciler.go | 3 +- .../internal/services/chart_service.go | 3 +- .../internal/services/repo_sync_service.go | 3 +- .../services/repo_sync_service_test.go | 5 +- .../internal/utils/name.go | 22 -------- 11 files changed, 188 insertions(+), 30 deletions(-) create mode 100644 api/naming/naming.go create mode 100644 api/naming/naming_test.go 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/operator-helm-controller/internal/reconcile/helmclusteraddon/reconciler.go b/images/operator-helm-controller/internal/reconcile/helmclusteraddon/reconciler.go index ded26aee..664114e2 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" @@ -415,7 +416,7 @@ func (r *Reconciler) reconcileForceAnnotation(ctx context.Context, req reconcile } func (r *Reconciler) getHelmClusterAddonChart(ctx context.Context, addon *helmv1alpha1.HelmClusterAddon) (*helmv1alpha1.HelmClusterAddonChart, error) { - addonChartName := utils.GetHelmClusterAddonChartName(addon.Spec.Chart.HelmClusterAddonRepository, addon.Spec.Chart.HelmClusterAddonChartName) + addonChartName := naming.HelmClusterAddonChartName(addon.Spec.Chart.HelmClusterAddonRepository, addon.Spec.Chart.HelmClusterAddonChartName) addonChart := &helmv1alpha1.HelmClusterAddonChart{} err := r.Get(ctx, types.NamespacedName{Name: addonChartName}, addonChart) 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/repo_sync_service.go b/images/operator-helm-controller/internal/services/repo_sync_service.go index 4ccf9744..20d5188c 100644 --- a/images/operator-helm-controller/internal/services/repo_sync_service.go +++ b/images/operator-helm-controller/internal/services/repo_sync_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" repoclient "github.com/deckhouse/operator-helm/internal/client/repository" "github.com/deckhouse/operator-helm/internal/utils" @@ -151,7 +152,7 @@ func (s *RepoSyncService) reconcileCatalog( continue } - addonChartName := utils.GetHelmClusterAddonChartName(repo.Name, chart.Name) + addonChartName := naming.HelmClusterAddonChartName(repo.Name, chart.Name) existing := &helmv1alpha1.HelmClusterAddonChart{ ObjectMeta: metav1.ObjectMeta{Name: addonChartName}, } 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..7562c2ef 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 @@ -26,6 +26,7 @@ import ( "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" repoclient "github.com/deckhouse/operator-helm/internal/client/repository" "github.com/deckhouse/operator-helm/internal/utils" @@ -77,7 +78,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 +91,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"}, }, } 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)) From 7f39130a9caaeabcab45bc9d2f56d9d8d7ce25bc Mon Sep 17 00:00:00 2001 From: Ilya Drey Date: Wed, 2 Sep 2026 23:46:38 +0300 Subject: [PATCH 02/19] feat(oci): identify a chart artifact by manifest, config and layer media type Signed-off-by: Ilya Drey --- .../internal/client/repository/oci_chart.go | 100 ++++++++++++++ .../client/repository/oci_chart_test.go | 127 ++++++++++++++++++ 2 files changed, 227 insertions(+) create mode 100644 images/operator-helm-controller/internal/client/repository/oci_chart.go create mode 100644 images/operator-helm-controller/internal/client/repository/oci_chart_test.go 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) + } + }) + } +} From 41005e757f2c8483ad92ff20003dfa2e4a256bec Mon Sep 17 00:00:00 2001 From: Ilya Drey Date: Wed, 2 Sep 2026 23:55:42 +0300 Subject: [PATCH 03/19] feat(oci): resolve chart layer media type per tag with an incremental pass Signed-off-by: Ilya Drey --- images/operator-helm-controller/go.mod | 2 +- .../internal/client/repository/client.go | 51 ++- .../internal/client/repository/helm.go | 5 +- .../internal/client/repository/helm_test.go | 6 +- .../internal/client/repository/oci.go | 194 +++++++++- .../internal/client/repository/oci_test.go | 331 +++++++++++++++++- .../reconciler_test.go | 2 +- .../internal/services/repo_sync_service.go | 2 +- .../services/repo_sync_service_test.go | 2 +- 9 files changed, 571 insertions(+), 24 deletions(-) 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..c3472f47 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,57 @@ 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 +} + +// 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..9662351f 100644 --- a/images/operator-helm-controller/internal/client/repository/oci.go +++ b/images/operator-helm-controller/internal/client/repository/oci.go @@ -17,17 +17,21 @@ limitations under the License. package repository import ( + "bytes" "context" "errors" "fmt" + "net/http" "strings" "time" "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 +40,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 +105,38 @@ 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. The only error returned is a terminal one: a per-tag failure +// becomes a ResolvePending entry so the rest of the pass is still published. +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 +148,143 @@ 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}), + ) + + 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(repo.Tag(c.tag), c.version, tagOptions) + if err != nil { + return err + } + resolved[index] = version + + return nil + }) + } + + if err := group.Wait(); 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 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 := known.UnavailableReason + if reason == helmv1alpha1.UnavailableReasonRemovedFromRepository { + reason = "" + } + + return &ChartVersion{ + Version: version, + MediaType: known.MediaType, + UnavailableReason: reason, + } +} + +// 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. +func resolveChartVersion(ref name.Reference, version *semver.Version, options []remote.Option) (*ChartVersion, error) { + desc, err := remote.Get(ref, options...) + 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 + } + + return message[:unavailableMessageLimit] + "…" } func trimSchemaPrefixes(url string) string { @@ -124,11 +295,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_test.go b/images/operator-helm-controller/internal/client/repository/oci_test.go index 5884ac8d..daab820a 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,29 @@ package repository import ( "context" "errors" + "io" + "log" "net/http" + "net/http/httptest" + "strings" + "sync" "testing" + "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 +102,319 @@ 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{}) + + 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) + } +} + +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() + + known := KnownCharts{"podinfo": KnownVersions{ + "1.0.0": {MediaType: currentChartMediaType}, + "1.0.1": {UnavailableReason: helmv1alpha1.UnavailableReasonUnsupportedMediaType}, + "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.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) { + _, host := newFakeRegistry(t) + pushChart(t, host, "1.0.0", helmConfigMediaType, currentChartMediaType) + + known := KnownCharts{"podinfo": KnownVersions{"1.0.0": { + MediaType: currentChartMediaType, + UnavailableReason: helmv1alpha1.UnavailableReasonRemovedFromRepository, + }}} + + version := versionByTag(t, fetchOne(t, host, FetchOptions{Known: known}), "1.0.0") + if version.UnavailableReason != "" { + t.Fatalf("a listed tag must not stay removed, got reason %q", version.UnavailableReason) + } +} 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..1f6b8e06 100644 --- a/images/operator-helm-controller/internal/reconcile/helmclusteraddonrepository/reconciler_test.go +++ b/images/operator-helm-controller/internal/reconcile/helmclusteraddonrepository/reconciler_test.go @@ -48,7 +48,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 } 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 20d5188c..4789fe7c 100644 --- a/images/operator-helm-controller/internal/services/repo_sync_service.go +++ b/images/operator-helm-controller/internal/services/repo_sync_service.go @@ -99,7 +99,7 @@ func (s *RepoSyncService) fetchCharts( } } - charts, err := repoClient.FetchCharts(ctx, repo.Spec.URL, buildRepoConfig(repo)) + charts, err := repoClient.FetchCharts(ctx, repo.Spec.URL, buildRepoConfig(repo), repoclient.FetchOptions{}) if err == nil { return charts, FetchOutcome{} } 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 7562c2ef..7a2d9d04 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 @@ -37,7 +37,7 @@ 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 } From e46bb104049808b63a6869e93ad39224b2f6013a Mon Sep 17 00:00:00 2001 From: Ilya Drey Date: Thu, 3 Sep 2026 00:10:59 +0300 Subject: [PATCH 04/19] fix(oci): share one puller per pass and surface context cancellation - resolveChartVersions now builds one remote.Puller for the whole repository and passes it to every goroutine, instead of remote.Get building a fresh Puller (and paying a fresh /v2/ ping plus, on a bearer registry, a fresh token request) for every examined tag. - After group.Wait() succeeds, check the caller's context: a cancelled context previously surfaced as remote.Get failures that fell through to fabricated ResolvePending verdicts instead of an error, turning a cancelled pass into an apparently successful one. - KnownVersion carries UnavailableMessage so a skipped unsupported tag keeps its explanation across passes; carryKnown clears the message together with a cleared RemovedFromRepository reason. - truncate cuts on a UTF-8 rune boundary instead of a raw byte index. - Strengthened TestFetchChartsOCIDropsVanishedTag and TestFetchChartsOCIClearsRemovedFromRepository to be actual evidence for the behaviour they claim, and added tests for the cancelled context and the rune-safe truncation. Signed-off-by: Ilya Drey --- .../internal/client/repository/client.go | 5 +- .../internal/client/repository/oci.go | 52 +++++++--- .../internal/client/repository/oci_test.go | 95 ++++++++++++++++++- 3 files changed, 134 insertions(+), 18 deletions(-) diff --git a/images/operator-helm-controller/internal/client/repository/client.go b/images/operator-helm-controller/internal/client/repository/client.go index c3472f47..8d537ca7 100644 --- a/images/operator-helm-controller/internal/client/repository/client.go +++ b/images/operator-helm-controller/internal/client/repository/client.go @@ -49,8 +49,9 @@ type ChartVersion struct { // KnownVersion is the verdict a previous pass reached for one tag. type KnownVersion struct { - MediaType string - UnavailableReason string + MediaType string + UnavailableReason string + UnavailableMessage string } // KnownVersions maps a tag to its recorded verdict. diff --git a/images/operator-helm-controller/internal/client/repository/oci.go b/images/operator-helm-controller/internal/client/repository/oci.go index 9662351f..96ed469b 100644 --- a/images/operator-helm-controller/internal/client/repository/oci.go +++ b/images/operator-helm-controller/internal/client/repository/oci.go @@ -24,6 +24,7 @@ import ( "net/http" "strings" "time" + "unicode/utf8" "github.com/Masterminds/semver/v3" "github.com/google/go-containerregistry/pkg/authn" @@ -174,6 +175,16 @@ func resolveChartVersions( 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] @@ -184,7 +195,7 @@ func resolveChartVersions( return nil } - version, err := resolveChartVersion(repo.Tag(c.tag), c.version, tagOptions) + version, err := resolveChartVersion(groupCtx, puller, repo.Tag(c.tag), c.version) if err != nil { return err } @@ -198,6 +209,14 @@ func resolveChartVersions( 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 { @@ -210,27 +229,31 @@ func resolveChartVersions( } // carryKnown reuses a recorded verdict. A tag that is listed again is by definition no -// longer removed from the repository, so that marker is dropped here: presence in the -// listing is registry truth, which is this client's domain. +// 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 := known.UnavailableReason + reason, message := known.UnavailableReason, known.UnavailableMessage if reason == helmv1alpha1.UnavailableReasonRemovedFromRepository { - reason = "" + reason, message = "", "" } return &ChartVersion{ - Version: version, - MediaType: known.MediaType, - UnavailableReason: reason, + 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. -func resolveChartVersion(ref name.Reference, version *semver.Version, options []remote.Option) (*ChartVersion, error) { - desc, err := remote.Get(ref, options...) +// 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) { @@ -284,7 +307,12 @@ func truncate(message string) string { return message } - return message[:unavailableMessageLimit] + "…" + cut := unavailableMessageLimit + for cut > 0 && !utf8.RuneStart(message[cut]) { + cut-- + } + + return message[:cut] + "…" } func trimSchemaPrefixes(url string) string { 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 daab820a..31d05456 100644 --- a/images/operator-helm-controller/internal/client/repository/oci_test.go +++ b/images/operator-helm-controller/internal/client/repository/oci_test.go @@ -26,6 +26,7 @@ import ( "strings" "sync" "testing" + "unicode/utf8" "github.com/google/go-containerregistry/pkg/name" "github.com/google/go-containerregistry/pkg/registry" @@ -324,6 +325,10 @@ func TestFetchChartsOCIDropsVanishedTag(t *testing.T) { 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") @@ -349,6 +354,35 @@ func TestFetchChartsOCIEscalatesUnauthorized(t *testing.T) { } } +// 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) @@ -357,9 +391,17 @@ func TestFetchChartsOCISkipsKnownTags(t *testing.T) { 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}, + "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": {}, @@ -383,6 +425,9 @@ func TestFetchChartsOCISkipsKnownTags(t *testing.T) { 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) } @@ -405,16 +450,58 @@ func TestFetchChartsOCIFullIgnoresKnown(t *testing.T) { } func TestFetchChartsOCIClearsRemovedFromRepository(t *testing.T) { - _, host := newFakeRegistry(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, + 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) + } } From 71309d230c79b0b996b43ddc0d7b161246a26498 Mon Sep 17 00:00:00 2001 From: Ilya Drey Date: Thu, 3 Sep 2026 00:23:13 +0300 Subject: [PATCH 05/19] feat(core): merge chart versions incrementally and protect referenced ones from pruning Signed-off-by: Ilya Drey --- .../cmd/operator-helm-controller/main.go | 3 +- .../internal/index/index.go | 52 ++++ .../reconciler_test.go | 9 + .../internal/services/outcomes.go | 5 +- .../internal/services/repo_sync_service.go | 189 ++++++++++++++- .../services/repo_sync_service_test.go | 229 ++++++++++++++++++ .../webhook/helmclusteraddon/webhook.go | 17 +- .../helmclusteraddonrepository/lifecycle.go | 8 +- 8 files changed, 483 insertions(+), 29 deletions(-) create mode 100644 images/operator-helm-controller/internal/index/index.go 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..dedea8e1 100644 --- a/images/operator-helm-controller/cmd/operator-helm-controller/main.go +++ b/images/operator-helm-controller/cmd/operator-helm-controller/main.go @@ -33,6 +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/index" "github.com/deckhouse/operator-helm/internal/utils" helmclusteraddonwebhook "github.com/deckhouse/operator-helm/internal/webhook/helmclusteraddon" ) @@ -92,7 +93,7 @@ func main() { os.Exit(1) } - if err = helmclusteraddonwebhook.SetupIndexes(mgr); err != nil { + if err = index.SetupAddonChart(mgr); err != nil { logger.Error(err, "unable to setup indexes", "webhook", "HelmClusterAddon") os.Exit(1) } 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..50e1f194 --- /dev/null +++ b/images/operator-helm-controller/internal/index/index.go @@ -0,0 +1,52 @@ +/* +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, + )} + }, + ) +} 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 1f6b8e06..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" @@ -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/outcomes.go b/images/operator-helm-controller/internal/services/outcomes.go index ca6477da..012bbcad 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. 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 4789fe7c..0177e93f 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" @@ -32,6 +33,7 @@ import ( "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" ) @@ -76,7 +78,15 @@ 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 { + 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} } @@ -84,10 +94,46 @@ func (s *RepoSyncService) Sync( return SyncOutcome{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) + } + + known := make(repoclient.KnownCharts, len(charts.Items)) + + for _, chart := range charts.Items { + chartName := chart.Labels[LabelChartName] + if chartName == "" { + 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 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 { @@ -99,9 +145,9 @@ func (s *RepoSyncService) fetchCharts( } } - charts, err := repoClient.FetchCharts(ctx, repo.Spec.URL, buildRepoConfig(repo), repoclient.FetchOptions{}) + 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 { @@ -120,6 +166,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 @@ -148,11 +208,10 @@ func (s *RepoSyncService) reconcileCatalog( desiredCharts := make(map[string]struct{}, len(charts)) for _, chart := range charts { - if len(chart.Versions) == 0 { - continue - } - 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}, } @@ -186,12 +245,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)} @@ -208,6 +272,19 @@ func (s *RepoSyncService) reconcileCatalog( continue } + inUse, err := s.inUseVersions(ctx, repo.Name, chart.Labels[LabelChartName]) + 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)} } @@ -215,3 +292,91 @@ 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{}{} + if last := addon.Status.LastAppliedChart; last != nil { + 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. +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)) + + for _, version := range fetched { + name := version.Version.Original() + listed[name] = struct{}{} + + merged = append(merged, helmv1alpha1.HelmClusterAddonChartVersion{ + Version: name, + MediaType: version.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 — and ordering +// versions that do not parse — by a reverse string comparison. The order has to be +// deterministic: 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) + + if leftErr == nil && rightErr == nil && !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 7a2d9d04..68d4cb17 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 @@ -29,6 +29,7 @@ import ( "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" ) @@ -41,6 +42,17 @@ func (s stubRepoClient) FetchCharts(_ context.Context, _ string, _ *repoclient.R 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() @@ -49,6 +61,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) { @@ -58,6 +78,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, @@ -145,3 +205,172 @@ 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, + }, + ) + + 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 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") + } +} + +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"} + 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) + } +} 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/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") } }) }) From 1803348f0d19a54174b3c2b766f147265142e9ba Mon Sep 17 00:00:00 2001 From: Ilya Drey Date: Thu, 3 Sep 2026 00:44:06 +0300 Subject: [PATCH 06/19] fix(core): stop reporting a pre-fetch cluster failure as a successful fetch Add SyncOutcome.FetchAttempted so the reconciler only trusts a Fetch outcome when the registry was actually contacted; without it a knownCharts listing failure reset ConsecutiveFetchFailures and could write Ready=True off a fetch that never ran. Also: guard the last-applied-version credit by repository/chart identity, make sortChartVersions a strict weak ordering, log the label-less prune and knownCharts-drop branches, and fix the startup log attribution for the shared field index. Signed-off-by: Ilya Drey --- .../cmd/operator-helm-controller/main.go | 2 +- .../helmclusteraddonrepository/evaluate.go | 7 +++ .../evaluate_test.go | 50 ++++++++++++++++ .../helmclusteraddonrepository/reconciler.go | 8 ++- .../internal/services/outcomes.go | 12 +++- .../internal/services/repo_sync_service.go | 59 ++++++++++++++++--- .../services/repo_sync_service_test.go | 58 +++++++++++++++++- 7 files changed, 180 insertions(+), 16 deletions(-) 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 dedea8e1..098b6543 100644 --- a/images/operator-helm-controller/cmd/operator-helm-controller/main.go +++ b/images/operator-helm-controller/cmd/operator-helm-controller/main.go @@ -94,7 +94,7 @@ func main() { } if err = index.SetupAddonChart(mgr); err != nil { - logger.Error(err, "unable to setup indexes", "webhook", "HelmClusterAddon") + logger.Error(err, "unable to setup indexes", "index", index.AddonChart) os.Exit(1) } diff --git a/images/operator-helm-controller/internal/reconcile/helmclusteraddonrepository/evaluate.go b/images/operator-helm-controller/internal/reconcile/helmclusteraddonrepository/evaluate.go index 54257a39..b33b7ec3 100644 --- a/images/operator-helm-controller/internal/reconcile/helmclusteraddonrepository/evaluate.go +++ b/images/operator-helm-controller/internal/reconcile/helmclusteraddonrepository/evaluate.go @@ -424,6 +424,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..aa3677cc 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,56 @@ 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 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/services/outcomes.go b/images/operator-helm-controller/internal/services/outcomes.go index 012bbcad..deb756a8 100644 --- a/images/operator-helm-controller/internal/services/outcomes.go +++ b/images/operator-helm-controller/internal/services/outcomes.go @@ -52,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 0177e93f..20a4ff31 100644 --- a/images/operator-helm-controller/internal/services/repo_sync_service.go +++ b/images/operator-helm-controller/internal/services/repo_sync_service.go @@ -80,6 +80,9 @@ func (s *RepoSyncService) Sync( ) SyncOutcome { 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}} } @@ -88,10 +91,10 @@ func (s *RepoSyncService) Sync( Full: repo.ForceReconcileRequired(), }) if fetch.Err != nil { - return SyncOutcome{Fetch: fetch} + return SyncOutcome{FetchAttempted: true, Fetch: fetch} } - return SyncOutcome{Fetch: fetch, Catalog: s.reconcileCatalog(ctx, repo, charts)} + return SyncOutcome{FetchAttempted: true, Fetch: fetch, Catalog: s.reconcileCatalog(ctx, repo, charts)} } // knownCharts collects the verdicts recorded by previous passes, so the client can @@ -106,11 +109,19 @@ func (s *RepoSyncService) knownCharts( 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 } @@ -272,7 +283,17 @@ func (s *RepoSyncService) reconcileCatalog( continue } - inUse, err := s.inUseVersions(ctx, repo.Name, chart.Labels[LabelChartName]) + 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} } @@ -313,7 +334,14 @@ func (s *RepoSyncService) inUseVersions(ctx context.Context, repoName, chartName for _, addon := range addons.Items { inUse[addon.Spec.Chart.Version] = struct{}{} - if last := addon.Status.LastAppliedChart; last != nil { + + // 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{}{} } } @@ -364,16 +392,29 @@ func mergeChartVersions( return merged } -// sortChartVersions orders versions by descending semver, breaking ties — and ordering -// versions that do not parse — by a reverse string comparison. The order has to be -// deterministic: the merge goes through maps, and an unstable order would produce a -// status patch on every synchronization for a catalog that did not change. +// 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) - if leftErr == nil && rightErr == nil && !left.Equal(right) { + leftParses, rightParses := leftErr == nil, rightErr == nil + + if leftParses != rightParses { + return leftParses + } + + if leftParses && !left.Equal(right) { return left.GreaterThan(right) } 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 68d4cb17..00f9e3e7 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,6 +25,7 @@ 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" @@ -211,8 +212,9 @@ func TestSyncPassesKnownVersionsToTheClient(t *testing.T) { chart := existingChart(repo.Name, "podinfo", helmv1alpha1.HelmClusterAddonChartVersion{Version: "6.7.1", MediaType: "application/tar+gzip"}, helmv1alpha1.HelmClusterAddonChartVersion{ - Version: "6.7.2", - UnavailableReason: helmv1alpha1.UnavailableReasonUnsupportedMediaType, + Version: "6.7.2", + UnavailableReason: helmv1alpha1.UnavailableReasonUnsupportedMediaType, + UnavailableMessage: "layer media type application/vnd.example is not a chart", }, ) @@ -237,6 +239,9 @@ func TestSyncPassesKnownVersionsToTheClient(t *testing.T) { 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") } @@ -323,6 +328,9 @@ func TestSyncOrdersVersionsBySemverDescending(t *testing.T) { 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) @@ -374,3 +382,49 @@ func TestSyncKeepsChartReferencedByAddon(t *testing.T) { 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) + } +} From e56a78cd19e945b90f1105f827574dfe2c82d500 Mon Sep 17 00:00:00 2001 From: Ilya Drey Date: Thu, 3 Sep 2026 00:50:08 +0300 Subject: [PATCH 07/19] feat(core): report an incomplete first catalog read as PartialSync Signed-off-by: Ilya Drey --- .../helmclusteraddonrepository/evaluate.go | 15 ++- .../evaluate_test.go | 97 +++++++++++++++++++ 2 files changed, 111 insertions(+), 1 deletion(-) diff --git a/images/operator-helm-controller/internal/reconcile/helmclusteraddonrepository/evaluate.go b/images/operator-helm-controller/internal/reconcile/helmclusteraddonrepository/evaluate.go index b33b7ec3..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, "" } 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 aa3677cc..e43d75fb 100644 --- a/images/operator-helm-controller/internal/reconcile/helmclusteraddonrepository/evaluate_test.go +++ b/images/operator-helm-controller/internal/reconcile/helmclusteraddonrepository/evaluate_test.go @@ -526,6 +526,103 @@ func TestEvaluatePreservesFailuresWhenNoFetchWasAttempted(t *testing.T) { } } +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() From 1bbc84ee76919c8f7b11ab74ccb060f97dddd4f6 Mon Sep 17 00:00:00 2001 From: Ilya Drey Date: Thu, 3 Sep 2026 00:56:31 +0300 Subject: [PATCH 08/19] feat(core): build the internal OCIRepository from the recorded chart media type Signed-off-by: Ilya Drey --- .../reconcile/helmclusteraddon/reconciler.go | 59 ++++++++-- .../internal/services/oci_repo_service.go | 33 +++++- .../services/oci_repo_service_test.go | 110 ++++++++++++++++++ 3 files changed, 186 insertions(+), 16 deletions(-) create mode 100644 images/operator-helm-controller/internal/services/oci_repo_service_test.go diff --git a/images/operator-helm-controller/internal/reconcile/helmclusteraddon/reconciler.go b/images/operator-helm-controller/internal/reconcile/helmclusteraddon/reconciler.go index 664114e2..623e83bd 100644 --- a/images/operator-helm-controller/internal/reconcile/helmclusteraddon/reconciler.go +++ b/images/operator-helm-controller/internal/reconcile/helmclusteraddon/reconciler.go @@ -189,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: @@ -213,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 @@ -227,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, @@ -415,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 := naming.HelmClusterAddonChartName(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/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..9f71c350 --- /dev/null +++ b/images/operator-helm-controller/internal/services/oci_repo_service_test.go @@ -0,0 +1,110 @@ +/* +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") + } +} From 5bd52b7538acca4e94bf8046e6a45f5bb8372fc5 Mon Sep 17 00:00:00 2001 From: Ilya Drey Date: Thu, 3 Sep 2026 01:07:02 +0300 Subject: [PATCH 09/19] test(core): cover the OCI media-type deploy gate and the ready/removed override Signed-off-by: Ilya Drey --- .../helmclusteraddon/reconciler_test.go | 212 ++++++++++++++++++ .../services/oci_repo_service_test.go | 72 ++++++ 2 files changed, 284 insertions(+) create mode 100644 images/operator-helm-controller/internal/reconcile/helmclusteraddon/reconciler_test.go 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..cd1bf18f --- /dev/null +++ b/images/operator-helm-controller/internal/reconcile/helmclusteraddon/reconciler_test.go @@ -0,0 +1,212 @@ +/* +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, + }, + { + 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/services/oci_repo_service_test.go b/images/operator-helm-controller/internal/services/oci_repo_service_test.go index 9f71c350..ee6cce40 100644 --- a/images/operator-helm-controller/internal/services/oci_repo_service_test.go +++ b/images/operator-helm-controller/internal/services/oci_repo_service_test.go @@ -108,3 +108,75 @@ func TestEnsureInternalOCIRepositoryReportsRemovedVersion(t *testing.T) { 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) + } +} From f217362314c45f5938843143988ba2707e7e20fa Mon Sep 17 00:00:00 2001 From: Ilya Drey Date: Thu, 3 Sep 2026 01:16:56 +0300 Subject: [PATCH 10/19] feat(chart-values): take the chart layer media type from the catalog status Signed-off-by: Ilya Drey --- .../internal/resolver/resolver.go | 62 ++++++++- .../internal/resolver/resolver_test.go | 123 ++++++++++++++++++ .../chart-values-controller/rbac-for-us.yaml | 2 + 3 files changed, 180 insertions(+), 7 deletions(-) create mode 100644 images/chart-values-controller/internal/resolver/resolver_test.go diff --git a/images/chart-values-controller/internal/resolver/resolver.go b/images/chart-values-controller/internal/resolver/resolver.go index a78fe896..b7b47fe6 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,49 @@ 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 == "" { + 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 +194,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 +282,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 +302,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..acbddc06 --- /dev/null +++ b/images/chart-values-controller/internal/resolver/resolver_test.go @@ -0,0 +1,123 @@ +/* +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) + } + }) +} 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 From 99733699cbb64235cf2761ab9a838cd70d9e9c9e Mon Sep 17 00:00:00 2001 From: Ilya Drey Date: Thu, 3 Sep 2026 01:24:19 +0300 Subject: [PATCH 11/19] fix(chart-values): treat ResolvePending as retryable, cover RemovedFromRepository, name empty verdicts Signed-off-by: Ilya Drey --- .../internal/resolver/resolver.go | 14 +++++++ .../internal/resolver/resolver_test.go | 38 +++++++++++++++++++ 2 files changed, 52 insertions(+) diff --git a/images/chart-values-controller/internal/resolver/resolver.go b/images/chart-values-controller/internal/resolver/resolver.go index b7b47fe6..d8b9459a 100644 --- a/images/chart-values-controller/internal/resolver/resolver.go +++ b/images/chart-values-controller/internal/resolver/resolver.go @@ -149,7 +149,21 @@ func (r *Resolver) chartVersionMediaType(ctx context.Context, req Request) (stri } if version.MediaType == "" { + if version.UnavailableReason == helmv1alpha1.UnavailableReasonResolvePending { + // Pending means the catalog has not reached a verdict yet: the manifest + // request failed and will be retried on the next normal synchronization, + // so the caller should retry too rather than being told the version is + // permanently unreadable. + return "", &Result{Outcome: OutcomePending}, nil + } + + // Every other reason is a 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 detail == "" { + detail = "no verdict recorded" + } if version.UnavailableMessage != "" { detail += ": " + version.UnavailableMessage } diff --git a/images/chart-values-controller/internal/resolver/resolver_test.go b/images/chart-values-controller/internal/resolver/resolver_test.go index acbddc06..1155b57e 100644 --- a/images/chart-values-controller/internal/resolver/resolver_test.go +++ b/images/chart-values-controller/internal/resolver/resolver_test.go @@ -120,4 +120,42 @@ func TestChartVersionMediaType(t *testing.T) { 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 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) + } + }) } From 3eb39e57f3abaa32eea366b110d720e0454b260f Mon Sep 17 00:00:00 2001 From: Ilya Drey Date: Thu, 3 Sep 2026 01:42:52 +0300 Subject: [PATCH 12/19] fix(chart-values): treat empty unavailableReason as pending, not values-not-found An empty mediaType with an empty unavailableReason is the pre-upgrade shape of a version entry, re-resolved on the next normal sync exactly like ResolvePending. Reporting it as OutcomeValuesNotFound turned every OCI chart values request into a permanent 422 for the whole upgrade window, up to five minutes, instead of a retryable pending. Signed-off-by: Ilya Drey --- .../internal/resolver/resolver.go | 23 ++++++++++--------- .../internal/resolver/resolver_test.go | 18 +++++++++++++++ 2 files changed, 30 insertions(+), 11 deletions(-) diff --git a/images/chart-values-controller/internal/resolver/resolver.go b/images/chart-values-controller/internal/resolver/resolver.go index d8b9459a..c7e947e1 100644 --- a/images/chart-values-controller/internal/resolver/resolver.go +++ b/images/chart-values-controller/internal/resolver/resolver.go @@ -149,21 +149,22 @@ func (r *Resolver) chartVersionMediaType(ctx context.Context, req Request) (stri } if version.MediaType == "" { - if version.UnavailableReason == helmv1alpha1.UnavailableReasonResolvePending { - // Pending means the catalog has not reached a verdict yet: the manifest - // request failed and will be retried on the next normal synchronization, - // so the caller should retry too rather than being told the version is - // permanently unreadable. + 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 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. + // 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 detail == "" { - detail = "no verdict recorded" - } if version.UnavailableMessage != "" { detail += ": " + version.UnavailableMessage } diff --git a/images/chart-values-controller/internal/resolver/resolver_test.go b/images/chart-values-controller/internal/resolver/resolver_test.go index 1155b57e..533f0aaa 100644 --- a/images/chart-values-controller/internal/resolver/resolver_test.go +++ b/images/chart-values-controller/internal/resolver/resolver_test.go @@ -138,6 +138,24 @@ func TestChartVersionMediaType(t *testing.T) { } }) + 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{ From c8ebfec4dff5a7929ce91a83266a1bb1a208cd2a Mon Sep 17 00:00:00 2001 From: Ilya Drey Date: Thu, 3 Sep 2026 01:43:06 +0300 Subject: [PATCH 13/19] fix(core): keep a referenced version's media type on a fresh unsupported verdict mergeChartVersions wrote every fetched entry verbatim, so a tag re-pushed as a non-chart artifact wiped MediaType even for a version an addon still references, tripping the D4 deploy gate and bricking the addon. Carry the previously recorded media type forward for in-use versions while keeping the fresh UnavailableReason/Message, matching D4's table; leave unreferenced versions untouched. Also corrects a stale doc comment on resolveChartVersions that still claimed the only returned error is terminal, missing the puller- construction and cancelled-context cases added since. Signed-off-by: Ilya Drey --- .../internal/client/repository/oci.go | 14 +++- .../internal/services/repo_sync_service.go | 25 ++++++- .../services/repo_sync_service_test.go | 67 +++++++++++++++++++ 3 files changed, 103 insertions(+), 3 deletions(-) diff --git a/images/operator-helm-controller/internal/client/repository/oci.go b/images/operator-helm-controller/internal/client/repository/oci.go index 96ed469b..7ad6140d 100644 --- a/images/operator-helm-controller/internal/client/repository/oci.go +++ b/images/operator-helm-controller/internal/client/repository/oci.go @@ -121,8 +121,18 @@ func (c *ociRepositoryClient) FetchCharts(ctx context.Context, url string, confi // 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. The only error returned is a terminal one: a per-tag failure -// becomes a ResolvePending entry so the rest of the pass is still published. +// 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, 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 20a4ff31..ecd5aa37 100644 --- a/images/operator-helm-controller/internal/services/repo_sync_service.go +++ b/images/operator-helm-controller/internal/services/repo_sync_service.go @@ -354,6 +354,15 @@ func (s *RepoSyncService) inUseVersions(ctx context.Context, repoName, chartName // 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, @@ -362,13 +371,27 @@ func mergeChartVersions( 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: version.MediaType, + MediaType: mediaType, UnavailableReason: version.UnavailableReason, UnavailableMessage: version.UnavailableMessage, }) 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 00f9e3e7..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 @@ -312,6 +312,73 @@ func TestSyncRetainsReferencedVersionRemovedFromRepository(t *testing.T) { } } +// 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{{ From 5d0533ab7f5d978d1cba0cdb27c2fd611a7af329 Mon Sep 17 00:00:00 2001 From: Ilya Drey Date: Thu, 3 Sep 2026 10:07:48 +0300 Subject: [PATCH 14/19] refactor(core): move AddonRepository field index into internal/index Keep all HelmClusterAddon field indexes in one package: relocate the repository index out of internal/utils next to AddonChart, renaming it AddonRepository/SetupAddonRepository for consistency with the existing pair. Behaviour is unchanged, including the empty-repository guard. Signed-off-by: Ilya Drey --- .../cmd/operator-helm-controller/main.go | 5 ++--- .../internal/index/index.go | 18 ++++++++++++++++++ .../internal/utils/mapper.go | 18 ++---------------- 3 files changed, 22 insertions(+), 19 deletions(-) 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 098b6543..fdf9f8d0 100644 --- a/images/operator-helm-controller/cmd/operator-helm-controller/main.go +++ b/images/operator-helm-controller/cmd/operator-helm-controller/main.go @@ -34,7 +34,6 @@ import ( "github.com/deckhouse/operator-helm/internal/controller/helmclusteraddon" "github.com/deckhouse/operator-helm/internal/controller/helmclusteraddonrepository" "github.com/deckhouse/operator-helm/internal/index" - "github.com/deckhouse/operator-helm/internal/utils" helmclusteraddonwebhook "github.com/deckhouse/operator-helm/internal/webhook/helmclusteraddon" ) @@ -78,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) } diff --git a/images/operator-helm-controller/internal/index/index.go b/images/operator-helm-controller/internal/index/index.go index 50e1f194..aa9114e5 100644 --- a/images/operator-helm-controller/internal/index/index.go +++ b/images/operator-helm-controller/internal/index/index.go @@ -50,3 +50,21 @@ func SetupAddonChart(mgr ctrl.Manager) error { }, ) } + +// 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/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 } From f9a902de75507e9776afe244a7bbc00d5b616cec Mon Sep 17 00:00:00 2001 From: Ilya Drey Date: Thu, 3 Sep 2026 10:07:56 +0300 Subject: [PATCH 15/19] test(core): cover the deploy gate across a repository type switch Add two cases to the getHelmClusterAddonChart table test: an OCI-era catalog entry (media type set, no reason) evaluated against a Helm repository, which must still pass since the Helm gate never reads the media type; and a Helm-era entry (no media type, no reason) evaluated against an OCI repository, which must be rejected with the "has not resolved it yet" detail. These pin the two windows a repository's spec.url can pass through when it flips between oci:// and https://. Signed-off-by: Ilya Drey --- .../helmclusteraddon/reconciler_test.go | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/images/operator-helm-controller/internal/reconcile/helmclusteraddon/reconciler_test.go b/images/operator-helm-controller/internal/reconcile/helmclusteraddon/reconciler_test.go index cd1bf18f..5c03c0b7 100644 --- a/images/operator-helm-controller/internal/reconcile/helmclusteraddon/reconciler_test.go +++ b/images/operator-helm-controller/internal/reconcile/helmclusteraddon/reconciler_test.go @@ -147,6 +147,30 @@ func TestGetHelmClusterAddonChart(t *testing.T) { }, 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"}, From d3b13f67f095c9b8808ef31d2096d908dbc19ccc Mon Sep 17 00:00:00 2001 From: Ilya Drey Date: Thu, 3 Sep 2026 10:42:29 +0300 Subject: [PATCH 16/19] test(e2e): probe the HelmClusterAddon webhook's real TLS admission path The caBundle/ca.crt equality check in UntilModuleEnabled proves two API objects agree, but not that the API server can complete a TLS handshake with the certificate the running webhook pod serves. Add a dry-run create of a uniquely-named, schema-valid HelmClusterAddon at the end of setup so a certificate that isn't trusted yet fails there instead of mid-spec. Treat an Invalid response from the dry-run create as a hard, immediate failure of the probe object itself (schema validation runs before the webhook, so it proves nothing about reachability), rather than retrying it away until the timeout. Signed-off-by: Ilya Drey --- tests/e2e/internal/util/moduleconfig.go | 60 +++++++++++++++++++++++++ 1 file changed, 60 insertions(+) diff --git a/tests/e2e/internal/util/moduleconfig.go b/tests/e2e/internal/util/moduleconfig.go index dfab6515..cfd718b1 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" ) @@ -257,6 +259,64 @@ func UntilModuleEnabled(deployAt metav1.Time, timeout time.Duration) { } } }, "60s", "1s") + + // 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) { From fb86cdf65d25df4818b14e9fa31adf0d77f1a53b Mon Sep 17 00:00:00 2001 From: Ilya Drey Date: Thu, 3 Sep 2026 10:56:29 +0300 Subject: [PATCH 17/19] fix(core): roll the controller pods when the webhook certificate rotates MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The admission webhook's certificate is mounted from the operator-helm-controller-tls secret and read at startup, so rotating it updated the secret without rolling the pods and left them serving the previous one. That is not cosmetic staleness. The same values render both the secret and the caBundle of the ValidatingWebhookConfiguration, so the API server starts trusting the new CA while the pods still present the old certificate, and every HelmClusterAddon admission request fails with "certificate signed by unknown authority" until something else restarts them — which is exactly what CI hit. Hashing the certificate into the pod template makes the rollout part of the very release that rotates it, so the pods and the caBundle can never disagree. A pod-reloader annotation was considered instead and rejected: it reacts after the secret is written rather than atomically with it, and the pod-reloader module is absent from the Minimal bundle, where this failure is total. Signed-off-by: Ilya Drey --- templates/operator-helm-controller/deployment.yaml | 1 + 1 file changed, 1 insertion(+) 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 }} From fa5d33f9a4f287dac187a3b97aab960bf81ce7d6 Mon Sep 17 00:00:00 2001 From: Ilya Drey Date: Thu, 3 Sep 2026 11:04:03 +0300 Subject: [PATCH 18/19] fix(e2e): make two dead assertions in the module setup actually run MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Neither Eventually nor Consistently invokes its closure without a terminal matcher: .WithTimeout and .WithPolling only configure the AsyncAssertion, and Consistently's interval arguments do the same. Both blocks lacked .Should, so the step that removes deckhouse's webhook-handler pods never ran and the 60s stability check never ran either — the module setup only looked like it verified the webhook handler had settled. Two defects the dead code was hiding are fixed with them: the DeleteCollection block asserted with Expect instead of g.Expect, which would fail the spec outright instead of retrying, and the stability check listed pods by "app=webhook-hander", a selector that matches nothing. Signed-off-by: Ilya Drey --- tests/e2e/internal/util/moduleconfig.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/e2e/internal/util/moduleconfig.go b/tests/e2e/internal/util/moduleconfig.go index cfd718b1..aefa96bf 100644 --- a/tests/e2e/internal/util/moduleconfig.go +++ b/tests/e2e/internal/util/moduleconfig.go @@ -237,15 +237,15 @@ 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) 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)) @@ -258,7 +258,7 @@ 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 From 700016909fb0b275103d8d729f8b94f2ed89c366 Mon Sep 17 00:00:00 2001 From: Ilya Drey Date: Thu, 3 Sep 2026 11:20:57 +0300 Subject: [PATCH 19/19] fix(e2e): ignore terminating pods in e2e pod assertions A terminating pod 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. This flaked SynchronizedBeforeSuite after the webhook-handler pods were deliberately deleted and immediately recreated. Extract the DeletionTimestamp check UntilPodCount already used into a shared notTerminating helper and apply it everywhere pods are counted or asserted on: UntilControllerReady, UntilAllPodsReady, the Consistently block in UntilModuleEnabled that reproduced the failure, the module-namespace pod loop above it, and (for consistency, given their >= semantics) AssertPodsExist and UntilPodsExist. Signed-off-by: Ilya Drey --- tests/e2e/internal/util/moduleconfig.go | 16 +++++++--- tests/e2e/internal/util/pod.go | 41 +++++++++++++++++++------ 2 files changed, 43 insertions(+), 14 deletions(-) diff --git a/tests/e2e/internal/util/moduleconfig.go b/tests/e2e/internal/util/moduleconfig.go index aefa96bf..2702b3d7 100644 --- a/tests/e2e/internal/util/moduleconfig.go +++ b/tests/e2e/internal/util/moduleconfig.go @@ -220,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), @@ -242,15 +243,20 @@ func UntilModuleEnabled(deployAt metav1.Time, timeout time.Duration) { 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-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 { 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 {