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

infra: add support for decoding configuration hostnames from SDK keys… #252

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

Conversation

leoromanovsky
Copy link
Member

@leoromanovsky leoromanovsky commented Mar 18, 2025

… (FF-4143)

Motivation and Context

🎫 https://linear.app/eppo/issue/FF-4143/add-support-for-js-commons-to-decode-per-sdk-key-host

To add support for off-boarding customers from both the assignment and edge configuration APIs, we will be encoding a company specific hostname into key. Please see associated design doc for further details.

There will be 3 generations of SDKs "in the wild":

  • Those without any hostname encoding (majority at this time)
  • Containing an event ingestion hostname encoding (all created after November 2024)
  • Future created with both an event ingestion and configuration hostname (after related server changes are merged)

We need to augment the existing decoder class to handle the new cases and support existing ones.

Description

  • Adds two new public methods to SdkDecoder: decodeAssignmentConfigurationUrl and decodeEdgeConfigurationUrl. These will be used by the rules-based client and precomputed client, respectively. Those changes are not in scope for this PR.
  • Refactored into a re-usable method decodeHostnames

How has this been tested?

New unit tests handling the 3 generations of SDK encodings.

@leoromanovsky leoromanovsky marked this pull request as ready for review March 18, 2025 02:59
@@ -1,27 +1,70 @@
import { Base64 } from 'js-base64';

const PATH = 'v0/i';
const EVENT_INGESTION_HOSTNAME_KEY = 'eh';
const CONFIGURATION_HOSTNAME_KEY = 'ch';
Copy link
Member Author

Choose a reason for hiding this comment

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

@typotter Since I got here first 😉 , I selected ch (for configuration host). Please reflect this with the server changes.

Comment on lines +19 to +33
/**
* Decodes and returns the configuration hostname from the provided Eppo SDK key string.
* If the SDK key doesn't contain the configuration hostname, or it's invalid, it returns null.
*/
decodeAssignmentConfigurationUrl(sdkKey: string): string | null {
return this.decodeHostnames(sdkKey, ASSIGNMENT_CONFIG_PATH).configurationHostname;
}

/**
* Decodes and returns the edge configuration hostname from the provided Eppo SDK key string.
* If the SDK key doesn't contain the edge configuration hostname, or it's invalid, it returns null.
*/
decodeEdgeConfigurationUrl(sdkKey: string): string | null {
return this.decodeHostnames(sdkKey, EDGE_CONFIG_PATH).configurationHostname;
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Looking into the future a bit: the clients allow for a baseUrl to be passed in; however we will no longer allow this to be fscdn.eppo.cloud if a hostname is decoded here as non-null and it will thrown an exception otherwise people will be able to bypass our DNS blocks. Down the line once all SDK keys have rotated or some future date, we can enforce a subdomain to be present and block bare requests to fscdn.eppo.cloud in VCL.


const EVENT_INGESTION_PATH = 'v0/i';
const ASSIGNMENT_CONFIG_PATH = 'assignment';
const EDGE_CONFIG_PATH = 'edge';
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems this is intended to follow the same pattern used for event ingestion. The EVENT_INGESTION_PATH is used to return the full path/URL to be used. This doesn't seem to be doing that. I'd recommend we put the full path here in this file to be consistent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants