OSAC-3424: implement M360 metering adapter - #236
Conversation
|
@amito: This pull request references OSAC-3424 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: amito The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe PR adds an M360 Kafka adapter that translates OSAC CloudEvents, sends them to the M360 Usage API, exposes health and metrics endpoints, and provides container, CI, and conditional Helm deployment support. ChangesM360 adapter
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Kafka
participant Runner
participant m360Adapter
participant translateEvent
participant m360Client
participant M360UsageAPI
Kafka->>Runner: Deliver OSAC CloudEvent
Runner->>m360Adapter: Process event
m360Adapter->>translateEvent: Translate CloudEvent
translateEvent-->>m360Adapter: Return M360 payload
m360Adapter->>m360Client: Post usage event
m360Client->>M360UsageAPI: Authenticated HTTP POST
M360UsageAPI-->>m360Client: Return response
m360Client-->>m360Adapter: Return delivery result
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5 | ❌ 6❌ Failed checks (6 inconclusive)
✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
1f43bc5 to
f15efae
Compare
f15efae to
ba9705a
Compare
ba9705a to
fc245b6
Compare
fc245b6 to
e490c40
Compare
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Final Review — Follow-up VerificationRe-checked out the latest head ( Fixed ✅
Still open — CRITICAL — CI still doesn't run the new tests
|
f9c2f9d to
e05f805
Compare
Auto-dismissed: only Prow labels gate merging
|
🤖 Finished Review · ✅ Success · Started 8:43 AM UTC · Completed 8:58 AM UTC Commit: |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Implements translateEvent() function that converts OSAC metering CloudEvents to flat M360 Usage API payloads with endpoint routing. - Maps resource types to M360 endpoints (compute_instance -> /vmaas/event, cluster_order -> /caas/event, maas_inference -> /maas/event) - Flattens nested billing_dimensions to top-level fields - Converts nil/empty values to M360 space string convention - Returns NonRetryableError for unknown resource types and malformed data Test coverage: 6 tests covering VMaaS, CaaS, MaaS events and error cases. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Amit Oren <amoren@redhat.com>
Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Amit Oren <amoren@redhat.com>
Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Amit Oren <amoren@redhat.com>
Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Amit Oren <amoren@redhat.com>
e05f805 to
380e6f7
Compare
|
🤖 Review · ❌ Terminated · Started 10:15 AM UTC · Ended 10:34 AM UTC Commit: |
|
/lgtm |
| } | ||
| return v | ||
| } | ||
|
|
There was a problem hiding this comment.
[low] edge-case
isZeroValue filters numeric zero from billing_dimensions, silently dropping zero-valued dimensions (e.g., cache_creation_tokens: 0). M360 cannot distinguish not applicable from measured as zero. The behavior is intentional and tested, but the semantic distinction may matter for token-based MaaS billing.
Suggested fix: Consider sending zero values and using nil or absence to indicate not applicable. If M360 treats zero and absent identically, document this explicitly in the code comment referencing the M360 API contract.
| core="${version%%-*}" | ||
| echo "RELEASE_VERSION=${version}" >> "$GITHUB_ENV" | ||
| echo "RELEASE_MAJOR_MINOR=${core%.*}" >> "$GITHUB_ENV" | ||
| echo "RELEASE_MAJOR=${core%%.*}" >> "$GITHUB_ENV" |
There was a problem hiding this comment.
[low] GHA workflow command injection
The ::error:: workflow command interpolates GITHUB_REF_NAME without sanitization for :: sequences. Practical risk is very low (requires collaborator push access, ::set-env:: disabled by default, semver regex rejects most payloads). Same pattern as existing echo-adapter workflow.
Suggested fix: Sanitize GITHUB_REF_NAME before embedding in ::error:: or use GITHUB_STEP_SUMMARY approach.
| Err: fmt.Errorf("marshal M360 payload: %w", err), | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
[low] authentication
No HTTPS enforcement on M360_API_URL. If misconfigured with an http:// URL, the Bearer token would transmit in cleartext.
Suggested fix: Consider validating that M360_API_URL starts with https:// at startup, or log a warning when using plaintext HTTP.
| # tag: latest | ||
| # pullPolicy: Always | ||
|
|
||
| ## M360 adapter — forwards metering events to Monetize360 Usage API. |
There was a problem hiding this comment.
[low] pattern-inconsistency
The m360-adapter provides real image defaults while the echo-adapter image config is commented out. The required wrappers in the deployment template are redundant given the defaults exist. Minor inconsistency with no functional impact.
|
🤖 Finished Review · ✅ Success · Started 10:15 AM UTC · Completed 10:34 AM UTC Commit: |
4 participants
Description
Standalone Go binary that consumes OSAC metering CloudEvents from Kafka and
forwards them to the Monetize360 (M360) Usage API via REST. Implements the
ProviderAdapterinterface using the existing adapter Runner framework(Kafka consumption, dedup, retry, offset management all handled by the Runner).
The adapter translates nested CloudEvents to M360's flat payload format and
routes to the correct M360 endpoint by resource type:
compute_instance(VMaaS)POST /vmaas/eventcluster_order(CaaS)POST /caas/eventmaas_inference(MaaS)POST /maas/eventKey behaviors:
adapters.AllTopics(lifecycle, heartbeat, corrections, inference); overridable viaKAFKA_TOPICSenv var (rejects empty override)" "(space string) per M360 conventionFLUSH_INTERVALenv varFLUSH_INTERVALvalues (preventstime.NewTickerpanic)v1)GET /healthz— liveness probe (always 200)GET /readyz— readiness probe (M360 connectivity check,timeoutSeconds: 5to accommodate upstream latency)m360Adapter.enabled: false(disabled by default)requiredchecks forapiKeySecret,apiUrl,image.repository,image.tagReadHeaderTimeoutbuild-metering-m360-adapter-image.yaml) — mirrors echo-adapter workflowFiles
adapters/cmd/m360-adapter/translate.goadapters/cmd/m360-adapter/client.goadapters/cmd/m360-adapter/main.goadapters/cmd/m360-adapter/m360_adapter_suite_test.goadapters/Containerfile.m360-adapteradapters/Makefilealldefault target,build-m360-adapter, recursive test/lint.github/workflows/build-metering-m360-adapter-image.yamlcharts/osac-metering/templates/m360-adapter-*charts/osac-metering/templates/kafka-secrets-rbac.yamlcharts/osac-metering/values.yamlm360Adapterconfig sectionJira
Not Included
uses REST API only
exists
metering-service's Go module, depending only on CloudEvents SDK and
M360 API contract; track as fast-follow if schema drift becomes a concern
version tags; pinning and signing should be a repo-wide effort, not
introduced in a single adapter PR
Secret to have a key named
api-key(documented in values.yaml); makingthis configurable is a follow-up enhancement
automountServiceAccountToken: false— valid security hardening, butshould be applied consistently across all chart deployments (metering-service,
echo-adapter, m360-adapter) in a dedicated follow-up
References
How Has This Been Tested?
23 Ginkgo unit tests across 3 suites (105 total specs):
conversion, empty string→space conversion,
nilbilling_dimensionshandling,non-billable billing_dimensions filtering, malformed billing_dimensions (non-map)
error, canonical field overwrite protection, unknown
resource_typeerror,empty CloudEvent ID error, zero timestamp error, malformed CloudEvent error
version,
NonRetryableErroron 400/401, retryable error on 408/429, retryableerror on 500, context cancellation, health check success/failure
Manual end-to-end testing (adapter → M360 UAT/simulator) to be performed
during deployment validation.
Merge criteria: