Skip to content

✨ (GoodPracticesValidator) : Add a new check to warn authors when permissions to create CRDs are found #241

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
openshift-merge-robot merged 1 commit into from
May 24, 2022
Merged

✨ (GoodPracticesValidator) : Add a new check to warn authors when permissions to create CRDs are found #241

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 68 additions & 1 deletion pkg/validation/internal/good_practices.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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) {
Expand Down Expand Up @@ -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()))
Expand Down Expand Up @@ -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 {

@dinhxuanvu dinhxuanvu May 19, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we have a better way to check for this. This looks rather ugly though I guess you didn't have much of a choice.

@camilamacedo86 camilamacedo86 May 20, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is easily readable and understandable then we try to make it less ugly when behind the scene will do the same.

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
}
}
}
}
}
}
}
}
}
}
Comment thread
camilamacedo86 marked this conversation as resolved.

return false
}

// getUniqueValues return the values without duplicates
func getUniqueValues(array []string) []string {
var result []string
Expand Down
98 changes: 89 additions & 9 deletions pkg/validation/internal/good_practices_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand Down Expand Up @@ -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)
}
}
})
}
}
Expand Down Expand Up @@ -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())
}
})
}
}