feat: add S3 Configuration - BED-9174 - #3116
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThe change adds configurable local and S3 file services, passes contexts during initialization, adds storage configuration tests, and documents client artifact upload-session, multipart upload, completion, and download contracts. ChangesArtifact storage and upload API
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Entrypoint
participant FileServiceResolver
participant AWSConfig
participant LocalStore
participant S3Store
Entrypoint->>FileServiceResolver: initialize with context and configuration
FileServiceResolver->>AWSConfig: load configuration when S3 is required
FileServiceResolver->>LocalStore: create local stores
FileServiceResolver->>S3Store: create S3 stores
FileServiceResolver-->>Entrypoint: return services or validation error
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 oasdiff (1.27.0)packages/go/openapi/src/openapi.yamlError: failed to load base spec from "/tmp/coderabbit-oasdiff-base.hoam1y": encountered disallowed external reference: "./paths/eula.accept-eula.yaml" Comment |
coderabbitai
Bot
added
documentation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
cmd/api/src/services/storage/fileserviceresolver.go (1)
169-169: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a descriptive configuration parameter name.
Rename
cfgat Line 169 and Line 258 toconfiguration. This improves clarity in the configuration-resolution path.As per coding guidelines, “Prefer descriptive variable names, such as
databaseInterface, instead of abbreviated names such asdiordbi.”Also applies to: 258-258
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/api/src/services/storage/fileserviceresolver.go` at line 169, Rename the configuration parameter `cfg` to `configuration` in `resolveFileServiceDefinitions` and the additionally referenced declaration at line 258, updating all usages within their respective scopes while preserving behavior.Source: Coding guidelines
packages/go/openapi/src/schemas/model.client-artifact-upload-session.yaml (1)
19-59: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueConsider adding
artifact_typeto the session model.The create request requires
artifact_type, and the create response returnsclient_id. The session model returns neither. A client that only callsGET /api/v2/clients/management/artifacts/{artifact_id}cannot read the artifact type it declared. Addartifact_typeif the session response is meant to be a complete view of the artifact.♻️ Proposed addition
- type: object properties: storage_key: type: string + artifact_type: + $ref: './enum.artifact-type.yaml' content_type: type: string🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/go/openapi/src/schemas/model.client-artifact-upload-session.yaml` around lines 19 - 59, Update the client artifact upload session schema to include an artifact_type property, matching the type and naming used by the create request, so GET session responses expose the declared artifact type alongside the existing session fields.packages/go/openapi/doc/openapi.json (1)
17573-17599: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the two checksum encodings and the two algorithm spellings.
The upload flow now uses two different representations of the same algorithm set:
enum.checksum-algorithmusessha256andsha512in the create request and in the session response.- The
Content-Digestheader usessha-256andsha-512, as required by RFC 9530.The encodings also differ.
checksumfields are hex-encoded. TheContent-Digestvalue is base64 inside a structured field byte sequence. Do not rename the enum, because that changes the JSON contract. Instead, state the mapping in the operation description so a client author can convert correctly.Apply the change in the source file
packages/go/openapi/src/paths/clients.clients.management.artifacts.id.parts.part-number.yamland regenerateopenapi.json.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/go/openapi/doc/openapi.json` around lines 17573 - 17599, The upload-part operation description should explicitly map enum.checksum-algorithm values sha256/sha512 to Content-Digest algorithm names sha-256/sha-512, and distinguish hex-encoded checksum fields from base64 structured-field Content-Digest values. Update the operation description in the source YAML without renaming the enum, then regenerate the derived openapi.json.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/go/openapi/src/schemas/model.client-artifact-upload-session.yaml`:
- Around line 21-22: Remove the storage_key property from the client-facing
upload session, upload session part, and create-response schemas, retaining
artifact_id, part_number, and operation_id as the client contract; then
regenerate packages/go/openapi/doc/openapi.json to reflect these schema changes.
---
Nitpick comments:
In `@cmd/api/src/services/storage/fileserviceresolver.go`:
- Line 169: Rename the configuration parameter `cfg` to `configuration` in
`resolveFileServiceDefinitions` and the additionally referenced declaration at
line 258, updating all usages within their respective scopes while preserving
behavior.
In `@packages/go/openapi/doc/openapi.json`:
- Around line 17573-17599: The upload-part operation description should
explicitly map enum.checksum-algorithm values sha256/sha512 to Content-Digest
algorithm names sha-256/sha-512, and distinguish hex-encoded checksum fields
from base64 structured-field Content-Digest values. Update the operation
description in the source YAML without renaming the enum, then regenerate the
derived openapi.json.
In `@packages/go/openapi/src/schemas/model.client-artifact-upload-session.yaml`:
- Around line 19-59: Update the client artifact upload session schema to include
an artifact_type property, matching the type and naming used by the create
request, so GET session responses expose the declared artifact type alongside
the existing session fields.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 3d78e421-678c-4620-87a8-6284ca64c5cf
📒 Files selected for processing (23)
cmd/api/src/api/constant.gocmd/api/src/config/config.gocmd/api/src/config/config_test.gocmd/api/src/daemons/datapipe/datapipe_integration_test.gocmd/api/src/services/entrypoint.gocmd/api/src/services/graphify/graphify_integration_test.gocmd/api/src/services/storage/fileserviceresolver.gocmd/api/src/services/storage/fileserviceresolver_test.gopackages/go/openapi/doc/openapi.jsonpackages/go/openapi/src/openapi.yamlpackages/go/openapi/src/paths/clients.clients.id.artifacts.id.yamlpackages/go/openapi/src/paths/clients.clients.management.artifacts.id.complete.yamlpackages/go/openapi/src/paths/clients.clients.management.artifacts.id.parts.part-number.yamlpackages/go/openapi/src/paths/clients.clients.management.artifacts.id.yamlpackages/go/openapi/src/paths/clients.clients.management.artifacts.yamlpackages/go/openapi/src/schemas/enum.artifact-status.yamlpackages/go/openapi/src/schemas/enum.artifact-type.yamlpackages/go/openapi/src/schemas/enum.checksum-algorithm.yamlpackages/go/openapi/src/schemas/model.client-artifact-upload-session-complete-request.yamlpackages/go/openapi/src/schemas/model.client-artifact-upload-session-create-request.yamlpackages/go/openapi/src/schemas/model.client-artifact-upload-session-create-response.yamlpackages/go/openapi/src/schemas/model.client-artifact-upload-session-part.yamlpackages/go/openapi/src/schemas/model.client-artifact-upload-session.yaml
Comment on lines
+21
to
+22
| storage_key: | ||
| type: string |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Determine how storage keys are generated and whether client-facing code reads them.
set -euo pipefail
rg -n --type=go -C3 '\bStorageKey\b'
rg -n --iglob '*.yaml' -C2 'storage_key'Repository: SpecterOps/BloodHound
Length of output: 159
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- candidate files ---'
git ls-files | rg '(^|/)(model\.client-artifact-upload-session(-part)?\.yaml|openapi\.json)$'
printf '%s\n' '--- schema definitions ---'
for f in $(git ls-files | rg '(^|/)model\.client-artifact-upload-session(-part)?\.yaml$'); do
echo "### $f"
cat -n "$f"
done
printf '%s\n' '--- storage_key references ---'
rg -n -C3 --glob '!node_modules/**' --glob '!dist/**' --glob '!build/**' 'storage_key|StorageKey|storageKey' .
printf '%s\n' '--- upload-session references ---'
rg -n -C2 --glob '!node_modules/**' --glob '!dist/**' --glob '!build/**' 'artifact-upload-session|ArtifactUploadSession|upload session|part_number|artifact_id' packagesRepository: SpecterOps/BloodHound
Length of output: 31309
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- client artifact upload response paths ---'
for f in \
packages/go/openapi/src/paths/clients.clients.management.artifacts.yaml \
packages/go/openapi/src/paths/clients.clients.management.artifacts.id.yaml \
packages/go/openapi/src/paths/clients.clients.management.artifacts.id.parts.part-number.yaml \
packages/go/openapi/src/paths/clients.clients.management.artifacts.id.complete.yaml \
packages/go/openapi/src/schemas/model.client-artifact-upload-session-create-response.yaml; do
echo "### $f"
cat -n "$f"
done
printf '%s\n' '--- all artifact-upload implementation references ---'
rg -n -i -C3 --glob '!node_modules/**' --glob '!dist/**' --glob '!build/**' \
'artifact.?upload|upload.?session|storage[_-]?key|storagekey' .
printf '%s\n' '--- generated OpenAPI storage_key schema contexts ---'
sed -n '24985,25080p' packages/go/openapi/doc/openapi.json
printf '%s\n' '--- tracked generated/client artifacts ---'
git ls-files | rg -i 'openapi|artifact|upload|generated|client' | head -300Repository: SpecterOps/BloodHound
Length of output: 50377
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- working-tree summary ---'
git diff --stat
printf '%s\n' '--- targeted diff for upload schemas and generated contract ---'
git diff --no-ext-diff --unified=8 -- \
packages/go/openapi/src/schemas/model.client-artifact-upload-session.yaml \
packages/go/openapi/src/schemas/model.client-artifact-upload-session-part.yaml \
packages/go/openapi/src/schemas/model.client-artifact-upload-session-create-response.yaml \
packages/go/openapi/doc/openapi.json
printf '%s\n' '--- exact storage_key occurrences outside OpenAPI files ---'
rg -n --glob '!packages/go/openapi/**' --glob '!node_modules/**' \
'storage_key|StorageKey|storageKey' . || true
printf '%s\n' '--- storage-related schema and path names ---'
git ls-files packages/go/openapi/src | rg -i 'artifact|upload|storage|object' | head -120Repository: SpecterOps/BloodHound
Length of output: 1921
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- upload request schemas ---'
for f in \
packages/go/openapi/src/schemas/model.client-artifact-upload-session-create-request.yaml \
packages/go/openapi/src/schemas/model.client-artifact-upload-session-complete-request.yaml; do
echo "### $f"
cat -n "$f"
done
printf '%s\n' '--- all client-facing references to storage_key schemas ---'
rg -n -C2 'model\.client-artifact-upload-session(-create-response|-part)?|storage_key' \
packages/go/openapi/src packages/go/openapi/doc/openapi.jsonRepository: SpecterOps/BloodHound
Length of output: 13237
Remove storage_key from client-facing upload responses. Upload endpoints use artifact_id, part_number, and operation_id; clients do not send storage_key.
Remove it from the session, part, and create-response schemas. If clients require it, document that contract and its purpose. Regenerate packages/go/openapi/doc/openapi.json.
📍 Affects 2 files
packages/go/openapi/src/schemas/model.client-artifact-upload-session.yaml#L21-L22(this comment)packages/go/openapi/src/schemas/model.client-artifact-upload-session-part.yaml#L31-L32
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/go/openapi/src/schemas/model.client-artifact-upload-session.yaml`
around lines 21 - 22, Remove the storage_key property from the client-facing
upload session, upload session part, and create-response schemas, retaining
artifact_id, part_number, and operation_id as the client contract; then
regenerate packages/go/openapi/doc/openapi.json to reflect these schema changes.
mykeelium
force-pushed
the
mcuomo/BED-9174
branch
from
August 7, 2026 19:49
f362ce0 to
4eb4361
Compare
juggernot325
approved these changes
Aug 7, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Adding S3 configuration to be able to use as storage backend for FileServices.
Motivation and Context
Resolves BED-9174
Why is this change required? What problem does it solve?
How Has This Been Tested?
Please describe in detail how you tested your changes.
Include details of your testing environment, and the tests you ran to
see how your change affects other areas of the code, etc.
Screenshots (optional):
Types of changes
Checklist:
Summary by CodeRabbit