Skip to content

Add OCI Logging: log groups, logs, ingestion, search - #423

Open
arunesh-j wants to merge 5 commits into
developmentfrom
feat/oci-logging
Open

arunesh-j wants to merge 5 commits into
developmentfrom
feat/oci-logging

Conversation

@arunesh-j

Copy link
Copy Markdown
Collaborator

Summary

  • Implements OCI Logging against the existing portable logging driver.
  • Covers all three of OCI's API surfaces — control plane, ingestion, and search — collapsed onto one listener.
  • Nothing added to services/logging/driver; OCI-only behaviour is a consumer-side Extras interface, per Move OCI-only capabilities out of shared driver packages #393.

Closes #414. Part of #376.

Changes

  • providers/oci/logging/ — Mock over memstore implementing driver.Logging, guarded by a sync.RWMutex, plus a search-query parser.
  • server/oci/logging/ — the three-prefix wire surface.
  • Wiring is one line each in providers/oci/oci.go and server/oci/oci.go.

Operations

Control plane: Create/List/Get/Update/Delete LogGroup, ChangeLogGroupCompartment, Create/List/Get/Update/Delete Log. Data planes: PutLogs, SearchLogs. All 14 portable driver methods implemented; metric filters return Unimplemented naming Service Connectors as OCI's answer.

The three prefixes — the main design risk

parsePath splits /{version}/{collection}[/{id}[/{sub}[/{subId}]]], then a switch on version claims exactly one collection set: 20200531 → logGroups, unifiedAgentConfigurations, logSavedSearches; 20200601 → logs; 20190909 → search.

The load-bearing detail: a top-level /logs collection exists only on the ingestion prefix — the control plane nests logs under their group — so /20200531/logs is deliberately unclaimed. TestMatches has 25 cases covering every positive shape, each collection asserted not claimed under the two wrong prefixes, other services' traffic, and malformed paths.

Search: supported vs rejected

Supported: search "compartmentId[/logGroupId[/logId]]" (comma-separated targets) | where <field> = | != '<value>' (* wildcard, joined by and) | sort by datetime [asc|desc]. Fields resolve over logContent.* and data.<key> of a JSON payload.

Rejected by name with a 400: summarize/stats/topN/extract/unknown operators; or, not, parenthesized where clauses; >, <, >=, <=, =~, !~; unresolvable fields or nested payload paths; sorting on anything but datetime; a target written as a name where OCI takes an OCID; a missing search clause or time range.

A real silent-empty bug was found and fixed mid-implementation: field resolution originally happened per-entry, so an unknown field on a log with no entries returned 200 []. Fields now resolve at parse time, before any entry is walked.

