Skip to content

Add OCI Object Storage: buckets, objects, multipart, PARs - #422

Merged
arunesh-j merged 1 commit into
developmentfrom
feat/oci-objectstorage
Oct 12, 2026
Merged

arunesh-j merged 1 commit into
developmentfrom
feat/oci-objectstorage

Conversation

@arunesh-j

Copy link
Copy Markdown
Collaborator

Summary

  • Implements OCI Object Storage against the existing portable storage driver.
  • Namespaces, buckets, objects, multipart, versioning, retention rules, lifecycle policies, and Pre-Authenticated Requests.
  • Nothing added to services/storage/driver — OCI-only behaviour is a consumer-side Extras interface, per the rule set in Move OCI-only capabilities out of shared driver packages #393.

Closes #410. Part of #376.

Changes

  • providers/oci/objectstorage/ — Mock over memstore implementing driver.Bucket, guarded by a sync.RWMutex: namespace derivation, buckets, objects, versioning, multipart, retention, PARs.
  • server/oci/objectstorage/ — the /n/{namespace}/b/{bucket}/o/{object} surface plus PAR redemption at /p/{token}/n/….
  • Wiring is one line each in providers/oci/oci.go and server/oci/oci.go.

Operations

Namespace + metadata; bucket create/get/head/update/delete/list; object put/get/head/delete/list (prefix, delimiter, paging); rename and updateObjectStorageTier; async copyObject; multipart create/upload/list-parts/commit/abort/list; object versions (Disabled/Enabled/Suspended, version-addressable GET/HEAD/DELETE, delete markers, objectversions); retention rules with lock semantics; lifecycle policy PUT/GET/DELETE; PARs including redemption.

PARs are modelled as real resources

A PAR gets its own OCID, timeExpires, an opaque redemption token, and is listable and revocable — not a fabricated signature. GeneratePresignedURL creates one, so the URL it returns actually works and can be revoked. Demonstrated in the transcript: read-only PAR serves GET, refuses PUT with 403, and 404s after revocation.

Compartment scoping — one deliberate narrowing

Only ListBuckets calls RequireCompartmentID; it is the one collection real OCI scopes by compartment. Object, PAR, retention and upload lists are bucket-scoped and take no compartmentId, so requiring it would reject calls real OCI accepts. CreateBucket requires it in the body. Stated in the package doc.

Never accept-and-ignore

Rejected by name rather than silently dropped: bucket policies (→ Identity policies), CORS, object tags (→ opc-meta-), reencrypt, restoreObjects, multipart partsToExclude, cross-namespace copy, more than one lifecycle inclusionPrefix, disabling encryption, versioning back to Disabled, and unknown access types / tiers / time units.

reencrypt is a 501 rather than a 202: with no per-object key material there is nothing to re-wrap, and a 202 there would be theatre.

Provider Coverage

  • AWS
  • Azure
  • GCP
  • OCI

Checklist

  • All tests pass (go test ./...) — exit 0, 272 packages
  • Linter passes (golangci-lint run --timeout=9m) — 0 issues
  • Every provider the change applies to implements the same behavior — OCI-only, additive
  • Integration tests added to cloudemu_test.go — driver + handler tests instead
  • Unit tests added to provider test files

Test Plan

go build ./...                                              clean
go test ./...                                               exit 0, 272 packages
go test -race ./providers/oci/... ./server/oci/...          11/11 ok
golangci-lint run --timeout=9m ./providers/oci/... ./server/oci/...   0 issues
go generate ./...                                           docs/coverage committed

