diff --git a/crates/stackable-operator/CHANGELOG.md b/crates/stackable-operator/CHANGELOG.md index 0a99d1e78..20d59b0ff 100644 --- a/crates/stackable-operator/CHANGELOG.md +++ b/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 diff --git a/crates/stackable-operator/src/commons/rbac.rs b/crates/stackable-operator/src/commons/rbac.rs index 1b7ef2d85..2c38e1d46 100644 --- a/crates/stackable-operator/src/commons/rbac.rs +++ b/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>( 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 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>( 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,15 +72,25 @@ pub fn build_rbac_resources>( .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)) @@ -77,13 +98,19 @@ pub fn build_rbac_resources>( /// 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" );