Skip to content

Add OCI Notifications: topics and subscriptions - #421

Open
arunesh-j wants to merge 4 commits into
developmentfrom
feat/oci-notifications
Open

arunesh-j wants to merge 4 commits into
developmentfrom
feat/oci-notifications

Conversation

@arunesh-j

@arunesh-j arunesh-j commented Aug 21, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

  • Implements OCI Notifications (ONS) against the existing portable notification driver.
  • Topics, subscriptions with the real PENDING → ACTIVE confirmation flow, and message publishing.
  • Nothing added to services/notification/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 #415. Part of #376.

Changes

  • providers/oci/notifications/ — Mock over memstore implementing driver.Notification, guarded by a sync.RWMutex.
  • server/oci/notifications/ — the /20181201/ surface. Topics: Create/Get/List/Update/Delete/ChangeCompartment. Subscriptions: Create/Get/List/Update/Delete/Confirm/Unsubscribe/ResendConfirmation/ChangeCompartment. Data plane: PublishMessage.
  • Wiring is one line each in providers/oci/oci.go and server/oci/oci.go.

The confirmation flow

CreateSubscription mints an OCID, sets lifecycleState: PENDING and a confirmation token. Real ONS mails the token to the endpoint; the emulator has no channel, so the token rides back in the response body and is dropped once the subscription is ACTIVE. GET /subscriptions/{id}/confirmation validates token and protocol, flips to ACTIVE, and returns an unsubscribeUrl pointing at this emulator's own origin.

PublishMessage skips anything not ACTIVE, so a publish to a PENDING subscription delivers to nobody — asserted at both driver and wire layers, and visible in the transcript below.

Two corrections to the issue as written

  1. The prefixes claim was wrong. OCI Notifications: topics and subscriptions #415 says the control and data planes sit under different API prefixes. They don't — real ONS splits them by host (the topic's apiEndpoint), both on /20181201. Implemented as POST /20181201/topics/{id}/messages, with every topic reporting the requesting origin as its apiEndpoint, so an SDK pointed at the returned endpoint lands back on the same listener.
  2. DeleteTopic returns 204, not 202. Real ONS answers 204 with an opc-work-request-id. docs/oci-conventions.md's generic 202 is the common case, not ONS's. The work request is still recorded and pollable.

Never accept-and-ignore

Refused with a named 400 rather than silently dropped: message attributes (ONS has no such field), definedTags, protocols outside the ONS set, unknown messageType, unsupported sortBy/sortOrder. Protocol aliases (email, https → CUSTOM_HTTPS) are mapped, not dropped.

Provider Coverage

  • AWS
  • Azure
  • GCP
  • OCI

Checklist

  • All tests pass (go test ./...) — see note below
  • 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 ./...                                               271 ok
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

Correction on a suite note. An earlier run of this branch showed cmd/cloudemu TestServeOutOfProcess failing, and I first attributed it to the sandbox. That was wrong. It is contention on the shared ~/.cloudemu daemon lock between the six Wave 2 worktrees running their suites in parallel — self-inflicted, not a repo bug. Re-run with nothing else running, the whole suite is exit 0 including that test.

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 4616):

create topic                        -> 200, apiEndpoint = requesting origin
create subscription                 -> 200, lifecycleState PENDING + confirmationToken
publish BEFORE confirmation         -> 200 accepted, delivered to nobody
list                                -> still PENDING
confirm with token                  -> 200 + unsubscribeUrl
get subscription                    -> ACTIVE, token no longer echoed
publish AFTER confirmation          -> 200, delivered
list topics without compartmentId   -> 400 InvalidParameter
delete topic                        -> 204 + Opc-Work-Request-Id
list topics / subscriptions         -> [] , []  (cascade)
poll work request                   -> SUCCEEDED, DELETE_TOPIC, resources[0] DELETED

Left out

No oci-go-sdk compat test — the SDK splits ONS across two clients with a host override, needing more scaffolding than it would buy; wire shapes are asserted field-by-field instead. Publishing records deliveries rather than pushing to real HTTPS/email endpoints, matching how providers/aws/sns handles non-SQS protocols. definedTags is refused rather than modelled, consistent with the rest of the OCI surface.

@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.

Findings

Medium · docs — docs/services.md not updated with OCI Notifications — unmet Definition-of-done checkbox
docs/services.md:23
If a user consults docs/services.md (the human-facing service catalog) to learn what OCI surface CloudEmu emulates -> they find no ONS topics/subscriptions/publish entry and conclude the service is unimplemented, because the vertical slice's documentation step was skipped even though the code and generated coverage exist.

