Merge branch 'main' into fix/SUP-148

Author: siegfriedweber

.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

@@ -4,6 +4,16 @@ All notable changes to this project will be documented in this file.
 
 ## [Unreleased]
 
+### 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]).
+
+[#910]: https://github.com/stackabletech/operator-rs/pull/910
+
 ## [0.81.0] - 2024-11-05
 
 ### Added
@@ -12,15 +22,15 @@ 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]).
 - 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])
+  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])
+  (e.g. simple-nifi-rolebinding) ([#909]).
 
 [#903]: https://github.com/stackabletech/operator-rs/pull/903
 [#909]: https://github.com/stackabletech/operator-rs/pull/909

crates/stackable-operator/CHANGELOG.md

@@ -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

@@ -38,7 +38,6 @@ itertools.workspace = true
 k8s-openapi = { workspace = true, optional = true }
 kube = { workspace = true, optional = true }
 proc-macro2.workspace = true
-strum.workspace = true
 syn.workspace = true
 quote.workspace = true
 

crates/stackable-versioned-macros/Cargo.toml

@@ -0,0 +1,23 @@
+#[versioned(
+    version(name = "v1alpha1"),
+    version(name = "v1"),
+    version(name = "v2alpha1")
+)]
+// ---
+pub(crate) mod versioned {
+    pub struct Foo {
+        bar: usize,
+
+        #[versioned(added(since = "v1"))]
+        baz: bool,
+
+        #[versioned(deprecated(since = "v2alpha1"))]
+        deprecated_foo: String,
+    }
+
+    // The following attribute is just to ensure no strange behavior occurs.
+    #[versioned]
+    pub struct Bar {
+        baz: String,
+    }
+}

crates/stackable-versioned-macros/fixtures/inputs/default/module.rs

@@ -0,0 +1,23 @@
+#[versioned(
+    version(name = "v1alpha1"),
+    version(name = "v1"),
+    version(name = "v2alpha1"),
+    preserve_module
+)]
+// ---
+pub(crate) mod versioned {
+    pub struct Foo {
+        bar: usize,
+
+        #[versioned(added(since = "v1"))]
+        baz: bool,
+
+        #[versioned(deprecated(since = "v2alpha1"))]
+        deprecated_foo: String,
+    }
+
+    #[versioned]
+    pub struct Bar {
+        baz: String,
+    }
+}

crates/stackable-versioned-macros/fixtures/inputs/default/module_preserve.rs

@@ -0,0 +1,22 @@
+#[versioned(
+    version(name = "v1alpha1"),
+    version(name = "v1beta1"),
+    version(name = "v1"),
+    k8s(
+        group = "stackable.tech",
+        singular = "foo",
+        plural = "foos",
+        namespaced,
+        skip(merged_crd)
+    )
+)]
+// ---
+#[derive(Clone, Debug, serde::Deserialize, serde::Serialize, schemars::JsonSchema)]
+pub struct FooSpec {
+    #[versioned(
+        added(since = "v1beta1"),
+        changed(since = "v1", from_name = "bah", from_type = "u16")
+    )]
+    bar: usize,
+    baz: bool,
+}