Skip to content
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

✨ Add registry+v1 bundle validation framework #1885

Merged
merged 3 commits into from
Apr 4, 2025
Merged

✨ Add registry+v1 bundle validation framework #1885

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f508c3f
Add CRDs field to RegistryV1 struct
Mar 31, 2025
a699574
Add registry+v1 bundlvalidators
Mar 31, 2025
45b1610
Refactor converter to use validator
Apr 1, 2025
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
51 changes: 37 additions & 14 deletions internal/operator-controller/rukpak/convert/registryv1.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import (
appsv1 "k8s.io/api/apps/v1"
corev1 "k8s.io/api/core/v1"
rbacv1 "k8s.io/api/rbac/v1"
apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/apimachinery/pkg/runtime"
Expand All @@ -32,6 +33,7 @@ import (
type RegistryV1 struct {
PackageName string
CSV v1alpha1.ClusterServiceVersion
CRDs []apiextensionsv1.CustomResourceDefinition
Others []unstructured.Unstructured
}

Expand All @@ -45,7 +47,7 @@ func RegistryV1ToHelmChart(rv1 fs.FS, installNamespace string, watchNamespace st
return nil, err
}

plain, err := Convert(reg, installNamespace, []string{watchNamespace})
plain, err := PlainConverter.Convert(reg, installNamespace, []string{watchNamespace})
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -121,6 +123,12 @@ func ParseFS(rv1 fs.FS) (RegistryV1, error) {
}
reg.CSV = csv
foundCSV = true
case "CustomResourceDefinition":
crd := apiextensionsv1.CustomResourceDefinition{}
if err := runtime.DefaultUnstructuredConverter.FromUnstructured(info.Object.(*unstructured.Unstructured).Object, &crd); err != nil {
return err
}
reg.CRDs = append(reg.CRDs, crd)
default:
reg.Others = append(reg.Others, *info.Object.(*unstructured.Unstructured))
}
Expand Down Expand Up @@ -222,15 +230,23 @@ func saNameOrDefault(saName string) string {
return saName
}

func Convert(in RegistryV1, installNamespace string, targetNamespaces []string) (*Plain, error) {
type Converter struct {
BundleValidator BundleValidator
}

func (c Converter) Convert(rv1 RegistryV1, installNamespace string, targetNamespaces []string) (*Plain, error) {
if err := c.BundleValidator.Validate(&rv1); err != nil {
return nil, err
}

if installNamespace == "" {
installNamespace = in.CSV.Annotations["operatorframework.io/suggested-namespace"]
installNamespace = rv1.CSV.Annotations["operatorframework.io/suggested-namespace"]
}
if installNamespace == "" {
installNamespace = fmt.Sprintf("%s-system", in.PackageName)
installNamespace = fmt.Sprintf("%s-system", rv1.PackageName)
}
supportedInstallModes := sets.New[string]()
for _, im := range in.CSV.Spec.InstallModes {
for _, im := range rv1.CSV.Spec.InstallModes {
if im.Supported {
supportedInstallModes.Insert(string(im.Type))
}
Expand All @@ -247,18 +263,18 @@ func Convert(in RegistryV1, installNamespace string, targetNamespaces []string)
return nil, err
}

if len(in.CSV.Spec.APIServiceDefinitions.Owned) > 0 {
if len(rv1.CSV.Spec.APIServiceDefinitions.Owned) > 0 {
return nil, fmt.Errorf("apiServiceDefintions are not supported")
}

if len(in.CSV.Spec.WebhookDefinitions) > 0 {
if len(rv1.CSV.Spec.WebhookDefinitions) > 0 {
return nil, fmt.Errorf("webhookDefinitions are not supported")
}

deployments := []appsv1.Deployment{}
serviceAccounts := map[string]corev1.ServiceAccount{}
for _, depSpec := range in.CSV.Spec.InstallStrategy.StrategySpec.DeploymentSpecs {
annotations := util.MergeMaps(in.CSV.Annotations, depSpec.Spec.Template.Annotations)
for _, depSpec := range rv1.CSV.Spec.InstallStrategy.StrategySpec.DeploymentSpecs {
annotations := util.MergeMaps(rv1.CSV.Annotations, depSpec.Spec.Template.Annotations)
annotations["olm.targetNamespaces"] = strings.Join(targetNamespaces, ",")
depSpec.Spec.Template.Annotations = annotations
deployments = append(deployments, appsv1.Deployment{
Expand Down Expand Up @@ -292,8 +308,8 @@ func Convert(in RegistryV1, installNamespace string, targetNamespaces []string)
clusterRoles := []rbacv1.ClusterRole{}
clusterRoleBindings := []rbacv1.ClusterRoleBinding{}

permissions := in.CSV.Spec.InstallStrategy.StrategySpec.Permissions
clusterPermissions := in.CSV.Spec.InstallStrategy.StrategySpec.ClusterPermissions
permissions := rv1.CSV.Spec.InstallStrategy.StrategySpec.Permissions
clusterPermissions := rv1.CSV.Spec.InstallStrategy.StrategySpec.ClusterPermissions
allPermissions := append(permissions, clusterPermissions...)

// Create all the service accounts
Expand All @@ -320,7 +336,7 @@ func Convert(in RegistryV1, installNamespace string, targetNamespaces []string)
for _, ns := range targetNamespaces {
for _, permission := range permissions {
saName := saNameOrDefault(permission.ServiceAccountName)
name, err := generateName(fmt.Sprintf("%s-%s", in.CSV.Name, saName), permission)
name, err := generateName(fmt.Sprintf("%s-%s", rv1.CSV.Name, saName), permission)
if err != nil {
return nil, err
}
Expand All @@ -331,7 +347,7 @@ func Convert(in RegistryV1, installNamespace string, targetNamespaces []string)

for _, permission := range clusterPermissions {
saName := saNameOrDefault(permission.ServiceAccountName)
name, err := generateName(fmt.Sprintf("%s-%s", in.CSV.Name, saName), permission)
name, err := generateName(fmt.Sprintf("%s-%s", rv1.CSV.Name, saName), permission)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -362,7 +378,10 @@ func Convert(in RegistryV1, installNamespace string, targetNamespaces []string)
obj := obj
objs = append(objs, &obj)
}
for _, obj := range in.Others {
for _, obj := range rv1.CRDs {
objs = append(objs, &obj)
}
for _, obj := range rv1.Others {
obj := obj
supported, namespaced := registrybundle.IsSupported(obj.GetKind())
if !supported {
Expand All @@ -380,6 +399,10 @@ func Convert(in RegistryV1, installNamespace string, targetNamespaces []string)
return &Plain{Objects: objs}, nil
}

var PlainConverter = Converter{
BundleValidator: RegistryV1BundleValidator,
}

const maxNameLength = 63

func generateName(base string, o interface{}) (string, error) {
Expand Down
41 changes: 30 additions & 11 deletions internal/operator-controller/rukpak/convert/registryv1_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package convert_test

import (
"errors"
"fmt"
"io/fs"
"os"
Expand Down Expand Up @@ -52,6 +53,24 @@ func getCsvAndService() (v1alpha1.ClusterServiceVersion, corev1.Service) {
return csv, svc
}

func TestConverterValidatesBundle(t *testing.T) {
converter := convert.Converter{
BundleValidator: []func(rv1 *convert.RegistryV1) []error{
func(rv1 *convert.RegistryV1) []error {
return []error{errors.New("test error")}
},
},
}

_, err := converter.Convert(convert.RegistryV1{}, "installNamespace", []string{"watchNamespace"})
require.Error(t, err)
require.Contains(t, err.Error(), "test error")
}

func TestPlainConverterUsedRegV1Validator(t *testing.T) {
require.Equal(t, convert.RegistryV1BundleValidator, convert.PlainConverter.BundleValidator)
}

func TestRegistryV1SuiteNamespaceNotAvailable(t *testing.T) {
var targetNamespaces []string

Expand All @@ -70,7 +89,7 @@ func TestRegistryV1SuiteNamespaceNotAvailable(t *testing.T) {
}

t.Log("By converting to plain")
plainBundle, err := convert.Convert(registryv1Bundle, installNamespace, targetNamespaces)
plainBundle, err := convert.PlainConverter.Convert(registryv1Bundle, installNamespace, targetNamespaces)
require.NoError(t, err)

t.Log("By verifying if plain bundle has required objects")
Expand Down Expand Up @@ -104,7 +123,7 @@ func TestRegistryV1SuiteNamespaceAvailable(t *testing.T) {
}

t.Log("By converting to plain")
plainBundle, err := convert.Convert(registryv1Bundle, installNamespace, targetNamespaces)
plainBundle, err := convert.PlainConverter.Convert(registryv1Bundle, installNamespace, targetNamespaces)
require.NoError(t, err)

t.Log("By verifying if plain bundle has required objects")
Expand Down Expand Up @@ -145,7 +164,7 @@ func TestRegistryV1SuiteNamespaceUnsupportedKind(t *testing.T) {
}

t.Log("By converting to plain")
plainBundle, err := convert.Convert(registryv1Bundle, installNamespace, targetNamespaces)
plainBundle, err := convert.PlainConverter.Convert(registryv1Bundle, installNamespace, targetNamespaces)
require.Error(t, err)
require.ErrorContains(t, err, "bundle contains unsupported resource")
require.Nil(t, plainBundle)
Expand Down Expand Up @@ -179,7 +198,7 @@ func TestRegistryV1SuiteNamespaceClusterScoped(t *testing.T) {
}

t.Log("By converting to plain")
plainBundle, err := convert.Convert(registryv1Bundle, installNamespace, targetNamespaces)
plainBundle, err := convert.PlainConverter.Convert(registryv1Bundle, installNamespace, targetNamespaces)
require.NoError(t, err)

t.Log("By verifying if plain bundle has required objects")
Expand Down Expand Up @@ -266,7 +285,7 @@ func TestRegistryV1SuiteGenerateAllNamespace(t *testing.T) {
}

t.Log("By converting to plain")
plainBundle, err := convert.Convert(registryv1Bundle, installNamespace, watchNamespaces)
plainBundle, err := convert.PlainConverter.Convert(registryv1Bundle, installNamespace, watchNamespaces)
require.NoError(t, err)

t.Log("By verifying if plain bundle has required objects")
Expand Down Expand Up @@ -299,7 +318,7 @@ func TestRegistryV1SuiteGenerateMultiNamespace(t *testing.T) {
}

t.Log("By converting to plain")
plainBundle, err := convert.Convert(registryv1Bundle, installNamespace, watchNamespaces)
plainBundle, err := convert.PlainConverter.Convert(registryv1Bundle, installNamespace, watchNamespaces)
require.NoError(t, err)

t.Log("By verifying if plain bundle has required objects")
Expand Down Expand Up @@ -332,7 +351,7 @@ func TestRegistryV1SuiteGenerateSingleNamespace(t *testing.T) {
}

t.Log("By converting to plain")
plainBundle, err := convert.Convert(registryv1Bundle, installNamespace, watchNamespaces)
plainBundle, err := convert.PlainConverter.Convert(registryv1Bundle, installNamespace, watchNamespaces)
require.NoError(t, err)

t.Log("By verifying if plain bundle has required objects")
Expand Down Expand Up @@ -365,7 +384,7 @@ func TestRegistryV1SuiteGenerateOwnNamespace(t *testing.T) {
}

t.Log("By converting to plain")
plainBundle, err := convert.Convert(registryv1Bundle, installNamespace, watchNamespaces)
plainBundle, err := convert.PlainConverter.Convert(registryv1Bundle, installNamespace, watchNamespaces)
require.NoError(t, err)

t.Log("By verifying if plain bundle has required objects")
Expand Down Expand Up @@ -470,7 +489,7 @@ func TestConvertInstallModeValidation(t *testing.T) {
}

t.Log("By converting to plain")
plainBundle, err := convert.Convert(registryv1Bundle, tc.installNamespace, tc.watchNamespaces)
plainBundle, err := convert.PlainConverter.Convert(registryv1Bundle, tc.installNamespace, tc.watchNamespaces)
require.Error(t, err)
require.Nil(t, plainBundle)
})
Expand Down Expand Up @@ -559,7 +578,7 @@ func TestRegistryV1SuiteGenerateNoWebhooks(t *testing.T) {
}

t.Log("By converting to plain")
plainBundle, err := convert.Convert(registryv1Bundle, installNamespace, watchNamespaces)
plainBundle, err := convert.PlainConverter.Convert(registryv1Bundle, installNamespace, watchNamespaces)
require.Error(t, err)
require.ErrorContains(t, err, "webhookDefinitions are not supported")
require.Nil(t, plainBundle)
Expand Down Expand Up @@ -590,7 +609,7 @@ func TestRegistryV1SuiteGenerateNoAPISerciceDefinitions(t *testing.T) {
}

t.Log("By converting to plain")
plainBundle, err := convert.Convert(registryv1Bundle, installNamespace, watchNamespaces)
plainBundle, err := convert.PlainConverter.Convert(registryv1Bundle, installNamespace, watchNamespaces)
require.Error(t, err)
require.ErrorContains(t, err, "apiServiceDefintions are not supported")
require.Nil(t, plainBundle)
Expand Down
89 changes: 89 additions & 0 deletions internal/operator-controller/rukpak/convert/validator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
package convert

import (
"errors"
"fmt"
"slices"

"k8s.io/apimachinery/pkg/util/sets"
)

type BundleValidator []func(v1 *RegistryV1) []error

func (v BundleValidator) Validate(rv1 *RegistryV1) error {
var errs []error
for _, validator := range v {
errs = append(errs, validator(rv1)...)
}
return errors.Join(errs...)
}

var RegistryV1BundleValidator = BundleValidator{
// NOTE: if you update this list, Test_BundleValidatorHasAllValidationFns will fail until
// you bring the same changes over to that test. This helps ensure all validation rules are executed
// while giving us the flexibility to test each validation function individually
CheckDeploymentSpecUniqueness,
CheckCRDResourceUniqueness,
CheckOwnedCRDExistence,
}

// CheckDeploymentSpecUniqueness checks that each strategy deployment spec in the csv has a unique name.
// Errors are sorted by deployment name.
func CheckDeploymentSpecUniqueness(rv1 *RegistryV1) []error {
deploymentNameSet := sets.Set[string]{}
duplicateDeploymentNames := sets.Set[string]{}
for _, dep := range rv1.CSV.Spec.InstallStrategy.StrategySpec.DeploymentSpecs {
if deploymentNameSet.Has(dep.Name) {
duplicateDeploymentNames.Insert(dep.Name)
}
deploymentNameSet.Insert(dep.Name)
}

//nolint:prealloc
var errs []error
for _, d := range slices.Sorted(slices.Values(duplicateDeploymentNames.UnsortedList())) {
errs = append(errs, fmt.Errorf("cluster service version contains duplicate strategy deployment spec '%s'", d))
}
return errs
}

// CheckOwnedCRDExistence checks bundle owned custom resource definitions declared in the csv exist in the bundle
func CheckOwnedCRDExistence(rv1 *RegistryV1) []error {
crdsNames := sets.Set[string]{}
for _, crd := range rv1.CRDs {
crdsNames.Insert(crd.Name)
}

//nolint:prealloc
var errs []error
missingCRDNames := sets.Set[string]{}
for _, crd := range rv1.CSV.Spec.CustomResourceDefinitions.Owned {
if !crdsNames.Has(crd.Name) {
missingCRDNames.Insert(crd.Name)
}
}

for _, crdName := range slices.Sorted(slices.Values(missingCRDNames.UnsortedList())) {
errs = append(errs, fmt.Errorf("cluster service definition references owned custom resource definition '%s' not found in bundle", crdName))
}
return errs
}

// CheckCRDResourceUniqueness checks that the bundle CRD names are unique
func CheckCRDResourceUniqueness(rv1 *RegistryV1) []error {
crdsNames := sets.Set[string]{}
duplicateCRDNames := sets.Set[string]{}
for _, crd := range rv1.CRDs {
if crdsNames.Has(crd.Name) {
duplicateCRDNames.Insert(crd.Name)
}
crdsNames.Insert(crd.Name)
}

//nolint:prealloc
var errs []error
for _, crdName := range slices.Sorted(slices.Values(duplicateCRDNames.UnsortedList())) {
errs = append(errs, fmt.Errorf("bundle contains duplicate custom resource definition '%s'", crdName))
}
return errs
}
Loading
Loading