Scaffolding: Federation gateway setup#299
Conversation
klpoland
added
feature
klpoland
changed the title
|
Added some dedicated API endpoints, serializers, and signals for federation and configuration that defaults to disabling federation sync. |
klpoland
force-pushed
the
feature-kpoland-federation-gateway-scaffolding
branch
2 times, most recently
from
9f94dfd to
83d51d0
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
klpoland
force-pushed
the
feature-kpoland-federation-gateway-scaffolding
branch
from
83d51d0 to
4bbcbed
Compare
| # FEDERATION | ||
| # ------------------------------------------------------------------------------ | ||
| # the below environment variables are used for site federation with peers. | ||
| # By default, federation events are disabled. | ||
| # If FEDERATION_ENABLED is TRUE, deployment will need to include | ||
| # federation sync service configuration. | ||
| # FEDERATION_ENABLED=true | ||
| # FEDERATION_EVENTS_ENABLED=true | ||
| # FEDERATION_EVENTS_CHANNEL=federation:events | ||
| # FEDERATION_SYNC_USER_EMAIL= | ||
| # FEDERATION_SITE_NAME= | ||
| # FEDERATION_SYNC_HEALTH_URL=http://federation-sync:8000/sync/health | ||
| # FEDERATION_EXPORT_INTERNAL_HEADER_SECRET= | ||
| # FEDERATION_EXPORT_TRUST_X_FORWARDED_FOR=false | ||
|
|
There was a problem hiding this comment.
add a one line comment explaining each value; highlight that federation-sync is a docker service in the same network, defined in /federation
lucaspar
added
the
migrations
|
|
||
| # Create files | ||
| minio_client = MinioClient() | ||
| minio_client = get_minio_client() |
There was a problem hiding this comment.
Test command calls missing upload method
Low Severity
This commit switches the dev command to get_minio_client(), which returns ObjectStoreFacade with MinIO-compatible methods like fput_object, but the upload loop still calls upload_file. That method is not defined on the facade or delegated MinIO client, so the command fails at runtime when uploading fixtures.
Reviewed by Cursor Bugbot for commit 2c09fae. Configure here.
| user=user, | ||
| source=KeySources.FederationSync, | ||
| description="Federation sync service (export endpoints only)", | ||
| ) |
There was a problem hiding this comment.
Command always rotates sync keys
Medium Severity
Each run of create_federation_sync_api_key deletes every existing FederationSync API key for the sync user before creating a new one. Re-running the command invalidates keys already configured in federation-sync without an explicit rotate flag.
Reviewed by Cursor Bugbot for commit 3393c0f. Configure here.
| # FEDERATION | ||
| # ------------------------------------------------------------------------------ | ||
| # Peer sync uses the federation-sync Docker service (same sds-network as gateway; | ||
| # service definition lives under /federation). Bootstrap: enable federation, run | ||
| # create_federation_sync_api_key, pass the key to federation-sync. Set FEDERATION_SITE_NAME | ||
| # (e.g. crc) when enabling federation; use SDS_SITE_FQDN for the public host (RFC [site].fqdn). | ||
| # FEDERATION_ENABLED=true # Master switch for export APIs and Redis federation events. | ||
| # FEDERATION_SITE_NAME=crc # RFC [site].name (short peer id); set SDS_SITE_FQDN separately for [site].fqdn. | ||
| # FEDERATION_EVENTS_CHANNEL=federation:events # Redis pub/sub channel federation-sync subscribes to. | ||
| # FEDERATION_SYNC_HEALTH_URL=http://federation-sync:8000/sync/health # Health probe target (federation-sync service). | ||
| # FEDERATION_SYNC_USER_EMAIL=federation-sync@internal.local # Service user email for create_federation_sync_api_key. | ||
| # FEDERATION_EXPORT_ALLOWED_CIDRS= # Comma-separated CIDRs allowed to call export (default: private Docker ranges). | ||
|
|
There was a problem hiding this comment.
comments need to be in a new line for .env files; otherwise the # ... will unintentionally be part of the values set:
cat .env | grep SSH_KEY
SSH_KEY= # this is not interpreted as a comment
docker exec -it ubuntu bash -c 'env | grep SSH_KEY'
SSH_KEY=# this is not interpreted as a comment
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 4 potential issues.
There are 6 total unresolved issues (including 2 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 751d03a. Configure here.
| initialize_federation_operational_state, | ||
| ) | ||
|
|
||
| initialize_federation_operational_state() |
There was a problem hiding this comment.
Ready hook queries pre-migrate
High Severity
Calling initialize_federation_operational_state() from AppConfig.ready() runs federation checks during app load, including a UserAPIKey database query when FEDERATION_ENABLED is true. Django invokes ready() before migrations apply, so a fresh migrate with federation enabled can raise a missing-table error and abort bootstrap.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 751d03a. Configure here.
| ) | ||
|
|
||
| def _parse_cidrs(raw: list[str]) -> list[ipaddress.IPv4Network | ipaddress.IPv6Network]: | ||
| return [ipaddress.ip_network(item.strip(), strict=False) for item in raw] |
There was a problem hiding this comment.
Empty CIDR crashes startup
High Severity
_parse_cidrs parses every list entry with ipaddress.ip_network and does not drop blank tokens. An empty or malformed FEDERATION_EXPORT_ALLOWED_CIDRS value (for example a trailing comma or an empty assignment) can include "", which makes settings import raise ValueError and prevents Django from starting.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 751d03a. Configure here.
| "192.168.0.0/16", | ||
| ], | ||
| ), | ||
| ) |
There was a problem hiding this comment.
Empty CIDR list blocks export
Medium Severity
Setting FEDERATION_EXPORT_ALLOWED_CIDRS to an empty value bypasses the documented private-network defaults and yields an empty allowlist, so is_client_ip_allowed_for_federation_export always denies export clients even when federation is otherwise operational.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 751d03a. Configure here.
| item_type=ItemType.DATASET, | ||
| uuid=instance.uuid, | ||
| timestamp=instance.updated_at, | ||
| ) |
There was a problem hiding this comment.
Wrong federation event types
Medium Severity
_event_type maps any non-exportable save to deleted, including brand-new private or draft records that were never federated, so Redis can emit spurious delete notifications. When a record first becomes exportable on a later save, created is false, so the event is updated instead of created, which can confuse sync indexing.
Reviewed by Cursor Bugbot for commit 751d03a. Configure here.
2 participants
Note
Medium Risk
Introduces a new internal export surface and global API-key permission behavior (sync keys scoped to federation only); misconfigured CIDRs or leaked sync keys could expose public metadata beyond the intended Docker network.
Overview
Adds gateway-side federation scaffolding so a local
federation-syncservice can pull public dataset/capture metadata and subscribe to change notifications.Internal export API (
/api/.../federation/export/...) lists and returns federation-shaped JSON for public finalized datasets and public captures, gated byFEDERATION_ENABLED, startup/periodic operational checks (sync health, Redis,FederationSyncAPI key), CIDR allowlists, and a dedicatedFederationSyncAPI key (create_federation_sync_api_key). Export payloads use newDatasetFederationSerializer/CaptureFederationSerializer, with contract tests aligned tosds_federationPydantic models.Change propagation:
post_savesignals on datasets/captures publish created/updated/deleted events to a configurable Redis channel when federation is operational. Example env docs and settings cover site name, channel naming, health URL, and export CIDRs.Auth hardening:
APIKeyAuthenticationnow setsrequest.authto theUserAPIKeyobject;DisallowFederationSyncKeyis added to global DRF default permissions so sync keys cannot call normal asset APIs (inverse: export routes requireIsFederationSyncKey).KeySources.FederationSyncplus a users migration extend API key sources.Smaller follow-ons: app
ready()initializes federation operational state;fallow-cross-file-dupes.shprefers localfallow;create_test_filesimport paths updated.Reviewed by Cursor Bugbot for commit 751d03a. Bugbot is set up for automated code reviews on this repo. Configure here.