CCXDEV-16040: Add multicluster gatherer - #1337
Conversation
Add gatherer function for MultiClusterEngine CRs to collect multicluster engine configuration data. Signed-off-by: Ondrej Pokorny <opokorny@redhat.com>
Signed-off-by: Ondrej Pokorny <opokorny@redhat.com>
Signed-off-by: Ondrej Pokorny <opokorny@redhat.com>
|
@opokornyy: This pull request references CCXDEV-16040 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. |
openshift-ci-robot
added
the
jira/valid-reference
📝 WalkthroughWalkthroughChangesMultiClusterEngine gathering
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (13 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
|
||
| ### Sample data | ||
| - [docs/insights-archive-sample/config/revisioned_objects.json](./insights-archive-sample/config/revisioned_objects.json) | ||
| - [docs/insights-archive-sample/config/versioned_object_revision_counts.json](./insights-archive-sample/config/versioned_object_revision_counts.json) |
There was a problem hiding this comment.
This is a left over from #1316 where I forgot to run the make docs after updating doc string
|
[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 |
![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
🧹 Nitpick comments (1)
pkg/gatherers/clusterconfig/gather_multicluster_engine_test.go (1)
103-118: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the NotFound case to the table-driven test.
This standalone test does not use the required table-driven pattern. Add a case with an optional fake-client reactor setup function to the table in
Test_gatherMultiClusterEngine.As per coding guidelines,
**/*_test.go: Use table-driven tests in Go test files`.🤖 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_multicluster_engine_test.go` around lines 103 - 118, Move the CRD-not-found scenario from Test_gatherMultiClusterEngine_CRDNotFound into the existing table-driven Test_gatherMultiClusterEngine cases. Extend the table with an optional fake-client reactor setup function, configure the NotFound list reactor there, and preserve the expected empty records and errors assertions. Remove the standalone test.Source: Coding guidelines
🤖 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 `@pkg/gatherers/clusterconfig/gather_multicluster_engine.go`:
- Around line 48-66: Before constructing each record in
gatherMultiClusterEngine, redact or remove every value at
spec.overrides.components[].configOverrides.deployments[].containers[].env[].value
from the copied resource, then pass the sanitized resource to
record.ResourceMarshaller. Preserve the rest of the MultiClusterEngine data and
add a fixture asserting the environment-variable value is absent from the
archived output.
---
Nitpick comments:
In `@pkg/gatherers/clusterconfig/gather_multicluster_engine_test.go`:
- Around line 103-118: Move the CRD-not-found scenario from
Test_gatherMultiClusterEngine_CRDNotFound into the existing table-driven
Test_gatherMultiClusterEngine cases. Extend the table with an optional
fake-client reactor setup function, configure the NotFound list reactor there,
and preserve the expected empty records and errors assertions. Remove the
standalone test.
🪄 Autofix
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: 2993cbe0-7642-448e-a7f3-01185c09bf13
📒 Files selected for processing (6)
docs/gathered-data.mddocs/insights-archive-sample/cluster-scoped-resources/multicluster.openshift.io/multiclusterengines/engine.jsonpkg/gatherers/clusterconfig/clusterconfig_gatherer.gopkg/gatherers/clusterconfig/const.gopkg/gatherers/clusterconfig/gather_multicluster_engine.gopkg/gatherers/clusterconfig/gather_multicluster_engine_test.go
| func gatherMultiClusterEngine(ctx context.Context, dynamicClient dynamic.Interface) ([]record.Record, []error) { | ||
| mceList, err := dynamicClient.Resource(multiClusterEngineGVR).List(ctx, metav1.ListOptions{}) | ||
| if errors.IsNotFound(err) { | ||
| return nil, nil | ||
| } | ||
| if err != nil { | ||
| klog.V(2).Infof("Unable to list %s resource due to: %s", multiClusterEngineGVR, err) | ||
| return nil, []error{err} | ||
| } | ||
|
|
||
| var records []record.Record | ||
| for i := range mceList.Items { | ||
| item := &mceList.Items[i] | ||
| records = append(records, record.Record{ | ||
| Name: fmt.Sprintf("cluster-scoped-resources/%s/%s/%s", | ||
| multiClusterEngineGVR.Group, | ||
| multiClusterEngineGVR.Resource, | ||
| item.GetName()), | ||
| Item: record.ResourceMarshaller{Resource: item}, |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect ResourceMarshaller and any existing archive sanitization.
ast-grep outline pkg/record --items all
rg -n -C 6 'ResourceMarshaller|MarshalJSON|sanitize|redact|secret' pkg/record pkg/gatherers
# Inspect whether the gatherer or its tests remove sensitive MCE override values.
rg -n -C 6 'configOverrides|containers|env|value|imagePullSecret' \
pkg/gatherers/clusterconfig docs/insights-archive-sampleRepository: openshift/insights-operator
Length of output: 50384
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- record package files ---'
git ls-files 'pkg/record/*'
printf '%s\n' '--- ResourceMarshaller definitions and uses ---'
rg -n -C 8 --glob '*.go' 'type ResourceMarshaller|ResourceMarshaller\{|ResourceMarshaller' pkg/record pkg/gatherers
printf '%s\n' '--- focused clusterconfig references ---'
rg -n -C 8 --glob '*.go' 'gatherMultiClusterEngine|configOverrides|MultiClusterEngine' pkg/gatherers/clusterconfig
printf '%s\n' '--- archive serialization path ---'
rg -n -C 8 --glob '*.go' 'record\.Record|json\.Marshal|yaml\.Marshal|Marshal\(|ResourceMarshaller' pkg | head -n 500Repository: openshift/insights-operator
Length of output: 50384
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
needles = (
"type ResourceMarshaller",
"func (",
"ResourceMarshaller",
"json.Marshal",
"MarshalJSON",
)
for path in Path("pkg").rglob("*.go"):
text = path.read_text(errors="replace")
if "ResourceMarshaller" in text:
print(f"--- {path} ---")
lines = text.splitlines()
for i, line in enumerate(lines, 1):
if any(n in line for n in needles):
lo, hi = max(1, i - 5), min(len(lines), i + 15)
for j in range(lo, hi + 1):
print(f"{j}:{lines[j-1]}")
print()
PYRepository: openshift/insights-operator
Length of output: 50384
Sensitive Data Exposure (CWE-200): Exposure of Sensitive Information to an Unauthorized Actor
Reachability: External · Exploitability: Moderate
Reachability path
● Entry
pkg/gatherers/clusterconfig/clusterconfig_gatherer.go:63
GatherMultiClusterEngine
│
▼
● Sink
pkg/gatherers/clusterconfig/gather_multicluster_engine.go
Redact literal environment-variable values before archiving.
record.ResourceMarshaller does not redact the complete MultiClusterEngine. Remove or redact values under spec.overrides.components[].configOverrides.deployments[].containers[].env[].value before creating the marshaller. Add a fixture that asserts the value is absent.
🤖 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_multicluster_engine.go` around lines 48 -
66, Before constructing each record in gatherMultiClusterEngine, redact or
remove every value at
spec.overrides.components[].configOverrides.deployments[].containers[].env[].value
from the copied resource, then pass the sanitized resource to
record.ResourceMarshaller. Preserve the rest of the MultiClusterEngine data and
add a fixture asserting the environment-variable value is absent from the
archived output.
|
@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
Add gatherer function for MultiClusterEngine CRs
to collect multicluster engine configuration data.
Categories
Sample Archive
docs/insights-archive-sample/cluster-scoped-resources/multicluster.openshift.io/multiclusterengines/engine.jsonDocumentation
docs/gathered-data.mdUnit Tests
pkg/gatherers/clusterconfig/gather_multicluster_engine_test.goPrivacy
Yes. There are no sensitive data in the newly collected information.
Changelog
NoneBreaking Changes
No
References
https://redhat.atlassian.net/browse/CCXDEV-16040
Summary by CodeRabbit
New Features
Documentation
Bug Fixes