From 052c4dffb640d78814322a3aa55282c945bb63e2 Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Mon, 25 Nov 2024 12:37:24 +0100 Subject: [PATCH 1/3] fix: Construction of OIDC endpoint when rootPath has a trailing slash --- Cargo.lock | 9 +-- Cargo.nix | 18 +++-- Cargo.toml | 3 +- crate-hashes.json | 6 +- rust/operator-binary/Cargo.toml | 2 +- rust/operator-binary/src/config.rs | 103 ++++++++++++++++++----------- 6 files changed, 88 insertions(+), 53 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 0efb5c36..fdda7751 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2198,6 +2198,7 @@ dependencies = [ "futures 0.3.31", "indoc", "product-config", + "rstest", "serde", "serde_yaml", "snafu 0.8.5", @@ -2210,8 +2211,8 @@ dependencies = [ [[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.82.0" +source = "git+https://github.com/stackabletech/operator-rs.git?tag=stackable-operator-0.82.0#415bbd031bd52e9c0c5392060235030e9930b46b" dependencies = [ "chrono", "clap", @@ -2249,7 +2250,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?tag=stackable-operator-0.82.0#415bbd031bd52e9c0c5392060235030e9930b46b" dependencies = [ "darling", "proc-macro2", @@ -2260,7 +2261,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?tag=stackable-operator-0.82.0#415bbd031bd52e9c0c5392060235030e9930b46b" dependencies = [ "kube", "semver", diff --git a/Cargo.nix b/Cargo.nix index 22c13476..f3b0d8be 100644 --- a/Cargo.nix +++ b/Cargo.nix @@ -6822,6 +6822,10 @@ rec { } ]; devDependencies = [ + { + name = "rstest"; + packageId = "rstest"; + } { name = "serde_yaml"; packageId = "serde_yaml"; @@ -6831,13 +6835,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 = [ @@ -6994,8 +6998,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"; @@ -7029,8 +7033,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/Cargo.toml b/Cargo.toml index 38123e38..1ec6feb8 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -23,10 +23,11 @@ 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" } 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" } +# stackable-operator = { path = "../operator-rs/crates/stackable-operator" } 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 diff --git a/rust/operator-binary/Cargo.toml b/rust/operator-binary/Cargo.toml index 9ffd50f7..e2e45049 100644 --- a/rust/operator-binary/Cargo.toml +++ b/rust/operator-binary/Cargo.toml @@ -26,8 +26,8 @@ tracing.workspace = true indoc.workspace = true [build-dependencies] - built.workspace = true [dev-dependencies] +rstest.workspace = true serde_yaml.workspace = true diff --git a/rust/operator-binary/src/config.rs b/rust/operator-binary/src/config.rs index 2d7feff7..ddc741fe 100644 --- a/rust/operator-binary/src/config.rs +++ b/rust/operator-binary/src/config.rs @@ -26,6 +26,16 @@ pub enum Error { FailedToCreateLdapEndpointUrl { source: stackable_operator::commons::authentication::ldap::Error, }, + + #[snafu(display("invalid OIDC endpoint"))] + InvalidOidcEndpoint { + source: stackable_operator::commons::authentication::oidc::Error, + }, + + #[snafu(display("invalid well-known OIDC configuration URL"))] + InvalidWellKnownConfigUrl { + source: stackable_operator::commons::authentication::oidc::Error, + }, } pub fn add_airflow_config( @@ -78,7 +88,7 @@ fn append_authentication_config( } if !oidc_providers.is_empty() { - append_oidc_config(config, &oidc_providers); + append_oidc_config(config, &oidc_providers)?; } config.insert( @@ -192,7 +202,7 @@ fn append_oidc_config( &oidc::AuthenticationProvider, &oidc::ClientAuthenticationOptions<()>, )], -) { +) -> Result<(), Error> { // Debatable: AUTH_OAUTH or AUTH_OID // Additionally can be set via config config.insert( @@ -217,6 +227,13 @@ fn append_oidc_config( let oauth_providers_config_entry = match oidc_provider { oidc::IdentityProviderHint::Keycloak => { + let endpoint_url = oidc.endpoint_url().context(InvalidOidcEndpointSnafu)?; + let mut api_base_url = endpoint_url.as_str().trim_end_matches('/').to_owned(); + api_base_url.push_str("/protocol/"); + let well_known_config_url = oidc + .well_known_config_url() + .context(InvalidWellKnownConfigUrlSnafu)?; + formatdoc!( " {{ 'name': 'keycloak', @@ -228,11 +245,10 @@ fn append_oidc_config( 'client_kwargs': {{ 'scope': '{scopes}' }}, - 'api_base_url': '{url}/protocol/', - 'server_metadata_url': '{url}/.well-known/openid-configuration', + 'api_base_url': '{api_base_url}', + 'server_metadata_url': '{well_known_config_url}', }}, }}", - url = oidc.endpoint_url().unwrap(), scopes = scopes.join(" "), ) } @@ -251,12 +267,15 @@ fn append_oidc_config( joined_oauth_providers_config = oauth_providers_config.join(",\n") ), ); + + Ok(()) } #[cfg(test)] mod tests { use crate::config::add_airflow_config; - use indoc::indoc; + use indoc::formatdoc; + use rstest::rstest; use stackable_airflow_crd::authentication::{ default_sync_roles_at, default_user_registration, AirflowAuthenticationClassResolved, AirflowClientAuthenticationDetailsResolved, FlaskRolesSyncMoment, @@ -339,12 +358,15 @@ mod tests { ]), result); } - #[test] - fn test_oidc_config() { - let oidc_provider_yaml1 = r#" - hostname: my.keycloak1.server + #[rstest] + #[case("/realms/sdp")] + #[case("/realms/sdp/")] + #[case("/realms/sdp/////")] + fn test_oidc_config(#[case] root_path: &str) { + let oidc_provider_yaml1 = formatdoc!( + "hostname: my.keycloak1.server port: 12345 - rootPath: my-root-path + rootPath: {root_path} tls: verification: server: @@ -356,8 +378,9 @@ mod tests { - email - profile provider_hint: Keycloak - "#; - let deserializer = serde_yaml::Deserializer::from_str(oidc_provider_yaml1); + " + ); + let deserializer = serde_yaml::Deserializer::from_str(&oidc_provider_yaml1); let oidc_provider1: oidc::AuthenticationProvider = serde_yaml::with::singleton_map_recursive::deserialize(deserializer).unwrap(); @@ -399,41 +422,47 @@ mod tests { let mut result = BTreeMap::new(); add_airflow_config(&mut result, &authentication_config).expect("Ok"); - assert_eq!(BTreeMap::from([ - ("AUTH_ROLES_SYNC_AT_LOGIN".into(), "false".into()), - ("AUTH_TYPE".into(), "AUTH_OAUTH".into()), - ("AUTH_USER_REGISTRATION".into(), "true".into()), - ("AUTH_USER_REGISTRATION_ROLE".into(), "Admin".into()), - ("OAUTH_PROVIDERS".into(), indoc!(" + assert_eq!( + BTreeMap::from([ + ("AUTH_ROLES_SYNC_AT_LOGIN".into(), "false".into()), + ("AUTH_TYPE".into(), "AUTH_OAUTH".into()), + ("AUTH_USER_REGISTRATION".into(), "true".into()), + ("AUTH_USER_REGISTRATION_ROLE".into(), "Admin".into()), + ( + "OAUTH_PROVIDERS".into(), + formatdoc! {" [ - { 'name': 'keycloak', + {{ 'name': 'keycloak', 'icon': 'fa-key', 'token_key': 'access_token', - 'remote_app': { + 'remote_app': {{ 'client_id': os.environ.get('OIDC_A96BCC4FA49835D2_CLIENT_ID'), 'client_secret': os.environ.get('OIDC_A96BCC4FA49835D2_CLIENT_SECRET'), - 'client_kwargs': { + 'client_kwargs': {{ 'scope': 'openid email profile roles' - }, - 'api_base_url': 'https://my.keycloak1.server:12345/my-root-path/protocol/', - 'server_metadata_url': 'https://my.keycloak1.server:12345/my-root-path/.well-known/openid-configuration', - }, - }, - { 'name': 'keycloak', + }}, + 'api_base_url': 'https://my.keycloak1.server:12345/realms/sdp/protocol/', + 'server_metadata_url': 'https://my.keycloak1.server:12345/realms/sdp/.well-known/openid-configuration', + }}, + }}, + {{ 'name': 'keycloak', 'icon': 'fa-key', 'token_key': 'access_token', - 'remote_app': { + 'remote_app': {{ 'client_id': os.environ.get('OIDC_3A305E38C3B561F3_CLIENT_ID'), 'client_secret': os.environ.get('OIDC_3A305E38C3B561F3_CLIENT_SECRET'), - 'client_kwargs': { + 'client_kwargs': {{ 'scope': 'openid' - }, - 'api_base_url': 'http://my.keycloak2.server//protocol/', - 'server_metadata_url': 'http://my.keycloak2.server//.well-known/openid-configuration', - }, - } + }}, + 'api_base_url': 'http://my.keycloak2.server/protocol/', + 'server_metadata_url': 'http://my.keycloak2.server/.well-known/openid-configuration', + }}, + }} ] - ").into()) - ]), result); + "} + ) + ]), + result + ); } } From 97a531957c3c5175678e91de3fa4b5ff9c8ad47d Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Mon, 25 Nov 2024 12:42:33 +0100 Subject: [PATCH 2/3] changelog --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6fc34dbb..49878a9c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,7 @@ - Pass gitsync credentials through properly and use a fine-grained access token ([#489]). - Failing to parse one `AirflowCluster`/`AuthenticationClass` should no longer cause the whole operator to stop functioning ([#520]). +- Fix OIDC endpoint construction in case the `rootPath` does not have a trailing slash ([#547]). [#488]: https://github.com/stackabletech/airflow-operator/pull/488 [#489]: https://github.com/stackabletech/airflow-operator/pull/489 @@ -35,6 +36,7 @@ [#520]: https://github.com/stackabletech/airflow-operator/pull/520 [#524]: https://github.com/stackabletech/airflow-operator/pull/524 [#530]: https://github.com/stackabletech/airflow-operator/pull/530 +[#547]: https://github.com/stackabletech/airflow-operator/pull/547 ## [24.7.0] - 2024-07-24 From f34eaae2bd55d8963b48d800a07a80796469db0e Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Mon, 25 Nov 2024 13:03:39 +0100 Subject: [PATCH 3/3] changelog --- CHANGELOG.md | 1 - 1 file changed, 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8f839ea2..593f0951 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -28,7 +28,6 @@ - Failing to parse one `AirflowCluster`/`AuthenticationClass` should no longer cause the whole operator to stop functioning ([#520]). - BREAKING: Use distinct ServiceAccounts for the Stacklets, so that multiple Stacklets can be deployed in one namespace. Existing Stacklets will use the newly created ServiceAccounts after restart ([#545]). - Fix OIDC endpoint construction in case the `rootPath` does not have a trailing slash ([#547]). -======= [#488]: https://github.com/stackabletech/airflow-operator/pull/488 [#489]: https://github.com/stackabletech/airflow-operator/pull/489