From f718c911ae218fa4362ee60670b6dfc6503a6388 Mon Sep 17 00:00:00 2001 From: Camila Macedo <7708031+camilamacedo86@users.noreply.github.com> Date: Wed, 30 Mar 2022 13:29:38 +0100 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B=20fix=20channel=20naming=20validat?= =?UTF-8?q?ion=20(#231)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: check for warning when the channel naming does not follow the convention * applying nit suggestions * fix-channel-check * adressing nit review --- pkg/manifests/bundleloader.go | 36 ++- pkg/validation/internal/good_practices.go | 51 ++++ .../internal/good_practices_test.go | 55 ++++ pkg/validation/internal/operatorhub.go | 31 -- pkg/validation/internal/operatorhub_test.go | 33 --- .../cache.example.com_memcacheds.yaml | 66 +++++ ...cached-operator.clusterserviceversion.yaml | 268 ++++++++++++++++++ .../metadata/annotations.yaml | 13 + 8 files changed, 486 insertions(+), 67 deletions(-) create mode 100644 pkg/validation/internal/testdata/bundle_with_metadata/manifests/cache.example.com_memcacheds.yaml create mode 100644 pkg/validation/internal/testdata/bundle_with_metadata/manifests/memcached-operator.clusterserviceversion.yaml create mode 100644 pkg/validation/internal/testdata/bundle_with_metadata/metadata/annotations.yaml diff --git a/pkg/manifests/bundleloader.go b/pkg/manifests/bundleloader.go index 6a8d7e4cf..5d5bf2dc1 100644 --- a/pkg/manifests/bundleloader.go +++ b/pkg/manifests/bundleloader.go @@ -19,9 +19,10 @@ import ( // bundleLoader loads a bundle directory from disk type bundleLoader struct { - dir string - bundle *Bundle - foundCSV bool + dir string + bundle *Bundle + foundCSV bool + annotationsFile AnnotationsFile } func NewBundleLoader(dir string) bundleLoader { @@ -37,6 +38,7 @@ func (b *bundleLoader) LoadBundle() error { } errs = append(errs, b.calculateCompressedBundleSize()) + b.addChannelsFromAnnotationsFile() if !b.foundCSV { errs = append(errs, fmt.Errorf("unable to find a csv in bundle directory %s", b.dir)) @@ -47,6 +49,21 @@ func (b *bundleLoader) LoadBundle() error { return utilerrors.NewAggregate(errs) } +// Add values from the annotations when the values are not loaded +func (b *bundleLoader) addChannelsFromAnnotationsFile() { + // Note that they will not get load for Bundle Format directories + // and PackageManifest should not have the annotationsFile. However, + // the following check to ensure that channels and default channels + // are empty before set the annotations is just an extra precaution + channels := strings.Split(b.annotationsFile.Annotations.Channels, ",") + if len(channels) > 0 && len(b.bundle.Channels) == 0 { + b.bundle.Channels = channels + } + if len(b.annotationsFile.Annotations.DefaultChannelName) > 0 && len(b.bundle.DefaultChannel) == 0 { + b.bundle.DefaultChannel = b.annotationsFile.Annotations.DefaultChannelName + } +} + // Compress the bundle to check its size func (b *bundleLoader) calculateCompressedBundleSize() error { if b.bundle == nil { @@ -108,6 +125,19 @@ func (b *bundleLoader) LoadBundleWalkFunc(path string, f os.FileInfo, err error) return nil } + annotationsFile := AnnotationsFile{} + if strings.HasPrefix(f.Name(), "annotations") { + annFile, err := os.ReadFile(path) + if err != nil { + return err + } + if err := yaml.Unmarshal(annFile, &annotationsFile); err == nil { + b.annotationsFile = annotationsFile + } else { + return fmt.Errorf("unable to load the annotations file %s: %s", path, err) + } + } + fileReader, err := os.Open(path) if err != nil { return fmt.Errorf("unable to load file %s: %s", path, err) diff --git a/pkg/validation/internal/good_practices.go b/pkg/validation/internal/good_practices.go index 4d294ea6b..246c4e26f 100644 --- a/pkg/validation/internal/good_practices.go +++ b/pkg/validation/internal/good_practices.go @@ -3,6 +3,7 @@ package internal import ( goerrors "errors" "fmt" + "strings" "github.com/operator-framework/api/pkg/manifests" operatorsv1alpha1 "github.com/operator-framework/api/pkg/operators/v1alpha1" @@ -17,6 +18,9 @@ import ( // This validator will raise an WARNING when: // // - The resources request for CPU and/or Memory are not defined for any of the containers found in the CSV +// +// - The channel names seems are not following the convention https://olm.operatorframework.io/docs/best-practices/channel-naming/ +// var GoodPracticesValidator interfaces.Validator = interfaces.ValidatorFunc(goodPracticesValidator) func goodPracticesValidator(objs ...interface{}) (results []errors.ManifestResult) { @@ -51,6 +55,10 @@ func validateGoodPracticesFrom(bundle *manifests.Bundle) errors.ManifestResult { result.Add(errors.WarnFailedValidation(warn.Error(), bundle.CSV.GetName())) } + channels := append(bundle.Channels, bundle.DefaultChannel) + if warn := validateHubChannels(channels); warn != nil { + result.Add(errors.WarnFailedValidation(warn.Error(), bundle.CSV.GetName())) + } return result } @@ -75,3 +83,46 @@ func validateResourceRequests(csv *operatorsv1alpha1.ClusterServiceVersion) (err } return errs, warns } + +// validateHubChannels will check the channels. The motivation for the following check is to ensure that operators +// authors knows if their operator bundles are or not respecting the Naming Convention Rules. +// However, the operator authors still able to choose the names as please them. +func validateHubChannels(channels []string) error { + const candidate = "candidate" + const stable = "stable" + const fast = "fast" + + channels = getUniqueValues(channels) + var channelsNotFollowingConventional []string + for _, channel := range channels { + if !strings.HasPrefix(channel, candidate) && + !strings.HasPrefix(channel, stable) && + !strings.HasPrefix(channel, fast) && + channel != "" { + channelsNotFollowingConventional = append(channelsNotFollowingConventional, channel) + } + + } + + if len(channelsNotFollowingConventional) > 0 { + return fmt.Errorf("channel(s) %+q are not following the recommended naming convention: "+ + "https://olm.operatorframework.io/docs/best-practices/channel-naming", + channelsNotFollowingConventional) + } + + return nil +} + +// getUniqueValues return the values without duplicates +func getUniqueValues(array []string) []string { + var result []string + uniqueValues := make(map[string]string) + for _, n := range array { + uniqueValues[strings.TrimSpace(n)] = "" + } + + for k, _ := range uniqueValues { + result = append(result, k) + } + return result +} diff --git a/pkg/validation/internal/good_practices_test.go b/pkg/validation/internal/good_practices_test.go index 5d2424f05..86ef36d81 100644 --- a/pkg/validation/internal/good_practices_test.go +++ b/pkg/validation/internal/good_practices_test.go @@ -62,6 +62,14 @@ func Test_ValidateGoodPractices(t *testing.T) { }, errStrings: []string{"Error: Value etcdoperator.v0.9.4: unable to find a deployment to install in the CSV"}, }, + { + name: "should raise an warn when the channel does not follows the convention", + wantWarning: true, + args: args{ + bundleDir: "./testdata/bundle_with_metadata", + }, + warnStrings: []string{"Warning: Value memcached-operator.v0.0.1: channel(s) [\"alpha\"] are not following the recommended naming convention: https://olm.operatorframework.io/docs/best-practices/channel-naming"}, + }, } for _, tt := range tests { @@ -92,3 +100,50 @@ func Test_ValidateGoodPractices(t *testing.T) { }) } } + +func TestValidateHubChannels(t *testing.T) { + type args struct { + channels []string + } + tests := []struct { + name string + args args + wantWarn bool + warnStrings []string + }{ + { + name: "should not return warning when the channel names following the convention", + args: args{ + channels: []string{"fast", "candidate"}, + }, + wantWarn: false, + }, + { + name: "should return warning when the channel names are NOT following the convention", + args: args{ + channels: []string{"mychannel-4.5"}, + }, + wantWarn: true, + warnStrings: []string{"channel(s) [\"mychannel-4.5\"] are not following the recommended naming convention: https://olm.operatorframework.io/docs/best-practices/channel-naming"}, + }, + { + name: "should return warning when has 1 channel NOT following the convention along the others which follows up", + args: args{ + channels: []string{"alpha", "fast-v2.1", "candidate-v2.2"}, + }, + wantWarn: true, + warnStrings: []string{"channel(s) [\"alpha\"] are not following the recommended naming convention: https://olm.operatorframework.io/docs/best-practices/channel-naming"}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := validateHubChannels(tt.args.channels) + if (err != nil) != tt.wantWarn { + t.Errorf("validateHubChannels() error = %v, wantWarn %v", err, tt.wantWarn) + } + if len(tt.warnStrings) > 0 { + require.Contains(t, tt.warnStrings, err.Error()) + } + }) + } +} diff --git a/pkg/validation/internal/operatorhub.go b/pkg/validation/internal/operatorhub.go index 48456b308..cca7eddac 100644 --- a/pkg/validation/internal/operatorhub.go +++ b/pkg/validation/internal/operatorhub.go @@ -215,40 +215,9 @@ func validateBundleOperatorHub(bundle *manifests.Bundle, k8sVersion string) erro result.Add(errors.WarnFailedValidation(warn.Error(), bundle.CSV.GetName())) } - if warn := validateHubChannels(bundle.Channels); warn != nil { - result.Add(errors.WarnFailedValidation(warn.Error(), bundle.CSV.GetName())) - } - return result } -// validateHubChannels will check the channels. The motivation for the following check is to ensure that operators -// authors knows if their operator bundles are or not respecting the Naming Convention Rules. -// However, the operator authors still able to choose the names as please them. -func validateHubChannels(channels []string) error { - const candidate = "candidate" - const stable = "stable" - const fast = "fast" - - var channelsNotFollowingConventional []string - for _, channel := range channels { - if !strings.HasPrefix(channel, candidate) && - !strings.HasPrefix(channel, stable) && - !strings.HasPrefix(channel, fast) { - channelsNotFollowingConventional = append(channelsNotFollowingConventional, channel) - } - - } - - if len(channelsNotFollowingConventional) > 0 { - return fmt.Errorf("channel(s) %+q are not following the recommended naming convention: "+ - "https://olm.operatorframework.io/docs/best-practices/channel-naming", - channelsNotFollowingConventional) - } - - return nil -} - // validateHubCSVSpec will check the CSV against the criteria to publish an // operator bundle in the OperatorHub.io func validateHubCSVSpec(csv v1alpha1.ClusterServiceVersion) CSVChecks { diff --git a/pkg/validation/internal/operatorhub_test.go b/pkg/validation/internal/operatorhub_test.go index a046ad321..1c1df7216 100644 --- a/pkg/validation/internal/operatorhub_test.go +++ b/pkg/validation/internal/operatorhub_test.go @@ -342,36 +342,3 @@ func TestCheckSpecMinKubeVersion(t *testing.T) { }) } } - -func TestValidateHubChannels(t *testing.T) { - type args struct { - channels []string - } - tests := []struct { - name string - args args - wantWarn bool - }{ - { - name: "should not return warning when the channel names following the convention", - args: args{ - channels: []string{"fast", "candidate"}, - }, - wantWarn: false, - }, - { - name: "should return warning when the channel names are NOT following the convention", - args: args{ - channels: []string{"mychannel-4.5"}, - }, - wantWarn: true, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - if err := validateHubChannels(tt.args.channels); (err != nil) != tt.wantWarn { - t.Errorf("validateHubChannels() error = %v, wantWarn %v", err, tt.wantWarn) - } - }) - } -} diff --git a/pkg/validation/internal/testdata/bundle_with_metadata/manifests/cache.example.com_memcacheds.yaml b/pkg/validation/internal/testdata/bundle_with_metadata/manifests/cache.example.com_memcacheds.yaml new file mode 100644 index 000000000..db64899c2 --- /dev/null +++ b/pkg/validation/internal/testdata/bundle_with_metadata/manifests/cache.example.com_memcacheds.yaml @@ -0,0 +1,66 @@ +apiVersion: apiextensions.k8s.io/v1 +kind: CustomResourceDefinition +metadata: + annotations: + controller-gen.kubebuilder.io/version: v0.8.0 + creationTimestamp: null + name: memcacheds.cache.example.com +spec: + group: cache.example.com + names: + kind: Memcached + listKind: MemcachedList + plural: memcacheds + singular: bundle_with_metadata + scope: Namespaced + versions: + - name: v1alpha1 + schema: + openAPIV3Schema: + description: Memcached is the Schema for the memcacheds API + properties: + apiVersion: + description: 'APIVersion defines the versioned schema of this representation + of an object. Servers should convert recognized schemas to the latest + internal value, and may reject unrecognized values. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources' + type: string + kind: + description: 'Kind is a string value representing the REST resource this + object represents. Servers may infer this from the endpoint the client + submits requests to. Cannot be updated. In CamelCase. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds' + type: string + metadata: + type: object + spec: + description: MemcachedSpec defines the desired state of Memcached + properties: + foo: + description: Foo is an example field of Memcached. Edit memcached_types.go + to remove/update + type: string + size: + description: Size defines the number of Memcached instances + format: int32 + type: integer + type: object + status: + description: MemcachedStatus defines the observed state of Memcached + properties: + nodes: + description: Nodes store the name of the pods which are running Memcached + instances + items: + type: string + type: array + type: object + type: object + served: true + storage: true + subresources: + status: {} +status: + acceptedNames: + kind: "" + plural: "" + conditions: [] + storedVersions: [] diff --git a/pkg/validation/internal/testdata/bundle_with_metadata/manifests/memcached-operator.clusterserviceversion.yaml b/pkg/validation/internal/testdata/bundle_with_metadata/manifests/memcached-operator.clusterserviceversion.yaml new file mode 100644 index 000000000..57c01965e --- /dev/null +++ b/pkg/validation/internal/testdata/bundle_with_metadata/manifests/memcached-operator.clusterserviceversion.yaml @@ -0,0 +1,268 @@ +apiVersion: operators.coreos.com/v1alpha1 +kind: ClusterServiceVersion +metadata: + annotations: + alm-examples: |- + [ + { + "apiVersion": "cache.example.com/v1alpha1", + "kind": "Memcached", + "metadata": { + "name": "memcached-sample" + }, + "spec": { + "size": 1 + } + } + ] + capabilities: Basic Install + name: memcached-operator.v0.0.1 + namespace: placeholder +spec: + apiservicedefinitions: {} + customresourcedefinitions: + owned: + - description: Memcached is the Schema for the memcacheds API + displayName: Memcached + kind: Memcached + name: memcacheds.cache.example.com + version: v1alpha1 + description: Memcached Operator description. TODO. + displayName: Memcached Operator + icon: + - base64data: "" + mediatype: "" + install: + spec: + clusterPermissions: + - rules: + - apiGroups: + - apps + resources: + - deployments + verbs: + - create + - delete + - get + - list + - patch + - update + - watch + - apiGroups: + - cache.example.com + resources: + - memcacheds + verbs: + - create + - delete + - get + - list + - patch + - update + - watch + - apiGroups: + - cache.example.com + resources: + - memcacheds/finalizers + verbs: + - update + - apiGroups: + - cache.example.com + resources: + - memcacheds/status + verbs: + - get + - patch + - update + - apiGroups: + - "" + resources: + - pods + verbs: + - get + - list + - watch + - apiGroups: + - authentication.k8s.io + resources: + - tokenreviews + verbs: + - create + - apiGroups: + - authorization.k8s.io + resources: + - subjectaccessreviews + verbs: + - create + serviceAccountName: bundle_with_metadata-operator-controller-manager + deployments: + - label: + control-plane: controller-manager + name: bundle_with_metadata-operator-controller-manager + spec: + replicas: 1 + selector: + matchLabels: + control-plane: controller-manager + strategy: {} + template: + metadata: + annotations: + kubectl.kubernetes.io/default-container: manager + labels: + control-plane: controller-manager + spec: + containers: + - args: + - --secure-listen-address=0.0.0.0:8443 + - --upstream=http://127.0.0.1:8080/ + - --logtostderr=true + - --v=0 + image: gcr.io/kubebuilder/kube-rbac-proxy:v0.8.0 + name: kube-rbac-proxy + ports: + - containerPort: 8443 + name: https + protocol: TCP + resources: + limits: + cpu: 500m + memory: 128Mi + requests: + cpu: 5m + memory: 64Mi + - args: + - --health-probe-bind-address=:8081 + - --metrics-bind-address=127.0.0.1:8080 + - --leader-elect + command: + - /manager + image: quay.io/example/bundle_with_metadata-operator:v0.0.1 + livenessProbe: + httpGet: + path: /healthz + port: 8081 + initialDelaySeconds: 15 + periodSeconds: 20 + name: manager + ports: + - containerPort: 9443 + name: webhook-server + protocol: TCP + readinessProbe: + httpGet: + path: /readyz + port: 8081 + initialDelaySeconds: 5 + periodSeconds: 10 + resources: + limits: + cpu: 500m + memory: 128Mi + requests: + cpu: 10m + memory: 64Mi + securityContext: + allowPrivilegeEscalation: false + securityContext: + runAsNonRoot: true + serviceAccountName: bundle_with_metadata-operator-controller-manager + terminationGracePeriodSeconds: 10 + permissions: + - rules: + - apiGroups: + - "" + resources: + - configmaps + verbs: + - get + - list + - watch + - create + - update + - patch + - delete + - apiGroups: + - coordination.k8s.io + resources: + - leases + verbs: + - get + - list + - watch + - create + - update + - patch + - delete + - apiGroups: + - "" + resources: + - events + verbs: + - create + - patch + serviceAccountName: bundle_with_metadata-operator-controller-manager + strategy: deployment + installModes: + - supported: false + type: OwnNamespace + - supported: false + type: SingleNamespace + - supported: false + type: MultiNamespace + - supported: true + type: AllNamespaces + keywords: + - bundle_with_metadata-operator + links: + - name: Memcached Operator + url: https://bundle_with_metadata-operator.domain + maintainers: + - email: your@email.com + name: Maintainer Name + maturity: alpha + provider: + name: Provider Name + url: https://your.domain + version: 0.0.1 + webhookdefinitions: + - admissionReviewVersions: + - v1 + containerPort: 443 + deploymentName: bundle_with_metadata-operator-controller-manager + failurePolicy: Fail + generateName: mmemcached.kb.io + rules: + - apiGroups: + - cache.example.com + apiVersions: + - v1alpha1 + operations: + - CREATE + - UPDATE + resources: + - memcacheds + sideEffects: None + targetPort: 9443 + type: MutatingAdmissionWebhook + webhookPath: /mutate-cache-example-com-v1alpha1-bundle_with_metadata + - admissionReviewVersions: + - v1 + containerPort: 443 + deploymentName: bundle_with_metadata-operator-controller-manager + failurePolicy: Fail + generateName: vmemcached.kb.io + rules: + - apiGroups: + - cache.example.com + apiVersions: + - v1alpha1 + operations: + - CREATE + - UPDATE + resources: + - memcacheds + sideEffects: None + targetPort: 9443 + type: ValidatingAdmissionWebhook + webhookPath: /validate-cache-example-com-v1alpha1-bundle_with_metadata diff --git a/pkg/validation/internal/testdata/bundle_with_metadata/metadata/annotations.yaml b/pkg/validation/internal/testdata/bundle_with_metadata/metadata/annotations.yaml new file mode 100644 index 000000000..8007e1176 --- /dev/null +++ b/pkg/validation/internal/testdata/bundle_with_metadata/metadata/annotations.yaml @@ -0,0 +1,13 @@ +annotations: + # Core bundle_with_metadata annotations. + operators.operatorframework.io.bundle.mediatype.v1: registry+v1 + operators.operatorframework.io.bundle.manifests.v1: manifests/ + operators.operatorframework.io.bundle.metadata.v1: metadata/ + operators.operatorframework.io.bundle.package.v1: memcache-operator + operators.operatorframework.io.bundle.channels.v1: alpha + operators.operatorframework.io.bundle.channel.default.v1: alpha + + # Annotations for testing. + operators.operatorframework.io.test.mediatype.v1: scorecard+v1 + operators.operatorframework.io.test.config.v1: tests/scorecard/ +