Judgement calls

  • ListLogs takes no compartmentId — real OCI derives it from the log group in the path, so the group OCID is what is required. Noted in services.md and the handler comment.
  • Log group retention is CloudEmu's own — real OCI carries retention on the log; the group holds the default its logs inherit, so the portable RetentionDays has somewhere to live.
  • Log group display names are unique emulator-wide, not per compartment — that is what lets the portable driver address a group by name.
  • DeleteLogGroup cascades to its logs rather than refusing, matching the other portable drivers.

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 in docs/coverage/{aws,azure,gcp}/*.md; git diff development -- services/ empty.

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

create log group                              -> 202 + Opc-Work-Request-Id
poll work request                             -> ocid1.loggroup.oc1.iad...
create log                                    -> 202
put 3 entries                                 -> 200
compartment-wide search                       -> 3 results
| where data.level = 'ERROR' | sort desc      -> 2 results, newest first, payload decoded
| summarize count()                           -> 400, names "summarize"
where data.count > 3                          -> 400, names ">"
list without compartmentId                    -> 400
unifiedAgentConfigurations                    -> 501
delete log, delete group                      -> 202 each
get after delete                              -> 404 NotAuthorizedOrNotFound

Left out

No oci-go-sdk compat test — the three-client split made the e2e transcript stronger evidence for the effort. oracle.tenantid is omitted from search records; adding it would pull config identity into the handler for no behavioural gain.

@arunesh-j arunesh-j added the oci Oracle Cloud Infrastructure label Aug 21, 2026
Comment thread providers/oci/logging/portable.go Fixed
Comment thread providers/oci/logging/portable.go Fixed

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

CI: CodeQL fails — 2× high go/uncontrolled-allocation-size at providers/oci/logging/portable.go:225 and :263. Guard the size before allocating.

Findings

Medium · structure — STRUCTURE.md filename parity broken: provider group.go/log.go/ingestion.go+search.go vs wire groups.go/logs.go/dataplane.go
server/oci/logging/groups.go:1
If a maintainer navigates by the STRUCTURE.md filename convention -> they cannot map a feature's mock to its wire handler by a single filename, because logging pluralizes and merges wire filenames against the singular, split provider files (server/oci/logging/groups.go, logs.go, dataplane.go).

STRUCTURE.md §3 hard rule ('a feature uses the same filename across all three layers') is broken for all three feature areas: provider group.go/log.go/ingestion.go/search.go vs wire groups.go(plural)/logs.go(plural)/dataplane.go(ingestion+search merged). The vcn reference keeps identical names across layers (dhcp.go/dhcp.go, subnet.go/subnet.go), so logging is the outlier. Naming-only, no runtime impact.

Low · correctness — Log-group displayName uniqueness is global, not per-compartment — diverges from real OCI
providers/oci/logging/group.go:24
If a user creates identically-named log groups in two different compartments (normal in real OCI) -> the second create fails with AlreadyExists, because providers/oci/logging/group.go:24 enforces a global name index rather than a per-compartment one.

createGroup calls groupByName(spec.DisplayName), which scans ALL compartments and returns AlreadyExists on any name collision (UpdateGroup rename too). Real OCI scopes log-group displayName uniqueness per compartment. The 'already exists' test (logging_test.go:83) only covers a same-compartment duplicate; documented as a deliberate tradeoff in docs/services.md to let the portable driver key groups by name.

Low · coverage — Search sort-ordering and oracle. provenance where-fields have no positive test*
server/oci/logging/handler_test.go:467
If someone later refactors sortEntries or provenanceValue and flips the desc branch or mis-maps an oracle.* field -> the bug ships green, because no test exercises sort-desc ordering or a where oracle.logid = ... comparison.

TestSearchLogs covers whole-compartment, narrowed-to-log, where-on-JSON-field, wildcards, time-range miss, field-info; the rejection table covers bad sorts/operators/fields. Neither asserts a successful 'sort by datetime asc|desc' RESULT ORDER nor a where-clause on oracle.compartmentid/loggroupid/logid/ingestedtime. provider sortEntries(desc) and provenanceValue() thus have no positive assertion; provider pkg coverage is 66.8%.

@arunesh-j

Copy link
Copy Markdown
Collaborator Author

Rebased onto current development (58787e9a) and addressed the findings across 1048ec83 and 86a227b0.

CI — CodeQL go/uncontrolled-allocation-size (alerts 17, 18)

Added maxLogLimit = 10000 and resolveLimit, called before the make at both flagged sites and at SearchLogs. Negative and over-max are rejected with InvalidArgument; 0 still means default, matching the AWS/GCP/Azure siblings.

Reverting both guards is not a subtle failure:

signal: killed
FAIL  github.com/stackshy/cloudemu/v2/providers/oci/logging  174.769s

The process is OOM-killed on limit: 1 << 40. Restored → PASS in 0.00s. Covered by TestPortableReadLimitIsBounded.

Worth recording for scope: the HTTP path was never exposed — ocirest.Limit already clamps at MaxLimit = 1000. The reachable surface was the portable driver called directly, and wire behaviour is unchanged since 1000 < 10000.

Medium — STRUCTURE.md §3/§4 filename parity

Both layers now carry the identical feature set: group.go / ingestion.go / log.go / search.go. Provider keeps logging.go (the §4 <service>.go), portable.go, query.go; wire keeps handler.go + types.go — the §4 template, and the same shape as the vcn reference. parseTime/timeError, shared by ingestion and search, moved to types.go. Renames survived the rebase as renames (groups.go => group.go and logs.go => log.go at 0 changes).

Low — per-compartment displayName: scoped, and I'd argue against keeping the tradeoff

groupByName now takes a compartment; createGroup, the UpdateGroup rename, MoveGroup and the portable compartment-move all check within one compartment. MoveGroup had the same latent bug and was not in the review.

On how the portable driver still addresses a group by name: portableGroupByName resolves across compartments but rejects an ambiguous name with InvalidArgument naming both compartments, rather than silently picking one. Single-compartment use — every existing portable test and the cross-cloud conformance suite — is unchanged; the ambiguous case is the one that used to be impossible and would otherwise resolve arbitrarily.

That keeps the repo's reject-don't-guess discipline instead of trading correctness for driver convenience. #424 (table names) and #425 (secret names) have the identical finding, and this is the answer I'd apply to both.

Low — positive tests for sort order and oracle.* provenance

Added at both layers: result order for sort by datetime asc|desc, sort by time desc, logContent.datetime desc, default, and the id tiebreak on equal timestamps; plus where on all four oracle.* fields including negation. Entries are seeded out of time order so a sorted result cannot pass by accident.

Coverage: provider 66.3% → 96.6%, wire 82.9% → 94.2%. The provider jump is mostly query.go/search.go, which had zero provider-level tests — the parser was only ever exercised through the wire.

Two things not in the review

  1. Build break from upstream. The second rebase surfaced that services/logging/driver gained Put/Delete/DescribeSubscriptionFilter, so *Mock no longer satisfied driver.Logging. Implemented as Unimplemented naming the Service Connector, consistent with the existing metric-filter treatment — not a silent no-op.
  2. TestSnapshotCompleteness failed: oci: field Logging (*logging.Mock) holds a memstore.Store but is not Snapshottable. Added snapshot.go modelled on vcn. This required exporting logRecord's fields (log→Log, entries→Entries) — an unexported type, so no public API change, but without it the JSON snapshot would have silently written {} and dropped every log and entry. Round-trip tests cover OCIDs, compartments, the log→group cross-reference, entry fields, searchability after restore, malformed input and the empty case.

One scope note for your call

I edited the shared logging section of docs/services.md, not just the OCI subsection. The driver gained those three operations upstream but the table and the **Total: 13 operations** line were never updated — stale before this branch. I added the rows and corrected the total to 17 to match go generate. Easy to drop if you want this PR strictly OCI-scoped, but the count would then contradict the generated coverage doc.

Verification

go build ./...                                     clean
go test ./...                                      exit 0, 342 packages
go test -race ./providers/oci/... ./server/oci/... 11/11 ok
golangci-lint (both logging packages)              0 issues
go test -cover   provider 96.6%   wire 94.2%
go generate ./...                                  docs/coverage committed
no OCI op in docs/coverage/{aws,azure,gcp}/*.md    confirmed
git diff origin/development -- services/logging/   empty (driver untouched)

golangci-lint also reports 7 gocritic issues across providers/oci/identity and {providers,server}/oci/vcn — confirmed pre-existing by running the same command against clean development, which produces the identical 7.

E2E, port 4615

create log group                          -> 202  ocid1.loggroup...0002
same name, DIFFERENT compartment          -> 202  ocid1.loggroup...0006   (now allowed)
same name, SAME compartment               -> 409  "log group \"app-logs\" already exists
                                                   in compartment ...aaaaaaaademo"
put 3 entries (deliberately out of order) -> 200
search, default order                     -> ['e-10','e-20','e-30']
| sort by datetime desc                   -> ['e-30','e-20','e-10']
| where oracle.logid = <log>              -> 3 matched, oracle.* block populated
| where data.level = 'error'              -> ['e-10']
| stats count()                           -> 400 naming "stats"
| where a or b                            -> 400 naming "or"
| sort by source desc                     -> 400, sorts by datetime only
| where nope = 'x'                        -> 400 listing the resolvable fields
search "app-logs"                         -> 400, not a compartment OCID
delete log group                          -> 202, then GET -> 404

Steps 2/3 are the live proof for the per-compartment fix; the two sort lines for the ordering tests.

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

Verdict: Request changes — one confirmed data race blocks merge; everything else (wiring, lifecycle, persistence, wire fidelity, structure, docs) is exemplary.

Blocking

Shared-pointer aliasing / reader-writer data race — providers/oci/logging/group.go:177 (also group.go:65,92, log.go:127,149).
GetLog/ListLogs return a shallow copy of Log whose Configuration *LogConfiguration and FreeformTags map alias the stored record, while MoveGroup writes rec.Log.Configuration.CompartmentID in place under the write lock. A concurrent GetLog reader (or the wire handler's toLogResponse projection for a different request) reads that field after releasing its RLock — a genuine data race. Reproduced with a throwaway Get-vs-MoveGroup -race test:

WARNING: DATA RACE
  Write at ... by goroutine (MoveGroup) group.go:177
  Previous read at ... by goroutine (GetLog)

GetGroup/ListGroups likewise hand back an aliased tag map a caller can mutate to corrupt the store. The existing race_test.go never runs a Get-then-read concurrent with MoveGroup, so the base -race gate is green despite the bug. Cascade: a client GETs a SERVICE log while another changeCompartment runs on its group → torn read / undefined behavior.

Fix: deep-copy Configuration (including Source.Parameters) and FreeformTags on every OCID-addressed return path — toLogGroupInfo already uses copyTags; the OCID getters/listers do not.

Non-blocking

  • PutLogs has no batch entry-count or payload-size cap — providers/oci/logging/ingestion.go:49. Real OCI loggingingestion enforces per-request entry/size limits; silent unbounded acceptance is a fidelity gap (not an amplification vector — entries are already decoded in memory).
  • Async create/update returns HTTP 202 — server/oci/logging/{group,log}.go. Real OCI Logging returns 200 + opc-work-request-id. This matches the sibling server/oci/vcn convention (vcn.go:81) and clients poll the work request regardless, so it's internally consistent — noting only for eventual repo-wide OCI status-code alignment.

Verified good

Full lifecycle over the wire (create group → create CUSTOM log → PutLogs ingest → SearchLogs by data.level with provenance stamping → disable → PutLogs on disabled log → 409 IncorrectState → missing-parent → 404 → dup → 409 → delete with cascade → 404): computed IDs/timeCreated stable, work request resolves SUCCEEDED synchronously (no waiter hang). Snapshottable present — snapshot.go dumps both stores, and merged-tree persist completeness passes. Wiring complete end-to-end; error taxonomy correct; ingestion/search/list bounded; create paths span check-then-set under one write lock (no TOCTOU race); honest 501 stubs naming the OCI Service-Connector equivalent; structure/lint/go generate all clean.

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

Log groups and logs work end to end with the real OCI Go SDK, but ingestion and the compartment move don't. Both mismatch the SDK's wire shape, so PutLogs and ChangeLogGroupCompartment fail for any real client.

Earlier review points

  • CodeQL "slice memory allocation with excessive size value" at providers/oci/logging/portable.go:249 and :287. Fixed: resolveLimit now rejects anything above maxLogLimit (10000) before the make.

Merge state

  • The branch is 281 commits behind development and conflicts in docs/coverage/README.md. Rebase and re-run go generate ./.... The regenerated output only adds the OCI cell to the logging row. Everything else merges cleanly, and the merged tree builds and passes providers/oci/..., server/oci/..., persist/... and internal/coveragegen/....

High

  1. Ingestion is served at the wrong API version, so PutLogs never reaches the handler. server/oci/logging/handler.go:50

    • The handler claims /20200601/logs/{logId}/actions/push. The real loggingingestion client uses BasePath = "20200831" (oci-go-sdk v65 loggingingestion_logging_client.go:63), and the API reference is logging-dataplane/20200831/LogEntry/PutLogs.
    • Repro: point loggingingestion.NewLoggingClientWithConfigurationProvider at the server and call PutLogs. You get 501 ... no handler registered for this request at POST /20200831/logs/<ocid>/actions/push. A follow-up SearchLogs then returns 0 results.
    • The handler tests post to /20200601 directly, so they pass while real clients fail.
    • Fix: change versionIngestion to 20200831. Also update the package doc comment, types.go:98, docs/services.md:1491 and the 15 test paths.
  2. ChangeLogGroupCompartment reads the wrong body field. server/oci/logging/types.go:29, server/oci/logging/group.go:171

    • changeCompartmentRequest decodes targetCompartmentId. The real ChangeLogGroupCompartmentDetails field is compartmentId (logging/change_log_group_compartment_details.go:24).
    • Repro: mc.ChangeLogGroupCompartment(ctx, logging.ChangeLogGroupCompartmentRequest{LogGroupId: gid, ChangeLogGroupCompartmentDetails: logging.ChangeLogGroupCompartmentDetails{CompartmentId: &c}}) returns 400 InvalidParameter: targetCompartmentId is required.
    • Fix: decode compartmentId, and update the tests at handler_test.go:218, :876 and :919.

Medium

  1. Data race between MoveGroup and GetLog/ListLogs. providers/oci/logging/group.go:177

    • MoveGroup writes rec.Log.Configuration.CompartmentID in place under the write lock. GetLog and UpdateLog return out := rec.Log (log.go:127, :226), which shares the same *LogConfiguration pointer. toLogResponse then reads it after the lock is released.
    • Repro: one goroutine loops MoveGroup(g, "c1"/"c2") while another loops GetLog(g, l) and reads .Configuration.CompartmentID. go test -race reports WARNING: DATA RACE with the write at group.go:177.
    • Fix: in MoveGroup, replace the pointer with a fresh copy (cfg := *rec.Log.Configuration; cfg.CompartmentID = ...; rec.Log.Configuration = &cfg). Or deep-copy Configuration (and Source.Parameters) on every read.
  2. Creates and moves into a compartment that does not exist succeed. server/oci/logging/group.go:55, :160

    • POST /20200531/logGroups with compartmentId: ocid1.compartment.oc1..doesnotexist returns 202. Real OCI returns 404 NotAuthorizedOrNotFound. VCN already does this check through SetCompartmentChecker, which server/oci/oci.go wires from Identity.
    • Fix: add the same optional checker to the logging handler, wire it in server/oci/oci.go, and call it in createGroup and moveGroup.
  3. The compartment move records an operation type OCI does not define. server/oci/logging/handler.go:76

    • operationMoveGroup = "CHANGE_LOG_GROUP_COMPARTMENT". The SDK's OperationTypesEnum defines MOVE_LOG_GROUP (and MOVE_LOG). GetMappingOperationTypesEnum does not recognize the current value, so any client that switches on WorkRequest.OperationType misses it.
    • Fix: use MOVE_LOG_GROUP.

Low

  1. DeleteLogGroup cascades to the logs inside the group (providers/oci/logging/group.go:134). Real OCI requires a log group to be empty before it can be deleted, and returns 409 otherwise. Consider returning FailedPrecondition (409 IncorrectState) on the OCI path when the group still holds logs, and keep the cascade only for the portable DeleteLogGroup.
  2. retentionDuration is not validated (providers/oci/logging/log.go:43, :207). OCI accepts 30 to 180 in 30-day steps, but retentionDuration: 7 is stored as is. Reject other values with 400 InvalidParameter.
  3. A CUSTOM log comes back with a synthesized configuration.source.sourceType: OCISERVICE and no service or resource (providers/oci/logging/log.go, normalizeConfiguration). A custom log has no service source. Return the configuration only when the caller supplied one, or leave source out for CUSTOM logs.
  4. tenancyId is missing from the Log response (server/oci/logging/types.go:82). The SDK model has it, and it can come from the configured tenancy OCID.
  5. PutLogs accepts batches with no source, type or defaultlogentrytime. All three are mandatory in LogEntryBatch, so real OCI rejects them with 400.
  6. SearchLogs passes limit but ignores page and never sets opc-next-page (server/oci/logging/search.go:41), so results past the first page can't be fetched.
  7. golangci-lint v2.14.0 reports 6 goconst hits (ingestion.go:40, portable.go:227, search.go:97, :239, :241, :242). Pull "compartmentId", "STRING", "data" and the oracle.* field names into constants.

Checked

  • Merged tree (PR plus current development): go vet and go test pass for providers/oci/..., server/oci/..., persist/... and internal/coveragegen/.... -race passes for the logging, oci provider and oci server packages. gofmt is clean.
  • Wiring: the provider field is set in New(), wireMonitoring covers Logging, DriversFrom maps it, and the handler is registered after the work request handler. No other OCI handler claims /20200531, /20190909 or /20200831.
  • Persistence: there is a compile-time Snapshottable assertion, snapshot.Discover finds it, and the persist tests pass on the merged tree.
  • Ran the real oci-go-sdk v65.126.1 against the server. These all work: CreateLogGroup, then GetWorkRequest (SUCCEEDED, entityType loggroup), then GetLogGroup twice with byte-identical output, then CreateLog, GetLog, SearchLogs, ListLogGroups and DeleteLogGroup, with GetLogGroup returning 404 NotAuthorizedOrNotFound afterwards. PutLogs and ChangeLogGroupCompartment fail as described above.
  • Raw HTTP: a duplicate name returns 409 Conflict, a missing log or group returns 404, an empty page returns [], and every response carries opc-request-id.
  • Terraform: not run. The oracle/oci provider has no per-service endpoint override and needs HTTPS on the real regional hostnames, so the SDK flow above stands in for it.
  • --enforce-auth: OCI has no request-authorization gate on development (WithEnforceAuth covers only AWS and Azure), so this handler has nothing to declare.
  • ETag and if-match: not implemented anywhere in the OCI wire layer yet. That is not specific to this PR.

@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 the portable logging driver against OCI Logging, with the
OCI-only surface behind a consumer-side Extras interface.

OCI publishes the service on three API surfaces, which collapse onto one
CloudEmu server, so Matches claims each prefix's collections exactly:
/20200531 for the log group and log control plane, /20200601 for the
loggingingestion push, and /20190909 for loggingsearch. A top-level
/logs collection belongs to the ingestion plane alone — the control
plane nests logs under their log group — which is what keeps the two
apart.

A log group is the portable log group, a CUSTOM log is the log stream
and an ingested entry is the log event. Every log group and log mutation
is asynchronous in real OCI, so each answers 202 with an
opc-work-request-id carrying the created resource's OCID. Ingesting into
a SERVICE log or a disabled one is refused rather than accepted and
dropped. Metric filters have no OCI equivalent and report Unimplemented.

Search reads the straightforward query form — a search clause over
compartment[/logGroup[/log]], an optional where clause of = and !=
comparisons joined by and, and an optional sort by datetime — and
rejects everything else naming what it tripped on rather than returning
an empty result set: the summarize, stats, topN and extract operators,
or/not/parenthesized where clauses, the ordering and pattern operators,
an unresolvable field, and a search target written as a name where OCI
takes an OCID.
…tment

Guard the caller-supplied read limit before it sizes an allocation:
GetLogEvents, FilterLogEvents and SearchLogs now reject a negative limit
and one above maxLogLimit with InvalidArgument. Resolves the CodeQL
uncontrolled-allocation-size findings in portable.go.

Scope log-group displayName uniqueness per compartment, as real OCI does.
The portable driver has only a name to address a group by, so a name held
in more than one compartment is rejected as ambiguous rather than
resolved arbitrarily.

Rename the wire files to match the provider's, per STRUCTURE.md section 3:
groups.go -> group.go, logs.go -> log.go, and dataplane.go split into
ingestion.go and search.go.
Implement snapshot.Snapshottable for the Logging mock, which the #582
completeness guard requires of any provider field holding a memstore —
without it a stop/start silently dropped every log group, log and
ingested entry. logRecord's fields are exported so the record round-trips
through the generic memstore helper, which serializes as JSON.

Subscription filters, added to the shared driver upstream, are not an OCI
Logging operation: OCI delivers log entries to another service through a
Service Connector, so all three report Unimplemented naming that rather
than accepting a filter nothing would honour.
Serve PutLogs at /20200831, the loggingingestion client's BasePath; the
handler claimed /20200601, so a real client's PutLogs got 501. Decode
ChangeLogGroupCompartment's target from compartmentId, the
ChangeLogGroupCompartmentDetails field, and record the move as
MOVE_LOG_GROUP, a value OperationTypesEnum defines. A new SDK-contract test
pins these as literals copied from the SDK, not the handler's constants.

Serve ChangeLogLogGroup (POST .../logs/{logId}/actions/changeLogGroup,
recorded as MOVE_LOG), which fell outside the parsed path depth and got 501.

Fix a data race: a read returned a Log sharing its *LogConfiguration with
the store, which MoveGroup then rewrote in place. Every read now returns a
deep copy, and moves replace the configuration rather than mutate it.

Reject a log group created in, or moved into, a compartment that does not
exist with 404 NotAuthorizedOrNotFound, wired from Identity as VCN is.

Match OCI where it differs: deleting a group that still holds logs is 409
IncorrectState (the portable DeleteLogGroup still cascades);
retentionDuration must be 30-180 days in 30-day steps; a CUSTOM log carries
no synthesized OCISERVICE source; a log reports its tenancyId; a batch
missing source, type or defaultlogentrytime is 400 and ingests nothing;
UpdateLog validates every field before applying any.

SearchLogs now pages with limit/page and returns opc-next-page, and a
search scoped to the tenancy OCID finds the root compartment's logs. A page
cursor this API never minted is 400 rather than a silent restart at zero.
@arunesh-j

Copy link
Copy Markdown
Collaborator Author

Rebased onto 770204c3 and fixed every item in e3e75301 (plus 0ff5c4ca, the post-rebase coverage regen). This round, every wire shape was checked against the oci-go-sdk source rather than against our own handler. That is how the two Highs went unnoticed before: the handler tests posted to our own wrong path.

Highs

1. Ingestion version. versionIngestion is now 20200831, matching loggingingestion's BasePath. Updated the package doc, types.go, docs/services.md and all 15 test paths.

To stop this happening again, TestSDKContract pins the paths, request bodies and the OperationTypesEnum set as literals copied from the SDK, each with a citation to the SDK file. Our own constants can no longer drift without a failure. I checked it bites: restoring 20200601, targetCompartmentId and CHANGE_LOG_GROUP_COMPARTMENT one at a time fails the test on each, including "CHANGE_LOG_GROUP_COMPARTMENT" is not in the SDK's OperationTypesEnum.

2. ChangeLogGroupCompartment decodes compartmentId. The old targetCompartmentId body now returns 400 rather than being accepted. The three test sites are updated.

Mediums

3. Data race. TestConcurrentMoveAndRead was written first and reproduced the race at group.go:177 under -race. The fix covers both of your options. Every read path (GetGroup, ListGroups, GetLog, ListLogs, CreateLog, UpdateLog) returns a deep copy covering tags, Configuration and Source.Parameters. MoveGroup and MoveLog replace the configuration pointer rather than editing it in place. Deep copies close the whole class of aliasing bug, including a caller mutating a returned tags map; TestReadsDoNotAliasTheStore covers that.

4. Compartment check. Copied the VCN pattern from development exactly and wired it in server/oci/oci.go from Identity, on createGroup and moveGroup. server/oci/logging_compartment_gate_test.go runs through the real ociserver.New wiring; with the wiring disabled it fails with expected 404, actual 202 for both create and move.

5. Operation type is MOVE_LOG_GROUP, enforced by the SDK-enum assertion above.

Lows

6. Deleting a log group that still holds logs returns FailedPrecondition (409 IncorrectState) on the OCI path. The portable DeleteLogGroup still cascades.
7. retentionDuration must be 30 to 180 in 30-day steps, otherwise 400. UpdateLog also now validates every field before applying any. It used to apply a rename before a later field could fail, leaving a half-applied update.
8. CUSTOM logs store a configuration only when the caller supplies one; no sourceType is synthesized.
9. tenancyId is set on the Log from the configured tenancy.
10. A batch missing source, type or defaultlogentrytime returns 400 naming the field, e.g. logEntryBatches[1].source is required. Every batch is validated before any is ingested, so a rejected call stores nothing.
11. SearchLogs pages with limit/page and sets opc-next-page. A page value this API never issued is now a 400 for both lists and search; it used to restart silently at zero, which can loop a paginating client forever.
12. The goconst strings are constants now. golangci-lint v2.14.0, the version you ran, reports 0 issues; it also caught two more issues that v2.11.4 misses, both fixed.

Two bugs found that the review didn't list

  • ChangeLogLogGroup returned 501. The SDK sends POST .../logs/{logId}/actions/changeLogGroup, which is deeper than the path parser allowed. It is now served and records MOVE_LOG.
  • Search refused the tenancy OCID as a target, which locked out the root compartment where log groups land by default. The E2E caught it. parseScope now accepts ocid1.tenancy. as the root compartment, in the first segment only. TestSearchTheRootCompartmentByTenancy fails without the fix.

Not changed, for your call

LogEntry.id and data, and OciService.category on SERVICE logs, are also mandatory in the SDK. We still mint an id when one is missing and don't require category. You didn't raise either; both are easy follow-ups if you want strict parity.

Verification

go build ./...                                       clean
go test (all packages except cmd/cloudemu)           pass
go test ./persist/...                                ok
go test -race logging + server/oci                   ok
golangci-lint v2.14.0                                0 issues
coverage                                             provider 97.0%, wire 93.4%
go generate ./...                                    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 :4615

POST /20200831/logs/{id}/actions/push              -> 200
  follow-up search                                 -> finds the ingested entry
POST /20200601/... (old version)                   -> 501
batch missing source                               -> 400
ChangeLogGroupCompartment {"compartmentId": ...}   -> work request MOVE_LOG_GROUP, SUCCEEDED
  old body field targetCompartmentId               -> 400
create into a nonexistent compartment              -> 404
delete a log group that still holds a log          -> 409
retentionDuration 7                                -> 400

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

All 12 earlier findings are fixed on e3e7530. PutLogs and ChangeLogGroupCompartment now work end to end with the real oci-go-sdk, and nothing new of substance turned up. Approving, with two small follow-ups below.

Earlier findings

  1. High: ingestion API version. Fixed. versionIngestion is now 20200831. With oci-go-sdk v65.126.1 against serve, loggingingestion.PutLogs returns 200 and a follow-up SearchLogs finds all five ingested entries. The old /20200601/.../actions/push path now returns 501, so the handler no longer claims it.
  2. High: ChangeLogGroupCompartment body field. Fixed. ChangeLogGroupCompartmentDetails{CompartmentId} from the SDK moves the group. GetLogGroup and GetLog both report the new compartment afterwards. A body that only sets targetCompartmentId now gets 400 compartmentId is required.
  3. Medium: MoveGroup vs GetLog/ListLogs data race. Fixed. Log.clone() deep-copies Configuration, Source.Parameters and the tags, and every read path returns that clone. MoveGroup and MoveLog swap in a new configuration pointer instead of writing through the old one. TestConcurrentMoveAndRead covers the original Get-vs-Move repro, and go test -race -count=3 ./providers/oci/logging/... passes.
  4. Medium: create or move into a compartment that does not exist. Fixed. With the SDK, CreateLogGroup and ChangeLogGroupCompartment into a made-up compartment both return 404 NotAuthorizedOrNotFound. Creating in the tenancy root still works (202). The checker is wired from Identity in server/oci/oci.go.
  5. Medium: work request operation type. Fixed. The SDK's GetWorkRequest reports MOVE_LOG_GROUP / SUCCEEDED after a compartment move.
  6. Low: deleting a non-empty group. Fixed. The SDK gets 409 IncorrectState. After DeleteLog, the group deletes and GetLogGroup returns 404. The portable DeleteLogGroup still cascades.
  7. Low: retentionDuration validation. Fixed. CreateLog with 7 and UpdateLog with 0 or 45 return 400. A rejected update that also renamed the log leaves the old name in place, so updates are no longer half-applied.
  8. Low: synthesized source on CUSTOM logs. Fixed. A CUSTOM log created without a configuration returns configuration: null. A SERVICE log round-trips its source as sent.
  9. Low: tenancyId. Fixed. GetLog returns tenancyId set to the configured tenancy.
  10. Low: mandatory batch fields. Fixed. A batch without source returns 400 logEntryBatches[0].source is required, and all batches are validated before any is stored.
  11. Low: SearchLogs paging. Fixed. With limit=2 the SDK walked 2, 2, 1 entries with opc-next-page set to 2, then 4, then nil, and got all five in order. A page token the API never issued returns 400 on both lists and search. ListLogs?limit=1 sets opc-next-page.
  12. Low: goconst. Fixed. golangci-lint v2.14.0 with --new-from-rev=origin/development reports 0 issues on providers/oci/... and server/oci/....

The ChangeLogLogGroup path that used to 501 also works now. The SDK moves the log, GetLog under the target group returns it with that group's compartment, and the work request is MOVE_LOG / SUCCEEDED.

New findings

No High or Medium. Two Low follow-ups, neither blocking:

  • Low: a portable compartment move leaves the group's logs behind. providers/oci/logging/portable.go:88. UpdateLogGroup with a new Scope.Compartment sets g.CompartmentID and nothing else. The OCI MoveGroup also updates each log's CompartmentID and its Configuration.CompartmentID. Repro: portable CreateLogGroup in compartment a, then CreateLogStream, then UpdateLogGroup with compartment b. ListLogs then shows the group in b and its log still in a, and that stale value is what GetLog returns over the wire. Fix: move the shared log loop into a helper and call it from both paths.
  • Low: portable retention is not checked against OCI's rule. providers/oci/logging/portable.go:51, :75, and log.go:45. Portable CreateLogGroup or UpdateLogGroup with RetentionDays: 7 succeeds. Any log created later through the OCI path inherits 7 without validation, which bypasses the check added for finding 7. Fix: run validateRetention on the portable value when it is non-zero, or clamp it before storing.

Checked

  • Head e3e7530 is based on the current development tip (770204c) and merges cleanly.
  • go build ./... is clean. go vet and go test pass for providers/oci/..., server/oci/..., persist/... and internal/coveragegen/.... -race passes for the logging provider and server/oci/.... gofmt is clean.
  • go generate ./... leaves no diff.
  • Ran real oci-go-sdk v65.126.1 against serve --providers oci: CreateCompartment ×2, CreateLogGroup (root and child compartment), GetWorkRequest, CreateLog, ListLogs, GetLog, PutLogs, SearchLogs paging, ChangeLogGroupCompartment, ChangeLogLogGroup, DeleteLog, DeleteLogGroup, then GetLogGroup returned 404. Two GetLogGroup reads were byte-identical.
  • Raw HTTP: a PutLogs into a disabled log returns 409 IncorrectState, and every bad-input case above returns the expected 400 or 404.

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 Logging: log groups, logs, search

3 participants