Fixes accidentally deleted serviceaccount and rolebinding objects (#909)

* Potential fix for SUP-148

* Expanded comment on build_rbac_resources a bit.

* fix rustdocs complaints

* Adapt tests to the changed behavior.

* Changed role_binding_name and service_account_name to not be public anymore.
Operators should not call these with potentially wrong parameters, but instead use `build_rbac_resources` to retrieve the objects and read the name from there.

* Add legacy name of serviceAccount to roleBinding as well for better transition period.

* Added changelog entry.

* Update crates/stackable-operator/src/commons/rbac.rs

Co-authored-by: Natalie Klestrup Röijezon <[email protected]>

* Fix changelog

---------

Co-authored-by: Natalie Klestrup Röijezon <[email protected]>
Co-authored-by: Siegfried Weber <[email protected]>

Author: 3 people

crates/stackable-operator/CHANGELOG.md

@@ -11,7 +11,16 @@ All notable changes to this project will be documented in this file.
 ### Changed
 
 - BREAKING: Made `DEFAULT_OIDC_WELLKNOWN_PATH` private. Use `AuthenticationProvider::well_known_config_url` instead ([#910]).
-
+- BREAKING: Changed visibility of `commons::rbac::service_account_name` and `commons::rbac::role_binding_name` to
+  private, as these functions should not be called directly by the operators. This is likely to result in naming conflicts
+  as the result is completely dependent on what is passed to this function. Operators should instead rely on the roleBinding
+  and serviceAccount objects created by `commons::rbac::build_rbac_resources` and retrieve the name from the returned
+  objects if they need it ([#909]).
+- Changed the names of the objects that are returned from `commons::rbac::build_rbac_resources` to not rely solely on the product
+  they refer to (e.g. "nifi-rolebinding") but instead include the name of the resource to be unique per cluster
+  (e.g. simple-nifi-rolebinding) ([#909]).
+
+[#909]: https://github.com/stackabletech/operator-rs/pull/909
 [#910]: https://github.com/stackabletech/operator-rs/pull/910
 
 ## [0.81.0] - 2024-11-05

crates/stackable-operator/src/commons/rbac.rs

@@ -28,14 +28,25 @@ pub enum Error {
 }
 
 /// Build RBAC objects for the product workloads.
-/// The `rbac_prefix` is meant to be the product name, for example: zookeeper, airflow, etc.
-/// and it is a assumed that a ClusterRole named `{rbac_prefix}-clusterrole` exists.
+/// The `product_name` is meant to be the product name, for example: zookeeper, airflow, etc.
+/// and it is a assumed that a ClusterRole named `{product_name}-clusterrole` exists.
+
 pub fn build_rbac_resources<T: Clone + Resource<DynamicType = ()>>(
     resource: &T,
-    rbac_prefix: &str,
+    // 'product_name' is not used to build the names of the serviceAccount and roleBinding objects,
+    // as this caused problems with multiple clusters of the same product within the same namespace
+    // see <https://stackable.atlassian.net/browse/SUP-148> for more details.
+    // Instead the names for these objects are created by reading the name from the cluster object
+    // and appending [-rolebinding|-serviceaccount] to create unique names instead of using the
+    // same objects for multiple clusters.
+    product_name: &str,
     labels: Labels,
 ) -> Result<(ServiceAccount, RoleBinding)> {
-    let sa_name = service_account_name(rbac_prefix);
+    let sa_name = service_account_name(&resource.name_any());
+    // We add the legacy serviceAccount name to the binding here for at least one
+    // release cycle, so that the switchover during the upgrade can be smoother.
+    // To be removed in v24.3+1.
+    let legacy_sa_name = service_account_name(product_name);
     let service_account = ServiceAccount {
         metadata: ObjectMetaBuilder::new()
             .name_and_namespace(resource)
@@ -52,7 +63,7 @@ pub fn build_rbac_resources<T: Clone + Resource<DynamicType = ()>>(
     let role_binding = RoleBinding {
         metadata: ObjectMetaBuilder::new()
             .name_and_namespace(resource)
-            .name(role_binding_name(rbac_prefix))
+            .name(role_binding_name(&resource.name_any()))
             .ownerreference_from_resource(resource, None, Some(true))
             .context(RoleBindingOwnerReferenceFromResourceSnafu {
                 name: resource.name_any(),
@@ -61,29 +72,45 @@ pub fn build_rbac_resources<T: Clone + Resource<DynamicType = ()>>(
             .build(),
         role_ref: RoleRef {
             kind: "ClusterRole".to_string(),
-            name: format!("{rbac_prefix}-clusterrole"),
+            name: format!("{product_name}-clusterrole"),
             api_group: "rbac.authorization.k8s.io".to_string(),
         },
-        subjects: Some(vec![Subject {
-            kind: "ServiceAccount".to_string(),
-            name: sa_name,
-            namespace: resource.namespace(),
-            ..Subject::default()
-        }]),
+        subjects: Some(vec![
+            Subject {
+                kind: "ServiceAccount".to_string(),
+                name: sa_name,
+                namespace: resource.namespace(),
+                ..Subject::default()
+            },
+            // We add the legacy serviceAccount name to the binding here for at least one
+            // release cycle, so that the switchover during the upgrade can be smoother.
+            Subject {
+                kind: "ServiceAccount".to_string(),
+                name: legacy_sa_name,
+                namespace: resource.namespace(),
+                ..Subject::default()
+            },
+        ]),
     };
 
     Ok((service_account, role_binding))
 }
 
 /// Generate the service account name.
 /// The `rbac_prefix` is meant to be the product name, for example: zookeeper, airflow, etc.
-pub fn service_account_name(rbac_prefix: &str) -> String {
+/// This is private because operators should not use this function to calculate names for
+/// serviceAccount objects, but rather read the name from the objects returned by
+/// `build_rbac_resources` if they need the name.
+fn service_account_name(rbac_prefix: &str) -> String {
     format!("{rbac_prefix}-serviceaccount")
 }
 
 /// Generate the role binding name.
 /// The `rbac_prefix` is meant to be the product name, for example: zookeeper, airflow, etc.
-pub fn role_binding_name(rbac_prefix: &str) -> String {
+/// This is private because operators should not use this function to calculate names for
+/// roleBinding objects, but rather read the name from the objects returned by
+/// `build_rbac_resources` if they need the name.
+fn role_binding_name(rbac_prefix: &str) -> String {
     format!("{rbac_prefix}-rolebinding")
 }
 
@@ -130,7 +157,7 @@ mod tests {
             build_rbac_resources(&cluster, RESOURCE_NAME, Labels::new()).unwrap();
 
         assert_eq!(
-            Some(service_account_name(RESOURCE_NAME)),
+            Some(service_account_name(CLUSTER_NAME)),
             rbac_sa.metadata.name,
             "service account does not match"
         );
@@ -141,7 +168,7 @@ mod tests {
         );
 
         assert_eq!(
-            Some(role_binding_name(RESOURCE_NAME)),
+            Some(role_binding_name(CLUSTER_NAME)),
             rbac_rolebinding.metadata.name,
             "rolebinding does not match"
         );