From 9146bf6d605c0e17f5dcb0140d4dd99363bde46b Mon Sep 17 00:00:00 2001 From: "Sergio C. Arteaga" Date: Wed, 5 May 2021 12:59:20 +0200 Subject: [PATCH] Improve OLM and Tekton trackers errors handling (#1290) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Sergio Castaño Arteaga --- .golangci.yml | 2 - internal/tracker/source/olm/olm.go | 106 ++++++++++++------ internal/tracker/source/olm/olm_test.go | 24 +++- ...operator.v0.1.0.clusterserviceversion.yaml | 22 +--- ...operator.v0.1.0.clusterserviceversion.yaml | 64 +++++++++++ .../test-operator/test-operator.package.yaml | 5 + internal/tracker/source/tekton/tekton.go | 5 +- 7 files changed, 165 insertions(+), 63 deletions(-) create mode 100644 internal/tracker/source/olm/testdata/path4/test-operator/0.1.0/test-operator.v0.1.0.clusterserviceversion.yaml create mode 100644 internal/tracker/source/olm/testdata/path4/test-operator/test-operator.package.yaml diff --git a/.golangci.yml b/.golangci.yml index a22dd83a..6fd2202d 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -15,11 +15,9 @@ linters: - gosimple - govet - ineffassign - - interfacer - misspell - nakedret - prealloc - - scopelint - staticcheck - structcheck - stylecheck diff --git a/internal/tracker/source/olm/olm.go b/internal/tracker/source/olm/olm.go index 68e29a33..cb8a492d 100644 --- a/internal/tracker/source/olm/olm.go +++ b/internal/tracker/source/olm/olm.go @@ -115,7 +115,11 @@ func (s *TrackerSource) GetPackagesAvailable() (map[string]*hub.Package, error) } // Prepare and store package version - p := s.preparePackage(s.i.Repository, manifest, csv, csvData) + p, err := s.preparePackage(s.i.Repository, manifest, csv, csvData) + if err != nil { + s.warn(fmt.Errorf("error preparing package %s version %s: %w", pkgName, version, err)) + continue + } packagesAvailable[pkg.BuildKey(p)] = p } @@ -134,25 +138,31 @@ func (s *TrackerSource) preparePackage( manifest *manifests.PackageManifest, csv *operatorsv1alpha1.ClusterServiceVersion, csvData []byte, -) *hub.Package { +) (*hub.Package, error) { // Prepare package from manifest and csv p := &hub.Package{ - Name: manifest.PackageName, - DisplayName: csv.Spec.DisplayName, - Description: csv.Annotations["description"], - Keywords: csv.Spec.Keywords, - Readme: csv.Spec.Description, - Version: csv.Spec.Version.String(), - IsOperator: true, - Capabilities: csv.Annotations["capabilities"], - DefaultChannel: manifest.DefaultChannelName, - License: csv.Annotations[licenseAnnotation], - Provider: csv.Spec.Provider.Name, - ContainersImages: getContainersImages(csv, csvData), - Install: csv.Annotations[installAnnotation], - Repository: r, + Name: manifest.PackageName, + DisplayName: csv.Spec.DisplayName, + Description: csv.Annotations["description"], + Keywords: csv.Spec.Keywords, + Readme: csv.Spec.Description, + Version: csv.Spec.Version.String(), + IsOperator: true, + Capabilities: csv.Annotations["capabilities"], + DefaultChannel: manifest.DefaultChannelName, + License: csv.Annotations[licenseAnnotation], + Provider: csv.Spec.Provider.Name, + Install: csv.Annotations[installAnnotation], + Repository: r, } + // Containers images + containersImages, err := getContainersImages(csv, csvData) + if err != nil { + return nil, err + } + p.ContainersImages = containersImages + // TS ts, err := time.Parse(time.RFC3339, csv.Annotations["createdAt"]) if err == nil { @@ -241,38 +251,54 @@ func (s *TrackerSource) preparePackage( p.CRDsExamples = crdsExamples } + // Changes + if v, ok := csv.Annotations[changesAnnotation]; ok { + changes, err := source.ParseChangesAnnotation(v) + if err != nil { + return nil, err + } + p.Changes = changes + } + + // Prerelease + if v, ok := csv.Annotations[prereleaseAnnotation]; ok { + prerelease, err := strconv.ParseBool(v) + if err != nil { + return nil, fmt.Errorf("invalid prerelease value: %s", v) + } + p.Prerelease = prerelease + } + // Recommendations if v, ok := csv.Annotations[recommendationsAnnotation]; ok { var recommendations []*hub.Recommendation - if err := yaml.Unmarshal([]byte(v), &recommendations); err == nil { - p.Recommendations = recommendations + if err := yaml.Unmarshal([]byte(v), &recommendations); err != nil { + return nil, fmt.Errorf("invalid recommendations value: %s", v) } + p.Recommendations = recommendations } - // Misc + // Security updates + if v, ok := csv.Annotations[securityUpdatesAnnotation]; ok { + containsSecurityUpdates, err := strconv.ParseBool(v) + if err != nil { + return nil, fmt.Errorf("invalid containsSecurityUpdates value: %s", v) + } + p.ContainsSecurityUpdates = containsSecurityUpdates + } + + // Prepare data specific to the package kind var isGlobalOperator bool for _, e := range csv.Spec.InstallModes { if e.Type == operatorsv1alpha1.InstallModeTypeAllNamespaces && e.Supported { isGlobalOperator = true } } - changes, err := source.ParseChangesAnnotation(csv.Annotations[changesAnnotation]) - if err == nil { - p.Changes = changes - } - containsSecurityUpdates, err := strconv.ParseBool(csv.Annotations[securityUpdatesAnnotation]) - if err == nil { - p.ContainsSecurityUpdates = containsSecurityUpdates - } - prerelease, err := strconv.ParseBool(csv.Annotations[prereleaseAnnotation]) - if err == nil { - p.Prerelease = prerelease - } p.Data = map[string]interface{}{ "isGlobalOperator": isGlobalOperator, } - return p + return p, nil } // warn is a helper that sends the error provided to the errors collector and @@ -335,7 +361,10 @@ func getCSV(path string) (*operatorsv1alpha1.ClusterServiceVersion, []byte, erro // getContainersImages returns all containers images declared in the csv data // provided. -func getContainersImages(csv *operatorsv1alpha1.ClusterServiceVersion, csvData []byte) []*hub.ContainerImage { +func getContainersImages( + csv *operatorsv1alpha1.ClusterServiceVersion, + csvData []byte, +) ([]*hub.ContainerImage, error) { var images []*hub.ContainerImage // Container image annotation @@ -354,8 +383,13 @@ func getContainersImages(csv *operatorsv1alpha1.ClusterServiceVersion, csvData [ if err := yaml.Unmarshal(csvData, &csvRI); err == nil { images = append(images, csvRI.Spec.RelatedImages...) } - var imagesWhitelist []string - if err := yaml.Unmarshal([]byte(csv.Annotations[imagesWhitelistAnnotation]), &imagesWhitelist); err == nil { + + // Images whitelisting + if v, ok := csv.Annotations[imagesWhitelistAnnotation]; ok { + var imagesWhitelist []string + if err := yaml.Unmarshal([]byte(v), &imagesWhitelist); err != nil { + return nil, fmt.Errorf("invalid imagesWhitelist value: %s", v) + } for _, image := range images { if contains(imagesWhitelist, image.Image) { image.Whitelisted = true @@ -363,7 +397,7 @@ func getContainersImages(csv *operatorsv1alpha1.ClusterServiceVersion, csvData [ } } - return images + return images, nil } // contains is a helper to check if a list contains the string provided. diff --git a/internal/tracker/source/olm/olm_test.go b/internal/tracker/source/olm/olm_test.go index 049515c2..c4147980 100644 --- a/internal/tracker/source/olm/olm_test.go +++ b/internal/tracker/source/olm/olm_test.go @@ -136,7 +136,7 @@ func TestTrackerSource(t *testing.T) { sw.AssertExpectations(t) }) - t.Run("error saving logo image, package returned anyway", func(t *testing.T) { + t.Run("invalid changes annotation", func(t *testing.T) { t.Parallel() // Setup services and expectations @@ -146,6 +146,26 @@ func TestTrackerSource(t *testing.T) { BasePath: "testdata/path3", Svc: sw.Svc, } + expectedErr := "error preparing package test-operator version 0.1.0: invalid changes annotation: single string" + sw.Ec.On("Append", i.Repository.RepositoryID, expectedErr).Return() + + // Run test and check expectations + packages, err := NewTrackerSource(i).GetPackagesAvailable() + assert.Equal(t, map[string]*hub.Package{}, packages) + assert.NoError(t, err) + sw.AssertExpectations(t) + }) + + t.Run("error saving logo image, package returned anyway", func(t *testing.T) { + t.Parallel() + + // Setup services and expectations + sw := source.NewTestsServicesWrapper() + i := &hub.TrackerSourceInput{ + Repository: &hub.Repository{}, + BasePath: "testdata/path4", + Svc: sw.Svc, + } sw.Is.On("SaveImage", sw.Svc.Ctx, imageData).Return("", tests.ErrFake) expectedErr := "error saving package test-operator image: fake error for tests" sw.Ec.On("Append", i.Repository.RepositoryID, expectedErr).Return() @@ -168,7 +188,7 @@ func TestTrackerSource(t *testing.T) { sw := source.NewTestsServicesWrapper() i := &hub.TrackerSourceInput{ Repository: &hub.Repository{}, - BasePath: "testdata/path3", + BasePath: "testdata/path4", Svc: sw.Svc, } sw.Is.On("SaveImage", sw.Svc.Ctx, imageData).Return("logoImageID", nil) diff --git a/internal/tracker/source/olm/testdata/path3/test-operator/0.1.0/test-operator.v0.1.0.clusterserviceversion.yaml b/internal/tracker/source/olm/testdata/path3/test-operator/0.1.0/test-operator.v0.1.0.clusterserviceversion.yaml index e25af03c..53061513 100644 --- a/internal/tracker/source/olm/testdata/path3/test-operator/0.1.0/test-operator.v0.1.0.clusterserviceversion.yaml +++ b/internal/tracker/source/olm/testdata/path3/test-operator/0.1.0/test-operator.v0.1.0.clusterserviceversion.yaml @@ -2,25 +2,7 @@ apiVersion: operators.coreos.com/v1alpha1 kind: ClusterServiceVersion metadata: annotations: - artifacthub.io/changes: | - - feature 1 - - fix 1 - artifacthub.io/containsSecurityUpdates: "true" - artifacthub.io/imagesWhitelist: | - - registry.io/image2:1.0.0 - artifacthub.io/install: | - Install instructions (markdown) - artifacthub.io/license: Apache-2.0 - artifacthub.io/prerelease: "true" - artifacthub.io/recommendations: | - - url: https://artifacthub.io/packages/helm/artifact-hub/artifact-hub - capabilities: Basic Install - categories: Application Runtime - containerImage: repo.url:latest - createdAt: "2019-06-28T15:23:00Z" - description: This is just a test - repository: https://github.com/test/test-operator - alm-examples: '[{"apiVersion": "crds.com/v1", "kind": "Test"}]' + artifacthub.io/changes: single string name: test-operator.v0.1.0 namespace: placeholder spec: @@ -33,8 +15,6 @@ spec: displayName: Test description: Test Operator README displayName: Test Operator - icon: - - base64data: iVBORw0KGgoAAAANSUhEUgAAAAUAAAAFCAYAAACNbyblAAAAHElEQVQI12P4//8/w38GIAXDIBKE0DHxgljNBAAO9TXL0Y4OHwAAAABJRU5ErkJggg== installModes: - supported: true type: OwnNamespace diff --git a/internal/tracker/source/olm/testdata/path4/test-operator/0.1.0/test-operator.v0.1.0.clusterserviceversion.yaml b/internal/tracker/source/olm/testdata/path4/test-operator/0.1.0/test-operator.v0.1.0.clusterserviceversion.yaml new file mode 100644 index 00000000..e25af03c --- /dev/null +++ b/internal/tracker/source/olm/testdata/path4/test-operator/0.1.0/test-operator.v0.1.0.clusterserviceversion.yaml @@ -0,0 +1,64 @@ +apiVersion: operators.coreos.com/v1alpha1 +kind: ClusterServiceVersion +metadata: + annotations: + artifacthub.io/changes: | + - feature 1 + - fix 1 + artifacthub.io/containsSecurityUpdates: "true" + artifacthub.io/imagesWhitelist: | + - registry.io/image2:1.0.0 + artifacthub.io/install: | + Install instructions (markdown) + artifacthub.io/license: Apache-2.0 + artifacthub.io/prerelease: "true" + artifacthub.io/recommendations: | + - url: https://artifacthub.io/packages/helm/artifact-hub/artifact-hub + capabilities: Basic Install + categories: Application Runtime + containerImage: repo.url:latest + createdAt: "2019-06-28T15:23:00Z" + description: This is just a test + repository: https://github.com/test/test-operator + alm-examples: '[{"apiVersion": "crds.com/v1", "kind": "Test"}]' + name: test-operator.v0.1.0 + namespace: placeholder +spec: + customresourcedefinitions: + owned: + - description: Test CRD + kind: Test + name: test.crds.com + version: v1 + displayName: Test + description: Test Operator README + displayName: Test Operator + icon: + - base64data: iVBORw0KGgoAAAANSUhEUgAAAAUAAAAFCAYAAACNbyblAAAAHElEQVQI12P4//8/w38GIAXDIBKE0DHxgljNBAAO9TXL0Y4OHwAAAABJRU5ErkJggg== + installModes: + - supported: true + type: OwnNamespace + - supported: true + type: SingleNamespace + - supported: false + type: MultiNamespace + - supported: true + type: AllNamespaces + keywords: + - Test + links: + - name: Sample link + url: https://sample.link + maintainers: + - email: test@email.com + name: Test + provider: + name: Test + version: 0.1.0 + install: + strategy: deployment + relatedImages: + - name: image1 + image: registry.io/image1:1.0.0 + - name: image2 + image: registry.io/image2:1.0.0 diff --git a/internal/tracker/source/olm/testdata/path4/test-operator/test-operator.package.yaml b/internal/tracker/source/olm/testdata/path4/test-operator/test-operator.package.yaml new file mode 100644 index 00000000..ee093e52 --- /dev/null +++ b/internal/tracker/source/olm/testdata/path4/test-operator/test-operator.package.yaml @@ -0,0 +1,5 @@ +channels: + - currentCSV: test-operator.v0.1.0 + name: alpha +defaultChannel: alpha +packageName: test-operator diff --git a/internal/tracker/source/tekton/tekton.go b/internal/tracker/source/tekton/tekton.go index ab055414..15487ddd 100644 --- a/internal/tracker/source/tekton/tekton.go +++ b/internal/tracker/source/tekton/tekton.go @@ -214,9 +214,10 @@ func enrichPackageFromAnnotations(p *hub.Package, annotations map[string]string) // Changes if v, ok := annotations[changesAnnotation]; ok { changes, err := source.ParseChangesAnnotation(v) - if err == nil { - p.Changes = changes + if err != nil { + return err } + p.Changes = changes } // License