From 817bd0f492f679b65120ebd18a6fbc20aef3adb8 Mon Sep 17 00:00:00 2001 From: Mikalai Radchuk Date: Mon, 13 May 2024 16:28:57 +0200 Subject: [PATCH] Remove admission for package uniqueness API servers can't guarantee uniqueness because they can handle more than one concurrent request for the same resource and both can be past admission but before storage. There's also latency between creation and an update to the binding's param informer. The problems gets worse in HA setups when you have multiple kube-apiserver instances. Signed-off-by: Mikalai Radchuk --- config/admission/admission.yaml | 37 ------- config/admission/kustomization.yaml | 5 - config/admission/kustomizeconfig.yaml | 9 -- config/default/kustomization.yaml | 1 - test/e2e/cluster_extension_admission_test.go | 103 ------------------- 5 files changed, 155 deletions(-) delete mode 100644 config/admission/admission.yaml delete mode 100644 config/admission/kustomization.yaml delete mode 100644 config/admission/kustomizeconfig.yaml delete mode 100644 test/e2e/cluster_extension_admission_test.go diff --git a/config/admission/admission.yaml b/config/admission/admission.yaml deleted file mode 100644 index 8a795ae96..000000000 --- a/config/admission/admission.yaml +++ /dev/null @@ -1,37 +0,0 @@ -apiVersion: admissionregistration.k8s.io/v1beta1 -kind: ValidatingAdmissionPolicy -metadata: - name: "clusterextensions-package-uniqueness" -spec: - failurePolicy: Fail - paramKind: - apiVersion: olm.operatorframework.io/v1alpha1 - kind: ClusterExtension - matchConstraints: - resourceRules: - - apiGroups: ["olm.operatorframework.io"] - apiVersions: ["v1alpha1"] - operations: ["CREATE", "UPDATE"] - resources: ["clusterextensions"] - matchConditions: - # Only apply the policy when the request operation is CREATE - # or when the package is being changed - - name: 'only-create-or-package-change' - expression: request.operation == 'CREATE' || oldObject.spec.packageName != object.spec.packageName - validations: - - expression: object.spec.packageName != params.spec.packageName - messageExpression: "'Package \"' + string(object.spec.packageName) + '\" is already installed via ClusterExtension \"' + string(params.metadata.name) + '\"'" - reason: Invalid - ---- - -apiVersion: admissionregistration.k8s.io/v1beta1 -kind: ValidatingAdmissionPolicyBinding -metadata: - name: "clusterextensions-package-uniqueness-binding" -spec: - policyName: "clusterextensions-package-uniqueness" - validationActions: [Deny] - paramRef: - parameterNotFoundAction: Allow - selector: {} diff --git a/config/admission/kustomization.yaml b/config/admission/kustomization.yaml deleted file mode 100644 index 4e1629d33..000000000 --- a/config/admission/kustomization.yaml +++ /dev/null @@ -1,5 +0,0 @@ -configurations: -- kustomizeconfig.yaml - -resources: -- admission.yaml diff --git a/config/admission/kustomizeconfig.yaml b/config/admission/kustomizeconfig.yaml deleted file mode 100644 index 55fc4088a..000000000 --- a/config/admission/kustomizeconfig.yaml +++ /dev/null @@ -1,9 +0,0 @@ -# This file is for teaching kustomize how to substitute name in ValidatingAdmissionPolicyBinding -# This might become obsolete depending on the outcome of https://github.com/kubernetes-sigs/kustomize/issues/5674 -nameReference: -- kind: ValidatingAdmissionPolicy - group: admissionregistration.k8s.io - fieldSpecs: - - kind: ValidatingAdmissionPolicyBinding - group: admissionregistration.k8s.io - path: spec/policyName diff --git a/config/default/kustomization.yaml b/config/default/kustomization.yaml index 2719a8565..6e2a672dd 100644 --- a/config/default/kustomization.yaml +++ b/config/default/kustomization.yaml @@ -16,7 +16,6 @@ namePrefix: operator-controller- resources: - ../crd -- ../admission - ../rbac - ../manager # [WEBHOOK] To enable webhook, uncomment all the sections with [WEBHOOK] prefix including the one in diff --git a/test/e2e/cluster_extension_admission_test.go b/test/e2e/cluster_extension_admission_test.go deleted file mode 100644 index b1cca6c3f..000000000 --- a/test/e2e/cluster_extension_admission_test.go +++ /dev/null @@ -1,103 +0,0 @@ -package e2e - -import ( - "context" - "fmt" - "testing" - - "github.com/stretchr/testify/require" - "k8s.io/apimachinery/pkg/api/errors" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/types" - "sigs.k8s.io/controller-runtime/pkg/client" - - ocv1alpha1 "github.com/operator-framework/operator-controller/api/v1alpha1" -) - -func TestClusterExtensionPackageUniqueness(t *testing.T) { - ctx := context.Background() - fieldOwner := client.FieldOwner("operator-controller-e2e") - - deleteClusterExtension := func(clusterExtension *ocv1alpha1.ClusterExtension) { - require.NoError(t, c.Delete(ctx, clusterExtension)) - require.Eventually(t, func() bool { - err := c.Get(ctx, types.NamespacedName{Name: clusterExtension.Name}, &ocv1alpha1.ClusterExtension{}) - return errors.IsNotFound(err) - }, pollDuration, pollInterval) - } - - const firstResourceName = "test-extension-first" - const firstResourcePackageName = "package1" - - t.Log("create first resource") - clusterExtension1 := &ocv1alpha1.ClusterExtension{ - ObjectMeta: metav1.ObjectMeta{ - Name: firstResourceName, - }, - Spec: ocv1alpha1.ClusterExtensionSpec{ - PackageName: firstResourcePackageName, - InstallNamespace: "default", - }, - } - require.NoError(t, c.Create(ctx, clusterExtension1)) - defer deleteClusterExtension(clusterExtension1) - - t.Log("create second resource with the same package as the first resource") - clusterExtension2 := &ocv1alpha1.ClusterExtension{ - ObjectMeta: metav1.ObjectMeta{ - GenerateName: "test-extension-", - }, - Spec: ocv1alpha1.ClusterExtensionSpec{ - PackageName: firstResourcePackageName, - InstallNamespace: "default", - }, - } - err := c.Create(ctx, clusterExtension2) - require.ErrorContains(t, err, fmt.Sprintf("Package %q is already installed via ClusterExtension %q", firstResourcePackageName, firstResourceName)) - - t.Log("create second resource with different package") - clusterExtension2 = &ocv1alpha1.ClusterExtension{ - ObjectMeta: metav1.ObjectMeta{ - GenerateName: "test-extension-", - }, - Spec: ocv1alpha1.ClusterExtensionSpec{ - PackageName: "package2", - InstallNamespace: "default", - }, - } - require.NoError(t, c.Create(ctx, clusterExtension2)) - defer deleteClusterExtension(clusterExtension2) - - t.Log("update second resource with package which already exists on the cluster") - intent := &ocv1alpha1.ClusterExtension{ - TypeMeta: metav1.TypeMeta{ - APIVersion: ocv1alpha1.GroupVersion.String(), - Kind: "ClusterExtension", - }, - ObjectMeta: metav1.ObjectMeta{ - Name: clusterExtension2.Name, - }, - Spec: ocv1alpha1.ClusterExtensionSpec{ - PackageName: firstResourcePackageName, - InstallNamespace: "default", - }, - } - err = c.Patch(ctx, intent, client.Apply, client.ForceOwnership, fieldOwner) - require.ErrorContains(t, err, fmt.Sprintf("Package %q is already installed via ClusterExtension %q", firstResourcePackageName, firstResourceName)) - - t.Log("update second resource with package which does not exist on the cluster") - intent = &ocv1alpha1.ClusterExtension{ - TypeMeta: metav1.TypeMeta{ - APIVersion: ocv1alpha1.GroupVersion.String(), - Kind: "ClusterExtension", - }, - ObjectMeta: metav1.ObjectMeta{ - Name: clusterExtension2.Name, - }, - Spec: ocv1alpha1.ClusterExtensionSpec{ - PackageName: "package3", - InstallNamespace: "default", - }, - } - require.NoError(t, c.Patch(ctx, intent, client.Apply, client.ForceOwnership, fieldOwner)) -}