CCXDEV-16159: controlplanemachinesets gatherer#1294
Conversation
Signed-off-by: Ondrej Pokorny <opokorny@redhat.com>
Signed-off-by: Ondrej Pokorny <opokorny@redhat.com>
Signed-off-by: Ondrej Pokorny <opokorny@redhat.com>
Signed-off-by: Ondrej Pokorny <opokorny@redhat.com>
Signed-off-by: Ondrej Pokorny <opokorny@redhat.com>
openshift-ci-robot
added
the
jira/valid-reference
|
@opokornyy: This pull request references CCXDEV-16159 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: opokornyy The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR adds a new gatherer for collecting OpenShift ControlPlaneMachineSet resources. It includes schema and RBAC configuration, gatherer registration and implementation with sensitive field redaction, comprehensive table-driven tests with fixtures, and documentation with a sample manifest. ChangesControlPlaneMachineSet Gatherer
🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 13 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (13 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@manifests/03-clusterrole.yaml`:
- Line 250: The ClusterRole currently grants the wrong RBAC resource name
"controlplanemachineset" (singular) while the gatherer expects
"controlplanemachinesets" (plural); update the resources list in the ClusterRole
to use "controlplanemachinesets" so the verb rules (get/list/watch) apply to the
plural resource and avoid Forbidden errors when the gatherer queries
controlplanemachinesets. Ensure the exact resource string
"controlplanemachinesets" replaces "controlplanemachineset" in the ClusterRole's
resources section.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 839433e8-30c6-4c2b-a42d-d5f64e4ac534
📒 Files selected for processing (6)
docs/insights-archive-sample/config/controlplanemachinesets/openshift-machine-api/cluster.jsonmanifests/03-clusterrole.yamlpkg/gatherers/clusterconfig/clusterconfig_gatherer.gopkg/gatherers/clusterconfig/const.gopkg/gatherers/clusterconfig/gather_control_plane_machine_sets.gopkg/gatherers/clusterconfig/gather_control_plane_machine_sets_test.go
| - machine.openshift.io | ||
| resources: | ||
| - machinesets | ||
| - controlplanemachineset |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify RBAC resource name matches gatherer GVR resource.
rg -n 'controlplanemachineset|controlplanemachinesets' manifests/03-clusterrole.yaml pkg/gatherers/clusterconfig/const.goRepository: openshift/insights-operator
Length of output: 216
🏁 Script executed:
rg -n 'controlplanemachineset|controlplanemachinesets' manifests/03-clusterrole.yaml pkg/gatherers/clusterconfig/const.goRepository: openshift/insights-operator
Length of output: 216
Fix RBAC resource name mismatch for controlplane machinesets
manifests/03-clusterrole.yaml grants controlplanemachineset (singular), but the gatherer queries controlplanemachinesets (plural). Update the RBAC resource to match to avoid Forbidden errors.
Suggested fix
- - controlplanemachineset
+ - controlplanemachinesets📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - controlplanemachineset | |
| - controlplanemachinesets |
🧰 Tools
🪛 Trivy (0.69.3)
[error] 250-257: Manage all resources
ClusterRole 'insights-operator-gather' shouldn't manage all resources
Rule: KSV-0046
(IaC/Kubernetes)
🤖 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 `@manifests/03-clusterrole.yaml` at line 250, The ClusterRole currently grants
the wrong RBAC resource name "controlplanemachineset" (singular) while the
gatherer expects "controlplanemachinesets" (plural); update the resources list
in the ClusterRole to use "controlplanemachinesets" so the verb rules
(get/list/watch) apply to the plural resource and avoid Forbidden errors when
the gatherer queries controlplanemachinesets. Ensure the exact resource string
"controlplanemachinesets" replaces "controlplanemachineset" in the ClusterRole's
resources section.
|
/retest |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@docs/insights-archive-sample/config/controlplanemachinesets/cluster.json`:
- Around line 11-13: The sample JSON includes a metadata.namespace field but is
intended to be the non-namespaced archive variant; remove the metadata.namespace
entry (the "namespace": "openshift-machine-api" line under metadata) so the
object is truly non-namespaced, or alternatively convert this file into the
namespaced archive variant by keeping metadata.namespace and ensuring it is
placed under the namespaced output layout; specifically update the JSON that
contains "name": "cluster" and the metadata.namespace key to match the chosen
variant.
In `@pkg/gatherers/clusterconfig/gather_control_plane_machine_sets.go`:
- Around line 58-80: The loop is taking the address of the range variable ms
when building record.ResourceMarshaller{Resource: &ms}, which causes every
appended record to point to the same loop variable; fix it by making a local
copy inside the loop (e.g., mscopy := ms), perform the
unstructured.SetNestedField changes against that copy (use mscopy.Object), and
append record.ResourceMarshaller{Resource: &mscopy} to records so each record
references a distinct object instead of the shared range variable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 5ed3de81-5324-48fc-83eb-3257b3630be8
📒 Files selected for processing (4)
docs/gathered-data.mddocs/insights-archive-sample/config/controlplanemachinesets/cluster.jsonpkg/gatherers/clusterconfig/gather_control_plane_machine_sets.gopkg/gatherers/clusterconfig/gather_control_plane_machine_sets_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/gatherers/clusterconfig/gather_control_plane_machine_sets_test.go
| "name": "cluster", | ||
| "namespace": "openshift-machine-api", | ||
| "resourceVersion": "16052", |
There was a problem hiding this comment.
Sample location and object namespace are inconsistent.
This file path represents the non-namespaced archive variant, but metadata.namespace is set. With current gatherer logic, this object would be written under config/controlplanemachinesets/{namespace}/{name} instead.
🧩 Suggested adjustment (if this file is intended to represent non-namespaced output)
- "namespace": "openshift-machine-api",🤖 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 `@docs/insights-archive-sample/config/controlplanemachinesets/cluster.json`
around lines 11 - 13, The sample JSON includes a metadata.namespace field but is
intended to be the non-namespaced archive variant; remove the metadata.namespace
entry (the "namespace": "openshift-machine-api" line under metadata) so the
object is truly non-namespaced, or alternatively convert this file into the
namespaced archive variant by keeping metadata.namespace and ensuring it is
placed under the namespaced output layout; specifically update the JSON that
contains "name": "cluster" and the metadata.namespace key to match the chosen
variant.
| for _, ms := range controlPlaneMachineSets.Items { | ||
| recordName := fmt.Sprintf("config/controlplanemachinesets/%s", ms.GetName()) | ||
| if ms.GetNamespace() != "" { | ||
| recordName = fmt.Sprintf("config/controlplanemachinesets/%s/%s", ms.GetNamespace(), ms.GetName()) | ||
| } | ||
|
|
||
| // remove the sensitive content by overwriting the values | ||
| err = unstructured.SetNestedField(ms.Object, nil, "spec", "templates") | ||
| if err != nil { | ||
| klog.Errorf("unable to set nested field: %v", err) | ||
| errs = append(errs, err) | ||
| } | ||
|
|
||
| err = unstructured.SetNestedField(ms.Object, nil, "spec", "template") | ||
| if err != nil { | ||
| klog.Errorf("unable to set nested field: %v", err) | ||
| errs = append(errs, err) | ||
| } | ||
|
|
||
| records = append(records, record.Record{ | ||
| Name: recordName, | ||
| Item: record.ResourceMarshaller{Resource: &ms}, | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify this exact range-variable-address pattern in the changed file.
rg -n 'for _, ms := range controlPlaneMachineSets.Items' pkg/gatherers/clusterconfig/gather_control_plane_machine_sets.go -A30
rg -n 'Resource:\s*&ms' pkg/gatherers/clusterconfig/gather_control_plane_machine_sets.go -A2 -B2Repository: openshift/insights-operator
Length of output: 1121
Fix address-of-range-variable capture (&ms) that can corrupt per-item payloads
pkg/gatherers/clusterconfig/gather_control_plane_machine_sets.go (lines 58-80) stores record.ResourceMarshaller{Resource: &ms} where ms is the for _, ms := range ... loop variable. In Go, taking the address of the range variable means all appended records can end up referencing the same loop variable (effectively the last element after the loop), producing incorrect/mismatched record payloads.
🔧 Proposed fix
- for _, ms := range controlPlaneMachineSets.Items {
- recordName := fmt.Sprintf("config/controlplanemachinesets/%s", ms.GetName())
- if ms.GetNamespace() != "" {
- recordName = fmt.Sprintf("config/controlplanemachinesets/%s/%s", ms.GetNamespace(), ms.GetName())
+ for i := range controlPlaneMachineSets.Items {
+ ms := &controlPlaneMachineSets.Items[i]
+ recordName := fmt.Sprintf("config/controlplanemachinesets/%s", ms.GetName())
+ if ms.GetNamespace() != "" {
+ recordName = fmt.Sprintf("config/controlplanemachinesets/%s/%s", ms.GetNamespace(), ms.GetName())
}
// remove the sensitive content by overwriting the values
err = unstructured.SetNestedField(ms.Object, nil, "spec", "templates")
if err != nil {
@@
records = append(records, record.Record{
Name: recordName,
- Item: record.ResourceMarshaller{Resource: &ms},
+ Item: record.ResourceMarshaller{Resource: ms},
})
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for _, ms := range controlPlaneMachineSets.Items { | |
| recordName := fmt.Sprintf("config/controlplanemachinesets/%s", ms.GetName()) | |
| if ms.GetNamespace() != "" { | |
| recordName = fmt.Sprintf("config/controlplanemachinesets/%s/%s", ms.GetNamespace(), ms.GetName()) | |
| } | |
| // remove the sensitive content by overwriting the values | |
| err = unstructured.SetNestedField(ms.Object, nil, "spec", "templates") | |
| if err != nil { | |
| klog.Errorf("unable to set nested field: %v", err) | |
| errs = append(errs, err) | |
| } | |
| err = unstructured.SetNestedField(ms.Object, nil, "spec", "template") | |
| if err != nil { | |
| klog.Errorf("unable to set nested field: %v", err) | |
| errs = append(errs, err) | |
| } | |
| records = append(records, record.Record{ | |
| Name: recordName, | |
| Item: record.ResourceMarshaller{Resource: &ms}, | |
| }) | |
| for i := range controlPlaneMachineSets.Items { | |
| ms := &controlPlaneMachineSets.Items[i] | |
| recordName := fmt.Sprintf("config/controlplanemachinesets/%s", ms.GetName()) | |
| if ms.GetNamespace() != "" { | |
| recordName = fmt.Sprintf("config/controlplanemachinesets/%s/%s", ms.GetNamespace(), ms.GetName()) | |
| } | |
| // remove the sensitive content by overwriting the values | |
| err = unstructured.SetNestedField(ms.Object, nil, "spec", "templates") | |
| if err != nil { | |
| klog.Errorf("unable to set nested field: %v", err) | |
| errs = append(errs, err) | |
| } | |
| err = unstructured.SetNestedField(ms.Object, nil, "spec", "template") | |
| if err != nil { | |
| klog.Errorf("unable to set nested field: %v", err) | |
| errs = append(errs, err) | |
| } | |
| records = append(records, record.Record{ | |
| Name: recordName, | |
| Item: record.ResourceMarshaller{Resource: ms}, | |
| }) | |
| } |
🤖 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 `@pkg/gatherers/clusterconfig/gather_control_plane_machine_sets.go` around
lines 58 - 80, The loop is taking the address of the range variable ms when
building record.ResourceMarshaller{Resource: &ms}, which causes every appended
record to point to the same loop variable; fix it by making a local copy inside
the loop (e.g., mscopy := ms), perform the unstructured.SetNestedField changes
against that copy (use mscopy.Object), and append
record.ResourceMarshaller{Resource: &mscopy} to records so each record
references a distinct object instead of the shared range variable.
|
/retest |
The ControlPlaneMachineSet CRD could contain sensitive information in the spec.template and spec.templates fields, so we are removing these fields from the gathered data. Signed-off-by: Ondrej Pokorny <opokorny@redhat.com>
opokornyy
force-pushed
the
CCXDEV-16159-controlplanemachinesets
branch
from
699c023 to
f6f8bbf
Compare
|
Actionable comments posted: 0 |
|
/test insights-runtime-extractor-tests |
1 similar comment
|
/test insights-runtime-extractor-tests |
|
/retest |
2 similar comments
|
/retest |
|
/retest |
|
/test tls-scanner-job |
|
/test tls-scanner |
|
/retest |
|
@opokornyy: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
2 participants
This PR implements a ControlPlaneMachineSet gatherer.
Categories
Sample Archive
docs/insights-archive-sample/config/controlplanemachinesets/openshift-machine-api/cluster.jsonDocumentation
docs/gathered-data.mdUnit Tests
pkg/gatherers/clusterconfig/gather_control_plane_machine_sets_test.goPrivacy
Yes. There are no sensitive data in the newly collected information.
Changelog
Breaking Changes
No
References
https://redhat.atlassian.net/browse/CCXDEV-16092
Summary by CodeRabbit
Release Notes
New Features
Documentation