ROX-33555: Emit VM index ACK/NACK from Central#19323
Conversation
|
This change is part of the following stack: Change managed by git-spice. |
|
Skipping CI for Draft Pull Request. |
|
Images are ready for the commit at 9bfde0b. To use with deploy scripts, first |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #19323 +/- ##
==========================================
+ Coverage 49.26% 49.28% +0.01%
==========================================
Files 2726 2726
Lines 205628 205642 +14
==========================================
+ Hits 101297 101343 +46
+ Misses 96791 96762 -29
+ Partials 7540 7537 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
9192241 to
30044d9
Compare
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
SendSensorACKyou passctxintoinjector.InjectMessage, but the mocks (andcommon.MessageInjectorin the rest of the codebase) expect aconcurrency.Waitableargument; consider using a properWaitable(e.g. a no-op signal) instead ofcontext.Contextto keep the call signature consistent and avoid compilation or runtime issues. - The behavior of
SendSensorACKfor injectors that do not implementcapabilityCheckercontradicts the comment about only sending whenSensorACKSupportis advertised; either gate those calls as well or update the comment/tests to make the intended behavior for older/non-capability-aware injectors explicit. - Importing
vmindexpipelineintoconnection_impl.goto callSendSensorACKcreates a cross-layer dependency from the generic connection layer into a specific pipeline; consider moving the ACK helper to a more neutral package (e.g.pipeline/commonor a utility package) to keep the layering between connection and individual pipelines cleaner.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `SendSensorACK` you pass `ctx` into `injector.InjectMessage`, but the mocks (and `common.MessageInjector` in the rest of the codebase) expect a `concurrency.Waitable` argument; consider using a proper `Waitable` (e.g. a no-op signal) instead of `context.Context` to keep the call signature consistent and avoid compilation or runtime issues.
- The behavior of `SendSensorACK` for injectors that do not implement `capabilityChecker` contradicts the comment about only sending when `SensorACKSupport` is advertised; either gate those calls as well or update the comment/tests to make the intended behavior for older/non-capability-aware injectors explicit.
- Importing `vmindexpipeline` into `connection_impl.go` to call `SendSensorACK` creates a cross-layer dependency from the generic connection layer into a specific pipeline; consider moving the ACK helper to a more neutral package (e.g. `pipeline/common` or a utility package) to keep the layering between connection and individual pipelines cleaner.
## Individual Comments
### Comment 1
<location path="central/sensor/service/pipeline/virtualmachineindex/pipeline.go" line_range="132-144" />
<code_context>
+ Reason: reason,
+ },
+ },
+ }); err != nil {
+ log.Warnf("Failed injecting SensorACK (%s) for VM index report (vm_id=%s): %v", action, vmID, err)
+ }
+}
</code_context>
<issue_to_address>
**suggestion:** Use a more appropriate format verb for the SensorACK_Action enum in the warning log.
`action` is a `central.SensorACK_Action` enum. With `%s`, you’ll just get its numeric value rendered as a string, which isn’t useful and can be misleading in logs. Prefer `%v` (or `%d` for the numeric value), or keep `%s` only if you explicitly convert `action` to a string representation first.
```suggestion
if err := injector.InjectMessage(ctx, ¢ral.MsgToSensor{
Msg: ¢ral.MsgToSensor_SensorAck{
SensorAck: ¢ral.SensorACK{
Action: action,
MessageType: central.SensorACK_VM_INDEX_REPORT,
ResourceId: vmID,
Reason: reason,
},
},
}); err != nil {
log.Warnf("Failed injecting SensorACK (%v) for VM index report (vm_id=%s): %v", action, vmID, err)
}
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
central/sensor/service/pipeline/virtualmachineindex/pipeline.go
Outdated
Show resolved
Hide resolved
f535cc1 to
eb98235
Compare
|
/retest |
7ad2f8f to
a49b5c2
Compare
8b9adc1 to
97c2430
Compare
a49b5c2 to
37a1bae
Compare
central/sensor/service/pipeline/virtualmachineindex/pipeline.go
Outdated
Show resolved
Hide resolved
2868bae to
e8a2476
Compare
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- There are many repeated
SendSensorACKcalls inpipeline.Runwith similar parameters; consider introducing a small helper (e.g.,sendVMIndexACK/NACK) to centralize messageType and reason handling and reduce duplication/error risk. - In
sensorConnection.multiplexedPush, when emitting a NACK for rate-limited VM index reports you always useSensorACKReasonRateLimited; if the rate limiter provides more specific context, consider including or mapping that information so sensor-side behavior can distinguish different rate-limit scenarios if needed.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- There are many repeated `SendSensorACK` calls in `pipeline.Run` with similar parameters; consider introducing a small helper (e.g., `sendVMIndexACK/NACK`) to centralize messageType and reason handling and reduce duplication/error risk.
- In `sensorConnection.multiplexedPush`, when emitting a NACK for rate-limited VM index reports you always use `SensorACKReasonRateLimited`; if the rate limiter provides more specific context, consider including or mapping that information so sensor-side behavior can distinguish different rate-limit scenarios if needed.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Well, the reason provided from the rate limiter is always the same. We won't get any extra info from there. Constant reason in this branch remains. |
This is done. |
central/sensor/service/pipeline/virtualmachineindex/pipeline_test.go
Outdated
Show resolved
Hide resolved
central/sensor/service/pipeline/virtualmachineindex/pipeline_test.go
Outdated
Show resolved
Hide resolved
central/sensor/service/pipeline/virtualmachineindex/pipeline_test.go
Outdated
Show resolved
Hide resolved
Central's VM index pipeline was missing ACK/NACK responses, breaking the end-to-end ACK flow. Adds ACK on successful enrichment+store and NACK when the rate limiter drops a report. ACKs are gated behind the SensorACKSupport capability so older Sensors are unaffected.
349f562 to
9bfde0b
Compare
Description
Sensor's handler currently has the forwarding logic for the ACK messages, but no message was never triggered from Central. In this PR, the VM scanning flow sends ACKs and NACKs based on the situation.
Changes in Central:
virtualmachineindex/pipeline.go: SendsSensorACK(ACK)after successful vulnerabilityenrichment and DB store.
SendSensorACKis gated behindSensorACKSupportcapability soolder Sensors (pre-4.10) are unaffected.
connection_impl.go: When the rate limiter drops aVirtualMachineIndexReport, sendsSensorACK(NACK)with the rate limit reason so Sensor/compliance can act on it.AI-assisted: implementation and tests generated by AI based on user requirements
(ACK on success, NACK on rate limit, capability gating). Reviewed and verified by the author.
User-facing documentation
Testing and quality
Automated testing
How I validated my change
Tested on a cluster by deploying the full stack (including PRs #19319 #19321 and #19322). The cluster had a single VM. Enabled debug logs on sensor and let the VM be scanned.
I still plan to rephrase
reason=rate limited: rate limit exceededintoreason=central rate limit exceeded.