Skip to content

feat(docs): update the loculus docs with AWS specific configuration information - #7313

Open
anna-parker wants to merge 4 commits into
mainfrom
aws-s3-docs
Open

feat(docs): update the loculus docs with AWS specific configuration information#7313
anna-parker wants to merge 4 commits into
mainfrom
aws-s3-docs

Conversation

@anna-parker

@anna-parker anna-parker commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

resolves #4926

Additionally fixes the values.schema.json: Protocol prefix is actually required and backend will not be able to talk to S3 without it.

Screenshot

PR Checklist

  • All necessary documentation has been adapted.
  • The implemented feature is covered by appropriate, automated tests.
  • Any manual testing that has been done is documented (i.e. what exactly was tested?)

🚀 Preview: Add preview label to enable

@claude claude Bot added the deployment Code changes targetting the deployment infrastructure label Sep 11, 2026
@claude

claude Bot commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

This PR may be related to: #4926

@anna-parker anna-parker changed the title feat(docs): update the loculus docs with aws specific configuration information feat(docs): update the loculus docs with AWS specific configuration information Sep 11, 2026
@anna-parker
anna-parker requested a review from tombch September 11, 2026 16:07
@anna-parker
anna-parker marked this pull request as ready for review September 11, 2026 16:07
@claude

claude Bot commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

Claude finished @anna-parker's task in 3m 21s —— View job


Review

Docs-only change plus one schema fix. The core claim checks out: loculus.s3Url (kubernetes/loculus/templates/_urls.tpl:31) passes s3.bucket.endpoint straight through to --loculus.s3.bucket.endpoint, which lands in S3Client.endpointOverride(URI.create(...)) (backend/src/main/kotlin/org/loculus/backend/service/files/S3Service.kt:208) — a value without a scheme does fail. The path-style note is also correct (pathStyleAccessEnabled(true), line 226), and getPublicUrl builds endpoint/bucket/key, so the regional-vs-virtual-hosted warning is the right guidance.

Three inline comments, one of which I'd treat as blocking:

  • IAM policy is missing s3:ListBucket — without it AWS returns 403 rather than 404 for HeadObject on a missing key, which turns validateFilesUploaded's intended 422 into a 500. Details inline.
  • values.schema.json: suggest a ^https?:// pattern so Helm rejects a scheme-less endpoint up front, and the default value documents a case where the field is ignored.
  • kubernetes/README.md: root-absolute link won't resolve on GitHub.

Two things that could be worth adding while you're covering AWS specifics (not blocking, both outside the changed lines):

  1. Block Public Access. New AWS buckets have "Block all public access" on by default, which silently neutralises the tag-based bucket policy in "Set S3 policy" (line 192). Admins using fileSharing.outputFileUrlType: s3 will hit this; a sentence pointing at BlockPublicPolicy/RestrictPublicBuckets would save some debugging.
  2. CORS format. The existing CORS example is s3cmd-style XML; the AWS console and aws s3api put-bucket-cors both take JSON. A JSON equivalent alongside it would round out the AWS path.

The IAM section reads accurately against what the backend actually calls — GetObject/PutObject (including the multipart create/upload-part/complete flow), PutObjectTagging, DeleteObject are all there, with no unnecessary extras.


  • Read the diff
  • Verify endpoint/protocol claims against backend + Helm templates
  • Check IAM policy against actual S3 operations
  • Post inline comments and summary

No code changes made — this was a review only.
· branch aws-s3-docs

Comment on lines +99 to +105
"Statement": [
{
"Effect": "Allow",
"Action": ["s3:GetObject", "s3:PutObject", "s3:PutObjectTagging", "s3:DeleteObject"],
"Resource": "arn:aws:s3:::my-loculus-bucket/*"
}
]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This policy is missing s3:ListBucket, which the backend needs in practice.

S3Service.getFileSize (backend/.../files/S3Service.kt:234) does a HeadObject and treats a 404 as "file not uploaded yet". On AWS, if the identity has s3:GetObject but not s3:ListBucket on the bucket, a HeadObject for a nonexistent key returns 403 AccessDenied instead of 404 (AWS docs). getFileSize then rethrows, s3ErrorMapping maps the unknown code to RuntimeException("Unexpected S3 error: ..."), and FileMappingPreconditionValidator.validateFilesUploaded returns a 500 instead of the intended 422 "No file uploaded for file IDs: ...".

