diff --git a/frontend/packages/helm-plugin/locales/en/helm-plugin.json b/frontend/packages/helm-plugin/locales/en/helm-plugin.json index 7c52d917308..fbebb1f70f2 100644 --- a/frontend/packages/helm-plugin/locales/en/helm-plugin.json +++ b/frontend/packages/helm-plugin/locales/en/helm-plugin.json @@ -62,6 +62,7 @@ "Helm Chart": "Helm Chart", "Helm Chart cannot be installed": "Helm Chart cannot be installed", "Helm Chart repositories": "Helm Chart repositories", + "Helm Chart repository error": "Helm Chart repository error", "Helm Chart repository URL.": "Helm Chart repository URL.", "Helm chart URL": "Helm chart URL", "Helm Charts": "Helm Charts", @@ -138,6 +139,7 @@ "Source": "Source", "Status": "Status", "Status Box": "Status Box", + "The following repositories are unreachable: {{repoList}}": "The following repositories are unreachable: {{repoList}}", "The Helm Chart is currently unavailable. {{chartError}}": "The Helm Chart is currently unavailable. {{chartError}}", "The Helm Release can be created by completing the form. Default values may be provided by the Helm chart authors.": "The Helm Release can be created by completing the form. Default values may be provided by the Helm chart authors.", "The Helm Release can be created by manually entering YAML or JSON definitions.": "The Helm Release can be created by manually entering YAML or JSON definitions.", diff --git a/frontend/packages/helm-plugin/src/catalog/providers/useHelmCharts.tsx b/frontend/packages/helm-plugin/src/catalog/providers/useHelmCharts.tsx index 9aad63566dc..80c5aac924a 100644 --- a/frontend/packages/helm-plugin/src/catalog/providers/useHelmCharts.tsx +++ b/frontend/packages/helm-plugin/src/catalog/providers/useHelmCharts.tsx @@ -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'; @@ -19,8 +21,10 @@ const useHelmCharts: ExtensionHook = ({ namespace, }): [CatalogItem[], boolean, any] => { const { t } = useTranslation('helm-plugin'); + const toast = useToast(); const [helmCharts, setHelmCharts] = useState(); const [loadedError, setLoadedError] = useState(); + const shownWarningRef = useRef(); const resourceSelector = useMemo( () => ({ @@ -52,8 +56,40 @@ const useHelmCharts: ExtensionHook = ({ .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; + }; 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: false, + }); + } catch { + // ignore malformed annotation + } + } } }) .catch((err) => { @@ -65,7 +101,7 @@ const useHelmCharts: ExtensionHook = ({ return () => { mounted = false; }; - }, [namespaceParam]); + }, [namespaceParam, toast, t]); const normalizedHelmCharts: CatalogItem[] = useMemo( () => diff --git a/pkg/helm/actions/utility.go b/pkg/helm/actions/utility.go index 91df5a82d69..18bb9a25e28 100644 --- a/pkg/helm/actions/utility.go +++ b/pkg/helm/actions/utility.go @@ -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) } diff --git a/pkg/helm/chartproxy/proxy.go b/pkg/helm/chartproxy/proxy.go index 22faf42320f..936e9fa3d2b 100644 --- a/pkg/helm/chartproxy/proxy.go +++ b/pkg/helm/chartproxy/proxy.go @@ -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" @@ -75,12 +76,12 @@ 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 { @@ -88,7 +89,7 @@ func (p *proxy) IndexFile(onlyCompatible bool, namespace string) (*repo.IndexFil 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 } @@ -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 + } } for _, delKey := range delKeys { diff --git a/pkg/helm/chartproxy/repos.go b/pkg/helm/chartproxy/repos.go index 76e1287d9ed..8f5165c881d 100644 --- a/pkg/helm/chartproxy/repos.go +++ b/pkg/helm/chartproxy/repos.go @@ -4,7 +4,6 @@ import ( "context" "crypto/tls" "crypto/x509" - "errors" "fmt" "io" "net/http" @@ -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 @@ -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 { @@ -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 { @@ -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 = "" + } + 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) - 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 { diff --git a/pkg/helm/chartproxy/repos_test.go b/pkg/helm/chartproxy/repos_test.go index 22c61baeddc..baf21878609 100644 --- a/pkg/helm/chartproxy/repos_test.go +++ b/pkg/helm/chartproxy/repos_test.go @@ -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) } @@ -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", @@ -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", @@ -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", @@ -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 { @@ -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) } @@ -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) + } + } }) } } @@ -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) }