Conversation
NitinKumar004
left a comment
There was a problem hiding this comment.
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%.
b155d84 to
86a227b
Compare
|
Rebased onto current CI — CodeQL
|
NitinKumar004
left a comment
There was a problem hiding this comment.
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
PutLogshas 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 returns200+opc-work-request-id. This matches the siblingserver/oci/vcnconvention (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
left a comment
There was a problem hiding this comment.
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:
resolveLimitnow rejects anything abovemaxLogLimit(10000) before themake.
Merge state
- The branch is 281 commits behind development and conflicts in
docs/coverage/README.md. Rebase and re-rungo generate ./.... The regenerated output only adds the OCI cell to theloggingrow. Everything else merges cleanly, and the merged tree builds and passesproviders/oci/...,server/oci/...,persist/...andinternal/coveragegen/....
High
-
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 realloggingingestionclient usesBasePath = "20200831"(oci-go-sdk v65loggingingestion_logging_client.go:63), and the API reference islogging-dataplane/20200831/LogEntry/PutLogs. - Repro: point
loggingingestion.NewLoggingClientWithConfigurationProviderat the server and callPutLogs. You get501 ... no handler registered for this requestatPOST /20200831/logs/<ocid>/actions/push. A follow-upSearchLogsthen returns 0 results. - The handler tests post to
/20200601directly, so they pass while real clients fail. - Fix: change
versionIngestionto20200831. Also update the package doc comment,types.go:98,docs/services.md:1491and the 15 test paths.
- The handler claims
-
ChangeLogGroupCompartment reads the wrong body field.
server/oci/logging/types.go:29,server/oci/logging/group.go:171changeCompartmentRequestdecodestargetCompartmentId. The realChangeLogGroupCompartmentDetailsfield iscompartmentId(logging/change_log_group_compartment_details.go:24).- Repro:
mc.ChangeLogGroupCompartment(ctx, logging.ChangeLogGroupCompartmentRequest{LogGroupId: gid, ChangeLogGroupCompartmentDetails: logging.ChangeLogGroupCompartmentDetails{CompartmentId: &c}})returns400 InvalidParameter: targetCompartmentId is required. - Fix: decode
compartmentId, and update the tests at handler_test.go:218, :876 and :919.
Medium
-
Data race between MoveGroup and GetLog/ListLogs.
providers/oci/logging/group.go:177MoveGroupwritesrec.Log.Configuration.CompartmentIDin place under the write lock.GetLogandUpdateLogreturnout := rec.Log(log.go:127, :226), which shares the same*LogConfigurationpointer.toLogResponsethen reads it after the lock is released.- Repro: one goroutine loops
MoveGroup(g, "c1"/"c2")while another loopsGetLog(g, l)and reads.Configuration.CompartmentID.go test -racereportsWARNING: DATA RACEwith 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-copyConfiguration(andSource.Parameters) on every read.
-
Creates and moves into a compartment that does not exist succeed.
server/oci/logging/group.go:55,:160POST /20200531/logGroupswithcompartmentId: ocid1.compartment.oc1..doesnotexistreturns 202. Real OCI returns 404 NotAuthorizedOrNotFound. VCN already does this check throughSetCompartmentChecker, whichserver/oci/oci.gowires from Identity.- Fix: add the same optional checker to the logging handler, wire it in
server/oci/oci.go, and call it increateGroupandmoveGroup.
-
The compartment move records an operation type OCI does not define.
server/oci/logging/handler.go:76operationMoveGroup = "CHANGE_LOG_GROUP_COMPARTMENT". The SDK'sOperationTypesEnumdefinesMOVE_LOG_GROUP(andMOVE_LOG).GetMappingOperationTypesEnumdoes not recognize the current value, so any client that switches onWorkRequest.OperationTypemisses it.- Fix: use
MOVE_LOG_GROUP.
Low
DeleteLogGroupcascades 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 returningFailedPrecondition(409 IncorrectState) on the OCI path when the group still holds logs, and keep the cascade only for the portableDeleteLogGroup.retentionDurationis not validated (providers/oci/logging/log.go:43,:207). OCI accepts 30 to 180 in 30-day steps, butretentionDuration: 7is stored as is. Reject other values with 400 InvalidParameter.- A CUSTOM log comes back with a synthesized
configuration.source.sourceType: OCISERVICEand 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 leavesourceout for CUSTOM logs. tenancyIdis 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.- PutLogs accepts batches with no
source,typeordefaultlogentrytime. All three are mandatory inLogEntryBatch, so real OCI rejects them with 400. - SearchLogs passes
limitbut ignorespageand never setsopc-next-page(server/oci/logging/search.go:41), so results past the first page can't be fetched. - golangci-lint v2.14.0 reports 6
goconsthits (ingestion.go:40, portable.go:227, search.go:97, :239, :241, :242). Pull"compartmentId","STRING","data"and theoracle.*field names into constants.
Checked
- Merged tree (PR plus current development):
go vetandgo testpass forproviders/oci/...,server/oci/...,persist/...andinternal/coveragegen/....-racepasses for the logging, oci provider and oci server packages. gofmt is clean. - Wiring: the provider field is set in
New(),wireMonitoringcovers Logging,DriversFrommaps it, and the handler is registered after the work request handler. No other OCI handler claims/20200531,/20190909or/20200831. - Persistence: there is a compile-time
Snapshottableassertion,snapshot.Discoverfinds 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 carriesopc-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 (WithEnforceAuthcovers 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.
|
Please merge the latest |
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.
86a227b to
e3e7530
Compare
|
Rebased onto Highs1. Ingestion version. To stop this happening again, 2. ChangeLogGroupCompartment decodes Mediums3. Data race. 4. Compartment check. Copied the VCN pattern from development exactly and wired it in 5. Operation type is Lows6. Deleting a log group that still holds logs returns Two bugs found that the review didn't list
Not changed, for your call
Verification
E2E against
|
NitinKumar004
left a comment
There was a problem hiding this comment.
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
- High: ingestion API version. Fixed.
versionIngestionis now20200831. With oci-go-sdk v65.126.1 againstserve,loggingingestion.PutLogsreturns 200 and a follow-upSearchLogsfinds all five ingested entries. The old/20200601/.../actions/pushpath now returns 501, so the handler no longer claims it. - High: ChangeLogGroupCompartment body field. Fixed.
ChangeLogGroupCompartmentDetails{CompartmentId}from the SDK moves the group.GetLogGroupandGetLogboth report the new compartment afterwards. A body that only setstargetCompartmentIdnow gets 400compartmentId is required. - Medium: MoveGroup vs GetLog/ListLogs data race. Fixed.
Log.clone()deep-copiesConfiguration,Source.Parametersand the tags, and every read path returns that clone.MoveGroupandMoveLogswap in a new configuration pointer instead of writing through the old one.TestConcurrentMoveAndReadcovers the original Get-vs-Move repro, andgo test -race -count=3 ./providers/oci/logging/...passes. - Medium: create or move into a compartment that does not exist. Fixed. With the SDK,
CreateLogGroupandChangeLogGroupCompartmentinto a made-up compartment both return 404 NotAuthorizedOrNotFound. Creating in the tenancy root still works (202). The checker is wired from Identity inserver/oci/oci.go. - Medium: work request operation type. Fixed. The SDK's
GetWorkRequestreportsMOVE_LOG_GROUP/ SUCCEEDED after a compartment move. - Low: deleting a non-empty group. Fixed. The SDK gets 409 IncorrectState. After
DeleteLog, the group deletes andGetLogGroupreturns 404. The portableDeleteLogGroupstill cascades. - Low: retentionDuration validation. Fixed.
CreateLogwith 7 andUpdateLogwith 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. - Low: synthesized source on CUSTOM logs. Fixed. A CUSTOM log created without a configuration returns
configuration: null. A SERVICE log round-trips itssourceas sent. - Low: tenancyId. Fixed.
GetLogreturnstenancyIdset to the configured tenancy. - Low: mandatory batch fields. Fixed. A batch without
sourcereturns 400logEntryBatches[0].source is required, and all batches are validated before any is stored. - Low: SearchLogs paging. Fixed. With
limit=2the SDK walked 2, 2, 1 entries withopc-next-pageset 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=1setsopc-next-page. - Low: goconst. Fixed. golangci-lint v2.14.0 with
--new-from-rev=origin/developmentreports 0 issues onproviders/oci/...andserver/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.UpdateLogGroupwith a newScope.Compartmentsetsg.CompartmentIDand nothing else. The OCIMoveGroupalso updates each log'sCompartmentIDand itsConfiguration.CompartmentID. Repro: portableCreateLogGroupin compartment a, thenCreateLogStream, thenUpdateLogGroupwith compartment b.ListLogsthen shows the group in b and its log still in a, and that stale value is whatGetLogreturns 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, andlog.go:45. PortableCreateLogGrouporUpdateLogGroupwithRetentionDays: 7succeeds. Any log created later through the OCI path inherits 7 without validation, which bypasses the check added for finding 7. Fix: runvalidateRetentionon 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 vetandgo testpass forproviders/oci/...,server/oci/...,persist/...andinternal/coveragegen/....-racepasses for the logging provider andserver/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.
Summary
loggingdriver.services/logging/driver; OCI-only behaviour is a consumer-sideExtrasinterface, per Move OCI-only capabilities out of shared driver packages #393.Closes #414. Part of #376.
Changes
providers/oci/logging/—Mockovermemstoreimplementingdriver.Logging, guarded by async.RWMutex, plus a search-query parser.server/oci/logging/— the three-prefix wire surface.providers/oci/oci.goandserver/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 returnUnimplementednaming Service Connectors as OCI's answer.The three prefixes — the main design risk
parsePathsplits/{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
/logscollection exists only on the ingestion prefix — the control plane nests logs under their group — so/20200531/logsis deliberately unclaimed.TestMatcheshas 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 byand)| sort by datetime [asc|desc]. Fields resolve overlogContent.*anddata.<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
ListLogstakes nocompartmentId— real OCI derives it from the log group in the path, so the group OCID is what is required. Noted inservices.mdand the handler comment.RetentionDayshas somewhere to live.DeleteLogGroupcascades to its logs rather than refusing, matching the other portable drivers.Provider Coverage
Checklist
go test ./...) — exit 0, 272 packagesgolangci-lint run --timeout=9m) — 0 issuescloudemu_test.go— driver + handler tests insteadTest Plan
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):
Left out
No
oci-go-sdkcompat test — the three-client split made the e2e transcript stronger evidence for the effort.oracle.tenantidis omitted from search records; adding it would pull config identity into the handler for no behavioural gain.