diff --git a/.golangci.yml b/.golangci.yml index 584d5865f..23b394cd5 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -16,6 +16,11 @@ linters-settings: - style disabled-checks: - whyNoLint + settings: + tooManyResultsChecker: + # Maximum number of results. + # Default: 5 + maxResults: 10 gocyclo: min-complexity: 20 mnd: diff --git a/Makefile b/Makefile index 723eacfea..916ca251c 100644 --- a/Makefile +++ b/Makefile @@ -153,8 +153,14 @@ build-image-local: -t ${REGISTRY}/${CERTSUITE_IMAGE_NAME_LEGACY}:${IMAGE_TAG} \ -f Dockerfile . +# Acts differently depending on the host architecture +# Mac vs Linux results-html: - curl -s -O --output-dir internal/results/html ${RESULTS_HTML_URL} + if [ "$(shell uname)" = "Darwin" ]; then \ + curl -s -O --output-dir internal/results/html ${RESULTS_HTML_URL}; \ + else \ + curl -s -o internal/results/html/results.html ${RESULTS_HTML_URL}; \ + fi check-results: ./certsuite check results diff --git a/internal/clientsholder/clientsholder.go b/internal/clientsholder/clientsholder.go index fa280d848..569ea75a2 100644 --- a/internal/clientsholder/clientsholder.go +++ b/internal/clientsholder/clientsholder.go @@ -167,6 +167,15 @@ func SetTestK8sClientsHolder(k8sClient kubernetes.Interface) { clientsHolder.ready = true } +func SetTestK8sDynamicClientsHolder(dynamicClient dynamic.Interface) { + clientsHolder.DynamicClient = dynamicClient + clientsHolder.ready = true +} + +func SetTestClientGroupResources(groupResources []*metav1.APIResourceList) { + clientsHolder.GroupResources = groupResources +} + func ClearTestClientsHolder() { clientsHolder.K8sClient = nil clientsHolder.ready = false diff --git a/pkg/autodiscover/autodiscover.go b/pkg/autodiscover/autodiscover.go index 034880ce5..b9976a04e 100644 --- a/pkg/autodiscover/autodiscover.go +++ b/pkg/autodiscover/autodiscover.go @@ -20,7 +20,6 @@ import ( "context" "errors" "fmt" - "path" "regexp" "strings" "time" @@ -336,52 +335,6 @@ func DoAutoDiscover(config *configuration.TestConfiguration) DiscoveredTestData return data } -func getNetworkAttachmentDefinitions(client *clientsholder.ClientsHolder, namespaces []string) ([]nadClient.NetworkAttachmentDefinition, error) { - var nadList []nadClient.NetworkAttachmentDefinition - - for _, ns := range namespaces { - nad, err := client.CNCFNetworkingClient.K8sCniCncfIoV1().NetworkAttachmentDefinitions(ns).List(context.TODO(), metav1.ListOptions{}) - if err != nil && !kerrors.IsNotFound(err) { - return nil, err - } - - // Append the list of networkAttachmentDefinitions to the nadList slice - nadList = append(nadList, nad.Items...) - } - - return nadList, nil -} - -func getSriovNetworks(client *clientsholder.ClientsHolder, namespaces []string) (sriovNetworks []sriovNetworkOp.SriovNetwork, err error) { - var sriovNetworkList []sriovNetworkOp.SriovNetwork - - for _, ns := range namespaces { - snl, err := client.SriovNetworkingClient.SriovNetworks(ns).List(context.TODO(), metav1.ListOptions{}) - if err != nil && !kerrors.IsNotFound(err) { - return nil, err - } - - // Append the list of sriovNetworks to the sriovNetworks slice - sriovNetworkList = append(sriovNetworkList, snl.Items...) - } - return sriovNetworkList, nil -} - -func getSriovNetworkNodePolicies(client *clientsholder.ClientsHolder, namespaces []string) (sriovNetworkNodePolicies []sriovNetworkOp.SriovNetworkNodePolicy, err error) { - var sriovNetworkNodePolicyList []sriovNetworkOp.SriovNetworkNodePolicy - - for _, ns := range namespaces { - snnp, err := client.SriovNetworkingClient.SriovNetworkNodePolicies(ns).List(context.TODO(), metav1.ListOptions{}) - if err != nil && !kerrors.IsNotFound(err) { - return nil, err - } - - // Append the list of sriovNetworkNodePolicies to the sriovNetworkNodePolicies slice - sriovNetworkNodePolicyList = append(sriovNetworkNodePolicyList, snnp.Items...) - } - return sriovNetworkNodePolicyList, nil -} - func namespacesListToStringList(namespaceList []configuration.Namespace) (stringList []string) { for _, ns := range namespaceList { stringList = append(stringList, ns.Name) @@ -465,62 +418,3 @@ func getPodsOwnedByCsv(csvName, operatorNamespace string, client *clientsholder. } return managedPods, nil } - -// getOperandPodsFromTestCsvs returns a subset of pods whose owner CRs are managed by any of the testCsvs. -func getOperandPodsFromTestCsvs(testCsvs []*olmv1Alpha.ClusterServiceVersion, pods []corev1.Pod) ([]*corev1.Pod, error) { - // Helper var to store all the managed crds from the operators under test - // They map key is "Kind.group/version" or "Kind.APIversion", which should be the same. - // e.g.: "Subscription.operators.coreos.com/v1alpha1" - crds := map[string]*olmv1Alpha.ClusterServiceVersion{} - - // First, iterate on each testCsv to fill the helper crds map. - for _, csv := range testCsvs { - ownedCrds := csv.Spec.CustomResourceDefinitions.Owned - if len(ownedCrds) == 0 { - continue - } - - for i := range ownedCrds { - crd := &ownedCrds[i] - - _, group, found := strings.Cut(crd.Name, ".") - if !found { - return nil, fmt.Errorf("failed to parse resources and group from crd name %q", crd.Name) - } - - log.Info("CSV %q owns crd %v", csv.Name, crd.Kind+"/"+group+"/"+crd.Version) - - crdPath := path.Join(crd.Kind, group, crd.Version) - crds[crdPath] = csv - } - } - - // Now, iterate on every pod in the list to check whether they're owned by any of the CRs that - // the csvs are managing. - operandPods := []*corev1.Pod{} - for i := range pods { - pod := &pods[i] - owners, err := podhelper.GetPodTopOwner(pod.Namespace, pod.OwnerReferences) - if err != nil { - return nil, fmt.Errorf("failed to get top owners of pod %v/%v: %v", pod.Namespace, pod.Name, err) - } - - for _, owner := range owners { - versionedCrdPath := path.Join(owner.Kind, owner.APIVersion) - - var csv *olmv1Alpha.ClusterServiceVersion - if csv = crds[versionedCrdPath]; csv == nil { - // The owner is not a CR or it's not a CR owned by any operator under test - continue - } - - log.Info("Pod %v/%v has owner CR %s of CRD %q (CSV %v)", pod.Namespace, pod.Name, - owner.Name, versionedCrdPath, csv.Name) - - operandPods = append(operandPods, pod) - break - } - } - - return operandPods, nil -} diff --git a/pkg/autodiscover/autodiscover_nads.go b/pkg/autodiscover/autodiscover_nads.go new file mode 100644 index 000000000..5d7a670b1 --- /dev/null +++ b/pkg/autodiscover/autodiscover_nads.go @@ -0,0 +1,26 @@ +package autodiscover + +import ( + "context" + + nadClient "github.com/k8snetworkplumbingwg/network-attachment-definition-client/pkg/apis/k8s.cni.cncf.io/v1" + "github.com/redhat-best-practices-for-k8s/certsuite/internal/clientsholder" + kerrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +func getNetworkAttachmentDefinitions(client *clientsholder.ClientsHolder, namespaces []string) ([]nadClient.NetworkAttachmentDefinition, error) { + var nadList []nadClient.NetworkAttachmentDefinition + + for _, ns := range namespaces { + nad, err := client.CNCFNetworkingClient.K8sCniCncfIoV1().NetworkAttachmentDefinitions(ns).List(context.TODO(), metav1.ListOptions{}) + if err != nil && !kerrors.IsNotFound(err) { + return nil, err + } + + // Append the list of networkAttachmentDefinitions to the nadList slice + nadList = append(nadList, nad.Items...) + } + + return nadList, nil +} diff --git a/pkg/autodiscover/autodiscover_nads_test.go b/pkg/autodiscover/autodiscover_nads_test.go new file mode 100644 index 000000000..ffb6e761d --- /dev/null +++ b/pkg/autodiscover/autodiscover_nads_test.go @@ -0,0 +1 @@ +package autodiscover diff --git a/pkg/autodiscover/autodiscover_operators.go b/pkg/autodiscover/autodiscover_operators.go index 1d0a03714..dc8904709 100644 --- a/pkg/autodiscover/autodiscover_operators.go +++ b/pkg/autodiscover/autodiscover_operators.go @@ -19,17 +19,21 @@ package autodiscover import ( "context" "fmt" + "path" + "strings" helmclient "github.com/mittwald/go-helm-client" olmv1Alpha "github.com/operator-framework/api/pkg/operators/v1alpha1" "github.com/operator-framework/operator-lifecycle-manager/pkg/api/client/clientset/versioned/typed/operators/v1alpha1" "github.com/redhat-best-practices-for-k8s/certsuite/internal/log" "github.com/redhat-best-practices-for-k8s/certsuite/pkg/configuration" + "github.com/redhat-best-practices-for-k8s/certsuite/pkg/podhelper" olmpkgv1 "github.com/operator-framework/operator-lifecycle-manager/pkg/package-server/apis/operators/v1" olmpkgclient "github.com/operator-framework/operator-lifecycle-manager/pkg/package-server/client/clientset/versioned/typed/operators/v1" "github.com/redhat-best-practices-for-k8s/certsuite/pkg/stringhelper" "helm.sh/helm/v3/pkg/release" + corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" appv1client "k8s.io/client-go/kubernetes/typed/apps/v1" @@ -238,3 +242,62 @@ func getAllPackageManifests(olmPkgClient olmpkgclient.PackageManifestInterface) } return out } + +// getOperandPodsFromTestCsvs returns a subset of pods whose owner CRs are managed by any of the testCsvs. +func getOperandPodsFromTestCsvs(testCsvs []*olmv1Alpha.ClusterServiceVersion, pods []corev1.Pod) ([]*corev1.Pod, error) { + // Helper var to store all the managed crds from the operators under test + // They map key is "Kind.group/version" or "Kind.APIversion", which should be the same. + // e.g.: "Subscription.operators.coreos.com/v1alpha1" + crds := map[string]*olmv1Alpha.ClusterServiceVersion{} + + // First, iterate on each testCsv to fill the helper crds map. + for _, csv := range testCsvs { + ownedCrds := csv.Spec.CustomResourceDefinitions.Owned + if len(ownedCrds) == 0 { + continue + } + + for i := range ownedCrds { + crd := &ownedCrds[i] + + _, group, found := strings.Cut(crd.Name, ".") + if !found { + return nil, fmt.Errorf("failed to parse resources and group from crd name %q", crd.Name) + } + + log.Info("CSV %q owns crd %v", csv.Name, crd.Kind+"/"+group+"/"+crd.Version) + + crdPath := path.Join(crd.Kind, group, crd.Version) + crds[crdPath] = csv + } + } + + // Now, iterate on every pod in the list to check whether they're owned by any of the CRs that + // the csvs are managing. + operandPods := []*corev1.Pod{} + for i := range pods { + pod := &pods[i] + owners, err := podhelper.GetPodTopOwner(pod.Namespace, pod.OwnerReferences) + if err != nil { + return nil, fmt.Errorf("failed to get top owners of pod %v/%v: %v", pod.Namespace, pod.Name, err) + } + + for _, owner := range owners { + versionedCrdPath := path.Join(owner.Kind, owner.APIVersion) + + var csv *olmv1Alpha.ClusterServiceVersion + if csv = crds[versionedCrdPath]; csv == nil { + // The owner is not a CR or it's not a CR owned by any operator under test + continue + } + + log.Info("Pod %v/%v has owner CR %s of CRD %q (CSV %v)", pod.Namespace, pod.Name, + owner.Name, versionedCrdPath, csv.Name) + + operandPods = append(operandPods, pod) + break + } + } + + return operandPods, nil +} diff --git a/pkg/autodiscover/autodiscover_sriov.go b/pkg/autodiscover/autodiscover_sriov.go new file mode 100644 index 000000000..5154f63ed --- /dev/null +++ b/pkg/autodiscover/autodiscover_sriov.go @@ -0,0 +1,40 @@ +package autodiscover + +import ( + "context" + + sriovNetworkOp "github.com/k8snetworkplumbingwg/sriov-network-operator/api/v1" + "github.com/redhat-best-practices-for-k8s/certsuite/internal/clientsholder" + kerrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +func getSriovNetworks(client *clientsholder.ClientsHolder, namespaces []string) (sriovNetworks []sriovNetworkOp.SriovNetwork, err error) { + var sriovNetworkList []sriovNetworkOp.SriovNetwork + + for _, ns := range namespaces { + snl, err := client.SriovNetworkingClient.SriovNetworks(ns).List(context.TODO(), metav1.ListOptions{}) + if err != nil && !kerrors.IsNotFound(err) { + return nil, err + } + + // Append the list of sriovNetworks to the sriovNetworks slice + sriovNetworkList = append(sriovNetworkList, snl.Items...) + } + return sriovNetworkList, nil +} + +func getSriovNetworkNodePolicies(client *clientsholder.ClientsHolder, namespaces []string) (sriovNetworkNodePolicies []sriovNetworkOp.SriovNetworkNodePolicy, err error) { + var sriovNetworkNodePolicyList []sriovNetworkOp.SriovNetworkNodePolicy + + for _, ns := range namespaces { + snnp, err := client.SriovNetworkingClient.SriovNetworkNodePolicies(ns).List(context.TODO(), metav1.ListOptions{}) + if err != nil && !kerrors.IsNotFound(err) { + return nil, err + } + + // Append the list of sriovNetworkNodePolicies to the sriovNetworkNodePolicies slice + sriovNetworkNodePolicyList = append(sriovNetworkNodePolicyList, snnp.Items...) + } + return sriovNetworkNodePolicyList, nil +} diff --git a/pkg/autodiscover/autodiscover_sriov_test.go b/pkg/autodiscover/autodiscover_sriov_test.go new file mode 100644 index 000000000..ffb6e761d --- /dev/null +++ b/pkg/autodiscover/autodiscover_sriov_test.go @@ -0,0 +1 @@ +package autodiscover diff --git a/pkg/podhelper/podhelper.go b/pkg/podhelper/podhelper.go index 85ff4f18e..738ac53a2 100644 --- a/pkg/podhelper/podhelper.go +++ b/pkg/podhelper/podhelper.go @@ -22,7 +22,12 @@ type TopOwner struct { // Get the list of top owners of pods func GetPodTopOwner(podNamespace string, podOwnerReferences []metav1.OwnerReference) (topOwners map[string]TopOwner, err error) { topOwners = make(map[string]TopOwner) - err = followOwnerReferences(clientsholder.GetClientsHolder().GroupResources, clientsholder.GetClientsHolder().DynamicClient, topOwners, podNamespace, podOwnerReferences) + err = followOwnerReferences( + clientsholder.GetClientsHolder().GroupResources, + clientsholder.GetClientsHolder().DynamicClient, + topOwners, + podNamespace, + podOwnerReferences) if err != nil { return topOwners, fmt.Errorf("could not get top owners, err: %v", err) } diff --git a/pkg/podhelper/podhelper_test.go b/pkg/podhelper/podhelper_test.go index 0d4ccaa0a..5f88aed49 100644 --- a/pkg/podhelper/podhelper_test.go +++ b/pkg/podhelper/podhelper_test.go @@ -1,10 +1,13 @@ package podhelper import ( + "errors" "maps" "testing" olmv1Alpha "github.com/operator-framework/api/pkg/operators/v1alpha1" + "github.com/redhat-best-practices-for-k8s/certsuite/internal/clientsholder" + "github.com/stretchr/testify/assert" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" @@ -133,3 +136,109 @@ func Test_followOwnerReferences(t *testing.T) { }) } } + +func TestSearchAPIResource(t *testing.T) { + testCases := []struct { + testKind string + testAPIVersion string + testResourceList []*metav1.APIResourceList + expectedError error + }{ + { // Test Case #1 - APIResource found + testKind: "ClusterServiceVersion", + testAPIVersion: "operators.coreos.com/v1alpha1", + testResourceList: []*metav1.APIResourceList{ + { + GroupVersion: "operators.coreos.com/v1alpha1", + APIResources: []metav1.APIResource{ + { + Name: "clusterserviceversions", + Kind: "ClusterServiceVersion", + Namespaced: true, + }, + }, + }, + }, + expectedError: nil, + }, + { // Test Case #2 - APIResource not found + testKind: "ClusterServiceVersion", + testAPIVersion: "operators.coreos.com/v1alpha1", + testResourceList: []*metav1.APIResourceList{ + { + GroupVersion: "operators.redhat-test.com/v1alpha1", + APIResources: []metav1.APIResource{ + { + Name: "clusterserviceversions", + Kind: "ClusterServiceVersion", + Namespaced: true, + }, + }, + }, + }, + expectedError: errors.New("apiResource not found for kind=ClusterServiceVersion and APIVersion=operators.coreos.com/v1alpha1"), + }, + } + + for _, tc := range testCases { + resource, err := searchAPIResource(tc.testKind, tc.testAPIVersion, tc.testResourceList) + if tc.expectedError != nil { + assert.Equal(t, tc.expectedError, err) + } else { + assert.Nil(t, err) + assert.Equal(t, tc.testKind, resource.Kind) + } + } +} + +func TestGetTopOwners(t *testing.T) { + generatePod := func(name, namespace string, ownerRefs []metav1.OwnerReference) *corev1.Pod { + return &corev1.Pod{ + TypeMeta: metav1.TypeMeta{Kind: "Pod", APIVersion: "v1"}, + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: namespace, + OwnerReferences: ownerRefs, + }, + } + } + + generateDeployment := func(name, namespace string, ownerRefs []metav1.OwnerReference) *appsv1.Deployment { + return &appsv1.Deployment{ + TypeMeta: metav1.TypeMeta{Kind: "Deployment", APIVersion: "apps/v1"}, + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: namespace, + OwnerReferences: ownerRefs, + }, + } + } + + testDeployment := generateDeployment("dep1", "ns1", []metav1.OwnerReference{}) + testPod := generatePod("pod1", "ns1", []metav1.OwnerReference{{APIVersion: "apps/v1", Kind: "Deployment", Name: "dep1"}}) + + resourceList := []*metav1.APIResourceList{ + {GroupVersion: "apps/v1", APIResources: []metav1.APIResource{{Name: "deployments", Kind: "Deployment", Namespaced: true}}}, + {GroupVersion: "v1", APIResources: []metav1.APIResource{{Name: "pods", Kind: "Pod", Namespaced: true}}}, + } + + client := k8sDynamicFake.NewSimpleDynamicClient(k8sscheme.Scheme, testDeployment, testPod) + + // Spoof the get functions + client.Fake.AddReactor("get", "Deployment", func(action k8stesting.Action) (handled bool, ret runtime.Object, err error) { + return true, testDeployment, nil + }) + + client.Fake.AddReactor("get", "Pod", func(action k8stesting.Action) (handled bool, ret runtime.Object, err error) { + return true, testPod, nil + }) + + // Set the test clients + clientsholder.SetTestK8sDynamicClientsHolder(client) + clientsholder.SetTestClientGroupResources(resourceList) + + // Get the top owner for the pod which is a deployment + topOwners, err := GetPodTopOwner("ns1", testPod.OwnerReferences) + assert.Nil(t, err) + assert.Equal(t, map[string]TopOwner{"dep1": {APIVersion: "apps/v1", Namespace: "ns1", Kind: "Deployment", Name: "dep1"}}, topOwners) +} diff --git a/pkg/stringhelper/stringhelper_test.go b/pkg/stringhelper/stringhelper_test.go index bea34e694..84e8eb56a 100644 --- a/pkg/stringhelper/stringhelper_test.go +++ b/pkg/stringhelper/stringhelper_test.go @@ -259,3 +259,41 @@ func TestPointerToString(t *testing.T) { t.Errorf("PointerToString() = %v, want %v", got, want) } } + +func TestHasAtLeastOneCommonElement(t *testing.T) { + testCases := []struct { + slice1 []string + slice2 []string + expected bool + }{ + { + slice1: []string{"one", "two", "three"}, + slice2: []string{"one", "two"}, + expected: true, + }, + { + slice1: []string{"one", "two", "three"}, + slice2: []string{"four", "five"}, + expected: false, + }, + { + slice1: []string{"one", "two", "three"}, + slice2: []string{"one", "two", "three"}, + expected: true, + }, + { + slice1: []string{}, + slice2: []string{"one", "two", "three"}, + expected: false, + }, + { + slice1: []string{"one", "two", "three"}, + slice2: []string{"two", "one"}, + expected: true, + }, + } + + for _, tc := range testCases { + assert.Equal(t, tc.expected, HasAtLeastOneCommonElement(tc.slice1, tc.slice2)) + } +} diff --git a/tests/operator/access/access.go b/tests/operator/access/access.go new file mode 100644 index 000000000..4bf9282b7 --- /dev/null +++ b/tests/operator/access/access.go @@ -0,0 +1,39 @@ +package access + +import ( + "github.com/operator-framework/api/pkg/operators/v1alpha1" +) + +func PermissionsHaveBadRule(clusterPermissions []v1alpha1.StrategyDeploymentPermissions) bool { + badRuleFound := false + for permissionIndex := range clusterPermissions { + permission := &clusterPermissions[permissionIndex] + for ruleIndex := range permission.Rules { + rule := &permission.Rules[ruleIndex] + + // Check whether the rule is for the security api group. + securityGroupFound := false + for _, group := range rule.APIGroups { + if group == "*" || group == "security.openshift.io" { + securityGroupFound = true + break + } + } + + if !securityGroupFound { + continue + } + + // Now check whether it grants some access to securitycontextconstraint resources. + for _, resource := range rule.Resources { + if resource == "*" || resource == "securitycontextconstraints" { + // Keep reviewing other permissions' rules so we can log all the failing ones in the claim file. + badRuleFound = true + break + } + } + } + } + + return badRuleFound +} diff --git a/tests/operator/access/access_test.go b/tests/operator/access/access_test.go new file mode 100644 index 000000000..2d9c8ed6d --- /dev/null +++ b/tests/operator/access/access_test.go @@ -0,0 +1,57 @@ +package access + +import ( + "testing" + + "github.com/operator-framework/api/pkg/operators/v1alpha1" + "github.com/stretchr/testify/assert" + rbacv1 "k8s.io/api/rbac/v1" +) + +func TestPermissionsHaveBadRule(t *testing.T) { + generateSDP := func(apiGroups []string, resources []string) v1alpha1.StrategyDeploymentPermissions { + return v1alpha1.StrategyDeploymentPermissions{ + Rules: []rbacv1.PolicyRule{ + { + APIGroups: apiGroups, + Resources: resources, + }, + }, + } + } + + testCases := []struct { + testClusterPermissions []v1alpha1.StrategyDeploymentPermissions + expectedResult bool + }{ + { // SCC granted - this is a bad rule + testClusterPermissions: []v1alpha1.StrategyDeploymentPermissions{ + generateSDP([]string{"security.openshift.io"}, []string{"*"}), + }, + expectedResult: true, + }, + { // SCC granted - this is a bad rule + testClusterPermissions: []v1alpha1.StrategyDeploymentPermissions{ + generateSDP([]string{"security.openshift.io"}, []string{"securitycontextconstraints"}), + }, + expectedResult: true, + }, + { // SCC granted - this is a bad rule + testClusterPermissions: []v1alpha1.StrategyDeploymentPermissions{ + generateSDP([]string{"security.openshift.io"}, []string{"*"}), + generateSDP([]string{"security.openshift.io"}, []string{"securitycontextconstraints"}), + }, + expectedResult: true, + }, + { // No bad rule + testClusterPermissions: []v1alpha1.StrategyDeploymentPermissions{ + generateSDP([]string{"security.heathytest.io"}, []string{"*"}), + }, + expectedResult: false, + }, + } + + for _, tc := range testCases { + assert.Equal(t, tc.expectedResult, PermissionsHaveBadRule(tc.testClusterPermissions)) + } +} diff --git a/tests/operator/helper.go b/tests/operator/helper.go index 80cbedd94..6112beb13 100644 --- a/tests/operator/helper.go +++ b/tests/operator/helper.go @@ -24,6 +24,7 @@ import ( "strings" "github.com/operator-framework/api/pkg/operators/v1alpha1" + "github.com/redhat-best-practices-for-k8s/certsuite/internal/log" "github.com/redhat-best-practices-for-k8s/certsuite/pkg/podhelper" "github.com/redhat-best-practices-for-k8s/certsuite/pkg/provider" "github.com/redhat-best-practices-for-k8s/certsuite/pkg/stringhelper" @@ -91,7 +92,7 @@ func checkIfOperatorUnderTest(operator *provider.Operator) bool { } func checkValidOperatorInstallation(namespace string) (isDedicatedOperatorNamespace bool, singleOrMultiNamespaceOperators, - nonSingleOrMultiNamespaceOperators, csvsFoundButNotInOperatorInstallationNamespace, operatorsFoundButNotUnderTest, podsNotBelongingToOperators []string, err error) { + nonSingleOrMultiNamespaceOperators, csvsFoundButNotInOperatorInstallationNamespace, operatorsFoundButNotUnderTest, podsNotBelongingToOperators []string) { // 1. operator installation checks for _, operator := range getAllOperatorsBy(namespace, env.AllOperators) { @@ -111,14 +112,13 @@ func checkValidOperatorInstallation(namespace string) (isDedicatedOperatorNamesp } // 2. existing pods check - podsBelongingToNoOperators, err := findPodsNotBelongingToOperators(namespace) + podsNotBelongingToOperators, err := findPodsNotBelongingToOperators(namespace) if err != nil { - return false, singleOrMultiNamespaceOperators, nonSingleOrMultiNamespaceOperators, csvsFoundButNotInOperatorInstallationNamespace, operatorsFoundButNotUnderTest, podsNotBelongingToOperators, err + return false, singleOrMultiNamespaceOperators, nonSingleOrMultiNamespaceOperators, csvsFoundButNotInOperatorInstallationNamespace, operatorsFoundButNotUnderTest, podsNotBelongingToOperators } - return len(podsBelongingToNoOperators) == 0 && len(singleOrMultiNamespaceOperators) != 0, singleOrMultiNamespaceOperators, nonSingleOrMultiNamespaceOperators, - csvsFoundButNotInOperatorInstallationNamespace, operatorsFoundButNotUnderTest, podsNotBelongingToOperators, nil - + return len(podsNotBelongingToOperators) == 0 && len(singleOrMultiNamespaceOperators) != 0, singleOrMultiNamespaceOperators, nonSingleOrMultiNamespaceOperators, + csvsFoundButNotInOperatorInstallationNamespace, operatorsFoundButNotUnderTest, podsNotBelongingToOperators } func findPodsNotBelongingToOperators(namespace string) (podsBelongingToNoOperators []string, err error) { @@ -144,3 +144,36 @@ func findPodsNotBelongingToOperators(namespace string) (podsBelongingToNoOperato return podsBelongingToNoOperators, nil } + +func OperatorInstalledMoreThanOnce(operator1, operator2 *provider.Operator) bool { + // Safeguard against nil operators (should not happen) + if operator1 == nil || operator2 == nil { + return false + } + + log.Debug("Comparing operator %q with operator %q", operator1.Name, operator2.Name) + + // Retrieve the version from each CSV + csv1Version := operator1.Csv.Spec.Version.String() + csv2Version := operator2.Csv.Spec.Version.String() + + log.Debug("CSV1 Version: %s", csv1Version) + log.Debug("CSV2 Version: %s", csv2Version) + + // Strip the version from the CSV name by removing the suffix (which should be the version) + csv1Name := strings.TrimSuffix(operator1.Csv.Name, ".v"+csv1Version) + csv2Name := strings.TrimSuffix(operator2.Csv.Name, ".v"+csv2Version) + + log.Debug("Comparing CSV names %q and %q", csv1Name, csv2Name) + + // The CSV name should be the same, but the version should be different + // if the operator is installed more than once. + if operator1.Csv != nil && operator2.Csv != nil && + csv1Name == csv2Name && + csv1Version != csv2Version { + log.Error("Operator %q is installed more than once", operator1.Name) + return true + } + + return false +} diff --git a/tests/operator/helper_test.go b/tests/operator/helper_test.go index 00c9b2ecd..175843d5e 100644 --- a/tests/operator/helper_test.go +++ b/tests/operator/helper_test.go @@ -22,6 +22,13 @@ package operator import ( "testing" + + "github.com/blang/semver/v4" + opFrameworkVersion "github.com/operator-framework/api/pkg/lib/version" + "github.com/operator-framework/api/pkg/operators/v1alpha1" + "github.com/redhat-best-practices-for-k8s/certsuite/pkg/provider" + "github.com/stretchr/testify/assert" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) func TestSplitCsv(t *testing.T) { @@ -69,3 +76,57 @@ func TestSplitCsv(t *testing.T) { }) } } + +func TestOperatorInstalledMoreThanOnce(t *testing.T) { + generateOperator := func(name, csvName string, major, minor, patch int) *provider.Operator { + return &provider.Operator{ + Name: name, + Csv: &v1alpha1.ClusterServiceVersion{ + ObjectMeta: metav1.ObjectMeta{ + Name: csvName, + }, + Spec: v1alpha1.ClusterServiceVersionSpec{ + Version: opFrameworkVersion.OperatorVersion{ + Version: semver.Version{ + Major: uint64(major), + Minor: uint64(minor), + Patch: uint64(patch), + }, + }, + }, + }, + } + } + + testCases := []struct { + testOp1 *provider.Operator + testOp2 *provider.Operator + expectedOutput bool + }{ + { // Test Case #1 - Both operators are nil + testOp1: nil, + testOp2: nil, + expectedOutput: false, + }, + { // Test Case #2 - One operator is nil + testOp1: &provider.Operator{}, + testOp2: nil, + expectedOutput: false, + }, + { // Test Case #3 - Both operators are different + testOp1: generateOperator("test-operator-1", "test-operator-1.v1.0.0", 1, 0, 0), + testOp2: generateOperator("test-operator-2", "test-operator-2.v1.0.0", 1, 0, 0), + expectedOutput: false, + }, + { // Test Case #4 - Both operators are the same but different versions. + // OLM does not allow the same operator to be installed more than once with the same version. + testOp1: generateOperator("test-operator", "test-operator.v1.0.0", 1, 0, 0), + testOp2: generateOperator("test-operator", "test-operator.v1.0.1", 1, 0, 1), + expectedOutput: true, + }, + } + + for _, tc := range testCases { + assert.Equal(t, tc.expectedOutput, OperatorInstalledMoreThanOnce(tc.testOp1, tc.testOp2)) + } +} diff --git a/tests/operator/openapi/openapi.go b/tests/operator/openapi/openapi.go new file mode 100644 index 000000000..84bd54f22 --- /dev/null +++ b/tests/operator/openapi/openapi.go @@ -0,0 +1,23 @@ +package openapi + +import ( + "strings" + + "github.com/redhat-best-practices-for-k8s/certsuite/pkg/testhelper" + apiextv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" +) + +func IsCRDDefinedWithOpenAPI3Schema(crd *apiextv1.CustomResourceDefinition) bool { + for _, version := range crd.Spec.Versions { + crdSchema := version.Schema.String() + + containsOpenAPIV3SchemaSubstr := strings.Contains(strings.ToLower(crdSchema), + strings.ToLower(testhelper.OpenAPIV3Schema)) + + if containsOpenAPIV3SchemaSubstr { + return true + } + } + + return false +} diff --git a/tests/operator/openapi/openapi_test.go b/tests/operator/openapi/openapi_test.go new file mode 100644 index 000000000..859e33b1d --- /dev/null +++ b/tests/operator/openapi/openapi_test.go @@ -0,0 +1,68 @@ +package openapi + +import ( + "testing" + + "github.com/stretchr/testify/assert" + apiextv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" +) + +func TestIsCRDDefinedWithOpenAPI3Schema(t *testing.T) { + testCases := []struct { + testCRD *apiextv1.CustomResourceDefinition + expectedOutput bool + }{ + { + testCRD: &apiextv1.CustomResourceDefinition{ + Spec: apiextv1.CustomResourceDefinitionSpec{ + Versions: []apiextv1.CustomResourceDefinitionVersion{ + { + Schema: &apiextv1.CustomResourceValidation{ + OpenAPIV3Schema: &apiextv1.JSONSchemaProps{}, + }, + }, + }, + }, + }, + expectedOutput: true, + }, + { + testCRD: &apiextv1.CustomResourceDefinition{ + Spec: apiextv1.CustomResourceDefinitionSpec{ + Versions: []apiextv1.CustomResourceDefinitionVersion{ + { + Schema: &apiextv1.CustomResourceValidation{}, + }, + }, + }, + }, + expectedOutput: true, + }, + { + testCRD: &apiextv1.CustomResourceDefinition{ + Spec: apiextv1.CustomResourceDefinitionSpec{ + Versions: []apiextv1.CustomResourceDefinitionVersion{ + { + Schema: &apiextv1.CustomResourceValidation{ + OpenAPIV3Schema: &apiextv1.JSONSchemaProps{}, + }, + }, + }, + }, + }, + expectedOutput: true, + }, + { + testCRD: &apiextv1.CustomResourceDefinition{ + Spec: apiextv1.CustomResourceDefinitionSpec{ + Versions: nil, + }, + }, + expectedOutput: false, + }, + } + + for _, tc := range testCases { + assert.Equal(t, tc.expectedOutput, IsCRDDefinedWithOpenAPI3Schema(tc.testCRD)) + } +} diff --git a/tests/operator/suite.go b/tests/operator/suite.go index 586467eba..76aeba120 100644 --- a/tests/operator/suite.go +++ b/tests/operator/suite.go @@ -24,7 +24,9 @@ import ( "github.com/Masterminds/semver" "github.com/redhat-best-practices-for-k8s/certsuite/tests/common" "github.com/redhat-best-practices-for-k8s/certsuite/tests/identifiers" + "github.com/redhat-best-practices-for-k8s/certsuite/tests/operator/access" "github.com/redhat-best-practices-for-k8s/certsuite/tests/operator/catalogsource" + "github.com/redhat-best-practices-for-k8s/certsuite/tests/operator/openapi" "github.com/redhat-best-practices-for-k8s/certsuite/tests/operator/phasecheck" "github.com/redhat-best-practices-for-k8s/certsuite/internal/log" @@ -153,19 +155,13 @@ func testOnlySingleNamespacedOperatorsAllowedInTenantNamespaces(check *checksdb. check.LogInfo("Checking if namespace %s contains only valid single/ multi namespaced operators", operatorNamespace) isDedicatedOperatorNamespace, singleOrMultiNamespaceOperators, nonSingleOrMultiNamespaceOperators, - csvsFoundButNotInOperatorInstallationNamespace, operatorsFoundButNotUnderTest, podsNotBelongingToOperators, err := checkValidOperatorInstallation(operatorNamespace) - - if err != nil { - check.LogError("Error %v found in checking namespace %s", err, operatorNamespace) - continue - } + csvsFoundButNotInOperatorInstallationNamespace, operatorsFoundButNotUnderTest, podsNotBelongingToOperators := checkValidOperatorInstallation(operatorNamespace) if isDedicatedOperatorNamespace { msg := fmt.Sprintf("Namespace is dedicated to single/multi namespace operators %s ", strings.Join(singleOrMultiNamespaceOperators, ", ")) check.LogInfo(msg) compliantObjects = append(compliantObjects, testhelper.NewNamespacedReportObject(msg, testhelper.Namespace, true, operatorNamespace)) } else { - msg := "Namespace is non-compliant operator namespace, containing non-operator, invalid installed or not installed operators " nonCompliantNs := testhelper.NewNamespacedReportObject(msg, testhelper.Namespace, false, operatorNamespace) @@ -232,21 +228,7 @@ func testOperatorCrdOpenAPISpec(check *checksdb.Check, env *provider.TestEnviron var nonCompliantObjects []*testhelper.ReportObject for _, crd := range env.Crds { - isCrdDefinedWithOpenAPI3Schema := false - - for _, version := range crd.Spec.Versions { - crdSchema := version.Schema.String() - - containsOpenAPIV3SchemaSubstr := strings.Contains(strings.ToLower(crdSchema), - strings.ToLower(testhelper.OpenAPIV3Schema)) - - if containsOpenAPIV3SchemaSubstr { - isCrdDefinedWithOpenAPI3Schema = true - break - } - } - - if isCrdDefinedWithOpenAPI3Schema { + if openapi.IsCRDDefinedWithOpenAPI3Schema(crd) { check.LogInfo("Operator CRD %s is defined with OpenAPIV3 schema ", crd.Name) compliantObjects = append(compliantObjects, testhelper.NewOperatorReportObject(crd.Namespace, crd.Name, "Operator CRD is defined with OpenAPIV3 schema ", true).AddField(testhelper.OpenAPIV3Schema, crd.Name)) @@ -319,39 +301,9 @@ func testOperatorInstallationAccessToSCC(check *checksdb.Check, env *provider.Te } // Fails in case any cluster permission has a rule that refers to securitycontextconstraints. - badRuleFound := false - for permissionIndex := range clusterPermissions { - permission := &clusterPermissions[permissionIndex] - for ruleIndex := range permission.Rules { - rule := &permission.Rules[ruleIndex] - - // Check whether the rule is for the security api group. - securityGroupFound := false - for _, group := range rule.APIGroups { - if group == "*" || group == "security.openshift.io" { - securityGroupFound = true - break - } - } - - if !securityGroupFound { - continue - } - - // Now check whether it grants some access to securitycontextconstraint resources. - for _, resource := range rule.Resources { - if resource == "*" || resource == "securitycontextconstraints" { - check.LogInfo("Operator %s has a rule (index %d) for service account %s to access cluster SCCs", - operator, ruleIndex, permission.ServiceAccountName) - // Keep reviewing other permissions' rules so we can log all the failing ones in the claim file. - badRuleFound = true - break - } - } - } - } - - if badRuleFound { + if access.PermissionsHaveBadRule(clusterPermissions) { + check.LogInfo("Operator %s has a rule for a service account to access cluster SCCs", + operator) nonCompliantObjects = append(nonCompliantObjects, testhelper.NewOperatorReportObject(operator.Namespace, operator.Name, "One or more RBAC rules for Security Context Constraints found in CSV", false)) } else { compliantObjects = append(compliantObjects, testhelper.NewOperatorReportObject(operator.Namespace, operator.Name, "No RBAC rules for Security Context Constraints found in CSV", true)) @@ -393,17 +345,17 @@ func testOperatorSingleCrdOwner(check *checksdb.Check, env *provider.TestEnviron ownedCrds := operator.Csv.Spec.CustomResourceDefinitions.Owned // Helper map to filter out different versions of the same CRD name. - uniqueOnwnedCrds := map[string]struct{}{} + uniqueOwnedCrds := map[string]struct{}{} for j := range ownedCrds { - uniqueOnwnedCrds[ownedCrds[j].Name] = struct{}{} + uniqueOwnedCrds[ownedCrds[j].Name] = struct{}{} } // Now we can append the operator as CRD owner - for crdName := range uniqueOnwnedCrds { + for crdName := range uniqueOwnedCrds { crdOwners[crdName] = append(crdOwners[crdName], operator.Name) } - check.LogInfo("CRDs owned by operator %s: %+v", operator.Name, uniqueOnwnedCrds) + check.LogInfo("CRDs owned by operator %s: %+v", operator.Name, uniqueOwnedCrds) } // Flag those that are owned by more than one operator @@ -479,27 +431,8 @@ func testMultipleSameOperators(check *checksdb.Check, env *provider.TestEnvironm check.LogDebug("Checking operator %q", op.Name) check.LogDebug("Number of operators to check %s against: %d", op.Name, len(env.AllOperators)) for _, op2 := range env.AllOperators { - check.LogDebug("Comparing operator %q with operator %q", op.Name, op2.Name) - - // Retrieve the version from each CSV - csv1Version := op.Csv.Spec.Version.String() - csv2Version := op2.Csv.Spec.Version.String() - - log.Debug("CSV1 Version: %s", csv1Version) - log.Debug("CSV2 Version: %s", csv2Version) - - // Strip the version from the CSV name by removing the suffix (which should be the version) - csv1Name := strings.TrimSuffix(op.Csv.Name, ".v"+csv1Version) - csv2Name := strings.TrimSuffix(op2.Csv.Name, ".v"+csv2Version) - - check.LogDebug("Comparing CSV names %q and %q", csv1Name, csv2Name) - - // The CSV name should be the same, but the version should be different - // if the operator is installed more than once. - if op.Csv != nil && op2.Csv != nil && - csv1Name == csv2Name && - csv1Version != csv2Version { - check.LogError("Operator %q is installed more than once", op.Name) + // Check if the operator is installed more than once. + if OperatorInstalledMoreThanOnce(op, op2) { nonCompliantObjects = append(nonCompliantObjects, testhelper.NewOperatorReportObject( op.Namespace, op.Name, "Operator is installed more than once", false)) break