Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(keymanager): add tenant-id to keymanager requests #6968

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

dracarys18
Copy link
Member

@dracarys18 dracarys18 commented Jan 2, 2025

Type of Change

  • Enhancement

Description

Adds tenant-id for every requests to keymanager

Motivation and Context

Added tenant id header for the keymanager service to classify key ids based on tenants.

How did you test it?

  • Create Merchant Account with x-tenant-id as public
curl --location 'http://localhost:8080/accounts' \
--header 'Content-Type: application/json' \
--header 'Accept: application/json' \
--header 'x-feature: integ-custom' \
--header 'x-tenant-id: public' \
--header 'api-key: test_admin' \
--data-raw '{
  "merchant_id": "1735815159",
  "locker_id": "m0010",
  "merchant_name": "NewAge Retailer",
  "merchant_details": {
    "primary_contact_person": "John Test",
    "primary_email": "[email protected]",
    "primary_phone": "sunt laborum",
    "secondary_contact_person": "John Test2",
    "secondary_email": "[email protected]",
    "secondary_phone": "cillum do dolor id",
    "website": "www.example.com",
    "about_business": "Online Retail with a wide selection of organic products for North America",
    "address": {
      "line1": "1467",
      "line2": "Harrison Street",
      "line3": "Harrison Street",
      "city": "San Fransico",
      "state": "California",
      "zip": "94122",
      "country": "US"
    }
  },
  "return_url": "https://google.com/success",
  "webhook_details": {
    "webhook_version": "1.0.1",
    "webhook_username": "ekart_retail",
    "webhook_password": "password_ekart@123",
    "payment_created_enabled": true,
    "payment_succeeded_enabled": true,
    "payment_failed_enabled": true
  },
  "sub_merchants_enabled": false,
  "metadata": {
    "city": "NY",
    "unit": "245"
  },
  "primary_business_details": [
    {
      "country": "US",
      "business": "default"
    }
  ]
}'
  • Get the x-request-id for the request and query it in grafana and see the if the tenant_id is public
    Screenshot 2025-01-02 at 4 24 01 PM

Checklist

  • I formatted the code cargo +nightly fmt --all
  • I addressed lints thrown by cargo clippy
  • I reviewed the submitted code

@dracarys18 dracarys18 added A-core Area: Core flows S-waiting-on-review Status: This PR has been implemented and needs to be reviewed labels Jan 2, 2025
@dracarys18 dracarys18 added this to the December 2024 Release milestone Jan 2, 2025
@dracarys18 dracarys18 self-assigned this Jan 2, 2025
@dracarys18 dracarys18 requested a review from a team as a code owner January 2, 2025 09:46
Copy link

semanticdiff-com bot commented Jan 2, 2025

Review changes with  SemanticDiff

Changed Files
File Status
  crates/router/src/configs/settings.rs  27% smaller
  crates/common_utils/src/keymanager.rs  1% smaller
  config/development.toml Unsupported file format
  crates/common_utils/src/id_type/tenant.rs  0% smaller
  crates/common_utils/src/types/keymanager.rs  0% smaller
  crates/router/src/configs/defaults.rs  0% smaller
  crates/router/src/types/domain/types.rs  0% smaller

@dracarys18 dracarys18 linked an issue Jan 2, 2025 that may be closed by this pull request
crates/common_utils/src/id_type/tenant.rs Outdated Show resolved Hide resolved
crates/router/src/configs/settings.rs Outdated Show resolved Hide resolved
impl Default for super::settings::GlobalTenant {
fn default() -> Self {
Self {
tenant_id: id_type::TenantId::new_unchecked(String::new()),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel passing in empty string here could cause issues when this value is being serialized/deserialized.

@Narayanbhat166 Correct me if I'm wrong.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the new_unchecked does not validate the length

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But deserialization would. So, even if the application does start running with the empty string as the tenant ID, and say the tenant ID is serialized / deserialized when writing to / reading from Redis, then the serialization may go through but the deserialization would fail, thus possibly causing an internal server error.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why can't we have the default tenant id as well? isn't it global?

@@ -794,7 +794,7 @@ sdk_eligible_payment_methods = "card"

[multitenancy]
enabled = false
global_tenant = { schema = "public", redis_key_prefix = "", clickhouse_database = "default"}
global_tenant = { tenant_id = "global" ,schema = "public", redis_key_prefix = "global", clickhouse_database = "default"}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you replicate similar changes in the other TOML config files which have global_tenant configuration?

@@ -15,6 +15,13 @@ crate::impl_queryable_id_type!(TenantId);
crate::impl_to_sql_from_sql_id_type!(TenantId);

impl TenantId {
/// Construct TenantID without checking for length constraints
pub fn new_unchecked(input_string: String) -> Self {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function should not be public

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need this public to be used in router crate :/

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

then we can create the function which will create the tenant id with specified name, we cannot accept any generic string for unchecked functions.

Reference:

pub fn get_merchant_id_not_found() -> Self {

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-core Area: Core flows S-waiting-on-review Status: This PR has been implemented and needs to be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[feat] add multitenancy support to keymanager
3 participants