From fbcc4a5c9c12e885213aac16fb0d4229f360a617 Mon Sep 17 00:00:00 2001 From: Joe Lanford Date: Wed, 26 Feb 2025 15:02:16 -0500 Subject: [PATCH] auth: use synthetic user/group when service account is not defined Signed-off-by: Joe Lanford --- api/v1/clusterextension_types.go | 15 +- api/v1/zz_generated.deepcopy.go | 6 +- cmd/operator-controller/main.go | 2 +- ...peratorframework.io_clusterextensions.yaml | 12 +- .../base/operator-controller/rbac/role.yaml | 7 + config/samples/olm_v1_clusterextension.yaml | 33 +-- .../operator-controller-api-reference.md | 2 +- .../operator-controller/action/restconfig.go | 37 +++- .../clusterextension_admission_test.go | 25 +-- .../clusterextension_controller.go | 1 + .../clusterextension_controller_test.go | 33 +-- .../resolve/catalog_test.go | 3 +- test/e2e/cluster_extension_install_test.go | 208 +++++++----------- .../extension_developer_test.go | 3 - 14 files changed, 165 insertions(+), 222 deletions(-) diff --git a/api/v1/clusterextension_types.go b/api/v1/clusterextension_types.go index 0141f1a7a..ba83d302e 100644 --- a/api/v1/clusterextension_types.go +++ b/api/v1/clusterextension_types.go @@ -66,10 +66,19 @@ type ClusterExtensionSpec struct { // with the cluster that are required to manage the extension. // The ServiceAccount must be configured with the necessary permissions to perform these interactions. // The ServiceAccount must exist in the namespace referenced in the spec. - // serviceAccount is required. // - // +kubebuilder:validation:Required - ServiceAccount ServiceAccountReference `json:"serviceAccount"` + // serviceAccount is optional. If a service account is not defined, requests to the apiserver will instead use + // username "olmv1:clusterextensions::admin" and groups + // "olmv1:clusterextensions:admin" and "system:authenticated" + // + // Deprecated: Use of serviceAccount is not recommended. Instead, administrators are encouraged + // to use the synthetic user/groups described above. All of the same RBAC setup is still required with these + // synthetic user/groups. However, this mode is preferred because it requires administrators to specifically + // configure RBAC for extension management, rather than enabling piggybacking on existing highly privileged + // service accounts that already exist on the cluster. + // + // +optional + ServiceAccount *ServiceAccountReference `json:"serviceAccount"` // source is a required field which selects the installation source of content // for this ClusterExtension. Selection is performed by setting the sourceType. diff --git a/api/v1/zz_generated.deepcopy.go b/api/v1/zz_generated.deepcopy.go index 37694f61f..45a3e7de2 100644 --- a/api/v1/zz_generated.deepcopy.go +++ b/api/v1/zz_generated.deepcopy.go @@ -324,7 +324,11 @@ func (in *ClusterExtensionList) DeepCopyObject() runtime.Object { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *ClusterExtensionSpec) DeepCopyInto(out *ClusterExtensionSpec) { *out = *in - out.ServiceAccount = in.ServiceAccount + if in.ServiceAccount != nil { + in, out := &in.ServiceAccount, &out.ServiceAccount + *out = new(ServiceAccountReference) + **out = **in + } in.Source.DeepCopyInto(&out.Source) if in.Install != nil { in, out := &in.Install, &out.Install diff --git a/cmd/operator-controller/main.go b/cmd/operator-controller/main.go index ee6450a05..70a50d5bb 100644 --- a/cmd/operator-controller/main.go +++ b/cmd/operator-controller/main.go @@ -298,7 +298,7 @@ func run() error { return err } tokenGetter := authentication.NewTokenGetter(coreClient, authentication.WithExpirationDuration(1*time.Hour)) - clientRestConfigMapper := action.ServiceAccountRestConfigMapper(tokenGetter) + clientRestConfigMapper := action.ClusterExtensionUserRestConfigMapper(tokenGetter) cfgGetter, err := helmclient.NewActionConfigGetter(mgr.GetConfig(), mgr.GetRESTMapper(), helmclient.StorageDriverMapper(action.ChunkedStorageDriverMapper(coreClient, mgr.GetAPIReader(), cfg.systemNamespace)), diff --git a/config/base/operator-controller/crd/bases/olm.operatorframework.io_clusterextensions.yaml b/config/base/operator-controller/crd/bases/olm.operatorframework.io_clusterextensions.yaml index e54b68518..e8f6697e9 100644 --- a/config/base/operator-controller/crd/bases/olm.operatorframework.io_clusterextensions.yaml +++ b/config/base/operator-controller/crd/bases/olm.operatorframework.io_clusterextensions.yaml @@ -135,7 +135,16 @@ spec: with the cluster that are required to manage the extension. The ServiceAccount must be configured with the necessary permissions to perform these interactions. The ServiceAccount must exist in the namespace referenced in the spec. - serviceAccount is required. + + serviceAccount is optional. If a service account is not defined, requests to the apiserver will instead use + username "olmv1:clusterextensions::admin" and groups + "olmv1:clusterextensions:admin" and "system:authenticated" + + Deprecated: Use of serviceAccount is not recommended. Instead, administrators are encouraged + to use the synthetic user/groups described above. All of the same RBAC setup is still required with these + synthetic user/groups. However, this mode is preferred because it requires administrators to specifically + configure RBAC for extension management, rather than enabling piggybacking on existing highly privileged + service accounts that already exist on the cluster. properties: name: description: |- @@ -458,7 +467,6 @@ spec: has(self.catalog) : !has(self.catalog)' required: - namespace - - serviceAccount - source type: object status: diff --git a/config/base/operator-controller/rbac/role.yaml b/config/base/operator-controller/rbac/role.yaml index a929e78e9..a59440a31 100644 --- a/config/base/operator-controller/rbac/role.yaml +++ b/config/base/operator-controller/rbac/role.yaml @@ -4,6 +4,13 @@ kind: ClusterRole metadata: name: manager-role rules: +- apiGroups: + - "" + resources: + - groups + - users + verbs: + - impersonate - apiGroups: - "" resources: diff --git a/config/samples/olm_v1_clusterextension.yaml b/config/samples/olm_v1_clusterextension.yaml index 4438dfb76..51d4fc50b 100644 --- a/config/samples/olm_v1_clusterextension.yaml +++ b/config/samples/olm_v1_clusterextension.yaml @@ -2,13 +2,7 @@ apiVersion: v1 kind: Namespace metadata: - name: argocd ---- -apiVersion: v1 -kind: ServiceAccount -metadata: - name: argocd-installer - namespace: argocd + name: argocd-system --- apiVersion: rbac.authorization.k8s.io/v1 kind: ClusterRoleBinding @@ -19,9 +13,8 @@ roleRef: kind: ClusterRole name: argocd-installer-clusterrole subjects: -- kind: ServiceAccount - name: argocd-installer - namespace: argocd +- kind: User + name: "olmv1:clusterextensions:argocd-operator:admin" --- apiVersion: rbac.authorization.k8s.io/v1 kind: ClusterRole @@ -76,9 +69,8 @@ roleRef: kind: ClusterRole name: argocd-installer-rbac-clusterrole subjects: -- kind: ServiceAccount - name: argocd-installer - namespace: argocd +- kind: User + name: "olmv1:clusterextensions:argocd-operator:admin" --- apiVersion: rbac.authorization.k8s.io/v1 kind: ClusterRole @@ -222,7 +214,7 @@ apiVersion: rbac.authorization.k8s.io/v1 kind: Role metadata: name: argocd-installer-role - namespace: argocd + namespace: argocd-system rules: - apiGroups: [""] resources: [serviceaccounts] @@ -260,24 +252,21 @@ apiVersion: rbac.authorization.k8s.io/v1 kind: RoleBinding metadata: name: argocd-installer-binding - namespace: argocd + namespace: argocd-system roleRef: apiGroup: rbac.authorization.k8s.io kind: Role name: argocd-installer-role subjects: -- kind: ServiceAccount - name: argocd-installer - namespace: argocd +- kind: User + name: "olmv1:clusterextensions:argocd-operator:admin" --- apiVersion: olm.operatorframework.io/v1 kind: ClusterExtension metadata: - name: argocd + name: argocd-operator spec: - namespace: argocd - serviceAccount: - name: argocd-installer + namespace: argocd-system source: sourceType: Catalog catalog: diff --git a/docs/api-reference/operator-controller-api-reference.md b/docs/api-reference/operator-controller-api-reference.md index 84fdbfa64..24dfabf3b 100644 --- a/docs/api-reference/operator-controller-api-reference.md +++ b/docs/api-reference/operator-controller-api-reference.md @@ -306,7 +306,7 @@ _Appears in:_ | Field | Description | Default | Validation | | --- | --- | --- | --- | | `namespace` _string_ | namespace is a reference to a Kubernetes namespace.
This is the namespace in which the provided ServiceAccount must exist.
It also designates the default namespace where namespace-scoped resources
for the extension are applied to the cluster.
Some extensions may contain namespace-scoped resources to be applied in other namespaces.
This namespace must exist.

namespace is required, immutable, and follows the DNS label standard
as defined in [RFC 1123]. It must contain only lowercase alphanumeric characters or hyphens (-),
start and end with an alphanumeric character, and be no longer than 63 characters

[RFC 1123]: https://tools.ietf.org/html/rfc1123 | | MaxLength: 63
Required: \{\}
| -| `serviceAccount` _[ServiceAccountReference](#serviceaccountreference)_ | serviceAccount is a reference to a ServiceAccount used to perform all interactions
with the cluster that are required to manage the extension.
The ServiceAccount must be configured with the necessary permissions to perform these interactions.
The ServiceAccount must exist in the namespace referenced in the spec.
serviceAccount is required. | | Required: \{\}
| +| `serviceAccount` _[ServiceAccountReference](#serviceaccountreference)_ | serviceAccount is a reference to a ServiceAccount used to perform all interactions
with the cluster that are required to manage the extension.
The ServiceAccount must be configured with the necessary permissions to perform these interactions.
The ServiceAccount must exist in the namespace referenced in the spec.

serviceAccount is optional. If a service account is not defined, requests to the apiserver will instead use
username "olmv1:clusterextensions::admin" and groups
"olmv1:clusterextensions:admin" and "system:authenticated"

Deprecated: Use of serviceAccount is not recommended. Instead, administrators are encouraged
to use the synthetic user/groups described above. All of the same RBAC setup is still required with these
synthetic user/groups. However, this mode is preferred because it requires administrators to specifically
configure RBAC for extension management, rather than enabling piggybacking on existing highly privileged
service accounts that already exist on the cluster. | | | | `source` _[SourceConfig](#sourceconfig)_ | source is a required field which selects the installation source of content
for this ClusterExtension. Selection is performed by setting the sourceType.

Catalog is currently the only implemented sourceType, and setting the
sourcetype to "Catalog" requires the catalog field to also be defined.

Below is a minimal example of a source definition (in yaml):

source:
sourceType: Catalog
catalog:
packageName: example-package | | Required: \{\}
| | `install` _[ClusterExtensionInstallConfig](#clusterextensioninstallconfig)_ | install is an optional field used to configure the installation options
for the ClusterExtension such as the pre-flight check configuration. | | | diff --git a/internal/operator-controller/action/restconfig.go b/internal/operator-controller/action/restconfig.go index 6e0121281..7f735cd74 100644 --- a/internal/operator-controller/action/restconfig.go +++ b/internal/operator-controller/action/restconfig.go @@ -2,21 +2,54 @@ package action import ( "context" + "fmt" "net/http" "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/rest" + "k8s.io/client-go/transport" "sigs.k8s.io/controller-runtime/pkg/client" ocv1 "github.com/operator-framework/operator-controller/api/v1" "github.com/operator-framework/operator-controller/internal/operator-controller/authentication" ) -func ServiceAccountRestConfigMapper(tokenGetter *authentication.TokenGetter) func(ctx context.Context, o client.Object, c *rest.Config) (*rest.Config, error) { +func ClusterExtensionUserRestConfigMapper(tokenGetter *authentication.TokenGetter) func(ctx context.Context, o client.Object, c *rest.Config) (*rest.Config, error) { + saRestConfigMapper := serviceAccountRestConfigMapper(tokenGetter) + synthRestConfigMapper := sythenticUserRestConfigMapper() + + return func(ctx context.Context, o client.Object, c *rest.Config) (*rest.Config, error) { + cExt := o.(*ocv1.ClusterExtension) + if cExt.Spec.ServiceAccount != nil { //nolint:staticcheck + return saRestConfigMapper(ctx, o, c) + } + return synthRestConfigMapper(ctx, o, c) + } +} + +func sythenticUserRestConfigMapper() func(ctx context.Context, o client.Object, c *rest.Config) (*rest.Config, error) { + return func(ctx context.Context, o client.Object, c *rest.Config) (*rest.Config, error) { + cExt := o.(*ocv1.ClusterExtension) + cc := rest.CopyConfig(c) + cc.Wrap(func(rt http.RoundTripper) http.RoundTripper { + impersonateConfig := transport.ImpersonationConfig{ + UserName: fmt.Sprintf("olmv1:clusterextensions:%s:admin", cExt.Name), + Groups: []string{ + "system:authenticated", + "olmv1:clusterextensions:admin", + }, + } + return transport.NewImpersonatingRoundTripper(impersonateConfig, rt) + }) + return cc, nil + } +} + +func serviceAccountRestConfigMapper(tokenGetter *authentication.TokenGetter) func(ctx context.Context, o client.Object, c *rest.Config) (*rest.Config, error) { return func(ctx context.Context, o client.Object, c *rest.Config) (*rest.Config, error) { cExt := o.(*ocv1.ClusterExtension) saKey := types.NamespacedName{ - Name: cExt.Spec.ServiceAccount.Name, + Name: cExt.Spec.ServiceAccount.Name, //nolint:staticcheck Namespace: cExt.Spec.Namespace, } saConfig := rest.AnonymousClientConfig(c) diff --git a/internal/operator-controller/controllers/clusterextension_admission_test.go b/internal/operator-controller/controllers/clusterextension_admission_test.go index 3bf58fd48..6ba037ad5 100644 --- a/internal/operator-controller/controllers/clusterextension_admission_test.go +++ b/internal/operator-controller/controllers/clusterextension_admission_test.go @@ -44,9 +44,6 @@ func TestClusterExtensionSourceConfig(t *testing.T) { }, }, Namespace: "default", - ServiceAccount: ocv1.ServiceAccountReference{ - Name: "default", - }, })) } if tc.unionField == "" { @@ -55,9 +52,6 @@ func TestClusterExtensionSourceConfig(t *testing.T) { SourceType: tc.sourceType, }, Namespace: "default", - ServiceAccount: ocv1.ServiceAccountReference{ - Name: "default", - }, })) } @@ -114,9 +108,6 @@ func TestClusterExtensionAdmissionPackageName(t *testing.T) { }, }, Namespace: "default", - ServiceAccount: ocv1.ServiceAccountReference{ - Name: "default", - }, })) if tc.errMsg == "" { require.NoError(t, err, "unexpected error for package name %q: %w", tc.pkgName, err) @@ -212,9 +203,6 @@ func TestClusterExtensionAdmissionVersion(t *testing.T) { }, }, Namespace: "default", - ServiceAccount: ocv1.ServiceAccountReference{ - Name: "default", - }, })) if tc.errMsg == "" { require.NoError(t, err, "unexpected error for version %q: %w", tc.version, err) @@ -267,9 +255,6 @@ func TestClusterExtensionAdmissionChannel(t *testing.T) { }, }, Namespace: "default", - ServiceAccount: ocv1.ServiceAccountReference{ - Name: "default", - }, })) if tc.errMsg == "" { require.NoError(t, err, "unexpected error for channel %q: %w", tc.channels, err) @@ -320,9 +305,6 @@ func TestClusterExtensionAdmissionInstallNamespace(t *testing.T) { }, }, Namespace: tc.namespace, - ServiceAccount: ocv1.ServiceAccountReference{ - Name: "default", - }, })) if tc.errMsg == "" { require.NoError(t, err, "unexpected error for namespace %q: %w", tc.namespace, err) @@ -374,7 +356,7 @@ func TestClusterExtensionAdmissionServiceAccount(t *testing.T) { }, }, Namespace: "default", - ServiceAccount: ocv1.ServiceAccountReference{ + ServiceAccount: &ocv1.ServiceAccountReference{ Name: tc.serviceAccount, }, })) @@ -433,10 +415,7 @@ func TestClusterExtensionAdmissionInstall(t *testing.T) { }, }, Namespace: "default", - ServiceAccount: ocv1.ServiceAccountReference{ - Name: "default", - }, - Install: tc.installConfig, + Install: tc.installConfig, })) if tc.errMsg == "" { require.NoError(t, err, "unexpected error for install configuration %v: %w", tc.installConfig, err) diff --git a/internal/operator-controller/controllers/clusterextension_controller.go b/internal/operator-controller/controllers/clusterextension_controller.go index d914b831b..b7a64081e 100644 --- a/internal/operator-controller/controllers/clusterextension_controller.go +++ b/internal/operator-controller/controllers/clusterextension_controller.go @@ -95,6 +95,7 @@ type InstalledBundleGetter interface { //+kubebuilder:rbac:groups=olm.operatorframework.io,resources=clusterextensions/finalizers,verbs=update //+kubebuilder:rbac:namespace=system,groups=core,resources=secrets,verbs=create;update;patch;delete;deletecollection;get;list;watch //+kubebuilder:rbac:groups=core,resources=serviceaccounts/token,verbs=create +//+kubebuilder:rbac:groups=core,resources=users;groups,verbs=impersonate //+kubebuilder:rbac:groups=apiextensions.k8s.io,resources=customresourcedefinitions,verbs=get //+kubebuilder:rbac:groups=olm.operatorframework.io,resources=clustercatalogs,verbs=list;watch diff --git a/internal/operator-controller/controllers/clusterextension_controller_test.go b/internal/operator-controller/controllers/clusterextension_controller_test.go index be61891a0..8447c5954 100644 --- a/internal/operator-controller/controllers/clusterextension_controller_test.go +++ b/internal/operator-controller/controllers/clusterextension_controller_test.go @@ -69,9 +69,6 @@ func TestClusterExtensionResolutionFails(t *testing.T) { }, }, Namespace: "default", - ServiceAccount: ocv1.ServiceAccountReference{ - Name: "default", - }, }, } require.NoError(t, cl.Create(ctx, clusterExtension)) @@ -131,7 +128,6 @@ func TestClusterExtensionResolutionSuccessfulUnpackFails(t *testing.T) { pkgVer := "1.0.0" pkgChan := "beta" namespace := fmt.Sprintf("test-ns-%s", rand.String(8)) - serviceAccount := fmt.Sprintf("test-sa-%s", rand.String(8)) clusterExtension := &ocv1.ClusterExtension{ ObjectMeta: metav1.ObjectMeta{Name: extKey.Name}, @@ -145,9 +141,6 @@ func TestClusterExtensionResolutionSuccessfulUnpackFails(t *testing.T) { }, }, Namespace: namespace, - ServiceAccount: ocv1.ServiceAccountReference{ - Name: serviceAccount, - }, }, } err := cl.Create(ctx, clusterExtension) @@ -211,7 +204,6 @@ func TestClusterExtensionResolutionAndUnpackSuccessfulApplierFails(t *testing.T) pkgVer := "1.0.0" pkgChan := "beta" namespace := fmt.Sprintf("test-ns-%s", rand.String(8)) - serviceAccount := fmt.Sprintf("test-sa-%s", rand.String(8)) clusterExtension := &ocv1.ClusterExtension{ ObjectMeta: metav1.ObjectMeta{Name: extKey.Name}, @@ -225,9 +217,6 @@ func TestClusterExtensionResolutionAndUnpackSuccessfulApplierFails(t *testing.T) }, }, Namespace: namespace, - ServiceAccount: ocv1.ServiceAccountReference{ - Name: serviceAccount, - }, }, } err := cl.Create(ctx, clusterExtension) @@ -295,7 +284,7 @@ func TestClusterExtensionServiceAccountNotFound(t *testing.T) { }, }, Namespace: "default", - ServiceAccount: ocv1.ServiceAccountReference{ + ServiceAccount: &ocv1.ServiceAccountReference{ Name: "missing-sa", }, }, @@ -342,7 +331,6 @@ func TestClusterExtensionApplierFailsWithBundleInstalled(t *testing.T) { pkgVer := "1.0.0" pkgChan := "beta" namespace := fmt.Sprintf("test-ns-%s", rand.String(8)) - serviceAccount := fmt.Sprintf("test-sa-%s", rand.String(8)) clusterExtension := &ocv1.ClusterExtension{ ObjectMeta: metav1.ObjectMeta{Name: extKey.Name}, @@ -356,9 +344,6 @@ func TestClusterExtensionApplierFailsWithBundleInstalled(t *testing.T) { }, }, Namespace: namespace, - ServiceAccount: ocv1.ServiceAccountReference{ - Name: serviceAccount, - }, }, } err := cl.Create(ctx, clusterExtension) @@ -438,7 +423,6 @@ func TestClusterExtensionManagerFailed(t *testing.T) { pkgVer := "1.0.0" pkgChan := "beta" namespace := fmt.Sprintf("test-ns-%s", rand.String(8)) - serviceAccount := fmt.Sprintf("test-sa-%s", rand.String(8)) clusterExtension := &ocv1.ClusterExtension{ ObjectMeta: metav1.ObjectMeta{Name: extKey.Name}, @@ -452,9 +436,6 @@ func TestClusterExtensionManagerFailed(t *testing.T) { }, }, Namespace: namespace, - ServiceAccount: ocv1.ServiceAccountReference{ - Name: serviceAccount, - }, }, } err := cl.Create(ctx, clusterExtension) @@ -516,7 +497,6 @@ func TestClusterExtensionManagedContentCacheWatchFail(t *testing.T) { pkgVer := "1.0.0" pkgChan := "beta" installNamespace := fmt.Sprintf("test-ns-%s", rand.String(8)) - serviceAccount := fmt.Sprintf("test-sa-%s", rand.String(8)) clusterExtension := &ocv1.ClusterExtension{ ObjectMeta: metav1.ObjectMeta{Name: extKey.Name}, @@ -531,9 +511,6 @@ func TestClusterExtensionManagedContentCacheWatchFail(t *testing.T) { }, }, Namespace: installNamespace, - ServiceAccount: ocv1.ServiceAccountReference{ - Name: serviceAccount, - }, }, } err := cl.Create(ctx, clusterExtension) @@ -597,7 +574,6 @@ func TestClusterExtensionInstallationSucceeds(t *testing.T) { pkgVer := "1.0.0" pkgChan := "beta" namespace := fmt.Sprintf("test-ns-%s", rand.String(8)) - serviceAccount := fmt.Sprintf("test-sa-%s", rand.String(8)) clusterExtension := &ocv1.ClusterExtension{ ObjectMeta: metav1.ObjectMeta{Name: extKey.Name}, @@ -611,9 +587,6 @@ func TestClusterExtensionInstallationSucceeds(t *testing.T) { }, }, Namespace: namespace, - ServiceAccount: ocv1.ServiceAccountReference{ - Name: serviceAccount, - }, }, } err := cl.Create(ctx, clusterExtension) @@ -675,7 +648,6 @@ func TestClusterExtensionDeleteFinalizerFails(t *testing.T) { pkgVer := "1.0.0" pkgChan := "beta" namespace := fmt.Sprintf("test-ns-%s", rand.String(8)) - serviceAccount := fmt.Sprintf("test-sa-%s", rand.String(8)) clusterExtension := &ocv1.ClusterExtension{ ObjectMeta: metav1.ObjectMeta{Name: extKey.Name}, @@ -689,9 +661,6 @@ func TestClusterExtensionDeleteFinalizerFails(t *testing.T) { }, }, Namespace: namespace, - ServiceAccount: ocv1.ServiceAccountReference{ - Name: serviceAccount, - }, }, } err := cl.Create(ctx, clusterExtension) diff --git a/internal/operator-controller/resolve/catalog_test.go b/internal/operator-controller/resolve/catalog_test.go index 00467253e..1d28fedcb 100644 --- a/internal/operator-controller/resolve/catalog_test.go +++ b/internal/operator-controller/resolve/catalog_test.go @@ -585,8 +585,7 @@ func buildFooClusterExtension(pkg string, channels []string, version string, upg Name: pkg, }, Spec: ocv1.ClusterExtensionSpec{ - Namespace: "default", - ServiceAccount: ocv1.ServiceAccountReference{Name: "default"}, + Namespace: "default", Source: ocv1.SourceConfig{ SourceType: "Catalog", Catalog: &ocv1.CatalogFilter{ diff --git a/test/e2e/cluster_extension_install_test.go b/test/e2e/cluster_extension_install_test.go index a01124bfb..357bbccff 100644 --- a/test/e2e/cluster_extension_install_test.go +++ b/test/e2e/cluster_extension_install_test.go @@ -10,6 +10,7 @@ import ( "github.com/google/go-containerregistry/pkg/crane" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "golang.org/x/sync/errgroup" corev1 "k8s.io/api/core/v1" rbacv1 "k8s.io/api/rbac/v1" apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" @@ -19,6 +20,7 @@ import ( "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/rand" + "k8s.io/apimachinery/pkg/util/wait" "sigs.k8s.io/controller-runtime/pkg/client" ocv1 "github.com/operator-framework/operator-controller/api/v1" @@ -45,25 +47,10 @@ func createNamespace(ctx context.Context, name string) (*corev1.Namespace, error return ns, nil } -func createServiceAccount(ctx context.Context, name types.NamespacedName, clusterExtensionName string) (*corev1.ServiceAccount, error) { - sa := &corev1.ServiceAccount{ - ObjectMeta: metav1.ObjectMeta{ - Name: name.Name, - Namespace: name.Namespace, - }, - } - err := c.Create(ctx, sa) - if err != nil { - return nil, err - } - - return sa, createClusterRoleAndBindingForSA(ctx, name.Name, sa, clusterExtensionName) -} - -func createClusterRoleAndBindingForSA(ctx context.Context, name string, sa *corev1.ServiceAccount, clusterExtensionName string) error { +func createClusterExtensionPermissions(ctx context.Context, clusterExtensionName string) (*rbacv1.ClusterRole, *rbacv1.ClusterRoleBinding, error) { cr := &rbacv1.ClusterRole{ ObjectMeta: metav1.ObjectMeta{ - Name: name, + Name: clusterExtensionName, }, Rules: []rbacv1.PolicyRule{ { @@ -158,36 +145,41 @@ func createClusterRoleAndBindingForSA(ctx context.Context, name string, sa *core } err := c.Create(ctx, cr) if err != nil { - return err + return nil, nil, err } crb := &rbacv1.ClusterRoleBinding{ ObjectMeta: metav1.ObjectMeta{ - Name: name, + Name: clusterExtensionName, }, Subjects: []rbacv1.Subject{ { - Kind: "ServiceAccount", - Name: sa.Name, - Namespace: sa.Namespace, + Kind: "User", + // TODO: Define a helper method for this somewhere. + Name: fmt.Sprintf("olmv1:clusterextensions:%s:admin", clusterExtensionName), }, }, RoleRef: rbacv1.RoleRef{ APIGroup: "rbac.authorization.k8s.io", Kind: "ClusterRole", - Name: name, + Name: clusterExtensionName, }, } err = c.Create(ctx, crb) if err != nil { - return err + return nil, nil, err } - return nil + return cr, crb, nil } -func testInit(t *testing.T) (*ocv1.ClusterExtension, *ocv1.ClusterCatalog, *corev1.ServiceAccount, *corev1.Namespace) { - var err error +func testInit(t *testing.T) (*ocv1.ClusterExtension, *ocv1.ClusterCatalog, *rbacv1.ClusterRole, *rbacv1.ClusterRoleBinding, *corev1.Namespace) { + clusterExtension, extensionCatalog, ns := testInitNoPermissions(t) + clusterRole, clusterRoleBinding, err := createClusterExtensionPermissions(context.Background(), clusterExtension.Name) + require.NoError(t, err) + return clusterExtension, extensionCatalog, clusterRole, clusterRoleBinding, ns +} +func testInitNoPermissions(t *testing.T) (*ocv1.ClusterExtension, *ocv1.ClusterCatalog, *corev1.Namespace) { clusterExtensionName := fmt.Sprintf("clusterextension-%s", rand.String(8)) ns, err := createNamespace(context.Background(), clusterExtensionName) @@ -202,17 +194,9 @@ func testInit(t *testing.T) (*ocv1.ClusterExtension, *ocv1.ClusterCatalog, *core extensionCatalog, err := createTestCatalog(context.Background(), testCatalogName, os.Getenv(testCatalogRefEnvVar)) require.NoError(t, err) - name := types.NamespacedName{ - Name: clusterExtensionName, - Namespace: ns.GetName(), - } - - sa, err := createServiceAccount(context.Background(), name, clusterExtensionName) - require.NoError(t, err) - validateCatalogUnpack(t) - return clusterExtension, extensionCatalog, sa, ns + return clusterExtension, extensionCatalog, ns } func validateCatalogUnpack(t *testing.T) { @@ -273,36 +257,38 @@ func ensureNoExtensionResources(t *testing.T, clusterExtensionName string) { }, 2*pollDuration, pollInterval) } -func testCleanup(t *testing.T, cat *ocv1.ClusterCatalog, clusterExtension *ocv1.ClusterExtension, sa *corev1.ServiceAccount, ns *corev1.Namespace) { - t.Logf("By deleting ClusterCatalog %q", cat.Name) - require.NoError(t, c.Delete(context.Background(), cat)) - require.Eventually(t, func() bool { - err := c.Get(context.Background(), types.NamespacedName{Name: cat.Name}, &ocv1.ClusterCatalog{}) - return errors.IsNotFound(err) - }, pollDuration, pollInterval) - - t.Logf("By deleting ClusterExtension %q", clusterExtension.Name) - require.NoError(t, c.Delete(context.Background(), clusterExtension)) - require.Eventually(t, func() bool { - err := c.Get(context.Background(), types.NamespacedName{Name: clusterExtension.Name}, &ocv1.ClusterExtension{}) - return errors.IsNotFound(err) - }, pollDuration, pollInterval) - - t.Logf("By deleting ServiceAccount %q", sa.Name) - require.NoError(t, c.Delete(context.Background(), sa)) - require.Eventually(t, func() bool { - err := c.Get(context.Background(), types.NamespacedName{Name: sa.Name, Namespace: sa.Namespace}, &corev1.ServiceAccount{}) - return errors.IsNotFound(err) - }, pollDuration, pollInterval) - - ensureNoExtensionResources(t, clusterExtension.Name) - - t.Logf("By deleting Namespace %q", ns.Name) - require.NoError(t, c.Delete(context.Background(), ns)) - require.Eventually(t, func() bool { - err := c.Get(context.Background(), types.NamespacedName{Name: ns.Name}, &corev1.Namespace{}) - return errors.IsNotFound(err) - }, pollDuration, pollInterval) +func testCleanup(t *testing.T, objs ...client.Object) { + ctx, cancel := context.WithTimeout(context.Background(), pollDuration) + defer cancel() + eg, ctx := errgroup.WithContext(ctx) + + var clusterExtensionNames []string + for _, obj := range objs { + eg.Go(func() error { + gvk, err := c.GroupVersionKindFor(obj) + if err != nil { + return err + } + if gvk.Kind == "ClusterExtension" { + clusterExtensionNames = append(clusterExtensionNames, obj.GetName()) + } + t.Logf("By deleting %s %s", gvk.Kind, obj.GetName()) + if err := c.Delete(ctx, obj); err != nil && !errors.IsNotFound(err) { + return err + } + return wait.PollUntilContextCancel(ctx, pollInterval, true, func(ctx context.Context) (bool, error) { + err := c.Get(ctx, client.ObjectKeyFromObject(obj), obj) + if errors.IsNotFound(err) { + return true, nil + } + return false, err + }) + }) + } + require.NoError(t, eg.Wait()) + for _, clusterExtensionName := range clusterExtensionNames { + ensureNoExtensionResources(t, clusterExtensionName) + } } func TestClusterExtensionInstallRegistry(t *testing.T) { @@ -328,8 +314,8 @@ func TestClusterExtensionInstallRegistry(t *testing.T) { t.Log("When a cluster extension is installed from a catalog") t.Log("When the extension bundle format is registry+v1") - clusterExtension, extensionCatalog, sa, ns := testInit(t) - defer testCleanup(t, extensionCatalog, clusterExtension, sa, ns) + clusterExtension, extensionCatalog, clusterRole, clusterRoleBinding, ns := testInit(t) + defer testCleanup(t, extensionCatalog, clusterExtension, clusterRole, clusterRoleBinding, ns) defer utils.CollectTestArtifacts(t, artifactName, c, cfg) clusterExtension.Spec = ocv1.ClusterExtensionSpec{ @@ -343,9 +329,6 @@ func TestClusterExtensionInstallRegistry(t *testing.T) { }, }, Namespace: ns.Name, - ServiceAccount: ocv1.ServiceAccountReference{ - Name: sa.Name, - }, } t.Log("It resolves the specified package with correct bundle path") t.Log("By creating the ClusterExtension resource") @@ -388,8 +371,8 @@ func TestClusterExtensionInstallRegistryDynamic(t *testing.T) { t.Log("When a cluster extension is installed from a catalog") t.Log("When the extension bundle format is registry+v1") - clusterExtension, extensionCatalog, sa, ns := testInit(t) - defer testCleanup(t, extensionCatalog, clusterExtension, sa, ns) + clusterExtension, extensionCatalog, clusterRole, clusterRoleBinding, ns := testInit(t) + defer testCleanup(t, extensionCatalog, clusterExtension, clusterRole, clusterRoleBinding, ns) defer utils.CollectTestArtifacts(t, artifactName, c, cfg) clusterExtension.Spec = ocv1.ClusterExtensionSpec{ @@ -403,9 +386,6 @@ func TestClusterExtensionInstallRegistryDynamic(t *testing.T) { }, }, Namespace: ns.Name, - ServiceAccount: ocv1.ServiceAccountReference{ - Name: sa.Name, - }, } t.Log("It updates the registries.conf file contents") cm := corev1.ConfigMap{ @@ -460,11 +440,11 @@ location = "docker-registry.operator-controller-e2e.svc.cluster.local:5000"`, func TestClusterExtensionInstallRegistryMultipleBundles(t *testing.T) { t.Log("When a cluster extension is installed from a catalog") - clusterExtension, extensionCatalog, sa, ns := testInit(t) + clusterExtension, extensionCatalog, clusterRole, clusterRoleBinding, ns := testInit(t) extraCatalog, err := createTestCatalog(context.Background(), "extra-test-catalog", os.Getenv(testCatalogRefEnvVar)) require.NoError(t, err) - defer testCleanup(t, extensionCatalog, clusterExtension, sa, ns) + defer testCleanup(t, extensionCatalog, clusterExtension, clusterRole, clusterRoleBinding, ns) defer utils.CollectTestArtifacts(t, artifactName, c, cfg) defer func(cat *ocv1.ClusterCatalog) { require.NoError(t, c.Delete(context.Background(), cat)) @@ -482,9 +462,6 @@ func TestClusterExtensionInstallRegistryMultipleBundles(t *testing.T) { }, }, Namespace: ns.Name, - ServiceAccount: ocv1.ServiceAccountReference{ - Name: sa.Name, - }, } t.Log("It resolves to multiple bundle paths") t.Log("By creating the ClusterExtension resource") @@ -511,8 +488,8 @@ func TestClusterExtensionBlockInstallNonSuccessorVersion(t *testing.T) { t.Log("When a cluster extension is installed from a catalog") t.Log("When resolving upgrade edges") - clusterExtension, extensionCatalog, sa, ns := testInit(t) - defer testCleanup(t, extensionCatalog, clusterExtension, sa, ns) + clusterExtension, extensionCatalog, clusterRole, clusterRoleBinding, ns := testInit(t) + defer testCleanup(t, extensionCatalog, clusterExtension, clusterRole, clusterRoleBinding, ns) defer utils.CollectTestArtifacts(t, artifactName, c, cfg) t.Log("By creating an ClusterExtension at a specified version") @@ -526,9 +503,6 @@ func TestClusterExtensionBlockInstallNonSuccessorVersion(t *testing.T) { }, }, Namespace: ns.Name, - ServiceAccount: ocv1.ServiceAccountReference{ - Name: sa.Name, - }, } require.NoError(t, c.Create(context.Background(), clusterExtension)) t.Log("By eventually reporting a successful installation") @@ -574,8 +548,8 @@ func TestClusterExtensionForceInstallNonSuccessorVersion(t *testing.T) { t.Log("When a cluster extension is installed from a catalog") t.Log("When resolving upgrade edges") - clusterExtension, extensionCatalog, sa, ns := testInit(t) - defer testCleanup(t, extensionCatalog, clusterExtension, sa, ns) + clusterExtension, extensionCatalog, clusterRole, clusterRoleBinding, ns := testInit(t) + defer testCleanup(t, extensionCatalog, clusterExtension, clusterRole, clusterRoleBinding, ns) defer utils.CollectTestArtifacts(t, artifactName, c, cfg) t.Log("By creating an ClusterExtension at a specified version") @@ -588,9 +562,6 @@ func TestClusterExtensionForceInstallNonSuccessorVersion(t *testing.T) { }, }, Namespace: ns.Name, - ServiceAccount: ocv1.ServiceAccountReference{ - Name: sa.Name, - }, } require.NoError(t, c.Create(context.Background(), clusterExtension)) t.Log("By eventually reporting a successful resolution") @@ -623,8 +594,8 @@ func TestClusterExtensionForceInstallNonSuccessorVersion(t *testing.T) { func TestClusterExtensionInstallSuccessorVersion(t *testing.T) { t.Log("When a cluster extension is installed from a catalog") t.Log("When resolving upgrade edges") - clusterExtension, extensionCatalog, sa, ns := testInit(t) - defer testCleanup(t, extensionCatalog, clusterExtension, sa, ns) + clusterExtension, extensionCatalog, clusterRole, clusterRoleBinding, ns := testInit(t) + defer testCleanup(t, extensionCatalog, clusterExtension, clusterRole, clusterRoleBinding, ns) defer utils.CollectTestArtifacts(t, artifactName, c, cfg) t.Log("By creating an ClusterExtension at a specified version") @@ -637,9 +608,6 @@ func TestClusterExtensionInstallSuccessorVersion(t *testing.T) { }, }, Namespace: ns.Name, - ServiceAccount: ocv1.ServiceAccountReference{ - Name: sa.Name, - }, } require.NoError(t, c.Create(context.Background(), clusterExtension)) t.Log("By eventually reporting a successful resolution") @@ -671,8 +639,8 @@ func TestClusterExtensionInstallSuccessorVersion(t *testing.T) { func TestClusterExtensionInstallReResolvesWhenCatalogIsPatched(t *testing.T) { t.Log("When a cluster extension is installed from a catalog") t.Log("It resolves again when a catalog is patched with new ImageRef") - clusterExtension, extensionCatalog, sa, ns := testInit(t) - defer testCleanup(t, extensionCatalog, clusterExtension, sa, ns) + clusterExtension, extensionCatalog, clusterRole, clusterRoleBinding, ns := testInit(t) + defer testCleanup(t, extensionCatalog, clusterExtension, clusterRole, clusterRoleBinding, ns) defer utils.CollectTestArtifacts(t, artifactName, c, cfg) clusterExtension.Spec = ocv1.ClusterExtensionSpec{ @@ -692,9 +660,6 @@ func TestClusterExtensionInstallReResolvesWhenCatalogIsPatched(t *testing.T) { }, }, Namespace: ns.Name, - ServiceAccount: ocv1.ServiceAccountReference{ - Name: sa.Name, - }, } t.Log("It resolves the specified package with correct bundle path") t.Log("By creating the ClusterExtension resource") @@ -757,9 +722,9 @@ func TestClusterExtensionInstallReResolvesWhenNewCatalog(t *testing.T) { } ns, err := createNamespace(context.Background(), clusterExtensionName) require.NoError(t, err) - sa, err := createServiceAccount(context.Background(), types.NamespacedName{Name: clusterExtensionName, Namespace: ns.Name}, clusterExtensionName) + cr, crb, err := createClusterExtensionPermissions(context.Background(), clusterExtensionName) require.NoError(t, err) - defer testCleanup(t, extensionCatalog, clusterExtension, sa, ns) + defer testCleanup(t, extensionCatalog, clusterExtension, cr, crb, ns) defer utils.CollectTestArtifacts(t, artifactName, c, cfg) clusterExtension.Spec = ocv1.ClusterExtensionSpec{ @@ -773,9 +738,6 @@ func TestClusterExtensionInstallReResolvesWhenNewCatalog(t *testing.T) { }, }, Namespace: ns.Name, - ServiceAccount: ocv1.ServiceAccountReference{ - Name: sa.Name, - }, } t.Log("It resolves the specified package with correct bundle path") t.Log("By creating the ClusterExtension resource") @@ -819,8 +781,8 @@ func TestClusterExtensionInstallReResolvesWhenNewCatalog(t *testing.T) { func TestClusterExtensionInstallReResolvesWhenManagedContentChanged(t *testing.T) { t.Log("When a cluster extension is installed from a catalog") t.Log("It resolves again when managed content is changed") - clusterExtension, extensionCatalog, sa, ns := testInit(t) - defer testCleanup(t, extensionCatalog, clusterExtension, sa, ns) + clusterExtension, extensionCatalog, clusterRole, clusterRoleBinding, ns := testInit(t) + defer testCleanup(t, extensionCatalog, clusterExtension, clusterRole, clusterRoleBinding, ns) defer utils.CollectTestArtifacts(t, artifactName, c, cfg) clusterExtension.Spec = ocv1.ClusterExtensionSpec{ @@ -834,9 +796,6 @@ func TestClusterExtensionInstallReResolvesWhenManagedContentChanged(t *testing.T }, }, Namespace: ns.Name, - ServiceAccount: ocv1.ServiceAccountReference{ - Name: sa.Name, - }, } t.Log("It installs the specified package with correct bundle path") t.Log("By creating the ClusterExtension resource") @@ -872,18 +831,8 @@ func TestClusterExtensionRecoversFromInitialInstallFailedWhenFailureFixed(t *tes t.Log("When a cluster extension is installed from a catalog") t.Log("When the extension bundle format is registry+v1") - clusterExtension, extensionCatalog, _, ns := testInit(t) - - name := rand.String(10) - sa := &corev1.ServiceAccount{ - ObjectMeta: metav1.ObjectMeta{ - Name: name, - Namespace: ns.Name, - }, - } - err := c.Create(context.Background(), sa) - require.NoError(t, err) - defer testCleanup(t, extensionCatalog, clusterExtension, sa, ns) + clusterExtension, extensionCatalog, ns := testInitNoPermissions(t) + defer testCleanup(t, extensionCatalog, clusterExtension, ns) defer utils.CollectTestArtifacts(t, artifactName, c, cfg) clusterExtension.Spec = ocv1.ClusterExtensionSpec{ @@ -897,9 +846,6 @@ func TestClusterExtensionRecoversFromInitialInstallFailedWhenFailureFixed(t *tes }, }, Namespace: ns.Name, - ServiceAccount: ocv1.ServiceAccountReference{ - Name: sa.Name, - }, } t.Log("It resolves the specified package with correct bundle path") t.Log("By creating the ClusterExtension resource") @@ -920,7 +866,7 @@ func TestClusterExtensionRecoversFromInitialInstallFailedWhenFailureFixed(t *tes } }, pollDuration, pollInterval) - t.Log("By eventually failing to install the package successfully due to insufficient ServiceAccount permissions") + t.Log("By eventually failing to install the package successfully due to insufficient permissions") require.EventuallyWithT(t, func(ct *assert.CollectT) { assert.NoError(ct, c.Get(context.Background(), types.NamespacedName{Name: clusterExtension.Name}, clusterExtension)) cond := apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1.TypeInstalled) @@ -931,13 +877,15 @@ func TestClusterExtensionRecoversFromInitialInstallFailedWhenFailureFixed(t *tes } }, pollDuration, pollInterval) - t.Log("By fixing the ServiceAccount permissions") - require.NoError(t, createClusterRoleAndBindingForSA(context.Background(), name, sa, clusterExtension.Name)) + t.Log("By fixing the ClusterExtension admin permissions") + cr, crb, err := createClusterExtensionPermissions(context.Background(), clusterExtension.Name) + require.NoError(t, err) + defer testCleanup(t, crb, cr) // NOTE: In order to ensure predictable results we need to ensure we have a single // known failure with a singular fix operation. Additionally, due to the exponential // backoff of this eventually check we MUST ensure we do not touch the ClusterExtension - // after creating and binding the needed permissions to the ServiceAccount. + // after creating and binding the needed permissions. t.Log("By eventually installing the package successfully") require.EventuallyWithT(t, func(ct *assert.CollectT) { assert.NoError(ct, c.Get(context.Background(), types.NamespacedName{Name: clusterExtension.Name}, clusterExtension)) diff --git a/test/extension-developer-e2e/extension_developer_test.go b/test/extension-developer-e2e/extension_developer_test.go index c493887b0..c84c7a5b3 100644 --- a/test/extension-developer-e2e/extension_developer_test.go +++ b/test/extension-developer-e2e/extension_developer_test.go @@ -74,9 +74,6 @@ func TestExtensionDeveloper(t *testing.T) { }, }, Namespace: installNamespace, - ServiceAccount: ocv1.ServiceAccountReference{ - Name: sa.Name, - }, }, }