Repository navigation
feat(storage): storage coordinates only from storage_module; STORAGE_* credentials are the only env - #1879
feat(storage): storage coordinates only from storage_module; STORAGE_* credentials are the only env#1879pyramation wants to merge 2 commits into
Conversation
…RAGE_* credentials are the only env
|
I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".
|
|
Review complete. 🟡 1 medium 💬 Inline comments (1)
The change centralizes object-store connection details in the database:
Reviewed commit: bcfcc60 |
There was a problem hiding this comment.
This PR moves S3 storage coordinates (endpoint/provider/region/public URL prefix) from cdn env config into storage_module rows with platform-plane inheritance, renames s3 plugin options to credentials, and drops legacy AWS_*/CDN_* env aliases in favor of STORAGE_*.
Key findings
- 🟡 AWS_ credential env aliases dropped with no fallback* — env.ts:55
| STORAGE_ACCESS_KEY_ID, | ||
| STORAGE_SECRET_ACCESS_KEY, |
There was a problem hiding this comment.
🟡 bug · medium
AWS_ credential env aliases dropped with no fallback*
getEnvVars now reads only STORAGE_ACCESS_KEY_ID / STORAGE_SECRET_ACCESS_KEY (pgpm/env/src/env.ts:55). The previous implementation explicitly merged AWS_ACCESS_KEY || AWS_ACCESS_KEY_ID and AWS_SECRET_KEY || AWS_SECRET_ACCESS_KEY into the storage credentials, so deployments authenticating via the standard AWS SDK env names silently lose object-store credentials after upgrade. The failure surfaces only as a runtime STORAGE_CREDENTIALS_MISSING error on the first upload or presigned-URL request, not at boot.
📋 Prompt for AI Agents
In pgpm/env/src/env.ts around lines 55-56 and 138-141, restore backward compatibility: destructure AWS_ACCESS_KEY, AWS_ACCESS_KEY_ID, AWS_SECRET_KEY, AWS_SECRET_ACCESS_KEY from env and populate storage.accessKeyId with STORAGE_ACCESS_KEY_ID || AWS_ACCESS_KEY_ID || AWS_ACCESS_KEY and storage.secretAccessKey with STORAGE_SECRET_ACCESS_KEY || AWS_SECRET_ACCESS_KEY || AWS_SECRET_KEY, logging a deprecation notice when only the AWS_* names are present. The prior code honored these standard aliases; dropping them silently breaks existing deployments at first upload.
|
Found 16 test failures on Blacksmith runners: Failures
...and 6 more test failures. View all on Blacksmith
|
…dpoint/provider/region for the platform credentials
Summary
Storage configuration is now 100% DB-driven. The
cdnenv model is deleted. The only storage env isSTORAGE_ACCESS_KEY_ID/STORAGE_SECRET_ACCESS_KEY.pgpm/types/pgpm/env/graphql/types:CDNOptionsandpgpmDefaults.cdnare gone, and so is parsing ofCDN_*/BUCKET_*/AWS_*/MINIO_*. They're replaced bystorage?: { accessKeyId, secretAccessKey }. Thepgpm envcommand prints only the two credentials.graphile-presigned-url-plugin:storage-module-cache(the existing LRU) selects endpoint/provider/region from the platform row,coalesce(sm.public_url_prefix, platform row), andconnection_overrides. That last one lists any coordinate a non-platform row sets on its own.resolveS3ForDatabasebuilds oneS3Clientper resolved[provider, endpoint, region], memoized. It throwsSTORAGE_CONNECTION_NOT_CONFIGUREDwhen provider or region is missing. It throwsSTORAGE_CONNECTION_OVERRIDE_REFUSEDwhen a module row names its own endpoint/provider/region, so the platformSTORAGE_*credentials never sign for a tenant-chosen host (test:physical-bucket.test.ts).{ credentials }and not{ s3 }. There's no global-S3 fallback indownloadUrlanymore.graphile-settings:getStorageCredentials()(throwsSTORAGE_CREDENTIALS_MISSING) replaces the env S3 config.upload-resolverstreams through the DB-resolved client and the bucket'sphysical_name. The process-wideBUCKET_NAMEstreamer is gone.graphql/server/src/scripts/create-bucket.ts,uploads/s3-streamer/src/s3.ts(getClient) and the unusedgraphql/explorer/src/resolvers/uploads.ts.Streamernow takes{ client, defaultBucket }.simple-seed-storage,db-scope-storage) puthttp://localhost:9000/minio/us-east-1on the storage_module row. CI env is just the two credentials.This PR's storage suites only go green once
@pgpm/metaschema-moduleswithregionis published andpgpm.jsonis bumped (step 2 below). Verified locally: withregionoverlaid inextensions/,graphql/server-testupload+db-scope-uploadpass 51/51. The presigned-url plugin passes 91/91.Single source of truth
metaschema_modules_public.storage_moduleholdsendpoint,provider,region(new column, added in place) andpublic_url_prefix. Buckets get their physical identity from each bucket row'sphysical_name.scope = 'platform',key = 'default'row.endpoint/provider/regionalways come from that platform row. Other planes may set onlypublic_url_prefix, and a row that sets its own coordinates is refused (STORAGE_CONNECTION_OVERRIDE_REFUSED). This is resolved inside the query that is already cached, so there's no extra DB read.platform_configandplatform_secretshold no storage coordinates.STORAGE_ACCESS_KEY_ID/STORAGE_SECRET_ACCESS_KEY. Each storage consumer requires them and fails fast with an error naming both. There are no aliases and no fallbacks.Merge / publish order (4 PRs)
devin/1791334000-storage-region: merge, then publish@pgpm/metaschema-modules(the next lerna release after 0.47.0). This repo is what npm publishes. constructive-db'spgpm-modules/is a vendored mirror (docs/architecture/npm-publication-sources.md).devin/1791334000-storage-db-driven: bump the rootpgpm.jsonfixture@pgpm/metaschema-modulesto that release (re-runpnpm fixtures:install). The storage suites needstorage_module.region, so they only pass after that bump. Merge, then publish thegraphile-*,@constructive-io/*andpgpmpackages.devin/1791334000-storage-db-driven(targetsmain): CI doesn't depend on any npm publish. Its Feature document check needs aconstructive-testing-seedsrelease generated from that branch, because the pinned seed predatesregion. Before deploying, bump@constructive-io/graphql-server/graphile-settingsto the step-2 releases (the published ones before that still readCDN_*). Relabel the mirror to the step-1 version.devin/1791334000-storage-db-driven: apply only together with a release promotion that pins images built from steps 2 and 3. The bootstrap writesstorage_module.region, and pods no longer getCDN_*/BUCKET_*.Link to Devin session: https://app.devin.ai/sessions/f3fd69925240491c9d7507d3b97aa5cb
Open in Devin Desktop: https://app.devin.ai/desktop/session/f3fd69925240491c9d7507d3b97aa5cb?variant=devin
Requested by: @pyramation