ROX-33683: Add tailored profiles central capability#19518
Conversation
|
Skipping CI for Draft Pull Request. |
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The new early return in
TailoredProfileDispatcher.ProcessEventbased onComplianceV2TailoredProfilesmeans we now drop all tailored profile events (including deletes) when Central doesn't advertise this capability; please double‑check this aligns with expected downgrade/compat behavior and that we don't need to treat delete actions differently. - In the tests you call
centralcaps.Set(...)but never reset the capability state, which can leak configuration across tests in this package; consider usingt.Cleanupor a helper to restore the previous capabilities after each test. - In
service_impl.goyou appendcentralsensor.ComplianceV2TailoredProfiles.String()while other capabilities are appended directly asCentralCapabilityvalues; for consistency and to avoid type surprises, consider aligning this with how the other capabilities are added.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new early return in `TailoredProfileDispatcher.ProcessEvent` based on `ComplianceV2TailoredProfiles` means we now drop all tailored profile events (including deletes) when Central doesn't advertise this capability; please double‑check this aligns with expected downgrade/compat behavior and that we don't need to treat delete actions differently.
- In the tests you call `centralcaps.Set(...)` but never reset the capability state, which can leak configuration across tests in this package; consider using `t.Cleanup` or a helper to restore the previous capabilities after each test.
- In `service_impl.go` you append `centralsensor.ComplianceV2TailoredProfiles.String()` while other capabilities are appended directly as `CentralCapability` values; for consistency and to avoid type surprises, consider aligning this with how the other capabilities are added.
## Individual Comments
### Comment 1
<location path="central/sensor/service/service_impl.go" line_range="141" />
<code_context>
capabilities = append(capabilities, centralsensor.SendDeduperStateOnReconnect)
if features.ComplianceEnhancements.Enabled() {
capabilities = append(capabilities, centralsensor.ComplianceV2Integrations)
+ capabilities = append(capabilities, centralsensor.ComplianceV2TailoredProfiles.String())
}
if features.ComplianceRemediationV2.Enabled() {
</code_context>
<issue_to_address>
**issue (bug_risk):** Use the capability value consistently instead of calling `.String()` here
Other capabilities in this block (e.g. `ComplianceV2Integrations`) are appended directly, and `CentralCapability` is already a `string` type alias. Calling `.String()` here is unnecessary and may not compile if that method doesn’t exist. This should match the others, e.g.:
```go
capabilities = append(capabilities, centralsensor.ComplianceV2TailoredProfiles)
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Images are ready for the commit at 50a8511. To use with deploy scripts, first |
No need to keep delete for downgrade purposes. CO events are not deduped, and central reconciles after sync. So on downgrade no TPs will be sent, and central will delete those that previously existed. |
312b0b6 to
7f6b4c8
Compare
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In
TestProcessEvent_V2EventHasTailoredProfileKind, the assertion that the V2 message is at index 1 couples the test to the ordering ofForwardMessages; consider locating the V2 message by checking which entry hasGetComplianceOperatorProfileV2() != nilto make the test more robust to future changes. - The conditional in
ProcessEventnow checkscentralcaps.Has(ComplianceV2Integrations)andcentralcaps.Has(ComplianceV2TailoredProfiles)separately; if this pattern is expected to recur, consider introducing a small helper (e.g.,hasComplianceV2TailoredProfilesSupport()) to centralize the logic and make the intent clearer.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `TestProcessEvent_V2EventHasTailoredProfileKind`, the assertion that the V2 message is at index 1 couples the test to the ordering of `ForwardMessages`; consider locating the V2 message by checking which entry has `GetComplianceOperatorProfileV2() != nil` to make the test more robust to future changes.
- The conditional in `ProcessEvent` now checks `centralcaps.Has(ComplianceV2Integrations)` and `centralcaps.Has(ComplianceV2TailoredProfiles)` separately; if this pattern is expected to recur, consider introducing a small helper (e.g., `hasComplianceV2TailoredProfilesSupport()`) to centralize the logic and make the intent clearer.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
sensor/kubernetes/complianceoperator/dispatchers/complianceoperatortailoredprofiles.go
Outdated
Show resolved
Hide resolved
Co-authored-by: Mladen Todorovic <mtodor@gmail.com>
|
/test ocp-4-21-compliance-e2e-tests |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #19518 +/- ##
==========================================
- Coverage 49.27% 49.24% -0.03%
==========================================
Files 2726 2727 +1
Lines 205628 205765 +137
==========================================
+ Hits 101314 101329 +15
- Misses 96777 96899 +122
Partials 7537 7537
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:
|
Description
A Tailored Profiles (TP) central capability is needed to handle scenarios where a Sensor that supports TPs connects to a Central that doesn’t. Because TPs use the same proto message as regular profiles (adding a kind=TAILORED_PROFILE field), old Centrals would ignore that field and store the TP as a regular profile, allowing users to schedule scans using those TPs.
Those scans would fail to be created in Sensor because the Kind of the profiles they include would be wrong. Even if that were fixed in Sensor by figuring out the kind by reaching out to the k8s API, the resulting ComplianceCheckResults and associated reporting would be broken due to referring to TPs and (possibly) custom rules.
This PR adds a new capability (
ComplianceV2TailoredProfiles) and modifies logic so that Sensor only processes Compliance V2 tailored profiles if Central exposes the capability.User-facing documentation
Testing and quality
Automated testing
How I validated my change
CI