From f508c3fc85eeb647a6d14812588a822d34115d07 Mon Sep 17 00:00:00 2001 From: Per Goncalves da Silva Date: Mon, 31 Mar 2025 13:50:17 +0200 Subject: [PATCH 1/3] Add CRDs field to RegistryV1 struct Signed-off-by: Per Goncalves da Silva --- .../operator-controller/rukpak/convert/registryv1.go | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/internal/operator-controller/rukpak/convert/registryv1.go b/internal/operator-controller/rukpak/convert/registryv1.go index 155b86be8..289ac6648 100644 --- a/internal/operator-controller/rukpak/convert/registryv1.go +++ b/internal/operator-controller/rukpak/convert/registryv1.go @@ -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" @@ -32,6 +33,7 @@ import ( type RegistryV1 struct { PackageName string CSV v1alpha1.ClusterServiceVersion + CRDs []apiextensionsv1.CustomResourceDefinition Others []unstructured.Unstructured } @@ -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)) } @@ -362,6 +370,9 @@ func Convert(in RegistryV1, installNamespace string, targetNamespaces []string) obj := obj objs = append(objs, &obj) } + for _, obj := range in.CRDs { + objs = append(objs, &obj) + } for _, obj := range in.Others { obj := obj supported, namespaced := registrybundle.IsSupported(obj.GetKind()) From a699574cdb01b0c21abd23034fb844db6c3b0ef9 Mon Sep 17 00:00:00 2001 From: Per Goncalves da Silva Date: Mon, 31 Mar 2025 13:50:37 +0200 Subject: [PATCH 2/3] Add registry+v1 bundlvalidators Signed-off-by: Per Goncalves da Silva --- .../rukpak/convert/validator.go | 90 +++++++ .../rukpak/convert/validator_test.go | 224 ++++++++++++++++++ 2 files changed, 314 insertions(+) create mode 100644 internal/operator-controller/rukpak/convert/validator.go create mode 100644 internal/operator-controller/rukpak/convert/validator_test.go diff --git a/internal/operator-controller/rukpak/convert/validator.go b/internal/operator-controller/rukpak/convert/validator.go new file mode 100644 index 000000000..e95842f2b --- /dev/null +++ b/internal/operator-controller/rukpak/convert/validator.go @@ -0,0 +1,90 @@ +package convert + +import ( + "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 errs +} + +func NewBundleValidator() 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 + return BundleValidator{ + 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 +} diff --git a/internal/operator-controller/rukpak/convert/validator_test.go b/internal/operator-controller/rukpak/convert/validator_test.go new file mode 100644 index 000000000..b781d7ab4 --- /dev/null +++ b/internal/operator-controller/rukpak/convert/validator_test.go @@ -0,0 +1,224 @@ +package convert_test + +import ( + "errors" + "reflect" + "testing" + + "github.com/stretchr/testify/require" + apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + "github.com/operator-framework/api/pkg/operators/v1alpha1" + + "github.com/operator-framework/operator-controller/internal/operator-controller/rukpak/convert" +) + +func Test_BundleValidatorHasAllValidationFns(t *testing.T) { + expectedValidationFns := []func(v1 *convert.RegistryV1) []error{ + convert.CheckDeploymentSpecUniqueness, + convert.CheckCRDResourceUniqueness, + convert.CheckOwnedCRDExistence, + } + actualValidationFns := convert.NewBundleValidator() + + require.Equal(t, len(expectedValidationFns), len(actualValidationFns)) + for i := range expectedValidationFns { + require.Equal(t, reflect.ValueOf(expectedValidationFns[i]).Pointer(), reflect.ValueOf(actualValidationFns[i]).Pointer(), "bundle validator has unexpected validation function") + } +} + +func Test_BundleValidatorCallsAllValidationFnsInOrder(t *testing.T) { + actual := "" + validator := convert.BundleValidator{ + func(v1 *convert.RegistryV1) []error { + actual += "h" + return nil + }, + func(v1 *convert.RegistryV1) []error { + actual += "i" + return nil + }, + } + require.Empty(t, validator.Validate(nil)) + require.Equal(t, "hi", actual) +} + +func Test_CheckDeploymentSpecUniqueness(t *testing.T) { + for _, tc := range []struct { + name string + bundle *convert.RegistryV1 + expectedErrs []error + }{ + { + name: "accepts bundles with unique deployment strategy spec names", + bundle: &convert.RegistryV1{ + CSV: MakeCSV( + WithStrategyDeploymentSpecs( + v1alpha1.StrategyDeploymentSpec{Name: "test-deployment-one"}, + v1alpha1.StrategyDeploymentSpec{Name: "test-deployment-two"}, + ), + ), + }, + }, { + name: "rejects bundles with duplicate deployment strategy spec names", + bundle: &convert.RegistryV1{ + CSV: MakeCSV( + WithStrategyDeploymentSpecs( + v1alpha1.StrategyDeploymentSpec{Name: "test-deployment-one"}, + v1alpha1.StrategyDeploymentSpec{Name: "test-deployment-two"}, + v1alpha1.StrategyDeploymentSpec{Name: "test-deployment-one"}, + ), + ), + }, + expectedErrs: []error{ + errors.New("cluster service version contains duplicate strategy deployment spec 'test-deployment-one'"), + }, + }, { + name: "errors are ordered by deployment strategy spec name", + bundle: &convert.RegistryV1{ + CSV: MakeCSV( + WithStrategyDeploymentSpecs( + v1alpha1.StrategyDeploymentSpec{Name: "test-deployment-a"}, + v1alpha1.StrategyDeploymentSpec{Name: "test-deployment-b"}, + v1alpha1.StrategyDeploymentSpec{Name: "test-deployment-c"}, + v1alpha1.StrategyDeploymentSpec{Name: "test-deployment-b"}, + v1alpha1.StrategyDeploymentSpec{Name: "test-deployment-a"}, + ), + ), + }, + expectedErrs: []error{ + errors.New("cluster service version contains duplicate strategy deployment spec 'test-deployment-a'"), + errors.New("cluster service version contains duplicate strategy deployment spec 'test-deployment-b'"), + }, + }, + } { + t.Run(tc.name, func(t *testing.T) { + errs := convert.CheckDeploymentSpecUniqueness(tc.bundle) + require.Equal(t, tc.expectedErrs, errs) + }) + } +} + +func Test_CRDResourceUniqueness(t *testing.T) { + for _, tc := range []struct { + name string + bundle *convert.RegistryV1 + expectedErrs []error + }{ + { + name: "accepts bundles with unique custom resource definition resources", + bundle: &convert.RegistryV1{ + CRDs: []apiextensionsv1.CustomResourceDefinition{ + {ObjectMeta: metav1.ObjectMeta{Name: "a.crd.something"}}, + {ObjectMeta: metav1.ObjectMeta{Name: "b.crd.something"}}, + }, + }, + }, { + name: "rejects bundles with duplicate custom resource definition resources", + bundle: &convert.RegistryV1{CRDs: []apiextensionsv1.CustomResourceDefinition{ + {ObjectMeta: metav1.ObjectMeta{Name: "a.crd.something"}}, + {ObjectMeta: metav1.ObjectMeta{Name: "a.crd.something"}}, + }}, + expectedErrs: []error{ + errors.New("bundle contains duplicate custom resource definition 'a.crd.something'"), + }, + }, { + name: "errors are ordered by custom resource definition name", + bundle: &convert.RegistryV1{CRDs: []apiextensionsv1.CustomResourceDefinition{ + {ObjectMeta: metav1.ObjectMeta{Name: "c.crd.something"}}, + {ObjectMeta: metav1.ObjectMeta{Name: "c.crd.something"}}, + {ObjectMeta: metav1.ObjectMeta{Name: "a.crd.something"}}, + {ObjectMeta: metav1.ObjectMeta{Name: "a.crd.something"}}, + }}, + expectedErrs: []error{ + errors.New("bundle contains duplicate custom resource definition 'a.crd.something'"), + errors.New("bundle contains duplicate custom resource definition 'c.crd.something'"), + }, + }, + } { + t.Run(tc.name, func(t *testing.T) { + err := convert.CheckCRDResourceUniqueness(tc.bundle) + require.Equal(t, tc.expectedErrs, err) + }) + } +} + +func Test_CheckOwnedCRDExistence(t *testing.T) { + for _, tc := range []struct { + name string + bundle *convert.RegistryV1 + expectedErrs []error + }{ + { + name: "accepts bundles with existing owned custom resource definition resources", + bundle: &convert.RegistryV1{ + CRDs: []apiextensionsv1.CustomResourceDefinition{ + {ObjectMeta: metav1.ObjectMeta{Name: "a.crd.something"}}, + {ObjectMeta: metav1.ObjectMeta{Name: "b.crd.something"}}, + }, + CSV: MakeCSV( + WithOwnedCRDs( + v1alpha1.CRDDescription{Name: "a.crd.something"}, + v1alpha1.CRDDescription{Name: "b.crd.something"}, + ), + ), + }, + }, { + name: "rejects bundles with missing owned custom resource definition resources", + bundle: &convert.RegistryV1{ + CRDs: []apiextensionsv1.CustomResourceDefinition{}, + CSV: MakeCSV( + WithOwnedCRDs(v1alpha1.CRDDescription{Name: "a.crd.something"}), + ), + }, + expectedErrs: []error{ + errors.New("cluster service definition references owned custom resource definition 'a.crd.something' not found in bundle"), + }, + }, { + name: "errors are ordered by owned custom resource definition name", + bundle: &convert.RegistryV1{ + CRDs: []apiextensionsv1.CustomResourceDefinition{}, + CSV: MakeCSV( + WithOwnedCRDs( + v1alpha1.CRDDescription{Name: "a.crd.something"}, + v1alpha1.CRDDescription{Name: "c.crd.something"}, + v1alpha1.CRDDescription{Name: "b.crd.something"}, + ), + ), + }, + expectedErrs: []error{ + errors.New("cluster service definition references owned custom resource definition 'a.crd.something' not found in bundle"), + errors.New("cluster service definition references owned custom resource definition 'b.crd.something' not found in bundle"), + errors.New("cluster service definition references owned custom resource definition 'c.crd.something' not found in bundle"), + }, + }, + } { + t.Run(tc.name, func(t *testing.T) { + errs := convert.CheckOwnedCRDExistence(tc.bundle) + require.Equal(t, tc.expectedErrs, errs) + }) + } +} + +type CSVOption func(version *v1alpha1.ClusterServiceVersion) + +func WithStrategyDeploymentSpecs(strategyDeploymentSpecs ...v1alpha1.StrategyDeploymentSpec) CSVOption { + return func(csv *v1alpha1.ClusterServiceVersion) { + csv.Spec.InstallStrategy.StrategySpec.DeploymentSpecs = strategyDeploymentSpecs + } +} + +func WithOwnedCRDs(crdDesc ...v1alpha1.CRDDescription) CSVOption { + return func(csv *v1alpha1.ClusterServiceVersion) { + csv.Spec.CustomResourceDefinitions.Owned = crdDesc + } +} + +func MakeCSV(opts ...CSVOption) v1alpha1.ClusterServiceVersion { + csv := v1alpha1.ClusterServiceVersion{} + for _, opt := range opts { + opt(&csv) + } + return csv +} From 45b1610ed9349440d8e1d20feb2487c593eec166 Mon Sep 17 00:00:00 2001 From: Per Goncalves da Silva Date: Tue, 1 Apr 2025 12:56:54 +0200 Subject: [PATCH 3/3] Refactor converter to use validator Signed-off-by: Per Goncalves da Silva --- .../rukpak/convert/registryv1.go | 42 ++++++++++++------- .../rukpak/convert/registryv1_test.go | 41 +++++++++++++----- .../rukpak/convert/validator.go | 15 ++++--- .../rukpak/convert/validator_test.go | 36 ++++++++-------- test/convert/generate-manifests.go | 2 +- 5 files changed, 83 insertions(+), 53 deletions(-) diff --git a/internal/operator-controller/rukpak/convert/registryv1.go b/internal/operator-controller/rukpak/convert/registryv1.go index 289ac6648..ce599d46f 100644 --- a/internal/operator-controller/rukpak/convert/registryv1.go +++ b/internal/operator-controller/rukpak/convert/registryv1.go @@ -47,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 } @@ -230,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)) } @@ -255,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{ @@ -300,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 @@ -328,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 } @@ -339,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 } @@ -370,10 +378,10 @@ func Convert(in RegistryV1, installNamespace string, targetNamespaces []string) obj := obj objs = append(objs, &obj) } - for _, obj := range in.CRDs { + for _, obj := range rv1.CRDs { objs = append(objs, &obj) } - for _, obj := range in.Others { + for _, obj := range rv1.Others { obj := obj supported, namespaced := registrybundle.IsSupported(obj.GetKind()) if !supported { @@ -391,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) { diff --git a/internal/operator-controller/rukpak/convert/registryv1_test.go b/internal/operator-controller/rukpak/convert/registryv1_test.go index 20ea7fc88..34f7722d0 100644 --- a/internal/operator-controller/rukpak/convert/registryv1_test.go +++ b/internal/operator-controller/rukpak/convert/registryv1_test.go @@ -1,6 +1,7 @@ package convert_test import ( + "errors" "fmt" "io/fs" "os" @@ -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 @@ -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") @@ -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") @@ -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) @@ -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") @@ -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") @@ -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") @@ -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") @@ -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") @@ -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) }) @@ -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) @@ -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) diff --git a/internal/operator-controller/rukpak/convert/validator.go b/internal/operator-controller/rukpak/convert/validator.go index e95842f2b..73060faa2 100644 --- a/internal/operator-controller/rukpak/convert/validator.go +++ b/internal/operator-controller/rukpak/convert/validator.go @@ -1,6 +1,7 @@ package convert import ( + "errors" "fmt" "slices" @@ -9,23 +10,21 @@ import ( type BundleValidator []func(v1 *RegistryV1) []error -func (v BundleValidator) Validate(rv1 *RegistryV1) []error { +func (v BundleValidator) Validate(rv1 *RegistryV1) error { var errs []error for _, validator := range v { errs = append(errs, validator(rv1)...) } - return errs + return errors.Join(errs...) } -func NewBundleValidator() BundleValidator { +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 - return BundleValidator{ - CheckDeploymentSpecUniqueness, - CheckCRDResourceUniqueness, - CheckOwnedCRDExistence, - } + CheckDeploymentSpecUniqueness, + CheckCRDResourceUniqueness, + CheckOwnedCRDExistence, } // CheckDeploymentSpecUniqueness checks that each strategy deployment spec in the csv has a unique name. diff --git a/internal/operator-controller/rukpak/convert/validator_test.go b/internal/operator-controller/rukpak/convert/validator_test.go index b781d7ab4..7b218fba3 100644 --- a/internal/operator-controller/rukpak/convert/validator_test.go +++ b/internal/operator-controller/rukpak/convert/validator_test.go @@ -20,7 +20,7 @@ func Test_BundleValidatorHasAllValidationFns(t *testing.T) { convert.CheckCRDResourceUniqueness, convert.CheckOwnedCRDExistence, } - actualValidationFns := convert.NewBundleValidator() + actualValidationFns := convert.RegistryV1BundleValidator require.Equal(t, len(expectedValidationFns), len(actualValidationFns)) for i := range expectedValidationFns { @@ -40,7 +40,7 @@ func Test_BundleValidatorCallsAllValidationFnsInOrder(t *testing.T) { return nil }, } - require.Empty(t, validator.Validate(nil)) + require.NoError(t, validator.Validate(nil)) require.Equal(t, "hi", actual) } @@ -53,8 +53,8 @@ func Test_CheckDeploymentSpecUniqueness(t *testing.T) { { name: "accepts bundles with unique deployment strategy spec names", bundle: &convert.RegistryV1{ - CSV: MakeCSV( - WithStrategyDeploymentSpecs( + CSV: makeCSV( + withStrategyDeploymentSpecs( v1alpha1.StrategyDeploymentSpec{Name: "test-deployment-one"}, v1alpha1.StrategyDeploymentSpec{Name: "test-deployment-two"}, ), @@ -63,8 +63,8 @@ func Test_CheckDeploymentSpecUniqueness(t *testing.T) { }, { name: "rejects bundles with duplicate deployment strategy spec names", bundle: &convert.RegistryV1{ - CSV: MakeCSV( - WithStrategyDeploymentSpecs( + CSV: makeCSV( + withStrategyDeploymentSpecs( v1alpha1.StrategyDeploymentSpec{Name: "test-deployment-one"}, v1alpha1.StrategyDeploymentSpec{Name: "test-deployment-two"}, v1alpha1.StrategyDeploymentSpec{Name: "test-deployment-one"}, @@ -77,8 +77,8 @@ func Test_CheckDeploymentSpecUniqueness(t *testing.T) { }, { name: "errors are ordered by deployment strategy spec name", bundle: &convert.RegistryV1{ - CSV: MakeCSV( - WithStrategyDeploymentSpecs( + CSV: makeCSV( + withStrategyDeploymentSpecs( v1alpha1.StrategyDeploymentSpec{Name: "test-deployment-a"}, v1alpha1.StrategyDeploymentSpec{Name: "test-deployment-b"}, v1alpha1.StrategyDeploymentSpec{Name: "test-deployment-c"}, @@ -157,8 +157,8 @@ func Test_CheckOwnedCRDExistence(t *testing.T) { {ObjectMeta: metav1.ObjectMeta{Name: "a.crd.something"}}, {ObjectMeta: metav1.ObjectMeta{Name: "b.crd.something"}}, }, - CSV: MakeCSV( - WithOwnedCRDs( + CSV: makeCSV( + withOwnedCRDs( v1alpha1.CRDDescription{Name: "a.crd.something"}, v1alpha1.CRDDescription{Name: "b.crd.something"}, ), @@ -168,8 +168,8 @@ func Test_CheckOwnedCRDExistence(t *testing.T) { name: "rejects bundles with missing owned custom resource definition resources", bundle: &convert.RegistryV1{ CRDs: []apiextensionsv1.CustomResourceDefinition{}, - CSV: MakeCSV( - WithOwnedCRDs(v1alpha1.CRDDescription{Name: "a.crd.something"}), + CSV: makeCSV( + withOwnedCRDs(v1alpha1.CRDDescription{Name: "a.crd.something"}), ), }, expectedErrs: []error{ @@ -179,8 +179,8 @@ func Test_CheckOwnedCRDExistence(t *testing.T) { name: "errors are ordered by owned custom resource definition name", bundle: &convert.RegistryV1{ CRDs: []apiextensionsv1.CustomResourceDefinition{}, - CSV: MakeCSV( - WithOwnedCRDs( + CSV: makeCSV( + withOwnedCRDs( v1alpha1.CRDDescription{Name: "a.crd.something"}, v1alpha1.CRDDescription{Name: "c.crd.something"}, v1alpha1.CRDDescription{Name: "b.crd.something"}, @@ -201,21 +201,21 @@ func Test_CheckOwnedCRDExistence(t *testing.T) { } } -type CSVOption func(version *v1alpha1.ClusterServiceVersion) +type csvOption func(version *v1alpha1.ClusterServiceVersion) -func WithStrategyDeploymentSpecs(strategyDeploymentSpecs ...v1alpha1.StrategyDeploymentSpec) CSVOption { +func withStrategyDeploymentSpecs(strategyDeploymentSpecs ...v1alpha1.StrategyDeploymentSpec) csvOption { return func(csv *v1alpha1.ClusterServiceVersion) { csv.Spec.InstallStrategy.StrategySpec.DeploymentSpecs = strategyDeploymentSpecs } } -func WithOwnedCRDs(crdDesc ...v1alpha1.CRDDescription) CSVOption { +func withOwnedCRDs(crdDesc ...v1alpha1.CRDDescription) csvOption { return func(csv *v1alpha1.ClusterServiceVersion) { csv.Spec.CustomResourceDefinitions.Owned = crdDesc } } -func MakeCSV(opts ...CSVOption) v1alpha1.ClusterServiceVersion { +func makeCSV(opts ...csvOption) v1alpha1.ClusterServiceVersion { csv := v1alpha1.ClusterServiceVersion{} for _, opt := range opts { opt(&csv) diff --git a/test/convert/generate-manifests.go b/test/convert/generate-manifests.go index 36d859585..c80d0c06f 100644 --- a/test/convert/generate-manifests.go +++ b/test/convert/generate-manifests.go @@ -67,7 +67,7 @@ func generateManifests(outputPath, bundleDir, installNamespace, watchNamespace s } // Convert RegistryV1 to plain manifests - plain, err := convert.Convert(regv1, installNamespace, []string{watchNamespace}) + plain, err := convert.PlainConverter.Convert(regv1, installNamespace, []string{watchNamespace}) if err != nil { return fmt.Errorf("error converting registry+v1 bundle: %w", err) }