feat: Tenant specific config support via .cac.toml #246

ayushjain17 · 2024-09-23T13:23:04Z

Problem

Tenant specific configuration not present

Solution

Get tenant-config via cac using cac_toml which reads Superposition.cac.toml

knutties · 2024-09-24T05:44:31Z

crates/cac_toml/Cargo.toml

@@ -17,4 +17,4 @@ path = "src/bin.rs"
 clap = { version = "4.3.0", features = ["derive"] }
 pest = "2.6"
 pest_derive = "2.6"
-toml = { version = "0.8.8", features = ["preserve_order"] }
+toml = { workspace = true }


@ayushjain17 - Will this pick-up the preserve_order if someone just includes cac_toml outside of this workspace? Could we validate that?

yes this will set the features for the crate, in the same way it is specified in the workspace Cargo.toml
have verified this

Datron

Let me know if we can avoid the type ceremony regarding TenantConfig being a trait. Else this is fine

Datron · 2024-09-27T06:19:57Z

crates/superposition_types/src/lib.rs

@@ -257,6 +257,46 @@ impl Display for RegexEnum {
    }
 }

+pub trait SuperpositionTenantConfig {


This could be a struct with pub members right? Why define tenant config as a trait behaviour? No one can implement this elsewhere and pass it around. Why are we making it generic? Note: If the answer summarizes to "We will need this in the future", my response is "lets add it in the future". This is unecessary code complexity right now

I'm guessing this is because of internal asks for auth? The tenant config struct internal to Juspay will be different I'm assuming

Datron · 2024-09-27T06:28:17Z

crates/superposition/src/main.rs

@@ -64,69 +48,16 @@ async fn main() -> Result<()> {
        prefix => "/".to_owned() + prefix,
    };

-    let cac_host: String = get_from_env_unsafe("CAC_HOST").expect("CAC host is not set");
    let cac_port: u16 = get_from_env_unsafe("PORT").unwrap_or(8080);


Nice, we needed this

ShubhranshuSanjeev · 2024-09-28T12:39:16Z

crates/superposition/src/main.rs

            .wrap_fn(|req, srv| {
-                let user = User::default();
+                let state = req.app_data::<Data<AppState>>().unwrap();


@ayushjain17 , can you explain why this change is needed?

ShubhranshuSanjeev · 2024-09-28T12:45:17Z

crates/experimentation_platform/src/api/experiments/handlers.rs

@@ -234,30 +244,48 @@ async fn create(
    // creating variants' context in CAC
    let http_client = reqwest::Client::new();
    let url = state.cac_host.clone() + "/context/bulk-operations";
-    let headers_map = construct_header_map(tenant.as_str(), custom_headers.config_tags)?;
+    let user_str = serde_json::to_string(&user).map_err(|err| {


What are we trying to achieve with x-user header?

Someone who has access for only making changes in experimentation and not in context-aware-config, for them all requests of experimenation would start failing as the when these api calls would be made to make changes in context-aware-config, the authorization would fail due to lack of access.

So Internal authorization has been added
Now since this Internal auth would not have the details of the current user, and on validating it would give the details of a super-user then the audit logs would show wrong info regarding the maker of the contexts in context-aware-config, so in this case since experimentation has already decoded the user, it just forwards the user data

And this user data is only to be consumed if the Internal auth check is successful

Datron

want to understand Authorization internal before approving

Datron · 2024-09-30T06:54:04Z

crates/experimentation_platform/src/api/experiments/handlers.rs

-            "Authorization",
-            format!("{} {}", user.get_auth_type(), user.get_auth_token()),
+            header::AUTHORIZATION,
+            format!("Internal {}", state.superposition_token),


What is internal auth?

This is not a supported authentication scheme
https://developer.mozilla.org/en-US/docs/Web/HTTP/Authentication#authentication_schemes

https://www.iana.org/assignments/http-authschemes/http-authschemes.xhtml

ShubhranshuSanjeev

Can we restrict the internal token access to be only used by experimentation platform and also that this token should not have the full access, just access to CRUD of contexts/overrides will work?

Datron

Minor nitpicks and optimizations, not a PR blocker

Datron · 2024-09-30T09:50:48Z

crates/experimentation_platform/src/api/experiments/handlers.rs

+                None
+            }
+        })
+        .collect::<Vec<_>>();


nitpick: specify types here, makes it easy to read on a glance

Datron · 2024-09-30T09:52:05Z

crates/experimentation_platform/src/api/experiments/handlers.rs

@@ -371,6 +399,16 @@ pub async fn conclude(
                };
                operations.push(ContextAction::MOVE((context_id, context_move_req)));
            } else {
+                let user_str = serde_json::to_string(&user).map_err(|err| {


Do we want to send the whole user object in the headers or only the email address?

Whole user, as what all data would be needed later on the context api or which api being called would not be known

ayushjain17 requested a review from a team as a code owner September 23, 2024 13:23

ayushjain17 force-pushed the tenantConfig branch from 69bdf35 to 1268793 Compare September 23, 2024 13:27

knutties reviewed Sep 24, 2024

View reviewed changes

ayushjain17 requested a review from knutties September 24, 2024 07:59

ayushjain17 force-pushed the tenantConfig branch 2 times, most recently from d4fba6b to eda9cd5 Compare September 25, 2024 12:01

Datron requested changes Sep 27, 2024

View reviewed changes

Datron self-requested a review September 27, 2024 06:47

Datron approved these changes Sep 27, 2024

View reviewed changes

Datron self-requested a review September 27, 2024 07:25

Datron approved these changes Sep 27, 2024

View reviewed changes

ayushjain17 force-pushed the tenantConfig branch 4 times, most recently from d223128 to 008b169 Compare September 27, 2024 14:15

ShubhranshuSanjeev requested changes Sep 28, 2024

View reviewed changes

ShubhranshuSanjeev approved these changes Sep 30, 2024

View reviewed changes

Datron self-requested a review September 30, 2024 06:52

Datron requested changes Sep 30, 2024

View reviewed changes

ShubhranshuSanjeev requested changes Sep 30, 2024

View reviewed changes

Datron approved these changes Sep 30, 2024

View reviewed changes

feat: Tenant specific config support via .cac.toml

665d8b0

ayushjain17 force-pushed the tenantConfig branch from 008b169 to 665d8b0 Compare September 30, 2024 10:28

ShubhranshuSanjeev approved these changes Sep 30, 2024

View reviewed changes

ayushjain17 merged commit ffc247e into main Sep 30, 2024
4 checks passed

ayushjain17 deleted the tenantConfig branch September 30, 2024 10:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Tenant specific config support via .cac.toml #246

feat: Tenant specific config support via .cac.toml #246

ayushjain17 commented Sep 23, 2024

knutties Sep 24, 2024 •

edited

Loading

ayushjain17 Sep 24, 2024

Datron left a comment

Datron Sep 27, 2024 •

edited

Loading

Datron Sep 27, 2024

Datron Sep 27, 2024

ShubhranshuSanjeev Sep 28, 2024

ShubhranshuSanjeev Sep 28, 2024

ayushjain17 Sep 28, 2024

Datron left a comment

Datron Sep 30, 2024

Datron Sep 30, 2024

Datron Sep 30, 2024

ShubhranshuSanjeev left a comment

Datron left a comment

Datron Sep 30, 2024

Datron Sep 30, 2024

ayushjain17 Sep 30, 2024

feat: Tenant specific config support via .cac.toml #246

feat: Tenant specific config support via .cac.toml #246

Conversation

ayushjain17 commented Sep 23, 2024

Problem

Solution

knutties Sep 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Datron left a comment

Choose a reason for hiding this comment

Datron Sep 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Datron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShubhranshuSanjeev left a comment

Choose a reason for hiding this comment

Datron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knutties Sep 24, 2024 •

edited

Loading

Datron Sep 27, 2024 •

edited

Loading