feat: add new validator to check/warning when the resource request is not defined for a container used/shipped in the bundle

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))
 			}
 		}
 	}

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
+}

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)
+				}
+			}
+		})
+	}
+}

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:

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:

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{