Coverage leak check clean: no OCI operation appears in docs/coverage/{aws,azure,gcp}/*.md, and git diff development -- services/ is empty.

End-to-end on a running server (port 4611):

GET  /n                                     -> namespace 30aa137d4542
POST /n/{ns}/b                              -> 200, ocid1.bucket.oc1.iad..., etag, Standard tier
PUT  /o/holiday/beach.txt (opc-meta-owner)  -> 200, Etag + Opc-Content-Md5 + Opc-Meta-Owner
GET  /o/holiday/beach.txt                   -> 200 "sun and sea", metadata round-trips
GET  /o?prefix=holiday/&delimiter=/         -> 200, one object
GET  /b?compartmentId=...                   -> 200, one bucket
delete object / bucket / get deleted        -> 204, 204, 404

POST /n/{ns}/b/docs/p                       -> 200, PAR with accessUri
GET  /p/{token}/n/{ns}/b/docs/o/report.txt  -> 200 "quarterly"
PUT  (same URI, read-only PAR)              -> 403
DELETE the PAR, then GET                    -> 204, then 404
POST /b/docs/actions/copyObject             -> 202 + Opc-Work-Request-Id
poll work request                           -> SUCCEEDED, COPY_OBJECT, object CREATED

Note on the suite

Other Wave 2 branches see a pre-existing failure in cmd/cloudemu TestServeOutOfProcess ("AWS endpoint never became ready") which reproduces on clean development — it spawns a child process that cannot bind a listener in a sandbox. It passed on this run; flagging it as flaky-environmental rather than related to this change.

@arunesh-j arunesh-j added the oci Oracle Cloud Infrastructure label Aug 21, 2026

@NitinKumar004 NitinKumar004 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Review notes

Real data-plane engine (per #427): N/A — OCI Object Storage is object storage, which is explicitly NOT engine-eligible under the #427 model (ComputeEngine/DatabaseEngine/CacheEngine/FunctionEngine/ContainerEngine).

Findings

High · wire-fidelity — GetNamespaceMetadata (GET /n/{ns}) is misrouted to ListBuckets when the tenancy namespace begins with 'b'
server/oci/objectstorage/handler.go:287
If a user's tenancy hashes to a namespace starting with 'b' -> every GetNamespaceMetadata call (GET /n/{ns}) returns 400 'compartmentId is required' instead of the metadata body -> S3/Swift-compat setup and any SDK flow that reads namespace metadata breaks non-deterministically for ~6% of tenancies, while the identical code works for everyone else. Fix: carry a 'hadBucketSegment' bool out of parseNamespaced instead of re-sniffing the raw path.

serveBucketCollection distinguishes /n/{ns} from /n/{ns}/b with strings.Contains(r.URL.Path, "/"+segBuckets) (segBuckets=="b"). The parsed route already discards whether a /b segment was present, so this substring heuristic is the only signal. For a namespace whose first char is 'b', the path "/n/b25a97828eed" contains the substring "/b", so the request falls through to the GET branch = listBuckets, which requires compartmentId. Reproduced with tenancy "ocid1.tenancy.oc1..probe0100" -> namespace

Medium · docs — docs/services.md not updated with OCI Object Storage (Definition-of-done item)
docs/services.md:59
If a reader consults docs/services.md to learn which OCI services cloudemu emulates -> Object Storage is invisible there even though it is fully implemented -> the service is undiscoverable via the canonical human-facing doc, and the PR fails a stated done-criterion (a future migration-drift CI check keys off this file).

docs/oci-conventions.md Definition of done requires 'Operations added to docs/services.md'. The storage section header (line 59) still reads 'AWS: S3 | Azure: Blob Storage | GCP: GCS' with no 'OCI: ObjectStorage' entry, unlike networking (line 407 'OCI: VCN ...'), OCI Monitoring (623) and Identity (670) which each have OCI subsections. Only the generated docs/coverage/* files were updated in the diff; the hand-maintained services.md was not touched.

Medium · coverage — Changed packages below the 90% pillar; lifecycle-expiry and versioning wrappers untested
providers/oci/objectstorage/retention.go:366
If EvaluateLifecycle mis-evaluates expiry (e.g. off-by-one on the ExpirationDays*24h window, or a prefix mismatch) -> a caller relying on lifecycle-expiry reporting gets wrong object lists -> the regression ships silently because no test exercises the aged-out branch. Add a FakeClock test that advances past ExpirationDays and asserts the expired name, plus a HeadObjectVersion and portable-versioning round-trip.

go test -cover: providers/oci/objectstorage 66.2%, server/oci/objectstorage 71.6% (pillar target 90%). 0%-covered flows include EvaluateLifecycle + objectExpired (retention.go:366/399 — object age-out evaluation is never asserted by any test), HeadObjectVersion (versioning.go:228), and the portable SetBucketVersioning/GetBucketVersioning wrappers (versioning.go:140/159). TestBucketLifecycleCRUD stores and reads a policy but never drives an object past its ExpirationDays to confirm EvaluateLifecy

Low · wire-fidelity — ListObjects default page size is 100, not OCI's 1000
server/oci/objectstorage/object.go:233
If a client lists a bucket of 500 objects without an explicit limit -> it receives 100 + a nextStartWith cursor and must paginate 5x -> extra round-trips and a subtle behavioral difference from real OCI. Minor and partly a shared-ocirest.DefaultLimit choice; note only.

listOptions sets MaxKeys: ocirest.Limit(r), which returns ocirest.DefaultLimit (100) when 'limit' is absent and is always >=1, so the provider's defaultListLimit (1000, matching real OCI) at object.go:15 is never reached from the wire path. Real OCI ListObjects returns up to 1000 per page by default.

Low · coverage — No oci-go-sdk SDK-compat test
server/oci/objectstorage/handler_test.go:1
If the handler's wire shape drifts from what the real SDK emits/expects (header casing, envelope fields) -> hand-rolled tests that mirror the handler's own assumptions won't catch it -> a real SDK user hits the mismatch first. Recommended, not blocking.

Convention: 'An SDK-compat test using github.com/oracle/oci-go-sdk against httptest.NewServer is the strongest evidence the handler is right. Add one where the SDK makes it practical.' All wire tests are hand-rolled httptest requests; no oracle/oci-go-sdk client is driven against the handler.

@arunesh-j
arunesh-j force-pushed the feat/oci-objectstorage branch from b85c29f to f285446 Compare September 5, 2026 19:13
@arunesh-j

Copy link
Copy Markdown
Collaborator Author

Rebased onto current development (now 58787e9a) and addressed the findings in f2854465.

High — GET /n/{ns} misrouted when the namespace starts with b

You were right about the cause and the fix. route now carries HasBucketSeg, set in parseNamespaced when a /b segment is genuinely parsed; serveBucketCollection branches on that instead of strings.Contains(r.URL.Path, "/b").

Regression test TestNamespaceMetadataWithBPrefixedNamespace uses your exact repro tenancy ocid1.tenancy.oc1..probe0100 → namespace b25a97828eed. Reverting the fix:

--- FAIL: TestNamespaceMetadataWithBPrefixedNamespace
    expected: 200   actual: 400
    {"code":"InvalidParameter","message":"compartmentId is required"}

Confirmed live on the built binary, including that the collection is not over-corrected:

GET /n                  -> "b25a97828eed"
GET /n/b25a97828eed     -> 200 {"namespace":"b25a97828eed","defaultS3CompartmentId":...}
GET /n/b25a97828eed/b   -> 400 compartmentId is required   (still required, as it should be)

Medium — docs/services.md

Added an OCI Object Storage subsection under §1 Storage in the VCN/Monitoring/Identity format: a 36-row operation→route table plus prose on compartment-scoped ListBuckets, the 1000 page size, async copyObject, the versioning tri-state, retention locking, the storage-engine seam, and the two unemulated actions.

Medium — coverage

66.2% → 93.9% provider, 71.6% → 94.9% wire. Every gap you named is covered: EvaluateLifecycle/objectExpired via a config.FakeClock test that steps to one hour short of the window (asserts nothing expires) and then past it (asserts exactly the enabled logs/ names), plus HeadObjectVersion and a portable SetBucketVersioning/GetBucketVersioning round-trip.

Low — ListObjects page size

listLimit returns 0 when no limit is present, letting the provider's defaultListLimit (1000) apply. ocirest.DefaultLimit is untouched for other services. Live: 120 objects with no limit → all 120 and an empty cursor; limit=50 → 50 plus a cursor.

Low — SDK-compat test: not added, deliberately

oracle/oci-go-sdk/v65 requires a signed request built from an RSA keypair and a ConfigurationProvider. This emulator verifies no signature, so the client would mostly exercise its own signing rather than the handler's wire shape, and every sibling OCI service here is tested with hand-rolled httptest requests. I judged the dependency disproportionate. Instead I added contrib/realengine/blobstore/oci_test.go — a real over-HTTP E2E through httptest.NewServer plus the filesystem engine — with that reasoning recorded in its doc comment. Happy to revisit if you'd rather have the SDK dependency.

On the "engines N/A" note — I think Object Storage is engine-eligible

Your N/A referred to #427's compute-shaped engines, but services/storage/storageengine is a different seam, and its own package doc named AWS/Azure/GCP with OCI the only storage provider not wired to it. Now wired in providers/oci/objectstorage/engine.go across put, get, delete, rename, copy, multipart-commit and every versioned path, keyed by version id like S3.

Two design notes: the engine call happens under the write lock (S3 nils the byte slice after releasing it, which races a concurrent reader), and objectData/objectVersion gained a Size field so Head/List/approximateSize stay correct once bytes are offloaded.

Three defects found that were not in the review

  1. Object Storage metrics were silently dropped. The provider publishes to oci_objectstorage; providers/oci/monitoring rejected any oci_-prefixed namespace as Oracle-reserved, and emitMetric discards its error — so no metric was ever recorded. The reservation exists to stop customer posts, so it now applies to PostMetricData (customer-facing) and not to PutMetricData (the seam Oracle's own emulated services use). This touches an already-merged service, so say the word and I'll split it into its own PR.
  2. HeadObject returned no Content-Length and Content-Type: application/json (inherited from the JSON helper). A HEAD has no body, so SDKs read the size from the header. Now served by a dedicated writeHead.
  3. Persistence guard. Upstream Evolve persist/ to per-driver, identity-preserving snapshotting (full-surface) #582 added TestSnapshotCompleteness, which failed the suite for this provider. Added snapshot.go plus round-trip tests covering buckets, objects, version chains, PARs with their redemption tokens, retention rules and lifecycle — so an access URI issued before a snapshot still redeems after restore.

One scope addition for your call

The tenancy was unreachable from the CLI (AWS account-id, Azure subscription and GCP project all are), so the b-prefixed namespace could not be driven through the shipped binary at all. Added --oci-tenancy (server/serveflags, its pinned flag-name guard, and docs/standalone-server.md). Cleanly separable if you'd rather keep this PR narrow.

Verification

go build ./...                                   clean
go test ./...                                    exit 0, 342 packages
go test -race ./providers/oci/... ./server/oci/...   11/11 ok
golangci-lint (all changed packages)             0 issues
go test -cover  provider 93.9%   wire 94.9%
go generate ./...                                docs/coverage committed
no OCI op in docs/coverage/{aws,azure,gcp}/*.md  confirmed

The only remaining lint in ./providers/oci/... is 7 pre-existing gocritic findings in vcn/nsg.go and identity/portable.go, which reproduce on clean development.

E2E on port 4611 also exercised: bucket CRUD and the versioning:"On" rejection, object put/get/head/list with prefix+delimiter, version list / read-by-version / delete markers, rename, async copy (202 + opc-work-request-id), storage tier, both 501s, multipart including the partsToExclude 501, PARs (redeem, write-refused, wrong-object refused, list, delete), retention (create/get/update, MONTHS refused, object held, delete) and lifecycle (YEARS→365 days, unknown action refused, multi-prefix refused). Happy to paste the full transcript if useful.

@NitinKumar004 NitinKumar004 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Review — OCI Object Storage

Verdict: Approve (no blockers). This is the largest and highest-risk surface here (object bytes + multipart + PARs), and it's exemplary — it reuses the portable storagedriver.Bucket interface exactly as S3/GCS/Azure do, stores object bytes in-memory the way S3 does (no external engine required; optional engine offload), and follows the existing ocirest + server/oci/* conventions precisely.

Non-blocking

  • Enabling versioning on a bucket that already has objects drops the pre-existing object — providers/oci/objectstorage/versioning.go:18-41 (setVersioningLocked/storeObjectLocked). Reproduced: create unversioned bucket → PUT object → enable versioning → PUT overwrite → objectversions lists only the new version; the original is neither a null version nor recoverable. Real OCI retains the pre-existing object as the null version and preserves it on the next overwrite. The common IaC path (versioning = "Enabled" declared at bucket create) works perfectly — this only bites a create-then-enable sequence.
  • Object ETag is content-addressed (sha256 of bytes) — providers/oci/objectstorage/objectstorage.go:327 (objectETag). Real OCI mints a fresh opaque ETag per PUT; here an identical re-PUT yields the same ETag, and two distinct objects with identical content share one. A client using If-Match/If-None-Match on ETag could see a stale-looking match after an identical re-upload.
  • Generic OCI error codes from the shared codec — server/wire/ocirest/ocirest.go:75-98 (statusFor). Bucket-not-empty → 409 IncorrectState and duplicate create → 409 Conflict, where real OCI uses BucketNotEmpty / BucketAlreadyExists. Pre-existing shared behavior (not introduced here); noting because it now affects object-storage fidelity for a client branching on the exact code string.
  • CompleteMultipartUpload assembles the whole object via repeated append under the global write lock — providers/oci/objectstorage/multipart.go:155-164. A multi-GB commit does a large memcpy while holding m.mu, briefly blocking unrelated buckets. Bounded by uploaded data and consistent with the cohesive-mock single-mutex design; acceptable.

Verified good

Full lifecycle over the wire: get-namespace → create bucket → PUT object bytes → GET/HEAD bytes match with stable ETag/opc-content-md5 across GET/HEAD/List → multipart create/upload-parts/commit assembles in ascending part order → abort invalidates the upload → PAR create + redeem returns bytes, read-only PAR write → 403, GET PAR omits accessUri → versioning enable + overwrite lists versions with delete markers (newest-first, isLatest) → delete → 204 → GET-after-delete → 404 → delete non-empty bucket → 409. Computed IDs/namespace/etag/timeCreated stable across reads; Snapshottable present and auto-discovered (persistence round-trips objects + versions + PAR tokens); object size / part count / list page size all bounded; single-mutex spans check-then-set on every create (no TOCTOU race); reads deep-copy; -race clean; structure/lint/go generate and the full CI matrix all green.

@NitinKumar004 NitinKumar004 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This PR no longer merges with development, and against a running serve I hit a routing collision, PAR URLs that break after a restore, and lifecycle and conditional-request gaps that a real client would notice.

Earlier review points

  • GET /n/{ns} misrouted for a namespace starting with b: fixed. HasBucketSeg is in place and TestNamespaceMetadataWithBPrefixedNamespace passes.
  • docs/services.md: fixed.
  • Coverage: fixed. It is now 96.0% (provider) and 95.5% (wire).
  • ListObjects default page size: fixed. It is 1000 when no limit is given.
  • oci-go-sdk compat test: not added. The author explained why. That is fine with me, but see the Terraform note under Checked.
  • Enabling versioning after objects exist drops the original: still open. Details are under Medium.
  • Content-addressed object ETag: still open. objectETag is still sha256 of the bytes.
  • Generic error codes (Conflict / IncorrectState instead of BucketAlreadyExists / BucketNotEmpty): still open.
  • Multipart commit runs under the global lock: unchanged, and acceptable as noted before.

High

1. Does not merge with current development

  • Conflicts are in contrib/realengine/blobstore/blobstore.go, where development reworded the package doc, and in docs/coverage/README.md, which is generated.
  • After resolving them, go run ./internal/coveragegen still changes docs/coverage/README.md, because the storage row has no OCI link. It also changes docs/coverage/oci/objectstorage.md.
  • Fix: rebase on development, rerun go generate ./... and commit the output.
  • On the merged tree, go build ./... and go vet are clean. Tests pass for providers/oci, server/oci, persist (TestSnapshotCompleteness), internal/coveragegen, serveflags, serverkit and seed. -race passes on providers/oci and server/oci.

2. The work request handler takes over Object Storage paths (server/oci/oci.go:86, server/oci/workrequest/workrequest.go:200)

  • The workrequest handler is registered first. It claims any GET whose path has a workRequests segment followed by at most two more segments. Object keys and bucket names are user data, so real objects become unreachable.
  • Repro against serve:
    • PUT /n/{ns}/b/tfb/o/workRequests/x returns 200. GET on the same path returns 404 {"code":"NotAuthorizedOrNotFound","message":"work request x not found"}.
    • Create a bucket named workRequests, then GET /n/{ns}/b/workRequests returns 400 compartmentId is required.
  • Fix: have the workrequest handler ignore /n/... and /p/... paths. Or register Object Storage before it, since its Matches is anchored at /n and /p/{token}/n. Add a test with a key and a bucket named workRequests.

Medium

3. PAR tokens collide after a persist restart (providers/oci/objectstorage/par.go:123)

  • The redemption token comes from idgen.GenerateID(""), which is the process counter. On restart that counter starts again from 1 while restored PARs keep their tokens.
  • Repro:
    1. Start serve --providers oci --persist --persist-strategy on-shutdown --state-file s.json.
    2. Create a PAR on bucket secret. Its access URI is /p/00000007/....
    3. Restart, then create a PAR on bucket public. It also gets /p/00000007/... and the same PAR OCID.
    4. Now GET /p/00000007/n/{ns}/b/secret/o/payroll.csv returns 403 on every attempt.
  • This breaks the claim that an access URI issued before a snapshot still redeems after restore.
  • Fix: mint the token from crypto/rand. idgen already has random helpers. Also check for collisions in ResolvePAR, or key PARs by token.

4. Enabling versioning after objects exist loses the original object (providers/oci/objectstorage/versioning.go:18)

  • Repro: PUT k = "original", then UpdateBucket {"versioning":"Enabled"}, then PUT k = "second". GET /objectversions lists one version, and "original" is gone.
  • setVersioningLocked only allocates the history map. It never seeds the existing objects as versions.
  • Fix: when versioning moves to Enabled, add each current object to its chain before the first overwrite.

5. Lifecycle rules do not round-trip (server/oci/objectstorage/lifecycle.go:141, :129, types.go:221)

  • PUT {"timeAmount":1,"timeUnit":"YEARS"} reads back as timeAmount:365,timeUnit:"DAYS". Any client that compares the policy it set with what it reads back will see a diff.
  • target and objectNameFilter.inclusionPatterns / exclusionPatterns are silently dropped. Repro: PUT a DELETE rule with target:"previous-object-versions" and inclusionPatterns:["*.log"]. It is accepted with 200, and GET returns a plain DELETE rule with no target and no filter, so the rule would now act on live objects.
  • The PR says nothing is accepted and then ignored, but that is not the case here.
  • Fix: store the unit, target and all three filter lists as sent. Reject ABORT unless the target is multipart-uploads. Return the policy's timeCreated.

6. Conditional and range headers are ignored (server/oci/objectstorage/object.go:56, :94)

  • PUT with if-none-match: * on an existing object returns 200 and overwrites it. PUT with if-match: bogus also returns 200 and overwrites. Real OCI answers 412 IfNoneMatchFailed / IfMatchFailed.
  • GET with Range: bytes=0-2 returns 200 with the full body. Real OCI returns 206 with Content-Range. Ranged and parallel downloads (CLI, rclone, SDK range reads) get the wrong bytes.
  • Fix: honour if-match and if-none-match on Put, Get, Head and Delete object and on UpdateBucket/DeleteBucket. Also support a single byte range on GetObject.

7. UpdateBucket silently ignores a rename (server/oci/objectstorage/types.go:28)

  • UpdateBucketDetails.name renames the bucket in OCI. Here POST /n/{ns}/b/tfb {"name":"tfb2"} returns 200 with name:"tfb", and GET /b/tfb2 returns 404.
  • Fix: support the rename and move the bucket's stores under the new key. If that is out of scope, reject a name that differs from the current one with 400.

8. Compartment existence is not checked (server/oci/objectstorage/bucket.go:36, providers/oci/objectstorage/bucket.go:112, :195)

  • CreateBucket with compartmentId: "ocid1.compartment.oc1..doesnotexist" returns 200.
  • Development now rejects that for VCN through SetCompartmentChecker (#1295). Real OCI answers 404 NotAuthorizedOrNotFound.
  • Fix: wire the same identity-backed checker in server/oci/oci.go for create and for the compartmentId move on update.

9. PAR rules are stricter than OCI (providers/oci/objectstorage/par.go:16, :144, :96)

  • A timeExpires three months out is rejected with exceeds the maximum lifetime of 168h0m0s. Seven days is the S3 presign limit. OCI PARs commonly live for months or years, for example a Terraform time_expires a year out.
  • AnyObjectRead with objectName:"logs/" is rejected. OCI treats objectName on the AnyObject types as a prefix.
  • Fix: drop the cap or raise it far enough not to reject realistic values, and support prefix-scoped bucket PARs in PARAllows.

Low

  • Error codes (server/wire/ocirest/ocirest.go:75): a duplicate bucket returns 409 Conflict and a non-empty bucket returns 409 IncorrectState. OCI uses BucketAlreadyExists and BucketNotEmpty. Map these in the Object Storage handler rather than in the shared codec.
  • ETag is a content hash (providers/oci/objectstorage/objectstorage.go:327): two identical PUTs return the same ETag. Once item 6 lands, if-match will treat a re-upload as unchanged. Mint an opaque ETag per write.
  • Pagination is missing: ListBuckets, ListObjectVersions, ListPreauthenticatedRequests and ListMultipartUploads ignore limit and page and never set opc-next-page. objectversions?limit=1 returned all 3 versions.
  • copyObject drops fields (server/oci/objectstorage/object.go:323): destinationRegion is required by OCI but not checked. eu-frankfurt-1 was copied locally with 202. destinationObjectMetadata, destinationObjectStorageTier and the if-match fields are dropped. Reject a foreign region and apply or reject the other fields.
  • Multipart commit does not check ETags (server/oci/objectstorage/multipart.go:131): partsToCommit with "etag":"wrong-etag" commits successfully. Compare each part's ETag.
  • Bucket names are not validated: "bad name!" is accepted. OCI allows letters, digits, -, _ and ., up to 256 characters.

Checked

  • Real-user flow on serve --providers oci over raw HTTP:
    • Bucket create, then GET twice (byte-identical), update, list and delete.
    • Object put, get and head (Content-Length and type correct).
    • Version list and delete markers.
    • Async copy returns 202, and /workRequests/{id} reports SUCCEEDED.
    • Multipart create, upload and commit.
    • PAR redeem works, write through a read PAR returns 403, and redeem after revoke returns 404.
    • Wrong namespace returns 404, and deleting a missing object returns 404.
  • Terraform with oracle/oci was not run. The provider has no per-service endpoint override I know of, so it cannot reach a local port without DNS and TLS interception.
  • Wiring: provider factory, DriversFrom, monitoring wiring, resource discovery and the seed target all pick up Object Storage.
  • Snapshot: Snapshottable is discovered by the persist completeness test.
  • Architecture: behaviour lives in the provider and the wire layer delegates, so the library and serve behave the same.
  • Lint and format: golangci-lint is clean on the new packages, and gofmt is clean.
  • --enforce-auth: there is no OCI authorization gate on development, so there is nothing to declare for this handler.

@NitinKumar004

Copy link
Copy Markdown
Collaborator

Please merge the latest development into this branch (a merge is fine, no need to rebase). It is well behind now, and docs/coverage/README.md conflicts. After merging, run go generate ./... and commit the regenerated docs/coverage, then push. I'll re-review once it's up to date with the findings above addressed.

@arunesh-j
arunesh-j force-pushed the feat/oci-objectstorage branch from f285446 to 329035e Compare October 5, 2026 19:49
@arunesh-j

Copy link
Copy Markdown
Collaborator Author

Rebased onto 770204c3 and addressed every finding: the High, all Mediums and all Lows, with nothing deferred. The work is in a29e8483, plus 329035ec, which aligns the monitoring change with #421 (explained at the end).

High

1. Rebase. blobstore.go now uses development's "(no Docker)" wording with the OCI mention re-applied. docs/coverage/README.md is regenerated, so the storage row carries the OCI link and docs/coverage/oci/objectstorage.md is updated.

2. Work requests taking over Object Storage paths. Fixed at the root in server/oci/workrequest, because the matcher is shared by every OCI service. workRequests now matches only directly after a YYYYMMDD version segment, or as the first segment. Neither form can match a /n/... or /p/... path.

One deviation from a pure version-anchored rule: I kept the unversioned /workRequests/{id} form, because that is where Object Storage's own GetWorkRequest lives. Matching only /{date}/workRequests would have broken polling for a copy's work request.

TestWorkRequestsNamedObjectAndBucketStayReachable runs through the full server. On the old matcher it fails with your 400; it now passes, with both poll forms returning SUCCEEDED. Every server/oci/... test passes.

Medium

3. PAR tokens after a persist restart. Tokens are now 48 bytes from crypto/rand, base64url, with a collision check. The PAR OCID is also collision-checked, because it collided too. TestPARsDoNotCollideAfterRestore resets the id counter and replays the same sequence; without the fix it reproduces the duplicate OCID and token, with the old token resolving to the public bucket.

Worth flagging beyond this PR: every OCI service mints OCIDs from that process counter, and persist never advances it. That is the underlying cause, and it can affect any OCI service across a restore. I've fixed the PAR case here, but the general fix belongs in a separate change.

4. Versioning after objects exist. Turning versioning on now records every existing object as a version, moving its engine bytes to the versioned key. Covered for all three ways of enabling it, with and without an engine.

5. Lifecycle round-trip. The provider stores the OCI policy itself rather than the portable shape: unit, target, all three filter lists and timeCreated, all as sent. ABORT requires the multipart-uploads target. Only rules aimed at objects select live objects, with prefixes and patterns applied. A rule the portable shape cannot express is reported as Unimplemented on the portable read path rather than being flattened.

6. Conditional and Range headers. Preconditions are honoured everywhere you listed, plus both sides of copyObject. A failed write or delete precondition returns 412 IfMatchFailed/IfNoneMatchFailed. A single byte range returns 206 with Content-Range, an unsatisfiable range 416, and a multi-range request a 400 rather than the full body.

One deviation: a matching if-none-match on GET/HEAD returns 304, not 412, as HTTP and OCI's GetObject define it.

7. UpdateBucket rename. Implemented: objects, versions, PARs and engine bytes move to the new name. A taken name returns BucketAlreadyExists, and a different namespace is refused rather than ignored.

8. Compartment check. Mirrors development's VCN pattern exactly, wired from identity.Compartments in server/oci/oci.go, on create and on a compartmentId move.

9. PAR rules. The week cap is removed, so a PAR a year out is accepted. objectName on the AnyObject types is a prefix. timeExpires is now required, as in OCI; it used to be silently defaulted, and only the portable presign wrapper keeps a default.

Lows (all done)

  • BucketAlreadyExists / BucketNotEmpty mapped in the Object Storage handler, not the shared codec.
  • ETags are an opaque UUID per write, so if-match no longer treats a re-upload as unchanged.
  • limit/page and opc-next-page work on ListBuckets, ListObjectVersions, ListPARs, ListMultipartUploads and ListParts.
  • copyObject requires destinationRegion and rejects a foreign one; source version, both ETag preconditions, metadata and storage tier are all applied.
  • Multipart commit requires and compares each part's etag.
  • Bucket names are validated.

Aligning with #421 (329035ec)

This branch and #421 both touched providers/oci/monitoring, with different diffs, so whichever merged second would have conflicted. I took #421's version byte-for-byte: monitoring.go is back to development's version, and the only change is in metrics.go.

This also means PutMetricData again refuses oci_ namespaces from portable callers, which the #421 reviewer flagged as too permissive. Object Storage now emits through the dedicated PostServiceMetricData path, into the bucket's own compartment, keyed by resourceId. TestMetricsLandInTheBucketCompartment covers it and fails if the bucket-compartment lookup is removed. The existing TestMetricsEmission also still guards the original silent-drop bug, since an emitter going through PutMetricData would now record nothing.

Verification

go build ./...                                       clean
go test ./... (excluding cmd/cloudemu)               exit 0, 524 packages
go test -race providers/oci + server/oci             clean
golangci-lint (objectstorage x2, workrequest,
               server/oci, monitoring)               0 issues
coverage                                             provider 94.4%, wire 95.6%
go generate ./...                                    committed; idempotent
git diff origin/development -- cmd/                  empty

cmd/cloudemu was excluded locally only because parallel worktree suites collide on the shared ~/.cloudemu daemon; this branch touches nothing under cmd/, and CI runs it in isolation.

E2E against serve on :4611

PUT  /n/{ns}/b/tfb/o/workRequests/x      -> 200
GET  same                                -> "payload"
create bucket "workRequests"             -> 200
GET  /n/{ns}/b/workRequests              -> 200
copy work request, both poll forms       -> SUCCEEDED / SUCCEEDED
objectversions after PUT, enable, PUT    -> 2 items
PUT if-none-match: *                     -> 412
PUT if-match: bogus                      -> 412
GET Range: bytes=0-2                     -> 206 "hel"
lifecycle read-back                      -> target previous-object-versions, 1 YEARS, patterns ['*.log']
create into a nonexistent compartment    -> 404

For the PAR collision, serve --persist --persist-strategy on-shutdown was stopped with SIGINT and restarted from the same state file. Afterwards the secret PAR kept its token and still redeemed salaries, while a new public PAR got a different token and redeemed hello.

@NitinKumar004 NitinKumar004 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Every finding from the last round is fixed, and I confirmed each one against serve with the real oci-go-sdk (v65.126.1). Two new problems turned up in the same areas. Restored versions and retention rules can still be overwritten after a restart, and copy work requests report a status the Object Storage SDK does not define.

Earlier findings

  • High 1, merge with development: fixed. The branch is based on the current tip of development (770204c), so it merges cleanly. go run ./internal/coveragegen leaves no diff in docs/coverage.
  • High 2, work requests taking over Object Storage paths: fixed. PUT and GET of tfb/o/workRequests/x return payload. A bucket named workRequests can be created, read, written to and listed. Copy work requests still answer on /workRequests/{id} through GetWorkRequest, and ListWorkRequests works too.
  • Medium 3, PAR tokens after a persist restart: fixed. I ran the original repro: start with --persist --persist-strategy on-shutdown, create a PAR on secret, send SIGINT, restart, then create a PAR on public. The two PARs got different OCIDs and different tokens, and the secret URI still returns salaries.
  • Medium 4, enabling versioning after objects exist: fixed. The sequence PUT original, enable versioning, PUT second now lists 2 versions, and both bodies come back by versionId. Suspending, writing, re-enabling and writing again keeps all 5 versions.
  • Medium 5, lifecycle round-trip: fixed. A rule set as 1 YEARS with target previous-object-versions and all three filter lists reads back exactly as sent, and timeCreated is set. ABORT on the objects target returns 400.
  • Medium 6, conditional and range headers: fixed. PUT with if-none-match: * returns 412 IfNoneMatchFailed. A bogus if-match returns 412 IfMatchFailed on PUT, GET, DELETE and UpdateBucket. A matching if-none-match on GET and HEAD returns 304. Ranges bytes=0-2, bytes=-3 and bytes=6- return 206 with the right bytes and Content-Range. bytes=50-60 returns 416.
  • Medium 7, UpdateBucket rename: fixed. rb renamed to rb2 keeps its objects, the old name returns 404, and renaming onto a taken name returns 409 BucketAlreadyExists.
  • Medium 8, compartment check: fixed. CreateBucket and a compartment move both return 404 NotAuthorizedOrNotFound for a compartment that does not exist.
  • Medium 9, PAR rules: fixed. A PAR that expires a year out is accepted. With objectName: "logs/" on AnyObjectRead, logs/a.txt redeems and other/b.txt returns 403. A missing timeExpires returns 400.
  • Low, error codes: fixed. Duplicate bucket returns 409 BucketAlreadyExists. A non-empty delete returns 409 BucketNotEmpty.
  • Low, ETag: fixed. Two identical PUTs now return different ETags.
  • Low, pagination: fixed. objectversions?limit=1 returns 1 item plus opc-next-page, and page 2 returns a different version. Paging ListBuckets with limit=2 returns the same total as one unpaged call.
  • Low, copyObject: fixed. destinationRegion: eu-frankfurt-1 returns 400.
  • Low, multipart ETags: fixed. wrong-etag is rejected with 400, and the real part ETag commits.
  • Low, bucket names: fixed. bad name! returns 400.

New findings

Medium 1. Version ids and retention rule OCIDs still collide after a persist restore (providers/oci/objectstorage/versioning.go:15, providers/oci/objectstorage/retention.go:82)

This is the same root cause as the PAR token issue. The PAR fix did not cover the other ids this service mints. newVersionID and the retention rule OCID both come from the process counter, which starts again from 1 on restart.

Repro for versions:

  1. Start serve --providers oci --persist --persist-strategy on-shutdown.
  2. Create bucket vb with versioning Enabled and PUT k six times.
  3. Send SIGINT and restart, then PUT k twelve more times.
  4. GET /objectversions returns 18 items but only 13 distinct ids. 0000000f, 0000000d, 0000000b, 00000009 and 00000007 each appear twice.
  5. GET /o/k?versionId=0000000f returns before-6. The newer version with the same id cannot be read, and a delete by that versionId is ambiguous. With a storage engine wired, engineRef(bucket, key, version) is the same for both versions, so the new write replaces the old version's bytes.

Repro for retention rules:

  1. Before the restart, create three 365-day rules on rb. They get the OCIDs ...05, ...07 and ...09.
  2. After the restart, create six 1-day rules.
  3. The list now shows six rules, all 1-day. The new rules reused ...05, ...07 and ...09, and bkt.retention.Set(rule.ID, rule) silently replaced the original 365-day rules. A rule with timeRuleLocked set can be replaced the same way.

Fix: mint version ids with idgen.UUID(), since real OCI version ids are opaque UUIDs. Give retention rule OCIDs a random suffix as well, or refuse a Set that would replace an existing id. Bucket OCIDs collide the same way: after the restart a new bucket got ...12, the same OCID as vb. You already called that out as the general OCI counter problem, so a tracked follow-up is fine for buckets. Versions and retention rules lose data here, so they should be fixed in this PR. Add a restore test for both, like TestPARsDoNotCollideAfterRestore.

Medium 2. Copy work requests report SUCCEEDED, which is not an Object Storage status (server/oci/objectstorage/object.go:482, server/oci/workrequest/workrequest.go:93)

The Object Storage WorkRequestStatusEnum is ACCEPTED, IN_PROGRESS, FAILED, COMPLETED, CANCELING and CANCELED. The shared store always writes SUCCEEDED, which is the right value for VCN and Identity but not for Object Storage.

Repro: run CopyObject, then GetWorkRequest on the returned opc-work-request-id. The status is SUCCEEDED, and objectstorage.GetMappingWorkRequestStatusEnum("SUCCEEDED") returns false. A copy waiter that polls for COMPLETED never finishes. That includes the Terraform object copy and oci os object copy --wait-for-state COMPLETED.

Fix: let Accept take the terminal status, or add an Object Storage variant, and record COMPLETED for COPY_OBJECT. Add an SDK-shaped test that checks the status against the Object Storage enum.

Checked

  • The branch merges cleanly with origin/development, and coveragegen leaves no diff.
  • go build ./... is clean. go vet is clean on providers/oci, server/oci, services/storage, serveflags, persist and internal/coveragegen.
  • Tests pass on those packages and on contrib/realengine/blobstore. -race passes on providers/oci and server/oci.
  • golangci-lint --new-from-rev=origin/development reports 0 issues.
  • The metrics.go hunks match #421 line for line, so whichever PR merges second will not conflict there.
  • Every earlier repro was run against serve with oci-go-sdk v65, including a real SIGINT restart for the persist cases.
  • Terraform was not run, for the same reason as before. The oracle/oci provider has no per-service endpoint override.

@arunesh-j

Copy link
Copy Markdown
Collaborator Author

Thanks. Both Mediums are fixed in the latest commit, rebased on current development:

1. Version ids and retention rule OCIDs after restore

  • newVersionID now uses idgen.UUID(), which matches real OCI's opaque version ids. Multipart upload ids use it too, for the same reason.
  • CreateRetentionRule keeps minting until the OCID is free in the bucket, so it can never replace a restored rule, locked or not.
  • New TestVersionsAndRetentionRulesDoNotCollideAfterRestore replays your repro: 6 PUTs and 3 365-day rules, then snapshot, idgen.Reset() and restore, then 12 more PUTs and 6 1-day rules. It asserts 18 distinct version ids and that all 3 original rules are still there (9 rules in total). With the fix reverted it fails with 12 distinct ids out of 18.
  • Bucket OCIDs: the shared counter is now handled on development by fix(persist): keep the shared id counter across restore #1529. Persist records idgen.Counter() and calls idgen.AdvanceTo on restore, and this branch is rebased on it, so a real --persist restart no longer reuses restored OCIDs. The UUID ids and the rule guard still protect the provider-level Restore path, where the counter isn't persisted.

2. Copy work request status

  • workrequest.Store has a new AcceptAs(status, …), and Accept delegates to it with SUCCEEDED, so VCN and Identity are unchanged. CopyObject records StatusCompleted ("COMPLETED").
  • The copy poll test in server/oci/workrequest_collision_test.go now decodes the status and checks it is COMPLETED and in Object Storage's WorkRequestStatusEnum set, on both the unversioned and /20160918 poll paths.

Local: providers/oci/..., server/oci/..., persist/..., internal/coveragegen/..., services/storage/... and contrib/realengine/blobstore pass. -race passes on the objectstorage and workrequest packages. golangci-lint --new-from-rev=origin/development reports 0 issues, and go generate leaves no diff.

@NitinKumar004 NitinKumar004 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Both open items from the last round are fixed, and I confirmed them with a real --persist restart. This time I drove the whole service with the real oci-go-sdk (v65.126.1) and found one new break. UpdateRetentionRule is served on the wrong HTTP method, so the SDK, the CLI and a Terraform retention rule change all get 405. There are also a few smaller fidelity gaps around PAR bucket listing and MD5 headers.

Earlier findings

Finding Status Evidence
Medium 1: version ids and retention rule OCIDs collide after a persist restore Fixed serve --persist --persist-strategy on-shutdown, 6 PUTs to a versioned key and three 365-day rules, SIGINT, restart, then 12 more PUTs and six 1-day rules. objectversions returns 18 items with 18 distinct UUID ids. The rules list has 9 entries, and all three 365-day rules are still there. A new bucket created after the restart got a fresh OCID (...3a), not one held by vb (...04) or rb (...0c), so the counter restore from development covers buckets too. Reading the oldest version by id after the restart returns before-1.
Medium 2: copy work requests report SUCCEEDED Fixed CopyObject followed by GetWorkRequest through the SDK returns COMPLETED, and objectstorage.GetMappingWorkRequestStatusEnum accepts it. Both /workRequests/{id} and /20160918/workRequests/{id} return COMPLETED. VCN and Identity still go through Accept and keep SUCCEEDED.
All earlier rounds (routing, PAR tokens, versioning seed, lifecycle round-trip, conditionals and ranges, rename, compartment check, PAR rules, error codes, ETags, pagination, copy validation, multipart ETags, bucket names) Still fixed Re-checked through the SDK: if-match 412 on UpdateBucket, Range: bytes=1-3 returns ell with Content-Range: bytes 1-3/5, the lifecycle policy round-trips, a PAR a year out is accepted, and a wrong part number returns 400.

High

1. UpdateRetentionRule is served on POST, but the API (and the SDK, CLI and Terraform) use PUT (server/oci/objectstorage/retention.go:29)

  • PUT /n/{ns}/b/{bucket}/retentionRules/{id} returns 405 MethodNotAllowed. Repro with oci-go-sdk v65: client.UpdateRetentionRule(...) fails with Http Status Code: 405 ... PUT is not allowed on /n/.../retentionRules/ocid1.retentionrule....
  • The SDK sends PUT for this operation (objectstorage_client.go, http.MethodPut, "/n/{namespaceName}/b/{bucketName}/retentionRules/{retentionRuleId}"), and the PR's own docs/services.md table also says PUT.
  • Who it breaks: oci os retention-rule update and Terraform. Changing a retention_rules { duration {...} } block on oci_objectstorage_bucket goes through updateRetentionRulesHelper → UpdateRetentionRule, so the apply fails. Locking a rule later by setting time_rule_locked fails the same way.
  • The handler tests drive the update with POST, so they pass against the wrong method.
  • Fix: route http.MethodPut to updateRetentionRule. Either drop POST or return 405 for it, and switch the tests to PUT.

Medium

2. A PAR created with bucketListingAction: ListObjects cannot list (server/oci/objectstorage/par.go:119)

  • Create an AnyObjectRead PAR with bucketListingAction: "ListObjects". The create succeeds and echoes the value back. A GET on the returned accessUri (/p/{token}/n/{ns}/b/{bucket}/o/) then returns 400 a pre-authenticated request addresses an object under /o/.
  • OCI documents exactly this request: a GET on the PAR URL ending in /o/ returns {"objects":[...]} and accepts the ListObjects query parameters (prefix, fields, start, limit) (Working with PARs). It is the usual way to share a read-only bucket view, and the field is accepted here and then ignored.
  • Related: bucketListingAction is not validated. "Bogus" is stored and returned. The SDK enum is Deny / ListObjects.
  • Fix: when Rest == "" and the PAR is an AnyObject read type with ListObjects, serve listObjects scoped to the PAR's bucket and objectName prefix. Otherwise return 403/404 as now. Reject unknown bucketListingAction values with 400.

3. MD5 headers do not match the API (server/oci/objectstorage/object.go:557, :57)

  • GetObject and HeadObject return the hash as Opc-Content-Md5. The API returns it as Content-MD5, which is the header the SDK maps to GetObjectResponse.ContentMd5 / HeadObjectResponse.ContentMd5. Through the SDK, ContentMd5 is nil for every object. Tools that compare hashes from HeadObject (rclone's OCI backend, sync scripts) then cannot compare. opc-content-md5 is correct on the PutObject response only.
  • A multipart object should carry opc-multipart-md5 on CommitMultipartUpload, GET and HEAD. Here commit returns neither header, and HeadObject on the committed object has ContentMd5 and OpcMultipartMd5 both nil.
  • PutObject ignores a Content-MD5 request header. PUT with Content-MD5: AAAAAAAAAAAAAAAAAAAAAA== and body hello returns 200 and stores the object. OCI rejects a body whose MD5 does not match with 400 (PutObject), and that header is what the SDK upload manager and Terraform's content_md5 send to catch corrupt uploads.
  • Fix: in stampObjectHeaders emit Content-MD5 for single-part objects and opc-multipart-md5 (<base64 md5 of the part md5s>-<count>) for multipart ones, keeping opc-content-md5 on the PutObject response. Verify Content-MD5 in putObject and uploadPart.

Low

  • Duplicate part numbers in a commit are concatenated twice (providers/oci/objectstorage/multipart.go:195). partsToCommit [{1},{1},{2}] with parts AAA/BB commits AAAAAABB. Reject a repeated partNum with 400 rather than building a corrupt object.
  • PAR redemption ignores the namespace (server/oci/objectstorage/par.go:126). /p/{token}/n/wrongns/b/pb/o/logs/a.txt returns the object. Check rt.Namespace against Namespace() and return the same 404 as elsewhere.
  • PAR accessUri and fullPath (providers/oci/objectstorage/par.go:205, server/oci/objectstorage/par.go:70). An object name with a space comes back unescaped (/o/my file.txt), so the URI cannot be used as is. fullPath is the same relative path as accessUri, while OCI returns the absolute URL. Path-escape the object name, and build fullPath from the request host.
  • GetRetentionRule has no etag response header (server/oci/objectstorage/retention.go:77). The body has etag, but GetRetentionRuleResponse.Etag (a header field in the SDK) is nil. CreateRetentionRule sets the header, so stamp it on Get and Update as well.
  • Last-Modified is RFC 3339 (server/oci/objectstorage/object.go:553). HTTP and OCI use the HTTP-date format (Mon, 12 Oct 2026 12:40:52 GMT). The Go SDK tolerates it. http.ParseTime and most non-SDK clients do not.
  • Unclaimed SDK operations answer a generic 404: replicationPolicies, replicationSources, actions/batchDeleteObjects, actions/makeBucketWritable and actions/reencrypt/{object} return 404 unknown collection/action. PUT /n/{ns} (UpdateNamespaceMetadata) returns 405. /n/{ns}/pe (private endpoints) falls through to 501 no handler. Since reencrypt and restoreObjects already get an explicit 501, giving these the same 501 (and listing them under "Not emulated" in docs/services.md) would tell a caller what is going on. The OCI S3 Compatibility API is not emulated either. That is fine, because it lives on its own *.compat.objectstorage host and the OCI port shares nothing with the AWS S3 handler, but the docs should say so.

OCI docs divergences (summary)

Operation Documented Observed
UpdateRetentionRule PUT .../retentionRules/{id} 405 on PUT, served on POST
PAR bucket listing GET /p/.../o/ lists objects when ListObjects 400
GetObject / HeadObject Content-MD5 or opc-multipart-md5 header Opc-Content-Md5 only
PutObject Content-MD5 mismatch → 400 accepted
CreatePreauthenticatedRequest bucketListingAction ∈ {Deny, ListObjects} any string stored
CommitMultipartUpload opc-multipart-md5 header absent

Terraform and serve

  • Terraform was not run. The oracle/oci provider has no per-service endpoint override, and the HTTPS proxy route needs a local TLS-intercepting proxy, which I could not set up in this environment. As the end-to-end fallback I drove serve --providers oci with the real oci-go-sdk v65.126.1 (NewRawConfigurationProvider, client.Host set to serve) through each resource the Terraform types map to:
    • Bucket: create, then get twice (identical body and etag), then update (tags replaced, versioning Enabled, id and timeCreated unchanged). Bogus if-match returns 412. After delete, get returns 404. Deleting a bucket that still holds versions returns 409, and deletes once the versions are purged.
    • Object: put, then head twice (ETag stable and equal to the put ETag; length and opc-meta-* correct), a range get, list with fields, and overwrite (2 versions). After delete, get returns 404.
    • Preauthrequest: create, then redeem with no auth (200, correct bytes), then get twice (identical, timeCreated and timeExpires equal to the create response). After delete, get returns 404.
    • Lifecycle policy: put, then get twice (identical, timeCreated equal to put), then update (60 days reads back). After delete, get returns 404.
    • Retention rule: create and get twice (identical). Delete of a held object is refused. Delete of the rule followed by get returns 404. Update fails with 405 (High 1).
    • Multipart: create, upload 2 parts, list parts, commit. Copy works and its work request reads back as COMPLETED.
  • Mapped to the Terraform flow: oci_objectstorage_bucket, oci_objectstorage_object, oci_objectstorage_preauthrequest and oci_objectstorage_object_lifecycle_policy would apply and read back with stable computed fields. A retention_rules change on the bucket would fail on update.
  • Serve: OCI is reachable through serve --providers oci --oci-port <p>. The Dockerfile and serve wiring are unchanged apart from the new --oci-tenancy flag, so I did not rebuild the image. Library and serve go through the same provider mock, and the wire layer only translates.

What I verified

  • Head bf70760e merges cleanly with development 58fdd8c6. The delta since the last review is the one fix commit (UUID version and upload ids, retention rule id guard, AcceptAs/COMPLETED).
  • go build ./... is clean. go vet is clean on providers/oci, server/oci, services/storage, serveflags and persist.
  • go test -race passes on providers/oci/..., server/oci/..., persist, serveflags, services/storage/... and internal/coveragegen. contrib/realengine/blobstore tests pass.
  • go mod tidy leaves no diff. gofmt is clean. go generate ./... leaves no diff.
  • golangci-lint 2.14 with --new-from-rev on the touched packages reports 0 issues. The 10 goconst hits in monitoring/query.go and serveflags.go are on lines this PR does not touch.
  • Coverage: providers/oci/objectstorage 96.2%, server/oci/objectstorage 96.2%, server/oci/workrequest 96.4%.
  • A concurrent create/update/rename/put/PAR/retention/snapshot/delete probe across 16 goroutines is -race clean (3 runs).
  • Architecture: the provider implements storagedriver.Bucket plus the handler-side Extras. The wire layer uses ocirest. Copy goes through the shared work request store. The compartment checker matches the VCN pattern. Snapshottable is in place. OCIDs come from idgen and survive a restore.
  • CI: every check is green.

Verdict

Request changes. High 1 is a one-line routing fix, but it breaks a documented operation for the SDK, the CLI and Terraform. Mediums 2 and 3 are real fidelity gaps that clients will hit. Everything else looks solid, and the restore and work request fixes hold up under a real restart.

Comment thread server/oci/objectstorage/retention.go Outdated
switch r.Method {
case http.MethodGet:
h.getRetentionRule(w, r, rt.Bucket, rt.Rest)
case http.MethodPost:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

UpdateRetentionRule is PUT /n/{ns}/b/{bucket}/retentionRules/{id} in the API, and that is what oci-go-sdk sends. The table this PR adds to docs/services.md says PUT too. As written, client.UpdateRetentionRule gets 405 PUT is not allowed, and so do oci os retention-rule update and a Terraform change to a retention_rules block on oci_objectstorage_bucket (it goes through updateRetentionRulesHelper). Route http.MethodPut here and move the handler tests from POST to PUT.

return
}

if rt.Sub != subObjects || rt.Rest == "" {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

A PAR created with bucketListingAction: "ListObjects" should be able to list: OCI documents GET /p/{token}/n/{ns}/b/{bucket}/o/ returning {"objects":[...]} with the usual ListObjects parameters. Here that request returns 400, so the field is accepted and then ignored. When rt.Rest == "" and the PAR is AnyObjectRead/AnyObjectReadWrite with ListObjects, serve listObjects scoped to the PAR's bucket and objectName prefix. Also validate bucketListingAction against Deny/ListObjects at create ("Bogus" is stored today).

return
}

if rt.Bucket != par.Bucket {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The namespace in the redemption path is never checked: /p/{token}/n/wrongns/b/pb/o/logs/a.txt returns the object. Compare rt.Namespace with h.extras.Namespace() and return the same 404 as the rest of the handler.

Comment thread server/oci/objectstorage/object.go Outdated
w.Header().Set(headerStorageTier, d.StorageTier)

if d.MD5 != "" {
w.Header().Set("Opc-Content-Md5", d.MD5)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

GetObject and HeadObject return the hash as Content-MD5 in OCI (the SDK reads content-md5 into ContentMd5). Only the PutObject response uses opc-content-md5. Through the SDK, HeadObjectResponse.ContentMd5 is nil for every object, so hash-based sync tools cannot compare. Multipart objects should carry opc-multipart-md5 (<md5 of part md5s>-<n>) on commit, GET and HEAD. Today commit returns neither header.

}

func (h *Handler) putObject(w http.ResponseWriter, r *http.Request, bucket, object string) {
data, ok := readBody(w, r)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

A Content-MD5 request header is never checked: a PUT with Content-MD5: AAAAAAAAAAAAAAAAAAAAAA== and body hello returns 200. OCI rejects a body whose MD5 does not match with 400, and that is the integrity check the SDK upload manager and Terraform's content_md5 rely on. Verify it here and in uploadPart.


var data []byte

for _, p := range ordered {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

A repeated partNum in partsToCommit is appended twice: committing [{1},{1},{2}] over parts AAA/BB stores AAAAAABB. Reject duplicates with InvalidArgument instead of building a corrupt object.


// accessURI is the path a PAR is redeemed at, matching the shape real OCI
// returns: /p/{token}/n/{namespace}/b/{bucket}/o/{object}.
func (m *Mock) accessURI(par *parData) string {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The object name goes into the access URI unescaped, so my file.txt yields /o/my file.txt, which a client cannot use as is. Path-escape it. Separately, the handler sets fullPath to this same relative path, while OCI returns the absolute URL.

@arunesh-j

Copy link
Copy Markdown
Collaborator Author

Thanks for the SDK run. Everything is addressed in 78f386f, and every new or changed test fails on the pre-fix code.

High

  1. UpdateRetentionRule on PUT — PUT /retentionRules/{id} is routed to the update, and POST now returns 405. GetRetentionRule and UpdateRetentionRule also stamp the ETag header (the Low). TestRetentionWireErrors now uses PUT and checks both the POST 405 and the header.

Medium
2. PAR bucket listing — bucketListingAction is validated at create: Deny or ListObjects, empty means Deny, and anything else is 400. A GET on /p/{token}/n/{ns}/b/{bucket}/o/ through an AnyObjectRead or AnyObjectReadWrite PAR with ListObjects returns {"objects":[...]}. It is scoped to the PAR's bucket and objectName prefix, and takes prefix, start and limit the same way ListObjects does. fields is accepted; ListObjects here always returns every field, so the listing does too. PARs without listing keep the refusal. Tests: TestPARListObjects, TestPARListObjectsRefusedWithoutListing, TestPARBucketListingAction.
3. MD5 headers

  • GetObject and HeadObject return Content-MD5 for single-part objects and opc-multipart-md5 (<base64 md5 of the part md5s>-N) for multipart ones. CommitMultipartUpload returns opc-multipart-md5, and PutObject keeps opc-content-md5.
  • The multipart digest is persisted and carried through copy, rename and versions.
  • PutObject and UploadPart verify a Content-MD5 request header and return 400 InvalidParameter on a mismatch.
  • Tests: TestObjectMD5Headers, TestContentMD5IsVerified, TestMultipartMD5SurvivesSnapshot.

Low

  • A repeated partNum on commit is 400 (TestCommitRejectsDuplicatePartNumbers, provider and wire).
  • PAR redemption checks the namespace and returns the usual 404 (TestPARRedemptionChecksNamespace).
  • accessUri path-escapes the object name, and fullPath is the absolute URL from the request's scheme and host (TestPARAccessURIEscapesObjectName).
  • Last-Modified is an HTTP-date (TestLastModifiedIsHTTPDate).
  • replicationPolicies, replicationSources, batchDeleteObjects, makeBucketWritable, reencrypt/{object}, PUT /n/{ns} and /n/{ns}/pe now answer 501 with a reason, through the same helper as reencrypt and restoreObjects (TestUnemulatedOperationsAnswer501).
  • docs/services.md lists them under "Not emulated", notes that the S3 Compatibility API (its own *.compat.objectstorage host) is not emulated, and documents the listing route and the MD5 behaviour.

Local: providers/oci/..., server/oci/..., persist/..., internal/coveragegen/... and services/storage/... pass. -race is clean on both objectstorage packages, and golangci-lint --new-from-rev reports 0 issues.

…tention, lifecycle

Buckets, objects, multipart uploads, pre-authenticated requests, retention
rules, lifecycle policies, versioning and copy work requests, with
persistence and Monitoring metrics through the service-side ingest path.
@arunesh-j
arunesh-j force-pushed the feat/oci-objectstorage branch from 78f386f to 8f7482a Compare October 12, 2026 17:52
@arunesh-j

Copy link
Copy Markdown
Collaborator Author

Rebased onto development now that #421 (OCI Notifications) has merged, and squashed to one commit, since the repo squash-merges anyway and that keeps the conflict resolution in one place. The only conflicts were the shared wiring in providers/oci/oci.go / server/oci/oci.go, where both sides register a service (both are kept), and the generated docs/coverage, which is regenerated. The service code is byte-identical to the commit you reviewed. Build, tests and lint are clean on the rebased tree.

@NitinKumar004 NitinKumar004 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Every point from the last round is fixed. I checked each one against serve with the real oci-go-sdk (v65.126.1) at 8f7482aa, including a --persist restart. The service code matches what you described, and the rebase onto #421 only touches the shared wiring and the generated coverage docs. Two small gaps are left, both Low and neither blocking.

Earlier findings

Finding Status Evidence
High: UpdateRetentionRule served on POST Fixed client.UpdateRetentionRule (PUT) returns 200 with the new name and duration. The etag header matches the body and changes on update. A raw POST to the rule now returns 405.
Medium: PAR ListObjects listing returned 400, value not validated Fixed Ran an AnyObjectRead PAR with objectName: logs/ and ListObjects. GET {accessUri} lists only logs/*. prefix=logs/sub narrows the list, prefix=other returns [], limit=1 pages, and delimiter=/ returns prefixes: ["logs/sub/"]. An object outside the prefix gets 403, and so does another bucket. A PAR with no listing action stores Deny, and its listing is still refused. A raw "Bogus" is rejected with 400 (the provider test covers it, and the SDK enum blocks it before the wire). Listing PARs survive a restart, and the old token still lists.
Medium: MD5 headers Fixed (one gap, see Low 1) GET and HEAD carry Content-MD5, so the SDK's ContentMd5 is now set. PutObject returns opc-content-md5, and UploadPart does too. A mismatched Content-MD5 on PutObject or UploadPart returns 400 InvalidParameter, and the object is not stored. On commit, opc-multipart-md5 is 9ni10fXspkgns40PbziFLQ==-2, which I computed independently for parts AAA/BB. HEAD, GET, ListObjects md5 and a copy of the object all carry the same value, and it is still there after a persist restart.
Low: duplicate part numbers Fixed [{1},{1},{2}] returns 400 part 1 is listed more than once.
Low: PAR namespace not checked Fixed A wrong namespace returns 404 NotAuthorizedOrNotFound, both on object redemption and on the listing.
Low: PAR accessUri / fullPath Fixed my file.txt comes back as /o/my%20file.txt. fullPath is http://127.0.0.1:<port>/p/..., and both URLs redeem to the object.
Low: no etag header on GetRetentionRule Fixed GetRetentionRuleResponse.Etag is set and equals the body etag.
Low: Last-Modified format Fixed Mon, 12 Oct 2026 18:05:04 GMT, and http.ParseTime accepts it. Versioned reads use the same format.
Low: unclaimed operations return a generic 404 Fixed replicationPolicies, replicationSources, batchDeleteObjects, makeBucketWritable, reencrypt/{object}, PUT /n/{ns} and /n/{ns}/pe[/...] all return 501 with a reason. Through the SDK, UpdateNamespaceMetadata, ListReplicationPolicies and MakeBucketWritable surface 501 NotImplemented. Unknown collections and actions still return 404. docs/services.md lists all of them and notes that the S3 Compatibility API is not emulated.
Earlier rounds (restore collisions, copy work request COMPLETED, routing, conditionals, ranges, lifecycle, PAR tokens) Still fixed Range bytes=1-3 returns ell with Content-Range: bytes 1-3/5. PAR tokens and multipart digests survive the restart.

High

None.

Medium

None.

Low

1. GET and HEAD by versionId still have no MD5 header (server/oci/objectstorage/object.go:142, :300)

  • With versionId set, both reads go through stampInfoHeaders, which sets neither Content-MD5 nor opc-multipart-md5. Through the SDK, GetObject and HeadObject with VersionId return ContentMd5 = nil for a plain PUT, while the same object without versionId reports it.
  • Real OCI returns the version's digest. The version chain already keeps contentMD5 and multipartMD5, so the data is there.
  • Fix: expose them through the versioned details and call stampObjectMD5 on those paths too.

2. if-match is ignored on retention rule and lifecycle writes (server/oci/objectstorage/retention.go:60, server/oci/objectstorage/lifecycle.go:24)

  • UpdateRetentionRule with if-match: bogus returns 200 and applies the update. The API takes if-match on UpdateRetentionRule, DeleteRetentionRule, PutObjectLifecyclePolicy and DeleteObjectLifecyclePolicy (the SDK request types carry IfMatch for all four), and buckets and objects here already enforce it with 412.
  • This matters most for retention rules, because the etag that Get and Update now return can't be used to guard a concurrent change.
  • Fix: pass If-Match through to the provider and return 412 IfMatchFailed on a mismatch, as UpdateBucket does.

OCI docs divergences (summary)

Operation Documented Observed
GetObject / HeadObject with versionId Content-MD5 or opc-multipart-md5 neither header
UpdateRetentionRule / DeleteRetentionRule if-match mismatch returns 412 ignored
PutObjectLifecyclePolicy / DeleteObjectLifecyclePolicy if-match mismatch returns 412 ignored

Terraform and serve

  • I did not run Terraform. The oracle/oci provider has no per-service endpoint override, and the HTTPS proxy route was not available in this environment. As the fallback I drove serve --providers oci --oci-port <p> with the real oci-go-sdk through each operation behind oci_objectstorage_bucket's retention_rules, oci_objectstorage_object, oci_objectstorage_preauthrequest and multipart uploads.
  • The earlier blocker for Terraform, a retention_rules change on the bucket going through UpdateRetentionRule, now succeeds with PUT and reads back the new duration.
  • A --persist --persist-strategy on-shutdown restart keeps multipart digests (on the original and on its copy), single-part Content-MD5, all four PARs with their bucketListingAction, and working listing tokens. A PAR created after the restart lists correctly.

What I verified

  • Head 8f7482aa is one squashed commit on development 4cc5c573, and it merges cleanly. The delta from bf70760e is the fixes above. The rest is the #421 wiring in providers/oci/oci.go and server/oci/oci.go, where both services are registered, plus regenerated coverage docs.
  • go build ./... is clean. go vet is clean on providers/oci, server/oci, persist and serveflags. gofmt is clean.
  • go generate ./... and go mod tidy leave no diff.
  • go test -race passes on providers/oci/..., server/oci/..., persist/..., server/serveflags and internal/coveragegen.
  • golangci-lint with --new-from-rev on the touched packages reports 0 issues.
  • Coverage: providers/oci/objectstorage 96.2%, server/oci/objectstorage 96.6%.
  • A concurrent probe across 12 goroutines is -race clean over 3 runs. It mixed puts, multipart create/upload/commit, HEAD, PAR listing, PUT retention updates, gets, snapshots and deletes.
  • CI: all checks are green.

Verdict

Approve. The High and both Mediums from the last round are fixed and hold up through the SDK and a real restart. The two Lows above are small follow-ups and can land here or separately.


etag = details.ETag
} else {
stampInfoHeaders(w, &obj.Info)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

With versionId set, GET (and HEAD at line 300) goes through stampInfoHeaders, which sets no MD5 header. Through the SDK, GetObject/HeadObject with VersionId return ContentMd5 = nil, while the same object read without versionId has it. The version chain already keeps contentMD5/multipartMD5, so expose them and call stampObjectMD5 on the versioned paths too.

return
}

rule, err := h.extras.UpdateRetentionRule(r.Context(), bucket, ruleID, spec)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

if-match is not checked here: UpdateRetentionRule with if-match: bogus returns 200 and applies the update. OCI takes if-match on Update/DeleteRetentionRule and Put/DeleteObjectLifecyclePolicy (all four SDK request types have IfMatch), and buckets and objects here already return 412. Pass If-Match through and return 412 IfMatchFailed on a mismatch. That way the etag that Get and Update now return can actually guard a concurrent change.

@arunesh-j
arunesh-j merged commit 7803c76 into development Oct 12, 2026
23 checks passed
@arunesh-j
arunesh-j deleted the feat/oci-objectstorage branch October 12, 2026 18:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

oci Oracle Cloud Infrastructure

Projects

None yet

Development

Successfully merging this pull request may close these issues.

OCI Object Storage: buckets, objects, multipart

2 participants