Merge branch 'main' into refactor/remove-derivative

Author: sbernauer

.github/workflows/build.yml

@@ -17,7 +17,7 @@ env:
   CARGO_TERM_COLOR: always
   CARGO_INCREMENTAL: '0'
   CARGO_PROFILE_DEV_DEBUG: '0'
-  RUST_TOOLCHAIN_VERSION: "1.81.0"
+  RUST_TOOLCHAIN_VERSION: "1.82.0"
   RUSTFLAGS: "-D warnings"
   RUSTDOCFLAGS: "-D warnings"
   RUST_LOG: "info"

.github/workflows/pr_pre-commit.yaml

@@ -6,7 +6,7 @@ on:
 
 env:
   CARGO_TERM_COLOR: always
-  RUST_TOOLCHAIN_VERSION: "1.81.0"
+  RUST_TOOLCHAIN_VERSION: "1.82.0"
   HADOLINT_VERSION: "v1.17.6"
 
 jobs:
@@ -16,29 +16,10 @@ jobs:
       - uses: actions/checkout@eef61447b9ff4aafe5dcd4e0bbf5d482be7e7871 # v4.2.1
         with:
           fetch-depth: 0
-      - uses: actions/setup-python@82c7e631bb3cdc910f68e0081d67478d79c6982d # v5.1.0
+      - uses: stackabletech/actions/run-pre-commit@9bd13255f286e4b7a654617268abe1b2f37c3e0a # v0.3.0
         with:
-          python-version: '3.12'
-      - uses: dtolnay/rust-toolchain@master
-        with:
-          toolchain: ${{ env.RUST_TOOLCHAIN_VERSION }}
-          components: rustfmt,clippy
-      - name: Setup Hadolint
-        shell: bash
-        run: |
-          set -euo pipefail
-
-          LOCATION_DIR="$HOME/.local/bin"
-          LOCATION_BIN="$LOCATION_DIR/hadolint"
-
-          SYSTEM=$(uname -s)
-          ARCH=$(uname -m)
-
-          mkdir -p "$LOCATION_DIR"
-          curl -sL -o "${LOCATION_BIN}" "https://github.com/hadolint/hadolint/releases/download/${{ env.HADOLINT_VERSION }}/hadolint-$SYSTEM-$ARCH"
-          chmod 700 "${LOCATION_BIN}"
-
-          echo "$LOCATION_DIR" >> "$GITHUB_PATH"
-      - uses: pre-commit/action@2c7b3805fd2a0fd8c1884dcaebf91fc102a13ecd # v3.0.1
-        with:
-          extra_args: "--from-ref ${{ github.event.pull_request.base.sha }} --to-ref ${{ github.event.pull_request.head.sha }}"
+          rust: ${{ env.RUST_TOOLCHAIN_VERSION }}
+          # rust-src is required for trybuild stderr output comparison to work
+          # for our cases.
+          # See: https://github.com/dtolnay/trybuild/issues/236#issuecomment-1620950759
+          rust-components: rustfmt,clippy,rust-src

Cargo.lock

