Merge pull request #6768 from pedjak/bz-1877891

Bug 1877891: Aggregated Helm index contains only entries from healthy repos

go.mod

@@ -28,6 +28,9 @@ require (
 )
 
 replace (
+	// ww/goautoneg repo does not exist on butbucket anymore, hence we
+	// need to use a mirror. It can be removed when we update to newer libary-go
+	bitbucket.org/ww/goautoneg => github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822
 	github.com/Azure/go-autorest/autorest => github.com/Azure/go-autorest/autorest v0.9.0
 	github.com/docker/docker => github.com/moby/moby v0.7.3-0.20190826074503-38ab9da00309
 )

pkg/helm/actions/fake/helmcrconfigs.go

@@ -52,3 +52,14 @@ func K8sDynamicClient(indexFiles ...string) dynamic.Interface {
 
 	return fake.NewSimpleDynamicClient(scheme, objs...)
 }
+
+func K8sDynamicClientFromCRs(crs ...*unstructured.Unstructured) dynamic.Interface {
+	scheme := runtime.NewScheme()
+	var objs []runtime.Object
+
+	for _, cr := range crs {
+		objs = append(objs, cr)
+	}
+
+	return fake.NewSimpleDynamicClient(scheme, objs...)
+}

pkg/helm/chartproxy/proxy.go

@@ -73,7 +73,8 @@ func (p *proxy) IndexFile() (*repo.IndexFile, error) {
 	for _, helmRepo := range helmRepos {
 		idxFile, err := helmRepo.IndexFile()
 		if err != nil {
-			return nil, err
+			plog.Errorf("Error retrieving index file for %v: %v", helmRepo, err)
+			continue
 		}
 		indexFiles = append(indexFiles, idxFile)
 	}

pkg/helm/chartproxy/proxy_test.go

@@ -1,8 +1,11 @@
 package chartproxy
 
 import (
+	"golang.org/x/net/context"
 	"helm.sh/helm/v3/pkg/repo"
 	"io/ioutil"
+	v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"reflect"
 	"testing"
 
@@ -16,6 +19,7 @@ func TestProxy_IndexFile(t *testing.T) {
 		name       string
 		indexFiles []string
 		mergedFile string
+		helmCRS    []*unstructured.Unstructured
 	}{
 		{
 			name:       "returned index file for configured helm repo",
@@ -32,6 +36,28 @@ func TestProxy_IndexFile(t *testing.T) {
 			indexFiles: []string{},
 			mergedFile: "",
 		},
+		{
+			name:       "returned merged index file for all accessible helm repos",
+			indexFiles: []string{"testdata/azureRepoIndex.yaml"},
+			mergedFile: "testdata/azureRepoIndex.yaml",
+			helmCRS: []*unstructured.Unstructured{
+				{
+					Object: map[string]interface{}{
+						"apiVersion": "helm.openshift.io/v1beta1",
+						"kind":       "HelmChartRepository",
+						"metadata": map[string]interface{}{
+							"namespace": "",
+							"name":      "repo1",
+						},
+						"spec": map[string]interface{}{
+							"connectionConfig": map[string]interface{}{
+								"url": "http://foo.com/bar",
+							},
+						},
+					},
+				},
+			},
+		},
 	}
 
 	for _, tt := range tests {
@@ -44,8 +70,15 @@ func TestProxy_IndexFile(t *testing.T) {
 				}
 				indexFileContents = append(indexFileContents, string(content))
 			}
+			dynamicClient := fake.K8sDynamicClient(indexFileContents...)
+			for _, helmcr := range tt.helmCRS {
+				_, err := dynamicClient.Resource(helmChartRepositoryGVK).Create(context.TODO(), helmcr, v1.CreateOptions{})
+				if err != nil {
+					t.Error(err)
+				}
+			}
 			fakeProxyOption := func(p *proxy) error {
-				p.dynamicClient = fake.K8sDynamicClient(indexFileContents...)
+				p.dynamicClient = dynamicClient
 				return nil
 			}
 			p, err := New(func() (r *rest.Config, err error) {

pkg/helm/chartproxy/repos.go

@@ -6,6 +6,7 @@ import (
 	"crypto/x509"
 	"errors"
 	"fmt"
+	"github.com/coreos/pkg/capnslog"
 	"io/ioutil"
 	"net/http"
 	"net/url"
@@ -28,6 +29,7 @@ var (
 		Version:  "v1beta1",
 		Resource: "helmchartrepositories",
 	}
+	plog = capnslog.NewPackageLogger("github.com/openshift/console", "helm/chartproxy")
 )
 
 const (
@@ -75,6 +77,9 @@ func (hr helmRepo) IndexFile() (*repo.IndexFile, error) {
 	if err != nil {
 		return nil, err
 	}
+	if resp.StatusCode != 200 {
+		return nil, errors.New(fmt.Sprintf("Response for %v returned %v with status code %v", indexURL, resp, resp.StatusCode))
+	}
 	body, err := ioutil.ReadAll(resp.Body)
 	if err != nil {
 		return nil, err
@@ -126,7 +131,7 @@ func (b helmRepoGetter) unmarshallConfig(repo unstructured.Unstructured) (*helmR
 	if caReference != "" {
 		configMap, err := b.CoreClient.ConfigMaps(configNamespace).Get(context.TODO(), caReference, v1.GetOptions{})
 		if err != nil {
-			return nil, errors.New(fmt.Sprintf("Failed to GET configmap %s", caReference))
+			return nil, errors.New(fmt.Sprintf("Failed to GET configmap %s, reason %v", caReference, err))
 		}
 		caBundleKey := "ca-bundle.crt"
 		caCert, found := configMap.Data[caBundleKey]
@@ -152,7 +157,7 @@ func (b helmRepoGetter) unmarshallConfig(repo unstructured.Unstructured) (*helmR
 	if tlsReference != "" {
 		secret, err := b.CoreClient.Secrets(configNamespace).Get(context.TODO(), tlsReference, v1.GetOptions{})
 		if err != nil {
-			return nil, errors.New(fmt.Sprintf("Failed to GET secret %s", tlsReference))
+			return nil, errors.New(fmt.Sprintf("Failed to GET secret %s reason %v", tlsReference, err))
 		}
 		tlsCertSecretKey := "tls.crt"
 		tlsCert, ok := secret.Data[tlsCertSecretKey]
@@ -189,12 +194,14 @@ func (b *helmRepoGetter) List() ([]*helmRepo, error) {
 	var helmRepos []*helmRepo
 	repos, err := b.Client.Resource(helmChartRepositoryGVK).List(context.TODO(), v1.ListOptions{})
 	if err != nil {
-		return helmRepos, err
+		plog.Errorf("Error listing helm chart repositories: %v \nempty repository list will be used", err)
+		return helmRepos, nil
 	}
 	for _, item := range repos.Items {
 		helmConfig, err := b.unmarshallConfig(item)
 		if err != nil {
-			return nil, err
+			plog.Errorf("Error unmarshalling repo %v: %v", item, err)
+			continue
 		}
 		helmRepos = append(helmRepos, helmConfig)
 	}