diff --git a/internal/operator-controller/rukpak/convert/registryv1.go b/internal/operator-controller/rukpak/convert/registryv1.go index 155b86be8..ce599d46f 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 } @@ -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 } @@ -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)) } @@ -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)) } @@ -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{ @@ -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 @@ -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 } @@ -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 } @@ -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 { @@ -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) { diff --git a/internal/operator-controller/rukpak/convert/registryv1_test.go b/internal/operator-controller/rukpak/convert/registryv1_test.go index 20ea7fc88..bd2e1d8db 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.Converter{}.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 new file mode 100644 index 000000000..297b1e0e3 --- /dev/null +++ b/internal/operator-controller/rukpak/convert/validator.go @@ -0,0 +1,189 @@ +package convert + +import ( + "cmp" + "errors" + "fmt" + "maps" + "slices" + + "k8s.io/apimachinery/pkg/util/sets" + + "github.com/operator-framework/api/pkg/operators/v1alpha1" +) + +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, + CheckWebhookDeploymentReferentialIntegrity, + CheckWebhookNameUniqueness, + CheckConversionWebhookCRDReferences, +} + +// 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 +} + +// CheckWebhookDeploymentReferentialIntegrity checks that each webhook definition in the csv +// references an existing strategy deployment spec. Errors are sorted by strategy deployment spec name, +// webhook type, and webhook name. +func CheckWebhookDeploymentReferentialIntegrity(rv1 *RegistryV1) []error { + webhooksByDeployment := map[string][]v1alpha1.WebhookDescription{} + for _, wh := range rv1.CSV.Spec.WebhookDefinitions { + webhooksByDeployment[wh.DeploymentName] = append(webhooksByDeployment[wh.DeploymentName], wh) + } + + for _, depl := range rv1.CSV.Spec.InstallStrategy.StrategySpec.DeploymentSpecs { + delete(webhooksByDeployment, depl.Name) + } + + var errs []error + // Loop through sorted keys to keep error messages ordered by deployment name + for _, deploymentName := range slices.Sorted(maps.Keys(webhooksByDeployment)) { + webhookDefns := webhooksByDeployment[deploymentName] + slices.SortFunc(webhookDefns, func(a, b v1alpha1.WebhookDescription) int { + return cmp.Or(cmp.Compare(a.Type, b.Type), cmp.Compare(a.GenerateName, b.GenerateName)) + }) + for _, webhookDef := range webhookDefns { + errs = append(errs, fmt.Errorf("webhook '%s' of type '%s' references non-existent deployment '%s'", webhookDef.GenerateName, webhookDef.Type, webhookDef.DeploymentName)) + } + } + return errs +} + +// CheckWebhookNameUniqueness checks that each webhook definition of each type (validating, mutating, or conversion) +// has a unique name. Webhooks of different types can have the same name. Errors are sorted by webhook type +// and name. +func CheckWebhookNameUniqueness(rv1 *RegistryV1) []error { + webhookNameSetByType := map[v1alpha1.WebhookAdmissionType]sets.Set[string]{} + duplicateWebhooksByType := map[v1alpha1.WebhookAdmissionType]sets.Set[string]{} + for _, wh := range rv1.CSV.Spec.WebhookDefinitions { + if _, ok := webhookNameSetByType[wh.Type]; !ok { + webhookNameSetByType[wh.Type] = sets.Set[string]{} + } + if webhookNameSetByType[wh.Type].Has(wh.GenerateName) { + if _, ok := duplicateWebhooksByType[wh.Type]; !ok { + duplicateWebhooksByType[wh.Type] = sets.Set[string]{} + } + duplicateWebhooksByType[wh.Type].Insert(wh.GenerateName) + } + webhookNameSetByType[wh.Type].Insert(wh.GenerateName) + } + + var errs []error + for _, whType := range slices.Sorted(maps.Keys(duplicateWebhooksByType)) { + for _, webhookName := range slices.Sorted(slices.Values(duplicateWebhooksByType[whType].UnsortedList())) { + errs = append(errs, fmt.Errorf("duplicate webhook '%s' of type '%s'", webhookName, whType)) + } + } + return errs +} + +// CheckConversionWebhookCRDReferences checks defined conversion webhooks reference bundle owned CRDs. +// Errors are sorted by webhook name and CRD name. +func CheckConversionWebhookCRDReferences(rv1 *RegistryV1) []error { + //nolint:prealloc + var conversionWebhooks []v1alpha1.WebhookDescription + for _, wh := range rv1.CSV.Spec.WebhookDefinitions { + if wh.Type != v1alpha1.ConversionWebhook { + continue + } + conversionWebhooks = append(conversionWebhooks, wh) + } + + if len(conversionWebhooks) == 0 { + return nil + } + + ownedCRDNames := sets.Set[string]{} + for _, crd := range rv1.CSV.Spec.CustomResourceDefinitions.Owned { + ownedCRDNames.Insert(crd.Name) + } + + slices.SortFunc(conversionWebhooks, func(a, b v1alpha1.WebhookDescription) int { + return cmp.Compare(a.GenerateName, b.GenerateName) + }) + + var errs []error + for _, webhook := range conversionWebhooks { + webhookCRDs := webhook.ConversionCRDs + slices.Sort(webhookCRDs) + for _, crd := range webhookCRDs { + if !ownedCRDNames.Has(crd) { + errs = append(errs, fmt.Errorf("conversion webhook '%s' references custom resource definition '%s' not owned bundle", webhook.GenerateName, crd)) + } + } + } + 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..4ced1af15 --- /dev/null +++ b/internal/operator-controller/rukpak/convert/validator_test.go @@ -0,0 +1,621 @@ +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, + convert.CheckWebhookDeploymentReferentialIntegrity, + convert.CheckWebhookNameUniqueness, + convert.CheckConversionWebhookCRDReferences, + } + actualValidationFns := convert.RegistryV1BundleValidator + + 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.NoError(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) + }) + } +} + +func Test_CheckWebhookDeploymentReferentialIntegrity(t *testing.T) { + for _, tc := range []struct { + name string + bundle *convert.RegistryV1 + expectedErrs []error + }{ + { + name: "accepts bundles where webhook definitions reference existing strategy deployment specs", + bundle: &convert.RegistryV1{ + CSV: MakeCSV( + WithStrategyDeploymentSpecs( + v1alpha1.StrategyDeploymentSpec{Name: "test-deployment-one"}, + v1alpha1.StrategyDeploymentSpec{Name: "test-deployment-two"}, + ), + WithWebhookDefinitions( + v1alpha1.WebhookDescription{ + Type: v1alpha1.MutatingAdmissionWebhook, + GenerateName: "test-webhook", + DeploymentName: "test-deployment-one", + }, + ), + ), + }, + }, { + name: "rejects bundles with webhook definitions that reference non-existing strategy deployment specs", + bundle: &convert.RegistryV1{ + CSV: MakeCSV( + WithStrategyDeploymentSpecs( + v1alpha1.StrategyDeploymentSpec{Name: "test-deployment-one"}, + ), + WithWebhookDefinitions( + v1alpha1.WebhookDescription{ + Type: v1alpha1.ValidatingAdmissionWebhook, + GenerateName: "test-webhook", + DeploymentName: "test-deployment-two", + }, + ), + ), + }, + expectedErrs: []error{ + errors.New("webhook 'test-webhook' of type 'ValidatingAdmissionWebhook' references non-existent deployment 'test-deployment-two'"), + }, + }, { + name: "errors are ordered by deployment strategy spec name, webhook type, and webhook name", + bundle: &convert.RegistryV1{ + CSV: MakeCSV( + WithStrategyDeploymentSpecs( + v1alpha1.StrategyDeploymentSpec{Name: "test-deployment-one"}, + ), + WithWebhookDefinitions( + v1alpha1.WebhookDescription{ + Type: v1alpha1.ValidatingAdmissionWebhook, + GenerateName: "test-val-webhook-c", + DeploymentName: "test-deployment-c", + }, + v1alpha1.WebhookDescription{ + Type: v1alpha1.MutatingAdmissionWebhook, + GenerateName: "test-mute-webhook-a", + DeploymentName: "test-deployment-a", + }, + v1alpha1.WebhookDescription{ + Type: v1alpha1.ConversionWebhook, + GenerateName: "test-conv-webhook-b", + DeploymentName: "test-deployment-b", + }, v1alpha1.WebhookDescription{ + Type: v1alpha1.MutatingAdmissionWebhook, + GenerateName: "test-mute-webhook-c", + DeploymentName: "test-deployment-c", + }, + v1alpha1.WebhookDescription{ + Type: v1alpha1.ConversionWebhook, + GenerateName: "test-conv-webhook-c-b", + DeploymentName: "test-deployment-c", + }, v1alpha1.WebhookDescription{ + Type: v1alpha1.ConversionWebhook, + GenerateName: "test-conv-webhook-c-a", + DeploymentName: "test-deployment-c", + }, + ), + ), + }, + expectedErrs: []error{ + errors.New("webhook 'test-mute-webhook-a' of type 'MutatingAdmissionWebhook' references non-existent deployment 'test-deployment-a'"), + errors.New("webhook 'test-conv-webhook-b' of type 'ConversionWebhook' references non-existent deployment 'test-deployment-b'"), + errors.New("webhook 'test-conv-webhook-c-a' of type 'ConversionWebhook' references non-existent deployment 'test-deployment-c'"), + errors.New("webhook 'test-conv-webhook-c-b' of type 'ConversionWebhook' references non-existent deployment 'test-deployment-c'"), + errors.New("webhook 'test-mute-webhook-c' of type 'MutatingAdmissionWebhook' references non-existent deployment 'test-deployment-c'"), + errors.New("webhook 'test-val-webhook-c' of type 'ValidatingAdmissionWebhook' references non-existent deployment 'test-deployment-c'"), + }, + }, + } { + t.Run(tc.name, func(t *testing.T) { + errs := convert.CheckWebhookDeploymentReferentialIntegrity(tc.bundle) + require.Equal(t, tc.expectedErrs, errs) + }) + } +} + +func Test_CheckWebhookNameUniqueness(t *testing.T) { + for _, tc := range []struct { + name string + bundle *convert.RegistryV1 + expectedErrs []error + }{ + { + name: "accepts bundles without webhook definitions", + bundle: &convert.RegistryV1{ + CSV: MakeCSV(), + }, + }, { + name: "accepts bundles with unique webhook names", + bundle: &convert.RegistryV1{ + CSV: MakeCSV( + WithWebhookDefinitions( + v1alpha1.WebhookDescription{ + Type: v1alpha1.MutatingAdmissionWebhook, + GenerateName: "test-webhook-one", + }, v1alpha1.WebhookDescription{ + Type: v1alpha1.ValidatingAdmissionWebhook, + GenerateName: "test-webhook-two", + }, v1alpha1.WebhookDescription{ + Type: v1alpha1.ConversionWebhook, + GenerateName: "test-webhook-three", + }, v1alpha1.WebhookDescription{ + Type: v1alpha1.MutatingAdmissionWebhook, + GenerateName: "test-webhook-four", + }, v1alpha1.WebhookDescription{ + Type: v1alpha1.ValidatingAdmissionWebhook, + GenerateName: "test-webhook-five", + }, v1alpha1.WebhookDescription{ + Type: v1alpha1.ConversionWebhook, + GenerateName: "test-webhook-six", + }, + ), + ), + }, + }, { + name: "accepts bundles with webhooks with the same name but different types", + bundle: &convert.RegistryV1{ + CSV: MakeCSV( + WithWebhookDefinitions( + v1alpha1.WebhookDescription{ + Type: v1alpha1.MutatingAdmissionWebhook, + GenerateName: "test-webhook", + }, v1alpha1.WebhookDescription{ + Type: v1alpha1.ValidatingAdmissionWebhook, + GenerateName: "test-webhook", + }, v1alpha1.WebhookDescription{ + Type: v1alpha1.ConversionWebhook, + GenerateName: "test-webhook", + }, + ), + ), + }, + }, { + name: "rejects bundles with duplicate validating webhook definitions", + bundle: &convert.RegistryV1{ + CSV: MakeCSV( + WithWebhookDefinitions( + v1alpha1.WebhookDescription{ + Type: v1alpha1.ValidatingAdmissionWebhook, + GenerateName: "test-webhook", + }, v1alpha1.WebhookDescription{ + Type: v1alpha1.ValidatingAdmissionWebhook, + GenerateName: "test-webhook", + }, + ), + ), + }, + expectedErrs: []error{ + errors.New("duplicate webhook 'test-webhook' of type 'ValidatingAdmissionWebhook'"), + }, + }, { + name: "rejects bundles with duplicate mutating webhook definitions", + bundle: &convert.RegistryV1{ + CSV: MakeCSV( + WithWebhookDefinitions( + v1alpha1.WebhookDescription{ + Type: v1alpha1.MutatingAdmissionWebhook, + GenerateName: "test-webhook", + }, v1alpha1.WebhookDescription{ + Type: v1alpha1.MutatingAdmissionWebhook, + GenerateName: "test-webhook", + }, + ), + ), + }, + expectedErrs: []error{ + errors.New("duplicate webhook 'test-webhook' of type 'MutatingAdmissionWebhook'"), + }, + }, { + name: "rejects bundles with duplicate conversion webhook definitions", + bundle: &convert.RegistryV1{ + CSV: MakeCSV( + WithWebhookDefinitions( + v1alpha1.WebhookDescription{ + Type: v1alpha1.ConversionWebhook, + GenerateName: "test-webhook", + }, v1alpha1.WebhookDescription{ + Type: v1alpha1.ConversionWebhook, + GenerateName: "test-webhook", + }, + ), + ), + }, + expectedErrs: []error{ + errors.New("duplicate webhook 'test-webhook' of type 'ConversionWebhook'"), + }, + }, { + name: "orders errors by webhook type and name", + bundle: &convert.RegistryV1{ + CSV: MakeCSV( + WithWebhookDefinitions( + v1alpha1.WebhookDescription{ + Type: v1alpha1.ValidatingAdmissionWebhook, + GenerateName: "test-val-webhook-b", + }, v1alpha1.WebhookDescription{ + Type: v1alpha1.ValidatingAdmissionWebhook, + GenerateName: "test-val-webhook-a", + }, + v1alpha1.WebhookDescription{ + Type: v1alpha1.ValidatingAdmissionWebhook, + GenerateName: "test-val-webhook-a", + }, v1alpha1.WebhookDescription{ + Type: v1alpha1.ValidatingAdmissionWebhook, + GenerateName: "test-val-webhook-b", + }, + v1alpha1.WebhookDescription{ + Type: v1alpha1.ConversionWebhook, + GenerateName: "test-conv-webhook-b", + }, v1alpha1.WebhookDescription{ + Type: v1alpha1.ConversionWebhook, + GenerateName: "test-conv-webhook-a", + }, + v1alpha1.WebhookDescription{ + Type: v1alpha1.ConversionWebhook, + GenerateName: "test-conv-webhook-a", + }, v1alpha1.WebhookDescription{ + Type: v1alpha1.ConversionWebhook, + GenerateName: "test-conv-webhook-b", + }, v1alpha1.WebhookDescription{ + Type: v1alpha1.MutatingAdmissionWebhook, + GenerateName: "test-mute-webhook-b", + }, v1alpha1.WebhookDescription{ + Type: v1alpha1.MutatingAdmissionWebhook, + GenerateName: "test-mute-webhook-a", + }, + v1alpha1.WebhookDescription{ + Type: v1alpha1.MutatingAdmissionWebhook, + GenerateName: "test-mute-webhook-a", + }, v1alpha1.WebhookDescription{ + Type: v1alpha1.MutatingAdmissionWebhook, + GenerateName: "test-mute-webhook-b", + }, + ), + ), + }, + expectedErrs: []error{ + errors.New("duplicate webhook 'test-conv-webhook-a' of type 'ConversionWebhook'"), + errors.New("duplicate webhook 'test-conv-webhook-b' of type 'ConversionWebhook'"), + errors.New("duplicate webhook 'test-mute-webhook-a' of type 'MutatingAdmissionWebhook'"), + errors.New("duplicate webhook 'test-mute-webhook-b' of type 'MutatingAdmissionWebhook'"), + errors.New("duplicate webhook 'test-val-webhook-a' of type 'ValidatingAdmissionWebhook'"), + errors.New("duplicate webhook 'test-val-webhook-b' of type 'ValidatingAdmissionWebhook'"), + }, + }, + } { + t.Run(tc.name, func(t *testing.T) { + errs := convert.CheckWebhookNameUniqueness(tc.bundle) + require.Equal(t, tc.expectedErrs, errs) + }) + } +} + +func Test_CheckConversionWebhookCRDReferences(t *testing.T) { + for _, tc := range []struct { + name string + bundle *convert.RegistryV1 + expectedErrs []error + }{ + { + name: "accepts bundles without webhook definitions", + bundle: &convert.RegistryV1{}, + }, { + name: "accepts bundles without conversion webhook definitions", + bundle: &convert.RegistryV1{ + CSV: MakeCSV( + WithWebhookDefinitions( + v1alpha1.WebhookDescription{ + Type: v1alpha1.ValidatingAdmissionWebhook, + GenerateName: "test-val-webhook", + }, + v1alpha1.WebhookDescription{ + Type: v1alpha1.MutatingAdmissionWebhook, + GenerateName: "test-mute-webhook", + }, + ), + ), + }, + }, { + name: "accepts bundles with conversion webhooks that reference owned CRDs", + bundle: &convert.RegistryV1{ + CSV: MakeCSV( + WithOwnedCRDs( + v1alpha1.CRDDescription{Name: "some.crd.something"}, + v1alpha1.CRDDescription{Name: "another.crd.something"}, + ), + WithWebhookDefinitions( + v1alpha1.WebhookDescription{ + Type: v1alpha1.ConversionWebhook, + GenerateName: "test-webhook", + ConversionCRDs: []string{ + "some.crd.something", + "another.crd.something", + }, + }, + ), + ), + }, + }, { + name: "rejects bundles with conversion webhooks that reference existing CRDs that are not owned", + bundle: &convert.RegistryV1{ + CSV: MakeCSV( + WithOwnedCRDs( + v1alpha1.CRDDescription{Name: "some.crd.something"}, + ), + WithWebhookDefinitions( + v1alpha1.WebhookDescription{ + Type: v1alpha1.ConversionWebhook, + GenerateName: "test-webhook", + ConversionCRDs: []string{ + "some.crd.something", + "another.crd.something", + }, + }, + ), + ), + }, + expectedErrs: []error{ + errors.New("conversion webhook 'test-webhook' references custom resource definition 'another.crd.something' not owned bundle"), + }, + }, { + name: "errors are ordered by webhook name and CRD name", + bundle: &convert.RegistryV1{ + CSV: MakeCSV( + WithOwnedCRDs( + v1alpha1.CRDDescription{Name: "b.crd.something"}, + ), + WithWebhookDefinitions( + v1alpha1.WebhookDescription{ + Type: v1alpha1.ConversionWebhook, + GenerateName: "test-webhook-b", + ConversionCRDs: []string{ + "b.crd.something", + }, + }, v1alpha1.WebhookDescription{ + Type: v1alpha1.ConversionWebhook, + GenerateName: "test-webhook-a", + ConversionCRDs: []string{ + "c.crd.something", + "a.crd.something", + }, + }, v1alpha1.WebhookDescription{ + Type: v1alpha1.ConversionWebhook, + GenerateName: "test-webhook-c", + ConversionCRDs: []string{ + "a.crd.something", + "d.crd.something", + }, + }, + ), + ), + }, + expectedErrs: []error{ + errors.New("conversion webhook 'test-webhook-a' references custom resource definition 'a.crd.something' not owned bundle"), + errors.New("conversion webhook 'test-webhook-a' references custom resource definition 'c.crd.something' not owned bundle"), + errors.New("conversion webhook 'test-webhook-c' references custom resource definition 'a.crd.something' not owned bundle"), + errors.New("conversion webhook 'test-webhook-c' references custom resource definition 'd.crd.something' not owned bundle"), + }, + }, + } { + t.Run(tc.name, func(t *testing.T) { + errs := convert.CheckConversionWebhookCRDReferences(tc.bundle) + require.Equal(t, tc.expectedErrs, errs) + }) + } +} + +func WithWebhookDefinitions(webhookDefinitions ...v1alpha1.WebhookDescription) CSVOption { + return func(csv *v1alpha1.ClusterServiceVersion) { + csv.Spec.WebhookDefinitions = webhookDefinitions + } +} + +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 +}