diff --git a/internal/operator-controller/rukpak/render/registryv1/registryv1.go b/internal/operator-controller/rukpak/render/registryv1/registryv1.go index 6621a6ca4..c9caa4c5f 100644 --- a/internal/operator-controller/rukpak/render/registryv1/registryv1.go +++ b/internal/operator-controller/rukpak/render/registryv1/registryv1.go @@ -22,6 +22,7 @@ var BundleValidator = render.BundleValidator{ validators.CheckCRDResourceUniqueness, validators.CheckOwnedCRDExistence, validators.CheckPackageNameNotEmpty, + validators.CheckConversionWebhookSupport, validators.CheckWebhookDeploymentReferentialIntegrity, validators.CheckWebhookNameUniqueness, validators.CheckWebhookNameIsDNS1123SubDomain, diff --git a/internal/operator-controller/rukpak/render/registryv1/registryv1_test.go b/internal/operator-controller/rukpak/render/registryv1/registryv1_test.go index c75f1d602..cf56863af 100644 --- a/internal/operator-controller/rukpak/render/registryv1/registryv1_test.go +++ b/internal/operator-controller/rukpak/render/registryv1/registryv1_test.go @@ -26,6 +26,7 @@ func Test_BundleValidatorHasAllValidationFns(t *testing.T) { validators.CheckCRDResourceUniqueness, validators.CheckOwnedCRDExistence, validators.CheckPackageNameNotEmpty, + validators.CheckConversionWebhookSupport, validators.CheckWebhookDeploymentReferentialIntegrity, validators.CheckWebhookNameUniqueness, validators.CheckWebhookNameIsDNS1123SubDomain, diff --git a/internal/operator-controller/rukpak/render/registryv1/validators/validator.go b/internal/operator-controller/rukpak/render/registryv1/validators/validator.go index 61d0aad7c..960b4ce73 100644 --- a/internal/operator-controller/rukpak/render/registryv1/validators/validator.go +++ b/internal/operator-controller/rukpak/render/registryv1/validators/validator.go @@ -101,21 +101,32 @@ func CheckPackageNameNotEmpty(rv1 *bundle.RegistryV1) []error { return nil } -// CheckWebhookSupport checks that if the bundle cluster service version declares webhook definitions -// that it is a singleton operator, i.e. that it only supports AllNamespaces mode. This keeps parity -// with OLMv0 behavior for conversion webhooks, +// CheckConversionWebhookSupport checks that if the bundle cluster service version declares conversion webhook definitions, +// that the bundle also only supports AllNamespaces install mode. This keeps parity with OLMv0 behavior for conversion webhooks, // https://github.com/operator-framework/operator-lifecycle-manager/blob/dfd0b2bea85038d3c0d65348bc812d297f16b8d2/pkg/controller/install/webhook.go#L193 -// Since OLMv1 considers APIs to be cluster-scoped, we initially extend this constraint to validating and mutating webhooks. -// While this might restrict the number of supported bundles, we can tackle the issue of relaxing this constraint in turn -// after getting the webhook support working. -func CheckWebhookSupport(rv1 *bundle.RegistryV1) []error { - if len(rv1.CSV.Spec.WebhookDefinitions) > 0 { +func CheckConversionWebhookSupport(rv1 *bundle.RegistryV1) []error { + var conversionWebhookNames []string + for _, wh := range rv1.CSV.Spec.WebhookDefinitions { + if wh.Type == v1alpha1.ConversionWebhook { + conversionWebhookNames = append(conversionWebhookNames, wh.GenerateName) + } + } + + if len(conversionWebhookNames) > 0 { supportedInstallModes := sets.Set[v1alpha1.InstallModeType]{} for _, mode := range rv1.CSV.Spec.InstallModes { - supportedInstallModes.Insert(mode.Type) + if mode.Supported { + supportedInstallModes.Insert(mode.Type) + } } + if len(supportedInstallModes) != 1 || !supportedInstallModes.Has(v1alpha1.InstallModeTypeAllNamespaces) { - return []error{errors.New("bundle contains webhook definitions but supported install modes beyond AllNamespaces")} + sortedModes := slices.Sorted(slices.Values(supportedInstallModes.UnsortedList())) + errs := make([]error, len(conversionWebhookNames)) + for i, webhookName := range conversionWebhookNames { + errs[i] = fmt.Errorf("bundle contains conversion webhook %q and supports install modes %v - conversion webhooks are only supported for bundles that only support AllNamespaces install mode", webhookName, sortedModes) + } + return errs } } diff --git a/internal/operator-controller/rukpak/render/registryv1/validators/validator_test.go b/internal/operator-controller/rukpak/render/registryv1/validators/validator_test.go index 6c1d7491b..c33af4850 100644 --- a/internal/operator-controller/rukpak/render/registryv1/validators/validator_test.go +++ b/internal/operator-controller/rukpak/render/registryv1/validators/validator_test.go @@ -249,32 +249,6 @@ func Test_CheckWebhookSupport(t *testing.T) { bundle *bundle.RegistryV1 expectedErrs []error }{ - { - name: "accepts bundles with validating webhook definitions when they only support AllNamespaces install mode", - bundle: &bundle.RegistryV1{ - CSV: MakeCSV( - WithInstallModeSupportFor(v1alpha1.InstallModeTypeAllNamespaces), - WithWebhookDefinitions( - v1alpha1.WebhookDescription{ - Type: v1alpha1.ValidatingAdmissionWebhook, - }, - ), - ), - }, - }, - { - name: "accepts bundles with mutating webhook definitions when they only support AllNamespaces install mode", - bundle: &bundle.RegistryV1{ - CSV: MakeCSV( - WithInstallModeSupportFor(v1alpha1.InstallModeTypeAllNamespaces), - WithWebhookDefinitions( - v1alpha1.WebhookDescription{ - Type: v1alpha1.MutatingAdmissionWebhook, - }, - ), - ), - }, - }, { name: "accepts bundles with conversion webhook definitions when they only support AllNamespaces install mode", bundle: &bundle.RegistryV1{ @@ -289,7 +263,7 @@ func Test_CheckWebhookSupport(t *testing.T) { }, }, { - name: "rejects bundles with validating webhook definitions when they support more modes than AllNamespaces install mode", + name: "accepts bundles with validating webhook definitions when they support more modes than AllNamespaces install mode", bundle: &bundle.RegistryV1{ CSV: MakeCSV( WithInstallModeSupportFor(v1alpha1.InstallModeTypeAllNamespaces, v1alpha1.InstallModeTypeSingleNamespace), @@ -300,7 +274,6 @@ func Test_CheckWebhookSupport(t *testing.T) { ), ), }, - expectedErrs: []error{errors.New("bundle contains webhook definitions but supported install modes beyond AllNamespaces")}, }, { name: "accepts bundles with mutating webhook definitions when they support more modes than AllNamespaces install mode", @@ -314,25 +287,32 @@ func Test_CheckWebhookSupport(t *testing.T) { ), ), }, - expectedErrs: []error{errors.New("bundle contains webhook definitions but supported install modes beyond AllNamespaces")}, }, { - name: "accepts bundles with conversion webhook definitions when they support more modes than AllNamespaces install mode", + name: "rejects bundles with conversion webhook definitions when they support more modes than AllNamespaces install mode", bundle: &bundle.RegistryV1{ CSV: MakeCSV( - WithInstallModeSupportFor(v1alpha1.InstallModeTypeAllNamespaces, v1alpha1.InstallModeTypeSingleNamespace), + WithInstallModeSupportFor(v1alpha1.InstallModeTypeSingleNamespace, v1alpha1.InstallModeTypeAllNamespaces), WithWebhookDefinitions( v1alpha1.WebhookDescription{ - Type: v1alpha1.ConversionWebhook, + GenerateName: "webhook-b", + Type: v1alpha1.ConversionWebhook, + }, + v1alpha1.WebhookDescription{ + GenerateName: "webhook-a", + Type: v1alpha1.ConversionWebhook, }, ), ), }, - expectedErrs: []error{errors.New("bundle contains webhook definitions but supported install modes beyond AllNamespaces")}, + expectedErrs: []error{ + errors.New("bundle contains conversion webhook \"webhook-b\" and supports install modes [AllNamespaces SingleNamespace] - conversion webhooks are only supported for bundles that only support AllNamespaces install mode"), + errors.New("bundle contains conversion webhook \"webhook-a\" and supports install modes [AllNamespaces SingleNamespace] - conversion webhooks are only supported for bundles that only support AllNamespaces install mode"), + }, }, } { t.Run(tc.name, func(t *testing.T) { - errs := validators.CheckWebhookSupport(tc.bundle) + errs := validators.CheckConversionWebhookSupport(tc.bundle) require.Equal(t, tc.expectedErrs, errs) }) } diff --git a/internal/operator-controller/rukpak/util/testing/testing.go b/internal/operator-controller/rukpak/util/testing/testing.go index f5c9b36a3..e544e546c 100644 --- a/internal/operator-controller/rukpak/util/testing/testing.go +++ b/internal/operator-controller/rukpak/util/testing/testing.go @@ -6,6 +6,7 @@ import ( "github.com/stretchr/testify/require" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/util/sets" "sigs.k8s.io/controller-runtime/pkg/client" "github.com/operator-framework/api/pkg/operators/v1alpha1" @@ -55,15 +56,22 @@ func WithOwnedCRDs(crdDesc ...v1alpha1.CRDDescription) CSVOption { } func WithInstallModeSupportFor(installModeType ...v1alpha1.InstallModeType) CSVOption { + var installModes = []v1alpha1.InstallModeType{ + v1alpha1.InstallModeTypeAllNamespaces, + v1alpha1.InstallModeTypeSingleNamespace, + v1alpha1.InstallModeTypeMultiNamespace, + v1alpha1.InstallModeTypeOwnNamespace, + } return func(csv *v1alpha1.ClusterServiceVersion) { - installModes := make([]v1alpha1.InstallMode, 0, len(installModeType)) - for _, t := range installModeType { - installModes = append(installModes, v1alpha1.InstallMode{ + supportedInstallModes := sets.New(installModeType...) + csvInstallModes := make([]v1alpha1.InstallMode, 0, len(installModeType)) + for _, t := range installModes { + csvInstallModes = append(csvInstallModes, v1alpha1.InstallMode{ Type: t, - Supported: true, + Supported: supportedInstallModes.Has(t), }) } - csv.Spec.InstallModes = installModes + csv.Spec.InstallModes = csvInstallModes } } diff --git a/internal/operator-controller/rukpak/util/testing/testing_test.go b/internal/operator-controller/rukpak/util/testing/testing_test.go index c8744cfa0..703cc0018 100644 --- a/internal/operator-controller/rukpak/util/testing/testing_test.go +++ b/internal/operator-controller/rukpak/util/testing/testing_test.go @@ -209,6 +209,14 @@ func Test_MakeCSV_WithInstallModeSupportFor(t *testing.T) { Type: v1alpha1.InstallModeTypeSingleNamespace, Supported: true, }, + { + Type: v1alpha1.InstallModeTypeMultiNamespace, + Supported: false, + }, + { + Type: v1alpha1.InstallModeTypeOwnNamespace, + Supported: false, + }, }, }, }, csv)