Skip to content

feat: Add environment document endpoint #150

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.

Sign up for GitHub

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

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

feat: Add environment document endpoint #150

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
a381b3d
add environment-document endpoint
rolodato Mar 14, 2025
8c6bb9c
nit
rolodato Mar 14, 2025
2a91b59
remove prints
rolodato Mar 17, 2025
c1fba53
separate environment document test cases
rolodato Mar 17, 2025
f5bae61
Abstract environment-document into service, make args explicit
rolodato Mar 17, 2025
bb36b68
Merge branch 'main' into feat/serve-env-doc
rolodato Mar 17, 2025
6033eb3
lint
rolodato Mar 17, 2025
2a47f01
Merge branch 'main' into feat/serve-env-doc
rolodato Apr 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions src/edge_proxy/server.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,22 @@ async def get_identities(
return ORJSONResponse(data)


@app.get("/api/v1/environment-document", response_class=ORJSONResponse)
async def environment_document(
x_environment_key: str = Header(None),
) -> ORJSONResponse:
for key_pair in settings.environment_key_pairs:
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess some alternatives to this loop would be:

  1. Create an entry in the cache for each key, including the server keys (this would obviously increase memory usage)
  2. Create some other map in the cache to point from a server key to a client key

Of these 2, probably the 2nd option is the better option, but it does require a non-trivial amount of effort. I think, based on that, I'm happy with the for loop as a concept for now, but I think we should abstract it to the environment service layer and create some method like get_client_key_from_server_key(server_key: str). I guess then, we might as well just extend the get_environment method to check if the key starts with "ser.".

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 kept the same caching behaviour and moved this to get_environment in the service layer.

I'm not a fan of checking for server/client keys by looking at the ser. prefix, so I made get_environment require explicitly named client_side_key or server_side_key arguments.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not a fan of checking for server/client keys by looking at the ser. prefix, so I made get_environment require explicitly named client_side_key or server_side_key arguments.

While I tend to agree that it's not the neatest way of differentiation, I don't really see any actual issues with it. The issue with the approach that you've taken is that we might end up in a scenario where an application, pointing to the edge proxy, has a local evaluation and remote evaluation client running. They might though, share the same key from some environment variable. The remote evaluation client will thus fail, because it's expecting the key to be a client-side key.

Maybe this is unnecessary complication, but in opening up one endpoint to the server key, we may need to consider opening them all up.

Copy link
Contributor

Choose a reason for hiding this comment

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

I just re-read the above, and it took me a while to understand again what my point was, so here's an example application to demonstrate the issue that I'm talking about.

from flagsmith import Flagsmith, Flags


FLAGSMITH_KEY = os.environ["FLAGSMITH_KEY"]

class FeatureFlagWrapper:
    def __init__(self) -> None:
        self.remote_evaluation_client = Flagsmith(environment_key=FLAGSMITH_KEY)
        self.local_evaluation_client = Flagsmith(
            environment_key=FLAGSMITH_KEY,
            enable_local_evaluation=True,
            environment_refresh_interval_seconds=300,
        )

    def get_flag_state(flag_key: str, from_local: bool = True) -> bool:
        if from_local:
            return self.local_evaluation_client.get_environment_flags().is_feature_enabled(flag_key)
        else:
            return self.remote_evaluation_client.get_environment_flags().is_feature_enabled(flag_key)

Obviously this is an over simplification, but I suspect that this is often a valid use case, where people care about certain flags being updated sooner than others, so they set a longer refresh interval on their local evaluation client, and then use a remove evaluation client for anything more sensitive. Obviously this is further complicated by the fact that we're talking about the edge proxy here, which has it's own refresh interval, but I still think this is a valid concern.

print(key_pair)
if key_pair.server_side_key == x_environment_key:
environment_doc = environment_service.get_environment(
key_pair.client_side_key
)
print(environment_doc is None)
if environment_doc:
return ORJSONResponse(environment_doc)
return ORJSONResponse(status_code=401, content=None)


@app.on_event("startup")
@repeat_every(
seconds=settings.api_poll_frequency_seconds,
Expand Down
39 changes: 38 additions & 1 deletion tests/test_server.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
from fastapi.testclient import TestClient
from pytest_mock import MockerFixture

from edge_proxy.settings import AppSettings, HealthCheckSettings
from edge_proxy.settings import AppSettings, HealthCheckSettings, EnvironmentKeyPair
from tests.fixtures.response_data import environment_1

if typing.TYPE_CHECKING:
Expand Down Expand Up @@ -287,3 +287,40 @@ def test_get_identities(
assert response.status_code == 200
assert data["traits"] == []
assert data["flags"]


@pytest.mark.parametrize(
"environment_key,expected_status",
[
("ser.good", 200),
("ser.bad", 401),
(None, 401),
],
)
def test_get_environment_document(
mocker: MockerFixture,
client: TestClient,
environment_key: str | None,
expected_status: int,
) -> None:
# Given
environment_key_pairs = [
EnvironmentKeyPair(server_side_key="ser.good", client_side_key="foo")
]
mocker.patch(
"edge_proxy.server.settings.environment_key_pairs", environment_key_pairs
)
mocker.patch(
"edge_proxy.server.environment_service.cache"
).get_environment.return_value = environment_1

# When
response = client.get(
"/api/v1/environment-document",
headers={"X-Environment-Key": environment_key} if environment_key else None,
)

# Then
assert response.status_code == expected_status
if expected_status == 200:
assert response.json() == environment_1