diff --git a/OWNERS_ALIASES b/OWNERS_ALIASES index d24c20b01..1776b9b65 100644 --- a/OWNERS_ALIASES +++ b/OWNERS_ALIASES @@ -20,6 +20,7 @@ aliases: - thetechnick - tmshort - trgeiger + - pedjak api-approvers: - grokspawn diff --git a/api/v1/clusterextensionrevision_types.go b/api/v1/clusterextensionrevision_types.go index f06b1e262..375c17737 100644 --- a/api/v1/clusterextensionrevision_types.go +++ b/api/v1/clusterextensionrevision_types.go @@ -55,13 +55,13 @@ type ClusterExtensionRevisionSpec struct { // +kubebuilder:validation:XValidation:rule="self == oldSelf", message="revision is immutable" Revision int64 `json:"revision"` // Phases are groups of objects that will be applied at the same time. - // All objects in the a phase will have to pass their probes in order to progress to the next phase. + // All objects in the phase will have to pass their probes in order to progress to the next phase. // - // +kubebuilder:validation:Required // +kubebuilder:validation:XValidation:rule="self == oldSelf || oldSelf.size() == 0", message="phases is immutable" // +listType=map // +listMapKey=name - Phases []ClusterExtensionRevisionPhase `json:"phases"` + // +optional + Phases []ClusterExtensionRevisionPhase `json:"phases,omitempty"` // Previous references previous revisions that objects can be adopted from. // // +kubebuilder:validation:XValidation:rule="self == oldSelf", message="previous is immutable" @@ -104,6 +104,7 @@ type ClusterExtensionRevisionObject struct { // already existing on the cluster or even owned by another controller. // // +kubebuilder:default="Prevent" + // +optional CollisionProtection CollisionProtection `json:"collisionProtection,omitempty"` } diff --git a/api/v1/clusterextensionrevision_types_test.go b/api/v1/clusterextensionrevision_types_test.go new file mode 100644 index 000000000..a57d958c0 --- /dev/null +++ b/api/v1/clusterextensionrevision_types_test.go @@ -0,0 +1,94 @@ +package v1 + +import ( + "context" + "fmt" + "testing" + + "github.com/stretchr/testify/require" + "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +func TestClusterExtensionRevisionImmutability(t *testing.T) { + c := newClient(t) + ctx := context.Background() + i := 0 + for name, tc := range map[string]struct { + spec ClusterExtensionRevisionSpec + updateFunc func(*ClusterExtensionRevision) + allowed bool + }{ + "revision is immutable": { + spec: ClusterExtensionRevisionSpec{ + Revision: 1, + }, + updateFunc: func(cer *ClusterExtensionRevision) { + cer.Spec.Revision = 2 + }, + }, + "phases may be initially empty": { + spec: ClusterExtensionRevisionSpec{ + Phases: []ClusterExtensionRevisionPhase{}, + }, + updateFunc: func(cer *ClusterExtensionRevision) { + cer.Spec.Phases = []ClusterExtensionRevisionPhase{ + { + Name: "foo", + Objects: []ClusterExtensionRevisionObject{}, + }, + } + }, + allowed: true, + }, + "phases may be initially unset": { + spec: ClusterExtensionRevisionSpec{}, + updateFunc: func(cer *ClusterExtensionRevision) { + cer.Spec.Phases = []ClusterExtensionRevisionPhase{ + { + Name: "foo", + Objects: []ClusterExtensionRevisionObject{}, + }, + } + }, + allowed: true, + }, + "phases are immutable if not empty": { + spec: ClusterExtensionRevisionSpec{ + Phases: []ClusterExtensionRevisionPhase{ + { + Name: "foo", + Objects: []ClusterExtensionRevisionObject{}, + }, + }, + }, + updateFunc: func(cer *ClusterExtensionRevision) { + cer.Spec.Phases = []ClusterExtensionRevisionPhase{ + { + Name: "foo2", + Objects: []ClusterExtensionRevisionObject{}, + }, + } + }, + }, + } { + t.Run(name, func(t *testing.T) { + cer := &ClusterExtensionRevision{ + ObjectMeta: metav1.ObjectMeta{ + Name: fmt.Sprintf("foo%d", i), + }, + Spec: tc.spec, + } + i = i + 1 + require.NoError(t, c.Create(ctx, cer)) + tc.updateFunc(cer) + err := c.Update(ctx, cer) + if tc.allowed && err != nil { + t.Fatal("expected update to succeed, but got:", err) + } + if !tc.allowed && !errors.IsInvalid(err) { + t.Fatal("expected update to fail due to invalid payload, but got:", err) + } + }) + } +} diff --git a/api/v1/suite_test.go b/api/v1/suite_test.go new file mode 100644 index 000000000..bc7a0c22b --- /dev/null +++ b/api/v1/suite_test.go @@ -0,0 +1,61 @@ +/* +Copyright 2025. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package v1 + +import ( + "log" + "os" + "testing" + + "github.com/stretchr/testify/require" + apimachineryruntime "k8s.io/apimachinery/pkg/runtime" + utilruntime "k8s.io/apimachinery/pkg/util/runtime" + "k8s.io/client-go/rest" + "sigs.k8s.io/controller-runtime/pkg/client" + + "github.com/operator-framework/operator-controller/test" +) + +func newScheme(t *testing.T) *apimachineryruntime.Scheme { + sch := apimachineryruntime.NewScheme() + require.NoError(t, AddToScheme(sch)) + return sch +} + +func newClient(t *testing.T) client.Client { + cl, err := client.New(config, client.Options{Scheme: newScheme(t)}) + require.NoError(t, err) + require.NotNil(t, cl) + return cl +} + +var config *rest.Config + +func TestMain(m *testing.M) { + testEnv := test.NewEnv() + + var err error + config, err = testEnv.Start() + utilruntime.Must(err) + if config == nil { + log.Panic("expected cfg to not be nil") + } + + code := m.Run() + utilruntime.Must(testEnv.Stop()) + os.Exit(code) +} diff --git a/commitchecker.yaml b/commitchecker.yaml index ba47d14b7..a87eb084f 100644 --- a/commitchecker.yaml +++ b/commitchecker.yaml @@ -1,4 +1,4 @@ -expectedMergeBase: 33fdce258350eb563885ee41da087491a15829bd +expectedMergeBase: 47f8d312187f9f6a41118ee1c5c2f11c09afb7ce upstreamBranch: main upstreamOrg: operator-framework upstreamRepo: operator-controller diff --git a/helm/olmv1/base/operator-controller/crd/experimental/olm.operatorframework.io_clusterextensionrevisions.yaml b/helm/olmv1/base/operator-controller/crd/experimental/olm.operatorframework.io_clusterextensionrevisions.yaml index a1575258a..ffbe7e3cb 100644 --- a/helm/olmv1/base/operator-controller/crd/experimental/olm.operatorframework.io_clusterextensionrevisions.yaml +++ b/helm/olmv1/base/operator-controller/crd/experimental/olm.operatorframework.io_clusterextensionrevisions.yaml @@ -57,7 +57,7 @@ spec: phases: description: |- Phases are groups of objects that will be applied at the same time. - All objects in the a phase will have to pass their probes in order to progress to the next phase. + All objects in the phase will have to pass their probes in order to progress to the next phase. items: description: |- ClusterExtensionRevisionPhase are groups of objects that will be applied at the same time. @@ -130,7 +130,6 @@ spec: - message: revision is immutable rule: self == oldSelf required: - - phases - revision type: object status: diff --git a/internal/operator-controller/controllers/suite_test.go b/internal/operator-controller/controllers/suite_test.go index ccd59f11f..02d538237 100644 --- a/internal/operator-controller/controllers/suite_test.go +++ b/internal/operator-controller/controllers/suite_test.go @@ -21,7 +21,6 @@ import ( "io/fs" "log" "os" - "path/filepath" "testing" "github.com/stretchr/testify/require" @@ -29,11 +28,11 @@ import ( utilruntime "k8s.io/apimachinery/pkg/util/runtime" "k8s.io/client-go/rest" "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/envtest" crfinalizer "sigs.k8s.io/controller-runtime/pkg/finalizer" ocv1 "github.com/operator-framework/operator-controller/api/v1" "github.com/operator-framework/operator-controller/internal/operator-controller/controllers" + "github.com/operator-framework/operator-controller/test" ) func newScheme(t *testing.T) *apimachineryruntime.Scheme { @@ -93,27 +92,7 @@ func newClientAndReconciler(t *testing.T) (client.Client, *controllers.ClusterEx var config *rest.Config func TestMain(m *testing.M) { - testEnv := &envtest.Environment{ - CRDDirectoryPaths: []string{ - filepath.Join("..", "..", "..", "config", "base", "operator-controller", "crd", "experimental"), - }, - ErrorIfCRDPathMissing: true, - } - - // ENVTEST-based tests require specific binaries. By default, these binaries are located - // in paths defined by controller-runtime. However, the `BinaryAssetsDirectory` needs - // to be explicitly set when running tests directly (e.g., debugging tests in an IDE) - // without using the Makefile targets. - // - // This is equivalent to configuring your IDE to export the `KUBEBUILDER_ASSETS` environment - // variable before each test execution. The following function simplifies this process - // by handling the configuration for you. - // - // To ensure the binaries are in the expected path without manual configuration, run: - // `make envtest-k8s-bins` - if getFirstFoundEnvTestBinaryDir() != "" { - testEnv.BinaryAssetsDirectory = getFirstFoundEnvTestBinaryDir() - } + testEnv := test.NewEnv() var err error config, err = testEnv.Start() @@ -126,15 +105,3 @@ func TestMain(m *testing.M) { utilruntime.Must(testEnv.Stop()) os.Exit(code) } - -// getFirstFoundEnvTestBinaryDir finds and returns the first directory under the given path. -func getFirstFoundEnvTestBinaryDir() string { - basePath := filepath.Join("..", "..", "bin", "envtest-binaries", "k8s") - entries, _ := os.ReadDir(basePath) - for _, entry := range entries { - if entry.IsDir() { - return filepath.Join(basePath, entry.Name()) - } - } - return "" -} diff --git a/internal/operator-controller/rukpak/render/registryv1/registryv1.go b/internal/operator-controller/rukpak/render/registryv1/registryv1.go index 6621a6ca4..1cfefbb8b 100644 --- a/internal/operator-controller/rukpak/render/registryv1/registryv1.go +++ b/internal/operator-controller/rukpak/render/registryv1/registryv1.go @@ -22,11 +22,13 @@ var BundleValidator = render.BundleValidator{ validators.CheckCRDResourceUniqueness, validators.CheckOwnedCRDExistence, validators.CheckPackageNameNotEmpty, + validators.CheckConversionWebhookSupport, validators.CheckWebhookDeploymentReferentialIntegrity, validators.CheckWebhookNameUniqueness, validators.CheckWebhookNameIsDNS1123SubDomain, validators.CheckConversionWebhookCRDReferenceUniqueness, validators.CheckConversionWebhooksReferenceOwnedCRDs, + validators.CheckWebhookRules, } // ResourceGenerators a slice of ResourceGenerators required to generate plain resource manifests for diff --git a/internal/operator-controller/rukpak/render/registryv1/registryv1_test.go b/internal/operator-controller/rukpak/render/registryv1/registryv1_test.go index c75f1d602..afe19d805 100644 --- a/internal/operator-controller/rukpak/render/registryv1/registryv1_test.go +++ b/internal/operator-controller/rukpak/render/registryv1/registryv1_test.go @@ -26,11 +26,13 @@ func Test_BundleValidatorHasAllValidationFns(t *testing.T) { validators.CheckCRDResourceUniqueness, validators.CheckOwnedCRDExistence, validators.CheckPackageNameNotEmpty, + validators.CheckConversionWebhookSupport, validators.CheckWebhookDeploymentReferentialIntegrity, validators.CheckWebhookNameUniqueness, validators.CheckWebhookNameIsDNS1123SubDomain, validators.CheckConversionWebhookCRDReferenceUniqueness, validators.CheckConversionWebhooksReferenceOwnedCRDs, + validators.CheckWebhookRules, } actualValidationFns := registryv1.BundleValidator diff --git a/internal/operator-controller/rukpak/render/registryv1/validators/validator.go b/internal/operator-controller/rukpak/render/registryv1/validators/validator.go index 61d0aad7c..60978aa83 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 } } @@ -264,3 +275,55 @@ func CheckWebhookNameIsDNS1123SubDomain(rv1 *bundle.RegistryV1) []error { } return errs } + +// forbiddenWebhookRuleAPIGroups contain the API groups that are forbidden for webhook configuration rules in OLMv1 +var forbiddenWebhookRuleAPIGroups = sets.New("olm.operatorframework.io", "*") + +// forbiddenAdmissionRegistrationResources contain the resources that are forbidden for webhook configuration rules +// for the admissionregistration.k8s.io api group +var forbiddenAdmissionRegistrationResources = sets.New( + "*", + "mutatingwebhookconfiguration", + "mutatingwebhookconfigurations", + "validatingwebhookconfiguration", + "validatingwebhookconfigurations", +) + +// CheckWebhookRules ensures webhook rules do not reference forbidden API groups or resources in line with OLMv0 behavior +// The following are forbidden, rules targeting: +// - all API groups (i.e. '*') +// - OLMv1 API group (i.e. 'olm.operatorframework.io') +// - all resources under the 'admissionregistration.k8s.io' API group +// - the 'ValidatingWebhookConfiguration' resource under the 'admissionregistration.k8s.io' API group +// - the 'MutatingWebhookConfiguration' resource under the 'admissionregistration.k8s.io' API group +// +// These boundaries attempt to reduce the blast radius of faulty webhooks and avoid deadlocks preventing the user +// from deleting OLMv1 resources installing and managing the faulty webhook, or deleting faulty admission webhook +// configurations. +// See https://github.com/operator-framework/operator-lifecycle-manager/blob/ccf0c4c91f1e7673e87f3a18947f9a1f88d48438/pkg/controller/install/webhook.go#L19 +// for more details +func CheckWebhookRules(rv1 *bundle.RegistryV1) []error { + var errs []error + for _, wh := range rv1.CSV.Spec.WebhookDefinitions { + // Rules are not used for conversion webhooks + if wh.Type == v1alpha1.ConversionWebhook { + continue + } + webhookName := wh.GenerateName + for _, rule := range wh.Rules { + for _, apiGroup := range rule.APIGroups { + if forbiddenWebhookRuleAPIGroups.Has(apiGroup) { + errs = append(errs, fmt.Errorf("webhook %q contains forbidden rule: admission webhook rules cannot reference API group %q", webhookName, apiGroup)) + } + if apiGroup == "admissionregistration.k8s.io" { + for _, resource := range rule.Resources { + if forbiddenAdmissionRegistrationResources.Has(strings.ToLower(resource)) { + errs = append(errs, fmt.Errorf("webhook %q contains forbidden rule: admission webhook rules cannot reference resource %q for API group %q", webhookName, resource, apiGroup)) + } + } + } + } + } + } + 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..135a942ec 100644 --- a/internal/operator-controller/rukpak/render/registryv1/validators/validator_test.go +++ b/internal/operator-controller/rukpak/render/registryv1/validators/validator_test.go @@ -5,6 +5,7 @@ import ( "testing" "github.com/stretchr/testify/require" + admissionregistrationv1 "k8s.io/api/admissionregistration/v1" apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -250,10 +251,23 @@ func Test_CheckWebhookSupport(t *testing.T) { expectedErrs []error }{ { - name: "accepts bundles with validating webhook definitions when they only support AllNamespaces install mode", + name: "accepts bundles with conversion webhook definitions when they only support AllNamespaces install mode", bundle: &bundle.RegistryV1{ CSV: MakeCSV( WithInstallModeSupportFor(v1alpha1.InstallModeTypeAllNamespaces), + WithWebhookDefinitions( + v1alpha1.WebhookDescription{ + Type: v1alpha1.ConversionWebhook, + }, + ), + ), + }, + }, + { + 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), WithWebhookDefinitions( v1alpha1.WebhookDescription{ Type: v1alpha1.ValidatingAdmissionWebhook, @@ -263,10 +277,10 @@ func Test_CheckWebhookSupport(t *testing.T) { }, }, { - name: "accepts bundles with mutating webhook definitions when they only support AllNamespaces install mode", + name: "accepts bundles with mutating webhook definitions when they support more modes than AllNamespaces install mode", bundle: &bundle.RegistryV1{ CSV: MakeCSV( - WithInstallModeSupportFor(v1alpha1.InstallModeTypeAllNamespaces), + WithInstallModeSupportFor(v1alpha1.InstallModeTypeAllNamespaces, v1alpha1.InstallModeTypeSingleNamespace), WithWebhookDefinitions( v1alpha1.WebhookDescription{ Type: v1alpha1.MutatingAdmissionWebhook, @@ -276,63 +290,339 @@ func Test_CheckWebhookSupport(t *testing.T) { }, }, { - name: "accepts bundles with conversion webhook definitions when they only support 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), + 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 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.CheckConversionWebhookSupport(tc.bundle) + require.Equal(t, tc.expectedErrs, errs) + }) + } +} + +func Test_CheckWebhookRules(t *testing.T) { + for _, tc := range []struct { + name string + bundle *bundle.RegistryV1 + expectedErrs []error + }{ { - name: "rejects bundles with validating webhook definitions when they support more modes than AllNamespaces install mode", + name: "accepts bundles with webhook definitions without rules", bundle: &bundle.RegistryV1{ CSV: MakeCSV( - WithInstallModeSupportFor(v1alpha1.InstallModeTypeAllNamespaces, v1alpha1.InstallModeTypeSingleNamespace), + WithInstallModeSupportFor(v1alpha1.InstallModeTypeAllNamespaces), WithWebhookDefinitions( v1alpha1.WebhookDescription{ Type: v1alpha1.ValidatingAdmissionWebhook, }, + v1alpha1.WebhookDescription{ + Type: v1alpha1.MutatingAdmissionWebhook, + }, ), ), }, - 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", + name: "accepts bundles with webhook definitions with supported rules", bundle: &bundle.RegistryV1{ CSV: MakeCSV( - WithInstallModeSupportFor(v1alpha1.InstallModeTypeAllNamespaces, v1alpha1.InstallModeTypeSingleNamespace), + WithInstallModeSupportFor(v1alpha1.InstallModeTypeAllNamespaces), WithWebhookDefinitions( + v1alpha1.WebhookDescription{ + Type: v1alpha1.ValidatingAdmissionWebhook, + Rules: []admissionregistrationv1.RuleWithOperations{ + { + Rule: admissionregistrationv1.Rule{ + APIGroups: []string{"appsv1"}, + Resources: []string{"deployments", "replicasets", "statefulsets"}, + }, + }, + }, + }, v1alpha1.WebhookDescription{ Type: v1alpha1.MutatingAdmissionWebhook, + Rules: []admissionregistrationv1.RuleWithOperations{ + { + Rule: admissionregistrationv1.Rule{ + APIGroups: []string{""}, + Resources: []string{"services"}, + }, + }, + }, }, ), ), }, - 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: "reject bundles with webhook definitions with rules containing '*' api group", bundle: &bundle.RegistryV1{ CSV: MakeCSV( - WithInstallModeSupportFor(v1alpha1.InstallModeTypeAllNamespaces, v1alpha1.InstallModeTypeSingleNamespace), + WithInstallModeSupportFor(v1alpha1.InstallModeTypeAllNamespaces), WithWebhookDefinitions( v1alpha1.WebhookDescription{ - Type: v1alpha1.ConversionWebhook, + Type: v1alpha1.ValidatingAdmissionWebhook, + GenerateName: "webhook-z", + Rules: []admissionregistrationv1.RuleWithOperations{ + { + Rule: admissionregistrationv1.Rule{ + APIGroups: []string{"*"}, + }, + }, + }, + }, + v1alpha1.WebhookDescription{ + Type: v1alpha1.MutatingAdmissionWebhook, + GenerateName: "webhook-a", + Rules: []admissionregistrationv1.RuleWithOperations{ + { + Rule: admissionregistrationv1.Rule{ + APIGroups: []string{"*"}, + }, + }, + }, + }, + ), + ), + }, + expectedErrs: []error{ + errors.New("webhook \"webhook-z\" contains forbidden rule: admission webhook rules cannot reference API group \"*\""), + errors.New("webhook \"webhook-a\" contains forbidden rule: admission webhook rules cannot reference API group \"*\""), + }, + }, + { + name: "reject bundles with webhook definitions with rules containing 'olm.operatorframework.io' api group", + bundle: &bundle.RegistryV1{ + CSV: MakeCSV( + WithInstallModeSupportFor(v1alpha1.InstallModeTypeAllNamespaces), + WithWebhookDefinitions( + v1alpha1.WebhookDescription{ + Type: v1alpha1.ValidatingAdmissionWebhook, + GenerateName: "webhook-z", + Rules: []admissionregistrationv1.RuleWithOperations{ + { + Rule: admissionregistrationv1.Rule{ + APIGroups: []string{"olm.operatorframework.io"}, + }, + }, + }, + }, + v1alpha1.WebhookDescription{ + Type: v1alpha1.MutatingAdmissionWebhook, + GenerateName: "webhook-a", + Rules: []admissionregistrationv1.RuleWithOperations{ + { + Rule: admissionregistrationv1.Rule{ + APIGroups: []string{"olm.operatorframework.io"}, + }, + }, + }, + }, + ), + ), + }, + expectedErrs: []error{ + errors.New("webhook \"webhook-z\" contains forbidden rule: admission webhook rules cannot reference API group \"olm.operatorframework.io\""), + errors.New("webhook \"webhook-a\" contains forbidden rule: admission webhook rules cannot reference API group \"olm.operatorframework.io\""), + }, + }, + { + name: "reject bundles with webhook definitions with rules containing 'admissionregistration.k8s.io' api group and '*' resource", + bundle: &bundle.RegistryV1{ + CSV: MakeCSV( + WithInstallModeSupportFor(v1alpha1.InstallModeTypeAllNamespaces), + WithWebhookDefinitions( + v1alpha1.WebhookDescription{ + Type: v1alpha1.ValidatingAdmissionWebhook, + GenerateName: "webhook-a", + Rules: []admissionregistrationv1.RuleWithOperations{ + { + Rule: admissionregistrationv1.Rule{ + APIGroups: []string{"admissionregistration.k8s.io"}, + Resources: []string{"*"}, + }, + }, + }, }, ), ), }, - expectedErrs: []error{errors.New("bundle contains webhook definitions but supported install modes beyond AllNamespaces")}, + expectedErrs: []error{ + errors.New("webhook \"webhook-a\" contains forbidden rule: admission webhook rules cannot reference resource \"*\" for API group \"admissionregistration.k8s.io\""), + }, + }, + { + name: "reject bundles with webhook definitions with rules containing 'admissionregistration.k8s.io' api group and 'MutatingWebhookConfiguration' resource", + bundle: &bundle.RegistryV1{ + CSV: MakeCSV( + WithInstallModeSupportFor(v1alpha1.InstallModeTypeAllNamespaces), + WithWebhookDefinitions( + v1alpha1.WebhookDescription{ + Type: v1alpha1.ValidatingAdmissionWebhook, + GenerateName: "webhook-a", + Rules: []admissionregistrationv1.RuleWithOperations{ + { + Rule: admissionregistrationv1.Rule{ + APIGroups: []string{"admissionregistration.k8s.io"}, + Resources: []string{"MutatingWebhookConfiguration"}, + }, + }, + }, + }, + ), + ), + }, + expectedErrs: []error{ + errors.New("webhook \"webhook-a\" contains forbidden rule: admission webhook rules cannot reference resource \"MutatingWebhookConfiguration\" for API group \"admissionregistration.k8s.io\""), + }, + }, + { + name: "reject bundles with webhook definitions with rules containing 'admissionregistration.k8s.io' api group and 'mutatingwebhookconfiguration' resource", + bundle: &bundle.RegistryV1{ + CSV: MakeCSV( + WithInstallModeSupportFor(v1alpha1.InstallModeTypeAllNamespaces), + WithWebhookDefinitions( + v1alpha1.WebhookDescription{ + Type: v1alpha1.ValidatingAdmissionWebhook, + GenerateName: "webhook-a", + Rules: []admissionregistrationv1.RuleWithOperations{ + { + Rule: admissionregistrationv1.Rule{ + APIGroups: []string{"admissionregistration.k8s.io"}, + Resources: []string{"mutatingwebhookconfiguration"}, + }, + }, + }, + }, + ), + ), + }, + expectedErrs: []error{ + errors.New("webhook \"webhook-a\" contains forbidden rule: admission webhook rules cannot reference resource \"mutatingwebhookconfiguration\" for API group \"admissionregistration.k8s.io\""), + }, + }, + { + name: "reject bundles with webhook definitions with rules containing 'admissionregistration.k8s.io' api group and 'mutatingwebhookconfigurations' resource", + bundle: &bundle.RegistryV1{ + CSV: MakeCSV( + WithInstallModeSupportFor(v1alpha1.InstallModeTypeAllNamespaces), + WithWebhookDefinitions( + v1alpha1.WebhookDescription{ + Type: v1alpha1.ValidatingAdmissionWebhook, + GenerateName: "webhook-a", + Rules: []admissionregistrationv1.RuleWithOperations{ + { + Rule: admissionregistrationv1.Rule{ + APIGroups: []string{"admissionregistration.k8s.io"}, + Resources: []string{"mutatingwebhookconfigurations"}, + }, + }, + }, + }, + ), + ), + }, + expectedErrs: []error{ + errors.New("webhook \"webhook-a\" contains forbidden rule: admission webhook rules cannot reference resource \"mutatingwebhookconfigurations\" for API group \"admissionregistration.k8s.io\""), + }, + }, + { + name: "reject bundles with webhook definitions with rules containing 'admissionregistration.k8s.io' api group and 'ValidatingWebhookConfiguration' resource", + bundle: &bundle.RegistryV1{ + CSV: MakeCSV( + WithInstallModeSupportFor(v1alpha1.InstallModeTypeAllNamespaces), + WithWebhookDefinitions( + v1alpha1.WebhookDescription{ + Type: v1alpha1.ValidatingAdmissionWebhook, + GenerateName: "webhook-a", + Rules: []admissionregistrationv1.RuleWithOperations{ + { + Rule: admissionregistrationv1.Rule{ + APIGroups: []string{"admissionregistration.k8s.io"}, + Resources: []string{"ValidatingWebhookConfiguration"}, + }, + }, + }, + }, + ), + ), + }, + expectedErrs: []error{ + errors.New("webhook \"webhook-a\" contains forbidden rule: admission webhook rules cannot reference resource \"ValidatingWebhookConfiguration\" for API group \"admissionregistration.k8s.io\""), + }, + }, + { + name: "reject bundles with webhook definitions with rules containing 'admissionregistration.k8s.io' api group and 'validatingwebhookconfiguration' resource", + bundle: &bundle.RegistryV1{ + CSV: MakeCSV( + WithInstallModeSupportFor(v1alpha1.InstallModeTypeAllNamespaces), + WithWebhookDefinitions( + v1alpha1.WebhookDescription{ + Type: v1alpha1.ValidatingAdmissionWebhook, + GenerateName: "webhook-a", + Rules: []admissionregistrationv1.RuleWithOperations{ + { + Rule: admissionregistrationv1.Rule{ + APIGroups: []string{"admissionregistration.k8s.io"}, + Resources: []string{"validatingwebhookconfiguration"}, + }, + }, + }, + }, + ), + ), + }, + expectedErrs: []error{ + errors.New("webhook \"webhook-a\" contains forbidden rule: admission webhook rules cannot reference resource \"validatingwebhookconfiguration\" for API group \"admissionregistration.k8s.io\""), + }, + }, + { + name: "reject bundles with webhook definitions with rules containing 'admissionregistration.k8s.io' api group and 'validatingwebhookconfigurations' resource", + bundle: &bundle.RegistryV1{ + CSV: MakeCSV( + WithInstallModeSupportFor(v1alpha1.InstallModeTypeAllNamespaces), + WithWebhookDefinitions( + v1alpha1.WebhookDescription{ + Type: v1alpha1.ValidatingAdmissionWebhook, + GenerateName: "webhook-a", + Rules: []admissionregistrationv1.RuleWithOperations{ + { + Rule: admissionregistrationv1.Rule{ + APIGroups: []string{"admissionregistration.k8s.io"}, + Resources: []string{"validatingwebhookconfigurations"}, + }, + }, + }, + }, + ), + ), + }, + expectedErrs: []error{ + errors.New("webhook \"webhook-a\" contains forbidden rule: admission webhook rules cannot reference resource \"validatingwebhookconfigurations\" for API group \"admissionregistration.k8s.io\""), + }, }, } { t.Run(tc.name, func(t *testing.T) { - errs := validators.CheckWebhookSupport(tc.bundle) + errs := validators.CheckWebhookRules(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) diff --git a/manifests/experimental-e2e.yaml b/manifests/experimental-e2e.yaml index cb0ace956..63a8b0f74 100644 --- a/manifests/experimental-e2e.yaml +++ b/manifests/experimental-e2e.yaml @@ -648,7 +648,7 @@ spec: phases: description: |- Phases are groups of objects that will be applied at the same time. - All objects in the a phase will have to pass their probes in order to progress to the next phase. + All objects in the phase will have to pass their probes in order to progress to the next phase. items: description: |- ClusterExtensionRevisionPhase are groups of objects that will be applied at the same time. @@ -721,7 +721,6 @@ spec: - message: revision is immutable rule: self == oldSelf required: - - phases - revision type: object status: diff --git a/manifests/experimental.yaml b/manifests/experimental.yaml index 9621e6a1a..478d11446 100644 --- a/manifests/experimental.yaml +++ b/manifests/experimental.yaml @@ -613,7 +613,7 @@ spec: phases: description: |- Phases are groups of objects that will be applied at the same time. - All objects in the a phase will have to pass their probes in order to progress to the next phase. + All objects in the phase will have to pass their probes in order to progress to the next phase. items: description: |- ClusterExtensionRevisionPhase are groups of objects that will be applied at the same time. @@ -686,7 +686,6 @@ spec: - message: revision is immutable rule: self == oldSelf required: - - phases - revision type: object status: diff --git a/openshift/operator-controller/manifests-experimental.yaml b/openshift/operator-controller/manifests-experimental.yaml index c09c43e0c..550c429ef 100644 --- a/openshift/operator-controller/manifests-experimental.yaml +++ b/openshift/operator-controller/manifests-experimental.yaml @@ -142,7 +142,7 @@ spec: phases: description: |- Phases are groups of objects that will be applied at the same time. - All objects in the a phase will have to pass their probes in order to progress to the next phase. + All objects in the phase will have to pass their probes in order to progress to the next phase. items: description: |- ClusterExtensionRevisionPhase are groups of objects that will be applied at the same time. @@ -211,7 +211,6 @@ spec: - message: revision is immutable rule: self == oldSelf required: - - phases - revision type: object status: diff --git a/openshift/operator-controller/manifests-experimental/05-customresourcedefinition-clusterextensionrevisions.olm.operatorframework.io.yml b/openshift/operator-controller/manifests-experimental/05-customresourcedefinition-clusterextensionrevisions.olm.operatorframework.io.yml index 363fecdb9..8c1107c4b 100644 --- a/openshift/operator-controller/manifests-experimental/05-customresourcedefinition-clusterextensionrevisions.olm.operatorframework.io.yml +++ b/openshift/operator-controller/manifests-experimental/05-customresourcedefinition-clusterextensionrevisions.olm.operatorframework.io.yml @@ -55,7 +55,7 @@ spec: phases: description: |- Phases are groups of objects that will be applied at the same time. - All objects in the a phase will have to pass their probes in order to progress to the next phase. + All objects in the phase will have to pass their probes in order to progress to the next phase. items: description: |- ClusterExtensionRevisionPhase are groups of objects that will be applied at the same time. @@ -124,7 +124,6 @@ spec: - message: revision is immutable rule: self == oldSelf required: - - phases - revision type: object status: diff --git a/requirements.txt b/requirements.txt index ec821077a..8d703a058 100644 --- a/requirements.txt +++ b/requirements.txt @@ -8,7 +8,7 @@ cssselect==1.3.0 ghp-import==2.1.0 idna==3.10 Jinja2==3.1.6 -lxml==6.0.1 +lxml==6.0.2 Markdown==3.9 markdown2==2.5.4 MarkupSafe==3.0.2 @@ -27,7 +27,7 @@ python-dateutil==2.9.0.post0 PyYAML==6.0.2 pyyaml_env_tag==1.1 readtime==3.0.0 -regex==2025.9.1 +regex==2025.9.18 requests==2.32.5 six==1.17.0 soupsieve==2.8 diff --git a/test/utils.go b/test/utils.go new file mode 100644 index 000000000..22a50b2b8 --- /dev/null +++ b/test/utils.go @@ -0,0 +1,54 @@ +package test + +import ( + "os" + "path" + "path/filepath" + "runtime" + + "sigs.k8s.io/controller-runtime/pkg/envtest" +) + +// NewEnv creates a new envtest.Environment instance. +func NewEnv() *envtest.Environment { + testEnv := &envtest.Environment{ + CRDDirectoryPaths: []string{ + pathFromProjectRoot("helm/olmv1/base/operator-controller/crd/experimental"), + }, + ErrorIfCRDPathMissing: true, + } + // ENVTEST-based tests require specific binaries. By default, these binaries are located + // in paths defined by controller-runtime. However, the `BinaryAssetsDirectory` needs + // to be explicitly set when running tests directly (e.g., debugging tests in an IDE) + // without using the Makefile targets. + // + // This is equivalent to configuring your IDE to export the `KUBEBUILDER_ASSETS` environment + // variable before each test execution. The following function simplifies this process + // by handling the configuration for you. + // + // To ensure the binaries are in the expected path without manual configuration, run: + // `make envtest-k8s-bins` + if getFirstFoundEnvTestBinaryDir() != "" { + testEnv.BinaryAssetsDirectory = getFirstFoundEnvTestBinaryDir() + } + return testEnv +} + +// pathFromProjectRoot returns the absolute path to the given relative path from the project root. +func pathFromProjectRoot(relativePath string) string { + _, filename, _, _ := runtime.Caller(0) + p := path.Join(path.Dir(path.Dir(filename)), relativePath) + return p +} + +// getFirstFoundEnvTestBinaryDir finds and returns the first directory under the given path. +func getFirstFoundEnvTestBinaryDir() string { + basePath := pathFromProjectRoot(filepath.Join("bin", "envtest-binaries", "k8s")) + entries, _ := os.ReadDir(basePath) + for _, entry := range entries { + if entry.IsDir() { + return filepath.Join(basePath, entry.Name()) + } + } + return "" +}