From 2ffc06216e00cb97ca29cdd31fb4fa6e26e1ea69 Mon Sep 17 00:00:00 2001 From: Camila Macedo Date: Sat, 14 May 2022 19:56:59 +0100 Subject: [PATCH] :sparkles: (GoodPracticesValidator) : new check to warning that authors should not create an operator to manage another operator by looking for RBAC permissions to create CRDs --- pkg/validation/internal/good_practices.go | 69 ++++++++++++- .../internal/good_practices_test.go | 98 +++++++++++++++++-- 2 files changed, 157 insertions(+), 10 deletions(-) diff --git a/pkg/validation/internal/good_practices.go b/pkg/validation/internal/good_practices.go index 4a915db19..bdbdbe86f 100644 --- a/pkg/validation/internal/good_practices.go +++ b/pkg/validation/internal/good_practices.go @@ -6,6 +6,7 @@ import ( "strings" "github.com/operator-framework/api/pkg/manifests" + "github.com/operator-framework/api/pkg/operators/v1alpha1" 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" @@ -22,6 +23,13 @@ import ( // - The channel names seems are not following the convention https://olm.operatorframework.io/docs/best-practices/channel-naming/ // // - CRDs defined in the bundle have empty descriptions +// +// - Check if the CSV has permissions to create CRDs. Note that: +// a) "Operators should own a CRD and only one Operator should control a CRD on a cluster. Two Operators managing the same CRD is not a recommended best practice. In the case where an API exists but with multiple implementations, this is typically an example of a no-op Operator because it doesn't have any deployment or reconciliation loop to define the shared API and other Operators depend on this Operator to provide one implementation of the API, e.g. similar to PVCs or Ingress." +// +// b) "An Operator shouldn't deploy or manage other operators (such patterns are known as meta or super operators or include CRDs in its Operands). It's the Operator Lifecycle Manager's job to manage the deployment and lifecycle of operators. For further information check Dependency Resolution: https://olm.operatorframework.io/docs/concepts/olm-architecture/dependency-resolution/" +// +// WARNING: if you create CRD's via the reconciliations or via the Operands then, OLM cannot handle CRDs migration and update, validation. var GoodPracticesValidator interfaces.Validator = interfaces.ValidatorFunc(goodPracticesValidator) func goodPracticesValidator(objs ...interface{}) (results []errors.ManifestResult) { @@ -51,7 +59,7 @@ func validateGoodPracticesFrom(bundle *manifests.Bundle) errors.ManifestResult { errs, warns := validateResourceRequests(bundle.CSV) warns = append(warns, validateCrdDescriptions(bundle.CSV.Spec.CustomResourceDefinitions)...) warns = append(warns, validateHubChannels(bundle)) - + warns = append(warns, validateRBACForCRDsWith(bundle.CSV)) for _, err := range errs { if err != nil { result.Add(errors.ErrFailedValidation(err.Error(), bundle.CSV.GetName())) @@ -118,6 +126,65 @@ func validateHubChannels(bundle *manifests.Bundle) error { return nil } +// validateRBACForCRDsWith to warning when/if permissions to create CRD are found in the rules +func validateRBACForCRDsWith(csv *operatorsv1alpha1.ClusterServiceVersion) error { + apiGroupResourceMap := map[string][]string{ + "apiextensions.k8s.io": {"customresourcedefinitions", "*", "[*]"}, + } + verbs := []string{"create", "*", "[*]", "patch"} + warning := goerrors.New("CSV contains permissions to create CRD. An Operator shouldn't deploy or manage " + + "other operators (such patterns are known as meta or super operators or include CRDs in its Operands)." + + " It's the Operator Lifecycle Manager's job to manage the deployment and lifecycle of operators. " + + " Please, review the design of your solution and if you should not be using Dependency Resolution from OLM instead." + + " More info: https://sdk.operatorframework.io/docs/best-practices/common-recommendation/") + + for _, perm := range csv.Spec.InstallStrategy.StrategySpec.Permissions { + if hasRBACFor(perm, apiGroupResourceMap, verbs) { + return warning + } + } + + for _, perm := range csv.Spec.InstallStrategy.StrategySpec.ClusterPermissions { + if hasRBACFor(perm, apiGroupResourceMap, verbs) { + return warning + } + } + + return nil +} + +func hasRBACFor(perm v1alpha1.StrategyDeploymentPermissions, apiGroupResourceMap map[string][]string, verbs []string) bool { + // For each APIGroup and list of resources that we are looking for + for apiFromMap, resourcesFromMap := range apiGroupResourceMap { + for _, rule := range perm.Rules { + for _, api := range rule.APIGroups { + // If we found the APIGroup + if api == apiFromMap { + for _, res := range rule.Resources { + for _, resFromMap := range resourcesFromMap { + // If we found the resource + if resFromMap == res { + // Check if we find the verbs: + for _, verbFromList := range verbs { + for _, ruleVerb := range rule.Verbs { + // If we found the verb + if verbFromList == ruleVerb { + // stopping by returning true + return true + } + } + } + } + } + } + } + } + } + } + + return false +} + // getUniqueValues return the values without duplicates func getUniqueValues(array []string) []string { var result []string diff --git a/pkg/validation/internal/good_practices_test.go b/pkg/validation/internal/good_practices_test.go index a41b7de9b..af0a90c76 100644 --- a/pkg/validation/internal/good_practices_test.go +++ b/pkg/validation/internal/good_practices_test.go @@ -3,6 +3,8 @@ package internal import ( "testing" + operatorsv1alpha1 "github.com/operator-framework/api/pkg/operators/v1alpha1" + "github.com/operator-framework/api/pkg/manifests" "github.com/stretchr/testify/require" ) @@ -100,15 +102,6 @@ func Test_ValidateGoodPractices(t *testing.T) { 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) - } - } }) } } @@ -162,3 +155,90 @@ func TestValidateHubChannels(t *testing.T) { }) } } + +func TestValidateRBACForCRDsWith(t *testing.T) { + + bundle, err := manifests.GetBundleFromDir("./testdata/valid_bundle") + require.NoError(t, err) + + bundleWithPermissions, _ := manifests.GetBundleFromDir("./testdata/valid_bundle") + bundleWithPermissions.CSV.Spec.InstallStrategy.StrategySpec.Permissions[0].Rules[0].APIGroups = []string{"apiextensions.k8s.io"} + bundleWithPermissions.CSV.Spec.InstallStrategy.StrategySpec.Permissions[0].Rules[0].Resources = []string{"*"} + bundleWithPermissions.CSV.Spec.InstallStrategy.StrategySpec.Permissions[0].Rules[0].Verbs = []string{"*"} + + bundleWithPermissionsResource, _ := manifests.GetBundleFromDir("./testdata/valid_bundle") + bundleWithPermissionsResource.CSV.Spec.InstallStrategy.StrategySpec.Permissions[0].Rules[0].APIGroups = []string{"apiextensions.k8s.io"} + bundleWithPermissionsResource.CSV.Spec.InstallStrategy.StrategySpec.Permissions[0].Rules[0].Resources = []string{"customresourcedefinitions"} + bundleWithPermissionsResource.CSV.Spec.InstallStrategy.StrategySpec.Permissions[0].Rules[0].Verbs = []string{"*"} + + bundleWithPermissionsResourceCreate, _ := manifests.GetBundleFromDir("./testdata/valid_bundle") + bundleWithPermissionsResourceCreate.CSV.Spec.InstallStrategy.StrategySpec.Permissions[0].Rules[0].APIGroups = []string{"apiextensions.k8s.io"} + bundleWithPermissionsResourceCreate.CSV.Spec.InstallStrategy.StrategySpec.Permissions[0].Rules[0].Resources = []string{"customresourcedefinitions"} + bundleWithPermissionsResourceCreate.CSV.Spec.InstallStrategy.StrategySpec.Permissions[0].Rules[0].Verbs = []string{"create"} + + bundleWithPermissionsResourcePatch, _ := manifests.GetBundleFromDir("./testdata/valid_bundle") + bundleWithPermissionsResourcePatch.CSV.Spec.InstallStrategy.StrategySpec.Permissions[0].Rules[0].APIGroups = []string{"apiextensions.k8s.io"} + bundleWithPermissionsResourcePatch.CSV.Spec.InstallStrategy.StrategySpec.Permissions[0].Rules[0].Resources = []string{"customresourcedefinitions"} + bundleWithPermissionsResourcePatch.CSV.Spec.InstallStrategy.StrategySpec.Permissions[0].Rules[0].Verbs = []string{"patch"} + + type args struct { + bundleCSV *operatorsv1alpha1.ClusterServiceVersion + } + tests := []struct { + name string + args args + wantWarn bool + warnStrings []string + }{ + { + name: "should not return warning when has no permissions", + args: args{ + bundleCSV: bundle.CSV, + }, + wantWarn: false, + }, + { + name: "should return warning when has permissions for all verbs and resources kind of the apiGroup", + args: args{ + bundleCSV: bundleWithPermissions.CSV, + }, + wantWarn: true, + warnStrings: []string{"CSV contains permissions to create CRD. An Operator shouldn't deploy or manage other operators (such patterns are known as meta or super operators or include CRDs in its Operands). It's the Operator Lifecycle Manager's job to manage the deployment and lifecycle of operators. Please, review the design of your solution and if you should not be using Dependency Resolution from OLM instead. More info: https://sdk.operatorframework.io/docs/best-practices/common-recommendation/"}, + }, + { + name: "should return warning when has permissions for all verbs with the resource specified", + args: args{ + bundleCSV: bundleWithPermissionsResource.CSV, + }, + wantWarn: true, + warnStrings: []string{"CSV contains permissions to create CRD. An Operator shouldn't deploy or manage other operators (such patterns are known as meta or super operators or include CRDs in its Operands). It's the Operator Lifecycle Manager's job to manage the deployment and lifecycle of operators. Please, review the design of your solution and if you should not be using Dependency Resolution from OLM instead. More info: https://sdk.operatorframework.io/docs/best-practices/common-recommendation/"}, + }, + { + name: "should return warning when has permissions to create a CRD", + args: args{ + bundleCSV: bundleWithPermissionsResourceCreate.CSV, + }, + wantWarn: true, + warnStrings: []string{"CSV contains permissions to create CRD. An Operator shouldn't deploy or manage other operators (such patterns are known as meta or super operators or include CRDs in its Operands). It's the Operator Lifecycle Manager's job to manage the deployment and lifecycle of operators. Please, review the design of your solution and if you should not be using Dependency Resolution from OLM instead. More info: https://sdk.operatorframework.io/docs/best-practices/common-recommendation/"}, + }, + { + name: "should return warning when has permissions to create a Patch a CRD", + args: args{ + bundleCSV: bundleWithPermissionsResourcePatch.CSV, + }, + wantWarn: true, + warnStrings: []string{"CSV contains permissions to create CRD. An Operator shouldn't deploy or manage other operators (such patterns are known as meta or super operators or include CRDs in its Operands). It's the Operator Lifecycle Manager's job to manage the deployment and lifecycle of operators. Please, review the design of your solution and if you should not be using Dependency Resolution from OLM instead. More info: https://sdk.operatorframework.io/docs/best-practices/common-recommendation/"}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err = validateRBACForCRDsWith(tt.args.bundleCSV) + if (err != nil) != tt.wantWarn { + t.Errorf("validateHubChannels() error = %v, wantWarn %v", err, tt.wantWarn) + } + if err != nil && len(tt.warnStrings) > 0 { + require.Contains(t, tt.warnStrings, err.Error()) + } + }) + } +}