Skip to content

ROX-33555: Emit VM index ACK/NACK from Central#19323

Merged
vikin91 merged 7 commits intomasterfrom
piotr/ROX-32848-sensor-vm-ack-emit-in-central
Mar 20, 2026
Merged

ROX-33555: Emit VM index ACK/NACK from Central#19323
vikin91 merged 7 commits intomasterfrom
piotr/ROX-32848-sensor-vm-ack-emit-in-central

Conversation

@vikin91
Copy link
Copy Markdown
Contributor

@vikin91 vikin91 commented Mar 6, 2026

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: Sends SensorACK(ACK) after successful vulnerability
    enrichment and DB store. SendSensorACK is gated behind SensorACKSupport capability so
    older Sensors (pre-4.10) are unaffected.
  • connection_impl.go: When the rate limiter drops a VirtualMachineIndexReport, sends
    SensorACK(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

  • the change is production ready: the change is GA, or otherwise the functionality is gated by a feature flag
  • CI results are inspected

Automated testing

  • added unit tests
  • added e2e tests
  • added regression tests
  • added compatibility tests
  • modified existing tests

How I validated my change

  • CI unit tests
  • Manually on a cluster:

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.

➜ logs-sensor | grep "Received " | grep -v "NODE_" | grep -v "node-"
common/virtualmachine/index: 2026/03/11 08:38:07.522305 handler_impl.go:133: Debug: Received ACK from Central for VM index report: vm_id=36f8206d-733b-4ffd-983b-c8472e81b063
common/virtualmachine/index: 2026/03/11 08:38:13.740323 handler_impl.go:133: Debug: Received ACK from Central for VM index report: vm_id=36f8206d-733b-4ffd-983b-c8472e81b063
(...)
common/virtualmachine/index: 2026/03/11 08:41:37.362109 handler_impl.go:136: Warn: Received NACK from Central for VM index report: vm_id=36f8206d-733b-4ffd-983b-c8472e81b063, reason=rate limited: rate limit exceeded
common/virtualmachine/index: 2026/03/11 08:41:42.284817 handler_impl.go:136: Warn: Received NACK from Central for VM index report: vm_id=36f8206d-733b-4ffd-983b-c8472e81b063, reason=rate limited: rate limit exceeded

I still plan to rephrase reason=rate limited: rate limit exceeded into reason=central rate limit exceeded.

@vikin91
Copy link
Copy Markdown
Contributor Author

vikin91 commented Mar 6, 2026

@openshift-ci
Copy link
Copy Markdown

openshift-ci bot commented Mar 6, 2026

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@vikin91 vikin91 changed the title ROX-32848: Ensure Central emits the new N/ACK for VMs ROX-32848: Emit VM index ACK/NACK from Central Mar 6, 2026
@rhacs-bot
Copy link
Copy Markdown
Contributor

rhacs-bot commented Mar 6, 2026

Images are ready for the commit at 9bfde0b.

To use with deploy scripts, first export MAIN_IMAGE_TAG=4.11.x-393-g9bfde0ba0e.

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 6, 2026

Codecov Report

❌ Patch coverage is 80.00000% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 49.28%. Comparing base (be83dc4) to head (9bfde0b).
⚠️ Report is 10 commits behind head on master.

Files with missing lines Patch % Lines
...ntral/sensor/service/connection/connection_impl.go 0.00% 3 Missing ⚠️
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     
Flag Coverage Δ
go-unit-tests 49.28% <80.00%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@vikin91 vikin91 changed the base branch from piotr/ROX-32316-sensor-vm-ack-forwarding to master March 9, 2026 10:00
@vikin91 vikin91 force-pushed the piotr/ROX-32848-sensor-vm-ack-emit-in-central branch from 9192241 to 30044d9 Compare March 9, 2026 10:00
@vikin91 vikin91 marked this pull request as ready for review March 9, 2026 11:21
Copy link
Copy Markdown
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - I've found 1 issue, and left some high level feedback:

  • 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.
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, &central.MsgToSensor{
		Msg: &central.MsgToSensor_SensorAck{
			SensorAck: &central.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>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@vikin91 vikin91 changed the title ROX-32848: Emit VM index ACK/NACK from Central ROX-33555: Emit VM index ACK/NACK from Central Mar 11, 2026
@vikin91 vikin91 force-pushed the piotr/ROX-32848-sensor-vm-ack-emit-in-central branch 2 times, most recently from f535cc1 to eb98235 Compare March 11, 2026 16:24
@vikin91
Copy link
Copy Markdown
Contributor Author

vikin91 commented Mar 12, 2026

/retest

@vikin91 vikin91 changed the base branch from master to piotr/ROX-33555-sensor-ack-node-scanning March 13, 2026 15:29
@vikin91 vikin91 force-pushed the piotr/ROX-32848-sensor-vm-ack-emit-in-central branch 2 times, most recently from 7ad2f8f to a49b5c2 Compare March 13, 2026 15:35
@vikin91 vikin91 force-pushed the piotr/ROX-33555-sensor-ack-node-scanning branch from 8b9adc1 to 97c2430 Compare March 16, 2026 09:36
@vikin91 vikin91 force-pushed the piotr/ROX-32848-sensor-vm-ack-emit-in-central branch from a49b5c2 to 37a1bae Compare March 16, 2026 12:06
Base automatically changed from piotr/ROX-33555-sensor-ack-node-scanning to master March 16, 2026 16:22
@vikin91 vikin91 force-pushed the piotr/ROX-32848-sensor-vm-ack-emit-in-central branch from 2868bae to e8a2476 Compare March 16, 2026 16:23
@vikin91
Copy link
Copy Markdown
Contributor Author

vikin91 commented Mar 19, 2026

@sourcery-ai review

Copy link
Copy Markdown
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - I've left some high level feedback:

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

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@vikin91
Copy link
Copy Markdown
Contributor Author

vikin91 commented Mar 19, 2026

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

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.

@vikin91
Copy link
Copy Markdown
Contributor Author

vikin91 commented Mar 19, 2026

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

This is done.

@vikin91 vikin91 requested a review from rhybrillou March 19, 2026 14:51
vikin91 added 7 commits March 20, 2026 13:51
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.
@vikin91 vikin91 force-pushed the piotr/ROX-32848-sensor-vm-ack-emit-in-central branch from 349f562 to 9bfde0b Compare March 20, 2026 12:52
@vikin91 vikin91 merged commit 7971857 into master Mar 20, 2026
158 of 180 checks passed
@vikin91 vikin91 deleted the piotr/ROX-32848-sensor-vm-ack-emit-in-central branch March 20, 2026 16:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants