Skip to content

OCPBUGS-99434: Show toast notification for invalid Helm Chart repositories #16792

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
martinszuc wants to merge 7 commits into
base: main
Choose a base branch
from
+122 −46
Open

OCPBUGS-99434: Show toast notification for invalid Helm Chart repositories #16792

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
7c439bc
HELM-586: Show toast notification for invalid Helm Chart repositories
martinszuc Jul 14, 2026
4fa0d7b
HELM-586: Send structured JSON in console-warning annotation for i18n…
martinszuc Jul 14, 2026
880ecdb
HELM-586: Address review feedback for chartproxy error handling
martinszuc Jul 21, 2026
f17a539
HELM-586: Update i18n keys for helm-plugin locale
martinszuc Jul 21, 2026
4d910f8
HELM-586: Redact credentials from URL in error log
martinszuc Jul 21, 2026
f5a4f88
HELM-586: Address review feedback for chartproxy error handling
martinszuc Jul 21, 2026
0bcda93
HELM-586: Deduplicate repo warning toast per session using sessionSto…
martinszuc Jul 21, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions frontend/packages/helm-plugin/locales/en/helm-plugin.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@
"Browse for charts that help manage complex installations and upgrades. Cluster administrators can customize the content made available in the catalog.<1></1>": "Browse for charts that help manage complex installations and upgrades. Cluster administrators can customize the content made available in the catalog.<1></1>",
"README": "README",
"README not available": "README not available",
"Helm Chart repository error": "Helm Chart repository error",
"The following repositories are unreachable: {{repoList}}": "The following repositories are unreachable: {{repoList}}",
"This Helm Chart is provided by a trusted partner and has been verified for ease of integration.": "This Helm Chart is provided by a trusted partner and has been verified for ease of integration.",
"Certified": "Certified",
"Latest Chart version": "Latest Chart version",
Expand Down
42 changes: 39 additions & 3 deletions frontend/packages/helm-plugin/src/catalog/providers/useHelmCharts.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
import { useState, useMemo, useEffect } from 'react';
import { useState, useMemo, useEffect, useRef } from 'react';
import { AlertVariant } from '@patternfly/react-core';
import { safeLoad } from 'js-yaml';
import { useTranslation } from 'react-i18next';
import type { ExtensionHook, CatalogItem, WatchK8sResults } from '@console/dynamic-plugin-sdk';
import { useK8sWatchResources } from '@console/internal/components/utils/k8s-watch-hook';
import type { K8sResourceKind } from '@console/internal/module/k8s';
import { referenceForModel } from '@console/internal/module/k8s';
import { useToast } from '@console/shared/src/components/toast/useToast';
import type { APIError } from '@console/shared/src/types/resource';
import { coFetch } from '@console/shared/src/utils/console-fetch';
import { HelmChartRepositoryModel, ProjectHelmChartRepositoryModel } from '../../models/helm';
Expand All @@ -19,8 +21,10 @@ const useHelmCharts: ExtensionHook<CatalogItem[]> = ({
namespace,
}): [CatalogItem[], boolean, any] => {
const { t } = useTranslation('helm-plugin');
const toast = useToast();
const [helmCharts, setHelmCharts] = useState<HelmChartEntries>();
const [loadedError, setLoadedError] = useState<APIError>();
const shownWarningRef = useRef<string>();

const resourceSelector = useMemo(
() => ({
Expand Down Expand Up @@ -52,8 +56,40 @@ const useHelmCharts: ExtensionHook<CatalogItem[]> = ({
.then(async (res) => {
if (mounted) {
const yaml = await res.text();
const json = safeLoad(yaml) as { entries: HelmChartEntries };
const json = safeLoad(yaml) as {
entries: HelmChartEntries;
annotations?: Record<string, string>;
};
setHelmCharts(json.entries);
const warningData = json.annotations?.['console-warning'];
const sessionKey = 'helm-repo-warning-shown';
if (
warningData &&
warningData !== shownWarningRef.current &&
warningData !== sessionStorage.getItem(sessionKey)
) {
shownWarningRef.current = warningData;
try {
sessionStorage.setItem(sessionKey, warningData);
} catch {
// sessionStorage may be unavailable
}
try {
const repos: { name: string; error: string }[] = JSON.parse(warningData);
const repoList = repos.map((r) => `${r.name}: ${r.error}`).join(', ');
toast.addToast({
variant: AlertVariant.danger,
title: t('Helm Chart repository error'),
content: t('The following repositories are unreachable: {{repoList}}', {
repoList,
}),
dismissible: true,
timeout: true,
});
} catch {
// ignore malformed annotation
}
}
}
})
.catch((err) => {
Expand All @@ -65,7 +101,7 @@ const useHelmCharts: ExtensionHook<CatalogItem[]> = ({
return () => {
mounted = false;
};
}, [namespaceParam]);
}, [namespaceParam, toast, t]);

const normalizedHelmCharts: CatalogItem[] = useMemo(
() =>
Expand Down
2 changes: 1 addition & 1 deletion pkg/helm/actions/utility.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ func getChartInfoFromChartUrl(
client dynamic.Interface,
coreClient corev1client.CoreV1Interface,
) (*ChartInfo, error) {
repositories, err := chartproxy.NewRepoGetter(client, coreClient).List(namespace)
repositories, _, err := chartproxy.NewRepoGetter(client, coreClient).List(namespace)
if err != nil {
return nil, fmt.Errorf("error listing repositories: %v", err)
}
Expand Down
20 changes: 13 additions & 7 deletions pkg/helm/chartproxy/proxy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
package chartproxy

import (
"encoding/json"
"slices"
"sort"
"strings"

chartutil "helm.sh/helm/v4/pkg/chart/v2/util"
repo "helm.sh/helm/v4/pkg/repo/v1"
Expand Down Expand Up @@ -75,20 +76,20 @@ func New(k8sConfig RestConfigProvider, kubeVersionGetter version.KubeVersionGett
}

func (p *proxy) IndexFile(onlyCompatible bool, namespace string) (*repo.IndexFile, error) {
helmRepos, err := p.helmRepoGetter.List(namespace)
helmRepos, configErrors, err := p.helmRepoGetter.List(namespace)
if err != nil {
return nil, err
}
annotations := make(map[string]string)
var invalidRepos []string
invalidRepos := slices.Clone(configErrors)
indexFile := repo.NewIndexFile()
var delKeys []string = make([]string, 0, 20)
for _, helmRepo := range helmRepos {
overwrites := helmRepo.OverwrittenRepoName()
if !helmRepo.Disabled {
idxFile, err := helmRepo.IndexFile()
if err != nil {
invalidRepos = append(invalidRepos, helmRepo.Name)
invalidRepos = append(invalidRepos, InvalidRepo{Name: helmRepo.Name, Error: err.Error()})
klog.Errorf("Error retrieving index file for %v: %v", helmRepo, err)
continue
}
Expand Down Expand Up @@ -125,9 +126,14 @@ func (p *proxy) IndexFile(onlyCompatible bool, namespace string) (*repo.IndexFil
}
}
}
if invalidRepos != nil {
annotations[warning] = ErrorMessage + strings.Join(invalidRepos, ", ")
indexFile.Annotations = annotations
if len(invalidRepos) > 0 {
data, err := json.Marshal(invalidRepos)
if err != nil {
klog.Errorf("Error marshalling invalid repos annotation: %v", err)
} else {
annotations[warning] = string(data)
indexFile.Annotations = annotations
}
Comment on lines +130 to +136

@webbnh webbnh Jul 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If json.Marshal() returns an error here, we silently ignore it and move on...which is kind of unsatisfying.

If this is a "never happen" situation, then we should consider panicking if it does. Otherwise, maybe attach the error as an annotation, or log it, or somehow try to alert somebody that things aren't working?

}

for _, delKey := range delKeys {
Expand Down
65 changes: 41 additions & 24 deletions pkg/helm/chartproxy/repos.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import (
"context"
"crypto/tls"
"crypto/x509"
"errors"
"fmt"
"io"
"net/http"
Expand Down Expand Up @@ -41,9 +40,13 @@ var (
const (
configNamespace = "openshift-config"
warning = "console-warning"
ErrorMessage = "The following repositories seem to be invalid or unreachable: "
)

type InvalidRepo struct {
Name string `json:"name"`
Error string `json:"error"`
}

type helmRepo struct {
Name string
Namespace string
Expand Down Expand Up @@ -84,8 +87,12 @@ func (hr helmRepo) IndexFile() (*repo.IndexFile, error) {
if err != nil {
return nil, err
}
defer resp.Body.Close()
if resp.StatusCode == 401 || resp.StatusCode == 403 {
return nil, fmt.Errorf("authentication failed (HTTP %d)", resp.StatusCode)
}
if resp.StatusCode != 200 {
return nil, errors.New(fmt.Sprintf("Response for %v returned %v with status code %v", indexURL, resp, resp.StatusCode))
return nil, fmt.Errorf("failed to fetch index (HTTP %d)", resp.StatusCode)
}
body, err := io.ReadAll(resp.Body)
if err != nil {
Expand All @@ -110,7 +117,7 @@ func (hr helmRepo) IndexFile() (*repo.IndexFile, error) {
}

type HelmRepoGetter interface {
List(namespace string) ([]*helmRepo, error)
List(namespace string) ([]*helmRepo, []InvalidRepo, error)
}

type helmRepoGetter struct {
Expand Down Expand Up @@ -254,40 +261,50 @@ func (b helmRepoGetter) unmarshallConfig(repo unstructured.Unstructured, namespa
return h, nil
}

func (b *helmRepoGetter) List(namespace string) ([]*helmRepo, error) {
func (b *helmRepoGetter) processRepoItems(items []unstructured.Unstructured, namespace string, isClusterScoped bool) ([]*helmRepo, []InvalidRepo) {
var repos []*helmRepo
var configErrors []InvalidRepo
for _, item := range items {
helmConfig, err := b.unmarshallConfig(item, namespace, isClusterScoped)
if err != nil {
repoName, found, nameErr := unstructured.NestedString(item.Object, "metadata", "name")
if nameErr != nil || !found {
repoName = "<unknown>"
}
klog.Errorf("Error unmarshalling repo %v: %v", item, err)
configErrors = append(configErrors, InvalidRepo{Name: repoName, Error: err.Error()})
continue
}
repos = append(repos, helmConfig)
}
return repos, configErrors
}

func (b *helmRepoGetter) List(namespace string) ([]*helmRepo, []InvalidRepo, error) {
var helmRepos []*helmRepo
var configErrors []InvalidRepo

clusterRepos, err := b.Client.Resource(helmChartRepositoryClusterGVK).List(context.TODO(), v1.ListOptions{})
if err != nil {
klog.Errorf("Error listing cluster helm chart repositories: %v \nempty repository list will be used", err)
return helmRepos, nil
}
for _, item := range clusterRepos.Items {
helmConfig, err := b.unmarshallConfig(item, "", true)
if err != nil {
klog.Errorf("Error unmarshalling repo %v: %v", item, err)
continue
}
helmRepos = append(helmRepos, helmConfig)
return helmRepos, configErrors, nil
}
repos, errs := b.processRepoItems(clusterRepos.Items, "", true)
helmRepos = append(helmRepos, repos...)
configErrors = append(configErrors, errs...)

if namespace != "" {
namespaceRepos, err := b.Client.Resource(helmChartRepositoryNamespaceGVK).Namespace(namespace).List(context.TODO(), v1.ListOptions{})
if err != nil {
klog.Errorf("Error listing namespace helm chart repositories: %v \nempty repository list will be used", err)

@webbnh webbnh Jul 22, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It hardly matters, but, assuming that helmRepos contains entries from the Cluster/default namespace, it won't be empty when we return it in this error case.

In general, there's very little value in having the log message "predict" what will happen, because, when the log is read, it will have already happened. And, the developer can look at the code where the message was produced and see what it would have done.

So, the second half of this log message is pointless. If you have to make an edit for some other reason, feel free to remove it (here and at line 289, although that one, while also largely pointless, is at least accurate 🙂).

return helmRepos, nil
}
for _, item := range namespaceRepos.Items {
helmConfig, err := b.unmarshallConfig(item, namespace, false)
if err != nil {
klog.Errorf("Error unmarshalling repo %v: %v", item, err)
continue
}
helmRepos = append(helmRepos, helmConfig)
return helmRepos, configErrors, nil
}
repos, errs := b.processRepoItems(namespaceRepos.Items, namespace, false)
helmRepos = append(helmRepos, repos...)
configErrors = append(configErrors, errs...)
}

return helmRepos, nil
return helmRepos, configErrors, nil
}

func NewRepoGetter(client dynamic.Interface, corev1Client corev1.CoreV1Interface) HelmRepoGetter {
Expand Down
37 changes: 26 additions & 11 deletions pkg/helm/chartproxy/repos_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ func TestHelmRepoGetter_List(t *testing.T) {
})
}
repoGetter := NewRepoGetter(client, nil)
repos, err := repoGetter.List(tt.namespace)
repos, _, err := repoGetter.List(tt.namespace)
if err != nil {
t.Error(err)
}
Expand All @@ -136,11 +136,12 @@ func TestHelmRepoGetter_List(t *testing.T) {

func TestHelmRepoGetter_ListErrors(t *testing.T) {
tests := []struct {
name string
helmCRS []*unstructured.Unstructured
expectedRepoName []string
apiErrors []apiError
namespace string
name string
helmCRS []*unstructured.Unstructured
expectedRepoName []string
expectedConfigErrorRepo []string
apiErrors []apiError
namespace string
}{
{
name: "skip repo that refer non-existent config map",
Expand Down Expand Up @@ -179,7 +180,8 @@ func TestHelmRepoGetter_ListErrors(t *testing.T) {
},
},
},
expectedRepoName: []string{"repo2"},
expectedRepoName: []string{"repo2"},
expectedConfigErrorRepo: []string{"repo1"},
},
{
name: "skip repo that refer config map that cannot be accessed",
Expand Down Expand Up @@ -225,7 +227,8 @@ func TestHelmRepoGetter_ListErrors(t *testing.T) {
msg: "foo",
},
},
expectedRepoName: []string{"repo2"},
expectedRepoName: []string{"repo2"},
expectedConfigErrorRepo: []string{"repo1"},
},
{
name: "skip repo that refer secret that cannot be accessed",
Expand Down Expand Up @@ -271,7 +274,8 @@ func TestHelmRepoGetter_ListErrors(t *testing.T) {
msg: "foo",
},
},
expectedRepoName: []string{"repo2"},
expectedRepoName: []string{"repo2"},
expectedConfigErrorRepo: []string{"repo1"},
},
}
for _, tt := range tests {
Expand All @@ -284,7 +288,7 @@ func TestHelmRepoGetter_ListErrors(t *testing.T) {
})
}
repoGetter := NewRepoGetter(client, coreClient.CoreV1())
repos, err := repoGetter.List(tt.namespace)
repos, configErrors, err := repoGetter.List(tt.namespace)
if err != nil {
t.Error(err)
}
Expand All @@ -296,6 +300,17 @@ func TestHelmRepoGetter_ListErrors(t *testing.T) {
t.Errorf("Expected %v but got %v", repoName, repos[i].Name)
}
}
if len(configErrors) != len(tt.expectedConfigErrorRepo) {
t.Fatalf("Expected %v config errors, but got %v", len(tt.expectedConfigErrorRepo), len(configErrors))
}
for i, expectedName := range tt.expectedConfigErrorRepo {
if configErrors[i].Name != expectedName {
t.Errorf("Expected config error repo %v but got %v", expectedName, configErrors[i].Name)
}
if configErrors[i].Error == "" {
t.Errorf("Expected non-empty error message for repo %v", expectedName)
}
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Comment on lines +303 to +313

@webbnh webbnh Jul 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not to disagree with Coderabbitai or anything, but, if this test fails because the counts don't match, the next question that a developer is going to ask is, "What was the mismatch?", and the code isn't going to offer any help. 🫤

Ideally, you would do a "set difference" operation (or two), expecting there to be no difference, and reporting any "extra" ones, either unexpectedly present or absent. Comparing the counts is a quick and easy way to do the check, but it doesn't reveal any helpful information about the failure, which means that the subsequent diagnosis requires reproducing the problem under the debugger, which is potentially cumbersome.

I'll also note that expecting the config errors to come out in a particular order seems fragile at best. (You've finessed this problem by ensuring that there is only ever one error, but you've set the test up to handle multiple...although you don't actually test that.)

I don't have any good suggestions, but I might be inclined to drop the initial count check and to have a nested loop which searches for each expected error in the actual list (instead of assuming and requiring that it show up at a particular index): that would enable reporting any expected ones which were missing. If you put that into a helper function, you could then call it again with the arguments reversed to report any actual ones which weren't in the expected list. This would yield full information without too much code, but it's probably more trouble than it's worth...especially if you only expect one error. 😉 Alternatively, after ensuring that the expected ones were present, you could then use the count-based check to ensure that there weren't any extras...which would at least give you half the information.

})
}
}
Expand Down Expand Up @@ -499,7 +514,7 @@ func TestHelmRepoGetter_SkipDisabled(t *testing.T) {
client := fake.K8sDynamicClientFromCRs(tt.helmCRS...)
coreClient := k8sfake.NewSimpleClientset()
repoGetter := NewRepoGetter(client, coreClient.CoreV1())
repos, err := repoGetter.List(tt.namespace)
repos, _, err := repoGetter.List(tt.namespace)
if err != nil {
t.Error(err)
}
Expand Down