From 4d1229a327ebcbbea847ad6b165c918de1f47aa4 Mon Sep 17 00:00:00 2001 From: Ilya Drey Date: Tue, 1 Sep 2026 19:31:27 +0300 Subject: [PATCH 01/33] feat(core): add repository sync timestamps and kstatus condition types Signed-off-by: Ilya Drey --- api/v1alpha1/conditions.go | 16 +++++++ api/v1alpha1/helm_cluster_addon_repository.go | 25 ++++++++++ crds/doc-ru-helmclusteraddonrepositories.yaml | 15 +++++- crds/helmclusteraddonrepositories.yaml | 47 ++++++++++++++++++- 4 files changed, 100 insertions(+), 3 deletions(-) diff --git a/api/v1alpha1/conditions.go b/api/v1alpha1/conditions.go index fd34bd3e..efaffe7d 100644 --- a/api/v1alpha1/conditions.go +++ b/api/v1alpha1/conditions.go @@ -26,6 +26,10 @@ const ( ConditionTypeSynced = "Synced" ConditionTypeUninstallFailed = "UninstallFailed" + // kstatus abnormal-true conditions. Present only while applicable. + ConditionTypeReconciling = "Reconciling" + ConditionTypeStalled = "Stalled" + ReasonMaintenanceModeActive = "MaintenanceModeActive" ReasonMaintenanceModeInactive = "MaintenanceModeInactive" ReasonSyncFailed = "SyncFailed" @@ -36,6 +40,18 @@ const ( ReasonUninstallFailed = "UninstallFailed" ReasonChartClaimConflict = "ChartClaimConflict" + // HelmClusterAddonRepository condition reasons. + ReasonAuxiliaryResourcesFailed = "AuxiliaryResourcesFailed" + ReasonCatalogUpdateFailed = "CatalogUpdateFailed" + ReasonAwaitingInitialSync = "AwaitingInitialSync" + ReasonProgressingWithRetry = "ProgressingWithRetry" + ReasonRetriesExceeded = "RetriesExceeded" + ReasonAuthenticationFailed = "AuthenticationFailed" + ReasonSourceNotFound = "SourceNotFound" + ReasonSourceRejectedRequest = "SourceRejectedRequest" + ReasonInvalidRepositoryURL = "InvalidRepositoryURL" + ReasonUnsupportedRepositoryType = "UnsupportedRepositoryType" + // HelmRelease error reasons ReasonReleaseFailed = "ReleaseFailed" ReasonTestFailed = "TestFailed" diff --git a/api/v1alpha1/helm_cluster_addon_repository.go b/api/v1alpha1/helm_cluster_addon_repository.go index f4466faa..ada390e4 100644 --- a/api/v1alpha1/helm_cluster_addon_repository.go +++ b/api/v1alpha1/helm_cluster_addon_repository.go @@ -36,6 +36,10 @@ const ( // +kubebuilder:resource:singular=helmclusteraddonrepository,scope=Cluster // +kubebuilder:printcolumn:name="Status",type="string",JSONPath=".status.conditions[?(@.type=='Ready')].status",description="The readiness status of the repository" // +kubebuilder:printcolumn:name="Synced",type="string",JSONPath=".status.conditions[?(@.type=='Synced')].status",description="Repository synchronization status" +// +kubebuilder:printcolumn:name="Last Sync",type="date",JSONPath=".status.lastSuccessfulSyncTime",description="Time of the last successful catalog synchronization" +// +kubebuilder:printcolumn:name="Age",type="date",JSONPath=".metadata.creationTimestamp" +// +kubebuilder:printcolumn:name="Next Sync",type="date",JSONPath=".status.nextSyncTime",priority=1,description="Scheduled time of the next synchronization attempt" +// +kubebuilder:printcolumn:name="Message",type="string",JSONPath=".status.conditions[?(@.type=='Ready')].message",priority=1 // +genclient // +genclient:nonNamespaced // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object @@ -108,10 +112,31 @@ type HelmClusterAddonRepositoryAuth struct { type HelmClusterAddonRepositoryStatus struct { // Conditions represent the latest available observations of the repository state. + // + // Ready reports whether the repository is usable: auxiliary resources are in place, + // the internal source object is healthy and the repository has responded to a catalog + // read on the current spec. A transient read failure does not flip Ready to False. + // + // Synced reports whether the chart catalog is up to date. + // + // Reconciling and Stalled follow the kstatus convention: they are present only while + // applicable. Reconciling means work is in progress or a retry is scheduled; Stalled + // means the repository will not recover without a change. // +optional Conditions []metav1.Condition `json:"conditions,omitempty"` // Generation represents resource generation that was last processed by the controller. ObservedGeneration int64 `json:"observedGeneration,omitempty"` + // LastSuccessfulSyncTime is the last time the chart catalog was fully brought up to date, + // including creating and pruning chart resources. + // +optional + LastSuccessfulSyncTime *metav1.Time `json:"lastSuccessfulSyncTime,omitempty"` + // NextSyncTime is the scheduled time of the next synchronization attempt. + // +optional + NextSyncTime *metav1.Time `json:"nextSyncTime,omitempty"` + // ConsecutiveFetchFailures counts consecutive failures to read from the repository. + // It drives the retry backoff and resets on the first success. + // +optional + ConsecutiveFetchFailures int32 `json:"consecutiveFetchFailures,omitempty"` } // HelmClusterAddonRepositoryList contains a list of HelmClusterAddonRepositories. diff --git a/crds/doc-ru-helmclusteraddonrepositories.yaml b/crds/doc-ru-helmclusteraddonrepositories.yaml index f1d31128..c060c75f 100644 --- a/crds/doc-ru-helmclusteraddonrepositories.yaml +++ b/crds/doc-ru-helmclusteraddonrepositories.yaml @@ -31,6 +31,19 @@ spec: status: properties: conditions: - description: Условия отражают последние наблюдения за состоянием репозитория. + description: | + Условия отражают последние наблюдения за состоянием репозитория. + + `Ready` сообщает, пригоден ли репозиторий: вспомогательные ресурсы на месте, внутренний объект источника исправен, и репозиторий ответил на чтение каталога на текущей спецификации. Транзиентная ошибка чтения не переводит `Ready` в `False`. + + `Synced` сообщает, актуален ли каталог чартов. + + `Reconciling` и `Stalled` следуют соглашению kstatus: они присутствуют, только когда применимы. `Reconciling` означает, что работа выполняется или запланирован повтор; `Stalled` — что репозиторий не восстановится без вмешательства. observedGeneration: description: Поколение ресурса, обработанное контроллером последним. + lastSuccessfulSyncTime: + description: Время последнего успешного приведения каталога чартов в актуальное состояние. + nextSyncTime: + description: Запланированное время следующей попытки синхронизации. + consecutiveFetchFailures: + description: Число подряд идущих неудачных обращений к репозиторию. Определяет задержку повтора и обнуляется при первом успехе. diff --git a/crds/helmclusteraddonrepositories.yaml b/crds/helmclusteraddonrepositories.yaml index 4b20d5a3..28cbcad1 100644 --- a/crds/helmclusteraddonrepositories.yaml +++ b/crds/helmclusteraddonrepositories.yaml @@ -26,6 +26,22 @@ spec: jsonPath: .status.conditions[?(@.type=='Synced')].status name: Synced type: string + - description: Time of the last successful catalog synchronization + jsonPath: .status.lastSuccessfulSyncTime + name: Last Sync + type: date + - jsonPath: .metadata.creationTimestamp + name: Age + type: date + - description: Scheduled time of the next synchronization attempt + jsonPath: .status.nextSyncTime + name: Next Sync + priority: 1 + type: date + - jsonPath: .status.conditions[?(@.type=='Ready')].message + name: Message + priority: 1 + type: string name: v1alpha1 schema: openAPIV3Schema: @@ -88,8 +104,18 @@ spec: status: properties: conditions: - description: Conditions represent the latest available observations - of the repository state. + description: |- + Conditions represent the latest available observations of the repository state. + + Ready reports whether the repository is usable: auxiliary resources are in place, + the internal source object is healthy and the repository has responded to a catalog + read on the current spec. A transient read failure does not flip Ready to False. + + Synced reports whether the chart catalog is up to date. + + Reconciling and Stalled follow the kstatus convention: they are present only while + applicable. Reconciling means work is in progress or a retry is scheduled; Stalled + means the repository will not recover without a change. items: description: Condition contains details for one aspect of the current state of this API Resource. @@ -145,6 +171,23 @@ spec: - type type: object type: array + consecutiveFetchFailures: + description: |- + ConsecutiveFetchFailures counts consecutive failures to read from the repository. + It drives the retry backoff and resets on the first success. + format: int32 + type: integer + lastSuccessfulSyncTime: + description: |- + LastSuccessfulSyncTime is the last time the chart catalog was fully brought up to date, + including creating and pruning chart resources. + format: date-time + type: string + nextSyncTime: + description: NextSyncTime is the scheduled time of the next synchronization + attempt. + format: date-time + type: string observedGeneration: description: Generation represents resource generation that was last processed by the controller. From 160259934e49e8e34e31ff42037098f81a7c8651 Mon Sep 17 00:00:00 2001 From: Ilya Drey Date: Tue, 1 Sep 2026 19:33:34 +0300 Subject: [PATCH 02/33] chore(core): regenerate api artifacts for repository status fields Signed-off-by: Ilya Drey --- api/v1alpha1/zz_generated.deepcopy.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go index 36a80652..e5ec01d8 100644 --- a/api/v1alpha1/zz_generated.deepcopy.go +++ b/api/v1alpha1/zz_generated.deepcopy.go @@ -332,6 +332,14 @@ func (in *HelmClusterAddonRepositoryStatus) DeepCopyInto(out *HelmClusterAddonRe (*in)[i].DeepCopyInto(&(*out)[i]) } } + if in.LastSuccessfulSyncTime != nil { + in, out := &in.LastSuccessfulSyncTime, &out.LastSuccessfulSyncTime + *out = (*in).DeepCopy() + } + if in.NextSyncTime != nil { + in, out := &in.NextSyncTime, &out.NextSyncTime + *out = (*in).DeepCopy() + } return } From 52a85705c2daae4bbe5ac27a020f0f913fb134a6 Mon Sep 17 00:00:00 2001 From: Ilya Drey Date: Tue, 1 Sep 2026 19:39:37 +0300 Subject: [PATCH 03/33] feat(core): classify terminal helm repository errors and skip invalid chart versions Signed-off-by: Ilya Drey --- .../internal/client/repository/errors.go | 79 +++++++++++++ .../internal/client/repository/helm.go | 13 ++- .../internal/client/repository/helm_test.go | 106 ++++++++++++++++++ 3 files changed, 195 insertions(+), 3 deletions(-) create mode 100644 images/operator-helm-controller/internal/client/repository/errors.go create mode 100644 images/operator-helm-controller/internal/client/repository/helm_test.go diff --git a/images/operator-helm-controller/internal/client/repository/errors.go b/images/operator-helm-controller/internal/client/repository/errors.go new file mode 100644 index 00000000..5cd70fd7 --- /dev/null +++ b/images/operator-helm-controller/internal/client/repository/errors.go @@ -0,0 +1,79 @@ +/* +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 ( + "errors" + "fmt" + "net/http" + + helmv1alpha1 "github.com/deckhouse/operator-helm/api/v1alpha1" +) + +// TerminalError marks a repository failure that will not resolve by retrying: +// the remote rejected the request or the configuration is invalid. Callers use +// it to move the repository to Stalled instead of scheduling another attempt at +// the normal cadence. +type TerminalError struct { + Reason string + Message string + Err error +} + +func (e *TerminalError) Error() string { + if e.Err == nil { + return e.Message + } + + return fmt.Sprintf("%s: %s", e.Message, e.Err) +} + +func (e *TerminalError) Unwrap() error { return e.Err } + +// AsTerminal reports whether err wraps a TerminalError and returns it. +func AsTerminal(err error) (*TerminalError, bool) { + var terminal *TerminalError + if errors.As(err, &terminal) { + return terminal, true + } + + return nil, false +} + +// TerminalFromStatusCode maps a rejection status code to a terminal error and +// returns nil for codes that are worth retrying. +func TerminalFromStatusCode(code int, url string) *TerminalError { + switch { + case code == http.StatusUnauthorized, code == http.StatusForbidden: + return &TerminalError{ + Reason: helmv1alpha1.ReasonAuthenticationFailed, + Message: fmt.Sprintf("repository %s rejected the credentials (HTTP %d)", url, code), + } + case code == http.StatusNotFound: + return &TerminalError{ + Reason: helmv1alpha1.ReasonSourceNotFound, + Message: fmt.Sprintf("repository %s not found (HTTP %d)", url, code), + } + case code >= 400 && code < 500: + return &TerminalError{ + Reason: helmv1alpha1.ReasonSourceRejectedRequest, + Message: fmt.Sprintf("repository %s rejected the request (HTTP %d)", url, code), + } + default: + return nil + } +} diff --git a/images/operator-helm-controller/internal/client/repository/helm.go b/images/operator-helm-controller/internal/client/repository/helm.go index 0e1dc34e..d81eef22 100644 --- a/images/operator-helm-controller/internal/client/repository/helm.go +++ b/images/operator-helm-controller/internal/client/repository/helm.go @@ -26,6 +26,7 @@ import ( "go.yaml.in/yaml/v3" "k8s.io/apimachinery/pkg/util/wait" + "sigs.k8s.io/controller-runtime/pkg/log" "github.com/Masterminds/semver/v3" ) @@ -88,8 +89,8 @@ func (c *helmRepositoryClient) FetchCharts(ctx context.Context, url string, conf return false, nil } - if resp.StatusCode >= 400 { - return true, fmt.Errorf("fatal client error: received status %d", resp.StatusCode) + if terminal := TerminalFromStatusCode(resp.StatusCode, url); terminal != nil { + return true, terminal } if err := yaml.NewDecoder(resp.Body).Decode(&indexFile); err != nil { @@ -114,7 +115,13 @@ func (c *helmRepositoryClient) FetchCharts(ctx context.Context, url string, conf semVersion, err := semver.NewVersion(chartVersion.Version) if err != nil { - return nil, fmt.Errorf("failed to parse chart %q version %q: %w", chartName, chartVersion.Version, err) + // A single malformed entry must not cost the whole catalog: the OCI + // client already skips such tags, and the repository owner may publish + // non-semver artifacts we simply cannot address. + log.FromContext(ctx).V(1).Info("Skipping chart version that is not valid semver", + "chart", chartName, "version", chartVersion.Version) + + continue } chart.Versions = append(chart.Versions, ChartVersion{Version: semVersion, IconURL: chartVersion.Icon}) diff --git a/images/operator-helm-controller/internal/client/repository/helm_test.go b/images/operator-helm-controller/internal/client/repository/helm_test.go new file mode 100644 index 00000000..3ae579f0 --- /dev/null +++ b/images/operator-helm-controller/internal/client/repository/helm_test.go @@ -0,0 +1,106 @@ +/* +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 ( + "context" + "net/http" + "net/http/httptest" + "testing" + + helmv1alpha1 "github.com/deckhouse/operator-helm/api/v1alpha1" +) + +const testIndex = `apiVersion: v1 +entries: + podinfo: + - version: 6.7.1 + icon: https://example.invalid/icon.png + - version: not-a-semver + - version: 6.7.0 +` + +func TestFetchChartsTerminalStatusCodes(t *testing.T) { + cases := []struct { + name string + statusCode int + wantReason string + }{ + {"unauthorized", http.StatusUnauthorized, helmv1alpha1.ReasonAuthenticationFailed}, + {"forbidden", http.StatusForbidden, helmv1alpha1.ReasonAuthenticationFailed}, + {"not found", http.StatusNotFound, helmv1alpha1.ReasonSourceNotFound}, + {"teapot", http.StatusTeapot, helmv1alpha1.ReasonSourceRejectedRequest}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(tc.statusCode) + })) + defer srv.Close() + + _, err := HelmRepositoryDefaultClient.FetchCharts(context.Background(), srv.URL, nil) + if err == nil { + t.Fatalf("expected error for status %d", tc.statusCode) + } + + terminal, ok := AsTerminal(err) + if !ok { + t.Fatalf("expected terminal error, got %v", err) + } + if terminal.Reason != tc.wantReason { + t.Fatalf("expected reason %q, got %q", tc.wantReason, terminal.Reason) + } + }) + } +} + +func TestFetchChartsServerErrorIsNotTerminal(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + })) + defer srv.Close() + + _, err := HelmRepositoryDefaultClient.FetchCharts(context.Background(), srv.URL, nil) + if err == nil { + t.Fatal("expected error for repeated 500 responses") + } + if _, ok := AsTerminal(err); ok { + t.Fatalf("5xx must stay retriable, got terminal error: %v", err) + } +} + +func TestFetchChartsSkipsInvalidVersion(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte(testIndex)) + })) + defer srv.Close() + + charts, err := HelmRepositoryDefaultClient.FetchCharts(context.Background(), srv.URL, nil) + if err != nil { + t.Fatalf("expected invalid version to be skipped, got error: %v", err) + } + if len(charts) != 1 { + t.Fatalf("expected 1 chart, got %d", len(charts)) + } + if len(charts[0].Versions) != 2 { + t.Fatalf("expected 2 valid versions, got %d", len(charts[0].Versions)) + } + if got := charts[0].Versions[0].Version.Original(); got != "6.7.1" { + t.Fatalf("expected newest version first, got %q", got) + } +} From ce711052ad72709b9b992f8bba62897f8f5dd51a Mon Sep 17 00:00:00 2001 From: Ilya Drey Date: Tue, 1 Sep 2026 19:46:08 +0300 Subject: [PATCH 04/33] feat(core): classify terminal OCI repository errors Signed-off-by: Ilya Drey --- .../internal/client/repository/oci.go | 37 +++++++- .../internal/client/repository/oci_test.go | 91 +++++++++++++++++++ 2 files changed, 124 insertions(+), 4 deletions(-) create mode 100644 images/operator-helm-controller/internal/client/repository/oci_test.go diff --git a/images/operator-helm-controller/internal/client/repository/oci.go b/images/operator-helm-controller/internal/client/repository/oci.go index 4e02b01c..e20e4e3d 100644 --- a/images/operator-helm-controller/internal/client/repository/oci.go +++ b/images/operator-helm-controller/internal/client/repository/oci.go @@ -27,6 +27,9 @@ import ( "github.com/google/go-containerregistry/pkg/authn" "github.com/google/go-containerregistry/pkg/name" "github.com/google/go-containerregistry/pkg/v1/remote" + "github.com/google/go-containerregistry/pkg/v1/remote/transport" + + helmv1alpha1 "github.com/deckhouse/operator-helm/api/v1alpha1" ) var OCIRepositoryDefaultClient ClientInterface = &ociRepositoryClient{} @@ -38,19 +41,29 @@ func (c *ociRepositoryClient) FetchCharts(ctx context.Context, url string, confi url = strings.TrimSuffix(url, "/") if !strings.Contains(url, "/") { - return nil, errors.New("url must contain chart/image name") + return nil, &TerminalError{ + Reason: helmv1alpha1.ReasonInvalidRepositoryURL, + Message: "repository url must contain the chart image name", + } } urlParts := strings.Split(url, "/") chartName := urlParts[len(urlParts)-1] if len(chartName) == 0 { - return nil, errors.New("failed to parse chart/image name from the url") + return nil, &TerminalError{ + Reason: helmv1alpha1.ReasonInvalidRepositoryURL, + Message: "cannot parse the chart image name from the repository url", + } } repo, err := name.NewRepository(url) if err != nil { - return nil, fmt.Errorf("failed to parse repository url: %w", err) + return nil, &TerminalError{ + Reason: helmv1alpha1.ReasonInvalidRepositoryURL, + Message: "cannot parse the repository url", + Err: err, + } } options := []remote.Option{ @@ -77,7 +90,7 @@ func (c *ociRepositoryClient) FetchCharts(ctx context.Context, url string, confi tags, err := remote.List(repo, options...) if err != nil { - return nil, fmt.Errorf("listing image tags: %w", err) + return nil, classifyRemoteError(err, url) } var chartVersions []ChartVersion @@ -125,3 +138,19 @@ func isCosignTag(tag string) bool { return false } + +// classifyRemoteError maps a registry rejection to a terminal error and leaves +// transport-level failures retriable. The original error is always wrapped so +// callers keep the full cause. +func classifyRemoteError(err error, url string) error { + var transportErr *transport.Error + if errors.As(err, &transportErr) { + if terminal := TerminalFromStatusCode(transportErr.StatusCode, url); terminal != nil { + terminal.Err = err + + return terminal + } + } + + return fmt.Errorf("listing image tags: %w", err) +} diff --git a/images/operator-helm-controller/internal/client/repository/oci_test.go b/images/operator-helm-controller/internal/client/repository/oci_test.go new file mode 100644 index 00000000..5884ac8d --- /dev/null +++ b/images/operator-helm-controller/internal/client/repository/oci_test.go @@ -0,0 +1,91 @@ +/* +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 ( + "context" + "errors" + "net/http" + "testing" + + "github.com/google/go-containerregistry/pkg/v1/remote/transport" + + helmv1alpha1 "github.com/deckhouse/operator-helm/api/v1alpha1" +) + +func TestFetchChartsOCIRejectsURLWithoutImageName(t *testing.T) { + _, err := OCIRepositoryDefaultClient.FetchCharts(context.Background(), "oci://ghcr.io", nil) + if err == nil { + t.Fatal("expected error for url without an image name") + } + + terminal, ok := AsTerminal(err) + if !ok { + t.Fatalf("expected terminal error, got %v", err) + } + if terminal.Reason != helmv1alpha1.ReasonInvalidRepositoryURL { + t.Fatalf("expected reason %q, got %q", helmv1alpha1.ReasonInvalidRepositoryURL, terminal.Reason) + } +} + +func TestClassifyRemoteError(t *testing.T) { + cases := []struct { + name string + err error + wantTerminal bool + wantReason string + }{ + { + name: "unauthorized is terminal", + err: &transport.Error{StatusCode: http.StatusUnauthorized}, + wantTerminal: true, + wantReason: helmv1alpha1.ReasonAuthenticationFailed, + }, + { + name: "not found is terminal", + err: &transport.Error{StatusCode: http.StatusNotFound}, + wantTerminal: true, + wantReason: helmv1alpha1.ReasonSourceNotFound, + }, + { + name: "server error is retriable", + err: &transport.Error{StatusCode: http.StatusBadGateway}, + wantTerminal: false, + }, + { + name: "transport failure is retriable", + err: errors.New("dial tcp: connection refused"), + wantTerminal: false, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got := classifyRemoteError(tc.err, "ghcr.io/example/chart") + terminal, ok := AsTerminal(got) + if ok != tc.wantTerminal { + t.Fatalf("terminal=%v, want %v (err %v)", ok, tc.wantTerminal, got) + } + if tc.wantTerminal && terminal.Reason != tc.wantReason { + t.Fatalf("expected reason %q, got %q", tc.wantReason, terminal.Reason) + } + if !errors.Is(got, tc.err) { + t.Fatalf("classified error must wrap the original, got %v", got) + } + }) + } +} From 6bed0053fa7d0c636af69e37744427984d9f3572 Mon Sep 17 00:00:00 2001 From: Ilya Drey Date: Tue, 1 Sep 2026 19:56:54 +0300 Subject: [PATCH 05/33] feat(core): derive repository conditions in a pure evaluate function Signed-off-by: Ilya Drey --- .../helmclusteraddonrepository/evaluate.go | 324 ++++++++++++++++++ .../evaluate_test.go | 307 +++++++++++++++++ .../internal/services/outcomes.go | 56 +++ 3 files changed, 687 insertions(+) create mode 100644 images/operator-helm-controller/internal/reconcile/helmclusteraddonrepository/evaluate.go create mode 100644 images/operator-helm-controller/internal/reconcile/helmclusteraddonrepository/evaluate_test.go create mode 100644 images/operator-helm-controller/internal/services/outcomes.go diff --git a/images/operator-helm-controller/internal/reconcile/helmclusteraddonrepository/evaluate.go b/images/operator-helm-controller/internal/reconcile/helmclusteraddonrepository/evaluate.go new file mode 100644 index 00000000..e5d77427 --- /dev/null +++ b/images/operator-helm-controller/internal/reconcile/helmclusteraddonrepository/evaluate.go @@ -0,0 +1,324 @@ +/* +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 helmclusteraddonrepository + +import ( + "time" + + apimeta "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + helmv1alpha1 "github.com/deckhouse/operator-helm/api/v1alpha1" + "github.com/deckhouse/operator-helm/internal/services" +) + +// Inputs carries everything Evaluate needs. It holds no clients and no clock: +// Now and Jitter are supplied by the caller so the function is deterministic +// and unit testable. +type Inputs struct { + Generation int64 + Now time.Time + Jitter float64 + Current helmv1alpha1.HelmClusterAddonRepositoryStatus + + SecretsErr error + InternalRepositoryErr error + InternalRepository services.InternalRepositoryState + ConfigErr *services.ConfigOutcome + + // Attempted reports whether a synchronization attempt ran in this pass. + // Fetch and Catalog are nil when it did not. + Attempted bool + Fetch *services.FetchOutcome + Catalog *services.CatalogOutcome +} + +// Decision is the full desired status plus the scheduling verdict. Removing a +// condition is expressed by its absence from Status.Conditions. +type Decision struct { + Status helmv1alpha1.HelmClusterAddonRepositoryStatus + RequeueAfter time.Duration + Err error +} + +// abnormalCondition is an optional abnormal-true condition. +type abnormalCondition struct { + set bool + reason string + message string +} + +// Evaluate derives the desired repository status from the results of a single +// reconcile pass. +func Evaluate(in Inputs) Decision { + var status helmv1alpha1.HelmClusterAddonRepositoryStatus + in.Current.DeepCopyInto(&status) + status.ObservedGeneration = in.Generation + + internal := in.InternalRepository + if in.InternalRepositoryErr != nil { + // A failure to reconcile the internal object is a structural failure: + // report it the same way as an unhealthy object so Ready reflects it. + internal = services.InternalRepositoryState{ + Present: true, + Reason: helmv1alpha1.ReasonFailed, + Message: in.InternalRepositoryErr.Error(), + } + } + + fetchFailed := in.Attempted && in.Fetch != nil && in.Fetch.Err != nil + fetchSucceeded := in.Attempted && in.Fetch != nil && in.Fetch.Err == nil + catalogFailed := in.Attempted && in.Catalog != nil && in.Catalog.Err != nil + + failures := in.Current.ConsecutiveFetchFailures + if in.Generation != in.Current.ObservedGeneration { + // The spec changed: previous failures were about the previous source. + failures = 0 + } + failures = nextFailureCount(failures, in.Attempted, fetchFailed, in.Fetch) + + stalled := evaluateStalled(in, internal, failures, fetchFailed) + ready := evaluateReady(in, internal, stalled, fetchSucceeded) + reconciling := evaluateReconciling(in, internal, stalled, ready, failures, catalogFailed) + + setCondition(&status, in, helmv1alpha1.ConditionTypeReady, ready.Status, ready.Reason, ready.Message) + + if in.Attempted { + syncedStatus, syncedReason, syncedMessage := evaluateSynced(in, fetchFailed, catalogFailed) + setCondition(&status, in, helmv1alpha1.ConditionTypeSynced, syncedStatus, syncedReason, syncedMessage) + } + + applyAbnormal(&status, in, helmv1alpha1.ConditionTypeReconciling, reconciling) + applyAbnormal(&status, in, helmv1alpha1.ConditionTypeStalled, stalled) + + status.ConsecutiveFetchFailures = failures + + return Decision{ + Status: status, + Err: firstErr(in.SecretsErr, in.InternalRepositoryErr, catalogErr(in)), + } +} + +func evaluateReady( + in Inputs, + internal services.InternalRepositoryState, + stalled abnormalCondition, + fetchSucceeded bool, +) metav1.Condition { + switch { + case in.SecretsErr != nil: + return metav1.Condition{ + Status: metav1.ConditionFalse, + Reason: helmv1alpha1.ReasonAuxiliaryResourcesFailed, + Message: "Failed to reconcile auxiliary resources: " + in.SecretsErr.Error(), + } + case internal.Present && !internal.Ready: + return metav1.Condition{ + Status: metav1.ConditionFalse, + Reason: internal.Reason, + Message: internal.Message, + } + case stalled.set: + return metav1.Condition{ + Status: metav1.ConditionFalse, + Reason: stalled.reason, + Message: stalled.message, + } + case fetchSucceeded: + return metav1.Condition{Status: metav1.ConditionTrue, Reason: helmv1alpha1.ReasonSuccess} + case hasEvidence(in.Current, in.Generation): + return metav1.Condition{Status: metav1.ConditionTrue, Reason: helmv1alpha1.ReasonSuccess} + default: + return metav1.Condition{ + Status: metav1.ConditionUnknown, + Reason: helmv1alpha1.ReasonAwaitingInitialSync, + Message: "Waiting for the first successful repository read", + } + } +} + +func evaluateSynced(in Inputs, fetchFailed, catalogFailed bool) (metav1.ConditionStatus, string, string) { + switch { + case fetchFailed: + return metav1.ConditionFalse, helmv1alpha1.ReasonSyncFailed, in.Fetch.Message + case catalogFailed: + return metav1.ConditionFalse, helmv1alpha1.ReasonCatalogUpdateFailed, + "Failed to update the chart catalog: " + in.Catalog.Err.Error() + default: + return metav1.ConditionTrue, helmv1alpha1.ReasonSuccess, "" + } +} + +func evaluateStalled( + in Inputs, + internal services.InternalRepositoryState, + failures int32, + fetchFailed bool, +) abnormalCondition { + switch { + case in.ConfigErr != nil: + return abnormalCondition{set: true, reason: in.ConfigErr.Reason, message: in.ConfigErr.Message} + case internal.Present && internal.Stalled: + return abnormalCondition{set: true, reason: internal.Reason, message: internal.Message} + case !in.Attempted: + // A pass without an attempt carries the previous verdict forward so the + // specific reason is not replaced by a generic one. + if cond := apimeta.FindStatusCondition(in.Current.Conditions, helmv1alpha1.ConditionTypeStalled); cond != nil && + cond.Status == metav1.ConditionTrue { + return abnormalCondition{set: true, reason: cond.Reason, message: cond.Message} + } + + return abnormalCondition{} + case fetchFailed && in.Fetch.Terminal: + return abnormalCondition{set: true, reason: in.Fetch.Reason, message: in.Fetch.Message} + case failures >= MaxFetchFailures: + return abnormalCondition{ + set: true, + reason: helmv1alpha1.ReasonRetriesExceeded, + message: "Giving up on the repository after repeated read failures", + } + default: + return abnormalCondition{} + } +} + +func evaluateReconciling( + in Inputs, + internal services.InternalRepositoryState, + stalled abnormalCondition, + ready metav1.Condition, + failures int32, + catalogFailed bool, +) abnormalCondition { + switch { + case stalled.set: + return abnormalCondition{} + case internal.Present && !internal.Ready: + return abnormalCondition{set: true, reason: helmv1alpha1.ReasonReconciling, message: internal.Message} + case in.SecretsErr != nil: + return abnormalCondition{ + set: true, + reason: helmv1alpha1.ReasonProgressingWithRetry, + message: "Retrying after an auxiliary resource failure", + } + case catalogFailed: + return abnormalCondition{ + set: true, + reason: helmv1alpha1.ReasonProgressingWithRetry, + message: "Retrying after a chart catalog update failure", + } + case failures > 0: + return abnormalCondition{ + set: true, + reason: helmv1alpha1.ReasonProgressingWithRetry, + message: "Retrying the repository read", + } + case ready.Status == metav1.ConditionUnknown: + return abnormalCondition{ + set: true, + reason: helmv1alpha1.ReasonAwaitingInitialSync, + message: ready.Message, + } + default: + return abnormalCondition{} + } +} + +// hasEvidence reports whether the repository is already proven usable on the +// current generation: the previous verdict was True and was made for this spec. +func hasEvidence(current helmv1alpha1.HelmClusterAddonRepositoryStatus, generation int64) bool { + cond := apimeta.FindStatusCondition(current.Conditions, helmv1alpha1.ConditionTypeReady) + + return cond != nil && cond.Status == metav1.ConditionTrue && cond.ObservedGeneration == generation +} + +func setCondition( + status *helmv1alpha1.HelmClusterAddonRepositoryStatus, + in Inputs, + conditionType string, + conditionStatus metav1.ConditionStatus, + reason, message string, +) { + apimeta.SetStatusCondition(&status.Conditions, metav1.Condition{ + Type: conditionType, + Status: conditionStatus, + Reason: reason, + Message: message, + ObservedGeneration: in.Generation, + LastTransitionTime: metav1.NewTime(in.Now), + }) +} + +func applyAbnormal( + status *helmv1alpha1.HelmClusterAddonRepositoryStatus, + in Inputs, + conditionType string, + cond abnormalCondition, +) { + if !cond.set { + apimeta.RemoveStatusCondition(&status.Conditions, conditionType) + + return + } + + setCondition(status, in, conditionType, metav1.ConditionTrue, cond.reason, cond.message) +} + +func firstErr(errs ...error) error { + for _, err := range errs { + if err != nil { + return err + } + } + + return nil +} + +func catalogErr(in Inputs) error { + if in.Catalog == nil { + return nil + } + + return in.Catalog.Err +} + +// MaxFetchFailures is the number of consecutive read failures after which the +// repository is reported as Stalled. Reaching it does not stop the retries: +// the cause may disappear on the remote side. +const MaxFetchFailures = 5 + +func nextFailureCount(failures int32, attempted, fetchFailed bool, fetch *services.FetchOutcome) int32 { + if !attempted { + return failures + } + + if !fetchFailed { + return 0 + } + + if fetch.Terminal { + // A terminal failure is reported immediately; saturating the counter keeps + // a single formula for the schedule and puts the retry at the cap. + return MaxFetchFailures + } + + if failures < MaxFetchFailures { + return failures + 1 + } + + return failures +} diff --git a/images/operator-helm-controller/internal/reconcile/helmclusteraddonrepository/evaluate_test.go b/images/operator-helm-controller/internal/reconcile/helmclusteraddonrepository/evaluate_test.go new file mode 100644 index 00000000..21be8083 --- /dev/null +++ b/images/operator-helm-controller/internal/reconcile/helmclusteraddonrepository/evaluate_test.go @@ -0,0 +1,307 @@ +/* +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 helmclusteraddonrepository + +import ( + "errors" + "testing" + "time" + + apimeta "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + helmv1alpha1 "github.com/deckhouse/operator-helm/api/v1alpha1" + "github.com/deckhouse/operator-helm/internal/services" +) + +var testNow = time.Date(2026, 9, 1, 12, 0, 0, 0, time.UTC) + +// readyStatus builds a status that already carries proven Ready for the given generation. +func readyStatus(generation int64) helmv1alpha1.HelmClusterAddonRepositoryStatus { + return helmv1alpha1.HelmClusterAddonRepositoryStatus{ + ObservedGeneration: generation, + Conditions: []metav1.Condition{ + { + Type: helmv1alpha1.ConditionTypeReady, + Status: metav1.ConditionTrue, + Reason: helmv1alpha1.ReasonSuccess, + ObservedGeneration: generation, + LastTransitionTime: metav1.NewTime(testNow.Add(-time.Hour)), + }, + { + Type: helmv1alpha1.ConditionTypeSynced, + Status: metav1.ConditionTrue, + Reason: helmv1alpha1.ReasonSuccess, + ObservedGeneration: generation, + LastTransitionTime: metav1.NewTime(testNow.Add(-time.Hour)), + }, + }, + } +} + +func conditionOf(t *testing.T, status helmv1alpha1.HelmClusterAddonRepositoryStatus, conditionType string) *metav1.Condition { + t.Helper() + + return apimeta.FindStatusCondition(status.Conditions, conditionType) +} + +func TestEvaluateConditions(t *testing.T) { + fetchErr := errors.New("connection refused") + writeErr := errors.New("etcdserver: request timed out") + + cases := []struct { + name string + in Inputs + + wantReady metav1.ConditionStatus + wantReadyReason string + wantSynced metav1.ConditionStatus + wantReconciling string // reason; "" means the condition must be absent + wantStalled string // reason; "" means the condition must be absent + }{ + { + name: "healthy repository", + in: Inputs{ + Generation: 1, Now: testNow, Current: readyStatus(1), + InternalRepository: services.InternalRepositoryState{Present: true, Ready: true}, + Attempted: true, + Fetch: &services.FetchOutcome{}, + Catalog: &services.CatalogOutcome{}, + }, + wantReady: metav1.ConditionTrue, wantReadyReason: helmv1alpha1.ReasonSuccess, + wantSynced: metav1.ConditionTrue, + }, + { + name: "awaiting first sync on a fresh object", + in: Inputs{ + Generation: 1, Now: testNow, + InternalRepository: services.InternalRepositoryState{Present: true, Ready: true}, + }, + wantReady: metav1.ConditionUnknown, wantReadyReason: helmv1alpha1.ReasonAwaitingInitialSync, + wantSynced: "", + wantReconciling: helmv1alpha1.ReasonAwaitingInitialSync, + }, + { + name: "auxiliary resources failed", + in: Inputs{ + Generation: 1, Now: testNow, Current: readyStatus(1), + SecretsErr: writeErr, + }, + wantReady: metav1.ConditionFalse, wantReadyReason: helmv1alpha1.ReasonAuxiliaryResourcesFailed, + wantSynced: metav1.ConditionTrue, + wantReconciling: helmv1alpha1.ReasonProgressingWithRetry, + }, + { + name: "internal repository not ready", + in: Inputs{ + Generation: 1, Now: testNow, Current: readyStatus(1), + InternalRepository: services.InternalRepositoryState{ + Present: true, Ready: false, + Reason: "FetchFailed", Message: "failed to fetch index", + }, + Attempted: true, + Fetch: &services.FetchOutcome{}, + Catalog: &services.CatalogOutcome{}, + }, + wantReady: metav1.ConditionFalse, wantReadyReason: "FetchFailed", + wantSynced: metav1.ConditionTrue, + wantReconciling: helmv1alpha1.ReasonReconciling, + }, + { + name: "internal repository stalled", + in: Inputs{ + Generation: 1, Now: testNow, Current: readyStatus(1), + InternalRepository: services.InternalRepositoryState{ + Present: true, Ready: false, Stalled: true, + Reason: "InvalidSecretRef", Message: "secret not found", + }, + Attempted: true, + Fetch: &services.FetchOutcome{}, + Catalog: &services.CatalogOutcome{}, + }, + wantReady: metav1.ConditionFalse, wantReadyReason: "InvalidSecretRef", + wantSynced: metav1.ConditionTrue, + wantStalled: "InvalidSecretRef", + }, + { + name: "transient fetch failure keeps Ready latched", + in: Inputs{ + Generation: 1, Now: testNow, Current: readyStatus(1), + InternalRepository: services.InternalRepositoryState{Present: true, Ready: true}, + Attempted: true, + Fetch: &services.FetchOutcome{Err: fetchErr, Message: "cannot read index.yaml"}, + }, + wantReady: metav1.ConditionTrue, wantReadyReason: helmv1alpha1.ReasonSuccess, + wantSynced: metav1.ConditionFalse, + wantReconciling: helmv1alpha1.ReasonProgressingWithRetry, + }, + { + name: "transient fetch failure without evidence", + in: Inputs{ + Generation: 1, Now: testNow, + InternalRepository: services.InternalRepositoryState{Present: true, Ready: true}, + Attempted: true, + Fetch: &services.FetchOutcome{Err: fetchErr, Message: "cannot read index.yaml"}, + }, + wantReady: metav1.ConditionUnknown, wantReadyReason: helmv1alpha1.ReasonAwaitingInitialSync, + wantSynced: metav1.ConditionFalse, + wantReconciling: helmv1alpha1.ReasonProgressingWithRetry, + }, + { + name: "terminal fetch failure", + in: Inputs{ + Generation: 1, Now: testNow, Current: readyStatus(1), + InternalRepository: services.InternalRepositoryState{Present: true, Ready: true}, + Attempted: true, + Fetch: &services.FetchOutcome{ + Err: fetchErr, Terminal: true, + Reason: helmv1alpha1.ReasonAuthenticationFailed, + Message: "repository rejected the credentials (HTTP 401)", + }, + }, + wantReady: metav1.ConditionFalse, wantReadyReason: helmv1alpha1.ReasonAuthenticationFailed, + wantSynced: metav1.ConditionFalse, + wantStalled: helmv1alpha1.ReasonAuthenticationFailed, + }, + { + name: "unsupported repository type", + in: Inputs{ + Generation: 1, Now: testNow, Current: readyStatus(1), + ConfigErr: &services.ConfigOutcome{ + Reason: helmv1alpha1.ReasonUnsupportedRepositoryType, + Message: "unsupported repository schema in use: ftp", + }, + }, + wantReady: metav1.ConditionFalse, wantReadyReason: helmv1alpha1.ReasonUnsupportedRepositoryType, + wantSynced: metav1.ConditionTrue, + wantStalled: helmv1alpha1.ReasonUnsupportedRepositoryType, + }, + { + name: "catalog write failure", + in: Inputs{ + Generation: 1, Now: testNow, Current: readyStatus(1), + InternalRepository: services.InternalRepositoryState{Present: true, Ready: true}, + Attempted: true, + Fetch: &services.FetchOutcome{}, + Catalog: &services.CatalogOutcome{Err: writeErr}, + }, + wantReady: metav1.ConditionTrue, wantReadyReason: helmv1alpha1.ReasonSuccess, + wantSynced: metav1.ConditionFalse, + wantReconciling: helmv1alpha1.ReasonProgressingWithRetry, + }, + { + name: "generation bump voids the latch", + in: Inputs{ + Generation: 2, Now: testNow, Current: readyStatus(1), + InternalRepository: services.InternalRepositoryState{Present: true, Ready: true}, + Attempted: true, + Fetch: &services.FetchOutcome{Err: fetchErr, Message: "cannot read index.yaml"}, + }, + wantReady: metav1.ConditionUnknown, wantReadyReason: helmv1alpha1.ReasonAwaitingInitialSync, + wantSynced: metav1.ConditionFalse, + wantReconciling: helmv1alpha1.ReasonProgressingWithRetry, + }, + { + name: "oci repository has no internal object", + in: Inputs{ + Generation: 1, Now: testNow, Current: readyStatus(1), + InternalRepository: services.InternalRepositoryState{Present: false}, + Attempted: true, + Fetch: &services.FetchOutcome{}, + Catalog: &services.CatalogOutcome{}, + }, + wantReady: metav1.ConditionTrue, wantReadyReason: helmv1alpha1.ReasonSuccess, + wantSynced: metav1.ConditionTrue, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got := Evaluate(tc.in) + + ready := conditionOf(t, got.Status, helmv1alpha1.ConditionTypeReady) + if ready == nil { + t.Fatal("Ready condition must always be present") + } + if ready.Status != tc.wantReady { + t.Fatalf("Ready status is %q, want %q", ready.Status, tc.wantReady) + } + if ready.Reason != tc.wantReadyReason { + t.Fatalf("Ready reason is %q, want %q", ready.Reason, tc.wantReadyReason) + } + if ready.ObservedGeneration != tc.in.Generation { + t.Fatalf("Ready observedGeneration is %d, want %d", ready.ObservedGeneration, tc.in.Generation) + } + + synced := conditionOf(t, got.Status, helmv1alpha1.ConditionTypeSynced) + switch { + case tc.wantSynced == "" && synced != nil: + t.Fatalf("Synced must be absent, got %q", synced.Status) + case tc.wantSynced != "" && synced == nil: + t.Fatal("Synced condition is missing") + case tc.wantSynced != "" && synced.Status != tc.wantSynced: + t.Fatalf("Synced status is %q, want %q", synced.Status, tc.wantSynced) + } + + assertAbnormal(t, got.Status, helmv1alpha1.ConditionTypeReconciling, tc.wantReconciling) + assertAbnormal(t, got.Status, helmv1alpha1.ConditionTypeStalled, tc.wantStalled) + + // I3: the processed generation is always recorded. + if got.Status.ObservedGeneration != tc.in.Generation { + t.Fatalf("status.observedGeneration is %d, want %d", got.Status.ObservedGeneration, tc.in.Generation) + } + + // I1 and I2: exactly one abnormal-true condition while unhealthy, none while healthy. + healthy := ready.Status == metav1.ConditionTrue && synced != nil && synced.Status == metav1.ConditionTrue + abnormal := 0 + for _, conditionType := range []string{helmv1alpha1.ConditionTypeReconciling, helmv1alpha1.ConditionTypeStalled} { + if conditionOf(t, got.Status, conditionType) != nil { + abnormal++ + } + } + if healthy && abnormal != 0 { + t.Fatalf("healthy repository must carry no abnormal-true conditions, got %d", abnormal) + } + if !healthy && abnormal != 1 { + t.Fatalf("unhealthy repository must carry exactly one abnormal-true condition, got %d", abnormal) + } + }) + } +} + +func assertAbnormal(t *testing.T, status helmv1alpha1.HelmClusterAddonRepositoryStatus, conditionType, wantReason string) { + t.Helper() + + cond := conditionOf(t, status, conditionType) + if wantReason == "" { + if cond != nil { + t.Fatalf("%s must be absent, got reason %q", conditionType, cond.Reason) + } + + return + } + + if cond == nil { + t.Fatalf("%s is missing, want reason %q", conditionType, wantReason) + } + if cond.Status != metav1.ConditionTrue { + t.Fatalf("%s status is %q, want True", conditionType, cond.Status) + } + if cond.Reason != wantReason { + t.Fatalf("%s reason is %q, want %q", conditionType, cond.Reason, wantReason) + } +} diff --git a/images/operator-helm-controller/internal/services/outcomes.go b/images/operator-helm-controller/internal/services/outcomes.go new file mode 100644 index 00000000..ca6477da --- /dev/null +++ b/images/operator-helm-controller/internal/services/outcomes.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 services + +// InternalRepositoryState describes the observed state of the internal FluxCD +// repository object. Present is false for OCI repositories: they have no +// internal object at the repository level, each addon creates its own. +type InternalRepositoryState struct { + Present bool + Ready bool + Stalled bool + Reason string + Message string +} + +// FetchOutcome is the result of reading the chart catalog from the remote +// repository. Terminal marks a failure that will not resolve by retrying. +type FetchOutcome struct { + Err error + Terminal bool + Reason string + Message string +} + +// CatalogOutcome is the result of writing the chart catalog into the cluster. +type CatalogOutcome struct { + Err error +} + +// ConfigOutcome is a terminal configuration failure detected before any attempt +// to reach the repository, such as an unsupported url scheme. +type ConfigOutcome struct { + Reason string + Message string + Err error +} + +// SyncOutcome carries both phases of a synchronization attempt. +type SyncOutcome struct { + Fetch FetchOutcome + Catalog CatalogOutcome +} From e26a1a75f39e76a9412d9c2707447f7f449cb395 Mon Sep 17 00:00:00 2001 From: Ilya Drey Date: Tue, 1 Sep 2026 20:09:46 +0300 Subject: [PATCH 06/33] fix(core): pin Decision.Err filter and void stale Stalled reason on generation bump - Gate evaluateStalled's carry-forward branch on the condition having been set for the current generation, mirroring hasEvidence, so a generation bump no longer republishes a Stalled reason that described the old spec. - Add TestEvaluateDecisionErr to lock in that only cluster-write failures (SecretsErr, InternalRepositoryErr, Catalog.Err) reach Decision.Err; repository-read failures (Fetch.Err, ConfigErr) are excluded because their retry is scheduled through nextSyncTime instead. Signed-off-by: Ilya Drey --- .../helmclusteraddonrepository/evaluate.go | 6 +- .../evaluate_test.go | 105 ++++++++++++++++++ 2 files changed, 109 insertions(+), 2 deletions(-) diff --git a/images/operator-helm-controller/internal/reconcile/helmclusteraddonrepository/evaluate.go b/images/operator-helm-controller/internal/reconcile/helmclusteraddonrepository/evaluate.go index e5d77427..a7795fb6 100644 --- a/images/operator-helm-controller/internal/reconcile/helmclusteraddonrepository/evaluate.go +++ b/images/operator-helm-controller/internal/reconcile/helmclusteraddonrepository/evaluate.go @@ -176,9 +176,11 @@ func evaluateStalled( return abnormalCondition{set: true, reason: internal.Reason, message: internal.Message} case !in.Attempted: // A pass without an attempt carries the previous verdict forward so the - // specific reason is not replaced by a generic one. + // specific reason is not replaced by a generic one. Only carry it forward + // when it was set for the current generation: a generation bump voids + // evidence about the previous spec, mirroring hasEvidence. if cond := apimeta.FindStatusCondition(in.Current.Conditions, helmv1alpha1.ConditionTypeStalled); cond != nil && - cond.Status == metav1.ConditionTrue { + cond.Status == metav1.ConditionTrue && cond.ObservedGeneration == in.Generation { return abnormalCondition{set: true, reason: cond.Reason, message: cond.Message} } 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 21be8083..47f0013d 100644 --- a/images/operator-helm-controller/internal/reconcile/helmclusteraddonrepository/evaluate_test.go +++ b/images/operator-helm-controller/internal/reconcile/helmclusteraddonrepository/evaluate_test.go @@ -53,6 +53,22 @@ func readyStatus(generation int64) helmv1alpha1.HelmClusterAddonRepositoryStatus } } +// stalledStatus builds a status like readyStatus, plus a Stalled=True condition +// recorded for staleGeneration — used to test that a generation bump voids a +// carried-forward Stalled reason that described the previous spec. +func stalledStatus(generation, staleGeneration int64, reason string) helmv1alpha1.HelmClusterAddonRepositoryStatus { + status := readyStatus(generation) + status.Conditions = append(status.Conditions, metav1.Condition{ + Type: helmv1alpha1.ConditionTypeStalled, + Status: metav1.ConditionTrue, + Reason: reason, + ObservedGeneration: staleGeneration, + LastTransitionTime: metav1.NewTime(testNow.Add(-time.Hour)), + }) + + return status +} + func conditionOf(t *testing.T, status helmv1alpha1.HelmClusterAddonRepositoryStatus, conditionType string) *metav1.Condition { t.Helper() @@ -227,6 +243,17 @@ func TestEvaluateConditions(t *testing.T) { wantReady: metav1.ConditionTrue, wantReadyReason: helmv1alpha1.ReasonSuccess, wantSynced: metav1.ConditionTrue, }, + { + name: "generation bump voids a stale stalled reason when no attempt runs", + in: Inputs{ + Generation: 2, Now: testNow, + Current: stalledStatus(1, 1, helmv1alpha1.ReasonAuthenticationFailed), + SecretsErr: writeErr, + }, + wantReady: metav1.ConditionFalse, wantReadyReason: helmv1alpha1.ReasonAuxiliaryResourcesFailed, + wantSynced: metav1.ConditionTrue, + wantReconciling: helmv1alpha1.ReasonProgressingWithRetry, + }, } for _, tc := range cases { @@ -283,6 +310,84 @@ func TestEvaluateConditions(t *testing.T) { } } +// TestEvaluateDecisionErr pins the filter behind Decision.Err: only failures that +// belong on the controller-runtime work queue reach it (auxiliary resources, +// the internal repository object, the chart catalog write). Repository-read +// failures (Fetch, ConfigErr) are deliberately excluded — their retry is +// scheduled through nextSyncTime instead, and routing them into the work queue +// as well would double-schedule the retry. +func TestEvaluateDecisionErr(t *testing.T) { + secretsErr := errors.New("failed to reconcile secret") + internalErr := errors.New("failed to reconcile internal repository object") + catalogWriteErr := errors.New("etcdserver: request timed out") + fetchErr := errors.New("connection refused") + + cases := []struct { + name string + in Inputs + wantErr error + }{ + { + name: "auxiliary resource failure reaches Decision.Err", + in: Inputs{ + Generation: 1, Now: testNow, Current: readyStatus(1), + SecretsErr: secretsErr, + }, + wantErr: secretsErr, + }, + { + name: "internal repository reconcile failure reaches Decision.Err", + in: Inputs{ + Generation: 1, Now: testNow, Current: readyStatus(1), + InternalRepositoryErr: internalErr, + }, + wantErr: internalErr, + }, + { + name: "chart catalog write failure reaches Decision.Err", + in: Inputs{ + Generation: 1, Now: testNow, Current: readyStatus(1), + InternalRepository: services.InternalRepositoryState{Present: true, Ready: true}, + Attempted: true, + Fetch: &services.FetchOutcome{}, + Catalog: &services.CatalogOutcome{Err: catalogWriteErr}, + }, + wantErr: catalogWriteErr, + }, + { + name: "repository read failure never reaches Decision.Err", + in: Inputs{ + Generation: 1, Now: testNow, Current: readyStatus(1), + InternalRepository: services.InternalRepositoryState{Present: true, Ready: true}, + Attempted: true, + Fetch: &services.FetchOutcome{Err: fetchErr, Message: "cannot read index.yaml"}, + }, + wantErr: nil, + }, + { + name: "configuration failure never reaches Decision.Err", + in: Inputs{ + Generation: 1, Now: testNow, Current: readyStatus(1), + ConfigErr: &services.ConfigOutcome{ + Reason: helmv1alpha1.ReasonUnsupportedRepositoryType, + Message: "unsupported repository schema in use: ftp", + }, + }, + wantErr: nil, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got := Evaluate(tc.in) + + if got.Err != tc.wantErr { + t.Fatalf("Decision.Err is %v, want %v", got.Err, tc.wantErr) + } + }) + } +} + func assertAbnormal(t *testing.T, status helmv1alpha1.HelmClusterAddonRepositoryStatus, conditionType, wantReason string) { t.Helper() From f35445d86eb1228f7e2f9855d97041ad7f550fb1 Mon Sep 17 00:00:00 2001 From: Ilya Drey Date: Tue, 1 Sep 2026 20:15:43 +0300 Subject: [PATCH 07/33] feat(core): schedule repository sync with exponential backoff Signed-off-by: Ilya Drey --- .../helmclusteraddonrepository/evaluate.go | 92 +++++- .../schedule_test.go | 265 ++++++++++++++++++ 2 files changed, 351 insertions(+), 6 deletions(-) create mode 100644 images/operator-helm-controller/internal/reconcile/helmclusteraddonrepository/schedule_test.go diff --git a/images/operator-helm-controller/internal/reconcile/helmclusteraddonrepository/evaluate.go b/images/operator-helm-controller/internal/reconcile/helmclusteraddonrepository/evaluate.go index a7795fb6..905abedf 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 ( + "math/rand/v2" "time" apimeta "k8s.io/apimachinery/pkg/api/meta" @@ -107,9 +108,27 @@ func Evaluate(in Inputs) Decision { status.ConsecutiveFetchFailures = failures + if in.Attempted { + if fetchSucceeded && !catalogFailed { + status.LastSuccessfulSyncTime = &metav1.Time{Time: in.Now} + } + + next := in.Now.Add(withJitter(syncDelay(failures), in.Jitter)) + status.NextSyncTime = &metav1.Time{Time: next} + } + + requeueAfter := time.Duration(0) + if in.ConfigErr == nil && status.NextSyncTime != nil { + requeueAfter = status.NextSyncTime.Sub(in.Now) + if requeueAfter < minRequeue { + requeueAfter = minRequeue + } + } + return Decision{ - Status: status, - Err: firstErr(in.SecretsErr, in.InternalRepositoryErr, catalogErr(in)), + Status: status, + RequeueAfter: requeueAfter, + Err: firstErr(in.SecretsErr, in.InternalRepositoryErr, catalogErr(in)), } } @@ -298,10 +317,71 @@ func catalogErr(in Inputs) error { return in.Catalog.Err } -// MaxFetchFailures is the number of consecutive read failures after which the -// repository is reported as Stalled. Reaching it does not stop the retries: -// the cause may disappear on the remote side. -const MaxFetchFailures = 5 +const ( + // SyncInterval is the normal catalog synchronization cadence and the base of + // the retry backoff, so a broken repository is never polled more often than a + // healthy one. + SyncInterval = 5 * time.Minute + // MaxSyncBackoff caps the retry delay. + MaxSyncBackoff = 1 * time.Hour + // MaxFetchFailures is the number of consecutive read failures after which the + // repository is reported as Stalled. Reaching it does not stop the retries: + // the cause may disappear on the remote side. Moved here from task 4's + // standalone declaration — the value does not change. + MaxFetchFailures = 5 + // SyncBackoffJitter spreads the schedule of repositories that share a remote. + SyncBackoffJitter = 0.1 + // minRequeue keeps a due schedule from being reported as "no requeue", + // which is what a zero RequeueAfter means to controller-runtime. + minRequeue = time.Second +) + +// ShouldAttempt reports whether a synchronization attempt is due. The caller +// additionally requires the auxiliary resources to be in place. +func ShouldAttempt( + current helmv1alpha1.HelmClusterAddonRepositoryStatus, + generation int64, + now time.Time, + forced bool, +) bool { + switch { + case forced: + return true + case generation != current.ObservedGeneration: + return true + case current.NextSyncTime == nil: + return true + default: + return !now.Before(current.NextSyncTime.Time) + } +} + +// NewJitter returns the random factor Evaluate applies to the computed delay. +// It lives outside Evaluate to keep that function deterministic. +func NewJitter() float64 { + return (rand.Float64()*2 - 1) * SyncBackoffJitter +} + +func syncDelay(failures int32) time.Duration { + if failures <= 0 { + return SyncInterval + } + + delay := SyncInterval << (failures - 1) + if delay > MaxSyncBackoff || delay <= 0 { + return MaxSyncBackoff + } + + return delay +} + +func withJitter(delay time.Duration, jitter float64) time.Duration { + if jitter == 0 { + return delay + } + + return delay + time.Duration(float64(delay)*jitter) +} func nextFailureCount(failures int32, attempted, fetchFailed bool, fetch *services.FetchOutcome) int32 { if !attempted { diff --git a/images/operator-helm-controller/internal/reconcile/helmclusteraddonrepository/schedule_test.go b/images/operator-helm-controller/internal/reconcile/helmclusteraddonrepository/schedule_test.go new file mode 100644 index 00000000..ea63f21e --- /dev/null +++ b/images/operator-helm-controller/internal/reconcile/helmclusteraddonrepository/schedule_test.go @@ -0,0 +1,265 @@ +/* +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 helmclusteraddonrepository + +import ( + "errors" + "testing" + "time" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + helmv1alpha1 "github.com/deckhouse/operator-helm/api/v1alpha1" + "github.com/deckhouse/operator-helm/internal/services" +) + +func TestBackoffProgression(t *testing.T) { + fetchErr := errors.New("connection refused") + + cases := []struct { + name string + failuresIn int32 + wantFailures int32 + wantRequeue time.Duration + }{ + {name: "first failure", failuresIn: 0, wantFailures: 1, wantRequeue: 5 * time.Minute}, + {name: "second failure", failuresIn: 1, wantFailures: 2, wantRequeue: 10 * time.Minute}, + {name: "third failure", failuresIn: 2, wantFailures: 3, wantRequeue: 20 * time.Minute}, + {name: "fourth failure", failuresIn: 3, wantFailures: 4, wantRequeue: 40 * time.Minute}, + {name: "fifth failure caps", failuresIn: 4, wantFailures: 5, wantRequeue: time.Hour}, + {name: "beyond cap stays capped", failuresIn: 5, wantFailures: 5, wantRequeue: time.Hour}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got := Evaluate(Inputs{ + Generation: 1, + Now: testNow, + Current: helmv1alpha1.HelmClusterAddonRepositoryStatus{ + ObservedGeneration: 1, + ConsecutiveFetchFailures: tc.failuresIn, + }, + InternalRepository: services.InternalRepositoryState{Present: true, Ready: true}, + Attempted: true, + Fetch: &services.FetchOutcome{Err: fetchErr, Message: "cannot read index.yaml"}, + }) + + if got.Status.ConsecutiveFetchFailures != tc.wantFailures { + t.Fatalf("failures are %d, want %d", got.Status.ConsecutiveFetchFailures, tc.wantFailures) + } + if got.RequeueAfter != tc.wantRequeue { + t.Fatalf("requeue after %s, want %s", got.RequeueAfter, tc.wantRequeue) + } + if got.Status.NextSyncTime == nil { + t.Fatal("nextSyncTime must be set after an attempt") + } + if !got.Status.NextSyncTime.Time.Equal(testNow.Add(tc.wantRequeue)) { + t.Fatalf("nextSyncTime is %s, want %s", got.Status.NextSyncTime.Time, testNow.Add(tc.wantRequeue)) + } + }) + } +} + +func TestSuccessResetsCounterAndRecordsSyncTime(t *testing.T) { + got := Evaluate(Inputs{ + Generation: 1, + Now: testNow, + Current: helmv1alpha1.HelmClusterAddonRepositoryStatus{ + ObservedGeneration: 1, + ConsecutiveFetchFailures: 3, + }, + InternalRepository: services.InternalRepositoryState{Present: true, Ready: true}, + Attempted: true, + Fetch: &services.FetchOutcome{}, + Catalog: &services.CatalogOutcome{}, + }) + + if got.Status.ConsecutiveFetchFailures != 0 { + t.Fatalf("counter must reset on success, got %d", got.Status.ConsecutiveFetchFailures) + } + if got.RequeueAfter != SyncInterval { + t.Fatalf("requeue after %s, want %s", got.RequeueAfter, SyncInterval) + } + if got.Status.LastSuccessfulSyncTime == nil || !got.Status.LastSuccessfulSyncTime.Time.Equal(testNow) { + t.Fatalf("lastSuccessfulSyncTime is %v, want %s", got.Status.LastSuccessfulSyncTime, testNow) + } +} + +func TestCatalogFailureDoesNotRecordSyncTime(t *testing.T) { + previous := metav1.NewTime(testNow.Add(-time.Hour)) + + got := Evaluate(Inputs{ + Generation: 1, + Now: testNow, + Current: helmv1alpha1.HelmClusterAddonRepositoryStatus{ + ObservedGeneration: 1, + LastSuccessfulSyncTime: &previous, + }, + InternalRepository: services.InternalRepositoryState{Present: true, Ready: true}, + Attempted: true, + Fetch: &services.FetchOutcome{}, + Catalog: &services.CatalogOutcome{Err: errors.New("etcdserver: request timed out")}, + }) + + if !got.Status.LastSuccessfulSyncTime.Time.Equal(previous.Time) { + t.Fatalf("lastSuccessfulSyncTime must not advance on a catalog failure, got %s", got.Status.LastSuccessfulSyncTime.Time) + } + if got.Status.ConsecutiveFetchFailures != 0 { + t.Fatalf("a catalog failure must not count as a fetch failure, got %d", got.Status.ConsecutiveFetchFailures) + } + if got.Err == nil { + t.Fatal("a catalog failure must be returned for the work queue") + } +} + +func TestTerminalFetchSaturatesCounter(t *testing.T) { + got := Evaluate(Inputs{ + Generation: 1, + Now: testNow, + Current: helmv1alpha1.HelmClusterAddonRepositoryStatus{ObservedGeneration: 1}, + InternalRepository: services.InternalRepositoryState{Present: true, Ready: true}, + Attempted: true, + Fetch: &services.FetchOutcome{ + Err: errors.New("unauthorized"), Terminal: true, + Reason: helmv1alpha1.ReasonAuthenticationFailed, Message: "rejected the credentials", + }, + }) + + if got.Status.ConsecutiveFetchFailures != MaxFetchFailures { + t.Fatalf("terminal failure must saturate the counter, got %d", got.Status.ConsecutiveFetchFailures) + } + if got.RequeueAfter != MaxSyncBackoff { + t.Fatalf("requeue after %s, want %s", got.RequeueAfter, MaxSyncBackoff) + } +} + +func TestConfigErrorDoesNotRequeue(t *testing.T) { + got := Evaluate(Inputs{ + Generation: 1, + Now: testNow, + Current: helmv1alpha1.HelmClusterAddonRepositoryStatus{ObservedGeneration: 1}, + ConfigErr: &services.ConfigOutcome{ + Reason: helmv1alpha1.ReasonUnsupportedRepositoryType, + Message: "unsupported repository schema in use: ftp", + }, + }) + + if got.RequeueAfter != 0 { + t.Fatalf("a spec-only failure must not requeue, got %s", got.RequeueAfter) + } +} + +func TestGenerationBumpResetsCounter(t *testing.T) { + got := Evaluate(Inputs{ + Generation: 2, + Now: testNow, + Current: helmv1alpha1.HelmClusterAddonRepositoryStatus{ + ObservedGeneration: 1, + ConsecutiveFetchFailures: 4, + }, + InternalRepository: services.InternalRepositoryState{Present: true, Ready: true}, + Attempted: true, + Fetch: &services.FetchOutcome{}, + Catalog: &services.CatalogOutcome{}, + }) + + if got.Status.ConsecutiveFetchFailures != 0 { + t.Fatalf("counter must reset on a spec change, got %d", got.Status.ConsecutiveFetchFailures) + } +} + +func TestPassWithoutAttemptKeepsSchedule(t *testing.T) { + next := metav1.NewTime(testNow.Add(3 * time.Minute)) + + got := Evaluate(Inputs{ + Generation: 1, + Now: testNow, + Current: helmv1alpha1.HelmClusterAddonRepositoryStatus{ + ObservedGeneration: 1, + NextSyncTime: &next, + ConsecutiveFetchFailures: 2, + Conditions: []metav1.Condition{ + { + Type: helmv1alpha1.ConditionTypeReady, + Status: metav1.ConditionTrue, + Reason: helmv1alpha1.ReasonSuccess, + ObservedGeneration: 1, + LastTransitionTime: metav1.NewTime(testNow.Add(-time.Hour)), + }, + }, + }, + InternalRepository: services.InternalRepositoryState{Present: true, Ready: true}, + }) + + if !got.Status.NextSyncTime.Time.Equal(next.Time) { + t.Fatalf("nextSyncTime must not move without an attempt, got %s", got.Status.NextSyncTime.Time) + } + if got.Status.ConsecutiveFetchFailures != 2 { + t.Fatalf("counter must not move without an attempt, got %d", got.Status.ConsecutiveFetchFailures) + } + if got.RequeueAfter != 3*time.Minute { + t.Fatalf("requeue after %s, want the remaining 3m", got.RequeueAfter) + } +} + +func TestShouldAttempt(t *testing.T) { + future := metav1.NewTime(testNow.Add(time.Minute)) + past := metav1.NewTime(testNow.Add(-time.Minute)) + + cases := []struct { + name string + current helmv1alpha1.HelmClusterAddonRepositoryStatus + generation int64 + forced bool + want bool + }{ + {name: "fresh object", current: helmv1alpha1.HelmClusterAddonRepositoryStatus{}, generation: 1, want: true}, + { + name: "schedule not reached", + current: helmv1alpha1.HelmClusterAddonRepositoryStatus{ObservedGeneration: 1, NextSyncTime: &future}, + generation: 1, + want: false, + }, + { + name: "schedule reached", + current: helmv1alpha1.HelmClusterAddonRepositoryStatus{ObservedGeneration: 1, NextSyncTime: &past}, + generation: 1, + want: true, + }, + { + name: "forced beats the schedule", + current: helmv1alpha1.HelmClusterAddonRepositoryStatus{ObservedGeneration: 1, NextSyncTime: &future}, + generation: 1, + forced: true, + want: true, + }, + { + name: "spec change beats the schedule", + current: helmv1alpha1.HelmClusterAddonRepositoryStatus{ObservedGeneration: 1, NextSyncTime: &future}, + generation: 2, + want: true, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := ShouldAttempt(tc.current, tc.generation, testNow, tc.forced); got != tc.want { + t.Fatalf("ShouldAttempt = %v, want %v", got, tc.want) + } + }) + } +} From 5f522570928a10fa7dcbba61b656c1b2ba0fe549 Mon Sep 17 00:00:00 2001 From: Ilya Drey Date: Tue, 1 Sep 2026 20:15:49 +0300 Subject: [PATCH 08/33] fix(core): pass the addon name and wrap the cause in the uniqueness check error Signed-off-by: Ilya Drey --- .../internal/webhook/helmclusteraddon/webhook.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/images/operator-helm-controller/internal/webhook/helmclusteraddon/webhook.go b/images/operator-helm-controller/internal/webhook/helmclusteraddon/webhook.go index 5f7a5a2a..ccb605e8 100644 --- a/images/operator-helm-controller/internal/webhook/helmclusteraddon/webhook.go +++ b/images/operator-helm-controller/internal/webhook/helmclusteraddon/webhook.go @@ -127,7 +127,7 @@ func isUniquenessBypassed(ctx context.Context) bool { func (v *HelmClusterAddonWebhookValidator) checkUniqueness(ctx context.Context, addon *helmv1alpha1.HelmClusterAddon) error { owned, err := v.claimService.OwnedBy(ctx, addon) if err != nil { - return fmt.Errorf("failed to check if helmclusteraddon/%s owns chart claim: %w", err) + return fmt.Errorf("failed to check if helmclusteraddon/%s owns chart claim: %w", addon.Name, err) } if owned { return nil From f7714d4bdf982b4b7f8de8308221a34e79ff1ecc Mon Sep 17 00:00:00 2001 From: Ilya Drey Date: Tue, 1 Sep 2026 20:24:13 +0300 Subject: [PATCH 09/33] test(core): cover ShouldAttempt nil-schedule branch and requeue floor Signed-off-by: Ilya Drey --- .../schedule_test.go | 26 +++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/images/operator-helm-controller/internal/reconcile/helmclusteraddonrepository/schedule_test.go b/images/operator-helm-controller/internal/reconcile/helmclusteraddonrepository/schedule_test.go index ea63f21e..68913615 100644 --- a/images/operator-helm-controller/internal/reconcile/helmclusteraddonrepository/schedule_test.go +++ b/images/operator-helm-controller/internal/reconcile/helmclusteraddonrepository/schedule_test.go @@ -216,6 +216,26 @@ func TestPassWithoutAttemptKeepsSchedule(t *testing.T) { } } +func TestOverdueScheduleWithoutAttemptFloorsRequeue(t *testing.T) { + overdue := metav1.NewTime(testNow.Add(-time.Minute)) + + got := Evaluate(Inputs{ + Generation: 1, + Now: testNow, + Current: helmv1alpha1.HelmClusterAddonRepositoryStatus{ + ObservedGeneration: 1, + NextSyncTime: &overdue, + }, + // SecretsErr blocks the attempt: the reconciler never gets to Fetch/Catalog, + // so Attempted stays false while the schedule is already due. + SecretsErr: errors.New("failed to reconcile secret"), + }) + + if got.RequeueAfter != minRequeue { + t.Fatalf("requeue after %s, want the floor %s", got.RequeueAfter, minRequeue) + } +} + func TestShouldAttempt(t *testing.T) { future := metav1.NewTime(testNow.Add(time.Minute)) past := metav1.NewTime(testNow.Add(-time.Minute)) @@ -253,6 +273,12 @@ func TestShouldAttempt(t *testing.T) { generation: 2, want: true, }, + { + name: "schedule never set on a matching generation", + current: helmv1alpha1.HelmClusterAddonRepositoryStatus{ObservedGeneration: 1, NextSyncTime: nil}, + generation: 1, + want: true, + }, } for _, tc := range cases { From a0fcdce9155a54708d04a52fc970d332ece91137 Mon Sep 17 00:00:00 2001 From: Ilya Drey Date: Tue, 1 Sep 2026 20:33:25 +0300 Subject: [PATCH 10/33] test(core): verify repository status against kstatus Signed-off-by: Ilya Drey --- images/operator-helm-controller/go.mod | 3 +- images/operator-helm-controller/go.sum | 2 + .../kstatus_test.go | 118 ++++++++++++++++++ 3 files changed, 122 insertions(+), 1 deletion(-) create mode 100644 images/operator-helm-controller/internal/reconcile/helmclusteraddonrepository/kstatus_test.go diff --git a/images/operator-helm-controller/go.mod b/images/operator-helm-controller/go.mod index f819642e..0a2e57d6 100644 --- a/images/operator-helm-controller/go.mod +++ b/images/operator-helm-controller/go.mod @@ -9,6 +9,7 @@ require ( github.com/deckhouse/operator-helm/api v0.0.0-00010101000000-000000000000 github.com/google/go-containerregistry v0.20.6 github.com/opencontainers/go-digest v1.0.0 + github.com/samber/lo v1.53.0 github.com/werf/3p-fluxcd-pkg/apis/meta v1.23.0-nelm.1 github.com/werf/3p-fluxcd-pkg/chartutil v1.17.0-nelm.1 github.com/werf/3p-helm-controller/api v0.1.5 @@ -19,6 +20,7 @@ require ( k8s.io/apimachinery v0.35.1 k8s.io/client-go v0.35.1 k8s.io/utils v0.0.0-20251002143259-bc988d571ff4 + sigs.k8s.io/cli-utils v0.37.2 sigs.k8s.io/controller-runtime v0.23.1 ) @@ -61,7 +63,6 @@ require ( github.com/prometheus/client_model v0.6.2 // indirect github.com/prometheus/common v0.66.1 // indirect github.com/prometheus/procfs v0.16.1 // indirect - github.com/samber/lo v1.53.0 // indirect github.com/santhosh-tekuri/jsonschema/v6 v6.0.2 // indirect github.com/sirupsen/logrus v1.9.3 // indirect github.com/spf13/pflag v1.0.10 // indirect diff --git a/images/operator-helm-controller/go.sum b/images/operator-helm-controller/go.sum index 560d15e2..bbec7d6f 100644 --- a/images/operator-helm-controller/go.sum +++ b/images/operator-helm-controller/go.sum @@ -226,6 +226,8 @@ k8s.io/kube-openapi v0.0.0-20260127142750-a19766b6e2d4 h1:HhDfevmPS+OalTjQRKbTHp k8s.io/kube-openapi v0.0.0-20260127142750-a19766b6e2d4/go.mod h1:kdmbQkyfwUagLfXIad1y2TdrjPFWp2Q89B3qkRwf/pQ= k8s.io/utils v0.0.0-20251002143259-bc988d571ff4 h1:SjGebBtkBqHFOli+05xYbK8YF1Dzkbzn+gDM4X9T4Ck= k8s.io/utils v0.0.0-20251002143259-bc988d571ff4/go.mod h1:OLgZIPagt7ERELqWJFomSt595RzquPNLL48iOWgYOg0= +sigs.k8s.io/cli-utils v0.37.2 h1:GOfKw5RV2HDQZDJlru5KkfLO1tbxqMoyn1IYUxqBpNg= +sigs.k8s.io/cli-utils v0.37.2/go.mod h1:V+IZZr4UoGj7gMJXklWBg6t5xbdThFBcpj4MrZuCYco= sigs.k8s.io/controller-runtime v0.23.1 h1:TjJSM80Nf43Mg21+RCy3J70aj/W6KyvDtOlpKf+PupE= sigs.k8s.io/controller-runtime v0.23.1/go.mod h1:B6COOxKptp+YaUT5q4l6LqUJTRpizbgf9KSRNdQGns0= sigs.k8s.io/json v0.0.0-20250730193827-2d320260d730 h1:IpInykpT6ceI+QxKBbEflcR5EXP7sU1kvOlxwZh5txg= diff --git a/images/operator-helm-controller/internal/reconcile/helmclusteraddonrepository/kstatus_test.go b/images/operator-helm-controller/internal/reconcile/helmclusteraddonrepository/kstatus_test.go new file mode 100644 index 00000000..d472ef38 --- /dev/null +++ b/images/operator-helm-controller/internal/reconcile/helmclusteraddonrepository/kstatus_test.go @@ -0,0 +1,118 @@ +/* +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 helmclusteraddonrepository + +import ( + "errors" + "testing" + + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime" + "sigs.k8s.io/cli-utils/pkg/kstatus/status" + + helmv1alpha1 "github.com/deckhouse/operator-helm/api/v1alpha1" + "github.com/deckhouse/operator-helm/internal/services" +) + +// computeStatus renders the decision as the repository object would look in the +// cluster and asks the real kstatus library for its verdict. +func computeStatus(t *testing.T, generation int64, decision Decision) status.Status { + t.Helper() + + repo := &helmv1alpha1.HelmClusterAddonRepository{} + repo.SetGroupVersionKind(helmv1alpha1.HelmClusterAddonRepositoryGVK) + repo.SetName("test-repo") + repo.SetGeneration(generation) + repo.Status = decision.Status + + content, err := runtime.DefaultUnstructuredConverter.ToUnstructured(repo) + if err != nil { + t.Fatalf("converting repository to unstructured: %v", err) + } + + result, err := status.Compute(&unstructured.Unstructured{Object: content}) + if err != nil { + t.Fatalf("computing kstatus: %v", err) + } + + return result.Status +} + +func TestKstatusVerdicts(t *testing.T) { + cases := []struct { + name string + in Inputs + want status.Status + }{ + { + name: "healthy repository is current", + in: Inputs{ + Generation: 1, Now: testNow, Current: readyStatus(1), + InternalRepository: services.InternalRepositoryState{Present: true, Ready: true}, + Attempted: true, + Fetch: &services.FetchOutcome{}, + Catalog: &services.CatalogOutcome{}, + }, + want: status.CurrentStatus, + }, + { + name: "retrying repository is in progress", + in: Inputs{ + Generation: 1, Now: testNow, Current: readyStatus(1), + InternalRepository: services.InternalRepositoryState{Present: true, Ready: true}, + Attempted: true, + Fetch: &services.FetchOutcome{Err: errors.New("connection refused"), Message: "cannot read index.yaml"}, + }, + want: status.InProgressStatus, + }, + { + name: "terminal failure is failed", + in: Inputs{ + Generation: 1, Now: testNow, Current: readyStatus(1), + InternalRepository: services.InternalRepositoryState{Present: true, Ready: true}, + Attempted: true, + Fetch: &services.FetchOutcome{ + Err: errors.New("unauthorized"), Terminal: true, + Reason: helmv1alpha1.ReasonAuthenticationFailed, Message: "rejected the credentials", + }, + }, + want: status.FailedStatus, + }, + { + name: "stalled is not masked by a lagging observedGeneration", + in: Inputs{ + Generation: 3, Now: testNow, Current: readyStatus(1), + InternalRepository: services.InternalRepositoryState{Present: true, Ready: true}, + Attempted: true, + Fetch: &services.FetchOutcome{ + Err: errors.New("not found"), Terminal: true, + Reason: helmv1alpha1.ReasonSourceNotFound, Message: "repository not found", + }, + }, + want: status.FailedStatus, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got := computeStatus(t, tc.in.Generation, Evaluate(tc.in)) + if got != tc.want { + t.Fatalf("kstatus verdict is %s, want %s", got, tc.want) + } + }) + } +} From 2ee247f32f621ba4d66b5823286534fb965440d6 Mon Sep 17 00:00:00 2001 From: Ilya Drey Date: Tue, 1 Sep 2026 20:42:49 +0300 Subject: [PATCH 11/33] refactor(core): extract shared repository secret reconciliation Signed-off-by: Ilya Drey --- .../internal/services/base.go | 15 +++ .../internal/services/base_test.go | 120 ++++++++++++++++++ 2 files changed, 135 insertions(+) create mode 100644 images/operator-helm-controller/internal/services/base_test.go diff --git a/images/operator-helm-controller/internal/services/base.go b/images/operator-helm-controller/internal/services/base.go index 67a3d769..b0745c9d 100644 --- a/images/operator-helm-controller/internal/services/base.go +++ b/images/operator-helm-controller/internal/services/base.go @@ -64,6 +64,21 @@ type BaseRepoService struct { TargetNamespace string } +// EnsureSecrets reconciles every auxiliary secret the repository needs, for both +// repository types. Its success is the gate for attempting a catalog +// synchronization: without credentials there is nothing to try. +func (s *BaseRepoService) EnsureSecrets(ctx context.Context, repo *helmv1alpha1.HelmClusterAddonRepository) error { + if err := s.reconcileAuthSecret(ctx, repo); err != nil { + return fmt.Errorf("reconciling auth secret: %w", err) + } + + if err := s.reconcileTLSSecret(ctx, repo); err != nil { + return fmt.Errorf("reconciling tls secret: %w", err) + } + + return nil +} + func (s *BaseRepoService) reconcileAuthSecret(ctx context.Context, repo *helmv1alpha1.HelmClusterAddonRepository) error { secretName := utils.GetInternalRepositoryAuthSecretName(repo.Name) diff --git a/images/operator-helm-controller/internal/services/base_test.go b/images/operator-helm-controller/internal/services/base_test.go new file mode 100644 index 00000000..ee5d42e3 --- /dev/null +++ b/images/operator-helm-controller/internal/services/base_test.go @@ -0,0 +1,120 @@ +/* +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" + + 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/runtime" + "k8s.io/apimachinery/pkg/types" + clientgoscheme "k8s.io/client-go/kubernetes/scheme" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + + helmv1alpha1 "github.com/deckhouse/operator-helm/api/v1alpha1" + "github.com/deckhouse/operator-helm/internal/utils" +) + +const testNamespace = "d8-operator-helm" + +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 newBaseRepoService(t *testing.T, objects ...client.Object) (*BaseRepoService, client.Client) { + t.Helper() + + scheme := testScheme(t) + c := fake.NewClientBuilder().WithScheme(scheme).WithObjects(objects...).Build() + + return &BaseRepoService{ + BaseService: BaseService{Client: c, Scheme: scheme}, + TargetNamespace: testNamespace, + }, c +} + +func TestEnsureSecretsCreatesAuthAndTLS(t *testing.T) { + repo := &helmv1alpha1.HelmClusterAddonRepository{ + ObjectMeta: metav1.ObjectMeta{Name: "example"}, + Spec: helmv1alpha1.HelmClusterAddonRepositorySpec{ + URL: "https://example.invalid/charts", + Auth: &helmv1alpha1.HelmClusterAddonRepositoryAuth{Username: "user", Password: "secret"}, + CACertificate: "-----BEGIN CERTIFICATE-----", + }, + } + + service, c := newBaseRepoService(t, repo) + + if err := service.EnsureSecrets(context.Background(), repo); err != nil { + t.Fatalf("EnsureSecrets returned %v", err) + } + + auth := &corev1.Secret{} + authKey := types.NamespacedName{Name: utils.GetInternalRepositoryAuthSecretName(repo.Name), Namespace: testNamespace} + if err := c.Get(context.Background(), authKey, auth); err != nil { + t.Fatalf("auth secret was not created: %v", err) + } + // The fake client stores what the controller wrote: unlike the API server it + // does not fold StringData into Data. + if got := auth.StringData["username"]; got != "user" { + t.Fatalf("auth secret username is %q, want %q", got, "user") + } + + tls := &corev1.Secret{} + tlsKey := types.NamespacedName{Name: utils.GetInternalRepositoryTLSSecretName(repo.Name), Namespace: testNamespace} + if err := c.Get(context.Background(), tlsKey, tls); err != nil { + t.Fatalf("tls secret was not created: %v", err) + } +} + +func TestEnsureSecretsRemovesObsoleteSecrets(t *testing.T) { + repo := &helmv1alpha1.HelmClusterAddonRepository{ + ObjectMeta: metav1.ObjectMeta{Name: "example"}, + Spec: helmv1alpha1.HelmClusterAddonRepositorySpec{URL: "https://example.invalid/charts"}, + } + obsolete := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: utils.GetInternalRepositoryAuthSecretName(repo.Name), + Namespace: testNamespace, + }, + } + + service, c := newBaseRepoService(t, repo, obsolete) + + if err := service.EnsureSecrets(context.Background(), repo); err != nil { + t.Fatalf("EnsureSecrets returned %v", err) + } + + err := c.Get(context.Background(), client.ObjectKeyFromObject(obsolete), &corev1.Secret{}) + if !apierrors.IsNotFound(err) { + t.Fatalf("obsolete auth secret must be deleted, got %v", err) + } +} From d749383b572b77962993ba1a1d1d74ef9d6f474d Mon Sep 17 00:00:00 2001 From: Ilya Drey Date: Tue, 1 Sep 2026 20:48:36 +0300 Subject: [PATCH 12/33] feat(core): report internal repository state instead of a status provider Signed-off-by: Ilya Drey --- .../internal/services/helm_repo_service.go | 57 ++++++ .../services/helm_repo_service_test.go | 165 ++++++++++++++++++ 2 files changed, 222 insertions(+) create mode 100644 images/operator-helm-controller/internal/services/helm_repo_service_test.go diff --git a/images/operator-helm-controller/internal/services/helm_repo_service.go b/images/operator-helm-controller/internal/services/helm_repo_service.go index 76ae754b..4bf002de 100644 --- a/images/operator-helm-controller/internal/services/helm_repo_service.go +++ b/images/operator-helm-controller/internal/services/helm_repo_service.go @@ -24,6 +24,7 @@ import ( "github.com/werf/3p-fluxcd-pkg/apis/meta" sourcev1 "github.com/werf/nelm-source-controller/api/v1" corev1 "k8s.io/api/core/v1" + apimeta "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" @@ -126,6 +127,62 @@ func (s *HelmRepoService) EnsureInternalHelmRepository(ctx context.Context, repo return HelmRepoResult{Status: status.Unknown(repo, helmv1alpha1.ReasonReconciling)} } +// EnsureInternalRepositoryState reconciles the internal HelmRepository and +// reports its observed state. The returned error is an API failure that the +// caller must surface to the work queue; an unhealthy internal object is not an +// error and is reported through the state instead. +func (s *HelmRepoService) EnsureInternalRepositoryState( + ctx context.Context, + repo *helmv1alpha1.HelmClusterAddonRepository, +) (InternalRepositoryState, error) { + logger := log.FromContext(ctx) + + existing := &sourcev1.HelmRepository{ + ObjectMeta: metav1.ObjectMeta{ + Name: utils.GetInternalHelmRepositoryName(repo.Name), + Namespace: s.TargetNamespace, + }, + } + + op, err := controllerutil.CreateOrPatch(ctx, s.Client, existing, func() error { + applyHelmRepositorySpec(repo, existing) + + return nil + }) + if err != nil { + return InternalRepositoryState{Present: true}, fmt.Errorf("creating helm repository: %w", err) + } + + if op != controllerutil.OperationResultNone { + logger.Info("Reconciled helm repository", "operation", op) + } + + state := InternalRepositoryState{Present: true} + + if stalled := apimeta.FindStatusCondition(existing.Status.Conditions, helmv1alpha1.ConditionTypeStalled); stalled != nil && + stalled.Status == metav1.ConditionTrue { + state.Stalled = true + state.Reason = stalled.Reason + state.Message = stalled.Message + + return state, nil + } + + cond, observed := status.IsConditionObserved(existing.Status.Conditions, helmv1alpha1.ConditionTypeReady, existing.Generation) + if !observed { + state.Reason = helmv1alpha1.ReasonReconciling + state.Message = "Waiting for the internal repository to be reconciled" + + return state, nil + } + + state.Ready = cond.Status == metav1.ConditionTrue + state.Reason = cond.Reason + state.Message = cond.Message + + return state, nil +} + func (s *HelmRepoService) RemoveHelmRepository(ctx context.Context, repoName string) error { name := utils.GetInternalHelmRepositoryName(repoName) nn := types.NamespacedName{Name: name, Namespace: s.TargetNamespace} diff --git a/images/operator-helm-controller/internal/services/helm_repo_service_test.go b/images/operator-helm-controller/internal/services/helm_repo_service_test.go new file mode 100644 index 00000000..c01fc7df --- /dev/null +++ b/images/operator-helm-controller/internal/services/helm_repo_service_test.go @@ -0,0 +1,165 @@ +/* +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" + + sourcev1 "github.com/werf/nelm-source-controller/api/v1" + 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" + + helmv1alpha1 "github.com/deckhouse/operator-helm/api/v1alpha1" + "github.com/deckhouse/operator-helm/internal/utils" +) + +func sourceScheme(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) + } + if err := sourcev1.AddToScheme(scheme); err != nil { + t.Fatalf("registering source scheme: %v", err) + } + + return scheme +} + +func newHelmRepoService(t *testing.T, objects ...client.Object) *HelmRepoService { + t.Helper() + + scheme := sourceScheme(t) + c := fake.NewClientBuilder().WithScheme(scheme).WithObjects(objects...).Build() + + return NewHelmRepoService(c, scheme, testNamespace) +} + +func testRepository() *helmv1alpha1.HelmClusterAddonRepository { + return &helmv1alpha1.HelmClusterAddonRepository{ + ObjectMeta: metav1.ObjectMeta{Name: "example", Generation: 1}, + Spec: helmv1alpha1.HelmClusterAddonRepositorySpec{URL: "https://example.invalid/charts"}, + } +} + +func TestEnsureInternalRepositoryStateReportsNotObservedAsNotReady(t *testing.T) { + repo := testRepository() + service := newHelmRepoService(t, repo) + + state, err := service.EnsureInternalRepositoryState(context.Background(), repo) + if err != nil { + t.Fatalf("EnsureInternalRepositoryState returned %v", err) + } + if !state.Present { + t.Fatal("helm repositories must report an internal object") + } + if state.Ready { + t.Fatal("a freshly created internal object must not be reported ready") + } +} + +func TestEnsureInternalRepositoryStateMirrorsConditions(t *testing.T) { + repo := testRepository() + // The spec and labels must already match what applyHelmRepositorySpec writes: + // otherwise CreateOrPatch mutates the object, the fake client bumps its + // generation, and the Ready condition stops counting as observed. + internal := &sourcev1.HelmRepository{ + ObjectMeta: metav1.ObjectMeta{ + Name: utils.GetInternalHelmRepositoryName(repo.Name), + Namespace: testNamespace, + Labels: map[string]string{ + helmv1alpha1.LabelManagedBy: helmv1alpha1.LabelManagedByValue, + helmv1alpha1.HelmClusterAddonRepositoryLabelSourceName: repo.Name, + }, + }, + Spec: sourcev1.HelmRepositorySpec{ + URL: repo.Spec.URL, + Interval: metav1.Duration{Duration: InternalRepositoryInterval}, + }, + Status: sourcev1.HelmRepositoryStatus{ + Conditions: []metav1.Condition{ + { + Type: helmv1alpha1.ConditionTypeReady, + Status: metav1.ConditionFalse, + Reason: "FetchFailed", + Message: "failed to fetch index", + LastTransitionTime: metav1.Now(), + }, + }, + }, + } + + service := newHelmRepoService(t, repo, internal) + + // The fixture is created with generation 0 and the condition observes 0, so + // the state must mirror the condition rather than report "not observed yet". + + state, err := service.EnsureInternalRepositoryState(context.Background(), repo) + if err != nil { + t.Fatalf("EnsureInternalRepositoryState returned %v", err) + } + if state.Ready { + t.Fatal("state must mirror Ready=False from the internal object") + } + if state.Reason != "FetchFailed" || state.Message != "failed to fetch index" { + t.Fatalf("state must translate reason and message, got %q / %q", state.Reason, state.Message) + } +} + +func TestEnsureInternalRepositoryStateReportsStalled(t *testing.T) { + repo := testRepository() + internal := &sourcev1.HelmRepository{ + ObjectMeta: metav1.ObjectMeta{ + Name: utils.GetInternalHelmRepositoryName(repo.Name), + Namespace: testNamespace, + Generation: 1, + }, + Status: sourcev1.HelmRepositoryStatus{ + Conditions: []metav1.Condition{ + { + Type: helmv1alpha1.ConditionTypeStalled, + Status: metav1.ConditionTrue, + Reason: "InvalidSecretRef", + Message: "secret not found", + ObservedGeneration: 1, + LastTransitionTime: metav1.Now(), + }, + }, + }, + } + + service := newHelmRepoService(t, repo, internal) + + state, err := service.EnsureInternalRepositoryState(context.Background(), repo) + if err != nil { + t.Fatalf("EnsureInternalRepositoryState returned %v", err) + } + if !state.Stalled { + t.Fatal("state must report Stalled from the internal object") + } + if state.Reason != "InvalidSecretRef" { + t.Fatalf("state reason is %q, want %q", state.Reason, "InvalidSecretRef") + } +} From fb9325c0b9c11884c6019a7179a6b356898c102f Mon Sep 17 00:00:00 2001 From: Ilya Drey Date: Tue, 1 Sep 2026 20:57:02 +0300 Subject: [PATCH 13/33] test(core): cover the API-failure and Stalled-precedence branches of EnsureInternalRepositoryState Signed-off-by: Ilya Drey --- .../services/helm_repo_service_test.go | 99 +++++++++++++++++++ 1 file changed, 99 insertions(+) diff --git a/images/operator-helm-controller/internal/services/helm_repo_service_test.go b/images/operator-helm-controller/internal/services/helm_repo_service_test.go index c01fc7df..ce8a4e77 100644 --- a/images/operator-helm-controller/internal/services/helm_repo_service_test.go +++ b/images/operator-helm-controller/internal/services/helm_repo_service_test.go @@ -18,6 +18,7 @@ package services import ( "context" + "errors" "testing" sourcev1 "github.com/werf/nelm-source-controller/api/v1" @@ -26,6 +27,7 @@ import ( clientgoscheme "k8s.io/client-go/kubernetes/scheme" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" helmv1alpha1 "github.com/deckhouse/operator-helm/api/v1alpha1" "github.com/deckhouse/operator-helm/internal/utils" @@ -163,3 +165,100 @@ func TestEnsureInternalRepositoryStateReportsStalled(t *testing.T) { t.Fatalf("state reason is %q, want %q", state.Reason, "InvalidSecretRef") } } + +// TestEnsureInternalRepositoryStateStalledPrecedesReady pins the precedence rule: +// Stalled=True must win even when a healthy, observed Ready=True condition sits +// right next to it. The fixture's spec and labels already match what +// applyHelmRepositorySpec writes (same reason as the mirroring test above), so +// CreateOrPatch is a no-op and the internal object's generation stays at 1 - +// which is what lets the Ready condition below count as observed. +func TestEnsureInternalRepositoryStateStalledPrecedesReady(t *testing.T) { + repo := testRepository() + internal := &sourcev1.HelmRepository{ + ObjectMeta: metav1.ObjectMeta{ + Name: utils.GetInternalHelmRepositoryName(repo.Name), + Namespace: testNamespace, + Generation: 1, + Labels: map[string]string{ + helmv1alpha1.LabelManagedBy: helmv1alpha1.LabelManagedByValue, + helmv1alpha1.HelmClusterAddonRepositoryLabelSourceName: repo.Name, + }, + }, + Spec: sourcev1.HelmRepositorySpec{ + URL: repo.Spec.URL, + Interval: metav1.Duration{Duration: InternalRepositoryInterval}, + }, + Status: sourcev1.HelmRepositoryStatus{ + Conditions: []metav1.Condition{ + { + Type: helmv1alpha1.ConditionTypeReady, + Status: metav1.ConditionTrue, + Reason: "Succeeded", + Message: "index fetched", + ObservedGeneration: 1, + LastTransitionTime: metav1.Now(), + }, + { + Type: helmv1alpha1.ConditionTypeStalled, + Status: metav1.ConditionTrue, + Reason: "InvalidSecretRef", + Message: "secret not found", + ObservedGeneration: 1, + LastTransitionTime: metav1.Now(), + }, + }, + }, + } + + service := newHelmRepoService(t, repo, internal) + + state, err := service.EnsureInternalRepositoryState(context.Background(), repo) + if err != nil { + t.Fatalf("EnsureInternalRepositoryState returned %v", err) + } + if !state.Stalled { + t.Fatal("Stalled=True must take precedence even when an observed Ready=True is also present") + } + if state.Ready { + t.Fatal("state must not report Ready when Stalled=True takes precedence") + } + if state.Reason != "InvalidSecretRef" { + t.Fatalf("state reason is %q, want the Stalled reason %q, not the Ready reason", state.Reason, "InvalidSecretRef") + } +} + +// TestEnsureInternalRepositoryStateReturnsAPIError verifies the split this task +// exists to create: an API failure while reconciling the internal object is the +// caller's problem and comes back as a non-nil error (still with Present: true, +// since the internal object does exist as far as the caller is concerned), not +// swallowed into the state. The failure is injected on Create because the fixture +// has no pre-existing internal HelmRepository, so CreateOrPatch's Get finds +// nothing and falls through to Create. +func TestEnsureInternalRepositoryStateReturnsAPIError(t *testing.T) { + repo := testRepository() + scheme := sourceScheme(t) + + sentinel := errors.New("synthetic create failure") + c := fake.NewClientBuilder(). + WithScheme(scheme). + WithObjects(repo). + WithInterceptorFuncs(interceptor.Funcs{ + Create: func(ctx context.Context, _ client.WithWatch, obj client.Object, opts ...client.CreateOption) error { + return sentinel + }, + }). + Build() + + service := NewHelmRepoService(c, scheme, testNamespace) + + state, err := service.EnsureInternalRepositoryState(context.Background(), repo) + if err == nil { + t.Fatal("EnsureInternalRepositoryState must return an error when the API call fails") + } + if !errors.Is(err, sentinel) { + t.Fatalf("returned error must wrap the underlying API failure, got %v", err) + } + if !state.Present { + t.Fatal("state must still report Present: true even when reconciling failed") + } +} From 65c9e52ed118d910236865ff1603744a2a23c203 Mon Sep 17 00:00:00 2001 From: Ilya Drey Date: Tue, 1 Sep 2026 21:03:07 +0300 Subject: [PATCH 14/33] feat(core): split repository sync into fetch and catalog phases Signed-off-by: Ilya Drey --- .../helmclusteraddonrepository/controller.go | 3 +- .../internal/services/repo_sync_service.go | 175 ++++++++++-------- .../services/repo_sync_service_test.go | 146 +++++++++++++++ 3 files changed, 248 insertions(+), 76 deletions(-) create mode 100644 images/operator-helm-controller/internal/services/repo_sync_service_test.go diff --git a/images/operator-helm-controller/internal/controller/helmclusteraddonrepository/controller.go b/images/operator-helm-controller/internal/controller/helmclusteraddonrepository/controller.go index e96564c0..0e54e421 100644 --- a/images/operator-helm-controller/internal/controller/helmclusteraddonrepository/controller.go +++ b/images/operator-helm-controller/internal/controller/helmclusteraddonrepository/controller.go @@ -26,6 +26,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/predicate" helmv1alpha1 "github.com/deckhouse/operator-helm/api/v1alpha1" + repoclient "github.com/deckhouse/operator-helm/internal/client/repository" "github.com/deckhouse/operator-helm/internal/manager/status" reconcile "github.com/deckhouse/operator-helm/internal/reconcile/helmclusteraddonrepository" "github.com/deckhouse/operator-helm/internal/services" @@ -43,7 +44,7 @@ func SetupWithManager(mgr ctrl.Manager) error { client, services.NewHelmRepoService(client, mgr.GetScheme(), helmv1alpha1.TargetNamespace), services.NewOCIRepoService(client, mgr.GetScheme(), helmv1alpha1.TargetNamespace), - services.NewRepoSyncService(client, mgr.GetScheme()), + services.NewRepoSyncService(client, mgr.GetScheme(), repoclient.NewClient), status.NewManager(client), ) 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 af552114..40cb9c53 100644 --- a/images/operator-helm-controller/internal/services/repo_sync_service.go +++ b/images/operator-helm-controller/internal/services/repo_sync_service.go @@ -21,6 +21,7 @@ import ( "fmt" "time" + "github.com/samber/lo" apimeta "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" @@ -34,7 +35,6 @@ import ( repoclient "github.com/deckhouse/operator-helm/internal/client/repository" "github.com/deckhouse/operator-helm/internal/manager/status" "github.com/deckhouse/operator-helm/internal/utils" - "github.com/samber/lo" ) const ( @@ -48,14 +48,25 @@ const ( type RepoSyncService struct { BaseService + + clientFactory RepoClientFactory } -func NewRepoSyncService(client client.Client, scheme *runtime.Scheme) *RepoSyncService { +// RepoClientFactory builds the client used to read a repository catalog. It is +// injected so the synchronization can be tested without a live repository. +type RepoClientFactory func(repoType utils.InternalRepositoryType) (repoclient.ClientInterface, error) + +func NewRepoSyncService(client client.Client, scheme *runtime.Scheme, factory RepoClientFactory) *RepoSyncService { + if factory == nil { + factory = repoclient.NewClient + } + return &RepoSyncService{ BaseService: BaseService{ Client: client, Scheme: scheme, }, + clientFactory: factory, } } @@ -86,52 +97,101 @@ func (r RepoSyncResult) InProgress() bool { } func (s *RepoSyncService) EnsureAddonCharts(ctx context.Context, repo *helmv1alpha1.HelmClusterAddonRepository, repoType utils.InternalRepositoryType) RepoSyncResult { - logger := log.FromContext(ctx) - if !isRepoSyncRequired(repo) { return RepoSyncResult{Status: status.Empty()} } else if !isRepoSyncInProgress(repo) { return RepoSyncResult{Status: status.Unknown(repo, helmv1alpha1.ReasonReconciling)} } - repoClient, err := repoclient.NewClient(repoType) + outcome := s.Sync(ctx, repo, repoType) + + switch { + case outcome.Fetch.Err != nil: + return RepoSyncResult{Status: status.Failed(repo, helmv1alpha1.ReasonSyncFailed, outcome.Fetch.Message, outcome.Fetch.Err)} + case outcome.Catalog.Err != nil: + return RepoSyncResult{Status: status.Failed(repo, helmv1alpha1.ReasonSyncFailed, "Failed to update the chart catalog", outcome.Catalog.Err)} + default: + return RepoSyncResult{Status: status.Success(repo)} + } +} + +// Sync reads the repository catalog and reconciles the HelmClusterAddonChart +// resources that mirror it. The two phases are reported separately: a fetch +// failure is about the remote, a catalog failure is about this cluster. +func (s *RepoSyncService) Sync( + ctx context.Context, + repo *helmv1alpha1.HelmClusterAddonRepository, + repoType utils.InternalRepositoryType, +) SyncOutcome { + charts, fetch := s.fetchCharts(ctx, repo, repoType) + if fetch.Err != nil { + return SyncOutcome{Fetch: fetch} + } + + return SyncOutcome{Fetch: fetch, Catalog: s.reconcileCatalog(ctx, repo, charts)} +} + +func (s *RepoSyncService) fetchCharts( + ctx context.Context, + repo *helmv1alpha1.HelmClusterAddonRepository, + repoType utils.InternalRepositoryType, +) ([]repoclient.Chart, FetchOutcome) { + repoClient, err := s.clientFactory(repoType) if err != nil { - return RepoSyncResult{ - Status: status.Failed( - repo, - helmv1alpha1.ReasonSyncFailed, - "Failed to get repository client on chart sync", - fmt.Errorf("getting repository client: %w", err), - ), + return nil, FetchOutcome{ + Err: err, + Terminal: true, + Reason: helmv1alpha1.ReasonUnsupportedRepositoryType, + Message: "Unsupported repository type", } } - var repoConfig *repoclient.RepoConfig - if repo.Spec.Auth != nil || repo.Spec.CACertificate != "" || repo.Spec.InsecureSkipVerify { - repoConfig = &repoclient.RepoConfig{ - Insecure: repo.Spec.InsecureSkipVerify, - } - if repo.Spec.Auth != nil { - repoConfig.Username = repo.Spec.Auth.Username - repoConfig.Password = repo.Spec.Auth.Password - } - if repo.Spec.CACertificate != "" { - repoConfig.CACertificate = repo.Spec.CACertificate - } + charts, err := repoClient.FetchCharts(ctx, repo.Spec.URL, buildRepoConfig(repo)) + if err == nil { + return charts, FetchOutcome{} } - charts, err := repoClient.FetchCharts(ctx, repo.Spec.URL, repoConfig) - if err != nil { - return RepoSyncResult{ - Status: status.Failed( - repo, - helmv1alpha1.ReasonSyncFailed, - "Failed to fetch charts from repository", - fmt.Errorf("fetching charts: %w", err), - ), + if terminal, ok := repoclient.AsTerminal(err); ok { + return nil, FetchOutcome{ + Err: err, + Terminal: true, + Reason: terminal.Reason, + Message: terminal.Message, } } + return nil, FetchOutcome{ + Err: err, + Reason: helmv1alpha1.ReasonSyncFailed, + Message: "Failed to read the repository catalog: " + err.Error(), + } +} + +func buildRepoConfig(repo *helmv1alpha1.HelmClusterAddonRepository) *repoclient.RepoConfig { + if repo.Spec.Auth == nil && repo.Spec.CACertificate == "" && !repo.Spec.InsecureSkipVerify { + return nil + } + + config := &repoclient.RepoConfig{ + Insecure: repo.Spec.InsecureSkipVerify, + CACertificate: repo.Spec.CACertificate, + } + + if repo.Spec.Auth != nil { + config.Username = repo.Spec.Auth.Username + config.Password = repo.Spec.Auth.Password + } + + return config +} + +func (s *RepoSyncService) reconcileCatalog( + ctx context.Context, + repo *helmv1alpha1.HelmClusterAddonRepository, + charts []repoclient.Chart, +) CatalogOutcome { + logger := log.FromContext(ctx) + desiredCharts := make(map[string]struct{}, len(charts)) for _, chart := range charts { @@ -141,9 +201,7 @@ func (s *RepoSyncService) EnsureAddonCharts(ctx context.Context, repo *helmv1alp addonChartName := utils.GetHelmClusterAddonChartName(repo.Name, chart.Name) existing := &helmv1alpha1.HelmClusterAddonChart{ - ObjectMeta: metav1.ObjectMeta{ - Name: addonChartName, - }, + ObjectMeta: metav1.ObjectMeta{Name: addonChartName}, } desiredCharts[existing.Name] = struct{}{} @@ -164,17 +222,11 @@ func (s *RepoSyncService) EnsureAddonCharts(ctx context.Context, repo *helmv1alp LabelRepositoryName: repo.Name, LabelChartName: chart.Name, } + return nil }) if err != nil { - return RepoSyncResult{ - Status: status.Failed( - repo, - helmv1alpha1.ReasonSyncFailed, - fmt.Sprintf("Failed to create HelmClusterAddonChart %q", addonChartName), - fmt.Errorf("cannot create or update HelmClusterAddonChart: %w", err), - ), - } + return CatalogOutcome{Err: fmt.Errorf("creating or updating chart %q: %w", addonChartName, err)} } if op != controllerutil.OperationResultNone { @@ -189,29 +241,13 @@ func (s *RepoSyncService) EnsureAddonCharts(ctx context.Context, repo *helmv1alp }) if err := s.Client.Status().Patch(ctx, existing, client.MergeFrom(base)); err != nil { - return RepoSyncResult{ - Status: status.Failed( - repo, - helmv1alpha1.ReasonSyncFailed, - fmt.Sprintf("Failed to update HelmClusterAddonChart %q versions", addonChartName), - fmt.Errorf("updating chart versions: %w", err), - ), - } + return CatalogOutcome{Err: fmt.Errorf("updating versions of chart %q: %w", addonChartName, err)} } - - logger.Info("Successfully synced HelmClusterAddonChart versions", "operation", op, "addonChartName", addonChartName) } var existingCharts helmv1alpha1.HelmClusterAddonChartList if err := s.Client.List(ctx, &existingCharts, client.MatchingLabels{LabelRepositoryName: repo.Name}); err != nil { - return RepoSyncResult{ - Status: status.Failed( - repo, - helmv1alpha1.ReasonSyncFailed, - "Failed to list stale charts for pruning", - fmt.Errorf("listing existing charts for pruning: %w", err), - ), - } + return CatalogOutcome{Err: fmt.Errorf("listing charts for pruning: %w", err)} } for _, chart := range existingCharts.Items { @@ -220,22 +256,11 @@ func (s *RepoSyncService) EnsureAddonCharts(ctx context.Context, repo *helmv1alp } if err := s.ensureResourceDeleted(ctx, types.NamespacedName{Name: chart.Name}, &chart); err != nil { - return RepoSyncResult{ - Status: status.Failed( - repo, - helmv1alpha1.ReasonSyncFailed, - "Failed to delete stale charts", - fmt.Errorf("deleting stale charts: %w", err), - ), - } + return CatalogOutcome{Err: fmt.Errorf("deleting stale charts: %w", err)} } } - logger.Info(fmt.Sprintf("Scheduling next repo sync in %s", ChartsSyncInterval)) - - return RepoSyncResult{ - Status: status.Success(repo), - } + return CatalogOutcome{} } func isRepoSyncRequired(repo *helmv1alpha1.HelmClusterAddonRepository) bool { 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 new file mode 100644 index 00000000..fcbb27e8 --- /dev/null +++ b/images/operator-helm-controller/internal/services/repo_sync_service_test.go @@ -0,0 +1,146 @@ +/* +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" + "errors" + "testing" + + "github.com/Masterminds/semver/v3" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + + helmv1alpha1 "github.com/deckhouse/operator-helm/api/v1alpha1" + repoclient "github.com/deckhouse/operator-helm/internal/client/repository" + "github.com/deckhouse/operator-helm/internal/utils" +) + +type stubRepoClient struct { + charts []repoclient.Chart + err error +} + +func (s stubRepoClient) FetchCharts(_ context.Context, _ string, _ *repoclient.RepoConfig) ([]repoclient.Chart, error) { + return s.charts, s.err +} + +func newRepoSyncService(t *testing.T, stub stubRepoClient, objects ...client.Object) (*RepoSyncService, client.Client) { + t.Helper() + + scheme := testScheme(t) + c := fake.NewClientBuilder(). + WithScheme(scheme). + WithObjects(objects...). + WithStatusSubresource(&helmv1alpha1.HelmClusterAddonChart{}, &helmv1alpha1.HelmClusterAddonRepository{}). + Build() + + factory := func(_ utils.InternalRepositoryType) (repoclient.ClientInterface, error) { + return stub, nil + } + + return NewRepoSyncService(c, scheme, factory), c +} + +func chartFixture(name, version string) repoclient.Chart { + return repoclient.Chart{ + Name: name, + Versions: []repoclient.ChartVersion{{Version: semver.MustParse(version), IconURL: "https://example.invalid/icon.png"}}, + } +} + +func TestSyncCreatesChartsAndRecordsVersions(t *testing.T) { + repo := testRepository() + service, c := newRepoSyncService(t, stubRepoClient{charts: []repoclient.Chart{chartFixture("podinfo", "6.7.1")}}, repo) + + outcome := service.Sync(context.Background(), repo, utils.InternalHelmRepository) + if outcome.Fetch.Err != nil { + t.Fatalf("fetch failed: %v", outcome.Fetch.Err) + } + if outcome.Catalog.Err != nil { + t.Fatalf("catalog update failed: %v", outcome.Catalog.Err) + } + + chart := &helmv1alpha1.HelmClusterAddonChart{} + key := client.ObjectKey{Name: utils.GetHelmClusterAddonChartName(repo.Name, "podinfo")} + if err := c.Get(context.Background(), key, chart); err != nil { + t.Fatalf("chart was not created: %v", err) + } + if len(chart.Status.Versions) != 1 || chart.Status.Versions[0].Version != "6.7.1" { + t.Fatalf("chart versions are %v, want [6.7.1]", chart.Status.Versions) + } +} + +func TestSyncPrunesStaleCharts(t *testing.T) { + repo := testRepository() + stale := &helmv1alpha1.HelmClusterAddonChart{ + ObjectMeta: metav1.ObjectMeta{ + Name: utils.GetHelmClusterAddonChartName(repo.Name, "removed"), + Labels: map[string]string{LabelRepositoryName: repo.Name, LabelChartName: "removed"}, + }, + } + + service, c := newRepoSyncService(t, stubRepoClient{charts: []repoclient.Chart{chartFixture("podinfo", "6.7.1")}}, repo, stale) + + outcome := service.Sync(context.Background(), repo, utils.InternalHelmRepository) + if outcome.Catalog.Err != nil { + t.Fatalf("catalog update failed: %v", outcome.Catalog.Err) + } + + err := c.Get(context.Background(), client.ObjectKeyFromObject(stale), &helmv1alpha1.HelmClusterAddonChart{}) + if err == nil { + t.Fatal("stale chart must be pruned") + } +} + +func TestSyncReportsTerminalFetchFailure(t *testing.T) { + repo := testRepository() + terminal := &repoclient.TerminalError{ + Reason: helmv1alpha1.ReasonAuthenticationFailed, + Message: "repository rejected the credentials (HTTP 401)", + } + + service, _ := newRepoSyncService(t, stubRepoClient{err: terminal}, repo) + + outcome := service.Sync(context.Background(), repo, utils.InternalHelmRepository) + if outcome.Fetch.Err == nil { + t.Fatal("expected a fetch failure") + } + if !outcome.Fetch.Terminal { + t.Fatal("a TerminalError must be reported as terminal") + } + if outcome.Fetch.Reason != helmv1alpha1.ReasonAuthenticationFailed { + t.Fatalf("fetch reason is %q, want %q", outcome.Fetch.Reason, helmv1alpha1.ReasonAuthenticationFailed) + } +} + +func TestSyncReportsTransientFetchFailure(t *testing.T) { + repo := testRepository() + service, _ := newRepoSyncService(t, stubRepoClient{err: errors.New("connection refused")}, repo) + + outcome := service.Sync(context.Background(), repo, utils.InternalHelmRepository) + if outcome.Fetch.Err == nil { + t.Fatal("expected a fetch failure") + } + if outcome.Fetch.Terminal { + t.Fatal("a plain error must stay retriable") + } + if outcome.Fetch.Reason != helmv1alpha1.ReasonSyncFailed { + t.Fatalf("fetch reason is %q, want %q", outcome.Fetch.Reason, helmv1alpha1.ReasonSyncFailed) + } +} From 920f82e0446aecd96905872a32d261d8a99d43d7 Mon Sep 17 00:00:00 2001 From: Ilya Drey Date: Tue, 1 Sep 2026 21:12:13 +0300 Subject: [PATCH 15/33] feat(core): rework HelmClusterAddonRepository status semantics Ready now reports whether the repository is usable: auxiliary resources are in place, the internal source object is healthy and the repository responded to a catalog read on the current spec. Reconciling and Stalled are added following kstatus and are present only while applicable. Synchronization is scheduled from status fields with an exponential backoff instead of a fixed interval. Signed-off-by: Ilya Drey --- .../internal/manager/status/manager.go | 31 +-- .../helmclusteraddonrepository/reconciler.go | 136 ++++++------ .../reconciler_test.go | 208 ++++++++++++++++++ .../internal/services/helm_repo_service.go | 78 +------ .../services/helm_repo_service_test.go | 34 +-- .../internal/services/oci_repo_service.go | 41 ---- .../internal/services/repo_sync_service.go | 69 ------ 7 files changed, 305 insertions(+), 292 deletions(-) create mode 100644 images/operator-helm-controller/internal/reconcile/helmclusteraddonrepository/reconciler_test.go diff --git a/images/operator-helm-controller/internal/manager/status/manager.go b/images/operator-helm-controller/internal/manager/status/manager.go index 78f92b93..db36d122 100644 --- a/images/operator-helm-controller/internal/manager/status/manager.go +++ b/images/operator-helm-controller/internal/manager/status/manager.go @@ -125,31 +125,20 @@ func (s *Manager) Update(ctx context.Context, obj ObjectWithConditions, mutatorF return s.Status().Patch(ctx, obj, client.MergeFrom(oldObj)) } -func (s *Manager) InitializeConditions(ctx context.Context, obj ObjectWithConditions, conditionTypes ...string) error { +// PatchStatus applies mutate to the object and patches the status subresource +// when it actually changed. It is the thin apply path used by reconcilers that +// compute the whole desired status themselves. +func (s *Manager) PatchStatus(ctx context.Context, obj ObjectWithConditions, mutate func()) error { oldObj := obj.DeepCopyObject().(ObjectWithConditions) - patchBase := client.MergeFrom(oldObj) - conditions := obj.GetConditions() - changed := false - - for _, t := range conditionTypes { - if meta.FindStatusCondition(*conditions, t) == nil { - meta.SetStatusCondition(conditions, metav1.Condition{ - Type: t, - Status: metav1.ConditionUnknown, - Reason: "Initialized", - }) - changed = true - } - } + mutate() - if changed { - logger := log.FromContext(ctx) - logger.Info("Initializing conditions", "name", obj.GetName(), "types", conditionTypes) + if reflect.DeepEqual(obj.GetStatus(), oldObj.GetStatus()) { + return nil + } - if err := s.Client.Status().Patch(ctx, obj, patchBase); err != nil { - return fmt.Errorf("initializing conditions: %w", err) - } + if err := s.Status().Patch(ctx, obj, client.MergeFrom(oldObj)); err != nil { + return fmt.Errorf("patching status: %w", err) } return nil diff --git a/images/operator-helm-controller/internal/reconcile/helmclusteraddonrepository/reconciler.go b/images/operator-helm-controller/internal/reconcile/helmclusteraddonrepository/reconciler.go index 6ef7ac9e..14fea78d 100644 --- a/images/operator-helm-controller/internal/reconcile/helmclusteraddonrepository/reconciler.go +++ b/images/operator-helm-controller/internal/reconcile/helmclusteraddonrepository/reconciler.go @@ -22,7 +22,6 @@ import ( "time" apierrors "k8s.io/apimachinery/pkg/api/errors" - apimeta "k8s.io/apimachinery/pkg/api/meta" "k8s.io/client-go/util/retry" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" @@ -75,14 +74,11 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (reco if apierrors.IsNotFound(err) { return reconcile.Result{}, nil } + return reconcile.Result{}, fmt.Errorf("getting helm cluster addon repository: %w", err) } - repoType, err := utils.GetRepositoryType(repo.Spec.URL) - if err != nil { - logger.Error(err, "failed to determine repository type") - return reconcile.Result{}, err - } + repoType, repoTypeErr := utils.GetRepositoryType(repo.Spec.URL) if !repo.DeletionTimestamp.IsZero() { return r.reconcileDelete(ctx, &repo, repoType) @@ -100,69 +96,82 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (reco // would not trigger a follow-up reconcile. } - var helmRepoRes services.HelmRepoResult - var ociRepoRes services.OCIRepoResult - var chartSyncRes services.RepoSyncResult + in := Inputs{ + Generation: repo.Generation, + Now: time.Now().UTC(), + Jitter: NewJitter(), + Current: *repo.Status.DeepCopy(), + } - switch repoType { - case utils.InternalHelmRepository: - helmRepoRes = r.helmRepositoryService.EnsureInternalHelmRepository(ctx, &repo) - case utils.InternalOCIRepository: - if err := r.helmRepositoryService.RemoveHelmRepository(ctx, repo.Name); err != nil { - ociRepoRes = services.OCIRepoResult{ - Status: status.Failed(&repo, helmv1alpha1.ReasonFailed, "Repository change failed", err), - } - break + if repoTypeErr != nil { + in.ConfigErr = &services.ConfigOutcome{ + Reason: helmv1alpha1.ReasonUnsupportedRepositoryType, + Message: repoTypeErr.Error(), + Err: repoTypeErr, } - ociRepoRes = r.ociRepositoryService.EnsureRepositorySecrets(ctx, &repo) - default: - err := fmt.Errorf("unsupported repository type: %q", repoType) - helmRepoRes = services.HelmRepoResult{Status: status.Failed(&repo, "UnsupportedRepositoryType", err.Error(), err)} + + return r.finish(ctx, &repo, in, false) } - if helmRepoRes.IsReady() || ociRepoRes.IsReady() { - chartSyncRes = r.chartSyncService.EnsureAddonCharts(ctx, &repo, repoType) - } else { - chartSyncRes = services.RepoSyncResult{Status: status.Failed(&repo, helmv1alpha1.ReasonRepositoryNotReady, helmRepoRes.Status.Message, nil)} + // Both services embed the same BaseRepoService with the same target namespace, + // so one of them reconciles the auxiliary secrets for either repository type. + in.SecretsErr = r.helmRepositoryService.EnsureSecrets(ctx, &repo) + + if in.SecretsErr == nil { + switch repoType { + case utils.InternalHelmRepository: + in.InternalRepository, in.InternalRepositoryErr = r.helmRepositoryService.EnsureInternalHelmRepository(ctx, &repo) + case utils.InternalOCIRepository: + // The url may have changed from helm to oci: drop the internal object + // that is no longer used. OCI repositories have none of their own. + in.InternalRepositoryErr = r.helmRepositoryService.RemoveHelmRepository(ctx, repo.Name) + } } - if err := r.reconcileForceAnnotation(ctx, req); err != nil { - return reconcile.Result{}, fmt.Errorf("failed to reconcile force annotation: %w", err) + forced := repo.ForceReconcileRequired() + + if in.SecretsErr == nil && in.InternalRepositoryErr == nil && + ShouldAttempt(in.Current, in.Generation, in.Now, forced) { + outcome := r.chartSyncService.Sync(ctx, &repo, repoType) + + in.Attempted = true + in.Fetch = &outcome.Fetch + in.Catalog = &outcome.Catalog } - if err := r.statusManager.Update( - ctx, - &repo, - status.NoopStatusMutator, - status.NoopStatusMapper, - helmRepoRes, - ociRepoRes, - chartSyncRes, - ); client.IgnoreNotFound(err) != nil { + return r.finish(ctx, &repo, in, in.Attempted) +} + +// finish applies the decision and consumes the force annotation when an attempt +// actually ran. The annotation is removed after the status patch so a conflict +// does not lose the request. +func (r *Reconciler) finish( + ctx context.Context, + repo *helmv1alpha1.HelmClusterAddonRepository, + in Inputs, + attempted bool, +) (reconcile.Result, error) { + decision := Evaluate(in) + + if err := r.statusManager.PatchStatus(ctx, repo, func() { + repo.Status = decision.Status + }); client.IgnoreNotFound(err) != nil { return reconcile.Result{}, fmt.Errorf("failed to update status: %w", err) } - // EnsureAddonCharts is a two-phase state machine: the first pass only marks - // the Synced condition Reconciling, the second pass performs the actual chart - // fetch. Run the second pass in the same reconcile (the status update above - // already persisted the Reconciling state and advanced the condition's - // LastTransitionTime) instead of relying on the status-update watch event to - // trigger it — otherwise predicates that ignore status-only changes would - // stall the scheduled sync. - if chartSyncRes.InProgress() { - chartSyncRes = r.chartSyncService.EnsureAddonCharts(ctx, &repo, repoType) - if err := r.statusManager.Update( - ctx, - &repo, - status.NoopStatusMutator, - status.NoopStatusMapper, - chartSyncRes, - ); client.IgnoreNotFound(err) != nil { - return reconcile.Result{}, fmt.Errorf("failed to update sync status: %w", err) + if attempted { + if err := r.reconcileForceAnnotation(ctx, client.ObjectKeyFromObject(repo)); err != nil { + return reconcile.Result{}, fmt.Errorf("failed to reconcile force annotation: %w", err) } } - return r.requeueAtSyncInterval(&repo) + if decision.Err != nil { + // Cluster write failures are handed to the work queue rate limiter; the + // schedule is re-established on the next pass. + return reconcile.Result{}, decision.Err + } + + return reconcile.Result{RequeueAfter: decision.RequeueAfter}, nil } func (r *Reconciler) reconcileDelete(ctx context.Context, repo *helmv1alpha1.HelmClusterAddonRepository, repoType utils.InternalRepositoryType) (reconcile.Result, error) { @@ -224,13 +233,14 @@ func (r *Reconciler) awaitInternalResourceDeletion(ctx context.Context, repo *he return reconcile.Result{RequeueAfter: internalResourceDeletionRequeueInterval}, nil } -func (r *Reconciler) reconcileForceAnnotation(ctx context.Context, req reconcile.Request) error { +func (r *Reconciler) reconcileForceAnnotation(ctx context.Context, key client.ObjectKey) error { var repo helmv1alpha1.HelmClusterAddonRepository - if err := r.Get(ctx, req.NamespacedName, &repo); err != nil { + if err := r.Get(ctx, key, &repo); err != nil { if apierrors.IsNotFound(err) { return nil } + return fmt.Errorf("getting helm cluster addon repository: %w", err) } @@ -248,15 +258,3 @@ func (r *Reconciler) reconcileForceAnnotation(ctx context.Context, req reconcile return nil } - -func (r *Reconciler) requeueAtSyncInterval(repo *helmv1alpha1.HelmClusterAddonRepository) (reconcile.Result, error) { - repoSyncCond := apimeta.FindStatusCondition(repo.Status.Conditions, helmv1alpha1.ConditionTypeSynced) - if repoSyncCond != nil { - remaining := time.Until(repoSyncCond.LastTransitionTime.Add(services.ChartsSyncInterval)) - if remaining > 0 { - return reconcile.Result{RequeueAfter: remaining}, nil - } - } - - return reconcile.Result{RequeueAfter: services.ChartsSyncInterval}, nil -} diff --git a/images/operator-helm-controller/internal/reconcile/helmclusteraddonrepository/reconciler_test.go b/images/operator-helm-controller/internal/reconcile/helmclusteraddonrepository/reconciler_test.go new file mode 100644 index 00000000..72dae2e0 --- /dev/null +++ b/images/operator-helm-controller/internal/reconcile/helmclusteraddonrepository/reconciler_test.go @@ -0,0 +1,208 @@ +/* +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 helmclusteraddonrepository + +import ( + "context" + "testing" + "time" + + "github.com/Masterminds/semver/v3" + sourcev1 "github.com/werf/nelm-source-controller/api/v1" + apimeta "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" + clientgoscheme "k8s.io/client-go/kubernetes/scheme" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/reconcile" + + helmv1alpha1 "github.com/deckhouse/operator-helm/api/v1alpha1" + repoclient "github.com/deckhouse/operator-helm/internal/client/repository" + "github.com/deckhouse/operator-helm/internal/manager/status" + "github.com/deckhouse/operator-helm/internal/services" + "github.com/deckhouse/operator-helm/internal/utils" +) + +type stubRepoClient struct { + charts []repoclient.Chart + err error +} + +func (s stubRepoClient) FetchCharts(_ context.Context, _ string, _ *repoclient.RepoConfig) ([]repoclient.Chart, error) { + return s.charts, s.err +} + +func newReconciler(t *testing.T, stub stubRepoClient, objects ...client.Object) (*Reconciler, client.Client) { + t.Helper() + + scheme := runtime.NewScheme() + for _, add := range []func(*runtime.Scheme) error{ + clientgoscheme.AddToScheme, + helmv1alpha1.AddToScheme, + sourcev1.AddToScheme, + } { + if err := add(scheme); err != nil { + t.Fatalf("registering scheme: %v", err) + } + } + + c := fake.NewClientBuilder(). + WithScheme(scheme). + WithObjects(objects...). + WithStatusSubresource( + &helmv1alpha1.HelmClusterAddonRepository{}, + &helmv1alpha1.HelmClusterAddonChart{}, + ). + Build() + + factory := func(_ utils.InternalRepositoryType) (repoclient.ClientInterface, error) { + return stub, nil + } + + r := New( + c, + services.NewHelmRepoService(c, scheme, helmv1alpha1.TargetNamespace), + services.NewOCIRepoService(c, scheme, helmv1alpha1.TargetNamespace), + services.NewRepoSyncService(c, scheme, factory), + status.NewManager(c), + ) + + return r, c +} + +func ociRepository() *helmv1alpha1.HelmClusterAddonRepository { + return &helmv1alpha1.HelmClusterAddonRepository{ + ObjectMeta: metav1.ObjectMeta{Name: "example", Generation: 1}, + Spec: helmv1alpha1.HelmClusterAddonRepositorySpec{URL: "oci://ghcr.io/example/podinfo"}, + } +} + +func reconcileUntilStable(t *testing.T, r *Reconciler, name string) reconcile.Result { + t.Helper() + + var result reconcile.Result + // The first pass only adds the finalizer path; two passes are enough to reach + // a stable state for a repository whose source responds. + for range 2 { + var err error + result, err = r.Reconcile(context.Background(), reconcile.Request{ + NamespacedName: types.NamespacedName{Name: name}, + }) + if err != nil { + t.Fatalf("Reconcile returned %v", err) + } + } + + return result +} + +func TestReconcileOCIRepositoryBecomesReady(t *testing.T) { + repo := ociRepository() + stub := stubRepoClient{charts: []repoclient.Chart{{ + Name: "podinfo", + Versions: []repoclient.ChartVersion{{Version: semver.MustParse("6.7.1")}}, + }}} + + r, c := newReconciler(t, stub, repo) + result := reconcileUntilStable(t, r, repo.Name) + + if result.RequeueAfter <= 0 || result.RequeueAfter > time.Hour { + t.Fatalf("expected a scheduled requeue, got %s", result.RequeueAfter) + } + + updated := &helmv1alpha1.HelmClusterAddonRepository{} + if err := c.Get(context.Background(), client.ObjectKeyFromObject(repo), updated); err != nil { + t.Fatalf("getting repository: %v", err) + } + + if !apimeta.IsStatusConditionTrue(updated.Status.Conditions, helmv1alpha1.ConditionTypeReady) { + t.Fatalf("Ready must be True, conditions: %v", updated.Status.Conditions) + } + if !apimeta.IsStatusConditionTrue(updated.Status.Conditions, helmv1alpha1.ConditionTypeSynced) { + t.Fatal("Synced must be True") + } + if apimeta.FindStatusCondition(updated.Status.Conditions, helmv1alpha1.ConditionTypeReconciling) != nil { + t.Fatal("Reconciling must be absent on a healthy repository") + } + if updated.Status.LastSuccessfulSyncTime == nil || updated.Status.NextSyncTime == nil { + t.Fatal("sync timestamps must be recorded") + } +} + +func TestReconcileSkipsFetchBeforeSchedule(t *testing.T) { + repo := ociRepository() + stub := stubRepoClient{charts: []repoclient.Chart{{ + Name: "podinfo", + Versions: []repoclient.ChartVersion{{Version: semver.MustParse("6.7.1")}}, + }}} + + r, c := newReconciler(t, stub, repo) + reconcileUntilStable(t, r, repo.Name) + + before := &helmv1alpha1.HelmClusterAddonRepository{} + if err := c.Get(context.Background(), client.ObjectKeyFromObject(repo), before); err != nil { + t.Fatalf("getting repository: %v", err) + } + + // A watch-driven pass before nextSyncTime must not move the schedule. + if _, err := r.Reconcile(context.Background(), reconcile.Request{ + NamespacedName: types.NamespacedName{Name: repo.Name}, + }); err != nil { + t.Fatalf("Reconcile returned %v", err) + } + + after := &helmv1alpha1.HelmClusterAddonRepository{} + if err := c.Get(context.Background(), client.ObjectKeyFromObject(repo), after); err != nil { + t.Fatalf("getting repository: %v", err) + } + + if !after.Status.NextSyncTime.Time.Equal(before.Status.NextSyncTime.Time) { + t.Fatalf("nextSyncTime moved without a due schedule: %s -> %s", + before.Status.NextSyncTime.Time, after.Status.NextSyncTime.Time) + } +} + +func TestReconcileTerminalFetchFailureStalls(t *testing.T) { + repo := ociRepository() + stub := stubRepoClient{err: &repoclient.TerminalError{ + Reason: helmv1alpha1.ReasonAuthenticationFailed, + Message: "repository rejected the credentials (HTTP 401)", + }} + + r, c := newReconciler(t, stub, repo) + reconcileUntilStable(t, r, repo.Name) + + updated := &helmv1alpha1.HelmClusterAddonRepository{} + if err := c.Get(context.Background(), client.ObjectKeyFromObject(repo), updated); err != nil { + t.Fatalf("getting repository: %v", err) + } + + stalled := apimeta.FindStatusCondition(updated.Status.Conditions, helmv1alpha1.ConditionTypeStalled) + if stalled == nil || stalled.Reason != helmv1alpha1.ReasonAuthenticationFailed { + t.Fatalf("expected Stalled=AuthenticationFailed, got %v", stalled) + } + if apimeta.FindStatusCondition(updated.Status.Conditions, helmv1alpha1.ConditionTypeReconciling) != nil { + t.Fatal("Reconciling and Stalled must be mutually exclusive") + } + // The fake client bumps generation on the finalizer update, so compare with + // the live object rather than with the fixture. + if updated.Status.ObservedGeneration != updated.Generation { + t.Fatalf("observedGeneration is %d, want %d", updated.Status.ObservedGeneration, updated.Generation) + } +} diff --git a/images/operator-helm-controller/internal/services/helm_repo_service.go b/images/operator-helm-controller/internal/services/helm_repo_service.go index 4bf002de..64f89763 100644 --- a/images/operator-helm-controller/internal/services/helm_repo_service.go +++ b/images/operator-helm-controller/internal/services/helm_repo_service.go @@ -37,10 +37,7 @@ import ( "github.com/deckhouse/operator-helm/internal/utils" ) -const ( - InternalRepositoryInterval = 5 * time.Minute - ChartsSyncInterval = 5 * time.Minute -) +const InternalRepositoryInterval = 5 * time.Minute type HelmRepoService struct { BaseRepoService @@ -58,80 +55,11 @@ func NewHelmRepoService(client client.Client, scheme *runtime.Scheme, namespace } } -var _ status.Provider = (*HelmRepoResult)(nil) - -type HelmRepoResult struct { - Status status.Status -} - -func (r HelmRepoResult) GetStatus() status.Status { - return r.Status -} - -func (r HelmRepoResult) IsReady() bool { - return r.Status.IsReady() -} - -func (r HelmRepoResult) GetConditionType() string { - return helmv1alpha1.ConditionTypeReady -} - -func (s *HelmRepoService) EnsureInternalHelmRepository(ctx context.Context, repo *helmv1alpha1.HelmClusterAddonRepository) HelmRepoResult { - logger := log.FromContext(ctx) - - if err := s.reconcileAuthSecret(ctx, repo); err != nil { - return HelmRepoResult{Status: status.Failed(repo, helmv1alpha1.ReasonFailed, "Failed to reconcile auth secret", err)} - } - - if err := s.reconcileTLSSecret(ctx, repo); err != nil { - return HelmRepoResult{Status: status.Failed(repo, helmv1alpha1.ReasonFailed, "Failed to reconcile tls secret", err)} - } - - existing := &sourcev1.HelmRepository{ - ObjectMeta: metav1.ObjectMeta{ - Name: utils.GetInternalHelmRepositoryName(repo.Name), - Namespace: s.TargetNamespace, - }, - } - - op, err := controllerutil.CreateOrPatch(ctx, s.Client, existing, func() error { - applyHelmRepositorySpec(repo, existing) - - return nil - }) - if err != nil { - return HelmRepoResult{ - Status: status.Failed( - repo, - helmv1alpha1.ReasonFailed, - "Failed to reconcile helm repository", - fmt.Errorf("creating helm repository: %w", err), - ), - } - } - - if op != controllerutil.OperationResultNone { - logger.Info("Reconciled helm repository", "operation", op) - } - - if cond, ok := status.IsConditionObserved(existing.Status.Conditions, helmv1alpha1.ConditionTypeReady, existing.Generation); ok { - return HelmRepoResult{Status: status.Status{ - Observed: ok, - Status: cond.Status, - ObservedGeneration: repo.Generation, - Reason: cond.Reason, - Message: cond.Message, - }} - } - - return HelmRepoResult{Status: status.Unknown(repo, helmv1alpha1.ReasonReconciling)} -} - -// EnsureInternalRepositoryState reconciles the internal HelmRepository and +// EnsureInternalHelmRepository reconciles the internal HelmRepository and // reports its observed state. The returned error is an API failure that the // caller must surface to the work queue; an unhealthy internal object is not an // error and is reported through the state instead. -func (s *HelmRepoService) EnsureInternalRepositoryState( +func (s *HelmRepoService) EnsureInternalHelmRepository( ctx context.Context, repo *helmv1alpha1.HelmClusterAddonRepository, ) (InternalRepositoryState, error) { diff --git a/images/operator-helm-controller/internal/services/helm_repo_service_test.go b/images/operator-helm-controller/internal/services/helm_repo_service_test.go index ce8a4e77..6fb33b2c 100644 --- a/images/operator-helm-controller/internal/services/helm_repo_service_test.go +++ b/images/operator-helm-controller/internal/services/helm_repo_service_test.go @@ -66,13 +66,13 @@ func testRepository() *helmv1alpha1.HelmClusterAddonRepository { } } -func TestEnsureInternalRepositoryStateReportsNotObservedAsNotReady(t *testing.T) { +func TestEnsureInternalHelmRepositoryReportsNotObservedAsNotReady(t *testing.T) { repo := testRepository() service := newHelmRepoService(t, repo) - state, err := service.EnsureInternalRepositoryState(context.Background(), repo) + state, err := service.EnsureInternalHelmRepository(context.Background(), repo) if err != nil { - t.Fatalf("EnsureInternalRepositoryState returned %v", err) + t.Fatalf("EnsureInternalHelmRepository returned %v", err) } if !state.Present { t.Fatal("helm repositories must report an internal object") @@ -82,7 +82,7 @@ func TestEnsureInternalRepositoryStateReportsNotObservedAsNotReady(t *testing.T) } } -func TestEnsureInternalRepositoryStateMirrorsConditions(t *testing.T) { +func TestEnsureInternalHelmRepositoryMirrorsConditions(t *testing.T) { repo := testRepository() // The spec and labels must already match what applyHelmRepositorySpec writes: // otherwise CreateOrPatch mutates the object, the fake client bumps its @@ -118,9 +118,9 @@ func TestEnsureInternalRepositoryStateMirrorsConditions(t *testing.T) { // The fixture is created with generation 0 and the condition observes 0, so // the state must mirror the condition rather than report "not observed yet". - state, err := service.EnsureInternalRepositoryState(context.Background(), repo) + state, err := service.EnsureInternalHelmRepository(context.Background(), repo) if err != nil { - t.Fatalf("EnsureInternalRepositoryState returned %v", err) + t.Fatalf("EnsureInternalHelmRepository returned %v", err) } if state.Ready { t.Fatal("state must mirror Ready=False from the internal object") @@ -130,7 +130,7 @@ func TestEnsureInternalRepositoryStateMirrorsConditions(t *testing.T) { } } -func TestEnsureInternalRepositoryStateReportsStalled(t *testing.T) { +func TestEnsureInternalHelmRepositoryReportsStalled(t *testing.T) { repo := testRepository() internal := &sourcev1.HelmRepository{ ObjectMeta: metav1.ObjectMeta{ @@ -154,9 +154,9 @@ func TestEnsureInternalRepositoryStateReportsStalled(t *testing.T) { service := newHelmRepoService(t, repo, internal) - state, err := service.EnsureInternalRepositoryState(context.Background(), repo) + state, err := service.EnsureInternalHelmRepository(context.Background(), repo) if err != nil { - t.Fatalf("EnsureInternalRepositoryState returned %v", err) + t.Fatalf("EnsureInternalHelmRepository returned %v", err) } if !state.Stalled { t.Fatal("state must report Stalled from the internal object") @@ -166,13 +166,13 @@ func TestEnsureInternalRepositoryStateReportsStalled(t *testing.T) { } } -// TestEnsureInternalRepositoryStateStalledPrecedesReady pins the precedence rule: +// TestEnsureInternalHelmRepositoryStalledPrecedesReady pins the precedence rule: // Stalled=True must win even when a healthy, observed Ready=True condition sits // right next to it. The fixture's spec and labels already match what // applyHelmRepositorySpec writes (same reason as the mirroring test above), so // CreateOrPatch is a no-op and the internal object's generation stays at 1 - // which is what lets the Ready condition below count as observed. -func TestEnsureInternalRepositoryStateStalledPrecedesReady(t *testing.T) { +func TestEnsureInternalHelmRepositoryStalledPrecedesReady(t *testing.T) { repo := testRepository() internal := &sourcev1.HelmRepository{ ObjectMeta: metav1.ObjectMeta{ @@ -212,9 +212,9 @@ func TestEnsureInternalRepositoryStateStalledPrecedesReady(t *testing.T) { service := newHelmRepoService(t, repo, internal) - state, err := service.EnsureInternalRepositoryState(context.Background(), repo) + state, err := service.EnsureInternalHelmRepository(context.Background(), repo) if err != nil { - t.Fatalf("EnsureInternalRepositoryState returned %v", err) + t.Fatalf("EnsureInternalHelmRepository returned %v", err) } if !state.Stalled { t.Fatal("Stalled=True must take precedence even when an observed Ready=True is also present") @@ -227,14 +227,14 @@ func TestEnsureInternalRepositoryStateStalledPrecedesReady(t *testing.T) { } } -// TestEnsureInternalRepositoryStateReturnsAPIError verifies the split this task +// TestEnsureInternalHelmRepositoryReturnsAPIError verifies the split this task // exists to create: an API failure while reconciling the internal object is the // caller's problem and comes back as a non-nil error (still with Present: true, // since the internal object does exist as far as the caller is concerned), not // swallowed into the state. The failure is injected on Create because the fixture // has no pre-existing internal HelmRepository, so CreateOrPatch's Get finds // nothing and falls through to Create. -func TestEnsureInternalRepositoryStateReturnsAPIError(t *testing.T) { +func TestEnsureInternalHelmRepositoryReturnsAPIError(t *testing.T) { repo := testRepository() scheme := sourceScheme(t) @@ -251,9 +251,9 @@ func TestEnsureInternalRepositoryStateReturnsAPIError(t *testing.T) { service := NewHelmRepoService(c, scheme, testNamespace) - state, err := service.EnsureInternalRepositoryState(context.Background(), repo) + state, err := service.EnsureInternalHelmRepository(context.Background(), repo) if err == nil { - t.Fatal("EnsureInternalRepositoryState must return an error when the API call fails") + t.Fatal("EnsureInternalHelmRepository must return an error when the API call fails") } if !errors.Is(err, sentinel) { t.Fatalf("returned error must wrap the underlying API failure, got %v", err) 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 5f6db411..ce6ccbb6 100644 --- a/images/operator-helm-controller/internal/services/oci_repo_service.go +++ b/images/operator-helm-controller/internal/services/oci_repo_service.go @@ -123,47 +123,6 @@ func (s *OCIRepoService) EnsureInternalOCIRepository(ctx context.Context, addon } } -func (s *OCIRepoService) EnsureRepositorySecrets(ctx context.Context, repo *helmv1alpha1.HelmClusterAddonRepository) OCIRepoResult { - if err := s.reconcileAuthSecret(ctx, repo); err != nil { - return OCIRepoResult{ - Status: status.Status{ - ConditionType: helmv1alpha1.ConditionTypeReady, - Observed: true, - Status: metav1.ConditionFalse, - ObservedGeneration: repo.Generation, - Reason: helmv1alpha1.ReasonFailed, - Message: "Failed to reconcile auth secret", - Err: err, - }, - } - } - - if err := s.reconcileTLSSecret(ctx, repo); err != nil { - return OCIRepoResult{ - Status: status.Status{ - ConditionType: helmv1alpha1.ConditionTypeReady, - Observed: true, - Status: metav1.ConditionFalse, - ObservedGeneration: repo.Generation, - Reason: helmv1alpha1.ReasonFailed, - Message: "Failed to reconcile tls secret", - Err: err, - }, - } - } - - return OCIRepoResult{ - Artifact: &meta.Artifact{}, - Status: status.Status{ - ConditionType: helmv1alpha1.ConditionTypeReady, - Observed: true, - Status: metav1.ConditionTrue, - ObservedGeneration: repo.Generation, - Reason: helmv1alpha1.ReasonSuccess, - }, - } -} - func (s *OCIRepoService) CleanupOCIRepository(ctx context.Context, repoName string) error { resources := []struct { name string 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 40cb9c53..4ccf9744 100644 --- a/images/operator-helm-controller/internal/services/repo_sync_service.go +++ b/images/operator-helm-controller/internal/services/repo_sync_service.go @@ -19,10 +19,8 @@ package services import ( "context" "fmt" - "time" "github.com/samber/lo" - apimeta "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" @@ -33,7 +31,6 @@ import ( helmv1alpha1 "github.com/deckhouse/operator-helm/api/v1alpha1" repoclient "github.com/deckhouse/operator-helm/internal/client/repository" - "github.com/deckhouse/operator-helm/internal/manager/status" "github.com/deckhouse/operator-helm/internal/utils" ) @@ -70,51 +67,6 @@ func NewRepoSyncService(client client.Client, scheme *runtime.Scheme, factory Re } } -var _ status.Provider = (*RepoSyncResult)(nil) - -type RepoSyncResult struct { - Status status.Status -} - -func (r RepoSyncResult) GetStatus() status.Status { - return r.Status -} - -func (r RepoSyncResult) IsReady() bool { - return r.Status.IsReady() -} - -func (r RepoSyncResult) GetConditionType() string { - return helmv1alpha1.ConditionTypeSynced -} - -// InProgress reports the first phase of the sync state machine: the Synced -// condition has just been marked Reconciling and the actual chart fetch still -// needs to run. The caller performs that fetch in the same reconcile so progress -// does not depend on a status-update watch event. -func (r RepoSyncResult) InProgress() bool { - return r.Status.Status == metav1.ConditionUnknown && r.Status.Reason == helmv1alpha1.ReasonReconciling -} - -func (s *RepoSyncService) EnsureAddonCharts(ctx context.Context, repo *helmv1alpha1.HelmClusterAddonRepository, repoType utils.InternalRepositoryType) RepoSyncResult { - if !isRepoSyncRequired(repo) { - return RepoSyncResult{Status: status.Empty()} - } else if !isRepoSyncInProgress(repo) { - return RepoSyncResult{Status: status.Unknown(repo, helmv1alpha1.ReasonReconciling)} - } - - outcome := s.Sync(ctx, repo, repoType) - - switch { - case outcome.Fetch.Err != nil: - return RepoSyncResult{Status: status.Failed(repo, helmv1alpha1.ReasonSyncFailed, outcome.Fetch.Message, outcome.Fetch.Err)} - case outcome.Catalog.Err != nil: - return RepoSyncResult{Status: status.Failed(repo, helmv1alpha1.ReasonSyncFailed, "Failed to update the chart catalog", outcome.Catalog.Err)} - default: - return RepoSyncResult{Status: status.Success(repo)} - } -} - // Sync reads the repository catalog and reconciles the HelmClusterAddonChart // resources that mirror it. The two phases are reported separately: a fetch // failure is about the remote, a catalog failure is about this cluster. @@ -262,24 +214,3 @@ func (s *RepoSyncService) reconcileCatalog( return CatalogOutcome{} } - -func isRepoSyncRequired(repo *helmv1alpha1.HelmClusterAddonRepository) bool { - if repo.ForceReconcileRequired() { - return true - } - - syncCond := apimeta.FindStatusCondition(repo.Status.Conditions, helmv1alpha1.ConditionTypeSynced) - if syncCond != nil && syncCond.Status == metav1.ConditionTrue && syncCond.LastTransitionTime.UTC().Add(ChartsSyncInterval).After(time.Now().UTC()) { - return false - } - return true -} - -func isRepoSyncInProgress(repo *helmv1alpha1.HelmClusterAddonRepository) bool { - syncCond := apimeta.FindStatusCondition(repo.Status.Conditions, helmv1alpha1.ConditionTypeSynced) - if syncCond != nil && syncCond.Status == metav1.ConditionUnknown && syncCond.Reason == helmv1alpha1.ReasonReconciling { - return true - } - - return false -} From eb9a291d60f1930ecbcec99afd9667324c2d8e7e Mon Sep 17 00:00:00 2001 From: Ilya Drey Date: Tue, 1 Sep 2026 21:23:41 +0300 Subject: [PATCH 16/33] fix(core): clean up internal objects when the repository url no longer parses The url validation regex on the CRD is looser than url.Parse, so a repository whose internal objects already exist can be edited to an unparsable url and then deleted. reconcileDelete matched no case for an unknown repository type and left the internal HelmRepository and both auxiliary secrets behind; the helm cleanup is now the default branch. Also log a repository read failure, which is reported only through a condition, and drop a duplicated error wrap. Signed-off-by: Ilya Drey --- .../evaluate_test.go | 2 +- .../helmclusteraddonrepository/reconciler.go | 27 ++++++--- .../reconciler_test.go | 56 +++++++++++++++++++ 3 files changed, 77 insertions(+), 8 deletions(-) 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 47f0013d..f609a20b 100644 --- a/images/operator-helm-controller/internal/reconcile/helmclusteraddonrepository/evaluate_test.go +++ b/images/operator-helm-controller/internal/reconcile/helmclusteraddonrepository/evaluate_test.go @@ -381,7 +381,7 @@ func TestEvaluateDecisionErr(t *testing.T) { t.Run(tc.name, func(t *testing.T) { got := Evaluate(tc.in) - if got.Err != tc.wantErr { + if !errors.Is(got.Err, tc.wantErr) { t.Fatalf("Decision.Err is %v, want %v", got.Err, tc.wantErr) } }) diff --git a/images/operator-helm-controller/internal/reconcile/helmclusteraddonrepository/reconciler.go b/images/operator-helm-controller/internal/reconcile/helmclusteraddonrepository/reconciler.go index 14fea78d..fe66d55e 100644 --- a/images/operator-helm-controller/internal/reconcile/helmclusteraddonrepository/reconciler.go +++ b/images/operator-helm-controller/internal/reconcile/helmclusteraddonrepository/reconciler.go @@ -153,10 +153,16 @@ func (r *Reconciler) finish( ) (reconcile.Result, error) { decision := Evaluate(in) + if in.Fetch != nil && in.Fetch.Err != nil { + // A repository read failure is not returned to the work queue — its retry + // is carried by nextSyncTime — so this is the only place it is logged. + log.FromContext(ctx).Error(in.Fetch.Err, in.Fetch.Message, "repository", repo.Name) + } + if err := r.statusManager.PatchStatus(ctx, repo, func() { repo.Status = decision.Status }); client.IgnoreNotFound(err) != nil { - return reconcile.Result{}, fmt.Errorf("failed to update status: %w", err) + return reconcile.Result{}, err } if attempted { @@ -182,7 +188,19 @@ func (r *Reconciler) reconcileDelete(ctx context.Context, repo *helmv1alpha1.Hel } switch repoType { - case utils.InternalHelmRepository: + case utils.InternalOCIRepository: + if err := r.ociRepositoryService.CleanupOCIRepository(ctx, repo.Name); err != nil && !apierrors.IsNotFound(err) { + _ = r.statusManager.MarkDeletionFailed(ctx, repo, "internal repository", err) + return reconcile.Result{}, err + } + default: + // The helm path is the default rather than a case of its own because an + // unknown repository type is a state a real repository can reach: the url + // validation regex on the CRD is looser than url.Parse, so a repository + // whose internal objects already exist can be edited to a url that no + // longer parses and then deleted. Cleaning up the helm way is safe for + // either type — it removes both auxiliary secrets and tolerates a missing + // internal repository — and leaving it out would orphan them. helmRepo, err := r.helmRepositoryService.CleanupHelmRepository(ctx, repo.Name) if err != nil && !apierrors.IsNotFound(err) { _ = r.statusManager.MarkDeletionFailed(ctx, repo, "internal repository", err) @@ -191,11 +209,6 @@ func (r *Reconciler) reconcileDelete(ctx context.Context, repo *helmv1alpha1.Hel if helmRepo != nil { return r.awaitInternalResourceDeletion(ctx, repo, "internal repository", helmRepo) } - case utils.InternalOCIRepository: - if err := r.ociRepositoryService.CleanupOCIRepository(ctx, repo.Name); err != nil && !apierrors.IsNotFound(err) { - _ = r.statusManager.MarkDeletionFailed(ctx, repo, "internal repository", err) - return reconcile.Result{}, err - } } if err := retry.RetryOnConflict(retry.DefaultRetry, func() error { 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 72dae2e0..4954d8db 100644 --- a/images/operator-helm-controller/internal/reconcile/helmclusteraddonrepository/reconciler_test.go +++ b/images/operator-helm-controller/internal/reconcile/helmclusteraddonrepository/reconciler_test.go @@ -23,6 +23,8 @@ import ( "github.com/Masterminds/semver/v3" sourcev1 "github.com/werf/nelm-source-controller/api/v1" + corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" apimeta "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" @@ -140,6 +142,9 @@ func TestReconcileOCIRepositoryBecomesReady(t *testing.T) { if apimeta.FindStatusCondition(updated.Status.Conditions, helmv1alpha1.ConditionTypeReconciling) != nil { t.Fatal("Reconciling must be absent on a healthy repository") } + if apimeta.FindStatusCondition(updated.Status.Conditions, helmv1alpha1.ConditionTypeStalled) != nil { + t.Fatal("Stalled must be absent on a healthy repository") + } if updated.Status.LastSuccessfulSyncTime == nil || updated.Status.NextSyncTime == nil { t.Fatal("sync timestamps must be recorded") } @@ -197,6 +202,9 @@ func TestReconcileTerminalFetchFailureStalls(t *testing.T) { if stalled == nil || stalled.Reason != helmv1alpha1.ReasonAuthenticationFailed { t.Fatalf("expected Stalled=AuthenticationFailed, got %v", stalled) } + if !apimeta.IsStatusConditionFalse(updated.Status.Conditions, helmv1alpha1.ConditionTypeReady) { + t.Fatalf("Ready must be False while Stalled, conditions: %v", updated.Status.Conditions) + } if apimeta.FindStatusCondition(updated.Status.Conditions, helmv1alpha1.ConditionTypeReconciling) != nil { t.Fatal("Reconciling and Stalled must be mutually exclusive") } @@ -206,3 +214,51 @@ func TestReconcileTerminalFetchFailureStalls(t *testing.T) { t.Fatalf("observedGeneration is %d, want %d", updated.Status.ObservedGeneration, updated.Generation) } } + +// TestReconcileDeleteCleansUpWhenURLNoLongerParses covers a repository whose url +// satisfies the CRD's validation regex but is rejected by url.Parse, so the +// repository type cannot be determined. Its internal objects were created while +// the url still parsed, so the deletion path must still remove them. +func TestReconcileDeleteCleansUpWhenURLNoLongerParses(t *testing.T) { + now := metav1.Now() + repo := &helmv1alpha1.HelmClusterAddonRepository{ + ObjectMeta: metav1.ObjectMeta{ + Name: "example", + Generation: 1, + Finalizers: []string{helmv1alpha1.FinalizerName}, + DeletionTimestamp: &now, + }, + // Passes the CRD rule ^(https?|oci)://.+$ and fails url.Parse. + Spec: helmv1alpha1.HelmClusterAddonRepositorySpec{URL: "https://exa mple.invalid/charts"}, + } + + if _, err := utils.GetRepositoryType(repo.Spec.URL); err == nil { + t.Fatal("the fixture url must be unparsable, otherwise the test proves nothing") + } + + internalRepo := &sourcev1.HelmRepository{ObjectMeta: metav1.ObjectMeta{ + Name: utils.GetInternalHelmRepositoryName(repo.Name), + Namespace: helmv1alpha1.TargetNamespace, + }} + authSecret := &corev1.Secret{ObjectMeta: metav1.ObjectMeta{ + Name: utils.GetInternalRepositoryAuthSecretName(repo.Name), + Namespace: helmv1alpha1.TargetNamespace, + }} + tlsSecret := &corev1.Secret{ObjectMeta: metav1.ObjectMeta{ + Name: utils.GetInternalRepositoryTLSSecretName(repo.Name), + Namespace: helmv1alpha1.TargetNamespace, + }} + + r, c := newReconciler(t, stubRepoClient{}, repo, internalRepo, authSecret, tlsSecret) + + // The first pass deletes the internal objects and waits for the internal + // repository to disappear; the second removes the finalizer. + reconcileUntilStable(t, r, repo.Name) + + for _, obj := range []client.Object{internalRepo, authSecret, tlsSecret} { + key := client.ObjectKeyFromObject(obj) + if err := c.Get(context.Background(), key, obj.DeepCopyObject().(client.Object)); !apierrors.IsNotFound(err) { + t.Fatalf("%s must be deleted, got %v", key, err) + } + } +} From 62ee8cb5234c05cdea0f429c73bc1238956999ac Mon Sep 17 00:00:00 2001 From: Ilya Drey Date: Tue, 1 Sep 2026 21:39:50 +0300 Subject: [PATCH 17/33] test(core): cover repository stall and recovery in e2e Signed-off-by: Ilya Drey --- .../helmclusteraddonrepository/lifecycle.go | 93 +++++++++++++++++++ tests/e2e/internal/util/resource.go | 31 +++++++ 2 files changed, 124 insertions(+) diff --git a/tests/e2e/helmclusteraddonrepository/lifecycle.go b/tests/e2e/helmclusteraddonrepository/lifecycle.go index 96338772..874acc18 100644 --- a/tests/e2e/helmclusteraddonrepository/lifecycle.go +++ b/tests/e2e/helmclusteraddonrepository/lifecycle.go @@ -79,6 +79,29 @@ func DefineLifecycleTests(repoType, repoURL string) { created, ) + By("Healthy repository must carry no abnormal-true conditions") + util.UntilConditionAbsent( + apiv1alpha1.ConditionTypeReconciling, + framework.LongTimeout, + created, + ) + util.UntilConditionAbsent( + apiv1alpha1.ConditionTypeStalled, + framework.LongTimeout, + created, + ) + + By("Repository must report its synchronization schedule") + Eventually(func(g Gomega) { + current, err := f.OperatorClient().HelmV1alpha1(). + HelmClusterAddonRepositories(). + Get(context.Background(), repoName, metav1.GetOptions{}) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(current.Status.LastSuccessfulSyncTime).NotTo(BeNil()) + g.Expect(current.Status.NextSyncTime).NotTo(BeNil()) + g.Expect(current.Status.ConsecutiveFetchFailures).To(BeZero()) + }).WithTimeout(framework.LongTimeout).WithPolling(framework.PollingInterval).Should(Succeed()) + By("Should have existing HelmClusterAddonChart") labelSelector := fmt.Sprintf("repository=%s", repoName) charts, err := f.OperatorClient(). @@ -134,3 +157,73 @@ var _ = Describe("Create HelmClusterAddonRepository with invalid url", Ordered, Expect(err).To(MatchError(ContainSubstring("is invalid: spec.url"))) }) }) + +var _ = Describe("HelmClusterAddonRepository with an unreachable source", Ordered, func() { + f := framework.NewFramework("repository-lifecycle") + repoName := "repo-source-not-found" + + BeforeAll(func() { + DeferCleanup(f.After) + f.Before() + }) + + // No AssertNoErrorsFor here on purpose: this scenario breaks the repository + // deliberately, so error-level log lines from the controller are expected. + + It("should stall on a missing source and recover after the url is fixed", func() { + repo := &apiv1alpha1.HelmClusterAddonRepository{ + ObjectMeta: metav1.ObjectMeta{Name: repoName}, + Spec: apiv1alpha1.HelmClusterAddonRepositorySpec{ + URL: "https://stefanprodan.github.io/podinfo-does-not-exist", + }, + } + + created, err := f.OperatorClient().HelmV1alpha1(). + HelmClusterAddonRepositories(). + Create(context.Background(), repo, metav1.CreateOptions{}) + Expect(err).NotTo(HaveOccurred()) + + f.DeferDelete(created) + + By("Waiting for the repository to become Stalled") + util.UntilConditionTrue( + apiv1alpha1.ConditionTypeStalled, + framework.LongTimeout, + created, + ) + + By("Reconciling must be absent while Stalled") + util.UntilConditionAbsent( + apiv1alpha1.ConditionTypeReconciling, + framework.LongTimeout, + created, + ) + + By("Fixing the url") + Eventually(func(g Gomega) { + current, err := f.OperatorClient().HelmV1alpha1(). + HelmClusterAddonRepositories(). + Get(context.Background(), repoName, metav1.GetOptions{}) + g.Expect(err).NotTo(HaveOccurred()) + + current.Spec.URL = "https://stefanprodan.github.io/podinfo" + + _, err = f.OperatorClient().HelmV1alpha1(). + HelmClusterAddonRepositories(). + Update(context.Background(), current, metav1.UpdateOptions{}) + g.Expect(err).NotTo(HaveOccurred()) + }).WithTimeout(framework.LongTimeout).WithPolling(framework.PollingInterval).Should(Succeed()) + + By("Waiting for the repository to recover") + util.UntilConditionTrue( + apiv1alpha1.ConditionTypeReady, + framework.LongTimeout, + created, + ) + util.UntilConditionAbsent( + apiv1alpha1.ConditionTypeStalled, + framework.LongTimeout, + created, + ) + }) +}) diff --git a/tests/e2e/internal/util/resource.go b/tests/e2e/internal/util/resource.go index 018dcc46..2b0e4beb 100644 --- a/tests/e2e/internal/util/resource.go +++ b/tests/e2e/internal/util/resource.go @@ -175,6 +175,37 @@ func UntilConditionReason(conditionType, expectedReason string, timeout time.Dur }).WithTimeout(timeout).WithPolling(framework.PollingInterval).Should(Succeed()) } +// UntilConditionAbsent waits until the specified condition is not present on the +// objects. Abnormal-true conditions are removed rather than set to False, so +// absence is the assertion that matters. +func UntilConditionAbsent(conditionType string, timeout time.Duration, objs ...client.Object) { + GinkgoHelper() + + Eventually(func(g Gomega) { + for _, obj := range objs { + u := toUnstructured(obj) + err := framework.GetClients().GenericClient().Get( + context.Background(), client.ObjectKeyFromObject(obj), u, + ) + g.Expect(err).NotTo(HaveOccurred()) + + conditions, _, err := unstructured.NestedSlice(u.Object, "status", "conditions") + g.Expect(err).NotTo(HaveOccurred(), + "failed to access status.conditions of %s", u.GetName()) + + for _, c := range conditions { + m, ok := c.(map[string]interface{}) + if !ok { + continue + } + t, _ := m["type"].(string) + g.Expect(t).NotTo(Equal(conditionType), + "condition %s must be absent on %s", conditionType, u.GetName()) + } + } + }).WithTimeout(timeout).WithPolling(framework.PollingInterval).Should(Succeed()) +} + func untilObjectField(fieldPath, expected string, timeout time.Duration, objs ...client.Object) { GinkgoHelper() Eventually(func(g Gomega) { From d7b47e378e3a122f15caeb0db955441db006f1c5 Mon Sep 17 00:00:00 2001 From: Ilya Drey Date: Tue, 1 Sep 2026 21:48:57 +0300 Subject: [PATCH 18/33] fix(core): use post-update object for recovery waits in e2e Signed-off-by: Ilya Drey --- .../e2e/helmclusteraddonrepository/lifecycle.go | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/tests/e2e/helmclusteraddonrepository/lifecycle.go b/tests/e2e/helmclusteraddonrepository/lifecycle.go index 874acc18..f8df08c3 100644 --- a/tests/e2e/helmclusteraddonrepository/lifecycle.go +++ b/tests/e2e/helmclusteraddonrepository/lifecycle.go @@ -200,6 +200,14 @@ var _ = Describe("HelmClusterAddonRepository with an unreachable source", Ordere ) By("Fixing the url") + // The waits below must use the object as it exists AFTER the update: + // correcting the url bumps metadata.generation, and UntilConditionStatus + // compares the condition's observedGeneration against the generation of + // the object handed to it. Reusing `created`, frozen at generation 1 by + // the Create call, would compare 2 against 1 on every poll and fail the + // spec deterministically even though recovery worked. + var recovered *apiv1alpha1.HelmClusterAddonRepository + Eventually(func(g Gomega) { current, err := f.OperatorClient().HelmV1alpha1(). HelmClusterAddonRepositories(). @@ -208,22 +216,24 @@ var _ = Describe("HelmClusterAddonRepository with an unreachable source", Ordere current.Spec.URL = "https://stefanprodan.github.io/podinfo" - _, err = f.OperatorClient().HelmV1alpha1(). + updated, err := f.OperatorClient().HelmV1alpha1(). HelmClusterAddonRepositories(). Update(context.Background(), current, metav1.UpdateOptions{}) g.Expect(err).NotTo(HaveOccurred()) + + recovered = updated }).WithTimeout(framework.LongTimeout).WithPolling(framework.PollingInterval).Should(Succeed()) By("Waiting for the repository to recover") util.UntilConditionTrue( apiv1alpha1.ConditionTypeReady, framework.LongTimeout, - created, + recovered, ) util.UntilConditionAbsent( apiv1alpha1.ConditionTypeStalled, framework.LongTimeout, - created, + recovered, ) }) }) From 7487cd70f06e74d782b936f26bd31db6f6f35950 Mon Sep 17 00:00:00 2001 From: Ilya Drey Date: Tue, 1 Sep 2026 21:53:53 +0300 Subject: [PATCH 19/33] docs(core): document repository conditions and sync schedule Signed-off-by: Ilya Drey --- docs/README.md | 30 ++++++++++++++++++++++++++++++ docs/README.ru.md | 30 ++++++++++++++++++++++++++++++ docs/RELEASE_NOTES.md | 16 ++++++++++++++++ docs/RELEASE_NOTES.ru.md | 16 ++++++++++++++++ 4 files changed, 92 insertions(+) diff --git a/docs/README.md b/docs/README.md index 929ebc31..d27fd7d0 100644 --- a/docs/README.md +++ b/docs/README.md @@ -32,3 +32,33 @@ The following custom resources are used to manage Helm charts in the module: - A HelmClusterAddon resource referencing a specific HelmClusterAddonChart can only be created as a single instance in the cluster. This is because Helm charts can contain custom resource definitions (CRDs), and installing them multiple times at the cluster level is not allowed. See [usage examples](example.html) for practical scenarios. + +## Repository status + +`HelmClusterAddonRepository` reports four conditions. + +`Ready` tells whether the repository is usable: its auxiliary resources are in +place, its internal source object is healthy, and the repository responded to a +catalog read on the current spec. A transient read failure does not flip `Ready` +to `False` — installed addons keep working and only the catalog goes stale. + +`Synced` tells whether the chart catalog is up to date. + +`Reconciling` and `Stalled` follow the kstatus convention and are present only +while they apply. `Reconciling` means work is in progress or a retry is +scheduled; `Stalled` means the repository will not recover on its own. + +| Ready | Synced | What it means | What to do | +|---|---|---|---| +| True | True | The repository is healthy. | Nothing. | +| True | False | The catalog read failed but the repository was usable before. | Check the `Reconciling` message and `Last Sync`. Retries are already scheduled. | +| False | True | The catalog is fresh, but the source of chart artifacts is unhealthy. | Check the `Ready` message: it is translated from the internal source object. | +| False | False | The repository is unreachable or misconfigured. | Check `Stalled`: `AuthenticationFailed`, `SourceNotFound` and `InvalidRepositoryURL` need a change in `spec`. | +| Unknown | any | The first catalog read on the current spec has not succeeded yet. | Wait for the next attempt shown in `Next Sync`. | + +Synchronization runs every 5 minutes. After a failed read the delay doubles — +5m, 10m, 20m, 40m — up to one hour, and the repository is reported as `Stalled` +with reason `RetriesExceeded` once the delay reaches the cap. Retries continue +at that cadence, because the cause may disappear on the repository side. The +schedule is visible in `status.nextSyncTime` and with +`kubectl get helmclusteraddonrepository -o wide`. diff --git a/docs/README.ru.md b/docs/README.ru.md index c15c6cc1..6fd750ba 100644 --- a/docs/README.ru.md +++ b/docs/README.ru.md @@ -32,3 +32,33 @@ weight: 10 - Ресурс HelmClusterAddon, ссылающийся на заданный HelmClusterAddonChart, может быть создан в кластере только в единственном экземпляре. Это обусловлено тем, что Helm-чарты могут содержать определения кастомных ресурсов (CRD), повторная установка которых на уровне кластера недопустима. Примеры использования приведены в разделе [примеры использования](example.html). + +## Статус репозитория + +`HelmClusterAddonRepository` сообщает о себе четырьмя условиями. + +`Ready` показывает, пригоден ли репозиторий: вспомогательные ресурсы на месте, +внутренний объект источника исправен, и репозиторий ответил на чтение каталога +на текущей спецификации. Транзиентная ошибка чтения не переводит `Ready` в +`False` — установленные аддоны продолжают работать, устаревает только каталог. + +`Synced` показывает, актуален ли каталог чартов. + +`Reconciling` и `Stalled` следуют соглашению kstatus и присутствуют, только +когда применимы. `Reconciling` означает, что работа выполняется или запланирован +повтор; `Stalled` — что репозиторий сам не восстановится. + +| Ready | Synced | Что означает | Что делать | +|---|---|---|---| +| True | True | Репозиторий исправен. | Ничего. | +| True | False | Чтение каталога не удалось, но до этого репозиторий был пригоден. | Посмотреть сообщение `Reconciling` и колонку `Last Sync`. Повторы уже запланированы. | +| False | True | Каталог свежий, но источник артефактов чартов неисправен. | Посмотреть сообщение `Ready` — оно транслируется от внутреннего объекта источника. | +| False | False | Репозиторий недоступен или настроен неверно. | Посмотреть `Stalled`: `AuthenticationFailed`, `SourceNotFound` и `InvalidRepositoryURL` требуют правки `spec`. | +| Unknown | любое | Первое чтение каталога на текущей спецификации ещё не удалось. | Дождаться следующей попытки, время которой указано в `Next Sync`. | + +Синхронизация выполняется каждые 5 минут. После неудачного чтения задержка +удваивается — 5m, 10m, 20m, 40m — до часа, и по достижении потолка репозиторий +переводится в `Stalled` с причиной `RetriesExceeded`. Повторы при этом +продолжаются, потому что причина может уйти на стороне репозитория. Расписание +видно в `status.nextSyncTime` и через +`kubectl get helmclusteraddonrepository -o wide`. diff --git a/docs/RELEASE_NOTES.md b/docs/RELEASE_NOTES.md index 78ccdbbc..32b4fbae 100644 --- a/docs/RELEASE_NOTES.md +++ b/docs/RELEASE_NOTES.md @@ -3,6 +3,22 @@ title: "Release Notes" description: "Release notes for Deckhouse operator-helm." --- +## Unreleased + +### Breaking Changes + +* `HelmClusterAddonRepository` condition `Ready` changed its meaning. It now reports whether the repository is usable as a whole — auxiliary resources, the internal source object and a confirmed catalog read on the current spec — instead of only the readiness of the internal source object. Alerting and dashboards that treat `Ready` as "the internal source object is ready" must be reviewed. + +### New Features + +* `HelmClusterAddonRepository` reports `Reconciling` and `Stalled` conditions following the kstatus convention. They are present only while applicable. +* `HelmClusterAddonRepository` status carries `lastSuccessfulSyncTime`, `nextSyncTime` and `consecutiveFetchFailures`, and `kubectl get` shows `Last Sync`, `Age` and, with `-o wide`, `Next Sync` and the `Ready` message. +* Repository reads that fail are retried with an exponential backoff — 5m, 10m, 20m, 40m, up to one hour — instead of a fixed 5 minute interval. + +### Bug Fixes + +* A single chart version that is not valid semver no longer fails the whole repository synchronization; the version is skipped. + ## v0.1.0 ### New Features diff --git a/docs/RELEASE_NOTES.ru.md b/docs/RELEASE_NOTES.ru.md index fb5242d1..0f4b1974 100644 --- a/docs/RELEASE_NOTES.ru.md +++ b/docs/RELEASE_NOTES.ru.md @@ -3,6 +3,22 @@ title: "Релизы" description: "Релизы Deckhouse operator-helm." --- +## Unreleased + +### Критические изменения + +* Условие (condition) `Ready` у `HelmClusterAddonRepository` изменило смысл. Теперь оно показывает, пригоден ли репозиторий в целом — на месте ли вспомогательные ресурсы, исправен ли внутренний объект источника и подтверждено ли чтение каталога на текущей спецификации, — а не только готовность внутреннего объекта источника. Алертинг и дашборды, которые трактуют `Ready` как «внутренний объект источника готов», нужно пересмотреть. + +### Новые возможности + +* `HelmClusterAddonRepository` сообщает условия (conditions) `Reconciling` и `Stalled` по соглашению kstatus. Они присутствуют, только когда применимы. +* В статусе `HelmClusterAddonRepository` появились поля `lastSuccessfulSyncTime`, `nextSyncTime` и `consecutiveFetchFailures`, а `kubectl get` показывает `Last Sync`, `Age` и, с флагом `-o wide`, `Next Sync` и сообщение `Ready`. +* Неудачные попытки чтения репозитория повторяются с экспоненциальной задержкой — 5m, 10m, 20m, 40m, вплоть до часа — вместо фиксированного интервала в 5 минут. + +### Исправления + +* Одна версия чарта с невалидным semver больше не приводит к сбою всей синхронизации репозитория; такая версия пропускается. + ## v0.1.0 ### Новые возможности From c39ea94dd675d878d3e134dcee346f8afc4dac74 Mon Sep 17 00:00:00 2001 From: Ilya Drey Date: Tue, 1 Sep 2026 21:56:33 +0300 Subject: [PATCH 20/33] docs(core): drop hand-written entries from generated release notes Signed-off-by: Ilya Drey --- docs/RELEASE_NOTES.md | 16 ---------------- docs/RELEASE_NOTES.ru.md | 16 ---------------- 2 files changed, 32 deletions(-) diff --git a/docs/RELEASE_NOTES.md b/docs/RELEASE_NOTES.md index 32b4fbae..78ccdbbc 100644 --- a/docs/RELEASE_NOTES.md +++ b/docs/RELEASE_NOTES.md @@ -3,22 +3,6 @@ title: "Release Notes" description: "Release notes for Deckhouse operator-helm." --- -## Unreleased - -### Breaking Changes - -* `HelmClusterAddonRepository` condition `Ready` changed its meaning. It now reports whether the repository is usable as a whole — auxiliary resources, the internal source object and a confirmed catalog read on the current spec — instead of only the readiness of the internal source object. Alerting and dashboards that treat `Ready` as "the internal source object is ready" must be reviewed. - -### New Features - -* `HelmClusterAddonRepository` reports `Reconciling` and `Stalled` conditions following the kstatus convention. They are present only while applicable. -* `HelmClusterAddonRepository` status carries `lastSuccessfulSyncTime`, `nextSyncTime` and `consecutiveFetchFailures`, and `kubectl get` shows `Last Sync`, `Age` and, with `-o wide`, `Next Sync` and the `Ready` message. -* Repository reads that fail are retried with an exponential backoff — 5m, 10m, 20m, 40m, up to one hour — instead of a fixed 5 minute interval. - -### Bug Fixes - -* A single chart version that is not valid semver no longer fails the whole repository synchronization; the version is skipped. - ## v0.1.0 ### New Features diff --git a/docs/RELEASE_NOTES.ru.md b/docs/RELEASE_NOTES.ru.md index 0f4b1974..fb5242d1 100644 --- a/docs/RELEASE_NOTES.ru.md +++ b/docs/RELEASE_NOTES.ru.md @@ -3,22 +3,6 @@ title: "Релизы" description: "Релизы Deckhouse operator-helm." --- -## Unreleased - -### Критические изменения - -* Условие (condition) `Ready` у `HelmClusterAddonRepository` изменило смысл. Теперь оно показывает, пригоден ли репозиторий в целом — на месте ли вспомогательные ресурсы, исправен ли внутренний объект источника и подтверждено ли чтение каталога на текущей спецификации, — а не только готовность внутреннего объекта источника. Алертинг и дашборды, которые трактуют `Ready` как «внутренний объект источника готов», нужно пересмотреть. - -### Новые возможности - -* `HelmClusterAddonRepository` сообщает условия (conditions) `Reconciling` и `Stalled` по соглашению kstatus. Они присутствуют, только когда применимы. -* В статусе `HelmClusterAddonRepository` появились поля `lastSuccessfulSyncTime`, `nextSyncTime` и `consecutiveFetchFailures`, а `kubectl get` показывает `Last Sync`, `Age` и, с флагом `-o wide`, `Next Sync` и сообщение `Ready`. -* Неудачные попытки чтения репозитория повторяются с экспоненциальной задержкой — 5m, 10m, 20m, 40m, вплоть до часа — вместо фиксированного интервала в 5 минут. - -### Исправления - -* Одна версия чарта с невалидным semver больше не приводит к сбою всей синхронизации репозитория; такая версия пропускается. - ## v0.1.0 ### Новые возможности From dfc43c88d68bc9614616cba459c830b958f60c47 Mon Sep 17 00:00:00 2001 From: Ilya Drey Date: Tue, 1 Sep 2026 22:02:59 +0300 Subject: [PATCH 21/33] docs(core): name all sync-status fields and default kubectl columns Signed-off-by: Ilya Drey --- docs/README.md | 9 +++++++-- docs/README.ru.md | 7 ++++++- 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/docs/README.md b/docs/README.md index d27fd7d0..6f43a7b4 100644 --- a/docs/README.md +++ b/docs/README.md @@ -33,7 +33,7 @@ The following custom resources are used to manage Helm charts in the module: See [usage examples](example.html) for practical scenarios. -## Repository status +## Repository Status `HelmClusterAddonRepository` reports four conditions. @@ -61,4 +61,9 @@ Synchronization runs every 5 minutes. After a failed read the delay doubles — with reason `RetriesExceeded` once the delay reaches the cap. Retries continue at that cadence, because the cause may disappear on the repository side. The schedule is visible in `status.nextSyncTime` and with -`kubectl get helmclusteraddonrepository -o wide`. +`kubectl get helmclusteraddonrepository -o wide`. `kubectl get +helmclusteraddonrepository` shows the `Last Sync` and `Age` columns by +default, and `-o wide` adds `Next Sync` and the `Ready` message. The same +values are in the status itself as `lastSuccessfulSyncTime` and +`nextSyncTime`, alongside `consecutiveFetchFailures`, which counts the +consecutive failed reads driving the backoff. diff --git a/docs/README.ru.md b/docs/README.ru.md index 6fd750ba..0791e744 100644 --- a/docs/README.ru.md +++ b/docs/README.ru.md @@ -61,4 +61,9 @@ weight: 10 переводится в `Stalled` с причиной `RetriesExceeded`. Повторы при этом продолжаются, потому что причина может уйти на стороне репозитория. Расписание видно в `status.nextSyncTime` и через -`kubectl get helmclusteraddonrepository -o wide`. +`kubectl get helmclusteraddonrepository -o wide`. `kubectl get +helmclusteraddonrepository` по умолчанию показывает колонки `Last Sync` и +`Age`, а `-o wide` добавляет `Next Sync` и сообщение из `Ready`. Те же +значения лежат в самом статусе — `lastSuccessfulSyncTime` и `nextSyncTime`, — +рядом с `consecutiveFetchFailures`, который считает подряд идущие неудачные +чтения, определяющие задержку повтора. From 41cd55677a09dae63f8caf468e605c6242fb44b8 Mon Sep 17 00:00:00 2001 From: Ilya Drey Date: Tue, 1 Sep 2026 22:30:31 +0300 Subject: [PATCH 22/33] fix(core): keep repository conditions honest between sync attempts Signed-off-by: Ilya Drey --- .../helmclusteraddonrepository/evaluate.go | 42 ++++++++- .../evaluate_test.go | 88 +++++++++++++++++++ .../kstatus_test.go | 12 +++ 3 files changed, 139 insertions(+), 3 deletions(-) diff --git a/images/operator-helm-controller/internal/reconcile/helmclusteraddonrepository/evaluate.go b/images/operator-helm-controller/internal/reconcile/helmclusteraddonrepository/evaluate.go index 905abedf..54257a39 100644 --- a/images/operator-helm-controller/internal/reconcile/helmclusteraddonrepository/evaluate.go +++ b/images/operator-helm-controller/internal/reconcile/helmclusteraddonrepository/evaluate.go @@ -242,6 +242,19 @@ func evaluateReconciling( reason: helmv1alpha1.ReasonProgressingWithRetry, message: "Retrying after a chart catalog update failure", } + case !in.Attempted && carriesCatalogFailure(in): + // The catalog write failure is handed to the work queue, whose immediate + // retry runs a pass with no attempt, so the case above cannot see it. Carry + // the retry forward the way evaluateStalled carries its verdict: without it + // the object would show Ready=True, Synced=False and no abnormal-true + // condition at all, which kstatus reads as healthy, and the fetch-failure + // counter — which deliberately ignores catalog failures — would never + // escalate it to Stalled either. + return abnormalCondition{ + set: true, + reason: helmv1alpha1.ReasonProgressingWithRetry, + message: "Retrying after a chart catalog update failure", + } case failures > 0: return abnormalCondition{ set: true, @@ -259,12 +272,35 @@ func evaluateReconciling( } } +// carriesCatalogFailure reports whether the current status still records an +// unresolved chart catalog write failure. Only a record made for the current +// generation counts: a generation bump voids evidence about the previous spec, +// mirroring hasEvidence and the carry-forward in evaluateStalled. +func carriesCatalogFailure(in Inputs) bool { + cond := apimeta.FindStatusCondition(in.Current.Conditions, helmv1alpha1.ConditionTypeSynced) + + return cond != nil && cond.Status == metav1.ConditionFalse && + cond.Reason == helmv1alpha1.ReasonCatalogUpdateFailed && + cond.ObservedGeneration == in.Generation +} + // hasEvidence reports whether the repository is already proven usable on the -// current generation: the previous verdict was True and was made for this spec. +// current generation: a fetch succeeded for this spec. func hasEvidence(current helmv1alpha1.HelmClusterAddonRepositoryStatus, generation int64) bool { - cond := apimeta.FindStatusCondition(current.Conditions, helmv1alpha1.ConditionTypeReady) + // Evidence is "a fetch succeeded on this spec". Ready alone cannot carry it: + // a higher-priority rule (an unhealthy internal repository, a failed secret) + // owns Ready on the very pass where the fetch succeeded, overwriting it. + // Synced is written only on a pass that attempted, and is True only when the + // fetch and the catalog write both succeeded, so Synced=True on the current + // generation means exactly that. + for _, conditionType := range []string{helmv1alpha1.ConditionTypeReady, helmv1alpha1.ConditionTypeSynced} { + if cond := apimeta.FindStatusCondition(current.Conditions, conditionType); cond != nil && + cond.Status == metav1.ConditionTrue && cond.ObservedGeneration == generation { + return true + } + } - return cond != nil && cond.Status == metav1.ConditionTrue && cond.ObservedGeneration == generation + return false } func setCondition( 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 f609a20b..b1614d45 100644 --- a/images/operator-helm-controller/internal/reconcile/helmclusteraddonrepository/evaluate_test.go +++ b/images/operator-helm-controller/internal/reconcile/helmclusteraddonrepository/evaluate_test.go @@ -69,6 +69,66 @@ func stalledStatus(generation, staleGeneration int64, reason string) helmv1alpha return status } +// syncedNotReadyStatus builds the status left behind by the pass on which the +// first fetch succeeded while the internal repository was still unhealthy: +// Synced records the successful read, but Ready was written False by the +// higher-priority internal-repository rule, so Ready alone carries no evidence. +func syncedNotReadyStatus(generation int64) helmv1alpha1.HelmClusterAddonRepositoryStatus { + return helmv1alpha1.HelmClusterAddonRepositoryStatus{ + ObservedGeneration: generation, + Conditions: []metav1.Condition{ + { + Type: helmv1alpha1.ConditionTypeReady, + Status: metav1.ConditionFalse, + Reason: "FetchFailed", + Message: "failed to fetch index", + ObservedGeneration: generation, + LastTransitionTime: metav1.NewTime(testNow.Add(-time.Minute)), + }, + { + Type: helmv1alpha1.ConditionTypeSynced, + Status: metav1.ConditionTrue, + Reason: helmv1alpha1.ReasonSuccess, + ObservedGeneration: generation, + LastTransitionTime: metav1.NewTime(testNow.Add(-time.Minute)), + }, + { + Type: helmv1alpha1.ConditionTypeReconciling, + Status: metav1.ConditionTrue, + Reason: helmv1alpha1.ReasonReconciling, + Message: "failed to fetch index", + ObservedGeneration: generation, + LastTransitionTime: metav1.NewTime(testNow.Add(-time.Minute)), + }, + }, + } +} + +// catalogFailedStatus builds the status left behind by a pass whose fetch +// succeeded and whose catalog write failed: Ready stays latched True, Synced is +// False with CatalogUpdateFailed and Reconciling carries the retry. +func catalogFailedStatus(generation int64) helmv1alpha1.HelmClusterAddonRepositoryStatus { + status := readyStatus(generation) + apimeta.SetStatusCondition(&status.Conditions, metav1.Condition{ + Type: helmv1alpha1.ConditionTypeSynced, + Status: metav1.ConditionFalse, + Reason: helmv1alpha1.ReasonCatalogUpdateFailed, + Message: "Failed to update the chart catalog: etcdserver: request timed out", + ObservedGeneration: generation, + LastTransitionTime: metav1.NewTime(testNow.Add(-time.Minute)), + }) + apimeta.SetStatusCondition(&status.Conditions, metav1.Condition{ + Type: helmv1alpha1.ConditionTypeReconciling, + Status: metav1.ConditionTrue, + Reason: helmv1alpha1.ReasonProgressingWithRetry, + Message: "Retrying after a chart catalog update failure", + ObservedGeneration: generation, + LastTransitionTime: metav1.NewTime(testNow.Add(-time.Minute)), + }) + + return status +} + func conditionOf(t *testing.T, status helmv1alpha1.HelmClusterAddonRepositoryStatus, conditionType string) *metav1.Condition { t.Helper() @@ -243,6 +303,34 @@ func TestEvaluateConditions(t *testing.T) { wantReady: metav1.ConditionTrue, wantReadyReason: helmv1alpha1.ReasonSuccess, wantSynced: metav1.ConditionTrue, }, + { + // The fetch succeeded on an earlier pass, but the internal repository + // was unhealthy then, so the higher-priority rule owned Ready and wrote + // it False. Ready alone therefore carries no evidence; Synced=True on + // this generation does, and must keep the repository from falling back + // to Unknown/AwaitingInitialSync until the next scheduled sync. + name: "synced carries the evidence when Ready was owned by another rule", + in: Inputs{ + Generation: 1, Now: testNow, Current: syncedNotReadyStatus(1), + InternalRepository: services.InternalRepositoryState{Present: true, Ready: true}, + }, + wantReady: metav1.ConditionTrue, wantReadyReason: helmv1alpha1.ReasonSuccess, + wantSynced: metav1.ConditionTrue, + }, + { + // The work-queue retry after a catalog write failure runs a pass with no + // attempt. Without a carry-forward the repository would show Ready=True, + // Synced=False and no abnormal-true condition at all — Current to + // kstatus — and ConsecutiveFetchFailures never escalates it to Stalled. + name: "catalog write failure keeps Reconciling on a pass with no attempt", + in: Inputs{ + Generation: 1, Now: testNow, Current: catalogFailedStatus(1), + InternalRepository: services.InternalRepositoryState{Present: true, Ready: true}, + }, + wantReady: metav1.ConditionTrue, wantReadyReason: helmv1alpha1.ReasonSuccess, + wantSynced: metav1.ConditionFalse, + wantReconciling: helmv1alpha1.ReasonProgressingWithRetry, + }, { name: "generation bump voids a stale stalled reason when no attempt runs", in: Inputs{ diff --git a/images/operator-helm-controller/internal/reconcile/helmclusteraddonrepository/kstatus_test.go b/images/operator-helm-controller/internal/reconcile/helmclusteraddonrepository/kstatus_test.go index d472ef38..2594c6f7 100644 --- a/images/operator-helm-controller/internal/reconcile/helmclusteraddonrepository/kstatus_test.go +++ b/images/operator-helm-controller/internal/reconcile/helmclusteraddonrepository/kstatus_test.go @@ -92,6 +92,18 @@ func TestKstatusVerdicts(t *testing.T) { }, want: status.FailedStatus, }, + { + // The work-queue retry after a catalog write failure runs a pass with no + // attempt. Without the carry-forward in evaluateReconciling the object + // would carry no abnormal-true condition and kstatus would call a + // permanently broken catalog write Current. + name: "catalog write failure stays in progress between attempts", + in: Inputs{ + Generation: 1, Now: testNow, Current: catalogFailedStatus(1), + InternalRepository: services.InternalRepositoryState{Present: true, Ready: true}, + }, + want: status.InProgressStatus, + }, { name: "stalled is not masked by a lagging observedGeneration", in: Inputs{ From f87de7e5f2aaa0bfbfd4c670c8f4fa211808f825 Mon Sep 17 00:00:00 2001 From: Ilya Drey Date: Tue, 1 Sep 2026 22:30:33 +0300 Subject: [PATCH 23/33] fix(core): report the cause behind an exhausted repository read Signed-off-by: Ilya Drey --- .../internal/client/repository/helm.go | 19 +++++++++++++++++++ .../internal/client/repository/helm_test.go | 6 ++++++ 2 files changed, 25 insertions(+) diff --git a/images/operator-helm-controller/internal/client/repository/helm.go b/images/operator-helm-controller/internal/client/repository/helm.go index d81eef22..685fa7e2 100644 --- a/images/operator-helm-controller/internal/client/repository/helm.go +++ b/images/operator-helm-controller/internal/client/repository/helm.go @@ -69,6 +69,13 @@ func (c *helmRepositoryClient) FetchCharts(ctx context.Context, url string, conf ctx, cancel := context.WithTimeout(ctx, 8*time.Second) defer cancel() + // lastErr keeps the cause of the most recent retriable failure. The backoff + // helper reports only its own timeout once the steps are exhausted, and a + // transient read failure — 5xx, DNS, connection refused, TLS — is the most + // common one there is: without this the operator would surface "timed out + // waiting for the condition" as the whole diagnostic. + var lastErr error + err := wait.ExponentialBackoffWithContext(ctx, backoff, func(ctx context.Context) (done bool, err error) { req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil) if err != nil { @@ -81,11 +88,15 @@ func (c *helmRepositoryClient) FetchCharts(ctx context.Context, url string, conf resp, err := httpClient.Do(req) if err != nil { + lastErr = err + return false, nil } defer resp.Body.Close() if resp.StatusCode >= 500 { + lastErr = fmt.Errorf("repository %s is unavailable (HTTP %d)", url, resp.StatusCode) + return false, nil } @@ -100,6 +111,14 @@ func (c *helmRepositoryClient) FetchCharts(ctx context.Context, url string, conf return true, nil }) if err != nil { + if lastErr != nil && wait.Interrupted(err) { + // The loop ran out of steps or the context ended with every attempt + // failing retriably, so err is the bare timeout. Report the cause + // instead. A terminal error never reaches here: it stops the loop as + // the callback's own error and stays reachable through errors.As. + return nil, fmt.Errorf("helm repository index.yaml request failed: %w", lastErr) + } + return nil, fmt.Errorf("helm repository index.yaml request failed: %w", err) } 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 3ae579f0..f52d16e5 100644 --- a/images/operator-helm-controller/internal/client/repository/helm_test.go +++ b/images/operator-helm-controller/internal/client/repository/helm_test.go @@ -20,6 +20,7 @@ import ( "context" "net/http" "net/http/httptest" + "strings" "testing" helmv1alpha1 "github.com/deckhouse/operator-helm/api/v1alpha1" @@ -82,6 +83,11 @@ func TestFetchChartsServerErrorIsNotTerminal(t *testing.T) { if _, ok := AsTerminal(err); ok { t.Fatalf("5xx must stay retriable, got terminal error: %v", err) } + // The exhausted backoff must report what actually went wrong rather than the + // bare "timed out waiting for the condition" the wait helper returns. + if !strings.Contains(err.Error(), "500") { + t.Fatalf("expected the status code in the reported cause, got %v", err) + } } func TestFetchChartsSkipsInvalidVersion(t *testing.T) { From 018b555dc47e25ce748cfb90fda18e01fd0a7590 Mon Sep 17 00:00:00 2001 From: Ilya Drey Date: Tue, 1 Sep 2026 22:30:34 +0300 Subject: [PATCH 24/33] fix(core): patch the force annotation only when it is set Signed-off-by: Ilya Drey --- .../reconcile/helmclusteraddonrepository/reconciler.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/images/operator-helm-controller/internal/reconcile/helmclusteraddonrepository/reconciler.go b/images/operator-helm-controller/internal/reconcile/helmclusteraddonrepository/reconciler.go index fe66d55e..5a8574e4 100644 --- a/images/operator-helm-controller/internal/reconcile/helmclusteraddonrepository/reconciler.go +++ b/images/operator-helm-controller/internal/reconcile/helmclusteraddonrepository/reconciler.go @@ -257,7 +257,10 @@ func (r *Reconciler) reconcileForceAnnotation(ctx context.Context, key client.Ob return fmt.Errorf("getting helm cluster addon repository: %w", err) } - if repo.Annotations == nil { + if _, found := repo.Annotations[helmv1alpha1.AnnotationForceReconcile]; !found { + // Guard on the annotation itself, not on the map: a repository carrying + // any unrelated annotation would otherwise take an empty PATCH on every + // attempted pass. return nil } From 9a09f37f630f0352100c5a0aee60cd17820bb2db Mon Sep 17 00:00:00 2001 From: Ilya Drey Date: Tue, 1 Sep 2026 22:30:35 +0300 Subject: [PATCH 25/33] test(core): cover abnormal condition removal through the status patch Signed-off-by: Ilya Drey --- .../reconciler_test.go | 75 +++++++++++++++++-- 1 file changed, 69 insertions(+), 6 deletions(-) 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 4954d8db..bc896aa7 100644 --- a/images/operator-helm-controller/internal/reconcile/helmclusteraddonrepository/reconciler_test.go +++ b/images/operator-helm-controller/internal/reconcile/helmclusteraddonrepository/reconciler_test.go @@ -46,11 +46,13 @@ type stubRepoClient struct { err error } -func (s stubRepoClient) FetchCharts(_ context.Context, _ string, _ *repoclient.RepoConfig) ([]repoclient.Chart, error) { +// 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) { return s.charts, s.err } -func newReconciler(t *testing.T, stub stubRepoClient, objects ...client.Object) (*Reconciler, client.Client) { +func newReconciler(t *testing.T, stub *stubRepoClient, objects ...client.Object) (*Reconciler, client.Client) { t.Helper() scheme := runtime.NewScheme() @@ -116,7 +118,7 @@ func reconcileUntilStable(t *testing.T, r *Reconciler, name string) reconcile.Re func TestReconcileOCIRepositoryBecomesReady(t *testing.T) { repo := ociRepository() - stub := stubRepoClient{charts: []repoclient.Chart{{ + stub := &stubRepoClient{charts: []repoclient.Chart{{ Name: "podinfo", Versions: []repoclient.ChartVersion{{Version: semver.MustParse("6.7.1")}}, }}} @@ -152,7 +154,7 @@ func TestReconcileOCIRepositoryBecomesReady(t *testing.T) { func TestReconcileSkipsFetchBeforeSchedule(t *testing.T) { repo := ociRepository() - stub := stubRepoClient{charts: []repoclient.Chart{{ + stub := &stubRepoClient{charts: []repoclient.Chart{{ Name: "podinfo", Versions: []repoclient.ChartVersion{{Version: semver.MustParse("6.7.1")}}, }}} @@ -185,7 +187,7 @@ func TestReconcileSkipsFetchBeforeSchedule(t *testing.T) { func TestReconcileTerminalFetchFailureStalls(t *testing.T) { repo := ociRepository() - stub := stubRepoClient{err: &repoclient.TerminalError{ + stub := &stubRepoClient{err: &repoclient.TerminalError{ Reason: helmv1alpha1.ReasonAuthenticationFailed, Message: "repository rejected the credentials (HTTP 401)", }} @@ -215,6 +217,67 @@ func TestReconcileTerminalFetchFailureStalls(t *testing.T) { } } +// TestReconcileRemovesStalledOnRecovery pins that an abnormal-true condition is +// removed from the STORED object and not merely from the in-memory status. +// Removal rides on the JSON merge patch client.MergeFrom produces, which +// replaces the whole conditions array; were that ever to stop holding, a +// repository would keep reporting Failed to kstatus forever after one stall. +func TestReconcileRemovesStalledOnRecovery(t *testing.T) { + repo := ociRepository() + stub := &stubRepoClient{err: &repoclient.TerminalError{ + Reason: helmv1alpha1.ReasonSourceNotFound, + Message: "repository not found (HTTP 404)", + }} + + r, c := newReconciler(t, stub, repo) + reconcileUntilStable(t, r, repo.Name) + + stalled := &helmv1alpha1.HelmClusterAddonRepository{} + if err := c.Get(context.Background(), client.ObjectKeyFromObject(repo), stalled); err != nil { + t.Fatalf("getting repository: %v", err) + } + if apimeta.FindStatusCondition(stalled.Status.Conditions, helmv1alpha1.ConditionTypeStalled) == nil { + t.Fatalf("the fixture must reach Stalled first, conditions: %v", stalled.Status.Conditions) + } + + // The source recovers. The force annotation makes the next pass attempt + // regardless of the schedule the stall left behind. + stub.err = nil + stub.charts = []repoclient.Chart{{ + Name: "podinfo", + Versions: []repoclient.ChartVersion{{Version: semver.MustParse("6.7.1")}}, + }} + + stalled.Annotations = map[string]string{helmv1alpha1.AnnotationForceReconcile: ""} + if err := c.Update(context.Background(), stalled); err != nil { + t.Fatalf("annotating repository: %v", err) + } + + if _, err := r.Reconcile(context.Background(), reconcile.Request{ + NamespacedName: types.NamespacedName{Name: repo.Name}, + }); err != nil { + t.Fatalf("Reconcile returned %v", err) + } + + recovered := &helmv1alpha1.HelmClusterAddonRepository{} + if err := c.Get(context.Background(), client.ObjectKeyFromObject(repo), recovered); err != nil { + t.Fatalf("getting repository: %v", err) + } + + if cond := apimeta.FindStatusCondition(recovered.Status.Conditions, helmv1alpha1.ConditionTypeStalled); cond != nil { + t.Fatalf("Stalled must be gone from the stored object, got %+v", cond) + } + if !apimeta.IsStatusConditionTrue(recovered.Status.Conditions, helmv1alpha1.ConditionTypeReady) { + t.Fatalf("Ready must be True after recovery, conditions: %v", recovered.Status.Conditions) + } + if apimeta.FindStatusCondition(recovered.Status.Conditions, helmv1alpha1.ConditionTypeReconciling) != nil { + t.Fatal("Reconciling must be absent on a recovered repository") + } + if _, found := recovered.Annotations[helmv1alpha1.AnnotationForceReconcile]; found { + t.Fatal("the force annotation must be consumed by the pass it triggered") + } +} + // TestReconcileDeleteCleansUpWhenURLNoLongerParses covers a repository whose url // satisfies the CRD's validation regex but is rejected by url.Parse, so the // repository type cannot be determined. Its internal objects were created while @@ -249,7 +312,7 @@ func TestReconcileDeleteCleansUpWhenURLNoLongerParses(t *testing.T) { Namespace: helmv1alpha1.TargetNamespace, }} - r, c := newReconciler(t, stubRepoClient{}, repo, internalRepo, authSecret, tlsSecret) + r, c := newReconciler(t, &stubRepoClient{}, repo, internalRepo, authSecret, tlsSecret) // The first pass deletes the internal objects and waits for the internal // repository to disappear; the second removes the finalizer. From d154340b6dae3517cd455668f984f5d7b8bbbbf8 Mon Sep 17 00:00:00 2001 From: Ilya Drey Date: Tue, 1 Sep 2026 22:30:36 +0300 Subject: [PATCH 26/33] test(core): exclude the deliberate stall from the e2e log watcher Signed-off-by: Ilya Drey --- tests/e2e/default_config.yaml | 3 +++ .../helmclusteraddonrepository/lifecycle.go | 20 +++++++++++++++++++ 2 files changed, 23 insertions(+) diff --git a/tests/e2e/default_config.yaml b/tests/e2e/default_config.yaml index eaefa374..ae4864fe 100644 --- a/tests/e2e/default_config.yaml +++ b/tests/e2e/default_config.yaml @@ -13,6 +13,9 @@ controllers: - "Failed to get internal repository" # Expected in the negative test that sets an invalid chart version; surfaced via status. - "failed to get desired chart version" + # Expected in the stall scenario that points a repository at a missing + # source; the reconciler logs every failed read with the repository name. + - "repo-source-not-found" # Transient controller-runtime cache reflector reconnects (watch/list drop, # unexpected EOF); self-recovering, not a real failure. - "Unexpected error when reading response body" diff --git a/tests/e2e/helmclusteraddonrepository/lifecycle.go b/tests/e2e/helmclusteraddonrepository/lifecycle.go index f8df08c3..d13fde19 100644 --- a/tests/e2e/helmclusteraddonrepository/lifecycle.go +++ b/tests/e2e/helmclusteraddonrepository/lifecycle.go @@ -169,6 +169,10 @@ var _ = Describe("HelmClusterAddonRepository with an unreachable source", Ordere // No AssertNoErrorsFor here on purpose: this scenario breaks the repository // deliberately, so error-level log lines from the controller are expected. + // Dropping the assertion is not enough on its own — the log watcher + // accumulates errors suite-wide and never resets, so every other scenario's + // assertion and the suite-teardown one would fail too. The repository name is + // excluded in default_config.yaml; keep the two in step if it ever changes. It("should stall on a missing source and recover after the url is fixed", func() { repo := &apiv1alpha1.HelmClusterAddonRepository{ @@ -192,6 +196,22 @@ var _ = Describe("HelmClusterAddonRepository with an unreachable source", Ordere created, ) + By("Stalled must name the terminal cause") + util.UntilConditionReason( + apiv1alpha1.ConditionTypeStalled, + apiv1alpha1.ReasonSourceNotFound, + framework.LongTimeout, + created, + ) + + By("Ready must be False while Stalled") + util.UntilConditionStatus( + apiv1alpha1.ConditionTypeReady, + string(metav1.ConditionFalse), + framework.LongTimeout, + created, + ) + By("Reconciling must be absent while Stalled") util.UntilConditionAbsent( apiv1alpha1.ConditionTypeReconciling, From d01594779bd0b92170016db9bd4cb385b51bc71a Mon Sep 17 00:00:00 2001 From: Ilya Drey Date: Wed, 2 Sep 2026 11:40:29 +0300 Subject: [PATCH 27/33] fix(core): pass the repository type into EnsureSecrets The merge that made EnsureSecrets kind-aware staged only internal/services, so this call site was left behind and the merge commit did not compile. Signed-off-by: Ilya Drey --- .../internal/reconcile/helmclusteraddonrepository/reconciler.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/images/operator-helm-controller/internal/reconcile/helmclusteraddonrepository/reconciler.go b/images/operator-helm-controller/internal/reconcile/helmclusteraddonrepository/reconciler.go index 5a8574e4..5b7dfdbe 100644 --- a/images/operator-helm-controller/internal/reconcile/helmclusteraddonrepository/reconciler.go +++ b/images/operator-helm-controller/internal/reconcile/helmclusteraddonrepository/reconciler.go @@ -115,7 +115,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (reco // Both services embed the same BaseRepoService with the same target namespace, // so one of them reconciles the auxiliary secrets for either repository type. - in.SecretsErr = r.helmRepositoryService.EnsureSecrets(ctx, &repo) + in.SecretsErr = r.helmRepositoryService.EnsureSecrets(ctx, &repo, repoType) if in.SecretsErr == nil { switch repoType { From e0049f40e841790d516bb063cf8c4bfc5bf8f666 Mon Sep 17 00:00:00 2001 From: Ilya Drey Date: Wed, 2 Sep 2026 15:55:08 +0300 Subject: [PATCH 28/33] fix(core): render Next Sync as a timestamp instead of an age A "date" print column shows how long ago its value was, and kubectl's HumanDuration returns "" for anything more than a second in the future. nextSyncTime is always in the future by design, so the column read "" for every repository: NAME STATUS SYNCED LAST SYNC AGE NEXT SYNC bitnami True True 4m 5m24s Switched to a string column, which prints the absolute RFC3339 instant. Last Sync stays a date: its value is in the past, where the relative rendering is what you want. Signed-off-by: Ilya Drey --- api/v1alpha1/helm_cluster_addon_repository.go | 4 +++- crds/helmclusteraddonrepositories.yaml | 10 ++++++---- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/api/v1alpha1/helm_cluster_addon_repository.go b/api/v1alpha1/helm_cluster_addon_repository.go index ada390e4..d3867877 100644 --- a/api/v1alpha1/helm_cluster_addon_repository.go +++ b/api/v1alpha1/helm_cluster_addon_repository.go @@ -38,7 +38,9 @@ const ( // +kubebuilder:printcolumn:name="Synced",type="string",JSONPath=".status.conditions[?(@.type=='Synced')].status",description="Repository synchronization status" // +kubebuilder:printcolumn:name="Last Sync",type="date",JSONPath=".status.lastSuccessfulSyncTime",description="Time of the last successful catalog synchronization" // +kubebuilder:printcolumn:name="Age",type="date",JSONPath=".metadata.creationTimestamp" -// +kubebuilder:printcolumn:name="Next Sync",type="date",JSONPath=".status.nextSyncTime",priority=1,description="Scheduled time of the next synchronization attempt" +// Rendered as an absolute timestamp on purpose: a "date" column prints how long +// ago the value was, and kubectl renders any future instant as . +// +kubebuilder:printcolumn:name="Next Sync",type="string",JSONPath=".status.nextSyncTime",priority=1,description="Scheduled time of the next synchronization attempt" // +kubebuilder:printcolumn:name="Message",type="string",JSONPath=".status.conditions[?(@.type=='Ready')].message",priority=1 // +genclient // +genclient:nonNamespaced diff --git a/crds/helmclusteraddonrepositories.yaml b/crds/helmclusteraddonrepositories.yaml index 28cbcad1..b435c5af 100644 --- a/crds/helmclusteraddonrepositories.yaml +++ b/crds/helmclusteraddonrepositories.yaml @@ -37,7 +37,7 @@ spec: jsonPath: .status.nextSyncTime name: Next Sync priority: 1 - type: date + type: string - jsonPath: .status.conditions[?(@.type=='Ready')].message name: Message priority: 1 @@ -45,9 +45,11 @@ spec: name: v1alpha1 schema: openAPIV3Schema: - description: HelmClusterAddonRepository represents a Helm or OCI-compliant - repository containing Helm charts that can be referenced by HelmClusterAddon - resources. + description: |- + HelmClusterAddonRepository represents a Helm or OCI-compliant repository containing Helm charts that can be referenced by HelmClusterAddon resources. + + Rendered as an absolute timestamp on purpose: a "date" column prints how long + ago the value was, and kubectl renders any future instant as . properties: apiVersion: description: |- From a00803ae0548888769a7b47957208a57bc649ee3 Mon Sep 17 00:00:00 2001 From: Ilya Drey Date: Wed, 2 Sep 2026 18:36:52 +0300 Subject: [PATCH 29/33] fix(core): keep the print-column note out of the API description controller-gen folds every non-marker line of a type's doc comment into the resource description, so the note explaining why "Next Sync" is a string column ended up as user-facing documentation on HelmClusterAddonRepository itself, visible in kubectl explain and in the rendered module docs, and out of step with the Russian mirror. Moved the note into its own comment block above the doc comment, separated by a blank line so controller-gen does not pick it up. The generated description is again byte-identical to the one on main; the column stays a string. Signed-off-by: Ilya Drey --- api/v1alpha1/helm_cluster_addon_repository.go | 9 +++++++-- crds/helmclusteraddonrepositories.yaml | 8 +++----- 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/api/v1alpha1/helm_cluster_addon_repository.go b/api/v1alpha1/helm_cluster_addon_repository.go index d3867877..3dd8591c 100644 --- a/api/v1alpha1/helm_cluster_addon_repository.go +++ b/api/v1alpha1/helm_cluster_addon_repository.go @@ -28,6 +28,13 @@ const ( HelmClusterAddonRepositoryLabelSourceName = "helm.deckhouse.io/cluster-addon-repository" ) +// The "Next Sync" print column below is a string, not a date, on purpose: a date +// column prints how long ago its value was, and kubectl renders any instant more +// than a second in the future as . nextSyncTime is always in the future. +// +// This note is deliberately outside the doc comment below — controller-gen folds +// every non-marker line of that block into the resource's API description. + // HelmClusterAddonRepository represents a Helm or OCI-compliant repository containing Helm charts that can be referenced by HelmClusterAddon resources. // // +kubebuilder:object:root=true @@ -38,8 +45,6 @@ const ( // +kubebuilder:printcolumn:name="Synced",type="string",JSONPath=".status.conditions[?(@.type=='Synced')].status",description="Repository synchronization status" // +kubebuilder:printcolumn:name="Last Sync",type="date",JSONPath=".status.lastSuccessfulSyncTime",description="Time of the last successful catalog synchronization" // +kubebuilder:printcolumn:name="Age",type="date",JSONPath=".metadata.creationTimestamp" -// Rendered as an absolute timestamp on purpose: a "date" column prints how long -// ago the value was, and kubectl renders any future instant as . // +kubebuilder:printcolumn:name="Next Sync",type="string",JSONPath=".status.nextSyncTime",priority=1,description="Scheduled time of the next synchronization attempt" // +kubebuilder:printcolumn:name="Message",type="string",JSONPath=".status.conditions[?(@.type=='Ready')].message",priority=1 // +genclient diff --git a/crds/helmclusteraddonrepositories.yaml b/crds/helmclusteraddonrepositories.yaml index b435c5af..5cd62350 100644 --- a/crds/helmclusteraddonrepositories.yaml +++ b/crds/helmclusteraddonrepositories.yaml @@ -45,11 +45,9 @@ spec: name: v1alpha1 schema: openAPIV3Schema: - description: |- - HelmClusterAddonRepository represents a Helm or OCI-compliant repository containing Helm charts that can be referenced by HelmClusterAddon resources. - - Rendered as an absolute timestamp on purpose: a "date" column prints how long - ago the value was, and kubectl renders any future instant as . + description: HelmClusterAddonRepository represents a Helm or OCI-compliant + repository containing Helm charts that can be referenced by HelmClusterAddon + resources. properties: apiVersion: description: |- From 322d863372737c855362eef137f5d62db228588a Mon Sep 17 00:00:00 2001 From: Ilya Drey Date: Wed, 2 Sep 2026 18:43:45 +0300 Subject: [PATCH 30/33] test(core): stop requiring a freshly created webhook and TLS secret UntilModuleEnabled asserted that the ValidatingWebhookConfiguration and the operator-helm-controller-tls secret were both created after the suite enabled the module. That holds only when the module was not already deployed in the cluster: enabling an already-deployed module leaves both objects in place, and the assertion then fails with a bare "false" after the full 600s timeout, which is what SynchronizedBeforeSuite has been failing on. Both freshness assertions are removed. What the block still verifies is the part that describes the module rather than the environment: the webhook exists and has entries, the TLS secret exists and carries ca.crt, and the webhook's caBundle matches that certificate. The tradeoff is deliberate: a stale deployment can now pass this gate, so the suite no longer proves it is exercising a freshly rolled-out module. Signed-off-by: Ilya Drey --- tests/e2e/internal/util/moduleconfig.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tests/e2e/internal/util/moduleconfig.go b/tests/e2e/internal/util/moduleconfig.go index 8693ec4c..b981a13d 100644 --- a/tests/e2e/internal/util/moduleconfig.go +++ b/tests/e2e/internal/util/moduleconfig.go @@ -167,17 +167,19 @@ func UntilModuleEnabled(deployAt metav1.Time, timeout time.Duration) { UntilConditionStatusWithLastTransitionTime("EnabledByModuleManager", string(metav1.ConditionTrue), deployAt, framework.LongTimeout, module) UntilConditionStatusWithLastTransitionTime("IsReady", string(metav1.ConditionTrue), deployAt, framework.MaxTimeout, module) + // The webhook configuration and the TLS secret are checked for existence and + // mutual consistency, not for freshness: whether enabling the module recreated + // them depends on whether the module was already deployed in the cluster, which + // is a property of the environment rather than of the code under test. Eventually(func(g Gomega) { webhook, err := framework.GetClients().KubeClient().AdmissionregistrationV1().ValidatingWebhookConfigurations().Get(context.TODO(), "operator-helm-controller-admission-webhook", metav1.GetOptions{}) g.Expect(err).NotTo(HaveOccurred()) - g.Expect(webhook.CreationTimestamp.After(deployAt.UTC().Add(-1 * time.Second))).To(BeTrue()) g.Expect(webhook.Webhooks).NotTo(BeEmpty()) caBundle := webhook.Webhooks[0].ClientConfig.CABundle secret, err := framework.GetClients().KubeClient().CoreV1().Secrets(moduleNamespace).Get(context.TODO(), "operator-helm-controller-tls", metav1.GetOptions{}) g.Expect(err).NotTo(HaveOccurred()) - g.Expect(secret.CreationTimestamp.After(deployAt.UTC().Add(-1 * time.Second))).To(BeTrue()) caCert, found := secret.Data["ca.crt"] g.Expect(found).To(BeTrue()) From 24d7a6b044ed842f099f35ad641b19a45cd41371 Mon Sep 17 00:00:00 2001 From: Ilya Drey Date: Wed, 2 Sep 2026 19:12:43 +0300 Subject: [PATCH 31/33] test(core): pass the built module digest into the e2e run The build job pushes the module under a mutable tag (pr or the branch name) and prints the resulting digest, but nothing carried that value forward, so the e2e job could only ask for the tag and had no way to tell which artifact the cluster actually pulled. deckhouse/modules-actions/build declares no outputs, so the digest is resolved in a step of its own right after the build, while the setup action's registry login and its crane install are still in scope. From there it travels the same route the tag already takes: step output, job output, then the e2e job's env as E2E_MODULE_DIGEST. The suite compares it against ModulePullOverride's status.imageDigest, which is where Deckhouse records the digest the tag resolved to. The check is skipped when the variable is empty, so a local run without it behaves as before. This also restores, precisely, what the removed webhook and TLS-secret freshness assertions were groping at: proof that the cluster runs the artifact under test rather than whatever an older build left behind the tag. Signed-off-by: Ilya Drey --- .github/workflows/build_dev.yml | 17 +++++++++++++++++ tests/e2e/Taskfile.dist.yaml | 1 + tests/e2e/internal/framework/config.go | 4 ++++ tests/e2e/internal/util/moduleconfig.go | 21 +++++++++++++++++++++ 4 files changed, 43 insertions(+) diff --git a/.github/workflows/build_dev.yml b/.github/workflows/build_dev.yml index 55da21a9..885b095a 100644 --- a/.github/workflows/build_dev.yml +++ b/.github/workflows/build_dev.yml @@ -31,6 +31,7 @@ jobs: name: Build and Push images outputs: modules_module_tag: ${{ steps.modules_module_tag.outputs.MODULES_MODULE_TAG }} + modules_module_digest: ${{ steps.modules_module_digest.outputs.MODULES_MODULE_DIGEST }} steps: - name: Set vars id: modules_module_tag @@ -65,6 +66,21 @@ jobs: module_tag: ${{ steps.modules_module_tag.outputs.MODULES_MODULE_TAG }} svace_enabled: false + # The build action exposes no outputs, so the digest of what it just pushed + # is resolved here and handed to the e2e job, which asserts the cluster is + # running exactly this artifact. + - name: Resolve module digest + id: modules_module_digest + run: | + IMAGE="dev-registry.deckhouse.io/sys/deckhouse-oss/modules/${{ vars.MODULES_MODULE_NAME }}:${{ steps.modules_module_tag.outputs.MODULES_MODULE_TAG }}" + DIGEST="$(crane digest "$IMAGE")" + if [[ ! "$DIGEST" =~ ^sha256:[0-9a-f]{64}$ ]]; then + echo "::error title=Cannot resolve module digest::crane digest $IMAGE returned '$DIGEST'" + exit 1 + fi + echo "$IMAGE -> $DIGEST" + echo "MODULES_MODULE_DIGEST=$DIGEST" >> "$GITHUB_OUTPUT" + show_dev_manifest: runs-on: [self-hosted, large] name: Show manifest @@ -142,6 +158,7 @@ jobs: KIND_CLUSTER_NAME: d8-operator-helm-${{ github.run_number }} DEV_REGISTRY_DOCKER_CONFIG: ${{ secrets.DEV_REGISTRY_DOCKER_CONFIG }} E2E_MODULE_TAG_NAME: ${{ needs.build_dev.outputs.modules_module_tag }} + E2E_MODULE_DIGEST: ${{ needs.build_dev.outputs.modules_module_digest }} E2E_MODULE_SOURCE: operator-helm - name: Delete kind cluster diff --git a/tests/e2e/Taskfile.dist.yaml b/tests/e2e/Taskfile.dist.yaml index af0f7a2b..8c5d9e70 100644 --- a/tests/e2e/Taskfile.dist.yaml +++ b/tests/e2e/Taskfile.dist.yaml @@ -41,5 +41,6 @@ tasks: env: E2E_CLUSTERTRANSPORT_KUBECONFIG: "./kind/{{.KIND_CLUSTER_NAME}}/kubeconfig-external" E2E_MODULE_TAG_NAME: '{{.E2E_MODULE_TAG_NAME | default "main"}}' + E2E_MODULE_DIGEST: "{{.E2E_MODULE_DIGEST}}" E2E_MODULE_SOURCE: '{{.E2E_MODULE_SOURCE | default "operator-helm"}}' DEV_REGISTRY_DOCKER_CONFIG: "{{.DEV_REGISTRY_DOCKER_CONFIG}}" diff --git a/tests/e2e/internal/framework/config.go b/tests/e2e/internal/framework/config.go index 9648ab9d..25fd9cbc 100644 --- a/tests/e2e/internal/framework/config.go +++ b/tests/e2e/internal/framework/config.go @@ -76,6 +76,7 @@ type Config struct { ModuleSource string ModuleSourceDockerCfg string ModuleTagName string + ModuleDigest string } type ControllerConfig struct { @@ -147,6 +148,9 @@ func (c *Config) applyEnvOverrides() { if s, ok := os.LookupEnv("E2E_MODULE_TAG_NAME"); ok { c.ModuleTagName = s } + if s, ok := os.LookupEnv("E2E_MODULE_DIGEST"); ok { + c.ModuleDigest = s + } if s, ok := os.LookupEnv("E2E_MODULE_SOURCE"); ok { c.ModuleSource = s } else { diff --git a/tests/e2e/internal/util/moduleconfig.go b/tests/e2e/internal/util/moduleconfig.go index b981a13d..adf1e0e2 100644 --- a/tests/e2e/internal/util/moduleconfig.go +++ b/tests/e2e/internal/util/moduleconfig.go @@ -109,6 +109,27 @@ func EnsureModuleConfig(f *framework.Framework) { g.Expect(f.EnsureDynamicWithoutCleanup(context.Background(), modulePullOverrideGVR, "", mpo, true)).To(Succeed()) }).WithTimeout(framework.LongTimeout).WithPolling(framework.PollingInterval).Should(Succeed()) + // When the caller knows which artifact it wants tested — in CI the digest the + // build job just pushed — verify the cluster resolved the tag to exactly that. + // A tag is mutable, so without this the suite can silently exercise an older + // build that happens to still sit behind it. + if c.ModuleDigest != "" { + By("Verifying the module pull override resolved to digest " + c.ModuleDigest) + + Eventually(func(g Gomega) { + override, err := framework.GetClients().DynamicClient(). + Resource(modulePullOverrideGVR). + Get(context.TODO(), moduleName, metav1.GetOptions{}) + g.Expect(err).NotTo(HaveOccurred()) + + digest, found, err := unstructured.NestedString(override.Object, "status", "imageDigest") + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(found).To(BeTrue(), "module pull override has no status.imageDigest yet") + g.Expect(digest).To(Equal(c.ModuleDigest), + "cluster is running module digest %s, expected %s", digest, c.ModuleDigest) + }).WithTimeout(framework.LongTimeout).WithPolling(framework.PollingInterval).Should(Succeed()) + } + mc := &unstructured.Unstructured{ Object: map[string]interface{}{ "apiVersion": "deckhouse.io/v1alpha1", From 81be56cdc0544904830cf8371312df69d6ec5092 Mon Sep 17 00:00:00 2001 From: Ilya Drey Date: Wed, 2 Sep 2026 19:23:20 +0300 Subject: [PATCH 32/33] test(core): require the module digest instead of skipping the check The digest comparison was guarded by a non-empty check, so a run without E2E_MODULE_DIGEST skipped it silently. A skipped verification reads exactly like a passing one in the output, which is how a suite ends up exercising whatever an older build left behind a mutable tag while still looking green. The digest is now mandatory. The assertion sits at the top of EnsureModuleConfig, before anything is created in the cluster, so a missing value fails in milliseconds rather than after a 600s timeout, and the message names the variable and the crane invocation that produces it. Signed-off-by: Ilya Drey --- tests/e2e/internal/util/moduleconfig.go | 46 ++++++++++++++----------- 1 file changed, 26 insertions(+), 20 deletions(-) diff --git a/tests/e2e/internal/util/moduleconfig.go b/tests/e2e/internal/util/moduleconfig.go index adf1e0e2..88975d23 100644 --- a/tests/e2e/internal/util/moduleconfig.go +++ b/tests/e2e/internal/util/moduleconfig.go @@ -66,6 +66,15 @@ func EnsureModuleConfig(f *framework.Framework) { c := framework.GetConfig() + // An unknown digest fails the run instead of skipping the check. A skipped + // verification is indistinguishable from a passing one in the output, which is + // exactly how a suite ends up silently exercising whatever an older build left + // behind a mutable tag. + Expect(c.ModuleDigest).NotTo(BeEmpty(), + "E2E_MODULE_DIGEST is not set, so the suite cannot tell which module artifact the cluster will pull. "+ + "In CI the build job supplies it; locally resolve it with "+ + "crane digest dev-registry.deckhouse.io/sys/deckhouse-oss/modules/operator-helm:$E2E_MODULE_TAG_NAME") + moduleSource := &unstructured.Unstructured{ Object: map[string]interface{}{ "apiVersion": "deckhouse.io/v1alpha1", @@ -109,26 +118,23 @@ func EnsureModuleConfig(f *framework.Framework) { g.Expect(f.EnsureDynamicWithoutCleanup(context.Background(), modulePullOverrideGVR, "", mpo, true)).To(Succeed()) }).WithTimeout(framework.LongTimeout).WithPolling(framework.PollingInterval).Should(Succeed()) - // When the caller knows which artifact it wants tested — in CI the digest the - // build job just pushed — verify the cluster resolved the tag to exactly that. - // A tag is mutable, so without this the suite can silently exercise an older - // build that happens to still sit behind it. - if c.ModuleDigest != "" { - By("Verifying the module pull override resolved to digest " + c.ModuleDigest) - - Eventually(func(g Gomega) { - override, err := framework.GetClients().DynamicClient(). - Resource(modulePullOverrideGVR). - Get(context.TODO(), moduleName, metav1.GetOptions{}) - g.Expect(err).NotTo(HaveOccurred()) - - digest, found, err := unstructured.NestedString(override.Object, "status", "imageDigest") - g.Expect(err).NotTo(HaveOccurred()) - g.Expect(found).To(BeTrue(), "module pull override has no status.imageDigest yet") - g.Expect(digest).To(Equal(c.ModuleDigest), - "cluster is running module digest %s, expected %s", digest, c.ModuleDigest) - }).WithTimeout(framework.LongTimeout).WithPolling(framework.PollingInterval).Should(Succeed()) - } + // Verify the cluster resolved the tag to the artifact under test. The tag is + // mutable, so without this the suite can exercise an older build that happens + // to still sit behind it. + By("Verifying the module pull override resolved to digest " + c.ModuleDigest) + + Eventually(func(g Gomega) { + override, err := framework.GetClients().DynamicClient(). + Resource(modulePullOverrideGVR). + Get(context.TODO(), moduleName, metav1.GetOptions{}) + g.Expect(err).NotTo(HaveOccurred()) + + digest, found, err := unstructured.NestedString(override.Object, "status", "imageDigest") + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(found).To(BeTrue(), "module pull override has no status.imageDigest yet") + g.Expect(digest).To(Equal(c.ModuleDigest), + "cluster is running module digest %s, expected %s", digest, c.ModuleDigest) + }).WithTimeout(framework.LongTimeout).WithPolling(framework.PollingInterval).Should(Succeed()) mc := &unstructured.Unstructured{ Object: map[string]interface{}{ From 64bfab12cf6a682f422facd00ed3f2f78817fa70 Mon Sep 17 00:00:00 2001 From: Ilya Drey Date: Wed, 2 Sep 2026 19:41:08 +0300 Subject: [PATCH 33/33] test(core): check the module digest after the module is enabled The comparison sat between creating the ModulePullOverride and creating the ModuleConfig, so it ran before the module was enabled. Deckhouse records which artifact the tag resolved to when it actually pulls the module, and it pulls it once the module is enabled, so status.imageDigest was still an empty string and the check timed out after the full 600s with an empty actual value. Moved to the end of UntilModuleEnabled, after the module reports Ready. The mandatory non-empty assertion on E2E_MODULE_DIGEST stays where it was, at the top of EnsureModuleConfig, so a missing value still fails in milliseconds. Signed-off-by: Ilya Drey --- tests/e2e/internal/util/moduleconfig.go | 51 ++++++++++++------------- tests/e2e/internal/util/resource.go | 3 -- 2 files changed, 25 insertions(+), 29 deletions(-) diff --git a/tests/e2e/internal/util/moduleconfig.go b/tests/e2e/internal/util/moduleconfig.go index 88975d23..dfab6515 100644 --- a/tests/e2e/internal/util/moduleconfig.go +++ b/tests/e2e/internal/util/moduleconfig.go @@ -114,28 +114,6 @@ func EnsureModuleConfig(f *framework.Framework) { }, } - Eventually(func(g Gomega) { - g.Expect(f.EnsureDynamicWithoutCleanup(context.Background(), modulePullOverrideGVR, "", mpo, true)).To(Succeed()) - }).WithTimeout(framework.LongTimeout).WithPolling(framework.PollingInterval).Should(Succeed()) - - // Verify the cluster resolved the tag to the artifact under test. The tag is - // mutable, so without this the suite can exercise an older build that happens - // to still sit behind it. - By("Verifying the module pull override resolved to digest " + c.ModuleDigest) - - Eventually(func(g Gomega) { - override, err := framework.GetClients().DynamicClient(). - Resource(modulePullOverrideGVR). - Get(context.TODO(), moduleName, metav1.GetOptions{}) - g.Expect(err).NotTo(HaveOccurred()) - - digest, found, err := unstructured.NestedString(override.Object, "status", "imageDigest") - g.Expect(err).NotTo(HaveOccurred()) - g.Expect(found).To(BeTrue(), "module pull override has no status.imageDigest yet") - g.Expect(digest).To(Equal(c.ModuleDigest), - "cluster is running module digest %s, expected %s", digest, c.ModuleDigest) - }).WithTimeout(framework.LongTimeout).WithPolling(framework.PollingInterval).Should(Succeed()) - mc := &unstructured.Unstructured{ Object: map[string]interface{}{ "apiVersion": "deckhouse.io/v1alpha1", @@ -153,6 +131,10 @@ func EnsureModuleConfig(f *framework.Framework) { Eventually(func(g Gomega) { g.Expect(f.EnsureDynamicWithoutCleanup(context.Background(), moduleConfigGVR, "", mc, true)).To(Succeed()) }).WithTimeout(framework.LongTimeout).WithPolling(framework.PollingInterval).Should(Succeed()) + + Eventually(func(g Gomega) { + g.Expect(f.EnsureDynamicWithoutCleanup(context.Background(), modulePullOverrideGVR, "", mpo, true)).To(Succeed()) + }).WithTimeout(framework.LongTimeout).WithPolling(framework.PollingInterval).Should(Succeed()) } func DisableModuleConfig(timeout time.Duration) { @@ -194,10 +176,27 @@ func UntilModuleEnabled(deployAt metav1.Time, timeout time.Duration) { UntilConditionStatusWithLastTransitionTime("EnabledByModuleManager", string(metav1.ConditionTrue), deployAt, framework.LongTimeout, module) UntilConditionStatusWithLastTransitionTime("IsReady", string(metav1.ConditionTrue), deployAt, framework.MaxTimeout, module) - // The webhook configuration and the TLS secret are checked for existence and - // mutual consistency, not for freshness: whether enabling the module recreated - // them depends on whether the module was already deployed in the cluster, which - // is a property of the environment rather than of the code under test. + // Only now is the digest meaningful: Deckhouse records which artifact the tag + // resolved to when it actually pulls the module, and it pulls it once the + // module is enabled. Checked here rather than right after the pull override is + // created, where status.imageDigest is still empty. + digestCfg := framework.GetConfig() + + By("Verifying the module pull override resolved to digest " + digestCfg.ModuleDigest) + + Eventually(func(g Gomega) { + override, err := framework.GetClients().DynamicClient(). + Resource(modulePullOverrideGVR). + Get(context.TODO(), moduleName, metav1.GetOptions{}) + g.Expect(err).NotTo(HaveOccurred()) + + digest, found, err := unstructured.NestedString(override.Object, "status", "imageDigest") + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(found).To(BeTrue(), "module pull override has no status.imageDigest yet") + g.Expect(digest).To(Equal(digestCfg.ModuleDigest), + "cluster is running module digest %q, expected %q", digest, digestCfg.ModuleDigest) + }).WithTimeout(framework.LongTimeout).WithPolling(framework.PollingInterval).Should(Succeed()) + Eventually(func(g Gomega) { webhook, err := framework.GetClients().KubeClient().AdmissionregistrationV1().ValidatingWebhookConfigurations().Get(context.TODO(), "operator-helm-controller-admission-webhook", metav1.GetOptions{}) g.Expect(err).NotTo(HaveOccurred()) diff --git a/tests/e2e/internal/util/resource.go b/tests/e2e/internal/util/resource.go index 2b0e4beb..cfe04860 100644 --- a/tests/e2e/internal/util/resource.go +++ b/tests/e2e/internal/util/resource.go @@ -175,9 +175,6 @@ func UntilConditionReason(conditionType, expectedReason string, timeout time.Dur }).WithTimeout(timeout).WithPolling(framework.PollingInterval).Should(Succeed()) } -// UntilConditionAbsent waits until the specified condition is not present on the -// objects. Abnormal-true conditions are removed rather than set to False, so -// absence is the assertion that matters. func UntilConditionAbsent(conditionType string, timeout time.Duration, objs ...client.Object) { GinkgoHelper()