diff --git a/crds/zz_defs.go b/crds/zz_defs.go index e7af33e53..cebebc2ca 100644 --- a/crds/zz_defs.go +++ b/crds/zz_defs.go @@ -1,6 +1,6 @@ // Code generated by go-bindata. (@generated) DO NOT EDIT. - //Package crds generated by go-bindata.// sources: +//Package crds generated by go-bindata.// sources: // operators.coreos.com_catalogsources.yaml // operators.coreos.com_clusterserviceversions.yaml // operators.coreos.com_installplans.yaml diff --git a/pkg/validation/internal/bundle.go b/pkg/validation/internal/bundle.go index 79fa93e35..d109deab7 100644 --- a/pkg/validation/internal/bundle.go +++ b/pkg/validation/internal/bundle.go @@ -52,11 +52,11 @@ func validateServiceAccounts(bundle *manifests.Bundle) []errors.Error { sa := v1.ServiceAccount{} if err := runtime.DefaultUnstructuredConverter.FromUnstructured(obj.Object, &sa); err == nil { if _, ok := saNamesFromCSV[sa.Name]; ok { - errs = append(errs, errors.ErrInvalidBundle(fmt.Sprintf("invalid service account found in bundle. " + - "This service account %s in your bundle is not valid, because a service account with the same name " + - "was already specified in your CSV. If this was unintentional, please remove the service account " + - "manifest from your bundle. If it was intentional to specify a separate service account, " + - "please rename the SA in either the bundle manifest or the CSV.",sa.Name), sa.Name)) + errs = append(errs, errors.ErrInvalidBundle(fmt.Sprintf("invalid service account found in bundle. "+ + "This service account %s in your bundle is not valid, because a service account with the same name "+ + "was already specified in your CSV. If this was unintentional, please remove the service account "+ + "manifest from your bundle. If it was intentional to specify a separate service account, "+ + "please rename the SA in either the bundle manifest or the CSV.", sa.Name), sa.Name)) } } } diff --git a/pkg/validation/internal/good_practices.go b/pkg/validation/internal/good_practices.go new file mode 100644 index 000000000..5f22c86fd --- /dev/null +++ b/pkg/validation/internal/good_practices.go @@ -0,0 +1,77 @@ +package internal + +import ( + goerrors "errors" + "fmt" + + "github.com/operator-framework/api/pkg/manifests" + operatorsv1alpha1 "github.com/operator-framework/api/pkg/operators/v1alpha1" + "github.com/operator-framework/api/pkg/validation/errors" + interfaces "github.com/operator-framework/api/pkg/validation/interfaces" +) + +// GoodPracticesValidator validates the bundle against criteria and suggestions defined as +// good practices for bundles under the operator-framework solutions. (You might give a +// look at https://sdk.operatorframework.io/docs/best-practices/) +// +// 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 +var GoodPracticesValidator interfaces.Validator = interfaces.ValidatorFunc(goodPracticesValidator) + +func goodPracticesValidator(objs ...interface{}) (results []errors.ManifestResult) { + for _, obj := range objs { + switch v := obj.(type) { + case *manifests.Bundle: + results = append(results, validateGoodPracticesFrom(v)) + } + } + return results +} + +func validateGoodPracticesFrom(bundle *manifests.Bundle) errors.ManifestResult { + result := errors.ManifestResult{} + if bundle == nil { + result.Add(errors.ErrInvalidBundle("Bundle is nil", nil)) + return result + } + + result.Name = bundle.Name + + if bundle.CSV == nil { + result.Add(errors.ErrInvalidBundle("Bundle csv is nil", bundle.Name)) + return result + } + + errs, warns := validateResourceRequests(bundle.CSV) + for _, err := range errs { + result.Add(errors.ErrFailedValidation(err.Error(), bundle.CSV.GetName())) + } + for _, warn := range warns { + result.Add(errors.WarnFailedValidation(warn.Error(), bundle.CSV.GetName())) + } + + return result +} + +// validateResourceRequests will return a WARN when the resource request is not set +func validateResourceRequests(csv *operatorsv1alpha1.ClusterServiceVersion) (errs, warns []error) { + if csv.Spec.InstallStrategy.StrategySpec.DeploymentSpecs == nil { + errs = append(errs, goerrors.New("unable to find a deployment to install in the CSV")) + return errs, warns + } + deploymentSpec := csv.Spec.InstallStrategy.StrategySpec.DeploymentSpecs + + for _, dSpec := range deploymentSpec { + for _, c := range dSpec.Spec.Template.Spec.Containers { + if c.Resources.Requests == nil || !(len(c.Resources.Requests.Cpu().String()) != 0 && len(c.Resources.Requests.Memory().String()) != 0) { + msg := fmt.Errorf("unable to find the resource requests for the container %s. It is recommended "+ + "to ensure the resource request for CPU and Memory. Be aware that for some clusters configurations "+ + "it is required to specify requests or limits for those values. Otherwise, the system or quota may "+ + "reject Pod creation. More info: https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/", c.Name) + warns = append(warns, msg) + } + } + } + return errs, warns +} diff --git a/pkg/validation/internal/good_practices_test.go b/pkg/validation/internal/good_practices_test.go new file mode 100644 index 000000000..ed8a5adf4 --- /dev/null +++ b/pkg/validation/internal/good_practices_test.go @@ -0,0 +1,94 @@ +package internal + +import ( + "github.com/operator-framework/api/pkg/manifests" + "github.com/stretchr/testify/require" + "testing" +) + +func Test_ValidateGoodPractices(t *testing.T) { + bundleWithDeploymentSpecEmpty, _ := manifests.GetBundleFromDir("./testdata/valid_bundle") + bundleWithDeploymentSpecEmpty.CSV.Spec.InstallStrategy.StrategySpec.DeploymentSpecs = nil + + type args struct { + bundleDir string + bundle *manifests.Bundle + } + tests := []struct { + name string + args args + wantError bool + wantWarning bool + errStrings []string + warnStrings []string + }{ + { + name: "should pass successfully when the resource request is set for " + + "all containers defined in the bundle", + args: args{ + bundleDir: "./testdata/valid_bundle", + }, + }, + { + name: "should raise an waring when the resource request is NOT set for any of the containers defined in the bundle", + wantWarning: true, + warnStrings: []string{"Warning: Value memcached-operator.v0.0.1: unable to find the resource requests for the container kube-rbac-proxy. It is recommended to ensure the resource request for CPU and Memory. Be aware that for some clusters configurations it is required to specify requests or limits for those values. Otherwise, the system or quota may reject Pod creation. More info: https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/", + "Warning: Value memcached-operator.v0.0.1: unable to find the resource requests for the container manager. It is recommended to ensure the resource request for CPU and Memory. Be aware that for some clusters configurations it is required to specify requests or limits for those values. Otherwise, the system or quota may reject Pod creation. More info: https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/"}, + args: args{ + bundleDir: "./testdata/valid_bundle_v1", + }, + }, + { + name: "should fail when the bundle is nil", + wantError: true, + args: args{ + bundle: nil, + }, + errStrings: []string{"Error: : Bundle is nil"}, + }, + { + name: "should fail when the bundle csv is nil", + wantError: true, + args: args{ + bundle: &manifests.Bundle{CSV: nil, Name: "test"}, + }, + errStrings: []string{"Error: Value test: Bundle csv is nil"}, + }, + { + name: "should fail when the csv.Spec.InstallStrategy.StrategySpec.DeploymentSpecs is nil", + wantError: true, + args: args{ + bundle: bundleWithDeploymentSpecEmpty, + }, + errStrings: []string{"Error: Value etcdoperator.v0.9.4: unable to find a deployment to install in the CSV"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var err error + if len(tt.args.bundleDir) > 0 { + tt.args.bundle, err = manifests.GetBundleFromDir(tt.args.bundleDir) + require.NoError(t, err) + } + results := validateGoodPracticesFrom(tt.args.bundle) + require.Equal(t, tt.wantWarning, len(results.Warnings) > 0) + if tt.wantWarning { + require.Equal(t, len(tt.warnStrings), len(results.Warnings)) + for _, w := range results.Warnings { + wString := w.Error() + require.Contains(t, tt.warnStrings, wString) + } + } + + require.Equal(t, tt.wantError, len(results.Errors) > 0) + if tt.wantError { + require.Equal(t, len(tt.errStrings), len(results.Errors)) + for _, err := range results.Errors { + errString := err.Error() + require.Contains(t, tt.errStrings, errString) + } + } + }) + } +} diff --git a/pkg/validation/internal/testdata/valid_bundle/etcdoperator.v0.9.4.clusterserviceversion.yaml b/pkg/validation/internal/testdata/valid_bundle/etcdoperator.v0.9.4.clusterserviceversion.yaml index 67e2d1562..f5ff405eb 100644 --- a/pkg/validation/internal/testdata/valid_bundle/etcdoperator.v0.9.4.clusterserviceversion.yaml +++ b/pkg/validation/internal/testdata/valid_bundle/etcdoperator.v0.9.4.clusterserviceversion.yaml @@ -207,6 +207,13 @@ spec: fieldPath: metadata.name image: quay.io/coreos/etcd-operator@sha256:66a37fd61a06a43969854ee6d3e21087a98b93838e284a6086b13917f96b0d9b name: etcd-operator + resources: + limits: + cpu: 500m + memory: 128Mi + requests: + cpu: 10m + memory: 64Mi - command: - etcd-backup-operator - --create-crd=false @@ -221,6 +228,13 @@ spec: fieldPath: metadata.name image: quay.io/coreos/etcd-operator@sha256:66a37fd61a06a43969854ee6d3e21087a98b93838e284a6086b13917f96b0d9b name: etcd-backup-operator + resources: + limits: + cpu: 500m + memory: 128Mi + requests: + cpu: 10m + memory: 64Mi - command: - etcd-restore-operator - --create-crd=false @@ -235,6 +249,13 @@ spec: fieldPath: metadata.name image: quay.io/coreos/etcd-operator@sha256:66a37fd61a06a43969854ee6d3e21087a98b93838e284a6086b13917f96b0d9b name: etcd-restore-operator + resources: + limits: + cpu: 500m + memory: 128Mi + requests: + cpu: 10m + memory: 64Mi serviceAccountName: etcd-operator permissions: - rules: diff --git a/pkg/validation/internal/testdata/valid_bundle_v1/memcached-operator.clusterserviceversion.yaml b/pkg/validation/internal/testdata/valid_bundle_v1/memcached-operator.clusterserviceversion.yaml index 8d9dd2847..bddc80769 100644 --- a/pkg/validation/internal/testdata/valid_bundle_v1/memcached-operator.clusterserviceversion.yaml +++ b/pkg/validation/internal/testdata/valid_bundle_v1/memcached-operator.clusterserviceversion.yaml @@ -144,13 +144,6 @@ spec: port: 8081 initialDelaySeconds: 5 periodSeconds: 10 - resources: - limits: - cpu: 100m - memory: 30Mi - requests: - cpu: 100m - memory: 20Mi securityContext: allowPrivilegeEscalation: false securityContext: diff --git a/pkg/validation/validation.go b/pkg/validation/validation.go index 3c35db4b1..66f260fd1 100644 --- a/pkg/validation/validation.go +++ b/pkg/validation/validation.go @@ -46,6 +46,9 @@ var CommunityOperatorValidator = internal.CommunityOperatorValidator // for API deprecation requirements. var AlphaDeprecatedAPIsValidator = internal.AlphaDeprecatedAPIsValidator +// GoodPracticesValidator implements Validator to validate the criteria defined as good practices +var GoodPracticesValidator = internal.GoodPracticesValidator + // AllValidators implements Validator to validate all Operator manifest types. var AllValidators = interfaces.Validators{ PackageManifestValidator, @@ -57,6 +60,7 @@ var AllValidators = interfaces.Validators{ OperatorGroupValidator, CommunityOperatorValidator, AlphaDeprecatedAPIsValidator, + GoodPracticesValidator, } var DefaultBundleValidators = interfaces.Validators{