So an admin following this doc gets a 500 on every submission that references a file ID that was never uploaded — a normal user error.

Suggested change
"Statement": [
{
"Effect": "Allow",
"Action": ["s3:GetObject", "s3:PutObject", "s3:PutObjectTagging", "s3:DeleteObject"],
"Resource": "arn:aws:s3:::my-loculus-bucket/*"
}
]
{
"Effect": "Allow",
"Action": ["s3:GetObject", "s3:PutObject", "s3:PutObjectTagging", "s3:DeleteObject"],
"Resource": "arn:aws:s3:::my-loculus-bucket/*"
},
{
"Effect": "Allow",
"Action": "s3:ListBucket",
"Resource": "arn:aws:s3:::my-loculus-bucket"
}
]
}

Fix this →

Comment on lines +1904 to +1905
"default": "https://s3-<host value>",
"description": "The base URL of the S3-compatible service, including the protocol (e.g. https://). This is passed directly to the AWS SDK's endpoint override, which requires a scheme; a value without one will fail with 'The scheme of the endpoint override must not be null.' The backend addresses objects path-style (endpoint/bucket/key), so for AWS S3 use the regional endpoint rather than a bucket-specific virtual-hosted one, e.g. https://s3.eu-central-1.amazonaws.com for a bucket in eu-central-1. Ignored when runDevelopmentS3 is true: the dev S3 (MinIO) URL is generated automatically, scheme included."

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Two things here:

  1. Enforce the scheme in the schema. Since the whole point of this change is that a missing protocol prefix breaks the backend at runtime, a pattern would turn that into a helm validation error instead of a crash after deploy. s3.bucket.endpoint has no default in values.yaml, so the pattern only applies when a user actually sets it (dev/MinIO deployments that rely on runDevelopmentS3 are unaffected):

    "type": "string",
    "pattern": "^https?://",
  2. The default is misleading. loculus.s3Url (kubernetes/loculus/templates/_urls.tpl:23) only produces https://s3<sep><host> when runDevelopmentS3 is true and environment == "server"; otherwise it's http://<localHost>:8084. The description in the same field says the value is ignored when runDevelopmentS3 is true — so the documented "default" is exactly the case where the field does nothing. Dropping default (or changing it to something like "" / none) would be less confusing than documenting a value that never applies to this key.

Minor: this description renders into a table cell via docs/src/components/SchemaDocs.astro; at ~600 chars it will be a fairly unwieldy cell. Consider trimming to the essentials (scheme required, path-style/regional endpoint) and leaving the SDK error message detail to the prose doc.

Also, if the quoted SDK error was paraphrased rather than copied from a log, worth pasting it verbatim — AWS SDK v2 raises The URI scheme of endpointOverride must not be null., which is what an admin would actually grep for.

Comment thread kubernetes/README.md

## Configuring an S3 bucket (e.g. AWS)

See [Configuring extra file submission](/docs/src/content/docs/for-administrators/configuring-extra-files.md) for the full walkthrough (bucket config, credentials via sealed secrets, CORS, bucket policy).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Root-absolute links in GitHub-rendered markdown resolve against github.com, not the repo root, so this points at https://github.com/docs/src/... (404). A relative path works from kubernetes/README.md:

Suggested change
See [Configuring extra file submission](/docs/src/content/docs/for-administrators/configuring-extra-files.md) for the full walkthrough (bucket config, credentials via sealed secrets, CORS, bucket policy).
See [Configuring extra file submission](../docs/src/content/docs/for-administrators/configuring-extra-files.md) for the full walkthrough (bucket config, credentials via sealed secrets, CORS, bucket policy).

(There's an existing instance of the same pattern at line 55, so this isn't introduced by the PR — but worth not adding another.) Linking to the published docs page instead would also work and is arguably friendlier for readers.

@@ -130,7 +170,6 @@ You can set a permissive CORS policy on your bucket with `s3cmd setcors cors.xml
<AllowedMethod>HEAD</AllowedMethod>
<AllowedMethod>POST</AllowedMethod>

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

it also seems to work without POST - but potentially we should still add it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

deployment Code changes targetting the deployment infrastructure

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Improve s3 documentation

1 participant