From 664a51fab2e26c61b99ee69692abf88c3d15c36b Mon Sep 17 00:00:00 2001 From: rashmi_kh Date: Tue, 10 Sep 2024 00:37:10 +0530 Subject: [PATCH 01/35] changes to derice minimum service account Signed-off-by: rashmi_kh --- docs/drafts/derive-serviceaccount.md | 358 +++++++++++++++++++++++++++ 1 file changed, 358 insertions(+) create mode 100644 docs/drafts/derive-serviceaccount.md diff --git a/docs/drafts/derive-serviceaccount.md b/docs/drafts/derive-serviceaccount.md new file mode 100644 index 000000000..9623c12e9 --- /dev/null +++ b/docs/drafts/derive-serviceaccount.md @@ -0,0 +1,358 @@ +# Derive minimal ServiceAccount required for ClusterExtension Installation and Management + +OLM v1 security stance (secure by default) + +Adhering to OLM v1's "Secure by Default" tenet, OLM v1 does not have the permissions necessary to install content. This follows the least privilege principle and reduces the chance of a [confused deputy attack](https://en.wikipedia.org/wiki/Confused_deputy_problem). Instead, a ServiceAccount must be provided by users to install and manage content. + +Explain installing a CE requires a Service Account + +OLMv1 does not provide cluster admin privileges by default for installing cluster extensions. It depends on the cluster extension developer to specify the exact permissions required for the management of any specific bundle. A ServiceAccount needs to be explicitly specified for installing and upgrading operators else will face errors when deploying your cluster extension. + +The ServiceAccount is specified in the ClusterExtension manifest as follows: + +```yaml +apiVersion: olm.operatorframework.io/v1alpha1 +kind: ClusterExtension +metadata: + name: argocd +spec: + source: + sourceType: Catalog + catalog: + packageName: argocd-operator + version: 0.6.0 + install: + namespace: argocd + serviceAccount: + name: argocd-installer +``` + +The cluster extension installer will need RBAC in order to be able to assign the controller the RBAC it requires. +In order to derive the minimal RBAC for the installer service account, you must specify the following permissions: +* ClusterRole with all the roles specified in the bundle ClusterServiceVersion. +* ClusterExtension finalizer +* Allow ClusterExtenstion to set blockOwnerDeletion ownerReferences +* create the controller deployment +* create the ClusterResourceDefnitions +* create the other manifest objects +* create the necessary Cluster/Roles for the controller to be able to perform its job. +* get, list, watch, update, patch, delete the specific resources that get created +* update finalizers on the ClusterExtension to be able to set blockOwnerDeletion and ownerReferences + + +The following ClusterRole rules are needed: +# Allow ClusterExtension to set blockOwnerDeletion ownerReferences +- apiGroups: [olm.operatorframework.io] + resources: [clusterextensions/finalizers] + verbs: [update] + resourceNames: [] +# Manage CRDs +- apiGroups: [apiextensions.k8s.io] + resources: [customresourcedefinitions] + verbs: [create, list, watch] +- apiGroups: [apiextensions.k8s.io] + resources: [customresourcedefinitions] + verbs: [get, update, patch, delete] + resourceNames: [, ..., ] +# Manage ClusterRoles +- apiGroups: [rbac.authorization.k8s.io] + resources: [clusterroles] + verbs: [create, list, watch] +- apiGroups: [rbac.authorization.k8s.io] + resources: [clusterroles] + verbs: [get, update, patch, delete] + resourceNames: [, ..., , , ..., ] +# Manage ClusterRoleBindings +- apiGroups: [rbac.authorization.k8s.io] + resources: [clusterrolebindings] + verbs: [create, list, watch] +- apiGroups: [rbac.authorization.k8s.io] + resources: [clusterrolebindings] + verbs: [get, update, patch, delete] + resourceNames: [, ..., , , ..., ] +* Rules defined in the CSV under `.spec.install.clusterPermissions` and `.spec.install.permissions` +* Rules to manage any other cluster-scoped resources shipped with the bundle +* Rules to manage any other namespace-scoped resources +* Rules to manage the deployment defined in the ClusterServiceVersion +* Rules to manage the service account used for the deployment +- apiGroups: [apps] + resources: [deployments] + verbs: [create, list, watch] + +- apiGroups: [apps] + resources: [deployments] + verbs: [get, update, patch, delete] + resourceNames: [argocd-operator-controller-manager] + +- apiGroups: [""] + resources: [serviceaccounts] + verbs: [create, list, watch] + +- apiGroups: [""] + resources: [serviceaccounts] + verbs: [get, update, patch, delete] + resourceNames: [argocd-operator-controller-manager] +``` + +Below is an example of the argocd installer with the necessary RBAC to deploy the ArgoCD ClusterExtension: + +```yaml +apiVersion: rbac.authorization.k8s.io/v1 +kind: ClusterRole +metadata: + name: argocd-installer-clusterrole +rules: +# Allow ClusterExtension to set blockOwnerDeletion ownerReferences +- apiGroups: [olm.operatorframework.io] + resources: [clusterextensions/finalizers] + verbs: [update] + resourceNames: [argocd] +# Manage ArgoCD CRDs +- apiGroups: [apiextensions.k8s.io] + resources: [customresourcedefinitions] + verbs: [create] +- apiGroups: [apiextensions.k8s.io] + resources: [customresourcedefinitions] + verbs: [get, list, watch, update, patch, delete] + resourceNames: + - appprojects.argoproj.io + - argocds.argoproj.io + - applications.argoproj.io + - argocdexports.argoproj.io + - applicationsets.argoproj.io +# Manage ArgoCD ClusterRoles and ClusterRoleBindings +- apiGroups: [rbac.authorization.k8s.io] + resources: [clusterroles] + verbs: [create] +- apiGroups: [rbac.authorization.k8s.io] + resources: [clusterroles] + verbs: [get, list, watch, update, patch, delete] + resourceNames: + - argocd-operator.v0-1dhiybrldl1gyksid1dk2dqjsc72psdybc7iyvse5gpx + - argocd-operator-metrics-reader + - argocd-operator.v0-22gmilmgp91wu25is5i2ec598hni8owq3l71bbkl7iz3 +- apiGroups: [rbac.authorization.k8s.io] + resources: [clusterrolebindings] + verbs: [create] +- apiGroups: [rbac.authorization.k8s.io] + resources: [clusterrolebindings] + verbs: [get, list, watch, update, patch, delete] + resourceNames: + - argocd-operator.v0-1dhiybrldl1gyksid1dk2dqjsc72psdybc7iyvse5gpx + - argocd-operator.v0-22gmilmgp91wu25is5i2ec598hni8owq3l71bbkl7iz3 +--- +apiVersion: rbac.authorization.k8s.io/v1 +kind: ClusterRoleBinding +metadata: + name: argocd-installer-rbac-binding +roleRef: + apiGroup: rbac.authorization.k8s.io + kind: ClusterRole + name: argocd-installer-rbac-clusterrole +subjects: +- kind: ServiceAccount + name: argocd-installer + namespace: argocd +--- +apiVersion: rbac.authorization.k8s.io/v1 +kind: ClusterRole +metadata: + name: argocd-installer-rbac-clusterrole +rules: +# ArgoCD's operator requires the following permissions, which means the +# installer also needs them in order to create ArgoCD's RBAC objects. +- apiGroups: [""] + resources: [configmaps] + verbs: ['*'] +- apiGroups: [""] + resources: [endpoints] + verbs: ['*'] +- apiGroups: [""] + resources: [events] + verbs: ['*'] +- apiGroups: [""] + resources: [namespaces] + verbs: ['*'] +- apiGroups: [""] + resources: [persistentvolumeclaims] + verbs: ['*'] +- apiGroups: [""] + resources: [pods] + verbs: ['*', get] +- apiGroups: [""] + resources: [pods/log] + verbs: [get] +- apiGroups: [""] + resources: [secrets] + verbs: ['*'] +- apiGroups: [""] + resources: [serviceaccounts] + verbs: ['*'] +- apiGroups: [""] + resources: [services] + verbs: ['*'] +- apiGroups: [""] + resources: [services/finalizers] + verbs: ['*'] +- apiGroups: [apps] + resources: [daemonsets] + verbs: ['*'] +- apiGroups: [apps] + resources: [deployments] + verbs: ['*'] +- apiGroups: [apps] + resources: [deployments/finalizers] + resourceNames: [argocd-operator] + verbs: [update] +- apiGroups: [apps] + resources: [replicasets] + verbs: ['*'] +- apiGroups: [apps] + resources: [statefulsets] + verbs: ['*'] +- apiGroups: [apps.openshift.io] + resources: [deploymentconfigs] + verbs: ['*'] +- apiGroups: [argoproj.io] + resources: [applications] + verbs: ['*'] +- apiGroups: [argoproj.io] + resources: [appprojects] + verbs: ['*'] +- apiGroups: [argoproj.io] + resources: [argocdexports] + verbs: ['*'] +- apiGroups: [argoproj.io] + resources: [argocdexports/finalizers] + verbs: ['*'] +- apiGroups: [argoproj.io] + resources: [argocdexports/status] + verbs: ['*'] +- apiGroups: [argoproj.io] + resources: [argocds] + verbs: ['*'] +- apiGroups: [argoproj.io] + resources: [argocds/finalizers] + verbs: ['*'] +- apiGroups: [argoproj.io] + resources: [argocds/status] + verbs: ['*'] +- apiGroups: [authentication.k8s.io] + resources: [tokenreviews] + verbs: [create] +- apiGroups: [authorization.k8s.io] + resources: [subjectaccessreviews] + verbs: [create] +- apiGroups: [autoscaling] + resources: [horizontalpodautoscalers] + verbs: ['*'] +- apiGroups: [batch] + resources: [cronjobs] + verbs: ['*'] +- apiGroups: [batch] + resources: [jobs] + verbs: ['*'] +- apiGroups: [config.openshift.io] + resources: [clusterversions] + verbs: [get, list, watch] +- apiGroups: [monitoring.coreos.com] + resources: [prometheuses] + verbs: ['*'] +- apiGroups: [monitoring.coreos.com] + resources: [servicemonitors] + verbs: ['*'] +- apiGroups: [networking.k8s.io] + resources: [ingresses] + verbs: ['*'] +- apiGroups: [oauth.openshift.io] + resources: [oauthclients] + verbs: [create, delete, get, list, patch, update, watch] +- apiGroups: [rbac.authorization.k8s.io] + resources: ['*'] + verbs: ['*'] +- apiGroups: [rbac.authorization.k8s.io] + resources: [clusterrolebindings] + verbs: ['*'] +- apiGroups: [rbac.authorization.k8s.io] + resources: [clusterroles] + verbs: ['*'] +- apiGroups: [route.openshift.io] + resources: [routes] + verbs: ['*'] +- apiGroups: [route.openshift.io] + resources: [routes/custom-host] + verbs: ['*'] +- apiGroups: [template.openshift.io] + resources: [templateconfigs] + verbs: ['*'] +- apiGroups: [template.openshift.io] + resources: [templateinstances] + verbs: ['*'] +- apiGroups: [template.openshift.io] + resources: [templates] + verbs: ['*'] +--- +apiVersion: rbac.authorization.k8s.io/v1 +kind: Role +metadata: + name: argocd-installer-role + namespace: argocd +rules: +- apiGroups: [""] + resources: [serviceaccounts] + verbs: [create] +- apiGroups: [""] + resources: [serviceaccounts] + verbs: [get, list, watch, update, patch, delete] + resourceNames: [argocd-operator-controller-manager] +- apiGroups: [""] + resources: [configmaps] + verbs: [create] +- apiGroups: [""] + resources: [configmaps] + verbs: [get, list, watch, update, patch, delete] + resourceNames: [argocd-operator-manager-config] +- apiGroups: [""] + resources: [services] + verbs: [create] +- apiGroups: [""] + resources: [services] + verbs: [get, list, watch, update, patch, delete] + resourceNames: [argocd-operator-controller-manager-metrics-service] +- apiGroups: [apps] + resources: [deployments] + verbs: [create] +- apiGroups: [apps] + resources: [deployments] + verbs: [get, list, watch, update, patch, delete] + resourceNames: [argocd-operator-controller-manager] +``` + +# Creation of ClusterRoleBinding using the cluster-admin ClusterRole in non-production environments + +```yaml +# Create ClusterRole +kubectl apply -f - < Date: Tue, 10 Sep 2024 00:47:55 +0530 Subject: [PATCH 02/35] remove headers Signed-off-by: rashmi_kh --- docs/drafts/derive-serviceaccount.md | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/docs/drafts/derive-serviceaccount.md b/docs/drafts/derive-serviceaccount.md index 9623c12e9..e349ec131 100644 --- a/docs/drafts/derive-serviceaccount.md +++ b/docs/drafts/derive-serviceaccount.md @@ -41,12 +41,15 @@ In order to derive the minimal RBAC for the installer service account, you must The following ClusterRole rules are needed: -# Allow ClusterExtension to set blockOwnerDeletion ownerReferences +* Allow ClusterExtension to set blockOwnerDeletion ownerReferences + - apiGroups: [olm.operatorframework.io] resources: [clusterextensions/finalizers] verbs: [update] resourceNames: [] -# Manage CRDs + +* Manage CRDs + - apiGroups: [apiextensions.k8s.io] resources: [customresourcedefinitions] verbs: [create, list, watch] @@ -54,7 +57,9 @@ The following ClusterRole rules are needed: resources: [customresourcedefinitions] verbs: [get, update, patch, delete] resourceNames: [, ..., ] -# Manage ClusterRoles + +* Manage ClusterRoles + - apiGroups: [rbac.authorization.k8s.io] resources: [clusterroles] verbs: [create, list, watch] @@ -62,7 +67,9 @@ The following ClusterRole rules are needed: resources: [clusterroles] verbs: [get, update, patch, delete] resourceNames: [, ..., , , ..., ] -# Manage ClusterRoleBindings + +* Manage ClusterRoleBindings + - apiGroups: [rbac.authorization.k8s.io] resources: [clusterrolebindings] verbs: [create, list, watch] @@ -70,11 +77,13 @@ The following ClusterRole rules are needed: resources: [clusterrolebindings] verbs: [get, update, patch, delete] resourceNames: [, ..., , , ..., ] + * Rules defined in the CSV under `.spec.install.clusterPermissions` and `.spec.install.permissions` * Rules to manage any other cluster-scoped resources shipped with the bundle * Rules to manage any other namespace-scoped resources * Rules to manage the deployment defined in the ClusterServiceVersion * Rules to manage the service account used for the deployment + - apiGroups: [apps] resources: [deployments] verbs: [create, list, watch] @@ -92,7 +101,7 @@ The following ClusterRole rules are needed: resources: [serviceaccounts] verbs: [get, update, patch, delete] resourceNames: [argocd-operator-controller-manager] -``` + Below is an example of the argocd installer with the necessary RBAC to deploy the ArgoCD ClusterExtension: From 24d93f109cea5cd46f7a8e692f263835ff44f733 Mon Sep 17 00:00:00 2001 From: rashmi_kh Date: Tue, 10 Sep 2024 15:33:44 +0530 Subject: [PATCH 03/35] add details about registry+v1 support --- docs/drafts/derive-serviceaccount.md | 77 +++++++++++++++++----------- 1 file changed, 46 insertions(+), 31 deletions(-) diff --git a/docs/drafts/derive-serviceaccount.md b/docs/drafts/derive-serviceaccount.md index e349ec131..cd4088a4a 100644 --- a/docs/drafts/derive-serviceaccount.md +++ b/docs/drafts/derive-serviceaccount.md @@ -1,14 +1,8 @@ # Derive minimal ServiceAccount required for ClusterExtension Installation and Management -OLM v1 security stance (secure by default) +OLMv1 does not provide cluster admin privileges by default for installing cluster extensions.This means that the installation process will require a service account with sufficient privileges to install the bundle. It depends on the cluster extension developer to specify the exact permissions required for the management of any specific bundle. A Service Account needs to be explicitly specified for installing and upgrading operators else will face errors when deploying your cluster extension. -Adhering to OLM v1's "Secure by Default" tenet, OLM v1 does not have the permissions necessary to install content. This follows the least privilege principle and reduces the chance of a [confused deputy attack](https://en.wikipedia.org/wiki/Confused_deputy_problem). Instead, a ServiceAccount must be provided by users to install and manage content. - -Explain installing a CE requires a Service Account - -OLMv1 does not provide cluster admin privileges by default for installing cluster extensions. It depends on the cluster extension developer to specify the exact permissions required for the management of any specific bundle. A ServiceAccount needs to be explicitly specified for installing and upgrading operators else will face errors when deploying your cluster extension. - -The ServiceAccount is specified in the ClusterExtension manifest as follows: +The Service Account is specified in the ClusterExtension manifest as shown below: ```yaml apiVersion: olm.operatorframework.io/v1alpha1 @@ -27,20 +21,31 @@ spec: name: argocd-installer ``` -The cluster extension installer will need RBAC in order to be able to assign the controller the RBAC it requires. -In order to derive the minimal RBAC for the installer service account, you must specify the following permissions: -* ClusterRole with all the roles specified in the bundle ClusterServiceVersion. -* ClusterExtension finalizer -* Allow ClusterExtenstion to set blockOwnerDeletion ownerReferences -* create the controller deployment -* create the ClusterResourceDefnitions -* create the other manifest objects -* create the necessary Cluster/Roles for the controller to be able to perform its job. -* get, list, watch, update, patch, delete the specific resources that get created -* update finalizers on the ClusterExtension to be able to set blockOwnerDeletion and ownerReferences +The initial stable version (v1.0.0) only supports FBC catalogs containing registry+v1 bundles. OLMv1 will not support all OLMv0 content. OLMv1 will only support bundles that meet the following criteria: +* AllNamespaces install mode is enabled +* No dependencies on other packages or GVKs +* No webhooks +* Does not make use of the OperatorConditions API + +### Required RBAC +The cluster extension installer should have the prerequisite permissions in order to be able to assign the controller the RBAC it requires. In order to derive the minimal RBAC for the installer service account, you must specify the following permissions: + +* ClusterRole with all the roles specified in the bundle ClusterServiceVersion. This includes all the + rules defined in the CSV under `.spec.install.clusterPermissions` and `.spec.install.permissions` +* ClusterRole to create and manage CustomResourceDefinitions +* Update finalizers on the ClusterExtension to be able to set blockOwnerDeletion and ownerReferences +* Permissions to create the controller deployment, this corresponds to the rules to manage the + deployment defined in the ClusterServiceVersion +* Permissions to create the other manifest objects, rules to manage any other cluster-scoped resources + shipped with the bundle +* Rules to manage any other namespace-scoped resources +* Permissions to create the necessary roles and rolebindings for the controller to be able to perform its job +* Get, list, watch, update, patch, delete the specific resources that get created + + +### Below are snippets of the ClusterRole rule definitions: -The following ClusterRole rules are needed: * Allow ClusterExtension to set blockOwnerDeletion ownerReferences - apiGroups: [olm.operatorframework.io] @@ -48,7 +53,7 @@ The following ClusterRole rules are needed: verbs: [update] resourceNames: [] -* Manage CRDs +* Manage Custom Resource Definitions - apiGroups: [apiextensions.k8s.io] resources: [customresourcedefinitions] @@ -58,7 +63,7 @@ The following ClusterRole rules are needed: verbs: [get, update, patch, delete] resourceNames: [, ..., ] -* Manage ClusterRoles +* Manage Cluster Roles - apiGroups: [rbac.authorization.k8s.io] resources: [clusterroles] @@ -68,7 +73,7 @@ The following ClusterRole rules are needed: verbs: [get, update, patch, delete] resourceNames: [, ..., , , ..., ] -* Manage ClusterRoleBindings +* Manage Cluster Role Bindings - apiGroups: [rbac.authorization.k8s.io] resources: [clusterrolebindings] @@ -78,11 +83,7 @@ The following ClusterRole rules are needed: verbs: [get, update, patch, delete] resourceNames: [, ..., , , ..., ] -* Rules defined in the CSV under `.spec.install.clusterPermissions` and `.spec.install.permissions` -* Rules to manage any other cluster-scoped resources shipped with the bundle -* Rules to manage any other namespace-scoped resources -* Rules to manage the deployment defined in the ClusterServiceVersion -* Rules to manage the service account used for the deployment +* Manage deployments - apiGroups: [apps] resources: [deployments] @@ -93,6 +94,9 @@ The following ClusterRole rules are needed: verbs: [get, update, patch, delete] resourceNames: [argocd-operator-controller-manager] + +* Manage service accounts used for the deployment + - apiGroups: [""] resources: [serviceaccounts] verbs: [create, list, watch] @@ -105,7 +109,7 @@ The following ClusterRole rules are needed: Below is an example of the argocd installer with the necessary RBAC to deploy the ArgoCD ClusterExtension: -```yaml +???+ note apiVersion: rbac.authorization.k8s.io/v1 kind: ClusterRole metadata: @@ -337,7 +341,18 @@ rules: resourceNames: [argocd-operator-controller-manager] ``` -# Creation of ClusterRoleBinding using the cluster-admin ClusterRole in non-production environments +### Manual process for minimal RBAC creation + +There are no production tools available to help you understand the precise RBAC required for your cluster extensions. However, if you want to figure this manually you can try the below: + +* Create all the intial rbac and then iterate over the ClusterExtention failures, examining conditions and updating the RBAC to include the generated cluster role names (name will be in the failure condition). +Install the ClusterExtension, read the failure condition, update installer RBAC and iterate until you are out of errors +* You can get the bundle image, unpacking the same and inspect the manifests to figure out the required permissions. +* The `oc` cli-tool creates cluster roles with a hash in their name, query the newly created ClusterRole names, then reduce the installer RBAC scope to just the ClusterRoles needed (inc. the generated ones). You can achieve this by allowing the installer to get, list, watch and update any cluster roles. + + + +### Creation of ClusterRoleBinding using the cluster-admin ClusterRole in non-production environments ```yaml # Create ClusterRole @@ -356,7 +371,7 @@ subjects: namespace: my-cluster-extension-namespace EOF -``` +```sh kubectl create clusterrolebinding my-cluster-extension-installer-role-binding \ --clusterrole=cluster-admin \ --serviceaccount=my-cluster-extension-namespace:my-cluster-installer-service-account From fdf7e9d5551964b712eb0f085d5bdf23ccddf1e0 Mon Sep 17 00:00:00 2001 From: rashmi_kh Date: Tue, 10 Sep 2024 15:37:06 +0530 Subject: [PATCH 04/35] render yml correctly Signed-off-by: rashmi_kh --- docs/drafts/derive-serviceaccount.md | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/docs/drafts/derive-serviceaccount.md b/docs/drafts/derive-serviceaccount.md index cd4088a4a..62cb1305d 100644 --- a/docs/drafts/derive-serviceaccount.md +++ b/docs/drafts/derive-serviceaccount.md @@ -48,13 +48,16 @@ The cluster extension installer should have the prerequisite permissions in orde * Allow ClusterExtension to set blockOwnerDeletion ownerReferences +```yaml - apiGroups: [olm.operatorframework.io] resources: [clusterextensions/finalizers] verbs: [update] resourceNames: [] +``` * Manage Custom Resource Definitions +```yaml - apiGroups: [apiextensions.k8s.io] resources: [customresourcedefinitions] verbs: [create, list, watch] @@ -62,9 +65,11 @@ The cluster extension installer should have the prerequisite permissions in orde resources: [customresourcedefinitions] verbs: [get, update, patch, delete] resourceNames: [, ..., ] +``` * Manage Cluster Roles +```yaml - apiGroups: [rbac.authorization.k8s.io] resources: [clusterroles] verbs: [create, list, watch] @@ -72,9 +77,11 @@ The cluster extension installer should have the prerequisite permissions in orde resources: [clusterroles] verbs: [get, update, patch, delete] resourceNames: [, ..., , , ..., ] +``` * Manage Cluster Role Bindings +```yaml - apiGroups: [rbac.authorization.k8s.io] resources: [clusterrolebindings] verbs: [create, list, watch] @@ -82,9 +89,11 @@ The cluster extension installer should have the prerequisite permissions in orde resources: [clusterrolebindings] verbs: [get, update, patch, delete] resourceNames: [, ..., , , ..., ] +``` * Manage deployments +```yaml - apiGroups: [apps] resources: [deployments] verbs: [create, list, watch] @@ -93,10 +102,11 @@ The cluster extension installer should have the prerequisite permissions in orde resources: [deployments] verbs: [get, update, patch, delete] resourceNames: [argocd-operator-controller-manager] - +``` * Manage service accounts used for the deployment - + +```yaml - apiGroups: [""] resources: [serviceaccounts] verbs: [create, list, watch] @@ -105,11 +115,12 @@ The cluster extension installer should have the prerequisite permissions in orde resources: [serviceaccounts] verbs: [get, update, patch, delete] resourceNames: [argocd-operator-controller-manager] - +``` Below is an example of the argocd installer with the necessary RBAC to deploy the ArgoCD ClusterExtension: ???+ note +```yaml apiVersion: rbac.authorization.k8s.io/v1 kind: ClusterRole metadata: From 157cce066ab053be2730d40c65ac657ed6a5f49b Mon Sep 17 00:00:00 2001 From: rashmi_kh Date: Mon, 18 Nov 2024 17:31:18 +0530 Subject: [PATCH 05/35] sa not found Signed-off-by: rashmi_kh --- internal/authentication/tokengetter.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/internal/authentication/tokengetter.go b/internal/authentication/tokengetter.go index 3fec56f37..c14c36941 100644 --- a/internal/authentication/tokengetter.go +++ b/internal/authentication/tokengetter.go @@ -1,6 +1,7 @@ package authentication import ( + "fmt" "context" "sync" "time" @@ -9,6 +10,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" corev1 "k8s.io/client-go/kubernetes/typed/core/v1" + "k8s.io/apimachinery/pkg/api/errors" "k8s.io/utils/ptr" ) @@ -86,6 +88,10 @@ func (t *TokenGetter) getToken(ctx context.Context, key types.NamespacedName) (* Spec: authenticationv1.TokenRequestSpec{ExpirationSeconds: ptr.To(int64(t.expirationDuration / time.Second))}, }, metav1.CreateOptions{}) if err != nil { + if errors.IsNotFound(err) { + var saNotFoundError = fmt.Errorf("service account not found") + return nil, saNotFoundError + } return nil, err } return &req.Status, nil From 60626774658f0da39406a4391071d96812e69970 Mon Sep 17 00:00:00 2001 From: Rashmi Khanna Date: Mon, 18 Nov 2024 18:58:05 +0530 Subject: [PATCH 06/35] Delete docs/drafts/derive-serviceaccount.md --- docs/drafts/derive-serviceaccount.md | 393 --------------------------- 1 file changed, 393 deletions(-) delete mode 100644 docs/drafts/derive-serviceaccount.md diff --git a/docs/drafts/derive-serviceaccount.md b/docs/drafts/derive-serviceaccount.md deleted file mode 100644 index 62cb1305d..000000000 --- a/docs/drafts/derive-serviceaccount.md +++ /dev/null @@ -1,393 +0,0 @@ -# Derive minimal ServiceAccount required for ClusterExtension Installation and Management - -OLMv1 does not provide cluster admin privileges by default for installing cluster extensions.This means that the installation process will require a service account with sufficient privileges to install the bundle. It depends on the cluster extension developer to specify the exact permissions required for the management of any specific bundle. A Service Account needs to be explicitly specified for installing and upgrading operators else will face errors when deploying your cluster extension. - -The Service Account is specified in the ClusterExtension manifest as shown below: - -```yaml -apiVersion: olm.operatorframework.io/v1alpha1 -kind: ClusterExtension -metadata: - name: argocd -spec: - source: - sourceType: Catalog - catalog: - packageName: argocd-operator - version: 0.6.0 - install: - namespace: argocd - serviceAccount: - name: argocd-installer -``` - -The initial stable version (v1.0.0) only supports FBC catalogs containing registry+v1 bundles. OLMv1 will not support all OLMv0 content. OLMv1 will only support bundles that meet the following criteria: -* AllNamespaces install mode is enabled -* No dependencies on other packages or GVKs -* No webhooks -* Does not make use of the OperatorConditions API - -### Required RBAC - -The cluster extension installer should have the prerequisite permissions in order to be able to assign the controller the RBAC it requires. In order to derive the minimal RBAC for the installer service account, you must specify the following permissions: - -* ClusterRole with all the roles specified in the bundle ClusterServiceVersion. This includes all the - rules defined in the CSV under `.spec.install.clusterPermissions` and `.spec.install.permissions` -* ClusterRole to create and manage CustomResourceDefinitions -* Update finalizers on the ClusterExtension to be able to set blockOwnerDeletion and ownerReferences -* Permissions to create the controller deployment, this corresponds to the rules to manage the - deployment defined in the ClusterServiceVersion -* Permissions to create the other manifest objects, rules to manage any other cluster-scoped resources - shipped with the bundle -* Rules to manage any other namespace-scoped resources -* Permissions to create the necessary roles and rolebindings for the controller to be able to perform its job -* Get, list, watch, update, patch, delete the specific resources that get created - - -### Below are snippets of the ClusterRole rule definitions: - -* Allow ClusterExtension to set blockOwnerDeletion ownerReferences - -```yaml -- apiGroups: [olm.operatorframework.io] - resources: [clusterextensions/finalizers] - verbs: [update] - resourceNames: [] -``` - -* Manage Custom Resource Definitions - -```yaml -- apiGroups: [apiextensions.k8s.io] - resources: [customresourcedefinitions] - verbs: [create, list, watch] -- apiGroups: [apiextensions.k8s.io] - resources: [customresourcedefinitions] - verbs: [get, update, patch, delete] - resourceNames: [, ..., ] -``` - -* Manage Cluster Roles - -```yaml -- apiGroups: [rbac.authorization.k8s.io] - resources: [clusterroles] - verbs: [create, list, watch] -- apiGroups: [rbac.authorization.k8s.io] - resources: [clusterroles] - verbs: [get, update, patch, delete] - resourceNames: [, ..., , , ..., ] -``` - -* Manage Cluster Role Bindings - -```yaml -- apiGroups: [rbac.authorization.k8s.io] - resources: [clusterrolebindings] - verbs: [create, list, watch] -- apiGroups: [rbac.authorization.k8s.io] - resources: [clusterrolebindings] - verbs: [get, update, patch, delete] - resourceNames: [, ..., , , ..., ] -``` - -* Manage deployments - -```yaml -- apiGroups: [apps] - resources: [deployments] - verbs: [create, list, watch] - -- apiGroups: [apps] - resources: [deployments] - verbs: [get, update, patch, delete] - resourceNames: [argocd-operator-controller-manager] -``` - -* Manage service accounts used for the deployment - -```yaml -- apiGroups: [""] - resources: [serviceaccounts] - verbs: [create, list, watch] - -- apiGroups: [""] - resources: [serviceaccounts] - verbs: [get, update, patch, delete] - resourceNames: [argocd-operator-controller-manager] -``` - -Below is an example of the argocd installer with the necessary RBAC to deploy the ArgoCD ClusterExtension: - -???+ note -```yaml -apiVersion: rbac.authorization.k8s.io/v1 -kind: ClusterRole -metadata: - name: argocd-installer-clusterrole -rules: -# Allow ClusterExtension to set blockOwnerDeletion ownerReferences -- apiGroups: [olm.operatorframework.io] - resources: [clusterextensions/finalizers] - verbs: [update] - resourceNames: [argocd] -# Manage ArgoCD CRDs -- apiGroups: [apiextensions.k8s.io] - resources: [customresourcedefinitions] - verbs: [create] -- apiGroups: [apiextensions.k8s.io] - resources: [customresourcedefinitions] - verbs: [get, list, watch, update, patch, delete] - resourceNames: - - appprojects.argoproj.io - - argocds.argoproj.io - - applications.argoproj.io - - argocdexports.argoproj.io - - applicationsets.argoproj.io -# Manage ArgoCD ClusterRoles and ClusterRoleBindings -- apiGroups: [rbac.authorization.k8s.io] - resources: [clusterroles] - verbs: [create] -- apiGroups: [rbac.authorization.k8s.io] - resources: [clusterroles] - verbs: [get, list, watch, update, patch, delete] - resourceNames: - - argocd-operator.v0-1dhiybrldl1gyksid1dk2dqjsc72psdybc7iyvse5gpx - - argocd-operator-metrics-reader - - argocd-operator.v0-22gmilmgp91wu25is5i2ec598hni8owq3l71bbkl7iz3 -- apiGroups: [rbac.authorization.k8s.io] - resources: [clusterrolebindings] - verbs: [create] -- apiGroups: [rbac.authorization.k8s.io] - resources: [clusterrolebindings] - verbs: [get, list, watch, update, patch, delete] - resourceNames: - - argocd-operator.v0-1dhiybrldl1gyksid1dk2dqjsc72psdybc7iyvse5gpx - - argocd-operator.v0-22gmilmgp91wu25is5i2ec598hni8owq3l71bbkl7iz3 ---- -apiVersion: rbac.authorization.k8s.io/v1 -kind: ClusterRoleBinding -metadata: - name: argocd-installer-rbac-binding -roleRef: - apiGroup: rbac.authorization.k8s.io - kind: ClusterRole - name: argocd-installer-rbac-clusterrole -subjects: -- kind: ServiceAccount - name: argocd-installer - namespace: argocd ---- -apiVersion: rbac.authorization.k8s.io/v1 -kind: ClusterRole -metadata: - name: argocd-installer-rbac-clusterrole -rules: -# ArgoCD's operator requires the following permissions, which means the -# installer also needs them in order to create ArgoCD's RBAC objects. -- apiGroups: [""] - resources: [configmaps] - verbs: ['*'] -- apiGroups: [""] - resources: [endpoints] - verbs: ['*'] -- apiGroups: [""] - resources: [events] - verbs: ['*'] -- apiGroups: [""] - resources: [namespaces] - verbs: ['*'] -- apiGroups: [""] - resources: [persistentvolumeclaims] - verbs: ['*'] -- apiGroups: [""] - resources: [pods] - verbs: ['*', get] -- apiGroups: [""] - resources: [pods/log] - verbs: [get] -- apiGroups: [""] - resources: [secrets] - verbs: ['*'] -- apiGroups: [""] - resources: [serviceaccounts] - verbs: ['*'] -- apiGroups: [""] - resources: [services] - verbs: ['*'] -- apiGroups: [""] - resources: [services/finalizers] - verbs: ['*'] -- apiGroups: [apps] - resources: [daemonsets] - verbs: ['*'] -- apiGroups: [apps] - resources: [deployments] - verbs: ['*'] -- apiGroups: [apps] - resources: [deployments/finalizers] - resourceNames: [argocd-operator] - verbs: [update] -- apiGroups: [apps] - resources: [replicasets] - verbs: ['*'] -- apiGroups: [apps] - resources: [statefulsets] - verbs: ['*'] -- apiGroups: [apps.openshift.io] - resources: [deploymentconfigs] - verbs: ['*'] -- apiGroups: [argoproj.io] - resources: [applications] - verbs: ['*'] -- apiGroups: [argoproj.io] - resources: [appprojects] - verbs: ['*'] -- apiGroups: [argoproj.io] - resources: [argocdexports] - verbs: ['*'] -- apiGroups: [argoproj.io] - resources: [argocdexports/finalizers] - verbs: ['*'] -- apiGroups: [argoproj.io] - resources: [argocdexports/status] - verbs: ['*'] -- apiGroups: [argoproj.io] - resources: [argocds] - verbs: ['*'] -- apiGroups: [argoproj.io] - resources: [argocds/finalizers] - verbs: ['*'] -- apiGroups: [argoproj.io] - resources: [argocds/status] - verbs: ['*'] -- apiGroups: [authentication.k8s.io] - resources: [tokenreviews] - verbs: [create] -- apiGroups: [authorization.k8s.io] - resources: [subjectaccessreviews] - verbs: [create] -- apiGroups: [autoscaling] - resources: [horizontalpodautoscalers] - verbs: ['*'] -- apiGroups: [batch] - resources: [cronjobs] - verbs: ['*'] -- apiGroups: [batch] - resources: [jobs] - verbs: ['*'] -- apiGroups: [config.openshift.io] - resources: [clusterversions] - verbs: [get, list, watch] -- apiGroups: [monitoring.coreos.com] - resources: [prometheuses] - verbs: ['*'] -- apiGroups: [monitoring.coreos.com] - resources: [servicemonitors] - verbs: ['*'] -- apiGroups: [networking.k8s.io] - resources: [ingresses] - verbs: ['*'] -- apiGroups: [oauth.openshift.io] - resources: [oauthclients] - verbs: [create, delete, get, list, patch, update, watch] -- apiGroups: [rbac.authorization.k8s.io] - resources: ['*'] - verbs: ['*'] -- apiGroups: [rbac.authorization.k8s.io] - resources: [clusterrolebindings] - verbs: ['*'] -- apiGroups: [rbac.authorization.k8s.io] - resources: [clusterroles] - verbs: ['*'] -- apiGroups: [route.openshift.io] - resources: [routes] - verbs: ['*'] -- apiGroups: [route.openshift.io] - resources: [routes/custom-host] - verbs: ['*'] -- apiGroups: [template.openshift.io] - resources: [templateconfigs] - verbs: ['*'] -- apiGroups: [template.openshift.io] - resources: [templateinstances] - verbs: ['*'] -- apiGroups: [template.openshift.io] - resources: [templates] - verbs: ['*'] ---- -apiVersion: rbac.authorization.k8s.io/v1 -kind: Role -metadata: - name: argocd-installer-role - namespace: argocd -rules: -- apiGroups: [""] - resources: [serviceaccounts] - verbs: [create] -- apiGroups: [""] - resources: [serviceaccounts] - verbs: [get, list, watch, update, patch, delete] - resourceNames: [argocd-operator-controller-manager] -- apiGroups: [""] - resources: [configmaps] - verbs: [create] -- apiGroups: [""] - resources: [configmaps] - verbs: [get, list, watch, update, patch, delete] - resourceNames: [argocd-operator-manager-config] -- apiGroups: [""] - resources: [services] - verbs: [create] -- apiGroups: [""] - resources: [services] - verbs: [get, list, watch, update, patch, delete] - resourceNames: [argocd-operator-controller-manager-metrics-service] -- apiGroups: [apps] - resources: [deployments] - verbs: [create] -- apiGroups: [apps] - resources: [deployments] - verbs: [get, list, watch, update, patch, delete] - resourceNames: [argocd-operator-controller-manager] -``` - -### Manual process for minimal RBAC creation - -There are no production tools available to help you understand the precise RBAC required for your cluster extensions. However, if you want to figure this manually you can try the below: - -* Create all the intial rbac and then iterate over the ClusterExtention failures, examining conditions and updating the RBAC to include the generated cluster role names (name will be in the failure condition). -Install the ClusterExtension, read the failure condition, update installer RBAC and iterate until you are out of errors -* You can get the bundle image, unpacking the same and inspect the manifests to figure out the required permissions. -* The `oc` cli-tool creates cluster roles with a hash in their name, query the newly created ClusterRole names, then reduce the installer RBAC scope to just the ClusterRoles needed (inc. the generated ones). You can achieve this by allowing the installer to get, list, watch and update any cluster roles. - - - -### Creation of ClusterRoleBinding using the cluster-admin ClusterRole in non-production environments - -```yaml -# Create ClusterRole -kubectl apply -f - < Date: Mon, 18 Nov 2024 23:24:50 +0530 Subject: [PATCH 07/35] add custom sa not found Signed-off-by: rashmi_kh --- internal/authentication/tokengetter.go | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/internal/authentication/tokengetter.go b/internal/authentication/tokengetter.go index c14c36941..ef3d3b621 100644 --- a/internal/authentication/tokengetter.go +++ b/internal/authentication/tokengetter.go @@ -23,6 +23,14 @@ type TokenGetter struct { type TokenGetterOption func(*TokenGetter) +type SANotFoundError struct { + Msg string +} + +func (e *SANotFoundError) Error() string { + return fmt.Sprintf(" %s", e.Msg) +} + const ( rotationThresholdFraction = 0.1 DefaultExpirationDuration = 5 * time.Minute @@ -88,11 +96,10 @@ func (t *TokenGetter) getToken(ctx context.Context, key types.NamespacedName) (* Spec: authenticationv1.TokenRequestSpec{ExpirationSeconds: ptr.To(int64(t.expirationDuration / time.Second))}, }, metav1.CreateOptions{}) if err != nil { - if errors.IsNotFound(err) { - var saNotFoundError = fmt.Errorf("service account not found") - return nil, saNotFoundError - } - return nil, err + errMsg := err.Error() + stripErrMsg := errMsg[strings.LastIndex(errMsg, ":")+1:] + saErr := &SANotFoundError{stripErrMsg} + return nil, saErr } return &req.Status, nil } From 6ed7b58dd13e075d898c938b8c83ff6fecb406eb Mon Sep 17 00:00:00 2001 From: rashmi_kh Date: Mon, 18 Nov 2024 23:39:18 +0530 Subject: [PATCH 08/35] remove unused imports Signed-off-by: rashmi_kh --- internal/authentication/tokengetter.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/internal/authentication/tokengetter.go b/internal/authentication/tokengetter.go index ef3d3b621..0c76d3034 100644 --- a/internal/authentication/tokengetter.go +++ b/internal/authentication/tokengetter.go @@ -1,8 +1,8 @@ package authentication import ( - "fmt" "context" + "strings" "sync" "time" @@ -10,7 +10,6 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" corev1 "k8s.io/client-go/kubernetes/typed/core/v1" - "k8s.io/apimachinery/pkg/api/errors" "k8s.io/utils/ptr" ) From 5d5d2de2607c4addff2b9b066e98953edc70e8b5 Mon Sep 17 00:00:00 2001 From: Rashmi Khanna Date: Tue, 19 Nov 2024 00:03:48 +0530 Subject: [PATCH 09/35] Update tokengetter.go --- internal/authentication/tokengetter.go | 1 + 1 file changed, 1 insertion(+) diff --git a/internal/authentication/tokengetter.go b/internal/authentication/tokengetter.go index 0c76d3034..49e165f70 100644 --- a/internal/authentication/tokengetter.go +++ b/internal/authentication/tokengetter.go @@ -2,6 +2,7 @@ package authentication import ( "context" + "fmt" "strings" "sync" "time" From 815115e96d9360bca4e7acb7fba141b53f3c514f Mon Sep 17 00:00:00 2001 From: Rashmi Khanna Date: Tue, 19 Nov 2024 00:03:48 +0530 Subject: [PATCH 10/35] Update tokengetter.go MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Update go version checker (#1474) * Handle new files (old version is empty) * Handle the case where .0 patch is added/removed Signed-off-by: Todd Short sa not found Signed-off-by: rashmi_kh 📖 Document OLMv1 permission model (#1380) * changes to derice minimum service account Signed-off-by: rashmi_kh * remove headers Signed-off-by: rashmi_kh * add details about registry+v1 support * render yml correctly Signed-off-by: rashmi_kh * create doc for olmv1 permission model Signed-off-by: rashmi_kh * Delete docs/drafts directory * Update permission-model.md * update permission models with link Signed-off-by: rashmi_kh * add space Signed-off-by: rashmi_kh * add more structure Signed-off-by: rashmi_kh * incorporate review comments Signed-off-by: rashmi_kh * incorporate review comments Signed-off-by: rashmi_kh * pers review comments-s * example as header-s * update the example Signed-off-by: rashmi_kh --------- Signed-off-by: rashmi_kh Delete docs/drafts/derive-serviceaccount.md add custom sa not found Signed-off-by: rashmi_kh --- internal/authentication/tokengetter.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/internal/authentication/tokengetter.go b/internal/authentication/tokengetter.go index 49e165f70..7a7890314 100644 --- a/internal/authentication/tokengetter.go +++ b/internal/authentication/tokengetter.go @@ -1,6 +1,7 @@ package authentication import ( + "fmt" "context" "fmt" "strings" @@ -11,6 +12,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" corev1 "k8s.io/client-go/kubernetes/typed/core/v1" + "k8s.io/apimachinery/pkg/api/errors" "k8s.io/utils/ptr" ) From 49549f2a4297e60b031a189358f86f32a2f8c531 Mon Sep 17 00:00:00 2001 From: rashmi_kh Date: Tue, 19 Nov 2024 14:05:32 +0530 Subject: [PATCH 11/35] update error message string Signed-off-by: rashmi_kh --- internal/authentication/tokengetter.go | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/internal/authentication/tokengetter.go b/internal/authentication/tokengetter.go index 7a7890314..02d624284 100644 --- a/internal/authentication/tokengetter.go +++ b/internal/authentication/tokengetter.go @@ -1,10 +1,8 @@ package authentication import ( - "fmt" "context" "fmt" - "strings" "sync" "time" @@ -12,7 +10,6 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" corev1 "k8s.io/client-go/kubernetes/typed/core/v1" - "k8s.io/apimachinery/pkg/api/errors" "k8s.io/utils/ptr" ) @@ -23,16 +20,16 @@ type TokenGetter struct { mu sync.RWMutex } -type TokenGetterOption func(*TokenGetter) - type SANotFoundError struct { Msg string } -func (e *SANotFoundError) Error() string { - return fmt.Sprintf(" %s", e.Msg) +func (e *SANotFoundError) Error(serviceAccountName string) string { + return fmt.Sprintf(" Unable to authenticate with Kubernetes cluster using ServiceAccount \"%s\": ServiceAccount \"%s\" not found.", serviceAccountName, serviceAccountName) } +type TokenGetterOption func(*TokenGetter) + const ( rotationThresholdFraction = 0.1 DefaultExpirationDuration = 5 * time.Minute @@ -95,12 +92,10 @@ func (t *TokenGetter) getToken(ctx context.Context, key types.NamespacedName) (* req, err := t.client.ServiceAccounts(key.Namespace).CreateToken(ctx, key.Name, &authenticationv1.TokenRequest{ - Spec: authenticationv1.TokenRequestSpec{ExpirationSeconds: ptr.To(int64(t.expirationDuration / time.Second))}, + Spec: authenticationv1.TokenRequestSpec{ExpirationSeconds: ptr.To[int64](int64(t.expirationDuration / time.Second))}, }, metav1.CreateOptions{}) if err != nil { - errMsg := err.Error() - stripErrMsg := errMsg[strings.LastIndex(errMsg, ":")+1:] - saErr := &SANotFoundError{stripErrMsg} + saErr := &SANotFoundError{key.Name} return nil, saErr } return &req.Status, nil From aa4f6e9537d0ef0a7b3e9bdcd8651d4ea6056ce5 Mon Sep 17 00:00:00 2001 From: rashmi_kh Date: Tue, 19 Nov 2024 14:32:29 +0530 Subject: [PATCH 12/35] pass sa name to error message Signed-off-by: rashmi_kh --- internal/authentication/tokengetter.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/internal/authentication/tokengetter.go b/internal/authentication/tokengetter.go index 02d624284..296e91dbc 100644 --- a/internal/authentication/tokengetter.go +++ b/internal/authentication/tokengetter.go @@ -21,11 +21,11 @@ type TokenGetter struct { } type SANotFoundError struct { - Msg string + ServiceAccountName string } -func (e *SANotFoundError) Error(serviceAccountName string) string { - return fmt.Sprintf(" Unable to authenticate with Kubernetes cluster using ServiceAccount \"%s\": ServiceAccount \"%s\" not found.", serviceAccountName, serviceAccountName) +func (e *SANotFoundError) Error() string { + return fmt.Sprintf(" Unable to authenticate with Kubernetes cluster using ServiceAccount \"%s\": ServiceAccount \"%s\" not found.", e.ServiceAccountName, e.ServiceAccountName) } type TokenGetterOption func(*TokenGetter) From 1bbb34ae901f8c1be5432c8dba47b9ed8e197761 Mon Sep 17 00:00:00 2001 From: Rashmi Khanna Date: Tue, 19 Nov 2024 14:33:18 +0530 Subject: [PATCH 13/35] Update tokengetter.go --- internal/authentication/tokengetter.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/authentication/tokengetter.go b/internal/authentication/tokengetter.go index 296e91dbc..fb2f12be2 100644 --- a/internal/authentication/tokengetter.go +++ b/internal/authentication/tokengetter.go @@ -92,7 +92,7 @@ func (t *TokenGetter) getToken(ctx context.Context, key types.NamespacedName) (* req, err := t.client.ServiceAccounts(key.Namespace).CreateToken(ctx, key.Name, &authenticationv1.TokenRequest{ - Spec: authenticationv1.TokenRequestSpec{ExpirationSeconds: ptr.To[int64](int64(t.expirationDuration / time.Second))}, + Spec: authenticationv1.TokenRequestSpec{ExpirationSeconds: ptr.To(int64(t.expirationDuration / time.Second))}, }, metav1.CreateOptions{}) if err != nil { saErr := &SANotFoundError{key.Name} From 5196201391e57db65be2f1e94c9f475548d1a8bb Mon Sep 17 00:00:00 2001 From: rashmi_kh Date: Tue, 19 Nov 2024 20:16:58 +0530 Subject: [PATCH 14/35] wrap error message Signed-off-by: rashmi_kh --- internal/controllers/clusterextension_controller.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/internal/controllers/clusterextension_controller.go b/internal/controllers/clusterextension_controller.go index dea94a7f2..558a3c05d 100644 --- a/internal/controllers/clusterextension_controller.go +++ b/internal/controllers/clusterextension_controller.go @@ -47,6 +47,7 @@ import ( "github.com/operator-framework/api/pkg/operators/v1alpha1" catalogd "github.com/operator-framework/catalogd/api/v1" helmclient "github.com/operator-framework/helm-operator-plugins/pkg/client" + tokengetter "github.com/operator-framework/operator-controller/internal/authentication" "github.com/operator-framework/operator-registry/alpha/declcfg" ocv1 "github.com/operator-framework/operator-controller/api/v1" @@ -206,6 +207,11 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1.Cl installedBundle, err := r.InstalledBundleGetter.GetInstalledBundle(ctx, ext) if err != nil { setInstallStatus(ext, nil) + var saerr *tokengetter.SANotFoundError + if errors.As(err, &saerr) { + l.Info("is a SAError") + err = saerr + } setInstalledStatusConditionUnknown(ext, err.Error()) setStatusProgressing(ext, errors.New("retrying to get installed bundle")) return ctrl.Result{}, err From e8e76c6061de0717e2a338aa58d7834dd1986ed9 Mon Sep 17 00:00:00 2001 From: Rashmi Khanna Date: Wed, 20 Nov 2024 20:05:25 +0530 Subject: [PATCH 15/35] Update internal/authentication/tokengetter.go Co-authored-by: Bryce Palmer --- internal/authentication/tokengetter.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/authentication/tokengetter.go b/internal/authentication/tokengetter.go index fb2f12be2..f244a9b65 100644 --- a/internal/authentication/tokengetter.go +++ b/internal/authentication/tokengetter.go @@ -20,7 +20,7 @@ type TokenGetter struct { mu sync.RWMutex } -type SANotFoundError struct { +type ServiceAccountNotFoundError struct { ServiceAccountName string } From 3ef45b67eedc8916c0a856bd24ea545ba9811813 Mon Sep 17 00:00:00 2001 From: Rashmi Khanna Date: Wed, 20 Nov 2024 20:07:08 +0530 Subject: [PATCH 16/35] Update tokengetter.go --- internal/authentication/tokengetter.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/authentication/tokengetter.go b/internal/authentication/tokengetter.go index f244a9b65..5aa3e0a86 100644 --- a/internal/authentication/tokengetter.go +++ b/internal/authentication/tokengetter.go @@ -24,7 +24,7 @@ type ServiceAccountNotFoundError struct { ServiceAccountName string } -func (e *SANotFoundError) Error() string { +func (e *ServiceAccountNotFoundError) Error() string { return fmt.Sprintf(" Unable to authenticate with Kubernetes cluster using ServiceAccount \"%s\": ServiceAccount \"%s\" not found.", e.ServiceAccountName, e.ServiceAccountName) } @@ -95,7 +95,7 @@ func (t *TokenGetter) getToken(ctx context.Context, key types.NamespacedName) (* Spec: authenticationv1.TokenRequestSpec{ExpirationSeconds: ptr.To(int64(t.expirationDuration / time.Second))}, }, metav1.CreateOptions{}) if err != nil { - saErr := &SANotFoundError{key.Name} + saErr := &ServiceAccountNotFoundError{key.Name} return nil, saErr } return &req.Status, nil From 1334b699e99650c54a4222245fa00368f5dd0672 Mon Sep 17 00:00:00 2001 From: Rashmi Khanna Date: Wed, 20 Nov 2024 20:07:33 +0530 Subject: [PATCH 17/35] Update clusterextension_controller.go --- internal/controllers/clusterextension_controller.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/controllers/clusterextension_controller.go b/internal/controllers/clusterextension_controller.go index 558a3c05d..a3845098d 100644 --- a/internal/controllers/clusterextension_controller.go +++ b/internal/controllers/clusterextension_controller.go @@ -207,7 +207,7 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1.Cl installedBundle, err := r.InstalledBundleGetter.GetInstalledBundle(ctx, ext) if err != nil { setInstallStatus(ext, nil) - var saerr *tokengetter.SANotFoundError + var saerr *tokengetter.ServiceAccountNotFoundError if errors.As(err, &saerr) { l.Info("is a SAError") err = saerr From 83e188c8f6273d68164be16f71a9116771647811 Mon Sep 17 00:00:00 2001 From: rashmi_kh Date: Tue, 26 Nov 2024 11:14:12 +0530 Subject: [PATCH 18/35] add unit test cases Signed-off-by: rashmi_kh --- .../clusterextension_controller_test.go | 87 +++++++++++++++++++ 1 file changed, 87 insertions(+) diff --git a/internal/controllers/clusterextension_controller_test.go b/internal/controllers/clusterextension_controller_test.go index ab4dc5e18..0f5191a2a 100644 --- a/internal/controllers/clusterextension_controller_test.go +++ b/internal/controllers/clusterextension_controller_test.go @@ -677,6 +677,93 @@ func TestClusterExtensionInstallationSucceeds(t *testing.T) { require.NoError(t, cl.DeleteAllOf(ctx, &ocv1.ClusterExtension{})) } +func TestClusterExtensionInstallationFailsWithNoServiceAccount(t *testing.T) { + cl, reconciler := newClientAndReconciler(t) + reconciler.Unpacker = &MockUnpacker{ + result: &source.Result{ + State: source.StateUnpacked, + Bundle: fstest.MapFS{}, + }, + } + + ctx := context.Background() + extKey := types.NamespacedName{Name: fmt.Sprintf("cluster-extension-test-%s", rand.String(8))} + + t.Log("When the cluster extension specifies a channel with version that exist") + t.Log("By initializing cluster state") + pkgName := "prometheus" + pkgVer := "1.0.0" + pkgChan := "beta" + namespace := fmt.Sprintf("test-ns-%s", rand.String(8)) + serviceAccount := fmt.Sprintf("test-does-not-exist-%s", rand.String(8)) + + clusterExtension := &ocv1.ClusterExtension{ + ObjectMeta: metav1.ObjectMeta{Name: extKey.Name}, + Spec: ocv1.ClusterExtensionSpec{ + Source: ocv1.SourceConfig{ + SourceType: "Catalog", + Catalog: &ocv1.CatalogSource{ + PackageName: pkgName, + Version: pkgVer, + Channels: []string{pkgChan}, + }, + }, + Namespace: namespace, + ServiceAccount: ocv1.ServiceAccountReference{ + Name: serviceAccount, + }, + }, + } + err := cl.Create(ctx, clusterExtension) + require.NoError(t, err) + + t.Log("It sets resolution success status") + t.Log("By running reconcile") + reconciler.Resolver = resolve.Func(func(_ context.Context, _ *ocv1.ClusterExtension, _ *ocv1.BundleMetadata) (*declcfg.Bundle, *bsemver.Version, *declcfg.Deprecation, error) { + v := bsemver.MustParse("1.0.0") + return &declcfg.Bundle{ + Name: "prometheus.v1.0.0", + Package: "prometheus", + Image: "quay.io/operatorhubio/prometheus@fake1.0.0", + }, &v, nil, nil + }) + reconciler.Applier = &MockApplier{ + objs: []client.Object{}, + } + reconciler.Manager = &MockManagedContentCacheManager{ + cache: &MockManagedContentCache{}, + } + res, err := reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: extKey}) + require.Equal(t, ctrl.Result{}, res) + require.NoError(t, err) + + t.Log("By fetching updated cluster extension after reconcile") + require.NoError(t, cl.Get(ctx, extKey, clusterExtension)) + + t.Log("By checking the status fields") + require.Equal(t, ocv1.BundleMetadata{Name: "prometheus.v1.0.0", Version: "1.0.0"}, clusterExtension.Status.Install.Bundle) + + t.Log("By checking the expected installed conditions") + installedCond := apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1.TypeInstalled) + t.Log("By checking the installed conditions message", installedCond.Message) + require.NotNil(t, installedCond) + t.Log("By checking the installed conditions status", installedCond.Status) + t.Log("By checking the installed conditions reason", installedCond.Reason) + require.Equal(t, metav1.ConditionTrue, installedCond.Status) + require.Equal(t, ocv1.ReasonSucceeded, installedCond.Reason) + + t.Log("By checking the expected progressing conditions") + progressingCond := apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1.TypeProgressing) + t.Log("Progressing condition message", progressingCond.Message) + require.NotNil(t, progressingCond) + t.Log("Progressing condition status", progressingCond.Status) + t.Log("Progressing condition reason", progressingCond.Reason) + //require.Equal(t, metav1.ConditionTrue, progressingCond.Status) + require.Equal(t, ocv1.ReasonFailed, progressingCond.Reason) + + //require.NoError(t, cl.DeleteAllOf(ctx, &ocv1.ClusterExtension{})) +} + func TestClusterExtensionDeleteFinalizerFails(t *testing.T) { cl, reconciler := newClientAndReconciler(t) reconciler.Unpacker = &MockUnpacker{ From 683db587eb811255bc31dbd5559cbe94a573107f Mon Sep 17 00:00:00 2001 From: rashmi_kh Date: Tue, 26 Nov 2024 21:44:27 +0530 Subject: [PATCH 19/35] updates testcases Signed-off-by: rashmi_kh --- internal/authentication/tokengetter.go | 11 ++++++++--- internal/authentication/tokengetter_test.go | 2 +- .../controllers/clusterextension_controller_test.go | 6 +++++- 3 files changed, 14 insertions(+), 5 deletions(-) diff --git a/internal/authentication/tokengetter.go b/internal/authentication/tokengetter.go index 5aa3e0a86..7c436f2e9 100644 --- a/internal/authentication/tokengetter.go +++ b/internal/authentication/tokengetter.go @@ -7,6 +7,7 @@ import ( "time" authenticationv1 "k8s.io/api/authentication/v1" + "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" corev1 "k8s.io/client-go/kubernetes/typed/core/v1" @@ -25,7 +26,7 @@ type ServiceAccountNotFoundError struct { } func (e *ServiceAccountNotFoundError) Error() string { - return fmt.Sprintf(" Unable to authenticate with Kubernetes cluster using ServiceAccount \"%s\": ServiceAccount \"%s\" not found.", e.ServiceAccountName, e.ServiceAccountName) + return fmt.Sprintf("Unable to authenticate with Kubernetes cluster using ServiceAccount \"%s\": ServiceAccount \"%s\" not found.", e.ServiceAccountName, e.ServiceAccountName) } type TokenGetterOption func(*TokenGetter) @@ -95,8 +96,12 @@ func (t *TokenGetter) getToken(ctx context.Context, key types.NamespacedName) (* Spec: authenticationv1.TokenRequestSpec{ExpirationSeconds: ptr.To(int64(t.expirationDuration / time.Second))}, }, metav1.CreateOptions{}) if err != nil { - saErr := &ServiceAccountNotFoundError{key.Name} - return nil, saErr + if errors.IsNotFound(err) { + saErr := &ServiceAccountNotFoundError{key.Name} + return nil, saErr + } else { + return nil, err + } } return &req.Status, nil } diff --git a/internal/authentication/tokengetter_test.go b/internal/authentication/tokengetter_test.go index b9553cac3..e60a5e18f 100644 --- a/internal/authentication/tokengetter_test.go +++ b/internal/authentication/tokengetter_test.go @@ -71,7 +71,7 @@ func TestTokenGetterGet(t *testing.T) { {"Testing token that expired 10 seconds ago", "test-service-account-3", "test-namespace-3", "test-token-3", "failed to get token"}, {"Testing error when getting token from fake client", "test-service-account-4", - "test-namespace-4", "error when fetching token", "error when fetching token"}, + "test-namespace-4", "error when fetching token.", "error when fetching token"}, } for _, tc := range tests { diff --git a/internal/controllers/clusterextension_controller_test.go b/internal/controllers/clusterextension_controller_test.go index 0f5191a2a..433249dad 100644 --- a/internal/controllers/clusterextension_controller_test.go +++ b/internal/controllers/clusterextension_controller_test.go @@ -744,6 +744,7 @@ func TestClusterExtensionInstallationFailsWithNoServiceAccount(t *testing.T) { require.Equal(t, ocv1.BundleMetadata{Name: "prometheus.v1.0.0", Version: "1.0.0"}, clusterExtension.Status.Install.Bundle) t.Log("By checking the expected installed conditions") + installedCond := apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1.TypeInstalled) t.Log("By checking the installed conditions message", installedCond.Message) require.NotNil(t, installedCond) @@ -760,8 +761,11 @@ func TestClusterExtensionInstallationFailsWithNoServiceAccount(t *testing.T) { t.Log("Progressing condition reason", progressingCond.Reason) //require.Equal(t, metav1.ConditionTrue, progressingCond.Status) require.Equal(t, ocv1.ReasonFailed, progressingCond.Reason) + failedCond := apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1.ReasonFailed) + t.Log("By checking the failed conditions message", failedCond.Message) + t.Log("By checking the failed conditions status", failedCond.Status, failedCond.Reason) - //require.NoError(t, cl.DeleteAllOf(ctx, &ocv1.ClusterExtension{})) + require.NoError(t, cl.DeleteAllOf(ctx, &ocv1.ClusterExtension{})) } func TestClusterExtensionDeleteFinalizerFails(t *testing.T) { From 7893d9044f15117df9c9ca927f53819b74b5d713 Mon Sep 17 00:00:00 2001 From: rashmi_kh Date: Tue, 26 Nov 2024 22:11:33 +0530 Subject: [PATCH 20/35] reverting this change Signed-off-by: rashmi_kh --- internal/authentication/tokengetter_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/authentication/tokengetter_test.go b/internal/authentication/tokengetter_test.go index e60a5e18f..b9553cac3 100644 --- a/internal/authentication/tokengetter_test.go +++ b/internal/authentication/tokengetter_test.go @@ -71,7 +71,7 @@ func TestTokenGetterGet(t *testing.T) { {"Testing token that expired 10 seconds ago", "test-service-account-3", "test-namespace-3", "test-token-3", "failed to get token"}, {"Testing error when getting token from fake client", "test-service-account-4", - "test-namespace-4", "error when fetching token.", "error when fetching token"}, + "test-namespace-4", "error when fetching token", "error when fetching token"}, } for _, tc := range tests { From bad6695ad04068ea71c4c46f32124061cc366a81 Mon Sep 17 00:00:00 2001 From: rashmi_kh Date: Tue, 26 Nov 2024 22:17:02 +0530 Subject: [PATCH 21/35] review comment Signed-off-by: rashmi_kh --- internal/authentication/tokengetter.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/internal/authentication/tokengetter.go b/internal/authentication/tokengetter.go index 7c436f2e9..bd835cf91 100644 --- a/internal/authentication/tokengetter.go +++ b/internal/authentication/tokengetter.go @@ -99,9 +99,8 @@ func (t *TokenGetter) getToken(ctx context.Context, key types.NamespacedName) (* if errors.IsNotFound(err) { saErr := &ServiceAccountNotFoundError{key.Name} return nil, saErr - } else { - return nil, err } + return nil, err } return &req.Status, nil } From 9c0df21789a438eba76e00d9d317873a705257c4 Mon Sep 17 00:00:00 2001 From: rashmi_kh Date: Tue, 17 Dec 2024 19:57:53 +0530 Subject: [PATCH 22/35] update error msg Signed-off-by: rashmi_kh --- internal/controllers/clusterextension_controller.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/controllers/clusterextension_controller.go b/internal/controllers/clusterextension_controller.go index a3845098d..91d52147b 100644 --- a/internal/controllers/clusterextension_controller.go +++ b/internal/controllers/clusterextension_controller.go @@ -209,7 +209,7 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1.Cl setInstallStatus(ext, nil) var saerr *tokengetter.ServiceAccountNotFoundError if errors.As(err, &saerr) { - l.Info("is a SAError") + l.Info("ServiceAccount defined in the ClusterExtension is not found") err = saerr } setInstalledStatusConditionUnknown(ext, err.Error()) From 5df5a1a0b62f2092e3c33eacc444b4436d992401 Mon Sep 17 00:00:00 2001 From: rashmi_kh Date: Tue, 17 Dec 2024 20:46:38 +0530 Subject: [PATCH 23/35] review comments incorporated Signed-off-by: rashmi_kh --- internal/authentication/tokengetter.go | 8 ++++---- internal/controllers/clusterextension_controller.go | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/internal/authentication/tokengetter.go b/internal/authentication/tokengetter.go index bd835cf91..11a5f98c8 100644 --- a/internal/authentication/tokengetter.go +++ b/internal/authentication/tokengetter.go @@ -22,11 +22,12 @@ type TokenGetter struct { } type ServiceAccountNotFoundError struct { - ServiceAccountName string + ServiceAccountName string // The name of the missing ServiceAccount. } +// Error implements the error interface for ServiceAccountNotFoundError. func (e *ServiceAccountNotFoundError) Error() string { - return fmt.Sprintf("Unable to authenticate with Kubernetes cluster using ServiceAccount \"%s\": ServiceAccount \"%s\" not found.", e.ServiceAccountName, e.ServiceAccountName) + return fmt.Sprintf("ServiceAccount \"%s\" not found: Unable to authenticate with the Kubernetes cluster.", e.ServiceAccountName) } type TokenGetterOption func(*TokenGetter) @@ -97,8 +98,7 @@ func (t *TokenGetter) getToken(ctx context.Context, key types.NamespacedName) (* }, metav1.CreateOptions{}) if err != nil { if errors.IsNotFound(err) { - saErr := &ServiceAccountNotFoundError{key.Name} - return nil, saErr + return nil, &ServiceAccountNotFoundError{ServiceAccountName: key.Name} } return nil, err } diff --git a/internal/controllers/clusterextension_controller.go b/internal/controllers/clusterextension_controller.go index 91d52147b..d7e746832 100644 --- a/internal/controllers/clusterextension_controller.go +++ b/internal/controllers/clusterextension_controller.go @@ -47,10 +47,10 @@ import ( "github.com/operator-framework/api/pkg/operators/v1alpha1" catalogd "github.com/operator-framework/catalogd/api/v1" helmclient "github.com/operator-framework/helm-operator-plugins/pkg/client" - tokengetter "github.com/operator-framework/operator-controller/internal/authentication" "github.com/operator-framework/operator-registry/alpha/declcfg" ocv1 "github.com/operator-framework/operator-controller/api/v1" + "github.com/operator-framework/operator-controller/internal/authentication" "github.com/operator-framework/operator-controller/internal/bundleutil" "github.com/operator-framework/operator-controller/internal/conditionsets" "github.com/operator-framework/operator-controller/internal/contentmanager" @@ -207,7 +207,7 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1.Cl installedBundle, err := r.InstalledBundleGetter.GetInstalledBundle(ctx, ext) if err != nil { setInstallStatus(ext, nil) - var saerr *tokengetter.ServiceAccountNotFoundError + var saerr *authentication.ServiceAccountNotFoundError if errors.As(err, &saerr) { l.Info("ServiceAccount defined in the ClusterExtension is not found") err = saerr From 452703d79fff9c6ec8fe90332f80841522be76b1 Mon Sep 17 00:00:00 2001 From: rashmi_kh Date: Sat, 25 Jan 2025 14:09:02 +0530 Subject: [PATCH 24/35] review comments Signed-off-by: rashmi_kh --- internal/authentication/tokengetter.go | 7 ++++--- internal/controllers/clusterextension_controller.go | 1 - 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/internal/authentication/tokengetter.go b/internal/authentication/tokengetter.go index 11a5f98c8..c12ae513f 100644 --- a/internal/authentication/tokengetter.go +++ b/internal/authentication/tokengetter.go @@ -22,12 +22,13 @@ type TokenGetter struct { } type ServiceAccountNotFoundError struct { - ServiceAccountName string // The name of the missing ServiceAccount. + ServiceAccountName string // The name of the missing ServiceAccount. + ServiceAccountNamespace string // The namespace where the ServiceAccount should exist } // Error implements the error interface for ServiceAccountNotFoundError. func (e *ServiceAccountNotFoundError) Error() string { - return fmt.Sprintf("ServiceAccount \"%s\" not found: Unable to authenticate with the Kubernetes cluster.", e.ServiceAccountName) + return fmt.Sprintf("ServiceAccount \"%s\" not found in namespace \"%s\": Unable to authenticate with the Kubernetes cluster.", e.ServiceAccountName, e.ServiceAccountNamespace) } type TokenGetterOption func(*TokenGetter) @@ -98,7 +99,7 @@ func (t *TokenGetter) getToken(ctx context.Context, key types.NamespacedName) (* }, metav1.CreateOptions{}) if err != nil { if errors.IsNotFound(err) { - return nil, &ServiceAccountNotFoundError{ServiceAccountName: key.Name} + return nil, &ServiceAccountNotFoundError{ServiceAccountName: key.Name, ServiceAccountNamespace: key.Namespace} } return nil, err } diff --git a/internal/controllers/clusterextension_controller.go b/internal/controllers/clusterextension_controller.go index 60ef1dd46..e2b5adad4 100644 --- a/internal/controllers/clusterextension_controller.go +++ b/internal/controllers/clusterextension_controller.go @@ -209,7 +209,6 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1.Cl setInstallStatus(ext, nil) var saerr *authentication.ServiceAccountNotFoundError if errors.As(err, &saerr) { - l.Info("ServiceAccount defined in the ClusterExtension is not found") err = saerr } setInstalledStatusConditionUnknown(ext, err.Error()) From 100c39eeac3b13d6f083e9e7e825984fb5fb26c6 Mon Sep 17 00:00:00 2001 From: rashmi_kh Date: Sat, 25 Jan 2025 14:10:10 +0530 Subject: [PATCH 25/35] review comments Signed-off-by: rashmi_kh --- .../clusterextension_controller_test.go | 18 +++++------------- 1 file changed, 5 insertions(+), 13 deletions(-) diff --git a/internal/controllers/clusterextension_controller_test.go b/internal/controllers/clusterextension_controller_test.go index 433249dad..bb60c9d11 100644 --- a/internal/controllers/clusterextension_controller_test.go +++ b/internal/controllers/clusterextension_controller_test.go @@ -695,7 +695,7 @@ func TestClusterExtensionInstallationFailsWithNoServiceAccount(t *testing.T) { pkgVer := "1.0.0" pkgChan := "beta" namespace := fmt.Sprintf("test-ns-%s", rand.String(8)) - serviceAccount := fmt.Sprintf("test-does-not-exist-%s", rand.String(8)) + serviceAccount := fmt.Sprintf("test-does1-not-exist-%s", rand.String(8)) clusterExtension := &ocv1.ClusterExtension{ ObjectMeta: metav1.ObjectMeta{Name: extKey.Name}, @@ -742,28 +742,20 @@ func TestClusterExtensionInstallationFailsWithNoServiceAccount(t *testing.T) { t.Log("By checking the status fields") require.Equal(t, ocv1.BundleMetadata{Name: "prometheus.v1.0.0", Version: "1.0.0"}, clusterExtension.Status.Install.Bundle) + res, err = reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: extKey}) + require.Error(t, err, res) t.Log("By checking the expected installed conditions") - installedCond := apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1.TypeInstalled) - t.Log("By checking the installed conditions message", installedCond.Message) require.NotNil(t, installedCond) - t.Log("By checking the installed conditions status", installedCond.Status) - t.Log("By checking the installed conditions reason", installedCond.Reason) require.Equal(t, metav1.ConditionTrue, installedCond.Status) require.Equal(t, ocv1.ReasonSucceeded, installedCond.Reason) t.Log("By checking the expected progressing conditions") progressingCond := apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1.TypeProgressing) - t.Log("Progressing condition message", progressingCond.Message) require.NotNil(t, progressingCond) - t.Log("Progressing condition status", progressingCond.Status) - t.Log("Progressing condition reason", progressingCond.Reason) - //require.Equal(t, metav1.ConditionTrue, progressingCond.Status) - require.Equal(t, ocv1.ReasonFailed, progressingCond.Reason) - failedCond := apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1.ReasonFailed) - t.Log("By checking the failed conditions message", failedCond.Message) - t.Log("By checking the failed conditions status", failedCond.Status, failedCond.Reason) + require.Equal(t, metav1.ConditionTrue, progressingCond.Status) + require.Equal(t, ocv1.ReasonSucceeded, progressingCond.Reason) require.NoError(t, cl.DeleteAllOf(ctx, &ocv1.ClusterExtension{})) } From 668e37091f11d395473a0dc4712b791277b50a5a Mon Sep 17 00:00:00 2001 From: rashmi_kh Date: Mon, 27 Jan 2025 11:56:33 +0530 Subject: [PATCH 26/35] add unwrap function Signed-off-by: rashmi_kh --- internal/authentication/tokengetter.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/internal/authentication/tokengetter.go b/internal/authentication/tokengetter.go index c12ae513f..2f4f7a719 100644 --- a/internal/authentication/tokengetter.go +++ b/internal/authentication/tokengetter.go @@ -24,6 +24,11 @@ type TokenGetter struct { type ServiceAccountNotFoundError struct { ServiceAccountName string // The name of the missing ServiceAccount. ServiceAccountNamespace string // The namespace where the ServiceAccount should exist + Err error // The underlying error +} + +func (e *ServiceAccountNotFoundError) Unwrap() error { + return e.Err } // Error implements the error interface for ServiceAccountNotFoundError. From f6e5d66caead70914c915eade8a5c68fe0699dd0 Mon Sep 17 00:00:00 2001 From: rashmi_kh Date: Mon, 27 Jan 2025 12:04:15 +0530 Subject: [PATCH 27/35] add import in the right section Signed-off-by: rashmi_kh --- internal/controllers/clusterextension_controller.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/controllers/clusterextension_controller.go b/internal/controllers/clusterextension_controller.go index e2b5adad4..fd66cfeb9 100644 --- a/internal/controllers/clusterextension_controller.go +++ b/internal/controllers/clusterextension_controller.go @@ -49,8 +49,8 @@ import ( "github.com/operator-framework/operator-registry/alpha/declcfg" ocv1 "github.com/operator-framework/operator-controller/api/v1" - "github.com/operator-framework/operator-controller/internal/authentication" catalogd "github.com/operator-framework/operator-controller/catalogd/api/v1" + "github.com/operator-framework/operator-controller/internal/authentication" "github.com/operator-framework/operator-controller/internal/bundleutil" "github.com/operator-framework/operator-controller/internal/conditionsets" "github.com/operator-framework/operator-controller/internal/contentmanager" From 91bd298868b0cb493eba0ac69a91e8c45742eeec Mon Sep 17 00:00:00 2001 From: rashmi_kh Date: Wed, 29 Jan 2025 13:27:27 +0530 Subject: [PATCH 28/35] review comment about error message with namespace --- internal/authentication/tokengetter.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/authentication/tokengetter.go b/internal/authentication/tokengetter.go index 2f4f7a719..868df0e08 100644 --- a/internal/authentication/tokengetter.go +++ b/internal/authentication/tokengetter.go @@ -33,7 +33,7 @@ func (e *ServiceAccountNotFoundError) Unwrap() error { // Error implements the error interface for ServiceAccountNotFoundError. func (e *ServiceAccountNotFoundError) Error() string { - return fmt.Sprintf("ServiceAccount \"%s\" not found in namespace \"%s\": Unable to authenticate with the Kubernetes cluster.", e.ServiceAccountName, e.ServiceAccountNamespace) + return fmt.Sprintf("service account \"%s\" not found in namespace \"%s\": unable to authenticate with the Kubernetes cluster.", e.ServiceAccountName, e.ServiceAccountNamespace) } type TokenGetterOption func(*TokenGetter) From 03187f4a55c43a2d9665ee1b7a1e52e21e974d2e Mon Sep 17 00:00:00 2001 From: Rashmi Khanna Date: Fri, 31 Jan 2025 14:26:26 +0530 Subject: [PATCH 29/35] Update internal/controllers/clusterextension_controller.go Co-authored-by: Camila Macedo <7708031+camilamacedo86@users.noreply.github.com> --- internal/controllers/clusterextension_controller.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/internal/controllers/clusterextension_controller.go b/internal/controllers/clusterextension_controller.go index fd66cfeb9..f02e8be2c 100644 --- a/internal/controllers/clusterextension_controller.go +++ b/internal/controllers/clusterextension_controller.go @@ -209,7 +209,9 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1.Cl setInstallStatus(ext, nil) var saerr *authentication.ServiceAccountNotFoundError if errors.As(err, &saerr) { - err = saerr + setInstalledStatusConditionUnknown(ext, saerr.Error()) + setStatusProgressing(ext, errors.New("installation cannot proceed due to missing ServiceAccount")) + return ctrl.Result{}, err } setInstalledStatusConditionUnknown(ext, err.Error()) setStatusProgressing(ext, errors.New("retrying to get installed bundle")) From ae8094d1dc18c7cd4f4c800ddf0e8fd725b1b6e0 Mon Sep 17 00:00:00 2001 From: rashmi_kh Date: Fri, 31 Jan 2025 14:31:26 +0530 Subject: [PATCH 30/35] update test with sa condition error Signed-off-by: rashmi_kh --- internal/controllers/clusterextension_controller_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/internal/controllers/clusterextension_controller_test.go b/internal/controllers/clusterextension_controller_test.go index bb60c9d11..0bf26d9b8 100644 --- a/internal/controllers/clusterextension_controller_test.go +++ b/internal/controllers/clusterextension_controller_test.go @@ -743,7 +743,8 @@ func TestClusterExtensionInstallationFailsWithNoServiceAccount(t *testing.T) { t.Log("By checking the status fields") require.Equal(t, ocv1.BundleMetadata{Name: "prometheus.v1.0.0", Version: "1.0.0"}, clusterExtension.Status.Install.Bundle) res, err = reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: extKey}) - require.Error(t, err, res) + require.Error(t, err) + require.Contains(t, err.Error(), "service account", "Expected error about missing ServiceAccount but got: %v", err) t.Log("By checking the expected installed conditions") installedCond := apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1.TypeInstalled) From b103f5ddf1a01029a9affb24cdd071ca57d846f1 Mon Sep 17 00:00:00 2001 From: rashmi_kh Date: Fri, 31 Jan 2025 16:39:11 +0530 Subject: [PATCH 31/35] add mock applier Signed-off-by: rashmi_kh --- .../clusterextension_controller.go | 4 +-- .../clusterextension_controller_test.go | 31 ++++++------------- 2 files changed, 12 insertions(+), 23 deletions(-) diff --git a/internal/controllers/clusterextension_controller.go b/internal/controllers/clusterextension_controller.go index f02e8be2c..9353790db 100644 --- a/internal/controllers/clusterextension_controller.go +++ b/internal/controllers/clusterextension_controller.go @@ -210,8 +210,8 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1.Cl var saerr *authentication.ServiceAccountNotFoundError if errors.As(err, &saerr) { setInstalledStatusConditionUnknown(ext, saerr.Error()) - setStatusProgressing(ext, errors.New("installation cannot proceed due to missing ServiceAccount")) - return ctrl.Result{}, err + setStatusProgressing(ext, errors.New("installation cannot proceed due to missing ServiceAccount")) + return ctrl.Result{}, err } setInstalledStatusConditionUnknown(ext, err.Error()) setStatusProgressing(ext, errors.New("retrying to get installed bundle")) diff --git a/internal/controllers/clusterextension_controller_test.go b/internal/controllers/clusterextension_controller_test.go index 0bf26d9b8..2ac402799 100644 --- a/internal/controllers/clusterextension_controller_test.go +++ b/internal/controllers/clusterextension_controller_test.go @@ -717,6 +717,9 @@ func TestClusterExtensionInstallationFailsWithNoServiceAccount(t *testing.T) { err := cl.Create(ctx, clusterExtension) require.NoError(t, err) + t.Log("By fetching updated cluster extension after reconcile") + require.NoError(t, cl.Get(ctx, extKey, clusterExtension)) + t.Log("It sets resolution success status") t.Log("By running reconcile") reconciler.Resolver = resolve.Func(func(_ context.Context, _ *ocv1.ClusterExtension, _ *ocv1.BundleMetadata) (*declcfg.Bundle, *bsemver.Version, *declcfg.Deprecation, error) { @@ -730,35 +733,21 @@ func TestClusterExtensionInstallationFailsWithNoServiceAccount(t *testing.T) { reconciler.Applier = &MockApplier{ objs: []client.Object{}, } + reconciler.Applier = &MockApplier{ + err: errors.New("missing ServiceAccount"), + } reconciler.Manager = &MockManagedContentCacheManager{ cache: &MockManagedContentCache{}, } - res, err := reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: extKey}) - require.Equal(t, ctrl.Result{}, res) - require.NoError(t, err) + + _, err = reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: extKey}) + require.Error(t, err) t.Log("By fetching updated cluster extension after reconcile") require.NoError(t, cl.Get(ctx, extKey, clusterExtension)) t.Log("By checking the status fields") - require.Equal(t, ocv1.BundleMetadata{Name: "prometheus.v1.0.0", Version: "1.0.0"}, clusterExtension.Status.Install.Bundle) - res, err = reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: extKey}) - require.Error(t, err) - require.Contains(t, err.Error(), "service account", "Expected error about missing ServiceAccount but got: %v", err) - - t.Log("By checking the expected installed conditions") - installedCond := apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1.TypeInstalled) - require.NotNil(t, installedCond) - require.Equal(t, metav1.ConditionTrue, installedCond.Status) - require.Equal(t, ocv1.ReasonSucceeded, installedCond.Reason) - - t.Log("By checking the expected progressing conditions") - progressingCond := apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1.TypeProgressing) - require.NotNil(t, progressingCond) - require.Equal(t, metav1.ConditionTrue, progressingCond.Status) - require.Equal(t, ocv1.ReasonSucceeded, progressingCond.Reason) - - require.NoError(t, cl.DeleteAllOf(ctx, &ocv1.ClusterExtension{})) + require.Contains(t, err.Error(), "missing ServiceAccount") } func TestClusterExtensionDeleteFinalizerFails(t *testing.T) { From 3c21bbab2d6f544de5bd26c87c02d122fbdb6449 Mon Sep 17 00:00:00 2001 From: rashmi_kh Date: Fri, 31 Jan 2025 22:38:27 +0530 Subject: [PATCH 32/35] add e2e test Signed-off-by: rashmi_kh --- .../clusterextension_controller_test.go | 73 ------------------- test/e2e/cluster_extension_install_test.go | 65 +++++++++++++++++ 2 files changed, 65 insertions(+), 73 deletions(-) diff --git a/internal/controllers/clusterextension_controller_test.go b/internal/controllers/clusterextension_controller_test.go index 2ac402799..ab4dc5e18 100644 --- a/internal/controllers/clusterextension_controller_test.go +++ b/internal/controllers/clusterextension_controller_test.go @@ -677,79 +677,6 @@ func TestClusterExtensionInstallationSucceeds(t *testing.T) { require.NoError(t, cl.DeleteAllOf(ctx, &ocv1.ClusterExtension{})) } -func TestClusterExtensionInstallationFailsWithNoServiceAccount(t *testing.T) { - cl, reconciler := newClientAndReconciler(t) - reconciler.Unpacker = &MockUnpacker{ - result: &source.Result{ - State: source.StateUnpacked, - Bundle: fstest.MapFS{}, - }, - } - - ctx := context.Background() - extKey := types.NamespacedName{Name: fmt.Sprintf("cluster-extension-test-%s", rand.String(8))} - - t.Log("When the cluster extension specifies a channel with version that exist") - t.Log("By initializing cluster state") - pkgName := "prometheus" - pkgVer := "1.0.0" - pkgChan := "beta" - namespace := fmt.Sprintf("test-ns-%s", rand.String(8)) - serviceAccount := fmt.Sprintf("test-does1-not-exist-%s", rand.String(8)) - - clusterExtension := &ocv1.ClusterExtension{ - ObjectMeta: metav1.ObjectMeta{Name: extKey.Name}, - Spec: ocv1.ClusterExtensionSpec{ - Source: ocv1.SourceConfig{ - SourceType: "Catalog", - Catalog: &ocv1.CatalogSource{ - PackageName: pkgName, - Version: pkgVer, - Channels: []string{pkgChan}, - }, - }, - Namespace: namespace, - ServiceAccount: ocv1.ServiceAccountReference{ - Name: serviceAccount, - }, - }, - } - err := cl.Create(ctx, clusterExtension) - require.NoError(t, err) - - t.Log("By fetching updated cluster extension after reconcile") - require.NoError(t, cl.Get(ctx, extKey, clusterExtension)) - - t.Log("It sets resolution success status") - t.Log("By running reconcile") - reconciler.Resolver = resolve.Func(func(_ context.Context, _ *ocv1.ClusterExtension, _ *ocv1.BundleMetadata) (*declcfg.Bundle, *bsemver.Version, *declcfg.Deprecation, error) { - v := bsemver.MustParse("1.0.0") - return &declcfg.Bundle{ - Name: "prometheus.v1.0.0", - Package: "prometheus", - Image: "quay.io/operatorhubio/prometheus@fake1.0.0", - }, &v, nil, nil - }) - reconciler.Applier = &MockApplier{ - objs: []client.Object{}, - } - reconciler.Applier = &MockApplier{ - err: errors.New("missing ServiceAccount"), - } - reconciler.Manager = &MockManagedContentCacheManager{ - cache: &MockManagedContentCache{}, - } - - _, err = reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: extKey}) - require.Error(t, err) - - t.Log("By fetching updated cluster extension after reconcile") - require.NoError(t, cl.Get(ctx, extKey, clusterExtension)) - - t.Log("By checking the status fields") - require.Contains(t, err.Error(), "missing ServiceAccount") -} - func TestClusterExtensionDeleteFinalizerFails(t *testing.T) { cl, reconciler := newClientAndReconciler(t) reconciler.Unpacker = &MockUnpacker{ diff --git a/test/e2e/cluster_extension_install_test.go b/test/e2e/cluster_extension_install_test.go index 4c05df8b4..c6320fd5f 100644 --- a/test/e2e/cluster_extension_install_test.go +++ b/test/e2e/cluster_extension_install_test.go @@ -351,6 +351,71 @@ func TestClusterExtensionInstallRegistry(t *testing.T) { } } +func TestClusterExtensionInstallFailsWithoutServiceAccount(t *testing.T) { + t.Log("When a cluster extension is installed from a catalog") + t.Log("And the ServiceAccount does not exist") + + // Step 1: Setup test environment without creating an SA + clusterExtensionName := fmt.Sprintf("clusterextension-%s", rand.String(8)) + + ns, err := createNamespace(context.Background(), clusterExtensionName) + require.NoError(t, err) + + extensionCatalog, err := createTestCatalog(context.Background(), testCatalogName, os.Getenv(testCatalogRefEnvVar)) + require.NoError(t, err) + + // Important: We do NOT create a ServiceAccount here + defer testCleanup(t, extensionCatalog, nil, nil, ns) + defer utils.CollectTestArtifacts(t, artifactName, c, cfg) + + // Step 2: Define the ClusterExtension with a non-existent SA + clusterExtension := &ocv1.ClusterExtension{ + ObjectMeta: metav1.ObjectMeta{ + Name: clusterExtensionName, + }, + Spec: ocv1.ClusterExtensionSpec{ + Source: ocv1.SourceConfig{ + SourceType: "Catalog", + Catalog: &ocv1.CatalogSource{ + PackageName: "test", + Selector: &metav1.LabelSelector{ + MatchLabels: map[string]string{"olm.operatorframework.io/metadata.name": extensionCatalog.Name}, + }, + }, + }, + Namespace: ns.Name, + ServiceAccount: ocv1.ServiceAccountReference{ + Name: "non-existent-sa", // This SA does NOT exist + }, + }, + } + + t.Log("By creating the ClusterExtension resource without a ServiceAccount") + require.NoError(t, c.Create(context.Background(), clusterExtension)) + + // Step 3: Validate failure due to missing ServiceAccount + t.Log("By eventually reporting an installation failure due to missing ServiceAccount") + require.EventuallyWithT(t, func(ct *assert.CollectT) { + assert.NoError(ct, c.Get(context.Background(), types.NamespacedName{Name: clusterExtension.Name}, clusterExtension)) + + // Validate that Progressing reports failure + cond := apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1.TypeProgressing) + if assert.NotNil(ct, cond) { + assert.Equal(ct, metav1.ConditionTrue, cond.Status) + assert.Equal(ct, ocv1.ReasonRetrying, cond.Reason) + assert.Contains(ct, cond.Message, "installation cannot proceed due to missing ServiceAccount") + } + + // Validate that Installed condition is false + installCond := apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1.TypeInstalled) + if assert.NotNil(ct, installCond) { + assert.Equal(ct, metav1.ConditionUnknown, installCond.Status) + assert.Equal(ct, ocv1.ReasonFailed, installCond.Reason) + assert.Contains(ct, installCond.Message, "service account") + } + }, pollDuration, pollInterval) +} + func TestClusterExtensionInstallRegistryDynamic(t *testing.T) { // NOTE: Like 'TestClusterExtensionInstallRegistry', this test also requires extra configuration in /etc/containers/registries.conf packageName := "dynamic" From ddf9c642afd2f85b6340ff34394c0e7cc7fdccc1 Mon Sep 17 00:00:00 2001 From: rashmi_kh Date: Mon, 3 Feb 2025 18:30:31 +0530 Subject: [PATCH 33/35] add nil checks in e2e testcase Signed-off-by: rashmi_kh --- test/e2e/cluster_extension_install_test.go | 35 ++++++++++++++-------- 1 file changed, 22 insertions(+), 13 deletions(-) diff --git a/test/e2e/cluster_extension_install_test.go b/test/e2e/cluster_extension_install_test.go index c6320fd5f..21fa4092c 100644 --- a/test/e2e/cluster_extension_install_test.go +++ b/test/e2e/cluster_extension_install_test.go @@ -251,21 +251,30 @@ func testCleanup(t *testing.T, cat *catalogd.ClusterCatalog, clusterExtension *o 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 ClusterExtension %q", clusterExtension.Name) + if clusterExtension != nil { + 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) + } else { + t.Log(" ClusterExtension is nil", clusterExtension) + } - 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) + if sa != nil { + 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) + if clusterExtension != nil { + ensureNoExtensionResources(t, clusterExtension.Name) + } t.Logf("By deleting Namespace %q", ns.Name) require.NoError(t, c.Delete(context.Background(), ns)) From 04f81cb82d89caa0af88b20f5bc48abe94137a31 Mon Sep 17 00:00:00 2001 From: rashmi_kh Date: Tue, 4 Feb 2025 15:41:10 +0530 Subject: [PATCH 34/35] remove e2e add unit Signed-off-by: rashmi_kh --- internal/authentication/tokengetter_test.go | 4 +- test/e2e/cluster_extension_install_test.go | 65 --------------------- 2 files changed, 3 insertions(+), 66 deletions(-) diff --git a/internal/authentication/tokengetter_test.go b/internal/authentication/tokengetter_test.go index b9553cac3..663e95f1b 100644 --- a/internal/authentication/tokengetter_test.go +++ b/internal/authentication/tokengetter_test.go @@ -72,13 +72,15 @@ func TestTokenGetterGet(t *testing.T) { "test-namespace-3", "test-token-3", "failed to get token"}, {"Testing error when getting token from fake client", "test-service-account-4", "test-namespace-4", "error when fetching token", "error when fetching token"}, + {"Testing service account not found", "missing-sa", + "test-namespace-5", "", "service account \"missing-sa\" not found in namespace \"test-namespace-5\": unable to authenticate with the Kubernetes cluster."}, } for _, tc := range tests { got, err := tg.Get(context.Background(), types.NamespacedName{Namespace: tc.namespace, Name: tc.serviceAccountName}) if err != nil { t.Logf("%s: expected: %v, got: %v", tc.testName, tc.want, err) - assert.EqualError(t, err, tc.errorMsg) + assert.EqualError(t, err, tc.errorMsg, "Error message should match expected output") } else { t.Logf("%s: expected: %v, got: %v", tc.testName, tc.want, got) assert.Equal(t, tc.want, got, tc.errorMsg) diff --git a/test/e2e/cluster_extension_install_test.go b/test/e2e/cluster_extension_install_test.go index 21fa4092c..369f268b4 100644 --- a/test/e2e/cluster_extension_install_test.go +++ b/test/e2e/cluster_extension_install_test.go @@ -360,71 +360,6 @@ func TestClusterExtensionInstallRegistry(t *testing.T) { } } -func TestClusterExtensionInstallFailsWithoutServiceAccount(t *testing.T) { - t.Log("When a cluster extension is installed from a catalog") - t.Log("And the ServiceAccount does not exist") - - // Step 1: Setup test environment without creating an SA - clusterExtensionName := fmt.Sprintf("clusterextension-%s", rand.String(8)) - - ns, err := createNamespace(context.Background(), clusterExtensionName) - require.NoError(t, err) - - extensionCatalog, err := createTestCatalog(context.Background(), testCatalogName, os.Getenv(testCatalogRefEnvVar)) - require.NoError(t, err) - - // Important: We do NOT create a ServiceAccount here - defer testCleanup(t, extensionCatalog, nil, nil, ns) - defer utils.CollectTestArtifacts(t, artifactName, c, cfg) - - // Step 2: Define the ClusterExtension with a non-existent SA - clusterExtension := &ocv1.ClusterExtension{ - ObjectMeta: metav1.ObjectMeta{ - Name: clusterExtensionName, - }, - Spec: ocv1.ClusterExtensionSpec{ - Source: ocv1.SourceConfig{ - SourceType: "Catalog", - Catalog: &ocv1.CatalogSource{ - PackageName: "test", - Selector: &metav1.LabelSelector{ - MatchLabels: map[string]string{"olm.operatorframework.io/metadata.name": extensionCatalog.Name}, - }, - }, - }, - Namespace: ns.Name, - ServiceAccount: ocv1.ServiceAccountReference{ - Name: "non-existent-sa", // This SA does NOT exist - }, - }, - } - - t.Log("By creating the ClusterExtension resource without a ServiceAccount") - require.NoError(t, c.Create(context.Background(), clusterExtension)) - - // Step 3: Validate failure due to missing ServiceAccount - t.Log("By eventually reporting an installation failure due to missing ServiceAccount") - require.EventuallyWithT(t, func(ct *assert.CollectT) { - assert.NoError(ct, c.Get(context.Background(), types.NamespacedName{Name: clusterExtension.Name}, clusterExtension)) - - // Validate that Progressing reports failure - cond := apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1.TypeProgressing) - if assert.NotNil(ct, cond) { - assert.Equal(ct, metav1.ConditionTrue, cond.Status) - assert.Equal(ct, ocv1.ReasonRetrying, cond.Reason) - assert.Contains(ct, cond.Message, "installation cannot proceed due to missing ServiceAccount") - } - - // Validate that Installed condition is false - installCond := apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1.TypeInstalled) - if assert.NotNil(ct, installCond) { - assert.Equal(ct, metav1.ConditionUnknown, installCond.Status) - assert.Equal(ct, ocv1.ReasonFailed, installCond.Reason) - assert.Contains(ct, installCond.Message, "service account") - } - }, pollDuration, pollInterval) -} - func TestClusterExtensionInstallRegistryDynamic(t *testing.T) { // NOTE: Like 'TestClusterExtensionInstallRegistry', this test also requires extra configuration in /etc/containers/registries.conf packageName := "dynamic" From 53494ca311164711e8f9447a33a471d121fc3370 Mon Sep 17 00:00:00 2001 From: rashmi_kh Date: Tue, 4 Feb 2025 17:56:22 +0530 Subject: [PATCH 35/35] remove nil checks --- test/e2e/cluster_extension_install_test.go | 31 +++++++++------------- 1 file changed, 12 insertions(+), 19 deletions(-) diff --git a/test/e2e/cluster_extension_install_test.go b/test/e2e/cluster_extension_install_test.go index 369f268b4..7d5906b45 100644 --- a/test/e2e/cluster_extension_install_test.go +++ b/test/e2e/cluster_extension_install_test.go @@ -251,26 +251,19 @@ func testCleanup(t *testing.T, cat *catalogd.ClusterCatalog, clusterExtension *o return errors.IsNotFound(err) }, pollDuration, pollInterval) - //t.Logf("By deleting ClusterExtension %q", clusterExtension.Name) - if clusterExtension != nil { - 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) - } else { - t.Log(" ClusterExtension is nil", clusterExtension) - } + 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) - if sa != nil { - 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) - } + 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) if clusterExtension != nil { ensureNoExtensionResources(t, clusterExtension.Name)