From d113092a486093aa3d9296ab386b8b8b62b825fc Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Fri, 22 Nov 2024 15:18:27 +0100 Subject: [PATCH 1/7] fix: Calculation of OIDC endpoint when rootPath has a trailing slash --- CHANGELOG.md | 1 + Cargo.lock | 8 ++++---- Cargo.toml | 4 ++-- 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a1f72c51..665f3800 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,7 @@ All notable changes to this project will be documented in this file. - Don't ignore envOverrides ([#633]). - Don't print credentials to STDOUT during startup. Ideally we should use [config-utils](https://github.com/stackabletech/config-utils), but that's not easy (see [here](https://github.com/stackabletech/trino-operator/tree/fix/secret-printing)) ([#634]). - Invalid `TrinoCluster`, `TrinoCatalog` or `AuthenticationClass` objects don't stop the operator from reconciliation ([#657]) +- Fix OIDC endpoint calculation in case the `rootPath` does have a trailing slash ([#XXX]). ### Removed diff --git a/Cargo.lock b/Cargo.lock index 645815c7..02efe51c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2225,8 +2225,8 @@ checksum = "6980e8d7511241f8acf4aebddbb1ff938df5eebe98691418c4468d0b72a96a67" [[package]] name = "stackable-operator" -version = "0.80.0" -source = "git+https://github.com/stackabletech/operator-rs.git?tag=stackable-operator-0.80.0#6fbe32300b60f95e0baa2ab0ff2daf961b06531c" +version = "0.81.0" +source = "git+https://github.com/stackabletech//operator-rs.git?branch=fix%2Foidc-endpoint-calculation#9cea26304fdefc42753c34f3e3554e9c63625016" dependencies = [ "chrono", "clap", @@ -2264,7 +2264,7 @@ dependencies = [ [[package]] name = "stackable-operator-derive" version = "0.3.1" -source = "git+https://github.com/stackabletech/operator-rs.git?tag=stackable-operator-0.80.0#6fbe32300b60f95e0baa2ab0ff2daf961b06531c" +source = "git+https://github.com/stackabletech//operator-rs.git?branch=fix%2Foidc-endpoint-calculation#9cea26304fdefc42753c34f3e3554e9c63625016" dependencies = [ "darling", "proc-macro2", @@ -2275,7 +2275,7 @@ dependencies = [ [[package]] name = "stackable-shared" version = "0.0.1" -source = "git+https://github.com/stackabletech/operator-rs.git?tag=stackable-operator-0.80.0#6fbe32300b60f95e0baa2ab0ff2daf961b06531c" +source = "git+https://github.com/stackabletech//operator-rs.git?branch=fix%2Foidc-endpoint-calculation#9cea26304fdefc42753c34f3e3554e9c63625016" dependencies = [ "kube", "semver", diff --git a/Cargo.toml b/Cargo.toml index 118c741d..817f834f 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -30,6 +30,6 @@ strum = { version = "0.26", features = ["derive"] } tokio = { version = "1.40", features = ["full"] } tracing = "0.1" -# [patch."https://github.com/stackabletech/operator-rs.git"] -# stackable-operator = { git = "https://github.com/stackabletech//operator-rs.git", branch = "main" } +[patch."https://github.com/stackabletech/operator-rs.git"] +stackable-operator = { git = "https://github.com/stackabletech//operator-rs.git", branch = "fix/oidc-endpoint-calculation" } # stackable-operator = { path = "../operator-rs/crates/stackable-operator" } From 16bfb585e8aa1b20e9dc6b04dcde8754386badc6 Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Fri, 22 Nov 2024 15:38:23 +0100 Subject: [PATCH 2/7] Update test --- .../src/authentication/oidc/mod.rs | 27 ++++++++++++++----- 1 file changed, 20 insertions(+), 7 deletions(-) diff --git a/rust/operator-binary/src/authentication/oidc/mod.rs b/rust/operator-binary/src/authentication/oidc/mod.rs index 05104f00..8b75b547 100644 --- a/rust/operator-binary/src/authentication/oidc/mod.rs +++ b/rust/operator-binary/src/authentication/oidc/mod.rs @@ -210,10 +210,10 @@ mod tests { use std::mem; use super::*; + use rstest::rstest; use stackable_trino_crd::Container; const IDP_PORT: u16 = 8080; - const IDP_ROOT_PATH: &str = "/realms/master"; const IDP_SCOPE_1: &str = "openid"; const IDP_SCOPE_2: &str = "test"; const AUTH_CLASS_NAME_1: &str = "trino-oidc-auth-1"; @@ -223,12 +223,13 @@ mod tests { fn setup_test_authenticator( auth_class_name: &str, credential_secret: String, + root_path: &str, ) -> OidcAuthenticator { let input = format!( r#" hostname: keycloak port: {IDP_PORT} - rootPath: {IDP_ROOT_PATH} + rootPath: {root_path} scopes: - {IDP_SCOPE_1} principalClaim: preferred_username @@ -249,8 +250,16 @@ mod tests { #[test] fn test_oidc_authentication_limit_one_error() { let oidc_authentication = TrinoOidcAuthentication::new(vec![ - setup_test_authenticator(AUTH_CLASS_NAME_1, AUTH_CLASS_CREDENTIAL_SECRET.to_string()), - setup_test_authenticator(AUTH_CLASS_NAME_2, AUTH_CLASS_CREDENTIAL_SECRET.to_string()), + setup_test_authenticator( + AUTH_CLASS_NAME_1, + AUTH_CLASS_CREDENTIAL_SECRET.to_string(), + "/", + ), + setup_test_authenticator( + AUTH_CLASS_NAME_2, + AUTH_CLASS_CREDENTIAL_SECRET.to_string(), + "/", + ), ]); let error = oidc_authentication @@ -264,17 +273,21 @@ mod tests { ); } - #[test] - fn test_oidc_authentication_settings() { + #[rstest] + #[case("/realms/sdp")] + #[case("/realms/sdp/")] + #[case("/realms/sdp/////")] + fn test_oidc_authentication_settings(#[case] root_path: &str) { let oidc_authentication = TrinoOidcAuthentication::new(vec![setup_test_authenticator( AUTH_CLASS_NAME_1, AUTH_CLASS_CREDENTIAL_SECRET.to_string(), + root_path, )]); let trino_oidc_auth = oidc_authentication.oauth2_authentication_config().unwrap(); assert_eq!( - Some(&format!("http://keycloak:{IDP_PORT}{IDP_ROOT_PATH}")), + Some(&format!("http://keycloak:{IDP_PORT}/realms/sdp")), trino_oidc_auth .config_properties .get(&TrinoRole::Coordinator) From db14917e42b4538353f574963749637a4c980d1a Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Mon, 25 Nov 2024 07:53:31 +0100 Subject: [PATCH 3/7] Use op-rs 0.82.0 --- Cargo.lock | 8 ++++---- Cargo.toml | 6 +++--- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 02efe51c..9eccdf12 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2225,8 +2225,8 @@ checksum = "6980e8d7511241f8acf4aebddbb1ff938df5eebe98691418c4468d0b72a96a67" [[package]] name = "stackable-operator" -version = "0.81.0" -source = "git+https://github.com/stackabletech//operator-rs.git?branch=fix%2Foidc-endpoint-calculation#9cea26304fdefc42753c34f3e3554e9c63625016" +version = "0.82.0" +source = "git+https://github.com/stackabletech/operator-rs.git?tag=stackable-operator-0.82.0#415bbd031bd52e9c0c5392060235030e9930b46b" dependencies = [ "chrono", "clap", @@ -2264,7 +2264,7 @@ dependencies = [ [[package]] name = "stackable-operator-derive" version = "0.3.1" -source = "git+https://github.com/stackabletech//operator-rs.git?branch=fix%2Foidc-endpoint-calculation#9cea26304fdefc42753c34f3e3554e9c63625016" +source = "git+https://github.com/stackabletech/operator-rs.git?tag=stackable-operator-0.82.0#415bbd031bd52e9c0c5392060235030e9930b46b" dependencies = [ "darling", "proc-macro2", @@ -2275,7 +2275,7 @@ dependencies = [ [[package]] name = "stackable-shared" version = "0.0.1" -source = "git+https://github.com/stackabletech//operator-rs.git?branch=fix%2Foidc-endpoint-calculation#9cea26304fdefc42753c34f3e3554e9c63625016" +source = "git+https://github.com/stackabletech/operator-rs.git?tag=stackable-operator-0.82.0#415bbd031bd52e9c0c5392060235030e9930b46b" dependencies = [ "kube", "semver", diff --git a/Cargo.toml b/Cargo.toml index 817f834f..b9c57ba9 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -24,12 +24,12 @@ serde = { version = "1.0", features = ["derive"] } serde_json = "1.0" serde_yaml = "0.9" snafu = "0.8" -stackable-operator = { git = "https://github.com/stackabletech/operator-rs.git", tag = "stackable-operator-0.80.0" } +stackable-operator = { git = "https://github.com/stackabletech/operator-rs.git", tag = "stackable-operator-0.82.0" } product-config = { git = "https://github.com/stackabletech/product-config.git", tag = "0.7.0" } strum = { version = "0.26", features = ["derive"] } tokio = { version = "1.40", features = ["full"] } tracing = "0.1" -[patch."https://github.com/stackabletech/operator-rs.git"] -stackable-operator = { git = "https://github.com/stackabletech//operator-rs.git", branch = "fix/oidc-endpoint-calculation" } +# [patch."https://github.com/stackabletech/operator-rs.git"] +# stackable-operator = { git = "https://github.com/stackabletech//operator-rs.git", branch = "main" } # stackable-operator = { path = "../operator-rs/crates/stackable-operator" } From dcb76b59c5d90543ae2fe3bdf553758e3a290cab Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Mon, 25 Nov 2024 07:55:29 +0100 Subject: [PATCH 4/7] nixfiles --- Cargo.nix | 14 +++++++------- crate-hashes.json | 6 +++--- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/Cargo.nix b/Cargo.nix index f1cef947..47c80888 100644 --- a/Cargo.nix +++ b/Cargo.nix @@ -6816,13 +6816,13 @@ rec { }; "stackable-operator" = rec { crateName = "stackable-operator"; - version = "0.80.0"; + version = "0.82.0"; edition = "2021"; workspace_member = null; src = pkgs.fetchgit { url = "https://github.com/stackabletech/operator-rs.git"; - rev = "6fbe32300b60f95e0baa2ab0ff2daf961b06531c"; - sha256 = "16jrq3wdwz63210jgmqbx3snrr15wxw6l1smqhzv7b7jpq8qvya3"; + rev = "415bbd031bd52e9c0c5392060235030e9930b46b"; + sha256 = "0phasjwb64rxgn5hs8vks92icmx9255bd5v9dms280clrfpcg4hy"; }; libName = "stackable_operator"; authors = [ @@ -6979,8 +6979,8 @@ rec { workspace_member = null; src = pkgs.fetchgit { url = "https://github.com/stackabletech/operator-rs.git"; - rev = "6fbe32300b60f95e0baa2ab0ff2daf961b06531c"; - sha256 = "16jrq3wdwz63210jgmqbx3snrr15wxw6l1smqhzv7b7jpq8qvya3"; + rev = "415bbd031bd52e9c0c5392060235030e9930b46b"; + sha256 = "0phasjwb64rxgn5hs8vks92icmx9255bd5v9dms280clrfpcg4hy"; }; procMacro = true; libName = "stackable_operator_derive"; @@ -7014,8 +7014,8 @@ rec { workspace_member = null; src = pkgs.fetchgit { url = "https://github.com/stackabletech/operator-rs.git"; - rev = "6fbe32300b60f95e0baa2ab0ff2daf961b06531c"; - sha256 = "16jrq3wdwz63210jgmqbx3snrr15wxw6l1smqhzv7b7jpq8qvya3"; + rev = "415bbd031bd52e9c0c5392060235030e9930b46b"; + sha256 = "0phasjwb64rxgn5hs8vks92icmx9255bd5v9dms280clrfpcg4hy"; }; libName = "stackable_shared"; authors = [ diff --git a/crate-hashes.json b/crate-hashes.json index 562fb18b..0ca37e6e 100644 --- a/crate-hashes.json +++ b/crate-hashes.json @@ -1,6 +1,6 @@ { - "git+https://github.com/stackabletech/operator-rs.git?tag=stackable-operator-0.80.0#stackable-operator-derive@0.3.1": "16jrq3wdwz63210jgmqbx3snrr15wxw6l1smqhzv7b7jpq8qvya3", - "git+https://github.com/stackabletech/operator-rs.git?tag=stackable-operator-0.80.0#stackable-operator@0.80.0": "16jrq3wdwz63210jgmqbx3snrr15wxw6l1smqhzv7b7jpq8qvya3", - "git+https://github.com/stackabletech/operator-rs.git?tag=stackable-operator-0.80.0#stackable-shared@0.0.1": "16jrq3wdwz63210jgmqbx3snrr15wxw6l1smqhzv7b7jpq8qvya3", + "git+https://github.com/stackabletech/operator-rs.git?tag=stackable-operator-0.82.0#stackable-operator-derive@0.3.1": "0phasjwb64rxgn5hs8vks92icmx9255bd5v9dms280clrfpcg4hy", + "git+https://github.com/stackabletech/operator-rs.git?tag=stackable-operator-0.82.0#stackable-operator@0.82.0": "0phasjwb64rxgn5hs8vks92icmx9255bd5v9dms280clrfpcg4hy", + "git+https://github.com/stackabletech/operator-rs.git?tag=stackable-operator-0.82.0#stackable-shared@0.0.1": "0phasjwb64rxgn5hs8vks92icmx9255bd5v9dms280clrfpcg4hy", "git+https://github.com/stackabletech/product-config.git?tag=0.7.0#product-config@0.7.0": "0gjsm80g6r75pm3824dcyiz4ysq1ka4c1if6k1mjm9cnd5ym0gny" } \ No newline at end of file From 1abaa607d74685b7f2b0313c1549d68c08b67620 Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Mon, 25 Nov 2024 08:03:13 +0100 Subject: [PATCH 5/7] Update tests and docs --- .../trino/examples/usage-guide/trino-oidc-auth-snippet.yaml | 2 +- examples/simple-trino-oauth2.yaml | 2 +- rust/operator-binary/src/authentication/mod.rs | 2 +- .../kuttl/authentication/create-authentication-classes.yaml.j2 | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/docs/modules/trino/examples/usage-guide/trino-oidc-auth-snippet.yaml b/docs/modules/trino/examples/usage-guide/trino-oidc-auth-snippet.yaml index beda8d29..32534eef 100644 --- a/docs/modules/trino/examples/usage-guide/trino-oidc-auth-snippet.yaml +++ b/docs/modules/trino/examples/usage-guide/trino-oidc-auth-snippet.yaml @@ -21,7 +21,7 @@ spec: oidc: hostname: keycloak.default.svc.cluster.local port: 8080 - rootPath: /realms/stackable + rootPath: /realms/stackable/ scopes: - openid principalClaim: preferred_username diff --git a/examples/simple-trino-oauth2.yaml b/examples/simple-trino-oauth2.yaml index adc9a3bd..7f5f6ee8 100644 --- a/examples/simple-trino-oauth2.yaml +++ b/examples/simple-trino-oauth2.yaml @@ -53,7 +53,7 @@ spec: oidc: hostname: keycloak port: 8080 - rootPath: /realms/stackable + rootPath: /realms/stackable/ scopes: ["openid"] principalClaim: preferred_username --- diff --git a/rust/operator-binary/src/authentication/mod.rs b/rust/operator-binary/src/authentication/mod.rs index b6cdcf86..d0dcce89 100644 --- a/rust/operator-binary/src/authentication/mod.rs +++ b/rust/operator-binary/src/authentication/mod.rs @@ -651,7 +651,7 @@ mod tests { provider: oidc: hostname: {HOST_NAME} - rootPath: /realms/master + rootPath: /realms/master/ scopes: ["openid"] principalClaim: preferred_username "#, diff --git a/tests/templates/kuttl/authentication/create-authentication-classes.yaml.j2 b/tests/templates/kuttl/authentication/create-authentication-classes.yaml.j2 index af0b04bf..2c90ac43 100644 --- a/tests/templates/kuttl/authentication/create-authentication-classes.yaml.j2 +++ b/tests/templates/kuttl/authentication/create-authentication-classes.yaml.j2 @@ -8,7 +8,7 @@ spec: oidc: hostname: keycloak.$NAMESPACE.svc.cluster.local port: 8443 - rootPath: /realms/stackable + rootPath: /realms/stackable/ scopes: - openid principalClaim: preferred_username From 3540de36a07c29633191edf3d0e140c73c4183f9 Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Mon, 25 Nov 2024 08:10:12 +0100 Subject: [PATCH 6/7] changelog --- CHANGELOG.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 665f3800..0b6c1011 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,7 +24,7 @@ All notable changes to this project will be documented in this file. - Don't ignore envOverrides ([#633]). - Don't print credentials to STDOUT during startup. Ideally we should use [config-utils](https://github.com/stackabletech/config-utils), but that's not easy (see [here](https://github.com/stackabletech/trino-operator/tree/fix/secret-printing)) ([#634]). - Invalid `TrinoCluster`, `TrinoCatalog` or `AuthenticationClass` objects don't stop the operator from reconciliation ([#657]) -- Fix OIDC endpoint calculation in case the `rootPath` does have a trailing slash ([#XXX]). +- Fix OIDC endpoint calculation in case the `rootPath` does have a trailing slash ([#673]). ### Removed @@ -37,6 +37,7 @@ All notable changes to this project will be documented in this file. [#646]: https://github.com/stackabletech/trino-operator/pull/646 [#655]: https://github.com/stackabletech/trino-operator/pull/655 [#657]: https://github.com/stackabletech/trino-operator/pull/657 +[#673]: https://github.com/stackabletech/trino-operator/pull/673 ## [24.7.0] - 2024-07-24 From 890ab263e9854c0fcecfeb74b0f701a26b04e08e Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Mon, 25 Nov 2024 11:10:32 +0100 Subject: [PATCH 7/7] Rename oidc -> client_auth_options --- CHANGELOG.md | 2 +- rust/crd/src/authentication.rs | 4 ++-- rust/operator-binary/src/authentication/mod.rs | 10 +++++----- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0b6c1011..7c61502f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,7 +24,7 @@ All notable changes to this project will be documented in this file. - Don't ignore envOverrides ([#633]). - Don't print credentials to STDOUT during startup. Ideally we should use [config-utils](https://github.com/stackabletech/config-utils), but that's not easy (see [here](https://github.com/stackabletech/trino-operator/tree/fix/secret-printing)) ([#634]). - Invalid `TrinoCluster`, `TrinoCatalog` or `AuthenticationClass` objects don't stop the operator from reconciliation ([#657]) -- Fix OIDC endpoint calculation in case the `rootPath` does have a trailing slash ([#673]). +- Fix OIDC endpoint construction in case the `rootPath` does have a trailing slash ([#673]). ### Removed diff --git a/rust/crd/src/authentication.rs b/rust/crd/src/authentication.rs index fad0d3d0..05c13e30 100644 --- a/rust/crd/src/authentication.rs +++ b/rust/crd/src/authentication.rs @@ -25,7 +25,7 @@ type Result = std::result::Result; pub struct ResolvedAuthenticationClassRef { /// An [AuthenticationClass](DOCS_BASE_URL_PLACEHOLDER/concepts/authentication) to use. pub authentication_class: AuthenticationClass, - pub oidc: Option, + pub client_auth_options: Option, } /// Retrieve all provided AuthenticationClass references. @@ -43,7 +43,7 @@ pub async fn resolve_authentication_classes( let auth_class_name = resolved_auth_class.name_any(); resolved_auth_classes.push(ResolvedAuthenticationClassRef { - oidc: match &resolved_auth_class.spec.provider { + client_auth_options: match &resolved_auth_class.spec.provider { AuthenticationClassProvider::Oidc(_) => Some( client_authentication_detail .oidc_or_error(&auth_class_name) diff --git a/rust/operator-binary/src/authentication/mod.rs b/rust/operator-binary/src/authentication/mod.rs index d0dcce89..bfad5804 100644 --- a/rust/operator-binary/src/authentication/mod.rs +++ b/rust/operator-binary/src/authentication/mod.rs @@ -508,7 +508,7 @@ impl TryFrom> for TrinoAuthenticationTypes { ); } AuthenticationClassProvider::Oidc(provider) => { - let oidc = resolved_auth_class.oidc.context( + let oidc = resolved_auth_class.client_auth_options.context( OidcAuthenticationDetailsNotSpecifiedSnafu { auth_class_name: auth_class_name.clone(), }, @@ -591,7 +591,7 @@ mod tests { ResolvedAuthenticationClassRef { authentication_class: input, - oidc: None, + client_auth_options: None, } } @@ -610,7 +610,7 @@ mod tests { ResolvedAuthenticationClassRef { authentication_class: input, - oidc: None, + client_auth_options: None, } } @@ -636,7 +636,7 @@ mod tests { ResolvedAuthenticationClassRef { authentication_class: input, - oidc: None, + client_auth_options: None, } } @@ -663,7 +663,7 @@ mod tests { deserializer, ) .unwrap(), - oidc: Some(ClientAuthenticationOptions { + client_auth_options: Some(ClientAuthenticationOptions { client_credentials_secret_ref: "my-oidc-secret".to_string(), extra_scopes: Vec::new(), product_specific_fields: (),