git diff merge-base..HEAD shows docs/services.md untouched; only auto-generated docs/coverage/* changed. oci-conventions.md 'Definition of done' requires 'Operations added to docs/services.md', and sibling OCI services each have a hand-written section (OCI Monitoring line 623, OCI identity line 749, OCI VCN note line 407). Notifications has none.

Low · structure — Per-feature filename not mirrored across provider/wire layers for topics and publish
providers/oci/notifications/publish.go:1
If a maintainer greps for the publish feature by filename (publish.go) -> they find only the provider side and miss that the wire implementation lives in topics.go, because the feature file name is not mirrored on the wire layer; a navigation cost, not a behavioral defect.

STRUCTURE.md §3: 'A feature uses the same filename across all three layers.' subscriptions.go matches (provider+wire), but topic CRUD is provider notifications.go vs wire topics.go, and publish is provider publish.go vs wire (folded into topics.go). provider notifications.go is defensible as the §4 .go core-CRUD file, but publish.go/topics.go breaks the one-grep goal.

@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 Notifications

Verdict: Request changes — one blocker (persistence completeness), otherwise clean and faithful to the existing OCI convention.

Blocking

Mock is not Snapshottable — providers/oci/notifications/notifications.go (Mock).
The Mock holds three memstore.Store fields (topics, subscriptions, deliveries) but implements no snapshot.Snapshottable, and there's no snapshot.go. Every sibling OCI provider ships one (providers/oci/{identity,vcn,monitoring}/snapshot.go). On a tree that includes the persistence-completeness guard, go test ./persist/... fails:

oci: field Notifications (*notifications.Mock) holds a memstore.Store but is not
Snapshottable — add persistence (see #582) or justify in guardExclude

Consequence: CI go test ./... goes red, and a serve stop/start silently drops every topic and subscription.

Fix: add providers/oci/notifications/snapshot.go with var _ snapshot.Snapshottable = (*Mock)(nil) plus Snapshot/Restore over topics + subscriptions (decide whether the transient deliveries store persists or is justified in guardExclude). The branch is also behind development, so rebase + re-run go generate before merge.

Non-blocking

  • Create returns 200, not 201 — server/oci/notifications/topics.go (CreateTopic) and subscriptions.go (CreateSubscription). Real ONS returns 201 Created. SDK/Terraform accept any 2xx, but a strict raw-HTTP client asserting 201 diverges.
  • if-match/etag not enforced — server/oci/notifications/topics.go (update/delete). The etag is minted, stored, and returned correctly, but the handler ignores the if-match header, so a stale-etag update never returns 412.
  • No endpoint-format / message-size validation — providers/oci/notifications/subscriptions.go (endpoint) and publish.go (body). EMAIL endpoints aren't checked for @, CUSTOM_HTTPS endpoints aren't required to be https, and the message body has no 64 KB cap; real ONS 400s these.

Verified good

Full lifecycle exercised over the wire (create topic → subscribe → confirm → publish → delete with cascade): computed IDs/timeCreated stable across reads, etag rotates on update, dup→409, missing→404, subscription-under-missing-topic→404, bad protocol→400, delete returns Opc-Work-Request-Id, subscription cascade-deleted with its topic. Wiring is complete end-to-end (provider field + New() + monitoring wire + registration + from_provider.go + work-request store). No check-then-set or shared-pointer aliasing races (-race clean; every Get/List deep-copies). Structure, lint, and go generate idempotency all pass.

@arunesh-j
arunesh-j force-pushed the feat/oci-notifications branch from b22f8b3 to 344b484 Compare September 7, 2026 18:54
@arunesh-j

Copy link
Copy Markdown
Collaborator Author

Rebased onto a45ac909 and addressed everything in 344b484d (plus 5c328532 from the earlier round). Two rebases, zero conflicts both times, and no services/notification/driver drift.

Blocking — snapshot.go

Added, mirroring providers/oci/vcn/snapshot.go's shared storeDump table. go test ./persist/... now ok.

I checked the four traps that bit the sibling OCI branches, and none applied here — recording it so the next reviewer doesn't re-derive it:

  • Unexported fields — none needing export. topicData is an unexported type, but every field on it is exported, as are Subscription, Message and DeliveryPolicy. No renames, no public API change.
  • Nested stores — none. No *memstore.Store appears inside a stored value, so no custom MarshalJSON was needed. This is what broke Compute's poolData.
  • Lazily-minted defaults — no equivalent. ONS mints nothing by default, unlike Vault's defaultVaultID.
  • Counters — no per-mock counter. IDs come from idgen's process-global atomic, shared across every provider and not this snapshot's state; the siblings don't snapshot it either. Confirmation tokens travel as data on the subscription, which is what actually matters.

deliveries is persisted, not excluded, and guardExclude stays empty. It stands in for the endpoint inbox real ONS pushes to and is the only thing Deliveries() reads — dropping it would report a restored ACTIVE subscription as having received nothing, which is silent data loss rather than transience.

Round-trip tests cover topics, subscriptions, the topic-subscription cross-reference, delivery history, and a PENDING subscription still pending after restore, receiving nothing on a pre-confirm publish, and still confirmable with its original token, plus malformed and empty input.

Non-blocking — all three fixed

  • 201 on create — createTopic/createSubscription now return StatusCreated; existing 200 assertions updated, pinned by TestCreateAnswers201.
  • if-match enforced — new checkIfMatch; stale gives 412/NoEtagMatch, an absent header is unconditional, an unknown resource still 404s through the driver. Scope note: you named only topics.go; I applied it to subscriptions too — same defect, same helper, and leaving one half unenforced would be incoherent. Easy to revert if you disagree.
  • Validation — validateEndpoint (EMAIL needs an @; CUSTOM_HTTPS/SLACK/PAGERDUTY need https://) and a 64 KB maxMessageBytes cap, each rejecting by name. Boundary tested: exactly 64 KiB passes, one byte more is a 400.

A bug found outside the findings — and it overlaps #422

ONS's metric namespace oci_notification was being silently dropped: providers/oci/monitoring rejects oci_-prefixed namespaces and emitMetric discards the error, so the entire SetMonitoring path was dead code. Fixed by splitting the reserved-prefix check off the in-process producer path — PostMetricData (the public API, where the rule belongs, rejecting caller input) still enforces it; PutMetricData (how a sibling mock emits its own service metrics) now admits it.

#422 independently found and fixed the same bug, with a slightly different diff. Whichever merges first will conflict the other. Both touch providers/oci/monitoring, which neither service branch really owns — happy to extract it into its own small PR and rebase both onto it, if you would prefer that to resolving a conflict at merge time.

Coverage

package before after
providers/oci/notifications 95.8% 99.0%
server/oci/notifications 78.6% 96.2%

Verification

go build ./...                                     clean
go test ./...                                      exit 0
go test ./persist/...                              ok        (the blocker)
go test -race (both notifications packages)        2/2 ok
golangci-lint (both packages)                      0 issues
go generate ./...                                  re-running produces no diff
no OCI op in docs/coverage/{aws,azure,gcp}/*.md    confirmed

Two lint findings surfaced and were fixed rather than waived: canonicalheader on my if-match (to If-Match; Header.Get is case-insensitive so wire behaviour is unchanged) and gocritic hugeParam on Publish, given the same //nolint the neighbouring interface methods already carry.

E2E, port 4616

CreateTopic                -> 201 Created, etag-00000003
CreateSubscription         -> 201 Created, PENDING, confirmationToken token-00000009
Publish (PENDING)          -> 200, delivers to nobody
Confirm                    -> 200
Publish (confirmed)        -> 200
if-match fresh             -> 200
if-match replayed          -> 412 NoEtagMatch, and the resource is unchanged
bad EMAIL endpoint         -> 400 "... must hold an @"
CUSTOM_HTTPS over http     -> 400 "... must be an https URL"
body 64KiB+1               -> 400 "message body is 65537 bytes; ONS caps a message at 65536"
body exactly 64KiB         -> 200
DeleteTopic                -> 204 + Opc-Work-Request-Id
work request               -> DELETE_TOPIC / SUCCEEDED / 100%
GET topic / subscription   -> 404 / 404  (cascade)

On the cmd/cloudemu flake

An interim run showed cmd/cloudemu failing. It is not this branch — the diff touches nothing under cmd/ or persist/ (verified: 0 files). With nothing else running, go test ./cmd/cloudemu/ passes on clean development in 5.4s. The failures come from parallel worktree suites colliding on the shared ~/.cloudemu daemon lock.

@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 is close. The ONS surface is broad and the PENDING to ACTIVE flow is right for most protocols. A few wire-shape gaps still show up when a real SDK client is used. The branch also needs a rebase onto current development.

Earlier review points

  • Mock not Snapshottable: fixed. snapshot.go is there and go test ./persist/... passes on the merged tree. A topic created before a graceful serve --persist restart was still there afterwards.
  • Create returned 200: fixed. CreateTopic and CreateSubscription now answer 201.
  • if-match ignored: fixed for topics and subscriptions. A stale etag gets 412 NoEtagMatch. See the Low note on atomicity below.
  • Endpoint and message-size validation: fixed. A bad EMAIL endpoint, a CUSTOM_HTTPS endpoint over http, and a 64 KiB + 1 byte body are each rejected with 400.
  • docs/services.md missing ONS: fixed. There is now an "OCI Notifications (ONS)" section.
  • Publish not mirrored on the wire: fixed. server/oci/notifications/publish.go now exists.

High

  1. Branch does not merge cleanly with current development. git merge origin/development conflicts in docs/coverage/README.md. Development has since regenerated it with plain hyphens and new services. docs/coverage/coverage.json and docs/services.md merge on their own. After I took development's README and re-ran go run ./internal/coveragegen, the only diff left was the new OCI Notifications cell. Fix: rebase onto development, re-run go generate ./... and commit the regenerated docs/coverage.

Medium

  1. GetSubscription loses the delivery policy for SDK and Terraform clients. server/oci/notifications/types.go:75
    The ONS Subscription model (used by Get, Create and Update) carries the policy as a JSON string in deliverPolicy. Only SubscriptionSummary (List) uses the deliveryPolicy object. The handler sends deliveryPolicy as an object on every response.
    Repro with oci-go-sdk v65: call UpdateSubscription with BackoffRetryPolicy{MaxRetryDuration: 7200000, PolicyType: EXPONENTIAL}, then GetSubscription. DeliverPolicy is nil, while ListSubscriptions returns the policy. The Terraform oci_ons_subscription.delivery_policy attribute is read from DeliverPolicy, so a configured policy drifts on every plan.
    Fix: in subscriptionResponse, send deliverPolicy as the marshalled JSON string. Keep the object form only for list items.

  2. PublishMessage reads messageType from the query string, but the SDK sends it as a header. server/oci/notifications/publish.go:27
    PublishMessageRequest.MessageType is contributesTo:"header" name:"messageType". Repro: curl -X POST -H 'messageType: NOPE' .../20181201/topics/{id}/messages -d '{"body":"b"}' returns 200. A JSON publish from the SDK is stored as RAW_TEXT. The handler's 400 for an unknown type only works through ?messageType=, which no client sends.
    Fix: read r.Header.Get("messageType"). The query fallback can stay if you want it.

  3. ORACLE_FUNCTIONS subscriptions are created PENDING. providers/oci/notifications/subscriptions.go:151
    The OCI docs say "Confirmation isn't required for function subscriptions." Here a Functions subscription starts PENDING and gets a confirmation token, so a publish delivers nothing until the caller confirms something real ONS never asks it to confirm. Repro: create a subscription with protocol: ORACLE_FUNCTIONS and you get "lifecycleState":"PENDING" plus a confirmationToken.
    Fix: create ORACLE_FUNCTIONS subscriptions ACTIVE with no token, and add a driver test and a wire test for it.

  4. Compartments are not validated, and topic names are unique per compartment instead of per tenancy. server/oci/notifications/topics.go:87, providers/oci/notifications/notifications.go:138
    Development now gates VCN creates on an existing compartment through vcnHandler.SetCompartmentChecker in server/oci/oci.go. POST /20160918/vcns into ocid1.compartment.oc1..bogus returns 404 NotAuthorizedOrNotFound. ONS accepts the same compartment with 201, for both CreateTopic and ChangeTopicCompartment (202), and CreateSubscription accepts any compartment too.
    Separately, CreateTopicDetails.name in the SDK says "The topic name must be unique across the tenancy". Creating t1 in the root compartment and then t1 in another compartment returns 201 both times. Real ONS returns 409.
    Fix: give the notifications handler the same compartment checker hook as VCN and wire it in server/oci/oci.go. Make topicByName look across the whole tenancy, and apply the same check on compartment moves.

Low

  1. The if-match check and the write are not atomic. server/oci/notifications/topics.go:164, handler.go:207
    topicIfMatch reads the etag and the update then takes the provider lock separately. In a throwaway test, 8 goroutines sent PUT /topics/{id} with the same current etag. In some rounds 2 of them got 200 where real OCI accepts one and returns 412 to the rest. The run was -race clean, so this is lost-update semantics and not a data race. Fix: pass the expected etag into the provider methods and compare it under m.mu.

  2. There is no etag response header. The SDK maps CreateTopicResponse.Etag and GetTopicResponse.Etag from the etag header, and both come back nil against serve. Only the body field is set. Fix: set etag on topic create, get and update.

  3. A bad page token silently restarts the listing. server/oci/notifications/topics.go:362
    ?limit=1&page=garbage returns 200 with page one. A client with a corrupted cursor can loop forever. This paginate is also a copy of the one in server/oci/vcn/handler.go. Fix: return 400 InvalidParameter for a token that does not parse. A follow-up could move one paginate helper into ocirest so the two handlers share it.

  4. ChangeTopicCompartment returns 202 with an opc-work-request-id. In the SDK, ChangeTopicCompartmentResponse (like DeleteTopic) only has opc-request-id. SDK clients ignore the extra header, so this is harmless. The handler docs should not say real ONS returns a work request here, though.

  5. Confirm and unsubscribe treat protocol as optional. GetConfirmSubscriptionRequest and GetUnsubscriptionRequest mark it mandatory. Fix: return 400 when it is missing (server/oci/notifications/subscriptions.go:282).

  6. Delivery history grows without a limit. providers/oci/notifications/publish.go:120 appends every message to every ACTIVE subscription forever, and snapshot.go persists all of it. A long-running serve with a busy topic grows memory and state-file size without bound. Fix: keep only the last N messages per subscription.

  7. ONS metrics always land in the default compartment with a topicId dimension. providers/oci/monitoring/monitoring.go:88, notifications.go:325 Real oci_notification metrics are scoped to the topic's compartment and keyed by resourceId. Also, PutMetricData now accepts oci_ namespaces from any portable-API caller, not only from sibling mocks.

  8. Nits: the shortTopicID comment says "leading run" but the code takes the trailing 8 characters (notifications.go:344). Development's comments have moved to plain hyphens, but this diff still adds em dashes in docs/services.md and in code comments.

  9. For later, not this PR: development has no OCI gate under --enforce-auth. Unsigned ONS and VCN calls both succeed with it on. When an OCI gate is added, /subscriptions/{id}/confirmation and /unsubscription must be exempt as token-authenticated public endpoints.

Checked

  • Build, vet and go test on the merged tree (PR plus current development) for providers/oci, server/oci, persist, internal/coveragegen and server/wire/ocirest: all green.
  • -race on both notifications packages and providers/oci/monitoring, plus a concurrent publish, list, subscribe and update probe: no races.
  • golangci-lint 2.14 on the changed packages: clean. The 6 goconst hits are in providers/oci/monitoring/query.go, which this PR does not touch.
  • Wiring: provider field, New(), wireMonitoring, DriversFrom and handler registration all in place.
  • go generate is idempotent after the conflict is resolved.
  • Real oci-go-sdk v65.126.1 against serve: CreateTopic, GetTopic, CreateSubscription, UpdateSubscription, GetSubscription, ListSubscriptions, PublishMessage, ListTopics, ChangeTopicCompartment, DeleteSubscription and DeleteTopic all work, and GetTopic returns 404 after the delete.
  • Raw wire against serve: full confirmation flow, unsubscribe link, and topic delete cascading to its subscriptions.
  • Persistence: a topic survives a graceful serve --persist restart.
  • Library and serve use the same provider.
  • Terraform with oracle/oci was not run: the provider builds https://<service>.<region>.<realm-domain> endpoints and has no per-service endpoint override for a plain-HTTP listener, so the real SDK was used instead.

@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.

Implements OCI Notifications against the portable notification driver.

providers/oci/notifications: a Mock over memstore.Store guarded by a single
RWMutex. Topics carry an ocid1.onstopic OCID, a compartment recorded at create
and filtered on every list, a short topic id, lifecycle state, etag and
creation time. Subscriptions carry an ocid1.onssubscription OCID and start
PENDING with a confirmation token; a publish reaches only ACTIVE ones, so an
unconfirmed endpoint receives nothing. Deleting a topic takes its subscriptions
with it, as ONS does.

server/oci/notifications: the /20181201 wire handler for topics,
subscriptions, the two token-authenticated confirmation endpoints, the
changeCompartment and resendConfirmation actions, and PublishMessage on the
topic's own endpoint. Real ONS splits control and data plane by host rather
than by prefix, so a topic reports the requesting origin as its apiEndpoint and
a publish lands back on the same listener. DeleteTopic is asynchronous: it
records a work request and answers 204 with opc-work-request-id.

The OCI-only surface — subscription compartments, tags, metadata, delivery
policy, the confirmation handshake and a topic's lifecycle state — is declared
consumer-side as an Extras interface in the handler, with its value types in
the provider. A driver that does not satisfy it is served 501. Nothing was
added to services/notification/driver.

Input the emulator cannot honour is refused rather than stored unused: message
attributes, defined tags, protocols outside the ONS set, unknown message types
and unsupported sort keys all answer 400 naming what is unsupported.
…n the wire

Adds the hand-written OCI Notifications section docs/oci-conventions.md's
Definition of done requires, splits the wire publish handler out of topics.go
so the feature has one filename in both layers, and raises coverage on both
packages.

PutMetricData is the in-process path a sibling mock emits its own service
metrics on, so it now admits the oci_ namespaces those metrics live under;
ONS's oci_notification counters were being dropped.
…t limits

Mock holds three memstore stores but implemented no snapshot.Snapshottable, so
the persistence-completeness guard failed and a serve stop/start dropped every
topic and subscription. Adds snapshot.go over topics, subscriptions and
deliveries, mirroring providers/oci/vcn's shared storeDump table so Snapshot
and Restore cannot drift.

Deliveries are snapshotted rather than excluded: they stand in for the endpoint
inbox real ONS pushes to and are the only thing Deliveries reads, so dropping
them would report an ACTIVE subscription as having received nothing.

Also brings three responses in line with real ONS: both creates answer 201,
update and delete honour an if-match precondition (412 NoEtagMatch on a stale
etag), and a malformed endpoint or an over-64 KB message body is rejected
naming what is wrong instead of being stored unused.
…c if-match

Wire shapes, checked against oci-go-sdk's ons package:
- Create/Get/UpdateSubscription return ONS's Subscription, which carries the
  delivery policy as a JSON string in deliverPolicy; only the List item
  (SubscriptionSummary) keeps the deliveryPolicy object. Terraform's
  oci_ons_subscription.delivery_policy reads the former.
- PublishMessage reads messageType from the header the SDK sends it in; the
  query parameter stays as a fallback.
- Create, get and update of topics and subscriptions set the etag response
  header the SDK maps Etag from.
- Confirm and unsubscribe require protocol, which ONS marks mandatory.

Behaviour:
- ORACLE_FUNCTIONS subscriptions are created ACTIVE with no token; ONS does not
  ask a function to confirm.
- Topic names are unique across the tenancy, not per compartment.
- CreateTopic, ChangeTopicCompartment, CreateSubscription and
  ChangeSubscriptionCompartment reject a nonexistent compartment with 404,
  via the same SetCompartmentChecker hook VCN uses, wired from Identity.
- if-match is compared under the provider lock on topic update, delete and
  compartment move and on subscription update and delete, so of several
  writers holding one etag exactly one wins; the rest get 412 NoEtagMatch.
- A page token the listing never issued is 400 instead of restarting.
- Delivery history keeps the last 100 messages per subscription.
- ONS metrics land in the topic's compartment keyed by resourceId, through a
  new monitoring PostServiceMetricData. PutMetricData is back to rejecting
  oci_ namespaces for portable callers.

Docs: DeleteTopic and ChangeTopicCompartment return only opc-request-id in real
ONS; CloudEmu's extra work request is now described as such. Em dashes this
branch added are replaced with plain punctuation.
@arunesh-j
arunesh-j force-pushed the feat/oci-notifications branch from 344b484 to a2b74eb Compare October 5, 2026 19:38
@arunesh-j

Copy link
Copy Markdown
Collaborator Author

Rebased onto 770204c3 and addressed this round in a2b74eb7. Every wire shape was checked against the oci-go-sdk ons package rather than against our own handler.

High

1. Rebase. Only docs/coverage/README.md conflicted. Took development's side and re-ran go generate; the only remaining diff is the OCI Notifications cell. The upstream notification driver interface did not change.

Medium

2. deliverPolicy. Create, Get and Update now return the SDK's Subscription shape with the policy as a JSON string in deliverPolicy; List returns SubscriptionSummary with the object in deliveryPolicy. TestUpdateAndMoveSubscription is your repro: it asserts the string on update and get, the object on list, and that neither form leaks into the other response.

3. messageType. Read from the messageType header; the query parameter stays as a fallback. TestPublishReadsMessageTypeFromTheHeader checks NOPE in the header gets 400 and JSON is delivered with type JSON.

4. ORACLE_FUNCTIONS. Created ACTIVE with no token and receives the next publish. Resending a confirmation returns 409, since there is nothing to confirm. Driver and wire tests both added.

5. Compartments and tenancy-wide names. Copied the VCN pattern from development exactly: compartmentExists, SetCompartmentChecker, requireCompartment (404 NotAuthorizedOrNotFound, no-op when unset), wired in server/oci/oci.go. Applied on CreateTopic, ChangeTopicCompartment and CreateSubscription, and also on ChangeSubscriptionCompartment, which had the same gap. TestNotificationsCompartmentCheckIsWiredFromIdentity drives the real Identity provider end to end; development has no equivalent test for the VCN wiring.

topicByName and resolveTopic now search the whole tenancy, so t1 in a second compartment returns 409.

One part I would push back on: "enforce it on compartment moves too". A move never changes the name, so a uniqueness check at move time could never fire. TestTopicNameStaysTakenAcrossACompartmentMove instead shows the name stays taken in the compartment the topic left.

Low

6. Atomic if-match. The etag is now compared under m.mu in the same critical section as the write (UpdateTopicIfMatch, DeleteTopicIfMatch, SubscriptionPatch.IfMatch). The old non-atomic topicIfMatch/subscriptionIfMatch/checkIfMatch are gone. Your 8-goroutine repro is now TestConcurrentTopicUpdatesWithOneEtagHaveOneWinner, asserting exactly {200:1, 412:7}; it held across 100 runs under -race, and with the comparison disabled it fails with {200:8}. ChangeTopicCompartment takes if-match too, since the SDK request carries it.

7. etag header. Set on create, get and update for topics, and also for subscriptions, whose SDK responses read Etag from the same header.

8. Bad page token. Garbage or negative tokens get 400 InvalidParameter on both list routes. The shared ocirest paginate helper is left as the follow-up you suggested.

9. Work-request docs corrected. This also corrects a claim I made in an earlier reply on this PR: I wrote that DeleteTopic returns 204 with an opc-work-request-id as real ONS behaviour. That was wrong - the SDK's DeleteTopicResponse carries only opc-request-id, same as ChangeTopicCompartment. CloudEmu still emits the extra header since SDK clients ignore it, and the docs now describe it as a CloudEmu addition rather than OCI's.

10. protocol required on confirm and unsubscribe. One old test, the wrong-token case in TestUnsubscribeByToken, had been passing for the wrong reason (missing protocol, not wrong token) and now tests what it says.

11. Delivery history capped at the last 100 messages per subscription, copied with slices.Clone so dropped messages are actually freed.

12. Metrics. Now land in the topic's compartment keyed by resourceId. This also closes your other point: last round's change had let any portable PutMetricData caller write oci_ namespaces. monitoring.go is now byte-identical to development; the whole change is metrics.go +18/-3, with a new PostServiceMetricData as the only path allowed to write reserved namespaces. Notifications reaches it through a private interface, so a portable monitoring driver gets no emission.

Note for merge order: #422 also touches providers/oci/monitoring. I will align #422 to this exact change so whichever merges second resolves cleanly.

13. Nits. The shortTopicID comment now says "trailing", and the branch adds zero em dashes relative to development.

14. OCI --enforce-auth gate left alone, as you suggested.

Verification

go build ./...                                          clean
go test ./... (excluding cmd/cloudemu)                  exit 0, 524 packages
go test -race notifications + providers/oci/monitoring  ok
golangci-lint (notifications, server/oci, monitoring)   0 issues
go generate ./...                                       no diff
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 :4616

create topic                              -> etag header present (etag-00000003)
create topic in nonexistent compartment   -> 404
same topic name in a second compartment   -> 409
ORACLE_FUNCTIONS subscription             -> lifecycleState ACTIVE, no confirmationToken
GET subscription after policy update      -> deliverPolicy is a string; deliveryPolicy absent
LIST subscriptions                        -> deliveryPolicy is an object
publish, header messageType: NOPE         -> 400
publish, header messageType: JSON         -> 200
list topics with page=garbage             -> 400
confirm without protocol                  -> 400
update with stale if-match                -> 412

@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.

Nearly there. Every earlier finding is fixed and checks out with the real SDK. One new wire bug is left: resending a subscription confirmation 404s for SDK clients.

Earlier findings

  1. Merge with development: fixed. The branch is on current development (770204c), so it merges cleanly. go generate ./... leaves no diff in docs/coverage.
  2. deliverPolicy on Get: fixed. After UpdateSubscription with a 7200000 ms EXPONENTIAL policy, GetSubscription returns DeliverPolicy as the JSON string, and ListSubscriptions returns the DeliveryPolicy object. There is one leftover on the Update response, listed as a new Low below.
  3. messageType header: fixed. PublishMessage with PublishMessageMessageTypeJson from the SDK returns 200, and a raw messageType: NOPE header returns 400.
  4. ORACLE_FUNCTIONS: fixed. The subscription is created ACTIVE with no confirmationToken in the body.
  5. Compartments and tenancy-wide names: fixed. A bogus compartment gets 404 on CreateTopic, ChangeTopicCompartment, CreateSubscription and ChangeSubscriptionCompartment. t1 in a second compartment gets 409, and the name is still taken in the old compartment after a move. Fair point on the move check, the name can't change there.
  6. Atomic if-match: fixed. 8 concurrent SDK UpdateTopic calls with the same etag gave exactly one 200 and seven 412. A stale if-match on ChangeTopicCompartment and DeleteSubscription also returns 412.
  7. etag header: fixed. Etag is set on CreateTopic, GetTopic, UpdateTopic, CreateSubscription and GetSubscription responses, and it matches the body.
  8. Bad page token: fixed. page=garbage returns 400 on both ListTopics and ListSubscriptions.
  9. Work-request docs: fixed.
  10. protocol required: fixed. Confirm and unsubscribe without protocol both return 400.
  11. Delivery history cap: fixed. After 152 publishes, the saved state file holds exactly 100 messages for each subscription.
  12. Metrics: fixed. ListMetrics in the topic's compartment shows PublishedMessages and DeliveredMessages under oci_notification with a resourceId dimension. A portable PostMetricData into oci_notification is rejected with 400 again.
  13. Nits: fixed. The comment now says "trailing", and the diff adds no em dashes.
  14. OCI auth gate: left for later, as agreed.

New findings

Medium

  1. ResendSubscriptionConfirmation is served on the wrong path. server/oci/notifications/subscriptions.go:24 and :36, handler.go:14, docs/services.md:1520
    The SDK sends POST /20181201/subscriptions/{id}/resendConfirmation (ons_notificationdataplane_client.go:620). It has no /actions/ segment. The handler only routes /subscriptions/{id}/actions/resendConfirmation.
    Repro: create a PENDING EMAIL subscription, then POST /20181201/subscriptions/{id}/resendConfirmation. You get 404, and /actions/resendConfirmation gets 200. ResendSubscriptionConfirmation from oci-go-sdk returns 404 for every subscription.
    Fix: route rt.Sub == "resendConfirmation" with no action to resendConfirmation. Update the path in the tests and in docs/services.md, and add an SDK-path wire test. While there, set the etag header on that response too, since it returns a Subscription.

Low

  1. The UpdateSubscription response body is the wrong shape for the SDK. server/oci/notifications/subscriptions.go:179
    In the SDK, UpdateSubscriptionResponse decodes its body as UpdateSubscriptionDetails, which reads the policy from the deliveryPolicy object. It does not decode a Subscription. My earlier note put Update in the Subscription group, and that was wrong. Right now resp.DeliveryPolicy is nil after a successful update. Get and List are correct.
    Fix: have the update response also send deliveryPolicy as an object. Keeping deliverPolicy alongside it is harmless.

  2. ChangeSubscriptionCompartment ignores if-match. server/oci/notifications/subscriptions.go:275
    ChangeSubscriptionCompartmentRequest carries if-match, the same as the topic move. A move with IfMatch: "stale" returns 204 and moves the subscription.
    Fix: pass ifMatch(r) into the provider and check it under m.mu, the same way as the other writes.

  3. golangci-lint canonicalheader at server/oci/notifications/handler.go:257: use "ETag". CI only reports lint findings as warnings, but this is the only finding the PR adds.

Checked

  • The branch is on current development (770204c) and merges cleanly. go generate ./... leaves no diff.
  • go build ./... is green. vet and test pass on providers/oci, server/oci, persist, internal/coveragegen and server/wire/ocirest.
  • -race -count=3 on both notifications packages and providers/oci/monitoring: clean.
  • golangci-lint 2.14 --new-from-rev=origin/development: one finding, item 4 above.
  • Real oci-go-sdk v65.126.1 against serve -providers oci: every earlier repro listed above, plus GetConfirmSubscription, ListMetrics and DeleteTopic cascading to its subscriptions.
  • serve --persist graceful restart: topics, subscriptions and the capped delivery history all come back.
  • Not specific to this PR: after a restore the shared idgen counter starts again from a low number. A new ONS subscription took the same OCID as a restored one and overwrote it. Every service uses that counter, so the fix belongs in idgen or persist. It should be tracked separately.

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 Notifications: topics and subscriptions

2 participants