From ad2dd708836428942058ac72b225fa27ea130fc4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=B6nke=20Liebau?= Date: Thu, 21 Nov 2024 18:57:04 +0100 Subject: [PATCH 1/9] Potential fix for SUP-148 --- crates/stackable-operator/src/commons/rbac.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/stackable-operator/src/commons/rbac.rs b/crates/stackable-operator/src/commons/rbac.rs index 1b7ef2d85..276e16361 100644 --- a/crates/stackable-operator/src/commons/rbac.rs +++ b/crates/stackable-operator/src/commons/rbac.rs @@ -35,7 +35,7 @@ pub fn build_rbac_resources>( rbac_prefix: &str, labels: Labels, ) -> Result<(ServiceAccount, RoleBinding)> { - let sa_name = service_account_name(rbac_prefix); + let sa_name = service_account_name(&resource.name_any()); let service_account = ServiceAccount { metadata: ObjectMetaBuilder::new() .name_and_namespace(resource) @@ -52,7 +52,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(), From 7edfbc2865c19dabb1108d1ba284fa51185eb8f2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=B6nke=20Liebau?= Date: Thu, 21 Nov 2024 20:59:45 +0100 Subject: [PATCH 2/9] Expanded comment on build_rbac_resources a bit. --- crates/stackable-operator/src/commons/rbac.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/crates/stackable-operator/src/commons/rbac.rs b/crates/stackable-operator/src/commons/rbac.rs index 276e16361..89aa5d759 100644 --- a/crates/stackable-operator/src/commons/rbac.rs +++ b/crates/stackable-operator/src/commons/rbac.rs @@ -30,6 +30,12 @@ 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. +/// 'rbac_prefix' 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 +/// (https://stackable.atlassian.net/browse/SUP-148). +/// 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. pub fn build_rbac_resources>( resource: &T, rbac_prefix: &str, From 35a8c9d0fed2c591706eb83c16166a9c04862286 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=B6nke=20Liebau?= Date: Thu, 21 Nov 2024 21:24:28 +0100 Subject: [PATCH 3/9] fix rustdocs complaints --- crates/stackable-operator/src/commons/rbac.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/stackable-operator/src/commons/rbac.rs b/crates/stackable-operator/src/commons/rbac.rs index 89aa5d759..91e962b79 100644 --- a/crates/stackable-operator/src/commons/rbac.rs +++ b/crates/stackable-operator/src/commons/rbac.rs @@ -32,7 +32,7 @@ pub enum Error { /// and it is a assumed that a ClusterRole named `{rbac_prefix}-clusterrole` exists. /// 'rbac_prefix' 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 -/// (https://stackable.atlassian.net/browse/SUP-148). +/// 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. From 154ce7e8de746aa43e120cb839f9494089c620ef Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=B6nke=20Liebau?= Date: Thu, 21 Nov 2024 21:39:22 +0100 Subject: [PATCH 4/9] Adapt tests to the changed behavior. --- crates/stackable-operator/src/commons/rbac.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/stackable-operator/src/commons/rbac.rs b/crates/stackable-operator/src/commons/rbac.rs index 91e962b79..b28dd2bee 100644 --- a/crates/stackable-operator/src/commons/rbac.rs +++ b/crates/stackable-operator/src/commons/rbac.rs @@ -136,7 +136,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" ); @@ -147,7 +147,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" ); From d04b987c75beecd5bc65d40a07084d9e991babf2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=B6nke=20Liebau?= Date: Fri, 22 Nov 2024 11:30:31 +0100 Subject: [PATCH 5/9] 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. --- crates/stackable-operator/src/commons/rbac.rs | 20 ++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/crates/stackable-operator/src/commons/rbac.rs b/crates/stackable-operator/src/commons/rbac.rs index b28dd2bee..1a8dc8950 100644 --- a/crates/stackable-operator/src/commons/rbac.rs +++ b/crates/stackable-operator/src/commons/rbac.rs @@ -28,9 +28,9 @@ 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. -/// 'rbac_prefix' is not used to build the names of the serviceAccount and roleBinding objects, +/// 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. +/// '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 @@ -38,7 +38,7 @@ pub enum Error { /// same objects for multiple clusters. pub fn build_rbac_resources>( resource: &T, - rbac_prefix: &str, + product_name: &str, labels: Labels, ) -> Result<(ServiceAccount, RoleBinding)> { let sa_name = service_account_name(&resource.name_any()); @@ -67,7 +67,7 @@ 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 { @@ -83,13 +83,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") } From 90a6c6eb0da3b064909fd94c907e390647614906 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=B6nke=20Liebau?= Date: Fri, 22 Nov 2024 13:35:19 +0100 Subject: [PATCH 6/9] Add legacy name of serviceAccount to roleBinding as well for better transition period. --- crates/stackable-operator/src/commons/rbac.rs | 38 +++++++++++++------ 1 file changed, 26 insertions(+), 12 deletions(-) diff --git a/crates/stackable-operator/src/commons/rbac.rs b/crates/stackable-operator/src/commons/rbac.rs index 1a8dc8950..9b90d73de 100644 --- a/crates/stackable-operator/src/commons/rbac.rs +++ b/crates/stackable-operator/src/commons/rbac.rs @@ -30,18 +30,22 @@ pub enum Error { /// Build RBAC objects for the product workloads. /// 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. -/// '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. + pub fn build_rbac_resources>( resource: &T, + // '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(&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. + let legacy_sa_name = service_account_name(product_name); let service_account = ServiceAccount { metadata: ObjectMetaBuilder::new() .name_and_namespace(resource) @@ -70,12 +74,22 @@ pub fn build_rbac_resources>( 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)) From eaefb1abb9dada6b9ef37af5b3bd5e2bd4d64467 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=B6nke=20Liebau?= Date: Fri, 22 Nov 2024 13:55:21 +0100 Subject: [PATCH 7/9] Added changelog entry. --- crates/stackable-operator/CHANGELOG.md | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/crates/stackable-operator/CHANGELOG.md b/crates/stackable-operator/CHANGELOG.md index 69ca8485f..3350e5ac6 100644 --- a/crates/stackable-operator/CHANGELOG.md +++ b/crates/stackable-operator/CHANGELOG.md @@ -13,8 +13,17 @@ All notable changes to this project will be documented in this file. ### Changed - BREAKING: Split `ListenerClass.spec.preferred_address_type` into a new `PreferredAddressType` type. Use `resolve_preferred_address_type()` to access the `AddressType` as before. ([#903]) +- 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]) [#903]: https://github.com/stackabletech/operator-rs/pull/903 +[#909]: https://github.com/stackabletech/operator-rs/pull/909 ## [0.80.0] - 2024-10-23 From c219c2a0fd63ffac17738f9c869239f5772681cf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=B6nke=20Liebau?= Date: Fri, 22 Nov 2024 14:05:00 +0100 Subject: [PATCH 8/9] Update crates/stackable-operator/src/commons/rbac.rs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Natalie Klestrup Röijezon --- crates/stackable-operator/src/commons/rbac.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/crates/stackable-operator/src/commons/rbac.rs b/crates/stackable-operator/src/commons/rbac.rs index 9b90d73de..2c38e1d46 100644 --- a/crates/stackable-operator/src/commons/rbac.rs +++ b/crates/stackable-operator/src/commons/rbac.rs @@ -45,6 +45,7 @@ pub fn build_rbac_resources>( 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() From cd36be73cb4a77ca5c3a8ff6914419af264217c9 Mon Sep 17 00:00:00 2001 From: Siegfried Weber Date: Fri, 22 Nov 2024 17:58:29 +0100 Subject: [PATCH 9/9] Fix changelog --- crates/stackable-operator/CHANGELOG.md | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/crates/stackable-operator/CHANGELOG.md b/crates/stackable-operator/CHANGELOG.md index bc1556260..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 @@ -23,17 +32,8 @@ All notable changes to this project will be documented in this file. ### Changed - BREAKING: Split `ListenerClass.spec.preferred_address_type` into a new `PreferredAddressType` type. Use `resolve_preferred_address_type()` to access the `AddressType` as before ([#903]). -- 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]). [#903]: https://github.com/stackabletech/operator-rs/pull/903 -[#909]: https://github.com/stackabletech/operator-rs/pull/909 ## [0.80.0] - 2024-10-23