ROX-34167: make compliance scan node roles configurable#21825
Draft
guzalv wants to merge 1 commit into
Draft
Conversation
|
Skipping CI for Draft Pull Request. |
github-actions
Bot
added
area/central
area/ui
area/sensor
ai-review
coderabbit-review
labels
Contributor
📝 WalkthroughWalkthroughCompliance scan configurations now support node roles across API and storage schemas, Central validation and conversion, Central-to-Sensor requests, Sensor scan settings, and the scheduling UI. Empty roles retain the ChangesCompliance scan node-role targeting
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant ScheduleUI
participant CentralAPI
participant ComplianceManager
participant SensorConnection
participant Sensor
ScheduleUI->>CentralAPI: Submit scan configuration with nodeRoles
CentralAPI->>ComplianceManager: Convert and validate scan request
ComplianceManager->>SensorConnection: Build scan settings with NodeRoles
SensorConnection->>Sensor: Send scheduled scan request
Sensor->>Sensor: Apply explicit roles or default roles
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ui/apps/platform/src/Containers/ComplianceEnhanced/Schedules/Wizard/ScanConfigOptions.tsx (1)
181-186: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCommit pending input on blur to prevent data loss.
If a user types a valid role but clicks outside the field before pressing Enter, their input is silently ignored. Updating
onBlurto evaluate pending input ensures a smoother user experience and prevents incomplete configurations.💡 Proposed fix to evaluate input on blur
- onBlur={() => - formik.setFieldTouched( - 'parameters.nodeRoles', - true - ) - } + onBlur={() => { + if (nodeRoleInput) { + addNodeRole(nodeRoleInput); + } + formik.setFieldTouched( + 'parameters.nodeRoles', + true + ); + }}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/apps/platform/src/Containers/ComplianceEnhanced/Schedules/Wizard/ScanConfigOptions.tsx` around lines 181 - 186, Update the nodeRoles field’s onBlur handler to evaluate and commit any valid pending input before marking parameters.nodeRoles as touched. Preserve the existing Enter-key behavior and ensure clicking away does not discard a typed role.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@ui/apps/platform/src/Containers/ComplianceEnhanced/Schedules/Wizard/ScanConfigOptions.tsx`:
- Around line 181-186: Update the nodeRoles field’s onBlur handler to evaluate
and commit any valid pending input before marking parameters.nodeRoles as
touched. Preserve the existing Enter-key behavior and ensure clicking away does
not discard a typed role.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: e8c89221-acb4-48f9-882a-872be66c3743
⛔ Files ignored due to path filters (7)
generated/api/v2/compliance_scan_configuration_service.pb.gois excluded by!**/*.pb.go,!**/generated/**generated/api/v2/compliance_scan_configuration_service.swagger.jsonis excluded by!**/generated/**generated/api/v2/compliance_scan_configuration_service_vtproto.pb.gois excluded by!**/*.pb.go,!**/generated/**generated/internalapi/central/compliance_operator.pb.gois excluded by!**/*.pb.go,!**/generated/**generated/internalapi/central/compliance_operator_vtproto.pb.gois excluded by!**/*.pb.go,!**/generated/**generated/storage/compliance_operator_v2.pb.gois excluded by!**/*.pb.go,!**/generated/**generated/storage/compliance_operator_v2_vtproto.pb.gois excluded by!**/*.pb.go,!**/generated/**
📒 Files selected for processing (20)
central/complianceoperator/v2/compliancemanager/manager_impl.gocentral/complianceoperator/v2/compliancemanager/utils.gocentral/complianceoperator/v2/scanconfigurations/service/convert.gocentral/complianceoperator/v2/scanconfigurations/service/service_impl.gocentral/complianceoperator/v2/scanconfigurations/service/service_impl_test.gocentral/sensor/service/connection/connection_impl.goproto/api/v2/compliance_scan_configuration_service.protoproto/internalapi/central/compliance_operator.protoproto/storage/compliance_operator_v2.protosensor/kubernetes/complianceoperator/types.gosensor/kubernetes/complianceoperator/utils.gosensor/kubernetes/complianceoperator/utils_test.goui/apps/platform/src/Containers/ComplianceEnhanced/Schedules/Wizard/ReviewConfig.tsxui/apps/platform/src/Containers/ComplianceEnhanced/Schedules/Wizard/ScanConfigOptions.tsxui/apps/platform/src/Containers/ComplianceEnhanced/Schedules/Wizard/useFormikScanConfig.tsxui/apps/platform/src/Containers/ComplianceEnhanced/Schedules/compliance.scanConfigs.utils.test.tsui/apps/platform/src/Containers/ComplianceEnhanced/Schedules/compliance.scanConfigs.utils.tsxui/apps/platform/src/Containers/ComplianceEnhanced/Schedules/components/ConfigDetails.tsxui/apps/platform/src/Containers/ComplianceEnhanced/Schedules/components/ScanConfigParametersView.tsxui/apps/platform/src/services/ComplianceScanConfigurationService.ts
Contributor
🚀 Build Images ReadyImages are ready for commit 9c2f276. To use with deploy scripts: export MAIN_IMAGE_TAG=4.12.x-537-g9c2f276143 |
Contributor
🚀 Build Images ReadyImages are ready for commit d02aa3d. To use with deploy scripts: export MAIN_IMAGE_TAG=4.12.x-537-gd02aa3d3b3 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #21825 +/- ##
==========================================
- Coverage 51.33% 51.33% -0.01%
==========================================
Files 2855 2856 +1
Lines 178578 178612 +34
==========================================
+ Hits 91678 91686 +8
- Misses 78878 78895 +17
- Partials 8022 8031 +9
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
ACS previously hardcoded ["master", "worker"] as node roles in every ScanSetting it generated for the Compliance Operator. This prevented scanning nodes with other roles (e.g. infra, control-plane, custom). Changes: - Storage proto: replace unused NodeRole enum field (tag 9, reserved) with repeated string node_roles (tag 19) on scan config - API proto: expose node_roles in BaseComplianceScanConfigurationSettings - Internal API proto: add node_roles to BaseScanSettings for Central->Sensor communication - Central: wire node_roles through conversion, validation, manager, and startup sync paths. Default to [master, worker] when empty. - Sensor: read roles from request instead of hardcoding. Fall back to [master, worker] when field is empty (old Central compat). - UI: add node roles input (text+labels) to scan config wizard Parameters step, with display in review and detail views. - Validation matches Compliance Operator rules: alphanumeric+hyphens, 1-39 chars, @ALL exclusive. Partially generated by AI.
guzalv
force-pushed
the
gualvare/rox-34167-configurable-node-roles
branch
from
July 21, 2026 12:20
d02aa3d to
9c2f276
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
ACS previously hardcoded
["master", "worker"]as node roles in every ScanSetting it generated for the Compliance Operator. This prevented scanning nodes with custom roles (e.g.,infra,control-plane,gpu).This change makes node roles configurable through the API and UI, with backward-compatible defaults (
["master", "worker"]).Changes
Proto layer:
NodeRoleenum field (tag 9), addedrepeated string node_roles(tag 19)node_rolesfield toBaseComplianceScanConfigurationSettingsnode_rolestoBaseScanSettingsfor Central-Sensor communicationCentral:
node_rolesbetween API and storage, applies["master", "worker"]default when empty@allexclusive, rejects duplicates)node_rolesthroughbuildScanConfigSensorMsgnode_rolesinSyncComplianceScanConfigRequestSensor:
["master", "worker"]["master", "worker"]when field is empty (backward compat with old Central)UI:
master,workerpre-populatedCoverage page
Node roles are intentionally not shown on the compliance coverage pages. Coverage answers "what passed/failed?" while node roles are a scan configuration input. Surfacing them on results pages would conflate configuration with outcomes and add visual noise without actionable value. Node roles are visible on the scan schedule detail page, one click from the coverage dropdown.
Design decisions
repeated stringover enum: The Compliance Operator accepts arbitrary role strings. An enum would require an ACS release for each new role name.NodeRoleenum (varint wire type); changing to string would break wire compatibility. Reserved tag 9 per proto best practices.masterandworker. Users can override or clear them; empty defaults to["master", "worker"]server-side.User-facing documentation
Testing and quality
Automated testing
Unit tests added for:
@all, mixed@all, invalid chars, duplicates, too long@allnodeRolesfieldHow I validated my change
Deployed to an OCP 4.22 cluster with Compliance Operator v1.9.1 and verified end-to-end.
API tests (curl against deployed Central
4.12.x-537-g9c2f276143):["master","worker"]["worker","infra"]["worker","infra"]["@all"]["@all"]["master","worker","control-plane"]["master","worker","control-plane"]["gpu"](label added to node)["gpu"]["control-plane","worker"]["control-plane","worker"]UI screenshots:
Create wizard — Node roles as a top-level section below Schedule, with defaults pre-populated:
Adding multiple roles (master, worker, infra, +1 more):
Invalid role input error:
Empty roles — no error, helper text explains defaults:
Config detail view showing Node roles:
AI-assisted implementation.