[braddf]

Pull Request

Description

Each v1 deployment can now say which countries it serves, and dev can show countries and models that prod doesn't. It's the groundwork for EU and India running from the same image, and it puts Germany 🇩🇪 on dev.

Countries per deployment.

V1_COUNTRIES is a comma-separated allowlist, e.g. GB,NL. The full catalogue in country_config.py is now ALL_COUNTRIES, and COUNTRIES is what this deployment serves. Everything already reads COUNTRIES (the OpenAPI enums, the path validation, /countries, /region-types and cache warming), so they all narrow with it. A request for a country the deployment doesn't serve is a 422, and /v1/docs only lists the countries it serves.

Dev-only entries

Countries, region types and models can be tagged stage="dev". With V1_STAGE=prod, the default, they're hidden. With V1_STAGE=dev, everything is served. Promoting something to prod is a one-line PR, plus adding the country to prod's V1_COUNTRIES if it's a new country.

Misconfigured config startup check

The server won't start on an unknown country code, an unknown stage, or a region type whose default model is hidden at that stage. An empty /countries would look like a DP outage. The startup log shows what was selected: v1 serving countries ['DE', 'GB', 'NL'] at stage 'dev'.

Both vars are read from the environment when country_config is imported, because the OpenAPI enums are built at import time. They're also in server.conf like every other setting, and startup fails if the two disagree, so a default can't be changed in one place only.

Germany, dev only.

National plus the four TSOs (50Hertz, Amprion, TenneT, TransnetBW) as region type tso, with a 15-minute time step, entsoe_de for observed generation, and the read:de scope. Six models, named after their inputs like GB and NL:

model_name	DP forecaster
ecmwf_mo_pv_sat (default)	de_ecmwf_pv_mo_sat
ecmwf_pv	de_ecmwf_pv
ecmwf	de_ecmwf_only
mo	de_mo_only
sat_8h	de_sat_only
pv	de_pv_only

The adjuster is only available nationally. The DP has no _adjust forecasts for the TSOs, so adjusted is ignored there, like GB gsp. For now, every model is listed for every region type; we'll remove the ones that don't apply once it's clearer which ones run where.

Needs the infra PR to set both vars explicitly in Terraform:

Env	V1_COUNTRIES	V1_STAGE
dev	GB,NL,DE	dev
prod	GB,NL	prod

Tip

The two PRs can be merged in either order. Nothing reads the vars before this is deployed, and without them this serves GB and NL by default at prod, as per usual.

This isn't access control; which data a deployment can reach is still decided by the DP it points at, and the read:* scopes still determine who can see what.

Testing

• Unit tests for the filter: allowlist parsing, hiding by stage, and each case that stops startup. Another test checks that the real catalogue is valid at both stages.
• Startup tests that run the server with different V1_COUNTRIES / V1_STAGE combinations. Each checks the OpenAPI country enum and which countries return 200 or 422, and that bad values stop startup.
• Tests run against the full catalogue even when those vars are set in the shell.
• Checked DE against the live DP: TSO locations, forecasts for every model at national and TSO level, entsoe_de generation, and the TSO snapshot. Also ran it through the new UI locally.

Checklist:

• My code follows OCF's coding style guidelines
• I have performed a self-review of my own code
• I have made corresponding changes to the documentation
• I have added tests that prove my fix is effective or that my feature works
• I have checked my code and corrected any misspellings

[devsjc]

Ah, I see now what's going on with this stage now: it's basically an implementation of feature flags.

It feels to me at first pass that this could be simplified by not having the separately configured routers and stages, but rather having both of these things simply as feature flags, and every route is tagged or gated behind the appropriate set of flags. So, the environment variable would become FEATURE_FLAGS and it would be set to e.g. "SITES,GB,DEV", and that would only enable the routes that are tagged with all of the above.

Some pseudocode example:

def feature_flags(*flags: str):
    def decorator(func: Callable) -> Callable:
        func._required_flags = flags
        return func
    return decorator

@router.get("/blah")
@feature_flags("GB", "DEV")
def wow_check_out_this_route():
    return ...
    
 # ... in the lifecycle ...
 app.router.routes = [
    route for route in app.router.routes
    if not hasattr(route, "endpoint") or 
    all(flag in ACTIVE_FLAGS for flag in getattr(route.endpoint, "_required_flags", []))
]

ACTIVE_FLAGS coming from the environment. Seems a bit cleaner to me than two different implementations of the same sort of idea. (And yes, I know I set up the separate routers in the first place! I should have used flags from the start!)

[braddf]

@devsjc I agree, the routers def feels like a clean cut feature flags situation – in hindsight! It was much more flexible than not having it before, so harder to see ahead of time if that makes you feel any better...

I can see the consolidation angle on this suggested approach, but I do feel it loses some of the rigour here – the typings mostly... if countries are a specific limiting factor on a deployment, we can validate on boot, and not accidentally think we're un-gating something when we're not; then the countries all live in path params rather than the route themselves, so you'd have dynamic validation functions within the handlers regardless – validate_model and the VALID_COUNTRY_CODES check both do that atm, and a decorator would sit in front of them but not replace them. Same with models, they need to be reconciled with allowed models at deployment-level and user-level, so it's not quite as straightforward as a nice split set of routes or single decorator AFAICS.

The other thing route filtering doesn't reach is everything downstream of the spec that isn't a route: _warm_all_v1_caches derives its warm targets from COUNTRIES, so a dev-only country would still get its caches pre-warmed on prod with nothing to serve them from. You could still do this with feature flags, but not with the same decorator. Same for the model_name enum in the OpenAPI schema and /region-types – dropping a route can't drop a value out of an enum.

Also, obv this would be user error, but DEV and PROD in the same set is expressible, and meaningless/wrong, e.g. "GB,DEV,PROD". A stage is one of two(?) values, which to me casts it as an enum rather than a set.

Gating on sites with a decorator fits better I reckon, that is a whole set of routes, and I'm not sure exactly how that will be authorised at a country level yet (read:sites vs. read:sites-gb) but cross that bridge. 😁

I get that this doesn't make it super straightforward to see exactly what an environment exposes in one place, but if they are configuring different things in slightly different ways, I think keeping these configs close but separate feels better.

Rather than a single feature flag config string, I'm tempted to do kind of the inverse: keep separate, typed env vars, but make the entry point clearer, with a set function like from_env() that parses and validates these flags separately but in one place, and outputs a typed config object that holds everything specific to the active deployment (including defaults)?

But I think this would be a separate abstraction/Issue/PR as it wouldn't change the env vars, although I'll strip out the duplicate variable declaration here as it doesn't actually make sense in the same way as other config vars, and I'll make the environment names match TF to leave that coupled to local.environment for now.

[devsjc]

I hadn't considered the dynamic appearance requirements of some of the routes according to a user's allowed scopes. I guess it depends on desired behaviour - if, as you say, you want a request for a country that doesn't exist to say "hey that country doesn't exist" as opposed to just returning nothing (mentioned to "look like a DP error" in the PR, but I don't think returning nothing for a non-existant country is truly looking like an error), then it makes sense to do what you've done and have it explicit and verifiable. Also, you're right in that the flags are prone to input error. As for the parsing of flags, that already is being done by the HOCON file, it is just missing the validation as mentioned (though that is in VALID_COUNTRY_CODES etc). But from_env would be duplication of logic to the ConfigParser.

Happy to go with your solution, thanks for explaining.