@@ -10,6 +10,27 @@ All notable changes to this project will be documented in this file.
 
 [#907]: https://github.com/stackabletech/operator-rs/pull/907
 
+## [0.82.0] - 2024-11-23
+
+### Fixed
+
+- Fixed URL handling related to OIDC and `rootPath` with and without trailing slashes. Also added a bunch of tests ([#910]).
+
+### 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
 
 ### Added
@@ -18,7 +39,7 @@ 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: Split `ListenerClass.spec.preferred_address_type` into a new `PreferredAddressType` type. Use `resolve_preferred_address_type()` to access the `AddressType` as before ([#903]).
 
 [#903]: https://github.com/stackabletech/operator-rs/pull/903
 

crates/stackable-operator/CHANGELOG.md

@@ -1,7 +1,7 @@
 [package]
 name = "stackable-operator"
 description = "Stackable Operator Framework"
-version = "0.81.0"
+version = "0.82.0"
 authors.workspace = true
 license.workspace = true
 edition.workspace = true

crates/stackable-operator/Cargo.toml

@@ -17,17 +17,19 @@ use crate::commons::{
 
 pub type Result<T, E = Error> = std::result::Result<T, E>;
 
-pub const DEFAULT_OIDC_WELLKNOWN_PATH: &str = ".well-known/openid-configuration";
 pub const CLIENT_ID_SECRET_KEY: &str = "clientId";
 pub const CLIENT_SECRET_SECRET_KEY: &str = "clientSecret";
 
+/// Do *not* use this for [`Url::join`], as the leading slash will erase the existing path!
+const DEFAULT_WELLKNOWN_OIDC_CONFIG_PATH: &str = "/.well-known/openid-configuration";
+
 #[derive(Debug, PartialEq, Snafu)]
 pub enum Error {
     #[snafu(display("failed to parse OIDC endpoint url"))]
     ParseOidcEndpointUrl { source: ParseError },
 
     #[snafu(display(
-        "failed to set OIDC endpoint scheme '{scheme}' for endpoint url '{endpoint}'"
+        "failed to set OIDC endpoint scheme '{scheme}' for endpoint url \"{endpoint}\""
     ))]
     SetOidcEndpointScheme { endpoint: Url, scheme: String },
 }
@@ -111,10 +113,10 @@ impl AuthenticationProvider {
         }
     }
 
-    /// Returns the OIDC endpoint [`Url`]. To append the default OIDC well-known
-    /// configuration path, use `url.join()`. This module provides the default
-    /// path at [`DEFAULT_OIDC_WELLKNOWN_PATH`].
-    pub fn endpoint_url(&self) -> Result<Url> {
+    /// Returns the OIDC base [`Url`] without any path segments.
+    ///
+    /// The base url only contains the scheme, the host, and an optional port.
+    fn base_url(&self) -> Result<Url> {
         let mut url = Url::parse(&format!(
             "http://{host}:{port}",
             host = self.hostname.as_url_host(),
@@ -132,7 +134,37 @@ impl AuthenticationProvider {
             })?;
         }
 
-        url.set_path(&self.root_path);
+        Ok(url)
+    }
+
+    /// Returns the OIDC endpoint [`Url`] without a trailing slash.
+    ///
+    /// To retrieve the well-known OIDC configuration url, please use [`Self::well_known_config_url`].
+    pub fn endpoint_url(&self) -> Result<Url> {
+        let mut url = self.base_url()?;
+        // Some tools can not cope with a trailing slash, so let's remove that
+        url.set_path(self.root_path.trim_end_matches('/'));
+        Ok(url)
+    }
+
+    /// Returns the well-known OIDC configuration [`Url`] without a trailing slash.
+    ///
+    /// The returned url is a combination of [`Self::endpoint_url`] joined with
+    /// the well-known OIDC configuration path `DEFAULT_WELLKNOWN_OIDC_CONFIG_PATH`.
+    pub fn well_known_config_url(&self) -> Result<Url> {
+        let mut url = self.base_url()?;
+
+        // Taken from https://docs.rs/url/latest/url/struct.Url.html#method.join:
+        // A trailing slash is significant. Without it, the last path component is considered to be
+        // a “file” name to be removed to get at the “directory” that is used as the base.
+        //
+        // Because of that behavior, we first need to make sure that the root path doesn't contain
+        // any trailing slashes to finally append the well-known config path to the url. The path
+        // already contains a prefixed slash.
+        let mut root_path_with_trailing_slash = self.root_path.trim_end_matches('/').to_string();
+        root_path_with_trailing_slash.push_str(DEFAULT_WELLKNOWN_OIDC_CONFIG_PATH);
+        url.set_path(&root_path_with_trailing_slash);
+
         Ok(url)
     }
 
@@ -246,6 +278,8 @@ pub struct ClientAuthenticationOptions<T = ()> {
 
 #[cfg(test)]
 mod test {
+    use rstest::rstest;
+
     use super::*;
 
     #[test]
@@ -310,12 +344,8 @@ mod test {
         .unwrap();
 
         assert_eq!(
-            oidc.endpoint_url()
-                .unwrap()
-                .join(DEFAULT_OIDC_WELLKNOWN_PATH)
-                .unwrap()
-                .as_str(),
-            "https://my.keycloak.server/.well-known/openid-configuration"
+            oidc.endpoint_url().unwrap().as_str(),
+            "https://my.keycloak.server/"
         );
     }
 
@@ -338,6 +368,70 @@ mod test {
         );
     }
 
+    #[rstest]
+    #[case("/", "http://my.keycloak.server:1234/")]
+    #[case("/realms/sdp", "http://my.keycloak.server:1234/realms/sdp")]
+    #[case("/realms/sdp/", "http://my.keycloak.server:1234/realms/sdp")]
+    #[case("/realms/sdp//////", "http://my.keycloak.server:1234/realms/sdp")]
+    #[case(
+        "/realms/my/realm/with/slashes//////",
+        "http://my.keycloak.server:1234/realms/my/realm/with/slashes"
+    )]
+    fn root_path_endpoint_url(#[case] root_path: String, #[case] expected_endpoint_url: &str) {
+        let oidc = serde_yaml::from_str::<AuthenticationProvider>(&format!(
+            "
+            hostname: my.keycloak.server
+            port: 1234
+            rootPath: {root_path}
+            scopes: [openid]
+            principalClaim: preferred_username
+            "
+        ))
+        .unwrap();
+
+        assert_eq!(oidc.endpoint_url().unwrap().as_str(), expected_endpoint_url);
+    }
+
+    #[rstest]
+    #[case("/", "https://my.keycloak.server/.well-known/openid-configuration")]
+    #[case(
+        "/realms/sdp",
+        "https://my.keycloak.server/realms/sdp/.well-known/openid-configuration"
+    )]
+    #[case(
+        "/realms/sdp/",
+        "https://my.keycloak.server/realms/sdp/.well-known/openid-configuration"
+    )]
+    #[case(
+        "/realms/sdp//////",
+        "https://my.keycloak.server/realms/sdp/.well-known/openid-configuration"
+    )]
+    #[case(
+        "/realms/my/realm/with/slashes//////",
+        "https://my.keycloak.server/realms/my/realm/with/slashes/.well-known/openid-configuration"
+    )]
+    fn root_path_well_known_url(#[case] root_path: String, #[case] expected_well_known_url: &str) {
+        let oidc = serde_yaml::from_str::<AuthenticationProvider>(&format!(
+            "
+            hostname: my.keycloak.server
+            rootPath: {root_path}
+            scopes: [openid]
+            principalClaim: preferred_username
+            tls:
+              verification:
+                server:
+                  caCert:
+                    webPki: {{}}
+            "
+        ))
+        .unwrap();
+
+        assert_eq!(
+            oidc.well_known_config_url().unwrap().as_str(),
+            expected_well_known_url
+        );
+    }
+
     #[test]
     fn client_env_vars() {
         let secret_name = "my-keycloak-client";

crates/stackable-operator/src/commons/authentication/oidc.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"
         );