ROX-33477: Add support for tailored profiles to CO scan setting binding creation#19316
Conversation
I had to do: docker run --platform linux/amd64 --rm -v "$(pwd):/src" -w /src quay.io/stackrox-io/apollo-ci:stackrox-test-0.5.2 bash -c "rm -rf .gotools && go mod tidy && make proto-generated-srcs"
Extract this to separate PR?
… sensor" Let's look at a different approach. This reverts commit 0ed1a0a.
|
This change is part of the following stack: Change managed by git-spice. |
|
Skipping CI for Draft Pull Request. |
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- The
log.Infofcalls inside the profile loops inconvertCentralRequestToScanSettingBindingandupdateScanSettingBindingFromCentralRequestwill emit a log per profile on every apply/update, which may be noisy in large clusters; consider reducing to debug level or aggregating into a single log statement. - The logic that builds
profileRefsfor scan setting bindings is duplicated betweenconvertCentralRequestToScanSettingBindingandupdateScanSettingBindingFromCentralRequest; consider extracting a shared helper to keep the legacy/new-path behavior consistent and easier to maintain. - In
profileKindToString, all unknown or futureComplianceOperatorProfileKindvalues silently fall back toProfile.Kind; consider handling unexpected enum values explicitly (e.g., via aswitchwith a default log or error) to make misconfigurations easier to detect.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `log.Infof` calls inside the profile loops in `convertCentralRequestToScanSettingBinding` and `updateScanSettingBindingFromCentralRequest` will emit a log per profile on every apply/update, which may be noisy in large clusters; consider reducing to debug level or aggregating into a single log statement.
- The logic that builds `profileRefs` for scan setting bindings is duplicated between `convertCentralRequestToScanSettingBinding` and `updateScanSettingBindingFromCentralRequest`; consider extracting a shared helper to keep the legacy/new-path behavior consistent and easier to maintain.
- In `profileKindToString`, all unknown or future `ComplianceOperatorProfileKind` values silently fall back to `Profile.Kind`; consider handling unexpected enum values explicitly (e.g., via a `switch` with a default log or error) to make misconfigurations easier to detect.
## Individual Comments
### Comment 1
<location path="sensor/kubernetes/complianceoperator/utils.go" line_range="84-86" />
<code_context>
- })
+ if refs := request.GetProfileRefs(); len(refs) > 0 {
+ // New Central: kind is provided directly — no k8s API call needed.
+ for _, ref := range refs {
+ kind := profileKindToString(ref.GetKind())
+ log.Infof("Profile %q in namespace %q: kind %q provided by Central", ref.GetName(), namespace, kind)
+ profileRefs = append(profileRefs, v1alpha1.NamedObjectReference{
+ Name: ref.GetName(),
</code_context>
<issue_to_address>
**suggestion (performance):** Profile-level Info logging may be noisy at scale
For larger clusters, logging at Info for every profile on each scan could overwhelm logs. If this is mainly for debugging or initial verification, consider using Debug level, a verbosity flag, or some rate limiting to reduce log noise.
```suggestion
kind := profileKindToString(ref.GetKind())
log.Debugf("Profile %q in namespace %q: kind %q provided by Central", ref.GetName(), namespace, kind)
profileRefs = append(profileRefs, v1alpha1.NamedObjectReference{
```
</issue_to_address>
### Comment 2
<location path="central/complianceoperator/v2/compliancemanager/manager_impl.go" line_range="290-292" />
<code_context>
+ // Build profile refs from the already-fetched profiles so Sensor can use the correct CO kind
+ // (Profile vs TailoredProfile) without needing to probe the k8s API.
+ profileRefs := make([]*central.ApplyComplianceScanConfigRequest_ProfileReference, 0, len(returnedProfiles))
+ for _, p := range returnedProfiles {
+ profileRefs = append(profileRefs, ¢ral.ApplyComplianceScanConfigRequest_ProfileReference{
+ Name: p.GetName(),
+ Kind: central.ComplianceOperatorProfileV2_ComplianceOperatorProfileKind(p.GetComplianceOperatorKind()),
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Relying on a cast between two enums can be brittle if they diverge
This line relies on `central.ComplianceOperatorProfileV2_ComplianceOperatorProfileKind(p.GetComplianceOperatorKind())` preserving matching numeric values between the datastore and proto enums. If either enum changes independently, the wrong kind could be sent. Prefer an explicit mapping (e.g., a small switch) to make the dependency and failure modes explicit.
Suggested implementation:
```golang
// Build profile refs from the already-fetched profiles so Sensor can use the correct CO kind
// (Profile vs TailoredProfile) without needing to probe the k8s API.
profileRefs := make([]*central.ApplyComplianceScanConfigRequest_ProfileReference, 0, len(returnedProfiles))
for _, p := range returnedProfiles {
var kind central.ComplianceOperatorProfileV2_ComplianceOperatorProfileKind
switch p.GetComplianceOperatorKind() {
case storage.ComplianceOperatorProfileV2_PROFILE:
kind = central.ComplianceOperatorProfileV2_PROFILE
case storage.ComplianceOperatorProfileV2_TAILORED_PROFILE:
kind = central.ComplianceOperatorProfileV2_TAILORED_PROFILE
default:
// Make dependency and failure mode explicit if enums diverge or an unexpected
// value is stored. Log and skip this profile rather than sending an incorrect kind.
log.Errorf("Unknown compliance operator profile kind %v for profile %q; skipping profile in scan configuration %q",
p.GetComplianceOperatorKind(), p.GetName(), scanRequest.GetScanConfigName())
continue
}
profileRefs = append(profileRefs, ¢ral.ApplyComplianceScanConfigRequest_ProfileReference{
Name: p.GetName(),
Kind: kind,
})
}
```
1. This change assumes that:
- The datastore enum type used by `p.GetComplianceOperatorKind()` is `storage.ComplianceOperatorProfileV2_ComplianceOperatorProfileKind`.
- The datastore enum values are `storage.ComplianceOperatorProfileV2_PROFILE` and `storage.ComplianceOperatorProfileV2_TAILORED_PROFILE`.
- The central proto enum values are `central.ComplianceOperatorProfileV2_PROFILE` and `central.ComplianceOperatorProfileV2_TAILORED_PROFILE`.
If the actual enum names differ, adjust the `switch` cases and assigned central constants accordingly.
2. Ensure that the `storage` package is imported in this file (it typically is in this codebase). If it is not, add:
`storage "github.com/stackrox/rox/generated/storage"`
to the imports block.
</issue_to_address>
### Comment 3
<location path="sensor/kubernetes/complianceoperator/utils.go" line_range="79" />
<code_context>
func convertCentralRequestToScanSettingBinding(namespace string, request *central.ApplyComplianceScanConfigRequest_BaseScanSettings, _ string) runtime.Object {
profileRefs := make([]v1alpha1.NamedObjectReference, 0, len(request.GetProfiles()))
- for _, profile := range request.GetProfiles() {
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting the duplicated profile reference branching and construction logic into a shared helper function used by both callers to simplify and unify the code.
You can reduce the added complexity by extracting the duplicated branching + construction into a small helper that both call sites use.
For example:
```go
func buildProfileRefs(
request *central.ApplyComplianceScanConfigRequest_BaseScanSettings,
namespace string,
action string, // e.g. "create" or "update"
) []v1alpha1.NamedObjectReference {
refs := make([]v1alpha1.NamedObjectReference, 0, len(request.GetProfiles()))
if profileRefs := request.GetProfileRefs(); len(profileRefs) > 0 {
// New Central: kind is provided directly — no k8s API call needed.
for _, ref := range profileRefs {
kind := profileKindToString(ref.GetKind())
log.Infof("Profile %q in namespace %q: kind %q provided by Central (%s)", ref.GetName(), namespace, kind, action)
refs = append(refs, v1alpha1.NamedObjectReference{
Name: ref.GetName(),
Kind: kind,
APIGroup: complianceoperator.GetGroupVersion().String(),
})
}
return refs
}
// Legacy: old Central without profile_refs — default to Profile kind.
for _, profile := range request.GetProfiles() {
refs = append(refs, v1alpha1.NamedObjectReference{
Name: profile,
Kind: complianceoperator.Profile.Kind,
APIGroup: complianceoperator.GetGroupVersion().String(),
})
}
return refs
}
```
Then both callers become simpler and consistent:
```go
func convertCentralRequestToScanSettingBinding(namespace string, request *central.ApplyComplianceScanConfigRequest_BaseScanSettings, _ string) runtime.Object {
profileRefs := buildProfileRefs(request, namespace, "create")
return &v1alpha1.ScanSettingBinding{
TypeMeta: v1.TypeMeta{
Kind: complianceoperator.ScanSettingBinding.Kind,
APIVersion: complianceoperator.GetGroupVersion().String(),
},
ObjectMeta: v1.ObjectMeta{
Name: request.GetScanName(),
Namespace: namespace,
Labels: stackroxLabels,
Annotations: map[string]string{},
},
Profiles: profileRefs,
ScanSettingsName: request.GetScanSettingsName(),
}
}
```
```go
func updateScanSettingBindingFromCentralRequest(scanSettingBinding *v1alpha1.ScanSettingBinding, request *central.ApplyComplianceScanConfigRequest_BaseScanSettings) *v1alpha1.ScanSettingBinding {
ns := scanSettingBinding.GetNamespace()
scanSettingBinding.Profiles = buildProfileRefs(request, ns, "update")
// TODO: Update additional fields as ACS capability expands
return scanSettingBinding
}
```
This keeps all current behavior, logging, and backward compatibility, while centralizing the branching and `NamedObjectReference` construction in one place.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| kind := profileKindToString(ref.GetKind()) | ||
| log.Infof("Profile %q in namespace %q: kind %q provided by Central", ref.GetName(), namespace, kind) | ||
| profileRefs = append(profileRefs, v1alpha1.NamedObjectReference{ |
There was a problem hiding this comment.
suggestion (performance): Profile-level Info logging may be noisy at scale
For larger clusters, logging at Info for every profile on each scan could overwhelm logs. If this is mainly for debugging or initial verification, consider using Debug level, a verbosity flag, or some rate limiting to reduce log noise.
| kind := profileKindToString(ref.GetKind()) | |
| log.Infof("Profile %q in namespace %q: kind %q provided by Central", ref.GetName(), namespace, kind) | |
| profileRefs = append(profileRefs, v1alpha1.NamedObjectReference{ | |
| kind := profileKindToString(ref.GetKind()) | |
| log.Debugf("Profile %q in namespace %q: kind %q provided by Central", ref.GetName(), namespace, kind) | |
| profileRefs = append(profileRefs, v1alpha1.NamedObjectReference{ |
| profileRefs := make([]*central.ApplyComplianceScanConfigRequest_ProfileReference, 0, len(returnedProfiles)) | ||
| for _, p := range returnedProfiles { | ||
| profileRefs = append(profileRefs, ¢ral.ApplyComplianceScanConfigRequest_ProfileReference{ |
There was a problem hiding this comment.
suggestion (bug_risk): Relying on a cast between two enums can be brittle if they diverge
This line relies on central.ComplianceOperatorProfileV2_ComplianceOperatorProfileKind(p.GetComplianceOperatorKind()) preserving matching numeric values between the datastore and proto enums. If either enum changes independently, the wrong kind could be sent. Prefer an explicit mapping (e.g., a small switch) to make the dependency and failure modes explicit.
Suggested implementation:
// Build profile refs from the already-fetched profiles so Sensor can use the correct CO kind
// (Profile vs TailoredProfile) without needing to probe the k8s API.
profileRefs := make([]*central.ApplyComplianceScanConfigRequest_ProfileReference, 0, len(returnedProfiles))
for _, p := range returnedProfiles {
var kind central.ComplianceOperatorProfileV2_ComplianceOperatorProfileKind
switch p.GetComplianceOperatorKind() {
case storage.ComplianceOperatorProfileV2_PROFILE:
kind = central.ComplianceOperatorProfileV2_PROFILE
case storage.ComplianceOperatorProfileV2_TAILORED_PROFILE:
kind = central.ComplianceOperatorProfileV2_TAILORED_PROFILE
default:
// Make dependency and failure mode explicit if enums diverge or an unexpected
// value is stored. Log and skip this profile rather than sending an incorrect kind.
log.Errorf("Unknown compliance operator profile kind %v for profile %q; skipping profile in scan configuration %q",
p.GetComplianceOperatorKind(), p.GetName(), scanRequest.GetScanConfigName())
continue
}
profileRefs = append(profileRefs, ¢ral.ApplyComplianceScanConfigRequest_ProfileReference{
Name: p.GetName(),
Kind: kind,
})
}- This change assumes that:
- The datastore enum type used by
p.GetComplianceOperatorKind()isstorage.ComplianceOperatorProfileV2_ComplianceOperatorProfileKind. - The datastore enum values are
storage.ComplianceOperatorProfileV2_PROFILEandstorage.ComplianceOperatorProfileV2_TAILORED_PROFILE. - The central proto enum values are
central.ComplianceOperatorProfileV2_PROFILEandcentral.ComplianceOperatorProfileV2_TAILORED_PROFILE.
If the actual enum names differ, adjust theswitchcases and assigned central constants accordingly.
- The datastore enum type used by
- Ensure that the
storagepackage is imported in this file (it typically is in this codebase). If it is not, add:
storage "github.com/stackrox/rox/generated/storage"
to the imports block.
| @@ -70,12 +78,26 @@ func convertCentralRequestToScanSetting(namespace string, request *central.Apply | |||
|
|
|||
| func convertCentralRequestToScanSettingBinding(namespace string, request *central.ApplyComplianceScanConfigRequest_BaseScanSettings, _ string) runtime.Object { | |||
There was a problem hiding this comment.
issue (complexity): Consider extracting the duplicated profile reference branching and construction logic into a shared helper function used by both callers to simplify and unify the code.
You can reduce the added complexity by extracting the duplicated branching + construction into a small helper that both call sites use.
For example:
func buildProfileRefs(
request *central.ApplyComplianceScanConfigRequest_BaseScanSettings,
namespace string,
action string, // e.g. "create" or "update"
) []v1alpha1.NamedObjectReference {
refs := make([]v1alpha1.NamedObjectReference, 0, len(request.GetProfiles()))
if profileRefs := request.GetProfileRefs(); len(profileRefs) > 0 {
// New Central: kind is provided directly — no k8s API call needed.
for _, ref := range profileRefs {
kind := profileKindToString(ref.GetKind())
log.Infof("Profile %q in namespace %q: kind %q provided by Central (%s)", ref.GetName(), namespace, kind, action)
refs = append(refs, v1alpha1.NamedObjectReference{
Name: ref.GetName(),
Kind: kind,
APIGroup: complianceoperator.GetGroupVersion().String(),
})
}
return refs
}
// Legacy: old Central without profile_refs — default to Profile kind.
for _, profile := range request.GetProfiles() {
refs = append(refs, v1alpha1.NamedObjectReference{
Name: profile,
Kind: complianceoperator.Profile.Kind,
APIGroup: complianceoperator.GetGroupVersion().String(),
})
}
return refs
}Then both callers become simpler and consistent:
func convertCentralRequestToScanSettingBinding(namespace string, request *central.ApplyComplianceScanConfigRequest_BaseScanSettings, _ string) runtime.Object {
profileRefs := buildProfileRefs(request, namespace, "create")
return &v1alpha1.ScanSettingBinding{
TypeMeta: v1.TypeMeta{
Kind: complianceoperator.ScanSettingBinding.Kind,
APIVersion: complianceoperator.GetGroupVersion().String(),
},
ObjectMeta: v1.ObjectMeta{
Name: request.GetScanName(),
Namespace: namespace,
Labels: stackroxLabels,
Annotations: map[string]string{},
},
Profiles: profileRefs,
ScanSettingsName: request.GetScanSettingsName(),
}
}func updateScanSettingBindingFromCentralRequest(scanSettingBinding *v1alpha1.ScanSettingBinding, request *central.ApplyComplianceScanConfigRequest_BaseScanSettings) *v1alpha1.ScanSettingBinding {
ns := scanSettingBinding.GetNamespace()
scanSettingBinding.Profiles = buildProfileRefs(request, ns, "update")
// TODO: Update additional fields as ACS capability expands
return scanSettingBinding
}This keeps all current behavior, logging, and backward compatibility, while centralizing the branching and NamedObjectReference construction in one place.
|
Images are ready for the commit at f590332. To use with deploy scripts, first |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master-base/gualvare/ROX-31229-tailored-profiles-tracking #19316 +/- ##
=============================================================================================
- Coverage 49.59% 49.58% -0.01%
=============================================================================================
Files 2681 2681
Lines 202308 202349 +41
=============================================================================================
+ Hits 100336 100343 +7
- Misses 94495 94525 +30
- Partials 7477 7481 +4
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:
|
4d040fc to
52d90b6
Compare
Determine whether profile is tailored by calling k8s api from sensor
Revert "Determine whether profile is tailored by calling k8s api from sensor"
Let's look at a different approach.
This reverts commit 0ed1a0a.
Add protos to pass profile kind from central to sensor
Use correct profile kind when creating scanSettingBinding
Description
change me!
User-facing documentation
Testing and quality
Automated testing
How I validated my change
change me!